[PR #85] [MERGED] fix(groom): a PRE-agent finder failure no longer counts as a spent audit (BE-4814) #92

New issue

Closed

opened 2026-10-26 22:36:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:09 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/85
Author: @mattmillerai
Created: 7/28/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4814-groom-preagent-failure

📝 Commits (7)

4018935 fix(groom): a PRE-agent finder failure no longer counts as a spent audit (BE-4814)
142f013 fix(groom): count a timed-out/cancelled finder as a spent audit (BE-4814)
5ccf951 docs(groom): refresh the _AUDITED_CONCLUSIONS note for the widened evidence set (BE-4814)
2592314 fix(groom): walk the NEWEST earlier attempts and anchor on the audited one (BE-4814)
f42b758 docs(groom): follow the audited_run_anchor rename through the comments (BE-4814)
b2aab89 fix(groom): never anchor the cadence clock on a re-run timestamp (BE-4814)
de43620 Merge remote-tracking branch 'origin/main' into matt/be-4814-groom-preagent-failure

📊 Changes

5 files changed (+763 additions, -370 deletions)

View changed files

📝 .github/groom/README.md (+35 -15)
📝 .github/groom/interval.py (+244 -99)
📝 .github/groom/tests/test_interval.py (+458 -251)
📝 .github/workflows/groom.yml (+16 -0)
📝 .github/workflows/test-groom-scripts.yml (+10 -5)

📄 Description

ELI-5

Groom runs on a daily cron but only really runs every GROOM_INTERVAL_DAYS; a gate decides "did we already do this recently?" by looking at whether a previous run's finder job finished. The problem: it counted a failed finder job as "we did the audit" — even when the job blew up in checkout, before the expensive agent step ever started. So one typo'd input could make groom go silent for a whole week, when it should have kept failing loudly every day until someone noticed. Now a failure only counts if we can see that the agent step actually started.

What changed

interval.py: run_audited no longer treats every failure conclusion as a spent audit. success still counts on the job conclusion alone. A failure counts only with positive evidence from the jobs API's per-job steps[] that the billed agent step (Run finder) started — new helper agent_step_started().
The half that must not regress is preserved: a run that paid for the agent and then died at a later step (the JSON assert, an artifact upload) still counts, so it can't re-spend on tomorrow's tick. That was the original rationale for counting failures at all (#56 / BE-4004) and it survives intact — only the pre-agent window is carved out.
Fail-OPEN on every ambiguity, per the ticket: no steps[], an empty one, the step absent (a job that died early reports only the steps it reached), still queued, skipped, cancelled, or a malformed entry all read as not audited → the next tick re-runs. A duplicated audit costs one run; a suppressed one hides a broken caller for a full interval.
Producer/consumer pinned, the same pattern scoped_job_marker uses: the literal lives in interval.py (agent_step_name()), groom.yml carries a load-bearing comment on the step, and test_interval.py pins both halves — scoped to the audit_find: job block, so the pin means "the billed step is named this", not "the string appears somewhere in the file".
test-groom-scripts.yml now also triggers on .github/workflows/groom.yml. Without this the pin is decorative: renaming the step touches no file under .github/groom/, so the test that catches the rename would never run on that PR.
Docs: module docstring + .github/groom/README.md bullet describing the evidence rule.

Falsification / evidence

The whole change rests on "the jobs API returns steps[], with step names verbatim even for a job nested inside a reusable workflow". That is a claim about a live API, not something a unit test I wrote can establish — tests would just assert my own premise. So I checked it against a real failed groom run (GET /repos/Comfy-Org/github-workflows/actions/runs/30258329036/jobs):

steps[] is present and populated on the groom / Audit — finder job, and step names come back unprefixed (Run finder), unlike the job name (groom / Audit — finder).
That run's Run finder step was completed / failure — a genuine post-agent failure — so under this change it still counts as a spent audit. The old behavior is preserved exactly where it should be.
The steps after the failure came back as completed / skipped (Assert finder produced JSON, Upload finder candidates), confirming the shape the pre-agent case takes: a job that dies in checkout leaves Run finder as completed/skipped, which this change correctly reads as "never started".
The payload also contains a Post Run finder step, which is exactly what a substring match would have swallowed — hence the exact-match rule and a test case for it.

Tests

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v → 99 pass. New PreAgentFailureTest covers the ticket's four cases plus: the skipped/cancelled step shapes, the truncated-steps[] shape, exact-vs-substring matching, malformed entries, and two end-to-end evaluate() assertions — a pre-agent failure yesterday leaves an 8-day-old real run as the anchor and the tick runs; a post-agent failure yesterday still skips. The sibling suites (cursor-review, agents-md-integrity) were re-run untouched and are green.

The producer pin was mutation-tested: renaming the step to Run the finder agent fails test_groom_yml_names_exactly_the_agent_step_this_module_matches (AssertionError: 0 != 1), so the pin is not decorative.

Judgment calls

I did not rename the step. The ticket asks to "name the agent step with a stable, matched-on literal"; it has been Run finder since groom.yml was introduced (#49, verified with git log -S), so a rename would be churn that also breaks the match against every historical run the gate walks. I pinned + documented the existing name instead.
Chesterton's fence — this narrows an existing check. _AUDITED_CONCLUSIONS came from #56 (BE-4004) with the stated rationale "counting a failure keeps a run that spent money but died at a later step from re-spending on the very next daily tick". That rationale is preserved by the evidence path; only the case it never contemplated (dying before the agent) changes. The change is deliberate and is the ticket's whole point.
A step with a name but no status/conclusion reads as no-evidence → not audited. The live API always sends status, so this is the unknown-shape case, and resolving it fail-open matches every other branch here.
Merge-order note for #83 (BE-4757): that PR edits the same run_audited body to add path scoping, so git will flag a conflict — deliberately, since it needs a human resolve rather than a silent take-one-side. The two conditions compose: keep #83's scope-marker matching as the job filter, then apply this PR's conclusion in _AUDITED_CONCLUSIONS or (conclusion in _AGENT_EVIDENCE_CONCLUSIONS and agent_step_started(job)) in place of its conclusion not in _AUDITED_CONCLUSIONS: continue. Neither behavior subsumes the other. This PR is based on main, not stacked on #83 — the defect predates path scoping and applies to whole-repo runs too.
Cost direction: a persistently broken caller now retries daily instead of weekly. Those retries die pre-agent, so they burn runner minutes, not agent tokens — and the visibility is the point.

Risk

Low, and one-directional. The only behavior that changes is which runs anchor the cadence clock; the worst case of a wrong answer here is one extra groom run, never a suppressed one. No consumer-facing input or output changed, so callers need no update beyond the usual SHA bump.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/85 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/28/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4814-groom-preagent-failure` --- ### 📝 Commits (7) - [`4018935`](https://github.com/Comfy-Org/github-workflows/commit/401893521b21a38c1eb9584c70a5264901f4859b) fix(groom): a PRE-agent finder failure no longer counts as a spent audit (BE-4814) - [`142f013`](https://github.com/Comfy-Org/github-workflows/commit/142f013750f1ff97174a805a844dc6a81d3233a9) fix(groom): count a timed-out/cancelled finder as a spent audit (BE-4814) - [`5ccf951`](https://github.com/Comfy-Org/github-workflows/commit/5ccf951ebf69a52d3533ccd4ec3e7746124cde2c) docs(groom): refresh the _AUDITED_CONCLUSIONS note for the widened evidence set (BE-4814) - [`2592314`](https://github.com/Comfy-Org/github-workflows/commit/259231400138d94b9eed0c2c78dff4c972f29579) fix(groom): walk the NEWEST earlier attempts and anchor on the audited one (BE-4814) - [`f42b758`](https://github.com/Comfy-Org/github-workflows/commit/f42b75857227d3d2bd253ee38e17b22caf7d54e2) docs(groom): follow the audited_run_anchor rename through the comments (BE-4814) - [`b2aab89`](https://github.com/Comfy-Org/github-workflows/commit/b2aab8994d56aba76b1c42a5dffc7bccb41a3356) fix(groom): never anchor the cadence clock on a re-run timestamp (BE-4814) - [`de43620`](https://github.com/Comfy-Org/github-workflows/commit/de43620c458528578935a2b9148aced9f89596df) Merge remote-tracking branch 'origin/main' into matt/be-4814-groom-preagent-failure ### 📊 Changes **5 files changed** (+763 additions, -370 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+35 -15) 📝 `.github/groom/interval.py` (+244 -99) 📝 `.github/groom/tests/test_interval.py` (+458 -251) 📝 `.github/workflows/groom.yml` (+16 -0) 📝 `.github/workflows/test-groom-scripts.yml` (+10 -5) </details> ### 📄 Description ## ELI-5 Groom runs on a daily cron but only *really* runs every `GROOM_INTERVAL_DAYS`; a gate decides "did we already do this recently?" by looking at whether a previous run's finder job finished. The problem: it counted a **failed** finder job as "we did the audit" — even when the job blew up in checkout, before the expensive agent step ever started. So one typo'd input could make groom go silent for a whole week, when it should have kept failing loudly every day until someone noticed. Now a failure only counts if we can *see* that the agent step actually started. ## What changed - `interval.py`: `run_audited` no longer treats every `failure` conclusion as a spent audit. `success` still counts on the job conclusion alone. A `failure` counts only with **positive evidence** from the jobs API's per-job `steps[]` that the billed agent step (`Run finder`) started — new helper `agent_step_started()`. - The half that must not regress is preserved: a run that **paid for the agent** and then died at a later step (the JSON assert, an artifact upload) still counts, so it can't re-spend on tomorrow's tick. That was the original rationale for counting failures at all (#56 / BE-4004) and it survives intact — only the *pre*-agent window is carved out. - **Fail-OPEN on every ambiguity**, per the ticket: no `steps[]`, an empty one, the step absent (a job that died early reports only the steps it reached), still `queued`, `skipped`, `cancelled`, or a malformed entry all read as **not audited** → the next tick re-runs. A duplicated audit costs one run; a suppressed one hides a broken caller for a full interval. - **Producer/consumer pinned**, the same pattern `scoped_job_marker` uses: the literal lives in `interval.py` (`agent_step_name()`), `groom.yml` carries a load-bearing comment on the step, and `test_interval.py` pins both halves — scoped to the `audit_find:` job block, so the pin means "the billed step is named this", not "the string appears somewhere in the file". - `test-groom-scripts.yml` now also triggers on `.github/workflows/groom.yml`. Without this the pin is decorative: renaming the step touches no file under `.github/groom/`, so the test that catches the rename would never run on that PR. - Docs: module docstring + `.github/groom/README.md` bullet describing the evidence rule. ## Falsification / evidence The whole change rests on "the jobs API returns `steps[]`, with step names verbatim even for a job nested inside a reusable workflow". That is a claim about a live API, not something a unit test I wrote can establish — tests would just assert my own premise. So I checked it against a **real failed groom run** (`GET /repos/Comfy-Org/github-workflows/actions/runs/30258329036/jobs`): - `steps[]` is present and populated on the `groom / Audit — finder` job, and step names come back **unprefixed** (`Run finder`), unlike the job name (`groom / Audit — finder`). - That run's `Run finder` step was `completed` / `failure` — a genuine post-agent failure — so under this change it still counts as a spent audit. The old behavior is preserved exactly where it should be. - The steps *after* the failure came back as `completed` / `skipped` (`Assert finder produced JSON`, `Upload finder candidates`), confirming the shape the pre-agent case takes: a job that dies in checkout leaves `Run finder` as `completed`/`skipped`, which this change correctly reads as "never started". - The payload also contains a `Post Run finder` step, which is exactly what a substring match would have swallowed — hence the exact-match rule and a test case for it. ## Tests `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` → **99 pass**. New `PreAgentFailureTest` covers the ticket's four cases plus: the `skipped`/`cancelled` step shapes, the truncated-`steps[]` shape, exact-vs-substring matching, malformed entries, and two end-to-end `evaluate()` assertions — a pre-agent failure yesterday leaves an 8-day-old real run as the anchor and the tick **runs**; a post-agent failure yesterday still **skips**. The sibling suites (cursor-review, agents-md-integrity) were re-run untouched and are green. The producer pin was mutation-tested: renaming the step to `Run the finder agent` fails `test_groom_yml_names_exactly_the_agent_step_this_module_matches` (`AssertionError: 0 != 1`), so the pin is not decorative. ## Judgment calls - **I did not rename the step.** The ticket asks to "name the agent step with a stable, matched-on literal"; it has been `Run finder` since groom.yml was introduced (#49, verified with `git log -S`), so a rename would be churn that also breaks the match against every *historical* run the gate walks. I pinned + documented the existing name instead. - **Chesterton's fence** — this narrows an existing check. `_AUDITED_CONCLUSIONS` came from #56 (BE-4004) with the stated rationale "counting a failure keeps a run that spent money but died at a later step from re-spending on the very next daily tick". That rationale is preserved by the evidence path; only the case it never contemplated (dying *before* the agent) changes. The change is deliberate and is the ticket's whole point. - **A step with a name but no `status`/`conclusion` reads as no-evidence → not audited.** The live API always sends `status`, so this is the unknown-shape case, and resolving it fail-open matches every other branch here. - **Merge-order note for #83 (BE-4757):** that PR edits the same `run_audited` body to add path scoping, so git will flag a conflict — deliberately, since it needs a human resolve rather than a silent take-one-side. The two conditions **compose**: keep #83's scope-marker matching as the job filter, then apply this PR's `conclusion in _AUDITED_CONCLUSIONS or (conclusion in _AGENT_EVIDENCE_CONCLUSIONS and agent_step_started(job))` in place of its `conclusion not in _AUDITED_CONCLUSIONS: continue`. Neither behavior subsumes the other. This PR is based on `main`, not stacked on #83 — the defect predates path scoping and applies to whole-repo runs too. - **Cost direction:** a persistently broken caller now retries daily instead of weekly. Those retries die pre-agent, so they burn runner minutes, not agent tokens — and the visibility is the point. ## Risk Low, and one-directional. The only behavior that changes is *which* runs anchor the cadence clock; the worst case of a wrong answer here is one extra groom run, never a suppressed one. No consumer-facing input or output changed, so callers need no update beyond the usual SHA bump. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:09 +00:00