Differential conformance harness: Harper's REST parser vs. the RQL 2.0 reference parser - #87
Open
kriszyp wants to merge 9 commits into
Open
Differential conformance harness: Harper's REST parser vs. the RQL 2.0 reference parser#87kriszyp wants to merge 9 commits into
kriszyp wants to merge 9 commits into
Conversation
Runs a 349-case corpus through both Harper's REST query parser and the reference parser, maps Harper's output into the canonical model (§6) with an adapter, diffs the two, and classifies every difference as an existing row of Harper's divergence ledger (harper#2440, which spec Appendix D links to), a NEW divergence with a proposed row, or a reference-parser bug. An unclassified divergence fails the run, so the report cannot quietly go stale. Record/replay, per the ruling on the planning gate: `--record` gives every Harper parse its own short-lived process (Harper's parser holds module-global state) and writes a provenance-stamped fixture; ordinary runs replay it, so CI needs no Harper checkout and `--check` asserts the committed report is reproduced byte for byte. Replay uses one persistent reference worker, killed and replaced on a timeout. `conformance/ledger.json` is a refreshable cache of harper#2440, not a second source of truth. First run: 190 agree, 33 map to ledger rows, 117 are new divergences across 24 proposed rows, 9 are reference-parser bugs, 0 unclassified. Nothing in specification/ or src/ is touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
From the pre-push review (Cursor/Grok, round 1): - ReferenceRunner had no `exit`/`error` handler, so a worker that died mid-parse left its promise unsettled and `--check` hung forever instead of failing. The same applied to a replacement that would not start: the rejection escaped a `setTimeout` callback and the parse never resolved. Both now settle every in-flight parse with a harness error, which reaches the classifier, matches no rule, and fails the run with the query named. - `rejected` on one side and `deferred-error` on the other are no longer counted as agreement — the deferring side still produced a partial result. Neither is a shared timeout or adapter gap: an unobserved parse must never read as a pass. - `ledger-6` and `ledger-7` matched on the corpus tag alone, so any other disagreement on those queries would have been attributed to the tolerance row. Both now also require the shape the tolerance produces. - Numeric CLI flags are validated: `--concurrency nope` silently recorded an empty fixture and `--timeout nope` recorded every case as timed out. - A malformed escape in a scoped-match name is an `AdapterError` (adapter gap) rather than an unstructured harness error. - The deferred-mode table carries a fingerprint of the full produced value, so a regression past the 90-character truncation still breaks `conformance:check`. Report totals are unchanged: 190 agree, 33 ledger, 117 new, 9 reference-parser bugs, 0 unclassified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
From the pre-push review (Codex graded leg, round 3): - An unobserved parse — a timeout, an adapter gap, a harness error — no longer reaches the classification rules. Several rules are pinned to witness queries and matched on the query string alone, so a timed-out reference parse for `limit(x)` was reported as the limit-validation divergence and the run could exit 0 having compared nothing. Such a case is now `unclassified`, which fails the run. - The record worker no longer reports a tagged-encoding failure as a Harper parse rejection: only the parse call may fail that way, and an encoding fault is sent as `fatal`. - The adapter validates known fields instead of guessing: a non-list `conditions`, an operator that is neither `and` nor `or`, and a non-boolean `negated`/`descending`/`asArray` now raise `AdapterError` rather than being read as an empty result or silently defaulted. - The fixture is assembled in corpus order rather than worker-completion order. It differed from corpus order at 156 of 349 positions, so every unchanged re-record produced a huge diff that buried real parser changes. - The committed report is no longer overwritten before the run is validated: an unclassified divergence writes `conformance-report.md.actual` and leaves the last good report alone, and the success path renames a temp file into place. - `ReferenceRunner` moved to `conformance/referenceRunner.ts` with a worker-lifecycle test suite that kills, silences and deletes real workers, so the recovery paths added in the previous commit are covered by tests that actually crash something. The end-to-end replay test never exercised them. - Removed a `pnpm-lock.yaml` that a tool on the recording host generated; this repo uses npm, and a second lockfile does not belong in this change. - Comment cleanup: dropped identifier restatements and reviewer-directed notes, and removed the unused `taggedCtor` export. Verified: 111 parser + 97 conformance tests, `typecheck:conformance` clean, `conformance:check` byte-identical, and two `--record` runs at different concurrency produced byte-identical recorded outcomes. Report totals unchanged at 190/33/117/9/0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
Two regressions the fail-closed changes prevent now have tests that would catch them coming back: a pinned-witness query (`limit(x)`) stays unclassified when either parse was not observed, and a known adapter field carrying an unknown shape or value raises AdapterError rather than being silently defaulted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
…from The provenance row read "Reference parser commit", which implied the reference results below it were produced by that commit. They are not: Harper's side is the recorded fixture and is exactly the commit named, but the reference side is parsed live at replay time by whatever is in `src/`. The row is now labelled as the recording-time stamp and the report says which side is which, and how the byte-equality check keeps the two honest. Dirtiness for that row is also measured over `src/` alone — an edit to the harness said nothing about which parser produced the reference results, but it still stamped the fixture "+ uncommitted changes". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
Last finding from the validation round: the adapter read the members it knows and ignored everything else, so a result member Harper grows later would be dropped without a word — and dropping one silently is how the harness would come to report agreement on a query whose meaning had changed. Known members are now an explicit set, split between the ones that carry query semantics and the RequestTarget plumbing (`id`, `isCollection`, `pathname`, `search`) that deliberately does not. Both lists were built from what the recorded fixture actually contains; anything outside them raises AdapterError, which the report surfaces as an adapter gap. Also trimmed the narration out of `referenceRunner.ts` — the rationale for its failure paths belongs in `conformance/design.md`, which has it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
From the validation round (Cursor/Grok): - A worker that missed its startup budget was rejected but not killed, so the process was orphaned and a late `ready` could revive a worker the caller had already recorded as dead — on a restart that meant every later case reported a harness error while a live worker sat there. Every startup failure now kills the child on its way out. - The "replacement cannot start" test had the worker delete its own script inside the parse handler, racing the 300ms timeout: under a loaded test runner the restart wins, finds the file, and the assertion fails. The parent now removes the script after startup, so the condition is set before the parse begins. - A run that could not be classified told the operator to add a rule in `classify.ts` even when the reason was that a parser was never observed, which no rule can ever cover. Those two causes are now reported separately. Also completes the previous commit: the fast path skipped the unknown-member check, so a new result member on a query Harper never parses would still have been dropped silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
The inline version documented that only one parse is ever in flight; the extraction dropped the note and nothing enforced it. Replacing a timed-out worker discards the pending set with it, so a second concurrent parse would be resolved from its own timer as a spurious timeout and would replace the worker again — silently, and precisely for whoever tries to speed the corpus up by not awaiting each parse. It now throws instead, so that attempt fails immediately and says where the parallelism belongs: one runner per concurrent parse. The invariant is recorded in the design note rather than narrated in the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
The guard was derived from the pending map, which a timed-out parse empties before it kills and replaces the worker — so it lapsed for exactly the window it existed to protect. It is now an explicit flag held until the returned promise settles, restart included, and the test observes that window with a worker that is deliberately slow to report ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015EVCBmd9GozJihPeck1CvM
kriszyp
marked this pull request as ready for review
September 2, 2026 23:05
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
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.
Builds the differential conformance harness for RQL 2.0: a corpus of query strings is parsed by both Harper's REST query parser and the reference parser in
src/, Harper's output is mapped into the canonical model (§6) by an adapter, the two are diffed, and every difference is classified. Nothing inspecification/orsrc/is touched — the harness lives in newconformance/,scripts/andtest/conformance/directories.Spec Appendix D deliberately carries no vendor rows; it links to each implementation's own public ledger, which for Harper is HarperFast/harper#2440. This turns that ledger from a hand-written list into an exhaustive, re-runnable one.
What the first run found
349 corpus cases: 190 agree, 159 diverge, 0 unclassified.
src/parser.ts/src/comparators.ts, not in HarperThe full classification, the worked examples and a row-per-case appendix are in
conformance/conformance-report.md. The largest single divergence is that Harper'stypedDecodingnever applies §5.2.2 to a bare token, soa==3parses as the string"3"anda==trueas"true"— 70 cases carry it. The most consequential are the ones that silently change which records match:?a==drops the condition entirely and returns everything;()/[]/prop[]become an empty AND group that matches everything; a duplicatelimit(...)silently wins on last-write;not_lt/not_ge/not_<unknown>are not negations at all.The 6 reference-parser bugs are the reason to read this before the harness is trusted as a Harper scorecard: the reference parser applies the
*wildcard to!=(§5.1.2 restricts it to==), lets aURIErrorescape uncaught where §6.1 requires a 400, refuses to chain onto abetween, accepts a leading|, and is missing thenotEqualalias. They are described in the report; this PR does not fix them —src/is out of scope here.How it runs
Record/replay, per the ruling on the planning gate.
--recordgives every Harper parse its own short-lived process — Harper's parser holds module-global state (lastIndex/currentQuery/queryStringinresources/search.ts), so two parses must never interleave, and a separate process makes that structural instead of a convention. Ordinary runs replay the committed fixture, so CI needs no cross-repo dependency and--checkasserts the report is reproduced byte for byte. Replay uses one persistent reference worker (the reference parser holds no global state), killed and replaced on a timeout.conformance/ledger.jsonis a provenance-stamped cache of harper#2440 with a refresh command; the issue stays canonical and the report prints the snapshot's age.For the human reviewer
Where to look. The diff is ~29k lines, but ~25k of that is two generated artifacts —
conformance/fixtures/harper-parse.json(the recorded raw Harper output) andconformance/conformance-report.md(the result). Everything else is ~4.1k lines, roughly half ofit tests, and the part that carries judgement is
conformance/harperAdapter.tsandconformance/classify.ts.conformance/harperAdapter.tsstates five rules at the top, and the load-bearing one is that values pass through untouched: re-interpreting"3"as3there would erase the single largest divergence the run found. The second is that alias resolution follows Harper's vocabulary, not the spec's —includesmaps toinbecause that is what Harper means by it, so the disagreement with Appendix B is reported rather than absorbed. Both are worth disagreeing with if you read them differently.unclassifiedand fails the run until someone widens the rule.test/conformance/replay.test.tsalso fails on a rule that matches nothing, so a rule made obsolete by a Harper fix has to be deleted with it.(),a===1===2, a barebina=1&b); the verdict goes to Harper and the rationale records the reference parser's own gap. If you would rather see those split into two findings, say so — the shape ofclassify.tssupports it.sort()is classified as a Harper divergence on the reading that §4'scallmakes the argument list optional. §5.6 is ambiguous; it may be thatsortshould require a key, in which case the reference parser is the one that is wrong.elements,containscoercing numbers) and no parse-only harness can witness them. The report says so rather than leaving them looking untested.conformance/fixtures/harper-parse.jsonis Harper's parse output committed into the spec repo. That is what makes CI independent of a
Harper checkout, and it is what the planning ruling chose; the cost is that the fixture is stale
the moment Harper's parser changes, and only a deliberate
conformance:recordnotices. If youwould rather this harness (or just its fixture) lived in
harper, now is the time to say so.(§5.2.2), which touches 70 cases and would otherwise decorate most other divergences. Those
differences are removed from a case before the rules see it, and the case is marked
"+ uninterpreted values". The risk of that choice is that it is one rule's worth of masking
applied globally — if a second cause ever produces exactly the same difference shape, it will be
attributed to this one.
fixture, so it is exactly the commit the report names. The reference side is parsed live at
replay time from
src/, which the provenance row dates only as of the recording — the reportsays so, and
conformance:checkis what keeps the two from drifting apart silently.conformance/referenceRunner.ts's moduledocstring into the design note. It is now two sentences stating the two invariants the class
exists to hold — every request settles, and only one parse is in flight — which is the part a
reader cannot get from the code. The rationale behind them is in
conformance/design.md.npm run buildandnpm run typecheckalready fail onkris/rql-v2—src/*.tsimports carry.tsextensions withoutallowImportingTsExtensions, andtsccannot emit with that flag set.npm run typecheck:conformance(new) does pass and coverssrc/too, so the code is type-clean; the build configuration is the problem, and picking between.jsimport specifiers and a strip-only publish is a call for the repo owner.Verification
npm test— 111 parser tests, 103 conformance tests, all passing. The conformance suite includes an end-to-end replay that asserts the committed report is byte-for-byte what the pipeline produces, negative controls that an unexplained difference staysunclassified, and a check that no classification rule is dead.npm run typecheck:conformance— clean overconformance/,test/conformance/andsrc/.npm run conformance:check— clean. Two--recordruns against the same Harper commit, at different--concurrency, produced byte-identical recorded outcomes (the fixture differs only in itsrecordedAtstamp) and the same 190/33/117/9/0 totals.11a1c489(v5.2.2-14-g11a1c4891) on Node v26.2.0.which was a narrow delta. Across all five the outside legs were Codex (every round),
Cursor/Grok (rounds 1 and 4) and the Harper domain pass (rounds 3 and 4). 21 findings were
raised, 20 fixed — including two hang blockers in the replay supervisor — and the one left open
is the comment nit above. Gemini was unauthenticated on the recording host in every round.
Refs #4
Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=5 @ 97e7b8b
Human-Review-Need: 4 @ 97e7b8b