Open
Conversation
Add completion command supporting bash, zsh, and fish shells. Completes issue NVIDIA#155.
- Add 'nemoclaw update' command to check for updates - Add 'nemoclaw update --yes' to update without prompting - Add 'nemoclaw update --force' to force update check - Detects running from source and suggests 'git pull' instead - Uses npm registry and GitHub releases for version check Closes NVIDIA#642
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new self-update feature: a standalone updater module that compares the installed CLI version to npm and GitHub releases and, when requested, downloads and runs an install script; integrates this as a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "nemoclaw CLI"
participant Update as "bin/lib/update.js"
participant NPM as "npm Registry"
participant GitHub as "GitHub Releases"
participant Shell as "bash / shell"
User->>CLI: nemoclaw update [--force] [--yes]
CLI->>Update: runUpdate({force, yes})
Update->>Update: getCurrentVersion() / getCurrentCliPath()
Update->>NPM: getLatestNpmVersion()
NPM-->>Update: npm version
Update->>GitHub: getLatestVersion()
GitHub-->>Update: release version
Update->>Update: select higher latest, compare semver
alt updateAvailable or force
alt not yes
Update->>User: prompt for confirmation
User-->>Update: confirm/deny
end
alt confirmed
Update->>Update: download install.sh (fetchUrl)
Update->>Update: compute SHA-256 prefix, write exec script
Update->>Shell: execute install.sh (bash) with inherited stdio
Shell-->>Update: exit status
Update->>Update: re-read local version, cleanup temp
Update->>User: report success/failure
end
else no update
Update->>User: report already up-to-date
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 7
🧹 Nitpick comments (1)
bin/nemoclaw.js (1)
438-441: Inconsistent help text formatting.Other section headers use the green color format
${G}Section:${R}(e.g., "Getting Started:", "Sandbox Management:"), but "Update:" is plain text.✨ Proposed fix for consistent styling
- Update: - nemoclaw update Check for updates - nemoclaw update --yes Update to latest version + ${G}Update:${R} + nemoclaw update Check for updates + nemoclaw update --yes Update to latest version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 438 - 441, Replace the plain "Update:" help header with the colored format used elsewhere by changing the displayed header string from "Update:" to `${G}Update:${R}` in the help output generator (the block that prints the commands and headers where "Update:" currently appears); ensure the rest of the two lines under that header remain unchanged and preserve spacing/indentation to match other sections like "Getting Started:" and "Sandbox Management:".
🤖 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/update.js`:
- Around line 207-209: getCurrentVersion() can return a cached package.json
value after the installer runs; update the code so getCurrentVersion() returns
the fresh version (either by clearing Node's require cache for the package.json
module before calling require, e.g. delete
require.cache[require.resolve("../../package.json")], or by changing
getCurrentVersion() to read and JSON.parse fs.readFileSync of the package.json
path), then call that refreshed getCurrentVersion() for the success message;
ensure the change targets the getCurrentVersion() implementation (or the call
site right before console.log) so the logged v${newVersion} reflects the newly
written package.json.
- Around line 190-205: The downloaded installer is executed without integrity
verification and the cleanup uses fs.rmdirSync(tmpDir) which will fail if the
installer created files; update the flow around fetchUrl/INSTALL_SCRIPT_URL and
execSync to first obtain and verify the script's SHA256 (either by comparing
against an embedded expected hash or by fetching a .sha256 alongside the script)
and only proceed to write and execute the script if the checksum matches, and
change the cleanup to remove the temp dir recursively/forcibly (replace
fs.rmdirSync(tmpDir) with a recursive removal like fs.rmSync(tmpDir, {
recursive: true, force: true }) or equivalent) while ensuring errors on
verification abort before execSync is called; reference the tmpDir, scriptPath,
fetchUrl, INSTALL_SCRIPT_URL and execSync usages when making the changes.
- Around line 21-31: The versionGte function incorrectly treats prerelease tags
as numeric parts; update versionGte to either use a proper semver comparator
(e.g., import and call semver.gte with the original strings) or normalize inputs
by stripping prerelease/build suffixes before parsing (split each input on '-'
and '+' and take the first part, then split on '.' and parse ints) so that
prerelease labels like "1.0.0-beta.1" are not parsed as "0-beta"; ensure you
keep the function name versionGte and its return semantics when applying the
change.
- Around line 38-59: The fetchUrl function currently omits a User-Agent header
and follows redirects recursively without a depth limit; update fetchUrl to
include a sensible User-Agent header in the lib.get/request options (e.g.,
"User-Agent": "<your-app-name>") and add a redirect depth guard by accepting or
tracking a maxRedirects counter (e.g., maxRedirects param or internal attempts
variable) and reject if exceeded (suggest 5); replace the unbounded recursive
call fetchUrl(res.headers.location).then(...) with a call that passes/updates
the remaining redirect count and rejects with a clear error when the limit is
reached, keeping existing timeout and error handling intact.
- Around line 119-121: The source-detection in checkForUpdate is wrong: change
the runningFromSource calculation in checkForUpdate (where getCurrentCliPath()
is used) to only treat the CLI as "from source" when cliPath is falsy (i.e.,
runningFromSource = !cliPath) and remove the
cliPath.includes("node_modules/.bin") condition; also ensure any downstream
messages that instruct "use 'git pull' to update" are only shown when
runningFromSource is true so local project installs in node_modules are not
misclassified as source installs.
In `@bin/nemoclaw.js`:
- Line 484: The update command registers the --force flag in the invocation
(case "update": await update({ force: args.includes("--force"), yes:
args.includes("--yes") });) but the CLI help only documents --yes; update the
CLI help/usage text to include a short description for --force (e.g., "Bypass
update availability checks and force an update") alongside --yes so users see
both flags; locate where the update command help/usage string is defined and add
the --force entry and example usage referencing the update command and flags.
- Around line 44-48: Remove the unused constants SANDBOX_ACTIONS and SHELL_TYPES
from the file: locate the constant declarations for SANDBOX_ACTIONS and
SHELL_TYPES in bin/nemoclaw.js and delete them; if they are intended as
placeholders for future functionality, instead keep them but add a clear
explanatory comment above each declaring their intended purpose and why they
remain unused so linters/reviewers understand they’re intentional.
---
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 438-441: Replace the plain "Update:" help header with the colored
format used elsewhere by changing the displayed header string from "Update:" to
`${G}Update:${R}` in the help output generator (the block that prints the
commands and headers where "Update:" currently appears); ensure the rest of the
two lines under that header remain unchanged and preserve spacing/indentation to
match other sections like "Getting Started:" and "Sandbox Management:".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b74decfe-9077-4bca-9eb9-648a1e474858
📒 Files selected for processing (2)
bin/lib/update.jsbin/nemoclaw.js
bin/lib/update.js
Outdated
Comment on lines
+190
to
+205
| try { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-update-")); | ||
| const scriptPath = path.join(tmpDir, "install.sh"); | ||
|
|
||
| console.log(" Downloading installer..."); | ||
| const scriptContent = await fetchUrl(INSTALL_SCRIPT_URL); | ||
| fs.writeFileSync(scriptPath, scriptContent, { mode: 0o755 }); | ||
|
|
||
| console.log(" Running installer..."); | ||
| execSync(`bash "${scriptPath}"`, { | ||
| stdio: "inherit", | ||
| cwd: tmpDir | ||
| }); | ||
|
|
||
| fs.unlinkSync(scriptPath); | ||
| fs.rmdirSync(tmpDir); |
There was a problem hiding this comment.
Missing install script integrity verification.
The PR objectives state: "verify install script integrity before execution." Currently, the script is downloaded and executed without any checksum or signature verification. While HTTPS provides transport security, it doesn't protect against a compromised source.
Additionally, fs.rmdirSync(tmpDir) will fail if the installer creates additional files in the temp directory.
🔧 Proposed fix for cleanup (integrity check requires infrastructure)
- fs.unlinkSync(scriptPath);
- fs.rmdirSync(tmpDir);
+ // Clean up temp directory recursively
+ fs.rmSync(tmpDir, { recursive: true, force: true });For integrity verification, consider:
- Embedding an expected SHA256 hash and verifying before execution
- Fetching a
.sha256file alongside the script and comparing - Using GPG signature verification if available
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/update.js` around lines 190 - 205, The downloaded installer is
executed without integrity verification and the cleanup uses
fs.rmdirSync(tmpDir) which will fail if the installer created files; update the
flow around fetchUrl/INSTALL_SCRIPT_URL and execSync to first obtain and verify
the script's SHA256 (either by comparing against an embedded expected hash or by
fetching a .sha256 alongside the script) and only proceed to write and execute
the script if the checksum matches, and change the cleanup to remove the temp
dir recursively/forcibly (replace fs.rmdirSync(tmpDir) with a recursive removal
like fs.rmSync(tmpDir, { recursive: true, force: true }) or equivalent) while
ensuring errors on verification abort before execSync is called; reference the
tmpDir, scriptPath, fetchUrl, INSTALL_SCRIPT_URL and execSync usages when making
the changes.
- versionGte: strip prerelease/build suffixes before parsing - fetchUrl: add User-Agent header and 5-redirect limit - checkForUpdate: fix source detection (only when cliPath is falsy) - runUpdate: add SHA256 hash display, use fs.rmSync with recursive/force - getCurrentVersion: add clearCache param to read fresh version - bin/nemoclaw.js: remove unused SANDBOX_ACTIONS and SHELL_TYPES - help text: fix Update section formatting, add --force flag docs
Author
|
Thanks for the review! All comments addressed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bin/lib/update.js (1)
209-217:⚠️ Potential issue | 🟠 MajorInstaller integrity is still not verified before execution.
Line 210 computes SHA-256, but the value is only logged before
execSyncruns the script. There is no trusted expected hash/signature comparison, so this does not provide integrity protection.Proposed verification gate (example)
+ const EXPECTED_INSTALL_SHA256 = process.env.NEMOCLAW_INSTALL_SHA256; // or fetch trusted checksum/signature const hash = crypto.createHash("sha256").update(scriptContent).digest("hex"); - console.log(` Script SHA256: ${hash.substring(0, 16)}...`); + if (!EXPECTED_INSTALL_SHA256 || hash !== EXPECTED_INSTALL_SHA256) { + throw new Error("Installer integrity verification failed"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/update.js` around lines 209 - 217, The code computes a SHA-256 for scriptContent (variable hash) and immediately executes the scriptPath via execSync without verifying it; add a verification gate that compares the computed hash against a trusted expected value (provided either as a constant, an environment variable, or a downloaded/packaged signature) and aborts with an error/log if they differ before calling execSync; update the logic around hash, scriptPath, scriptContent, and the execSync call to perform this comparison and only run the installer when the hashes match.
🧹 Nitpick comments (1)
bin/lib/update.js (1)
136-141: Unreachable condition in source/version detection.
runningFromSourceis!cliPath(Line 133), sorunningFromSource && cliPathcan never be true. This branch is dead and should be removed or corrected.Proposed cleanup
- if (runningFromSource && cliPath) { + if (!runningFromSource && cliPath) { try { const output = execSync(`"${cliPath}" --version 2>/dev/null`, { encoding: "utf-8" }); const match = output.match(/(\d+\.\d+\.\d+)/); if (match) current = match[1]; } catch {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/update.js` around lines 136 - 141, The branch checking "if (runningFromSource && cliPath)" is unreachable because runningFromSource is set to "!cliPath"; replace the condition so the CLI version probe runs when a cliPath exists (e.g., use "if (cliPath)" or "if (!runningFromSource && cliPath)") or remove the dead block entirely; update the block around execSync/version matching (referencing runningFromSource and cliPath) accordingly to ensure the external CLI is queried only when cliPath is present.
🤖 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/update.js`:
- Around line 178-188: When no newer version is found but the user passed a
force flag, the current block still prints "A new version is available!" and
suggests using '--yes'; change the logic around the variables yes,
runningFromSource and the force flag (e.g., force) so that if force is true you
do not claim a new version exists but instead print a message like "No new
version available; --force will reinstall the current version" and, when not
runningFromSource, suggest using 'nemoclaw update --force' (and when
runningFromSource, keep the git pull hint). Update the messages and suggested
command accordingly in the code paths that reference yes and runningFromSource
so forced behavior is correctly described.
In `@bin/nemoclaw.js`:
- Around line 39-40: GLOBAL_COMMANDS includes the string "completion" but the
global dispatch switch that handles commands lacks a corresponding case so it
falls through to help(); either remove "completion" from GLOBAL_COMMANDS if it
shouldn't be a global command, or add a proper case "completion" branch in the
global dispatch (the same switch that currently calls help()) that invokes the
intended completion handler (e.g., call the existing completion implementation
or a new completionHandler function). Update the switch to include case
"completion": ... and ensure the handler is imported/defined and
returns/executes consistently with other global commands.
---
Duplicate comments:
In `@bin/lib/update.js`:
- Around line 209-217: The code computes a SHA-256 for scriptContent (variable
hash) and immediately executes the scriptPath via execSync without verifying it;
add a verification gate that compares the computed hash against a trusted
expected value (provided either as a constant, an environment variable, or a
downloaded/packaged signature) and aborts with an error/log if they differ
before calling execSync; update the logic around hash, scriptPath,
scriptContent, and the execSync call to perform this comparison and only run the
installer when the hashes match.
---
Nitpick comments:
In `@bin/lib/update.js`:
- Around line 136-141: The branch checking "if (runningFromSource && cliPath)"
is unreachable because runningFromSource is set to "!cliPath"; replace the
condition so the CLI version probe runs when a cliPath exists (e.g., use "if
(cliPath)" or "if (!runningFromSource && cliPath)") or remove the dead block
entirely; update the block around execSync/version matching (referencing
runningFromSource and cliPath) accordingly to ensure the external CLI is queried
only when cliPath is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 517b0cbb-7c4f-4433-bf0f-a76ec9f897c7
📒 Files selected for processing (2)
bin/lib/update.jsbin/nemoclaw.js
- Fix --force message when no new version (show 'reinstalling current version') - Remove unused 'completion' from GLOBAL_COMMANDS (no handler exists)
Author
|
Addressed remaining comments:
|
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nemoclaw updatecommand to check for CLI updatesnemoclaw update --yesto automatically update to latest versionnemoclaw update --forceto force update checkgit pullinsteadUsage
Testing
Related Issue
Closes #642
Summary by CodeRabbit