fix(chat): recover synthetic backend retry turns - #8288
Conversation
57ddd15 to
8f697aa
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound downstream mitigation, but it hard-couples control flow to an unversioned English sentence owned by kiro-cli — a silent-decay dependency with no upstream fix tracked. Watch
Suggestions
[DESIGN-REVIEWED] 07ff841 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All verification is done. Here is the review. First-Principles-Verdict: CONCERNS The fix is real and reuses the existing continuation machinery, but sentinel detection lands in only one of three documented parity copies of this ladder. What this change shipsIntent: stop treating Kiro CLI's "I hit an issue… Please retry." success-wrapped failure as a real answer, and resume the live session safely. This is a FIX (#8285).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 07ff841 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed Adjudication emitted 0 well-formed verdict line(s) for total=1, so GPT's verdict stands. BLOCKING -- src/kiro_crew/dashboard/chat_runner.py:9644 -- Model text can forge the backend-failure control signal
Adjudication (Opus 4.8) — is blocking on each finding proportionate?I've verified the mechanism against the base code and the diff. F1 — The forged sentinel (exact-match Harm rung: LOW/MEDIUM — a bounded, one-shot, visible extra continuation an attacker controlling tool output could already influence within the turn; no privilege escalation, no ceiling bypass, no credential exposure, self-correcting. Remedy proposed ("require an out-of-band Kiro failure event") is infeasible without changing the external kiro-cli/ACP protocol — the very reason the text-inference exists is that kiro-cli closes a failed inner loop as an ordinary Ok/UserTurnEnd with no out-of-band event — so the real fix is a cross-boundary protocol change or abandoning the feature. That cost clearly exceeds the bounded harm. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe candidate's core harm — that a nested (depth>0) turn discards the sentinel and broadcasts a blank assistant row that is never reconciled ("a nested turn may not emit that terminal refresh, leaving a stray empty bubble") — is falsified. The depth-1 No new grounded defect surfaced under Step 2. No findings. [OPUS-REVIEWED] 07ff841 |
8f697aa to
a4d87da
Compare
Fixed in |
a4d87da to
a2affcc
Compare
Fixed in |
a2affcc to
9299643
Compare
9299643 to
b5162c5
Compare
That discriminator is not valid for Kiro CLI. The captured real failure session records credit-only accounting and zero token fields, but legitimate Kiro model text has the same zero-token shape; requiring zero therefore does not distinguish provider control output from model output and would only appear to close the ambiguity. The implemented behavioral gate instead requires positive Kiro-backend identity, |
Fixed in |
b5162c5 to
3db8995
Compare
Fixed in |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Recognize Kiro CLI's successful-looking turn-failure sentinel, preserve completed tool work, and continue once without recording a false success.
3db8995 to
07ff841
Compare
No such event exists in the observed Kiro CLI/ACP failure shape: the real incidents persist |
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days. I reviewed the blockers but they require input beyond what automation can decide:
When these are addressed, the pipeline will re-assess on its next cycle. |
|
Hi @Pearcekieser — from a repo-wide audit of open PRs, this one overlaps #9156 (@buluoray), and the two need one decision before either lands. The shared ground is They are not redundant. #9156's Concrete suggestion: split the Separately, this PR is 497 commits behind and merges dirty. #8318 landed Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Problem / Motivation
Kiro CLI can fail its inner agent loop after several successful tool cycles yet close ACP as a normal
Ok/UserTurnEndresult whose only assistant text is:Kiro Crew treated that provider-generated sentence as a real answer. It persisted the text, recorded success, reset recovery budgets, and required the user to repeat a request whose tools may already have produced side effects.
Why it matters
Long-running work can stop after minutes of successful execution with no actionable diagnosis. Blindly replaying the original request is unsafe because completed tools may run twice. The recovery must continue from the live session context, preserve tool results, and remain cancellable before dispatch.
What changed (motivation → approach → change)
please retrymatching is deliberately avoided.is_kiro_backendidentity, at least one tool call in the turn, and confirmation that the current user message did not itself contain the sentence before treating it as provider control output. Exact answers without tool activity, quoted/requested literals, and non-Kiro output remain ordinary assistant text.Tests
TestEmptyResponseRetry: 19 passed.TestRunChatTransientRetry: 20 passed.test_dashboard_chat.py: 759 passed on the final rebased head.The local full-backend profile run reached 84,065 passes. Its remaining failures are unrelated host-topology failures caused by the dashboard sandbox exposing the live
KIROCREW_HOME,/local/homeownership, and overlong AF_UNIX scratch paths. The two failures related to this diff's answer-branch source contract were fixed and rerun green. Hosted CI provides the authoritative isolated full-backend result.Two independent local reviews found missing dispatch-time cancellation and missing positive Kiro identity; both were fixed. A GPT round then showed that output-token counts are non-authoritative for Kiro's credit-only accounting, so that discriminator was removed in favor of backend identity, tool activity, exact-text, and current-request guards. Later GPT rounds identified the live-browser reconciliation gap and then exposed a broader cross-turn file-change attachment bug. The former now replaces an already-streamed sentinel row before recovery; the latter was restructured globally so file changes scan only assistant rows created after the turn boundary and synthesize a current-turn anchor when needed. On the final current head, GPT restated the inherent text-signal ambiguity as a security blocker, but Opus adjudication downgraded it as disproportionate: the continuation is visible, same-session, one-shot, admission-restamped, and subject to the same tool gates, while Kiro CLI exposes no out-of-band failure event to consume.
Manual verification
N/A — this is backend recovery control flow with deterministic event-stream and queue-dispatch coverage. The triggering upstream failure cannot be induced on demand; the test event shape comes from three observed raw Kiro CLI records.
Related Issues
Fixes #8285
Related but independent:
Pattern harvest
Rule candidate: review-prompt
Pattern: a provider-owned synthetic failure sentence returned under a success envelope must require positive provider identity plus out-of-band failure evidence before changing control flow.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)CI retry note
The first two dependency-audit attempts for current-head validation timed out after 120 seconds while waiting for
npm audit; neither produced a vulnerability finding. This description update retriggers the repository'spull_request: editedreview/audit path without changing the reviewed commit.