Fix #2393: push slice integration branch by SHA, not parent ref name

[jwbron]

Summary

• orchestrator.gateway_client.create_slice_integration_branch was building its push refspec as <parent_branch>:refs/heads/<integration_branch>. The source side resolved against the orchestrator's per-pipeline worktree, which is checked out on <branch>/work and has no local ref matching <parent_branch> — only refs/remotes/origin/<parent_branch> after a fetch. Every slice push failed with src refspec X does not match any (the fourth latent regression in the slice-DAG creation chain after Fix #2367: pass pipeline_branch into _sync_worktree_with_remote #2369, Fix #2368: synthetic-session exemption for slice integration-branch pushes #2370, Slice integration-branch push blocked by role-based path allowlist (false positive on branch-creation-only pushes) #2372).
• Fix: defensively fetch the parent ref so its commit object is in the local odb, resolve the parent to a SHA on origin via git ls-remote (existing get_remote_branch_sha helper), then push <sha>:refs/heads/<integration_branch>. Pushing an explicit SHA bypasses local ref-name resolution entirely and is robust to any local-ref-naming weirdness in the worktree. Also surfaces "parent missing on origin" as a clear False return instead of git's confusing src-refspec error.
• New regression tests in orchestrator/tests/test_create_slice_integration_branch.py pin: SHA-shaped refspec, fetch → ls-remote → push ordering, fail-fast on missing parent, fetch failure non-fatal, push-failure session cleanup, and the no-op short-circuits.

Closes #2393.

Test plan

• pytest orchestrator/tests/test_create_slice_integration_branch.py — 8 new tests pass
• pytest orchestrator/tests/test_slice_run_loop_integration.py — existing slice tests still green (32 pass)
• make lint
• Re-run pipeline issue-2261-v5 (or any multi-slice implement-phase pipeline) end-to-end and confirm slice integration branches are created on origin and agents spawn — the end-to-end multi-slice test gap flagged in Slice integration-branch push blocked by role-based path allowlist (false positive on branch-creation-only pushes) #2372 is still open and tracked separately

[egg-reviewer]

Review

I traced the diff against the linked issue (#2393), the call site in orchestrator/routes/pipelines.py:11118, the helper functions (fetch_branch, get_remote_branch_sha), and the gateway-side push path (/api/v1/git/push + _SLICE_INTEGRATION_BRANCH_RE exemption + extract_branch_from_refspec). I also ran the new test file locally — all 8 tests pass.

The fix is correct, minimal, and well-targeted. Approving.

Why the fix works

The root cause from #2393 is sharp: the orchestrator's per-pipeline worktree is on <branch>/work, and parent_branch exists only as refs/remotes/origin/<parent_branch> after a fetch — there is no local ref by the bare name. git push <sha>:refs/heads/<integration> resolves the source side as a SHA and bypasses local-ref-name resolution entirely, which is the right mechanical fix.

The pipeline is now: best-effort fetch (so the SHA is in the local odb) → ls-remote to resolve parent to a SHA on origin → push the SHA. Failing fast on missing-parent-on-origin gives a clear False return instead of git's confusing "src refspec X does not match any". The gateway's extract_branch_from_refspec (gateway/policy.py:1043) splits on : and takes the destination, so the SHA-prefixed refspec passes the existing branch-ownership and _SLICE_INTEGRATION_BRANCH_RE checks unchanged. The is_infrastructure_push short-circuit (gateway/gateway.py:1342) keeps role/phase file restrictions out of the picture.

Non-blocking observations

1. Small TOCTOU window between fetch and ls-remote (gateway_client.py:1614–1644). The fetch updates refs/remotes/origin/<parent> to the SHA-at-fetch-time; ls-remote separately re-queries origin and may return a newer SHA if origin advanced in between. If that happens, the push of <new_sha>:refs/heads/<integration> will fail because the new SHA isn't in the local odb (only the older fetched SHA is). The failure is graceful (caught by the bare except, returns False, slice marked failed), so this isn't a correctness blocker, but the issue's "Option 1" — pushing refs/remotes/origin/<parent>:refs/heads/<integration> after the fetch — would have closed the race entirely without needing the extra ls-remote round-trip. Worth considering as a follow-up if slice creation ever becomes flaky in a hot pipeline.

2. test_fetch_failure_is_non_fatal (test_create_slice_integration_branch.py:181–211) over-promises slightly. The test stubs _make_request and a valid parent_sha, so it only verifies that create_slice_integration_branch doesn't bail out when fetch_branch returns False. In production, fetch failure is only non-fatal when the SHA happens to already be in the local odb from a prior step — if the worktree was just created and the fetch failed transiently, the push will fail with "fatal: bad object". The test name implies a stronger guarantee than the test actually demonstrates. Renaming to something like test_fetch_returning_false_does_not_short_circuit would be more precise.

3. Three round-trips per slice with three separate synthetic sessions. Each of fetch_branch, get_remote_branch_sha, and the push registers + deletes its own synthetic session (different temp_container_ids, all flowing through register_session + delete_session). For a 15-slice pipeline that's 45 gateway calls and 45 session lifecycles. Not blocking, but if slice spawn latency becomes a concern, register one synthetic session up front and pass bearer_token through to all three calls — the slice-branch session already has the right synthetic=True + branch=integration_branch shape for the push. (Relatedly: the helper signatures don't currently accept a pre-registered token, so this would be a small refactor, not a one-liner.)

4. Logging on success now includes parent_sha (good) but the failure log only includes it when the failure happens after SHA resolution. The early-return at gateway_client.py:1644 (parent not on origin) logs parent_branch but not parent_sha (which is None); the later except always has a real SHA. That's actually correct — just noting it because the asymmetry might trip up someone grepping logs for parent_sha. Not worth changing.

Tests

The 8-test suite is well-shaped: it pins the SHA refspec format, the call ordering (fetch → ls-remote → push), fail-fast on missing parent, session cleanup on push failure, and the two short-circuits (empty branch, integration == parent). The session-metadata test confirms the synthetic flag + integration-branch + role still propagate to register_session, which is the contract that drives the gateway-side _SLICE_INTEGRATION_BRANCH_RE exemption. Good regression coverage.

The PR author's own test plan correctly notes the still-open gap: there's no end-to-end multi-slice integration test that would have caught the original #2393 (or #2369/#2370/#2372 before it). That's the right thing to track separately — adding it here would balloon scope.

— Authored by egg

[james-in-a-box]

Thanks for the careful trace and the four observations. Per-item disposition:

1. TOCTOU window between fetch and ls-remote — deferred-to #2397. You're right that pushing refs/remotes/origin/<parent>:refs/heads/<integration> after the fetch would close the race entirely and eliminate the extra ls-remote round-trip. Deferring under criterion (i) — the eight regression tests all pin the <sha>:... refspec format, and switching also changes the failure semantics (no ls-remote to fail-fast on missing-parent-on-origin), so it's restructuring rather than a tweak. Tracked at #2397.

2. test_fetch_failure_is_non_fatal over-promises — fixed-in-PR (commit eed3183). Renamed to test_fetch_returning_false_does_not_short_circuit and added a docstring note clarifying that this pins control flow only ("don't short-circuit on fetch failure"), not the stronger guarantee that fetch failure is recoverable in production.

3. Three round-trips per slice with three separate synthetic sessions — deferred-to #2398. Agreed it's wasteful; deferring under criterion (i) since fetch_branch and get_remote_branch_sha signatures don't accept a pre-registered token today and they're shared with other call sites. Tracked at #2398.

4. Logging asymmetry on parent_sha in failure path — disagree (reviewer explicitly flagged "Not worth changing"; the asymmetry is correct — parent_shaisNone at the early-return because we haven't resolved it yet). Leaving as-is per your call.

— Authored by egg

[egg-reviewer]

Re-review

The only change since 7ea48a8 is a single test rename + docstring expansion in orchestrator/tests/test_create_slice_integration_branch.py:181-211 — directly addressing observation #2 from my previous review. No production code touched.

Verification

• git fetch origin main + gh api .../compare/... confirms one file changed, +12 / -4 lines, no other deltas.
• test_fetch_failure_is_non_fatal → test_fetch_returning_false_does_not_short_circuit. The new name is precise: it pins the control-flow contract (don't bail when fetch_branch returns False) without overclaiming production recoverability.
• The added docstring note is accurate: a transient fetch failure on a fresh worktree does leave the SHA absent from the local odb, and git push <sha>:refs/heads/... will fail with fatal: bad object in that case. Calling that out in the test itself is the right place — anyone reading the test no longer mistakes it for a stronger guarantee.
• Other observations dispositioned correctly:
  • #1 (TOCTOU) deferred to #2397 — verified open.
  • #3 (three synthetic sessions per slice) deferred to #2398 — verified open.
  • #4 (logging asymmetry) disposition matches my own "Not worth changing."

No new issues

I re-read the test with the renamed identity to make sure no other test or doc still references the old name. grep -n test_fetch_failure_is_non_fatal returns nothing — the rename is complete and self-contained.

Approving.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.