feat(onboard): preset onboard wizard#1397
Open
cheese-head wants to merge 7 commits intoNVIDIA:mainfrom
Open
Conversation
Introduces a terminal UI wizard for policy selection that replaces the previous placeholder in setupPoliciesWithSelection. No LLM required — policies are composed deterministically from preset YAML files. - bin/lib/policy-compose.js: preset loading, access/tier filtering, YAML serialization - bin/lib/policy-wizard-static.js: checklist TUI with j/k navigation, / search, Tab access toggle, exfil risk warnings - bin/lib/policies.js: add applyPresetFromContent() to apply composed YAML to a sandbox - bin/lib/onboard.js: wire selectAndMerge() into Step 7 of the onboard flow Signed-off-by: Patrick Riel <priel@nvidia.com>
Adds per-rule exfil_risk fields to existing preset YAML files so the policy wizard can surface data exfiltration warnings at selection time without hardcoding risk metadata in the wizard itself. Signed-off-by: Patrick Riel <priel@nvidia.com>
Adds presets for: calendar, confluence, datadog, email, github, github-t1, gitlab, grafana, linear, notion, openai, pagerduty, sendgrid, stripe, teams, websearch. Each preset includes per-rule exfil_risk fields on endpoints that can move data outside the sandbox. Signed-off-by: Patrick Riel <priel@nvidia.com>
Signed-off-by: Patrick Riel <priel@nvidia.com>
Contributor
📝 WalkthroughWalkthroughReplaces manual preset prompts with a static, offline TUI wizard and deterministic policy composer; adds an API to apply preset YAML content directly; and adds ~20+ new or updated preset YAML files for third-party services. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Wizard as policy-wizard-static
participant Compose as policy-compose
participant Policies as bin/lib/policies
participant Sandbox as Sandbox/Registry
User->>Wizard: invoke selectAndMerge()
Wizard->>Compose: load presets from PRESETS_DIR
Compose-->>Wizard: preset metadata
Wizard->>User: preset checklist (select)
User-->>Wizard: confirm selections
Wizard->>User: access-level grid (read/write)
User-->>Wizard: confirm access choices
Wizard->>Compose: composePresets(selections)
Compose->>Compose: filter rules, dedupe, merge, warn
Compose-->>Wizard: merged policies + warnings
Wizard->>User: show exfil-risk & review
User-->>Wizard: approve
Wizard->>Compose: buildPresetYaml(name, policies, fsPaths)
Compose-->>Wizard: YAML
Wizard-->>User: return merged YAML
User->>Policies: applyPresetFromContent(sandboxName, mergedYaml)
Policies->>Sandbox: get current policy
Sandbox-->>Policies: current policy YAML
Policies->>Policies: mergePresetIntoPolicy(current, extracted)
Policies->>Sandbox: apply merged policy
Policies->>Sandbox: update registry preset list
Policies-->>User: success / errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
cheese-head
changed the title
Author
|
new policies all need to be validated |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoclaw-blueprint/policies/presets/docker.yaml (1)
13-38:⚠️ Potential issue | 🟠 MajorUse
access: fullfor Docker registry endpoints instead of TLS termination.Lines 17 and 34 keep Docker registry traffic on
protocol: rest+tls: terminate. For container-registry flows, this is brittle and can break auth/push-pull behavior. Please switch these endpoints to CONNECT-styleaccess: fulland rely onbinariesscoping for control.Based on learnings: In
nemoclaw-blueprint/policies/presets/, for package manager and container registry–type presets that use CONNECT tunneling, useaccess: fullinstead oftls: terminatebecause TLS termination can break authentication and cause connection drops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/docker.yaml` around lines 13 - 38, Update the Docker registry preset entries for host: registry-1.docker.io and host: nvcr.io to use CONNECT-style tunneling by replacing TLS termination with access: full (i.e., remove or stop using tls: terminate and protocol: rest for these container-registry endpoints and set access: full while keeping enforcement: enforce and existing rules), so the registry flows use full CONNECT tunneling and rely on binaries scoping for control; locate the host blocks named "registry-1.docker.io" and "nvcr.io" and make this change.
🧹 Nitpick comments (10)
bin/lib/policies.js (1)
228-228: Avoid redundant current-policy normalization.
mergePresetIntoPolicyalready normalizes/parses current policy. PassingparseCurrentPolicy(rawPolicy)here does duplicate work.Proposed cleanup
- const merged = mergePresetIntoPolicy(parseCurrentPolicy(rawPolicy), presetEntries); + const merged = mergePresetIntoPolicy(rawPolicy, presetEntries);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policies.js` at line 228, The code currently calls mergePresetIntoPolicy(parseCurrentPolicy(rawPolicy), presetEntries) which redundantly normalizes the policy because mergePresetIntoPolicy already parses/normalizes the current policy; change the call to pass rawPolicy directly (i.e., mergePresetIntoPolicy(rawPolicy, presetEntries)) and remove the unnecessary parseCurrentPolicy invocation to avoid double work while keeping merged, rawPolicy and presetEntries identifiers unchanged.nemoclaw-blueprint/policies/presets/stripe.yaml (1)
7-8: Minor formatting: extra blank line.There's a double blank line after the description which is inconsistent with other presets in this PR (e.g., github.yaml has a single blank line).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/stripe.yaml` around lines 7 - 8, The file's preset entry for Stripe has an extra blank line after the description field; open the presets/presets/stripe.yaml (look for the description: entry in stripe.yaml) and remove the duplicate empty line so the description is followed by a single blank line consistent with other presets (e.g., github.yaml). Ensure spacing matches other preset files.nemoclaw-blueprint/policies/presets/huggingface.yaml (1)
37-39: Consider addingexfil_riskannotation to the GET rule for consistency.The
GET /**rule onapi-inference.huggingface.colacks anexfil_riskannotation, while other GET rules in this preset (lines 19, 29) include one. GET requests to inference endpoints could expose model outputs or metadata.💡 Suggested addition
rules: - allow: { method: GET, path: "/**" } + exfil_risk: "GET exposes inference results and model metadata from Hugging Face Inference API" - allow: { method: POST, path: "/**" } exfil_risk: "POST sends arbitrary input data to Hugging Face hosted inference endpoints"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/huggingface.yaml` around lines 37 - 39, Add an exfil_risk annotation to the existing GET rule (the entry with allow: { method: GET, path: "/**" }) so it matches the other GET rules in this preset; update the GET rule for api-inference.huggingface.co to include an exfil_risk string (e.g., "GET may expose model outputs or metadata") right alongside the rule to document the potential data exfiltration risk.nemoclaw-blueprint/policies/presets/websearch.yaml (1)
7-8: Minor formatting: extra blank line.Double blank line after description - inconsistent with other presets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/websearch.yaml` around lines 7 - 8, Remove the extra blank line after the description field in the websearch preset so the YAML matches the formatting of other presets; open the websearch.yaml entry, locate the description: value for the websearch preset and delete the redundant empty line immediately following it so there is only a single newline after the description.nemoclaw-blueprint/policies/presets/pagerduty.yaml (1)
7-8: Minor formatting: extra blank line.Double blank line after description - inconsistent with other presets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/pagerduty.yaml` around lines 7 - 8, The preset YAML has an extra blank line after the description field in pagerduty.yaml; remove the duplicate empty line so the description block follows the same single-line spacing as other presets (edit the description entry in pagerduty.yaml to collapse the double blank line into a single blank line).bin/lib/onboard.js (1)
3094-3101: Error handling returns without clear feedback on next steps.When
selectAndMerge()throws (e.g., user cancels), the function returns silently after logging. This is acceptable for cancellation, but the error message "cancelled or failed" doesn't distinguish between intentional cancellation and unexpected errors.Consider differentiating these cases:
💡 Optional improvement
try { mergedYaml = await selectAndMerge(); } catch (err) { - console.error(` Policy selection cancelled or failed: ${err.message}`); + if (err.message === "cancelled") { + console.log(" Policy selection skipped."); + } else { + console.error(` Policy selection failed: ${err.message}`); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 3094 - 3101, The catch around selectAndMerge() currently logs "cancelled or failed" and returns; update it to distinguish user-initiated cancellation from unexpected errors by inspecting the thrown error (e.g., err.name/err.code/err.isCancellation or specific err.message returned by selectAndMerge). For cancellation, log an informational message like "Policy selection cancelled by user" and return; for other errors, log the full error with stack/context and either rethrow or exit non-zero so callers can react (e.g., process.exit(1)). Ensure the console output uses clear messages and includes err.stack or err.message for unexpected failures and keep handling tied to selectAndMerge().bin/lib/policy-wizard-static.js (2)
61-61: Calendarhostsarray is incomplete.The
calendarpreset YAML defines two endpoints (oauth2.googleapis.comandwww.googleapis.com), but the TOOLS catalog only listswww.googleapis.com. While this is cosmetic (actual policy composition reads from preset files vialoadAllPresets()), it could confuse users browsing the tool list.Suggested fix
- calendar: { id: "calendar", name: "Google Calendar", hosts: ["www.googleapis.com"], risk: { read: 2, write: 3 }, category: "cal" }, + calendar: { id: "calendar", name: "Google Calendar", hosts: ["oauth2.googleapis.com", "www.googleapis.com"], risk: { read: 2, write: 3 }, category: "cal" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` at line 61, The calendar preset object (symbol "calendar") has an incomplete hosts array—add "oauth2.googleapis.com" to the hosts list so the TOOLS catalog matches the preset YAML; locate the calendar entry in bin/lib/policy-wizard-static.js (the calendar: { id: "calendar", ... hosts: [...] } object) and update its hosts array to include "oauth2.googleapis.com" alongside "www.googleapis.com" to keep the tool listing consistent with loadAllPresets() outputs.
75-112:USE_CASESandTOOLSdefinitions are unused dead code.Both the
USE_CASESarray (lines 75–112) and theTOOLSobject (lines 50–69) appear nowhere in the codebase except their definitions. Additionally, thepromptChecklist()function (line 237) is never called.The
run()andselectAndMerge()functions proceed directly fromloadAllPresets()topromptPresetChecklist(), bypassing any use-case-based selection flow entirely.Consider removing these unused definitions to reduce code clutter, or integrate use-case selection if that functionality is still desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` around lines 75 - 112, The USE_CASES array and TOOLS object are dead code and promptChecklist() is never invoked; either remove these unused symbols (USE_CASES, TOOLS, promptChecklist) to eliminate clutter, or wire them into the existing flow by updating run() / selectAndMerge() to call a new use-case selection step between loadAllPresets() and promptPresetChecklist() (e.g., add a promptUseCase() that references USE_CASES/TOOLS and passes the chosen tools into selectAndMerge()/promptPresetChecklist()); pick one approach and implement it consistently so the definitions are either deleted or actually consumed.bin/lib/policy-compose.js (2)
160-169: Duplicate tool selections may silently overwrite each other.If
toolSelectionscontains the sametool.idmultiple times (e.g., selecting "slack" twice with different access levels), the composed key at line 162 would collide, and the later entry silently overwrites the earlier one. Consider either:
- Deduplicating
toolSelectionsbytool.idupfront (keeping highest access level), or- Documenting that duplicate selections are unsupported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-compose.js` around lines 160 - 169, toolSelections can contain duplicate tool.id entries which cause the composed key (`const key = multi ? `${tool.id}-${blockName}` : blockName;`) to collide and let later selections silently overwrite earlier ones; fix by deduplicating toolSelections before composing policies (e.g., collapse entries for the same tool.id keeping the highest access level) or validate and throw when duplicates exist—update the preprocessing where toolSelections is consumed to produce a unique list keyed by tool.id (and adjust any logic that relies on the original array) so policies[key] cannot be silently overwritten.
188-195: Mutating input objects in-place may cause unexpected side effects.The loop at lines 189-195 mutates the
policiesobject's endpoint entries directly (settingenforcementandtls). Sincepoliciesis passed by reference fromcomposePresets, these mutations persist in the caller's data structure. IfcomposePresetsresults are reused or inspected afterbuildPresetYaml, the original endpoint objects will have been modified.Consider cloning endpoints before mutation:
♻️ Proposed fix to avoid mutation
function buildPresetYaml(presetName, policies, fsAccess) { - // Re-stamp enforcement + tls on each REST endpoint. - for (const block of Object.values(policies)) { - for (const ep of block.endpoints || []) { - if (ep.access === "full") continue; // tunnel — leave alone - ep.enforcement = "enforce"; - ep.tls = ep.tls ?? "terminate"; + // Re-stamp enforcement + tls on each REST endpoint (clone to avoid mutation). + const clonedPolicies = {}; + for (const [key, block] of Object.entries(policies)) { + clonedPolicies[key] = { + ...block, + endpoints: (block.endpoints || []).map(ep => { + if (ep.access === "full") return ep; + return { ...ep, enforcement: "enforce", tls: ep.tls ?? "terminate" }; + }), + }; + } } } const doc = { preset: { name: presetName }, // ... - network_policies: policies, + network_policies: clonedPolicies, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-compose.js` around lines 188 - 195, The loop mutates endpoint objects in the input "policies" (used by composePresets) causing side effects; instead create new endpoint objects when applying defaults so the caller's data isn't changed: iterate blocks in buildPresetYaml (or the function containing the shown loop), map block.endpoints to a new array of cloned endpoint objects (preserving fields), skip cloning for ep.access === "full", and set enforcement/tls on the cloned objects (ep.enforcement and ep.tls) before assigning the new array back to the block; this preserves original composePresets output while returning modified copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/policies.js`:
- Around line 219-239: applyPresetFromContent omits the RFC1123/truncation guard
used by applyPreset, which can allow invalid/truncated sandbox names to be
passed into buildPolicyGetCommand/buildPolicySetCommand; update
applyPresetFromContent to validate/sanitize the sandboxName the same way
applyPreset does (reuse the same helper or logic that enforces RFC1123 and
truncation) before calling runCapture(buildPolicyGetCommand(...)) and
run(buildPolicySetCommand(...)) so both composed and direct apply paths behave
identically.
- Around line 219-239: applyPresetFromContent currently applies merged policy to
the sandbox but does not record applied preset metadata like applyPreset does;
update applyPresetFromContent to capture which preset entries were actually
applied (use extractPresetEntries/mergePresetIntoPolicy or parseCurrentPolicy to
determine applied policy names) and call registry.updateSandbox with the same
metadata payload used by applyPreset (i.e., include the sandbox identifier and
the applied presets/policies) after successfully running buildPolicySetCommand
(or return the applied metadata to callers and ensure onboard.js uses
registry.updateSandbox when invoking applyPresetFromContent); ensure
registry.updateSandbox is invoked only on successful policy application and
preserve existing temp-file cleanup behavior.
In `@bin/lib/policy-compose.js`:
- Around line 200-204: filesystem_policy construction currently assumes fsAccess
is an object and directly accesses fsAccess.readOnly and fsAccess.readWrite,
which throws if fsAccess is null/undefined; update the code that builds
filesystem_policy (the filesystem_policy object) to defensively handle fsAccess
by defaulting it (e.g., treat fsAccess as {} when falsy) or use guards/optional
chaining and default to empty arrays for readOnly/readWrite so the spread
operations safely work even when fsAccess is missing.
In `@bin/lib/policy-wizard-static.js`:
- Around line 536-542: Remove the dead helper: delete the unused savePreset
function and its PRESETS_OUT_DIR constant (symbols: savePreset, PRESETS_OUT_DIR)
from the module since savePreset is never called or exported and run() writes
directly to CUSTOM_DIR using fs.writeFileSync; ensure no other code references
PRESETS_OUT_DIR before committing.
In `@nemoclaw-blueprint/policies/presets/email.yaml`:
- Around line 29-30: Update the Gmail threads rule to match the actual API path:
replace the path string "/gmail/v1/threads" with the user-scoped pattern
"/gmail/v1/users/*/threads" so the policy rule (the entry with allow: { method:
GET, path: ... } and exfil_risk "GET exposes email thread metadata") will
properly match Gmail's threads list endpoint.
In `@nemoclaw-blueprint/policies/presets/github.yaml`:
- Line 49: The YAML entry for the permission mapping that currently reads an
unquoted glob (the allow mapping with method GET and path /**) should quote the
glob pattern to match the style used elsewhere (e.g., "/**"); update the allow
mapping's path value to a quoted string ("/**") so YAML parsing is consistent
with the other presets.
In `@nemoclaw-blueprint/policies/presets/gitlab.yaml`:
- Around line 6-40: The description promises repository access but
network_policies.gitlab only allows REST API endpoints; update the gitlab preset
to either add Git HTTP paths or remove repo access language: if adding, append
rules under network_policies.gitlab.endpoints[*].rules to allow Git HTTP
endpoints like allow: { method: GET, path: /{group}/{project}.git/* } and
specific endpoints used for clone/fetch/push (e.g., /*.git/info/refs,
/*.git/git-upload-pack, /*.git/git-receive-pack) so the listed binary
/usr/bin/git can perform repository operations; alternatively, change the
top-level description to remove "repository access" and ensure binaries still
reflect API-only usage (remove /usr/bin/git if not needed).
---
Outside diff comments:
In `@nemoclaw-blueprint/policies/presets/docker.yaml`:
- Around line 13-38: Update the Docker registry preset entries for host:
registry-1.docker.io and host: nvcr.io to use CONNECT-style tunneling by
replacing TLS termination with access: full (i.e., remove or stop using tls:
terminate and protocol: rest for these container-registry endpoints and set
access: full while keeping enforcement: enforce and existing rules), so the
registry flows use full CONNECT tunneling and rely on binaries scoping for
control; locate the host blocks named "registry-1.docker.io" and "nvcr.io" and
make this change.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 3094-3101: The catch around selectAndMerge() currently logs
"cancelled or failed" and returns; update it to distinguish user-initiated
cancellation from unexpected errors by inspecting the thrown error (e.g.,
err.name/err.code/err.isCancellation or specific err.message returned by
selectAndMerge). For cancellation, log an informational message like "Policy
selection cancelled by user" and return; for other errors, log the full error
with stack/context and either rethrow or exit non-zero so callers can react
(e.g., process.exit(1)). Ensure the console output uses clear messages and
includes err.stack or err.message for unexpected failures and keep handling tied
to selectAndMerge().
In `@bin/lib/policies.js`:
- Line 228: The code currently calls
mergePresetIntoPolicy(parseCurrentPolicy(rawPolicy), presetEntries) which
redundantly normalizes the policy because mergePresetIntoPolicy already
parses/normalizes the current policy; change the call to pass rawPolicy directly
(i.e., mergePresetIntoPolicy(rawPolicy, presetEntries)) and remove the
unnecessary parseCurrentPolicy invocation to avoid double work while keeping
merged, rawPolicy and presetEntries identifiers unchanged.
In `@bin/lib/policy-compose.js`:
- Around line 160-169: toolSelections can contain duplicate tool.id entries
which cause the composed key (`const key = multi ? `${tool.id}-${blockName}` :
blockName;`) to collide and let later selections silently overwrite earlier
ones; fix by deduplicating toolSelections before composing policies (e.g.,
collapse entries for the same tool.id keeping the highest access level) or
validate and throw when duplicates exist—update the preprocessing where
toolSelections is consumed to produce a unique list keyed by tool.id (and adjust
any logic that relies on the original array) so policies[key] cannot be silently
overwritten.
- Around line 188-195: The loop mutates endpoint objects in the input "policies"
(used by composePresets) causing side effects; instead create new endpoint
objects when applying defaults so the caller's data isn't changed: iterate
blocks in buildPresetYaml (or the function containing the shown loop), map
block.endpoints to a new array of cloned endpoint objects (preserving fields),
skip cloning for ep.access === "full", and set enforcement/tls on the cloned
objects (ep.enforcement and ep.tls) before assigning the new array back to the
block; this preserves original composePresets output while returning modified
copies.
In `@bin/lib/policy-wizard-static.js`:
- Line 61: The calendar preset object (symbol "calendar") has an incomplete
hosts array—add "oauth2.googleapis.com" to the hosts list so the TOOLS catalog
matches the preset YAML; locate the calendar entry in
bin/lib/policy-wizard-static.js (the calendar: { id: "calendar", ... hosts:
[...] } object) and update its hosts array to include "oauth2.googleapis.com"
alongside "www.googleapis.com" to keep the tool listing consistent with
loadAllPresets() outputs.
- Around line 75-112: The USE_CASES array and TOOLS object are dead code and
promptChecklist() is never invoked; either remove these unused symbols
(USE_CASES, TOOLS, promptChecklist) to eliminate clutter, or wire them into the
existing flow by updating run() / selectAndMerge() to call a new use-case
selection step between loadAllPresets() and promptPresetChecklist() (e.g., add a
promptUseCase() that references USE_CASES/TOOLS and passes the chosen tools into
selectAndMerge()/promptPresetChecklist()); pick one approach and implement it
consistently so the definitions are either deleted or actually consumed.
In `@nemoclaw-blueprint/policies/presets/huggingface.yaml`:
- Around line 37-39: Add an exfil_risk annotation to the existing GET rule (the
entry with allow: { method: GET, path: "/**" }) so it matches the other GET
rules in this preset; update the GET rule for api-inference.huggingface.co to
include an exfil_risk string (e.g., "GET may expose model outputs or metadata")
right alongside the rule to document the potential data exfiltration risk.
In `@nemoclaw-blueprint/policies/presets/pagerduty.yaml`:
- Around line 7-8: The preset YAML has an extra blank line after the description
field in pagerduty.yaml; remove the duplicate empty line so the description
block follows the same single-line spacing as other presets (edit the
description entry in pagerduty.yaml to collapse the double blank line into a
single blank line).
In `@nemoclaw-blueprint/policies/presets/stripe.yaml`:
- Around line 7-8: The file's preset entry for Stripe has an extra blank line
after the description field; open the presets/presets/stripe.yaml (look for the
description: entry in stripe.yaml) and remove the duplicate empty line so the
description is followed by a single blank line consistent with other presets
(e.g., github.yaml). Ensure spacing matches other preset files.
In `@nemoclaw-blueprint/policies/presets/websearch.yaml`:
- Around line 7-8: Remove the extra blank line after the description field in
the websearch preset so the YAML matches the formatting of other presets; open
the websearch.yaml entry, locate the description: value for the websearch preset
and delete the redundant empty line immediately following it so there is only a
single newline after the description.
🪄 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
Run ID: 78fa3578-2e72-4446-bfbc-90ed635a8c5f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
bin/lib/onboard.jsbin/lib/policies.jsbin/lib/policy-compose.jsbin/lib/policy-wizard-static.jsnemoclaw-blueprint/policies/presets/calendar.yamlnemoclaw-blueprint/policies/presets/confluence.yamlnemoclaw-blueprint/policies/presets/datadog.yamlnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/email.yamlnemoclaw-blueprint/policies/presets/github-t1.yamlnemoclaw-blueprint/policies/presets/github.yamlnemoclaw-blueprint/policies/presets/gitlab.yamlnemoclaw-blueprint/policies/presets/grafana.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/jira.yamlnemoclaw-blueprint/policies/presets/linear.yamlnemoclaw-blueprint/policies/presets/notion.yamlnemoclaw-blueprint/policies/presets/openai.yamlnemoclaw-blueprint/policies/presets/outlook.yamlnemoclaw-blueprint/policies/presets/pagerduty.yamlnemoclaw-blueprint/policies/presets/sendgrid.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/policies/presets/stripe.yamlnemoclaw-blueprint/policies/presets/teams.yamlnemoclaw-blueprint/policies/presets/telegram.yamlnemoclaw-blueprint/policies/presets/websearch.yaml
Signed-off-by: Patrick Riel <priel@nvidia.com>
Signed-off-by: Patrick Riel <priel@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bin/lib/policy-wizard-static.js (2)
491-508: Prefer reusingpolicy-composemerge logic to avoid behavior drift.
mergePresetsduplicates core composition behavior already implemented inbin/lib/policy-compose.js(including filtering semantics and warnings/missing handling). Keeping two merge implementations in parallel will diverge over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` around lines 491 - 508, The mergePresets function duplicates composition logic already implemented in the policy-compose module; replace this custom implementation by importing and reusing the canonical merge/compose function from bin/lib/policy-compose.js (instead of re-implementing filtering, naming, binaries handling, and multi-selection behavior) so behavior stays consistent and warnings/missing handling are preserved—locate the mergePresets function and change it to call the compose/merge export from policy-compose (referencing the exported function name in that file) and pass through the same selections and any needed context.
368-371: Add defensive filtering for host normalization.Line 390 calls
.toLowerCase()on hosts without type validation. While current YAML data contains only valid string hosts, defensive filtering would prevent crashes if data structure changes in the future.Consider adding type and length validation:
const hosts = Object.values(networkPolicies) .flatMap((b) => b.endpoints || []) .map((ep) => ep.host) .filter((h) => typeof h === "string" && h.length > 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` around lines 368 - 371, The hosts extraction needs defensive validation before normalizing; update the hosts creation that reads from networkPolicies and endpoints (the hosts variable derived from Object.values(networkPolicies).flatMap(b => b.endpoints || []).map(ep => ep.host)) to filter out non-string or empty values (e.g., typeof h === "string" && h.length > 0) before any normalization like .toLowerCase(), then dedupe with [...new Set(hosts)]; this prevents calling .toLowerCase() on null/undefined/non-string values when processing ep.host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/policy-wizard-static.js`:
- Around line 79-163: The review notes promptArrowSelect, FS_PRESETS, and
promptFilesystemPaths are dead/unreachable and causing lint noise; either remove
them or rename to indicate intentional unused status by prefixing with an
underscore (e.g., _promptArrowSelect, _FS_PRESETS, _promptFilesystemPaths) so
ESLint won't flag them; update any internal references if you rename, and ensure
the YAML build sites truly reflect filesystem flow being disabled if you remove
the logic entirely.
---
Nitpick comments:
In `@bin/lib/policy-wizard-static.js`:
- Around line 491-508: The mergePresets function duplicates composition logic
already implemented in the policy-compose module; replace this custom
implementation by importing and reusing the canonical merge/compose function
from bin/lib/policy-compose.js (instead of re-implementing filtering, naming,
binaries handling, and multi-selection behavior) so behavior stays consistent
and warnings/missing handling are preserved—locate the mergePresets function and
change it to call the compose/merge export from policy-compose (referencing the
exported function name in that file) and pass through the same selections and
any needed context.
- Around line 368-371: The hosts extraction needs defensive validation before
normalizing; update the hosts creation that reads from networkPolicies and
endpoints (the hosts variable derived from
Object.values(networkPolicies).flatMap(b => b.endpoints || []).map(ep =>
ep.host)) to filter out non-string or empty values (e.g., typeof h === "string"
&& h.length > 0) before any normalization like .toLowerCase(), then dedupe with
[...new Set(hosts)]; this prevents calling .toLowerCase() on
null/undefined/non-string values when processing ep.host.
🪄 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
Run ID: ad08a394-e8c5-4c0e-a45d-8730732f154a
📒 Files selected for processing (12)
bin/lib/onboard.jsbin/lib/policies.jsbin/lib/policy-compose.jsbin/lib/policy-wizard-static.jsnemoclaw-blueprint/policies/presets/docker.yamlnemoclaw-blueprint/policies/presets/email.yamlnemoclaw-blueprint/policies/presets/github.yamlnemoclaw-blueprint/policies/presets/gitlab.yamlnemoclaw-blueprint/policies/presets/huggingface.yamlnemoclaw-blueprint/policies/presets/pagerduty.yamlnemoclaw-blueprint/policies/presets/stripe.yamlnemoclaw-blueprint/policies/presets/websearch.yaml
✅ Files skipped from review due to trivial changes (7)
- nemoclaw-blueprint/policies/presets/huggingface.yaml
- nemoclaw-blueprint/policies/presets/pagerduty.yaml
- nemoclaw-blueprint/policies/presets/github.yaml
- nemoclaw-blueprint/policies/presets/websearch.yaml
- nemoclaw-blueprint/policies/presets/gitlab.yaml
- nemoclaw-blueprint/policies/presets/stripe.yaml
- nemoclaw-blueprint/policies/presets/email.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- nemoclaw-blueprint/policies/presets/docker.yaml
- bin/lib/policies.js
- bin/lib/policy-compose.js
- bin/lib/onboard.js
Comment on lines
+79
to
+163
| function promptArrowSelect(title, options, defaultIdx = 0) { | ||
| return new Promise((resolve) => { | ||
| let cursor = defaultIdx < options.length ? defaultIdx : 0; | ||
| let filter = ""; | ||
| let lineCount = 0; | ||
|
|
||
| function filtered() { | ||
| if (!filter) return options.map((o, i) => ({ ...o, origIdx: i })); | ||
| const lf = filter.toLowerCase(); | ||
| return options | ||
| .map((o, i) => ({ ...o, origIdx: i })) | ||
| .filter((o) => o.name.toLowerCase().includes(lf) || (o.desc || "").toLowerCase().includes(lf)); | ||
| } | ||
|
|
||
| function render() { | ||
| let n = 0; | ||
| const vis = filtered(); | ||
| if (cursor >= vis.length) cursor = Math.max(0, vis.length - 1); | ||
| n = writeLine(` ${C.bold}${title}${C.reset}`, n); | ||
| n = writeLine("", n); | ||
| if (vis.length === 0) { | ||
| n = writeLine(` ${C.dim}No matches — press Enter to use "${filter}" as custom value${C.reset}`, n); | ||
| } else { | ||
| for (let i = 0; i < vis.length; i++) { | ||
| const { name, desc } = vis[i]; | ||
| if (i === cursor) { | ||
| n = writeLine(` ${C.green}${C.bold}▶ ${name.padEnd(22)}${C.reset} ${C.dim}${desc || ""}${C.reset}`, n); | ||
| } else { | ||
| n = writeLine(` ${C.dim}${name.padEnd(22)} ${desc || ""}${C.reset}`, n); | ||
| } | ||
| } | ||
| } | ||
| n = writeLine("", n); | ||
| const hint = filter | ||
| ? ` ${C.dim}Filter:${C.reset} ${filter}${C.dim}▌ Esc clear · Enter confirm${C.reset}` | ||
| : ` ${C.dim}↑↓ cycle · Enter confirm · type to filter${C.reset}`; | ||
| n = writeLine(hint, n); | ||
| lineCount = n; | ||
| } | ||
|
|
||
| function renderConfirmed(name) { | ||
| process.stdout.write(`\x1b[${lineCount}A\x1b[0J`); | ||
| writeLine(` ${C.bold}${title}${C.reset}`, 0); | ||
| writeLine("", 0); | ||
| writeLine(` ${C.green}✓ ${name}${C.reset}`, 0); | ||
| writeLine("", 0); | ||
| } | ||
|
|
||
| render(); | ||
|
|
||
| const handler = (key) => { | ||
| const vis = filtered(); | ||
| if (key === "\x1b[A") { | ||
| cursor = (cursor - 1 + Math.max(vis.length, 1)) % Math.max(vis.length, 1); | ||
| redraw(); | ||
| } else if (key === "\x1b[B") { | ||
| cursor = (cursor + 1) % Math.max(vis.length, 1); | ||
| redraw(); | ||
| } else if (key === "\r" || key === "\n") { | ||
| rawCleanup(handler); | ||
| if (vis.length === 0 || (filter && vis[cursor]?.name.toLowerCase() !== filter.toLowerCase())) { | ||
| const label = filter || (vis.length > 0 ? vis[cursor].name : ""); | ||
| renderConfirmed(label); | ||
| resolve(filter ? { _freeText: filter } : vis.length > 0 ? vis[cursor].origIdx : null); | ||
| } else { | ||
| renderConfirmed(vis[cursor].name); | ||
| resolve(vis[cursor].origIdx); | ||
| } | ||
| } else if (key === "\x1b") { | ||
| filter = ""; cursor = 0; redraw(); | ||
| } else if (key === "\x7f" || key === "\b") { | ||
| filter = filter.slice(0, -1); cursor = 0; redraw(); | ||
| } else if (isPrintable(key)) { | ||
| filter += key; cursor = 0; redraw(); | ||
| } else if (key === "\u0003") { | ||
| rawCleanup(handler); | ||
| process.stderr.write(`\n ${C.dim}Goodbye.${C.reset}\n\n`); | ||
| process.exit(0); | ||
| } | ||
| }; | ||
|
|
||
| rawInput(handler); | ||
| function redraw() { process.stdout.write(`\x1b[${lineCount}A\x1b[0J`); render(); } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Unused wizard flows are shipped but never reachable.
promptArrowSelect, FS_PRESETS, and promptFilesystemPaths are currently dead code, and both YAML build sites hardcode empty filesystem scopes. That means the Q4 filesystem flow cannot affect output at all, while also risking lint failures for unused declarations.
♻️ Minimal wiring fix
async function run() {
@@
- const content = buildPresetYaml(outName, policies, { readOnly: [], readWrite: [] });
+ const fsAccess = await promptFilesystemPaths();
+ const content = buildPresetYaml(outName, policies, fsAccess);
@@
async function selectAndMerge() {
@@
- return buildPresetYaml(name, policies, { readOnly: [], readWrite: [] });
+ const fsAccess = await promptFilesystemPaths();
+ return buildPresetYaml(name, policies, fsAccess);
}If Q4 is intentionally out of scope, remove these helpers (or prefix with _) in this PR to keep behavior and code aligned.
As per coding guidelines, "Prefix unused variables with underscore (_) to comply with ESLint rules".
Also applies to: 224-355, 622-623, 658-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/policy-wizard-static.js` around lines 79 - 163, The review notes
promptArrowSelect, FS_PRESETS, and promptFilesystemPaths are dead/unreachable
and causing lint noise; either remove them or rename to indicate intentional
unused status by prefixing with an underscore (e.g., _promptArrowSelect,
_FS_PRESETS, _promptFilesystemPaths) so ESLint won't flag them; update any
internal references if you rename, and ensure the YAML build sites truly reflect
filesystem flow being disabled if you remove the logic entirely.
wscurran
added
Getting Started
Contributor
|
✨ Thanks for submitting this pull request, which proposes a way to simplify onboarding by introducing named presets for common integrations. |
Signed-off-by: Patrick Riel <priel@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
bin/lib/policy-wizard-static.js (2)
361-373: Consider catching parse errors for individual preset files.If any single preset file has malformed YAML,
yaml.parsewill throw and crash the entire wizard. For a more resilient UX, consider catching parse errors per-file and logging a warning while continuing with valid presets.♻️ Proposed resilient loading
function loadAllPresets() { const files = fs.readdirSync(PRESETS_DIR).filter((f) => f.endsWith(".yaml")).sort(); - return files.map((file) => { - const raw = fs.readFileSync(path.join(PRESETS_DIR, file), "utf-8"); - const doc = yaml.parse(raw); + const presets = []; + for (const file of files) { + let doc; + try { + const raw = fs.readFileSync(path.join(PRESETS_DIR, file), "utf-8"); + doc = yaml.parse(raw); + } catch (err) { + process.stderr.write(` ${C.yellow}⚠ Skipping ${file}: ${err.message}${C.reset}\n`); + continue; + } const name = doc?.preset?.name || file.replace(/\.yaml$/, ""); const networkPolicies = doc?.network_policies || {}; const hosts = Object.values(networkPolicies) .flatMap((b) => b.endpoints || []) .map((ep) => ep.host); - return { file, name, id: name, doc, hosts: [...new Set(hosts)] }; - }); + presets.push({ file, name, id: name, doc, hosts: [...new Set(hosts)] }); + } + return presets; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` around lines 361 - 373, The loadAllPresets function currently calls yaml.parse on every file and will throw on malformed YAML; wrap the per-file parsing and processing in a try/catch inside loadAllPresets (around yaml.parse and subsequent doc usage), log a warning that includes the file name and the parse error (e.g., console.warn or the project logger) and skip that file so the function continues returning valid presets (keep existing behavior for name, id, doc, hosts deduping for successful parses).
491-508: Consider reusingcomposePresetsfrom policy-compose.js to reduce duplication.This
mergePresetsfunction duplicates the core merging logic fromcomposePresetsinpolicy-compose.js. The main difference is input format (preset objects vs tool selections). Consider adapting one to call the other, or extracting shared logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-wizard-static.js` around lines 491 - 508, mergePresets duplicates merging logic already implemented by composePresets in policy-compose.js; refactor by having mergePresets convert its selections array into the same input shape composePresets expects (or extract the shared merge routine into a new helper used by both) and then call composePresets to produce policies. Specifically, map each selection {preset, level} to the preset input shape used by composePresets (preserving preset.doc.network_policies, preset.name and level), or move the loop/endpoint filtering logic (the code iterating network_policies, using filterEndpoints and building policy entries) into a shared function that both mergePresets and composePresets call so you remove the duplicate implementation.bin/lib/policy-compose.js (1)
255-263: Annotation-to-comment conversion is fragile but acceptable for controlled output.The regex-based conversion of annotation fields to YAML comments works because
yaml.stringifyproduces predictable output. However, this approach depends on the YAML serializer's formatting. Consider adding a unit test to ensure this transformation behaves correctly across yaml library updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policy-compose.js` around lines 255 - 263, The regex-based conversion of annotation fields to YAML comments (using yaml.stringify, ANNOTATION_FIELDS, annotationRe and the annotated result) can break if the yaml library changes formatting; add a unit test that constructs representative docs containing each field from ANNOTATION_FIELDS (including edge cases like leading spaces, nested mappings, and multi-line values), runs yaml.stringify(doc) and then the same replace logic that produces annotated, and asserts the output contains the expected commented annotation lines (e.g., "# <field>: <value>") and that no uncommented annotation fields remain; ensure the test fails if annotationRe no longer matches the serializer output so future changes are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/policy-compose.js`:
- Around line 216-221: The code mutates the input policies by assigning to
block.endpoints inside the for-loop; avoid in-place mutation by creating a
cloned policies object or cloned blocks before changing endpoints. Update
buildPresetYaml (or the routine that iterates over policies) to produce a new
object/blocks (e.g., map over Object.entries(policies) and return {...block,
endpoints: (block.endpoints||[]).map(...)}) or use structuredClone to copy each
block, then set the transformed endpoints on the clone so the original policies
input remains unchanged.
In `@bin/lib/policy-wizard-static.js`:
- Around line 646-666: The Ctrl+C handlers used by selectAndMerge and its
helpers (promptPresetChecklist, promptAccessLevels, warnExfil and any internal
prompt handlers) currently call process.exit(0), which prevents onboard.js from
receiving a cancellation error; instead add a CancellationError class (with
message "User cancelled" and property isCancellation = true) near the top of the
file and replace all process.exit(0) calls in those handlers with throw new
CancellationError(); this ensures selectAndMerge will propagate a catchable
error (recognized by onboard.js via isCancellation) rather than terminating the
process.
---
Nitpick comments:
In `@bin/lib/policy-compose.js`:
- Around line 255-263: The regex-based conversion of annotation fields to YAML
comments (using yaml.stringify, ANNOTATION_FIELDS, annotationRe and the
annotated result) can break if the yaml library changes formatting; add a unit
test that constructs representative docs containing each field from
ANNOTATION_FIELDS (including edge cases like leading spaces, nested mappings,
and multi-line values), runs yaml.stringify(doc) and then the same replace logic
that produces annotated, and asserts the output contains the expected commented
annotation lines (e.g., "# <field>: <value>") and that no uncommented annotation
fields remain; ensure the test fails if annotationRe no longer matches the
serializer output so future changes are caught.
In `@bin/lib/policy-wizard-static.js`:
- Around line 361-373: The loadAllPresets function currently calls yaml.parse on
every file and will throw on malformed YAML; wrap the per-file parsing and
processing in a try/catch inside loadAllPresets (around yaml.parse and
subsequent doc usage), log a warning that includes the file name and the parse
error (e.g., console.warn or the project logger) and skip that file so the
function continues returning valid presets (keep existing behavior for name, id,
doc, hosts deduping for successful parses).
- Around line 491-508: mergePresets duplicates merging logic already implemented
by composePresets in policy-compose.js; refactor by having mergePresets convert
its selections array into the same input shape composePresets expects (or
extract the shared merge routine into a new helper used by both) and then call
composePresets to produce policies. Specifically, map each selection {preset,
level} to the preset input shape used by composePresets (preserving
preset.doc.network_policies, preset.name and level), or move the loop/endpoint
filtering logic (the code iterating network_policies, using filterEndpoints and
building policy entries) into a shared function that both mergePresets and
composePresets call so you remove the duplicate implementation.
🪄 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
Run ID: 9ab0232f-fb21-42a6-828b-cfe0bdfbdc95
📒 Files selected for processing (2)
bin/lib/policy-compose.jsbin/lib/policy-wizard-static.js
Comment on lines
+216
to
+221
| for (const block of Object.values(policies)) { | ||
| block.endpoints = (block.endpoints || []).map((ep) => { | ||
| if (ep.access === "full") return ep; // tunnel — leave alone | ||
| return { ...ep, enforcement: "enforce", tls: ep.tls ?? "terminate" }; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Mutation of input policies object can cause subtle bugs.
The loop mutates the input policies object by reassigning block.endpoints. While the current callers may not reuse policies after calling buildPresetYaml, this is a fragile assumption that can lead to bugs if the function is used differently in the future.
🛡️ Proposed fix: clone blocks before modification
// Re-stamp enforcement + tls on each REST endpoint (clone to avoid mutating input).
- for (const block of Object.values(policies)) {
- block.endpoints = (block.endpoints || []).map((ep) => {
+ const clonedPolicies = {};
+ for (const [key, block] of Object.entries(policies)) {
+ const endpoints = (block.endpoints || []).map((ep) => {
if (ep.access === "full") return ep; // tunnel — leave alone
return { ...ep, enforcement: "enforce", tls: ep.tls ?? "terminate" };
});
+ clonedPolicies[key] = { ...block, endpoints };
}
const doc = {
preset: { name: presetName },
// ... rest unchanged ...
- network_policies: policies,
+ network_policies: clonedPolicies,
};📝 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.
Suggested change
| for (const block of Object.values(policies)) { | |
| block.endpoints = (block.endpoints || []).map((ep) => { | |
| if (ep.access === "full") return ep; // tunnel — leave alone | |
| return { ...ep, enforcement: "enforce", tls: ep.tls ?? "terminate" }; | |
| }); | |
| } | |
| // Re-stamp enforcement + tls on each REST endpoint (clone to avoid mutating input). | |
| const clonedPolicies = {}; | |
| for (const [key, block] of Object.entries(policies)) { | |
| const endpoints = (block.endpoints || []).map((ep) => { | |
| if (ep.access === "full") return ep; // tunnel — leave alone | |
| return { ...ep, enforcement: "enforce", tls: ep.tls ?? "terminate" }; | |
| }); | |
| clonedPolicies[key] = { ...block, endpoints }; | |
| } | |
| const doc = { | |
| preset: { name: presetName }, | |
| policies: clonedPolicies, | |
| network_policies: clonedPolicies, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/policy-compose.js` around lines 216 - 221, The code mutates the input
policies by assigning to block.endpoints inside the for-loop; avoid in-place
mutation by creating a cloned policies object or cloned blocks before changing
endpoints. Update buildPresetYaml (or the routine that iterates over policies)
to produce a new object/blocks (e.g., map over Object.entries(policies) and
return {...block, endpoints: (block.endpoints||[]).map(...)}) or use
structuredClone to copy each block, then set the transformed endpoints on the
clone so the original policies input remains unchanged.
Comment on lines
+646
to
+666
| async function selectAndMerge() { | ||
| const allPresets = loadAllPresets(); | ||
| if (allPresets.length === 0) throw new Error(`No preset files found in ${PRESETS_DIR}`); | ||
|
|
||
| const selected = await promptPresetChecklist(allPresets); | ||
| const accessMap = await promptAccessLevels(selected); | ||
| const selections = selected.map((preset) => ({ preset, level: accessMap[preset.id] })); | ||
|
|
||
| const policies = mergePresets(selections); | ||
|
|
||
| if (Object.keys(policies).length === 0) { | ||
| throw new Error("No endpoints remained after filtering. Try write access or check preset files."); | ||
| } | ||
|
|
||
| await warnExfil(policies); | ||
|
|
||
| renderMergedReview(policies); | ||
|
|
||
| const name = selections.map(({ preset, level }) => `${preset.name}-${level[0]}`).join("--"); | ||
| return buildPresetYaml(name, policies, { readOnly: [], readWrite: [] }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how onboard.js expects to handle cancellation from selectAndMerge
rg -n -B2 -A8 'selectAndMerge' bin/lib/onboard.jsRepository: NVIDIA/NemoClaw
Length of output: 1006
🏁 Script executed:
#!/bin/bash
# Check what happens in the handlers that selectAndMerge depends on
rg -n -B3 -A5 'process\.exit\(0\)' bin/lib/policy-wizard-static.jsRepository: NVIDIA/NemoClaw
Length of output: 2440
🏁 Script executed:
#!/bin/bash
# Check function context for each Ctrl+C handler
for line in 153 208 317 342 444 471; do
echo "=== Context around line $line ==="
sed -n "$((line-15)),$((line+2))p" bin/lib/policy-wizard-static.js | grep -E "^(async )?function|^const .* = \(|^const .* = async" | tail -1
doneRepository: NVIDIA/NemoClaw
Length of output: 250
🏁 Script executed:
#!/bin/bash
# View context to identify functions
sed -n '130,160p' bin/lib/policy-wizard-static.js | head -35
echo "---"
sed -n '185,215p' bin/lib/policy-wizard-static.js | head -35
echo "---"
sed -n '295,325p' bin/lib/policy-wizard-static.js | head -35Repository: NVIDIA/NemoClaw
Length of output: 3796
🏁 Script executed:
#!/bin/bash
# Find function names by looking further back
echo "=== Function around line 153 ==="
sed -n '80,160p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 208 ==="
sed -n '110,215p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 317 ==="
sed -n '250,325p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 342 ==="
sed -n '275,350p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 444 ==="
sed -n '375,450p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 471 ==="
sed -n '402,480p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3
echo "=== Function around line 564 ==="
sed -n '495,570p' bin/lib/policy-wizard-static.js | grep -E "^(async )?function |^const .* = " | tail -3Repository: NVIDIA/NemoClaw
Length of output: 444
🏁 Script executed:
#!/bin/bash
# View file structure to understand function organization
echo "=== All function definitions ==="
rg -n "^(async )?function |^const .* = (async )?\(" bin/lib/policy-wizard-static.js | head -30Repository: NVIDIA/NemoClaw
Length of output: 685
🏁 Script executed:
#!/bin/bash
# View selectAndMerge function to see what it calls
sed -n '646,680p' bin/lib/policy-wizard-static.jsRepository: NVIDIA/NemoClaw
Length of output: 1055
Replace process.exit(0) with thrown cancellation errors in Ctrl+C handlers.
When selectAndMerge is called from onboard.js, Ctrl+C handlers in promptPresetChecklist (line 447), promptAccessLevels (line 211), warnExfil (line 564), and internal dependencies call process.exit(0), which terminates the entire process rather than allowing the caller to handle cancellation gracefully. The caller (onboard.js lines 3099-3101 and 3230-3232) explicitly checks for errors with an isCancellation property but never receives them.
🛡️ Proposed fix: throw cancellation error instead of exit
Create a helper and use it in handlers:
// Add near the top of the file
class CancellationError extends Error {
constructor() {
super("User cancelled");
this.isCancellation = true;
}
}
// Then in each Ctrl+C handler (multiple locations), replace:
// process.exit(0);
// with:
// throw new CancellationError();This allows onboard.js to catch and handle the cancellation as intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/policy-wizard-static.js` around lines 646 - 666, The Ctrl+C handlers
used by selectAndMerge and its helpers (promptPresetChecklist,
promptAccessLevels, warnExfil and any internal prompt handlers) currently call
process.exit(0), which prevents onboard.js from receiving a cancellation error;
instead add a CancellationError class (with message "User cancelled" and
property isCancellation = true) near the top of the file and replace all
process.exit(0) calls in those handlers with throw new CancellationError(); this
ensures selectAndMerge will propagate a catchable error (recognized by
onboard.js via isCancellation) rather than terminating the process.
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
2 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
Adds a preset-based policy selector to the onboarding wizard (Step 7) so users can pick named integration presets (Slack, GitHub, Stripe, etc.) rather than crafting raw network policies by hand. Each preset now carries
exfil_riskannotations to surface data-exfiltration risk at selection time.Related Issue
Changes
bin/lib/policy-wizard-static.js,bin/lib/onboard.js)bin/lib/policy-compose.jsto merge selected presets into a composed policycalendar,confluence,datadog,email,github,gitlab,grafana,linear,notion,openai,outlook,pagerduty,sendgrid,stripe,teams,websearch)exfil_riskmetadata (low / medium / high)Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Patrick Riel priel@nvidia.com
Signed-off-by: Patrick Riel priel@nvidia.com
Summary by CodeRabbit
New Features
New Presets
Improvements