feat: gate that the two deliveries of a review agent report the same outcomes - #66
Merged
Merged
Conversation
Adds node lib/check-delivery-parity.mjs as a CI step, comparing the two deliveries of each review agent (agents/X.md for Claude Code vs opencode/agents/X.md for OpenCode) on the set of outcomes each side can report — mapped from the rubric's own words (Approve/Clean/Healthy, Warning/Concerns, Block/At-risk/UB-found, INCOMPLETE (not run)), never from the uppercase machine tokens: an OpenCode agent's VERDICT: X line enumerates all four tokens regardless of which the rubric above it can actually reach, so counting tokens would be blind to a port that lost an outcome entirely. It deliberately does not compare anything else: guidance, rubric detail, and worked examples in the two bodies can still diverge with this gate green. nix-reviewer stays unpaired by design (no OpenCode Nix profile). Also updates AGENTS.md's Commands table and the now-false "nothing checks that the two deliveries agree" gotcha, and drops an unused readFileSync import left over from check-delivery-parity.test.mjs's draft.
…ous checks in delivery-parity check-delivery-parity measured the whole agent file, so the YAML description line alone could satisfy every outcome — three of the four pairs on this branch derived their entire measured outcome set from the description line, with zero signal from the body. Strip frontmatter before measuring, so an outcome dropped from the body (e.g. INCOMPLETE gutted to APPROVE) is actually caught; pin it against the real opencode/agents/rust-reviewer.md. Also: the "ok" row's vocabulary tripwire asserted parseVerdict's return value, but its fallthrough is Approve by design, so any word (real or invented) passed — assert recognition via hasVerdictLine instead. The word-boundary test exercised only the \w part of the boundary class, never the hyphen the comment claims it pins — add assertions that actually require it (Semi-Clean, Block-list). And the UNPAIRED_BY_DESIGN staleness check only walked names present in root, so an entry whose root file was deleted was never revisited — iterate the allowlist's own keys too.
UNPAIRED_BY_DESIGN was read as a module-level constant inside checkParity, so every synthetic-map test had to pad in the real nix-reviewer entry just to stay quiet under the staleness loop added in 98f6d1d. A second entry in the real allowlist would have broken every one of those unrelated tests, and the obvious fix — pad again — would have made each test assert less about its own subject with every entry added. checkParity now takes `unpaired = UNPAIRED_BY_DESIGN` as a third parameter. Synthetic-map tests pass their own allowlist (usually {}); tests about the excuse mechanism state their own premise instead of borrowing the real one; the whole-tree test and the CLI keep the default so the real allowlist is still exercised against the real tree. Added a test pinning the property this buys: a second, unrelated allowlist entry must not disturb a test about something else.
…measured The claim that "three of the four pairs derived their entire measured outcome set from the description line alone, with zero signal from the body" was a reviewer's phrasing relayed without verification. Measured directly with outcomesReported over all 8 agent files: every body carries the full outcome set on its own (frontmatter stripped or not), and it is the frontmatter ALONE that reaches the full set in 7 of 8 files (missing only rust-miri's `bad`) — the opposite of the recorded claim. That near- total frontmatter coverage is what actually justifies stripping it: it would mask a body that lost an outcome, exactly as the file's own demonstration (opencode/agents/rust-reviewer.md read as covered with all INCOMPLETE occurrences swapped for APPROVE) already showed. Also names a ceiling the gate did not state: outcomesReported matches a rubric word anywhere in the text, including ordinary prose, so a port that drops a verdict path while still mentioning the word in passing still passes. And switches check-delivery-parity.test.mjs to fileURLToPath, matching the rest of lib/ (new URL(...).pathname leaves percent-encoding and can misresolve on a checkout path with a space or #). Trims the AGENTS.md delivery-parity bullet, which had grown to duplicate the checker's own header comment.
…elivery-parity This branch recorded an unmeasured claim about how many agent files carry a full outcome set TWICE — first "three of four pairs derived their outcomes from the description alone" (false), then bc36bf1's correction landed a second false claim in its place ("every one of 8 bodies carries the full set, description short one outcome for the 8th") when a third measurement found nine files, two short bodies, and frontmatter short two outcomes on both. Hand-written counts rot and get miscopied — demonstrated twice on this one branch. The fix is not better numbers: it is no numbers. The comment now states the PROPERTY (a description: line can mask a body that lost an outcome, verified against the real opencode/agents/rust-reviewer.md) and a copy-pasteable one-liner to re-measure the current table on demand, rather than a count that goes stale the moment either delivery changes. Also fixes a stale "see the demonstration two paragraphs below" cross-reference to name stripFrontmatter directly, adds the accepted (not implemented) engine-to-table ceiling gap, and adds check-delivery-parity.mjs to AGENTS.md's Reality table.
… engines, match fm/body matching Four corrections from a fourth cold review of the delivery-parity gate: fold the "token appears on both sides" clause under the same measurement date as the counts it follows; name both worstVerdict (lib/run-record.mjs) and RANK (opencode/plugin/run-record.mjs) as the parsers this gate's vocabulary answers to, note the tripwire only runs words through the former, and flag Pass as a concrete instance of the ENGINE->TABLE gap; soften the Reality-table claim class to what the checker actually proves; and make the frontmatter column of the diagnostic one-liner use the same whole-word matching as outcomesReported instead of substring matching.
…cises f94df77 inverted which reader the delivery-parity tripwire exercises: it claimed the vocabulary was confirmed against `worstVerdict` (lib/run-record.mjs) and named `RANK` (opencode/plugin/run-record.mjs) as the second, unexercised reader. Measured: the tripwire imports `parseVerdict`/`hasVerdictLine`, not `worstVerdict`; `RANK` is a precedence lookup over already-parsed tokens, not a reader of rubric words at all — `parseVerdict` is. So the exercised engine is the OpenCode one, and the unexercised one is `worstVerdict`. This is the third unmeasured claim recorded as fact on this branch, each time inside a commit meant to correct the previous one. Also: the two engines don't just disagree in principle — running `worstVerdict(['INCOMPLETE (not run)'])` returns `'Warning'`, while `parseVerdict` on the same input reports `'INCOMPLETE (not run)'` itself. Named as a second instance alongside the existing `Pass` gap, not reconciled here. AGENTS.md's Reality-table claim class over-corrected in the same commit: the gate compares outcome buckets, not literal words, so "name the same outcome words" was too strong — reworded to "can name the same set of outcomes".
The fourth prose claim on this branch that the artifact did not support:
the header said "every word claimed here is confirmed against
parseVerdict", but the tripwire pins the ok row (Approve/Clean/Healthy) by
RECOGNITION through hasVerdictLine, not by the returned token — because
parseVerdict's fallthrough is Approve for any input, so asserting the
returned token there would be vacuous (parseVerdict('banana') -> 'Approve'
too). Only the other three rows (concern/bad/incomplete) are pinned by the
direct parseVerdict assertion. Corrected both mentions to name which
mechanism pins which row.
Also: separated the "Pass" framing from the "second-engine-unexercised"
limit it was folded into — Pass instances the ENGINE->TABLE gap two
sentences earlier, not the same gap as the unexercised worstVerdict
engine, even though both are true. And collapsed the frontmatter-masking
rationale, which was stated twice near-verbatim (header and
stripFrontmatter comment), down to one full statement in stripFrontmatter
with a short pointer left in the header, so it can only go stale in one
place.
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.
Every review agent ships twice —
agents/X.mdfor Claude Code,opencode/agents/X.mdfor OpenCode — and the two bodies are maintained by hand. Nothing compared them: each existing gate judges one file's shape in isolation, and shape is exactly what stays valid while meaning drifts apart.That is not hypothetical. A fix shipped for the Claude Code delivery did not reach the port and survived there verbatim — an unrun Miri reporting
Clean— with every gate green, because none was looking at the pair.What the gate compares
The set of outcomes each side can report, canonicalised to
ok/concern/bad/incompletefrom the rubric's own words, in the body, with YAML frontmatter stripped.Three things it deliberately does not key on, each learned by getting it wrong first:
VERDICT: Xline, and that instruction enumerates all four tokens — so a port that lost an outcome entirely still contains the word. Counting tokens is blind to exactly the defect this gate exists for. The Claude Code side carries no such line at all, so comparing the words as written flagged every pair: measuring the transport and calling it the meaning.description:line names the outcomes its agent reports, so measuring the whole file lets the description stand in for a gutted body. Verified on the realopencode/agents/rust-reviewer.md: replace every body occurrence ofINCOMPLETEwithAPPROVEand the unstripped file still reads as fully covered.node -eone-liner that prints the current body-vs-frontmatter table, so a reader re-measures instead of trusting a number.Demonstrated firing
The historical defect reproduced on the real file —
INCOMPLETE (not run)removed from the port's body while the word still appears on five lines. The new gate fails;check-skills,check:typesand the OpenCode frontmatter check all stay green. That is the measure of the hole they could not see.Ceiling — what this does not close
This gate compares only the set of outcomes (
ok/concern/bad/incomplete) each side of a review-agent pair can name, matched against the rubric's own words in the body with YAML frontmatter stripped. It does not compare guidance, rubric detail, wording, or the twodescription:lines — those remain unmeasured and can diverge with CI green. It matches a rubric word anywhere in the body, including in ordinary prose, so a port that deletes a real outcome-reporting branch while leaving the word in a passing mention still reads as covered: the gate proves a word is reachable in the text, not that a working path to report it survives. TheOUTCOMEStable is a closed four-bucket set, and the tripwire proves only table→engine, and only against the readers it exercises (hasVerdictLinefor theokrow,parseVerdictfor the rest, both inopencode/plugin/run-record.mjs); nothing proves engine→table, and the second reader of these verdicts —worstVerdictinlib/run-record.mjs— is not merely unexercised, it disagrees with the one this gate does exercise:worstVerdictmapsINCOMPLETE (not run)toWarning, whileparseVerdictreportsINCOMPLETE (not run)as itself. A fifth verdict added to either reader would fall outside this gate silently:worstVerdictalready matchesPass, which the table does not carry at all — that is the engine→table limit, a different one from the unexercised-second-engine limit above, andPassstays unproven whichever engine is asked.agents/nix-reviewer.mdis excused by name (UNPAIRED_BY_DESIGN, no OpenCode Nix profile) and its body is therefore compared against nothing. And it closes nothing about whether either delivery's agents trigger or produce good findings — the two ceilingsAGENTS.mdalready names.One open item is recorded rather than fixed here: the CLI's guard against a vacuous read (empty
agents/) has no test — replacing it withif (false)leaves all twelve green. It lives in the CLI block, so pinning it needs a spawned process against a fixture directory.Verification
Eight gates green at raw exit code: lint,
node --testoverlib/andopencode/plugin/, check-workflows, check-skills, check-delivery-parity, check-evals,tsc --noEmit --strict, OpenCode frontmatter.Four cold reviews, each with its own mutation pass. Every test in the new file was broken on exactly the property it names and reddened on exactly that test; no test passes regardless of the code. The reviews found, in order: the whole-file measurement (three of four pairs then passing on the description alone), a vacuous tripwire whose
okrow admitted any invented word, a boundary test that never exercised the hyphen it documented, a one-directional staleness check, fixtures coupled to the allowlist's real contents — and twice, a claim recorded as fact without being measured, the second time inside the commit written to correct the first.