Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnhanced CLI and installer user experience across multiple scripts with ANSI color support gated by NO_COLOR and TTY detection, added progress indicators (spinners and step headers), implemented version flag handling, introduced async confirmation for destructive operations, and improved command output streaming with selective filtering and error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
🧹 Nitpick comments (1)
uninstall.sh (1)
293-296: Minor: Silent success in spin() branch obscures intent.The
: # spin already printed ✓comment explains the empty then-block, but a more explicit approach would improve readability.🔧 Optional: Use `||` pattern instead of if-then-else
- if spin "Removing nemoclaw npm package..." \ - npm uninstall -g --loglevel=error nemoclaw; then - : # spin already printed ✓ - else + spin "Removing nemoclaw npm package..." \ + npm uninstall -g --loglevel=error nemoclaw || \ warn "Global nemoclaw npm package not found or already removed" - fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@uninstall.sh` around lines 293 - 296, The then-branch after the call to spin(...) with npm uninstall -g --loglevel=error nemoclaw is currently empty with a comment; update the branch to be explicit: either replace the if-then-else with the recommended short-circuit pattern (spin "Removing..." npm uninstall -g --loglevel=error nemoclaw || { handle failure; }) or keep the if and add an explicit no-op/log statement (e.g., an explicit echo or a commentless noop) inside the then branch so the intent around spin()’s success is clear; refer to the spin(...) invocation and the npm uninstall -g --loglevel=error nemoclaw command to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Line 32: Remove the unused color variable C_WHITE from the install.sh color
definitions: locate the assignment line that declares C_GREEN, C_CLAW, C_BOLD,
C_DIM, C_RED, C_YELLOW, C_CYAN, C_WHITE, C_RESET and remove C_WHITE from the
comma-separated/space-separated list so only used color variables remain (e.g.,
leave C_GREEN, C_CLAW, C_BOLD, C_DIM, C_RED, C_YELLOW, C_CYAN, C_RESET).
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 154-161: The spinner color constant _spinGreen currently ignores
NO_COLOR; update its expression to also check process.env.NO_COLOR (treating any
set value as disabling color) so that when NO_COLOR is present the value is the
empty string instead of an escape code, and also apply the same NO_COLOR check
to the spinner reset code (the reset constant used after the checkmark, e.g.,
_spinReset) so no ANSI reset is emitted when colors are disabled; ensure both
symbols (_spinGreen and the reset constant) mirror the NO_COLOR handling used in
prompt.ts.
---
Nitpick comments:
In `@uninstall.sh`:
- Around line 293-296: The then-branch after the call to spin(...) with npm
uninstall -g --loglevel=error nemoclaw is currently empty with a comment; update
the branch to be explicit: either replace the if-then-else with the recommended
short-circuit pattern (spin "Removing..." npm uninstall -g --loglevel=error
nemoclaw || { handle failure; }) or keep the if and add an explicit no-op/log
statement (e.g., an explicit echo or a commentless noop) inside the then branch
so the intent around spin()’s success is clear; refer to the spin(...)
invocation and the npm uninstall -g --loglevel=error nemoclaw command to locate
and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b38311f-3079-4f0a-8a0d-cbc9fd332862
📒 Files selected for processing (7)
bin/nemoclaw.jsinstall.shnemoclaw/src/commands/onboard.tsnemoclaw/src/onboard/prompt.tstest/install-preflight.test.jstest/uninstall.test.jsuninstall.sh
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 53-58: The final banner printf in install.sh is missing the
trailing "╚══╝" block for the CLAW wordmark, causing the right edge to be
trimmed; update the last printf (the one using ${C_GREEN}${C_BOLD} ╚═╝
╚═══╝╚══════╝╚═╝ ╚═╝ ╚═════╝ ${C_RESET}${C_CLAW}${C_BOLD}╚═════╝╚══════╝╚═╝
╚═╝ ╚══╝╚══╝${C_RESET}) to include the additional "╚══╝" at the end (matching
uninstall.sh) so the CLAW segment prints fully, keeping the same color variables
(${C_CLAW}, ${C_GREEN}, ${C_BOLD}, ${C_RESET}) and formatting.
- Around line 117-125: The spin() function currently calls wait "$pid" directly
which, under set -e, can abort the script before the status is captured; change
both wait calls inside spin() so they are executed inside an if/else wrapper
that captures the exit code into the local variable status (e.g., run wait
"$pid" inside an if; in the then branch set status=0, in the else branch set
status=$?); then retain the existing success/failure printf, cat "$log" and rm
-f "$log" behavior so error logging and cleanup always run even when the
background job fails.
- Around line 252-256: The Node install runs in a child shell so the selected
Node version isn't active in the parent shell; after the existing spin call that
invokes nvm install 22 (the line that sources $NVM_DIR/nvm.sh and runs nvm
install 22), source nvm in the current shell (ensure_nvm_loaded or by sourcing
"$NVM_DIR/nvm.sh") and run nvm use 22 so the parent shell's PATH is updated;
update the script around the spin "Installing Node.js 22..." invocation to
source nvm and call nvm use 22 in the parent shell before running
ensure_supported_runtime or command_exists node.
In `@nemoclaw/src/commands/onboard.ts`:
- Around line 178-181: The error path in the onboard spinner handler is
hardcoding ANSI red escape sequences instead of using the same color constants
that respect NO_COLOR; define a new constant (e.g., _spinRed) alongside
_spinGreen/_spinReset and replace the hardcoded red sequence in the error
handler ((err: unknown) => { ... }) so the handler uses _spinRed and _spinReset
when writing the failure message and before throwing the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1e22fbb-91f5-4bc2-acfc-23e1fd5337d3
📒 Files selected for processing (2)
install.shnemoclaw/src/commands/onboard.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install.sh (1)
259-261: ReuseRECOMMENDED_NODE_MAJORhere.A constant for the recommended major already exists, so hard-coding
22again gives the installer two sources of truth for the Node target.Suggested refactor
- spin "Installing Node.js 22..." bash -c ". \"$NVM_DIR/nvm.sh\" && nvm install 22 --no-progress" + spin "Installing Node.js ${RECOMMENDED_NODE_MAJOR}..." \ + bash -c ". \"$NVM_DIR/nvm.sh\" && nvm install ${RECOMMENDED_NODE_MAJOR} --no-progress" ensure_nvm_loaded - nvm use 22 --silent + nvm use "${RECOMMENDED_NODE_MAJOR}" --silent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 259 - 261, The installer hard-codes Node major version "22" in the spin and nvm commands; change those to use the existing RECOMMENDED_NODE_MAJOR constant instead. Update the calls around spin "Installing Node.js 22..." and the subsequent nvm install and nvm use invocations to interpolate or reference RECOMMENDED_NODE_MAJOR (and adjust the human-facing message string to include it) so the script has a single source of truth (refer to the spin invocation, ensure_nvm_loaded usage, and the nvm install/use lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 452-457: The CLI flag parsing loop (for arg in "$@"; do ... case
"$arg" in) currently ignores unknown options; update the case in that loop to
add a default *) branch that prints an error including the invalid flag, calls
usage (or prints short help), and exits with a non-zero status so mistyped flags
like --noninteractive fail fast; ensure the branch references the same
variables/functions (e.g., NON_INTERACTIVE, usage) and does not allow unknown
flags to fall through.
- Around line 473-477: verify_nemoclaw may return success while warning that
PATH still needs updating, but the script then immediately calls run_onboard
which fails if the nemoclaw binary isn't actually runnable; modify the installer
flow so that after verify_nemoclaw (and before run_onboard) you explicitly check
that the nemoclaw executable is available and runnable (e.g., via command -v or
a small test invocation) and only call run_onboard when that check passes,
otherwise skip onboarding and surface the PATH/instruction warning (retain
post_install_message behavior).
---
Nitpick comments:
In `@install.sh`:
- Around line 259-261: The installer hard-codes Node major version "22" in the
spin and nvm commands; change those to use the existing RECOMMENDED_NODE_MAJOR
constant instead. Update the calls around spin "Installing Node.js 22..." and
the subsequent nvm install and nvm use invocations to interpolate or reference
RECOMMENDED_NODE_MAJOR (and adjust the human-facing message string to include
it) so the script has a single source of truth (refer to the spin invocation,
ensure_nvm_loaded usage, and the nvm install/use lines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf1da863-d310-4ca8-9c41-7792b34e5b89
📒 Files selected for processing (2)
install.shnemoclaw/src/commands/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw/src/commands/onboard.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
install.sh (1)
53-58:⚠️ Potential issue | 🟡 MinorThe install banner is still missing the last block of
CLAW.Line 58's "W" character ends with
╚══╝╚══╝(two legs), but the matching banner inuninstall.shends with╚══╝╚══╝╚══╝(three legs). This trims the right edge of the wordmark and creates visual inconsistency between the two scripts.🖼️ Proposed fix
- printf " ${C_GREEN}${C_BOLD} ╚═╝ ╚═══╝╚══════╝╚═╝ ╚═╝ ╚═════╝ ${C_RESET}${C_CLAW}${C_BOLD}╚═════╝╚══════╝╚═╝ ╚═╝ ╚══╝╚══╝${C_RESET}\n" + printf " ${C_GREEN}${C_BOLD} ╚═╝ ╚═══╝╚══════╝╚═╝ ╚═╝ ╚═════╝ ${C_RESET}${C_CLAW}${C_BOLD}╚═════╝╚══════╝╚═╝ ╚═╝ ╚══╝╚══╝╚══╝${C_RESET}\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 53 - 58, The install script's ASCII banner is missing the final "╚══╝" segment for the rightmost "CLAW" block; update the final printf line in install.sh (the printf that currently ends with "╚══╝╚══╝") to match the uninstall.sh banner by appending the missing "╚══╝" so the string becomes "╚══╝╚══╝╚══╝", keeping all existing color variables (C_GREEN, C_BOLD, C_RESET, C_CLAW) and escape sequences intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@install.sh`:
- Around line 53-58: The install script's ASCII banner is missing the final
"╚══╝" segment for the rightmost "CLAW" block; update the final printf line in
install.sh (the printf that currently ends with "╚══╝╚══╝") to match the
uninstall.sh banner by appending the missing "╚══╝" so the string becomes
"╚══╝╚══╝╚══╝", keeping all existing color variables (C_GREEN, C_BOLD, C_RESET,
C_CLAW) and escape sequences intact.
wscurran
added
priority: high
Contributor
Author
|
Opinions so far. Single color green will update later |
kjw3
force-pushed
the
feat/nemoclaw-sugar
branch
from
24a69b9 to
c4c9076
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/lib/onboard.js (1)
89-144: Consider handling theerrorevent on the spawned process.If
bashis missing or the spawn fails (e.g., ENOENT), theerrorevent fires but the promise may never resolve or may resolve with an undefined status. Adding an error handler would make this more resilient.💡 Proposed fix to handle spawn errors
child.stdout.on("data", onChunk); child.stderr.on("data", onChunk); return new Promise((resolve) => { + child.on("error", (err) => { + lines.push(`spawn error: ${err.message}`); + resolve({ status: 1, output: lines.join("\n"), sawProgress }); + }); child.on("close", (code) => { if (pending) flushLine(pending); resolve({ status: code ?? 1, output: lines.join("\n"), sawProgress }); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 89 - 144, streamSandboxCreate currently never listens for the spawned process's "error" event so if spawn fails the returned Promise can hang or return undefined status; add a child.on("error", ...) handler (near where child.stdout/onChunk and child.stderr are wired) that flushes any pending via flushLine, pushes the error.message (and/or error.code) into lines or sets a descriptive output, and resolves the Promise with a non‑zero status and sawProgress=false (or otherwise rejects consistently) so callers always get a deterministic result when spawn fails; reference streamSandboxCreate, child, flushLine, onChunk and the Promise resolution in your change.test/uninstall.test.js (1)
28-55: LGTM! Well-structured isolation test.The stub executable approach effectively isolates the uninstall script from real system state. The test validates both the
--yesflag behavior and the expected output markers.Consider cleaning up the temp directory after the test for tidiness, though this is not critical since Node's test runner and the OS will eventually clean
/tmp:💡 Optional cleanup
const output = `${result.stdout}${result.stderr}`; assert.match(output, /NemoClaw/); assert.match(output, /Claws retracted/); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/uninstall.test.js` around lines 28 - 55, Test creates a temp directory in the "--yes skips the confirmation prompt and completes successfully" test (variable tmp) but never removes it; add cleanup to remove tmp (and its fakeBin contents) after the test completes — e.g., wrap the spawnSync/assertions in try/finally or add a finally-like cleanup that calls fs.rmSync(tmp, { recursive: true, force: true }) so the temporary directory is deleted; locate the test block in test/uninstall.test.js referencing tmp, fakeBin, spawnSync and UNINSTALL_SCRIPT and insert the cleanup there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 89-144: streamSandboxCreate currently never listens for the
spawned process's "error" event so if spawn fails the returned Promise can hang
or return undefined status; add a child.on("error", ...) handler (near where
child.stdout/onChunk and child.stderr are wired) that flushes any pending via
flushLine, pushes the error.message (and/or error.code) into lines or sets a
descriptive output, and resolves the Promise with a non‑zero status and
sawProgress=false (or otherwise rejects consistently) so callers always get a
deterministic result when spawn fails; reference streamSandboxCreate, child,
flushLine, onChunk and the Promise resolution in your change.
In `@test/uninstall.test.js`:
- Around line 28-55: Test creates a temp directory in the "--yes skips the
confirmation prompt and completes successfully" test (variable tmp) but never
removes it; add cleanup to remove tmp (and its fakeBin contents) after the test
completes — e.g., wrap the spawnSync/assertions in try/finally or add a
finally-like cleanup that calls fs.rmSync(tmp, { recursive: true, force: true })
so the temporary directory is deleted; locate the test block in
test/uninstall.test.js referencing tmp, fakeBin, spawnSync and UNINSTALL_SCRIPT
and insert the cleanup there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff49e0b1-08e9-4cc2-b86b-496b60c3b119
📒 Files selected for processing (6)
bin/lib/onboard.jsbin/nemoclaw.jsinstall.shtest/install-preflight.test.jstest/uninstall.test.jsuninstall.sh
✅ Files skipped from review due to trivial changes (1)
- test/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- uninstall.sh
- install.sh
Contributor
Author
|
Rebased and polished on top of current What changed in this pass:
Validation:
Current state:
|
- ANSI Shadow NEMOCLAW ASCII art banner (install + uninstall) - Exact NVIDIA green #76B900 with truecolor detection; 256-color fallback - Pure-bash braille spinner wrapping npm/nvm slow operations - Numbered step headers [1/3], [2/3], [3/3] with dim dividers - --help and --version flags for install.sh and uninstall.sh - Bold green NemoClaw wordmark + elapsed timer at install completion - GitHub and Docs links in completion message - Motivational completion copy for new devs - Same treatment on uninstall.sh: matching banner, "Claws retracted" signoff - withSpinner<T>() helper in onboard.ts for slow validation steps - Colorized promptSelect with NVIDIA green default marker and dimmed hints - Regression tests for --help, --yes, non-TTY spinner, and interactive paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Split the ANSI Shadow NEMOCLAW header art so the NEMO half renders in NVIDIA green (#76B900) and the CLAW half in OpenClaw red (#ff4d4d), with proper 256-color fallbacks. Applies to both install.sh and uninstall.sh banners, and to the NemoClaw wordmark in print_done() / print_bye(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- onboard.ts: _spinGreen now emits "" when NO_COLOR is set (previously always emitted a 256-color code on TTYs); add _spinReset to replace the hardcoded \x1b[0m reset so no ANSI codes escape under NO_COLOR. Pattern mirrors prompt.ts. - install.sh: remove unused C_WHITE variable (definition and no-color fallback assignment). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- install.sh spin(): replace `wait "$pid"; local status=$?` with an if/else block so set -e cannot abort the script before the exit code is captured when the background job fails. - install.sh install_nodejs(): source nvm and run `nvm use 22 --silent` in the parent shell after the Node.js 22 spin so PATH is updated before `node --version` is called. - onboard.ts: add _spinRed constant (NO_COLOR-aware, mirrors _spinGreen/ _spinReset) and replace the hardcoded \x1b[1;31m/\x1b[0m in the spinner error path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Flag parsing: add *) catch-all that prints usage and errors on unknown options so mistyped flags like --noninteractive fail fast instead of silently falling through. - main(): guard run_onboard behind command_exists nemoclaw so the onboarding step is skipped (with a clear warning) when nemoclaw is installed but not yet on PATH, rather than failing with a cryptic command-not-found error. - install_nodejs(): replace hardcoded "22" in the spin message, nvm install, and nvm use calls with RECOMMENDED_NODE_MAJOR so there is a single source of truth for the target Node version. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kjw3
force-pushed
the
feat/nemoclaw-sugar
branch
from
b9afab7 to
f5b3d23
Compare
ericksoa
approved these changes
Mar 22, 2026
Contributor
|
Super cool and needed, thanks @kjw3 ! |
This was referenced Mar 22, 2026
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
3 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
install.shanduninstall.shwith truecolor and 256-color fallbacks;NO_COLORrespectednpm install,nvm install, and dependency steps with clean non-TTY fallback--help,--version, and-v; expanded help now documents the useful non-interactive onboarding environment variables$ nemoclaw <sandbox> connect,sandbox@<sandbox>$ openclaw tui)NemoClaw resources,NemoClaw providers)withSpinner<T>()helper inonboard.tsfor slow async validation stepspromptSelectupdated with NVIDIA green default marker (▶) and dimmed hints--help,--version,-v, non-TTY spinner failure output, and interactive/non-interactive install pathsInstall Header
Install Footer
Uninstall Header
Uninstall Footer
NO_COLOR=1
Test plan
node --test test/install-preflight.test.js— installer tests passnode --test test/uninstall.test.js— uninstaller tests passnode --test test/cli.test.js— help sections and dispatch tests passbash install.sh --helpshows usage and environment variables and exits 0bash uninstall.sh --helpshows usage and exits 0NO_COLOR=1 bash install.sh— no color codes in output🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
NO_COLORenvironment variable for terminal styling preferences