feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) - #77
Conversation
…the groom caller fleet (BE-4346) Comfy-Org/cloud#5572 added a `max_prs` workflow_dispatch input to ONE caller, and 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') }} All 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 must 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. Every caller that wanted the knob would have carried its own copy of that, which is exactly the drift this repo exists to prevent: the logic belongs here, once. So `max_prs` becomes `type: string` and the reusable does the parse. Verified with actionlint that the asymmetry is real: a string expression into a `type: number` input is rejected, while a plain `max_prs: 1` into a `type: string` input is accepted — so existing callers need no change. The caller collapses to: max_prs: ${{ github.event.inputs.max_prs || '1' }} Parsing lives in build_select, and none of its cases can abort a run: empty (what a SCHEDULE yields, carrying no inputs) takes the default; a number is clamped to >= 0; anything else opens ZERO PRs with a loud warning. That last case is the one judgement call — falling back to the DEFAULT would be actively wrong, since a caller piloting at `max_prs: 1` would silently get 5, and exiting non-zero would throw away a finder+verifier run already paid for while the findings still file as issues on the 0-PR path. No upper bound is enforced: a caller's `options:` list is a UI convention and a typo guard, not a security boundary — anyone who can API-dispatch already has write access and could edit `options:` anyway. Also adds the groom caller fleet, which did not exist. It is the fleet that most needs one: a groom caller pins the reusable TWICE (`uses:` and the `workflows_ref:` that loads the briefs + ledger), and 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; it now also re-points the `# main @ <short>` pin comment those callers carry, because a comment still naming the old commit after the pin moved is worse than no comment. Anchored to the github-workflows line and bounded to {7,12} hex so an unrelated note and a deliberate full-SHA comment are both left alone. - .github/workflows/bump-groom-callers.yml — thin entrypoint, GROOM_CALLERS, ALLOW_EMPTY=true (the fleet grows as callers land), no WIRE_BOT_SCRIPT. - test_bump_callers.sh — a groom case covering both pins, the comment rewrite, an untouched actions/checkout pin, and an intact max_prs forward expression. Verified: actionlint clean; shellcheck clean; 97/97 bump-callers tests (9 new); 59/59 groom ledger tests; agents-md-integrity passes. Parse exercised over '', ' ', '5.0', '-2', 'abc', '1e3', 'inf', 'nan', '01', ' 2 '. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 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 (8)
✨ 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 | 2 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…pin-comment rewrite (BE-4346) Addresses the cursor-review panel on #77. - bump-groom-callers.yml checked out with the mutable tag `actions/checkout@v6` while holding an org-scoped bot token. AGENTS.md mandates a full-SHA pin for every third-party action; pinned to the same SHA groom.yml already carries. - The minted token requested no permissions, so it carried everything Cloud Code Bot holds on every repo it is installed on. Narrowed to the trio this bumper actually uses — contents (Git Data commit), pull-requests (open/update the bump PR), issues (`gh pr create --label`) — matching groom.yml's own PR job. `owner:` with no `repositories:` stays: the caller list is a runtime variable, and naming those repos here would leak private names into a public file. - The `# main @ <short>` comment rewrite was unanchored at its right edge, so `[0-9a-f]{7,12}` would match the first 12 characters of a longer hex run. On a deliberate `# main @ <40hex>` comment — which rule 1 has just rewritten to NEW_SHA — it swapped those 12 for the 7-char SHORT and stranded the other 28, mangling the exact full-form comment the {7,12} bound was there to protect. Split into two rules requiring a non-hex character or EOL after the run, so the match is a whole token; a 13+ hex run now matches neither and is left intact. Portable ERE rather than a `\b`/`[[:>:]]` assertion, which spells differently in GNU and BSD sed. 99/99 bump-callers functional tests (2 new; the full-form case fails against the old rewrite), shellcheck -x clean, actionlint clean, 59/59 groom ledger tests, agents-md-integrity passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 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.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR: |
…o (BE-4346) The 40-hex pin rewrite fires on `/github-workflows|workflows_ref/`, but the two `# main @ <short>` comment rewrites were anchored to `/github-workflows/` alone. A groom caller that annotates its `workflows_ref:` pin — the second of the two pins groom callers carry — got that line's SHA bumped while the comment kept naming the old commit: the stale "confident lie" pin comment these rules exist to kill, reintroduced on the pin the groom fleet was built to keep in lock-step. Widen the comment anchors to match rule 1's exactly, and cover both directions in the groom fixture: the `workflows_ref:` line now carries its own `# main @` note (must move), plus an unanchored note on a line naming neither pin context (must NOT move, proving the anchor still bounds the rewrite). The first check fails against the pre-fix script. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two conflicts, one textual and one semantic. Both sides' intents preserved. TEXTUAL — .github/bump-callers/bump-callers.sh, the pin-rewrite block. Main's BE-4523 (#75) added the multi-reusable attribution guard (GW_USES / SHA_ADDR tightening / the `main (<short>)` marker refresh); this branch added the groom callers' `# main @ <short>` rewrite. Resolved by keeping main's structure whole and placing the `# main @` rules in the SAME sed as the 40-hex rule, sharing `${SHA_ADDR}` verbatim. That placement — rather than folding them under the `GW_ONLY_OURS` guard — is what preserves both intents at once. `SHA_ADDR` is exactly the set of lines whose pin the 40-hex rule rewrites, and the `# 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 is stamped with our SHA. The `main (<short>)` form cannot use the same trick — it also appears in prose ABOVE the `uses:` line, naming no pin context — which is why it keeps the caller-level guard. Gating `# main @` on that guard instead would have manufactured the exact stale comment BE-4346 removed: a `workflows_ref:` pin is bumped even in a multi-reusable file (that context stays broad by main's own reasoning), so its annotation must move with it. Three assertions added for the merged interaction, which neither branch's suite covered. Mutation-checked both ways: dropping the `# main @` rules fails the groom fixture; widening them back to the pre-BE-4523 broad address fails the new sibling assertion. SEMANTIC — .github/workflows/bump-groom-callers.yml. No textual conflict (a new file against modified siblings), but main's BE-4058 (#62) hardened all four existing bump entrypoints while this branch adds a fifth written from the pre-hardening template, so the merged result shipped one unguarded fleet. Brought to parity: the main-only ref guard, the stale re-run tip check, the decommission guard (checking `.github/groom/` as well as `groom.yml`, since a groom caller pins both), and the SHA-pinned checkout the sweep moved to v6.0.3. The ref guard here is main's accidental-stale-dispatch guard, not a security control — the workflow_dispatch token-escalation path stays open and stays tracked as BE-4661, which needs the server-side environment gate. 123 bump-callers assertions pass, shellcheck clean, groom/agents-md/cursor-review suites green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merged Textual — Resolved by keeping main's structure whole and putting the Gating Three assertions added for the merged interaction, which neither branch's suite covered (sibling's Semantic — Worth naming, since it touches an earlier reply: the 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. |
PR #77 (the groom-max-prs-reusable branch) squash-merged to main while this branch was mid-review, retargeting this PR from that branch to main and reproducing the squash double-diff: main's squashed content in bump-callers.sh/README.md is identical to what this branch already reconciled against pre-squash, so the already-reconciled side wins.
Resolves the single README.md conflict: #77 (BE-4346) appended the `max_prs` typed-`string` sentence to the same groom table row this branch rewrote for the interval gate. Keeps both — this branch's cadence-knob row plus main's `max_prs` string-typing note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Comfy-Org/cloud#5572 added a
max_prsworkflow_dispatchinput to ONE groom caller. It cost ~40 lines of expression gymnastics to do it:Every line of that exists because the reusable declared
max_prsastype: number. Aworkflow_dispatchinput is always a string; GitHub rejects a string expression assigned to atype: numberreusable input at startup; so the caller has to cast withfromJSON()— andfromJSON()on a non-numeric string throws during expression evaluation, killing the run before any job starts. Hence the mirroredcontains()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_prsis nowtype: string, and the reusable does the parse. The caller collapses to:No
fromJSON(), no allowlist mirror, no startup-throw class of failure.Verified the type asymmetry is real rather than assumed, with
actionlinton a scratch pair of workflows:type: numberinputstring value cannot be assigned(this is #5572's whole problem)max_prs: 1→type: stringinputThat 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:''/ whitespace — what a SCHEDULE yields, carrying no inputs5,5.0,1e3,2)>= 0abc,inf,nan)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: 1would 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'soptions: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 editoptions:. The real backstops are structural and unchanged: builder PRs are review-gated and never auto-merged, the finder emits ~6-12 findings a run, andpr_size_limitbails 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 theworkflows_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.shalready rewrote both pins (confirmed by dry-running itssedagainst 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 thegithub-workflowsline 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), noWIRE_BOT_SCRIPT(bot-identity injection is cursor-review-only; a groom caller must setbot_app_iditself).test_bump_callers.sh— a groom case covering both pins moving together, the comment rewrite, an untouchedactions/checkoutpin, and an intactmax_prsforward expression.Verification
actionlint— clean across all workflows.shellcheck -x— clean on the script + tests.agents-md-integritypasses (129 lines, under the 150 aim).'',' ','5.0','-2','0','abc','1e3','inf','-inf','nan','01',' 2 ','1.9'.seddry-run against the two real callers: both 40-hex pins rewrite,actions/checkoutuntouched.Follow-ups (not in this PR)
GROOM_CALLERSwith the two existing callers (a variable edit, no commit).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