fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662)#79
Open
mattmillerai wants to merge 2 commits into
Conversation
…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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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).
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.