Skip to content

fix(install): align Node version, use HTTPS, fix recovery hint (Fixes #120)#121

Closed
stevenobiajulu wants to merge 1 commit intoNVIDIA:mainfrom
stevenobiajulu:fix/install-node-version-and-protocol
Closed

fix(install): align Node version, use HTTPS, fix recovery hint (Fixes #120)#121
stevenobiajulu wants to merge 1 commit intoNVIDIA:mainfrom
stevenobiajulu:fix/install-node-version-and-protocol

Conversation

@stevenobiajulu
Copy link
Copy Markdown

@stevenobiajulu stevenobiajulu commented Mar 17, 2026

Summary

Three installer consistency fixes in install.sh that reduce first-run friction for new users.

# Line Before After
1 116 nvm install 24 nvm install "$RECOMMENDED_NODE_MAJOR"
2 200 git+ssh://git@github.com/nvidia/NemoClaw.git git+https://github.com/NVIDIA/NemoClaw.git
3 233 npm install -g nemoclaw npm install -g "git+https://github.com/NVIDIA/NemoClaw.git"

Fixes #120

Detail

Node version: RECOMMENDED_NODE_MAJOR=22 is defined on line 20 but was never referenced. The Dockerfile, test Dockerfile, brev-setup.sh, scripts/install.sh, and README all use Node 22. Now the installer does too.

SSH → HTTPS: The fallback npm install -g path used git+ssh://, which requires SSH keys configured for GitHub. For a public repo, git+https:// works without auth setup. scripts/install.sh already uses HTTPS (PR #89).

Recovery hint: When nemoclaw isn't found on PATH after install, the error suggested npm install -g nemoclaw, which fails because the package isn't published to npm yet (#71). Updated to use the same GitHub URL.

Test plan

  • All 52 tests pass (npm test)
  • Pre-existing lint errors unchanged (make check — 3 TS errors in unrelated files, same on main)
  • RECOMMENDED_NODE_MAJOR=22 confirmed consistent across README, Dockerfile, test/Dockerfile.sandbox, brev-setup.sh, scripts/install.sh
  • Rebased on current main (includes PR security: verify integrity of downloaded scripts before execution #106 SHA-256 verification)
  • Manual: run ./install.sh on a machine without Node.js installed

Summary by CodeRabbit

  • Bug Fixes

    • Fixed formatting of installation instructions to properly handle shell command parsing.
  • Chores

    • Made Node.js installation configurable instead of using a fixed version.

@wscurran wscurran added Getting Started Use this label to identify setup, installation, or onboarding issues. NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). labels Mar 19, 2026
@wscurran
Copy link
Copy Markdown
Contributor

Thanks for streamlining the installer and making it easier for new users to get started with NemoClaw, these fixes will definitely improve the onboarding experience.

@wscurran wscurran added the priority: high Important issue that should be resolved in the next release label Mar 19, 2026
@cv
Copy link
Copy Markdown
Contributor

cv commented Mar 21, 2026

Hey @stevenobiajulu, thanks for tackling the Node version alignment and HTTPS fix — those install-time issues really matter for first impressions. Wanted to let you know the repo has been evolving quickly since this PR was opened, with new CI checks and a number of other changes landing. Would you be up for rebasing onto the latest main? We'd like to consider merging this but want to make sure everything lines up. Thanks!

@stevenobiajulu
Copy link
Copy Markdown
Author

Thanks @cv! Happy to rebase -- I'll get that done today and push the update.

- Use $RECOMMENDED_NODE_MAJOR (22) instead of hardcoded 24.
  Aligns with Dockerfile (node:22-slim), brev-setup.sh (setup_22.x),
  test/Dockerfile.sandbox, and the variable defined on line 20.
- Switch fallback npm install from git+ssh:// to git+https://.
  Users without GitHub SSH keys configured would fail to install.
  Matches the HTTPS URL used in scripts/install.sh (PR NVIDIA#89).
- Update recovery hint to use GitHub URL instead of `npm install -g
  nemoclaw`, which fails until the package is published (NVIDIA#71).

Signed-off-by: Steven Obiajulu <steven@usejunior.com>
@stevenobiajulu stevenobiajulu force-pushed the fix/install-node-version-and-protocol branch from f34d64d to 9731f75 Compare March 21, 2026 20:06
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2551b89e-8365-43b5-8938-0461d350f781

📥 Commits

Reviewing files that changed from the base of the PR and between b0d498c and 9731f75.

📒 Files selected for processing (1)
  • install.sh

📝 Walkthrough

Walkthrough

The changes update install.sh to use a configurable Node.js major version variable instead of a hardcoded value, and adjust the fallback installation command for NemoClaw to properly handle the GitHub URL with quotes for correct shell parsing.

Changes

Cohort / File(s) Summary
Installation Script Configuration
install.sh
Updated install_nodejs() to reference the $RECOMMENDED_NODE_MAJOR variable instead of hardcoded Node version; wrapped GitHub URL in quotes within verify_nemoclaw() remediation message to ensure proper shell command parsing.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A script so fine, now fixed with care,
Node's version flows from variables fair,
Quotes guard our GitHub path from harm,
Installation whispers its soothing charm! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the three main fixes: Node version alignment, HTTPS protocol switch, and recovery hint update, directly matching the changeset.
Linked Issues check ✅ Passed The PR fully implements all three objectives from issue #120: uses RECOMMENDED_NODE_MAJOR variable for Node.js, switches SSH to HTTPS protocol, and updates recovery hint to actual GitHub install URL.
Out of Scope Changes check ✅ Passed All changes in install.sh are directly scoped to the three issues addressed: Node version, SSH protocol, and recovery hint. No unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can customize the tone of the review comments and chat replies.

Configure the tone_instructions setting to customize the tone of the review comments and chat replies. For example, you can set the tone to Act like a strict teacher, Act like a pirate and more.

@stevenobiajulu
Copy link
Copy Markdown
Author

@cv Rebased onto latest main — conflicts resolved, ready for another look.

What changed during rebase: The original PR had 3 fixes; one (SSH → HTTPS in the fallback install path) is now redundant since upstream rewrote that block to use git clone --depth 1 https://... + npm link (#503). The rebased diff is down to 2 changes:

  1. nvm install 22nvm install "$RECOMMENDED_NODE_MAJOR" (use the variable already defined on line 20)
  2. Added quotes around the GitHub URL in the recovery hint

Verification:

  • All 187 tests pass (npm test)
  • bash -n install.sh syntax check passes
  • Manual install test: ran ./install.sh in a clean environment (no Node/nvm) — correctly installed Node v22.22.1 via nvm and completed setup
  • Signed-off-by preserved

@stevenobiajulu
Copy link
Copy Markdown
Author

Both fixes in this PR have been independently addressed upstream (Node version aligned to 22, SSH→HTTPS switched). Closing as superseded. Thanks for the review @cv @wscurran!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Getting Started Use this label to identify setup, installation, or onboarding issues. NemoClaw CLI Use this label to identify issues with the NemoClaw command-line interface (CLI). priority: high Important issue that should be resolved in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install.sh: Node version mismatch, SSH protocol, and stale recovery hint

4 participants