[PR #152] [MERGED] ci(bump-callers): preflight pathspec staleness + per-file decommission (BE-6676) #163

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6676-preflight-pathspecs

📝 Commits (2)

689e141 ci(bump-callers): preflight pathspec staleness + per-file decommission (BE-6676)
97bdbf1 ci(bump-callers): close the silent-green holes in the preflight list inputs (BE-6676)

📊 Changes

4 files changed (+857 additions, -17 deletions)

View changed files

📝 .github/bump-callers/README.md (+90 -5)
📝 .github/bump-callers/preflight.sh (+388 -11)
📝 .github/bump-callers/tests/test_preflight.sh (+376 -0)
📝 AGENTS.md (+3 -1)

📄 Description

ELI-5

preflight.sh is the guard that decides, before a bump fleet re-pins its consumers, "is this run the newest one, and does the thing I'm about to pin still exist?" It answers the first question by comparing objects: one blob for the reusable workflow, optionally one tree OID for its asset directory. That works for seven of the eight fleets. It cannot work for pr-risk, whose trigger says "watch scripts/pr-risk/** — except its tests and its README", because a tree OID has no way to say "except". A test-only commit moves that OID, the guard concludes "someone newer will handle this", and nobody does: the fleet's one real run is thrown away and every caller freezes on a stale pin. Its deletion check has the mirror-image hole — it asks whether the directory still exists, and the directory happily survives once only tests/ and the README are left in it, so callers would get pinned to a commit where the scripts they execute are gone.

This adds the two optional inputs that let that fleet be expressed: WATCHED_PATHSPECS (ask git, with the exclusions, instead of comparing OIDs) and WATCHED_EXEC (probe the files a caller actually runs, not the folder they live in). Both are unset everywhere else, so nothing else changes.

What changed

.github/bump-callers/preflight.sh

WATCHED_PATHSPECS (optional, newline-separated git pathspecs, :(exclude) allowed). When set it replaces the WATCHED/WATCHED_ASSETS object comparison as the staleness test with git diff --quiet <head-oid> <fetched-tip-oid> -- <pathspecs>: exit 0 → unchanged → re-point to the verified tip; exit 1 → changed → stale no-op; exit >1 → ::error:: and exit 1, because a comparison that could not be performed is not a verdict in either direction (reading it as "changed" freezes the fleet, reading it as "unchanged" re-points callers to a tip nothing was verified at — the same not-evidence principle as resolve_oid). It compares two trees and walks no history, so it composes with the --unshallow deepening the direction guard needs without depending on it.
WATCHED_EXEC (optional, newline-separated repo-relative files a pinned caller executes). Each is probed for deletion at the tip through the existing resolve_oid (absent → ::warning:: decommissioned, proceed=false) and again locally with [[ -f ]] at the bottom of the script. The tip probe runs before the staleness test on purpose: a deletion also changes the watched surface, and reporting it as "a newer commit has its own run" would swallow the ::warning:: that is the fleet's only chance to say live callers are about to hard-fail at startup.
Shape validation, in the spirit of the existing glob/trailing-slash rejections — the recurring failure mode here is an input that silently verifies less than the entrypoint asked for, behind a green run. Set-but-blank is an error for both (it would fall back to exactly the check the fleet cannot use); WATCHED_EXEC rejects globs, trailing slashes and pathspec magic (nothing expands those, so every probe would report a decommission forever); WATCHED_PATHSPECS accepts only :(exclude)<path> magic and refuses an all-negative list, which git reads as every path except those — the widest possible watched surface.
Header updated: required/optional environment block, and the COUPLED TO THE PATH FILTER note now binds WATCHED_PATHSPECS explicitly — it MUST mirror the fleet's paths: filter, exclusions included; dropping one :(exclude) line reinstates the false-stale freeze exactly.

.github/bump-callers/tests/test_preflight.sh — 52 new assertions across 6 cases (107 → 159 total). Each new verdict is paired with a run of the input it replaces, so the tests show the difference, not just the new behaviour: the excluded-only commit that the tree-OID config false-stales; a mixed excluded+watched commit that is still correctly stale; a deleted grader that the -d directory probe would have bumped straight past.

Docs — .github/bump-callers/README.md (input table, shape rules, when a fleet needs these, a second consumption snippet) and the AGENTS.md bump-callers bullet.

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      # 159 passed, 0 failed
bash .github/bump-callers/tests/test_bump_callers.sh   # 175 passed, 0 failed

The 107 pre-existing preflight assertions pass unchanged, which is the evidence for "the seven simple fleets do not change behaviour". Independently: no bump-*-callers.yml entrypoint consumes preflight.sh yet (only test-bump-callers.yml references it), so this merge cannot alter a live fleet either way.

Two behavioural claims in the code and error messages were verified against real git rather than asserted: an all-negative pathspec really does mean "everything except" (a change to an unlisted file makes git diff --quiet -- ':(exclude)x' exit 1), and a pathspec git refuses — an absolute or ../ path, the reachable case the shape checks deliberately cannot catch — really does exit 128, which is what makes the >1 branch live rather than dead code. There is a test for the latter.

Judgment calls

Compared $head_sha/$fetched_tip, not the literal FETCH_HEAD HEAD the ticket wrote. Same trees, but FETCH_HEAD is a mutable ref and the surrounding code already deliberately reads the verified OIDs instead (see the comment above the direction guard); using the ref here would reintroduce exactly what that comment rules out.
Read the lists with a read loop, not mapfile. mapfile is bash 4+ and this file is run by hand on macOS's bash 3.2 (whose empty-array/set -u behaviour the existing header also calls out). Same semantics — split on newlines, trim, drop blanks.
WATCHED_ASSETS may still be set alongside WATCHED_PATHSPECS: its tree-OID comparison is superseded, its decommission probes still run. Documented in the header and README rather than rejected, because the combination is legitimate (pathspec staleness + a directory deletion probe) — but it is the one place an input's primary role goes quiet, so flagging it.
Only :(exclude) magic is accepted. :! is valid git shorthand and is nonetheless rejected, in favour of one spelled-out form the error message names. Conservative on purpose: a magic prefix this script has not reasoned about, or a typo in one, changes or empties what gets compared with no other signal.
WATCHED_EXEC has no separate "absent at this run's own commit" message the way WATCHED/WATCHED_ASSETS do. The both-absent case is caught by the tip probe and the HEAD-only case by the local probe, so every path reaches the same proceed=false + ::warning:: verdict; only the log wording differs.
Scope: this is the script + tests + docs. Actually swapping bump-pr-risk-callers.yml onto preflight.sh with these inputs is the follow-on change, exactly as BE-6475 left it.

No unmet acceptance criteria.

Review resolution (`97bdbf1`)

All ten panel findings addressed; every one was the same shape — a check that
verifies less than the entrypoint asked for, behind a green run.

WATCHED_PATHSPECS — a list that selects nothing is now an error rather
than an "unchanged" verdict (git diff --quiet exits 0 for both, and the second
re-points every caller to a tip at which nothing was compared): each positive
must select a tracked path in either tree, checked with the exclusions applied
and asked of git diff itself, so coverage is never judged by looser rules than
the verdict. The list must also select WATCHED (and, when WATCHED_ASSETS
is set, something under it) — the header stated that as a MUST but only
convention enforced it. !path, the paths: filter's own negation syntax, is
rejected with the :(exclude) spelling named (git reads the ! literally:
excludes nothing, matches nothing, and satisfied the all-negative guard).
Whole-line # comments are dropped, so the commented filter these docs invite
pasting works as pasted.

WATCHED_EXEC — a directory entry is rejected and the tip probe insists on a
blob, closing the case where the tip side ("tree resolves → present") and the
local [[ -f ]] ("absent") disagreed and the verdict turned on whether main
moved. Absolute and ../ paths are rejected for every watched path, which was
the same disagreement by another route. The local probe is skipped once the run
has been re-pointed — new_sha is then the tip, already cleared by the tip
probe, and this checkout is no longer the SHA callers get pinned to. A file
deleted by this run's own commit is now attributed to it instead of to the tip.

Plus the parsed-list array is no longer named LINES (bash manages that name).

Verification. 199 assertions, up from 159; the 107 pre-existing ones still
pass unchanged, and green under bash 3.2 as well as 5.x. The one vacuous
assertion the panel caught ("reusable change is stale too" — an earlier commit in
the same case sat between the baseline and the tip, so it reported a change
whether or not the reusable was covered) is rebuilt on a fresh baseline and
paired with the omitting list the coverage guard now refuses. Each new guard was
mutation-checked: reverting it fails the assertion written for it.

Two behaviours were measured rather than assumed: !path really does match and
exclude nothing, and git ls-tree cannot serve as the pathspec oracle (it
rejects :(exclude) outright and does not expand **), which is why the check
uses an empty-tree git diff.

_{🔄 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/152 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/11/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6676-preflight-pathspecs` --- ### 📝 Commits (2) - [`689e141`](https://github.com/Comfy-Org/github-workflows/commit/689e14151e60671debc5dbec98ccab94bfa4b5ce) ci(bump-callers): preflight pathspec staleness + per-file decommission (BE-6676) - [`97bdbf1`](https://github.com/Comfy-Org/github-workflows/commit/97bdbf1010dd7708c16b475de12d147fc6db01f1) ci(bump-callers): close the silent-green holes in the preflight list inputs (BE-6676) ### 📊 Changes **4 files changed** (+857 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+90 -5) 📝 `.github/bump-callers/preflight.sh` (+388 -11) 📝 `.github/bump-callers/tests/test_preflight.sh` (+376 -0) 📝 `AGENTS.md` (+3 -1) </details> ### 📄 Description ## ELI-5 `preflight.sh` is the guard that decides, before a bump fleet re-pins its consumers, "is this run the newest one, and does the thing I'm about to pin still exist?" It answers the first question by comparing objects: one blob for the reusable workflow, optionally one tree OID for its asset directory. That works for seven of the eight fleets. It cannot work for `pr-risk`, whose trigger says "watch `scripts/pr-risk/**` — *except* its tests and its README", because a tree OID has no way to say "except". A test-only commit moves that OID, the guard concludes "someone newer will handle this", and nobody does: the fleet's one real run is thrown away and every caller freezes on a stale pin. Its deletion check has the mirror-image hole — it asks whether the *directory* still exists, and the directory happily survives once only `tests/` and the README are left in it, so callers would get pinned to a commit where the scripts they execute are gone. This adds the two optional inputs that let that fleet be expressed: `WATCHED_PATHSPECS` (ask git, with the exclusions, instead of comparing OIDs) and `WATCHED_EXEC` (probe the files a caller actually runs, not the folder they live in). Both are unset everywhere else, so nothing else changes. ## What changed **`.github/bump-callers/preflight.sh`** - **`WATCHED_PATHSPECS`** (optional, newline-separated git pathspecs, `:(exclude)` allowed). When set it replaces the `WATCHED`/`WATCHED_ASSETS` object comparison as the staleness test with `git diff --quiet <head-oid> <fetched-tip-oid> -- <pathspecs>`: exit 0 → unchanged → re-point to the verified tip; exit 1 → changed → stale no-op; **exit >1 → `::error::` and exit 1**, because a comparison that could not be performed is not a verdict in either direction (reading it as "changed" freezes the fleet, reading it as "unchanged" re-points callers to a tip nothing was verified at — the same not-evidence principle as `resolve_oid`). It compares two trees and walks no history, so it composes with the `--unshallow` deepening the direction guard needs without depending on it. - **`WATCHED_EXEC`** (optional, newline-separated repo-relative files a pinned caller executes). Each is probed for deletion at the tip through the existing `resolve_oid` (absent → `::warning::` decommissioned, `proceed=false`) and again locally with `[[ -f ]]` at the bottom of the script. The tip probe runs **before** the staleness test on purpose: a deletion also changes the watched surface, and reporting it as "a newer commit has its own run" would swallow the `::warning::` that is the fleet's only chance to say live callers are about to hard-fail at startup. - **Shape validation**, in the spirit of the existing glob/trailing-slash rejections — the recurring failure mode here is an input that silently verifies *less* than the entrypoint asked for, behind a green run. Set-but-blank is an error for both (it would fall back to exactly the check the fleet cannot use); `WATCHED_EXEC` rejects globs, trailing slashes and pathspec magic (nothing expands those, so every probe would report a decommission forever); `WATCHED_PATHSPECS` accepts only `:(exclude)<path>` magic and refuses an all-negative list, which git reads as *every path except those* — the widest possible watched surface. - Header updated: required/optional environment block, and the `COUPLED TO THE PATH FILTER` note now binds `WATCHED_PATHSPECS` explicitly — it MUST mirror the fleet's `paths:` filter, exclusions included; dropping one `:(exclude)` line reinstates the false-stale freeze exactly. **`.github/bump-callers/tests/test_preflight.sh`** — 52 new assertions across 6 cases (107 → 159 total). Each new verdict is paired with a run of the input it replaces, so the tests show the *difference*, not just the new behaviour: the excluded-only commit that the tree-OID config false-stales; a mixed excluded+watched commit that is still correctly stale; a deleted grader that the `-d` directory probe would have bumped straight past. **Docs** — `.github/bump-callers/README.md` (input table, shape rules, when a fleet needs these, a second consumption snippet) and the `AGENTS.md` bump-callers bullet. ## 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 # 159 passed, 0 failed bash .github/bump-callers/tests/test_bump_callers.sh # 175 passed, 0 failed ``` The 107 pre-existing preflight assertions pass unchanged, which is the evidence for "the seven simple fleets do not change behaviour". Independently: no `bump-*-callers.yml` entrypoint consumes `preflight.sh` yet (only `test-bump-callers.yml` references it), so this merge cannot alter a live fleet either way. Two behavioural claims in the code and error messages were verified against real git rather than asserted: an all-negative pathspec really does mean "everything except" (a change to an unlisted file makes `git diff --quiet -- ':(exclude)x'` exit 1), and a pathspec git refuses — an absolute or `../` path, the reachable case the shape checks deliberately cannot catch — really does exit 128, which is what makes the `>1` branch live rather than dead code. There is a test for the latter. ## Judgment calls - **Compared `$head_sha`/`$fetched_tip`, not the literal `FETCH_HEAD HEAD` the ticket wrote.** Same trees, but `FETCH_HEAD` is a mutable ref and the surrounding code already deliberately reads the verified OIDs instead (see the comment above the direction guard); using the ref here would reintroduce exactly what that comment rules out. - **Read the lists with a `read` loop, not `mapfile`.** `mapfile` is bash 4+ and this file is run by hand on macOS's bash 3.2 (whose empty-array/`set -u` behaviour the existing header also calls out). Same semantics — split on newlines, trim, drop blanks. - **`WATCHED_ASSETS` may still be set alongside `WATCHED_PATHSPECS`**: its tree-OID *comparison* is superseded, its decommission probes still run. Documented in the header and README rather than rejected, because the combination is legitimate (pathspec staleness + a directory deletion probe) — but it is the one place an input's primary role goes quiet, so flagging it. - **Only `:(exclude)` magic is accepted.** `:!` is valid git shorthand and is nonetheless rejected, in favour of one spelled-out form the error message names. Conservative on purpose: a magic prefix this script has not reasoned about, or a typo in one, changes or empties what gets compared with no other signal. - **`WATCHED_EXEC` has no separate "absent at this run's own commit" message** the way `WATCHED`/`WATCHED_ASSETS` do. The both-absent case is caught by the tip probe and the HEAD-only case by the local probe, so every path reaches the same `proceed=false` + `::warning::` verdict; only the log wording differs. - **Scope**: this is the script + tests + docs. Actually swapping `bump-pr-risk-callers.yml` onto `preflight.sh` with these inputs is the follow-on change, exactly as BE-6475 left it. No unmet acceptance criteria. --- ## Review resolution (97bdbf1) All ten panel findings addressed; every one was the same shape — a check that verifies *less* than the entrypoint asked for, behind a green run. **`WATCHED_PATHSPECS`** — a list that selects **nothing** is now an error rather than an "unchanged" verdict (`git diff --quiet` exits 0 for both, and the second re-points every caller to a tip at which nothing was compared): each positive must select a tracked path in either tree, checked with the exclusions applied and asked of `git diff` itself, so coverage is never judged by looser rules than the verdict. The list must also **select `WATCHED`** (and, when `WATCHED_ASSETS` is set, something under it) — the header stated that as a MUST but only convention enforced it. `!path`, the `paths:` filter's own negation syntax, is rejected with the `:(exclude)` spelling named (git reads the `!` literally: excludes nothing, matches nothing, and satisfied the all-negative guard). Whole-line `#` comments are dropped, so the commented filter these docs invite pasting works as pasted. **`WATCHED_EXEC`** — a directory entry is rejected and the tip probe insists on a blob, closing the case where the tip side ("tree resolves → present") and the local `[[ -f ]]` ("absent") disagreed and the verdict turned on whether main moved. Absolute and `../` paths are rejected for every watched path, which was the same disagreement by another route. The local probe is skipped once the run has been **re-pointed** — `new_sha` is then the tip, already cleared by the tip probe, and this checkout is no longer the SHA callers get pinned to. A file deleted by *this run's own* commit is now attributed to it instead of to the tip. Plus the parsed-list array is no longer named `LINES` (bash manages that name). **Verification.** 199 assertions, up from 159; the 107 pre-existing ones still pass unchanged, and green under bash 3.2 as well as 5.x. The one vacuous assertion the panel caught ("reusable change is stale too" — an earlier commit in the same case sat between the baseline and the tip, so it reported a change whether or not the reusable was covered) is rebuilt on a fresh baseline and paired with the omitting list the coverage guard now refuses. Each new guard was mutation-checked: reverting it fails the assertion written for it. Two behaviours were measured rather than assumed: `!path` really does match and exclude nothing, and `git ls-tree` cannot serve as the pathspec oracle (it rejects `:(exclude)` outright and does not expand `**`), which is why the check uses an empty-tree `git diff`. --- <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:56 +00:00