From 708c087f79ea7005873c398f4f74b2510c187de4 Mon Sep 17 00:00:00 2001 From: Teakowa <27560638+Teakowa@users.noreply.github.com> Date: Sun, 11 Oct 2026 12:54:02 +0800 Subject: [PATCH] fix(bench): keep regrade copies faithful and mirror grader availability (#614) Review follow-ups on #621: copytree now preserves agent-written symlinks as links instead of dereferencing outside targets into --out; a re-grade that hits an unavailable check records the grader-unavailable reason and invalid status like a fresh run (the reason is still stripped when unavailability resolves); same-named input directories fail upfront instead of dying mid-copy; and the docs note that trials without a parseable contract result.json are skipped. --- benchmarks/agent/bench_regrade.py | 35 +++++++++++------- benchmarks/agent/test_agent_bench.py | 53 ++++++++++++++++++++++++++++ docs/agent-benchmark-howto.md | 8 +++-- 3 files changed, 81 insertions(+), 15 deletions(-) diff --git a/benchmarks/agent/bench_regrade.py b/benchmarks/agent/bench_regrade.py index 9e8e425c..84750423 100644 --- a/benchmarks/agent/bench_regrade.py +++ b/benchmarks/agent/bench_regrade.py @@ -24,7 +24,9 @@ def regrade_trial(result: dict, dest: Path, scenario: dict, wright: str) -> bool `grade` owns are replaced. Returns True when the trial was re-graded. """ source = result["_dir"] - shutil.copytree(source, dest) + # `symlinks=True` keeps agent-written links as links; dereferencing them + # would copy an outside target's contents recursively into `--out`. + shutil.copytree(source, dest, symlinks=True) current = bench_grade.grader_hash(scenario) recorded = (result.get("grader") or {}).get("hash") if recorded == current: @@ -36,15 +38,19 @@ def regrade_trial(result: dict, dest: Path, scenario: dict, wright: str) -> bool verdict = bench_grade.grade(scenario, workspace, wright, dest / "grading") result.update(verdict) result["grader"]["regradedFrom"] = recorded - # The agent-side invalid reasons cannot be re-derived, but a grader that - # was unavailable then and resolves now stops being a blocking reason. - if "invalid" in result and not any(c.get("unavailable") for c in result["checks"]): - reasons = [r for r in result["invalid"].split("; ") if r != GRADER_UNAVAILABLE] - if reasons != result["invalid"].split("; "): - if reasons: - result["invalid"] = "; ".join(reasons) - else: - del result["invalid"] + # Grader availability is re-derived like a fresh run: a re-grade that + # hits an unavailable check records the reason, and one that resolves it + # drops it. Agent-side invalid reasons cannot be re-derived and stay. + reasons = result["invalid"].split("; ") if result.get("invalid") else [] + if any(c.get("unavailable") for c in result["checks"]): + if GRADER_UNAVAILABLE not in reasons: + reasons.append(GRADER_UNAVAILABLE) + else: + reasons = [r for r in reasons if r != GRADER_UNAVAILABLE] + if reasons: + result["invalid"] = "; ".join(reasons) + else: + result.pop("invalid", None) result["status"] = bench_report.run_status(result) result["_verdictChanged"] = usable_before != result.get("usable") bench_report.write_json(dest / "result.json", {k: v for k, v in result.items() if not k.startswith("_")}) @@ -52,17 +58,22 @@ def regrade_trial(result: dict, dest: Path, scenario: dict, wright: str) -> bool def regrade(dirs: list[Path], wright: str, load_scenario, out: Path) -> dict: - """Copy every trial under `dirs` into `out`, re-grading hash-mismatched trials. + """Copy every completed trial under `dirs` into `out`, re-grading hash-mismatched trials. `out` is laid out as `//` so the result is consumable by `report`, `score`, and `publish` exactly like a run - directory. No input directory is modified. + directory. A trial dir without a parseable contract `result.json` is not + a finished trial and is skipped, as `load` does everywhere. No input + directory is modified. """ resolved = [base.resolve() for base in dirs] out = out.resolve() for base in resolved: if base == out or out in base.parents or base in out.parents: raise SystemExit("--out must be disjoint from every input directory") + names = [base.name for base in resolved] + if len(set(names)) != len(names): + raise SystemExit("input directories must have distinct names") if out.exists() and any(out.iterdir()): raise SystemExit(f"--out {out} already exists and is not empty") scenarios: dict[str, dict] = {} diff --git a/benchmarks/agent/test_agent_bench.py b/benchmarks/agent/test_agent_bench.py index 502cbc9a..6083ccc0 100644 --- a/benchmarks/agent/test_agent_bench.py +++ b/benchmarks/agent/test_agent_bench.py @@ -1077,6 +1077,59 @@ def test_a_pre_hash_archive_regrades_and_reports_the_transition(self): self.assertIsNone(result["grader"]["regradedFrom"]) self.assertEqual(result["grader"]["hash"], new_hash) + def test_a_symlink_in_the_trial_is_copied_as_a_link(self): + # An agent can write a link outside its workspace; dereferencing it + # during the copy would pull the target's contents into `--out`. + trial = self.stored_trial("1", bench_grade.grader_hash(self.scenario("tiny"))) + outside = self.out / "outside.txt" + outside.write_text("payload") + (trial / "workspace" / "link").symlink_to(outside) + bench_regrade.regrade([self.out / "runs"], WRIGHT, self.scenario, self.out / "regraded") + link = self.out / "regraded" / "runs" / "tiny" / "fake" / "none-none_off-1" / "workspace" / "link" + self.assertTrue(link.is_symlink(), "the copy must keep the link, not its target") + self.assertEqual(link.read_text(), "payload") + + def test_grader_unavailability_mirrors_a_fresh_run(self): + # A fresh `run_trial` appends "a required grader was unavailable" and + # marks the result invalid; a re-grade that hits or resolves an + # unavailable check mirrors both directions (#621 review). + verdict = { + "passed": False, "usable": False, + "checks": [{"id": "has-text", "kind": "contains", "passed": False, "unavailable": True}], + "grader": {"hash": "new-hash", "oracle": None}, + } + with patch.object(bench_regrade.bench_grade, "grade", return_value=verdict): + # Hits unavailability -> gains the reason and the invalid status. + trial = self.stored_trial("1", "stale") + result = json.loads((trial / "result.json").read_text()) + result["_dir"] = trial + bench_regrade.regrade_trial(result, self.out / "dest1", self.scenario("tiny"), WRIGHT) + self.assertEqual(result["invalid"], bench_regrade.GRADER_UNAVAILABLE) + self.assertEqual(result["status"], "invalid") + # Resolves unavailability -> the stored reason is stripped. + trial = self.stored_trial("2", "stale") + stored = json.loads((trial / "result.json").read_text()) + stored["invalid"] = "; ".join(["an agent-side reason", bench_regrade.GRADER_UNAVAILABLE]) + stored["status"] = "invalid" + (trial / "result.json").write_text(json.dumps(stored)) + resolved = {**verdict, "checks": [{"id": "has-text", "kind": "contains", "passed": True}], "usable": True, "passed": True} + with patch.object(bench_regrade.bench_grade, "grade", return_value=resolved): + stored["_dir"] = trial + bench_regrade.regrade_trial(stored, self.out / "dest2", self.scenario("tiny"), WRIGHT) + self.assertEqual(stored["invalid"], "an agent-side reason") + self.assertEqual(stored["status"], "invalid") + + def test_same_named_inputs_fail_before_any_copy(self): + # `//...` collides for same-named inputs; the failure + # must fire upfront, not mid-copy into a partial `--out`. + self.stored_trial("1", bench_grade.grader_hash(self.scenario("tiny"))) + second = self.out / "other" / "runs" / "tiny" / "fake" / "none-none_off-1" + second.mkdir(parents=True) + (second / "result.json").write_text((self.out / "runs" / "tiny" / "fake" / "none-none_off-1" / "result.json").read_text()) + with self.assertRaises(SystemExit): + bench_regrade.regrade([self.out / "runs", self.out / "other" / "runs"], WRIGHT, self.scenario, self.out / "regraded") + self.assertFalse((self.out / "regraded").exists()) + def test_report_regrade_leaves_no_scratch_inside_the_run_directory(self): current = bench_grade.grader_hash(self.scenario("tiny")) trial = self.stored_trial("1", current) diff --git a/docs/agent-benchmark-howto.md b/docs/agent-benchmark-howto.md index 1bfd40ce..c0d8c4e1 100644 --- a/docs/agent-benchmark-howto.md +++ b/docs/agent-benchmark-howto.md @@ -115,9 +115,11 @@ scenario or the grader, before reporting on older run directories: python3 benchmarks/agent/agent_bench.py regrade ~/.local/share/wright-agent-bench/runs/ --out ``` -Every trial is copied under `--out` — the input run directories are never -written — and only the trials whose recorded grader hash differs are graded -again with the current scenario and `--wright` binary. A re-graded +Every completed trial — one carrying a parseable contract `result.json` — is +copied under `--out` (a trial dir without one was never finished and is +skipped); the input run directories are never written, and only the trials +whose recorded grader hash differs are graded again with the current +scenario and `--wright` binary. A re-graded `result.json` keeps everything the agent produced (transcript, trace, usage) and records the earlier hash as `grader.regradedFrom`. Point `report`, `score`, `leaderboard`, `compare`, or `publish` at `--out` exactly as at a run