[PR #84] [MERGED] fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809) #93

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/84
Author: @mattmillerai
Created: 7/28/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4809-groom-interval-agent-step

📝 Commits (2)

2ed2cf7 fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809)
63a6413 fix(groom): judge the finder's spend across ALL run attempts (BE-4809)

📊 Changes

4 files changed (+402 additions, -14 deletions)

View changed files

📝 .github/groom/README.md (+15 -0)
📝 .github/groom/interval.py (+108 -10)
📝 .github/groom/tests/test_interval.py (+273 -4)
📝 .github/workflows/test-groom-scripts.yml (+6 -0)

📄 Description

ELI-5

Groom is billed work, so the cadence gate asks "did a real groom already run recently?" by looking at whether the finder job ran. It counted a failed finder job as "yes, we spent the money" — sensible when the agent ran and the job died later while filing issues. But the finder does a bunch of cheap setup first (two checkouts, npm install, building the prompt, chmod'ing the clone read-only), and if any of that flakes the job also fails — having spent nothing. The gate then believed a groom had happened and skipped every daily tick for a full GROOM_INTERVAL_DAYS. One flaky npm install cost a whole cycle, silently. Now a failed finder job only counts if the billed step (Run finder) actually ran.

What changed

run_audited (.github/groom/interval.py) — a failure conclusion is now eligible but not sufficient: it also needs _agent_step_ran(job). success still counts unconditionally (the job cannot have succeeded without the agent). New _AGENT_STEP_HINTS = ("run finder",) sits next to _FINDER_JOB_HINTS and matches case-insensitively, mirroring the existing hint style.
_agent_step_ran reads the per-job steps array ({name, status, conclusion, number}) that GET /actions/runs/{id}/jobs already returns. Three cases: agent step present with a real conclusion → ran; agent step absent from a non-empty array, or present as skipped / no conclusion → did not run; no usable steps at all → count it (fail-safe, see below).
fetch_run_jobs was already unprojected (no --jq, no field filter), so nothing had to be threaded through — but its docstring now states that steps is load-bearing, because adding a field filter later would silently degrade the gate with no test failing.
Pin test — AgentStepNamePinTest parses .github/workflows/groom.yml, extracts the audit_find job's step names, and asserts Run finder is there and is the only step _AGENT_STEP_HINTS matches. Producer and consumer live in different files; this is the only thing that keeps them honest. .github/workflows/test-groom-scripts.yml now also path-filters on groom.yml so a rename there actually runs the pin (without that, the pin would sit dormant for exactly the change it guards).
README / docstrings updated with the rule and its deliberate fail-safe direction.

Verified the pin bites: renaming the step to Run the agent in a scratch copy of groom.yml fails test_groom_yml_names_exactly_the_agent_step_this_module_matches; restoring it goes green.

The one place the bias is deliberately inverted

Every other ambiguity in interval.py fails OPEN (groom more often). Missing step data does the opposite and counts the failure, per the ticket: a job with no usable steps (API shape change, truncated payload, an old hand-built fixture) keeps today's behavior, because making a genuinely-spent audit re-bill on the very next daily tick is the expensive direction. Called out explicitly in the code comment, the docstring, the README bullet and a test, since it reads as inconsistent otherwise.

Judgment calls

skipped / null agent-step conclusion counts as "did not run", not just "absent from the array". The ticket phrased the check as "the agent step reached a conclusion". GitHub reports steps after a failing one as conclusion: skipped rather than omitting them, so a literal "is it in the array with any conclusion?" test would still count a job that died in npm install, i.e. would not fix the bug on the more likely payload shape. Handling both renderings (truncated array and present-but-skipped) is what actually closes it. Run finder carries no if:, so it can never be legitimately skipped while the agent nonetheless ran.
Test-pin mirror. The ticket asked to pin the step name "the same way test_groom_yml_produces_exactly_the_marker_this_module_matches pins the (scoped: <path>) marker" — that test lives on #83's branch, which is unmerged, so it is not on main to mirror. The pin here was written from scratch to the same principle (parse the producing workflow, assert the literal the module matches).
Based on main, not stacked on #83. _AUDITED_CONCLUSIONS and run_audited predate #83 (BE-4004, #56) and this change needs nothing from it, so it builds and is verified on main.
Behavior is monotone toward grooming more. The set of jobs that can reset the clock only shrinks, so the last-real-run timestamp can only get older (or become None); a tick that used to run still runs. This change can turn a skip into a run, never a run into a skip.
Negative-claim falsification: not triggered. The diff adds no "unsupported"/deny/dead-end path and denies no capability — its user-facing effect is that groom runs more often after an unspent failure.

Overlap with #83 (BE-4757) — expect a textual conflict, they compose cleanly

#83 rewrites run_audited to make the clock per-scope, so whichever of the two lands second needs a small manual reconcile. The two checks are orthogonal and #83 already converged on the same guard-clause shape, so the merged body is: job-name hint → _AUDITED_CONCLUSIONS eligibility → if conclusion == "failure" and not _agent_step_ran(job): continue (this PR) → scope-marker branch (#83). _agent_step_ran, _AGENT_STEP_HINTS and _UNRUN_STEP_CONCLUSIONS are additive and untouched by #83. The pre-agent steps #83 adds (scope.py contain, the empty-tracked-file-list guard) land before Run finder, so they are covered by this gate automatically once merged.

Testing

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v → 95 tests, all green (was 87). New coverage: failure-after-the-agent still counts (agent success/failure/cancelled); failure whose steps stop before Run finder does NOT count; failure with Run finder present but skipped/null does NOT count; failure with no steps key / empty array / steps: null counts; success unaffected by step data; a spent failure behind an unspent one still counts (job order doesn't decide it); an end-to-end evaluate() case where yesterday's npm install failure leaves today's tick due; and the groom.yml step-name pin.

python3 .github/agents-md-integrity/check_agents_md.py --root . also passes (one pre-existing CODEOWNERS warning, unrelated).

Closes BE-4809.

_{🔄 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/84 **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-4809-groom-interval-agent-step` --- ### 📝 Commits (2) - [`2ed2cf7`](https://github.com/Comfy-Org/github-workflows/commit/2ed2cf7b9718cf3133ba27bf0167f9f097fc611d) fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809) - [`63a6413`](https://github.com/Comfy-Org/github-workflows/commit/63a6413bf3689c0c22fc1d0d8e429da1c0cc1ce3) fix(groom): judge the finder's spend across ALL run attempts (BE-4809) ### 📊 Changes **4 files changed** (+402 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+15 -0) 📝 `.github/groom/interval.py` (+108 -10) 📝 `.github/groom/tests/test_interval.py` (+273 -4) 📝 `.github/workflows/test-groom-scripts.yml` (+6 -0) </details> ### 📄 Description ## ELI-5 Groom is billed work, so the cadence gate asks "did a real groom already run recently?" by looking at whether the finder job ran. It counted a *failed* finder job as "yes, we spent the money" — sensible when the agent ran and the job died later while filing issues. But the finder does a bunch of cheap setup first (two checkouts, `npm install`, building the prompt, chmod'ing the clone read-only), and if any of that flakes the job also fails — having spent nothing. The gate then believed a groom had happened and skipped every daily tick for a full `GROOM_INTERVAL_DAYS`. One flaky `npm install` cost a whole cycle, silently. Now a failed finder job only counts if the billed step (`Run finder`) actually ran. ## What changed - **`run_audited` (`.github/groom/interval.py`)** — a `failure` conclusion is now *eligible* but not sufficient: it also needs `_agent_step_ran(job)`. `success` still counts unconditionally (the job cannot have succeeded without the agent). New `_AGENT_STEP_HINTS = ("run finder",)` sits next to `_FINDER_JOB_HINTS` and matches case-insensitively, mirroring the existing hint style. - **`_agent_step_ran`** reads the per-job `steps` array (`{name, status, conclusion, number}`) that `GET /actions/runs/{id}/jobs` already returns. Three cases: agent step present with a real conclusion → ran; agent step absent from a non-empty array, or present as `skipped` / no conclusion → did not run; **no usable `steps` at all → count it** (fail-safe, see below). - **`fetch_run_jobs`** was already unprojected (no `--jq`, no field filter), so nothing had to be threaded through — but its docstring now states that `steps` is load-bearing, because adding a field filter later would silently degrade the gate with no test failing. - **Pin test** — `AgentStepNamePinTest` parses `.github/workflows/groom.yml`, extracts the `audit_find` job's step names, and asserts `Run finder` is there and is the *only* step `_AGENT_STEP_HINTS` matches. Producer and consumer live in different files; this is the only thing that keeps them honest. `.github/workflows/test-groom-scripts.yml` now also path-filters on `groom.yml` so a rename there actually runs the pin (without that, the pin would sit dormant for exactly the change it guards). - **README / docstrings** updated with the rule and its deliberate fail-*safe* direction. Verified the pin bites: renaming the step to `Run the agent` in a scratch copy of `groom.yml` fails `test_groom_yml_names_exactly_the_agent_step_this_module_matches`; restoring it goes green. ## The one place the bias is deliberately inverted Every other ambiguity in `interval.py` fails OPEN (groom more often). Missing step data does the opposite and counts the `failure`, per the ticket: a job with no usable `steps` (API shape change, truncated payload, an old hand-built fixture) keeps today's behavior, because making a genuinely-spent audit re-bill on the very next daily tick is the expensive direction. Called out explicitly in the code comment, the docstring, the README bullet and a test, since it reads as inconsistent otherwise. ## Judgment calls - **`skipped` / null agent-step conclusion counts as "did not run", not just "absent from the array".** The ticket phrased the check as "the agent step reached a conclusion". GitHub reports steps *after* a failing one as `conclusion: skipped` rather than omitting them, so a literal "is it in the array with any conclusion?" test would still count a job that died in `npm install`, i.e. would not fix the bug on the more likely payload shape. Handling both renderings (truncated array *and* present-but-`skipped`) is what actually closes it. `Run finder` carries no `if:`, so it can never be legitimately skipped while the agent nonetheless ran. - **Test-pin mirror.** The ticket asked to pin the step name "the same way `test_groom_yml_produces_exactly_the_marker_this_module_matches` pins the `(scoped: <path>)` marker" — that test lives on #83's branch, which is unmerged, so it is not on `main` to mirror. The pin here was written from scratch to the same principle (parse the producing workflow, assert the literal the module matches). - **Based on `main`, not stacked on #83.** `_AUDITED_CONCLUSIONS` and `run_audited` predate #83 (BE-4004, #56) and this change needs nothing from it, so it builds and is verified on `main`. - **Behavior is monotone toward grooming more.** The set of jobs that can reset the clock only shrinks, so the last-real-run timestamp can only get older (or become `None`); a tick that used to run still runs. This change can turn a skip into a run, never a run into a skip. - **Negative-claim falsification: not triggered.** The diff adds no "unsupported"/deny/dead-end path and denies no capability — its user-facing effect is that groom runs *more* often after an unspent failure. ## Overlap with #83 (BE-4757) — expect a textual conflict, they compose cleanly #83 rewrites `run_audited` to make the clock per-scope, so whichever of the two lands second needs a small manual reconcile. The two checks are orthogonal and #83 already converged on the same guard-clause shape, so the merged body is: job-name hint → `_AUDITED_CONCLUSIONS` eligibility → **`if conclusion == "failure" and not _agent_step_ran(job): continue`** (this PR) → scope-marker branch (#83). `_agent_step_ran`, `_AGENT_STEP_HINTS` and `_UNRUN_STEP_CONCLUSIONS` are additive and untouched by #83. The pre-agent steps #83 adds (`scope.py contain`, the empty-tracked-file-list guard) land *before* `Run finder`, so they are covered by this gate automatically once merged. ## Testing `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` → **95 tests, all green** (was 87). New coverage: failure-after-the-agent still counts (agent `success`/`failure`/`cancelled`); failure whose steps stop before `Run finder` does NOT count; failure with `Run finder` present but `skipped`/null does NOT count; failure with no `steps` key / empty array / `steps: null` counts; `success` unaffected by step data; a spent failure behind an unspent one still counts (job order doesn't decide it); an end-to-end `evaluate()` case where yesterday's `npm install` failure leaves today's tick due; and the groom.yml step-name pin. `python3 .github/agents-md-integrity/check_agents_md.py --root .` also passes (one pre-existing CODEOWNERS warning, unrelated). Closes BE-4809. --- <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