feat(policy): add simulate command to dry-run policy against execution traces - #6269
feat(policy): add simulate command to dry-run policy against execution traces#6269sauravdev wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a policy simulation library, a sandbox orchestration layer that loads traces and presets, and a new ChangesSandbox policy simulation tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as PolicySimulateCommand
participant Host as simulateSandboxPolicy()
participant Registry as Sandbox Registry
participant Sim as simulate()
User->>CLI: sandbox:policy:simulate <sandboxName> --from-file ...
CLI->>CLI: readStdin() if from-file is "-"
CLI->>Host: simulateSandboxPolicy(options)
alt policyFile provided
Host->>Host: loadPolicyFile(policyFile)
else
Host->>Registry: get active preset names
Host->>Host: load each active preset YAML
end
Host->>Sim: simulate(requests, presets)
Sim-->>Host: SimulationSummary
Host-->>CLI: SimulatePolicyResult
CLI->>CLI: renderSimulationReport(summary, json)
CLI-->>User: report + exit code
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/inference/custom-llm-provider.md`:
- Around line 272-307: The configuration reference is too narrow compared with
the actual onboarding schema. Update the Config File Reference in the custom LLM
provider docs to either include the full set of fields handled by
src/onboard/config.ts and src/onboard/config.test.ts, such as provider,
providerLabel, ncpPartner, and onboardedAt, plus the supported provider values
openai, anthropic, and gemini, or explicitly label this section as only covering
the custom-provider subset. Keep the table and example aligned with the real
config shape so ~/.nemoclaw/config.json matches the documented schema.
- Around line 334-338: The Related Topics links still use .md file paths instead
of the published route-style docs links. Update the markdown in the
custom-llm-provider documentation so the links for Switch Inference Models at
Runtime, Inference Profiles Reference, and Network Policy — Approve Network
Requests use route-style targets without file extensions, following the same
convention used across the docs.
In `@docs/inference/switch-to-nemotron-super-120b.md`:
- Around line 159-172: Update the Nemotron 3 Super 120B examples to use the
fully-qualified OpenClaw catalog key instead of the bare model ID. In the
affected snippets in switch-to-nemotron-super-120b.md, change the model
references used by the config generation and the openclaw models set example so
they consistently point to openai/nvidia/nemotron-3-super-120b-a12b. Keep the
rest of the example unchanged and make sure the documented primary model value
matches the catalog entry exactly.
In `@src/commands/sandbox/policy/simulate.ts`:
- Line 121: Move the active-policy loading orchestration out of simulate.ts so
the command class stays a thin adapter. Refactor loadActivePresetsForSandbox and
the SandboxPolicySimulateCommand flow so registry.getSandbox and the
filesystem/YAML lookup happen in a dedicated action under src/lib/actions with
the registry/fs passed in as injectable dependencies, and have the command only
parse argv and delegate typed inputs to that action. Update any call sites in
the command to use the new action boundary while preserving the existing
preset-loading behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f3a7e5a-e49f-43a7-96e6-4afce52dd597
📒 Files selected for processing (13)
docs/inference/custom-llm-provider.mddocs/inference/switch-to-brev-nemotron-120b.mddocs/inference/switch-to-nemotron-super-120b.mdnemoclaw-sandbox-policy.yamlskill/nat/SKILL.mdskill/nat/references/a2a-server.mdskill/nat/references/custom-tools.mdskill/nat/references/examples.mdskill/nat/references/function-groups.mdskill/nat/references/install-from-source.mdsrc/commands/sandbox/policy/simulate.tssrc/lib/policy/simulate.test.tssrc/lib/policy/simulate.ts
ddc919c to
7357aa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/commands/sandbox/policy/simulate.ts (1)
57-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
dependsOnfor--preset-name.
--preset-nameis only meaningful with--policy-file; without it, the value is silently ignored. oclif supportsdependsOn: ['policy-file']on the flag definition to catch this at parse time instead of silently no-op'ing.♻️ Proposed diff
"preset-name": Flags.string({ description: "Name to assign to the candidate preset when --policy-file is used. Defaults to the file basename.", required: false, + dependsOn: ["policy-file"], }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/sandbox/policy/simulate.ts` around lines 57 - 61, The "--preset-name" flag in the simulate command is currently accepted even when "--policy-file" is not provided, causing it to be ignored silently. Update the flag definition in the command’s flag setup to add a dependency on "policy-file" so oclif validates this at parse time. Use the existing flag symbol "preset-name" in the simulate command to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/sandbox/policy/simulate.ts`:
- Around line 57-61: The --preset-name flag description in simulateSandboxPolicy
is inaccurate because presetName is only used when explicitly provided and there
is no basename fallback in the policy-file flow. Update the flag help text in
the Flags.string definition to match actual behavior, or implement the basename
fallback in the simulateSandboxPolicy logic if that was intended; also consider
adding dependsOn: ['policy-file'] so presetName cannot be used without
--policy-file.
In `@src/lib/actions/sandbox/policy-simulate.test.ts`:
- Around line 21-158: The tests in simulateSandboxPolicy are using branching
`if` narrowing after checking `result.kind`, which triggers the
test-conditionals scan guardrail. Update the assertions in
policy-simulate.test.ts to keep the discriminant check but replace each `if
(result.kind === ...)` block with a direct type assertion on `result` before
accessing `lines` or `summary`, covering every occurrence in the existing test
cases. Use the `simulateSandboxPolicy` result shape and `result.kind` checks as
the locating points, and ensure each test remains a single non-branching
assertion path.
In `@src/lib/policy/simulate.ts`:
- Around line 106-123: The simulation verdict handling is collapsing
matched-but-denied requests into uncovered because endpointAllows() returns null
both for “no endpoint match” and for “endpoint matched but no rule allowed it.”
Update endpointAllows() in simulate.ts to return a distinct blocked verdict when
host/port match but no allow rule matches, and adjust the summary aggregation
that builds summary.blocked/summary.uncovered so blocked requests increment
blocked instead of uncovered.
- Around line 70-82: The host pattern matching in globMatch()/hostMatches() is
too permissive for hostnames because "*" currently matches across dots, so
"*.example.com" can incorrectly match multi-label subdomains. Update hostMatches
in simulate.ts to use hostname-aware wildcard semantics where "*" matches
exactly one DNS label and "**" is the only multi-label wildcard, and keep
globMatch for non-host paths unchanged if needed. Use the existing hostMatches
and globMatch symbols to route hostname patterns through the stricter matcher so
simulation only allows requests for the intended hostnames.
---
Nitpick comments:
In `@src/commands/sandbox/policy/simulate.ts`:
- Around line 57-61: The "--preset-name" flag in the simulate command is
currently accepted even when "--policy-file" is not provided, causing it to be
ignored silently. Update the flag definition in the command’s flag setup to add
a dependency on "policy-file" so oclif validates this at parse time. Use the
existing flag symbol "preset-name" in the simulate command to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eebcf40d-a551-4f9a-8783-fdeb2553b381
📒 Files selected for processing (5)
src/commands/sandbox/policy/simulate.tssrc/lib/actions/sandbox/policy-simulate.test.tssrc/lib/actions/sandbox/policy-simulate.tssrc/lib/policy/simulate.test.tssrc/lib/policy/simulate.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/policy/simulate.test.ts
7357aa0 to
1bee765
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/policy/simulate.ts (1)
169-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffKeep the policy engine pure; move file I/O to the action layer.
loadPolicyFile()andloadTraceFile()performfs.readFileSyncdirectly inside the simulation engine. The pure/testable primitives (extractEndpoints,parseTraceLines,simulate) are already isolated; the fs wrappers here mix I/O intosrc/lib/policy/**. Prefer exposing only the pure functions from the engine and lettingsrc/lib/actions/sandbox/policy-simulate.tsload bytes (or inject an fs adapter).As per path instructions: "ensure the simulation 'engine' stays pure and testable, while the action layer handles I/O (loading files/registry/presets) via adapters or injected deps."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/policy/simulate.ts` around lines 169 - 187, The policy simulation engine currently mixes file I/O into the pure layer via loadPolicyFile and loadTraceFile, so move fs.readFileSync out of src/lib/policy/simulate.ts and keep only pure parsing/extraction helpers there. Update the action-layer entrypoint in src/lib/actions/sandbox/policy-simulate.ts to read bytes or use an injected fs adapter, then pass the raw content into parseTraceLines/extractEndpoints (and any simulate-related helpers) so the engine remains pure and testable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/policy/simulate.ts`:
- Around line 169-187: The policy simulation engine currently mixes file I/O
into the pure layer via loadPolicyFile and loadTraceFile, so move
fs.readFileSync out of src/lib/policy/simulate.ts and keep only pure
parsing/extraction helpers there. Update the action-layer entrypoint in
src/lib/actions/sandbox/policy-simulate.ts to read bytes or use an injected fs
adapter, then pass the raw content into parseTraceLines/extractEndpoints (and
any simulate-related helpers) so the engine remains pure and testable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 15bd1cd1-cbf3-4d2d-8dd0-040a60bc2bb2
📒 Files selected for processing (5)
src/commands/sandbox/policy/simulate.tssrc/lib/actions/sandbox/policy-simulate.test.tssrc/lib/actions/sandbox/policy-simulate.tssrc/lib/policy/simulate.test.tssrc/lib/policy/simulate.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/policy/simulate.test.ts
- src/lib/actions/sandbox/policy-simulate.test.ts
- src/commands/sandbox/policy/simulate.ts
- src/lib/actions/sandbox/policy-simulate.ts
1bee765 to
00c3e89
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/policy/simulate.ts`:
- Around line 189-207: `loadPolicyFile` and `loadTraceFile` in `simulate.ts`
still perform direct `fs.readFileSync` I/O, which breaks the intended purity of
the policy/simulation layer. Move the filesystem reads into a host-boundary
adapter such as a new policy-files adapter under `src/lib/adapters/**`, and keep
`extractEndpoints`, `parseTraceLines`, `simulate`, and `renderSimulationReport`
as deterministic pure helpers. Update the action/wiring code in
`src/lib/actions/sandbox/policy-simulate.ts` to call the adapter and pass raw
content into the pure parsing/evaluation functions.
- Around line 146-184: The endpoint extraction logic in extractEndpoints
currently accepts any object with a host, which lets malformed rules values
reach endpointDecision() and fail later. Update both endpoint collection loops
in extractEndpoints() so an endpoint is only pushed when it is a PolicyObject
with a string host and an array-shaped rules field. Keep the filtering
consistent for both top-level endpoints and nested network_policies endpoints.
- Around line 189-192: loadPolicyFile currently lets YAML.parse(raw) throw on
malformed policy files, which can escape simulateSandboxPolicy and break the
CLI. Update loadPolicyFile to catch YAML parse failures and return a controlled
empty result or error indicator, or handle the failure at the
simulateSandboxPolicy call site so candidate --policy-file inputs produce a
user-facing error instead of an uncaught exception. Reference loadPolicyFile and
simulateSandboxPolicy when wiring the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9558bf2-7b60-4b41-8582-0b6df9b2303b
📒 Files selected for processing (5)
src/commands/sandbox/policy/simulate.tssrc/lib/actions/sandbox/policy-simulate.test.tssrc/lib/actions/sandbox/policy-simulate.tssrc/lib/policy/simulate.test.tssrc/lib/policy/simulate.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/policy/simulate.test.ts
- src/commands/sandbox/policy/simulate.ts
- src/lib/actions/sandbox/policy-simulate.ts
- src/lib/actions/sandbox/policy-simulate.test.ts
00c3e89 to
160a962
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/policy/simulate.ts (1)
157-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant dead branch and untested nested-group traversal.
Two independent observations in
extractEndpoints:
- Line 168's
isPolicyObject(presetVal) ? presetVal : {}is unreachable —presetValis already narrowed toPolicyObjectby the guard on line 160, so the{}fallback can never execute.- The second loop (lines 179-186), which extracts endpoints from nested
network_policiessub-groups within a preset, has no test coverage insimulate.test.ts. Only the top-levelendpointsarray path (lines 173-177) is exercised by tests.♻️ Proposed simplification for the dead branch
- const policyBlock: PolicyObject = isPolicyObject(networkPolicies) - ? networkPolicies - : isPolicyObject(presetVal) - ? presetVal - : {}; + const policyBlock: PolicyObject = isPolicyObject(networkPolicies) ? networkPolicies : presetVal;Consider adding a test for the nested-groups shape to lock in the intended behavior of this second traversal path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/policy/simulate.ts` around lines 157 - 191, The extractEndpoints helper in simulate.ts has an unreachable fallback branch because presetVal is already guaranteed to be a PolicyObject before building policyBlock; simplify that assignment to remove the dead conditional while preserving the network_policies behavior. Also add a test in simulate.test.ts that covers nested network_policies sub-groups so the second traversal loop in extractEndpoints is exercised, not just the top-level endpoints path. Use the extractEndpoints and isPolicyObject logic as the anchor points when updating the implementation and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/policy-simulate.ts`:
- Around line 95-167: simulateSandboxPolicy currently wraps loadPolicy in a
try/catch but leaves loadTrace unprotected, so read failures can escape the
SimulatePolicyResult contract. Add the same error handling around the
loadTrace(options.fromFile) path in simulateSandboxPolicy, using the existing
fileExists/loadTrace dependency pattern, and return a typed { kind: "error",
lines: [...] } result with the caught error message instead of letting the
exception propagate.
---
Nitpick comments:
In `@src/lib/policy/simulate.ts`:
- Around line 157-191: The extractEndpoints helper in simulate.ts has an
unreachable fallback branch because presetVal is already guaranteed to be a
PolicyObject before building policyBlock; simplify that assignment to remove the
dead conditional while preserving the network_policies behavior. Also add a test
in simulate.test.ts that covers nested network_policies sub-groups so the second
traversal loop in extractEndpoints is exercised, not just the top-level
endpoints path. Use the extractEndpoints and isPolicyObject logic as the anchor
points when updating the implementation and tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dfb5126f-6896-4951-a872-3113c3a43cb3
📒 Files selected for processing (5)
src/commands/sandbox/policy/simulate.tssrc/lib/actions/sandbox/policy-simulate.test.tssrc/lib/actions/sandbox/policy-simulate.tssrc/lib/policy/simulate.test.tssrc/lib/policy/simulate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commands/sandbox/policy/simulate.ts
- src/lib/actions/sandbox/policy-simulate.test.ts
0ab6801 to
a4c15c1
Compare
cv
left a comment
There was a problem hiding this comment.
policy simulate currently reports authoritative allowed or blocked results while omitting executable ancestry, resolved IP and allowed_ips, paths, protocol and MCP constraints, custom/generated policies, and live gateway drift. Missing fields become wildcards and malformed JSONL is silently dropped, allowing false-clean answers. Redesign toward fail-closed or explicit unknown semantics and document and test the public command.
8a57ca7 to
b3a83fe
Compare
|
✨ Thanks for the PR. This adds a useful dry-run capability for sandbox policy validation. Ready for maintainer review. |
366486e to
5174287
Compare
|
Exact head |
…n traces
Adds `nemoclaw <sandbox> policy simulate` — a new subcommand that evaluates
a JSONL execution trace against the active (or a candidate) sandbox policy
to predict which requests would be allowed, blocked, or uncovered before
any policy change is applied.
Trace format (one JSON object per line):
{"host":"api.slack.com","port":443,"method":"POST","path":"/api/..."}
Key capabilities:
- --from-file <path> or --from-file - (stdin) to supply the trace
- --policy-file <yaml> to test a candidate policy YAML without applying it
- --json for machine-readable output (CI integration)
- Exits with code 1 if any requests are blocked or uncovered (pipeline-safe)
Supports host glob matching (*.slack.com), method and path rules,
and monitor-mode endpoints. Unit tests cover all verdict outcomes,
glob matching, method restrictions, and JSON/text rendering.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5174287 to
b7c913f
Compare
|
Reworked at
Also rebased onto current main (includes the |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thank you for the substantial work here. After further product-direction review, NemoClaw is moving toward a unified plan/apply mechanism for onboarding and sandbox operations. A standalone We are therefore not going to pursue this command in its current form. The fail-closed parsing and policy-evaluation lessons from this PR remain useful input for the future plan phase, where policy effects should be presented as part of the same operation users later apply. Closing as not planned rather than asking for more fixes. |
Summary
Adds
nemoclaw <sandbox> policy simulate— a dry-run mode that evaluates a recorded JSONL execution trace against the active (or a candidate) sandbox policy, reporting which requests would be allowed, blocked, or uncovered before any policy change is applied. This lets operators validate a policy against real observed agent traffic instead of applying and watching for breakage.Changes
src/lib/policy/simulate.ts: simulation engine — parses JSONL traces ({"host":...,"port":...,"method":...,"path":...}per line), evaluates against preset endpoints with glob matching for hosts (*.slack.com) and paths, honors method rules and monitor-mode endpoints; returns a structuredSimulationSummary.src/commands/sandbox/policy/simulate.ts: oclif command with--from-file <path|->(file or stdin),--policy-file <yaml>to test a candidate policy without applying it,--preset-name, and--json; exits 1 when any requests are blocked or uncovered so it is pipeline-safe.src/lib/policy/simulate.test.ts: 13 unit tests covering trace parsing, verdict outcomes, wildcard host matching, method restrictions, monitor-mode, and JSON/text report rendering.Type of Change
Quality Gates
--helpdescription, usage strings, and examples; a network-policy docs page update can follow if maintainers want it surfaced there.Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification detail:
npm run build:clipasses;npx vitest run src/lib/policy/simulate.test.tspasses 13/13.Signed-off-by: sauravdev saurava@nvidia.com
Summary by CodeRabbit
sandbox:policy:simulatecommand to evaluate JSONL trace requests against YAML policy presets (from file or stdin) with optional JSON output and preset selection.allowed,blocked, oruncovered, including monitor-mode behavior.