Skip to content

fix(ui): keep failed agent turns visibly failed [AI-assisted]#97860

Closed
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/97849-chat-turn-outcome-boundary
Closed

fix(ui): keep failed agent turns visibly failed [AI-assisted]#97860
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/97849-chat-turn-outcome-boundary

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Closes #97849

What Problem This Solves

Fixes an issue where users reviewing cron, scheduled, or autonomous Control UI/WebChat sessions could see a genuinely failed earlier agent-initiated turn as successful when a later adjacent run produced an assistant reply.

Why This Change Was Made

The shared chat item projection now scopes tool-error success suppression to explicit run/turn metadata (runId, agentRunId, turnId, and the same keys under __openclaw). Adjacent assistant/tool display groups split when that metadata changes, while legacy transcripts without run metadata keep the existing user-boundary fallback from #90122.

User Impact

Failed autonomous turns stay visibly failed instead of silently collapsing their red tool-error banner. The prior behavior for a failed internal tool followed by a same-run assistant reply is preserved.

Evidence

Behavior addressed: a genuinely failed agent-initiated turn was marked as succeeded when a later adjacent agent turn produced an assistant reply.

Real environment tested: local OpenClaw source checkout on current main fast-forwarded to 6de357ad47, using the exported buildChatItems projection and the grouped renderer unit coverage.

Exact steps or command run after this patch:

node scripts/run-vitest.mjs ui/src/ui/chat/build-chat-items.test.ts
node scripts/run-vitest.mjs ui/src/ui/chat/grouped-render.test.ts
node scripts/run-vitest.mjs ui/src/ui/chat/build-chat-items.test.ts ui/src/ui/chat/grouped-render.test.ts
git diff --check

Evidence after fix:

  • build-chat-items.test.ts: 48 tests passed, including the new adjacent run-boundary regression.
  • grouped-render.test.ts: 65 tests passed.
  • Combined focused run: 113 tests passed.
  • git diff --check: clean.

Observed result after fix: the earlier failed run keeps turnSucceeded=false, while the later run with the assistant reply is turnSucceeded=true.

What was not tested: no browser screenshot or live scheduled job was run; this is a shared projection fix covered at the deterministic UI model and renderer boundary. Local autoreview was attempted with .agents/skills/autoreview/scripts/autoreview --mode local but the Codex app rejected it under the external-review data-export policy, so no workaround was attempted.

AI-assisted: yes. I understand the change: it keeps the #90122 same-turn suppression behavior, but prevents explicit run/turn metadata from leaking a later reply backward into an earlier failed autonomous turn.

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 12:19 PM ET / 16:19 UTC.

Summary
The branch adds run/turn metadata-based grouping and turnSucceeded annotation in buildChatItems, plus a regression test for adjacent agent-initiated run boundaries.

PR surface: Source +43, Tests +14. Total +57 across 2 files.

Reproducibility: yes. source-reproducible: current live tool messages carry runId, while terminal assistant messages are appended from payload.message without that key, so the PR's keyed-only success set can miss same-run success. I did not run tests because this review is read-only.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: vector/embedding metadata: ui/src/ui/chat/build-chat-items.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97849
Summary: This PR is the candidate fix for the open cross-turn tool-error suppression regression, while the earlier merged PR is related provenance rather than remaining canonical work.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Repair the keyed live-tool plus unkeyed assistant-final path.
  • [P1] Add focused regression coverage for that mixed-key live Control UI shape.
  • [P1] Add redacted real behavior proof after the repair; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports focused tests only; it still needs redacted after-fix Control UI/WebChat proof such as a screenshot, recording, terminal live output, or logs with private details removed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A short Control UI/WebChat visual proof would materially help after the source-level blocker is repaired. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify Control UI/WebChat keeps an earlier failed autonomous tool turn visibly failed while same-run non-terminal tool failures remain collapsed.

Risk before merge

  • [P1] Merging as-is can make keyed live tool messages ignore an unkeyed same-run assistant final, leaving a non-terminal internal tool failure visibly failed and regressing the recently merged within-turn suppression behavior.
  • [P1] Contributor proof is limited to focused projection/renderer tests and stated results; no live Control UI/WebChat capture, terminal runtime output, or redacted logs show the after-fix UI path.

Maintainer options:

  1. Repair the mixed-key live path (recommended)
    Before merge, make the same-run live tool case work when the tool message has runId but the terminal assistant message does not, and cover that shape with a focused regression test.
  2. Pause until proof is stronger
    Keep the draft open until the contributor can show repaired Control UI/WebChat behavior with redacted real behavior proof.

Next step before merge

  • [P1] Human/contributor follow-up is needed because the PR is draft, has a source-level correctness blocker, and the external real-behavior proof gate is not satisfied.

Security
Cleared: The diff is limited to UI chat projection and tests, with no dependency, workflow, credential, package, or code-execution surface changes.

Review findings

  • [P1] Preserve same-run keyed tool suppression — ui/src/ui/chat/build-chat-items.ts:413
Review details

Best possible solution:

Repair the turn-key contract so live tool messages and terminal assistant messages share the same boundary signal, cover the mixed-key regression, and then add redacted real Control UI/WebChat proof.

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

Yes, source-reproducible: current live tool messages carry runId, while terminal assistant messages are appended from payload.message without that key, so the PR's keyed-only success set can miss same-run success. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

No, not as written. The projection boundary is the right layer, but the key contract is incomplete unless terminal assistant messages are keyed too or keyed tools can still use a safe same-run fallback.

Full review comments:

  • [P1] Preserve same-run keyed tool suppression — ui/src/ui/chat/build-chat-items.ts:413
    The new keyed branch makes a tool group with runId succeed only when a later assistant group has the same messageTurnKey. In the live Control UI path, tool messages include runId, but terminal assistant messages are appended from payload.message without stamping that run id, so a failed internal tool followed by a same-run assistant final can keep turnSucceeded=false. Please either stamp the terminal assistant message with the run/turn key before projection or make this fallback handle keyed tools when no keyed assistant final exists, with regression coverage for that mixed-key shape.
    Confidence: 0.87

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6de357ad4772.

Label changes

Label justifications:

  • P2: The PR addresses a contained but user-visible chat transcript correctness regression in the Control UI/WebChat surface.
  • merge-risk: 🚨 session-state: The diff changes how visible transcript turn outcome state is associated with run/turn metadata and can misclassify same-run live tool results.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports focused tests only; it still needs redacted after-fix Control UI/WebChat proof such as a screenshot, recording, terminal live output, or logs with private details removed. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +43, Tests +14. Total +57 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 52 9 +43
Tests 1 15 1 +14
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 67 10 +57

What I checked:

  • PR keyed success path: PR head sets a keyed tool group succeeded only when successfulTurnKeys contains the same groupTurnKey, so a keyed tool cannot use the legacy same-turn fallback if the later assistant final is unkeyed. (ui/src/ui/chat/build-chat-items.ts:413, 4f8ab4412654)
  • Live tool messages are keyed: Current main's live tool-stream projection includes top-level runId on the tool-stream message that the PR's messageTurnKey would treat as explicit turn metadata. (ui/src/ui/app-tool-stream.ts:385, 6de357ad4772)
  • Final assistant payload message is unkeyed: Current main sends final assistant payload.message with role/content/timestamp, while runId stays on the surrounding payload rather than inside the message object consumed by buildChatItems. (src/gateway/server-chat.ts:944, 6de357ad4772)
  • UI appends final message as-is: The Control UI normalizes payload.message and appends that message object directly, so the surrounding payload runId is not stamped onto the terminal assistant message before projection. (ui/src/ui/controllers/chat.ts:1368, 6de357ad4772)
  • Regression coverage gap: The PR adds a keyed-tool plus keyed-assistant adjacent-run test, but does not cover the live mixed-key shape of a keyed tool message followed by an unkeyed terminal assistant payload. (ui/src/ui/chat/build-chat-items.test.ts:1202, 4f8ab4412654)
  • Regression provenance: Blame ties the current turnSucceeded annotation and within-turn suppression behavior to the merged related UI fix, which this PR is trying to refine. (ui/src/ui/chat/build-chat-items.ts:341, 355c43fe0c9e)

Likely related people:

  • harjothkhara: Blame and PR metadata tie the existing turnSucceeded annotation to the merged related UI fix in commit 355c43f. (role: introduced current annotation behavior; confidence: high; commits: 355c43fe0c9e; files: ui/src/ui/chat/build-chat-items.ts, ui/src/ui/chat/grouped-render.ts, ui/src/ui/types/chat-types.ts)
  • altaywtf: GitHub metadata for the merged related fix records this user as the squash merger of the commit that introduced the annotation behavior. (role: merger of related behavior; confidence: medium; commits: 355c43fe0c9e; files: ui/src/ui/chat/build-chat-items.ts, ui/src/ui/chat/grouped-render.ts)
  • programmingWTF: Recent history on the same grouped-render area includes Dashboard webchat interaction work shortly before this PR. (role: recent adjacent contributor; confidence: low; commits: 05cf49776766; files: ui/src/ui/chat/grouped-render.ts)
  • steipete: GitHub file history for the live tool-stream path shows recent Control UI source work touching the area that produces keyed tool messages. (role: adjacent live-tool-stream contributor; confidence: low; commits: 12a56d4d46dd, a509c48f0ea2, e71d10fd4d74; files: ui/src/ui/app-tool-stream.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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. 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: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 29, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@harjothkhara

harjothkhara commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #97873, which addresses the same failed-turn tool-error banner regression (#97849) and is further along — proof: sufficient, ready for maintainer look.

This was referenced Jul 1, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failed cron/autonomous turn shown as succeeded: cross-turn tool-error banner suppression (regression of #90122)

1 participant

@harjothkhara