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>
📝 WalkthroughWalkthroughAdded a new interactive preset selector helper that supports both TUI (terminal user interface) mode with keyboard navigation and a non-TTY fallback. Updated interactive policy preset selection to use this new selector, seeding it with currently applied and suggested presets. Changes
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)
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
| const selected = new Set(initialSelected); | ||
| const n = allPresets.length; |
There was a problem hiding this comment.
Guard the zero-preset case before entering raw mode.
policies.listPresets() can legitimately return an empty array, and with n === 0 the navigation handlers do modulo-by-zero while Space dereferences allPresets[cursor].name. That can throw after raw mode is enabled and leave the terminal in a bad state.
Suggested fix
async function presetsCheckboxSelector(allPresets, initialSelected) {
const selected = new Set(initialSelected);
const n = allPresets.length;
+ if (n === 0) {
+ console.log(" No policy presets are available.");
+ return [];
+ }
// ── Fallback: non-TTY (piped input) ──────────────────────────────
if (!process.stdin.isTTY) {Also applies to: 3243-3256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 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).
| if (!process.stdin.isTTY) { | ||
| console.log(""); | ||
| console.log(" Available policy presets:"); | ||
| allPresets.forEach((p) => { | ||
| const marker = selected.has(p.name) ? "[\x1b[32m✓\x1b[0m]" : "[ ]"; | ||
| console.log(` ${marker} ${p.name.padEnd(14)} — ${p.description}`); | ||
| }); |
There was a problem hiding this comment.
Only use the redraw UI when both terminal streams support it.
This path writes cursor-control and color escapes directly to process.stdout, so checking only process.stdin.isTTY will dump raw ANSI into redirected or tee'd output. The hardcoded green checkmarks also bypass the existing USE_COLOR gate.
Suggested fix
- if (!process.stdin.isTTY) {
+ const canUseTui =
+ process.stdin.isTTY &&
+ process.stdout.isTTY &&
+ typeof process.stdin.setRawMode === "function";
+ if (!canUseTui) {
console.log("");
console.log(" Available policy presets:");
allPresets.forEach((p) => {
- const marker = selected.has(p.name) ? "[\x1b[32m✓\x1b[0m]" : "[ ]";
+ const marker = selected.has(p.name)
+ ? USE_COLOR
+ ? "[\x1b[32m✓\x1b[0m]"
+ : "[✓]"
+ : "[ ]";
console.log(` ${marker} ${p.name.padEnd(14)} — ${p.description}`);
});
@@
const renderLines = () => {
const lines = [" Available policy presets:"];
allPresets.forEach((p, i) => {
- const check = selected.has(p.name) ? "[\x1b[32m✓\x1b[0m]" : "[ ]";
+ const check = selected.has(p.name)
+ ? USE_COLOR
+ ? "[\x1b[32m✓\x1b[0m]"
+ : "[✓]"
+ : "[ ]";
const arrow = i === cursor ? ">" : " ";
lines.push(` ${arrow} ${check} ${p.name.padEnd(14)} — ${p.description}`);
});Also applies to: 3193-3225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 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).
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. |
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