sequenceDiagram
  participant benchmarks_job as benchmarks job
  participant pytest as pytest
  participant regression_script as scripts/check_benchmark_regression.py
  participant upload_artifact as actions/upload-artifact

  benchmarks_job->>pytest: run tests/benchmarks/ with --benchmark-only
  pytest-->>benchmarks_job: benchmark-results.json
  benchmarks_job->>regression_script: compare benchmark-results.json to benchmarks/baselines.json
  regression_script-->>benchmarks_job: exit code 0, 1, or 2
  benchmarks_job->>upload_artifact: upload benchmark-results.json with if: always()

_{Comment @coderabbitai help to get the list of available commands.}

@CodeRabbit, review this PR in more depth

#!/bin/bash
echo "=== .github/workflows/tests.yml ==="
cat -n .github/workflows/tests.yml

echo ""
echo "=== pyproject.toml ==="
cat -n pyproject.toml

echo ""
echo "=== scripts/check_benchmark_regression.py ==="
cat -n scripts/check_benchmark_regression.py

echo ""
echo "=== tests/benchmarks/conftest.py ==="
cat -n tests/benchmarks/conftest.py

echo ""
echo "=== tests/benchmarks/test_summary_cache_bench.py ==="
cat -n tests/benchmarks/test_summary_cache_bench.py

echo ""
echo "=== benchmarks/baselines.json ==="
cat -n benchmarks/baselines.json

echo ""
echo "=== .gitignore (last 20 lines) ==="
tail -20 .gitignore

echo ""
echo "=== requirements-lock.txt ==="
cat -n requirements-lock.txt

echo ""
echo "=== services/summary_cache.py ==="
cat -n services/summary_cache.py

echo ""
echo "=== Directory tree of tests/benchmarks/ ==="
tree tests/benchmarks/ 2>/dev/null || find tests/benchmarks/ -type f

echo ""
echo "=== Directory tree of scripts/ ==="
tree scripts/ 2>/dev/null || find scripts/ -type f

echo ""
echo "=== benchmarks/ directory ==="
tree benchmarks/ 2>/dev/null || find benchmarks/ -type f

@clean6378-max-it Here is a deep review of PR #120.

Deep Review: PR #120 — pytest-benchmark suite for summary_cache.py

The PR is well-structured overall — no production code is touched, the fixture isolation via monkeypatch is correct, and the regression gate script has solid error-handling. However, there are several issues ranging from critical to minor that should be addressed before merge.

🔴 Critical

1. Baseline name format may never match pytest-benchmark output (benchmarks/baselines.json + scripts/check_benchmark_regression.py)

The regression gate in check_regression() (Line 96) looks up current results by name from the baselines. The baselines use short names like test_summary_cache_hit and test_fingerprint_workspace_entries[10].

pytest-benchmark emits the name field as the short test node name (e.g. test_summary_cache_hit) in some configurations, but in others it emits the full node ID including the module path (tests/benchmarks/test_summary_cache_bench.py::test_summary_cache_hit). If the format doesn't match, every baseline entry will show as MISSING and the gate will always fail (exit code 1) — or always pass if the lookup silently misses.

You should verify the exact format by inspecting an actual benchmark-results.json produced by the CI job and cross-referencing the name field against the keys in baselines.json. Consider adding a normalization step in load_results() that strips the module prefix:

# In load_results(), after extracting `name`:
# Normalize "tests/benchmarks/test_x.py::test_foo[param]" → "test_foo[param]"
name = str(name).rsplit("::", 1)[-1]

2. Baselines appear manually fabricated — future-dated and unverified (benchmarks/baselines.json)

"updated": "2026-06-25T12:00:00Z",

The timestamp is in the future and the _note claims these values come from "ubuntu-latest CI benchmark-results.json", but there is no CI artifact proving that. Values like 6.3e-05 for both cache hit and cache miss (identical to 6 decimal places) suggest they were hand-written, not measured. If these baselines are too tight for a slower runner, the gate will spuriously fail; if too loose, the gate provides no real protection.

Recommendation: Do not commit hand-crafted baselines. Run the benchmarks in CI first, capture the artifact, update the file from the actual numbers, and commit that as a follow-up.

3. benchmarks CI job has no needs: dependency on unittest (.github/workflows/tests.yml Line 219)

The benchmark job runs in parallel with unit tests. If unit tests fail due to a production code regression, benchmarks can still be reported as "passing" — giving a misleading green signal.

# Add this to the benchmarks job:
needs: [unittest]

🟡 Medium

4. addopts = "--benchmark-disable" is missing from pyproject.toml (Line 39–43)

The PR objectives state: "adds default benchmark skipping for normal test runs." But the [tool.pytest.ini_options] section has no addopts at all:

[tool.pytest.ini_options]
testpaths = ["tests"]
markers = [
    "benchmark: performance benchmarks (pytest-benchmark)",
]

Since testpaths includes tests/, tests/benchmarks/ is collected on every plain pytest invocation. Without --benchmark-disable, pytest-benchmark will execute benchmark functions normally (one call each, with timing overhead). This slows down the developer loop and contradicts the stated goal.

[tool.pytest.ini_options]
testpaths = ["tests"]
addopts = "--benchmark-disable"
markers = [
    "benchmark: performance benchmarks (pytest-benchmark)",
]

Note: the CI benchmark job already overrides this with -o addopts=, so adding it won't break CI.

5. sys.path manipulation in conftest.py is fragile (Lines 12–14)

REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
if REPO_ROOT not in sys.path:
    sys.path.insert(0, REPO_ROOT)

This is three levels of dirname that breaks silently if the file moves. The idiomatic solution is to install the package in editable mode in CI (pip install -e .) and remove the manual path insertion entirely. Alternatively, the [tool.pytest.ini_options] section can set pythonpath = ["."] (pytest ≥ 7.0):

[tool.pytest.ini_options]
pythonpath = ["."]

6. pytest-benchmark not pinned in requirements-lock.txt (.github/workflows/tests.yml Line 236)

python -m pip install 'pytest>=8,<9' 'pytest-benchmark>=4,<5'

All other runtime deps are resolved from requirements-lock.txt for determinism, but pytest-benchmark is installed ad-hoc. A transitive dep (e.g. py-cpuinfo, attrs) could change versions silently. pytest-benchmark should be pinned in the lock file, or at minimum pinned to a specific patch version in the workflow.

7. stale_fingerprint relies on an undocumented assumption (tests/benchmarks/conftest.py Line 88–89)

`@pytest.fixture`
def stale_fingerprint(workspace_fingerprint: dict[str, Any]) -> dict[str, Any]:
    return {**workspace_fingerprint, "global_db_mtime_ns": 9_999_999_999}

This works because global_db_path=None causes _file_mtime_ns(None) to return None, making the real fingerprint's global_db_mtime_ns equal to None. Setting it to 9_999_999_999 creates a mismatch. But this is completely implicit — if fingerprint_workspace_storage ever changes its handling of None paths, this fixture silently stops simulating a miss.

A more robust approach is to use a sentinel that always differs:

`@pytest.fixture`
def stale_fingerprint(workspace_fingerprint: dict[str, Any]) -> dict[str, Any]:
    """Return a fingerprint guaranteed to differ from the stored one."""
    return {**workspace_fingerprint, "rules_digest": "deadbeefdeadbeef"}

This works regardless of what global_db_mtime_ns is.

🔵 Minor

8. Missing tests/benchmarks/__init__.py

tests/benchmarks/
├── conftest.py
└── test_summary_cache_bench.py

The directory has no __init__.py. While modern pytest handles this correctly, the rest of tests/ likely uses __init__.py files (consistent with python -m unittest discover). Adding one avoids potential namespace collisions if identically-named modules exist elsewhere.

9. load_baseline_means silently skips non-dict group values (scripts/check_benchmark_regression.py Line 69)

if not isinstance(value, dict):
    continue  # silently skips

A typo in baselines.json (e.g. a string value for a group) is silently ignored, which could cause a group's benchmarks to never be gated. Consider issuing a warning or raising BenchmarkDataError.

10. Redundant str() cast in load_results (scripts/check_benchmark_regression.py Line 43)

name = str(name)  # name was already set from entry["name"] above

name is set on Line 37 directly from entry["name"], which would have been accepted as str already. The redundant cast can be removed.

11. Missing blank line before first test function (tests/benchmarks/test_summary_cache_bench.py Line 16)

PEP 8 requires two blank lines between the import block and the first top-level definition:

from services.summary_cache import (
    fingerprint_workspace_storage,
    get_cached_projects,
    set_cached_projects,
)
                                         # ← two blank lines needed here
`@pytest.mark.benchmark`(group="summary-cache")
def test_summary_cache_hit(...):

12. No unit tests for check_benchmark_regression.py

The regression gate script has good internal logic (load_results, load_baseline_means, check_regression) but zero test coverage. Given that this script is the only thing blocking regressions in CI, a small test file (e.g. tests/test_check_benchmark_regression.py) covering the happy path, missing-benchmark, and threshold-exceeded cases would greatly increase confidence.

Summary table