Fix #2778: detach the per-slice BRC worktree to survive branch contention

[jwbron]

Context

Closes #2778.

For slice-DAG pipelines, the per-slice implement-phase BRC consensus transcript was missing from every slice branch — slice PRs opened without the transcript that approved their code. Issue #2778 traced this to #2758 and listed three candidate failure modes, but couldn't pin the root cause because the observed pipeline's logs had been recycled.

Root cause (pinned)

The affected pipeline (issue-2769) was re-run, and its orchestrator logs were still retained. Both slices failed at the identical step in _commit_slice_brc_history_to_integration_branch:

WARNING orchestrator.pipelines: Per-slice BRC commit: worktree add failed, skipping (#2548)
  slice_id=slice-1 integration_branch=egg/issue-2769/slice-1
  stderr="fatal: 'egg/issue-2769/slice-1' is already used by worktree at '/home/egg/.eg...'"

This is none of #2778's three hypothesized failure modes. The hook materialised its temp worktree with git worktree add -B <integration_branch>. Git refuses to check a branch out in two linked worktrees at once, and the slice's own agent worktrees hold the integration branch checked out for the duration of the slice run — so the hook lost that race. Because the hook is best-effort, the False return was swallowed and the transcript was dropped silently.

The git worktree add call predates #2758, so #2758 didn't create this. But before #2758 the per-slice files were also copied onto work, so a hook failure was masked — the transcript still existed somewhere. #2758 correctly stopped the work copy (to fix the #2755 add/add merge conflict), which turned a long-latent hook bug into outright data loss. That is the "incomplete other half" #2778 describes.

Changes

1. git worktree add --detach instead of -B <integration_branch>. The hook only builds one commit on top of origin/<integration_branch> and pushes HEAD (push_worktree_branch with ref=None sends HEAD:refs/heads/<branch>) — it never needed the local branch ref. A detached worktree claims no ref, so it coexists with whatever worktree holds the branch. The fix is holder-agnostic by design: it does not matter which worktree currently has the branch checked out.
2. Docstring + comments. Step 2 of the hook docstring now explains the detached-worktree rationale. The stale call-site comment claiming "the BRC files remain on the work worktree as a fallback audit trail" is corrected — Fix #2755: stop committing per-slice BRC files to work #2758 removed that fallback, so a hook failure now loses the transcript outright.
3. Regression test (test_worktree_add_is_detached_not_branch_ref) pins that the hook's git worktree add is detached and never passes -B or the bare integration-branch name.

Impact

Per-slice BRC consensus transcripts again land on their slice branches and appear in slice PR diffs. Idempotency and crash re-entry are preserved (a detached worktree re-enters at least as cleanly as -B did). No behavior change for non-slice pipelines.

Test plan

• make test — changeset-aware suite, 2869 passed
• make lint — clean
• New regression test fails against the old -B invocation, passes with --detach

[egg-reviewer]

Review: Fix #2778 — detach the per-slice BRC worktree

Verdict: Approve. No blocking issues. The fix is correct, surgically scoped, and well-tested. A few non-blocking observations below.

Fix correctness — verified end-to-end

I traced the full execution path to confirm --detach is functionally equivalent to -B for everything this hook actually does, and strictly safer for the contention case:

1. The hook never needs the local branch ref. push_worktree_branch is called with ref=None, so the refspec is HEAD:refs/heads/<integration_branch> (gateway_client.py:1090). The destination is named explicitly in the refspec — the source being a detached HEAD vs. a branch tip is irrelevant.
2. _commit_statefiles_to_worktree is branch-agnostic. It runs git read-tree HEAD → git add --force → git diff --cached --quiet → git commit --no-verify (routes/pipelines.py:7634-7710). git commit on a detached HEAD just advances the detached HEAD — no branch ref required.
3. The reconcile path survives detachment. On a non-fast-forward rejection, _reconcile_and_retry_push runs git fetch + _rebase_with_agent_output_autoresolve and retries with the same HEAD:refs/heads/<branch> refspec. git rebase origin/<branch> (or the --onto form) rebases the detached HEAD and leaves it detached at the new tip — the retry push still works. No code path in the reconcile depends on a branch being checked out.
4. --detach claims no ref, so the collision is structurally impossible. git worktree add -B <branch> reset and checked out the local branch ref; git refuses that when the branch is checked out in any other linked worktree. --detach is holder-agnostic — it cannot lose the race regardless of which worktree currently holds the integration branch.

Idempotency / crash re-entry — preserved (slightly improved)

The old docstring claimed -B "re-points the local branch ref so a prior tick that crashed mid-flight can re-enter cleanly." In practice -B would fail re-entry if a crashed tick left a stale worktree still holding the branch (the very error #2778 documents). --detach never claims the ref, so a leftover detached worktree from a crashed tick cannot block a subsequent git worktree add --detach. The commit step remains idempotent (skips on empty stage) and the push remains a no-op fast-forward. The PR's "re-enters at least as cleanly as -B did" is accurate.

Test — exercises the production path, genuine regression

test_worktree_add_is_detached_not_branch_ref calls the real _commit_slice_brc_history_to_integration_branch and spies on the actual subprocess.run argv — it is not a hand-built fixture and not self-seeding. I confirmed it fails against the old -B invocation (assert "--detach" in cmd trips immediately) and passes with --detach. Full file: 22/22 passing.

Documentation corrections — accurate

The stale call-site comment ("BRC files remain on the work worktree as a fallback audit trail") was genuinely wrong — the function's own docstring already states the per-slice files exist only on the integration branch. The correction is consistent with that and with the #2755/#2758 history described in the PR body.

Non-blocking observations

1. push_worktree_branch docstring is now slightly imprecise for this caller. gateway_client.py:1059 describes the ref=None mode as "used when repo_path is a worktree checked out to branch." With --detach the worktree is no longer checked out to the branch — the actual contract is just "push the worktree's HEAD." Functionally fine, but a future reader could be misled into thinking ref=None requires a branch-checked-out worktree. Worth a one-line tweak to that docstring in a follow-up.

2. Sibling hook _open_context_pr_for_pipeline uses the same -B pattern. routes/pipelines.py:10353 runs git worktree add -B <context_branch> ... origin/<context_branch> with the same crash-re-entry rationale this PR just disproved. I checked: it is not subject to the same race, because the context branch (egg/<pipeline_id>/context) is not checked out by any agent worktree — agents check out per-slice integration branches, not the context branch. So -B there is safe and no change is needed. Flagging only so the next person who reads that comment knows the analysis was done.

3. Stale .git/worktrees/ entries from a crash between git worktree add and the finally cleanup still accumulate (the hook never prunes). Pre-existing, out of scope, and --detach makes it strictly less harmful — a stale detached worktree no longer pins a branch ref. Mentioning only for completeness.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review and the end-to-end trace. The verdict was Approve with three non-blocking observations — dispositions below.

Observation 1 — push_worktree_branch ref=None docstring imprecise

fixed-in-PR (commit d7e146f)

Fixed in this PR rather than deferred — it's a one-line accuracy correction directly caused by this PR's change, and bundling it is cheaper than shepherding a follow-up issue. The ref=None paragraph in gateway_client.py no longer claims the worktree must be "checked out to branch"; it now states the contract precisely: repo_path is a dedicated pipeline worktree whose HEAD (attached or detached) is pushed via HEAD:refs/heads/<branch>, and it cites #2778 / the per-slice BRC hook as the detached case. The reconcile-rebase sentence is unchanged — it still depends only on repo_path being a worktree, not on a branch checkout.

Observation 2 — sibling hook _open_context_pr_for_pipeline uses the same -B pattern

disagree (no change needed — reviewer's own analysis confirms it)

The review itself concludes this call site is not subject to the #2778 race: the context branch (egg/<pipeline_id>/context) is never checked out by an agent worktree, so -B there cannot lose the contention race. I confirmed the same. No code change — flagged here only so the disposition is on record.

Observation 3 — stale .git/worktrees/ entries accumulate without prune

disagree (pre-existing, explicitly out of scope)

Pre-existing behavior, unrelated to the -B → --detach change, and as the review notes --detach makes it strictly less harmful — a leftover detached worktree no longer pins a branch ref. No follow-up filed because the reviewer explicitly framed this as out of scope and "for completeness," not as a defer request.

— Authored by egg

[egg-reviewer]

Re-review: Fix #2778 — detach the per-slice BRC worktree

Verdict: Approve. No blocking issues. The single new commit since my last review (d7e146f) addresses the one actionable item from that review and introduces no regressions.

Delta reviewed

Only one commit was added since 8eb2743: d7e146f "Clarify push_worktree_branch ref=None docstring for detached worktrees". It is a docstring-only change to orchestrator/gateway_client.py — no functional code change.

Observation 1 — properly addressed

My prior review's Observation 1 noted the push_worktree_branch ref=None docstring still claimed repo_path must be "a worktree checked out to branch", which became false once the per-slice BRC hook switched to --detach. The new docstring (gateway_client.py:1047-1056) now states the contract precisely:

• ref=None pushes the worktree's HEAD via HEAD:refs/heads/{branch};
• the worktree's HEAD may be on branch or detached (citing #2778 / the per-slice BRC hook);
• the refspec names the destination branch explicitly, so a branch checked out at repo_path is not required.

This is accurate and matches the actual code path: refspec = "HEAD:refs/heads/{branch}" when ref is None (gateway_client.py:1093). The reconcile-rebase sentence is unchanged and still correct — the rebase depends only on repo_path being a worktree, not on a branch being checked out. The ref-is-set paragraph below is untouched and remains correct.

Observations 2 & 3 — dispositions confirmed

The author dispositioned the other two observations as no-change-needed:

• Observation 2 (sibling _open_context_pr_for_pipeline uses -B): my own prior analysis already concluded that call site is not subject to the #2778 race — the context branch is never checked out by an agent worktree. Disagreement is correct; no change needed.
• Observation 3 (stale .git/worktrees/ entries without prune): pre-existing, explicitly flagged out of scope, and --detach makes it strictly less harmful. Correct to defer.

Verification

• git diff 8eb2743..d7e146f confirms the change is confined to the docstring.
• orchestrator/tests/test_per_slice_brc_commit.py: 22/22 passing, including test_worktree_add_is_detached_not_branch_ref which exercises the real _commit_slice_brc_history_to_integration_branch and spies on the actual subprocess.run argv.

The fix from the original commit remains correct as verified in my prior review. Nothing in the new commit changes that assessment. Ready to merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.