fix(bump-callers): fetch the roster at run time and mask it before the public log ever sees it (BE-6482) - #138
fix(bump-callers): fetch the roster at run time and mask it before the public log ever sees it (BE-6482)#138mattmillerai wants to merge 2 commits into
Conversation
…e log (BE-6482) Every bump-*-callers entrypoint passed its caller roster to bump-callers.sh through the step's `env:` block. GitHub Actions prints a step's env block into the run log BEFORE the step runs, so in this PUBLIC repo the raw roster landed in a world-readable log ahead of the script's `::add-mask::` loop — masking could not win a race it started after. bump-callers.sh now reads the roster itself, from the Actions variable VAR_NAME names, keyed on GITHUB_REPOSITORY. The fetch uses `gh api` rather than `gh variable get` because only the REST path distinguishes 404 (no such variable → an empty roster, so the existing ALLOW_EMPTY semantics are preserved exactly) from 403 (the token cannot read Actions variables → an explicit, operator-actionable error). No error path echoes the fetched value. CALLERS_JSON survives as an explicit override for the test suite and emergency manual runs. The test is `+x`, not `-z`: a set-but-empty value still means "empty roster", so an operator's deliberate `CALLERS_JSON=''` can never fall through to bumping the live fleet. Least-privilege regression, deliberate and unavoidable: the four entrypoints that downscoped their App token (auto-label, groom, pr-risk, detect-unreviewed-merge) no longer pass any `permission-*` input. A downscoped installation token cannot read Actions variables, and there is no `permission-variables` to ask for — GitHub's app-permissions schema has no `variables` key, verified against the action's own action.yml at the pinned v1.12.0 (48 permission-* inputs) and at its default branch (54): zero occurrences of "variab" in either. Minting with no downscoping inputs is the only spelling that yields the App's "Variables: read" grant, which is what the other four entrypoints already did. DEPLOY PRECONDITION: the Cloud Code Bot App needs repository permission "Variables: read" granted and approved on the Comfy-Org installation. Without it every fleet fails with the named 403 error.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 3 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…en, and harden the read's failure modes (BE-6482) Addresses the cursor-review panel on #138. - Least-privilege regression (8/8 reviewers): the write token no longer has to be un-downscoped to read one variable. Each entrypoint now mints a SECOND app token, confined by `repositories: github-workflows` to this one public repo, and hands it to the script as VAR_TOKEN for the roster read alone. The four entrypoints that had `permission-contents`/`pull-requests`/`issues` get that trio back; VAR_TOKEN falls back to GH_TOKEN when unset (manual runs). - Silent-404 (6/8): an absent variable still means "empty roster", but now always logs a ::warning:: naming both scopes tried — a 404 is also how GitHub answers "this token cannot see the repository at all", which would otherwise stop a whole ALLOW_EMPTY fleet from bumping in total silence. - Repo-only narrowing: the read falls back to the org-level variable, matching what the `${{ vars.* }}` binding it replaced resolved (repo wins on a clash). Best-effort — a failed org probe stays an empty roster, never a hard error. - 403 over-attribution: rate limiting is reported as a throttle, not as a missing grant, and the grant error now carries the API's own reason. - mktemp is checked and the temp file is trapped on EXIT. - The `::` squash loops to a fixpoint; one `s|::|:|g` pass is not one. Docs: the bump-callers README and the entrypoint headers now describe the two-token split instead of the removed downscoping; the detect-unreviewed-merge ALLOW_EMPTY comment no longer contradicts its own header about seeding. Tests: 7 new cases (which token performs the read + its fallback, the org-level fallback and repo-wins precedence, a best-effort org failure, the 404 warning, a rate-limited 403, and an adversarial error body that must not smuggle a workflow command into the public log). 238 passed, 0 failed; shellcheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed The headline one: the least-privilege regression is gone, not documented. Each entrypoint now mints a second 7 new test cases, Not merging — this is review-gated. |
2 participants
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.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-*-callersrun fails with an explicit error naming exactly that grant: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 variableVAR_NAMEnames, keyed onGITHUB_REPOSITORY, instead of arriving via the step env.CALLERS_JSONis 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 deliberateCALLERS_JSON=''can never fall through into bumping the live fleet.gh api …/actions/variables/<VAR>rather thangh variable get, because only the REST path surfaces the HTTP status. 404 →CALLERS_JSON="", which hands straight to the existingALLOW_EMPTYblock, 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.::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.ymlentrypoints —CALLERS_JSON:deleted from the stepenv:block;VAR_NAMEkept and its comment updated to say the script reads the roster from it.Docs — the "Known gap" blockquote in
.github/bump-callers/README.md, theKNOWN GAPheader paragraphs in the auto-label / pr-risk / detect-unreviewed-merge entrypoints, and the AGENTS.md line that referenced the gap. Thegh variable setupdate-flow instructions are unchanged.Tests — the
ghstub gained anapi 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 -xclean 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-permissionsschema (the installation-token request body) has novariableskey, so there is nopermission-variablesinput to ask for. I verified that against the action's ownaction.ymlat both refs:permission-*inputsvariabd72941d(the pinned v1.12.0)The first cut resolved that by dropping
permission-contents/pull-requests/issuesfrom 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-tokenwithrepositories: github-workflows, and the script uses it (VAR_TOKEN) for the roster read alone.GH_TOKENstays on its narrow trio, restored on all four entrypoints that had it. The read token is still un-downscoped — that part is forced — butrepositories:confines it to this one public repo rather than every repo the App is installed on.VAR_TOKENis optional: unset, the read falls back toGH_TOKEN, so a manual invocation still works. A test asserts the read runs underVAR_TOKENand 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
ALLOW_EMPTYkeeps 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 futureVAR_NAMEtypo — which would otherwise stop a wholeALLOW_EMPTY=truefleet from bumping in total silence.repos/…/actions/variables/…is narrower than the${{ vars.* }}binding it replaced, which also resolved org-level variables. The read now falls back toorgs/{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.mktempis checked and the temp file is trapped onEXIT.::squash loops to a fixpoint — a singles|::|:|gpass 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_JSONdeletion (anticipating pr-risk "if ci(bump-callers): add the pr-risk caller-bump fleet (BE-6308) #119 has merged" — it has). Since thenbump-pr-risk-callers.yml(ci(bump-callers): add the pr-risk caller-bump fleet (BE-6308) #119) andbump-detect-unreviewed-merge-callers.ymlhave landed. Applying the ticket's list literally would have left those two 403-ing on every run. I followed the ticket's stated intent ("EVERYbump-*-callers.ymlpresent at implementation time") over its enumerated line numbers. All 8 get theVAR_TOKENstep; 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_dispatchbump-pr-size-callers (roster seeded empty,ALLOW_EMPTY=true): green, with noCALLERS_JSONin 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_dispatchbump-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 19CURSOR_REVIEW_CALLERSentries; run30604087518, auto-label, identically). GitHub's 90-day retention ages the July runs out around 2026-10-29;gh run deleteon 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.