Skip to content

fix(chat): flag false in-progress replies after a turn ends - #6850

Open
rubencu wants to merge 2 commits into
kirodotdev:mainfrom
rubencu:fix/no-false-continuation
Open

fix(chat): flag false in-progress replies after a turn ends#6850
rubencu wants to merge 2 commits into
kirodotdev:mainfrom
rubencu:fix/no-false-continuation

Conversation

@rubencu

@rubencu rubencu commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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)

  • Added a narrow, model-agnostic detector for sentence-initial first-person foreground-work claims (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.
  • Added a normal-completion, top-level notice arm after the existing promise-only recovery. It preserves every completed tool result, queues no continuation, and states the actual lifecycle: the main-agent turn ended; separately shown subagents or monitor loops may continue; otherwise a new message is required.
  • Kept the existing stop, queued-follow-up, pending-steer, refusal, and orchestrator-stage guards so the notice cannot override newer user/system intent.
  • Scoped sentence extraction to a named regex group so punctuation consumed by the boundary cannot bypass content-delivery exclusions; newline is also a valid boundary.
  • Documented the notice-only mixed-turn contract beside the existing automatic-recovery behavior.

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.
  • Manual mutation proof: restoring both production files to origin/main while retaining the tests produces two assertion failures (not collection errors).
  • Focused Python suite: 27 passed; isort, flake8, Black scope gate, and mypy passed.
  • Canonical local gates 3–35 passed: subprocess/lockdown/security checks, full isort/flake8/mypy, harness parity, docs, vendor/scrub/CFN checks, production frontend build, TypeScript, ESLint, i18n check/render, scoped frontend tests, duplication, bundle-size analysis, and Electron under CI’s Node 24 runtime (1,422 passed, 0 failed, 1 skipped).
  • The full backend run reached 74,641 passes but cannot be fully green under this dashboard sandbox: pytest’s required scratch root is inside the live data-home path, /home canonicalizes to a /local/home mount owned by uid 65534, and AF_UNIX fixtures exceed Linux’s path limit. Four representative failures reproduce unchanged on pristine origin/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:

Turn ended after 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

  • 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

@rubencu
rubencu requested a review from a team as a code owner August 29, 2026 21:12
@rubencu
rubencu requested a review from iamwhatever August 29, 2026 21:12
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rubencu

rubencu commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author
  • Sentence-boundary exclusion bypass — fixed. The detector now captures a named claim group and slices from match.start("claim"), so punctuation consumed by the boundary cannot produce an empty clause. The exact Done. I'm continuing with the explanation: here is the answer. regression and a leading-sentence true-positive both pass on b7a74f6023c7.

@rubencu

rubencu commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author
  • Newline boundary miss — fixed. (?:^|[.!?\n]\s*) now matches a first-person claim after an unpunctuated line, with Here's the plan\nI'm continuing with the build now. as a regression on b7a74f6023c7.
  • Exclude every claim followed by later prose — rebutted as disproportionate. That rule would suppress the reported shape itself: the false progress claim is followed by legitimate screenshot commentary. The branch is notice-only, never queues recovery or replays a prior tool, and its operative lifecycle sentence remains true. The focused Opus verifier accepted this trade and found no blocker on b7a74f6023c7.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
@rubencu

rubencu commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

CI triage: run 33275489470 was green everywhere except Backend Tests (3.10, 2), which lost xdist worker gw1, recovered, reached 99%, and then hit the 30-minute job cancellation before pytest could print a summary. The corresponding shard on current main (run 33275299849, job 99162286710) passed in 12 minutes. Coverage Gate failed only because backend-test=cancelled; it reported no independent coverage deficit. My contributor token cannot rerun upstream jobs (Must have admin rights). Could a maintainer rerun the failed/cancelled jobs for this SHA?

@bolichen97

Copy link
Copy Markdown
Collaborator

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

  • PR #7626 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7626: REBASE. Genuinely complementary detectors for two different non-answer text shapes; no code collision, only a consistency concern. Files: src/kiro_crew/dashboard/chat_runner.py.
  • PR #8288 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8288: KEEP. Complementary halves of the same lifecycle-honesty problem: 6850 diagnoses false model progress prose without replaying, 8288 recovers a provider control sentence by continuing. Neither subsumes the other, and 8288's own Description already names 6850 as related-but-independent. Merge order only decides which one resolves a trivial import-block and chat_utils placement conflict. Files: src/kiro_crew/dashboard/chat_utils.py, 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.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 2026
@chenmingwei23 chenmingwei23 added needs-pr-triage PR scanner: awaiting automated triage drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 7, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

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.

@bolichen97

Copy link
Copy Markdown
Collaborator

@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: src/kiro_crew/dashboard/chat_utils.py, src/kiro_crew/dashboard/chat_runner.py and test/test_dashboard_chat.py. It adds is_upstream_turn_failure_text / contains_upstream_turn_failure_text plus an _upstream_turn_failed branch in _run_chat that purges chunks and queues one recovery continuation. Yours adds has_unfinished_progress_claim plus a notice-only arm that queues nothing.

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 chat_utils import block in chat_runner.py, and classifier placement in the same detector region of chat_utils.py.

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 should_notice_* predicates in chat_utils.py, with two leaked-tool-call arms now ahead of the promise arms, so express your guard as such a predicate and re-decide arm order instead of keeping the inline condition. Also expect a real conflict with #7626, which rewrites the same completion-path hunks.

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.
@bolichen97
bolichen97 force-pushed the fix/no-false-continuation branch from b7a74f6 to 2472383 Compare September 8, 2026 17:23
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 8534cbf7 by a maintainer as part of the 2026-09-08 open-PR audit (was 1417 commits behind, mergeable_state: dirty).

Conflicts and resolution:

  • src/kiro_crew/dashboard/chat_runner.py — one conflict, in the chat_utils import block only: kept main's classify_empty_turn, kept your has_unfinished_progress_claim, dropped has_leaked_tool_call (main no longer imports it in the runner and nothing here calls it).
  • Everything else auto-merged. Your new arm still sits last in the turn-completion chain, after main's should_notice_leaked_tool_call / should_notice_mixed_turn_leak arms, so those keep precedence. No behaviour changed; diff is still +161/-0 across 6 files.

Gates run locally on changed files: black, isort, flake8, test/test_promise_only_recovery.py (26 passed), TestRunChatSegmentFlush (8 passed).

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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

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

The base tree confirms temp-screenshots/ is an established convention (.gitignore explicitly calls it a deliverable directory), "Stop hook" is a real dashboard concept, and the new elif arm slots coherently after the existing zero-tool promise arms — mixed turns fall through the earlier _turn_tool_calls == 0 gates into the new notice-only arm, so no double-fire and no replay risk. The detector reuses the existing terminal-sentence/reject-bias design, the doc update lands in the same commit beside the recovery-card contract it extends, and every description claim has backing code.

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 3dff8f1

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 3dff8f1759000478325792abe24876ef9c99a56f 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.

Verification complete. The Stop-hook premise in the notice text is real (chat_runner.py:11405-11487), the new detector is not a second spelling of _PROMISE_NOW_RE (which requires an immediacy marker the progressive claims lack), the existing recovery arms are genuinely zero-tool-gated (turn_tool_calls == 0 at chat_runner.py:11215, should_recover_promise_only), the committed screenshot follows the repo's sanctioned temp-screenshots/ deliverables convention (.gitignore:97, ~355 existing dirs), and defect provenance is satisfied by the reproduction in the description plus the on-base-failing event-stream test.

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 ships

Intent: stop the last chat bubble claiming work is still running after the turn has gone idle — a FIX (notice-only).

Inventory (5 items)
  1. Tool-running turns ending "I'm continuing…" now get an info notice that the turn ended — justified
  2. Tool-running turns ending with an "I'll do it now" promise now get the same notice — justified
  3. Zero-tool turns ending in present-progressive claims (outside the replay detector's phrasing) also get it — justified
  4. Spec paragraph documenting the notice-only mixed-turn contract — justified
  5. One dark-mode verification screenshot committed under temp-screenshots/false-progress/ — justified

[FIRST-PRINCIPLES-REVIEWED] 3dff8f1

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/dashboard/chat_utils.py:1787 -- "excludes truthful reports about a subagent or monitor" contradicts the regex, which matches “I'm running the monitor in the background” -> Fix: limit the claim to third-person background reports and align the added spec text.
[GPT-REVIEWED] 3dff8f1

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 8, 2026
auto-merge was automatically disabled September 8, 2026 17:43

Head branch was pushed to by a user without write access

@rubencu

rubencu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

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.

@rubencu

rubencu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Fixed. has_unfinished_progress_claim now extracts _terminal_sentence(text) before matching. I am running checks. They passed. therefore lands on They passed and returns false, while Setup is done. I am running checks. still detects the terminal claim. The new negative fails against the prior implementation.

@rubencu

rubencu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebutted. temp-screenshots/README.md defines this directory as committed PR review evidence, states that authors should not delete files before merge and reviewers should not request deletion, and delegates removal exclusively to the scheduled 14-day cleanup workflow. The image is linked from the PR body with the new exact-head SHA and is the visual evidence for this user-visible notice. No product package imports it.

@rubencu

rubencu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Accepted and fixed. _PROGRESS_DELIVERED_CONTENT_RE and its branch were removed. Terminal-sentence scoping plus the existing colon-with-content exclusion now provide the smaller contract, with positive and negative regressions covering both sides.

@rubencu

rubencu commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Head 3dff8f1759000478325792abe24876ef9c99a56f fast-forwards the maintainer-rebased commit with the two GPT fixes and the accepted First-Principles subtraction. Focused suite: 27 passed; both new regressions fail against the prior production code; Black, isort, flake8, and mypy pass. The current pull_request workflows are all action_required. Please approve the latest workflow runs when convenient; I will not self-approve fork workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drive-to-green PR claimed by drive-to-green pipeline fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants