[GH-ISSUE #248] [groom] Five bump fleets watch an asset DIRECTORY kept alive by tests/ and README.md with no WATCHED_EXEC, so deleting an executed script would still read as a healthy surface #6

New issue

Open

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

bsakaguchi commented

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

Owner

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

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

Verified

preflight.sh:854-861 documents the failure mode in its own words: a per-file fleet's directory can OUTLIVE the scripts inside it - scripts/pr-risk still exists once tests/ and README.md are all that is left - so a directory probe would report the surface healthy and bump every caller onto a SHA where the graders it executes are gone. WATCHED_EXEC (documented at preflight.sh:129-138, implemented at :862-892) exists for exactly that, and a miss takes the ::warning:: decommission path instead of bumping.

Fleets that DO set WATCHED_EXEC: area-label, linear-ticket, pr-risk, pr-derisk and public-repo-hygiene (bump-public-repo-hygiene-callers.yml:91-93). The finder said four and missed public-repo-hygiene - correct the count to five.

Fleets whose WATCHED_ASSETS names a DIRECTORY and that set no WATCHED_EXEC (verified by reading each preflight step):

bump-agents-md-callers.yml:101 - .github/agents-md-integrity (holds check_agents_md.py, README.md, tests/)
bump-coderabbit-config-callers.yml:113 - .github/coderabbit-config (check_coderabbit_config.py, schema_drift.py, schema.v2.json, requirements.txt, README.md, tests/)
bump-cursor-review-callers.yml:145-147 - .github/cursor-review AND scripts/check-pr-size
bump-groom-callers.yml:117 - .github/groom (15 entries, of which README.md and tests/ are not executed)
bump-pr-size-callers.yml:125 - scripts/check-pr-size, whose own paths: filter excludes *_test.go, so the files that would keep the directory alive are ones the fleet deliberately does not watch

Each of those directories would retain README.md and/or tests/ after every executed file was deleted, so the tree-existence probe would still resolve. Deleting e.g. .github/cursor-review/post-review.py or .github/groom/ledger.py could therefore let the fleet re-pin every enrolled caller to that SHA, and each consumer's reusable would hard-fail at run time with no signal from this side. Inferred from the code paths above; no incident is claimed.

The three fleets with no asset directory at all (auto-label, detect-unreviewed-merge, assign-reviewers) say so in their own comments and are correctly exempt.

Fix

Add WATCHED_EXEC to the five entrypoints, listing the files a pinned caller actually executes - the same audit bump-pr-risk-callers.yml:160-183 already walked, which found its inherited list was short of what a caller runs. Plain repo-relative FILE paths only; preflight.sh rejects a directory there.
Close it structurally: preflight.sh already refuses a set-but-empty WATCHED_EXEC, so extend it to REQUIRE WATCHED_EXEC whenever a WATCHED_ASSETS entry resolves to a directory, mirroring the existing WATCHED_ASSETS / WATCHED_PATHSPECS coverage assertion at :118-126. Land step 1 before step 2, or every affected fleet reds at once.

Risk

Low-medium. A too-broad WATCHED_EXEC makes a legitimate rename read as a decommission and no-ops the fleet behind a ::warning:: - loud and recoverable, unlike the silent bump it replaces. Not security-adjacent: no credential or permission decision changes.

Originally created by @cloud-code-bot[bot] on GitHub (Aug 29, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/248 **Groom auto-builder** — Comfy-Org/github-workflows: this CONFIRMED finding could not be auto-built (patch modifies a CI-privileged or dataset-of-record path (per .github/groom/patch_policy.py — workflow/action defs, lockfiles, build/test config that executes in pre-review CI, graded eval cases under a suites/**/cases/ tree, or a symlink into such a tree) — a human must author these changes), so it is filed for a human. · [run](https://github.com/Comfy-Org/github-workflows/actions/runs/33245434399) ## Verified `preflight.sh:854-861` documents the failure mode in its own words: a per-file fleet's directory can OUTLIVE the scripts inside it - `scripts/pr-risk` still exists once `tests/` and `README.md` are all that is left - so a directory probe would report the surface healthy and bump every caller onto a SHA where the graders it executes are gone. `WATCHED_EXEC` (documented at `preflight.sh:129-138`, implemented at :862-892) exists for exactly that, and a miss takes the `::warning::` decommission path instead of bumping. Fleets that DO set `WATCHED_EXEC`: area-label, linear-ticket, pr-risk, pr-derisk **and public-repo-hygiene** (`bump-public-repo-hygiene-callers.yml:91-93`). The finder said four and missed public-repo-hygiene - correct the count to five. Fleets whose `WATCHED_ASSETS` names a DIRECTORY and that set no `WATCHED_EXEC` (verified by reading each preflight step): * `bump-agents-md-callers.yml:101` - `.github/agents-md-integrity` (holds `check_agents_md.py`, `README.md`, `tests/`) * `bump-coderabbit-config-callers.yml:113` - `.github/coderabbit-config` (`check_coderabbit_config.py`, `schema_drift.py`, `schema.v2.json`, `requirements.txt`, `README.md`, `tests/`) * `bump-cursor-review-callers.yml:145-147` - `.github/cursor-review` AND `scripts/check-pr-size` * `bump-groom-callers.yml:117` - `.github/groom` (15 entries, of which `README.md` and `tests/` are not executed) * `bump-pr-size-callers.yml:125` - `scripts/check-pr-size`, whose own `paths:` filter excludes `*_test.go`, so the files that would keep the directory alive are ones the fleet deliberately does not watch Each of those directories would retain `README.md` and/or `tests/` after every executed file was deleted, so the tree-existence probe would still resolve. Deleting e.g. `.github/cursor-review/post-review.py` or `.github/groom/ledger.py` could therefore let the fleet re-pin every enrolled caller to that SHA, and each consumer's reusable would hard-fail at run time with no signal from this side. Inferred from the code paths above; no incident is claimed. The three fleets with no asset directory at all (auto-label, detect-unreviewed-merge, assign-reviewers) say so in their own comments and are correctly exempt. ## Fix 1. Add `WATCHED_EXEC` to the five entrypoints, listing the files a pinned caller actually executes - the same audit `bump-pr-risk-callers.yml:160-183` already walked, which found its inherited list was short of what a caller runs. Plain repo-relative FILE paths only; `preflight.sh` rejects a directory there. 2. Close it structurally: `preflight.sh` already refuses a set-but-empty `WATCHED_EXEC`, so extend it to REQUIRE `WATCHED_EXEC` whenever a `WATCHED_ASSETS` entry resolves to a directory, mirroring the existing `WATCHED_ASSETS` / `WATCHED_PATHSPECS` coverage assertion at :118-126. Land step 1 before step 2, or every affected fleet reds at once. ## Risk Low-medium. A too-broad `WATCHED_EXEC` makes a legitimate rename read as a decommission and no-ops the fleet behind a `::warning::` - loud and recoverable, unlike the silent bump it replaces. Not security-adjacent: no credential or permission decision changes.

bsakaguchi added the

groom

label

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

bsakaguchi commented

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

Author

Owner

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

Triage: confirmed, still exactly five fleets. Leaving this open, tracked internally.

Re-derived the census independently against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b rather than re-reading the body, and it lands on the same answer. Note the body's line numbers are now stale by +5 — 45c51bc (#264) inserted a GH_TOKEN comment plus a variable into every entrypoint's preflight env:.

Fleets with a directory-valued WATCHED_ASSETS and no WATCHED_EXEC, current lines:

fleet	`WATCHED_ASSETS`
`bump-agents-md-callers.yml`	`:106`
`bump-coderabbit-config-callers.yml`	`:118`
`bump-cursor-review-callers.yml`	`:150-152`
`bump-groom-callers.yml`	`:122`
`bump-pr-size-callers.yml`	`:130`

The five that do set it: area-label :76-80, linear-ticket :82-85, pr-risk :183-191, pr-derisk :217-226, public-repo-hygiene :96-98 (the verifier's correction to the finder holds — that last one was the one the finder missed). The three workflow-file-only fleets are correctly exempt, and 5 + 5 + 3 = 13 matches the full file count.

The mechanism checks out end to end: preflight.sh:906-909 documents the failure mode in its own words, WATCHED_EXEC is documented at :129-138 and implemented at :411-438 / :900-945 / :1196-1218, and the local directory probe at :1187-1195 uses -e rather than -d. When the deleting commit is the run's own commit, the tree-OID comparison and the pathspec diff both see no change and both presence probes pass, so the run emits proceed=true.

Fix step (b) has not landed — nothing in preflight.sh cross-references asset_dirs against watched_exec. Step (a) has not either; no commit since 2026-08-29 touches this.

One correction to the body. "Kept alive by tests/ and README.md" is accurate for four of the five, but not for scripts/check-pr-size, which holds only go.mod, main.go, main_test.go, size.go, size_test.go — no tests/ dir, no README. Same failure mode, different survivors: deleting main.go/size.go/go.mod leaves the *_test.go files holding the directory open, and those are exactly the files that fleet's own filter excludes. Your parenthetical about the *_test.go exclusion already had this right; only the summary phrasing needs adjusting.

A constraint for whoever writes step (b), recorded here so it is not rediscovered the hard way: preflight.sh:1181-1186 deliberately uses -e and not -d because a fleet may legitimately watch a single FILE. So the requirement has to be conditional on the entry actually resolving to a directory — mirroring the existing [[ -d "$f" ]] idiom at :433 — never unconditional.

Also worth folding into the same change: .github/bump-callers/README.md:190 says WATCHED_EXEC is needed "Only by pr-risk and pr-derisk today", which is already wrong for three fleets and would be wrong about ten after this lands. The same claim repeats around :401-427.

Tracked internally; the issue stays open until the fix lands. Reopen or comment if the census misses a fleet.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: confirmed, still exactly five fleets. Leaving this open, tracked internally.** Re-derived the census independently against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b` rather than re-reading the body, and it lands on the same answer. **Note the body's line numbers are now stale by +5** — `45c51bc` (#264) inserted a `GH_TOKEN` comment plus a variable into every entrypoint's preflight `env:`. Fleets with a directory-valued `WATCHED_ASSETS` and **no** `WATCHED_EXEC`, current lines: | fleet | `WATCHED_ASSETS` | |---|---| | `bump-agents-md-callers.yml` | `:106` | | `bump-coderabbit-config-callers.yml` | `:118` | | `bump-cursor-review-callers.yml` | `:150-152` | | `bump-groom-callers.yml` | `:122` | | `bump-pr-size-callers.yml` | `:130` | The five that do set it: area-label `:76-80`, linear-ticket `:82-85`, pr-risk `:183-191`, pr-derisk `:217-226`, public-repo-hygiene `:96-98` (the verifier's correction to the finder holds — that last one was the one the finder missed). The three workflow-file-only fleets are correctly exempt, and 5 + 5 + 3 = 13 matches the full file count. The mechanism checks out end to end: `preflight.sh:906-909` documents the failure mode in its own words, `WATCHED_EXEC` is documented at `:129-138` and implemented at `:411-438` / `:900-945` / `:1196-1218`, and the local directory probe at `:1187-1195` uses `-e` rather than `-d`. When the deleting commit is the run's own commit, the tree-OID comparison and the pathspec diff both see no change and both presence probes pass, so the run emits `proceed=true`. Fix step (b) has **not** landed — nothing in `preflight.sh` cross-references `asset_dirs` against `watched_exec`. Step (a) has not either; no commit since 2026-08-29 touches this. **One correction to the body.** "Kept alive by `tests/` and `README.md`" is accurate for four of the five, but **not** for `scripts/check-pr-size`, which holds only `go.mod`, `main.go`, `main_test.go`, `size.go`, `size_test.go` — no `tests/` dir, no README. Same failure mode, different survivors: deleting `main.go`/`size.go`/`go.mod` leaves the `*_test.go` files holding the directory open, and those are exactly the files that fleet's own filter excludes. Your parenthetical about the `*_test.go` exclusion already had this right; only the summary phrasing needs adjusting. **A constraint for whoever writes step (b)**, recorded here so it is not rediscovered the hard way: `preflight.sh:1181-1186` deliberately uses `-e` and not `-d` because a fleet may legitimately watch a single FILE. So the requirement has to be conditional on the entry actually resolving to a directory — mirroring the existing `[[ -d "$f" ]]` idiom at `:433` — never unconditional. Also worth folding into the same change: `.github/bump-callers/README.md:190` says `WATCHED_EXEC` is needed "Only by `pr-risk` and `pr-derisk` today", which is already wrong for three fleets and would be wrong about ten after this lands. The same claim repeats around `:401-427`. Tracked internally; the issue stays open until the fix lands. Reopen or comment if the census misses a fleet. *Created by Claude Code*

bsakaguchi referenced this issue

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

[PR #6] [MERGED] docs(cursor-review): add directory README with overview + install guide #18

bsakaguchi referenced this issue

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

[PR #306] [MERGED] test(workflow-pins): table-driven declared-vs-documented input drift harness for all 16 reusables #307