fix(cypher): refuse a query the parser did not fully read - #1875
Conversation
|
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. |
c0a9671 to
ddea061
Compare
|
This is the parent of the family: #1877, #1880 and #1881 all came from a scan for siblings of the shape this PR fixes — a parse reports success while the input stays unread. Same argument as #1922: the code answers confidently instead of saying it could not read its input, and nothing downstream can tell that answer from a real one. Re-reading my own change before asking for a look, one thing to flag and one nit. The line worth a reviewer's attention is The nit is mine. The error message appends "Note that only one WITH clause is supported." unconditionally, so |
ddea061 to
d21bb13
Compare
|
Went ahead and fixed the nit I raised above, so you do not need to answer that question — amended in place, The note about the one-WITH limit is now appended only when a standalone Both rejection tests now pin the message. The
If you would rather this PR had stayed exactly as it was reviewed, reverting is one commit — say so and I will put it back. |
|
The red That is #1952's signature exactly, and the job also prints the I cannot rerun the job from the fork ( |
|
Approved. This is the most consequential of the four PRs from your parse audit, and the reproduction is what makes that clear. A query that refuses is an annoyance; a query that answers confidently with unfiltered rows sends someone off to act on them. 223 rows including functions you had just proven had callers, with the only clue being the column names, is exactly the failure mode worth prioritising. The UNION coupling is not scope creep and you argued it correctly. The red-green evidence is the strongest form of it. Removing only the guard and showing exactly two failures plus the control passing proves the guard is load-bearing for those two behaviours and nothing else. A guard like this is easy to over-tighten, and And leaving the checklist box unticked was right. "I would rather leave the box honest than tick it with a footnote" — yes. For what it is worth, those two Your CI red is not yours
Everything that needs an indexed repo then skipped on a failed precondition — I have now seen the identical cluster on three unrelated pull requests across four days, so it is a property of that lane rather than of any change in it. That is ours to fix and I am recording it as such — please do not chase it. Please rebase — Working the class rather than the bug — this, then the confidence pair in #1877, timestamps in #1881, environment settings in #1880 — is why those four read as one audit. Thank you. |
DeusData
added
bug
cbm_parse built a query and returned success without checking that it had read every token. The grammar accepts at most one WITH and treats RETURN as optional, so the parser stopped at the first thing it did not understand and reported success anyway. The dropped tail took the filter and the RETURN with it. The engine then answered from the fragment it had parsed, using its default projection. It reported success and returned wrong rows, which is worse than a refusal, because nothing tells the caller to look. Two shapes hit this: MATCH (f:Function) WHERE f.name = 'x' RETURN f.name AS n BANANA SPLIT 99 -> one row, no error, the trailing words silently dropped MATCH (f:Function) OPTIONAL MATCH (a)-[:CALLS]->(f) WITH f, count(a) AS calls OPTIONAL MATCH (b)-[:USAGE]->(f) WITH f, calls, count(b) AS usages WHERE calls = 0 AND usages = 0 RETURN f.name AS n -> every function, unfiltered, under the default column names cbm_parse now checks that the cursor sits on the end-of-input token before it returns success, and names the leftover token when it does not. The message adds a note about the one-WITH limit, but only when a standalone WITH really sits in the part that went unread. Appending it every time pointed the reader at WITH when the problem was a typo -- the BANANA query above holds no WITH anywhere. The test cannot simply look at the token the parse stopped on either: on the second-WITH query above it stops at OPTIONAL rather than at WITH, because parse_post_where consumes the first WITH and then has no way to take another MATCH stage. So it scans the unread tail, and skips a WITH straight after STARTS, which is the STARTS WITH operator rather than a clause. The check exposed a second defect. parse_post_where parses the branch after UNION by calling cbm_parse on a slice of the same tokens, using a separate parser. The outer parser's cursor never moved past the UNION keyword, so a valid UNION query looked unfinished. That cursor was already wrong; nothing caught it, because nothing checked where the cursor ended up. The UNION branch now moves the cursor to the end after the sub-parse succeeds, which is sound because that sub-parse no longer returns success with tokens left over. Three tests cover both directions. Removing only the guard turns the two rejection tests red with rc == 0 -- the parser reporting success on input it never finished reading -- while the acceptance test stays green, so the guard is load-bearing for exactly these two behaviours. The two rejection tests also pin the message. The BANANA case must name BANANA and must not mention WITH; that assertion was seen red first, at tests/test_cypher.c:226. The second-WITH case must still carry the note; that one passed before and after, and is the control that stops the note being suppressed everywhere instead of only where it misleads. Verified on macOS with Apple clang: make -f Makefile.cbm test-focused TEST_SUITES=cypher -> 186 passed guard removed, tests kept -> 184 passed, 2 failed make -f Makefile.cbm cbm -> exit 0, no warnings The full suite reports 7623 passed, 2 failed. Both failures are in tests/test_cli.c (lines 1748 and 6723) and reproduce on a clean tree with this change stashed out. They depend on the coding agents installed on the machine, not on this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
d21bb13 to
25a7aac
Compare
|
Rebased onto current The rebase applied cleanly. Built and ran the cypher suite on the rebased tree: 189 passed, which is the branch's three new tests plus the scope work that arrived in the same file. Thank you for chasing the |
|
Merged as Worth restating why it matters: a query the parser did not fully read is the silently wrong failure, not the loud one. Answering from a partial parse returns rows that look authoritative while describing a query nobody wrote — strictly worse than refusing, because nothing in the response says so. Refusing is the right call. The red you saw on the earlier run was ours, not yours, which is why the rebase alone was enough to clear it. |
2 participants
What does this PR do?
cbm_parsebuilt a query and returned success without checking that it hadread every token. The grammar accepts at most one
WITHand treatsRETURNas optional, so the parser stopped at the first thing it did not understand
and reported success anyway.
The dropped tail took the filter and the
RETURNwith it. The engine thenanswered from the fragment it had parsed, using its default projection. It
reported success and returned wrong rows — which is worse than a refusal,
because nothing tells the caller to look.
Reproductions (both on
main, before this change)Returns one row, no error. The trailing words are silently dropped.
Returns every function in the project, unfiltered, under the default column
names rather than the requested alias. I hit this one for real while hunting
dead code: it returned 223 rows including functions I had just proven had
callers. The only clue anything was wrong was the column names — the second
WITHand everything after it had been dropped, andWHERE calls = 0neverran.
That second case is the reason I am filing this. A query that refuses is a
minor annoyance. A query that answers confidently with the wrong rows sends
you off to act on data that was never filtered.
The fix
cbm_parsenow checks that the cursor sits on the end-of-input token beforereturning success, and names the leftover token when it does not. The message
also states that only one
WITHclause is supported, since that is the limitpeople actually meet in practice.
Why this touches UNION
This is the part worth reviewing hardest, and it is not scope creep — the
check could not land without it.
parse_post_whereparses the branch afterUNIONby callingcbm_parseon aslice of the same token array, using a separate parser struct. The outer
parser's cursor therefore never moved past the
UNIONkeyword. That cursorwas already wrong on
maintoday; nothing caught it, because nothing checkedwhere the cursor ended up. Adding the end-of-input check is what exposed it —
three
UNIONtests went red the first time I ran the suite.The
UNIONbranch now moves the outer cursor to the end after the sub-parsesucceeds. That is sound only because the recursive
cbm_parseno longerreturns success with tokens left over, so the two changes depend on each
other and neither is safe alone.
Tests
Three tests in
tests/test_cypher.c, covering both directions:cypher_parse_rejects_trailing_tokensBANANA SPLIT 99case is an errorcypher_parse_rejects_second_with_clausecypher_parse_accepts_single_with_clauseWITH+WHERE+RETURNstill parsesThe third one is the control. A guard like this is easy to over-tighten, and
without it a green suite would not distinguish "still accepts valid queries"
from "started rejecting everything".
Red-green evidence
Removing only the guard, keeping the tests:
rc == 0is the defect itself:cbm_parsereporting success on input itnever finished reading. Restoring the guard returns 186 passed. The control
test passes in both states, so the guard is load-bearing for exactly these two
behaviours and nothing else.
A note on the full suite
make -f Makefile.cbm testreports 7623 passed, 2 failed, 8 skipped on mymachine (macOS 15, Apple clang). Both failures are in
tests/test_cli.c(lines 1748 and 6723) and I confirmed they reproduce on a clean tree with this
change stashed out — 284 passed, 2 failed, same two line numbers. They fail
with
error: one or more agent cleanup operations failed, which depends onthe coding agents installed on the machine, not on this change.
Flagging it because a contributor running the full suite will see red and may
assume they caused it. Happy to open a separate issue if that is not already
known.
Scope
Per CONTRIBUTING.md this is filed without a prior issue under the bug-fix
exception (line 124). It is one defect, plus the
UNIONcursor fix that thedefect's own check uncovered and which cannot be separated from it. 82 lines
added, nothing removed, two files.
Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test) — not ticked, andhere is why: 7623 pass, 2 fail. Both failures are in
tests/test_cli.c(1748, 6723), reproduce on a clean tree with thischange stashed out, and depend on the coding agents installed on the
machine. The cypher suite this change touches is 186/186. I would
rather leave the box honest than tick it with a footnote.
make -f Makefile.cbm lint-ci) — cppcheck, clang-formatand the NOLINT whitelist check all pass
🤖 Generated with Claude Code
Fixes #1979