[PR #77] [MERGED] feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) #86

New issue

Closed

opened 2026-10-26 22:36:05 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:05 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/77
Author: @mattmillerai
Created: 7/26/2026
Status: ✅ Merged
Merged: 7/26/2026
Merged by: @mattmillerai

Base: main ← Head: matt/groom-max-prs-reusable

📝 Commits (4)

62fa54c feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346)
ac575f8 fix(bump-callers): SHA-pin checkout, scope the bot token, anchor the pin-comment rewrite (BE-4346)
46abc9f fix(bump-callers): anchor the pin-comment rewrite to workflows_ref too (BE-4346)
84f5a32 Merge origin/main into matt/groom-max-prs-reusable (BE-4346)

📊 Changes

8 files changed (+364 additions, -26 deletions)

View changed files

📝 .github/bump-callers/README.md (+11 -0)
📝 .github/bump-callers/bump-callers.sh (+43 -8)
📝 .github/bump-callers/tests/test_bump_callers.sh (+77 -1)
➕ .github/workflows/bump-groom-callers.yml (+145 -0)
📝 .github/workflows/groom.yml (+76 -8)
📝 .github/workflows/test-bump-callers.yml (+7 -6)
📝 AGENTS.md (+4 -2)
📝 README.md (+1 -1)

📄 Description

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)

Seed GROOM_CALLERS with the two existing callers (a variable edit, no commit).
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

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/77 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/26/2026 **Status:** ✅ Merged **Merged:** 7/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/groom-max-prs-reusable` --- ### 📝 Commits (4) - [`62fa54c`](https://github.com/Comfy-Org/github-workflows/commit/62fa54cf9c4546690f3dedc852d159227c56a6bf) feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) - [`ac575f8`](https://github.com/Comfy-Org/github-workflows/commit/ac575f8c59bd10301b3cec0d12694e91b2d52d26) fix(bump-callers): SHA-pin checkout, scope the bot token, anchor the pin-comment rewrite (BE-4346) - [`46abc9f`](https://github.com/Comfy-Org/github-workflows/commit/46abc9f61f54c979d63cdc34a7c6328dc0ab83bf) fix(bump-callers): anchor the pin-comment rewrite to workflows_ref too (BE-4346) - [`84f5a32`](https://github.com/Comfy-Org/github-workflows/commit/84f5a320d5b20929a0f5494bb833619260bd7591) Merge origin/main into matt/groom-max-prs-reusable (BE-4346) ### 📊 Changes **8 files changed** (+364 additions, -26 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+11 -0) 📝 `.github/bump-callers/bump-callers.sh` (+43 -8) 📝 `.github/bump-callers/tests/test_bump_callers.sh` (+77 -1) ➕ `.github/workflows/bump-groom-callers.yml` (+145 -0) 📝 `.github/workflows/groom.yml` (+76 -8) 📝 `.github/workflows/test-bump-callers.yml` (+7 -6) 📝 `AGENTS.md` (+4 -2) 📝 `README.md` (+1 -1) </details> ### 📄 Description [Comfy-Org/cloud#5572](https://github.com/Comfy-Org/cloud/pull/5572) added a `max_prs` `workflow_dispatch` input to ONE groom caller. It cost ~40 lines of expression gymnastics to do it: ```yaml 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: ```yaml 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](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:05 +00:00