fix(embed): install @huggingface/transformers into codegraph's host node_modules#1178
Conversation
…ode_modules
`embed`'s lazy install calls `execFileSync('npm', ['install', ...])` with
no `cwd`. Default cwd = `process.cwd()`, which is whatever directory the
user invoked codegraph from — typically NOT the directory where codegraph
itself is installed.
The resulting install lands in the user's project node_modules, but the
subsequent `await import('@huggingface/transformers')` resolves from
codegraph's own location, where the dep is missing. The user sees:
[ENGINE_UNAVAILABLE]: @huggingface/transformers was installed
but failed to load. Please check your environment.
…while their project's node_modules silently gains an unwanted dep.
Pin `cwd` to the host directory that contains @optave/codegraph (resolved
via `_require.resolve('@optave/codegraph/package.json')` then four
`path.dirname` hops). The install now lands in the same node_modules that
contains codegraph, so the dynamic import resolves correctly without
polluting the user's working directory.
Falls back to `cwd: undefined` (= process.cwd()) when @optave/codegraph
can't be resolved — typically a source-of-truth dev checkout — so the
existing test suite (which mocks the import to fail) continues to pass.
Closes #1175
|
Claude finished @carlos-alm's task —— View job Code Review CompleteReviewing PR #1178: fix(embed): install @huggingface/transformers into codegraph's host node_modules Review Checklist:
Root Cause AnalysisProblem Being Solved: The embed feature's lazy install mechanism was installing Why This Approach: The fix correctly identifies that Risk Assessment: Low risk. This is a focused bug fix that doesn't change API contracts or core functionality. The fallback mechanism preserves existing behavior for development environments. Backlog Compliance
Code Quality AssessmentExcellent implementation quality:
Security considerations: ✅ Safe
Edge cases handled:
Test IntegrityThe existing test suite remains valid because:
Note: Cannot verify test execution due to tool restrictions, but the logic is sound. Critical ConcernsNone identified. This is a clean, well-implemented bug fix. Minor observation: The function name Final Recommendation
Recommendation: Merge this PR. It fixes a real user-facing issue where the embedding feature would fail after appearing to install successfully, which is confusing UX. The implementation is robust and safe. |
Greptile SummaryThis PR fixes a bug where
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to pinning the npm install working directory and does not touch any embedding, indexing, or query logic. The root-cause analysis is correct: without a pinned cwd, npm installs the package in the wrong location and the dynamic import consistently fails. The four-hop dirname computation is verified by a dedicated unit test, the undefined fallback keeps the dev/source-checkout path working, and both install branches (TTY and non-TTY) now pass --no-save and cwd symmetrically. No regressions are introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[embed / loadTransformers called] --> B{import transformers}
B -- success --> C[Return module]
B -- fails --> D[promptInstall called]
D --> E[resolveNpmInstallCwd]
E --> F{resolve codegraph package.json}
F -- resolves --> G["4x path.dirname → host dir"]
F -- throws --> H["installCwd = undefined (process.cwd fallback)"]
G --> I[installCwd = host dir]
I --> J{process.stdin.isTTY?}
H --> J
J -- no TTY --> K["execFileSync npm install --no-save cwd=installCwd"]
J -- TTY --> L[Prompt user y/N]
L -- declined --> M[throw EngineError]
L -- accepted --> K
K -- success --> N[retry dynamic import]
K -- fails --> M
N -- success --> C
N -- fails --> O[throw ENGINE_UNAVAILABLE]
Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/1175-embed-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| execFileSync(NPM_BIN, ['install', packageName], { | ||
| stdio: 'inherit', | ||
| timeout: 300_000, | ||
| cwd: installCwd, | ||
| }); |
There was a problem hiding this comment.
--no-save. Now that cwd is pinned to codegraph's host directory, a user who confirms the prompt will silently add @huggingface/transformers to the package.json sitting in that host directory (e.g., the project root when codegraph is a local dep, or the npm global prefix). The non-TTY path on line 141 already passes --no-save correctly.
| execFileSync(NPM_BIN, ['install', packageName], { | |
| stdio: 'inherit', | |
| timeout: 300_000, | |
| cwd: installCwd, | |
| }); | |
| execFileSync(NPM_BIN, ['install', '--no-save', packageName], { | |
| stdio: 'inherit', | |
| timeout: 300_000, | |
| cwd: installCwd, | |
| }); |
There was a problem hiding this comment.
Fixed in 15f11ed — TTY install path now passes --no-save and pins cwd: installCwd symmetrically with the non-TTY path.
Codegraph Impact Analysis2 functions changed → 4 callers affected across 1 files
|
The non-TTY auto-install path already passes `--no-save`. The TTY path did not, which was fine when `cwd` defaulted to `process.cwd()` because that already polluted the user's working dir (a separate, original bug). Now that `cwd` is pinned to codegraph's host directory, omitting `--no-save` would silently add `@huggingface/transformers` to the `package.json` sitting next to codegraph's `node_modules` (the user's project root when codegraph is a local dep, or the npm global prefix when global-installed). Match the non-TTY path so installs stay ephemeral regardless of code path. Updates the corresponding assertion in tests/unit/prompt-install.test.ts.
|
@greptileai addressed the TTY install --no-save asymmetry you flagged — pushed 15f11ed. |
|
The fix looks correct. Both the TTY and non-TTY paths now consistently pass The change is minimal and targeted, the test assertion was updated to match, and the symmetry concern is fully resolved. LGTM. ✓ |
Mock createRequire so resolve returns a synthetic scoped package.json path, then assert the 4 dirname hops land at the host directory. Without this, only the catch-to-undefined branch was exercised and a miscounted dirname would pass CI silently. Closes follow-up suggestion from #1178 review.
|
@greptileai addressed the happy-path coverage gap you flagged in the review summary — pushed 2c96d78. Added two unit tests in |
|
The coverage additions look solid. Both branches of
One minor note: the The bonus This is ready to merge from a testing standpoint. LGTM. |
1 participant
Summary
embed's lazy install callsexecFileSync('npm', ['install', ...])with nocwd. Defaultcwd=process.cwd(), which is whatever directory the user invoked codegraph from — typically not the directory where codegraph itself is installed.The install lands in the user's project
node_modules(and silently grows a@huggingface/transformersdep there), but the subsequentawait import('@huggingface/transformers')resolves from codegraph's own package location, where the dep is missing. The user sees:Pin
cwdto the host directory that contains@optave/codegraph(resolved via_require.resolve('@optave/codegraph/package.json')then fourpath.dirnamehops). The install now lands in the samenode_modulesthat contains codegraph, so the dynamic import resolves correctly without polluting the user's working directory.Falls back to
cwd: undefined(≡process.cwd()) when@optave/codegraphcan't be resolved — typically a source-of-truth dev checkout — so the existing test suite (which mocks the import to fail) continues to pass.Found during
Dogfooding v3.10.1-dev.80 — see #1175.
Test plan
npx vitest run tests/unit/prompt-install.test.ts— all 5 existing tests pass with the change in place