Skip to content

feat: enhance native parser loading and add tests for missing bindings - #394

Open
nsdevaraj wants to merge 2 commits into
trailhq:mainfrom
nsdevaraj:main
Open

nsdevaraj wants to merge 2 commits into
trailhq:mainfrom
nsdevaraj:main

Conversation

@nsdevaraj

Copy link
Copy Markdown

Fix for native binding is missing for macOS ARM64 / Node 26.6.0.

@trailhq-graft

trailhq-graft Bot commented Sep 15, 2026

Copy link
Copy Markdown

🌱 graft blast radius

1 area changed → 6 areas can be affected. 22 dependent symbols, depth 2.
Tests: Graph Construction has tests the diff did not touch.
Tag: @anirudhkumar-nanonets — 5 of 7 areas · @shhdwi — 6 of 7 areas · @bhavesh-gupta-investis — Synchronous Claude Run

flowchart TB
  A0(("Pull Request Review<br/>8 symbols"))
  A1(("Workspace Graph Freshness<br/>6 symbols"))
  A2(("MCP Tool Invocation<br/>3 symbols"))
  A3(("Graph CLI Entry<br/>3 symbols"))
  A4(("Viewer Build Script<br/>1 symbol"))
  AX(("1 smaller area<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2,A3,A4 reached;
  classDef tail fill:#EEF2F3,stroke:#9AA4A9,stroke-width:1px,color:#3A4247;
  class AX tail;
Loading
Can be affected Symbols Nearest hop Reached from
Pull Request Review 8 src/app/brain-build.ts:L251-L358 readRepository — calls, depth 1 Graph Construction
Workspace Graph Freshness 6 src/graph/container.ts:L151-L208 extractContainer — calls, depth 1 Graph Construction
MCP Tool Invocation 3 src/mcp/tools.ts:L216-L244 callTool — calls, depth 2 Graph Construction
Graph CLI Entry 3 src/engine.ts:L91-L101 graph — calls, depth 1 Graph Construction
Viewer Build Script 1 scripts/build-viewer.mjs:L1-L45 build-viewer.mjs — calls, depth 2 Graph Construction
Synchronous Claude Run 1 src/claude/sync-run.ts:L19-L33 runSync — calls, depth 2 Graph Construction
Who knows this code — 3 people across 7 areas
Area Who knows it
Graph Construction · changed @anirudhkumar-nanonets — 20 commits, last 20d ago · @shhdwi — 10 commits, last 1mo ago
Pull Request Review · affected @anirudhkumar-nanonets — 12 commits, last 5d ago
Workspace Graph Freshness · affected @anirudhkumar-nanonets — 9 commits, last 1mo ago · @shhdwi — 6 commits, last 1mo ago
MCP Tool Invocation · affected @shhdwi — 14 commits, last 1mo ago · @anirudhkumar-nanonets — 7 commits, last 14d ago
Graph CLI Entry · affected @anirudhkumar-nanonets — 41 commits, last 6d ago · @shhdwi — 24 commits, last 1mo ago
Viewer Build Script · affected @shhdwi — 2 commits, last 1mo ago
Synchronous Claude Run · affected @shhdwi — 3 commits, last 2mo ago · @bhavesh-gupta-investis — 1 commit, last 23d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 22 dependent symbols, grouped by area

Pull Request Review — 8 symbols in 6 files

  • src/app/brain-build.ts:L251-L358 — readRepository (calls, depth 1)
    282: await buildGraph(checkout.dir, { graphOnly: true });
  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 1)
    55: await buildGraph(checkout.dir);
  • src/app/brain-build-worker.ts:L1-L83 — brain-build-worker.ts (calls, depth 2)
    12: import { readRepository, type BrainBuildJob, type RepoReadAuth } from "./brain-build.js";
  • src/app/brain-build-worker.ts:L29-L32 — DoneMessage (references, depth 2)
  • src/app/brain-build.ts:L237-L239 — buildRepoIntoBrain (calls, depth 2)
  • src/app/review-process.ts:L179-L183 — childReviewer (references, depth 2)
  • src/app/review-worker.ts:L67-L87 — run (calls, depth 2)
  • src/app/server.ts:L34-L46 — AppSeams (references, depth 2)

Workspace Graph Freshness — 6 symbols in 4 files

  • src/graph/container.ts:L151-L208 — extractContainer (calls, depth 1)
    171: inner = extractFile(rel, script, lang.inner);
  • src/graph/refresh.ts:L150-L227 — ensureFreshGraph (calls, depth 1)
    197: // and that rebuild very likely fixed the same drift we saw. Re-probe rather
  • src/graph/workspace.ts:L630-L655 — federateCheck (calls, depth 1)
    638: const g = await checkGraph(join(root, child));
  • src/graph/refresh.ts:L235-L261 — ensureFreshChildren (calls, depth 2)
  • src/graph/workspace-cli.ts:L49-L70 — buildChild (calls, depth 2)
  • src/graph/workspace-cli.ts:L126-L130 — runWorkspaceCheck (calls, depth 2)

MCP Tool Invocation — 3 symbols in 1 file

  • src/mcp/tools.ts:L216-L244 — callTool (calls, depth 2)
  • src/mcp/tools.ts:L247-L329 — callSingleTool (calls, depth 2)
    272: const g = await engine.checkGraph(root);
  • src/mcp/tools.ts:L153-L199 — callWorkspaceTool (calls, depth 2)

Graph CLI Entry — 3 symbols in 2 files

  • src/engine.ts:L91-L101 — graph (calls, depth 1)
    92: return buildGraph(dir, {
  • src/engine.ts:L82-L84 — checkGraph (calls, depth 1)
    82: checkGraph(dir: string): Promise<GraphCheckResult> {
  • src/cli.ts:L168-L178 — refreshBefore (calls, depth 2)

Viewer Build Script — 1 symbol in 1 file

  • scripts/build-viewer.mjs:L1-L45 — build-viewer.mjs (calls, depth 2)
    3: * assets). Runs as part of `npm run build`; the bundle ships in the package

Synchronous Claude Run — 1 symbol in 1 file

  • src/claude/sync-run.ts:L19-L33 — runSync (calls, depth 2)
Test signal per changed area — 1 ⚠

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Graph Construction — 3 of 7 reached · 36 test files reach it, none changed here
    • not reached: probeNative, swapWasmFallbackForTest, warmNativeFallbacks, parseSource
36 test suites also reference this code

47 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/ask-index.test.ts
  • test/ask.test.ts
  • test/container-extract.test.ts
  • test/context-only-dir.test.ts
  • test/context.test.ts
  • test/covers.test.ts
  • test/generic-extract.test.ts
  • test/graph-bindings.test.ts
  • test/graph-extract-dedup.test.ts
  • test/graph-go.test.ts
  • test/graph-incremental.test.ts
  • test/graph-invariants.test.ts
  • test/graph-java.test.ts
  • test/graph-languages.test.ts
  • test/graph-php.test.ts
  • test/graph-posix-paths.test.ts
  • test/graph-python.test.ts
  • test/graph-r-classes.test.ts
  • test/graph-r-phase3.test.ts
  • test/graph-r-phase4.test.ts
  • …16 more

⚠️ 1 changed file not in the graph (README.md) — no parser claims the extension, or the index predates the file.

graft blast · origin/main...HEAD · depth 2 · 5 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Sep 15, 2026

@Frankie-Xu Frankie-Xu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy-loading native grammars so a missing tree-sitter-kotlin does not take down graft --version or a TypeScript-only build is the right direction, and the dlopen probe tests are useful.

The product question for a required missing binding is already answered on Ready #214 / #119: fall back to the WASM grammar rather than throw. This PR's extractFile(Main.kt, …) path still throws No native build was found, and the README documents that as intentional. That is the opposite of #214 — on Windows without MSVC, or Node 26 without a prebuild, Kotlin (and PHP, Swift, …) would still fail the whole file instead of using tree-sitter-wasm.

src/graph/extract.ts is also the file #214 rewrites for the WASM fallback and Swift completion. I would not layer another native-loading pass onto #214 unless a maintainer wants these two merged. If this lands first, the remaining work is still throw → WASM, not more native require plumbing.

Not opening a parallel extract.ts PR.

github-actions Bot added a commit that referenced this pull request Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants