Bail out of the HITL gate when the operator cancels - #3655
Conversation
The refine/plan gate blocks in wait_for_decision, which the PATCH cancel route unblocks by cancelling the decision. A cancelled decision carries no resolution, and "" is in _APPROVE_KEYWORDS, so the gate read the operator's cancel as an approval: it wrote RUNNING over the persisted CANCELLED and advanced the phase, minting a fresh cohort. That is #3633 through the one path the persisted-status layers cannot see, because they all re-read a status this block has already overwritten. _gate_wait_cancelled checks both the decision's own status and the persisted pipeline status, and is applied at all three wait_for_decision sites in the gate. It returns a new "break" action that the driver maps onto the same loop exit its own CANCELLED check uses, so the finally observes CANCELLED and preserves the worktrees restart_phase resumes from. Also corrects the _pipeline_cancelled docstring (the persisted status is not inviolable — this gate overwrites it), narrows the pre-spawn guard's comment about the integration-branch window, notes that the slice-loop tests pass a production-faithful run_epoch that is not what they exercise, and pins skip_cleanup-on-CANCELLED plus the pre-spawn guard ordering.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #3655 — HITL gate cancel bail (#3633)
The diagnosis is right and the mechanism is right. wait_for_decision polls forever with no timeout, the sweep in the cancel route unblocks it, and _APPROVE_KEYWORDS contains "" so an unset resolution reads as an approval — that is a real hole and _gate_wait_cancelled is the right shape of patch for it.
But the PR is scoped to one of the three blocking waits reachable from _run_hitl_gate, and the two it skips are the ones with the longest human-latency windows. As merged, #3633 still reproduces verbatim through _queue_and_await_contract_decisions. I built standalone reproductions for each of the three findings below; transcripts are inline.
The parts I checked and found correct, so they are not flagged: the _gate_wait_cancelled barrel export at __init__.py:1394 resolves at runtime; the "break" → driver → finally → skip_cleanup chain does preserve worktrees as claimed (_run_pipeline.py:944 breaks out of the while True: at line 326 the same way the loop-head check at 351 does, and the finally at 1347 sets skip_cleanup=True for CANCELLED at 1415-1435); and the _run_concurrent.py comment correction is factually accurate — _run_implement.py:777 calls create_slice_integration_branch before line 892 reaches _run_concurrent_phase_with_impasse_retry, so the pre-spawn guard genuinely cannot prevent integration-branch creation. All 20 tests in test_cancel_stops_driver.py pass.
Blocking
1. _run_hitl_gate.py:655 — a cancel during the contract-decision bridge resurrects the pipeline. #3633 verbatim.
_pkg._queue_and_await_contract_decisions(...) at line 655 blocks in _ledger.py:719 (resolved = dq.wait_for_decision(queued.id)), once per contract question. This is the longest human-latency window in the whole gate — N sequential operator questions — and it has no _gate_wait_cancelled check on either side.
The cancel route sweeps that decision to CANCELLED. _ledger.py:720 does if resolved.status != DecisionStatus.RESOLVED: continue, so resolved_count stays 0, the converge branch is skipped, and control falls through to the "Approved — resume and advance" block at lines 745-753, which writes PipelineStatus.RUNNING over the operator's CANCELLED and marks the phase COMPLETE. The gate returns None, the driver reads that as "advance", and the next phase spawns a fresh agent cohort.
$ python3 /tmp/repro_bridge.py
statuses written by gate: ['awaiting_human', 'running']
action returned to driver: None (None => advance the phase)
RESURRECTED
That is the exact failure #3633 describes — cancelled pipeline, agents keep spawning — and it survives this PR.
2. _run_hitl_gate.py:279 — a cancel during the explicit-none attestation gate erases the cancel and hangs the driver thread permanently.
_pkg._handle_explicit_none_attestation_gate(...) at line 279 blocks in _ledger.py:322. On a non-RESOLVED terminal status it deliberately "fails open to the phase gate", returning (False, "... Attestation confirmation was cancelled; deferring to the phase gate.", pipeline).
Failing open was safe before, when a cancelled attestation just meant one more operator prompt. It is not safe now: the gate proceeds to lines 423-430, writes AWAITING_HUMAN over the persisted CANCELLED under the state lock, then queues a fresh phase_gate decision — created after the sweep already ran, so nothing will ever cancel it — and blocks at line 440 forever.
$ python3 /tmp/repro_attest.py
gate is now blocked forever on freshly-queued decision new-gate
statuses written by gate after the cancel: ['awaiting_human', 'awaiting_human']
Three consequences, all worse than the bug being fixed: the operator's CANCELLED is gone from the store; the driver thread leaks for the process lifetime; and because the thread never leaves the loop, the finally block never runs, so no cleanup and no worktree preservation decision happens at all.
Note the same shape exists at _ledger.py:926-928 (unresolved-gap gate) — wait_for_decision followed immediately by _set_status(RUNNING) before the non-RESOLVED check is even consulted.
3. Pre-existing UnboundLocalError in the block this PR edits.
draft_content and phase_label are bound only at lines 319 and 326, inside the else: arm of if existing_pending_gate:. They are read unconditionally at lines 546 and 549, inside the follow-up branch — the same block the PR adds its new bail to at line 556:
followup = dq.queue_decision(
question=(... f"changes you'd like to see in the {phase_label}, " ...), # 546
context=draft_content, # 549
)Take the existing_pending_gate path (orchestrator restart or driver respawn while parked at a refine/plan gate — routine, not exotic), then have the operator submit a bare "request changes" with no specifics, and the gate raises before it can queue the follow-up:
$ python3 /tmp/repro_gate.py
RAISED UnboundLocalError: cannot access local variable 'phase_label' where it is not associated with a value
Flagging per the stated rule that pre-existing broken behavior in code the PR modifies is blocking. It is also a two-line fix — hoist both assignments above the if existing_pending_gate: branch — and doing it here is cheaper than a follow-up PR, since the PR is already rewriting the control flow of this exact block.
Non-blocking
4. _gate_wait_cancelled's decision-status half fires on a lone decision cancel. routes/decisions/_lifecycle.py:14 exposes a standalone endpoint that cancels a single decision without touching pipeline status. After this PR, hitting it against a live pipeline makes the driver break and exit, leaving the pipeline stranded at AWAITING_HUMAN with no driver and a log line that falsely claims the pipeline was cancelled. Both real cancel paths (PATCH route in _routes_crud.py, and _cancel_pipeline_in_process in routes/decisions/_handlers.py) write the persisted status before sweeping the queue, so the _pipeline_cancelled half already covers them — the decision-status half is defense-in-depth that opens a small hole of its own. Gating it on _pipeline_cancelled (or on the status being terminal) would close it.
5. The load-bearing comment and the _pipeline_cancelled docstring are factually wrong. Both assert the HITL gate is "the one place that overwrites the persisted status." It is not: _ledger.py:926-928 (unresolved-gap gate) and _alerts.py:281+ (divergence-reconcile pause) both do the identical AWAITING_HUMAN→RUNNING overwrite around a wait_for_decision. That claim is what justifies the PR's scope, so it is worth correcting even if the extra sites are handled separately.
6. Test coverage does not match the claim. The PR says all three new bail sites are pinned, but _run_gate only drives the main phase-gate bail at line 455. The ledger-backstop site (line 199) and the follow-up site (line 556) have no integration coverage — and site 556 is inside the block carrying finding #3.
7. FAILED partially bypasses the #1273 carve-out. The PATCH route sweeps pending decisions on FAILED as well as CANCELLED. So the decision-status half of _gate_wait_cancelled fires on a FAILED transition too — including the container_monitor false-positive FAILED that #1273 deliberately carved out. The docstring claims to inherit that carve-out via _pipeline_cancelled; the decision-status half does not.
Findings 1 and 2 are the ones I would not merge without: the PR's stated goal is that a cancel at a HITL gate stops the driver, and through the two uncovered waits it still does the opposite — resurrects the pipeline in one case, permanently hangs the driver in the other. The fix itself extends cleanly: the same _gate_wait_cancelled check on the return path of each of the two ledger calls.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on #3655. The cancel bail is extended from the phase gate to every blocking wait that parks at AWAITING_HUMAN and writes RUNNING back: the decision-ledger backstop, the explicit-none attestation gate, the follow-up specifics prompt, the contract-decision bridge, the unresolved-gap gate, and the divergence-reconcile pause. The divergence-reconcile pause returns aborted=True on a cancel, and _fail_pipeline_after_divergence_abort is now a status no-op on an already-CANCELLED pipeline (pre_event_hook still runs) so an operator cancel is not rewritten as a FAILED pin plus a pipeline.failed broadcast. Also pins the finding-4 case: a lone cancelled decision on a live pipeline must not stop the driver.
|
Thanks — the two blocking findings were real, and the reproductions made them cheap to confirm. All seven items are fixed in this PR; nothing is deferred. Commit: 1.
2. Fail-open to the phase gate is preserved for every other terminal status; the new bail sits between the attestation call and the Your note about 3. Pre-existing Both assignments hoisted above the 4. The decision-status half fires on a lone decision cancel. Removed rather than gated. 5. The load-bearing comment and the Corrected, and — since the claim was what justified the scope — the two extra sites are fixed here rather than separately. The docstring in 6. Test coverage does not match the claim.
One idiom worth flagging for anyone extending these: a shared 7. FAILED partially bypasses the #1273 carve-out. Same fix as 4 — with the decision-status half gone, the carve-out is inherited intact from Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3655 — cancel bail at every park-and-resume gate (#3633)
Delta reviewed: 50fe3ae4..f0a19a08 (one commit, "Bail on operator cancel at every park-and-resume gate"), +646/-103 across 6 production files and 4 test files.
Prior findings 1–7: all addressed, and addressed properly. Details in the verification section below. One new blocking issue found in the code this commit adds.
Blocking
1. _ledger.py:955-966 — the unresolved-gap-gate bail is a cross-module no-op. The driver writes COMPLETE over the operator's CANCELLED twelve lines later, and then deletes the worktrees.
The new bail is correct in isolation: it returns before _set_status(RUNNING), so the persisted CANCELLED survives the gate. But the value it returns is gated = True — the same value the override path and the non-RESOLVED path return. Nothing downstream can tell a cancel from a normal gating, and the driver proceeds straight to the terminal-phase branch.
Trace, all in one iteration of the driver loop:
-
Operator cancels while
_await_unresolved_gap_gateis parked inwait_for_decision(IMPLEMENT phase,hitl_gates=True, open gaps)._routes_crud.py:597persistsCANCELLED, then:633-638sweeps the queue and the wait returns. -
New bail fires →
return gated(True).CANCELLEDintact. So far so good. -
_run_phase_blocks.py:43seesgap_gatedtruthy → commits statefiles and pushes the work branch (:46,:67) — on a pipeline the operator just stopped — thenreturn pipelineat:91. It does not stop the driver and does not re-read cancel state. -
_run_pipeline.py:918→_run_hitl_gate_convergewithcurrent_phase == IMPLEMENT._HITL_GATE_PHASES = {"refine", "plan"}(__init__.py:985), so neither theifat_run_hitl_gate.py:73nor theelifat:142matches, and the function falls through to its last line,return pipeline, None(:862). Action is neither"continue"nor"break", so_run_pipeline.py:945does not break. -
_run_pipeline.py:1017→_next_phases_for_epic(pipeline, IMPLEMENT, transitions.get(IMPLEMENT, [])).PHASE_TRANSITIONS[IMPLEMENT]is[](routes/phases/_transitions.py, terminal since #2777), and_next_phases_for_epicreturnsdefault_next_phasesunchanged for both the non-epic and the epic+IMPLEMENT case (_ledger.py:1027-1054) →[]. -
_run_pipeline.py:1023-1028, with no cancel guard anywhere between:907and:1023:if not next_phases: # Terminal phase — pipeline complete with _pkg.get_pipeline_state_lock(pipeline_id): pipeline = store.load_pipeline(pipeline_id) pipeline.status = _pkg.PipelineStatus.COMPLETE store.save_pipeline(pipeline)
The operator's
CANCELLEDis overwritten withCOMPLETE, andreport_pipeline_status(..., event_type="pipeline.completed", message="Pipeline completed successfully")+_emit_pipeline_event(pipeline, "pipeline.completed")are broadcast. Thenbreak. -
finally(_run_pipeline.py:1352+) re-loads and seesCOMPLETE.run_epochis unchanged (the terminal branch does not respawn), so neitherskip_cleanuparm fires →skip_cleanupstaysFalse→_spawner.gateway.delete_worktrees(container_id=pipeline_id, force=True).
So the operator cancels, and gets: status COMPLETE, a "Pipeline completed successfully" broadcast, and the worktrees destroyed — the exact worktree preservation this PR's own finally comment says restart_phase depends on (#1725).
Relative to the base branch the end state is unchanged (pre-PR the gate wrote RUNNING, and the same COMPLETE overwrite followed). That is the point: the new bail changes nothing observable. Its output is defaulted away by the next consumer. The PR description says the unresolved-gap gate "got the same treatment" as the four gate sites — it did not. The four sites in _run_hitl_gate.py return "break" and the driver stops; this one returns a boolean that already means something else.
The fix has to propagate a stop, not just skip a write. The cheapest shape consistent with the rest of the PR is to have _run_implement_advance re-read cancel state after the gate and hand the driver a stop signal (it already re-loads the pipeline at _run_phase_blocks.py:36), and to guard the terminal-phase branch on _pipeline_cancelled before it writes COMPLETE. The second half is worth doing regardless — an unguarded status = COMPLETE at the end of a terminal phase is a hole any future park-and-resume block in IMPLEMENT falls into, and _run_support.py's new "Any new park-and-resume block belongs on that list" comment is not enough to catch it, because this block is on the list and still loses.
Note also step 3: the commit + push happen after the cancel. Minor next to the status overwrite, but it means a cancelled pipeline still mutates the remote branch.
Non-blocking
2. test_pipeline_cancelled_during_the_gate_keeps_cancelled (test_unresolved_gap_gate.py:271+) cannot catch finding 1, and the assertion it makes is the only one that holds. It calls _await_unresolved_gap_gate directly and asserts gated is True and PipelineStatus.RUNNING not in saved. Both are true on the current code and both stay true under finding 1 — the COMPLETE write happens two modules away, in a driver loop this test never enters. Every other new bail site got a test that drives the real containing function (_run_gate drives _run_hitl_gate_converge; the _alerts.py tests drive _sync_worktree_reconciling_divergence). This one is the exception, and it is the one site where the containing function is where the bug lives. A test that drives _run_implement_advance — or better, one iteration of _run_pipeline — would have caught it.
3. The FAILED sweep now reads as an approval at the phase gate. Dropping the decision-status half (my prior finding 4/7) is the right call and the docstring at _run_hitl_gate.py:33-51 argues it well. The residual: _routes_crud.py:633 sweeps pending decisions on FAILED as well as CANCELLED, _gate_wait_cancelled deliberately does not fire on FAILED (#1273 carve-out), and _APPROVE_KEYWORDS contains "" (__init__.py:988) — so a FAILED transition arriving through the PATCH route unblocks the gate wait and the empty resolution parses as an approval at :510, advancing the phase. I could not name a concrete writer that PATCHes status=FAILED through that route (kubernetes_monitor.py has no PipelineStatus.FAILED), so I'm not calling this confirmed — but the docstring's two bullets explain why bailing on FAILED is wrong without noting that not bailing means silently approving. Worth a sentence there so the next reader doesn't have to re-derive it.
4. The hoist adds a draft read to the reuse path. _read_phase_draft at _run_hitl_gate.py:340 now runs unconditionally, including on the existing_pending_gate branch where it previously never ran. On that branch a missing draft logs "HITL gate: draft not found on work branch" at WARNING for a gate that isn't rendering a draft anywhere. Intentional per the comment at :336-338 ("costs one draft read on the reuse path"), and cheap, but the warning is new operator-facing noise on a path that didn't produce it before.
Prior findings — verification
1 (contract-decision bridge) — fixed, verified. _ledger.py _queue_and_await_contract_decisions takes cancelled: _pkg.Callable[[], bool] | None = None and checks it after both blocking waits (the per-decision loop and the feedback branch), each returning resolved_count early. _run_hitl_gate.py:707 passes cancelled=lambda: _pkg._gate_wait_cancelled(store, pipeline_id) and :725 bails with return pipeline, "break". The annotation is safe — _ledger.py has from __future__ import annotations and Callable is re-exported at __init__.py:14. It is the only caller (grep confirms), so the None default is unexercised, and test_bridge_without_a_cancelled_predicate_answers_every_decision pins that anyway.
2 (explicit-none attestation gate) — fixed, verified. New bail at _run_hitl_gate.py:319-326, placed after _rerun_requested and before the fall-through into the phase gate. test_gate_bails_when_cancelled_at_the_attestation_gate drives it through the real _run_hitl_gate_converge.
3 (UnboundLocalError on the reuse path) — fixed, verified. phase_label / draft_content hoisted to :339-360, at 8-space indent above if existing_pending_gate: (:373), so both arms see them. The regression test genuinely exercises the reuse arm: _gate_pipeline (test_cancel_stops_driver.py:650) seeds pipeline.decisions with a PENDING phase_gate for PipelinePhase.PLAN, so existing_pending_gate at :366 is True and test_bare_request_changes_on_a_reused_gate_reaches_the_followup would raise on the pre-hoist code. Asserting waits == [1, 2] is the right pin — it proves the follow-up was queued rather than just that nothing raised.
4 + 7 (decision-status half over-fires on a lone decision cancel and on FAILED) — fixed, verified. The half is removed, not gated, so _gate_wait_cancelled is now return _pkg._pipeline_cancelled(store, pipeline_id) and genuinely inherits the #1273 carve-out. I verified the load-bearing ordering claim in the docstring: _routes_crud.py:597 persists via store.update_pipeline before the sweep at :633-638, so a swept wait always reads the authoritative status. All five call sites updated to the new (store, pipeline_id) signature. test_gate_still_advances_when_only_the_decision_was_cancelled pins the lone-decision case. See non-blocking 3 for the residual.
5 (docstrings claimed the HITL gate was the only overwriter) — fixed. _run_support.py:91-110 now names all three park-and-resume families and states the extension rule.
6 (test coverage didn't match the claim) — fixed. _run_gate gained _StatusCell plus ledger_status / attestation / bridge / on_wait hooks and now drives all four _run_hitl_gate_converge bail sites through the production function rather than a stub. _StatusCell correctly models a cancel as a store write landing mid-wait, and load_pipeline hands out a fresh object per call so the gate's own AWAITING_HUMAN write can't mask it.
Divergence-reconcile paths — correct. _alerts.py:139-148 early-returns before the FAILED write and the pipeline.failed broadcast while still running pre_event_hook; test_cancelled_pipeline_keeps_cancelled_and_skips_the_broadcast asserts save_pipeline was never called and order == ["hook"]. _sync_worktree_reconciling_divergence's bail returns outcome, True with outcome bound before the while, so no UnboundLocalError. Both consumers of aborted=True stop the driver: _run_pipeline.py:752 breaks unconditionally after _fail_pipeline_after_divergence_abort, and _run_pipeline_setup.py:697 returns pipeline, True → _run_pipeline.py:255-256 return, which is inside the try at :52 so the finally still runs and sees CANCELLED.
Blocking on finding 1. Everything else on this commit is solid work — the four _run_hitl_gate.py sites and both _alerts.py sites are correct, the tests drive production code, and removing the decision-status half rather than patching around it was the right structural call. The gap gate is the one site where "skip the status write" wasn't sufficient, and the current shape makes it look handled when it isn't.
(Note: I did not run the test suite — per this workflow's constraints I only read the code and traced call paths. The findings above are from static tracing with every line quoted.)
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address the round-2 review on #3655. The unresolved-gap gate's bail returned the same `gated=True` an ordinary gating returns, so the driver could not tell a cancel from a normal gating: it fell through to the terminal-phase branch (IMPLEMENT has no successor), wrote COMPLETE over the operator's CANCELLED, broadcast "completed successfully", and — with the finally no longer seeing CANCELLED — deleted the worktrees restart_phase resumes from. _run_implement_advance now re-reads the persisted status after the gate and returns "break", which _run_pipeline maps onto the same loop exit the HITL gate's bail uses. The re-read sits before the post-gate commit+push, so a cancelled pipeline stops mutating its remote branch too. The terminal-phase branch gains its own _pipeline_cancelled guard as a backstop for future park-and-resume blocks that forget to propagate. Also: the FAILED-sweep-reads-as-approval residual is now spelled out in the _gate_wait_cancelled docstring, and the draft-missing WARNING is scoped back to the create arm so the hoist doesn't add operator-facing noise on the reuse path.
|
The blocking finding was right, and the trace was exact — the bail returned a value that already meant something else, and the next consumer defaulted it away. Fixed in this PR; nothing deferred. Commit: 1. The stop is now propagated rather than implied. The second half you asked for is in as well, and independently: the terminal-phase branch now guards on 2. Correct — it asserts the only two things that stay true under the bug. Two new tests drive the containing functions:
3. The Added to the 4. The hoist adds a draft read to the reuse path. The Verification: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3655 — propagate the gap gate's cancel bail to the driver (#3633)
Delta reviewed: f0a19a08..7285d4c8 (one commit, "Propagate the gap gate's cancel bail to the driver"), +356/-22 across 5 production files and 2 test files.
Note: this PR merged as ed77cdee at 2026-07-27T00:44:43Z, before this round completed. Nothing below is blocking, so the merge is not a problem — but the four advisory items are still worth a follow-up.
Prior blocking finding: fixed, and I verified it independently rather than taking the fix on faith. Details below.
Prior findings — verification
1 (the gap-gate bail is a cross-module no-op) — fixed, verified by reproduction.
The stop is now propagated, not implied. _run_phase_blocks.py:51-59 re-reads _pipeline_cancelled after the gate's store.load_pipeline and returns (pipeline, "break"); _run_pipeline.py:907 unpacks the new tuple and :916-923 maps "break" onto the same loop exit the five _run_hitl_gate.py sites use. The re-read is placed before the if gap_gated: commit+push at :64-90, so step 3 of my prior trace — a cancelled pipeline still mutating its remote work branch — is closed too.
I confirmed the fix is load-bearing with a read-only scratch check rather than by reading it: running the two new tests under a pytest plugin that monkeypatches routes.pipelines._pipeline_cancelled to always return False (i.e. the guard absent, everything else production code) reproduces the exact failure from my prior review in the captured log —
INFO orchestrator.pipelines:_run_pipeline.py:1066 Pipeline complete
INFO orchestrator.pipelines:_run_pipeline.py:1484 Pipeline worktrees cleaned up
— on a pipeline whose persisted status is CANCELLED. Both tests fail under the neuter and pass without it. That is a genuine non-vacuity proof for both, independent of the author's claim.
The terminal-phase guard (_run_pipeline.py:1044-1052) landed as well, and correctly: break exits the while True: inside the try, so the finally at :1381 runs, re-loads CANCELLED, and takes the skip_cleanup = True arm at :1452-1469. I also checked the one thing that break skips — the phase-overseer teardown at :1001-1015 — and it is covered: the finally stops overseer_container_id at :1411-1430, so nothing leaks. And /status/wait long-pollers are not stranded by the missing pipeline.completed: the PATCH route emits pipeline.cancelled itself at _routes_crud.py:607 before the sweep.
2 (the old test couldn't catch finding 1) — fixed, verified. test_implement_advance_stops_the_driver_on_a_cancel_at_the_gap_gate drives the real _run_implement_advance over the real _await_unresolved_gap_gate (only load_contract, the decision queue and the store are faked), with the cancel modelled as a store write landing inside wait_for_decision — the shape the PATCH route actually produces. test_terminal_phase_does_not_complete_a_cancelled_pipeline drives one iteration of the real _run_pipeline with a block that bails without propagating, which is the right way to pin a backstop. Patching egg_contracts.loader.load_contract works because _ledger.py:851 imports it inside the function body; the two-element side_effect matches the two _load_open_gaps() calls on the approval path exactly.
3 (FAILED sweep reads as an approval) — fixed. _run_hitl_gate.py:46-56 states it plainly and points the eventual fix at the resolution parsing rather than at a FAILED bail. I re-verified the mechanism it describes: _APPROVE_KEYWORDS at __init__.py:988 is {"approved", "approve", "lgtm", "yes", ""}, and the legacy branch at :573-575 does if resolution.lower() in _pkg._APPROVE_KEYWORDS: _is_approved = True. Accurate.
4 (the hoist added a WARNING to the reuse path) — fixed. existing_pending_gate moved above the draft read and the WARNING is scoped to the create arm. I checked the hoist is semantics-preserving: nothing between the new position (:336-344) and the old one reloads pipeline or mutates pipeline.decisions — only _read_phase_draft, which touches the filesystem and git show, not the store.
ruff check clean on all seven changed files. 4/4 targeted tests pass.
Non-blocking
A. _run_pipeline.py:1044 — the terminal-phase guard reads the status outside the lock the write takes, leaving a narrow TOCTOU window for the exact clobber it exists to prevent.
if _pkg._pipeline_cancelled(store, pipeline_id): # lock-free read
...
break
with _pkg.get_pipeline_state_lock(pipeline_id):
pipeline = store.load_pipeline(pipeline_id) # authoritative reload
pipeline.status = _pkg.PipelineStatus.COMPLETE # unconditional
store.save_pipeline(pipeline)The cancel route's write takes the same lock — store.update_pipeline wraps its load-modify-save in get_pipeline_state_lock(pipeline_id) (state_store/_crud.py:443). A cancel that lands after the lock-free read and before the driver acquires the lock is silently overwritten: the reload inside the lock returns CANCELLED, and the next line ignores it.
The window is small and this is strictly better than the unguarded status quo, so it is not blocking. But the fix is free — the reload inside the lock already holds the authoritative status, so the check belongs there:
with _pkg.get_pipeline_state_lock(pipeline_id):
pipeline = store.load_pipeline(pipeline_id)
if pipeline.status == _pkg.PipelineStatus.CANCELLED:
...log...
break
pipeline.status = _pkg.PipelineStatus.COMPLETE
store.save_pipeline(pipeline)The same shape applies at _run_phase_blocks.py:51, though it matters less there — that guard fronts a commit+push, not a status write.
B. _run_support.py:117 — the _pipeline_cancelled docstring contradicts itself on the site count, four lines apart.
:107-108: "_gate_wait_cancelled, at all five of its blocking waits". :116-118: "the four _run_hitl_gate.py sites … return "break"". There are five: _run_hitl_gate.py lines 233, 337, 534, 635, 758 (grep -c 'return pipeline, "break"' → 5), one per _gate_wait_cancelled call at 226, 330, 527, 628, 751. Since round 1's finding 5 made this docstring the canonical extension rule for new park-and-resume blocks, an off-by-one in it is the kind of thing the next author trips on.
C. The terminal-branch backstop only backstops terminal phases, but the docstring claims it generally.
_run_support.py:119-121 calls the terminal branch "a backstop for the case a future block forgets". It isn't, for a non-terminal phase. A future block that bails without propagating in refine/plan/apply lands on the advance branch instead (_run_pipeline.py:1084+): pipeline.current_phase = next_phase, run_epoch bumped, driver respawned, return. The new thread's loop-head check at :347-351 does stop it — so no COMPLETE, and the epoch bump means the old thread's finally preserves worktrees — but the phase has already been advanced past the cancel point and the epoch rewritten, which is what restart_phase keys on.
No live hole today: the five gate sites and the divergence pause all propagate, so nothing currently reaches the advance branch on a cancel. This is purely about a future-proofing claim that is narrower than it reads. Either scope the sentence to terminal phases, or add the same one-line re-check to the advance branch and make the claim true.
D. _run_phase_blocks.py:53 — the log message misattributes a guard that is broader than the gate.
"Unresolved-gap gate: pipeline cancelled while awaiting the operator" fires whenever the pipeline is CANCELLED at that point, including when the gate never waited at all — a clean contract makes _await_unresolved_gap_gate return False at _ledger.py:873 without queueing anything, and a cancel that landed during the IMPLEMENT phase run is then caught here. That broadening is a real improvement (pre-PR, a cancel during IMPLEMENT also reached the terminal branch and became COMPLETE), which is exactly why the message should say so rather than blaming a wait that may not have happened.
E. _run_hitl_gate.py:369-374 — the comment overstates its own justification.
"warning there would be new operator-facing noise about a draft nothing is about to show" — but the very next sentence notes the placeholder is still bound on both arms because "the follow-up prompt uses it as decision context". So on the reuse arm a missing draft does reach the operator, as the **Warning**: No {phase_label} draft was found… placeholder in the follow-up decision, while the orchestrator log now stays at DEBUG. Restoring the pre-PR log behaviour is the right call and I am not asking for it back; the justification just claims more than it should.
No blocking issues in this delta. The propagation fix is correct, the guard placement (branch rather than block) is the right structural call, and both new tests drive production code across the module boundary where the bug lived — I confirmed that by reproduction, not by reading. Items A–E are follow-up material.
(Per this workflow's constraints I did not run the full suite — 4 targeted tests plus one read-only neutered-guard scratch run, and ruff check on the changed files. CI owns the rest.)
— Authored by egg
|
egg review completed. View run logs 6 previous review(s) hidden. |
1 participant
Address the third review round on #3645.
Blocking — the HITL gate reads an operator cancel as an approval. The
refine/plan gate blocks in
wait_for_decision, which the PATCH cancel routeunblocks by calling
cancel_decisionon it. A cancelled decision carries noresolution, and""is a member of_APPROVE_KEYWORDS, so the gate took its"Approved — resume and advance" branch: it wrote
RUNNINGover the persistedCANCELLEDand advanced the phase, minting a fresh cohort. That is #3633verbatim, through the one path the four persisted-status layers cannot see —
they all re-read a status this block has already overwritten.
_gate_wait_cancelledchecks both the decision's own status and the persistedpipeline status, and is applied at all three
wait_for_decisionsites in thegate (ledger backstop, main gate, follow-up specifics). It returns a new
"break"action that_run_pipelinemaps onto the same loop exit its ownCANCELLEDcheck at the loop head uses, so thefinallyobservesCANCELLEDand preserves the worktrees
restart_phaseresumes from.Also in this round: the
_pipeline_cancelleddocstring no longer claims thepersisted status survives every mechanism (this gate overwrites it); the
pre-spawn guard's comment no longer implies it covers integration-branch
creation; the slice-loop tests thread a production-faithful
run_epochwith anote that it is not the arm under test; and two regression pins the reviewer
asked for —
skip_cleanupon CANCELLED, and the pre-spawn guard reachingbefore
spawn_all.Issue: #3633
Test Plan
orchestrator/tests/test_cancel_stops_driver.pygains a "Layer 5"section (4 tests: the cancel-route bail, the persisted-status bail, a genuine
approval still advancing, and unit coverage of
_gate_wait_cancelledincluding the FAILED carve-out and store-hiccup tolerance) plus the pre-spawn
guard ordering pin.
test_pipeline_failure_path.pygainstest_worktree_cleanup_skipped_on_cancellationnext to its FAILED twin —verified non-vacuous by reverting the CANCELLED arm locally (it fails).
22 targeted tests pass;
make lintclean.Manual Steps
Authored-by: egg