Skip to content

fix(install): prevent nvm/login shell from resetting PATH in subshells#651

Open
cv wants to merge 1 commit intoNVIDIA:mainfrom
cv:fix/install-subshell-helpers
Open

fix(install): prevent nvm/login shell from resetting PATH in subshells#651
cv wants to merge 1 commit intoNVIDIA:mainfrom
cv:fix/install-subshell-helpers

Conversation

@cv
Copy link
Contributor

@cv cv commented Mar 22, 2026

Summary

  • bash -lcbash -c in install_nemoclaw() subshells. The login shell flag sourced /etc/profile which runs path_helper on macOS, resetting PATH to system defaults and hiding the caller's binaries.
  • Skip nvm.sh sourcing when node is already on PATH. ensure_nvm_loaded() unconditionally sourced nvm.sh, which also resets PATH — breaking test stubs and unnecessarily mutating the environment.
  • Export info/warn helpers via declare -f so pre_extract_openclaw can use them in the subshell.
  • Fix SC2206 shellcheck warning in version_gte() — use read -ra instead of unquoted word splitting.
  • Add .shellcheckrc to suppress SC2059 (printf format string with color vars) and SC1091 (dynamically sourced files).

Why these tests failed locally but passed in CI

CI runs on ubuntu-latest with a minimal shell profile — no nvm, no path_helper. On macOS with nvm installed, both bash -l (via /etc/profile) and ensure_nvm_loaded (via nvm.sh) reset PATH, causing the fake node/npm/git stubs in install-preflight.test.js to be overridden by real system binaries.

Test plan

  • node --test test/install-preflight.test.js — all 11 tests pass (previously 4 failed)
  • node --test test/*.test.js — all 217 tests pass
  • npx shellcheck install.sh scripts/install.sh — clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores

    • Added ShellCheck linting configuration.
  • Performance

    • Optimized installation scripts for improved efficiency in dependency detection and build execution.

Two issues caused install_nemoclaw() subshells to lose the caller's
PATH, making install-preflight tests fail on macOS while passing in CI:

1. `bash -lc` sourced /etc/profile which runs path_helper on macOS,
   resetting PATH to system defaults and hiding the caller's binaries.
   Fix: replace `bash -lc` with `bash -c` — the parent shell already
   has the correct PATH, so a login shell is unnecessary.

2. `ensure_nvm_loaded()` unconditionally sourced nvm.sh, which also
   resets PATH. Fix: skip sourcing when node is already on PATH.

Also export `info` and `warn` helpers via `declare -f` so
pre_extract_openclaw can use them in the subshell.

Additionally:
- Fix SC2206 shellcheck warning in version_gte() by using `read -ra`
  instead of unquoted word splitting into arrays.
- Add .shellcheckrc to suppress SC2059 (printf format string with color
  vars) and SC1091 (dynamically sourced files) across the project.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

Added a ShellCheck configuration file to disable specific linting rules for intentional patterns in the project. Updated shell scripts to optimize NVM loading by short-circuiting when node exists on PATH, refactored semver parsing logic, and modified function declaration handling in build commands.

Changes

Cohort / File(s) Summary
ShellCheck Configuration
.shellcheckrc
Added configuration file disabling SC2059 (variables in printf format strings) and SC1091 (sourced file not following) rules, with comments explaining intentional ANSI escape sequences and dynamically resolved script sourcing.
Shell Script Updates
install.sh, scripts/install.sh
Optimized ensure_nvm_loaded() to return early if node is already on PATH, avoiding unnecessary nvm.sh sourcing. Refactored version_gte() semver parsing to use shared array declarations. Modified install_nemoclaw() to use bash -c instead of bash -lc and expanded function declarations in embedded commands.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 With whiskers twitched and nose up high,
Our scripts now parse versions spry!
NVM checks quick—no path delay,
ShellCheck warnings fade away! ✨
A tidy warren, clean and bright,
Each tweak precise, each logic right.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main fix: preventing nvm/login shell from resetting PATH in subshells, which directly addresses the core issue described in the PR objectives.

✏️ 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

CodeRabbit can approve the review once all CodeRabbit's comments are resolved.

Enable the reviews.request_changes_workflow setting to automatically approve the review once all CodeRabbit's comments are resolved.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.shellcheckrc (1)

9-9: Scope ShellCheck suppressions more narrowly.

Line 9 disables SC2059 and SC1091 repo-wide, which can mask future issues outside installer helpers. Prefer keeping global config minimal and using local # shellcheck disable=... at intentional callsites.

Example tightening
-disable=SC2059,SC1091
+disable=SC1091
# In files with intentional colorized printf format strings:
# shellcheck disable=SC2059
info() { printf "${C_CYAN}[INFO]${C_RESET}  %s\n" "$*"; }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.shellcheckrc at line 9, Remove the repo‑wide suppression of SC2059 and
SC1091 from .shellcheckrc and instead scope those disables to specific call
sites that intentionally require them (for example the colorized printf helper
like info() and any installer helper that sources generated files), by adding
local comments such as "# shellcheck disable=SC2059" or "# shellcheck
disable=SC1091" directly above the offending function or source statement; keep
the global .shellcheckrc minimal so other files still get checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.shellcheckrc:
- Line 9: Remove the repo‑wide suppression of SC2059 and SC1091 from
.shellcheckrc and instead scope those disables to specific call sites that
intentionally require them (for example the colorized printf helper like info()
and any installer helper that sources generated files), by adding local comments
such as "# shellcheck disable=SC2059" or "# shellcheck disable=SC1091" directly
above the offending function or source statement; keep the global .shellcheckrc
minimal so other files still get checked.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dee031f5-e9f3-4ee0-b586-67c22701b172

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce2905 and 8c79539.

📒 Files selected for processing (3)
  • .shellcheckrc
  • install.sh
  • scripts/install.sh

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant