fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809)

.github/groom/README.md

@@ -199,6 +199,21 @@ real groom, so a skipped tick costs ~nothing (it never reaches the finder).
   interval-skip ticks in between never reset the clock. (A repo variable would
   need a `Variables: write` credential the run doesn't carry, and a missing grant
   would fail *silently* into a daily over-spend — run history has no such trap.)
+- **A *failed* finder job counts only if the billed agent step ran** (BE-4809).
+  The job runs two checkouts, `npm install`, the prompt build and the read-only
+  chmod BEFORE the agent; a transient failure in any of them concludes the job
+  `failure` having spent nothing, and counting that would silently skip every
+  tick for a full interval. So a `failure` additionally requires the run-history
+  payload's per-job `steps` to show `Run finder` reaching a real conclusion —
+  `success` still counts unconditionally. The missing-data direction here is
+  deliberately the OPPOSITE of the rest of the gate: no usable `steps` array
+  counts the failure, because re-billing a genuinely-spent audit on the next
+  daily tick is the expensive mistake. For the same reason the jobs are fetched
+  with `filter=all` (the endpoint defaults to `latest`): the question is now
+  step-level, so a manual re-run that flakes in `npm install` must not erase the
+  agent an *earlier* attempt already paid for — one attempt showing the step ran
+  is enough. A unit test pins the step name against `groom.yml` so a rename can't
+  silently drift the two apart.
 - **`workflow_dispatch` bypasses THIS gate** — a manual dispatch is never
   interval-throttled. It is not a blanket "always runs": the volume gate is a
   second, independent throttle, so a live dispatch into a quiescent repo can

.github/groom/interval.py

@@ -19,8 +19,12 @@
 otherwise carry, and a missing grant would fail *silently* into a daily
 over-spend). A prior run "counts" as a real groom only if it actually reached the
 finder (its `Audit — finder` job ran, i.e. was not `skipped` by this very gate),
-so the interval-skip ticks in between never reset the clock. Run history is
-durable across the stateless CI runs and readable with only `actions: read`.
+so the interval-skip ticks in between never reset the clock. A *failed* finder
+job counts only when its billed agent step (`Run finder`) actually ran, so a
+flaky checkout or `npm install` before the agent cannot burn a whole cycle
+(BE-4809) — judged across ALL of a run's attempts, so a re-run that flakes early
+cannot erase the spend of an earlier attempt. Run history is durable across the
+stateless CI runs and readable with only `actions: read`.
 
 The gate is **fail-open**, matching the volume gate: any error deriving the last
 run (API hiccup, unparseable timestamp, no history) RUNS the audit rather than
@@ -64,12 +68,34 @@
 # `skipped`, so it never matches the audited conclusions below.
 _FINDER_JOB_HINTS = ("finder", "audit_find")
 
-# A finder job that reached success OR failure spent the (billed) audit, so both
-# count as a real run: counting a failure keeps a run that spent money but died
+# A finder job that reached success OR failure MAY have spent the (billed) audit,
+# so both are eligible: counting a failure keeps a run that spent money but died
 # at a later step (e.g. filing) from re-spending on the very next daily tick.
 # `skipped` (the interval-skip case), `cancelled`, and a null conclusion do not.
+# A `failure` is eligible but not sufficient — see `_agent_step_ran` (BE-4809).
 _AUDITED_CONCLUSIONS = {"success", "failure"}
 
+# The finder job runs several cheap steps BEFORE the billed agent (two checkouts,
+# the CLI install, the prompt build, the read-only chmod). A transient failure in
+# any of them concludes the JOB `failure` while spending nothing — and counting
+# that as a spent audit silently skips every tick for a full interval. So for a
+# `failure` job we additionally require the agent step itself to have run; the
+# gate names it `Run finder` in `.github/workflows/groom.yml`.
+#
+# Matched by EXACT (case-insensitive) name, unlike `_FINDER_JOB_HINTS` above:
+# that one is a substring match because GitHub genuinely mangles JOB names in a
+# nested reusable ("<caller-job> / Audit — finder"), but STEP names come back
+# verbatim from the YAML, so there is nothing to be tolerant of — and a substring
+# would also match a neighbour like `Rerun finder`. A test pins these names
+# against the producing workflow so a rename can't silently drift the two apart.
+_AGENT_STEP_NAMES = {"run finder"}
+
+# Step conclusions that mean the agent step did NOT execute. GitHub reports the
+# steps AFTER a failing one as `skipped` rather than omitting them, so merely
+# finding the step in the array is not evidence it ran; a null conclusion is the
+# same "never reached a conclusion" case.
+_UNRUN_STEP_CONCLUSIONS = {None, "skipped"}
+
 # Default cadence when GROOM_INTERVAL_DAYS is unset/blank/garbage — 7 days keeps
 # the documented weekly behavior (AC: unset variable stays weekly, matching today).
 _DEFAULT_INTERVAL_DAYS = 7.0
@@ -113,6 +139,13 @@
 # (fail-open). Far more than any sane interval's worth of daily skip-ticks.
 _MAX_RUNS_SCANNED = 100
 
+# Page size and page cap for the per-run jobs walk. `filter=all` returns one set
+# of jobs PER ATTEMPT (see fetch_run_jobs), so a much-re-run workflow can spill
+# past a single page — groom is ~8 jobs a run, so 5 pages covers ~60 attempts.
+# The cap only bounds a pathological payload; the gate must stay cheap.
+_JOBS_PAGE_SIZE = 100
+_MAX_JOB_PAGES = 5
+
 
 def parse_interval_days(raw, default: float = _DEFAULT_INTERVAL_DAYS):
     """Parse the GROOM_INTERVAL_DAYS value; fall back to the weekly default.
@@ -176,12 +209,51 @@ def days_since(then_iso: str, now: datetime) -> float:
     return (now - then).total_seconds() / 86400.0
 
 
+def _agent_step_ran(job) -> bool:
+    """True if this job's billed agent step reached a real conclusion (BE-4809).
+
+    Fail-SAFE on missing data: if the job carries no usable `steps` array (an API
+    shape change, a truncated payload, a hand-built fixture), fall back to the
+    pre-BE-4809 behavior and treat the agent as having run. The alternative would
+    make a genuinely-spent audit re-bill on the very next daily tick, which is the
+    expensive direction — the opposite of the elapsed-time logic, where the cheap
+    direction is to run.
+
+    A step that is present but `skipped` (GitHub's rendering for every step after
+    a failing one) or still without a conclusion did NOT execute, so it does not
+    count. Neither does an agent step that is absent from a non-empty array —
+    that is a run that died before reaching it.
+    """
+    steps = job.get("steps")
+    if not isinstance(steps, list) or not steps:
+        return True
+    for step in steps:
+        if not isinstance(step, dict):
+            continue
+        name = (step.get("name") or "").strip().lower()
+        if name in _AGENT_STEP_NAMES:
+            return step.get("conclusion") not in _UNRUN_STEP_CONCLUSIONS
+    return False
+
+
 def run_audited(jobs) -> bool:
-    """True if a run's jobs show the finder actually ran (not an interval-skip)."""
+    """True if a run's jobs show the finder actually ran (not an interval-skip).
+
+    `success` counts unconditionally — the job cannot have succeeded without the
+    agent step. `failure` counts only when the agent step itself ran (BE-4809): a
+    flaky checkout / `npm install` before it fails the job having spent nothing,
+    and counting that would silently skip grooming for a whole interval.
+    """
     for job in jobs or []:
         name = (job.get("name") or "").lower()
-        if any(hint in name for hint in _FINDER_JOB_HINTS) and job.get("conclusion") in _AUDITED_CONCLUSIONS:
-            return True
+        if not any(hint in name for hint in _FINDER_JOB_HINTS):
+            continue
+        conclusion = job.get("conclusion")
+        if conclusion not in _AUDITED_CONCLUSIONS:
+            continue
+        if conclusion == "failure" and not _agent_step_ran(job):
+            continue
+        return True
     return False
 
 
@@ -257,11 +329,37 @@ def fetch_workflow_runs(repo: str, workflow_file: str, run=subprocess.run):
 
 
 def fetch_run_jobs(repo: str, run_id, run=subprocess.run):
-    """The jobs of one workflow run (single page)."""
+    """Every job of one workflow run, across ALL of its attempts.
+
+    `filter=all` is load-bearing, not tidiness (BE-4809). The endpoint defaults
+    to `filter=latest` — only the most recent attempt of a re-run workflow. That
+    default was harmless while every `failure` counted, but the answer now
+    depends on WHICH STEPS ran: if the first attempt spent the billed agent and
+    someone then hit "re-run" and that attempt flaked in `npm install`, the
+    latest attempt alone reads as unspent and the next daily tick re-bills a
+    groom that already happened — the exact double-spend this gate prevents.
+    `run_audited` needs only ONE attempt to show the agent ran, so handing it the
+    union of attempts is both correct and order-independent.
+
+    Returns each job UNPROJECTED — `run_audited` reads the per-job `steps` array
+    as well as `name`/`conclusion`, so do not add a `--jq`/field filter here
+    without keeping `steps`: dropping it degrades the gate to counting every
+    `failure` as a spent audit, silently and without a test failing.
+    """
     if not _REPO_RE.match(repo or ""):
         raise ValueError(f"invalid repo {repo!r}: expected owner/name")
-    payload = _api_json([f"/repos/{repo}/actions/runs/{run_id}/jobs?per_page=100"], run)
-    return payload.get("jobs", []) if isinstance(payload, dict) else []
+    jobs = []
+    for page in range(1, _MAX_JOB_PAGES + 1):
+        payload = _api_json(
+            [f"/repos/{repo}/actions/runs/{run_id}/jobs"
+             f"?filter=all&per_page={_JOBS_PAGE_SIZE}&page={page}"],
+            run,
+        )
+        page_jobs = payload.get("jobs", []) if isinstance(payload, dict) else []
+        jobs.extend(page_jobs)
+        if len(page_jobs) < _JOBS_PAGE_SIZE:
+            break
+    return jobs
 
 
 def find_last_audited_run_at(repo, workflow_file, current_run_id, run=subprocess.run):