[PR #79] [MERGED] fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662) #89

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4662-bump-callers-precise-pin

📝 Commits (8)

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)
d4d807d fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662)
b929a00 fix(bump-callers): bound the pin token's edges — sibling repo, owner case, key boundary (BE-4662)
84f5a32 Merge origin/main into matt/groom-max-prs-reusable (BE-4346)
fc8e2b9 Merge origin/matt/groom-max-prs-reusable into matt/be-4662-bump-callers-precise-pin
a2bf511 Merge origin/main into matt/be-4662-bump-callers-precise-pin

📊 Changes

4 files changed (+517 additions, -57 deletions)

View changed files

📝 .github/bump-callers/README.md (+56 -5)
📝 .github/bump-callers/bump-callers.sh (+182 -49)
📝 .github/bump-callers/tests/test_bump_callers.sh (+273 -0)
📝 .github/workflows/bump-groom-callers.yml (+6 -3)

📄 Description

STACKED — merging lands on matt/groom-max-prs-reusable (owned by @mattmillerai, PR #77), NOT main. This PR's base is #77's branch because #77 rewrites the same sed block; basing on main would conflict with it and silently drop its # main @ pin-comment rules. GitHub retargets this PR to main once #77 merges. Review the net diff, not the merge button.

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 a workflows_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 the uses: 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 the workflows_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

The rewrite targets the pin token, not 40-hex-ness. Two patterns matched by position: Comfy-Org/github-workflows[^@\s]*@<ref> and workflows_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.
The workflows_ref rule 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.
A post-rewrite assertion gates staging. The file is re-read with a deliberately broader reader (any non-whitespace value sitting where a ref belongs, comments excluded); if anything but the new SHA is still there, it emits a ::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.
A dedicated rule for the full-sha # 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 #77's intent. Verified by deleting the rule and watching #77's groom case go red (2 failures).
Doc sync: .github/bump-callers/README.md gains 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, plus shellcheck -x clean on the script and the suite. Five new cases:

Case	Asserts
`reftag`	a `workflows_ref: v1` moves in lock-step with `uses:`
`refshort`	a quoted short sha `'2222222'` moves, and the closing quote survives
`coloc`	an unrelated 40-hex on a `github-workflows` line and one on a `workflows_ref` line are both untouched; exactly one pin is rewritten
`assertfire`	a `workflows_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 value
`prosecomment`	a `# workflows_ref: …` prose note neither trips the assertion nor gets mangled, while the real pin below it still bumps

Ran: shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh and bash .github/bump-callers/tests/test_bump_callers.sh.

Judgment calls

Stacked on #77 rather than main. #77 is open and rewrites this exact sed block (it added the # main @ comment rules). Building on main would have conflicted and effectively reverted them. Banner at the top; base is matt/groom-max-prs-reusable.
A uses: pin at a tag is now rewritten to the SHA too. The ticket specified shape-agnostic matching for workflows_ref only, but applying it to uses: as well is what makes the pair coherent — and it is required, because the assertion demands every uses: pin equal the new SHA, so a @v1 pin would otherwise fail the repo forever. It also matches the repo standard ("pin everything by full commit SHA"; bare @v1 fails consumers' pinact/zizmor).
The assertion is intentionally broader than the rewrite, and that asymmetry is the design. Anything the rewrite cannot move surfaces as a loud per-repo failure rather than a silent half-bump. The known consequence: a caller whose workflows_ref is 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.
Falsification of the new deny path. The only outcome this diff denies is "bump a caller whose pin the rewrite cannot move." I checked every workflows_ref usage 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_ref substitution, which replaces whatever token follows the key; that is safe because workflows_ref is this repo's own input and its only meaning is "the github-workflows ref to load assets from."

Closes BE-4662.

_{🔄 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/79 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/26/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4662-bump-callers-precise-pin` --- ### 📝 Commits (8) - [`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) - [`d4d807d`](https://github.com/Comfy-Org/github-workflows/commit/d4d807dfe8f46905ae4beb3ebe81a29283c69737) fix(bump-callers): target the github-workflows pin token and assert both pins moved (BE-4662) - [`b929a00`](https://github.com/Comfy-Org/github-workflows/commit/b929a0086fead861b4169784fde83877a2444ae5) fix(bump-callers): bound the pin token's edges — sibling repo, owner case, key boundary (BE-4662) - [`84f5a32`](https://github.com/Comfy-Org/github-workflows/commit/84f5a320d5b20929a0f5494bb833619260bd7591) Merge origin/main into matt/groom-max-prs-reusable (BE-4346) - [`fc8e2b9`](https://github.com/Comfy-Org/github-workflows/commit/fc8e2b974b439ecd2f5b0e412b1306ea2e61ae0b) Merge origin/matt/groom-max-prs-reusable into matt/be-4662-bump-callers-precise-pin - [`a2bf511`](https://github.com/Comfy-Org/github-workflows/commit/a2bf511059345a997d8fc3f0f7bfd75b02fd893b) Merge origin/main into matt/be-4662-bump-callers-precise-pin ### 📊 Changes **4 files changed** (+517 additions, -57 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+56 -5) 📝 `.github/bump-callers/bump-callers.sh` (+182 -49) 📝 `.github/bump-callers/tests/test_bump_callers.sh` (+273 -0) 📝 `.github/workflows/bump-groom-callers.yml` (+6 -3) </details> ### 📄 Description > **STACKED — merging lands on `matt/groom-max-prs-reusable` (owned by @mattmillerai, PR #77), NOT `main`.** This PR's base is #77's branch because #77 rewrites the same `sed` block; basing on `main` would conflict with it and silently drop its `# main @` pin-comment rules. GitHub retargets this PR to `main` once #77 merges. Review the net diff, not the merge button. ## 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 a `workflows_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 the `uses:` 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 the `workflows_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 - **The rewrite targets the pin token, not 40-hex-ness.** Two patterns matched by *position*: `Comfy-Org/github-workflows[^@\s]*@<ref>` and `workflows_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. - **The `workflows_ref` rule 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. - **A post-rewrite assertion gates staging.** The file is re-read with a deliberately *broader* reader (any non-whitespace value sitting where a ref belongs, comments excluded); if anything but the new SHA is still there, it emits a `::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. - **A dedicated rule for the full-sha `# 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 #77's intent. Verified by deleting the rule and watching #77's `groom` case go red (2 failures). - Doc sync: `.github/bump-callers/README.md` gains 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**, plus `shellcheck -x` clean on the script and the suite. Five new cases: | Case | Asserts | |---|---| | `reftag` | a `workflows_ref: v1` moves in lock-step with `uses:` | | `refshort` | a quoted short sha `'2222222'` moves, and the closing quote survives | | `coloc` | an unrelated 40-hex on a `github-workflows` line and one on a `workflows_ref` line are both untouched; exactly one pin is rewritten | | `assertfire` | a `workflows_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 value | | `prosecomment` | a `# workflows_ref: …` prose note neither trips the assertion nor gets mangled, while the real pin below it still bumps | Ran: `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh` and `bash .github/bump-callers/tests/test_bump_callers.sh`. ## Judgment calls 1. **Stacked on #77 rather than `main`.** #77 is open and rewrites this exact `sed` block (it added the `# main @` comment rules). Building on `main` would have conflicted and effectively reverted them. Banner at the top; base is `matt/groom-max-prs-reusable`. 2. **A `uses:` pin at a tag is now rewritten to the SHA too.** The ticket specified shape-agnostic matching for `workflows_ref` only, but applying it to `uses:` as well is what makes the pair coherent — and it is required, because the assertion demands *every* `uses:` pin equal the new SHA, so a `@v1` pin would otherwise fail the repo forever. It also matches the repo standard ("pin everything by full commit SHA"; bare `@v1` fails consumers' `pinact`/`zizmor`). 3. **The assertion is intentionally broader than the rewrite, and that asymmetry is the design.** Anything the rewrite cannot move surfaces as a loud per-repo failure rather than a silent half-bump. The known consequence: a caller whose `workflows_ref` is 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. 4. **Falsification of the new deny path.** The only outcome this diff denies is "bump a caller whose pin the rewrite cannot move." I checked every `workflows_ref` usage 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_ref` substitution, which replaces whatever token follows the key; that is safe because `workflows_ref` is this repo's own input and its only meaning is "the github-workflows ref to load assets from." Closes BE-4662. --- <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:06 +00:00