[luv-286] fix: detect PATH-shadowed failproofai installs at install + run time#286
Merged
Conversation
A stale `bun link` from a contributor's prior `bun run dev` (or a `bun install -g failproofai` whose ~/.bun/bin sorts ahead of npm's prefix) shadows later `npm install -g failproofai@<new>` updates. Customers then see a confusing "Cannot find server.js at: <old-dev-tree>/.next/standalone/server.js" runtime error pointing at the shadowed install — and the recommended fix (`npm install -g failproofai@latest`) doesn't help when the new install is itself being shadowed. New scripts/install-diagnosis.mjs resolves the PATH-first failproofai via `command -v` (POSIX) / `where` (Win32), compares its package root + version against the running install, and emits a copy-pasteable cleanup command. Surfaced in two places: scripts/postinstall.mjs warns at install time before any broken run, and scripts/launch.ts rewrites the existing missing-server.js error to name the actual stale install when the cause is a shadow rather than a broken build. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces automatic detection of PATH-shadowed ChangesPath Shadow Detection and Recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/launch.ts (1)
53-78: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a launch-level unit test for the new stale-shadow error path.
This TypeScript file introduces new runtime behavior but there’s no direct test shown for message selection when
server.jsis missing and diagnostics indicate an alternate install.As per coding guidelines,
**/*.ts: Always add unit tests in__tests__/for new behavior; never modify existing tests just to make them pass.🤖 Prompt for 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. In `@scripts/launch.ts` around lines 53 - 78, Add a unit test in __tests__ that exercises the new stale-shadow path by stubbing diagnoseShadow to return an object with shadowed: true and populated fields (pathFirstPath, pathFirstVersion, npmGlobalPath/bunGlobalPath, npmGlobalVersion/bunGlobalVersion, recommendation), then invoke the launch behavior that computes shadowMessage (the code path using diagnoseShadow, packageRoot, version, serverJsPath) and assert that console.error is called with the shadowMessage (not the fallback serverJsPath message) and that process.exit(1) is invoked; use spies/mocks for console.error and process.exit and import or require the module entrypoint so the test runs the same logic in scripts/launch.ts (reference symbols: diagnoseShadow, shadowMessage, serverJsPath, packageRoot, version).
🧹 Nitpick comments (1)
__tests__/scripts/install-diagnosis.test.ts (1)
88-129: ⚡ Quick winAdd an explicit assertion for bun-link remediation output in this scenario.
This case is the highest-risk path, but it currently doesn’t assert
diag.recommendation. Adding that assertion would lock in the expected cleanup command and prevent regressions.🤖 Prompt for 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. In `@__tests__/scripts/install-diagnosis.test.ts` around lines 88 - 129, Add an assertion that diag.recommendation explicitly suggests remediating the bun-linked dev tree in this shadowing scenario: after calling diagnoseShadow (and existing expects), assert that diag.recommendation contains the BUN_LINKED_PKG identifier (and optionally NPM_GLOBAL_PKG) so the message recommends removing or unlinking the bun-linked dev tree (i.e., the expected cleanup command/path is locked in); reference diagnoseShadow and the diag.recommendation field when adding this expectation.
🤖 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 `@scripts/install-diagnosis.mjs`:
- Around line 108-116: The cleanup command selection in buildRecommendation
currently only checks pathFirstPackageRoot and misses bun-link cases; update
buildRecommendation (function name) to detect bun installations by also checking
for the existence of bun shadow files/dirs (e.g.,
homedir()+"/.bun/bin/"+PKG_NAME and
homedir()+"/.bun/install/global/node_modules/"+PKG_NAME) or whether PATH
contains homedir()+"/.bun/bin" and treat those as isBun true; implement this by
importing fs and using fs.existsSync on those bun paths (and optionally inspect
process.env.PATH), then return the same bun cleanup string when found so
bun-link shadows get the correct rm -f and rm -rf commands.
In `@scripts/launch.ts`:
- Around line 59-68: The current guard uses only diag.shadowed so it misses the
case where the PATH-first binary lives in the same package root but is a stale
runtime; update the condition around the diagnoseShadow result to also detect a
mismatched PATH-first version or missing build when diag.pathFirstPath ===
diag.selfPackageRoot: e.g. change the if to `if (diag.shadowed ||
(diag.pathFirstPath === diag.selfPackageRoot && diag.pathFirstVersion &&
diag.pathFirstVersion !== version))` (or the equivalent using
packageRoot/version variables) so the stale-install message is shown when the
PATH-first copy is the same root but has a different/older version; keep using
diagnoseShadow, diag.pathFirstPath, diag.selfPackageRoot, diag.pathFirstVersion,
packageRoot and version to locate the logic and produce shadowMessage.
---
Outside diff comments:
In `@scripts/launch.ts`:
- Around line 53-78: Add a unit test in __tests__ that exercises the new
stale-shadow path by stubbing diagnoseShadow to return an object with shadowed:
true and populated fields (pathFirstPath, pathFirstVersion,
npmGlobalPath/bunGlobalPath, npmGlobalVersion/bunGlobalVersion, recommendation),
then invoke the launch behavior that computes shadowMessage (the code path using
diagnoseShadow, packageRoot, version, serverJsPath) and assert that
console.error is called with the shadowMessage (not the fallback serverJsPath
message) and that process.exit(1) is invoked; use spies/mocks for console.error
and process.exit and import or require the module entrypoint so the test runs
the same logic in scripts/launch.ts (reference symbols: diagnoseShadow,
shadowMessage, serverJsPath, packageRoot, version).
---
Nitpick comments:
In `@__tests__/scripts/install-diagnosis.test.ts`:
- Around line 88-129: Add an assertion that diag.recommendation explicitly
suggests remediating the bun-linked dev tree in this shadowing scenario: after
calling diagnoseShadow (and existing expects), assert that diag.recommendation
contains the BUN_LINKED_PKG identifier (and optionally NPM_GLOBAL_PKG) so the
message recommends removing or unlinking the bun-linked dev tree (i.e., the
expected cleanup command/path is locked in); reference diagnoseShadow and the
diag.recommendation field when adding this expectation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5de8bc8f-4401-4f5a-8b7f-4f64ce76ed78
📒 Files selected for processing (5)
CHANGELOG.md__tests__/scripts/install-diagnosis.test.tsscripts/install-diagnosis.mjsscripts/launch.tsscripts/postinstall.mjs
(1) buildRecommendation now keys on `pathFirstBin` rather than the realpath'd package root. In bun-link cases, realpath unwraps the link into the dev-tree path (not under ~/.bun/), so the previous root-based isBun check mis-classified bun-link shadows as npm and produced a useless `npm rm -g` recommendation. Trusting the un-resolved binary path catches both bun-link and bun-install shadows correctly. (2) Shadow detection now also fires when `selfPackageRoot` matches `pathFirstPath` but a different failproofai exists at the npm or bun global. This is the runtime stale-binary case the diagnosis was designed for: the user is running the shadow, so the two roots agree, and the previous "self != path-first" check returned false there. The launch.ts caller now picks the alternate install (npm or bun global, whichever differs from path-first) for the "Newer copy:" line so we never point the user back at the same binary they're already running. Adds an explicit recommendation assertion to the existing bun-link test plus a new test covering the runtime stale-binary scenario. Co-Authored-By: Claude Opus 4.7 <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
scripts/install-diagnosis.mjs(Node built-ins only, never throws) that resolves the PATH-firstfailproofaiviacommand -v/where, compares it against the running install, and emits a copy-pasteable cleanup command for the offending install (bun-side cleanup targets~/.bun/bin/failproofaiand the bun globals dir; npm-side usesnpm rm -g failproofai).scripts/postinstall.mjs(warns at install time when the just-installed copy is shadowed) andscripts/launch.ts(rewrites the existingCannot find server.js at:runtime error to name the actual stale install when the cause is a PATH shadow rather than a broken build).__tests__/scripts/install-diagnosis.test.tscovering same-install / bun-link shadow / bun-global shadow / secondary-npm shadow / nothing on PATH / corrupt package.json /npm root -gfailure.Why
Customers who previously ran
bun install -g failproofai(or any contributor whose priorbun run dev/starttriggeredbun linkviapredev/prestart) end up with~/.bun/bin/failproofaishadowing laternpm install -g failproofai@<new>updates because~/.bun/binsorts ahead of npm's prefix on PATH. The current runtime error blames "missing build output" and recommendsnpm install -g failproofai@latest— which doesn't help when the new install is itself being shadowed. Concrete repro this fix targets:After this change: postinstall warns immediately at install time naming both paths + cleanup command; if the customer misses that warning, the runtime error explains the real cause + exact fix instead of recommending a no-op upgrade.
Test plan
bun run test:run(1451 unit tests pass, incl. 7 new)bun run lint(clean)bunx tsc --noEmit(clean)bun run build:cli(bundles cleanly)bun run build:cli && bun link, thennpm install -g failproofai@<latest beta>from elsewhere; verify postinstall prints shadow warning andfailproofaithen prints the shadow-shaped errorbun run test:e2eGenerated with Claude Code.
Summary by CodeRabbit
Bug Fixes
failproofaiinstallation on the system's PATH shadows the newly installed version.npm rm -gor bun unlink commands) when shadowing is detected.Tests