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: 30 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 (5)
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.
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.