fix(chat): stop replaying a productive turn as an empty response

[pepmach]

Problem / Motivation

A dashboard session produced three consecutive Empty model response warnings. Two of them were not empty: the transcript shows an assistant preamble and successfully completed tool calls, and the usage records show real billed inference ending on a clean end_turn.

assistant_text is reset at every tool boundary, so a turn that answers and then calls a tool reaches the terminal chain with an empty final segment. Nothing recorded that visible output had already been flushed — _produced_visible_output is deliberately narrow (it is only set on paths that reset the buffer mid-turn, and the promise-only guard depends on that meaning). So the empty-response ladder classified a productive turn as "the model returned nothing" and took rung 1, which re-queues the ORIGINAL user message. That re-runs tool calls that already completed — a second send_message, a second write, a second PR — and re-derives an answer the user has already read.

The third attempt was genuinely empty, and could not be diagnosed at all. Five physically different realities collapsed onto the same branch and the same log line:

• the provider generated nothing (end_turn, no content),
• the terminal carried no stop reason,
• no terminal event arrived,
• the terminal was synthesized by the provider layer,
• the turn produced only tool calls or thinking.

The warning printed none of the state the runner already held, so the three attempts were indistinguishable in the log even though they had three different owners.

What changed

A productive turn can never be replayed verbatim. EmptyTurnActivity.productive is the load-bearing predicate — a flushed visible segment, a dispatched tool call, or thinking. Such a turn skips rung 1 entirely and gets at most one continuation, whose body does not tell the model its completed work produced no output (that wording is itself an invitation to redo the side effect). That continuation consumes the remaining recovery budget, so a second continuation can never follow.

A genuinely activity-free empty turn is unchanged — it keeps the bounded replay → continue → give-up ladder, which is the self-heal a real provider-side empty depends on.

The verdict now names its cause. One warning per verdict, emitted after the rung is chosen, carrying a closed cause (no_terminal_event, synthetic_completion, visible_partial, tool_only, thinking_only, provider_empty, other), the rung, a normalised stop reason, and booleans. An omitted stop reason is deliberately distinct from a clean end_turn: "the provider said the turn ended and produced nothing" is a model-side event, "the provider never said why it stopped" is a transport-side one, and that is the distinction the incident's third attempt needed and did not have.

Two ACP-side silences are closed. The pre-turn drain destroys frames from an abandoned turn — possibly including that turn's terminal — and said nothing; it now reports how many, once per turn. And a prompt stream that exhausts cleanly without ever yielding a terminal completion now warns, which is the one state the dashboard structurally cannot tell apart from a model that answered with nothing. Only clean exhaustion is reported, so an ordinary consumer close or cancellation stays silent.

Privacy

Every diagnostic value is a bool or a member of a closed set. No prompt, no response text, no thinking, no tool arguments or results, no paths, no identities, no token counts and no costs — billing is a bool (billed), which is exactly the fact needed to separate "the provider ran and charged for this turn" from "it never ran". Frame counts stay in the drain's own log line and carry no sizes, because a size leaks response length.

Testing

• TestProductiveTurnNeverReplaysVerbatim — the incident's own shape (TEXT → TOOL_CALL → TOOL_RESULT → COMPLETE(end_turn)) must not re-queue the prompt, must queue exactly one continuation, and the completed tool must be dispatched once. Plus tool-only, thinking-only, and the scoping test that a genuinely activity-free turn keeps rung 1.
• TestEmptyTurnDiagnostics — cause and rung vocabularies asserted from the classifier's own inputs (the ranking between overlapping causes is what regresses silently); provider-empty vs no-terminal reported distinctly; an omitted stop reason not laundered into provider_empty; and a privacy test that drives a turn carrying secret-marked text, a secret-marked tool title and real billing amounts, then asserts none of them appear in the rendered line while billed=True does.
• ACP: the drain reports a count and leaks no frame content; a clean exhaustion without a terminal warns, while a consumer close does not.
• Mutation-checked. Replacing the productive-turn guard with True turns exactly the three productive cases red (the ORIGINAL user message was re-queued after a turn that already ran a tool) while the provider-empty case stays green.

test_subagent_delivery_ttl_anchor.py's consumption-signal guard is re-anchored on the rung marker rather than the branch condition, because that condition now carries the guard and is reformatted whenever it grows a term.

Validation

Rebased onto current main; the three affected files are 1,097 passed, and mypy --platform linux is clean across 1,288 files. Black ratchet, subprocess-encoding ratchet, isort, flake8, docs-lint and the brand gate all pass. session_handle.py stays grandfathered in the black baseline — the added hunks are black-clean, and formatting the file wholesale would rewrite 33 pre-existing regions.

Correcting an earlier claim in this PR's first revision: the first push attributed a set of local shard failures to the environment. That was wrong for the ones that mattered. test/test_slot_close_recreation_race.py was failing because the branch was based on a main that predated #8536, #8549 and #8583 — the three commits that widened the slot-race request double's can_read_body surface — and #5697, which serializes slot model switches under the slot lock. On the stale base that file timed out; on current main it is 42 passed in 4.8s, with no change to this PR's code. The rebase is the fix, and no source change was needed to obtain it.

test/test_transcribe.py still cannot be collected in the local CI-parity venv (imageio_ffmpeg absent). That one is genuinely unrelated — this branch touches no transcribe, voice or STT code.

Pattern harvest

Rule candidate: semgrep
Pattern: a boolean "did this turn produce output" flag consulted on a recovery or retry path that can re-send a side-effecting request

Defect class: a predicate whose narrow, documented meaning is correct for its original caller, reused by a later branch that needs a broader question answered — here "did this turn produce visible output" answered by a flag that only means "was the buffer reset mid-turn". The second caller acts on a false negative, and because its action is a replay, the cost is duplicated side effects rather than a merely wrong message. The two callers are 4,000 lines apart, so neither reads like a misuse locally.

Retired by: the activity snapshot is assembled from state the runner already held (terminal seen, synthetic, text, flushed, tools, thinking, billing) and the recovery decision reads productive, so a future branch asking the same question gets an answer scoped to it rather than borrowing one. The mutation check pins that the guard is load-bearing, and the cause enum makes a misclassification visible in the log instead of silent.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A field-observed side-effect-duplication bug fixed at the exact decision point that caused it, with rejected alternatives explained and the recovery bound preserved.

The guard is scoped correctly: broadening _produced_visible_output was considered and rejected for a stated load-bearing reason, the productive turn still spends the same ladder budget (counter forced to 2, so no second continuation), and the genuinely-empty path keeps its existing self-heal untouched. The diagnostics honor a named boundary (content-free by construction, wire strings normalized onto a closed set), the deliberate non-bridging of the drain count avoids a provider-contract change disproportionate to the fault, and specs are updated in the same commit. Everything added is reversible log-and-branch logic with no new public surface.

[DESIGN-REVIEWED] be245e3

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/chat_runner.py:10050 -- "slot.key" leaks channel/thread identifiers despite the no-identities diagnostic contract -> Fix: remove the slot placeholder and argument.
[GPT-REVIEWED] be245e3

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings that block; one advisory.

FINDING — src/kiro_crew/dashboard/chat_runner.py:10029 — a depth-0 productive empty turn that skips rung 1 (_may_replay_verbatim False) and skips rung 2 (auto-continue disabled via session.empty_response_auto_continue=false, or _should_suppress_requeue True) now reaches the give-up else with _empty_response_retries==0 and shows "it was retried and auto-continued automatically" when neither a retry nor an auto-continue occurred → Fix: branch _empty_msg on _empty_activity.productive (in scope here) so the give-up notice for a productive turn does not assert a retry/continue that never ran.

[OPUS-REVIEWED] be245e3

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of be245e334647334c8531765667c0f763fa2c83e1 — 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.

The empty-response ladder exists only in chat_runner.py (no sibling ladders elsewhere — _empty_response_retries has one owner), the author's claim that _produced_visible_output is deliberately narrower checks out (18 uses, all tied to mid-turn buffer resets and the promise-only guard), and metrics/turns.py:turn_outcome — the one existing stop-reason mapper — folds an absent reason into "ok", which is precisely the distinction the diagnostic exists to preserve, so normalize_stop_reason is meaningfully different rather than a second spelling. Everything in the diff is declared in the description, and each rider traces to the same reported field incident.

First-Principles-Verdict: PASS

A reported replay-of-completed-side-effects defect is fixed at cause level, and every rider carries a harm named by the same incident.

What this change ships

Intent: stop the runner from silently re-running a user's message — and its completed tool calls — after a turn that did work but ended with an empty final segment. This is a FIX.

1. A turn that answered or ran tools is never re-queued verbatim after an "empty" verdict — the fix; justified.
2. Such a turn gets exactly one continuation, worded so the model does not redo completed work — justified (old wording invites the duplicated side effect).
3. That continuation spends the whole recovery budget (no second continuation) — declared, justified.
4. A genuinely activity-free empty turn keeps the old replay→continue→give-up ladder — scoping, justified.
5. The empty-response warning now names a closed cause, rung, and booleans — rides along; justified (the incident's third attempt was undiagnosable; a reported defect).
6. New notice-card wording for the productive case — justified (existing text is false for a turn that did work).
7. The continuation reuses the existing recovery marker instead of minting one — a considered subtraction; justified.
8. ACP pre-turn drain warns with a count of destroyed frames — rides along; justified (a destroyed terminal reached the dashboard as an unattributable empty).
9. ACP prompt stream warns on clean exhaustion without a terminal — rides along; justified (the one state the runner cannot classify itself).
10. New chat_utils surface (EmptyTurnActivity, classify_empty_turn, normalize_stop_reason, cause/rung constants) — 1 real consumer each (chat_runner.py); the closed-set shape is derived from the no-content-in-logs invariant (issue No diagnostics when the model backend rejects a turn (Improperly formed request) #6022 precedent in acp-client.md).

Duplication checks run: _produced_visible_output (18 uses, all mid-turn-reset semantics — reusing it would break the promise-only guard, as the author states); turn_outcome (metrics/turns.py:115 — reads absence as "ok" by documented design, so it cannot serve here); _empty_response_retries ladder (single instance, no unfixed siblings). The three mirrored ACP stop-reason literals are mandated by the check_agent_sdk_boundary baseline and pinned by test — a recorded decision, not relitigated.

[FIRST-PRINCIPLES-REVIEWED] be245e3

[buluoray]

Reviewed against source at be245e3, with the new guards mutation-tested rather than taken on trust. 0 blocking findings.

The fix is at the right decision point. chat_runner.py:9942 sets _may_replay_verbatim = not _empty_activity.productive and gates the rung-1 verbatim replay on it at :9943, where productive = flushed_visible or had_tools or had_thinking. A turn that already emitted visible text or ran tools no longer gets its message replayed as a fresh empty response; a genuinely activity-free turn keeps the old recovery. The recovery budget is preserved by forcing _empty_response_retries = 2 on the productive path, so exactly one continuation follows and a second cannot.

Mutation results — every new test pins its intended behavior, and the scoping tests correctly stay green:

Mutation (source)	Result
_may_replay_verbatim = not …productive → = True	RED: the 3 productive tests; test_activity_free…keeps…rung stays GREEN
Drop counter-forcing (= 2 → += 1)	RED: test_text_then_tool_turn_continues…
normalize_stop_reason: absent → _STOP_END_TURN	RED: 2 tests; the provider-empty distinctness test stays GREEN
ACP drain _stale_dropped += 1 → += 0	RED: test_pre_turn_drain_counts_discarded_frames…

Also verified: _produced_visible_output was correctly not reused (all 18 uses are mid-turn buffer resets, a different concept); the continuation body reuses EMPTY_RESPONSE_RECOVERY_PREFIX rather than minting a new marker/locale pair; the rendered warning keeps billed=True while omitting prompt, preamble, tool titles and token counts; and session.md + acp-client.md are updated in the same commit per the same-commit spec rule.

Non-blocking, worth a follow-up:

1. The give-up notice can overstate what happened. At chat_runner.py ~10025 the else sets an unconditional "it was retried and auto-continued automatically", but a productive turn skips rung 1 and — when rung 2 is also skipped (empty_response_auto_continue=false, or _should_suppress_requeue) — lands there with _empty_response_retries == 0, asserting a retry that never ran. Notice-card wording only; the duplication fix itself is unaffected. Branching _empty_msg on _empty_activity.productive would close it.
2. slot.key in the new diagnostic warning is not new exposure — the predecessor warning already logged it, and the EmptyTurnActivity payload is all bools and closed constants.

Not verified: the full suite / mypy / ratchet claims (I ran the targeted files only, per scope) and the production incident logs themselves, though the reproduced TEXT→TOOL_CALL→TOOL_RESULT→COMPLETE(end_turn) shape matches the described symptom.