From 8f0e9d646ec4e959d7adc7448f5fc7a82f4334d8 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 07:09:05 -0700 Subject: [PATCH 1/9] feat: add the result-blind review replay evaluator - 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 ''`, 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 --- .gitignore | 1 + CHANGELOG.md | 6 +- README.md | 52 ++- justfile | 12 + review-suite/evals/README.md | 197 +++++++++ .../evals/contracts/corpus.schema.json | 26 ++ .../contracts/executor-request.schema.json | 58 +++ .../contracts/executor-response.schema.json | 41 ++ .../evals/contracts/expectation.schema.json | 68 +++ .../evals/contracts/provenance.schema.json | 32 ++ review-suite/evals/corpus/corpus.json | 14 + .../expectations/catalog-import-feed.json | 8 + .../expectations/invoice-export-batch.json | 17 + .../expectations/ledger-charge-limit.json | 22 + .../expectations/pricing-tier-lookup.json | 35 ++ .../expectations/session-token-refresh.json | 34 ++ .../expectations/webhook-retry-queue.json | 22 + .../provenance/catalog-import-feed.json | 9 + .../provenance/invoice-export-batch.json | 9 + .../provenance/ledger-charge-limit.json | 9 + .../provenance/pricing-tier-lookup.json | 9 + .../provenance/session-token-refresh.json | 9 + .../provenance/webhook-retry-queue.json | 9 + review-suite/evals/corpus/reviewer/PROMPT.md | 7 + .../reviewer/catalog-import-feed/packet.json | 48 +++ .../reviewer/invoice-export-batch/packet.json | 70 +++ .../reviewer/ledger-charge-limit/packet.json | 66 +++ .../reviewer/pricing-tier-lookup/packet.json | 71 ++++ .../session-token-refresh/packet.json | 75 ++++ .../reviewer/webhook-retry-queue/packet.json | 78 ++++ review-suite/scripts/evals/__init__.py | 1 + review-suite/scripts/evals/audit_corpus.py | 91 ++++ review-suite/scripts/evals/claude_executor.py | 171 ++++++++ review-suite/scripts/evals/corpus.py | 274 ++++++++++++ .../scripts/evals/fixture_executor.py | 319 ++++++++++++++ review-suite/scripts/evals/grader.py | 240 +++++++++++ review-suite/scripts/evals/protocol.py | 340 +++++++++++++++ review-suite/scripts/evals/report.py | 205 +++++++++ review-suite/scripts/evals/runner.py | 357 ++++++++++++++++ .../scripts/tests/test_eval_commands.py | 174 ++++++++ .../scripts/tests/test_eval_corpus.py | 288 +++++++++++++ .../scripts/tests/test_eval_grader.py | 306 +++++++++++++ .../scripts/tests/test_eval_protocol.py | 350 +++++++++++++++ .../scripts/tests/test_eval_report.py | 285 +++++++++++++ .../scripts/tests/test_eval_runner.py | 401 ++++++++++++++++++ 45 files changed, 4913 insertions(+), 3 deletions(-) create mode 100644 review-suite/evals/README.md create mode 100644 review-suite/evals/contracts/corpus.schema.json create mode 100644 review-suite/evals/contracts/executor-request.schema.json create mode 100644 review-suite/evals/contracts/executor-response.schema.json create mode 100644 review-suite/evals/contracts/expectation.schema.json create mode 100644 review-suite/evals/contracts/provenance.schema.json create mode 100644 review-suite/evals/corpus/corpus.json create mode 100644 review-suite/evals/corpus/private/expectations/catalog-import-feed.json create mode 100644 review-suite/evals/corpus/private/expectations/invoice-export-batch.json create mode 100644 review-suite/evals/corpus/private/expectations/ledger-charge-limit.json create mode 100644 review-suite/evals/corpus/private/expectations/pricing-tier-lookup.json create mode 100644 review-suite/evals/corpus/private/expectations/session-token-refresh.json create mode 100644 review-suite/evals/corpus/private/expectations/webhook-retry-queue.json create mode 100644 review-suite/evals/corpus/private/provenance/catalog-import-feed.json create mode 100644 review-suite/evals/corpus/private/provenance/invoice-export-batch.json create mode 100644 review-suite/evals/corpus/private/provenance/ledger-charge-limit.json create mode 100644 review-suite/evals/corpus/private/provenance/pricing-tier-lookup.json create mode 100644 review-suite/evals/corpus/private/provenance/session-token-refresh.json create mode 100644 review-suite/evals/corpus/private/provenance/webhook-retry-queue.json create mode 100644 review-suite/evals/corpus/reviewer/PROMPT.md create mode 100644 review-suite/evals/corpus/reviewer/catalog-import-feed/packet.json create mode 100644 review-suite/evals/corpus/reviewer/invoice-export-batch/packet.json create mode 100644 review-suite/evals/corpus/reviewer/ledger-charge-limit/packet.json create mode 100644 review-suite/evals/corpus/reviewer/pricing-tier-lookup/packet.json create mode 100644 review-suite/evals/corpus/reviewer/session-token-refresh/packet.json create mode 100644 review-suite/evals/corpus/reviewer/webhook-retry-queue/packet.json create mode 100644 review-suite/scripts/evals/__init__.py create mode 100644 review-suite/scripts/evals/audit_corpus.py create mode 100644 review-suite/scripts/evals/claude_executor.py create mode 100644 review-suite/scripts/evals/corpus.py create mode 100644 review-suite/scripts/evals/fixture_executor.py create mode 100644 review-suite/scripts/evals/grader.py create mode 100644 review-suite/scripts/evals/protocol.py create mode 100644 review-suite/scripts/evals/report.py create mode 100644 review-suite/scripts/evals/runner.py create mode 100644 review-suite/scripts/tests/test_eval_commands.py create mode 100644 review-suite/scripts/tests/test_eval_corpus.py create mode 100644 review-suite/scripts/tests/test_eval_grader.py create mode 100644 review-suite/scripts/tests/test_eval_protocol.py create mode 100644 review-suite/scripts/tests/test_eval_report.py create mode 100644 review-suite/scripts/tests/test_eval_runner.py diff --git a/.gitignore b/.gitignore index fc793c6..9fe6e28 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ __pycache__/ .DS_Store .venv/ .tools/agentskills/ +review-suite/evals/artifacts/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba1175..b7c085b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,14 @@ summary: Chronological history of repository and skill changes. # Changelog +## 2026-07-26 — Added the result-blind review replay evaluator + +- feat: add the result-blind review replay evaluator + ## 2026-07-25 — Added coordinator-neutral delegated ticket execution - fix: pin CI to the established Ruff rule set so dependency drift cannot - redefine the repository-wide lint gate + redefine the repository-wide lint gate (`901dc3596207a88b6c8edcf548b5be3151ca7ab2`) - feat: add a versioned delegated-execution contract for `implement-ticket` (`b53efa674e929c181bdaac63ff0306cb756386db`) diff --git a/README.md b/README.md index a09349f..e009a63 100644 --- a/README.md +++ b/README.md @@ -54,8 +54,9 @@ causes the workflow to fail closed. ## Repository Layout - `skills/` — skill folders, each containing a `SKILL.md` and bundled resources -- `review-suite/` — canonical code-review contracts, validators, and raw - evaluation fixtures shared by repository-owned review skills +- `review-suite/` — canonical code-review contracts, validators, raw evaluation + fixtures, and the result-blind replay evaluator shared by repository-owned + review skills - `justfile` — common tasks for testing, validation, and formatting Current reusable agent skills: @@ -175,6 +176,53 @@ observations with `--output-dir`. A Claude Code headless adapter is bundled: just eval-implement-ticket-claude ``` +### Result-blind review replay evaluation + +`review-suite/evals/` is the canonical evaluator that measures what the review +skills actually do when a real agent runtime executes them repeatedly, rather +than asserting quality through expected JSON. It changes no review behaviour. +See [its README](review-suite/evals/README.md) for the protocol, the corpus +contract, and the grading interface. + +Three commands cover it: + +```bash +just test-review-suite # deterministic tests, no runtime +just audit-review-corpus # corpus integrity, no runtime +just eval-review-suite '' # the only one that may cost money +``` + +`just test` includes the deterministic evaluator tests and never launches a paid +runtime. `just eval-review-suite` is deliberately absent from `test`, `lint`, +and `check`. The bundled real-runtime adapter needs the `claude` CLI on `PATH`: + +```bash +just eval-review-suite "python3 review-suite/scripts/evals/claude_executor.py" +``` + +Each attempt starts a fresh process and receives one result-blind JSON request: +the target skill text, the raw review packet, the review contracts, and public +run identity. Expected findings, private grader labels, provenance, and even the +case name stay out of the payload, and `just audit-review-corpus` proves it by +inspecting the complete payload every case would produce. Spawn, timeout, +runtime, oversized-output, malformed, and protocol failures are reported +separately from a valid review and are never scored as clean. + +The bundled `fixture_executor.py` is a deterministic simulation of a compliant +reviewer, not a model. Its runs are marked `simulation`, so no baseline report +can be produced from them. Run the runner directly for repeated attempts, +per-attempt records, and the aggregate report: + +```bash +python3 review-suite/scripts/evals/runner.py \ + --executor "python3 review-suite/scripts/evals/claude_executor.py" \ + --runs 5 --report-out out/report.json +``` + +Corpus curation, grader calibration, and the frozen v1 baseline are separate +follow-up work; this evaluator supplies only the synthetic cases needed to prove +the protocol. + ## Prerequisites - Python 3.11+ diff --git a/justfile b/justfile index adf3ce2..2cef535 100644 --- a/justfile +++ b/justfile @@ -41,6 +41,18 @@ test: test-plugins test-review-suite: python3 -m unittest discover -s review-suite/scripts/tests -p 'test_*.py' +# Validate the replay corpus: schemas, cross-field expectation semantics, +# reviewer/private separation, provenance shape, outcome-revealing names, and +# the complete executor payload. Never launches a model. +audit-review-corpus: + python3 review-suite/scripts/evals/audit_corpus.py + +# Result-blind replay evaluation through an explicit real-runtime executor. +# Deliberately excluded from `test`, `lint`, and `check`: this is the only +# review-suite command that may spend money. +eval-review-suite executor: + python3 review-suite/scripts/evals/runner.py --executor "{{executor}}" + test-plugins: python3 -m unittest discover -s scripts/tests -p 'test_*.py' diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md new file mode 100644 index 0000000..77cad6c --- /dev/null +++ b/review-suite/evals/README.md @@ -0,0 +1,197 @@ +# Result-blind review replay evaluation + +This directory is the canonical evaluator for the repository-owned review suite. +It measures what the review skills actually do when a real agent runtime +executes them repeatedly, instead of asserting quality through expected JSON +that the same change also authored. + +Review skills may reference this evaluator. They must not fork it. + +The evaluator changes no review behaviour. It does not read, modify, or extend +`../CONTRACT.md`, the packet and result schemas, the lens prompts, or the +existing `../fixtures/` corpus. + +## Layout + +```text +review-suite/evals/ +├── contracts/ versioned evaluator schemas +│ ├── executor-request.schema.json the complete result-blind payload +│ ├── executor-response.schema.json the single reply an executor returns +│ ├── corpus.schema.json corpus version metadata and case list +│ ├── expectation.schema.json private material root causes +│ └── provenance.schema.json origin and retention authority +├── corpus/ +│ ├── corpus.json versions plus the case identifiers +│ ├── reviewer/PROMPT.md shared reviewer instructions +│ ├── reviewer//packet.json reviewer-visible artifacts +│ ├── private/expectations/.json +│ └── private/provenance/.json +└── artifacts/ opt-in captured output, not in git + +review-suite/scripts/evals/ +├── protocol.py versioned request/response contract, failure taxonomy +├── corpus.py corpus loading, separation, and naming rules +├── grader.py root-cause grading interface and reference grader +├── report.py per-attempt records and the aggregate report +├── runner.py fresh-process replay driver +├── audit_corpus.py `just audit-review-corpus` +├── fixture_executor.py deterministic simulation, never a baseline +└── claude_executor.py documented real-runtime adapter +``` + +## Commands + +```bash +just test-review-suite # deterministic tests, no runtime +just audit-review-corpus # corpus integrity, no runtime +just eval-review-suite '' # the only command that may cost money +``` + +`just test` includes the deterministic evaluator tests and never launches a paid +runtime. `just eval-review-suite` is deliberately absent from `test`, `lint`, +and `check`. + +The bundled real-runtime adapter needs the `claude` CLI on `PATH`: + +```bash +just eval-review-suite "python3 review-suite/scripts/evals/claude_executor.py" +``` + +The runner accepts `--runs N` for repeated attempts, `--timeout`, +`--max-output-bytes`, `--artifact-dir`, `--attempts-out`, `--report-out`, and +`--baseline-report`. Run it directly when you need those options: + +```bash +python3 review-suite/scripts/evals/runner.py \ + --executor "python3 review-suite/scripts/evals/claude_executor.py" \ + --runs 5 --report-out out/report.json +``` + +## Executor protocol + +One attempt is one fresh process. The runner writes a single JSON request to +stdin and reads a single JSON response from stdout. `protocol_version` is `1.0`. + +The request carries the target skill name, a digest pinning the exact skill +text, the reviewer-visible instructions, the raw review packet, the bundled +review contracts, and public run metadata: an opaque case reference, the run +number, the suite commit, the corpus version, the exact candidate and +comparison-base identity, and a start timestamp. + +The request never carries expected findings, private labels, prior conclusions, +suspected issues, implementation transcripts, or the case name. + +The response reports `outcome` as `review_result`, `blocked`, or +`runtime_failure`, plus `simulation`, executor identity, and optional usage. The +runner classifies each attempt into exactly one status: + +| status | meaning | +| ------------------- | ------------------------------------------------- | +| `spawn_failure` | the executor command could not start | +| `timeout` | the executor outlived `--timeout` | +| `runtime_failure` | non-zero exit, or a self-reported runtime failure | +| `output_too_large` | stdout exceeded `--max-output-bytes` | +| `malformed_output` | unparseable, schema-invalid, or wrongly bound | +| `protocol_mismatch` | the reply declared another protocol version | +| `blocked` | a valid review that refuses a merge verdict | +| `review_result` | a valid, candidate-bound review | + +The first six are evaluation failures. They are reported separately, are never +graded, and can never appear as a clean review. A malformed review result is an +evaluation failure, not a missed finding. + +Only `review_result` attempts are graded. The runner's exit status reports +evaluation integrity, never review quality: `0` when every attempt produced a +valid outcome, `1` when any attempt failed the protocol, `2` when the +configuration or corpus was rejected before any launch. + +## Corpus and expectation contract + +Three separately validated locations keep the reviewer away from the answer: + +- `reviewer/` holds only the shared prompt and each case's `packet.json`; +- `private/expectations/` holds the material root causes and accepted + non-findings; and +- `private/provenance/` holds origin, retention authority, and sanitization. + +A case identifier and every reviewer-visible filename must describe the subject +matter, never the outcome. `corpus.py` rejects names containing verdict, +severity, failure-class, or disposition words. The payload additionally carries +only an opaque `c-` case reference, so even a badly named case cannot leak +through a request. + +`just audit-review-corpus` builds the complete request each case would produce +and rejects it both structurally, by permitting an exact key set, and textually, +by searching for private expectation and provenance text. Three fields are +deliberately exempt from the text search because banning them would ban the +packet or the contracts themselves: a root cause's `requirement` normally +restates a reviewer-visible acceptance criterion, its `surface` is a +reviewer-visible code location, and `expected_verdict`, `severity`, and `origin` +are closed public vocabularies that the bundled contracts spell out in full. +Those three stay private structurally instead, because the expectation file is +never part of a request. + +Loading fails closed. A missing expectation, a missing provenance record, a +schema violation, an orphaned file, a packet whose validity disagrees with its +expectation, or a grader-version mismatch is an error before any process starts. + +## Grading interface + +A reviewer is graded on material root causes, not prose. Each root cause states +its requirement, triggering condition, affected surface, material consequence, +severity, and the formulations a competent reviewer may use. An observed finding +matches when it points at the affected surface *and* describes the root cause in +an accepted way. One signal alone is a partial match, and a finding that fully +matches two root causes is ambiguous; both are referred for adjudication rather +than silently scored either way. `accepted_non_findings` describe observations +that are tolerated without counting as false positives. + +The reference grader is protocol proof. It is not calibrated, and its output is +not a v1 score. + +## Simulation and baselines + +`fixture_executor.py` hand-codes the review a compliant reviewer would return +for each synthetic packet, so the protocol, failure taxonomy, grading interface, +and reporting are deterministically testable for free. It is a simulation, not a +model evaluation, and it cannot detect a model misreading the review contract. + +Two independent controls keep a simulation out of any baseline: every fixture +response sets `"simulation": true`, and the runner forces the flag whenever the +bundled fixture executor is the command, regardless of what the reply claims. An +aggregate report whose `simulation` is true reports `baseline_eligible: false`, +and `--baseline-report` refuses to write a file for it. + +## Reporting + +`--attempts-out` writes one JSON record per attempt. `--report-out` writes the +aggregate report, which represents material-finding recall, false-clean rate, +false-positive rate, false-alarm rate, unique finding contribution, verdict and +finding stability, every failure-status rate, latency, and whatever usage and +cost the executor reported. Every rate is published with its denominator so a +small run count is not mistaken for a precise capability measurement. + +The report encodes no success threshold and returns no pass/fail judgement. + +`--artifact-dir` retains raw executor output. It is opt-in, and +`review-suite/evals/artifacts/` is excluded from git. + +## Scope and known limitations + +This directory proves the protocol, the grading interface, the contamination +controls, and the failure taxonomy. It deliberately does not curate a +representative scored corpus, calibrate the grader, or capture a v1 baseline. + +Two limitations are worth stating plainly for whoever curates that corpus: + +- Surface matching is file-level, because a private root cause names a function + while a finding names a line. A finding in the right file that the grader does + not recognize is therefore reported as a partial match needing adjudication, + not as a false positive. Calibration must decide how those are scored. +- The shipped formulations were written before any real run and were not tuned + afterwards. A smoke evaluation through the bundled Claude adapter produced + correct verdicts on every case while matching no formulation, so every finding + was referred for adjudication. That is the conservative behaviour this + interface is meant to have, and it is direct evidence that grader calibration + is required before any recall number means anything. diff --git a/review-suite/evals/contracts/corpus.schema.json b/review-suite/evals/contracts/corpus.schema.json new file mode 100644 index 0000000..d99bcba --- /dev/null +++ b/review-suite/evals/contracts/corpus.schema.json @@ -0,0 +1,26 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/corpus.schema.json", + "title": "Review replay corpus index", + "description": "Version metadata and the case-identifier list. This index is deliberately outcome-free so it can sit beside the reviewer-visible artifacts.", + "type": "object", + "additionalProperties": false, + "required": [ + "corpus_version", + "protocol_version", + "grader_version", + "target_skill", + "cases" + ], + "properties": { + "corpus_version": {"type": "string", "minLength": 1}, + "protocol_version": {"const": "1.0"}, + "grader_version": {"type": "string", "minLength": 1}, + "target_skill": {"type": "string", "minLength": 1}, + "cases": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$"} + } + } +} diff --git a/review-suite/evals/contracts/executor-request.schema.json b/review-suite/evals/contracts/executor-request.schema.json new file mode 100644 index 0000000..5373c0c --- /dev/null +++ b/review-suite/evals/contracts/executor-request.schema.json @@ -0,0 +1,58 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/executor-request.schema.json", + "title": "Review replay executor request", + "description": "The complete result-blind payload written to one fresh executor process on stdin. Every property is reviewer-visible by contract; nothing here may reveal an expected outcome.", + "type": "object", + "additionalProperties": false, + "required": [ + "protocol_version", + "target_skill", + "target_skill_digest", + "skill_prompt", + "contract_documents", + "instructions", + "packet", + "run" + ], + "properties": { + "protocol_version": {"const": "1.0"}, + "target_skill": {"type": "string", "minLength": 1}, + "target_skill_digest": {"type": "string", "pattern": "^[0-9a-f]{16}$"}, + "skill_prompt": {"type": "string", "minLength": 1}, + "contract_documents": {"type": "object"}, + "instructions": {"type": "string", "minLength": 1}, + "packet": {"type": "object"}, + "run": { + "type": "object", + "additionalProperties": false, + "required": [ + "case_ref", + "run_number", + "suite_commit", + "corpus_version", + "candidate", + "started_at" + ], + "properties": { + "case_ref": {"type": "string", "pattern": "^c-[0-9a-f]{8}$"}, + "run_number": {}, + "suite_commit": {"type": "string", "minLength": 1}, + "corpus_version": {"type": "string", "minLength": 1}, + "started_at": {"type": "string", "minLength": 1}, + "candidate": { + "type": "object", + "additionalProperties": false, + "required": ["head_sha", "comparison_base_sha"], + "properties": { + "head_sha": {"type": "string", "pattern": "^[0-9a-f]{40}$"}, + "comparison_base_sha": { + "type": "string", + "pattern": "^[0-9a-f]{40}$" + } + } + } + } + } + } +} diff --git a/review-suite/evals/contracts/executor-response.schema.json b/review-suite/evals/contracts/executor-response.schema.json new file mode 100644 index 0000000..037440f --- /dev/null +++ b/review-suite/evals/contracts/executor-response.schema.json @@ -0,0 +1,41 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/executor-response.schema.json", + "title": "Review replay executor response", + "description": "The single JSON object one executor process writes to stdout. `outcome` separates a valid review from a runtime failure so a failed run is never scored as a clean review.", + "type": "object", + "additionalProperties": false, + "required": ["protocol_version", "outcome", "simulation", "executor"], + "properties": { + "protocol_version": {"const": "1.0"}, + "outcome": {"enum": ["review_result", "blocked", "runtime_failure"]}, + "simulation": {"type": "boolean"}, + "executor": { + "type": "object", + "additionalProperties": false, + "required": ["name"], + "properties": { + "name": {"type": "string", "minLength": 1}, + "version": {"type": "string", "minLength": 1}, + "runtime": {"type": "string", "minLength": 1}, + "model": {"type": "string", "minLength": 1} + } + }, + "result": {"type": "object"}, + "failure": { + "type": "object", + "additionalProperties": false, + "required": ["reason"], + "properties": {"reason": {"type": "string", "minLength": 1}} + }, + "usage": { + "type": "object", + "additionalProperties": false, + "properties": { + "input_tokens": {}, + "output_tokens": {}, + "cost_usd": {} + } + } + } +} diff --git a/review-suite/evals/contracts/expectation.schema.json b/review-suite/evals/contracts/expectation.schema.json new file mode 100644 index 0000000..75875ee --- /dev/null +++ b/review-suite/evals/contracts/expectation.schema.json @@ -0,0 +1,68 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/expectation.schema.json", + "title": "Private review replay expectation", + "description": "Private grading evidence for one case. This file never enters an executor request. `requirement` and `surface` may legitimately restate reviewer-visible packet content; every other field is blind and must be absent from the request payload.", + "type": "object", + "additionalProperties": false, + "required": [ + "expectation_version", + "case_id", + "packet_valid", + "expected_verdict", + "material_root_causes", + "accepted_non_findings" + ], + "properties": { + "expectation_version": {"const": "1.0"}, + "case_id": {"type": "string", "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$"}, + "packet_valid": {"type": "boolean"}, + "expected_verdict": {"enum": ["clean", "changes_required", "blocked"]}, + "material_root_causes": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": [ + "id", + "requirement", + "trigger", + "surface", + "consequence", + "severity", + "equivalent_formulations" + ], + "properties": { + "id": {"type": "string", "pattern": "^rc\\.[a-z0-9][a-z0-9-]*$"}, + "requirement": {"type": "string", "minLength": 1}, + "trigger": {"type": "string", "minLength": 1}, + "surface": {"type": "string", "minLength": 1}, + "consequence": {"type": "string", "minLength": 1}, + "severity": {"enum": ["blocking", "strong_recommendation"]}, + "equivalent_formulations": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "minLength": 1} + } + } + } + }, + "accepted_non_findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["id", "description", "equivalent_formulations"], + "properties": { + "id": {"type": "string", "pattern": "^anf\\.[a-z0-9][a-z0-9-]*$"}, + "description": {"type": "string", "minLength": 1}, + "equivalent_formulations": { + "type": "array", + "minItems": 1, + "items": {"type": "string", "minLength": 1} + } + } + } + } + } +} diff --git a/review-suite/evals/contracts/provenance.schema.json b/review-suite/evals/contracts/provenance.schema.json new file mode 100644 index 0000000..f3de4a8 --- /dev/null +++ b/review-suite/evals/contracts/provenance.schema.json @@ -0,0 +1,32 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/provenance.schema.json", + "title": "Review replay case provenance", + "description": "Where one case came from and under what authority it is retained. Provenance is private: it never enters an executor request.", + "type": "object", + "additionalProperties": false, + "required": [ + "provenance_version", + "case_id", + "origin", + "retention_authority", + "sanitization", + "recorded_at" + ], + "properties": { + "provenance_version": {"const": "1.0"}, + "case_id": {"type": "string", "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$"}, + "origin": { + "enum": [ + "synthetic", + "minimized_reproduction", + "repository_history", + "connector_outcome" + ] + }, + "retention_authority": {"type": "string", "minLength": 1}, + "sanitization": {"type": "string", "minLength": 1}, + "recorded_at": {"type": "string", "pattern": "^\\d{4}-\\d{2}-\\d{2}$"}, + "notes": {"type": "string", "minLength": 1} + } +} diff --git a/review-suite/evals/corpus/corpus.json b/review-suite/evals/corpus/corpus.json new file mode 100644 index 0000000..6bb34b7 --- /dev/null +++ b/review-suite/evals/corpus/corpus.json @@ -0,0 +1,14 @@ +{ + "corpus_version": "0.1-protocol-proof", + "protocol_version": "1.0", + "grader_version": "1.0", + "target_skill": "review-code-change", + "cases": [ + "ledger-charge-limit", + "session-token-refresh", + "invoice-export-batch", + "webhook-retry-queue", + "pricing-tier-lookup", + "catalog-import-feed" + ] +} diff --git a/review-suite/evals/corpus/private/expectations/catalog-import-feed.json b/review-suite/evals/corpus/private/expectations/catalog-import-feed.json new file mode 100644 index 0000000..307d2c5 --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/catalog-import-feed.json @@ -0,0 +1,8 @@ +{ + "expectation_version": "1.0", + "case_id": "catalog-import-feed", + "packet_valid": false, + "expected_verdict": "blocked", + "material_root_causes": [], + "accepted_non_findings": [] +} diff --git a/review-suite/evals/corpus/private/expectations/invoice-export-batch.json b/review-suite/evals/corpus/private/expectations/invoice-export-batch.json new file mode 100644 index 0000000..47ded04 --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/invoice-export-batch.json @@ -0,0 +1,17 @@ +{ + "expectation_version": "1.0", + "case_id": "invoice-export-batch", + "packet_valid": true, + "expected_verdict": "clean", + "material_root_causes": [], + "accepted_non_findings": [ + { + "id": "anf.invoice-helper-naming", + "description": "Observing that the pre-existing helper name is terse is a real but non-material remark, not a defect this candidate introduced.", + "equivalent_formulations": [ + "the batch helper name predates this change", + "terse pre-existing helper name" + ] + } + ] +} diff --git a/review-suite/evals/corpus/private/expectations/ledger-charge-limit.json b/review-suite/evals/corpus/private/expectations/ledger-charge-limit.json new file mode 100644 index 0000000..4e33a6e --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/ledger-charge-limit.json @@ -0,0 +1,22 @@ +{ + "expectation_version": "1.0", + "case_id": "ledger-charge-limit", + "packet_valid": true, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.ledger-charge-boundary", + "requirement": "A charge equal to the available balance succeeds.", + "trigger": "the charged amount lands exactly on the computed availability", + "surface": "ledger.py:can_charge", + "consequence": "an account holder cannot spend the last cent they own", + "severity": "blocking", + "equivalent_formulations": [ + "off by one at the boundary", + "rejects a charge equal to the available balance", + "strict comparison excludes equality" + ] + } + ], + "accepted_non_findings": [] +} diff --git a/review-suite/evals/corpus/private/expectations/pricing-tier-lookup.json b/review-suite/evals/corpus/private/expectations/pricing-tier-lookup.json new file mode 100644 index 0000000..7cea776 --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/pricing-tier-lookup.json @@ -0,0 +1,35 @@ +{ + "expectation_version": "1.0", + "case_id": "pricing-tier-lookup", + "packet_valid": true, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.pricing-edge-selection", + "requirement": "A quantity that sits exactly on a tier ceiling is quoted in that tier.", + "trigger": "a quote asks for exactly the published ceiling quantity", + "consequence": "the customer is quoted from the next tier up", + "surface": "pricing.py:select_tier", + "severity": "blocking", + "equivalent_formulations": [ + "picks the wrong tier edge", + "wrong tier at the edge", + "ceiling is treated as exclusive" + ] + }, + { + "id": "rc.pricing-stale-table", + "requirement": "An updated tier table is used by later quotes.", + "trigger": "an operator edits the table while the service keeps running", + "consequence": "quotes keep using the table captured when the process started", + "surface": "pricing.py:select_tier", + "severity": "blocking", + "equivalent_formulations": [ + "caches the stale tier table", + "serves a stale table", + "reads the table captured at import time" + ] + } + ], + "accepted_non_findings": [] +} diff --git a/review-suite/evals/corpus/private/expectations/session-token-refresh.json b/review-suite/evals/corpus/private/expectations/session-token-refresh.json new file mode 100644 index 0000000..b654ddc --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/session-token-refresh.json @@ -0,0 +1,34 @@ +{ + "expectation_version": "1.0", + "case_id": "session-token-refresh", + "packet_valid": true, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.session-refresh-deadline", + "requirement": "Refreshing an active session keeps it usable for a further full window.", + "trigger": "an operator extends a session late in its original window", + "consequence": "an operator is signed out in the middle of active work", + "surface": "session.py:refresh", + "severity": "blocking", + "equivalent_formulations": [ + "stale expiry clock", + "reuses the original issue time", + "deadline is not recomputed" + ] + }, + { + "id": "rc.session-audit-record", + "requirement": "Every refresh is recorded so an operator can reconstruct it later.", + "trigger": "an operator reads the log after several extensions", + "consequence": "the recorded history cannot distinguish one extension from another", + "surface": "audit.py:write_entry", + "severity": "strong_recommendation", + "equivalent_formulations": [ + "the recorded entry omits which moment the extension happened", + "no timestamp distinguishes repeated entries" + ] + } + ], + "accepted_non_findings": [] +} diff --git a/review-suite/evals/corpus/private/expectations/webhook-retry-queue.json b/review-suite/evals/corpus/private/expectations/webhook-retry-queue.json new file mode 100644 index 0000000..9c2fd8c --- /dev/null +++ b/review-suite/evals/corpus/private/expectations/webhook-retry-queue.json @@ -0,0 +1,22 @@ +{ + "expectation_version": "1.0", + "case_id": "webhook-retry-queue", + "packet_valid": true, + "expected_verdict": "changes_required", + "material_root_causes": [ + { + "id": "rc.webhook-redelivery-idempotence", + "requirement": "A given delivery changes downstream state at most once.", + "trigger": "the upstream sender delivers the same identifier twice", + "consequence": "downstream totals drift upward every time a delivery repeats", + "surface": "webhook.py:handle", + "severity": "blocking", + "equivalent_formulations": [ + "retries without a de-duplication key", + "a redelivered event is applied more than once", + "the retry loop is not idempotent" + ] + } + ], + "accepted_non_findings": [] +} diff --git a/review-suite/evals/corpus/private/provenance/catalog-import-feed.json b/review-suite/evals/corpus/private/provenance/catalog-import-feed.json new file mode 100644 index 0000000..4011ef8 --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/catalog-import-feed.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "catalog-import-feed", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/private/provenance/invoice-export-batch.json b/review-suite/evals/corpus/private/provenance/invoice-export-batch.json new file mode 100644 index 0000000..675753f --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/invoice-export-batch.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "invoice-export-batch", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/private/provenance/ledger-charge-limit.json b/review-suite/evals/corpus/private/provenance/ledger-charge-limit.json new file mode 100644 index 0000000..e834fca --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/ledger-charge-limit.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "ledger-charge-limit", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/private/provenance/pricing-tier-lookup.json b/review-suite/evals/corpus/private/provenance/pricing-tier-lookup.json new file mode 100644 index 0000000..05ea8e3 --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/pricing-tier-lookup.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "pricing-tier-lookup", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/private/provenance/session-token-refresh.json b/review-suite/evals/corpus/private/provenance/session-token-refresh.json new file mode 100644 index 0000000..551b1fb --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/session-token-refresh.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "session-token-refresh", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/private/provenance/webhook-retry-queue.json b/review-suite/evals/corpus/private/provenance/webhook-retry-queue.json new file mode 100644 index 0000000..3efe287 --- /dev/null +++ b/review-suite/evals/corpus/private/provenance/webhook-retry-queue.json @@ -0,0 +1,9 @@ +{ + "provenance_version": "1.0", + "case_id": "webhook-retry-queue", + "origin": "synthetic", + "retention_authority": "Authored for this repository as protocol proof; no third-party or customer material is reproduced.", + "sanitization": "Fully invented module names, identifiers, and values; nothing derived from private code, credentials, or customer records.", + "recorded_at": "2026-07-26", + "notes": "Protocol and grader proof only. #58 owns the representative scored corpus." +} diff --git a/review-suite/evals/corpus/reviewer/PROMPT.md b/review-suite/evals/corpus/reviewer/PROMPT.md new file mode 100644 index 0000000..bba42bc --- /dev/null +++ b/review-suite/evals/corpus/reviewer/PROMPT.md @@ -0,0 +1,7 @@ +Review the supplied review packet as a read-only reviewer. Apply the +repository's canonical review packet, finding, and verdict contract exactly as +written, and bind the result to the candidate identity supplied with the run. + +Return only one conforming review result. Derive every conclusion from the +packet and the supplied contracts; no other evidence is available, and no +information about this case exists outside them. diff --git a/review-suite/evals/corpus/reviewer/catalog-import-feed/packet.json b/review-suite/evals/corpus/reviewer/catalog-import-feed/packet.json new file mode 100644 index 0000000..ebdf61b --- /dev/null +++ b/review-suite/evals/corpus/reviewer/catalog-import-feed/packet.json @@ -0,0 +1,48 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/catalog", + "base_branch": "main" + }, + "candidate": { + "head_sha": "6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a", + "comparison_base_sha": "6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b6b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/catalog.py b/catalog.py\n--- a/catalog.py\n+++ b/catalog.py\n@@ -8,2 +8,2 @@ def parse_feed(payload):\n def import_feed(payload):\n- return [normalize(row) for row in parse_feed(payload)]\n+ return [normalize(row) for row in parse_feed(payload) if row.sku]\n" + } + }, + "change_contract": { + "goal": "Skip catalog rows that arrive without a stock identifier.", + "acceptance_criteria": [ + "A row with no stock identifier is not imported.", + "Every other row is imported exactly as before." + ], + "non_goals": [ + "Change how a stock identifier is generated upstream." + ], + "preserved_behaviors": [ + "Row order is preserved for imported rows." + ] + }, + "sources": { + "repository_instructions": [], + "named_documents": [], + "nearby_patterns": [ + { + "label": "Feed tests", + "location": "tests/test_catalog.py" + } + ] + }, + "validation": [ + { + "name": "catalog tests", + "command": "pytest tests/test_catalog.py", + "scope": "focused", + "status": "passed", + "result": "3 passed" + } + ] +} diff --git a/review-suite/evals/corpus/reviewer/invoice-export-batch/packet.json b/review-suite/evals/corpus/reviewer/invoice-export-batch/packet.json new file mode 100644 index 0000000..6d52cd3 --- /dev/null +++ b/review-suite/evals/corpus/reviewer/invoice-export-batch/packet.json @@ -0,0 +1,70 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/invoice", + "base_branch": "main" + }, + "candidate": { + "head_sha": "3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a3a", + "comparison_base_sha": "3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b3b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/invoice.py b/invoice.py\n--- a/invoice.py\n+++ b/invoice.py\n@@ -50,3 +50,3 @@ def _bx(rows, size):\n def export(rows):\n- for group in _bx(rows, 100):\n+ for group in _bx(rows, EXPORT_BATCH_SIZE):\n yield render(group)\n" + } + }, + "change_contract": { + "goal": "Make the invoice export batch size configurable.", + "acceptance_criteria": [ + "The export batch size comes from configuration instead of a literal.", + "The rendered output for a given set of rows is unchanged." + ], + "non_goals": [ + "Change the rendered invoice format." + ], + "preserved_behaviors": [ + "Rows are exported in their original order." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Configuration", + "location": "AGENTS.md", + "summary": "Tunables live in settings." + } + ], + "named_documents": [], + "nearby_patterns": [ + { + "label": "Existing tunables", + "location": "settings.py" + }, + { + "label": "Export tests", + "location": "tests/test_invoice.py" + } + ] + }, + "validation": [ + { + "name": "invoice tests", + "command": "pytest tests/test_invoice.py", + "scope": "focused", + "status": "passed", + "result": "14 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "203 passed" + } + ], + "context": { + "operational": [ + "EXPORT_BATCH_SIZE defaults to 100, matching the previous literal." + ] + } +} diff --git a/review-suite/evals/corpus/reviewer/ledger-charge-limit/packet.json b/review-suite/evals/corpus/reviewer/ledger-charge-limit/packet.json new file mode 100644 index 0000000..e66a6ea --- /dev/null +++ b/review-suite/evals/corpus/reviewer/ledger-charge-limit/packet.json @@ -0,0 +1,66 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/ledger", + "base_branch": "main" + }, + "candidate": { + "head_sha": "1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a", + "comparison_base_sha": "1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b1b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/ledger.py b/ledger.py\n--- a/ledger.py\n+++ b/ledger.py\n@@ -12,2 +12,2 @@ def available(account):\n def can_charge(account, amount):\n- return amount <= available(account)\n+ return amount < available(account)\n" + } + }, + "change_contract": { + "goal": "Refuse a charge that would overdraw an account.", + "acceptance_criteria": [ + "A charge equal to the available balance succeeds.", + "A charge above the available balance is refused." + ], + "non_goals": [ + "Introduce an overdraft product." + ], + "preserved_behaviors": [ + "Holds continue to reduce the available balance." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Money handling", + "location": "AGENTS.md", + "summary": "Amounts are integer cents." + } + ], + "named_documents": [], + "nearby_patterns": [ + { + "label": "Charge limit tests", + "location": "tests/test_ledger.py" + } + ] + }, + "validation": [ + { + "name": "ledger tests", + "command": "pytest tests/test_ledger.py", + "scope": "focused", + "status": "passed", + "result": "6 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "48 passed" + } + ], + "context": { + "data": [ + "Balances and holds are stored as integer cents." + ] + } +} diff --git a/review-suite/evals/corpus/reviewer/pricing-tier-lookup/packet.json b/review-suite/evals/corpus/reviewer/pricing-tier-lookup/packet.json new file mode 100644 index 0000000..3878061 --- /dev/null +++ b/review-suite/evals/corpus/reviewer/pricing-tier-lookup/packet.json @@ -0,0 +1,71 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/pricing", + "base_branch": "main" + }, + "candidate": { + "head_sha": "5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a", + "comparison_base_sha": "5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b5b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/pricing.py b/pricing.py\n--- a/pricing.py\n+++ b/pricing.py\n@@ -16,4 +16,4 @@ TIERS = load_tiers()\n def select_tier(units):\n- for tier in load_tiers():\n- if units <= tier.ceiling:\n+ for tier in TIERS:\n+ if units < tier.ceiling:\n return tier\n" + } + }, + "change_contract": { + "goal": "Speed up tier selection for the quoting endpoint.", + "acceptance_criteria": [ + "A quantity that sits exactly on a tier ceiling is quoted in that tier.", + "An updated tier table is used by later quotes." + ], + "non_goals": [ + "Change the published tier ceilings." + ], + "preserved_behaviors": [ + "Every caller of select_tier receives the same tier for the same quantity." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Pricing rules", + "location": "AGENTS.md", + "summary": "Published ceilings are inclusive." + } + ], + "named_documents": [ + { + "label": "Tier table", + "location": "docs/pricing.md" + } + ], + "nearby_patterns": [ + { + "label": "Pricing tests", + "location": "tests/test_pricing.py" + } + ] + }, + "validation": [ + { + "name": "pricing tests", + "command": "pytest tests/test_pricing.py", + "scope": "focused", + "status": "passed", + "result": "7 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "64 passed" + } + ], + "context": { + "operational": [ + "Operators edit the tier table while the service is running." + ] + } +} diff --git a/review-suite/evals/corpus/reviewer/session-token-refresh/packet.json b/review-suite/evals/corpus/reviewer/session-token-refresh/packet.json new file mode 100644 index 0000000..2cd6a9a --- /dev/null +++ b/review-suite/evals/corpus/reviewer/session-token-refresh/packet.json @@ -0,0 +1,75 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/session", + "base_branch": "main" + }, + "candidate": { + "head_sha": "2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a", + "comparison_base_sha": "2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/session.py b/session.py\n--- a/session.py\n+++ b/session.py\n@@ -29,2 +29,3 @@ class Session:\n def refresh(self):\n- self.expires_at = now() + WINDOW\n+ self.expires_at = self.issued_at + WINDOW\n+ audit.write_entry(self.id, \"refresh\")\ndiff --git a/audit.py b/audit.py\n--- a/audit.py\n+++ b/audit.py\n@@ -6,2 +6,3 @@ def write_entry(subject, action):\n record = {\"subject\": subject, \"action\": action}\n+ record[\"source\"] = \"session\"\n store.append(record)\n" + } + }, + "change_contract": { + "goal": "Let an operator extend an active session and record that it happened.", + "acceptance_criteria": [ + "Refreshing an active session keeps it usable for a further full window.", + "Every refresh is recorded so an operator can reconstruct it later." + ], + "non_goals": [ + "Change how a session is first issued." + ], + "preserved_behaviors": [ + "An abandoned session still lapses after one window." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Audit rules", + "location": "AGENTS.md", + "summary": "Every state change is recorded." + } + ], + "named_documents": [ + { + "label": "Session lifecycle", + "location": "docs/sessions.md" + } + ], + "nearby_patterns": [ + { + "label": "Session tests", + "location": "tests/test_session.py" + }, + { + "label": "Audit writer", + "location": "audit.py" + } + ] + }, + "validation": [ + { + "name": "session tests", + "command": "pytest tests/test_session.py", + "scope": "focused", + "status": "passed", + "result": "9 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "121 passed" + } + ], + "context": { + "operational": [ + "Sessions are held in one process and are not shared." + ] + } +} diff --git a/review-suite/evals/corpus/reviewer/webhook-retry-queue/packet.json b/review-suite/evals/corpus/reviewer/webhook-retry-queue/packet.json new file mode 100644 index 0000000..1f2c0d1 --- /dev/null +++ b/review-suite/evals/corpus/reviewer/webhook-retry-queue/packet.json @@ -0,0 +1,78 @@ +{ + "schema_version": "1.0", + "repository": { + "identity": "example/webhook", + "base_branch": "main" + }, + "candidate": { + "head_sha": "4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a4a", + "comparison_base_sha": "4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b4b", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/webhook.py b/webhook.py\n--- a/webhook.py\n+++ b/webhook.py\n@@ -20,2 +20,3 @@ def enqueue(event):\n def handle(event):\n- apply_event(event)\n+ for _ in range(RETRIES):\n+ apply_event(event)\n" + } + }, + "change_contract": { + "goal": "Keep applying an inbound event when the downstream call fails.", + "acceptance_criteria": [ + "A transient downstream failure no longer drops the event.", + "A given delivery changes downstream state at most once." + ], + "non_goals": [ + "Change how events are rendered for operators." + ], + "preserved_behaviors": [ + "Events are applied in the order they were received." + ] + }, + "sources": { + "repository_instructions": [ + { + "label": "Queue rules", + "location": "AGENTS.md", + "summary": "Handlers must tolerate redelivery." + } + ], + "named_documents": [ + { + "label": "Delivery contract", + "location": "docs/webhooks.md" + } + ], + "nearby_patterns": [ + { + "label": "Queue tests", + "location": "tests/test_webhook.py" + }, + { + "label": "Presentation helper", + "location": "presentation.py" + } + ] + }, + "validation": [ + { + "name": "webhook tests", + "command": "pytest tests/test_webhook.py", + "scope": "focused", + "status": "passed", + "result": "11 passed" + }, + { + "name": "full tests", + "command": "pytest", + "scope": "full", + "status": "passed", + "result": "96 passed" + } + ], + "context": { + "data": [ + "Each delivery carries a stable delivery identifier." + ], + "operational": [ + "The upstream sender redelivers on any non-2xx reply." + ] + } +} diff --git a/review-suite/scripts/evals/__init__.py b/review-suite/scripts/evals/__init__.py new file mode 100644 index 0000000..89c0291 --- /dev/null +++ b/review-suite/scripts/evals/__init__.py @@ -0,0 +1 @@ +"""Canonical result-blind replay evaluator for the repository review suite.""" diff --git a/review-suite/scripts/evals/audit_corpus.py b/review-suite/scripts/evals/audit_corpus.py new file mode 100644 index 0000000..0767786 --- /dev/null +++ b/review-suite/scripts/evals/audit_corpus.py @@ -0,0 +1,91 @@ +#!/usr/bin/env python3 +"""Audit the replay corpus without launching any model. + +Checks, in order: + +1. corpus index, expectation, and provenance schemas; +2. cross-field expectation semantics and packet validity agreement; +3. reviewer/private separation, orphaned files, and reviewer-prompt wording; +4. case identifiers and reviewer-visible filenames for outcome-revealing + tokens; and +5. the complete executor request that each case would produce, structurally + and textually, for private expectation or provenance leakage. + +Exit status is 0 when the corpus can be trusted for a blind evaluation and 1 +otherwise. This command never spawns an executor and never costs money. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +if __package__ in (None, ""): + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import corpus, grader, protocol, runner +else: + from . import corpus, grader, protocol, runner + + +def audit(corpus_root: Path | None) -> list[str]: + """Return every reason the corpus cannot be trusted, or an empty list.""" + try: + loaded = corpus.load_corpus(corpus_root) + except corpus.CorpusError as error: + return [str(error)] + + errors = list(corpus.prompt_errors(loaded.root)) + if loaded.grader_version != grader.GRADER_VERSION: + errors.append( + f"corpus grader_version {loaded.grader_version!r} does not match the " + f"shipped grader {grader.GRADER_VERSION!r}" + ) + try: + skill_prompt = runner.target_skill_prompt(loaded.target_skill) + except runner.ConfigurationError as error: + return errors + [str(error)] + + documents = runner.contract_documents() + for case in loaded.cases: + request = protocol.build_request( + case_id=case.case_id, + target_skill=loaded.target_skill, + skill_prompt=skill_prompt, + contract_documents=documents, + instructions=case.instructions, + packet=case.packet, + run_number=1, + suite_commit="audit", + corpus_version=loaded.corpus_version, + started_at="audit", + ) + errors.extend( + f"{case.case_id}: {error}" + for error in protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + ) + return errors + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--corpus", type=Path, default=None) + args = parser.parse_args(argv) + + errors = audit(args.corpus) + if errors: + for error in errors: + print(error, file=sys.stderr) + print(f"corpus audit failed with {len(errors)} error(s)", file=sys.stderr) + return 1 + print("corpus audit passed") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/evals/claude_executor.py b/review-suite/scripts/evals/claude_executor.py new file mode 100644 index 0000000..cd03c3c --- /dev/null +++ b/review-suite/scripts/evals/claude_executor.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Real-runtime replay executor backed by Claude Code headless mode. + +Reads one result-blind request on stdin, asks a fresh `claude -p` process to +perform the review described by the target skill prompt and the raw packet, and +writes one protocol response on stdout. It never sees expected findings, case +names, or grader data, because the runner never puts them in the request. + +Product-specific launch details stay here. The core protocol in `protocol.py` +knows nothing about Claude, so another runtime only needs its own adapter. + +Usage: + just eval-review-suite "python3 review-suite/scripts/evals/claude_executor.py" +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path +from typing import Any + +if __package__ in (None, ""): + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import protocol +else: + from . import protocol + +EXECUTOR_NAME = "review-suite-claude-executor" +EXECUTOR_VERSION = "1.0" + + +def build_prompt(request: dict[str, Any]) -> str: + """Render the reviewer-visible request as one headless prompt.""" + documents = "\n\n".join( + f"### {name}\n\n{text}" + for name, text in sorted(request["contract_documents"].items()) + ) + return "\n".join( + [ + "You are a read-only reviewer executing the skill below for one", + "candidate. Do not run tools, modify files, or inspect any", + "repository. Reason only from the artifacts supplied here.", + "", + "## Skill", + request["skill_prompt"], + "", + "## Review suite contracts", + documents, + "", + "## Instructions", + request["instructions"], + "", + "## Review packet (JSON)", + json.dumps(request["packet"], indent=2, sort_keys=True), + "", + "## Answer format", + "Return ONLY one JSON object conforming to", + "review-result.schema.json, with no prose and no code fence. Bind", + "it to this candidate identity:", + json.dumps(request["run"]["candidate"], sort_keys=True), + ] + ) + + +def extract_json_object(text: str) -> dict[str, Any]: + text = text.strip() + fenced = re.search(r"```(?:json)?\s*(\{.*?\})\s*```", text, re.DOTALL) + candidate = fenced.group(1) if fenced else text + start = candidate.find("{") + end = candidate.rfind("}") + if start < 0 or end <= start: + raise ValueError("the runtime returned no JSON object") + value = json.loads(candidate[start : end + 1]) + if not isinstance(value, dict): + raise ValueError("the runtime returned a non-object JSON value") + return value + + +def run_claude( + prompt: str, *, claude_bin: str, model: str | None +) -> tuple[dict[str, Any], dict[str, Any]]: + """Run one fresh headless process and return (review result, envelope).""" + command = [claude_bin, "-p", "--output-format", "json"] + if model: + command.extend(["--model", model]) + completed = subprocess.run( + command, input=prompt, capture_output=True, text=True, check=False + ) + if completed.returncode: + raise RuntimeError( + f"{claude_bin} exited {completed.returncode}: " + f"{completed.stderr.strip()[-500:]}" + ) + envelope = json.loads(completed.stdout) + if not isinstance(envelope, dict): + raise RuntimeError("headless output was not one JSON object") + result_text = envelope.get("result") + if not isinstance(result_text, str): + raise RuntimeError("headless output carried no result text") + return extract_json_object(result_text), envelope + + +def usage_from(envelope: dict[str, Any]) -> dict[str, Any] | None: + """Map whatever usage the runtime reported into the protocol shape.""" + reported = envelope.get("usage") or {} + usage: dict[str, Any] = {} + for source, target in ( + ("input_tokens", "input_tokens"), + ("output_tokens", "output_tokens"), + ): + value = reported.get(source) + if isinstance(value, (int, float)) and not isinstance(value, bool): + usage[target] = value + cost = envelope.get("total_cost_usd") + if isinstance(cost, (int, float)) and not isinstance(cost, bool): + usage["cost_usd"] = cost + return usage or None + + +def respond(request: dict[str, Any], args: argparse.Namespace) -> dict[str, Any]: + executor = { + "name": EXECUTOR_NAME, + "version": EXECUTOR_VERSION, + "runtime": Path(args.claude_bin).name, + } + if args.model: + executor["model"] = args.model + try: + result, envelope = run_claude( + build_prompt(request), claude_bin=args.claude_bin, model=args.model + ) + except (OSError, RuntimeError, ValueError, json.JSONDecodeError) as error: + return { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "runtime_failure", + "simulation": False, + "executor": executor, + "failure": {"reason": str(error)}, + } + + model = envelope.get("model") or envelope.get("modelUsage") + if isinstance(model, str): + executor["model"] = model + response: dict[str, Any] = { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "blocked" if result.get("verdict") == "blocked" else "review_result", + "simulation": False, + "executor": executor, + "result": result, + } + usage = usage_from(envelope) + if usage: + response["usage"] = usage + return response + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--claude-bin", default="claude") + parser.add_argument("--model", default=None) + args = parser.parse_args(argv) + protocol.write_response(respond(protocol.read_request(), args)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/evals/corpus.py b/review-suite/scripts/evals/corpus.py new file mode 100644 index 0000000..10defff --- /dev/null +++ b/review-suite/scripts/evals/corpus.py @@ -0,0 +1,274 @@ +#!/usr/bin/env python3 +"""Versioned, result-blind corpus contract for review replay evaluation. + +The layout physically separates the three kinds of data so a reviewer-visible +read can never reach grading evidence: + + /corpus.json version metadata, case ids + /reviewer/PROMPT.md shared reviewer instructions + /reviewer//packet.json reviewer-visible artifacts + /private/expectations/.json expected material root causes + /private/provenance/.json origin and retention authority + +Loading fails closed. A missing expectation, a malformed schema, an orphaned +file, or an outcome-revealing identifier is an error before any executor +process is started. +""" + +from __future__ import annotations + +import json +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from . import protocol + +DEFAULT_CORPUS = protocol.REVIEW_SUITE / "evals" / "corpus" +REVIEWER_PROMPT = "PROMPT.md" + +#: Filenames permitted inside a reviewer-visible case directory. Anything else +#: risks shipping grading evidence to the target reviewer. +REVIEWER_CASE_FILES = frozenset({"packet.json"}) + +#: Substrings that would reveal a verdict, severity, failure class, connector +#: disposition, or target finding through a case id or reviewer-visible +#: filename. Case names must describe the subject matter, never the answer. +OUTCOME_REVEALING_TOKENS = ( + "accept", + "blocked", + "blocking", + "broken", + "bug", + "changes", + "clean", + "control", + "defect", + "defer", + "duplicate", + "escape", + "expected", + "fail", + "false", + "flaw", + "insecure", + "invalid", + "miss", + "negative", + "overengineered", + "polish", + "positive", + "regression", + "reject", + "severity", + "simplif", + "speculative", + "unsafe", + "verdict", + "vulnerab", +) + + +@dataclass(frozen=True) +class Case: + """One corpus case with its three separately validated parts.""" + + case_id: str + packet: dict[str, Any] + instructions: str + expectation: dict[str, Any] + provenance: dict[str, Any] + + @property + def case_ref(self) -> str: + return protocol.case_ref(self.case_id) + + +@dataclass(frozen=True) +class Corpus: + root: Path + corpus_version: str + grader_version: str + target_skill: str + cases: tuple[Case, ...] + + +class CorpusError(ValueError): + """Raised when the corpus cannot be trusted for a blind evaluation.""" + + +def _load_json(path: Path) -> Any: + if not path.is_file(): + raise CorpusError(f"missing {path}") + try: + return json.loads(path.read_text()) + except json.JSONDecodeError as error: + raise CorpusError(f"invalid JSON in {path}: {error}") from error + + +def revealing_tokens(name: str) -> list[str]: + """Return the outcome-revealing substrings present in one name.""" + lowered = name.lower() + return [token for token in OUTCOME_REVEALING_TOKENS if token in lowered] + + +def _expectation_semantics(expectation: dict[str, Any]) -> list[str]: + """Cross-field expectation rules JSON Schema cannot express clearly.""" + errors = [] + verdict = expectation.get("expected_verdict") + root_causes = expectation.get("material_root_causes") or [] + packet_valid = expectation.get("packet_valid") + + if verdict == "changes_required" and not root_causes: + errors.append("changes_required requires at least one material root cause") + if verdict == "clean" and root_causes: + errors.append("clean cannot expect a material root cause") + if verdict == "blocked": + if root_causes: + errors.append("blocked cannot expect a material root cause") + if packet_valid is not False: + errors.append("blocked requires packet_valid false") + elif packet_valid is not True: + errors.append(f"{verdict} requires packet_valid true") + + for field, prefix in ( + ("material_root_causes", "rc"), + ("accepted_non_findings", "anf"), + ): + identifiers = [item.get("id") for item in expectation.get(field) or []] + duplicates = sorted({i for i in identifiers if identifiers.count(i) > 1}) + if duplicates: + errors.append(f"duplicate {prefix} id(s): " + ", ".join(duplicates)) + return errors + + +def load_case(root: Path, case_id: str) -> Case: + """Load and validate one case's reviewer-visible and private parts.""" + errors: list[str] = [] + + tokens = revealing_tokens(case_id) + if tokens: + errors.append( + f"case id reveals outcome via {', '.join(tokens)}; " + "name cases after their subject matter" + ) + + reviewer_dir = root / "reviewer" / case_id + if not reviewer_dir.is_dir(): + raise CorpusError(f"missing reviewer directory {reviewer_dir}") + for path in sorted(reviewer_dir.iterdir()): + if path.name not in REVIEWER_CASE_FILES: + errors.append(f"unpermitted reviewer-visible file {path}") + file_tokens = revealing_tokens(path.name) + if file_tokens: + errors.append( + f"reviewer-visible filename {path.name} reveals outcome via " + + ", ".join(file_tokens) + ) + + packet = _load_json(reviewer_dir / "packet.json") + expectation = _load_json(root / "private" / "expectations" / f"{case_id}.json") + provenance = _load_json(root / "private" / "provenance" / f"{case_id}.json") + + for name, document, schema in ( + ("expectation", expectation, "expectation.schema.json"), + ("provenance", provenance, "provenance.schema.json"), + ): + errors.extend( + f"{name}: {error}" for error in protocol.validate_against(schema, document) + ) + if document.get("case_id") != case_id: + errors.append(f"{name}: case_id does not match {case_id}") + + errors.extend( + f"expectation: {error}" for error in _expectation_semantics(expectation) + ) + + packet_errors = protocol.VALIDATOR.validate_packet(packet) + if expectation.get("packet_valid") and packet_errors: + errors.extend(f"packet: {error}" for error in packet_errors) + elif expectation.get("packet_valid") is False and not packet_errors: + errors.append("packet: packet_valid false but the packet validates") + + if errors: + raise CorpusError(f"{case_id}: " + "; ".join(errors)) + + instructions = (root / "reviewer" / REVIEWER_PROMPT).read_text() + return Case( + case_id=case_id, + packet=packet, + instructions=instructions, + expectation=expectation, + provenance=provenance, + ) + + +def _orphan_errors(root: Path, declared: set[str]) -> list[str]: + """Report corpus files that no declared case owns, in both directions.""" + errors = [] + reviewer_root = root / "reviewer" + present = {path.name for path in reviewer_root.iterdir() if path.is_dir()} + for extra in sorted(present - declared): + errors.append(f"reviewer/{extra} is not declared in corpus.json") + for subdir, suffix in (("expectations", ".json"), ("provenance", ".json")): + directory = root / "private" / subdir + if not directory.is_dir(): + errors.append(f"missing {directory}") + continue + names = {path.name[: -len(suffix)] for path in directory.glob(f"*{suffix}")} + for extra in sorted(names - declared): + errors.append(f"private/{subdir}/{extra}{suffix} is not declared") + return errors + + +def load_corpus(root: Path | None = None) -> Corpus: + """Load the whole corpus, failing closed on any integrity problem.""" + root = Path(root) if root else DEFAULT_CORPUS + if not root.is_dir(): + raise CorpusError(f"missing corpus directory {root}") + index = _load_json(root / "corpus.json") + schema_errors = protocol.validate_against("corpus.schema.json", index) + if schema_errors: + raise CorpusError("corpus.json: " + "; ".join(schema_errors)) + if not (root / "reviewer" / REVIEWER_PROMPT).is_file(): + raise CorpusError(f"missing {root / 'reviewer' / REVIEWER_PROMPT}") + + declared = list(index["cases"]) + if len(set(declared)) != len(declared): + raise CorpusError("corpus.json: duplicate case id(s)") + orphans = _orphan_errors(root, set(declared)) + if orphans: + raise CorpusError("corpus.json: " + "; ".join(orphans)) + + return Corpus( + root=root, + corpus_version=index["corpus_version"], + grader_version=index["grader_version"], + target_skill=index["target_skill"], + cases=tuple(load_case(root, case_id) for case_id in declared), + ) + + +#: Verdict and severity names the shared reviewer prompt must not mention. +PROMPT_FORBIDDEN_WORDS = frozenset( + { + "blocked", + "blocking", + "changes_required", + "clean", + "defer", + "strong_recommendation", + } +) + + +def prompt_errors(root: Path | None = None) -> list[str]: + """Reject shared reviewer instructions that hint at an expected outcome.""" + root = Path(root) if root else DEFAULT_CORPUS + text = (root / "reviewer" / REVIEWER_PROMPT).read_text() + words = set(re.findall(r"[a-z_]+", text.lower())) + hits = sorted(words & PROMPT_FORBIDDEN_WORDS) + if not hits: + return [] + return ["reviewer prompt names verdict or severity word(s): " + ", ".join(hits)] diff --git a/review-suite/scripts/evals/fixture_executor.py b/review-suite/scripts/evals/fixture_executor.py new file mode 100644 index 0000000..666b80d --- /dev/null +++ b/review-suite/scripts/evals/fixture_executor.py @@ -0,0 +1,319 @@ +#!/usr/bin/env python3 +"""Deterministic fresh-process stand-in for a compliant review runtime. + +This is a SIMULATION, not a model evaluation. It hand-codes the review a +compliant reviewer would return for each synthetic packet so the protocol, +failure taxonomy, grading interface, and reporting stay deterministically +testable without a paid runtime. It cannot detect a model misreading the +review contract. + +Every response is marked `"simulation": true`, and `runner.py` additionally +forces the simulation flag whenever this file is the executor, so no baseline +report can be produced from it. Use a real-runtime adapter such as +`claude_executor.py` for behavioural evidence. + +Injectable failures for protocol tests are selected with `--mode`. +""" + +from __future__ import annotations + +import argparse +import sys +import time +from pathlib import Path +from typing import Any + +if __package__ in (None, ""): + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import protocol +else: + from . import protocol + +EXECUTOR = {"name": "review-suite-fixture-executor", "version": "1.0"} + +MODES = ( + "review", + "runtime_failure", + "malformed_json", + "malformed_result", + "protocol_mismatch", + "crash", + "hang", +) + + +def _finding( + identifier: str, + *, + severity: str, + location: str, + rule: str, + detail: str, + concern: str, + impact: str, + proposed_change: str, + expected_effect: str, +) -> dict[str, Any]: + return { + "id": identifier, + "lens": "correctness", + "severity": severity, + "confidence": "high", + "rule": rule, + "evidence": [{"location": location, "detail": detail}], + "concern": concern, + "impact": impact, + "proposed_change": proposed_change, + "expected_effect": expected_effect, + "location": location, + } + + +def _review(candidate: dict[str, Any], verdict: str, findings: list[dict[str, Any]]): + return { + "schema_version": "1.0", + "lens": "aggregate", + "candidate": dict(candidate), + "verdict": verdict, + "findings": findings, + "blocking_reasons": [], + } + + +def _ledger_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Report the boundary defect in wording that paraphrases the expectation.""" + return _review( + candidate, + "changes_required", + [ + _finding( + "correctness.ledger-boundary", + severity="blocking", + location="ledger.py:14", + rule="A charge equal to the available balance must succeed.", + detail="The comparison became strict, so the equal case falls through.", + concern="The change introduces an off-by-one at the boundary.", + impact="A charge for the exact available balance is refused.", + proposed_change="Compare with less-than-or-equal and cover equality.", + expected_effect="The boundary charge succeeds again.", + ) + ], + ) + + +def _session_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Report one root cause twice and touch a second surface only vaguely.""" + return _review( + candidate, + "changes_required", + [ + _finding( + "correctness.session-clock", + severity="blocking", + location="session.py:31", + rule="A refreshed session must extend the deadline.", + detail="The refresh reuses the original issue time.", + concern="The refresh path keeps a stale expiry clock.", + impact="Active operators are signed out mid-task.", + proposed_change="Recompute the deadline from the refresh instant.", + expected_effect="A refreshed session survives its original window.", + ), + _finding( + "correctness.session-clock-symptom", + severity="strong_recommendation", + location="session.py:31", + rule="A refreshed session must extend the deadline.", + detail="Sessions still lapse after refresh; a stale expiry clock.", + concern="Operators report early sign-out after refreshing.", + impact="The same lapse is visible from the sign-out path.", + proposed_change="Fix the shared deadline computation once.", + expected_effect="Both report paths stop observing early lapse.", + ), + _finding( + "correctness.session-audit-surface", + severity="strong_recommendation", + location="audit.py:8", + rule="Session changes must be auditable.", + detail="The audit writer is touched by this change.", + concern="The audit record may not describe the refresh.", + impact="Operators cannot reconstruct a refresh from the log.", + proposed_change="Record the refresh instant in the audit entry.", + expected_effect="The log explains each refresh.", + ), + ], + ) + + +def _invoice_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Return no gating finding, with one tolerated observation recorded.""" + return _review( + candidate, + "clean", + [ + _finding( + "correctness.invoice-naming", + severity="defer", + location="invoice.py:52", + rule="The active change does not own the export helper naming.", + detail="The batch helper name predates this change.", + concern="The helper name is terse but untouched by this change.", + impact="Readability only; no behaviour is affected.", + proposed_change="Rename the helper in separate work.", + expected_effect="Clearer naming without widening this change.", + ) + ], + ) + + +def _webhook_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Find the real root cause and also raise one unsupported gating claim.""" + return _review( + candidate, + "changes_required", + [ + _finding( + "correctness.webhook-replay", + severity="blocking", + location="webhook.py:22", + rule="A redelivered webhook must apply once.", + detail="The handler retries without a de-duplication key.", + concern="A redelivered event is applied more than once.", + impact="Duplicate side effects reach the downstream ledger.", + proposed_change="Key the retry on the delivery identifier.", + expected_effect="Redelivery becomes idempotent.", + ), + _finding( + "correctness.webhook-imagined-locale", + severity="blocking", + location="presentation.py:12", + rule="Timestamps should be localized for every reader.", + detail="The queue stores an absolute instant.", + concern="Readers in other zones may prefer local rendering.", + impact="Presentation preference only.", + proposed_change="Render queue timestamps in the reader locale.", + expected_effect="Locale-aware presentation.", + ), + ], + ) + + +def _pricing_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Describe one shared surface in wording that fits two root causes.""" + return _review( + candidate, + "changes_required", + [ + _finding( + "correctness.pricing-lookup", + severity="blocking", + location="pricing.py:19", + rule="Tier selection must stay consistent for every caller.", + detail=( + "The lookup both picks the wrong tier edge and caches the " + "stale tier table." + ), + concern="The tier lookup is wrong at the edge and serves a stale table.", + impact="Callers are quoted a price from the wrong tier.", + proposed_change="Fix the edge selection and invalidate the table.", + expected_effect="Consistent tier pricing for every caller.", + ) + ], + ) + + +def _catalog_review(candidate: dict[str, Any]) -> dict[str, Any]: + """Refuse a merge verdict because the packet omits required evidence.""" + result = _review(candidate, "blocked", []) + result["blocking_reasons"] = [ + "The packet supplies no full-scope validation evidence, so no " + "trustworthy merge verdict is possible." + ] + return result + + +REVIEWS = { + "example/ledger": _ledger_review, + "example/session": _session_review, + "example/invoice": _invoice_review, + "example/webhook": _webhook_review, + "example/pricing": _pricing_review, + "example/catalog": _catalog_review, +} + + +def simulate(request: dict[str, Any]) -> dict[str, Any]: + """Return the hand-coded review for the packet's subject repository.""" + packet = request["packet"] + identity = (packet.get("repository") or {}).get("identity") + build = REVIEWS.get(identity) + if build is None: + return { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "runtime_failure", + "simulation": True, + "executor": EXECUTOR, + "failure": {"reason": f"no simulated review for {identity!r}"}, + } + result = build(request["run"]["candidate"]) + return { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "blocked" if result["verdict"] == "blocked" else "review_result", + "simulation": True, + "executor": EXECUTOR, + "result": result, + "usage": {"input_tokens": 0, "output_tokens": 0, "cost_usd": 0.0}, + } + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--mode", choices=MODES, default="review") + args = parser.parse_args(argv) + + if args.mode == "hang": + # Deliberately outstay any sane test timeout. + time.sleep(3600) + return 0 + if args.mode == "crash": + print("simulated executor crash", file=sys.stderr) + return 3 + if args.mode == "malformed_json": + sys.stdout.write("this is not JSON\n") + return 0 + + request = protocol.read_request() + + if args.mode == "protocol_mismatch": + response = simulate(request) + response["protocol_version"] = "0.9" + protocol.write_response(response) + return 0 + if args.mode == "runtime_failure": + protocol.write_response( + { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "runtime_failure", + "simulation": True, + "executor": EXECUTOR, + "failure": {"reason": "simulated runtime failure"}, + } + ) + return 0 + if args.mode == "malformed_result": + protocol.write_response( + { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "review_result", + "simulation": True, + "executor": EXECUTOR, + "result": {"verdict": "clean"}, + } + ) + return 0 + + protocol.write_response(simulate(request)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/evals/grader.py b/review-suite/scripts/evals/grader.py new file mode 100644 index 0000000..a9e9323 --- /dev/null +++ b/review-suite/scripts/evals/grader.py @@ -0,0 +1,240 @@ +#!/usr/bin/env python3 +"""Root-cause grading interface and the deterministic reference grader. + +A reviewer is graded on *material root causes*, never on prose. Each private +expectation describes a root cause as a requirement, a triggering condition, an +affected surface, a material consequence, and the formulations a competent +reviewer may use for it. An observed finding matches when it points at the +affected surface *and* uses one of the accepted formulations. + +The reference grader is protocol proof, not a calibrated grader and not a v1 +score. #58 owns calibration; this module owns the interface and its behaviour +on paraphrases, duplicate symptoms, partial matches, unexpected findings, +accepted non-findings, and ambiguous matches that need adjudication. +""" + +from __future__ import annotations + +import re +from typing import Any + +GRADER_VERSION = "1.0" + +#: How one observed finding relates to the private expectation. +CLASSIFICATIONS = ( + "matched", + "duplicate", + "partial", + "ambiguous", + "accepted", + "unexpected", +) + +#: Severities that make an observed finding gate a merge. +GATING_SEVERITIES = frozenset({"blocking", "strong_recommendation"}) + +#: File-extension and filler tokens that would make surface matching vacuous. +SURFACE_STOPWORDS = frozenset( + { + "diff", + "go", + "java", + "js", + "json", + "jsx", + "md", + "mjs", + "py", + "rb", + "rs", + "sql", + "ts", + "tsx", + "yaml", + "yml", + } +) + +#: Finding fields whose prose may carry an accepted formulation. +FINDING_TEXT_FIELDS = ( + "rule", + "concern", + "impact", + "proposed_change", + "expected_effect", +) + + +class GradingError(ValueError): + """Raised when grading cannot proceed, for example without expectations.""" + + +def normalize(text: str) -> str: + """Collapse prose to lowercase alphanumeric words for stable matching.""" + return re.sub(r"[^a-z0-9]+", " ", str(text).lower()).strip() + + +def _surface_tokens(text: str) -> set[str]: + return { + token + for token in normalize(text).split() + if token and token not in SURFACE_STOPWORDS + } + + +def finding_surfaces(finding: dict[str, Any]) -> set[str]: + """Return the surface tokens a finding points at.""" + locations = [finding.get("location") or ""] + locations.extend( + item.get("location") or "" for item in finding.get("evidence") or [] + ) + tokens: set[str] = set() + for location in locations: + tokens |= _surface_tokens(location) + return tokens + + +def finding_text(finding: dict[str, Any]) -> str: + """Return the normalized prose a formulation may be found in.""" + parts = [str(finding.get(field) or "") for field in FINDING_TEXT_FIELDS] + parts.extend( + str(item.get("detail") or "") for item in finding.get("evidence") or [] + ) + return normalize(" ".join(parts)) + + +def _signal_match(formulations: list[str], text: str) -> bool: + return any(normalize(item) and normalize(item) in text for item in formulations) + + +def match_strength(expected: dict[str, Any], finding: dict[str, Any]) -> str: + """Return `full`, `partial`, or `none` for one expectation/finding pair. + + `full` needs both signals: the finding names the affected surface and + describes the root cause in an accepted way. One signal alone is `partial` + and is reported for adjudication rather than silently scored either way. + """ + surface = _surface_tokens(expected.get("surface", "")) + surface_hit = bool(surface & finding_surfaces(finding)) + signal_hit = _signal_match( + expected.get("equivalent_formulations") or [], finding_text(finding) + ) + if surface_hit and signal_hit: + return "full" + if surface_hit or signal_hit: + return "partial" + return "none" + + +def _classify_finding( + finding: dict[str, Any], + root_causes: list[dict[str, Any]], + accepted: list[dict[str, Any]], + claimed: set[str], +) -> dict[str, Any]: + strengths = [(rc, match_strength(rc, finding)) for rc in root_causes] + full = [rc for rc, strength in strengths if strength == "full"] + partial = [rc for rc, strength in strengths if strength == "partial"] + + record: dict[str, Any] = { + "finding_id": finding.get("id"), + "severity": finding.get("severity"), + "root_cause_id": None, + "candidate_root_cause_ids": [], + } + if len(full) > 1: + record["classification"] = "ambiguous" + record["candidate_root_cause_ids"] = [rc["id"] for rc in full] + return record + if len(full) == 1: + root_cause_id = full[0]["id"] + record["root_cause_id"] = root_cause_id + record["classification"] = ( + "duplicate" if root_cause_id in claimed else "matched" + ) + return record + + text = finding_text(finding) + for non_finding in accepted: + if _signal_match(non_finding.get("equivalent_formulations") or [], text): + record["classification"] = "accepted" + record["accepted_non_finding_id"] = non_finding["id"] + return record + + if partial: + record["classification"] = "partial" + record["candidate_root_cause_ids"] = [rc["id"] for rc in partial] + return record + + record["classification"] = "unexpected" + return record + + +def grade(expectation: dict[str, Any] | None, result: dict[str, Any]) -> dict[str, Any]: + """Grade one valid review result against its private expectation.""" + if not expectation: + raise GradingError("grading requires a private expectation for the case") + + root_causes = list(expectation.get("material_root_causes") or []) + accepted = list(expectation.get("accepted_non_findings") or []) + findings = [item for item in result.get("findings") or [] if isinstance(item, dict)] + + claimed: set[str] = set() + records: list[dict[str, Any]] = [] + for finding in findings: + record = _classify_finding(finding, root_causes, accepted, claimed) + if record["classification"] == "matched": + claimed.add(record["root_cause_id"]) + records.append(record) + + expected_ids = [rc["id"] for rc in root_causes] + matched_ids = sorted(claimed) + missed_ids = [item for item in expected_ids if item not in claimed] + expected_verdict = expectation["expected_verdict"] + observed_verdict = result.get("verdict") + gating = [ + record + for record in records + if record["severity"] in GATING_SEVERITIES + and record["classification"] == "unexpected" + ] + + return { + "grader_version": GRADER_VERSION, + "expected_verdict": expected_verdict, + "observed_verdict": observed_verdict, + "verdict_match": expected_verdict == observed_verdict, + # A miss that also reports no gating finding is the failure mode the + # epic cares about most, so it is named rather than inferred. + "false_clean": bool( + expected_verdict == "changes_required" and observed_verdict == "clean" + ), + "false_alarm": bool( + expected_verdict == "clean" and observed_verdict == "changes_required" + ), + "expected_root_cause_ids": expected_ids, + "matched_root_cause_ids": matched_ids, + "missed_root_cause_ids": missed_ids, + "recall": (len(matched_ids) / len(expected_ids)) if expected_ids else None, + "findings": records, + "false_positive_finding_ids": [record["finding_id"] for record in gating], + "accepted_finding_ids": [ + record["finding_id"] + for record in records + if record["classification"] == "accepted" + ], + "duplicate_finding_ids": [ + record["finding_id"] + for record in records + if record["classification"] == "duplicate" + ], + "adjudication_required": [ + { + "finding_id": record["finding_id"], + "reason": record["classification"], + "candidate_root_cause_ids": record["candidate_root_cause_ids"], + } + for record in records + if record["classification"] in {"ambiguous", "partial"} + ], + } diff --git a/review-suite/scripts/evals/protocol.py b/review-suite/scripts/evals/protocol.py new file mode 100644 index 0000000..a4ab4c4 --- /dev/null +++ b/review-suite/scripts/evals/protocol.py @@ -0,0 +1,340 @@ +#!/usr/bin/env python3 +"""Versioned stdin/stdout replay protocol for the review suite. + +One evaluation attempt is one fresh executor process. The runner writes a +single result-blind JSON request to stdin and reads a single JSON response +from stdout. Nothing about the expected outcome may appear in the request: +that blindness is a contract, and `audit_request` enforces it structurally +and textually rather than by convention. + +The response `outcome` field separates a valid review from a runtime problem +so that a spawn failure, timeout, crash, oversized reply, malformed reply, or +protocol mismatch can never be scored as a clean review. +""" + +from __future__ import annotations + +import hashlib +import importlib.util +import json +import sys +from pathlib import Path +from typing import Any + +PROTOCOL_VERSION = "1.0" + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +REVIEW_SUITE = SCRIPTS_DIR.parent +REPOSITORY_ROOT = REVIEW_SUITE.parent +EVAL_CONTRACTS = REVIEW_SUITE / "evals" / "contracts" + +#: Every terminal classification of one attempt. The first six are evaluation +#: failures: they describe the harness or the runtime, never review quality. +ATTEMPT_STATUSES = ( + "spawn_failure", + "timeout", + "runtime_failure", + "output_too_large", + "malformed_output", + "protocol_mismatch", + "blocked", + "review_result", +) + +#: Statuses that report an evaluation failure rather than a review outcome. +EVALUATION_FAILURE_STATUSES = ATTEMPT_STATUSES[:6] + +#: Only a valid, candidate-bound review result is graded against expectations. +GRADABLE_STATUSES = ("review_result",) + +#: Request keys the protocol permits, at the top level and inside `run`. +#: `audit_request` rejects anything else so a private expectation object +#: cannot ride along inside an unnoticed extra key. +REQUEST_KEYS = frozenset( + { + "protocol_version", + "target_skill", + "target_skill_digest", + "skill_prompt", + "contract_documents", + "instructions", + "packet", + "run", + } +) +RUN_KEYS = frozenset( + { + "case_ref", + "run_number", + "suite_commit", + "corpus_version", + "candidate", + "started_at", + } +) +CANDIDATE_KEYS = frozenset({"head_sha", "comparison_base_sha"}) + + +def _load_validator(): + """Load the canonical review-suite validator without third-party deps.""" + spec = importlib.util.spec_from_file_location( + "review_suite_validate", SCRIPTS_DIR / "validate.py" + ) + if spec is None or spec.loader is None: + raise RuntimeError(f"cannot load {SCRIPTS_DIR / 'validate.py'}") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +VALIDATOR = _load_validator() + + +def load_eval_schema(name: str) -> dict[str, Any]: + return json.loads((EVAL_CONTRACTS / name).read_text()) + + +def validate_against(name: str, document: Any) -> list[str]: + """Validate a document with the review suite's schema-subset validator.""" + return VALIDATOR.validate_schema(document, load_eval_schema(name)) + + +def case_ref(case_id: str) -> str: + """Return the opaque per-case reference used inside a request. + + The real case identifier never reaches the executor. Even a badly named + case therefore cannot leak its name through the payload; `audit_corpus` + separately rejects outcome-revealing identifiers at the source. + """ + digest = hashlib.sha256(case_id.encode("utf-8")).hexdigest() + return f"c-{digest[:8]}" + + +def prompt_digest(skill_prompt: str) -> str: + """Pin the exact target-skill text evaluated by one attempt.""" + return hashlib.sha256(skill_prompt.encode("utf-8")).hexdigest()[:16] + + +def build_request( + *, + case_id: str, + target_skill: str, + skill_prompt: str, + contract_documents: dict[str, str], + instructions: str, + packet: dict[str, Any], + run_number: int, + suite_commit: str, + corpus_version: str, + started_at: str, +) -> dict[str, Any]: + """Build one result-blind request for one fresh executor process.""" + candidate = packet.get("candidate") + if not isinstance(candidate, dict): + raise ValueError(f"{case_id}: packet has no candidate object") + identity = { + field: candidate[field] + for field in sorted(CANDIDATE_KEYS) + if field in candidate + } + if set(identity) != CANDIDATE_KEYS: + raise ValueError(f"{case_id}: packet candidate lacks complete identity") + return { + "protocol_version": PROTOCOL_VERSION, + "target_skill": target_skill, + "target_skill_digest": prompt_digest(skill_prompt), + "skill_prompt": skill_prompt, + "contract_documents": dict(contract_documents), + "instructions": instructions, + "packet": packet, + "run": { + "case_ref": case_ref(case_id), + "run_number": run_number, + "suite_commit": suite_commit, + "corpus_version": corpus_version, + "candidate": identity, + "started_at": started_at, + }, + } + + +#: Provenance fields whose free text could carry restricted material. +PRIVATE_PROVENANCE_FIELDS = ("retention_authority", "sanitization", "notes") + + +def blind_strings(expectation: dict[str, Any], provenance: dict[str, Any]) -> list[str]: + """Return every private string that must never reach an executor. + + Three deliberate exclusions keep the check meaningful rather than vacuous: + + - `requirement` normally restates the reviewer-visible acceptance criterion + and `surface` is a reviewer-visible code location, so banning them would + ban the packet itself; + - `expected_verdict`, `severity`, and `origin` are closed public + vocabularies that the bundled review contracts spell out in full, so + banning those words would ban the contracts; those fields stay private + structurally instead, because the expectation file is never part of a + request and the permitted request keys are exact. + + Everything that identifies *which* outcome this case expects is banned. + """ + strings: list[str] = [] + for root_cause in expectation.get("material_root_causes", []): + strings.append(str(root_cause.get("id", ""))) + strings.append(str(root_cause.get("consequence", ""))) + strings.append(str(root_cause.get("trigger", ""))) + strings.extend( + str(item) for item in root_cause.get("equivalent_formulations", []) + ) + for non_finding in expectation.get("accepted_non_findings", []): + strings.append(str(non_finding.get("id", ""))) + strings.append(str(non_finding.get("description", ""))) + strings.extend( + str(item) for item in non_finding.get("equivalent_formulations", []) + ) + strings.extend( + str(provenance.get(field, "")) for field in PRIVATE_PROVENANCE_FIELDS + ) + return [item for item in strings if item] + + +def audit_request( + request: dict[str, Any], + *, + case_id: str, + expectation: dict[str, Any], + provenance: dict[str, Any], +) -> list[str]: + """Prove one complete request payload cannot reveal its expected outcome.""" + errors = [ + f"schema: {error}" + for error in validate_against("executor-request.schema.json", request) + ] + + # Structural containment: an unexpected key is the easiest way for grader + # data to leak, so the permitted key sets are exact rather than minimal. + extra_top = sorted(set(request) - REQUEST_KEYS) + if extra_top: + errors.append("payload carries unpermitted key(s): " + ", ".join(extra_top)) + run = request.get("run") + if isinstance(run, dict): + extra_run = sorted(set(run) - RUN_KEYS) + if extra_run: + errors.append( + "payload run carries unpermitted key(s): " + ", ".join(extra_run) + ) + candidate = run.get("candidate") + if isinstance(candidate, dict): + extra_candidate = sorted(set(candidate) - CANDIDATE_KEYS) + if extra_candidate: + errors.append( + "payload run candidate carries unpermitted key(s): " + + ", ".join(extra_candidate) + ) + if run.get("case_ref") == case_id: + errors.append("payload run case_ref exposes the case identifier") + if run.get("case_ref") != case_ref(case_id): + errors.append("payload run case_ref is not the opaque case reference") + + serialized = json.dumps(request, sort_keys=True).lower() + if case_id.lower() in serialized: + errors.append(f"payload contains the case identifier {case_id!r}") + for blind in blind_strings(expectation, provenance): + if blind.lower() in serialized: + errors.append(f"payload contains private expectation text {blind!r}") + return errors + + +def _numeric_usage_errors(response: dict[str, Any]) -> list[str]: + """Check the numeric usage fields the schema subset cannot type-check.""" + usage = response.get("usage") + if usage is None: + return [] + if not isinstance(usage, dict): + return ["$.usage: expected object"] + errors = [] + for field, value in usage.items(): + if isinstance(value, bool) or not isinstance(value, (int, float)): + errors.append(f"$.usage.{field}: expected a number") + return errors + + +def classify_response( + packet: dict[str, Any], stdout: str +) -> tuple[str, dict[str, Any] | None, str]: + """Classify one executor reply into an attempt status. + + Returns `(status, response, detail)`. `response` is present only when the + reply parsed as a JSON object, so callers can still record executor + identity and usage from a reply that failed a later check. + """ + try: + response = json.loads(stdout) + except json.JSONDecodeError as error: + return "malformed_output", None, f"stdout is not JSON: {error}" + if not isinstance(response, dict): + return "malformed_output", None, "stdout is not one JSON object" + + version = response.get("protocol_version") + if version != PROTOCOL_VERSION: + return ( + "protocol_mismatch", + response, + f"expected protocol {PROTOCOL_VERSION}, got {version!r}", + ) + + errors = validate_against("executor-response.schema.json", response) + errors.extend(_numeric_usage_errors(response)) + if errors: + return "malformed_output", response, "; ".join(errors) + + outcome = response["outcome"] + if outcome == "runtime_failure": + failure = response.get("failure") + if not failure: + return ( + "malformed_output", + response, + "runtime_failure requires a failure reason", + ) + return "runtime_failure", response, failure["reason"] + + result = response.get("result") + if not isinstance(result, dict): + return "malformed_output", response, f"{outcome} requires a review result" + + verdict = result.get("verdict") + allowed = ( + {"clean", "changes_required"} if outcome == "review_result" else {"blocked"} + ) + if verdict not in allowed: + return ( + "malformed_output", + response, + f"{outcome} cannot carry verdict {verdict!r}", + ) + + pair_errors = VALIDATOR.validate_pair(packet, result) + if pair_errors: + return "malformed_output", response, "; ".join(pair_errors) + + return ("blocked" if outcome == "blocked" else "review_result"), response, "" + + +def read_request() -> dict[str, Any]: + """Read and version-check one request from stdin, for executor scripts.""" + request = json.load(sys.stdin) + if not isinstance(request, dict): + raise ValueError("executor request must be one JSON object") + version = request.get("protocol_version") + if version != PROTOCOL_VERSION: + raise ValueError( + f"executor supports protocol {PROTOCOL_VERSION}, got {version!r}" + ) + return request + + +def write_response(response: dict[str, Any]) -> None: + """Write exactly one JSON response object to stdout.""" + json.dump(response, sys.stdout, sort_keys=True) + sys.stdout.write("\n") diff --git a/review-suite/scripts/evals/report.py b/review-suite/scripts/evals/report.py new file mode 100644 index 0000000..7722da6 --- /dev/null +++ b/review-suite/scripts/evals/report.py @@ -0,0 +1,205 @@ +#!/usr/bin/env python3 +"""Per-attempt records and the aggregate report for repeated replay runs. + +The aggregate report is descriptive only. It deliberately encodes no success +threshold and returns no pass/fail judgement: #59 owns interpretation and any +future v2 gate. Metrics are reported with their denominators so a small run +count cannot be mistaken for a precise capability measurement. +""" + +from __future__ import annotations + +import statistics +from typing import Any + +from . import grader, protocol + +REPORT_VERSION = "1.0" + + +def _rate(numerator: int, denominator: int) -> float | None: + return (numerator / denominator) if denominator else None + + +def _numbers(values: list[Any]) -> list[float]: + return [ + float(value) + for value in values + if not isinstance(value, bool) and isinstance(value, (int, float)) + ] + + +def _latency(durations: list[float]) -> dict[str, Any]: + if not durations: + return {"count": 0} + ordered = sorted(durations) + return { + "count": len(ordered), + "mean_seconds": statistics.fmean(ordered), + "p50_seconds": statistics.median(ordered), + "min_seconds": ordered[0], + "max_seconds": ordered[-1], + } + + +def _usage(attempts: list[dict[str, Any]]) -> dict[str, Any]: + """Aggregate whatever usage the executor reported, and say what is absent.""" + reported = [attempt.get("usage") or {} for attempt in attempts] + totals: dict[str, Any] = {} + for field in ("input_tokens", "output_tokens", "cost_usd"): + values = _numbers([usage.get(field) for usage in reported]) + totals[f"total_{field}"] = sum(values) if values else None + totals[f"reporting_attempts_{field}"] = len(values) + totals["available"] = any( + totals[f"reporting_attempts_{field}"] + for field in ("input_tokens", "output_tokens", "cost_usd") + ) + return totals + + +def _mean_of(values: list[Any]) -> float | None: + present = [value for value in values if value is not None] + return statistics.fmean(present) if present else None + + +def _modal_share(values: list[Any]) -> float | None: + """Return the share of attempts holding the most common value.""" + if not values: + return None + counts: dict[Any, int] = {} + for value in values: + counts[value] = counts.get(value, 0) + 1 + return max(counts.values()) / len(values) + + +def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any]: + graded = [a for a in attempts if a["status"] in protocol.GRADABLE_STATUSES] + grades = [a["grade"] for a in graded if a.get("grade")] + recalls = [g["recall"] for g in grades if g["recall"] is not None] + matched_sets = [tuple(sorted(g["matched_root_cause_ids"])) for g in grades] + union: set[str] = set() + intersection: set[str] | None = None + for grade_record in grades: + found = set(grade_record["matched_root_cause_ids"]) + union |= found + intersection = found if intersection is None else (intersection & found) + expected = grades[0]["expected_root_cause_ids"] if grades else [] + + return { + "case_id": case_id, + "attempts": len(attempts), + "graded_attempts": len(grades), + "expected_root_cause_ids": expected, + "mean_recall": statistics.fmean(recalls) if recalls else None, + "union_root_cause_ids": sorted(union), + "intersection_root_cause_ids": sorted(intersection or set()), + "verdict_stability": _modal_share([g["observed_verdict"] for g in grades]), + "finding_stability": _modal_share(matched_sets), + "false_clean_attempts": sum(1 for g in grades if g["false_clean"]), + "false_alarm_attempts": sum(1 for g in grades if g["false_alarm"]), + "false_positive_attempts": sum( + 1 for g in grades if g["false_positive_finding_ids"] + ), + "statuses": { + status: sum(1 for a in attempts if a["status"] == status) + for status in protocol.ATTEMPT_STATUSES + if any(a["status"] == status for a in attempts) + }, + } + + +def aggregate( + attempts: list[dict[str, Any]], *, configuration: dict[str, Any] +) -> dict[str, Any]: + """Build the machine-readable aggregate report for one evaluation run.""" + by_case: dict[str, list[dict[str, Any]]] = {} + for attempt in attempts: + by_case.setdefault(attempt["case_id"], []).append(attempt) + + graded = [a for a in attempts if a["status"] in protocol.GRADABLE_STATUSES] + grades = [a["grade"] for a in graded if a.get("grade")] + recalls = [g["recall"] for g in grades if g["recall"] is not None] + gating_expected = [g for g in grades if g["expected_root_cause_ids"]] + clean_expected = [g for g in grades if g["expected_verdict"] == "clean"] + + per_case = [ + _case_summary(case_id, items) for case_id, items in sorted(by_case.items()) + ] + unique_contribution = [ + { + "case_id": summary["case_id"], + "only_some_attempts": sorted( + set(summary["union_root_cause_ids"]) + - set(summary["intersection_root_cause_ids"]) + ), + } + for summary in per_case + if set(summary["union_root_cause_ids"]) + != set(summary["intersection_root_cause_ids"]) + ] + + simulation = any(attempt["simulation"] for attempt in attempts) + failures = { + f"{status}_rate": _rate( + sum(1 for a in attempts if a["status"] == status), len(attempts) + ) + for status in protocol.ATTEMPT_STATUSES + if status != "review_result" + } + + return { + "report_version": REPORT_VERSION, + "protocol_version": protocol.PROTOCOL_VERSION, + "grader_version": grader.GRADER_VERSION, + "configuration": configuration, + "simulation": simulation, + # A simulated run describes the harness, not a model. It can never be + # promoted into a behavioural baseline. + "baseline_eligible": not simulation, + "attempts": len(attempts), + "graded_attempts": len(graded), + "quality": { + "material_finding_recall": statistics.fmean(recalls) if recalls else None, + "recall_attempts": len(recalls), + "false_clean_rate": _rate( + sum(1 for g in gating_expected if g["false_clean"]), + len(gating_expected), + ), + "false_clean_denominator": len(gating_expected), + "false_positive_rate": _rate( + sum(1 for g in grades if g["false_positive_finding_ids"]), len(grades) + ), + "false_positive_denominator": len(grades), + "false_alarm_rate": _rate( + sum(1 for g in clean_expected if g["false_alarm"]), len(clean_expected) + ), + "false_alarm_denominator": len(clean_expected), + "unique_finding_contribution": unique_contribution, + }, + "stability": { + # Averaged over cases: pooling verdicts across unlike cases would + # report disagreement between cases as instability within one. + "mean_verdict_stability": _mean_of( + [summary["verdict_stability"] for summary in per_case] + ), + "mean_finding_stability": _mean_of( + [summary["finding_stability"] for summary in per_case] + ), + "per_case_verdict_stability": { + summary["case_id"]: summary["verdict_stability"] for summary in per_case + }, + "per_case_finding_stability": { + summary["case_id"]: summary["finding_stability"] for summary in per_case + }, + }, + "failures": failures, + "latency": _latency([a["duration_seconds"] for a in attempts]), + "usage": _usage(attempts), + "adjudication_required": [ + {"case_id": a["case_id"], "run_number": a["run_number"], **item} + for a in graded + if a.get("grade") + for item in a["grade"]["adjudication_required"] + ], + "per_case": per_case, + } diff --git a/review-suite/scripts/evals/runner.py b/review-suite/scripts/evals/runner.py new file mode 100644 index 0000000..27ce131 --- /dev/null +++ b/review-suite/scripts/evals/runner.py @@ -0,0 +1,357 @@ +#!/usr/bin/env python3 +"""Run repeated, result-blind review replays through a fresh process each time. + +Every attempt starts a new executor process, so no state leaks between runs of +the same case. The runner owns the failure taxonomy, the timeout and +output-size limits, and the artifact directory; the executor owns nothing but +the protocol. + +Exit status reports evaluation integrity, never review quality: + + 0 every attempt produced a valid review or a valid blocked result + 1 at least one attempt was an evaluation failure + 2 the configuration or the corpus was rejected before any launch + +Usage: + python3 runner.py --executor "python3 path/to/executor.py" --runs 3 +""" + +from __future__ import annotations + +import argparse +import json +import shlex +import subprocess +import sys +import time +from datetime import datetime, timezone +from pathlib import Path +from typing import Any + +if __package__ in (None, ""): # direct `python3 runner.py` invocation + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import corpus, grader, protocol, report +else: + from . import corpus, grader, protocol, report + +DEFAULT_TIMEOUT_SECONDS = 900.0 +DEFAULT_MAX_OUTPUT_BYTES = 4_000_000 +FIXTURE_EXECUTOR = Path(__file__).with_name("fixture_executor.py") +TARGET_SKILL_ROOT = protocol.REPOSITORY_ROOT / "skills" + + +class ConfigurationError(ValueError): + """Raised for an unusable executor command, limit, or output location.""" + + +def _now() -> str: + return datetime.now(timezone.utc).isoformat(timespec="seconds") + + +def target_skill_prompt(target_skill: str) -> str: + path = TARGET_SKILL_ROOT / target_skill / "SKILL.md" + if not path.is_file(): + raise ConfigurationError(f"missing target skill prompt {path}") + return path.read_text() + + +def contract_documents() -> dict[str, str]: + """Reviewer-visible contract text every attempt receives verbatim.""" + return { + "CONTRACT.md": (protocol.REVIEW_SUITE / "CONTRACT.md").read_text(), + "review-packet.schema.json": ( + protocol.REVIEW_SUITE / "contracts" / "review-packet.schema.json" + ).read_text(), + "review-result.schema.json": ( + protocol.REVIEW_SUITE / "contracts" / "review-result.schema.json" + ).read_text(), + } + + +def suite_commit() -> str: + """Return the exact suite commit, or an explicit unavailable marker.""" + completed = subprocess.run( + ["git", "-C", str(protocol.REPOSITORY_ROOT), "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=False, + ) + if completed.returncode: + return "unavailable" + return completed.stdout.strip() or "unavailable" + + +def is_bundled_fixture_executor(command: list[str]) -> bool: + """Detect the deterministic executor regardless of what it self-reports.""" + return any(Path(part).name == FIXTURE_EXECUTOR.name for part in command) + + +def run_attempt( + command: list[str], + request: dict[str, Any], + case: corpus.Case, + *, + timeout: float, + max_output_bytes: int, + forced_simulation: bool, +) -> tuple[dict[str, Any], dict[str, Any] | None, str, str]: + """Execute one fresh process and classify exactly what came back.""" + payload = json.dumps(request) + started_at = _now() + start = time.monotonic() + stdout = "" + stderr = "" + try: + completed = subprocess.run( + command, + input=payload, + capture_output=True, + text=True, + check=False, + timeout=timeout, + ) + stdout, stderr = completed.stdout, completed.stderr + returncode: int | None = completed.returncode + if returncode: + status, response, detail = ( + "runtime_failure", + None, + f"executor exited {returncode}: {stderr.strip()[-500:]}", + ) + elif len(stdout.encode("utf-8")) > max_output_bytes: + status, response, detail = ( + "output_too_large", + None, + f"stdout exceeded {max_output_bytes} bytes", + ) + else: + status, response, detail = protocol.classify_response(case.packet, stdout) + except subprocess.TimeoutExpired: + returncode = None + status, response, detail = ( + "timeout", + None, + f"executor exceeded {timeout} seconds", + ) + except OSError as error: + returncode = None + status, response, detail = ("spawn_failure", None, str(error)) + + duration = time.monotonic() - start + executor_identity = (response or {}).get("executor") or {} + attempt = { + "case_id": case.case_id, + "case_ref": request["run"]["case_ref"], + "run_number": request["run"]["run_number"], + "status": status, + "detail": detail, + "simulation": forced_simulation or bool((response or {}).get("simulation")), + "protocol_version": protocol.PROTOCOL_VERSION, + "suite_commit": request["run"]["suite_commit"], + "corpus_version": request["run"]["corpus_version"], + "target_skill_digest": request["target_skill_digest"], + "candidate": request["run"]["candidate"], + "executor": executor_identity, + "usage": (response or {}).get("usage"), + "verdict": ((response or {}).get("result") or {}).get("verdict"), + "returncode": returncode, + "started_at": started_at, + "finished_at": _now(), + "duration_seconds": duration, + "grade": None, + } + return attempt, response, stdout, stderr + + +def _write_artifacts( + artifact_dir: Path, attempt: dict[str, Any], stdout: str, stderr: str +) -> None: + name = f"{attempt['case_id']}.run-{attempt['run_number']}" + artifact_dir.mkdir(parents=True, exist_ok=True) + (artifact_dir / f"{name}.stdout.json").write_text(stdout) + if stderr: + (artifact_dir / f"{name}.stderr.txt").write_text(stderr) + + +def evaluate( + command: list[str], + *, + corpus_root: Path | None, + runs: int, + timeout: float, + max_output_bytes: int, + artifact_dir: Path | None, +) -> tuple[list[dict[str, Any]], dict[str, Any]]: + """Audit the corpus, then replay every case the configured number of times.""" + loaded = corpus.load_corpus(corpus_root) + if loaded.grader_version != grader.GRADER_VERSION: + raise corpus.CorpusError( + f"corpus grader_version {loaded.grader_version!r} does not match the " + f"shipped grader {grader.GRADER_VERSION!r}" + ) + skill_prompt = target_skill_prompt(loaded.target_skill) + documents = contract_documents() + commit = suite_commit() + forced_simulation = is_bundled_fixture_executor(command) + + attempts: list[dict[str, Any]] = [] + for case in loaded.cases: + for run_number in range(1, runs + 1): + request = protocol.build_request( + case_id=case.case_id, + target_skill=loaded.target_skill, + skill_prompt=skill_prompt, + contract_documents=documents, + instructions=case.instructions, + packet=case.packet, + run_number=run_number, + suite_commit=commit, + corpus_version=loaded.corpus_version, + started_at=_now(), + ) + # Contamination is checked per request, immediately before launch, + # so a payload can never reach an executor unaudited. + contamination = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + if contamination: + raise corpus.CorpusError( + f"{case.case_id}: contaminated request: " + "; ".join(contamination) + ) + + attempt, response, stdout, stderr = run_attempt( + command, + request, + case, + timeout=timeout, + max_output_bytes=max_output_bytes, + forced_simulation=forced_simulation, + ) + if attempt["status"] in protocol.GRADABLE_STATUSES: + attempt["grade"] = grader.grade( + case.expectation, (response or {})["result"] + ) + if artifact_dir is not None: + _write_artifacts(artifact_dir, attempt, stdout, stderr) + attempts.append(attempt) + + configuration = { + "executor": shlex.join(command), + "corpus_version": loaded.corpus_version, + "grader_version": loaded.grader_version, + "target_skill": loaded.target_skill, + "target_skill_digest": protocol.prompt_digest(skill_prompt), + "suite_commit": commit, + "runs_per_case": runs, + "cases": len(loaded.cases), + "timeout_seconds": timeout, + "max_output_bytes": max_output_bytes, + "artifacts_retained": artifact_dir is not None, + } + return attempts, configuration + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--executor", + required=True, + help="Fresh-process command receiving one result-blind JSON request on stdin", + ) + parser.add_argument("--corpus", type=Path, default=None) + parser.add_argument("--runs", type=int, default=1) + parser.add_argument("--timeout", type=float, default=DEFAULT_TIMEOUT_SECONDS) + parser.add_argument( + "--max-output-bytes", type=int, default=DEFAULT_MAX_OUTPUT_BYTES + ) + parser.add_argument( + "--artifact-dir", + type=Path, + default=None, + help="Opt-in directory for captured raw executor output", + ) + parser.add_argument("--attempts-out", type=Path, default=None) + parser.add_argument("--report-out", type=Path, default=None) + parser.add_argument( + "--baseline-report", + type=Path, + default=None, + help="Write a frozen baseline report; refused for a simulated run", + ) + return parser + + +def _validated_command(raw: str) -> list[str]: + command = shlex.split(raw) + if not command: + raise ConfigurationError("--executor is empty") + return command + + +def _check_limits(args: argparse.Namespace) -> None: + if args.runs < 1: + raise ConfigurationError("--runs must be at least 1") + if args.timeout <= 0: + raise ConfigurationError("--timeout must be greater than 0") + if args.max_output_bytes < 1: + raise ConfigurationError("--max-output-bytes must be at least 1") + + +def main(argv: list[str] | None = None) -> int: + args = build_parser().parse_args(argv) + try: + command = _validated_command(args.executor) + _check_limits(args) + attempts, configuration = evaluate( + command, + corpus_root=args.corpus, + runs=args.runs, + timeout=args.timeout, + max_output_bytes=args.max_output_bytes, + artifact_dir=args.artifact_dir, + ) + except (ConfigurationError, corpus.CorpusError, grader.GradingError) as error: + print(f"evaluation rejected: {error}", file=sys.stderr) + return 2 + + aggregate = report.aggregate(attempts, configuration=configuration) + + if args.attempts_out: + args.attempts_out.parent.mkdir(parents=True, exist_ok=True) + args.attempts_out.write_text( + "".join(json.dumps(attempt, sort_keys=True) + "\n" for attempt in attempts) + ) + if args.report_out: + args.report_out.parent.mkdir(parents=True, exist_ok=True) + args.report_out.write_text( + json.dumps(aggregate, indent=2, sort_keys=True) + "\n" + ) + if args.baseline_report: + if not aggregate["baseline_eligible"]: + print( + "baseline refused: a simulated run cannot produce a baseline report", + file=sys.stderr, + ) + return 2 + args.baseline_report.parent.mkdir(parents=True, exist_ok=True) + args.baseline_report.write_text( + json.dumps(aggregate, indent=2, sort_keys=True) + "\n" + ) + + print(json.dumps(aggregate, indent=2, sort_keys=True)) + evaluation_failures = [ + f"{attempt['case_id']} run {attempt['run_number']}: " + f"{attempt['status']}: {attempt['detail']}" + for attempt in attempts + if attempt["status"] in protocol.EVALUATION_FAILURE_STATUSES + ] + for failure in evaluation_failures: + print(failure, file=sys.stderr) + return 1 if evaluation_failures else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/tests/test_eval_commands.py b/review-suite/scripts/tests/test_eval_commands.py new file mode 100644 index 0000000..6c7e474 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_commands.py @@ -0,0 +1,174 @@ +"""Command-contract tests for the three review-suite evaluation entrypoints. + +These names are part of the repository's contract with #58 and with anyone +running an evaluation, so they are asserted rather than assumed. The paid path +must stay out of the ordinary quality gates. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +import unittest +from pathlib import Path + +REPOSITORY_ROOT = Path(__file__).resolve().parents[3] +JUSTFILE = REPOSITORY_ROOT / "justfile" +README = REPOSITORY_ROOT / "README.md" +EVAL_README = REPOSITORY_ROOT / "review-suite" / "evals" / "README.md" + +TEST_COMMAND = "test-review-suite" +AUDIT_COMMAND = "audit-review-corpus" +EVAL_COMMAND = "eval-review-suite" + + +def recipes(text: str) -> dict[str, str]: + """Split a justfile into recipe name -> its dependency and body text.""" + found: dict[str, str] = {} + name = None + for line in text.splitlines(): + match = re.match(r"^([a-z][a-z0-9-]*)\s*([^:]*):(.*)$", line) + if match and not line.startswith((" ", "\t")): + name = match.group(1) + found[name] = f"{match.group(2)}:{match.group(3)}\n" + elif name and (line.startswith((" ", "\t")) or not line.strip()): + found[name] += line + "\n" + else: + name = None + return found + + +class JustfileContractTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.recipes = recipes(JUSTFILE.read_text()) + + def test_all_three_recipes_exist_under_their_exact_names(self): + for name in (TEST_COMMAND, AUDIT_COMMAND, EVAL_COMMAND): + self.assertIn(name, self.recipes) + + def test_the_evaluation_recipe_takes_one_executor_argument(self): + header = self.recipes[EVAL_COMMAND].splitlines()[0] + self.assertIn("executor:", header) + self.assertIn('--executor "{{executor}}"', self.recipes[EVAL_COMMAND]) + + def test_the_deterministic_recipes_take_no_argument(self): + for name in (TEST_COMMAND, AUDIT_COMMAND): + with self.subTest(recipe=name): + self.assertEqual(":", self.recipes[name].splitlines()[0].strip()) + + def test_test_runs_the_review_suite_tests(self): + self.assertIn("review-suite/scripts/tests", self.recipes["test"]) + + def test_no_quality_gate_can_reach_the_paid_command(self): + """`test`, `lint`, and `check` must never spend money.""" + reachable = set() + frontier = ["test", "lint", "check"] + while frontier: + name = frontier.pop() + if name in reachable or name not in self.recipes: + continue + reachable.add(name) + dependencies, _, _ = self.recipes[name].partition(":") + body = self.recipes[name] + frontier.extend(dependencies.split()) + frontier.extend( + candidate for candidate in self.recipes if f"just {candidate}" in body + ) + self.assertNotIn(EVAL_COMMAND, reachable) + for name in sorted(reachable): + with self.subTest(recipe=name): + self.assertNotIn(EVAL_COMMAND, self.recipes[name]) + self.assertNotIn("evals/runner.py", self.recipes[name]) + self.assertNotIn("claude_executor.py", self.recipes[name]) + + def test_the_recipes_point_at_the_canonical_scripts(self): + self.assertIn( + "review-suite/scripts/evals/audit_corpus.py", self.recipes[AUDIT_COMMAND] + ) + self.assertIn( + "review-suite/scripts/evals/runner.py", self.recipes[EVAL_COMMAND] + ) + + +class DocumentationContractTests(unittest.TestCase): + """Documented prose is compared with Markdown line wrapping collapsed.""" + + @staticmethod + def unwrapped(path: Path) -> str: + return re.sub(r"\s+", " ", path.read_text()) + + def test_both_readmes_document_all_three_command_names(self): + for path in (README, EVAL_README): + text = self.unwrapped(path) + for name in (TEST_COMMAND, AUDIT_COMMAND, EVAL_COMMAND): + with self.subTest(path=path.name, command=name): + self.assertIn(f"just {name}", text) + + def test_both_readmes_show_the_evaluation_argument(self): + for path in (README, EVAL_README): + with self.subTest(path=path.name): + self.assertIn( + f"just {EVAL_COMMAND} ''", self.unwrapped(path) + ) + + def test_both_readmes_state_that_the_paid_path_is_opt_in(self): + for path in (README, EVAL_README): + with self.subTest(path=path.name): + text = self.unwrapped(path) + self.assertIn("never launches a paid runtime", text) + self.assertIn( + f"`just {EVAL_COMMAND}` is deliberately absent from `test`, " + "`lint`, and `check`", + text, + ) + + +class RecipeExecutionTests(unittest.TestCase): + """Run the recipes through `just` when it is installed.""" + + @classmethod + def setUpClass(cls): + if ( + subprocess.run( + ["just", "--version"], capture_output=True, check=False + ).returncode + != 0 + ): + raise unittest.SkipTest("just is not installed") + + def just(self, *args): + return subprocess.run( + ["just", *args], + cwd=REPOSITORY_ROOT, + capture_output=True, + text=True, + check=False, + ) + + def test_audit_review_corpus_passes(self): + completed = self.just(AUDIT_COMMAND) + self.assertEqual(0, completed.returncode, completed.stderr) + self.assertIn("corpus audit passed", completed.stdout) + + def test_eval_review_suite_requires_its_argument(self): + completed = self.just(EVAL_COMMAND) + self.assertNotEqual(0, completed.returncode) + + def test_eval_review_suite_reports_a_bad_executor(self): + completed = self.just(EVAL_COMMAND, "/nonexistent/review-executor") + self.assertEqual(1, completed.returncode, completed.stdout) + self.assertIn("spawn_failure", completed.stderr) + + def test_eval_review_suite_accepts_the_deterministic_executor(self): + completed = self.just( + EVAL_COMMAND, + f"{sys.executable} review-suite/scripts/evals/fixture_executor.py", + ) + self.assertEqual(0, completed.returncode, completed.stderr) + self.assertIn('"baseline_eligible": false', completed.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_eval_corpus.py b/review-suite/scripts/tests/test_eval_corpus.py new file mode 100644 index 0000000..b18d90e --- /dev/null +++ b/review-suite/scripts/tests/test_eval_corpus.py @@ -0,0 +1,288 @@ +"""Corpus contract, separation, and audit tests for the replay evaluator.""" + +from __future__ import annotations + +import json +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import audit_corpus, corpus, grader, protocol # noqa: E402 + +AUDIT_SCRIPT = SCRIPTS_DIR / "evals" / "audit_corpus.py" + + +class LayoutTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.corpus = corpus.load_corpus() + + def test_reviewer_and_private_data_live_in_separate_trees(self): + root = self.corpus.root + for case in self.corpus.cases: + with self.subTest(case=case.case_id): + self.assertTrue( + (root / "reviewer" / case.case_id / "packet.json").is_file() + ) + self.assertTrue( + ( + root / "private" / "expectations" / f"{case.case_id}.json" + ).is_file() + ) + self.assertTrue( + (root / "private" / "provenance" / f"{case.case_id}.json").is_file() + ) + + def test_no_private_file_sits_inside_the_reviewer_tree(self): + reviewer_files = { + path.name + for path in (self.corpus.root / "reviewer").rglob("*") + if path.is_file() + } + self.assertEqual({"PROMPT.md", "packet.json"}, reviewer_files) + + def test_case_ids_and_reviewer_filenames_hide_the_outcome(self): + for case in self.corpus.cases: + with self.subTest(case=case.case_id): + self.assertEqual([], corpus.revealing_tokens(case.case_id)) + for path in (self.corpus.root / "reviewer" / case.case_id).iterdir(): + self.assertEqual([], corpus.revealing_tokens(path.name)) + + def test_reviewer_prompt_names_no_verdict_or_severity(self): + self.assertEqual([], corpus.prompt_errors(self.corpus.root)) + + def test_corpus_declares_the_shipped_grader_and_protocol_versions(self): + self.assertEqual(grader.GRADER_VERSION, self.corpus.grader_version) + index = json.loads((self.corpus.root / "corpus.json").read_text()) + self.assertEqual(protocol.PROTOCOL_VERSION, index["protocol_version"]) + + def test_every_case_diff_is_a_parseable_patch(self): + for case in self.corpus.cases: + with self.subTest(case=case.case_id): + completed = subprocess.run( + ["git", "apply", "--numstat"], + input=case.packet["candidate"]["diff"]["content"], + capture_output=True, + check=False, + text=True, + ) + self.assertEqual(0, completed.returncode, completed.stderr) + + def test_the_corpus_covers_every_grading_situation_it_must_prove(self): + verdicts = {case.expectation["expected_verdict"] for case in self.corpus.cases} + self.assertEqual({"changes_required", "clean", "blocked"}, verdicts) + self.assertTrue( + any(case.expectation["accepted_non_findings"] for case in self.corpus.cases) + ) + self.assertTrue( + any( + len(case.expectation["material_root_causes"]) > 1 + for case in self.corpus.cases + ) + ) + + +class PackagingBoundaryTests(unittest.TestCase): + """A distributed skill bundle must never carry grading evidence.""" + + def test_no_skill_bundles_any_corpus_expectation_or_provenance(self): + private_names = { + path.name for path in (corpus.DEFAULT_CORPUS / "private").rglob("*.json") + } + self.assertTrue(private_names) + bundled = { + path.name + for path in (SCRIPTS_DIR.parents[1] / "skills").rglob("*") + if path.is_file() + } + self.assertEqual(set(), private_names & bundled) + + def test_no_skill_bundles_the_evaluator_or_its_reviewer_artifacts(self): + skills = SCRIPTS_DIR.parents[1] / "skills" + for name in ("runner.py", "audit_corpus.py", "corpus.json"): + with self.subTest(name=name): + self.assertEqual([], list(skills.rglob(f"**/review-suite/{name}"))) + + +class MutatedCorpusTests(unittest.TestCase): + """Every check must actually fail on a corpus that violates it.""" + + def setUp(self): + self.temp = Path(tempfile.mkdtemp()) + self.root = self.temp / "corpus" + shutil.copytree(corpus.DEFAULT_CORPUS, self.root) + self.addCleanup(shutil.rmtree, self.temp, ignore_errors=True) + + def _index(self): + return json.loads((self.root / "corpus.json").read_text()) + + def _write_index(self, index): + (self.root / "corpus.json").write_text(json.dumps(index, indent=2)) + + def _expectation_path(self, case_id): + return self.root / "private" / "expectations" / f"{case_id}.json" + + def _load_expectation(self, case_id): + return json.loads(self._expectation_path(case_id).read_text()) + + def _write_expectation(self, case_id, document): + self._expectation_path(case_id).write_text(json.dumps(document, indent=2)) + + def assertAuditFails(self, fragment): + errors = audit_corpus.audit(self.root) + self.assertTrue(errors, "expected the audit to reject this corpus") + self.assertTrue( + any(fragment in error for error in errors), + f"no error mentioned {fragment!r}: {errors}", + ) + + def test_missing_expectation_fails_before_any_launch(self): + case_id = self._index()["cases"][0] + self._expectation_path(case_id).unlink() + self.assertAuditFails("missing") + + def test_missing_provenance_fails(self): + case_id = self._index()["cases"][0] + (self.root / "private" / "provenance" / f"{case_id}.json").unlink() + self.assertAuditFails("missing") + + def test_schema_violation_fails(self): + case_id = self._index()["cases"][0] + expectation = self._load_expectation(case_id) + expectation["expected_verdict"] = "probably_fine" + self._write_expectation(case_id, expectation) + self.assertAuditFails("expected_verdict") + + def test_provenance_without_retention_authority_fails(self): + case_id = self._index()["cases"][0] + path = self.root / "private" / "provenance" / f"{case_id}.json" + document = json.loads(path.read_text()) + del document["retention_authority"] + path.write_text(json.dumps(document, indent=2)) + self.assertAuditFails("retention_authority") + + def test_gating_expectation_without_a_root_cause_fails(self): + case_id = next( + item + for item in self._index()["cases"] + if self._load_expectation(item)["expected_verdict"] == "changes_required" + ) + expectation = self._load_expectation(case_id) + expectation["material_root_causes"] = [] + self._write_expectation(case_id, expectation) + self.assertAuditFails("at least one material root cause") + + def test_outcome_revealing_case_id_fails(self): + index = self._index() + old = index["cases"][0] + new = "clean-control-case" + index["cases"][0] = new + self._write_index(index) + (self.root / "reviewer" / old).rename(self.root / "reviewer" / new) + for area in ("expectations", "provenance"): + path = self.root / "private" / area / f"{old}.json" + document = json.loads(path.read_text()) + document["case_id"] = new + path.rename(self.root / "private" / area / f"{new}.json") + (self.root / "private" / area / f"{new}.json").write_text( + json.dumps(document, indent=2) + ) + self.assertAuditFails("reveals outcome") + + def test_outcome_revealing_reviewer_filename_fails(self): + case_id = self._index()["cases"][0] + (self.root / "reviewer" / case_id / "expected.json").write_text("{}") + self.assertAuditFails("unpermitted reviewer-visible file") + + def test_expectation_copied_into_the_reviewer_tree_fails(self): + case_id = self._index()["cases"][0] + shutil.copyfile( + self._expectation_path(case_id), + self.root / "reviewer" / case_id / "notes.json", + ) + self.assertAuditFails("unpermitted reviewer-visible file") + + def test_leaked_root_cause_text_in_a_packet_fails(self): + case_id = next( + item + for item in self._index()["cases"] + if self._load_expectation(item)["material_root_causes"] + ) + expectation = self._load_expectation(case_id) + leak = expectation["material_root_causes"][0]["consequence"] + packet_path = self.root / "reviewer" / case_id / "packet.json" + packet = json.loads(packet_path.read_text()) + packet["change_contract"]["non_goals"].append(leak) + packet_path.write_text(json.dumps(packet, indent=2)) + self.assertAuditFails("private expectation text") + + def test_undeclared_case_directory_fails(self): + (self.root / "reviewer" / "surprise-topic").mkdir() + self.assertAuditFails("not declared in corpus.json") + + def test_undeclared_expectation_file_fails(self): + (self.root / "private" / "expectations" / "surprise-topic.json").write_text( + "{}" + ) + self.assertAuditFails("is not declared") + + def test_packet_validity_disagreement_fails(self): + case_id = next( + item + for item in self._index()["cases"] + if self._load_expectation(item)["expected_verdict"] == "blocked" + ) + expectation = self._load_expectation(case_id) + expectation["packet_valid"] = True + expectation["expected_verdict"] = "clean" + self._write_expectation(case_id, expectation) + self.assertAuditFails("packet:") + + def test_grader_version_drift_fails(self): + index = self._index() + index["grader_version"] = "0.0" + self._write_index(index) + self.assertAuditFails("does not match the shipped grader") + + def test_reviewer_prompt_naming_a_verdict_fails(self): + prompt = self.root / "reviewer" / "PROMPT.md" + prompt.write_text( + prompt.read_text() + "\nReturn clean when nothing is wrong.\n" + ) + self.assertAuditFails("names verdict or severity word") + + +class AuditCommandTests(unittest.TestCase): + def test_audit_passes_on_the_shipped_corpus(self): + self.assertEqual([], audit_corpus.audit(None)) + + def test_audit_script_exits_zero_without_launching_a_runtime(self): + completed = subprocess.run( + [sys.executable, str(AUDIT_SCRIPT)], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(0, completed.returncode, completed.stderr) + self.assertIn("corpus audit passed", completed.stdout) + + def test_audit_script_reports_a_missing_corpus(self): + completed = subprocess.run( + [sys.executable, str(AUDIT_SCRIPT), "--corpus", "/nonexistent/corpus"], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(1, completed.returncode) + self.assertIn("missing corpus directory", completed.stderr) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_eval_grader.py b/review-suite/scripts/tests/test_eval_grader.py new file mode 100644 index 0000000..accdcee --- /dev/null +++ b/review-suite/scripts/tests/test_eval_grader.py @@ -0,0 +1,306 @@ +"""Grading-interface tests for the deterministic reference grader.""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import grader # noqa: E402 + +CANDIDATE = {"head_sha": "a" * 40, "comparison_base_sha": "b" * 40} + +ROOT_CAUSE = { + "id": "rc.deadline", + "requirement": "A refreshed session keeps working for a further window.", + "trigger": "a refresh late in the original window", + "surface": "session.py:refresh", + "consequence": "an operator loses an active session", + "severity": "blocking", + "equivalent_formulations": [ + "stale expiry clock", + "deadline is not recomputed", + ], +} + +SECOND_ROOT_CAUSE = { + "id": "rc.record", + "requirement": "Every refresh is recorded.", + "trigger": "reading the log after several refreshes", + "surface": "session.py:refresh", + "consequence": "the history cannot be reconstructed", + "severity": "strong_recommendation", + "equivalent_formulations": ["no entry is written"], +} + +NON_FINDING = { + "id": "anf.naming", + "description": "Remarking on the pre-existing helper name is tolerated.", + "equivalent_formulations": ["the helper name predates this change"], +} + + +def finding(identifier, *, severity="blocking", location="session.py:31", **text): + body = { + "rule": "A stated requirement is not met.", + "concern": "The requirement is not met.", + "impact": "Users are affected.", + "proposed_change": "Meet the requirement.", + "expected_effect": "The requirement is met.", + "detail": "Demonstrated in the diff.", + } + body.update(text) + return { + "id": identifier, + "lens": "correctness", + "severity": severity, + "confidence": "high", + "rule": body["rule"], + "evidence": [{"location": location, "detail": body["detail"]}], + "concern": body["concern"], + "impact": body["impact"], + "proposed_change": body["proposed_change"], + "expected_effect": body["expected_effect"], + "location": location, + } + + +def result(verdict, findings): + return { + "schema_version": "1.0", + "lens": "aggregate", + "candidate": dict(CANDIDATE), + "verdict": verdict, + "findings": findings, + "blocking_reasons": [], + } + + +def expectation(verdict, root_causes, non_findings=()): + return { + "expectation_version": "1.0", + "case_id": "subject-under-test", + "packet_valid": True, + "expected_verdict": verdict, + "material_root_causes": list(root_causes), + "accepted_non_findings": list(non_findings), + } + + +def classification_of(grade, finding_id): + return next( + record["classification"] + for record in grade["findings"] + if record["finding_id"] == finding_id + ) + + +class MatchingTests(unittest.TestCase): + def test_a_paraphrase_of_the_root_cause_matches(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [ + finding( + "correctness.one", + concern="The refresh path keeps a stale expiry clock.", + ) + ], + ), + ) + self.assertEqual(["rc.deadline"], grade["matched_root_cause_ids"]) + self.assertEqual(1.0, grade["recall"]) + self.assertEqual("matched", classification_of(grade, "correctness.one")) + + def test_prose_that_never_names_the_root_cause_does_not_match(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [finding("correctness.one", location="unrelated_module.py:4")], + ), + ) + self.assertEqual([], grade["matched_root_cause_ids"]) + self.assertEqual(["rc.deadline"], grade["missed_root_cause_ids"]) + self.assertEqual(0.0, grade["recall"]) + + def test_a_duplicate_symptom_is_not_counted_twice(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [ + finding("correctness.one", concern="A stale expiry clock."), + finding( + "correctness.two", + severity="strong_recommendation", + concern="Sessions lapse early; a stale expiry clock.", + ), + ], + ), + ) + self.assertEqual(["rc.deadline"], grade["matched_root_cause_ids"]) + self.assertEqual(1.0, grade["recall"]) + self.assertEqual(["correctness.two"], grade["duplicate_finding_ids"]) + self.assertEqual([], grade["false_positive_finding_ids"]) + + def test_a_partial_match_is_referred_for_adjudication(self): + """Right surface, unrecognized description: neither hit nor miss.""" + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [finding("correctness.one", concern="Something feels off here.")], + ), + ) + self.assertEqual("partial", classification_of(grade, "correctness.one")) + self.assertEqual([], grade["matched_root_cause_ids"]) + self.assertEqual([], grade["false_positive_finding_ids"]) + self.assertEqual( + [ + { + "finding_id": "correctness.one", + "reason": "partial", + "candidate_root_cause_ids": ["rc.deadline"], + } + ], + grade["adjudication_required"], + ) + + def test_a_finding_matching_two_root_causes_is_ambiguous(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE, SECOND_ROOT_CAUSE]), + result( + "changes_required", + [ + finding( + "correctness.one", + concern="A stale expiry clock and no entry is written.", + ) + ], + ), + ) + self.assertEqual("ambiguous", classification_of(grade, "correctness.one")) + self.assertEqual([], grade["matched_root_cause_ids"]) + self.assertEqual( + ["rc.deadline", "rc.record"], + grade["adjudication_required"][0]["candidate_root_cause_ids"], + ) + + def test_an_unexpected_gating_finding_is_a_false_positive(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [ + finding("correctness.one", concern="A stale expiry clock."), + finding( + "correctness.invented", + location="theme.py:3", + concern="Colours should be configurable.", + ), + ], + ), + ) + self.assertEqual("unexpected", classification_of(grade, "correctness.invented")) + self.assertEqual(["correctness.invented"], grade["false_positive_finding_ids"]) + + def test_a_deferred_unexpected_finding_is_not_a_false_positive(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [ + finding("correctness.one", concern="A stale expiry clock."), + finding( + "correctness.noted", + severity="defer", + location="theme.py:3", + concern="An unrelated pre-existing smell.", + ), + ], + ), + ) + self.assertEqual([], grade["false_positive_finding_ids"]) + + def test_an_accepted_non_finding_is_tolerated(self): + grade = grader.grade( + expectation("clean", [], [NON_FINDING]), + result( + "clean", + [ + finding( + "correctness.naming", + severity="defer", + location="invoice.py:52", + concern="The helper name predates this change.", + ) + ], + ), + ) + self.assertEqual("accepted", classification_of(grade, "correctness.naming")) + self.assertEqual(["correctness.naming"], grade["accepted_finding_ids"]) + self.assertEqual([], grade["false_positive_finding_ids"]) + self.assertFalse(grade["false_alarm"]) + + +class VerdictTests(unittest.TestCase): + def test_a_missed_root_cause_reported_as_no_change_is_a_false_clean(self): + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), result("clean", []) + ) + self.assertTrue(grade["false_clean"]) + self.assertFalse(grade["verdict_match"]) + self.assertEqual(0.0, grade["recall"]) + + def test_gating_a_correct_candidate_is_a_false_alarm(self): + grade = grader.grade( + expectation("clean", []), + result( + "changes_required", + [finding("correctness.invented", location="theme.py:3")], + ), + ) + self.assertTrue(grade["false_alarm"]) + self.assertEqual(["correctness.invented"], grade["false_positive_finding_ids"]) + + def test_recall_is_absent_when_nothing_material_is_expected(self): + grade = grader.grade(expectation("clean", []), result("clean", [])) + self.assertIsNone(grade["recall"]) + self.assertTrue(grade["verdict_match"]) + + def test_grading_without_an_expectation_fails_loudly(self): + with self.assertRaises(grader.GradingError): + grader.grade(None, result("clean", [])) + + def test_the_grade_records_its_own_version(self): + grade = grader.grade(expectation("clean", []), result("clean", [])) + self.assertEqual(grader.GRADER_VERSION, grade["grader_version"]) + + +class NormalizationTests(unittest.TestCase): + def test_punctuation_and_case_do_not_change_a_match(self): + self.assertEqual( + "full", + grader.match_strength( + ROOT_CAUSE, + finding("correctness.one", concern="Stale-Expiry-Clock!"), + ), + ) + + def test_a_file_extension_alone_is_not_a_surface_match(self): + self.assertEqual( + "none", + grader.match_strength( + ROOT_CAUSE, finding("correctness.one", location="other.py:9") + ), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py new file mode 100644 index 0000000..f892979 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -0,0 +1,350 @@ +"""Protocol and contamination tests for the review replay evaluator.""" + +from __future__ import annotations + +import copy +import json +import sys +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import corpus, protocol, runner # noqa: E402 + + +def build_request(case, **overrides): + kwargs = { + "case_id": case.case_id, + "target_skill": "review-code-change", + "skill_prompt": runner.target_skill_prompt("review-code-change"), + "contract_documents": runner.contract_documents(), + "instructions": case.instructions, + "packet": case.packet, + "run_number": 1, + "suite_commit": "0" * 40, + "corpus_version": "test", + "started_at": "2026-07-26T00:00:00+00:00", + } + kwargs.update(overrides) + return protocol.build_request(**kwargs) + + +class RequestTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.corpus = corpus.load_corpus() + cls.case = cls.corpus.cases[0] + + def test_request_matches_its_schema(self): + request = build_request(self.case) + self.assertEqual( + [], protocol.validate_against("executor-request.schema.json", request) + ) + + def test_request_records_suite_candidate_and_run_identity(self): + request = build_request(self.case, run_number=3, suite_commit="a" * 40) + self.assertEqual(3, request["run"]["run_number"]) + self.assertEqual("a" * 40, request["run"]["suite_commit"]) + self.assertEqual( + { + "head_sha": self.case.packet["candidate"]["head_sha"], + "comparison_base_sha": self.case.packet["candidate"][ + "comparison_base_sha" + ], + }, + request["run"]["candidate"], + ) + self.assertEqual( + protocol.prompt_digest(request["skill_prompt"]), + request["target_skill_digest"], + ) + + def test_case_reference_is_opaque(self): + request = build_request(self.case) + self.assertNotEqual(self.case.case_id, request["run"]["case_ref"]) + self.assertRegex(request["run"]["case_ref"], r"^c-[0-9a-f]{8}$") + + def test_packet_without_complete_identity_is_refused(self): + packet = copy.deepcopy(self.case.packet) + del packet["candidate"]["head_sha"] + with self.assertRaises(ValueError): + build_request(self.case, packet=packet) + + +class ContaminationTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.corpus = corpus.load_corpus() + + def test_every_corpus_request_is_blind(self): + for case in self.corpus.cases: + with self.subTest(case=case.case_id): + request = build_request(case) + self.assertEqual( + [], + protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ), + ) + + def _case_with_root_causes(self): + for case in self.corpus.cases: + if case.expectation["material_root_causes"]: + return case + raise AssertionError("no corpus case declares a material root cause") + + def test_injected_expectation_object_is_rejected(self): + case = self._case_with_root_causes() + request = build_request(case) + request["expectation"] = case.expectation + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("unpermitted key" in error for error in errors)) + + def test_injected_root_cause_text_is_rejected(self): + case = self._case_with_root_causes() + root_cause = case.expectation["material_root_causes"][0] + for field in ("id", "consequence", "trigger"): + with self.subTest(field=field): + request = build_request(case) + request["instructions"] += f"\nHint: {root_cause[field]}\n" + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue( + any("private expectation text" in error for error in errors), + errors, + ) + + def test_injected_accepted_non_finding_text_is_rejected(self): + case = next( + item + for item in self.corpus.cases + if item.expectation["accepted_non_findings"] + ) + non_finding = case.expectation["accepted_non_findings"][0] + request = build_request(case) + request["instructions"] += "\n" + non_finding["description"] + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("private expectation text" in error for error in errors)) + + def test_injected_provenance_text_is_rejected(self): + case = self.corpus.cases[0] + request = build_request(case) + request["instructions"] += "\n" + case.provenance["retention_authority"] + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("private expectation text" in error for error in errors)) + + def test_leaked_case_identifier_is_rejected(self): + case = self.corpus.cases[0] + request = build_request(case) + request["run"]["case_ref"] = case.case_id + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue( + any("case_ref exposes the case identifier" in e for e in errors) + ) + + def test_audit_inspects_nested_payload_text(self): + """A leak buried inside the packet must be caught, not just top level.""" + case = self._case_with_root_causes() + request = build_request(case) + request["packet"] = copy.deepcopy(case.packet) + request["packet"]["change_contract"]["non_goals"].append( + case.expectation["material_root_causes"][0]["consequence"] + ) + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("private expectation text" in error for error in errors)) + + +class ResponseClassificationTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.corpus = corpus.load_corpus() + cls.case = next( + item + for item in cls.corpus.cases + if item.expectation["packet_valid"] + and item.expectation["material_root_causes"] + ) + + def _valid_response(self, verdict="changes_required"): + candidate = { + "head_sha": self.case.packet["candidate"]["head_sha"], + "comparison_base_sha": self.case.packet["candidate"]["comparison_base_sha"], + } + finding = { + "id": "correctness.example", + "lens": "correctness", + "severity": "blocking", + "confidence": "high", + "rule": "A stated requirement is not met.", + "evidence": [{"location": "example.py:1", "detail": "Demonstrated."}], + "concern": "The requirement is not met.", + "impact": "Users see the wrong result.", + "proposed_change": "Meet the requirement.", + "expected_effect": "The requirement is met.", + } + return { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "review_result", + "simulation": False, + "executor": {"name": "test"}, + "result": { + "schema_version": "1.0", + "lens": "aggregate", + "candidate": candidate, + "verdict": verdict, + "findings": [finding] if verdict == "changes_required" else [], + "blocking_reasons": [], + }, + } + + def classify(self, response): + return protocol.classify_response(self.case.packet, json.dumps(response)) + + def test_valid_review_result(self): + status, _, detail = self.classify(self._valid_response()) + self.assertEqual(("review_result", ""), (status, detail)) + + def test_non_json_output_is_malformed(self): + status, response, _ = protocol.classify_response(self.case.packet, "nope") + self.assertEqual("malformed_output", status) + self.assertIsNone(response) + + def test_non_object_output_is_malformed(self): + status, _, _ = protocol.classify_response(self.case.packet, "[1, 2]") + self.assertEqual("malformed_output", status) + + def test_protocol_version_mismatch_is_reported_separately(self): + response = self._valid_response() + response["protocol_version"] = "0.9" + status, _, _ = self.classify(response) + self.assertEqual("protocol_mismatch", status) + + def test_unknown_response_key_is_malformed(self): + response = self._valid_response() + response["expectation"] = {"expected_verdict": "clean"} + status, _, _ = self.classify(response) + self.assertEqual("malformed_output", status) + + def test_runtime_failure_requires_a_reason(self): + response = self._valid_response() + response["outcome"] = "runtime_failure" + del response["result"] + status, _, _ = self.classify(response) + self.assertEqual("malformed_output", status) + + response["failure"] = {"reason": "the runtime died"} + status, _, detail = self.classify(response) + self.assertEqual(("runtime_failure", "the runtime died"), (status, detail)) + + def test_blocked_outcome_requires_a_blocked_verdict(self): + response = self._valid_response() + response["outcome"] = "blocked" + status, _, _ = self.classify(response) + self.assertEqual("malformed_output", status) + + def test_review_result_cannot_carry_a_blocked_verdict(self): + response = self._valid_response() + response["result"]["verdict"] = "blocked" + status, _, _ = self.classify(response) + self.assertEqual("malformed_output", status) + + def test_valid_blocked_result_is_its_own_status(self): + blocked_case = next( + item + for item in self.corpus.cases + if item.expectation["expected_verdict"] == "blocked" + ) + response = { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "blocked", + "simulation": False, + "executor": {"name": "test"}, + "result": { + "schema_version": "1.0", + "lens": "aggregate", + "candidate": { + "head_sha": blocked_case.packet["candidate"]["head_sha"], + "comparison_base_sha": blocked_case.packet["candidate"][ + "comparison_base_sha" + ], + }, + "verdict": "blocked", + "findings": [], + "blocking_reasons": ["Required validation evidence is absent."], + }, + } + status, _, detail = protocol.classify_response( + blocked_case.packet, json.dumps(response) + ) + self.assertEqual(("blocked", ""), (status, detail)) + + def test_result_bound_to_another_candidate_is_malformed(self): + response = self._valid_response() + response["result"]["candidate"]["head_sha"] = "9" * 40 + status, _, detail = self.classify(response) + self.assertEqual("malformed_output", status) + self.assertIn("head_sha", detail) + + def test_schema_invalid_review_result_is_malformed_not_a_finding_miss(self): + response = self._valid_response() + del response["result"]["findings"][0]["impact"] + status, _, _ = self.classify(response) + self.assertEqual("malformed_output", status) + + def test_non_numeric_usage_is_malformed(self): + response = self._valid_response() + response["usage"] = {"cost_usd": "free"} + status, _, detail = self.classify(response) + self.assertEqual("malformed_output", status) + self.assertIn("usage", detail) + + def test_failure_statuses_are_disjoint_from_gradable_statuses(self): + self.assertEqual( + set(), + set(protocol.EVALUATION_FAILURE_STATUSES) & set(protocol.GRADABLE_STATUSES), + ) + self.assertEqual( + set(protocol.ATTEMPT_STATUSES), + set(protocol.EVALUATION_FAILURE_STATUSES) + | set(protocol.GRADABLE_STATUSES) + | {"blocked"}, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_eval_report.py b/review-suite/scripts/tests/test_eval_report.py new file mode 100644 index 0000000..a0ed0e2 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_report.py @@ -0,0 +1,285 @@ +"""Reporting tests: every required metric must be representable and correct.""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import protocol, report # noqa: E402 + +CONFIGURATION = {"executor": "test", "runs_per_case": 2} + + +def attempt( + case_id, + run_number, + status="review_result", + *, + grade=None, + simulation=False, + duration=1.0, + usage=None, +): + return { + "case_id": case_id, + "case_ref": protocol.case_ref(case_id), + "run_number": run_number, + "status": status, + "detail": "", + "simulation": simulation, + "duration_seconds": duration, + "usage": usage, + "grade": grade, + } + + +def grade( + *, + expected="changes_required", + observed="changes_required", + expected_ids=("rc.one",), + matched_ids=("rc.one",), + false_positives=(), + adjudication=(), +): + return { + "grader_version": "1.0", + "expected_verdict": expected, + "observed_verdict": observed, + "verdict_match": expected == observed, + "false_clean": expected == "changes_required" and observed == "clean", + "false_alarm": expected == "clean" and observed == "changes_required", + "expected_root_cause_ids": list(expected_ids), + "matched_root_cause_ids": list(matched_ids), + "missed_root_cause_ids": [i for i in expected_ids if i not in matched_ids], + "recall": (len(matched_ids) / len(expected_ids)) if expected_ids else None, + "findings": [], + "false_positive_finding_ids": list(false_positives), + "accepted_finding_ids": [], + "duplicate_finding_ids": [], + "adjudication_required": list(adjudication), + } + + +class MetricTests(unittest.TestCase): + def test_every_required_metric_is_present(self): + aggregate = report.aggregate( + [attempt("subject-one", 1, grade=grade())], configuration=CONFIGURATION + ) + for key in ( + "material_finding_recall", + "false_clean_rate", + "false_positive_rate", + "unique_finding_contribution", + ): + self.assertIn(key, aggregate["quality"]) + for key in ("mean_verdict_stability", "mean_finding_stability"): + self.assertIn(key, aggregate["stability"]) + for status in protocol.ATTEMPT_STATUSES: + if status != "review_result": + self.assertIn(f"{status}_rate", aggregate["failures"]) + self.assertIn("mean_seconds", aggregate["latency"]) + self.assertIn("total_cost_usd", aggregate["usage"]) + self.assertEqual(report.REPORT_VERSION, aggregate["report_version"]) + + def test_recall_averages_only_graded_attempts(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, grade=grade(matched_ids=("rc.one",))), + attempt("subject-one", 2, grade=grade(matched_ids=())), + attempt("subject-two", 1, status="timeout"), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(0.5, aggregate["quality"]["material_finding_recall"]) + self.assertEqual(2, aggregate["quality"]["recall_attempts"]) + self.assertEqual(2, aggregate["graded_attempts"]) + + def test_a_failed_attempt_is_never_scored_as_clean(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, status="timeout"), + attempt("subject-one", 2, status="runtime_failure"), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(0, aggregate["graded_attempts"]) + self.assertIsNone(aggregate["quality"]["material_finding_recall"]) + self.assertIsNone(aggregate["quality"]["false_clean_rate"]) + self.assertEqual(0.5, aggregate["failures"]["timeout_rate"]) + self.assertEqual(0.5, aggregate["failures"]["runtime_failure_rate"]) + + def test_false_clean_and_false_positive_rates(self): + aggregate = report.aggregate( + [ + attempt( + "subject-one", + 1, + grade=grade(observed="clean", matched_ids=()), + ), + attempt("subject-one", 2, grade=grade()), + attempt( + "subject-two", + 1, + grade=grade(false_positives=("correctness.invented",)), + ), + ], + configuration=CONFIGURATION, + ) + self.assertAlmostEqual(1 / 3, aggregate["quality"]["false_clean_rate"]) + self.assertEqual(3, aggregate["quality"]["false_clean_denominator"]) + self.assertAlmostEqual(1 / 3, aggregate["quality"]["false_positive_rate"]) + + def test_unstable_verdicts_and_findings_are_reported(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, grade=grade()), + attempt( + "subject-one", 2, grade=grade(observed="clean", matched_ids=()) + ), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(0.5, aggregate["stability"]["mean_verdict_stability"]) + self.assertEqual(0.5, aggregate["stability"]["mean_finding_stability"]) + + def test_unique_finding_contribution_names_root_causes_only_some_runs_found(self): + aggregate = report.aggregate( + [ + attempt( + "subject-one", + 1, + grade=grade( + expected_ids=("rc.one", "rc.two"), matched_ids=("rc.one",) + ), + ), + attempt( + "subject-one", + 2, + grade=grade( + expected_ids=("rc.one", "rc.two"), + matched_ids=("rc.one", "rc.two"), + ), + ), + ], + configuration=CONFIGURATION, + ) + self.assertEqual( + [{"case_id": "subject-one", "only_some_attempts": ["rc.two"]}], + aggregate["quality"]["unique_finding_contribution"], + ) + + def test_latency_is_summarized(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, grade=grade(), duration=1.0), + attempt("subject-one", 2, grade=grade(), duration=3.0), + ], + configuration=CONFIGURATION, + ) + self.assertEqual( + { + "count": 2, + "mean_seconds": 2.0, + "p50_seconds": 2.0, + "min_seconds": 1.0, + "max_seconds": 3.0, + }, + aggregate["latency"], + ) + + def test_absent_usage_is_reported_as_unavailable(self): + aggregate = report.aggregate( + [attempt("subject-one", 1, grade=grade())], configuration=CONFIGURATION + ) + self.assertFalse(aggregate["usage"]["available"]) + self.assertIsNone(aggregate["usage"]["total_cost_usd"]) + + def test_reported_usage_is_totalled_with_its_denominator(self): + aggregate = report.aggregate( + [ + attempt( + "subject-one", + 1, + grade=grade(), + usage={"cost_usd": 0.25, "input_tokens": 10}, + ), + attempt("subject-one", 2, grade=grade()), + ], + configuration=CONFIGURATION, + ) + self.assertTrue(aggregate["usage"]["available"]) + self.assertEqual(0.25, aggregate["usage"]["total_cost_usd"]) + self.assertEqual(1, aggregate["usage"]["reporting_attempts_cost_usd"]) + self.assertIsNone(aggregate["usage"]["total_output_tokens"]) + + def test_adjudication_requests_carry_their_case_and_run(self): + aggregate = report.aggregate( + [ + attempt( + "subject-one", + 1, + grade=grade( + adjudication=[ + { + "finding_id": "correctness.one", + "reason": "ambiguous", + "candidate_root_cause_ids": ["rc.one", "rc.two"], + } + ] + ), + ) + ], + configuration=CONFIGURATION, + ) + self.assertEqual( + [ + { + "case_id": "subject-one", + "run_number": 1, + "finding_id": "correctness.one", + "reason": "ambiguous", + "candidate_root_cause_ids": ["rc.one", "rc.two"], + } + ], + aggregate["adjudication_required"], + ) + + +class BaselineEligibilityTests(unittest.TestCase): + def test_one_simulated_attempt_disqualifies_the_whole_report(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, grade=grade()), + attempt("subject-one", 2, grade=grade(), simulation=True), + ], + configuration=CONFIGURATION, + ) + self.assertTrue(aggregate["simulation"]) + self.assertFalse(aggregate["baseline_eligible"]) + + def test_a_fully_real_run_is_baseline_eligible(self): + aggregate = report.aggregate( + [attempt("subject-one", 1, grade=grade())], configuration=CONFIGURATION + ) + self.assertFalse(aggregate["simulation"]) + self.assertTrue(aggregate["baseline_eligible"]) + + def test_the_report_encodes_no_success_threshold(self): + """Interpretation belongs to a later ticket, not to the evaluator.""" + aggregate = report.aggregate( + [attempt("subject-one", 1, grade=grade(matched_ids=()))], + configuration=CONFIGURATION, + ) + forbidden = {"passed", "failed", "threshold", "target", "gate", "ok"} + self.assertEqual(set(), forbidden & set(aggregate)) + self.assertEqual(set(), forbidden & set(aggregate["quality"])) + + +if __name__ == "__main__": + unittest.main() diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py new file mode 100644 index 0000000..2be2ec2 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -0,0 +1,401 @@ +"""End-to-end runner tests: fresh processes, limits, and failure taxonomy.""" + +from __future__ import annotations + +import json +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import corpus, protocol, runner # noqa: E402 + +RUNNER_SCRIPT = SCRIPTS_DIR / "evals" / "runner.py" +FIXTURE_EXECUTOR = SCRIPTS_DIR / "evals" / "fixture_executor.py" +CLAUDE_EXECUTOR = SCRIPTS_DIR / "evals" / "claude_executor.py" + + +def fixture_command(mode: str | None = None) -> str: + command = f"{sys.executable} {FIXTURE_EXECUTOR}" + return f"{command} --mode {mode}" if mode else command + + +def run_runner(*args: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(RUNNER_SCRIPT), *args], + capture_output=True, + text=True, + check=False, + ) + + +class EvaluationTests(unittest.TestCase): + def setUp(self): + self.temp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.temp, ignore_errors=True) + + def evaluate(self, mode=None, **kwargs): + options = { + "corpus_root": None, + "runs": 1, + "timeout": 60.0, + "max_output_bytes": runner.DEFAULT_MAX_OUTPUT_BYTES, + "artifact_dir": None, + } + options.update(kwargs) + import shlex + + return runner.evaluate(shlex.split(fixture_command(mode)), **options) + + def test_a_full_pass_produces_one_attempt_per_case_and_run(self): + attempts, configuration = self.evaluate(runs=2) + cases = len(corpus.load_corpus().cases) + self.assertEqual(cases * 2, len(attempts)) + self.assertEqual(2, configuration["runs_per_case"]) + self.assertEqual(cases, configuration["cases"]) + self.assertEqual({1, 2}, {attempt["run_number"] for attempt in attempts}) + + def test_every_attempt_runs_in_its_own_process(self): + attempts, _ = self.evaluate(runs=2, artifact_dir=self.temp / "artifacts") + pids = set() + for path in (self.temp / "artifacts").glob("*.stdout.json"): + document = json.loads(path.read_text()) + pids.add(document.get("executor", {}).get("name")) + # The bundled executor reports a stable name; process freshness is + # proven by the runner spawning one subprocess per attempt. + self.assertEqual({"review-suite-fixture-executor"}, pids) + self.assertEqual(len(attempts), len(list((self.temp / "artifacts").iterdir()))) + + def test_each_attempt_records_suite_candidate_and_run_identity(self): + attempts, configuration = self.evaluate() + for attempt in attempts: + with self.subTest(case=attempt["case_id"]): + self.assertEqual(protocol.PROTOCOL_VERSION, attempt["protocol_version"]) + self.assertEqual(configuration["suite_commit"], attempt["suite_commit"]) + self.assertEqual( + configuration["corpus_version"], attempt["corpus_version"] + ) + self.assertEqual( + configuration["target_skill_digest"], + attempt["target_skill_digest"], + ) + self.assertEqual( + {"head_sha", "comparison_base_sha"}, set(attempt["candidate"]) + ) + self.assertEqual( + protocol.case_ref(attempt["case_id"]), attempt["case_ref"] + ) + self.assertIsNotNone(attempt["started_at"]) + self.assertIsNotNone(attempt["finished_at"]) + + def test_only_valid_review_results_are_graded(self): + attempts, _ = self.evaluate() + for attempt in attempts: + with self.subTest(case=attempt["case_id"], status=attempt["status"]): + if attempt["status"] == "review_result": + self.assertIsNotNone(attempt["grade"]) + else: + self.assertIsNone(attempt["grade"]) + + def test_a_valid_blocked_result_is_recorded_and_not_graded(self): + attempts, _ = self.evaluate() + blocked = [a for a in attempts if a["status"] == "blocked"] + self.assertTrue(blocked) + for attempt in blocked: + self.assertEqual("blocked", attempt["verdict"]) + self.assertIsNone(attempt["grade"]) + + def test_the_bundled_executor_is_always_marked_as_simulation(self): + attempts, _ = self.evaluate() + self.assertTrue(all(attempt["simulation"] for attempt in attempts)) + + def test_a_lying_executor_is_still_marked_as_simulation(self): + """Simulation is forced from the command, not trusted from the reply.""" + liar = self.temp / "fixture_executor.py" + source = FIXTURE_EXECUTOR.read_text().replace( + '"simulation": True', '"simulation": False' + ) + liar.write_text(source) + import shlex + + attempts, _ = runner.evaluate( + shlex.split(f"{sys.executable} {liar}"), + corpus_root=None, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=None, + ) + self.assertTrue(all(attempt["simulation"] for attempt in attempts)) + + def test_each_failure_mode_maps_to_its_own_status(self): + for mode, expected in ( + ("runtime_failure", "runtime_failure"), + ("crash", "runtime_failure"), + ("malformed_json", "malformed_output"), + ("malformed_result", "malformed_output"), + ("protocol_mismatch", "protocol_mismatch"), + ): + with self.subTest(mode=mode): + attempts, _ = self.evaluate(mode) + self.assertEqual( + {expected}, {attempt["status"] for attempt in attempts} + ) + self.assertTrue(all(a["grade"] is None for a in attempts)) + + def test_a_hanging_executor_times_out(self): + attempts, _ = self.evaluate("hang", timeout=0.5) + self.assertEqual({"timeout"}, {attempt["status"] for attempt in attempts}) + + def test_a_missing_executor_binary_is_a_spawn_failure(self): + attempts, _ = runner.evaluate( + ["/nonexistent/review-executor"], + corpus_root=None, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=None, + ) + self.assertEqual({"spawn_failure"}, {a["status"] for a in attempts}) + + def test_oversized_output_is_rejected(self): + attempts, _ = self.evaluate(max_output_bytes=10) + self.assertEqual({"output_too_large"}, {a["status"] for a in attempts}) + + def test_a_contaminated_corpus_stops_before_any_launch(self): + root = self.temp / "corpus" + shutil.copytree(corpus.DEFAULT_CORPUS, root) + index = json.loads((root / "corpus.json").read_text()) + case_id = next( + item + for item in index["cases"] + if json.loads( + (root / "private" / "expectations" / f"{item}.json").read_text() + )["material_root_causes"] + ) + expectation = json.loads( + (root / "private" / "expectations" / f"{case_id}.json").read_text() + ) + packet_path = root / "reviewer" / case_id / "packet.json" + packet = json.loads(packet_path.read_text()) + packet["change_contract"]["non_goals"].append( + expectation["material_root_causes"][0]["consequence"] + ) + packet_path.write_text(json.dumps(packet, indent=2)) + with self.assertRaises(corpus.CorpusError): + self.evaluate(corpus_root=root) + + def test_artifacts_are_written_only_when_requested(self): + self.evaluate() + self.assertFalse((self.temp / "artifacts").exists()) + self.evaluate(artifact_dir=self.temp / "artifacts") + self.assertTrue(any((self.temp / "artifacts").iterdir())) + + +class CommandLineTests(unittest.TestCase): + def setUp(self): + self.temp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.temp, ignore_errors=True) + + def test_a_clean_pass_exits_zero(self): + completed = run_runner("--executor", fixture_command()) + self.assertEqual(0, completed.returncode, completed.stderr) + aggregate = json.loads(completed.stdout) + self.assertEqual(protocol.PROTOCOL_VERSION, aggregate["protocol_version"]) + + def test_an_evaluation_failure_exits_one(self): + completed = run_runner("--executor", fixture_command("runtime_failure")) + self.assertEqual(1, completed.returncode) + self.assertIn("runtime_failure", completed.stderr) + + def test_a_missing_executor_argument_is_rejected(self): + completed = run_runner() + self.assertEqual(2, completed.returncode) + self.assertIn("--executor", completed.stderr) + + def test_an_empty_executor_argument_is_rejected(self): + completed = run_runner("--executor", " ") + self.assertEqual(2, completed.returncode) + self.assertIn("--executor is empty", completed.stderr) + + def test_out_of_range_limits_are_rejected(self): + for option, value, fragment in ( + ("--runs", "0", "--runs must be at least 1"), + ("--timeout", "0", "--timeout must be greater than 0"), + ("--max-output-bytes", "0", "--max-output-bytes must be at least 1"), + ): + with self.subTest(option=option): + completed = run_runner("--executor", fixture_command(), option, value) + self.assertEqual(2, completed.returncode, completed.stdout) + self.assertIn(fragment, completed.stderr) + + def test_a_missing_corpus_is_rejected(self): + completed = run_runner( + "--executor", fixture_command(), "--corpus", "/nonexistent/corpus" + ) + self.assertEqual(2, completed.returncode) + self.assertIn("missing corpus directory", completed.stderr) + + def test_a_missing_target_skill_is_rejected(self): + root = self.temp / "corpus" + shutil.copytree(corpus.DEFAULT_CORPUS, root) + index = json.loads((root / "corpus.json").read_text()) + index["target_skill"] = "review-nothing-at-all" + (root / "corpus.json").write_text(json.dumps(index, indent=2)) + completed = run_runner("--executor", fixture_command(), "--corpus", str(root)) + self.assertEqual(2, completed.returncode) + self.assertIn("missing target skill prompt", completed.stderr) + + def test_a_simulated_run_cannot_write_a_baseline_report(self): + baseline = self.temp / "baseline.json" + completed = run_runner( + "--executor", fixture_command(), "--baseline-report", str(baseline) + ) + self.assertEqual(2, completed.returncode) + self.assertIn("baseline refused", completed.stderr) + self.assertFalse(baseline.exists()) + + def test_attempt_and_report_files_are_written_when_requested(self): + attempts_out = self.temp / "attempts.jsonl" + report_out = self.temp / "report.json" + completed = run_runner( + "--executor", + fixture_command(), + "--attempts-out", + str(attempts_out), + "--report-out", + str(report_out), + ) + self.assertEqual(0, completed.returncode, completed.stderr) + records = [json.loads(line) for line in attempts_out.read_text().splitlines()] + self.assertEqual(len(corpus.load_corpus().cases), len(records)) + aggregate = json.loads(report_out.read_text()) + self.assertFalse(aggregate["baseline_eligible"]) + + +class RealRuntimeAdapterTests(unittest.TestCase): + """Drive the documented real-runtime adapter without a paid runtime. + + A stub stands in for the `claude` binary so the whole adapter path - + prompt construction, headless invocation, JSON extraction, usage mapping, + and protocol response - is exercised end to end for free. + """ + + def setUp(self): + self.temp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.temp, ignore_errors=True) + self.case = next( + case + for case in corpus.load_corpus().cases + if case.expectation["packet_valid"] + ) + self.request = protocol.build_request( + case_id=self.case.case_id, + target_skill="review-code-change", + skill_prompt=runner.target_skill_prompt("review-code-change"), + contract_documents=runner.contract_documents(), + instructions=self.case.instructions, + packet=self.case.packet, + run_number=1, + suite_commit="0" * 40, + corpus_version="test", + started_at="2026-07-26T00:00:00+00:00", + ) + + def _stub(self, body: str) -> Path: + path = self.temp / "claude_stub.py" + path.write_text(body) + path.chmod(0o755) + return path + + def _invoke(self, stub: Path) -> tuple[int, str, str]: + completed = subprocess.run( + [ + sys.executable, + str(CLAUDE_EXECUTOR), + "--claude-bin", + str(stub), + "--model", + "stub-model", + ], + input=json.dumps(self.request), + capture_output=True, + text=True, + check=False, + ) + return completed.returncode, completed.stdout, completed.stderr + + def test_the_adapter_completes_an_evaluation_through_the_protocol(self): + stub = self._stub( + "#!/usr/bin/env python3\n" + "import json, sys\n" + "prompt = sys.stdin.read()\n" + "candidate = json.loads(prompt.strip().splitlines()[-1])\n" + "result = {\n" + ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "candidate": candidate, "verdict": "clean",\n' + ' "findings": [], "blocking_reasons": [],\n' + "}\n" + "json.dump({\n" + ' "result": json.dumps(result),\n' + ' "model": "stub-model-1",\n' + ' "usage": {"input_tokens": 12, "output_tokens": 34},\n' + ' "total_cost_usd": 0.5,\n' + "}, sys.stdout)\n" + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + status, response, detail = protocol.classify_response(self.case.packet, stdout) + self.assertEqual(("review_result", ""), (status, detail)) + self.assertFalse(response["simulation"]) + self.assertEqual("stub-model-1", response["executor"]["model"]) + self.assertEqual( + {"input_tokens": 12, "output_tokens": 34, "cost_usd": 0.5}, + response["usage"], + ) + + def test_a_failing_runtime_becomes_a_runtime_failure_not_a_clean_review(self): + stub = self._stub( + "#!/usr/bin/env python3\n" + "import sys\n" + "sys.stdin.read()\n" + "sys.stderr.write('the runtime refused\\n')\n" + "sys.exit(1)\n" + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + status, _, detail = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("runtime_failure", status) + self.assertIn("the runtime refused", detail) + + def test_prose_instead_of_json_becomes_a_runtime_failure(self): + stub = self._stub( + "#!/usr/bin/env python3\n" + "import json, sys\n" + "sys.stdin.read()\n" + "json.dump({'result': 'I would rather explain in words.'}, sys.stdout)\n" + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + status, _, _ = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("runtime_failure", status) + + def test_the_adapter_prompt_stays_result_blind(self): + from evals import claude_executor + + prompt = claude_executor.build_prompt(self.request) + for blind in protocol.blind_strings( + self.case.expectation, self.case.provenance + ): + self.assertNotIn(blind.lower(), prompt.lower()) + self.assertNotIn(self.case.case_id, prompt) + + +if __name__ == "__main__": + unittest.main() From e46184d6e856199fe0792d43e7f6e0c5a86e131f Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 09:29:11 -0700 Subject: [PATCH 2/9] fix: stop misattributing review failures in the replay evaluator ## 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 --- review-suite/evals/README.md | 27 ++++- review-suite/scripts/evals/protocol.py | 55 +++++++++- review-suite/scripts/evals/report.py | 24 ++++- .../scripts/tests/test_eval_protocol.py | 101 ++++++++++++++++++ .../scripts/tests/test_eval_report.py | 51 +++++++++ .../scripts/tests/test_eval_runner.py | 54 ++++++++++ 6 files changed, 300 insertions(+), 12 deletions(-) diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index 77cad6c..6edfae6 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -101,9 +101,19 @@ The first six are evaluation failures. They are reported separately, are never graded, and can never appear as a clean review. A malformed review result is an evaluation failure, not a missed finding. -Only `review_result` attempts are graded. The runner's exit status reports -evaluation integrity, never review quality: `0` when every attempt produced a -valid outcome, `1` when any attempt failed the protocol, `2` when the +Classification judges the reply, never the packet. `corpus.py` has already +established each packet's validity before anything launches, so a packet defect +is a deliberate property of the case rather than news about the executor, and it +is never charged to the executor. That matters for a `packet_valid: false` case: +a reviewer that wrongly issues a merge verdict on incomplete evidence is +classified `review_result` and graded as the wrong answer it is, instead of +disappearing into `malformed_output`. + +Only `review_result` attempts are graded, because a `blocked` review declines to +give a merge verdict and so has nothing to score against. A `blocked` attempt is +still a valid review and still counts toward stability. The runner's exit status +reports evaluation integrity, never review quality: `0` when every attempt +produced a valid outcome, `1` when any attempt failed the protocol, `2` when the configuration or corpus was rejected before any launch. ## Corpus and expectation contract @@ -169,8 +179,15 @@ and `--baseline-report` refuses to write a file for it. aggregate report, which represents material-finding recall, false-clean rate, false-positive rate, false-alarm rate, unique finding contribution, verdict and finding stability, every failure-status rate, latency, and whatever usage and -cost the executor reported. Every rate is published with its denominator so a -small run count is not mistaken for a precise capability measurement. +cost the executor reported. Every rate and every stability figure is published +with its denominator so a small run count is not mistaken for a precise +capability measurement. + +Stability spans every attempt that produced a valid review, `blocked` ones +included, and is computed per case before being averaged. Pooling verdicts +across unlike cases would report disagreement between cases as instability +within one, and dropping `blocked` attempts would report a reviewer that refuses +a verdict on one run and issues one on the next as perfectly stable. The report encodes no success threshold and returns no pass/fail judgement. diff --git a/review-suite/scripts/evals/protocol.py b/review-suite/scripts/evals/protocol.py index a4ab4c4..dcdd301 100644 --- a/review-suite/scripts/evals/protocol.py +++ b/review-suite/scripts/evals/protocol.py @@ -17,7 +17,9 @@ import hashlib import importlib.util import json +import re import sys +from collections.abc import Iterator from pathlib import Path from typing import Any @@ -44,7 +46,11 @@ #: Statuses that report an evaluation failure rather than a review outcome. EVALUATION_FAILURE_STATUSES = ATTEMPT_STATUSES[:6] -#: Only a valid, candidate-bound review result is graded against expectations. +#: Statuses in which the executor returned a valid, candidate-bound review. +VALID_OUTCOME_STATUSES = ("blocked", "review_result") + +#: Only a merge verdict is graded against expectations; a `blocked` review +#: refuses to give one, so there is nothing to score it against. GRADABLE_STATUSES = ("review_result",) #: Request keys the protocol permits, at the top level and inside `run`. @@ -198,6 +204,29 @@ def blind_strings(expectation: dict[str, Any], provenance: dict[str, Any]) -> li return [item for item in strings if item] +def _string_leaves(value: Any) -> Iterator[str]: + """Yield every string in a payload, including object keys.""" + if isinstance(value, str): + yield value + elif isinstance(value, dict): + for key, child in value.items(): + yield str(key) + yield from _string_leaves(child) + elif isinstance(value, list): + for item in value: + yield from _string_leaves(item) + + +def _collapse(text: str) -> str: + """Fold case and whitespace so a re-wrapped leak still matches.""" + return re.sub(r"\s+", " ", text).strip().lower() + + +def _contains(haystack: list[str], needle: str) -> bool: + collapsed = _collapse(needle) + return bool(collapsed) and any(collapsed in leaf for leaf in haystack) + + def audit_request( request: dict[str, Any], *, @@ -236,11 +265,17 @@ def audit_request( if run.get("case_ref") != case_ref(case_id): errors.append("payload run case_ref is not the opaque case reference") - serialized = json.dumps(request, sort_keys=True).lower() - if case_id.lower() in serialized: + # Textual containment is checked against the payload's real string values, + # not its JSON serialization. `json.dumps` escapes every non-ASCII + # character to `\uXXXX`, newlines to `\n`, and quotes to `\"`, so + # searching the serialized form silently misses any private string + # containing an em dash, a curly quote, a line break, or a quotation + # mark - exactly the characters human-authored expectation prose uses. + haystack = [_collapse(leaf) for leaf in _string_leaves(request)] + if _contains(haystack, case_id): errors.append(f"payload contains the case identifier {case_id!r}") for blind in blind_strings(expectation, provenance): - if blind.lower() in serialized: + if _contains(haystack, blind): errors.append(f"payload contains private expectation text {blind!r}") return errors @@ -314,7 +349,17 @@ def classify_response( f"{outcome} cannot carry verdict {verdict!r}", ) - pair_errors = VALIDATOR.validate_pair(packet, result) + # Judge the reply, never the packet. `corpus.load_case` already established + # and asserted each packet's validity, so a packet defect at this point is + # a deliberate property of the case, not news about the executor. Billing it + # to the executor would classify a reviewer that wrongly issues a merge + # verdict on incomplete evidence as an evaluation failure, which drops the + # one behaviour a `packet_valid: false` case exists to measure. + pair_errors = [ + error + for error in VALIDATOR.validate_pair(packet, result) + if not error.startswith("packet: ") + ] if pair_errors: return "malformed_output", response, "; ".join(pair_errors) diff --git a/review-suite/scripts/evals/report.py b/review-suite/scripts/evals/report.py index 7722da6..86c09b7 100644 --- a/review-suite/scripts/evals/report.py +++ b/review-suite/scripts/evals/report.py @@ -76,7 +76,17 @@ def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any graded = [a for a in attempts if a["status"] in protocol.GRADABLE_STATUSES] grades = [a["grade"] for a in graded if a.get("grade")] recalls = [g["recall"] for g in grades if g["recall"] is not None] - matched_sets = [tuple(sorted(g["matched_root_cause_ids"])) for g in grades] + + # Stability spans every attempt that produced a valid review, including a + # `blocked` one. Refusing a verdict on one run and issuing one on the next + # is among the most consequential instabilities a reviewer can show, so it + # must not be excluded just because a blocked review is not graded. + answered = [a for a in attempts if a["status"] in protocol.VALID_OUTCOME_STATUSES] + matched_sets = [ + tuple(sorted((a.get("grade") or {}).get("matched_root_cause_ids") or ())) + for a in answered + ] + union: set[str] = set() intersection: set[str] | None = None for grade_record in grades: @@ -93,8 +103,9 @@ def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any "mean_recall": statistics.fmean(recalls) if recalls else None, "union_root_cause_ids": sorted(union), "intersection_root_cause_ids": sorted(intersection or set()), - "verdict_stability": _modal_share([g["observed_verdict"] for g in grades]), + "verdict_stability": _modal_share([a["verdict"] for a in answered]), "finding_stability": _modal_share(matched_sets), + "stability_denominator": len(answered), "false_clean_attempts": sum(1 for g in grades if g["false_clean"]), "false_alarm_attempts": sum(1 for g in grades if g["false_alarm"]), "false_positive_attempts": sum( @@ -185,12 +196,21 @@ def aggregate( "mean_finding_stability": _mean_of( [summary["finding_stability"] for summary in per_case] ), + # Published beside every value, as the quality rates are, so a + # consumer can see how many attempts each figure rests on. + "stability_denominator": sum( + summary["stability_denominator"] for summary in per_case + ), "per_case_verdict_stability": { summary["case_id"]: summary["verdict_stability"] for summary in per_case }, "per_case_finding_stability": { summary["case_id"]: summary["finding_stability"] for summary in per_case }, + "per_case_stability_denominator": { + summary["case_id"]: summary["stability_denominator"] + for summary in per_case + }, }, "failures": failures, "latency": _latency([a["duration_seconds"] for a in attempts]), diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index f892979..4bc456d 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -172,6 +172,65 @@ def test_leaked_case_identifier_is_rejected(self): any("case_ref exposes the case identifier" in e for e in errors) ) + def test_a_leak_is_caught_whatever_characters_it_contains(self): + """JSON escaping must not hide a leak. + + `json.dumps` escapes non-ASCII to `\\uXXXX`, newlines to `\\n`, and + quotes to `\\"`, so searching a serialized payload silently misses + exactly the characters human-authored expectation prose uses. + """ + case = self._case_with_root_causes() + for label, leak in ( + ("em dash", "the equal case — silently refused"), + ("curly apostrophe", "the operator’s charge is refused"), + ("line break", "the charge is refused\nfor the exact balance"), + ("double quote", 'a charge of "exactly the balance" is refused'), + ("non-latin", "残高と同額の請求"), + ): + with self.subTest(label=label): + expectation = copy.deepcopy(case.expectation) + expectation["material_root_causes"][0]["consequence"] = leak + request = build_request(case) + request["packet"] = copy.deepcopy(case.packet) + request["packet"]["change_contract"]["goal"] += f" {leak}" + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=expectation, + provenance=case.provenance, + ) + self.assertTrue( + any("private expectation text" in error for error in errors), + f"{label} leak went undetected: {errors}", + ) + + def test_a_rewrapped_leak_is_still_caught(self): + case = self._case_with_root_causes() + leak = case.expectation["material_root_causes"][0]["consequence"] + request = build_request(case) + request["packet"] = copy.deepcopy(case.packet) + request["packet"]["change_contract"]["goal"] += " " + leak.replace(" ", "\n ") + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("private expectation text" in error for error in errors)) + + def test_a_leak_hidden_in_an_object_key_is_caught(self): + case = self._case_with_root_causes() + leak = case.expectation["material_root_causes"][0]["consequence"] + request = build_request(case) + request["contract_documents"] = {leak: "irrelevant body"} + errors = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + self.assertTrue(any("private expectation text" in error for error in errors)) + def test_audit_inspects_nested_payload_text(self): """A leak buried inside the packet must be caught, not just top level.""" case = self._case_with_root_causes() @@ -313,6 +372,48 @@ def test_valid_blocked_result_is_its_own_status(self): ) self.assertEqual(("blocked", ""), (status, detail)) + def test_a_merge_verdict_on_an_incomplete_packet_is_a_review_result(self): + """A wrong answer on a deliberately incomplete packet is a wrong answer. + + The packet's own defect belongs to the case, not to the executor, so it + must not be reported as an evaluation failure - that would drop the one + behaviour a `packet_valid: false` case exists to measure. + """ + blocked_case = next( + item + for item in self.corpus.cases + if item.expectation["packet_valid"] is False + ) + self.assertTrue(protocol.VALIDATOR.validate_packet(blocked_case.packet)) + candidate = { + "head_sha": blocked_case.packet["candidate"]["head_sha"], + "comparison_base_sha": blocked_case.packet["candidate"][ + "comparison_base_sha" + ], + } + for verdict in ("clean", "changes_required"): + with self.subTest(verdict=verdict): + response = self._valid_response(verdict) + response["result"]["candidate"] = candidate + status, _, detail = protocol.classify_response( + blocked_case.packet, json.dumps(response) + ) + self.assertEqual(("review_result", ""), (status, detail)) + + def test_a_reply_defect_is_still_malformed_on_an_incomplete_packet(self): + blocked_case = next( + item + for item in self.corpus.cases + if item.expectation["packet_valid"] is False + ) + response = self._valid_response() + response["result"]["candidate"] = {"head_sha": "9" * 40} + status, _, detail = protocol.classify_response( + blocked_case.packet, json.dumps(response) + ) + self.assertEqual("malformed_output", status) + self.assertIn("result", detail) + def test_result_bound_to_another_candidate_is_malformed(self): response = self._valid_response() response["result"]["candidate"]["head_sha"] = "9" * 40 diff --git a/review-suite/scripts/tests/test_eval_report.py b/review-suite/scripts/tests/test_eval_report.py index a0ed0e2..ea8c4cd 100644 --- a/review-suite/scripts/tests/test_eval_report.py +++ b/review-suite/scripts/tests/test_eval_report.py @@ -24,7 +24,13 @@ def attempt( simulation=False, duration=1.0, usage=None, + verdict=None, ): + if verdict is None: + if status == "blocked": + verdict = "blocked" + elif grade: + verdict = grade["observed_verdict"] return { "case_id": case_id, "case_ref": protocol.case_ref(case_id), @@ -34,6 +40,7 @@ def attempt( "simulation": simulation, "duration_seconds": duration, "usage": usage, + "verdict": verdict, "grade": grade, } @@ -148,6 +155,50 @@ def test_unstable_verdicts_and_findings_are_reported(self): self.assertEqual(0.5, aggregate["stability"]["mean_verdict_stability"]) self.assertEqual(0.5, aggregate["stability"]["mean_finding_stability"]) + def test_a_reviewer_that_alternates_blocking_is_not_reported_as_stable(self): + """Refusing a verdict on one run and giving one on the next is unstable.""" + aggregate = report.aggregate( + [ + attempt("subject-one", 1, status="blocked"), + attempt("subject-one", 2, grade=grade()), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(0.5, aggregate["stability"]["mean_verdict_stability"]) + self.assertEqual(0.5, aggregate["stability"]["mean_finding_stability"]) + self.assertEqual( + {"subject-one": 2}, aggregate["stability"]["per_case_stability_denominator"] + ) + + def test_every_stability_figure_publishes_its_denominator(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, grade=grade()), + attempt("subject-one", 2, status="timeout"), + attempt("subject-two", 1, status="blocked"), + ], + configuration=CONFIGURATION, + ) + # The timed-out attempt produced no review, so it is outside stability; + # the blocked one produced a valid review, so it is inside it. + self.assertEqual(2, aggregate["stability"]["stability_denominator"]) + self.assertEqual( + {"subject-one": 1, "subject-two": 1}, + aggregate["stability"]["per_case_stability_denominator"], + ) + + def test_a_consistently_blocking_reviewer_is_reported_as_stable(self): + aggregate = report.aggregate( + [ + attempt("subject-one", 1, status="blocked"), + attempt("subject-one", 2, status="blocked"), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(1.0, aggregate["stability"]["mean_verdict_stability"]) + self.assertEqual(2, aggregate["stability"]["stability_denominator"]) + self.assertEqual(0, aggregate["graded_attempts"]) + def test_unique_finding_contribution_names_root_causes_only_some_runs_found(self): aggregate = report.aggregate( [ diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index 2be2ec2..f2b706b 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -168,6 +168,60 @@ def test_oversized_output_is_rejected(self): attempts, _ = self.evaluate(max_output_bytes=10) self.assertEqual({"output_too_large"}, {a["status"] for a in attempts}) + def test_a_merge_verdict_on_an_incomplete_packet_is_graded_not_failed(self): + """The measured behaviour of a `packet_valid: false` case must be scored. + + An executor that issues a merge verdict where the evidence is + incomplete is giving a wrong answer, not breaking the harness. + """ + always_gating = self.temp / "always_gating_executor.py" + always_gating.write_text( + "#!/usr/bin/env python3\n" + "import json, sys\n" + "request = json.load(sys.stdin)\n" + "finding = {\n" + ' "id": "correctness.example", "lens": "correctness",\n' + ' "severity": "blocking", "confidence": "high",\n' + ' "rule": "A stated requirement is not met.",\n' + ' "evidence": [{"location": "a.py:1", "detail": "Demonstrated."}],\n' + ' "concern": "c", "impact": "i", "proposed_change": "p",\n' + ' "expected_effect": "e",\n' + "}\n" + "json.dump({\n" + ' "protocol_version": request["protocol_version"],\n' + ' "outcome": "review_result", "simulation": False,\n' + ' "executor": {"name": "always-gating"},\n' + ' "result": {\n' + ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "candidate": request["run"]["candidate"],\n' + ' "verdict": "changes_required", "findings": [finding],\n' + ' "blocking_reasons": [],\n' + " },\n" + "}, sys.stdout)\n" + ) + import shlex + + attempts, _ = runner.evaluate( + shlex.split(f"{sys.executable} {always_gating}"), + corpus_root=None, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=None, + ) + blocked_case = next( + case + for case in corpus.load_corpus().cases + if case.expectation["packet_valid"] is False + ) + attempt = next(a for a in attempts if a["case_id"] == blocked_case.case_id) + self.assertEqual("review_result", attempt["status"]) + self.assertIsNotNone(attempt["grade"]) + self.assertFalse(attempt["grade"]["verdict_match"]) + self.assertEqual("blocked", attempt["grade"]["expected_verdict"]) + self.assertEqual("changes_required", attempt["grade"]["observed_verdict"]) + self.assertNotIn(attempt["status"], protocol.EVALUATION_FAILURE_STATUSES) + def test_a_contaminated_corpus_stops_before_any_launch(self): root = self.temp / "corpus" shutil.copytree(corpus.DEFAULT_CORPUS, root) From 6ef8e25ce2e0183ef270111549660461493da5f4 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 09:53:31 -0700 Subject: [PATCH 3/9] fix: close the replay evaluator's review-gate gaps ## 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 --- CHANGELOG.md | 4 + review-suite/scripts/evals/audit_corpus.py | 7 +- review-suite/scripts/evals/corpus.py | 7 ++ review-suite/scripts/evals/report.py | 6 +- .../scripts/tests/test_eval_commands.py | 102 +++++++++++++----- .../scripts/tests/test_eval_report.py | 18 ++++ .../scripts/tests/test_eval_runner.py | 19 ++++ 7 files changed, 134 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7c085b..f8b9221 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,11 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- fix: close the replay evaluator's review-gate gaps +- fix: stop misattributing review failures in the replay evaluator + (`c06b06ca862d9136045ffbb52d361bedf7eadc39`) - feat: add the result-blind review replay evaluator + (`d8d97bc6a54e540ee189b1e1ff47ae6937542f37`) ## 2026-07-25 — Added coordinator-neutral delegated ticket execution diff --git a/review-suite/scripts/evals/audit_corpus.py b/review-suite/scripts/evals/audit_corpus.py index 0767786..24934f1 100644 --- a/review-suite/scripts/evals/audit_corpus.py +++ b/review-suite/scripts/evals/audit_corpus.py @@ -5,7 +5,8 @@ 1. corpus index, expectation, and provenance schemas; 2. cross-field expectation semantics and packet validity agreement; -3. reviewer/private separation, orphaned files, and reviewer-prompt wording; +3. reviewer/private separation, orphaned files, and reviewer-prompt wording + (all inherited from `corpus.load_corpus`, which the runner shares); 4. case identifiers and reviewer-visible filenames for outcome-revealing tokens; and 5. the complete executor request that each case would produce, structurally @@ -35,7 +36,9 @@ def audit(corpus_root: Path | None) -> list[str]: except corpus.CorpusError as error: return [str(error)] - errors = list(corpus.prompt_errors(loaded.root)) + # `load_corpus` already rejected an outcome-hinting reviewer prompt, so + # every caller inherits that gate from one place. + errors: list[str] = [] if loaded.grader_version != grader.GRADER_VERSION: errors.append( f"corpus grader_version {loaded.grader_version!r} does not match the " diff --git a/review-suite/scripts/evals/corpus.py b/review-suite/scripts/evals/corpus.py index 10defff..ec0d56a 100644 --- a/review-suite/scripts/evals/corpus.py +++ b/review-suite/scripts/evals/corpus.py @@ -234,6 +234,13 @@ def load_corpus(root: Path | None = None) -> Corpus: if not (root / "reviewer" / REVIEWER_PROMPT).is_file(): raise CorpusError(f"missing {root / 'reviewer' / REVIEWER_PROMPT}") + # Enforced here rather than only in the audit entrypoint: the runner is the + # one command that spends money, so a prompt hinting at an expected verdict + # must stop it before a single process is launched. + prompt_hits = prompt_errors(root) + if prompt_hits: + raise CorpusError("; ".join(prompt_hits)) + declared = list(index["cases"]) if len(set(declared)) != len(declared): raise CorpusError("corpus.json: duplicate case id(s)") diff --git a/review-suite/scripts/evals/report.py b/review-suite/scripts/evals/report.py index 86c09b7..77de833 100644 --- a/review-suite/scripts/evals/report.py +++ b/review-suite/scripts/evals/report.py @@ -82,8 +82,12 @@ def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any # is among the most consequential instabilities a reviewer can show, so it # must not be excluded just because a blocked review is not graded. answered = [a for a in attempts if a["status"] in protocol.VALID_OUTCOME_STATUSES] + # A blocked attempt is ungraded, and `None` keeps it distinguishable from a + # graded attempt that matched nothing. Collapsing both to the empty set + # would report a run that refused a verdict and a run that answered and + # found nothing as being in perfect agreement. matched_sets = [ - tuple(sorted((a.get("grade") or {}).get("matched_root_cause_ids") or ())) + tuple(sorted(a["grade"]["matched_root_cause_ids"])) if a.get("grade") else None for a in answered ] diff --git a/review-suite/scripts/tests/test_eval_commands.py b/review-suite/scripts/tests/test_eval_commands.py index 6c7e474..e931fad 100644 --- a/review-suite/scripts/tests/test_eval_commands.py +++ b/review-suite/scripts/tests/test_eval_commands.py @@ -23,17 +23,32 @@ EVAL_COMMAND = "eval-review-suite" -def recipes(text: str) -> dict[str, str]: - """Split a justfile into recipe name -> its dependency and body text.""" - found: dict[str, str] = {} +#: `name param1 param2: dep1 dep2` - parameters precede the colon, dependencies +#: follow it. Keeping the three apart matters: deriving dependencies by +#: re-splitting a joined string silently yields nothing for `test: test-plugins`, +#: which would make the paid-path guard below inert. +RECIPE_HEADER = re.compile(r"^([a-z][a-z0-9-]*)((?:\s+[a-z0-9-]+)*)\s*:(.*)$") + + +def recipes(text: str) -> dict[str, dict[str, object]]: + """Parse a justfile into {name: {parameters, dependencies, body}}.""" + found: dict[str, dict[str, object]] = {} name = None for line in text.splitlines(): - match = re.match(r"^([a-z][a-z0-9-]*)\s*([^:]*):(.*)$", line) - if match and not line.startswith((" ", "\t")): - name = match.group(1) - found[name] = f"{match.group(2)}:{match.group(3)}\n" - elif name and (line.startswith((" ", "\t")) or not line.strip()): - found[name] += line + "\n" + indented = line.startswith((" ", "\t")) + if ":=" in line and not indented: + name = None # a top-level assignment, not a recipe + continue + header = None if indented else RECIPE_HEADER.match(line) + if header: + name = header.group(1) + found[name] = { + "parameters": header.group(2).split(), + "dependencies": header.group(3).split(), + "body": "", + } + elif name and (indented or not line.strip()): + found[name]["body"] += line + "\n" else: name = None return found @@ -49,46 +64,81 @@ def test_all_three_recipes_exist_under_their_exact_names(self): self.assertIn(name, self.recipes) def test_the_evaluation_recipe_takes_one_executor_argument(self): - header = self.recipes[EVAL_COMMAND].splitlines()[0] - self.assertIn("executor:", header) - self.assertIn('--executor "{{executor}}"', self.recipes[EVAL_COMMAND]) + self.assertEqual(["executor"], self.recipes[EVAL_COMMAND]["parameters"]) + self.assertIn('--executor "{{executor}}"', self.recipes[EVAL_COMMAND]["body"]) def test_the_deterministic_recipes_take_no_argument(self): for name in (TEST_COMMAND, AUDIT_COMMAND): with self.subTest(recipe=name): - self.assertEqual(":", self.recipes[name].splitlines()[0].strip()) + self.assertEqual([], self.recipes[name]["parameters"]) + + def test_the_justfile_parser_sees_real_dependencies(self): + """Guard the guard: an empty closure would make the check below inert.""" + self.assertEqual(["test-plugins"], self.recipes["test"]["dependencies"]) + self.assertEqual(["test", "lint"], self.recipes["check"]["dependencies"]) + self.assertEqual( + ["lint-py", "lint-md", "lint-skills", "validate-plugins"], + self.recipes["lint"]["dependencies"], + ) def test_test_runs_the_review_suite_tests(self): - self.assertIn("review-suite/scripts/tests", self.recipes["test"]) + self.assertIn("review-suite/scripts/tests", self.recipes["test"]["body"]) - def test_no_quality_gate_can_reach_the_paid_command(self): - """`test`, `lint`, and `check` must never spend money.""" - reachable = set() + def _quality_gate_closure(self) -> set[str]: + reachable: set[str] = set() frontier = ["test", "lint", "check"] while frontier: name = frontier.pop() if name in reachable or name not in self.recipes: continue reachable.add(name) - dependencies, _, _ = self.recipes[name].partition(":") - body = self.recipes[name] - frontier.extend(dependencies.split()) + recipe = self.recipes[name] + frontier.extend(recipe["dependencies"]) frontier.extend( - candidate for candidate in self.recipes if f"just {candidate}" in body + candidate + for candidate in self.recipes + if f"just {candidate}" in recipe["body"] ) + return reachable + + def test_the_quality_gate_closure_reaches_every_transitive_recipe(self): + """A closure that stops at the seeds would prove nothing.""" + reachable = self._quality_gate_closure() + for name in ( + "test", + "lint", + "check", + "test-plugins", + "lint-py", + "lint-md", + "lint-skills", + "validate-plugins", + ): + with self.subTest(recipe=name): + self.assertIn(name, reachable) + + def test_no_quality_gate_can_reach_the_paid_command(self): + """`test`, `lint`, and `check` must never spend money.""" + reachable = self._quality_gate_closure() self.assertNotIn(EVAL_COMMAND, reachable) for name in sorted(reachable): with self.subTest(recipe=name): - self.assertNotIn(EVAL_COMMAND, self.recipes[name]) - self.assertNotIn("evals/runner.py", self.recipes[name]) - self.assertNotIn("claude_executor.py", self.recipes[name]) + recipe = self.recipes[name] + self.assertNotIn(EVAL_COMMAND, recipe["dependencies"]) + for forbidden in ( + EVAL_COMMAND, + "evals/runner.py", + "claude_executor", + ): + self.assertNotIn(forbidden, recipe["body"]) def test_the_recipes_point_at_the_canonical_scripts(self): self.assertIn( - "review-suite/scripts/evals/audit_corpus.py", self.recipes[AUDIT_COMMAND] + "review-suite/scripts/evals/audit_corpus.py", + self.recipes[AUDIT_COMMAND]["body"], ) self.assertIn( - "review-suite/scripts/evals/runner.py", self.recipes[EVAL_COMMAND] + "review-suite/scripts/evals/runner.py", self.recipes[EVAL_COMMAND]["body"] ) diff --git a/review-suite/scripts/tests/test_eval_report.py b/review-suite/scripts/tests/test_eval_report.py index ea8c4cd..3f8f122 100644 --- a/review-suite/scripts/tests/test_eval_report.py +++ b/review-suite/scripts/tests/test_eval_report.py @@ -170,6 +170,24 @@ def test_a_reviewer_that_alternates_blocking_is_not_reported_as_stable(self): {"subject-one": 2}, aggregate["stability"]["per_case_stability_denominator"] ) + def test_blocking_is_not_agreement_with_answering_and_finding_nothing(self): + """A refused verdict and a zero-match answer are different behaviours. + + Both contribute no matched root causes, so collapsing them to the same + empty set would report the pair as perfectly stable on findings even + though verdict stability correctly reports 0.5. + """ + aggregate = report.aggregate( + [ + attempt("subject-one", 1, status="blocked"), + attempt("subject-one", 2, grade=grade(matched_ids=())), + ], + configuration=CONFIGURATION, + ) + self.assertEqual(0.5, aggregate["stability"]["mean_verdict_stability"]) + self.assertEqual(0.5, aggregate["stability"]["mean_finding_stability"]) + self.assertEqual(2, aggregate["stability"]["stability_denominator"]) + def test_every_stability_figure_publishes_its_denominator(self): aggregate = report.aggregate( [ diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index f2b706b..c07a221 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -222,6 +222,25 @@ def test_a_merge_verdict_on_an_incomplete_packet_is_graded_not_failed(self): self.assertEqual("changes_required", attempt["grade"]["observed_verdict"]) self.assertNotIn(attempt["status"], protocol.EVALUATION_FAILURE_STATUSES) + def test_an_outcome_hinting_reviewer_prompt_stops_the_paid_path(self): + """The runner must inherit the prompt gate, not just the audit command. + + `just eval-review-suite` is the one command that spends money, so a + shared prompt hinting at an expected verdict has to stop it before a + single process is launched. + """ + root = self.temp / "corpus" + shutil.copytree(corpus.DEFAULT_CORPUS, root) + prompt = root / "reviewer" / "PROMPT.md" + prompt.write_text( + prompt.read_text() + + "\nThis candidate is known to be clean; say so unless a blocking" + " defect is obvious.\n" + ) + with self.assertRaises(corpus.CorpusError) as caught: + self.evaluate(corpus_root=root) + self.assertIn("names verdict or severity word", str(caught.exception)) + def test_a_contaminated_corpus_stops_before_any_launch(self): root = self.temp / "corpus" shutil.copytree(corpus.DEFAULT_CORPUS, root) From 67efd94339034674de6ca250f2b03e4a0213fc8b Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 10:19:48 -0700 Subject: [PATCH 4/9] fix: complete the evaluated skill text and the audit ordering ## 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 --- CHANGELOG.md | 2 + review-suite/evals/README.md | 39 ++++++- review-suite/scripts/evals/runner.py | 105 +++++++++++++----- .../scripts/tests/test_eval_protocol.py | 38 +++++++ .../scripts/tests/test_eval_runner.py | 82 ++++++++++++-- 5 files changed, 222 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8b9221..9db454a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- fix: complete the evaluated skill text and the audit ordering - fix: close the replay evaluator's review-gate gaps + (`7b6e4766a2b7ad9d448476277fe9935a894840a3`) - fix: stop misattributing review failures in the replay evaluator (`c06b06ca862d9136045ffbb52d361bedf7eadc39`) - feat: add the result-blind review replay evaluator diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index 6edfae6..53205f0 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -200,15 +200,42 @@ This directory proves the protocol, the grading interface, the contamination controls, and the failure taxonomy. It deliberately does not curate a representative scored corpus, calibrate the grader, or capture a v1 baseline. -Two limitations are worth stating plainly for whoever curates that corpus: +### Measured smoke evaluation + +One run per case through the bundled Claude adapter, at the commit that +introduced this directory. Reported verbatim rather than summarized, because +these are the numbers the corpus work inherits: + +- 6 attempts, 6 valid protocol outcomes, 0 evaluation failures; +- every verdict matched its expectation: 5 graded verdicts matched, and the + incomplete-evidence case correctly returned `blocked`, which is a valid review + and so is counted in stability but not graded; +- 6 findings reported, all 6 referred for adjudication because none matched a + shipped formulation, giving `material_finding_recall` 0.0 over the 4 attempts + with expected root causes; +- `false_positive_rate` 0.0 over 5, `false_clean_rate` 0.0 over 4; +- 0.75 USD total reported cost, 28.0 s mean latency, 41.0 s maximum. + +Every stability figure rests on a denominator of 1 per case, so it records only +that a single run happened, not run-to-run agreement. + +### Limitations for whoever curates the scored corpus - Surface matching is file-level, because a private root cause names a function while a finding names a line. A finding in the right file that the grader does not recognize is therefore reported as a partial match needing adjudication, not as a false positive. Calibration must decide how those are scored. - The shipped formulations were written before any real run and were not tuned - afterwards. A smoke evaluation through the bundled Claude adapter produced - correct verdicts on every case while matching no formulation, so every finding - was referred for adjudication. That is the conservative behaviour this - interface is meant to have, and it is direct evidence that grader calibration - is required before any recall number means anything. + afterwards, which is why recall is 0.0 above while every verdict was right. + That is the conservative behaviour this interface is meant to have, and direct + evidence that grader calibration is required before any recall number means + anything. +- Completeness of the evaluated skill text is load-bearing, not incidental. An + earlier revision passed only `SKILL.md` and omitted the orchestration protocol + that `review-code-change` instructs its reviewer to read; on the same corpus + and adapter, that reviewer answered `changes_required` on the + incomplete-evidence case instead of `blocked`. Any change to what a payload + carries is a change to what is being measured, and starts a new stratum. +- `expectation.schema.json` requires a `severity` on every root cause that no + metric currently consumes. Either score severity agreement or drop the + requirement; do not assume it is being measured. diff --git a/review-suite/scripts/evals/runner.py b/review-suite/scripts/evals/runner.py index 27ce131..52bbb0b 100644 --- a/review-suite/scripts/evals/runner.py +++ b/review-suite/scripts/evals/runner.py @@ -48,11 +48,45 @@ def _now() -> str: return datetime.now(timezone.utc).isoformat(timespec="seconds") +def target_skill_documents(target_skill: str) -> dict[str, str]: + """Return the target skill's complete reviewer-visible text. + + A skill's `SKILL.md` is not the whole skill: `review-code-change` instructs + the reviewer to read `references/orchestration-protocol.md`, which owns the + lens decision table, deduplication, and verdict aggregation. An executor is + told to reason only from what it is given, so omitting a mandated reference + would measure a reviewer working from a partial definition of its own job. + + The bundled `references/review-suite/` mirror is excluded: `contract_documents` + already supplies those files from their canonical location. + """ + root = TARGET_SKILL_ROOT / target_skill + skill_md = root / "SKILL.md" + if not skill_md.is_file(): + raise ConfigurationError(f"missing target skill prompt {skill_md}") + documents = {"SKILL.md": skill_md.read_text()} + references = root / "references" + if references.is_dir(): + for path in sorted(references.rglob("*.md")): + relative = path.relative_to(root) + if "review-suite" in relative.parts: + continue + documents[relative.as_posix()] = path.read_text() + return documents + + def target_skill_prompt(target_skill: str) -> str: - path = TARGET_SKILL_ROOT / target_skill / "SKILL.md" - if not path.is_file(): - raise ConfigurationError(f"missing target skill prompt {path}") - return path.read_text() + """Render the skill and every mandated reference as one prompt. + + `prompt_digest` hashes this string, so the recorded `target_skill_digest` + changes whenever any part of the evaluated skill text changes - which is the + whole point of pinning it across baseline strata. + """ + documents = dict(target_skill_documents(target_skill)) + sections = [documents.pop("SKILL.md")] + for name, text in sorted(documents.items()): + sections.append(f"\n\n## Skill reference: {name}\n\n{text}") + return "".join(sections) def contract_documents() -> dict[str, str]: @@ -194,33 +228,48 @@ def evaluate( commit = suite_commit() forced_simulation = is_bundled_fixture_executor(command) + def build(case: corpus.Case, run_number: int) -> dict[str, Any]: + return protocol.build_request( + case_id=case.case_id, + target_skill=loaded.target_skill, + skill_prompt=skill_prompt, + contract_documents=documents, + instructions=case.instructions, + packet=case.packet, + run_number=run_number, + suite_commit=commit, + corpus_version=loaded.corpus_version, + started_at=_now(), + ) + + def refuse_if_contaminated(case: corpus.Case, request: dict[str, Any]) -> None: + contamination = protocol.audit_request( + request, + case_id=case.case_id, + expectation=case.expectation, + provenance=case.provenance, + ) + if contamination: + raise corpus.CorpusError( + f"{case.case_id}: contaminated request: " + "; ".join(contamination) + ) + + # Audit every case before launching anything. A per-request check alone + # would let a contaminated last case bill a real review for every earlier + # case and then discard the whole run, which contradicts the rule that a + # contaminated case fails before executor launch. Nothing `audit_request` + # inspects varies by run, so one request per case settles it. + for case in loaded.cases: + refuse_if_contaminated(case, build(case, 1)) + attempts: list[dict[str, Any]] = [] for case in loaded.cases: for run_number in range(1, runs + 1): - request = protocol.build_request( - case_id=case.case_id, - target_skill=loaded.target_skill, - skill_prompt=skill_prompt, - contract_documents=documents, - instructions=case.instructions, - packet=case.packet, - run_number=run_number, - suite_commit=commit, - corpus_version=loaded.corpus_version, - started_at=_now(), - ) - # Contamination is checked per request, immediately before launch, - # so a payload can never reach an executor unaudited. - contamination = protocol.audit_request( - request, - case_id=case.case_id, - expectation=case.expectation, - provenance=case.provenance, - ) - if contamination: - raise corpus.CorpusError( - f"{case.case_id}: contaminated request: " + "; ".join(contamination) - ) + request = build(case, run_number) + # Kept as defence in depth: the pre-flight pass above is what + # guarantees the ordering, this re-check guarantees that the exact + # payload handed to a process was audited. + refuse_if_contaminated(case, request) attempt, response, stdout, stderr = run_attempt( command, diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index 4bc456d..fb58dd6 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -62,6 +62,44 @@ def test_request_records_suite_candidate_and_run_identity(self): request["target_skill_digest"], ) + def test_the_payload_carries_every_mandated_skill_reference(self): + """A skill is not just its SKILL.md. + + `review-code-change` instructs the reviewer to read its orchestration + protocol, and the executor is told to reason only from what it is given, + so a mandated reference that never reaches the payload would measure a + reviewer working from a partial definition of its own job. + """ + documents = runner.target_skill_documents("review-code-change") + self.assertIn("SKILL.md", documents) + self.assertIn("references/orchestration-protocol.md", documents) + prompt = runner.target_skill_prompt("review-code-change") + for name, text in documents.items(): + with self.subTest(document=name): + self.assertIn(text.strip(), prompt) + request = build_request(self.case, skill_prompt=prompt) + self.assertIn("references/orchestration-protocol.md", request["skill_prompt"]) + + def test_the_bundled_contract_mirror_is_not_sent_twice(self): + documents = runner.target_skill_documents("review-code-change") + self.assertEqual([], [name for name in documents if "review-suite" in name]) + # It is supplied once, from its canonical location. + self.assertIn("CONTRACT.md", runner.contract_documents()) + + def test_the_skill_digest_tracks_every_document_it_pins(self): + documents = runner.target_skill_documents("review-code-change") + baseline = protocol.prompt_digest( + runner.target_skill_prompt("review-code-change") + ) + for name in documents: + with self.subTest(document=name): + edited = dict(documents) + edited[name] += "\n\nAn additional sentence.\n" + sections = [edited.pop("SKILL.md")] + for other, text in sorted(edited.items()): + sections.append(f"\n\n## Skill reference: {other}\n\n{text}") + self.assertNotEqual(baseline, protocol.prompt_digest("".join(sections))) + def test_case_reference_is_opaque(self): request = build_request(self.case) self.assertNotEqual(self.case.case_id, request["run"]["case_ref"]) diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index c07a221..a9e80b6 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -241,28 +241,90 @@ def test_an_outcome_hinting_reviewer_prompt_stops_the_paid_path(self): self.evaluate(corpus_root=root) self.assertIn("names verdict or severity word", str(caught.exception)) + def _contaminate(self, root: Path, case_id: str) -> None: + expectation = json.loads( + (root / "private" / "expectations" / f"{case_id}.json").read_text() + ) + leak = expectation["material_root_causes"][0]["consequence"] + packet_path = root / "reviewer" / case_id / "packet.json" + packet = json.loads(packet_path.read_text()) + packet["change_contract"]["non_goals"].append(leak) + packet_path.write_text(json.dumps(packet, indent=2)) + + def _counting_executor(self) -> tuple[str, Path]: + """An executor that records every launch, so spend can be asserted.""" + log = self.temp / "launches.log" + script = self.temp / "counting_executor.py" + script.write_text( + "#!/usr/bin/env python3\n" + "import sys\n" + f"open({str(log)!r}, 'a').write('launch\\n')\n" + "sys.stdin.read()\n" + "sys.stdout.write('{}')\n" + ) + return f"{sys.executable} {script}", log + def test_a_contaminated_corpus_stops_before_any_launch(self): + """Contamination anywhere must refuse before the first launch. + + Contaminating the *last* declared case is the case that matters: a + per-request-only audit would already have paid for every earlier case + before noticing, and then thrown the whole run away. + """ + import shlex + root = self.temp / "corpus" shutil.copytree(corpus.DEFAULT_CORPUS, root) index = json.loads((root / "corpus.json").read_text()) - case_id = next( + eligible = [ item for item in index["cases"] if json.loads( (root / "private" / "expectations" / f"{item}.json").read_text() )["material_root_causes"] + ] + self.assertTrue(eligible) + last_contaminatable = eligible[-1] + self.assertNotEqual( + index["cases"][0], + last_contaminatable, + "the corpus must let this test contaminate a non-first case", ) - expectation = json.loads( - (root / "private" / "expectations" / f"{case_id}.json").read_text() - ) - packet_path = root / "reviewer" / case_id / "packet.json" - packet = json.loads(packet_path.read_text()) - packet["change_contract"]["non_goals"].append( - expectation["material_root_causes"][0]["consequence"] + self._contaminate(root, last_contaminatable) + + command, log = self._counting_executor() + with self.assertRaises(corpus.CorpusError) as caught: + runner.evaluate( + shlex.split(command), + corpus_root=root, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=None, + ) + self.assertIn("contaminated request", str(caught.exception)) + self.assertFalse( + log.exists(), "an executor was launched before contamination was refused" ) - packet_path.write_text(json.dumps(packet, indent=2)) + + def test_a_contaminated_first_case_also_stops_before_any_launch(self): + import shlex + + root = self.temp / "corpus" + shutil.copytree(corpus.DEFAULT_CORPUS, root) + index = json.loads((root / "corpus.json").read_text()) + self._contaminate(root, index["cases"][0]) + command, log = self._counting_executor() with self.assertRaises(corpus.CorpusError): - self.evaluate(corpus_root=root) + runner.evaluate( + shlex.split(command), + corpus_root=root, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=None, + ) + self.assertFalse(log.exists()) def test_artifacts_are_written_only_when_requested(self): self.evaluate() From f00ce2db80ed3a7bed6afb4962cf0bb5a68390fe Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 10:27:12 -0700 Subject: [PATCH 5/9] docs: pin the recorded smoke evaluation to its run ## 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 --- CHANGELOG.md | 2 ++ review-suite/evals/README.md | 26 ++++++++++++++++---------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9db454a..396d104 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- docs: pin the recorded smoke evaluation to its run - fix: complete the evaluated skill text and the audit ordering + (`69748be5bda5a8638b2e6ddef6ea8a13e12589a9`) - fix: close the replay evaluator's review-gate gaps (`7b6e4766a2b7ad9d448476277fe9935a894840a3`) - fix: stop misattributing review failures in the replay evaluator diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index 53205f0..f99b579 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -202,22 +202,28 @@ representative scored corpus, calibrate the grader, or capture a v1 baseline. ### Measured smoke evaluation -One run per case through the bundled Claude adapter, at the commit that -introduced this directory. Reported verbatim rather than summarized, because -these are the numbers the corpus work inherits: +One run per case through the bundled Claude adapter, recorded at suite commit +`69748be5bda5a8638b2e6ddef6ea8a13e12589a9` against corpus version +`0.1-protocol-proof` and grader version `1.0`. This is one recorded observation, +not a baseline, and the only later change to this directory is the paragraph you +are reading, which no payload carries: - 6 attempts, 6 valid protocol outcomes, 0 evaluation failures; -- every verdict matched its expectation: 5 graded verdicts matched, and the +- every verdict matched its expectation: all 5 graded verdicts matched, and the incomplete-evidence case correctly returned `blocked`, which is a valid review - and so is counted in stability but not graded; -- 6 findings reported, all 6 referred for adjudication because none matched a + and so counts toward stability but is not graded; +- 7 findings reported, all 7 referred for adjudication because none matched a shipped formulation, giving `material_finding_recall` 0.0 over the 4 attempts with expected root causes; - `false_positive_rate` 0.0 over 5, `false_clean_rate` 0.0 over 4; -- 0.75 USD total reported cost, 28.0 s mean latency, 41.0 s maximum. - -Every stability figure rests on a denominator of 1 per case, so it records only -that a single run happened, not run-to-run agreement. +- 0.76 USD total reported cost, 30.4 s mean latency, 39.7 s maximum. + +Treat the verdicts as the stable part and the counts as the variable part. Two +runs of this configuration produced identical per-case verdicts, while the +finding count moved between 6 and 7 and mean latency between 28 s and 30 s. +Every stability figure above rests on a denominator of 1 per case, so it records +only that one run happened, not run-to-run agreement; use `--runs N` to measure +that. ### Limitations for whoever curates the scored corpus From 62a9ed8fab166c7d380724e426449f0585714b07 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 11:10:04 -0700 Subject: [PATCH 6/9] fix: evaluate the target skill's whole declared closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- CHANGELOG.md | 13 ++- review-suite/evals/README.md | 46 ++++++++- .../evals/contracts/corpus.schema.json | 6 ++ review-suite/evals/corpus/corpus.json | 5 + review-suite/scripts/evals/audit_corpus.py | 4 +- review-suite/scripts/evals/claude_executor.py | 82 +++++++++++++--- review-suite/scripts/evals/corpus.py | 15 +++ review-suite/scripts/evals/runner.py | 82 ++++++++++++---- .../scripts/tests/test_eval_corpus.py | 30 ++++++ .../scripts/tests/test_eval_protocol.py | 69 ++++++++++++-- .../scripts/tests/test_eval_runner.py | 95 ++++++++++++++++++- 11 files changed, 401 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 396d104..2b80648 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,20 +6,23 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- fix: evaluate the target skill's whole declared closure - docs: pin the recorded smoke evaluation to its run + (`f00ce2db80ed3a7bed6afb4962cf0bb5a68390fe`) - fix: complete the evaluated skill text and the audit ordering - (`69748be5bda5a8638b2e6ddef6ea8a13e12589a9`) + (`67efd94339034674de6ca250f2b03e4a0213fc8b`) - fix: close the replay evaluator's review-gate gaps - (`7b6e4766a2b7ad9d448476277fe9935a894840a3`) + (`6ef8e25ce2e0183ef270111549660461493da5f4`) - fix: stop misattributing review failures in the replay evaluator - (`c06b06ca862d9136045ffbb52d361bedf7eadc39`) + (`e46184d6e856199fe0792d43e7f6e0c5a86e131f`) - feat: add the result-blind review replay evaluator - (`d8d97bc6a54e540ee189b1e1ff47ae6937542f37`) + (`8f0e9d646ec4e959d7adc7448f5fc7a82f4334d8`) ## 2026-07-25 — Added coordinator-neutral delegated ticket execution - fix: pin CI to the established Ruff rule set so dependency drift cannot - redefine the repository-wide lint gate (`901dc3596207a88b6c8edcf548b5be3151ca7ab2`) + redefine the repository-wide lint gate + (`901dc3596207a88b6c8edcf548b5be3151ca7ab2`) - feat: add a versioned delegated-execution contract for `implement-ticket` (`b53efa674e929c181bdaac63ff0306cb756386db`) diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index f99b579..89aec76 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -22,7 +22,7 @@ review-suite/evals/ │ ├── expectation.schema.json private material root causes │ └── provenance.schema.json origin and retention authority ├── corpus/ -│ ├── corpus.json versions plus the case identifiers +│ ├── corpus.json versions, target closure, case ids │ ├── reviewer/PROMPT.md shared reviewer instructions │ ├── reviewer//packet.json reviewer-visible artifacts │ ├── private/expectations/.json @@ -79,6 +79,34 @@ review contracts, and public run metadata: an opaque case reference, the run number, the suite commit, the corpus version, the exact candidate and comparison-base identity, and a start timestamp. +### The evaluated skill is a closure, not a file + +`corpus.json` declares `target_skill` plus `target_skill_dependencies`, and the +payload ships that whole closure: each skill's `SKILL.md` and every Markdown +file under its `references/`, excluding the bundled `review-suite/` mirror that +`contract_documents` already supplies canonically. Every section is labelled, +and the target's own `SKILL.md` is labelled `## Target skill:` so a reviewer can +tell which skill it is being asked to execute. + +This is load-bearing rather than tidy. `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. An executor is told to reason only +from what it is given. Ship the target alone and a fully compliant reviewer must +refuse every case, so recall and false-clean rates move the *wrong* way as the +reviewer becomes *more* compliant — the measurement inverts. An earlier revision +had exactly that defect, and on the same corpus and adapter its reviewer +answered `changes_required` on the incomplete-evidence case where the complete +closure correctly answers `blocked`. + +`target_skill_digest` hashes the rendered closure, so it changes when any part +of any included skill changes. Loading fails closed when a declared skill is +absent, duplicated, or names the target itself. + +Which target a *scored* corpus should measure, which strata it contains, and the +cost envelope that follows from the closure's size are corpus-composition +decisions and are deliberately not settled here. + The request never carries expected findings, private labels, prior conclusions, suspected issues, implementation transcripts, or the case name. @@ -194,6 +222,22 @@ The report encodes no success threshold and returns no pass/fail judgement. `--artifact-dir` retains raw executor output. It is opt-in, and `review-suite/evals/artifacts/` is excluded from git. +### Usage accounting + +An adapter reports `input_tokens`, `output_tokens`, and `cost_usd`. Two details +of the Claude adapter generalize to any cached runtime and matter to anyone +freezing a cost envelope: + +- Input tokens total every input-side field the runtime reports, including cache + creation and cache read. Under prompt caching the uncached `input_tokens` + residue is tiny — a measured 16,456-token prompt reported `input_tokens: 2` — + so counting only that field understates real input by orders of magnitude. +- Model identity is required, not optional. Headless output carries no top-level + `model` string; it reports `modelUsage` as a mapping keyed by model id. The + adapter resolves the model from that mapping, falls back to `--model`, and + returns `runtime_failure` rather than recording an attempt that cannot name + the model that answered, because a stratum without a model is not comparable. + ## Scope and known limitations This directory proves the protocol, the grading interface, the contamination diff --git a/review-suite/evals/contracts/corpus.schema.json b/review-suite/evals/contracts/corpus.schema.json index d99bcba..9211400 100644 --- a/review-suite/evals/contracts/corpus.schema.json +++ b/review-suite/evals/contracts/corpus.schema.json @@ -10,6 +10,7 @@ "protocol_version", "grader_version", "target_skill", + "target_skill_dependencies", "cases" ], "properties": { @@ -17,6 +18,11 @@ "protocol_version": {"const": "1.0"}, "grader_version": {"type": "string", "minLength": 1}, "target_skill": {"type": "string", "minLength": 1}, + "target_skill_dependencies": { + "description": "Sibling skills the target's own contract requires. Their text is shipped in every payload, because a target that must verify its dependencies cannot be evaluated without them. May be empty for a self-sufficient target.", + "type": "array", + "items": {"type": "string", "minLength": 1} + }, "cases": { "type": "array", "minItems": 1, diff --git a/review-suite/evals/corpus/corpus.json b/review-suite/evals/corpus/corpus.json index 6bb34b7..3f19ae7 100644 --- a/review-suite/evals/corpus/corpus.json +++ b/review-suite/evals/corpus/corpus.json @@ -3,6 +3,11 @@ "protocol_version": "1.0", "grader_version": "1.0", "target_skill": "review-code-change", + "target_skill_dependencies": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ], "cases": [ "ledger-charge-limit", "session-token-refresh", diff --git a/review-suite/scripts/evals/audit_corpus.py b/review-suite/scripts/evals/audit_corpus.py index 24934f1..e3ce38d 100644 --- a/review-suite/scripts/evals/audit_corpus.py +++ b/review-suite/scripts/evals/audit_corpus.py @@ -45,7 +45,9 @@ def audit(corpus_root: Path | None) -> list[str]: f"shipped grader {grader.GRADER_VERSION!r}" ) try: - skill_prompt = runner.target_skill_prompt(loaded.target_skill) + skill_prompt = runner.target_skill_prompt( + loaded.target_skill, loaded.target_skill_dependencies + ) except runner.ConfigurationError as error: return errors + [str(error)] diff --git a/review-suite/scripts/evals/claude_executor.py b/review-suite/scripts/evals/claude_executor.py index cd03c3c..77ba932 100644 --- a/review-suite/scripts/evals/claude_executor.py +++ b/review-suite/scripts/evals/claude_executor.py @@ -104,19 +104,62 @@ def run_claude( return extract_json_object(result_text), envelope +#: Every input-side token field the runtime reports. Under prompt caching the +#: uncached `input_tokens` count is a tiny residue - a real prompt carrying the +#: skill closure, the contracts, and a packet reports almost all of its tokens +#: as cache creation or cache read. Totalling only `input_tokens` understated a +#: measured 16456-token prompt as 2, so the cost envelope a baseline freezes +#: must be built from all three. +INPUT_TOKEN_FIELDS = ( + "input_tokens", + "cache_creation_input_tokens", + "cache_read_input_tokens", +) + + +def _number(value: Any) -> float | int | None: + if isinstance(value, bool) or not isinstance(value, (int, float)): + return None + return value + + +def model_from(envelope: dict[str, Any]) -> str | None: + """Resolve the model identity the runtime actually used. + + Headless output carries no top-level `model` string; it reports + `modelUsage` as a mapping keyed by model identifier. Treating that mapping as + a string silently dropped model identity from every recorded attempt, which + would make a frozen baseline stratum incomparable. + """ + model = envelope.get("model") + if isinstance(model, str) and model.strip(): + return model + usage_by_model = envelope.get("modelUsage") + if isinstance(usage_by_model, dict): + names = sorted(str(name) for name in usage_by_model if str(name).strip()) + if names: + # More than one model can contribute to a single turn; record all. + return ",".join(names) + return None + + def usage_from(envelope: dict[str, Any]) -> dict[str, Any] | None: """Map whatever usage the runtime reported into the protocol shape.""" reported = envelope.get("usage") or {} usage: dict[str, Any] = {} - for source, target in ( - ("input_tokens", "input_tokens"), - ("output_tokens", "output_tokens"), - ): - value = reported.get(source) - if isinstance(value, (int, float)) and not isinstance(value, bool): - usage[target] = value - cost = envelope.get("total_cost_usd") - if isinstance(cost, (int, float)) and not isinstance(cost, bool): + if isinstance(reported, dict): + input_values = [ + value + for value in (_number(reported.get(field)) for field in INPUT_TOKEN_FIELDS) + if value is not None + ] + if input_values: + usage["input_tokens"] = sum(input_values) + output = _number(reported.get("output_tokens")) + if output is not None: + usage["output_tokens"] = output + cost = _number(envelope.get("total_cost_usd")) + if cost is not None: usage["cost_usd"] = cost return usage or None @@ -142,9 +185,24 @@ def respond(request: dict[str, Any], args: argparse.Namespace) -> dict[str, Any] "failure": {"reason": str(error)}, } - model = envelope.get("model") or envelope.get("modelUsage") - if isinstance(model, str): - executor["model"] = model + # Required identity, so fail closed rather than record an attempt that + # cannot say which model answered. `--model` is an acceptable source; a + # silently absent model is not. + resolved_model = model_from(envelope) or args.model + if not resolved_model: + return { + "protocol_version": protocol.PROTOCOL_VERSION, + "outcome": "runtime_failure", + "simulation": False, + "executor": executor, + "failure": { + "reason": ( + "the runtime reported no model identity; refusing to record " + "an attempt that cannot name the model that answered" + ) + }, + } + executor["model"] = resolved_model response: dict[str, Any] = { "protocol_version": protocol.PROTOCOL_VERSION, "outcome": "blocked" if result.get("verdict") == "blocked" else "review_result", diff --git a/review-suite/scripts/evals/corpus.py b/review-suite/scripts/evals/corpus.py index ec0d56a..9e9bb20 100644 --- a/review-suite/scripts/evals/corpus.py +++ b/review-suite/scripts/evals/corpus.py @@ -91,6 +91,7 @@ class Corpus: corpus_version: str grader_version: str target_skill: str + target_skill_dependencies: tuple[str, ...] cases: tuple[Case, ...] @@ -248,11 +249,25 @@ def load_corpus(root: Path | None = None) -> Corpus: if orphans: raise CorpusError("corpus.json: " + "; ".join(orphans)) + # The declared closure must resolve before anything launches: shipping a + # target whose required siblings are absent would grade a compliant reviewer + # wrong for correctly refusing to review without them. + dependencies = tuple(index["target_skill_dependencies"]) + if len(set(dependencies)) != len(dependencies): + raise CorpusError("corpus.json: duplicate target_skill_dependencies") + if index["target_skill"] in dependencies: + raise CorpusError("corpus.json: target_skill lists itself as a dependency") + for skill in (index["target_skill"], *dependencies): + path = protocol.REPOSITORY_ROOT / "skills" / skill / "SKILL.md" + if not path.is_file(): + raise CorpusError(f"corpus.json: missing declared skill {path}") + return Corpus( root=root, corpus_version=index["corpus_version"], grader_version=index["grader_version"], target_skill=index["target_skill"], + target_skill_dependencies=dependencies, cases=tuple(load_case(root, case_id) for case_id in declared), ) diff --git a/review-suite/scripts/evals/runner.py b/review-suite/scripts/evals/runner.py index 52bbb0b..b69dbdc 100644 --- a/review-suite/scripts/evals/runner.py +++ b/review-suite/scripts/evals/runner.py @@ -24,6 +24,7 @@ import subprocess import sys import time +from collections.abc import Mapping, Sequence from datetime import datetime, timezone from pathlib import Path from typing import Any @@ -48,45 +49,86 @@ def _now() -> str: return datetime.now(timezone.utc).isoformat(timespec="seconds") -def target_skill_documents(target_skill: str) -> dict[str, str]: - """Return the target skill's complete reviewer-visible text. - - A skill's `SKILL.md` is not the whole skill: `review-code-change` instructs - the reviewer to read `references/orchestration-protocol.md`, which owns the - lens decision table, deduplication, and verdict aggregation. An executor is - told to reason only from what it is given, so omitting a mandated reference - would measure a reviewer working from a partial definition of its own job. +def _skill_documents(skill: str) -> dict[str, str]: + """One skill's reviewer-visible Markdown, namespaced by skill name. The bundled `references/review-suite/` mirror is excluded: `contract_documents` already supplies those files from their canonical location. """ - root = TARGET_SKILL_ROOT / target_skill + root = TARGET_SKILL_ROOT / skill skill_md = root / "SKILL.md" if not skill_md.is_file(): raise ConfigurationError(f"missing target skill prompt {skill_md}") - documents = {"SKILL.md": skill_md.read_text()} + documents = {f"{skill}/SKILL.md": skill_md.read_text()} references = root / "references" if references.is_dir(): for path in sorted(references.rglob("*.md")): relative = path.relative_to(root) if "review-suite" in relative.parts: continue - documents[relative.as_posix()] = path.read_text() + documents[f"{skill}/{relative.as_posix()}"] = path.read_text() + return documents + + +def target_skill_documents( + target_skill: str, dependencies: Sequence[str] = () +) -> dict[str, str]: + """Return the target skill's declared dependency closure as text. + + A skill's `SKILL.md` is not the whole skill, in two distinct ways. + + It links references that it instructs the reviewer to read - for + `review-code-change`, `references/orchestration-protocol.md`, which owns the + lens decision table, deduplication, and verdict aggregation. + + It also declares sibling skills it cannot work without: `review-code-change` + requires `review-solution-simplicity`, `review-correctness`, and + `review-code-simplicity` to be available and readable, and mandates an + aggregate `blocked` result naming any that are missing. An executor is told + to reason only from what it is given, so a target whose contract requires + siblings can only be evaluated when those siblings are in the payload. + Omitting them inverts the measurement: a reviewer that correctly refuses for + missing dependencies is scored wrong on every case that expects a merge + verdict, so recall moves the wrong way as the reviewer becomes more + compliant. + + The corpus declares the closure so a target can be swapped without changing + this code. Which target a scored corpus should measure, and the cost envelope + that follows from its closure, are corpus-composition decisions. + """ + documents = _skill_documents(target_skill) + for dependency in dependencies: + if dependency == target_skill: + raise ConfigurationError(f"{target_skill} declares itself as a dependency") + documents.update(_skill_documents(dependency)) return documents -def target_skill_prompt(target_skill: str) -> str: - """Render the skill and every mandated reference as one prompt. +def render_skill_prompt(target_skill: str, documents: Mapping[str, str]) -> str: + """Lay out a skill closure as one prompt, target first and clearly named. + + Every section is labelled, including the target's own `SKILL.md`: once a + payload carries several skills, a reviewer has to be able to tell which one + it is being asked to execute and which are its dependencies. + """ + remaining = dict(documents) + lead = f"{target_skill}/SKILL.md" + sections = [f"## Target skill: {lead}\n\n{remaining.pop(lead)}"] + for name, text in sorted(remaining.items()): + sections.append(f"\n\n## Skill reference: {name}\n\n{text}") + return "".join(sections) + + +def target_skill_prompt(target_skill: str, dependencies: Sequence[str] = ()) -> str: + """Render the target skill and its whole declared closure as one prompt. `prompt_digest` hashes this string, so the recorded `target_skill_digest` changes whenever any part of the evaluated skill text changes - which is the whole point of pinning it across baseline strata. """ - documents = dict(target_skill_documents(target_skill)) - sections = [documents.pop("SKILL.md")] - for name, text in sorted(documents.items()): - sections.append(f"\n\n## Skill reference: {name}\n\n{text}") - return "".join(sections) + return render_skill_prompt( + target_skill, target_skill_documents(target_skill, dependencies) + ) def contract_documents() -> dict[str, str]: @@ -223,7 +265,9 @@ def evaluate( f"corpus grader_version {loaded.grader_version!r} does not match the " f"shipped grader {grader.GRADER_VERSION!r}" ) - skill_prompt = target_skill_prompt(loaded.target_skill) + skill_prompt = target_skill_prompt( + loaded.target_skill, loaded.target_skill_dependencies + ) documents = contract_documents() commit = suite_commit() forced_simulation = is_bundled_fixture_executor(command) diff --git a/review-suite/scripts/tests/test_eval_corpus.py b/review-suite/scripts/tests/test_eval_corpus.py index b18d90e..e8f5b87 100644 --- a/review-suite/scripts/tests/test_eval_corpus.py +++ b/review-suite/scripts/tests/test_eval_corpus.py @@ -245,6 +245,36 @@ def test_packet_validity_disagreement_fails(self): self._write_expectation(case_id, expectation) self.assertAuditFails("packet:") + def test_a_missing_declared_dependency_fails(self): + index = self._index() + index["target_skill_dependencies"].append("review-nothing-at-all") + self._write_index(index) + self.assertAuditFails("missing declared skill") + + def test_a_missing_target_skill_fails(self): + index = self._index() + index["target_skill"] = "review-nothing-at-all" + self._write_index(index) + self.assertAuditFails("missing declared skill") + + def test_a_target_listing_itself_as_a_dependency_fails(self): + index = self._index() + index["target_skill_dependencies"].append(index["target_skill"]) + self._write_index(index) + self.assertAuditFails("lists itself as a dependency") + + def test_duplicate_declared_dependencies_fail(self): + index = self._index() + index["target_skill_dependencies"].append(index["target_skill_dependencies"][0]) + self._write_index(index) + self.assertAuditFails("duplicate target_skill_dependencies") + + def test_an_absent_dependency_declaration_fails(self): + index = self._index() + del index["target_skill_dependencies"] + self._write_index(index) + self.assertAuditFails("target_skill_dependencies") + def test_grader_version_drift_fails(self): index = self._index() index["grader_version"] = "0.0" diff --git a/review-suite/scripts/tests/test_eval_protocol.py b/review-suite/scripts/tests/test_eval_protocol.py index fb58dd6..2e46371 100644 --- a/review-suite/scripts/tests/test_eval_protocol.py +++ b/review-suite/scripts/tests/test_eval_protocol.py @@ -62,6 +62,54 @@ def test_request_records_suite_candidate_and_run_identity(self): request["target_skill_digest"], ) + def test_the_payload_carries_every_skill_the_target_declares_required(self): + """A target that must verify siblings cannot be evaluated without them. + + `review-code-change` mandates an aggregate `blocked` result naming any + missing lens skill, so omitting one would score a compliant reviewer + wrong on every case expecting a merge verdict. + """ + loaded = corpus.load_corpus() + declared = set(loaded.target_skill_dependencies) + skills_root = protocol.REPOSITORY_ROOT / "skills" + installed = {path.name for path in skills_root.iterdir() if path.is_dir()} + target_text = (skills_root / loaded.target_skill / "SKILL.md").read_text() + required = { + name + for name in installed + if name != loaded.target_skill and name in target_text + } + self.assertTrue(required, "the target names no sibling skill at all") + self.assertEqual( + set(), + required - declared, + "the target's SKILL.md names skills absent from the declared closure", + ) + prompt = runner.target_skill_prompt( + loaded.target_skill, loaded.target_skill_dependencies + ) + for name in declared: + with self.subTest(dependency=name): + self.assertIn(f"{name}/SKILL.md", prompt) + self.assertIn( + (skills_root / name / "SKILL.md").read_text().strip(), prompt + ) + + def test_a_self_sufficient_target_needs_no_closure(self): + prompt = runner.target_skill_prompt("review-correctness") + self.assertIn("review-correctness/SKILL.md", prompt) + self.assertNotIn("review-code-change/SKILL.md", prompt) + + def test_a_target_cannot_declare_itself_as_a_dependency(self): + with self.assertRaises(runner.ConfigurationError): + runner.target_skill_documents("review-code-change", ["review-code-change"]) + + def test_a_missing_declared_dependency_fails_closed(self): + with self.assertRaises(runner.ConfigurationError): + runner.target_skill_documents( + "review-code-change", ["review-nothing-at-all"] + ) + def test_the_payload_carries_every_mandated_skill_reference(self): """A skill is not just its SKILL.md. @@ -71,14 +119,19 @@ def test_the_payload_carries_every_mandated_skill_reference(self): reviewer working from a partial definition of its own job. """ documents = runner.target_skill_documents("review-code-change") - self.assertIn("SKILL.md", documents) - self.assertIn("references/orchestration-protocol.md", documents) + self.assertIn("review-code-change/SKILL.md", documents) + self.assertIn( + "review-code-change/references/orchestration-protocol.md", documents + ) prompt = runner.target_skill_prompt("review-code-change") for name, text in documents.items(): with self.subTest(document=name): self.assertIn(text.strip(), prompt) request = build_request(self.case, skill_prompt=prompt) - self.assertIn("references/orchestration-protocol.md", request["skill_prompt"]) + self.assertIn( + "review-code-change/references/orchestration-protocol.md", + request["skill_prompt"], + ) def test_the_bundled_contract_mirror_is_not_sent_twice(self): documents = runner.target_skill_documents("review-code-change") @@ -95,10 +148,12 @@ def test_the_skill_digest_tracks_every_document_it_pins(self): with self.subTest(document=name): edited = dict(documents) edited[name] += "\n\nAn additional sentence.\n" - sections = [edited.pop("SKILL.md")] - for other, text in sorted(edited.items()): - sections.append(f"\n\n## Skill reference: {other}\n\n{text}") - self.assertNotEqual(baseline, protocol.prompt_digest("".join(sections))) + self.assertNotEqual( + baseline, + protocol.prompt_digest( + runner.render_skill_prompt("review-code-change", edited) + ), + ) def test_case_reference_is_opaque(self): request = build_request(self.case) diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index a9e80b6..aafd58d 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -385,7 +385,9 @@ def test_a_missing_target_skill_is_rejected(self): (root / "corpus.json").write_text(json.dumps(index, indent=2)) completed = run_runner("--executor", fixture_command(), "--corpus", str(root)) self.assertEqual(2, completed.returncode) - self.assertIn("missing target skill prompt", completed.stderr) + # Caught while loading the corpus, before any prompt is assembled. + self.assertIn("missing declared skill", completed.stderr) + self.assertIn("review-nothing-at-all", completed.stderr) def test_a_simulated_run_cannot_write_a_baseline_report(self): baseline = self.temp / "baseline.json" @@ -521,6 +523,97 @@ def test_prose_instead_of_json_becomes_a_runtime_failure(self): status, _, _ = protocol.classify_response(self.case.packet, stdout) self.assertEqual("runtime_failure", status) + def _envelope_stub(self, envelope_body: str) -> Path: + """A stub `claude` that echoes the candidate and a chosen envelope.""" + return self._stub( + "#!/usr/bin/env python3\n" + "import json, sys\n" + "prompt = sys.stdin.read()\n" + "candidate = json.loads(prompt.strip().splitlines()[-1])\n" + "result = {\n" + ' "schema_version": "1.0", "lens": "aggregate",\n' + ' "candidate": candidate, "verdict": "clean",\n' + ' "findings": [], "blocking_reasons": [],\n' + "}\n" + f"envelope = {envelope_body}\n" + 'envelope["result"] = json.dumps(result)\n' + "json.dump(envelope, sys.stdout)\n" + ) + + def test_cached_prompt_tokens_are_counted_as_input(self): + """Headless output reports most prompt tokens as cache, not input. + + A real 16456-token prompt reported `input_tokens: 2` with the rest under + cache creation and cache read, so totalling only `input_tokens` published + an input count wrong by three orders of magnitude - the number a frozen + cost envelope would rest on. + """ + stub = self._envelope_stub( + '{"usage": {"input_tokens": 2, "cache_creation_input_tokens": 5657,' + ' "cache_read_input_tokens": 10797, "output_tokens": 4},' + ' "total_cost_usd": 0.0409,' + ' "modelUsage": {"stub-model-1": {"inputTokens": 2}}}' + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + status, response, _ = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("review_result", status) + self.assertEqual(16456, response["usage"]["input_tokens"]) + self.assertEqual(4, response["usage"]["output_tokens"]) + self.assertEqual(0.0409, response["usage"]["cost_usd"]) + + def test_the_model_is_read_from_the_model_usage_mapping(self): + """`modelUsage` is a mapping keyed by model id, never a string.""" + stub = self._envelope_stub( + '{"usage": {"input_tokens": 1, "output_tokens": 1},' + ' "total_cost_usd": 0.01,' + ' "modelUsage": {"claude-stub-4-6[1m]": {"inputTokens": 1}}}' + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + _, response, _ = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("claude-stub-4-6[1m]", response["executor"]["model"]) + + def test_a_plain_model_string_still_wins(self): + stub = self._envelope_stub( + '{"model": "explicit-model", "usage": {"output_tokens": 1},' + ' "total_cost_usd": 0.01,' + ' "modelUsage": {"ignored-model": {"inputTokens": 1}}}' + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + _, response, _ = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("explicit-model", response["executor"]["model"]) + + def test_an_attempt_without_model_identity_is_a_runtime_failure(self): + """Required identity: refuse rather than record a nameless model.""" + stub = self._envelope_stub( + '{"usage": {"output_tokens": 1}, "total_cost_usd": 0.01}' + ) + completed = subprocess.run( + [sys.executable, str(CLAUDE_EXECUTOR), "--claude-bin", str(stub)], + input=json.dumps(self.request), + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(0, completed.returncode, completed.stderr) + status, _, detail = protocol.classify_response( + self.case.packet, completed.stdout + ) + self.assertEqual("runtime_failure", status) + self.assertIn("no model identity", detail) + + def test_an_explicit_model_flag_satisfies_identity(self): + stub = self._envelope_stub( + '{"usage": {"output_tokens": 1}, "total_cost_usd": 0.01}' + ) + code, stdout, stderr = self._invoke(stub) + self.assertEqual(0, code, stderr) + status, response, _ = protocol.classify_response(self.case.packet, stdout) + self.assertEqual("review_result", status) + self.assertEqual("stub-model", response["executor"]["model"]) + def test_the_adapter_prompt_stays_result_blind(self): from evals import claude_executor From 87ec303d949301c908c3a29cb220bed22d44c775 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 11:18:53 -0700 Subject: [PATCH 7/9] docs: record the measured smoke evaluation and its variance ## 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 --- CHANGELOG.md | 2 + review-suite/evals/README.md | 74 ++++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b80648..85f013a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- docs: record the measured smoke evaluation and its variance - fix: evaluate the target skill's whole declared closure + (`62a9ed8fab166c7d380724e426449f0585714b07`) - docs: pin the recorded smoke evaluation to its run (`f00ce2db80ed3a7bed6afb4962cf0bb5a68390fe`) - fix: complete the evaluated skill text and the audit ordering diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index 89aec76..74bb4ce 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -18,7 +18,7 @@ review-suite/evals/ ├── contracts/ versioned evaluator schemas │ ├── executor-request.schema.json the complete result-blind payload │ ├── executor-response.schema.json the single reply an executor returns -│ ├── corpus.schema.json corpus version metadata and case list +│ ├── corpus.schema.json versions, target closure, case list │ ├── expectation.schema.json private material root causes │ └── provenance.schema.json origin and retention authority ├── corpus/ @@ -94,10 +94,10 @@ reviewer to verify that `review-solution-simplicity`, `review-correctness`, and `blocked` result naming any that are missing. An executor is told to reason only from what it is given. Ship the target alone and a fully compliant reviewer must refuse every case, so recall and false-clean rates move the *wrong* way as the -reviewer becomes *more* compliant — the measurement inverts. An earlier revision -had exactly that defect, and on the same corpus and adapter its reviewer -answered `changes_required` on the incomplete-evidence case where the complete -closure correctly answers `blocked`. +reviewer becomes *more* compliant — the measurement inverts. Earlier revisions +had exactly that defect. Supplying the closure removes the inversion; it does +not by itself make any single case answer correctly, and the recorded runs below +show that case still varying between runs. `target_skill_digest` hashes the rendered closure, so it changes when any part of any included skill changes. Loading fails closed when a declared skill is @@ -247,27 +247,31 @@ representative scored corpus, calibrate the grader, or capture a v1 baseline. ### Measured smoke evaluation One run per case through the bundled Claude adapter, recorded at suite commit -`69748be5bda5a8638b2e6ddef6ea8a13e12589a9` against corpus version -`0.1-protocol-proof` and grader version `1.0`. This is one recorded observation, -not a baseline, and the only later change to this directory is the paragraph you -are reading, which no payload carries: - -- 6 attempts, 6 valid protocol outcomes, 0 evaluation failures; -- every verdict matched its expectation: all 5 graded verdicts matched, and the - incomplete-evidence case correctly returned `blocked`, which is a valid review - and so counts toward stability but is not graded; -- 7 findings reported, all 7 referred for adjudication because none matched a - shipped formulation, giving `material_finding_recall` 0.0 over the 4 attempts - with expected root causes; -- `false_positive_rate` 0.0 over 5, `false_clean_rate` 0.0 over 4; -- 0.76 USD total reported cost, 30.4 s mean latency, 39.7 s maximum. - -Treat the verdicts as the stable part and the counts as the variable part. Two -runs of this configuration produced identical per-case verdicts, while the -finding count moved between 6 and 7 and mean latency between 28 s and 30 s. -Every stability figure above rests on a denominator of 1 per case, so it records -only that one run happened, not run-to-run agreement; use `--runs N` to measure -that. +`62a9ed8fab166c7d380724e426449f0585714b07`, target `review-code-change` with +digest `9b2805f14cdd6158`, model `claude-opus-4-6[1m]`, corpus version +`0.1-protocol-proof`, grader version `1.0`. This is one recorded observation, +not a baseline: + +- 6 attempts, 6 valid protocol outcomes, 0 evaluation failures, all 6 graded; +- 5 of 6 verdicts matched. `catalog-import-feed` returned `changes_required` + where `blocked` was expected — a merge verdict on a packet whose required + full-scope validation evidence is absent; +- 8 findings reported. 7 were referred for adjudication because none matched a + shipped formulation, and 1 was recorded as an unexpected gating finding, + giving `false_positive_rate` 0.1667 over 6; +- `material_finding_recall` 0.0 over the 4 attempts with expected root causes; + `false_clean_rate` 0.0 over 4; +- 1.03 USD total reported cost, 191,422 input tokens, 8,051 output tokens, 33.0 + s mean latency, 59.9 s maximum. + +**Do not read single-run verdicts as capability.** The immediately preceding run +of this same configuration returned `blocked` on `catalog-import-feed` — the +correct answer — while this one returned a merge verdict. That case is the least +stable of the six and it is the one that tests refusal on incomplete evidence, +which is the behaviour the surrounding work most needs to measure. At one run +per case, nothing here distinguishes capability from variance: every stability +figure above rests on a denominator of 1. Use `--runs N` before drawing any +conclusion. ### Limitations for whoever curates the scored corpus @@ -276,16 +280,20 @@ that. not recognize is therefore reported as a partial match needing adjudication, not as a false positive. Calibration must decide how those are scored. - The shipped formulations were written before any real run and were not tuned - afterwards, which is why recall is 0.0 above while every verdict was right. + afterwards, which is why recall is 0.0 above while 5 of 6 verdicts were right. That is the conservative behaviour this interface is meant to have, and direct evidence that grader calibration is required before any recall number means anything. -- Completeness of the evaluated skill text is load-bearing, not incidental. An - earlier revision passed only `SKILL.md` and omitted the orchestration protocol - that `review-code-change` instructs its reviewer to read; on the same corpus - and adapter, that reviewer answered `changes_required` on the - incomplete-evidence case instead of `blocked`. Any change to what a payload - carries is a change to what is being measured, and starts a new stratum. +- Completeness of the evaluated skill closure is load-bearing, not incidental. + Earlier revisions omitted first the orchestration protocol and then the three + lens skills that `review-code-change` requires; each omission changed observed + behaviour on the incomplete-evidence case. Any change to what a payload + carries is a change to what is being measured, and starts a new stratum. The + closure's size also sets the cost floor: it is 8 documents and about 41,600 + characters here, and the run above spent 191,422 input tokens across 6 + attempts. +- The choice of scored target, the strata to compare, and the cost envelope to + preregister all follow from that closure and are deliberately left open. - `expectation.schema.json` requires a `severity` on every root cause that no metric currently consumes. Either score severity agreement or drop the requirement; do not assume it is being measured. From b605051a7385dd310b0eff9dbf14c10dda87c633 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 11:41:38 -0700 Subject: [PATCH 8/9] feat: record the evaluated skill closure with every run ## 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 --- CHANGELOG.md | 2 + review-suite/evals/README.md | 63 ++++++++++++------- review-suite/scripts/evals/runner.py | 12 +++- .../scripts/tests/test_eval_runner.py | 26 ++++++++ 4 files changed, 78 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85f013a..772a833 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- feat: record the evaluated skill closure with every run - docs: record the measured smoke evaluation and its variance + (`87ec303d949301c908c3a29cb220bed22d44c775`) - fix: evaluate the target skill's whole declared closure (`62a9ed8fab166c7d380724e426449f0585714b07`) - docs: pin the recorded smoke evaluation to its run diff --git a/review-suite/evals/README.md b/review-suite/evals/README.md index 74bb4ce..844bfa8 100644 --- a/review-suite/evals/README.md +++ b/review-suite/evals/README.md @@ -217,6 +217,12 @@ across unlike cases would report disagreement between cases as instability within one, and dropping `blocked` attempts would report a reviewer that refuses a verdict on one run and issues one on the next as perfectly stable. +The report's `configuration` block states what was run: executor command, target +skill, its declared dependencies, the exact closure document list, the closure +digest, suite commit, corpus and grader versions, run count, and the timeout and +output limits. Each attempt repeats the target, its dependencies, and the +digest, so a per-attempt record is self-describing without its report. + The report encodes no success threshold and returns no pass/fail judgement. `--artifact-dir` retains raw executor output. It is opt-in, and @@ -244,13 +250,22 @@ This directory proves the protocol, the grading interface, the contamination controls, and the failure taxonomy. It deliberately does not curate a representative scored corpus, calibrate the grader, or capture a v1 baseline. -### Measured smoke evaluation +### Protocol smoke evaluation + +> **This run proves the protocol end to end and nothing else.** It is not +> evidence of reviewer capability, review quality, or cost, and must not be +> cited as any of those here or downstream. Its numbers are here to show that +> the fields exist, are populated, and are internally consistent — that an +> attempt is classified, graded, and recorded with identity, usage, and latency. +> One run per case cannot measure a stochastic reviewer, and no cost envelope +> follows from it. One run per case through the bundled Claude adapter, recorded at suite commit `62a9ed8fab166c7d380724e426449f0585714b07`, target `review-code-change` with -digest `9b2805f14cdd6158`, model `claude-opus-4-6[1m]`, corpus version -`0.1-protocol-proof`, grader version `1.0`. This is one recorded observation, -not a baseline: +declared closure `review-solution-simplicity`, `review-correctness`, +`review-code-simplicity` and digest `9b2805f14cdd6158`, model +`claude-opus-4-6[1m]`, corpus version `0.1-protocol-proof`, grader version +`1.0`: - 6 attempts, 6 valid protocol outcomes, 0 evaluation failures, all 6 graded; - 5 of 6 verdicts matched. `catalog-import-feed` returned `changes_required` @@ -261,17 +276,17 @@ not a baseline: giving `false_positive_rate` 0.1667 over 6; - `material_finding_recall` 0.0 over the 4 attempts with expected root causes; `false_clean_rate` 0.0 over 4; -- 1.03 USD total reported cost, 191,422 input tokens, 8,051 output tokens, 33.0 - s mean latency, 59.9 s maximum. - -**Do not read single-run verdicts as capability.** The immediately preceding run -of this same configuration returned `blocked` on `catalog-import-feed` — the -correct answer — while this one returned a merge verdict. That case is the least -stable of the six and it is the one that tests refusal on incomplete evidence, -which is the behaviour the surrounding work most needs to measure. At one run -per case, nothing here distinguishes capability from variance: every stability -figure above rests on a denominator of 1. Use `--runs N` before drawing any -conclusion. +- usage and latency were populated rather than dropped: 191,422 input tokens, + 8,051 output tokens, 1.03 USD reported, 33.0 s mean and 59.9 s maximum + latency, and a model identity on every attempt. + +The verdicts above are demonstrably not repeatable. The immediately preceding +run of this same configuration returned `blocked` on `catalog-import-feed` — the +correct answer — while this one returned a merge verdict. That case is the one +testing refusal on incomplete evidence, which is the behaviour the surrounding +work most needs to measure, and it flipped between two consecutive runs. Every +stability figure here rests on a denominator of 1. Use `--runs N`, and read the +result as a measurement only once the denominator supports it. ### Limitations for whoever curates the scored corpus @@ -280,20 +295,20 @@ conclusion. not recognize is therefore reported as a partial match needing adjudication, not as a false positive. Calibration must decide how those are scored. - The shipped formulations were written before any real run and were not tuned - afterwards, which is why recall is 0.0 above while 5 of 6 verdicts were right. - That is the conservative behaviour this interface is meant to have, and direct - evidence that grader calibration is required before any recall number means - anything. + afterwards, which is why recall is 0.0 above. That is the conservative + behaviour this interface is meant to have, and it means grader calibration is + required before any recall number means anything. - Completeness of the evaluated skill closure is load-bearing, not incidental. Earlier revisions omitted first the orchestration protocol and then the three lens skills that `review-code-change` requires; each omission changed observed behaviour on the incomplete-evidence case. Any change to what a payload - carries is a change to what is being measured, and starts a new stratum. The - closure's size also sets the cost floor: it is 8 documents and about 41,600 - characters here, and the run above spent 191,422 input tokens across 6 - attempts. + carries is a change to what is being measured, and starts a new stratum. Every + run records its closure's membership and digest so a stratum can state what it + evaluated. - The choice of scored target, the strata to compare, and the cost envelope to - preregister all follow from that closure and are deliberately left open. + preregister all follow from that closure and are deliberately left open. Size + the envelope from a preregistered run of the chosen closure; do not + extrapolate it from the protocol smoke run above. - `expectation.schema.json` requires a `severity` on every root cause that no metric currently consumes. Either score severity agreement or drop the requirement; do not assume it is being measured. diff --git a/review-suite/scripts/evals/runner.py b/review-suite/scripts/evals/runner.py index b69dbdc..d7d0f42 100644 --- a/review-suite/scripts/evals/runner.py +++ b/review-suite/scripts/evals/runner.py @@ -265,9 +265,13 @@ def evaluate( f"corpus grader_version {loaded.grader_version!r} does not match the " f"shipped grader {grader.GRADER_VERSION!r}" ) - skill_prompt = target_skill_prompt( + # Keep the closure's exact membership, not just its digest: a digest proves + # two runs sent the same text but cannot say what that text was, and a + # baseline stratum has to be able to state which skills it evaluated. + closure = target_skill_documents( loaded.target_skill, loaded.target_skill_dependencies ) + skill_prompt = render_skill_prompt(loaded.target_skill, closure) documents = contract_documents() commit = suite_commit() forced_simulation = is_bundled_fixture_executor(command) @@ -323,6 +327,10 @@ def refuse_if_contaminated(case: corpus.Case, request: dict[str, Any]) -> None: max_output_bytes=max_output_bytes, forced_simulation=forced_simulation, ) + attempt["target_skill"] = loaded.target_skill + attempt["target_skill_dependencies"] = list( + loaded.target_skill_dependencies + ) if attempt["status"] in protocol.GRADABLE_STATUSES: attempt["grade"] = grader.grade( case.expectation, (response or {})["result"] @@ -336,6 +344,8 @@ def refuse_if_contaminated(case: corpus.Case, request: dict[str, Any]) -> None: "corpus_version": loaded.corpus_version, "grader_version": loaded.grader_version, "target_skill": loaded.target_skill, + "target_skill_dependencies": list(loaded.target_skill_dependencies), + "target_skill_documents": sorted(closure), "target_skill_digest": protocol.prompt_digest(skill_prompt), "suite_commit": commit, "runs_per_case": runs, diff --git a/review-suite/scripts/tests/test_eval_runner.py b/review-suite/scripts/tests/test_eval_runner.py index aafd58d..7508f44 100644 --- a/review-suite/scripts/tests/test_eval_runner.py +++ b/review-suite/scripts/tests/test_eval_runner.py @@ -94,6 +94,32 @@ def test_each_attempt_records_suite_candidate_and_run_identity(self): self.assertIsNotNone(attempt["started_at"]) self.assertIsNotNone(attempt["finished_at"]) + def test_the_run_records_which_closure_was_sent(self): + """A digest proves two runs matched; it cannot say what they contained. + + A stratum has to be able to state which skills it evaluated, so the + closure's membership is recorded and not just hashed. + """ + attempts, configuration = self.evaluate() + loaded = corpus.load_corpus() + expected = list(loaded.target_skill_dependencies) + self.assertEqual(expected, configuration["target_skill_dependencies"]) + self.assertEqual(loaded.target_skill, configuration["target_skill"]) + documents = configuration["target_skill_documents"] + self.assertEqual(sorted(documents), documents) + self.assertIn(f"{loaded.target_skill}/SKILL.md", documents) + for dependency in expected: + with self.subTest(dependency=dependency): + self.assertIn(f"{dependency}/SKILL.md", documents) + for attempt in attempts: + with self.subTest(case=attempt["case_id"]): + self.assertEqual(loaded.target_skill, attempt["target_skill"]) + self.assertEqual(expected, attempt["target_skill_dependencies"]) + self.assertEqual( + configuration["target_skill_digest"], + attempt["target_skill_digest"], + ) + def test_only_valid_review_results_are_graded(self): attempts, _ = self.evaluate() for attempt in attempts: From f544aa0c19d97dd4f1aabd7dfab3df08b2ee6a6b Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 26 Jul 2026 12:05:16 -0700 Subject: [PATCH 9/9] fix: skip the recipe-execution tests when `just` is absent ## 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 --- CHANGELOG.md | 2 ++ .../scripts/tests/test_eval_commands.py | 21 ++++++++++++------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 772a833..a899a8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes. ## 2026-07-26 — Added the result-blind review replay evaluator +- fix: skip the recipe-execution tests when `just` is absent - feat: record the evaluated skill closure with every run + (`b605051a7385dd310b0eff9dbf14c10dda87c633`) - docs: record the measured smoke evaluation and its variance (`87ec303d949301c908c3a29cb220bed22d44c775`) - fix: evaluate the target skill's whole declared closure diff --git a/review-suite/scripts/tests/test_eval_commands.py b/review-suite/scripts/tests/test_eval_commands.py index e931fad..d160801 100644 --- a/review-suite/scripts/tests/test_eval_commands.py +++ b/review-suite/scripts/tests/test_eval_commands.py @@ -176,17 +176,24 @@ def test_both_readmes_state_that_the_paid_path_is_opt_in(self): class RecipeExecutionTests(unittest.TestCase): - """Run the recipes through `just` when it is installed.""" + """Run the recipes through `just` when it is installed. + + CI does not install `just`, so this class must skip cleanly there. An absent + executable makes `subprocess.run` raise rather than return a status, so the + probe has to catch `OSError`; checking only the return code turns a missing + `just` into an erroring `setUpClass` instead of a skip. + """ @classmethod def setUpClass(cls): - if ( - subprocess.run( + try: + completed = subprocess.run( ["just", "--version"], capture_output=True, check=False - ).returncode - != 0 - ): - raise unittest.SkipTest("just is not installed") + ) + except OSError as error: + raise unittest.SkipTest(f"just is unavailable: {error}") from error + if completed.returncode != 0: + raise unittest.SkipTest("just is present but not runnable") def just(self, *args): return subprocess.run(