Conversation
Preserve gateway run identity, isolate stale chat and tool output, and gate notifications on chat admission. Keep trailing finals, bounded terminal retention, and legitimate lifecycle retries consistent with the pinned upstream contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 17, 2026, 7:18 PM ET / 23:18 UTC (Revision 2). ClawSweeper reviewWhat this changesPreserves response run identity so delayed cancelled responses cannot overwrite newer chat turns or trigger stale notifications, while retaining legitimate trailing results and retries. Merge readiness⛔ Blocked before merge - 3 items remain The fix remains useful and is absent from inspected current-main code. No actionable patch defect was found. The unchanged head still carries the previously declared native UI and Gateway proof gap, which needs maintainer disposition. Priority: P2 Review scores
Verification
How this fits togetherWindows native chat receives response and lifecycle events from the Gateway. Its conversation state decides which events update the timeline, complete a turn, or produce notifications. flowchart TD
A[Gateway response events] --> B[Preserve session and run identity]
B --> C[Conversation admission]
D[Active cancelled and completed runs] --> C
C -->|Accepted| E[Chat timeline and turn completion]
C -->|Rejected| F[Suppress frame notification]
E --> G[Notification delivery]
Decision needed
Why: The automated coverage is substantial, but the repository's capacity-dependent native proof requires maintainer scheduling or explicit acceptance of the blocker. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the existing chat ownership boundaries and legacy compatibility, with native evidence establishing stale-output rejection and successful current-response completion before landing. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the mechanism: current main discards incoming chat run identity and completes the active thread run when a delayed assistant final arrives. The patch includes focused cancellation/restart regression cases; this review did not execute them. Is this the best way to solve the issue? Yes, the patch repairs admission through the existing lifecycle and conversation owners, reuses retained tool correlation, and preserves missing-ID compatibility without introducing another notification cache. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 0246e33f6e8f. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
What Problem This Solves
Fixes: a cancelled response's delayed messages can overwrite or end a newer response in the same conversation.
User Impact
Stopping one response no longer lets its late text, completion, tool activity, or notification interfere with the next response. Legitimate late tool-result reconciliation and retryable failures remain supported.
Draft: live UI/gateway proof has not been collected. No merge-readiness claim is made.
Why This Change Was Made
The chat parser discarded the payload's run ID, and assistant finals were admitted using thread-level suppression. A stale final could therefore complete the currently active run. After old terminal cleanup, stale agent deltas could also resume flowing.
This change preserves wire run identity and applies admission through the existing
ChatConversationState/ChatLifecycleStateowners. It also:App.Ownership remains unchanged: the conversation state coordinates atomic domain decisions, lifecycle state owns run/abort/terminal identity, and the reducer owns retained tool correlation. Missing-run-ID frames retain legacy thread-level compatibility.
Evidence
The original cancelled-final regression, two-wave late-event sequence, and review-discovered terminal ordering/cache/tool/notification cases were reproduced before the fixes. Assertions run before further new-run output can mask the failure.
openclaw/openclaw@eb82ef8b80a05058619557dff09f758a6710d4d5: 30,438 combinations, zero mismatches. Probe artifacts are retained locally, not uploaded.The additional structured autoreview helper did not complete: local mode misinterpreted Windows line endings as a whole-repository diff; commit mode accepted the approximately 80 KB bundle but its Codex process exited 1 without a usable result. This is not reported as a clean helper review.
Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: visually verify cancel/restart, absence of stale output, current-response completion, and notification behavior in the isolated app. Not verified / blocked: not launched in this workstream.windows-wsl-gateway-e2e: validate gateway event ordering and cancellation through the real gateway path. The changed chat sequence needs focused proof in addition to the pool's setup/connect baseline. Not verified / blocked: no live gateway/WSL execution was authorized.Validation
Validated source is byte-identical to the files committed in
896d323601866c74014d83ec8c70d9fa1fd1e4f7. Runs completed before the commit; final changed-file manifest SHA256:9550A945489A4E6C9F190EC91C136FE48BDE7260A423E339D7810C6C79EE3561..\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoreFocused command: 624 passed, 0 failed.
All final validation processes set
OPENCLAW_REPO_ROOTand the four tray data-root overrides to worktree/session-owned locations. Optional integration/E2E/MXC/gateway-scenario flags were unset, not set to0.Intermediate generated-reference corruption and unrelated MCP-disposal/root-environment failures were resolved with a worktree-only dependency rebuild and isolated reruns. No unrelated source was changed; all required commands subsequently passed.
Real Behavior Proof
896d323601866c74014d83ec8c70d9fa1fd1e4f7, verified against the validated manifest above.N/A. No current-head screenshots or externally uploaded logs are claimed.Security Impact
NoNoNoNo. Tool timeline admission changes only; execution is unchanged.NoYes, explain the risk and mitigation: N/A. Notification delivery fails closed when chat admission rejects a frame or its consumer throws; exceptions remain logged.Compatibility and Migration
Yes, preserving legacy missing-ID handling.NoNoPinned upstream tests establish ordinary tool segments as deltas followed by a terminal final after lifecycle end. A separate supplemental/status-final composition lacks an end-to-end reproduced pair and reliable correction identity; that behavior remains unchanged rather than adding a speculative content/sequence heuristic.
Review Conversations
No hosted review conversations existed at publication; local independent review findings were resolved or explicitly classified above.