fix(cypher): refuse a query naming more variables than a binding holds

[CaptainMittens]

The out-of-scope RETURN refusal added by #1922 stops working on a wide query. Same
undeclared variable, same clause, opposite answers — the only difference is how many names
the pattern declares:

# 10 declared names
MATCH (a0:NoSuchLabelXYZ)-[:CALLS]->(a1)-…->(a9) RETURN zzz.name
  → variable 'zzz' is not in scope for RETURN — no pattern in this query names it

# 35 declared names
MATCH (a0:NoSuchLabelXYZ)-[:CALLS]->(a1)-…->(a34) RETURN zzz.name
  → rows: 0  (cols: zzz.name)

check_projection_scope models declared names in a fixed 32-entry array.
collect_declared_names answers -1 when a query declares more, and the caller then
returns NULL — no check at all. Nothing bounds how many variables a pattern can declare,
because pat->node_count grows on demand.

Why the fix is a bound and not a bigger array

That -1 branch was deliberate, and its comment says why: a wrong refusal costs the caller
a working query, which is worse than the silence. The premise is what turned out to be
wrong.

A query naming more variables than a binding can hold cannot be answered for the names
past the bound. A binding_t holds exactly CYP_MAX_VARS node variables and
CYP_MAX_EDGE_VARS edge variables in plain arrays. binding_set / binding_set_edge
append in call order and drop anything past those without a word. Names inside the bound
still bind correctly, so a query that declares more but projects only those does answer
correctly today, and this change refuses it too. Names past the bound are the ones that
cannot be answered: they bind to nothing and project as empty strings, which reads as "the
graph holds no such data". For those the choice was never "refuse a working query or stay
quiet" — it was "refuse it, or answer it wrong". The remedy the error names is exact for
both cases, because an unnamed node takes no slot.

check_pattern_var_capacity refuses it, before any row is touched:

too many node variables: a query can name at most 16 — leave the name off the ones you
do not use, or run separate queries

Both ways out are real ones. An unnamed node takes no slot, so dropping a name the query
never uses is the cheap one. Splitting the MATCH is deliberately not offered: every
pattern in one query shares one binding, which is why the check counts across all of them.
Separate queries do work, because each gets a binding of its own.

One bound, four silent sites

Bounding the input rather than each consumer closes three further drops that were all the
same root cause:

Site	Past its bound before	After
check_projection_scope	stops checking scope above 32 names	cannot overflow: 16 + 8 + 1 UNWIND alias = 25
binding_set	never binds the 17th node variable	unreachable
binding_set_edge	never binds the 9th edge variable	unreachable
execute_default_projection	stops collecting columns at 16	unreachable

The last three would otherwise need an error path threaded up through every caller, since
both binding_set and binding_set_edge return void. Bounding the input removes that
need.

The two < 0 branches in check_projection_scope stay, with comments saying they can no
longer fire. They cost nothing and they are what keeps the function correct if either bound
ever moves.

Scope of the change

The check counts DISTINCT variables across every pattern, because they all land in one
binding: a multi-MATCH query shares one, and an OPTIONAL MATCH pattern sits in the same
q->patterns array. It runs once per UNION arm, in the loop that already runs the scope
check.

Nothing real is caught by the bound. The widest pattern anywhere in this repository names
three variables, across tests/, src/ and internal/.

Tests

Three in tests/test_cypher.c, each anchored on a label that matches nothing so they are
instant and depend on no fixture:

Test	Covers
cypher_wide_pattern_refused	20 node variables refused, 16 still succeed
cypher_wide_edge_pattern_refused	9 named edge variables refused
cypher_scope_check_survives_wide_pattern	the report above — RETURN zzz.name is refused at 10 declared names AND at 35

All three verified failing before the change, each at the assertion that the query was
accepted, and passing after.

Note the wide case is refused for width, not for scope — the capacity check runs first, so
the message names the limit rather than zzz. Either way the caller gets an error instead
of a column of blanks, which is what #1995 asks for. Through the built binary:

10 names → variable 'zzz' is not in scope for RETURN — no pattern in this query names it
35 names → too many node variables: a query can name at most 16 — leave the name off the
           ones you do not use, or run separate queries

The 35-name line previously read rows: 0 (cols: zzz.name).

The bound lands where binding_t ends, and nothing narrower. On the same index, with
every name declared so scope is not in play:

16 names → rows: 0  (cols: a0.name)      accepted
17 names → too many node variables: a query can name at most 16 — …

and an ordinary query is untouched: MATCH (a:Function)-[rel:CALLS]->(b:Function) RETURN a.name, b.name LIMIT 3 still answers 3 rows.

Fixes #1995

[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]

This is one of the best-argued PRs I have reviewed in this queue. I checked the load-bearing claims against the source rather than taking them, and every one holds:

• CYP_MAX_VARS = 16 and CYP_MAX_EDGE_VARS are plain enum bounds on binding_t's arrays (cypher.c:25, :2209).
• binding_set and binding_set_edge both return; at capacity — silently, and both are void, which is exactly why threading an error up from them would have been the expensive fix.
• collect_declared_names returns -1 at cap, and check_projection_scope answers NULL with the comment you quote: "too many names to model — stay quiet rather than guess".

And the placement is right: check_pattern_var_capacity runs at the top of check_projection_scope, which the executor calls per UNION arm immediately after parse and before execute_single. So no query reaches execution unchecked, and the bounds check precedes every array write (node_n >= CYP_MAX_VARS before node_vars[node_n++]), so the checker itself cannot overflow.

The core insight is the one worth keeping: the -1 branch's premise was that refusing costs the caller a working query. Once you notice that over-capacity names bind to nothing and project as empty strings, the trade was never "refuse or stay quiet" — it was "refuse or answer wrong". That reframing is what makes bounding the input obviously correct rather than merely convenient, and it retires three other silent drops for free.

Lint is red, and it is yours

src/cypher/cypher.c:5059:37: error: Uninitialized variable: edge_vars [uninitvar]
            if (!var || scope_holds(edge_vars, edge_n, var)) {

Same for node_vars. make lint-ci gates on cppcheck, so this blocks.

In substance it is a false positive — scope_holds(arr, n, var) reads only the first n entries and n starts at 0, so nothing uninitialized is ever read. But the fix is cheap and I would rather have it than a suppression:

const char *node_vars[CYP_MAX_VARS] = {0};
const char *edge_vars[CYP_MAX_EDGE_VARS] = {0};

Worth knowing why the neighbouring const char *declared[CYP_SCOPE_MAX_NAMES]; does not trip the same rule: it is passed straight into collect_declared_names, which writes it before anything reads it, so cppcheck sees the initialization. Yours is read in the same function that fills it, which is the shape the checker cannot follow. House rule here is refactor before suppressing, and = {0} is that refactor — 16 and 8 pointers, no measurable cost.

One place the description claims slightly more than is true

A query naming more variables than a binding can hold is not a working query.

As I read binding_set, names bind in call order and only the overflow is dropped — so a query that declares 20 nodes but projects only names among the first 16 does answer correctly today, and will be refused after this change. Not "answered wrong", just refused.

That does not change my view, because the remedy is already the first thing your error message says — "leave the name off the ones you do not use" — and an unnamed node takes no slot, so the fix is exact and local. But the claim as written is stronger than what the code supports, and I would rather the PR body said "cannot be answered for the names past the bound". If I have misread the ordering, tell me.

Also

You are 3 commits behind main; please rebase so the re-run is against current main.

The three tests are well chosen — anchoring on a label that matches nothing makes them instant and fixture-free, and pinning the reporter's case at both 10 and 35 declared names is what actually demonstrates the bug rather than the symptom. Noting that the wide case is refused for width rather than scope, so the message names the limit instead of zzz, is the kind of detail that saves the next reader an hour.

Fix the lint, rebase, and I am happy to merge this.

[DeusData]

Heads-up on sequencing, and a reminder of the one outstanding item.

Still outstanding here: the cppcheck uninitvar fix — const char *node_vars[CYP_MAX_VARS] = {0}; and the same for edge_vars. lint / lint gates on cppcheck, so this cannot merge until that lands.

And this conflicts with your own #1918. I verified it rather than assuming: merging main + #1918 + this produces CONFLICT (content): Merge conflict in tests/test_cypher.c. The production file auto-merges; the test file does not, because both add cases to the same region.

The two are the same defect family — this refuses a query naming more pattern variables than a binding holds, #1918 refuses a WITH wider than a binding can carry, and #1875 (merged earlier today) refused a query the parser did not fully read. Three PRs on one theme, each correct alone.

I have suggested on #1918 that it goes first, purely because it needs nothing further from you while this one still owes the lint fix. That would leave this PR needing a small, mechanical test-file rebase — additive test cases, not competing logic. If you would rather this one landed first, say so and I will sequence it that way instead.

My review from yesterday otherwise stands unchanged: approved on merit, and the reframing that made it convincing — that the old -1 branch's premise was wrong because over-capacity names bind to nothing and project as blanks, so the real choice was never "refuse or stay quiet" but "refuse or answer wrong".

[CaptainMittens]

The cppcheck fix is in — pushed about an hour ago, so it likely landed after you wrote this. It is the shape you named:

/* Initialized because cppcheck cannot see that scope_holds reads only the
 * node_n / edge_n entries already written, and reports the first call as a
 * read of an uninitialized array. */
const char *node_vars[CYP_MAX_VARS] = {NULL};
const char *edge_vars[CYP_MAX_EDGE_VARS] = {NULL};

I kept the comment because the code was already correct — node_n starts at 0 and scope_holds reads nothing when its count is 0, which cppcheck cannot connect across the call. I chose the initializer over restructuring the fill loop into a helper like collect_declared_names: that form also passes, but only because the analyzer can prove the count is 0 on the first call, and CI builds cppcheck from source rather than using a released build. An initialized array does not depend on that inference.

Locally, make -f Makefile.cbm lint-cppcheck exits 0 across the full source set, and the cypher suite is 192 passed, 0 failed — including the three capacity tests and cypher_wide_return_projection_bounded. CI's lint / lint is still running as I write this. I amended the existing commit rather than adding a fix-up, so this is still one commit, and I rebased onto current main while I was there.

On sequencing: #1918 first is fine, no need to change it. I will take the tests/test_cypher.c rebase here once it lands.

One thing that may help that decision — neither of #1918's reds is its code:

• test-unix (macos-14, cc, c++) failed on Failed to CreateArtifact: Unable to make request: ENOTFOUND while uploading shard-manifest.txt. Every test passed first: 7618 passed, 0 failed. The job failed after the run, on the upload.
• test-windows-guards is the shared setup fault from the ci(coverage): fail a PR when this repo's own parse-coverage report goes bad (#963) #1968 thread.

So #1918 needs a re-run rather than a change from me.

[CaptainMittens]

You did not misread the ordering. I checked binding_set before editing, and it appends in
call order — cypher.c:2562-2577: it scans the existing names, returns early on a rename,
then refuses only once b->var_count >= CYP_MAX_VARS. binding_set_edge has the same
shape at :2519-2534. So the first 16 named nodes bind correctly and only the overflow is
dropped, which means a query declaring 20 nodes and projecting names among the first 16
does answer correctly today and will be refused after this change. Refused, not answered
wrong.

I have rewritten that paragraph in the description. It now says the query "cannot be
answered for the names past the bound", separates the two cases, and states plainly that
this change refuses the correct-today case as well. The "refuse or answer wrong" framing
now applies only to the names past the bound, which is where it is true.

Everything else you asked for is already pushed, as of my earlier comment: the initializers
on both arrays, and the rebase onto current main. lint / lint has since gone green on
that head, so the gate you named is clear.

Still happy to take the tests/test_cypher.c rebase here after #1918 lands.