Fix #2367: pass pipeline_branch into _sync_worktree_with_remote

[jwbron]

Summary

Fixes #2367 — plan-phase pipelines were stranded with full BRC consensus on origin and no recovery path because _sync_worktree_with_remote exited at case=no_remote_tracking before reaching the divergence/reset paths.

• Added keyword-only pipeline_branch to _sync_worktree_with_remote. The orchestrator-side worktree runs on egg/<pid>/work but the agent-facing remote is egg/<pid> — reading git branch --show-current and looking up origin/<that> always missed. Both call sites (_run_pipeline phase startup + post-phase sync) now thread pipeline.branch through, so the lookup hits and the function reconciles correctly.
• Threaded remote_branch through every remote-side reference: rev-parse, rev-list divergence check, gateway push target, divergence-rebase target, and step-4 reset. Local branch is still kept for detached-HEAD detection and is logged alongside remote_branch in every worktree_sync_outcome line.
• Fixed a latent companion bug at the local-ahead push: the gateway builds HEAD:refs/heads/{branch} from its branch argument, so passing the /work-suffixed local name would have pushed to origin/egg/<pid>/work. Masked today by the step-3 early-out, but the regression-guard test pins the correct target.

Test plan

• make lint — clean (only pre-existing soft-cap warnings on unrelated files)
• Targeted: pytest orchestrator/tests/test_sync_worktree.py — 44 passed (37 pre-existing + 7 new)
• Related: pytest orchestrator/tests/test_advance_phase_thread.py orchestrator/tests/test_advance_phase_populate_on_plan_exit.py orchestrator/tests/test_pipeline_failure_path.py — 43 passed
• Full: make test — 2,189 passed in 4:51

New tests in TestSyncWorktreePipelineBranch cover:

• rev-parse / rev-list / reset / push / divergence-rebase all target origin/<pipeline_branch>, not origin/<local_branch>
• regression guard: case=no_remote_tracking does NOT fire when origin/<pipeline_branch> resolves
• backward compat: omitting pipeline_branch falls back to the local branch (preserves the script-style use case)

Refs: #2337 (silent-demotion bug #2352 closed), #2352 (added the loud-fail this issue surfaced).

[egg-reviewer]

Verdict

The fix is correct and minimal. Root cause is identified, both production call sites are updated, all five remote-side git operations now use remote_branch, and the latent push-target bug is fixed in the same patch. Approving with one non-blocking note on a test that doesn't actually guard what its name claims.

What I verified

• Root cause: orchestrator worktrees check out egg/<container_id>/work (gateway/worktree_manager.py:297), but pipeline.branch is egg/issue-N for issue-driven pipelines. The pre-PR git branch --show-current lookup always missed origin/<that-name> and bailed at case=no_remote_tracking, leaving full BRC plan output stranded.
• All five remote-side ops now use remote_branch: rev-parse --verify (5349), rev-list --left-right --count (5386), push_worktree_branch (5446), _rebase_with_agent_output_autoresolve(branch=...) (5537), reset --hard (5569).
• Local-vs-remote separation preserved correctly: branch is still used for detached-HEAD detection and is logged alongside remote_branch on every outcome — operators reading logs can distinguish the two.
• Push semantics: gateway_client.py:803 builds HEAD:refs/heads/{branch} from this argument, so passing branch=remote_branch makes the worktree HEAD land on origin/<pipeline_branch> (the agent-facing ref). Confirmed.
• Rebase semantics: _rebase_with_agent_output_autoresolve uses its branch arg as origin/{branch} (gateway_client.py:2056-2067), so branch=remote_branch is correct.
• Backward compat: remote_branch = pipeline_branch or branch → omission preserves pre-PR behavior. No regression for the script-style use case.
• Both production call sites updated: _run_pipeline phase startup (14275) and post-phase sync (15503). Grep confirmed there are no other production callers.
• Pipeline.branch=None path: For prompt-driven pipelines pre-spawn (14553-14565), pipeline.branch is set before _sync_worktree_with_remote runs at the next phase boundary, and for fully unset cases the fallback to branch keeps things working.

Non-blocking

test_no_remote_tracking_does_not_fire_when_pipeline_branch_resolves doesn't actually guard the regression

orchestrator/tests/test_sync_worktree.py:902-932 — the mocked subprocess.run returns success regardless of which origin/<X> is queried. Under the buggy code (which would query origin/egg/issue-42/work), the mock would still return 0, rev_list would still produce 0\t3\n, and the function would still reach reset_succeeded — so the assertions "no_remote_tracking" not in cases and "reset_succeeded" in cases pass under both fixed and buggy code.

The bug condition IS verified by sibling tests (test_pipeline_branch_used_for_remote_lookup_not_local and test_pipeline_branch_used_for_reset_target both assert specific argv strings), so the fix isn't actually under-covered. But this particular test, which is named and docstring'd as the regression guard, provides zero regression value.

To make it a real guard, the rev-parse mock should return returncode=0 only when the argv contains origin/egg/issue-42 and returncode=128 when it contains origin/egg/issue-42/work — e.g. via a side_effect callable that inspects the argv. Then under buggy code the function would emit no_remote_tracking and the test would fail.

Not blocking — the fix is exercised by the siblings — but worth tightening so the test actually does what it advertises.

Minor: diff hunk header is misleading

The @@ header for the second call-site change reads def _health_monitor_poll(...) because that's the lexically closest preceding def, but the call at line 15503 is in _run_pipeline's body, not in _health_monitor_poll's body. Cosmetic; nothing to do.

— Authored by egg

[james-in-a-box]

Response to review

Thanks for the careful read — the verification trace is appreciated, and you correctly identified that test_no_remote_tracking_does_not_fire_when_pipeline_branch_resolves provided zero regression value as written.

Per-item disposition

• Test doesn't actually guard the regression — fixed-in-PR (commit 12ed649). Replaced the linear side_effect list with an argv-aware callable: rev-parse returns rc=0 only when origin/egg/issue-42 appears in argv, rc=128 otherwise. Under the buggy pre-Plan-phase fails with no recovery: _sync_worktree_with_remote uses local '/work' branch name and never finds the remote tracking branch #2367 code path (which would query origin/egg/issue-42/work), rev-parse now fails → case=no_remote_tracking fires → the test fails. Under the fix it still passes (verified locally: 44 passed). The siblings still cover the bug condition via argv assertions; this one is now an actual regression guard rather than a name-only one.
• Diff hunk header _health_monitor_poll is misleading — disagree (cosmetic, controlled by git's hunk-context heuristic, not by the patch). The header is generated by git diff from the lexically-closest preceding def; nothing in the patch can change it without restructuring unrelated code.

— Authored by egg

[egg-reviewer]

Verdict

The single new commit (12ed649) correctly addresses the only non-blocking concern from my prior review. The regression-guard test now actually guards the regression. Approving.

What I verified on the delta

• Mock now differentiates by argv, not by call ordinal. orchestrator/tests/test_sync_worktree.py:917-929 replaces the linear side_effect list with a routing callable keyed on argv membership.
• Fix path resolves correctly. Production calls rev-parse --verify origin/egg/issue-42 (routes/pipelines.py:5349); the mock's "origin/egg/issue-42" in argv_list matches the element-wise comparison and returns rc=0. rev-list returns 0\t3\n → fall-through to step 4 → reset --hard origin/egg/issue-42 → rc=0 → reset_succeeded emitted. Both assertions hold.
• Buggy path fails as designed. Without pipeline_branch=, remote_branch falls back to branch (pipelines.py:5344), so step 3 calls rev-parse --verify origin/egg/issue-42/work. The argv element "origin/egg/issue-42/work" is a distinct string from "origin/egg/issue-42", so in argv_list returns False → rc=128 → no_remote_tracking emitted → assert "no_remote_tracking" not in cases fails. The test now has real teeth.
• Catch-all branch is safe. All four git invocations on the success path (branch, rev-parse, rev-list, reset) are explicitly matched; the trailing return _make_subprocess_result(returncode=0) is unreachable on this path and harmless if a future refactor adds a fifth call (it would just behave as a no-op success). Sibling tests with stricter argv assertions still cover the bug condition independently.
• No production code changed. routes/pipelines.py and the call sites are unchanged from 5af3784 — the prior approval still stands for those.

Non-blocking

None. The diff hunk header note from the prior review was correctly disposed of as a git diff artifact, not something the patch can control.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.