ci(bump-callers): seed the pr-risk roster and hard-fail on an empty one (BE-6480)

[mattmillerai]

ELI-5

pr-risk is a workflow other repos borrow. When it changes, a "bumper" is supposed to open a PR in each borrowing repo moving its pin forward. That bumper was built last week but its address book was left empty, and it was configured to shrug and exit successfully when the address book is empty. So it did nothing, quietly, and one borrower has been running a two-revisions-old grader. This fills in the address book with the two real borrowers and removes the shrug, so an empty address book is now a loud failure instead of a silent one.

Scope note — most of this ticket already shipped

BE-6480 was written against a spike (BE-6474) that predates #119 (BE-6308), which already landed the entrypoint and the docs registration. Rather than re-do that, I verified each of the ticket's criteria against main and implemented only the genuine remainder. Criterion-by-criterion:

Ticket criterion	State
1. bump-pr-risk-callers.yml exists, correct name/concurrency/paths/workflow_dispatch/env/decommission guard/stale guard/no WIRE_BOT_SCRIPT	Already on main via #119 — verified line by line, all match (#119's decommission guard and stale-run guard are strictly stronger than the groom model the ticket specified)
1. no ALLOW_EMPTY	Was wrong — #119 set ALLOW_EMPTY: "true". Fixed here
2. Seed PR_RISK_CALLERS	Done (out-of-band, see below)
3. README fleet row / AGENTS.md bullet / test-bump-callers.yml both path lists	Already on main via #119. The README row's Seeded column said empty [] allowed, now corrected
4. Post-merge workflow_dispatch	Cannot be done pre-merge — runbook below

Roster seeding (step 2) — done, not in this diff

Seeded as repo config, deliberately not as a commit: this repo is public, so caller names must never appear in a file, a commit message, or PR text. Two entries, {"repo","file","label":""}, matching the GROOM_CALLERS convention.

Verified in both directions, as AGENTS.md requires:

• Roster → reality: both enrolled caller files exist and carry the double pin the rewrite moves (a full 40-hex uses: SHA and a bare workflows_ref:).
• Reality → roster: swept all 200 Comfy-Org repos' .github/workflows/ via the contents API for any pr-risk caller. Exactly two exist, both enrolled. No third caller is missing from the roster, and no roster entry is dead.

I seeded both a variable and a same-named secret. #135 (BE-6472, open) flips every entrypoint from vars.X to secrets.X; its judgment call 1 assumes this roster is unseeded and therefore inert under the flip. With ALLOW_EMPTY gone that assumption would turn a merge-order accident into a red fleet, so the roster now resolves under either binding. Operational consequence worth knowing: there are now two copies, and a future edit to one silently staleness the other. Once #135 merges and the variables are deleted per its runbook, delete PR_RISK_CALLERS the variable too and keep only the secret.

Riskiest line

Removing ALLOW_EMPTY: "true". Safe because of ordering: the roster was seeded and read back before this change, and checked against bump-callers.sh's own jq validator, so there is no window where the fleet hard-fails against a roster that does not exist. If the roster ever does vanish, the new outcome is a loud red run — which is the point.

Verified empirically rather than by reading the script:

empty roster, ALLOW_EMPTY unset (new)  -> exit 1, ::error:: ... is not a non-empty JSON array
empty roster, ALLOW_EMPTY=true  (old)  -> exit 0, "has no callers yet — nothing to bump"

Negative-claim falsification

This diff adds a deny path (exit 0 → exit 1), so the falsification rule applies. The premise being asserted is "an empty pr-risk roster is never legitimate." That is falsifiable by finding zero callers — so I went and looked rather than assuming: the 200-repo org-wide sweep above returned exactly two live callers, both with pins the bumper can rewrite. Had it returned zero, the hard-fail would be wrong and ALLOW_EMPTY should stay. It returned two. The denial is also loud and one-line reversible, not a silent dead-end.

Verification

• bash .github/bump-callers/tests/test_bump_callers.sh — 175 passed, 0 failed
• shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh — clean
• python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings: AGENTS.md 186 lines, no CODEOWNERS — unchanged, AGENTS.md is not touched here)
• All 32 .github/workflows/*.yml parse
• test-bump-callers.yml already lists bump-pr-risk-callers.yml in both path filters, so CI exercises this change on the PR

Judgment calls

1. Not re-adding the entrypoint. The ticket says "add" it; it exists and is better than the groom copy the ticket asked for. Re-creating it would be a no-op diff or a regression.
2. Doc fixes beyond the ticket's three. docs/callers/pr-risk.md told enrollers "This workflow has no roster yet … so there is no vars.*_CALLERS entry to add" — actively wrong now, and it is precisely the "enrolling is two steps" half AGENTS.md calls the most repeated mistake here. Both enrollment roster tables (root README.md, docs/callers/README.md) also omitted PR_RISK_CALLERS. Fixed all three; they are the same registration the ticket's step 3 is about.
3. Left alone: those two tables also omit detect-unreviewed-merge. That fleet's roster is deliberately unseeded and its story is different, so adding it is someone else's call, not a drive-by here.
4. Conflict with ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472) #135 is likely and expected. It edits the same header region of this entrypoint and the same README table. Both diffs are small there; whoever merges second resolves. Nothing here changes the vars. binding — that is ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472) #135's job.

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

1. gh workflow run bump-pr-risk-callers.yml -R Comfy-Org/github-workflows. Landing this touches no watched path, so nothing fires on its own and the stale caller stays stale until this one dispatch.
2. Expected: one bump PR on branch ci/bump-pr-risk. The other caller is already at main's tip (7f7c9bf) and should no-op.
3. Confirm the run masks caller names. Note the fleet-wide known gap still applies until ci(bump-callers): move every caller roster from an Actions variable to a secret (BE-6472) #135 lands: the roster is printed unmasked in the step's env: dump before the script's ::add-mask:: can run.

Warning

Do not merge the generated bump PR as a bare pin bump. That caller predates the off-by-default enabled input from #115 and does not set enabled: true — I confirmed this against its current file. A pin-only bump would move it onto a revision where grading is off by default and silently disable its grading. A sibling ticket from the same spike fixes that caller atomically (pin bump plus enabled: true in one commit); it has not merged yet. Leave the bot's PR open and let that ticket supersede or close it.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bf5f8f64-8119-4eee-8f14-a9255b4360db

📥 Commits

Reviewing files that changed from the base of the PR and between 7f7c9bf and 306c6d5.

📒 Files selected for processing (5)

• .github/bump-callers/README.md
• .github/workflows/bump-pr-risk-callers.yml
• README.md
• docs/callers/README.md
• docs/callers/pr-risk.md

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 6 finding(s).

Severity	Count
🟠 High	2
🟡 Medium	1
🟢 Low	2
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[wei-hai]

Review: LGTM on the change. Two operational notes and one thing about this repo being public.

Removing the silent-success path is right, and the reasoning order is what makes it safe rather than the change itself: seed the roster, read it back, validate it against the script's own checker, then remove the escape hatch. That sequencing means there's no window where the fleet hard-fails against a roster that doesn't exist. Doing the org-wide sweep in both directions — roster entries all resolve to real callers, and no caller exists outside the roster — is the check that actually justifies the hard-fail, since the premise "an empty roster is never legitimate" is only true if you've confirmed callers exist.

Scoping to the genuine remainder rather than re-implementing what already shipped is also the right call. A no-op diff that re-adds an existing entrypoint would be worse than useless.

---

1. The variable/secret duplication needs to outlive this PR description.

Seeding both bindings so the roster resolves regardless of merge order is a good defensive move — with the hard-fail in place, losing that race would take the fleet red rather than just inert. But the consequence you name is real and permanent until someone acts: two copies of the same list, and an edit to one silently staling the other.

Right now that warning lives only in this PR body, which stops being anywhere anyone looks the moment it merges. The person who edits the roster six weeks from now will read the header comment in the workflow, not this thread. Worth putting the "there are two copies; update both; delete the variable once the secrets flip lands" note in the entrypoint header where the update flow is already documented — that's the one place someone editing the roster is guaranteed to be looking.

2. The hard-fail makes the roster's visibility in run logs a guarantee rather than a possibility.

The header is honest that the env binding is dumped before the script can mask it, and that's a pre-existing gap you didn't create. But it's worth noting this PR changes its character: with an empty roster there was nothing to disclose, and now there's a permanently non-empty list printed on every run of a public repo. The mitigation you documented — check before adding a private repo, weigh that a public log entry can't be unpublished — is the right guidance, and putting it in the enrollment doc rather than only the workflow header is where an enroller will actually meet it.

Given the roster is now load-bearing and permanently populated, the masking fix seems worth tracking as real work rather than a standing known-gap comment. It's the kind of thing that stays "documented" indefinitely precisely because it's documented.

3. This repo is public.

The title carries an internal tracker ID, and the description walks through internal rollout sequencing, ticket numbering, and how the fleet is being staged. None of that is secret in the dangerous sense, and the caller names themselves were correctly kept out of the diff — you were clearly thinking about exposure. But the same reasoning that keeps caller names out of a public file applies to internal process detail in public PR text, and it's easy to lose track of which repos are public when several in the same series aren't. Worth a quick check on whether the internal grounding belongs here or in the internal tracker, for this PR and the others in the series.

---

Correcting docs/callers/pr-risk.md was the right call to make here rather than defer. The doc said in plain language that no roster exists and no entry needs adding — that's not just stale after this change, it actively instructs an enroller to skip the step the docs elsewhere call the most-missed one. Fixing the thing your own change falsifies isn't scope creep.

Equally, leaving the other fleet's roster row alone was right. Different rollout, different posture, someone else's call.

The empirical before/after on the two ALLOW_EMPTY states is worth more than the code reading, since the whole change is about which exit code a missing roster produces.