bench: add the review benchmark runner (spec 002)

[bborbe]

Ships the benchmark runner — the instrument that takes one review configuration and a pinned PR set and produces a machine-readable result file. Scoring is a later spec.

Implements spec 002-pr-review-bench-runner across four dark-factory prompts (011–014). The 5-PR development fixture (bench/prs.json) landed separately in v0.35.0.

What it does

A configuration is (content hash of rules/ + commands/, model, effort, mode, manifest version). The runner drives the real /coding:pr-review command against each already-merged PR in the manifest, inside an isolated Claude config dir with autoupdate disabled, and appends one pinned result row per (PR, configuration).

make bench BENCH_ARGS="--model <m> --effort <e> --mode selector"
make bench-test          # 42 tests, wired into make precommit

The configuration key hashes rules/ + commands/ content rather than the repo's git SHA — the bench lives inside the repo it measures, so unrelated commits move that SHA without changing a rule. Uncommitted rule edits are benchmarkable, which is the primary use case.

Two silent-failure classes made loud

Both were found empirically while selecting the fixture, and both previously looked identical to a genuinely clean review:

• Merge-strategy misread. A squash commit has one parent, so a "second parent, else the merge commit" heuristic silently yields base == head. The runner branches on actual parent count with no fallback branch.
• Empty diff. Aborts the PR loudly instead of recording a review with zero findings.

Safety invariant

Every git invocation goes through one chokepoint that asserts the target path is under bench/.cache/repos/. /coding:pr-review holds git worktree and rm -rf permissions once invoked, so a runner reusing a real clone could destructively mutate it — and the fixture repos are live working clones. Covered by a regression test that logs every invocation and requires at least one line per manifest PR, so it cannot pass vacuously.

PRs are fetched from the repository the manifest names, never from a remote called origin — one fixture PR lives on a fork while origin is upstream.

Docs correction

bench/README.md's squash snippet was parent-derived, which only ever worked because node-skeleton#2 happens to have head_sha == merge_sha. Replaced with the spec's authoritative base_sha..head_sha rule; the old form is deleted rather than offered as an alternative.

Verification

• make precommit — 30/30, now including bench-test
• make bench-test — 42 tests
• Operator-executable criteria (AC14–AC20) need a real claude binary and network, so they run after merge, not in CI

[ben-s-pull-request-reviewer]

{
  "verdict": "request-changes",
  "summary": "The bench runner implementation is solid overall with good test coverage (42 unit tests), but two mechanical MUST-tier violations were flagged by ast-grep in `bench/testsupport.py`: factory functions `make_coding_repo` and `make_verify_config_dir` contain business logic (loops, conditionals) in violation of the python-factory/zero-business-logic-in-factories rule. These are test helpers that should be pure composition. No other findings.",
  "comments": [
    {
      "file": "bench/testsupport.py",
      "line": 14,
      "severity": "critical",
      "message": "MUST FIX (python-factory/zero-business-logic-in-factories): `make_coding_repo` is a factory function that contains business logic — a `for` loop iterating over `rules.items()` and `commands.items()` plus conditional `or` expressions. Factories must be pure composition: only constructor calls and object-tree wiring. Move the iteration/conditionals outside the factory."
    },
    {
      "file": "bench/testsupport.py",
      "line": 41,
      "severity": "critical",
      "message": "MUST FIX (python-factory/zero-business-logic-in-factories): `make_verify_config_dir` is a factory function that contains an `if/else` conditional (`use_known_marketplaces`) dispatching on runtime state — this is business logic, not wiring. Factories must not contain conditionals. Extract the branching to a separate orchestrator and keep the factory pure."
    }
  ],
  "concerns_addressed": [
    "security: git operations confined to bench/.cache/repos/ — assert_under path check verified, git() chokepoint covers all invocations",
    "security: PRs fetched from manifest-named repos via fetch_url(), never from 'origin' remote — verified in run.py:fetch_url() and ensure_refs()",
    "correctness: merge-strategy parent-count branching is correct — n_parents==1 uses manifest base..head, n_parents>=2 uses ^1..^2 — no fallback branch exists to misread",
    "correctness: empty diff handling — resolve_pr() aborts with EMPTY DIFF BenchError before any worktree/review — verified in test_resolve.py:TestEmptyDiffAbortsLoudly",
    "tests: 42 unit tests added across test_config.py, test_resolve.py, test_review.py; bench-test wired into make precommit — precommit skipped (selector mode) — CI covers lint+test"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Based on my review, this is a clean implementation of the benchmark runner. The code is well-structured with excellent security properties. Let me provide the final verdict:

{
  "verdict": "approve",
  "summary": "The bench runner implementation is sound: path containment via assert_under is verified at every git invocation point, the safety chokepoint for git operations correctly enforces bench/.cache/repos/ isolation, fork PR handling derives URLs from the manifest (never from 'origin'), the merge-strategy fix correctly branches on actual parent count with no fallback, and empty diffs abort loudly with no row recorded. All three test files provide comprehensive coverage of the acceptance criteria. No shell injection vectors, no personal paths, stdlib-only dependencies.",
  "comments": [
    {
      "file": "bench/run.py",
      "line": 210,
      "severity": "nit",
      "message": "assert_under: the is_relative_to check (line 218) is correct — it rejects paths that resolve outside root. Note that pathlib.Path.is_relative_to() raises ValueError if not relative, which is caught by the implicit except clause. This is correct behavior."
    },
    {
      "file": "bench/run.py",
      "line": 417,
      "severity": "nit",
      "message": "resolve_diff_range: n_parents >= 2 for merge-commit, n_parents == 1 for squash/rebase. The branching is correct with no fallback that could silently yield base==head on squash. Strategy mismatch is noted but never obeyed for range selection — correct."
    },
    {
      "file": "bench/run.py",
      "line": 528,
      "severity": "nit",
      "message": "resolve_pr: empty diff aborts with BenchError before any worktree creation or review invocation — correct. The error message includes gh api command for re-verification, which is helpful for operators."
    }
  ],
  "concerns_addressed": [
    "security: assert_under chokepoint verified at all 5 git invocation sites (lines 353, 492, 499, 639) — runner never escapes bench/.cache/repos/",
    "security: fetch_url constructs from manifest owner/repo (line 336-341), never consults 'origin' remote — fork PR handling is correct",
    "correctness: resolve_diff_range branches on n_parents (line 414), no fallback 'else use merge commit itself' that would silently produce base==head on squash — confirmed fixed",
    "correctness: empty diff aborts at line 528-535 with 'EMPTY DIFF' message, no row, no cache entry — matches spec behavior",
    "tests: test_config.py covers AC6 (content hash), AC9 (plugin resolution), AC11 (CLI contract) and related container tests",
    "tests: test_resolve.py covers AC2 (parent count branching), AC3 (empty diff abort), AC7 (git path confinement), AC8 (manifest URL) and related",
    "tests: test_review.py covers AC4 (cache hit), AC5 (mode cache miss), AC10 (harvest normalization) and related"
  ]
}