From 58444d849271027bef20a1c52d84066a6606022e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 17 Jun 2026 07:27:55 +0000 Subject: [PATCH] evals: prevent silent overwrites in results writer artifacts Two silent data-loss gaps in the per-case markdown artifacts: - Distinct case names sanitized with re.sub(r'[^\w\-]', '_', name) could collide ('a/b' and 'a:b' both -> 'a_b.md'), the second silently overwriting the first. Track the stems used in each run dir and append a numeric suffix on collision so every case keeps its own file. - Run directories are timestamped to the second with mkdir(exist_ok=True), so two runs of the same suite within one second (or two racing processes) merged their case files. Create with exist_ok=False and bump a numeric suffix until mkdir wins; mkdir is atomic, so this is safe across processes. Closes #119 --- protest/evals/results_writer.py | 55 +++++++++++++++----- tests/evals/test_results_writer.py | 83 ++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 tests/evals/test_results_writer.py diff --git a/protest/evals/results_writer.py b/protest/evals/results_writer.py index 570556d..5f9a9e4 100644 --- a/protest/evals/results_writer.py +++ b/protest/evals/results_writer.py @@ -33,6 +33,7 @@ def __init__(self, history_dir: Path | None = None) -> None: (history_dir / "results") if history_dir else DEFAULT_RESULTS_DIR ) self._run_dirs: dict[str, Path] = {} + self._used_stems: dict[str, set[str]] = {} @classmethod def activate(cls, ctx: PluginContext) -> EvalResultsWriter: @@ -54,7 +55,10 @@ def _maybe_write(self, result: TestResult) -> None: def _write_case_file(self, case_result: EvalCaseResult, suite_name: str) -> None: if suite_name not in self._run_dirs: self._run_dirs[suite_name] = _make_run_dir(suite_name, self._results_base) - _write_case_file(case_result, self._run_dirs[suite_name]) + self._used_stems[suite_name] = set() + _write_case_file( + case_result, self._run_dirs[suite_name], self._used_stems[suite_name] + ) def on_eval_suite_end(self, report: Any) -> None: """Print results dir path for the suite.""" @@ -72,19 +76,46 @@ def on_eval_suite_end(self, report: Any) -> None: def _make_run_dir(suite_name: str, base_dir: Path | None = None) -> Path: - """Create and return the timestamped directory for this run.""" + """Create and return a unique timestamped directory for this run. + + The timestamp has one-second resolution, so two runs of the same suite + within the same second (or two processes racing) would otherwise land on + the same directory and merge their case files. Create with + ``exist_ok=False`` and bump a numeric suffix until mkdir wins - the call + is atomic, so this is also safe across concurrent processes. + """ base = base_dir or DEFAULT_RESULTS_DIR + base.mkdir(parents=True, exist_ok=True) ts = datetime.now(tz=timezone.utc).strftime("%Y%m%d_%H%M%S") - safe_suite = re.sub(r"[^\w\-]", "_", suite_name) - run_dir = base / f"{safe_suite}_{ts}" - run_dir.mkdir(parents=True, exist_ok=True) - return run_dir - - -def _write_case_file(case: EvalCaseResult, run_dir: Path) -> None: - """Write a markdown file for a single eval case.""" - safe_name = re.sub(r"[^\w\-]", "_", case.case_name) - path = run_dir / f"{safe_name}.md" + safe_suite = re.sub(r"[^\w\-]", "_", suite_name) or "evals" + candidate = base / f"{safe_suite}_{ts}" + suffix = 2 + while True: + try: + candidate.mkdir(exist_ok=False) + except FileExistsError: + candidate = base / f"{safe_suite}_{ts}-{suffix}" + suffix += 1 + else: + return candidate + + +def _write_case_file(case: EvalCaseResult, run_dir: Path, used_stems: set[str]) -> None: + """Write a markdown file for a single eval case. + + Distinct case names can sanitize to the same stem (``a/b`` and ``a:b`` + both become ``a_b``), which would silently overwrite the first file. + Track the stems already used in this run dir and disambiguate collisions + with a numeric suffix so every case keeps its own file. + """ + base = re.sub(r"[^\w\-]", "_", case.case_name) or "case" + stem = base + suffix = 2 + while stem in used_stems: + stem = f"{base}-{suffix}" + suffix += 1 + used_stems.add(stem) + path = run_dir / f"{stem}.md" path.write_text(_render_case(case), encoding="utf-8") diff --git a/tests/evals/test_results_writer.py b/tests/evals/test_results_writer.py new file mode 100644 index 0000000..586b5cd --- /dev/null +++ b/tests/evals/test_results_writer.py @@ -0,0 +1,83 @@ +"""Tests for EvalResultsWriter robustness (issue #119). + +Two silent-data-loss gaps in the per-case markdown artifacts: + +- distinct case names that sanitize to the same stem (``a/b`` and ``a:b`` -> + ``a_b``) overwrote each other; +- two runs of the same suite within one second merged into one directory + (timestamp has one-second resolution). +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from typing import TYPE_CHECKING + +from protest.evals.results_writer import EvalResultsWriter, _make_run_dir +from protest.evals.types import EvalCaseResult + +if TYPE_CHECKING: + from pathlib import Path + + +def _case(name: str) -> EvalCaseResult: + return EvalCaseResult( + case_name=name, + node_id=f"mod::{name}", + scores=(), + duration=0.0, + passed=True, + ) + + +class TestCaseNameCollisions: + def test_colliding_sanitized_names_do_not_overwrite(self, tmp_path: Path) -> None: + writer = EvalResultsWriter(history_dir=tmp_path) + writer._write_case_file(_case("a/b"), "suite") + writer._write_case_file(_case("a:b"), "suite") + + run_dir = next((tmp_path / "results").iterdir()) + files = sorted(p.name for p in run_dir.iterdir()) + assert files == ["a_b-2.md", "a_b.md"] + + def test_each_file_keeps_its_own_content(self, tmp_path: Path) -> None: + writer = EvalResultsWriter(history_dir=tmp_path) + writer._write_case_file(_case("a/b"), "suite") + writer._write_case_file(_case("a:b"), "suite") + + run_dir = next((tmp_path / "results").iterdir()) + contents = {(run_dir / name).read_text() for name in ("a_b.md", "a_b-2.md")} + # The original (unsanitized) names survive in the rendered title. + assert any("a/b" in c for c in contents) + assert any("a:b" in c for c in contents) + + def test_empty_name_falls_back_to_case_stem(self, tmp_path: Path) -> None: + # case_name can be "" (payload.case_name or ""); sanitizing yields an + # empty stem, which would produce a bare ".md" - fall back to "case". + writer = EvalResultsWriter(history_dir=tmp_path) + writer._write_case_file(_case(""), "suite") + + run_dir = next((tmp_path / "results").iterdir()) + assert [p.name for p in run_dir.iterdir()] == ["case.md"] + + +class TestRunDirUniqueness: + def test_successive_runs_get_distinct_dirs(self, tmp_path: Path) -> None: + # Three back-to-back calls almost certainly share a one-second + # timestamp, exercising the suffix-bump loop. Regardless of whether a + # second boundary is crossed, each dir must be unique and exist. + dirs = [_make_run_dir("suite", tmp_path) for _ in range(3)] + assert len({str(d) for d in dirs}) == 3 + assert all(d.exists() for d in dirs) + + def test_collision_with_existing_dir_is_suffixed(self, tmp_path: Path) -> None: + # Pre-create the exact directory the next call will compute, forcing + # the FileExistsError branch deterministically. + ts = datetime.now(tz=timezone.utc).strftime("%Y%m%d_%H%M%S") + (tmp_path / f"suite_{ts}").mkdir() + + run_dir = _make_run_dir("suite", tmp_path) + # Either the pre-created second elapsed (new ts) or we got the suffix; + # in the common same-second case the suffix branch is taken. + assert run_dir.name != f"suite_{ts}" + assert run_dir.exists()