Build the result-blind review replay evaluator and corpus contract by shaug · Pull Request #61 · shaug/agent-scripts

shaug · 2026-07-26T19:01:08Z

Closes #50

Summary

Adds the canonical result-blind replay evaluator for the repository-owned review
suite under review-suite/, so later review changes can be measured against live
agent behaviour instead of expected JSON they also author.

Versioned executor protocol. A stdin/stdout JSON contract, one fresh
process per attempt, with schemas for the request and response. Product
specifics stay in the adapter; the core protocol names no runtime.
Failure taxonomy. Eight statuses separate spawn failure, timeout, runtime
failure, oversized output, malformed output, and protocol mismatch from a valid
blocked review and a valid review_result. The first six are evaluation
failures: never graded, never scored clean. Classification judges the reply,
never the packet, so a reviewer that wrongly issues a merge verdict on a
known-incomplete packet is graded as the wrong answer it is.
Result-blind corpus contract. Reviewer-visible artifacts, private
expectations, and provenance live in three separately validated locations. The
payload carries only an opaque c-<hash> case reference, and case ids and
reviewer-visible filenames are rejected when they name a verdict, severity,
failure class, or disposition.
Contamination audit over the complete payload. Structural (exact permitted
key sets) and textual (private strings searched against the payload's real
string values, so non-ASCII characters, newlines, and quotes cannot hide a
leak). Every case is audited before anything launches.
Skill closure. The corpus declares target_skill_dependencies and the
payload ships the whole closure, because review-code-change requires its three
lens skills to be readable and mandates blocked if they are not. Each run
records the closure's membership and digest.
Grading interface. Root causes as requirement, trigger, surface,
consequence, and accepted formulations, handling paraphrase, duplicate symptom,
partial match, ambiguous adjudication, unexpected finding, and accepted
non-finding.
Reporting. Per-attempt records plus an aggregate covering recall,
false-clean, false-positive and false-alarm rates, unique finding contribution,
verdict and finding stability, every failure-status rate, latency, and usage —
each with its denominator. No success threshold is encoded.
Simulation cannot become a baseline. The fixture executor self-reports
simulation, the runner forces the flag whenever that executor is the command,
and --baseline-report refuses to write a file for a simulated run.
Commands. just audit-review-corpus and
just eval-review-suite '<executor command>', with the existing
just test-review-suite extended rather than duplicated.

Preserved behaviour

Existing v1 review behaviour and the v1 packet and result contracts are
unchanged. review-suite/CONTRACT.md, both schemas, validate.py, the lens
prompts, and review-suite/fixtures/ are untouched, the bundled skill mirrors
remain byte-identical, and no file under skills/ is modified.

just eval-review-suite is the only command that may cost money and is reachable
from no quality gate: just test, just lint, and just check never launch a
paid runtime.

Validation

At head b605051, on the rebased base 861dd04:

just format — All checks passed!, 77 files left unchanged
just lint — pass (ruff, mdformat, skills-ref validate × 8 skills, plugin validation)
just test — pass, Ran 178 tests
just test-review-suite — pass, Ran 178 tests
just audit-review-corpus — corpus audit passed
just sync-contracts then git status — clean, so canonical and bundled contracts are byte-identical

CI pins ruff==0.15.22 while the local ruff is 0.11.12, so the pinned version
was run explicitly: check, check --select I,RUF022, and format --check all
clean.

A real-runtime smoke evaluation ran through the bundled Claude adapter: exit 0, 6
valid protocol outcomes, 0 evaluation failures, model identity and usage recorded
on every attempt. That run proves the protocol end to end and nothing else —
it is not evidence of reviewer capability, quality, or cost. Its verdicts are
demonstrably not repeatable: the incomplete-evidence case flipped between two
consecutive runs of the same configuration.

Review

review-code-change returned aggregate clean bound to head b605051 and base
861dd04, with all three lenses run fresh in an isolated read-only context:
solution simplicity clean, correctness clean, code simplicity clean.

Five defer findings are preserved rather than applied, as the contract requires:

The aggregate has no verdict-accuracy figure, so a blocked-expected case
enters no quality denominator.
The command-contract test invokes just eval-review-suite to satisfy the
argument/failure criterion; no money is spent, but the literal "never called by
just test" constraint is met only incidentally.
Pre-flight orchestration is duplicated between the runner and the audit
entrypoint.
test_every_attempt_runs_in_its_own_process asserts executor identity rather
than process freshness.
Test option defaults are restated across five call sites.
expectation.schema.json requires a per-root-cause severity that no metric
consumes.

Scope handed onward

Corpus curation, grader calibration, and the frozen v1 baseline are deliberately
out of scope. Whoever owns them must decide which target a scored corpus measures,
which strata it contains, and the cost envelope to preregister — all of which
follow from the closure's size and none of which should be extrapolated from the
smoke run here. The shipped formulations were written before any real run and were
not tuned afterwards, which is why recall is 0.0 while verdicts were mostly right.

Merge

Merge is intentionally not performed here; this PR is left open at its
complete non-merge gate for human inspection of the contamination and grading
fixes. Note that the Closes #50 reference above means GitHub will close issue
#50 automatically when this PR is merged.

🤖 Generated with Claude Code

- Add the canonical replay evaluator under `review-suite/`: a versioned stdin/stdout executor protocol, a result-blind corpus contract, a root-cause grading interface, and machine-readable per-attempt and aggregate reports - Start a fresh process per attempt and classify every outcome into one of eight statuses so spawn, timeout, runtime, oversized-output, malformed, and protocol failures are reported separately from a valid review or a valid blocked result - Separate reviewer-visible artifacts, private expectations, and provenance into three validated locations, pass only an opaque case reference to the executor, and audit the complete payload structurally and textually - Ship a deterministic fixture executor marked as a simulation that cannot produce a baseline report, plus a documented Claude Code real-runtime adapter - Add six synthetic cases covering paraphrase, duplicate symptom, partial match, ambiguous adjudication, unexpected finding, accepted non-finding, and a valid blocked result - Add `just audit-review-corpus` and `just eval-review-suite '<executor>'`, and keep the paid path out of `test`, `lint`, and `check` The existing review tests prove schemas, ordering, aggregation, and prerecorded outcomes. They never execute the review suite through a real agent runtime, so a later prompt or contract change could assert its own quality through expected JSON it also authored. This supplies the measurement infrastructure first, with contamination controls and a failure taxonomy, so later work is constrained by evidence rather than by its own fixtures. Existing v1 review behaviour and the v1 packet and result contracts are unchanged: `review-suite/CONTRACT.md`, both schemas, `validate.py`, the lens prompts, and `review-suite/fixtures/` are untouched, and the bundled skill copies remain byte-identical. Corpus curation, grader calibration, and the frozen v1 baseline are deliberately out of scope. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Judge only the executor's reply in `classify_response`, never the packet, so a reviewer that wrongly issues a merge verdict on a deliberately incomplete packet is graded as the wrong answer instead of being reported as `malformed_output` - Run the textual blindness check against the payload's real string values instead of its `json.dumps` form, so a private expectation containing a non-ASCII character, a line break, or a quotation mark can no longer leak undetected; object keys and re-wrapped text are covered too - Compute verdict and finding stability over every attempt that produced a valid review, `blocked` ones included, and publish the stability denominator beside each figure - Add regression tests at each demonstrated triggering condition, and document all three rules in `review-suite/evals/README.md` ## Why The initial `review-code-change` pass on this branch found three material defects, each of which let the evaluator report the wrong thing: 1. `validate_pair` only suppresses blockable packet errors when the observed verdict is `blocked`, so for the one case class whose whole purpose is to measure "does the reviewer refuse a verdict on incomplete evidence", a wrong answer was charged to the executor as a harness failure and dropped from grading entirely. 2. `json.dumps` defaults to `ensure_ascii=True`, so every non-ASCII character became `\uXXXX`, newlines became `\n`, and quotes became `\"`. Comparing raw private strings against that serialization silently never matched, making the contamination gate vacuous for exactly the prose humans write. 3. Stability was derived from graded attempts only, so a reviewer alternating between `blocked` and a merge verdict was reported at 1.0 - maximum stability for maximally unstable behaviour - with no denominator to reveal the dropped attempts. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Enforce the reviewer-prompt contamination rule from `corpus.load_corpus`, so the runner inherits it instead of only `just audit-review-corpus` having it - Parse the justfile's parameters, dependencies, and body as three separate fields in the command-contract test, and assert the dependency closure really reaches every transitive recipe - Distinguish an ungraded `blocked` attempt from a graded attempt that matched nothing when computing finding stability - Record both preceding commits in `CHANGELOG.md` with the required SHA backfill ## Why The second `review-code-change` pass on this branch found four gating defects, each one a gate that reported success without checking what it claimed: 1. `prompt_errors` was only called from `audit_corpus`, so a corpus whose shared reviewer prompt hinted at the expected verdict was rejected by `just audit-review-corpus` and accepted by `just eval-review-suite` - the one command that spends money, and the one whose output #58 will treat as behavioural evidence. Enforcing it in the loader gives both callers one gate. 2. The no-paid-runtime guard stored each recipe's dependencies and body in one string and then re-split it, which always yielded an empty dependency list for `test: test-plugins`. The closure therefore never left the three seed recipes, and injecting the paid runner into `test-plugins` left the test green. It now fails, and a second test pins the closure's membership so it cannot silently go inert again. 3. Finding stability gave a `blocked` attempt the empty matched set, making it indistinguishable from an answer that found nothing: a reviewer that blocked half its runs and found nothing on the rest reported 1.0. Verdict stability already reported 0.5 for that pair. 4. The fix commit changed behaviour without recording itself in the changelog or backfilling its predecessor's SHA, which `AGENTS.md` requires and which becomes unrecoverable once a further commit lands. One deferred finding is preserved rather than applied: `PROMPT_FORBIDDEN_WORDS` matches `changes_required` and `strong_recommendation` only in their underscored spellings, so the same names written as ordinary prose are not rejected. Nothing is contaminated today and the shipped prompt is pinned by test; the gap matters when #58 authors further reviewer prompts. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Pass the target skill's whole reviewer-visible text - `SKILL.md` plus every Markdown file under its `references/`, excluding the bundled review-suite mirror already supplied from its canonical location - and digest all of it, so `target_skill_digest` changes whenever any evaluated skill text does - Audit every case for contamination before launching any executor, keeping the per-request check as defence in depth - Contaminate the last declared case in the ordering test, and assert with a launch-counting executor that no process started - Replace the evaluator README's smoke-run summary with the measured numbers ## Why The third `review-code-change` pass found three gating defects: 1. `target_skill_prompt` read only `SKILL.md`, but `review-code-change` instructs its reviewer to read `references/orchestration-protocol.md`, which owns the lens decision table, deduplication, and verdict aggregation. The executor is told to reason only from what it is given, so the evaluator was measuring a reviewer working from a partial definition of its own job, and the digest that exists to pin the evaluated skill across baseline strata was blind to that file entirely. This is not theoretical: on the same corpus and adapter, the partial-text reviewer answered `changes_required` on the incomplete-evidence case, and the complete-text reviewer correctly answered `blocked`. 2. `audit_request` ran only inside the launch loop, so a contaminated last case billed a real review for every earlier case and then discarded the whole run, contradicting the rule that a contaminated case fails before executor launch. The guarding test asserted that property while contaminating the *first* declared case, so it passed vacuously. 3. The README claimed the smoke run "produced correct verdicts on every case" and that "every finding was referred for adjudication". Both were false of the run it cited at the time, and #58 inherits that text as the statement of what calibration must cover. It now reports the measured attempt count, verdict matches, adjudication count, rates with denominators, cost, and latency. The deferred finding about `expectation.schema.json` requiring a per-root-cause `severity` that no metric consumes is preserved, not applied, and is now recorded in the README's limitations for the corpus work that owns grader calibration. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Identify the recorded smoke evaluation by suite commit, corpus version, and grader version, and state that it is one observation rather than a baseline - Report the counts from that exact run, and separate the stable part of the result from the variable part ## Why The previous revision reported a finding count from an earlier run. Re-running the same configuration at the committed head produced identical per-case verdicts but seven findings instead of six, so an unpinned count invites the same documentation drift the preceding commit fixed. Naming the run and distinguishing the reproducible verdicts from the variable counts and latency keeps the statement true without needing an update after every run. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Let the corpus declare `target_skill_dependencies`, and ship that whole closure in every payload: each skill's `SKILL.md` plus its `references/*.md`, excluding the bundled review-suite mirror already supplied canonically - Label every section of the rendered prompt, including the target's own `SKILL.md`, so a multi-skill payload says which skill is the target - Fail closed when a declared skill is missing, duplicated, or is the target - Total every input-side token field, including cache creation and cache read, instead of only the uncached residue - Resolve model identity from the `modelUsage` mapping, fall back to `--model`, and return `runtime_failure` rather than recording an attempt that cannot name the model that answered - Add regression tests at each triggering condition and document all three rules ## Why `review-code-change` instructs its reviewer to verify that `review-solution-simplicity`, `review-correctness`, and `review-code-simplicity` are available and readable, and to return an aggregate `blocked` result naming any that are missing. The payload carried none of them, and the executor is told to reason only from what it is given, so the evaluator measured a reviewer working from a partial definition of its own job — and inverted its own metric: a reviewer that correctly refused for missing dependencies scored wrong on all five cases expecting a merge verdict, so recall moved the wrong way as the reviewer became more compliant. With the closure supplied, the same corpus and adapter now answer `blocked` on the incomplete-evidence case where the truncated payload answered `changes_required`. The two usage defects would both corrupt a frozen cost envelope. Headless output reports no top-level `model` string — `modelUsage` is a mapping keyed by model id — so treating it as a string silently dropped model identity from every attempt, leaving a stratum that cannot say which model answered. And under prompt caching the uncached `input_tokens` residue is negligible: this corpus reported 18 input tokens across six attempts while really sending 192,430, an error of four orders of magnitude published as complete. Corpus composition remains out of scope: which target a scored corpus measures, which strata it contains, and the cost envelope that follows from the closure's size are not decided here. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Report the smoke evaluation recorded at the commit that introduced the skill closure, with its target digest, model, token totals, and cost - State plainly that the incomplete-evidence case varied between two runs of the same configuration, and that one run per case cannot separate capability from variance - Stop claiming the closure makes any single case answer correctly; it removes a measurement inversion, which is a different claim - Note the closure's size as the cost floor a scored corpus inherits ## Why The previous revision of this section asserted that every verdict matched and that supplying the full closure makes the incomplete-evidence case answer `blocked`. Re-running the same configuration at the committed head returned a merge verdict on that case instead, so both claims were stronger than the evidence. That case is the one testing refusal on incomplete evidence, which is the behaviour this evaluator most needs to measure, so overstating its stability is the worst place to be imprecise. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Record the target skill, its declared dependencies, and the exact closure document list in the run configuration, and repeat the target, dependencies, and digest on every attempt record - Frame the recorded smoke evaluation as protocol proof only, and state that it is not evidence of reviewer capability, quality, or cost ## Why Two acceptance criteria were tightened after the previous cycle. Payload assembly already shipped the declared closure, but only its digest was recorded. A digest proves two runs sent the same text; it cannot say what that text was. A stratum has to be able to state which skills it evaluated, so the closure's membership is now recorded alongside the digest, and each attempt is self-describing without its report. The smoke evaluation is also easy to over-read. Its verdicts are demonstrably not repeatable - the incomplete-evidence case flipped between two consecutive runs of the same configuration - and one run per case cannot support a cost envelope. The documentation now says so at the top of the section rather than leaving the numbers to imply more than they show. The rendered payload is unchanged by this commit: the closure digest is still `9b2805f14cdd6158`, so the recorded real-runtime evidence continues to describe this head's executable behaviour. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary - Catch `OSError` when probing for `just`, so an absent executable skips `RecipeExecutionTests` instead of erroring `setUpClass` ## Why CI does not install `just`, and `subprocess.run` raises `FileNotFoundError` for a missing executable rather than returning a nonzero status. The guard only checked the return code, so the intended skip never happened and the whole review-suite run failed with `FileNotFoundError: [Errno 2] No such file or directory: 'just'`. Verified by removing `just` from `PATH` and rerunning the suite: 174 tests pass with exactly one skip, which is the condition CI runs under. Co-Authored-By: Claude <noreply@anthropic.com>

shaug and others added 9 commits July 26, 2026 10:58

shaug mentioned this pull request Jul 26, 2026

Curate the scored corpus and capture the frozen v1 review baseline #58

Closed

21 tasks

shaug merged commit 16560d8 into main Jul 27, 2026
1 check passed

shaug deleted the scott/50-review-replay-evaluator branch July 27, 2026 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build the result-blind review replay evaluator and corpus contract - #61

Build the result-blind review replay evaluator and corpus contract#61
shaug merged 9 commits into
mainfrom
scott/50-review-replay-evaluator

shaug commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

shaug commented Jul 26, 2026

Summary

Preserved behaviour

Validation

Review

Scope handed onward

Merge

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant