[PR #159] [MERGED] ci(bump-callers): swap bump-pr-risk-callers.yml onto the shared preflight (BE-6677) #167

New issue

Closed

opened 2026-10-26 22:36:57 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:57 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/159
Author: @mattmillerai
Created: 8/12/2026
Status: ✅ Merged
Merged: 8/12/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6677-pr-risk-preflight

📝 Commits (2)

614677d ci(bump-callers): swap bump-pr-risk-callers.yml onto the shared preflight (BE-6677)
f9558b4 ci(bump-callers): widen pr-risk's preflight inputs to what a caller runs (BE-6677)

📊 Changes

3 files changed (+134 additions, -141 deletions)

View changed files

📝 .github/bump-callers/README.md (+31 -9)
📝 .github/bump-callers/preflight.sh (+23 -13)
📝 .github/workflows/bump-pr-risk-callers.yml (+80 -119)

📄 Description

ELI-5

bump-pr-risk-callers.yml had its own hand-written copy of the "should this run bump anything at all?" guard — is this run stale, and has the thing it watches been deleted? That guard now lives in one shared, tested script (.github/bump-callers/preflight.sh), so this entrypoint just calls it and does what it says. pr-risk is the first fleet swapped over, and deliberately so: it is the only one whose path filter has exclusions and whose decommission check is per-executed-file, so the two list inputs added in BE-6676 (WATCHED_PATHSPECS, WATCHED_EXEC) finally have a real consumer instead of only tests.

What changed

.github/workflows/bump-pr-risk-callers.yml: the inline ls-remote → fetch → per-file rev-parse → rev-list → [[ ! -f ]] block (~115 lines) is replaced by the two-step consumption documented in .github/bump-callers/README.md — a Preflight (staleness / decommission guard) step passing WATCHED / WATCHED_PATHSPECS / WATCHED_EXEC / NEW_SHA, and the bump step gated on steps.preflight.outputs.proceed == 'true' with NEW_SHA: ${{ steps.preflight.outputs.new_sha }}. Everything else is untouched: the paths: filter, the concurrency group, timeout-minutes, the main-only if:, the app-token step and its permission trio, and every other env key (GH_TOKEN, VAR_NAME, TAG, WORKFLOW_FILE, ALLOW_EMPTY, CALLERS_JSON), plus all the header documentation about the roster secret and the 300-file paths:-filter recovery note.
.github/bump-callers/README.md: the "the entrypoints still carry their inline copies" note now records pr-risk as swapped (and why it went first), and the tag-shadow fix below.
.github/bump-callers/preflight.sh: header-comment only — it no longer says "nothing consumes it yet".

The two list inputs were verified against the current filter and reusable rather than copied on faith: WATCHED_PATHSPECS mirrors the paths: filter line for line, exclusions included, and WATCHED_EXEC mirrors the four files the retired inline probe checked (pr-risk.yml plus the three graders it executes, directly or via grade-targets.sh).

Deliberate behaviour changes (decided in spike BE-6670)

pr-risk gains preflight's re-point-to-tip. When a later commit exists but the watched surface is byte-identical, callers are pinned to the verified tip rather than to a github.sha that is no longer current. Safe by construction (every watched object was just proved identical at both commits) and it self-heals the 300-file paths:-filter gap the header describes.
The rev-list "did a later COMMIT touch a watched path" test is retired, superseded by re-point-to-tip plus the pathspec-accurate diff. The case it existed for — a land-then-revert — now ends pinned at the tip instead of no-opped: same end state, one less failure mode, and never backwards. Verified empirically, see below.
The is-ancestor refusal is retained via the shared guard — and gets stricter, since preflight measures direction twice (the ls-remote tip must be an ancestor of the fetched one, and the fetched one a descendant of this run's commit). It also fixes a latent hazard the inline copy carried: it fetched a bare main, which resolves refs/tags/main before refs/heads/main, in a repo that routinely force-moves major tags. preflight fetches refs/heads/main.

Verification

actionlint .github/workflows/bump-pr-risk-callers.yml — clean. (Repo-wide actionlint reports only the pre-existing github.job_workflow_sha warnings in groom.yml.)
shellcheck -x on the four bump-callers shell files — clean.
bash .github/bump-callers/tests/test_preflight.sh — 199 passed, 0 failed; bash .github/bump-callers/tests/test_bump_callers.sh — 368 passed, 0 failed.
python3 .github/workflow-pins/check_workflow_pins.py — OK. (check_agents_md.py fails on AGENTS.md being 227 lines — pre-existing on main, untouched here.)
End-to-end smoke with the exact env block this file now passes, against a synthetic origin built from the real tree: (a) tip == github.sha → proceed=true; (b) a later commit touching only scripts/pr-risk/tests/** → not stale, re-points to the tip and proceeds (the false-stale freeze an over-broad input would cause); (c) a later commit changing a grader → proceed=false, stale; (d) grade-targets.sh deleted at the tip → ::warning:: decommission, proceed=false; (e) land-then-revert re-run → proceed=true pinned at the revert commit, where the retired rev-list test reported "2 commits touched a watched path" and would have skipped entirely.

Judgment calls / criteria not met as written

The ticket's git grep -n 'merge-base --is-ancestor' .github/workflows/ "should return nothing" does not hold, and should not. pr-risk.yml itself uses merge-base --is-ancestor for its own workflows_ref pin-ancestry check — unrelated to the bumper's inline guard. The check I actually ran is the one the criterion meant: no bump-*-callers.yml entrypoint matches merge-base --is-ancestor / rev-list --count / ls-remote any more for pr-risk (the other seven fleets still carry their inline copies by design; swapping them is a separate change each).
WATCHED_EXEC keeps exact parity with the retired inline list, which is narrower than "every file a caller executes." grade-targets.sh also invokes apply-risk-label.sh (hard-required — it dies if absent) and publish-risk-surfaces.sh (only under the opt-in check_run:/sticky_comment: surfaces), and the graders read risk-map.v0.json / runbook-registry.v0.json. A commit deleting apply-risk-label.sh alone would therefore still bump the fleet onto a SHA where a required script is gone. That gap predates this PR, and widening the decommission surface here would be a fourth behaviour change on a swap meant to be behaviour-preserving apart from the three above — so it is flagged rather than silently fixed. Worth a follow-up ticket if reviewers agree.
The preflight step is placed before the token step (it needs only the checkout). The token is still minted on a run preflight will skip; gating that step too would have been an extra change beyond the ticket, and the README's documented pattern gates only the bump.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/159 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/12/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6677-pr-risk-preflight` --- ### 📝 Commits (2) - [`614677d`](https://github.com/Comfy-Org/github-workflows/commit/614677d3d4a9fbf140da9c9d1eaa5c7ea34b5ae1) ci(bump-callers): swap bump-pr-risk-callers.yml onto the shared preflight (BE-6677) - [`f9558b4`](https://github.com/Comfy-Org/github-workflows/commit/f9558b4e541c851c5dab2e2ce3e9678933a4e61e) ci(bump-callers): widen pr-risk's preflight inputs to what a caller runs (BE-6677) ### 📊 Changes **3 files changed** (+134 additions, -141 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+31 -9) 📝 `.github/bump-callers/preflight.sh` (+23 -13) 📝 `.github/workflows/bump-pr-risk-callers.yml` (+80 -119) </details> ### 📄 Description ## ELI-5 `bump-pr-risk-callers.yml` had its own hand-written copy of the "should this run bump anything at all?" guard — is this run stale, and has the thing it watches been deleted? That guard now lives in one shared, tested script (`.github/bump-callers/preflight.sh`), so this entrypoint just calls it and does what it says. pr-risk is the first fleet swapped over, and deliberately so: it is the only one whose path filter has exclusions and whose decommission check is per-executed-file, so the two list inputs added in BE-6676 (`WATCHED_PATHSPECS`, `WATCHED_EXEC`) finally have a real consumer instead of only tests. ## What changed - `.github/workflows/bump-pr-risk-callers.yml`: the inline `ls-remote` → fetch → per-file `rev-parse` → `rev-list` → `[[ ! -f ]]` block (~115 lines) is replaced by the two-step consumption documented in `.github/bump-callers/README.md` — a `Preflight (staleness / decommission guard)` step passing `WATCHED` / `WATCHED_PATHSPECS` / `WATCHED_EXEC` / `NEW_SHA`, and the bump step gated on `steps.preflight.outputs.proceed == 'true'` with `NEW_SHA: ${{ steps.preflight.outputs.new_sha }}`. Everything else is untouched: the `paths:` filter, the concurrency group, `timeout-minutes`, the main-only `if:`, the app-token step and its permission trio, and every other env key (`GH_TOKEN`, `VAR_NAME`, `TAG`, `WORKFLOW_FILE`, `ALLOW_EMPTY`, `CALLERS_JSON`), plus all the header documentation about the roster secret and the 300-file `paths:`-filter recovery note. - `.github/bump-callers/README.md`: the "the entrypoints still carry their inline copies" note now records pr-risk as swapped (and why it went first), and the tag-shadow fix below. - `.github/bump-callers/preflight.sh`: header-comment only — it no longer says "nothing consumes it yet". The two list inputs were verified against the current filter and reusable rather than copied on faith: `WATCHED_PATHSPECS` mirrors the `paths:` filter line for line, exclusions included, and `WATCHED_EXEC` mirrors the four files the retired inline probe checked (`pr-risk.yml` plus the three graders it executes, directly or via `grade-targets.sh`). ## Deliberate behaviour changes (decided in spike BE-6670) 1. **pr-risk gains preflight's re-point-to-tip.** When a later commit exists but the watched surface is byte-identical, callers are pinned to the *verified tip* rather than to a `github.sha` that is no longer current. Safe by construction (every watched object was just proved identical at both commits) and it self-heals the 300-file `paths:`-filter gap the header describes. 2. **The `rev-list` "did a later COMMIT touch a watched path" test is retired**, superseded by re-point-to-tip plus the pathspec-accurate diff. The case it existed for — a land-then-revert — now ends *pinned at the tip* instead of no-opped: same end state, one less failure mode, and never backwards. Verified empirically, see below. 3. **The is-ancestor refusal is retained via the shared guard** — and gets stricter, since preflight measures direction twice (the `ls-remote` tip must be an ancestor of the fetched one, and the fetched one a descendant of this run's commit). It also fixes a latent hazard the inline copy carried: it fetched a bare `main`, which resolves `refs/tags/main` **before** `refs/heads/main`, in a repo that routinely force-moves major tags. preflight fetches `refs/heads/main`. ## Verification - `actionlint .github/workflows/bump-pr-risk-callers.yml` — clean. (Repo-wide `actionlint` reports only the pre-existing `github.job_workflow_sha` warnings in `groom.yml`.) - `shellcheck -x` on the four bump-callers shell files — clean. - `bash .github/bump-callers/tests/test_preflight.sh` — **199 passed, 0 failed**; `bash .github/bump-callers/tests/test_bump_callers.sh` — **368 passed, 0 failed**. - `python3 .github/workflow-pins/check_workflow_pins.py` — OK. (`check_agents_md.py` fails on `AGENTS.md` being 227 lines — pre-existing on `main`, untouched here.) - **End-to-end smoke with the exact env block this file now passes**, against a synthetic origin built from the real tree: (a) tip == `github.sha` → `proceed=true`; (b) a later commit touching only `scripts/pr-risk/tests/**` → **not** stale, re-points to the tip and proceeds (the false-stale freeze an over-broad input would cause); (c) a later commit changing a grader → `proceed=false`, stale; (d) `grade-targets.sh` deleted at the tip → `::warning::` decommission, `proceed=false`; (e) land-then-revert re-run → `proceed=true` pinned at the revert commit, where the retired `rev-list` test reported "2 commits touched a watched path" and would have skipped entirely. ## Judgment calls / criteria not met as written - **The ticket's `git grep -n 'merge-base --is-ancestor' .github/workflows/` "should return nothing" does not hold, and should not.** `pr-risk.yml` itself uses `merge-base --is-ancestor` for its own `workflows_ref` pin-ancestry check — unrelated to the bumper's inline guard. The check I actually ran is the one the criterion meant: **no `bump-*-callers.yml` entrypoint** matches `merge-base --is-ancestor` / `rev-list --count` / `ls-remote` any more for pr-risk (the other seven fleets still carry their inline copies by design; swapping them is a separate change each). - **`WATCHED_EXEC` keeps exact parity with the retired inline list, which is narrower than "every file a caller executes."** `grade-targets.sh` also invokes `apply-risk-label.sh` (hard-required — it `die`s if absent) and `publish-risk-surfaces.sh` (only under the opt-in `check_run:`/`sticky_comment:` surfaces), and the graders read `risk-map.v0.json` / `runbook-registry.v0.json`. A commit deleting `apply-risk-label.sh` alone would therefore still bump the fleet onto a SHA where a required script is gone. That gap predates this PR, and widening the decommission surface here would be a fourth behaviour change on a swap meant to be behaviour-preserving apart from the three above — so it is flagged rather than silently fixed. Worth a follow-up ticket if reviewers agree. - The preflight step is placed before the token step (it needs only the checkout). The token is still minted on a run preflight will skip; gating that step too would have been an extra change beyond the ticket, and the README's documented pattern gates only the bump. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:57 +00:00