feat(chat): ask_question tool — blocking clickable question card

[kyleseaman]

Summary

Lets an agent pause mid-turn, ask you a multiple-choice question, and receive the answer as the tool's result — no extra turn, no [OPTIONS:] text parsing. You click an option or type a custom answer in the card.

QuestionCard and the question_card websocket event already existed but were unreachable: they keyed off an ACP tool_call titled AskUserQuestion, and that tool does not exist in kiro-cli 2.14.0 (the string appears nowhere in the 662MB binary). This PR supplies the missing trigger from KiroCrew's own MCP server rather than waiting on the agent CLI.

Why not ACP elicitation

kiro-cli 2.14.0 does compile the ACP elicitation/create schema (form/url modes, requestedSchema with enum/oneOf single-select and array multi-select) and gates it on clientCapabilities.elicitation. Its schema maps almost exactly onto QuestionCard's data model, so it would be the ideal wire.

It isn't usable yet. I built a stub MCP server that issues elicitation/create, registered it via session/new, and declared the capability — the agent returned:

{"jsonrpc":"2.0","id":1000,"error":{"code":-32601,"message":"elicitation/create"}}

Method not found: the MCP→ACP forwarding path is unimplemented. So ask_question supplies the capability in-process instead.

Advertising clientCapabilities.elicitation so the native prompt lights up when upstream ships the bridge was originally bundled here; it is split out into #512 (no functional coupling to this PR).

Flow

agent calls ask_question (MCP)
  └─ POST /api/ask-question                                    [blocks]
       ├─ validate_ask_user_question
       ├─ 404 on unknown slot (never block on a card nobody renders)
       └─ request_question → broadcast question_card{ask_id} → await future
                                    … user clicks / types, hits Submit …
  POST /api/ask-question/{ask_id}/answer → resolve_question
       └─ blocked POST returns {status:"answered", answers}
            └─ agent sees "The user answered: …"
  finally: broadcast question_card_resolved{ask_id}

Mirrors DashboardState.request_approval. The differences: the resolution value is the user's answer map rather than an allow/deny boolean, and the card is addressed to one slot rather than the whole gateway.

Design decisions

• Dashboard-only, strict session resolution (env var or HMAC-verified pid sidecar, never the /proc ancestor walk) so a subagent under a parent slot's process tree can't post a card into the parent's conversation. Other surfaces get a refusal pointing at [OPTIONS:].
• 300s default, 1800s ceiling — matches the existing wait tool, since this holds an MCP call open rather than using the 2h human approval window. The HTTP socket timeout is deliberately timeout_secs + 30 so the socket can't trip first and strand a question you're still answering.
• Timeout and dismissal are indistinguishable to the agent, and the tool output tells it not to auto-re-ask — a retry loop would spam the chat.
• question_card_resolved carries the ask_id and fires in finally, so timed-out cards are always retracted instead of staying clickable and 404-ing. The reducer matches on ask_id so a late resolution can't wipe a newer card.
• Answers coerced to str — they're echoed into the transcript, so nested objects can't smuggle structure in.
• Legacy path untouched and cannot double-render: MCP tool calls arrive titled Running: @<server>/<tool>, so the == "AskUserQuestion" sniff never matches; cards from that path carry no ask_id and keep their send-as-message behavior.

Verification

End-to-end against an isolated dev gateway (own KIROCREW_HOME, port 6799; live gateway never touched — kirocrew pod is unavailable on this host, no D-Bus). 8/8:

PASS ws-broadcast              question_card ask_id=710e76f9 slot=chat-6-… questions=1
PASS caller-blocked-until-answered   ask thread alive=True unresolved=True at answer time
PASS answer-accepted           POST answer -> 200
PASS answer-returned-to-caller blocked caller got {'Which trust model…': 'Same-URL carve-out'}
PASS unknown-slot-404          -> 404 in 0.00s
PASS stale-ask-404             -> 404
PASS empty-questions-400       -> 400

Browser round-trip (Playwright on the real built SPA): clicking options then Submit resolved the blocked backend call with {"…trust model…": "Same-URL carve-out", "…environments…": "staging, prod"} — multi-select preserved. A typed custom answer likewise reached the blocked caller.

Gates: pytest 17,172 passed (29 new), vitest 4,503, tsc, isort, flake8 clean. mypy clean apart from a pre-existing faiss overload false-positive in vector_memory.py (reproduces on main).

Known pre-existing failures on this host, all reproducing on clean main: test_dashboard_origin::TestParseDashboardUrlMalformed (×3) and test_skills::test_flat_copy_untouched_when_nested_missing. Under -n auto one additional test flakes per run with a rotating identity (a gc/unraisable-warning attribution issue — test_dashboard_approval on one run, test_apps_registry on the next); each passes in isolation and the whole file passes 6/6 deterministically on both branch and base.

Not covered

• Non-dashboard surfaces (Slack/Discord keep [OPTIONS:]).
• Rate limiting — only the prompt-level "don't re-ask" instruction.
• In an empty session the card visually overlaps the centered welcome suggestions (visible in the dark screenshot); with any chat history it sits normally above the composer.

Spec: src/kiro_crew/docs/agent-questions.md

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/server.py:690 -- function-local "from kiro_crew.dashboard.handlers.ask_question import" violates the top-level-imports rule -> Fix: move this import to the module import block.
[GPT-REVIEWED] 878d638

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 878d638f7dadc63cdc682228f690062c3ede0d1e — updated in place on each push; does not block merge.

Design-Verdict: PASS

Sound design: mirrors the proven request_approval round-trip, alternatives empirically ruled out, and every stop/reset/delete path releases the blocking wait.

Suggestions

• _QUESTION_TIMEOUT_MAX = 540 is silently coupled to _TOOL_STALL_TIMEOUT = 600 in acp/client.py ("540 not 1800: the ACP tool-stall watchdog (600s) kills the turn first") — add an assertion or test tying _QUESTION_TIMEOUT_MAX + margin < _TOOL_STALL_TIMEOUT so a future watchdog tune fails a test instead of killing turns mid-question.
• The PR body still claims "300s default, 1800s ceiling — matches the existing wait tool," but the shipped code enforces 540 and calls 1800 "the bug" — correct the description so reviewers and users don't rely on the stale ceiling.

[DESIGN-REVIEWED] 878d638

[kyleseaman]

Round 1 disposition — head b40e75f8

Both CI failures were mine, both in the new test file. Fixed.

CodeQL — 2× high py/incomplete-url-substring-sanitization (test_ask_question_roundtrip.py:146,157). Legitimate, not a false positive. The stub redactors in the redaction test keyed off a hostname fragment ("evil.example.com" in s), which is exactly the shape of a real sanitizer bug — worth flagging even in a test double. Replaced with opaque sentinels (LEAKY_URL_SENTINEL/LEAKY_CRED_SENTINEL); the test asserts that request_question` routes every text field through the redactors, so the stubs never needed URL parsing. 0 alerts expected now.

Backend Tests (Windows) shard 1 — UnicodeDecodeError: 'charmap' codec can't decode byte 0x90 in test_acp_initialize_sends_capabilities. My bug: Path.read_text() defaults to the locale codec, which is cp1252 on the Windows runners, and acp/client.py contains non-ASCII (em dashes) in comments. Now passes encoding="utf-8" explicitly. Checked the rest of the new files for other unencoded reads — none.

Triage of local failures (not fixed, evidence below)

• test_dashboard_origin::TestParseDashboardUrlMalformed (×3) and test_skills::test_flat_copy_untouched_when_nested_missing — reproduce on clean main on this host; pre-existing.
• test_mcp_gateway_wedge_ping_gate::test_unknown_notification_silently_ignored and test_vector_memory::test_concurrent_write_and_search_no_crash appeared this run. I did not wave these off as flaky without checking: the mcp_gateway one fails even in isolation, so I sampled it 12× on clean main — 11 passed, 1 failed. Pre-existing non-determinism on this host (a gc/unraisable-warning attribution artifact), not a regression. It produces no assertion failure, only an escalated unraisable warning.
• mypy's single error is a faiss overload false-positive in vector_memory.py, also present on main (a faiss-equipped local venv is stricter than CI, which sees faiss as a missing import).

Gates re-run before push: pytest 17,171 passed, isort, flake8 clean. Frontend untouched this round. Screenshot SHAs re-pinned to b40e75f8 and blob presence verified.

[github-actions]

Opus 5 Review — ✅ human override accepted

Reviewed 878d638f7dadc63cdc682228f690062c3ede0d1e — this comment is updated in place on each push.

Human judgment by @kyleseaman overrides the Opus 5 finding for 878d638f7dadc63cdc682228f690062c3ede0d1e; the recorded reason is authoritative for this commit.

Verdict recorded from an authorized human decision for commit 878d638f7dadc63cdc682228f690062c3ede0d1e.

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

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 878d638f7dadc63cdc682228f690062c3ede0d1e.

Second-order review for 878d638f7dadc63cdc682228f690062c3ede0d1e; this comment is updated in place on each push.

Review details

I've read the full sub-threshold findings file and the PR diff. The findings inventory for this commit is small: Opus 5's finding was overridden by an authorized human decision (nothing remains to arbitrate there), GPT 5.6 raised a single style-level finding (a function-local import in server.py), and the design review passed with two suggestions (an untested timeout-constant coupling, and a stale PR description). None of these are one-way doors or concrete harms: the import placement is pure convention, the _QUESTION_TIMEOUT_MAX/_TOOL_STALL_TIMEOUT coupling is already correct in the shipped code (540 < 600 with margin) and only lacks a guard test against future drift — fully reversible in a follow-up — and the PR-body wording is metadata, not merged code.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

• Guard test tying _QUESTION_TIMEOUT_MAX to _TOOL_STALL_TIMEOUT (design reviewer) — the 540s ceiling silently depends on the 600s ACP tool-stall watchdog in acp/client.py; the shipped values are correct today, and a one-line assertion/test (_QUESTION_TIMEOUT_MAX + margin < _TOOL_STALL_TIMEOUT) can be added in a later change so a future watchdog tune fails a test instead of killing turns mid-question. Fix in test/ alongside the existing ask_question tests (also mirror the hard-coded max_val=540 in validation.py::ASK_QUESTION_SCHEMA, which duplicates the constant).
• Move the function-local handlers.ask_question import to the module import block in src/kiro_crew/dashboard/server.py (GPT 5.6) — convention-only; no runtime effect, trivially fixable in any later cleanup.
• Correct the PR body's stale "1800s ceiling" claim (design reviewer) — the description contradicts the shipped 540s ceiling; editing the PR/commit description is an author action outside the diff and carries no code risk, but should be done so future readers of the merge commit aren't misled.

[ARBITER-REVIEWED] 878d638

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

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[kyleseaman]

Round 2 disposition — head 45782a83

Both Arbiter BLOCK items fixed, plus two of the deferred MEDIUMs.

BLOCK 1 — cancel_questions_for_slot had zero production callers

Accepted in full. The design reviewer's framing was exact: a documented safety property with no call site is worse than no property.

Rather than add a second line at each stop path, the two blocking waits are now released through one chokepoint, chat_handlers._unblock_pending_waits(state, slot), which calls _reject_pending_approvals and cancel_questions_for_slot. Wired into all four paths: force-stop, soft-stop, interrupt, and slot-delete (delete releases before cancelling the task, since the slot is going away and nobody will answer its card). The reason for combining them is the defect itself — three separate sites each needing their own second line is how one gets missed, and a future third blocking wait would repeat it.

Two regression tests: one asserts the helper releases both halves; the other asserts on source that exactly one _reject_pending_approvals(slot) call site remains (the one inside the helper) and that at least four paths use the chokepoint. That second test is deliberately structural — three integration tests through the stop handlers still would not catch a fourth path added later, which is how this arose.

agent-questions.md now names the enforcing call site instead of asserting a bare guarantee.

BLOCK 2 — spec-management MUST-rule

Accepted. Added:

• ask_question to the AGENTS.md kirocrew-core tool table.
• A new Agent Questions (ask_question) section in docs/system-specs/modules/learn-cron-dashboard.md (the module that owns the approvals round-trip this mirrors): the three state methods, the both-waits invariant and its chokepoint, question and answer bounds, the timeout window and socket-margin rationale, strict session resolution, and the per-slot frontend keying. Both endpoints added to Key Endpoints.
• A clientCapabilities subsection in docs/system-specs/modules/acp-client.md with a per-key table and the consequence stated plainly: once upstream ships the bridge, inbound elicitation/create will be rejected by _reject_unknown_server_request until a handler is wired — same failure mode as today, but then ours.

Also fixed (were deferred as non-gating)

• M2 — single global pendingQuestion. Real shipped defect, cheap to fix, so fixed now rather than filed. chatSlice.pendingQuestions is keyed by slot; resolveQuestionCard deletes by ask_id match so a stale resolution cannot clear another slot's live card. Reducers tolerate the key being absent, since existing fixtures build partial preloaded state. 8 frontend tests including explicit two-slot coexistence.
• M3 — unbounded answer payload. Answers now capped at _ASK_MAX_QUESTIONS entries and _ASK_MAX_ANSWER_LEN (2000) chars per value, mirroring the question-side bounds. Two tests.

Not done

The remaining Arbiter follow-ups (grid-view ChatPane card, retry-on-submit-failure, WS-reconnect replay, duplicate question text) are untouched — they are genuine gaps but each is independent and non-gating, and folding them in would widen this PR past what the block required.

Verification

pytest 17,177 passed — down to exactly the 4 pre-existing host failures (test_dashboard_origin ×3, test_skills). The one new failure this round was test_stop_handler_idempotent, fixed by teaching its _FakeState the new state method rather than making production defensive (a getattr guard there would mask real wiring breakage). vitest 4,505 passed, tsc clean, isort/flake8 clean, mypy clean apart from the pre-existing faiss false-positive. Frontend dist rebuilt and restaged. Screenshots re-pinned to 45782a83, blobs verified.

The two Windows shard reds are inherited from main, not this PR: shard 1 is the crash_dump_store negative-age assertion (what #444 clamps) and shard 4 is the signing-secret concurrent-init convergence check. My own Windows failure from round 1 (the cp1252 decode in the capabilities test) is gone.

[kyleseaman]

Round 3 disposition — head 39054966

Claude AI Review failure — harness, not a finding

Not a code issue. The run went 84 turns over ~63 minutes and ended with "is_error": true and no structured output:

--json-schema was provided but Claude did not return structured_output. Result subtype: success
Fable 5 review step did not succeed (outcome=failure) for 45782a83… Failing closed.

The gate fails closed when the action produces no structured_output, which is correct behavior — but the cause is turn/error exhaustion inside the reviewer, not anything in the diff. Its posted comment still carries the previous head (b40e75f8, ✅ no blocking findings), which is why the summary and the check disagree. Re-running.

GPT HIGH [BLOCK-MERGE] — app-token authorization. Legitimate, fixed.

Verified the premise before fixing rather than taking it on faith: the middleware's _enforce_app_scope (token_auth.py:1080) checks only that the route appears in the calling app's manifest permissions.api allowlist, then returns None. There is no ownership check and no owner-only route seam. So an app listing /api/ask-question would pass the middleware and could target any slot — including the owner's — broadcast a crafted card, and read the user's typed answer straight out of its own blocked HTTP response. Cross-slot phishing plus answer exfiltration. GPT was right.

Fixed with _deny_app_token() on both endpoints: any request carrying request["app"] gets 403 plus a SEL app_isolation denial record. These are owner-only rather than ownership-scoped, because the legitimate callers are the ask_question MCP tool (via _post_user, a dashboard-user credential) and the dashboard UI — never an app.

That also disposes of the second half of the finding ("bind each ask_id to its originating app"): with app credentials refused outright, the only party that can answer is the single dashboard owner, who is the actor the card is addressed to. Binding would add state with nothing left to distinguish.

Three tests: an app cannot ask (403, and asserts nothing was broadcast), an app cannot answer (403, and asserts the question is still pending afterwards), and the owner path still works — that last one matters because a deny gate that also locks out the real caller would have passed the first two.

GPT MEDIUM — non-object JSON body. Legitimate, fixed.

[], null and bare scalars all parse as valid JSON, then raise AttributeError on .get() → 500 instead of 400. Both handlers now reject non-dict bodies. Test covers all four shapes against both endpoints.

GPT MEDIUM — activeSlot type narrowing. Premise wrong, but there was a real latent bug underneath.

The claim was that strict TS "can reject this call". It does not: tsc -b --force is clean under strict: true with src included, because the action creators are destructured off a large createSlice whose payload types are not narrowed at that call site. So the stated consequence (build failure) is not reachable.

I did not stop there, because activeSlot genuinely is string | null and passing null would have made delete state.pendingQuestions?.[null] a silent no-op that left the card on screen. Unreachable today — the card only renders when the slot-keyed selector found a card, which requires a non-null activeSlot — but fragile. Both dispatches now use pendingQuestion.slot, which is typed string and is also more obviously correct: clear the card belonging to this card, not to whatever happens to be active.

Deferred, consistent with the Arbiter's round-2 ruling

WS-reconnect replay, split-pane ChatPane rendering, submit-failure retry state, and duplicate question text were all raised again this round. The Arbiter explicitly considered and did not escalate these, judging each an independently fixable non-gating gap. I'm holding that line rather than reopening scope — they are listed in its follow-up section.

Verification

pytest 17,181 passed — the only failures are the 4 pre-existing host ones (test_dashboard_origin ×3, test_skills). vitest 4,505 passed, tsc -b --force clean, isort/flake8 clean, mypy clean apart from the known faiss false-positive. Frontend dist rebuilt and restaged. Screenshots re-pinned to 39054966, blob presence verified.

[kyleseaman]

Round 3 disposition — head 8c0a1499

Addressed the GPT 5.6 review of 45782a83. GPT's [BLOCK-MERGE] was driven by the pass-2 HIGH; that plus two MEDIUMs are fixed, the rest are rebutted or deferred with reasons.

GPT HIGH — app-token cross-slot phishing on the ask-question endpoints

ask_question.py

Accepted. The middleware's _enforce_app_scope only checks the route is in an app's manifest permissions.api allowlist — it does not check slot ownership. An app that lists /api/ask-question would pass scope enforcement and could then target the owner's (or another app's) slot, broadcast a crafted card, and read the typed answer out of its own blocked HTTP response — cross-slot phishing + answer exfiltration. The answer endpoint had the same gap (resolves by ask_id alone).

Fixed with GPT's first suggested option — deny app tokens outright on both endpoints (_deny_app_token), rather than ownership-scoping. Both legitimate callers are the dashboard owner: the ask_question MCP tool authenticates via _post_user (a dashboard-user token, empty app claim), and the answer POST comes from the dashboard UI. With only owner tokens accepted there is exactly one party who can be asked and one who can answer, so binding each ask_id to an originating app is unnecessary. 3 regression tests: app token refused on ask (403, nothing broadcast), app token refused on answer (403, question stays pending), owner token still allowed through.

GPT MEDIUM — non-object JSON body → 500

ask_question.py

Accepted. [], null, and bare scalars parse as valid JSON then raise AttributeError on .get() (a 500). Both handlers now reject non-dict bodies with a 400. Parametrized test over [] / null / "str" / 7 on both endpoints.

GPT MEDIUM — activeSlot narrowing at ChatPage.tsx:3538/3545

Not a live compile error — npx tsc -b is green in CI (Frontend Lint & Type Check ✅) and locally on this head, so the strict build does not reject the call. Hardened regardless: the clear now keys off the already-narrowed pendingQuestion.slot instead of the separately-selected activeSlot. This also closes a latent wrong-slot bug (if activeSlot changed between the two selectors, the old code would clear the wrong slot's card).

Deferred (Arbiter round-2 explicitly ruled these non-gating follow-ups)

Each is independent, non-breaking to defer, and was considered-but-not-escalated by the Arbiter on b40e75f8:

• WS-reconnect / refresh replay (GPT pass 1, Design 2): a pending card is not re-sent after a socket reconnect or page refresh. Real gap; the fix is a new GET pending-questions endpoint + rehydrate-on-connect mirroring /api/approvals. Deferred as a self-contained follow-up.
• elicitation capability with no elicitation/create handler (GPT pass 1, Design 1): a deliberate, documented forward-bet (see the clientCapabilities subsection added in round 2). Claude judged the failure mode acceptable; Arbiter said "track it, don't gate." Reversing it is a product decision, so left as-is.
• Split-mode ChatPane has no card (GPT pass 2): a card asked from a split pane isn't rendered. Feature-coverage gap in a dashboard-leaf; deferred.
• Retry-state on answer-submit failure (GPT pass 3): the fallback sends a normal message on any failure. UX robustness; deferred.
• Duplicate question text / option labels (GPT pass 3): answers are keyed by question text, so two identically-worded questions collapse. Edge case (agent asks two identical questions); the internal API can change freely later, so not a one-way door. Deferred.

Verification

pytest 17,181 passed (only the 4 documented host-baseline failures: test_dashboard_origin ×3, test_skills::test_flat_copy_untouched_when_nested_missing — all reproduce on clean main). isort + flake8 clean. mypy clean apart from the pre-existing faiss overload false-positive in vector_memory.py (also on main). tsc clean, vitest 4,505 passed. Single commit; screenshots re-pinned to 8c0a1499, blobs verified at the new SHA.

[kyleseaman]

Round 4 — split the ACP change out (head aeb79c01)

No code finding this round. The Claude AI Review gate failed three times on three
different heads with the same harness outcome, not a review verdict:

"subtype": "error_max_turns", "num_turns": 61, "is_error": true

The workflow runs with --max-turns 60, so the reviewer exhausted its budget and emitted no
structured output; the gate then failed closed, which is correct — a review that produced no
verdict cannot be assumed clean.

Why this PR specifically. Four recent successful Claude AI Review runs on other PRs used
34, 36, 46 and 55 turns against the 60 cap. This PR passed the gate once, at 22 files / +1249,
then failed on every head after review rounds 2–3 grew it to 27 files / +1690 — spanning ten
files of 1,000–5,200 lines (mcp_core.py 5173, acp/client.py 4476, ChatPage.tsx 3830,
state.py 3091). The prompt mandates reading every changed file with surrounding context and
re-scanning without sampling, so touching the repo's largest files is what consumes turns,
independent of hunk size. An earlier rebase cut wall-clock 63m → 31m but landed on the cap
again, so diff inflation was not the operative cause; read surface is.

Change. The ACP clientCapabilities work was an unrelated forward-bet bundled into this
PR. It is now #512, which removes 4 files and ~5,600 lines of read surface from this
review, including acp/client.py and acp/runtime.py. This PR is 27 → 23 files. The two
have no functional coupling; the Why not ACP elicitation section here now states the upstream
gap and points at #512 rather than claiming the capability is advertised by this PR.

Also moved: the two ACP capability tests, now test/test_acp_client_capabilities.py on #512.

Gates on the new head. 17,292 passed. The 3 failures are the known host-environment
test_dashboard_origin port cases that reproduce on clean main; test_skills is gone, fixed
upstream. tsc clean, vitest green, isort and flake8 clean. Screenshot URLs re-pinned to
aeb79c01 and all 5 blobs verified present at that SHA.

If Claude still exhausts its budget at 23 files, the cap itself is the constraint rather than
this PR — worth a separate look at --max-turns, given a passing run already sits at 55/60.

[kyleseaman]

Babysit round — head 8817dce1

No open code findings this round. GPT 5.6 Review ✅, Design Review ✅, CodeQL ✅ (alerts #407/#408 are dismissed test-code false positives in test_ask_question_roundtrip.py — evil.example.com substring in an assertion, not production sanitization). All non-review CI is green: backend 3.10/3.12 ×4 + Windows ×4, frontend, wheel/desktop builds, Coverage Gate, SAST/Semgrep, PR Hygiene.

Blocker: Claude AI Review gate — error_max_turns (61/60), not a code verdict. Fifth consecutive head to exhaust the reviewer's --max-turns 60 budget and emit no structured_output; the gate then fails closed (correct). This is a harness constraint, not a diff issue:

• The ACP forward-bet was already split out to feat(acp): advertise clientCapabilities in the initialize handshake #512 (27 → 23 files) and Claude still exhausts the budget, so diff inflation is not the operative cause.
• The prompt mandates reading every changed file with surrounding context and re-scanning without sampling; the turn cost is driven by read surface, not hunk size. This PR necessarily touches the repo's largest files (mcp_core.py 5173 LOC, state.py 3091, ChatPage.tsx 3830) — core seams the feature cannot avoid.
• A passing Claude run on another PR already sits at 55/60, so the cap is marginal even for smaller surfaces.

There is no further code change on this PR that makes Claude stop under 60 turns without gutting the feature. Resolution requires a decision outside this diff (raise --max-turns, or human/maintainer override). Surfacing to the user; not merging.

Worktree clean, single commit, screenshots re-verified at 8817dce1 (all 5 blobs present). PR is 1 behind main but GitHub reports MERGEABLE (not CONFLICTING/BEHIND), so no rebase — avoids restarting the ~1 h Coverage Gate for no mergeability gain.

[kyleseaman]

Round 5 — the eight remaining GPT findings (head 32dcc41d)

Round 4 landed only the duplicate-question guard, and GPT went green on the next
pass without the other eight being touched. A reviewer going quiet is not
evidence a gap closed, so all eight are addressed here. Each was verified against
the code before fixing; none turned out to be a false positive.

HIGH — non-owner dashboard tokens could ask and answer

Confirmed: both endpoints called only _deny_app_token. That rejects app
tokens, but a dashboard session token is also minted for every allowed Slack user
(!dashboard) and carries an empty app claim — so it cleared the gate while
belonging to someone who is not the owner. Such a caller could address a card at
any slot (phishing the owner with crafted options, then reading the typed answer
out of its own blocked response) or resolve a card the owner was still looking at.

Fixed with _deny_non_owner, which reuses is_owner_dashboard_request rather
than re-deriving the rule, so the dashboard keeps one definition of "owner". That
predicate accepts an exact owner_id match, or a signed
local-app/local-startup bootstrap subject when no owner is configured — which
is also the identity the ask_question tool itself carries, since its token is
minted as generate_token(owner_id or "local-app"). Local installs therefore keep
working; that was checked before choosing the predicate rather than after.

MEDIUM — session resets stranded the wait

api_chat_slot_agent, _model, _slots_model, _reasoning_effort and
_workspace all call sessions.reset, tearing down the agent. A pending question
lives in dashboard state, not the session, so it survived: card on screen, MCP
worker held, no agent left to receive the answer.

Rather than add a second call at five sites — the drift that produced the earlier
Arbiter BLOCK on this same PR — every reset now goes through
_reset_slot_session, which unblocks then resets. A source-level test asserts
exactly one raw sessions.reset remains (the one inside the helper), so a
sixth switch handler added later cannot quietly skip it.

MEDIUM — remaining six

• No reconnect rehydration. question_card is one-shot, so a reload left the
  agent blocked with nothing on screen. Added GET /api/ask-question/pending
  (owner-only, returns the already-redacted text) and a syncPendingQuestions
  re-sync on websocket open, beside the existing syncPendingApprovals.
• catch() treated every failure as expiration. Now only a 404 — the sole
  proof the wait is gone — clears the card and falls back to a message. Anything
  else (offline, 5xx, tunnel throttle) keeps the card for a retry, because
  clearing it would strand the tool call and start a second turn it could never
  join.
• No card in split view. SessionGridView's panes never rendered one, so a
  pane agent waited out its window. Both surfaces now render one shared
  PendingQuestionCard; a pane that rendered the card but not the ask_id branch
  would have had exactly the stranding bug above, so sharing the component is
  what prevents the drift.
• Submit with 1-of-N answered. The gate was some, now every: the answer map
  is keyed by question text, so a partial submit resumes the agent with a map
  missing entries it asked for, and it cannot tell "unanswered" from "never asked".
• Empty-session overlap. The welcome hero now stands down while a card is
  pending (it is centred in the space the card occupies). This was the cosmetic
  issue disclosed in the PR body.
• No frontend test for the ask_id branch. Added six, including the two that
  matter: answering must NOT send a message, and a 500 must NOT clear the card.

Verification

17,301 backend passed. The 3 remaining failures are test_dashboard_origin port
cases that reproduce on clean main (this host sets KIROCREW_PORT). vitest 393
files / 4,550 tests; tsc, isort, flake8 clean; mypy clean.

The six new frontend tests were revert-tested: flipping every back to some and
removing the 404 branch fails exactly the two assertions that should fail, and
nothing else.

Two notes on collateral, since both could have hidden a real problem. Adding the
owner gate broke ten existing tests whose MagicMock requests had no identity —
is_owner_dashboard_request reads the claim via in, [] and .get, and a
bare request["app"] returns a MagicMock that is not "". Those fakes now share
one _as_owner helper. I also tightened the two app-token tests to assert the
app-specific error string, so they can no longer pass via the new owner gate
instead of the gate they are testing.

Stale screenshot: card-dark.png was captured before the welcome-suppression
fix, so it still shows the card overlapping the centred welcome chips. The other
four are unaffected. Happy to re-capture on request.

Spec docs updated in src/kiro_crew/docs/agent-questions.md (owner-only rationale,
the new endpoint, frontend behaviour) and
docs/system-specs/modules/learn-cron-dashboard.md (endpoint list plus the
reset-chokepoint invariant beside the existing stop-path one).

[kyleseaman]

Round 6 — the HIGH plus the four sharpest MEDIUMs (head 1b8d260d)

Scoped deliberately: the HIGH and the four findings that can each cause a wrong
outcome for the user. The remaining six MEDIUMs are listed at the bottom, not
silently dropped.

HIGH — question cards were broadcast to every socket

Correct, and my own round-5 work is what made it load-bearing: I gated the three
HTTP endpoints to the owner but left broadcast_ws("question_card", ...) on the
all-clients channel. An allowed Slack user's !dashboard session registers as an
ordinary WS client, so it received the owner's question text, options, and
ask_id even though it could not call any endpoint. Owner-gating the endpoints
alone was cosmetic.

Added DashboardState.broadcast_ws_owners, which sends only to
_owner_ws_clients (the set _send_ws_owners already served for owner-scoped
slot pushes — same pattern used on #461). Both question_card and
question_card_resolved now use it; the resolved event is scoped too, since a
non-owner never received the card and has nothing to drop.

MEDIUM — clear by ask_id, not by slot

A slow response for ask A could erase a newer ask B that had already replaced it
in the same slot, leaving B blocked with no card until its own timeout.
resolveQuestionCard already existed for exactly this; clearQuestionCard({slot})
is now used only for legacy cards, which have no id to match on.

MEDIUM — no in-flight guard

A double-click fired two answerQuestion calls: the first resolved the wait, the
second 404'd, and the 404 handler then sent the answer again as a chat
message — a duplicate turn from one user intent. Submit and Dismiss now both lock
while a request is in flight.

MEDIUM — reconnect only added cards

Both WS events are one-shot, so a reload could miss either: a card that should
show is absent, or one resolved while disconnected is still on screen. Reconnect
now reconciles both directions via staleAskIds. Legacy cards are never dropped
by it — the server has no record of them, so their absence says nothing.

MEDIUM — the dismiss API was unreachable

The backend accepted {dismissed: true} and nothing in the UI could send it. Added
a Dismiss control (aria-label'd) shown only on ask_id cards, since a legacy
card blocks nothing.

Verification

17,306 backend passed, zero failures. The three test_dashboard_origin
failures reported in earlier rounds were an environment artifact of this host
exporting KIROCREW_PORT; running with env -u KIROCREW_PORT they pass, which
also retires that caveat from previous disposition comments. vitest 393 files /
4,558 tests; tsc, isort, flake8, mypy clean.

Six new tests, all revert-verified. Two worth calling out:

test_broadcast_ws_owners_targets_the_owner_client_set stubs _send_ws_all to
fail the test if it is ever called, so a future regression that reaches for
the all-clients channel is caught by construction rather than by an equality
assertion someone could weaken.

One correction to make in public: the clear-by-ask_id test I first wrote was
vacuous. It asserted end state, and in a single-card slot both clear paths
leave the same end state, so it passed against the buggy version. Rewritten to
assert the dispatched action (chat/resolveQuestionCard present,
chat/clearQuestionCard absent), which does fail on revert. Flagging it because a
test that cannot fail is worse than no test — it advertises coverage that is not
there.

The reconcile logic is exported as staleAskIds specifically so it is testable
without standing up a live socket; four cases cover stale, all-live, legacy, and
absent state.

Deliberately not in this round

Six MEDIUMs remain open, each with a posted rationale rather than a silent defer:
cancellation-unaware blocking wait in mcp_core, redaction able to collapse two
distinct questions into identical keys (round 4 deduped only pre-redaction),
no server-side validation of answers against the pending payload, missing
2000-char input cap, unbounded card height on short viewports, and the
silently-dropped fallback send in ChatPane. Happy to take them next round —
they were left out to keep this round's surface reviewable, not because they are
disputed.

Stale screenshot: card-dark.png predates the welcome-suppression fix and
still shows the overlap; it also predates the Dismiss control, so all five shots
now under-report the footer. Re-capture on request.

[kyleseaman]

Prior reviewed SHA: 1a5d300

• website/src/pages/ChatPage.tsx offline stale-card fallback: fixed. When the WebSocket is disconnected, the fallback answer is now appended to the active composer draft instead of entering send()'s intentional offline no-op; the real-ChatPage regression test preserves existing draft text and proves no API send occurs.
• src/kiro_crew/dashboard/server.py function-local handler import: rebutted. Route registration already uses function-local imports for adjacent handler groups such as autonudge and workflows, and the repository rules do not require all imports at module scope.

[kyleseaman]

Prior reviewed SHA: f61e50d

• src/kiro_crew/dashboard/state.py / history-session deletion lifecycle: fixed at the teardown root in src/kiro_crew/dashboard/handlers/sessions.py. _remove_slot_for_history_key now resolves all pending questions for the matched slot before cancelling its task or destroying its provider, allowing request_question's finally block to remove state and broadcast question_card_resolved. Focused coverage asserts the cancellation occurs while the owning task is still live.

[kyleseaman]

Prior reviewed SHA: 48b7525

• ChatPage stale-card fallback HTTP error path: fixed. Fallback answers now append to the existing composer draft for explicit retry instead of auto-sending, so resolved HTTP failures such as Kiro-readiness 503 cannot discard the answer. Added connected-WebSocket regression coverage asserting the existing draft is preserved and api.sendChat is not called.

[kyleseaman]

Prior reviewed SHA: baa0c29

• temp-screenshots/ask-question/ repository binaries: fixed. Removed all five PNGs (~470 KB) from the single feature commit, so they will not enter main history. Removed the corresponding SHA-pinned screenshot section from the PR body; it now contains zero temp-screenshots references. I did not install a third-party uploader requiring a full-privilege GitHub session cookie.

[kyleseaman]

Prior reviewed SHA: d5bb79f

• PendingQuestionCard stale response unlocking a newer submission: fixed. The finally handler now clears busyFor only when its current value still matches that request’s askId. Added a two-request race regression proving ask A settling leaves ask B disabled/in flight and a duplicate click does not make a third API call.
• dashboard/server.py function-local ask-question handler import: rebutted as non-blocking style. Adjacent route groups already use function-local handler imports, and Arbiter explicitly classified this as a reversible convention nit with no runtime or contract impact.

[kyleseaman]

/ai-review override fable 878d638: Opus timed out three times at the 30-minute infrastructure ceiling without producing a finding; all other automated gates and two independent current-head reviews passed.

[github-actions]

Human judgment recorded

@kyleseaman marked the fable AI finding as false positive, not applicable, or explicitly accepted for 878d638f7dadc63cdc682228f690062c3ede0d1e.

Opus timed out three times at the 30-minute infrastructure ceiling without producing a finding; all other automated gates and two independent current-head reviews passed.

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