Stop the coverage-map bot pushing a no-op commit on every refresh

.github/scripts/check_coverage_map_health.py

@@ -1,5 +1,6 @@
 """Fail loudly if the committed coverage map is stale or under-covers. Used by coverage-health.yml."""
 import datetime
+import subprocess
 import sys
 from pathlib import Path
 
@@ -10,6 +11,26 @@
 MAX_AGE_DAYS = 10
 MIN_FRACTION = 0.80
 
+# Must mirror the `paths:` trigger of coverage-refresh.yml. A superset would raise false
+# alarms (a change that never triggers a refresh would look like a missed refresh).
+COVERAGE_RELEVANT_PATHS = [":(glob)src/**/*.fpp", "toolchain/mfc/test/cases.py"]
+
+
+def built_after_last_change(git_sha):
+    """Was the map built at or after the last coverage-relevant commit? None if unknowable.
+
+    This is the direct question the wall-clock age check only approximated: it stays quiet
+    over a genuinely quiet repo (no refresh needed, so no heartbeat commit needed either)
+    and fires on the next relevant push after a refresh breaks, rather than 10 days later.
+    """
+    if not git_sha:
+        return None
+    last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False)
+    if last.returncode != 0 or not last.stdout.strip():
+        return None  # shallow clone or no such commit -> fall back to the age rule
+    ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False)
+    return {0: True, 1: False}.get(ancestor.returncode)  # anything else -> None (unknown sha, shallow history)
+
 entries, meta = load_map(COVERAGE_MAP_PATH)
 if entries is None:
     sys.exit("Coverage map missing or corrupt.")
@@ -37,6 +58,7 @@
     now=datetime.datetime.now(datetime.timezone.utc).isoformat(),
     max_age_days=MAX_AGE_DAYS,
     min_fraction=MIN_FRACTION,
+    built_after_last_change=built_after_last_change(meta.get("git_sha")),
 )
 print(msg)
 sys.exit(0 if ok else 1)

.github/scripts/coverage_map_changed.py

@@ -0,0 +1,52 @@
+"""Decide whether a rebuilt coverage map is worth committing. Used by coverage-refresh.yml.
+
+The refresh job rebuilds the map on every master push touching src/**/*.fpp or cases.py,
+but the rebuild is usually identical in substance -- only _meta (built_at, git_sha) moves.
+A file-level `git diff` therefore always reports a change, so the bot pushed a no-op commit
+on nearly every run. Compare the coverage entries instead.
+
+Exit codes: 0 = entries changed, commit. UNCHANGED (10) = skip. Anything else = error.
+
+`unchanged` deliberately is NOT 1: an uncaught exception exits 1, and the caller must not
+be able to mistake a crashed comparison for "nothing to push" -- that would turn a broken
+refresh into a silently green job. Every code outside {0, 10} fails the workflow.
+"""
+
+import subprocess
+import sys
+import tempfile
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "toolchain"))
+from mfc.test.coverage import COVERAGE_MAP_PATH, entries_equal, load_map  # noqa: E402
+
+UNCHANGED = 10
+
+new_entries, _ = load_map(COVERAGE_MAP_PATH)
+if new_entries is None:
+    print(f"Rebuilt {COVERAGE_MAP_PATH} is missing or corrupt.", file=sys.stderr)
+    sys.exit(2)
+
+committed = subprocess.run(["git", "show", f"HEAD:{COVERAGE_MAP_PATH}"], capture_output=True, check=False)
+if committed.returncode != 0:
+    print("No coverage map committed at HEAD -> commit the rebuilt one.")
+    sys.exit(0)
+
+with tempfile.NamedTemporaryFile(suffix=".json.gz") as tmp:
+    tmp.write(committed.stdout)
+    tmp.flush()
+    old_entries, _ = load_map(Path(tmp.name))
+
+if old_entries is None:
+    print("Committed coverage map is corrupt -> commit the rebuilt one.")
+    sys.exit(0)
+
+if entries_equal(old_entries, new_entries):
+    print(f"Coverage entries unchanged ({len(new_entries)} tests) -> nothing to push.")
+    sys.exit(UNCHANGED)
+
+added = len(set(new_entries) - set(old_entries))
+removed = len(set(old_entries) - set(new_entries))
+recovered = sum(1 for k in set(old_entries) & set(new_entries) if sorted(old_entries[k]) != sorted(new_entries[k]))
+print(f"Coverage entries changed: +{added} -{removed}, {recovered} with different coverage -> committing.")
+sys.exit(0)

.github/workflows/coverage-health.yml

@@ -9,7 +9,10 @@ jobs:
     if: github.repository == 'MFlowCode/MFC'
     runs-on: ubuntu-latest
     steps:
+      # Full history: the freshness check asks whether the map's git_sha contains the last
+      # commit touching src/**/*.fpp or cases.py, which a shallow clone cannot answer.
       - uses: actions/checkout@v5
+        with: { fetch-depth: 0 }
       - uses: actions/setup-python@v6
         with: { python-version: '3.12' }
       - name: Initialize MFC

.github/workflows/coverage-refresh.yml

@@ -35,7 +35,25 @@ jobs:
         env:
           CACHE_PUSH_TOKEN: ${{ secrets.CACHE_PUSH_TOKEN }}
         run: |
-          if ! git diff --quiet tests/coverage_map.json.gz; then
+          # Compare coverage ENTRIES, not file bytes: _meta (built_at, git_sha) moves on
+          # every rebuild, so `git diff` on the .gz always reported a change and the bot
+          # pushed a no-op commit nearly every run. rc 0 = changed, 10 = unchanged.
+          # The comparison script is stdlib-only, so the system interpreter is a fine
+          # fallback if the SLURM job never got far enough to create build/venv.
+          PY=build/venv/bin/python3
+          [ -x "$PY" ] || PY=python3
+          set +e
+          "$PY" .github/scripts/coverage_map_changed.py
+          changed=$?
+          set -e
+          # Every other code is a hard error, never a silent skip: an uncaught Python
+          # exception exits 1 and a missing interpreter exits 127, and treating either as
+          # "unchanged" would let a permanently broken comparison run permanently green.
+          case "$changed" in
+          0|10) ;;
+          *)    echo "::error::Coverage map comparison failed (rc=$changed)."; exit 1 ;;
+          esac
+          if [ "$changed" -eq 0 ]; then
             git config user.name  "mfc-bot"
             git config user.email "mfc-bot@users.noreply.github.com"
             git add tests/coverage_map.json.gz
@@ -50,5 +68,6 @@ jobs:
             # ensures this token is actually used for the push.
             git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" HEAD:master
           else
-            echo "Coverage map unchanged."
+            # Discard the rebuilt file so the runner's working tree matches master.
+            git checkout -- tests/coverage_map.json.gz
           fi

toolchain/mfc/test/case.py

@@ -251,9 +251,9 @@ def get_uuid(self) -> str:
         return trace_to_uuid(self.trace)
 
     def coverage_key(self) -> str:
-        from .coverage import param_hash
+        from .coverage import canonicalize_param_paths, param_hash
 
-        return param_hash(self.params)
+        return param_hash(canonicalize_param_paths(self.params, common.MFC_ROOT_DIR))
 
     def get_dirpath(self):
         return os.path.join(common.MFC_TEST_DIR, self.get_uuid())

toolchain/mfc/test/coverage.py

@@ -8,11 +8,51 @@
 import gzip
 import hashlib
 import json
+import os
 import subprocess
 from pathlib import Path
 from typing import Optional, Tuple
 
 
+def canonicalize_param_paths(params: dict, root: str) -> dict:
+    """Rewrite absolute in-repo paths to repo-relative ones, for hashing only.
+
+    TestCaseBuilder.to_case() absolutizes file-valued params (e.g. the STL cases'
+    stl_models(1)%model_filepath) so a test runs from any cwd. Those absolutes embed the
+    checkout location, which differs between the map-building runner and PR runners.
+    Hashing them gave the same test a different key on every machine, which churned ~8
+    entries out of the map on every refresh and left those tests permanently unmapped
+    (rung 5 -> always run). Hash the repo-relative form so the key names the test, not
+    the machine. Paths outside the repo are left alone -- they are genuinely part of the
+    test's identity.
+    """
+    root = os.path.abspath(root)
+    out = {}
+    for key, value in params.items():
+        canonical = value
+        if isinstance(value, str) and os.path.isabs(value):
+            try:
+                rel = os.path.relpath(value, root)
+            except ValueError:  # different drive on Windows -> not in-repo
+                rel = os.pardir
+            # Only a leading `..` COMPONENT means "outside the repo". Testing the string
+            # prefix alone would also reject an in-repo name that merely starts with dots
+            # (`..foo`), leaving it absolute -- the churn this function exists to remove.
+            if rel != os.pardir and not rel.startswith(os.pardir + os.sep):
+                canonical = rel.replace(os.sep, "/")
+        out[key] = canonical
+    return out
+
+
+def entries_equal(a: Optional[dict], b: Optional[dict]) -> bool:
+    """True if two maps describe the same coverage, ignoring per-test list order."""
+    if a is None or b is None:
+        return False
+    if set(a) != set(b):
+        return False
+    return all(sorted(a[k]) == sorted(b[k]) for k in a)
+
+
 def param_hash(params: dict) -> str:
     """Stable 16-hex key identifying a test by its defining params.
 
@@ -35,8 +75,14 @@ def save_map(path: Path, entries: dict, *, n_tests: int, git_sha: str, gfortran_
         "n_tests": n_tests,
     }
     path.parent.mkdir(parents=True, exist_ok=True)
-    with gzip.open(path, "wt", encoding="utf-8") as f:
-        json.dump(payload, f, indent=2, sort_keys=True)
+    body = json.dumps(payload, indent=2, sort_keys=True).encode("utf-8")
+    # mtime=0 and an empty filename keep the gzip header byte-identical across builds.
+    # gzip.open() stamps the current time and the source filename into the header, so two
+    # runs producing the same map still wrote different bytes -- which defeated the
+    # "only commit if the file changed" guard in coverage-refresh.yml.
+    with open(path, "wb") as raw:
+        with gzip.GzipFile(filename="", mode="wb", fileobj=raw, mtime=0) as f:
+            f.write(body)
 
 
 # Test-definition file: changing it adds/modifies tests, but only the tests it touches
@@ -223,12 +269,22 @@ def format_summary(*, ran, total, reason, meta, now) -> str:
     return f"Coverage selection: ran {ran}/{total} tests · {age} · {reason}"
 
 
-def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction):
-    """Return (ok, message). Loud anti-rot check used by the health workflow."""
+def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, built_after_last_change=None):
+    """Return (ok, message). Loud anti-rot check used by the health workflow.
+
+    `built_after_last_change` is the caller's git verdict on whether the map was built at
+    or after the most recent coverage-relevant commit: True (provably current), False
+    (provably behind), or None (undeterminable -> fall back to the wall-clock rule).
+    A map only decays when the sources or test list it was built from move, so wall-clock
+    age is a poor proxy: it cries STALE over a quiet weekend and stays silent for 10 days
+    after a refresh actually breaks.
+    """
     if not meta or not meta.get("built_at"):
         return False, "Coverage map has no build metadata."
     age = (datetime.datetime.fromisoformat(now) - datetime.datetime.fromisoformat(meta["built_at"])).days
-    if age > max_age_days:
+    if built_after_last_change is False:
+        return False, "Coverage map is STALE: built before the most recent coverage-relevant commit. Refresh workflow may be broken."
+    if built_after_last_change is None and age > max_age_days:
         return False, f"Coverage map is STALE: {age}d old (max {max_age_days}d). Refresh workflow may be broken."
     if current_keys:
         frac = len(current_keys & mapped_keys) / len(current_keys)