[PR #135] [MERGED] ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472) #143

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6472-callers-rosters-to-secrets

📝 Commits (4)

f9dad3c ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472)
113758a ci(bump-callers): harden the roster fingerprint and fix stale roster docs (BE-6472)
463933c Merge main into matt/be-6472-callers-rosters-to-secrets
d983d47 Merge main into matt/be-6472-callers-rosters-to-secrets

📊 Changes

27 files changed (+579 additions, -256 deletions)

View changed files

📝 .github/bump-callers/README.md (+98 -36)
📝 .github/bump-callers/bump-callers.sh (+79 -12)
📝 .github/bump-callers/tests/test_bump_callers.sh (+74 -4)
📝 .github/workflows/bump-agents-md-callers.yml (+28 -14)
📝 .github/workflows/bump-assign-reviewers-callers.yml (+28 -15)
📝 .github/workflows/bump-auto-label-callers.yml (+40 -25)
📝 .github/workflows/bump-cursor-review-callers.yml (+26 -11)
📝 .github/workflows/bump-detect-unreviewed-merge-callers.yml (+47 -41)
📝 .github/workflows/bump-groom-callers.yml (+26 -12)
📝 .github/workflows/bump-pr-risk-callers.yml (+33 -25)
📝 .github/workflows/bump-pr-size-callers.yml (+28 -14)
📝 .github/workflows/ci-assign-reviewers.yml (+1 -1)
📝 .github/workflows/ci-groom.yml (+1 -1)
📝 .github/workflows/test-bump-callers.yml (+19 -16)
📝 AGENTS.md (+10 -5)
📝 CONTRIBUTING.md (+6 -2)
📝 README.md (+5 -3)
📝 docs/callers/README.md (+13 -7)
📝 docs/callers/agents-md-integrity.md (+1 -1)
📝 docs/callers/assign-prs-to-author.md (+1 -1)

...and 7 more files

📄 Description

ELI-5

The workflows that keep other repos' pins fresh need a list of which repos to bump. That list was stored as an Actions variable, and it was handed to the script through the step's env: block — but GitHub prints a step's env block before the step runs, so the script's own name-masking could never run early enough. On a public repo like this one, that meant the whole roster was printed in the clear in a world-readable log on every run. This moves each roster to an Actions secret with the same name. The runner masks a secret everywhere, including that env dump, so the names stop being published. Nothing else about how the fleets work changes.

What changed

All eight bump-*-callers.yml entrypoints: CALLERS_JSON: ${{ vars.X }} → ${{ secrets.X }}. VAR_NAME keeps its historical key name (it exists only to name the roster in an error message) with a comment saying it now names a secret.
Header prose in each entrypoint rewritten: repo-level Actions secret, why (variables are unmasked in the env dump; secrets are masked everywhere including there), and what it costs (no read-back — the canonical callers.json in the private ops repo is the sole source of truth). The KNOWN GAP paragraphs are deleted; the gap is closed.
Update-flow snippets everywhere: gh variable set NAME --body "$(jq -c . callers.json)" → jq -c . callers.json | gh secret set NAME --repo Comfy-Org/github-workflows.
bump-callers.sh: error wording says secret; after the parse/mask loop it logs roster: <N> caller(s), sha256 <digest> so ops can audit which roster ran without read-back. The per-repo ::add-mask:: is unchanged and still load-bearing — Actions masks the roster as one exact string, not each name inside it.
tests/test_bump_callers.sh: a static guard iterating ../../workflows/bump-*-callers.yml asserting (a) no CALLERS_JSON: ${{ vars. binding and (b) every CALLERS_JSON: binding is ${{ secrets.<NAME> }} — plus assertions that the new fingerprint line has the right shape and carries no caller name.
test-bump-callers.yml: path filter switched from eight enumerated entrypoints to the bump-*-callers.yml glob, so a fleet added later is covered by the guard on day one instead of silently exempt.
Docs: bump-callers/README.md (fleet table column + a "Why a secret and not a variable" section), AGENTS.md, and the caller-facing enrollment docs (README.md, CONTRIBUTING.md, docs/callers/*).

Pre-flight already done

All six live rosters were mirrored into same-named secrets before this PR was opened, straight from the variables' API values in one command each, and verified present in gh secret list. Unused secrets are inert, so there is no window in which a fleet has no roster. The reverse order would have broken every fleet between merge and seeding.

Verification

bash .github/bump-callers/tests/test_bump_callers.sh — 210 passed, 0 failed.
shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh — clean.
All .github/workflows/*.yml parse; python3 .github/agents-md-integrity/check_agents_md.py --root . passes (two pre-existing warnings, unchanged).
The static guard was negative-tested: reintroducing a vars. binding in one entrypoint makes both of its sub-checks fail, and reverting restores green. A guard that cannot fail is not a guard.
The "a narrowed app token can never read a variable" claim in the docs was checked against the source rather than assumed: actions/create-github-app-token exposes 48 permission-* inputs at the SHA pinned here (54 on its default branch) and the string variab appears nowhere in its action.yml at either — permission-secrets exists, permission-variables does not.

Judgment calls (please sanity-check these)

Eight entrypoints migrated, not the six named in the ticket. The repo gained bump-pr-risk-callers.yml and bump-detect-unreviewed-merge-callers.yml since the ticket was written. Both rosters are unseeded, so ${{ vars.X }} and ${{ secrets.X }} both evaluate to empty and the flip is behaviourally inert for them today. I moved them anyway because the ticket's own guard test iterates the whole glob — leaving two behind would either fail the guard or force it into an allowlist that goes stale. Seeding the detect-unreviewed-merge roster remains out of scope; its header and the README now say the masking blocker is gone and seeding is the tracked follow-on.
The fingerprint hashes the canonical form, not the raw bytes. The ticket's snippet was sha256sum <<<"$CALLERS_JSON". That is not reproducible in practice: gh secret set stores whatever bytes it is fed, and printf '%s' "$v" | gh secret set and a jq -c … | pipe differ by a trailing newline, so the same roster could fingerprint two ways and an operator comparing them would see a phantom mismatch. It now hashes jq -cS . <<<"$CALLERS_JSON", which is reproducible with exactly jq -cS . callers.json | sha256sum (documented in the README). Array order is deliberately preserved — that is semantic.
I did not add the optional bare-repo-name mask. Masking a name stripped of its Comfy-Org/ prefix would register very common substrings, and ::add-mask:: replaces them everywhere — it would garble unrelated log lines and error messages for no gain, since the script only ever echoes the full owner/name form, which is already masked.
No specific run ID is cited in any file here. The ticket names the public run that shows the leak; pointing at it from a public file is a pointer to a log still holding the old roster, so the files describe the evidence without the link.
Diff size. ~640 changed lines, above the usual one-PR ceiling, but the functional change is about ten lines: the rest is the same explanatory paragraph and update-flow snippet repeated across eight near-identical entrypoints plus the docs that referenced them. Splitting it would leave the repo self-contradicting mid-way.
Doc sweep beyond the ticket's list. README.md, CONTRIBUTING.md and docs/callers/* still told caller-repo owners to edit vars.*_CALLERS, and docs/callers/README.md handed out a gh api …/actions/variables/GROOM_CALLERS --jq .value read-back that stops working the moment the variables are deleted. Those are now instructions to ask a maintainer to set the roster secret. Left as-is: one stale claim in docs/callers/README.md that ci-groom.yml is absent from GROOM_CALLERS (it is enrolled) — pre-existing and unrelated to this change.

Merge-time runbook (ticket step 3 — cannot be done pre-merge)

After merge: gh workflow run bump-groom-callers.yml --repo Comfy-Org/github-workflows. Groom is the safe fleet to probe with (ALLOW_EMPTY: true, all pins current → clean no-op).
Fetch that run's log and assert the env dump shows CALLERS_JSON: *** and no unmasked Comfy-Org/ caller name appears anywhere. The run should also print one roster: <N> caller(s), sha256 … line — check <N> and the digest against jq -cS . callers.json | sha256sum on the canonical file.
Only after that assertion passes, delete the six now-redundant variables (gh variable delete <NAME> --repo Comfy-Org/github-workflows for each of CURSOR_REVIEW_CALLERS, AUTO_LABEL_CALLERS, AGENTS_MD_CALLERS, PR_SIZE_CALLERS, ASSIGN_REVIEWERS_CALLERS, GROOM_CALLERS) so no stale unmasked copy remains readable. Note that merging this PR does not itself trip any bump fleet's path filter, so step 1 is the only thing that exercises the change.

_{🔄 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/135 **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-6472-callers-rosters-to-secrets` --- ### 📝 Commits (4) - [`f9dad3c`](https://github.com/Comfy-Org/github-workflows/commit/f9dad3c9165ea8060dd431a6f9dc0d1813da8f94) ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472) - [`113758a`](https://github.com/Comfy-Org/github-workflows/commit/113758aa7d3217ce7e740d733116a63f96ee49a6) ci(bump-callers): harden the roster fingerprint and fix stale roster docs (BE-6472) - [`463933c`](https://github.com/Comfy-Org/github-workflows/commit/463933c221a9dd549b99d4656f29e19876263606) Merge main into matt/be-6472-callers-rosters-to-secrets - [`d983d47`](https://github.com/Comfy-Org/github-workflows/commit/d983d47531b48e28a455e07448cbbcce72741998) Merge main into matt/be-6472-callers-rosters-to-secrets ### 📊 Changes **27 files changed** (+579 additions, -256 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+98 -36) 📝 `.github/bump-callers/bump-callers.sh` (+79 -12) 📝 `.github/bump-callers/tests/test_bump_callers.sh` (+74 -4) 📝 `.github/workflows/bump-agents-md-callers.yml` (+28 -14) 📝 `.github/workflows/bump-assign-reviewers-callers.yml` (+28 -15) 📝 `.github/workflows/bump-auto-label-callers.yml` (+40 -25) 📝 `.github/workflows/bump-cursor-review-callers.yml` (+26 -11) 📝 `.github/workflows/bump-detect-unreviewed-merge-callers.yml` (+47 -41) 📝 `.github/workflows/bump-groom-callers.yml` (+26 -12) 📝 `.github/workflows/bump-pr-risk-callers.yml` (+33 -25) 📝 `.github/workflows/bump-pr-size-callers.yml` (+28 -14) 📝 `.github/workflows/ci-assign-reviewers.yml` (+1 -1) 📝 `.github/workflows/ci-groom.yml` (+1 -1) 📝 `.github/workflows/test-bump-callers.yml` (+19 -16) 📝 `AGENTS.md` (+10 -5) 📝 `CONTRIBUTING.md` (+6 -2) 📝 `README.md` (+5 -3) 📝 `docs/callers/README.md` (+13 -7) 📝 `docs/callers/agents-md-integrity.md` (+1 -1) 📝 `docs/callers/assign-prs-to-author.md` (+1 -1) _...and 7 more files_ </details> ### 📄 Description ## ELI-5 The workflows that keep other repos' pins fresh need a list of which repos to bump. That list was stored as an Actions **variable**, and it was handed to the script through the step's `env:` block — but GitHub prints a step's env block *before* the step runs, so the script's own name-masking could never run early enough. On a public repo like this one, that meant the whole roster was printed in the clear in a world-readable log on every run. This moves each roster to an Actions **secret** with the same name. The runner masks a secret everywhere, including that env dump, so the names stop being published. Nothing else about how the fleets work changes. ## What changed - **All eight `bump-*-callers.yml` entrypoints**: `CALLERS_JSON: ${{ vars.X }}` → `${{ secrets.X }}`. `VAR_NAME` keeps its historical key name (it exists only to name the roster in an error message) with a comment saying it now names a secret. - **Header prose** in each entrypoint rewritten: repo-level Actions **secret**, why (variables are unmasked in the env dump; secrets are masked everywhere including there), and what it costs (no read-back — the canonical `callers.json` in the private ops repo is the sole source of truth). The KNOWN GAP paragraphs are deleted; the gap is closed. - **Update-flow snippets** everywhere: `gh variable set NAME --body "$(jq -c . callers.json)"` → `jq -c . callers.json | gh secret set NAME --repo Comfy-Org/github-workflows`. - **`bump-callers.sh`**: error wording says *secret*; after the parse/mask loop it logs `roster: <N> caller(s), sha256 <digest>` so ops can audit which roster ran without read-back. The per-repo `::add-mask::` is unchanged and still load-bearing — Actions masks the roster as one exact string, not each name inside it. - **`tests/test_bump_callers.sh`**: a static guard iterating `../../workflows/bump-*-callers.yml` asserting (a) no `CALLERS_JSON: ${{ vars.` binding and (b) every `CALLERS_JSON:` binding is `${{ secrets.<NAME> }}` — plus assertions that the new fingerprint line has the right shape and carries no caller name. - **`test-bump-callers.yml`**: path filter switched from eight enumerated entrypoints to the `bump-*-callers.yml` glob, so a fleet added later is covered by the guard on day one instead of silently exempt. - **Docs**: `bump-callers/README.md` (fleet table column + a "Why a secret and not a variable" section), `AGENTS.md`, and the caller-facing enrollment docs (`README.md`, `CONTRIBUTING.md`, `docs/callers/*`). ## Pre-flight already done All six live rosters were mirrored into same-named secrets **before** this PR was opened, straight from the variables' API values in one command each, and verified present in `gh secret list`. Unused secrets are inert, so there is no window in which a fleet has no roster. The reverse order would have broken every fleet between merge and seeding. ## Verification - `bash .github/bump-callers/tests/test_bump_callers.sh` — **210 passed, 0 failed**. - `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh` — clean. - All `.github/workflows/*.yml` parse; `python3 .github/agents-md-integrity/check_agents_md.py --root .` passes (two pre-existing warnings, unchanged). - The static guard was **negative-tested**: reintroducing a `vars.` binding in one entrypoint makes both of its sub-checks fail, and reverting restores green. A guard that cannot fail is not a guard. - The "a narrowed app token can never read a variable" claim in the docs was checked against the source rather than assumed: `actions/create-github-app-token` exposes 48 `permission-*` inputs at the SHA pinned here (54 on its default branch) and the string `variab` appears nowhere in its `action.yml` at either — `permission-secrets` exists, `permission-variables` does not. ## Judgment calls (please sanity-check these) 1. **Eight entrypoints migrated, not the six named in the ticket.** The repo gained `bump-pr-risk-callers.yml` and `bump-detect-unreviewed-merge-callers.yml` since the ticket was written. Both rosters are **unseeded**, so `${{ vars.X }}` and `${{ secrets.X }}` both evaluate to empty and the flip is behaviourally inert for them today. I moved them anyway because the ticket's own guard test iterates the whole glob — leaving two behind would either fail the guard or force it into an allowlist that goes stale. **Seeding** the detect-unreviewed-merge roster remains out of scope; its header and the README now say the masking blocker is gone and seeding is the tracked follow-on. 2. **The fingerprint hashes the canonical form, not the raw bytes.** The ticket's snippet was `sha256sum <<<"$CALLERS_JSON"`. That is not reproducible in practice: `gh secret set` stores whatever bytes it is fed, and `printf '%s' "$v" | gh secret set` and a `jq -c … |` pipe differ by a trailing newline, so the same roster could fingerprint two ways and an operator comparing them would see a phantom mismatch. It now hashes `jq -cS . <<<"$CALLERS_JSON"`, which is reproducible with exactly `jq -cS . callers.json | sha256sum` (documented in the README). Array order is deliberately preserved — that is semantic. 3. **I did not add the optional bare-repo-name mask.** Masking a name stripped of its `Comfy-Org/` prefix would register very common substrings, and `::add-mask::` replaces them everywhere — it would garble unrelated log lines and error messages for no gain, since the script only ever echoes the full `owner/name` form, which is already masked. 4. **No specific run ID is cited in any file here.** The ticket names the public run that shows the leak; pointing at it from a public file is a pointer to a log still holding the old roster, so the files describe the evidence without the link. 5. **Diff size.** ~640 changed lines, above the usual one-PR ceiling, but the functional change is about ten lines: the rest is the same explanatory paragraph and update-flow snippet repeated across eight near-identical entrypoints plus the docs that referenced them. Splitting it would leave the repo self-contradicting mid-way. 6. **Doc sweep beyond the ticket's list.** `README.md`, `CONTRIBUTING.md` and `docs/callers/*` still told caller-repo owners to edit `vars.*_CALLERS`, and `docs/callers/README.md` handed out a `gh api …/actions/variables/GROOM_CALLERS --jq .value` read-back that stops working the moment the variables are deleted. Those are now instructions to ask a maintainer to set the roster secret. Left as-is: one stale claim in `docs/callers/README.md` that `ci-groom.yml` is absent from `GROOM_CALLERS` (it is enrolled) — pre-existing and unrelated to this change. ## Merge-time runbook (ticket step 3 — cannot be done pre-merge) 1. After merge: `gh workflow run bump-groom-callers.yml --repo Comfy-Org/github-workflows`. Groom is the safe fleet to probe with (`ALLOW_EMPTY: true`, all pins current → clean no-op). 2. Fetch that run's log and assert the env dump shows `CALLERS_JSON: ***` and no unmasked `Comfy-Org/` caller name appears anywhere. The run should also print one `roster: <N> caller(s), sha256 …` line — check `<N>` and the digest against `jq -cS . callers.json | sha256sum` on the canonical file. 3. **Only after that assertion passes**, delete the six now-redundant variables (`gh variable delete <NAME> --repo Comfy-Org/github-workflows` for each of `CURSOR_REVIEW_CALLERS`, `AUTO_LABEL_CALLERS`, `AGENTS_MD_CALLERS`, `PR_SIZE_CALLERS`, `ASSIGN_REVIEWERS_CALLERS`, `GROOM_CALLERS`) so no stale unmasked copy remains readable. Note that merging this PR does **not** itself trip any bump fleet's path filter, so step 1 is the only thing that exercises the change. --- <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:42 +00:00