fix(subagent): refuse a spawn whose approval prompt has no surface (#2381)

[iamwhatever]

Problem / Motivation

A spawn_run issued during a turn that originated on a channel (Telegram in the report) on a headless install hangs for ~30 minutes and is then killed.

None of the four auto-approve rungs in subagent_manager/admission.py match, so the gate raises an interactive approval prompt. Only two surfaces can render one: a Slack owner DM (Block Kit buttons) and an attached dashboard client (the approvals feed). With neither, DashboardState.request_approval broadcasts the prompt to nobody and waits out its timeout. The run sits registered at turn 0 with pid: null, and the caller learns nothing until the reaper force-kills it.

The reporter also hit it from the dashboard, when the session lost its slot registration — so this is not "Telegram has no trust affordance". It is "any spawn whose prompt reaches no surface waits out the deadline".

Why it matters

Sub-agent orchestration is unusable from a channel: every spawn hangs unless a human happens to be watching the dashboard. The 30-minute silent stall with no channel-side feedback is the expensive part — the reporter mis-diagnosed it twice from the surfaced message.

What changed (motivation → approach → change)

Symptom → a spawn approval that nobody received is still waited on.
Root cause → nothing distinguishes "the prompt is unanswered" from "the prompt reached no one". Waiting is only correct in the first case; in the second the wait can only end one way, and the caller pays the full deadline to find that out.

Why the check is in the callback, not in the gate's cascade. Four non-human auto-approve shortcuts live inside _interactive_approval and never reach the gate: hooks.auto_approve_sources, the CLI --approval mode, the YOLO override, and slot trust. A gate-side probe would have to re-derive all four, and would refuse spawns those rungs mean to allow — one of them, adding "subagent" to hooks.auto_approve_sources, is this issue's own documented workaround.

Why the check is at the dashboard-only fallback and nowhere else. That branch is reached only after every shortcut was skipped and after the Slack branch either was not taken or fell through after failing to post. "Nobody received this prompt" is the only remaining reading there.

What counts as a surface, and the two near-misses that do not. Both are the same mistake — a configured connection is not a reached one — and both were caught in review:

• A Slack owner DM does not count at this branch, because reaching it means the DM was absent or posting to it raised. (GPT r1.)
• An app-token socket does not count either. It registers on /api/ws like any client, but _ws_client_allowed short-circuits only for a dashboard user; an app socket goes through the deny-by-default event-scope gate, so it receives the approval frame only if its manifest declared that event. An open app UI is not somebody who can answer. (GPT r2.)

So the probe asks one narrow question: is a dashboard-user socket open? WebSocketHub.dashboard_user_ws_count() lives beside _ws_client_allowed, whose first line it mirrors, rather than being re-derived in the gateway.

The change, five files:

1. subagent.py — a SpawnApprovalUnreachable signal a SpawnApprovalCallback may raise. Its message names the missing surface, because the raiser is the only party that knows what the surfaces are.
2. dashboard/websocket_hub.py + dashboard/state.py — dashboard_user_ws_count(), which skips app tokens and closed-but-unpruned sockets.
3. slack/gateway.py — _dashboard_client_attached(), and a separate callback instance for the spawn gate with raise_when_unreachable=True that raises at that one park point.
4. subagent_manager/admission.py — the gate catches the signal above its generic except Exception (which previously flattened it into the same "spawn rejected" a human decline produces) and refuses immediately. The audit row keeps outcome="rejected" and carries reason: no_approval_surface, so an auditor can separate the two without a new outcome value.

The refusal has two audiences, and which text each gets is a security decision. (Design r3.) The four-rung how-to is the operator's and goes only in the logger.warning: it names two config.json keys, and security.py:8369 records that storing a control there "would leave it writable by any auto-approved agent shell". info.error travels to the calling agent as a completion event — automation input — so putting the how-to there hands the party this gate constrains the recipe for removing it, which an unattended or prompt-injected agent can simply follow. The agent's text is terse and names no file and no key: ask the operator to open the dashboard, or to enable spawn auto-approval. Actionable for an agent means "who to ask", not "which key to flip".

Prose only, deliberately no error_code. The single reader of that field (POST /api/spawn, messaging.py:287) runs before this task does — api_spawn checks info.done and info.error with no await after the synchronous spawn() — so a code minted here would reach no caller, and an unread code is contract surface bought for nothing (error_code's own note says so). Round 1 shipped one; the First Principles lane caught it and it is now subtracted, which is also why no spec change is due.

A relay reader is not a false positive: it consumes the SSE stream (dashboard/remote_mirror) and never registers on /api/ws at all.

Unchanged on purpose:

• A prompt that was delivered still waits out its deadline.
• A human decline still reports the plain "spawn rejected".
• Mid-run tool approvals keep parking — they have no terminal path that could report a refusal, so raising there would turn a recoverable park into a lost turn. That is why the spawn gate gets a separate closure instead of a flag on the shared one, with a source ratchet against the two collapsing back together.

The refusal reaches the calling agent as a completion event (the same channel a declined spawn uses), within milliseconds instead of ~30 minutes.

Tests

test/test_spawn_approval_no_surface_2381.py, 26 tests in six groups:

• The fix — the refusal is immediate and actionable; it quotes the raiser's surface clause; a bare signal still reads as a sentence rather than (); it releases the concurrency slot; it reaches the caller through on_done; it audits reason: no_approval_surface; and it writes a log line keyed to the run id.
• The two audiences — the operator log names all four rungs, and a ratchet asserts none of config.json, hooks.auto_approve_subagent_spawn, hooks.auto_approve_sources or approval_mode="auto" appears in info.error. A ratchet rather than a behavioural check, because a leak reads exactly like a helpful message.
• The other two outcomes are unchanged — a human decline keeps "spawn rejected" with an empty error_code; a delivered prompt still parks with _awaiting_approval set and its slot held.
• The socket count — the real dashboard_user_ws_count over hand-built sockets: app-only → 0, dashboard user → 1, both → 1, closed → 0.
• The probe — no client → no surface; one client → surface; a configured Slack owner DM does not make it reachable (configured and absent must read identically here); no dashboard_state → no surface; a broken count reads as attached; and the probe never consults ws_client_count even when it would answer 7.
• Only the spawn gate raises — the real _interactive_approval closure raises with 0 clients and never awaits request_approval; a failed Slack post still fails fast; it still prompts with 1 client; the non-raising instance parks under the identical posture; plus a source ratchet that on_tool_approval stays wired to the non-raising closure.

Red-before, proven rather than argued, once per round:

• r1 — against pristine origin/main 3094693, a throwaway probe (deleted, not committed) showed that with no Slack and zero clients _interactive_approval parks on request_approval — the hang itself — and that any callback exception becomes error == "spawn rejected", so main cannot express "nobody could answer". The new test file cannot even import on main.
• r2 (Slack term) — re-adding it turns 4 of 19 tests red, including DID NOT RAISE SpawnApprovalUnreachable.
• r3 (raw socket count) — reverting the probe to ws_client_count() turns 5 of 24 tests red, same marker test among them.
• r4 (bypass recipe) — putting the rung list back into info.error turns the security ratchet red (1 of 26).

Manual verification

N/A — the failure is entirely inside the approval callback and the spawn gate, and both are driven directly by the tests, including the real _interactive_approval closure and the real socket counter rather than stand-ins.

Local gates green: black, isort, flake8, mypy on all five changed modules; pytest -k "subagent or spawn or slack or telegram or approval or gateway or websocket" → 8747 passed, 0 failed. (Two setup errors in test_autonudge_reconciler.py are a shared-venv pytest-asyncio event_loop fixture removal, identical on the base. A wider round-3 pass of 12151 tests also showed one test_sandbox_argv.py parallelism flake — 17/17 green in isolation, and this diff touches no sandbox code.)

Two decisions still open for maintainers

Neither is taken here, per @bolichen97's triage:

1. Is an operator-scoped per-agent auto_approve_spawn rung an acceptable shape? (@cschnidr's proposal.) It would be a new per-agent security primitive — KiroCrewAgentConfig carries no approval or trust field today — which feat: Run subagents under interactive approvals instead of requiring full trust #4751's operator comment says should be specified once together with Enforce no-nesting and read-only reviewer scope for spawn_run at the runtime, not just in prompts #4693. No new rung is added by this PR; the fix names the four that already exist.
2. Should the tombstone carry the prompt target (channel, surfaced)? No tombstone field is added. The reap epitaph from fix(subagent): report spawn-approval-parked reaps accurately, not as a missed deadline #7325 is untouched, and a fail-fast spawn never reaches the reaper at all.

Residual work (why Refs, not Closes)

The issue's first suggested fix — delivering the spawn approval to the originating channel with approve/reject actions — is not in this PR. It is genuinely larger: Telegram already has an interactive approval keyboard (telegram/renderer.py::on_prompt_choice with TelegramApprovalDecider nonce arming), but it is driven by the provider's in-turn permission prompts, while a spawn approval goes through on_spawn_approval → _interactive_approval → dashboard/Slack only. Routing spawn approvals into each channel's renderer + decider is a separate reviewable change per channel.

The exception's surface-clause split is designed for exactly that follow-up: a Telegram raiser supplies its own clause and the gate's rung list stays put.

So this PR fixes the unambiguous defect only — the ~30-minute hang with nobody who could answer — and leaves channel-side delivery, plus both design questions above, open on #2381.

Related Issues

Refs #2381

Pattern harvest

Rule candidate: review-prompt
Pattern: addressed is not delivered. The defect and all three review rounds are one shape at four depths — waiting out a human deadline without checking the prompt reached a surface (the bug); counting a Slack DM that was never posted to (GPT r1); counting an app-token socket the frame is not delivered to (GPT r2); and, one level up, delivering the right information to the wrong party — a bypass how-to addressed to the agent the gate constrains (Design r3). A prompt-level rule would ask, of any message a fix emits: who actually receives this, and is that who it is for?

Sibling instance already on the issue: #2854 (a mid-turn tool permission never delivered, same 30-minute cap, different gate).

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 (if applicable) — no spec change is due: no new error_code ships. The cascade docstring in admission.py was corrected, since it listed four rungs and omitted the pre-existing parent-trust one that the new refusal names
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

All claims verified: the _impl global-rebinding makes the runtime except clause resolve; the fidelity between description and diff is exact; alternatives were genuinely weighed. One design premise deserves scrutiny — GET /api/approvals (src/kiro_crew/dashboard/handlers/sessions.py:1596) re-syncs pending approvals to late-attaching dashboard clients, which contradicts the PR's core premise that an undelivered prompt's wait "can only end one way."

Design-Verdict: CONCERNS

Fail-fast is built on "nobody attached now = nobody can ever answer," but pending approvals re-sync to late-attaching dashboards — the wait was answerable.

Watch

• The justifying premise — "when it reached none, the wait can only end one way" — is factually wrong: GET /api/approvals (handlers/sessions.py:1596) replays pending approvals, so under old behavior an operator opening the dashboard any time inside the deadline could approve the spawn. Raising before request_approval registers the prompt forecloses that: a spawn issued during a page reload or a brief disconnect is now hard-refused instead of answerable on reconnect. The trade (instant feedback vs. late answerability) is probably still net-right for the headless/channel case, but it is a real behavior regression for the attach-within-deadline case and the PR presents it as impossible rather than as a choice.

Suggestions

• State the foreclosure explicitly (docstring says "could never answer" — it can, via re-sync) and consider a short grace re-check before raising, which keeps fail-fast for genuinely headless installs without penalizing a mid-reload dashboard user.

[DESIGN-REVIEWED] baa57c1

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] baa57c1

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 6690b0191e3e6d1662fde1b89b9c005065811bbc — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 6690b01

Verdict parsed from the review's SHA-scoped output markers for commit 6690b0191e3e6d1662fde1b89b9c005065811bbc.

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of baa57c176e926da6e21a9783aa1c448c593fe6fd — 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 mechanical checks are done: consumer counts verified, the one candidate duplicate (has_dashboard_surface) confirmed as a different question, and sibling park points counted. Final review:

First-Principles-Verdict: PASS

Every item traces to the reported 30-minute hang (#2381) or a named trust boundary, sits where the description says it sits, and the one tempting duplicate answers a different question.

What this change ships

Intent: when a spawn's approval prompt can reach no human, refuse immediately instead of silently waiting out the reaper — a FIX.

1. A spawn nobody could approve fails in milliseconds, not ~30 minutes — justified (reported defect spawn_run from Telegram hangs 1800s: scoped-trust approval prompt is dashboard-only #2381).
2. The agent-facing refusal says who to ask, never which key to flip — justified (agent-untrusted-of-its-own-gate boundary).
3. The operator log names all four auto-approve rungs — justified, declared.
4. Audit rejection rows carry reason: no_approval_surface, same rejected outcome — declared; vocabulary kept.
5. New dashboard-user socket counter (hub + state delegate) — justified; 1 consumer (slack/gateway.py:1854), not generalized.
6. raise_when_unreachable opt-in; set by 1 of 7 _interactive_approval call sites — justified, declared reason for the split.
7. SpawnApprovalUnreachable signal — justified; 2 consumers (slack/gateway.py:2253, admission.py:770).
8. Spawn-cascade docstring now lists the pre-existing Trust rung (admission.py:480) — rides along (doc correction).
9. A broken socket count reads as "attached", restoring today's park — declared fallback.

Duplication check: session_surface.has_dashboard_surface is per-session and prefix-True for any dashboard: key even with zero tabs open — it would miss the reporter's own lost-slot dashboard case — so the new probe is meaningfully different, not a second spelling. The pre-subtracted error_code claim holds: its one reader is messaging.py:287, which runs before this task.

Watch

• Six sibling park points share the root cause and keep parking (slack/gateway.py:4562, 4701, 5670, 5680, 8374, 8650 — cron ×2, autonudge ×2, tool approvals, taskrunner). Declared "unchanged on purpose" with a named constraint (no terminal path to report a refusal): accepted-and-deferred, not a demand.

Subtractions

• ws_client_count (dashboard/websocket_hub.py:341, dashboard/state.py:8151): grep ws_client_count finds 0 src consumers — only the two definitions and test mocks — and the new counter's docstring records why it gives the wrong answer. Delete it.

[FIRST-PRINCIPLES-REVIEWED] baa57c1

[iamwhatever]

• src/kiro_crew/slack/gateway.py — Slack term treats failed Slack delivery as reachable (span=f00224b0077d) — fixed in 42c9aba64.

"self.slack is not None and self._owner_id" treats failed Slack delivery as reachable, causing a 180-second dashboard wait with no connected client -> Fix: after Slack fallback, determine reachability solely from attached dashboard clients.

Legitimate and reachable, and it defeated the fix in exactly the case the fix is for. The Slack branch is wrapped in its own except and falls through to the dashboard-only fallback, so a Slack outage on a channel install landed right back in the park this PR removes.

Applied the suggested fix as stated. _approval_surface_attached() is now _dashboard_client_attached() and asks about the dashboard alone, because at its one call site Slack has already had its turn — either no owner DM was configured, or posting to it raised — so a Slack term there reports a surface that demonstrably received nothing.

Red-before proven, not assumed: re-adding the Slack term to the new probe turns 4 of the 19 tests red, including test_a_failed_slack_post_still_fails_fast with DID NOT RAISE SpawnApprovalUnreachable. That test drives the real _interactive_approval closure with post_blocks raising and asserts request_approval is never awaited. Two probe tests also now pin that a configured and an absent Slack read identically.

[iamwhatever]

• SPAWN_APPROVAL_UNREACHABLE_CODE is contract surface with zero consumers — fixed (subtracted) in 42c9aba64.

AGENT_NOT_FOUND is set synchronously inside spawn(), so POST /api/spawn catches it at messaging.py:272; this refusal happens in a task created after the handler's info.done check (no await between them), so it never reaches messaging.py:287 — the ONLY reader of info.error_code in src/.

Verified and accepted. grep 'info.error_code' src/ returns exactly two hits: messaging.py:287 (the reader) and my own setter. api_spawn calls state.subagents.spawn(...) and checks info.done and info.error with no await between them, while the refusal runs in the asyncio.create_task(_spawn_with_approval(...)) the gate creates — so the handler has already answered status: spawned by the time the code is set. The code could not acquire a consumer, and error_code's own note ten lines up says a code with no consumer is contract surface bought for nothing.

Subtracted exactly as prescribed: the constant, the info.error_code = assignment, and the test assertion on it are gone. What separates the two outcomes remains — reason: no_approval_surface in the audit row for a machine, and the remediation prose for the agent that receives the completion event, which is the only path the async refusal actually travels.

This also dissolves Design Review's spec-drift concern: with no new code, docs/system-specs/modules/subagent.md's error_code vocabulary is unchanged, so there is nothing to update.

On the Watch item about the relay reader: the docstring claim was right but its reasoning was not stated, so I made it explicit. A relay reader consumes the SSE stream (dashboard/remote_mirror, opted in per-turn with ?relay=1 on /api/chat); it never registers on /api/ws, so it is not in _ws_clients and ws_client_count() cannot count it. The docstring now says that rather than reasoning from the mirror's slot scoping.

[iamwhatever]

• Watch: a new error_code shipped without the spec that enumerates its siblings — fixed in 42c9aba64, by removing the code rather than the drift.

docs/system-specs/modules/subagent.md is the home of the SubagentInfo.error_code vocabulary ... and this PR adds spawn_approval_unreachable to that carrier with no spec change.

Correct, and the First Principles lane arrived at the same surface from the other side: that code had zero consumers and could not acquire one, because error_code's only reader (POST /api/spawn) runs before the task that would set it. The code, its assignment, and the test asserting it are gone, so the vocabulary is unchanged and there is no spec to update. Answering the concern by writing a spec entry for a field nobody reads would have been the worse of the two fixes.

(Reposted as one record per finding — the previous comment covered this and the suggestion below it together, which the disposition rule rejects. The suggestion now has its own record.)

[iamwhatever]

• Suggestion: carry the remediation prose on SpawnApprovalUnreachable so the gate stays surface-agnostic — fixed in 42c9aba64, split rather than moved wholesale.

carrying it on SpawnApprovalUnreachable from the raiser would keep the surface-agnostic gate surface-agnostic and stop the prose going stale when channel-side delivery (the disclosed residual work) lands.

The gate no longer names any surface. SpawnApprovalUnreachable now carries the surface detail from its raiser ("no dashboard client is connected"), and the gate quotes it and adds the four config rungs, which are its own cascade and not the gateway's to know. The exception's docstring states that contract, an empty message falls back to "no interactive surface is attached" so the sentence never renders as (), and two tests pin both halves — one that a raiser's own wording reaches info.error, one that a bare signal still reads as a sentence.

That keeps the prose from going stale when channel-side delivery lands: a Telegram raiser will supply its own surface clause and the rung list stays put.

[iamwhatever]

• src/kiro_crew/slack/gateway.py — ws_client_count() counts app-token sockets (span=f00224b0077d) — fixed in 25897c83b.

"ws_client_count() > 0" counts app-token sockets that cannot receive a slotless approval, so channel spawns still park for 180 seconds -> Fix: count only open sockets marked _is_dashboard_user.

Legitimate, verified, and the same defect class as your round-1 finding — a configured connection is not a reached one, now in a second location. dashboard/ws.py:606 sets ws["_is_dashboard_user"] = request.get("is_dashboard_user", False), and token_auth.py resolves that as not app, so an app token registers on /api/ws and lands in _ws_clients. _ws_client_allowed (websocket_hub.py:140) short-circuits True only for a dashboard user; an app socket goes through the deny-by-default event-scope gate, so it receives the approval frame only if its manifest declared that event. An open app UI beside a Telegram spawn therefore made the probe report a surface that gets nothing.

Applied the suggested fix as stated, with the filter placed where it belongs rather than re-derived in the gateway: WebSocketHub.dashboard_user_ws_count() sits beside _ws_client_allowed, whose first line it mirrors, exposed through the DashboardState facade like ws_client_count. It also skips closed-but-unpruned sockets, since the registry prunes lazily on the next broadcast.

Red-before proven, not assumed: reverting the probe to ws_client_count() turns 5 of the 24 tests red, including test_a_failed_slack_post_still_fails_fast with DID NOT RAISE. Four new tests drive the real dashboard_user_ws_count over hand-built sockets (app-only → 0, dashboard user → 1, both → 1, closed → 0), and one pins that the probe never consults ws_client_count even when it would answer 7.

[iamwhatever]

• Watch: the refusal prose coaches the agent on how to disable its own approval gate — fixed in 6690b0191.

an unattended (or prompt-injected) agent hits the refusal → follows the quoted instruction and edits config.json → permanently removes the human spawn-approval gate with no human ever seeing a prompt. The spawn gate exists precisely because the agent should not approve its own spawns; handing the bypass recipe to the party the gate constrains inverts that.

Legitimate, and the cause→mechanism→consequence chain is verifiable in the tree — security.py:8369 states it outright about the very file I was naming: storing a control in config.json "would leave it writable by any auto-approved agent shell (verified: is_sensitive_bash_command("echo x > …/config.json") is None)". info.error reaches the caller as a completion event, so the how-to was automation input. That is a security inversion, not a wording preference, and the correction is a subtraction.

Applied your suggestion as stated. The rung list now lives only in the operator-facing logger.warning — which is where an operator looks, and what #6484 asked for in the first place. The agent-facing info.error is terse and names no file and no key: "…Ask the operator to open the dashboard and spawn again, or to enable spawn auto-approval." Actionable for the agent means "who to ask", not "which key to flip"; nothing in it can be followed into a self-grant.

The information is not lost, it is re-addressed. Two tests pin both halves, and the security one is a ratchet rather than a behavioural check, since a leak reads exactly like a helpful message: test_the_operator_log_names_every_rung requires all four rungs in the log, and test_the_agent_facing_refusal_carries_no_bypass_recipe asserts none of config.json, hooks.auto_approve_subagent_spawn, hooks.auto_approve_sources, approval_mode="auto" appears in info.error.

Red-before proven, not assumed: putting the rung list back into info.error turns that ratchet red (1 of 26).

This finding is also the third instance of this PR's own harvested pattern, one level up — the first two rounds counted a configured channel as a reached one; this one addressed the right information to the wrong party. I have added it to the Pattern harvest section.

[bolichen97]

Approved.