bench: add the review benchmark runner (spec 002)

.gitignore

@@ -7,3 +7,4 @@
 /specs/log
 /bench/results/
 /bench/.cache/
+__pycache__/

CHANGELOG.md

@@ -8,6 +8,11 @@ Please choose versions by [Semantic Versioning](http://semver.org/).
 * MINOR version when you add functionality in a backwards-compatible manner, and
 * PATCH version when you make backwards-compatible bug fixes.
 
+## Unreleased
+
+- bench: add `make bench` and `make bench-test` Makefile targets; wire `bench-test` into `make precommit` so bench unit tests gate every later change
+- bench: rewrite `bench/README.md` to document the shipped runner (CLI surface, exit codes, result row schema, fixed invariants, safety invariant) and replace the parent-derived squash diff snippet with the spec's authoritative `base_sha..head_sha` rule
+- bench: add `__pycache__/` to `.gitignore` so bytecode produced by the precommit-gated test suite does not appear as untracked files
 ## v0.35.0
 
 - Add `bench/` — outcome tier of the test pyramid, scoring a review configuration against expected findings

Makefile

@@ -1,7 +1,16 @@
 SHELL := /bin/bash
 
 .PHONY: precommit
-precommit: check-links check-json check-index check-coverage check-acceptance
+precommit: check-links check-json check-index check-coverage check-acceptance bench-test
+
+.PHONY: bench
+bench:
+	@python3 bench/run.py $(BENCH_ARGS)
+
+.PHONY: bench-test
+bench-test:
+	@echo "bench-test: running bench unit tests..."
+	@python3 -m unittest discover -s bench -p 'test_*.py' 2>&1
 
 .PHONY: check-acceptance
 check-acceptance:

bench/README.md

@@ -9,41 +9,45 @@ The missing tier of this repo's test pyramid:
 | E2E | `scenarios/` | does the pipeline walk end to end? |
 | **Outcome** | **`bench/`** | **does the rule set actually catch bugs?** |
 
-A *configuration* is the tuple `(rules + commands state, model, effort level)`. The bench scores a configuration against a curated set of expected findings, so a rule, model, or effort change carries a measured before/after instead of shipping blind.
+A *configuration* is the tuple `(rules + commands content, model, effort level, review mode)`. Mode is not cosmetic — `short` / `full` / `selector` route through materially different code paths in `/coding:pr-review`, so a result row that did not distinguish mode would conflate two different instruments under one key. The bench scores a configuration against a curated set of expected findings, so a rule, model, effort, or mode change carries a measured before/after instead of shipping blind.
 
 Goal: `[[PR Review Bench]]` in the Personal vault.
 
 ## Current state
 
-Only `prs.json` exists — the development fixture. The runner, golden set, and scoring are not built yet.
+The runner drives the real `/coding:pr-review` slash command over the pinned PR manifest (`bench/prs.json`) and writes one machine-readable row per PR. The golden set and scoring semantics belong to a later spec; `--golden` is recognised and rejected with exit code 2 rather than silently ignored.
 
 ## `prs.json`
 
 Five already-merged PRs, deliberately **not** representative. They exist to build the runner against: language spread (Go ×2, TypeScript, Node, Python), size spread (3 → 783 lines), one known-clean PR, one with two documented defects, and both merge strategies.
 
-Every entry records `base_sha` and `head_sha` explicitly rather than a URL, because reconstructing a merged PR's diff is not obvious:
+Every entry records `base_sha` and `head_sha` explicitly because reconstructing a merged PR's diff requires knowing the merge strategy.
 
-```bash
-# merge-commit: two parents — ^1 is the base branch at merge time, ^2 is the PR head
-git diff <merge_sha>^1..<merge_sha>^2
+## Running it
 
-# squash: one parent — the squash commit IS the head
-git diff <merge_sha>^1..<merge_sha>
+```bash
+make bench BENCH_ARGS="--model <model> --effort <effort> --mode <short|full|selector>"
+make bench-test
 ```
 
-**Branch on parent count, never on a fallback.** Deriving head as "second parent, else the merge commit" yields `base == head` on a squash and produces an **empty diff with no error** — which scores as a clean review. This was hit for real while selecting `node-skeleton#2`.
+`--model`, `--effort`, and `--mode` are mandatory: they are recorded as the configuration identity in every result row and have no safe default. Results land in `bench/results/results.jsonl`. `make bench-test` is also wired into `make precommit` so the unit tests gate every later change to the repo.
 
-```python
-parents = commit["parents"]
-if len(parents) == 2:
-    base, head = parents[0], parents[1]      # merge-commit
-else:
-    base, head = parents[0], merge_sha       # squash (or rebase)
-```
+`python3 bench/run.py --print-config-hash` prints the content hash of `rules/` + `commands/` from the current `--coding-repo` and exits immediately.
+
+**Exit codes:** 0 when every PR produced a row (ok or cache hit); 1 when one or more PRs failed; 2 for a usage, manifest, or preflight failure.
+
+## Diff-range rule
 
-The same failure family: `git diff <base_branch>...<pr_head>` on a merged PR also returns empty, because the head is now an ancestor of the base branch.
+The correct range depends on the merge strategy, **not** on a fallback from parent count:
 
-**Requirement for the runner:** fail loudly on an empty diff. Two independent code paths produce a silent no-op review, and both look identical to a genuinely clean PR.
+- **merge-commit** (two or more parents): `<merge_sha>^1..<merge_sha>^2`
+- **squash or rebase** (exactly one parent): the manifest's recorded `base_sha..head_sha`
+
+The manifest's recorded `base_sha..head_sha` is the single authoritative source for single-parent commits. It is derived from the manifest and **never** reconstructed by walking the merge commit's parents.
+
+> **Why the parent-derived form looked right on the fixture.** The only squash entry in `bench/prs.json` (`node-skeleton#2`) has `head_sha` equal to its `merge_sha`, so `git diff <merge_sha>^1..<merge_sha>` produced the correct diff by coincidence. A coincidence on one fixture entry is not a rule. Deriving head as "second parent, else the merge commit" yields `base == head` on any squash whose head is not the merge commit — an empty diff with no error, which scores as a clean review.
+
+The runner aborts loudly on an empty diff (`EMPTY DIFF`) — a resolved range with zero changed files is never recorded as a zero-finding review and produces no row and no cache entry.
 
 ## Verifying an entry without cloning
 
@@ -52,3 +56,44 @@ gh api repos/<owner>/<repo>/compare/<base_sha>...<head_sha> --jq '.files | lengt
 ```
 
 All five entries were verified this way on 2026-08-06: 1 / 17 / 21 / 18 / 8 files.
+
+## Fixed invariants
+
+These are deliberately not configurable:
+
+- **Review timeout:** 45 minutes per PR (`REVIEW_TIMEOUT_SECONDS = 45 * 60`)
+- **Cache:** lives under `bench/.cache/` (gitignored — no benchmark output is ever committed)
+- **Results:** live under `bench/results/` (gitignored)
+- **Isolated config:** `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`; the runner aborts the whole run before the first review if that directory would resolve the `coding` plugin to content whose hash differs from `--coding-repo`'s
+
+## Safety invariant
+
+Every `git` invocation the runner issues targets a path under `bench/.cache/repos/`. The runner never touches a clone the operator uses for real work. `/coding:pr-review` itself holds `git worktree`, `git fetch`, `git branch`, and `rm -rf` permissions once invoked, so reusing a real clone could destructively mutate it.
+
+## Result row
+
+Each row in `bench/results/results.jsonl` records:
+
+| Field | Description |
+|---|---|
+| `config_hash` | SHA-256 of the full configuration identity |
+| `rules_commands_hash` | SHA-256 of all files in `rules/` and `commands/` |
+| `model` | Model name passed to `/coding:pr-review` |
+| `effort` | Effort level passed to `/coding:pr-review` |
+| `mode` | Mode (`short`, `full`, or `selector`) |
+| `prs_version` | Manifest version string |
+| `pr_id` | PR identifier (e.g. `owner/repo#123`) |
+| `base_sha` | Resolved base SHA |
+| `head_sha` | Resolved head SHA |
+| `diff_range` | The diff range string used (e.g. `abc123^1..abc123^2`) |
+| `changed_files` | Number of files in the diff |
+| `parent_count` | Number of parents on the merge commit (1 = squash/rebase) |
+| `notes` | Strategy-label mismatch warnings, if any |
+| `review_command` | The full `claude … /coding:pr-review …` argv |
+| `started_at` | ISO-8601 timestamp when the review started |
+| `duration_seconds` | Wall-clock seconds for this review |
+| `findings` | Normalised list of `{path, line, rule_id, body}` |
+| `raw_output_ref` | Path to the raw stdout file |
+| `runner_version` | Runner version string |
+
+Repeating a configuration is free: completed `(PR, configuration)` pairs are served from cache and invoke no review. The cache key includes the mode, so changing only `--mode` re-runs the review.