fix(bump-callers): refresh the already-converted main (<short>) pin marker (BE-4523)#75
fix(bump-callers): refresh the already-converted main (<short>) pin marker (BE-4523)#75mattmillerai wants to merge 2 commits into
main (<short>) pin marker (BE-4523)#75Conversation
… marker (BE-4523) The marker rewrite only handled the legacy `# github-workflows#NN` form, so once a caller had been migrated to `# github-workflows main (<short>)` every later bump advanced the real 40-hex pin and left the human-readable annotation frozen at the SHA the file no longer uses. Add a rule for the converted form, unanchored from `# ` so it also catches the prose spelling callers put above the `uses:` line. Guard it on the file pinning exactly one github-workflows reusable — the one being bumped — so a marker belonging to a sibling fleet's pin is never stamped with this fleet's SHA. Covered by two new functional cases: a converted marker is refreshed (both spellings), and a second reusable's marker in the same file is left untouched.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 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 (2)
✨ 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 5 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
Panel: 8/8 reviewers contributed findings.
…reusable caller (BE-4523) Review follow-ups on the marker-refresh change: - The 40-hex substitution was addressed at any `/github-workflows/` line, so in a file that also calls a SIBLING github-workflows reusable it repinned that sibling's `uses: …/other.yml@<40-hex>` to THIS fleet's SHA — pointing that caller at a commit its own fleet never shipped. The attribution guard spared the sibling's comment while the real pin was still clobbered. When a sibling reusable is present the address is now tightened to our own reusable's path. The legacy `# github-workflows#NN` rewrite moves under the same guard, since it is unattributed for exactly the same reason. - The multi-reusable detection scanned the whole file, so a prose comment, a docs URL, or a commented-out `uses:` naming a sibling workflow misread a single-reusable caller as multi-reusable — suppressing the marker refresh and leaving the annotation stale, i.e. re-creating the bug this fixes. It now reads `uses:` lines only. - The multi-reusable test asserted only that the sibling's marker survived, not its actual pin, so the clobber above stayed green. It now asserts the pin, the sibling's legacy marker, and that our own pin still bumps; plus a new case for the comment-only mention. Verified load-bearing: reverting the script alone turns 9 assertions red. Also fixed a dead `! grep` assertion whose pattern no longer existed in any message. `workflows_ref:` deliberately stays broad — it is a `with:` input carrying no workflow name, so attributing it needs YAML structure a line-wise sed lacks, and leaving ours un-bumped is the worse failure. Documented in place. Falsified against live caller data rather than assumed: all four caller variables resolved and every caller file fetched at its default branch. 22 of 23 entries exist (the 23rd is a genuine 404, already a per-file skip); all 22 call exactly one github-workflows reusable, identical under whole-file and `uses:`-only detection, so both changes are byte-for-byte no-ops for every caller today — including the one legacy-marker caller, which still converts. Suite: 107 passed / 0 failed; `shellcheck -x` clean.
1 participant
ELI-5
Each caller repo's workflow file pins our shared reusable workflow twice: once as the machine-readable 40-character commit SHA, and once as a human-readable comment that says which commit that is —
# github-workflows main (dc65b8b). The auto-bumper knew how to update the SHA, and knew how to convert the old comment style (# github-workflows#27) into the new one — but it never learned to update the new one again. So the moment a caller had been converted, its comment froze forever while the real pin kept moving, and the comment started lying about which commit the file uses. This teaches the bumper to keep that comment in step with the pin.What changed
.github/bump-callers/bump-callers.shgains a third rewrite rule alongside the existing two, plus a guard:github-workflows main (<hex>)→github-workflows main (${SHORT}),{7,40}so both the short and full-SHA spellings are covered.#. Callers carry the same annotation in two spellings: the trailing comment on theuses:line, and a prose comment above it (# Pinned to github-workflows main (dc65b8b). The bump-… workflow …). The ticket's proposed pattern was anchored to# github-workflows main (…), which matches only the trailing form; dropping the anchor catches both, which is what the hand-fix on the symptom PR actually had to do.uses:is the one this fleet is bumping; otherwise it leaves every marker as found and emits a::warning::. That is deliberately more conservative than the ticket's "rewrite the one adjacent to$WORKFLOW_FILE" — see the judgment call below.main (<hex>)marker, so agents-md-style# v1callers and marker-less callers see no new warning and no behavior change at all.The legacy
# github-workflows#[0-9]+rule and the 40-hex substitution are untouched (their existing test coverage still passes unchanged)..github/bump-callers/tests/test_bump_callers.shgains the two cases the ticket asked for. Suite is 100 passed / 0 failed;shellcheck -xclean on both files. Verified the tests are load-bearing: reverting onlybump-callers.shturns them red (96 passed / 4 failed), and all four failures are the new assertions.Evidence it fixes the reported symptom
Ran the exact rewrite pipeline over
Comfy-Org/Comfy-iOS's real.github/workflows/ci-cursor-review.ymlatorigin/mainwithNEW_SHA=377f7e18605615508898095cfa9ac65173aae2f9. Output is byte-for-byte the diff that PR had to be hand-fixed into — all four lines move together (prose marker,uses:SHA,uses:trailing marker,workflows_ref), where the bumper previously produced only the last three.Falsification of the guard's premise
The guard is only safe if no real caller pins more than one github-workflows reusable from one file — otherwise it would silently suppress marker updates fleet-wide, i.e. quietly restore today's bug. I did not assume this; I checked it. Fetching all four caller variables and resolving every caller file live: 22 of 23 caller entries resolve to exactly one github-workflows reusable, matching their own fleet's
WORKFLOW_FILE— so the guard passes for every live caller and never blocks a bump. 18 of those files carry at least onemain (<short>)marker that is stale today (two carry the Comfy-iOS-style prose + trailing pair). One caller file was unreadable with this token (permissions), so it is unverified rather than confirmed; worst case there is the conservative branch — markers left alone plus a warning, never a wrong rewrite. Caller repo names are deliberately omitted here: this repo is public and the caller list is masked out of its run logs.Judgment calls
uses:-proximity matching. The ticket suggested scoping the marker rewrite to the marker on/adjacent to theuses:line for$WORKFLOW_FILE. I did not do that, for two reasons. First, it does not actually work for the reported symptom: the stale prose marker in Comfy-iOS sits four lines above theuses:line it annotates, so any proximity rule tight enough to be safe is too tight to fix the bug that was filed. Second, proximity would produce a worse multi-reusable outcome than refusal — the pre-existing 40-hex rule would still bump the sibling's SHA while proximity kept its marker at the old value, which is the exact "marker disagrees with the pin" defect this ticket exists to kill, just relocated. Refusing to rewrite anything in an ambiguous file and saying so in the log is the honest failure mode./github-workflows|workflows_ref/ s/[0-9a-f]{40}/…/grule already clobbers a sibling reusable's pin to this fleet's SHA. That is a separate, older bug; no caller triggers it today (see falsification above) and narrowing that rule would change long-standing bump behavior, so I left it and am flagging it here rather than folding it in.Unmet criteria
None. Both requested test cases are present, the legacy rule is preserved, and the scope concern is addressed (differently, and explained above).
Test plan
bash .github/bump-callers/tests/test_bump_callers.sh→ 100 passed, 0 failedshellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh→ clean.github/workflows/test-bump-callers.ymlon this PR (its path filter covers.github/bump-callers/**).