Bläddra i källkod

fix: harden benchmark bundle publication

Problem: report publication could delete a completed backing bundle after a symlink was created but before publication state was recorded, and explicit timestamps could escape the output directory.

Risk: ambiguous symlink failures now intentionally retain a complete hidden backing bundle, which may require later manual cleanup when no public link was created.
zhenyu.hu 2 veckor sedan
förälder
incheckning
9da8b079a6
2 ändrade filer med 158 tillägg och 15 borttagningar
  1. 22 6
      src/agent_lab/infrastructure/benchmark_reporting.py
  2. 136 9
      tests/test_benchmark_reporting.py

+ 22 - 6
src/agent_lab/infrastructure/benchmark_reporting.py

@@ -24,6 +24,10 @@ from agent_lab.application.benchmark import (
 REPORT_SCHEMA_VERSION = 1
 ERROR_LIMIT = 2048
 REDACTED = "[REDACTED]"
+_SAFE_REPORT_TIMESTAMP = re.compile(
+    r"[A-Za-z0-9][A-Za-z0-9._-]{0,127}\Z",
+    re.ASCII,
+)
 
 
 class _StrictReportModel(BaseModel):
@@ -506,14 +510,12 @@ def write_benchmark_report(
     *,
     timestamp: str | None = None,
 ) -> tuple[Path, Path]:
+    stem = _report_stem(report, timestamp)
     directory = Path(output_dir)
     bundle_dir: Path | None = None
-    published = False
+    publication_started = False
     try:
         directory.mkdir(parents=True, exist_ok=True)
-        stem = timestamp or report.generated_at.astimezone(timezone.utc).strftime(
-            "%Y%m%dT%H%M%SZ"
-        )
         bundle_dir = Path(
             tempfile.mkdtemp(
                 prefix=f".{stem}.",
@@ -529,19 +531,20 @@ def write_benchmark_report(
         while True:
             candidate = stem if suffix == 0 else f"{stem}-{suffix}"
             published_dir = directory / candidate
+            publication_started = True
             try:
                 os.symlink(bundle_dir.name, published_dir)
             except FileExistsError:
+                publication_started = False
                 suffix += 1
                 continue
-            published = True
             _fsync_directory(directory)
             return (
                 published_dir / "report.json",
                 published_dir / "report.md",
             )
     except BaseException as exc:
-        if bundle_dir is not None and not published:
+        if bundle_dir is not None and not publication_started:
             shutil.rmtree(bundle_dir, ignore_errors=True)
         if not isinstance(exc, Exception):
             raise
@@ -550,6 +553,19 @@ def write_benchmark_report(
         ) from exc
 
 
+def _report_stem(
+    report: BenchmarkReport,
+    timestamp: str | None,
+) -> str:
+    if timestamp is None:
+        return report.generated_at.astimezone(timezone.utc).strftime(
+            "%Y%m%dT%H%M%SZ"
+        )
+    if _SAFE_REPORT_TIMESTAMP.fullmatch(timestamp) is None:
+        raise BenchmarkReportWriteError("invalid benchmark report timestamp")
+    return timestamp
+
+
 def _write_text(path: Path, content: str) -> None:
     with path.open("x", encoding="utf-8") as handle:
         handle.write(content)

+ 136 - 9
tests/test_benchmark_reporting.py

@@ -744,6 +744,98 @@ def test_bundle_is_complete_and_matching_before_single_publication(
     _assert_no_publication_artifacts(tmp_path)
 
 
+def test_interrupt_after_symlink_creation_keeps_resolvable_complete_bundle(
+    tmp_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+):
+    reporting = _reporting_module()
+    real_symlink = reporting.os.symlink
+    timestamp = "20260714T011224Z-interrupted"
+
+    def publish_then_interrupt(source: str, destination: Path) -> None:
+        real_symlink(source, destination)
+        raise KeyboardInterrupt
+
+    monkeypatch.setattr(reporting.os, "symlink", publish_then_interrupt)
+
+    with pytest.raises(KeyboardInterrupt):
+        reporting.write_benchmark_report(
+            _marker_report("interrupt-after-symlink"),
+            tmp_path,
+            timestamp=timestamp,
+        )
+
+    published_dir = tmp_path / timestamp
+    assert published_dir.is_symlink()
+    assert published_dir.resolve(strict=True).is_dir()
+    assert (
+        json.loads(published_dir.joinpath("report.json").read_text())["target"][
+            "model"
+        ]
+        == "interrupt-after-symlink"
+    )
+    assert "- Model: `interrupt-after-symlink`" in (
+        published_dir.joinpath("report.md").read_text()
+    )
+    _assert_no_publication_artifacts(tmp_path)
+
+
+@pytest.mark.parametrize(
+    "raw_timestamp",
+    [
+        pytest.param("<absolute>", id="absolute-path"),
+        pytest.param("../outside-report", id="parent-traversal"),
+        pytest.param("segment/report", id="forward-slash"),
+        pytest.param(r"segment\report", id="backslash"),
+        pytest.param(".", id="dot-segment"),
+        pytest.param("..", id="double-dot-segment"),
+        pytest.param("", id="empty"),
+        pytest.param(" ", id="space"),
+        pytest.param("\t", id="tab"),
+        pytest.param("2026\n0714", id="control-newline"),
+        pytest.param("2026\x00714", id="control-nul"),
+        pytest.param("-20260714", id="non-alnum-prefix-hyphen"),
+        pytest.param("_20260714", id="non-alnum-prefix-underscore"),
+        pytest.param("2026\u00a00714", id="unicode-whitespace"),
+        pytest.param("2026\uff0f0714", id="unicode-fullwidth-slash"),
+        pytest.param("2026\u22150714", id="unicode-division-slash"),
+        pytest.param("20260714", id="unicode-fullwidth-digits"),
+        pytest.param("2026\u202ejson", id="unicode-bidi-control"),
+        pytest.param("a" * 129, id="too-long"),
+    ],
+)
+def test_invalid_explicit_timestamp_is_rejected_before_any_filesystem_write(
+    tmp_path: Path,
+    raw_timestamp: str,
+):
+    reporting = _reporting_module()
+    output_dir = tmp_path / "reports"
+    timestamp = (
+        str(tmp_path / "outside-absolute")
+        if raw_timestamp == "<absolute>"
+        else raw_timestamp
+    )
+    entries_before = sorted(
+        path.relative_to(tmp_path) for path in tmp_path.rglob("*")
+    )
+
+    with pytest.raises(
+        reporting.BenchmarkReportWriteError,
+        match="invalid benchmark report timestamp",
+    ):
+        reporting.write_benchmark_report(
+            _marker_report("invalid-timestamp"),
+            output_dir,
+            timestamp=timestamp,
+        )
+
+    entries_after = sorted(
+        path.relative_to(tmp_path) for path in tmp_path.rglob("*")
+    )
+    assert entries_after == entries_before
+    assert not output_dir.exists()
+
+
 def test_report_files_and_bundle_directory_are_fsynced_before_publication(
     tmp_path: Path,
     monkeypatch: pytest.MonkeyPatch,
@@ -751,18 +843,30 @@ def test_report_files_and_bundle_directory_are_fsynced_before_publication(
     reporting = _reporting_module()
     real_fsync = reporting.os.fsync
     real_symlink = reporting.os.symlink
-    fsync_calls = 0
+    fsynced_objects: list[tuple[int, int]] = []
     symlink_calls = 0
 
     def record_fsync(file_descriptor: int) -> None:
-        nonlocal fsync_calls
-        fsync_calls += 1
+        file_stat = reporting.os.fstat(file_descriptor)
+        fsynced_objects.append((file_stat.st_dev, file_stat.st_ino))
         real_fsync(file_descriptor)
 
     def inspect_before_symlink(source: str, destination: Path) -> None:
         nonlocal symlink_calls
         symlink_calls += 1
-        assert fsync_calls == 3
+        destination = Path(destination)
+        bundle_dir = destination.parent / source
+        expected_before_publication = {
+            (
+                bundle_dir.joinpath(name).stat().st_dev,
+                bundle_dir.joinpath(name).stat().st_ino,
+            )
+            for name in ("report.json", "report.md")
+        }
+        bundle_stat = bundle_dir.stat()
+        expected_before_publication.add((bundle_stat.st_dev, bundle_stat.st_ino))
+        assert set(fsynced_objects) == expected_before_publication
+        assert len(fsynced_objects) == 3
         real_symlink(source, destination)
 
     monkeypatch.setattr(reporting.os, "fsync", record_fsync)
@@ -775,7 +879,10 @@ def test_report_files_and_bundle_directory_are_fsynced_before_publication(
     )
 
     assert symlink_calls == 1
-    assert fsync_calls == 4
+    output_stat = tmp_path.stat()
+    assert (output_stat.st_dev, output_stat.st_ino) in fsynced_objects
+    assert len(fsynced_objects) == 4
+    assert len(set(fsynced_objects)) == 4
     _assert_no_publication_artifacts(tmp_path)
 
 
@@ -836,7 +943,7 @@ def test_bundle_fsync_failure_removes_unpublished_private_bundle(
     assert list(tmp_path.iterdir()) == []
 
 
-def test_symlink_failure_removes_unpublished_bundle_without_public_rollback(
+def test_symlink_failure_after_publication_start_keeps_complete_private_bundle(
     tmp_path: Path,
     monkeypatch: pytest.MonkeyPatch,
 ):
@@ -858,10 +965,20 @@ def test_symlink_failure_removes_unpublished_bundle_without_public_rollback(
             timestamp="20260714T011228Z",
         )
 
-    assert list(tmp_path.iterdir()) == []
+    entries = list(tmp_path.iterdir())
+    assert len(entries) == 1
+    bundle_dir = entries[0]
+    assert bundle_dir.name.startswith(".20260714T011228Z.")
+    assert bundle_dir.name.endswith(".bundle")
+    assert json.loads(bundle_dir.joinpath("report.json").read_text())["target"][
+        "model"
+    ] == "symlink-failure"
+    assert "- Model: `symlink-failure`" in bundle_dir.joinpath(
+        "report.md"
+    ).read_text()
 
 
-def test_symlink_interrupt_removes_unpublished_bundle_before_propagating(
+def test_symlink_interrupt_after_publication_start_keeps_private_bundle(
     tmp_path: Path,
     monkeypatch: pytest.MonkeyPatch,
 ):
@@ -880,7 +997,17 @@ def test_symlink_interrupt_removes_unpublished_bundle_before_propagating(
             timestamp="20260714T011229Z",
         )
 
-    assert list(tmp_path.iterdir()) == []
+    entries = list(tmp_path.iterdir())
+    assert len(entries) == 1
+    bundle_dir = entries[0]
+    assert bundle_dir.name.startswith(".20260714T011229Z.")
+    assert bundle_dir.name.endswith(".bundle")
+    assert json.loads(bundle_dir.joinpath("report.json").read_text())["target"][
+        "model"
+    ] == "symlink-interrupt"
+    assert "- Model: `symlink-interrupt`" in bundle_dir.joinpath(
+        "report.md"
+    ).read_text()
 
 
 def test_post_publication_directory_fsync_failure_keeps_public_bundle(