feat: replace legacy exec approvals with V2#1051
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Codex review: found issues before merge. Reviewed July 24, 2026, 2:47 PM ET / 18:47 UTC. ClawSweeper reviewWhat this changesThe PR replaces Windows node legacy command approvals with a V2 coordinator, persistence store, hardened local dialog, canonical argv-only Merge readiness⛔ Blocked by patch quality or review findings - 8 items remain Keep this collaborator-authored PR open. The V2 execution path and its stated MXC/MCP proof are substantial, but the branch removes the legacy approval policy without a demonstrated upgrade path for existing nodes, so a previously working Priority: P1 Review scores
Verification
How this fits togetherGateway and local MCP clients invoke flowchart LR
A[Gateway or local MCP call] --> B[Windows node tool bridge]
B --> C[V2 approval coordinator]
C --> D[Policy store and local dialog]
C --> E[Approved canonical argv]
E --> F[Local runner or MXC sandbox]
F --> G[Command result and diagnostics]
Decision needed
Why: The policy formats authorize different things, full command text versus resolved executable paths, so code review cannot safely choose a migration semantic or an intentional breaking security boundary without maintainer direction. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Define and implement an explicit V1-to-V2 upgrade contract: preserve the old policy until the owner completes a reviewed conversion, or provide a narrow backed-up migration that fails closed and is covered by fresh-install and upgrade tests. Do we have a high-confidence way to reproduce the issue? Yes, from source: start with an existing node whose Is this the best way to solve the issue? No. Replacing V1 with V2 may be the intended architecture, but removing the old runtime path without an explicit migration or retained compatibility contract is not the safest solution for existing operators. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against d2638ddbef57. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: feat: replace legacy exec approvals with V2 This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Summary
system.runandsystem.run.preparecanonical argv-only, matching the upstream gateway contract.cmd.exe /d /s /c.Architecture ownership
SystemCapability,ExecApprovalPolicy, andExecApprovalPromptServicesplit approval text matching, prompting, and shell execution across parallel paths.ExecApprovalsCoordinatorowns authorization;ExecApprovalsStoreowns policy persistence/CAS;ExecApprovalV2UiPromptHandlerandExecApprovalDialogown presentation; runners receive only the approved execution payload.Behavior changes
commandasstring[]argv. The normal gatewayexec host=nodepath already wraps Windows command strings as["cmd.exe","/d","/s","/c",command].security=allowlist,ask=on-miss,askFallback=deny, and an empty allowlist. Only truly missing trustworthy state gets this default; invalid or unreadable active/legacy paths fail closed tosecurity=deny.envis rejected until environment values can be identity-bound and displayed safely.system.execApprovals.setmay preserve or remove existing grants, but cannot add/change grants or set full access.%APPDATA%\OpenClawTray\exec-approvals.json;OPENCLAW_STATE_DIRoverrides it.Upgrade contract
Windows V1
exec-policy.jsoncommand-text globs are intentionally retired without migration.Mechanical conversion and V1 runtime fallback were rejected because V1 authorized shell command text while V2 binds resolved executable identity and canonical argv. Conversion could widen narrow text rules into broad command-host grants, while dual evaluators would preserve parallel authorization paths.
Validation
.\build.ps1dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.WinNode.Cli.Tests\OpenClaw.WinNode.Cli.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.SetupEngine.Tests\OpenClaw.SetupEngine.Tests.csproj --no-restoredotnet test .\tests\OpenClaw.Tray.IntegrationTests\OpenClaw.Tray.IntegrationTests.csproj --no-restore.\scripts\validate-mxc-e2e.ps1Real behavior proof
Gateway to Windows node to MXC
Canonical argv:
Observed:
The companion execution proof returned the expected marker with exit code 0 through the same contained path.
Local MCP
winnode --list-toolsadvertised:Canonical invocation returned:
{ "stdout": "OPENCLAW_LOCAL_MCP_CANONICAL_ARGV_OK", "stderr": "", "exitCode": 0, "timedOut": false, "success": true }Runtime logs confirmed
path=v2,decision=Allow,contained=True, andcontainment=mxc.Upgrade characterization
Automated upgrade tests cover:
Adversarial review
Claude Opus 4.8 and GPT-5.6 Sol independently reviewed the complete diff through three rounds with cross-model challenge and re-review after fixes.