Skip to content

fix(chat): keep cancelled responses from ending newer turns - #1440

Open
bkudiess wants to merge 1 commit into
mainfrom
bkudiess-chat-run-correlation
Open

bkudiess wants to merge 1 commit into
mainfrom
bkudiess-chat-run-correlation

Conversation

@bkudiess

Copy link
Copy Markdown
Collaborator

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 / ChatLifecycleState owners. It also:

  • Separates FIFO terminal retention from the latest legitimate trailing-final eligibility.
  • Requires retained run/tool identity for late tool repair, without reactivating an idle turn.
  • Carries a synchronous per-frame notification veto to the existing parser notification path, without another run cache or changes to App.
  • Keeps aborted/completed starts fenced while allowing a genuine same-run retry after a non-definitive lifecycle error.

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.

  • 12 added review-gap cases: failed before, passed after.
  • Retry/definitive-error controls cover plain failures, settled failures, exhausted fallback, cancellation, timeout, liveness facts, and newer-run fences.
  • A differential probe compared the compiled C# retry predicate with actual upstream TypeScript at openclaw/openclaw@eb82ef8b80a05058619557dff09f758a6710d4d5: 30,438 combinations, zero mismatches. Probe artifacts are retained locally, not uploaded.
  • Independent Opus 5 and GPT-5.6 Sol Hanselman reviews, followed by source-verified immutable-revision checks: no actionable review blockers on the final source.

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

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required 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.

Command Result
.\build.ps1 Passed, all five targets
dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore 3,996 passed, 33 skipped, 0 failed
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore 3,057 passed, 0 failed

Focused command: 624 passed, 0 failed.

dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter 'FullyQualifiedName~ChatRunCorrelationTests|FullyQualifiedName~OpenClawChatDataProviderTests|FullyQualifiedName~ChatTimelineReducerTests|FullyQualifiedName~ChatConversationStateTests|FullyQualifiedName~ChatResetStateTests'

All final validation processes set OPENCLAW_REPO_ROOT and the four tray data-root overrides to worktree/session-owned locations. Optional integration/E2E/MXC/gateway-scenario flags were unset, not set to 0.

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

  • Environment tested: native Windows, isolated automated parser/provider/state/reducer tests; no live app or gateway.
  • PR head or commit tested: source in 896d323601866c74014d83ec8c70d9fa1fd1e4f7, verified against the validated manifest above.
  • Exact steps or command run: focused command above. Cancel A, inject late events while idle, start and stream B, inject A again, assert B remains unchanged before any additional B output, then finish B.
  • Evidence after fix: both cancellation-wave variants and terminal-order controls pass. Rejected wire finals produce no notification; accepted current finals still notify.
  • Observed result: no stale timeline mutation or premature new-turn termination in the automated cases.
  • Screenshot or artifact links verified? N/A. No current-head screenshots or externally uploaded logs are claimed.
  • Not verified or blocked: live UI, toast/TTS playback, real gateway delivery, and hosted CI at publication time. The test-only harness branch remains separate and was not imported.

Security Impact

  • New permissions or capabilities? No
  • Secrets or tokens handling changed? No
  • New or changed network calls? No
  • Command or tool execution surface changed? No. Tool timeline admission changes only; execution is unchanged.
  • Data access scope changed? No
  • If any answer is Yes, 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

  • Backward compatible? Yes, preserving legacy missing-ID handling.
  • Config or environment changes? No
  • Migration needed? No
  • If yes, list the exact upgrade steps: N/A.

Pinned 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

  • I replied to or resolved every bot review conversation addressed by this PR.
  • I left unresolved only conversations that still need maintainer judgment.

No hosted review conversations existed at publication; local independent review findings were resolved or explicitly classified above.

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>
@clawsweeper

clawsweeper Bot commented Sep 17, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. 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. labels Sep 17, 2026
@clawsweeper

clawsweeper Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 17, 2026, 7:18 PM ET / 23:18 UTC (Revision 2).

ClawSweeper review

What this changes

Preserves 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
Reviewed head: 896d323601866c74014d83ec8c70d9fa1fd1e4f7
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with extensive regression coverage and no identified blocking code defect; native proof disposition remains separate from patch quality.
Proof confidence 🌊 off-meta tidepool Not applicable: The ordinary external-contributor gate does not apply to this COLLABORATOR-authored PR. Reported tests exercise parser/provider/state/reducer admission, including stale-final notification vetoes, but use injected frames and a fake bridge; declared real Gateway, WinUI, toast, and TTS coverage remains a separate maintainer proof decision.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The ordinary external-contributor gate does not apply to this COLLABORATOR-authored PR. Reported tests exercise parser/provider/state/reducer admission, including stale-final notification vetoes, but use injected frames and a fake bridge; declared real Gateway, WinUI, toast, and TTS coverage remains a separate maintainer proof decision.
Evidence reviewed 10 items Applicable repository policy: Read the complete root AGENTS.md and proof-validation skill. No applicable nested AGENTS.md or maintainer-notes directory was found. The architecture ledger keeps conversation transactions, lifecycle identity, and event mapping in the owners used by this patch.
Pinned patch scope: Inspected the complete introduced changes across 12 files. Host-verified original head and test-merge parentage distinguish these changes from unrelated main-branch workflow and packaging updates.
Current main still needs run-aware admission: Direct inspection of current-main ChatConversationState, ChatLifecycleState, and gateway message construction shows no incoming chat run-ID gate; assistant completion still removes the thread's active run. The latest supplied release, v2026.9.4, is the PR merge base. A broader comparison encountered an unavailable promisor blob, so this conclusion uses the successfully inspected individual source files and supplied introduction evidence.
Findings None None.
Security None None.

How this fits together

Windows 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]
Loading

Decision needed

Question Recommendation
Must the declared WinUI and Gateway proof pools complete before landing, or may this collaborator-authored fix land with the explicitly recorded native coverage gap? Complete the declared proof: Obtain redacted current-head evidence for the already-declared cancellation, newer-turn, and notification behavior before landing.

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

  • Resolve merge risk (P1) - Actual Gateway event ordering and native timeline, toast, and TTS behavior remain unverified in the declared proof pools.
  • Complete next step (P2) - Resolve the declared WinUI/Gateway proof gap by completing its current-head coverage or recording explicit maintainer acceptance of the unverified native behavior.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production +273 net (+304/-31); tests +736; docs +31 Production growth implements run correlation and admission within existing owners, with substantially larger regression coverage.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the existing chat ownership boundaries and legacy compatibility, with native evidence establishing stale-output rejection and successful current-response completion before landing.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best 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.

Labels

Label justifications:

  • P2: This is a bounded native-chat correctness repair for delayed cancelled responses interfering with later turns.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The ordinary external-contributor gate does not apply to this COLLABORATOR-authored PR. Reported tests exercise parser/provider/state/reducer admission, including stale-final notification vetoes, but use injected frames and a fake bridge; declared real Gateway, WinUI, toast, and TTS coverage remains a separate maintainer proof decision.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md and proof-validation skill. No applicable nested AGENTS.md or maintainer-notes directory was found. The architecture ledger keeps conversation transactions, lifecycle identity, and event mapping in the owners used by this patch. (AGENTS.md:86, 896d32360186)
  • Pinned patch scope: Inspected the complete introduced changes across 12 files. Host-verified original head and test-merge parentage distinguish these changes from unrelated main-branch workflow and packaging updates. (896d32360186)
  • Current main still needs run-aware admission: Direct inspection of current-main ChatConversationState, ChatLifecycleState, and gateway message construction shows no incoming chat run-ID gate; assistant completion still removes the thread's active run. The latest supplied release, v2026.9.4, is the PR merge base. A broader comparison encountered an unavailable promisor blob, so this conclusion uses the successfully inspected individual source files and supplied introduction evidence. (src/OpenClaw.Tray.WinUI/Chat/ChatConversationState.cs:1318, 0246e33f6e8f)
  • Run admission and notification ownership: Lifecycle admission rejects aborted, mismatched, and completed output while preserving the latest eligible trailing final. The provider synchronously vetoes rejected notifications, and the existing bridge forwards the same message object before parser notification emission. (src/OpenClaw.Tray.WinUI/Chat/ChatLifecycleState.cs:90, 896d32360186)
  • Regression coverage: Added tests cover cancelled output before and after terminal cleanup, newer-turn preservation, terminal retention order, retry eligibility, reset fences, known tool reconciliation, and rejected versus accepted wire notifications. The wire notification test invokes the parser through reflection with a fake bridge; it does not exercise a real Gateway transport. (tests/OpenClaw.Tray.Tests/OpenClawChatDataProviderTests.cs:2549, 896d32360186)
  • Explicit upstream contract dependency: The changed connection documentation explicitly identifies the pinned OpenClaw lifecycle predicate as the retry contract. This establishes a dependency on OpenClaw normalization semantics, not on a Codex runtime. (docs/CONNECTION_ARCHITECTURE.md:109, 896d32360186)

Likely related people:

  • Barbara Kudiess: Raw commit f8a8825 adds src/OpenClaw.Tray.WinUI/Chat/ChatLifecycleState.cs:9 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: f8a8825ccbb4; files: src/OpenClaw.Tray.WinUI/Chat/ChatLifecycleState.cs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Complete the declared WinUI and Gateway coverage with redacted current-head evidence of stale-output rejection and accepted-current-response notifications.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-17T23:09:54.869Z sha 896d323 :: blocked before merge. :: none

@bkudiess
bkudiess marked this pull request as ready for review September 17, 2026 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant