Execute approved exec-approval commands as a resolved, shell-free argv by AlexAlves87 · Pull Request #799 · openclaw/openclaw-windows-node

AlexAlves87 · 2026-06-21T13:37:54Z

Summary

The exec-approvals V2 path could decide to allow a command but had no way to run
it: the allow result carried no payload, and the local runner could only execute
through a shell that re-tokenizes the command. This change makes an approved
command executable exactly as it was evaluated.

The allow result now carries the approved execution payload: the validated
argument vector with its executable resolved to an absolute path, the sanitized
environment, the working directory, and the timeout. argv[0] is the resolved
path, never the raw command token, so the executable cannot be re-resolved
against PATH or the working directory at execution time. If the executable
cannot be resolved, the allow fails closed.
LocalCommandRunner gains a direct-argv mode: when a request carries an argv,
the process is launched with ProcessStartInfo.ArgumentList and
UseShellExecute=false, so the approved arguments reach the process verbatim
with no shell re-parsing them. The legacy shell-wrapped path is unchanged when
no argv is supplied.
When the approved command uses a transparent env wrapper (e.g. ["env", "git", "status"]), the payload is built from the effective argv the resolver evaluated,
not the original request tail, so the approved and executed command identity
match exactly.

Safety

The payload builder fails closed before any approval state is written, so a
command that cannot be represented faithfully never leaves an allowlist entry
or usage record behind. It rejects:

An unresolved executable (no absolute path to pin the command identity to).
A batch script (.bat/.cmd), which needs cmd.exe and would re-parse the arguments.
An env wrapper carrying modifiers (VAR=val assignments or flags such as -i
or --unset), whose environment semantics a direct-argv payload cannot carry,
including nested forms such as env env FOO=bar node, whose modifier the resolver
would otherwise strip. Transparent env wrappers (including nested) are unwrapped safely.

The direct-argv runner enforces the same guards independently:

An empty argv, a non-absolute executable (which Windows would otherwise guess
from PATH/cwd), or a batch script are rejected rather than degraded to a shell.
A non-null but empty argv is treated as invalid, never as a request to fall
back to the shell.
Argument values are not written to logs in direct mode; only the executable
name and the argument count are logged.
The payload takes defensive copies of argv and env so it cannot be mutated
between approval and execution.

The sandboxed runner has no argv transport yet, so it fails closed rather than
degrade silently: when sandboxing is available and enabled and a request carries
a direct argv, it is blocked instead of being serialized as the legacy command
fields. When sandboxing is unavailable or disabled, the request routes to the
host runner, which does honor argv.

Scope

This adds the capability to execute an approved command faithfully. It does not
enable the new path in production: wiring the coordinator behind its feature flag,
and a faithful argv transport for the sandboxed runner, are separate changes. The
new path stays unreachable until those land, and the sandbox fails closed in the
interim.

Proof

A standalone program approves a command carrying deliberately hostile arguments
(spaces, an empty string, an embedded quote, a trailing backslash, shell
metacharacters, a tab, and Unicode with an emoji), runs it through the real
coordinator and the real runner, and has the launched child process dump the argv
it actually received. Each argument is shown with its character length and its
exact UTF-8 bytes in hex, so a space, a tab, and an empty string are
distinguishable rather than ambiguous on screen. No pass/fail verdict is printed
by the program: the three blocks are compared by reading the bytes.

APPROVED — what was handed to the coordinator.
PAYLOAD — what the coordinator emitted (argv[0] resolved to an absolute path,
arguments unchanged).
RECEIVED — what the child process got, dumped by the child itself.

APPROVED and RECEIVED match byte for byte; PAYLOAD is the same arguments with the
resolved executable as argv[0].

Proof output below:

Testing

Local validation after latest changes:

./build.ps1 (full solution build) — succeeded, 0 errors, 0 warnings
dotnet test OpenClaw.Shared.Tests  — 2333 passing, 0 failed
dotnet test OpenClaw.Tray.Tests    — 1088 passing, 0 failed

Coverage includes: launch-planning decision (direct-argv vs legacy, verbatim
argument preservation, fail-closed guards), payload builder (resolved path becomes
argv[0]; unresolved executable, batch script, and modified env wrapper each produce
no payload), empty-argv guard, sanitized environment in payload, end-to-end
coordinator→runner handoff without shell, regression for transparent env wrapper,
no allowlist persistence when the payload fails closed, and the sandboxed runner
failing closed on a direct-argv request while the host fallback still honors argv.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

Exec-approvals must run an approved command exactly as it was evaluated, with no shell between the policy decision and process creation. The local runner only had the shell-wrapped path, which re-tokenizes the command with its own quoting. CommandRequest.Argv (optional) selects direct mode: FileName = Argv[0] and the remaining arguments go through ProcessStartInfo.ArgumentList with UseShellExecute=false, so they reach the process verbatim. The decision is extracted into PlanExecution (internal) so it is unit-testable without spawning a process; a null Argv leaves the legacy shell-wrapped path unchanged. Direct mode fails closed on a malformed payload: an empty argv, a non-absolute executable (which Windows would otherwise guess from PATH/cwd), or a batch script (.bat/.cmd require cmd.exe, which re-parses arguments) are rejected rather than degraded to a shell. An invalid argv returns an error result instead of crashing the host. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…esult An allow decision now carries the command to execute: the validated argument vector, the sanitized environment, the working directory, and the timeout. argv[0] is the executable resolved to an absolute path, never the raw command token, so it cannot be re-resolved against PATH or the working directory at execution time. If the executable cannot be resolved, the allow fails closed. The payload takes defensive copies of argv and env and rejects an empty argv. The result type enforces the invariant that an allow always carries a payload and a deny never does. An end-to-end test confirms the payload emitted on allow is directly executable by the runner with no shell. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

clawsweeper · 2026-06-21T13:38:38Z

Codex review: needs maintainer review before merge. Reviewed June 21, 2026, 2:52 PM ET / 18:52 UTC.

Summary
The branch adds an approved execution payload to exec-approvals V2, direct-argv launching in the local runner, sandbox fail-closed handling for direct argv, and shared regression tests.

Reproducibility: not applicable. as a PR feature/security-hardening review rather than a standalone bug report. The relevant check is source review plus the provided runtime screenshot showing approved, payload, and received argv bytes.

Review metrics: 2 noteworthy metrics.

Changed Surface: 9 files, +706/-19. The diff is bounded but spans shared approval results, local execution, sandbox execution, and regression tests.
Execution Boundaries: 2 runners changed. Both the host runner and MXC sandbox runner define whether an approved command executes with the reviewed argv identity.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster ✨ media proof bonus
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P2] Have a maintainer explicitly review the command execution boundary before merge.
Keep production V2 routing and sandbox argv transport as separate reviewed follow-ups.

Risk before merge

[P1] Merging changes the security-sensitive approval-to-process boundary, so argv identity, environment semantics, and fail-closed behavior need maintainer review beyond green CI.
[P1] The production V2 coordinator handoff and faithful sandbox argv transport remain separate follow-ups; maintainers should keep this staged path unwired until those contracts are reviewed.

Maintainer options:

Land Staged Direct-Argv Support (recommended)
Merge after maintainer security review confirms the payload contract remains staged and the sandbox fail-closed behavior is intentional until argv transport exists.
Hold For Sandbox Transport
Pause this PR if maintainers want no direct-argv payload API to land until the MXC sandbox protocol can carry argv faithfully end to end.

Next step before merge

No automated repair remains; the next action is maintainer security review of the staged command execution boundary.

Security
Cleared: No concrete security regression was found after the nested-env, batch-script, side-effect-ordering, and sandbox fail-closed fixes, but the PR remains security-boundary-sensitive.

Review details

Best possible solution:

Land the staged direct-argv payload and host-runner support after maintainer security review confirms the boundary, while keeping production V2 routing and sandbox argv transport as explicit follow-up work.

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

Not applicable as a PR feature/security-hardening review rather than a standalone bug report. The relevant check is source review plus the provided runtime screenshot showing approved, payload, and received argv bytes.

Is this the best way to solve the issue?

Yes. Carrying a resolved argv payload and executing it via ProcessStartInfo.ArgumentList is the narrowest maintainable way to avoid shell re-tokenization, with sandboxed direct argv failing closed until transport support exists.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5d1c65bb6ff8.

Label changes

Label justifications:

P2: This is bounded command-execution hardening with meaningful security-review importance but no confirmed production outage because the V2 handoff remains staged.
merge-risk: 🚨 security-boundary: The PR changes how approved commands are represented and carried into process execution, where argv identity and sandbox fail-closed behavior must remain exact.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (screenshot): The PR body includes inspected screenshot proof from a real Windows PowerShell run showing approved, emitted, and child-received argv byte dumps after the change.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes inspected screenshot proof from a real Windows PowerShell run showing approved, emitted, and child-received argv byte dumps after the change.
proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The PR body includes inspected screenshot proof from a real Windows PowerShell run showing approved, emitted, and child-received argv byte dumps after the change.

Evidence reviewed

What I checked:

AGENTS.md policy read: Repository policy was found and fully read; this read-only review applied its validation and command-execution sensitivity guidance without running write-producing validation commands. (AGENTS.md:1, 5d1c65bb6ff8)
Current main lacks the requested direct argv execution path: On current main, SystemCapability still returns an error for any V2 handler result and LocalCommandRunner only builds a shell-wrapped FileName/Arguments pair, so the PR is not obsolete on main. (src/OpenClaw.Shared/Capabilities/SystemCapability.cs:318, 5d1c65bb6ff8)
PR head carries allow execution payloads: ExecApprovedExecution carries defensive-copy argv/env payloads, and Allow now requires a payload instead of a parameterless allow result. (src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs:15, 2539e923c94b)
PR head launches direct argv without a shell: LocalCommandRunner plans non-null CommandRequest.Argv as direct mode, validates argv[0], and passes argv[1..] through ProcessStartInfo.ArgumentList without setting shell arguments. (src/OpenClaw.Shared/LocalCommandRunner.cs:52, 2539e923c94b)
Sandbox path fails closed for unsupported direct argv: MxcCommandRunner rejects direct-argv requests before legacy sandbox serialization when sandboxing is enabled and available, while preserving host fallback when sandboxing is unavailable or disabled. (src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs:74, 2539e923c94b)
Regression coverage covers payload handoff and env wrappers: Tests cover resolved argv payloads, coordinator-to-runner handoff, transparent nested env wrappers, modified env wrapper fail-closed behavior, and sandbox direct-argv blocking. (tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs:456, 2539e923c94b)

Likely related people:

AlexAlves87: Current-main history shows prior merged work adding ExecApprovalsCoordinator and later persistence behavior in the coordinator/result area this PR extends. (role: exec-approvals feature contributor; confidence: high; commits: 12416d282a23, cfca9f59c6fc; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2Result.cs, tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs)
Scott Hanselman: Current-main history introduced system.run, ICommandRunner, LocalCommandRunner, and later shell quoting hardening that the new direct-argv path changes. (role: system.run and local runner feature owner; confidence: high; commits: 4003e58d8f92, 8c1bd38f3fe2; files: src/OpenClaw.Shared/LocalCommandRunner.cs, src/OpenClaw.Shared/ICommandRunner.cs, src/OpenClaw.Shared/Capabilities/SystemCapability.cs)
Barbara Kudiess: Current-main history introduced AppContainer sandboxing for system.run, which is the sandbox boundary this PR intentionally fails closed for direct argv. (role: MXC sandbox feature owner; confidence: high; commits: 62533e2901bd; files: src/OpenClaw.Shared/Mxc/MxcCommandRunner.cs, src/OpenClaw.Shared/Mxc/MxcConfigBuilder.cs, tests/OpenClaw.Shared.Tests/Mxc/MxcCommandRunnerTests.cs)
Vincent Koc: Recent current-main work migrated exec-approvals state directory handling near the coordinator/store behavior touched by this payload ordering. (role: recent adjacent exec-approvals contributor; confidence: medium; commits: 913ba4e8f504; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalsStore.cs, tests/OpenClaw.Shared.Tests/ExecApprovalsCoordinatorTests.cs)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

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.

… tail When a transparent env wrapper prefixes a command (e.g. ["env", "git", "status"]), the resolver strips it and resolves the inner executable. BuildApprovedExecution was copying identity.Command[1..] — the original request tail — so the payload executed a different argv than the one that was approved and evaluated. Now uses ExecEnvInvocationUnwrapper.UnwrapForResolution to derive the same effective argv the resolver used, replacing only argv[0] with the resolved absolute path. Regression test added for the env-wrapper case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

AlexAlves87 · 2026-06-21T14:36:11Z

@clawsweeper re-review

clawsweeper · 2026-06-21T14:36:14Z

🦞🧹
ClawSweeper re-review requested.