Fix cross-turn assistant overwrite + separate reasoning bubbles#676
Open
RBrid wants to merge 1 commit into
Open
Fix cross-turn assistant overwrite + separate reasoning bubbles#676RBrid wants to merge 1 commit into
RBrid wants to merge 1 commit into
Conversation
Three related fixes for chat timeline rendering, validated by three rounds of adversarial dual-model code review. 1. Cross-turn assistant overwrite (the original repro) In a user -> reply1 -> tool-call -> tool-output -> reply2 flow, reply2 was silently overwriting reply1's text in place instead of appending a new bubble. Cause: the chat.message final for reply2 carried reconcilePrevious=true, and UpsertAssistant's fast-path merged into the most recent assistant entry unconditionally - including a previously finalised one. Fix in ChatTimelineReducer.UpsertAssistant: narrow reconcile to only merge into entries that are still IsStreaming=true. A finalised assistant entry (IsStreaming=false) belongs to a completed turn and must not be overwritten. The byte-equal duplicate safety net remains unconditional so genuine duplicate emissions still collapse. Defense-in-depth: extract ClearStreamingAtTurnBoundary helper and call from both AddLocalUser (production typed-input path) and ApplyUserMessage (gateway-injected path), hoisted above the nonce early-return. This handles the dropped-ChatTurnEndEvent scenario where a stale ActiveAssistantId could otherwise leak across turns. 2. Concatenated thinking prose (Option B) The gateway emits a stream:"item", kind:"reasoning", phase:"end" bracket marker between distinct thinking passes within a single turn. Map it to a new ChatReasoningEndEvent that nulls ActiveReasoningId so the next reasoning chunk starts a fresh bubble instead of appending to / replacing the previous one. Also tag delta-state ChatMessageEvents with IsStreaming=true so the reducer's new reconcile guard distinguishes them from finals. 3. Trace logging plumbing Add IOpenClawLogger.Trace verbose channel with a default no-op so existing implementers (tests, console logger, setup logger) don't need updating. Logger.Trace in the tray is gated by OPENCLAW_TRAY_TRACE=1|true. Forward Trace through DiagnosticTeeLogger and AppLogger; SetupOpenClawLogger inherits the default no-op (no opt-in gate available in setup engine). OpenClawGatewayClient now surfaces item-event kind+phase metadata (no payload content) at trace level for shape diagnostics. Test coverage added in ChatTimelineReducerTests: - StaleStreamingPreview_DoesNotMergeAcrossUserBoundary - UserMessage_AsTurnBoundary_PreventsCrossTurnOverwrite - AddLocalUser_AsTurnBoundary_PreventsCrossTurnOverwrite - AddLocalUser_ClearsStaleStreamingPreviewAcrossTurns Validation: - ./build.ps1: succeeds - OpenClaw.Tray.Tests: 944 passed - OpenClaw.Shared.Tests: 2045 passed / 29 skipped Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed June 3, 2026, 4:26 PM ET / 20:26 UTC. Summary Reproducibility: yes. Source inspection shows current master applies ReconcilePrevious from live assistant chat.message frames and merges into the latest assistant entry without checking IsStreaming, which matches the overwrite path described by the PR. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge only after redacted live tray/gateway proof and required validation confirm finalized assistant bubbles are preserved and reasoning passes split correctly in the real UI. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current master applies ReconcilePrevious from live assistant chat.message frames and merges into the latest assistant entry without checking IsStreaming, which matches the overwrite path described by the PR. Is this the best way to solve the issue? Yes for the code direction: limiting reconcile merges to still-streaming assistant entries and closing reasoning bubbles on item end is a narrow reducer/provider fix. Merge should still wait for live behavior proof because the changed path depends on real gateway event ordering. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model gpt-5.5, reasoning high; reviewed against be64bea0bec8. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
the
merge-risk: 🚨 message-delivery
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related fixes for chat timeline rendering, validated by three rounds of adversarial dual-model code review.
Cross-turn assistant overwrite (the original repro) In a user -> reply1 -> tool-call -> tool-output -> reply2 flow, reply2 was silently overwriting reply1's text in place instead of appending a new bubble. Cause: the chat.message final for reply2 carried reconcilePrevious=true, and UpsertAssistant's fast-path merged into the most recent assistant entry unconditionally - including a previously finalised one.
Fix in ChatTimelineReducer.UpsertAssistant: narrow reconcile to only merge into entries that are still IsStreaming=true. A finalised assistant entry (IsStreaming=false) belongs to a completed turn and must not be overwritten. The byte-equal duplicate safety net remains unconditional so genuine duplicate emissions still collapse.
Defense-in-depth: extract ClearStreamingAtTurnBoundary helper and call from both AddLocalUser (production typed-input path) and ApplyUserMessage (gateway-injected path), hoisted above the nonce early-return. This handles the dropped-ChatTurnEndEvent scenario where a stale ActiveAssistantId could otherwise leak across turns.
Concatenated thinking prose (Option B) The gateway emits a stream:"item", kind:"reasoning", phase:"end" bracket marker between distinct thinking passes within a single turn. Map it to a new ChatReasoningEndEvent that nulls ActiveReasoningId so the next reasoning chunk starts a fresh bubble instead of appending to / replacing the previous one. Also tag delta-state ChatMessageEvents with IsStreaming=true so the reducer's new reconcile guard distinguishes them from finals.
Trace logging plumbing Add IOpenClawLogger.Trace verbose channel with a default no-op so existing implementers (tests, console logger, setup logger) don't need updating. Logger.Trace in the tray is gated by OPENCLAW_TRAY_TRACE=1|true. Forward Trace through DiagnosticTeeLogger and AppLogger; SetupOpenClawLogger inherits the default no-op (no opt-in gate available in setup engine). OpenClawGatewayClient now surfaces item-event kind+phase metadata (no payload content) at trace level for shape diagnostics.
Test coverage added in ChatTimelineReducerTests:
Validation: