ci: block PRs that add O(store) SPARQL query shapes - #1745
Conversation
Adds a SPARQL scalability lint that scans PR-changed source for query
shapes behind past production incidents and fails the check when a PR
ADDS one:
R1 unscoped-all-var-scan whole-store ?s ?p ?o scans
R2 graph-var-scan all-var triple inside GRAPH ?var — the
#1597 listGraphs-storm shape
R3 offset-pagination O(offset)-per-page walks + torn reads
R4 bucket-graph-scan unbounded scans over growing graph
families (_shared_memory/_meta/data
graph/_catalog) — the #1609 shape
Deliberate exemptions keep the blessed idioms green: FILTER EXISTS
existence probes (the FIXED #1597 listGraphs form), plain ASK,
LIMIT-without-ORDER-BY, VALUES-bound graph vars, exact per-KA graph
reads, and CONSTRUCT/INSERT/DELETE output templates.
Ratchet semantics: findings are fingerprinted by (rule, normalized
query text); pre-existing debt is grandfathered as notices (current
baseline ~52 findings) and only NEW findings block. Provably bounded
queries can be acknowledged in code with
'sparql-scan-allow: <rule> -- <justification>', making every allowed
scan a reviewed, diffable decision.
The scanner is zero-dependency (no pnpm install in the job), extracts
SPARQL from TS template literals (regex-literal-aware tokenizer;
interpolations normalized), and self-tests against 20 fixtures —
including the exact #1597 bad/fixed pair — before every scan.
The workflow runs on every pull_request (no branch filter, so stacked
PRs whose base is a feature branch are covered) and on merge_group so
the merge queue cannot stall once the check is marked required.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| for (const file of files) { | ||
| const baseSource = fileAt(baseSha, file); | ||
| if (!baseSource) continue; | ||
| for (const f of scanSource(file, baseSource)) baseline.add(f.fingerprint); |
There was a problem hiding this comment.
🔴 Bug: Duplicate copies of a grandfathered query are never treated as new
What's wrong
The ratchet promises to block PRs that add offending SPARQL shapes, but fingerprint membership alone cannot distinguish an unchanged pre-existing query from an added duplicate with the same normalized text. A developer can copy an existing grandfathered scan in the same changed file and the gate will mark every copy as pre-existing.
Example
Base file: one SELECT ?s WHERE { ?s ?p ?o }. Head file: the same query plus a second identical copy. scanSource returns two head findings with the same fingerprint, and both compare as present in the baseline, so blocking stays 0 even though the PR added a new store-wide scan.
Suggested direction
Use a multiset/count map for baseline fingerprints and decrement it as matching head findings are consumed; any remaining matching head occurrences should be considered new.
For Agents
In runDiff, preserve multiplicity for baseline findings, not just membership. Compare head findings against a counter keyed by fingerprint so only the first matching pre-existing occurrence is grandfathered; added duplicates should be reported as new. Add a regression that duplicates an existing offending query in a touched file and expects one blocking finding.
There was a problem hiding this comment.
Fixed in 330cd41. The baseline is now a multiset (fingerprint → count) and every non-acknowledged head finding consumes one occurrence; the surplus blocks as new. The diff core is extracted as computeDiffFindings() so the gate itself is testable.
Test evidence: the new diff-gate integration self-test (throwaway git repo, runs inside --self-test and therefore before every CI scan) covers your exact scenario — base has one SELECT ?s WHERE { ?s ?p ?o }, head adds an identical copy → exactly 1 new blocking + 1 grandfathered. Reindent-only and genuinely-new-shape cases pinned alongside. 27/27 fixtures + diff-gate pass.
There was a problem hiding this comment.
🟡 Issue: Moved grandfathered queries are treated as new findings
What's wrong
The ratchet promises that moving an existing offending query does not re-flag it, but the implementation only baselines the base version of each current changed path. Renames and moves therefore lose their baseline occurrence and can fail unrelated refactors.
Example
If src/a.ts already contains a grandfathered SELECT ?s WHERE { ?s ?p ?o } and a PR only runs git mv src/a.ts src/b.ts, the base lookup for src/b.ts returns null, so the unchanged query in src/b.ts is reported as new. Expected behavior per the ratchet docs is grandfathered, while copying without deleting should still block.
Suggested direction
Include deleted/renamed old-path findings in the baseline, while preserving multiset semantics so true duplicates still block.
For Agents
Update changedSourceFiles/computeDiffFindings to account for old paths from git diff --name-status -M or compare base/head multisets in a way that consumes removed occurrences. Add a diff-gate self-test for git mv of a grandfathered query and keep the existing duplicate-copy blocking test.
| const stripped = stripLiteralsAndComments(queryText); | ||
| const findings = []; | ||
|
|
||
| const hasLimit = /\bLIMIT\s+(\d+|⟪\d+⟫)/i.test(stripped); |
There was a problem hiding this comment.
🔴 Bug: A LIMIT inside a subquery suppresses findings for unrelated outer scans
What's wrong
The exemption is global to the template literal, not scoped to the suspicious graph pattern. Any nested or unrelated LIMIT can make boundedNoSort true and suppress R1/R2 even when the offending triple is in an unbounded outer query.
Example
SELECT ?s WHERE { ?s ?p ?o . { SELECT ?x WHERE { <urn:a> <urn:b> ?x } LIMIT 1 } } has no top-level limit on the outer store-wide scan, but the scanner reports no R1 because it sees the subquery LIMIT 1.
Suggested direction
Track query/group nesting and apply the LIMIT without ORDER BY exemption only when it actually bounds the scan being reported.
For Agents
Scope LIMIT detection to the group/query level that contains the suspicious triple, or at least only honor a top-level solution modifier for the enclosing query. Add a regression where an unrelated limited subquery appears beside an unbounded all-variable triple and the outer R1 still fires.
Nested LIMIT behavior is not locked by a self-test fixture
What's wrong
The LIMIT exemption is high-risk because it can turn a blocking full-store scan into no finding. The tests cover only the simple top-level LIMIT case, while the scanner uses a query-wide hasLimit; that leaves the nested-LIMIT edge called out in validation notes without durable automated coverage.
Example
A fixture like const q = SELECT ?s WHERE { { SELECT ?x WHERE { GRAPH urn:g { ?x urn:p ?o } } LIMIT 1 } ?s ?p ?o }; should still expect R1, because the outer ?s ?p ?o is not bounded by the inner subquery LIMIT.
Suggested direction
Promote the manually checked nested-LIMIT scenario into the fixture suite so future scanner edits cannot silently broaden the LIMIT exemption.
Confidence note
This is based on the new fixture suite and analyzeQuery shape in the diff; I did not execute a synthetic nested-LIMIT case in this read-only review environment.
For Agents
Add scanner fixtures for nested/subquery LIMIT behavior: one where the LIMIT legitimately bounds the flagged pattern, and one where a LIMIT in another group must not suppress an outer R1/R2 finding. Keep the assertions behavior-oriented, not tied to parser internals.
There was a problem hiding this comment.
Fixed in 330cd41. The exemption now honors only a top-level LIMIT (brace-depth 0, still requiring no top-level ORDER BY); a subquery LIMIT no longer suppresses R1/R2/R4 for outer scans. Your example (?s ?p ?o beside a LIMIT 1 subquery) is a pinned fixture and fires R1.
On the second half: per-group LIMIT binding is deliberately not modeled — a subquery-local LIMIT does not exempt even its own group's all-variable scan. That's the fail-closed direction (a false block is one pragma away; a false pass is an incident), and it's pinned by its own fixture with a comment stating the policy, plus the legit top-level-LIMIT-exempts case, so future edits can't silently broaden the exemption. Documented in docs/sparql-scale-lint.md.
There was a problem hiding this comment.
🟡 Issue: VALUES-bound graph exemption is under-tested
What's wrong
The scanner claims GRAPH ?g is exempt when ?g is bound by VALUES, but the self-test only verifies the simplest bare-variable form. That gives false confidence for a real SPARQL syntax variant: parenthesized VALUES still binds the graph variable, but the current tests would stay green if it is flagged as an unbounded graph scan.
Example
Add a fixture like: const q = SELECT ?s WHERE { VALUES (?g) { (urn:a) } GRAPH ?g { ?s ?p ?o } }; with expect: []. It currently reports ['R2'].
Suggested direction
Cover SPARQL's parenthesized VALUES syntax in the self-test suite before relying on the exemption, especially since production code in this repo already emits parenthesized/multi-variable VALUES clauses.
Confidence note
Based on a targeted scanSource probe. I did not run the full --self-test because this sandbox is read-only and the integration self-test writes under /tmp.
For Agents
Look at FIXTURES around the R2 VALUES exemption and analyzeQuery's valuesBound extraction. Add regression coverage for parenthesized single-variable and/or multi-variable VALUES forms that bind ?g, then adjust the scanner while preserving the existing bare VALUES ?g behavior.
There was a problem hiding this comment.
🔴 Bug: R4 can be bypassed with ORDER BY plus LIMIT
What's wrong
The new lint is meant to block unbounded scans over growing graph families, but any top-level LIMIT suppresses R4. A sorted LIMIT still has to consider the unbounded graph in common SPARQL execution plans, so this lets a known store-melting shape pass CI.
Example
A new query like SELECT ?s WHERE { GRAPH <${metaGraph}> { ?s ?p ?o } } ORDER BY ?s LIMIT 100 is accepted. Because the ORDER BY applies before LIMIT, the store may still scan/sort the whole unbounded meta graph; this is the same materialization risk the script already blocks for R1/R2.
Suggested direction
Use the same boundedNoSort distinction for R4, or otherwise require evidence of keyset/bound-term pagination before suppressing bucket-graph scans.
For Agents
Look at analyzeQuery R4 handling. Preserve the safe exemption for truly bounded unsorted LIMITs if intended, but make R4 treat ORDER BY ... LIMIT on unbounded graph families as blocking unless there is a safer keyset/bound-term shape or a pragma. Add a fixture mirroring the R1 ORDER BY LIMIT case for R4.
| */ | ||
| const REGEX_PRECEDING_KEYWORD = /(^|[^\w$])(return|typeof|instanceof|in|of|new|delete|void|do|else|case|yield|await)\s*$/; | ||
|
|
||
| export function extractTemplateLiterals(source) { |
There was a problem hiding this comment.
🟡 Issue: Split the scanner instead of landing a monolithic parser, CLI, and test harness
What's wrong
This lands a critical CI gate as one large standalone script with several distinct layers packed together. The file is still under 1k lines, but the structure makes maintenance costly because rule policy, parsing mechanics, reporting, Git plumbing, and test data all change in the same place.
Example
A future R5 rule would need changes across the rule table, analyzeQuery, report output, embedded fixtures, and docs from inside the same production CLI file. No behavior bug is claimed; the problem is the amount of unrelated responsibility in one file.
Suggested direction
Use the repo's existing scripts/lib plus scripts/lib/tests pattern so parser/rule code, CLI orchestration, and fixtures have clear boundaries. That keeps the CI entry point small while making the rule engine easier to review and extend.
For Agents
Move the reusable scanner into scripts/lib/sparql-scale-lint/ or a single scripts/lib/sparql-scale-lint.mjs module, keep scripts/sparql-scale-lint.mjs as a thin CLI, and move fixtures into scripts/lib/tests/sparql-scale-lint.test.mjs using node:test. Preserve the zero-dependency workflow and keep the CLI self-test behavior if desired.
There was a problem hiding this comment.
Deferring the split, deliberately: this script is a CI gate, and the property I want to preserve is that its entire policy — extraction, rules, ratchet, fixtures — is auditable at one file/one SHA, the same reason ci.yml's plan job pins its trusted controller by commit. A multi-module layout would also mean the fixtures only run when a separate test lane runs them; here --self-test executes before every scan, so the gate can't drift from its spec. The one seam that mattered for testability (the diff core) is now an exported function. If a real R5 lands and the file crosses ~1k lines, splitting into scripts/lib/sparql-scale-lint/ per your layout is the right follow-up — happy to note that as the trigger condition.
There was a problem hiding this comment.
🟡 Issue: Avoid making the SPARQL lint own a custom JavaScript parser
What's wrong
This is a large amount of bespoke parsing for a problem the codebase can treat as an input-extraction step. It makes the scanner harder to reason about because maintainers must validate both JavaScript lexical behavior and SPARQL rule behavior in the same function, and every new source-shape exception expands this state machine rather than the actual lint model.
Example
A future maintainer adding support for a normal JavaScript syntax corner has to modify this linter's hand-written source walker, even though the linter's actual domain is SPARQL query-shape analysis.
Suggested direction
Push the JavaScript extraction behind a dedicated boundary, ideally using a real JS/TS parser rather than maintaining regex/division and nested-template heuristics inline. That would let the rest of the file stay focused on SPARQL shapes instead of incidental JavaScript lexing.
For Agents
Look at scripts/sparql-scale-lint.mjs around extractTemplateLiterals. Preserve the current template-literal scan behavior and line reporting, but replace or isolate the JavaScript-source parsing boundary. Prefer an AST-backed extractor using an existing parser if CI can install it, or a very small standalone source-extraction module with focused fixtures if zero-dependency CI remains mandatory.
There was a problem hiding this comment.
🟡 Issue: Move the self-test harness out of the production CLI
What's wrong
Embedding the full fixture corpus and temporary-repo integration harness inside the executable script makes the file sprawl and couples unrelated reasons to edit the same module. This is exactly the kind of growth that will make the linter harder to maintain as more rules and fixtures are added.
Example
A reviewer trying to change report formatting or diff-gate orchestration has to scroll through hundreds of inline fixtures and self-test setup in the same file, even though those are separate concerns.
Suggested direction
Keep the ratchet self-test, but make it a separate test file that imports the scanner and diff-gate core. The CLI should mostly parse args and call the exported operations.
For Agents
Split scripts/sparql-scale-lint.mjs into a small CLI plus exported scanner/diff-gate modules, and move FIXTURES/diffGateSelfTest into a separate scripts/sparql-scale-lint.test.mjs or similar zero-dependency node test. Update the workflow to run the test command before the scan so the current self-test-before-scan behavior is preserved.
There was a problem hiding this comment.
🟡 Issue: Decompose the lint script before it becomes a monolith
What's wrong
This PR lands a near-1k-line script whose responsibilities are tightly packed into one file. Even though it is still under the hard 1000-line threshold, it already has multiple independently complex domains in the same module. That creates maintainability risk: rule evolution, CLI behavior, baseline semantics, and self-test mechanics are now coupled by file-level state and broad helper visibility instead of clear ownership boundaries.
Example
A future R2/R4 rule change has to edit the same large executable that also owns regex-vs-division tokenization, git plumbing, GitHub annotation formatting, and throwaway-repo tests, so reviewers must re-load several unrelated concepts before they can validate a local policy change.
Suggested direction
Keep the executable as orchestration only and move the scanner, diff gate, and fixture harness behind small explicit module boundaries. That would make the rule policy easier to review without dragging the JS lexer and CI reporting code into every change.
For Agents
Split scripts/sparql-scale-lint.mjs into focused modules while preserving behavior: a scanner/analyzer module, a ratchet/diff module, a CLI/reporting wrapper, and a self-test/fixture module invoked by --self-test and CI. Prove the split by running the current --self-test and one representative --diff invocation against a temporary repo.
There was a problem hiding this comment.
🟡 Issue: Keep the self-test harness out of the production scanner module
What's wrong
The file combines production scanning, reporting, CLI dispatch, unit fixtures, and an integration test harness. That turns a focused linter into a 774-line multipurpose module and makes unrelated concerns change together. The test harness is valuable, but embedding it here is avoidable structural weight.
Example
The repository already keeps comparable audit fixtures in separate files, such as scripts/audit-dial-protocol.test.mjs, while the production audit script exports the scanner surface. This new script instead ships the fixture table and temp-repo integration harness in the executable path.
Suggested direction
Separate the scanner/CLI from its test fixture corpus. That preserves the self-test behavior in CI while making the runtime script smaller, easier to scan, and free of temp-repo test orchestration.
For Agents
Move fixture and diff-gate tests into scripts/sparql-scale-lint.test.mjs or a nearby test module. Export scanSource/computeDiffFindings; have the workflow run the test before --diff. If the local CLI must keep --self-test, dynamically import the test module only for that mode.
There was a problem hiding this comment.
🟡 Issue: The scanner, CLI, and self-test harness are all collapsed into one large script
What's wrong
This PR introduces a new 830-line script where production logic and test-only orchestration are interleaved. That makes the code harder to review and evolve, and it diverges from the repo's existing pattern of keeping script tests in separate .test.mjs files. The self-test requirement is valid, but it does not require embedding all fixtures and temporary-git-repo orchestration in the same production module.
Example
A future change to the scanner now has to edit one 830-line file containing rule metadata, parser/walker logic, git diff orchestration, CLI handling, fixture data, integration-test repo setup, and spawned-CLI assertions. Even reviewing a rule tweak means scanning through the test harness and vice versa.
Suggested direction
Decompose the file before this becomes the permanent home for every future rule and fixture. The clean split is scanner core, diff/CLI orchestration, and tests/self-test fixtures.
For Agents
Split this into focused modules while preserving current behavior: keep the CLI thin, move extraction/analysis/diff computation into a library module, and move FIXTURES/diffGateSelfTest into a node:test file or dedicated self-test module. Update the workflow to run the self-test before the scan, and keep assertions proving duplicate-copy blocking, reindent grandfathering, TSX scanning, and CLI scan behavior.
There was a problem hiding this comment.
🟡 Issue: Decompose the new lint script before it lands over 1k lines
What's wrong
This lands a large, multi-responsibility script as a single new module. The size is not just fixture bulk: production analysis, policy, diff orchestration, reporting, and test harness code are all coupled by shared local helpers. That makes the scanner harder to evolve safely and violates the repo-health bar for files crossing 1,000 lines.
Example
A future rule change now has to touch or at least mentally load the scanner, output formatting, git diff baseline logic, runtime fixtures, spawned CLI tests, and docs in one file. The file already exceeds the 1k-line decomposition threshold on introduction.
Suggested direction
Move the fixtures and diff-gate self-test out of the production scanner module first, then separate the scanner core from the git/CLI driver. That would make each layer easier to reason about without changing behavior.
For Agents
Split scripts/sparql-scale-lint.mjs into focused modules while preserving CLI behavior: e.g. scanner/extraction + query analysis, diff ratchet/reporting, CLI entrypoint, and self-test fixtures/harness. Keep the public commands unchanged and prove parity with the existing --self-test plus one --diff smoke test.
| * enclosing context: default graph, GRAPH <bound>, or GRAPH ?var — while | ||
| * tracking FILTER [NOT] EXISTS / MINUS probe scopes. | ||
| */ | ||
| function analyzeQuery(queryText, exprs) { |
There was a problem hiding this comment.
🟡 Issue: Separate query-shape collection from rule policy
What's wrong
The analyzer currently mixes scanning mechanics and lint policy in a single state machine. That makes the implementation feel brittle: adding or changing a rule means threading new state through a busy parser instead of extending a rule table or pure predicate.
Example
Inside one flow, boundedNoSort and isPlainAsk gate R1/R2, valuesBound changes graph variable semantics, and R4 checks UNBOUNDED_GRAPH_EXPR against source expression text. Those are separate concepts, but a reader must reason about all of them together before changing any rule.
Suggested direction
Introduce a small intermediate model for candidate triple patterns and query modifiers, then let each rule decide whether it matches. That would remove much of the conditional coupling from analyzeQuery and make future rule changes local.
For Agents
In scripts/sparql-scale-lint.mjs, split analyzeQuery into a collector that emits explicit pattern records such as { offset, graphScope, graphRef, inProbe, inTemplate, modifiers }, then implement R1-R4 as independent rule predicates over those records. Preserve current fixture behavior with the existing cases before adding new ones.
There was a problem hiding this comment.
Agreed in principle — a collector emitting { offset, graphScope, inProbe, inTemplate, modifiers } records with R1–R4 as pure predicates is the cleaner shape. Deferring it from this PR to keep the review-fix diff small and behavior-focused: the four correctness fixes just landed (330cd41) are individually pinned by fixtures, and refactoring the analyzer in the same commit would make it hard to see that behavior didn't move. With 27 fixtures + the diff-gate test now locking the semantics, the record/predicate refactor becomes a safe mechanical follow-up — noted as the first change if/when an R5 rule is added (same trigger as the file-split thread).
There was a problem hiding this comment.
🟡 Issue: Separate SPARQL walking from rule policy
What's wrong
The current function is doing too many jobs at once, so rule behavior is encoded as incidental control flow in the parser. That makes future rule changes risky and makes the intended invariants harder to see than they need to be.
Example
Changing the LIMIT policy for one rule currently requires reasoning through topLevelLimit, boundedNoSort, isPlainAsk, graphFrame selection, and R4's separate hasLimit branch all inside the same walker.
Suggested direction
Introduce a small intermediate model for candidates/contexts and move each rule into its own predicate. That would make the implementation smaller in concept count: one pass to understand query shape, one pass to apply rule policy.
For Agents
In scripts/sparql-scale-lint.mjs, keep the same findings for the existing fixture corpus. Refactor analyzeQuery into two phases: first produce normalized scan candidates such as {triple, context, offset, modifiers}; then apply R1/R2/R3/R4 as small independent rule functions. The existing fixtures should prove behavior is preserved.
There was a problem hiding this comment.
🟡 Issue: Separate query walking from rule policy
What's wrong
The analyzer currently interleaves parsing state, context classification, query-level exemptions, and all rule decisions in one mutable function. This is exactly the kind of spaghetti growth that will get worse as the lint policy evolves: every new rule or exemption has to be added inside the same closure and share the same pending-statement state.
Example
Adding another exemption such as a new bounded graph family or another probe-like construct would require threading it through the shared mutable walker and checkPending side effects instead of adding a small rule against a stable intermediate representation.
Suggested direction
Have one pass produce simple scan facts, then let each rule consume those facts independently. That would remove most of the shared branching from checkPending and make new rules or exemptions local instead of adding more conditions to the central walk.
Confidence note
This is a maintainability concern from the diff alone; the exact scanner behavior may be intentional for the first rule set, but the current shape makes future rule ownership harder to keep clean.
For Agents
Introduce a small intermediate representation from the SPARQL walk, such as normalized triple-pattern events with context {graphKind, graphRef, inProbe, inTemplate} plus query-level modifiers. Then express R1-R4 as separate pure rule functions over those events. Preserve current fixture expectations, especially pragma and diff-gate behavior.
There was a problem hiding this comment.
🟡 Issue: Split the parser from the rule policy before this grows further
What's wrong
The core analyzer is doing too many jobs at once. The mutable pending buffer and stack are also the rule engine, so implementation details of the walker leak directly into policy decisions. For a CI gate that will accumulate new incident shapes over time, that coupling makes maintenance brittle and raises the cost of safely extending the lint.
Example
Adding a fifth scan rule or one more SPARQL construct means editing the same pending/stack walker, the statement-trimming regex, and possibly a second top-level regex pass. That makes each policy change depend on incidental parser state rather than a small explicit query model.
Suggested direction
Use a two-phase structure: first tokenize/walk the query into explicit scan candidates with scope metadata, then run independent rule functions. This would delete much of the cross-coupling inside checkPending and make future rules local changes.
For Agents
Refactor scripts/sparql-scale-lint.mjs so parsing produces a small intermediate model, e.g. top-level modifiers, VALUES-bound vars, graph/probe/template scoped triple patterns, and offsets. Preserve the existing fixture outcomes, then express R1-R4 as separate predicates over that model.
There was a problem hiding this comment.
🟡 Issue: The SPARQL analyzer mixes parsing state, context modeling, and rule emission in one mutable flow
What's wrong
The central analyzer is doing too many jobs at once. It tracks brace depth, detects headers, strips trailing clauses, maintains graph/probe/template stack frames, applies LIMIT/ASK exemptions, and emits multiple rule types from shared mutable state. That makes the implementation fragile to extend: future rules will add more special cases into an already dense flow instead of composing with a clear scanner model.
Example
Adding one more exempt scope or rule would likely touch tokenRe, graphHeader/probeHeader, the trailing-clause strip in checkPending, stack-frame creation, and rule emission. That is a sign the model is missing a layer: syntax/context discovery and rule evaluation are not separated.
Suggested direction
Introduce a minimal internal model for query contexts/triple-pattern events so rules become small predicates instead of branches inside the scanner loop.
Confidence note
The current behavior may be intentionally fail-closed, but the maintainability problem is visible from the structure: each new SPARQL shape or exemption has to be threaded through the same mutable walker.
For Agents
Refactor analyzeQuery into a small pipeline: tokenize stripped query text, build or stream explicit group/context events, then run rule predicates over normalized triple-pattern events. Preserve the current fixtures exactly, especially LIMIT/ORDER BY, probe scope, template scope, VALUES-bound graph variables, and R4 graph-family behavior.
There was a problem hiding this comment.
🟡 Issue: Collapse the duplicate SPARQL group walkers into one state model
What's wrong
The implementation models SPARQL scope in two independent passes: one pass indexes VALUES bindings by brace offsets, and a later pass classifies graph scans using that side table. This is harder to maintain than a single explicit scope model because future grammar tweaks have to preserve hidden coupling between two partial parsers.
Example
To add support for another scoped construct or VALUES shape, a maintainer has to update both the pre-pass and the main walker, and keep their ideas of group identity aligned through raw brace offsets. That is fragile parser state, not a clean model.
Suggested direction
Use one tokenizer/walker that tracks group frames, graph frames, probe/template state, and scoped VALUES bindings together. That would delete collectScopedValuesBindings/matchingBrace coupling and make the parser invariants local.
For Agents
Refactor analyzeQuery so the main query walk owns group frames and scoped bindings directly. When a VALUES group is encountered, attach concrete bindings to the current frame or pre-tokenize once into events; then GRAPH ?g classification can consult the active frame instead of a separate parent-offset map. Preserve current fixture expectations around sibling subqueries and concrete VALUES bindings.
Review follow-ups on #1745: - Multiset ratchet: the baseline is now fingerprint -> count, and each head occurrence consumes one. Duplicating a grandfathered query in a changed file previously passed as pre-existing; it now blocks as one new finding. The diff-gate core is extracted as computeDiffFindings() and covered by an integration self-test on a throwaway git repo (duplicate blocks / reindent stays grandfathered / new shape blocks). - Keyword-terminated triples: SPARQL allows a triples block to run straight into FILTER/OPTIONAL/BIND/... with no dot; the walker now flushes the preceding triple at keyword boundaries and before group-opening headers, so '?s ?p ?o FILTER(...)' is caught in default and GRAPH ?var scopes. String-literal contents are dropped during analysis so quoted keywords cannot confuse detection. - Scoped LIMIT exemption: only a TOP-LEVEL 'LIMIT without ORDER BY' exempts R1/R2 (and R4); a subquery LIMIT no longer suppresses findings for unrelated outer scans. Fail-closed corollary pinned by fixture: a subquery-local LIMIT does not exempt its own group's scan (pragma if intentional). - Fixture suite grows 20 -> 27, all analyzer fixes pinned; self-test (fixtures + diff gate) still runs before every CI scan. - Replace a stray NUL byte in the fingerprint separator with ':' (NUL made the file grep/diff-hostile). Full-tree baseline stays at 52 findings (18 R1 / 9 R2 / 12 R3 / 13 R4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Classic branch-protection API 404s on this repo (main is governed by ruleset 14325863); document the fetch-modify-PUT and the merge-before- flip ordering so the check cannot deadlock open PRs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| }, | ||
| }; | ||
|
|
||
| const UNBOUNDED_GRAPH_EXPR = /bucket|sharedMemoryUri|sharedMemoryMetaUri|dataGraphUri|contextGraphDataUri|metaGraphUri|contextGraphMetaUri|metaGraph|workspaceMetaGraph|wsMetaGraph|catalogGraph|ontologyGraph/i; |
There was a problem hiding this comment.
🟡 Issue: Use a parser boundary instead of maintaining a custom JavaScript lexer
What's wrong
The implementation adds a substantial hand-written lexer for JavaScript/TypeScript template literals. That is a lot of incidental complexity for a SPARQL lint rule, and it makes the most fragile part of the tool live in the same path as the policy logic. The maintainability issue is not whether the current fixtures pass; it is that future reviewers must reason about JavaScript tokenization edge cases before they can safely adjust a SPARQL rule.
Example
The function has to decide whether / begins a regex or division using lastSignificant, skip nested templates manually, and update source line numbers while walking arbitrary JS/TS. None of that is specific to SPARQL scalability rules, but every scanner change now inherits that parsing burden.
Suggested direction
Move template extraction onto a real JavaScript/TypeScript parser boundary, then keep the custom logic focused on SPARQL-shape analysis. If zero install in CI is mandatory, this can still be structured behind a small extractor interface so the hand-rolled fallback is isolated rather than fused into the main lint implementation.
For Agents
Replace the source extraction boundary with a standard JS/TS parser or an existing lexer utility if the repository already has one. Preserve the current extractTemplateLiterals contract or adapt scanSource to consume parser-provided template literal nodes, then run the existing fixtures to confirm the lint policy remains unchanged.
There was a problem hiding this comment.
Agreed and done in 8dd4166 — this one earned its keep rather than a defer: the hand lexer was demonstrably the fragile layer (an IRI-safety regex containing a quote desynced an early version during development). Extraction now goes through ts.createSourceFile + a template-literal walk; typescript is already a root devDependency, so the workflow gains only pnpm install --frozen-lockfile --ignore-scripts (~8s warm, cached via the standard pnpm setup) and zero new dependencies. The extractTemplateLiterals contract is unchanged and the whole regex-vs-division/nested-template/escape burden is gone; nested templates inside interpolations are now analyzed as their own nodes (a small coverage gain).
Verification: 27/27 fixtures + diff-gate self-test pass unchanged; full-tree audit identical at 52 findings (18 R1 / 9 R2 / 12 R3 / 13 R4); --all runs in ~1s.
There was a problem hiding this comment.
🟡 Issue: Make the R4 graph-family policy data-driven instead of regex magic
What's wrong
The most domain-specific rule in the scanner is encoded as a compact regex. That hides the policy boundary: which graph families are intentionally unbounded, why they are dangerous, and how new families should be added. The broad substring matching also makes the rule harder to reason about during review.
Example
A future maintainer adding a new graph family has to decide where to patch the alternation and whether to use a broad substring or an exact naming convention. There is no place to attach the reason a family is unbounded or examples that should match it.
Suggested direction
Model unbounded graph families as named records rather than a pair of opaque regexes. The matcher can still compile those records to regexes internally, but the policy should be readable and extensible as data.
Confidence note
I am treating this as a maintainability concern, not claiming a specific false positive or false negative from the current diff.
For Agents
Replace the two R4 regex constants with a data table of graph-family descriptors, each with explicit expression-name patterns, literal suffix/prefix patterns, and a short rationale. Keep the same fixtures passing and add one fixture per family entry as living documentation.
Review follow-up on #1745: replace the hand-written JS/TS lexer in the SPARQL scale lint with ts.createSourceFile + a template-literal walk. The lexer was the fragile layer (regex-vs-division disambiguation, nested templates, escape handling — an IRI-safety regex containing a quote desynced an early version); the parser boundary removes that whole class. typescript is already a root devDependency, so the workflow gains only 'pnpm install --frozen-lockfile --ignore-scripts' (~8s warm) and no new dependency. Extraction contract unchanged; template literals nested inside interpolations are now analyzed as their own nodes. Behavior verified identical: 27/27 fixtures + diff-gate self-test pass, full-tree audit unchanged at 52 findings (18 R1 / 9 R2 / 12 R3 / 13 R4), ~1s runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| import process from 'node:process'; | ||
| import ts from 'typescript'; | ||
|
|
||
| const SCANNED_EXTENSIONS = /\.(ts|mts|cts|js|mjs|cjs)$/; |
There was a problem hiding this comment.
🔴 Bug: TSX and JSX source files are excluded from SPARQL scanning
What's wrong
The lint gate filters changed files by extension before scanning. Since TSX/JSX are omitted, dangerous SPARQL added to React/UI source bypasses the required check entirely.
Example
Adding const q = �SELECT ?s WHERE { GRAPH ?g { ?s ?p ?o } }�; to a .tsx UI module would not be scanned at all because changedSourceFiles() filters the path out before scanSource() runs.
Suggested direction
Include .tsx and .jsx in the scannable extension set, since production UI code already builds SPARQL there.
For Agents
Update SCANNED_EXTENSIONS in scripts/sparql-scale-lint.mjs to include .tsx/.jsx, or otherwise prove production SPARQL cannot live there. Add a diff-gate self-test with a changed .tsx file containing a blocked R2 shape.
There was a problem hiding this comment.
Fixed in 0620768. .tsx/.jsx are now in the scanned extension set, with ScriptKind derived per extension — that detail is load-bearing: parsing .ts as TSX misreads generic arrows as JSX (and vice versa), and either misparse can silently drop template literals from the AST. A generic-arrow-in-.ts fixture pins the split.
You were right that this wasn't hypothetical: the expanded audit immediately surfaced a real R2 graph-var scan in packages/node-ui/src/ui/views/project/components/query.tsx (grandfathered by the ratchet; baseline 52 → 53).
Test evidence, per your ask: the diff-gate self-test now commits a changed .tsx file whose JSX only parses under ScriptKind.TSX, carrying the R2 shape — it must block as one new finding. 28/28 fixtures + diff-gate pass; 1101 files scanned in ~1s.
Review follow-ups on #1745 (round 3): - .tsx/.jsx files are now scanned, with ScriptKind derived from the extension (parsing .ts as TSX misreads generic arrows as JSX and vice versa — both can silently drop template literals from the AST). The concern was live, not hypothetical: the expanded audit surfaces a real R2 graph-var scan in node-ui's project query.tsx component (grandfathered; baseline 52 -> 53). Diff-gate self-test now includes a changed .tsx file whose JSX only parses under TSX, carrying an R2 shape that must block; a generic-arrow-in-.ts fixture pins the ScriptKind split. - --all and --files now run the embedded self-test before scanning, matching --diff: audit FINDINGS stay non-blocking, but a broken scanner fails the run instead of reporting garbage with exit 0. Proven by a spawned-CLI check inside the diff-gate self-test (env-guarded against recursion) that requires the self-test banner before the findings output. - Capture git stderr in fileAt(): probing base versions of added files is expected to fail and must not print 'fatal:' noise into CI logs. 28/28 fixtures + diff-gate pass; full-tree audit 53 findings (18 R1 / 10 R2 / 12 R3 / 13 R4) across 1101 scanned files, ~1s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| const ref = graphFrame.ref ?? ''; | ||
| const phMatch = ref.match(/⟪(\d+)⟫/); | ||
| const exprText = phMatch ? (exprs[Number(phMatch[1])] ?? '') : ''; | ||
| const literalRef = ref.replace(/⟪\d+⟫/g, ''); |
There was a problem hiding this comment.
🔴 Bug: Interpolated context-graph data IRIs are missed by R4
What's wrong
R4 explicitly covers scans over the context graph data graph, but the implementation only detects static did:dkg:context-graph:x literals or expression names matching dataGraphUri-style heuristics. Directly interpolated graph IRIs using ordinary names like cgId fall through and allow the #1609-style whole-graph scan shape to merge.
Example
Failing self-test sketch:
source: 'const q = `CONSTRUCT { ?s ?p ?o } WHERE { GRAPH <did:dkg:context-graph:${cgId}> { ?s ?p ?o } }`;',
expect: ['R4']Current behavior is no finding, even though the literal prefix identifies the unbounded context-graph data graph family documented as R4.
Suggested direction
Treat did:dkg:context-graph: prefixes with a placeholder and no bounded per-KA suffix as unbounded data graph scans, rather than stripping the placeholder before matching.
For Agents
In R4 graph-family detection, preserve enough literal structure around interpolation to classify did:dkg:context-graph:${...} as the data graph family. Add fixtures for direct interpolated data graph IRIs and keep exact per-KA graph exemptions intact.
2 participants
What
A new required-check candidate: SPARQL scalability lint — scans every PR's changed source for SPARQL query shapes that have taken production nodes down before, and fails when a PR adds one. Pre-existing debt is grandfathered (ratchet); nothing currently merged starts failing.
Rules — each encodes a real incident class
R1 unscoped-all-var-scan?s ?p ?owith no bound term, no graph scopeR2 graph-var-scanGRAPH ?glistGraphssync stormR3 offset-paginationOFFSET n(n>0/interpolated)R4 bucket-graph-scan_shared_memory/_meta/data-graph/_catalogfamilies, no LIMITDeliberate exemptions keep the blessed idioms green:
FILTER EXISTSprobes (the fixed #1597 form is a fixture), plainASK,LIMITwithoutORDER BY,VALUES-bound graph vars, exact per-KA graph reads, CONSTRUCT/INSERT/DELETE output templates, and test/e2e/bench files.Escape hatch
A provably bounded query is acknowledged in code, so the decision is diffable and review-visible:
// sparql-scan-allow: R4 -- catalog floor is capped at 64 triples per CGRule id must match; justification must be non-empty.
Validation performed
SELECT DISTINCT ?g WHERE { GRAPH ?g { ?s ?p ?o } }topackages/publisher/src/metadata.ts→ blocks (exit 1) with a GitHub error annotation; adding the pragma → passes as acknowledged.graph-plan.ts, real bucket scans indkg-publisher.ts/catalog-extractor.ts, and the adapter-level store primitives. Runnode scripts/sparql-scale-lint.mjs --allto see the debt list.pnpm install.Trigger design
pull_requestwith no branch filter — stacked PRs whose base is a feature branch (outsideci.yml's list, the gap observed on feat: add graph-scoped KA publish engine #1712) still get the gate.merge_group— required so the merge queue doesn't stall once this is a required check.HEAD^1), per the stale-base.shaincident documented inci.yml's plan job.To make it blocking
After this lands and a few PRs confirm the signal quality:
gh api repos/OriginTrail/dkg/branches/main/protection/required_status_checks/contexts \ -X POST -f 'contexts[]=SPARQL scalability lint'Docs:
docs/sparql-scale-lint.md.🤖 Generated with Claude Code