fix(install): surface Node shell-reload hint adjacent to install line (#2178) - #2298
Conversation
…ll line Closes #2178. When nvm upgrades Node to v22 during install.sh, the user's parent shell still resolves node to the pre-install version until the shell is reloaded. The existing generic "source <profile>" hint at the bottom of the installer is easy to miss after the rest of the output. Surface the mismatch loudly adjacent to the "Node.js installed" line with the exact activation command. Test: install-preflight guards the hint's presence in the upgrade path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 installer captures the installed Node.js version into a local variable, logs an nvm-specific success message tied to the default alias, warns the parent shell may still resolve Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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)
test/install-preflight.test.ts (1)
1546-1548: Tighten this regression check to match executable statements, not comment text.Line 1546 can currently pass by matching the nearby comment, even if the
warncall is removed later. Consider anchoring towarn/printfstatements.Proposed assertion tightening
- expect(body).toMatch(/current shell may still resolve/); + expect(body).toMatch(/\n\s*warn\s+"Your current shell may still resolve/); - expect(body).toMatch(/exec \\"\\\$SHELL\\" -l/); + expect(body).toMatch(/\n\s*printf\s+".*exec \\"\\\$SHELL\\" -l/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.ts` around lines 1546 - 1548, The current regexes can match nearby comments; tighten them to assert the actual executable lines by including the function/statement tokens: change the first assertion to require the warn call (e.g. match /warn\([^)]*current shell may still resolve/) and change the second to require the printf statement (e.g. match /printf[^;]*exec \\\\"\\\\\\$SHELL\\\\" -l/), so the tests fail if the warn/printf calls are removed or altered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/install-preflight.test.ts`:
- Around line 1546-1548: The current regexes can match nearby comments; tighten
them to assert the actual executable lines by including the function/statement
tokens: change the first assertion to require the warn call (e.g. match
/warn\([^)]*current shell may still resolve/) and change the second to require
the printf statement (e.g. match /printf[^;]*exec \\\\"\\\\\\$SHELL\\\\" -l/),
so the tests fail if the warn/printf calls are removed or altered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2367bf3-853a-49d0-8ed9-dad7faeef4b0
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
Layered on top of the loud warning — when install_nodejs runs the nvm upgrade path AND the user is on a real TTY AND --non-interactive is not set, prompt [Y/n] to replace the installer process with `exec $SHELL -l` so Node v22 is active in the very next prompt without the user having to run anything manually. Gated to preserve scriptability: piped invocations (curl | bash) and explicit --non-interactive skip the prompt, so `curl | bash && nemoclaw onboard` chains keep working. Warning + manual command remain for those paths. Test: guards on NODE_UPGRADED_VERSION flag, NON_INTERACTIVE gate, TTY check, and the exec $SHELL -l accept path. Co-Authored-By: Claude Opus 4.7 (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 `@scripts/install.sh`:
- Around line 324-327: The prompt block that prints "Reload your shell now..."
is only gated by a stdin TTY check ([[ -t 0 ]]) so when stdout is redirected the
prompt text is written to the file and the script still waits for input; update
the gating condition to require both stdin and stdout to be TTY (so the prompt
and read -r answer are only executed when both are interactive) by changing the
check that guards the prompt/ read block (the lines around [[ -t 0 ]] || return
0, the printf using NODE_UPGRADED_VERSION, and the read -r answer) to test both
file descriptors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81421e66-2c8e-4d9c-a91f-bcfb241358c0
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/install-preflight.test.ts
… test regex - maybe_offer_shell_reload: require both stdin AND stdout to be TTY so `install.sh > log.txt` in an interactive terminal doesn't print the prompt to the redirected file while blocking on invisible input. - install-preflight: anchor the regex to the actual warn/printf call tokens instead of accepting a match in nearby comment text.
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
The core problem: exec "$SHELL" -l in a child process creates a nested shell, not a reload
When a user runs bash install.sh (or the installer runs as any subprocess), the process tree looks like:
Parent shell (PID 100, user's terminal)
└── bash install.sh (PID 200)
When the script does exec "$SHELL" -l, it replaces PID 200 — the script's process — with a new login shell:
Parent shell (PID 100, user's terminal, still has old PATH)
└── /bin/zsh -l (PID 200, new login shell, has correct PATH)
The user lands in what looks like a working shell with the new Node, but they're actually in a nested shell. When they type exit, they drop back to PID 100 — the original parent shell — which still resolves node to the old version. The "fix" is an illusion that makes the problem harder to diagnose later.
A child process fundamentally cannot mutate its parent's environment. exec replaces the current process, not the calling one.
Additional concerns
1. Breaks && chains when accepted. If a user runs bash install.sh && nemoclaw onboard on a TTY and accepts the prompt, exec replaces the script process with an interactive shell. The parent shell blocks waiting for that new shell to exit. nemoclaw onboard won't run until the user manually exits the nested shell — and by then the && chain context is stale. The -t 0 gate only protects piped stdin (curl | bash), not normal terminal invocations with chained commands.
2. The tests are source-code pattern matching, not behavioral tests. Both new tests read install.sh as a string and regex-match over the bash source text. They verify that certain keywords exist in certain functions — not that the feature actually works at runtime. They'll pass even if the logic is broken, and they'll break on any cosmetic refactor (renaming a variable, reordering lines).
Suggestion
Drop the exec auto-reload prompt entirely. The loud warning adjacent to the install line (layer 1 of this PR) is the right fix — it tells the user exactly what to do. Trying to be clever with exec creates worse UX than just being direct. If you want to go further, print the command they need to copy/paste in a visually distinct box they can't miss.
Agreed, Initially thought warning was enough, but tried to be clever automatic exec... |
…rning only Aaron caught the core mistake: exec \$SHELL -l inside install.sh creates a nested login shell, not a parent-shell reload. When the user exits that nested shell they're back in the original shell with the OLD Node still on PATH — an illusion that masks the real issue. It also breaks \`bash install.sh && nemoclaw onboard\` chains when accepted on a TTY. Revert layer 2 entirely. Keep layer 1 (loud warning + exact command), and switch the suggested command to \`source ~/.nvm/nvm.sh && nvm use 22\` — that actually activates Node in the user's current shell without the nested-shell trap. Also drops the source-pattern test for the deleted helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
The exec auto-reload is gone and replaced with an honest warning + copy-paste command. LGTM.
wscurran
added
CI/CD
github_actions
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 `@scripts/install.sh`:
- Around line 753-755: The printed message is too absolute and hardcodes the nvm
path; update the warn/printf block (the warn call and the two following printf
lines) to soften the claim about "previous version" (e.g., "may still resolve
`node` to an older version") and substitute the hardcoded ~/.nvm/nvm.sh with the
environment-aware $NVM_DIR (falling back to a sensible default if unset) in the
suggested command (e.g., use "$NVM_DIR/nvm.sh" or
"${NVM_DIR:-$HOME/.nvm}/nvm.sh") so users with custom NVM_DIR or fresh installs
see a correct, non-misleading instruction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 706a7d96-2f0e-4223-847e-7a76b8ff3fc3
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/install-preflight.test.ts (1)
1553-1553: Avoid hardcodingnvm use 22in the matcher.Line 1553 couples the test to a fixed major version. Prefer deriving expected major from installer constants so the test only fails on behavior changes, not routine version bumps.
♻️ Suggested diff
- expect(body).toMatch(/\n\s*printf\s+'[^']*NVM_DIR:-\$HOME\/\.nvm[^']*nvm use 22/); + const minNodeMajor = script.match(/MIN_NODE_VERSION="v?(\d+)\./)?.[1]; + expect(minNodeMajor).toBeDefined(); + expect(body).toMatch( + new RegExp( + String.raw`\n\s*printf\s+'[^']*NVM_DIR:-\$HOME\/\.nvm[^']*nvm use ${minNodeMajor}`, + ), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.ts` at line 1553, The test currently hardcodes "nvm use 22" in the regex against body; replace that hardcoded major with the installer constant's major version (e.g., derive nodeMajor from the exported installer constant like INSTALLER_NODE_VERSION or INSTALLER_NODE_MAJOR or parse the major from INSTALLER_NODE_VERSION) and interpolate that value into the regex used in the expect(body). Ensure you still escape regex-special characters as needed and keep the same surrounding pattern (printf ... NVM_DIR ... nvm use <major>) so the expectation checks the same text but uses the computed nodeMajor instead of 22.
🤖 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.ts`:
- Around line 1541-1553: The test currently only performs static source matching
on INSTALLER_PAYLOAD (reading script and regexing for install_nodejs() contents)
which won't catch regressions in runtime behavior; update test
install-preflight.test.ts to run the installer payload in a stubbed runtime (use
spawnSync like surrounding runtime tests), set a controlled environment/PATH and
capture stdout/stderr, and assert the actual installer output contains the
expected warn and printf hint strings (the same patterns currently checked
against body, e.g., the warn message and the single-quoted printf that mentions
NVM_DIR:-$HOME/.nvm and the nvm use 22 hint) so the test verifies real runtime
printed hints rather than only source text.
---
Nitpick comments:
In `@test/install-preflight.test.ts`:
- Line 1553: The test currently hardcodes "nvm use 22" in the regex against
body; replace that hardcoded major with the installer constant's major version
(e.g., derive nodeMajor from the exported installer constant like
INSTALLER_NODE_VERSION or INSTALLER_NODE_MAJOR or parse the major from
INSTALLER_NODE_VERSION) and interpolate that value into the regex used in the
expect(body). Ensure you still escape regex-special characters as needed and
keep the same surrounding pattern (printf ... NVM_DIR ... nvm use <major>) so
the expectation checks the same text but uses the computed nodeMajor instead of
22.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb4d47e3-e8a6-4221-98e2-880dccfcbe5d
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
| const script = fs.readFileSync(INSTALLER_PAYLOAD, "utf-8"); | ||
| const installNodejs = script.match(/install_nodejs\(\)\s*\{[\s\S]*?\n\}/); | ||
| expect(installNodejs).not.toBeNull(); | ||
| const body = installNodejs![0]; | ||
| // Anchor to the actual warn/printf calls (not the comment) so the test | ||
| // fails if the executable statements are removed. A child process can't | ||
| // mutate the parent's PATH, so the honest fix is printing the exact | ||
| // command the user can run in their existing shell (no exec tricks — | ||
| // those create a nested shell that masks the problem; see PR #2298). | ||
| expect(body).toMatch(/\n\s*warn\s+"Your current shell may still resolve/); | ||
| // Single-quoted printf avoids bash expansion of $NVM_DIR / $HOME in the | ||
| // printed text — the user gets a literal, env-aware command to paste. | ||
| expect(body).toMatch(/\n\s*printf\s+'[^']*NVM_DIR:-\$HOME\/\.nvm[^']*nvm use 22/); |
There was a problem hiding this comment.
This test is static-source matching, not runtime verification.
Lines 1541-1553 only regex-match script text, so this can pass even if runtime behavior regresses (e.g., gating/prompt flow changes). Please assert the hint via a real spawnSync installer run in a stubbed environment, like the surrounding runtime tests do.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/install-preflight.test.ts` around lines 1541 - 1553, The test currently
only performs static source matching on INSTALLER_PAYLOAD (reading script and
regexing for install_nodejs() contents) which won't catch regressions in runtime
behavior; update test install-preflight.test.ts to run the installer payload in
a stubbed runtime (use spawnSync like surrounding runtime tests), set a
controlled environment/PATH and capture stdout/stderr, and assert the actual
installer output contains the expected warn and printf hint strings (the same
patterns currently checked against body, e.g., the warn message and the
single-quoted printf that mentions NVM_DIR:-$HOME/.nvm and the nvm use 22 hint)
so the test verifies real runtime printed hints rather than only source text.
wscurran
added
area: ci
wscurran
added
NV QA
3 participants
Summary
Fixes #2178 (NV QA UAT: `node` still reports v20 after installer claims v22.22.2 was installed).
When `scripts/install.sh` upgrades Node via nvm, `nvm use 22` only takes effect inside the installer's subshell — the user's parent shell still resolves `node` to the pre-install version until they reload. The existing generic `source ` hint at the bottom of the installer is easy to miss.
Two layers:
1. Loud warning adjacent to install line
2. Opt-in auto-activation prompt (new)
At the end of `print_done`, when the upgrade path actually ran AND the user is on a real TTY AND `--non-interactive` is not set:
On accept → `exec "$SHELL" -l`, replacing the installer process so `node --version` prints v22 in the very next prompt with zero extra user action. On decline → noop (warning + manual command still on screen).
Gates preserving scriptability
In all skipped cases the loud warning + manual command remain visible.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests