Skip to content

fix: canon directional literals, terse K: boundary, mcp nl_query config, gpu OOB GROUP BY keys - #6659

Merged
jeswr merged 4 commits into
mainfrom
fix/issues-canon-terse-mcp-gpu
Oct 5, 2026
Merged

jeswr merged 4 commits into
mainfrom
fix/issues-canon-terse-mcp-gpu

Conversation

@jeswr

@jeswr jeswr commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Requested by Jesse · project thread

Summary

Four small, independent fixes, each with a regression test that failed before the change.

  • sparq-canon (sparq-canon: standard path silently rejects RDF-1.2 directional-language literals via the oxrdf-0.2 bridge #5359). An RDF 1.2 directional-language literal ("hello"@en--ltr) reaching a standard rdf-canon-backed entry point used to fail as a generic CanonError::Bridge, because the oxttl-0.1 re-parse is RDF 1.1. Every standard path now checks for these literals up front and returns a typed CanonError::DirectionalLiteral. The enum is #[non_exhaustive], so adding the variant is non-breaking. The crate docs and README now say so, and the error message points to the rdf12-triple-terms profile, which canonicalizes these literals natively; a test asserts that. This PR doesn't conflict with fix(canon): label-independent tie-break for tied RDFC-1.0 N-degree hashes #6645, which only vendors rdf-canon and adds a test file; it doesn't touch the bridge in src/lib.rs.
  • sparq-terse (sparq-terse: the K: sigil scanner has the same ASCII-only name-boundary bug as keyword_hints did #4662). The K:, V( and PREFIX token-boundary checks used an ASCII-only predicate, so ?aéK:x was misread as the K: sigil and a valid query was rejected with UnknownKeyword. The new preceded_by_name treats any non-ASCII byte (outside strings, IRIs and comments it can only be a PN_CHARS name character) and a preceding PN_LOCAL_ESC (\-, etc.) as part of a name. Tests cover the Unicode variable, the ex:a\-K:x local-name case (also broken before) and the V( scanner.
  • sparq-mcp (sparq-mcp: nl_query hardcodes NlqConfig::default(), so it would silently diverge from ask if check_dictionary is ever enabled #4833). ask and nl_query now start from one base_config(). A new run_nl_query_with(graph, question, config, llm) applies the same check_dictionary repair branch that Nlq::ask has, so the two tools can't drift if that flag is ever turned on. The default behaviour doesn't change, and both_tools_accept_an_ungrounded_predicate still passes. The new test fails if the branch is disabled. skills/agent-tools/SKILL.md is updated.
  • sparq-gpu (sparq-gpu: GPU silently undercounts on out-of-range GROUP BY keys where the CPU oracle panics, and an unbounded probe loop can wedge the device #4603).
    • Out-of-range GROUP BY keys. A key >= groups now sets a flag word on the device, with no host scan, and both Gpu::group_aggregate and the CPU oracle cpu::group_aggregate return Err(GroupKeyOutOfRange). Before, the GPU silently dropped the row and the CPU panicked. Checked on lavapipe: with the old kernel behaviour the new differential test fails with Ok([...9999 rows...]) where the CPU returns Err.
    • Table checks. upload_table now rejects tables that aren't a power of two or have no EMPTY_KEY slot, in release builds as well. Before, this was only a debug_assert!.
    • Bounded waits. The WGSL probe walk, and its CPU mirror, stop after mask + 1 steps. run() now waits at most POLL_TIMEOUT (60 s) for its own submission and then panics, instead of using wait_indefinitely().
    • write_table deliberately skips the O(n) table check. It's the timed refill in the e2e benchmark legs, and the bounded walk already stops a full table from hanging.
    • group_aggregate now returns a Result. The callers in the example and tests, and skills/gpu-kernels/SKILL.md, are updated; the skill's sample also no longer uploads a full table.

Closes #5359
Closes #4662
Closes #4833
Closes #4603

Base gate (always required)

  • cargo build --workspace succeeds. (Left to CI; only the touched crates were built locally.)
  • cargo clippy --workspace ... is clean. (Left to CI. Ran crate-scoped cargo clippy -p {sparq-canon,sparq-terse,sparq-gpu,sparq-mcp} --all-targets --all-features -- -D warnings: clean.)
  • The code this PR touches is formatted (only touched hunks; no cargo fmt --all).
  • cargo test passes for every crate this PR touches: sparq-canon (default and --all-features), sparq-terse --all-features, sparq-gpu (run on a real adapter: Mesa lavapipe/llvmpipe Vulkan, so the GPU differential tests actually ran and weren't skipped), sparq-mcp --features nlq.

Targeted re-evaluation

  • Public API: CanonError::DirectionalLiteral, sparq_mcp::nlq::run_nl_query_with, sparq_gpu::{GroupKeyOutOfRange, POLL_TIMEOUT}, and the group_aggregate signatures (GPU and CPU) now returning Result. Updated skills/agent-tools/SKILL.md and skills/gpu-kernels/SKILL.md, plus the sparq-canon README and crate docs.

Ratchets and conventions

  • No ratchet lowered.
  • No TODO/FIXME markers added.
  • Docs whose statements changed were updated in this change.

Security

  • No security regression. The sparq-gpu change closes the device-hang and silent-undercount items T-GPU-INT and T-GPU-DoS in research/gpu-threat-model.md.

Performance check (local, before vs after main)

Non-canonical, shared-box measurements: 4 cores, other jobs running, release profile. Each binary was built from origin/main and from this branch in separate target dirs. The runs alternate main/PR to cancel drift. Noise band is the p10–p90 spread of the per-run times, relative to the median.

Workload main (median ms) PR (median ms) Δ median (Δ best) Noise band
sparq-canon canonicalize_quads, 100k quads (20k blank nodes, 4 graphs, lang/typed literals); 15 runs, best of 3 1138.3 1121.3 −1.5% (−3.9%) ±12%
sparq-canon canonicalize_triples, default-graph subset (25k triples); 15 runs, best of 3 224.5 221.2 −1.5% (−1.0%) ±9%
sparq-terse terse_to_sparql, 6,000 queries (K: sigils, Unicode names); 25 runs × median of 7 45.8 47.0 +2.5% (0.0%) ±13–31%

Verdict: no regression beyond noise. The up-front directional-literal scan is one pass over object terms and doesn't show up next to the RDFC-1.0 work. The terse boundary check costs O(1) per candidate sigil byte; best-of times are identical. sparq-gpu was not timed: lavapipe is too noisy.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ScyGGohDhirnbLbrUSnA5n


Generated by Claude Code

…ig, gpu OOB keys

- sparq-canon (#5359): standard (rdf-canon-backed) paths fail closed with a
  typed CanonError::DirectionalLiteral on RDF 1.2 directional-language
  literals instead of a generic Bridge error from the oxttl-0.1 re-parse;
  documented in crate docs + README, pinned by tests/directional_literal.rs.
- sparq-terse (#4662): the K:/V()/PREFIX scanners treat non-ASCII bytes and a
  preceding PN_LOCAL_ESC as name characters, so `?aéK:x` and `ex:a\-K:x`
  pass through unchanged.
- sparq-mcp (#4833): nl_query and ask share one base NlqConfig; new
  run_nl_query_with takes an explicit config and applies the same
  check_dictionary repair branch Nlq::ask does.
- sparq-gpu (#4603): out-of-range GROUP BY keys set a device flag and return
  GroupKeyOutOfRange (the CPU oracle returns the same error instead of
  panicking); upload_table rejects non-power-of-two / full tables in release;
  the probe walk is bounded at mask+1 steps (CPU mirror too); run() polls with
  a 60 s timeout instead of waiting indefinitely.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ScyGGohDhirnbLbrUSnA5n
@jeswr jeswr self-assigned this Oct 5, 2026
@jeswr
jeswr marked this pull request as ready for review October 5, 2026 19:34
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ScyGGohDhirnbLbrUSnA5n
@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head cba440f6fd59. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

No correctness, security, soundness or design problems found.

Verdict: The PR is safe to merge as is.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ScyGGohDhirnbLbrUSnA5n
@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 3318578eb2c8. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

  1. [medium] Timeout panic can enter another unbounded GPU wait — crates/sparq-gpu/src/lib.rs:749
    If a stalled device returns a timeout, .expect() panics. With panic unwinding, an owning caller drops Gpu; wgpu 30’s queue destructor then calls wait_for_idle() without a timeout, potentially blocking indefinitely on the same stalled submission. This defeats the bounded-failure behavior in dev/test builds, although the repository’s release panic = "abort" avoids it.

    Make timeout handling avoid synchronous teardown of a stalled queue. If fatal termination is intended, use a non-unwinding failure path; otherwise isolate GPU execution so it can be terminated independently.

Verdict: Fix the timeout cleanup path before merging.

…ed queue

The #4603 bounded poll panicked on timeout. On an unwind the caller's Gpu
is dropped, and wgpu-core 30's Drop for Queue calls the HAL's
wait_for_idle() with no timeout, so it could block forever on the same
stalled submission.

Kernel calls now return Result<_, GpuError>. On a poll failure, run()
marks the Gpu stalled and returns GpuError::Stalled. Later calls fail
fast without touching the device, and Drop for Gpu forgets an extra
clone of the Arc-backed wgpu Device and Queue, so their destructors
(and the unbounded idle wait) never run. This stays safe code
(forbid(unsafe_code)). group_aggregate's out-of-range key error becomes
GpuError::GroupKeyOutOfRange, with From<GroupKeyOutOfRange>.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ScyGGohDhirnbLbrUSnA5n
@jeswr

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 16887eec1e6c. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

No correctness, security, soundness or design problems found.

Verdict: The PR is safe to merge as is.

jeswr commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Local ci-fast gate (GitHub Actions outage; Jesse approved local-gate merges). PR head 16887eec merged with main afb6badb:

  • clippy -D warnings (core crates, all targets): pass
  • nextest (core crates, ci profile): pass
  • doctests (core crates): pass
  • W3C SPARQL conformance 1229/1229 (ratchet 1229): pass

Squash-merging under the local-gate rule.


Generated by Claude Code

@jeswr
jeswr merged commit bb7c604 into main Oct 5, 2026
6 checks passed
@jeswr
jeswr deleted the fix/issues-canon-terse-mcp-gpu branch October 5, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment