feat(groom): configurable run cadence via GROOM_INTERVAL_DAYS interval gate (BE-4004)#56
feat(groom): configurable run cadence via GROOM_INTERVAL_DAYS interval gate (BE-4004)#56mattmillerai wants to merge 8 commits into
Conversation
…l gate (BE-4004) GitHub Actions `schedule:` cron is static in the workflow file — there is no native runtime "every N days". Replace groom's hardcoded weekly cron with the standard frequent-base-cron + runtime-gate pattern so cadence is a live knob: - ci-groom.yml now fires DAILY; the effective cadence is a new runtime gate. - New reusable input `interval_days` (default 7 = today's weekly behavior), wired in the caller to repo Actions variable GROOM_INTERVAL_DAYS. Changing the variable retunes cadence (weekly -> every-3-days -> daily) with no file edit. - `.github/groom/interval.py`: the gate. At run start it derives the last REAL groom run from this caller's Actions run history (a run counts only if its finder job actually ran, so interval-skip ticks never reset the clock) and early-exits cheaply — before the finder — unless interval_days have elapsed. Durable across stateless CI runs, no net-new secret, only `actions: read`. Pure decision logic split from the gh I/O shell + a unittest suite, mirroring ledger.py. - `workflow_dispatch` always bypasses the interval gate (manual override). - Fail-open on any history-read error (mirrors the volume gate). The gate runs in the existing `gate` job before the finder; the volume gate is kept as a second throttle with its window matched to the effective cadence.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 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 (3)
📝 WalkthroughWalkthroughThe groom workflows now run on a daily base schedule and apply configurable interval and volume gates. The interval gate inspects audited workflow history, supports manual-dispatch bypasses, fails open on errors, and emits JSON decisions. ChangesGroom cadence control
Sequence Diagram(s)sequenceDiagram
participant GroomWorkflow
participant interval.py
participant GitHubActions
participant VolumeGate
GroomWorkflow->>interval.py: Evaluate interval gate
interval.py->>GitHubActions: Read workflow runs and finder jobs
GitHubActions-->>interval.py: Return run history
interval.py-->>GroomWorkflow: Return interval result
GroomWorkflow->>VolumeGate: Evaluate merged-PR volume when interval allows
VolumeGate-->>GroomWorkflow: Return volume result
GroomWorkflow-->>GroomWorkflow: Set should_run from both gate results
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
agent-coded
mattmillerai
added
cursor-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 55-59: Update the interval.py description in AGENTS.md to state
that the cadence gate derives its state from GitHub Actions run history, rather
than GitHub issue and PR state. Keep the issue/PR storage description associated
only with the ledger behavior and preserve the existing GROOM_INTERVAL_DAYS
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 679677d0-a97e-48ad-837f-ec1ed95ed544
📒 Files selected for processing (7)
.github/groom/README.md.github/groom/interval.py.github/groom/tests/test_interval.py.github/workflows/ci-groom.yml.github/workflows/groom.ymlAGENTS.mdREADME.md
The cadence gate derives state from Actions run history, not GitHub issue+PR state (that's the ledger's store); drop the misplaced sentence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…terval-days # Conflicts: # .github/workflows/ci-groom.yml # .github/workflows/groom.yml # README.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci-groom.yml (2)
82-94: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace both placeholders before merge.
uses:andworkflows_refneed the same real squash-merge SHA, anduses:should keep the trailing# v1comment. As-is, the reusable pin won’t resolve cleanly once this lands, and the groomer will go bonkers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-groom.yml around lines 82 - 94, Replace both REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA placeholders in the groom workflow configuration with the actual squash-merge commit SHA, using the identical SHA for uses and workflows_ref. Preserve the trailing # v1 comment on the uses reference.Source: Coding guidelines
98-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize the cadence once before both gates.
interval.pyalready turns blank, negative, and bad values into7while honoring0, but the volume gate still feedsinputs.cadencestraight intodate -d. That means the two gates can drift apart, and0won’t mean the same thing in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-groom.yml around lines 98 - 104, Normalize GROOM_INTERVAL_DAYS once using the existing interval.py behavior, including blank, negative, invalid, and zero values, then reuse that normalized value for both interval_days and cadence. Update the workflow’s volume-gate input so it no longer passes the raw inputs.cadence directly to date -d, keeping both gates aligned on the same effective cadence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 18: The README workflow description incorrectly says the audit job is
contents-read-only. Update the groom.yml description in the table entry to state
that the audit agent jobs have no repository write permissions, while preserving
the existing explanation that finder/verifier jobs require id-token: write for
OIDC.
---
Outside diff comments:
In @.github/workflows/ci-groom.yml:
- Around line 82-94: Replace both REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA
placeholders in the groom workflow configuration with the actual squash-merge
commit SHA, using the identical SHA for uses and workflows_ref. Preserve the
trailing # v1 comment on the uses reference.
- Around line 98-104: Normalize GROOM_INTERVAL_DAYS once using the existing
interval.py behavior, including blank, negative, invalid, and zero values, then
reuse that normalized value for both interval_days and cadence. Update the
workflow’s volume-gate input so it no longer passes the raw inputs.cadence
directly to date -d, keeping both gates aligned on the same effective cadence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35764ef5-53ef-4885-9882-f0855d3d0063
📒 Files selected for processing (4)
.github/workflows/ci-groom.yml.github/workflows/groom.ymlAGENTS.mdREADME.md
…4004)
Both gates are wired to the same GROOM_INTERVAL_DAYS knob in the caller, but
only the interval gate normalized it — the volume gate fed inputs.cadence
straight to `date -d`, so the two drifted on reachable values:
-3 interval.py -> 7 (safe weekly default), but `date -d '-3 days ago'` is a
cutoff 3 days in the FUTURE, matching no merged PR -> the volume gate
skips EVERY run, silently disabling groom.
0 a legitimate "no throttle" for the interval gate, but a 0-day merge
window is today-only, so most ticks are judged quiescent.
Add normalize_cadence_days() next to parse_interval_days() (same
blank/garbage/negative -> 7 degradation, then floored at 1 whole day) plus a
flag-only `interval.py --normalize-cadence <raw>` mode the volume gate shells
out to, so one parser backs both gates. The floor removes no capability:
`volume_gate: false` is the input that expresses "no merge-activity throttle".
Also correct the README's groom permission claim — the audit jobs are not
`contents: read` only; they also hold the `id-token: write` claude-code-action
needs to mint its OIDC token. Reworded to "no repository write credentials",
which is the property that actually matters.
Both per CodeRabbit review on #56.
|
Addressed the CodeRabbit review in 160fdea. Both out-of-diff findings, plus the inline README one:
Fixed in the reusable (
Confirmed the drift empirically against a GNU-
The Covered by 5 new cases in
|
Reconcile the interval-gate feature (BE-4004) with main's BE-4214 agent-CLI migration: agent jobs no longer need id-token: write (they invoke the Claude CLI directly), but the interval gate still needs actions: read to inspect this workflow's run history.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/groom/interval.py (1)
93-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle non-finite cadence values before the
int()cast
GROOM_INTERVAL_DAYS=infornanslips pastparse_interval_days()and makesnormalize_cadence_days()throw on theint()conversion, so the--normalize-cadencepath can crash instead of falling back like the other bad inputs. A tinymath.isfinite()guard would keep the float gremlins from sneaking through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/groom/interval.py around lines 93 - 133, Update parse_interval_days to reject non-finite float values using math.isfinite before returning the parsed value, so inf and nan fall back to default like other invalid inputs. Keep normalize_cadence_days and valid finite-value behavior unchanged..github/workflows/groom.yml (1)
287-294: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
issues: readandpull-requests: readtogate
gaterunsgh api search/issuesin the volume gate, and that token scope needs both permissions. Without them, the call can 403 and the gate fail open, so the repo keeps on gadding about instead of skipping quiet runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/groom.yml around lines 287 - 294, Add issues: read and pull-requests: read to the gate job’s permissions block in the workflow, alongside contents: read and actions: read, so the gh api search/issues call in the volume gate has the required scopes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/groom/interval.py:
- Around line 93-133: Update parse_interval_days to reject non-finite float
values using math.isfinite before returning the parsed value, so inf and nan
fall back to default like other invalid inputs. Keep normalize_cadence_days and
valid finite-value behavior unchanged.
In @.github/workflows/groom.yml:
- Around line 287-294: Add issues: read and pull-requests: read to the gate
job’s permissions block in the workflow, alongside contents: read and actions:
read, so the gh api search/issues call in the volume gate has the required
scopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28e9a0c1-f810-4a68-9e12-1a4aa040be20
📒 Files selected for processing (6)
.github/groom/README.md.github/groom/interval.py.github/groom/tests/test_interval.py.github/workflows/ci-groom.yml.github/workflows/groom.ymlREADME.md
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Solid design overall — daily base cron + runtime gate, state from run history, fail-open everywhere, pure logic split from gh I/O, good test coverage. Two issues before merge (inline comments):
- Cron-jitter drift in
interval_decision:elapsed >= interval_dayswith once-a-day ticks and GitHub's late-firing cron makes a 7-day interval oscillate to 7–8 days and drift later over time. One-line fix: half-a-tick tolerance or calendar-day comparison. type: numberoninterval_days/cadencedefeats the documented garbage-degradation: a non-numericGROOM_INTERVAL_DAYSfails the workflow at call time (fail-closed) beforeparse_interval_daysever runs. Either switch the inputs totype: stringso the Python parser owns normalization, or fix the docs.
…nputs (BE-4004)
Review follow-ups on the BE-4004 cadence gate.
1. Cron jitter drifted the cadence by whole days. The gate compared fractional
days elapsed since the last real run's `run_started_at` against the full
`interval_days`, but ticks only arrive once per day and GitHub's scheduler
fires cron late by an unpredictable amount. A last real run that started Mon
09:31 (scheduler delay) against next Monday's 09:19 tick measures 6.99 days
< 7 and skips; the run slips to Tuesday, the clock re-anchors on THAT run,
and the cadence ratchets later every cycle. The bar is now `interval_days`
less a half-tick of slack (`interval_threshold`), capped at half the interval
so a sub-daily cadence keeps a real throttle. It cannot pull a run a whole
tick early — consecutive daily ticks are ~1.0 day apart, well over the 0.5
tolerance. Boundary tests at 6.99/6.75/6.5 (run) and 6.49/6.25/6.0 (skip).
2. `type: number` made the documented garbage-degradation unreachable, and
failed CLOSED. Both `interval_days` and `cadence` are wired to
`${{ vars.GROOM_INTERVAL_DAYS || '7' }}` — a free-text Actions variable a
human edits. A non-numeric value ("weekly", "7d") is rejected by GitHub's
string->number coercion at workflow-call time, so the run fails loudly and
`parse_interval_days` / `normalize_cadence_days` never get to degrade it to
7. That is fail-closed, the opposite of this gate's stated design, and the
README/docstrings claimed a path GitHub's own type check made unreachable.
Both inputs are now `type: string` (default '7'), making interval.py the
single normalization authority. Both only ever flow into shell env vars, and
groom.yml's sole caller passes string expressions already.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/groom.yml (1)
101-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument manual dispatch behavior precisely.
The documentation presents
workflow_dispatchas an always-run bypass, but the final decision at Lines [426-438] requires both interval and volume gates to be true. Manual dispatch bypasses interval throttling; the existing volume gate can still prevent execution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/groom.yml around lines 101 - 109, Update the workflow documentation near the cadence description and the final decision logic to state that workflow_dispatch bypasses interval throttling but remains subject to the volume gate. Ensure the documented behavior matches the implementation at the final decision path, rather than describing manual dispatch as an unconditional run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/groom/interval.py:
- Around line 76-90: Correct the explanatory comment above the elapsed-days
tolerance so it states that consecutive daily ticks are approximately one day
apart and therefore exceed the 0.5-day tolerance. Do not change the tolerance
logic or surrounding cadence behavior.
---
Outside diff comments:
In @.github/workflows/groom.yml:
- Around line 101-109: Update the workflow documentation near the cadence
description and the final decision logic to state that workflow_dispatch
bypasses interval throttling but remains subject to the volume gate. Ensure the
documented behavior matches the implementation at the final decision path,
rather than describing manual dispatch as an unconditional run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1adbf2e6-69f5-4dd1-ad7c-7082ee06c2dc
📒 Files selected for processing (4)
.github/groom/README.md.github/groom/interval.py.github/groom/tests/test_interval.py.github/workflows/groom.yml
… comment (BE-4004) Addresses remaining CodeRabbit findings: parse_interval_days now rejects inf/nan (would otherwise wedge the interval gate via a NaN threshold, or crash normalize_cadence_days's int() cast); the gate job's permissions block was missing issues:read/pull-requests:read, so the volume gate's search/issues call 403s on every run and always fails open, silently neutering the gate; and corrects the inverted tolerance comment at interval.py:90.
|
Addressed the remaining CodeRabbit findings in 55edfa6:
|
2 participants
ELI-5
Groom used to run on a hardcoded weekly cron — to change how often it runs you had to edit the workflow file. GitHub Actions crons can't be "every N days" at runtime, so this switches groom to the standard trick: fire a cheap tick every day, but only actually do work every
Ndays, whereNis a live knob you set with a repo variable (GROOM_INTERVAL_DAYS, default7= today's weekly). A tick that's too soon exits in a few seconds before the expensive finder ever starts. Change the variable, cadence changes — no PR, no YAML edit.The interval gate lives in the new
groom.yml, soci-groom.ymlpins a placeholder (@REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA) that fails loud (unresolvable ref / failed assets checkout) until bumped. This is the same reusable-lands-then-bump-caller two-step the pilot used (#49 → #53), compressed into one PR. After squash-merge, bump bothuses:andworkflows_refin.github/workflows/ci-groom.ymlto this PR's merge SHA. groom is schedule/dispatch-only (never on-PR), so nothing breaks on the PR itself; the placeholder just guarantees the daily cron can't silently run against the old (gate-less) reusable.(No
bump-groom-callers.ymlautomation exists yet — groom has a single caller. A dedicated bumper is a reasonable follow-up if more repos enroll.)What changed
ci-groom.yml(caller): weekly cron → daily17 9 * * *; grantsactions: read; wiresinterval_days+cadenceto${{ vars.GROOM_INTERVAL_DAYS || '7' }}.groom.yml(reusable): newinterval_daysinput (default 7); thegatejob now checks out the assets, runs the interval gate before the finder, then the (unchanged) volume gate, composing both intoshould_run. Gate job gainsactions: read..github/groom/interval.py+ tests: the gate. Derives the last real groom run from this caller's Actions run history — a run counts only if itsAudit — finderjob actually ran, so interval-skip ticks never reset the clock — and early-exits unlessinterval_dayselapsed. Pure decision logic split from theghI/O shell, mirroringledger.py. Fail-open on any history-read error..github/groom/README.md(new section), AGENTS.md.Acceptance criteria
GROOM_INTERVAL_DAYS(default 7); changing it changes frequency with no workflow-file edit.gatejob, before the finder).workflow_dispatchalways runs regardless of the interval.actions: read).Judgment calls (worth a reviewer's eye)
GROOM_LAST_RUNrepo variable. The ticket sanctioned either. A repo variable needs aVariables: writecredential the run doesn't otherwise carry — and a missing grant would fail silently into a daily over-spend. Run history is durable, needs onlyactions: read, and has no silent-failure trap.inputwired to the var (rather than the reusable readingvars.GROOM_INTERVAL_DAYSdirectly). The caller evaluates the var in its own context — unambiguous — matching howcadence/dry_run/volume_gateare already wired. Same "no file edit" ergonomics.workflow_dispatchbypasses the interval gate but not the pre-existing volume gate for a non-dry-run dispatch. That volume-gate-on-dispatch behavior is a deliberate spend guard the pilot chose (volume_gate: dry_run != 'true'); the ticket's "always runs regardless of the interval" is about the interval gate, so I left the volume gate alone (Chesterton's fence). A dry-run dispatch bypasses both, as before.github.workflow_refresolves to the caller (ci-groom.yml), not the reusable — the OIDCworkflow_ref(caller) vsjob_workflow_ref(reusable) distinction. If that assumption were ever wrong, the run-history query returns nothing → the gate fails open (runs), never crashes.interval.py.Testing
python3 -m unittest discover -s .github/groom/tests→ 54 passed (16 new for the interval gate: within/at/after-interval, skip-ticks-don't-reset, dispatch-always-runs, no-history/API-error fail-open, job-name matching).actionlint(with shellcheck on the embeddedrun:blocks) clean on both workflows.