fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662) - #79
Open
mattmillerai wants to merge 8 commits into
Open
fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662)#79mattmillerai wants to merge 8 commits into
mattmillerai wants to merge 8 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>
…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)
…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>
…oth pins moved (BE-4662)
The caller pin rewrite keyed on "any 40-hex on a line that mentions
github-workflows or workflows_ref" and never checked its own outcome, which
failed silently in both directions:
* Under-rewrite. A caller pins this repo TWICE — the `uses:` sha and the
`workflows_ref:` input that loads the briefs/prompts/scripts at run time. A
`workflows_ref` pinned to a tag (`v1`) or a short sha is not 40 hex, so it was
left behind while `uses:` moved. The content-equality check still saw a
difference, so the file was staged and a green-looking bump PR opened on a
caller now running one version's workflow against another version's assets —
the exact split this fleet exists to prevent.
* Over-rewrite. An unrelated 40-hex value sharing such a line was clobbered.
Anchor the substitution to the pin TOKEN instead — `Comfy-Org/github-workflows…@`
and the `workflows_ref:` key — and take whatever ref follows by position, so any
literal shape (full sha, short sha, tag) moves and a co-located SHA is
unreachable. The `workflows_ref` rule is `^`-anchored to a block-mapping key so
prose that merely mentions the input is left alone.
Precision cuts both ways, so add the guard it needs: before a rewritten file can
be staged, re-read it with a deliberately broader reader and assert every
github-workflows pin now equals the new SHA. If one does not (today: a
`workflows_ref` fed by a `${{ … }}` expression, which is never rewritten), warn
with the file + the stale value and fail that repo — the same posture a transient
fetch error already takes, because a partial bump is worse than no bump (BE-3896).
The full-sha `# main @ <40hex>` comment form gets its own rule now that rule 1 no
longer sprays every 40-hex on the line; it used to be corrected as collateral.
Tests: +31 cases (130 total) — tag-pinned and short-sha `workflows_ref`, an
unrelated 40-hex sharing a github-workflows line, the assertion firing (no
commit, no PR, non-zero for that repo), and a prose comment not tripping it.
shellcheck -x clean.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 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 (4)
✨ 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 9 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…case, key boundary (BE-4662) Review follow-ups on the pin-token rewrite + assertion. All five are edges of the same idea: the token has to end where the ref begins, and the assertion has to read exactly what the rewrite writes. * the `uses:` pattern now requires a DELIMITER after the repo name (`/` or `@`), so a sibling repo whose name merely starts the same (`Comfy-Org/github-workflows-tools/action@v1`) is no longer swallowed and repinned to this repo's SHA — and, because the assertion reuses the pattern, no longer read back as NEW_SHA and staged silently; * the owner/repo is matched case-INSENSITIVELY, because GitHub resolves `uses:` that way. A `comfy-org/…` caller previously had its (repo-agnostic) `workflows_ref` half bumped while `uses:` stayed stale, and the assertion — reading `uses:` with the same pattern — missed the stale half too: precisely the split half-bump this guard exists to prevent; * the assertion's `workflows_ref` reader gained a left boundary, so a longer key like `upstream_workflows_ref: v1` (which the rewrite correctly leaves alone) is no longer read as an un-bumped pin and no longer hard-fails a clean caller's bump on every run; * comments are dropped by YAML's own rule (a `#` preceded by whitespace) instead of at the first `#`, so a `#` INSIDE a ref cannot fake its way past: REF_RE half-moves `'feature#1'` to `'<NEW_SHA>#1'`, which now compares unequal and fails the repo rather than being read back as a clean NEW_SHA; * an empty pin (`workflows_ref: ""`) is named `(empty)` rather than filtered out as a blank — the rewrite cannot move it either, so dropping it let a silent half-bump through. The failure warning is also sanitized before it reaches this public repo's run logs (carriage return stripped, `::` neutralized) so a caller-supplied value cannot inject a workflow command, and the value/prose spacing is now explicit rather than relying on the here-string's trailing newline. Five regression cases added; each fails against the pre-fix script (16 checks).
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>
…rs-precise-pin Reconciles BE-4662's token-anchored pin rewrite with BE-4523's sibling-reusable address tightening: rule 1 (the `uses:` pin) is now address-restricted to OUR workflow file when a caller pins two github-workflows reusables, the `# main @` comment rules ride the same address, and the post-rewrite assertion scans only the same address-eligible lines so a deliberately untouched sibling pin can't false-fail the repo. Both test suites (BE-4662 precise-pin + BE-4523 sibling-marker) now live together and pass, 175/175.
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.
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
1 participant
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.
ELI-5
When one of this repo's reusable workflows changes, a bot opens a "bump the pin" PR in every repo that uses it. A repo pins us in two places: the
uses:line and aworkflows_ref:input that loads our prompts/briefs/scripts at run time. Both have to point at the same commit, or the run uses one version's workflow with another version's briefs.The bot used to find those pins by looking for "any 40-character hex string on a line that mentions github-workflows." That's the wrong thing to look for, and it never checked its own work. So if a repo pinned
workflows_ref: v1(a tag, not 40 hex), the bot bumped theuses:line, left the tag alone, and happily opened a green-looking PR that splits the caller. And if some unrelated 40-hex value happened to sit on a line that mentioned github-workflows, the bot overwrote it.Now the bot looks for the pin token —
Comfy-Org/github-workflows…@and theworkflows_ref:key — and takes whatever comes right after it, whatever shape it is. Then, before it stages anything, it re-reads the file and checks that every pin really did move. If one didn't, it says so out loud and fails that repo instead of opening a half-bumped PR.What changed
Comfy-Org/github-workflows[^@\s]*@<ref>andworkflows_ref: <ref>(optionally quoted). Whatever sits right after the token is the ref, so a full sha, a short sha, or a tag all move — and an unrelated SHA sharing the line is unreachable.workflows_refrule is^-anchored to a block-mapping key, so a prose comment mentioning the input (# workflows_ref: keep in sync with uses:) is not rewritten into a sentence with a SHA spliced through it.::warning::naming the file and the stale value and fails that repo. Same posture line 203 already takes for a transient fetch error — a partial bump is worse than no bump (BE-3896). Nothing is written to the caller repo before this point (all API writes are in Pass 2), so the failure is genuinely all-or-nothing.# main @ <40hex>pin comment. That comment used to be corrected as collateral of the old line-scoped 40-hex substitution. With the substitution now precise, it needs its own rule or it would be left naming the old commit — a regression against feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) #77's intent. Verified by deleting the rule and watching feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) #77'sgroomcase go red (2 failures)..github/bump-callers/README.mdgains a "How the pin rewrite is scoped" section;bump-groom-callers.yml's header no longer describes the old line-keying.Tests
.github/bump-callers/tests/test_bump_callers.sh— 99 → 130 cases, all green, plusshellcheck -xclean on the script and the suite. Five new cases:reftagworkflows_ref: v1moves in lock-step withuses:refshort'2222222'moves, and the closing quote survivescolocgithub-workflowsline and one on aworkflows_refline are both untouched; exactly one pin is rewrittenassertfireworkflows_ref: ${{ inputs.workflows_ref }}fails the repo — no blob, no commit, no PR,bump failed for 1 repo, and the warning names both the file and the stale valueprosecomment# workflows_ref: …prose note neither trips the assertion nor gets mangled, while the real pin below it still bumpsRan:
shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.shandbash .github/bump-callers/tests/test_bump_callers.sh.Judgment calls
main. feat(groom): own the max_prs dispatch override in the reusable + add the groom caller fleet (BE-4346) #77 is open and rewrites this exactsedblock (it added the# main @comment rules). Building onmainwould have conflicted and effectively reverted them. Banner at the top; base ismatt/groom-max-prs-reusable.uses:pin at a tag is now rewritten to the SHA too. The ticket specified shape-agnostic matching forworkflows_refonly, but applying it touses:as well is what makes the pair coherent — and it is required, because the assertion demands everyuses:pin equal the new SHA, so a@v1pin would otherwise fail the repo forever. It also matches the repo standard ("pin everything by full commit SHA"; bare@v1fails consumers'pinact/zizmor).workflows_refis a${{ … }}expression, or in flow style (with: {workflows_ref: v1}), will fail its repo on every run until a human fixes the config. That is deliberate — there is no literal ref for the bumper to move in those shapes, and rewriting them would break the caller. Comments are stripped before the assertion scans, so prose can never cause that failure.workflows_refusage in the repo: the${{ inputs.workflows_ref }}forms all live inside the reusable workflows themselves (never in a caller list), and every documented caller pattern (workflows_ref: <sha>,workflows_ref: main,workflows_ref: <same-sha>) is a literal the new rewrite moves. So no caller shape that works today is newly denied — the shapes that fail are the ones that previously produced a silently half-bumped PR.Risk
The change is confined to one script that backs all five bump fleets, so blast radius is "every fleet" — but every write to a caller repo happens in Pass 2, after the assertion, and the suite covers each fleet's fixture shape. The riskiest line is the
workflows_refsubstitution, which replaces whatever token follows the key; that is safe becauseworkflows_refis this repo's own input and its only meaning is "the github-workflows ref to load assets from."Closes BE-4662.