ci: trigger eval-calibration on the whole package (#1160)

.github/workflows/eval-calibration.yml

@@ -25,9 +25,18 @@ on:
   pull_request:
     branches: [main]
     paths:
-      - 'src/aelfrice/eval_harness.py'
-      - 'src/aelfrice/calibration_metrics.py'
-      - 'src/aelfrice/cli.py'
+      # The whole package, not an enumeration of it. `aelf eval` measures
+      # `retrieve()` (eval_harness.py:167,182), so the metric depends on
+      # the retrieval + scoring stack, none of which the previous list
+      # named — a PR editing only `scoring.py` never triggered this job,
+      # merged, and then the unconditional push-to-main run below turned
+      # main red with no owning PR (#1160). Enumerating the reachable
+      # modules is what drifted in the first place, and the coupling is
+      # not stable: the harness pins `l1_limit` and disables the entity
+      # and BFS lanes, so which modules are live moves with the call.
+      # A superset costs a sub-second measurement; a subset costs a red
+      # main. Mirrors `ci.yml`, which filters on the package too.
+      - 'src/aelfrice/**'
       - 'benchmarks/posterior_ranking/**'
       - '.github/workflows/eval-calibration.yml'
       - 'pyproject.toml'

CHANGELOG/v4.md

@@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **The nightly reproducibility band-check reported success when it had measured nothing ([#1160](https://github.com/robotrocketscience/aelfrice/issues/1160)).** `tolerance.summarize` returned PASS whenever no leaf was FAIL or WARN. Ignoring an *individual* SKIP is correct and ratified (#479) — one uncomputable metric is not a regression — but the rollup ignored *every* SKIP, so a run in which nothing could be computed rolled up to PASS. That is precisely the shape a failed dataset download on the runner takes: every adapter exits because its data dir is absent, every leaf becomes SKIP, counts read `{pass: 0, warn: 0, fail: 0, skip: 12}`, and the verdict was PASS. An empty check list returned PASS as well, and `bench-canonical.yml` exited 0 on anything that was not `fail`, so the cron went green having compared nothing at all. PASS is a claim that something was measured and stayed in band, so it now requires at least one leaf that actually passed; otherwise the rollup is a new `NO_DATA` verdict, and the workflow exits 1 on it. #479's behaviour is preserved exactly — a SKIP beside any real PASS still rolls up to PASS, and FAIL and WARN still dominate, because a leaf that was compared to its band is evidence that measurement happened (so WARN with no PASS stays WARN, not `NO_DATA`). `NO_DATA` is a distinct verdict rather than reusing FAIL because the two demand different responses: a regression means read the diff, no data means fix the runner. Leaf tallying also moved to `.get` so a leaf carrying an unexpected verdict is counted and reported instead of raising `KeyError` inside the gate whose job is to report it. **Not covered:** the same acceptance criterion also asks for one-sided bands so that an improvement is not classified as a failure. That needs a per-metric direction table — quality metrics improve upward, latency metrics downward — and guessing a direction would make the gate blind to regressions in the wrong direction, which is the same class of defect this work exists to remove. Left for a deliberate decision rather than inferred from metric names.
 - **Every latency benchmark in the suite was unreachable, and one could not have passed ([#1160](https://github.com/robotrocketscience/aelfrice/issues/1160)).** Five perf tests across four modules (`test_bm25_index.py`, `test_heat_kernel.py`, `test_graph_spectral.py`, `test_hrr_struct_index.py`) gate a wall-clock assertion on a `_has_run_perf` helper reading `--run-perf` — an option **nothing registered**. `test_bm25_index.py` carried a `pytest_addoption` stub, but pytest only invokes that hook from `conftest.py` or an installed plugin, never from a plain test module, so `getoption` always raised, every helper fell back to `False`, and `pytest --run-perf` exited 4 with `unrecognized arguments`. The tests were reachable only by hand-editing the guard, which means no measurement of retrieval latency at realistic store size had run in a long time. Registering the option in `tests/conftest.py` then exposed a second, independent defect: `pyproject.toml` sets a global `timeout = 5` sized for unit tests, while each perf test asserts its own larger budget, so pytest-timeout — not the assertion — decided the outcome. `test_eigsolve_under_budget_n10k` asserts `elapsed < 30.0` and measures 6.76 s, and was killed at 5.0 s inside the ARPACK solve: a guaranteed failure, with the test's own comment already reading "allow 30s for noisy hosts". Two more were latent rather than broken — hrr's `test_build_latency_at_n_10k` asserts `elapsed <= 5.0`, *exactly* the global cap, so it could never pass at its own boundary, and bm25's `test_score_latency_under_5ms_n50k` spends 3.07 s building a 50k-belief store before it starts measuring, leaving under 2 s of headroom on a slower host for an assertion about per-query milliseconds. Each perf test now carries a `@pytest.mark.timeout` override at 120 s, the per-test convention `pyproject.toml:125-127` already documents; the bound is deliberately generous because it is only a hang guard, while the budget remains the assertion. All five pass with `--run-perf` (14.87 s together) and still skip without it, so default behaviour is unchanged. A new guard pins both failure modes: the registration check is behavioural rather than source-level (`config.getoption` raises if the option is absent), and the budget check walks the AST for every test calling `_has_run_perf` and compares its timeout override against the `timeout` read from `pyproject.toml`. **Not covered:** no CI job runs `--run-perf`. Wiring one is deliberately left to an operator call, because these are wall-clock assertions on shared runners and this repo has a documented history of latency tests false-failing under load; a nightly perf job is the natural home if the noise is acceptable.
 - **PRs touching only `benchmarks/` or `scripts/` reported a passing pytest matrix without running any tests ([#1160](https://github.com/robotrocketscience/aelfrice/issues/1160)).** `ci.yml` deliberately carries no `paths-ignore`, so the workflow always runs and the required `pytest (3.12)` / `pytest (3.13)` checks report on every PR including docs-only ones (#413/#427); which PRs actually execute the suite is decided inside the job by a `dorny/paths-filter` glob list. That list covered `src/**` and `tests/**` but not `benchmarks/**` or `scripts/**`, while `pythonpath = ["."]` makes both importable from the suite — 27 test modules import `benchmarks` and 6 load a `scripts/` module through `importlib`. A PR editing only those directories therefore took the `echo "No code paths changed"` branch and turned both required checks green having run nothing. The coverage it skipped is real, not nominal: widening `compute_band`'s absolute floor 2× in `benchmarks/tolerance.py` fails two tests in `test_bench_tolerance.py`, and breaking `scripts/check_migration_policy.py` errors 14 in `test_migration_policy.py` — the gate that guards destructive schema migrations. Both would have merged green. `docs/` and `CHANGELOG/` stay out of the filter deliberately, and that exclusion was checked rather than assumed: truncating `README.md` to a quarter of its length leaves the full suite passing, so no test asserts on prose and the docs-only exemption costs no coverage. The new guard **derives** its expectation instead of hand-listing names — it scans `tests/` for top-level in-repo packages actually imported or path-resolved and asserts each is matched by a glob, so a package the suite starts using fails the test until the filter covers it. Its own vacuity is guarded too (an empty scan would satisfy the coverage assertion for free, and the filter parser refuses to run unless it finds exactly one `code:` key), because a hand-maintained list compared against the constants it was copied from is the tautology #1161 hit on the doctor side. One acceptance criterion of the #1160 umbrella; the others remain open.
+- **The `eval-calibration` gate never ran on the code it calibrates ([#1160](https://github.com/robotrocketscience/aelfrice/issues/1160)).** The job pins `aelf eval --json` byte-for-byte against `benchmarks/posterior_ranking/baseline.json`, but its PR `paths:` filter named only `eval_harness.py`, `calibration_metrics.py` and `cli.py` — while the metric is produced by `retrieve()` (`eval_harness.py:167` imports it, `:182` calls it), so the entire retrieval and scoring stack sat outside the trigger. Because the `push:` trigger carries no `paths:` key, that asymmetry converted a skipped check into a broken main: a PR editing only `scoring.py` never ran the job, merged, and the post-merge run then failed the baseline assertion with no owning PR to revert. The hidden coverage is real — negating the bm25 term in `scoring.py` moves `roc_auc` from 0.8444 to 0.7347 and `spearman_rho` from 0.5241 to 0.3572, which the byte-exact assertion catches the moment it is allowed to run. Now filters on `src/aelfrice/**`, matching what `ci.yml` already does, rather than a longer enumeration: the enumeration is what drifted, and the coupling is not stable enough to enumerate safely — the harness pins `l1_limit` and passes `entity_index_enabled=False, bfs_enabled=False`, so `DEFAULT_L1_LIMIT` and `DEFAULT_K1` are both unreachable today and which modules are live moves with the call. Over-triggering costs a 0.45–0.68 s measurement; under-triggering costs a red main. The accompanying guard walks imports from the harness (via `ast.walk`, since the `retrieve` import sits inside a function body — a top-level-only scan misses exactly the dependency at issue) and asserts all 25 reachable modules match the filter, with its own vacuity pinned so an empty walk cannot satisfy the assertion for free. **Not fixed here:** the same gate is still blind to the posterior rerank it is named for — `AELFRICE_POSTERIOR_WEIGHT=0.0`, `1.0` and `5.0` all emit byte-identical metrics, because the calibration corpus builds every belief at `alpha=0.5, beta=0.5` and a constant posterior is a constant offset that cannot reorder anything. That is a separate acceptance criterion on the umbrella and needs a corpus change plus a deliberate baseline recut.
 
 ## [4.2.0] - 2026-07-21
 

tests/test_eval_calibration_gate.py

@@ -0,0 +1,191 @@
+"""The eval-calibration trigger must cover everything its metric depends on.
+
+`.github/workflows/eval-calibration.yml` pins `aelf eval --json` output
+byte-for-byte against `benchmarks/posterior_ranking/baseline.json`. It
+runs on PRs behind a `paths:` filter, and unconditionally on push to
+main.
+
+That asymmetry is the hazard. If a PR changes the measured output but
+does not match the PR filter, the job never runs on the PR, the PR
+merges, and the push-to-main run then fails the baseline assertion with
+no owning PR to revert. #1160 found exactly that: the filter named
+`eval_harness.py`, `calibration_metrics.py` and `cli.py`, but the metric
+is produced by `retrieve()` (`eval_harness.py:167,182`), so the whole
+retrieval and scoring stack was outside it.
+
+This test derives the dependency set by walking imports from the
+harness rather than restating a list of module names, since a
+hand-maintained list is what drifted.
+"""
+from __future__ import annotations
+
+import ast
+import re
+from pathlib import Path
+
+_REPO = Path(__file__).resolve().parents[1]
+_WORKFLOW = _REPO / ".github" / "workflows" / "eval-calibration.yml"
+_PACKAGE = _REPO / "src" / "aelfrice"
+_ROOT_MODULE = "eval_harness"
+
+
+def _pr_path_globs() -> list[str]:
+    """Return the `paths:` list under the workflow's `pull_request:` trigger."""
+    lines = _WORKFLOW.read_text(encoding="utf-8").splitlines()
+    starts = [
+        i for i, line in enumerate(lines) if line.strip() == "pull_request:"
+    ]
+    assert len(starts) == 1, (
+        f"expected one `pull_request:` trigger in {_WORKFLOW.name}, found "
+        f"{len(starts)} — a parser that finds nothing would make every "
+        f"assertion here pass for free"
+    )
+    paths_at = None
+    for i in range(starts[0] + 1, len(lines)):
+        stripped = lines[i].strip()
+        if stripped == "paths:":
+            paths_at = i
+            break
+        # a new top-level trigger key (e.g. `push:`) ends the block
+        if stripped and not lines[i].startswith("    "):
+            break
+    assert paths_at is not None, (
+        "the pull_request trigger has no `paths:` filter — if it was dropped "
+        "deliberately the job now runs on every PR, which is also a valid fix "
+        "for #1160; delete this test rather than weakening it"
+    )
+    indent = len(lines[paths_at]) - len(lines[paths_at].lstrip())
+    globs: list[str] = []
+    for line in lines[paths_at + 1 :]:
+        stripped = line.strip()
+        if not stripped or stripped.startswith("#"):
+            continue
+        if len(line) - len(line.lstrip()) <= indent:
+            break
+        matched = re.fullmatch(r"-\s*'([^']+)'", stripped)
+        assert matched, f"unparsed entry in the paths filter: {stripped!r}"
+        globs.append(matched.group(1))
+    return globs
+
+
+def _glob_matches(glob: str, path: str) -> bool:
+    """Match a GitHub-Actions path glob, where `**` spans separators."""
+    pattern = "".join(
+        r"[^/]*" if part == "*" else r".*" if part == "**" else re.escape(part)
+        for part in re.split(r"(\*\*|\*)", glob)
+    )
+    return re.fullmatch(pattern, path) is not None
+
+
+def _path_is_included(globs: list[str], path: str) -> bool:
+    """Whether GitHub would trigger on `path` given `globs`, in order.
+
+    A `paths:` list may negate with `!`, and GitHub evaluates the entries
+    top to bottom: a later negative excludes a path an earlier positive
+    matched, and a later positive re-includes it. Matching with `any()`
+    ignores negation entirely, so `src/aelfrice/**` followed by
+    `!src/aelfrice/foo.py` would read as covering `foo.py` when the job
+    would in fact skip it — the guard failing *open* in exactly the case
+    it exists to catch. There are no negated entries in the filter today;
+    this keeps the guard correct if one is ever added.
+    """
+    included = False
+    for glob in globs:
+        negated = glob.startswith("!")
+        if _glob_matches(glob[1:] if negated else glob, path):
+            included = not negated
+    return included
+
+
+def _reachable_modules() -> set[str]:
+    """`aelfrice.*` module names transitively imported from the harness.
+
+    Walks the AST, so imports written inside a function body — which is
+    how the harness reaches `retrieve` — are found too.
+    """
+    seen: set[str] = set()
+    queue = [_ROOT_MODULE]
+    while queue:
+        name = queue.pop()
+        if name in seen:
+            continue
+        source = _PACKAGE / f"{name}.py"
+        if not source.is_file():
+            continue
+        seen.add(name)
+        tree = ast.parse(source.read_text(encoding="utf-8"))
+        for node in ast.walk(tree):
+            found: list[str] = []
+            if isinstance(node, ast.ImportFrom) and node.module:
+                if node.module.startswith("aelfrice."):
+                    found.append(node.module.split(".", 1)[1])
+            elif isinstance(node, ast.Import):
+                found += [
+                    alias.name.split(".", 1)[1]
+                    for alias in node.names
+                    if alias.name.startswith("aelfrice.")
+                ]
+            queue += [m for m in found if m not in seen]
+    return seen
+
+
+def test_the_import_walk_is_not_vacuous() -> None:
+    """Guard the guard: an empty walk would satisfy the coverage test."""
+    reachable = _reachable_modules()
+    assert {"eval_harness", "retrieval", "scoring"} <= reachable, (
+        f"the import walk reached {len(reachable)} modules and is missing the "
+        f"ones #1160 is about, so the coverage assertion below is vacuous: "
+        f"{sorted(reachable)}"
+    )
+
+
+def test_pr_filter_covers_every_module_the_metric_depends_on() -> None:
+    globs = _pr_path_globs()
+    uncovered = sorted(
+        module
+        for module in _reachable_modules()
+        if not _path_is_included(globs, f"src/aelfrice/{module}.py")
+    )
+    assert not uncovered, (
+        f"{uncovered} are reachable from `aelf eval` but do not match the "
+        f"pull_request paths filter in {_WORKFLOW.name}. A PR touching only "
+        f"those files skips this gate, merges, and then fails the "
+        f"unconditional push-to-main run with no owning PR. Prefer widening "
+        f"to 'src/aelfrice/**' over adding names."
+    )
+
+
+def test_the_push_trigger_stays_unconditional() -> None:
+    """The post-merge re-assertion is what makes a subset filter dangerous.
+
+    Kept as an explicit expectation: if the push trigger ever grows a
+    `paths:` filter, the failure mode this module guards changes shape
+    and the reasoning above needs revisiting.
+    """
+    text = _WORKFLOW.read_text(encoding="utf-8")
+    push_block = text.split("push:", 1)
+    assert len(push_block) == 2, "no `push:` trigger in the workflow"
+    following = push_block[1].split("permissions:", 1)[0]
+    assert "paths:" not in following, (
+        "the push-to-main trigger grew a paths filter; re-check whether a "
+        "PR-side subset filter can still leave main red with no owning PR"
+    )
+
+
+def test_a_negated_glob_excludes_a_path_an_earlier_glob_matched() -> None:
+    """Order matters, and `any()` would get this wrong.
+
+    Pinned as a unit on the helper rather than by editing the workflow,
+    since the filter carries no negation and should not grow one just to
+    be tested.
+    """
+    globs = ["src/aelfrice/**", "!src/aelfrice/scoring.py"]
+    assert _path_is_included(globs, "src/aelfrice/retrieval.py")
+    assert not _path_is_included(globs, "src/aelfrice/scoring.py")
+    # A later positive wins over an earlier negative.
+    assert _path_is_included(
+        ["src/aelfrice/**", "!src/aelfrice/scoring.py", "src/aelfrice/scoring.py"],
+        "src/aelfrice/scoring.py",
+    )
+    # No entry matches at all -> not included.
+    assert not _path_is_included(globs, "docs/README.md")