[PR #75] [MERGED] fix(bump-callers): refresh the already-converted `main (<short>)` pin marker (BE-4523) #84

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4523-bump-callers-marker

📝 Commits (2)

d38b4f2 fix(bump-callers): refresh the already-converted main (<short>) pin marker (BE-4523)
858c077 fix(bump-callers): scope the SHA bump to our own reusable in a multi-reusable caller (BE-4523)

📊 Changes

2 files changed (+171 additions, -9 deletions)

View changed files

📝 .github/bump-callers/bump-callers.sh (+76 -9)
📝 .github/bump-callers/tests/test_bump_callers.sh (+95 -0)

📄 Description

ELI-5

Each caller repo's workflow file pins our shared reusable workflow twice: once as the machine-readable 40-character commit SHA, and once as a human-readable comment that says which commit that is — # github-workflows main (dc65b8b). The auto-bumper knew how to update the SHA, and knew how to convert the old comment style (# github-workflows#27) into the new one — but it never learned to update the new one again. So the moment a caller had been converted, its comment froze forever while the real pin kept moving, and the comment started lying about which commit the file uses. This teaches the bumper to keep that comment in step with the pin.

What changed

.github/bump-callers/bump-callers.sh gains a third rewrite rule alongside the existing two, plus a guard:

Refresh the converted marker. github-workflows main (<hex>) → github-workflows main (${SHORT}), {7,40} so both the short and full-SHA spellings are covered.
Not anchored to # . Callers carry the same annotation in two spellings: the trailing comment on the uses: line, and a prose comment above it (# Pinned to github-workflows main (dc65b8b). The bump-… workflow …). The ticket's proposed pattern was anchored to # github-workflows main (…), which matches only the trailing form; dropping the anchor catches both, which is what the hand-fix on the symptom PR actually had to do.
Attribution guard. A marker names a SHA but not which reusable it annotates, so in a file pinning several github-workflows reusables there is no honest way to tell whose marker is whose. The rule only fires when every github-workflows reusable that file uses: is the one this fleet is bumping; otherwise it leaves every marker as found and emits a ::warning::. That is deliberately more conservative than the ticket's "rewrite the one adjacent to $WORKFLOW_FILE" — see the judgment call below.
The guard block is entered only when the file already carries a main (<hex>) marker, so agents-md-style # v1 callers and marker-less callers see no new warning and no behavior change at all.

The legacy # github-workflows#[0-9]+ rule and the 40-hex substitution are untouched (their existing test coverage still passes unchanged).

.github/bump-callers/tests/test_bump_callers.sh gains the two cases the ticket asked for. Suite is 100 passed / 0 failed; shellcheck -x clean on both files. Verified the tests are load-bearing: reverting only bump-callers.sh turns them red (96 passed / 4 failed), and all four failures are the new assertions.

Evidence it fixes the reported symptom

Ran the exact rewrite pipeline over Comfy-Org/Comfy-iOS's real .github/workflows/ci-cursor-review.yml at origin/main with NEW_SHA=377f7e18605615508898095cfa9ac65173aae2f9. Output is byte-for-byte the diff that PR had to be hand-fixed into — all four lines move together (prose marker, uses: SHA, uses: trailing marker, workflows_ref), where the bumper previously produced only the last three.

Falsification of the guard's premise

The guard is only safe if no real caller pins more than one github-workflows reusable from one file — otherwise it would silently suppress marker updates fleet-wide, i.e. quietly restore today's bug. I did not assume this; I checked it. Fetching all four caller variables and resolving every caller file live: 22 of 23 caller entries resolve to exactly one github-workflows reusable, matching their own fleet's WORKFLOW_FILE — so the guard passes for every live caller and never blocks a bump. 18 of those files carry at least one main (<short>) marker that is stale today (two carry the Comfy-iOS-style prose + trailing pair). One caller file was unreadable with this token (permissions), so it is unverified rather than confirmed; worst case there is the conservative branch — markers left alone plus a warning, never a wrong rewrite. Caller repo names are deliberately omitted here: this repo is public and the caller list is masked out of its run logs.

Judgment calls

Whole-file refusal instead of uses:-proximity matching. The ticket suggested scoping the marker rewrite to the marker on/adjacent to the uses: line for $WORKFLOW_FILE. I did not do that, for two reasons. First, it does not actually work for the reported symptom: the stale prose marker in Comfy-iOS sits four lines above the uses: line it annotates, so any proximity rule tight enough to be safe is too tight to fix the bug that was filed. Second, proximity would produce a worse multi-reusable outcome than refusal — the pre-existing 40-hex rule would still bump the sibling's SHA while proximity kept its marker at the old value, which is the exact "marker disagrees with the pin" defect this ticket exists to kill, just relocated. Refusing to rewrite anything in an ambiguous file and saying so in the log is the honest failure mode.
Pre-existing defect left alone (not in scope). In that same hypothetical multi-reusable file, the existing /github-workflows|workflows_ref/ s/[0-9a-f]{40}/…/g rule already clobbers a sibling reusable's pin to this fleet's SHA. That is a separate, older bug; no caller triggers it today (see falsification above) and narrowing that rule would change long-standing bump behavior, so I left it and am flagging it here rather than folding it in.
One-time comment-only bump PRs. Because the already-pinned check compares rewritten-vs-original content, a caller that is already at the target SHA but carries a frozen marker now differs, so it will be staged and get a bump PR whose diff is only the comment. That is the intended repair (it is how the 18 stale markers get corrected), it happens once per caller, and it reuses the existing stable bump branch rather than opening anything new.

Unmet criteria

None. Both requested test cases are present, the legacy rule is preserved, and the scope concern is addressed (differently, and explained above).

Test plan

bash .github/bump-callers/tests/test_bump_callers.sh → 100 passed, 0 failed
shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh → clean
Both run automatically by .github/workflows/test-bump-callers.yml on this PR (its path filter covers .github/bump-callers/**).

_{🔄 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/75 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/25/2026 **Status:** ✅ Merged **Merged:** 7/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4523-bump-callers-marker` --- ### 📝 Commits (2) - [`d38b4f2`](https://github.com/Comfy-Org/github-workflows/commit/d38b4f22588aca22025b33feb04cfca517307328) fix(bump-callers): refresh the already-converted `main (<short>)` pin marker (BE-4523) - [`858c077`](https://github.com/Comfy-Org/github-workflows/commit/858c0775c222e3d1756d01bf843559caff3924f3) fix(bump-callers): scope the SHA bump to our own reusable in a multi-reusable caller (BE-4523) ### 📊 Changes **2 files changed** (+171 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/bump-callers.sh` (+76 -9) 📝 `.github/bump-callers/tests/test_bump_callers.sh` (+95 -0) </details> ### 📄 Description ## ELI-5 Each caller repo's workflow file pins our shared reusable workflow twice: once as the machine-readable 40-character commit SHA, and once as a human-readable comment that says which commit that is — `# github-workflows main (dc65b8b)`. The auto-bumper knew how to update the SHA, and knew how to convert the *old* comment style (`# github-workflows#27`) into the new one — but it never learned to update the new one again. So the moment a caller had been converted, its comment froze forever while the real pin kept moving, and the comment started lying about which commit the file uses. This teaches the bumper to keep that comment in step with the pin. ## What changed `.github/bump-callers/bump-callers.sh` gains a third rewrite rule alongside the existing two, plus a guard: - **Refresh the converted marker.** `github-workflows main (<hex>)` → `github-workflows main (${SHORT})`, `{7,40}` so both the short and full-SHA spellings are covered. - **Not anchored to `# `.** Callers carry the same annotation in two spellings: the trailing comment on the `uses:` line, and a prose comment above it (`# Pinned to github-workflows main (dc65b8b). The bump-… workflow …`). The ticket's proposed pattern was anchored to `# github-workflows main (…)`, which matches only the trailing form; dropping the anchor catches both, which is what the hand-fix on the symptom PR actually had to do. - **Attribution guard.** A marker names a SHA but not *which* reusable it annotates, so in a file pinning several github-workflows reusables there is no honest way to tell whose marker is whose. The rule only fires when every github-workflows reusable that file `uses:` is the one this fleet is bumping; otherwise it leaves every marker as found and emits a `::warning::`. That is deliberately more conservative than the ticket's "rewrite the one adjacent to `$WORKFLOW_FILE`" — see the judgment call below. - The guard block is entered only when the file already carries a `main (<hex>)` marker, so agents-md-style `# v1` callers and marker-less callers see no new warning and no behavior change at all. The legacy `# github-workflows#[0-9]+` rule and the 40-hex substitution are **untouched** (their existing test coverage still passes unchanged). `.github/bump-callers/tests/test_bump_callers.sh` gains the two cases the ticket asked for. Suite is 100 passed / 0 failed; `shellcheck -x` clean on both files. Verified the tests are load-bearing: reverting only `bump-callers.sh` turns them red (96 passed / 4 failed), and all four failures are the new assertions. ## Evidence it fixes the reported symptom Ran the exact rewrite pipeline over `Comfy-Org/Comfy-iOS`'s real `.github/workflows/ci-cursor-review.yml` at `origin/main` with `NEW_SHA=377f7e18605615508898095cfa9ac65173aae2f9`. Output is byte-for-byte the diff that PR had to be hand-fixed into — all four lines move together (prose marker, `uses:` SHA, `uses:` trailing marker, `workflows_ref`), where the bumper previously produced only the last three. ## Falsification of the guard's premise The guard is only safe if no *real* caller pins more than one github-workflows reusable from one file — otherwise it would silently suppress marker updates fleet-wide, i.e. quietly restore today's bug. I did not assume this; I checked it. Fetching all four caller variables and resolving every caller file live: **22 of 23 caller entries resolve to exactly one github-workflows reusable, matching their own fleet's `WORKFLOW_FILE`** — so the guard passes for every live caller and never blocks a bump. 18 of those files carry at least one `main (<short>)` marker that is stale today (two carry the Comfy-iOS-style prose + trailing pair). One caller file was unreadable with this token (permissions), so it is unverified rather than confirmed; worst case there is the conservative branch — markers left alone plus a warning, never a wrong rewrite. Caller repo names are deliberately omitted here: this repo is public and the caller list is masked out of its run logs. ## Judgment calls - **Whole-file refusal instead of `uses:`-proximity matching.** The ticket suggested scoping the marker rewrite to the marker on/adjacent to the `uses:` line for `$WORKFLOW_FILE`. I did not do that, for two reasons. First, it does not actually work for the reported symptom: the stale prose marker in Comfy-iOS sits **four lines above** the `uses:` line it annotates, so any proximity rule tight enough to be safe is too tight to fix the bug that was filed. Second, proximity would produce a *worse* multi-reusable outcome than refusal — the pre-existing 40-hex rule would still bump the sibling's SHA while proximity kept its marker at the old value, which is the exact "marker disagrees with the pin" defect this ticket exists to kill, just relocated. Refusing to rewrite anything in an ambiguous file and saying so in the log is the honest failure mode. - **Pre-existing defect left alone (not in scope).** In that same hypothetical multi-reusable file, the existing `/github-workflows|workflows_ref/ s/[0-9a-f]{40}/…/g` rule already clobbers a *sibling* reusable's pin to this fleet's SHA. That is a separate, older bug; no caller triggers it today (see falsification above) and narrowing that rule would change long-standing bump behavior, so I left it and am flagging it here rather than folding it in. - **One-time comment-only bump PRs.** Because the already-pinned check compares rewritten-vs-original content, a caller that is already at the target SHA but carries a frozen marker now differs, so it will be staged and get a bump PR whose diff is only the comment. That is the intended repair (it is how the 18 stale markers get corrected), it happens once per caller, and it reuses the existing stable bump branch rather than opening anything new. ## Unmet criteria None. Both requested test cases are present, the legacy rule is preserved, and the scope concern is addressed (differently, and explained above). ## Test plan - `bash .github/bump-callers/tests/test_bump_callers.sh` → 100 passed, 0 failed - `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh` → clean - Both run automatically by `.github/workflows/test-bump-callers.yml` on this PR (its path filter covers `.github/bump-callers/**`). --- <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:03 +00:00