ci(bump-callers): add preflight.sh — one tested staleness/decommission guard (BE-6475) - #136
ci(bump-callers): add preflight.sh — one tested staleness/decommission guard (BE-6475)#136mattmillerai wants to merge 2 commits into
Conversation
…n guard (BE-6475) Every bump-*-callers.yml entrypoint carries an inline copy of the staleness/decommission guard that runs ahead of bump-callers.sh, and the eight copies have drifted: five skip on a bare tip mismatch (throwing away the only run for a change), one compares content but never re-points the pin at the verified tip, one is the hardened #117 version, and pr-risk has grown a different hardening again. Extract the #117 guard as .github/bump-callers/preflight.sh, generalized with an optional WATCHED_ASSETS input so a multi-path fleet compares the asset directory too — which is exactly what the "COUPLED TO THE PATH FILTER" note in #117 requires before its re-point can be reused. Emits proceed/new_sha as step outputs (never $GITHUB_ENV, which a step-level env: NEW_SHA: would silently override). Phase 1 of 2: script + tests + docs only. No entrypoint is swapped over yet, so no fleet behaviour changes and no bump fleet fires on this merge.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 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 |
![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 | 5 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…sion verdict (BE-6475) Cursor review panel findings on the phase-1 extraction. Behaviour-affecting fixes, all with functional coverage in tests/test_preflight.sh: - Decommission at the tip is now EITHER watched surface being gone, not both. Retirement is normally staged (delete the reusable, clean up its asset directory later), so the `&&` let the common case fall through to the "stale run/re-run" branch and exit green — suppressing the ::warning:: that is the fleet's only signal that live callers now hard-fail at startup. It also disagreed with the local -f/-d guards, which already used OR semantics. The message now names the surface that went away. - NEW_SHA is validated as a full 40-hex lowercase SHA. It is the one value never derived from a lookup, yet it is emitted verbatim into $GITHUB_OUTPUT and handed to bump-callers.sh's pin rewrite: a newline in it injects extra output lines, and an injected `proceed=true` would win over the `proceed=false` this script wrote. - HEAD is asserted equal to $GITHUB_SHA. Every "here" side is read from HEAD while every decision is keyed off GITHUB_SHA; a `ref:` override in the consuming checkout would have the script compare main against itself, so every comparison reads "unchanged" and every stale re-run proceeds. - The tip is fetched as `refs/heads/main`, not the bare name. Refspec resolution consults refs/tags/<name> BEFORE refs/heads/<name>, and this repo routinely creates and force-moves major tags — a tag named `main` would silently become the FETCH_HEAD every comparison and the re-point run against. A tip that advances between the ls-remote and the fetch is now logged as the benign race it is. - WATCHED / WATCHED_ASSETS are rejected when glob-shaped or slash-terminated. The guidance to widen them "to match the fleet's paths: filter" points straight at `.github/groom/**`, which resolves to nothing and would make every comparison verify nothing behind a green run. - rev-parse lookups distinguish "absent from the tree" (exit 1) from "the lookup failed" (any other status); `|| true` collapsed both into a silent decommissioned/stale verdict. - An asset tree already gone at the run's own commit is a decommission, not a "changed since", matching the existing here_blob guard. Docs: the multi-path fleet list was wrong in both directions — pr-size IS multi-path, and agents-md-integrity and pr-risk were missing. Script comment and README now list all five and tell you to read the entrypoint's `paths:` rather than trust the list. Tests: 74 assertions (was 42), including staged-retirement in both orders, the local -f decommission branch, the tag-shadowing fixture, and the three input guards. Verified discriminating by mutation: reverting the OR semantics fails 4 assertions, reverting the branch refspec fails 3. The direction/ancestry check (a fetched tip that is BEHIND HEAD) is not fixed here: `--depth=1` leaves FETCH_HEAD parentless, so `merge-base --is-ancestor` would reject the legitimate re-point. Doing it right needs pr-risk's unshallow probe, which is exactly the phase-2 decision this PR defers by design. Deferred to a follow-up.
|
Addressed the review panel in Behaviour fixes (each with a discriminating functional case):
Docs. The multi-path fleet list was wrong in both directions: Tests. 42 → 74 assertions, 9 → 14 cases: staged retirement in both orders, the local Deferred (1). The direction/ancestry check — no assertion that the fetched tip is ahead of No follow-up filed for the "frozen fleet after a failed newer run" note — every entrypoint already carries Verification (all clean, no |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
2 participants
ELI-5
Every
bump-*-callers.ymlworkflow in this repo has the same ~40 lines of "wait, is this run actually the newest one, and does the thing I'm about to pin still exist?" pasted into it before it calls the shared bump script. There are eight copies and they have quietly drifted apart, so different fleets behave differently on the same situation. This PR lifts the best copy out into one tested file,.github/bump-callers/preflight.sh, and adds a test suite that builds throwaway git repos to prove each branch. Nothing uses it yet — swapping the entrypoints over is a separate change — so this merge changes no fleet's behaviour and fires no bump run.What this adds
.github/bump-callers/preflight.sh— the guard, extracted frombump-detect-unreviewed-merge-callers.yml(PR ci(bump-callers): add detect-unreviewed-merge caller fleet (BE-6294) #117, the most-hardened copy) and generalized. InputsWATCHED(required),WATCHED_ASSETS(optional, for multi-path fleets),NEW_SHA, plus Actions'GITHUB_SHA/GITHUB_OUTPUT. Outputsproceed=true|falseandnew_sha=<sha>on every exit-0 path. Exits non-zero only for a lookup it could not perform..github/bump-callers/tests/test_preflight.sh— 42 assertions across 9 cases, no network: each case builds a local bare repo asoriginplus a clone as the run workspace, drives the real script, and asserts exit code, both outputs, and the presence/absence of::error::/::warning::.test-bump-callers.ymlshellchecks both scripts and both suites and runs the new suite;.github/bump-callers/README.mdgains a "Preflight" section (inputs, outputs, the two-step consumption pattern);AGENTS.mdCommands/Layout updated to match.Why the #117 semantics specifically
The five simple copies skip on a bare tip mismatch. That is wrong: the push trigger is path-filtered, so an unrelated commit landing between the trigger and the check starts no run of its own — skipping there discards the only run for the change and freezes every caller, which is the exact pin-drift this directory exists to prevent.
bump-auto-label-callers.ymlcompares content but then pins the stalegithub.sharather than the verified tip. The #117 copy gets both right, plus the exact-refname tip parse (a branch literally namedfoo/refs/heads/mainmatches thels-remotepattern at component boundaries and must not be consumed), theFETCH_HEADverification (an unresolvableFETCH_HEADmust not be read as deletion), and the$WATCHED-variable deletion test (two literals drift on a rename and turn the fleet into a permanent silent no-op).The one generalization is
WATCHED_ASSETS: #117's own comment says its single-blob comparison is coupled to the path filter and must be widened before the re-point is reused by a fleet whosepaths:has more than one entry (cursor-review,groom,pr-size). Test case 6 is that case — a commit touching only the asset directory must read as stale, and the same fixture run withoutWATCHED_ASSETSproceeds, which is the under-verification the widening removes.Test cases
refs/heads/foo/refs/heads/mainsorting first is not consumed; run proceeds at the real tip, no "main moved" log::error::,proceednottrueproceed=false, stale logproceed=true,new_sha== the new tip (the re-point)proceed=false,::warning::decommissionproceed=false— and the single-path config on the same fixture proceeds::warning::own-commit decommission, not reported as staleproceed=true,new_sha==GITHUB_SHA, no fetch/compare-d "$WATCHED_ASSETS"guard firesVerification
git statusconfirms nobump-*-callers.ymlfile is touched.Judgment calls worth a reviewer's attention
bump-pr-risk-callers.ymlhas diverged past ci(bump-callers): add detect-unreviewed-merge caller fleet (BE-6294) #117 and this script does not implement its extra checks. Since ci(bump-callers): add detect-unreviewed-merge caller fleet (BE-6294) #117 landed, the pr-risk entrypoint grew agit rev-list --count HEAD..FETCH_HEAD -- "${WATCHED[@]}"test ("did a later commit touch a watched path", which survives a land-then-revert that nets to zero content diff), an--unshallowprobe that test needs, path-filter exclusions (:(exclude)scripts/pr-risk/tests), and agit merge-base --is-ancestorrefusal to pin an orphaned commit. The ticket is explicit that the extraction adopts the ci(bump-callers): add detect-unreviewed-merge caller fleet (BE-6294) #117 semantics, and this PR does exactly that — but that means the phase-2 swap must decide whether to fold pr-risk's checks in or leave that fleet on its own guard. Deleting them in the name of consolidation would be a regression. Flagged in the script header, the README, and here.mainis supported as the default branch, hardcoded — same as all eight existing copies. Not generalized, because no fleet needs it and an unused input is a footgun.WATCHED_ASSETSstring. That is fine forcursor-review/groom/pr-sizeand is the scope the ticket specifies; see (1) for pr-risk.proceed=falseeither way). That follows the specified condition — both objects must be gone for the decommission message — and is defensible: the deletion commit did touch the path filter, so it does have its own run.