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 6 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.
📝 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
1 participant
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.