Skip to content

feat: support Sandbox-backed Windows file transfers - #2028

Draft
bkudiess wants to merge 13 commits into
openclaw:mainfrom
bkudiess:bkudiess-windows-file-command-plan
Draft

bkudiess wants to merge 13 commits into
openclaw:mainfrom
bkudiess:bkudiess-windows-file-command-plan

Conversation

@bkudiess

@bkudiess bkudiess commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

What Problem This Solves

Windows nodes need typed file and directory transfers governed by the same Sandbox settings as command execution.

User Impact

Users can invoke file.stat, file.fetch, file.create, file.write, dir.list, dir.fetch, fs.listDir and terminal.upload through their applicable authenticated Gateway/MCP paths, with binary transfers supported by winnode.

Draft, not an unconditional merge-readiness claim. The previously reported token-generation, queued-approval, worker-signing and executor-disposal defects have code repairs and passing current-head regression records linked below. ClawSweeper's existing scored review targets the older eaa67e95, not this head. Final live authority-chain, native ARM64, rendered transfer-budget rollback and installed upgrade acceptance remain incomplete.

Browser/Canvas local file reads now use the shared file-access boundary and may be rejected by Sandbox policy. Positive unsupported-Windows compatibility is intentionally uncontained, matching existing system.run; containment failures on supported Windows do not allow host fallback.

Why This Change Was Made

One capability registry serves Gateway and local MCP. One Sandbox executor owns admission, committed policy, process/pipe lifetime and verified worker leases. A fixed native .NET worker performs retained-handle filesystem I/O with bounded streams/archives, no-clobber creation, in-place overwrite and identity-owned cleanup. Local-only app.* tools remain excluded from Gateway exposure.

The bounded persistent worker cache keeps executable infrastructure separate from protected app data. Signed first-party worker bytes are verified before manifest hashing; release consumers stage signed inputs outside the checkout. Current review fixes also publish immutable commit-bound settings snapshots and restore a rejected transfer budget in the native NumberBox.

Evidence

Current head: cd64009240995a6f1af1b4d0662a98053befad21.
Completed CI: Build and Test run 42326156137, attempt 1, conclusion success. The runner checked out merge da744ee3dcb7b314f78ea64b6fc1855ab1275740, whose parents are main 96ea36fe and this PR head. Its tree is exactly the PR-head tree, ec026eae844a2101c9d3312d590fd4fb499e7533, not an inferred association with an older run.

Downloaded and inspected 18 TRX reports from that run: 15,912 passed, zero failed, 47 skipped/not executed. This counts result records across lanes, not unique behaviors. CI Gate, core/CLI, Tray/setup/integration and UI/functional/accessibility all passed. The run's downloadable artifacts contain the actual reports, not just this summary.

Earlier review finding Current source and executed regression What it establishes / limit
Replaced bearer can acquire the new channel generation Coherent authentication publication; McpFileTransportTests.RequestDuringBearerRotationCannotAcquireTheReplacementEpoch and LeaseAuthBindingReplayAndRotationFailClosed: Passed, Shared TRX Real loopback HTTP/control handling with deterministic rotation interleaving. Old-generation open fails; replacement bearer opens a lease. This uses synthetic capabilities, not a native-worker final-I/O proof.
Queued command can outlive approval Final queue-boundary revalidation; NodeSandboxExecutorTests.QueuedCommandRechecksRevokedApprovalBeforeNativeSpawn: Passed The second command loses approval while the first launch blocks the dispatcher; rejection is ExecPolicyDenied, and the supplied launcher is called only once. Fake launcher/approval callback: not a real ExecApprovalsStore + SDK process proof.
Executor leases outlive service shutdown NodeService disposal; ExecutorShutdownReleasesUncertainPayloadPinsWithoutDeletingUnknownRuntime: Passed Owner-level uncertain-launch teardown releases the lease and preserves unknown runtime storage. Service-call wiring is source evidence, not a live service-replacement run.
Unsigned worker / manifest integration fails release Signing workflow tests; FixedWorkerSigningPrecedesManifestEmbeddingInEveryDistribution, ReleaseWorkflow_VerifiesExecutableSigningPolicy, FileWorkerManifestTests.ReleaseManifestRejectsUnsignedWorkerBeforeWritingDigests for both RIDs: Passed Source/build contract and fail-closed unsigned-input checks. x64 publish, x64/ARM64 Dev+Store MSIX artifacts and Store bundle succeeded. Official signing job was skipped for this PR: not signed-release, Smart App Control or installed-upgrade acceptance.
Settings/data compatibility SettingsSandboxPublicationTests.FailedStoreEditRollsBackWithoutChangingCommittedPolicyOrUnknownFields, SettingsPersistenceNotificationTests.TypedStoreRollbackAndExplicitReloadPreserveExternalFieldsAndProtectedSecrets, FileWorkerTests.CompleteLegacyRecordsMigrateBeforeReapingAndSurviveInterruptedMarkerDeletion: Passed Actual settings persistence/secret and legacy upload-record compatibility at their owners. Not a latest-stable installer upgrade.

The subsequent three Copilot review conversations are resolved: commit-bound notification values/versions, transfer-budget restoration, and repeated binary-fetch chunks/ACKs. Their deterministic settings regressions pass in current CI. No Sandbox-specific screenshot or mounted rejected-budget proof is claimed.

Historical native runtime evidence, kept separate from current-head proof
  • x64 snapshot corresponding to 6f14dcd41496f5b9c84c9ca439e7eba45fba1ef7: actual cached worker/MXC and isolated Companion MCP passed 34 checks across all eight commands. Real task-local Gateway 2026.10.9 passed 21 public-client checks and 12 actual owning-service duplex checks. Cancellation/recovery, overwrite uncertainty and expired-upload cleanup were exercised; recorded owned process survivors and real-profile deltas were zero. Source-specific session evidence is not attached as public reproducible traces here.
  • Supplied native ARM64 report at 6f14dcd4: all eight commands passed 30 direct Companion MCP calls, three namespace tests passed, and the post-reboot formal WSL Gateway/MXC lane passed 18/18 with zero skips.
  • That report's complete owner suites still had 29 failures (28 Shared, one FileWorker) related to long-path/link fixtures. Later c8429c02 and 35eb0205 contain fixes, but there is no complete post-fix native ARM64 report attached. Those failures are not waived or relabeled as resolved by x64 CI.
  • Historical review/simplification and runtime evidence are not a new full review or runtime execution of cd640092.

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-wsl-mxc: command/file containment and protected-folder denial. Current CI skips the selected native system.run MXC cases; earlier native proof is historical.
  • windows-wsl-gateway-e2e: registration, pairing and Gateway invocation. Current setup/connect, revocation and network lanes pass; no current live all-eight file authority-chain claim.
  • windows-11-arm64: native worker/retained-I/O loading. Current ARM64 MSIX construction is not native execution proof.
  • windows-winui-interactive: Sandbox settings and visible failure/cleanup state. Sandbox accessibility tests pass; rejected-budget pixels remain unverified.
  • windows-clean-installer-upgrade: worker/cache installation, upgrade, cleanup and preserved data. Owner-level compatibility passes; installed upgrade remains blocked.
  • windows-11-sac-on: officially signed worker execution under enforcing reputation policy. Not exercised by PR signing skips.

These are required obligations, not completed pool claims.

Validation

The following local commands ran on the exact current head cd640092, with short task-owned NTFS TEMP, all three isolated tray roots, and inherited OpenClaw opt-ins cleared. Report/logger arguments are omitted; selectors/build flags are preserved.

Exact command Result
.\build.ps1 Passed all five product targets, including WinUI
dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore 5,118 passed, 28 ordinary opt-in skips
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter 'FullyQualifiedName!~OpenClaw.Tray.Tests.SettingsManagerIsolationTests.OpenClawTrayDataDirRedirectsSettingsAwayFromRealAppData' 5,272 passed, zero skipped; one explicitly excluded real-profile reader
dotnet test .\tests\OpenClaw.FileWorker.Tests\OpenClaw.FileWorker.Tests.csproj --no-restore 52 passed, zero skipped
dotnet test .\tests\OpenClaw.WinNode.Cli.Tests\OpenClaw.WinNode.Cli.Tests.csproj --no-restore 141 passed, zero skipped
dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter 'FullyQualifiedName~SettingsSandboxPublicationTests|FullyQualifiedName~SandboxFileSettingsContractTests|FullyQualifiedName~SettingsStoreTests|FullyQualifiedName~SettingsSharedStateContractTests|FullyQualifiedName~SettingsPersistenceNotificationTests|FullyQualifiedName~ReleaseSigningWorkflowTests' 55 passed, zero skipped
.\scripts\test-ci-workflow-contract.ps1 Passed
.\scripts\test-alpha-promotion.ps1 34 offline cases plus workflow contracts passed
.\scripts\validate-docs.ps1 Passed, 72 Markdown files
git diff --check Passed; exact validated/pushed worktree clean

The local excluded profile-reader test was executed and passed in current hosted Tray CI (5,273 passed, zero skipped), so the local exclusion is no longer an entirely missing automated case. CI also ran Dev settings (31 passed). This does not authorize or imply reading the local user's real profile.

Current hosted Shared: 5,135 passed/11 skipped; Connection: 1,746 passed/1 skipped; FileWorker: 52 passed; CLI: 141 passed. UI reports: Tray UI 645/7, functional UI 19/0, accessibility 21/13, fixture app 38/0, fixture UI 81/0, released-profile fixture 1/0, hardware profiles 12/0 (passed/skipped). SetupEngine 2,597/4; Tray integration 64/4. Exact results are in the linked run's artifacts. Do not add local and CI counts as independent behavior coverage.

A bounded independent rubber-duck implementation/proof-plan review of the eight review-fix files found no blocking introduced defects before publication. It is not a current-head exhaustive review of the entire feature.

Real Behavior Proof

  • Environment tested: local x64 Windows 10.0.26310.0 plus current hosted Windows CI. Native file-owner tests perform real retained-handle I/O in isolated test directories; transport tests use actual loopback HTTP with synthetic capabilities. Fixture UI uses synthetic Gateway data.
  • PR head or commit tested: head cd64009240995a6f1af1b4d0662a98053befad21; CI merge da744ee3dcb7b314f78ea64b6fc1855ab1275740, identical tree ec026eae844a2101c9d3312d590fd4fb499e7533.
  • Exact steps or command run: the owner tests in Validation and the named regression records below. Reports downloaded from run 42326156137, attempt 1, then inspected for outcomes and executed/skipped totals.
  • Evidence after fix, copied result records (all Passed, not merely source assertions):
Shared TRX:
McpFileTransportTests.RequestDuringBearerRotationCannotAcquireTheReplacementEpoch
McpFileTransportTests.LeaseAuthBindingReplayAndRotationFailClosed
NodeSandboxExecutorTests.QueuedCommandRechecksRevokedApprovalBeforeNativeSpawn
NodeSandboxExecutorTests.ExecutorShutdownReleasesUncertainPayloadPinsWithoutDeletingUnknownRuntime

FileWorker TRX:
FileWorkerTests.NewDestinationsInheritFinalParentAccessButStagesAndUploadsRemainPrivate
FileWorkerTests.PrivateStagesAndUploadStorageAreOwnedByTheCurrentUser
FileWorkerTests.StagePublicationRetainsTheNewDestinationDirectoryAgainstReplacement
FileWorkerTests.BoundWriteRejectsReplacementAndNoClobberCommitCannotReplaceARacedName
FileWorkerTests.WriteHashVerifiesNewBytesAndOverwritePreservesIdentity
FileWorkerTests.PaxArchivePreservesLongUnicodeNamesAndNestedPaths

Tray TRX:
SettingsSandboxPublicationTests.DelayedSavedNotificationCannotPublishAConcurrentRejectedEdit
SettingsSandboxPublicationTests.DelayedSavedNotificationsKeepCommitOrderVersionsAndDetachedValues
SettingsPersistenceNotificationTests.TypedStoreRollbackAndExplicitReloadPreserveExternalFieldsAndProtectedSecrets

Accessibility TRX:
AccessibilityScanTests.Page_PassesAccessibilityScan(pageName:"SandboxPage",pageTag:"sandbox",pageMarkerAutomationId:"SandboxPageMarker")
AccessibilityScanTests.SandboxExpanders_ExposeNamesWhenExpandedAndCollapsed
  • Observed result: permitted native worker-owner I/O and retained identity/integrity/ACL checks pass; deterministic rotation and queued-revocation boundaries reject their simulated forbidden callers. Sandbox controls pass the selected mounted accessibility checks. Full composed Companion/Gateway/MXC final effects are not inferred from these separate owner checks.
  • Screenshot or artifact links verified? (Yes/No/N/A) Yes for artifacts: test-results-core (artifact 13554769452), test-results-tray (13555359055), test-results-ui (13556502414), and three E2E result artifacts were downloaded from the linked current-head run. No inspected Sandbox-specific screenshot or rejected-budget pixels were found.
  • Not verified or blocked: current-head allowed file I/O through the full Companion/MXC/Gateway path together with replaced-bearer final-I/O denial, real store revocation while an SDK start blocks dispatch, and Gateway non-admin rejection of fs.listDir/terminal.upload; native ARM64 all-eight invocation and historical owner-failure disposition; rendered rejected-budget state; latest-stable signed/MSIX/Inno installed upgrade, Smart App Control and native-Gateway lifecycle. Setup/connect CI passed 53 and skipped 7, including native MXC system.run cases; revocation and network recovery passed 1 and 2 respectively. Those skips are not containment proof.

Security Impact

  • New permissions or capabilities? (Yes/No) Yes: eight file/directory commands, preserving their respective agent/operator authority.
  • Secrets or tokens handling changed? (Yes/No) Yes: immutable MCP authentication-generation publication and generation-fenced file-channel admission. Rotation regressions above pass; full native final-effect authority proof remains outstanding. Gateway credential precedence/storage is intentionally unchanged.
  • New or changed network calls? (Yes/No) Yes: authenticated file-transfer control/progress/input on existing Gateway and loopback MCP transports.
  • Command or tool execution surface changed? (Yes/No) Yes: verified fixed worker invocation and typed commands; local-only app.* remains local.
  • Data access scope changed? (Yes/No) Yes: file operations use existing Sandbox grants; browser/Canvas adapters share that boundary.
  • If any answer is Yes, explain the risk and mitigation: retained native identities, no-follow/hardlink checks, bounded streams/archive inventories, integrity checks, no-clobber rules and identity-owned cleanup constrain access. Worker-code/scratch rights stay distinct from caller data. Final queued admission revalidates approval. Signed worker bytes are hashed before embedding; official missing-signed-worker inputs fail closed. Positive unsupported-Windows host compatibility is explicit, not a fallback on supported-Windows errors. Passing owner regressions are not a blanket security acceptance claim.

Compatibility and Migration

  • Backward compatible? (Yes/No) Yes, intended for existing command/wire contracts, with browser/Canvas Sandbox-access tightening disclosed above. Current owner-level persistence and legacy-upload migration tests pass; installed-product acceptance remains unverified.
  • Config or environment changes? (Yes/No) Yes: SandboxMaxTransferBytes, SandboxUploadStagingPath, bounded executor-private cache and upload-root inventory. No separate file permission store/toggle or caller-controlled execution environment.
  • Migration needed? (Yes/No) No manual migration intended. Unknown/uncertain cache/upload state is not adopted or deleted by name alone. Existing valid legacy upload records migrate under ownership checks, with current-head passing cases.
  • If yes, list the exact upgrade steps: N/A. Install the matching-RID worker payload and configure existing Sandbox folder grants. Upload staging creation requires permission on its existing parent. Do not delete uncertain profile/cache state as an automatic repair.

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.

All three inline threads were verified resolved on GitHub. The older top-level ClawSweeper acceptance checklist is being re-reviewed, not silently treated as satisfied. The platinum rating is not self-assigned.

bkudiess and others added 7 commits October 30, 2026 15:28
Share system.run policy and execution ownership across eight typed file commands, Gateway and MCP transports, and winnode. Package a verified native worker with bounded identity-backed code caching and retained filesystem operations.

Native MXC, installed deployment, Gateway/MCP/UI and native ARM64 acceptance remain pending on a clean proof host. This commit is a validation candidate, not a shipping-readiness claim.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve bounded stdout, stderr, readiness, native exit/finalization and cleanup evidence even when a namespace child exits before READY. Keep native argv, containment policy and assertions unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compare PowerShell inline startup with the unchanged file-based namespace proofs using identical synthetic script and direct-SDK policy. Preserve original failures and assertions; do not override execution policy or fall back to host execution.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove unused forwarding methods, obsolete worker-path state, redundant overwrite state, and duplicate coverage. Separate failure-first and deterministically gated cancellation tests while preserving native ownership and rollback contracts.

Validated with the full build, required suites, and fresh x64 MCP and real Gateway functional proof. ARM64 and release-specific acceptance remain separate.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… fixtures

MxcFolderAdmission.ReadMetadata now passes extended-length (\\?\ or
\\?\UNC\) paths to CreateFileW so metadata reads succeed past MAX_PATH
when LongPathsEnabled is off. A long-path theory row covers this and
fails without the fix.

Test fixture corrections:
- Create resolver/cache hardlinks through extended-length paths.
- Set the copied private ACL owner to the current user so the unowned
  root test reaches its "root ownership changed" assertion.
- Run the FileWorker apphost cache test from a short temp root and fall
  back to it when RUNNER_TEMP is absent.

Document extended-length native paths, symlink privilege, and the
temp-root apphost fixture in TEST_COVERAGE.md.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The CreateHardLink test helper unconditionally prepended \\?\, so an
OPENCLAW_REPO_ROOT already spelled with an extended prefix produced
\\?\\\?\C:\... and failed fixture creation with Windows error 123
before reaching the alias-rejection assertion.

Keep existing extended paths, convert ordinary UNC paths to \\?\UNC\,
and prefix other paths after full-path normalization. Add a
hardlink-extended case that fails with error 123 before this change
and asserts the same "file aliases" rejection afterward.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper

clawsweeper Bot commented Nov 6, 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. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Nov 6, 2026
@clawsweeper

clawsweeper Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge.

What this changes

Adds eight Sandbox-governed file commands, authenticated transfers, a verified native worker, and transfer settings.

Example: Transfer C:\Data\sample.bin using winnode file.fetch with maxBytes 52428800.

  • Before: The Windows node does not provide this typed binary-transfer path.
  • After: The authenticated transfer uses Sandbox admission and verifies received size and SHA256 before publishing the local file.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The feature has substantial owner and transport coverage and repairs the previous blockers, but a concrete UI defect and outstanding production authorization acceptance prevent a pass; installed upgrade compatibility is also unproven.
Proof confidence 🧂 unranked krab (1/6) Real behavior proof is necessary before merge. See Before merge.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Product

Kind: Feature · Worth it: Yes · Fix scope: Complete
User problem: Windows-node callers lack typed file and directory transfers governed by the existing Sandbox settings.
Reason: The collaborator-authored direction provides a concrete capability through existing command owners and explicitly accepts the Browser/Canvas access tightening.

Merge readiness

⛔ Blocked before merge - 5 items remain

Keep this maintainer-authored draft open: the four previous blockers have source repairs, but the transfer-budget control can still display a different limit from the one enforced.

Priority: P2
Reviewed head: 3c477a787c9bf5b382ffee087e130b3192dec576

Before merge

  • Add real behavior proof - Authority-chain proof required: show allowed current-head Companion/MXC/Gateway file operations and rejection before final effects for a replaced MCP bearer, a queued command revoked through the real approval store, and a Gateway operator without admin authority invoking fs.listDir or terminal.upload. The complete body supplies owner/harness results at cd64009 and historical runtime summaries, while reviewed head 3c477a7 changes transfer and cleanup lifecycle. It explicitly leaves composed authorization, native ARM64, rendered rollback, and signed installed acceptance incomplete. Supply redacted terminal output or logs, removing credentials, IPs, and non-public endpoints. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.
  • Show the committed transfer budget after rounding (P2) - Entering 1.5 in TransferBudgetBox successfully saves 2 MiB because this expression rounds to whole MiB, but the successful path never updates sender.Value. The page therefore continues displaying 1.5 MiB while new transfers may consume 2 MiB. Apply the normalized committed value with change events suppressed after a successful save, or preserve the entered fractional limit. This concern was missed in the earlier review: comparison with eaa67e9 confirms the same rounding and successful-save behavior were already present.
  • Resolve security concern: Establish authorization at the final production effect - The supplied rotation and queued-revocation checks use synthetic capabilities or approval callbacks; they do not establish denial at native filesystem I/O or SDK launch, and Gateway non-admin rejection of fs.listDir and terminal.upload remains explicitly unverified.
  • Resolve merge risk - Before merge, the PR owner must establish that replaced MCP credentials, revoked queued approvals, and Gateway callers without admin authority cannot reach forbidden final filesystem effects through the composed production path.
  • Add data-model compatibility proof - The review found that existing stored data may not work after upgrade. Show that existing data still loads and works with this change.

Findings

  • [P2] Show the committed transfer budget after rounding — src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:386-389
  • [medium] Establish authorization at the final production effect — src/OpenClaw.Shared/Mxc/NodeSandboxExecutor.cs:200

Tests

  • Missing end-to-end proof: Current-head composed authorization, native ARM64, rendered budget state, signed installation/SAC, and latest-stable upgrade evidence remain incomplete; the fractional-budget failure also lacks a mounted regression.
Agent review details

How this fits together

Gateway and local MCP requests enter the shared capability registry, pass through committed Sandbox admission, and reach a verified worker that performs retained-handle filesystem operations and returns bounded results.

flowchart LR
 A[Gateway callers] --> C[Shared capability registry]
 B[Local MCP and winnode] --> C
 C --> D[Sandbox executor]
 E[Committed settings] --> D
 D --> F[Verified file worker]
 F --> G[Retained filesystem handles]
Loading

Technical review

Best possible solution:

Keep the shared Sandbox and filesystem owners, normalize the budget control to its committed value, and complete the scoped runtime acceptance gates.

Do we have a high-confidence way to reproduce the issue?

The budget mismatch is source-reproducible: entering 1.5 leaves the NumberBox unchanged after a successful save while Math.Round persists a 2 MiB limit; no target code was executed during this read-only review.

Is this the best way to solve the issue?

The shared executor, committed policy store, retained native handles, and separate infrastructure grants fit the existing ownership boundaries; the budget mismatch needs only control normalization, not another settings owner.

Full review comments:

  • [P2] Show the committed transfer budget after rounding — src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:386-389
    Entering 1.5 in TransferBudgetBox successfully saves 2 MiB because this expression rounds to whole MiB, but the successful path never updates sender.Value. The page therefore continues displaying 1.5 MiB while new transfers may consume 2 MiB. Apply the normalized committed value with change events suppressed after a successful save, or preserve the entered fractional limit. This concern was missed in the earlier review: comparison with eaa67e9 confirms the same rounding and successful-save behavior were already present.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b6f158ab257a.

Merge-risk options

Maintainer options:

  1. Complete authorization acceptance before landing (recommended)
    The PR owner supplies current-head allowed and nearest-forbidden production-path evidence, including bearer replacement and queued approval revocation before final effects.

Provenance checked

  • Sandbox execution and unsupported-Windows compatibility keeps the original intent (c6e0f6d: The SDK migration preserved approvals, bounded process ownership, explicit unsupported-Windows compatibility, and fail-closed containment errors.)
  • Browser and Canvas local file access changes intended behavior with a stated reason (9fa43f3: The existing Canvas path enforced temp-root containment and native handle validation; the PR explains its move to the shared Sandbox file boundary while retaining the additional temp-root restriction.)
  • Settings publication and writer origins keeps the original intent (f4e1cf3: The shared-state refactor established serialized field-scoped writes and origin-aware versioned notifications; committed snapshots preserve that contract without resampling concurrent rejected edits.)
  • MCP authentication keeps the original intent (9fa43f3: Bearer authentication and loopback/CSRF hardening protect local capability dispatch; the new immutable authentication generation extends that boundary to file leases.)
  • Gateway refusal and diagnostic semantics keeps the original intent (849cba5: Definite approval and execution-policy refusals must remain distinguishable from ambiguous execution failures; typed executor failures and file semantic outcomes preserve that distinction.)

Testing

Proof path: in-process harness.

Security

Needs attention: The prior authorization repairs are present, but the new authority chain still lacks composed forbidden-final-effect acceptance.

Evidence

Security concerns:

  • [medium] Establish authorization at the final production effect — src/OpenClaw.Shared/Mxc/NodeSandboxExecutor.cs:200
    The supplied rotation and queued-revocation checks use synthetic capabilities or approval callbacks; they do not establish denial at native filesystem I/O or SDK launch, and Gateway non-admin rejection of fs.listDir and terminal.upload remains explicitly unverified.
    Confidence: 0.99

What I checked:

  • Reviewed introduction and current-main boundary: The pinned introduction adds the file subsystem; fetched main lacks both FileTransfer and FileWorker trees and still registers the earlier capabilities. The central feature is not implemented there. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:327, 3c477a787c9b)
  • Previous authorization findings repaired: MCP publishes bearer and epoch together after channel reset; queued launches revalidate approval after dispatch resumes and before invoking the launch callback. (src/OpenClaw.Shared/Mxc/NodeSandboxPolicy.cs:89, 3c477a787c9b)
  • Previous packaging and disposal findings repaired: The workflow signs fixed workers before manifest generation, distribution jobs consume signed payloads, and NodeService disposes its shared executor after stopping producers. (src/OpenClaw.Tray.WinUI/Services/NodeService.cs:2584, 3c477a787c9b)
  • Transfer-budget mismatch: The NumberBox accepts a numeric value, but the save handler rounds it to whole MiB and restores the control only on failure; a successful 1.5 MiB edit therefore persists 2097152 bytes while leaving 1.5 displayed. (src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:386, 3c477a787c9b)
  • Late-finding verification: Comparison with the previous reviewed head confirms that the rounding expression and missing successful-save normalization were already present; the later change adds failure rollback only. (src/OpenClaw.Tray.WinUI/Pages/SandboxPage.xaml.cs:386, 3c477a787c9b)
  • Proof revision and acknowledged limits: The complete PR body distinguishes synthetic transport and native owner tests from composed runtime proof. Its linked successful run targets cd64009, preceding the reviewed lifecycle commit; authorization, native ARM64, rendered rollback, signed installation, and upgrade acceptance remain explicitly incomplete. (3c477a787c9b)

Likely related people:

  • Barbara Kudiess: Raw commit c6e0f6d adds src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs:11 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: c6e0f6d56242; files: src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs)
  • Scott Hanselman: Raw commit ff92351 adds src/OpenClaw.Shared/Capabilities/BrowserProxyCapability.cs:13 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: ff923514afa1; files: src/OpenClaw.Shared/Capabilities/BrowserProxyCapability.cs)
  • Chris Anderson: Raw commit 9fa43f3 adds src/OpenClaw.Shared/Mcp/McpHttpServer.cs:33 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 9fa43f347703; files: src/OpenClaw.Shared/Mcp/McpHttpServer.cs)

Preflight source

Source 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

Metric Value Why it matters
Production and test changes Production +6015/-668; tests +7172/-65 The growth spans native I/O, authenticated transports, packaging, persistence, and distinct lifecycle boundaries; the shared executor replaces the previous independent process path.

Labels

Label changes:

  • remove merge-risk: 🚨 automation: Current PR review merge-risk labels are merge-risk: 🚨 compatibility, merge-risk: 🚨 security-boundary.

Label justifications:

  • P2: This is a useful new transfer capability with a bounded settings-display defect, without evidence of an urgent released regression.
  • merge-risk: 🚨 compatibility: The change adds a public transport extension and persisted settings, changes worker installation, and intentionally tightens existing Browser/Canvas file access.
  • merge-risk: 🚨 security-boundary: New authenticated channels and retained filesystem authority cross into real reads, writes, execution, and cleanup; composed forbidden-final-effect acceptance remains unresolved.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rank-up moves

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

  • Add current-head final-effect proof for replaced MCP credentials, real queued approval revocation, and a Gateway caller without admin authority.

Rating scale

6/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.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-11-06T01:39:32.891Z sha eaa67e9 :: needs real behavior proof before merge. :: [P1] Publish the MCP bearer and file-channel generation atomically | [P1] Revalidate command approval after the launch queue drains | [P1] Integrate the worker with release signing before manifest generation | [P2] Dispose the shared Sandbox executor during service teardown

Reviewed November 7, 2026, 12:46 AM ET / 05:46 UTC (Revision 2).

bkudiess and others added 2 commits November 5, 2026 21:44
Set the owner explicitly in the creation-time security descriptor rather than relying on the token default owner. Preserve the protected User/System ACL and rejection of foreign-owned storage. Add independent owner-readback coverage and avoid redundant WRITE_OWNER requests in the cache fixture.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the file-transfer PR and current main histories while recording the
validated integration, authorization, executor teardown and worker-signing fixes.

Includes regression coverage and the optional-profile worker fixture repair.
Full native ARM64 build and Shared, Tray, CLI and FileWorker suites passed.
Live Gateway/MXC and signed-package acceptance remain unverified.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A save-notification race can publish uncommitted settings to observers, leaving them with rejected values after rollback and no corrective event.

3 open issues
What changed in this PR

This PR adds typed Windows file and directory transfers governed by the same saved Sandbox policy as system.run. It integrates file capabilities with Companion, Gateway/MCP transport, and winnode, while routing existing browser and Canvas file reads through the shared access boundary.

Changes:

  • Add file-transfer contracts, authenticated channels, worker execution, and upload ownership tracking.
  • Add transfer-budget and upload-storage settings, localized guidance, and committed-policy publication.
  • Package signed, manifest-verified workers for x64 and ARM64, with regression tests and protocol documentation.

Validation here was limited to source inspection; builds, tests, and native runtime proof were not run.

File Description
tests/​OpenClaw.WinNode.Cli.Tests/​SkillMdDriftTests.cs Includes file commands in documentation drift checks.
tests/​OpenClaw.WinNode.Cli.Tests/​OpenClaw.WinNode.Cli.Tests.csproj Builds the worker for CLI tests.
tests/​OpenClaw.Tray.Tests/​SettingsSandboxPublicationTests.cs Tests committed-policy publication and concurrent saves.
tests/​OpenClaw.Tray.Tests/​SandboxFileSettingsContractTests.cs Checks transfer-control placement and limits.
tests/​OpenClaw.Tray.Tests/​ReleaseSigningWorkflowTests.cs Checks worker signing and manifest ordering.
tests/​OpenClaw.Tray.Tests/​NodeModeUiStateTests.cs Updates shared-executor ownership assertions.
tests/​OpenClaw.Tray.Tests/​MxcCompanionPackagePathsTests.cs Checks executor context injection.
tests/​OpenClaw.Tray.Tests/​InstallerIssAssertionTests.cs Updates process-launch ownership guards.
tests/​OpenClaw.Tray.Tests/​FileWorkerManifestTests.cs Tests manifest validation and payload normalization.
tests/​OpenClaw.Tray.Tests/​CiNugetCacheWorkflowTests.cs Includes worker projects in cache checks.
tests/​OpenClaw.Tray.Tests/​AppRefactorContractTests.cs Checks executor teardown ordering.
tests/​OpenClaw.Shared.Tests/​OpenClaw.Shared.Tests.csproj Embeds worker manifest fixtures.
tests/​OpenClaw.Shared.Tests/​Mxc/​NodeSandboxPolicyTests.cs Tests policy revisions and launch admission.
tests/​OpenClaw.Shared.Tests/​Mxc/​MxcPhysicalPathsTests.cs Tests executor-private path exclusions.
tests/​OpenClaw.Shared.Tests/​Mxc/​MxcOwnedOperationTests.cs Tests identity-owned cleanup and retained operations.
tests/​OpenClaw.Shared.Tests/​Mxc/​MxcNamespaceLaunchTests.cs Tests file and inline PowerShell arguments.
tests/​OpenClaw.Shared.Tests/​Mxc/​MxcFolderAdmissionTests.cs Adds long-path metadata coverage.
tests/​OpenClaw.Shared.Tests/​Mxc/​MxcCommandRunnerTests.cs Adapts tests to shared executor ownership.
tests/​OpenClaw.Shared.Tests/​FileTransfer/​UploadRetentionCoordinatorTests.cs Tests upload inventory, recovery, and blocked cleanup.
tests/​OpenClaw.Shared.Tests/​FileTransfer/​Fixtures/​worker-manifest-x64.json Adds the x64 manifest fixture.
tests/​OpenClaw.Shared.Tests/​FileTransfer/​Fixtures/​worker-manifest-arm64.json Adds the ARM64 manifest fixture.
tests/​OpenClaw.Shared.Tests/​FileTransfer/​FileFrameChannelTests.cs Tests framing, bounds, ordering, and cancellation.
tests/​OpenClaw.Shared.Tests/​FileTransfer/​ExistingFileAdapterTests.cs Tests browser use of the Sandbox reader.
tests/​OpenClaw.Shared.Tests/​CapabilityTests.cs Injects the Canvas file-reader adapter.
tests/​OpenClaw.Shared.Tests/​A2UICapabilitySecurityTests.cs Tests missing-reader and Sandbox-denial behavior.
tests/​OpenClaw.Shared.TestHost/​Program.cs Adds worker-cache process fixtures.
tests/​OpenClaw.Shared.TestHost/​OpenClaw.Shared.TestHost.csproj Embeds cache manifest fixtures.
tests/​OpenClaw.Shared.TestHost/​Fixtures/​worker-cache-x64.json Adds the x64 cache fixture.
tests/​OpenClaw.Shared.TestHost/​Fixtures/​worker-cache-arm64.json Adds the ARM64 cache fixture.
tests/​OpenClaw.Shared.TestHost/​CacheManifestAssembly.cs Supplies manifests to process fixtures.
tests/​OpenClaw.SetupEngine.Tests/​SetupConfigTests.cs Checks file capability registration and ordering.
tests/​OpenClaw.FileWorker.Tests/​WorkerCacheProcessTests.cs Tests cache eviction with a running worker.
tests/​OpenClaw.FileWorker.Tests/​OpenClaw.FileWorker.Tests.csproj Defines the worker test project.
tests/​OpenClaw.E2ETests/​Setup/​MxcSetupAndConnectTests.cs Uses an explicit child executable path.
src/​OpenClaw.WinNode.Cli/​Program.cs Adds transfer flags and file-specific envelope limits.
src/​OpenClaw.Tray.WinUI/​Strings/​zh-tw/​Resources.resw Adds Traditional Chinese Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Strings/​zh-cn/​Resources.resw Adds Simplified Chinese Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Strings/​pt-br/​Resources.resw Adds Brazilian Portuguese Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Strings/​nl-nl/​Resources.resw Adds Dutch Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Strings/​fr-fr/​Resources.resw Adds French Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Strings/​en-us/​Resources.resw Adds English Sandbox transfer copy.
src/​OpenClaw.Tray.WinUI/​Services/​SettingsManager.cs Publishes saved policies and serializes persistence.
src/​OpenClaw.Tray.WinUI/​Services/​NodeService.cs Wires file capabilities, cleanup, and executor disposal.
src/​OpenClaw.Tray.WinUI/​Presentation/​SettingsStore.cs Tracks concurrent save origins without a callback lock.
src/​OpenClaw.Tray.WinUI/​Pages/​SandboxPage.xaml.cs Handles transfer settings and cleanup status.
src/​OpenClaw.Tray.WinUI/​Pages/​SandboxPage.xaml Adds storage, budget, and cleanup controls.
src/​OpenClaw.Tray.WinUI/​OpenClaw.Tray.WinUI.csproj Imports worker packaging targets.
src/​OpenClaw.Tray.WinUI/​FileWorker.Build.targets Builds, verifies, embeds, and copies worker payloads.
src/​OpenClaw.Shared/​SettingsData.cs Adds upload location and transfer budget settings.
src/​OpenClaw.Shared/​NodeCommandException.cs Defines a preservable command-owned exception.
src/​OpenClaw.Shared/​NodeCapabilities.cs Adds transport-owned channels and semantic results.
src/​OpenClaw.Shared/​Mxc/​UnsupportedWindowsCommandExecutor.cs Restricts the adapter to host start information.
src/​OpenClaw.Shared/​Mxc/​SandboxPathRules.cs Centralizes path normalization and specificity.
src/​OpenClaw.Shared/​Mxc/​NodeSandboxResources.cs Separates caller-data and infrastructure access.
src/​OpenClaw.Shared/​Mxc/​NodeSandboxPolicy.cs Orders policy persistence and launch admission.
src/​OpenClaw.Shared/​Mxc/​MxcRequestContext.cs Records executor-private runtime paths.
src/​OpenClaw.Shared/​Mxc/​MxcRequestBuilder.cs Composes shared data and infrastructure permissions.
src/​OpenClaw.Shared/​Mxc/​MxcProcessLifetime.cs Centralizes process and pipe lifetime handling.
src/​OpenClaw.Shared/​Mxc/​MxcFolderPermissions.cs Reuses shared permission specificity rules.
src/​OpenClaw.Shared/​Mxc/​MxcFolderAdmission.cs Uses extended native paths for metadata queries.
src/​OpenClaw.Shared/​Mxc/​FixedFileWorkload.cs Defines trusted worker leases and protocol callbacks.
src/​OpenClaw.Shared/​Mcp/​McpFileChannels.cs Adds authenticated file-channel leases and polling.
src/​OpenClaw.Shared/​FileTransfer/​IUploadRetentionOperations.cs Defines the upload cleanup scheduling boundary.
src/​OpenClaw.Shared/​FileTransfer/​IFileInvocationChannel.cs Defines bounded invocation messaging.
src/​OpenClaw.Shared/​FileTransfer/​GatewayFileInvocation.cs Coordinates Gateway frames and progress heartbeats.
src/​OpenClaw.Shared/​FileTransfer/​FileWorkerProtocol.cs Defines bounded binary worker packets.
src/​OpenClaw.Shared/​FileTransfer/​FileOutcomeTelemetry.cs Maps file execution mode to telemetry.
src/​OpenClaw.Shared/​FileTransfer/​FileOperations.cs Exchanges worker requests through owned pipes.
src/​OpenClaw.Shared/​FileTransfer/​FileContracts.cs Defines limits, permissions, requests, and results.
src/​OpenClaw.Shared/​FileTransfer/​FileCommandParser.cs Parses commands and validates base64 content.
src/​OpenClaw.Shared/​FileTransfer/​FileCapability.cs Exposes file commands and shapes outcomes.
src/​OpenClaw.Shared/​Capabilities/​CanvasCapability.cs Routes JSONL reads through the Sandbox adapter.
src/​OpenClaw.Shared/​Capabilities/​BrowserProxyCapability.cs Routes returned-file reads through the Sandbox adapter.
src/​OpenClaw.SetupEngine/​SetupContext.cs Advertises file capabilities independently of system tools.
src/​OpenClaw.FileWorker/​UploadOwnership.cs Defines native upload ownership metadata.
src/​OpenClaw.FileWorker/​Program.cs Implements the fixed worker’s standard-I/O entry point.
src/​OpenClaw.FileWorker/​OpenClaw.FileWorker.csproj Defines the architecture-specific worker build.
scripts/​Write-FileWorkerManifest.ps1 Validates payloads and generates digest manifests.
scripts/​Test-ReleaseExecutableSignatures.ps1 Classifies worker binaries for signature verification.
scripts/​test-ci-workflow-contract.ps1 Includes worker tests and restore inputs.
openclaw-windows-node.slnx Adds worker and worker-test projects.
docs/​RELEASING.md Documents signing before manifest generation.
docs/​MCP_MODE.md Documents authenticated file transport and bounds.
.github/​workflows/​ci.yml Adds worker testing, signing, and artifact distribution.
.agents/​skills/​winnode/​SKILL.md Documents file commands and binary-transfer flags.

Review effort: Balanced

Comment thread docs/MCP_MODE.md Resolved
bkudiess and others added 3 commits November 6, 2026 17:25
… budgets

Document repeated binary fetch chunks and acknowledgments.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve runtime-bound signed payload paths outside the source checkout for cleanup host and fixed workers.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bkudiess

bkudiess commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

Please reassess current head cd64009240995a6f1af1b4d0662a98053befad21. The previous scored review was on eaa67e95.

The PR body now links completed CI run 42326156137, attempt 1, its downloaded TRX artifacts, current-head native file-owner and authorization-boundary results, settings/legacy-upload compatibility checks, and signing-workflow regressions. CI merge da744ee3dcb7b314f78ea64b6fc1855ab1275740 has the exact PR-head tree ec026eae844a2101c9d3312d590fd4fb499e7533. The 18 inspected reports contain 15,912 passed result records, zero failures and 47 skips/not-executed records.

The old token-generation, queued-approval, worker-signing and executor-disposal findings now have repairs and passing regressions; all three later inline review threads are resolved. Please evaluate those fixes and the new evidence rather than the stale body.

The body explicitly distinguishes owner/synthetic transport tests from full native authority-chain proof. Current-head composed Gateway/MXC forbidden final effects, native ARM64 acceptance, rejected-budget pixels and latest-stable installed upgrade/SAC proof remain incomplete. No platinum label or completed proof pool is being asserted.

@clawsweeper

clawsweeper Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

Fix bounded Gateway transfer pipelining, worker deadlines, upload retention ownership and cleanup, and retained write target validation. Add regression coverage and update the related contracts.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. label Nov 7, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants