feat(policy): support custom preset files via --from-file / --from-dir (closes #2039)#2077
Conversation
📝 WalkthroughWalkthroughAdds CLI support to apply user-authored preset YAMLs via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "nemoclaw CLI"
participant FS as Filesystem
participant Policies as policies.ts
participant OpenShell as openshell
User->>CLI: run "nemoclaw <sandbox> policy-add --from-file /path/preset.yaml [--dry-run] [--yes]"
CLI->>FS: read /path/preset.yaml
FS-->>CLI: file contents
CLI->>Policies: loadPresetFromFile(filePath)
Policies-->>CLI: { presetName, content } or error
alt load success
CLI->>CLI: show endpoint preview & warning
alt dry-run
CLI-->>User: preview only (no apply)
else apply
CLI->>Policies: applyPresetContent(sandbox, presetName, content, options)
Policies->>OpenShell: "openshell policy set --wait" (merge network_policies)
OpenShell-->>Policies: success / failure
Policies-->>CLI: result
CLI-->>User: success / error
end
else load failure
CLI-->>User: error & exit non-zero
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
9e08ba8 to
6ce9ecb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/policies.test.ts (1)
1120-1164: Make prompt response configurable to cover external decline paths.
runPolicyAddExternalhardcodes prompt return value to"y", so the interactive decline behavior for external presets is untestable. Consider parameterizing it and adding a"no"case to prevent regressions around confirmation safety.Possible refactor
- function runPolicyAddExternal(extraArgs = [], envOverrides = {}) { + function runPolicyAddExternal(extraArgs = [], envOverrides = {}, promptAnswer = "y") { ... -credentials.prompt = async (message) => { +credentials.prompt = async (message) => { calls.push({ type: "prompt", message }); - return "y"; + return ${JSON.stringify("PROMPT_SENTINEL")}; }; ... - fs.writeFileSync(scriptPath, script); + fs.writeFileSync(scriptPath, script.replace("PROMPT_SENTINEL", promptAnswer));+ it("does not apply when external preset confirmation is declined with 'no'", () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-from-file-no-")); + const file = path.join(tmp, "custom-rule.yaml"); + fs.writeFileSync(file, "preset:\n name: custom-rule\nnetwork_policies: {}\n"); + const result = runPolicyAddExternal(["--from-file", file], {}, "no"); + expect(result.status).toBe(0); + const calls = JSON.parse(result.stdout.split("__CALLS__")[1].trim()); + expect(calls.some((c) => c.type === "apply")).toBeFalsy(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.ts` around lines 1120 - 1164, runPolicyAddExternal currently hardcodes the interactive prompt response by setting policies.prompt = async (...) => "y", which prevents testing the decline path; change runPolicyAddExternal to accept a promptResponse parameter (default "y"), pass that into the generated script and set policies.prompt = async (message) => { calls.push({ type: "prompt", message }); return promptResponse; }, update any callers in tests to invoke runPolicyAddExternal(..., ..., "no") to cover the decline path and assert behavior based on the calls array and process output; keep existing extraArgs and envOverrides parameters intact so other tests remain unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 1539-1542: The confirmation logic incorrectly treats only the
exact string "n" as a decline, so inputs like "no" are accepted; update both
confirmation branches (the block using askPrompt and the similar block around
the code handling the alternative at lines ~1572-1577) to normalize the response
with trim() and toLowerCase() and treat any response starting with "n" (e.g.,
response.toLowerCase().trim().startsWith("n")) as a decline; reference the
askPrompt call and the local yes variable to locate and replace the current
exact-equality checks.
---
Nitpick comments:
In `@test/policies.test.ts`:
- Around line 1120-1164: runPolicyAddExternal currently hardcodes the
interactive prompt response by setting policies.prompt = async (...) => "y",
which prevents testing the decline path; change runPolicyAddExternal to accept a
promptResponse parameter (default "y"), pass that into the generated script and
set policies.prompt = async (message) => { calls.push({ type: "prompt", message
}); return promptResponse; }, update any callers in tests to invoke
runPolicyAddExternal(..., ..., "no") to cover the decline path and assert
behavior based on the calls array and process output; keep existing extraArgs
and envOverrides parameters intact so other tests remain unaffected.
🪄 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: Pro Plus
Run ID: 4f840bb0-6819-4a75-892e-de82dbe3ff7e
📒 Files selected for processing (5)
.agents/skills/nemoclaw-user-manage-policy/SKILL.mddocs/network-policy/customize-network-policy.mdsrc/lib/policies.tssrc/nemoclaw.tstest/policies.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/network-policy/customize-network-policy.md
- .agents/skills/nemoclaw-user-manage-policy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/policies.ts
6ce9ecb to
14ca974
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/policies.ts`:
- Around line 519-536: In loadPresetFromFile, fs.readFileSync is called outside
the try-catch which allows a TOCTOU read error to escape; fix by declaring let
content; (and keeping parsed declared) then move the call to
fs.readFileSync(abs, "utf-8") inside the try block before calling
YAML.parse(content), catch any error and call console.error with a descriptive
message (including err.message) and return null so all read/parse failures
follow the same graceful null-return pattern.
🪄 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: Pro Plus
Run ID: cc1781f7-bdb1-42ab-8f84-88dfa7b2d333
📒 Files selected for processing (5)
.agents/skills/nemoclaw-user-manage-policy/SKILL.mddocs/network-policy/customize-network-policy.mdsrc/lib/policies.tssrc/nemoclaw.tstest/policies.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/network-policy/customize-network-policy.md
| function loadPresetFromFile(filePath) { | ||
| const abs = path.resolve(filePath); | ||
| if (!fs.existsSync(abs) || !fs.statSync(abs).isFile()) { | ||
| console.error(` Preset file not found: ${filePath}`); | ||
| return null; | ||
| } | ||
| if (!/\.ya?ml$/i.test(abs)) { | ||
| console.error(` Preset file must be .yaml or .yml: ${filePath}`); | ||
| return null; | ||
| } | ||
| const content = fs.readFileSync(abs, "utf-8"); | ||
| let parsed; | ||
| try { | ||
| parsed = YAML.parse(content); | ||
| } catch (err) { | ||
| console.error(` Invalid YAML in ${filePath}: ${err.message}`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Uncaught exception if file becomes unreadable between existence check and read.
fs.readFileSync at line 529 is outside the try-catch block, so if the file is deleted or permissions change after the fs.existsSync check (line 521) but before the read, the exception propagates uncaught. While this is a narrow TOCTOU window in a CLI context, it's inconsistent with the graceful null-return pattern used for other failure cases.
Proposed fix to wrap readFileSync
if (!/\.ya?ml$/i.test(abs)) {
console.error(` Preset file must be .yaml or .yml: ${filePath}`);
return null;
}
- const content = fs.readFileSync(abs, "utf-8");
let parsed;
try {
+ const content = fs.readFileSync(abs, "utf-8");
parsed = YAML.parse(content);
} catch (err) {
- console.error(` Invalid YAML in ${filePath}: ${err.message}`);
+ const msg = err.code === "ENOENT" || err.code === "EACCES"
+ ? `Cannot read ${filePath}: ${err.message}`
+ : `Invalid YAML in ${filePath}: ${err.message}`;
+ console.error(` ${msg}`);
return null;
}Note: You'll need to move const content inside the try block and keep it in scope for the return statement by storing it alongside parsed, or restructure slightly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function loadPresetFromFile(filePath) { | |
| const abs = path.resolve(filePath); | |
| if (!fs.existsSync(abs) || !fs.statSync(abs).isFile()) { | |
| console.error(` Preset file not found: ${filePath}`); | |
| return null; | |
| } | |
| if (!/\.ya?ml$/i.test(abs)) { | |
| console.error(` Preset file must be .yaml or .yml: ${filePath}`); | |
| return null; | |
| } | |
| const content = fs.readFileSync(abs, "utf-8"); | |
| let parsed; | |
| try { | |
| parsed = YAML.parse(content); | |
| } catch (err) { | |
| console.error(` Invalid YAML in ${filePath}: ${err.message}`); | |
| return null; | |
| } | |
| function loadPresetFromFile(filePath) { | |
| const abs = path.resolve(filePath); | |
| if (!fs.existsSync(abs) || !fs.statSync(abs).isFile()) { | |
| console.error(` Preset file not found: ${filePath}`); | |
| return null; | |
| } | |
| if (!/\.ya?ml$/i.test(abs)) { | |
| console.error(` Preset file must be .yaml or .yml: ${filePath}`); | |
| return null; | |
| } | |
| let parsed; | |
| try { | |
| const content = fs.readFileSync(abs, "utf-8"); | |
| parsed = YAML.parse(content); | |
| } catch (err) { | |
| const msg = err.code === "ENOENT" || err.code === "EACCES" | |
| ? `Cannot read ${filePath}: ${err.message}` | |
| : `Invalid YAML in ${filePath}: ${err.message}`; | |
| console.error(` ${msg}`); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/policies.ts` around lines 519 - 536, In loadPresetFromFile,
fs.readFileSync is called outside the try-catch which allows a TOCTOU read error
to escape; fix by declaring let content; (and keeping parsed declared) then move
the call to fs.readFileSync(abs, "utf-8") inside the try block before calling
YAML.parse(content), catch any error and call console.error with a descriptive
message (including err.message) and return null so all read/parse failures
follow the same graceful null-return pattern.
Lets users apply user-authored preset YAML to a running sandbox without editing the baseline policy or dropping to openshell policy set. - --from-file <path> applies a single custom preset - --from-dir <path> applies every .yaml preset in a directory; stops at the first failure and does not roll back already-applied presets - --yes / -y and NEMOCLAW_NON_INTERACTIVE=1 skip the confirmation prompt, also covering the original built-in preset path - Custom presets must declare preset.name as an RFC 1123 label that does not collide with a built-in preset; the file must include a network_policies section - Custom hosts are not vetted and a warning is printed before apply; the docs page spells out the risk Closes NVIDIA#2039 Signed-off-by: Kim Yang <kimy@nvidia.com>
14ca974 to
5ba18f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemoclaw.ts (1)
1534-1536:⚠️ Potential issue | 🟠 Major
--dry-runnever reaches the diff/merge path.Both dry-run branches return before
policies.applyPreset(...)/policies.applyPresetContent(...)runs, so the command only prints endpoint summaries and never shows the merged policy diff promised by this feature. That leaves operators unable to validate the actual policy change before applying a custom preset.Also applies to: 1567-1569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1534 - 1536, The dryRun branch returns early and thus never invokes policies.applyPreset or policies.applyPresetContent, so the tool only prints endpoint summaries and never computes/shows the merged policy diff — remove the early return and instead call the same applyPreset/applyPresetContent logic in dry-run mode but avoid persisting changes; specifically, when dryRun is true, call policies.applyPreset(...) or policies.applyPresetContent(...), capture the merged/updated policy result and render the diff/merge output (same code used for the real path), but skip any persistence/commit step; apply this change to both occurrences referencing dryRun around policies.applyPreset and policies.applyPresetContent so dry-run shows the merged diff without applying it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nemoclaw.ts`:
- Around line 2394-2396: The help text currently claims `--yes /
NEMOCLAW_NON_INTERACTIVE=1 skip prompts`, which is inaccurate; update the string
in the CLI help template that contains `${D}--yes / -y or
NEMOCLAW_NON_INTERACTIVE=1 skip prompts${R}` so it clarifies that `--yes` (or
the env var) only skips the final confirmation prompt and does not bypass the
preset-selection flow — note that preset selection remains interactive unless
`--from-file` or `--from-dir` is supplied; make the wording concise like “skip
confirmation prompt; preset selection remains interactive unless --from-file or
--from-dir is used.”
In `@test/policies.test.ts`:
- Around line 276-286: The tests calling policies.buildPolicySetCommand assume
NEMOCLAW_OPENSHELL_BIN is unset and delete it in finally, making them
non-hermetic; modify the test(s) to capture the current value of
process.env.NEMOCLAW_OPENSHELL_BIN at the start (e.g., const prior =
process.env.NEMOCLAW_OPENSHELL_BIN), then set or delete the env var for the test
case as needed, and in the finally block restore it
(process.env.NEMOCLAW_OPENSHELL_BIN = prior === undefined ? undefined : prior)
so the original environment is preserved; apply the same pattern to the other
argv test(s) that manipulate NEMOCLAW_OPENSHELL_BIN and reference
policies.buildPolicySetCommand to locate the affected tests.
---
Outside diff comments:
In `@src/nemoclaw.ts`:
- Around line 1534-1536: The dryRun branch returns early and thus never invokes
policies.applyPreset or policies.applyPresetContent, so the tool only prints
endpoint summaries and never computes/shows the merged policy diff — remove the
early return and instead call the same applyPreset/applyPresetContent logic in
dry-run mode but avoid persisting changes; specifically, when dryRun is true,
call policies.applyPreset(...) or policies.applyPresetContent(...), capture the
merged/updated policy result and render the diff/merge output (same code used
for the real path), but skip any persistence/commit step; apply this change to
both occurrences referencing dryRun around policies.applyPreset and
policies.applyPresetContent so dry-run shows the merged diff without applying
it.
🪄 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: Pro Plus
Run ID: fa3bcdb8-b5ae-4425-89d6-31dc821775e3
📒 Files selected for processing (5)
.agents/skills/nemoclaw-user-manage-policy/SKILL.mddocs/network-policy/customize-network-policy.mdsrc/lib/policies.tssrc/nemoclaw.tstest/policies.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/network-policy/customize-network-policy.md
- .agents/skills/nemoclaw-user-manage-policy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/policies.ts
| ${D}--from-file <path> apply a custom preset YAML${R} | ||
| ${D}--from-dir <path> apply every .yaml preset in a directory${R} | ||
| ${D}--yes / -y or NEMOCLAW_NON_INTERACTIVE=1 skip prompts${R} |
There was a problem hiding this comment.
Help text overstates the non-interactive behavior.
--yes / NEMOCLAW_NON_INTERACTIVE=1 only skips the confirmation step here; the default preset-selection flow still prompts unless --from-file or --from-dir is supplied. Saying “skip prompts” is misleading and can send automation into a hang.
✏️ Suggested wording
- ${D}--yes / -y or NEMOCLAW_NON_INTERACTIVE=1 skip prompts${R}
+ ${D}--yes / -y or NEMOCLAW_NON_INTERACTIVE=1 skip confirmations${R}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${D}--from-file <path> apply a custom preset YAML${R} | |
| ${D}--from-dir <path> apply every .yaml preset in a directory${R} | |
| ${D}--yes / -y or NEMOCLAW_NON_INTERACTIVE=1 skip prompts${R} | |
| ${D}--from-file <path> apply a custom preset YAML${R} | |
| ${D}--from-dir <path> apply every .yaml preset in a directory${R} | |
| ${D}--yes / -y or NEMOCLAW_NON_INTERACTIVE=1 skip confirmations${R} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nemoclaw.ts` around lines 2394 - 2396, The help text currently claims
`--yes / NEMOCLAW_NON_INTERACTIVE=1 skip prompts`, which is inaccurate; update
the string in the CLI help template that contains `${D}--yes / -y or
NEMOCLAW_NON_INTERACTIVE=1 skip prompts${R}` so it clarifies that `--yes` (or
the env var) only skips the final confirmation prompt and does not bypass the
preset-selection flow — note that preset selection remains interactive unless
`--from-file` or `--from-dir` is supplied; make the wording concise like “skip
confirmation prompt; preset selection remains interactive unless --from-file or
--from-dir is used.”
| it("returns an argv array with sandbox name as a separate element", () => { | ||
| const cmd = policies.buildPolicySetCommand("/tmp/policy.yaml", "my-assistant"); | ||
| expect(cmd).toEqual(["openshell", "policy", "set", "--policy", "/tmp/policy.yaml", "--wait", "my-assistant"]); | ||
| expect(cmd).toEqual([ | ||
| "openshell", | ||
| "policy", | ||
| "set", | ||
| "--policy", | ||
| "/tmp/policy.yaml", | ||
| "--wait", | ||
| "my-assistant", | ||
| ]); |
There was a problem hiding this comment.
Make these argv tests restore NEMOCLAW_OPENSHELL_BIN instead of assuming it is unset.
The first case depends on the ambient environment not defining NEMOCLAW_OPENSHELL_BIN, and the second case always deletes that variable in finally instead of restoring any preexisting value. That makes the suite non-hermetic and can cause order- or environment-dependent failures.
🧪 Suggested fix
it("returns an argv array with sandbox name as a separate element", () => {
- const cmd = policies.buildPolicySetCommand("/tmp/policy.yaml", "my-assistant");
- expect(cmd).toEqual([
- "openshell",
- "policy",
- "set",
- "--policy",
- "/tmp/policy.yaml",
- "--wait",
- "my-assistant",
- ]);
+ const originalBin = process.env.NEMOCLAW_OPENSHELL_BIN;
+ delete process.env.NEMOCLAW_OPENSHELL_BIN;
+ try {
+ const cmd = policies.buildPolicySetCommand("/tmp/policy.yaml", "my-assistant");
+ expect(cmd).toEqual([
+ "openshell",
+ "policy",
+ "set",
+ "--policy",
+ "/tmp/policy.yaml",
+ "--wait",
+ "my-assistant",
+ ]);
+ } finally {
+ if (originalBin === undefined) delete process.env.NEMOCLAW_OPENSHELL_BIN;
+ else process.env.NEMOCLAW_OPENSHELL_BIN = originalBin;
+ }
});
…
it("uses the resolved openshell binary when provided by the installer path", () => {
- process.env.NEMOCLAW_OPENSHELL_BIN = "/tmp/fake path/openshell";
+ const originalBin = process.env.NEMOCLAW_OPENSHELL_BIN;
+ process.env.NEMOCLAW_OPENSHELL_BIN = "/tmp/fake path/openshell";
try {
const cmd = policies.buildPolicySetCommand("/tmp/policy.yaml", "my-assistant");
expect(cmd).toEqual([
"/tmp/fake path/openshell",
"policy",
"set",
"--policy",
"/tmp/policy.yaml",
"--wait",
"my-assistant",
]);
} finally {
- delete process.env.NEMOCLAW_OPENSHELL_BIN;
+ if (originalBin === undefined) delete process.env.NEMOCLAW_OPENSHELL_BIN;
+ else process.env.NEMOCLAW_OPENSHELL_BIN = originalBin;
}
});Also applies to: 302-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/policies.test.ts` around lines 276 - 286, The tests calling
policies.buildPolicySetCommand assume NEMOCLAW_OPENSHELL_BIN is unset and delete
it in finally, making them non-hermetic; modify the test(s) to capture the
current value of process.env.NEMOCLAW_OPENSHELL_BIN at the start (e.g., const
prior = process.env.NEMOCLAW_OPENSHELL_BIN), then set or delete the env var for
the test case as needed, and in the finally block restore it
(process.env.NEMOCLAW_OPENSHELL_BIN = prior === undefined ? undefined : prior)
so the original environment is preserved; apply the same pattern to the other
argv test(s) that manipulate NEMOCLAW_OPENSHELL_BIN and reference
policies.buildPolicySetCommand to locate the affected tests.
Summary
Lets users apply user-authored preset YAML to a running sandbox without editing the baseline or dropping to
openshell policy set. Adds--from-file,--from-dir,--yes/-y, andNEMOCLAW_NON_INTERACTIVE=1support tonemoclaw <name> policy-add.Related Issue
Closes #2039
Changes
src/nemoclaw.ts: extendsandboxPolicyAddwith--from-file <path>,--from-dir <path>,--yes/-y, andNEMOCLAW_NON_INTERACTIVE=1handling; newapplyExternalPresethelper; updated help text.src/lib/policies.ts: newloadPresetFromFile(validates path, YAML,preset.nameas RFC 1123 label, presence ofnetwork_policies, rejects collisions with built-in preset names); refactorapplyPresetto delegate to a newapplyPresetContentthat works on raw content so the external path can reuse the merge + policy-set logic.test/policies.test.ts: unit tests forloadPresetFromFile(valid/invalid/missing fields/name collision) and integration tests for the--from-file/--from-dirCLI paths (success, dry-run, non-interactive env, mutually exclusive flags, missing path, abort-on-first-failure).docs/network-policy/customize-network-policy.md: new Custom Preset Files section with YAML skeleton,--from-file/--from-dirusage, and a warning that external hosts bypass review..agents/skills/nemoclaw-user-manage-policy/SKILL.md: regenerated from docs viascripts/docs-to-skills.py.Design decisions
preset.namematches a built-in preset — the user must rename it. Keeps the preset registry unambiguous.--from-dirfailure mode: stop at the first failure and leave earlier, successfully-applied presets in place. Rollback would require deleting already-merged entries from the running sandbox policy, which is risky; let the user fix the failing file and re-run.customize-network-policy.md.Type of Change
Verification
npx prek run --all-filespassesnpm testpasses (policies project: 95/95; install-preflight project: 57/57)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Kim Yang kimy@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests