Conversation
fed0c1b to
2eeb15f
Compare
2eeb15f to
5b4ea78
Compare
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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
left a comment
There was a problem hiding this comment.
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 symbol → unique 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
changescheck is our CI's fault. That job asks the GitHub API for the PR's diff, and the API answersHTTP 422: this diff is taking too long to generatefor 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.
Summary
PRAGMA,AUTOINCREMENT, SQLiteCREATE TRIGGER ... BEGIN ... END, and SQLiteIF NOT EXISTSmigration forms&in JSX text is accepted without changing HTML entity parsingMotivation
codebase-memory-mcp 0.11.0reports valid files as partially parsed:PRAGMA,AUTOINCREMENT, and SQLite trigger bodiesIdentity & contactBoth inputs pass their native validation, but the vendored parsers emit error regions. This causes
parse_partial_countto report false positives and can omit graph constructs.Validation
build/c/test-runner parse_coverage: 40/40 passedbuild/c/test-runner grammar_regression: passedscripts/security-vendored.sh: passedparse_partial_count=0,parse_unusable_count=0,nodes=1507,edges=3973tests/test_cli.c:11201), 10 skippedThe 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.