ci(bump-callers): seed the pr-risk roster and hard-fail on an empty one (BE-6480) - #137
ci(bump-callers): seed the pr-risk roster and hard-fail on an empty one (BE-6480)#137mattmillerai wants to merge 2 commits into
Conversation
…ne (BE-6480) The pr-risk fleet entrypoint landed in #119 seeded empty, with ALLOW_EMPTY: "true" so an empty roster exited 0. pr-risk now has two live callers, so an empty roster is a bug rather than a rollout state: without the hard-fail it degrades into a silent clean exit that leaves every caller pinned to a stale grader with nothing red to say so. Roster seeded out-of-band (repo config, not a commit — this repo is public and caller names must never land in a file or a log) with the two callers a 200-repo org sweep found, verified in both directions. Dropping ALLOW_EMPTY after seeding keeps the ordering safe: there is no window where the fleet hard-fails against a roster that does not exist yet. Also corrects the docs that still described pr-risk as fleet-less or seeded-empty: the bump-callers fleet table's Seeded column, the caller setup guide (which told enrollers there was no roster entry to add), and the two enrollment roster tables that omitted PR_RISK_CALLERS.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute 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 (5)
Comment |
There was a problem hiding this comment.
🔍 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.
…ter doc gaps (BE-6480) Addresses the cursor-review panel on #137. - Set `ALLOW_EMPTY: "false"` explicitly rather than leaning on `bump-callers.sh`'s default, matching the auto-label and detect-unreviewed-merge entrypoints, so a future flip of that default cannot silently degrade this fleet back into no-op pin drift. - Fix the header, which claimed there was "deliberately no ALLOW_EMPTY below", and record that every currently-rostered name was already published by another seeded roster's identical env binding. - README: the roster list read as exhaustive but omitted `AUTO_LABEL_CALLERS` and `DETECT_UNREVIEWED_MERGE_CALLERS`; add both and point at the canonical docs/callers table. - docs/callers/README.md: add the missing detect-unreviewed-merge row. - docs/callers/pr-risk.md: enrolment does not backfill a pin (the fleet is path-filtered — ask for the one-time workflow_dispatch), and enrolling a private repo publishes its name via the known run-log gap.
ELI-5
pr-riskis 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
mainand implemented only the genuine remainder. Criterion-by-criterion:bump-pr-risk-callers.ymlexists, correctname/concurrency/paths/workflow_dispatch/env/decommission guard/stale guard/noWIRE_BOT_SCRIPTmainvia #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)ALLOW_EMPTYALLOW_EMPTY: "true". Fixed herePR_RISK_CALLERStest-bump-callers.ymlboth path listsmainvia #119. The README row's Seeded column saidempty [] allowed, now correctedworkflow_dispatchRoster 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 theGROOM_CALLERSconvention.Verified in both directions, as AGENTS.md requires:
uses:SHA and a bareworkflows_ref:).Comfy-Orgrepos'.github/workflows/via the contents API for anypr-riskcaller. 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.Xtosecrets.X; its judgment call 1 assumes this roster is unseeded and therefore inert under the flip. WithALLOW_EMPTYgone 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, deletePR_RISK_CALLERSthe 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 againstbump-callers.sh's ownjqvalidator, 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:
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_EMPTYshould 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 failedshellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh— cleanpython3 .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).github/workflows/*.ymlparsetest-bump-callers.ymlalready listsbump-pr-risk-callers.ymlin both path filters, so CI exercises this change on the PRJudgment calls
docs/callers/pr-risk.mdtold enrollers "This workflow has no roster yet … so there is novars.*_CALLERSentry 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 (rootREADME.md,docs/callers/README.md) also omittedPR_RISK_CALLERS. Fixed all three; they are the same registration the ticket's step 3 is about.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.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)
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.ci/bump-pr-risk. The other caller is already atmain's tip (7f7c9bf) and should no-op.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
enabledinput from #115 and does not setenabled: 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 plusenabled: truein one commit); it has not merged yet. Leave the bot's PR open and let that ticket supersede or close it.