Feat: One worktree, all its PRs -- subagent pull requests are no longer invisible - #606
Open
cheapsteak wants to merge 26 commits into
Open
Feat: One worktree, all its PRs -- subagent pull requests are no longer invisible#606cheapsteak wants to merge 26 commits into
cheapsteak wants to merge 26 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
🧌 Changes requested This is a large, well-structured PR (multi-PR-per-worktree binding) with a spec that closely tracks the implementation, and most of the design's own claims held up under verification — dedup, tombstones, cap, cross-repo rejection, the ownership-arm merge rule, and the nonisolated binding refresh path all checked out. Two issues need attention before merge, plus a small spec-drift note. HIGH —
|
Adds the CLI surface over the pr.bindings/pr.attach/pr.detach RPC methods from Task 5: `tbd pr list` prints one line per binding, `attach`/`detach` take a bare number, `#number`, or full PR URL, and `bind --from-hook` is the PostToolUse hook entry point that reads a Claude Code tool-result payload from stdin and binds any PR a `gh pr create` reported. `bind` always exits 0 and stays silent on the empty path so it never disturbs the Bash call it observes.
Adds a PostToolUse/Bash entry to the Claude hook overlay that greps the tool payload for `gh pr create` and, only on a match, pipes it to `tbd pr bind --from-hook`. The grep prefilter (-E, not the GNU-only \+ form) keeps every other Bash call across the fleet from spawning a tbd process, and `|| true` guarantees the hook can never fail the tool call it observes. Updates the pre-existing AskUserQuestion PostToolUse-count assertion, which is now one of two entries in that array.
Adds a binding-keyed refresh path beside the existing worktree-keyed one: refreshBindings groups bindings by (host, owner, repo) and reuses numberedPRQuery / parseNumberedPRNodes / mapStateAndReason / fetchCheckSignals, keeping the previous status on any transient failure. fetchAll now also returns its surviving branch matches as ParsedPRURLs so the poll can bind them with source .branch; the worktree-keyed cache, head-ref heals and merged-transition path are untouched. The poll persists each binding's status and writes the worst of a worktree's bindings into Worktree.prStatus, skipping .merged so the merge-while-daemon-down re-fire is preserved.
Auto-archive and auto-hibernate now run behind AllResolvedMergeTrigger: every non-detached binding terminal and at least one merged (PRBinding.allResolved), edge-triggered so a poll fires it once. Un-bound worktrees keep the single-PR behavior through observedMerge. The archive/hibernate precedence and the never-persist-.merged recovery guarantee are unchanged.
A worktree created from a PR row carries `Worktree.prNumber` and got no binding, so its PR was invisible to `tbd pr list`, to the toolbar dropdown and to the status-bar chips — and for a fork PR the stored number is the only handle that exists, because a fork head never appears in the viewer-authored batch and branch matching can therefore never find it. `PRBindingCoordinator.seedProvenance` writes that binding with source `manual`, and the `pr.list` poll reconciles it for every worktree that has a number and no row. Seeding runs on the poll rather than at creation because worktrees that predate bindings need it too, and because a new worktree's `prStatus` is populated by this same poll — so the binding appears exactly when the PR does. The trap the seam exists for: `manual` is the one source permitted to clear a tombstone, so a plain `bind(source: .manual)` reconciled every poll would undo a `tbd pr detach` within seconds. `seedProvenance` only ever writes the FIRST row for an identity; anything already on record, tombstone included, is left as it is.
… toolbar row does The multi-PR work shipped three click targets that disagreed. The toolbar's split button and its dropdown rows opened an in-app webview tab (reusing an existing tab for the same URL, ⌘-click for the browser), while the status-bar chips and the `+N` overflow rows went straight to the default browser — not a decision, just the consequence of that logic being `private` to `ContentView`. Hoist it to `AppState.openPR(url:number:worktreeID:inBrowser:)` and point the toolbar and the chips at it. The toolbar's behaviour is unchanged; the chips move to match. `inBrowser` defaults to the ⌘ state at the CALL site, which keeps the modifier check next to the click that carries it and lets tests drive both arms without synthesizing an NSEvent. The reuse rule is the part worth pinning, so it comes out as a pure `AppState.webviewTabIndex(in:showing:)` — 5 new tier-1 tests cover create, reuse, a second distinct PR, per-worktree scoping, and the decision itself. The sidebar row still opens the browser. That predates this work and changing it is out of scope.
…reate" binding a PR Two defects in the PR-binding shared code. The worst-state order contradicted the design: `changesRequested` outranked `blocked`, where the spec's order is checks failing, blocked, changes requested, pending, mergeable, draft. One severity table drives the toolbar icon, the sidebar dot and the `Worktree.prStatus` column, so the inversion was visible in all three. A test named "prefers blocked over pending over mergeable" had the inversion baked into its third assertion, which is how it survived review; it now asserts its own name, pins the blocked / changes-requested pair explicitly, and walks the whole chain. The `gh pr create` gate was a substring match, wrong in both directions. `git log --grep 'gh pr create'` counted as a creation event, so any PR URL in that command's output bound — and an already-merged one satisfies `allResolved`, auto-archiving a worktree that never opened a PR. Meanwhile `gh -R owner/name pr create`, the normal way to target another repo, matched nothing and silently bound no PR. The command is now tokenized: quoted runs stay one word, the string is cut into segments at `;`, `&`, `|` and newline, and a segment counts only when its first word is `gh` (or a path ending in `/gh`) and its subcommand path — flags and their values skipped — is exactly `pr create`. Pragmatic, not a shell parser: an unusual construction fails closed.
…ache Four defects in the multi-PR poll, all about what one pass does in what order. **A heal never reached the binding table.** `refreshBindingGroup` re-queries a stored binding purely by (host, owner, repo, number) and never re-validates it, so a `branch` binding written by an earlier pass survived every heal: the head-ref heal and the cross-repo poisoned-cache heal both cleared and persisted the cache clear while the row kept driving the worktree's icon and, on merge, satisfied `allResolved` and auto-archived a worktree that was merely tracking someone else's PR. `fetchAll` now returns a `PollOutcome` with both what it discovered and what it disproved, and the poll removes the corresponding binding. Only `branch` bindings, and by **hard delete rather than tombstone**. A `hook` binding is direct evidence that this session ran the `gh pr create`, and a `manual` binding is the user's explicit statement; neither may be undone by an inference drawn from branch names. And a heal's evidence can itself be wrong (a stale `repo.defaultBranch`, a momentarily narrower candidate list, a remote pointed away and back), so a tombstone would block the correct binding forever, silently, with no user gesture behind it — while a delete costs at most one poll, because a heal is part of re-discovery and re-derives its verdict every pass. Durability is what a *detach* needs, not a heal. **The `Worktree.prStatus` write compared against a pre-poll snapshot.** `fetchAll` → `apply` → `onStatusPersist` writes that same column mid-pass, so a worst-of-bindings verdict equal to the snapshot was skipped while the column held something else — pinning a green icon over a bound PR whose checks were failing, on every poll. It now compares against the current persisted value. The skip-when-unchanged optimisation is preserved (an idle poll still issues no UPDATE, asserted via `total_changes()`), as is the rule that `.merged` is never written to that column. **`headBranch` and `baseRef` were migrated, encoded, decoded and rendered but never written**, so `tbd pr list` printed an em-dash on every row. `baseRefName` joins the shared node field selection, and a refresh now reports both refs alongside the status; a pass that resolved nothing reports neither, so a `gh` outage cannot blank a branch name. **`observedMerge` was permanently suppressed by `evaluate`.** They shared one once-only set, and nothing re-armed a worktree that had left the bound population — with every binding detached the poll's grouping no longer contains it, so `evaluate` is never called and its re-arm never runs. Each path now owns its own set and clears it on its own condition. Smaller, same batch: - `setDetached` returned `updateAll`'s MATCHED-row count, so `tbd pr detach` on an already-detached PR reported success. The current-state check moved into the predicate. - A bare PR number whose repo cannot be resolved yet — the ordinary state of a worktree seconds after creation — collapsed into the generic "not a PR number or a GitHub PR URL" error. It now reports the `deferredUnknownRepo` deferral, so the CLI says "try again shortly". Every one of the four is mutation-checked: the fix was reverted and the new assertions shown going red.
…l-detached Two user-land-facing corrections to the PR binding feature. The `PostToolUse`/`Bash` hook carried no `timeout`. It is the first TBD hook matching a universally-used tool, so Claude Code's 60 s default would let one wedged daemon socket stall every Bash call every agent in the fleet runs. Three seconds is far more than the happy path needs — the common case is a `grep` that matches nothing and spawns nothing — and a missed binding is re-derived by branch matching on the next poll. The skill text said detaching a stale PR "is also how you unblock auto-archive", which reads as an invitation to detach them all. A worktree with no live bindings falls back to archiving on the next merge it observes (a documented residual of the design), so that gesture does the opposite of what an agent reaching for it intends. The useful advice stays; the text now names the supported way to suppress auto-archive.
…bindings Multi-PR moved every PR surface off `AppState.prStatuses` onto `prBindings`, which silently dropped the toolbar control and the sidebar indicator for a worktree that HAS a persisted status but NO bindings. That is a resting state, not a startup blip: with `gh` unavailable or unauthenticated the daemon still hydrates `Worktree.prStatus`, but no bind can resolve a repo, so the bindings table stays empty forever and the user's last-known PR state just disappears. The same happens transiently on the first launch after upgrade. `PRBindingPresentation.effectiveBindings` now decides in one place — bindings when there are any, else the legacy status lifted into one synthetic binding, else nothing — and the toolbar, the sidebar row and the status-bar chips all read it through `AppState.effectivePRBindings`, so they cannot disagree. The synthetic binding is value-stable (its id is the worktree's UUID, its boundAt a fixed sentinel) because it feeds ForEach identity, the split button's `.id` key and SwiftUI's own view diffing. Also: - Fold `PRStatus.reason` and `headBranch` into `prSplitButtonID`. Both are rendered into every menu row title by `menuRows`, and AppKit materializes that NSMenu once — so "1 check failing" becoming "3 checks failing" under an unchanged `.checksFailed` left the stale title on screen. Two comments and one test asserted the opposite; they were wrong and are corrected. - Run `refreshPRBindings`' per-worktree fetches concurrently in a task group and apply them as one batch, instead of 40 serial round trips per poll on a 40-worktree fleet. A failed fetch still keeps the previous value; an empty result still drops the key. - Add the missing explicit `privacy:` on that path's logger call.
… stop heredoc bodies opening the bind gate Two second-round review findings on multi-PR bindings. H1 — the all-resolved trigger judged only the SET of bindings, never whose work they were. A hook binding is spared the head-ref heal and refresh re-queries it by number, so a PR a subagent opened on its own branch (or one opened by `cd ../other-worktree && gh pr create`, which binds to the CURRENT worktree) stayed attached forever. With that as a worktree's only binding, its merge fired auto-archive on a worktree whose own branch never had a PR and whose agent was still running — something the single-PR path could never do, because no branch matched and no status ever moved. That invalidated the "strictly more conservative" argument the feature ships unflagged on. The trigger now additionally requires at least one MERGED binding to be the worktree's own work: its head branch is one of the worktree's branch candidates (`PRStatusManager.candidatesFor` — the same derivation the matcher and the heal share), or its number is the worktree's `Worktree.prNumber`. The number arm is required, not belt-and-braces: a fork PR row's head branch belongs to the fork and matches nothing local. A merged binding with no observed head branch satisfies neither arm — unknown holds the gate shut, as a nil status already does. The rule is a pure function beside `allResolved`; the poll passes the two facts in. H4 — the `gh pr create` gate split on newlines, so a heredoc BODY line reading `gh pr create …` opened it and any PR URL in the command's output bound. The tokenizer now skips heredoc bodies to their terminator (quoted, unquoted, and `<<-` with a tab-indented one), and keeps failing closed: an unterminated heredoc, or a `<<` that is not one, loses a bind rather than inventing one. Tests: 7 new trigger scenarios (own-branch merge fires; a foreign-branch subagent merge does not; own + open, own + merged, own + closed; provenance number; unobserved head branch; push-branch candidate), 4 pure ownership-rule tests, 6 heredoc extractor tests. Mutation-checked: dropping the ownership conjunct reddens exactly the two tests that assert it and nothing else. Existing `AllResolvedTriggerTests` cases encoded the old weaker trigger — they bound PRs with no head branch at all — so the harness now binds on the worktree's own branch by default, which is what those cases always meant.
Five review findings on the multi-PR work. **Detach was invisible to the app (regression from the legacy-status fallback).** `effectiveBindings` fell back to the worktree's cached single `prStatus` whenever the binding list was empty — but "empty" is exactly the state `tbd pr detach` produces on a worktree's last PR, since tombstones are excluded from `pr.bindings` and nothing ever clears `Worktree.prStatus`. The toolbar split button, sidebar dot and status-bar chip kept showing the detached PR indefinitely. The app could not tell "no bindings because binding is impossible" (offline `gh`, the case the fallback exists for) from "no bindings because the user removed them", so the distinction now travels on the wire: `PRBindingsResult.detachedCount`, optional so older responses still decode, populated from one `includeDetached: true` read. A non-zero count suppresses the fallback. **Unbounded concurrent blocking RPCs.** `refreshPRBindings` added one task-group child per worktree — ~40 on a full fleet, every poll — and the comment justifying it was wrong: `DaemonClient.sendRawAsync` escapes actor isolation with `Task.detached` onto a `private nonisolated sendRaw` that opens a fresh socket per request, so nothing serializes them. Nothing interleaves on the wire either, but ~40 blocking `connect`/`recv` loops land on the cooperative pool, bounded only by the 300 s recv deadline. The group is now windowed at 6 via `mapConcurrently`, and the comment describes what actually holds. The documented two-branch contract (a FAILED fetch keeps the previous value, an EMPTY result drops the key) moves into `PRBindingRefresh.merge` and finally has tests. **A lone binding with an unparseable URL was a dead control** — it fell into the several-PR shape while the menu still gated its rows on `count > 1`, so the label read `#412` and nothing anywhere offered that PR. `prPrimaryActionURL` now decides the branch, the menu renders rows whenever there is no primary action, and the tooltip drops its "Open" promise. **`prSplitButtonID` could be forged.** It joined free-text `reason` and `headBranch` with `-` and `|`, and git permits `|` in a branch name; since AppKit materializes the menu once per key, a collision freezes the menu on the stale set. Components are escaped injectively, with an unreachable sentinel for absent values so a literal "nil" stays distinct. **The `+N` overflow chip's wording** implied it listed only the overflowed PRs; it has always listed all of them. Wording fixed, behaviour unchanged. Tests: +26 in TBDAppTests (2017 → 2043), +1 in TBDDaemonTests (2740 → 2741).
…is pass observed Four small review findings on the multi-PR-per-worktree branch. A detached-then-re-attached binding could not fire the merged-transition fan-out again. `AllResolvedMergeTrigger.evaluate` clears its own once-only guard, but the poll never calls `evaluate` for a worktree with no live bindings — so a worktree that left the bound population kept a fired-guard nothing could clear, and `tbd pr attach` was a silent no-op. The poll now reports its whole polled population through `retainBound(polled:bound:)` before the no-bindings early return, so leaving the bound population re-arms the worktree. The doc comment already promised this. The ownership check read a stale `headBranch` for one poll: the refreshed binding handed to the trigger carried the pre-pass refs while the row got the fresh ones. `PRBinding.withObservation(status:headBranch:baseRef:)` replaces `withStatus` as the single fold — nil still means "not observed", never "cleared" — and `RPCRouter.folding(_:onto:)` applies it to both the row and the value the merge rule judges. The fan-out can legitimately fire twice in one pass (the un-bound fallback fires inside `fetchAll`, before the pass creates the binding that `evaluate` then judges), which is harmless only because both coordinators are idempotent. Two regression tests pin that: a doubled fan-out archives once and notifies once, and a doubled fan-out with archive blocked by children parks once and notifies once. Finally, the hook prefilter's cost claim said "one grep" in the spec and in `ClaudeHookOverlay`; the command is `payload=$(cat); printf … | grep -qE …`. Both now say what actually runs — the intent (no `tbd` spawn, no daemon round trip) is unchanged.
…he browser The app only asked for `pr.bindings` on worktrees it already believed had a PR — one with a branch-derived status, an existing binding, a tombstone, or the current selection. A worktree whose only PR was bound by the `gh pr create` hook sits on a branch it never checked out, so it appears in no branch-derived status cache and that target set never named it: the PR stayed invisible until the user selected the worktree once. Measured on a live daemon, `pr.list` returns zero statuses, which collapses the set to the selection alone. A new `pr.bindingsAll` takes no worktree parameter and reports the whole binding table from one indexed read, so the app replaces its published maps wholesale each poll instead of fanning out per worktree. That also gives a poll one outcome rather than N: a failure keeps the previous maps intact, and a worktree absent from a success loses its entry, which is how a detach is observed. The windowed fan-out, its `PRBindingFetchOutcome` fold and the `mapConcurrency` helper it needed all go away as dead code. Separately, the status-bar PR chips and their `+N` overflow menu now open the default browser rather than an in-app webview tab, agreeing with the sidebar row indicator. The toolbar split button and its dropdown rows still route through `AppState.openPR` and still open tabs — the status bar is an at-a-glance strip where a click means "take me to GitHub", while the toolbar is where a PR gets parked as a tab.
cheapsteak
force-pushed
the
multi-pr-per-worktree
branch
from
August 11, 2026 06:06
e22b1ed to
ad08ae3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A worktree can produce more than one pull request — a long session opens one, gets it reviewed, starts the next piece of work on a fresh branch, and opens another; subagents open their own. TBD showed exactly one of them.
This was not merely a display limit. TBD discovered a worktree's PR by matching the worktree's head branch against the viewer's authored PRs, so a subagent's PR — typically on a branch the worktree never had checked out — was invisible by construction, not by omission. No amount of polling would ever find it.
worktree_pull_requesttable (migrationv70). Three discovery sources converge on it: aPostToolUse/Bashhook that binds PRs fromgh pr createoutput, the existing branch matcher, and manualtbd pr attach/detach.#412, click opens the tab, ⌘-click the browser — and becomes a worst-state icon plusN PRsopening a dropdown when there are several. The status bar grows a chip per PR, and the sidebar dot takes the same worst-state rule so the three surfaces cannot disagree.Worktree.prNumber).Why there's no feature flag
The repo convention is that autonomous behavior ships behind a default-off flag. This ships unflagged because the new merge rule is strictly stronger than the one it replaces: identical at one binding, and harder to satisfy at more. The old rule fired when the worktree's own PR merged; the new one fires when its own PR merged and every other PR it opened is finished too.
That claim is load-bearing, so it was tested rather than assumed. Two review rounds found paths where it was false, and both are closed:
branch-source binding too (only that source: ahookbinding is evidence a session created the PR, amanualone is the user's explicit statement).gh pr create, which is not necessarily the worktree whose branch it's on. Without the ownership condition, a worktree whose own branch had no PR at all would tear itself down the moment a subagent's PR merged — something branch matching could never do. That condition is why the rule is now genuinely conservative.Design notes
attachrevives one; the provenance seed re-runs every poll and is barred from reviving.tbd pr attach, a false bind can archive a worktree. Quoted'gh pr create'is an argument;gh -R owner/repo pr createis a command; heredoc bodies are data, not commands. The prefilter means an unrelated Bash call costs acatand agrep, never atbdspawn.Worktree.prStatuskeeps working for existing readers, written with the worst-state status — except when that status is.merged, which would lethydraterestore an already-merged baseline at daemon start and permanently lose a retried auto-archive.Spec:
docs/specs/2026-08-10-multi-pr-per-worktree-design.md. Prior art surveyed before designing: Claude Desktop'sGitHubPrManager(bind-at-creation from tool output, per-sessionprs[], cap 20), vibe-kanban (pull_requeststable, archive waits for all PRs), GitButler (per-branch review cache, resync rather than trust), and Cursor's confirmed head-branch mis-attribution bug.Test plan
ProviderEventsSupervisor,CodexUsageFetcherLifecycle,MarkdownStylesheet/ThemeStore,ArchivedWorktreeSearch/AppearanceDebounce), each verified passing in isolation.swiftlint --strictclean across 696 files.2 PRsand a dropdown listing both; detaching the last one clears the toolbar, chip and sidebar dot; the+Noverflow chip opens; hovering a chip then switching selection doesn't stick the pointing-hand cursor.🤖 Generated with Claude Code
🔀 Multi PR Per Worktree