feat(graph): provision tree-sitter into embed-deps so the auto-build hook works#323
Merged
Conversation
…hook works The code-graph auto-build hook (graph-on-stop, on codex/cursor/hermes) resolves the native tree-sitter module at runtime from the shared ~/.hivemind/embed-deps/node_modules that each agent's plugin symlinks to. But `ensureSharedDeps` only installs @huggingface/transformers there, so on a real install tree-sitter is absent and the graph never auto-builds — the hook just degrades to a graceful skip (the CLI `hivemind graph build` works because it resolves tree-sitter from its own package). This provisions the parsers where the hook actually looks. tree-sitter is a native module: symlinking the package dir alone is not enough (it needs its own node-gyp-build / node-addon-api siblings resolved), so we run a scoped npm install and let npm lay down the full tree. - src/cli/embeddings.ts: new `ensureGraphDeps()` — idempotent (specs-hash marker + per-package presence check catch partial installs and version bumps) and best-effort (any failure is logged and swallowed, never aborts the caller). Two-phase native provisioning: `npm install --ignore-scripts` so a platform without a prebuild (arm64 / Node 24) doesn't fail the download, then scripts/ensure-tree-sitter.mjs ALWAYS runs to validate bindings + compile from source where needed (fast no-op on healthy prebuilds; also repairs an interrupted / ABI-mismatched install). Versions are read from the package's own optionalDependencies (`treeSitterSpecs`) so embed-deps never drifts from what the bundles were built against. Decoupled from the ~600 MB embeddings download — installs only the parsers (tens of MB). - src/cli/embeddings.ts: `ensureSharedDeps` now MERGES the shared package.json instead of overwriting it, so a prior graph-deps declaration survives a first-time embeddings install. - Wired into `installEmbeddings()` and `hivemind graph init` (best-effort). - tests: treeSitterSpecs (version derivation, no drift, excludes transformers) + isGraphDepsInstalled (all-parsers-present / partial-install). Verified e2e: `graph init` provisions tree-sitter into a sandbox embed-deps (12 pkgs), a second run skips the reinstall (idempotent), and the codex graph-on-stop hook then builds the graph (Nodes: 5, Edges: 2). Full suite 5546 passed. Codex review: both prior HIGH findings resolved. Known limitation (pre-existing, unchanged): npm is invoked as bare `npm`, which does not resolve on Windows — same as the existing ensureSharedDeps invocation; a proper cross-platform npm exec is out of scope here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI provisions tree-sitter parser dependencies in shared embedding dependencies, heals native bindings, preserves existing dependency declarations, and invokes provisioning during embeddings installation and graph initialization. Tests cover discovery, installation state, marker handling, failures, command boundaries, and embedding workflows. ChangesGraph dependency provisioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ensureGraphDeps
participant SharedDependencies
participant npm
participant NativeHealing
CLI->>ensureGraphDeps: provision graph dependencies
ensureGraphDeps->>SharedDependencies: read metadata and parser state
ensureGraphDeps->>npm: install tree-sitter specs
npm-->>SharedDependencies: write parser packages
ensureGraphDeps->>NativeHealing: heal native bindings
NativeHealing-->>CLI: complete best-effort provisioning
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 3 files changed
Generated for commit 725135e. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/cli/embeddings.ts`:
- Around line 215-225: The shared-dependency installation in the embeddings
install flow must disable package lifecycle scripts. Update the npm install
invocation associated with the package.json written after `ensureGraphDeps` to
include the `--ignore-scripts` option, preserving the existing dependency merge
and installation behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cfcbd462-e011-4233-b06e-6e5f4421c4ab
📒 Files selected for processing (3)
src/cli/embeddings.tssrc/cli/index.tstests/shared/graph-deps.test.ts
Respond to CodeRabbit's Major on the shared-deps install: document that the tree-sitter parsers are intentionally recorded in the shared package.json. A plain `npm install` PRUNES unsaved packages (verified on npm 11), so an unsaved install would be wiped by ensureSharedDeps' transformers reconcile. Being declared, they are neither pruned nor (once present and satisfied) rebuilt by that reconcile — so ensureSharedDeps stays free of --ignore-scripts, which would break onnxruntime-node / sharp (both run real native install steps).
…age gate The graph-deps provisioning logic lived in src/cli/embeddings.ts (~50% covered, ungated), so its new code wasn't held to the repo's per-file bar. Extract it to a dedicated, fully unit-tested module and gate it. - src/cli/graph-deps.ts (new): treeSitterSpecs, isGraphDepsInstalled, and ensureGraphDeps, moved verbatim. ensureGraphDeps now takes an injectable deps object (sharedDir/sharedNodeModules/specs/runNpm/runHeal/logFn/warnFn) whose defaults reproduce the prior inline behavior exactly (same `npm install --ignore-scripts`, same ensure-tree-sitter.mjs heal spawn), so the branch logic can be unit-tested without spawning npm or compiling. - embeddings.ts / index.ts import ensureGraphDeps from the new module. - tests/shared/graph-deps.test.ts: cover every ensureGraphDeps branch — empty specs, fresh install + marker + heal, already-present skip, stale-marker and partial-install reinstall, existing-package.json preservation, npm/heal failure swallowed, and a default-boundaries test that mocks node:child_process.execFileSync to exercise the real npm + heal wrappers. - vitest.config.ts: gate src/cli/graph-deps.ts at 90/85/90/90. Coverage: graph-deps.ts 100% stmts / 93.5% branch / 100% funcs / 100% lines. Full suite + coverage exit 0. Codex review: refactor sound, no blocker.
| if (haveKey !== wantKey || !isGraphDepsInstalled(sharedNodeModules, specs)) { | ||
| logFn(` Graph installing tree-sitter parsers into ${sharedDir} (code-graph; ~tens of MB)`); | ||
| runNpm(specs, sharedDir); | ||
| writeFileSync(marker, wantKey); |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/shared/graph-deps.test.ts (2)
180-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate function call.
There appears to be a duplicate call to
ensureGraphDepson this line. Unless there's an intention to test idempotency here (which isn't asserted), the duplicate line should be removed.♻️ Proposed fix
- ensureGraphDeps({ sharedDir: dir, sharedNodeModules: nm, specs: SPECS, logFn: () => {}, warnFn: () => {} }); ensureGraphDeps({ sharedDir: dir, sharedNodeModules: nm, specs: SPECS, logFn: () => {}, warnFn: () => {} });🤖 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/shared/graph-deps.test.ts` around lines 180 - 192, Remove the unintended duplicate ensureGraphDeps call in the test case, retaining the single invocation that exercises the default runNpm/runHeal wrappers and the existing execFileSync assertions.
116-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert on the specific warning message.
As per path instructions, prefer asserting on specific values (paths, messages) over generic substrings in tests. Ensure the exact warning messages, including error details where applicable, are asserted instead of using
expect.stringContaining.
tests/shared/graph-deps.test.ts#L116-L124: replaceexpect.stringContaining("no tree-sitter optionalDependencies")with the exact message" Graph no tree-sitter optionalDependencies found in package.json — skipping".tests/shared/graph-deps.test.ts#L165-L172: replaceexpect.stringContaining("tree-sitter provisioning failed")with" Graph tree-sitter provisioning failed (npm exploded); code graph stays disabled — everything else works".tests/shared/graph-deps.test.ts#L173-L179: replaceexpect.stringContaining("tree-sitter provisioning failed")with" Graph tree-sitter provisioning failed (heal exploded); code graph stays disabled — everything else works".🤖 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/shared/graph-deps.test.ts` around lines 116 - 124, Update the warning assertions in tests/shared/graph-deps.test.ts at lines 116-124, 165-172, and 173-179 to compare exact messages instead of using expect.stringContaining: use the specified complete no-optionalDependencies message at 116-124, and the complete npm-exploded and heal-exploded provisioning messages at 165-172 and 173-179 respectively.Source: Path instructions
🤖 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.
Nitpick comments:
In `@tests/shared/graph-deps.test.ts`:
- Around line 180-192: Remove the unintended duplicate ensureGraphDeps call in
the test case, retaining the single invocation that exercises the default
runNpm/runHeal wrappers and the existing execFileSync assertions.
- Around line 116-124: Update the warning assertions in
tests/shared/graph-deps.test.ts at lines 116-124, 165-172, and 173-179 to
compare exact messages instead of using expect.stringContaining: use the
specified complete no-optionalDependencies message at 116-124, and the complete
npm-exploded and heal-exploded provisioning messages at 165-172 and 173-179
respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89c42cc7-8b31-47ae-9c91-92c8c162612f
📒 Files selected for processing (5)
src/cli/embeddings.tssrc/cli/graph-deps.tssrc/cli/index.tstests/shared/graph-deps.test.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/index.ts
…> 87%) installEmbeddings, ensureSharedDeps, statusEmbeddings, and uninstall --prune used module-level HOME/SHARED_DIR with no injectable seam, so they were unreachable from the existing per-function tests. Add a sandboxed suite: sandbox the home dir via tests/shared/fake-home (POSIX + Windows) BEFORE importing embeddings.ts, and mock only node:child_process.execFileSync so the npm installs never spawn. Restores env + removes the tmp home in afterAll. Covers: fresh install (provisions shared deps + flips the config flag), already-present skip path, agent symlink wiring, prune vs non-prune uninstall, and statusEmbeddings output. embeddings.ts: 52% -> 87% lines. Codex review: sound (env-restore + Windows-home fixes applied).
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/cli/cli-embeddings-install.test.ts`:
- Around line 135-137: Replace the loose output checks in the affected CLI
embedding-install tests with strict assertions matching the exact expected
output lines: update both checks at tests/cli/cli-embeddings-install.test.ts
lines 135-137 and the “codex” check at line 151. Preserve the intended messages
while avoiding generic regex or substring matching.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 846e7cc7-7474-4e57-ac83-2bcbf1f3cb9b
📒 Files selected for processing (1)
tests/cli/cli-embeddings-install.test.ts
Address CodeRabbit (tests/** path instruction): the statusEmbeddings tests matched the loose substring "codex", which could pass on an incidental match. Assert the exact rendered lines instead — `Installed: no/yes` and the per-agent `codex ✗ not linked` / `codex ✓ linked → shared` label.
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
2 participants
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.
Problem
The code-graph auto-build hook (
graph-on-stop, on codex/cursor/hermes) resolves the nativetree-sittermodule at runtime from the shared~/.hivemind/embed-deps/node_modulesthat each agent's plugin symlinks to. ButensureSharedDepsonly installs@huggingface/transformersthere — tree-sitter is never provisioned into embed-deps, so on a real install the graph never auto-builds. The hook just degrades to a graceful skip (thanks to the crash fix in #321).Note: the CLI
hivemind graph build/graph initdoes work in prod (verified end-to-end via a cleannpm packinstall) because it resolves tree-sitter from its own package. This PR fixes the hook path.Fix
ensureGraphDeps()(src/cli/embeddings.ts) — provisions the tree-sitter parsers into embed-deps where the hook looks. tree-sitter is a native module (needs itsnode-gyp-build/node-addon-apisiblings resolved), so a scopednpm installlays down the full tree rather than symlinking a single dir..graph-depsspecs-hash marker + a per-package presence check skip the re-download and catch partial installs / version bumps.npm install --ignore-scripts(so a platform without a prebuild — arm64 / Node 24 — doesn't fail the download), thenscripts/ensure-tree-sitter.mjsalways runs to validate bindings + compile from source where needed (fast no-op on healthy prebuilds; also repairs an interrupted / ABI-mismatched install).optionalDependencies.ensureSharedDepsnow MERGES the shared package.json instead of overwriting it, so a prior graph-deps declaration survives a first-time embeddings install.installEmbeddings()andhivemind graph init(best-effort).Verification
End-to-end in an isolated sandbox (
HOMEoverride):hivemind graph init→ensureGraphDepsinstalls 12 packages into the sandbox embed-deps; all 10 parsers present + marker written.graph init→already present(idempotent, no reinstall).graph-on-stophook, with embed-deps now populated, builds the graph:Nodes: 5, Edges: 2, snapshot written, exit 0.Known limitation (pre-existing, unchanged)
npm is invoked as bare
npm, which does not resolve on Windows — identical to the existingensureSharedDepsinvocation. A proper cross-platform npm exec is out of scope here (it affects both paths).Session Context
Session transcript
Summary by CodeRabbit
graph initnow best-effort provisions required Tree-sitter parser dependencies (non-blocking on failures).package.jsonis no longer clobbered; dependency updates are merged.