evals: prevent silent overwrites in results writer artifacts

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")
 
 

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()