fix: stop recovery after question cards

[junjiequ]

Problem / Motivation

A successful non-blocking ask_question call posts a dashboard card and explicitly tells the model to end its turn without assistant text. The dashboard runner currently treats that intentional tool-only ending as an empty response and queues a synthetic continuation. The continuation can re-enter the same unresolved question intent and post the same card again.

Why it matters

A user can become trapped behind a question card that reappears after they answer or dismiss it. Repeated recovery turns also waste model work and make the session appear to ignore the user's response.

What changed (motivation → approach → change)

The card itself is the intended terminal output, so the fix records a turn-local terminal outcome only when ask_question was successfully delivered to an attached dashboard client. The marker and out-of-band directive paths both feed that outcome. The successful-card prefix is defined beside the directive applier and imported by the runner, so control flow cannot silently drift from outcome wording. The empty-response ladder is skipped for that one successful terminal case.

Delivery failures keep the existing behavior, allowing the model to fall back to asking in plain text. Other tool-only turns still receive the normal closing-continuation recovery.

Tests

• Added an integration regression that drives the real dashboard runner and real directive applier.
• Asserts exactly one question-card delivery.
• Asserts no original-prompt replay or synthetic continuation is queued.
• Asserts the empty-response retry counter and recovery notices remain untouched.
• Existing directive-seam tests remain green.

Manual verification

N/A — the integration test reproduces the exact tool-call/result/turn-complete shape and observes both card delivery and the runner queue.

Related Issues

No linked issue: field-reported incident without a public issue.

Pattern harvest

Rule candidate: review-prompt
Pattern: A successfully delivered non-blocking question card must terminate without generic missing-closing-reply recovery.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 0340669

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

The design story checks out end-to-end: the flag is recorded at both directive-consumption paths (marker and out-of-band), only on the delivery-success outcome, and gates only the empty-response elif — delivery failures and generic tool-only turns still fall through to the existing ladder. The string-prefix coupling is mitigated by the shared constant, and the new integration test pins the seam so wording drift breaks red, not silent. The owning spec (session.md) is updated in the same commit. No design-level findings survive.

Design-Verdict: PASS

Terminal-outcome flag is recorded at the delivery point of truth, gates only the empty-response arm, and failure paths keep the plain-text fallback.

[DESIGN-REVIEWED] 0340669

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The single candidate concerns the post-compaction continuation arm at chat_runner.py:10644, a code path the diff does not touch. For it to fire, should_continue_after_compaction requires a real compaction that both started and completed mid-turn (compaction_started and compaction_settled), the turn ended on a normal end_turn with blank final_segment_text, no queued follow-up/steer/stop, prompt_depth == 0, and an unspent one-shot budget — AND, in the same turn, a successful ask_question card.

The candidate requires me to establish that this conjunction occurs in practice. I cannot: mid-turn compaction is a context-ceiling event whose co-occurrence with a successful non-blocking question card in a single turn is not something I can derive at the required confidence from the code — the candidate's (a) resolves to "rare but reachable," which is the "could/might" case Step 1 drops. It is also on an unchanged line whose behavior the PR neither introduces nor worsens (before this PR the empty-response ladder fired on the same shape). It fails the falsification bar and does not survive.

The changed code itself is sound: both record sites (_record_terminal_question) pass the raw applier result before _redact_tool_field, the prefix match is exact, delivery-failure outcomes ("no dashboard client attached", etc.) do not match QUESTION_CARD_SHOWN_PREFIX and correctly preserve normal recovery, and the guard is additive on the empty-response elif. No new grounded defect found.

No findings.

[OPUS-REVIEWED] 0340669

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

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

I have everything I need. Checking the one remaining count — usages of the new prefix constant and the directive kinds sharing the "end your turn" outcome shape are confirmed (grep "End your turn" in src/kiro_crew/dashboard/session_directive_apply.py: 2 hits — _ask_question at line 852, fixed, and the monitor/auto-nudge applier at line 288, untouched).

First-Principles-Verdict: CONCERNS

The fix is real and mechanism-level, but it terminalizes only ask_question; the monitor directive's identical "End your turn now" outcome keeps the same continuation.

What this change ships

Intent: stop an answered/dismissed question card from reappearing because the runner "recovers" a turn the tool deliberately ended silently — a FIX.

1. A delivered question card no longer triggers replay/continuation/give-up recovery — justified (field-reported trap).
2. Card delivery failure still gets normal recovery, so the model can re-ask in text — justified.
3. The card-success wording becomes a shared constant (QUESTION_CARD_SHOWN_PREFIX) imported by the runner — declared; one consumer, but it exists to stop wording/control-flow drift.
4. session.md gains the terminal-question paragraph — mandated same-commit spec update.
5. Test harness _drive can now run the real directive applier (applied_result=None) — declared, test-only.

Watch

• Point patch with 1 counted unfixed sibling: grep "End your turn" in session_directive_apply.py = 2 outcomes; _monitor_start (line 288, "End your turn now — the loop wakes you") also declares a terminal tool-only turn, so it still receives the closing continuation this PR calls "wasted model work". Weaker harm (no card to re-post, the loop wakes the model regardless) and no field report, so accepted-and-deferred — but the chosen shape (a per-kind prefix sniff named _record_terminal_question) cannot absorb that sibling without a second sniff; a human should decide whether the deferral is deliberate.

[FIRST-PRINCIPLES-REVIEWED] 0340669

[junjiequ]

• fixed

The successful-card prefix is now defined beside _ask_question as QUESTION_CARD_SHOWN_PREFIX and imported by the runner, so control flow and the applier outcome share one definition. The real-applier integration regression remains the end-to-end guard. Fixed in 385345375ba92e8a9557dea5a5f795afb2b5cb4f.

[junjiequ]

• needs-a-decision

The sibling observation is legitimate: successful monitor/subagent paths also instruct the model to end, and a structured terminal-tool contract would be broader than this question-card incident fix. This PR now narrows its Pattern harvest to the non-blocking question-card defect rather than claiming that whole class is closed.

I attempted the repository's required accepted-and-deferred path, but a fork contributor cannot assign an upstream issue, so it cannot satisfy the mandated label + assignee + due-date contract. Maintainer decision requested: should this PR expand into the structured terminal-tool contract, or should a maintainer create/assign that follow-up?

[junjiequ]

CI note: the Backend Tests (Windows) (4) failure is inherited from current main, not this diff.

Evidence:

• test_empty_session_stats[False/True] fail identically when run alone.
• The only mismatch is the production payload's new refused_transcripts: 0 field missing from the expected dictionary.
• This branch has no diff in src/kiro_crew/dashboard/handlers/usage.py or test/test_usage.py.
• fix(usage): add refused_transcripts to the empty-stats expected dict #9322 is the existing targeted fix, and its Windows shard 4 is green.

I am leaving this unrelated base repair out of #9320. After #9322 lands, this branch should rebase onto the fixed main and rerun CI.

[junjiequ]

Update: the refused_transcripts expectation fix is now present on main at 12ecd4512d29. This branch has been rebased onto that fixed base and force-pushed as 0340669408c4bb41005858d8f9f8fc06c8d71c1f.

Post-rebase targeted validation (single process, no full local suite):

• question-card terminal-turn regression: passed
• test_empty_session_stats[False]: passed
• test_empty_session_stats[True]: passed

The inherited Windows/coverage reds from the prior head should clear in this new CI round.

[iamwhatever]

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: a successfully delivered non-blocking ask_question card is the turn's intended terminal output, but empty-response recovery treated it as a generic tool-only turn and injected a closing continuation that could re-post the same card; the runner now records the delivered-card outcome and skips the empty-response ladder, while delivery failures keep the old fallback. Spec files changed as a ride-along (a minority of the diff on both file count and changed lines), not reviewed as a design decision: docs/system-specs/modules/session.md. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.