fix(bump-callers): fetch the roster at run time and mask it before the public log ever sees it (BE-6482)

.github/bump-callers/README.md

@@ -5,9 +5,9 @@ workflows from rotting. When a reusable workflow is updated on `main`, it opens
 a SHA-bump PR in every repo that pins a caller against it — so consumers move
 forward automatically instead of silently drifting commits behind.
 
-- **`bump-callers.sh`** — the one, fleet-agnostic bump script (parse the caller
-  list, mask private repo names, rewrite the pin, keep one bump PR per caller
-  current). It is the single source of truth; the workflow entrypoints are
+- **`bump-callers.sh`** — the one, fleet-agnostic bump script (fetch the caller
+  list from its fleet's Actions variable, mask private repo names, rewrite the
+  pin, keep one bump PR per caller current). It is the single source of truth; the workflow entrypoints are
   thin wrappers that only supply per-fleet parameters. A forked copy is how other
   shared machinery in the org has drifted — this stays one file on purpose.
   - **One open bump PR per (repo, fleet), updated in place.** The head branch is
@@ -38,19 +38,22 @@ Its 12 live callers are known and correctly wired (each pins a full 40-hex SHA
 against this repo's path, i.e. exactly what the rewrite moves). It is unseeded
 anyway, on purpose.
 
-Every roster reaches `bump-callers.sh` through the step's `env:` block, and
+Every roster used to reach `bump-callers.sh` through the step's `env:` block, and
 Actions prints that block — values and all — before the script's `::add-mask::`
-can run. This repo is public, so each seeded fleet already publishes its roster
-in a world-readable log. That is the known gap documented in every `bump-*`
-header. The difference here is that two of this fleet's callers are non-public
-repos that appear in **no** already-seeded roster, so seeding would publish two
-names that are not out yet — and a public log entry cannot be unpublished.
-
-So the trade is: seed now and take an irreversible disclosure, or leave it
-unseeded and take a red run. The red run is reversible and is already the
-designed behaviour for an empty roster, so it wins. Seed the variable as the
-immediate follow-on to the masking fix — never as a way to turn that red run
-green.
+could run. This repo is public, so each seeded fleet had already published its
+roster in a world-readable log. The difference here was that two of this fleet's
+callers are non-public repos appearing in **no** already-seeded roster, so
+seeding would have published two names that are not out yet — and a public log
+entry cannot be unpublished. The trade was: seed now and take an irreversible
+disclosure, or leave it unseeded and take a red run. The red run is reversible
+and is already the designed behaviour for an empty roster, so it won.
+
+**That blocker is gone** (BE-6482 — the roster is fetched and masked inside the
+script), but the variable is still unset, so the red run persists until someone
+seeds it. Do that once a dispatch of the entrypoint has been seen reading its
+variable *without* the "cannot read Actions variables" error — that error means
+the App still lacks **Variables: read**, and a roster the script cannot read is
+one it cannot mask either. Never seed it merely to turn the red run green.
 
 ### Reusables with no fleet — deliberate, not an oversight
 
@@ -154,13 +157,39 @@ repo-level Actions **variable** (config, not a credential) as a JSON array of
 `{"repo","file","label"}` objects (`label` optional). `bump-callers.sh`
 `::add-mask::`es every repo name out of the run logs before echoing it.
 
-> **Known gap.** Each entrypoint hands the roster to the script through the
-> step's `env:`, and Actions prints a step's env block *before* the step runs —
-> so the raw roster appears in the (public) log ahead of any masking. Closing it
-> means fetching the variable at run time (`gh variable get`) and masking it
-> before first use, which needs a token permission the fleets do not mint today.
-> It is fleet-wide; no single entrypoint can fix it. Until then, assume the
-> roster is public.
+The roster is **fetched at run time by the script**, keyed by the `VAR_NAME` its
+entrypoint passes, and masked before the first log line — it is not bound into
+the step's `env:` block any more (BE-6482). That binding was the leak: Actions
+prints a step's env block *before* the step runs, so the raw roster reached the
+public log ahead of the masking meant to cover it. Fetching inside the script
+puts the read, the shape check and the `::add-mask::` in an order the log cannot
+get in front of.
+
+Two consequences worth knowing:
+
+- **Two tokens, not one.** A downscoped App token cannot read Actions variables
+  — the app-permissions schema (the installation-token request body) has no
+  `variables` key, so there is no `permission-variables` input to ask for, and
+  only a token minted with *no* `permission-*` inputs carries the App's
+  **Variables: read** grant. Rather than widen the token that writes across the
+  whole fleet, each entrypoint mints a **second** token scoped to this repo alone
+  (`repositories: github-workflows`) and hands it to the script as `VAR_TOKEN`,
+  used for the roster read and nothing else. The write token (`GH_TOKEN`) keeps
+  its `contents` / `pull-requests` / `issues` downscoping. `VAR_TOKEN` is
+  optional: unset, the read falls back to `GH_TOKEN` (manual runs).
+- **The App needs the grant.** The Cloud Code Bot App itself must hold the
+  repository permission **Variables: read**, approved on the Comfy-Org
+  installation, or every fleet fails with an explicit error naming that grant.
+
+The read tries the **repo-level** variable first and falls back to the
+**org-level** one, matching what the `${{ vars.* }}` binding it replaced
+resolved (repo wins on a name clash). Absent at both scopes is an *empty roster*
+— handled by `ALLOW_EMPTY` exactly as an empty variable is — but it always logs
+a `::warning::`, because a 404 is also how GitHub answers "this token cannot see
+the repository at all".
+
+`CALLERS_JSON` still works as an explicit override for a manual run and is what
+the test suite drives; a set-but-empty value means "empty roster", not "fetch".
 
 Adding/removing a caller needs **no public commit** — edit the variable:
 

.github/bump-callers/bump-callers.sh

@@ -20,15 +20,39 @@
 # every repo name is `::add-mask::`ed out of the run logs before it is ever
 # echoed.
 #
+# The roster is FETCHED HERE at run time rather than handed in through the
+# entrypoint's step `env:` block (BE-6482). Actions prints a step's env block into
+# the run log BEFORE the step runs, so an `env:`-passed roster appeared in this
+# PUBLIC repo's world-readable logs ahead of the `::add-mask::` loop below — the
+# masking could not win a race it started after. Reading the variable inside the
+# script puts the fetch, the validation and the mask in an order the log cannot
+# get in front of.
+#
 # Required environment:
 #   GH_TOKEN       Token with contents:write + pull-requests:write on the callers
 #                  (the Cloud Code Bot app token generated by the entrypoint).
+#                  This is the WRITE token and stays downscoped; it is used for
+#                  the roster read only as a fallback when VAR_TOKEN is unset.
 #   NEW_SHA        The github-workflows commit to pin callers to (github.sha).
-#   CALLERS_JSON   The caller list (value of the fleet's Actions variable).
-#   VAR_NAME       Name of that variable — used only in error messages.
+#   VAR_NAME       Name of the fleet's Actions variable. The roster is READ from
+#                  it at run time (and it names the variable in error messages).
+#   GITHUB_REPOSITORY  Owner/name of THIS repo, for that read (Actions sets it).
 #   TAG            Human tag for branch/commit/PR text (e.g. "cursor-review").
 #   WORKFLOW_FILE  Reusable workflow filename referenced in the PR body.
 # Optional:
+#   VAR_TOKEN      Token used for the roster read ALONE. The entrypoints mint a
+#                  second app token for it, scoped to THIS repo only and with no
+#                  `permission-*` downscoping, because the app-permissions API
+#                  schema (the installation-token request body) has no `variables`
+#                  key to ask for — only an un-downscoped token carries the App's
+#                  "Variables: read" grant (BE-6477). Splitting it off is what
+#                  keeps GH_TOKEN, which writes to every caller repo in the fleet,
+#                  on its narrow trio. Falls back to GH_TOKEN when unset.
+#   CALLERS_JSON   OVERRIDE for the runtime fetch — the caller list, verbatim.
+#                  Used by the test suite and available for an emergency manual
+#                  run; the entrypoints deliberately do NOT set it. Set-but-EMPTY
+#                  is an override too (it means "empty roster", handled by
+#                  ALLOW_EMPTY below); only a genuinely UNSET value fetches.
 #   ALLOW_EMPTY      "true" → an empty caller list is a clean no-op (a fleet that
 #                    is seeded empty and grows). Default "false" → an empty/missing
 #                    list is a hard error (a fleet that always has callers must
@@ -53,10 +77,105 @@ set -uo pipefail
 : "${VAR_NAME:?VAR_NAME is required}"
 : "${TAG:?TAG is required}"
 : "${WORKFLOW_FILE:?WORKFLOW_FILE is required}"
-CALLERS_JSON="${CALLERS_JSON-}"
 ALLOW_EMPTY="${ALLOW_EMPTY:-false}"
 WIRE_BOT_SCRIPT="${WIRE_BOT_SCRIPT-}"
 
+# Load the roster. Normally it is READ HERE, from this repo's own Actions
+# variable named by VAR_NAME — see the header for why it is not passed in
+# through the step's `env:`.
+#
+# The test is `+x`, i.e. "is CALLERS_JSON set AT ALL", NOT `-z "$CALLERS_JSON"`.
+# A set-but-EMPTY override must keep meaning "the roster is empty" and fall
+# through to the ALLOW_EMPTY handling below — the suite drives the empty-variable
+# hard-fail that way, and more importantly an emergency manual run that passes
+# `CALLERS_JSON=''` must not silently reach past it and bump the LIVE roster.
+if [[ -z "${CALLERS_JSON+x}" ]]; then
+  if [[ -z "${GITHUB_REPOSITORY:-}" ]]; then
+    echo "::error::GITHUB_REPOSITORY is unset, so the ${VAR_NAME} Actions variable cannot be read. Run this from Actions, or pass the roster explicitly in CALLERS_JSON."
+    exit 1
+  fi
+  # `gh api`, NOT `gh variable get`: only the REST path surfaces the HTTP status,
+  # and telling 404 from 403 is the whole point here. `gh variable get` exits 1
+  # for both, so an absent variable (a legitimate empty roster for a seeded-empty
+  # fleet) would be indistinguishable from a token that cannot read variables at
+  # all — one is a clean no-op, the other needs an operator to grant a permission.
+  #
+  # The read runs under VAR_TOKEN when the entrypoint mints one: a SECOND app
+  # token, scoped to THIS repo alone (`repositories: github-workflows`) and left
+  # un-downscoped so it carries the App's "Variables: read" grant. That is what
+  # lets GH_TOKEN — the token that actually WRITES across every caller repo —
+  # keep its narrow `permission-contents`/`pull-requests`/`issues` trio, instead
+  # of the whole fleet's write token being widened to full installation
+  # permissions just to enable one variable read (BE-6482 review). Falling back
+  # to GH_TOKEN keeps a manual/legacy invocation working.
+  VAR_ERR=$(mktemp) || {
+    echo "::error::Could not create a temp file for the ${VAR_NAME} read's stderr (mktemp failed)."
+    exit 1
+  }
+  trap 'rm -f "$VAR_ERR"' EXIT
+  VAR_READ_TOKEN="${VAR_TOKEN:-${GH_TOKEN:-}}"
+
+  # Reduce an API error body to ONE safe log line: the REASON only, never the
+  # value (the value goes to stdout; this reads stderr). The run logs of this
+  # public repo are a workflow-command sink, so a `\r::` in an error body could
+  # otherwise inject a command of its own. The `::` squash LOOPS to a fixpoint —
+  # a single `s|::|:|g` pass is not one, since `::::x::::` collapses straight
+  # back into `::x::`.
+  sanitize_api_error() {
+    local s
+    s=$(tr -d '\r' <"$1" | tr '\n' ' ' | sed -E 's|[[:space:]]+| |g; s|^ ||; s| $||')
+    while [[ "$s" == *::* ]]; do s="${s//::/:}"; done
+    printf '%s' "$s"
+  }
+
+  # A non-404 failure of the repo-scope read. Never recoverable here, so it ends
+  # the run — but with the RIGHT remediation: 403 is not only "missing grant".
+  var_read_die() {
+    local reason; reason=$(sanitize_api_error "$VAR_ERR")
+    if grep -qiE 'rate limit|secondary rate|abuse detection' "$VAR_ERR"; then
+      echo "::error::Could not read the ${VAR_NAME} Actions variable — GitHub throttled the request (retry later, no permission change needed): ${reason}"
+    elif grep -qi 'HTTP 403' "$VAR_ERR"; then
+      # Overwhelmingly the missing grant, so lead with it — but a 403 is also how
+      # GitHub answers an IP-allow-list / SAML-enforcement / suspended-installation
+      # denial, so carry the sanitized reason rather than asserting the cause.
+      echo "::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). A 403 can also mean an IP allow-list, SAML enforcement or a suspended installation — API said: ${reason}"
+    else
+      echo "::error::Could not read the ${VAR_NAME} Actions variable: ${reason}"
+    fi
+    exit 1
+  }
+
+  if ! CALLERS_JSON=$(GH_TOKEN="$VAR_READ_TOKEN" gh api "repos/${GITHUB_REPOSITORY}/actions/variables/${VAR_NAME}" --jq '.value' 2>"$VAR_ERR"); then
+    grep -qi 'HTTP 404' "$VAR_ERR" || var_read_die
+    # A repo-scope 404 is not yet "no roster". The `${{ vars.* }}` binding this
+    # read replaced ALSO resolved ORG-level variables (repo-level winning on a
+    # name clash), so try the org before concluding the roster is empty —
+    # otherwise a fleet whose roster is defined org-wide silently becomes an
+    # empty one. Best-effort: this repo's rosters are repo-level by convention
+    # and the App may hold no org-level Variables grant at all, so a FAILED org
+    # probe must not turn today's clean no-op into a hard failure.
+    VAR_OWNER="${GITHUB_REPOSITORY%%/*}"
+    if ! CALLERS_JSON=$(GH_TOKEN="$VAR_READ_TOKEN" gh api "orgs/${VAR_OWNER}/actions/variables/${VAR_NAME}" --jq '.value' 2>"$VAR_ERR"); then
+      # Genuinely absent at BOTH scopes == an empty roster, spelled the same way
+      # an empty variable is, so the ALLOW_EMPTY block below keeps its EXACT
+      # semantics: a clean no-op for a fleet seeded empty, and the loud
+      # clobber-detecting error for a fleet that must always have callers
+      # (auto-label, cursor-review, detect-unreviewed-merge).
+      #
+      # Say so OUT LOUD either way. A 404 is also what GitHub returns when the
+      # token cannot see the repository at all — App uninstalled, repo-selection
+      # drift, a renamed repo, a VAR_NAME typo in some future fleet — and for the
+      # ALLOW_EMPTY=true fleets that would otherwise be a completely silent
+      # "nothing to do" while the whole fleet quietly stops being bumped, which
+      # is the exact pin rot this bumper exists to prevent.
+      echo "::warning::No ${VAR_NAME} Actions variable found on ${GITHUB_REPOSITORY} or on org ${VAR_OWNER} (HTTP 404 at the repo scope; org probe: $(sanitize_api_error "$VAR_ERR")) — treating the roster as EMPTY. If this fleet is supposed to have callers, the variable is missing or misnamed, or the token cannot see this repository."
+      CALLERS_JSON=""
+    fi
+  fi
+  rm -f "$VAR_ERR"
+  trap - EXIT
+fi
+
 SHORT="${NEW_SHA:0:7}"
 # Stable branch per (repo, TAG) — deliberately NOT SHA-stamped. A fixed head
 # branch is what lets a subsequent bump reuse (and update in place) the one open
@@ -141,7 +260,16 @@ fi
 
 # Parse the JSON into repo|file|label|wire_bot tuples, and mask every repo name
 # in the (publicly viewable) run logs BEFORE the loop that echoes it, so all
-# per-repo output shows *** instead of a private repo name. jq's `//` treats
+# per-repo output shows *** instead of a private repo name.
+#
+# INVARIANT (BE-6482): this loop is the FIRST place the roster's repo names are
+# used for anything, and nothing above it may log the roster — not the fetch, not
+# the empty/shape validation, not an error path. Everything before this point
+# either handles the value silently or reports only a REASON, and the order
+# fetch → validate → mask must stay that way. The whole reason the roster is read
+# here instead of arriving through the step's `env:` is that Actions printed the
+# env block before this loop could run; adding an echo of the value above it
+# re-opens exactly that hole, in a public log, irreversibly. jq's `//` treats
 # both `false` and `null` as falsy, so an absent/false/null wire_bot all print
 # the empty string here — exactly the "not flagged" case.
 CALLERS=()