Repository navigation
GitHub integration: gh-backed panel with checklist and merge, Solo PR link, and watch_pr - #1325
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a01671039
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <Button | ||
| variant={merge.bypass ? "attention" : "prominent"} | ||
| loading={busy === "merge"} | ||
| onClick={() => void run({ type: "merge", method: confirming })} |
There was a problem hiding this comment.
If the 30-second refresh completes while the confirmation dialog is open, Actions receives the refreshed pull prop but confirming retains only the merge method. Clicking this button then calls run, which uses the new pull.head, so a commit pushed after the dialog opened can be merged without a new confirmation—the exact race expectedHeadOid is intended to prevent. Store the confirmed head/pull snapshot and submit that value, or close the dialog whenever the head changes.
Useful? React with 👍 / 👎.
| autoMergeRequest{mergeMethod enabledBy{login}} | ||
| reviewRequests(first:20){nodes{requestedReviewer{__typename ... on User{login} ... on Team{name} ... on Bot{login} ... on Mannequin{login}}}} | ||
| latestOpinionatedReviews(first:50){nodes{author{login} state}} | ||
| reviewThreads(last:100){nodes{isResolved}} |
There was a problem hiding this comment.
For a PR with more than 100 review threads, this reads only the newest 100 and later reports their unresolved count as the total; if the omitted older page contains an unresolved thread, the checklist can even say “None unresolved” and encourage an unsafe merge. GitHub exposes pageInfo and totalCount on the review-thread connection, so paginate before presenting an exact count or explicitly mark the row incomplete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 9c5f556: the query reads totalCount, and the row says "N unresolved in the latest 100" (pending) when older threads went unread.
| mergeStateStatus reviewDecision viewerCanUpdate viewerCanMergeAsAdmin | ||
| autoMergeRequest{mergeMethod enabledBy{login}} |
There was a problem hiding this comment.
This queries only the general viewerCanUpdate flag, while the UI uses it for Update branch and uses repository-wide autoMergeAllowed for enabling/canceling auto-merge. On cross-fork PRs or for read-only viewers, those controls can therefore be shown even though every click is guaranteed to fail. GitHub documents distinct viewerCanUpdateBranch, viewerCanEnableAutoMerge, and viewerCanDisableAutoMerge PullRequest fields; query and retain them to gate their corresponding controls, as required by the adopted permission-state guidance in DESIGN.md:226-233.
AGENTS.md reference: AGENTS.md:L234-L238
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 9c5f556: Update branch, Merge when ready and Cancel auto-merge are gated on viewerCanUpdateBranch / viewerCanEnableAutoMerge / viewerCanDisableAutoMerge.
| "description": "View GitHub repositories, pull requests, issues, and commits, and merge pull requests.", | ||
| "discovery": { "category": "developer-tools-and-integrations" }, | ||
| "host": { | ||
| "processes": [ | ||
| { | ||
| "id": "graphql", | ||
| "program": "gh", | ||
| "args": ["api", "graphql", "--input", "-"] |
There was a problem hiding this comment.
This manifest now adds signed-in gh access and the panel adds GitHub mutations, but docs/channels.md:11-13 still says the panel reads only the public API and has no account connection, while docs/plugin-architecture.md:273-276 says private GitHub connections are a future capability. Update both documents in this change so setup, platform support, and the security boundary are not documented as the opposite of the shipped behavior.
AGENTS.md reference: AGENTS.md:L76-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 9c5f556: docs/channels.md and docs/plugin-architecture.md now describe the gh-backed reads, actions and the account-wide scope.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Fix the confirmation race, host selection, and review status defects before merging. Additional lifecycle findings are inline.
| <Button | ||
| variant={merge.bypass ? "attention" : "prominent"} | ||
| loading={busy === "merge"} | ||
| onClick={() => void run({ type: "merge", method: confirming })} |
There was a problem hiding this comment.
🤖 [P1] Keep the confirmed head fixed while the dialog is open
Open confirmation on head A, then let a push and the 30-second refresh load head B before clicking Merge. The dialog retains only the method, so this handler sends B as expectedHeadOid even though the confirmation text is unchanged. The guard therefore permits an unseen revision. This was reproduced through the mounted app in Chromium and WebKit with modeled GitHub responses. Snapshot the head and bypass consent when opening confirmation, or invalidate confirmation when the head/base changes.
There was a problem hiding this comment.
🤖 Fixed in 7d7c3f0: the confirmation snapshots head and bypass consent and closes when a refresh changes either; merging sends the snapshotted head. Test: "closes the merge confirmation when a refresh brings a newer push" (fails without the fix).
| { | ||
| "id": "graphql", | ||
| "program": "gh", | ||
| "args": ["api", "graphql", "--input", "-"] |
There was a problem hiding this comment.
🤖 [P2] Pin the GitHub CLI hostname to github.com
The panel accepts github.com URLs, but this gh api graphql invocation omits --hostname github.com. The native process inherits GH_HOST, which gh documents as selecting the target host and its credentials. With an enterprise GH_HOST and a matching owner/repository/PR number, the panel can read or mutate that enterprise PR while presenting github.com links. Pass --hostname github.com explicitly. This is established by the command/environment path; no enterprise mutation was attempted.
There was a problem hiding this comment.
🤖 Fixed in 7d7c3f0 (and the merged watcher command): both manifest entries pass --hostname github.com.
| Changes requested by {names(review.changes)} | ||
| </Row> | ||
| ); | ||
| if (review.decision === "APPROVED" || review.approved.length) |
There was a problem hiding this comment.
🤖 [P2] Preserve outstanding required reviews when an approval exists
For reviewDecision=REVIEW_REQUIRED with one approval and an outstanding code-owner review (or an unmet approval count), review.approved.length selects the green Approved branch and hides the remaining requirement. Both Chromium and WebKit reproduce this. Give the aggregate review decision precedence and show existing approvals as supplementary information; use the approvals-only fallback when the aggregate decision is absent.
There was a problem hiding this comment.
🤖 Fixed in 7d7c3f0: REVIEW_REQUIRED takes precedence and shows approvals alongside, e.g. "Required · approved by wes · waiting on kalvin"; approvals-only stays the fallback when there is no decision.
| alive.current = false; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
🤖 [P3] Restore the liveness flag during effect setup
StrictMode replays setup/cleanup and leaves alive.current permanently false because setup never restores it. Subsequent action completion skips error, busy and confirmation updates: a failed merge stays busy without showing its error. A targeted StrictMode reproduction confirms this development-build failure. Set alive.current = true in setup before returning the cleanup.
There was a problem hiding this comment.
🤖 Fixed in 7d7c3f0: setup sets alive.current = true. The merge-failure test now renders under StrictMode.
| void load(); | ||
| return () => live.current?.abort(); | ||
| }, [load]); | ||
| const watching = typeof loaded === "object" && !!loaded.checklist; |
There was a problem hiding this comment.
🤖 [P3] Stop polling terminal pull requests
!!loaded.checklist remains true for merged and closed PRs, so a visible terminal PR panel continues invoking gh every 30 seconds. A targeted reproduction on a MERGED PR records the initial request plus three more after three intervals. Restrict watching to open/draft states to match the intended refresh lifecycle.
There was a problem hiding this comment.
🤖 Fixed in 7d7c3f0: only open and draft pull requests are read again; the refresh test checks a merged PR stops.
…flags Addresses Codex review on #1325: - The merge dialog keeps the head it was opened at, so a 30s refresh that brings a newer push can't change what gets merged. - Threads says "in the latest 100" when older threads went unread, instead of claiming none are unresolved. - Update branch and enabling/cancelling auto-merge use GitHub's viewerCanUpdateBranch / viewerCanEnableAutoMerge / viewerCanDisableAutoMerge. - docs/channels.md and docs/plugin-architecture.md describe the gh-backed reads and actions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
From kalvinnchau's review on #1325: - The merge confirmation closes when a refresh changes the head or whether the merge bypasses branch rules, so consent always matches what merges. - `gh` is pinned to `--hostname github.com`, so an enterprise GH_HOST can't answer for github.com links. - A required review stays pending when an approval is in, listing approvals alongside who it still waits on. - The actions' liveness flag is restored on effect setup (StrictMode). - Merged and closed pull requests are no longer read again every 30s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
baxen
changed the title
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Reviewed head 964e461e023e51d6694cb5dc6ba7115e87be5949 against base 4ad00a8aeabea1b8ab95e7392574223980c95319. Five actionable findings are inline: one P1, three P2s and one P3. Assessment: 7/10; the trust-boundary and lifecycle/recovery defects keep this below the repository’s 9/10 standard. Small repairs and ordering/recovery tests are described inline.
The earlier fixes for confirmed-head binding, required-review precedence, thread-count truncation and viewer flags are present. The account-wide gh capability is an explicit product decision. The public PR description has no attached images; no disclosure finding was identified in that description or the inspected commit messages.
Validation: source only, using pinned Git objects; no PR code, tests, builds or app launches were run. One hosted-check snapshot showed CI required and the listed automated lanes successful, with Windows native validation skipped. Native merge/auto-merge, real signing/delivery, rendered keyboard/focus recovery, cold/warm sidebar performance and test-cost measurements remain unverified. The PR reports bypassing its local pre-push test hook; hosted success does not establish native acceptance. Test replacement/fail-then-pass evidence and the new chat-icon contribution documentation could be clearer; these are follow-up evidence/documentation notes, not additional demonstrated defects. This is a COMMENT review, not approval or merge authorization.
| lines.push(`CI failed on ${sha} for ${url}:`); | ||
| listed( | ||
| lines, | ||
| snapshot.failed.map((name) => name.slice(0, 80)), |
There was a problem hiding this comment.
🤖 [P1] Keep third-party text out of owner-signed agent wakes
Failed check names come from workflow/job or status-context text, and the feedback branch below also interpolates PR file paths. Both can be controlled by someone other than the owner. watcher.ts:62–71 sends this text through the owner’s message session with an agent mention, promoting external text into a trusted owner message; truncating names to 80 characters does not establish a trust boundary. This contradicts the explicit owner-trust contract in docs/agents.md. Keep the wake to fixed wording, counts, commit IDs and canonical links, and have the agent retrieve names/paths as external data. Update the documentation and add regression coverage asserting that arbitrary names and paths never enter the owner-signed wake.
There was a problem hiding this comment.
🤖 Fixed in 6f19afd. The wake now has only fixed wording, a failed-check count, the head commit (when it is hex) and feedback links that match a GitHub URL shape. Check names, authors and file paths are gone from the wake and from the query. docs/agents.md says so. The new test never puts text others control into the owner's wake passes a hostile head, author, foreign link and link fragment, and fails on the old code.
| ctx.effect(() => { | ||
| const timer = setInterval(() => void poll(), POLL_MS); | ||
| void poll(); | ||
| return () => clearInterval(timer); |
There was a problem hiding this comment.
🤖 [P2] Fence in-flight work when the watcher is disposed
Cleanup stops future intervals but leaves a poll awaiting read() alive. If the plugin is disabled or replaced during that read, the old map still passes the identity check at line 107, so the retired instance continues into wake/settle; even if service access rejects the wake, the catch calls failed() and writes its stale map to localStorage. After quick disable/re-enable this can overwrite the replacement instance’s persisted watches and can duplicate notifications where the old session remains usable. Set a disposed/generation guard during cleanup and check it after awaits and before all message/storage effects, including the tool-call continuation. Cover a deferred read resolving after disposal and replacement.
There was a problem hiding this comment.
🤖 Fixed in 6f19afd. Cleanup sets a disposed flag. The poll returns after its await, the tool call throws after its read, and save/wake/failed do nothing once it is set. The new test leaves a read pending, disposes, lets a replacement stop the watch, then resolves the old read: no wake, and storage stays []. It fails on the old code.
| const limit = AbortSignal.timeout(DEADLINE_MS); | ||
| const stop = () => void process.kill(); | ||
| const abort = signal ? AbortSignal.any([signal, limit]) : limit; | ||
| abort.addEventListener("abort", stop, { once: true }); |
There was a problem hiding this comment.
🤖 [P2] Handle cancellation that arrives while spawn is pending
A panel close or last sidebar unsubscribe can abort the caller while await spawn() is pending. The composite signal is then already aborted when this listener is registered, so no abort event invokes stop; the code still writes the request and awaits process exit. The later 20-second timeout cannot re-abort that composite, leaving a stuck process without this client’s deadline until plugin teardown. Immediately kill and reject when the signal is already aborted after spawn, before writing stdin, while retaining the listener for later cancellation. Extend the test with a deferred spawn that resolves after the caller aborts.
There was a problem hiding this comment.
🤖 Fixed in 6f19afd. If the signal is already aborted after spawn resolves, it kills the process and throws before writing stdin. The listener stays for later aborts. The new test aborts during a deferred spawn and checks kill ran and end did not. It fails (hangs) on the old code.
| try { | ||
| return await readStates(gh, urls, signal); | ||
| } catch (error) { | ||
| if (error instanceof GhUnavailable) usePublic = true; |
There was a problem hiding this comment.
🤖 [P2] Retry authenticated reads after a temporary gh outage
GhUnavailable includes offline/no-answer failures and signed-out credentials, but this permanently sets usePublic for the session cache. If Buzz starts offline or before gh sign-in, reconnecting/signing in never restores authenticated reads: every future refresh skips readStates, and linked private PRs remain unavailable until the session/plugin is recreated. Retry gh on later refreshes (with an appropriate cooldown if needed) and clear the fallback after success. Add a recovery test where the first gh call is unavailable and a subsequent refresh succeeds.
There was a problem hiding this comment.
🤖 Fixed in 6f19afd. GhUnavailable now switches to public data for one public refresh (5 min), not for the whole session. The next refresh tries gh again, and a success goes back to the 60s cadence. The new test fails the first gh call, then succeeds, and checks the next refresh reads with gh. It fails on the old code.
| variant={merge.bypass ? "attention" : "prominent"} | ||
| size="sm" | ||
| disabled={!!busy} | ||
| onClick={() => |
There was a problem hiding this comment.
🤖 [P3] Clear the previous action’s error when opening merge confirmation
On a behind PR where an admin can bypass, a failed Update branch sets the shared error. Clicking Merge then only sets confirming, which moves that update error into the new confirmation’s role="alert" even though no merge has been attempted. A failed draft conversion has the same problem. Clear the error in both merge-opening paths (this button and the alternate-method menu), or associate errors with their action. This keeps error feedback beside the operation that failed, as required by DESIGN.md’s states/feedback guidance.
There was a problem hiding this comment.
🤖 Fixed in 6f19afd. The Merge button and the alternate-method menu items now both go through one confirm(method), which clears the error first. The new test fails Convert to draft, then opens Merge and checks the dialog has no alert. It fails on the old code.
964e461 to
6f19afd
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot lanes.
Follow-up reviewed at head 6f19afd82a69cf0d149c15b525b0fd58a4622f3b, base abdc0e4913952690644e6b645e7ae1152c8ad3be (merge-base 7a68e01c40b900747564d44adb3798a621890c1f). No new actionable findings; 9/10.
All five findings from the previous review are addressed in source:
- Owner-signed wakes omit third-party names, paths and text, retaining counts, validated commit text and filtered GitHub links.
- Disposal fences resumed watcher reads, wakes and storage writes.
- Cancellation during process startup kills gh before stdin is written.
- Solo PR reads retry gh after the five-minute public fallback window.
- Both merge-confirmation entry points clear an earlier action’s error.
The prior GitHub feature source is preserved by the squashed rebase, apart from the shared-main icon change; the new behavior is isolated in the 11-file fix commit. The sidebar contribution remains connected after App’s refactor. Regression cases were inspected, not executed. The public description has no attached images; the linked #1328 sidebar screenshot and the two commit messages were also inspected, with no disclosure finding.
Validation is source-only: no PR code, tests, builds or app launches were run. A single hosted-check snapshot showed the listed checks successful, including CI required; Windows native validation was skipped. Native merge/auto-merge, process termination, signing/delivery, rendered keyboard/focus recovery, both themes/enlarged text and cold/warm sidebar performance remain unverified. Reported local hooks and live acceptance were not independently reproduced. This is a COMMENT review, not approval or merge authorization.
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two previously identified P2 defects remain open, so approval is still blocked. The cancellation finding is nonblocking.
| kinds: [ARTIFACT_KIND], | ||
| "#h": page, | ||
| artifact: "current", | ||
| limit: 1000, |
There was a problem hiding this comment.
🤖 [P2] Keep the session-reader request within its supported limit
This requests 1,000 entries, but the production session reader accepts at most 500. The mismatch prevents the Solo PR reader from reaching its normal lookup and fallback flow in the actual app. Request a supported page size and paginate if more history is needed; add a regression against the real reader contract.
There was a problem hiding this comment.
🤖 Fixed in 1049488. Link reads now ask for each chat's link artifact by #h plus #d (uuidv5 of the chat and type, worked out once per chat). That is 100 chats per read with limit: 500, the reader's cap, which leaves room for concurrent revisions, so no paging is needed. The new test reads links within the session reader's limit goes through the real createRelaySession reader. It fails on the old code because the reader refused the 1000 limit and never queried.
| useState< | ||
| Readonly<{ method: MergeMethod; head: string; bypass: boolean }> | ||
| >(); | ||
| const alive = useRef(true); |
There was a problem hiding this comment.
🤖 [P2] Invalidate merge consent when the base changes
The confirmation snapshot binds the method, head and bypass state but omits the base branch. If the PR is retargeted while confirmation is open and its head and bypass state remain unchanged, the existing confirmation still permits merging into a different destination than the user confirmed. Bind confirmation to the PR/base identity and invalidate it on retargeting.
There was a problem hiding this comment.
🤖 Fixed in 1049488. The confirmation snapshot now includes base, and the dialog only stays open while head, base and bypass all match the latest read. The new test closes the merge confirmation when the pull request is retargeted changes only the base on a refresh. It fails on the old code.
| ]; | ||
| const found = await readAll(urls, signal); | ||
| for (const [i, url] of urls.entries()) | ||
| states.set(url, found[i] ?? "failed"); |
There was a problem hiding this comment.
🤖 [P3] Discard aborted refresh results before updating cached state
Unmounting the last sidebar row or superseding an in-flight refresh aborts its request, but readAll converts cancellation into missing results and this loop marks the PRs failed. Showing a known row again only arms the timer, leaving “Pull request, unavailable” until the next poll. A production createSoloPulls reproduction retained failed state at +59.999 seconds without another gh call; a superseding linked() also marked in-flight PRs failed. Preserve cancellation through readAll and check that the refresh is still current before writing state. Nonblocking.
There was a problem hiding this comment.
🤖 Fixed in 1049488. refresh returns without writing links or states once its signal has aborted, whether it was given up on or replaced (for example by linked()). watch also reads right away when a row comes back with a known link but no state yet. The new test unwatches mid-read, watches again, and checks the row is never marked unavailable and a second read fills it. It fails on the old code.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security) and Groot (product/accessibility) reviews.
Follow-up review of head 104948880a767376112caf746f9025184c83e2b1, base abdc0e4913952690644e6b645e7ae1152c8ad3be, focusing on changes since reviewed 6f19afd82a69cf0d149c15b525b0fd58a4622f3b and their supported callers. Two low-priority correctness findings are inline. The relay read now respects the real 500-event cap and scopes to the expected link artifacts; replacement/unwatch cancellation is guarded, and retargeting hides the current merge confirmation. The inline findings cover remaining deadline and confirmation-lifecycle regressions. These are non-blocking comments; humans decide disposition.
Source review only: no tests, builds, app execution or live merge/auto-merge performed. Added regression tests were read, not run. A single hosted-check snapshot showed completed checks passing, several browser jobs still running and Windows native validation skipped; it does not establish final CI success. Native/browser focus, themes, enlarged text, performance, real relay/GitHub behavior and human acceptance remain unverified. Public PR description and follow-up commit text inspected; no attached images or disclosure findings in those surfaces. This is not an approval or merge authorization.
|
🤖 Pushed 0621297: lower relay and GitHub load for Solo PR links, before merge.
|
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) lanes.
Follow-up at head 0621297cb426f76f30688ed85a1e15a0b10df62e, base abdc0e4913952690644e6b645e7ae1152c8ad3be (merge-base 7a68e01c40b900747564d44adb3798a621890c1f). Reviewed the three-file change since 104948880a767376112caf746f9025184c83e2b1 and its supported callers. Two new P2 recovery findings are inline; assessment 8/10 until these are addressed. The batching and partial GraphQL response handling reduce repeated reads, but initial failure and closed-to-reopened transitions need recovery paths.
The previous review’s deadline and confirmation findings remain unchanged in source; they are not duplicated here. The documented choice to refresh cross-device links on a later window return is preserved. Public PR description and follow-up commit message inspected; no attached images or disclosure finding in those surfaces.
Source-only validation: no tests, builds, PR code or app execution. Regression tests were inspected, not run. One hosted-check snapshot had browser, JavaScript and Rust jobs still running; completed checks were successful except Windows native validation was skipped. Native focus/accessibility, real relay/GitHub recovery, performance and human acceptance remain unverified. These are non-blocking comments, not approval or merge authorization.
0621297 to
7de7088
Compare
|
🤖 Rebased onto #1468 (
|
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord's automated source review via Wes's account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/user-flow) lanes.
Follow-up at head 7de70888c850783330a9148190babe81c397260e, base a6e0c66d8d1eb57c059701623d652a32ee3bfa6c. Compared actual source endpoints against covered 0621297cb426f76f30688ed85a1e15a0b10df62e; the GitHub implementation changes are in watch.ts, watcher.ts and their tests, with the new plugin-wake contract supplied by the stacked base.
One new P2 delivery finding is inline. Source assessment: 8/10 until the oversized-response failure is handled and the previously reported recovery defects are resolved. The prior Solo-link retry and reopened-PR findings, plus the earlier deadline and merge-confirmation findings, remain unchanged in the compared source; their inline comments are not duplicated here.
The owner-signed message path is gone. The shared plugin-wake section explicitly treats its contents as data, and the watcher retains unsent news when synchronous admission fails. The documented thread/Solo-only scope and shared-account 🤖 filtering are preserved as product choices.
Optional for users of earlier unmerged builds: normalize saved v1 pending feedback before calling item.body.trim() (watch.ts:176). Older pending records have no body, so they repeatedly fail until the watch is removed. Defaulting an absent body avoids that failure; this is a pre-merge upgrade concern, not evidence of a released regression. Saved rootless DM/channel watches also need deliberate retirement or migration because the new wake contract cannot deliver them. The rejection wording could clarify that channel-scoped sessions cannot start a watch even when the request appeared in a thread.
Public PR description and follow-up commit message inspected; no attached images or disclosure finding in those surfaces. Source-only: no tests, builds, app launches or PR-code execution; regression tests were read, not run. CI was not assessed in this cycle. Native plugin-wake delivery, live GitHub/relay behavior, performance, rendered error/retry focus and accessibility, themes/widths/enlarged text, and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
… link, and watch_pr The GitHub panel reads with the user's own gh sign-in (public API fallback), replaces the Discussion/Checks tabs with a one-page checklist (checks, review, threads, merge), and merges, enables auto-merge, toggles draft and updates the branch from the panel. A Solo chat can be linked to one pull request (NIP-AR artifact), and its sidebar row shows the PR's state through the new conversation.registerChatIcon contribution. Agents get watch_pr, which polls the PR and wakes the agent in its conversation when CI settles or new feedback arrives, and link_pr. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
- The watch_pr wake, sent as the owner, carries only fixed wording, counts, the head commit and GitHub links: no check names, authors or file paths, which others control. - A watcher whose plugin stopped mid-read no longer wakes agents or saves its stale watches over its replacement's. - gh is killed at once when the caller gave up while it was starting. - Solo PR states go back to gh after it was unavailable, instead of staying on public data for the session. - Opening the merge confirmation clears an earlier action's error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
- Solo link reads ask for each chat's link artifact by #d, 100 chats and at most 500 events per read, within the session reader's cap. At 1000 the real reader refused every read. - The merge confirmation is bound to the base branch too, so retargeting the pull request closes it. - A Solo refresh that was given up on or replaced writes nothing, and a row shown again before its state is known reads at once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
…hange Links were read for every shown Solo chat every minute, by every desktop user, even with none linked. Now each chat's link is read once (rows that appear together share a read, a new row reads only its own), and again when the window comes back after five minutes. The live routes do not carry kind 45010, so there is no subscription to follow. GitHub is polled only while a shown chat links a pull request that can still change; merged and closed ones are read once. GitHub answers an aliased query for the pull requests it can read even when one is missing, so GitHubError keeps that data: an unreadable pull request leaves the shared query and is tried alone every five minutes, instead of every pull request being read alone each minute. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
The watcher wakes the agent through `ctx.agents.wake` in the conversation that called watch_pr, instead of posting a message signed by the owner that mentions it. The agent is told the wake is the plugin's and its text data, so the wake now quotes what GitHub says: failed check names and new comments, each line quoted and clipped, with GitHub's links. The owner's own review comments wake the agent. Agents comment as the same gh account, so a comment of that account that starts with 🤖, as agents' do, is taken as the agent's and wakes nobody. watch_pr needs a thread or a Solo chat, where a wake has somewhere to answer. Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
watch_pr lists only feedback ids, then reads the text of new feedback apart, at most 50 at a time with each body clipped by gh's --jq, so long comments can no longer push the answer past the 1 MiB host limit and cost CI wakes. Inline comments are listed through submitted reviews, so a reply on an older thread is seen however many threads there are. Solo pull requests retry an unread link on the poll after a failed first read, keep polling closed pull requests since they can reopen, and keep what a timed-out refresh read. A merge confirmation that a refresh invalidates is forgotten rather than hidden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
7de7088 to
3e931e3
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency) and Groot (product/accessibility) lanes.
Follow-up at head 3e931e3115d0b5b48afbde325e29772c7a10d834, base d9793a8dcf9052271b0e87365a9c7b189c512fa5. Compared the actual GitHub source endpoints against reviewed 7de70888c850783330a9148190babe81c397260e; the prior feature source is preserved by the rebase and the follow-up changes nine GitHub files plus formatting in a SoloInventory test.
No new actionable findings; source assessment 9/10. All five remaining findings from the prior reviews are addressed in source:
- Refresh deadlines preserve successful reads and record failures while explicit cancellation remains fenced.
- Invalidated merge confirmation is forgotten, so returning to the original base does not reopen it.
- Failed initial Solo link reads receive a bounded retry.
- Closed PRs keep refreshing and can become open again.
- Watch metadata is read separately from feedback bodies; body reads are batched and clipped before the host output boundary, and a failed body read leaves CI processing and later retries intact.
Optional for users of earlier unmerged builds: saved feedback IDs change from database IDs to node IDs without a storage-version change, so old comments can be reported again after upgrading. This is pre-merge compatibility advice, not a released regression. Feedback discovery remains bounded; this review does not establish exhaustive history ingestion.
Public PR description and follow-up commit message inspected; no attached images or disclosure finding in those surfaces. Regression tests were inspected, not executed. Validation is source-only: no PR code, tests, builds or app launches were run. One hosted-check snapshot showed JavaScript, Rust, browser journeys/flake checks and measurements still running; completed checks were successful, with Windows native validation skipped. Native gh clipping and plugin-wake delivery, real merge/auto-merge, relay recovery, rendered keyboard/focus behavior, both themes/enlarged text, performance and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
The longer description wrapped to four lines in short narrow Settings at 200% interface size, taller than the scroll area, so the Settings journey could never show it whole and failed in both engines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account.
Follow-up at head 75c3dd1b4f5fe708201eab498491199aef1bffd3, base d9793a8dcf9052271b0e87365a9c7b189c512fa5, compared directly with reviewed 3e931e3115d0b5b48afbde325e29772c7a10d834. No new actionable findings; 9/10 for this follow-up.
The complete delta is two one-line changes: shorten the GitHub plugin description and update the existing Settings browser journey’s exact-text selector. The test retains its 480×320 viewport, 200% interface size, full-visibility assertions, toggle keyboard/focus checks and responsive-navigation assertions. Settings still presents the description through its existing row/detail owners. The previous feature implementation is unchanged.
This uses the small unchanged-feature follow-up exception: solo source review of this delta, building on the previous four-lane review; no new independent team lanes were run. The public PR description has no attached images; that description and the follow-up commit message were inspected with no disclosure finding.
Validation is source-only: no PR code, tests, builds or app launches were run, and CI was not assessed. The claimed three-line rendering, native/browser layout in both themes, enlarged-text accessibility, error/retry focus and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
Co-authored-by: John Tennant <jtennant@block.xyz> * origin/main: test(settings): match the Usage plugin name in scroll regression (#1488) Let a plugin leave a note for an agent's next turn (#1466) Read goose memory through the shared connection-scoped MemoryReads (#1476) Make Claude's standalone turns honour the runner's deadline signal (#1477) feat(plugins): simplify manual plugin management in settings (#1397) feat(relay-notes): add a folder-based shared notes reader (#1395) feat(buzz-mcp): create and list system channels (#1465) Unify profile headers and polish agent activity controls (#1405) Polish video timestamp chips and open videos with context (#1474) GitHub integration: gh-backed panel with checklist and merge, Solo PR link, and watch_pr (#1325) Estimate Codex costs in Usage with genai-prices (#1458) improve agent activity hierarchy and transcript spacing (#1421) Signed-off-by: Gigi <65cc00ce5da00abcd27be4a87ad29d7ef8431766d769a24d94f004fb8fe07066@buzz.block.builderlab.xyz>
4 participants
What
Five commits on #1468: the feature, the fixes from Wes's review, the fixes from Kalvin's, lower relay and GitHub load for Solo links, then
watch_prwaking throughctx.agents.wake.One GitHub integration for "I don't use the GitHub UI, but I still merge; my agents read the comments and check logs." It folds in what were #1322 and #1328.
gh. The panel reads repos, issues, commits and pull requests with your ownghsign-in, so private org links render. Withoutgh(web, signed out) it falls back to the public API, read-only.watch_pragent tool (was Let agents watch GitHub pull requests for CI and feedback #1322). An agent watches a PR and is woken in its conversation when CI settles or new feedback arrives, with no model calls while waiting.How it works
ghaccess. The manifest declaresgh api graphql --hostname github.com --input -twice: as a host process (graphql, used by the panel, becauseghexits non-zero on GraphQL errors and the reason is only in its output) and as a host command (pull, used by the watcher). The token never leavesgh.--hostnamekeeps an enterpriseGH_HOSTfrom answering for github.com links.Checklist.
An open or draft PR re-reads every 30s while visible. Merged and closed ones don't.
Actions.
expectedHeadOid, so a later push is never merged unseen.viewerCanUpdateBranch/viewerCanEnableAutoMerge/viewerCanDisableAutoMerge.Solo PR link.
buzz.solo.pr,d= uuidv5(channel, type), content{version: 1, url}), always signed by the chat's owner. It is written from the panel's Link to this chat, or by an agent'slink_prtool in its own Solo chat. Unlink writesurl: null. Linking needs desktop (configArtifactWriter), and reading works everywhere.#hand#d(100 chats per read, limit 500, the session reader's cap). Rows that appear together share one read, and a new row reads only its own link. Shown chats read their links again when the window comes back after 5 minutes or more. Links are never read on a timer: the live routes don't carry kind 45010, and adding them would be a shared relay change.ghquery covers every shown linked PR that can still change. It runs every 60s while visible, and only while there is one. Merged and closed PRs are read once. A PR GitHub can't read (deleted, access lost) leaves the shared query, which still answers the rest, and is tried alone every 5 minutes. Withoutgh, public REST every 5 minutes.conversation.registerChatIconcontribution.SoloInventorypasses the bubble asfallback, so nothing changes without a link.watch_pr.watch_pr { url, stop? }remembers the agent and conversation.sha:result, so rerun-to-green countsctx.agents.wake(Let a plugin wake an agent in a conversation (ctx.agents.wake) #1468), not a message. Nothing is posted and nothing is signed as baxen. The harness takes it in that conversation's session, under a<plugin-wake plugin="buzz.github">section. That section says neither the owner nor anyone in the channel wrote it, and that its text is data.>, and each piece is clipped: 20 check names of up to 100 characters, 10 comments of up to 1,000. So the agent rarely needs to runghto see what happened.ghaccount as baxen, so GitHub can't tell the two apart. The rule I picked: a comment from that account wakes the agent unless it starts with 🤖. Agents already prefix their GitHub comments with 🤖, and the tool description tells them to. So baxen's own review comments wake the agent and its own comments don't. An agent that forgets the 🤖 wakes itself once per batch, and the 20-wake limit still ends the watch.stop.Wes's P1, under the new framing. The P1 was that third-party text (check names, file paths) went into a message signed by the owner. Agents trust their owner, so that text spoke with the owner's authority. That message is gone. The wake isn't signed by anyone, and the agent is told it's the plugin's data, not the owner's words. Quoting a check name or a comment now gives it no more authority than the agent reading it with
gh, which it did before. Hostile text is still marked as quoted, line by line, so it can't pass for the wake's own wording, and links that aren't GitHub's are dropped. The test "quotes what others wrote, line by line, and links only GitHub's pages" covers that.FOUNDATION edits (approved by baxen)
src/features/conversation/service.tsx: newchatIconsreader +registerChatIcon, modelled onregisterTabs.src/app/App.tsx: one prop,chatIcons={services.conversation.chatIcons}, passed toPrimarySidebar.Validation
watch_prCI wake: arrived 41s afterCI requiredwent green on a rerun.watch_prfeedback wakes, for Codex's summary and for both reviews on this PR. Each landed ~2 min after the last comment, and was handled by the agent, which produced this push.link_prfrom an agent: the chat's sidebar row showed the PR icon (baxen confirmed).github-checklist.spec.mjs(narrow-pane wrap of the public checklist row, browser-only). Removedgithub-conversation/github-checks(surfaces gone). Updatedgithub-body,github-metadata,external-links,layout. No browser spec for the Solo link: the fixture relay has no NIP-AR support.ghstarts kills it before writing; Solo PR states return toghafter it was unavailable; opening Merge clears an earlier action's error.createRelaySession); retargeting the base closes the merge confirmation; a cancelled or replaced Solo refresh writes nothing, and a row shown again with no state reads at once.ctx.agents.wakewith the thread (or none, in a Solo chat), that a watch outside a thread or Solo chat is refused, and that unsent news is kept while this copy can't run the agent.navigation.test.tsxandread-state.test.tswhile two full suites ran at once, and both files pass alone (57 tests). The GitHub plugin and Agents service tests (369) pass on the pushed head7de70888c.types-and-related-unit-tests. Nothing was skipped.Deferred
watch_prwake throughctx.agents.wake. The earlier live wakes used the owner-signed message.manifest.jsonchanged, so trying this needs a native rebuild.Follow-ups
watch_pr: watches poll every 60s even with the window hidden, with no per-user limit (each costs ~60 GraphQL points an hour).Supersedes #1322 and #1328.
🤖 Generated with Claude Code