TraceQL: enumerate the accept surface from the reference grammar, and close the reachable divergences (#335) - #444
Merged
Conversation
Checkpoint before the remaining Stage D0 artefacts (the 19-key plan freeze, the registry split, the CI addition rule).
The audit's subject was 24 of the reference grammar's 33 productions while its prose said "the whole production set". This stage enumerates all 33, one committed row each, and probes what the omission hid. * accept_surface/grammar_slots.json — one row per production, with the command that produced the list, its disposition, its probes and its reason. GRAMMAR_SLOTS is a fixed-size array, so deleting a row does not compile. What CANNOT be gated (that the list is 33 and not 34 — the grammar is not vendored and CI has no Tempo checkout) is stated in the file's own header rather than dressed as a check. * matrix.json — 85 probes appended, 29 agree / 56 diverge, classes D13-D24, each replayed twice against the digest-pinned oracle with no inconclusive answer. TOTAL 221->306, DIVERGE 0->56. Nothing regressed: the surface became measured, and the reason sits in the doc block above the constants. * accept_surface/reachability.json — nine declared anchors and one captured HTTP body, named by id. A probe's record carries no path, line range or request, so 56 forgery surfaces become ten reviewed once. Every diverging probe records which client path reaches its construct; every field of that record is compared against something. REACHABILITY_DIGEST feeds the anchors' CONTENT, so editing a line range moves it. * wire_baseline.json — 85 entries re-derived from the tree, constants 10->63 diverging, plus a `note` beside them that a test holds to naming both numbers. * traces_by_key_plan_freeze.rs — the 19 served by-key kinds, plan-pinned BEFORE the Stage D2 grammar change, so D2 cannot generate the artefact it is measured against. * conformance: static.kind_enum declared six spellings, probed one and claimed `supported` — split into six per-spelling constructs, and static.min_int / static.max_int ADDED, because the registry is built from the reference's list and could not previously see a construct we never implemented. Three land as tracked interim gaps owned by #335. A new gate refuses a construct that declares a spelling nothing probes. * ci.yml — the addition rule (A1-A5) so the gate can admit a new true measurement, with A3 refusing a disguised oracle re-pin; and a set-difference evidence-debt gate that fails open LOUDLY. * PROVENANCE.md — the Stage D0 section, and the two `by(...)` operand rows that were false in BOTH directions, split by production and each now compared against its own probe by a test. No src/ change.
Three parser-local closures. Ten probes move diverge->agree and nothing
moves the other way.
D13 — `unspecified` is the sixth span kind. The reference's Kind enum
opens with KindUnspecified (enum_statics.go:146-155 @ v3.0.2), its lexer
maps the bare word unconditionally (lexer.go:43), and `static` carries
KIND_UNSPECIFIED as a terminal (expr.y:436). This is the divergence our
own product created: pulsus-read renders `unspecified` into by(kind)
group values, select(kind) projections and the by(kind) series label of
served SQL, the reference offers it back from its kind tag-values
endpoint (captured), and the Grafana datasource writes an enum-intrinsic
ad-hoc value unquoted — so `{kind=unspecified}` is a query a user gets
handed. The reference served it 200; we answered 400.
filter.rs's kind_code gains `Unspecified => 0` — OTLP's code, matching
metrics_sql.rs's KIND_MAP, which has mapped 0 to 'unspecified' since
before the keyword was parseable. That match is exhaustive with no
wildcard and is the only one of the three read-path kind sites the
compiler could have refused a new variant at; the note saying so is at
the site.
D14 — minInt/maxInt. Resolved in `static_value_of` from the token alone,
which is where the reference resolves them (expr.y:442-452). i64 because
the pinned image is linux/amd64 and Go's math.MinInt is the platform int;
the reason is written at the constant.
D23 — repeated root with(). `root: root hints` (expr.y:134) is
recursive, so more than one clause is legal. The LAST clause WINS:
withHints is `r.Hints = h`, a plain assignment (ast.go:112-115). The
plan's sketch said clauses concatenate; the reference does not, and
shipping that reading would have been a quiet semantic divergence behind
an accept-surface fix that looked complete.
Also: a hint VALUE is the whole `static` production (expr.y:371-373), so
parse_hint routes its identifier through static_value_of instead of a
shorter hand-written list. `with(k=server)` was refused too, though no
probe named it.
`{ .a = unspecified }` now agrees on the parse axis and is still a
planner 400 — the attribute predicate has no kind/status arm, a gap that
predates D13 and that D13 made reachable. Recorded as D13's wire_status
with its note, and the probe names its owning issue.
One corpus round-trip exception, with the reason and the measurement:
i64::MIN has no positive literal spelling, so its rendering must carry a
`-`, which this grammar re-parses as an operator. The reference cannot
re-parse its own rendering either, and fails harder — measured 400,
`strconv.Atoi: parsing "9223372036854775808": value out of range`.
PINNED_SQL_CORPUS and the 19-key plan freeze are both unchanged.
`groupOperation` is `BY '(' fieldExpression ')'` (expr.y:177-179 @ Tempo
v3.0.2) — one operand, and that operand is a full field expression. Ours
was a comma list of bare fields, wrong in BOTH directions, so both are
fixed in one stage: they are the same production and splitting them would
touch the same AST field twice.
WITHDRAWN: `| by(.b, .c)`. The production carries no COMMA and
fieldExpression has none either, so the reference parse-rejects it —
measured 400 `syntax error: unexpected ,`. We accepted it AND SERVED it,
which is the only shape in the whole audit where a user gets a
wrong-looking 200 here and a 400 there: the query works, so the user
builds on it, and it is not portable to the system we claim
compatibility with. Ledgered as `traceql-spanset-by-multi-key-withdrawn`
and removed from docs/api.md §4.2, where it was promised.
WIDENED: `by(.b + .c)`, `by(-.b)`, `by(!.b)`, `by((.b))`, `by(.b = 1)` —
all reference 200s we refused. Four of the five are still a clean planner
400 (a grouping key must resolve to one per-span value), named as such
rather than answered with a silent flat 200 — the Stage C
aggregate-argument pattern one production over. D16 carries
`wire_status: "open"` with its note, and each probe names its owner.
The METRICS by(...) is untouched: it is `attributeList` (expr.y:195-198)
and its comma list is correct. That conflation — one keyword, two
productions — is what made this invisible for four stages.
Nothing served moved, and the evidence is not the frozen SQL corpus
alone: `traces_by_key_plan_freeze.rs`, pinned in Stage D0 BEFORE this
change, re-derives all NINETEEN served by-key plans byte-identically.
PINNED_SQL_CORPUS is unchanged too, but it covers three of the nineteen
and is labelled corroboration.
Found by replaying the job against rebuilt git states rather than by reading it: the CONTROL case — HEAD identical to origin/main — exited GATE ERROR. Under `jq -e` a filter that emits nothing exits 4, and the filter ended in `.[]`, so an EMPTY difference (the pass) was indistinguishable from a broken gate. `join` always produces one value, a string is truthy in jq including the empty one, and the caller decides on the text. A compile error is still 3 and a runtime error 5, both reported as GATE ERROR. Replay, script extracted from ci.yml with a YAML parser: rc=0 control: HEAD == main rc=0 this branch (debt shrank by one) rc=1 a NEW supported row with evidence: [] rc=1 a count-preserving swap (new bare row + another gains evidence) rc=2 the filter itself broken -> GATE ERROR, never a pass
Both are review round 1 findings, and both are the same defect: prose outliving the change under it. matrix.json's D23 class row still said repeated `with(...)` clauses "concatenate". The code does not, and neither does the reference: the rule's action is `expr.withHints($2)` and `withHints` is `r.Hints = h`, a plain ASSIGNMENT (ast.go:112-115 @ v3.0.2), so the last clause wins. A data file that is the machine-readable record of what we believe the reference does may not contradict the parser built from it. The row states what backs it and no more: the rule and its citation, the measured fact this accept-surface class actually scores (every clause order is 200), and the corpus golden that pins our side. It does NOT claim a live confirmation of the last-wins RESULT: the review round measured one, but my own attempt at that channel was non-discriminating in my container config — `most_recent` returned the same trace for `true`, `false`, both clause orders and no hint at all — and an artefact must carry what its author measured. PROVENANCE.md still called Stage D1 "parser-local" after D1 touched `crates/pulsus-read/src/traces/filter.rs`. Corrected to say why it is not, and why that is the intended constraint rather than an accident: `kind_code`'s match is exhaustive with no wildcard, so it is the only one of the three read-path kind sites where the compiler can refuse a sixth variant. The section also records that a diff-path gate was considered and withdrawn BEFORE implementation — incomplete if it lists only the read path, vacuous if it lists the parser too, because the AST is a type contract — so no check was weakened to accommodate the wording.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #335.
The previous audit built its probe set from our own precedence table and covered 24 of the reference grammar's 33 productions. Enumerating from the grammar found 85 probes, 29 agreeing and 56 diverging, in twelve classes — and none of the new ones was visible to any existing check.
The top divergence is one we inflict on ourselves. Our read path emits
unspecifiedas a span-kind value; Grafana displays it and writes{kind=unspecified}back; the reference serves200; we answered400. We produced a value we then refused to accept. Fixed, along withminInt/maxIntand repeated rootwith()— exactly 10 probes move diverge→agree and none the other way.One accept is deleted rather than kept.
{ … } | by(.b, .c)parsed and served here and is a parse error at the reference. It is the only shape in the whole audit where a user gets a working answer here and a400there — so they build on it, and the query is not portable to the thing we claim compatibility with. Withdrawn, ledgered, and witnessed by a probe that now rejects.The plan was wrong about repeated
with(). It said the clauses concatenate; the reference replaces —withHintsisr.Hints = h(pkg/traceql/ast.go:112-115@ v3.0.2). Implemented last-wins.Eleven plan revisions, and the findings were settled at the second. The rest built the evidence: every claim now records whether it was measured, read or argued, with the command and its scope; the reachability ranking is 56 rows of committed data rather than a summary; and the whole apparatus carries a stated boundary — it checks the form of its evidence, not its truth, because CI has no reference checkout.
Reviewer
VERDICT: PASSonbd9db3a: #335 (comment)mainmerged in; both this branch's and #385's ledger entries kept, and both parents verified to have lost no line.https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv