fix(chat): flag false in-progress replies after a turn ends - #6850
fix(chat): flag false in-progress replies after a turn ends#6850rubencu wants to merge 2 commits into
Conversation
|
|
|
CI triage: run 33275489470 was green everywhere except |
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. |
|
Kiro Crew [operator: chenmingwei23]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable -- I'll push fixes directly to this branch as a co-author. Assessment: Merge conflict with main plus a Backend Tests (3.10, 2) shard that lost an xdist worker and hit the 30-min job timeout before printing a summary (the same shard passes on main in ~12 min); Coverage Gate red only cascades from that cancelled job. The GPT/Opus review points are already dispositioned by you. Plan: rebase onto main to clear the conflict, then re-run the timed-out shard. If you'd prefer I don't touch this PR, add the pr-no-autofix label. |
|
@rubencu Thanks for this, it still addresses something main does not cover, so I am not asking you to close it. One coordination note before it lands. #8288 by @Pearcekieser touches the same three files: These are complementary halves of one problem: a turn that ends looking normal while the work did not finish. #8288 detects the provider's own fixed failure sentence and recovers on the same session. Yours detects the model's first-person progress prose and deliberately only informs, because replaying a mixed turn can duplicate a side effect. Neither subsumes the other and no logic collides. The collisions are textual only: the shared So no split and no absorption is needed. Since this PR needs a rebase regardless, my suggestion is to land #8288 first, then rebase this one on top and resolve the import block. On that rebase, please note that merged #8352 moved these decisions into named Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Flag a completed top-level turn when its final assistant segment says foreground work is still running. Keep prior tool results landed, never replay earlier side effects, and surface the true idle lifecycle with regression and visual evidence.
b7a74f6 to
2472383
Compare
|
Rebased onto main Conflicts and resolution:
Gates run locally on changed files: black, isort, flake8, Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong. |
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The base tree confirms Design-Verdict: PASS Notice-only diagnosis is the right shape: replaying a mixed turn is unsafe, so surfacing the true lifecycle without queuing a continuation is proportionate and reversible. [DESIGN-REVIEWED] 3dff8f1 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
First Principles Review (Fable 5, fork) — ✅ PASSPremise-level review of Verification complete. The Stop-hook premise in the notice text is real ( First-Principles-Verdict: PASS Verify the detector's extra verbs (running, checking, testing…) on real transcripts — only "continuing with…" was reported, and the notice has no per-slot repeat budget. What this change shipsIntent: stop the last chat bubble claiming work is still running after the turn has gone idle — a FIX (notice-only). Inventory (5 items)
[FIRST-PRINCIPLES-REVIEWED] 3dff8f1 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsFINDING -- src/kiro_crew/dashboard/chat_utils.py:1787 -- |
Head branch was pushed to by a user without write access
|
Fixed. The notice now says that no further main-agent steps run unless a Stop hook explicitly requests a bounded continuation. The real mixed-turn regression asserts that exception is present. Mutation proof: the assertion fails against the prior production wording. |
|
Fixed. |
|
Rebutted. |
|
Accepted and fixed. |
|
Head |
Problem / Motivation
A dashboard turn can execute many tools and then end with a first-person progress claim such as “I’m continuing with the full local gate run.” Once the turn has returned control, no further main-agent work is running, but the terminal assistant bubble still reads as if work continues invisibly.
The existing unfinished-action guard catches only zero-tool turns with narrow “I’ll do it now” phrasing. It intentionally excludes any turn that already ran a tool, because automatically replaying a mixed turn could duplicate a push, deployment, message, or other side effect. That safety exclusion also suppressed every visible warning for the reported long-turn shape.
Why it matters
The transcript says work is ongoing while the slot is actually idle. Users wait for progress that cannot arrive, and the mismatch is especially confusing when no subagent or monitor is shown. Model phrasing and tool-call count make the symptom intermittent, but the completion contract is provider-independent.
What changed (motivation → approach → change)
I’m continuing with…,I’m still working on…,Next, I’m running…). It excludes third-person background status and common in-message content delivery such as an explanation introduced by a colon.Tests
test/test_promise_only_recovery.py: exact reported wording; curly/straight apostrophes; ongoing-work variants; completed/background/explanation negatives; leading-sentence and newline boundaries; mutation-proof call-time resolution.test/test_dashboard_chat.py::TestRunChatSegmentFlush::test_tool_turn_progress_claim_surfaces_idle_notice: real event stream (text → write tool → progress claim → end_turn) proves the assistant text and earlier tool work land, the notice appears, and no recovery is queued.origin/mainwhile retaining the tests produces two assertion failures (not collection errors)./homecanonicalizes to a/local/homemount owned by uid 65534, and AF_UNIX fixtures exceed Linux’s path limit. Four representative failures reproduce unchanged on pristineorigin/main; changed-path tests are green. Server CI runs under its clean hosted path topology.Manual verification
Rendered the exact new notice through the real built SPA using the repository’s transcript fixture server and Playwright. The harness asserted the full rendered text before capturing, and the resulting NoticeCard uses the existing info tone/Lucide icon with no new frontend component or style path.
Screenshots / video
The notice shown immediately after a terminal false-progress claim:
Pattern harvest
Rule candidate: shared terminal-turn diagnostic helper and regression template.
Pattern: lifecycle notices that classify unfinished foreground work must evaluate only the terminal sentence, enumerate framework-owned continuation paths such as Stop hooks, and remain notice-only after any tool side effect.
Related Issues
no linked issue: this dashboard-reported regression is related to the behavior introduced for #2686, which is already closed.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)