[GH-ISSUE #214] [groom] Four bump fleets watch their whole asset directory, tests and README included, so a test-only commit fans a churn bump PR across the fleet #2

New issue

Open

opened 2026-10-26 22:34:21 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:34:21 +00:00

Owner

Originally created by @cloud-code-bot[bot] on GitHub (Aug 24, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/214

Groom auto-builder — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · run

Verified

Fleets whose push paths filter carries the exclusions: bump-area-label (35-36), bump-linear-ticket (41-42), bump-pr-risk (87-88), bump-pr-derisk (106-109), bump-public-repo-hygiene (46-47) - each excluding the tool tests tree and its README.md, with the documented reason that a test-only or docs-only commit would fan a pure-churn SHA-bump PR across the whole fleet. bump-pr-size excludes scripts/check-pr-size/*_test.go with a five-line rationale (that tree has no README). So the comparison set is five fleets excluding tests+README and one excluding tests only - the finder said six excluding exactly those paths, which is close but not exact.

Fleets that have an asset directory and NO exclusion:

bump-agents-md-callers.yml:48-50 - .github/agents-md-integrity/**, which contains tests/ and README.md.
bump-coderabbit-config-callers.yml:60-62 - .github/coderabbit-config/**, same.
bump-groom-callers.yml:58-65 - .github/groom/**, whose tests/ holds 9 files.
bump-cursor-review-callers.yml:56-72 - inconsistent inside one file: it excludes scripts/check-pr-size/*_test.go but pulls in .github/cursor-review/** (which contains tests/ and README.md) with no exclusion.

The remaining three fleets (auto-label, detect-unreviewed-merge, assign-reviewers) watch a workflow file only and are out of scope.

Impact (counterfactual)

A tests-only or README-only commit under those directories would start a bump run that mints the org-wide App token, force-resets the fleet stable ci/bump-* branch and opens a review round in every enrolled caller repo for a diff with no consumer-visible behaviour change.

Fix

Add the two negations to the four filters, and in the SAME change move those fleets preflight inputs across: preflight.sh header is explicit that when a filter carries exclude entries, the bare WATCHED_ASSETS tree-OID comparison reads a test-only commit as changed-since and produces a FALSE stale verdict that freezes the fleet - so these four need WATCHED_PATHSPECS mirroring the filter (exclusions included) plus WATCHED_EXEC, exactly as bump-pr-risk, bump-pr-size and bump-cursor-review already do for their excluded trees. .github/bump-callers/tests/test_paths_contract.sh enforces the mirror in both directions, so a half-done change fails there rather than silently.

Risk

Low ONLY IF the preflight inputs move at the same time; doing the filter alone is the documented fleet-freeze failure mode. That makes this more than add-two-lines-times-four - treat the pathspec migration as the main body of work.

Steelman

For groom the exclusion is least obviously right, because .github/groom mixes executed prose (the finder/verifier/builder briefs, which consumers DO run at the pinned SHA) with tests; but that argues for excluding tests/ and README.md specifically, which is exactly the proposal.

Originally created by @cloud-code-bot[bot] on GitHub (Aug 24, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/214 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, or build/test config that executes in pre-review CI) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/32712056525) ## Verified Fleets whose push paths filter carries the exclusions: bump-area-label (35-36), bump-linear-ticket (41-42), bump-pr-risk (87-88), bump-pr-derisk (106-109), bump-public-repo-hygiene (46-47) - each excluding the tool tests tree and its README.md, with the documented reason that a test-only or docs-only commit would fan a pure-churn SHA-bump PR across the whole fleet. bump-pr-size excludes scripts/check-pr-size/*_test.go with a five-line rationale (that tree has no README). So the comparison set is five fleets excluding tests+README and one excluding tests only - the finder said six excluding exactly those paths, which is close but not exact. Fleets that have an asset directory and NO exclusion: * bump-agents-md-callers.yml:48-50 - .github/agents-md-integrity/**, which contains tests/ and README.md. * bump-coderabbit-config-callers.yml:60-62 - .github/coderabbit-config/**, same. * bump-groom-callers.yml:58-65 - .github/groom/**, whose tests/ holds 9 files. * bump-cursor-review-callers.yml:56-72 - inconsistent inside one file: it excludes scripts/check-pr-size/*_test.go but pulls in .github/cursor-review/** (which contains tests/ and README.md) with no exclusion. The remaining three fleets (auto-label, detect-unreviewed-merge, assign-reviewers) watch a workflow file only and are out of scope. ## Impact (counterfactual) A tests-only or README-only commit under those directories would start a bump run that mints the org-wide App token, force-resets the fleet stable ci/bump-* branch and opens a review round in every enrolled caller repo for a diff with no consumer-visible behaviour change. ## Fix Add the two negations to the four filters, and in the SAME change move those fleets preflight inputs across: preflight.sh header is explicit that when a filter carries exclude entries, the bare WATCHED_ASSETS tree-OID comparison reads a test-only commit as changed-since and produces a FALSE stale verdict that freezes the fleet - so these four need WATCHED_PATHSPECS mirroring the filter (exclusions included) plus WATCHED_EXEC, exactly as bump-pr-risk, bump-pr-size and bump-cursor-review already do for their excluded trees. .github/bump-callers/tests/test_paths_contract.sh enforces the mirror in both directions, so a half-done change fails there rather than silently. ## Risk Low ONLY IF the preflight inputs move at the same time; doing the filter alone is the documented fleet-freeze failure mode. That makes this more than add-two-lines-times-four - treat the pathspec migration as the main body of work. ## Steelman For groom the exclusion is least obviously right, because .github/groom mixes executed prose (the finder/verifier/builder briefs, which consumers DO run at the pinned SHA) with tests; but that argues for excluding tests/ and README.md specifically, which is exactly the proposal.

bsakaguchi added the

groom

label

2026-10-26 22:34:21 +00:00

bsakaguchi commented

2026-10-26 22:34:24 +00:00

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: confirmed, and it fired on the tip of main today. Leaving this open, tracked internally.

Re-derived the census against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b; the four-fleet defect set is unchanged and the body's line numbers are still exact.

A live reproduction, not a counterfactual. cf7b8d9 ("test(groom): prove name resolution is dead in the sandbox, non-vacuously", #300) — the current tip — touches exactly two files: .github/groom/README.md and .github/groom/tests/sandbox-tests.sh. Pure tests+README. bump-groom-callers.yml:65 watches .github/groom/** with no exclusion and the commit carries no skip trailer, so it fired a churn bump. The "Impact (counterfactual)" section can be promoted to an observation.

Still-unfixed defect set: agents-md :48-50, coderabbit-config :60-62, groom :58-65, cursor-review :56-82 (which excludes scripts/check-pr-size/*_test.go at :82 but pulls in .github/cursor-review/** unexcluded). Your comparison set is right too, including the note that pr-size excludes tests only because that tree has no README — confirmed, it holds only go.mod, main.go, main_test.go, size.go, size_test.go.

The freeze mechanic is real and still documented at preflight.sh:60-74 and :1128-1152, and test_paths_contract.sh still enforces the mirror in both directions (:278-281 hard-fails on a ! entry with no WATCHED_PATHSPECS; :294-300 set-equivalence). One softening: because that test runs in CI, a filter-alone change is a red build, not a fleet outage. The sequencing advice stands; the severity is lower than "fleet-freeze".

Two corrections to the fix prescription:

The body says these four need WATCHED_EXEC "exactly as bump-pr-risk, bump-pr-size and bump-cursor-review already do". pr-size and cursor-review do not set WATCHED_EXEC — they set WATCHED_PATHSPECS only. And test_paths_contract.sh never mentions WATCHED_EXEC at all, so nothing enforces it. That is a separate defect (tracked separately, see #248) and is not required by this fix.
.github/groom/tests/ holds 10 files, not 9. Trivial, but the count is in the body.

A trap worth knowing before anyone writes this. test_paths_contract.sh:316-330 intends to skip /** directory exclusions — its comment says "File globs only. A /** directory exclusion … cannot diverge this way". But the guard is case "$negbase" in *'*'*) ;; *) continue ;; esac with negbase="**", and ** matches *'*'*, so it is not skipped; it then runs find . -mindepth 2 -name "**", which matches every ordinary filename. .github/cursor-review/tests/ contains fixtures/catalog-2026-08-10.txt at depth 2 — so adding !.github/cursor-review/tests/** would fail at :328 with a misleading message. The five existing …/tests/** exclusions pass only because those trees happen to be flat, as do the other three here; only cursor-review trips it. This was established by reading the script and checking both bash primitives in isolation — the suite was not run, so confirming it is the first task of the tracked work rather than a claim.

Also since filing: the Skip-caller-bump: trailer landed (#235, hardened by #264) as a per-commit opt-out. It has never been used by any commit, including cf7b8d9, so it does not close this — but it is a complementary remediation worth weighing.

Reopen or comment if the census or the corrections miss something.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: confirmed, and it fired on the tip of `main` today. Leaving this open, tracked internally.** Re-derived the census against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`; the four-fleet defect set is unchanged and the body's line numbers are still exact. **A live reproduction, not a counterfactual.** `cf7b8d9` ("test(groom): prove name resolution is dead in the sandbox, non-vacuously", #300) — the current tip — touches exactly two files: `.github/groom/README.md` and `.github/groom/tests/sandbox-tests.sh`. Pure tests+README. `bump-groom-callers.yml:65` watches `.github/groom/**` with no exclusion and the commit carries no skip trailer, so it fired a churn bump. The "Impact (counterfactual)" section can be promoted to an observation. Still-unfixed defect set: agents-md `:48-50`, coderabbit-config `:60-62`, groom `:58-65`, cursor-review `:56-82` (which excludes `scripts/check-pr-size/*_test.go` at `:82` but pulls in `.github/cursor-review/**` unexcluded). Your comparison set is right too, including the note that pr-size excludes tests only because that tree has no README — confirmed, it holds only `go.mod`, `main.go`, `main_test.go`, `size.go`, `size_test.go`. The freeze mechanic is real and still documented at `preflight.sh:60-74` and `:1128-1152`, and `test_paths_contract.sh` still enforces the mirror in both directions (`:278-281` hard-fails on a `!` entry with no `WATCHED_PATHSPECS`; `:294-300` set-equivalence). One softening: because that test runs in CI, a filter-alone change is a **red build**, not a fleet outage. The sequencing advice stands; the severity is lower than "fleet-freeze". **Two corrections to the fix prescription:** 1. The body says these four need `WATCHED_EXEC` "exactly as bump-pr-risk, bump-pr-size and bump-cursor-review already do". **pr-size and cursor-review do not set `WATCHED_EXEC`** — they set `WATCHED_PATHSPECS` only. And `test_paths_contract.sh` never mentions `WATCHED_EXEC` at all, so nothing enforces it. That is a separate defect (tracked separately, see #248) and is **not** required by this fix. 2. `.github/groom/tests/` holds 10 files, not 9. Trivial, but the count is in the body. **A trap worth knowing before anyone writes this.** `test_paths_contract.sh:316-330` intends to skip `/**` directory exclusions — its comment says "File globs only. A `/**` directory exclusion … cannot diverge this way". But the guard is `case "$negbase" in *'*'*) ;; *) continue ;; esac` with `negbase="**"`, and `**` *matches* `*'*'*`, so it is not skipped; it then runs `find . -mindepth 2 -name "**"`, which matches every ordinary filename. `.github/cursor-review/tests/` contains `fixtures/catalog-2026-08-10.txt` at depth 2 — so adding `!.github/cursor-review/tests/**` would fail at `:328` with a misleading message. The five existing `…/tests/**` exclusions pass only because those trees happen to be flat, as do the other three here; only cursor-review trips it. This was established by reading the script and checking both bash primitives in isolation — **the suite was not run**, so confirming it is the first task of the tracked work rather than a claim. Also since filing: the `Skip-caller-bump:` trailer landed (#235, hardened by #264) as a per-commit opt-out. It has **never been used by any commit**, including `cf7b8d9`, so it does not close this — but it is a complementary remediation worth weighing. Reopen or comment if the census or the corrections miss something. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:24 +00:00

[PR #2] [MERGED] fix(cursor-review): correct Matt's handle in slack-notify DM map #12

bsakaguchi referenced this issue

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

[PR #27] [MERGED] ci(bump-callers): move caller list to CURSOR_REVIEW_CALLERS variable + mask repo names #37