[PR #134] [MERGED] fix(bump-callers): validate roster entries and fail on un-bumpable callers (BE-6471) #145

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/134
Author: @mattmillerai
Created: 8/5/2026
Status: ✅ Merged
Merged: 8/12/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6471-roster-validation

📝 Commits (6)

ef8aa14 fix(bump-callers): validate roster entries and fail on un-bumpable callers (BE-6471)
239a32a fix(bump-callers): close the review panel's gaps in the roster validation (BE-6471)
331c1d4 fix(bump-callers): close round-2 review findings on the roster gate (BE-6471)
76cc890 fix(bump-callers): close round-3 review findings on the roster gate (BE-6471)
65eb430 Merge origin/main into matt/be-6471-roster-validation
60467dc fix(bump-callers): reconcile merge — drop stale BE-6015 tests, fix doc drift (BE-6471)

📊 Changes

4 files changed (+1137 additions, -157 deletions)

View changed files

📝 .github/bump-callers/README.md (+83 -0)
📝 .github/bump-callers/bump-callers.sh (+430 -99)
📝 .github/bump-callers/tests/test_bump_callers.sh (+617 -51)
📝 AGENTS.md (+7 -7)

📄 Description

ELI-5

A config list tells our robot which repos to keep up to date. The robot trusted that list completely: it never checked that an entry named a real Comfy-Org repo or a real workflow file, and when an entry pointed somewhere with nothing to update, the robot cheerfully logged "already up to date" and went green — forever. This makes the robot check the list before it touches anything, and say "I can't move this one" instead of pretending it's fine.

What this does

Three changes, all in bump-callers.sh plus its tests. No entrypoint workflow is touched (one bumper, not several).

1. Roster entry validation, before any API call. Every field reaches a privileged sink — repo is interpolated into each gh api repos/<repo>/… write and gh pr create --repo, file becomes the path committed into the caller's tree, label goes to gh pr create --label — all under an org-wide app token (owner: Comfy-Org, contents + pull-requests + issues write, no repositories: narrowing). The roster is an Actions variable, editable outside code review, so roster-edit access currently converts into bot-authored commits, branch force-moves and labelled PRs in any org repo the app is installed on. repo must now match ^Comfy-Org/[A-Za-z0-9._-]+$ (and not be a bare dot segment), file must match ^\.github/workflows/[A-Za-z0-9._-]+\.ya?ml$ — the class excludes /, so ../ traversal cannot appear — and label must be a string with no | and no newline. One jq pass emits every violation, so an entry breaking two rules reports both.

The label rule closes a real delimiter bug: entries are carried internally as repo|file|label|wire_bot tuples and read back with cut -d'|' -f3, so a pipe-bearing label truncated at the pipe and bled its tail into wire_bot — silently flagging for identity wiring an entry that never asked for it. The dedup comment that assumed pipe-bearing labels were representable is corrected in the same commit.

Errors name the entry's zero-based index and the violated rule, never the value. Masking has not been applied at that point in the script, and this repo's run logs are public.

2. An un-bumpable caller file fails the run. A file that carries no pin this fleet can address rewrites to itself, so the content-equality check reported it as already at <short> — skipping and the job exited green — indefinitely, for a roster entry that is simply wrong. Each file is now checked before the rewrite, using the same regexes the rewrite uses so the two cannot drift apart. Two shapes trip it: no Comfy-Org/github-workflows pin at all, or (via the sibling-tightened address) a file whose only github-workflows uses: belongs to a different fleet's reusable.

3. End-of-run accounting. Offenders are warned per file and tallied into one ::error:: after the whole fleet has been processed, then the job exits 1. Failing at the end rather than at the offending file preserves the fan-out — one bad entry never blocks another repo's bump, matching how FAILED already behaves. Only the count is printed; the per-file warnings carry the (masked) repo and the path.

Judgment calls — please read these two

The ticket's rule for part 2 was written against a stale base, and I did not implement it as literally specified. BE-6467 verified against main @ e4a8f7c (Aug 3, 18:56) and specified that a recognisable pin must be a 40-hex SHA. #79 / BE-4662 merged the next morning (Aug 4, 10:55) and re-anchored the rewrite to the pin token rather than to 40-hex-ness — so a caller on @v1 is now self-healed to the new SHA. Requiring a full SHA here would therefore hard-fail exactly the floating-tag callers the fleet exists to drag back onto immutable pins, which is a regression against #79's intent. The check requires an addressable pin of any literal ref shape instead, and there is a test asserting the @v1 self-heal stays green. The ticket's test case 1 (floating tag → warning + exit 1) is consequently not implemented as written; the genuinely-undetectable case it was reaching for — a roster entry pointing at a file with no pin of ours — is covered instead.

Overlap with open PR #107 (BE-6015). #107 implements the same "un-bumpable caller" idea, from the pre-#79 base, and is currently CONFLICTING with main. Its checks require a 40-hex pin, which is the rule #79 superseded. I have not touched it. Recommend reconciling: either close it in favour of this, or rebase it and keep only the parts this does not cover (its # main @ / placeholder-specific test fixtures are still useful). Flagging rather than deciding, since it is not mine to close.

Falsification — I went and looked, I did not just test my own premise

This diff's user-facing outcome denies a capability (it turns previously-green runs red), so internal tests only assert my own premise. I checked it against production instead:

Roster validation: read all eight fleet variables. Six are seeded (CURSOR_REVIEW_CALLERS 19, AGENTS_MD_CALLERS 17, GROOM_CALLERS 9, AUTO_LABEL_CALLERS 5, ASSIGN_REVIEWERS_CALLERS 3, PR_SIZE_CALLERS 1 = 54 entries); PR_RISK_CALLERS and DETECT_UNREVIEWED_MERGE_CALLERS do not exist yet, as documented. Ran the exact jq against all 54: zero violations. No live fleet breaks.
Pin check: fetched every one of those 54 caller files and ran the exact recognisability logic. 51 carry a recognisable pin. The other 3 return HTTP 404 — the script's existing per-file 404 path warns and continues before the new check, so they never reach it. Net: zero real callers would trip the new hard-fail today.
Side observation, no action taken here: those 3 404s are the "roster entry whose caller file does not exist" direction AGENTS.md warns about. Worth a separate roster clean-up; repos deliberately unnamed.

Testing

shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh — clean.
bash .github/bump-callers/tests/test_bump_callers.sh — 235 passed, 0 failed (was 175; all 175 pre-existing assertions pass unchanged, including the ALLOW_EMPTY seeded-empty no-op and the malformed-variable hard-fail).
New coverage: repo outside Comfy-Org; bare dot-segment repo name (and Comfy-Org/.github still valid); non-workflow file; .github/workflows/../evil.yml traversal; pipe-bearing label; an entry breaking two rules reporting both; every legitimate roster shape still validating and bumping; each rejection asserting the offending value is never echoed and that it bailed before the mask loop; @v1 self-heal stays green; a file with no pin of ours failing with the aggregate tally; a sibling-fleet-only file; and one un-bumpable entry not blocking a healthy repo's bump in the same run (the gh stub gained a small per-repo fixture override for that last one).

Round 2 — the review panel's ten findings (`331c1d4`)

Two High, one Medium, four Low, two Nits, plus the README follow-on. All ten
addressed; each thread carries the detail.

The two High ones were live defects, not hardening:

Case-variant roster spellings were two bumps of one repo. Accepting
comfy-org/x case-insensitively is correct, but nothing folded the spelling,
so the byte-exact repo grouping produced two REPOS entries and bump_repo
ran twice — the second force-PATCHing the shared bump branch off the first's
commit. Green run, half a bump. The owner is canonicalized on the way in and
repos are grouped (and their entries collected) case-insensitively.
A bare workflows_ref: could vouch for a file. The uses:-pin
requirement was gated on GW_HAS_SIBLING, which derives from a
case-sensitive grep — so a caller spelled GitHub-Workflows, or one whose
uses: had been removed, left the flag at 0, took the relaxed address, and
had this fleet's SHA stamped onto its assets ref by the unaddressed rule 2,
invisibly to the address-filtered assertion. The requirement is unconditional
now, which also makes the README bullet true as written.

The rest: a rejected path's label no longer folds onto the healthy file's PR;
the label rule bars commas (cobra CSV-splits --label) and all control
characters (bash's read eats a NUL) rather than just |/CR/LF; the sibling
address anchors the filename to its leading / so legacy-groom.yml is not
treated as groom.yml; the caller's default-branch name no longer reaches these
public logs; the tally counts caller files and excludes repos that failed
wholesale; and the two doc nits.

One thing the fixes surfaced: SHA_ADDR is consumed both by grep -E and as a
sed /…/ address, so adding the / closed the address early and silently
disabled the rewrite. There is now a separate escaped SHA_ADDR_SED for the sed
side. The existing multireusable test caught it, which is the argument for it
existing.

Falsification, again

Same reasoning as the first round — this diff still denies a capability, so the
new tests only assert my own premise. Re-checked against production:

Reproduced the post-fix gate exactly (same GW_USES grep, same
SHA_ADDR, same comment-stripping, same ADDR_RE) over all 51 caller files
in the six seeded rosters: 51 pass, 0 rejected. The roster's three 404s from
round 1 have since been cleaned up.
Every label in use across those rosters is "", ci or Core — nothing live
trips the tightened label rule.
No roster carries two case-variant spellings today, so the double-bump was
latent; it was also one roster edit away, which is the threat model this PR is
about.

Testing

shellcheck -x — clean.
bash .github/bump-callers/tests/test_bump_callers.sh — 299 passed, 0
failed (was 235).
Every new behavioural assertion was re-run against the pre-fix script and
fails there, so none of them pass vacuously.

_{🔄 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/134 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/5/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6471-roster-validation` --- ### 📝 Commits (6) - [`ef8aa14`](https://github.com/Comfy-Org/github-workflows/commit/ef8aa1430b92153c3c2d5ac35348042ad76d5c6e) fix(bump-callers): validate roster entries and fail on un-bumpable callers (BE-6471) - [`239a32a`](https://github.com/Comfy-Org/github-workflows/commit/239a32afa23fe010037da2b8020549be62e9e5f3) fix(bump-callers): close the review panel's gaps in the roster validation (BE-6471) - [`331c1d4`](https://github.com/Comfy-Org/github-workflows/commit/331c1d424e2c1ef1f46a93e8c21d9aaca26756cb) fix(bump-callers): close round-2 review findings on the roster gate (BE-6471) - [`76cc890`](https://github.com/Comfy-Org/github-workflows/commit/76cc890b8009aa0fcbf99fc23d59a1a6dabc35b9) fix(bump-callers): close round-3 review findings on the roster gate (BE-6471) - [`65eb430`](https://github.com/Comfy-Org/github-workflows/commit/65eb430a5aebc600a5f77e1ea1e1d1edd8897748) Merge origin/main into matt/be-6471-roster-validation - [`60467dc`](https://github.com/Comfy-Org/github-workflows/commit/60467dc7d7ad77254e92b74a0acac10aa726eda1) fix(bump-callers): reconcile merge — drop stale BE-6015 tests, fix doc drift (BE-6471) ### 📊 Changes **4 files changed** (+1137 additions, -157 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+83 -0) 📝 `.github/bump-callers/bump-callers.sh` (+430 -99) 📝 `.github/bump-callers/tests/test_bump_callers.sh` (+617 -51) 📝 `AGENTS.md` (+7 -7) </details> ### 📄 Description ## ELI-5 A config list tells our robot which repos to keep up to date. The robot trusted that list completely: it never checked that an entry named a real Comfy-Org repo or a real workflow file, and when an entry pointed somewhere with nothing to update, the robot cheerfully logged "already up to date" and went green — forever. This makes the robot check the list before it touches anything, and say "I can't move this one" instead of pretending it's fine. ## What this does Three changes, all in `bump-callers.sh` plus its tests. No entrypoint workflow is touched (one bumper, not several). **1. Roster entry validation, before any API call.** Every field reaches a privileged sink — `repo` is interpolated into each `gh api repos/<repo>/…` write and `gh pr create --repo`, `file` becomes the path committed into the caller's tree, `label` goes to `gh pr create --label` — all under an org-wide app token (`owner: Comfy-Org`, contents + pull-requests + issues write, no `repositories:` narrowing). The roster is an Actions **variable**, editable outside code review, so roster-edit access currently converts into bot-authored commits, branch force-moves and labelled PRs in any org repo the app is installed on. `repo` must now match `^Comfy-Org/[A-Za-z0-9._-]+$` (and not be a bare dot segment), `file` must match `^\.github/workflows/[A-Za-z0-9._-]+\.ya?ml$` — the class excludes `/`, so `../` traversal cannot appear — and `label` must be a string with no `|` and no newline. One jq pass emits every violation, so an entry breaking two rules reports both. The `label` rule closes a real delimiter bug: entries are carried internally as `repo|file|label|wire_bot` tuples and read back with `cut -d'|' -f3`, so a pipe-bearing label truncated at the pipe and bled its tail into `wire_bot` — silently flagging for identity wiring an entry that never asked for it. The dedup comment that assumed pipe-bearing labels were representable is corrected in the same commit. **Errors name the entry's zero-based index and the violated rule, never the value.** Masking has not been applied at that point in the script, and this repo's run logs are public. **2. An un-bumpable caller file fails the run.** A file that carries no pin this fleet can address rewrites to itself, so the content-equality check reported it as `already at <short> — skipping` and the job exited green — indefinitely, for a roster entry that is simply wrong. Each file is now checked before the rewrite, using the *same* regexes the rewrite uses so the two cannot drift apart. Two shapes trip it: no `Comfy-Org/github-workflows` pin at all, or (via the sibling-tightened address) a file whose only `github-workflows` `uses:` belongs to a different fleet's reusable. **3. End-of-run accounting.** Offenders are warned per file and tallied into one `::error::` after the whole fleet has been processed, then the job exits 1. Failing at the end rather than at the offending file preserves the fan-out — one bad entry never blocks another repo's bump, matching how `FAILED` already behaves. Only the count is printed; the per-file warnings carry the (masked) repo and the path. ## Judgment calls — please read these two **The ticket's rule for part 2 was written against a stale base, and I did not implement it as literally specified.** BE-6467 verified against `main` @ `e4a8f7c` (Aug 3, 18:56) and specified that a recognisable pin must be a **40-hex SHA**. #79 / BE-4662 merged the next morning (Aug 4, 10:55) and re-anchored the rewrite to the pin *token* rather than to 40-hex-ness — so a caller on `@v1` is now **self-healed** to the new SHA. Requiring a full SHA here would therefore hard-fail exactly the floating-tag callers the fleet exists to drag back onto immutable pins, which is a regression against #79's intent. The check requires an *addressable* pin of any literal ref shape instead, and there is a test asserting the `@v1` self-heal stays green. The ticket's test case 1 (floating tag → warning + exit 1) is consequently **not** implemented as written; the genuinely-undetectable case it was reaching for — a roster entry pointing at a file with no pin of ours — is covered instead. **Overlap with open PR #107 (BE-6015).** #107 implements the same "un-bumpable caller" idea, from the pre-#79 base, and is currently `CONFLICTING` with main. Its checks require a 40-hex pin, which is the rule #79 superseded. I have not touched it. Recommend reconciling: either close it in favour of this, or rebase it and keep only the parts this does not cover (its `# main @` / placeholder-specific test fixtures are still useful). Flagging rather than deciding, since it is not mine to close. ## Falsification — I went and looked, I did not just test my own premise This diff's user-facing outcome **denies** a capability (it turns previously-green runs red), so internal tests only assert my own premise. I checked it against production instead: - **Roster validation:** read all eight fleet variables. Six are seeded (`CURSOR_REVIEW_CALLERS` 19, `AGENTS_MD_CALLERS` 17, `GROOM_CALLERS` 9, `AUTO_LABEL_CALLERS` 5, `ASSIGN_REVIEWERS_CALLERS` 3, `PR_SIZE_CALLERS` 1 = **54 entries**); `PR_RISK_CALLERS` and `DETECT_UNREVIEWED_MERGE_CALLERS` do not exist yet, as documented. Ran the exact jq against all 54: **zero violations**. No live fleet breaks. - **Pin check:** fetched every one of those 54 caller files and ran the exact recognisability logic. **51 carry a recognisable pin.** The other 3 return HTTP 404 — the script's existing per-file 404 path warns and `continue`s *before* the new check, so they never reach it. Net: **zero real callers would trip the new hard-fail today.** - Side observation, no action taken here: those 3 404s are the "roster entry whose caller file does not exist" direction AGENTS.md warns about. Worth a separate roster clean-up; repos deliberately unnamed. ## Testing - `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh` — clean. - `bash .github/bump-callers/tests/test_bump_callers.sh` — **235 passed, 0 failed** (was 175; all 175 pre-existing assertions pass unchanged, including the `ALLOW_EMPTY` seeded-empty no-op and the malformed-variable hard-fail). - New coverage: repo outside Comfy-Org; bare dot-segment repo name (and `Comfy-Org/.github` still valid); non-workflow `file`; `.github/workflows/../evil.yml` traversal; pipe-bearing label; an entry breaking two rules reporting both; every legitimate roster shape still validating and bumping; each rejection asserting the offending value is never echoed and that it bailed before the mask loop; `@v1` self-heal stays green; a file with no pin of ours failing with the aggregate tally; a sibling-fleet-only file; and one un-bumpable entry not blocking a healthy repo's bump in the same run (the `gh` stub gained a small per-repo fixture override for that last one). --- ## Round 2 — the review panel's ten findings (`331c1d4`) Two **High**, one Medium, four Low, two Nits, plus the README follow-on. All ten addressed; each thread carries the detail. The two High ones were live defects, not hardening: - **Case-variant roster spellings were two bumps of one repo.** Accepting `comfy-org/x` case-insensitively is correct, but nothing folded the spelling, so the byte-exact repo grouping produced two `REPOS` entries and `bump_repo` ran twice — the second force-PATCHing the shared bump branch off the first's commit. Green run, half a bump. The owner is canonicalized on the way in and repos are grouped (and their entries collected) case-insensitively. - **A bare `workflows_ref:` could vouch for a file.** The `uses:`-pin requirement was gated on `GW_HAS_SIBLING`, which derives from a case-**sensitive** grep — so a caller spelled `GitHub-Workflows`, or one whose `uses:` had been removed, left the flag at 0, took the relaxed address, and had this fleet's SHA stamped onto its assets ref by the unaddressed rule 2, invisibly to the address-filtered assertion. The requirement is unconditional now, which also makes the README bullet true as written. The rest: a rejected path's label no longer folds onto the healthy file's PR; the label rule bars commas (cobra CSV-splits `--label`) and all control characters (bash's `read` eats a NUL) rather than just `|`/CR/LF; the sibling address anchors the filename to its leading `/` so `legacy-groom.yml` is not treated as `groom.yml`; the caller's default-branch name no longer reaches these public logs; the tally counts caller **files** and excludes repos that failed wholesale; and the two doc nits. One thing the fixes surfaced: `SHA_ADDR` is consumed both by `grep -E` and as a sed `/…/` **address**, so adding the `/` closed the address early and silently disabled the rewrite. There is now a separate escaped `SHA_ADDR_SED` for the sed side. The existing `multireusable` test caught it, which is the argument for it existing. ### Falsification, again Same reasoning as the first round — this diff still denies a capability, so the new tests only assert my own premise. Re-checked against production: - Reproduced the **post-fix** gate exactly (same `GW_USES` grep, same `SHA_ADDR`, same comment-stripping, same `ADDR_RE`) over **all 51 caller files in the six seeded rosters: 51 pass, 0 rejected.** The roster's three 404s from round 1 have since been cleaned up. - Every label in use across those rosters is `""`, `ci` or `Core` — nothing live trips the tightened label rule. - No roster carries two case-variant spellings today, so the double-bump was latent; it was also one roster edit away, which is the threat model this PR is about. ### Testing - `shellcheck -x` — clean. - `bash .github/bump-callers/tests/test_bump_callers.sh` — **299 passed, 0 failed** (was 235). - Every new behavioural assertion was re-run against the pre-fix script and **fails** there, so none of them pass vacuously. --- <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:44 +00:00