feat(policy): expose active presets, allowed hosts, and approval paths in agent context#4915
Open
laitingsheng wants to merge 13 commits into
Open
feat(policy): expose active presets, allowed hosts, and approval paths in agent context#4915laitingsheng wants to merge 13 commits into
laitingsheng wants to merge 13 commits into
Conversation
…s in agent context Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an agent-facing PolicyContext: build/render and failure classification, a new ChangesPolicy Context Explanation
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant CLI as sandbox:policy:explain
participant Explain as explainSandboxPolicy
participant Builder as buildPolicyContext
participant Registry as SandboxRegistry
participant Exec as SandboxExec (openshell)
User->>CLI: run sandbox:policy:explain <name> [--json] [--write]
CLI->>Explain: explainSandboxPolicy(sandboxName, options)
Explain->>Builder: buildPolicyContext(sandboxName)
Builder->>Registry: read tier, applied presets, registry content
Explain->>CLI: output Markdown or JSON
alt writeToSandbox
Explain->>Exec: execute base64 decode write to /sandbox/.openclaw/workspace/POLICY.md
Exec-->>Explain: { status, stdout, stderr } or null
Explain->>CLI: warn on failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/actions/sandbox/policy-channel.ts (1)
1210-1232:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMissing policy context refresh in channel preset operations.
The documentation states that POLICY.md is refreshed whenever a preset is added or removed. However,
applyChannelPresetIfAvailableapplies a policy preset (Line 1212) but does not callrefreshSandboxPolicyContextFile. Similarly,removeChannelPresetIfPresent(Line 1352) removes presets without refreshing the context file.When a user adds or removes a messaging channel, the corresponding policy preset is applied or removed, but the in-sandbox policy context at
/sandbox/.openclaw/workspace/POLICY.mdremains stale until the next explicit policy operation. Agents reading this file will not see the channel-related policy changes.🔍 Verify preset application paths
#!/bin/bash # Description: Verify all preset modification call sites in policy-channel.ts rg -n 'policies\.(applyPreset|removePreset|applyPresetContent)' src/lib/actions/sandbox/policy-channel.ts -A 2 -B 2🛠️ Proposed fix
Add refresh calls after successful preset operations in both functions:
In
applyChannelPresetIfAvailable(after Line 1222):} syncSessionPolicyPresetsWithRegistry(sandboxName, channelName, "add"); + refreshSandboxPolicyContextFile(sandboxName); return true;In
removeChannelPresetIfPresent(after Line 1361):} else { syncSessionPolicyPresetsWithRegistry(sandboxName, channelName, "remove"); + refreshSandboxPolicyContextFile(sandboxName); }Also add a refresh after the error path in
removeChannelPresetIfPresent(after Line 1356, inside the!removedblock) if you want to attempt a refresh even when removal fails, though this is optional since the preset wasn't actually removed.🤖 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/actions/sandbox/policy-channel.ts` around lines 1210 - 1232, applyChannelPresetIfAvailable applies a policy preset via policies.applyPreset but never refreshes the in-sandbox policy context; update applyChannelPresetIfAvailable to call refreshSandboxPolicyContextFile(sandboxName) after a successful apply (i.e., after syncSessionPolicyPresetsWithRegistry) so POLICY.md is updated for agents. Likewise, update removeChannelPresetIfPresent to call refreshSandboxPolicyContextFile(sandboxName) after a successful policies.removePreset and after syncSessionPolicyPresetsWithRegistry("remove"); optionally also call refreshSandboxPolicyContextFile(sandboxName) inside the !removed/error branch if you want to attempt a refresh when removal fails.docs/reference/commands.mdx (1)
897-900:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe flag table under
policy-explaindocuments the wrong command.Line 897–Line 900 list
policy-removeflags (--yes/--force,--dry-run) under thepolicy-explainsection. Replace this table with--json/--write(or move this table back underpolicy-remove) to avoid incorrect operator guidance.🤖 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 `@docs/reference/commands.mdx` around lines 897 - 900, The table of flags under the policy-explain section is incorrect — it currently lists policy-remove flags (`--yes`, `--force`, `--dry-run`); update the documentation by either moving that table back to the policy-remove section or replacing it with the correct policy-explain flags (`--json`, `--write`) so the policy-explain entry reflects the actual flags; search for the policy-explain heading and the table containing `--yes`/`--force`/`--dry-run` and change it to reference `--json` and `--write` (or relocate the existing table under the policy-remove heading).
🧹 Nitpick comments (2)
docs/reference/commands.mdx (1)
895-895: 💤 Low valueReduce em-dash overload in the classification sentence.
Line 895 uses two em dashes in one sentence. Split this into separate sentences or use commas for one boundary.
As per coding guidelines, "Excessive em dashes. One per paragraph is fine; multiple per paragraph or em dashes used instead of commas/periods should be flagged."🤖 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 `@docs/reference/commands.mdx` at line 895, The sentence containing "The context also documents how a failed host or integration attempt should be classified — `blocked-by-policy`, `missing-approval`, `unsupported`, or `unknown` — so the agent can pick a remediation step instead of surfacing a lower-level network error." has two em dashes; replace them by splitting into two sentences or using commas: e.g., end the first sentence after "classified." then start a new sentence listing the classifications (`blocked-by-policy`, `missing-approval`, `unsupported`, `unknown`) and finish with "so the agent can pick a remediation step instead of surfacing a lower-level network error." Ensure only one em dash (if any) remains in the paragraph.Source: Coding guidelines
test/policy-explain-cli.test.ts (1)
19-22: ⚡ Quick winMake spawned CLI env hermetic to prevent flaky E2E behavior.
Line 21 inherits the full parent
process.env, which can leak CI/local channel or agent variables into this test. Prefer a minimal allowlist (HOME,PATH, plus explicit test overrides) and explicitly clear unrelated messaging/provider env keys.
Based on learnings, "for hermetic messaging-channel tests ... ensure the spawned helper script does not inherit CI/local messaging credentials/config ... delete/remove unrelated messaging env vars such as DISCORD_* and TELEGRAM_*."🤖 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 `@test/policy-explain-cli.test.ts` around lines 19 - 22, The test currently passes the full parent environment to the spawned CLI (env: { ...process.env, ...env }) which can leak CI/local messaging credentials; change the spawnSync call in test/policy-explain-cli.test.ts to construct a hermetic env object instead: start from a minimal allowlist (at least HOME and PATH), then apply the test-specific overrides from the existing env variable, and explicitly remove or unset messaging/provider variables (example patterns: any DISCORD_*, TELEGRAM_*, and other provider-related keys) before passing it to spawnSync (refer to process.execPath, CLI and args to locate the call).Source: Learnings
🤖 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/network-policy/customize-network-policy.mdx`:
- Around line 350-355: The table maps HTTP 403 to both `blocked-by-policy` and
`missing-approval`; remove the duplicate by keeping HTTP 403 in only one
classification and update the corresponding row text so each status code maps
unambiguously—specifically edit the `blocked-by-policy` and `missing-approval`
rows to ensure 403 appears in only one of them (choose which behavior you want
to preserve), adjust the error-code list (EHOSTUNREACH, ENETUNREACH, ENOTFOUND,
ECONNREFUSED, ETIMEDOUT, EAI_AGAIN) accordingly, and update any adjacent
examples or explanations to reflect the single authoritative classification for
403.
In `@docs/reference/commands.mdx`:
- Line 872: The docs heading "### `$$nemoclaw <name> policy-explain`" does not
match the command listed by the CLI help, causing the parity job to fail; update
this heading to the exact help-surfaced command name (as shown by `$$nemoclaw
--help`) or add the `policy-explain` alias to the CLI help metadata so they
match. Locate the heading in docs/reference/commands.mdx and change the
backticked command token to the CLI's canonical form (or update the CLI's help
registration code to emit the alias) ensuring the token exactly matches the
output of `$$nemoclaw --help` (reference the `$$nemoclaw <name> policy-explain`
token and the help metadata registration).
In `@src/commands/sandbox/policy/explain.ts`:
- Around line 16-20: Update the command's user-facing help by adding the --write
flag to the static usage and examples in the Explain command (look for the
static usage and static examples in src/commands/sandbox/policy/explain.ts);
include --write in the usage string (e.g., "<name> [--json] [--write]" or
similar) and add at least one example showing the flag in static examples
alongside the existing examples (e.g., a "--write" invocation and one combining
"--json --write") so the help output reflects the actual supported flags.
In `@src/lib/actions/sandbox/policy-explain.ts`:
- Around line 91-95: The current branch in the writeToSandbox block suppresses
the "sandbox unreachable" failure; update the logic around
writePolicyContextToSandbox(sandboxName, { ...deps, build, render }) so that any
failed write (writeResult.written === false) always surfaces (e.g., call warn or
throw) instead of ignoring when writeResult.reason === "sandbox unreachable";
reference writeToSandbox, writePolicyContextToSandbox, writeResult,
POLICY_CONTEXT_SANDBOX_PATH and the existing warn call to ensure the same
message path is used for all failure reasons.
In `@src/lib/cli/public-route-metadata.ts`:
- Line 29: The test assertion in public-argv-translation.test.ts that checks
Object.keys(SANDBOX_ROUTE_OVERRIDES).sort() uses a hard-coded expected list and
needs to be updated to include the new "sandbox:policy:explain" override; open
the test around the "keeps explicit compatibility route overrides..." case and
add "sandbox:policy:explain" to the expected array (or replace the hard-coded
array with a derivation from SANDBOX_ROUTE_OVERRIDES to avoid future desyncs),
referencing SANDBOX_ROUTE_OVERRIDES and the new entry added in
public-route-metadata.ts ("sandbox:policy:explain") when making the change.
In `@src/lib/onboard.ts`:
- Around line 5816-5823: Extract the inline best-effort seeding into a small
helper function (e.g., export async function seedPolicyContext(sandboxName:
string)) and move the try/require +
policyExplain.writePolicyContextToSandbox(sandboxName) logic into that helper
(including the silent catch). In the existing onboard thin call site, replace
the inline block with a single call to seedPolicyContext(sandboxName) so
onboard.ts only delegates; keep the same behavior and error swallowing in the
helper. Ensure the helper exports its function name so onboard can import and
call it.
---
Outside diff comments:
In `@docs/reference/commands.mdx`:
- Around line 897-900: The table of flags under the policy-explain section is
incorrect — it currently lists policy-remove flags (`--yes`, `--force`,
`--dry-run`); update the documentation by either moving that table back to the
policy-remove section or replacing it with the correct policy-explain flags
(`--json`, `--write`) so the policy-explain entry reflects the actual flags;
search for the policy-explain heading and the table containing
`--yes`/`--force`/`--dry-run` and change it to reference `--json` and `--write`
(or relocate the existing table under the policy-remove heading).
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 1210-1232: applyChannelPresetIfAvailable applies a policy preset
via policies.applyPreset but never refreshes the in-sandbox policy context;
update applyChannelPresetIfAvailable to call
refreshSandboxPolicyContextFile(sandboxName) after a successful apply (i.e.,
after syncSessionPolicyPresetsWithRegistry) so POLICY.md is updated for agents.
Likewise, update removeChannelPresetIfPresent to call
refreshSandboxPolicyContextFile(sandboxName) after a successful
policies.removePreset and after syncSessionPolicyPresetsWithRegistry("remove");
optionally also call refreshSandboxPolicyContextFile(sandboxName) inside the
!removed/error branch if you want to attempt a refresh when removal fails.
---
Nitpick comments:
In `@docs/reference/commands.mdx`:
- Line 895: The sentence containing "The context also documents how a failed
host or integration attempt should be classified — `blocked-by-policy`,
`missing-approval`, `unsupported`, or `unknown` — so the agent can pick a
remediation step instead of surfacing a lower-level network error." has two em
dashes; replace them by splitting into two sentences or using commas: e.g., end
the first sentence after "classified." then start a new sentence listing the
classifications (`blocked-by-policy`, `missing-approval`, `unsupported`,
`unknown`) and finish with "so the agent can pick a remediation step instead of
surfacing a lower-level network error." Ensure only one em dash (if any) remains
in the paragraph.
In `@test/policy-explain-cli.test.ts`:
- Around line 19-22: The test currently passes the full parent environment to
the spawned CLI (env: { ...process.env, ...env }) which can leak CI/local
messaging credentials; change the spawnSync call in
test/policy-explain-cli.test.ts to construct a hermetic env object instead:
start from a minimal allowlist (at least HOME and PATH), then apply the
test-specific overrides from the existing env variable, and explicitly remove or
unset messaging/provider variables (example patterns: any DISCORD_*, TELEGRAM_*,
and other provider-related keys) before passing it to spawnSync (refer to
process.execPath, CLI and args to locate the call).
🪄 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: 1be9a847-0cb9-47da-ad98-2bb09fe9e798
📒 Files selected for processing (11)
docs/network-policy/customize-network-policy.mdxdocs/reference/commands.mdxsrc/commands/sandbox/policy/explain.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/policy-explain.test.tssrc/lib/actions/sandbox/policy-explain.tssrc/lib/cli/public-route-metadata.tssrc/lib/onboard.tssrc/lib/policy/context.test.tssrc/lib/policy/context.tstest/policy-explain-cli.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27097815833
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
area: policy
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4915.docs.buildwithfern.com/nemoclaw |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/commands-nemohermes.mdx (1)
724-724: ⚡ Quick winReduce em-dash usage in this sentence (LLM pattern detected).
This line uses multiple em dashes in one sentence; please rewrite with commas/periods (or keep a single em dash max).
As per coding guidelines: "Excessive em dashes. One per paragraph is fine; multiple per paragraph or em dashes used instead of commas/periods should be flagged."
🤖 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 `@docs/reference/commands-nemohermes.mdx` at line 724, The sentence containing the phrase "blocked-by-policy, `missing-approval`, `unsupported`, or `unknown`" uses multiple em dashes; rewrite it to avoid excessive em-dash usage by replacing the em dashes with commas or a period (or keep a single em dash at most), e.g., split into two sentences or use commas to list the classifications so the agent can still pick a remediation step instead of surfacing a lower-level network error.Source: Coding guidelines
🤖 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 `@docs/reference/commands-nemohermes.mdx`:
- Line 724: The sentence containing the phrase "blocked-by-policy,
`missing-approval`, `unsupported`, or `unknown`" uses multiple em dashes;
rewrite it to avoid excessive em-dash usage by replacing the em dashes with
commas or a period (or keep a single em dash at most), e.g., split into two
sentences or use commas to list the classifications so the agent can still pick
a remediation step instead of surfacing a lower-level network error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e12faad-695d-4c5d-877d-35544774a1b2
📒 Files selected for processing (3)
docs/reference/commands-nemohermes.mdxsrc/lib/cli/command-registry.test.tssrc/lib/cli/public-argv-translation.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27101010496
|
…nnel removes Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27112501540
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27113087954
|
…r findings Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/context.ts`:
- Around line 546-579: The code calls buildPolicyContext(..., {
skipGatewayProbe: true }) and then treats findMatchingPreset(...) results as
authoritative; change the logic in the branch that uses matched (and references
ctx.activePresets, ctx.approvalPath.inspect) to first verify the preset's
verification state (e.g., matched.verification === "verified" ||
matched.verification === "gateway-only") or perform an actual gateway probe via
buildPolicyContext without skipGatewayProbe before returning high-confidence
"missing-approval"/"unknown" responses; if the preset is
unverified/gateway-unavailable, downgrade confidence or return a
policy-blocked/indeterminate result and advise running the gateway
probe/inspection instead.
🪄 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: 72f8af3e-3535-46fd-adae-ca2f62b77999
📒 Files selected for processing (11)
docs/network-policy/customize-network-policy.mdxsrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/policy-context-refresh.test.tssrc/lib/actions/sandbox/policy-context-refresh.tssrc/lib/actions/sandbox/policy-explain.test.tssrc/lib/actions/sandbox/policy-explain.tssrc/lib/onboard.tssrc/lib/onboard/policy-selection.tssrc/lib/policy/context.test.tssrc/lib/policy/context.tstest/policy-explain-cli.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/network-policy/customize-network-policy.mdx
- src/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/policy/context.test.ts
- src/lib/actions/sandbox/policy-explain.test.ts
- test/policy-explain-cli.test.ts
… failures Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…ion, extract onboard seed Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27117590115
|
1 similar comment
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27117590115
|
…inst symlink TOCTOU Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27120127651
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27120127651
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27120658115
|
…y-policy Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27124157028
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
nemoclaw <sandbox> policy-explain, a redacted summary of the active policy presets, allowed host categories, approval paths, and support boundaries that an in-sandbox agent can read to reason about what is allowed, why something is blocked, and how to request a change.Related Issue
Resolves #4629
Changes
nemoclaw <sandbox> policy-explain [--json] [--write]command and matchingsandbox policy explainoclif topic route.src/lib/policy/context.ts:buildPolicyContext,renderPolicyContextMarkdown, andclassifyAccessFailure(blocked-by-policy/missing-approval/unsupported/unknown)./sandbox/.openclaw/workspace/POLICY.mdafter the onboard policy step and refresh on everypolicy-add/policy-remove.--writetriggers the seed on demand. Output omits raw policy YAML, binary allowlists, and credential metadata.docs/network-policy/customize-network-policy.mdxand a command entry indocs/reference/commands.mdx.bin/nemoclaw.jsagainst a tempHOMEand asserts the markdown / JSON output and routing.Type of Change
Verification
Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests