fix(groom): pin ci-groom.yml by SHA and stop the bumper reporting unpinnable callers as converged (BE-6015) - #107
Conversation
… fleet can bump it (BE-6015) BE-4004 (#56) landed the interval gate behind a fail-loud REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA placeholder that was meant to be hand-bumped to that PR's squash SHA. The hand-bump never happened, so every scheduled ci-groom run since has failed at startup on an unresolvable ref (zero jobs, six consecutive schedule failures). The placeholder is not 40-hex, so the bumper's rewrite could not move it either and logged the file as already converged. Pin both refs (uses: and workflows_ref, in lock-step) to bcde90f — the SHA the other eight groom callers already carry — which puts this caller back under bump-groom-callers.yml, where future bumps move it automatically.
…nable caller (BE-6015) A no-op rewrite only means "already at NEW_SHA" when the file carries a 40-hex pin the pass could have moved. A caller that names the reusable but pins it with a placeholder, tag or branch produces a byte-identical no-op, so the skip branch reported it as converged — which is what let a caller that failed every run at startup read as healthy in the bump log. Control flow is unchanged (the entry is still skipped); only the report differs. Silent when the reusable-name probe cannot identify a uses: spelling, so a converged caller never warns spuriously.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 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 caller bumping flow now validates roster entries, repairs supported pin formats, records unpinnable files, and reports aggregate failures after processing the full fleet. Tests and guidance cover sibling-only, empty, dynamic, and missing references. ChangesCaller roster validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
agent-coded
![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 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
| ⚪ Nit | 2 |
Panel: 8/8 reviewers contributed findings.
… the run (BE-6015) Review follow-ups on the BE-6015 fix. The pin check now runs against the ORIGINAL file content and independently of whether the rewrite changed anything, so it also catches the case where an unmovable `uses:` sits beside a stale-but-movable `workflows_ref:` — the rewrite changes bytes there, the file is staged, and the bump PR would have shipped with the unresolvable ref still in place and no warning at all. - anchor both probes to `^[^#]*<key>:` so a commented-out old pin or a docs URL cannot vouch for a live pin that is still a placeholder - require a whitespace/EOL boundary after the 40-hex run, so a mutable ref that merely STARTS with 40 hex (`<sha>-wip`) is not read as a commit pin - validate the `workflows_ref:` half too — a `uses:` at NEW_SHA beside a `workflows_ref: main` is the split state a twice-pinning caller prevents - distinguish a stale ROSTER entry (file calls only a sibling reusable) from an unpinned file, and blame the variable rather than telling a human to hand-pin a workflow the file does not use - accumulate offenders and exit non-zero with an aggregate `::error::` after the whole fleet is processed: the fan-out still completes, but a run containing a caller the bumper cannot keep current no longer reports success ci-groom.yml: correct the now-stale `permissions:` ordering note (the pin is at main's tip, i.e. at/after the BE-4214 CLI migration, so the shorter grant matches what that groom.yml declares; only a BACKWARDS move is unsafe). Verified `Comfy-Org/github-workflows` IS present in `vars.GROOM_CALLERS` (`.github/workflows/ci-groom.yml`), so the caller is genuinely back under the fleet and the pin will move on the next bump.
|
Parked during the merge-backlog sweep — this needs a design decision, not a rebase. Attempting to merge current BE-4662 rewrote the pin substitution to target the pin token rather than "a 40-hex on a line mentioning github-workflows", so any literal ref shape now moves — full SHA, short SHA, tag, or a That invalidates this PR's central premise. The pin-integrity check here warns "the bumper cannot move it; pin it by full SHA by hand" for precisely the three shapes the bumper now self-heals. #79's tests that this PR breaks (all expected
This PR's own tests that are now factually wrong:
Also worth noting: the two other things this PR does are already on
What still looks worth salvaging, and the reason this is parked rather than closed:
Both are separable from the obsolete No changes were pushed to this branch — it is exactly as it was. |
|
Parked — needs re-authoring, not conflict resolution. Left untouched (head still I attempted this one properly and the test suite settled it. Three findings:
What genuinely survives and is still worth landing: the one case BE-4662 cannot see — a roster entry pointing at a file that names no Suggested shape: drop commits 1–2, keep a single commit implementing only the no- Nothing was pushed; branch is byte-identical to before I started. |
Resolves a semantic conflict with #79 (BE-4662), which anchored the bump-callers rewrite to the pin TOKEN rather than 40-hex-ness after this branch was opened — so a placeholder/tag/short-SHA uses:/workflows_ref pin is now self-healed on the next bump instead of needing a human. Drop this branch's two now-redundant/contradicting pre-rewrite pin-shape checks (uses: not-40-hex, workflows_ref not-40-hex) and keep only the roster-entry check (a caller registered in vars.*_CALLERS whose file names no uses: of this fleet's reusable at all), which BE-4662 does not cover. ci-groom.yml's own pin is superseded by main (#102, then further bumps) so that file now matches main byte-for-byte. Delete the five BE-6015 tests whose expectations BE-4662 invalidated, keep the two (roster-entry, no-github-workflows-uses) that still hold, and correct the AGENTS.md/README prose that claimed a placeholder/tag/branch pin is un-bumpable — it no longer is.
…eals pin shape (BE-6015) The merge with main pulled in BE-4662, which anchors the caller rewrite to the pin token rather than 40-hex-ness — a placeholder/tag/branch/short-SHA pin now self-heals on the next bump instead of needing a human. Update the AGENTS.md and bump-callers README prose that still claimed those shapes are un-bumpable; the only case that remains genuinely un-bumpable is a roster entry naming no uses: of this fleet's reusable at all.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bump-callers.sh:
- Around line 419-437: After appending a failed entry to UNPINNABLE in the
caller-processing flow, immediately skip further handling of that roster entry
so it cannot be added to PEND_CONTENT or staged. Update the wrongfleet fixture
to include workflows_ref: and verify the scenario creates neither a commit nor a
PR, while preserving processing of other entries.
In `@AGENTS.md`:
- Around line 156-159: Update the roster-failure description in AGENTS.md to
state that the script fails only when it parses a github-workflows reusable call
whose sibling workflow differs from WORKFLOW_FILE. Remove the claim that a
roster entry with no fleet uses: causes the run to fail, while preserving the
distinction between an invalid roster entry and a movable pin.
🪄 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: 6a9dabf3-bbd3-40db-8627-5976a7172128
📒 Files selected for processing (4)
.github/bump-callers/README.md.github/bump-callers/bump-callers.sh.github/bump-callers/tests/test_bump_callers.shAGENTS.md
…ws_ref (BE-6015) CodeRabbit review: the roster-entry check flagged PIN_OK=0 for a file that calls only a sibling fleet's reusable but did not stop processing, so rule 2's deliberately-unaddressed workflows_ref: rewrite (it can't distinguish 'ours' from a sibling job's with: input) still fired and staged the sibling's asset ref at this fleet's SHA. continue immediately after recording the failure so a bad roster entry can no longer corrupt a sibling's config while it's being reported. Extends the wrongfleet fixture with a workflows_ref: line so the suite proves it (red without the fix: 'committed nothing' / 'opened no PR' both fail). Also tightens the AGENTS.md roster-failure description: the run fails only when a github-workflows reusable IS parsed and it's a sibling's, not on every file with no fleet uses: at all (that case stays a silent skip).
2 participants
ELI-5
The groom workflow lives here, and other repos "call" it by pointing at a specific commit of this repo. A robot keeps those pointers up to date, but it only knows about repos listed in a config list. Four repos were calling groom without being on that list, so their pointers had gone stale — and this repo's own caller had a leftover
REPLACE_MEplaceholder instead of a real pointer, so it had been crashing on startup every single day while the robot cheerfully logged it as "already up to date." This fixes our own pointer, and makes the robot say "I can't move this one" instead of pretending it's fine.What this does
Two changes, both from the same root cause — a caller that is registered with the fleet but that the fleet cannot actually move.
1.
ci-groom.ymlis pinned by a real SHA. BE-4004 (#56) landed the interval gate behind a deliberate fail-loudREPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHAplaceholder in bothuses:andworkflows_ref:, to be hand-bumped to that PR's squash SHA after merge. The hand-bump never happened. Every scheduledci-groomrun since has failed at startup on an unresolvable ref — six consecutive schedule failures, each with zero jobs. Both refs now pinbcde90f4, the same SHA the other groom callers carry, which puts this caller back underbump-groom-callers.ymlso future bumps move it automatically.2.
bump-callers.shno longer reports an unpinnable caller as converged. The skip branch treats a no-op rewrite as "already atNEW_SHA", but a no-op rewrite only means that when the file carries a 40-hex pin the pass could have moved. A caller that names the reusable and pins it with a placeholder, tag or branch produces a byte-identical no-op — so the log saidalready at <short> — skippingfor a file that had been crashing for weeks. It now emits a::warning::naming the file instead. Control flow is unchanged: the entry is still skipped, still a warning rather than a failure, so one unpinnable caller cannot abort the fan-out. It stays silent when the reusable-name probe (GW_USES, the same probe the rewrite address uses) cannot identify auses:spelling, so a converged caller never warns spuriously.Out-of-repo work this PR completes (no commit — it is an Actions variable)
The bulk of BE-6015 is a
GROOM_CALLERSedit, already applied and verified: four live callers that were never registered are now on the roster, and the stale pre-rename repo name was corrected in the same edit. Dispatchingbump-groom-callers.ymlthen opened bump PRs against exactly those four, each movinguses:andworkflows_ref:in lock-step and touching nothing else. An independent sweep of all 196Comfy-Orgrepos confirms nine live groom callers exist and all nine are now registered — no tenth was missed. Consumer names are deliberately not listed here; this repo is public and most callers are private.Merging this PR is what gets the ninth caller onto the same SHA as the other eight.
Verification
shellcheck -xclean;bash .github/bump-callers/tests/test_bump_callers.sh→ 128 passed, 0 failed; the cursor-review (40), agents-md-integrity (18) and groom (170) unittest suites all pass;check_agents_md.py --root .passes (its one warning, no CODEOWNERS, is pre-existing onmain). The new bump-callers test is a genuine red→green regression test: reverting onlybump-callers.shdrops it to 126 passed / 2 failed.Judgment calls and things a reviewer should weigh
main. Flagging rather than assuming: if you would rather split thebump-callers.shwarning into its own PR, it is the second commit and drops cleanly.groom.ymlatbcde90f4; both secrets it forwards resolve (ANTHROPIC_API_KEYfrom the org,CLOUD_CODE_BOT_PRIVATE_KEYfrom the repo);builderis unset so it defaults tofalse— issues only, no auto-PRs; andinterval_daysfalls back to 7, so weekly. Reasonable toworkflow_dispatchit once withdry_run: trueafter merge before letting the schedule take it.@v1fails the consumer-side pin validation — but it is a behavior change visible in every fleet's run summary, not just groom's..supervisor-state/BE-6015-groom-callers/{before,after}.json.CURSOR_REVIEW_CALLERSandAGENTS_MD_CALLERSstill carry the same stale pre-rename repo name. BE-5921 owns that class across every*_CALLERSvariable, and the ticket says to fix it in one place, not two.env:block prints before masking) now exposes four more names. Pre-existing and fleet-wide, but this edit widened it.