Skip to content

Track token consumption in the graph DSL - #328

Open
EnRaiha wants to merge 2 commits into
mainfrom
fix/issue296-graph-cursor
Open

EnRaiha wants to merge 2 commits into
mainfrom
fix/issue296-graph-cursor

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

The graph DSL clause readers were independent forward scans over a flat token list. A mistyped clause keyword (DEPTS for DEPTH) left its value unread, the clause kept its default, and the statement answered a different question than it asked — with no error anywhere. An audit of the parser showed the same silent-default class across every multi-word clause, and for GRAPH RAG FUSION the exemption was explicit: the variant called consume_rest(), claiming every remaining token so the dispatcher's unclaimed-token check could never fire.

This PR makes token consumption explicit. Every clause reads through a claim-tracking cursor; a token no clause claims is refused by name; a clause that is present but unreadable is an error, never a default.

Behaviour changes

Path Before After
GRAPH TRAVERSE … DEPTS 3 (mistyped clause keyword) the depth defaulted; DEPTS 3 went unread 42601, the error names DEPTS
GRAPH RAG FUSION … VECTOR_TOPK 5 ran with the default cap 42601, names VECTOR_TOPK
… VECTOR_TOP_K five silent default 42601, names VECTOR_TOP_K
RRF_K (60.0) or RRF_K (1,2,3,4) silently ignored, or the first values used 42601; two or three numbers required
BM25 'text' ON 'field' in the DSL the collection name was read as the field field
SEARCH c USING FUSION(… VECTOR_TOPK 5) the unclaimed token was ignored 42601, names the token

Closes #296.

Root cause

  • Clause readers were free functions over &[Tok] (quoted_after, word_after, usize_after, float_pair_after, float_triple_after, array_floats_after). Each searched forward for its keyword and ignored everything between keywords.
  • The dispatcher gained an unclaimed-token check (Cursor::finish), but GRAPH RAG FUSION bypassed it with consume_rest() because its parameter extractor read the ARRAY[…] payload from the raw statement text and therefore claimed no tokens.
  • FusionParams::extract validated nothing: an absent or misspelled keyword produced None for that parameter and the executor applied its default. The extractor's own doc comment claimed it refused malformed options; it did not.
  • floats_after (originally added for the fusion RRF_K pair) had no library caller at all — tests only.

What changed

Commit 62cf9dd34 — the cursor and the per-variant rewiring:

  • graph_parse/cursor.rs (new): one claim-tracking reader per token shape — claim_text, find, word_after, quoted_after, quoted_after_from, object_after, usize_after, usize_after_checked, float_after, direction_after, and finish, which refuses the first token no clause owns, naming it.
  • Every variant (parse_traverse, parse_path, parse_algo, parse_rag_fusion, parse_insert_vertex/parse_insert_edge, …) reads through the cursor. MATCH statements are untouched.
  • entry.rs runs the dispatcher and finish with the statement label, so a refusal names the statement and the token.

Commit 94214abad — the fusion rework:

  • FusionParams::extract takes &mut Cursor and reads every option through it: keywords and values are claimed, so a mistyped option keyword is refused by name instead of defaulting.
  • A present-but-unreadable value is an error: usize_after_checked refuses VECTOR_TOP_K five; floats_after_max refuses RRF_K fast and accepts exactly two or three numbers (RRF_K (60.0), RRF_K (1,2,3,4) refused).
  • The QUERY ARRAY[…] payload is claimed element by element from the same token stream (Cursor::floats_array_after claims the anchor, the ARRAY word, and the numeric run). No raw-text reader remains; helpers.rs keeps only missing_clause.
  • consume_rest() is deleted. The only exemption in the parser is gone.
  • The BM25 field's ON is read with quoted_after_from after the BM25 anchor: ON also introduces the collection, and the first match was the collection name, not the field's. This was a latent mis-parse the new wire test surfaced.
  • The floats_after dead-code item is resolved by deletion, replaced by floats_after_max (its test rewritten): the accessor now has a real library caller.
  • parse_search_using_fusion returns Result<Option<…>> and runs finish on its own cursor, so the wrapped SEARCH … USING FUSION(…) surface validates with the same extractor. The one caller maps the error to 42601.

Regression proof

On base main, without this change, four wire tests fail: a mistyped clause keyword, a stray literal, a mistyped fusion option keyword, and an unreadable option value. With this change all four pass, and the neighbouring DSL and fusion modules pass unchanged.

Tested

  • cargo nextest run -p nodedb-sql --lib899 passed (includes the new cursor unit tests, the fusion-parameter unit tests, and the DSL dispatcher tests in entry.rs)
  • cargo nextest run -p nodedb --test wire -E 'test(cases::graph_rag_fusion) | test(cases::sql_three_source_rrf) | test(cases::graph_dsl_handlers)'36 passed
  • combined re-run after the rework: 49 passed
  • cargo fmt --all -- --check — clean
  • cargo clippy -p nodedb-sql -p nodedb --lib -- -D warnings — clean
  • preflight (repo hooks: commit hygiene, narration, dead code, file size, test registry) — pass

Review

A separate, read-only parity audit ran over this branch before submission, covering test-inventory parity, caller closure, producer/consumer enumeration, invariant chokepoints, module contracts, and repository norms. All items passed with no blockers. The items it could not execute read-only are covered here: the fail-before evidence is in Regression proof, the full test count is measured by CI, and cross-crate consumers are covered by the workspace build.

Exclusions

  • MATCH statements are deliberately untouched (their parser was not part of the defect).
  • The maintainer-owned paths from the same batch are not touched.
  • No refactor rides along: the dead raw-text readers are deleted because the fix replaces them, and floats_after is removed because it had no caller.

Commits

  • 62cf9dd34fix(sql): track token consumption in the graph DSL
  • 94214abadfix(sql): read fusion options through a claim-tracking cursor

Copilot AI lite review requested due to automatic review settings September 16, 2026 20:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 16, 2026
@EnRaiha
EnRaiha force-pushed the fix/issue296-graph-cursor branch from befb0de to 94214ab Compare September 17, 2026 02:16
@EnRaiha EnRaiha removed the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 17, 2026
A mistyped clause keyword (`DEPTS` for `DEPTH`) defaulted the clause and
left its value unread, so the statement answered a different question than
it asked, with no error. The clause readers were independent forward scans
that ignored every token between a keyword and its value.

A cursor now claims the tokens each clause consumes, and the dispatcher
refuses the first token no clause claimed, naming it. All statement
variants are covered; `MATCH` statements are untouched.
Fusion parameters were read from a raw token slice and the variant then
claimed every remaining token, so a mistyped option keyword ran the
statement with a default and no error. The shared extractor now reads
through the cursor: each keyword and value is claimed, the ARRAY[...]
payload is claimed element by element, and a token no clause owns is
refused by name. A present-but-unreadable value is an error, never a
default.

- drop the raw-text readers; the ARRAY payload and the RRF_K count come
  from the same token stream
- the BM25 field's ON is read after the BM25 anchor: ON also introduces
  the collection, and the first match is not the field's
- the wrapped SEARCH ... USING FUSION(...) surface validates with the
  same extractor and refuses unclaimed tokens
@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 17, 2026
@EnRaiha
EnRaiha force-pushed the fix/issue296-graph-cursor branch from 94214ab to ddf5394 Compare September 17, 2026 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci Opt this PR into the full test suite; re-add to force a re-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Graph DSL ignores unconsumed tokens — a typo'd clause keyword silently runs the statement with default semantics

2 participants