From 4a7d8fe5b913a77dcbd1bd93db5ba30f6aa80532 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Thu, 27 Aug 2026 17:32:53 +0800 Subject: [PATCH] eval: monthly review-quality probe of omni-reviewbot as a harness arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One command — `bash eval/dataset/run_reviewbot_monthly.sh 2026-09` — generates fresh omni-reviewbot Direct reviews over the dataset's train+val PR items (15; test stays frozen) and judges them blind against the pinned claudecode_opus5 baseline with the pinned claude-sonnet-5 judge, 3 generation x 3 judge replicates, reported with the campaign's paired-within-verdict, item-clustered statistics. The bot is a loop+model combination: results live in results/reviewbot/ and are never merged into model_comparison.md. run_reviewbot_arm.py enforces the measurement contract per invocation, fail-closed: POST_MODE=shadow asserted from CLI output (nothing can reach GitHub); REVIEW_CONTEXT_MODE=no_discussion asserted via the bot's printed provenance line (on these PRs the historical review threads ARE the ground truth); reviewed head pinned to expected_pr_heads.json and changed_files matched to the frozen GT diff; sanitized artifacts (markers stripped, neutral heading — labels must not reach the judge) must fit the judge's 24k cap. Replication has one owner (the runner: ARM_TAG -> arms/_r1..rN), manifests record resolved config and refuse to mix a changed bot into a half-generated arm, and the cursor provider is refused without the contamination-vault ack. build_reviewbot_report.py --verify audits exact denominators before any statistics: judge_val.py skips existing verdicts and succeeds on empty job sets, so the verifier independently checks {manifest stems} x {judge reps} per replicate dir, pinned judge identity, and each verdict's arm/baseline sha256 against the CURRENT capped candidate texts — stale judgments fail here, not in the numbers. Reports are written atomically; a tag upserts one row in results/reviewbot/INDEX.md (rerun replaces, never duplicates); smoke tags never touch the series. Companion: omni-reviewbot#20 (REVIEW_CONTEXT_MODE + printed provenance). Offline coverage in test/test_reviewbot_eval.py (10 cases). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PchwdRpCaWWiRbJDVHKUiD --- eval/dataset/REVIEWBOT_EVAL.md | 70 +++++ eval/dataset/build_reviewbot_report.py | 400 +++++++++++++++++++++++++ eval/dataset/run_reviewbot_arm.py | 398 ++++++++++++++++++++++++ eval/dataset/run_reviewbot_monthly.sh | 40 +++ test/test_reviewbot_eval.py | 337 +++++++++++++++++++++ 5 files changed, 1245 insertions(+) create mode 100644 eval/dataset/REVIEWBOT_EVAL.md create mode 100755 eval/dataset/build_reviewbot_report.py create mode 100755 eval/dataset/run_reviewbot_arm.py create mode 100755 eval/dataset/run_reviewbot_monthly.sh create mode 100644 test/test_reviewbot_eval.py diff --git a/eval/dataset/REVIEWBOT_EVAL.md b/eval/dataset/REVIEWBOT_EVAL.md new file mode 100644 index 00000000..6fadd827 --- /dev/null +++ b/eval/dataset/REVIEWBOT_EVAL.md @@ -0,0 +1,70 @@ +# Reviewbot monthly review-quality probe + +Judges the **live omni-reviewbot configuration** (its Direct pipeline: +Codex + InferMatrix knowledge routes + its own gates) against the pinned +`baselines/claudecode_opus5`, on the dataset's **train+val PR items** +(15; test stays frozen), with the pinned `claude-sonnet-5` judge and the +campaign's paired/clustered statistics. + +**These are loop+model numbers.** They live in `results/reviewbot/` and +are never merged into `results/model_comparison.md` (generator ablations +ride our own pipeline and swap only the model — 2026-08-14 decision). + +## The one command + +```bash +bash eval/dataset/run_reviewbot_monthly.sh 2026-09 +``` + +First time on a machine, smoke one item end-to-end first: + +```bash +ONLY_ITEMS=4893 GEN_REPLICATES=1 bash eval/dataset/run_reviewbot_monthly.sh smoke +``` + +A non-month tag writes `results/reviewbot/REVIEWBOT_smoke.md` and never +touches the month-over-month `INDEX.md`. + +## Prerequisites + +- An installed omni-reviewbot checkout (default: the sibling + `omni-reviewbot/` next to this repo; override `REVIEWBOT_DIR`) with its + `.venv` and `.env` set up, `AGENT_PROVIDER=codex` logged in, and a bot + build that has `REVIEW_CONTEXT_MODE` (PR #20+). +- The `claude` CLI logged in (judge, native auth). +- Cost: generation rides the Codex subscription, judging rides the + Claude subscription — **zero API-key spend expected**. 15 items ×3 + generation ×3 judge ≈ 45 reviews + 135 judge calls; a few hours + wall-clock at `ARM_JOBS=2`. + +## What the harness enforces (all fail-closed, per item) + +- `POST_MODE=shadow` asserted from CLI output — nothing can reach GitHub. +- `REVIEW_CONTEXT_MODE=no_discussion` asserted (`review_context: + no_discussion (0 threads)`): on these PRs the historical review + threads ARE the ground truth; reading them measures leakage. +- Reviewed head == `goal-eval/expected_pr_heads.json`, and the bot's + `changed_files` == the frozen GT diff's file count. +- Sanitized artifacts (markers stripped, neutral heading) must fit the + judge's 24k cap — over-cap is a campaign failure, not a silent + truncation. +- Before reporting, `build_reviewbot_report.py --verify` audits exact + denominators, pinned judge identity, and fresh candidate sha256s in + every verdict (judge_val skips existing files, so its exit code is not + a completeness proof). + +## On partial failure + +Everything is resumable: rerun the same command. Generated arm files and +existing verdicts are skipped; the manifest refuses to mix a changed bot +configuration into a half-generated arm (delete the `arms/_r*` dirs +to restart a month from scratch). + +## Reading the report — three inherited rules + +1. Compare only the **paired** deltas (arm − baseline inside one + verdict); raw side means drift ±.08 across judge batches. +2. Replicates of one item are not independent: CIs are over **items** + (n=15), which resolves ≈.06 — treat smaller movements as noise. +3. Months are comparable only under the same judge model and the same + pinned baseline; both are recorded per row in `INDEX.md`. diff --git a/eval/dataset/build_reviewbot_report.py b/eval/dataset/build_reviewbot_report.py new file mode 100755 index 00000000..e917dc0a --- /dev/null +++ b/eval/dataset/build_reviewbot_report.py @@ -0,0 +1,400 @@ +#!/usr/bin/env python3 +"""Verify and report a reviewbot judgment campaign. + +`--verify` runs the exact-denominator audit (judge_val.py skips existing +verdict files unconditionally and succeeds on an empty job set, so its +exit code proves nothing about completeness): + + per replicate dir, verdict files are exactly {arm-manifest stems} x + {judge reps}; every file parses; `_roles` names this arm and the pinned + baseline; the recorded judge backend/model equal the pinned + claude/claude-sonnet-5; and each verdict's arm_a/arm_b sha256 matches + the sha256 of the CURRENT candidate texts (first 24k, exactly as the + judge read them) — a stale verdict left over from a changed artifact or + baseline fails here, not in the statistics. + +Without `--verify` it builds the report: paired-within-verdict deltas +(the ONLY honest comparison — raw side means drift +-.08 across judge +batches), clustered by item (replicates of one item are not independent +observations), pooled over generation replicates. Output is written +atomically; a `reviewbot_` tag also upserts one row in +results/reviewbot/INDEX.md (rerunning a month replaces its row; any +other tag, e.g. a smoke run, never touches INDEX). + +These numbers are a MONITORING PROBE of the live bot configuration, +never a promotion gate, and never comparable with the generator +ablation tables in results/model_comparison.md (the bot is loop+model). + +Usage: build_reviewbot_report.py [--verify] --tag reviewbot_ + [--gen-reps 3] [--judge-reps 3] +""" +from __future__ import annotations + +import argparse +import hashlib +import json +import math +import os +import re +import statistics as st +import sys +import tempfile +from collections import defaultdict +from pathlib import Path + +DS = Path(os.environ.get("REVIEWBOT_EVAL_ROOT") or Path(__file__).parent) +BASELINE = "claudecode_opus5" +JUDGE_BACKEND = "claude" +JUDGE_MODEL = "claude-sonnet-5" +CAP = 24_000 +DIMS = ("recall", "precision", "actionability") +# GOLD latent-gap items (one per split; train+val scope carries two). +GOLD_STEMS = {"pr4870", "pr4810", "pr4834"} +MONTH_TAG = re.compile(r"^reviewbot_(\d{4}-\d{2})$") + +# Two-sided t .975 critical values, df 1..30 (as in paired_analysis.py). +_T95 = [12.71, 4.30, 3.18, 2.78, 2.57, 2.45, 2.36, 2.31, 2.26, 2.23, + 2.20, 2.18, 2.16, 2.14, 2.13, 2.12, 2.11, 2.10, 2.09, 2.09, + 2.08, 2.07, 2.07, 2.06, 2.06, 2.06, 2.05, 2.05, 2.05, 2.04] + + +def t95(df: int) -> float: + if df < 1: + return float("nan") + return _T95[df - 1] if df <= 30 else 1.96 + + +def _sha(path: Path) -> str: + return hashlib.sha256( + path.read_text()[:CAP].encode() + ).hexdigest() + + +class CampaignError(RuntimeError): + pass + + +def load_campaign(tag: str, gen_reps: int, judge_reps: int): + """Verify everything, return (verdicts, stems, config). + + verdicts: list of (stem, gen_rep, judge_rep, verdict_dict, arm_side) + where arm_side is "x" or "y" — the slot holding the reviewbot text. + """ + problems: list[str] = [] + verdicts = [] + stems: list[str] | None = None + config = None + for rep in range(1, gen_reps + 1): + arm_dir = DS / "arms" / f"{tag}_r{rep}" + judge_dir = DS / "judgments" / f"{tag}_r{rep}" + manifest_path = arm_dir / "manifest.json" + if not manifest_path.is_file(): + problems.append(f"{manifest_path} missing") + continue + manifest = json.loads(manifest_path.read_text()) + if stems is None: + stems = list(manifest["stems"]) + elif manifest["stems"] != stems: + problems.append(f"r{rep}: stems differ from r1 — mixed campaign") + if config is None: + config = manifest.get("config") + elif manifest.get("config") != config: + problems.append(f"r{rep}: config differs from r1 — mixed campaign") + expected = {f"{s}.r{k}.json" for s in stems for k in + range(1, judge_reps + 1)} + actual = {p.name for p in judge_dir.glob("*.r*.json")} + for name in sorted(expected - actual): + problems.append(f"{judge_dir.name}: MISSING {name}") + for name in sorted(actual - expected): + problems.append(f"{judge_dir.name}: EXTRA {name}") + for stem in stems: + arm_file = arm_dir / f"{stem}.md" + base_file = DS / "baselines" / BASELINE / f"{stem}.md" + for k in range(1, judge_reps + 1): + vf = judge_dir / f"{stem}.r{k}.json" + if not vf.is_file(): + continue + try: + v = json.loads(vf.read_text()) + except json.JSONDecodeError as exc: + problems.append(f"{vf.name}: unparseable ({exc})") + continue + roles = v.get("_roles") or {} + if roles.get("arm") != f"{tag}_r{rep}" or ( + roles.get("baseline") != BASELINE + ): + problems.append(f"{vf.name}: roles {roles} != " + f"({tag}_r{rep}, {BASELINE})") + continue + meta = v.get("_arm_meta") or {} + if meta.get("judge_backend") != JUDGE_BACKEND or ( + meta.get("judge_model") != JUDGE_MODEL + ): + problems.append( + f"{vf.name}: judge {meta.get('judge_backend')}/" + f"{meta.get('judge_model')} is not the pinned " + f"{JUDGE_BACKEND}/{JUDGE_MODEL}" + ) + continue + if meta.get("arm_a_sha256") != _sha(arm_file): + problems.append(f"{vf.name}: STALE — arm text changed " + "since this verdict") + continue + if meta.get("arm_b_sha256") != _sha(base_file): + problems.append(f"{vf.name}: STALE — baseline text " + "changed since this verdict") + continue + # Schema, fail-closed: a malformed verdict must abort the + # campaign, never silently shrink a denominator or default + # the arm onto side Y. + blinding = v.get("_blinding") or {} + labels = {blinding.get("X"), blinding.get("Y")} + if labels != {f"{tag}_r{rep}", BASELINE}: + problems.append( + f"{vf.name}: blinding {blinding} does not map X/Y " + f"onto ({tag}_r{rep}, {BASELINE})" + ) + continue + schema_bad = False + for side in ("x", "y"): + scores = v.get(side) + if not isinstance(scores, dict): + problems.append(f"{vf.name}: side {side} missing") + schema_bad = True + continue + for dim in DIMS: + value = scores.get(dim) + if not isinstance(value, (int, float)) or not ( + 0.0 <= float(value) <= 1.0 + ): + problems.append( + f"{vf.name}: {side}.{dim}={value!r} is not " + "a score in [0,1]" + ) + schema_bad = True + if not isinstance(scores.get("gap_hit"), bool): + problems.append( + f"{vf.name}: {side}.gap_hit missing or not a " + "bool" + ) + schema_bad = True + if v.get("winner") not in {"X", "Y", "tie"}: + problems.append( + f"{vf.name}: winner={v.get('winner')!r} invalid" + ) + schema_bad = True + if schema_bad: + continue + arm_side = ( + "x" if blinding.get("X") == f"{tag}_r{rep}" else "y" + ) + verdicts.append((stem, rep, k, v, arm_side)) + if problems: + raise CampaignError( + "campaign verification FAILED — do not report:\n " + + "\n ".join(problems) + ) + if not verdicts or stems is None: + raise CampaignError("no verdicts found") + return verdicts, stems, config or {} + + +def analyze(verdicts, stems): + """Paired within verdict, clustered by item.""" + per_item: dict[str, dict[str, list[float]]] = defaultdict( + lambda: defaultdict(list) + ) + wins = {"arm": 0, "baseline": 0, "tie": 0} + gap = defaultdict(lambda: {"arm": 0, "baseline": 0, "n": 0}) + for stem, _rep, _k, v, arm_side in verdicts: + base_side = "y" if arm_side == "x" else "x" + arm_scores, base_scores = v.get(arm_side) or {}, v.get(base_side) or {} + for dim in DIMS: + if dim in arm_scores and dim in base_scores: + per_item[stem][dim].append( + float(arm_scores[dim]) - float(base_scores[dim]) + ) + winner = v.get("winner") + slot = {"X": "x", "Y": "y"}.get(winner) + if slot is None: + wins["tie"] += 1 + else: + wins["arm" if slot == arm_side else "baseline"] += 1 + if stem in GOLD_STEMS: + entry = gap[stem] + entry["n"] += 1 + entry["arm"] += bool(arm_scores.get("gap_hit")) + entry["baseline"] += bool(base_scores.get("gap_hit")) + aggregate = {} + item_rows = {} + for dim in DIMS: + means = [ + st.mean(per_item[stem][dim]) + for stem in stems + if per_item[stem][dim] + ] + n = len(means) + mean = st.mean(means) if means else float("nan") + if n >= 2: + half = t95(n - 1) * st.stdev(means) / math.sqrt(n) + else: + half = float("nan") + aggregate[dim] = {"delta": mean, "ci": half, "n": n, + "pos": sum(1 for m in means if m > 0), + "neg": sum(1 for m in means if m < 0)} + for stem in stems: + item_rows[stem] = { + dim: (st.mean(per_item[stem][dim]) + if per_item[stem][dim] else None) + for dim in DIMS + } | {"n": len(per_item[stem][DIMS[0]])} + # Every judged GOLD item is reported, a 0/n double miss included — + # hiding it would overstate latent-gap coverage. + gap_rows = {stem: entry for stem, entry in gap.items() if entry["n"]} + return aggregate, item_rows, wins, gap_rows + + +def render(tag, aggregate, item_rows, wins, gap_rows, config, + gen_reps, judge_reps): + stems = list(item_rows) + lines = [ + f"# Reviewbot review-quality probe — `{tag}`", + "", + "> **Monitoring probe, not a gate.** n=" + f"{aggregate[DIMS[0]]['n']} items resolves ≈.06 at the measured " + "item sd; months are comparable only under the same judge model " + "and the same pinned baseline; these numbers are loop+model and " + "are NEVER merged into `results/model_comparison.md`.", + "", + f"- Arm: omni-reviewbot Direct pipeline, bot `" + f"{config.get('bot_sha', 'unknown')[:12]}`, infermatrix `" + f"{config.get('infermatrix_sha', 'unknown')[:12]}`, provider " + f"`{(config.get('env') or {}).get('AGENT_PROVIDER', 'codex')}`, " + f"`REVIEW_CONTEXT_MODE={config.get('review_context_mode')}`", + f"- Baseline: pinned `{BASELINE}` · Judge: pinned " + f"`{JUDGE_BACKEND}/{JUDGE_MODEL}` · {gen_reps} generation × " + f"{judge_reps} judge replicates", + "", + "## Paired deltas (arm − baseline, clustered by item)", + "", + "| dim | Δ | 95% CI | items +/− | n |", + "|---|---|---|---|---|", + ] + for dim in DIMS: + a = aggregate[dim] + lines.append( + f"| {dim} | {a['delta']:+.3f} | ±{a['ci']:.3f} | " + f"{a['pos']}/{a['neg']} | {a['n']} |" + ) + total = sum(wins.values()) + lines += [ + "", + f"**Verdict wins** (least informative statistic — margin is " + f"discarded): arm {wins['arm']} · baseline {wins['baseline']} · " + f"tie {wins['tie']} (of {total})", + "", + "## Per item (mean paired delta)", + "", + "| item | Δrecall | Δprecision | Δactionability | verdicts |", + "|---|---|---|---|---|", + ] + for stem in stems: + row = item_rows[stem] + + def fmt(value): + return f"{value:+.3f}" if value is not None else "—" + + lines.append( + f"| {stem} | {fmt(row['recall'])} | {fmt(row['precision'])} | " + f"{fmt(row['actionability'])} | {row['n']} |" + ) + if gap_rows: + lines += ["", "## GOLD latent-gap items", ""] + for stem, entry in sorted(gap_rows.items()): + lines.append( + f"- {stem}: arm hit {entry['arm']}/{entry['n']}, " + f"baseline {entry['baseline']}/{entry['n']} (1-item " + "measurement — a direction, not a number)" + ) + return "\n".join(lines) + "\n" + + +def write_atomic(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp(dir=path.parent, suffix=".tmp") + with os.fdopen(fd, "w") as handle: + handle.write(text) + os.replace(tmp, path) + + +def upsert_index(month: str, aggregate, wins, config) -> None: + index = DS / "results" / "reviewbot" / "INDEX.md" + header = [ + "# Reviewbot monthly probe series", + "", + "One row per month; rerunning a month replaces its row. " + "Loop+model numbers — never merge into model_comparison.md.", + "", + "| month | Δrecall | Δprecision | Δactionability | W-T-L | " + "bot | judge |", + "|---|---|---|---|---|---|---|", + ] + row = ( + f"| {month} | {aggregate['recall']['delta']:+.3f} " + f"±{aggregate['recall']['ci']:.3f} " + f"| {aggregate['precision']['delta']:+.3f} " + f"| {aggregate['actionability']['delta']:+.3f} " + f"| {wins['arm']}-{wins['tie']}-{wins['baseline']} " + f"| {config.get('bot_sha', 'unknown')[:12]} | {JUDGE_MODEL} |" + ) + rows: dict[str, str] = {} + if index.is_file(): + for line in index.read_text().splitlines(): + matched = re.match(r"^\| (\d{4}-\d{2}) \|", line) + if matched: + rows[matched.group(1)] = line + rows[month] = row + body = header + [rows[key] for key in sorted(rows)] + write_atomic(index, "\n".join(body) + "\n") + + +def main() -> int: + parser = argparse.ArgumentParser() + parser.add_argument("--tag", required=True) + parser.add_argument("--gen-reps", type=int, default=3) + parser.add_argument("--judge-reps", type=int, default=3) + parser.add_argument("--verify", action="store_true") + args = parser.parse_args() + + verdicts, stems, config = load_campaign( + args.tag, args.gen_reps, args.judge_reps + ) + if args.verify: + print( + f"[verify] ok: {len(verdicts)} verdicts = {len(stems)} items × " + f"{args.gen_reps} gen × {args.judge_reps} judge replicates, " + "roles/judge/sha all pinned" + ) + return 0 + aggregate, item_rows, wins, gap_rows = analyze(verdicts, stems) + report = render(args.tag, aggregate, item_rows, wins, gap_rows, + config, args.gen_reps, args.judge_reps) + matched = MONTH_TAG.match(args.tag) + name = matched.group(1) if matched else args.tag + out = DS / "results" / "reviewbot" / f"REVIEWBOT_{name}.md" + write_atomic(out, report) + print(f"[report] {out}") + if matched: + upsert_index(matched.group(1), aggregate, wins, config) + print(f"[report] INDEX.md upserted for {matched.group(1)}") + else: + print("[report] non-month tag — INDEX.md untouched") + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except CampaignError as exc: + print(exc, file=sys.stderr) + raise SystemExit(2) from exc diff --git a/eval/dataset/run_reviewbot_arm.py b/eval/dataset/run_reviewbot_arm.py new file mode 100755 index 00000000..8ff57a90 --- /dev/null +++ b/eval/dataset/run_reviewbot_arm.py @@ -0,0 +1,398 @@ +#!/usr/bin/env python3 +"""Reviewbot arm: generate omni-reviewbot Direct reviews over dataset PRs. + +The bot is a loop+model combination (harness arm, like the cursor arm): +its results live in results/reviewbot/ and are never merged into the +generator ablation tables. Scope is the dataset's train+val pr_review +items only — test stays frozen. + +Measurement contract enforced per invocation, all fail-closed: + - POST_MODE=shadow hard-set and `status: shadow` asserted in CLI output + (a review that reached a publish path aborts the campaign); + - REVIEW_CONTEXT_MODE=no_discussion hard-set and the CLI's + `review_context: no_discussion (0 threads)` line asserted — on these + PRs the historical review threads ARE the ground truth; + - reviewed head (from the artifact name pr--.md) must equal + goal-eval/expected_pr_heads.json[]; + - the CLI's `changed_files: N` must match the frozen GT diff's file + count (gt/pr.diff); + - the sanitized artifact must fit judge_val.py's 24k candidate cap. + +Replication has ONE owner: this runner. ARM_TAG= creates +arms/_r1..rN (GEN_REPLICATES, default 3); replicates of one item run +sequentially (the bot's artifact name repeats per head, and each artifact +is moved out immediately), items run with small parallelism (ARM_JOBS). + +Env: ARM_TAG (required) · GEN_REPLICATES=3 · ONLY_ITEMS=4893,4810 · +ARM_JOBS=2 · REVIEWBOT_DIR (default: sibling omni-reviewbot checkout) · +REVIEWBOT_PYTHON (default /.venv/bin/python, else python3) · +REVIEWBOT_ENV_FILE (default /.env; loaded first, hard overrides win) · +REVIEWBOT_TIMEOUT_S=1800 · REVIEWBOT_EVAL_ROOT (tests only) +Flags: --dry-run (print the plan, touch nothing) · --preflight (doctor only) +""" +from __future__ import annotations + +import json +import os +import re +import shutil +import subprocess +import sys +import threading +import time +from concurrent.futures import ThreadPoolExecutor, as_completed +from pathlib import Path + +import yaml + +HERE = Path(os.environ.get("REVIEWBOT_EVAL_ROOT") or Path(__file__).parent) +DATASET = HERE / "vllm_omni_dataset.yaml" +EXPECTED_HEADS = HERE / "goal-eval" / "expected_pr_heads.json" +GT = HERE / "gt" +ARMS = HERE / "arms" +STATE_DIR = HERE.parent / "raw" / "reviewbot_state" +JUDGE_CAP = 24_000 # judge_val.py silently truncates candidates here + +_MARKER = re.compile(r"", re.DOTALL) +_CONTEXT_LINE = re.compile(r"^review_context: (\S+) \((\d+) threads\)$", re.M) +_CHANGED_LINE = re.compile(r"^changed_files: (\d+)$", re.M) +_STATUS_LINE = re.compile(r"^status: (\S+)$", re.M) + + +def sanitize(body: str) -> str: + """Blind judging: explicit arm labels must not reach the judge.""" + body = _MARKER.sub("", body) + body = body.replace("## Omni ReviewBot review", "## Review", 1) + return body.strip() + "\n" + + +def gt_changed_files(pr: int) -> int: + text = (GT / f"pr{pr}.diff").read_text(encoding="utf-8", errors="replace") + return sum( + 1 for line in text.splitlines() if line.startswith("diff --git ") + ) + + +def dataset_items() -> list[int]: + data = yaml.safe_load(DATASET.read_text()) + items = [ + int(entry["pr"]) + for entry in data["pr_review"] + if entry.get("split") in {"train", "val"} + ] + only = os.environ.get("ONLY_ITEMS", "").strip() + if only: + keep = {int(x) for x in only.split(",") if x.strip()} + items = [n for n in items if n in keep] + return sorted(items) + + +# Child-env keys that change what the Direct review actually does; they are +# part of the arm's identity (secrets are deliberately NOT in this list and +# never reach a manifest). +_BEHAVIOR_KEYS = ( + "AGENT_PROVIDER", "REVIEW_MODEL", "CURSOR_MODEL", "CODEX_COMMAND", + "CODEX_TIMEOUT_SECONDS", "CURSOR_TIMEOUT_SECONDS", + "GITHUB_REPOSITORY", "INFERMATRIX_PATH", +) + + +def git_state(path: Path) -> tuple[str, bool]: + """(sha, dirty) for a checkout; ("unknown", True) when not a repo.""" + try: + sha = subprocess.run( + ["git", "-C", str(path), "rev-parse", "HEAD"], + capture_output=True, text=True, timeout=30, check=True, + ).stdout.strip() + dirty = bool(subprocess.run( + ["git", "-C", str(path), "status", "--porcelain"], + capture_output=True, text=True, timeout=60, check=True, + ).stdout.strip()) + return sha, dirty + except Exception: + return "unknown", True + + +def build_config(child_env: dict[str, str], bot_dir: Path) -> dict: + """The arm's identity: bot revision, InferMatrix revision (its knowledge + tree feeds the Direct routes), and the behavior-affecting env. A dirty + or unknown source state is refused — a resumed campaign must never mix + outputs from different implementations behind one manifest.""" + bot_sha, bot_dirty = git_state(bot_dir) + im_path = child_env.get("INFERMATRIX_PATH", "") + im_sha, im_dirty = ( + git_state(Path(im_path)) if im_path else ("unset", True) + ) + if (bot_dirty or bot_sha == "unknown" or im_dirty) and not os.environ.get( + "REVIEWBOT_EVAL_ALLOW_DIRTY" + ): + raise SystemExit( + f"source state is not clean/pinned (bot {bot_sha[:12]} " + f"dirty={bot_dirty}, infermatrix {im_sha[:12]} " + f"dirty={im_dirty}) — commit or stash first, or set " + "REVIEWBOT_EVAL_ALLOW_DIRTY=1 for a throwaway run" + ) + return { + "bot_sha": bot_sha, + "bot_dirty": bot_dirty, + "infermatrix_sha": im_sha, + "infermatrix_dirty": im_dirty, + "review_context_mode": "no_discussion", + "post_mode": "shadow", + "env": {key: child_env.get(key, "") for key in _BEHAVIOR_KEYS}, + } + + +def _parse_env_file(path: Path) -> dict[str, str]: + values: dict[str, str] = {} + if not path.is_file(): + return values + for line in path.read_text().splitlines(): + line = line.strip() + if not line or line.startswith("#") or "=" not in line: + continue + key, _, value = line.partition("=") + values[key.strip()] = value.strip().strip("'\"") + return values + + +class Runner: + def __init__(self) -> None: + self.tag = os.environ.get("ARM_TAG", "").strip() + if not self.tag: + sys.exit("ARM_TAG is required (e.g. ARM_TAG=reviewbot_2026-09)") + self.gen_reps = int(os.environ.get("GEN_REPLICATES", "3")) + self.jobs = int(os.environ.get("ARM_JOBS", "2")) + self.timeout = int(os.environ.get("REVIEWBOT_TIMEOUT_S", "1800")) + self.bot_dir = Path( + os.environ.get("REVIEWBOT_DIR") + or HERE.parent.parent.parent / "omni-reviewbot" + ).resolve() + venv_python = self.bot_dir / ".venv" / "bin" / "python" + self.python = os.environ.get("REVIEWBOT_PYTHON") or ( + str(venv_python) if venv_python.exists() else "python3" + ) + env_file = Path( + os.environ.get("REVIEWBOT_ENV_FILE") or self.bot_dir / ".env" + ) + # The bot's .env first, hard overrides LAST — the measurement + # contract must win over whatever the operator's file says. + child = dict(os.environ) + child.update(_parse_env_file(env_file)) + child.update( + { + "POST_MODE": "shadow", + "REVIEW_CONTEXT_MODE": "no_discussion", + "REVIEWBOT_STATE_DIR": str(STATE_DIR), + "PYTHONPATH": str(self.bot_dir / "src"), + } + ) + self.child_env = child + self.expected = { + int(k): v for k, v in json.loads(EXPECTED_HEADS.read_text()).items() + } + self.items = dataset_items() + self.failures: list[str] = [] + self._lock = threading.Lock() + self.config: dict | None = None + + def ensure_config(self) -> dict: + if self.config is None: + self.config = build_config(self.child_env, self.bot_dir) + return self.config + + # --- manifests --- + + def _arm_dir(self, rep: int) -> Path: + return ARMS / f"{self.tag}_r{rep}" + + def _manifest_path(self, rep: int) -> Path: + return self._arm_dir(rep) / "manifest.json" + + def _init_manifest(self, rep: int) -> None: + path = self._manifest_path(rep) + config = self.ensure_config() + if path.exists(): + stored = json.loads(path.read_text()) + if stored.get("config") != config: + sys.exit( + f"{path}: existing manifest was generated under a " + f"different configuration — refusing to mix.\n" + f"stored: {stored.get('config')}\n" + f"current: {config}" + ) + if stored.get("stems") != [f"pr{n}" for n in self.items]: + sys.exit( + f"{path}: existing manifest targets different items — " + "refusing to mix (delete the arm dir to restart)." + ) + return + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + json.dumps( + { + "arm_tag": self.tag, + "replicate": rep, + "stems": [f"pr{n}" for n in self.items], + "config": config, + "judge_cap": JUDGE_CAP, + "reviewed_heads": {}, + "started_at": time.strftime( + "%Y-%m-%dT%H:%M:%SZ", time.gmtime() + ), + }, + indent=2, + ) + ) + + def _record_head(self, rep: int, stem: str, head: str) -> None: + with self._lock: + path = self._manifest_path(rep) + manifest = json.loads(path.read_text()) + manifest["reviewed_heads"][stem] = head + path.write_text(json.dumps(manifest, indent=2)) + + # --- invocation --- + + def preflight(self) -> None: + config = self.ensure_config() + if config["env"]["AGENT_PROVIDER"] == "cursor" and not os.environ.get( + "REVIEWBOT_EVAL_ALLOW_CURSOR" + ): + sys.exit( + "AGENT_PROVIDER=cursor: cursor-family models have read the " + "imreview methodology skills from $HOME before (contamination " + "ledger). Vault the skill copies first, then set " + "REVIEWBOT_EVAL_ALLOW_CURSOR=1." + ) + for n in self.items: + if n not in self.expected: + sys.exit(f"pr{n} missing from {EXPECTED_HEADS}") + if not (GT / f"pr{n}.diff").is_file(): + sys.exit(f"gt/pr{n}.diff missing — cannot validate diff range") + doctor = subprocess.run( + [self.python, "-m", "omni_reviewbot", "doctor"], + cwd=self.bot_dir, env=self.child_env, + capture_output=True, text=True, timeout=300, + ) + if doctor.returncode != 0: + sys.exit( + "omni-reviewbot doctor failed:\n" + + (doctor.stdout or "") + (doctor.stderr or "") + ) + + def _invoke(self, pr: int, rep: int) -> None: + stem = f"pr{pr}" + out_md = self._arm_dir(rep) / f"{stem}.md" + if out_md.exists() and out_md.stat().st_size > 0: + return + completed = subprocess.run( + [self.python, "-m", "omni_reviewbot", "review", "--pr", str(pr)], + cwd=self.bot_dir, env=self.child_env, + capture_output=True, text=True, timeout=self.timeout, + ) + output = (completed.stdout or "") + (completed.stderr or "") + if completed.returncode != 0: + raise RuntimeError(f"exit {completed.returncode}: {output[-800:]}") + status = _STATUS_LINE.search(completed.stdout or "") + if not status or status.group(1) != "shadow": + raise RuntimeError( + f"outcome was not `status: shadow` — refusing " + f"(got {status.group(1) if status else 'no status line'})" + ) + context = _CONTEXT_LINE.search(completed.stdout or "") + if not context or context.group(1) != "no_discussion" or ( + context.group(2) != "0" + ): + raise RuntimeError( + "no `review_context: no_discussion (0 threads)` evidence — " + "the run may have seen the ground-truth threads" + ) + changed = _CHANGED_LINE.search(completed.stdout or "") + expected_files = gt_changed_files(pr) + if not changed or int(changed.group(1)) != expected_files: + raise RuntimeError( + f"changed_files {changed.group(1) if changed else '?'} != " + f"frozen GT diff's {expected_files} — diff range drifted" + ) + artifacts = sorted( + (STATE_DIR / "artifacts").glob(f"pr-{pr}-*.md"), + key=lambda p: p.stat().st_mtime, + ) + if not artifacts: + raise RuntimeError("no artifact produced") + artifact = artifacts[-1] + head = artifact.name[len(f"pr-{pr}-"):-len(".md")] + if head != self.expected[pr]: + raise RuntimeError( + f"reviewed head {head[:12]} != pinned {self.expected[pr][:12]}" + ) + body = sanitize(artifact.read_text()) + if len(body) > JUDGE_CAP: + raise RuntimeError( + f"sanitized review is {len(body)} chars — over the judge's " + f"{JUDGE_CAP} cap; it would be silently truncated" + ) + out_md.write_text(body) + # Move (not copy): the next replicate reuses the same artifact name. + artifact.unlink() + self._record_head(rep, stem, head) + + def _run_item(self, pr: int) -> str: + for rep in range(1, self.gen_reps + 1): + try: + self._invoke(pr, rep) + except Exception as exc: # noqa: BLE001 — every failure is a verdict + with self._lock: + self.failures.append(f"pr{pr} r{rep}: {exc}") + return f"pr{pr}: INVALID at r{rep}" + return f"pr{pr}: ok x{self.gen_reps}" + + def run(self, *, dry_run: bool) -> int: + plan = [ + f" pr{n} x{self.gen_reps} -> " + ", ".join( + str(self._arm_dir(r) / f"pr{n}.md") + for r in range(1, self.gen_reps + 1) + ) + for n in self.items + ] + print( + f"[arm] tag={self.tag} items={len(self.items)} " + f"reps={self.gen_reps} bot={self.bot_dir} " + f"provider={self.child_env.get('AGENT_PROVIDER', 'codex')}" + ) + print("\n".join(plan)) + if dry_run: + print("[arm] dry run — nothing invoked") + return 0 + self.preflight() + (STATE_DIR / "artifacts").mkdir(parents=True, exist_ok=True) + for rep in range(1, self.gen_reps + 1): + self._init_manifest(rep) + with ThreadPoolExecutor(max_workers=self.jobs) as pool: + futures = {pool.submit(self._run_item, n): n for n in self.items} + for future in as_completed(futures): + print(f"[arm] {future.result()}", flush=True) + if self.failures: + print( + f"[arm] {len(self.failures)} INVALID item(s) — the campaign " + "must not be judged:", flush=True, + ) + for failure in self.failures: + print(f" {failure}") + return 1 + print("[arm] complete") + return 0 + + +def main() -> int: + runner = Runner() + if "--preflight" in sys.argv: + runner.preflight() + print("[arm] preflight ok") + return 0 + return runner.run(dry_run="--dry-run" in sys.argv) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/eval/dataset/run_reviewbot_monthly.sh b/eval/dataset/run_reviewbot_monthly.sh new file mode 100755 index 00000000..dac67123 --- /dev/null +++ b/eval/dataset/run_reviewbot_monthly.sh @@ -0,0 +1,40 @@ +#!/usr/bin/env bash +# Monthly review-quality probe of omni-reviewbot against the pinned +# CC+Opus 5 baseline. One command: run_reviewbot_monthly.sh 2026-09 +# (a non-month tag, e.g. "smoke", writes a report but never touches the +# month-over-month INDEX). See REVIEWBOT_EVAL.md for the runbook. +set -euo pipefail +MONTH="${1:?usage: run_reviewbot_monthly.sh }" +TAG="reviewbot_${MONTH}" +GEN="${GEN_REPLICATES:-3}" +cd "$(dirname "$0")" +PY=${PYTHON:-python3} + +echo "=== preflight ===" +command -v claude >/dev/null || { + echo "judge CLI missing: install/login the claude CLI first" >&2; exit 1; } +ARM_TAG="$TAG" GEN_REPLICATES="$GEN" "$PY" run_reviewbot_arm.py --preflight + +echo "=== generate ($GEN replicates) ===" +ARM_TAG="$TAG" GEN_REPLICATES="$GEN" "$PY" run_reviewbot_arm.py + +echo "=== judge (pinned claude/claude-sonnet-5, 3 reps, vs claudecode_opus5) ===" +# Judge targets are derived from the arm manifest, so generation and +# judging can never disagree about the item set (smoke runs included). +ONLY=$("$PY" -c " +import json,sys +m = json.load(open('arms/${TAG}_r1/manifest.json')) +print(','.join(s[2:] for s in m['stems'])) +") +for i in $(seq 1 "$GEN"); do + JUDGE_BACKEND=claude JUDGE_MODEL=claude-sonnet-5 \ + SPLIT=all_pr ONLY_ITEMS="$ONLY" REPLICATES=3 \ + ARM_A_DIR="arms/${TAG}_r${i}" ARM_B_DIR="baselines/claudecode_opus5" \ + JUDGE_OUT="judgments/${TAG}_r${i}" "$PY" judge_val.py +done + +echo "=== verify (exact denominators, pinned judge, fresh sha256s) ===" +"$PY" build_reviewbot_report.py --verify --tag "$TAG" --gen-reps "$GEN" + +echo "=== report ===" +"$PY" build_reviewbot_report.py --tag "$TAG" --gen-reps "$GEN" diff --git a/test/test_reviewbot_eval.py b/test/test_reviewbot_eval.py new file mode 100644 index 00000000..2d736749 --- /dev/null +++ b/test/test_reviewbot_eval.py @@ -0,0 +1,337 @@ +"""Offline guards for the monthly reviewbot review-quality probe.""" +from __future__ import annotations + +import hashlib +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +DATASET_DIR = Path(__file__).resolve().parent.parent / "eval" / "dataset" +sys.path.insert(0, str(DATASET_DIR)) + +import build_reviewbot_report as report # noqa: E402 +import run_reviewbot_arm as arm # noqa: E402 + + +TRAIN_VAL_PRS = [4804, 4810, 4816, 4817, 4825, 4837, 4859, 4870, 4893, + 4923, 4926, 4950, 4970, 4977, 5009] + + +# --- runner: pure pieces --- + + +def test_sanitize_strips_markers_and_arm_identifying_heading(): + body = ( + "## Omni ReviewBot review\n\nfinding text\n\n" + "\n" + "\n" + ) + clean = arm.sanitize(body) + assert clean.startswith("## Review\n") + assert "omni-reviewbot" not in clean + assert "