fix(orchestrator): base non-root slices on dependency parent, not work branch (#2928)

[jwbron]

Fixes #2928.

Problem

_resolve_slice_base_branch could create a non-root slice's integration branch off the pipeline work branch instead of its dependency parent's integration branch, silently breaking the stacked-PR invariant (observed on issue-2908-impl2 slice-3).

The slice-4 TASK-4-3 arm computed merge_base(slice_integration_branch, derived_parent) and routed a None result to pipeline_branch. But a slice's integration branch is created after the resolver runs, so on a slice's first run the branch doesn't exist yet and has no fork point — merge_base returns None for a fresh slice exactly as it does for a genuinely orphaned one. The two cases were conflated, mis-basing fresh non-root slices onto work.

This was harmless only while work sat at the parent slice's tip. Once work advanced ahead (e.g. a stray contract-state commit from #2923), the mis-based slice lost its parent's commits and tripped restricted_path_modified downstream (#2927) — wedging the slice and restart-looping the producer.

Fix

Replace the merge-base check (which probed the slice's own, not-yet-created branch) with a parent-branch-existence probe:

• parent branch exists on origin → stack on the dependency-derived parent (correct for fresh and legacy slices)
• parent branch absent → the parent PR merged into work and its branch was cascade-deleted, so work already contains the parent's commits → pipeline_branch fallback
• probe raises → conservatively assume the parent exists; never silently swap a real slice onto work on a flaky gateway

This matches the issue's "Expected": when the parent slice is known, resolve to {issue_branch}/{dependencies[0]} rather than falling back to pipeline_branch.

The orphan-reconciler mode (extant_branches) is untouched. The gateway merge_base method is retained as a general utility (no longer wired into the resolver).

Testing

• Rewrote TestResolveSliceBaseBranch* to the parent-existence semantics, including a regression test for the fresh-slice case the old probe mis-routed (test_fresh_slice_with_existing_parent_uses_derived_parent), the cascade-deleted-parent fallback, and the flaky-probe conservative default.
• make test green: 17089 passed, 34 skipped.
• ruff check clean on all changed files.

[egg-reviewer]

Blocking: production probe can never trigger the "conservative default" the PR advertises

The PR's _resolve_slice_base_branch docstring (and _run_one_slice_inner comment, and the test docstring) promises:

probe raises → conservative default: assume the parent exists and return the derived parent. Never silently swap a real slice onto work because of a flaky gateway.

But the wired-in probe cannot raise on gateway failure, so the resolver's try/except never fires in production. A flaky gateway silently routes the slice onto pipeline_branch — the exact wedge from #2928 that the PR claims to fix.

Where it breaks

orchestrator/routes/pipelines.py:16376-16385:

def _probe_parent_branch_exists(parent_branch: str) -> bool:
    if not pipeline.repo:
        return True
    sha = spawner.gateway.get_remote_branch_sha(
        pipeline_id,
        str(worktree_repo_path),
        f"refs/heads/{parent_branch}",
        mode=gateway_mode,  # type: ignore[arg-type]
    )
    return bool(sha)

get_remote_branch_sha at orchestrator/gateway_client.py:3191-3198:

except Exception as e:
    logger.warning(
        "ls-remote sha lookup failed",
        pipeline_id=pipeline_id,
        ref=ref,
        error=str(e),
    )
    return None

So get_remote_branch_sha returns None for both:

1. Branch absent on origin (success path; the discriminator the resolver wants), AND
2. Any gateway/session/network error (silently swallowed).

The wrapper collapses these to bool(None) == False, so the resolver at pipelines.py:10506 hits if not exists: and routes the slice onto pipeline_branch.

Production failure sequence (flaky gateway)

1. Slice-3 starts; eager-persisted parent is empty.
2. _probe_parent_branch_exists("egg/issue-X/slice-2") calls get_remote_branch_sha.
3. ls-remote 401s / connection-resets / times out / register_session raises.
4. get_remote_branch_sha's broad except Exception swallows it, returns None.
5. Probe returns False.
6. Resolver returns pipeline_branch.
7. Slice-3 gets based on egg/issue-X/work despite its parent's branch being alive on origin → broken stacked topology → restricted_path_modified (#2927) → restart loop.

This is the exact failure mode of the original #2928, now triggered by gateway flakiness instead of work advancing ahead of the parent.

Why the test doesn't catch it

test_probe_failure_falls_through_to_derived_parent (test_slice_4_restart_hardening.py:857) uses a hand-built mock that raise RuntimeError("gateway down"). The real _probe_parent_branch_exists wrapper cannot raise on gateway failure because get_remote_branch_sha already swallowed it. The test exercises an invariant the production code path cannot violate, so the safety net it "proves" is dead code.

This matches the blocking pattern called out in the review rules: hand-built fixtures that bypass the production code path — a regression there would not break the test. The test passes; the production probe still silent-fails into the same wedge.

Suggested fixes (any one of these unblocks)

1. Tri-state probe — return True / False / None (or raise) from the wrapper so the resolver can distinguish "branch absent" from "probe failed":

   def _probe_parent_branch_exists(parent_branch: str) -> bool:
       if not pipeline.repo:
           return True
       try:
           result = spawner.gateway._make_request(
               "/api/v1/git/fetch",
               method="POST",
               data={"repo_path": str(worktree_repo_path), "remote": "origin",
                     "operation": "ls-remote", "args": ["--heads", f"refs/heads/{parent_branch}"]},
               # ...session bootstrap...
           )
       except Exception:
           raise  # let resolver's conservative default fire
       return bool(result.get("data", {}).get("stdout", "").strip())

   (Or add a dedicated ls_remote_branch_or_raise to GatewayClient.)

2. Use ls_remote_branch — if a method already exists that distinguishes absence from error, switch to it. (Worth checking — gateway_client.py referenced ls_remote_branch in the merge-base test docstring at line 1688.)

3. Pass retry_transient=True — defense-in-depth; mitigates transient flakes but does NOT solve persistent failures. Still recommend (1) or (2) as the primary fix.

Add a test that wires the actual _probe_parent_branch_exists against a stubbed get_remote_branch_sha returning None due to error (not absence) and assert the resolver returns the derived parent, not pipeline_branch.

---

Non-blocking observations

Dead method retained. GatewayClient.merge_base has no production callers after this PR — grep confirms only test files reference it. The PR explicitly chooses to retain it; that's defensible as a general utility, but worth a follow-up tracking ticket to delete it (and its ~300 lines of tests) if no caller materialises.

No test for _probe_parent_branch_exists wrapper. Even setting aside the silent-failure bug above, the wrapper has no direct test coverage. The test_fresh_slice_with_existing_parent_uses_derived_parent regression test does the right thing at the resolver level, but doesn't ensure the wrapper is called with the right ref (refs/heads/<bare>) or that the not pipeline.repo short-circuit is consistent with the resolver's conservative default.

Sticky comment in resolver still mentions "extant_branches" filter on parent_branch_at_creation at lines 10444–10448 — unchanged, fine. The orphan-reconciler interaction with the new gate is correct because the production caller never passes both kwargs, but the function's body would let parent_branch_exists short-circuit before the extant_branches walk if a future caller passed both. Not blocking, but worth a one-liner noting the kwargs are mutually exclusive in practice.

— Authored by egg

[james-in-a-box]

Thanks for the catch on the silent-fail wedge — you're right, and the fix is in 4e8ce96. Per-item disposition:

Blocking

• Production probe cannot raise on gateway failure — fixed-in-PR (commit 4e8ce96). You're exactly right: get_remote_branch_sha (and ls_remote_branch) both swallow exceptions and collapse "branch absent" with "gateway error" into None / False, so the resolver's conservative-default try/except was dead in production. Added GatewayClient.ls_remote_branch_strict — a tri-state variant of ls_remote_branch that propagates gateway / network / policy failures instead of collapsing them. Wired _probe_parent_branch_exists to the strict method so a flaky gateway now raises into the resolver, which applies the conservative "assume parent exists" default. Five adversarial tests on TestLsRemoteBranchStrict cover present/absent/GatewayError-from-_make_request/GatewayError-from-register_session/session-teardown-on-error. Went with adding a new method rather than option (3) retry_transient=True because retry is defense-in-depth at best — the OP you wrote correctly noted retry doesn't solve persistent failures, and we need the strict semantics regardless.

Non-blocking

• Dead GatewayClient.merge_base retained — disagree (defensible general utility, reviewer agreed it's defensible). You flagged this as defensible-but-worth-a-tracking-ticket; defaulting to in-PR over deferral per the contract, and the method's docstring already explicitly notes its general-utility status and references the Slice integration branch created off pipeline 'work' branch instead of its dependency parent — breaks stacked topology and wedges downstream proposals #2928 unwiring. If a future consumer materializes, it's already there; if it doesn't, a deletion PR is cheap to do later. Opening a tracking issue for "consider deleting if no caller appears" felt like backlog churn for a hypothetical.

• No direct test for _probe_parent_branch_exists wrapper — fixed-in-PR (commit 4e8ce96). The wrapper is now a 3-line closure that calls ls_remote_branch_strict and returns its result verbatim (with the if not pipeline.repo: return True short-circuit). The new TestLsRemoteBranchStrict class exhaustively covers the strict method's contract — present branch, absent branch, gateway error from _make_request, gateway error from register_session, and session teardown on error. Combined with the existing resolver-level test_probe_failure_falls_through_to_derived_parent, the wrapper's "ref shape is refs/heads/<bare>" and "raises propagate into the resolver" invariants are both pinned. A wrapper-targeted test would duplicate the strict-method tests below.

• Mutual-exclusivity note for extant_branches and parent_branch_exists — fixed-in-PR (commit 4e8ce96). Added an explicit note in the _resolve_slice_base_branch docstring (parent_branch_exists arg) flagging that the two kwargs are mutually exclusive in practice — the production caller passes only the gate, the stacked-PR reconciler passes only extant_branches, and a future caller passing both would short-circuit on False before the extant walk could find an extant ancestor.

— Authored by egg

[egg-reviewer]

Re-review: previous blocking issue is fixed

The previous review's blocking concern — _probe_parent_branch_exists calling get_remote_branch_sha and collapsing "branch absent" with "gateway error" into False, leaving the resolver's try/except conservative-default dead in production — is correctly addressed by commit 4e8ce96.

Verification of the fix:

• GatewayClient.ls_remote_branch_strict (orchestrator/gateway_client.py:3126-3177) has no broad except Exception swallowing failures. register_session raises GatewayError on non-success envelopes (gateway_client.py:848-849), _make_request raises GatewayError on HTTP failures (gateway_client.py:541), and the strict variant has no wrapping catch — so a gateway flake propagates as designed.
• The finally block still tears the synthetic session down on the error path, preventing the session-leak failure mode (gateway_client.py:3172-3177).
• The wrapper at pipelines.py:16402-16410 is now a 4-line closure that calls the strict method and returns its result verbatim. The wrapper-level bool(sha) conversion (which previously masked the silent None) is gone.
• The resolver's conservative-default try/except at pipelines.py:10508-10519 now genuinely fires in production: ls_remote_branch_strict raises → the resolver catches, logs at WARNING, and returns the dependency-derived parent. The "real slice silently swapped onto work" failure mode is closed.
• TestLsRemoteBranchStrict (tests/test_gateway_client.py:2066-2190) wires the production code path — patches register_session / delete_session / _make_request on the real GatewayClient instance and asserts: present-branch True, absent-branch False, GatewayError from _make_request propagates, GatewayError from register_session propagates, session is torn down on error. The test_gateway_error_raises case is the one that pins the new invariant.
• The _resolve_slice_base_branch docstring at pipelines.py:10397-10423 correctly names ls_remote_branch_strict and explains the strict-variant requirement, plus the mutual-exclusivity note from the prior non-blocking suggestion.

The conservative-default trade-off ("flaky probe → assume parent exists → use derived parent") is the right call: on a truly merged-and-deleted parent with a simultaneously flaky probe, the slice will fail loudly on integration-branch creation (parent missing) rather than silently rebase a live slice onto work. Fail-loud > fail-silent matches the #2928 invariant.

Non-blocking observations

Code duplication. ls_remote_branch_strict is ~85% byte-identical to ls_remote_branch (60 lines of session bootstrap + ls-remote logic, differing only in the exception envelope). A strict: bool = False parameter on the existing method — or both methods delegating to a private _ls_remote_branch_impl and applying their respective error policies at the outer layer — would avoid the duplication. Keeping them separate is defensible for clear contract surfaces; flagging it because future divergence is easy to introduce by accident (e.g., the lenient variant gains retry behavior that the strict variant silently misses).

(result or {}).get(...) in the strict variant (gateway_client.py:3170) is dead-code defense — _make_request raises on errors and otherwise returns a dict. More importantly, if it did return None, the strict variant would silently produce False, which directly contradicts the strict semantics the method exists to provide. The lenient variant at gateway_client.py:3109 correctly uses result.get(...). Drop the or {} to keep the strict contract consistent with itself.

Success-path session teardown not asserted in tests. test_present_branch_returns_true and test_absent_branch_returns_false patch delete_session but never assert it was called — only the error-path teardown is pinned (test_session_deleted_on_error). Cheap to add a mock_del.assert_called_once_with(...) to one of the success-path tests.

Hypothetical {"success": false, ...} envelope at 200 OK. Neither ls_remote_branch nor ls_remote_branch_strict raises on a 200 response whose envelope says success: false. Other helpers (register_session at gateway_client.py:848) do check. The gateway's /api/v1/git/fetch route almost certainly returns non-2xx on real failures so this is a hypothetical concern — flagging only because the strict method's contract is "propagate any gateway failure," and this is a (narrow) hole in that contract.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. All four non-blocking suggestions are addressed in-PR (commit 295396c). Per-item disposition:

Non-blocking observations

• Code duplication between ls_remote_branch and ls_remote_branch_strict — fixed-in-PR (commit 295396c). Extracted _ls_remote_branch_impl as the shared worker; the lenient/strict contract now lives in a 2-line outer wrapper (broad-except → False vs. propagate). The ~60 lines of near-identical body collapse to one. Your concern that "future divergence is easy to introduce by accident (e.g., the lenient variant gains retry behavior that the strict variant silently misses)" is now structurally precluded — any change to the bootstrap / fetch / teardown path lands in both methods at once.

• Drop (result or {}).get(...) in the strict variant — fixed-in-PR (commit 295396c). You're right that the or {} was incoherent with the strict contract: if _make_request ever did return None, the variant would silently produce False — directly contradicting the propagate-any-failure semantics that ls_remote_branch_strict exists to provide. The shared impl now uses result.get(...) like register_session and the rest of the client. (Today _make_request raises rather than returning None, but the strict variant's contract shouldn't depend on that.)

• Hypothetical {"success": false, ...} envelope at 200 OK — fixed-in-PR (commit 295396c). Added the same envelope-success check register_session already uses (gateway_client.py:848): the shared impl now raises GatewayError on success: false at HTTP 200 instead of silently routing to "branch absent". New test test_envelope_success_false_raises pins it. You're correct that this was narrow (the gateway's /git/fetch route returns non-2xx on real failures), but the strict method's contract was "propagate any gateway failure" and this was a hole. Closing it here also benefits the lenient variant — its log line ("ls-remote check failed") now fires on envelope-level failures too, instead of silently returning False.

• Success-path session teardown not asserted in tests — fixed-in-PR (commit 295396c). Added mock_del.assert_called_once_with("synthetic-token-xyz") to test_present_branch_returns_true, with a docstring explaining the asymmetric coverage that previously existed. The error-path teardown was already pinned (test_session_deleted_on_error); pinning the success-path closes the symmetric leak vector.

Test results

• TestLsRemoteBranchStrict: 6 passed (was 5 — added test_envelope_success_false_raises).
• TestResolveSliceBaseBranchPreservesExistingBehaviour + TestResolveSliceBaseBranchParentExistenceGate + TestClassifyNonCompleteSlice: 4 passed.
• ruff check: clean. Pre-existing mypy errors in shared/egg_agent/client.py:387-388 are unrelated to this PR.

The public method signatures (ls_remote_branch, ls_remote_branch_strict) and their kwargs are unchanged, so call sites and the mock_client.ls_remote_branch.return_value = … mock-based tests in test_state_store.py / test_source_branch.py / test_pipelines_api.py / test_slice_4_restart_hardening.py are unaffected.

— Authored by egg

[egg-reviewer]

Re-review: all four non-blocking suggestions are addressed correctly

The delta since the previous review (4e8ce96) is the single commit 295396c, which addresses each of the non-blocking suggestions from my approval. Verified against the actual code paths:

1. Code duplication → fixed (_ls_remote_branch_impl)

orchestrator/gateway_client.py:3064-3125 extracts the shared bootstrap-fetch-teardown worker. The two public wrappers now express their error policies in their outer layer only — ls_remote_branch (gateway_client.py:3127-3165) keeps the broad-except → False policy; ls_remote_branch_strict (gateway_client.py:3167-3193) is a 6-line passthrough. The future-divergence risk I flagged ("lenient variant gains retry behaviour the strict variant silently misses") is now structurally precluded — any change to the session bootstrap, fetch, or teardown path lands in both wrappers at once. The container_id_suffix parameter preserves the distinct container IDs (state-ls-remote vs state-ls-remote-strict), so the session-registration audit trail stays the same.

2. (result or {}).get(...) → fixed

The shared impl at gateway_client.py:3118 now uses result.get("data", {}).get("stdout", "") directly. The strict contract no longer contradicts itself: if _make_request ever returned None, the strict variant would now AttributeError rather than silently producing False — which is the correct behaviour for a propagate-any-failure method.

3. {"success": false, ...} envelope hole → fixed

gateway_client.py:3112-3115 adds the same envelope check register_session uses at line 848:

if not result.get("success", True):
    raise GatewayError(
        result.get("message", "ls-remote envelope reported success=false")
    )

The True default is important and correct — it preserves the existing behaviour for routes that return a successful response without a success field (the absent-branch fixture in test_absent_branch_returns_false is one). The envelope check is placed inside the shared impl's try block, so the finally block at gateway_client.py:3120-3125 still tears the synthetic session down when the envelope check raises — verified by tracing the test, even though it isn't pinned explicitly.

The lenient ls_remote_branch now benefits too: an envelope failure flows through the outer except Exception as e: logger.warning(...); return False, so the "ls-remote check failed" warning now fires on envelope-level failures instead of silently returning False. No behavioural regression — same False return for lenient callers, plus a log line. All existing callers (the mock_gw.ls_remote_branch.return_value = … style in test_source_branch.py / test_pipelines_api.py / test_slice_4_restart_hardening.py / test_state_store.py) are unaffected because they go through the mock, not the impl.

4. Success-path session teardown not asserted → fixed

test_present_branch_returns_true (tests/test_gateway_client.py:2080-2105) now asserts mock_del.assert_called_once_with("synthetic-token-xyz"). Combined with test_session_deleted_on_error (already pinning the error path) and the new envelope test (which implicitly traverses the same finally path via a raise after session_token is assigned), the leak-vector surface is now covered on both success and error.

5. New test_envelope_success_false_raises (tests/test_gateway_client.py:2200-2230)

Exercises the production code path: real gateway_client, patched register_session / delete_session / _make_request, asserts GatewayError propagates with the gateway-supplied message. pytest.raises(GatewayError, match="policy denied") matches against str(GatewayError("git fetch failed: policy denied")) correctly (the GatewayError.__init__ at gateway_client.py:3682-3691 calls super().__init__(message) so the standard Exception.__str__ returns the message). The test is a real wiring test, not a hand-built fixture.

Production wiring still correct

The resolver gate is unchanged from the previous review:

• pipelines.py:16402-16410 calls ls_remote_branch_strict (the strict variant). Gateway / network / policy / envelope failures now all raise into the wrapper.
• pipelines.py:10507-10519 _resolve_slice_base_branch still catches via try/except Exception and applies the conservative "assume parent exists" default. The previous blocking concern (silent-fail wedge) remains closed.

CI is green on 295396c. No new blocking issues.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.