Conversation
Signed-off-by: Patrick Riel <priel@nvidia.com>
Signed-off-by: Patrick Riel <priel@nvidia.com>
Signed-off-by: Patrick Riel <priel@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a selectable policy-presets UI ( Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant Onboard
end
rect rgba(255,200,200,0.5)
participant PoliciesService
end
User->>Onboard: invoke onboarding (TTY or non-TTY)
Onboard->>Onboard: call presetsCheckboxSelector(allPresets, initialSelected)
alt TTY
Onboard->>User: render TUI, accept navigation/toggle/select/all, confirm
else non-TTY
Onboard->>User: print presets, read comma-separated input
end
User-->>Onboard: selected presets
Onboard->>Onboard: compute newlySelected (exclude already applied)
loop for each preset in newlySelected
Onboard->>PoliciesService: policies.applyPreset(preset)
PoliciesService-->>Onboard: success / transient error
alt transient error
Onboard->>PoliciesService: retry up to 2 more times
end
end
Onboard->>User: final result / messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
bin/lib/onboard.js (1)
3355-3370:⚠️ Potential issue | 🟠 MajorApply only newly selected presets, through the same guarded path as the other branches.
initialSelectedis seeded fromapplied, so accepting the default selection on a reused sandbox will callpolicies.applyPresetagain for presets that are already active. This block also bypasses the existingUnimplemented/ transient failure handling, so those duplicate writes now go straight to an exception.Suggested fix
const knownNames = new Set(allPresets.map((p) => p.name)); const initialSelected = [ ...applied.filter((name) => knownNames.has(name)), ...suggestions.filter((name) => knownNames.has(name) && !applied.includes(name)), ]; const interactiveChoice = await presetsCheckboxSelector(allPresets, initialSelected); + const appliedSet = new Set(applied); if (onSelection) onSelection(interactiveChoice); if (!waitForSandboxReady(sandboxName)) { console.error(` Sandbox '${sandboxName}' was not ready for policy application.`); process.exit(1); } for (const name of interactiveChoice) { - policies.applyPreset(sandboxName, name); + if (appliedSet.has(name)) continue; + for (let attempt = 0; attempt < 3; attempt += 1) { + try { + policies.applyPreset(sandboxName, name); + break; + } catch (err) { + const message = err && err.message ? err.message : String(err); + if (message.includes("Unimplemented")) { + console.error(" OpenShell policy updates are not supported by this gateway build."); + console.error(" This is a known issue tracked in NemoClaw `#536`."); + throw err; + } + if (!message.includes("sandbox not found") || attempt === 2) { + throw err; + } + sleep(2); + } + } } return interactiveChoice; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 3355 - 3370, interactiveChoice currently includes presets already in applied and then calls policies.applyPreset directly, causing duplicate writes and bypassing the guarded handler for Unimplemented/transient errors; change this to compute newlySelected = interactiveChoice.filter(name => !applied.includes(name)) and only process those names, and invoke them via the same guarded application path used elsewhere (i.e., the helper/wrapper that handles Unimplemented/transient failures and retries) rather than calling policies.applyPreset directly; keep the existing waitForSandboxReady check and onSelection call intact.
🤖 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/onboard.js`:
- Around line 3173-3174: Guard entering raw/interactive mode when no presets
exist: before creating the Set/starting the interactive handlers, check if
allPresets.length (n) === 0 and abort/return or show a friendly message instead
of enabling raw mode; also update any key handlers that use cursor % n or access
allPresets[cursor].name (e.g., Space/navigation handlers) to avoid
modulo-by-zero by short-circuiting when n === 0. Ensure the early exit happens
prior to enabling raw mode so the terminal state is not left altered; apply the
same guard to the other interactive block that uses n (the block referenced
around the second handler group).
- Around line 3177-3183: The preset-listing currently writes raw ANSI escapes to
stdout and only checks process.stdin.isTTY; change the guard so the redraw UI
runs only when both process.stdin.isTTY and process.stdout.isTTY are true, and
honor the existing USE_COLOR (or equivalent color flag) instead of emitting
hardcoded green escapes—i.e., compute marker using the color helper only when
USE_COLOR is true (fall back to plain "[✓]" or "[ ]" without escapes when false)
and avoid writing any cursor-control/color codes when stdout is not a TTY; apply
the same change to the other similar block that renders policy presets (the code
using allPresets, selected, and marker).
---
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 3355-3370: interactiveChoice currently includes presets already in
applied and then calls policies.applyPreset directly, causing duplicate writes
and bypassing the guarded handler for Unimplemented/transient errors; change
this to compute newlySelected = interactiveChoice.filter(name =>
!applied.includes(name)) and only process those names, and invoke them via the
same guarded application path used elsewhere (i.e., the helper/wrapper that
handles Unimplemented/transient failures and retries) rather than calling
policies.applyPreset directly; keep the existing waitForSandboxReady check and
onSelection call intact.
🪄 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: ed00fcb6-fa42-4864-b420-85268f03e30b
📒 Files selected for processing (1)
bin/lib/onboard.js
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Nice work on this UX upgrade, @cheese-head! The TUI checkbox selector is a big improvement over the old text-based flow, and the vim-style j/k bindings are a thoughtful touch. A few things I noticed while reviewing:
Bugs
-
Zero-presets crash — If
listPresets()returns an empty array,n === 0causes modulo-by-zero (NaNcursor), a TypeError onallPresets[cursor].name, and leaves the terminal stuck in raw mode. Would a guard at the top of the function make sense here?if (n === 0) { console.log(" No policy presets are available."); return []; }
-
removeAllListeners("data")is aggressive — This removes alldatalisteners on stdin, not just yours. If anything else has attached a listener, it gets clobbered. What do you think about storing the handler reference and usingremoveListener("data", onData)instead? -
No raw-mode safety net — If the process receives SIGTERM while in raw mode, the terminal stays corrupted. Would it be worth wrapping in try/catch or adding a signal handler that calls cleanup?
-
Non-TTY empty input gives no feedback — The old code printed "Skipping policy presets." when the user opted out. Now returning
[]works functionally but the user gets silence. Intentional? -
Non-TTY fallback silently drops unknown names — If someone typos a preset name in a piped scenario, it's filtered out with no warning. Worth logging unrecognized names?
Style
-
Hardcoded ANSI escapes bypass
USE_COLOR— The file already definesUSE_COLORrespectingNO_COLOR, but the new checkmarks hardcode\x1b[32m. Could these be gated onUSE_COLOR? -
Only
stdin.isTTYis checked — If stdin is a TTY but stdout is redirected (nemoclaw onboard | tee log.txt), the cursor-control escapes will appear as garbage. Might be worth checking both?
Testing
No tests yet (acknowledged in the PR). The TUI is inherently hard to test, but the non-TTY fallback and renderLines logic could be extracted and covered fairly easily.
Overall this is a solid improvement — mainly just needs the zero-presets guard and the raw-mode safety before merge. 👍
wscurran
added
priority: medium
|
✨ Thanks for submitting this pull request, which proposes a way to improve the onboarding experience by adding an interactive preset selector with keyboard navigation and auto-detection. |
Signed-off-by: Patrick Riel <priel@nvidia.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Really nice work on this, @cheese-head! The TUI selector is a big step up from the old Y/n/list flow, and I appreciate you addressing all the feedback from the first round — the zero-presets guard, SIGTERM cleanup, USE_COLOR gating, and the newlySelected retry logic all look solid.
A few small nits that don't need to block this PR — would you mind filing an issue to track them as follow-ups?
- The
[resume]path (line ~3529) doesn't have the retry/Unimplementedguard that the interactive and non-interactive paths now have process.exit(1)on Ctrl+C could beprocess.kill(process.pid, 'SIGINT')after cleanup for more conventional signal propagationrenderLinescould be extracted and unit-tested to get some coverage on the TUI rendering without needing a real TTY_setupPoliciesis dead code that could be removed in a cleanup pass
Approving as-is — thanks for the contribution! 👍
ericksoa
changed the title
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
3 participants
Summary
Users navigate presets with arrow keys or j/k, toggle with Space, select all/none with
a, and confirm with Enter. Credential-detected presets (pypi, npm, slack, etc.) are pre-checked automatically.Related Issue
Changes
presetsCheckboxSelector— a raw-mode TUI that renders a navigable checklist with green checkmarks, redraws in-place on each keystroke, and falls back to a plain text prompt when stdin is not a TTY.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
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests