fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809) - #84
fix(groom): a failed finder job counts as a spent audit only if the agent step ran (BE-4809)#84mattmillerai wants to merge 2 commits into
Conversation
…gent step ran (BE-4809) 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.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| ⚪ Nit | 2 |
Panel: 8/8 reviewers contributed findings.
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.
2 participants
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 fullGROOM_INTERVAL_DAYS. One flakynpm installcost 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) — afailureconclusion is now eligible but not sufficient: it also needs_agent_step_ran(job).successstill counts unconditionally (the job cannot have succeeded without the agent). New_AGENT_STEP_HINTS = ("run finder",)sits next to_FINDER_JOB_HINTSand matches case-insensitively, mirroring the existing hint style._agent_step_ranreads the per-jobstepsarray ({name, status, conclusion, number}) thatGET /actions/runs/{id}/jobsalready returns. Three cases: agent step present with a real conclusion → ran; agent step absent from a non-empty array, or present asskipped/ no conclusion → did not run; no usablestepsat all → count it (fail-safe, see below).fetch_run_jobswas already unprojected (no--jq, no field filter), so nothing had to be threaded through — but its docstring now states thatstepsis load-bearing, because adding a field filter later would silently degrade the gate with no test failing.AgentStepNamePinTestparses.github/workflows/groom.yml, extracts theaudit_findjob's step names, and assertsRun finderis there and is the only step_AGENT_STEP_HINTSmatches. Producer and consumer live in different files; this is the only thing that keeps them honest..github/workflows/test-groom-scripts.ymlnow also path-filters ongroom.ymlso a rename there actually runs the pin (without that, the pin would sit dormant for exactly the change it guards).Verified the pin bites: renaming the step to
Run the agentin a scratch copy ofgroom.ymlfailstest_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.pyfails OPEN (groom more often). Missing step data does the opposite and counts thefailure, per the ticket: a job with no usablesteps(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 asconclusion: skippedrather than omitting them, so a literal "is it in the array with any conclusion?" test would still count a job that died innpm 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 findercarries noif:, so it can never be legitimately skipped while the agent nonetheless ran.test_groom_yml_produces_exactly_the_marker_this_module_matchespins the(scoped: <path>)marker" — that test lives on feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83's branch, which is unmerged, so it is not onmainto mirror. The pin here was written from scratch to the same principle (parse the producing workflow, assert the literal the module matches).main, not stacked on feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83._AUDITED_CONCLUSIONSandrun_auditedpredate feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83 (BE-4004, feat(groom): configurable run cadence via GROOM_INTERVAL_DAYS interval gate (BE-4004) #56) and this change needs nothing from it, so it builds and is verified onmain.None); a tick that used to run still runs. This change can turn a skip into a run, never a run into a skip.Overlap with #83 (BE-4757) — expect a textual conflict, they compose cleanly
#83 rewrites
run_auditedto 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_CONCLUSIONSeligibility →if conclusion == "failure" and not _agent_step_ran(job): continue(this PR) → scope-marker branch (#83)._agent_step_ran,_AGENT_STEP_HINTSand_UNRUN_STEP_CONCLUSIONSare additive and untouched by #83. The pre-agent steps #83 adds (scope.py contain, the empty-tracked-file-list guard) land beforeRun 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 (agentsuccess/failure/cancelled); failure whose steps stop beforeRun finderdoes NOT count; failure withRun finderpresent butskipped/null does NOT count; failure with nostepskey / empty array /steps: nullcounts;successunaffected by step data; a spent failure behind an unspent one still counts (job order doesn't decide it); an end-to-endevaluate()case where yesterday'snpm installfailure 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.