fix(install): prevent nvm/login shell from resetting PATH in subshells#651
Open
cv wants to merge 6 commits intoNVIDIA:mainfrom
Open
fix(install): prevent nvm/login shell from resetting PATH in subshells#651cv wants to merge 6 commits intoNVIDIA:mainfrom
cv wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
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>
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a ShellCheck config and small stylistic/control-flow changes across shell scripts and CI hooks: short-circuited nvm sourcing when node is on PATH, adjusted semver parsing/read usage, replaced some Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (1)
.shellcheckrc (1)
9-9: Scope ShellCheck suppressions more narrowly.Line 9 disables
SC2059andSC1091repo-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
📒 Files selected for processing (3)
.shellcheckrcinstall.shscripts/install.sh
This was referenced Mar 22, 2026
Passing '.' to pyright overrides the include/exclude paths in pyproject.toml, causing it to scan .venv and test files. Without the argument, pyright respects the configured include paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Auto-formatted by shfmt via pre-commit hooks after merging upstream/main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test_endpoint_validation.py file imports pytest which is only available as a dev dependency. Pyright in strict mode cannot resolve it, breaking the pre-push hook. Exclude test files from pyright's include paths since they have their own type discipline via pytest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 572-579: Update the script's usage output (the usage function) to
document the new -h short alias alongside --help and --version/-v; specifically,
modify the displayed options so the help text lists both `-h, --help` (and where
applicable `-v, --version`) to match the case in the option parsing block that
now accepts `-h` in addition to `--help`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9587cdde-3ef8-4e1b-89f2-dd36b75ac933
📒 Files selected for processing (5)
.pre-commit-config.yamlinstall.shnemoclaw-blueprint/pyproject.tomlscripts/backup-workspace.shscripts/install-openshell.sh
✅ Files skipped from review due to trivial changes (2)
- nemoclaw-blueprint/pyproject.toml
- scripts/backup-workspace.sh
Comment on lines
+572
to
+579
| --version | -v) | ||
| printf "nemoclaw-installer v%s\n" "$NEMOCLAW_VERSION" | ||
| exit 0 | ||
| ;; | ||
| --help | -h) | ||
| usage | ||
| exit 0 | ||
| ;; |
There was a problem hiding this comment.
Document the new -h alias in usage output.
Line 576 adds -h, but the displayed help text still lists only --help, which can confuse users.
Suggested patch
- printf " --help Show this help message and exit\n\n"
+ printf " --help, -h Show this help message and exit\n\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 572 - 579, Update the script's usage output (the
usage function) to document the new -h short alias alongside --help and
--version/-v; specifically, modify the displayed options so the help text lists
both `-h, --help` (and where applicable `-v, --version`) to match the case in
the option parsing block that now accepts `-h` in addition to `--help`.
Same issue as the pre-commit hook fix — passing '.' to pyright overrides the include/exclude paths in pyproject.toml, causing it to scan .venv and site-packages (17k+ errors in CI). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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
bash -lc→bash -cininstall_nemoclaw()subshells. The login shell flag sourced/etc/profilewhich runspath_helperon macOS, resetting PATH to system defaults and hiding the caller's binaries.nvm.shsourcing when node is already on PATH.ensure_nvm_loaded()unconditionally sourcednvm.sh, which also resets PATH — breaking test stubs and unnecessarily mutating the environment.info/warnhelpers viadeclare -fsopre_extract_openclawcan use them in the subshell.version_gte()— useread -rainstead of unquoted word splitting..shellcheckrcto suppress SC2059 (printf format string with color vars) and SC1091 (dynamically sourced files)..arg from pyright invocations in Makefile and pre-commit hook (overridespyproject.tomlinclude/exclude, scanning.venv), and excludetest_*.pyfiles from strict type-checking since they import pytest which is a dev-only dependency.Why these tests failed locally but passed in CI
CI runs on
ubuntu-latestwith a minimal shell profile — no nvm, nopath_helper. On macOS with nvm installed, bothbash -l(via/etc/profile) andensure_nvm_loaded(vianvm.sh) reset PATH, causing the fake node/npm/git stubs ininstall-preflight.test.jsto be overridden by real system binaries.Test plan
npx vitest run— all 27 test files pass (303 tests)npx shellcheck install.sh scripts/install.sh— clean🤖 Generated with Claude Code