fix(groom): a PRE-agent finder failure no longer counts as a spent audit (BE-4814) - #85
fix(groom): a PRE-agent finder failure no longer counts as a spent audit (BE-4814)#85mattmillerai wants to merge 6 commits into
Conversation
…dit (BE-4814) The interval gate counted ANY finder job concluding `success` or `failure` as "this scope was audited", so a job that died before the billed agent step — checkout, the asset load, the prompt build, a runner hiccup — advanced the cadence clock. On a scheduled caller that suppressed every tick for a whole GROOM_INTERVAL_DAYS window, hiding a typo'd input or a broken caller for days instead of letting it recur daily until someone noticed. The gate is fail-OPEN by design everywhere else; this was the one branch that failed closed. A `failure` now needs positive evidence that the agent step ran: the runs-jobs API's per-job `steps[]` must show `Run finder` started. A post-agent failure still counts (that half is why failures counted at all — a run that spent money must not re-spend tomorrow); a `success` is trusted on the job conclusion alone. Every ambiguity — no `steps[]`, empty, step absent, still queued, or skipped — reads as NOT audited, i.e. re-run. The step name is the producer half of a matched literal, so groom.yml carries a load-bearing comment, interval.py owns the literal (`agent_step_name()`), and the suite pins both halves — with groom.yml added to test-groom-scripts.yml's trigger paths so a rename there actually runs the test that catches it.
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: 22 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 (5)
✨ 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 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…814) Review follow-ups on the pre-agent-failure gate. All four are the same bug class the PR set out to fix, in the two directions it can point. - `timed_out`/`cancelled` join `failure` on the evidence path. The finder job runs under `timeout-minutes: 40`, so a hung agent bills the FULL window and only then trips the timeout — the costliest outcome there is. Those conclusions could never count, so that spend was forgotten and repeated on every subsequent daily tick. - A `cancelled` STEP is no longer read as "never started": GitHub stamps an in-flight step `cancelled` when the job is cancelled or times out. The conclusion can't separate that from a step never reached, so the timestamps do — `started_at` strictly before `completed_at` proves it ran; no span is no evidence (fail-open, as everywhere else here). - Evidence is looked for across a run's earlier ATTEMPTS. The jobs endpoint reports only the latest, so a manual re-run that dies in checkout erased the record of an attempt that had reached the agent. Bounded at 5 and skipped entirely for the `run_attempt: 1` norm. - Junk payloads: every API field now reads through `_text()` and non-dict job/step entries are skipped, making good on the docstring's promise that the scan doesn't raise. Job `conclusion` is normalized like the step fields so a casing variance can't pass one check but not the other. The half that must not regress is pinned throughout: a run that ended before the agent still bills nothing and still may not advance the clock, under every one of the widened conclusions.
…idence set (BE-4814)
mattmillerai
added
cursor-review
|
Review pass complete — all 6 panel findings addressed in 142f013 / 5ccf951 (replies on each thread, all resolved). Fixed (5):
Deferred (1): 🟡 the "a started step is not proof of a billed request" finding (a bad API key/model id fails fast having spent nothing). Real, but closing it needs a new definition of "billed" — a duration floor vs. an explicit post-agent marker — and that is a design call, not a mechanical edit. Written up as a follow-up with both options, the trade-off and the required test directions; thread reply here. The half that must not regress is pinned throughout: a run that ended before the agent still bills nothing and still may not advance the cadence clock, under every one of the widened conclusions. Tests 88 → 111, all green; Note: CodeRabbit has not actually reviewed this PR — it is rate-limited under its fair-usage policy (its status check reports Not merging — review-gated, a human presses merge. |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
Panel: 7/8 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.6-sol-max:adversarial (error)
…d one (BE-4814) Second review round. The first fix's attempt-walk had a real direction bug. - `range(min(attempts, CAP) - 1, 0, -1)` scanned attempts 1..4 of a run re-run 50 times — the OLDEST — so a billed audit sitting on attempt 49 was never read and got re-spent, which is the exact forgotten-spend the function exists to prevent. The window now slides with `run_attempt`. The old test asserted only the CALL COUNT, which passes either way and masked it; it now asserts WHICH attempts are fetched (49..46, not 1..4). - When an EARLIER attempt supplies the evidence, the clock anchored on the run's `run_started_at` — which tracks the LATEST attempt. A pre-agent re-run today of a run whose attempt 1 paid a week ago therefore read as "audited today" and suppressed the next full interval: fail-CLOSED, the direction this gate exists to avoid. It now anchors on the audited attempt's own finder-job start, falling back to the run's creation (older anchor = more elapsed = fail-open) rather than the re-run time. - The evidence-conclusion allowlist became a denylist. Only `skipped` (the interval-skip) and an unfinished run are excluded; every other ending, including the rarer `neutral`/`stale`/`action_required`, is decided by the step evidence — if the agent ran, the audit was spent however the job was finally stamped. An allowlist silently forgets what it omits.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 6 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…4814) Third review round. - The earlier-attempt fallback chain ended with `run_started_at` — the re-run time the comment directly above it rejects. With no finder-job start and no `created_at` it would date a week-old audit to today and suppress the next full interval: fail-CLOSED. The chain now stops at `created_at`/None, so an unanchorable run is skipped and the scan moves to an older one, which is the fail-open direction. - The latest-attempt branch dropped the whole run when `run_started_at` was missing instead of using the same fallbacks. Both paths now share one chain. - `fetch_run_jobs` guards `run_id` the way it already guarded `repo`, and `find_last_audited_run_at` SKIPS a junk entry rather than letting the raise abort the scan (which would fail open on the whole decision and forget the real history behind it). - `finder_job_started_at` scans past a hint-matching job that carries no timestamp instead of giving up on the first match. - The gate job gets `timeout-minutes: 10` — the only job in groom.yml without a bound, and its cost is data-dependent (up to _MAX_RUNS_SCANNED runs x _MAX_ATTEMPTS_SCANNED attempts at a 30s per-call timeout). - The groom.yml pin test now also asserts the agent step carries no `if:`. A `success` is trusted on the job conclusion ALONE, so a conditionally skipped agent inside a succeeding job would count as a spent audit while billing nothing. That was defended only by a comment; now by a test.
|
Rounds 2 and 3 of the review panel are also addressed — 19 findings total across 3 rounds, all threads resolved. Rounds 2 and 3 reviewed my own fixes and each caught a real bug in them, which is why I re-requested rather than stopping at round 1. Round 2 (4 findings) —
|
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
2 participants
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_auditedno longer treats everyfailureconclusion as a spent audit.successstill counts on the job conclusion alone. Afailurecounts only with positive evidence from the jobs API's per-jobsteps[]that the billed agent step (Run finder) started — new helperagent_step_started().steps[], an empty one, the step absent (a job that died early reports only the steps it reached), stillqueued,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.scoped_job_markeruses: the literal lives ininterval.py(agent_step_name()),groom.ymlcarries a load-bearing comment on the step, andtest_interval.pypins both halves — scoped to theaudit_find:job block, so the pin means "the billed step is named this", not "the string appears somewhere in the file".test-groom-scripts.ymlnow 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..github/groom/README.mdbullet 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 thegroom / Audit — finderjob, and step names come back unprefixed (Run finder), unlike the job name (groom / Audit — finder).Run finderstep wascompleted/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.completed/skipped(Assert finder produced JSON,Upload finder candidates), confirming the shape the pre-agent case takes: a job that dies in checkout leavesRun finderascompleted/skipped, which this change correctly reads as "never started".Post Run finderstep, 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. NewPreAgentFailureTestcovers the ticket's four cases plus: theskipped/cancelledstep shapes, the truncated-steps[]shape, exact-vs-substring matching, malformed entries, and two end-to-endevaluate()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 agentfailstest_groom_yml_names_exactly_the_agent_step_this_module_matches(AssertionError: 0 != 1), so the pin is not decorative.Judgment calls
Run findersince groom.yml was introduced (feat(groom): reusable groom.yml — two-phase finder→verifier, GitHub-issue sink (BE-3872) #49, verified withgit 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._AUDITED_CONCLUSIONScame from feat(groom): configurable run cadence via GROOM_INTERVAL_DAYS interval gate (BE-4004) #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.status/conclusionreads as no-evidence → not audited. The live API always sendsstatus, so this is the unknown-shape case, and resolving it fail-open matches every other branch here.pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83 (BE-4757): that PR edits the samerun_auditedbody 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 feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83's scope-marker matching as the job filter, then apply this PR'sconclusion in _AUDITED_CONCLUSIONS or (conclusion in _AGENT_EVIDENCE_CONCLUSIONS and agent_step_started(job))in place of itsconclusion not in _AUDITED_CONCLUSIONS: continue. Neither behavior subsumes the other. This PR is based onmain, not stacked on feat(groom):pathinput scopes an audit to one directory, without resetting the cadence clock (BE-4757) #83 — the defect predates path scoping and applies to whole-repo runs too.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.