Part of #2224 (2/3): rebase pipeline branch against current base_branch before PR open

[jwbron]

Summary

Defense-in-depth guardrail (Phase 4, item 2 of 3) for #2224, follow-on to #2222.

Closes the gap where base_branch advances during the PR phase: before this fix, _auto_create_pr opened the PR against whatever tip origin/<pipeline_branch> had — which could be N commits behind current origin/<base_branch>. _rebase_pipeline_branch_onto_base runs at the start of each phase iteration to clean up stale branch state on resume; nothing between branch-cut and PR-open refreshes against origin/<base_branch>.

base_branch is read-only here. The helper only rewrites pipeline_branch; no commits are ever pushed to base_branch, even when it happens to be main.

Changes

• orchestrator/routes/pipelines.py — new helper _refresh_pipeline_branch_against_current_base:

  1. Fetches fresh refs.
  2. No-ops when origin/<pipeline_branch> is already current with origin/<base_branch>.
  3. Computes the merge-base and rebases via the safe 3-arg --onto form (--onto <new_base> <merge_base>) — the same contamination-safe shape _rebase_pipeline_branch_onto_base uses post-Investigate why pipeline branch ate main commits + carried stale prior-run history #2222.
  4. Force-pushes the rebased tip of pipeline_branch so the open PR's head SHA reflects current base_branch.

  Wired into _auto_create_pr immediately before gateway.create_pr. Best-effort: any failure (rebase conflict, push reject, transient gateway error) restores the worktree to origin/<pipeline_branch>, logs at WARNING, and returns False — the caller still opens the PR against the un-rebased tip. This is intentional: a merge conflict at PR-open time is better surfaced to the human reviewer than swallowed by failing the whole pipeline.

• orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py — 13 tests covering: empty/equal-branch no-ops, fetch failure, missing origin refs, already-current no-op, merge-base failure, reset failure, rebase conflict (abort + restore), push failure (restore + return False), full success path (rebase + push + re-fetch), and an explicit assertion that the rebase argv uses the safe --onto <new_base> <merge_base> form (so a refactor that drops --onto is caught at unit-test time).

User-visible behavior change

The PR head SHA at open time will now reflect the rebase when base_branch advanced during the PR phase. Reviewers will see a head SHA that didn't exist locally during the implement phase. The diff against current base_branch is cleaner; the change to the resulting PR is purely "less divergence", which is the whole point.

Scope

This is only PR 2 of 3 for issue #2224. PR 1 (gateway-side bare-rebase block) is in #2282; PR 3 (divergence OVERSEER_ALERT) is in #2290. Each is independent.

Test plan

• New unit tests pass (13/13).
• No regression in adjacent rebase tests (test_rebase_pipeline_branch.py, test_reconcile_and_push_pr_branch.py).
• ruff check clean.
• CI green.
• Manual: verify a real pipeline run where base_branch advances mid-PR-phase produces a PR with head SHA matching the rebased tip.

[jwbron]

Verified: #2222 regression test (test_rebase_does_not_contaminate_when_base_fetch_silently_failed) still passes on this branch.

[egg-reviewer]

Review summary

I traced the helper end-to-end against _finalize_pr_phase_failed's push_ok=True and push_ok=False paths, against _rebase_pipeline_branch_onto_base for prior-art parity, and against the gateway's push_worktree_branch/fetch_worktree_branch semantics. The logic is correct, the failure-mode story is consistent with the resume-time helper, and the unit tests cover every documented branch (13/13 pass; ruff clean).

The helper's choice of git rebase --onto origin/<base> <merge_base> (with HEAD implicitly being the rebased branch after the step-5 reset) is the right shape: it replays exactly merge_base..origin/<branch> onto current origin/<base>, which is what we want when main has advanced during the PR phase. The merge-base-as-upstream form sidesteps the contamination shape from #2222 cleanly.

The _auto_create_pr integration also correctly defends against a bug-in-helper case with the outer try/except — even though the helper itself swallows everything, the wrap means a future regression that lets an exception escape doesn't block PR creation.

I did not find any blocking issues. A handful of non-blocking suggestions below.

Non-blocking suggestions

1. The "prior art" reference in the rebase comment is misleading

orchestrator/routes/pipelines.py:5886-5888:

# Step 6: Rebase using the safe ``--onto`` form.  The shape is
# exactly the contamination-safe form from #2222 — see
# ``_rebase_pipeline_branch_onto_base`` step 5 for prior art.

_rebase_pipeline_branch_onto_base's actual rebase invocation (line 5660) is the plain form:

rebase = _run_git(["rebase", f"origin/{base_branch}"], timeout=120)

…not --onto. The closest --onto prior art is in gateway_client._build_rebase_cmd at gateway_client.py:2299, but that one is --onto origin/<branch> origin/<base_branch> — the opposite direction (rebase agents' commits onto the remote branch tip, not branch onto base).

The new helper's shape is correct for its purpose (replay branch-only commits on top of fresh origin/<base>), but the comment overstates the symmetry. Recommend either:

• Reword to "this is the same --onto <new_base> <old_base> shape the gateway uses for the reverse direction in _build_rebase_cmd — explicit upstream protects against the bare-form contamination from #2222 by pinning the replay range to merge_base..HEAD", or
• Drop the cross-reference entirely and just explain why merge-base-as-upstream is contamination-safe here.

2. Docstring describes a 3-arg --onto form, code uses 2-arg

orchestrator/routes/pipelines.py:5849-5854:

# Step 4: Compute the merge-base so we can use the safe 3-arg
# ``--onto`` form (``--onto <new_base> <old_base> <branch>``) — the
# form #2222 hardened against contamination.

The code emits the 2-arg form (--onto <new_base> <merge_base>) and relies on HEAD being on the branch (set by the step-5 reset). Functionally equivalent to the 3-arg form, but the docstring/comment should match the code so a reader doesn't grep for a 4th arg that isn't there. The same wording carries into the test docstring at test_refresh_pipeline_branch_at_pr_open.py:208 ("3-arg safe form"). Either:

• Adjust the comment to "2-arg --onto <new_base> <upstream> form (HEAD is the implicit branch after the step-5 reset)", or
• Add the explicit f"origin/{pipeline_branch}" as the third positional in the rebase call. Cleaner argv to read but otherwise no behavior change.

3. test_uses_safe_onto_form only asserts truthiness for "exactly one"

orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py:234:

assert rebase_calls, "expected exactly one rebase invocation"

This passes for len == 1 but also for len > 1. The downstream tail = rebase_calls[0] then ignores the rest. The success-path mock side-effect makes this fine in practice, but if a future change adds a second rebase invocation in the success path (e.g. a pre-flight --show-current-patch debug step), the assertion message becomes a lie. Two-line fix:

assert rebase_calls, "expected at least one rebase invocation"
assert len(rebase_calls) == 1, f"expected exactly one rebase invocation, got {len(rebase_calls)}"

4. Worth a one-line comment about why the resume helper's HEAD-ancestry check is safe to skip here

_rebase_pipeline_branch_onto_base runs the _head_on(...) ancestry check before the step-5 reset --hard origin/<branch> to avoid silently dropping local-only work in three documented worktree states (preserved/fresh/confused HEAD). The new helper's step 5 does the same reset --hard origin/<branch> but without that safety check.

This is correct for the PR-open call site:

• If push_ok=True reached _auto_create_pr, HEAD is on origin/<branch> and the reset is a no-op.
• If push_ok=False, the worktree carries unpushed orchestrator housekeeping commits — but _finalize_pr_phase_failed's docstring already declares those orphan-by-design ("dropping the orchestrator's housekeeping commits rather than failing the whole pipeline").

But that reasoning lives only in the caller's docstring. A reader reading the helper in isolation will wonder why this reset --hard doesn't have the same _head_on guard the resume-time helper does. A one-line comment near step 5 — e.g. "No HEAD-ancestry guard like _rebase_pipeline_branch_onto_base because at PR-open time any local-only work is already orphan-by-design (see _finalize_pr_phase_failed)" — would prevent that confusion.

5. Missing test for the outer try/except defense

The try/except Exception wrap at pipelines.py:7586-7602 exists for "a bug in the helper itself". There's no unit test that injects a raise from the helper and asserts _auto_create_pr still proceeds to gateway.create_pr. Worth one targeted test against _auto_create_pr (or a smaller surrogate) — small belt-and-braces add, and the wrap is asserting a non-trivial invariant that's otherwise only reasoned about in the comment.

What I checked and didn't find an issue with

• No-op when branch is at-or-ahead of base. behind_count == 0 short-circuit covers this; the rev-list direction (origin/<branch>..origin/<base>) is correct.
• Force-push race. If origin/<branch> is concurrently advanced between fetch and force-push we'd clobber it, but the orchestrator-side worktree has a single owner per pipeline phase, and the only writers in this pipeline window (agents) have already finished by PR phase end.
• Race between rebase and concurrent main advance. Rebase replays merge-base..HEAD onto origin/<base> as we saw it at fetch time; if main advances in the milliseconds between fetch and force-push we'd push a slightly-behind tip, which is strictly better than the pre-PR behavior.
• Cherry-pick-skip into an empty rebase. If every branch commit is content-equivalent to one already on origin/<base>, rebase fast-forwards branch to base and the resulting PR shows an empty diff — correct, since there's genuinely nothing to add.
• Boolean return ignored at the call site. Intentional — caller is best-effort.
• pipeline.branch arriving as egg/ prefix; safe in subprocess argv (no shell, list args, gateway-enforced prefix).

— Authored by egg

[egg-reviewer]

Re-review

The only delta on this PR since my prior review at 97be2330d235ed7aa42b5501f95d2c5f0d74583c is the merge commit 63d899c bringing in main (which carried PR #2292 — a 4-line pip install pyyaml step in action/action.yml). That change was already reviewed and merged independently on main; it does not touch this PR's surface.

The two PR-authored files — orchestrator/routes/pipelines.py and orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py — are byte-identical to the previously reviewed tip. No producer response was filed against the five non-blocking suggestions I posted earlier (misleading _rebase_pipeline_branch_onto_base cross-reference, 2-arg vs 3-arg --onto docstring/code mismatch, weak assert rebase_calls in test_uses_safe_onto_form, missing comment on why no _head_on guard at this call site, and the absent unit test for the outer try/except defense), but those were explicitly non-blocking and I am not re-NACKing on them.

Verdict unchanged: no blocking issues. The five suggestions above remain as advisory follow-ups; addressing them in this PR or in a follow-up is at the author's discretion.

— Authored by egg

[egg-reviewer]

Re-review

The only delta on this PR since my prior review at 63d899c3f529f751309aca86265c5503cad577a2 is the merge commit 60c71b3 bringing in main. The two commits that landed on main in that window were:

• ca4191d — docs-only (tester scaffold-first behavior and scripts dir).
• 2f1c1e1 — ruff pin sync to 0.15.12 + formatter sweep across the tree (the new 0.15 default drops parens around except (A, B): per PEP 758, which is why gateway/gateway.py shows except OSError, subprocess.TimeoutExpired: after the merge — valid Python 3.14, just stylistic).

Neither touches this PR's surface. The two PR-authored files — orchestrator/routes/pipelines.py and orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py — are byte-identical to the previously reviewed tip (git diff 63d899c..HEAD -- orchestrator/routes/pipelines.py orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py is empty).

No producer response has been filed against the five non-blocking suggestions from the original review (misleading _rebase_pipeline_branch_onto_base cross-reference at pipelines.py:5886-5888, 2-arg vs 3-arg --onto docstring/code mismatch at pipelines.py:5849-5854 and test_refresh_pipeline_branch_at_pr_open.py:208, weak assert rebase_calls in test_uses_safe_onto_form at line 234, missing comment on why no _head_on guard at this call site, and the absent unit test for the outer try/except defense at pipelines.py:7586-7602). They were explicitly non-blocking and I am not re-NACKing on them.

Verdict unchanged: no blocking issues. The five suggestions above remain advisory; addressing them in this PR or in a follow-up is at the author's discretion.

— Authored by egg

[james-in-a-box]

Addressed all five non-blocking suggestions in a12a153:

1. Misleading prior-art reference (pipelines.py:5894-5901) — reworded to point at gateway_client._build_rebase_cmd (the actual --onto-shape prior art, opposite direction) instead of _rebase_pipeline_branch_onto_base which uses the plain form.
2. 2-arg vs 3-arg --onto docstring mismatch (pipelines.py:5849-5856, test_refresh_pipeline_branch_at_pr_open.py:208-214) — reworded the comment and test docstring to match the 2-arg form the code emits ("HEAD is the implicit branch after the step-5 reset"). Kept the 2-arg shape rather than adding the explicit branch arg, since the reset already pins HEAD.
3. Weak assert rebase_calls (test_refresh_pipeline_branch_at_pr_open.py:235-238) — split into two asserts: truthiness check + explicit len == 1 so a future second rebase invocation is caught with a clear message.
4. Missing comment on absent _head_on guard (pipelines.py:5878-5886) — added a paragraph near step 5 explaining the call-site reasoning (push_ok=True → reset is no-op; push_ok=False → unpushed commits are orphan-by-design per _finalize_pr_phase_failed).
5. Missing test for outer try/except defense (test_auto_pr.py:502-531) — added test_proceeds_to_create_pr_when_refresh_helper_raises: injects a RuntimeError from _refresh_pipeline_branch_against_current_base and asserts gateway.create_pr is still called and the URL returned.

Verified: 53/53 tests pass on the affected modules, ruff clean.

— Authored by egg

[egg-reviewer]

Re-review

The new commit a12a153 addresses all five non-blocking suggestions from the original review at 97be2330. The delta is purely comment + test hardening — no behavioral change to _refresh_pipeline_branch_against_current_base itself.

Verification of the five addressed items

1. Misleading prior-art reference (pipelines.py:5897-5904) — Reworded to point at gateway_client._build_rebase_cmd (gateway_client.py:2299 emits ["rebase", "--onto", f"origin/{branch}", base_ref], which is the same explicit-upstream --onto <new_base> <upstream> shape, just in the opposite direction). Reference is accurate.

2. 2-arg vs 3-arg --onto form (pipelines.py:5849-5856, test_refresh_pipeline_branch_at_pr_open.py:208-214) — Comment and test docstring now correctly describe the 2-arg form with HEAD as the implicit branch (set by the step-5 reset). The argv assertion at test_refresh_pipeline_branch_at_pr_open.py:244-249 still pins tail = ["--onto", "origin/main", merge_base_sha] with len(tail) == 3, so a regression toward the bare form (or toward an unsafe 4th positional) remains caught.

3. Weak assert rebase_calls (test_refresh_pipeline_branch_at_pr_open.py:235-238) — Now split into truthiness check + explicit len(rebase_calls) == 1 with a message that prints the actual count on failure. A future second rebase invocation in the success path now fails loudly instead of being silently dropped by rebase_calls[0].

4. Missing comment on absent _head_on guard (pipelines.py:5878-5886) — Added a paragraph at step 5 explaining why this call site can skip the resume-time helper's ancestry check. The reasoning ("_finalize_pr_phase_failed either left HEAD on origin/<branch> or carries unpushed orchestrator housekeeping commits that are already orphan-by-design per its docstring") matches _finalize_pr_phase_failed's own docstring at pipelines.py:6810-6813 ("dropping the orchestrator's housekeeping commits rather than failing the whole pipeline"). A reader reading the new helper in isolation no longer has to chase the missing guard.

5. Missing test for outer try/except defense (test_auto_pr.py:502-530) — test_proceeds_to_create_pr_when_refresh_helper_raises injects a RuntimeError from _refresh_pipeline_branch_against_current_base via patch(..., side_effect=RuntimeError(...)), then asserts both that mock_refresh.assert_called_once() (the wrap is engaged) and that gateway.create_pr is still called and its URL returned. The non-trivial invariant the wrap was asserting in pure prose is now mechanically pinned.

What I checked on the new code

• The new test mocks the helper at the routes.pipelines._refresh_pipeline_branch_against_current_base symbol (the attribute lookup at the call site), so the patch correctly intercepts. Verified by the test passing as part of the full 53/53 suite.
• The two-line assert split in test_uses_safe_onto_form passes; the message-on-failure pattern is consistent with the rest of the file.
• Comment updates in pipelines.py Steps 4, 5, and 6 do not change argv, control flow, or logged fields. No semantic risk.
• Re-ran the full affected-modules suite locally: 53/53 pass.

Verdict: no blocking issues, no remaining suggestions. All previous review feedback has been addressed correctly.

— Authored by egg

[egg-reviewer]

Re-review

The only delta on this PR since my approval at a12a153 is one new commit, ca3f28f ("docs(#2291): use base_branch over main in pr-open rebase comments"). The change is purely documentation — git diff a12a153..ca3f28f touches only comments and docstrings in orchestrator/routes/pipelines.py (three blocks: helper docstring, step-4 rebase comment, _auto_create_pr callsite comment) and the module docstring of orchestrator/tests/test_refresh_pipeline_branch_at_pr_open.py. No code, control flow, argv, logged fields, or test assertions changed.

Verification

1. Rename is accurate. The substitution of main → base_branch in the comments matches the actual parameter name the helper uses (_run_git(["merge-base", f"origin/{pipeline_branch}", f"origin/{base_branch}"], ...)). Calling it main in prose was misleading because the helper handles whatever base the pipeline was configured against; main is the common case but not the only one.

2. The "read-only" invariant call-outs are correct. Both the helper docstring and _auto_create_pr's comment now state explicitly that only pipeline.branch is rewritten and base_branch is never modified or pushed to. Tracing the helper: the only force-push targets pipeline_branch (step 7) and the only git rebase invocation replays branch commits onto origin/<base_branch> without touching the remote base. Invariant holds.

3. Step-4 comment update is accurate. no base-branch commits get absorbed into the branch's linear history correctly describes what the merge-base-as-upstream form prevents — the same contamination shape #2222 hardened against, regardless of what base_branch happens to be.

4. Test docstring update. The module docstring's substitution does not change any test behavior; the assertions still pin tail = ["--onto", "origin/main", merge_base_sha] (i.e. main is still the literal value used by the test fixture, which is correct — the tests are written against base_branch="main" as the test scenario).

Verdict: no blocking issues. The doc-only delta is a clean improvement and is safe to merge as-is. Note the PR is already merged at ca3f28f; this re-review is for the record.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.