fix(autopilot): enforce the round cap, bound the plan, refuse dead plans

[iamwhatever]

Problem / Motivation

Three items from the autopilot hardening tracker (#1783), all the same shape: the
orchestrator advertises a guarantee that nothing implements.

• MAX_STAGE_ROUNDS was dead code on the dashboard. record_round() returns
  whether the stage has spent its round budget and _stage_loop discarded the
  return value. The "max 3 rounds per stage" the orchestrator prompt promises
  enforced nothing here.
• No whole-plan duration ceiling. OrchestratorConfig carried only
  stage_timeout_seconds, which bounds ONE stage. Ten stages at the 30-minute
  default is a five-hour unattended run with nothing to stop it.
• _capture_stage_result did blocking disk I/O on the event loop — the
  message walk, the redaction of every segment, the mkdir and the write.

And one defect found while working on a fourth item, which is fixed here instead
of that item:

• A restored plan's Go did nothing, silently. mode is persisted and the
  transcript keeps the plan turn's [OPTION: Go | Go All | Cancel] row, but the
  plan shape is in-memory only — so a restored slot renders buttons over a plan
  that no longer exists, and pressing one ran zero stages and returned no
  response at all. Indistinguishable from a hang.

Why it matters

A user who clicks Go All is handing over unattended execution on the strength of
two promises: that a stage cannot spin forever, and that the run as a whole is
bounded. Neither held on the dashboard path. The inert round cap means a stage
keeps spawning waves past its budget; the missing duration ceiling means the
worst case is unbounded once you multiply stages.

The silent Go is smaller but worse to sit in front of: the one thing a user
cannot do with it is tell whether anything is happening.

What changed (motivation → approach → change)

The round cap stops a dashboard plan. Where the rounds come from decides
where the gate belongs: _subagent_done records one round per completed spawn
wave against tracker.current_stage, i.e. while the stage is still running. So
the enforcing gate is the one after a stage's subagent wave, and it is placed
after the result capture so a stage that genuinely finished keeps its result on
disk.

Stage entry no longer spends a round (OrchestrationTracker.start_stage).
This is an off-by-one that only became reachable by enforcing the cap. The loop
entered a stage through record_round, called for its side effects rather than
its count — inert while nothing on this path read the cap, but once the cap IS
read that tick spends a third of the budget before any subagent runs: a dashboard
stage would be cut after two waves while the identical stage driven from
_subagent_done (no stage loop, so no entry tick) got three. Stricter than the
prompt promises and inconsistent between the two paths. start_stage registers
the stage at zero rounds and restarts the stage clock; counting stays in
record_round, so all three rounds belong to actual waves.

The cap halts auto-run only, like the watchdog and for the same reason: it
bounds an unattended plan. An attended stage that spent exactly its 3 allowed
waves and finished has done nothing wrong and is about to be offered Go anyway;
halting it said "Auto-run stopped" on a plan never in auto-run and skipped the Go
row, stranding the step-through (Opus round 8, chat_orchestrator.py:988).

The cap is a stage-boundary halt, not a per-spawn denial — deliberately.
Rounds are recorded when a wave completes, so within one stage turn the model
can issue a further wave before the boundary check runs (GPT round 7,
chat_orchestrator.py:995). Closing that would mean a hard gate inside the spawn
tool that reads the orchestration tracker: a new control layer in the spawn path,
which #1783 does not ask for, and which every spawned agent's own governance gate
already bounds. The boundary halt is the guarantee the prompt actually promises
("auto-run stops"), it is user-visible, and it is exactly what the Slack path has
always had. Overridden rather than built, for the same reason the persistence
item was withdrawn: the finding is real and the fix is out of proportion to it.

MAX_STAGE_ESCALATIONS is deliberately not checked here, and that is a
reachability fact rather than a preference. Escalations are only recorded by
reset_after_guidance, which zeroes the capped stage's rounds while KEEPING its
key — so current_stage (the highest key) does not move, the loop's next entry
starts at the stage after it, and an escalated stage is never re-entered. Nothing
on this path can observe is_force_failed, so a check here would be dead code:
the very defect this PR is fixing. It stays enforced in slack/gateway.py, where
the tracker is not driven by a stage loop.

Whole-plan watchdog. orchestrator.max_plan_duration_seconds (default 2 h,
0 disables) is checked at each stage boundary, with one warning at 75% latched
inside the tracker. At the boundary rather than mid-turn: the running stage
already has its own ceiling, and cutting between stages leaves every finished
stage captured on disk.

Auto-run only. The clock is wall-clock from the plan's first stage, and a
stage-gated plan spends most of it parked at an approval prompt — so the ceiling
cut a plan the user was actively stepping through, counting their own review time
between Go clicks against them. The budget exists to bound unattended runtime;
when the user clicks each stage they are the ceiling. Gated on the loop's
auto_run parameter rather than slot._auto_run, because the slot flag is
cleared by every halt path and would make the gate depend on whether something
had already gone wrong.

The budget load is a question about the tracker, not about the loop. It was
gated on tracker is None — "did this loop create the object" — so a tracker the
loop did not build ran the whole plan on constructor defaults, with the new plan
watchdog sitting at 0, which means DISABLED. That tracker is real: the Slack
gateway creates one lazily when a subagent result lands on a slot the loop has
not reached. The tracker now answers budgets_unset, and mark_budgets_loaded()
is recorded even when the load raised, so one bad config read cannot become one
per stage-loop entry. A failed load lands both budgets on OrchestratorConfig's
dataclass defaults rather than leaving the ceiling at 0.

Two things ride along. context_management.py and
test_completion_result_read_off_loop.py leave .github/black-baseline.txt:
both became black-clean as a side effect of formatting the code this change
touches, and the gate requires a file that has become clean to be pruned. Not
split into its own commit because the readiness guard asserts a single commit on
base.

One user-visible rendering change rides along. timeout_human inlined
minute/second formatting; extracting _human_secs so an hour-scale plan budget
can render 2h added an hours branch that the existing per-stage timeout
text now goes through too — a stage timeout configured at 3600s used to print
60m and now prints 1h. Nothing else about that message changed and no caller
parses the string. Kept rather than reverted, because without the hours branch
the 2 h plan budget would render 120m.

Capture off the loop, split at the boundary the repo already uses.
_collect_stage_result_parts walks the assistant messages on the loop, because
slot.messages is live state the loop mutates, and hands an immutable tuple of
raw strings to _write_stage_result on a worker, which redacts and writes. So
the redaction pass moves off the loop too, and nothing mutable is reachable from
that thread — the same split as _previous_result_paths / _read_previous_results.
_capture_stage_result is deleted: it was retained "for callers that are not
on the event loop" and there were none (one # noqa: F401 re-export plus test
files), so the retention rationale was fiction.

A plan whose stages are gone is refused out loud. _stage_loop posts
⚠️ This plan is no longer active …, logs auto_run_plan_expired /
plan_shape_absent, closes the turn out (chat_done, slot.task = None) and
returns — before the tracker is built, so before the config is read. Not gated on
auto_run, so Go All is refused the same way. The same gate covers a planning
turn that parsed no stages, which arrives in the identical state, so the message
names the state rather than a cause.

Scope change: cross-restart plan persistence is withdrawn

This PR opened with a fourth item — persisting plan state so a restart resumes.
That was implemented, reviewed over five rounds, and has been removed rather
than landed. It is worth stating why in the open, because the item is on #1783
and stays open there.

The review record. Nine of the eleven findings raised on this PR came from
that one item, and every one of them was the same class: a persisted record is
untrusted input and the design kept treating it as data it had written itself —
str()-coerced result values that let a hand-edited record skip a stage that
never ran, an unconfined result path that inlined any readable file into the next
stage's prompt, a snapshot() that iterated live ledger dicts from a worker
thread, bool("false") reading as started. Each was fixable and each was fixed.
The last one was not: a crash between recording a stage result in memory and the
next slot save leaves a record that says a stage finished when its work is gone,
or the reverse, and there is no answer to that inside the persistence design —
only a durability contract the module does not have.

The boundary that actually resolves it. Autopilot is a lightweight executor
of a plan the user is watching, not a task runner that owns work across process
lifetimes. Resuming means restoring an execution ledger — which stage ran, how
many rounds it spent, which results are real — and every restored fact is a way
to re-run a completed stage's side effects or to skip a stage that never ran. A
plan is cheap to re-ask for; a mis-resumed plan is not.

What the module owes the user is therefore honesty, not continuity. That is
the refusal above: the plan shape stays in memory, a restart ends the plan, and
the button says so instead of doing nothing. stage_*_result.md files still
survive on disk and the message says so.

Removed with it, because it becomes unreachable: the escalation-cap entry gate
(it could only be reached by a restored tracker) and the plan metadata field,
snapshot(), from_snapshot(), resume_stage(), _plan_state_for_save and
_restore_plan_state. chat_persistence.py and history.py are back to
origin/main byte-for-byte.

Kept from that work, because it fixes a pre-existing hole unrelated to
persistence: budgets_unset / mark_budgets_loaded, which is what stops the
gateway's lazily created tracker from running a plan on constructor defaults.

Tests

41 tests across four new files, each proven red on this tree before the fix.

• test/test_stage_round_cap_enforced.py (9) — a stage whose waves spend its
  round budget halts the plan, stops auto-run, keeps its result on disk (the
  ordering assertion, paired with the halt so it cannot pass vacuously), and is
  audited. Plus the off-by-one guard: two waves per stage must run the plan
  through, which fails the moment stage entry starts spending a round again, and
  the tracker-level pair — start_stage registers at zero rounds, and the full
  MAX_STAGE_ROUNDS is spendable after entry. And an attended stage at exactly
  the cap still gets its Go row rather than a halt.
• test/test_plan_duration_watchdog.py (21) — the clock starts with the plan and
  is not re-armed per stage; timeout, disabled (0), and the latched 75% warning;
  the loop halts at the boundary naming the budget and elapsed time; finished
  stages stay on disk; a stage-gated plan driven through a second Go with the
  clock already past the budget is not cut, while the auto-run cut still fires;
  the config default is 2 h; a tracker the loop did not create loads both budgets,
  one that already carries them does not reload, and a failed load is not
  re-attempted.
• test/test_stage_result_write_off_loop.py (7) — the write, the mkdir and the
  redaction all run off the loop thread; the message walk stays ON it (live slot
  state must not be reachable from the worker); plus preservation of file content,
  the stage-separator boundary, and redaction before disk.
• test/test_expired_plan_is_refused.py (4) — the refusal is audible rather than
  silent, closes the turn out (chat_done, slot.task, slot list), costs no
  tracker and no config load, and applies to Go All identically.

Two existing files move with the code. test_completion_result_read_off_loop.py
moves its intercept from _capture_stage_result to _write_stage_result —
against the split its substitute would never run and it would pass vacuously.
test_orchestrator_config_load_off_loop.py's fixture now carries one stage
title: it used an EMPTY plan to reach tracker initialisation, and the new refusal
turns an empty plan away before anything is built, so every assertion in that
file would have passed vacuously. Its subjects are unchanged.

Manual verification

N/A — unit coverage sufficient. Every path is exercised through the real
_stage_loop rather than mocked seams; the off-loop assertions wrap the actual
syscalls (Path.write_text, Path.mkdir) so they record the thread that
genuinely performed the I/O.

Gates run locally on the rebased head: black (the repo's baselined gate),
isort, flake8 (src/ + test/), mypy src/kiro_crew/ (1296 files clean),
docs_lint, and 885 tests across the orchestrator / tracker / dashboard-chat
families.

Related Issues

Related: #1783

no linked issue: #1783 is an umbrella tracker with items this PR deliberately
leaves open, so a closing keyword would close it while most of its checklist is
still unchecked.

This PR covers two P1 items (the dead round cap, the missing total-plan watchdog)
and one P2 item (the blocking capture). Cross-restart plan persistence — also P1 —
is withdrawn by design (see the scope-change section); #1783's checklist entry
for it stays unchecked, and the reasoning above is the argument for closing it as
won't-do rather than for keeping it open.

#8798 is now moot and should be re-scoped or closed: it proposed deriving the
stage loop's start index from resume_stage(), which no longer exists.

Also left for follow-up, unchecked on #1783 rather than silently dropped:

• P1: the recovery-retrigger cap never resetting at stage boundaries
  (gateway.py _retrigger_recovery); stage-1 timeout initialisation.
• P2: an empty stage counting as done; the 2 s subagent busy-wait; linear growth
  of previous-result injection; _pending_synthesis armed but never consumed in
  orchestrator mode; one-member-per-sweep stuck-wave reconciliation.
• All of P3.

Pattern harvest

Rule candidate: review-prompt

Pattern: a limit is only as real as the unit it counts, and wiring up an
ignored limit re-opens the definition of that unit. Enforcing
MAX_STAGE_ROUNDS was a two-line change; the defect it created was that the loop
had been calling record_round for its side effects for as long as the counter
was ignored, so switching the counter on silently repurposed a bookkeeping call
as a budget spend — and made one path stricter than both the prompt and the other
path. The proposed rule: when you make a previously-ignored counter load-bearing,
enumerate every existing call site and say, for each, whether it is an instance
of the thing being counted. Its companion, from the same change: state a gate as
a property of the object being gated (does this tracker have its budgets) rather
than of the code path that reached it (did I create this tracker) — the two
agree everywhere except the path you are adding.

Second, on scope rather than code: nine of eleven findings landing on one item
of a four-item PR is a design signal, not a run of bad luck. Every individual
finding on the persistence item was legitimate and fixable, and fixing them one at
a time is what kept the item alive for five rounds; the count was the thing worth
reading. The rule: when findings concentrate that heavily on one item, stop fixing
and ask whether that item's contract is the problem — and prefer withdrawing it
from the PR over converging it, since the other items are then reviewable on their
own merits.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title
• 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: the stage
  loop's new gates and the plan-shape refusal, start_stage, the split
  capture, the limits table with the escalation-cap reachability note, and
  the restored "plan progress is not persisted" limitation)
• No secrets, credentials, or internal references in the diff

Round 8 — advisory residue taken, one blocking finding overridden

Design Review and First Principles both PASS/CONCERNS; their notes are taken:
two stale persistence comments deleted (chat_orchestrator.py budget-load gate,
the orphaned Persistence across a gateway restart header), _round_cap_verdict
→ _round_cap_message returning just the string (its operation was a constant
with one consumer), redaction dropped from _halt_plan / _dead_msg /
_warn_msg (every input is an integer, a humanized duration or a literal — the
"model-authored" rationale was false), the baseline prune declared above, and the
attended→Go All clock asymmetry recorded in the spec as deliberate.

GPT's blocking finding (per-spawn cap enforcement) is overridden with the
reasoning in the round-cap section above.

Round 9 — Opus advisory taken; the remaining red is inherited from main

The round-cap halt is now gated on auto_run (see the round-cap section).

Round 10 — review lanes all green; one inherited red on main

On 5f1613d3d (rebased past #8844, which closed the members.py red from
#8846) all four review lanes pass: Design Review PASS, First Principles PASS,
Opus "No findings", GPT human override accepted. The only failing check is
Backend Tests (3.12, 4), one test — test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists::test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery
— which this PR does not touch (its diff is confined to context_management.py,
chat_orchestrator.py, config/* and their tests), which passes locally 3/3,
and which is red on main itself at 002081169 (already in this branch's base).
Tracked as #8915; it was introduced by #8576. This PR goes green when that is
fixed on main and this branch is rebased onto it.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 5f1613d3d45ac754aa1f4412cd89c0a404e944c5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Three inert guarantees made real at the boundaries the tracker already owns, with the unsalvageable fourth item (persistence) withdrawn instead of converged — the right call.

Watch

• max_plan_duration_seconds defaulting to 7200 retroactively cuts any existing operator's legitimately long unattended plan on upgrade ("Ten stages at the 30-minute default is a five-hour unattended run" was previously permitted behavior). The cut is clean, warned at 75%, and 0-disableable, so it is acceptable — but it is a changed default users may have configured around, and must land in the release's ### Before you upgrade section, not only in the spec.

[DESIGN-REVIEWED] 5f1613d

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @iamwhatever overrides the GPT 5.6 finding for 5f1613d3d45ac754aa1f4412cd89c0a404e944c5; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 5f1613d3d45ac754aa1f4412cd89c0a404e944c5: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 5f1613d3d45ac754aa1f4412cd89c0a404e944c5 — 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 checks are done. Every candidate finding died on verification: the manual append+broadcast pair in _halt_plan matches the file's 10 pre-existing sites (and append_and_surface emits a different frame type); format_ttl can't render seconds so it can't replace _human_secs without changing existing text; the watchdog config is an ACP liveness oracle, not a runtime ceiling; every new tracker method has ≥1 production consumer; reset_after_guidance does zero _stage_start, so the record_round comment's premise holds; and both riders are declared and mandated by the documented black-baseline gate.

First-Principles-Verdict: PASS

Three advertised-but-unimplemented guarantees made real, one declared bounded addition, riders declared and gate-mandated; every item names its harm and sits at cause level.

What this change ships

Intent: make the autopilot's unattended-run limits actually hold (round cap, whole-plan ceiling), refuse dead plans out loud, and stop stage capture blocking the gateway — a FIX carrying one declared addition (the plan ceiling, tracker item #1783).

1. Auto-run halts once a stage spends its 3 spawn rounds — justified (cap was recorded, never read)
2. Entering a stage no longer spends a round; all 3 belong to waves — justified (Slack-path parity)
3. New orchestrator.max_plan_duration_seconds, 2h default, cuts auto-run at a stage boundary — declared addition, justified
4. One warning at 75% of the plan budget — justified, rides with 3
5. Go on a restored dead plan gets a refusal instead of silence — justified; symptom level and the unpersisted cause are documented in Limitations
6. Budget load keyed on the tracker, not loop bootstrap; failed load lands on defaults, once — justified
7. Stage-result redaction and write moved off the event loop; _capture_stage_result deleted — justified, net surface shrink
8. Stage-timeout text now prints 1h instead of 60m — declared ride-along
9. Two files pruned from the black baseline, formatting-only hunks — declared ride-along, gate-mandated
10. Three new audit event types for the halts and the refusal — justified (SEL audit floor)

Duplication checked and cleared: format_ttl (messaging/commands.py:306) renders no seconds; WatchdogConfig is a liveness oracle, not a ceiling; the append+chat_append pair matches 10 pre-existing sites in chat_orchestrator.py. budgets_unset, start_stage, round_limit_reached each counted ≥1 production consumer.

[FIRST-PRINCIPLES-REVIEWED] 5f1613d

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5f1613d3d45ac754aa1f4412cd89c0a404e944c5 — this comment is updated in place on each push.

Review details

I've now examined both candidates against the actual code. Let me verify one final point on candidate 1 — whether the create_task auto-run path can coexist with a queued message.

The key facts I've established from the code:

Candidate 1 (dead-plan early return skips the finally's queue-drain and note-flush): The early return at chat_orchestrator.py:467 does perform the idle-close (chat_done, slot.task = None, push_slots_update), but skips flush_deferred_notes() and the _start_next_queued_turn handoff. However:

• The create_task(_stage_loop(...)) path in api_chat_plan_action (line 1287) only runs when not slot.running (line 1233 diverts a running slot to queue_append + return). So at loop entry there is no pre-existing in-flight turn, hence no queued message from a running turn.
• Restart case → _queue and deferred notes are in-memory, so both are empty; skipping the drain/flush is a no-op.
• An idle slot processes cron/subagent events rather than deferring them, so no held notes exist at entry.
• The only way a queue entry appears is a message racing into the microsecond window between create_task and the coroutine executing the gate — a "might," not an input that occurs in practice. The candidate itself concedes it could not confirm this.

So (a) a concrete reachable input producing the stranded-message/lost-note outcome is not established; (c) is a no-op on the reachable paths. Fails the 80+ bar.

Candidate 2 (final stage hitting round cap shows "send guidance" instead of "complete"): Reaching the cap on the last stage produces the halt message rather than the completion summary. Whether that message is wrong on the final stage is a product judgment (the stage did spawn all 3 waves, so "struggling, ask for guidance" is defensible) — not a crash, data loss, or security hole. The candidate's own confidence is "low" and it names this as an unsettleable product judgment. Fails the bar.

No new grounded finding surfaced while falsifying these.

No findings.

[OPUS-REVIEWED] 5f1613d

Verdict parsed from the review's SHA-scoped output markers for commit 5f1613d3d45ac754aa1f4412cd89c0a404e944c5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 5f1613d3d45ac754aa1f4412cd89c0a404e944c5: <one-sentence reason>

[iamwhatever]

Disposition: fixed — span=e5002d2b7419

• Invalid result values mark unfinished stages complete (src/kiro_crew/context_management.py)

Legitimate, and the consequence is worse than "invalid data": it is silent loss of
completed-looking work. The finding holds exactly as written.

from_snapshot coerced every stage-result value with str, which has no failure
mode — None became the truthy string "None", 0 became "0", and "" was
still installed as a key. resume_stage() decides a stage finished from the mere
PRESENCE of its key, so any of those made a resumed plan step straight over a
stage that had never run. Nothing downstream can detect it: the plan reports
complete and the skipped stage's work simply never happens.

Fixed in b652e6fda. from_snapshot now validates instead of coercing:

• a stage result must be a non-empty string (_result_path); anything else is
  rejected rather than stringified;
• a round or escalation count must be a non-negative int (_counter), with
  bool excluded because a JSON true here is a malformed record, not a count of
  one;
• a rejected entry is dropped, which leaves its stage absent from
  _stage_results — so the stage is re-run. That is the safe direction, and the
  gap also bounds resume_stage(), so a valid result sitting beyond a rejected one
  is not credited forward.

Regression coverage, all red before the fix: test_plan_state_survives_restart.py
now parametrises every unusable result value (None, 0, 1, "", whitespace,
True, False, list, dict, float) and asserts the stage is neither marked complete
nor present in the ledger, plus the credited-forward case and the counter cases
(-1, "3", True, 2.0 rejected; 0 kept).

[iamwhatever]

Disposition: fixed — span=e0b561a8bdb8

• if _bootstrapping skips budget loading for restored trackers, leaving the whole-plan timeout disabled after restart (src/kiro_crew/dashboard/chat_orchestrator.py)

Legitimate, and it disabled this PR's own new guard on the path this PR's other half
creates — the persistence work is what produces a non-None tracker at loop entry.

_bootstrapping = tracker is None answers "did this loop construct the object",
which is not the question the load needs answered. A restart-resumed plan enters
_stage_loop with a tracker rebuilt by from_snapshot, so the load was skipped:
_plan_timeout stayed 0, and 0 means DISABLED, so the whole-plan watchdog
never fired for the rest of that run. _stage_timeout likewise kept the
constructor default instead of the configured value.

Fixed in b652e6fda, and the suggested shape ("also load when the tracker has no
process-local plan start") is not quite what landed, because a bare
not tracker._plan_start also matches a tracker whose budget a caller set
deliberately — test_existing_tracker_does_not_reload_config pins that a tracker
carrying an explicit stage_timeout_seconds=77 must keep it. So the predicate moved
onto the tracker itself:

• OrchestrationTracker.__init__ now takes stage_timeout_seconds: int | None = None
  and records budgets_unset = stage_timeout_seconds is None, so "no budget was
  provided" is distinguishable from "a budget equal to the default was provided";
• the loop gates on tracker.budgets_unset, so a tracker built by from_snapshot
  or created lazily by slack/gateway.py when a subagent result lands both get
  their load, while an explicitly-budgeted tracker and a paused plan's later Go
  still pay for nothing;
• mark_budgets_loaded() is recorded even when the load raised, so one bad config
  read cannot become one per stage-loop entry.

Regression coverage, red before the fix: a from_snapshot tracker resumed through
the real _stage_loop now ends with max_plan_duration_seconds == 66 and
stage_timeout_seconds == 55 from config; a tracker that already carries budgets
performs zero KiroCrewConfig.load calls; and a raising loader is attempted exactly
once across two loop entries.

[iamwhatever]

Disposition: fixed — span=a83d963f6dea

• if _bootstrapping and not await _load_plan_budgets(slot, tracker): gates the config load on _bootstrapping = tracker is None, so a restart-resumed plan runs on constructor defaults (src/kiro_crew/dashboard/chat_orchestrator.py)

Legitimate, and the consequence chain completes exactly as traced: from_snapshot
calls cls(), so the resumed tracker arrives with _plan_timeout=0 and
_stage_timeout at the constructor default; the load is skipped because the tracker
is not None; is_plan_timed_out() short-circuits on a falsy budget; the whole-plan
watchdog is therefore inert for the entire resumed run, and the stage budget ignores
config.

The gate asked whether the loop had just built the tracker, where the invariant it
needed was whether the tracker has its configured budgets. Those two agree on
every path that existed before this PR, and disagree on the one it adds.

Fixed in b652e6fda by moving the predicate onto the tracker rather than widening
the branch: OrchestrationTracker now records budgets_unset (set when no
stage_timeout_seconds was supplied at construction) and the loop gates on
tracker.budgets_unset. That also covers the sibling arrival this PR did not
create — the tracker slack/gateway.py builds lazily when a subagent result lands on
an orchestrator slot — while leaving an explicitly-budgeted tracker untouched, which
test_existing_tracker_does_not_reload_config requires.

One existing test moved with it, and deliberately:
test_orchestrator_config_load_off_loop.py's resume-path control asserted
threads == [] ("the resume path must not load the config at all") while building
its tracker as a bare OrchestrationTracker(). That is precisely the
never-had-budgets shape this finding says DOES owe a load, so the fixture now passes
an explicit budget — which is what an in-process resume actually holds, since entry
one loaded them onto the same object entry two re-uses. Both of its assertions are
unchanged; the restart-resume shape is covered by a new test instead.

[iamwhatever]

Disposition: fixed — Design Review Watch item 1: the two headline features don't compose; a resumed plan skips the budget load

Legitimate, and the framing is the sharpest of the three reports on this defect:
the two features genuinely did not compose, because the persistence half is what
produces a non-None tracker at loop entry and the watchdog half only loaded its
budget when the tracker was None.

_stage_loop applied configured budgets only when _bootstrapping = tracker is None, while _restore_plan_state pre-sets slot._orch_tracker and
from_snapshot builds via cls() with _plan_timeout = 0. The resume Go — the
exact long-unattended run the watchdog targets — therefore executed with the plan
ceiling off and the stage timeout stuck at the constructor default, contradicting
snapshot()'s own claim that budgets are re-read from config on the next run.

Fixed in 5e9b43e79, gated on budgets-unset as suggested rather than run
unconditionally, because unconditional would clobber a budget a caller set
deliberately (test_existing_tracker_does_not_reload_config pins an explicit
stage_timeout_seconds=77). OrchestrationTracker.__init__ now takes
int | None and records budgets_unset = stage_timeout_seconds is None, so "no
budget supplied" is distinguishable from "a budget equal to the default"; the loop
gates on tracker.budgets_unset. That also covers the arrival this PR did not
create — the tracker slack/gateway.py builds lazily when a subagent result lands
— and mark_budgets_loaded() is recorded even when the load raised, so one bad
config read cannot become one per loop entry.

The "no restart test covers budget reload" gap is closed too: a from_snapshot
tracker driven through the real _stage_loop now ends with
max_plan_duration_seconds == 66 and stage_timeout_seconds == 55 read from
config, with companion tests for no-reload and load-attempted-once.

[iamwhatever]

Disposition: fixed — Design Review Watch item 2: tracker.started ignores escalations, so a force-failed stage restores as "not started"

Legitimate, and this one was found only by this lane. It defeated a guarantee the
PR states in its own body, so it is in scope regardless of the advisory verdict.

started checked only rounds and results, and _restore_plan_state DISCARDS the
tracker when it answers False. A stage-1 force-fail reaches a restore with
neither of the other two signals: reset_after_guidance zeroes that stage's
rounds as it increments the escalation, and from_snapshot drops the rounds of a
stage that produced no result. Rounds and results are both empty, the escalation
ledger is the only trace, the tracker is thrown away, and the transcript's
original Go re-runs the stage on a fresh ledger — the restart-launders-the-cap
path the PR claims restoring escalations whole prevents.

Fixed in 5e9b43e79 exactly as suggested: started now reads
bool(self._stage_rounds or self._stage_results or self._stage_escalations), and
its docstring records why escalations are part of the test rather than extra
evidence — they are the only signal that survives this shape.

Two regression tests, both red before the fix:

• the tracker-level shape — record_round(1), _stage_escalations[1] = 2,
  reset_after_guidance(), then from_snapshot — asserts empty rounds, empty
  results, started is True, and is_force_failed(1) is True;
• the same record through _restore_plan_state asserts the tracker is published
  (slot._orch_tracker is not None) and that stage 1 is the stage offered, so the
  force-fail gate on loop entry now fires instead of a clean re-run.

The ARMED-vs-RUNNING distinction started exists for is separately pinned: a
tracker with nothing recorded still answers False and still yields no resume offer.

[iamwhatever]

Disposition: rebutted — First Principles subtraction 1: drop PLAN_WARN_FRACTION, plan_warning_due(), _plan_warned and the warning block

Kept, on the ground that it is the requested shape of the acceptance item, not
scope this PR invented. #1783 specifies item (c) verbatim as:

No total-plan duration watchdog. … Suggested: max_plan_duration_seconds
(default ~2 h) checked at each stage boundary, warning at 75%.

So removing it would ship item (c) with a piece of its own definition missing, and
the next reader of the tracker would find the box checked and the warning absent.
That is a worse outcome than 25 lines of notice code.

On the merits of the premise, the review is right that there is a tension and I do
not want to paper over it:

The 75% warning's stated harm ("intervene before the cut") assumes a watching
user, while the same PR argues a restored plan must not auto-resume because
"nobody is watching"; its zero option costs nothing — the halt is at a boundary
and its own tests prove every finished stage stays captured and resumable.

Two things separate the cases. Go All is an attended decision at t=0 — the
user is present when they arm it — whereas a restart lands with nobody present by
construction; "do not silently resume unattended" and "tell the user before the
ceiling" are not the same claim about the same moment. And the notice is a
transcript row, so it does not require anyone to be watching at 75%: it is there
whenever they next look, which is the same delivery channel as the halt row the
review credits.

I accept the cost accounting: the zero option is cheap because the halt is
non-destructive. That is why this is a rebuttal about scope fidelity, not a claim
that the warning is load-bearing for safety. If a maintainer would rather item (c)
land without it, say so and I will drop the four pieces in one commit and note the
deviation on #1783 — that is a scope ruling, not a code question.

[iamwhatever]

Disposition: fixed — First Principles subtraction 2: delete _capture_stage_result and the chat.py re-export; zero non-test callers

Taken. The consumer count is correct and, more to the point, the retention
rationale I wrote was false:

_capture_stage_result's retention claims "callers that are not on the event
loop"; grep of _capture_stage_result finds 0 such callers — one # noqa: F401
re-export (chat.py:90) and 2 test files.

Verified independently before acting: grep -rn _capture_stage_result src/ test/
returns the definition, the chat.py:90 re-export, two call sites in
test_display_time_redaction.py and test_stage_result_write_off_loop.py, and two
docstring mentions. No production caller anywhere.

Deleted in 5e9b43e79, together with the chat.py re-export. The two tests now
compose the halves directly — _write_stage_result(slot.key, 1, _collect_stage_result_parts(slot)) — which is exactly the sequence _stage_loop
performs, so they exercise the real path rather than a wrapper that existed only for
them. Their assertions are unchanged: redaction still happens before the bytes reach
disk, and the composed output still matches byte-for-byte. The two docstring
references were repointed at _write_stage_result.

Worth recording as the general form, since it is the sharper half of this PR's
pattern harvest: a function kept for a caller that does not exist is not a
compatibility shim, it is a claim in the codebase that is simply untrue, and it
costs more than the lines — it makes the next reader believe there is an off-loop
consumer to preserve.

[iamwhatever]

Disposition: fixed — span=e5002d2b7419

• Restored result paths permit arbitrary file reads (src/kiro_crew/context_management.py)

Legitimate, and the round-2 validator on this same span is what left it open: I
tightened the type of a stage-result value and stopped there, when the value is not
free-form data at all.

_result_path required only a non-empty string, and the restored path is opened by
_read_previous_results, whose contents are inlined into the NEXT stage's prompt.
So the transcript's own metadata line could name any readable file on the host and
have its bytes handed to the model. The history JSONL is a plain file, so a
hand-edit — or anything able to write it — reaches this.

Fixed in 4bcbe4654 with a whitelist rather than a screen.
_confine_restored_result_paths runs at the restore boundary, where the slot key is
known, and keeps a value only when it equals the exact file the writer produces:
<config_dir>/sessions/<slot>/stage_<n>_result.md. _write_stage_result's output is
fully determined by the slot key and stage number, so there is nothing legitimate
outside that set and a denylist would have been the wrong shape. Compared as
strings against the writer's own rendering, deliberately: Path equality folds
different spellings of one location together, and for a whitelist the only tolerable
error is being too strict. A rejected entry is dropped, so its stage re-runs — the
same safe direction as every other rejection here.

Coverage, red before the fix: /etc/passwd, /etc/shadow, ~/.ssh/id_rsa, a
../../../../etc/passwd traversal, a bare relative filename, /tmp/..., and the
non-string cases are each dropped while the rest of the record passes through; a
path under another slot's session directory is dropped too, so the whitelist is
per-slot and not merely "somewhere under sessions/"; and the writer's own path
still restores and still advances the resume pointer.

This is the second round on this span. The invariant I should have written the first
time, and which the fix now encodes: a value that will be used as a capability
(a path to open, a command to run) cannot be validated by its type — it has to be
matched against the closed set of values the producer can emit.

[iamwhatever]

/ai-review override gpt 3ac4206: The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 3ac420691823050b40b2ccb074f5838b8daa2979.

The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

This decision applies only to this commit. A new push requires a new judgment.

[iamwhatever]

/ai-review override gpt 037702f: The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 037702f41870623f999a22cd99717f85494e8e48.

The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

This decision applies only to this commit. A new push requires a new judgment.

[iamwhatever]

Round 9 — Opus advisory fixed; the remaining red is inherited from main

Disposition: fixed — Opus 4.8 advisory on 3ac420691, chat_orchestrator.py:988
(attended plan halted with "Auto-run stopped" and no Go row when a stage uses its
full 3 rounds)

Legitimate, and the same shape as the round-6 watchdog fix: a gate that bounds
unattended runtime was firing on an attended plan. An attended stage that spent
exactly its allowed waves and finished has done nothing wrong, and the loop was
about to offer Go anyway — instead _halt_plan said "Auto-run stopped" on a plan
that was never in auto-run and break skipped the Go row, stranding the
step-through. Fixed in 037702f41: _round_cap_message is only consulted when
auto_run is set. Red-before test test_attended_stage_at_the_cap_still_offers_go
asserts the Go row is present and "Auto-run stopped" is absent.

Not from this PR — the three red checks on 3ac420691 (Backend Tests (3.12, 3),
Backend Tests (Windows) (3), and the fail-closed Coverage Gate) are all a single
test, test_members_dm_thread.py::TestDenialAuditOffload::test_no_members_sel_audit_is_offloaded.
This diff touches neither members.py nor that test; it is red on origin/main
itself (CI run for 2b9e5d916 fails identically) and is tracked as #8846. It clears
once that lands and this branch is rebased.

GPT override re-posted for the new head (overrides are SHA-scoped); the reasoning is
unchanged from 3ac420691.

[iamwhatever]

/ai-review override gpt 5f1613d: The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

[github-actions]

Human judgment recorded

@iamwhatever marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 5f1613d3d45ac754aa1f4412cd89c0a404e944c5.

The round cap is a stage-boundary halt by design; a per-spawn denial inside the spawn tool is a new control layer #1783 does not ask for, every spawned agent is already bounded by its own governance gate, and the boundary halt is user-visible and identical to what the Slack path has always had.

This decision applies only to this commit. A new push requires a new judgment.

[iamwhatever]

Round 10 — every review lane green on 5f1613d3d; the one red check is inherited from main

Rebased past #8844 (which closed the members.py red, #8846). On this head all four
review lanes pass: Design Review PASS, First Principles PASS, Opus "No findings",
GPT human override accepted for 5f1613d3d. No unresolved threads.

Disposition: not-from-this-PR — Backend Tests (3.12, 4), single failure:
test/test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists::test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery
("a delivery on a fresh gateway ran concurrently with the copy: 'no pool' was read as
'no writer'").

• This PR touches neither snapshot.py nor test_snapshot.py; its diff is confined
  to context_management.py, dashboard/chat_orchestrator.py, config/* and their
  tests.
• The same test fails on main at 002081169
  (run), a commit
  already in this branch's base — together with a sibling in the same class that
  passed here, so it is shard-timing dependent.
• The class passes locally 3/3 in a row.
• Introduced by fix(snapshot): validate notification records the copy branch installs #8576. Filed as test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists red on main (post-#8576): 'no pool' read as 'no writer' #8915 so it has an owner; this PR goes green once
  that lands and this branch is rebased onto it.

Nothing else is outstanding on this PR.