ci(bump-callers): add detect-unreviewed-merge caller fleet (BE-6294) - #117
Conversation
detect-unreviewed-merge.yml was the one reusable with live callers and no bump fleet, so its pins only ever moved by hand and consumers drifted behind. Add the missing thin entrypoint over the shared bump-callers.sh, copied from the pr-size template and reparameterised. The reusable loads nothing at run time (no workflows_ref: input, no script dir), so the push path filter and the decommission guard each have a single entry, unlike pr-size. ALLOW_EMPTY is deliberately unset (defaults to false): this fleet always has callers, so an empty roster must hard-fail rather than silently no-op. The roster variable is seeded separately, so runs in that window fail red on purpose. Registers the entrypoint in test-bump-callers.yml's path filters, adds the fleet row to the bump-callers README (and drops detect-unreviewed-merge from 'reusables with no fleet'), and updates AGENTS.md's catalog bullet + fleet-variable list. Originating spike: BE-6293.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 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 (3)
📝 WalkthroughWalkthroughThe PR adds a caller fleet and bump workflow for ChangesDetect unreviewed merge caller fleet
Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant GitHubAPI
participant SharedCallerBumper
participant CallerRepositories
GitHubActions->>GitHubAPI: Validate refs/heads/main and watched workflow blobs
GitHubAPI-->>GitHubActions: Return verified SHA and workflow state
GitHubActions->>SharedCallerBumper: Pass verified SHA and runtime caller roster
SharedCallerBumper->>CallerRepositories: Update pinned caller references
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 4 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
Panel: 8/8 reviewers contributed findings.
…294) Address the cursor-review panel on #117 by mirroring the four guards the sibling bump-auto-label-callers.yml already carries: - Staleness guard compared the bare main tip against github.sha, so any unrelated commit landing on main between the trigger and the check discarded this fleet's only bump while CI reported success (the push trigger is path-filtered, so that commit queues no replacement run). Compare the watched file's blob at the tip instead. - timeout-minutes: 20 — the job inherited the 6-hour default, and with cancel-in-progress: false a hung git/gh call blocked every queued bump behind it. - Narrow the app token to contents/pull-requests/issues write instead of every permission the app holds org-wide. (owner: stays unnarrowed — the roster is a runtime variable, and naming repos here would leak private caller names into a public file.) - Correct the header: it claimed every repo name is masked before it is ever echoed, but binding CALLERS_JSON via the step env means Actions dumps the raw roster before the script's ::add-mask:: runs. Document it as the known fleet-wide gap, as the sibling entrypoints do.
mattmillerai
added
cursor-review
|
Addressed all four panel findings in 577b1c8 by mirroring the guards the sibling |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
… caller-pattern doc (BE-6294) Round-2 cursor-review findings on the new fleet: - The reusable's documented caller pattern pointed at `Comfy-Org/unreviewed-merges/.github/workflows/detector.yml@v1` — a repo, path and floating tag that no live caller has used since the workflow moved into github-workflows. `bump-callers.sh` only rewrites a `Comfy-Org/github-workflows/...` line carrying a 40-hex SHA, so anyone wiring a caller from that template would have been skipped as "already pinned" and drifted forever. Corrected to the real shape (`@<sha> # v1`, matching the other reusables' headers) with the bumper's contract spelled out. - Seeded `DETECT_UNREVIEWED_MERGE_CALLERS` with the 12 live callers, so the fleet is enrolled in both directions rather than hard-failing until an operator noticed. Every one of them pins a full 40-hex SHA against this repo's path, i.e. exactly what the shared rewrite moves. - `ALLOW_EMPTY: "false"` set explicitly (matching bump-auto-label-callers.yml) so the hard-fail-on-empty-roster guarantee no longer rests on another file's default. - Deletion guard tests "$WATCHED" instead of a second copy of the literal path, which would have gone stale on a rename and made the fleet a silent no-op. - `git ls-remote --refs` + an exact-refname match, so a branch named `foo/refs/heads/main` cannot be the line the tip parse consumes. Because this touches the watched path, merging fires the new fleet's first run end to end instead of leaving it inert.
|
Round-2 review resolution — all 8 open threads addressed in 1d81fe9 (5 code fixes, 1 deferred to a fleet-wide follow-up, 2 answered with reasoning). Two things a reviewer should know before pressing merge:
Also fixed: Deferred, with a follow-up recorded for filing: the Local gates green: shellcheck, |
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…ot leaking (BE-6294) Round-3 review reversed a call I made in round 2, correctly. Seeding DETECT_UNREVIEWED_MERGE_CALLERS closed the enrollment gap, but combined with this PR touching the watched path it guaranteed a first-run disclosure: every roster reaches bump-callers.sh through the step's `env:` block, and Actions prints that block before the script's `::add-mask::` runs. Verified against a real public log (run 30737929800 prints GROOM_CALLERS verbatim), so this is live behaviour, not theory. Six fleets already publish their rosters that way, so those names are out. Two of this fleet's callers are non-public repos that appear in NO already-seeded roster — seeding would have published two names that are not out yet, and a public log entry cannot be unpublished. A red run can. So the variable is unset again and the fleet ships deliberately unseeded; the resulting hard-fail is the intentional nag ALLOW_EMPTY already documents. Seeding is the follow-on to the masking fix, not a way to turn that run green. Header, bump-callers README and AGENTS.md all say so plainly instead of claiming the gap is closed. Also from round 3: - On a stale-but-blob-identical re-run, pin callers to the verified `main_tip` rather than this run's older `github.sha`: the guard has just proved the reusable is byte-identical at both, so the tip is the same content at a commit that is actually current. Stops a land-then-revert re-run from re-pinning every caller backwards. - Both decommission exits use `::warning::` instead of a bare echo — a deleted reusable that still has live callers hard-fails all of them at startup, and a silently green run here was the fleet's only chance to say so.
Round 3 — I reversed my own round-2 call, and merging this now lands a deliberately RED workflowBoth of those need to be visible before anyone presses merge, so, plainly: 1. Seeding the roster was my mistake, and it is reverted. In round 2 I seeded Then I quantified it. Six fleets are already seeded and already publish their rosters, so those names are out. Intersecting this fleets 12 callers against every already-seeded roster leaves two non-public repos that appear in no existing roster. Seeding would have published two names that are not currently exposed. A public log entry cannot be unpublished; a red run can. The variable is unset again. 2. So this PR merges into a fleet that hard-fails until a follow-up lands. That is the designed behaviour ( If you would rather not land a knowingly-red workflow, the alternative is to hold this PR until the masking follow-up merges and seed both together. I have no strong objection to that; I just did not want to make the "publish two private repo names" trade silently in either direction. Also fixed this round
Answered with evidence, not changed
Recorded for follow-up (3)
Local gates green: shellcheck (including the inline guard, extracted and linted manually), |
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…-6294) Round-4 review caught that the previous commit left two paragraphs giving opposite instructions. The ALLOW_EMPTY block still read "this fleet is seeded NON-EMPTY" and told an operator an empty roster means the variable was clobbered and should be RESTORED — while the block above it says the fleet ships deliberately unseeded until the run-log masking fix lands. An operator triaging the first red run and following the lower block would seed the variable and cause exactly the irreversible disclosure the header exists to prevent. Rewritten so the empty roster reads as the expected pre-seed state, with the "clobbered, restore it" reading explicitly scoped to after seeding. Also: - Verify FETCH_HEAD resolves to a commit before reading blobs out of it. `git rev-parse --verify --quiet` returns empty both for "path absent from this tree" and "revision unresolvable", so a partial fetch was indistinguishable from a deletion and would exit 0 as "decommissioned" — the same "a lookup we couldn't perform is not evidence" anti-pattern the ls-remote guard above already rejects, but silently no-opping the fleet. - An absent $WATCHED at this run's own commit now reports as a decommission rather than falling into the "changed since" branch and being logged as a stale re-run. - Document that re-pointing NEW_SHA to main_tip is sound only because the paths: filter has exactly one entry, so whoever widens that filter knows the single-blob comparison has to widen with it.
Round 4 — and where this PR now standsAll 28 review threads across four rounds are resolved. The round-4 panel caught one thing that mattered more than the rest: The header was contradicting itself, in the dangerous direction. When I reversed the seeding decision in round 3 I rewrote the paragraph explaining why the roster ships unseeded — and left the Also fixed this round: The decision this PR needs from a humanIt is not a code question. Merging lands a workflow that is red by design until a follow-up merges, because the roster ships unseeded to avoid publishing two non-public repo names in this public repos logs. Two defensible options:
I have no strong preference. I flagged it rather than picking silently because the "publish two private repo names" side of that trade is irreversible. Follow-ups recorded (3)
Verificationshellcheck (including the inline One caveat worth stating: 9b93a60 has not itself been through a review panel — rounds 1–4 reviewed up to 72d5d79. Its changes are the header rewrite plus two defensive guards. Re-apply the |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/bump-callers/README.md:
- Line 34: Update the “Why the detect-unreviewed-merge roster is not seeded yet”
heading in the README from level 4 (`####`) to level 3 (`###`) so it follows the
surrounding level-2 heading hierarchy and satisfies MD001.
In @.github/workflows/bump-detect-unreviewed-merge-callers.yml:
- Around line 108-114: Update the uses entries in the Checkout and Generate
Cloud Code Bot token steps to end with the required trailing # v1 comment while
retaining their full SHA pins. Move the existing version-specific rationale to
preceding comment lines so the required trailing format remains intact.
- Around line 81-90: The workflow’s concurrency group can replace a newer
pending push run with an older manual re-run, preventing the current SHA from
updating callers. Add a non-concurrent preflight job that determines whether the
triggering run is still current, expose that result as an output, and gate the
serialized bump job on it; preserve the existing bump logic only for
confirmed-current runs.
- Around line 143-145: Update the explanatory comment near the pre-seed roster
validation to state that the hard failure is expected until masking and roster
seeding are complete, and that the workflow must remain red rather than
proceeding. Clarify that operators should not seed
DETECT_UNREVIEWED_MERGE_CALLERS before the masking fix, preserving the existing
failure behavior and preventing caller names from reaching the public step
environment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0819f468-cebf-425d-84ad-62fd699a261c
📒 Files selected for processing (5)
.github/bump-callers/README.md.github/workflows/bump-detect-unreviewed-merge-callers.yml.github/workflows/detect-unreviewed-merge.yml.github/workflows/test-bump-callers.ymlAGENTS.md
…d header (BE-6294) The round-4 header rewrite scoped the 'empty roster means clobbered, restore it' reading to after seeding, but the ALLOW_EMPTY comment 90 lines down still carried the old, unscoped version — the same trap in a second place. An operator triaging the first red run from that comment would seed the variable and cause the irreversible disclosure the header exists to prevent. Also: fix the README heading level (MD001, h2 -> h4), and document the concurrency edge where a manual re-run of a superseded run evicts a newer pending one. That edge is shared verbatim by all seven bump-* entrypoints and is self-healing plus dispatch-recoverable, so it is noted rather than forked into one of seven copies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 5 — the round-4 fix was one place shortCodeRabbit re-reviewed 9b93a60 and posted 4 findings. All 4 threads are resolved (32 across five rounds); 2 fixed in a8ef9a8, 2 answered. The one that mattered was a repeat of round 4's own bug. Round 4 rewrote the header so an empty roster reads as the expected pre-seed state rather than a clobbered variable — but the Also fixed: README heading level (h2 → h4, MD001). Answered, not changed
Verificationshellcheck (script, tests, and the inline Unchanged: the decision this PR still needs from a humanMerging lands a workflow that is red by design until the masking follow-up (BE-6466) merges and the roster is seeded, because seeding now would publish two non-public repo names in this public repo's logs. Merge now (fleet wired and correct, ~12 pins keep moving by hand as they do today) or hold and land both together. a8ef9a8 is comment/doc-only — no logic changed this round. |
…nreviewed-merge-fleet # Conflicts: # .github/workflows/detect-unreviewed-merge.yml # .github/workflows/test-bump-callers.yml # AGENTS.md
|
Merged The one judgement worth calling out: Verified: both workflow files parse as YAML; |
2 participants
ELI-5
Every reusable workflow in this repo has a little robot whose job is: "when I change, go open a PR in each repo that uses me so their pinned SHA moves forward."
detect-unreviewed-merge.yml— the SOC 2 workflow that flags PRs merged without approval — never got one, so its ~12 consumers were pinned by hand and quietly drifted onto an old commit. This adds that robot.What changed
.github/workflows/bump-detect-unreviewed-merge-callers.yml— a thin entrypoint over the shared.github/bump-callers/bump-callers.sh, copied frombump-pr-size-callers.yml(the cleanest current template: it carries both the stale-run guard and the decommission check) and reparameterised:VAR_NAME: DETECT_UNREVIEWED_MERGE_CALLERS,TAG: detect-unreviewed-merge,WORKFLOW_FILE: detect-unreviewed-merge.yml. Every SHA-pinned action is preserved verbatim (actions/checkoutv7.0.1,actions/create-github-app-tokenv1.12.0 with its dependabot-ignore comment).test-bump-callers.yml— the new entrypoint added to both thepull_requestandpushpath filters..github/bump-callers/README.md— fleet row added;detect-unreviewed-merge.ymlremoved from "Reusables with no fleet", which now lists only the genuinely caller-less reusables.AGENTS.md— the wrapper added to the catalog bullet (replacing the "known, deferred gap … pins move by hand" clause), andDETECT_UNREVIEWED_MERGE_CALLERSadded to the fleet-variable list. Still 145 lines, inside the standard's 200-line gate.Two deliberate differences from the pr-size template
Single-entry path filter and decommission check.
detect-unreviewed-merge.ymlloads nothing at run time — noworkflows_ref:input, no prompt/script directory (grepforworkflows_ref/scripts/in it returns nothing). So the workflow file is the entire surface a caller pins: the push filter lists only.github/workflows/detect-unreviewed-merge.yml, and the guard drops pr-size's second-d scripts/check-pr-sizecondition.ALLOW_EMPTYis deliberately not set, so it defaults tofalseand an empty/missing roster is a hard error (bump-callers.sh:69-83). This fleet has a known, non-empty caller set — per that script's own contract, a fleet that always has callers must never silently no-op, because that leaves every consumer un-bumped with nobody noticing. Roster seeding is a separate human step, so a run in the window before the variable exists will fail red on purpose: that is intentional nagging, not a bug, and the header comment says so explicitly to stop a future reader "fixing" it by flippingALLOW_EMPTYon. The failure is inert — the script exits before any caller-repo write, so nothing can be mis-pinned.Verification
shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh— clean.bash .github/bump-callers/tests/test_bump_callers.sh— 123 passed, 0 failed (no script changes expected or made).actionlinton the new workflow and ontest-bump-callers.yml— clean.python3 .github/agents-md-integrity/check_agents_md.py --root .— passed (the one warning, no CODEOWNERS, is pre-existing onmain).on:/concurrency:/ stepenv:blocks match the spec above.Judgment calls and follow-ups
… (BE-6293); BE-6293 is the spike that found this gap, so I used(BE-6294)— the implementation ticket — in the title and commit so the Linear autolink points at the work being reviewed, and named BE-6293 in the commit body as the origin.test-bump-callers.yml's header comment enumerates the fleets the shared script drives; I added this one to that sentence rather than leave the list stale next to my own edit.docs/callers/**is untouched (it does not exist onmainyet), and no caller repo name appears anywhere in this diff.DETECT_UNREVIEWED_MERGE_CALLERS, thenworkflow_dispatchthe new entrypoint once — landing a fleet touches no path filter, so its own merge fires no run and existing stale pins stay stale until the reusable next changes. When seeding, note that this repo's ownci-detect-unreviewed-merge.ymlis a caller and belongs in the roster: omitting a repo's own caller is the exact trap AGENTS.md records the groom fleet falling into withci-groom.yml.STOPstring, no new throw/deny dead-end, no test flipped to assert one. The twoexit 0paths (stale run, decommissioned workflow) are copied verbatim from the template, and the hard-fail-on-empty is the shared script's pre-existing documented default rather than a new dead-end.