Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 1 commit into from
Jul 27, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion orchestrator/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ Pure refactor, no behaviour change: the 27 method bodies are AST-identical to th
| Prompt building: `_prompt_phase.py` (1,407), `_prompt_agent.py` (1,346), `_prompt_review.py` (765), `_prompt_reviewer.py` (593) | Phase-prompt, agent-prompt, review-prompt and reviewer-prep prompt assembly | `build_phase_prompt`, `build_agent_prompt`, `build_review_prompt`, `build_reviewer_prompt` |
| Readers / synthesis: `_criteria.py` (961), `_drafts.py` (758), `_reviews.py` (186), `_context_pr.py` (1,220), `_brc_history.py` (961), `_populate.py` (1,460) | Review-criteria builders, draft-path + source-branch-artifact readers, review-verdict readers, context-PR composition, BRC-history readers, plan-draft synthesis + contract population | `build_review_criteria`, `read_draft_path`, `read_review_verdicts`, `compose_context_pr`, `populate_contract` |
| State / lifecycle: `_slice_state.py` (1,094), `_statefiles.py` (613), `_worktree_sync.py` (1,393), `_slice_completion.py` (135), `_lifecycle_helpers.py` (339), `_status_view.py` (391), `_status_wait.py` (147) | Slice-DAG state helpers, statefile read/write, worktree-sync, slice-completion, lifecycle helpers, status view + long-poll wait | `slice_state`, `sync_worktree`, `complete_slice`, `status_view`, `wait_for_status` |
| Decisions / overseer: `_ledger.py` (1,364), `_decision_candidates.py` (159), `_decisions.py` (259), `_resolve.py` (196), `_hitl_rerun.py` (337), `_overseer.py` (740), `_alerts.py` (1,277), `_pod_liveness.py` (228), `_first_principles.py` (247) | Decision-ledger + gap-gate + apply-handoff, considered-candidate rendering + refine→plan deferral handoff + ledger-summary persistence (#3526), HITL + divergence-reconcile decisions, decision resolution, HITL rerun, overseer detection-plane, divergence/alert/timeout emission, live-pod guarding, first-principles review seed | `register_decision`, `resolve_decision`, `rerun_hitl`, `detect_divergence`, `guard_live_pods` |
| Decisions / overseer: `_ledger.py` (1,364), `_contract_bridge.py` (299), `_decision_candidates.py` (159), `_decisions.py` (259), `_resolve.py` (196), `_hitl_rerun.py` (337), `_overseer.py` (740), `_alerts.py` (1,277), `_pod_liveness.py` (228), `_first_principles.py` (247) | Decision-ledger + gap-gate + apply-handoff, the contract-decision bridge (#1889 — split out of `_ledger.py` to stay under the file-size cap), considered-candidate rendering + refine→plan deferral handoff + ledger-summary persistence (#3526), HITL + divergence-reconcile decisions, decision resolution, HITL rerun, overseer detection-plane, divergence/alert/timeout emission, live-pod guarding, first-principles review seed | `register_decision`, `_queue_and_await_contract_decisions`, `resolve_decision`, `rerun_hitl`, `detect_divergence`, `guard_live_pods` |
| PR / drivers / salvage: `_drivers.py` (263), `_stacked_pr.py` (251), `_salvage.py` (71) | Pipeline-driver lifecycle helpers, stacked-PR assembly, agent-output salvage | `pipeline_drivers`, `build_stacked_pr`, `salvage_agent_output` |

Pure refactor, no behaviour change: every route handler body, helper, and constant is AST-identical to the pre-split file (modulo the sanctioned `_pkg.`-prefixing and the decorator relocation onto thin wrappers). Patch seams preserved: the 16 `@pipelines_bp.route` decorators stay in the barrel so the URL rule → handler map registers identically; the private submodules reach the test-patched module globals via `import routes.pipelines as _pkg`, and the barrel re-exports every externally-referenced symbol across the dominant back-compat import surface (~137 referencing files repo-wide; the audited ~57 distinct `patch("routes.pipelines.<name>")` targets), so both `from routes.pipelines import X` and `patch("routes.pipelines.X")` resolve unchanged — the existing dense seam coverage (`test_consensus_polling`, `test_brc_nack_iteration`, `test_concurrent_*`, `test_advance_phase_*`) stays green. `_run_pipeline` becomes a thin loop delegating to per-phase handlers with no transition-ordering change. **Packaging-neutral:** `orchestrator/routes/` is already shipped by the recursive `COPY orchestrator/routes/ ./routes/` (Dockerfile:45), so the new submodules are auto-included — no Dockerfile change. `pipelines.py`'s allowlist entry — the **LAST** in the program — is dropped, so `scripts/file-size-allowlist.yaml`'s `files:` map is now **EMPTY**: the terminal acceptance criterion of the file-size decomposition program, closing #3312.
Expand Down
5 changes: 4 additions & 1 deletion orchestrator/routes/pipelines/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,9 @@ def stream_pipeline(pipeline_id: str) -> Response:
_refresh_context_pr_body,
_repos_with_slices,
)
from ._contract_bridge import ( # noqa: E402,F401
_queue_and_await_contract_decisions,
)
from ._criteria import ( # noqa: E402,F401
_get_agent_design_criteria,
_get_code_review_criteria,
Expand Down Expand Up @@ -1246,7 +1249,6 @@ def stream_pipeline(pipeline_id: str) -> Response:
_ledger_attestation_rerun_directive,
_next_phases_for_epic,
_persist_phase_gate_resolution,
_queue_and_await_contract_decisions,
_sync_pipeline_decisions_to_contract,
_unwrap_choice_resolution,
_write_apply_phase_handoff,
Expand Down Expand Up @@ -1433,6 +1435,7 @@ def stream_pipeline(pipeline_id: str) -> Response:
)
from ._run_support import ( # noqa: E402,F401
_clear_stale_impasses_for_producers,
_park_at_gate_unless_cancelled,
_parse_resolution,
_pipeline_cancelled,
_spawn_and_wait,
Expand Down
78 changes: 58 additions & 20 deletions orchestrator/routes/pipelines/_alerts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,30 +237,56 @@ def _sync_worktree_reconciling_divergence(
# Persist the reconcile HITL and flip to AWAITING_HUMAN under the
# (reentrant) state lock so a reader never sees AWAITING_HUMAN
# without the pending decision.
#
# The status is re-read from inside that lock and a cancel short-
# circuits both writes. Two things go wrong otherwise: the park
# clobbers the operator's CANCELLED (so the post-wait check below
# reads back our own AWAITING_HUMAN and resumes the run), and the HITL
# is minted after the cancel route's one-time pending sweep, leaving
# ``wait_for_decision`` — an unbounded poll — blocking for the process
# lifetime (#3633 review round 3). The state lock is what makes this
# airtight: ``StateStore.update_pipeline``, the cancel route's
# persistence path, takes the same one.
_park_cancelled = False
decision = None
with _pkg.get_pipeline_state_lock(pipeline_id):
pipeline = store.load_pipeline(pipeline_id)
pipeline.status = PipelineStatus.AWAITING_HUMAN
if phase is not None:
phase_execution = pipeline.get_phase_execution(phase)
if phase_execution is not None:
phase_execution.status = PipelineStatus.AWAITING_HUMAN
store.save_pipeline(pipeline)
decision = _pkg._persist_hitl_decision(
pipeline_id,
pipeline,
store,
question=_pkg._divergence_reconcile_hitl_question(
pipeline_id=pipeline_id,
if pipeline.status == PipelineStatus.CANCELLED:
_park_cancelled = True
else:
pipeline.status = PipelineStatus.AWAITING_HUMAN
if phase is not None:
phase_execution = pipeline.get_phase_execution(phase)
if phase_execution is not None:
phase_execution.status = PipelineStatus.AWAITING_HUMAN
store.save_pipeline(pipeline)
decision = _pkg._persist_hitl_decision(
pipeline_id,
pipeline,
store,
question=_pkg._divergence_reconcile_hitl_question(
pipeline_id=pipeline_id,
phase=phase,
backup_ref=outcome.backup_ref,
local_only_commit_shas=outcome.local_only_commit_shas,
rebase_category=outcome.rebase_category,
rebase_detail=outcome.rebase_detail,
),
options=list(_pkg._DIVERGENCE_RECONCILE_HITL_OPTIONS),
phase=phase,
backup_ref=outcome.backup_ref,
local_only_commit_shas=outcome.local_only_commit_shas,
rebase_category=outcome.rebase_category,
rebase_detail=outcome.rebase_detail,
),
options=list(_pkg._DIVERGENCE_RECONCILE_HITL_OPTIONS),
phase=phase,
context=_pkg._DIVERGENCE_RECONCILE_HITL_CONTEXT,
context=_pkg._DIVERGENCE_RECONCILE_HITL_CONTEXT,
)
if _park_cancelled:
# Checked before the ``decision is None`` arm below: nothing was
# persisted on this path, and a cancel is not a persist failure.
_pkg.logger.info(
"Divergence reconcile pause: pipeline cancelled before the "
"pause was persisted — leaving the persisted CANCELLED "
"intact (#3633)",
pipeline_id=pipeline_id,
phase=phase_label,
)
return outcome, True
if decision is None:
# Could not persist the HITL — fail closed rather than spin on
# a pause the operator can never see.
Expand Down Expand Up @@ -314,6 +340,18 @@ def _sync_worktree_reconciling_divergence(
# spell "stop driving this phase"; the FAILED pin they route to
# is suppressed on a cancelled pipeline in
# ``_fail_pipeline_after_divergence_abort``.
#
# The phase box is deliberately left at AWAITING_HUMAN. Once the
# park above has landed, restoring it would mean a second write to
# a pipeline the operator has already stopped, and the only status
# that would be honest to write is the one already on the pipeline
# record. Leaving it records *where* the run stopped — parked at
# the reconcile gate — which is what an operator reading the phase
# timeline of a cancelled pipeline wants. This is uniform with
# every other gate: ``_gate_wait_cancelled`` and the gap /
# attestation gates all break without restoring their phase box
# either, so a cancelled pipeline consistently renders the gate it
# was sitting at (#3633 review round 4).
if _pkg._pipeline_cancelled(store, pipeline_id):
_pkg.logger.info(
"Divergence reconcile pause: pipeline cancelled while "
Expand Down
Loading
Loading