fix(acp): retry a turn abandoned after a throttled compaction

[iamwhatever]

Problem / Motivation

A dashboard turn on the KAS backend ends with a chat row reading, in full:

❌ Compaction failed: error

and the user's message is gone. Nothing further happens, nothing is queued, and
gateway.log has no compaction line at all to explain it — the reason on the row
is the only record, and the reason is the word "error".

The actual cause is in the backend's own log, not ours:

[SummarizationNode] Summarization failed {"name":"ModelThrottleError",
 "cause":{"name":"InternalServerException","httpStatusCode":500,"attempts":3,
          "reason":"MODEL_TEMPORARILY_UNAVAILABLE"},
 "userFacingSessionErrorMessage":"The model you've selected is experiencing a
   high volume of traffic. Try changing the model and re-running your prompt."}

KAS runs summarization as a separate billed model call on the session's own
model, so when that model is throttled the summarization call is throttled with
it. It reports summarization_failed, Crew arms the post-failure budget, the
backend never answers the prompt, and the turn ends with
STOP_REASON_COMPACTION_FAILED — which the dashboard treated as permanent.

Why it matters

On the dashboard, every throttled summarization silently drops a turn that would
have succeeded on the next attempt, and tells the user nothing they can act on. The throttle is
transient and common: six occurrences appeared in a single ~20-minute local log
window, five of them hitting ordinary turns as well. On a long session the
message lost this way can be the one carrying an hour of context.

The blank reason is the second half of the cost. With no reason on the row and no
log line server-side, there is nothing to grep and nothing to correlate — the
failure is unreportable, which is why it went unexplained rather than unnoticed.

What changed (motivation → approach → change)

Three defects on one path, each fixed at its own layer.

1. The reason was read one level deep. compaction_failure_detail checked
four flat keys on status and then on params. KAS puts a placeholder in the
flat position and the real cause under cause, so the placeholder won and became
the whole notice.

The reader is now a ranked walk over the frame: it collects every named reason at
any depth, prefers the sentence written for the user
(userFacingSessionErrorMessage) over the machine enum, and rejects placeholder
words (error, failed, unknown, …) so an uninformative one falls through to
the raw shape instead of becoming the notice. Separators are folded before
matching, because the same fault arrives as prose in one field
("temporarily unavailable") and as a SCREAMING_SNAKE enum in another
(MODEL_TEMPORARILY_UNAVAILABLE).

2. The KAS summarization branch logged nothing. AcpClient's kiro-cli twin
logs the whole notification at WARNING; the KAS branch in session_handle.py had
no logger call, which is why the chat row was the only evidence. It now logs the
full frame, so the next occurrence is debuggable from our own logs — including
which field the reason arrived in.

3. The abandoned turn was dropped unconditionally. That is correct for a
conversation that overflows the window: replaying it repeats the same overflow,
which is what the original no-retry comment reasoned about. It is wrong for a
throttled or 5xx'd summarization call, which has nothing wrong with it.

The reason is now classified from the structured payload
(compaction_failure_is_transient) rather than the rendered notice — the notice is
truncated, redacted, and sometimes only a raw repr, so matching its prose would
both miss real throttles and fire on a digit that happened to land in a summary.
An HTTP status is compared as a number under its own key, never as a substring,
and bool is excluded because it is an int subclass. The string scan is scoped
to the same reason-bearing keys the reason reader ranks: the frame also carries
backend-echoed, conversation-derived text (conversationSummary rides in the very
payload the KAS branch passes whole), so scanning every string leaf would let a
summary that merely mentions "timeout" upgrade a permanent overflow to transient.
A control decision must not be reachable from content the model wrote. A transient verdict
re-queues the abandoned message once (budget 2, its own budget rather than a share
of _acp_pipe_death_retries), guarded on not _turn_emitted so a verbatim replay
can never repeat a side effect. An emitted turn, and every permanent reason, keep
the existing give-up behaviour.

The verdict is set where the frame arrives (AcpClient, AcpSessionHandle) and
read where the turn ends (the dashboard's AcpProvider), so it is forwarded
through both wrapper hops — AcpProvider → AcpSessionProvider → the live
handle — mirroring the existing exit_code / last_prompt_stats properties, and
declared on the LLMProvider ABC with a safe False default so an adapter that
reports no verdict gives up the turn exactly as it did before. The boolean is
the whole of the new contract: the reason text is not forwarded, because the
chat row gets it from the compaction-status event title and the server log from
the WARNING each arming site emits. That
hop is load-bearing rather than cosmetic: without it the read falls to its default
and the retry is unreachable on exactly the KAS path it was written for.

Deliberately not included: retrying the compaction itself on
fallback_model. #8159 declines Crew-driven compaction on KAS entirely (KAS
cannot serve /compact, and dispatching one stranded the turn semaphore for the
full 300s budget), so on that backend there is no Crew-side compaction call to
retry. Re-running the turn is the recovery this path can actually offer, and
KAS's own remedy — try another model — now reaches the user because fix 1 puts the
sentence carrying it on the row.

Scope: the dashboard only. slack/handler.py:3694 and
messaging/dispatch.py:654 still return on this stop reason and still drop the
turn. Both predate this PR and neither is a regression from it; each surface has
its own requeue mechanics, so porting the branch verbatim is not the right shape.
The classifier and the provider contract added here are surface-agnostic, so each
one needs only to read the verdict and re-queue — tracked in #8256.

Tests

test/test_acp_client.py

• test_rejects_a_placeholder_reason — a named reason of "error" does not become the notice; the raw shape is surfaced instead. This is the reported symptom.
• test_prefers_the_user_facing_sentence_over_the_machine_reason — on the nested pair KAS actually sends, the sentence wins over cause.reason.
• TestCompactionFailureIsTransient (9) — throttle by error name; the SCREAMING_SNAKE spelling of the reason enum; a nested 5xx; 429; a 4xx staying permanent; an overflowing conversation staying permanent; httpStatusCode: True not read as a status code; a conversationSummary mentioning "timeout" not flipping a permanent overflow; and a genuine throttle beside that summary still matching, so the scoping costs no real case.
• TestCompactionVerdictReachesTheConsumer (5) — the ABC declares the verdict (and deliberately not the reason text), each wrapper hop forwards it, a client that never set it reads as permanent rather than raising, and a truthy stand-in is not accepted as a verdict.
• test_the_reason_and_the_verdict_read_the_same_frame — both readers run off one walker, so the displayed reason and the retry decision cannot be derived from different views of one frame.

test/test_chat_runner_coverage.py

• test_a_throttled_compaction_requeues_the_abandoned_message — the transient verdict re-queues and dispatches a follow-up turn, without touching the pipe-death budget or claiming a lost connection.
• test_the_throttle_requeue_is_bounded — a spent budget buys no further attempt and adds no notice.
• test_a_transient_failure_after_output_is_not_replayed — an emitted turn is never replayed, even for a retryable reason.
• test_compaction_failure_neither_retries_nor_claims_a_lost_link — docstring narrowed to the permanent case it actually pins; behaviour unchanged.

All new tests are mutation-verified: 14 fail against pristine src/, and the four
forwarding tests fail with either property removed.

Manual verification

N/A — unit coverage sufficient. The trigger is an upstream model throttle, which
cannot be induced on demand; the payload shapes under test are taken verbatim from
the [SummarizationNode] Summarization failed frames in the log quoted above.

Local gates on the rebased head: black, isort, flake8, mypy (1281 files), and the
12 ratchet gates all green; 1853 tests green across the ACP client, session
handle, session provider, provider, KAS backend, chat runner, dashboard chat and
connection-recovery suites.

Related Issues

No linked issue: diagnosed from a live session's own logs rather than from a
filed report.

Pattern harvest

Rule candidate: review-prompt
Pattern: a value read with getattr(wrapper, field, default) where the field is
only ever set on an inner object — the default silently makes the branch
unreachable, so the feature is dead code that no test using a mock will catch.

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]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real dropped-turn harm, classified fail-safe from structured payload, bounded retry, additive ABC contract mirroring existing property hops — sound and proportionate.

The one inherent fragility — the transient markers mirror KAS's current error vocabulary, so an upstream rewording silently degrades — fails toward the pre-PR give-up behavior with the numeric httpStatusCode check as a wording-independent backstop, so it costs a retry, never correctness. Misclassification in the other direction is capped at two verbatim replays of the user's own pre-output message. Scope exclusions (Slack/messaging surfaces, #8256) and the declined alternative (retrying compaction itself, foreclosed by #8159) are both accounted for.

[DESIGN-REVIEWED] 179b60b

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 179b60b90d0fd15b3a2e83cb09a99405a60482a3 — 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 checks are done. Composing the review.

First-Principles-Verdict: CONCERNS

Every item traces to the reported dropped-turn defect, but the classifier ships a third "is this transient?" vocabulary beside two the codebase already declares single-source.

What this change ships

Intent: stop a throttled KAS summarization from silently eating the user's message — a FIX.

1. Chat row shows the backend's real (nested, user-facing) reason instead of "error" — justified
2. Placeholder words ("error", "failed") fall through to the raw payload — justified
3. KAS summarization failures now logged redacted at WARNING — justified
4. Turn abandoned after a throttled/5xx compaction re-queues once (budget 2) with a "retrying…" row — justified
5. Overflow/permanent failures keep the give-up behaviour — justified
6. New compaction_failure_is_transient with its own 10-marker list — duplicate vocabulary, see Watch
7. last_compaction_transient on the ABC + two wrapper hops — justified (AGENTS.md harness-parity mandates ABC-with-safe-default); 1 consumer counted (chat_runner.py:9392), hops are load-bearing plumbing
8. Per-slot _compaction_failed_retries budget, reset with its siblings — justified
9. Slack/messaging still drop the turn — declared deferred; 2 siblings counted (slack/handler.py:3696, messaging/dispatch.py:654, grep STOP_REASON_COMPACTION_FAILED)

Watch

• Third transient vocabulary. _COMPACTION_TRANSIENT_MARKERS (client.py:1167) lands 240 lines above the comment declaring the regex family the "Single source of truth for 'is this ACP backend error a momentary, retry-worthy hiccup?' … so the two can never drift again" (client.py:1403), and beside llm_helpers._TRANSIENT_MARKERS — I count three vocabularies now. Divergence is already live: both existing classifiers put usage-limit/auth terminal-first ("ahead of the throttle check so limit wording that also reads as rate-limiting stays terminal", client.py:1659), but the new one matches "rate limit" with no such precedence, so an exhausted allowance phrased as rate-limiting replays twice to no purpose. The description never weighs reusing them.

Subtractions

• Shrink _COMPACTION_TRANSIENT_MARKERS to the observed KAS throttle family and route the folded reason text through the same-file _RE_THROTTLE_*/_RE_5XX_* patterns (with their terminal-first precedence) rather than a third list — llm_helpers is off-limits (it imports acp.client), the regex family is not.
• Drop "timed out" / "timeout" from the marker list: no reported occurrence carries either (the defect log shows throttle/500 only), no test sends one as an arriving reason, and both existing vocabularies deliberately omit plain timeout.

[FIRST-PRINCIPLES-REVIEWED] 179b60b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The single candidate concerns the asymmetry that httpStatusCode (the numeric transient check) is not key-scoped the way the string-marker scan is restricted to _COMPACTION_DETAIL_KEYS. Falsifying it:

• (a) requires a concrete failed frame where a nested httpStatusCode integer in the 5xx/429 range rides in from model/conversation-derived content rather than the failure structure itself. The only named model-influenced field, conversationSummary, is a plain string — the walk yields it as a string leaf, and a string cannot introduce a nested dict with an httpStatusCode int key. Dict structure/keys of the KAS notification frame are backend-generated, not model-authored.
• The candidate's own author could not ground such an input and rates it "low" confidence. Without a concrete input that occurs in practice, (a) fails, so (b) and (c) never engage.

The remaining changed code (verdict forwarding through the provider ABC with a safe False default, is True coercion against truthy stand-ins, the retry guards against stopped/superseded/emitted turns, bounded budget, redacted KAS frame logging) is guarded and consistent with its sibling recovery branches; nothing new grounds to the (a)/(b)/(c) bar.

No findings.

[OPUS-REVIEWED] 179b60b

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 179b60b

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

[iamwhatever]

• span=6dd7854a8bdf — fixed in 0588e6ede01e7621a07c5bc06785880ab3a932a8

new provider capability last_compaction_transient is declared only on the Acp providers, not on the LLMProvider ABC, so a missing attribute silently reads as False

Legitimate, and rule-backed rather than stylistic: the shape is exactly what blocking harness-parity H14 names, and child_fidelity_aware is the in-repo precedent for the remedy. Declaring only the leaf providers left the reader's getattr default as the contract for every other adapter — an unwritten default, which is the thing the rule exists to stop.

src/kiro_crew/providers/base.py now declares both members on LLMProvider as properties with safe defaults (last_compaction_failure → "", last_compaction_transient → False), matching child_fidelity_aware's shape and its "default is the SAFE value" reasoning. False is safe in the strict sense that matters here: an adapter reporting no verdict gives up the turn exactly as it did before the capability existed, so the new retry can only ever be reached on a provider that positively said the failure was transient.

Pinned by test_the_abc_declares_the_capability_with_a_safe_default, which reads the values through the descriptors (the ABC cannot be instantiated) and fails with the declaration removed.

[iamwhatever]

• Design Review CONCERNS — fixed in ec3c02e7268e78b7ae3e1ca5fe8f2a87fe512ac6

compaction_failure_is_transient walks the whole notification and marker-matches any string field … untrusted content can flip a control decision. Scoping the transient scan to reason-bearing keys (reuse _COMPACTION_DETAIL_KEYS + httpStatusCode) closes it without losing any case the PR targets.

Legitimate, and it caught the PR contradicting its own stated rationale: the body argued against matching prose because it would "fire on a digit that happened to land in a summary", and then the classifier scanned every string leaf — conversationSummary included, in the very frame the KAS branch passes whole to compaction_failure_verdict(kiro). Model-written text could therefore reach a control decision.

Taken exactly as recommended: the string scan now skips any key not in _COMPACTION_DETAIL_KEYS, the same ranked set the reason reader uses, and the numeric httpStatusCode branch is unchanged. Every case the PR targets keeps matching, because each one arrives in a reason-bearing key (name for ModelThrottleError, cause.reason for the enum, userFacingSessionErrorMessage for the sentence).

Two tests pin both directions: test_the_conversation_summary_cannot_flip_the_verdict (an overflow frame whose summary mentions "timeout", "rate limit" and "service unavailable" stays permanent) and test_a_reason_bearing_key_still_matches_beside_that_summary (a genuine throttle beside that same summary is still transient, so the scoping costs no real case). The first fails with the guard removed.

[iamwhatever]

• First Principles CONCERNS — accepted-and-deferred, and the description claim fixed in ec3c02e7268e78b7ae3e1ca5fe8f2a87fe512ac6

The fix is real and cause-level, but it repairs only the dashboard while the description claims "every throttled summarization" — Slack and messaging keep dropping the turn.

Both halves are correct, and they get different answers.

The description claim was wrong, and that is fixed here. "Why it matters" now reads "On the dashboard, every throttled summarization…", and a new Scope paragraph names slack/handler.py:3694 and messaging/dispatch.py:654 as still returning on this stop reason, notes that both predate this PR, and links the tracking issue. The overclaim was mine and in scope, so it is corrected rather than deferred.

The two surfaces are deferred, deliberately. Each has its own requeue mechanics — Slack and messaging dispatch do not share the dashboard's _queue_recovery — so porting the branch verbatim is the wrong shape, and doing all three here would roughly triple a diff whose blocking findings have already cost two rounds. Nothing is lost by splitting: the classifier and the LLMProvider contract added here are surface-agnostic, so each site needs only to read the verdict and re-queue.

Tracked in #8256 (deferred-finding, assigned, Due: 2026-09-17), which carries the two exact call sites, what this PR already provides, the per-surface test requirement, and your observation that this is the repo's third transient-classifier marker list — worth folding into that work rather than adding a fourth here.

[iamwhatever]

• Subtraction: drop last_compaction_failure from the provider contract — fixed in f2d405dbc9fb749472f836a17489398d2ab0b033

grep shows exactly 1 consumer, the 80-char log interpolation at chat_runner.py:9409. The row's reason already flows via the compaction-status event title, and item 4's WARNING already records the full frame server-side. Forward only the bool; compaction_failure_verdict then collapses into a direct compaction_failure_is_transient call.

Verified and taken in full. The grep held — one consumer, and a truncated duplicate of what each arming site had already logged whole. Worth naming why it was there: the ABC declaration came from Opus's H14 finding, which was about the verdict; I widened the contract with the reason string alongside it and never went back to ask whether that half had a reader.

Removed at all six sites: the ABC property (providers/base.py), both forwarders (providers/acp.py, acp/session_provider.py), both client fields (acp/client.py, acp/session_handle.py), and the log interpolation. compaction_failure_verdict goes with them — the four arming sites now call compaction_failure_is_transient directly, which also drops the tuple-unpack. last_compaction_transient is now the whole of the new contract.

Nothing was lost. The chat row still shows the sentence, via the EVENT_COMPACTION_STATUS title that compaction_failure_detail fills; the server log still has the whole frame, from the WARNING at each producer. test_the_abc_declares_the_capability_with_a_safe_default now also asserts not hasattr(LLMProvider, "last_compaction_failure"), so re-adding it without a reader trips a test.

[iamwhatever]

• Subtraction: shrink _COMPACTION_TRANSIENT_MARKERS by the two unobserved timeout entries — rebutted

"timed out"/"timeout" markers have no observed frame behind them — the defect is throttle/500, and the only timeout in the tests is the negative conversationSummary case.

The observation is accurate. I am keeping them, on the asymmetry rather than on the evidence.

A summarization call that times out is the same class of fault as one that is throttled — the work is fine, the call did not land, and the next attempt clears it. Classifying it permanent costs a dropped user turn, which is precisely the defect this PR exists to fix. Keeping the marker costs at worst two bounded, billed retries on a permanent fault that happens to be labelled "timeout" in a reason-bearing key — and a context overflow is not labelled that way, so that path is thin.

The prose-collision worry that would otherwise justify caution here is also already closed, as of ec3c02e7: the scan is scoped to _COMPACTION_DETAIL_KEYS, so "timeout" is only read from a field whose job is to name the fault, never from conversationSummary. Removing the markers now would narrow behaviour against a risk the previous round eliminated.

On the related Watch — the third transient vocabulary — you are right and I cannot close it here: _COMPACTION_TRANSIENT_MARKERS, llm_helpers._TRANSIENT_MARKERS and _is_transient_raw_error's regexes will diverge. Unifying them touches every transient path in the codebase, well outside this PR; it is recorded in #8256 alongside the Slack/messaging work, where all three lists have to be reasoned about anyway.

[iamwhatever]

• span=19934bb5ba3d — fixed in 179b60b90d0fd15b3a2e83cb09a99405a60482a3

Raw KAS frame exposes credentials in gateway logs. logger.warning("KAS summarization failed — raw frame: %s", kiro) — Credential in backend-echoed content -> summarization failure -> raw frame persisted in logs -> credential exposure. Fix: Log redact_text(str(kiro)) instead.

Legitimate, and the inconsistency was mine and local: the frame carries conversationSummary, and the two lines below this one already run that same field through redact_text before it reaches the dashboard. Logging the whole frame raw put the unredacted version somewhere more durable than the surface I was careful about — and the line exists to make failures diagnosable, so it would have been the exposure introduced by the fix for the diagnosability gap.

Taken exactly as recommended: redact_text(str(kiro)). Pinned by TestKasSummarizationFailureLogging::test_the_frame_is_redacted_before_it_reaches_the_log, which puts an aws_secret_access_key=... in conversationSummary, asserts the token is absent from caplog.text while the KAS summarization failed record is still emitted, and also asserts the verdict still lands — redacting the log must not cost the classification.

[iamwhatever]

• span=db37e66c9e96 — fixed in 179b60b90d0fd15b3a2e83cb09a99405a60482a3

Compaction retry overrides intervening user intent. Queued correction or completed Stop -> original message prepended at queue index 0 -> superseded action executes first. Fix: Require no Stop-generation change, active Stop, queued user follow-up, or pending steer before requeueing.

Legitimate, and the sharper of the two framings this hazard has had. Opus raised a neighbouring candidate last round and dropped it for lacking a groundable trigger; naming the ordering consequence is what makes it concrete — the requeue inserts at index 0, so a correction the user typed while the turn was failing would run after the message it was meant to replace, and a message they stopped would come back at all.

Taken as the four-part guard you specified, reusing the promise-only continuation's own precondition set rather than inventing a parallel one:

and not _should_suppress_requeue(slot)          # a stop is live
and not slot._stopping
and getattr(slot, "_stop_generation", _stop_gen_turn_start) == _stop_gen_turn_start
and not bool(getattr(slot, "_pending_steers", None))
and not _has_user_queued_followup(slot)

The generation comparison is the load-bearing one: a stop that completed during the turn leaves _stopping and _stop_state both snapped back to idle, so only the monotonic counter still records it — the same reason that check exists at the promise-only site.

Two tests, both mutation-verified (each fails with the guard deleted):

• test_a_stopped_turn_is_not_revived_by_the_throttle_requeue — the stop lands during the turn, which is where it has to land for the snapshot comparison to be the thing under test.
• test_a_user_followup_outranks_the_throttle_requeue — a user-authored queue entry, asserting the budget is not spent and no retry notice is appended.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is OVERLAPPING with PR #6307. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8215: KEEP. Highest file overlap of any open PR (8 shared files) and the same ABC-extension seam, but entirely different capabilities; the only interaction is ordinary co-editing of one class. Files: src/kiro_crew/providers/base.py.
• This PR is OVERLAPPING with PR #7396. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8215: KEEP. Complementary verdicts for different faults; neither covers the other. The shared insertion window in client.py plus the shared session_provider.py touch means the later of the two may need a trivial textual rebase, not a design change. Files: src/kiro_crew/acp/client.py.
• This PR is OVERLAPPING with PR #8288. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8215: KEEP. Two different backend faults on the same recovery ladder; neither implements the other's behaviour and each is useless for the other's trigger. 8215's timeline carries a cross-reference to 8288, so the authors are aware. Worth one review note on landing order: 8288's arm gates on Stop, stop-generation and queued user input and registers its continuation as purgeable, and 8215's arm deliberately does not (Opus rebutted that finding by pointing at the pipe-death sibling, GPT still holds it blocking). Files: src/kiro_crew/dashboard/chat_runner.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.