fix(cypher): reject a non-numeric SKIP/LIMIT operand instead of dropping it

[metehanulusoy]

Fixes #1994

The defect

parse_return_or_with() accepted SKIP/LIMIT only when the next token was a number. When expect(p, TOK_NUMBER) failed it filled p->error and returned NULL, but both call sites only tested if (num) and then fell through to *out = r; return 0;. r->limit kept the -1 "unset" initialiser, which execution reads as "no LIMIT":

rb_apply_skip_limit(rb, ret->skip, ret->limit >= 0 ? ret->limit : max_rows);

So a bounded query ran unbounded and reported success. SKIP $offset LIMIT 10 lost both clauses at once, because the orphaned operand also blocked the following match(p, TOK_LIMIT).

Measured on the built binary before the fix, against a 26-node fixture:

query	rows
MATCH (n) RETURN n.name LIMIT 1	1
MATCH (n) RETURN n.name LIMIT $limit	26
MATCH (n) RETURN n.name SKIP $offset LIMIT 1	26
MATCH (n) RETURN n.name LIMIT abc	26

The change

Propagate the expect() failure in both branches — free_return_clause(r); return CBM_NOT_FOUND; — exactly as the ORDER BY branch directly above already does. cbm_parse() then surfaces the existing expected token type ... got ... at pos N message instead of silently continuing.

This is the failure mode #1334 banned ("the old failure mode — ignore the remainder, drop the LIMIT — must never come back"), reached by a different route.

Tests

Four cases in tests/test_cypher.c, next to the #1334 regression:

• cypher_parse_nonnumeric_limit_rejected_issue1994 — LIMIT $limit
• cypher_parse_nonnumeric_skip_rejected_issue1994 — SKIP $offset LIMIT 10, covering the swallowed second clause
• cypher_parse_word_limit_operand_rejected_issue1994 — LIMIT abc
• cypher_parse_numeric_skip_limit_still_accepted — control: SKIP 2 LIMIT 10 still parses and still carries 2/10

The first three fail on main and pass with this change; the control passes both ways.

Verification

• scripts/test.sh — green (clean ASan+UBSan build, all suites, contract steps, prod-binary guards)
• make lint-format with clang-format-20 (the version _lint.yml pins) — no drift
• make lint-tidy-diff — clean
• cppcheck — clean
• Branch is on current main (1778637) — not stale

One note on how I ran that: scripts/test.sh is green when invoked directly, but the scripts/hooks pre-commit hook failed five times in a row on git_context_linked_worktree. That turned out to be unrelated to this change — git exports GIT_DIR into hook environments, GIT_DIR overrides git -C, and the git-shelling tests in tests/test_pipeline.c therefore run against the real repository instead of their temp fixture. Reproduced without any hook: GIT_DIR=/path/to/repo/.git scripts/test.sh --suites pipeline fails, plain scripts/test.sh --suites pipeline passes. main with no changes shows the same thing, so it is pre-existing. I filed it separately as #2003 rather than bundling it here; this commit therefore used --no-verify, with the gates run by hand (all listed above).

Behavior change — worth calling out

This is stricter than main: a query whose SKIP/LIMIT operand is not a number now returns a parse error where it previously returned rows. Anyone who was unknowingly sending LIMIT $limit was already getting the wrong answer, so the error is the correct outcome — but it is a visible change, not a pure internal fix, and I would rather flag it than have it surprise you. If you would prefer a warning-and-continue instead of a hard error, say so and I will rework it.

Scope

One file changed in src/, 13 insertions / 6 deletions. No MCP tool signature change, no new dependency, no new system()/popen()/network call.

[github-actions]

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]

Approved on merit. Both CI failures are unrelated to your change — details below — so this needs a rebase and a re-run, not a code change.

The diagnosis holds

I checked it against main rather than taking it:

• cypher.c:1826-1834 — the SKIP and LIMIT branches both do if (num) { ... } with no else, then fall through to *out = r; return 0;.
• cypher.c:4823 — ret->limit >= 0 ? ret->limit : max_rows, so the untouched -1 initialiser reads as "no LIMIT". A bounded query runs unbounded and reports success.
• Your #1334 reference is exact: cypher_parse_order_by_over_cap_rejected_issue1334 (test_cypher.c:410) already established that this parser's over-cap case must be a loud parse error, and its comment names the same banned failure mode.

The fix is the right shape because it removes an inconsistency rather than adding a rule. The ORDER BY branch immediately above already does free_return_clause(r); return CBM_NOT_FOUND;, and so does the r->count > CBM_SZ_32 bound above that. SKIP/LIMIT were the two branches that didn't. After this, all four behave the same way, which is a much better state than three-plus-a-special-case.

The measured before-table is what makes this reviewable in one pass — LIMIT $limit → 26 rows against a 26-node fixture says "silently unbounded" in a way no prose does. And catching that SKIP $offset LIMIT 10 loses both clauses, because the orphaned operand blocks the following match(p, TOK_LIMIT), is the detail that justifies fixing both branches together.

The two red checks

test / test-msan — infrastructure. It fails at the "Build MSan image (cached layers)" step and the "MSan suite" step is skipped, so no test of yours ever ran. This is a known image-build failure on our side.

test / test-windows-guards — not attributable to this diff. It goes red on tests/windows/test_daemon_stability.py :: section_start_status_port ("the daemon did not accept the UI configuration; browser was not opened"), with two SETUP FAIL lines and three precondition skips alongside it.

I did look for a mechanism by which stricter SKIP/LIMIT parsing could break a daemon guard, because that is the one way this could genuinely be yours: if any shipped query used a non-numeric operand, it would now be rejected. It does not. Every LIMIT ?N / LIMIT %d in the tree is SQLite (store.c, mcp.c), which is a different parser, and the Windows guard suite does not exercise Cypher at all. The same job passed on another PR earlier today, so it is not a standing red either.

Please rebase — you are 3 commits behind (#1703) — and the re-run should settle both. If windows-guards stays red on a fresh base, tell me and I will take it; it will not be yours to fix.

Your GIT_DIR finding

git exports GIT_DIR into hook environments, GIT_DIR overrides git -C, and the git-shelling tests in tests/test_pipeline.c therefore run against the real repository instead of their temp fixture

That is a real bug and a good one — it means those tests silently assert against whatever repo state the developer happens to have, and it only shows up when something else (your pre-commit hook) sets GIT_DIR. Please open it as its own issue with the GIT_DIR=... scripts/test.sh --suites pipeline reproducer. It deserves a fix on its own terms rather than a footnote here, and running it down while it was blocking you — then correctly concluding it was unrelated — is exactly the right instinct.

One heads-up: #1998 also touches src/cypher/cypher.c, in check_projection_scope around line 5006. Different region, so no textual conflict, but whichever lands second may want a rebase.

Rebase and I will merge on green.

[metehanulusoy]

Rebased onto 5fbab7bb — clean, no conflicts; upstream's three commits do not touch either file. Cypher suite green on the new base (190 passed, including the four new cases).

On the GIT_DIR finding: already filed as #2003, with the GIT_DIR=... scripts/test.sh --suites pipeline reproducer and a note that it reproduces on clean main too, so it is not something my branch introduced. I linked it from the PR description shortly after opening, so you may have read the earlier version.

Thanks for tracing the two red checks yourself rather than bouncing them back to me.

[DeusData]

Rebased and re-running — thank you. You are now 10 commits behind rather than 3 behind a main that has moved four times today, which is as current as anything in the queue.

And thank you for filing #2003 for the GIT_DIR finding. That was the right call and I want to be clear it is not a footnote: git exporting GIT_DIR into hook environments, where it overrides git -C, means the git-shelling tests in tests/test_pipeline.c have been asserting against whatever repository the developer happened to be standing in. A green there proves less than it appears to, and that is worse than an outright failure — it is the same "silently wrong rather than loudly broken" shape as the defect this PR fixes. It is now tracked on our side.

My review stands: approved on merit, merging on green.

For the two reds you saw earlier — both were ours and neither was yours. test-msan failed at the image build step with the suite skipped, and test-windows-guards went red on test_daemon_stability.py, which does not exercise Cypher at all. I checked the one mechanism by which stricter SKIP/LIMIT parsing could genuinely have caused a Windows failure — a shipped query with a non-numeric operand — and there is none: every LIMIT ?N / LIMIT %d in the tree is SQLite, a different parser entirely.

Fair warning that green may take a while. Our Actions pool is servicing roughly one job at a time against 45 queued runs, so the delay is ours, not yours.