Skip to content

fix(cypher): refuse a query the parser did not fully read - #1875

Open
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/cypher-reject-trailing-tokens
Open

fix(cypher): refuse a query the parser did not fully read#1875
CaptainMittens wants to merge 1 commit into
DeusData:mainfrom
CaptainMittens:fix/cypher-reject-trailing-tokens

Conversation

@CaptainMittens

Copy link
Copy Markdown

What does this PR do?

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.

Reproductions (both on main, before this change)

MATCH (f:Function) WHERE f.name = 'buildTree' RETURN f.qualified_name AS qn BANANA SPLIT 99

Returns one row, no error. The trailing words are 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

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
WITH and everything after it had been dropped, and WHERE calls = 0 never
ran.

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_parse now checks that the cursor sits on the end-of-input token before
returning success, and names the leftover token when it does not. The message
also states that only one WITH clause is supported, since that is the limit
people 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_where parses the branch after UNION by calling cbm_parse on a
slice of the same token array, using a separate parser struct. The outer
parser's cursor therefore never moved past the UNION keyword. That cursor
was already wrong on main today; nothing caught it, because nothing checked
where the cursor ended up. Adding the end-of-input check is what exposed it —
three UNION tests went red the first time I ran the suite.

The UNION branch now moves the outer cursor to the end after the sub-parse
succeeds. That is sound only because the recursive cbm_parse no longer
returns 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:

Test Holds
cypher_parse_rejects_trailing_tokens The BANANA SPLIT 99 case is an error
cypher_parse_rejects_second_with_clause The 223-row query is an error
cypher_parse_accepts_single_with_clause One WITH + WHERE + RETURN still parses

The 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:

  cypher_parse_rejects_trailing_tokens      FAIL tests/test_cypher.c:219: rc == 0 (both 0)
  cypher_parse_rejects_second_with_clause   FAIL tests/test_cypher.c:241: rc == 0 (both 0)
  cypher_parse_accepts_single_with_clause   PASS
  184 passed, 2 failed

rc == 0 is the defect itself: cbm_parse reporting success on input it
never 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 test reports 7623 passed, 2 failed, 8 skipped on my
machine (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 on
the 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 UNION cursor fix that the
defect's own check uncovered and which cannot be separated from it. 82 lines
added, nothing removed, two files.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test) — not ticked, and
    here is why:
    7623 pass, 2 fail. Both failures are in
    tests/test_cli.c (1748, 6723), reproduce on a clean tree with this
    change 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.
  • Lint passes (make -f Makefile.cbm lint-ci) — cppcheck, clang-format
    and the NOLINT whitelist check all pass
  • 新建 behavior is covered by a test (reproduce-first for bug fixes)

🤖 Generated with Claude Code

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 also says that only one WITH clause is supported, because
that is the limit people actually meet.

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.

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>
@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.

注册 for free to join this conversation on GitHub. Already have an account? 登录 to comment

标签

None yet

项目

None yet

Development

Successfully merging this pull request may close these issues.

1 participant