fix: add --ignore-scripts to npm install to unblock installs - #505
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThe pull request introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
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 `@test/install-preflight.test.js`:
- Around line 101-102: Tests currently only assert the GitHub URL in the install
command stub (the conditional using if [ "$1" = "install" ] && [ "$2" = "-g" ]
&& [[ "$*" == *"${GITHUB_INSTALL_URL}"* ]]) and so will pass even if
--ignore-scripts is missing; update the conditional and corresponding test
assertions to require the --ignore-scripts token as well (e.g., include an
additional check like [[ "$*" == *"--ignore-scripts"* ]]) and tighten the
assertions at the other referenced locations (lines around the same install stub
occurrences) so each stub/expectation verifies both the GitHub URL and the
--ignore-scripts flag is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4795b99-8e75-4f0d-9bb0-c12ebca5d823
📒 Files selected for processing (4)
.gitignoreinstall.shscripts/install.shtest/install-preflight.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
install.sh (1)
265-267: Consider adding minimal validation beforenpm link.Using
|| trueis necessary to tolerate postinstall failures per the PR objective, but it also masks complete install failures (e.g., network errors, registry unavailable). Ifnpm installfails entirely,npm linkon line 267 may behave unexpectedly.Consider checking that
node_modules/exists before proceeding:🛡️ Proposed validation
npm install --ignore-scripts || true + if [[ ! -d "$(pwd)/node_modules" ]]; then + error "npm install failed — node_modules not created" + fi repair_openclaw_tarball "$(pwd)/node_modules/openclaw" npm link🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 265 - 267, The script currently runs "npm install --ignore-scripts || true" which masks complete install failures and then proceeds to call repair_openclaw_tarball and "npm link"; add a minimal validation after the install to ensure node_modules exists and contains expected packages before running repair_openclaw_tarball or npm link: check for the presence of the node_modules directory and the openclaw package (used by repair_openclaw_tarball) and abort with an error message (non-zero exit) or skip npm link if validation fails; update the install.sh flow around the npm install, repair_openclaw_tarball, and npm link calls to gate those steps on this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@install.sh`:
- Around line 265-267: The script currently runs "npm install --ignore-scripts
|| true" which masks complete install failures and then proceeds to call
repair_openclaw_tarball and "npm link"; add a minimal validation after the
install to ensure node_modules exists and contains expected packages before
running repair_openclaw_tarball or npm link: check for the presence of the
node_modules directory and the openclaw package (used by
repair_openclaw_tarball) and abort with an error message (non-zero exit) or skip
npm link if validation fails; update the install.sh flow around the npm install,
repair_openclaw_tarball, and npm link calls to gate those steps on this
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 390ce1a7-023b-4942-8ded-8ffd395a3735
📒 Files selected for processing (2)
install.shscripts/install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
install.sh (1)
239-257: Consider trap-based cleanup for the temporary directory.If
tar xzf(line 245) ormkdir -p(line 244) fails,set -ecauses an immediate exit, leaving$tmpdirbehind. While minor (it's just a temp file), a trap ensures cleanup in all exit paths.♻️ Optional: trap-based cleanup
local tmpdir tmpdir="$(mktemp -d)" + trap 'rm -rf "$tmpdir"' RETURN if npm pack "openclaw@${openclaw_version}" --pack-destination "$tmpdir" > /dev/null 2>&1; then - local tgz="$tmpdir/openclaw-${openclaw_version}.tgz" - if [[ -f "$tgz" ]]; then + local tgz + tgz="$(find "$tmpdir" -maxdepth 1 -name 'openclaw-*.tgz' -print -quit)" + if [[ -n "$tgz" && -f "$tgz" ]]; then mkdir -p "${install_dir}/node_modules/openclaw" tar xzf "$tgz" -C "${install_dir}/node_modules/openclaw" --strip-components=1 info "openclaw pre-extracted successfully" else warn "npm pack succeeded but tarball not found" - rm -rf "$tmpdir" return 1 fi else warn "Failed to download openclaw tarball" - rm -rf "$tmpdir" return 1 fi - rm -rf "$tmpdir" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 239 - 257, Create a trap to always remove the temporary directory after tmpdir is created so failures in mkdir -p, tar xzf, npm pack, or any set -e exits don't leak it; specifically, immediately after tmpdir="$(mktemp -d)" set a trap like "trap 'rm -rf \"$tmpdir\"' EXIT" (and optionally clear the trap after the final rm -rf), then keep the existing npm pack, mkdir -p, and tar xzf logic unchanged so tmpdir is cleaned on any exit path.
🤖 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 241-251: The current install.sh block builds a tarball path using
openclaw_version which may be a semver range, causing tgz lookup to fail; update
the logic in the npm pack handling (the block using variables tmpdir,
openclaw_version, tgz, and install_dir/node_modules/openclaw) to locate the
produced tarball by searching tmpdir (e.g., using a glob or find for
openclaw-*.tgz) instead of constructing openclaw-${openclaw_version}.tgz, then
set tgz to the found file and proceed with mkdir -p and tar extraction as
before, handling the case where no match is found by issuing the warn, cleaning
tmpdir and returning 1.
---
Nitpick comments:
In `@install.sh`:
- Around line 239-257: Create a trap to always remove the temporary directory
after tmpdir is created so failures in mkdir -p, tar xzf, npm pack, or any set
-e exits don't leak it; specifically, immediately after tmpdir="$(mktemp -d)"
set a trap like "trap 'rm -rf \"$tmpdir\"' EXIT" (and optionally clear the trap
after the final rm -rf), then keep the existing npm pack, mkdir -p, and tar xzf
logic unchanged so tmpdir is cleaned on any exit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df595090-a475-46e0-9cb6-088e163dfaff
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/install-preflight.test.js
- scripts/install.sh
Contributor
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 245-247: The extraction block that uses mkdir, tar and then calls
info "openclaw pre-extracted successfully" must explicitly handle tar failures
instead of relying on set -e; check the exit status of the tar command (invoked
with tgz and install_dir/node_modules/openclaw) immediately after it and on
non-zero exit call warn with a clear message and return a non-zero status (or
exit) so the success info() is not printed; update the function containing this
block to use an explicit conditional around tar (and/or capture its exit code)
so failures are propagated correctly.
- Around line 264-277: The install script's npm link calls in install_nemoclaw
(the blocks running "npm install --ignore-scripts" and "npm link" in both the
local source and GitHub clone branches) assume a writable global npm prefix and
can fail with EACCES; before running npm install/link, either call
ensure_nvm_loaded at the start of install_nemoclaw so the user nvm/node is
active, or add a check that the current npm prefix is writable and if not set a
user-writable prefix (or configure npm to use a local prefix) so npm link
targets a writable location; update both occurrences where pre_extract_openclaw
is followed by "npm install --ignore-scripts" and "npm link" (the subshell "(cd
\"$nemoclaw_src\" && npm install --ignore-scripts && npm link)" and the earlier
branch) to perform this check or load nvm.
In `@scripts/install.sh`:
- Around line 406-411: The npm link call can fail for non-root users because the
system npm prefix may be unwritable; before running (cd "$NEMOCLAW_SRC" && npm
install --ignore-scripts && npm link) update npm to use a user-writable prefix
(e.g., run npm config set prefix "${HOME}/.npm-global") or invoke npm link with
an explicit user prefix (npm link --prefix "${HOME}/.npm-global") so the
NEMOCLAW_SRC install and the npm link step succeed for unprivileged users; apply
this change around the block that sets and uses NEMOCLAW_SRC and ensure any
created prefix directory exists and is added to PATH for subsequent commands.
- Around line 384-386: The tar extraction step using tar xzf "$tgz" -C
"${install_dir}/node_modules/openclaw" --strip-components=1 must be guarded so
failures are detected and cause the function/script to fail instead of logging
success; replace the current unguarded sequence (mkdir -p
"${install_dir}/node_modules/openclaw"; tar xzf "$tgz" ...; info "openclaw
pre-extracted successfully") with an explicit check that the tar command
succeeded (e.g. if ! tar ...; then call the error path or return/exit non-zero),
and only call info "openclaw pre-extracted successfully" after verifying tar
returned success; reference the tar invocation and the info call when applying
the change and ensure the same behavior integrates with the existing || warning
handler later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee3424d0-a993-40cb-a6bf-9d2ddb49bce5
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/install-preflight.test.js
Comment on lines
+264
to
+277
| pre_extract_openclaw "$(pwd)" || warn "Pre-extraction failed — npm install may fail if openclaw tarball is broken" | ||
| npm install --ignore-scripts | ||
| npm link | ||
| else | ||
| info "Installing NemoClaw from GitHub…" | ||
| # Revert once https://github.com/NVIDIA/NemoClaw/issues/71 is complete and the package is published | ||
| npm install -g git+https://github.com/NVIDIA/NemoClaw.git | ||
| # Clone first so we can pre-extract openclaw before npm install (GH-503). | ||
| # npm install -g git+https://... does this internally but we can't hook | ||
| # into its extraction pipeline, so we do it ourselves. | ||
| local nemoclaw_src="${HOME}/.nemoclaw/source" | ||
| rm -rf "$nemoclaw_src" | ||
| mkdir -p "$(dirname "$nemoclaw_src")" | ||
| git clone --depth 1 https://github.com/NVIDIA/NemoClaw.git "$nemoclaw_src" | ||
| pre_extract_openclaw "$nemoclaw_src" || warn "Pre-extraction failed — npm install may fail if openclaw tarball is broken" | ||
| (cd "$nemoclaw_src" && npm install --ignore-scripts && npm link) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the structure and find the install.sh file
git ls-files | grep install.sh | head -20Repository: NVIDIA/NemoClaw
Length of output: 132
🏁 Script executed:
# Read the install.sh file to understand the context around lines 264-277
wc -l install.shRepository: NVIDIA/NemoClaw
Length of output: 73
🏁 Script executed:
# Get the relevant section and context
head -n 280 install.sh | tail -n 100Repository: NVIDIA/NemoClaw
Length of output: 3944
🏁 Script executed:
# Search for any npm prefix related checks or configurations in the entire script
rg -n "npm prefix|npm config|writable|EACCES|npm link" install.shRepository: NVIDIA/NemoClaw
Length of output: 341
🏁 Script executed:
# Check if there are any npm prefix-related configurations elsewhere in the script
rg -n "HOME.*npm|\.npmrc|npm_config" install.shRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's see how npm_bin is being used in the script around those lines
sed -n '50,70p' install.shRepository: NVIDIA/NemoClaw
Length of output: 653
🏁 Script executed:
# Check the usage of npm_bin variable throughout the script
rg -n "npm_bin" install.sh -A 2 -B 2Repository: NVIDIA/NemoClaw
Length of output: 1493
🏁 Script executed:
# Let's understand the full context of the install_nemoclaw function
sed -n '230,290p' install.shRepository: NVIDIA/NemoClaw
Length of output: 2499
🏁 Script executed:
# Check if there's any documentation about npm link behavior and EACCES errors
# Also check what npm requires for link to work
web_search
npm link EACCES permission denied global prefixRepository: NVIDIA/NemoClaw
Length of output: 620
🏁 Script executed:
# Let me look at the full install_nemoclaw function and what happens after npm link
sed -n '245,310p' install.shRepository: NVIDIA/NemoClaw
Length of output: 2482
🏁 Script executed:
# Check if there are any npm prefix or config settings in .npmrc or elsewhere
find . -name ".npmrc" -o -name ".npm*" 2>/dev/null | head -20Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's understand npm link behavior by checking npm documentation expectations
# and see if there's any use of npm config set prefix in the script
rg -n "npm config set|npm prefix" install.shRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Check if there's environment variable setup for npm (like npm_config_prefix)
rg -n "npm_config|NPM_" install.shRepository: NVIDIA/NemoClaw
Length of output: 307
🏁 Script executed:
# Let's see if the script checks for write permissions anywhere
rg -n "test -w|writable|permission" install.shRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's check if nvm is being set up in the script and how it affects npm prefix
rg -n "nvm|NVM" install.sh -B 2 -A 2Repository: NVIDIA/NemoClaw
Length of output: 2164
🏁 Script executed:
# Check the beginning of the script to see overall setup and flow
head -n 100 install.shRepository: NVIDIA/NemoClaw
Length of output: 3347
🏁 Script executed:
# Look for any npm prefix configuration or checks before npm link is called
sed -n '1,280p' install.sh | grep -n "npm\|prefix\|nvm"Repository: NVIDIA/NemoClaw
Length of output: 3453
🏁 Script executed:
# Check if ensure_nvm_loaded is called before npm install and npm link
sed -n '260,280p' install.shRepository: NVIDIA/NemoClaw
Length of output: 1105
🏁 Script executed:
# Check the main entry point to see if ensure_nvm_loaded is called at script start
sed -n '360,401p' install.shRepository: NVIDIA/NemoClaw
Length of output: 1101
🏁 Script executed:
# Look for the main function and how install_nemoclaw is called
rg -n "main\|install_nemoclaw" install.sh | head -30Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's check if there's any explicit npm prefix configuration or checks
# Also check what happens with system npm vs nvm npm
sed -n '85,145p' install.shRepository: NVIDIA/NemoClaw
Length of output: 2541
🏁 Script executed:
# Check if there's any use of npm config set to configure prefix
rg -n "npm config set" install.shRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Let's understand the actual risk: check if npm prefix is guaranteed to be writable
# by examining what happens if user has system npm on PATH
rg -n "command -v npm\|which npm" install.shRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Verify the flow: does ensure_nvm_loaded get called before install_nemoclaw runs?
# And does it GUARANTEE nvm npm is used (not system npm)?
python3 << 'EOF'
# Parse the script flow to understand execution order
script_flow = [
"main() starts",
" install_nodejs() - calls ensure_nvm_loaded at end",
" ensure_supported_runtime() - uses npm from PATH (which one?)",
" install_nemoclaw() - calls npm install and npm link (which npm?)",
" verify_nemoclaw() - calls ensure_nemoclaw_shim which uses npm config get prefix",
" refresh_path() - calls ensure_nvm_loaded and adds npm bin to PATH",
]
for step in script_flow:
print(step)
print("\nCRITICAL ISSUE:")
print("- ensure_nvm_loaded() is called AFTER npm link in refresh_path()")
print("- Between install_nodejs() and install_nemoclaw(), npm command is ambiguous")
print("- If system npm is first on PATH, npm link targets system prefix (not writable)")
print("- Script validates versions but doesn't check npm prefix writability")
EOFRepository: NVIDIA/NemoClaw
Length of output: 711
Ensure npm link targets a writable prefix or explicitly load nvm before installation.
Both npm link calls at lines 266 and 277 assume the npm prefix is writable. Users with pre-existing system npm on PATH will hit EACCES errors since system npm prefixes (e.g., /usr/local) are not user-writable. Add ensure_nvm_loaded to install_nemoclaw() before npm install, or add a check that the npm prefix is writable and configure npm accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 264 - 277, The install script's npm link calls in
install_nemoclaw (the blocks running "npm install --ignore-scripts" and "npm
link" in both the local source and GitHub clone branches) assume a writable
global npm prefix and can fail with EACCES; before running npm install/link,
either call ensure_nvm_loaded at the start of install_nemoclaw so the user
nvm/node is active, or add a check that the current npm prefix is writable and
if not set a user-writable prefix (or configure npm to use a local prefix) so
npm link targets a writable location; update both occurrences where
pre_extract_openclaw is followed by "npm install --ignore-scripts" and "npm
link" (the subshell "(cd \"$nemoclaw_src\" && npm install --ignore-scripts &&
npm link)" and the earlier branch) to perform this check or load nvm.
ericksoa
force-pushed
the
fix/install-ignore-scripts-503
branch
2 times, most recently
from
March 20, 2026 20:40
0fd8b9b to
f1c4d3d
Compare
Contributor
Author
|
Running a full local E2E test pass on the latest force-push (rebased on current main, single clean commit). Test matrix:
Will post results shortly. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/install.sh (2)
384-386:⚠️ Potential issue | 🟠 MajorHandle
tarfailures explicitly.Because
pre_extract_openclawis invoked via... || warnon Line 410, Bash will not stop on a failingtaron Line 385. That lets Line 386 log success and return 0, so the fallback warning never runs.🔧 Suggested guard
if [ -n "$tgz" ] && [ -f "$tgz" ]; then - mkdir -p "${install_dir}/node_modules/openclaw" - tar xzf "$tgz" -C "${install_dir}/node_modules/openclaw" --strip-components=1 - info "openclaw pre-extracted successfully" + rm -rf "${install_dir}/node_modules/openclaw" + if mkdir -p "${install_dir}/node_modules/openclaw" \ + && tar xzf "$tgz" -C "${install_dir}/node_modules/openclaw" --strip-components=1 + then + info "openclaw pre-extracted successfully" + else + warn "Failed to extract openclaw tarball" + rm -rf "${install_dir}/node_modules/openclaw" + rm -rf "$tmpdir" + return 1 + fi elseAlso applies to: 410-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 384 - 386, The tar extraction's exit status is not checked, so the script always logs success; update the pre_extract_openclaw implementation to test the tar command (the tar xzf "$tgz" ... invocation) and bail or propagate failure before printing info "openclaw pre-extracted successfully" — e.g. if tar fails, call warn/error and return non-zero (so callers using "pre_extract_openclaw || warn" get the fallback), or explicitly check $? and handle the error path instead of unconditionally logging success.
411-411:⚠️ Potential issue | 🔴 Critical
npm linkstill assumes a writable global prefix.On the default Linux/system-npm path, Line 411 writes into npm’s global prefix, which is typically root-owned. For unprivileged users that turns this step into an
EACCESinstall blocker.🔧 Suggested direction
+NEMOCLAW_NPM_PREFIX="${HOME}/.nemoclaw/npm-global" +mkdir -p "$NEMOCLAW_NPM_PREFIX" ... -(cd "$NEMOCLAW_SRC" && npm install --ignore-scripts && cd nemoclaw && npm install --ignore-scripts && npm run build && cd .. && npm link) +(cd "$NEMOCLAW_SRC" && npm install --ignore-scripts && cd nemoclaw && npm install --ignore-scripts && npm run build && cd .. && npm link --prefix "$NEMOCLAW_NPM_PREFIX") +export PATH="$NEMOCLAW_NPM_PREFIX/bin:$PATH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` at line 411, The script currently runs npm link from within "$NEMOCLAW_SRC" which writes into npm’s global prefix (root-owned) and fails for unprivileged users; change the final step that calls npm link so it writes to a user-writable prefix instead (for example use npm link --prefix "$HOME/.npm-global" after ensuring that directory exists and is configured, or use npm install --global --prefix "$HOME/.npm-global" as an alternative). Update the call that follows the build (the npm link invocation in the command sequence inside the subshell for NEMOCLAW_SRC) to use a user-writable prefix variable (create the directory if needed) so the operation no longer requires root-owned global write access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 406-410: Don't remove the existing NEMOCLAW_SRC until the new
source is fully prepared; instead clone to a temporary path (e.g.,
"${NEMOCLAW_SRC}.tmp" or tmpdir), run pre_extract_openclaw and any npm
build/link steps against that temp clone, and only on success atomically replace
the old NEMOCLAW_SRC (rm -rf old then mv temp -> NEMOCLAW_SRC). Update the
operations that reference NEMOCLAW_SRC (the git clone, pre_extract_openclaw
invocation, and subsequent npm install/build/link steps) to use the temp path
and perform cleanup of the temp on failure to avoid leaving the system without
the original installation.
---
Duplicate comments:
In `@scripts/install.sh`:
- Around line 384-386: The tar extraction's exit status is not checked, so the
script always logs success; update the pre_extract_openclaw implementation to
test the tar command (the tar xzf "$tgz" ... invocation) and bail or propagate
failure before printing info "openclaw pre-extracted successfully" — e.g. if tar
fails, call warn/error and return non-zero (so callers using
"pre_extract_openclaw || warn" get the fallback), or explicitly check $? and
handle the error path instead of unconditionally logging success.
- Line 411: The script currently runs npm link from within "$NEMOCLAW_SRC" which
writes into npm’s global prefix (root-owned) and fails for unprivileged users;
change the final step that calls npm link so it writes to a user-writable prefix
instead (for example use npm link --prefix "$HOME/.npm-global" after ensuring
that directory exists and is configured, or use npm install --global --prefix
"$HOME/.npm-global" as an alternative). Update the call that follows the build
(the npm link invocation in the command sequence inside the subshell for
NEMOCLAW_SRC) to use a user-writable prefix variable (create the directory if
needed) so the operation no longer requires root-owned global write access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 259ae252-7d75-4064-a7a1-49c187467e69
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- install.sh
- test/install-preflight.test.js
Comment on lines
+406
to
+410
| NEMOCLAW_SRC="${HOME}/.nemoclaw/source" | ||
| rm -rf "$NEMOCLAW_SRC" | ||
| mkdir -p "$(dirname "$NEMOCLAW_SRC")" | ||
| git clone --depth 1 https://github.com/NVIDIA/NemoClaw.git "$NEMOCLAW_SRC" | ||
| pre_extract_openclaw "$NEMOCLAW_SRC" || warn "Pre-extraction failed — npm install may fail if openclaw tarball is broken" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the scripts/install.sh file
git ls-files | grep -E "scripts/install\.sh"Repository: NVIDIA/NemoClaw
Length of output: 77
🏁 Script executed:
# Read the relevant lines around 406-410
wc -l scripts/install.shRepository: NVIDIA/NemoClaw
Length of output: 81
🏁 Script executed:
# Get the context around lines 406-410
sed -n '400,420p' scripts/install.shRepository: NVIDIA/NemoClaw
Length of output: 989
🏁 Script executed:
# Check the entire section dealing with nemoclaw to understand the broader context
rg -n "NEMOCLAW_SRC" scripts/install.sh -A 5 -B 5Repository: NVIDIA/NemoClaw
Length of output: 1168
🏁 Script executed:
# Search for npm link usage in the script
rg -n "npm link" scripts/install.sh -A 2 -B 2Repository: NVIDIA/NemoClaw
Length of output: 703
🏁 Script executed:
# Check for error handling context - is set -e enabled?
head -50 scripts/install.sh | grep -E "set -e|set -o|trap"Repository: NVIDIA/NemoClaw
Length of output: 76
🏁 Script executed:
# Look for pre_extract_openclaw function definition
rg -n "pre_extract_openclaw" scripts/install.sh -B 30 | head -60Repository: NVIDIA/NemoClaw
Length of output: 2125
🏁 Script executed:
# Check if there's any error checking around npm operations
sed -n '410,430p' scripts/install.shRepository: NVIDIA/NemoClaw
Length of output: 997
Delete the directory only after the new installation is complete, not before.
With set -e active, if git clone fails at line 415, the old nemoclaw installation is already deleted and the script exits—leaving the system in a broken state. Even if clone succeeds, transient failures during npm install, npm run build, or npm link will cause the script to exit with the old installation already removed.
🔧 Suggested approach
NEMOCLAW_SRC="${HOME}/.nemoclaw/source"
-rm -rf "$NEMOCLAW_SRC"
mkdir -p "$(dirname "$NEMOCLAW_SRC")"
-git clone --depth 1 https://github.com/NVIDIA/NemoClaw.git "$NEMOCLAW_SRC"
-pre_extract_openclaw "$NEMOCLAW_SRC" || warn "Pre-extraction failed — npm install may fail if openclaw tarball is broken"
-(cd "$NEMOCLAW_SRC" && npm install --ignore-scripts && cd nemoclaw && npm install --ignore-scripts && npm run build && cd .. && npm link)
+NEMOCLAW_STAGING="$(mktemp -d "${HOME}/.nemoclaw/source.XXXXXX")"
+git clone --depth 1 https://github.com/NVIDIA/NemoClaw.git "$NEMOCLAW_STAGING"
+pre_extract_openclaw "$NEMOCLAW_STAGING" || warn "Pre-extraction failed — npm install may fail if openclaw tarball is broken"
+(cd "$NEMOCLAW_STAGING" && npm install --ignore-scripts && cd nemoclaw && npm install --ignore-scripts && npm run build)
+rm -rf "$NEMOCLAW_SRC"
+mv "$NEMOCLAW_STAGING" "$NEMOCLAW_SRC"
+(cd "$NEMOCLAW_SRC" && npm link)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.sh` around lines 406 - 410, Don't remove the existing
NEMOCLAW_SRC until the new source is fully prepared; instead clone to a
temporary path (e.g., "${NEMOCLAW_SRC}.tmp" or tmpdir), run pre_extract_openclaw
and any npm build/link steps against that temp clone, and only on success
atomically replace the old NEMOCLAW_SRC (rm -rf old then mv temp ->
NEMOCLAW_SRC). Update the operations that reference NEMOCLAW_SRC (the git clone,
pre_extract_openclaw invocation, and subsequent npm install/build/link steps) to
use the temp path and perform cleanup of the temp on failure to avoid leaving
the system without the original installation.
Contributor
|
LGTM and passed on local and GH install paths |
The openclaw npm tarball is missing directory entries for extensions/, skills/, and dist/plugin-sdk/config/. npm's tar extractor hard-fails on these, and the @whiskeysockets/baileys postinstall then crashes with `spawn sh ENOENT`, killing the entire install. Fix: pre-extract openclaw into node_modules using system tar (which handles missing directory entries) BEFORE npm install, so npm sees the dependency is already satisfied and skips the broken download. Both install scripts now clone the repo, pre-extract openclaw, then run `npm install --ignore-scripts` + build + `npm link` instead of the previous `npm install -g git+url` which we can't hook into. The pre-extraction is non-fatal — if it fails, the install continues and will succeed once the upstream tarball is fixed. Fixes #503
ericksoa
force-pushed
the
fix/install-ignore-scripts-503
branch
from
March 20, 2026 20:56
f1c4d3d to
1b9b4fb
Compare
liveaverage
self-requested a review
March 20, 2026 21:10
liveaverage
approved these changes
Mar 20, 2026
liveaverage
left a comment
liveaverage
left a comment
Contributor
There was a problem hiding this comment.
ran e2e with success (as normal user + root, albeit with sudo present) using local repo install and GH install
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
…#503) (NVIDIA#505) The openclaw npm tarball is missing directory entries for extensions/, skills/, and dist/plugin-sdk/config/. npm's tar extractor hard-fails on these, and the @whiskeysockets/baileys postinstall then crashes with `spawn sh ENOENT`, killing the entire install. Fix: pre-extract openclaw into node_modules using system tar (which handles missing directory entries) BEFORE npm install, so npm sees the dependency is already satisfied and skips the broken download. Both install scripts now clone the repo, pre-extract openclaw, then run `npm install --ignore-scripts` + build + `npm link` instead of the previous `npm install -g git+url` which we can't hook into. The pre-extraction is non-fatal — if it fails, the install continues and will succeed once the upstream tarball is fixed. Fixes NVIDIA#503
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
…#503) (NVIDIA#505) The openclaw npm tarball is missing directory entries for extensions/, skills/, and dist/plugin-sdk/config/. npm's tar extractor hard-fails on these, and the @whiskeysockets/baileys postinstall then crashes with `spawn sh ENOENT`, killing the entire install. Fix: pre-extract openclaw into node_modules using system tar (which handles missing directory entries) BEFORE npm install, so npm sees the dependency is already satisfied and skips the broken download. Both install scripts now clone the repo, pre-extract openclaw, then run `npm install --ignore-scripts` + build + `npm link` instead of the previous `npm install -g git+url` which we can't hook into. The pre-extraction is non-fatal — if it fails, the install continues and will succeed once the upstream tarball is fixed. Fixes NVIDIA#503
2 tasks
wscurran
added
area: cli
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
4 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
openclawdependency with systemtarbeforenpm installto bypass npm's broken tarball extractionnpm install -g git+...to clone → pre-extract →npm install --ignore-scripts && npm link--ignore-scriptsto allnpm installcalls to prevent@whiskeysockets/baileyspostinstall crashRoot Cause
The
openclawnpm tarball (v2026.3.11) is missing directory entries forextensions/,skills/, anddist/plugin-sdk/config/. npm's tar extractor hard-fails withENOENTbecause it doesn't create parent directories implicitly. Systemtarhandles this correctly.Fix
Pre-extract openclaw into
node_modules/openclawusing systemtarbeforenpm installruns. npm sees the dependency is already satisfied and skips the broken tarball download entirely.For the GitHub install path, the installer now:
~/.nemoclaw/sourcenpm packtar(handles missing dir entries)npm install --ignore-scripts && npm linkThe pre-extraction is non-fatal — if it fails, the install continues and will succeed once the upstream tarball is fixed.
Test plan
npm testpasses (167/167)node:22-bookwormcontainer, install from branch,nemoclaw --helpexits 0nemoclaw --helpprints full usagenpm installcalls have--ignore-scriptsFixes #503
Summary by CodeRabbit