bootstrap: harden fnm/Node activation across macOS + Windows (closes #3, #5)#6
Merged
Merged
Conversation
added 2 commits
May 13, 2026 14:55
When bootstrap.ps1 is executed via `irm <url> | iex`, the fnm activation done in the Node section does not persist into the child PowerShell scopes where later sections invoke npm. The first downstream npm call fails with `The term 'npm' is not recognized…`. Adds an EnsureFnmActivated helper and calls it immediately before each post-Node npm site: - corepack pnpm fallback - Claude Code migration cleanup (npm ls -g / npm uninstall -g) - Codex CLI install (npm install -g @openai/codex@latest) The guard is idempotent and a no-op when fnm is not installed. Closes #3
After two real-Mac incidents on 2026-05-13 where ad-hoc `brew install
node` (v25/v26, Current-line) shadowed fnm-managed LTS and broke `claude`
and `qmd` via better-sqlite3 ABI mismatch, harden macos/bootstrap.sh and
zshrc.snippet.sh against that whole class of drift.
New behavior:
- Pre-existing brew Node detection. If `brew list` reports node not on
the pinned major, prompt to `brew unlink node` (or auto-unlink with
--force-relink for CI / non-interactive reruns). 60s prompt timeout.
- Plan B `--brew-only` flag. Installs node@22 via brew and force-links
it, skipping fnm entirely. Mutually exclusive with explicit --fnm.
- Homebrew path ownership pre-flight. Probes /usr/local/share/zsh*,
/usr/local/var/homebrew, /usr/local/Cellar, and the /opt/homebrew/*
equivalents for writability. On the first broken path, requests sudo
once up front and chowns only the broken paths to $USER:staff.
- `--restore-globals FILE` flag. After Node is installed, reinstalls
each npm package named in FILE (one per line; blank lines and `#`
comments ignored). Failures are reported with captured stderr but
do not abort the bootstrap.
- Non-LTS drift guard in zshrc snippet. Warns on every interactive
shell startup when `node -v` is not the pinned LTS major (default
22), with the recovery command inline. Catches both odd-major
Current releases and even-but-Current drift (v26) — the broader
rule that #5's AC#4 narrowed to "odd-line only" missed in the
original incident.
Code is split into pure helpers + side-effecting glue:
- macos/lib/node-utils.sh — is_non_lts_node_version, parse_globals_file
- macos/lib/cli.sh — parse_flags
- macos/bootstrap.sh — preflight_brew_paths,
remediate_existing_brew_node,
restore_npm_globals
Tests (25 passing): plain-bash tests/run.sh + per-helper *.test.sh.
Side-effecting blocks are not unit-tested; gated by real-Mac runs on
mbp13 and the primary dev Mac per #5 AC#8.
README adds: macOS flags reference and a "Migrating a Mac that was
already set up ad-hoc" recovery section with copy-pasteable commands.
Closes #5
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR hardens macOS and Windows bootstrap scripts against fnm activation loss and pre-existing Node installations. It adds CLI flag parsing, Node utilities, preflight brew ownership fixes, existing-Node detection/remediation, optional npm globals restoration, a zsh Node-drift warning, tests, and README docs for the new macOS flags and migration workflow. ChangesBootstrap Hardening Across Platforms
Sequence Diagram(s)sequenceDiagram
participant User
participant Shell as macos/bootstrap.sh
participant CLI as parse_flags
participant Preflight as preflight_brew_paths
participant Remediate as remediate_existing_brew_node
participant Brew
participant Fnm
participant Npm as npm
User->>Shell: run macos/bootstrap.sh [--brew-only|--fnm] [--force-relink] [--restore-globals FILE]
Shell->>CLI: parse_flags
Shell->>Preflight: preflight_brew_paths
Preflight->>Brew: sudo chown broken brew paths
Shell->>Remediate: remediate_existing_brew_node
Remediate->>Brew: check/unlink node keg
Shell->>Fnm: install/activate fnm OR install node@PINNED if --brew-only
Fnm->>Shell: node/npm available
Shell->>Npm: restore_npm_globals (if provided)
Npm-->>Shell: globals restored
Shell-->>User: complete
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8 tasks
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/macos/globals-parser.test.sh`:
- Line 21: The expected multiline literals passed to assert_equals are unquoted
and get word-split; wrap each $'...'\ string in quotes so the entire multiline
string is a single argument. Update the assert_equals calls (e.g. the one using
assert_equals $'@anthropic-ai/claude-code\n@openai/codex\npnpm' "$got") to quote
the expected value (e.g. assert_equals
"$'@anthropic-ai/claude-code\n@openai/codex\npnpm'" "$got") for all occurrences
referenced (assert_equals calls at lines with the multiline $'...' strings).
In `@tests/run.sh`:
- Line 13: The script currently runs cd "$REPO_ROOT" without handling failure;
update the tests/run.sh around the cd "$REPO_ROOT" call to halt and report a
clear error if changing directories fails (for example by checking the exit
status of cd and printing an explanatory message then exiting non-zero, or
enabling "set -e" at the top of the script) so the script does not continue in
the wrong working directory.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd0b4836-d238-4f26-8aef-9c64a245dcd2
📒 Files selected for processing (10)
README.mdbootstrap.ps1macos/bootstrap.shmacos/lib/cli.shmacos/lib/node-utils.shmacos/zshrc.snippet.shtests/macos/cli.test.shtests/macos/globals-parser.test.shtests/macos/node-utils.test.shtests/run.sh
Without `set -e`, a failed `cd "$REPO_ROOT"` would silently leave the runner in the caller's working directory and produce confusing "tests/ not found" output. Surface the failure explicitly instead. Per CodeRabbit review on PR #6.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/run.sh`:
- Around line 63-72: The test loop runs each test in a subshell but doesn't
enable errexit, so a failed assertion can be masked by later commands; update
the invocation inside the loop that currently calls ( "$t" ) to run each test
with errexit enabled (e.g. use a subshell that runs set -e before executing
"$t") so that any failing assert_* inside the test function causes the subshell
to exit non‑zero and the test is recorded as FAIL; refer to the TESTS array and
the for loop that iterates over TESTS to locate the call to "$t".
- Around line 50-53: The script currently treats "no tests found" as a success
(exit 0) which can mask broken discovery in CI; in the block that checks
${`#TEST_FILES`[@]} (the if [[ ${`#TEST_FILES`[@]} -eq 0 ]] check), change the exit
code from 0 to a non-zero value (e.g., exit 1) so the process fails fast when no
tests are discovered, keeping the error message sent to stderr.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92739335-decd-4d96-9e1c-0714b815bc40
📒 Files selected for processing (2)
tests/macos/globals-parser.test.shtests/run.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/macos/globals-parser.test.sh
added 2 commits
May 14, 2026 09:57
This reverts commit f7db08c.
Two CodeRabbit findings: 1. `exit 0` on zero discovered tests masked broken globbing in CI as a green run. Switch to `exit 1` so a misconfigured discovery path is loud, not silent. 2. The per-test subshell did not set `errexit`, so a failing `assert_*` followed by any zero-exit command (e.g., trailing `echo`, `rm -f`) was masked and the test was recorded PASS. Run each test under `set -e` so the first failing assertion exits the subshell. All 25 existing tests still pass under the stricter loop; the change narrows what counts as PASS.
Owner
Author
I reverted the autofix in commit b529483 because it broke 4 tests ( The original review comment was a false positive: Repro: $ foo() { echo "args: $#"; for a in "$@"; do printf " [%s]\n" "$a"; done; }
$ foo $'a\nb\nc'
args: 1
[a
b
c]
Tests are back to 25 passed, 0 failed on this branch. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/run.sh`:
- Around line 30-36: The assert_success helper captures the failed command's
exit code incorrectly because $? is read inside the printf after the if test;
modify assert_success so it runs the command, immediately saves its exit status
into a variable (e.g., status=$?), then use that variable both for the
conditional test and in the error message; reference the assert_success function
and ensure the saved status is returned or used for the return value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5984b1ca-c42c-4a57-98ae-151ce15d1952
📒 Files selected for processing (2)
tests/macos/globals-parser.test.shtests/run.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/macos/globals-parser.test.sh
added 2 commits
May 14, 2026 10:06
Reuse:
- bootstrap.ps1 Node section now calls `EnsureFnmActivated` instead of
inlining the same `fnm env | iex` line (was duplicated across the Node
install, Claude Code migration, Codex install, and pnpm fallback).
- macos/bootstrap.sh::remediate_existing_brew_node now calls
`is_non_lts_node_version` instead of re-implementing the major-extract
logic that already lives in macos/lib/node-utils.sh.
- macos/lib/node-utils.sh now owns the canonical `PINNED_NODE_LTS_MAJOR`
with an inline "if you bump this, also update X/Y/Z" pointer.
- Promoted `make_globals_fixture` into tests/run.sh; cli.test.sh and
globals-parser.test.sh both use it instead of near-duplicate locals.
Quality:
- Dropped redundant `USE_FNM` global — `!BREW_ONLY` is the same signal,
mutex check uses local `*_explicit` flags. cli.sh and cli.test.sh
updated; one fewer piece of duplicated state.
- Inlined the nested `_unlink_brew_node` function (bash has no function
scoping; it was leaking to global anyway).
- Switched flag comparisons from `[[ "${X:-0}" == "1" ]]` to `(( X ))`
arithmetic — the defensive `:-0` is no longer needed now that
bootstrap.sh pre-initializes the globals.
- macos/zshrc.snippet.sh keeps the major-extract logic inlined (avoids
sourcing a lib in every interactive shell startup) with a comment
cross-referencing node-utils.sh so the two stay in sync.
Efficiency:
- restore_npm_globals now snapshots `npm ls -g --depth=0 --parseable`
once and matches per-package against an awk-extracted name list with
`grep -Fx` (exact-line literal). Removes N+1 `npm list -g` cold-start
cost on a long globals file and avoids the `/$pkg` substring false
positive (e.g. `foo` vs `foo-utilities`).
- Added a RETURN trap to clean up the failure-log tempfile even when
the function exits via an interrupt or the bootstrap aborts later.
- Dropped a redundant `brew list --formula | grep -qx node` probe —
`brew list --versions node` alone signals not-installed via empty
output.
- preflight_brew_paths now short-circuits when brew is absent (mirrors
remediate_existing_brew_node's early return).
Tests: 25 passed, 0 failed.
…cess `$?` inside the `if !` body reflects the `if` test's exit (always 0), not the underlying command's. The error message printed "exit 0" for every failure. Save `$?` immediately after the call instead. Cosmetic only — no test passes or fails differently.
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
One PR closes two bundled issues that share a theme — fnm / Node drift on bootstrapped dev machines.
npmnot found during Claude Code / Codex install — fnm not activated in piped execution scope #3 (bug, Windows):npm not foundduringirm | iexinstall because fnm activation does not persist across piped child PowerShell scopes.brew install node(Current-line v25 / v26) shadowed fnm-managed LTS and brokeclaude/qmdviabetter-sqlite3ABI mismatch on two real Macs on 2026-05-13 (~45 min recovery each).Changes by file
bootstrap.ps1EnsureFnmActivatedhelper called before each of the 3 post-Node npm sites (corepack pnpm fallback, Claude Code migration cleanup, Codex install).macos/bootstrap.sh--brew-only,--fnm,--force-relink,--restore-globals FILE. New blocks:preflight_brew_paths,remediate_existing_brew_node,restore_npm_globals. Branches on$BREW_ONLYbetween fnm path andnode@22install.macos/lib/node-utils.shis_non_lts_node_version,parse_globals_file. Pure helpers, sourced bybootstrap.sh.macos/lib/cli.shparse_flags. Handles flag parsing, mutex check, file existence validation for--restore-globals.macos/zshrc.snippet.shnode -vis not the pinned LTS major, with recovery command inline.README.mdtests/tests/run.sh) + 25 tests across 3 files covering all pure helpers. Zero external test deps.AC #4 of issue #5 — broadened from "odd-line" to "non-LTS"
The original AC named "odd-line release (v23 / v25 / v27 / v29)" but the 2026-05-13 incident on the primary dev Mac hit v26.0.0, which is even-major-but-Current and would slip an odd-only guard. The implementation broadens the check to "major ≠ pinned LTS major" (default 22). Override at shell-snippet level by exporting
DEV_BOOTSTRAP_PINNED_NODE_MAJORbefore the snippet runs.Merge gate (AC #8 of #5)
The side-effecting blocks (brew probe, sudo chown, brew unlink, brew link --force, --restore-globals execution) are not unit-tested. They will be gated by an end-to-end run on:
mbp13Once both reach green state on this branch, the PR can merge.
Test coverage
Pure helpers covered:
is_non_lts_node_version(7),parse_globals_file(7),parse_flags(11). Side-effecting blocks gated by the real-Mac runs above; bash syntax checked clean (bash -n).Out of scope
linux/bootstrap.sh— separate hardening pass.Review notes addressed
Pre-PR review surfaced 7 actionable findings, all fixed in this branch:
restore_npm_globalsnow capturesnpm install -gstderr to a tempfile and dumps failure details in the summary.BREW_ONLY/USE_FNM/FORCE_RELINK/RESTORE_GLOBALS_FILEat the top ofbootstrap.shbeforeparse_flags, soset -uis safe across future refactors.brew link --force --overwrite "node@${PIN}"is now gated by a check on the current node-binary symlink target — true zero-mutation rerun.brew unlink nodeandsudo chown -Rwrapped with explicit failure handling and per-path reporting.PINNED_NODE_LTS_MAJOR=re-declaration inbootstrap.sh(already set by sourcingnode-utils.sh).@anthropic-ai/claude-code) with whole-line and trailing#comments toglobals-parser.test.sh.cli.test.sh.The
read -r ansprompt inremediate_existing_brew_nodenow has a 60-second timeout (the low-priority CI-hang fix from the review).Summary by CodeRabbit
Documentation
New Features
Tests