fix(onboard): honor NEMOCLAW_POLICY_PRESETS on recreate - #2682
Merged
cv merged 14 commits intoMay 15, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughSandbox recreation now conditionally carries prior policy presets using a new exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![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: 2
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
289-307: Add a trim/empty-env boundary test forNEMOCLAW_POLICY_PRESETS.Current coverage validates non-empty override and interactive behavior, but it doesn’t lock the
" "/""case where previous policies should still carry. Adding that case would prevent regressions around env normalization.Suggested test addition
describe("shouldCarryPreviousPolicies (`#2675`)", () => { @@ it("ignores env var in interactive mode (previous list still wins)", () => { expect( shouldCarryPreviousPolicies(["npm"], { nonInteractive: false, envPolicyPresetsRaw: "pypi", }), ).toBe(true); }); + + it("treats empty/whitespace env var as not overriding previous policies", () => { + expect( + shouldCarryPreviousPolicies(["npm"], { + nonInteractive: true, + envPolicyPresetsRaw: " ", + }), + ).toBe(true); + expect( + shouldCarryPreviousPolicies(["npm"], { + nonInteractive: true, + envPolicyPresetsRaw: "", + }), + ).toBe(true); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 289 - 307, Add a test that verifies shouldCarryPreviousPolicies treats empty or whitespace-only NEMOCLAW_POLICY_PRESETS as "no override" so previous policies are carried; update the test suite around describe("shouldCarryPreviousPolicies (`#2675`)") to include a case (using shouldCarryPreviousPolicies([...], { nonInteractive: true, envPolicyPresetsRaw: " " }) and/or envPolicyPresetsRaw: "") that expects true, referencing the shouldCarryPreviousPolicies function to locate where to add the assertion.
🤖 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/onboard.ts`:
- Around line 3688-3701: When shouldCarryPreviousPolicies(previousPolicies) is
false, explicitly clear any stale session.policyPresets so old presets don't
persist on resume and interfere with setupPoliciesWithSelection or the
NEMOCLAW_POLICY_PRESETS override; inside the same branch that currently skips
assigning previousPolicies, call onboardSession.updateSession to set
current.policyPresets = [] (or null/undefined per session shape) so subsequent
logic in setupPoliciesWithSelection sees no explicit selection; ensure this is
done before the non-interactive/env-var check that logs the override and
reference isNonInteractive(), shouldCarryPreviousPolicies(),
onboardSession.updateSession, and setupPoliciesWithSelection to locate the
relevant code.
- Around line 5755-5765: The predicate shouldCarryPreviousPolicies currently
only checks NEMOCLAW_POLICY_PRESETS; adjust it to also read the effective
NEMOCLAW_POLICY_MODE (from options if provided, else
process.env.NEMOCLAW_POLICY_MODE) and treat any explicit mode as forcing a fresh
decision: if mode is one of ["skip","none","no","custom","list"]
(case-insensitive) return false so cached presets are not carried; preserve the
existing logic for when no explicit mode is set and still respect
options.envPolicyPresetsRaw and nonInteractive behavior.
---
Nitpick comments:
In `@test/onboard.test.ts`:
- Around line 289-307: Add a test that verifies shouldCarryPreviousPolicies
treats empty or whitespace-only NEMOCLAW_POLICY_PRESETS as "no override" so
previous policies are carried; update the test suite around
describe("shouldCarryPreviousPolicies (`#2675`)") to include a case (using
shouldCarryPreviousPolicies([...], { nonInteractive: true, envPolicyPresetsRaw:
" " }) and/or envPolicyPresetsRaw: "") that expects true, referencing the
shouldCarryPreviousPolicies function to locate where to add the assertion.
🪄 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: 3b76e527-43a6-456d-830e-3d9fcc5b78ff
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
5761-5770:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor explicit
NEMOCLAW_POLICY_MODEhere too.The carry-forward decision still only looks at
NEMOCLAW_POLICY_PRESETS. On non-interactive recreate, explicit modes likeskip|none|nocan still reuse cached presets via the resume path, andcustom|listcan bypass the required env-var validation.💡 Suggested fix
function shouldCarryPreviousPolicies( previousPolicies: string[] | null | undefined, - options: { nonInteractive?: boolean; envPolicyPresetsRaw?: string } = {}, + options: { + nonInteractive?: boolean; + envPolicyPresetsRaw?: string; + policyModeRaw?: string; + } = {}, ): boolean { if (!Array.isArray(previousPolicies) || previousPolicies.length === 0) return false; const nonInteractive = options.nonInteractive ?? isNonInteractive(); + const policyMode = (options.policyModeRaw ?? process.env.NEMOCLAW_POLICY_MODE ?? "suggested") + .trim() + .toLowerCase(); const envRaw = ( options.envPolicyPresetsRaw ?? process.env.NEMOCLAW_POLICY_PRESETS ?? "" ).trim(); + if (nonInteractive && ["skip", "none", "no", "custom", "list"].includes(policyMode)) { + return false; + } return !(nonInteractive && envRaw.length > 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 5761 - 5770, The function shouldCarryPreviousPolicies currently only checks NEMOCLAW_POLICY_PRESETS; update shouldCarryPreviousPolicies to also read and consider NEMOCLAW_POLICY_MODE (e.g., process.env.NEMOCLAW_POLICY_MODE or options if you add support) when deciding to carry previous policies: if nonInteractive and either envPolicyPresetsRaw is present OR NEMOCLAW_POLICY_MODE is set to an explicit mode that implies using presets (or conversely set to skip/none/no to prevent reuse), return false accordingly; adjust the return logic in shouldCarryPreviousPolicies to include this additional mode-based guard so resume paths cannot bypass env-var validation for modes like skip|none|no and custom|list are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 5761-5770: The function shouldCarryPreviousPolicies currently only
checks NEMOCLAW_POLICY_PRESETS; update shouldCarryPreviousPolicies to also read
and consider NEMOCLAW_POLICY_MODE (e.g., process.env.NEMOCLAW_POLICY_MODE or
options if you add support) when deciding to carry previous policies: if
nonInteractive and either envPolicyPresetsRaw is present OR NEMOCLAW_POLICY_MODE
is set to an explicit mode that implies using presets (or conversely set to
skip/none/no to prevent reuse), return false accordingly; adjust the return
logic in shouldCarryPreviousPolicies to include this additional mode-based guard
so resume paths cannot bypass env-var validation for modes like skip|none|no and
custom|list are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7071023f-06f1-4e37-beaf-29d8d180927a
📒 Files selected for processing (1)
src/lib/onboard.ts
Contributor
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/onboard.ts`:
- Around line 5765-5785: shouldCarryPreviousPolicies currently only treats
certain values in env policy mode as explicit; instead treat any provided
NEMOCLAW_POLICY_MODE (or options.envPolicyModeRaw) as an explicit decision.
Change the logic in shouldCarryPreviousPolicies so after computing envRaw for
presets you also compute envModeRaw = (options.envPolicyModeRaw ??
process.env.NEMOCLAW_POLICY_MODE ?? "").trim() and if envModeRaw.length > 0
return false (remove/replace the EXPLICIT_POLICY_MODES.includes check); keep the
rest of the nonInteractive and envRaw checks intact. This ensures any explicit
policy mode string prevents carrying previous policies.
🪄 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: bdc6e3a9-a2f8-46df-a1e2-1e19af1030ee
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
cv
approved these changes
May 13, 2026
…ts-recreate-override # Conflicts: # src/lib/onboard.ts
cv
enabled auto-merge (squash)
May 15, 2026 13:55
wscurran
added
area: cli
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
4 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
nemoclaw onboard --recreate-sandbox --non-interactive silently ignored the NEMOCLAW_POLICY_PRESETS env var when an existing sandbox had previous policies recorded in the registry.
The fix gates the registry-→-session carry-over on the env-var override: when running non-interactively with NEMOCLAW_POLICY_PRESETS explicitly set, the previous policies are not carried forward, and the policy step reads the env var like a fresh onboard. A [non-interactive] note prints the overridden list so the takeover is visible.
Related Issue
Fixes #2675
Changes
src/lib/onboard.ts: Added shouldCarryPreviousPolicies predicate; the createSandbox recreate branch now uses it to gate the previous-policies-→-session write, and prints a one-line note when an env-var override displaces a recorded list. Helper exported for testing.
test/onboard.test.ts: Added two unit tests under describe("shouldCarryPreviousPolicies (#2675)") covering the non-interactive override case and the interactive-mode boundary.
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: rluo8 ruluo@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests