ci(coverage): key map freshness on the refresh run, not on _meta.git_sha

.github/scripts/check_coverage_map_health.py

@@ -11,54 +11,93 @@
 MAX_AGE_DAYS = 10
 MIN_FRACTION = 0.80
 
+# coverage-refresh.yml moves this ref to the commit it just rebuilt the map from, on EVERY
+# successful refresh -- including the ones whose entries come out identical and therefore
+# push no commit at all.
+#
+# The map's own _meta.git_sha cannot answer the freshness question. It only advances when a
+# commit lands, and the refresh deliberately skips the commit when the coverage entries are
+# unchanged (a no-op commit on every push is what #1683 removed). So a coverage-relevant
+# commit whose coverage happens to be identical pinned git_sha in the past and made this
+# check fail every day thereafter, with no way to recover: a refresh that WAS working
+# looked broken. The question here is "has a refresh run since the last relevant commit",
+# and only the refresh itself can answer it.
+#
+# It lives outside refs/heads/ and refs/tags/ so that updating it fires no `push` workflow
+# trigger. actions/checkout does not fetch it; coverage-health.yml fetches it explicitly.
+VERIFIED_REF = "refs/coverage-map/verified"
+
 # 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.
+def verified_sha(cwd=None):
+    """Commit the last successful refresh verified the map against, or None if unknown.
+
+    None covers both "the ref was never pushed" (a fork, or the window before the first
+    refresh after VERIFIED_REF was introduced) and "the fetch did not bring it down". The
+    caller must read that as undeterminable and fall back to the wall-clock age rule, not
+    as a failure -- an absent ref is not evidence of a broken refresh.
+    """
+    rev = subprocess.run(["git", "rev-parse", "--verify", "--quiet", f"{VERIFIED_REF}^{{commit}}"], capture_output=True, text=True, check=False, cwd=cwd)
+    return rev.stdout.strip() or None
+
+
+def verified_after_last_change(git_sha, cwd=None):
+    """Did that refresh run 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.
+    over a genuinely quiet repo (no refresh needed, so no heartbeat 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)
+    last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False, cwd=cwd)
     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)
+    ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False, cwd=cwd)
     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.")
-# Compute each current test's coverage key. Loading a case executes its case
-# file; some (e.g. chemistry examples) import optional deps like cantera that are
-# not installed in this lightweight job. Skip any case that fails to load instead
-# of crashing — map_health measures the fraction of *loadable* current tests that
-# are mapped, so a smaller current_keys cannot produce a false "stale" result.
-current_keys = set()
-unloadable = []
-for b in list_cases():
-    try:
-        current_keys.add(b.to_case().coverage_key())
-    except Exception as exc:  # noqa: BLE001 — a case file that won't import must not crash the health check
-        last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140]
-        unloadable.append((getattr(b, "trace", repr(b)), last_line))
-if unloadable:
-    print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):")
-    for trace, err in unloadable[:15]:
-        print(f"  - {trace}: {err}")
-ok, msg = map_health(
-    meta=meta,
-    current_keys=current_keys,
-    mapped_keys=set(entries),
-    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)
+
+def main():
+    entries, meta = load_map(COVERAGE_MAP_PATH)
+    if entries is None:
+        sys.exit("Coverage map missing or corrupt.")
+    # Compute each current test's coverage key. Loading a case runs its case file as a
+    # subprocess, so anything that file imports must resolve. Keep skipping the ones that
+    # do not rather than crashing -- map_health measures the fraction of *loadable*
+    # current tests that are mapped, so a smaller current_keys cannot produce a false
+    # "stale" result -- but the skip is a safety net, not the expected path: every case
+    # here is meant to load. The 16 chemistry cases that used to land in this list did so
+    # because get_py_program_output ran them under PATH's python3 while this job invokes
+    # build/venv/bin/python3 directly, leaving the venv's cantera out of reach.
+    current_keys = set()
+    unloadable = []
+    for b in list_cases():
+        try:
+            current_keys.add(b.to_case().coverage_key())
+        except Exception as exc:  # noqa: BLE001 -- a case file that won't import must not crash the health check
+            last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140]
+            unloadable.append((getattr(b, "trace", repr(b)), last_line))
+    if unloadable:
+        print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):")
+        for trace, err in unloadable[:15]:
+            print(f"  - {trace}: {err}")
+    ok, msg = map_health(
+        meta=meta,
+        current_keys=current_keys,
+        mapped_keys=set(entries),
+        now=datetime.datetime.now(datetime.timezone.utc).isoformat(),
+        max_age_days=MAX_AGE_DAYS,
+        min_fraction=MIN_FRACTION,
+        verified_after_last_change=verified_after_last_change(verified_sha()),
+    )
+    print(msg)
+    return 0 if ok else 1
+
+
+# Guarded so the two git predicates above can be imported and unit-tested against a
+# throwaway repository; running the module still performs the full check.
+if __name__ == "__main__":
+    sys.exit(main())

.github/workflows/coverage-health.yml

@@ -9,10 +9,20 @@ 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.
+      # Full history: the freshness check asks whether refs/coverage-map/verified -- the
+      # commit the last successful refresh rebuilt from -- contains the last commit
+      # touching src/**/*.fpp or cases.py, which a shallow clone cannot answer.
       - uses: actions/checkout@v5
         with: { fetch-depth: 0 }
+      - name: Fetch the coverage-map verification ref
+        # actions/checkout fetches only refs/heads and refs/tags, and refs/coverage-map/
+        # verified deliberately lives outside both so that updating it fires no `push`
+        # workflow trigger. Absent on a fork and in the window before the first refresh
+        # after it was introduced, so this must not fail the job: the check treats a
+        # missing ref as undeterminable and falls back to the wall-clock age rule.
+        run: |
+          git fetch --no-tags origin '+refs/coverage-map/verified:refs/coverage-map/verified' \
+            || echo "::notice::No coverage-map verification ref yet; using the wall-clock age rule."
       - uses: actions/setup-python@v6
         with: { python-version: '3.12' }
       - name: Initialize MFC

.github/workflows/coverage-refresh.yml

@@ -17,7 +17,11 @@ concurrency:
 jobs:
   refresh:
     if: github.repository == 'MFlowCode/MFC'
-    timeout-minutes: 240
+    # Wall clock here is dominated by the SLURM queue, not by the build: observed waits on
+    # the phoenix `embers` QOS have reached 6h, and a 240-minute cap silently killed the
+    # refresh of #1717 at 4h05m. Cap generously -- a refresh that never finishes leaves the
+    # map behind, which is exactly what coverage-health.yml then reports.
+    timeout-minutes: 480
     runs-on:
       group:  phoenix
       labels: gt
@@ -71,3 +75,26 @@ jobs:
             # Discard the rebuilt file so the runner's working tree matches master.
             git checkout -- tests/coverage_map.json.gz
           fi
+          # Record that a refresh verified the map against THIS commit, in BOTH branches
+          # above. coverage-health.yml cannot use the map's _meta.git_sha for this: git_sha
+          # advances only when a commit lands, and the guard above deliberately skips the
+          # commit when the entries are unchanged, so a coverage-relevant commit whose
+          # coverage is identical left the map looking permanently stale to the health
+          # check while this workflow was working fine.
+          #
+          # refs/coverage-map/ is outside refs/heads/ and refs/tags/ on purpose: a branch or
+          # tag would fire the `push` trigger of any workflow that filters on paths alone
+          # (homebrew.yml does), starting unrelated jobs on every refresh.
+          #
+          # Non-fatal: the map itself is already committed and pushed by this point, so a
+          # failed bookkeeping push must not red a refresh that succeeded. It is not silent
+          # either -- the warning lands in the run summary, and a ref that stops advancing
+          # drops coverage-health.yml back to its wall-clock rule, which goes red within
+          # MAX_AGE_DAYS.
+          # $GITHUB_SHA, not HEAD: when the entries changed, HEAD is the bot's new map
+          # commit, one past the commit this refresh actually rebuilt from. Either answers
+          # the health check's ancestry question, but only $GITHUB_SHA names the same thing
+          # in both branches -- the source state the map was built against.
+          git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" \
+            --force "$GITHUB_SHA:refs/coverage-map/verified" \
+            || echo "::warning::Could not update refs/coverage-map/verified; coverage-health.yml falls back to the age rule."

toolchain/mfc/common.py

@@ -2,6 +2,7 @@
 import os
 import shutil
 import subprocess
+import sys
 import typing
 from os.path import abspath, dirname, join, normpath, realpath
 
@@ -135,10 +136,21 @@ def get_program_output(arguments: typing.List[str] = None, cwd=None):
 
 
 def get_py_program_output(filepath: str, arguments: typing.List[str] = None):
+    """Run a case file and capture its stdout.
+
+    sys.executable, not a bare "python3": a case file imports the same optional deps the
+    toolchain venv provides (cantera, pyrometheus, scipy, ...), so it must run under the
+    interpreter the toolchain itself is running under. Nearly every entry point activates
+    the venv first, which makes PATH's python3 the venv's -- but a caller that invokes
+    build/venv/bin/python3 DIRECTLY does not, and then case files silently ran under the
+    system interpreter instead. That is what made coverage-health.yml report 16 chemistry
+    cases as unloadable with ModuleNotFoundError: No module named 'cantera', in a job whose
+    venv had cantera installed.
+    """
     dirpath = os.path.abspath(os.path.dirname(filepath))
     filename = os.path.basename(filepath)
 
-    return get_program_output(["python3", filename] + arguments, cwd=dirpath)
+    return get_program_output([sys.executable, filename] + arguments, cwd=dirpath)
 
 
 def isspace(s: str) -> bool:

toolchain/mfc/test/coverage.py

@@ -269,22 +269,27 @@ 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, built_after_last_change=None):
+def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, verified_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
+    `verified_after_last_change` is the caller's git verdict on whether a refresh ran 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.
+
+    It asks about the refresh RUN, not about the map's own _meta.git_sha, which advances
+    only when the rebuilt entries differ from the committed ones. A relevant commit whose
+    coverage is unchanged leaves a correct map with an old git_sha; reading that as
+    "stale" made this check fail permanently while the refresh was working fine.
     """
     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 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:
+    if verified_after_last_change is False:
+        return False, "Coverage map is STALE: no successful refresh since the most recent coverage-relevant commit. Refresh workflow may be broken."
+    if verified_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)