From 2ed2cf7b9718cf3133ba27bf0167f9f097fc611d Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 27 Jul 2026 18:38:39 -0700 Subject: [PATCH 1/2] fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interval gate counted ANY finder job concluding `success` or `failure` as a real groom. Counting a `failure` is right when the billed agent ran and the job died later (filing, PR creation) — but the finder runs two checkouts, an `npm install`, the prompt build and the read-only chmod BEFORE the agent, and a transient failure in any of them concludes the job `failure` having spent nothing. The gate then skipped every tick for a full GROOM_INTERVAL_DAYS: a silent under-run, the exact failure mode the rest of the module biases against. `run_audited` now additionally requires, for a `failure`, that the agent step (`Run finder`) actually ran — read from the per-job `steps` array the jobs API already returns. `success` still counts unconditionally. Missing/empty step data falls back to today's behavior and counts the failure: re-billing a genuinely-spent audit is the expensive direction. A step present but `skipped` (GitHub's rendering for every step after a failing one) or without a conclusion did not execute, so it does not count. A test pins the step name against groom.yml's `audit_find` job, and the test workflow's path filter now includes groom.yml so a rename runs that pin. --- .github/groom/README.md | 11 ++ .github/groom/interval.py | 81 ++++++++++++-- .github/groom/tests/test_interval.py | 131 ++++++++++++++++++++++- .github/workflows/test-groom-scripts.yml | 6 ++ 4 files changed, 219 insertions(+), 10 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 0921c06..8d076be 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -199,6 +199,17 @@ 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. 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 diff --git a/.github/groom/interval.py b/.github/groom/interval.py index e162c54..685be8c 100644 --- a/.github/groom/interval.py +++ b/.github/groom/interval.py @@ -19,8 +19,11 @@ 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). 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 +67,29 @@ # `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 +# case-insensitively as a substring, mirroring `_FINDER_JOB_HINTS`, and pinned +# against the producing workflow by a test so a rename can't silently drift. +_AGENT_STEP_HINTS = ("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 @@ -176,12 +196,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 "").lower() + if any(hint in name for hint in _AGENT_STEP_HINTS): + 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,7 +316,13 @@ 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).""" + """The jobs of one workflow run (single page). + + Returns each job UNPROJECTED — `run_audited` reads the per-job `steps` array + (BE-4809) 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) diff --git a/.github/groom/tests/test_interval.py b/.github/groom/tests/test_interval.py index 773ebc4..7743bad 100644 --- a/.github/groom/tests/test_interval.py +++ b/.github/groom/tests/test_interval.py @@ -6,6 +6,9 @@ a tick at/after the interval runs. - The interval-skip ticks in between do NOT reset the clock (only a run whose finder actually ran counts). +- A FAILED finder job counts only when its billed agent step (`Run finder`) ran, + so a flaky pre-agent step (checkout, `npm install`) can't burn a whole cycle + (BE-4809) — with a fail-SAFE fallback when the payload carries no step data. - `workflow_dispatch` always runs, regardless of the interval. - The gate is fail-open: no history / an API error runs rather than skips. - The volume gate's window normalizes through the SAME parser (blank/garbage/ @@ -22,6 +25,7 @@ import io import json import os +import re import unittest from datetime import datetime, timezone @@ -62,8 +66,32 @@ def _run(cmd, **kwargs): return _run -def finder_job(conclusion="success"): - return {"name": "groom / Audit — finder", "conclusion": conclusion} +def finder_job(conclusion="success", steps=None): + """A finder job as the jobs API renders it. + + `steps=None` omits the array entirely — the fail-safe shape every pre-BE-4809 + fixture (and any truncated/changed payload) has, which still counts. + """ + job = {"name": "groom / Audit — finder", "conclusion": conclusion} + if steps is not None: + job["steps"] = steps + return job + + +def step(name, conclusion="success"): + return {"name": name, "status": "completed", "conclusion": conclusion, "number": 1} + + +# The steps the finder job runs BEFORE the billed agent (see groom.yml's +# `audit_find`) — any of these can flake and fail the job having spent nothing. +PRE_AGENT_STEPS = [ + step("Set up job"), + step("Checkout target repo (clean default branch)"), + step("Load groom assets (briefs)"), + step("Build finder prompt"), + step("Install Claude Code"), + step("Lock the clone read-only"), +] class ParseIntervalDaysTest(unittest.TestCase): @@ -143,6 +171,105 @@ def test_skipped_or_missing_does_not_count(self): self.assertFalse(interval.run_audited([])) +class FailedFinderSpentTheAgentTest(unittest.TestCase): + """A `failure` job counts only if the billed agent step actually ran (BE-4809).""" + + def test_failure_after_the_agent_ran_still_counts(self): + # The money was spent; the job died at a later step (filing, artifact + # upload). Re-running on the very next daily tick would double-bill. + for agent_conclusion in ("success", "failure", "cancelled"): + job = finder_job("failure", PRE_AGENT_STEPS + [ + step("Run finder", agent_conclusion), + step("Assert candidates", "failure"), + ]) + self.assertTrue(interval.run_audited([job]), agent_conclusion) + + def test_failure_before_the_agent_does_not_count(self): + # A flaky `npm install` (or either checkout) fails the job having spent + # nothing — the tick must stay due rather than eat a whole interval. + truncated = PRE_AGENT_STEPS[:-2] + [step("Install Claude Code", "failure")] + self.assertFalse(interval.run_audited([finder_job("failure", truncated)])) + + def test_failure_with_the_agent_step_skipped_does_not_count(self): + # GitHub renders every step after a failing one as `skipped` rather than + # omitting it, so the step being present is not evidence that it ran. + steps = PRE_AGENT_STEPS[:-1] + [ + step("Lock the clone read-only", "failure"), + step("Run finder", "skipped"), + ] + self.assertFalse(interval.run_audited([finder_job("failure", steps)])) + self.assertFalse(interval.run_audited([finder_job("failure", PRE_AGENT_STEPS + [step("Run finder", None)])])) + + def test_failure_without_step_data_counts(self): + # Fail-SAFE, deliberately the opposite bias to the elapsed-time logic: no + # usable `steps` (API shape change, truncated payload) keeps today's + # behavior, because re-billing a genuinely-spent audit is the expensive + # direction. + self.assertTrue(interval.run_audited([finder_job("failure")])) # key absent + self.assertTrue(interval.run_audited([finder_job("failure", [])])) # empty array + self.assertTrue(interval.run_audited([{"name": "groom / Audit — finder", + "conclusion": "failure", "steps": None}])) + + def test_success_is_unaffected_by_step_data(self): + # A successful job cannot have succeeded without the agent, so the step + # array is never consulted — including a (nonsensical) truncated one. + self.assertTrue(interval.run_audited([finder_job("success")])) + self.assertTrue(interval.run_audited([finder_job("success", PRE_AGENT_STEPS)])) + self.assertTrue(interval.run_audited([finder_job("success", PRE_AGENT_STEPS + [step("Run finder")])])) + + def test_a_spent_failure_still_counts_when_an_unspent_one_precedes_it(self): + # Job order must not decide the answer: one matching job that spent the + # agent is enough, even behind a matching job that didn't. + jobs = [ + finder_job("failure", [step("Install Claude Code", "failure")]), + finder_job("failure", PRE_AGENT_STEPS + [step("Run finder")]), + ] + self.assertTrue(interval.run_audited(jobs)) + + def test_unspent_failure_leaves_the_tick_due_end_to_end(self): + # The behavior that matters: yesterday's run died in `npm install`, so + # today's tick must still groom rather than wait out the interval. + runs = [ + {"id": 100, "status": "in_progress", "run_started_at": iso(0)}, + {"id": 99, "status": "completed", "run_started_at": iso(1)}, + ] + jobs = {"99": [finder_job("failure", [step("Install Claude Code", "failure")])]} + d = interval.evaluate("o/r", "ci-groom.yml", 100, 7.0, "schedule", NOW, run=make_gh_stub(runs, jobs)) + self.assertTrue(d["should_run"]) + self.assertIsNone(d["last_run_at"]) + + +class AgentStepNamePinTest(unittest.TestCase): + """Pin the step name this module matches to the one groom.yml produces. + + Producer (`.github/workflows/groom.yml`) and consumer (this module) live in + different files, so a rename of the step would otherwise silently degrade the + gate back to counting every failed job as a spent audit — no test failing. + """ + + def _audit_find_step_names(self): + path = os.path.join(os.path.dirname(__file__), "..", "..", "workflows", "groom.yml") + with open(path, encoding="utf-8") as f: + lines = f.read().splitlines() + names, inside = [], False + for line in lines: + if re.match(r"^ [A-Za-z_][A-Za-z0-9_-]*:\s*$", line): + inside = line.strip() == "audit_find:" + continue + if inside: + m = re.match(r"^\s*- name:\s*(\S.*?)\s*$", line) + if m: + names.append(m.group(1)) + return names + + def test_groom_yml_names_exactly_the_agent_step_this_module_matches(self): + names = self._audit_find_step_names() + self.assertIn("Run finder", names, "groom.yml's audit_find job no longer has a `Run finder` step") + matched = [n for n in names if any(h in n.lower() for h in interval._AGENT_STEP_HINTS)] + self.assertEqual(matched, ["Run finder"], + f"_AGENT_STEP_HINTS must match exactly the agent step, got {matched} from {names}") + + class IntervalThresholdTest(unittest.TestCase): def test_full_day_intervals_lose_a_half_tick(self): self.assertEqual(interval.interval_threshold(7.0), 6.5) diff --git a/.github/workflows/test-groom-scripts.yml b/.github/workflows/test-groom-scripts.yml index 3cc9ae9..72c85ed 100644 --- a/.github/workflows/test-groom-scripts.yml +++ b/.github/workflows/test-groom-scripts.yml @@ -10,11 +10,17 @@ on: pull_request: paths: - '.github/groom/**' + # interval.py pins the finder's agent step NAME against this workflow + # (BE-4809), so a rename there must run these tests too. + - '.github/workflows/groom.yml' - '.github/workflows/test-groom-scripts.yml' push: branches: [main] paths: - '.github/groom/**' + # interval.py pins the finder's agent step NAME against this workflow + # (BE-4809), so a rename there must run these tests too. + - '.github/workflows/groom.yml' - '.github/workflows/test-groom-scripts.yml' permissions: From 63a6413bf3689c0c22fc1d0d8e429da1c0cc1ce3 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 27 Jul 2026 18:57:12 -0700 Subject: [PATCH 2/2] fix(groom): judge the finder's spend across ALL run attempts (BE-4809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the cursor-review panel on #84. - `fetch_run_jobs` now asks for `filter=all` and pages the result. The endpoint defaults to `filter=latest`, which was harmless while every `failure` counted but is not once the answer is step-level: an attempt that spent the billed agent and died at filing, followed by a manual re-run that flakes in `npm install`, read as unspent and re-billed a groom that already happened — the exact double-spend this gate exists to prevent. `run_audited` needs one attempt to show the step ran, so the union of attempts is correct and order-independent. - Match the agent step by EXACT (case-insensitive) name instead of a substring. Job names need substring matching because GitHub mangles them in nested reusables; step names come back verbatim, so the tolerance bought nothing and would let a neighbour like `Rerun finder` stand in for the billed step. - Harden the pin test's groom.yml scan against benign reformats (reindentation, quoted step names, inline comments) and cover the scanner itself, so it fails on a rename rather than on a whitespace change. --- .github/groom/README.md | 8 +- .github/groom/interval.py | 59 +++++++-- .github/groom/tests/test_interval.py | 174 ++++++++++++++++++++++++--- 3 files changed, 210 insertions(+), 31 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 8d076be..f574f45 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -208,8 +208,12 @@ real groom, so a skipped tick costs ~nothing (it never reaches the finder). `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. A unit test pins the step name against - `groom.yml` so a rename can't silently drift the two apart. + 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 diff --git a/.github/groom/interval.py b/.github/groom/interval.py index 685be8c..d9bc324 100644 --- a/.github/groom/interval.py +++ b/.github/groom/interval.py @@ -22,8 +22,9 @@ 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). Run history is durable across the stateless CI runs and readable with -only `actions: read`. +(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 @@ -79,10 +80,15 @@ # 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 -# case-insensitively as a substring, mirroring `_FINDER_JOB_HINTS`, and pinned -# against the producing workflow by a test so a rename can't silently drift. -_AGENT_STEP_HINTS = ("run finder",) +# 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 (" / 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 @@ -133,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. @@ -217,8 +230,8 @@ def _agent_step_ran(job) -> bool: for step in steps: if not isinstance(step, dict): continue - name = (step.get("name") or "").lower() - if any(hint in name for hint in _AGENT_STEP_HINTS): + name = (step.get("name") or "").strip().lower() + if name in _AGENT_STEP_NAMES: return step.get("conclusion") not in _UNRUN_STEP_CONCLUSIONS return False @@ -316,17 +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 - (BE-4809) 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 + 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): diff --git a/.github/groom/tests/test_interval.py b/.github/groom/tests/test_interval.py index 7743bad..fbfc91a 100644 --- a/.github/groom/tests/test_interval.py +++ b/.github/groom/tests/test_interval.py @@ -8,7 +8,8 @@ finder actually ran counts). - A FAILED finder job counts only when its billed agent step (`Run finder`) ran, so a flaky pre-agent step (checkout, `npm install`) can't burn a whole cycle - (BE-4809) — with a fail-SAFE fallback when the payload carries no step data. + (BE-4809) — with a fail-SAFE fallback when the payload carries no step data, + and judged across ALL of a run's attempts so a re-run can't erase the spend. - `workflow_dispatch` always runs, regardless of the interval. - The gate is fail-open: no history / an API error runs rather than skips. - The volume gate's window normalizes through the SAME parser (blank/garbage/ @@ -54,13 +55,20 @@ def __init__(self, stdout="", returncode=0, stderr=""): def make_gh_stub(runs, jobs_by_run): - """A `gh api` stub: routes /runs vs /runs//jobs by URL.""" + """A `gh api` stub: routes /runs vs /runs//jobs by URL. + + Paginates the jobs response the way the real endpoint does, so the walk in + `fetch_run_jobs` is exercised (and can't silently spin on page 2). + """ def _run(cmd, **kwargs): url = cmd[-1] if "/jobs" in url: run_id = url.split("/actions/runs/")[1].split("/jobs")[0] - return Result(stdout=json.dumps({"jobs": jobs_by_run.get(run_id, [])})) + all_jobs = jobs_by_run.get(run_id, []) + page = int(re.search(r"[?&]page=(\d+)", url).group(1)) if "page=" in url else 1 + size = interval._JOBS_PAGE_SIZE + return Result(stdout=json.dumps({"jobs": all_jobs[(page - 1) * size:page * size]})) return Result(stdout=json.dumps({"workflow_runs": runs})) return _run @@ -226,6 +234,19 @@ def test_a_spent_failure_still_counts_when_an_unspent_one_precedes_it(self): ] self.assertTrue(interval.run_audited(jobs)) + def test_a_neighbouring_step_is_not_mistaken_for_the_agent(self): + # Step names come back verbatim from the YAML, so the match is EXACT: a + # substring match would let `Rerun finder` (or a `Post Run finder`) stand + # in for the billed step and count a failure that spent nothing. + for decoy in ("Rerun finder", "Run finder prompt lint", "Post Run finder"): + job = finder_job("failure", [step("Set up job"), step(decoy, "failure")]) + self.assertFalse(interval.run_audited([job]), decoy) + + def test_the_agent_step_name_is_matched_case_and_space_insensitively(self): + for rendered in ("Run finder", "run finder", "RUN FINDER", " Run finder "): + job = finder_job("failure", [step(rendered), step("Assert candidates", "failure")]) + self.assertTrue(interval.run_audited([job]), rendered) + def test_unspent_failure_leaves_the_tick_due_end_to_end(self): # The behavior that matters: yesterday's run died in `npm install`, so # today's tick must still groom rather than wait out the interval. @@ -239,6 +260,73 @@ def test_unspent_failure_leaves_the_tick_due_end_to_end(self): self.assertIsNone(d["last_run_at"]) +# --- a deliberately small, reformat-tolerant scan of one job's step names ------ +# +# Used only by the pin test below. It is NOT a YAML parser (the test job installs +# no PyYAML — see .github/workflows/test-groom-scripts.yml), so it is written to +# tolerate the benign edits a workflow file actually attracts: reindentation, +# quoting a step name, a trailing `# comment`. A rename of the step — the thing +# the pin exists to catch — still fails it loudly. +_JOB_KEY_RE = re.compile(r"^(\s*)([A-Za-z_][A-Za-z0-9_-]*):\s*(?:#.*)?$") +_STEP_NAME_RE = re.compile(r"^\s*-\s*name:\s*(\S.*?)\s*$") + + +def yaml_scalar(raw): + """A YAML scalar as written -> its value: unquote, else drop an inline comment.""" + raw = raw.strip() + if len(raw) >= 2 and raw[0] in "'\"" and raw[-1] == raw[0]: + return raw[1:-1] + return re.sub(r"\s+#.*$", "", raw).strip() + + +def job_step_names(lines, job_id): + """Every `- name:` value inside `:`, at whatever indentation it lives.""" + names, indent = [], None + for line in lines: + key = _JOB_KEY_RE.match(line) + if indent is None: + if key and key.group(2) == job_id: + indent = len(key.group(1)) + continue + stripped = line.strip() + if stripped and not stripped.startswith("#") and len(line) - len(line.lstrip()) <= indent: + break # dedented back out of the job block + name = _STEP_NAME_RE.match(line) + if name: + names.append(yaml_scalar(name.group(1))) + return names + + +class JobStepNameScanTest(unittest.TestCase): + """The pin test's scanner survives benign reformats of groom.yml.""" + + def test_tolerates_reindent_quoting_and_inline_comments(self): + for label, body in { + "as written": [" audit_find:", " steps:", " - name: Run finder", " next_job:"], + "reindented": [" audit_find:", " steps:", " - name: Run finder", " next_job:"], + "single-quoted": [" audit_find:", " - name: 'Run finder'", " next_job:"], + "double-quoted": [" audit_find:", ' - name: "Run finder"', " next_job:"], + "inline comment": [" audit_find:", " - name: Run finder # the billed agent", " next_job:"], + "job key commented": [" audit_find: # phase 1", " - name: Run finder", " next_job:"], + }.items(): + self.assertEqual(job_step_names(body, "audit_find"), ["Run finder"], label) + + def test_stops_at_the_next_job_and_ignores_other_jobs(self): + body = [ + " gate:", + " - name: Run finder", # a same-named step in ANOTHER job + " audit_find:", + " - name: Build finder prompt", + " audit_verify:", + " - name: Run verifier", + ] + self.assertEqual(job_step_names(body, "audit_find"), ["Build finder prompt"]) + + def test_a_renamed_step_is_visible(self): + body = [" audit_find:", " - name: Run the finder agent", " next_job:"] + self.assertEqual(job_step_names(body, "audit_find"), ["Run the finder agent"]) + + class AgentStepNamePinTest(unittest.TestCase): """Pin the step name this module matches to the one groom.yml produces. @@ -250,24 +338,23 @@ class AgentStepNamePinTest(unittest.TestCase): def _audit_find_step_names(self): path = os.path.join(os.path.dirname(__file__), "..", "..", "workflows", "groom.yml") with open(path, encoding="utf-8") as f: - lines = f.read().splitlines() - names, inside = [], False - for line in lines: - if re.match(r"^ [A-Za-z_][A-Za-z0-9_-]*:\s*$", line): - inside = line.strip() == "audit_find:" - continue - if inside: - m = re.match(r"^\s*- name:\s*(\S.*?)\s*$", line) - if m: - names.append(m.group(1)) - return names + return job_step_names(f.read().splitlines(), "audit_find") def test_groom_yml_names_exactly_the_agent_step_this_module_matches(self): names = self._audit_find_step_names() self.assertIn("Run finder", names, "groom.yml's audit_find job no longer has a `Run finder` step") - matched = [n for n in names if any(h in n.lower() for h in interval._AGENT_STEP_HINTS)] + matched = [n for n in names if n.strip().lower() in interval._AGENT_STEP_NAMES] self.assertEqual(matched, ["Run finder"], - f"_AGENT_STEP_HINTS must match exactly the agent step, got {matched} from {names}") + f"_AGENT_STEP_NAMES must match exactly the agent step, got {matched} from {names}") + + def test_the_pre_agent_step_fixtures_are_real_groom_yml_steps(self): + # PRE_AGENT_STEPS drives every "failed before the agent" case above; if it + # drifts from groom.yml those tests stop describing the real job. + names = self._audit_find_step_names() + for s in PRE_AGENT_STEPS: + if s["name"] == "Set up job": + continue # runner-generated, never in the YAML + self.assertIn(s["name"], names) class IntervalThresholdTest(unittest.TestCase): @@ -384,6 +471,61 @@ def test_bad_workflow_file_rejected(self): with self.assertRaises(ValueError): interval.fetch_workflow_runs("o/r", "ci-groom", run=make_gh_stub([], {})) + def test_bad_repo_rejected_for_jobs_too(self): + with self.assertRaises(ValueError): + interval.fetch_run_jobs("not-a-repo", 1, run=make_gh_stub([], {})) + + +class FetchRunJobsAcrossAttemptsTest(unittest.TestCase): + """The jobs walk must see EVERY attempt, not just the latest (BE-4809).""" + + def _recording_stub(self, pages): + urls = [] + + def _run(cmd, **kwargs): + urls.append(cmd[-1]) + return Result(stdout=json.dumps({"jobs": pages[len(urls) - 1] if len(urls) <= len(pages) else []})) + + return _run, urls + + def test_asks_for_all_attempts(self): + # `filter=latest` (the endpoint default) hides earlier attempts, which is + # what makes a re-run erase an already-billed agent step. + stub, urls = self._recording_stub([[finder_job("success")]]) + interval.fetch_run_jobs("o/r", 99, run=stub) + self.assertIn("filter=all", urls[0]) + + def test_pages_until_a_short_page(self): + full = [finder_job("success") for _ in range(interval._JOBS_PAGE_SIZE)] + stub, urls = self._recording_stub([full, [finder_job("failure")]]) + jobs = interval.fetch_run_jobs("o/r", 99, run=stub) + self.assertEqual(len(jobs), interval._JOBS_PAGE_SIZE + 1) + self.assertEqual(len(urls), 2) + self.assertIn("page=2", urls[1]) + + def test_page_walk_is_capped(self): + full = [finder_job("success") for _ in range(interval._JOBS_PAGE_SIZE)] + stub, urls = self._recording_stub([full] * (interval._MAX_JOB_PAGES + 3)) + interval.fetch_run_jobs("o/r", 99, run=stub) + self.assertEqual(len(urls), interval._MAX_JOB_PAGES) + + def test_an_early_attempt_that_spent_the_agent_still_counts(self): + # Attempt 1 ran the billed agent and died at filing; a manual re-run then + # flaked in `npm install`. The audit WAS spent — today's tick must skip, + # not re-bill it. (`filter=all` returns both attempts' jobs.) + runs = [ + {"id": 100, "status": "in_progress", "run_started_at": iso(0)}, + {"id": 99, "status": "completed", "run_started_at": iso(1)}, + ] + jobs = {"99": [ + finder_job("failure", [step("Install Claude Code", "failure")]), # attempt 2 + finder_job("failure", PRE_AGENT_STEPS + [step("Run finder"), + step("File issues", "failure")]), # attempt 1 + ]} + d = interval.evaluate("o/r", "ci-groom.yml", 100, 7.0, "schedule", NOW, run=make_gh_stub(runs, jobs)) + self.assertFalse(d["should_run"]) + self.assertEqual(d["last_run_at"], iso(1)) + if __name__ == "__main__": unittest.main()