fix(policy): refuse to apply a preset when the live policy could not be read#4589
fix(policy): refuse to apply a preset when the live policy could not be read#4589latenighthackathon wants to merge 3 commits into
Conversation
…be read
applyPresetContent and applyPresets fetch the live policy with
runCapture(..., { ignoreError: true }) and pass it through
parseCurrentPolicy, which returns "" both for a genuinely empty policy and
for exit-0-but-degraded output (an error/warning/status body, a non-object
top level, or unparseable YAML). On a degraded read mergePresetIntoPolicy
rebuilt the policy from only the new preset and policy set overwrote the
live policy, dropping every other preset and every non-network section.
The remove path already guards this; the apply paths did not.
Abort the apply when rawPolicy is non-empty but parseCurrentPolicy returns
empty (a degraded read), while still letting a genuinely empty read build
from the scaffold (fresh sandbox). This mirrors the existing remove-path
guard.
Fixes NVIDIA#4586
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthrough
ChangesPolicy Safety Guard for Preset Application
Sequence Diagram(s)sequenceDiagram
participant applyPreset as applyPresetContent/applyPresets
participant OpenShell as OpenShell (runCapture / `policy get`)
participant Parser as parseCurrentPolicy
participant Registry as registry.addCustomPolicy / policy set
applyPreset->>OpenShell: runCapture("policy get") -> rawOutput
OpenShell-->>applyPreset: rawOutput
applyPreset->>Parser: parseCurrentPolicy(rawOutput)
Parser-->>applyPreset: parsedPolicy (empty when unreadable)
alt parsedPolicy non-empty
applyPreset->>Registry: merge & policy set (apply preset)
Registry-->>applyPreset: success
else parsedPolicy empty but rawOutput non-empty
applyPreset-->>applyPreset: log error "could not read current policy"
applyPreset-->>Registry: abort (return false)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✨ Thanks for submitting this detailed PR about refusing to apply a preset when the live policy could not be read. This proposes a fix for a bug in the NemoClaw CLI that prevents silent overwriting of existing presets and policy rules. Related open issues: |
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM — mirrors the existing remove-path guard in applyPresetContent / applyPresets, narrow scope, and the new regression tests cover both apply functions plus the fresh-sandbox happy path. CI green on 1ad657e, no CodeRabbit threads.
Summary
When
policy getsucceeds but returns output NemoClaw cannot parse (an error or status line, or otherwise unexpected content),policy-addand the onboard preset apply rebuild the policy from only the preset being applied and write that back, silently dropping every other preset already in place along with the filesystem and process rules. A full gateway outage is safe, since the follow-uppolicy setalso fails and nothing is written; the damage happens when the gateway is healthy enough to accept the write but returned a degraded read. The matching remove command already refuses to act on a policy it could not read; the apply paths did not.Related Issue
Fixes #4586
Changes
applyPresetContentandapplyPresetsnow stop and report when the live policy read comes back non-empty but unparseable, instead of overwriting it. A genuinely empty read, which is the normal case for a fresh sandbox with no policy yet, still applies the first preset as before. This matches the guard the remove path already had.test/policies.test.tsfor the degraded-read abort (both functions) and the fresh-sandbox happy path.Type of Change
Verification
Ran
npm run build:cli,npm run typecheck:cli,npx @biomejs/biome lint, and the fullpoliciessuite (154/154). The new tests fail before the change and pass after.Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests