fix(subagent): report spawn-approval-parked reaps accurately, not as a missed deadline

[bolichen97]

Fixes #7306

Split out of #6484 / PR #7299 (which fixed the observability half). This lands the reap-message fix that could not ride along, plus the boundary-gate condition that blocked it.

Part 1 — the reaper misattributed the cause

A default install gates every spawn_run behind the interactive spawn approval. If nobody answers, the run sits with turns == 0, _pid is None, _exec_started is None. The subagent reaper's wall clock (1800s) always fires ~5400s before the dashboard approval window (7200s) closes, so _force_reap_impl stamped:

Reaped after 1801s (exceeded 1800s deadline) [turn 0/100]

That blames an execution deadline a run which executed nothing could not have reached — exactly the misleading turn-0 timeout the startup watchdog exists to avoid.

Fix: a new first branch in TerminalCoordinator._force_reap_impl, gated on info._awaiting_approval and info._exec_started is None, that reports the run as parked on a never-answered spawn approval (no "exceeded"/"deadline" wording). Both conjuncts are load-bearing: run.py sets _awaiting_approval for mid-run TOOL prompts (where _exec_started is set), so _exec_started is None is what separates 'never started' from 'was running and blew the deadline'. The branch keys on info flags rather than reason, because the buggy wall-clock reaper calls _force_reap with no reason.

Wiring fix (found in review): the new branch was dead code in this tree because nothing set _awaiting_approval on the pre-execution spawn-approval wait (the #7299 flag-setter is not an ancestor of this branch; only run.py's mid-run TOOL approvals set it). admission.py now wires info._awaiting_approval = True around the spawn-approval await in _spawn_with_approval_impl (cleared in a finally), matching the run.py lifecycle.

Part 2 — unblocking the agent-sdk boundary gate

terminal.py imports four private process-tree helpers directly from kiro_crew.acp.client, but had no entry in .github/agent-sdk-boundary-baseline.txt. The file-scoped gate is dormant on main and fires the moment any PR touches the file, on a violation it did not introduce (the gate itself reports 0 file(s) with new edges on added lines). The likely origin is the coordinator split-out: the edge moved into a new module while the baseline still tracked only subagent.py.

Route chosen: record the migrated edge (ROUTE 2), not build a facade (ROUTE 1). Grounding:

• The import is pre-existing; the gate confirms it was not added by this change.
• cron.py carries the identical four-helper import and is already baselined at count 1 — direct precedent.
• The facade route is RFC-scale: rfc-crew-agent-sdk-boundary.md §5.4 defers PID tracking/sweeping/reaping to a future AgentSupervisor phase, agent_sdk exposes none of these helpers today, and session.py + cron.py import the same set — out of scope for a bug fix.

Added 1 src/kiro_crew/subagent_manager/terminal.py in sorted position.

Changes (6 files)

1. .github/agent-sdk-boundary-baseline.txt — record the migrated ACP edge.
2. src/kiro_crew/subagent_manager/terminal.py — new accurate reap branch.
3. src/kiro_crew/subagent_manager/admission.py — set/clear _awaiting_approval around the spawn gate.
4. src/kiro_crew/subagent.py — widen the _awaiting_approval field comment: the flag covers the pre-execution spawn gate as well as a mid-run tool prompt.
5. docs/system-specs/modules/subagent.md — spec update for the same two facts (the widened flag meaning and the new reap message).
6. test/test_subagent_startup_watchdog.py — three tests: both-directions distinction (approval-parked -> new message; mid-run-tool-parked -> generic message, proving the _exec_started is None conjunct is load-bearing) plus a production-wiring test that the spawn gate sets the flag.

Testing

• The agent-sdk boundary gate (the exact CI gate) PASSES with this diff (0 offenders). Proven necessary end-to-end: removing the baseline line while keeping the terminal.py edit reproduces the issue's exact failure signature (1 new offender(s), 0 grown count(s), 0 file(s) with new edges on added lines).
• py_compile passes on all changed files.
• 785 targeted backend tests pass locally on a CI-parity Python 3.12 environment: the 8 reap/stall/watchdog
  modules closest to the change (106 tests, including the 3 new ones) and 11 broader subagent modules (679 tests).
• flake8 and mypy --platform linux are clean on the three touched source modules; black reports all four
  touched Python files unchanged, and the repo's black, docs-lint, harness-parity and brand gates pass in scope.

Deliberately untouched

Item #3 in the issue (_BACKGROUND_APPROVAL_SOURCES in slack/gateway.py) is approval-model policy, not a defect, and was left alone per the issue's explicit note.

Pattern harvest

Rule candidate: review-prompt + a rule line in docs/system-specs/common/error-handling.md

Pattern: a terminal-state failure message assembled by elimination instead of from observed state. _force_reap_impl synthesized its error with a two-arm dispatch — if reason == "startup_timeout" else "Reaped after {n}s (exceeded {default_timeout}s deadline)" — and the wall-clock sweep at monitoring.py:489 calls _force_reap(agent_id, info, elapsed) with no reason at all. So the else arm is the catch-all for every cause anyone ever adds, while asserting one specific limit was hit. A run with turns == 0, _pid is None and _exec_started is None therefore got an epitaph naming an execution deadline it provably never reached. The rule that would have caught it: a synthesized failure message may name a limit (deadline, timeout, turn cap) only when the observed state proves the run reached the phase that limit measures; a default arm may describe what was observed, but must not assert which limit was hit. error-handling.md carries three principles today and none of them says this — even though the startup watchdog next door exists precisely to avoid "a misleading 30-minute turn-0 timeout", i.e. the same defect, caught once, at one site, without the rule ever being written down. Semgrep is the wrong instrument here (it cannot distinguish failure text from any other f-string); the checkable form is the doc rule plus a review-prompt line, with a concrete shape for a reviewer to look for: a reason-dispatch whose default arm interpolates a configured limit, reached by a caller that passes no reason.

Rule candidate: ratchet tooling — conserve a baselined violation across a pure MOVE.

Pattern: this PR's second half, and the reason it cost a review round. Every baselined gate's --update-baseline "only ever lowers counts and deletes lines, never adds or raises one", and the verdicts are diff-scoped. So when #6944 moved _sigkill_session_impl and its from kiro_crew.acp.client import (...) out of subagent.py into the newly created subagent_manager/terminal.py, #7150 dutifully recorded the departure (9 -> 8, total 109 -> 108) and nothing could record the arrival. The destination file was left dirty-but-unbaselined and completely invisible — until the first unrelated PR to touch it, which then inherits a gate failure it did not cause and has to argue for a baseline ADD that the file's own header explicitly forbids. The fix for the class is mechanical, not judgment: run the ratchet family unscoped on main (post-merge, its own non-blocking job) so a dirty-but-unbaselined file is named by the refactor that created it, and/or give --update-baseline a --migrate <old> <new> mode that moves a count between files without raising the total. Either one makes "the recorded total is conserved across a refactor" something CI checks, instead of something a later contributor has to reconstruct with git log -L on the baseline file. This applies identically to check_black_formatting.py, check_subprocess_encoding.py and check_sync_io_in_async.py, which share the same shrink-only tooling and the same blind spot.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 816c38a8049868864521d1296b1125b10af3d61e — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 816c38a

Verdict parsed from the review's SHA-scoped output markers for commit 816c38a8049868864521d1296b1125b10af3d61e.

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Attribution fixed at the root — the message derives from observed state, not reason-dispatch elimination — with the wiring gap caught and test-pinned; the baseline add is a grounded migration record, not a widening.

Suggestions

• Follow-up (not this PR): the accurate new message still documents a lifecycle mismatch — the wall clock reaps a parked run ~90 minutes before its approval window closes ("the reaper's wall clock fired before the (longer) approval window closed"), so a pending dashboard prompt can outlive the run it gates; withdraw/auto-deny the pending approval at reap, or align the parked-run deadline with the approval window.

[DESIGN-REVIEWED] 816c38a

[github-actions]

First Principles Review (Fable 5) — ⚠️ could not complete

The first-principles review did not produce a verdict for 816c38a8049868864521d1296b1125b10af3d61e (the review step completed but returned no verdict header). See the First Principles Review job logs. Advisory — does not block merge.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 816c38a8049868864521d1296b1125b10af3d61e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 816c38a

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

[bolichen97]

• New exemption disables the shrink-only boundary guard (span=321e929890b5) — rebutted

The lane's finding, quoted in full:

BLOCKING -- .github/agent-sdk-boundary-baseline.txt:75 -- New exemption disables the shrink-only boundary guard
1 src/kiro_crew/subagent_manager/terminal.py
Touched file with forbidden ACP import -> baseline exemption -> boundary gate passes instead of rejecting it.
Anchor: residual/guard-removal
Fix: Remove the exemption and revert the terminal.py hunk until the dependency is routed through agent_sdk.

The premise is that this PR introduces or launders a new boundary violation. It does not.

1. The import is pre-existing on main. from kiro_crew.acp.client import (...) sits at main:517 and at the judged head:535 — byte-identical text, shifted down only because the fix added lines above it. git log --follow attributes it to a90d5820a refactor: extract subagent manager boundaries (#6944), a file split, not to this PR.

2. The gate reds purely because the file is touched. check_agent_sdk_boundary.py's _verdicts() computes new_offenders as files with violations not in the baseline and inside the touched-file scope. terminal.py has a pre-existing edge, was absent from the baseline, and this PR touches it. Deleting the baseline line and re-running the gate prints 1 new offender(s), 0 grown count(s), 0 file(s) with new edges on added lines — the gate itself certifies the edge is not on a line this PR added. Under that scoping terminal.py is untouchable by any PR until its orphaned edge is recorded, and that orphan belongs to refactor: extract subagent manager boundaries #6944.

3. "Disables the guard" is not what the entry does. It pins the count at 1, so the ratchet stays fully effective for this file: a second ACP import trips grown (2 > 1), and a new edge on an added line trips added_line_offenders, which applies to baselined files too. No other file's ceiling moves. The baseline header states these semantics itself — "It is a floor, not a countdown to zero … What the gate buys is that the dependency cannot SPREAD into a file that is clean today." terminal.py was never clean.

4. There is exact precedent. 1 src/kiro_crew/cron.py is baselined at count 1 for the same four helpers (_capture_child_records, _get_child_pids, _is_our_child, _kill_escaped_children) from kiro_crew.acp.client, for the same documented reason: cron.py's "circular import: cron → acp.client → session → cron" mirrors terminal.py's "circular import: subagent → acp.client → session → subagent".

The proposed fix fails on both halves. "Revert the terminal.py hunk" deletes the bug fix — terminal.py is where the corrected reap-error branch lives, so that reads as "do not fix #7306". And "until the dependency is routed through agent_sdk" is not a small change: agent_sdk/__init__.py's __all__ exports none of those four helpers, which are underscore-private ACP internals, so rerouting means promoting four private internals onto the public SDK surface. That is an RFC-scoped design decision (docs/request-for-change/rfc-crew-agent-sdk-boundary.md) and far outside a message-accuracy bugfix.

Honest caveat, because the reading is textually grounded rather than noise: the baseline header does say "Do NOT add or raise a line to make a red gate green". It is nonetheless wrong here, because both alternative readings fail above and the commit message already discloses the entry as "the pre-existing, coordinator-split-out ACP edge … matching the cron.py precedent".

[bolichen97]

• Accurate epitaph, but the timer mismatch it describes survives — accepted-and-deferred

The Design lane printed no FINDING -- / BLOCKING -- line, so it exposes no span id to claim; its verdict is 🟡 CONCERNS, advisory, and its check is green.

The concern, quoted:

The PR's own framing — "the reaper's wall clock (1800s) always fires ~5400s before the dashboard approval window (7200s) closes" — means every unanswered spawn approval is still reaped mid-window: a human who approves at minute 35 approves a corpse, and nothing in this series withdraws or resolves the still-pending dashboard prompt on reap. The new message accurately labels that behavior rather than deciding whether it is intended.
The flag this PR wires (_awaiting_approval and _exec_started is None) is exactly the predicate a wall-clock exemption (deferring to the approval window's own expiry) or a prompt-withdrawal-on-reap would need — a human should decide which half is the actual contract, in a follow-up.

Accepted as correct, and deliberately deferred. Two reasons this PR is the wrong place to resolve it:

1. The lane scopes the ask to a human, by its own words — "a human should decide which half is the actual contract, in a follow-up". Choosing between a wall-clock exemption and prompt-withdrawal-on-reap changes the reap contract; this change only makes the epitaph match the state that already exists, and does not alter when or whether a parked run is reaped.

2. The deferred half is not obviously the safe direction, which is exactly why it needs the decision the lane asks for rather than a guess made here. The First Principles lane independently examined the same alternative and found that anchoring the wall clock at _exec_started would let an unanswered approval pin a _running_count slot for the full 7200s window (admission.py:751 confirms a parked run holds one). So reap-at-1800s is load-bearing today, and lifting it would trade a misleading message for a capacity leak — a strictly worse failure for a user with a small slot budget.

On the lane's suggestion to track the follow-up against #6484: that is a maintainer action on an issue rather than a code change, and I am not filing or retitling issues from this PR. Recording it against the finding here so the decision is captured rather than lost, and so the polite error message does not become the reason the timer mismatch is never revisited.

[chenmingwei23]

Approved: PR Readiness passed, all review lanes green.