Guard the pre-wait window at every HITL gate on cancel

[james-in-a-box]

Guard the pre-wait window at every HITL gate on cancel

Round-4 review feedback on #3645. Every cancel guard added so far runs
after wait_for_decision, while the queue-and-park sequence in front of it
is unguarded. Two failures fall out of that gap: a decision minted after the
cancel route's one-time get_pending_decisions() sweep blocks
DecisionQueue.wait_for_decision (a while True 5s poll with no timeout)
for the process lifetime, so _run_pipeline's finally never runs — no
cleanup, no #1725 worktree preservation; and an unconditional
AWAITING_HUMAN park clobbers the operator's persisted CANCELLED, so the
post-wait check reads back the gate's own write and "" in _APPROVE_KEYWORDS
advances the phase — #3633 verbatim.

Adds a pre-queue_decision _pipeline_cancelled check at every site that
mints a decision, plus _park_at_gate_unless_cancelled, a shared helper that
re-reads the persisted status inside the same per-pipeline state lock that
performs the park write — the lock StateStore.update_pipeline takes, which
is what makes the check atomic against the cancel route. Applied at all seven
sites the reviewer enumerated: ledger backstop, phase gate (create arm),
follow-up specifics, attestation gate, gap gate, divergence-reconcile pause,
and the contract bridge (checked before pass 1). Each bail propagates a stop
the driver already acts on.

Also in this round: the _run_gate harness now feeds save_pipeline back
into the status cell (the lost update was invisible to every prior test — the
headline test fails without the fix now); the deliberate
phase-box-stays-AWAITING_HUMAN choice is documented on
_gate_wait_cancelled; the bridge test docstring inversion is corrected; and
_queue_and_await_contract_decisions moved to a new _contract_bridge.py so
_ledger.py stays under the 1500-line cap without a third allowlist entry.

Stacked on issue-3633-cancel-stops-driver (the gateway only accepts
egg/-prefixed branches).

Issue: #3633

Test Plan

• Automated: orchestrator/tests/test_cancel_stops_driver.py (new:
  test_gate_never_mints_a_decision_for_a_cancelled_pipeline,
  test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it,
  test_park_at_gate_unless_cancelled_checks_inside_the_lock),
  test_contract_decision_bridge.py (new:
  test_bridge_never_queues_for_an_already_cancelled_pipeline),
  test_hard_reset_recovery.py (new:
  test_cancel_before_the_pause_is_not_overwritten_by_the_park) — 77 passed
  locally. Each new guard was mutation-checked (guard reverted → test fails)
  to prove non-vacuity. make lint clean.
• Manual: none required — the paths are exercised by the tests above.

Manual Steps

• None.

Authored-by: egg

[egg-reviewer]

Review: Guard the pre-wait window at every HITL gate on cancel (#3633)

The core of this PR is right, and I verified its central claim rather than taking the docstring's word for it: _park_at_gate_unless_cancelled takes get_pipeline_state_lock(pipeline_id) (_run_support.py:196), and StateStore.update_pipeline — the cancel route's persistence path via _routes_crud.py:588 — takes the same per-pipeline threading.RLock from the _pipeline_state_locks registry (state_store/_crud.py:419, state_store/_locks.py). The in-lock re-read is genuinely atomic against the cancel. The _contract_bridge.py extraction is a faithful move (I diffed the removed _ledger.py hunk against the added file: identical apart from the module header, the rewritten docstring, and the one new pre-pass-1 guard), the barrel seam holds (_run_hitl_gate.py:810 reaches it via _pkg., so patch.object(pipelines_pkg, "_queue_and_await_contract_decisions") still intercepts), and both stop-propagation claims in the comments check out — _run_implement_advance really does re-read the cancel and return "break" (_run_phase_blocks.py:49), and _fail_pipeline_after_divergence_abort really is a status no-op on CANCELLED (_alerts.py:140), with both of its call sites breaking/returning.

But the PR's own three-check contract is not satisfied at every gate, and the gap is on a path where the missing check is the only thing standing between an operator cancel and a respawned phase.

---

Blocking

1. The explicit-none attestation gate's reject→re-run path has no post-wait cancel check, and the caller's check is unreachable from it. orchestrator/routes/pipelines/_ledger.py:347-393

This PR adds the pre-queue check (:299) and the in-lock park check (:322) to _handle_explicit_none_attestation_gate, but leaves the third leg of its own contract off the branch that actually writes RUNNING and respawns an agent:

attest_resolved = dq.wait_for_decision(attest_decision.id)   # :347
...
resolved_ok = attest_resolved.status == _pkg.DecisionStatus.RESOLVED   # :351
if resolved_ok and not confirmed:                                      # :354
    ...
    with _pkg.get_pipeline_state_lock(pipeline_id):
        pipeline = store.load_pipeline(pipeline_id)
        pipeline.status = _pkg.PipelineStatus.RUNNING                   # :367  ← unguarded
        ...
        _pkg._perform_hitl_phase_rerun(...)                            # :381
    return True, ledger_note, pipeline                                 # :393

There is no _pipeline_cancelled between :347 and :367. And the caller's post-wait check cannot cover it, because the rerun return short-circuits ahead of it:

# _run_hitl_gate.py
if _rerun_requested:
    return pipeline, "continue"          # :360  ← exits here
...
if _pkg._gate_wait_cancelled(store, pipeline_id):   # :374  ← never reached on this path
    return pipeline, "break"

Compare the decision-ledger backstop, which is the same shape and does have the check — _run_hitl_gate.py:269 wait, :270 _gate_wait_cancelled → "break", and only then the not _proceed rerun branch at :288 with its RUNNING write at :306. The attestation gate is missing exactly the check the backstop has 90 lines earlier in the same file.

Failure scenario (concrete, and not a microsecond race — DecisionQueue.wait_for_decision is a while True loop with a 5-second sleep, decision_queue.py:295, so the window is up to one poll interval wide):

1. A phase gate is parked on the explicit-none attestation decision; pipeline status AWAITING_HUMAN.
2. Operator rejects the attestation (picks _LEDGER_BACKSTOP_RERUN_OPTION). The decision goes to RESOLVED.
3. Within the same 5-second poll window, the operator changes their mind and cancels. _update_pipeline_body persists CANCELLED via update_pipeline and sweeps get_pending_decisions() — the attestation is already RESOLVED, so the sweep does not touch it, and _stop_pipeline_event_loops runs.
4. The driver's poll wakes and returns the decision: status == RESOLVED, so resolved_ok=True, confirmed=False → the rerun branch. It loads the pipeline (CANCELLED), writes pipeline.status = RUNNING and phase_execution.status = RUNNING over it, bumps hitl_review_cycles, and calls _perform_hitl_phase_rerun, which clears concurrent state (_clear_concurrent_state) and tears down the live containers. _perform_hitl_phase_rerun has no cancel check of its own — I grepped _hitl_rerun.py, there is no CANCELLED reference in the file.
5. return True, ... → _run_hitl_gate.py:360 returns "continue" → _run_pipeline.py:952 continue → the loop head at _run_pipeline.py:347 reloads the pipeline and reads RUNNING, because step 4 overwrote the cancel. The phase re-runs and respawns agents against a pipeline the operator stopped.

This is #3633 verbatim, surviving on one path, and it is the path this PR touched. _run_pipeline.py:1044's terminal _pipeline_cancelled backstop does not help — the status is no longer CANCELLED.

Fix: add the post-wait check immediately after :347, before the resolution parsing, mirroring _run_hitl_gate.py:270. Because the caller's "continue" return is what makes its own check unreachable, the bail also needs a return the caller stops on — the natural shape is a third return value, or reuse the existing "fail open to the phase gate" return (return False, ledger_note, pipeline) so control reaches _run_hitl_gate.py:374 and converts to "break", which is what the two new guards at :299/:322 already do.

Worth also stating explicitly in _pipeline_cancelled's docstring: the post-wait enumeration at _run_support.py:115-117 lists "_gate_wait_cancelled, at all five of its blocking waits; _await_unresolved_gap_gate; the divergence-reconcile pause" and silently omits the attestation gate's own wait. Right now that omission reads as intentional; it is the bug.

---

Non-blocking

2. Six of the ten new guards have no test. Covered: the phase-gate create-arm pre-queue and the shared park (test_gate_never_mints_a_decision_for_a_cancelled_pipeline, test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it), the helper itself (test_park_at_gate_unless_cancelled_checks_inside_the_lock), the bridge pre-pass-1 (test_bridge_never_queues_for_an_already_cancelled_pipeline), and the divergence park (test_cancel_before_the_pause_is_not_overwritten_by_the_park). Uncovered:

• backstop pre-queue (_run_hitl_gate.py:219) and park (:248) — the _run_gate harness's new on_draft hook fires from _read_phase_draft, which the gate calls at ~:395, after the whole backstop block at :210-340. on_wait(1, cell) reaches the backstop's post-wait check only.
• attestation pre-queue (_ledger.py:299) and park (:322) — _run_gate patches _handle_explicit_none_attestation_gate out entirely, and test_decision_ledger_gate.py's only cancel test (test_cancel_fails_open_with_accurate_note, :562) cancels the decision, not the pipeline before queueing.
• gap-gate pre-queue (_ledger.py:712) and park (:734) — test_unresolved_gap_gate.py has only the pre-existing post-wait test (test_pipeline_cancelled_during_the_gate_keeps_cancelled, :272).

The shared helper is unit-tested, so this is a wiring gap rather than a logic gap — but the wiring is where finding 1 lives, which is the argument for closing it.

3. test_cancel_before_the_pause_is_not_overwritten_by_the_park cannot observe the property it is named for. orchestrator/tests/test_hard_reset_recovery.py:519 — _patch_ctx() does a bare patch("routes.pipelines.get_pipeline_state_lock") (:373), so the lock is an unobservable MagicMock, and _load returns a pipeline whose .status is CANCELLED on every call. The test therefore passes identically whether _alerts.py checks inside the lock, just before it, or 50 lines earlier — the exact distinction its docstring calls "the point". _alerts.py uses a bespoke in-lock check (it has to, so _persist_hitl_decision lands in the same lock), so test_park_at_gate_unless_cancelled_checks_inside_the_lock does not cover it. The events-list pattern from that test (enter → load → exit, no save, _persist_hitl_decision not called) ports over directly.

4. Four gates leave an orphan PENDING decision if the cancel lands between the mint and the park. _run_hitl_gate.py:226→248 (backstop), :544→561 (phase gate create arm), _ledger.py:307→322 (attestation), :719→734 (gap gate). The park check fires and the gate bails before any wait, so this is not the driver-thread hang the PR fixes — but the pending decision was minted after the one-time sweep, so it survives on a CANCELLED pipeline and shows up in the operator's decision list as a live question for a stopped run. _contract_bridge.py:44-48 explicitly documents its equivalent residual window; the four gate sites don't. A best-effort dq.cancel_decision(decision.id) on the _park_cancelled path would close it, or a comment at least records it as known.

5. _park_at_gate_unless_cancelled deliberately does not inherit _pipeline_cancelled's hiccup tolerance — say so. _run_support.py:159-201. _pipeline_cancelled guards store is None and swallows load failures ("Best-effort: a missing store or a load failure returns False"). The new helper does a bare store.load_pipeline(pipeline_id) inside the lock, so a transient store failure raises out to the gate. I think that is the correct default here — failing open would park a possibly-cancelled pipeline, which is the opposite of the safe direction — and it matches the inline park it replaces, so exposure is unchanged. But the docstring positions the helper as the in-lock counterpart of _pipeline_cancelled, and a reader will assume the same posture carries over. One sentence explaining the asymmetry prevents someone "fixing" it into a fail-open later.

6. Stale line counts in the orchestrator/CLAUDE.md row this PR edits. :281 now reads _ledger.py (1,364) — actual 1,271 after this PR's ~256-line removal — and _alerts.py (1,277) — actual 1,359. _contract_bridge.py (299) is 298. Row :277, which the PR does not touch, is stale in the same direction for two files this PR grows: _run_hitl_gate.py (722 → 971) and _run_support.py (376 → 460). Not a cap violation — the barrel at 1,514 lines is allowlisted under #3587, and nothing else is over hard_lines: 1500.

---

Checked and clear

• except _pkg.json.JSONDecodeError, TypeError: (_contract_bridge.py:268) — valid, PEP 758 under requires-python = ">=3.14" / target-version = "py314". Pre-exists on main at four _run_hitl_gate.py sites.
• phase_label is bound at _alerts.py:212 before the new _park_cancelled log call at :295 — no UnboundLocalError. Ordering _park_cancelled ahead of the decision is None arm is right: a cancel is not a persist failure.
• The new if phase_execution is not None guard in the helper is a strict improvement over the inline park it replaces, which would have raised AttributeError.
• _set_status in the gap gate is now RUNNING-only, and the only remaining call (_ledger.py:779) confirms it.
• Both bridge waits (_contract_bridge.py:222, :250) have their post-wait check, and the caller re-checks at _run_hitl_gate.py:834.
• The follow-up site's "the gate's park is still in force" claim (_run_hitl_gate.py:685) holds — nothing restores RUNNING between the gate wait at :581 and the follow-up mint at :698.
• Reuse arm skipping the pre-queue check is fine: it mints nothing, and a swept decision is no longer PENDING, so a cancelled run falls to the create arm and hits :536.

Per repo convention I did not run the suite; CI covers it. Finding 1 is the only one I'd hold the merge for.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg failed to address feedback. View run logs

1 previous review(s) hidden.