[PR #173] [MERGED] [groom] EXEMPT_NO_PREFLIGHT in the paths-contract test is stale, unreachable, and advertises a guard it does not have #181

New issue

Closed

opened 2026-10-26 22:37:07 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:07 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/173
Author: @cloud-code-bot[bot]
Created: 8/16/2026
Status: ✅ Merged
Merged: 8/17/2026
Merged by: @mattmillerai

Base: main ← Head: groom/github-workflows-whole-repo-github-bump-callers--31939149292-1

📝 Commits (1)

69c5823 groom: EXEMPT_NO_PREFLIGHT in the paths-contract test is stale, unreachable, and advertises a guard it does not have

📊 Changes

2 files changed (+10 additions, -23 deletions)

View changed files

📝 .github/bump-callers/README.md (+4 -4)
📝 .github/bump-callers/tests/test_paths_contract.sh (+6 -19)

📄 Description

🤖 Auto-built by the groom sweep — this is a machine-proposed refactor. It runs full CI + cursor-review and requires human review; it is NOT auto-merged. Ranked from a CONFIRMED groom finding. · run

ELI-5

Every workflow in the bump-*-callers family is supposed to run a shared safety check called preflight.sh before it goes and updates other repos. A test checks that they all do. That test also kept a short "these ones are allowed to skip it" list, with one name on it: bump-pr-risk-callers.yml. But that fleet migrated onto preflight a while ago, so the name on the list no longer does anything — the code that reads the list is only reached for a fleet that has no preflight step, and every fleet now has one. Worse, the comment above the list promised a property it does not have: that a fleet which migrates onto preflight would fail the test until someone deleted its entry. It would not — the entry just goes quietly inert. This PR deletes the dead list and the dead branch, so the test says exactly one thing about a fleet with no preflight step: that is a failure. No production behaviour changes; this is test scaffolding.

What changed

.github/bump-callers/tests/test_paths_contract.sh — removed the EXEMPT_NO_PREFLIGHT variable and the comment above it, and collapsed the if ! has_preflight block: the exemption branch (the allow-list membership test and its nested "the filter must still carry a ! exclusion" check) is gone, leaving a single unconditional bad "... runs no preflight step — every fleet must run preflight.sh" plus continue. A short comment in its place records why no allow-list remains: the only exemption ever granted was pr-risk's excluding paths: filter, which WATCHED_PATHSPECS can now express, so there is no longer a shape that could legitimately join such a list.
.github/bump-callers/README.md — updated the one paragraph that repeated the same claim ("the pr-risk exemption is an explicit allow-list entry ... so migrating it later fails the test until the entry is removed") to state what the test now does: there is no exemption list, because every fleet runs the guard.

Why

The allow-list is consulted only inside the if ! has_preflight branch, and all nine bump-*-callers.yml entrypoints — bump-pr-risk-callers.yml included, via its - name: Preflight (staleness / decommission guard) step — run preflight.sh. The entry and its nested check are therefore unreachable, and the advertised self-draining property does not exist: a migration would have made the entry silently inert rather than red. The risk is that the next author of a non-preflight fleet reads that comment and trusts a guard that is not there.

Behaviour is preserved for every file the test actually walks: each of the nine entrypoints has a preflight step, so each one took the has_preflight path before this change and still does — the deleted code could only have run for a fleet with no preflight step, and for that case the outcome is the same failure the non-exempt branch already produced. Test-only, no workflow or script that runs in CI for consumers is touched.

Verifier rationale

Verified, with the line refs corrected. The allow-list at test_paths_contract.sh:37 holds exactly one entry, bump-pr-risk-callers.yml, and the comment above it (:33-36) claims that -a fleet that later migrates onto preflight.sh fails until it is removed from this list-. That fleet HAS migrated: has_preflight() (:146) greps for a step line beginning - name: Preflight, and bump-pr-risk-callers.yml:140 is exactly - name: Preflight (staleness / decommission guard), running .github/bump-callers/preflight.sh at :188. The preflight.sh header itself records pr-risk as the first fleet moved.

Because the allow-list is consulted only inside the if ! has_preflight branch (:245-256), the entry and its nested exclusion check are unreachable, so the advertised self-draining property does not exist: a migration makes the entry silently inert instead of red. The :245 anchor in the original finding is right; the false claim itself sits at :33-37.

Fix, either way:

Delete the entry and the whole if ! has_preflight exemption branch, leaving the -runs no preflight step and is not a documented exemption- failure (:257) as the only outcome; or
If a future non-preflight fleet is genuinely expected, invert the check the way KNOWN_EXEMPT does in .github/workflow-pins/check_workflow_pins.py (:48-56, where a STALE entry fails so the list drains itself): fail when a LISTED file does run preflight.

Steelman: it is harmless today, since the fleet it names is fully exercised by the normal assertion path and nothing is unchecked. Fair, and that is why this is test-scaffolding cleanup rather than a defect in shipped behaviour. It still earns the change because the comment makes a false claim about a guard, and the next new fleet is exactly the reader who would rely on that claim.

Risk: very low, test-only.

_{🔄 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/173 **Author:** [@cloud-code-bot[bot]](https://github.com/apps/cloud-code-bot) **Created:** 8/16/2026 **Status:** ✅ Merged **Merged:** 8/17/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `groom/github-workflows-whole-repo-github-bump-callers--31939149292-1` --- ### 📝 Commits (1) - [`69c5823`](https://github.com/Comfy-Org/github-workflows/commit/69c5823811b0c61db3ccaa47ac2d54ffaa356c45) groom: EXEMPT_NO_PREFLIGHT in the paths-contract test is stale, unreachable, and advertises a guard it does not have ### 📊 Changes **2 files changed** (+10 additions, -23 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+4 -4) 📝 `.github/bump-callers/tests/test_paths_contract.sh` (+6 -19) </details> ### 📄 Description > 🤖 **Auto-built by the groom sweep** — this is a machine-proposed refactor. It runs full CI + cursor-review and **requires human review; it is NOT auto-merged.** Ranked from a CONFIRMED groom finding. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/31939149292) ## ELI-5 Every workflow in the `bump-*-callers` family is supposed to run a shared safety check called `preflight.sh` before it goes and updates other repos. A test checks that they all do. That test also kept a short "these ones are allowed to skip it" list, with one name on it: `bump-pr-risk-callers.yml`. But that fleet migrated onto preflight a while ago, so the name on the list no longer does anything — the code that reads the list is only reached for a fleet that has *no* preflight step, and every fleet now has one. Worse, the comment above the list promised a property it does not have: that a fleet which migrates onto preflight would fail the test until someone deleted its entry. It would not — the entry just goes quietly inert. This PR deletes the dead list and the dead branch, so the test says exactly one thing about a fleet with no preflight step: that is a failure. No production behaviour changes; this is test scaffolding. ## What changed - `.github/bump-callers/tests/test_paths_contract.sh` — removed the `EXEMPT_NO_PREFLIGHT` variable and the comment above it, and collapsed the `if ! has_preflight` block: the exemption branch (the allow-list membership test and its nested "the filter must still carry a `!` exclusion" check) is gone, leaving a single unconditional `bad "... runs no preflight step — every fleet must run preflight.sh"` plus `continue`. A short comment in its place records why no allow-list remains: the only exemption ever granted was pr-risk's excluding `paths:` filter, which `WATCHED_PATHSPECS` can now express, so there is no longer a shape that could legitimately join such a list. - `.github/bump-callers/README.md` — updated the one paragraph that repeated the same claim ("the pr-risk exemption is an explicit allow-list entry ... so migrating it later fails the test until the entry is removed") to state what the test now does: there is no exemption list, because every fleet runs the guard. ## Why The allow-list is consulted only inside the `if ! has_preflight` branch, and all nine `bump-*-callers.yml` entrypoints — `bump-pr-risk-callers.yml` included, via its `- name: Preflight (staleness / decommission guard)` step — run `preflight.sh`. The entry and its nested check are therefore unreachable, and the advertised self-draining property does not exist: a migration would have made the entry silently inert rather than red. The risk is that the next author of a non-preflight fleet reads that comment and trusts a guard that is not there. Behaviour is preserved for every file the test actually walks: each of the nine entrypoints has a preflight step, so each one took the `has_preflight` path before this change and still does — the deleted code could only have run for a fleet with no preflight step, and for that case the outcome is the same failure the non-exempt branch already produced. Test-only, no workflow or script that runs in CI for consumers is touched. <details> <summary><strong>Verifier rationale</strong></summary> **Verified, with the line refs corrected.** The allow-list at test_paths_contract.sh:37 holds exactly one entry, bump-pr-risk-callers.yml, and the comment above it (:33-36) claims that -a fleet that later migrates onto preflight.sh fails until it is removed from this list-. That fleet HAS migrated: `has_preflight()` (:146) greps for a step line beginning `- name: Preflight`, and bump-pr-risk-callers.yml:140 is exactly `- name: Preflight (staleness / decommission guard)`, running `.github/bump-callers/preflight.sh` at :188. The preflight.sh header itself records pr-risk as the first fleet moved. Because the allow-list is consulted only inside the `if ! has_preflight` branch (:245-256), the entry and its nested exclusion check are unreachable, so the advertised self-draining property does not exist: a migration makes the entry silently inert instead of red. The :245 anchor in the original finding is right; the false claim itself sits at :33-37. **Fix, either way:** * Delete the entry and the whole `if ! has_preflight` exemption branch, leaving the -runs no preflight step and is not a documented exemption- failure (:257) as the only outcome; or * If a future non-preflight fleet is genuinely expected, invert the check the way KNOWN_EXEMPT does in `.github/workflow-pins/check_workflow_pins.py` (:48-56, where a STALE entry fails so the list drains itself): fail when a LISTED file does run preflight. **Steelman:** it is harmless today, since the fleet it names is fully exercised by the normal assertion path and nothing is unchecked. Fair, and that is why this is test-scaffolding cleanup rather than a defect in shipped behaviour. It still earns the change because the comment makes a false claim about a guard, and the next new fleet is exactly the reader who would rely on that claim. **Risk:** very low, test-only. </details>  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:07 +00:00