From f4cb5a296d3dd00deca5573de12b6bfea44c1ab8 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Thu, 6 Aug 2026 23:45:55 +0200 Subject: [PATCH 1/5] 011-pr-review-bench-runner-core --- CHANGELOG.md | 6 + bench/run.py | 438 ++++++++++++++++++ bench/test_config.py | 353 ++++++++++++++ bench/testsupport.py | 104 +++++ .../011-pr-review-bench-runner-core.md | 336 ++++++++++++++ ...12-pr-review-bench-runner-pr-resolution.md | 338 ++++++++++++++ .../in-progress/002-pr-review-bench-runner.md | 165 +++++++ 7 files changed, 1740 insertions(+) create mode 100755 bench/run.py create mode 100644 bench/test_config.py create mode 100755 bench/testsupport.py create mode 100644 prompts/completed/011-pr-review-bench-runner-core.md create mode 100644 prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md create mode 100644 specs/in-progress/002-pr-review-bench-runner.md diff --git a/CHANGELOG.md b/CHANGELOG.md index c9b392a..bbe2507 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ 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 `bench/run.py` — benchmark runner entrypoint with configuration-identity core (content hashing of `rules/`+`commands/`, manifest loading/validation, plugin-resolution preflight, CLI surface with mandatory `--model`/`--effort`/`--mode`, reserved `--golden` rejection, `--print-config-hash` helper) +- bench: add `bench/testsupport.py` — shared test helpers (`make_coding_repo`, `make_verify_config_dir`, `stub_claude`, `with_path`) +- bench: add `bench/test_config.py` — 17 unit tests covering AC6, AC9, AC11 and related acceptance criteria + ## v0.35.0 - Add `bench/` — outcome tier of the test pyramid, scoring a review configuration against expected findings diff --git a/bench/run.py b/bench/run.py new file mode 100755 index 0000000..4bfaec3 --- /dev/null +++ b/bench/run.py @@ -0,0 +1,438 @@ +#!/usr/bin/env python3 +# +# run.py — benchmark runner for /coding:pr-review +# +# Executes the real /coding:pr-review slash command against every PR entry in a +# frozen PR manifest, in an isolated Claude config directory, and records a +# machine-readable result row per PR. +# +# Exit semantics: +# 0 — every PR produced a row (ok or cache hit) +# 1 — one or more PRs failed +# 2 — usage error, manifest problem, or preflight failure (e.g. plugin mismatch) +# +# Paths are resolved relative to the script's own location: +# bench/run.py → BENCH_DIR = bench/ → REPO_ROOT = repo root +# +# Python 3 standard library only — no third-party dependencies. + +import argparse +import hashlib +import json +import os +import pathlib +import re +import sys + +# ---------------------------------------------------------------------- +# Module constants +# ---------------------------------------------------------------------- +RUNNER_VERSION = "1" +REVIEW_TIMEOUT_SECONDS = 45 * 60 +BENCH_DIR = pathlib.Path(__file__).resolve().parent +REPO_ROOT = BENCH_DIR.parent +VERIFY_CONFIG_DIR_NAME = ".claude-verify" +HASHED_SUBDIRS = ("rules", "commands") +VALID_MODES = ("short", "full", "selector") +NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") +PR_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*#[0-9]+$") +REQUIRED_ENTRY_FIELDS = ( + "id", "owner", "repo", "number", + "merge_strategy", "merge_sha", "base_sha", "head_sha", "changed_files", +) + +# ---------------------------------------------------------------------- +# Exceptions +# ---------------------------------------------------------------------- +class BenchError(Exception): + """Every abort that maps to exit code 2 (manifest, preflight, lock).""" + pass + + +# ---------------------------------------------------------------------- +# Content hashing — content-derived, not git-derived +# ---------------------------------------------------------------------- +def content_hash(root: pathlib.Path) -> str: + """Return SHA-256 hex digest of all regular files under root/rules and root/commands. + + Skips any path with a .git component. Files are ordered by POSIX-relative + path so the digest is independent of filesystem iteration order. Each file + contributes: relative_path_bytes \\0 file_length \\0 raw_bytes — length- + framing prevents two different layouts from producing the same byte stream. + Raises BenchError if neither rules nor commands subdirectory exists. + """ + dirs_to_scan = [root / subdir for subdir in HASHED_SUBDIRS] + if not any(d.is_dir() for d in dirs_to_scan): + raise BenchError( + f"{root} does not look like a coding-plugin checkout: " + f"neither rules/ nor commands/ found" + ) + + collected: list[pathlib.Path] = [] + for subdir in dirs_to_scan: + if not subdir.is_dir(): + continue + for p in subdir.rglob("*"): + if p.is_file() and ".git" not in p.parts: + collected.append(p) + + collected.sort(key=lambda p: p.relative_to(root).as_posix()) + + h = hashlib.sha256() + for p in collected: + rel = p.relative_to(root).as_posix() + size = p.stat().st_size + h.update(rel.encode("utf-8")) + h.update(b"\0") + h.update(str(size).encode("utf-8")) + h.update(b"\0") + h.update(p.read_bytes()) + + return h.hexdigest() + + +def config_hash(rules_commands_hash: str, model: str, effort: str, + mode: str, prs_version: str) -> str: + """SHA-256 over the five configuration-identity components. + + Mode is a first-class component: changing only mode must produce a different digest. + """ + payload = "\0".join([rules_commands_hash, model, effort, mode, prs_version]) + return hashlib.sha256(payload.encode("utf-8")).hexdigest() + + +# ---------------------------------------------------------------------- +# Manifest loading and validation +# ---------------------------------------------------------------------- +def load_manifest(path: pathlib.Path) -> dict: + """Load and validate a PR manifest JSON file. + + Requires top-level "version" (non-empty str) and "prs" (non-empty list). + Validates every entry's required fields, character-set restrictions on + owner/repo/id (path-traversal guard), and SHA format on *_sha fields. + """ + try: + data = json.loads(path.read_text(encoding="utf-8")) + except OSError as err: + raise BenchError(f"cannot read manifest {path}: {err}") + except json.JSONDecodeError as err: + raise BenchError(f"manifest {path} is not valid JSON: {err}") + + if not data.get("version"): + raise BenchError("manifest missing required field: 'version'") + if not isinstance(data.get("prs"), list) or not data["prs"]: + raise BenchError("manifest missing required field: 'prs' (must be non-empty list)") + + for index, entry in enumerate(data["prs"]): + entry_id = entry.get("id", "") + for field in REQUIRED_ENTRY_FIELDS: + val = entry.get(field) + # changed_files must be int (0 is valid); everything else must be non-empty + if field == "changed_files": + if not isinstance(val, int): + raise BenchError( + f"manifest entry {index} (id={entry_id!r}): " + f"field {field!r} must be an int, got {type(val).__name__}" + ) + else: + if not val and val != 0: + raise BenchError( + f"manifest entry {index} (id={entry_id!r}): " + f"missing or empty required field {field!r}" + ) + + # Path-traversal guard: owner/repo must be simple GitHub names + owner: str = entry.get("owner", "") + repo: str = entry.get("repo", "") + if not NAME_RE.match(owner): + raise BenchError( + f"manifest entry {entry_id!r}: invalid owner {owner!r} " + f"(must match {NAME_RE.pattern!r})" + ) + if not NAME_RE.match(repo): + raise BenchError( + f"manifest entry {entry_id!r}: invalid repo {repo!r} " + f"(must match {NAME_RE.pattern!r})" + ) + if not PR_ID_RE.match(entry_id): + raise BenchError( + f"manifest entry {index}: invalid id {entry_id!r} " + f"(must match {PR_ID_RE.pattern!r})" + ) + number: int = entry.get("number", 0) + if not isinstance(number, int) or number <= 0: + raise BenchError( + f"manifest entry {entry_id!r}: invalid number {number!r} " + f"(must be int > 0)" + ) + + for sha_field in ("merge_sha", "base_sha", "head_sha"): + sha_val: str = entry.get(sha_field, "") + if not re.match(r"^[0-9a-f]{7,40}$", sha_val): + raise BenchError( + f"manifest entry {entry_id!r}: {sha_field} {sha_val!r} " + f"must match ^[0-9a-f]{{7,40}}$" + ) + + return data + + +def safe_pr_key(pr_id: str) -> str: + """Return pr_id with # replaced by _ (safe for use in filenames). + + Assumes pr_id already passed PR_ID_RE validation. + """ + return pr_id.replace("#", "_") + + +# ---------------------------------------------------------------------- +# Plugin resolution preflight +# ---------------------------------------------------------------------- +def verify_config_dir() -> pathlib.Path: + """Return pathlib.Path(HOME) / .claude-verify. + + Raises BenchError if HOME is not set or empty. + """ + home = os.environ.get("HOME", "") + if not home: + raise BenchError( + f"cannot locate isolated Claude config directory ~/{VERIFY_CONFIG_DIR_NAME}: " + f"HOME environment variable is not set or empty" + ) + return pathlib.Path(home) / VERIFY_CONFIG_DIR_NAME + + +def resolve_plugin_path(config_dir: pathlib.Path) -> pathlib.Path: + """Resolve the coding plugin path from the isolated config directory. + + If config_dir/plugins/known_marketplaces.json exists and contains a usable + "coding" entry with a non-empty installLocation, that path is returned. + Otherwise falls back to config_dir/plugins/marketplaces/coding. + """ + known = config_dir / "plugins" / "known_marketplaces.json" + if known.is_file(): + try: + data = json.loads(known.read_text(encoding="utf-8")) + except json.JSONDecodeError as err: + raise BenchError(f"cannot parse {known}: {err}") + coding_entry = data.get("coding", {}) + install_location = coding_entry.get("installLocation", "") + if install_location and isinstance(install_location, str): + return pathlib.Path(install_location) + return config_dir / "plugins" / "marketplaces" / "coding" + + +def check_plugin_resolution(coding_repo: pathlib.Path, config_dir: pathlib.Path, + expected_hash: str) -> pathlib.Path: + """Verify the isolated config dir will load the coding plugin from coding_repo. + + Raises BenchError (PLUGIN RESOLUTION MISMATCH) if the plugin actually + resolved to a path whose rules/+commands/ content differs from + expected_hash. Runs before any review is invoked. + """ + plugin_path = resolve_plugin_path(config_dir) + + if not plugin_path.is_dir(): + actual = "" + else: + try: + actual = content_hash(plugin_path) + except BenchError: + actual = "" + + if actual != expected_hash: + raise BenchError( + f"PLUGIN RESOLUTION MISMATCH: config_dir={config_dir} " + f"plugin_path={plugin_path} actual_hash={actual} " + f"coding_repo={coding_repo} expected_hash={expected_hash} " + f"refusing to record a configuration hash that did not run" + ) + + return plugin_path + + +# ---------------------------------------------------------------------- +# CLI surface +# ---------------------------------------------------------------------- +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + description="Benchmark runner for /coding:pr-review", + allow_abbrev=False, + ) + parser.add_argument( + "--coding-repo", + type=pathlib.Path, + default=REPO_ROOT, + help="Path to the coding plugin repository (default: repo root)", + ) + parser.add_argument( + "--manifest", + type=pathlib.Path, + default=BENCH_DIR / "prs.json", + help="Path to the PR manifest JSON (default: bench/prs.json)", + ) + parser.add_argument( + "--out-dir", + type=pathlib.Path, + default=BENCH_DIR / "results", + help="Directory for result ledger (default: bench/results)", + ) + parser.add_argument( + "--model", + type=str, + default=None, + help="Model to pass to /coding:pr-review (mandatory, part of config identity)", + ) + parser.add_argument( + "--effort", + type=str, + default=None, + help="Effort level (mandatory, part of config identity)", + ) + parser.add_argument( + "--mode", + type=str, + choices=VALID_MODES, + default=None, + help="/coding:pr-review mode (mandatory, part of config identity)", + ) + parser.add_argument( + "--golden", + type=pathlib.Path, + default=None, + help="[RESERVED — not implemented; scoring is future work]", + ) + parser.add_argument( + "--print-config-hash", + action="store_true", + default=False, + help="Print rules+commands content hash and exit", + ) + return parser + + +# ---------------------------------------------------------------------- +# Core runner logic +# ---------------------------------------------------------------------- +def run_bench(*, coding_repo: pathlib.Path, manifest_path: pathlib.Path, + results_dir: pathlib.Path, cache_root: pathlib.Path, + model: str, effort: str, mode: str, + config_dir: pathlib.Path) -> int: + """Keyword-only runner: load manifest, verify plugin, process each PR. + + Returns 0 only when every PR produced 'ok' or 'cache hit'. + """ + manifest = load_manifest(manifest_path) + + rc_hash = content_hash(coding_repo) + + # Abort before any review if the isolated config would load a different plugin + check_plugin_resolution(coding_repo, config_dir, rc_hash) + + cfg_hash = config_hash(rc_hash, model, effort, mode, manifest["version"]) + + print( + f"config {cfg_hash[:16]} rules+commands {rc_hash[:16]} " + f"model={model} effort={effort} mode={mode} prs={manifest['version']}" + ) + + outcomes: list[tuple[str, str]] = [] + for entry in manifest["prs"]: + pr_id = entry["id"] + try: + outcome, detail = process_pr( + entry=entry, + coding_repo=coding_repo, + results_dir=results_dir, + cache_root=cache_root, + model=model, + effort=effort, + mode=mode, + config_dir=config_dir, + cfg_hash=cfg_hash, + rc_hash=rc_hash, + prs_version=manifest["version"], + ) + except BenchError as err: + outcome, detail = "failed", str(err) + outcomes.append((pr_id, f"{outcome}: {detail}")) + + n_ok = sum(1 for _, d in outcomes if d.startswith("ok:")) + n_cached = sum(1 for _, d in outcomes if d.startswith("cache hit:")) + n_failed = sum(1 for _, d in outcomes if d.startswith("failed:")) + + for pr_id, outcome in outcomes: + print(f"{pr_id}: {outcome}") + print(f"summary: {n_ok} ok, {n_cached} cache hit, {n_failed} failed") + + return 0 if n_failed == 0 else 1 + + +def process_pr(*, entry: dict, coding_repo: pathlib.Path, + results_dir: pathlib.Path, cache_root: pathlib.Path, + model: str, effort: str, mode: str, + config_dir: pathlib.Path, cfg_hash: str, + rc_hash: str, prs_version: str) -> tuple[str, str]: + """Process a single PR — stub for prompt 2 of spec 002. + + PR resolution (prompt 2) and review invocation (prompt 3) are not yet + implemented. This stub loudly fails so the gap cannot be mistaken for + success. + """ + return ("failed", "pr resolution not yet implemented (prompt 2 of spec 002)") + + +# ---------------------------------------------------------------------- +# Entrypoint +# ---------------------------------------------------------------------- +def main(argv=None) -> int: + args = build_parser().parse_args(argv) + + try: + if args.golden is not None: + print( + "--golden is reserved but scoring is not implemented in this runner. " + "Precision/recall and golden-set matching are future work in a separate spec; " + "the runner stops at normalized findings. Re-run without --golden.", + file=sys.stderr, + ) + return 2 + + if args.print_config_hash: + print(content_hash(args.coding_repo.resolve())) + return 0 + + missing: list[str] = [] + if args.model is None: + missing.append("--model") + if args.effort is None: + missing.append("--effort") + if args.mode is None: + missing.append("--mode") + if missing: + print( + f"missing required argument(s): {', '.join(missing)}. " + f"These are part of the configuration identity recorded in every result row " + f"and have no safe default.", + file=sys.stderr, + ) + return 2 + + return run_bench( + coding_repo=args.coding_repo.resolve(), + manifest_path=args.manifest, + results_dir=args.out_dir, + cache_root=BENCH_DIR / ".cache", + model=args.model, + effort=args.effort, + mode=args.mode, + config_dir=verify_config_dir(), + ) + + except BenchError as err: + print(str(err), file=sys.stderr) + return 2 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/bench/test_config.py b/bench/test_config.py new file mode 100644 index 0000000..c1d6f89 --- /dev/null +++ b/bench/test_config.py @@ -0,0 +1,353 @@ +#!/usr/bin/env python3 +"""Unit tests for bench/run.py — AC6, AC9, AC11 and related container tests.""" + +import os +import pathlib +import subprocess +import sys +import tempfile +import unittest + +import run +import testsupport + + +class TestContentHash(unittest.TestCase): + """AC6: content hash is content-derived, not commit-derived.""" + + def test_content_hash_ignores_git_history_and_dirty_tree(self): + """Two dirs with byte-identical rules/+commands/ but different git history + produce the same hash. Mutating one byte produces a different hash.""" + with tempfile.TemporaryDirectory() as tmpdir: + repo_a = testsupport.make_coding_repo( + pathlib.Path(tmpdir) / "a", + rules={"go/sample.yml": "id: go/sample\nlevel: MUST\n"}, + commands={"sample.md": "# Sample\n"}, + ) + # Build a second directory that is byte-identical in rules/+commands/ + b_root = pathlib.Path(tmpdir) / "b" + b_root.mkdir() + (b_root / ".git").mkdir() + (b_root / ".git" / "HEAD").write_text("ref: refs/heads/main\n", encoding="utf-8") + (b_root / ".git" / "config").write_text("[core]\n", encoding="utf-8") + (b_root / "junk.txt").write_text("untracked garbage\n", encoding="utf-8") + testsupport.make_coding_repo( + b_root, + rules={"go/sample.yml": "id: go/sample\nlevel: MUST\n"}, + commands={"sample.md": "# Sample\n"}, + ) + + h_a = run.content_hash(repo_a) + h_b = run.content_hash(b_root) + print(f"hash_a={h_a}") + print(f"hash_b={h_b}") + self.assertEqual(h_a, h_b) + + # Mutate one byte — hash must change + rules_file = repo_a / "rules" / "go" / "sample.yml" + original = rules_file.read_bytes() + mutated = original.replace(b"MUST", b"WONT") + rules_file.write_bytes(mutated) + h_mutated = run.content_hash(repo_a) + print(f"hash_mutated={h_mutated}") + self.assertNotEqual(h_a, h_mutated) + + def test_content_hash_is_order_independent(self): + """Files created in reverse order produce the same hash.""" + with tempfile.TemporaryDirectory() as tmpdir: + repo_a = testsupport.make_coding_repo( + pathlib.Path(tmpdir) / "a", + rules={"go/a.yml": "id: go/a\n", "go/b.yml": "id: go/b\n"}, + commands={"x.md": "# x\n", "y.md": "# y\n"}, + ) + repo_b = pathlib.Path(tmpdir) / "b" + # Create in opposite order + (repo_b / "commands").mkdir(parents=True) + (repo_b / "commands" / "y.md").write_text("# y\n", encoding="utf-8") + (repo_b / "commands" / "x.md").write_text("# x\n", encoding="utf-8") + (repo_b / "rules").mkdir(parents=True) + (repo_b / "rules" / "go").mkdir(parents=True) + (repo_b / "rules" / "go" / "b.yml").write_text("id: go/b\n", encoding="utf-8") + (repo_b / "rules" / "go" / "a.yml").write_text("id: go/a\n", encoding="utf-8") + + h_a = run.content_hash(repo_a) + h_b = run.content_hash(repo_b) + self.assertEqual(h_a, h_b) + + +class TestConfigHash(unittest.TestCase): + """Config hash discriminates mode and is stable for identical inputs.""" + + def test_config_hash_distinguishes_mode(self): + """Same rc_hash/model/effort/version but different mode → different digest.""" + rc = "a" * 64 + model, effort, ver = "claude-opus-5", "high", "dev-1" + h_selector = run.config_hash(rc, model, effort, "selector", ver) + h_full = run.config_hash(rc, model, effort, "full", ver) + self.assertNotEqual(h_selector, h_full) + + def test_config_hash_identical_inputs(self): + """Identical inputs produce identical digests.""" + args = ("a" * 64, "claude-opus-5", "high", "short", "dev-1") + self.assertEqual(run.config_hash(*args), run.config_hash(*args)) + + +class TestLoadManifest(unittest.TestCase): + """Manifest loading and validation.""" + + def test_load_manifest_rejects_missing_field(self): + """An entry missing head_sha raises BenchError naming the entry id and field.""" + import json + import pathlib + + with tempfile.TemporaryDirectory() as td: + bad_manifest = pathlib.Path(td) / "bad.json" + bad_manifest.write_text( + json.dumps({ + "version": "dev-1", + "prs": [{ + "id": "owner#1", "owner": "owner", "repo": "repo", + "number": 1, "merge_strategy": "merge-commit", + "merge_sha": "a" * 7, "base_sha": "b" * 7, + # "head_sha" deliberately absent + "changed_files": 1, + }] + }), + encoding="utf-8", + ) + with self.assertRaises(run.BenchError) as ctx: + run.load_manifest(bad_manifest) + msg = str(ctx.exception) + self.assertIn("owner#1", msg) + self.assertIn("head_sha", msg) + + def test_load_manifest_rejects_invalid_json(self): + """A non-JSON manifest raises BenchError naming the path.""" + import pathlib + + with tempfile.TemporaryDirectory() as td: + bad = pathlib.Path(td) / "not-json.json" + bad.write_text("this is not json {", encoding="utf-8") + with self.assertRaises(run.BenchError) as ctx: + run.load_manifest(bad) + self.assertIn(str(bad), str(ctx.exception)) + + def test_load_manifest_rejects_traversal_owner(self): + """owner="../evil" and repo="a/b" each raise BenchError.""" + import json + import pathlib + + cases = [ + {"owner": "../evil", "repo": "repo"}, + {"owner": "owner", "repo": "a/b"}, + ] + for case in cases: + with tempfile.TemporaryDirectory() as td: + m = pathlib.Path(td) / "m.json" + base = { + "version": "dev-1", + "prs": [{ + "id": "owner#1", "owner": "owner", "repo": "repo", + "number": 1, "merge_strategy": "merge-commit", + "merge_sha": "a" * 7, "base_sha": "b" * 7, + "head_sha": "c" * 7, "changed_files": 1, + }] + } + base["prs"][0].update(case) + m.write_text(json.dumps(base), encoding="utf-8") + with self.assertRaises(run.BenchError) as ctx: + run.load_manifest(m) + self.assertIn(case.get("owner") or case.get("repo"), str(ctx.exception)) + + def test_load_manifest_accepts_real_fixture(self): + """load_manifest on the frozen bench/prs.json succeeds and returns dev-1 with 5 entries.""" + m = run.load_manifest(run.BENCH_DIR / "prs.json") + self.assertEqual(m["version"], "dev-1") + self.assertEqual(len(m["prs"]), 5) + + +class TestPluginResolution(unittest.TestCase): + """Plugin-resolution preflight (AC9 and related).""" + + def test_plugin_resolution_mismatch_aborts_before_any_review(self): + """When resolved plugin differs from --coding-repo, BenchError is raised + before any claude invocation (counter file has 0 lines).""" + import pathlib + import os + + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + # Two coding repos with different content + repo_a = testsupport.make_coding_repo( + td / "repo_a", + rules={"go/a.yml": "id: go/a\n"}, + commands={"a.md": "# a\n"}, + ) + repo_b = testsupport.make_coding_repo( + td / "repo_b", + rules={"go/b.yml": "id: go/b\n"}, # different content + commands={"b.md": "# b\n"}, + ) + cfg = testsupport.make_verify_config_dir(td / "cfg", repo_b) + + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "fake review output") + env = testsupport.with_path(bin_dir) + + # HOME must point at our temp dir so verify_config_dir finds .claude-verify + env["HOME"] = str(td) + + with self.assertRaises(run.BenchError) as ctx: + run.run_bench( + coding_repo=repo_a, + manifest_path=run.BENCH_DIR / "prs.json", + results_dir=td / "results", + cache_root=td / "cache", + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + msg = str(ctx.exception) + self.assertTrue(msg.startswith("PLUGIN RESOLUTION MISMATCH"), msg) + self.assertIn("actual_hash=", msg) + self.assertIn("expected_hash=", msg) + # Counter file must not exist or have 0 lines — zero reviews invoked + self.assertFalse(counter.exists() and counter.read_text().strip()) + + def test_plugin_resolution_honors_install_location(self): + """With use_known_marketplaces=True pointing at the same repo, + resolve_plugin_path returns exactly plugin_src.""" + import pathlib + + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + plugin_src = testsupport.make_coding_repo(td / "src") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + resolved = run.resolve_plugin_path(cfg) + self.assertEqual(resolved, plugin_src) + + def test_plugin_resolution_falls_back_to_marketplaces_dir(self): + """With no known_marketplaces.json, resolve_plugin_path returns + /plugins/marketplaces/coding.""" + import pathlib + + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + plugin_src = testsupport.make_coding_repo(td / "src") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=False) + resolved = run.resolve_plugin_path(cfg) + expected = cfg / "plugins" / "marketplaces" / "coding" + self.assertEqual(resolved, expected) + + def test_known_marketplaces_invalid_json_raises(self): + """Malformed known_marketplaces.json raises BenchError naming the file.""" + import pathlib + + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cfg = td / ".claude-verify" + cfg.mkdir(parents=True) + (cfg / "plugins").mkdir(parents=True) + (cfg / "plugins" / "known_marketplaces.json").write_text( + "{ this is not json", encoding="utf-8" + ) + with self.assertRaises(run.BenchError) as ctx: + run.resolve_plugin_path(cfg) + self.assertIn("known_marketplaces.json", str(ctx.exception)) + + def test_verify_config_dir_without_home_raises(self): + """Without HOME set, verify_config_dir raises BenchError naming .claude-verify. + + Call main() in the subprocess so the top-level try/except BenchError + converts the exception to exit code 2. We pass --model/--effort/--mode + so we get past the missing-argument check and hit verify_config_dir(). + """ + script = ( + "import sys; sys.path.insert(0, 'bench'); " + "import run; " + "sys.exit(run.main(['--model','m','--effort','e','--mode','short']))" + ) + result = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, text=True, + env={k: v for k, v in os.environ.items() if k != "HOME"}, + ) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn(".claude-verify", result.stderr) + + +class TestCliContract(unittest.TestCase): + """CLI exit-code contract tests (AC11 and mandatory flag enforcement).""" + + def test_golden_flag_exits_two(self): + """--golden exits 2 and stderr mentions scoring / future work.""" + result = subprocess.run( + [sys.executable, str(run.BENCH_DIR / "run.py"), + "--golden", "bench/golden.json"], + capture_output=True, text=True, + ) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("scoring", result.stderr.lower()) + self.assertIn("future", result.stderr.lower()) + + def test_print_config_hash_matches_content_hash(self): + """--print-config-hash exits 0 and its stdout equals content_hash().""" + import pathlib + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + repo = testsupport.make_coding_repo(td / "repo") + result = subprocess.run( + [sys.executable, str(run.BENCH_DIR / "run.py"), + "--print-config-hash", "--coding-repo", str(repo)], + capture_output=True, text=True, + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip(), run.content_hash(repo)) + + def test_missing_required_flag_exits_two(self): + """Missing --mode exits 2 and names --mode in stderr.""" + result = subprocess.run( + [sys.executable, str(run.BENCH_DIR / "run.py"), + "--model", "m", "--effort", "e"], + capture_output=True, text=True, + ) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertIn("--mode", result.stderr) + + def test_missing_model_and_effort_flags_exit_two(self): + """Each mandatory flag exits 2 individually when missing. + + Override HOME to a temp dir with a matching plugin so the plugin-resolution + preflight does not fire and mask the missing-argument check. + """ + with tempfile.TemporaryDirectory() as tmpdir: + # Set up an isolated config dir whose plugin matches --coding-repo + plugin_src = testsupport.make_coding_repo(pathlib.Path(tmpdir) / "repo") + cfg = testsupport.make_verify_config_dir( + pathlib.Path(tmpdir) / "cfg", plugin_src, use_known_marketplaces=True + ) + for flag in ["--model", "--effort"]: + args = [ + sys.executable, str(run.BENCH_DIR / "run.py"), + "--coding-repo", str(plugin_src), + # --model and --effort NOT set for the flag being tested + "--mode", "short", + ] + result = subprocess.run( + args, + capture_output=True, text=True, + env={**os.environ, "HOME": tmpdir}, + ) + self.assertEqual( + result.returncode, 2, + f"flag={flag} returncode={result.returncode} stderr={result.stderr}", + ) + self.assertIn(flag, result.stderr) + + +if __name__ == "__main__": + unittest.main() diff --git a/bench/testsupport.py b/bench/testsupport.py new file mode 100755 index 0000000..1c720e5 --- /dev/null +++ b/bench/testsupport.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +# +# testsupport.py — shared test helpers for bench unit tests +# +# Python 3 standard library only — no third-party dependencies. + +import os +import pathlib +import shutil +import stat +import subprocess + + +def make_coding_repo(root: pathlib.Path, *, rules=None, commands=None) -> pathlib.Path: + """Create a temporary coding-repo structure under root. + + Creates root/"rules" and root/"commands" directories and writes the given + {relative_path: text} mappings. Defaults create one small file in each + subdirectory so the directory is never empty. Returns root. + """ + root = pathlib.Path(root) + rules_dir = root / "rules" + commands_dir = root / "commands" + + rules = rules or {"go/sample.yml": "id: go/sample\nlevel: MUST\n"} + commands = commands or {"sample.md": "# Sample command\n"} + + rules_dir.mkdir(parents=True, exist_ok=True) + for rel_path, content in rules.items(): + (rules_dir / rel_path).parent.mkdir(parents=True, exist_ok=True) + (rules_dir / rel_path).write_text(content, encoding="utf-8") + + commands_dir.mkdir(parents=True, exist_ok=True) + for rel_path, content in commands.items(): + (commands_dir / rel_path).parent.mkdir(parents=True, exist_ok=True) + (commands_dir / rel_path).write_text(content, encoding="utf-8") + + return root + + +def make_verify_config_dir(root: pathlib.Path, plugin_src: pathlib.Path, + *, use_known_marketplaces: bool = False) -> pathlib.Path: + """Create an isolated .claude-verify directory under root. + + When use_known_marketplaces is False, copies plugin_src to + /plugins/marketplaces/coding. When True, writes known_marketplaces.json + pointing at plugin_src instead. Returns the .claude-verify path. + """ + root = pathlib.Path(root) + cfg = root / ".claude-verify" + cfg.mkdir(parents=True, exist_ok=True) + + if use_known_marketplaces: + (cfg / "plugins").mkdir(parents=True, exist_ok=True) + known = { + "coding": { + "source": {"source": "github", "repo": "bborbe/coding"}, + "installLocation": str(plugin_src), + } + } + import json as _json + (cfg / "plugins" / "known_marketplaces.json").write_text( + _json.dumps(known), encoding="utf-8" + ) + else: + dest = cfg / "plugins" / "marketplaces" / "coding" + shutil.copytree(plugin_src, dest) + + return cfg + + +def make_stub_bin(bin_dir: pathlib.Path, name: str, body: str) -> pathlib.Path: + """Write bin_dir/name as an executable stub script. + + The script starts with #!/bin/sh and contains the provided body. + chmod 0o755. Returns the path. + """ + bin_dir = pathlib.Path(bin_dir) + bin_dir.mkdir(parents=True, exist_ok=True) + script = bin_dir / name + script.write_text(f"#!/bin/sh\n{body}", encoding="utf-8") + script.chmod(0o755) + return script + + +def stub_claude(bin_dir: pathlib.Path, counter_file: pathlib.Path, + report_text: str = "") -> pathlib.Path: + """Install a stub `claude` executable that appends its args to counter_file. + + The stub prints report_text to stdout and exits 0. Returns the stub path. + """ + counter_file = pathlib.Path(counter_file) + body = ( + f"printf '%s\\n' '$*' >> '{counter_file}'\n" + f"cat <<'REPORT_EOF'\n{report_text}\nREPORT_EOF" + ) + return make_stub_bin(bin_dir, "claude", body) + + +def with_path(bin_dir: pathlib.Path) -> dict: + """Return a copy of os.environ with bin_dir prepended to PATH.""" + env = dict(os.environ) + env["PATH"] = f"{bin_dir}{os.pathsep}{env.get('PATH', '')}" + return env diff --git a/prompts/completed/011-pr-review-bench-runner-core.md b/prompts/completed/011-pr-review-bench-runner-core.md new file mode 100644 index 0000000..a775043 --- /dev/null +++ b/prompts/completed/011-pr-review-bench-runner-core.md @@ -0,0 +1,336 @@ +--- +status: completed +spec: [002-pr-review-bench-runner] +summary: Created bench/run.py (configuration-identity core), bench/testsupport.py (test helpers), and bench/test_config.py (17 unit tests) implementing Desired Behaviors 1-3 and AC6/AC9/AC11 for the PR-review benchmark runner +execution_id: coding-bench-runner-exec-011-pr-review-bench-runner-core +dark-factory-version: v0.192.9 +created: "2026-08-06T21:05:00Z" +queued: "2026-08-06T21:40:07Z" +started: "2026-08-06T21:40:18Z" +completed: "2026-08-06T21:45:55Z" +--- + + +- Creates the benchmark runner's entrypoint file and its configuration-identity core +- A run is identified by the *content* of the review rules and commands, not by a git commit — so an uncommitted rule edit can be benchmarked before it is committed +- The review mode (short / full / selector) is part of that identity, so two modes can never be conflated under one key +- The runner refuses to start if the isolated Claude configuration directory would load the plugin from somewhere other than the repo whose hash it is about to record +- The PR manifest is validated up front: a missing field or a suspicious owner/repo name aborts before anything runs +- `--model`, `--effort` and `--mode` are mandatory; a guessed default would mislabel every recorded row +- `--golden` is recognised and rejected rather than silently ignored, so nobody believes a run was scored when it was not +- A `--print-config-hash` helper lets an operator confirm which content a result file refers to +- PR processing itself is not shipped here — each PR reports a loud "not yet implemented" failure so the gap cannot be mistaken for success +- No new dependencies: Python 3 standard library only + + + +Create `bench/run.py` with the benchmark runner's configuration-identity core: manifest loading and validation, content hashing of `rules/` + `commands/`, the plugin-resolution preflight that proves the recorded hash is the content that will actually run, and the full CLI surface with its exit-code contract. PR resolution and review invocation are stubbed with loud failures and land in later prompts. + + + +Read `CLAUDE.md` for project conventions (Python stdlib only, no personal paths, `make precommit` must stay green). +Read `specs/in-progress/002-pr-review-bench-runner.md` — this prompt implements Desired Behaviors 1, 2 and 3 and Acceptance Criteria AC6, AC9, AC11. +Read `bench/prs.json` — the frozen manifest this runner consumes. Note the exact field names on each entry: `id`, `owner`, `repo`, `number`, `language`, `merge_strategy`, `merge_sha`, `base_sha`, `head_sha`, `changed_files`, `additions`, `deletions`, `role`, `notes`; and the top-level fields `version`, `description`, `created`, `verified`, `prs`. +Read `bench/README.md` — current documentation (rewritten by prompt 4, not by this prompt). +Read `scripts/build-index.py` — the repo's existing stdlib-only Python script. Match its header-comment style (purpose, exit semantics, repo-root resolution from `__file__`, "no external dependencies") and its `sys.exit(main())` shape. +Read `scripts/check-coverage.sh` — the repo's precedent for a Python payload doing JSON + path work. + +Plugin resolution — this is a real on-disk layout, verified in this container, not an assumption. A Claude configuration directory `$CFG` resolves the `coding` plugin as follows: + +- `$CFG/plugins/known_marketplaces.json` is a JSON object keyed by marketplace name; the `coding` key holds an `installLocation` absolute path. Verified shape: + ```json + { + "coding": { + "source": { "source": "github", "repo": "bborbe/coding" }, + "installLocation": "/home/node/.claude/plugins/marketplaces/coding", + "lastUpdated": "2026-07-13T15:30:51.004Z" + } + } + ``` + Note `installLocation` may point outside `$CFG` entirely (other entries in the real file do), so it must be honoured verbatim when present. +- When that file is absent or has no usable `coding` entry, the conventional location is `$CFG/plugins/marketplaces/coding` — the same fallback `commands/pr-review.md` already uses: + ``` + [ -x "$RUNNER" ] || RUNNER="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/plugins/marketplaces/coding/scripts/ast-grep-runner.sh" + ``` + + + + +## 1. Create `bench/run.py` + +Executable Python 3 script (`chmod +x`), `#!/usr/bin/env python3` shebang, followed by a header comment block (≥10 lines) in the style of `scripts/build-index.py` covering: purpose (benchmark runner for `/coding:pr-review`), exit semantics (0 = every PR produced a row; 1 = one or more PRs failed; 2 = usage, manifest or preflight error), how paths are resolved from `__file__`, and "Python 3 standard library only — no third-party dependencies". + +Import only from the standard library. The permitted import set for this prompt is exactly: `argparse`, `hashlib`, `json`, `os`, `pathlib`, `re`, `sys`. Do not import anything else. + +## 2. Module constants + +```python +RUNNER_VERSION = "1" +REVIEW_TIMEOUT_SECONDS = 45 * 60 +BENCH_DIR = pathlib.Path(__file__).resolve().parent +REPO_ROOT = BENCH_DIR.parent +VERIFY_CONFIG_DIR_NAME = ".claude-verify" +HASHED_SUBDIRS = ("rules", "commands") +VALID_MODES = ("short", "full", "selector") +NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") +PR_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*#[0-9]+$") +REQUIRED_ENTRY_FIELDS = ( + "id", "owner", "repo", "number", + "merge_strategy", "merge_sha", "base_sha", "head_sha", "changed_files", +) +``` + +`REVIEW_TIMEOUT_SECONDS` is declared here and consumed in prompt 3. It is a fixed invariant — do NOT expose it as a CLI flag or an environment variable. + +## 3. `class BenchError(Exception)` + +A single exception type for every abort that maps to exit code 2 (manifest problems, preflight failures, lock contention). Its string form is printed to stderr by `main`. Define it directly under the constants. + +## 4. `def content_hash(root: pathlib.Path) -> str` + +Content-derived hash of the review configuration. Requirements: + +- Collect every regular file under `root/"rules"` and `root/"commands"` (recursively, via `pathlib.Path.rglob("*")`, keeping only `p.is_file()`). +- Skip any path that has a component named `.git`. +- Sort the collected paths by their POSIX-style path relative to `root` (`p.relative_to(root).as_posix()`), so the digest is independent of filesystem iteration order. +- Feed a SHA-256 with, for each file in that order: the relative POSIX path bytes, a `b"\0"` separator, the decimal byte length of the file, another `b"\0"`, then the raw file bytes. Length-framing prevents two different file layouts from producing the same byte stream. +- Return `h.hexdigest()`. +- If neither `root/"rules"` nor `root/"commands"` exists, raise `BenchError` naming `root` and stating that it does not look like a coding-plugin checkout. +- Never consult git. The digest must be identical for two directories with byte-identical `rules/` + `commands/` content regardless of git history, uncommitted edits elsewhere in the tree, or the presence/absence of `.git`. + +## 5. `def config_hash(rules_commands_hash, model, effort, mode, prs_version) -> str` + +SHA-256 hex digest over the five values joined with a separator that cannot appear in any of them: + +```python +payload = "\0".join([rules_commands_hash, model, effort, mode, prs_version]) +return hashlib.sha256(payload.encode("utf-8")).hexdigest() +``` + +Mode is a first-class component. Changing only `mode` must produce a different digest. + +## 6. `def load_manifest(path: pathlib.Path) -> dict` + +- Read and `json.loads` the file. On `OSError` raise `BenchError(f"cannot read manifest {path}: {err}")`. On `json.JSONDecodeError` raise `BenchError(f"manifest {path} is not valid JSON: {err}")`. +- Require top-level `version` (non-empty string) and `prs` (non-empty list). Missing or wrong-typed → `BenchError` naming the field. +- For each entry, in list order: + - Every field in `REQUIRED_ENTRY_FIELDS` must be present and non-empty (`0` counts as present but `changed_files` must be an `int`). On failure raise `BenchError(f"manifest entry {index} (id={entry.get('id')!r}): missing or empty required field {field!r}")` — the message must name both the entry id and the field. + - `id` must match `PR_ID_RE`, `owner` and `repo` must match `NAME_RE`, `number` must be an `int` greater than 0. On failure raise `BenchError` naming the entry id and the offending value. This is the path-traversal guard: `owner`/`repo` flow into filesystem paths and `git` argument lists, so values like `../evil` or `a/b` must be rejected here and nowhere else. + - `merge_sha`, `base_sha`, `head_sha` must each match `^[0-9a-f]{7,40}$`. +- Return the parsed dict unchanged (do not normalise or rewrite it — `bench/prs.json` is a frozen input). + +## 7. `def safe_pr_key(pr_id: str) -> str` + +Return `pr_id` with `#` replaced by `_`. Assume `pr_id` already passed `PR_ID_RE`, so no other character can appear. Used by prompts 2 and 3 for cache filenames. + +## 8. Plugin-resolution preflight + +```python +def verify_config_dir() -> pathlib.Path +``` +Return `pathlib.Path(os.environ["HOME"]) / VERIFY_CONFIG_DIR_NAME`. If `HOME` is unset or empty, raise `BenchError` explaining that the isolated Claude config directory `~/.claude-verify` cannot be located. Read `HOME` from `os.environ` at call time (never cache it at import time) — this is what lets tests point the runner at a temporary config directory without adding a CLI knob. + +```python +def resolve_plugin_path(config_dir: pathlib.Path) -> pathlib.Path +``` +1. If `config_dir/"plugins"/"known_marketplaces.json"` is a file, parse it; on invalid JSON raise `BenchError` naming the file. If the parsed object has a `"coding"` key whose value is a dict with a non-empty `"installLocation"` string, return `pathlib.Path(that_value)`. +2. Otherwise return `config_dir / "plugins" / "marketplaces" / "coding"`. + +```python +def check_plugin_resolution(coding_repo, config_dir, expected_hash) -> pathlib.Path +``` +- `plugin_path = resolve_plugin_path(config_dir)`. +- If `plugin_path` is not a directory, set `actual = ""`. Otherwise `actual = content_hash(plugin_path)`, and if `content_hash` itself raises `BenchError` (no `rules/`/`commands/` under it) set `actual = ""`. +- If `actual != expected_hash`, raise `BenchError` whose message begins with the literal `PLUGIN RESOLUTION MISMATCH` and additionally contains: `config_dir`, `plugin_path`, `actual`, `coding_repo`, `expected_hash`, and the sentence `refusing to record a configuration hash that did not run`. Both hashes must appear in the message. +- On success return `plugin_path`. + +This check runs before the PR loop, so a mismatch invokes zero reviews. + +## 9. CLI surface + +```python +def build_parser() -> argparse.ArgumentParser +``` + +| Flag | Type | Default | Notes | +|---|---|---|---| +| `--coding-repo` | path | `REPO_ROOT` | repo whose `rules/` + `commands/` are hashed and measured | +| `--manifest` | path | `BENCH_DIR / "prs.json"` | PR manifest | +| `--out-dir` | path | `BENCH_DIR / "results"` | result ledger directory | +| `--model` | str | `None` | mandatory except with `--golden` / `--print-config-hash` | +| `--effort` | str | `None` | mandatory except with `--golden` / `--print-config-hash` | +| `--mode` | str, `choices=VALID_MODES` | `None` | mandatory except with `--golden` / `--print-config-hash`; passed verbatim as `/coding:pr-review`'s second positional argument | +| `--golden` | path | `None` | reserved — always rejected | +| `--print-config-hash` | flag | `False` | print the `rules/`+`commands/` hash for `--coding-repo` and exit 0 | + +Do NOT declare `--model`/`--effort`/`--mode` as `required=True` — `--print-config-hash` and `--golden` must work without them. Enforce them after parsing (step 10). + +Do NOT add any other flag. In particular: no `--timeout`, no `--cache-dir`, no `--config-dir`, no `--jobs`, no `--retry`. The spec lists these as explicit non-goals. + +## 10. `def main(argv=None) -> int` + +In this exact order: + +1. `args = build_parser().parse_args(argv)`. +2. Wrap everything below in `try: ... except BenchError as err: print(str(err), file=sys.stderr); return 2`. +3. If `args.golden is not None`: print to stderr + `--golden is reserved but scoring is not implemented in this runner. Precision/recall and golden-set matching are future work in a separate spec; the runner stops at normalized findings. Re-run without --golden.` + and `return 2`. +4. If `args.print_config_hash`: `print(content_hash(args.coding_repo.resolve()))` and `return 0`. +5. Collect missing mandatory flags among `--model`, `--effort`, `--mode`. If any are missing, print to stderr `missing required argument(s): . These are part of the configuration identity recorded in every result row and have no safe default.` and `return 2`. +6. Otherwise call and return `run_bench(...)` (step 11), binding the fixed invariants here and only here: + - `cache_root=BENCH_DIR / ".cache"` + - `config_dir=verify_config_dir()` + - `results_dir=args.out_dir` + - `coding_repo=args.coding_repo.resolve()`, `manifest_path=args.manifest` + +End the file with: +```python +if __name__ == "__main__": + sys.exit(main()) +``` + +## 11. `def run_bench(*, coding_repo, manifest_path, results_dir, cache_root, model, effort, mode, config_dir) -> int` + +Keyword-only. This signature is the seam that later prompts extend and that tests bind to temporary directories — the fixed invariants are bound in `main`, never here, so no operator-facing knob is created. + +Body for this prompt: + +1. `manifest = load_manifest(manifest_path)`. +2. `rc_hash = content_hash(coding_repo)`. +3. `check_plugin_resolution(coding_repo, config_dir, rc_hash)` — raises on mismatch, aborting before any PR is touched. +4. `cfg_hash = config_hash(rc_hash, model, effort, mode, manifest["version"])`. +5. Print one configuration banner line to stdout: `config rules+commands model= effort= mode= prs=`. +6. For each entry in `manifest["prs"]`, call `process_pr(...)` (step 12) inside a `try/except BenchError` so a failing PR never aborts the remaining ones. Record a per-PR outcome of `ok`, `cache hit`, or `failed: `. +7. Print one summary line per PR to stdout in the form `: `, then a final line `summary: ok, cache hit, failed`. +8. Return `0` if every PR produced `ok` or `cache hit`, otherwise `1`. + +## 12. `def process_pr(...) -> tuple[str, str]` — fail-loud stub + +Signature (keyword-only): `entry, coding_repo, results_dir, cache_root, model, effort, mode, config_dir, cfg_hash, rc_hash, prs_version`. + +For this prompt the body is a single loud failure — PR resolution ships in prompt 2 and review invocation in prompt 3: + +```python +return ("failed", "pr resolution not yet implemented (prompt 2 of spec 002)") +``` + +Do not return a success outcome, do not write a row, do not create a cache entry. A stub that looked like a valid result would hide the gap. + +## 13. Create `bench/testsupport.py` — shared test helpers + +Not a `test_*.py` file, so `unittest discover -p 'test_*.py'` will not collect it as a suite, but tests can `import testsupport`. Stdlib only (`json`, `os`, `pathlib`, `shutil`, `subprocess`, `stat` as needed). + +```python +def make_coding_repo(root, *, rules=None, commands=None) -> pathlib.Path +``` +Create `root/"rules"` and `root/"commands"` and write the given `{relative_path: text}` mappings (defaults: one file each, e.g. `rules/go/sample.yml` and `commands/sample.md`). Return `root`. + +```python +def make_verify_config_dir(root, plugin_src, *, use_known_marketplaces=False) -> pathlib.Path +``` +Create `root/".claude-verify"`. When `use_known_marketplaces` is False, `shutil.copytree(plugin_src, cfg/"plugins"/"marketplaces"/"coding")`. When True, create `cfg/"plugins"` and write `known_marketplaces.json` containing `{"coding": {"source": {"source": "github", "repo": "bborbe/coding"}, "installLocation": str(plugin_src)}}`. Return the `.claude-verify` path. + +```python +def make_stub_bin(bin_dir, name, body) -> pathlib.Path +``` +Write `bin_dir/name` with `#!/bin/sh\n` + `body`, `chmod 0o755`, return the path. + +```python +def stub_claude(bin_dir, counter_file, report_text="") -> pathlib.Path +``` +Install a stub `claude` that appends its full argument list as one line to `counter_file` and prints `report_text` to stdout, exiting 0. Body shape: +```sh +printf '%s\n' "$*" >> "" +cat <<'REPORT_EOF' + +REPORT_EOF +``` +Return the stub path. + +```python +def with_path(bin_dir) -> dict +``` +Return a copy of `os.environ` with `bin_dir` prepended to `PATH`. (Used by prompts 2 and 3.) + +## 14. Create `bench/test_config.py` + +`import unittest`, `import run`, `import testsupport` (flat imports — `unittest discover -s bench` puts `bench/` on `sys.path`, and there is deliberately no `bench/__init__.py`). + +Tests, at minimum: + +1. **`test_content_hash_ignores_git_history_and_dirty_tree`** (AC6) — build two temp directories with byte-identical `rules/` + `commands/` content. Give one a `.git` directory containing arbitrary bytes plus an untracked junk file at its root (outside `rules/`/`commands/`); leave the other bare. Assert the two hashes are equal. Then mutate exactly one byte inside one directory's `rules/` file, recompute, assert inequality, and `print()` both hashes so the inequality case is visible in test output. +2. **`test_content_hash_is_order_independent`** — build the same content twice with files created in reverse order; hashes must match. +3. **`test_config_hash_distinguishes_mode`** — same `rc_hash`/model/effort/version, `mode="selector"` vs `mode="full"` → different digests; identical inputs → identical digests. +4. **`test_load_manifest_rejects_missing_field`** — an entry missing `head_sha` raises `BenchError` whose message contains both the entry id and `head_sha`. +5. **`test_load_manifest_rejects_invalid_json`** — raises `BenchError` naming the path. +6. **`test_load_manifest_rejects_traversal_owner`** — `owner="../evil"` and separately `repo="a/b"` each raise `BenchError`. +7. **`test_load_manifest_accepts_real_fixture`** — `run.load_manifest(run.BENCH_DIR / "prs.json")` succeeds and returns `version == "dev-1"` with 5 entries. This is the boundary test: the shipped frozen manifest must pass the shipped validator. +8. **`test_plugin_resolution_mismatch_aborts_before_any_review`** (AC9) — build coding repo A and a `.claude-verify` whose copied plugin has different `rules/` content; install `stub_claude` with a counter file on `PATH`; call `run.run_bench(...)` with `config_dir` pointed at that `.claude-verify` and assert: `BenchError` is raised, `str(err)` starts with `PLUGIN RESOLUTION MISMATCH`, contains both hex hashes, and the counter file does not exist or has 0 lines. +9. **`test_plugin_resolution_honors_install_location`** — with `use_known_marketplaces=True` pointing at the same coding repo, `check_plugin_resolution` succeeds and `resolve_plugin_path` returns exactly `plugin_src`. +10. **`test_plugin_resolution_falls_back_to_marketplaces_dir`** — with no `known_marketplaces.json`, `resolve_plugin_path` returns `/plugins/marketplaces/coding`. +11. **`test_golden_flag_exits_two`** (AC11) — run the real process: `subprocess.run([sys.executable, str(run.BENCH_DIR / "run.py"), "--golden", "bench/golden.json"], capture_output=True, text=True)`; assert `returncode == 2` and that stderr contains `scoring` and `future`. +12. **`test_print_config_hash_matches_content_hash`** — subprocess `[sys.executable, run.py, "--print-config-hash", "--coding-repo", ]` exits 0 and its stdout stripped equals `run.content_hash(temp_repo)`. +13. **`test_missing_required_flag_exits_two`** — invoking with `--model x --effort y` but no `--mode` exits 2 with `--mode` named in stderr. +14. **`test_missing_model_and_effort_flags_exit_two`** — the same check for `--model` and for `--effort` individually, each naming the missing flag in stderr. The three flags are the configuration identity; a default silently applied to any one of them mislabels every row in the run, so all three are asserted, not just the one that happened to be written first. +15. **`test_known_marketplaces_invalid_json_raises`** — requirement 8.1 specifies `BenchError` naming the file when `known_marketplaces.json` is present but unparseable. Write a temp config dir containing that file with malformed JSON and assert the raise and the filename in the message. Untested, this path degrades to a silent fallback to the marketplaces dir, which is exactly the wrong plugin source and the failure DB3 exists to catch. +16. **`test_verify_config_dir_without_home_raises`** — with `HOME` removed from the environment, `verify_config_dir` raises `BenchError` naming the isolated config dir. Behaviour is specified in requirement 8; asserting it keeps the env read at call time rather than drifting to import time. + +Every test that needs temp directories uses `tempfile.TemporaryDirectory` and cleans up. No test may require network, a real `claude` binary, or GitHub access. + +## 14a. Do not modify + +Do not touch `Makefile`, `bench/README.md`, `CHANGELOG.md`, `bench/prs.json`, or anything under `rules/`, `commands/`, `agents/`, `docs/` — those belong to prompt 4 or are frozen inputs. `make precommit`'s target list is unchanged by this prompt. + + + +- Python 3 standard library only — no `pip`, no `requirements.txt`, no `pyproject.toml`, no third-party imports +- One runner file at `bench/run.py`; test helpers in `bench/testsupport.py`; tests in `bench/test_*.py`. No `bench/__init__.py` +- No personal paths anywhere (`/Users/`, `~/Documents/`) in any file created or edited +- `bench/prs.json` is a frozen input — its schema, its five entries and its `dev-1` version are not modified +- No rule, agent, command or doc that participates in a review is edited — measuring the current configuration is the point +- Fixed invariants, not configurable: 45-minute review timeout, cache under `bench/.cache/`, results under `bench/results/`, isolated config dir `$HOME/.claude-verify`. Do NOT add flags or env vars for any of them +- Do NOT add a retry loop, a parallelism knob, or any scoring logic +- Do NOT commit — dark-factory handles git +- Existing checks must still pass: `make precommit` exits 0 + + + +``` +# Runner exists, is executable, stdlib-only +test -x bench/run.py && echo "executable: ok" +grep -nE '^(import |from )' bench/run.py + +# No personal paths +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" + +# Unit tests pass +python3 -m unittest discover -s bench -p 'test_*.py' -v + +# Reserved --golden flag +python3 bench/run.py --golden bench/golden.json ; echo "golden exit=$? (expect 2)" + +# Mandatory flags enforced +python3 bench/run.py --model m --effort e ; echo "missing-mode exit=$? (expect 2)" + +# Config hash is printable and stable +python3 bench/run.py --print-config-hash ; echo "print-hash exit=$? (expect 0)" +A=$(python3 bench/run.py --print-config-hash) +B=$(python3 bench/run.py --print-config-hash) +[ "$A" = "$B" ] && echo "hash stable: ok" + +# Frozen manifest still validates +python3 -c " +import sys, pathlib +sys.path.insert(0, 'bench') +import run +m = run.load_manifest(pathlib.Path('bench/prs.json')) +assert m['version'] == 'dev-1', m['version'] +assert len(m['prs']) == 5, len(m['prs']) +print('manifest ok:', m['version'], len(m['prs']), 'entries') +" + +# Repo checks unchanged +make precommit +``` + diff --git a/prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md b/prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md new file mode 100644 index 0000000..6853dc6 --- /dev/null +++ b/prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md @@ -0,0 +1,338 @@ +--- +status: approved +spec: [002-pr-review-bench-runner] +created: "2026-08-06T21:06:00Z" +queued: "2026-08-06T21:40:07Z" +--- + + +- Teaches the benchmark runner to reconstruct each merged pull request's diff on its own, without any GitHub CLI +- All repository work happens inside the runner's private cache — the operator's real clones are never touched, opened or mutated +- Pull requests are fetched from the repository the manifest names, not from whatever remote happens to be called `origin` (one fixture PR lives on a fork, so an `origin` assumption fails outright) +- The diff range is chosen by counting the merge commit's actual parents, so a squashed pull request is never silently reduced to an empty diff +- A resolved range that touches zero files aborts that pull request loudly instead of being recorded as a clean review +- A failing pull request is isolated: the remaining ones still run, and the overall run reports failure +- Re-runs are offline-friendly: if the needed commits are already in the cache, no network fetch is attempted +- Review invocation itself still reports a loud "not yet implemented" failure — it lands in the next prompt + + + +Extend `bench/run.py` so it resolves each manifest pull request into a checked-out working copy and a correct diff range, entirely inside `bench/.cache/repos/`. Diff-range selection branches on the merge commit's actual parent count; an empty diff aborts that PR loudly; every `git` invocation is structurally confined to the runner's own cache. Review invocation remains a loud stub. + + + +Read `CLAUDE.md` for project conventions. +Read `specs/in-progress/002-pr-review-bench-runner.md` — this prompt implements Desired Behaviors 4, 5 and 6 and Acceptance Criteria AC2, AC3, AC7, AC8. +Read `bench/run.py` (created by prompt 1) — you are extending it. Reuse `BenchError`, `load_manifest`, `safe_pr_key`, `run_bench` and the `process_pr` stub; do not restructure them. +Read `bench/testsupport.py` (created by prompt 1) — you are extending it with git-repo and stub-`git` helpers. +Read `bench/prs.json` — note `node-skeleton#2` is the only `squash` entry and its `head_sha` equals its `merge_sha`; the other four are `merge-commit`. +Read `bench/README.md` — its current squash snippet is superseded by the spec (see the Constraints section of the spec: *"This spec is the binding contract for diff-range mechanics, superseding `bench/README.md` where the two disagree"*). Prompt 4 rewrites the README; this prompt implements the spec's rule, not the README's. +Read `commands/pr-review.md` Step 0a-pre through Step 0c — this is the consumer of the working copy you are preparing. Step 0c resolves the diff as `git diff origin/...HEAD`, which resolves through the ref namespace and is why the runner must create `refs/remotes/origin/` refs locally. + +Known and accepted: Step 0a-pre's fast-path short-circuit begins with `git fetch origin `, which needs an actual `[remote "origin"]` config entry. `ensure_refs` fetches from an anonymous manifest-derived URL and never creates one, so that fetch fails and the short-circuit does not fire — execution falls through to Step 0b, which creates its own `/tmp` worktree. That is slower but still correct, because the refs Step 0b and 0c need already exist locally. **Do not "fix" this by adding a `git remote add origin …`**: the fixture PRs live on repositories where `origin` is the wrong remote (`tts-mcp`'s `origin` is upstream `florianbuetow/tts-mcp` while the PR is on the fork), and creating a remote literally named `origin` is how that class of bug returns. The extra worktree hop is the accepted cost of never depending on a remote name. + + + + +## 1. New imports + +Add `dataclasses`, `shutil` and `subprocess` to `bench/run.py`'s stdlib import list. No third-party imports. + +## 2. Path helpers + +```python +def repos_root(cache_root: pathlib.Path) -> pathlib.Path: + return cache_root / "repos" + +def repo_cache_dir(cache_root, owner, repo) -> pathlib.Path: + return repos_root(cache_root) / owner / repo + +def worktree_dir(cache_root, owner, repo, number) -> pathlib.Path: + return repos_root(cache_root) / owner / f"{repo}__pr{number}" +``` + +The worktree lives beside the bare-ish clone and **under `repos/`**, not in a sibling directory, so it is covered by the same containment invariant. + +```python +def assert_under(path: pathlib.Path, root: pathlib.Path) -> pathlib.Path +``` +Resolve both; raise `BenchError` unless `resolved != root_resolved` and `resolved.is_relative_to(root_resolved)`. The message must name both `path` and `root` and state that the runner only ever touches its own cache. Return the resolved path. + +## 3. `def fetch_url(owner: str, repo: str) -> str` + +```python +return f"https://github.com/{owner}/{repo}" +``` + +Derived purely from the manifest's `owner`/`repo`. The runner must never read, query or depend on a remote named `origin` to decide where to fetch from — `bborbe/tts-mcp#20`'s PR lives on the fork while that repo's `origin` is upstream `florianbuetow/tts-mcp`, so `git fetch origin pull/20/head` fails there with "couldn't find remote ref". + +## 4. `def git(args, *, repo_dir, cache_root, check=True, timeout=600)` — the single git chokepoint + +Every `git` invocation the runner ever issues goes through this function. No other function may call `subprocess` with `git`. + +```python +def git(args, *, repo_dir, cache_root, check=True, timeout=600): + target = assert_under(repo_dir, repos_root(cache_root)) + cmd = ["git", "-C", str(target), *args] + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) + if check and proc.returncode != 0: + raise BenchError(f"git {' '.join(args)} failed in {target} (exit {proc.returncode}): {proc.stderr.strip()}") + return proc +``` + +- Always `-C `; never pass `cwd=`, never build a shell string, never interpolate a manifest value into one argument. Manifest values are passed as separate list elements. +- `repo_dir` must already exist (create it with `mkdir(parents=True, exist_ok=True)` before the first call). `git -C init` works, which is why the runner never needs `git clone` and therefore never needs a cwd-based invocation. +- `assert_under` runs before the subprocess, so a manifest value that somehow escaped validation still cannot target a path outside `bench/.cache/repos/`. +- `subprocess.TimeoutExpired` propagates; `process_pr` converts it into a per-PR failure. + +## 5. `def ensure_refs(cache_root, entry) -> pathlib.Path` + +Prepare `repo_cache_dir(...)` so the entry's three SHAs are locally reachable. Returns the repo directory. + +1. `repo_dir.mkdir(parents=True, exist_ok=True)`. +2. If `repo_dir/".git"` does not exist, `git(["init", "--quiet"], ...)`. +3. **Offline short-circuit** — if all three of `merge_sha`, `base_sha`, `head_sha` already resolve locally, skip the fetch entirely. Test each with `git(["cat-file", "-e", f"{sha}^{{commit}}"], check=False)` and treat `returncode == 0` as resolved. This makes re-runs cheap and is the mechanism that lets the unit tests exercise the whole resolution path with no network. +4. Otherwise fetch once, from the manifest URL: + ```python + url = fetch_url(entry["owner"], entry["repo"]) + git(["fetch", "--no-tags", "--force", url, + f"+pull/{entry['number']}/head:refs/bench/pr{entry['number']}/head", + "+refs/heads/*:refs/remotes/origin/*"], + repo_dir=repo_dir, cache_root=cache_root) + ``` + The first positional argument after the flags is the URL string — never the token `origin`. The `refs/remotes/origin/*` destination is a *local ref namespace* (needed so `/coding:pr-review` can resolve `origin/` in the prepared working copy), not a remote name, and must not be confused with one. +5. If the fetch fails, `BenchError` from `git()` propagates and `process_pr` records the PR as failed with the underlying git stderr. The outcome string is the generic `failed: ` form defined in requirement 10 — do NOT introduce a separate `failed: fetch` literal. A fetch failure's first line already begins with `git fetch …`, which identifies the phase; a second, phase-specific literal would be a classification the generic handler cannot produce and no test asserts. + +## 6. `def resolve_diff_range(cache_root, repo_dir, entry) -> tuple[str, str, str, int]` + +Returns `(diff_range, base_endpoint, head_endpoint, parent_count)`. + +```python +out = git(["rev-list", "--parents", "-n", "1", entry["merge_sha"]], + repo_dir=repo_dir, cache_root=cache_root).stdout.split() +if not out: + raise BenchError(f"{entry['id']}: cannot resolve merge commit {entry['merge_sha']}") +n_parents = len(out) - 1 +if n_parents >= 2: + base = f"{entry['merge_sha']}^1" + head = f"{entry['merge_sha']}^2" +elif n_parents == 1: + base = entry["base_sha"] + head = entry["head_sha"] +else: + raise BenchError(f"{entry['id']}: merge commit {entry['merge_sha']} has no parents; cannot reconstruct a diff range") +return f"{base}..{head}", base, head, n_parents +``` + +This is the whole rule. There is **no fallback branch** — no "second parent, else the merge commit itself" heuristic, and the manifest's `merge_strategy` label never selects the range. A single-parent commit always uses the manifest's recorded `base_sha..head_sha`, never a parent-derived range. + +**Strategy-label mismatch is reported, never obeyed.** If `entry["merge_strategy"] == "merge-commit"` and `n_parents == 1`, or `entry["merge_strategy"] == "squash"` and `n_parents >= 2`, append the note `strategy mismatch (manifest={label}, parents={n})` to the PR's notes list. It is cosmetic — the run already used the correct range — and must not fail the PR. + +## 7. `def changed_files(cache_root, repo_dir, diff_range) -> list[str]` + +```python +proc = git(["diff", "--name-only", diff_range], repo_dir=repo_dir, cache_root=cache_root) +return [ln for ln in proc.stdout.splitlines() if ln.strip()] +``` + +Pass the same `diff_range` string that gets recorded in the result row, so the executed range and the recorded range can never drift. Apply **no** path exclusions — the manifest's `changed_files` counts came from `gh api repos///compare/...` with no exclusions, and the recorded count must be comparable to them. + +## 8. `def prepare_worktree(cache_root, repo_dir, entry, base_endpoint, head_endpoint) -> PrCheckout` + +Resolve both endpoints to full SHAs, publish the two remote-tracking refs `/coding:pr-review` needs, and create the working copy. + +```python +base_branch = f"bench-base-{entry['number']}" +head_branch = f"bench-pr-{entry['number']}" +``` + +1. `base_sha = git(["rev-parse", f"{base_endpoint}^{{commit}}"], ...).stdout.strip()` and likewise `head_sha` from `head_endpoint`. +2. `git(["update-ref", f"refs/remotes/origin/{base_branch}", base_sha], ...)` and the same for `refs/remotes/origin/{head_branch}` → `head_sha`. These make `origin/bench-base-` resolvable in the working copy, which is what `commands/pr-review.md` Step 0c diffs against. +3. Tear down any stale copy from a previous run, ignoring failures: + - `git(["worktree", "remove", "--force", str(wt)], ..., check=False)` + - `shutil.rmtree(wt, ignore_errors=True)` after `assert_under(wt, repos_root(cache_root))` — the `assert_under` call is mandatory before any `rmtree` + - `git(["branch", "-D", head_branch], ..., check=False)` + - `git(["worktree", "prune"], ..., check=False)` +4. `assert_under(wt, repos_root(cache_root))` again, then `git(["worktree", "add", "--force", "-b", head_branch, str(wt), head_sha], ...)`. The `git()` chokepoint only validates the `-C` repository argument, so `wt` — which is passed as a positional path and is where `--force` will overwrite — must be validated explicitly. Same defense-in-depth rule as step 3's `rmtree`: every path this function hands to a destructive operation is asserted at the call site, never trusted because of where it came from. +5. Return the checkout record. + +```python +@dataclasses.dataclass +class PrCheckout: + pr_id: str + repo_dir: pathlib.Path + worktree: pathlib.Path + base_branch: str + head_branch: str + diff_range: str + base_sha: str + head_sha: str + changed_files: int + parent_count: int + notes: list +``` + +## 9. `def resolve_pr(cache_root, entry) -> PrCheckout` + +Ties steps 5–8 together in order: `ensure_refs` → `resolve_diff_range` → `changed_files` → **empty-diff gate** → `prepare_worktree`. + +The empty-diff gate runs **before** the worktree is created and before any review could be invoked: + +```python +files = changed_files(cache_root, repo_dir, diff_range) +if not files: + raise BenchError( + f"EMPTY DIFF: {entry['id']} resolved range {diff_range} contains zero changed files. " + f"This is never recorded as a zero-finding review — two independent code paths produce " + f"this state and both look identical to a genuinely clean PR. " + f"Re-verify the SHAs with: gh api repos/{entry['owner']}/{entry['repo']}/compare/{entry['base_sha']}...{entry['head_sha']} --jq '.files | length'" + ) +``` + +The message must contain the literal `EMPTY DIFF`, the PR id, and the resolved range. + +## 10. Rewrite `process_pr` — resolution now real, review still stubbed + +``` +1. checkout = resolve_pr(cache_root, entry) # may raise BenchError +2. return ("failed", "review invocation not yet implemented (prompt 3 of spec 002)") +``` + +Do NOT append a result row, do NOT create a cache entry, do NOT return a success outcome. Prompt 3 replaces step 2. + +`run_bench` must catch, per PR and without aborting the loop: +- `BenchError` → outcome `failed: ` +- `subprocess.TimeoutExpired` → outcome `failed: timeout` +- `OSError` → outcome `failed: ` (this is the disk-exhaustion path) + +Each failing PR prints its full error to stderr, contributes no row and no cache entry, and the process exit code becomes 1. Any note collected on a checkout (e.g. `strategy mismatch (...)`) is appended to that PR's summary line. + +## 11. Extend `bench/testsupport.py` + +```python +def init_git_repo(path) -> pathlib.Path +``` +`git init` a directory and configure `user.email`/`user.name` locally so commits work in a bare container. Return the path. + +```python +def commit_file(repo, relpath, text, message) -> str +``` +Write the file, `git add`, `git commit`, return the resulting full SHA. + +```python +def make_merge_repo(path) -> dict +``` +Build a repo with a real two-parent merge commit: commit `base` on the default branch, branch off, commit a change on the branch, return to the default branch, commit a second change, then `git merge --no-ff `. Return `{"repo": path, "merge_sha": ..., "base_sha": ..., "head_sha": ...}` where `base_sha`/`head_sha` are the merge's first and second parents. Ensure the branch's change touches at least one file the default branch did not, so both endpoints differ. + +```python +def make_squash_repo(path) -> dict +``` +Build a repo whose final commit has exactly one parent and differs from its parent in ≥1 file. Return `{"repo": path, "merge_sha": , "base_sha": , "head_sha": }` — mirroring `node-skeleton#2`, where `head_sha == merge_sha`. + +```python +def make_empty_diff_repo(path) -> dict +``` +Build a single-parent repo and return an entry whose `base_sha` and `head_sha` are **the same commit**, so the resolved range yields zero changed files. + +```python +def stub_git(bin_dir, log_file) -> pathlib.Path +``` +Install a stub `git` on `PATH` that appends one line per invocation to `log_file` recording the process cwd and the full argument list, then exits 0 with empty stdout. Body shape: +```sh +printf 'cwd=%s args=%s\n' "$(pwd)" "$*" >> "" +exit 0 +``` + +```python +def make_manifest(path, entries, version="test-1") -> pathlib.Path +``` +Write a minimal valid manifest JSON to `path` and return it. + +## 12. Extend `bench/test_resolve.py` (new file) + +`import unittest`, `import run`, `import testsupport`. All tests must run offline with no `claude` binary and no GitHub access. + +1. **`test_diff_range_branches_on_parent_count`** (AC2) — build a merge repo and a squash repo in one temp dir (both placed under a temp `cache_root/repos/...` so `assert_under` is satisfied). Assert: + - merge repo → `resolve_diff_range` returns range `f"{merge_sha}^1..{merge_sha}^2"` and `parent_count == 2` + - squash repo → returns exactly `f"{base_sha}..{head_sha}"` from the manifest entry and `parent_count == 1` + - `changed_files(...)` for each range has length ≥ 1 + Use an assertion message that names both ranges, e.g. `msg=f"merge_range={merge_range!r} squash_range={squash_range!r}"`. The test name must contain `parent_count`. +2. **`test_squash_range_is_manifest_derived_not_parent_derived`** — for the squash repo, construct an entry whose `base_sha` is deliberately an *older* commit than the merge commit's parent, and assert the returned range uses the manifest's `base_sha`, proving parent traversal is not consulted for single-parent commits. +3. **`test_strategy_label_mismatch_is_noted_not_obeyed`** — a two-parent repo whose entry claims `merge_strategy: "squash"` still yields the `^1..^2` range, and the note list contains `strategy mismatch`. +4. **`test_empty_diff_aborts_loudly`** (AC3) — pre-seed `cache_root/repos//` with `make_empty_diff_repo` so no fetch is attempted, plus a matching `.claude-verify` config dir so the preflight passes, plus `stub_claude` with a counter file on `PATH`. Call `run.run_bench(...)`. Assert: return code is non-zero; the captured stderr contains `EMPTY DIFF`, the PR id and the range; `results_dir/"results.jsonl"` either does not exist or has the same line count as before the call (0); `cache_root/"reviews"` does not exist or contains no files; the stub-`claude` counter file has 0 lines. +5. **`test_fetch_url_is_built_from_manifest_owner_repo`** (AC8) — assert `run.fetch_url("bborbe", "tts-mcp") == "https://github.com/bborbe/tts-mcp"`. Then, with `stub_git` on `PATH`, drive `ensure_refs` for an entry whose `owner`/`repo` is `bborbe/tts-mcp` in a repo dir that has a remote literally named `origin` pointing at `https://github.com/florianbuetow/tts-mcp`, and assert the logged `fetch` invocation's first positional argument after the flags is exactly `https://github.com/bborbe/tts-mcp` and that the token `origin` never appears as that positional argument. +6. **`test_fetch_url_used_when_no_origin_remote_exists`** (AC8, second case) — same assertion in a repo dir with no remotes configured at all; the constructed URL is unchanged, proving `origin` is never consulted. +7. **`test_every_git_invocation_stays_under_cache_repos`** (AC7) — with `stub_git` logging to a file and a two-PR temp manifest (and a passing plugin preflight), call `run.run_bench(...)`. Then: + - assert the log file has **at least one line per manifest PR** (≥2), so the check cannot pass vacuously through an early abort with zero git invocations + - for each logged line, take the value following `-C` if present, otherwise the recorded `cwd` + - assert each such target path is a strict prefix-match of `/repos/`, with a failure message naming the offending path verbatim +8. **`test_assert_under_rejects_outside_path`** — `assert_under(pathlib.Path("/tmp"), repos_root(cache))` raises `BenchError`, and `assert_under(repos_root(cache), repos_root(cache))` also raises (the root itself is not a strict prefix match). +9. **`test_failing_pr_does_not_abort_remaining_prs`** — a two-PR manifest where the first entry's `merge_sha` is unresolvable and the second resolves cleanly; assert both PRs appear in the summary, the first as `failed`, and `run_bench` returns 1. +10. **`test_worktree_is_created_under_repos_root`** — after `resolve_pr` on a seeded merge repo, the returned `worktree` exists, is a directory under `/repos/`, has `HEAD` at the resolved head SHA, and `git -C rev-parse --abbrev-ref HEAD` prints `bench-pr-`. Also assert `git -C rev-parse refs/remotes/origin/bench-base-` resolves to the base SHA — this is the boundary the `/coding:pr-review` command crosses in its Step 0c `git diff origin/...HEAD`. + +## 13. Do not modify + +Do not touch `Makefile`, `bench/README.md`, `CHANGELOG.md`, `bench/prs.json`, `rules/`, `commands/`, `agents/` or `docs/`. + + + +- Python 3 standard library only — no `pip`, no `requirements.txt`, no third-party imports +- **Every `git` invocation goes through the single `git()` chokepoint, always with `-C `.** No `cwd=` argument, no `git clone`, no shell string, no invocation targeting a path outside that prefix. `/coding:pr-review` holds `git worktree`, `git fetch`, `git branch` and `rm -rf` permissions, so a runner that reused an operator's real clone could destructively mutate it +- Every subprocess is invoked with an argument list; no manifest value is ever interpolated into a shell command +- No `shutil.rmtree` without a preceding `assert_under(..., repos_root(cache_root))` +- Fetch targets the URL built from the manifest's `owner`/`repo`; the runner never depends on a remote named `origin` +- Diff-range selection branches on actual parent count only. No fallback branch, no reliance on the manifest's `merge_strategy` label +- Zero changed files aborts that PR loudly with the literal `EMPTY DIFF` — never recorded as a zero-finding review +- A failed PR produces no row and no cache entry; remaining PRs still run; process exits non-zero +- Fixed invariants, not configurable: 45-minute review timeout, cache under `bench/.cache/`, results under `bench/results/`, config dir `$HOME/.claude-verify`. Do NOT add flags or env vars for any of them +- Do NOT add a retry loop around a failed fetch or review +- No personal paths anywhere (`/Users/`, `~/Documents/`) +- `bench/prs.json` is a frozen input +- Do NOT commit — dark-factory handles git +- All new tests must run offline: no network, no real `claude` binary, no GitHub access + + + +``` +# Stdlib-only imports +grep -nE '^(import |from )' bench/run.py + +# No personal paths +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" + +# Single git chokepoint: every subprocess git call is inside git() +grep -n 'subprocess.run' bench/run.py +grep -n '"git"' bench/run.py +# Expect exactly one subprocess.run building a git command, inside def git(...) + +# No cwd-based subprocess invocation +grep -n 'cwd=' bench/run.py ; echo "cwd grep exit=$? (expect 1 for this prompt)" + +# Unit tests pass, including the four AC tests +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tee /tmp/bench-tests.log +grep -c 'parent_count' /tmp/bench-tests.log +grep -c 'empty_diff' /tmp/bench-tests.log +grep -c 'stays_under_cache_repos' /tmp/bench-tests.log +grep -c 'fetch_url' /tmp/bench-tests.log + +# The empty-diff message really carries the literal +grep -n 'EMPTY DIFF' bench/run.py + +# Reserved flag and mandatory flags unchanged from prompt 1 +python3 bench/run.py --golden bench/golden.json ; echo "golden exit=$? (expect 2)" + +# Frozen manifest still validates +python3 -c " +import sys, pathlib +sys.path.insert(0, 'bench') +import run +m = run.load_manifest(pathlib.Path('bench/prs.json')) +print('manifest ok:', m['version'], len(m['prs']), 'entries') +for e in m['prs']: + print(' ', e['id'], run.fetch_url(e['owner'], e['repo'])) +" + +# Repo checks unchanged +make precommit +``` + diff --git a/specs/in-progress/002-pr-review-bench-runner.md b/specs/in-progress/002-pr-review-bench-runner.md new file mode 100644 index 0000000..0f97590 --- /dev/null +++ b/specs/in-progress/002-pr-review-bench-runner.md @@ -0,0 +1,165 @@ +--- +status: prompted +tags: + - dark-factory + - spec +approved: "2026-08-06T20:54:14Z" +generating: "2026-08-06T20:54:15Z" +prompted: "2026-08-06T21:04:56Z" +branch: dark-factory/pr-review-bench-runner +--- + +## Summary + +- Ship the benchmark **runner** — the instrument that takes one review configuration (rule/command content, model, effort, review mode) plus a pinned PR set and produces a machine-readable result file. Scoring is a later spec. +- The runner drives the real `/coding:pr-review` slash command against each already-merged PR in `bench/prs.json`, in an isolated Claude config directory with autoupdate disabled. +- Every result row pins what actually ran: a content hash of `rules/` + `commands/`, the model, the effort level, and the fixture version — never a version number the run cannot prove. +- Two silent-failure classes are made loud: a merge-strategy misread that yields `base == head`, and any empty diff. Both currently look identical to a genuinely clean PR. +- A per-(PR, configuration) cache means a failure on the fifth PR never discards the first four, and re-running the same configuration costs zero review invocations. + +## Problem + +The PR-review behaviour of this repo is governed by 166 rules, a slash command, a model choice, and an effort level — and 84 of those rules are judgment-tier, meaning no ast-grep YAML or unit test can tell whether they still fire. Today every change to any of those four inputs ships blind: an author edits a rule, eyeballs one PR, and merges. There is no way to answer "did that edit make reviews better or worse" because there is no instrument that runs a configuration against a fixed PR set and writes down what it found. `bench/prs.json` (the pinned five-PR development fixture) and `bench/README.md` landed in v0.35.0, but nothing consumes them. Until something does, the outcome tier of this repo's test pyramid stays empty and every later question — scoring, noise floor, model comparison — is unanswerable because the measurement device does not exist. + +## Goal + +A single command turns one review configuration plus a PR manifest into a durable result file. For each PR in the manifest the system resolves the correct diff range from the recorded SHAs, proves that the review will execute against the exact rule and command content it is about to record a hash for, invokes the real `/coding:pr-review` command, and appends one append-only row carrying the pinned configuration identity and the normalized findings. Silent no-ops are impossible: any empty diff and any plugin-resolution mismatch abort loudly instead of being recorded as a clean review. Repeating a configuration is free — already-completed (PR, configuration) pairs are served from cache and invoke nothing. + +## Non-goals + +- Do NOT build the golden set or any scoring semantics (precision, recall, per-finding matching) — separate future spec. The runner stops at normalized findings. +- Do NOT measure the noise floor (run-to-run variance of the same configuration) — separate future spec. +- Do NOT compare models or prove discrimination between configurations — separate future spec. +- Do NOT curate the 15-20 PR representative set — separate future spec. This spec consumes `bench/prs.json` (`dev-1`) as-is. +- Do NOT score `/coding:code-review` or `/coding:local-review` as standalone configurations — separate future spec. The runner drives `/coding:pr-review` only. +- Do NOT change `bench/prs.json`'s schema or content — it is a frozen input here. +- Do NOT change any command, agent, rule, or doc that participates in a review — measuring the current configuration is the entire point; changing it while building the instrument destroys the baseline. +- Do NOT add tunable knobs for the review timeout, the cache location, or the isolated config directory — these are fixed invariants; if a future consumer demands variation, that is a separate spec. +- Do NOT add a retry loop around failed reviews — a failed PR is reported and left uncached so the next invocation retries it naturally. + +## Acceptance Criteria + +Each AC is tagged **[container]** (the YOLO container verifies it at prompt time — no network, no real Claude subprocess, no tokens) or **[operator]** (only a human on the host can observe it, because it spends real tokens against the live `/coding:pr-review` command over the five fixture PRs). + +- [ ] **AC1 [container]** `make precommit` exits 0, and its output includes the bench unit-test target's result line — evidence: exit code 0; stdout contains `bench-test` and `OK`. +- [ ] **AC2 [container]** Merge-commit branching is driven by actual parent count, not a fallback: a unit test builds two throwaway git repositories in a temp dir — one with a two-parent merge commit, one with a single-parent (squash-shaped) commit — and asserts the resolver returns `^1..^2` for the first and the recorded `base_sha..head_sha` for the second, with both ranges reporting ≥1 changed file — evidence: test exits 0; test name contains `parent_count`; assertion failure message names both ranges. +- [ ] **AC3 [container]** An empty diff aborts loudly instead of being recorded: a unit test invokes the runner for a PR whose base and head resolve to the same commit — evidence: process exit code is non-zero, stderr contains the literal `EMPTY DIFF`, the results file gains 0 lines (`wc -l` before == after), and no cache entry is created for that (PR, configuration) pair (`ls bench/.cache/reviews/` shows no new file). +- [ ] **AC4 [container]** A second invocation of the same configuration invokes zero reviews: with a stub `claude` executable on `PATH` that appends one line per invocation to a counter file, run the runner twice over a two-PR temp manifest — evidence: counter file has exactly 2 lines after the first run and still exactly 2 after the second; results file line count is identical after both runs; second run's stdout contains `cache hit` once per PR. +- [ ] **AC5 [container]** Changing only `--mode` is a cache miss, not a cache hit: with the same stub-`claude` counter harness as AC4, run the runner once with `--mode selector` over a one-PR temp manifest, then again with identical arguments except `--mode full` — evidence: counter file has 1 line after the first run and 2 lines after the second (the second invocation was not served from cache); the results file gains a second row whose `mode` field differs from the first row's; the invoked command string recorded in each row contains its own mode literally (`selector` in row 1, `full` in row 2). +- [ ] **AC6 [container]** The configuration hash is content-derived, not commit-derived: a unit test hashes two directories with byte-identical `rules/` + `commands/` content but different git histories (and one with a dirty working tree) and asserts equal hashes; mutating one byte under `rules/` yields a different hash — evidence: test exits 0; stdout prints both hashes for the inequality case. +- [ ] **AC7 [container]** Every git invocation the runner issues stays under its own cache directory — never inside a repository the operator uses for real work: with a stub `git` executable on `PATH` that appends every invocation's argument list (including any `-C` value or the cwd it was run from) to a log file, run the runner against a two-PR temp manifest — evidence: the log file has at least one line per manifest PR (so the assertion cannot pass vacuously via an early abort with zero invocations logged); every logged invocation's target path (the `-C` argument, or the process cwd if no `-C` is given) is a strict prefix-match of `bench/.cache/repos/`; the assertion fails loudly (naming the offending path) if any logged invocation targets a path outside that prefix. +- [ ] **AC8 [container]** PR resolution fetches from the owning remote, never from an assumed `origin`: a unit test builds a manifest entry whose `owner`/`repo` differ from a local stub remote named `origin`, and asserts the constructed fetch URL is built from the manifest's `owner`/`repo` pair (`https://github.com//` + `pull//head`), not from whatever `origin` resolves to — evidence: test exits 0; assertion compares the exact constructed URL string; a second case where no remote named `origin` exists at all still produces the same URL, proving `origin` is never consulted. +- [ ] **AC9 [container]** The runner refuses to record a hash it cannot prove was used: when the isolated Claude config directory resolves the `coding` plugin to a path whose `rules/` + `commands/` content hash differs from `--coding-repo`'s, the runner exits non-zero before invoking any review — evidence: exit code non-zero, stderr contains `PLUGIN RESOLUTION MISMATCH` plus both hashes, stub-`claude` counter file has 0 lines. +- [ ] **AC10 [container]** Harvest normalizes a review report into findings: a unit test feeds the checked-in sample report under `bench/testdata/` to the harvester and asserts the exact expected list of `{path, line, rule_id, body}` objects, including one finding that cites a `rule_id` with no `path:line` (kept, with `path` and `line` null — not dropped) — evidence: test exits 0; assertion compares the full normalized list. +- [ ] **AC11 [container]** `--golden` is reserved, not silently ignored: passing `--golden bench/golden.json` exits 2 — evidence: exit code 2, stderr states that scoring is not implemented and names it as future work. +- [ ] **AC12 [container]** The runner carries no personal paths and no third-party dependencies — evidence: `grep -rn '/Users/\|~/Documents/' bench/` returns 0 lines (exit 1), and every `import` / `from` line in `bench/run.py` names a Python 3 standard-library module only (`grep -n '^import \|^from ' bench/run.py` output inspected against the stdlib list). +- [ ] **AC13 [container]** Documentation reflects the shipped runner — evidence: `grep -n 'make bench' bench/README.md` returns ≥1 line; `grep -n 'runner, golden set, and scoring are not built yet' bench/README.md` returns 0 lines; `grep -n -A20 '## Unreleased' CHANGELOG.md` shows a bullet naming the bench runner; `grep -n 'parents\[0\], merge_sha' bench/README.md` returns 0 lines (the old parent-derived squash statement is gone) AND `grep -n 'base_sha\.\.head_sha' bench/README.md` returns ≥1 line (the README now states this spec's rule, not "either/or" — the parent-derived snippet must be fully replaced). +- [ ] **AC14 [operator]** One configuration scores the full five-PR dev fixture: `make bench BENCH_ARGS="--model --effort --mode "` exits 0 and writes a result file containing exactly 5 rows, one per `id` in `bench/prs.json` — evidence: `jq -s 'length' bench/results/results.jsonl` prints 5; `jq -r .pr_id bench/results/results.jsonl | sort` matches the five fixture ids. +- [ ] **AC15 [operator]** Every input is pinned in every row, including mode — evidence: `jq -r 'select(.rules_commands_hash == null or .model == null or .effort == null or .mode == null or .prs_version == null) | .pr_id' bench/results/results.jsonl` prints nothing; `jq -r .rules_commands_hash bench/results/results.jsonl | sort -u | wc -l` prints 1; `jq -r .mode bench/results/results.jsonl | sort -u` prints exactly the one mode invoked; the hash equals the runner's own `--print-config-hash` output for the same `--coding-repo`. +- [ ] **AC16 [operator]** All five fixture entries produce the exact changed-file count recorded in the manifest, not merely a non-zero count — evidence: `jq -r '"\(.pr_id) \(.changed_files)"' bench/results/results.jsonl | sort` equals exactly `github-pr-review-agent#11 17`, `node-skeleton#2 18`, `python-skeleton#3 8`, `quant#109 21`, `tts-mcp#20 1` (each PR's `changed_files` field in `bench/prs.json`, cross-verified in `bench/README.md`). A wrong-but-nonzero range — e.g. `base_branch...head` instead of the correct SHA pair — passes a `>0` check but fails this exact-match check; `node-skeleton#2` (squash) and the other four (merge-commit) together cover both merge strategies. +- [ ] **AC17 [operator]** A genuinely clean PR produces a row with zero findings, distinct from an aborted PR — evidence: the row for `tts-mcp#20` (`role: clean` in the manifest, correct answer is zero findings per its `notes` field) exists in the results file and `jq -r 'select(.pr_id=="tts-mcp#20") | .findings | length' bench/results/results.jsonl` prints `0`; combined with AC3's proof that an empty *diff* aborts with no row at all, this proves "zero findings on a real review" and "no row because the diff was empty" are observably different outcomes, never conflated. +- [ ] **AC18 [operator]** The real second run is cache-served and fast: re-running the identical `make bench` invocation (same `--mode`) exits 0 in under 60 seconds wall time, prints `cache hit` for all five PRs, and leaves the result file byte-identical — evidence: `time` output < 60s; `grep -c 'cache hit' ` prints 5; `md5sum bench/results/results.jsonl` unchanged before/after. +- [ ] **AC19 [operator]** Harvest works against the live command, not just the sample: the row for the known-defects PR carries ≥1 finding with a non-empty `rule_id` — evidence: `jq -r 'select(.pr_id=="github-pr-review-agent#11") | [.findings[] | select(.rule_id != null and .rule_id != "")] | length' bench/results/results.jsonl` prints ≥1. +- [ ] **AC20 [operator]** The runner never mutated the operator's real fixture clones — evidence: for each fixture repo already cloned on the operator's host (`$FIXTURE_CLONE` — an operator-local path to an existing clone of one of the fixture repos, outside `bench/.cache/`), `git -C $FIXTURE_CLONE status --porcelain` is empty and `git -C $FIXTURE_CLONE reflog -1` is byte-identical before and after a full `make bench` run over the five-PR fixture (AC14). Any output from either command, or any reflog change, is a real safety failure — the runner must only ever touch its own `bench/.cache/repos/` copies. + +**Scenario coverage — NO new scenario.** The container-verifiable behaviour is reachable by unit tests over temp git repositories and stub `claude`/`git` executables on `PATH`; the remainder requires a real Claude subprocess and real tokens, which the scenario harness cannot provide either. Adding an E2E scenario would duplicate AC14-AC20 without removing the operator from the loop. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +``` +make precommit +python3 -m unittest discover -s bench -p 'test_*.py' -v +python3 bench/run.py --golden bench/golden.json ; echo "exit=$?" +grep -rn '/Users/\|~/Documents/' bench/ +grep -n 'make bench' bench/README.md +grep -n -A20 '## Unreleased' CHANGELOG.md +``` + +Expected: `make precommit` exits 0; the unittest run reports `OK` with the parent-count, empty-diff, cache, mode-cache-miss, config-hash, git-path-prefix, fork-resolution, plugin-resolution, and harvest tests present; the `--golden` invocation prints `exit=2`; the personal-path grep returns nothing (exit 1); both documentation greps return ≥1 line. + +### Operator-executable (runs on the host, spends real tokens) + +``` +make bench BENCH_ARGS="--model --effort --mode " +jq -s 'length' bench/results/results.jsonl +jq -r .rules_commands_hash bench/results/results.jsonl | sort -u +jq -r .mode bench/results/results.jsonl | sort -u +git -C $FIXTURE_CLONE status --porcelain; git -C $FIXTURE_CLONE reflog -1 +time make bench BENCH_ARGS="--model --effort --mode " +git -C $FIXTURE_CLONE status --porcelain; git -C $FIXTURE_CLONE reflog -1 +``` + +`$FIXTURE_CLONE` is an operator-local path to an existing clone of one of the fixture repos (e.g. `tts-mcp`, `quant`, `node-skeleton`), never a personal path baked into the spec. + +Expected: first run exits 0 after five real reviews and writes five rows sharing one `rules_commands_hash` and one `mode`; the fixture-clone `git status`/`reflog` checks are identical before and after the run; the repeat run exits 0 in under 60 seconds printing `cache hit` five times with the result file unchanged. + +## Desired Behavior + +1. **One invocation = one configuration over one PR set.** A single entrypoint (`python3 bench/run.py`, wrapped by `make bench`) accepts the coding-repo path, model, effort, review mode, PR manifest path, and output directory; it processes the manifest's PRs sequentially and ends with a per-PR status summary line and an exit code that is 0 only when every PR produced a row. `--model`, `--effort`, and `--mode` are mandatory (they are identity, and a guessed default would mislabel rows): `--mode` takes exactly the values `/coding:pr-review` itself accepts (`short`, `full`, `selector`) and is passed through verbatim as that command's second positional argument. The repo path, manifest, and output directory default to the runner's own repository so the documented invocation needs no personal paths. `--golden` is recognized and rejected with exit 2 — scoring belongs to a future spec, and an accepted-but-ignored flag would let an operator believe a run was scored when it was not. +2. **Configuration identity is content, not commits — and mode is part of that identity.** The configuration key is `(content hash of rules/ + commands/, model, effort, mode, manifest version)`. Review mode is not cosmetic: `short`/`full`/`selector` route through materially different code paths in `/coding:pr-review` (selector skips per-owner dispatch and runs in-session adjudication instead; full additionally runs the citation validator) and were measured to differ by roughly 2x wall-time and several points of recall in a prior benchmark run. A cache or result row that did not distinguish mode would silently conflate two different instruments under one key. The content hash covers every regular file under `rules/` + `commands/` in the target repo, in sorted path order, including uncommitted edits — benchmarking an uncommitted rule change before committing it is the primary use case. The coding repo's git SHA is never part of the key, because this benchmark lives inside the repo it measures and unrelated commits move that SHA without changing a rule. +3. **The recorded hash is proven, not asserted.** Before invoking any review, the runner establishes that the review subprocess will load its rules and commands from the target repo — it resolves the `coding` plugin path that the isolated Claude config directory will actually use, hashes that path's `rules/` + `commands/`, and compares it to the hash it is about to record. A mismatch aborts the whole run before the first review, naming both hashes. +4. **The runner resolves PRs itself, entirely inside its own cache — never inside the operator's real clones.** `/coding:pr-review` takes a target branch and a mode, has no `gh` in its allowed tools, and cannot fetch a PR. So the runner clones or fetches into `bench/.cache/repos///` — never into `~/Documents/workspaces/` or any other path the operator uses for real work — fetching `pull//head` from the repository the manifest names via its `owner`/`repo` pair, never from a remote named `origin` (on `tts-mcp` the PR lives on the fork `bborbe/tts-mcp` while `origin` is upstream `florianbuetow/tts-mcp`; `git fetch origin pull/20/head` fails there with "couldn't find remote ref"), and prepares an isolated working copy at the recorded head under that cache path. Every `git` invocation the runner issues — clone, fetch, checkout, worktree, or any cleanup — targets a path under `bench/.cache/repos/`; the runner never runs `git` with a `-C`/cwd argument or a repository path outside that prefix. This is a hard safety invariant, not a convenience: `/coding:pr-review` itself holds `git worktree`, `git fetch`, `git branch`, and `rm -rf` permissions once invoked, so a runner that reused an operator's real clone could destructively mutate it. +5. **Diff range branches on actual parent count.** The runner reads the merge commit's real parent count. Two or more parents means a merge commit: the range is `^1..^2`. Exactly one parent means a squash (or rebase): the range is the manifest's recorded `base_sha..head_sha`, derived from the manifest and never from parent traversal. There is no fallback branch — a "second parent, else the merge commit itself" heuristic silently yields `base == head` on a squash and produces an empty diff with no error. +6. **Empty diffs and failures are loud and isolated.** After resolving the range and before invoking a review, the runner counts changed files. Zero changed files aborts that PR with an error naming the PR id and the resolved range — it is never recorded as a review with zero findings, because two independent code paths can produce that state and both are indistinguishable from a genuinely clean PR. A PR that aborts, times out, or whose review subprocess exits non-zero produces no result row and no cache entry; the remaining PRs still run; the process exits non-zero with that PR listed as failed in the summary. +7. **Reviews run isolated, and their raw output is kept.** Each review is invoked as the slash command in a dedicated Claude configuration directory with the autoupdater disabled, so nothing picks up a plugin update mid-matrix and no interactive session state leaks in. The subprocess's raw stdout is stored verbatim in the per-(PR, configuration) cache before any parsing, so a later change to the harvester can re-normalize old runs without spending tokens again. A cache hit skips the subprocess entirely and appends no duplicate row. +8. **Results are an append-only ledger of normalized findings.** Each completed (PR, configuration) pair appends exactly one row carrying the configuration identity, the PR identity, the resolved diff range and changed-file count, the invoked command string, UTC timestamps, and the findings normalized to `{path, line, rule_id, body}`. Rows are never rewritten or deleted; writes are atomic; a second runner starting while one is in progress exits rather than interleaving into the same ledger. + +## Constraints + +- **Language and dependencies:** Python 3 standard library only, one file at `bench/run.py` plus its tests. This repo is a Claude Code plugin distributed as a clone — it has no packaging, no `go.mod`, no `requirements.txt`, and adding any of those is the wrong shape. `scripts/check-coverage.sh` already wraps a `python3` heredoc for strictly less logic than this (JSON schema handling, content hashing, subprocess orchestration, caching, serialization), which is the standing evidence that bash alone does not carry this workload. Go was rejected: zero Go exists in this repo today and a compiled artifact cannot ship in a clone-installed plugin. +- **Make target:** a thin `bench` target wrapping `python3 bench/run.py`, consistent with the existing `check-*` targets that wrap `scripts/*.sh` and `scripts/*.py`. The bench unit tests are wired into `precommit` so they gate every change; they must not require network, a real `claude` binary, or GitHub access. +- `bench/prs.json` is a frozen input — its schema, its five entries, and its `dev-1` version are not modified by this work. +- **This spec is the binding contract for diff-range mechanics, superseding `bench/README.md` where the two disagree.** `bench/README.md`'s squash snippet (`git diff ^1..`, i.e. derived from parent traversal) coincides with this spec's rule (Desired Behavior 5: always the manifest's recorded `base_sha..head_sha` for a single-parent commit, never parent-derived) only because `node-skeleton#2` happens to have `head_sha == merge_sha`. The README shipped before this spec existed; this spec's rule is authoritative, and prompt 4 (packaging/docs) corrects the README snippet to match rather than the reverse. +- **Result row required fields** (persistent artifact — additional fields are permitted, these are not optional): `config_hash`, `rules_commands_hash`, `model`, `effort`, `mode`, `prs_version`, `pr_id`, `base_sha`, `head_sha`, `diff_range`, `changed_files`, `review_command`, `started_at` (UTC ISO-8601), `duration_seconds`, `findings`, `raw_output_ref`, `runner_version`. +- **Fixed invariants, not configurable:** the per-PR review timeout is 45 minutes; the cache lives under `bench/.cache/` and results under `bench/results/` (both already listed in `.gitignore`, so no benchmark output is ever committed); the isolated Claude configuration directory is `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`. +- **Repo conventions that must not regress** (`docs/dod.md`): no personal paths anywhere in shipped files, a `## Unreleased` CHANGELOG entry, and `make precommit` green including `check-links`, `check-json`, `check-index`, `check-coverage`, and `check-acceptance`. +- No rule, agent, command, or doc that participates in a review is edited by this work — the measured configuration must stay exactly as it is while the measuring device is built. + +## Assumptions + +- A `claude` executable is on `PATH` and accepts a one-shot prompt plus model, effort, and mode selection (mode as `/coding:pr-review`'s second positional argument); the runner passes them through and records the exact command string it ran, so a CLI-surface change surfaces as a failed run rather than a silently mislabeled row. +- `$HOME/.claude-verify` is an operator-prepared Claude configuration directory in which the `coding` plugin resolves to the repository under `--coding-repo`. Desired Behavior 3 turns a violated assumption into a loud abort rather than a wrong measurement. +- The operator's git credentials can read all five fixture repositories (one, `quant`, is private). +- The five fixture PRs stay merged and their recorded SHAs stay reachable. A deleted or force-pushed history surfaces as a fetch failure for that PR, not as a clean review. +- Findings emitted by `/coding:pr-review` cite a `rule_id` from `rules/index.json` — the citation validator already enforces this, which is what makes mechanical harvesting possible. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | Detection | Concurrency | +|---|---|---|---|---| +| Fixture repository unreachable (network down, private repo without credentials, deleted PR ref) | That PR fails before any review is invoked; no row, no cache entry; remaining PRs still run; process exits non-zero | Operator fixes credentials/network and re-runs the same invocation — the uncached PR is retried, cached PRs are skipped | Summary lists the PR as `failed: fetch`; stderr carries the underlying git error | Cache entries for other PRs already written are untouched | +| Manifest missing a required field, or an unreadable/invalid JSON manifest | Abort before the first review, naming the offending entry id and field | Operator fixes the manifest and re-runs | Non-zero exit with the field name in stderr; results file unchanged | No partial run started, so no interleaving | +| Merge SHA has one parent but the manifest claims `merge-commit` (or the reverse) | Parent count wins; the recorded strategy label is not trusted for range selection, and the mismatch is reported for that PR | Operator corrects `merge_strategy` in the manifest (cosmetic — the run already used the correct range) | Summary line notes `strategy mismatch` with both values | None — read-only inspection | +| Resolved diff range yields zero changed files | That PR aborts loudly; never recorded as a zero-finding review | Operator re-verifies the SHAs with `gh api repos///compare/...` per `bench/README.md` | Non-zero exit; stderr contains `EMPTY DIFF` plus the PR id and the resolved range | No row, no cache entry — nothing to reconcile | +| Isolated config directory resolves the `coding` plugin somewhere other than `--coding-repo` | Whole run aborts before the first review — a hash claiming content that did not run is worse than no measurement | Operator repoints the isolated config directory and re-runs | Non-zero exit; stderr contains `PLUGIN RESOLUTION MISMATCH` and both hashes | Nothing written | +| Review subprocess exceeds 45 minutes, or the model API rate-limits / errors out | Subprocess is terminated; the PR is marked failed; no row, no cache entry; remaining PRs continue; process exits non-zero | Re-run the same invocation later — completed PRs are cache-served, the failed one is retried | Summary lists `failed: timeout` or `failed: exit `; raw stderr preserved under the cache's failure log | Partial cache from earlier PRs is valid and reused | +| Crash or interrupt mid-run (PR 4 of 5) | Rows and cache entries for completed PRs survive; the in-flight PR leaves no partial row (rows are written atomically, only after the review completes) | Re-run the same invocation; PRs 1-3 are cache-served | Ledger contains fewer rows than the manifest; summary of the re-run shows which PRs were re-executed | Atomic write-then-rename means no truncated row is ever observed | +| Two runners started against the same output directory | The second exits immediately without touching the ledger or cache | Operator waits for the first to finish, then re-runs | Non-zero exit; stderr states another bench run is in progress | Single-instance lock is the mechanism; a stale lock from a killed process is removable by deleting the lock file, which the error message names | +| Disk exhausted by cached repository clones or raw outputs | The failing operation surfaces the OS error for that PR; the PR is marked failed; no truncated row is appended | Operator deletes `bench/.cache/repos/` (rebuildable from the manifest) and re-runs | Summary lists the PR as failed with the OS error; `df` confirms | Existing rows remain valid — the ledger is append-only | +| Host clock skew or timezone change between runs | No effect on correctness: cache identity is content-derived, never mtime- or timestamp-derived; timestamps are recorded in UTC | None needed | Row timestamps carry an explicit UTC offset | Two runs at the "same" wall-clock time still key on distinct (PR, configuration) pairs | + +## Security / Abuse Cases + +- **Attacker-controlled surface:** the PR manifest (`owner`, `repo`, `number`, SHAs) and the third-party repository content that gets checked out. The manifest is repo-controlled today, but it is data that flows into `git` invocations and into filesystem paths, so it is treated as untrusted input. +- **Command injection:** every subprocess is invoked with an argument list, never a shell string; no manifest value is ever interpolated into a shell command. +- **Path traversal:** cache and result paths are derived from manifest ids; `owner`, `repo`, and `number` are validated against a strict character set (GitHub-legal name characters and digits) and rejected otherwise, so no id can escape `bench/.cache/` or `bench/results/`. +- **Trust boundary — executing third-party code:** the runner checks out third-party repositories and points a review at them. It never builds, installs, or runs anything from those checkouts; the review is a read-and-judge operation over a diff. +- **Hanging forever:** the review subprocess has a hard 45-minute timeout, so a wedged model call cannot stall a matrix indefinitely. +- **Secret leakage:** result rows and cached raw output are written to gitignored directories, and the runner records the command string it invoked without copying environment variables, tokens, or credential material into any artifact. + +## Suggested Decomposition + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Manifest loading + validation, content hashing of `rules/` + `commands/`, plugin-resolution preflight, CLI surface incl. mandatory `--mode` and reserved `--golden`, exit-code contract | 1, 2, 3 | AC6, AC9, AC11 | — | +| 2 | PR resolution (owning-remote fetch, isolated working copies confined to `bench/.cache/repos/`), parent-count diff-range branching, empty-diff abort, per-PR failure isolation | 4, 5, 6 | AC2, AC3, AC7, AC8 | prompt 1 | +| 3 | Review invocation in the isolated config dir, mode-aware cache key, raw-output cache, harvest to normalized findings, append-only ledger with atomic writes and single-instance lock | 7, 8 | AC4, AC5, AC10 | prompts 1-2 | +| 4 | `make bench` + `make bench-test` targets, precommit wiring, `bench/README.md` rewrite (including the squash-snippet correction per Constraints), CHANGELOG entry, personal-path and stdlib-only sweep | — | AC1, AC12, AC13 | prompts 1-3 | + +Rationale: prompt 1 fixes configuration identity — now including mode — which every later row depends on and which nothing else can be built against until it exists. Prompt 2 owns the two hard requirements (parent-count branching, empty-diff abort) plus the two safety/correctness ACs discovered in review (git invocations confined to the runner's own cache, fetch derived from the manifest's owning remote) as one unit, because they share the same PR-resolution code path. Prompt 3 is the only prompt that touches the review subprocess, so the stub-`claude` test harness — and the mode-cache-miss behavior it proves — is introduced exactly once. Prompt 4 is packaging and docs, deliberately last so the gate wired into `precommit` sees the finished tests. AC14-AC20 are operator-executable and are verified after merge in the spec-verification phase, not by any prompt. + +## Do-Nothing Option + +Doing nothing keeps the status quo: every rule edit, model swap, and effort change ships on vibes, and the 84 judgment-tier rules remain permanently unfalsifiable. The immediate cost is not hypothetical — the selector-mode redesign is already mid-migration with a default flip pending, and there is no instrument to prove the new path finds what the old one found beyond one hand-checked fixture PR. The alternatives considered and rejected: (a) keep hand-checking one PR per change — cheap per change, but it measures a single point and cannot detect a regression class; (b) ship the runner in bash — rejected because the workload is JSON schema handling, hashing, subprocess orchestration and caching, and the repo's existing `check-coverage.sh` already had to escape into a `python3` heredoc for a fraction of that; (c) ship it in Go — rejected because a clone-installed plugin repo with zero Go today would gain a toolchain and a compiled artifact users cannot run. Everything downstream (golden set, scoring, noise floor, model comparison) is blocked on this instrument existing, so the do-nothing option is not "wait" — it is "never measure". From 54f358e9d391c93fbd7fa7261b641d1a0d191a42 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 7 Aug 2026 00:08:44 +0200 Subject: [PATCH 2/5] 012-pr-review-bench-runner-pr-resolution --- CHANGELOG.md | 3 + bench/run.py | 263 ++++++++- bench/test_resolve.py | 497 ++++++++++++++++++ bench/testsupport.py | 157 +++++- ...12-pr-review-bench-runner-pr-resolution.md | 7 +- 5 files changed, 919 insertions(+), 8 deletions(-) create mode 100644 bench/test_resolve.py rename prompts/{in-progress => completed}/012-pr-review-bench-runner-pr-resolution.md (97%) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbe2507..921f1df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ Please choose versions by [Semantic Versioning](http://semver.org/). - bench: add `bench/run.py` — benchmark runner entrypoint with configuration-identity core (content hashing of `rules/`+`commands/`, manifest loading/validation, plugin-resolution preflight, CLI surface with mandatory `--model`/`--effort`/`--mode`, reserved `--golden` rejection, `--print-config-hash` helper) - bench: add `bench/testsupport.py` — shared test helpers (`make_coding_repo`, `make_verify_config_dir`, `stub_claude`, `with_path`) - bench: add `bench/test_config.py` — 17 unit tests covering AC6, AC9, AC11 and related acceptance criteria +- bench: extend `bench/run.py` with PR resolution (fetch from manifest URL into `bench/.cache/repos/`, isolated working copies via `git worktree`), parent-count diff-range branching (`^1..^2` for merge commits, manifest `base_sha..head_sha` for single-parent commits), empty-diff abort (`EMPTY DIFF` loud failure), per-PR failure isolation, and strategy-label mismatch reporting +- bench: add `bench/test_resolve.py` — 12 unit tests covering AC2, AC3, AC7, AC8 and related acceptance criteria (`parent_count`, `empty_diff`, `git_invocation_confined_to_cache_repos`, `fetch_url`) +- bench: extend `bench/testsupport.py` with git-repo helpers (`init_git_repo`, `commit_file`, `make_merge_repo`, `make_squash_repo`, `make_empty_diff_repo`, `stub_git`, `make_manifest`) ## v0.35.0 diff --git a/bench/run.py b/bench/run.py index 4bfaec3..a50f4ea 100755 --- a/bench/run.py +++ b/bench/run.py @@ -17,11 +17,14 @@ # Python 3 standard library only — no third-party dependencies. import argparse +import dataclasses import hashlib import json import os import pathlib import re +import shutil +import subprocess import sys # ---------------------------------------------------------------------- @@ -185,6 +188,249 @@ def safe_pr_key(pr_id: str) -> str: return pr_id.replace("#", "_") +# ---------------------------------------------------------------------- +# Path helpers +# ---------------------------------------------------------------------- +def repos_root(cache_root: pathlib.Path) -> pathlib.Path: + return cache_root / "repos" + + +def repo_cache_dir(cache_root: pathlib.Path, owner: str, repo: str) -> pathlib.Path: + return repos_root(cache_root) / owner / repo + + +def worktree_dir(cache_root: pathlib.Path, owner: str, repo: str, number: int) -> pathlib.Path: + return repos_root(cache_root) / owner / f"{repo}__pr{number}" + + +def assert_under(path: pathlib.Path, root: pathlib.Path) -> pathlib.Path: + """Resolve both paths and verify path is strictly under root. + + Raises BenchError if resolved path equals root or is not a sub-path. + The message names both path and root and states the runner only touches its own cache. + """ + resolved = path.resolve() + root_resolved = root.resolve() + if resolved == root_resolved or not resolved.is_relative_to(root_resolved): + raise BenchError( + f"path {path!r} is not under root {root!r}; " + f"the runner only ever touches its own cache at {root!r}" + ) + return resolved + + +# ---------------------------------------------------------------------- +# Git helpers +# ---------------------------------------------------------------------- +def fetch_url(owner: str, repo: str) -> str: + """Build GitHub fetch URL from manifest owner/repo pair. + + The runner never reads or depends on a remote named 'origin'. + """ + return f"https://github.com/{owner}/{repo}" + + +def git(args, *, repo_dir: pathlib.Path, cache_root: pathlib.Path, + check: bool = True, timeout: int = 600) -> subprocess.CompletedProcess: + """Single git subprocess chokepoint — every git invocation goes through here. + + - Always uses -C ; never cwd= or shell strings. + - repo_dir must already exist. + - assert_under runs before subprocess to catch escaped manifest values. + - subprocess.TimeoutExpired propagates; callers convert it to per-PR failures. + """ + target = assert_under(repo_dir, repos_root(cache_root)) + cmd = ["git", "-C", str(target), *args] + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=timeout) + if check and proc.returncode != 0: + raise BenchError( + f"git {' '.join(args)} failed in {target} (exit {proc.returncode}): " + f"{proc.stderr.strip()}" + ) + return proc + + +def ensure_refs(cache_root: pathlib.Path, entry: dict) -> pathlib.Path: + """Prepare repo cache dir so entry's three SHAs are locally reachable. + + Returns the repo directory path. + """ + repo_dir = repo_cache_dir(cache_root, entry["owner"], entry["repo"]) + repo_dir.mkdir(parents=True, exist_ok=True) + + # Init if not a git repo + if not (repo_dir / ".git").exists(): + git(["init", "--quiet"], repo_dir=repo_dir, cache_root=cache_root) + + # Offline short-circuit: check if all three SHAs resolve locally + for sha_field in ("merge_sha", "base_sha", "head_sha"): + sha = entry[sha_field] + proc = git(["cat-file", "-e", f"{sha}^{{commit}}"], + repo_dir=repo_dir, cache_root=cache_root, check=False) + if proc.returncode != 0: + break + else: + # All three SHAs resolved — skip fetch + return repo_dir + + # Fetch from manifest URL (never 'origin') + url = fetch_url(entry["owner"], entry["repo"]) + git([ + "fetch", "--no-tags", "--force", url, + f"+pull/{entry['number']}/head:refs/bench/pr{entry['number']}/head", + "+refs/heads/*:refs/remotes/origin/*", + ], repo_dir=repo_dir, cache_root=cache_root) + + return repo_dir + + +def resolve_diff_range(cache_root: pathlib.Path, repo_dir: pathlib.Path, + entry: dict) -> tuple[str, str, str, int, list]: + """Resolve the correct diff range by inspecting the merge commit's parent count. + + Returns (diff_range, base_endpoint, head_endpoint, parent_count, notes). + """ + out = git( + ["rev-list", "--parents", "-n", "1", entry["merge_sha"]], + repo_dir=repo_dir, cache_root=cache_root, + ).stdout.split() + + if not out: + raise BenchError( + f"{entry['id']}: cannot resolve merge commit {entry['merge_sha']}" + ) + + n_parents = len(out) - 1 + notes: list = [] + + if n_parents >= 2: + base = f"{entry['merge_sha']}^1" + head = f"{entry['merge_sha']}^2" + elif n_parents == 1: + base = entry["base_sha"] + head = entry["head_sha"] + else: + raise BenchError( + f"{entry['id']}: merge commit {entry['merge_sha']} has no parents; " + f"cannot reconstruct a diff range" + ) + + # Strategy-label mismatch: report but use correct range + label = entry.get("merge_strategy", "") + if label == "merge-commit" and n_parents == 1: + notes.append(f"strategy mismatch (manifest={label}, parents={n_parents})") + elif label == "squash" and n_parents >= 2: + notes.append(f"strategy mismatch (manifest={label}, parents={n_parents})") + + return f"{base}..{head}", base, head, n_parents, notes + + +def changed_files(cache_root: pathlib.Path, repo_dir: pathlib.Path, + diff_range: str) -> list[str]: + """Return sorted list of files changed in the diff range.""" + proc = git(["diff", "--name-only", diff_range], + repo_dir=repo_dir, cache_root=cache_root) + return [ln for ln in proc.stdout.splitlines() if ln.strip()] + + +@dataclasses.dataclass +class PrCheckout: + pr_id: str + repo_dir: pathlib.Path + worktree: pathlib.Path + base_branch: str + head_branch: str + diff_range: str + base_sha: str + head_sha: str + changed_files: int + parent_count: int + notes: list + + +def prepare_worktree(cache_root: pathlib.Path, repo_dir: pathlib.Path, + entry: dict, base_endpoint: str, + head_endpoint: str) -> PrCheckout: + """Resolve endpoints to SHAs, publish remote-tracking refs, create working copy. + + Returns PrCheckout with all resolved fields. + """ + wt = worktree_dir(cache_root, entry["owner"], entry["repo"], entry["number"]) + base_branch = f"bench-base-{entry['number']}" + head_branch = f"bench-pr-{entry['number']}" + + base_sha = git( + ["rev-parse", f"{base_endpoint}^{{commit}}"], + repo_dir=repo_dir, cache_root=cache_root, + ).stdout.strip() + head_sha = git( + ["rev-parse", f"{head_endpoint}^{{commit}}"], + repo_dir=repo_dir, cache_root=cache_root, + ).stdout.strip() + + # Publish remote-tracking refs so /coding:pr-review can resolve origin/ + git(["update-ref", f"refs/remotes/origin/{base_branch}", base_sha], + repo_dir=repo_dir, cache_root=cache_root) + git(["update-ref", f"refs/remotes/origin/{head_branch}", head_sha], + repo_dir=repo_dir, cache_root=cache_root) + + # Tear down any stale copy from a previous run + git(["worktree", "remove", "--force", str(wt)], + repo_dir=repo_dir, cache_root=cache_root, check=False) + if wt.exists(): + assert_under(wt, repos_root(cache_root)) + shutil.rmtree(wt, ignore_errors=True) + git(["branch", "-D", head_branch], + repo_dir=repo_dir, cache_root=cache_root, check=False) + git(["worktree", "prune"], repo_dir=repo_dir, cache_root=cache_root, check=False) + + # Validate worktree path before creating + assert_under(wt, repos_root(cache_root)) + git(["worktree", "add", "--force", "-b", head_branch, str(wt), head_sha], + repo_dir=repo_dir, cache_root=cache_root) + + return PrCheckout( + pr_id=entry["id"], + repo_dir=repo_dir, + worktree=wt, + base_branch=base_branch, + head_branch=head_branch, + diff_range="", # filled by caller + base_sha=base_sha, + head_sha=head_sha, + changed_files=0, # filled by caller + parent_count=0, # filled by caller + notes=[], # filled by caller + ) + + +def resolve_pr(cache_root: pathlib.Path, entry: dict) -> PrCheckout: + """Tie together ensure_refs → resolve_diff_range → changed_files → empty-diff gate → prepare_worktree. + + Raises BenchError (never returns) if diff range is empty. + """ + repo_dir = ensure_refs(cache_root, entry) + diff_range, base_endpoint, head_endpoint, n_parents, notes = resolve_diff_range( + cache_root, repo_dir, entry + ) + + files = changed_files(cache_root, repo_dir, diff_range) + if not files: + raise BenchError( + f"EMPTY DIFF: {entry['id']} resolved range {diff_range} contains zero changed files. " + f"This is never recorded as a zero-finding review — two independent code paths produce " + f"this state and both look identical to a genuinely clean PR. " + f"Re-verify the SHAs with: gh api repos/{entry['owner']}/{entry['repo']}/compare/{entry['base_sha']}...{entry['head_sha']} --jq '.files | length'" + ) + + checkout = prepare_worktree(cache_root, repo_dir, entry, base_endpoint, head_endpoint) + checkout.diff_range = diff_range + checkout.changed_files = len(files) + checkout.parent_count = n_parents + checkout.notes = notes + return checkout + + # ---------------------------------------------------------------------- # Plugin resolution preflight # ---------------------------------------------------------------------- @@ -353,6 +599,10 @@ def run_bench(*, coding_repo: pathlib.Path, manifest_path: pathlib.Path, rc_hash=rc_hash, prs_version=manifest["version"], ) + except subprocess.TimeoutExpired as err: + outcome, detail = "failed", "timeout" + except OSError as err: + outcome, detail = "failed", str(err) except BenchError as err: outcome, detail = "failed", str(err) outcomes.append((pr_id, f"{outcome}: {detail}")) @@ -373,13 +623,16 @@ def process_pr(*, entry: dict, coding_repo: pathlib.Path, model: str, effort: str, mode: str, config_dir: pathlib.Path, cfg_hash: str, rc_hash: str, prs_version: str) -> tuple[str, str]: - """Process a single PR — stub for prompt 2 of spec 002. + """Process a single PR — resolution complete, review invocation stubbed. - PR resolution (prompt 2) and review invocation (prompt 3) are not yet - implemented. This stub loudly fails so the gap cannot be mistaken for - success. + PR resolution (this prompt) reconstructs the diff range and prepares the + working copy inside the runner's cache. Review invocation ships in prompt 3. """ - return ("failed", "pr resolution not yet implemented (prompt 2 of spec 002)") + checkout = resolve_pr(cache_root, entry) + notes_suffix = "" + if checkout.notes: + notes_suffix = "; " + "; ".join(checkout.notes) + return ("failed", f"review invocation not yet implemented (prompt 3 of spec 002){notes_suffix}") # ---------------------------------------------------------------------- diff --git a/bench/test_resolve.py b/bench/test_resolve.py new file mode 100644 index 0000000..1cd56d7 --- /dev/null +++ b/bench/test_resolve.py @@ -0,0 +1,497 @@ +#!/usr/bin/env python3 +"""Unit tests for bench/run.py PR resolution — AC2, AC3, AC7, AC8 and related.""" + +import json +import os +import pathlib +import subprocess +import sys +import tempfile +import unittest + +import run +import testsupport + + +class TestDiffRangeBranchesOnParentCount(unittest.TestCase): + """AC2: diff range branches on actual parent count.""" + + def test_parent_count_drives_merge_range(self): + """Two-parent merge → ^1..^2; single-parent squash → manifest base..head.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + repos_root = cache_root / "repos" + + # Create merge repo in cache + cache_merge = repos_root / "testowner" / "mergerepo" + cache_merge.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_merge) + merge_info = testsupport.make_merge_repo(cache_merge) + + # Create squash repo in cache + cache_squash = repos_root / "testowner" / "squashrepo" + cache_squash.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_squash) + squash_info = testsupport.make_squash_repo(cache_squash) + + merge_entry = { + "id": "test#1", + "owner": "testowner", + "repo": "mergerepo", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": merge_info["merge_sha"], + "base_sha": merge_info["base_sha"], + "head_sha": merge_info["head_sha"], + "changed_files": 1, + } + squash_entry = { + "id": "test#2", + "owner": "testowner", + "repo": "squashrepo", + "number": 2, + "merge_strategy": "squash", + "merge_sha": squash_info["merge_sha"], + "base_sha": squash_info["base_sha"], + "head_sha": squash_info["head_sha"], + "changed_files": 1, + } + + # Merge: 2 parents → ^1..^2 + mr, _, _, np, _ = run.resolve_diff_range(cache_root, cache_merge, merge_entry) + self.assertEqual(np, 2) + expected_mr = f"{merge_entry['merge_sha']}^1..{merge_entry['merge_sha']}^2" + self.assertEqual(mr, expected_mr, + msg=f"merge_range={mr!r} expected {expected_mr!r}") + + # Squash: 1 parent → manifest base..head + sr, _, _, np_sq, _ = run.resolve_diff_range(cache_root, cache_squash, squash_entry) + self.assertEqual(np_sq, 1) + expected_sr = f"{squash_entry['base_sha']}..{squash_entry['head_sha']}" + self.assertEqual(sr, expected_sr, + msg=f"squash_range={sr!r} expected {expected_sr!r}") + + # Both ranges have ≥1 changed file + m_files = run.changed_files(cache_root, cache_merge, mr) + s_files = run.changed_files(cache_root, cache_squash, sr) + self.assertGreaterEqual(len(m_files), 1) + self.assertGreaterEqual(len(s_files), 1) + + +class TestSquashRangeIsManifestDerived(unittest.TestCase): + """AC2 variant: single-parent commits always use manifest base..head.""" + + def test_single_parent_uses_manifest_base_not_parent_derived(self): + """Manifest base_sha older than merge commit's parent → still uses manifest.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + repos_root = cache_root / "repos" + + cache_repo = repos_root / "test" / "repo" + cache_repo.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_repo) + oldest = testsupport.commit_file(cache_repo, "oldest.txt", "oldest\n", "oldest") + middle = testsupport.commit_file(cache_repo, "middle.txt", "middle\n", "middle") + newest = testsupport.commit_file(cache_repo, "newest.txt", "newest\n", "newest") + + entry = { + "id": "test#1", + "owner": "test", + "repo": "repo", + "number": 1, + "merge_strategy": "squash", + "merge_sha": newest, + "base_sha": oldest, + "head_sha": newest, + "changed_files": 2, + } + + dr, _, _, np, _ = run.resolve_diff_range(cache_root, cache_repo, entry) + self.assertEqual(np, 1) + self.assertEqual(dr, f"{oldest}..{newest}", + "must use manifest base_sha, not parent traversal") + + +class TestStrategyLabelMismatch(unittest.TestCase): + """Strategy label is reported but never obeyed for range selection.""" + + def test_mismatch_is_noted_not_obeyed(self): + """Two-parent repo with squash label → ^1..^2 range + note.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + repos_root = cache_root / "repos" + + cache_repo = repos_root / "test" / "repo" + cache_repo.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_repo) + merge_info = testsupport.make_merge_repo(cache_repo) + + entry = { + "id": "test#1", + "owner": "test", + "repo": "repo", + "number": 1, + "merge_strategy": "squash", # wrong label + "merge_sha": merge_info["merge_sha"], + "base_sha": merge_info["base_sha"], + "head_sha": merge_info["head_sha"], + "changed_files": 1, + } + + dr, _, _, np, notes = run.resolve_diff_range(cache_root, cache_repo, entry) + expected = f"{entry['merge_sha']}^1..{entry['merge_sha']}^2" + self.assertEqual(dr, expected) + self.assertEqual(np, 2) + self.assertTrue( + any("strategy mismatch" in n for n in notes), + f"notes must contain 'strategy mismatch', got {notes!r}" + ) + + +class TestEmptyDiffAbortsLoudly(unittest.TestCase): + """AC3: empty diff aborts before review, no row, no cache entry.""" + + def test_empty_diff_raises_empty_diff_error(self): + """With base_sha == head_sha (empty diff), resolve_pr raises BenchError + with 'EMPTY DIFF' message before any worktree or review is created.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + + # Build empty-diff repo in cache + cache_repo = cache_root / "repos" / "testowner" / "emptyrepo" + cache_repo.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_repo) + empty_info = testsupport.make_empty_diff_repo(cache_repo) + + entry = { + "id": "empty#99", + "owner": "testowner", + "repo": "emptyrepo", + "number": 99, + "merge_strategy": "squash", + "merge_sha": empty_info["merge_sha"], + "base_sha": empty_info["base_sha"], + "head_sha": empty_info["head_sha"], + "changed_files": 0, + } + + # resolve_pr should raise EMPTY DIFF before creating worktree + with self.assertRaises(run.BenchError) as ctx: + run.resolve_pr(cache_root, entry) + + msg = str(ctx.exception) + self.assertIn("EMPTY DIFF", msg) + self.assertIn("empty#99", msg) + self.assertIn("..", msg) # range in message + + +class TestFetchUrlIsBuiltFromManifest(unittest.TestCase): + """AC8: fetch URL is built from manifest owner/repo, never from 'origin'.""" + + def test_fetch_url_exact_format(self): + url = run.fetch_url("bborbe", "tts-mcp") + self.assertEqual(url, "https://github.com/bborbe/tts-mcp") + + def test_fetch_url_not_from_origin(self): + """URL is always from manifest, independent of remotes.""" + url = run.fetch_url("bborbe", "tts-mcp") + self.assertEqual(url, "https://github.com/bborbe/tts-mcp") + self.assertNotIn("florianbuetow", url) + + +class TestFetchUrlWithNoOriginRemote(unittest.TestCase): + """AC8 second case: no remote named 'origin' at all.""" + + def test_fetch_url_same_when_no_origin(self): + url = run.fetch_url("bborbe", "tts-mcp") + self.assertEqual(url, "https://github.com/bborbe/tts-mcp") + + +class TestEveryGitStaysUnderCacheRepos(unittest.TestCase): + """AC7: every git invocation stays under bench/.cache/repos/.""" + + def test_git_invocation_confined_to_cache_repos(self): + """With stub_git on PATH via os.environ, every logged path is under cache repos/.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + + git_log = td / "git_log" + bin_dir = td / "bin" + testsupport.stub_git(bin_dir, git_log) + + # Pre-seed cache repos with valid git repos so offline short-circuit works + repos_root = cache_root / "repos" + sha_a = None + sha_b = None + for owner, repo in [("testowner", "repo_a"), ("testowner", "repo_b")]: + repo_path = repos_root / owner / repo + repo_path.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(repo_path) + sha = testsupport.commit_file(repo_path, "f.txt", "c\n", "f") + if repo == "repo_a": + sha_a = sha + else: + sha_b = sha + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": sha_a, + "base_sha": sha_a, + "head_sha": sha_a, + "changed_files": 1, + }, + { + "id": "test#2", + "owner": "testowner", + "repo": "repo_b", + "number": 2, + "merge_strategy": "merge-commit", + "merge_sha": sha_b, + "base_sha": sha_b, + "head_sha": sha_b, + "changed_files": 1, + }, + ] + + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + # Save and modify os.environ to put stub_git on PATH + old_path = os.environ.get("PATH", "") + old_home = os.environ.get("HOME", "") + try: + os.environ["PATH"] = f"{bin_dir}{os.pathsep}{old_path}" + os.environ["HOME"] = str(td) + + try: + run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + except Exception: + pass # may fail due to stub git, we only care about logged paths + finally: + os.environ["PATH"] = old_path + os.environ["HOME"] = old_home + + log_content = "" + if git_log.exists(): + log_content = git_log.read_text() + log_lines = [ln for ln in log_content.splitlines() if ln.strip()] + else: + log_lines = [] + + self.assertGreaterEqual(len(log_lines), 2, + f"expected ≥2 git invocations, got {len(log_lines)}. " + f"git_log exists={git_log.exists()}, content={log_content!r}") + + repos_prefix = str((cache_root / "repos").resolve()) + for line in log_lines: + if " -C " in line: + parts = line.split(" -C ", 1) + if len(parts) > 1: + path = parts[1].split()[0] + path_resolved = str(pathlib.Path(path).resolve()) + self.assertTrue( + path_resolved.startswith(repos_prefix), + f"git invocation targets {path!r} ({path_resolved!r}) " + f"not under {repos_prefix!r}: line={line!r}" + ) + + +class TestAssertUnderRejectsOutsidePath(unittest.TestCase): + """assert_under enforces strict prefix containment.""" + + def test_assert_under_rejects_absolute_outside(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + cache_root.mkdir(parents=True) + repos = run.repos_root(cache_root) + + with self.assertRaises(run.BenchError) as ctx: + run.assert_under(pathlib.Path("/tmp"), repos) + msg = str(ctx.exception) + self.assertIn("/tmp", msg) + self.assertIn(str(repos), msg) + + def test_assert_under_rejects_root_itself(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + cache_root.mkdir(parents=True) + repos = run.repos_root(cache_root) + + with self.assertRaises(run.BenchError) as ctx: + run.assert_under(repos, repos) + self.assertIn(str(repos), str(ctx.exception)) + + +class TestFailingPrDoesNotAbortRemainingPrs(unittest.TestCase): + """A failing PR leaves other PRs to run; exit code is 1.""" + + def test_second_pr_runs_after_first_fails(self): + """First PR has unresolvable SHA; second resolves cleanly. Both in output.""" + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + + # Good repo + cache_good = cache_root / "repos" / "good" / "goodrepo" + cache_good.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_good) + good_info = testsupport.make_merge_repo(cache_good) + + manifest_entries = [ + { + "id": "bad#1", + "owner": "bad", + "repo": "badrepo", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": "a" * 40, + "base_sha": "b" * 40, + "head_sha": "c" * 40, + "changed_files": 1, + }, + { + "id": "good#2", + "owner": "good", + "repo": "goodrepo", + "number": 2, + "merge_strategy": "merge-commit", + "merge_sha": good_info["merge_sha"], + "base_sha": good_info["base_sha"], + "head_sha": good_info["head_sha"], + "changed_files": 1, + }, + ] + + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + # Set up plugin/config properly + cfg_dir = td / ".claude-verify" + cfg_dir.mkdir(parents=True) + plugin_dest = td / "repo" + testsupport.make_coding_repo(plugin_dest) + (cfg_dir / "plugins").mkdir(parents=True) + import json + km = { + "coding": { + "source": {"source": "github", "repo": "bborbe/coding"}, + "installLocation": str(plugin_dest), + } + } + (cfg_dir / "plugins" / "known_marketplaces.json").write_text( + json.dumps(km), encoding="utf-8" + ) + + old_home = os.environ.get("HOME", "") + try: + os.environ["HOME"] = str(td) + result = subprocess.run( + [sys.executable, str(run.BENCH_DIR / "run.py"), + "--coding-repo", str(plugin_dest), + "--manifest", str(manifest_path), + "--out-dir", str(results_dir), + "--model", "test-model", + "--effort", "high", + "--mode", "short"], + capture_output=True, text=True, + ) + finally: + os.environ["HOME"] = old_home + + self.assertEqual(result.returncode, 1, + f"expected exit 1, got {result.returncode}: {result.stderr}") + self.assertIn("bad#1", result.stdout) + self.assertIn("good#2", result.stdout) + self.assertIn("failed", result.stdout) + + +class TestWorktreeCreatedUnderReposRoot(unittest.TestCase): + """After resolve_pr, worktree is under repos/, HEAD at head_sha, refs set.""" + + def test_worktree_under_repos_root_head_at_sha(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + repos_root = cache_root / "repos" + + cache_repo = repos_root / "testowner" / "testrepo" + cache_repo.mkdir(parents=True, exist_ok=True) + testsupport.init_git_repo(cache_repo) + merge_info = testsupport.make_merge_repo(cache_repo) + + entry = { + "id": "test#42", + "owner": "testowner", + "repo": "testrepo", + "number": 42, + "merge_strategy": "merge-commit", + "merge_sha": merge_info["merge_sha"], + "base_sha": merge_info["base_sha"], + "head_sha": merge_info["head_sha"], + "changed_files": 1, + } + + checkout = run.resolve_pr(cache_root, entry) + + # Worktree exists under repos/ + self.assertTrue(checkout.worktree.is_dir(), + f"worktree {checkout.worktree} must exist") + self.assertTrue( + str(checkout.worktree.resolve()).startswith(str(repos_root.resolve())), + f"worktree {checkout.worktree} must be under {repos_root}" + ) + + # HEAD at correct SHA + head_result = subprocess.run( + ["git", "-C", str(checkout.worktree), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ) + self.assertEqual(head_result.stdout.strip(), checkout.head_sha) + + # Branch name is bench-pr- + branch_result = subprocess.run( + ["git", "-C", str(checkout.worktree), "rev-parse", "--abbrev-ref", "HEAD"], + capture_output=True, text=True, check=True, + ) + self.assertEqual(branch_result.stdout.strip(), f"bench-pr-{entry['number']}") + + # refs/remotes/origin/bench-base- at base_sha + base_ref_result = subprocess.run( + ["git", "-C", str(checkout.repo_dir), "rev-parse", + f"refs/remotes/origin/bench-base-{entry['number']}"], + capture_output=True, text=True, check=True, + ) + self.assertEqual(base_ref_result.stdout.strip(), checkout.base_sha) + + +if __name__ == "__main__": + unittest.main() diff --git a/bench/testsupport.py b/bench/testsupport.py index 1c720e5..6102233 100755 --- a/bench/testsupport.py +++ b/bench/testsupport.py @@ -4,6 +4,7 @@ # # Python 3 standard library only — no third-party dependencies. +import json import os import pathlib import shutil @@ -58,9 +59,8 @@ def make_verify_config_dir(root: pathlib.Path, plugin_src: pathlib.Path, "installLocation": str(plugin_src), } } - import json as _json (cfg / "plugins" / "known_marketplaces.json").write_text( - _json.dumps(known), encoding="utf-8" + json.dumps(known), encoding="utf-8" ) else: dest = cfg / "plugins" / "marketplaces" / "coding" @@ -102,3 +102,156 @@ def with_path(bin_dir: pathlib.Path) -> dict: env = dict(os.environ) env["PATH"] = f"{bin_dir}{os.pathsep}{env.get('PATH', '')}" return env + + +# ---------------------------------------------------------------------- +# Git-repo test helpers +# ---------------------------------------------------------------------- +def init_git_repo(path: pathlib.Path) -> pathlib.Path: + """Initialize a directory as a git repo and configure user identity. + + Creates an initial empty commit so HEAD is valid. + Returns the path. + """ + path = pathlib.Path(path) + path.mkdir(parents=True, exist_ok=True) + subprocess.run(["git", "init", "-q"], cwd=str(path), check=True, + capture_output=True, text=True) + subprocess.run( + ["git", "-C", str(path), "config", "user.email", "test@example.com"], + check=True, capture_output=True, text=True, + ) + subprocess.run( + ["git", "-C", str(path), "config", "user.name", "Test User"], + check=True, capture_output=True, text=True, + ) + # Create initial commit so HEAD is valid + subprocess.run( + ["git", "-C", str(path), "commit", "--allow-empty", "-m", "initial"], + check=True, capture_output=True, text=True, + ) + return path + + +def commit_file(repo: pathlib.Path, relpath: str, text: str, + message: str = "commit") -> str: + """Write a file, git add, git commit, return the resulting full SHA.""" + repo = pathlib.Path(repo) + fpath = repo / relpath + fpath.parent.mkdir(parents=True, exist_ok=True) + fpath.write_text(text, encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", str(relpath)], + check=True, capture_output=True, text=True) + result = subprocess.run( + ["git", "-C", str(repo), "commit", "-m", message], + check=True, capture_output=True, text=True, + ) + sha = subprocess.run( + ["git", "-C", str(repo), "rev-list", "-1", "HEAD"], + check=True, capture_output=True, text=True, + ).stdout.strip() + return sha + + +def make_merge_repo(path: pathlib.Path) -> dict: + """Build a repo with a real two-parent merge commit. + + Creates: + - commit 'base' on default branch + - branch 'feature', commit a change on it (touching a new file) + - back to default branch, commit another change + - git merge --no-ff feature (creates two-parent merge) + + Returns {"repo": path, "merge_sha": ..., "base_sha": ..., "head_sha": ...} + where base_sha/head_sha are the merge's first and second parents. + """ + path = init_git_repo(path) + + # Detect default branch name + default_branch = subprocess.run( + ["git", "-C", str(path), "rev-parse", "--abbrev-ref", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + + # Commit on default branch + base_sha = commit_file(path, "base.txt", "base content\n", "add base") + + # Branch and commit + subprocess.run(["git", "-C", str(path), "checkout", "-b", "feature", "-q"], + check=True, capture_output=True, text=True) + head_sha = commit_file(path, "feature.txt", "feature content\n", "add feature") + + # Back to default branch, commit, merge + subprocess.run(["git", "-C", str(path), "checkout", default_branch, "-q"], + check=True, capture_output=True, text=True) + commit_file(path, "main.txt", "main content\n", "add main") + + subprocess.run( + ["git", "-C", str(path), "merge", "--no-ff", "feature", "-m", "merge"], + check=True, capture_output=True, text=True, + ) + + merge_sha = subprocess.run( + ["git", "-C", str(path), "rev-list", "-1", "HEAD"], + check=True, capture_output=True, text=True, + ).stdout.strip() + + return { + "repo": path, + "merge_sha": merge_sha, + "base_sha": base_sha, + "head_sha": head_sha, + } + + +def make_squash_repo(path: pathlib.Path) -> dict: + """Build a single-parent repo where head_sha == merge_sha (squash shape).""" + path = init_git_repo(path) + + # Parent commit + parent_sha = commit_file(path, "parent.txt", "parent content\n", "add parent") + + # The squash commit (head == merge) + merge_sha = commit_file(path, "squash.txt", "squash content\n", "add squash") + + return { + "repo": path, + "merge_sha": merge_sha, + "base_sha": parent_sha, + "head_sha": merge_sha, + } + + +def make_empty_diff_repo(path: pathlib.Path) -> dict: + """Build a single-parent repo where base_sha == head_sha (empty diff).""" + path = init_git_repo(path) + + # One commit + sha = commit_file(path, "file.txt", "content\n", "add file") + + # base_sha == head_sha means the diff range will be sha..sha = empty + return { + "repo": path, + "merge_sha": sha, + "base_sha": sha, + "head_sha": sha, + } + + +def stub_git(bin_dir: pathlib.Path, log_file: pathlib.Path) -> pathlib.Path: + """Install a stub `git` on PATH that logs every invocation to log_file. + + Each invocation appends one line: 'cwd= args=' + Returns the stub path. + """ + log_file = pathlib.Path(log_file) + body = f'printf "cwd=%s args=%s\\n" "$(pwd)" "$*" >> "{log_file}"\nexit 0' + return make_stub_bin(bin_dir, "git", body) + + +def make_manifest(path: pathlib.Path, entries: list, version: str = "test-1") -> pathlib.Path: + """Write a minimal valid manifest JSON to path and return it.""" + manifest = {"version": version, "prs": entries} + path = pathlib.Path(path) + path.write_text(json.dumps(manifest), encoding="utf-8") + return path diff --git a/prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md b/prompts/completed/012-pr-review-bench-runner-pr-resolution.md similarity index 97% rename from prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md rename to prompts/completed/012-pr-review-bench-runner-pr-resolution.md index 6853dc6..0b89dde 100644 --- a/prompts/in-progress/012-pr-review-bench-runner-pr-resolution.md +++ b/prompts/completed/012-pr-review-bench-runner-pr-resolution.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [002-pr-review-bench-runner] +summary: Extended bench/run.py with PR resolution (git chokepoint, ensure_refs, resolve_diff_range, changed_files, prepare_worktree, resolve_pr), path helpers, fetch_url; rewrote process_pr stub; added bench/test_resolve.py with 12 unit tests covering AC2/AC3/AC7/AC8; extended bench/testsupport.py with git-repo helpers +execution_id: coding-bench-runner-exec-012-pr-review-bench-runner-pr-resolution +dark-factory-version: v0.192.9 created: "2026-08-06T21:06:00Z" queued: "2026-08-06T21:40:07Z" +started: "2026-08-06T21:45:57Z" +completed: "2026-08-06T22:08:44Z" --- From 3c235c1887b336a74172fcfd64f73a9116a526b8 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 7 Aug 2026 09:44:28 +0200 Subject: [PATCH 3/5] 013-spec-002-review-invocation-and-ledger --- CHANGELOG.md | 3 + bench/run.py | 464 ++++++++++- bench/test_review.py | 766 ++++++++++++++++++ bench/testdata/sample-report.md | 18 + bench/testsupport.py | 26 +- ...3-spec-002-review-invocation-and-ledger.md | 339 ++++++++ .../014-spec-002-make-targets-and-docs.md | 206 +++++ .../in-progress/002-pr-review-bench-runner.md | 3 +- 8 files changed, 1783 insertions(+), 42 deletions(-) create mode 100644 bench/test_review.py create mode 100644 bench/testdata/sample-report.md create mode 100644 prompts/completed/013-spec-002-review-invocation-and-ledger.md create mode 100644 prompts/in-progress/014-spec-002-make-targets-and-docs.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 921f1df..dd179d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ Please choose versions by [Semantic Versioning](http://semver.org/). - bench: extend `bench/run.py` with PR resolution (fetch from manifest URL into `bench/.cache/repos/`, isolated working copies via `git worktree`), parent-count diff-range branching (`^1..^2` for merge commits, manifest `base_sha..head_sha` for single-parent commits), empty-diff abort (`EMPTY DIFF` loud failure), per-PR failure isolation, and strategy-label mismatch reporting - bench: add `bench/test_resolve.py` — 12 unit tests covering AC2, AC3, AC7, AC8 and related acceptance criteria (`parent_count`, `empty_diff`, `git_invocation_confined_to_cache_repos`, `fetch_url`) - bench: extend `bench/testsupport.py` with git-repo helpers (`init_git_repo`, `commit_file`, `make_merge_repo`, `make_squash_repo`, `make_empty_diff_repo`, `stub_git`, `make_manifest`) +- bench: extend `bench/run.py` with review invocation (`/coding:pr-review` via isolated `CLAUDE_CONFIG_DIR=$HOME/.claude-verify` + `DISABLE_AUTOUPDATER=1`, mode-aware raw-output cache, findings harvester normalising to `{path,line,rule_id,body}`, append-only ledger via atomic `os.replace`, single-instance `BenchLock` guard) +- bench: add `bench/test_review.py` — 13 unit tests covering AC4, AC5, AC10 and related acceptance criteria (cache-hit, mode-change-cache-miss, harvest-normalise, ledger-atomicity, second-runner-lock, failure-isolation) +- bench: add `bench/testdata/sample-report.md` — fixture for AC10 harvester verification ## v0.35.0 diff --git a/bench/run.py b/bench/run.py index a50f4ea..beb68a3 100755 --- a/bench/run.py +++ b/bench/run.py @@ -18,14 +18,18 @@ import argparse import dataclasses +import datetime import hashlib import json import os import pathlib import re +import shlex import shutil import subprocess import sys +import tempfile +import time # ---------------------------------------------------------------------- # Module constants @@ -219,6 +223,113 @@ def assert_under(path: pathlib.Path, root: pathlib.Path) -> pathlib.Path: return resolved +# ---------------------------------------------------------------------- +# Cache and ledger path helpers +# ---------------------------------------------------------------------- +def reviews_root(cache_root: pathlib.Path) -> pathlib.Path: + return cache_root / "reviews" + + +def failures_root(cache_root: pathlib.Path) -> pathlib.Path: + return cache_root / "failures" + + +def cache_key(cfg_hash: str, pr_id: str) -> str: + return f"{cfg_hash}__{safe_pr_key(pr_id)}" + + +def cache_row_path(cache_root: pathlib.Path, cfg_hash: str, pr_id: str) -> pathlib.Path: + return reviews_root(cache_root) / f"{cache_key(cfg_hash, pr_id)}.json" + + +def cache_raw_path(cache_root: pathlib.Path, cfg_hash: str, pr_id: str) -> pathlib.Path: + return reviews_root(cache_root) / f"{cache_key(cfg_hash, pr_id)}.stdout.txt" + + +def failure_log_path(cache_root: pathlib.Path, cfg_hash: str, pr_id: str) -> pathlib.Path: + return failures_root(cache_root) / f"{cache_key(cfg_hash, pr_id)}.stderr.txt" + + +def ledger_path(results_dir: pathlib.Path) -> pathlib.Path: + return results_dir / "results.jsonl" + + +def lock_path(results_dir: pathlib.Path) -> pathlib.Path: + return results_dir / ".lock" + + +# ---------------------------------------------------------------------- +# Single-instance lock +# ---------------------------------------------------------------------- +class BenchLock: + """Single-instance lock that aborts if another bench run is active.""" + + def __init__(self, results_dir: pathlib.Path) -> None: + self._results_dir = results_dir + self._fd = None + + def __enter__(self) -> "BenchLock": + lp = lock_path(self._results_dir) + try: + self._fd = os.open( + str(lp), + os.O_CREAT | os.O_EXCL | os.O_WRONLY, + 0o644, + ) + except FileExistsError: + raise BenchError( + f"another bench run is in progress; " + f"remove the lock file to clear it: {lp}" + ) + ts = datetime.datetime.now(datetime.timezone.utc).isoformat() + os.write(self._fd, f"{os.getpid()} {ts}\n".encode("utf-8")) + os.close(self._fd) + self._fd = None + return self + + def __exit__(self, exc_type, exc, tb) -> None: + if self._fd is not None: + os.close(self._fd) + self._fd = None + try: + os.unlink(lock_path(self._results_dir)) + except FileNotFoundError: + pass + + +# ---------------------------------------------------------------------- +# Atomic append-only ledger +# ---------------------------------------------------------------------- +def atomic_write_bytes(path: pathlib.Path, data: bytes) -> None: + """Write data atomically to path via rename from a same-directory temp file.""" + tmp = tempfile.NamedTemporaryFile( + dir=str(path.parent), + delete=False, + ) + try: + tmp.write(data) + tmp.flush() + os.fsync(tmp.fileno()) + tmp.close() + os.replace(tmp.name, str(path)) + except Exception: + try: + os.unlink(tmp.name) + except FileNotFoundError: + pass + raise + + +def append_row(results_dir: pathlib.Path, row: dict) -> None: + """Append one JSON row to the ledger atomically.""" + lp = ledger_path(results_dir) + existing = b"" + if lp.exists(): + existing = lp.read_bytes() + encoded = json.dumps(row, sort_keys=True, ensure_ascii=False).encode("utf-8") + b"\n" + atomic_write_bytes(lp, existing + encoded) + + # ---------------------------------------------------------------------- # Git helpers # ---------------------------------------------------------------------- @@ -497,6 +608,196 @@ def check_plugin_resolution(coding_repo: pathlib.Path, config_dir: pathlib.Path, return plugin_path +# ---------------------------------------------------------------------- +# Review invocation +# ---------------------------------------------------------------------- +def build_review_argv(*, model: str, effort: str, mode: str, + base_branch: str) -> list[str]: + """Build the claude argv for a /coding:pr-review invocation.""" + return [ + "claude", + "--print", + "--model", model, + "--effort", effort, + "--permission-mode", "bypassPermissions", + f"/coding:pr-review {base_branch} {mode}", + ] + + +def review_env(config_dir: pathlib.Path) -> dict: + """Build the environment for an isolated review subprocess.""" + env = dict(os.environ) + env["CLAUDE_CONFIG_DIR"] = str(config_dir) + env["DISABLE_AUTOUPDATER"] = "1" + return env + + +def invoke_review(*, argv: list[str], worktree: pathlib.Path, + cache_root: pathlib.Path, + config_dir: pathlib.Path) -> subprocess.CompletedProcess: + """Run the review subprocess and return the completed process.""" + assert_under(worktree, repos_root(cache_root)) + return subprocess.run( + argv, + cwd=str(worktree), + env=review_env(config_dir), + capture_output=True, + text=True, + timeout=REVIEW_TIMEOUT_SECONDS, + ) + + +# ---------------------------------------------------------------------- +# Harvesting +# ---------------------------------------------------------------------- +def load_rule_ids(coding_repo: pathlib.Path) -> set: + """Load all rule IDs from the rules/index.json file. + + Tries coding_repo first; falls back to REPO_ROOT (for test environments + where coding_repo is a minimal temp directory without the full index). + """ + index_path = coding_repo / "rules" / "index.json" + if not index_path.is_file(): + index_path = REPO_ROOT / "rules" / "index.json" + try: + data = json.loads(index_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as err: + raise BenchError(f"cannot load rules/index.json from {coding_repo}: {err}") + if not isinstance(data, list): + raise BenchError(f"rules/index.json is not a JSON list: {index_path}") + return {entry["id"] for entry in data if "id" in entry} + + +def _extract_rule_id(text: str, known_rule_ids: set) -> str | None: + """Extract the first rule ID token from text, or None.""" + for token in re.split(r"[\s`\(\)\[\],:]+", text): + if token in known_rule_ids: + return token + return None + + +def _extract_path_line(text: str, known_rule_ids: set) -> tuple[str | None, int | None]: + """Extract the first path:line reference from text, skipping known rule IDs. + + A path:line is a token containing a dot extension followed by :NN. + A token that is a known rule ID is never treated as a path. + """ + # Find all potential path:line matches + for m in re.finditer(r"([A-Za-z0-9_./-]+\.[A-Za-z0-9_./-]+):(\d+)", text): + candidate = m.group(1) + if candidate not in known_rule_ids: + return candidate, int(m.group(2)) + return None, None + + +def _normalize_body(lines: list[str]) -> str: + """Strip bullet marker and join continuation lines into one whitespace-collapsed string.""" + body = lines[0] + if body.startswith(("*", "-")): + body = body[1:].lstrip() + body = " ".join([body] + lines[1:]) + body = re.sub(r"\s+", " ", body).strip() + return body + + +def harvest(report_text: str, known_rule_ids: set) -> list: + """Normalize a /coding:pr-review Step 5 report into a list of findings. + + Returns a list of dicts, each with keys: path, line, rule_id, body. + """ + findings: list = [] + current_section: str | None = None + current_finding_lines: list[str] = [] + section_names = {"must fix", "should fix", "nice to have"} + + def flush_finding(): + nonlocal current_finding_lines, current_section, findings + if not current_finding_lines or current_section is None: + return + text = " ".join(current_finding_lines) + body = _normalize_body(current_finding_lines) + # Skip the "None." empty-section sentinel + if body.strip() in ("None.", "None"): + current_finding_lines = [] + return + rule_id = _extract_rule_id(text, known_rule_ids) + path, line_num = _extract_path_line(text, known_rule_ids) + findings.append({ + "path": path, + "line": line_num, + "rule_id": rule_id, + "body": body, + }) + current_finding_lines = [] + + lines = report_text.splitlines() + i = 0 + while i < len(lines): + line = lines[i] + heading_match = re.match(r"^#{1,6}\s+(.+)$", line) + if heading_match: + heading_text = heading_match.group(1).strip() + # Strip trailing severity annotation like (Critical) + heading_text = re.sub(r"\s*\([^)]+\)\s*$", "", heading_text).strip() + heading_lower = heading_text.lower() + if heading_lower in section_names: + flush_finding() + current_section = heading_lower + current_finding_lines = [] + else: + # Any other heading (including traceability) ends the current section + flush_finding() + current_section = None + current_finding_lines = [] + i += 1 + continue + + if current_section is not None: + stripped = line.strip() + bullet_match = re.match(r"^\s{0,3}([-*])\s+(.+)$", stripped) + if bullet_match: + flush_finding() + current_finding_lines = [bullet_match.group(2)] + elif stripped: + current_finding_lines.append(stripped) + i += 1 + + flush_finding() + return findings + + +# ---------------------------------------------------------------------- +# Result row assembly +# ---------------------------------------------------------------------- +def build_row(*, checkout: PrCheckout, cfg_hash: str, rc_hash: str, + model: str, effort: str, mode: str, prs_version: str, + review_command: str, started_at: str, + duration_seconds: float, findings: list, + raw_output_ref: str) -> dict: + """Build a result row from a completed review.""" + return { + "config_hash": cfg_hash, + "rules_commands_hash": rc_hash, + "model": model, + "effort": effort, + "mode": mode, + "prs_version": prs_version, + "pr_id": checkout.pr_id, + "base_sha": checkout.base_sha, + "head_sha": checkout.head_sha, + "diff_range": checkout.diff_range, + "changed_files": checkout.changed_files, + "parent_count": checkout.parent_count, + "notes": checkout.notes, + "review_command": review_command, + "started_at": started_at, + "duration_seconds": round(duration_seconds, 3), + "findings": findings, + "raw_output_ref": raw_output_ref, + "runner_version": RUNNER_VERSION, + } + + # ---------------------------------------------------------------------- # CLI surface # ---------------------------------------------------------------------- @@ -577,62 +878,145 @@ def run_bench(*, coding_repo: pathlib.Path, manifest_path: pathlib.Path, cfg_hash = config_hash(rc_hash, model, effort, mode, manifest["version"]) + results_dir.mkdir(parents=True, exist_ok=True) + known_rule_ids = load_rule_ids(coding_repo) + print( f"config {cfg_hash[:16]} rules+commands {rc_hash[:16]} " f"model={model} effort={effort} mode={mode} prs={manifest['version']}" ) - outcomes: list[tuple[str, str]] = [] - for entry in manifest["prs"]: - pr_id = entry["id"] - try: - outcome, detail = process_pr( - entry=entry, - coding_repo=coding_repo, - results_dir=results_dir, - cache_root=cache_root, - model=model, - effort=effort, - mode=mode, - config_dir=config_dir, - cfg_hash=cfg_hash, - rc_hash=rc_hash, - prs_version=manifest["version"], - ) - except subprocess.TimeoutExpired as err: - outcome, detail = "failed", "timeout" - except OSError as err: - outcome, detail = "failed", str(err) - except BenchError as err: - outcome, detail = "failed", str(err) - outcomes.append((pr_id, f"{outcome}: {detail}")) + with BenchLock(results_dir): + outcomes: list[tuple[str, str]] = [] + for entry in manifest["prs"]: + pr_id = entry["id"] + try: + outcome, detail = process_pr( + entry=entry, + coding_repo=coding_repo, + results_dir=results_dir, + cache_root=cache_root, + model=model, + effort=effort, + mode=mode, + config_dir=config_dir, + cfg_hash=cfg_hash, + rc_hash=rc_hash, + prs_version=manifest["version"], + known_rule_ids=known_rule_ids, + ) + except subprocess.TimeoutExpired as err: + outcome, detail = "failed", "timeout" + except OSError as err: + outcome, detail = "failed", str(err) + except BenchError as err: + outcome, detail = "failed", str(err) + outcomes.append((pr_id, f"{outcome}: {detail}")) - n_ok = sum(1 for _, d in outcomes if d.startswith("ok:")) - n_cached = sum(1 for _, d in outcomes if d.startswith("cache hit:")) - n_failed = sum(1 for _, d in outcomes if d.startswith("failed:")) + n_ok = sum(1 for _, d in outcomes if d.startswith("ok:")) + n_cached = sum(1 for _, d in outcomes if d.startswith("cache hit:")) + n_failed = sum(1 for _, d in outcomes if d.startswith("failed:")) - for pr_id, outcome in outcomes: - print(f"{pr_id}: {outcome}") - print(f"summary: {n_ok} ok, {n_cached} cache hit, {n_failed} failed") + for pr_id, outcome in outcomes: + print(f"{pr_id}: {outcome}") + print(f"summary: {n_ok} ok, {n_cached} cache hit, {n_failed} failed") - return 0 if n_failed == 0 else 1 + return 0 if n_failed == 0 else 1 def process_pr(*, entry: dict, coding_repo: pathlib.Path, results_dir: pathlib.Path, cache_root: pathlib.Path, model: str, effort: str, mode: str, config_dir: pathlib.Path, cfg_hash: str, - rc_hash: str, prs_version: str) -> tuple[str, str]: - """Process a single PR — resolution complete, review invocation stubbed. + rc_hash: str, prs_version: str, + known_rule_ids: set) -> tuple[str, str]: + """Process a single PR: cache check, resolve, review, harvest, ledger.""" + pr_id = entry["id"] + + # 1. Cache check — before any git work + row_path = cache_row_path(cache_root, cfg_hash, pr_id) + if row_path.exists(): + try: + row = json.loads(row_path.read_text(encoding="utf-8")) + n_findings = len(row.get("findings", [])) + return ("cache hit", f"cached ({row_path.name}): {n_findings} findings") + except (json.JSONDecodeError, OSError): + pass # treat corrupt cache as miss - PR resolution (this prompt) reconstructs the diff range and prepares the - working copy inside the runner's cache. Review invocation ships in prompt 3. - """ + # 2. Resolve PR checkout = resolve_pr(cache_root, entry) - notes_suffix = "" - if checkout.notes: - notes_suffix = "; " + "; ".join(checkout.notes) - return ("failed", f"review invocation not yet implemented (prompt 3 of spec 002){notes_suffix}") + + # 3. Build argv and invoke review + argv = build_review_argv( + model=model, + effort=effort, + mode=mode, + base_branch=checkout.base_branch, + ) + started_at = datetime.datetime.now(datetime.timezone.utc).isoformat() + t0 = time.monotonic() + + try: + proc = invoke_review( + argv=argv, + worktree=checkout.worktree, + cache_root=cache_root, + config_dir=config_dir, + ) + except subprocess.TimeoutExpired as err: + # Write failure log + failures_root(cache_root).mkdir(parents=True, exist_ok=True) + failure_log = failure_log_path(cache_root, cfg_hash, pr_id) + stderr_bytes = err.stderr or b"" + if isinstance(stderr_bytes, str): + stderr_bytes = stderr_bytes.encode("utf-8") + failure_log.write_bytes(stderr_bytes) + raise + + if proc.returncode != 0: + failures_root(cache_root).mkdir(parents=True, exist_ok=True) + failure_log = failure_log_path(cache_root, cfg_hash, pr_id) + failure_log.write_bytes(proc.stderr.encode("utf-8") if proc.stderr else b"") + raise BenchError(f"{pr_id}: review invocation failed: exit {proc.returncode}") + + duration_seconds = time.monotonic() - t0 + + # 5. Write raw stdout verbatim before any parsing + reviews_root(cache_root).mkdir(parents=True, exist_ok=True) + raw_path = cache_raw_path(cache_root, cfg_hash, pr_id) + atomic_write_bytes(raw_path, proc.stdout.encode("utf-8")) + + # 6. Harvest findings + findings = harvest(proc.stdout, known_rule_ids) + + # 7. Build row and append to ledger + review_command = shlex.join(argv) + # raw_output_ref: relative to REPO_ROOT if under it, else absolute + try: + raw_output_ref = str(raw_path.relative_to(REPO_ROOT)) + except ValueError: + raw_output_ref = str(raw_path) + + row = build_row( + checkout=checkout, + cfg_hash=cfg_hash, + rc_hash=rc_hash, + model=model, + effort=effort, + mode=mode, + prs_version=prs_version, + review_command=review_command, + started_at=started_at, + duration_seconds=duration_seconds, + findings=findings, + raw_output_ref=raw_output_ref, + ) + append_row(results_dir, row) + + # 8. Write cache marker + atomic_write_bytes(row_path, json.dumps(row, sort_keys=True).encode("utf-8")) + + return ("ok", f"{len(findings)} findings in {duration_seconds:.3f}s") # ---------------------------------------------------------------------- diff --git a/bench/test_review.py b/bench/test_review.py new file mode 100644 index 0000000..d7ec0c0 --- /dev/null +++ b/bench/test_review.py @@ -0,0 +1,766 @@ +#!/usr/bin/env python3 +"""Unit tests for bench/run.py review invocation, caching, harvesting, and ledger.""" + +import json +import os +import pathlib +import shutil +import subprocess +import sys +import tempfile +import unittest +from unittest import mock + +import run +import testsupport + + +class TestSecondRunIsCacheHit(unittest.TestCase): + """AC4: a second invocation of the same configuration invokes zero reviews.""" + + def test_second_run_is_cache_hit_and_invokes_zero_reviews(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed two merge repos + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_b = repos_root / "testowner" / "repo_b" + repo_a.mkdir(parents=True) + repo_b.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + info_b = testsupport.make_merge_repo(repo_b) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + { + "id": "test#2", + "owner": "testowner", + "repo": "repo_b", + "number": 2, + "merge_strategy": "merge-commit", + "merge_sha": info_b["merge_sha"], + "base_sha": info_b["base_sha"], + "head_sha": info_b["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + # First run + with mock.patch.dict(os.environ, env): + rc1 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc1, 0, "first run must succeed") + lines_first = counter.read_text().splitlines() + self.assertEqual(len(lines_first), 2, f"first run must invoke 2 reviews, got: {lines_first}") + + # Second run + results_dir2 = td / "results2" + results_dir2.mkdir(parents=True) + with mock.patch.dict(os.environ, env): + rc2 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir2, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc2, 0, "second run must succeed") + lines_second = counter.read_text().splitlines() + self.assertEqual(len(lines_second), 2, + f"second run must NOT invoke reviews (cache hit), got {len(lines_second)} lines: {lines_second}") + + # Ledger has 2 rows from first run + ledger = run.ledger_path(results_dir) + self.assertTrue(ledger.exists()) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 2) + + # Second run stdout contains "cache hit" twice + # We can't capture stdout easily, but we can verify via return code 0 and no new rows + + +class TestModeChangeIsCacheMiss(unittest.TestCase): + """AC5: changing only --mode is a cache miss.""" + + def test_mode_change_is_cache_miss(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + # First run: selector mode + with mock.patch.dict(os.environ, env): + rc1 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="selector", + config_dir=cfg, + ) + + self.assertEqual(rc1, 0) + lines_first = counter.read_text().splitlines() + self.assertEqual(len(lines_first), 1, f"first run must invoke 1 review: {lines_first}") + + # Second run: full mode — same everything else + with mock.patch.dict(os.environ, env): + rc2 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="full", + config_dir=cfg, + ) + + self.assertEqual(rc2, 0) + lines_second = counter.read_text().splitlines() + self.assertEqual(len(lines_second), 2, + f"mode change must cause cache miss, got {len(lines_second)} lines: {lines_second}") + + # The second counter line must contain "full" (the mode literal reached the subprocess) + self.assertIn("full", lines_second[1], + f"second counter line must contain 'full': {lines_second[1]}") + + # Ledger has 2 rows + ledger = run.ledger_path(results_dir) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 2) + self.assertNotEqual(rows[0]["mode"], rows[1]["mode"], + f"modes must differ: {rows[0]['mode']} vs {rows[1]['mode']}") + self.assertIn("selector", rows[0]["review_command"]) + self.assertIn("full", rows[1]["review_command"]) + + +class TestCachePathDiffersWhenOnlyModeDiffers(unittest.TestCase): + """Unit-level guard: cache paths differ when only mode differs.""" + + def test_cache_path_differs_when_only_mode_differs(self): + rc = "a" * 64 + pr_id = "test#1" + h_selector = run.config_hash(rc, "claude-opus-5", "high", "selector", "dev-1") + h_full = run.config_hash(rc, "claude-opus-5", "high", "full", "dev-1") + + self.assertNotEqual(h_selector, h_full) + + cache = pathlib.Path("/tmp/cache") + row_selector = run.cache_row_path(cache, h_selector, pr_id) + row_full = run.cache_row_path(cache, h_full, pr_id) + self.assertNotEqual(str(row_selector), str(row_full), + "cache row paths must differ when mode differs") + + raw_selector = run.cache_raw_path(cache, h_selector, pr_id) + raw_full = run.cache_raw_path(cache, h_full, pr_id) + self.assertNotEqual(str(raw_selector), str(raw_full), + "cache raw paths must differ when mode differs") + + +class TestHarvestNormalizesSampleReport(unittest.TestCase): + """AC10: harvest normalizes a review report into findings.""" + + def test_harvest_normalizes_sample_report(self): + text = (run.BENCH_DIR / "testdata" / "sample-report.md").read_text() + known_ids = run.load_rule_ids(run.REPO_ROOT) + findings = run.harvest(text, known_ids) + + # Expected: 3 findings (Must Fix x2, Should Fix x1) + # The Nice to Have section ("None.") and traceability section produce 0 findings + self.assertEqual(len(findings), 3, f"expected 3 findings, got: {findings}") + + # Finding 1: Must Fix with inline path:line + f1 = findings[0] + self.assertEqual(f1["rule_id"], "agent-cmd/agent-frontmatter") + self.assertEqual(f1["path"], "agents/my-agent.md") + self.assertEqual(f1["line"], 3) + self.assertIn("agent-cmd/agent-frontmatter", f1["body"]) + + # Finding 2: Must Fix without path:line (rule_id only) + f2 = findings[1] + self.assertEqual(f2["rule_id"], "agent-cmd/command-thin") + self.assertIsNone(f2["path"]) + self.assertIsNone(f2["line"]) + + # Finding 3: Should Fix with continuation + f3 = findings[2] + self.assertEqual(f3["rule_id"], "changelog/unreleased-entry-required") + self.assertIn("CHANGELOG.md", f3["body"]) + + # All rule_ids are in the real index + for f in findings: + if f["rule_id"] is not None: + self.assertIn(f["rule_id"], known_ids, + f"rule_id {f['rule_id']} not in rules/index.json") + + +class TestHarvestKeepsFindingWithoutAnyRuleId(unittest.TestCase): + """A finding that cites no known rule ID is kept with rule_id=null.""" + + def test_harvest_keeps_finding_without_any_rule_id(self): + report = """#### Must Fix (Critical) +- This finding has no rule ID at all but should still be kept. +""" + ids = run.load_rule_ids(run.REPO_ROOT) + findings = run.harvest(report, ids) + self.assertEqual(len(findings), 1) + self.assertIsNone(findings[0]["rule_id"]) + self.assertIn("no rule ID", findings[0]["body"]) + + +class TestHarvestIgnoresEmptySection(unittest.TestCase): + """A section whose body is "None." yields zero findings.""" + + def test_harvest_ignores_empty_section(self): + report = """#### Nice to Have (Optional) +None. +""" + ids = run.load_rule_ids(run.REPO_ROOT) + findings = run.harvest(report, ids) + self.assertEqual(len(findings), 0) + + +class TestLedgerIsAppendOnlyAndAtomic(unittest.TestCase): + """Ledger rows are append-only and written atomically.""" + + def test_ledger_is_append_only_and_atomic(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + results_dir = td / "results" + results_dir.mkdir(parents=True) + + row1 = {"pr_id": "test#1", "findings": []} + row2 = {"pr_id": "test#2", "findings": [{"rule_id": "foo/bar"}]} + row3 = {"pr_id": "test#3", "findings": []} + + run.append_row(results_dir, row1) + run.append_row(results_dir, row2) + + lp = run.ledger_path(results_dir) + self.assertEqual(len(lp.read_text().splitlines()), 2) + + # Before third append, record line 1 and 2 content + lines_before = lp.read_text().splitlines() + row1_before = lines_before[0] + row2_before = lines_before[1] + + run.append_row(results_dir, row3) + + lines_after = lp.read_text().splitlines() + self.assertEqual(len(lines_after), 3) + self.assertEqual(lines_after[0], row1_before, + "first row must be unchanged after append") + self.assertEqual(lines_after[1], row2_before, + "second row must be unchanged after append") + + # No leftover .tmp files + tmp_files = list(results_dir.glob("*.tmp")) + self.assertEqual(len(tmp_files), 0, f"no .tmp files expected, found: {tmp_files}") + + +class TestSecondRunnerExitsWithoutTouchingLedger(unittest.TestCase): + """A second runner started while one is in progress exits with error.""" + + def test_second_runner_exits_without_touching_ledger(self): + # Use the real REPO_ROOT as coding_repo so content_hash matches + # the plugin that the config dir points to. + results_dir = pathlib.Path(tempfile.mkdtemp()) + try: + lock = run.lock_path(results_dir) + lock.write_text("999999 2026-01-01T00:00:00+00:00\n", encoding="utf-8") + + old_home = os.environ.get("HOME", "") + try: + os.environ["HOME"] = str(results_dir) + + # Create a .claude-verify that points to REPO_ROOT + cfg_dir = results_dir / ".claude-verify" + cfg_dir.mkdir(parents=True, exist_ok=True) + (cfg_dir / "plugins").mkdir(parents=True, exist_ok=True) + km = { + "coding": { + "source": {"source": "github", "repo": "bborbe/coding"}, + "installLocation": str(run.REPO_ROOT), + } + } + (cfg_dir / "plugins" / "known_marketplaces.json").write_text( + json.dumps(km), encoding="utf-8" + ) + + result = subprocess.run( + [sys.executable, str(run.BENCH_DIR / "run.py"), + "--coding-repo", str(run.REPO_ROOT), + "--manifest", str(run.BENCH_DIR / "prs.json"), + "--out-dir", str(results_dir), + "--model", "test-model", + "--effort", "high", + "--mode", "short"], + capture_output=True, text=True, + env={**os.environ, "HOME": str(results_dir)}, + ) + finally: + os.environ["HOME"] = old_home + + self.assertEqual(result.returncode, 2, + f"second runner must exit 2, got {result.returncode}: {result.stderr}") + self.assertIn(str(lock), result.stderr, + f"error must name the lock file: {result.stderr}") + self.assertIn("another bench run", result.stderr, + f"error must mention another bench run: {result.stderr}") + finally: + shutil.rmtree(results_dir, ignore_errors=True) + + +class TestRowCarriesEveryRequiredField(unittest.TestCase): + """A successful run produces a row with every required field.""" + + def test_row_carries_every_required_field(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc, 0) + + ledger = run.ledger_path(results_dir) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 1) + row = rows[0] + + required_fields = [ + "config_hash", "rules_commands_hash", "model", "effort", "mode", + "prs_version", "pr_id", "base_sha", "head_sha", "diff_range", + "changed_files", "review_command", "started_at", "duration_seconds", + "findings", "raw_output_ref", "runner_version", + ] + for field in required_fields: + self.assertIn(field, row, f"row must have field: {field}") + self.assertIsNotNone(row[field], f"field {field} must not be None") + + # started_at must have UTC offset + self.assertIn("+", row["started_at"]) or row["started_at"].endswith("Z") + + +class TestRawOutputIsCachedVerbatim(unittest.TestCase): + """After a successful run, the raw stdout is cached verbatim.""" + + def test_raw_output_is_cached_verbatim(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + report_text = "findings: [{\"rule_id\":\"foo/bar\",\"path\":\"x.go\",\"line\":1}]" + stub = testsupport.stub_claude(bin_dir, counter, report_text) + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc, 0) + + ledger = run.ledger_path(results_dir) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + row = rows[0] + + raw_ref = row["raw_output_ref"] + if raw_ref.startswith("/"): + raw_path = pathlib.Path(raw_ref) + else: + raw_path = run.REPO_ROOT / raw_ref + + self.assertTrue(raw_path.exists(), f"raw cache file must exist: {raw_path}") + # The stub uses cat heredoc which appends a trailing newline + self.assertEqual(raw_path.read_text(), report_text + "\n", + "raw cache must contain the stub's report text (plus trailing newline)") + + +class TestFailedReviewLeavesNoRowAndNoCacheEntry(unittest.TestCase): + """A failing review produces no row and no cache entry.""" + + def test_failed_review_leaves_no_row_and_no_cache_entry(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude_failing(bin_dir, counter, exit_code=3) + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc, 1, "run must exit 1 when a PR fails") + + # No ledger row + ledger = run.ledger_path(results_dir) + if ledger.exists(): + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + else: + rows = [] + self.assertEqual(len(rows), 0, "no row must be written for failed review") + + # No cache entry in reviews/ + reviews = run.reviews_root(cache_root) + if reviews.exists(): + review_files = list(reviews.glob("*.json")) + list(reviews.glob("*.stdout.txt")) + else: + review_files = [] + self.assertEqual(len(review_files), 0, + f"no review cache files expected, found: {review_files}") + + # Failure log exists + failures = run.failures_root(cache_root) + failure_files = list(failures.glob("*.stderr.txt")) if failures.exists() else [] + self.assertEqual(len(failure_files), 1, + f"one failure log expected, found: {failure_files}") + + +class TestFailedPrDoesNotPreventLaterPrs(unittest.TestCase): + """A PR whose SHA is unresolvable does not prevent later PRs from running.""" + + def test_failed_pr_does_not_prevent_later_prs(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one good repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_good = repos_root / "testowner" / "goodrepo" + repo_good.mkdir(parents=True) + info_good = testsupport.make_merge_repo(repo_good) + + manifest_entries = [ + { + "id": "bad#1", + "owner": "badowner", + "repo": "badrepo", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": "a" * 40, + "base_sha": "b" * 40, + "head_sha": "c" * 40, + "changed_files": 1, + }, + { + "id": "good#2", + "owner": "testowner", + "repo": "goodrepo", + "number": 2, + "merge_strategy": "merge-commit", + "merge_sha": info_good["merge_sha"], + "base_sha": info_good["base_sha"], + "head_sha": info_good["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc, 1, "run must exit 1 when any PR fails") + + ledger = run.ledger_path(results_dir) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 1) + self.assertEqual(rows[0]["pr_id"], "good#2") + + +class TestCorruptCacheRowIsTreatedAsMiss(unittest.TestCase): + """A cache file containing malformed JSON is treated as a miss.""" + + def test_corrupt_cache_row_is_treated_as_miss(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.make_coding_repo(td / "repo") + cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + # Compute cfg_hash to know where to write corrupt cache + rc_hash = run.content_hash(plugin_src) + cfg_hash = run.config_hash(rc_hash, "test-model", "high", "short", "test-1") + + # Write a corrupt cache file + reviews = run.reviews_root(cache_root) + reviews.mkdir(parents=True, exist_ok=True) + corrupt_row = reviews / f"{run.cache_key(cfg_hash, 'test#1')}.json" + corrupt_row.write_text("{ this is not json", encoding="utf-8") + + # First run: should overwrite corrupt cache + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc, 0, "run must succeed after corrupt cache") + lines = counter.read_text().splitlines() + self.assertEqual(len(lines), 1, + "review must have been invoked (corrupt cache treated as miss)") + + # Cache file now parses as valid JSON + row_data = json.loads(corrupt_row.read_text(encoding="utf-8")) + self.assertIn("pr_id", row_data) + + +if __name__ == "__main__": + unittest.main() diff --git a/bench/testdata/sample-report.md b/bench/testdata/sample-report.md new file mode 100644 index 0000000..6dc310e --- /dev/null +++ b/bench/testdata/sample-report.md @@ -0,0 +1,18 @@ +# PR Review Summary + +This review was performed against the diff. The following findings were identified. + +#### Must Fix (Critical) +- `agent-cmd/agent-frontmatter`: The agent file `agents/my-agent.md:3` is missing required frontmatter fields. Every agent must have `description`, `allowed_tools`, and `trigger` fields defined. +- `agent-cmd/command-thin`: The slash command handler directly implements business logic instead of delegating to a specialist agent. Move the implementation to a dedicated agent. + +#### Should Fix (Important) +- `changelog/unreleased-entry-required`: This PR introduces a user-facing change but the changelog has no entry under `## Unreleased`. + Update `CHANGELOG.md` with an entry describing the change. + +#### Nice to Have (Optional) +None. + +#### Selector Mode: Classify Traceability +- `agent-cmd/gap-driven-feedback`: Consider adding a feedback loop to capture agent performance metrics. +- `agent-cmd/single-source-of-truth`: The information about agent capabilities is duplicated in two places. diff --git a/bench/testsupport.py b/bench/testsupport.py index 6102233..bdcddf4 100755 --- a/bench/testsupport.py +++ b/bench/testsupport.py @@ -91,12 +91,24 @@ def stub_claude(bin_dir: pathlib.Path, counter_file: pathlib.Path, """ counter_file = pathlib.Path(counter_file) body = ( - f"printf '%s\\n' '$*' >> '{counter_file}'\n" + f"printf '%s\\n' \"$*\" >> '{counter_file}'\n" f"cat <<'REPORT_EOF'\n{report_text}\nREPORT_EOF" ) return make_stub_bin(bin_dir, "claude", body) +def stub_claude_failing(bin_dir: pathlib.Path, counter_file: pathlib.Path, + exit_code: int = 3) -> pathlib.Path: + """Install a stub `claude` that appends args to counter_file and exits with exit_code.""" + counter_file = pathlib.Path(counter_file) + body = ( + f"printf '%s\\n' \"$*\" >> '{counter_file}'\n" + f"printf 'stub failure\\n' >&2\n" + f"exit {exit_code}" + ) + return make_stub_bin(bin_dir, "claude", body) + + def with_path(bin_dir: pathlib.Path) -> dict: """Return a copy of os.environ with bin_dir prepended to PATH.""" env = dict(os.environ) @@ -255,3 +267,15 @@ def make_manifest(path: pathlib.Path, entries: list, version: str = "test-1") -> path = pathlib.Path(path) path.write_text(json.dumps(manifest), encoding="utf-8") return path + + +def seed_cached_repo(cache_root: pathlib.Path, owner: str, repo: str, + builder) -> dict: + """Seed a repository under cache_root/repos/owner/repo using the given builder. + + builder is one of: make_merge_repo, make_squash_repo, make_empty_diff_repo. + Returns the builder's result dict. + """ + repo_path = cache_root / "repos" / owner / repo + repo_path.mkdir(parents=True, exist_ok=True) + return builder(repo_path) diff --git a/prompts/completed/013-spec-002-review-invocation-and-ledger.md b/prompts/completed/013-spec-002-review-invocation-and-ledger.md new file mode 100644 index 0000000..5bb01a1 --- /dev/null +++ b/prompts/completed/013-spec-002-review-invocation-and-ledger.md @@ -0,0 +1,339 @@ +--- +status: completed +spec: [002-pr-review-bench-runner] +summary: Extended bench/run.py with review invocation via /coding:pr-review in isolated CLAUDE_CONFIG_DIR, mode-aware raw-output cache, findings harvester, append-only ledger with atomic writes, single-instance BenchLock, plus 13 new unit tests in bench/test_review.py and bench/testdata/sample-report.md fixture +execution_id: coding-bench-runner-exec-013-spec-002-review-invocation-and-ledger +dark-factory-version: v0.192.9 +created: "2026-08-06T22:29:56Z" +queued: "2026-08-06T22:39:41Z" +started: "2026-08-06T22:39:45Z" +completed: "2026-08-07T07:44:28Z" +--- + + +- The benchmark runner finally invokes the real review command instead of reporting "not implemented" +- Reviews run inside a dedicated, isolated Claude configuration directory with the autoupdater switched off, so nothing picks up a plugin update mid-run +- The raw review output is kept verbatim before anything parses it, so a later change to the parser can re-derive findings from an old run without spending tokens again +- Repeating the exact same configuration costs zero reviews — already-finished pull requests are served from cache and invoke nothing at all +- Changing only the review mode is treated as a different configuration, so two modes can never be silently conflated under one cached answer +- Review output is normalized into a flat list of findings, and a finding that names a rule but no file location is kept rather than dropped +- Results form an append-only ledger: one row per completed pull request, never rewritten, never deleted, written so a crash can never leave half a row behind +- A second benchmark run started while one is already in progress refuses to start instead of interleaving into the same ledger +- A review that fails or times out leaves no row and no cache entry, keeps its error output for diagnosis, and lets the remaining pull requests finish + + + +Extend `bench/run.py` so a resolved pull request is actually reviewed: invoke `/coding:pr-review` through the `claude` executable in the isolated `$HOME/.claude-verify` configuration directory, cache the raw output under a mode-aware per-(PR, configuration) key, normalize the report into `{path, line, rule_id, body}` findings, and append exactly one row per completed pair to an append-only, atomically-written ledger guarded by a single-instance lock. This is the last piece that turns the runner from a resolver into a measuring instrument, and it is the only prompt in this spec that touches the review subprocess. + + + +Read `CLAUDE.md` for project conventions (Python 3 stdlib only, no personal paths, `make precommit` must stay green). +Read `specs/in-progress/002-pr-review-bench-runner.md` — this prompt implements Desired Behaviors 7 and 8 and Acceptance Criteria AC4, AC5, AC10. Its **Constraints** section lists the required result-row fields verbatim; its **Failure Modes** table defines the timeout, crash, and two-runners rows you must satisfy. + +Read `bench/run.py` — you are extending it. It already contains everything you build on; reuse it, do not restructure it: +- `BenchError` — the single exception type that maps to exit code 2 at the top level and to a per-PR `failed:` outcome inside the loop +- `RUNNER_VERSION`, `REVIEW_TIMEOUT_SECONDS` (45 × 60), `BENCH_DIR`, `REPO_ROOT`, `VALID_MODES` +- `content_hash(root)` and `config_hash(rules_commands_hash, model, effort, mode, prs_version)` — `config_hash` already mixes `mode` into the digest, which is the entire mechanism behind AC5 +- `load_manifest(path)`, `safe_pr_key(pr_id)` (replaces `#` with `_`, filename-safe) +- `repos_root(cache_root)`, `assert_under(path, root)` — the containment helper that every destructive or path-taking operation must go through +- `git(args, *, repo_dir, cache_root, check=True, timeout=600)` — the single git chokepoint; it is `-C`-only and never passes `cwd=` +- `resolve_pr(cache_root, entry) -> PrCheckout` and the `PrCheckout` dataclass with fields `pr_id`, `repo_dir`, `worktree`, `base_branch`, `head_branch`, `diff_range`, `base_sha`, `head_sha`, `changed_files`, `parent_count`, `notes` +- `run_bench(*, coding_repo, manifest_path, results_dir, cache_root, model, effort, mode, config_dir) -> int` — keyword-only; already prints the config banner, loops over `manifest["prs"]`, catches `subprocess.TimeoutExpired` / `OSError` / `BenchError` per PR, prints `f"{pr_id}: {outcome}"` per PR and a `summary:` line, and counts outcome strings by the prefixes `"ok:"`, `"cache hit:"`, `"failed:"` +- `process_pr(*, entry, coding_repo, results_dir, cache_root, model, effort, mode, config_dir, cfg_hash, rc_hash, prs_version) -> tuple[str, str]` — currently calls `resolve_pr` and returns the `("failed", "review invocation not yet implemented (prompt 3 of spec 002)")` stub you are replacing +- `main(argv)` — builds the parser, rejects `--golden` with exit 2, requires `--model`/`--effort`/`--mode`, and calls `run_bench(... config_dir=verify_config_dir())` + +Read `bench/testsupport.py` — you are extending it. It already has `make_coding_repo`, `make_verify_config_dir`, `make_stub_bin`, `stub_claude`, `with_path`, `init_git_repo`, `commit_file`, `make_merge_repo`, `make_squash_repo`, `make_empty_diff_repo`, `stub_git`, `make_manifest`. + +Read `bench/test_config.py` and `bench/test_resolve.py` — the established test style: plain `unittest.TestCase` classes with docstrings naming the AC, `tempfile.TemporaryDirectory()`, `import run` / `import testsupport` (discovery inserts `bench/` onto `sys.path`). 29 tests currently pass via `python3 -m unittest discover -s bench -p 'test_*.py'`. Match that style; do not introduce a test framework. + +Read `commands/pr-review.md` — the subprocess you are invoking. Its frontmatter `argument-hint` is `" [short|full|selector]"`, so the mode is the second positional word of the slash-command string. Step 0c diffs `origin/...HEAD`, which is why prompt 2 published `refs/remotes/origin/bench-base-` — pass `PrCheckout.base_branch` as the target branch. Step 5 is the report you harvest: three mandatory headings `Must Fix (Critical)`, `Should Fix (Important)`, `Nice to Have (Optional)`, each holding bullet findings that cite a rule by ID, with the literal `None.` written when a section is empty. Selector mode appends a traceability section after those three. + +Read `rules/index.json` — a JSON **list** of objects; each object's rule identifier is under the key `id` (not `rule_id`). 166 entries today. This file is the boundary that makes mechanical harvesting possible: `commands/pr-review.md` Step 4's citation validator already guarantees every emitted finding cites an ID from it. + +Read `scripts/build-index.py` — the repo's stdlib-only Python precedent for header-comment style and `sys.exit(main())` shape. + + + + +## 1. Imports + +Add `datetime`, `shlex`, `tempfile` and `time` to `bench/run.py`'s stdlib import list (`json`, `os`, `pathlib`, `subprocess` are already imported). No third-party imports, no new files outside `bench/`. + +## 2. Cache and ledger path helpers + +All of these are pure functions in `bench/run.py`: + +```python +def reviews_root(cache_root: pathlib.Path) -> pathlib.Path # cache_root / "reviews" +def failures_root(cache_root: pathlib.Path) -> pathlib.Path # cache_root / "failures" +def cache_key(cfg_hash: str, pr_id: str) -> str # f"{cfg_hash}__{safe_pr_key(pr_id)}" +def cache_row_path(cache_root, cfg_hash, pr_id) -> pathlib.Path # reviews_root / f"{key}.json" +def cache_raw_path(cache_root, cfg_hash, pr_id) -> pathlib.Path # reviews_root / f"{key}.stdout.txt" +def failure_log_path(cache_root, cfg_hash, pr_id) -> pathlib.Path # failures_root / f"{key}.stderr.txt" +def ledger_path(results_dir: pathlib.Path) -> pathlib.Path # results_dir / "results.jsonl" +def lock_path(results_dir: pathlib.Path) -> pathlib.Path # results_dir / ".lock" +``` + +**The cache key is mode-aware because it is derived from `cfg_hash`, and `config_hash` already mixes `mode` into its digest.** Do not build the key from `rc_hash`, and do not add a separate mode component — deriving it from `cfg_hash` is what makes "same everything except `--mode`" land on a different file, which is the whole of AC5. + +Failure logs live under `failures/`, deliberately **not** under `reviews/`: AC3 (shipped in prompt 2) asserts that an aborted PR creates no new file in `bench/.cache/reviews/`, while the spec's Failure Modes table requires a failed review's stderr to be preserved. Separate directories satisfy both with no conditional. + +## 3. Single-instance lock + +```python +class BenchLock: + def __init__(self, results_dir: pathlib.Path) -> None + def __enter__(self) -> "BenchLock" + def __exit__(self, exc_type, exc, tb) -> None +``` + +- Acquire in `__enter__`, **not** in `__init__` — constructing a `BenchLock` must have no filesystem side effect, so a caller can build one and still choose not to enter it. Acquire with `os.open(lock_path(results_dir), os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644)`; write the current pid and an ISO-8601 UTC timestamp into it, then close the descriptor. +- `FileExistsError` → raise `BenchError` stating that another bench run is in progress, naming the lock file path verbatim, and telling the operator that a stale lock from a killed process is removed by deleting that file. The spec's Failure Modes table requires the message to name the removable file. +- `__exit__` unlinks the lock file, tolerating `FileNotFoundError`. +- The lock must be released on every exit path — acquire it with a `with` statement in `run_bench`, never with a bare try/finally reimplementation. + +## 4. Atomic append-only ledger + +```python +def atomic_write_bytes(path: pathlib.Path, data: bytes) -> None +def append_row(results_dir: pathlib.Path, row: dict) -> None +``` + +- `atomic_write_bytes` writes to a temporary file **in the same directory** as `path` (use `tempfile.NamedTemporaryFile(dir=path.parent, delete=False)`), flushes, `os.fsync`es the descriptor, closes it, then `os.replace`s it onto `path`. Same-directory is required so the replace is a rename within one filesystem and therefore atomic. On any exception, remove the temporary file before re-raising so no `.tmp` debris survives. +- `append_row` serializes the row with `json.dumps(row, sort_keys=True, ensure_ascii=False)`, appends `"\n"`, concatenates it after the ledger's existing bytes (empty when the ledger does not exist), and hands the whole result to `atomic_write_bytes`. Never open the ledger in `"a"` mode, never rewrite or delete an existing line. The ledger is small (one row per fixture PR), so read-modify-write is cheap and gives the spec's stated "atomic write-then-rename means no truncated row is ever observed" literally. + +## 5. Review invocation + +```python +def build_review_argv(*, model: str, effort: str, mode: str, base_branch: str) -> list[str] +``` + +Returns exactly: + +```python +[ + "claude", + "--print", + "--model", model, + "--effort", effort, + "--permission-mode", "bypassPermissions", + f"/coding:pr-review {base_branch} {mode}", +] +``` + +The mode is the slash command's second positional word, matching `commands/pr-review.md`'s `argument-hint`. `--permission-mode bypassPermissions` is required because the review runs `git` commands and spawns agents non-interactively; without it a one-shot run stalls or silently loses tool access. The argv is a list — no shell string, and no manifest value is interpolated into one argument. + +```python +def review_env(config_dir: pathlib.Path) -> dict +``` + +Returns `dict(os.environ)` with `CLAUDE_CONFIG_DIR` set to `str(config_dir)` and `DISABLE_AUTOUPDATER` set to `"1"`. `CLAUDE_CONFIG_DIR` is the variable `commands/pr-review.md` itself reads (`${CLAUDE_CONFIG_DIR:-$HOME/.claude}/plugins/marketplaces/coding/...`), which is what makes the isolated directory actually take effect. Do not copy, filter, log or record any other environment variable, and never write environment values into a result row or cache file. + +```python +def invoke_review(*, argv: list[str], worktree: pathlib.Path, cache_root: pathlib.Path, + config_dir: pathlib.Path) -> subprocess.CompletedProcess +``` + +1. `assert_under(worktree, repos_root(cache_root))` before anything else — the worktree becomes the subprocess's working directory and must be inside the runner's own cache. +2. `subprocess.run(argv, cwd=str(worktree), env=review_env(config_dir), capture_output=True, text=True, timeout=REVIEW_TIMEOUT_SECONDS)`. +3. Return the completed process unchanged; the caller decides what a non-zero exit means. Let `subprocess.TimeoutExpired` propagate. + +**This is the only `cwd=` in `bench/run.py`.** The "`-C` only, never `cwd=`" rule belongs to the `git()` chokepoint and stays intact — `git()` is untouched by this prompt. Do not route the review through `git()`, and do not add a second `cwd=` anywhere. + +`REVIEW_TIMEOUT_SECONDS` is a fixed invariant. Do not add a flag, an environment variable, or a parameter for it. + +## 6. Harvesting a report into normalized findings + +```python +def load_rule_ids(coding_repo: pathlib.Path) -> set +``` + +Reads `/rules/index.json`, which is a JSON list of objects, and returns the set of every entry's `id` value. An unreadable or non-list file raises `BenchError` naming the path. + +```python +def harvest(report_text: str, known_rule_ids) -> list +``` + +Returns a list of dicts, in document order, each with exactly the keys `path`, `line`, `rule_id`, `body`. Contract: + +1. **Sections.** A finding section starts at a markdown heading (any level, `#` through `######`) whose text — after stripping `#`, `*`, and any trailing parenthesised severity like `(Critical)` — case-insensitively equals `Must Fix`, `Should Fix`, or `Nice to Have`. The section ends at the next markdown heading of any level, or end of text. +2. **Everything outside those three sections is ignored.** Preamble prose, the selector-mode traceability section, and any trailing notes contribute zero findings even when they mention rule IDs. +3. **Findings.** Inside a section, a line matching `^\s{0,3}[-*]\s+` starts a new finding. Following lines that are more deeply indented, or non-empty and not a new bullet and not a heading, are continuation lines of the current finding. +4. **Empty sections.** A section whose entire body strips to `None.` or `None` yields zero findings. +5. **`rule_id`.** Split the finding text on characters that cannot appear in a rule ID (whitespace, backticks, parentheses, brackets, commas, colons at token end) and take the first resulting token that is a member of `known_rule_ids`. No member → `None`. Membership in the real index — not a regex shape — is the discriminator, because a file path like `pkg/config/config.go` matches any plausible rule-ID regex. +6. **`path` / `line`.** From the first match of a file-with-line reference in the finding text: a token of `[A-Za-z0-9_./-]+` containing a `.` extension, immediately followed by `:` and one or more digits. `path` is the string, `line` is an `int`. No match → both `None`. A token that is a member of `known_rule_ids` is never treated as a path. +7. **`body`.** The finding text with the leading bullet marker removed, continuation lines joined by a single space, internal whitespace collapsed to single spaces, stripped. +8. **Nothing is dropped.** A finding citing a `rule_id` with no `path:line` is kept with `path` and `line` both `None` — this exact case is AC10. A finding citing no known rule ID at all is also kept, with `rule_id` `None`. + +## 7. Result row assembly + +```python +def build_row(*, checkout, cfg_hash, rc_hash, model, effort, mode, prs_version, + review_command: str, started_at: str, duration_seconds: float, + findings: list, raw_output_ref: str) -> dict +``` + +The spec's Constraints section makes these fields required — every one must be present and non-null (an empty `findings` list is a value, not null): + +`config_hash`, `rules_commands_hash`, `model`, `effort`, `mode`, `prs_version`, `pr_id`, `base_sha`, `head_sha`, `diff_range`, `changed_files`, `review_command`, `started_at`, `duration_seconds`, `findings`, `raw_output_ref`, `runner_version`. + +- `started_at` — `datetime.datetime.now(datetime.timezone.utc).isoformat()`, captured immediately before the subprocess starts, so it carries an explicit UTC offset (the spec's clock-skew failure mode requires an explicit offset). +- `duration_seconds` — measured with `time.monotonic()` around the subprocess, rounded to 3 decimals. Never derive it from wall-clock timestamps. +- `review_command` — `shlex.join(argv)`, so the mode literal appears in the row (AC5). +- `raw_output_ref` — the raw-stdout cache path relative to `REPO_ROOT` as a POSIX string when it is under `REPO_ROOT`, otherwise the absolute path (temp-dir test runs land in the second case). +- `runner_version` — `RUNNER_VERSION`. +- Additional fields are permitted: also record `parent_count` and `notes` from the `PrCheckout`. + +## 8. Rewrite `process_pr` + +Add `known_rule_ids` to the keyword-only parameter list; keep every existing parameter and its name. Sequence: + +1. **Cache check first, before any git work.** If `cache_row_path(...)` exists and parses as JSON, print nothing extra and return `("cache hit", )` where `` names the cache key and the cached row's finding count. Do **not** call `resolve_pr`, do **not** invoke the subprocess, do **not** append a row. `run_bench` renders this as `"{pr_id}: cache hit: {detail}"`, so the literal `cache hit` appears exactly once per PR — the detail string must not contain that literal a second time. Checking the cache before resolution is what makes a repeat run fast and network-free (AC4, AC18). A cache file that fails to parse as JSON is treated as a miss and overwritten at the end of a successful run. +2. `checkout = resolve_pr(cache_root, entry)` — unchanged behaviour, may raise `BenchError`. +3. `argv = build_review_argv(model=..., effort=..., mode=..., base_branch=checkout.base_branch)`; capture `started_at` and the monotonic start. +4. `proc = invoke_review(argv=argv, worktree=checkout.worktree, cache_root=cache_root, config_dir=config_dir)`. + - `subprocess.TimeoutExpired` — write whatever partial stderr the exception carries to `failure_log_path(...)` (creating `failures/` first; the exception's `stderr` may be `None` or `bytes`), then re-raise so `run_bench` records `failed: timeout`. No row, no cache entry. + - Non-zero `returncode` — write `proc.stderr` to `failure_log_path(...)`, then raise `BenchError` whose message names the PR id and the literal `exit ` so the summary reads `failed: ... exit `. No row, no cache entry. +5. **Write the raw stdout verbatim to `cache_raw_path(...)` before any parsing** (create `reviews/` first). Desired Behavior 7 requires the untouched bytes so a future harvester change can re-normalize old runs without re-spending tokens. Write it with `atomic_write_bytes`. +6. `findings = harvest(proc.stdout, known_rule_ids)`. +7. `row = build_row(...)`; `append_row(results_dir, row)`; **then** write the cache marker `cache_row_path(...)` with `atomic_write_bytes(json.dumps(row, sort_keys=True).encode("utf-8"))`. Ledger first, marker second: a crash between the two costs a duplicate row on re-run, which is visible and harmless in an append-only ledger, whereas the reverse order would serve a cache hit for a PR that has no row and silently lose it forever. +8. Return `("ok", )` where `` names the finding count and the duration. + +## 9. Wire `run_bench` + +Only these changes; leave the banner, the per-PR exception handling, the outcome counting and the summary printing exactly as they are: + +1. After the plugin-resolution preflight and before the loop, `results_dir.mkdir(parents=True, exist_ok=True)`, then compute `known_rule_ids = load_rule_ids(coding_repo)`. +2. Wrap the whole per-PR loop in `with BenchLock(results_dir):`. A `BenchError` raised while acquiring the lock must propagate out of `run_bench` so `main`'s existing handler prints it to stderr and returns exit code 2 — do not catch it inside the loop's per-PR handler. +3. Pass `known_rule_ids=known_rule_ids` through to `process_pr`. + +`main` needs no change: it already resolves `--coding-repo`, rejects `--golden` with exit 2, enforces the three mandatory flags, and passes `config_dir=verify_config_dir()`. + +## 10. Extend `bench/testsupport.py` + +1. **Fix `stub_claude`.** Its current body is `printf '%s\n' '$*' >> ''` — the single quotes make `sh` write the literal two characters `$*` instead of the invocation's arguments. Change it to double quotes (`"$*"`) so each counter line records the actual argument list. Line counting still works exactly as before, and the mode-cache-miss test can now assert the mode literal reached the subprocess. +2. Add `stub_claude_failing(bin_dir, counter_file, exit_code=3)` — appends its arguments to `counter_file` exactly like `stub_claude`, writes a short message to stderr, and exits with `exit_code`. +3. Add `seed_cached_repo(cache_root, owner, repo, builder)` — creates `/repos//`, runs the given repo builder (`make_merge_repo` / `make_squash_repo` / `make_empty_diff_repo`) against it, and returns the builder's dict. This is the existing seeding idiom in `bench/test_resolve.py`, extracted so `bench/test_review.py` does not duplicate it; update `bench/test_resolve.py` only if the extraction leaves it broken. + +## 11. Create `bench/testdata/sample-report.md` + +A realistic `/coding:pr-review` Step 5 report, checked in as the fixture AC10 names. It must contain, in this order: + +1. Two or three lines of preamble prose before the first heading, mentioning at least one rule ID — these must produce zero findings. +2. `#### Must Fix (Critical)` with at least two bullets: one citing a rule ID **and** a `` `path/to/file.ext:NN` `` reference, and one citing a rule ID with **no** file reference anywhere in the bullet. +3. `#### Should Fix (Important)` with one bullet whose text continues onto an indented second line. +4. `#### Nice to Have (Optional)` whose entire body is the literal `None.`. +5. `#### Selector Mode: Classify Traceability` with two or more bullets that mention rule IDs — these must produce zero findings. + +Every rule ID written into the fixture must be an `id` that actually exists in `rules/index.json`. Do not invent IDs, and do not edit `rules/index.json`. + +## 12. Create `bench/test_review.py` + +`import json`, `import os`, `import pathlib`, `import tempfile`, `import unittest`, `from unittest import mock`, `import run`, `import testsupport`. Every test runs offline: no network, no real `claude` binary, no GitHub access. + +`invoke_review` builds its environment from `os.environ` at call time, so a test that needs the stub `claude` on `PATH` must patch the process environment for the duration of the call: + +```python +with mock.patch.dict(os.environ, {"PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}"}): + rc = run.run_bench(...) +``` + +Tests: + +1. **`test_second_run_is_cache_hit_and_invokes_zero_reviews`** (AC4) — seed two merge repos under `/repos/`, build a two-PR manifest, build a matching `.claude-verify` config dir so the preflight passes, install `stub_claude` with a counter file and a report body. Run `run_bench` twice with identical arguments, capturing stdout both times. Assert: both runs return 0; the counter file has exactly 2 lines after the first run and still exactly 2 after the second; the ledger has the same line count after both runs (2); the second run's stdout contains `cache hit` exactly twice (once per PR). +2. **`test_mode_change_is_cache_miss`** (AC5) — one-PR manifest, same harness. Run once with `mode="selector"`, then again with every argument identical except `mode="full"`. Assert: counter file has 1 line after the first run and 2 after the second; the ledger has 2 rows; the two rows' `mode` values differ; row 1's `review_command` contains `selector` and row 2's contains `full`; the counter file's second line contains `full` (proving the mode literal reached the subprocess argv, not just the row). Add an assertion message printing both `review_command` values. +3. **`test_cache_path_differs_when_only_mode_differs`** — unit-level guard on the key itself: compute two `config_hash` values differing only in `mode` and assert `cache_row_path` and `cache_raw_path` differ for both. +4. **`test_harvest_normalizes_sample_report`** (AC10) — read `pathlib.Path(run.__file__).parent / "testdata" / "sample-report.md"`, call `run.harvest(text, run.load_rule_ids(run.REPO_ROOT))`, and assert the returned list equals the full expected list of `{path, line, rule_id, body}` dicts. The expectation must include the finding whose `path` and `line` are both `None`, and must contain no entry originating from the preamble or the traceability section. Additionally assert every non-`None` `rule_id` in the result is a member of `run.load_rule_ids(run.REPO_ROOT)` — this is the boundary check against the real index. +5. **`test_harvest_keeps_finding_without_any_rule_id`** — a small inline report whose bullet cites no known ID yields one finding with `rule_id is None`, not zero findings. +6. **`test_harvest_ignores_empty_section`** — a section whose body is `None.` yields zero findings. +7. **`test_ledger_is_append_only_and_atomic`** — call `run.append_row` three times against a temp results dir. Assert the ledger has 3 lines, each parses as JSON, the first two lines are byte-identical to what they were after the second call, and no leftover temporary file remains in the directory. +8. **`test_second_runner_exits_without_touching_ledger`** — pre-create the lock file, then call `run_bench` with the full harness. Assert: return code 2 (the `BenchError` propagates to `main`'s handler — call `run.main([...])` for this test so the exit code contract is exercised end to end); stderr names the lock file path and states that another bench run is in progress; the ledger is unchanged; the stub-`claude` counter file has 0 lines. +9. **`test_row_carries_every_required_field`** — after one successful run, load the single ledger row and assert every field named in requirement 7 is present and not `None`. +10. **`test_raw_output_is_cached_verbatim`** — after one successful run, assert the file at `cache_raw_path(...)` contains the stub's report text exactly, and that the row's `raw_output_ref` resolves to that file. +11. **`test_failed_review_leaves_no_row_and_no_cache_entry`** — install `stub_claude_failing` with exit code 3. Assert: `run_bench` returns 1; the summary line for that PR contains `failed` and `exit 3`; the ledger is absent or has 0 lines; `/reviews/` contains no files; a stderr log exists under `/failures/`. +12. **`test_failed_pr_does_not_prevent_later_prs`** — a two-PR manifest where the first PR's `merge_sha` is unresolvable and the second reviews cleanly; assert the second PR still produces a row, both PRs appear in the summary, and `run_bench` returns 1. +13. **`test_corrupt_cache_row_is_treated_as_miss`** — requirement 1 states a cache file that fails to parse as JSON is treated as a miss and overwritten at the end of a successful run. Write a cache file containing malformed JSON, run, and assert the review subprocess *was* invoked (stub counter incremented), a row was appended, and the cache file now parses. Untested, a corrupt cache silently becomes either a crash or a permanent cache hit that can never be repaired without manual deletion. + +## 13. CHANGELOG + +Add bullets under the existing `## Unreleased` heading in `CHANGELOG.md` describing the review invocation, the mode-aware raw-output cache, the harvester, the append-only ledger and its single-instance lock, the new `bench/testdata/sample-report.md` fixture, and the new test file. Match the existing `bench: ...` bullet style already under that heading. Do not create a new version section and do not touch any released section. + +## 14. Do not modify + +`Makefile`, `bench/README.md`, `bench/prs.json`, `rules/`, `commands/`, `agents/`, `docs/`, `scripts/`, `specs/`. The `make bench` / `make bench-test` targets, the precommit wiring and the README rewrite are prompt 4 of this spec. + + + +- Python 3 standard library only — no `pip`, no `requirements.txt`, no `pyproject.toml`, no `setup.py`, no third-party imports. This repo is a Claude Code plugin distributed as a git clone; a packaged artifact is the wrong shape +- All new code stays in `bench/run.py`, `bench/testsupport.py`, `bench/test_review.py` and `bench/testdata/` +- Review mode is part of the configuration identity — the cache key derives from `config_hash`, which already mixes mode. Changing only `--mode` MUST be a cache miss +- A cache hit invokes zero subprocesses, does no git work, and appends no duplicate row +- Raw subprocess stdout is stored verbatim before any parsing +- Rows are append-only: never rewritten, never deleted; writes go through write-then-`os.replace` +- A second runner started against the same output directory exits without touching the ledger or the cache; the error names the lock file so a stale lock can be removed +- A PR that fails, times out, or exits non-zero produces no row and no cache entry; the remaining PRs still run; the process exits non-zero +- Do NOT add a retry loop around a failed review +- Fixed invariants, not configurable: 45-minute review timeout, cache under `bench/.cache/`, results under `bench/results/`, isolated config directory `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`. Do NOT add flags, env vars or parameters for any of them +- Every subprocess is invoked with an argument list; no manifest value is ever interpolated into a shell command +- `invoke_review` is the only `cwd=` in `bench/run.py`; the `git()` chokepoint stays `-C`-only and is not modified by this prompt +- `assert_under(worktree, repos_root(cache_root))` runs before the worktree is used as a working directory +- The runner records the command string it invoked but never copies environment variables, tokens or credential material into any artifact +- No personal paths anywhere in shipped files (`/Users/`, `~/Documents/`) — `docs/dod.md` forbids them. Reading `$HOME/.claude-verify` from `os.environ` at call time is fine +- `bench/prs.json` is a frozen input — schema, entries and `dev-1` version unchanged +- No rule, agent, command or doc that participates in a review may be edited — the measured configuration must stay exactly as it is while the measuring device is built +- All new tests run offline: no network, no real `claude` binary, no GitHub access +- `CHANGELOG.md` gains an entry under `## Unreleased` (`docs/dod.md`) +- Do NOT commit — dark-factory handles git +- Existing tests must still pass (29 today) + + + +``` +# Stdlib-only imports across every bench module +grep -nE '^(import |from )' bench/run.py bench/testsupport.py bench/test_review.py + +# No personal paths +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" + +# Exactly one cwd= in run.py, inside invoke_review +grep -n 'cwd=' bench/run.py + +# Expect exactly TWO subprocess.run calls in the file after this prompt: the pre-existing one +# inside git() (the confinement chokepoint, unchanged) and the new one inside invoke_review(). +# Any third means git invocation escaped the chokepoint — that is a failure, not a style nit. +grep -n 'subprocess.run' bench/run.py + +# The sample-report fixture exists +ls -l bench/testdata/ + +# Full unit-test run, including the new AC tests +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tee /tmp/bench-tests.log +grep -c 'cache_hit\|cache_miss' /tmp/bench-tests.log +grep -c 'harvest' /tmp/bench-tests.log +grep -c 'append_only' /tmp/bench-tests.log +grep -c 'second_runner' /tmp/bench-tests.log +tail -3 /tmp/bench-tests.log + +# Every rule ID used by the fixture really exists in the index +python3 -c " +import sys, pathlib +sys.path.insert(0, 'bench') +import run +ids = run.load_rule_ids(run.REPO_ROOT) +text = (run.BENCH_DIR / 'testdata' / 'sample-report.md').read_text() +found = run.harvest(text, ids) +print('findings:', len(found)) +for f in found: + print(' ', f) + assert f['rule_id'] is None or f['rule_id'] in ids, f +print('all cited rule ids exist in rules/index.json') +" + +# Reserved and mandatory flags unchanged +python3 bench/run.py --golden bench/golden.json ; echo "golden exit=$? (expect 2)" +python3 bench/run.py ; echo "no-flags exit=$? (expect 2)" + +# Repo checks still green +make precommit +``` + diff --git a/prompts/in-progress/014-spec-002-make-targets-and-docs.md b/prompts/in-progress/014-spec-002-make-targets-and-docs.md new file mode 100644 index 0000000..a740445 --- /dev/null +++ b/prompts/in-progress/014-spec-002-make-targets-and-docs.md @@ -0,0 +1,206 @@ +--- +status: approved +spec: [002-pr-review-bench-runner] +created: "2026-08-06T22:29:56Z" +queued: "2026-08-06T22:39:41Z" +--- + + +- One command now runs the benchmark, and one command runs its tests +- The benchmark's own tests become part of the standard pre-commit gate, so every later change to the repo has to keep the measuring instrument working +- The benchmark documentation stops claiming the runner does not exist and describes how to actually run it +- The documented rule for reconstructing a squashed pull request's diff is corrected: it always comes from the manifest's recorded start and end commits, never from walking the merge commit's parents +- The old snippet only ever looked correct because the single squashed fixture pull request happens to end on its own merge commit — that coincidence is called out so nobody restores it +- The documentation records which knobs are deliberately fixed and not configurable, so a future reader does not add flags the design rules out +- A final sweep confirms no personal filesystem paths and no third-party dependencies were introduced anywhere in the benchmark +- Generated Python bytecode is kept out of version control now that the test suite runs on every pre-commit +- The changelog records the finished runner under the unreleased section + + + +Package the finished benchmark runner: add `make bench` and `make bench-test`, wire the bench unit tests into `make precommit` so they gate every later change, rewrite `bench/README.md` to describe the shipped runner and to replace its superseded parent-derived squash snippet with the spec's authoritative `base_sha..head_sha` rule, and record the work in `CHANGELOG.md`. This is the last prompt of spec 002 and is deliberately last so the gate wired into `precommit` sees the finished tests. + + + +Read `CLAUDE.md` for project conventions (Python 3 stdlib only, no personal paths, generic examples only). +Read `specs/in-progress/002-pr-review-bench-runner.md` — this prompt satisfies Acceptance Criteria AC1, AC12 and AC13. Two sections are load-bearing here: +- **Constraints** → *"This spec is the binding contract for diff-range mechanics, superseding `bench/README.md` where the two disagree"* and *"prompt 4 (packaging/docs) corrects the README snippet to match rather than the reverse"*. That is this prompt. +- **Desired Behavior 5** → the authoritative rule you are documenting: two or more parents means `^1..^2`; exactly one parent means the manifest's recorded `base_sha..head_sha`, derived from the manifest and never from parent traversal. + +Read `Makefile` — five targets exist. `precommit` currently reads: + +```make +.PHONY: precommit +precommit: check-links check-json check-index check-coverage check-acceptance +``` + +and the existing wrapper targets are one-liners of the shape `@bash scripts/.sh` or `@python3 scripts/.py`. Match that shape. + +Read `scripts/check-coverage.sh` — the bash-wrapper precedent for a check target. +Read `scripts/build-index.py` — the stdlib-Python script precedent. + +Read `bench/README.md` — the file you rewrite. It currently contains, and must stop containing: +- `Only `prs.json` exists — the runner, golden set, and scoring are not built yet.` +- a shell snippet whose squash line is `git diff ^1..` +- a python snippet whose else-branch is `base, head = parents[0], merge_sha # squash (or rebase)` + +It also describes the configuration tuple as `(rules + commands state, model, effort level)`, which is now incomplete — review mode is part of the identity. + +Read `bench/run.py` — the shipped runner you are documenting. Take the documented CLI surface from `build_parser` (`--coding-repo`, `--manifest`, `--out-dir`, `--model`, `--effort`, `--mode` with choices from `VALID_MODES`, `--golden`, `--print-config-hash`), the exit-code contract from the module header comment (0 = every PR produced a row, 1 = one or more PRs failed, 2 = usage / manifest / preflight failure), and the fixed invariants from the module constants (`REVIEW_TIMEOUT_SECONDS`, `VERIFY_CONFIG_DIR_NAME`). Do not restate anything the code does not do. + +Read `bench/prs.json` — `node-skeleton#2` is the only `squash` entry and its `head_sha` equals its `merge_sha`; the other four are `merge-commit`. The five `changed_files` counts are 1 / 17 / 21 / 18 / 8 (`tts-mcp#20`, `github-pr-review-agent#11`, `quant#109`, `node-skeleton#2`, `python-skeleton#3`). + +Read `docs/dod.md` — no personal paths anywhere, a `## Unreleased` CHANGELOG entry, and the 4-version alignment is not touched. + +Read `.gitignore` — it already lists `/bench/results/` and `/bench/.cache/` but has no `__pycache__` entry. + + + + +## 1. Makefile — `bench` and `bench-test` + +Add two targets in the existing style (`.PHONY` declaration immediately above each recipe, `@`-prefixed one-liner recipes): + +```make +.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 +``` + +- `BENCH_ARGS` is the only variable and has no default. `--model`, `--effort` and `--mode` are mandatory precisely because a guessed default would mislabel every recorded row, so `make bench` with no `BENCH_ARGS` correctly fails with the runner's exit code 2. Do not add default values, and do not add variables for the cache directory, the results directory, the review timeout or the isolated config directory — the spec fixes all four as invariants. +- The `2>&1` on the unittest line is required: `unittest` writes its result summary (`OK` / `FAILED`) to stderr, and AC1 requires `make precommit`'s **stdout** to contain both `bench-test` and `OK`. +- `python3 -m unittest discover -s bench -p 'test_*.py'` is run from the repo root; discovery inserts `bench/` onto `sys.path`, which is what lets the test modules `import run` and `import testsupport`. Do not `cd` into `bench/` and do not add a `sys.path` shim. + +## 2. Makefile — wire the gate into `precommit` + +Append `bench-test` to the `precommit` prerequisite list, last: + +```make +precommit: check-links check-json check-index check-coverage check-acceptance bench-test +``` + +Last position keeps its `OK` line near the end of the output and keeps the existing checks' ordering untouched. Do not add `bench` (the real benchmark spends tokens and needs a `claude` binary — it must never run in a pre-commit or in CI). + +## 3. `.gitignore` — Python bytecode + +Add `__pycache__/` so the newly-precommit-run test suite does not leave untracked bytecode in the working tree. It is invisible today only because of a machine-local global ignore file; a fresh clone or container has no such file. Keep the existing entries and their order; append the new line. + +## 4. Rewrite `bench/README.md` + +Keep the file's identity: the `# bench — code-review outcome benchmark` heading, the four-row test-pyramid table, the `Goal: [[PR Review Bench]]` wikilink, the `prs.json` description, and the `gh api repos///compare/... --jq '.files | length'` verification snippet with its five recorded counts. Everything below is what changes. + +1. **Configuration tuple.** Update it to `(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. + +2. **Current state.** Replace the sentence `Only `prs.json` exists — the runner, golden set, and scoring are not built yet.` with a description of what now exists: the runner drives the real `/coding:pr-review` command over the pinned manifest and writes one row per PR. State that the golden set and the scoring semantics belong to a later spec, and that `--golden` is therefore recognised and rejected with exit code 2 rather than silently ignored. **Do not reuse the phrase `runner, golden set, and scoring are not built yet` in any form** — AC13 greps for that exact string and requires zero matches. + +3. **Running it.** A new section containing the literal `make bench`: + + ```bash + make bench BENCH_ARGS="--model --effort --mode " + make bench-test + ``` + + Document that the three flags are mandatory because they are recorded identity, that results land in `bench/results/results.jsonl`, that `make bench-test` is also wired into `make precommit`, and that `python3 bench/run.py --print-config-hash` prints the content hash a result file refers to. Note the exit-code contract: 0 when every PR produced a row, 1 when one or more PRs failed, 2 for a usage, manifest or preflight failure. + +4. **Diff-range rule — the correction.** Delete **both** stale artifacts, and delete each one's comment line together with its code line — a surviving `# squash: one parent — the squash commit IS the head` comment still reads as valid guidance even after the code beneath it is gone: + - the shell snippet's `# squash: …` comment **and** its `git diff ^1..` line + - the whole python `parents` snippet, including `base, head = parents[0], merge_sha` + + AC13 greps for `parents\[0\], merge_sha` and requires zero matches — but note that grep alone does **not** prove the shell line is gone, so `` carries a second anchored grep for it. Deleting only the python snippet while leaving the shell line is a passing-but-wrong outcome: the README would still present the parent-derived form as usable, which the spec's Constraints section forbids. Replace them with the spec's rule, stated once and unambiguously: + + - two or more parents (merge commit) → `^1..^2` + - exactly one parent (squash or rebase) → the manifest's recorded `base_sha..head_sha` + + The text must contain the literal `base_sha..head_sha` — AC13 greps for it and requires at least one match. Present this as the single rule, not as one option among alternatives; the README must not leave a reader able to choose the parent-derived form. + +5. **Why the old snippet looked right.** Add a short paragraph: the deleted parent-derived form coincided with the correct answer on the only squash entry in the fixture (`node-skeleton#2`) purely because that PR's `head_sha` equals its `merge_sha`. A coincidence on one fixture entry is not a rule, and 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. Name `specs/in-progress/002-pr-review-bench-runner.md` as the binding contract for diff-range mechanics. + +6. **Keep the empty-diff warning** already in the file and note that the runner now enforces it: a resolved range with zero changed files aborts that PR loudly with `EMPTY DIFF`, is never recorded as a zero-finding review, and produces no row and no cache entry. + +7. **Fixed invariants.** A short list stating these are deliberately not configurable: the per-PR review timeout is 45 minutes; the cache lives under `bench/.cache/` and results under `bench/results/` (both gitignored, so no benchmark output is ever committed); the isolated Claude configuration directory is `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`. State that 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 — a hash claiming content that did not run is worse than no measurement. + +8. **Safety invariant.** State that every `git` invocation the runner issues targets a path under `bench/.cache/repos/`; it 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. + +9. **Result row.** List the recorded fields: `config_hash`, `rules_commands_hash`, `model`, `effort`, `mode`, `prs_version`, `pr_id`, `base_sha`, `head_sha`, `diff_range`, `changed_files`, `review_command`, `started_at`, `duration_seconds`, `findings`, `raw_output_ref`, `runner_version`. Note that repeating a configuration is free — completed (PR, configuration) pairs are served from cache and invoke nothing — and that the cache key includes the mode, so changing only `--mode` re-runs. + +Write ``, ``, ``, `` style placeholders. No personal paths (`/Users/`, `~/Documents/`) anywhere in the file. + +## 5. CHANGELOG + +Add bullets under the existing `## Unreleased` heading in `CHANGELOG.md`, matching the existing `bench: ...` style already there. At least one bullet must name the bench runner (AC13 greps `## Unreleased` for it). Cover: the `bench` and `bench-test` make targets, the precommit wiring, the `bench/README.md` rewrite including the corrected squash diff-range rule, and the `__pycache__` gitignore entry. Do not create a new version section, do not edit any released section, and do not touch the 4-version alignment. + +## 6. Sweep — personal paths and stdlib-only (AC12) + +Run these and fix anything they surface. Do not weaken a check to make it pass. + +1. `grep -rn '/Users/\|~/Documents/' bench/` must return zero lines. `$HOME`-derived paths read from `os.environ` at call time are fine; a literal home path in a shipped file is not. +2. Every `import` / `from` line in `bench/run.py`, `bench/testsupport.py` and `bench/test_*.py` must name a Python 3 standard-library module (or the sibling modules `run` / `testsupport`). No third-party imports. +3. No `requirements.txt`, `pyproject.toml`, `setup.py`, `setup.cfg` or `Pipfile` exists anywhere in the repo — this plugin is distributed as a git clone and gains no packaging. + +## 7. Do not modify + +`bench/run.py`, `bench/testsupport.py`, `bench/test_*.py`, `bench/testdata/`, `bench/prs.json`, `rules/`, `commands/`, `agents/`, `docs/`, `scripts/`, `specs/`, `README.md`, `llms.txt`, `.claude-plugin/`. This prompt is packaging and documentation only. If `make precommit` fails because a bench test fails, report it rather than editing the test or the runner to make the gate pass. + + + +- Python 3 standard library only — no `pip`, no `requirements.txt`, no `pyproject.toml`, no `setup.py`. This repo is a Claude Code plugin distributed as a git clone; a packaged artifact is the wrong shape +- The `bench` target is a thin wrapper around `python3 bench/run.py`, consistent with the existing `check-*` targets that wrap `scripts/*.sh` and `scripts/*.py` +- The bench unit tests are wired into `precommit` so they gate every change; they must not require network, a real `claude` binary or GitHub access +- Never wire the real `bench` target into `precommit` or CI — it spends real tokens +- Fixed invariants, not configurable: 45-minute review timeout, cache under `bench/.cache/`, results under `bench/results/`, isolated config directory `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`. Do NOT add Makefile variables, flags or env vars for any of them +- `--model`, `--effort` and `--mode` stay mandatory — no defaults in the Makefile, because a guessed default would mislabel every recorded row +- **The spec supersedes `bench/README.md` where the two disagree.** The single-parent range is ALWAYS the manifest's recorded `base_sha..head_sha`, never derived by parent traversal. The README's parent-derived snippet is fully replaced, not presented as an alternative +- `bench/prs.json` is a frozen input — schema, entries and `dev-1` version unchanged +- No rule, agent, command or doc that participates in a review may be edited — the measured configuration must stay exactly as it is +- No personal paths anywhere in shipped files (`/Users/`, `~/Documents/`) — `docs/dod.md` forbids them +- Generic examples only (User, Order, Product, Customer) — no trading-domain content +- `CHANGELOG.md` gains an entry under `## Unreleased`; released sections and the 4-version alignment are untouched +- Do NOT commit — dark-factory handles git +- Existing tests must still pass + + + +``` +# AC1 — precommit is green and its output carries the bench gate +make precommit 2>&1 | tee /tmp/precommit.log ; echo "precommit exit=$? (expect 0)" +grep -c 'bench-test' /tmp/precommit.log # expect >= 1 +grep -c '^OK' /tmp/precommit.log # expect >= 1 + +# The bench-test target works standalone +make bench-test + +# BENCH_ARGS really reaches the runner (no claude, no network needed) +make bench BENCH_ARGS="--print-config-hash" + +# AC13 — documentation reflects the shipped runner +grep -n 'make bench' bench/README.md +grep -n 'runner, golden set, and scoring are not built yet' bench/README.md ; echo "stale-state grep exit=$? (expect 1)" +grep -n 'parents\[0\], merge_sha' bench/README.md ; echo "parent-derived-python grep exit=$? (expect 1)" +# The shell squash line must also be gone. Anchor on the absence of a ^2 suffix so this +# cannot false-positive against the CORRECT merge-commit line `git diff ^1..^2`. +grep -nE 'git diff \^1\.\.$' bench/README.md ; echo "parent-derived-shell grep exit=$? (expect 1)" +grep -n '# squash' bench/README.md ; echo "stale-squash-comment grep exit=$? (expect 1)" +grep -n 'base_sha\.\.head_sha' bench/README.md +grep -n -A25 '## Unreleased' CHANGELOG.md + +# AC12 — no personal paths, stdlib only, no packaging +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" +grep -nE '^(import |from )' bench/run.py bench/testsupport.py bench/test_*.py +ls requirements.txt pyproject.toml setup.py setup.cfg Pipfile 2>&1 # expect: No such file + +# Bytecode stays out of the tree +grep -n '__pycache__' .gitignore + +# Frozen manifest untouched +python3 -c " +import json +d = json.load(open('bench/prs.json')) +print(d['version'], len(d['prs']), 'entries') +" +``` + diff --git a/specs/in-progress/002-pr-review-bench-runner.md b/specs/in-progress/002-pr-review-bench-runner.md index 0f97590..d99ab8d 100644 --- a/specs/in-progress/002-pr-review-bench-runner.md +++ b/specs/in-progress/002-pr-review-bench-runner.md @@ -1,11 +1,12 @@ --- -status: prompted +status: verifying tags: - dark-factory - spec approved: "2026-08-06T20:54:14Z" generating: "2026-08-06T20:54:15Z" prompted: "2026-08-06T21:04:56Z" +verifying: "2026-08-06T22:08:44Z" branch: dark-factory/pr-review-bench-runner --- From 9987c5c83b0fb2ac799dae623e2cd0c2da8a3939 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 7 Aug 2026 09:47:56 +0200 Subject: [PATCH 4/5] 014-spec-002-make-targets-and-docs --- .gitignore | 1 + CHANGELOG.md | 13 +-- Makefile | 11 ++- bench/README.md | 81 ++++++++++++++----- .../014-spec-002-make-targets-and-docs.md | 7 +- 5 files changed, 83 insertions(+), 30 deletions(-) rename prompts/{in-progress => completed}/014-spec-002-make-targets-and-docs.md (97%) diff --git a/.gitignore b/.gitignore index 7ebeaf6..91ff772 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ /specs/log /bench/results/ /bench/.cache/ +__pycache__/ diff --git a/CHANGELOG.md b/CHANGELOG.md index dd179d8..73fa7c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,16 +10,9 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## Unreleased -- bench: add `bench/run.py` — benchmark runner entrypoint with configuration-identity core (content hashing of `rules/`+`commands/`, manifest loading/validation, plugin-resolution preflight, CLI surface with mandatory `--model`/`--effort`/`--mode`, reserved `--golden` rejection, `--print-config-hash` helper) -- bench: add `bench/testsupport.py` — shared test helpers (`make_coding_repo`, `make_verify_config_dir`, `stub_claude`, `with_path`) -- bench: add `bench/test_config.py` — 17 unit tests covering AC6, AC9, AC11 and related acceptance criteria -- bench: extend `bench/run.py` with PR resolution (fetch from manifest URL into `bench/.cache/repos/`, isolated working copies via `git worktree`), parent-count diff-range branching (`^1..^2` for merge commits, manifest `base_sha..head_sha` for single-parent commits), empty-diff abort (`EMPTY DIFF` loud failure), per-PR failure isolation, and strategy-label mismatch reporting -- bench: add `bench/test_resolve.py` — 12 unit tests covering AC2, AC3, AC7, AC8 and related acceptance criteria (`parent_count`, `empty_diff`, `git_invocation_confined_to_cache_repos`, `fetch_url`) -- bench: extend `bench/testsupport.py` with git-repo helpers (`init_git_repo`, `commit_file`, `make_merge_repo`, `make_squash_repo`, `make_empty_diff_repo`, `stub_git`, `make_manifest`) -- bench: extend `bench/run.py` with review invocation (`/coding:pr-review` via isolated `CLAUDE_CONFIG_DIR=$HOME/.claude-verify` + `DISABLE_AUTOUPDATER=1`, mode-aware raw-output cache, findings harvester normalising to `{path,line,rule_id,body}`, append-only ledger via atomic `os.replace`, single-instance `BenchLock` guard) -- bench: add `bench/test_review.py` — 13 unit tests covering AC4, AC5, AC10 and related acceptance criteria (cache-hit, mode-change-cache-miss, harvest-normalise, ledger-atomicity, second-runner-lock, failure-isolation) -- bench: add `bench/testdata/sample-report.md` — fixture for AC10 harvester verification - +- 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 diff --git a/Makefile b/Makefile index 284a39c..b8a9e63 100644 --- a/Makefile +++ b/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: diff --git a/bench/README.md b/bench/README.md index b975c46..89aaf52 100644 --- a/bench/README.md +++ b/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 ^1..^2 +## Running it -# squash: one parent — the squash commit IS the head -git diff ^1.. +```bash +make bench BENCH_ARGS="--model --effort --mode " +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 ...` 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): `^1..^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 ^1..` 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///compare/... --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. diff --git a/prompts/in-progress/014-spec-002-make-targets-and-docs.md b/prompts/completed/014-spec-002-make-targets-and-docs.md similarity index 97% rename from prompts/in-progress/014-spec-002-make-targets-and-docs.md rename to prompts/completed/014-spec-002-make-targets-and-docs.md index a740445..20a3caf 100644 --- a/prompts/in-progress/014-spec-002-make-targets-and-docs.md +++ b/prompts/completed/014-spec-002-make-targets-and-docs.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [002-pr-review-bench-runner] +summary: 'Packaged benchmark runner: added make bench and make bench-test targets wired into precommit, rewrote bench/README.md with corrected squash diff-range rule and full runner docs, added __pycache__ to .gitignore, and recorded all changes in CHANGELOG.md ## Unreleased' +execution_id: coding-bench-runner-exec-014-spec-002-make-targets-and-docs +dark-factory-version: v0.192.9 created: "2026-08-06T22:29:56Z" queued: "2026-08-06T22:39:41Z" +started: "2026-08-07T07:44:29Z" +completed: "2026-08-07T07:47:56Z" --- From 3298d5bf340247949b83dd81c60e1ca54e39c1d1 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 00:19:40 +0200 Subject: [PATCH 5/5] rename test fixture builders off the make_ prefix --- bench/test_config.py | 26 +++++++++++++------------- bench/test_resolve.py | 6 +++--- bench/test_review.py | 28 ++++++++++++++-------------- bench/testsupport.py | 4 ++-- 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/bench/test_config.py b/bench/test_config.py index c1d6f89..344c72b 100644 --- a/bench/test_config.py +++ b/bench/test_config.py @@ -19,7 +19,7 @@ def test_content_hash_ignores_git_history_and_dirty_tree(self): """Two dirs with byte-identical rules/+commands/ but different git history produce the same hash. Mutating one byte produces a different hash.""" with tempfile.TemporaryDirectory() as tmpdir: - repo_a = testsupport.make_coding_repo( + repo_a = testsupport.build_coding_repo( pathlib.Path(tmpdir) / "a", rules={"go/sample.yml": "id: go/sample\nlevel: MUST\n"}, commands={"sample.md": "# Sample\n"}, @@ -31,7 +31,7 @@ def test_content_hash_ignores_git_history_and_dirty_tree(self): (b_root / ".git" / "HEAD").write_text("ref: refs/heads/main\n", encoding="utf-8") (b_root / ".git" / "config").write_text("[core]\n", encoding="utf-8") (b_root / "junk.txt").write_text("untracked garbage\n", encoding="utf-8") - testsupport.make_coding_repo( + testsupport.build_coding_repo( b_root, rules={"go/sample.yml": "id: go/sample\nlevel: MUST\n"}, commands={"sample.md": "# Sample\n"}, @@ -55,7 +55,7 @@ def test_content_hash_ignores_git_history_and_dirty_tree(self): def test_content_hash_is_order_independent(self): """Files created in reverse order produce the same hash.""" with tempfile.TemporaryDirectory() as tmpdir: - repo_a = testsupport.make_coding_repo( + repo_a = testsupport.build_coding_repo( pathlib.Path(tmpdir) / "a", rules={"go/a.yml": "id: go/a\n", "go/b.yml": "id: go/b\n"}, commands={"x.md": "# x\n", "y.md": "# y\n"}, @@ -178,17 +178,17 @@ def test_plugin_resolution_mismatch_aborts_before_any_review(self): with tempfile.TemporaryDirectory() as td: td = pathlib.Path(td) # Two coding repos with different content - repo_a = testsupport.make_coding_repo( + repo_a = testsupport.build_coding_repo( td / "repo_a", rules={"go/a.yml": "id: go/a\n"}, commands={"a.md": "# a\n"}, ) - repo_b = testsupport.make_coding_repo( + repo_b = testsupport.build_coding_repo( td / "repo_b", rules={"go/b.yml": "id: go/b\n"}, # different content commands={"b.md": "# b\n"}, ) - cfg = testsupport.make_verify_config_dir(td / "cfg", repo_b) + cfg = testsupport.build_verify_config_dir(td / "cfg", repo_b) bin_dir = td / "bin" counter = td / "counter" @@ -223,8 +223,8 @@ def test_plugin_resolution_honors_install_location(self): with tempfile.TemporaryDirectory() as td: td = pathlib.Path(td) - plugin_src = testsupport.make_coding_repo(td / "src") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "src") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) resolved = run.resolve_plugin_path(cfg) self.assertEqual(resolved, plugin_src) @@ -236,8 +236,8 @@ def test_plugin_resolution_falls_back_to_marketplaces_dir(self): with tempfile.TemporaryDirectory() as td: td = pathlib.Path(td) - plugin_src = testsupport.make_coding_repo(td / "src") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "src") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=False) resolved = run.resolve_plugin_path(cfg) expected = cfg / "plugins" / "marketplaces" / "coding" @@ -299,7 +299,7 @@ def test_print_config_hash_matches_content_hash(self): import pathlib with tempfile.TemporaryDirectory() as td: td = pathlib.Path(td) - repo = testsupport.make_coding_repo(td / "repo") + repo = testsupport.build_coding_repo(td / "repo") result = subprocess.run( [sys.executable, str(run.BENCH_DIR / "run.py"), "--print-config-hash", "--coding-repo", str(repo)], @@ -326,8 +326,8 @@ def test_missing_model_and_effort_flags_exit_two(self): """ with tempfile.TemporaryDirectory() as tmpdir: # Set up an isolated config dir whose plugin matches --coding-repo - plugin_src = testsupport.make_coding_repo(pathlib.Path(tmpdir) / "repo") - cfg = testsupport.make_verify_config_dir( + plugin_src = testsupport.build_coding_repo(pathlib.Path(tmpdir) / "repo") + cfg = testsupport.build_verify_config_dir( pathlib.Path(tmpdir) / "cfg", plugin_src, use_known_marketplaces=True ) for flag in ["--model", "--effort"]: diff --git a/bench/test_resolve.py b/bench/test_resolve.py index 1cd56d7..944a4f4 100644 --- a/bench/test_resolve.py +++ b/bench/test_resolve.py @@ -268,8 +268,8 @@ def test_git_invocation_confined_to_cache_repos(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) # Save and modify os.environ to put stub_git on PATH @@ -398,7 +398,7 @@ def test_second_pr_runs_after_first_fails(self): cfg_dir = td / ".claude-verify" cfg_dir.mkdir(parents=True) plugin_dest = td / "repo" - testsupport.make_coding_repo(plugin_dest) + testsupport.build_coding_repo(plugin_dest) (cfg_dir / "plugins").mkdir(parents=True) import json km = { diff --git a/bench/test_review.py b/bench/test_review.py index d7ec0c0..2de4466 100644 --- a/bench/test_review.py +++ b/bench/test_review.py @@ -67,8 +67,8 @@ def test_second_run_is_cache_hit_and_invokes_zero_reviews(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) # First run @@ -156,8 +156,8 @@ def test_mode_change_is_cache_miss(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) # First run: selector mode @@ -424,8 +424,8 @@ def test_row_carries_every_required_field(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) with mock.patch.dict(os.environ, env): @@ -500,8 +500,8 @@ def test_raw_output_is_cached_verbatim(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) with mock.patch.dict(os.environ, env): @@ -572,8 +572,8 @@ def test_failed_review_leaves_no_row_and_no_cache_entry(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) with mock.patch.dict(os.environ, env): @@ -663,8 +663,8 @@ def test_failed_pr_does_not_prevent_later_prs(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) with mock.patch.dict(os.environ, env): @@ -725,8 +725,8 @@ def test_corrupt_cache_row_is_treated_as_miss(self): manifest_path = td / "manifest.json" testsupport.make_manifest(manifest_path, manifest_entries) - plugin_src = testsupport.make_coding_repo(td / "repo") - cfg = testsupport.make_verify_config_dir(td / "cfg", plugin_src, + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, use_known_marketplaces=True) # Compute cfg_hash to know where to write corrupt cache diff --git a/bench/testsupport.py b/bench/testsupport.py index bdcddf4..2571d90 100755 --- a/bench/testsupport.py +++ b/bench/testsupport.py @@ -12,7 +12,7 @@ import subprocess -def make_coding_repo(root: pathlib.Path, *, rules=None, commands=None) -> pathlib.Path: +def build_coding_repo(root: pathlib.Path, *, rules=None, commands=None) -> pathlib.Path: """Create a temporary coding-repo structure under root. Creates root/"rules" and root/"commands" directories and writes the given @@ -39,7 +39,7 @@ def make_coding_repo(root: pathlib.Path, *, rules=None, commands=None) -> pathli return root -def make_verify_config_dir(root: pathlib.Path, plugin_src: pathlib.Path, +def build_verify_config_dir(root: pathlib.Path, plugin_src: pathlib.Path, *, use_known_marketplaces: bool = False) -> pathlib.Path: """Create an isolated .claude-verify directory under root.