Skip to content

fix(cypher): refuse a name that is not in scope instead of projecting a blank column - #1922

Merged
DeusData merged 1 commit into
DeusData:mainfrom
CaptainMittens:fix/reject-out-of-scope-projection
Aug 31, 2026
Merged

fix(cypher): refuse a name that is not in scope instead of projecting a blank column#1922
DeusData merged 1 commit into
DeusData:mainfrom
CaptainMittens:fix/reject-out-of-scope-projection

Conversation

@CaptainMittens

Copy link
Copy Markdown
Contributor

Fixes #1919.

What was wrong

A RETURN could name a variable the WITH before it dropped. The query ran,
exited clean, printed a column for that name, and filled it with empty strings.
Nothing reported an error.

MATCH (f:Function)-[:CALLS]->(g) WITH f.name AS caller RETURN caller, g.name

g does not survive the WITH. Measured through query_graph on a real
indexed project before the fix:

rows: 5  (cols: caller g.name)
  appWith          -
  send             -
  appWithNoPartner -

The silence is the harm. A reader — a person or an agent — sees a column of
nothing and reads it as "the graph holds no such data". The true answer is
"your query named something that is out of scope". For a code graph, a false
"nothing calls X" is the worst answer it can give.

This is the same failure shape as #373, where an unknown function used to
project a blank column and now fails loudly. Same answer here.

The fix

One check on the parsed query, between parse and execute. Every name a RETURN
or WITH item uses must be a variable some pattern declares, or an alias the
previous WITH made.

The check reads the parsed query, not the run-time bindings, and that split is
the whole point.
It asks whether the query DECLARED the name — a different
question from whether a row happened to bind it. So the existing convention at
cypher.c:3345 and :3350 is untouched: an OPTIONAL MATCH target that
matched nothing is still declared, still legal, and still projects "". One of
the three tests below holds that line.

Two placeholders are skipped, because neither names anything the query
declared: count(*) stores "*" (cypher.c:1614) and a CASE expression
stores "CASE" (cypher.c:1643). Both were found by the existing suite —
skipping them turned 7 red tests green.

A query with more names than the guard can model skips the check rather than
guessing. A wrong refusal costs the caller a working query, which is worse than
the silence this removes.

The error names the offending variable and the clause, because a message that
does not say WHICH name is wrong sends the reader back to guessing:

variable 'g' is not in scope for RETURN — the WITH clause did not carry it through

Tests

Three added, red before the change:

Test What it holds
cypher_rejects_projection_of_dropped_with_var_issue1919 the fault — must error and name g
cypher_optional_match_target_still_allowed_issue1919 an unmatched OPTIONAL MATCH target must still project ""
cypher_with_alias_stays_in_scope_issue1919 a WITH alias, and a variable carried through whole, still work

Red-green, both runs quoted:

before:  cypher_rejects_projection_of_dropped_with_var_issue1919  FAIL  ASSERT(rc != 0)
         185 passed, 1 failed
after:   cypher_rejects_projection_of_dropped_with_var_issue1919  PASS
         186 passed

The two guard tests passed before the change as well as after, which is what
makes them useful — they were there to catch a fix that over-rejects, and they
did: a first attempt broke 7 existing tests before the placeholder skip.

Gates run locally

Both through the entry points CONTRIBUTING.md names, on macOS arm64:

Gate Result
scripts/lint.sh --ci exit 0 — "All linters passed"
scripts/test.sh --suites cypher exit 0 — 186 passed, 0 failed

The full suite was not run locally. CI covers it.

Scope

One issue, one fix. This does not touch #1918, which fixes a neighbouring fault
in RETURN * and can land in either order — the two change different
functions.

Names inside a CASE expression and inside a multi-argument function's
arguments are still unchecked. Widening the check there is a separate change
with its own over-rejection risk.

… a blank

A RETURN could name a variable the WITH before it dropped. The query ran,
exited clean, printed a column for that name, and filled it with empty
strings. Nothing said anything was wrong.

That silence is the harm. A reader sees a column of nothing and reads it as
"the graph holds no such data". The true answer is "your query named
something that is out of scope". For a code graph, a false "nothing calls X"
is the worst answer it can give.

The check runs on the parsed query rather than on the run-time bindings, and
that split is the whole point. It asks whether the query DECLARED the name,
which is a different question from whether a row happened to bind it. So the
existing convention is untouched: an OPTIONAL MATCH target that matched
nothing is still declared, still legal, and still projects "".

Two placeholders are skipped, because neither names anything the query
declared: count(*) stores "*", and a CASE expression stores "CASE".

A query with more names than the guard can model skips the check rather than
guessing. A wrong refusal costs the caller a working query, which is worse
than the silence this removes.

Same failure shape as DeusData#373, and the same answer: say so out loud.

Fixes DeusData#1919

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.

@DeusData
DeusData merged commit a6468ed into DeusData:main Aug 31, 2026
35 checks passed
@DeusData

Copy link
Copy Markdown
Owner

Merged as a6468ed4.

The framing in your description is the reason this went in quickly, and it is worth repeating back because it is the right standard for this project: "The silence is the harm." A blank column reads as "the graph holds no such data" when the truth is "your query named something that is out of scope" — and for a code graph a false nothing calls X is the worst answer it can give. That is a graph-quality argument, not a validation nicety, and it is why this is a fix rather than a feature.

Two things made it reviewable without a round trip:

You drew the line the fix could have blurred, and then tested it. Checking the parsed query rather than the run-time bindings is the whole distinction — "did the query declare this name" is a different question from "did a row happen to bind it". That is what keeps an OPTIONAL MATCH target that matched nothing legal and still projecting "", and you have cypher_optional_match_target_still_allowed_issue1919 holding that line explicitly rather than leaving it to be discovered later.

Two negative controls, not just the reproduction. The OPTIONAL MATCH case and the WITH-alias case mean the fix cannot pass by over-rejecting. A guard like this fails in the over-eager direction far more often than the under-eager one, and both controls are registered in a suite that actually runs.

Citing #373 as the same failure shape was also the right move — it made the precedent do the arguing instead of the description.

Thank you. 35 checks green, no round trips needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

query_graph: a variable the WITH clause dropped is still accepted, and its column comes back empty with no error

2 participants