feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346)

[mattmillerai]

Comfy-Org/cloud#5572 added a max_prs workflow_dispatch input to ONE groom caller. It cost ~40 lines of expression gymnastics to do it:

max_prs: ${{ fromJSON(contains(fromJSON('["1","2","3","5"]'), github.event.inputs.max_prs) && github.event.inputs.max_prs || '1') }}

Every line of that exists because the reusable declared max_prs as type: number. A workflow_dispatch input is always a string; GitHub rejects a string expression assigned to a type: number reusable input at startup; so the caller has to cast with fromJSON() — and fromJSON() on a non-numeric string throws during expression evaluation, killing the run before any job starts. Hence the mirrored contains() allowlist guarding the cast.

Any second caller wanting the same knob would have carried its own copy of that expression and its own allowlist to keep in sync. That is precisely the drift this repo exists to prevent — the logic belongs here, once. So this moves it.

What changed

max_prs is now type: string, and the reusable does the parse. The caller collapses to:

max_prs: ${{ github.event.inputs.max_prs || '1' }}

No fromJSON(), no allowlist mirror, no startup-throw class of failure.

Verified the type asymmetry is real rather than assumed, with actionlint on a scratch pair of workflows:

assignment	result
string expression → type: number input	rejected — string value cannot be assigned (this is #5572's whole problem)
plain max_prs: 1 → type: string input	accepted, no diagnostic

That second row is what makes this non-breaking: every existing caller passing a bare number keeps working untouched.

Parsing lives in build_select, and no case can abort a run:

input	behavior
'' / whitespace — what a SCHEDULE yields, carrying no inputs	take the input default
a number (5, 5.0, 1e3, 2)	floor-clamped to >= 0
anything else (abc, inf, nan)	0 PRs + a loud warning

That last row is the one judgement call worth a reviewer's eye. Falling back to the default would be actively wrong — a caller piloting at max_prs: 1 would silently get 5. Exiting non-zero would throw away a finder+verifier run already paid for, when every finding still files as an issue on the 0-PR path. So: fail closed, stay alive, warn loudly.

No upper bound is enforced, deliberately. #5572's contains() allowlist read as a guard, but a caller's options: list is a UI convention and a typo guard, not a security boundary — the only principals who can post past it (an API/CLI dispatch) already have repo write access and could just edit options:. The real backstops are structural and unchanged: builder PRs are review-gated and never auto-merged, the finder emits ~6-12 findings a run, and pr_size_limit bails oversized patches to issues. The typo case, which was the genuine risk, is now handled above.

The groom caller fleet, which didn't exist

groom is the fleet that most needs a bumper: a groom caller pins the reusable twice — the uses: SHA and the workflows_ref: input that loads the finder/verifier/builder briefs plus the dedup ledger. Those must move in lock-step or a run executes one version's workflow against another version's briefs.

bump-callers.sh already rewrote both pins (confirmed by dry-running its sed against both live callers). It now also re-points the # main @ <short> pin comment those callers carry — the bumper previously left it naming the old commit while the pin moved, which is worse than no comment in the one file where the pin is the whole point. Anchored to the github-workflows line and bounded to {7,12} hex, so an unrelated # main @ … note and a deliberate full-SHA comment are both left alone.

• .github/workflows/bump-groom-callers.yml — thin entrypoint over the shared script. GROOM_CALLERS, ALLOW_EMPTY=true (the fleet grows as callers land), no WIRE_BOT_SCRIPT (bot-identity injection is cursor-review-only; a groom caller must set bot_app_id itself).
• test_bump_callers.sh — a groom case covering both pins moving together, the comment rewrite, an untouched actions/checkout pin, and an intact max_prs forward expression.

Verification

• actionlint — clean across all workflows.
• shellcheck -x — clean on the script + tests.
• 97/97 bump-callers functional tests (9 new), 59/59 groom ledger tests, agents-md-integrity passes (129 lines, under the 150 aim).
• Parse exercised directly over '', ' ', '5.0', '-2', '0', 'abc', '1e3', 'inf', '-inf', 'nan', '01', ' 2 ', '1.9'.
• Bumper sed dry-run against the two real callers: both 40-hex pins rewrite, actions/checkout untouched.

Follow-ups (not in this PR)

1. Seed GROOM_CALLERS with the two existing callers (a variable edit, no commit).
2. Reduce cloud#5572 to the one-line forward, pinned at this PR's merge SHA.

ELI5

The groomer opens at most a few cleanup PRs per weekly run. Somebody wanted a dropdown on the manual "Run workflow" button to ask for more just that once — and doing it in one repo took forty lines of fiddly YAML, because the shared workflow insisted the number be a number and dispatch buttons can only hand over text.

This teaches the shared workflow to accept the text and sort it out itself. The dropdown is now three words in a caller instead of forty lines, every repo gets it the same way, and a typo gets you zero PRs and a warning instead of a crashed run.

It also adds the robot that keeps groom callers' pinned versions up to date — groom callers pin the version in two places, and nobody had wired that up yet.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dbb952aa-eef4-4184-8e33-5f0a6a84a7d2

📥 Commits

Reviewing files that changed from the base of the PR and between 9722179 and 84f5a32.

📒 Files selected for processing (8)

• .github/bump-callers/README.md
• .github/bump-callers/bump-callers.sh
• .github/bump-callers/tests/test_bump_callers.sh
• .github/workflows/bump-groom-callers.yml
• .github/workflows/groom.yml
• .github/workflows/test-bump-callers.yml
• AGENTS.md
• README.md

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/groom-max-prs-reusable

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/groom-max-prs-reusable

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 6 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	2
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 7 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	3
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-4661 — Gate the bump-*-callers fleet's org-scoped bot token behind a main-only protected environment
• BE-4662 — Make bump-callers.sh target the github-workflows pin precisely and assert both pins moved before committing

[mattmillerai]

Merged origin/main (9722179) into the branch — two conflicts, one textual and one semantic.

Textual — bump-callers.sh, the pin-rewrite block. Main's BE-4523 (#75) landed the multi-reusable attribution guard (GW_USES / the SHA_ADDR tightening / the main (<short>) marker refresh) in the same lines this branch was rewriting for the groom callers' # main @ <short> form.

Resolved by keeping main's structure whole and putting the # main @ rules in the same sed as the 40-hex rule, sharing ${SHA_ADDR} verbatim — not under the GW_ONLY_OURS guard, which was the obvious-looking merge. SHA_ADDR is exactly the set of lines whose pin the 40-hex rule rewrites, and a # main @ annotation rides on that line, so the address is honest attribution for it: in the sibling regime the tightened address skips a sibling's uses: line, so neither its pin nor its annotation gets stamped with this fleet's SHA. The main (<short>) form can't use the same trick — it also appears in prose above the uses: line, naming no pin context of its own — which is why it keeps the caller-level guard.

Gating # main @ on that guard instead would have manufactured the exact stale comment 46abc9f removed: a workflows_ref: pin is bumped even in a multi-reusable file (that context stays broad, by main's own reasoning that an un-bumped workflows_ref disagreeing with its own uses: is the worse failure), so its annotation has to move with it. This also keeps the anchor-parity note from the earlier thread true for BE-4662: all three pin-line rules now share one address literal.

Three assertions added for the merged interaction, which neither branch's suite covered (sibling's # main @ not stamped; workflows_ref bumped even here; its # main @ moved with it). Mutation-checked both directions — dropping the # main @ rules fails the groom fixture, widening them back to the pre-BE-4523 broad address fails the new sibling assertion.

Semantic — bump-groom-callers.yml. No textual conflict (new file vs. modified siblings), which is why it merged silently: main's BE-4058 (#62) hardened all four existing bump entrypoints, and this branch adds a fifth written from the pre-hardening template. The merged result shipped four guarded fleets and one open one — and made the header's "the entrypoints differ only in their path-filter trigger and the parameters passed below" false. Brought to parity: main-only ref guard, stale re-run tip check, decommission guard (checking .github/groom/ as well as groom.yml, since a groom caller pins both), and the SHA-pinned actions/checkout@df4cb1c the sweep moved everything to.

Worth naming, since it touches an earlier reply: the if: github.ref == 'refs/heads/main' I added is main's accidental-stale-dispatch guard, not a security control. My reasoning on BE-4661 still stands — an attacker dispatching their own branch supplies the workflow file and deletes the guard first. That path stays open on all five fleets and stays tracked as BE-4661 (needs the server-side environment gate). Also stale now: that same reply said the four siblings "all still carry actions/checkout@v6" — #62 fixed them, and this merge brings groom onto the same pin.

Verification on the merged tree: 123 bump-callers assertions pass, shellcheck clean, groom / agents-md-integrity / cursor-review suites green.

No new deferrals — BE-4661 and BE-4662 already cover the two open findings.