fix(autopilot): observe a plan cancel at every stage-advance gate

[leonlaiyc]

Problem / Motivation

Cancelling a plan does not stop the plan. The user clicks Cancel, the UI posts 🛑 Plan cancelled., and the orchestrator then runs the next stage anyway — against an approval the user has just revoked.

Two independent channels revoke an orchestration run, and they mean different things:

Flag	Set by	Means
slot._stopping	the Stop button (POST /api/chat/slots/{slot}/stop)	the slot itself is being torn down — nothing on it may keep running
tracker.stopped	the Cancel control (api_chat_plan_action, action="cancel") and the typed stop/cancel/abort words	the user revoked approval to keep orchestrating — the slot stays alive, only the plan ends

_stage_loop has four advancement gates, and on main all four read slot._stopping only. Cancel never sets that flag, so no gate observes a cancel. grep tracker.stopped inside the loop on main returns zero readers.

Why it matters

The window that matters is _run_chat. It is the loop's longest await, so it is the likeliest place for a cancel to land — and it is the exact point the loop resumes from directly into the next stage.

So the failure lands on the user who is trying hardest to stop the agent: someone who sees a plan going wrong mid-stage and hits Cancel gets one more full stage of LLM turns and tool calls executed on their behalf, after the product told them the plan was cancelled. Sub-agent tasks are cancelled and _auto_run is cleared, which makes the UI look stopped, so the extra stage is not obviously attributable to the ignored cancel.

The existing test test_cancel_clears_auto_run asserts only slot._auto_run is False — it never asserted that the loop stops, which is why the gap survived.

What changed (motivation → approach → change)

Symptom — a cancel that lands during _run_chat is followed by the next stage running.

Root cause — the four advancement gates read one of the two stop channels. The cancel handler sets tracker.stopped and slot._auto_run, and deliberately not slot._stopping; the gates test only slot._stopping. The two channels were never wired together.

Change — one predicate, _orchestration_stopped(slot, tracker), reading both channels, called from all four gates (top-of-iteration, post-_run_chat, the sub-agent poll condition, pre-capture). Routing them through a single predicate rather than repeating a two-term condition four times is the point: it is what stops the two channels drifting apart again.

Deliberately not the other direction. #4783 offered a second option — have Cancel set slot._stopping. That flag carries session/ACP teardown semantics for paths outside this loop, and cancelling a plan is not a request to tear the session down. Folding one into the other would trade this bug for a semantics conflation.

One gate is deliberately left alone. The all-stages-complete summary still reads slot._stopping by itself. A cancel that arrives after the final stage's gate has already passed leaves a plan whose stages all genuinely ran, and suppressing a truthful completion summary there would be the worse trade. This is called out in the spec so it reads as a decision rather than an oversight.

Not touched: cancellation-handler semantics, slot._stopping, and the auto_run approval gate. With the gates fixed, a cancelled run breaks before reaching that gate, so the parameter-snapshot question #4783 also raises is moot here and stays available for separate work. One production file, one test file, one spec.

docs/system-specs/modules/autopilot.md documented the old single-flag behavior in two places (the step list, and the Stop-and-Cancel table); both are updated in the same commit, per AGENTS.md.

Tests

Three tests added in test/test_orchestrator_cancel_stops_advance.py, each driving the real HTTP cancel handler concurrently with a real _stage_loop — not a simulated flag flip.

Test	Behavior locked in
test_cancel_during_run_chat_does_not_advance	a cancel landing inside _run_chat stops the loop before stage 2
test_cancel_between_stages_blocks_reentry	a cancel taken at the manual-Go approval prompt is observed when the next Go re-enters the loop
test_cancel_during_subagent_wait_does_not_advance	a cancel landing in the sub-agent poll ends the wait instead of spinning

Fail-before / pass-after, measured on pristine main (c505a877) by reverting only the production file:

=== production reverted ===
FAILED test_cancel_during_run_chat_does_not_advance      - cancel landed inside _run_chat but the loop ran [1, 2]
FAILED test_cancel_between_stages_blocks_reentry         - cancelled plan still advanced: ran [1, 2]
FAILED test_cancel_during_subagent_wait_does_not_advance - subagent poll kept spinning after the plan was cancelled
3 failed

=== fix restored ===
3 passed

Each test also asserts slot._stopping is False, which pins the fix to reading the tracker rather than to widening what a plan cancel means — the assertion fails if someone later "fixes" this by conflating the flags.

Two details that produce a green-for-the-wrong-reason test if got wrong, both handled and commented in the fixture:

• The sub-agent check is fail-closed: a missing manager, or running_agents_for returning None, breaks the loop on its own. The fixture wires the permissive case (running_agents_for → []) so the cancel is the only thing that can stop the loop.
• test_cancel_between_stages_blocks_reentry runs stage 1 for real before cancelling. Cancel no-ops when slot._orch_tracker is still None, so a test that cancels before any stage has run cancels nothing and passes vacuously.

The sub-agent test substitutes the poll's sleep via the module reference chat_orchestrator holds, rather than patching asyncio.sleep itself — the substitute awaits the aiohttp client to issue the cancel, so a global patch would reach the very call it depends on.

Regression run — test_dashboard_chat.py plus both existing _stage_loop suites: 654 passed.

Gates: flake8 · isort · scripts/check_black_formatting.py (3 files in scope) · mypy · scripts/docs_lint.py · scripts/check_brand_name.py — all clean.

Manual verification

N/A — unit coverage sufficient: the tests exercise the real api_chat_plan_action handler over a real aiohttp TestClient against a real _stage_loop, so the HTTP cancel path and the orchestration loop are both the production code rather than stand-ins. The only mocked component is _run_chat (the LLM turn), which is what makes the cancel's arrival deterministic instead of a race.

Related Issues

Closes #4783.

Sibling context: the write-window case of this same failure shape was fixed in #3771; umbrella #1783.

Checklist

• Single commit with a Conventional Commits title (fix(autopilot): observe a plan cancel at every stage-advance gate)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (docs/system-specs/modules/autopilot.md, same commit)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

The template carries a placeholder here pending OSPO-supplied wording, so no CLA text is reproduced. Happy to agree to the CLA once it is published in the template.

🤖 Generated with Claude Code

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 733572ef89717c4efff8a76acbbb5f4541e1ca90 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 733572e

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 733572ef89717c4efff8a76acbbb5f4541e1ca90 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered — the base file confirms the description's claims (all four gates on main read only slot._stopping; grep for tracker.stopped under src/kiro_crew/dashboard finds zero readers inside _stage_loop). Here is the review.

First-Principles-Verdict: PASS

A confirmed defect — cancel ignored at every stage gate — fixed at its cause, with nothing riding along and no duplicate mechanism.

What this change ships

Intent: make the plan Cancel control actually stop the orchestration loop instead of letting one more stage run. This is a FIX.

1. Cancelling a plan now stops it before the next stage runs — justified (reported defect, Orchestrator cancel during _run_chat still advances to the next stage (stop-flag split: tracker.stopped vs slot._stopping) #4783; verified on base: all four gates read only slot._stopping)
2. Typed stop/cancel/abort mid-plan now also halt the loop — justified (same channel, chat_handlers.py:590 sets tracker.stopped)
3. A cancel landing during the sub-agent wait ends the wait — justified (same defect, third await site)
4. New helper _orchestration_stopped — justified; 4 consumers in this diff, not a generalization
5. Completion summary still reads slot._stopping alone — declared decision, not an oversight
6. Spec section in autopilot.md updated — mandated (AGENTS.md same-commit rule)
7. Three tests driving the real HTTP handler against the real loop — justified

Counts run: tracker.stopped readers in src/kiro_crew/ outside the setters — exactly two pre-existing (chat_handlers.py:587 gate on the setter itself, slack/gateway.py:4910 subagent-result drop), neither a stage-advance gate, so nothing already did this job. Sibling slot._stopping-only reads in chat_orchestrator.py after the fix: the completion summary (item 5, declared) and the finally-block queue handoff at line 671, which gates slot teardown, not plan advancement — not a sibling of this cause. Zero riders: every hunk is the fix, its tests, or the mandated spec.

[FIRST-PRINCIPLES-REVIEWED] 733572e

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 733572ef89717c4efff8a76acbbb5f4541e1ca90 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A missed-signal bug fixed at its root: one predicate unifies both revocation channels at every gate, with the flag-conflation alternative correctly rejected.

The single-predicate shape is the right one — tracker.stop() writers (cancel handler, typed stop words at chat_handlers.py:590) now reach every advancement gate without handing a plan cancel the session-teardown semantics of slot._stopping, and the deliberately-untouched completion-summary gate is a defensible trade, documented in the spec in the same commit. Tests drive the real HTTP handler against the real loop and pin the non-conflation with slot._stopping is False. No design-level concerns.

[DESIGN-REVIEWED] 733572e

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 733572ef89717c4efff8a76acbbb5f4541e1ca90 via the fork AI-review pipeline; updated in place on each push.

Review details

The change is a narrow, well-grounded correctness fix. Verified independently:

• tracker is guaranteed non-None at all four gate sites (set at lines 197–204 before the loop), and both slot._stopping and tracker.stopped always exist — no crash risk.
• Cancel (api_chat_plan_action) sets tracker.stop() → stopped=True, leaves _stopping alone; the new predicate reads both, so the loop now observes the cancel.
• A persisted stopped=True never wrongly blocks a fresh plan because _reset_auto_run_for_new_plan nulls the tracker (chat_title.py:623).
• The completion-else deliberately still reads _stopping alone, a documented and defensible trade.

The discovery pass produced no candidates, and Step 2 surfaced nothing groundable to the required bar.

No findings.

[OPUS-REVIEWED] 733572e

[bolichen97]

Approved after a description-vs-diff consistency review: every claim in the PR description is backed by the diff, and the diff carries no material change the description leaves unmentioned.