Codex review: needs maintainer review before merge. Reviewed July 29, 2026, 1:56 PM ET / 17:56 UTC.

ClawSweeper review

What this changes

This PR makes Windows node command execution use a V2 approval system that authorizes canonical argv, stores executable-path rules, prompts locally, and runs approved argv through local or MXC runners.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep this PR open for maintainer approval rather than automated cleanup. The latest head appears to address the prior concrete redaction and remote-policy findings, but it deliberately changes the external system.run contract and retires persisted V1 approvals, so the compatibility decision still needs an explicit owner. Likely related people: shanselman (recent security and compatibility reviewer, medium confidence) and AlexAlves87 (original V2 approval-dialog contributor, medium confidence).

Priority: P1
Reviewed head: 7b6240fec83f0a44aa1d0db7c8286161da59e9d1
Owner decision: Required. See Decision needed.

Review scores

Verification

How this fits together

Windows-node system.run accepts requests from the gateway, local MCP, CLI, and plugins, then applies a local approval policy before selecting a local or MXC execution runner. This PR replaces command-text policy matching with executable identity and canonical argv, affecting approval prompts, stored policy, and external callers of the node API.

flowchart LR
  A[Gateway, MCP, CLI, plugins] --> B[system.run canonical argv request]
  B --> C[V2 input and executable resolution]
  C --> D[Local approval policy and prompt]
  D --> E{Approved and still current?}
  E -->|No| F[Typed deny result]
  E -->|Yes| G[Local or MXC runner]
  G --> H[Command result and audit telemetry]

Decision needed

Why: The implementation intentionally avoids unsafe conversion and dual evaluators, but accepting the resulting upgrade behavior is a product and release-contract choice rather than a mechanical code-review decision.

Before merge

• Resolve merge risk (P1) - Existing raw MCP, direct node.invoke, plugin, and winnode integrations that send string-form commands or non-empty custom environment variables will fail until migrated to canonical argv.
• Resolve merge risk (P1) - Existing unattended installations with V1 exec-policy.json allow rules will stop executing those commands after upgrade; V1 rules remain untouched but are intentionally no longer evaluated.
• Resolve merge risk (P1) - This is a large authorization-boundary rewrite across 81 files, so release acceptance should include explicit confirmation that the documented breaking upgrade behavior is intended.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Review in progress
• Detail: The exact-review queue leased this run; Codex is reviewing the item.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/30244414687
• Updated: 2026-07-27T06:57:54.266Z

@clawsweeper re-review

I am holding this below the repository's 90% merge-confidence bar after independent Opus and Codex reviews of the current head c4440674. Both reviewers confirmed the core V2 authorization path is fail-closed, but three unresolved release/security-contract issues remain:

1. String-form system.run compatibility is removed. Existing external callers using a string command (or safe custom env) now receive a hard error. Please preserve a boundary compatibility shim or document and gate the versioned contract break with migration guidance.
2. Persisted V1 allow/deny policy is ignored on upgrade. Existing unattended allows stop working, and prior explicit denies lose their configured intent. Since V1 text rules cannot safely become V2 resolved identities, please add a visible reapproval/migration notice or obtain explicit product sign-off for the breaking upgrade behavior.
3. Remote system.execApprovals.set can weaken prompt semantics for existing grants. ValidateExecApprovalsDelta prevents new allowlist patterns but allows ask: always to become on-miss/off with the same patterns, converting confirm-every-time entries into unattended execution. Please enforce immutable-or-more-restrictive ask, askFallback, and security changes and add focused tests.

The first two may be deliberate product decisions, but they need explicit release handling. The third is a concrete authorization-policy gap that should be fixed before merge.

@shanselman Thanks for the review. The concrete remote-policy weakening gap is fixed on the updated head:

• system.execApprovals.set now compares effective defaults, wildcard, and every agent scope.
• Remote updates may preserve or tighten security, ask, askFallback, and autoAllowSkills, but cannot weaken them.
• Existing locally-established Full values can round-trip while another axis tightens; lower policies cannot introduce Full.
• Focused monotonicity tests cover all four weakening axes, tighter updates, lower-to-Full rejection, and Full-preserving tightening.

Explicit product/release decision for the other two items:

1. Low-level system.run is intentionally V2 argv-only. The normal gateway exec host=node flow already emits canonical Windows argv. Raw MCP, direct node.invoke, plugin, and winnode callers must migrate from string+shell to explicit argv. Runtime now returns command-array-required, and README, architecture docs, and winnode skill include exact before/after migration examples. Non-empty custom env remains intentionally unsupported until it can be identity-bound and displayed safely.
2. Windows V1 policy is intentionally retired without migration. V1 command-text rules remain byte-for-byte untouched but are not evaluated or converted. Fresh valid V2 uses empty allowlist + prompt-on-miss + unattended deny. Upgrade characterization covers V1 allow, deny, malformed, valid-V2 precedence, and invalid-V2 hard deny. This break is accepted to avoid unsafe semantic conversion and parallel authorization engines, matching the macOS cutover approach.

Current validation: build green; Shared 3,249 passed / 32 skipped; Tray 1,954; CLI 126; SetupEngine 697; Tray Integration 19; MXC E2E 2/2 with true contained Access denied proof. Opus and Sol found no remaining actionable issues at 95% / 96% confidence.

Please reconsider the merge-confidence gate on the updated head.

@clawsweeper re-review

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

I re-reviewed current head b6a83b70 against current main. The earlier remote monotonicity issue is fixed: the defaults, wildcard, and union of agent scopes are compared across security, ask, askFallback, and autoAllowSkills, and the focused tests cover the intended transitions. The argv-only and V1 retirement decisions are now explicit and documented.

Two new blockers remain:

1. Undefined numeric enum values can bypass remote policy monotonicity and runtime evaluation. ExecApprovalsStore.JsonOptions uses JsonStringEnumConverter with its default allowIntegerValues: true, and neither deserialization nor ValidateExecApprovalsDelta checks Enum.IsDefined. A remote replacement containing values such as security: -1 and ask: 99 passes the ordering checks as if it were more restrictive. At runtime those values match none of the Deny, Allowlist, Always, or OnMiss branches in ExecApprovalEvaluator, so evaluation falls through to Allow. Please reject integer enum input and explicitly validate every policy enum in defaults, wildcard, and agent scopes, with adversarial tests through system.execApprovals.set.

2. The approval redactor can hide arbitrary command payloads from the user. The PEM pattern matches an unbounded attacker-controlled span between BEGIN ... PRIVATE KEY and END ... PRIVATE KEY, and RedactPemBlock replaces the entire matched span. An agent can put destructive shell text between fake markers so the approval dialog shows only *** or the marker lines plus …redacted…. The prompt correctly denies truncated/oversized commands because hidden text is not reviewable, but it does not deny or visibly flag redacted commands. Please make redaction status explicit and fail closed or present an unavoidable warning, and constrain the PEM matcher so it cannot serve as a general-purpose hide-text primitive.

Validation on the current-main merged state:

• ./build.ps1: passed
• Shared: 3,249 passed, 32 skipped
• Tray: 1,955 passed
• WinNode CLI: 126 passed
• SetupEngine: 697 passed
• Tray Integration: 19 skipped because its live-app gate was not enabled

Recommendation: hold until both blockers have focused regression tests. Confidence: 96% on the numeric bypass and 93% on the redaction exploit path.

Re-review at 3f44199e: the two original blockers are fixed. Numeric enum policy values now fail closed, and fake PEM markers can no longer conceal arbitrary shell text in an approvable dialog.

I am still holding this head because the replacement PEM pattern introduces two verified secret-display regressions:

1. Escaped-newline PEM values are no longer redacted. echo -----BEGIN PRIVATE KEY-----\nQUJDREVGR0hJSktMTU5PUA==\n-----END PRIVATE KEY----- returns Redacted=false and preserves the full payload.
2. A valid PEM block whose final base64 line is short and padded is no longer redacted. A body ending with Ag== returns Redacted=false and preserves the full payload.

The pattern currently requires literal line breaks and at least four non-padding base64 characters on every line. It should accept literal or escaped line separators where those serialized forms are supported, and allow a valid short padded final line. Please add regressions for both cases.

There is also a concrete behavior regression in the new Redacted => Deny gate: heuristic matches deny before showing the dialog even when no secret value is present. For example, both ...?key=$GOOGLE_KEY and GitHub search ...?code=abc return Redacted=true, so the owner cannot approve them. The concealment fix needs a narrower status than "any masking occurred", or at minimum a distinct actionable denial result. Do not remove the deny gate without replacing its concealment invariant.

Focused exec-approval tests pass: 273/273. Independent Codex review found the original blockers closed; Opus found the cases above, and I reproduced all four inputs against the current-main merge. Hold confidence: 95%.