[PR #138] [MERGED] fix(bump-callers): fetch the roster at run time and mask it before the public log ever sees it (BE-6482) #148

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6482-fetch-roster-at-runtime

📝 Commits (4)

acd0c42 fix(bump-callers): fetch the roster at run time and mask it before the log (BE-6482)
4a2159f fix(bump-callers): split the roster read onto its own repo-scoped token, and harden the read's failure modes (BE-6482)
17b4f0e Merge remote-tracking branch 'origin/main' into matt/be-6482-fetch-roster-at-runtime
e4bd9cf Merge remote-tracking branch 'origin/main' into matt/be-6482-fetch-roster-at-runtime

📄 Description

ELI-5

The bumper's list of which repos to bump is private. It used to be handed to the script through the workflow step's env: block — and GitHub prints a step's env block into the run log before the step runs. This repo is public, so the private repo names showed up in a world-readable log a moment before the script's masking could hide them. Now the script goes and fetches the list itself and masks it before anything is printed, so there is nothing to print early.

⚠️ DEPLOY PRECONDITION — needs an org admin before this works

The Cloud Code Bot GitHub App must be granted the repository permission Variables: read, and that permission update must be approved on the Comfy-Org installation. An agent cannot verify or grant this. Until it is granted, every bump-*-callers run fails with an explicit error naming exactly that grant:

::error::App token cannot read Actions variables — grant the Cloud Code Bot App repository permission 'Variables: read' and approve it on the Comfy-Org installation (BE-6477).

That is a loud, named failure rather than a silent no-op, on purpose.

What changed

.github/bump-callers/bump-callers.sh — the roster is now read at run time from the Actions variable VAR_NAME names, keyed on GITHUB_REPOSITORY, instead of arriving via the step env.

The fetch runs only when CALLERS_JSON is genuinely unset ([ -z "${CALLERS_JSON+x}" ], not -z "$CALLERS_JSON"). A set-but-empty value still means "empty roster" — so the suite's empty-variable cases keep their meaning, and an operator's deliberate CALLERS_JSON='' can never fall through into bumping the live fleet.
gh api …/actions/variables/<VAR> rather than gh variable get, because only the REST path surfaces the HTTP status. 404 → CALLERS_JSON="", which hands straight to the existing ALLOW_EMPTY block, so its semantics are bit-for-bit preserved (seeded-empty fleet → clean no-op; must-have-callers fleet → the loud clobber-detecting error). 403 → the Variables-grant error above. Anything else → an ::error:: carrying the sanitized reason only.
No error path ever echoes the fetched value. The ::add-mask:: loop remains the first place the roster's repo names are used, and carries a comment stating that invariant.

All 8 bump-*-callers.yml entrypoints — CALLERS_JSON: deleted from the step env: block; VAR_NAME kept and its comment updated to say the script reads the roster from it.

Docs — the "Known gap" blockquote in .github/bump-callers/README.md, the KNOWN GAP header paragraphs in the auto-label / pr-risk / detect-unreviewed-merge entrypoints, and the AGENTS.md line that referenced the gap. The gh variable set update-flow instructions are unchanged.

Tests — the gh stub gained an api repos/*/actions/variables/* handler (configurable roster fixture, 404 mode, 403 mode, 500 mode, and it logs every invocation). Seven new cases; every existing case is untouched. bash .github/bump-callers/tests/test_bump_callers.sh → 207 passed, 0 failed; shellcheck -x clean on both files.

Least privilege — kept, via a second token

The roster read needs a token that can read Actions variables, and a downscoped App token cannot: GitHub's app-permissions schema (the installation-token request body) has no variables key, so there is no permission-variables input to ask for. I verified that against the action's own action.yml at both refs:

ref	`permission-*` inputs	occurrences of `variab`
`d72941d` (the pinned v1.12.0)	48	0
default branch (v3.x)	54	0

The first cut resolved that by dropping permission-contents/pull-requests/issues from four entrypoints, widening the write token — the one that commits, branches, opens PRs and stamps labels across every caller repo — to full installation permissions. Every reviewer on the panel flagged it, and they were right: it is avoidable.

Now there are two tokens. Each entrypoint mints a second create-github-app-token with repositories: github-workflows, and the script uses it (VAR_TOKEN) for the roster read alone. GH_TOKEN stays on its narrow trio, restored on all four entrypoints that had it. The read token is still un-downscoped — that part is forced — but repositories: confines it to this one public repo rather than every repo the App is installed on. VAR_TOKEN is optional: unset, the read falls back to GH_TOKEN, so a manual invocation still works. A test asserts the read runs under VAR_TOKEN and not the write token, so collapsing back to one token fails CI.

The action pin itself is untouched (v2+ breaks the cross-repo owner: scoping the bumper depends on — see the in-file comment and the dependabot ignore); repositories: is confirmed present at the pinned ref.

Read failure modes — hardened after review

A 404 is never silent. It still folds into the empty roster (so ALLOW_EMPTY keeps its exact semantics), but always logs a ::warning:: naming the variable and both scopes tried. A 404 is also how GitHub answers "this token cannot see the repository at all" — App uninstalled, repo-selection drift, a rename, a future VAR_NAME typo — which would otherwise stop a whole ALLOW_EMPTY=true fleet from bumping in total silence.
Org-level variables resolve again. repos/…/actions/variables/… is narrower than the ${{ vars.* }} binding it replaced, which also resolved org-level variables. The read now falls back to orgs/{owner}/… after a repo-scope 404, repo-level winning on a name clash. Best-effort: a failed org probe stays an empty roster, never a hard error.
A 403 is not automatically "missing grant". Rate limiting is matched first and reported as a throttle; the grant error names the other 403 causes (IP allow-list, SAML, suspended installation) and carries the API's own sanitized reason.
mktemp is checked and the temp file is trapped on EXIT.
The :: squash loops to a fixpoint — a single s|::|:|g pass is not one, since ::::x:::: collapses back into ::x::. An adversarial-error-body case asserts no workflow command survives into the log.

Judgment calls

Scope: 8 entrypoints, not 6. The ticket was written against the fleet as it stood then and named 6 entrypoints for the CALLERS_JSON deletion (anticipating pr-risk "if #119 has merged" — it has). Since then bump-pr-risk-callers.yml (#119) and bump-detect-unreviewed-merge-callers.yml have landed. Applying the ticket's list literally would have left those two 403-ing on every run. I followed the ticket's stated intent ("EVERY bump-*-callers.yml present at implementation time") over its enumerated line numbers. All 8 get the VAR_TOKEN step; the 4 that were downscoped stay downscoped.
detect-unreviewed-merge's unseeded roster. That fleet is deliberately unseeded specifically because of this gap — its header and the README say to seed it "as the immediate follow-on to the masking fix". I updated both to say the blocker is now gone and to seed only after a dispatch is observed reading the variable without the 403, rather than seeding it myself (it needs private caller names I don't have, and it is an operator action either way). The run stays red until someone seeds it.
404 folds into "empty". As specified. Worth knowing the consequence: if GitHub ever answered a permissions failure with 404 instead of 403, a seeded-empty fleet would no-op silently. The must-have-callers fleets (cursor-review, auto-label, detect-unreviewed-merge) would still fail loudly, so the condition cannot go unnoticed fleet-wide.

Verify after merge (needs the App grant first)

workflow_dispatch bump-pr-size-callers (roster seeded empty, ALLOW_EMPTY=true): green, with no CALLERS_JSON in the step env dump. A 403 here means the Variables grant is still missing — grant it, then re-dispatch. A ::warning::No PR_SIZE_CALLERS Actions variable found … instead means the variable is absent rather than seeded empty.
workflow_dispatch bump-cursor-review-callers: env dump carries no roster, repo names appear only as ***, and the caller bump PRs are still opened/updated.

Note for the operator — the already-leaked rosters

This fix stops future leaks; it cannot retract past ones. The rosters already sit in pre-fix public run logs (e.g. run 30604087646, bump-cursor-review, 2026-07-31 — all 19 CURSOR_REVIEW_CALLERS entries; run 30604087518, auto-label, identically). GitHub's 90-day retention ages the July runs out around 2026-10-29; gh run delete on the old bump runs removes them sooner. That is your call — out of scope here.

Out of scope

Converting the roster variables to secrets (rejected in the ticket: the repo deliberately keeps them API-readable for ops/audit; it is the fallback only if the Variables grant is refused). Deleting historical runs.

_{🔄 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/138 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/5/2026 **Status:** ✅ Merged **Merged:** 8/17/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6482-fetch-roster-at-runtime` --- ### 📝 Commits (4) - [`acd0c42`](https://github.com/Comfy-Org/github-workflows/commit/acd0c426d17bd79cf67ae7d550147192cfcb1b60) fix(bump-callers): fetch the roster at run time and mask it before the log (BE-6482) - [`4a2159f`](https://github.com/Comfy-Org/github-workflows/commit/4a2159f7c552290e7dcf02a580b6b1feba7a8732) fix(bump-callers): split the roster read onto its own repo-scoped token, and harden the read's failure modes (BE-6482) - [`17b4f0e`](https://github.com/Comfy-Org/github-workflows/commit/17b4f0e3008d7c6580fde6be8a4d840500324335) Merge remote-tracking branch 'origin/main' into matt/be-6482-fetch-roster-at-runtime - [`e4bd9cf`](https://github.com/Comfy-Org/github-workflows/commit/e4bd9cf2e59fdc0012f21e1cc7096f12975e78fe) Merge remote-tracking branch 'origin/main' into matt/be-6482-fetch-roster-at-runtime ### 📄 Description ## ELI-5 The bumper's list of which repos to bump is private. It used to be handed to the script through the workflow step's `env:` block — and GitHub prints a step's env block into the run log *before* the step runs. This repo is public, so the private repo names showed up in a world-readable log a moment before the script's masking could hide them. Now the script goes and fetches the list itself and masks it before anything is printed, so there is nothing to print early. ## ⚠️ DEPLOY PRECONDITION — needs an org admin before this works The **Cloud Code Bot** GitHub App must be granted the repository permission **Variables: read**, and that permission update must be **approved on the Comfy-Org installation**. An agent cannot verify or grant this. Until it is granted, every `bump-*-callers` run fails with an explicit error naming exactly that grant: ``` ::error::App token cannot read Actions variables — grant the Cloud Code Bot App repository permission 'Variables: read' and approve it on the Comfy-Org installation (BE-6477). ``` That is a loud, named failure rather than a silent no-op, on purpose. ## What changed **`.github/bump-callers/bump-callers.sh`** — the roster is now read at run time from the Actions variable `VAR_NAME` names, keyed on `GITHUB_REPOSITORY`, instead of arriving via the step env. - The fetch runs **only when `CALLERS_JSON` is genuinely unset** (`[ -z "${CALLERS_JSON+x}" ]`, not `-z "$CALLERS_JSON"`). A set-but-empty value still means "empty roster" — so the suite's empty-variable cases keep their meaning, and an operator's deliberate `CALLERS_JSON=''` can never fall through into bumping the live fleet. - `gh api …/actions/variables/<VAR>` rather than `gh variable get`, because only the REST path surfaces the HTTP status. **404 → `CALLERS_JSON=""`**, which hands straight to the existing `ALLOW_EMPTY` block, so its semantics are bit-for-bit preserved (seeded-empty fleet → clean no-op; must-have-callers fleet → the loud clobber-detecting error). **403 →** the Variables-grant error above. **Anything else →** an `::error::` carrying the sanitized *reason* only. - No error path ever echoes the fetched value. The `::add-mask::` loop remains the first place the roster's repo names are used, and carries a comment stating that invariant. **All 8 `bump-*-callers.yml` entrypoints** — `CALLERS_JSON:` deleted from the step `env:` block; `VAR_NAME` kept and its comment updated to say the script reads the roster from it. **Docs** — the "Known gap" blockquote in `.github/bump-callers/README.md`, the `KNOWN GAP` header paragraphs in the auto-label / pr-risk / detect-unreviewed-merge entrypoints, and the AGENTS.md line that referenced the gap. The `gh variable set` update-flow instructions are unchanged. **Tests** — the `gh` stub gained an `api repos/*/actions/variables/*` handler (configurable roster fixture, 404 mode, 403 mode, 500 mode, and it logs every invocation). Seven new cases; every existing case is untouched. `bash .github/bump-callers/tests/test_bump_callers.sh` → **207 passed, 0 failed**; `shellcheck -x` clean on both files. ## Least privilege — kept, via a second token The roster read needs a token that can read Actions variables, and a downscoped App token cannot: GitHub's `app-permissions` schema (the installation-token request body) has no `variables` key, so there is no `permission-variables` input to ask for. I verified that against the action's own `action.yml` at both refs: | ref | `permission-*` inputs | occurrences of `variab` | |---|---|---| | `d72941d` (the pinned v1.12.0) | 48 | **0** | | default branch (v3.x) | 54 | **0** | The first cut resolved that by dropping `permission-contents`/`pull-requests`/`issues` from four entrypoints, widening the *write* token — the one that commits, branches, opens PRs and stamps labels across every caller repo — to full installation permissions. Every reviewer on the panel flagged it, and they were right: it is avoidable. **Now there are two tokens.** Each entrypoint mints a second `create-github-app-token` with `repositories: github-workflows`, and the script uses it (`VAR_TOKEN`) for the roster read *alone*. `GH_TOKEN` stays on its narrow trio, restored on all four entrypoints that had it. The read token is still un-downscoped — that part is forced — but `repositories:` confines it to this one **public** repo rather than every repo the App is installed on. `VAR_TOKEN` is optional: unset, the read falls back to `GH_TOKEN`, so a manual invocation still works. A test asserts the read runs under `VAR_TOKEN` and not the write token, so collapsing back to one token fails CI. The action pin itself is untouched (v2+ breaks the cross-repo `owner:` scoping the bumper depends on — see the in-file comment and the dependabot ignore); `repositories:` is confirmed present at the pinned ref. ## Read failure modes — hardened after review - **A 404 is never silent.** It still folds into the empty roster (so `ALLOW_EMPTY` keeps its exact semantics), but always logs a `::warning::` naming the variable and both scopes tried. A 404 is also how GitHub answers "this token cannot see the repository at all" — App uninstalled, repo-selection drift, a rename, a future `VAR_NAME` typo — which would otherwise stop a whole `ALLOW_EMPTY=true` fleet from bumping in total silence. - **Org-level variables resolve again.** `repos/…/actions/variables/…` is narrower than the `${{ vars.* }}` binding it replaced, which also resolved org-level variables. The read now falls back to `orgs/{owner}/…` after a repo-scope 404, repo-level winning on a name clash. Best-effort: a failed org probe stays an empty roster, never a hard error. - **A 403 is not automatically "missing grant".** Rate limiting is matched first and reported as a throttle; the grant error names the other 403 causes (IP allow-list, SAML, suspended installation) and carries the API's own sanitized reason. - **`mktemp` is checked** and the temp file is trapped on `EXIT`. - **The `::` squash loops to a fixpoint** — a single `s|::|:|g` pass is not one, since `::::x::::` collapses back into `::x::`. An adversarial-error-body case asserts no workflow command survives into the log. ## Judgment calls 1. **Scope: 8 entrypoints, not 6.** The ticket was written against the fleet as it stood then and named 6 entrypoints for the `CALLERS_JSON` deletion (anticipating pr-risk "if #119 has merged" — it has). Since then `bump-pr-risk-callers.yml` (#119) and `bump-detect-unreviewed-merge-callers.yml` have landed. Applying the ticket's list literally would have left those two 403-ing on every run. I followed the ticket's stated intent ("EVERY `bump-*-callers.yml` present at implementation time") over its enumerated line numbers. All 8 get the `VAR_TOKEN` step; the 4 that were downscoped stay downscoped. 2. **detect-unreviewed-merge's unseeded roster.** That fleet is deliberately unseeded *specifically because of this gap* — its header and the README say to seed it "as the immediate follow-on to the masking fix". I updated both to say the blocker is now gone and to seed only after a dispatch is observed reading the variable without the 403, rather than seeding it myself (it needs private caller names I don't have, and it is an operator action either way). The run stays red until someone seeds it. 3. **404 folds into "empty".** As specified. Worth knowing the consequence: if GitHub ever answered a permissions failure with 404 instead of 403, a seeded-*empty* fleet would no-op silently. The must-have-callers fleets (cursor-review, auto-label, detect-unreviewed-merge) would still fail loudly, so the condition cannot go unnoticed fleet-wide. ## Verify after merge (needs the App grant first) 1. `workflow_dispatch` **bump-pr-size-callers** (roster seeded empty, `ALLOW_EMPTY=true`): green, with **no `CALLERS_JSON`** in the step env dump. A 403 here means the Variables grant is still missing — grant it, then re-dispatch. A `::warning::No PR_SIZE_CALLERS Actions variable found …` instead means the variable is absent rather than seeded empty. 2. `workflow_dispatch` **bump-cursor-review-callers**: env dump carries no roster, repo names appear only as `***`, and the caller bump PRs are still opened/updated. ## Note for the operator — the already-leaked rosters This fix stops *future* leaks; it cannot retract past ones. The rosters already sit in pre-fix public run logs (e.g. run `30604087646`, bump-cursor-review, 2026-07-31 — all 19 `CURSOR_REVIEW_CALLERS` entries; run `30604087518`, auto-label, identically). GitHub's 90-day retention ages the July runs out around **2026-10-29**; `gh run delete` on the old bump runs removes them sooner. That is your call — out of scope here. ## Out of scope Converting the roster variables to secrets (rejected in the ticket: the repo deliberately keeps them API-readable for ops/audit; it is the fallback only if the Variables grant is refused). Deleting historical runs. --- <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:46 +00:00