fix(install): upgrade Node.js via nvm when system version is below minimum#1079
Merged
fix(install): upgrade Node.js via nvm when system version is below minimum#1079
Conversation
…nimum install_nodejs() returned early when any Node.js was found on PATH, regardless of version. On systems with Node 20 pre-installed (e.g. GitHub Actions ubuntu-24.04 runners), the installer skipped the nvm upgrade path and then ensure_supported_runtime() rejected the old version. Additionally, ensure_nvm_loaded() skipped sourcing nvm.sh when any node was on PATH, preventing the newly-installed Node 22 from being activated in the parent shell. Fixes #1078
Contributor
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughinstall.sh now forces nvm sourcing during upgrades, verifies both node and npm versions before skipping installation, and ensures nvm is invoked for upgrades; tests and onboarding curl timeouts were updated to reflect the new behavior and timing changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Installer
participant SystemNode as "system node/npm"
participant NVM as "nvm (nvm.sh / installer)"
participant Curl as "curl (remote)"
User->>Installer: run install script
Installer->>SystemNode: detect `node` and `npm` versions
alt node >= MIN_NODE_VERSION and npm >= MIN_NPM_MAJOR
SystemNode-->>Installer: acceptable versions
Installer->>User: proceed without nvm
else node present but < MIN_NODE_VERSION or npm too old
SystemNode-->>Installer: old/insufficient versions
Installer->>Installer: warn and select nvm upgrade path
Installer->>NVM: ensure_nvm_loaded --force (source nvm.sh)
Installer->>NVM: nvm install <desired node>
NVM->>Curl: download nvm installer
Curl-->>NVM: may fail (test simulates failure)
NVM-->>Installer: report download result / errors
Installer->>User: emit upgrade attempt and any failure messages
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ericksoa
added
bug
![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: 1
🤖 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 397-405: In ensure_supported_runtime(), the early return when
command_exists node and node version >= MIN_NODE_VERSION must also verify npm
meets the required minimum (e.g., MIN_NPM_VERSION or npm >=10); currently the
code returns based only on node. Change the logic in the node-present branch
(where current_version is set via node --version) to also run npm --version
(capture npm_version), and only return when version_gte "${current_version#v}"
"$MIN_NODE_VERSION" AND version_gte "${npm_version}" "$MIN_NPM_VERSION";
otherwise emit the existing warn message and fall through to the nvm upgrade
path. Ensure you reference current_version, npm_version, MIN_NODE_VERSION,
MIN_NPM_VERSION, and the function ensure_supported_runtime().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The installer now attempts to upgrade Node.js via nvm when the system version is below minimum, instead of failing immediately. Update the test to verify the new warn-and-upgrade path, with a fake curl stub to keep the test fast.
…om/NVIDIA/NemoClaw into fix/installer-node-version-upgrade
ericksoa
had a problem deploying
to
NVIDIA_API_KEY
GitHub Actions
Failure
The early return in install_nodejs() only checked the Node.js version, but ensure_supported_runtime() also requires npm >= 10. Gate the early return on both Node.js and npm meeting the minimum requirements. Addresses CodeRabbit review feedback on #1079.
2 tasks
The onboard endpoint validation sends a full inference request to verify the provider is reachable. The 20s max-time was too tight for large models like nemotron-3-super-120b-a12b on NVIDIA Endpoints, causing the probe to time out and onboard to fail in non-interactive mode. Increase connect-timeout from 5s to 10s and max-time from 20s to 60s. This only runs once during onboard, so the longer timeout is acceptable.
ericksoa
had a problem deploying
to
NVIDIA_API_KEY
GitHub Actions
Failure
When upgrading from an existing nvm with a different default version, nvm use 22 only affects the current shell. Set the nvm default alias so that subsequent shells (e.g. the e2e test harness) that source nvm.sh also get Node 22.
ericksoa
had a problem deploying
to
NVIDIA_API_KEY
GitHub Actions
Failure
Two issues preventing nemoclaw from being found after install.sh exits: 1. verify_nemoclaw() skipped shim creation when nemoclaw was found on nvm's PATH, but that PATH only exists inside install.sh's subshell. Always call ensure_nemoclaw_shim() to create ~/.local/bin/nemoclaw. 2. nvm alias default had an invalid --silent flag that silently failed, so the nvm default wasn't set for subsequent shells.
ericksoa
had a problem deploying
to
NVIDIA_API_KEY
GitHub Actions
Failure
cv
approved these changes
Mar 30, 2026
quanticsoul4772
pushed a commit
to quanticsoul4772/NemoClaw
that referenced
this pull request
Mar 30, 2026
…nimum (NVIDIA#1079) * fix(install): upgrade Node.js via nvm when system version is below minimum install_nodejs() returned early when any Node.js was found on PATH, regardless of version. On systems with Node 20 pre-installed (e.g. GitHub Actions ubuntu-24.04 runners), the installer skipped the nvm upgrade path and then ensure_supported_runtime() rejected the old version. Additionally, ensure_nvm_loaded() skipped sourcing nvm.sh when any node was on PATH, preventing the newly-installed Node 22 from being activated in the parent shell. Fixes NVIDIA#1078 * fix(test): update preflight test for nvm upgrade behavior The installer now attempts to upgrade Node.js via nvm when the system version is below minimum, instead of failing immediately. Update the test to verify the new warn-and-upgrade path, with a fake curl stub to keep the test fast. * fix(install): also check npm version before skipping nvm upgrade The early return in install_nodejs() only checked the Node.js version, but ensure_supported_runtime() also requires npm >= 10. Gate the early return on both Node.js and npm meeting the minimum requirements. Addresses CodeRabbit review feedback on NVIDIA#1079. * fix(onboard): increase endpoint probe timeout for large model inference The onboard endpoint validation sends a full inference request to verify the provider is reachable. The 20s max-time was too tight for large models like nemotron-3-super-120b-a12b on NVIDIA Endpoints, causing the probe to time out and onboard to fail in non-interactive mode. Increase connect-timeout from 5s to 10s and max-time from 20s to 60s. This only runs once during onboard, so the longer timeout is acceptable. * fix(install): set nvm default after installing Node.js 22 When upgrading from an existing nvm with a different default version, nvm use 22 only affects the current shell. Set the nvm default alias so that subsequent shells (e.g. the e2e test harness) that source nvm.sh also get Node 22. * fix(install): always create nemoclaw shim and fix nvm alias default Two issues preventing nemoclaw from being found after install.sh exits: 1. verify_nemoclaw() skipped shim creation when nemoclaw was found on nvm's PATH, but that PATH only exists inside install.sh's subshell. Always call ensure_nemoclaw_shim() to create ~/.local/bin/nemoclaw. 2. nvm alias default had an invalid --silent flag that silently failed, so the nvm default wasn't set for subsequent shells.
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…nimum (#1079) * fix(install): upgrade Node.js via nvm when system version is below minimum install_nodejs() returned early when any Node.js was found on PATH, regardless of version. On systems with Node 20 pre-installed (e.g. GitHub Actions ubuntu-24.04 runners), the installer skipped the nvm upgrade path and then ensure_supported_runtime() rejected the old version. Additionally, ensure_nvm_loaded() skipped sourcing nvm.sh when any node was on PATH, preventing the newly-installed Node 22 from being activated in the parent shell. Fixes #1078 * fix(test): update preflight test for nvm upgrade behavior The installer now attempts to upgrade Node.js via nvm when the system version is below minimum, instead of failing immediately. Update the test to verify the new warn-and-upgrade path, with a fake curl stub to keep the test fast. * fix(install): also check npm version before skipping nvm upgrade The early return in install_nodejs() only checked the Node.js version, but ensure_supported_runtime() also requires npm >= 10. Gate the early return on both Node.js and npm meeting the minimum requirements. Addresses CodeRabbit review feedback on #1079. * fix(onboard): increase endpoint probe timeout for large model inference The onboard endpoint validation sends a full inference request to verify the provider is reachable. The 20s max-time was too tight for large models like nemotron-3-super-120b-a12b on NVIDIA Endpoints, causing the probe to time out and onboard to fail in non-interactive mode. Increase connect-timeout from 5s to 10s and max-time from 20s to 60s. This only runs once during onboard, so the longer timeout is acceptable. * fix(install): set nvm default after installing Node.js 22 When upgrading from an existing nvm with a different default version, nvm use 22 only affects the current shell. Set the nvm default alias so that subsequent shells (e.g. the e2e test harness) that source nvm.sh also get Node 22. * fix(install): always create nemoclaw shim and fix nvm alias default Two issues preventing nemoclaw from being found after install.sh exits: 1. verify_nemoclaw() skipped shim creation when nemoclaw was found on nvm's PATH, but that PATH only exists inside install.sh's subshell. Always call ensure_nemoclaw_shim() to create ~/.local/bin/nemoclaw. 2. nvm alias default had an invalid --silent flag that silently failed, so the nvm default wasn't set for subsequent shells.
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…nimum (NVIDIA#1079) * fix(install): upgrade Node.js via nvm when system version is below minimum install_nodejs() returned early when any Node.js was found on PATH, regardless of version. On systems with Node 20 pre-installed (e.g. GitHub Actions ubuntu-24.04 runners), the installer skipped the nvm upgrade path and then ensure_supported_runtime() rejected the old version. Additionally, ensure_nvm_loaded() skipped sourcing nvm.sh when any node was on PATH, preventing the newly-installed Node 22 from being activated in the parent shell. Fixes NVIDIA#1078 * fix(test): update preflight test for nvm upgrade behavior The installer now attempts to upgrade Node.js via nvm when the system version is below minimum, instead of failing immediately. Update the test to verify the new warn-and-upgrade path, with a fake curl stub to keep the test fast. * fix(install): also check npm version before skipping nvm upgrade The early return in install_nodejs() only checked the Node.js version, but ensure_supported_runtime() also requires npm >= 10. Gate the early return on both Node.js and npm meeting the minimum requirements. Addresses CodeRabbit review feedback on NVIDIA#1079. * fix(onboard): increase endpoint probe timeout for large model inference The onboard endpoint validation sends a full inference request to verify the provider is reachable. The 20s max-time was too tight for large models like nemotron-3-super-120b-a12b on NVIDIA Endpoints, causing the probe to time out and onboard to fail in non-interactive mode. Increase connect-timeout from 5s to 10s and max-time from 20s to 60s. This only runs once during onboard, so the longer timeout is acceptable. * fix(install): set nvm default after installing Node.js 22 When upgrading from an existing nvm with a different default version, nvm use 22 only affects the current shell. Set the nvm default alias so that subsequent shells (e.g. the e2e test harness) that source nvm.sh also get Node 22. * fix(install): always create nemoclaw shim and fix nvm alias default Two issues preventing nemoclaw from being found after install.sh exits: 1. verify_nemoclaw() skipped shim creation when nemoclaw was found on nvm's PATH, but that PATH only exists inside install.sh's subshell. Always call ensure_nemoclaw_shim() to create ~/.local/bin/nemoclaw. 2. nvm alias default had an invalid --silent flag that silently failed, so the nvm default wasn't set for subsequent shells.
2 tasks
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…nimum (NVIDIA#1079) * fix(install): upgrade Node.js via nvm when system version is below minimum install_nodejs() returned early when any Node.js was found on PATH, regardless of version. On systems with Node 20 pre-installed (e.g. GitHub Actions ubuntu-24.04 runners), the installer skipped the nvm upgrade path and then ensure_supported_runtime() rejected the old version. Additionally, ensure_nvm_loaded() skipped sourcing nvm.sh when any node was on PATH, preventing the newly-installed Node 22 from being activated in the parent shell. Fixes NVIDIA#1078 * fix(test): update preflight test for nvm upgrade behavior The installer now attempts to upgrade Node.js via nvm when the system version is below minimum, instead of failing immediately. Update the test to verify the new warn-and-upgrade path, with a fake curl stub to keep the test fast. * fix(install): also check npm version before skipping nvm upgrade The early return in install_nodejs() only checked the Node.js version, but ensure_supported_runtime() also requires npm >= 10. Gate the early return on both Node.js and npm meeting the minimum requirements. Addresses CodeRabbit review feedback on NVIDIA#1079. * fix(onboard): increase endpoint probe timeout for large model inference The onboard endpoint validation sends a full inference request to verify the provider is reachable. The 20s max-time was too tight for large models like nemotron-3-super-120b-a12b on NVIDIA Endpoints, causing the probe to time out and onboard to fail in non-interactive mode. Increase connect-timeout from 5s to 10s and max-time from 20s to 60s. This only runs once during onboard, so the longer timeout is acceptable. * fix(install): set nvm default after installing Node.js 22 When upgrading from an existing nvm with a different default version, nvm use 22 only affects the current shell. Set the nvm default alias so that subsequent shells (e.g. the e2e test harness) that source nvm.sh also get Node 22. * fix(install): always create nemoclaw shim and fix nvm alias default Two issues preventing nemoclaw from being found after install.sh exits: 1. verify_nemoclaw() skipped shim creation when nemoclaw was found on nvm's PATH, but that PATH only exists inside install.sh's subshell. Always call ensure_nemoclaw_shim() to create ~/.local/bin/nemoclaw. 2. nvm alias default had an invalid --silent flag that silently failed, so the nvm default wasn't set for subsequent shells.
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
install_nodejs()returned early when any Node.js was found on PATH, even if the version was belowMIN_NODE_VERSION(22.16.0). Now checks the version and falls through to the nvm upgrade path when too old.ensure_nvm_loaded()skipped sourcingnvm.shwhen anynodewas on PATH, preventing newly-installed Node 22 from being activated. Added--forceflag used by the install path.Fixes #1078
Test plan
ubuntu-24.04runner (Node 20 pre-installed)Summary by CodeRabbit
Improvements
Tests