Skip to content

feat: add structural input validation for system.run exec approvals#244

Merged
shanselman merged 7 commits intoopenclaw:masterfrom
AlexAlves87:feat/system-run-input-validation
May 1, 2026
Merged

feat: add structural input validation for system.run exec approvals#244
shanselman merged 7 commits intoopenclaw:masterfrom
AlexAlves87:feat/system-run-input-validation

Conversation

@AlexAlves87
Copy link
Copy Markdown
Contributor

I have the next small, fully tested slice prepared for the exec approvals work. In practice, it adds structural input validation for system.run, without introducing executable resolution, policy evaluation, prompting, or any production activation.

The goal here is to keep moving this area forward through small, well-scoped changes that can be reviewed and merged independently while keeping the current legacy path unchanged by default.

What this PR includes

  • a standalone ExecApprovalV2InputValidator
  • a typed intermediate ValidatedRunRequest
  • typed validation-failed outcomes for malformed or invalid input
  • structural validation for:
    • missing or empty command input
    • malformed command/args payloads
    • malformed cwd
    • malformed env
    • invalid timeout values
  • test coverage for valid, invalid, malformed, and edge-case inputs

What does not change

  • system.run still uses the current legacy path by default
  • this PR does not add executable resolution
  • this PR does not add canonical command identity
  • this PR does not add policy evaluation or prompting
  • this PR does not wire the validator into production behavior yet

Validation

  • ./build.ps1
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore

@shanselman
Copy link
Copy Markdown
Contributor

Thanks Alex — I took a look through this slice and the direction looks good to me. Keeping this as structural validation only, with no production wiring yet, feels like the right incremental step for the exec approvals v2 work.

One thing before merge: could you please rebase or merge current master into this branch? The PR head is a bit stale: it still has the older global.json SDK roll-forward behavior, and current master also has the V2 exec-approval result types this validator builds against. When I tested the branch with current master merged locally, the validator slice looked clean and validation passed in that merge context.

No code-level blocker from me after that refresh — this seems like a solid first slice to build on.

@AlexAlves87 AlexAlves87 force-pushed the feat/system-run-input-validation branch from 10abdfc to b0aa479 Compare April 30, 2026 08:05
@AlexAlves87
Copy link
Copy Markdown
Contributor Author

Hi @shanselman — thanks for the review and the feedback. I've rebased the branch on current master, so it should be up to date now.

AlexAlves87 and others added 2 commits April 30, 2026 23:05
Adds ExecApprovalV2InputValidator (phase 1 of the V2 exec approval
pipeline) and its typed output ValidatedRunRequest. Validates argv,
cwd, env, and timeout with fail-closed typed denials; no resolution,
policy evaluation, or production wiring.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
21 fork-internal entries (CLAUDE.md, AGENTS.md, .serena/, .claude/, etc.)
accumulated during rebase and do not belong in upstream PR openclaw#244.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AlexAlves87
Copy link
Copy Markdown
Contributor Author

Heads up — I'm force-pushing this branch after rebasing onto current master.

During the rebase/merge context, the branch picked up unrelated internal .gitignore entries from my local fork workflow. Those entries were not part of this PR, and they were the same kind of .gitignore noise that also affected #251.

This update removes that spillover so the diff stays limited to the system.run structural input-validation slice.

@AlexAlves87 AlexAlves87 force-pushed the feat/system-run-input-validation branch from b0aa479 to 5cfbde9 Compare May 1, 2026 17:38
@shanselman
Copy link
Copy Markdown
Contributor

Thanks again, @AlexAlves87. I took another pass now that the recent exec-approval hardening work has landed on master. I still like this as the next small V2 slice: it is isolated, tested, and keeps production system.run behavior unchanged for now.

Before merging, I’d like to tighten a few validator semantics since this will become foundational for later execution/policy phases:

  1. Preserve argv argument text exactly. Please don’t trim argv elements beyond using argv[0] to decide whether the executable is empty/whitespace. Arguments like " value " can be meaningful once this is wired into execution/policy logic.
  2. If command is a string and an args property is present but not an array, return a validation failure instead of silently ignoring it.
  3. Please either clamp max timeout here to match the legacy safety bound, or add a comment/test making clear that timeout bounding happens in a later V2 phase.

The PR merges cleanly on current master, and I verified the added test class locally with:

dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore --filter ExecApprovalV2InputValidationTests

Result: 49/49 passing.

With those small changes, I think this is a good slice to take.

…meout bound deferral

- Remove .Trim() from argv[1+] elements (array and separate-args paths); arguments
  like "  value  " must reach the execution/policy phase unchanged.
- argv[0] empty/whitespace detection now uses IsNullOrWhiteSpace instead of trim+IsNullOrEmpty.
- A non-array "args" property when command is a string is now a protocol violation
  (malformed-command) instead of being silently ignored.
- Add comment in timeout validation making explicit that upper-bound clamping is
  enforced in the execution/policy phase, not here.
- Update and add tests: 52 passing (3 new: SeparateArgs_NotAnArray_*, LargeTimeout_*).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@AlexAlves87
Copy link
Copy Markdown
Contributor Author

Hi @shanselman — thanks for the second pass, I've addressed all three:

  1. Removed .Trim() from all argv storage paths.
  2. args present but not an array → validation failure.
  3. Added a comment and a test (LargeTimeout_PassesStructuralValidation) making explicit that timeout clamping belongs to the execution phase.

Tests: 49 → 52.

@shanselman
Copy link
Copy Markdown
Contributor

Thanks @AlexAlves87 — this looks good now. I verified the updated branch and also tested the merge result over current master locally. The coverage is exactly what I wanted for this slice: structural validator success/failure paths, argv preservation, malformed args rejection, and timeout boundary semantics while keeping production system.run behavior unchanged. Merging this as the next exec-approvals V2 foundation piece.

@shanselman shanselman merged commit 4966186 into openclaw:master May 1, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants