fix(install): use GitHub repo for curl one-liner (Fixes #54)#89
fix(install): use GitHub repo for curl one-liner (Fixes #54)#89deepujain wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
- 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>
wscurran
added
bug
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
734a841 to
5162a53
Compare
|
Rebased onto latest main to resolve conflicts in README.md and scripts/install.sh. Upstream added the nvm/fnm PATH note (kept alongside Option B) and sudo for nodesource installs (applied to the git URL). npm test passes. |
|
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:
📝 WalkthroughWalkthroughREADME adds two explicit onboarding options (Option A: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (shell)
participant Installer as install.sh
participant GitHub as GitHub repo
participant npm as npm (local)
participant System as System PATH / $NPM_PREFIX
User->>Installer: curl .../install.sh | bash
Installer->>GitHub: fetch git+https://github.com/NVIDIA/NemoClaw.git (tarball)
Installer->>npm: run npm install -g <tarball> (prefix with sudo if nodesource)
npm->>System: write executables to $NPM_PREFIX/bin (e.g., `nemoclaw`)
Installer->>System: reshims / refresh PATH
Installer->>User: verify `nemoclaw --version` and completion message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (1)
scripts/install.sh (1)
369-371: Pin the Git install target to an immutable ref to avoid installer drift.Using an unpinned git+https URL means npm will always pull the latest default branch, making global installs non-reproducible and vulnerable to unexpected upstream changes. Pin to a specific tag or commit SHA.
Suggested change
- sudo npm install -g "git+https://github.com/NVIDIA/NemoClaw.git" + sudo npm install -g "git+https://github.com/NVIDIA/NemoClaw.git#<tag-or-commit>" ... - npm install -g "git+https://github.com/NVIDIA/NemoClaw.git" + npm install -g "git+https://github.com/NVIDIA/NemoClaw.git#<tag-or-commit>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 369 - 371, Replace the unpinned git install URL used in both the sudo and non-sudo npm install commands (the lines invoking npm install -g "git+https://github.com/NVIDIA/NemoClaw.git") with a pinned reference to a specific immutable ref (either a release tag or a commit SHA), e.g. change "git+https://github.com/NVIDIA/NemoClaw.git" to "git+https://github.com/NVIDIA/NemoClaw.git#vX.Y.Z" or "git+https://github.com/NVIDIA/NemoClaw.git#<commit-sha>" so the global installer is reproducible and won't drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 87: The README sentence claiming the installer "then runs the guided
onboard wizard" is incorrect; update that line to state the installer only
installs Node.js if missing and that the guided onboarding must be run
separately via the `nemoclaw onboard` command (or similar phrasing) so users
know to run `nemoclaw onboard` to create a sandbox, configure inference, and
apply security policies rather than expecting the installer to perform those
steps automatically.
- Line 69: The heading text "Option A — Clone and run (recommended):" in
README.md is misleading given the shown command uses a curl pipe-to-bash
installer; update that label (the literal string "Option A — Clone and run
(recommended):") to a more accurate phrase such as "Installer URL" or "Run
installer script" and ensure any adjacent descriptive text reflects that the
instructions use a remote installer (curl ... | bash) rather than a git clone
workflow.
- Around line 81-82: Remove the leading "$" shell prompt markers in the README
code block or change the fence to a bash-style fenced block so lint MD014 is
satisfied; specifically update the snippet containing the curl command and the
"nemoclaw onboard" command (the lines with "curl -fsSL
https://raw.githubusercontent.com/NVIDIA/NemoClaw/main/scripts/install.sh |
bash" and "nemoclaw onboard") by either stripping the "$ " prefixes or by
wrapping them in a fenced block like ```bash to indicate shell commands without
prompt markers.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 369-371: Replace the unpinned git install URL used in both the
sudo and non-sudo npm install commands (the lines invoking npm install -g
"git+https://github.com/NVIDIA/NemoClaw.git") with a pinned reference to a
specific immutable ref (either a release tag or a commit SHA), e.g. change
"git+https://github.com/NVIDIA/NemoClaw.git" to
"git+https://github.com/NVIDIA/NemoClaw.git#vX.Y.Z" or
"git+https://github.com/NVIDIA/NemoClaw.git#<commit-sha>" so the global
installer is reproducible and won't drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b1e199d-8516-41c8-b40f-52c1dd28f514
📒 Files selected for processing (2)
README.mdscripts/install.sh
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
5162a53 to
9e9ca04
Compare
|
CodeRabbit review comment resolved |
- 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>
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
9e9ca04 to
b082927
Compare
|
Rebased on latest main. Tests pass. Ready for review! |
deepujain
force-pushed
the
fix/54-install-from-git
branch
2 times, most recently
from
58d1003 to
1e857c4
Compare
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 `@test/install-preflight.test.js`:
- Around line 417-429: The stub that simulates a global npm install (the if
block checking [ "$1" = "install" ] && [[ "$*" == *"-g"* ]]) currently accepts
any npm -g invocation; update the stub that writes "$NPM_PREFIX/bin/nemoclaw" so
it verifies the install argv contains the expected GitHub package target (e.g.
check "$*" or "$@" for the GitHub URL pattern like "github.com/OWNER/REPO" or
the exact URL used in the test) and if the URL is missing print an error and
exit with non-zero status; keep the existing --version handling but return
success only when the argv includes the GitHub URL, otherwise fail the install
stub (exit 1).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2be93ffa-2d59-45f4-987e-90d19b67792b
📒 Files selected for processing (3)
README.mdscripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
1e857c4 to
96dffed
Compare
|
Rebased on latest main (clean, no conflicts). Addressed the CodeRabbit nitpick |
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
96dffed to
a5acb82
Compare
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
a5acb82 to
193486c
Compare
|
Rebased onto the latest main and resolved the installer drift against the newer legacy-wrapper layout. The one-line README example now points at the repository-root install.sh, the wrapper stays executable, and the focused installer preflight suite passes locally ( |
|
@deepujain is this still a valid PR to try to merge? |
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
c8aee3f to
9af042e
Compare
|
Rebased this onto the latest main. This still looks valid to me, but the scope is basically docs-only now because main already picked up most of the installer-side behavior. If that narrower README cleanup is still useful, this branch should be ready for another look. |
The npm registry package is a stub; install from git so the real CLI (with bin) is used. Fixes "could not determine executable to run" after curl | bash. README: add Option B one-liner and note for nvidia.com 404. Update e2e test npm stub to handle global git install. Fixes NVIDIA#54 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: dejain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/54-install-from-git
branch
from
9af042e to
117362a
Compare
|
Rebased this onto the latest main again. The diff is still the narrow README and installer-wrapper cleanup, and the branch now contains current main. |
|
Thanks for catching this early, @deepujain — the npm stub problem was real. Installing from GitHub directly landed in the same week the repo went public ( |
3 participants
Summary
Fixes #54 (and helps #32). After running the one-line install (
curl ... | bash), the script rannpm install -g nemoclaw, which installs the stub package from the npm registry. The stub has no properbin, sonemoclaw onboardfailed with "could not determine executable to run". This change makes the curl-pipe installer install from the GitHub repo so the real CLI is used.Changes
npm install -g nemoclawwithnpm install -g "git+https://github.com/NVIDIA/NemoClaw.git". Add a short comment that the npm package is a stub until Set up Continuous Releases #71 (publishing) is done and that this fixes GTC Keynote url didn't work #54.nemoclaw onboard. Add a note that ifhttps://nvidia.com/nemoclaw.shreturns 404, use the one-line command until that endpoint is live (see Endpoint for installhttps://nvidia.com/nemoclaw.shreturns 404 error #32).Testing
npm testpasses (39 tests).curl -fsSL https://raw.githubusercontent.com/NVIDIA/NemoClaw/main/scripts/install.sh | bash(with sudo for OpenShell step), thennemoclaw onboardshould work.Fixes #54
Summary by CodeRabbit
Documentation
Bug Fixes
Tests