Skip to content

compiler: the diagnostics contract for refused queries (RFC 0047, PR 1) - #759

Open
ragnorc wants to merge 2 commits into
mainfrom
rfc-0047-pr1-diagnostics
Open

ragnorc wants to merge 2 commits into
mainfrom
rfc-0047-pr1-diagnostics

Conversation

@ragnorc

@ragnorc ragnorc commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

What & why

Background

OmniGraph runs queries written in GQ, its query language. When the compiler refuses a query, the caller gets one line of text. Today that line is all there is. The parser's message points at the first character of the file and names a grammar rule, and the CLI wraps it in colour codes and a backtrace hint. A type error carries a code such as T33 only as the first word of its message. Nothing tells the caller where the problem is or what to write instead.

The callers that matter most are agents. An agent treats an error as the documentation it acts on, and it retries by default. We measured this with the in-context competence instrument (RFC 0047): 45 of 45 tasks ended correct, but 8 of 45 first attempts were refused, and every one of those refusals was the same case. The agent wrote query name { without the empty parameter list, and the parser answered parse error --> 1:1 … expected query_file. That message names neither the missing ( nor the fix.

What this change does

This is the first of nine pull requests that implement RFC 0047, "Search plan truth" (accepted on 2026-09-19 in #606). It lands the RFC's diagnostics contract. Every refusal of a query now carries four things:

  1. A stable code. Q… codes come from the parser, T… codes from the type checker.
  2. Where the failure is. A parse refusal carries the line and column. A refusal after parsing carries the compiler stage, and the expression when the site can render it.
  3. What was expected or violated.
  4. One concrete fix that names the construct to use. A refusal with no fix names the decision instead.

The measured case now reads:

error[Q002]: parse error: expected `(`: a query declares its parameters even when it has none
  --> line 1, column 11
  fix: query name()

The four fields travel as data through every surface that reports a refused query: the HTTP error body, the CLI in its human and machine formats, omnigraph lint, queries validate and cluster plan.

What does not change

  • Every existing error message keeps its exact one-line text. The Display form is the legacy form (parse error: …, type error: T33: …), so every existing assertion and every .gqt error needle still holds. One exception: a parse refusal's one-line text no longer embeds the parser's multi-line rendering.
  • Every existing HTTP error body is byte-identical. The new diagnostic field is optional and is omitted when absent.
  • No query that compiled before is refused now, and no query that was refused before is accepted now. This pull request changes how refusals are reported, not which queries are refused.
  • Schema (.pg) parse errors are outside the contract and are untouched.

Backing issue / RFC

How it is built

One type, converted once at each boundary.

  • Compiler. QueryDiagnostic { kind, code, message, position, stage, fix } lives in crates/omnigraph-compiler/src/query/diagnostic.rs, with the code catalogue in query/codes.rs. CompilerError::Query(Box<QueryDiagnostic>) replaces CompilerError::Type(String). CompilerError::Parse(String) stays for the schema parser.
  • Parser. A grammar mismatch is Q001, positioned at the parser's deepest failure and naming the rules it expected there. The missing parameter list is Q002, from a new grammar rule (missing_param_list). The pest parser records attempts per rule, not per token, so the missing ( is never an attempt it can name. The rule matches a declaration whose name is not followed by ( and reports it at the name's end. Settings refusals are Q003, branch and show statement refusals are Q004, and a declaration body the hand-written parser refused is Q005.
  • Type checker. The 124 sites that built a T… message convert mechanically to CompilerError::typed(T33, …), with the code as a value instead of a string prefix. Eight refusals that had no code get T38 to T44 (one duplicate reuses T6). Six internal consistency checks in the result descriptor become Plan errors, because they are not user diagnostics.
  • Wire. ErrorOutput gains diagnostic: DiagnosticOutput { code, position, stage, expression, expected, fix }. The RFC names flat fields. They are nested under one detail because a code needs a home the closed ErrorCode enum cannot give, and one optional field keeps every existing body byte-identical.
  • Server. ApiError::from_compiler maps every compiler refusal, on the Compiler arm and in classify, so every door carries the detail.
  • CLI. --format jsonl counts as a machine format. The embedded lane prints the same error body as the served lane. The human lane prints error[CODE]: …, the position or stage, and the fix on stderr. color-eyre installs without its environment and location footers, so no backtrace hint appears.
  • Stored-query validators. queries validate --json and cluster plan --json carry the same object beside their messages. A stored query that a later release refuses is therefore a finding before the upgrade, which the deployment gate in PR 2 and PR 5 relies on.
  • Size. The diagnostic stays under Clippy's 128-byte Result threshold: the code is a pointer-sized handle to a static catalogue entry, the byte offset is 32-bit, and a size test pins it.

How to review

  1. Read crates/omnigraph-compiler/src/query/diagnostic.rs and query/codes.rs first. They define the whole contract.
  2. Read the grammar change in query/query.pest and pest_error_to_diagnostic in query/parser.rs. The grammar comment explains why a recognizer rule is needed.
  3. The type checker diff is large but mechanical. Every CompilerError::Type(format!("Tnn: …")) became CompilerError::typed(Tnn, format!("…")). Spot-check a few sites.
  4. Read docs/user/queries/diagnostics.md. It is the user-facing statement of the contract.

Checklist

  • Change is focused (one logical change)
  • Tests added/updated: parser (Q001, Q002 and Q003 positions and fixes), type checker (a T… error exposes its diagnostic), server unit test and openapi pins, data_routes (every door reports Q001 and Q002 with position and fix), CLI (cli_queries: both transports, json, jsonl and human)
  • Public docs updated: new docs/user/queries/diagnostics.md, cross-references in the queries guide, CLI reference, troubleshooting page, and the release note
  • Reviewed against invariants: strengthens 9 (diagnostics are typed structures, no string-parsed codes) and 8 (loud, self-describing refusals); no deny-list item

Local verification

  • cargo test -p omnigraph-compiler -p omnigraph-api-types -p omnigraph-cluster --lib — 382, 4 and 151 passed
  • OMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapi openapi_spec_is_up_to_date then cargo test -p omnigraph-server --test openapi — 104 passed
  • cargo test -p omnigraph-server --lib a_refused_query — passed
  • cargo test -p omnigraph-server --test data_routes parse_error_precedes_policy_denial_on_every_door — passed
  • cargo test -p omnigraph-cli --test cli_queries q002 — passed (embedded and served)
  • cargo test -p omnigraph-gqt --locked — complete corpus, 101 cases passed (every expect error: T… needle holds)
  • cargo test -p omnigraph-cli --test cli_queries --test cli_data --test parity_matrix — 89, 24 and 23 passed
  • cargo test -p omnigraph-server --test data_routes --test stored_queries --test openapi — 147, 74 and 104 passed
  • cargo clippy on the six changed crates, --all-targets -- -D warnings -W clippy::dbg_macro — clean
  • cargo fmt --all --check — clean
  • cargo test -p omnigraph-vocabulary-guard --lib openapi_inventory_matches — passed after the three PositionOutput column occurrences were classified in the guard's inventory (the first CI run's only failure). The guard's git-based tests need a signing-capable environment locally and pass in CI.
  • python3 scripts/check-docs.py, bash scripts/check-agents-md.sh, typos — ok
  • CI: 24 checks pass, 6 skip by design.

Notes for reviewers

  • The T… catalogue gives every code a one-line short. Per-code fix text is written by the pull request that owns each code (T27 in PR 2, T26 in PR 5) and by a follow-up sweep. This pull request is the carrier, not a rewrite of 130 messages.
  • Q005 is deliberately coarse. The hand-written declaration parser has 59 refusal sites without positions. Positioning them is the same follow-up sweep.
  • docs/user/cli/reference.md sits exactly at the 350-line cap of the docs check, which is why the diagnostics text lives on its own page.
  • The two commits are unsigned. The signing agent was unavailable when they were made. They will be re-signed before merge.

Every query compile refusal now carries a stable code, where the
failure is, what was expected, and one fix, as one typed value from the
parser to every surface that reports it. `QueryDiagnostic` lives in the
compiler with a code catalogue; `CompilerError::Query` replaces
`CompilerError::Type(String)` with a `Display` that keeps the legacy
one-line form, so every existing assertion and `.gqt` needle holds.

The parser positions a grammar mismatch at pest's deepest failure with
the rules it expected (`Q001`). A declaration without its parameter list
is refused at the name's end with the fix `query name()` (`Q002`) by a
committal grammar recognizer, because pest tracks attempts per rule and
the missing `(` is never an attempt it can name; settings and branch
statement refusals carry their positions (`Q003`, `Q004`). The 124 coded
typecheck sites convert mechanically to `CompilerError::typed`; eight
uncoded user-facing refusals gain codes, and the descriptor's internal
consistency errors become plan errors.

On the wire, `ErrorOutput` gains an additive, skip-serialized
`diagnostic` detail, so every existing error body is byte-identical. The
server maps every compiler refusal through it at every door. The CLI
treats `--format jsonl` as a machine format, prints the same error body
for the embedded lane as for the served one, renders the four fields on
stderr for human formats, and installs color-eyre without its
environment and location footers. `queries validate --json` and
`cluster plan --json` carry the same detail beside their messages, so a
stored query a later release would refuse is a pre-upgrade finding.
A diagnostic's position carries a character column, the coordinate of
the diagnostics contract; the guard's OpenAPI inventory now records the
three occurrences (the schema prose, the property, the required name)
as retained renderer vocabulary, not a tabular or physical column.
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.

1 participant