[PR #154] [MERGED] ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084) #161

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-7045-multi-asset-watched ← Head: matt/be-7084-exclude-test-go

📝 Commits (5)

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)
bbfca53 Merge branch 'matt/be-6676-preflight-pathspecs' into matt/be-7084-exclude-test-go
949f012 ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084)
fd50a89 ci(bump-callers): close four review findings in the pathspec equivalence check (BE-7084)

📊 Changes

7 files changed (+1432 additions, -45 deletions)

View changed files

📝 .github/bump-callers/README.md (+140 -15)
📝 .github/bump-callers/preflight.sh (+446 -20)
📝 .github/bump-callers/tests/test_paths_contract.sh (+304 -9)
📝 .github/bump-callers/tests/test_preflight.sh (+487 -0)
📝 .github/workflows/bump-cursor-review-callers.yml (+26 -0)
📝 .github/workflows/bump-pr-size-callers.yml (+25 -0)
📝 AGENTS.md (+4 -1)

📄 Description

STACKED — merging lands on matt/be-7045-multi-asset-watched (owned by @mattmillerai, PR #153), NOT main. This PR also carries a reconciliation merge of matt/be-6676-preflight-pathspecs (#152). Merge order: #151 → #153 → #152 → this. GitHub retargets this PR as each foundation lands; its diff collapses to the "My own net change" table below once they do.

ELI-5

The bump robots watch scripts/check-pr-size/ and, whenever anything in it changes, open a "please update your pin" PR in every repo that uses these workflows. Trouble is, that folder also holds the tool's own Go tests — and nobody downstream ever runs those tests. So editing a test file woke the robot, minted an org-wide bot token, and sent a pointless PR to every consumer repo asking them to review a change that does nothing for them.

This teaches both robots to ignore *_test.go. Editing a test is now quiet; editing size.go, main.go or go.mod still sends the PR exactly as before.

The catch is that ignoring a file is a two-part job. Skipping the trigger is easy, but the robot has a second safety check that asks "did the watched folder change since?" by comparing folder fingerprints — and a fingerprint can't say "…except the tests". Left alone, that check would answer "yes, changed" for a test-only commit and then wait forever for a run that the trigger guarantees never starts, freezing the fleet. #152 added the input that can express an exclusion (WATCHED_PATHSPECS), so both halves now ask the same question.

What changed

bump-pr-size-callers.yml / bump-cursor-review-callers.yml — - '!scripts/check-pr-size/*_test.go' added to paths:, mirroring pr-risk's existing exclusion, plus a WATCHED_PATHSPECS block that mirrors the filter exclusions included. WATCHED_ASSETS is deliberately kept: the pathspec list supersedes its staleness comparison, not its decommission probes.
test_paths_contract.sh — the flat !-rejection becomes an equivalence check. A preflight fleet whose filter carries ! must set WATCHED_PATHSPECS (a missing one still fails, naming the freeze the old rejection prevented), and the two are compared as sets: positives normalized x/** → x, negations as :(exclude)x with the glob kept verbatim, no extras in either direction. A fleet with no ! that sets the input anyway is held to the same equivalence, so it can't drift.
Docs — README fleet table, the COUPLED TO THE PATH FILTER note, and AGENTS.md's fleet list.

This relaxation is also precisely what the future BE-6475 pr-risk-onto-preflight swap needs. That swap is not done here.

Reconciliation of #152 and #153 (please review this specifically)

The ticket assumed #152 and #153 would already be merged and asked me to verify the reconciliation hadn't dropped either feature. They are still open and their preflight.sh edits conflict, so this PR performs that reconciliation. It was not mechanical — #152 was written when WATCHED_ASSETS was still a scalar:

The pathspec coverage guard now requires the list to reach every WATCHED_ASSETS entry, not the value as a whole. cursor-review watches two directories; checking them jointly would have been strictly weaker than the OID comparison this input supersedes. It also matches a file asset as itself — an entry may be a file, and #152's prefix-only test would have failed those with an error the pathspec list cannot fix.
The staleness elif uses #153's per-entry assets_changed; #152's scalar tip_assets/here_assets no longer exist.
#153's "name the surface that moved" diagnostic is carried into the pathspec branch, so the fleets that need exclusions aren't left with a worse diagnostic than the ones that don't.
Header/README now document the three list inputs' genuinely different comment and empty-value rules instead of a blanket "both list inputs" claim that had become wrong.

Verification

shellcheck -x (all 5 scripts) and actionlint clean.

Suite	Result
`test_preflight.sh`	278 passed, 0 failed (261 before)
`test_paths_contract.sh`	40 passed, 0 failed (29 before)
`test_bump_callers.sh`	185 passed, 0 failed

The new preflight case pair runs this fleet's literal shape against a real git repo: a *_test.go-only commit reads unchanged and re-points; the same commit under the old tree-OID config false-stales (the freeze, reproduced, so the input is necessary rather than cosmetic); and size.go, go.mod, the reusable itself, and a code+test commit all still read as changed. New contract fixtures pin the rejections the real entrypoints never exercise — dropped exclusion, extra exclusion, missing positive, an exclusion normalized to its parent dir, and a raw ! entry not translated to :(exclude).

Falsified the load-bearing premise rather than trusting it. The change denies bump PRs on the claim that pinned callers never execute these tests, so I went and checked: the only go test in the repo is this repo's own CI job (test-pr-size.yml:55); pr-size.yml:264 and cursor-review.yml:489 both go build -o … . and run the binary, and go build does not compile _test.go files at all. test-pr-size.yml still watches scripts/check-pr-size/** with no exclusion, so the tests keep running on every change here — only the consumer-facing churn goes away.

Judgment calls and known limits

The ticket's precondition is unmet and I stacked instead of waiting. #151/#152/#153 are all still open (none closed unmerged). Per the standing always-stack directive I built on the real branches rather than blocking. Consequence: this PR's diff double-counts the foundations until they land, and the #152/#153 reconciliation above is mine, not a merge artifact — it deserves review attention on its own.
The GitHub-trigger half is reasoned, not executed. I cannot run Actions from here, so "a test-only commit matches no trigger" rests on documented paths: semantics plus the pr-risk precedent already in production (bump-pr-risk-callers.yml:71-72). The preflight half is executed, against a real repo, in the new case pair.
Known * asymmetry, benign and currently unreachable. GitHub's * does not cross /; git's pathspec * does (measured). So a hypothetical scripts/check-pr-size/sub/foo_test.go would still fire the trigger while preflight treated it as unchanged. The directory is flat today (go.mod, main.go, main_test.go, size.go, size_test.go), and the direction is safe — it can only re-point to a commit consumers don't execute, never freeze the fleet. I kept the literal pathspec the ticket and spike BE-7083 specified rather than switching to :(glob) magic, which would also have broken the verbatim-glob equivalence the contract test is specified to enforce.
The !-without-WATCHED_PATHSPECS guard is covered through its trigger condition (an absent input parses empty) rather than a full synthetic entrypoint; driving the whole file loop from a fixture would have meant restructuring code this ticket puts out of scope.

Out of scope (per the ticket)

BE-6475's pr-risk migration onto preflight, roster contents, and any change to bump-callers.sh.

_{🔄 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/154 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/12/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-7045-multi-asset-watched` ← **Head:** `matt/be-7084-exclude-test-go` --- ### 📝 Commits (5) - [`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) - [`bbfca53`](https://github.com/Comfy-Org/github-workflows/commit/bbfca53cab507b11a82f08674ec338cf2508e07f) Merge branch 'matt/be-6676-preflight-pathspecs' into matt/be-7084-exclude-test-go - [`949f012`](https://github.com/Comfy-Org/github-workflows/commit/949f0126a876c3e24803fb0e4f52011a45c60e3a) ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084) - [`fd50a89`](https://github.com/Comfy-Org/github-workflows/commit/fd50a89a8ee47f1fb0dbef8311c4fbe8b0c9d567) ci(bump-callers): close four review findings in the pathspec equivalence check (BE-7084) ### 📊 Changes **7 files changed** (+1432 additions, -45 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+140 -15) 📝 `.github/bump-callers/preflight.sh` (+446 -20) 📝 `.github/bump-callers/tests/test_paths_contract.sh` (+304 -9) 📝 `.github/bump-callers/tests/test_preflight.sh` (+487 -0) 📝 `.github/workflows/bump-cursor-review-callers.yml` (+26 -0) 📝 `.github/workflows/bump-pr-size-callers.yml` (+25 -0) 📝 `AGENTS.md` (+4 -1) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-7045-multi-asset-watched` (owned by @mattmillerai, PR #153), NOT `main`.** This PR also carries a reconciliation merge of `matt/be-6676-preflight-pathspecs` (#152). Merge order: **#151 → #153 → #152 → this**. GitHub retargets this PR as each foundation lands; its diff collapses to the "My own net change" table below once they do. ## ELI-5 The bump robots watch `scripts/check-pr-size/` and, whenever anything in it changes, open a "please update your pin" PR in every repo that uses these workflows. Trouble is, that folder also holds the tool's own Go **tests** — and nobody downstream ever runs those tests. So editing a test file woke the robot, minted an org-wide bot token, and sent a pointless PR to every consumer repo asking them to review a change that does nothing for them. This teaches both robots to ignore `*_test.go`. Editing a test is now quiet; editing `size.go`, `main.go` or `go.mod` still sends the PR exactly as before. The catch is that ignoring a file is a two-part job. Skipping the trigger is easy, but the robot has a second safety check that asks "did the watched folder change since?" by comparing folder fingerprints — and a fingerprint can't say "…except the tests". Left alone, that check would answer "yes, changed" for a test-only commit and then wait forever for a run that the trigger guarantees never starts, freezing the fleet. #152 added the input that *can* express an exclusion (`WATCHED_PATHSPECS`), so both halves now ask the same question. ## What changed 1. **`bump-pr-size-callers.yml` / `bump-cursor-review-callers.yml`** — `- '!scripts/check-pr-size/*_test.go'` added to `paths:`, mirroring pr-risk's existing exclusion, plus a `WATCHED_PATHSPECS` block that mirrors the filter exclusions included. `WATCHED_ASSETS` is deliberately **kept**: the pathspec list supersedes its staleness comparison, not its decommission probes. 2. **`test_paths_contract.sh`** — the flat `!`-rejection becomes an equivalence check. A preflight fleet whose filter carries `!` must set `WATCHED_PATHSPECS` (a missing one still fails, naming the freeze the old rejection prevented), and the two are compared as sets: positives normalized `x/**` → `x`, negations as `:(exclude)x` with the glob kept **verbatim**, no extras in either direction. A fleet with no `!` that sets the input anyway is held to the same equivalence, so it can't drift. 3. **Docs** — README fleet table, the `COUPLED TO THE PATH FILTER` note, and AGENTS.md's fleet list. This relaxation is also precisely what the future **BE-6475** pr-risk-onto-preflight swap needs. That swap is **not** done here. ## Reconciliation of #152 and #153 (please review this specifically) The ticket assumed #152 and #153 would already be merged and asked me to verify the reconciliation hadn't dropped either feature. They are still open and their `preflight.sh` edits conflict, so this PR performs that reconciliation. It was **not** mechanical — #152 was written when `WATCHED_ASSETS` was still a scalar: - The pathspec **coverage guard** now requires the list to reach **every** `WATCHED_ASSETS` entry, not the value as a whole. cursor-review watches two directories; checking them jointly would have been strictly weaker than the OID comparison this input supersedes. It also matches a **file** asset as itself — an entry may be a file, and #152's prefix-only test would have failed those with an error the pathspec list cannot fix. - The staleness `elif` uses #153's per-entry `assets_changed`; #152's scalar `tip_assets`/`here_assets` no longer exist. - #153's "name the surface that moved" diagnostic is carried **into** the pathspec branch, so the fleets that need exclusions aren't left with a worse diagnostic than the ones that don't. - Header/README now document the three list inputs' genuinely different comment and empty-value rules instead of a blanket "both list inputs" claim that had become wrong. ## Verification `shellcheck -x` (all 5 scripts) and `actionlint` clean. | Suite | Result | |---|---| | `test_preflight.sh` | **278 passed**, 0 failed (261 before) | | `test_paths_contract.sh` | **40 passed**, 0 failed (29 before) | | `test_bump_callers.sh` | **185 passed**, 0 failed | The new preflight case pair runs this fleet's literal shape against a real git repo: a `*_test.go`-only commit reads **unchanged** and re-points; **the same commit under the old tree-OID config false-stales** (the freeze, reproduced, so the input is necessary rather than cosmetic); and `size.go`, `go.mod`, the reusable itself, and a code+test commit all still read as changed. New contract fixtures pin the rejections the real entrypoints never exercise — dropped exclusion, extra exclusion, missing positive, an exclusion normalized to its parent dir, and a raw `!` entry not translated to `:(exclude)`. **Falsified the load-bearing premise rather than trusting it.** The change denies bump PRs on the claim that pinned callers never execute these tests, so I went and checked: the only `go test` in the repo is this repo's **own** CI job (`test-pr-size.yml:55`); `pr-size.yml:264` and `cursor-review.yml:489` both `go build -o … .` and run the binary, and `go build` does not compile `_test.go` files at all. `test-pr-size.yml` still watches `scripts/check-pr-size/**` with **no** exclusion, so the tests keep running on every change here — only the consumer-facing churn goes away. ## Judgment calls and known limits - **The ticket's precondition is unmet and I stacked instead of waiting.** #151/#152/#153 are all still open (none closed unmerged). Per the standing always-stack directive I built on the real branches rather than blocking. Consequence: this PR's diff double-counts the foundations until they land, and the #152/#153 reconciliation above is mine, not a merge artifact — it deserves review attention on its own. - **The GitHub-trigger half is reasoned, not executed.** I cannot run Actions from here, so "a test-only commit matches no trigger" rests on documented `paths:` semantics plus the pr-risk precedent already in production (`bump-pr-risk-callers.yml:71-72`). The **preflight** half is executed, against a real repo, in the new case pair. - **Known `*` asymmetry, benign and currently unreachable.** GitHub's `*` does not cross `/`; git's pathspec `*` does (measured). So a hypothetical `scripts/check-pr-size/sub/foo_test.go` would still fire the trigger while preflight treated it as unchanged. The directory is flat today (`go.mod`, `main.go`, `main_test.go`, `size.go`, `size_test.go`), and the direction is safe — it can only re-point to a commit consumers don't execute, never freeze the fleet. I kept the literal pathspec the ticket and spike BE-7083 specified rather than switching to `:(glob)` magic, which would also have broken the verbatim-glob equivalence the contract test is specified to enforce. - The `!`-without-`WATCHED_PATHSPECS` guard is covered through its trigger condition (an absent input parses empty) rather than a full synthetic entrypoint; driving the whole file loop from a fixture would have meant restructuring code this ticket puts out of scope. ## Out of scope (per the ticket) BE-6475's pr-risk migration onto preflight, roster contents, and any change to `bump-callers.sh`. --- <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:55 +00:00