fix(search): skip auto-install when embed targets npm's own global root#1722
fix(search): skip auto-install when embed targets npm's own global root#1722carlos-alm wants to merge 3 commits into
Conversation
resolveNpmInstallCwd() resolves to npm's own global modules root for a globally-installed codegraph (e.g. /opt/homebrew/lib on Homebrew Node). Running `npm install` there makes npm reify its own dependency tree as a side effect, which was observed to delete npm's own installation and co-located global packages before the lifecycle-script error surfaced. Add isNpmGlobalModulesRoot() to detect this case (presence of node_modules/npm) and skip the npm install entirely, pointing the user to `npm install -g <pkg>` instead of running an install that can nuke their global npm state. docs check acknowledged: this is an internal safety guard on an existing auto-install fallback path, not a documented feature or architectural change; no README/CLAUDE.md/ROADMAP.md content covers this behavior. Closes #1720 Impact: 3 functions changed, 6 callers affected Impact: 3 functions changed, 6 affected
Greptile SummaryThis PR prevents semantic-search auto-install from running in npm's own global install root. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(search): fold path case on Windows w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function isNpmGlobalModulesRoot(dir: string | undefined): boolean { | ||
| if (!dir) return false; | ||
| try { | ||
| return existsSync(path.join(dir, 'node_modules', 'npm', 'package.json')); |
There was a problem hiding this comment.
When @optave/codegraph is installed locally in a project that also has npm in node_modules, resolveNpmInstallCwd() resolves to that project root and this check classifies it as npm's global root. promptInstall() then skips the safe local npm install --no-save @huggingface/transformers path and tells the user to install globally, leaving semantic search unavailable for a normal project install.
There was a problem hiding this comment.
Fixed — isNpmGlobalModulesRoot() now asks npm directly via npm root -g and compares the real (symlink-resolved) path against the candidate directory, instead of just checking for the presence of node_modules/npm/package.json. A local project that happens to depend on the npm package itself no longer gets misclassified as npm's global root, since its node_modules/npm won't match npm's actual configured global root. The old file-existence check is kept only as a fallback for the rare case where the npm root -g call itself fails (e.g. npm binary unavailable in PATH). Added test coverage for: (1) authoritative match via npm root -g, (2) a project with npm as a dependency that is correctly NOT flagged, (3) fallback behavior when npm root -g fails. See commit ffa1adf.
| `${packageName} is missing, but codegraph is installed globally — auto-install is skipped to avoid modifying npm's own global installation.\nInstall it yourself with:\n npm install -g ${packageName}`, | ||
| ); | ||
| return Promise.resolve(false); |
There was a problem hiding this comment.
This recovery path tells users to run npm install -g @huggingface/transformers, but loadTransformers() loads the package with a plain dynamic import(pkg). A globally installed dependency is not resolved from the globally installed codegraph module unless it is installed under that module's own resolution path, so a user can follow this instruction and still hit the same ENGINE_UNAVAILABLE error.
There was a problem hiding this comment.
Investigated this — I don't believe it's actually broken, and here's why: Node's module resolution algorithm (both CJS require.resolve and ESM import()) walks up through every ancestor node_modules directory from the resolving file's real (symlink-resolved) path, not just the importing package's own node_modules. When codegraph is installed globally, it lives at <npmGlobalRoot>/node_modules/@optave/codegraph/.... Running npm install -g @huggingface/transformers places that package at <npmGlobalRoot>/node_modules/@huggingface/transformers — a sibling directory. I verified with a real filesystem test (mirroring codegraph's actual nested file structure, src/domain/search/models.ts, and using both require.resolve and dynamic import()) that this sibling resolves correctly in both CJS and ESM — the walk-up algorithm doesn't stop at the originating package's own directory. This matches how resolveNpmInstallCwd() already works for the auto-install case (which relies on the same walk-up behavior). I also confirmed npm's bin symlink (/opt/homebrew/bin/npm) resolves via realpath back into <npmGlobalRoot>/node_modules/npm/bin/..., so the same realpath-based resolution applies uniformly. Given this, the npm install -g <pkg> guidance should correctly make the package resolvable. If you have a concrete repro where this breaks (e.g. a package manager or Node version manager with a different global layout), please share it and I'll dig further — happy to be proven wrong with a reproduction.
There was a problem hiding this comment.
You're right — I was wrong. The walk-up algorithm does traverse to <npmGlobalRoot>/node_modules/ from within <npmGlobalRoot>/node_modules/@optave/codegraph/..., so a sibling global package is on the resolution path. The npm install -g <pkg> guidance is valid.
Thanks for the thorough investigation. Shall I remember this as a rule: "Do not flag global sibling node_modules resolution as broken — Node's walk-up algorithm resolves sibling packages under a shared global node_modules root correctly in both CJS and ESM"?
Codegraph Impact Analysis5 functions changed → 7 callers affected across 3 files
|
isNpmGlobalModulesRoot() previously flagged any directory containing node_modules/npm/package.json as npm's global install root, which misclassifies a normal local project that merely depends on the npm package itself (e.g. a tool that shells out to npm). Ask npm directly via `npm root -g` and compare its real path against the candidate directory; fall back to the old file-existence heuristic only if that call fails. Impact: 2 functions changed, 5 affected
| } | ||
|
|
||
| try { | ||
| return existsSync(path.join(dir, 'node_modules', 'npm', 'package.json')); |
There was a problem hiding this comment.
When npm root -g fails, this fallback goes back to treating any node_modules/npm/package.json as npm's global install root. A local project that depends on npm can still hit this path if the probe times out or the PATH/config used for the probe is broken, while the normal local npm install --no-save @huggingface/transformers path would have worked. In that case promptInstall() skips the local install and tells the user to install globally, leaving semantic search unavailable for the local project.
There was a problem hiding this comment.
Leaving this as-is — it's an intentional safety trade-off, not an oversight.
The fallback only triggers when npm root -g itself fails (ENOENT, timeout, broken PATH/config). In that scenario there are two ways to get it wrong:
- False positive (what you're flagging): a local project that happens to depend on the
npmpackage gets misclassified as npm's global root, so auto-install is skipped and the user has to runnpm install @huggingface/transformersmanually once. Annoying, but harmless and fully recoverable. - False negative (removing the fallback entirely, or defaulting to "not global" on probe failure): if npm can't be queried, we'd fall through to running
npm installunconditionally — which is exactly the destructive scenario bug: embed auto-install cwd resolves into global npm modules root for global installs, breaking install #1720 exists to prevent, and we'd have zero signal to catch it.
Given the original bug deleted npm's own global installation and co-located packages, I'd rather over-skip (extra manual step, easily recoverable) than under-skip (risk reification of npm's global tree again). It's also a narrow double-failure: npm root -g has to fail and the local project has to independently ship node_modules/npm/package.json. If npm root -g fails because npm/PATH is broken, the subsequent npm install call would very likely fail too anyway, so skipping straight to manual guidance is arguably the better UX even in the true-local case.
Happy to revisit if you see a way to disambiguate the two cases without another npm round-trip, but I don't think there's a safe way to remove the fallback outright.
…#1722) isNpmGlobalModulesRoot() compares a realpath'd candidate directory against npm's reported global root. On Windows, paths from execFileSync output and require.resolve can differ in drive-letter or segment casing despite referring to the same directory (NTFS is case-insensitive but case-preserving), which would cause a false-negative comparison. Fold case on win32 before comparing. Impact: 2 functions changed, 5 affected
1 participant
Summary
resolveNpmInstallCwd()resolves to npm's own global modules root for a globally-installed codegraph (e.g./opt/homebrew/libon Homebrew Node). Runningnpm installthere makes npm reify its own dependency tree, which was observed to delete npm's own installation and co-located global packages before the lifecycle-script error even surfaced.isNpmGlobalModulesRoot()to detect this case (presence ofnode_modules/npmin the resolved install cwd) and skip the auto-install entirely, pointing the user tonpm install -g <pkg>instead.promptInstall, and the final error message inloadTransformers.Closes #1720
Test plan
npx vitest run tests/unit/prompt-install.test.ts— 11/11 passing, including new tests forisNpmGlobalModulesRootand the end-to-end skip-install pathnpx tsc --noEmit— cleannpm run lint— cleancodegraph diff-impact --staged -T— 3 functions changed, 6 callers affected, no cycles/dead exports