feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346)#77
Open
mattmillerai wants to merge 3 commits into
Open
feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346)#77mattmillerai wants to merge 3 commits into
mattmillerai wants to merge 3 commits into
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: 53 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.
Contributor
Author
|
🤖 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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