Skip to content

[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 #248

Open

Description

@cloud-code-bot

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

  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.

Activity

  1. added
    groomRefactor / tech-debt cleanup candidate filed by the groom sweep (finds-only)
    on Aug 29, 2026
  2. mattmillerai commented on Sep 18, 2026

    @mattmillerai
    Member

    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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    groomRefactor / tech-debt cleanup candidate filed by the groom sweep (finds-only)

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions