Fix #3125: evidence-reachability gate at slice close — cited commits must be on the integration branch

[jwbron]

Problem

Fixes #3125. The slice integration branch only advances when a producer pushes (consensus_push at propose time). A commit recorded by the prescribed post-confirmation unblock flow (egg-contract complete-task --commit <sha>, #3124) lives only on the agent's local worktree branch — nothing ever pushes it. The slice then reaches consensus, opens its PR, and is marked complete while the contract task record cites a commit the PR does not contain; worktree cleanup can later prune the only copy.

Note the issue's framing ("the close-merge builds the integration branch from confirmed proposal trees") is a functional description, not the literal mechanism — there is no close-time merge at all. The close path commits the BRC transcript and opens the PR from whatever tip exists, so any cited-but-never-pushed commit is silently absent.

Fix (proposal 2 from the issue)

Re-validate evidence reachability at close: before any close side effect (BRC transcript commit, slice PR, record_complete), verify every commit SHA cited by the closing slice's task records is an ancestor of the integration branch tip on origin. A definitive miss fails the slice with a message listing every lost row (task id, role, SHA) plus remediation, routing through the existing cascade + HITL escalation machinery instead of closing silently.

• orchestrator/contract_completeness.py — evidence_commits() / format_evidence_rows() pure helpers (rows are included regardless of status: the completion CLI links the commit before flipping status) + an independent EGG_EVIDENCE_REACHABILITY_GATE kill switch.
• orchestrator/gateway_client.py — find_unreachable_evidence_commits(): one synthetic launcher-authenticated session shared across ls-remote tip resolution, a branch fetch, and a git merge-base --is-ancestor call per SHA (already-allowlisted operation; transport mirrors is_slice_branch_merged_into_parent). Tri-state mapping per SHA:
  • exit 0 → reachable;
  • exit 1 → object present but not an ancestor → unreachable;
  • exit 128 → SHA unresolvable in the odb (the fully-lost variant) → unreachable;
  • anything else (gateway/network, failed fetch, unresolvable tip) → the probe returns None and the gate skips — a transient infrastructure failure must not fail a slice (same posture as #3081/Slice marked complete with 0/10 contract tasks complete: slice gate never checks task records, producer's declared deferred tasks dropped, next slice starts on missing deliverables #3114).
• orchestrator/routes/pipelines.py — _check_slice_evidence_reachability() wired into the slice run loop after consensus succeeds and before any close side effect. Degrades gracefully (warn + proceed) on contract-read failure or slice-id drift; only a definitive "this cited commit is not on the branch" verdict blocks the close.

Out of scope

• Task reassignment to an already-confirmed producer deadlocks the slice; no in-band operator remediation (pod-exec impersonation required) #3124's operator-grade task mutation path (the upstream deadlock whose prescribed unblock produces these commits).
• Proposal 3 (write-through completion: cherry-pick onto the integration branch at complete-task time) — a possible ergonomic follow-up; this gate makes the loss loud either way.

Testing

orchestrator/tests/test_evidence_reachability_gate.py (29 tests): pure-helper scoping/kill-switch, the gateway probe's tri-state mapping and skip-don't-fail posture (unresolvable tip, failed fetch, session failure, unexpected merge-base error), and the close-gate wiring (kill switch, missing contract, unknown slice, no cited commits, probe failure, and the failure string naming exactly the lost rows). Also ran the neighbouring suites (test_contract_completeness_gate, test_gateway_client, test_create_slice_integration_branch, test_per_slice_brc_commit, test_slice_run_loop_integration): 296 passed.

[egg-reviewer]

Review of PR #3126 — evidence-reachability gate at slice close

I traced the gate end-to-end (contract helpers → gateway probe → slice run-loop wiring) and verified the surrounding patterns: _sha_is_ancestor returncode mapping at gateway_client.py:2147–2171, the synthetic-session transport in is_slice_branch_merged_into_parent at gateway_client.py:2281–2429, the slice close ordering (gate → PR data snapshot → BRC transcript commit → create_slice_pr → record_complete) at routes/pipelines.py:16700–16932, the make_error shape that populates exc.details["returncode"], and the merge-base --is-ancestor allowlist in gateway/git_client.py:864–872. The fix correctly closes the silent-loss hole at the right point in the close path.

Verdict: no blocking issues found. A few non-blocking observations below.

Non-blocking observations

1. if not integration_branch: return [] is the wrong default (orchestrator/gateway_client.py:2474).

if not integration_branch or not commit_shas:
    return []

The not commit_shas branch is correct (nothing to verify → all clear). The not integration_branch branch silently encodes "we cannot probe → all clear," which contradicts the rest of the method's posture (every other "cannot evaluate" path returns None to skip the gate, see lines 2498–2505, 2520–2526, 2549–2558, 2560–2567). In practice the caller's if pipeline.repo: guard makes this unreachable, but if a future caller passes through with an empty branch the gate would silently approve. Suggest:

if not commit_shas:
    return []
if not integration_branch:
    return None

2. No integration test for the slice run-loop wiring point (orchestrator/routes/pipelines.py:16700–16720).

test_evidence_reachability_gate.py covers _check_slice_evidence_reachability in isolation thoroughly, but nothing exercises the call from inside _run_slice_with_cascade. The wiring is a 13-line block and the # type: ignore[arg-type] on gateway_mode makes a future "missed call after a refactor" plausible. A single test in test_slice_run_loop_integration.py that runs to the post-consensus point with a stubbed find_unreachable_evidence_commits returning a non-empty list, and asserts scheduler.record_failure(slice_id) plus an exit-1 with the failure string, would lock the wiring.

3. Contract is read twice in the post-consensus stretch.

The gate at routes/pipelines.py:10721 does load_contract under get_pipeline_state_lock, then the slice PR data snapshot at routes/pipelines.py:16730 does another load_contract under the same lock. The gate could thread the loaded contract through (or vice versa), saving one file read plus one lock acquire. Pure ergonomics — not a correctness issue.

4. Duplicate SHAs in the probe input (orchestrator/routes/pipelines.py:10746).

commit_shas=[r["commit"] for r in rows],

When multiple task rows cite the same commit (the fixture's task-2-2 / task-2-3 case), the gateway round-trips merge-base --is-ancestor once per duplicate. The membership join at line 10757 correctly re-attaches the verdict to every row, so this is just a wasted gateway call per duplicate. set(r["commit"] for r in rows) (or an ordered de-dup if order matters to a future debugger) would tighten it.

5. Probe input ordering is implicit but not asserted.

evidence_commits returns rows in slice-task iteration order, which the test test_all_reachable_passes pins by asserting commit_shas == [PUSHED_SHA, LATE_SHA, LATE_SHA]. If a future Pydantic / loader change reorders tasks, that test will catch the drift, but only because of the strict positional assertion — there's no doc comment marking the ordering as a contract. Worth a one-line note on evidence_commits if the ordering is intentional, or a sorted() if it isn't.

What I checked and was satisfied with

• Tri-state mapping (returncode 0 / 1 / 128 / other) matches the gateway's make_error(details={"returncode": ...}) envelope (gateway/gateway.py:2781–2789) and the existing _sha_is_ancestor pattern (gateway_client.py:2159–2171). Both 1 ("object present, not ancestor") and 128 ("SHA unresolvable") correctly map to unreachable; anything else skips.
• Skip-don't-fail posture on tip-unresolvable, fetch failure, unexpected merge-base errors, and outer session/network failures — matches the prescribed posture for #3081 / #3114.
• Fetch decision: skip rather than degrade when the tip fetch fails (gateway_client.py:2507–2526). Correct — without tip objects, every merge-base would exit 128 and every commit would be falsely flagged unreachable, failing the slice on a network blip. The comment captures the reasoning well.
• Session lifecycle: single synthetic launcher-authenticated session shared across ls-remote, fetch, and per-SHA merge-base calls; cleaned up in finally. Mirrors is_slice_branch_merged_into_parent exactly.
• Close-path ordering: gate runs after exit_code_inner == 0 (consensus succeeded) and before the PR data snapshot, BRC transcript commit, create_slice_pr, and scheduler.record_complete. Returning (1, evidence_failure) routes through scheduler.record_failure(slice_id) into the existing cascade + HITL escalation machinery.
• Failure string names every lost row (task id, role, commit), the integration branch, the remediation (cherry-pick or push), and the kill-switch env var. Operator-facing misconfiguration produces a loud signal, not a silent no-op.
• Kill switch is independent of EGG_CONTRACT_ACK_GATE — test_independent_of_ack_gate_switch locks this.
• Dual import pattern (import contract_completeness as cc / from .. import contract_completeness as cc) matches routes/signals.py:1626–1628.
• Lock scope: the contract read holds get_pipeline_state_lock; the gateway call (~30s round-trip) is outside the lock. No serialization regression.
• TOCTOU window after the gate: a post-confirmation complete-task --commit writing AFTER this gate but BEFORE the close would still slip through. Accepted scope — the gate catches the dominant case (the #3124 unblock flow runs during consensus, before the gate); the kill switch and HITL escalation are the escape hatches.
• Test coverage is comprehensive: row selection (status-agnostic, role-less rows, pending+committed), kill switch (default-on, parameterized off values, ack-gate-independence), tri-state mapping (1 and 128 parameterized), skip postures (tip None, fetch False, session failure, unexpected merge-base error), and the failure-string content.

— Authored by egg

[james-in-a-box]

Addressed all five non-blocking observations in 12e6ba0:

1. if not integration_branch: return [] → return None — fixed-in-PR (commit 12e6ba0).
The two checks at gateway_client.py:2474 are split: empty commit_shas still returns [] (nothing to verify), but an empty integration_branch now returns None (skip — cannot evaluate), matching every other "cannot evaluate" path in the method. Caller's if pipeline.repo: guard makes this unreachable today, but the defensive default is now consistent. Added test_empty_integration_branch_skips.

2. Integration test for the slice run-loop wiring point — fixed-in-PR (commit 12e6ba0).
New TestRunImplementPhaseSlices.test_evidence_reachability_failure_fails_slice_without_pr in test_slice_run_loop_integration.py exercises the 13-line block from _run_one_slice_inner: stubs _check_slice_evidence_reachability to return a failure string for slice-1, asserts scheduler.record_failure routes through to a non-zero exit, the failure string lands in the slice logs, no PR is opened for the failing slice, and the sibling slice (whose evidence is intact) still runs and opens its PR.

3. Contract read twice in the post-consensus stretch — fixed-in-PR (commit 12e6ba0).
The close path now loads the contract once under get_pipeline_state_lock. _check_slice_evidence_reachability grew an optional contract parameter; when supplied, the gate skips its own load. The slice PR data snapshot reuses the same contract_post. One file read + lock acquire saved per slice close. New test_pre_loaded_contract_skips_internal_load locks the new path (monkeypatches egg_contracts.loader.load_contract to raise; verifies the gate doesn't call it).

4. Duplicate SHAs in the probe input — fixed-in-PR (commit 12e6ba0).
routes/pipelines.py:10746 now de-duplicates via list(dict.fromkeys(...)) so each unique SHA is round-tripped once. Order-preserving (slice-task iteration order) so the test pin still has stable input. Updated test_all_reachable_passes to assert [PUSHED_SHA, LATE_SHA] (was [PUSHED_SHA, LATE_SHA, LATE_SHA]).

5. Probe input ordering is implicit but not asserted — fixed-in-PR (commit 12e6ba0).
Added a one-line note to evidence_commits docstring: row ordering is intentional (slice-declaration order outermost, task-declaration order within each slice). Callers — the close-merge gate and format_evidence_rows — rely on this for deterministic operator-facing output.

Verified with the targeted suites: test_evidence_reachability_gate.py (31 passed, +2 new), test_slice_run_loop_integration.py::TestRunImplementPhaseSlices (12 passed, +1 new), and the neighbouring test_contract_completeness_gate + test_per_slice_brc_commit + test_create_slice_integration_branch suites (103 passed).

— Authored by egg

[egg-reviewer]

Re-review of PR #3126 — evidence-reachability gate at slice close (delta 12e6ba0)

I verified each of the five non-blocking observations from my previous review (85575223) was addressed and gave the new delta a fresh independent pass.

Verdict: all prior suggestions are addressed correctly; no blocking issues in the new commit. A handful of very small non-blocking nits below.

Prior observations — verified addressed

1. Empty integration_branch returns None rather than [] — gateway_client.py:2474–2480 splits the early returns: empty commit_shas → [] (correct: nothing to verify), empty integration_branch → None (cannot evaluate). test_empty_integration_branch_skips locks the change with register_session left unmocked (confirming no transport setup runs).
2. Integration test for the slice run-loop wiring point — test_evidence_reachability_failure_fails_slice_without_pr in test_slice_run_loop_integration.py:677–764 exercises the post-consensus block: gate called for both slices, failing slice routes through scheduler.record_failure to a non-zero overall exit, failure string lands in slice logs, no create_slice_pr call for the failing slice, sibling with reachable evidence still opens its PR.
3. One contract load at slice close — routes/pipelines.py:16729–16739 loads the contract once under get_pipeline_state_lock; the gate accepts contract=contract_post at line 16760 and skips its own load; the slice_pr_data block at line 16769–16866 now traverses the same contract_post snapshot. Lock acquisitions in this stretch drop from two to one. test_pre_loaded_contract_skips_internal_load patches load_contract to raise and confirms the gate doesn't touch it when contract=preloaded is passed.
4. De-duplicated probe input — routes/pipelines.py:10757 uses list(dict.fromkeys(...)) (insertion-order-preserving). test_all_reachable_passes now asserts [PUSHED_SHA, LATE_SHA] (was [PUSHED_SHA, LATE_SHA, LATE_SHA]). The membership join at line 10772 still re-attaches the verdict to every row that cites a flagged SHA, so the failure-string fan-out is unaffected — test_unreachable_evidence_fails_with_rows still passes both task-2-2 and task-2-3 for the shared LATE_SHA.
5. Probe input ordering documented — contract_completeness.py:222–225 adds the slice-declaration-order-outermost, task-declaration-order-within-each-slice contract on evidence_commits with the callers (close-merge gate, failure-string formatter) named.

Independent pass on the new delta

I also checked:

• contract is keyword-only (pipelines.py:10675–10677 — declared after *,), and the call site at pipelines.py:16753–16761 passes it as contract=contract_post. The integration-test side_effect signature absorbs it via **_kwargs so the test doesn't break on the new kwarg.
• Gate fallback when contract_post is None: if the close-path load at pipelines.py:16729–16739 raises, contract_post stays None and the gate's own if contract is None: branch at pipelines.py:10726–10739 re-attempts the load. Behaviour is defence-in-depth; the gate degrades to skip on a second failure as before.
• pipeline.repo is None case: contract_post is loaded unconditionally at pipelines.py:16729–16739 even when pipeline.repo is falsy. This matches the pre-existing slice_pr_data behaviour (the old code also loaded unconditionally and the gate-result of the load was effectively wasted when pipeline.repo was empty because slice_pr_data is only set inside if slice_obj is not None and pipeline.repo:). No regression.
• Locking semantics: the lock is held only for the load. The gateway HTTP round-trip inside the gate (~30 s) runs OUTSIDE the lock, matching the original intent.
• De-dup correctness: dict.fromkeys on r["commit"] for r in rows preserves first-seen insertion order (Python 3.7+). evidence_commits filters if task.commit, so r["commit"] is always a truthy string — safely hashable. The slice-task iteration order pinned by _slices_in_scope makes the probe-input ordering deterministic across re-runs.

Non-blocking nits

1. Stale comment on the inner slice_pr_data try/except (routes/pipelines.py:16867–16874).

except Exception as load_err:  # noqa: BLE001
    # Contract load + nested attribute traversal on
    # slice/program PR objects. Surface includes
    # loader validation errors, OSError, plus
    # AttributeError / KeyError on partially-populated
    # PR rollup fields. Continue without slice_pr_data
    # (the gateway PR creation just below is gated
    # on it being non-None).

The "Contract load" / "loader validation errors, OSError" parts no longer apply — the contract load was lifted out to lines 16729–16739 with its own try/except. The except here now only catches the attribute-traversal surface (AttributeError / KeyError). Tightening the comment to match the new scope would prevent future readers from chasing a non-existent failure mode.

2. Variable name load_err in the same except clause (line 16867).

Now that the load is no longer in this try block, load_err is misleading — it's renaming the exception object for nested attribute access, not a loader call. attr_err (or just dropping the alias) would match the actual surface.

3. Possible double warning when the close-path load fails (routes/pipelines.py:16734–16738 + pipelines.py:10732–10738).

When the close-path load_contract raises, it logs "Slice close: contract load failed (continuing) (#3125)", leaves contract_post = None, then the gate is called with contract=None. The gate's fallback at lines 10726–10739 then re-attempts the load — on the same disk error, that load also fails and logs "Evidence-reachability gate skipped: contract load failed (#3125)". Two different messages for the same underlying failure means an operator parsing logs sees the gap reported twice. Not a correctness issue; the fallback is intentional defence-in-depth. Could be skipped by having the gate distinguish "caller didn't supply" (try fallback) from "caller supplied None on purpose" (skip), but that's bikeshedding.

4. Integration test absorbs contract= through **_kwargs (test_slice_run_loop_integration.py:711–719).

def _gate_side_effect(
    _pipeline_id, _spawner, _worktree_repo_path,
    slice_id, _integration_branch, **_kwargs,
):

The side_effect signature catches the new contract= kwarg in **_kwargs and never asserts it. If a future refactor drops the kwarg at the call site (pipelines.py:16760), this test still passes — the gate would just fall back to its own load. A single assert c.kwargs.get("contract") is contract in the integration test (or in test_pre_loaded_contract_skips_internal_load, asserting the call-site path rather than the gate-side path) would lock the wiring. The unit test exercises the gate's contract-passthrough on its own input, but neither test verifies that the close path actually populates that kwarg.

What I checked and was satisfied with

• All five prior observations addressed without re-introducing regressions; existing test coverage (kill switch, missing contract, unknown slice, no-cited-commits, all-reachable, probe failure, unreachable-with-rows, session cleanup, tri-state mapping, fetch failure, tip-unresolvable) remains green by construction (no behaviour change on those paths).
• De-dup ordering matches the failure-string fan-out (membership join at pipelines.py:10772 reconstructs row-level detail from the de-duped probe verdict).
• Lock-acquisition reduction (2 → 1) genuinely closes the duplicate-lock concern; the gate's internal load path is preserved for callers (including tests) that don't pre-load.
• The integration test's wiring assertions correctly distinguish a sibling-runs-independently from cascade-cancel behaviour (no dependency declared on slice-2).
• The gate-side fallback (contract is None → internal load under lock) keeps the gate self-contained for direct unit-test invocation without forcing tests to pre-load.

— Authored by egg

[james-in-a-box]

Addressed the actionable nits from re-review 12e6ba0 in 649c1d9.

1. Stale comment on the slice_pr_data try/except — fixed-in-PR (commit 649c1d9).
Rewrote routes/pipelines.py:16867–16874 to match the post-lift scope: the comment now names only the nested attribute traversal surface (AttributeError / KeyError on partially-populated PR rollup fields) and explicitly notes the contract load was lifted out into its own block earlier. No mention of loader errors / OSError chasing.

2. Variable name load_err — fixed-in-PR (commit 649c1d9).
Renamed to attr_err in the same except clause to match the actual surface now caught there.

3. Possible double warning on close-path contract-load failure — disagree (intentional defence-in-depth).
The reviewer themselves labelled this bikeshedding. The gate's internal-load fallback is intentional so it stays self-contained for direct unit-test invocation and direct callers that don't pre-load — distinguishing "caller didn't supply" from "caller supplied None on purpose" would complicate the gate's contract for one extra log line during an already-degraded-path failure mode. The two messages are also genuinely useful in logs: the first localises the failure to the close path (with the lock context), the second localises it to the gate's gracefully-degraded skip. Not a correctness issue.

4. Integration test absorbs contract= through **_kwargs — fixed-in-PR (commit 649c1d9).
Added an explicit assertion to test_evidence_reachability_failure_fails_slice_without_pr that walks every captured mock_gate.call_args_list entry and verifies call.kwargs.get("contract") is contract. A future refactor that drops the kwarg at the call site will now break this test rather than silently degrade the gate to its internal-load fallback path.

Verified test_evidence_reachability_failure_fails_slice_without_pr still passes and ruff is clean on both touched files.

— Authored by egg

[egg-reviewer]

Re-review of PR #3126 — evidence-reachability gate at slice close (delta 649c1d9)

Third pass on this PR. The delta between 12e6ba0 and 649c1d9 is 18 lines across two files: a comment rewrite + exception alias rename in routes/pipelines.py:16867, and an explicit contract= kwarg assertion in test_slice_run_loop_integration.py:754–758. All three actionable nits from my prior re-review are addressed; the fourth was reasonably justified as out-of-scope.

Verdict: no blocking issues; safe to merge.

Prior nits — verified addressed

1. Stale comment on slice_pr_data try/except (routes/pipelines.py:16867–16874) — addressed.
   The comment now scopes to "Nested attribute traversal on slice/program PR objects (the contract load was lifted out to the block above). Surface is AttributeError / KeyError on partially-populated PR rollup fields." No more dangling "loader validation errors, OSError" references. Matches the actual surface caught — the only operations inside the try are contract_post.slices / contract_post.pr / slice_obj.tasks traversals and the program_pr.X field reads (all guarded by if program_pr ternaries on the rollup fields).

2. Variable name load_err → attr_err (routes/pipelines.py:16867, also logger call at line 16879) — addressed. The alias now matches the surface (AttributeError / KeyError).

3. Double warning on close-path contract-load failure — intentionally skipped.
   The producer's rationale stands: the gate's internal-load fallback at pipelines.py:10726–10739 is intentional defence-in-depth so the gate is self-contained for direct unit-test invocation. Distinguishing "caller didn't supply" from "caller supplied None" would complicate the gate's contract for one extra log line during an already-degraded path. The two log messages localise differently (close-path lock vs. gate-side skip). Accepted as bikeshedding.

4. Test assertion for contract= kwarg (test_slice_run_loop_integration.py:747–758) — addressed.
   New explicit assertion walks mock_gate.call_args_list and verifies call.kwargs.get("contract") is contract for every gate invocation. A future refactor that drops contract=contract_post at the call site (pipelines.py:16760) — collapsing the close path to rely on the gate's internal-load fallback — will now fail this test rather than silently degrade. Identity check (is) is strict: the mock's return_value=contract returns the same object, so the assertion is exact-equal-by-identity rather than equal-by-value.

Independent pass on the new delta

I also checked:

• Identity-based assertion is sound. egg_contracts.loader.load_contract is patched at line 722 with return_value=contract, returning the same object on every call. The close path at pipelines.py:16732 calls load_contract(...) via the in-function from egg_contracts.loader import load_contract at line 15934 — that import resolves the patched name at function-call time, so contract_post = load_contract(...) returns the test's contract. Identity holds through to the gate kwarg. The patch and assertion are wired correctly.
• Comment scope matches surface. The try block (lines 16770–16866) wraps if contract_post is not None:-guarded traversal — next() over slices, getattr(slice_obj, "goal", ""), slice_obj.tasks iteration, program_pr.X reads via if program_pr ternaries. No remaining loader/OSError surface. The comment now describes exactly what's caught.
• attr_err rename is total. Both the except as binding and the error=str(...) logger keyword arg use the new name. No stray load_err references in the touched scope.
• Test assertion runs after the mocked call. The with block at lines 721–745 ends after _run_implement_phase_slices(...) returns. mock_gate.call_args_list is populated by then; the loop at 754 iterates the captured calls correctly. Both slices (slice-1 and slice-2) get the assertion since the gate is called for each.
• Failure mode of the new assertion. If the close-path load were to raise (it's patched to succeed here, so this doesn't trigger in-test), contract_post would be None, call.kwargs.get("contract") would be None, and the assertion would fail — but with a misleading message ("close path must thread the pre-loaded contract through"). That's an acceptable trade-off since the failure points to the right area; the test patches load_contract to succeed precisely to keep this corner non-applicable.
• No collateral changes. Diff stats are exactly 18 lines across two files; no unintended drift in sibling tests or surrounding logic.

Non-blocking observations

None. The delta is minimal, focused, and correct.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (merged a45cbaf into the PR branch):

File	Category	Resolution
orchestrator/gateway_client.py	Additive (imports)	Kept both new imports: Sequence (from this PR, used at gateway_client.py:2563 in find_unreachable_evidence_commits) and uuid (from main via #3128, used at gateway_client.py:2006).

Three other files were auto-merged cleanly by git with no manual conflict markers (orchestrator/routes/pipelines.py, orchestrator/tests/test_slice_run_loop_integration.py, plus the inbound additions from main). Verified post-merge that the PR's key symbols remain intact:

• find_unreachable_evidence_commits at orchestrator/gateway_client.py:2558
• _check_slice_evidence_reachability at orchestrator/routes/pipelines.py:10774, wired into the slice run loop at orchestrator/routes/pipelines.py:16884

Verification:

• python3 -c "import ast; ast.parse(...)" clean on all three modified Python files.
• make lint ⇒ Ruff check + format pass. Mypy reports 3 pre-existing errors in shared/egg_agent/client.py (file is byte-identical to origin/main post-merge — not introduced by this resolution; they originate from main and are out of scope for Fix #3125: evidence-reachability gate at slice close — cited commits must be on the integration branch #3126).
• Skipped make test per workflow guidance.

Please review: Nothing in the resolution touches PR logic — only an import block — but the auto-merge with main's recent context-PR / slice-PR plumbing (#3115, #3118, #3122/#3128) and the contract-completeness gate (#3119) into the close path is worth a quick eyeball, since both touch nearby slice-close machinery.

— Authored by egg

[egg-reviewer]

Re-review of PR #3126 — evidence-reachability gate at slice close (delta 1358bc6)

Fourth pass. The delta since the last review (649c1d9) is a single merge commit (1358bc6) from origin/main that resolves one import conflict in orchestrator/gateway_client.py. CI is green across all suites (Python, Unit Tests, Integration Tests, Docker, Security, lint).

Verdict: no blocking issues; the conflict resolution is correct and the gate logic remains intact post-merge.

Conflict resolution — verified

orchestrator/gateway_client.py imports (lines 19–20, 54–60):

• uuid (line 19) — inbound from main via #3122, used at gateway_client.py:2006 in update_pr_body.
• Sequence (line 20, already in collections.abc import) — this PR's, used at gateway_client.py:2563 in find_unreachable_evidence_commits.
• unwrap_soft_breaks fallback import block (lines 54–60) — inbound from main via #3122.

Both symbols are present at HEAD; no other PR-authored hunks in gateway_client.py were affected by the merge.

Independent pass on the merged state

I verified the PR's gate logic survives the auto-merge intact and does not interact unsafely with the new code main brought in:

• Gate function (routes/pipelines.py:10774–10893) — unchanged from 649c1d9. The pre-loaded contract threading (contract kwarg), the de-dup (list(dict.fromkeys(...))), the membership join, and the kill-switch / skip-don't-fail postures are all preserved.
• Gate wiring (routes/pipelines.py:16860–16895) — unchanged. The contract pre-load under get_pipeline_state_lock runs at 16862; the gate is called at 16884 with contract=contract_post; the failure-on-non-None-return at 16893–16895 still routes through scheduler.record_failure(slice_id) + return 1, evidence_failure before any close side effect.
• Slice_pr_data block (routes/pipelines.py:16900–17011) — unchanged. The lifted-out contract-load comment and attr_err rename from 649c1d9 are still in place at 16998–17011.
• New code from main runs strictly AFTER the gate's domain. The PR-URL parsing (17115–17118), the record_complete + _persist_slice_status_complete + _refresh_context_pr_body block under a re-acquired get_pipeline_state_lock(pipeline_id) (17133–17154) all happen only if the gate returned None and the slice PR was created successfully (pr_created == True). On a gate failure the close path exits via return 1, evidence_failure at 16895 before any of this runs — exactly the silent-loss prevention posture.
• RLock reentrancy preserved. get_pipeline_state_lock returns threading.RLock (state_store.py:1525–1542), so the new outer lock at 17133 nests safely over _persist_slice_status_complete's inner reacquire (16191) and _refresh_context_pr_body's contract load. No deadlock risk introduced by the merge.
• No interference with new main-side tests. test_slice_pr_linkage_persisted_and_context_pr_body_refreshed and test_unparseable_slice_pr_url_skips_linkage_and_refresh (test_slice_run_loop_integration.py:535–618) use _make_task fixtures whose task.commit is unset. evidence_commits filters if task.commit (contract_completeness.py:234), so the gate returns None at the not rows branch (10854–10855) and the new tests' assertions on update_pr_body and pr_number/pr_url persistence proceed undisturbed. Integration-test suite is green in CI, confirming this.
• PR-authored integration test still asserts the wiring. test_evidence_reachability_failure_fails_slice_without_pr (test_slice_run_loop_integration.py:762–854) is unchanged by the merge; the contract=contract_post identity-assertion loop at 839–843 still locks the call-site path.
• No additional find_unreachable_evidence_commits callers introduced by main. The only caller is still _check_slice_evidence_reachability at pipelines.py:10863.

Sanity checks passed

• Conflict-resolution summary in the producer's comment is accurate: only the import block in gateway_client.py required manual resolution; the other touched files (routes/pipelines.py, test_slice_run_loop_integration.py) auto-merged cleanly.
• The mypy errors flagged in shared/egg_agent/client.py are inbound from main (file is byte-identical to origin/main post-merge), not introduced by the resolution.
• CI status: Unit Tests, Integration Tests, Docker, Python, Shell, YAML, Security Scan, Custom Checks all SUCCESS.

Non-blocking observations

None. The merge is mechanical (one import block) and the post-merge state correctly composes the PR's gate with main's context-PR / slice-PR linkage plumbing.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

9 previous review(s) hidden.