Skip to content

fix(policy): report custom presets that cannot be recorded locally#4576

Merged
cv merged 1 commit into
NVIDIA:mainfrom
latenighthackathon:fix/policy-add-from-file-persist-4510
Jun 3, 2026
Merged

fix(policy): report custom presets that cannot be recorded locally#4576
cv merged 1 commit into
NVIDIA:mainfrom
latenighthackathon:fix/policy-add-from-file-persist-4510

Conversation

@latenighthackathon
Copy link
Copy Markdown
Contributor

@latenighthackathon latenighthackathon commented May 31, 2026

Summary

nemoclaw <sandbox> policy-add --from-file <preset> applies a custom preset to the gateway and exits 0, but the preset never appears in policy-list or status. applyPresetContent records a custom preset under the sandbox's customPolicies only when the sandbox has a local registry entry, yet it returns true regardless, so a sandbox that is missing from the registry produces a silent success with nothing recorded.

Related Issue

Fixes #4510

Changes

  • applyPresetContent (src/lib/policy/index.ts) now returns false and warns when a custom preset reaches the gateway but cannot be recorded locally because the sandbox has no registry entry. policy-add --from-file already exits non-zero on a false result, so the command no longer reports success for a preset that policy-list and status cannot display.
  • The built-in preset path is unchanged: built-in presets are matched against the live gateway by name (getGatewayPresets iterates listPresets()), so they stay visible without a registry entry; only custom presets depend on the registry record (listCustomPresets and getGatewayPresets read registry.getCustomPolicies).
  • Added regression coverage in test/policies.test.ts for the unrecorded case (returns false, warns) and the registered-sandbox case (records the preset, returns true).

Type of Change

  • Code change (feature, bug fix, or refactor)

Verification

  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed

Ran npm run build:cli, npm run typecheck:cli, npx @biomejs/biome lint, and the policies and sandbox/policy vitest suites (157 passing, including the new regression tests, which fail before the change and pass after).

Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling for custom preset application: now correctly detects and warns when a preset is applied to the gateway but cannot be recorded locally, with guidance for users to verify their policies.
  • Tests

    • Added regression tests for custom preset recording when target sandbox is missing from the registry.

applyPresetContent applied a custom preset to the gateway and then
returned true even when the sandbox had no local registry entry, so
addCustomPolicy never ran and the preset was not recorded under
customPolicies. Because policy-list and status surface custom presets
only from the registry (listCustomPresets and getGatewayPresets both
read registry.getCustomPolicies), the preset stayed invisible while
policy-add --from-file still exited 0.

Return false and warn when a custom preset reaches the gateway but
cannot be recorded locally, so the command no longer reports success
for a preset that policy-list and status cannot show.

Fixes NVIDIA#4510

Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 31, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 31, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cf40a623-e0fd-4e12-8b5b-ff09765783a5

📥 Commits

Reviewing files that changed from the base of the PR and between a25b393 and ec74a85.

📒 Files selected for processing (2)
  • src/lib/policy/index.ts
  • test/policies.test.ts

📝 Walkthrough

Walkthrough

applyPresetContent now detects when a custom preset is successfully applied to the gateway but the target sandbox has no local registry entry. When this occurs, the function logs a warning and returns false instead of true, preventing the operation from being incorrectly reported as fully applied. Regression tests validate both the missing-sandbox error path and the normal success path.

Changes

Missing Sandbox Detection and Fallback

Layer / File(s) Summary
Missing sandbox detection and regression test
src/lib/policy/index.ts, test/policies.test.ts
When registry.getSandbox(sandboxName) returns null for a custom preset, the function logs a warning, does not record the preset locally, and returns false. Regression test for issue 4510 mocks registry sandbox lookup and validates both the missing-sandbox error path (returns false, no addCustomPolicy call) and the normal success path (returns true, calls addCustomPolicy correctly).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#4013: This PR directly affects session policyPresets syncing logic in the retrieved PR, which proceeds only when applyPresetContent succeeds.
  • NVIDIA/NemoClaw#4228: Both PRs modify applyPresetContent in src/lib/policy/index.ts and its test coverage in test/policies.test.ts.

Suggested labels

fix, bug, NemoClaw CLI, v0.0.53

Suggested reviewers

  • cv
  • ericksoa

Poem

🐰 A sandbox vanished, slipped from the local store,
Yet the gateway accepted, so what was in store?
Now we check if it's missing and warn what occurred,
The truth in false returned—now the error is heard! 📋

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: ensuring custom presets that cannot be recorded locally are reported as failures rather than silent successes.
Linked Issues check ✅ Passed The PR changes fully address issue #4510's requirements: applyPresetContent now returns false and logs warnings when custom presets cannot be recorded locally due to missing registry entries, preventing false success exits.
Out of Scope Changes check ✅ Passed All changes are directly within scope: modifications to applyPresetContent behavior for unrecorded custom presets and comprehensive regression tests covering both success and failure paths.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

@wscurran
Copy link
Copy Markdown
Contributor

wscurran commented Jun 1, 2026

✨ Thanks for submitting this detailed PR about the policy-add command silently succeeding even when a custom preset cannot be recorded locally due to a missing sandbox registry entry. This proposes a fix that modifies the applyPresetContent function to return false and warn in such cases, ensuring the command exits non-zero and does not report success falsely.


Related open issues:

@wscurran wscurran added the v0.0.58 Release target label Jun 2, 2026
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output area: policy Network policy, egress rules, presets, or sandbox policy bug-fix PR fixes a bug or regression feature PR adds or expands user-visible functionality and removed NemoClaw CLI labels Jun 3, 2026
@cv cv enabled auto-merge (squash) June 3, 2026 17:55
@cv cv merged commit a6581ee into NVIDIA:main Jun 3, 2026
21 of 23 checks passed
Copy link
Copy Markdown
Contributor

@cjagwani cjagwani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean fix for the silent-success path. Tests cover both branches.

One note on closing #4510: this fixes the unregistered-sandbox case. If the QA reporter's sandbox was actually in the registry, there'd be a second cause to chase. Worth a quick QA re-verify on the original env before closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: policy Network policy, egress rules, presets, or sandbox policy bug-fix PR fixes a bug or regression feature PR adds or expands user-visible functionality v0.0.58 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ubuntu 24.04][Policy] policy-add --from-file exits 0 but custom preset is absent from policy-list and status

4 participants