From af9e365d0e1e1bfbe9d3e22776cf6b95c26e8d1f Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 12:24:02 +0000 Subject: [PATCH 1/3] feat(bench): both skeptics over the same frozen passes, and the evidence bench replay dropped (#246) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #246 asks whether a cross-model skeptic cuts noise where a same-model one cannot. Running it as written would measure the configuration already in production (GUARDIAN_PROVIDER=mistral, GUARDIAN_SKEPTIC=gemini) and could not say why that still produced 19 findings with 1 real. What was never measured is the comparison itself: the same finder output judged by both. The corpus turns out to allow a stronger question than the one asked. Of the 37 frozen passes carrying findings, 30 came from a gemini finder and 7 from a mistral finder, so both orientations are present. Judging every pass with each skeptic separates two hypotheses the issue conflates — cross-vendor refuting more in both orientations, versus gemini refuting more whichever finder wrote the findings. The original single-orientation design cannot tell those apart. Also fixed, and found while building this: guardian_bench's replay path hardcoded evidence=None, justified by "a recording is not a checkout". True, and beside the point — the commit is named by the fixture, so a worktree gets one, which is what guardian_replay_skeptic already does. Production runs with GUARDIAN_EVIDENCE=1, so every --replay-finder measurement to date was of a configuration nobody runs. _judge_recording now takes evidence keyword-only with no default, like judge_all and for the same reason. Two guards earned their place before any measurement: Both arms are built before either runs. A single-arm result costs the same as none and reads like a result. This is what stopped the first attempt after two calls instead of 135, when the local mistral key returned HTTP 402. GUARDIAN_SKEPTIC_MODEL is dropped per arm rather than passed through. build_skeptic_provider applies it to whichever provider it builds, so an environment holding the production value would have handed a gemini model name to the mistral arm — one arm on a model that does not exist, reported as a comparison between vendors. The measurement is NOT run: the local mistral key returns 402 (subscription), and no ollama is running, so no second arm exists today. Production is unaffected — the last review ran mistral successfully on 2026-08-16 22:53, and the 402s I first thought I saw in that log were the PR number. Co-Authored-By: Claude Opus 5 --- scripts/guardian_bench.py | 51 +++++- scripts/skeptic_arms.py | 266 ++++++++++++++++++++++++++++ tests/unit/test_skeptic_arms.py | 304 ++++++++++++++++++++++++++++++++ 3 files changed, 613 insertions(+), 8 deletions(-) create mode 100644 scripts/skeptic_arms.py create mode 100644 tests/unit/test_skeptic_arms.py diff --git a/scripts/guardian_bench.py b/scripts/guardian_bench.py index 1bb0deee..af75cc9a 100644 --- a/scripts/guardian_bench.py +++ b/scripts/guardian_bench.py @@ -33,6 +33,7 @@ ) from cgis.guardian.chunked import run_review_routed from cgis.guardian.collector import ContextCollector, parse_features +from cgis.guardian.evidence import Evidence, collect_evidence from cgis.guardian.findings import ReviewResult from cgis.guardian.providers.base import BaseProvider from cgis.guardian.review_fingerprint import ( @@ -51,6 +52,11 @@ log = structlog.getLogger(__name__) _REPO_ROOT = Path(__file__).parent.parent.absolute() + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from guardian_replay_skeptic import changed_files, worktree_at # noqa: E402 + _BENCH_DIR = _REPO_ROOT / "benchmarks" / "guardian" @@ -103,7 +109,10 @@ async def _run_one( With ``replay_finder`` the finder is skipped entirely and its recorded findings are judged instead, so a skeptic variant is measured against a - frozen set (spec §4.1) — no worktree, no ingest, no finder tokens. + frozen set (spec §4.1) — no ingest and no finder tokens. There *is* a + worktree: the checkers have to report on the code the recording was taken + from, and this line claimed there was none for as long as the evidence was + silently skipped. With ``record_finder`` the finder pass is written to disk and the skeptic is skipped, producing exactly that frozen set. """ @@ -122,7 +131,17 @@ async def _run_one( if replay_finder is not None: recording = load_finder_recording(replay_finder) - result = await _judge_recording(recording, skeptic) + # Evidence at the reviewed commit, not None. The replay path used to + # hardcode None on the reasoning that a recording is not a checkout — + # true, and beside the point: the commit is named by the fixture, so a + # worktree gets one, which is exactly what `guardian_replay_skeptic` + # does. Production runs with GUARDIAN_EVIDENCE=1, so judging without it + # measured a configuration nobody runs and quietly attributed the + # difference to whatever else the arm changed (#246). + evidence = await asyncio.to_thread( + _evidence_at_head, truth.head, recording.diff, _REPO_ROOT + ) + result = await _judge_recording(recording, skeptic, evidence=evidence) await _score_and_record( truth, run_idx, results_path, result, model, provider, skeptic, chunks=None ) @@ -159,17 +178,33 @@ async def _run_one( ) +def _evidence_at_head(head: str, diff: str, repo_root: Path) -> Evidence | None: + """The checkers' output at the commit the recording was taken from. + + A worktree, because the recording is not a checkout — which is the fact the + old `evidence=None` was justified by, and it argues for building one rather + than for going without. + """ + with worktree_at(head, repo_root) as tree: + return collect_evidence(tree, changed_files(diff)) + + async def _judge_recording( - recording: FinderRecording, skeptic: tuple[BaseProvider, str] | None + recording: FinderRecording, + skeptic: tuple[BaseProvider, str] | None, + *, + evidence: Evidence | None, ) -> ReviewResult: - """Run ONLY the skeptic over a frozen finder pass (spec §4.1).""" + """Run ONLY the skeptic over a frozen finder pass (spec §4.1). + + `evidence` is keyword-only with no default, matching `judge_all` and for the + same reason: this function spent a year passing None, and a default would + let the next caller do it again without saying so. + """ result = recording.result if skeptic is None or not result.findings: return result - # The replay path holds a recording, not a checkout: there is no project - # root, so no checkers to run. Stated as None rather than defaulted — which - # is the point of the argument being required (#401). - judgements = await judge_all(skeptic[0], result.findings, recording.diff, evidence=None) + judgements = await judge_all(skeptic[0], result.findings, recording.diff, evidence=evidence) judged = sum(1 for j in judgements if j is not None) return result.model_copy( update={ diff --git a/scripts/skeptic_arms.py b/scripts/skeptic_arms.py new file mode 100644 index 00000000..3bb0c47d --- /dev/null +++ b/scripts/skeptic_arms.py @@ -0,0 +1,266 @@ +"""Judge the same frozen finder passes with two skeptics, and score both (#246). + +#246 asks whether a **cross-model** skeptic cuts noise where a same-model one +cannot: a mistral skeptic over a mistral finder was measured as binary — it +refuted everything or nothing — and the designed answer was a different vendor. + +That answer has since shipped: production runs `GUARDIAN_PROVIDER=mistral` with +`GUARDIAN_SKEPTIC=gemini`. So running the issue as written would measure the +configuration already in use and could not say *why* it still produces 19 +findings with 1 real. What was never measured is the comparison itself — the +same finder output judged by both. + +The corpus makes a stronger question answerable than the one asked. Of the 37 +frozen passes carrying findings, 30 came from a gemini finder and 7 from a +mistral finder, so **both orientations are present**. Judging every one with +each skeptic separates two hypotheses the issue conflates: + +* cross-vendor refutes more in *both* orientations → the mechanism is the vendor + difference, as #246 supposes; +* gemini refutes more whichever finder produced the findings → the mechanism is + gemini's disposition, and "cross-model" was never the operative word. + +The original single-orientation design cannot tell those apart. + +**Evidence is collected, not skipped.** `guardian_bench`'s replay path hardcodes +`evidence=None`, which would measure a configuration production does not run +(`GUARDIAN_EVIDENCE=1`). Here it is collected once per PR in a worktree at the +fixture head — the same mechanism `guardian_replay_skeptic` uses. Four of the six +fixtures touch Python and yield evidence, covering 96 of 135 findings; the other +two touch none, so their findings are judged with `evidence=None` because that is +also what production would do for them. + +No finder call is made. The cost is one skeptic call per finding per arm. +""" + +import argparse +import asyncio +import json +import os +import sys +import tempfile +from collections import defaultdict +from collections.abc import Mapping +from datetime import UTC, datetime +from pathlib import Path +from typing import Any + +from cgis.guardian.bench import ( + GroundTruth, + killed_ground_truth, + load_ground_truth, + match_findings, + score, +) +from cgis.guardian.evidence import Evidence, collect_evidence +from cgis.guardian.providers.base import BaseProvider +from cgis.guardian.recording import load_finder_recording +from cgis.guardian.runner import build_skeptic_provider +from cgis.guardian.skeptic import apply_judgements, judge_all, visible_findings + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from guardian_replay_skeptic import changed_files, worktree_at +from recordings_from_corpus import build, is_frozen_pass, row_key + +REPO_ROOT = Path(__file__).resolve().parent.parent +BENCH_DIR = REPO_ROOT / "benchmarks" / "guardian" + +#: The two arms. Named rather than derived, because "the opposite of the primary" +#: is exactly the rule under test — deriving the arm from the finder would build +#: the hypothesis into the instrument. +ARMS = ("gemini", "mistral") + + +class NoArmError(RuntimeError): + """Raised when an arm's provider cannot be built. + + Refused before any call is spent. Half an experiment costs the same as none + and reads like a result, which is worse. + """ + + +def arm_provider(name: str, env: Mapping[str, str]) -> tuple[BaseProvider, str]: + """The skeptic for one arm, or a refusal naming the missing key. + + `GUARDIAN_SKEPTIC_MODEL` is dropped rather than passed through. + `build_skeptic_provider` applies it to whichever provider it builds, so an + environment holding the production value (`gemini-2.5-flash`) would hand + that model name to the mistral arm — one arm running a model that does not + exist, while the other ran the intended one. Each arm takes its provider's + own default, and the models used are recorded on every row. + """ + per_arm = {k: v for k, v in env.items() if k != "GUARDIAN_SKEPTIC_MODEL"} + built = build_skeptic_provider({**per_arm, "GUARDIAN_SKEPTIC": name}, primary="none") + if built is None: + _msg = ( + f"Arm {name!r} could not be built — its API key is missing, or the model was " + f"rejected. Both arms must exist before either is run: a single-arm result " + f"answers nothing this experiment asks." + ) + raise NoArmError(_msg) + return built + + +def finder_models(results: Path) -> dict[str, str]: + """`row_key` → the model that produced that pass. + + The recording carries findings and a diff and nothing about who wrote them, + so the vendor split — the whole reason both orientations are separable — has + to come back from the corpus row. + """ + models: dict[str, str] = {} + for line in results.read_text(encoding="utf-8").splitlines(): + if not line.strip(): + continue + row = json.loads(line) + if is_frozen_pass(row): + models[row_key(row).replace(":", "-")] = str(row.get("model") or "unknown") + return models + + +def evidence_for_pr(truth: GroundTruth, diff: str, repo_root: Path) -> Evidence | None: + """Checker output at the reviewed commit, or None when there is nothing to check. + + Once per PR rather than once per pass: every pass of a PR was shown the same + diff, so the checkers would report the same thing, and a worktree costs a + `uv sync`. None here is not a degradation — a fixture whose diff touches no + Python has nothing for the checkers to say, and production would collect + nothing for it either. + """ + with worktree_at(truth.head, repo_root) as tree: + return collect_evidence(tree, changed_files(diff)) + + +async def judge_one( + provider: BaseProvider, + recording_path: Path, + truth: GroundTruth, + evidence: Evidence | None, +) -> dict[str, Any]: + """One recording under one arm, scored the way `guardian_bench` scores a run.""" + recording = load_finder_recording(recording_path) + findings = recording.result.findings + judgements = await judge_all(provider, findings, recording.diff, evidence=evidence) + judged = apply_judgements(findings, judgements) + visible = visible_findings(judged) + matches = match_findings(visible, truth) + bench_score = score(matches, truth) + return { + "findings": len(findings), + "refuted": sum(1 for f in judged if f.verdict == "refuted"), + "uncertain": sum(1 for f in judged if f.verdict == "uncertain"), + "confirmed": sum(1 for f in judged if f.verdict == "confirmed"), + "unruled": sum(1 for j in judgements if j is None), + "recall": bench_score.recall, + "precision": bench_score.precision, + "noise": bench_score.noise, + # The recall guard, and the reason `missed` is not enough: it cannot tell + # "the finder never found it" from "the finder found it and the skeptic + # killed it" (#270), and only the second is this experiment's doing. + "killed_gt": killed_ground_truth(judged, truth), + } + + +async def collect_rows(repo_root: Path, limit: int | None) -> list[dict[str, Any]]: + """Both arms over every frozen pass; one row per (pass, arm). + + Returns the rows rather than writing them. The write is the caller's, and + synchronous: a blocking file write inside the event loop is the same defect + the worktree collection above avoids with `to_thread`, and here there is + nothing to gain by being in the loop at all. + """ + arms = {name: arm_provider(name, os.environ) for name in ARMS} + models = finder_models(BENCH_DIR / "results.jsonl") + + with tempfile.TemporaryDirectory(prefix="arms-") as tmp: + written = build(BENCH_DIR / "results.jsonl", Path(tmp), BENCH_DIR, repo_root) + # Passes with no findings cost nothing to judge and contribute nothing to + # a comparison of judgements, so they are dropped before any worktree is + # built rather than filtered out of the numbers afterwards. + recordings = [p for p in written if load_finder_recording(p).result.findings] + recordings.sort(key=lambda p: p.stem) + if limit is not None: + recordings = recordings[:limit] + + by_pr: dict[int, list[Path]] = defaultdict(list) + for path in recordings: + by_pr[int(path.stem.split("@")[0])].append(path) + + rows: list[dict[str, Any]] = [] + for pr in sorted(by_pr): + truth = load_ground_truth(BENCH_DIR / f"pr-{pr}.yaml") + sample = load_finder_recording(by_pr[pr][0]) + evidence = await asyncio.to_thread(evidence_for_pr, truth, sample.diff, repo_root) + print( + f"pr-{pr}: {len(by_pr[pr])} passes, " + f"evidence {'collected' if evidence else 'unavailable'}", + file=sys.stderr, + ) + for path in by_pr[pr]: + for name, (provider, model) in arms.items(): + scored = await judge_one(provider, path, truth, evidence) + rows.append( + { + "timestamp": datetime.now(UTC).isoformat(), + "pr": pr, + "pass": path.stem, + "arm": name, + "skeptic_model": model, + "finder_model": models.get(path.stem, "unknown"), + "evidence": evidence is not None, + **scored, + } + ) + print( + f" {path.stem} [{name}] {scored['refuted']}/{scored['findings']} refuted", + file=sys.stderr, + ) + + return rows + + +def _vendor(model: str) -> str: + return "gemini" if "gemini" in model else "mistral" if "mistral" in model else "other" + + +def report(rows: list[dict[str, Any]]) -> None: + """The comparison the issue asks for, split by which vendor found the findings.""" + print(f"\n{len(rows)} (pass, arm) results\n") + print( + f"{'finder':9} {'skeptic':9} {'kind':7} {'passes':>6} {'refuted':>8} " + f"{'of':>5} {'killed GT':>10}" + ) + groups: dict[tuple[str, str], list[dict[str, Any]]] = defaultdict(list) + for row in rows: + groups[(_vendor(str(row["finder_model"])), str(row["arm"]))].append(row) + for (finder, arm), group in sorted(groups.items()): + kind = "same" if finder == arm else "cross" + refuted = sum(int(r["refuted"]) for r in group) + total = sum(int(r["findings"]) for r in group) + killed = sum(len(r["killed_gt"]) for r in group) + print(f"{finder:9} {arm:9} {kind:7} {len(group):6} {refuted:8} {total:5} {killed:10}") + print( + "\n'killed GT' is ground truth the finder matched and the skeptic then hid — the " + "recall cost, which `missed` cannot show." + ) + + +def main() -> int: + """Run both arms and write the rows.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--out", type=Path, default=REPO_ROOT / ".guardian-arms.jsonl") + parser.add_argument("--repo-root", type=Path, default=REPO_ROOT) + parser.add_argument( + "--limit", type=int, default=None, help="judge only the first N passes (a smoke run)" + ) + args = parser.parse_args() + rows = asyncio.run(collect_rows(args.repo_root, args.limit)) + args.out.parent.mkdir(parents=True, exist_ok=True) + args.out.write_text("".join(json.dumps(r) + "\n" for r in rows), encoding="utf-8") + report(rows) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/unit/test_skeptic_arms.py b/tests/unit/test_skeptic_arms.py new file mode 100644 index 00000000..076fd160 --- /dev/null +++ b/tests/unit/test_skeptic_arms.py @@ -0,0 +1,304 @@ +"""Both arms over one frozen pass, with the model calls stubbed (#246). + +The experiment itself needs two providers; these tests need none. What they +cover is everything that decides whether the numbers mean anything: which +skeptic each arm builds, that neither arm can inherit the other's model, and +that a same/cross classification is derived from the finder rather than assumed. +""" + +import json +import sys +from pathlib import Path +from typing import Any + +import pytest +from guardian_stubs import StubProvider + +from cgis.guardian.bench import load_ground_truth + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "scripts")) + +import skeptic_arms +from skeptic_arms import ARMS, NoArmError, _vendor, arm_provider, finder_models, judge_one, report + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent +BENCH_DIR = REPO_ROOT / "benchmarks" / "guardian" + + +class TestBuildingAnArm: + """Which provider each arm gets, and what it must not inherit.""" + + def test_a_missing_provider_refuses_before_any_call( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Half an experiment costs the same as none and reads like a result. + + This refusal is not hypothetical: it is what stopped the first real run + after 2 calls instead of 135, when the mistral key returned HTTP 402. + """ + monkeypatch.setattr(skeptic_arms, "build_skeptic_provider", lambda _e, **_k: None) + with pytest.raises(NoArmError, match="answers nothing this experiment asks"): + arm_provider("mistral", {}) + + def test_the_arm_name_is_what_selects_the_provider( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + seen: list[str] = [] + + def _capture(env: dict[str, str], **_kw: object) -> tuple[StubProvider, str]: + seen.append(env["GUARDIAN_SKEPTIC"]) + return StubProvider([]), "m" + + monkeypatch.setattr(skeptic_arms, "build_skeptic_provider", _capture) + for name in ARMS: + arm_provider(name, {}) + assert seen == list(ARMS) + + def test_a_skeptic_model_in_the_environment_reaches_neither_arm( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The cross-arm contamination that would have run one arm on a fake model. + + `build_skeptic_provider` applies `GUARDIAN_SKEPTIC_MODEL` to whichever + provider it builds. An environment holding the production value + (`gemini-2.5-flash`) would hand that name to the mistral arm, so one arm + would run a model that does not exist while the other ran the intended + one — and the comparison would be between a working skeptic and a broken + one, reported as a comparison between vendors. + """ + envs: list[dict[str, str]] = [] + + def _capture(env: dict[str, str], **_kw: object) -> tuple[StubProvider, str]: + envs.append(dict(env)) + return StubProvider([]), "m" + + monkeypatch.setattr(skeptic_arms, "build_skeptic_provider", _capture) + arm_provider("mistral", {"GUARDIAN_SKEPTIC_MODEL": "gemini-2.5-flash", "X": "keep"}) + assert "GUARDIAN_SKEPTIC_MODEL" not in envs[0] + assert envs[0]["X"] == "keep" + + +class TestTheVendorSplit: + """Both orientations are in the corpus, and that is what the split rests on.""" + + @pytest.mark.parametrize( + ("model", "vendor"), + [ + ("gemini-2.5-flash", "gemini"), + ("gemini-3.5-flash", "gemini"), + ("mistral-medium-latest", "mistral"), + ("codestral-latest", "other"), + ("unknown", "other"), + ], + ) + def test_a_model_name_maps_to_its_vendor(self, model: str, vendor: str) -> None: + assert _vendor(model) == vendor + + def test_the_corpus_really_carries_both_orientations(self) -> None: + """The premise of the whole design, asserted rather than assumed. + + If every frozen pass came from one vendor there would be one + orientation, and the experiment could not tell "cross-vendor refutes + more" from "gemini refutes more" — which is the distinction #246's + original single-orientation design could not make. + """ + vendors = {_vendor(m) for m in finder_models(BENCH_DIR / "results.jsonl").values()} + assert {"gemini", "mistral"} <= vendors + + def test_only_frozen_passes_are_mapped(self, tmp_path: Path) -> None: + """A judged row's findings carry rewritten confidences (#279), so it is not a pass.""" + rows = [ + { + "pr": 1, + "timestamp": "t1", + "matched": [], + "precision": 1.0, + "model": "m", + "findings": [], + }, + {"pr": 2, "timestamp": "t2", "matched": [], "precision": 1.0, "skeptic_model": "s"}, + ] + path = tmp_path / "results.jsonl" + path.write_text("".join(json.dumps(r) + "\n" for r in rows), encoding="utf-8") + assert finder_models(path) == {"1@t1": "m"} + + +@pytest.mark.asyncio +async def test_judge_one_scores_a_pass_and_reports_what_the_skeptic_killed( + tmp_path: Path, +) -> None: + """The row a single (pass, arm) produces, including the recall guard. + + `killed_gt` is carried because `missed` cannot tell "the finder never found + it" from "the finder found it and the skeptic hid it" (#270), and only the + second is something an arm did. + """ + recording = json.loads( + (BENCH_DIR / "experiments" / "401-evidence" / "pr-399-recording.json").read_text( + encoding="utf-8" + ) + ) + findings = recording["result"]["findings"][:2] + for finding in findings: + finding.pop("verdict", None) + finding.pop("skeptic_note", None) + path = tmp_path / "rec.json" + path.write_text( + json.dumps({"result": {"findings": findings, "summary": ""}, "diff": recording["diff"]}), + encoding="utf-8", + ) + truth = load_ground_truth(BENCH_DIR / "pr-143.yaml") + provider = StubProvider( + ['{"verdict": "refuted", "impact_score": 0, "rationale": "r"}'] * len(findings) + ) + + row = await judge_one(provider, path, truth, None) + + assert row["findings"] == len(findings) + assert row["refuted"] == len(findings) + assert row["confirmed"] == 0 + assert isinstance(row["killed_gt"], list) + + +class _Rec: + """A recording stand-in: findings and the diff they were found in.""" + + def __init__(self, findings: list[object]) -> None: + self.result = type("R", (), {"findings": findings})() + self.diff = "diff --git a/x.py b/x.py\n" + + +class TestTheRunItself: + """Orchestration, with every call stubbed: what runs, how often, and on what.""" + + def _wire( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, passes: list[str] + ) -> list[str]: + """Stub every outside edge; return the list evidence collection appends to.""" + paths = [tmp_path / f"{name}.json" for name in passes] + for path in paths: + path.write_text("{}", encoding="utf-8") + collected: list[str] = [] + monkeypatch.setattr( + skeptic_arms, "arm_provider", lambda name, _env: (StubProvider([]), f"{name}-model") + ) + monkeypatch.setattr(skeptic_arms, "finder_models", lambda _p: {}) + monkeypatch.setattr(skeptic_arms, "build", lambda *_a: paths) + monkeypatch.setattr(skeptic_arms, "load_finder_recording", lambda _p: _Rec(["f"])) + monkeypatch.setattr(skeptic_arms, "load_ground_truth", lambda _p: object()) + + def _evidence(_truth: object, _diff: str, _root: Path) -> object: + collected.append("once") + return object() + + monkeypatch.setattr(skeptic_arms, "evidence_for_pr", _evidence) + + async def _judge(*_a: object, **_k: object) -> dict[str, Any]: + return {"findings": 1, "refuted": 0, "killed_gt": []} + + monkeypatch.setattr(skeptic_arms, "judge_one", _judge) + return collected + + @pytest.mark.asyncio + async def test_evidence_is_collected_once_per_pr_not_once_per_pass( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Three passes of one PR share a diff, so the checkers would say the same thing. + + Per pass it would be three worktrees and three `uv sync`s for one answer. + Asserted by counting, because a per-pass version returns identical rows + and differs only in how long it takes — invisible in the output. + """ + collected = self._wire(monkeypatch, tmp_path, ["143@a", "143@b", "143@c"]) + rows = await skeptic_arms.collect_rows(REPO_ROOT, None) + assert len(collected) == 1 + assert len(rows) == 3 * len(ARMS) + + @pytest.mark.asyncio + async def test_each_pass_is_judged_by_every_arm( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Pairing is the whole design: the same findings, both skeptics.""" + self._wire(monkeypatch, tmp_path, ["143@a", "144@b"]) + rows = await skeptic_arms.collect_rows(REPO_ROOT, None) + by_pass: dict[str, set[str]] = {} + for row in rows: + by_pass.setdefault(str(row["pass"]), set()).add(str(row["arm"])) + assert by_pass == {"143@a": set(ARMS), "144@b": set(ARMS)} + + @pytest.mark.asyncio + async def test_limit_trims_the_work_before_a_worktree_is_built( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """`--limit` is the smoke run; it must cost one PR, not all of them.""" + collected = self._wire(monkeypatch, tmp_path, ["143@a", "144@b"]) + rows = await skeptic_arms.collect_rows(REPO_ROOT, 1) + assert len(collected) == 1 + assert len(rows) == len(ARMS) + + @pytest.mark.asyncio + async def test_a_pass_with_no_findings_is_dropped_before_any_worktree( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Nothing to judge means nothing to compare, and a worktree costs a uv sync.""" + self._wire(monkeypatch, tmp_path, ["143@a"]) + monkeypatch.setattr(skeptic_arms, "load_finder_recording", lambda _p: _Rec([])) + collected: list[str] = [] + monkeypatch.setattr( + skeptic_arms, "evidence_for_pr", lambda *_a: collected.append("x") or object() + ) + rows = await skeptic_arms.collect_rows(REPO_ROOT, None) + assert rows == [] + assert collected == [] + + +def test_main_writes_the_rows_it_collected( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The CLI wiring, and the reason the write is not inside the coroutine. + + `collect_rows` returns rows and `main` writes them, so the blocking file + write never happens on the event loop — the same rule the worktree + collection follows with `to_thread`. + """ + row = { + "finder_model": "gemini-2.5-flash", + "arm": "mistral", + "refuted": 1, + "findings": 2, + "killed_gt": [], + } + + async def _rows(_root: Path, _limit: int | None) -> list[dict[str, Any]]: + return [row] + + out = tmp_path / "nested" / "arms.jsonl" + monkeypatch.setattr(skeptic_arms, "collect_rows", _rows) + monkeypatch.setattr(sys, "argv", ["skeptic_arms.py", "--out", str(out)]) + assert skeptic_arms.main() == 0 + assert [json.loads(line) for line in out.read_text(encoding="utf-8").splitlines()] == [row] + assert "cross" in capsys.readouterr().out + + +def test_report_labels_same_and_cross_from_the_finder(capsys: pytest.CaptureFixture[str]) -> None: + """The label is derived, not stored — an arm is 'same' only against its own vendor.""" + rows: list[dict[str, Any]] = [ + { + "finder_model": "mistral-medium-latest", + "arm": "gemini", + "refuted": 3, + "findings": 5, + "killed_gt": [], + }, + { + "finder_model": "mistral-medium-latest", + "arm": "mistral", + "refuted": 0, + "findings": 5, + "killed_gt": ["a"], + }, + ] + report(rows) + out = capsys.readouterr().out + assert "cross" in out + assert "same" in out From 7921896f7af111d4cbf9ff32fd161e079e18eda5 Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 12:35:12 +0000 Subject: [PATCH 2/3] fix(bench): an unrecognised model must not be reported as cross-vendor (#246) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini-code-assist on #411 spotted that codestral-latest is a Mistral model whose name does not contain "mistral", so _vendor called it "other" and the report then labelled it cross when paired with the mistral arm. Sonar separately flagged the nested conditional. Both are the same line, and patching in "codestral" would have fixed one instance of a mechanism. The mechanism: "other" equals no arm name, so every row of an unattributed model read `cross` — in the exact variable this experiment measures. A new vendor would not have raised anything. It would have produced a wrong answer shaped like the expected one. So the markers are an explicit table (mistral also ships ministral and magistral), matching is case-insensitive, and an unrecognised model becomes UNKNOWN_VENDOR, which the report keeps as its own kind rather than folding into cross. same-vs-cross for a model nobody has attributed is not merely unmeasured, it is unknowable, and printing "cross" would be inventing it. The unattributed names are listed under the table with the one-line fix, because nobody can add a marker for a number they never saw. Mutation-verified: restoring the two-branch kind reddens the new test. Co-Authored-By: Claude Opus 5 --- scripts/skeptic_arms.py | 47 +++++++++++++++++++++++++++++++-- tests/unit/test_skeptic_arms.py | 36 +++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/scripts/skeptic_arms.py b/scripts/skeptic_arms.py index 3bb0c47d..3e3ade52 100644 --- a/scripts/skeptic_arms.py +++ b/scripts/skeptic_arms.py @@ -220,8 +220,39 @@ async def collect_rows(repo_root: Path, limit: int | None) -> list[dict[str, Any return rows +#: Substrings that identify a model's vendor, lower-cased. +#: +#: Explicit rather than "does the name contain the vendor's name", because +#: Mistral ships families that do not: `codestral-latest` is a Mistral model and +#: was the arm proposed for #246's first draft. Raised in review of #411. +_VENDOR_MARKERS: dict[str, tuple[str, ...]] = { + "gemini": ("gemini",), + "mistral": ("mistral", "codestral", "ministral", "magistral"), +} + +#: A model this table does not recognise. **Not** a third vendor — a refusal to +#: guess, and it has to travel as far as the report. +#: +#: The failure it replaces was silent and pointed one way. An unrecognised model +#: used to become "other", "other" never equals an arm name, so every one of its +#: rows read `cross` — in the exact variable this experiment measures. A new +#: vendor tomorrow would not have produced an error; it would have produced a +#: wrong answer that looked like the expected one. +UNKNOWN_VENDOR = "unknown" + + def _vendor(model: str) -> str: - return "gemini" if "gemini" in model else "mistral" if "mistral" in model else "other" + """The vendor behind a model name, or `UNKNOWN_VENDOR`. + + Lower-cased first: the corpus holds names as the provider reported them, and + a capitalised variant would fall through to unknown for no reason anyone + chose. + """ + lowered = model.lower() + for vendor, markers in _VENDOR_MARKERS.items(): + if any(marker in lowered for marker in markers): + return vendor + return UNKNOWN_VENDOR def report(rows: list[dict[str, Any]]) -> None: @@ -235,11 +266,23 @@ def report(rows: list[dict[str, Any]]) -> None: for row in rows: groups[(_vendor(str(row["finder_model"])), str(row["arm"]))].append(row) for (finder, arm), group in sorted(groups.items()): - kind = "same" if finder == arm else "cross" + # `unknown` is its own kind, never folded into `cross`. same-vs-cross is + # the variable under test, and for a model no one has attributed it is + # not merely unmeasured — it is unknowable, so saying "cross" would be + # inventing the answer. + kind = "unknown" if finder == UNKNOWN_VENDOR else "same" if finder == arm else "cross" refuted = sum(int(r["refuted"]) for r in group) total = sum(int(r["findings"]) for r in group) killed = sum(len(r["killed_gt"]) for r in group) print(f"{finder:9} {arm:9} {kind:7} {len(group):6} {refuted:8} {total:5} {killed:10}") + unattributed = sorted( + {str(r["finder_model"]) for r in rows if _vendor(str(r["finder_model"])) == UNKNOWN_VENDOR} + ) + if unattributed: + print( + f"\n{len(unattributed)} model name(s) match no vendor and are excluded from the " + f"same/cross split: {', '.join(unattributed)}. Add a marker to _VENDOR_MARKERS." + ) print( "\n'killed GT' is ground truth the finder matched and the skeptic then hid — the " "recall cost, which `missed` cannot show." diff --git a/tests/unit/test_skeptic_arms.py b/tests/unit/test_skeptic_arms.py index 076fd160..bfc81a1b 100644 --- a/tests/unit/test_skeptic_arms.py +++ b/tests/unit/test_skeptic_arms.py @@ -87,8 +87,14 @@ class TestTheVendorSplit: ("gemini-2.5-flash", "gemini"), ("gemini-3.5-flash", "gemini"), ("mistral-medium-latest", "mistral"), - ("codestral-latest", "other"), - ("unknown", "other"), + # Mistral families whose names do not contain "mistral". codestral + # was the arm proposed in #246's first draft, so this is not + # hypothetical. Raised in review of #411. + ("codestral-latest", "mistral"), + ("ministral-8b-latest", "mistral"), + ("Gemini-2.5-Flash", "gemini"), + ("gpt-5", "unknown"), + ("", "unknown"), ], ) def test_a_model_name_maps_to_its_vendor(self, model: str, vendor: str) -> None: @@ -302,3 +308,29 @@ def test_report_labels_same_and_cross_from_the_finder(capsys: pytest.CaptureFixt out = capsys.readouterr().out assert "cross" in out assert "same" in out + + +def test_an_unattributed_finder_is_never_reported_as_cross( + capsys: pytest.CaptureFixture[str], +) -> None: + """The silent failure behind the codestral case, and the general answer to it. + + An unrecognised model used to become "other"; "other" equals no arm name, so + every one of its rows read `cross` — in the exact variable this experiment + measures. A new vendor would not have raised anything; it would have + produced a wrong answer shaped like the expected one. So `unknown` is its + own kind and the model is named, because the fix is a one-line marker and + nobody can apply it to a number they never saw. + """ + rows: list[dict[str, Any]] = [ + {"finder_model": "gpt-5", "arm": "gemini", "refuted": 1, "findings": 4, "killed_gt": []} + ] + report(rows) + out = capsys.readouterr().out + # The table row, not the whole blob: the sentence naming the omission says + # "same/cross split", so a substring check over the output would pass on the + # explanation while the row said `cross`. + row_line = next(line for line in out.splitlines() if line.startswith("unknown")) + assert row_line.split()[:3] == ["unknown", "gemini", "unknown"] + assert "gpt-5" in out + assert "_VENDOR_MARKERS" in out From a15e94a4b08aa0a22ccd636cee1b4e7965fb5528 Mon Sep 17 00:00:00 2001 From: zaebee Date: Mon, 17 Aug 2026 12:42:37 +0000 Subject: [PATCH 3/3] refactor(bench): name the experiment's independent variable (#246) SonarCloud on #411: the same/cross/unknown choice was a nested conditional inside the report loop. It is the variable the whole experiment turns on, so it becomes a named function with its own tests rather than an expression read three clauses deep. Behaviour is unchanged; the parametrised test pins both orderings of the unknown case, because which arm an unattributed model happened to meet must not decide how it is classified. Co-Authored-By: Claude Opus 5 --- scripts/skeptic_arms.py | 23 ++++++++++++++++++----- tests/unit/test_skeptic_arms.py | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/scripts/skeptic_arms.py b/scripts/skeptic_arms.py index 3e3ade52..cec3f1b0 100644 --- a/scripts/skeptic_arms.py +++ b/scripts/skeptic_arms.py @@ -255,6 +255,23 @@ def _vendor(model: str) -> str: return UNKNOWN_VENDOR +def pairing_kind(finder: str, arm: str) -> str: + """How a (finder, skeptic) pair relates: `same`, `cross`, or `unknown`. + + Its own function rather than a conditional inside the report loop, because + it is the experiment's independent variable and deserves to be read and + tested on its own (raised by SonarCloud on #411 as a nested conditional). + + `unknown` is never folded into `cross`. For a model nobody has attributed, + same-vs-cross is not merely unmeasured but unknowable, and answering `cross` + would be inventing the very thing under test — which is exactly what the + previous version did, silently, to every unrecognised model. + """ + if finder == UNKNOWN_VENDOR: + return "unknown" + return "same" if finder == arm else "cross" + + def report(rows: list[dict[str, Any]]) -> None: """The comparison the issue asks for, split by which vendor found the findings.""" print(f"\n{len(rows)} (pass, arm) results\n") @@ -266,11 +283,7 @@ def report(rows: list[dict[str, Any]]) -> None: for row in rows: groups[(_vendor(str(row["finder_model"])), str(row["arm"]))].append(row) for (finder, arm), group in sorted(groups.items()): - # `unknown` is its own kind, never folded into `cross`. same-vs-cross is - # the variable under test, and for a model no one has attributed it is - # not merely unmeasured — it is unknowable, so saying "cross" would be - # inventing the answer. - kind = "unknown" if finder == UNKNOWN_VENDOR else "same" if finder == arm else "cross" + kind = pairing_kind(finder, arm) refuted = sum(int(r["refuted"]) for r in group) total = sum(int(r["findings"]) for r in group) killed = sum(len(r["killed_gt"]) for r in group) diff --git a/tests/unit/test_skeptic_arms.py b/tests/unit/test_skeptic_arms.py index bfc81a1b..ff20fb21 100644 --- a/tests/unit/test_skeptic_arms.py +++ b/tests/unit/test_skeptic_arms.py @@ -310,6 +310,25 @@ def test_report_labels_same_and_cross_from_the_finder(capsys: pytest.CaptureFixt assert "same" in out +@pytest.mark.parametrize( + ("finder", "arm", "kind"), + [ + ("gemini", "gemini", "same"), + ("mistral", "mistral", "same"), + ("gemini", "mistral", "cross"), + ("mistral", "gemini", "cross"), + # Both orderings, because `unknown` must not depend on which arm it met. + ("unknown", "gemini", "unknown"), + ("unknown", "mistral", "unknown"), + ], +) +def test_the_independent_variable_is_classified_on_its_own( + finder: str, arm: str, kind: str +) -> None: + """same / cross / unknown, read and checked apart from the report that prints it.""" + assert skeptic_arms.pairing_kind(finder, arm) == kind + + def test_an_unattributed_finder_is_never_reported_as_cross( capsys: pytest.CaptureFixture[str], ) -> None: