Skip to content

fix: clear false partial-parse warnings for SQLite and TSX - #2246

Open
kerfern wants to merge 1 commit into
DeusData:mainfrom
kerfern:kerfern/fix-sqlite-tsx-partial
Open

kerfern wants to merge 1 commit into
DeusData:mainfrom
kerfern:kerfern/fix-sqlite-tsx-partial

Conversation

@kerfern

@kerfern kerfern commented Sep 20, 2026

Copy link
Copy Markdown

Summary

  • regenerate vendored SQL parser with SQLite support for PRAGMA, AUTOINCREMENT, SQLite CREATE TRIGGER ... BEGIN ... END, and SQLite IF NOT EXISTS migration forms
  • regenerate vendored TSX parser so raw & in JSX text is accepted without changing HTML entity parsing
  • add parse-coverage regressions for valid SQLite migrations, valid TSX JSX text, and malformed SQL
  • refresh vendored parser checksums

Motivation

codebase-memory-mcp 0.11.0 reports valid files as partially parsed:

  • SQLite migrations using PRAGMA, AUTOINCREMENT, and SQLite trigger bodies
  • TSX containing ordinary JSX text such as Identity & contact

Both inputs pass their native validation, but the vendored parsers emit error regions. This causes parse_partial_count to report false positives and can omit graph constructs.

Validation

  • build/c/test-runner parse_coverage: 40/40 passed
  • build/c/test-runner grammar_regression: passed
  • scripts/security-vendored.sh: passed
  • re-indexed VEMCsite with this binary: parse_partial_count=0, parse_unusable_count=0, nodes=1507, edges=3973
  • full C suite: 8082 passed, 1 unrelated CLI lifecycle failure (tests/test_cli.c:11201), 10 skipped

The vendored parsers remain pinned to the commits recorded in internal/cbm/vendored/grammars/MANIFEST.md; the manifest records generator versions and local grammar additions, and the checksum manifest is refreshed for the generated artifacts.

@kerfern
kerfern requested a review from DeusData as a code owner September 20, 2026 09:38
@kerfern
kerfern force-pushed the kerfern/fix-sqlite-tsx-partial branch from fed0c1b to 2eeb15f Compare September 20, 2026 09:40
@kerfern
kerfern force-pushed the kerfern/fix-sqlite-tsx-partial branch from 2eeb15f to 5b4ea78 Compare September 20, 2026 09:58
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for this — both defects are real, and you characterised them the way I wish every report did: valid input, the native tool accepts it, our parser reports an error region, and parse_partial_count goes up for a file that is fine. A false "partially parsed" is worse than it sounds, because it teaches people to ignore the one signal that tells them the graph is missing something. The tests are good too: sqlite_malformed_still_flagged is exactly the control that stops "fixed" from meaning "stopped reporting errors".

What I checked, so you know where this stands. A regenerated LR parser cannot be reviewed by reading 959,393 lines, so I ran the exhaustive checks we use for vendored grammars on both new blobs, and the same checks on the blobs they replace:

sql (old → new) tsx (old → new)
bytes > 0x7F / control bytes other than LF 0 / 0 → 0 / 0 0 / 0 → 0 / 0
string literals containing a space none → none unique symbolunique symbol
comments 1 → 1 (the generator banner) 0 → 0
SYMBOL_COUNT 770 → 773 393 → 394
STATE_COUNT 30,622 → 31,010 5,986 → 5,986
ABI / EXTERNAL_TOKEN_COUNT 15 / 3 → 15 / 3 14 / 10 → 14 / 10

Zero non-ASCII bytes means hidden-character tricks are arithmetically impossible rather than merely unobserved; no spaced literals means no prose; and the symbol deltas (+3, +1) are the size your description predicts. scanner.c is untouched in both grammars and the licences are unchanged. I have no safety concern with these files.

What I cannot do yet is the other half — prove the bytes are upstream plus your declared change and nothing else. That is the substance of what I would like changed:

1. The grammar change itself needs to be in the repository, not only its output. MANIFEST.md describes the SQLite additions and the jsx_text_ampersand token in prose, but the grammar.js edits that produced them are not in the diff. Without them nobody — including you, six months from now — can regenerate these parsers, re-apply the change on the next upstream refresh, or verify the result by hash. Every local divergence we carry today is a few lines in a scanner.c that a reader can see in the MANIFEST's "Local source patches" table; a grammar-level change is a different size of thing and needs its source. Concretely: the patch against the pinned upstream commit for each grammar (a plain git diff of grammar.js is ideal), plus the exact regenerate commands, and a row for each in that table. With that, upstream @ pinned sha + your patch → generate → same sha256 as scripts/vendored-checksums.txt becomes a check anyone can run, and I will run it.

2. DCO sign-off. The commit has no Signed-off-by: trailer, which is why dco is red. git commit --amend -s and a force-push fixes it — force-pushing your own PR branch is completely fine here. A pseudonymous or noreply identity is acceptable, as long as it matches the commit author.

3. Two PRs rather than one, please. SQLite migrations and JSX ampersands are unrelated defects in unrelated grammars. Split, each can be verified, merged, bisected and — if it ever comes to it — reverted on its own, and a problem in one does not hold the other hostage. The TSX change is the smaller and more self-contained of the two and could land first.

4. Have you offered these upstream? A raw & in JSX text being rejected is a long-standing gap in tree-sitter-javascript, and SQLite's PRAGMA / AUTOINCREMENT / trigger bodies would help every user of tree-sitter-sql, not only us. If upstream takes them, our local patch row disappears at the next refresh, which is the best outcome for everyone. Not a condition for merging — but if you open those PRs, link them in the MANIFEST row.

Two things that are on our side, not yours, so you do not spend time on them:

  • The red changes check is our CI's fault. That job asks the GitHub API for the PR's diff, and the API answers HTTP 422: this diff is taking too long to generate for a parser this size. It would fail identically for any grammar refresh. I have recorded it and it will not be held against this PR.
  • Carrying a locally modified grammar — as opposed to a pinned upstream one — is a maintenance commitment we have so far only made for grammars we wrote ourselves (tools/tree-sitter-*). I think the case here is good, because the alternative is telling SQLite users their valid migrations are broken, but it is a decision I want to make deliberately rather than by merging. Item 1 is what makes it a decision I can say yes to.

One small note on the validation section: the tests/test_cli.c:11201 failure you saw in the full suite is not yours — line 11201 is the VS Code agent-install lifecycle test, which nothing in this PR touches. I have not reproduced it here, so I am not going to call it a known flake; if it fails for you on an unmodified main as well, a separate issue with the VS Code durable diag … line it prints would be very welcome.

Thank you again. This is careful work on a genuinely annoying problem, and I would like to see it land.

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