Skip to content

fix(server): local threads follow the branch their turn ran on - #129

Merged
NoahHendrickson merged 2 commits into
customfrom
fix/server-local-checkout-branch-follow
Sep 9, 2026
Merged

NoahHendrickson merged 2 commits into
customfrom
fix/server-local-checkout-branch-follow

Conversation

@NoahHendrickson

Copy link
Copy Markdown
Owner

Problem

A local-checkout thread records its branch once, at creation, and nothing rewrites it when the agent runs git checkout -b in the project's shared checkout. The sidebar card kept saying custom while the composer's chip showed the real branch, and the PR the agent opened never linked to the thread, because the PR reactor looks PRs up by the recorded branch.

Upstream's drift-follow (pingdotgg#5159) adopts the checked-out branch only inside a worktree owned by exactly one thread and refuses every shared cwd, since following it for all local threads would hand one PR to every card in the project.

Fix

Keep that refusal for worktree threads and lift it for local threads, but only for the thread whose turn just completed. It adopts the checked-out branch through the existing expectedBranch compare-and-swap dispatch; the other local threads keep their record and the composer's mismatch banner. Detached HEAD, temporary placeholder checkouts, and threads with no recorded branch are still skipped.

Accepted trade-off, recorded in the manifest: a thread that runs a casual turn on a branch another agent left checked out adopts that branch and its PR, and with auto-settle-on-merge it settles when the PR merges. That is factual and reversible, and was chosen over a first-claim ownership rule that would leave the second thread in exactly the stale state this fixes. The record still updates only at turn completion.

Fenced as server-local-checkout-branch-follow with a manifest entry, a reactor test for the local case (including a fenced two-line harness fix so a null worktree option means a local thread instead of defaulting to a worktree), and a fork guard.

Verification

  • CheckpointReactor.test.ts: 33 passed, including the new local-checkout case.
  • Fork guard serverLocalCheckoutBranchFollow.test.ts passes.
  • Server tsc --noEmit clean; fork lint reports no blocking warnings.

Claude Fable 5.1 in Claude Code.

🤖 Generated with Claude Code

A local-checkout thread records its branch once, at creation, and nothing
rewrites it when the agent runs `git checkout -b` in the shared checkout.
The sidebar card kept saying `custom` while the composer showed the real
branch, and the PR the agent opened never linked to the thread because
the PR reactor looks up PRs by the recorded branch.

Upstream's drift-follow (pingdotgg#5159) refuses every shared cwd. Keep that for
worktree threads and lift it for local threads, but only for the thread
whose turn just completed: it adopts the checked-out branch through the
existing compare-and-swap dispatch, and the other local threads keep
their record and their mismatch banner.

Fenced as server-local-checkout-branch-follow with a manifest entry, a
reactor test for the local case, and a fork guard.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Sep 9, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No major structural issues found.

The policy change stays in followWorktreeBranchDrift, the function that already owns checkout adoption. Inverting the shared-cwd refusal so local threads (worktreePath === null) fall through to the existing expectedBranch compare-and-swap is the small model: no second reactor, no ownership registry, no extra snapshot read on the local path. Worktree threads keep the exclusive-cwd check unchanged.

CheckpointReactor.ts stays under 1k. The harness null vs omitted threadWorktreePath fix matches the option's existing type and the three tests that already passed null meaning unset. The fork guard and reactor case cover the invariant without adding a wrapper or a parallel helper.

Open in Web View Automation 

Sent by Cursor Automation: Thermo nuke 4.6

@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.5 KiB −43 B (−0.3%) 15.1 KiB ✅
Codex Thread snapshot wire 7.0 KiB 7.0 KiB +1 B (+0.0%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.5 KiB 6.5 KiB −44 B (−0.7%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 57.1 KiB 57.0 KiB −88 B (−0.2%) 66.4 KiB ✅
Codex Live turn messages 10 8 −2 (−20.0%) 21 ✅
Claude Total thread wire 13.6 KiB 13.5 KiB −36 B (−0.3%) 15.1 KiB ✅
Claude Thread snapshot wire 7.0 KiB 7.0 KiB −2 B (−0.0%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.5 KiB 6.5 KiB −34 B (−0.5%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.8 KiB 57.8 KiB −44 B (−0.1%) 66.4 KiB ✅
Claude Live turn messages 9 8 −1 (−11.1%) 21 ✅

Baseline: 18f1d0b · PR result: 1624536 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.9 KiB
  • Claude decoded thread snapshot: 114.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@NoahHendrickson NoahHendrickson left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review — fix(server): local threads follow the branch their turn ran on

Verdict: the policy change is sound and lands in the right function; one behavioural gap and one vacuous assertion to address. All checks green.

What I verified

  • Inverting the refusal is minimal and correct: worktree threads keep both the exclusive-cwd check and the shared-worktree snapshot read, and the pre-existing it.each case ("does not adopt a drifted checkout … when the worktree is shared by another thread") still exercises that path unchanged.
  • The harness hoist is not cosmetic — it's required. options?.threadWorktreePath ?? cwd would have collapsed the explicit null back to cwd and silently turned the new test into a worktree case. === undefined ? cwd : … is the right shape and matches the option's ?: string | null type.
  • Adoption still goes through upstream's expectedBranch compare-and-swap, and refreshPullRequestAfterTurn re-reads the thread after adoption, so the PR refresh follows the new branch rather than the stale one.

1. A local thread now adopts the default branch when the checkout returns to it

apps/server/src/orchestration/Layers/CheckpointReactor.ts:576

followWorktreeBranchDrift has no isDefaultRef guard. Upstream can afford that: a dedicated worktree rarely sits on main. A shared local checkout sits on the default branch constantly — it is the normal resting state after a PR merges.

Concrete failure: a local thread records fix/foo and has PR #10. The PR merges, the user (or an agent in another thread) runs git checkout custom, then sends a follow-up message in the original thread — "thanks, can you summarise what shipped". The turn completes on custom, and the thread's recorded branch becomes custom. The card and the PR badge lose the association with #10 permanently unless someone notices and checks the branch back out from the composer picker.

Note that the very next function draws exactly this line: refreshPullRequestAfterTurn returns early on input.local.isDefaultRef, and there's a test pinning it ("does not re-ask for the pull request at turn end on the default branch"). Upstream already treats a default-ref checkout as "not this thread's work". I think the local path wants the same treatment:

if (thread.worktreePath === null && input.local.isDefaultRef) {
  return;
}

A local thread has nothing to gain from adopting the default branch — there's no PR there to follow — and it has a real record to lose. If this was a deliberate call, it belongs in the manifest's "accepted consequence" paragraph, which currently only covers adopting another feature branch.

2. expect(other?.branch).toBeNull() proves nothing

apps/server/src/orchestration/Layers/CheckpointReactor.test.ts:1106

The harness creates the second thread with a hardcoded branch: null (CheckpointReactor.test.ts:462), and followWorktreeBranchDrift returns early on thread.branch === null. Thread 2 could never have adopted the drift under any implementation, including one that followed the checkout for every local thread. The assertion passes for the wrong reason.

The isolation claim is the load-bearing half of the intent ("the other local threads keep their record and the composer's 'Branch changed' banner"), so it deserves a real test: give thread 2 a distinct non-null branch — e.g. thread the option through the harness as secondThreadBranch — and assert it is still that branch after the drain.

3. Nit — fence size in an upstream file

The customization is a ~15-line inline conditional inside followWorktreeBranchDrift, so every upstream edit to that function conflicts across it. #124 was refactored for exactly this reason (the branch resolver moved into its own module to shrink the ws.ts fence). Extracting a small predicate — shouldFollowCheckoutDrift({ thread, cwd, sharedThreads }) — would leave a one-line fence here. Optional; the current shape reads fine and the logic is genuinely intertwined with the snapshot read, so I'd only do it if sync friction on this function has bitten before.

Comment thread apps/server/src/orchestration/Layers/CheckpointReactor.ts
Comment thread apps/server/src/orchestration/Layers/CheckpointReactor.test.ts Outdated
…the default

A shared checkout sits on the default branch after every merge, so a follow-up
turn there would have traded the thread's recorded branch and PR for `main`.
Skip adoption on the default ref for local threads, the line
refreshPullRequestAfterTurn already draws. Give the second local thread in the
isolation test a real branch so the assertion can fail.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@NoahHendrickson

Copy link
Copy Markdown
Owner Author

Addressed in 1624536:

  • Local threads no longer adopt the default branch: worktreePath === null && input.local.isDefaultRef returns before adoption, matching the line refreshPullRequestAfterTurn draws. New reactor case covers it, and the manifest's accepted-consequence paragraph now states the exclusion.
  • The isolation test gives thread 2 a real branch via a fenced secondThreadBranch harness option and asserts it survives.
  • Fence size (nit 3): left as is. The condition reads inline with the snapshot read it guards, and this function has not been a sync conflict point yet.

CheckpointReactor.test.ts: 34 passed. Guard green, server tsc clean, fork lint and drift clean.

@NoahHendrickson
NoahHendrickson merged commit da2026e into custom Sep 9, 2026
19 checks passed
@NoahHendrickson
NoahHendrickson deleted the fix/server-local-checkout-branch-follow branch September 9, 2026 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants