fix: one unloadable grammar no longer takes down the whole CLI (#323) - #337
fix: one unloadable grammar no longer takes down the whole CLI (#323)#337Ercaner1988 wants to merge 2 commits into
Conversation
🌱 graft blast radius1 area changed → 8 areas can be affected. 41 dependent symbols, depth 2. flowchart TB
A0(("Graph Construction<br/>16 symbols"))
A1(("Review Processing<br/>10 symbols"))
A2(("CLI Engine<br/>6 symbols"))
A3(("MCP Tools<br/>4 symbols"))
A4(("Language Enrichment<br/>2 symbols"))
AX(("3 smaller areas<br/>3 symbols"))
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;
Who knows this code — 4 people across 9 areas
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 All 41 dependent symbols, grouped by areaGraph Construction — 16 symbols in 8 files
Review Processing — 10 symbols in 6 files
CLI Engine — 6 symbols in 2 files
MCP Tools — 4 symbols in 1 file
Language Enrichment — 2 symbols in 1 file
Viewer Build — 1 symbol in 1 file
Context Build — 1 symbol in 1 file
Sync Execution — 1 symbol in 1 file
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.
39 test suites also reference this code48 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.
Open the interactive graph → — click an area to see its dependent symbols at file:line. |
faa9ee5 to
05c74b3
Compare
extract.ts imported all nine depth-tier grammars at the top of the module. They are native (node-gyp) modules, so any one of them failing to load took the whole CLI down at import time, before argv was read — `--version` and `--help` died exactly like a build, with a node-gyp-build stack trace that never says "graft". On the machine in trailhq#323 that grammar is tree-sitter-kotlin, which ships no prebuilds at all and so cannot load without a C toolchain. Load them through createRequire instead and keep what loads. The extension table is filtered by that, which is all the rest of the pipeline needs: an unavailable language stops claiming its files, and they take the paths a language graft has no grammar for takes today — the breadth tier where a generic row claims the extension, unindexed where none does. Warned once per language, the first time a file it would have claimed comes past, so a repo with no Kotlin in it stays quiet and a Kotlin repo never indexes short in silence. The test breaks a grammar both ways a module can be reached, `import` and `require`, so it still fails if the static imports ever come back. Refs trailhq#323 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r is gone The previous commit keeps the CLI alive when a native grammar will not load, but a language with no breadth-tier row then indexes nothing at all — and the machine in trailhq#323 is one where tree-sitter-kotlin can never load, because the package ships no prebuilds and there is no compiler to build one. Starting and indexing no Kotlin is not much of an answer on a Kotlin repo. tree-sitter-wasm ships a kotlin grammar and queries/kotlin.scm has been in the tree unused since Kotlin was promoted to the depth tier, so the fallback is one registry row. A row whose extension the depth tier also claims is unreachable while the native grammar loads — `.java` has been exactly that all along, which is why the "must not collide" note needed rewriting rather than an exception. Both warmGenericGrammars calls now apply the tier precedence their own parse loops apply, so a fallback row cannot warm a WASM grammar the run will never call. That was already true of `.java`; a second row would have doubled it. Measured on a three-file repo (.ts/.kt/.rs) with tree-sitter-kotlin broken: 6 nodes [rust, typescript] before this commit, 11 nodes [kotlin, rust, typescript] after. With the grammar healthy the graph is identical either way. Refs trailhq#323 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
05c74b3 to
2b42289
Compare
Implements suggestion (3) from #323, with (2) for Kotlin as a second commit you can drop on its own.
What happens today
src/graph/extract.tsimports all nine depth-tier grammars at the top of the module. They are native (node-gyp) modules, so any one of them failing to load takes the CLI down at import time, before argv is read — with anode-gyp-buildstack trace that never says "graft".On current
main, withtree-sitter-kotlinmade unloadable:I have hit this twice from unrelated causes: Kotlin, as in this issue, and
tree-sitter-runder Bun, where the binding asks for a prebuild filename that the scoped package does not ship (r-lib/tree-sitter-r#205, one-line fix in r-lib/tree-sitter-r#206). Both were one line to fix once found, and neither was findable from the error, which is the part fix (3) changes.Commit 1 — the isolation (fix 3)
createRequireplus a try/catch per grammar, keeping whatever loads. The extension table is filtered by that, which is all the rest of the pipeline needs:entryForstops claiming the language's extensions, and its files then take the paths a language graft has no grammar for takes today — the breadth tier where a generic row claims the extension, unindexed where none does.listSourceFilesnever enumerates them, sobuild.ts'sgeneric!stays safe.The warning is once per language, from
entryFor— the first time a file that language would have claimed comes past. A repo with no Kotlin in it stays quiet; a Kotlin repo never indexes short in silence:This is the contract your two WASM tiers already state for themselves — "an unavailable grammar is skipped rather than fatal" (container.ts), "degrades to a file node only (never throws)" (generic.ts). The native tier was the one that could not say it.
tree-sitteritself stays a static import: it is the shared dependency of all nine, and there is no depth tier without it.Commit 2 — Kotlin's fallback row (fix 2), droppable on its own
With commit 1 alone that machine starts but indexes no Kotlin at all, which on a Kotlin repo is still a bad day.
tree-sitter-wasmships a kotlin grammar andsrc/graph/queries/kotlin.scmhas been in the tree unused since Kotlin was promoted to the depth tier, so the fallback costs one registry row.A row whose extension the depth tier also claims is unreachable while the native grammar loads —
.javahas been exactly that all along. BothwarmGenericGrammarscalls now apply the tier precedence their own parse loops apply, so a fallback row cannot warm a WASM grammar the run will never call (already true of.java; a second row would have doubled it).Measured on a three-file repo (
.ts,.kt,.rs):tree-sitter-kotlin11 nodes, 8 edges [kotlin, rust, typescript]main6 nodes, 4 edges [rust, typescript], plus the warning11 nodes, 5 edges [kotlin, rust, typescript]On a healthy install the graph is identical before and after commit 2 — same nodes, same edges, compared field by field. The row really is unreachable.
graft checkagrees with the build it follows on the fallback path (graph check: OK, withRepo,describe,reloadandopenRepoall present), so this does not reopen the tier-mismatch shape of #236.Tests
test/grammar-unavailable.test.tsdrives the real CLI in a child process, withtest/break-grammar-preload.cjsstanding in for the missing native build — so it runs on a runner that has a compiler, which is why CI never caught the original. The preload breaks both ways a module can be reached,importandrequire, so the test still fails if the static imports ever come back. Each of the three fails without the commit it belongs to.npm teston this branch: 1223 tests, 1210 pass, 8 fail, 5 skipped — the same 8 that fail onupstream/mainbefore it, and no others. They aretoLocaleString()assertions that assume a,thousands separator, where thistr-TRmachine renders~100.000. Nothing to do with this PR: they are #338, fixed separately in #345, and this branch is deliberately independent of that one.The three tests this PR adds pass; each fails without the commit it belongs to.
One thing you may see and should not blame on this branch:
test/mcp-server.test.tstimed out on two of my five full-suite runs (itsrpc()helper waits a fixed 15s, then readsundefined.result). It is a load-sensitive deadline — those tests need ~2s to get a first response when run alone, and I measured MCP server startup identical on this branch and onmain(1.7–2.0s across five spawns each), so nothing here made the server slower. But this PR does add a test file that spawns three short-lived CLI processes, so it adds parallel load, and that deadline has little headroom on a busy machine.Environment: Node v24.16.0, Windows 11 x64, rebased onto
de8456e(0.18.0).No CHANGELOG entry
I dropped the one I had. 0.18.0 is already released, so there is no unreleased section to add to, and
de8456eshows the release commit is what writes them. Happy to add a bullet wherever you want it.Not in this PR
Loading the nine grammars costs ~220 ms, paid by every command including
--version.GRAMMAR_MODULESmakes lazy per-language loading a small follow-up, but that is a performance change and does not belong in a fix.🤖 Generated with Claude Code