fix: harden benchmark gate per PR #91 review

Upload benchmark-results.json even when the gate fails; add BenchmarkDataError
handling to reduce_baselines; require explicit groups key; expose --threshold
CLI flag; add reduce_baselines and boundary tests; clarify README.

Author: clean6378-max-it

.github/workflows/ci.yml

@@ -239,6 +239,7 @@ jobs:
         run: python scripts/check_benchmark_regression.py benchmark-results.json benchmarks/baselines.json
 
       - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
+        if: always()
         with:
           name: benchmark-results
           path: benchmark-results.json

Makefile

@@ -1,4 +1,4 @@
-.PHONY: update-baselines check-benchmarks
+.PHONY: update-baselines check-benchmarks clean-benchmark-artifacts
 
 update-baselines:
 	pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json -o addopts=
@@ -7,3 +7,6 @@ update-baselines:
 check-benchmarks:
 	pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmark-results.json -o addopts=
 	python scripts/check_benchmark_regression.py benchmark-results.json benchmarks/baselines.json
+
+clean-benchmark-artifacts:
+	rm -f benchmarks/_raw.json benchmark-results.json

benchmarks/README.md

@@ -53,4 +53,4 @@ pytest tests/benchmarks/ --benchmark-only --benchmark-json=benchmarks/_raw.json
 python scripts/reduce_baselines.py benchmarks/_raw.json benchmarks/baselines.json
 ```
 
-Do not capture baselines on Windows/macOS for commit — ubuntu runners are slower and the gate will fail. Download `benchmark-results.json` from a green CI artifact to seed baselines if needed.
+Baselines must be captured on **ubuntu-latest** to match the gated CI runner. Cross-OS variance causes spurious failures. Download `benchmark-results.json` from a CI artifact to seed baselines if needed.

scripts/check_benchmark_regression.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import argparse
 import json
 import sys
 from pathlib import Path
@@ -50,9 +51,11 @@ def load_baseline_means(baselines_path: str | Path) -> dict[str, float]:
     if not isinstance(data, dict):
         raise BenchmarkDataError(f"{path} root value must be an object")
 
-    groups = data.get("groups", data)
+    if "groups" not in data:
+        raise BenchmarkDataError(f"{path} missing required 'groups' key")
+    groups = data["groups"]
     if not isinstance(groups, dict):
-        raise BenchmarkDataError(f"{path} missing 'groups' object")
+        raise BenchmarkDataError(f"{path} 'groups' must be an object")
 
     means: dict[str, float] = {}
     for group_name, value in groups.items():
@@ -104,14 +107,25 @@ def check_regression(
 
 
 def main(argv: list[str] | None = None) -> int:
-    argv = sys.argv[1:] if argv is None else argv
-    if len(argv) != 2:
-        print(
-            "usage: check_benchmark_regression.py <results.json> <baselines.json>",
-            file=sys.stderr,
+    parser = argparse.ArgumentParser(description=__doc__)
+    parser.add_argument("results_path", help="pytest-benchmark --benchmark-json output")
+    parser.add_argument("baselines_path", help="path to benchmarks/baselines.json")
+    parser.add_argument(
+        "--threshold",
+        type=float,
+        default=THRESHOLD,
+        help="fail when current mean exceeds baseline by more than this ratio (default: 1.20)",
+    )
+    args = parser.parse_args(argv)
+    try:
+        return check_regression(
+            args.results_path,
+            args.baselines_path,
+            threshold=args.threshold,
         )
+    except BenchmarkDataError as exc:
+        print(f"ERROR: {exc}", file=sys.stderr)
         return 2
-    return check_regression(argv[0], argv[1])
 
 
 if __name__ == "__main__":

scripts/reduce_baselines.py

@@ -8,6 +8,8 @@
 from datetime import UTC, datetime
 from pathlib import Path
 
+from scripts.check_benchmark_regression import BenchmarkDataError
+
 GATED_GROUPS = ("parse", "export", "search")
 
 
@@ -24,13 +26,34 @@ def reduce_baselines(
     *,
     slack: float = 1.0,
 ) -> dict[str, object]:
-    raw = json.loads(Path(raw_path).read_text(encoding="utf-8"))
+    path = Path(raw_path)
+    try:
+        raw = json.loads(path.read_text(encoding="utf-8"))
+    except json.JSONDecodeError as exc:
+        raise BenchmarkDataError(f"invalid JSON in {path}: {exc}") from exc
+
+    try:
+        entries = raw["benchmarks"]
+    except (KeyError, TypeError) as exc:
+        raise BenchmarkDataError(f"{path} missing top-level 'benchmarks' array") from exc
+    if not isinstance(entries, list):
+        raise BenchmarkDataError(f"{path} 'benchmarks' must be an array")
+
     groups: dict[str, dict[str, float]] = {group: {} for group in GATED_GROUPS}
-    for entry in raw["benchmarks"]:
+    for index, entry in enumerate(entries):
+        if not isinstance(entry, dict):
+            raise BenchmarkDataError(f"{path} benchmarks[{index}] must be an object")
+        try:
+            name = entry["name"]
+            mean = float(entry["stats"]["mean"])
+        except (KeyError, TypeError, ValueError) as exc:
+            raise BenchmarkDataError(
+                f"{path} benchmarks[{index}] missing 'name' or 'stats.mean'"
+            ) from exc
         group = entry.get("group")
         if group not in GATED_GROUPS:
             continue
-        groups[group][entry["name"]] = float(entry["stats"]["mean"]) * slack
+        groups[group][str(name)] = mean * slack
 
     machine_info = raw.get("machine_info", {})
     output: dict[str, object] = {
@@ -39,8 +62,8 @@ def reduce_baselines(
         "machine": machine_info.get("system"),
         "groups": groups,
     }
-    path = Path(out_path)
-    path.write_text(json.dumps(output, indent=2) + "\n", encoding="utf-8")
+    out = Path(out_path)
+    out.write_text(json.dumps(output, indent=2) + "\n", encoding="utf-8")
     return output
 
 
@@ -55,7 +78,11 @@ def main(argv: list[str] | None = None) -> int:
         help="multiply means by this factor (must be > 0)",
     )
     args = parser.parse_args(argv)
-    reduce_baselines(args.raw_path, args.out_path, slack=args.slack)
+    try:
+        reduce_baselines(args.raw_path, args.out_path, slack=args.slack)
+    except BenchmarkDataError as exc:
+        print(f"ERROR: {exc}", file=sys.stderr)
+        return 2
     return 0
 
 

tests/test_check_benchmark_regression.py

@@ -105,3 +105,45 @@ def test_zero_baseline_skips_ratio_check(tmp_path, capsys: pytest.CaptureFixture
 
     assert check_regression(results, baselines) == 0
     assert "baseline for 'test_parse_session_small' is zero" in capsys.readouterr().out
+
+
+def test_exactly_at_threshold_passes(tmp_path) -> None:
+    results = tmp_path / "results.json"
+    baselines = tmp_path / "baselines.json"
+    _write_results(
+        results,
+        [{"name": "test_parse_session_small", "stats": {"mean": 0.00012}}],
+    )
+    _write_baselines(
+        baselines,
+        {"parse": {"test_parse_session_small": 0.0001}},
+    )
+
+    assert check_regression(results, baselines) == 0
+
+
+def test_missing_current_result_warns_without_failing(
+    tmp_path, capsys: pytest.CaptureFixture[str]
+) -> None:
+    results = tmp_path / "results.json"
+    baselines = tmp_path / "baselines.json"
+    _write_results(results, [])
+    _write_baselines(
+        baselines,
+        {"parse": {"test_parse_session_small": 0.0001}},
+    )
+
+    assert check_regression(results, baselines) == 0
+    assert "no current result for baseline" in capsys.readouterr().out
+
+
+def test_main_reports_benchmark_data_error(tmp_path, capsys: pytest.CaptureFixture[str]) -> None:
+    from scripts.check_benchmark_regression import main
+
+    bad = tmp_path / "bad.json"
+    bad.write_text("{}", encoding="utf-8")
+    baselines = tmp_path / "baselines.json"
+    _write_baselines(baselines, {"parse": {"test_parse_session_small": 0.0001}})
+
+    assert main([str(bad), str(baselines)]) == 2
+    assert "ERROR:" in capsys.readouterr().err

tests/test_reduce_baselines.py

@@ -0,0 +1,78 @@
+"""Tests for scripts/reduce_baselines.py."""
+
+from __future__ import annotations
+
+import json
+
+import pytest
+
+from scripts.check_benchmark_regression import BenchmarkDataError
+from scripts.reduce_baselines import reduce_baselines
+
+
+def _write_raw(path, benchmarks: list[dict], *, machine: str = "Linux") -> None:
+    path.write_text(
+        json.dumps(
+            {
+                "machine_info": {"system": machine},
+                "benchmarks": benchmarks,
+            },
+            indent=2,
+        ),
+        encoding="utf-8",
+    )
+
+
+def test_reduce_baselines_writes_gated_groups_only(tmp_path) -> None:
+    raw = tmp_path / "raw.json"
+    out = tmp_path / "baselines.json"
+    _write_raw(
+        raw,
+        [
+            {"group": "parse", "name": "test_parse_session_small", "stats": {"mean": 0.0001}},
+            {"group": "cache", "name": "test_cache_warm_hit", "stats": {"mean": 1e-05}},
+        ],
+    )
+
+    output = reduce_baselines(raw, out)
+
+    assert output["machine"] == "Linux"
+    assert "test_parse_session_small" in output["groups"]["parse"]
+    assert "cache" not in output["groups"]
+    written = json.loads(out.read_text(encoding="utf-8"))
+    assert "test_cache_warm_hit" not in written["groups"]["parse"]
+
+
+def test_reduce_baselines_applies_slack(tmp_path) -> None:
+    raw = tmp_path / "raw.json"
+    out = tmp_path / "baselines.json"
+    _write_raw(
+        raw,
+        [{"group": "search", "name": "test_search_full_corpus", "stats": {"mean": 0.002}}],
+    )
+
+    reduce_baselines(raw, out, slack=1.5)
+    data = json.loads(out.read_text(encoding="utf-8"))
+
+    assert data["groups"]["search"]["test_search_full_corpus"] == pytest.approx(0.003)
+
+
+def test_reduce_baselines_rejects_missing_benchmarks_key(tmp_path) -> None:
+    raw = tmp_path / "raw.json"
+    raw.write_text("{}", encoding="utf-8")
+
+    with pytest.raises(BenchmarkDataError, match="'benchmarks' array"):
+        reduce_baselines(raw, tmp_path / "out.json")
+
+
+def test_reduce_baselines_cli_rejects_non_positive_slack(tmp_path) -> None:
+    from scripts.reduce_baselines import main
+
+    raw = tmp_path / "raw.json"
+    _write_raw(
+        raw,
+        [{"group": "parse", "name": "test_parse_session_small", "stats": {"mean": 0.0001}}],
+    )
+
+    with pytest.raises(SystemExit):
+        main([str(raw), str(tmp_path / "out.json"), "--slack", "0"])