[PR #308] [MERGED] test(bump-callers): make the glob-flatness guard skip `/**` directory exclusions, and self-test it #308

New issue

Closed

opened 2026-10-26 22:38:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:19 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/308
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15266-paths-contract-flatness-guard

📝 Commits (2)

4619bfb test(bump-callers): make the glob-flatness guard skip /** directory exclusions, and self-test it
a031063 test(bump-callers): measure the glob-flatness guard against the path, and make a refusal to measure say so

📊 Changes

2 files changed (+348 additions, -26 deletions)

View changed files

📝 .github/bump-callers/README.md (+25 -2)
📝 .github/bump-callers/tests/test_paths_contract.sh (+323 -24)

📄 Description

ELI-5

test_paths_contract.sh guards one sneaky way a workflow's paths: filter and its
git-pathspec mirror can look identical but mean different things: a filter's * stops at
a /, git's does not. The guard was measuring the wrong thing in two ways. It claimed to
skip /** directory exclusions, which cannot have that problem — but did not, because
** looked like a file glob to it and find -name '**' matches every file on earth, so
any /** exclusion over a directory that merely had a subdirectory failed the build
over a file glob it does not have. And it compared each file's basename where git
compares the whole path, so for a prefix-anchored exclusion like !x/test_*.sh it
missed real divergences and invented fake ones. This teaches it to measure what git
measures, to distinguish a proof from an admission from a failure, and adds fixtures for
all of it.

What changed

glob_exclusion_deep_matches() — the guard, extracted out of the per-fleet loop.
It matches the whole relative path through bash case (fnmatch without
FNM_PATHNAME — the matcher semantics git uses), not find -name, and treats ? and
[ as the globs they are on both sides.
Four verdicts, not "0 / not 0" — because a refusal to measure must never read as a
measurement:
- 0 MEASURED — stdout is the match list (LC_ALL=C sorted, capped at 3); empty
  stdout is the positive assertion.
- 1 EQUIVALENT — a proof that this shape cannot diverge: ** anywhere in the
  basename, or a literal file name.
- 2 UNMEASURED — an admission: a glob in the directory half (!x/*/tests/**
  genuinely can diverge), an absent directory, or a literal directory
  (!x/tests matches only a file named that, while :(exclude)x/tests drops the whole
  subtree). The two UNMEASURED checks run before the basename rules, since the **
  rule holds only for a fixed, present directory.
- 3 ERROR — the walk failed; a hard failure at the caller, never a skip. There is
  no head in the pipeline any more, so no SIGPIPE status can be laundered into a
  verdict, and no 2>/dev/null swallows the diagnostic.
-type f and a .git skip — neither a directory nor anything under .git can make
a paths: trigger and a git diff pathspec disagree, so neither may be reported as one.
Fourteen self-test fixtures, driving every verdict, plus a skip() helper so the
fleet loop prints a line for every exclusion — an unmeasured one can no longer look
like one that measured clean. A skip counts as neither pass nor fail, so a widening skip
rule cannot inflate the pass total while asserting less.
.github/bump-callers/README.md — the "Keep a * exclusion's directory flat" rule
now states its actual scope (globs only, path not basename, unmeasured ≠ clean) and
carries the trigger caveat below.

No .github/workflows/* file changes.

Why the pass count moves 110 → 118

The six pre-existing /** negations were producing ok: lines that asserted nothing —
they passed only because those directories happen to be flat, not because the guard had
decided anything. They are now skip: lines, leaving 104 from the real entrypoints;
the fourteen guard fixtures bring the total to 118. Across the tree today: twelve
exclusions skip (six /**, six literal README.md) and two are genuinely measured
(!scripts/check-pr-size/*_test.go, in the pr-size and cursor-review fleets).

Verification of the semantics, against git rather than the docs

Every claim about git's half is empirical. In a scratch repo holding x/foo/bar.go,
x/sub/b_test.go, x/sub/test_a.sh, x/test_dir/b.sh, x/top_test.go:

git ls-files -- . ':(exclude)x/test_*.sh'   → x/test_dir/b.sh GONE, x/sub/test_a.sh KEPT
git ls-files -- . ':(exclude)x/foo?bar.go'  → x/foo/bar.go GONE          (`?` crosses `/`)
git ls-files -- . ':(exclude)x/*_test.go'   → BOTH _test.go GONE, at either depth

The first line is both halves of the basename bug in one measurement: git does exclude
x/test_dir/b.sh (its * spans dir/b) while the filter does not — a real divergence a
basename test misses — and git does not exclude x/sub/test_a.sh, whose basename
matches but whose path lacks the literal x/test_ prefix — a false divergence a basename
test invents. And in this repo, for the /** claim:

git ls-files -- .github/cursor-review                                            → 10 files under tests/
git ls-files -- .github/cursor-review ':(exclude).github/cursor-review/tests'    → 0
git ls-files -- .github/cursor-review ':(exclude).github/cursor-review/tests/**' → 0

The paths:-filter half rests on GitHub's documented ** semantics — see Residual.

Non-vacuity

All fourteen fixtures fail on the implementation they replace: eleven with the
previous commit's guard in place (each on a distinct verdict — got caught, want equivalent; reported './sub/test_a.sh' rather than 'test_dir/b.sh'; got equivalent, want unmeasured ×3; got clean, want error; filesystem-order match lists), and the
remaining three were already non-vacuous against the pre-PR guard. Removing the .git
skip alone turns the bare-root fixture red. Reverting the original one-line ** fix turns
the regression fixture red.

Corpus sweep, including the half this PR does not fix

Nine tests/ trees across the repo are candidates for a /** exclusion. Exactly one
— .github/cursor-review/tests, via fixtures/ — has a subdirectory today and would trip
the pre-fix guard; the other eight are flat and would pass vacuously. That
distribution is why the bug survived: every /** negation in the tree today is in the
vacuous majority.

Reproduction of the reported failure

With the guard fix reverted and !.github/cursor-review/tests/** +
:(exclude).github/cursor-review/tests added to bump-cursor-review-callers.yml, the
suite fails exactly as reported — '.github/cursor-review/tests' now holds '**' in a SUBDIRECTORY (./fixtures/catalog-2026-08-10.txt ). With the fix and the same mutation it
is green. Both mutations were reverted; the diff touches no workflow file.

Residual

The paths:-filter half is documentation-grounded, not executed. Git's half is
measured above, but nothing here can run GitHub Actions' own filter matcher locally, so
"** matches all characters including /" is taken from GitHub's documented paths:
semantics. That is precisely why a literal directory exclusion is now reported
UNMEASURED rather than turned red: the divergence is real by the documentation, but the
filter half of it cannot be executed here, so the guard declines to fail a config it
cannot check instead of risking a false red.
The measurement only runs when its suite runs. test-bump-callers.yml is
path-filtered to .github/bump-callers/** plus the bump-*-callers.yml entrypoints, so
a PR that adds only a deep test file under a watched tool directory — the very PR that
creates this divergence — does not run the guard, which first fires on the next PR that
does touch this directory. Raised in review, deferred deliberately (it is a change to
the test workflow's trigger, with real CI-cost trade-offs, not to what the guard
measures) and proposed as a follow-up. The README now states the caveat rather than
claiming the guard "fails the build the day it becomes true" unqualified.
A tracked-only walk was declined. The walk is find -type f, not git ls-files:
restricting it to tracked paths would blind the guard to an untracked deep test file,
which is the thing it exists to catch the moment someone creates one, and the fixtures
live in a mktemp -d tree that is not a git repo, so it would need a second code path
nothing exercises. Residual exposure is a local-only false red from an
untracked-or-gitignored scratch file at depth ≥ 2 under scripts/check-pr-size/; it
names the offending file and cannot reach CI.
Only one of the four fleet diffs this unblocks was exercised. The acceptance
criterion names the cursor-review mutation and that is the one reproduced and re-verified
here. The agents-md-integrity, coderabbit-config and groom fleets in
Comfy-Org/github-workflows#214 were not mutated and re-run; their tests/ trees are flat
today (swept above), so their /** negations would have passed the pre-fix guard
vacuously rather than failing it — meaning this fix is a prerequisite for them only if
one of those trees later gains a subdirectory. The four-fleet filter +
WATCHED_PATHSPECS migration itself is out of scope here and remains open as #214.
The spike this was written from was not read directly. Its evidence reached this PR
second-hand through the ticket body; the spike's own transcript and its findings comment
were not accessible from this environment, so the "exactly six vacuous lines" claim was
re-derived here from scratch rather than taken on trust (it reproduces).

Provenance

Authored by: agent-work loop
Verified: shellcheck -x over all five bump-callers scripts+tests: clean;
test_paths_contract.sh: 118 passed, 0 failed (104 from the real entrypoints + 14 guard
fixtures), with 12 skip: lines and 2 real measurements across the fleets;
test_bump_callers.sh: 437 passed, 0 failed; test_preflight.sh: 445 passed, 0 failed;
check_workflow_pins.py: OK, 11 workflows, 0 exempt; check_agents_md.py --root .:
passed (2 pre-existing warnings, unrelated). Non-vacuity proven by splicing the previous
implementation back under the new fixtures: 107 passed, 11 failed. Git's pathspec
semantics for *, ? and /** measured directly (above), not assumed.
Deviations: all four acceptance criteria met. One review finding deferred rather than
fixed — the test workflow's paths: filter does not cover the watched tool surfaces, so
the guard does not run on the PR that would create the divergence; it is proposed as a
follow-up and documented in the README and in Residual above. The git ls-files half of
one nit was declined, with the exposure stated in Residual.

_{🔄 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/308 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15266-paths-contract-flatness-guard` --- ### 📝 Commits (2) - [`4619bfb`](https://github.com/Comfy-Org/github-workflows/commit/4619bfbab49295bc56a744027b543b7a5d593a3c) test(bump-callers): make the glob-flatness guard skip `/**` directory exclusions, and self-test it - [`a031063`](https://github.com/Comfy-Org/github-workflows/commit/a0310633eae46083f6f44e586a32b8d87f907141) test(bump-callers): measure the glob-flatness guard against the path, and make a refusal to measure say so ### 📊 Changes **2 files changed** (+348 additions, -26 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+25 -2) 📝 `.github/bump-callers/tests/test_paths_contract.sh` (+323 -24) </details> ### 📄 Description ## ELI-5 `test_paths_contract.sh` guards one sneaky way a workflow's `paths:` filter and its git-pathspec mirror can look identical but mean different things: a filter's `*` stops at a `/`, git's does not. The guard was measuring the wrong thing in two ways. It claimed to skip `/**` *directory* exclusions, which cannot have that problem — but did not, because `**` looked like a file glob to it and `find -name '**'` matches every file on earth, so any `/**` exclusion over a directory that merely *had* a subdirectory failed the build over a file glob it does not have. And it compared each file's **basename** where git compares the whole **path**, so for a prefix-anchored exclusion like `!x/test_*.sh` it missed real divergences and invented fake ones. This teaches it to measure what git measures, to distinguish a proof from an admission from a failure, and adds fixtures for all of it. ## What changed - **`glob_exclusion_deep_matches()`** — the guard, extracted out of the per-fleet loop. It matches the whole relative **path** through bash `case` (fnmatch *without* FNM_PATHNAME — the matcher semantics git uses), not `find -name`, and treats `?` and `[` as the globs they are on both sides. - **Four verdicts, not "0 / not 0"** — because a refusal to measure must never read as a measurement: - `0` **MEASURED** — stdout is the match list (`LC_ALL=C` sorted, capped at 3); empty stdout is the positive assertion. - `1` **EQUIVALENT** — a proof that this shape cannot diverge: `**` anywhere in the basename, or a literal file name. - `2` **UNMEASURED** — an admission: a glob in the directory half (`!x/*/tests/**` genuinely *can* diverge), an absent directory, or a literal *directory* (`!x/tests` matches only a file named that, while `:(exclude)x/tests` drops the whole subtree). The two UNMEASURED checks run **before** the basename rules, since the `**` rule holds only for a fixed, present directory. - `3` **ERROR** — the walk failed; a hard failure at the caller, never a skip. There is no `head` in the pipeline any more, so no SIGPIPE status can be laundered into a verdict, and no `2>/dev/null` swallows the diagnostic. - **`-type f` and a `.git` skip** — neither a directory nor anything under `.git` can make a `paths:` trigger and a `git diff` pathspec disagree, so neither may be reported as one. - **Fourteen self-test fixtures**, driving every verdict, plus a `skip()` helper so the fleet loop prints a line for *every* exclusion — an unmeasured one can no longer look like one that measured clean. A skip counts as neither pass nor fail, so a widening skip rule cannot inflate the pass total while asserting less. - **`.github/bump-callers/README.md`** — the "Keep a `*` exclusion's directory flat" rule now states its actual scope (globs only, path not basename, unmeasured ≠ clean) and carries the trigger caveat below. No `.github/workflows/*` file changes. ## Why the pass count moves 110 → 118 The six pre-existing `/**` negations were producing `ok:` lines that asserted nothing — they passed only because those directories happen to be flat, not because the guard had decided anything. They are now `skip:` lines, leaving **104** from the real entrypoints; the fourteen guard fixtures bring the total to **118**. Across the tree today: twelve exclusions skip (six `/**`, six literal `README.md`) and two are genuinely measured (`!scripts/check-pr-size/*_test.go`, in the pr-size and cursor-review fleets). ## Verification of the semantics, against git rather than the docs Every claim about git's half is empirical. In a scratch repo holding `x/foo/bar.go`, `x/sub/b_test.go`, `x/sub/test_a.sh`, `x/test_dir/b.sh`, `x/top_test.go`: ``` git ls-files -- . ':(exclude)x/test_*.sh' → x/test_dir/b.sh GONE, x/sub/test_a.sh KEPT git ls-files -- . ':(exclude)x/foo?bar.go' → x/foo/bar.go GONE (`?` crosses `/`) git ls-files -- . ':(exclude)x/*_test.go' → BOTH _test.go GONE, at either depth ``` The first line is both halves of the basename bug in one measurement: git *does* exclude `x/test_dir/b.sh` (its `*` spans `dir/b`) while the filter does not — a real divergence a basename test misses — and git does *not* exclude `x/sub/test_a.sh`, whose basename matches but whose path lacks the literal `x/test_` prefix — a false divergence a basename test invents. And in this repo, for the `/**` claim: ``` git ls-files -- .github/cursor-review → 10 files under tests/ git ls-files -- .github/cursor-review ':(exclude).github/cursor-review/tests' → 0 git ls-files -- .github/cursor-review ':(exclude).github/cursor-review/tests/**' → 0 ``` The `paths:`-filter half rests on GitHub's documented `**` semantics — see Residual. ## Non-vacuity All fourteen fixtures fail on the implementation they replace: **eleven** with the previous commit's guard in place (each on a distinct verdict — `got caught, want equivalent`; `reported './sub/test_a.sh' rather than 'test_dir/b.sh'`; `got equivalent, want unmeasured` ×3; `got clean, want error`; filesystem-order match lists), and the remaining three were already non-vacuous against the pre-PR guard. Removing the `.git` skip alone turns the bare-root fixture red. Reverting the original one-line `**` fix turns the regression fixture red. ## Corpus sweep, including the half this PR does not fix Nine `tests/` trees across the repo are candidates for a `/**` exclusion. Exactly **one** — `.github/cursor-review/tests`, via `fixtures/` — has a subdirectory today and would trip the pre-fix guard; the other **eight** are flat and would pass vacuously. That distribution is why the bug survived: every `/**` negation in the tree today is in the vacuous majority. ## Reproduction of the reported failure With the guard fix reverted and `!.github/cursor-review/tests/**` + `:(exclude).github/cursor-review/tests` added to `bump-cursor-review-callers.yml`, the suite fails exactly as reported — `'.github/cursor-review/tests' now holds '**' in a SUBDIRECTORY (./fixtures/catalog-2026-08-10.txt )`. With the fix and the same mutation it is green. Both mutations were reverted; the diff touches no workflow file. ## Residual - **The `paths:`-filter half is documentation-grounded, not executed.** Git's half is measured above, but nothing here can run GitHub Actions' own filter matcher locally, so "`**` matches all characters including `/`" is taken from GitHub's documented `paths:` semantics. That is precisely why a literal *directory* exclusion is now reported UNMEASURED rather than turned red: the divergence is real by the documentation, but the filter half of it cannot be executed here, so the guard declines to fail a config it cannot check instead of risking a false red. - **The measurement only runs when its suite runs.** `test-bump-callers.yml` is path-filtered to `.github/bump-callers/**` plus the `bump-*-callers.yml` entrypoints, so a PR that adds only a deep test file under a *watched tool* directory — the very PR that creates this divergence — does not run the guard, which first fires on the next PR that does touch this directory. Raised in review, deferred deliberately (it is a change to the test workflow's trigger, with real CI-cost trade-offs, not to what the guard measures) and proposed as a follow-up. The README now states the caveat rather than claiming the guard "fails the build the day it becomes true" unqualified. - **A tracked-only walk was declined.** The walk is `find -type f`, not `git ls-files`: restricting it to tracked paths would blind the guard to an untracked deep test file, which is the thing it exists to catch the moment someone creates one, and the fixtures live in a `mktemp -d` tree that is not a git repo, so it would need a second code path nothing exercises. Residual exposure is a local-only false red from an untracked-or-gitignored scratch file at depth ≥ 2 under `scripts/check-pr-size/`; it names the offending file and cannot reach CI. - **Only one of the four fleet diffs this unblocks was exercised.** The acceptance criterion names the cursor-review mutation and that is the one reproduced and re-verified here. The `agents-md-integrity`, `coderabbit-config` and `groom` fleets in Comfy-Org/github-workflows#214 were not mutated and re-run; their `tests/` trees are flat today (swept above), so their `/**` negations would have passed the pre-fix guard vacuously rather than failing it — meaning this fix is a prerequisite for them only if one of those trees later gains a subdirectory. The four-fleet filter + `WATCHED_PATHSPECS` migration itself is out of scope here and remains open as #214. - **The spike this was written from was not read directly.** Its evidence reached this PR second-hand through the ticket body; the spike's own transcript and its findings comment were not accessible from this environment, so the "exactly six vacuous lines" claim was re-derived here from scratch rather than taken on trust (it reproduces). ## Provenance - **Authored by:** agent-work loop - **Verified:** `shellcheck -x` over all five bump-callers scripts+tests: clean; `test_paths_contract.sh`: 118 passed, 0 failed (104 from the real entrypoints + 14 guard fixtures), with 12 `skip:` lines and 2 real measurements across the fleets; `test_bump_callers.sh`: 437 passed, 0 failed; `test_preflight.sh`: 445 passed, 0 failed; `check_workflow_pins.py`: OK, 11 workflows, 0 exempt; `check_agents_md.py --root .`: passed (2 pre-existing warnings, unrelated). Non-vacuity proven by splicing the previous implementation back under the new fixtures: 107 passed, 11 failed. Git's pathspec semantics for `*`, `?` and `/**` measured directly (above), not assumed. - **Deviations:** all four acceptance criteria met. One review finding deferred rather than fixed — the test workflow's `paths:` filter does not cover the watched tool surfaces, so the guard does not run on the PR that would create the divergence; it is proposed as a follow-up and documented in the README and in Residual above. The `git ls-files` half of one nit was declined, with the exposure stated in Residual. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:19 +00:00