[PR #136] [MERGED] ci(bump-callers): add preflight.sh — one tested staleness/decommission guard (BE-6475) #147

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6475-bump-callers-preflight

📝 Commits (3)

0ed4e0a ci(bump-callers): add preflight.sh — one tested staleness/decommission guard (BE-6475)
13c540f fix(bump-callers): harden preflight.sh inputs, tip fetch and decommission verdict (BE-6475)
7e8490a ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) (#141)

📊 Changes

5 files changed (+1109 additions, -8 deletions)

View changed files

📝 .github/bump-callers/README.md (+148 -1)
➕ .github/bump-callers/preflight.sh (+418 -0)
➕ .github/bump-callers/tests/test_preflight.sh (+529 -0)
📝 .github/workflows/test-bump-callers.yml (+8 -4)
📝 AGENTS.md (+6 -3)

📄 Description

ELI-5

Every bump-*-callers.yml workflow 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 from bump-detect-unreviewed-merge-callers.yml (PR #117, the most-hardened copy) and generalized. Inputs WATCHED (required), WATCHED_ASSETS (optional, for multi-path fleets), NEW_SHA, plus Actions' GITHUB_SHA / GITHUB_OUTPUT. Outputs proceed=true|false and new_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 as origin plus a clone as the run workspace, drives the real script, and asserts exit code, both outputs, and the presence/absence of ::error:: / ::warning::.
Wiring — test-bump-callers.yml shellchecks both scripts and both suites and runs the new suite; .github/bump-callers/README.md gains a "Preflight" section (inputs, outputs, the two-step consumption pattern); AGENTS.md Commands/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.yml compares content but then pins the stale github.sha rather than the verified tip. The #117 copy gets both right, plus the exact-refname tip parse (a branch literally named foo/refs/heads/main matches the ls-remote pattern at component boundaries and must not be consumed), the FETCH_HEAD verification (an unresolvable FETCH_HEAD must 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 whose paths: 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 without WATCHED_ASSETS proceeds, which is the under-verification the widening removes.

Test cases

Case	Asserts
decoy ref	a `refs/heads/foo/refs/heads/main` sorting first is not consumed; run proceeds at the real tip, no "main moved" log
failed ls-remote	exit 1, `::error::`, `proceed` not `true`
stale re-run, watched blob changed	exit 0, `proceed=false`, stale log
main moved, content identical	exit 0, `proceed=true`, `new_sha` == the new tip (the re-point)
watched file deleted at tip	exit 0, `proceed=false`, `::warning::` decommission
asset dir changed only (multi-path)	`proceed=false` — and the single-path config on the same fixture proceeds
absent at the run's OWN commit	`::warning::` own-commit decommission, not reported as stale
current tip happy path	`proceed=true`, `new_sha` == `GITHUB_SHA`, no fetch/compare
asset dir gone locally at the tip	the final `-d "$WATCHED_ASSETS"` guard fires

Verification

shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/preflight.sh \
  .github/bump-callers/tests/test_bump_callers.sh .github/bump-callers/tests/test_preflight.sh   # clean
bash .github/bump-callers/tests/test_preflight.sh      # 42 passed, 0 failed
bash .github/bump-callers/tests/test_bump_callers.sh   # 175 passed, 0 failed (unchanged)
python3 .github/agents-md-integrity/check_agents_md.py --root .   # passed (2 pre-existing warnings)

git status confirms no bump-*-callers.yml file is touched.

Judgment calls worth a reviewer's attention

bump-pr-risk-callers.yml has diverged past #117 and this script does not implement its extra checks. Since #117 landed, the pr-risk entrypoint grew a git 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 --unshallow probe that test needs, path-filter exclusions (:(exclude)scripts/pr-risk/tests), and a git merge-base --is-ancestor refusal to pin an orphaned commit. The ticket is explicit that the extraction adopts the #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.
Only main is 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.
A multi-path fleet whose extra surface is not a single directory (pr-risk's exclusions) is not expressible with one WATCHED_ASSETS string. That is fine for cursor-review/groom/pr-size and is the scope the ticket specifies; see (1) for pr-risk.
A deleted workflow file whose asset dir survives logs "stale" rather than "decommissioned" (proceed=false either 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.
Negative-claim check. The diff adds skip/warn paths, so: the proceed path is empirically exercised green in three test cases (decoy, re-point, current tip) rather than only asserted absent, and no capability is denied — nothing consumes the script on this branch, so no fleet's behaviour can change from this merge.

_{🔄 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/136 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/5/2026 **Status:** ✅ Merged **Merged:** 8/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6475-bump-callers-preflight` --- ### 📝 Commits (3) - [`0ed4e0a`](https://github.com/Comfy-Org/github-workflows/commit/0ed4e0ae11912a8c9b4ad5f2ee73b130c3ff19b5) ci(bump-callers): add preflight.sh — one tested staleness/decommission guard (BE-6475) - [`13c540f`](https://github.com/Comfy-Org/github-workflows/commit/13c540f76d611f16d054ef0c2f294b019325e28f) fix(bump-callers): harden preflight.sh inputs, tip fetch and decommission verdict (BE-6475) - [`7e8490a`](https://github.com/Comfy-Org/github-workflows/commit/7e8490a985982f4a5ccc5bb47f18d9960593cbbe) ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) (#141) ### 📊 Changes **5 files changed** (+1109 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+148 -1) ➕ `.github/bump-callers/preflight.sh` (+418 -0) ➕ `.github/bump-callers/tests/test_preflight.sh` (+529 -0) 📝 `.github/workflows/test-bump-callers.yml` (+8 -4) 📝 `AGENTS.md` (+6 -3) </details> ### 📄 Description ## ELI-5 Every `bump-*-callers.yml` workflow 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 from `bump-detect-unreviewed-merge-callers.yml` (PR #117, the most-hardened copy) and generalized. Inputs `WATCHED` (required), `WATCHED_ASSETS` (optional, for multi-path fleets), `NEW_SHA`, plus Actions' `GITHUB_SHA` / `GITHUB_OUTPUT`. Outputs `proceed=true|false` and `new_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 as `origin` plus a clone as the run workspace, drives the real script, and asserts exit code, both outputs, and the presence/absence of `::error::` / `::warning::`. - **Wiring** — `test-bump-callers.yml` shellchecks both scripts and both suites and runs the new suite; `.github/bump-callers/README.md` gains a "Preflight" section (inputs, outputs, the two-step consumption pattern); `AGENTS.md` Commands/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.yml` compares content but then pins the stale `github.sha` rather than the verified tip. The #117 copy gets both right, plus the exact-refname tip parse (a branch literally named `foo/refs/heads/main` matches the `ls-remote` pattern at component boundaries and must not be consumed), the `FETCH_HEAD` verification (an unresolvable `FETCH_HEAD` must 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 whose `paths:` 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 *without* `WATCHED_ASSETS` proceeds, which is the under-verification the widening removes. ## Test cases | Case | Asserts | |---|---| | decoy ref | a `refs/heads/foo/refs/heads/main` sorting first is not consumed; run proceeds at the real tip, no "main moved" log | | failed ls-remote | exit 1, `::error::`, `proceed` not `true` | | stale re-run, watched blob changed | exit 0, `proceed=false`, stale log | | main moved, content identical | exit 0, `proceed=true`, `new_sha` == the new tip (the re-point) | | watched file deleted at tip | exit 0, `proceed=false`, `::warning::` decommission | | asset dir changed only (multi-path) | `proceed=false` — and the single-path config on the same fixture proceeds | | absent at the run's OWN commit | `::warning::` own-commit decommission, *not* reported as stale | | current tip happy path | `proceed=true`, `new_sha` == `GITHUB_SHA`, no fetch/compare | | asset dir gone locally at the tip | the final `-d "$WATCHED_ASSETS"` guard fires | ## Verification ``` shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/preflight.sh \ .github/bump-callers/tests/test_bump_callers.sh .github/bump-callers/tests/test_preflight.sh # clean bash .github/bump-callers/tests/test_preflight.sh # 42 passed, 0 failed bash .github/bump-callers/tests/test_bump_callers.sh # 175 passed, 0 failed (unchanged) python3 .github/agents-md-integrity/check_agents_md.py --root . # passed (2 pre-existing warnings) ``` `git status` confirms no `bump-*-callers.yml` file is touched. ## Judgment calls worth a reviewer's attention 1. **`bump-pr-risk-callers.yml` has diverged past #117 and this script does not implement its extra checks.** Since #117 landed, the pr-risk entrypoint grew a `git 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 `--unshallow` probe that test needs, path-filter *exclusions* (`:(exclude)scripts/pr-risk/tests`), and a `git merge-base --is-ancestor` refusal to pin an orphaned commit. The ticket is explicit that the extraction adopts the #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. 2. **Only `main` is 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. 3. **A multi-path fleet whose extra surface is not a single directory** (pr-risk's exclusions) is not expressible with one `WATCHED_ASSETS` string. That is fine for `cursor-review`/`groom`/`pr-size` and is the scope the ticket specifies; see (1) for pr-risk. 4. **A deleted workflow file whose asset dir survives** logs "stale" rather than "decommissioned" (`proceed=false` either 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. 5. **Negative-claim check.** The diff adds skip/warn paths, so: the proceed path is empirically exercised green in three test cases (decoy, re-point, current tip) rather than only asserted absent, and no capability is denied — nothing consumes the script on this branch, so no fleet's behaviour can change from this merge. --- <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:46 +00:00