Repository navigation
Conversation
Retire native reasoning slider handlers and queued selections from component effect cleanup as well as ordinary element unmount. Add a mounted regression with a live selection control and document the lifecycle 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. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR cancels queued effort selections and detaches native slider handlers when a chat host is disposed. Example: Commit Low, queue High, then dispose the chat host before its dispatcher drains.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this focused lifecycle fix remains necessary on main, and no introduced defect was found; the draft hold and failed hosted UI gate still prevent landing. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe native effort picker receives advertised choices and user input, then forwards accepted selections to the existing chat controller; Reactor owns its mounting and disposal. flowchart TD
A[Advertised effort choices] --> B[Native effort picker]
C[Pointer and keyboard input] --> B
B --> D[Existing input revision policy]
D --> E[Chat controller]
F[Unmount or host disposal] --> D
F --> B
Technical reviewBest possible solution: Retain one picker-owned retirement function that invalidates pending input and removes native handlers from both supported teardown paths. Do we have a high-confidence way to reproduce the issue? Current-main source permits queued callbacks to survive host disposal, and the supplied native before/after regression records that exact failure; this read-only review ran no target code. Is this the best way to solve the issue? The patch is the smallest complete repair at the existing picker owner: preserve live selections while making unmount and disposal invalidate pending work through the same cleanup function. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b6f158ab257a. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Preflight sourceSource unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified. Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (4 earlier review cycles)
Reviewed November 7, 2026, 1:11 AM ET / 06:11 UTC (Revision 5). |
|
@clawsweeper re-review Additional verified baseline and CI evidence for unchanged head
The scoped before/after regression remains meaningful: the original picker dispatched No code, test assertions, rating labels, or draft state were changed for this reassessment. Native pointer/key teardown, combined unmount/disposal, and conclusive original-crash attribution remain unverified. A real-app smoke attempt was not counted as changed-behavior proof because its fixture did not advertise slider choices. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Additional real Companion lifecycle proofThe unchanged Companion executable, not an in-process renderer test, was exercised through computer-use and its existing local MCP entry points in a disposable Windows Sandbox with Windows App Runtime 2.6.4.0. Both real-app fixture runs exited successfully. The fixture supplied only valid synthetic Exact app:
Copied live app state for The contrast run similarly reports Empty=Low / Long=High before normal Slider unmount and after host disposal; the reopened five-choice Slider then commits Empty=Medium / Long=High. The normal-unmount and effect-cleanup paths are mapped by the unchanged The session-only launcher command was Verified state-journal SHA256s: first journey The two attached, unmodified computer-use frames show the one-choice High popup after ordinary Slider replacement (20:01 UTC) and the five-choice Medium Slider responding after Workspace disposal/reopen (20:06 UTC). They are after-fix runtime frames, not a before/after visual comparison or evidence of the original queued callback failure. Limits remain: the same-dispatcher queued-callback before/after proof is still the separate mounted native regression. The manual app journeys do not inspect private native handler lists or inject disposal during a held pointer. The full native UI testhost failure remains failed and unexplained; these passing shipped-app journeys do not override CI or establish the finalizer crash's cause. Host profile metadata changed during concurrent unowned app activity, so no unchanged-host-profile claim is made. The guest mapped no production profiles and was stopped after both app runs exited 0.
|
|
@clawsweeper re-review. New current-head evidence is in the updated body and #2046 (comment): unchanged Companion executable, actual pointer/keyboard input, ordinary Slider replacement, Workspace disposal/reopen, verified live session state, and two inspected runtime screenshots. Head remains da04581. Both real-app runs exited 0. Failed local and hosted native UI validation remain disclosed; the original crash is not claimed fixed. Please reassess the new proof, without treating passing focused journeys as a complete native CI pass. |
|
|
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
the
proof: sufficient
Complete exact-candidate native capture, without erasing earlier failuresOne additional full native UI invocation completed on unchanged head dotnet vstest '<same-ui-payload>\OpenClaw.Tray.UITests.dll' '--TestCaseFilter:Category!=Accessibility&Category!=NativeOnboardingProof&Category!=GatewayFixture' '--Logger:trx;LogFileName=candidate-ui.trx' '--ResultsDirectory:<owned-output>\TestResults\UI' '--Blame:CollectHangDump;HangDumpType=none;TestTimeout=300s'Result: failed, but complete. All 645 result records are present: 635 passed, 3 failed assertions, 7 existing permitted skips. The new queued-disposal regression and all seven current-value reasoning variants passed. The testhost exited 0; the runner exited 1 for assertions. No native fault or dump was captured.
The affected Workspace, SVG and UI fixture source files are byte-unchanged between the exact base and PR head. That is not, by itself, proof that transitive lifecycle changes cannot affect them. Debugger timing can also differ, so this attempt is not an uninstrumented acceptance pass. TRX SHA256: This completes a full current-head inventory that the earlier abort could not complete. It does not retract the earlier local finalizer access violation or hosted silent reasoning-body abort, establish their native cause, waive CI, or change the draft hold. No timeout, assertion, pooling behavior, source, PR head, or rating label was altered. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Complete uninstrumented native UI result on the unchanged picker fixThe original full native UI filter now completed without a debugger or native observer in the owned disposable Windows Sandbox with App Runtime 2.6.4.0: dotnet vstest '<staged-ui>\OpenClaw.Tray.UITests.dll' '--TestCaseFilter:Category!=Accessibility&Category!=NativeOnboardingProof&Category!=GatewayFixture' '--Logger:trx;LogFileName=uninstrumented.trx' '--ResultsDirectory:<owned-output>\UninstrumentedResults' '--Blame:CollectHangDump;HangDumpType=none;TestTimeout=300s'Passed: 638. Failed: 0. Existing permitted skips: 7. Total: 645. Runner exit 0. Started Source remains PR head TRX SHA256: The earlier failures remain: the hosted run 42270201206 attempt 1 is still failed, and the earlier local finalizer AV is preserved. This complete local pass is acceptance evidence for the current native test inventory, not a causal explanation or proof that the hosted crash is unrelated. No assertion, timeout, filter, runtime version, PR head or rating label was relaxed or changed. A separate full observer attempt produced no TRX/completion and its recorded processes disappeared. It is invalid diagnostic evidence and is not counted as a pass or native fault. The direct uninstrumented invocation above used the existing guest execution path independently. The unpublished observer prototype still requires investigation before use on a hosted full lane. The guest was stopped after preserving the reports. No production profile was mapped, and no CI rerun, code commit/push, ready switch or merge occurred. |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Related: #2028 (feat: support Sandbox-backed Windows file transfers). This is an independent lifetime fix, not the file-transfer implementation or a claim that its silent native CI crash is resolved.
Draft validation blocker: the fresh standalone full UI run aborted with a native finalizer-thread access violation. The new lifetime regression and subsequent real Companion input/lifecycle journeys passed, but this candidate is not merge-ready.
What Problem This Solves
Fixes: a queued effort-selection callback can run after its chat view is disposed.
User Impact
User impact: closing a chat view cancels pending effort input and releases its native slider handlers; the picker layout and live selection behavior are unchanged.
Why This Change Was Made
Reactor host disposal runs component effect cleanup without guaranteeing element
OnUnmount. The picker now uses the same idempotent retirement function from both paths, clearing native handlers and invalidating the existing queued-input revision. No dependency update, retry, timeout increase or weakened assertion is involved.Evidence
The new real-WinUI regression first commits a live
"low"selection, then queues"high"and disposes the host in the same dispatcher callback. Before the production change it failed withCollection: ["high"]after disposal; afterward it passed while preserving the live-selection control.Real Companion proof with two verified screenshots and copied live output now separately exercises the unchanged application executable: native pointer and Ctrl+Left input, closing/reopening the actual Workspace, and Slider-to-single-choice-to-Slider replacement. Both fixture app runs exited 0. The producer only seeds valid synthetic Gateway session metadata; it does not patch app controls or call selection handlers. These journeys complement, rather than replace, the queued-callback regression.
Claude Opus 5.5 and GPT-6 Astra independently reviewed the three-file patch and found no actionable issues. Review scope was the byte-identical local fix atop
2d0b530e5689506bd75a79c5420c46d42f2b70b1; the standalone rebase preserved all three reviewed file blobs. This is carried-forward source review, not a newly executed review of the new ancestry.Three earlier unchanged-source runs on runtime 2.6.4 did not reproduce the original silent native crash. However, the fresh standalone full UI run did fail with
0xC0000005inMarshal.Release -> NativeObjectWrapper.Release -> Finalize -> GC.RunFinalizers. Its active case wasWorkspaceWindowProofTests.SettingsChatLink_ReturnsToRetainedChatInSameWindow, not the new lifetime test. Whether this is the same failure as the original silent crash remains unproven. Independent exact-base and historical comparison retains the distinction: the exact-base run had two Workspace timing assertions and no native fault, while historical main had a matching finalizer stack. Neither clears this candidate's failed native validation.Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: native reasoning-slider lifetime and disposal. The mounted regression and real Companion input/lifecycle journeys exercise the changed contract on disposable Windows desktops; no visual layout changed. This does not claim the entire visual/accessibility pool is complete.Validation
Fresh checks on standalone commit
da045817f15cff77ba99a49f7a5e6a0c5c1627b4:.\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --logger 'trx;LogFileName=shared-pr.trx' --results-directory '<proof>\closeout\TestResults'dotnet build .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj -c Debugdotnet build .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -c Debug -r win-x64 -p:ProduceReferenceAssembly=falsedotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --no-build --logger 'trx;LogFileName=tray-pr.trx' --results-directory '<proof>\TestResults\Tray'dotnet vstest '<staged-ui>\OpenClaw.Tray.UITests.dll' '--TestCaseFilter:Category!=Accessibility&Category!=NativeOnboardingProof&Category!=GatewayFixture' '--Logger:trx;LogFileName=ui-pr.trx' '--ResultsDirectory:<proof>\TestResults\UI' '--Blame:CollectHangDump;HangDumpType=none;TestTimeout=300s'dotnet Launcher.dll <exact-App.exe> <owned-output>anddotnet Launcher.dll <exact-App.exe> <owned-output> contrastgit diff --checkThe full build includes documentation validation. Tray/UI execution uses prebuilt, hashed outputs in a writable disposable guest checkout.
--no-buildis explicit; fresh TRX inventories are checked to prevent successful no-op commands from being counted as passes. No test filters were weakened. The separate hosted UI job also failed: run 42270201206, attempt 1, UI job 128074377968, tested merge2834b4c1414b6fc766b80382c907786dcc705a17. Its silent abort has no native stack and is not equated with the local finalizer AV.Real Behavior Proof
da045817f15cff77ba99a49f7a5e6a0c5c1627b4. The guest verifies the exact source commit, changed-file blobs and staged test/app DLL hashes. Real app DLL SHA256:077C3A6520AB07A24EB3976788AC705C2FBB8071C29877426C0520B3F88906CC.ReasoningPicker_HostDisposalCancelsQueuedValueChange. It verifies a live slider change, queues a second change, disposes the host before the queued callback executes, and confirms no post-disposal selection. Separately, the real-app launcher commands above exercise native pointer and keyboard input via computer-use, actual Workspace close, existingapp.navigatereopen, normal Slider replacement by a one-choice button, host disposal, and new Slider input afterward. Existingapp.chat.snapshotandapp.statuscorroborate session levels and Connected state.ReasoningPicker_HostDisposalCancelsQueuedValueChangepassed in the exact-commit full UI TRX. Before the production change, the identical regression failed withCollection: ["high"]after disposal. Real Companion journeys show Low retained across close/reopen and successful input on the new Slider; the contrast session remains High. The fresh full UI run later crashed in native finalization.Yes/No/N/A): Yes. Current-head real-app screenshots and copied live output. These are after-fix runtime frames, not a visual before/after comparison or a screenshot of the queued callback itself.Copied fresh run signal:
UI TRX SHA256:
0D22C558D3702E288ACC17C03604EBFD9CDC05E5C4F94224000670196721DBF1. Blame names the active Workspace test but explicitly warns it may not be the crash source. No native dump was collected by this validation invocation.Security Impact
Yes/No): NoYes/No): NoYes/No): NoYes/No): NoYes/No): NoYes, explain the risk and mitigation: N/ACompatibility and Migration
Yes/No): YesYes/No): NoYes/No): NoReview Conversations