Repository navigation
Conversation
The grace timer can trip while stdout is drained after wxc-exec has exited. The executor already returns TimedOut false for that result. The caller was also treating the cancelled grace token as a timeout and replacing the exit code with -1. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
🦞👀 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
P2
|
Codex review: needs real behavior proof before merge. Reviewed October 7, 2026, 2:11 PM ET / 18:11 UTC (Revision 4). ClawSweeper reviewWhat this changesThe PR preserves sandbox command exit codes when the host grace token expires after completion and adds timeout-classification and real-executor cancellation tests. Merge readiness⛔ Blocked before merge - 2 items remain This PR remains useful: current main and the latest release retain the old classification. The new host-cancellation trace addresses part of the previous review, but real proof of the changed post-exit branch remains outstanding. Priority: P2 Review scores
Verification
How this fits togetherWindows node sandbox execution launches commands through Microsoft's MXC launcher. Its completion, captured output, and timeout status become the system.run result returned to the agent. flowchart TD
A[Agent command] --> B[Windows node sandbox runner]
B --> C[MXC launcher]
C --> D[Process completion or cancellation]
D --> E[Bounded output drain]
E --> F[Timeout classification]
F --> G[Agent command result]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use process completion as the timeout authority while preserving caller cancellation, backed by direct evidence of the post-exit grace-expiry case. Do we have a high-confidence way to reproduce the issue? No high-confidence runtime reproduction is established. Source shows the misclassification if grace expires after process completion, but the supplied real runs explicitly did not reach that timing condition. Is this the best way to solve the issue? Yes, trusting MxcExecutor's completion result is a narrow repair consistent with its existing contract; the added helper test alone does not prove the production timing path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5e3fb40bcc89. LabelsLabel changes: No label changes. Label 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 (3 earlier review cycles) |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
wxc-exec returns on its own script deadline, several seconds before the host grace timer. A sandbox command cannot hold it open that long. Run the production executor with a 60 second sandbox deadline and cancel the host token at 2 seconds. The long cmd loop is still running, so the host reports TimedOut and "Execution was cancelled." On this machine that returned in 2414 ms. The post-exit drain overlap inside ExecuteAsync is still not covered. That path exits about five seconds before the grace timer. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
The UI job failed one onboarding case, Existing route choice 0, on Assert.True(resultBar.IsOpen) after a cancelled check. The Remote case in the same theory passed. The previous head's Build and Test run was green, and this commit does not change that page. GitHub refused gh run rerun without admin rights, so this empty commit starts the lane again. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
This branch has not been deployed
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.
What Problem This Solves
Fixes: a sandboxed command that finishes while its output is draining is reported as a timeout.
User Impact
User impact: a completed command keeps its exit code. A command the executor marks as timed out is still reported as a timeout.
Why This Change Was Made
The host grace timer is the command timeout plus five seconds. After
wxc-execexits, the executor drains output and returnsTimedOutfalse. The caller also treated a cancelled grace token as a timeout, so the exit code became -1.Evidence
wxc-exec.exeis the@microsoft/mxc-sdkbinary the tray build copies totools\mxc\x64\.MxcAvailability.Probeon this host reportedtier=base-containerandcan_system_run=True.DirectAppContainerExecutor.ExecuteAsyncon heade4a7d6e19b56621f6067eec058ccee36e7d90d03then ran three commands:finishediscmd /c echo mxc-finishedwith a 30s limit.sleptis PowerShellStart-Sleep -Seconds 2with a 30s limit, and the marker was printed.over_timeoutis the same sleep for 8 seconds with a 2s limit. The process was stopped at about 2.1s, the marker was not printed, and the host grace (7s) had not fired, soTimedOutstayed false. The earlier helper check still holds: a finished result with a cancelled grace token reportsreported_timeout=False, and an executor result withTimedOuttrue still reports a timeout..\scripts\validate-mxc-e2e.ps1 -NoBuildon this head exited 0. The throwaway distro listened on127.0.0.1:27087and was unregistered by that run. Ubuntu-24.04 stayed registered. 18 tests passed. The three pool proofs passed. Gatewaynode.invokesystem.runofWrite-Outputreturned exit 0,timedOutfalse, containmentmxc, duration 510 ms, and the marker. The denied write returned exit 1,timedOutfalse, containmentmxc, duration 302 ms, and the target file was absent.Change Type
Scope
winnodeRequired proof pools
windows-wsl-mxc:.\scripts\validate-mxc-e2e.ps1 -NoBuildexited 0 on this head.MirroredWslSafeGatewayPort_IsListeningAndRecorded,RealGateway_SystemRun_ExecutesThroughWindowsNodeMxcSandbox, andRealGateway_SystemRun_BlocksWritesToTrayDataDirectoryInMxcSandboxpassed. The successfulsystem.runwas exit 0,timedOutfalse, containmentmxc.Validation
./build.ps1exit 0 (Shared, Cli, WinNodeCli, SetupEngine, WinUI).dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore: 4263 passed, 33 skipped, 0 failed, 4296 total. The skipped MXC integration tests are gated onOPENCLAW_RUN_INTEGRATION. TheExecuteAsynctrace above used the samewxc-exec.exe.dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj: 3990 passed, 0 skipped, 0 failed.Real behavior proof
wxc-exec.exefrom@microsoft/mxc-sdk, probe tierbase-container. Heade4a7d6e19b56621f6067eec058ccee36e7d90d03.MxcAvailability.Probe, thenDirectAppContainerExecutor.ExecuteAsyncforecho mxc-finished, a 2 second PowerShell sleep, and an 8 second sleep with a 2 second limit. Then.\scripts\validate-mxc-e2e.ps1 -NoBuild.TimedOutfalse, and the marker. The 8 second sleep was stopped at about 2.1 seconds, the marker was absent, andTimedOutstayed false because the 7 second host grace had not fired. The gatewaysystem.runreturned the marker with exit 0 andtimedOutfalse. The denied write did not create the file.wxc-execstops the process atTimeoutMs, and the host grace is that limit plus five seconds. The post-exit drain cap is 500 ms, so a normalwxc-execexit does not overlap the grace. The 8 second sleep returned in 2139 ms, before the 7 second grace.e4a7d6e19b56621f6067eec058ccee36e7d90d03ExecuteAsyncwithwxc-exec, then.\scripts\validate-mxc-e2e.ps1 -NoBuild.timed_out=Falsewith the marker. The 8 second sleep is exit -1, marker absent,timed_out=False.ExecuteAsync.wxc-execreturns about five seconds before that deadline, and the drain cap is 500 ms.Security Impact
Yes, explain the risk and mitigation:Compatibility and Migration
Review Conversations
Host token on this head
wxc-execstill returns on its own script deadline, about 0.2 seconds later, so a normal sandbox command never reaches the host grace timer at limit plus 5 seconds. Astart /bchild does not bridge that gap either.The production
MxcExecutorwas run with the sandbox deadline set to 60 seconds and the host token cancelled at 2 seconds, while acmdloop was still running. Result:exitCode=-1; timedOut=True; durationMs=2414; error=Execution was cancelled.Post-exit drain inside
ExecuteAsyncis still not shown. That drain is capped at 500 milliseconds, andwxc-execexits about 5 seconds before the grace timer.