-
Notifications
You must be signed in to change notification settings - Fork 16
fix(search): skip auto-install when embed targets npm's own global root #1722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3947c23
ffa1adf
9d93383
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,33 @@ | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { existsSync, realpathSync } from 'node:fs'; | ||
| import { createRequire } from 'node:module'; | ||
| import path from 'node:path'; | ||
| import { createInterface } from 'node:readline'; | ||
| import { info } from '../../infrastructure/logger.js'; | ||
| import { ConfigError, EngineError } from '../../shared/errors.js'; | ||
|
|
||
| const _require = createRequire(import.meta.url); | ||
| const NPM_BIN = process.platform === 'win32' ? 'npm.cmd' : 'npm'; | ||
|
|
||
| /** Resolve a path to its real (symlink-free) form, or return it unchanged if that fails. */ | ||
| function tryRealpath(p: string): string { | ||
| try { | ||
| return realpathSync(p); | ||
| } catch { | ||
| return p; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Normalize a path for equality comparison. Windows filesystems are | ||
| * case-insensitive (but case-preserving), and paths sourced from | ||
| * `execFileSync` output vs. `require.resolve` can differ in drive-letter or | ||
| * segment casing (e.g. `C:\Users\...` vs `c:\users\...`) despite pointing at | ||
| * the same directory — so comparisons must fold case on win32. | ||
| */ | ||
| function normalizeForComparison(p: string): string { | ||
| return process.platform === 'win32' ? p.toLowerCase() : p; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the directory where `npm install` should run so the installed | ||
|
|
@@ -35,6 +57,51 @@ export function resolveNpmInstallCwd(): string | undefined { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * True when `dir` is npm's own global modules root — the directory whose | ||
| * `node_modules` is npm's global install target (and therefore already | ||
| * contains npm's own installation, `node_modules/npm`). | ||
| * | ||
| * Running `npm install` with this as `cwd` makes npm reify its *own* | ||
| * dependency tree as a side effect of installing the requested package. | ||
| * On at least one observed setup (Homebrew-managed Node/npm on macOS) this | ||
| * deleted npm's own installation and other co-located global packages | ||
| * before the install's lifecycle-script error even surfaced (#1720). | ||
| * Global installs of codegraph must never run `npm install` here. | ||
| * | ||
| * Authoritative check: ask npm itself for its global modules root | ||
| * (`npm root -g`) and compare against `dir`. This avoids misclassifying a | ||
| * normal project that merely happens to depend on the `npm` package itself | ||
| * (e.g. a tool that shells out to npm) — such a project's `node_modules/npm` | ||
| * would satisfy the old file-existence heuristic without actually being | ||
| * npm's global root. Falls back to the file-existence heuristic only if the | ||
| * `npm root -g` call itself fails (e.g. npm binary unavailable in PATH). | ||
| * | ||
| * @internal Exported for unit tests; not part of the public barrel. | ||
| */ | ||
| export function isNpmGlobalModulesRoot(dir: string | undefined): boolean { | ||
| if (!dir) return false; | ||
|
|
||
| const candidate = normalizeForComparison(tryRealpath(path.join(dir, 'node_modules'))); | ||
| try { | ||
| const globalRoot = execFileSync(NPM_BIN, ['root', '-g'], { | ||
| encoding: 'utf8', | ||
| timeout: 10_000, | ||
| }).trim(); | ||
| if (globalRoot) { | ||
| return normalizeForComparison(tryRealpath(globalRoot)) === candidate; | ||
| } | ||
| } catch { | ||
| // npm unavailable/unresolvable — fall back to the heuristic below. | ||
| } | ||
|
|
||
| try { | ||
| return existsSync(path.join(dir, 'node_modules', 'npm', 'package.json')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this as-is — it's an intentional safety trade-off, not an oversight. The fallback only triggers when
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: 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. |
||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| export interface ModelConfig { | ||
| name: string; | ||
| dim: number; | ||
|
|
@@ -138,7 +205,6 @@ export const MODELS: Record<string, ModelConfig> = { | |
| export const EMBEDDING_STRATEGIES: readonly string[] = ['structured', 'source']; | ||
|
|
||
| export const DEFAULT_MODEL: string = 'nomic'; | ||
| const NPM_BIN = process.platform === 'win32' ? 'npm.cmd' : 'npm'; | ||
| const BATCH_SIZE_MAP: Record<string, number> = { | ||
| minilm: 32, | ||
| 'jina-small': 16, | ||
|
|
@@ -173,6 +239,14 @@ export function getModelConfig(modelKey?: string): ModelConfig { | |
| */ | ||
| export function promptInstall(packageName: string): Promise<boolean> { | ||
| const installCwd = resolveNpmInstallCwd(); | ||
|
|
||
| if (isNpmGlobalModulesRoot(installCwd)) { | ||
| info( | ||
| `${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); | ||
|
Comment on lines
+245
to
+247
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This recovery path tells users to run
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Investigated this — I don't believe it's actually broken, and here's why: Node's module resolution algorithm (both CJS
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right — I was wrong. The walk-up algorithm does traverse to Thanks for the thorough investigation. Shall I remember this as a rule: "Do not flag global sibling |
||
| } | ||
|
|
||
| if (!process.stdin.isTTY) { | ||
| info(`Installing ${packageName} (optional dependency for semantic search)…`); | ||
| try { | ||
|
|
@@ -237,7 +311,10 @@ export async function loadTransformers(): Promise<unknown> { | |
| ); | ||
| } | ||
| } | ||
| throw new EngineError(`Semantic search requires ${pkg}.\nInstall it with: npm install ${pkg}`); | ||
| const manualInstall = isNpmGlobalModulesRoot(resolveNpmInstallCwd()) | ||
| ? `npm install -g ${pkg}` | ||
| : `npm install ${pkg}`; | ||
| throw new EngineError(`Semantic search requires ${pkg}.\nInstall it with: ${manualInstall}`); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
@optave/codegraphis installed locally in a project that also hasnpminnode_modules,resolveNpmInstallCwd()resolves to that project root and this check classifies it as npm's global root.promptInstall()then skips the safe localnpm install --no-save @huggingface/transformerspath and tells the user to install globally, leaving semantic search unavailable for a normal project install.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed —
isNpmGlobalModulesRoot()now asks npm directly vianpm root -gand compares the real (symlink-resolved) path against the candidate directory, instead of just checking for the presence ofnode_modules/npm/package.json. A local project that happens to depend on thenpmpackage itself no longer gets misclassified as npm's global root, since itsnode_modules/npmwon't match npm's actual configured global root. The old file-existence check is kept only as a fallback for the rare case where thenpm root -gcall itself fails (e.g. npm binary unavailable in PATH). Added test coverage for: (1) authoritative match vianpm root -g, (2) a project withnpmas a dependency that is correctly NOT flagged, (3) fallback behavior whennpm root -gfails. See commit ffa1adf.