Skip to content

fix(agents): use currentAttemptAssistant for incomplete-turn classification#94637

Closed
LiuwqGit wants to merge 2 commits into
openclaw:mainfrom
LiuwqGit:fix/issue-80918-stale-last-assistant
Closed

fix(agents): use currentAttemptAssistant for incomplete-turn classification#94637
LiuwqGit wants to merge 2 commits into
openclaw:mainfrom
LiuwqGit:fix/issue-80918-stale-last-assistant

Conversation

@LiuwqGit

@LiuwqGit LiuwqGit commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Behavior addressed: When the agent's last tool call is update_plan (stopReason=toolUse) and the model then produces a final plain-text turn with stopReason=stop, resolveIncompleteTurnPayloadText incorrectly classifies the turn as incomplete because it checks lastAssistant.stopReason (stale snapshot) instead of the fresher currentAttemptAssistant.
  • Why this matters now: This is a silent message-loss bug — users receive "⚠️ Agent couldn't generate a response" instead of the actual final answer. Any agent that follows checklist discipline (update_plan closure) triggers this on every run.
  • Intended outcome: The final visible text is delivered to the user instead of being replaced with an incomplete-turn error.
  • Out of scope: No changes to payload delivery logic, isDeliverablePayload, assistantTexts incomplete branch, replayMetadata, or livenessState.

Linked context

Closes #80918

Related #76477 (original incomplete-turn safety contract — preserved)

Real behavior proof

  • Behavior or issue addressed: stale lastAssistant false-positive after update_plan → final stop turn

  • Real environment tested: Node.js v24.16.0 on Windows 11 x64, local checkout of openclaw/openclaw (commit 3fb6465) on branch fix/issue-80918-stale-last-assistant. The proof script proof-80918-before-after.mjs simulates both the BEFORE (stale lastAssistant) and AFTER (resolved currentAttemptAssistant) classification paths with a fixture matching the reporter's trace from issue Silent send miss: incomplete-turn classifier discards stopReason=stop final after update_plan #80918.

  • Exact steps or command run after this patch:

    node proof-80918-before-after.mjs

  • Evidence (terminal capture):

    === PR #94637 Before/After Proof (#80918) ===
Node.js: v24.16.0
Platform: win32 x64
Date: 2026-06-24T16:31:40.584Z

Fixture (reporter's trace from issue #80918):
  lastAssistant.stopReason            = "toolUse"
  currentAttemptAssistant.stopReason  = "stop"
  currentAttemptAssistant.content     = [{"type":"text","text":"Compression complete, verified. Here is the summary:\n done."}]

BEFORE fix (classification uses stale lastAssistant):
  result: "⚠️ Agent couldn't generate a response. Please try again."  ← BUG: warning replaces real answer!

AFTER fix (classification uses currentAttemptAssistant ?? lastAssistant):
  result: null  ← correct: no warning

✓ PROOF PASSED

  • Observed result after fix: With stale lastAssistant.stopReason="toolUse" and fresh currentAttemptAssistant.stopReason="stop", the function's assistant-resolution now selects currentAttemptAssistant, so the downstream incompleteTerminalAssistant check sees the fresh stopReason="stop" + visible text. The function returns null and the real final text passes through to payload delivery.

  • Before evidence (same fixture, same script): The BEFORE simulation passes params.attempt.lastAssistant (stale — stopReason="toolUse") into isIncompleteTerminalAssistantTurn, which returns true and causes the function to return the warning payload. This is the exact wire-visible failure mode reported in Silent send miss: incomplete-turn classifier discards stopReason=stop final after update_plan #80918.

  • What was not tested: No live WhatsApp / OpenRouter reproduction. The reporter's production trace from the issue covers the live channel symptom; this PR covers the source-level misclassification and regression tests.

  • Proof limitations: L2 vitest + L2 runtime fixture. Live channel reproduction requires a real OpenClaw setup with a configured agent and update_plan tool that exercises the exact trace shape.

  • Reproduction command for maintainers:

    cd work/openclaw
node proof-80918-before-after.mjs
node --import tsx scripts/run-vitest.mjs src/agents/embedded-agent-runner/run.incomplete-turn.test.ts

Tests and validation

  • node proof-80918-before-after.mjs — exit 0, BEFORE returns the warning, AFTER returns null
  • All 228 tests pass (2 test files, 228 tests, 0 failures) — includes all existing incomplete-turn tests plus 3 new ones
  • 3 new tests added:
    1. uses currentAttemptAssistant over stale lastAssistant for incomplete-turn classification (#80918) — unit test for isIncompleteTerminalAssistantTurn
    2. does not flag stale lastAssistant=toolUse when currentAttemptAssistant=stop exists (#80918) — integration test for resolveIncompleteTurnPayloadText
    3. still flags incomplete-turn when currentAttemptAssistant is absent and lastAssistant=toolUse (#76477 regression) — confirms real incomplete-turn still fires
  • All 6 existing #76477 regression tests still pass (unchanged behavior for legitimate incomplete-turn scenarios)

Risk checklist

  • Did user-visible behavior change? Yes — fixes a silent message-loss bug. Users will now receive the final assistant text instead of an error payload.
  • Did config, environment, or migration behavior change? No
  • Did security, auth, secrets, network, or tool execution behavior change? No
  • Highest-risk area: The isIncompleteTerminalAssistantTurn call in resolveIncompleteTurnPayloadText now uses currentAttemptAssistant ?? lastAssistant instead of raw lastAssistant. If currentAttemptAssistant exists but has an unexpected stopReason, classification could change. Mitigated by: (a) the regression test for currentAttemptAssistant=undefined still flags incomplete-turn, (b) the existing toolUseTerminal early guard on line 282 still uses the stale lastAssistant to prevent pre-tool text from suppressing the check, (c) all 228 existing tests pass.

Current review state

@LiuwqGit

LiuwqGit commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Clean PR rebased on latest main (81eaa88). Changes:

  • src/agents/embedded-agent-runner/run/incomplete-turn.ts:288: 1-line fix — uses resolved assistant (prefers currentAttemptAssistant) instead of stale lastAssistant
  • 3 new tests: stale lastAssistant false-positive fix + #76477 regression

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI. labels Jun 18, 2026
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 10:39 PM ET / 02:39 UTC.

Summary
The PR changes the embedded-agent incomplete-turn classifier to use the resolved current-attempt assistant for the final incomplete-terminal check and adds stale-assistant regression coverage.

PR surface: Source 0, Tests +71. Total +71 across 2 files.

Reproducibility: yes. at source level: current main resolves a fresh current-attempt assistant but still passes stale lastAssistant into the final classifier call. I did not run the live WhatsApp/OpenRouter reproduction in this read-only review.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #80918
Summary: This PR is the current open candidate fix for the stale lastAssistant incomplete-turn classifier issue; older PRs are closed predecessors or broader overlaps.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] The change sits on the final agent message-delivery classification path; a wrong classifier choice can decide whether final assistant text is delivered normally or replaced by an incomplete-turn warning, and the available proof is source-level rather than a live WhatsApp/OpenRouter run.

Maintainer options:

  1. Accept the source-level proof (recommended)
    Merge after normal checks if maintainers are comfortable that the fixture covers the classifier boundary without live transport proof.
  2. Ask for a live channel run
    Require a redacted live WhatsApp/OpenRouter or equivalent channel transcript/log showing the final post-update_plan answer is delivered before merge.

Next step before merge

  • [P2] No repair lane is needed; the branch already contains the focused fix and the remaining step is maintainer proof acceptance plus normal merge gates.

Maintainer decision needed

  • Question: Is source-level terminal proof plus regression coverage sufficient to merge this classifier-only fix, or should the PR wait for live WhatsApp/OpenRouter transport proof?
  • Rationale: The patch is narrow and source-backed, but it changes the terminal message-delivery classifier and the contributor did not run the full live channel scenario from the report.
  • Likely owner: steipete — Recent terminal-classifier simplification and merge history make this the best routing candidate for the proof-bar decision.
  • Options:
    • Accept source-level proof (recommended): Merge after normal gates if maintainers accept the before/after fixture and regression tests as sufficient for this classifier-only delivery-path fix.
    • Require live transport proof: Pause until someone proves a real configured channel run with update_plan followed by a stopReason=stop final answer delivers the final text.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes one TypeScript runtime classifier argument and focused regression tests only.

Review details

Best possible solution:

Land this narrow classifier fix after maintainer acceptance and normal gates, keeping broader delivery recovery and observability follow-up separate.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main resolves a fresh current-attempt assistant but still passes stale lastAssistant into the final classifier call. I did not run the live WhatsApp/OpenRouter reproduction in this read-only review.

Is this the best way to solve the issue?

Yes. This is the narrowest maintainable fix because it changes only the final classifier input while preserving the earlier toolUseTerminal guard for the existing incomplete-turn safety contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 58bbf5a117eb.

Label changes

Label justifications:

  • P2: The PR addresses a bounded embedded-agent bug that can suppress or replace a final user-visible answer, but it is not a core runtime outage.
  • merge-risk: 🚨 message-delivery: The changed classifier decides whether terminal assistant text continues to normal delivery or becomes an incomplete-turn warning.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal before/after output from a local Node proof plus regression-test output for the stale-assistant fixture; it does not include a live WhatsApp/OpenRouter run.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal before/after output from a local Node proof plus regression-test output for the stale-assistant fixture; it does not include a live WhatsApp/OpenRouter run.
Evidence reviewed

PR surface:

Source 0, Tests +71. Total +71 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 1 1 0
Tests 1 71 0 +71
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 72 1 +71

What I checked:

Likely related people:

  • steipete: Authored and merged the recent terminal-classifier simplification touching the same incomplete-turn helper and tests. (role: recent classifier contributor and merger; confidence: medium; commits: 0314819f918a; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts)
  • fuller-stack-dev: Authored the recent empty post-tool final-turn retry PR that modified the same incomplete-turn helper and tests. (role: adjacent runtime contributor; confidence: medium; commits: 77a682c5def2; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts)
  • vincentkoc: Authored post-tool empty reply recovery work near the same terminal-classification and runtime recovery boundary. (role: recent area contributor; confidence: medium; commits: 67dc8053149d, e085fa1a3ffd; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (2 earlier review cycles)
  • reviewed 2026-06-25T01:43:18.350Z sha 70d886c :: needs maintainer review before merge. :: none
  • reviewed 2026-07-05T23:52:24.304Z sha 9bf7040 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 18, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 21, 2026
@LiuwqGit LiuwqGit closed this Jun 21, 2026
@LiuwqGit LiuwqGit reopened this Jun 21, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 24, 2026
@LiuwqGit
LiuwqGit force-pushed the fix/issue-80918-stale-last-assistant branch from 3fb6465 to 70d886c Compare June 24, 2026 23:26
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 24, 2026
@LiuwqGit

LiuwqGit commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Hi @frankekn @vincentkoc — this PR has been open for about a week. It's a narrow one-line fix for #80918 (stale lastAssistant causing silent message loss after update_plan → stop). ClawSweeper rated it 🐚 platinum hermit / proof: sufficient. Could you take a look when you have a moment?

@LiuwqGit
LiuwqGit force-pushed the fix/issue-80918-stale-last-assistant branch from 70d886c to 9bf7040 Compare July 5, 2026 23:33
@steipete steipete self-assigned this Jul 6, 2026
@steipete
steipete force-pushed the fix/issue-80918-stale-last-assistant branch from 9bf7040 to aa4b3ca Compare July 6, 2026 04:36
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for continuing to narrow this. I rechecked the exact current head, the attempt producer, its caller and return boundary, and the shipped v2026.5.7 source.

The new tests still create a state the production producer cannot emit: lastAssistant is the latest assistant in messagesSnapshot, while currentAttemptAssistant is the latest assistant in a suffix of that same snapshot. Whenever the latter exists, both selectors necessarily return the same message (src/agents/embedded-agent-runner/run/attempt.ts:5140-5147; src/agents/embedded-agent-runner/run/attempt.context-engine-helpers.ts:128-139).

The broader follow-up therefore cannot repair the reported stale-snapshot symptom, and it expands metadata and failover behavior without a producible regression. This is the same missing evidence called out when #93805 was closed. I’m closing this candidate; the next useful proof is a redacted real attempt capture containing messagesSnapshot, lastAssistant, and currentAttemptAssistant. If that shows the snapshot itself is stale relative to the terminal session event, the fix belongs at attempt finalization.

@steipete steipete closed this Jul 6, 2026
@steipete steipete mentioned this pull request Jul 6, 2026
Merged
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Follow-up: the maintainer replacement #100655 landed as c1cbc15.

The landed fix goes beyond the original classifier-only change: it carries one canonical current-attempt terminal assistant through classification, failover, payload construction, stop reason, usage, and terminal metadata, with explicit stale-session and cross-provider regression coverage. Thanks @LiuwqGit for the original report and for narrowing the failure boundary; #80918 is now closed by the landed replacement.

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

Reviewers

No reviews

Assignees

@steipete steipete

Labels

agents Agent runtime and tooling merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. triage: mock-only-proof Candidate: PR proof only shows tests, mocks, snapshots, lint, typecheck, or CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Silent send miss: incomplete-turn classifier discards stopReason=stop final after update_plan

2 participants

@LiuwqGit @steipete