[PR #153] [MERGED] ci(bump-callers): multi-asset WATCHED_ASSETS; watch scripts/check-pr-size in the cursor-review fleet (BE-7045) #162

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/153
Author: @mattmillerai
Created: 8/11/2026
Status: ✅ Merged
Merged: 8/12/2026
Merged by: @mattmillerai

Base: matt/be-6476-preflight-swap ← Head: matt/be-7045-multi-asset-watched

📝 Commits (3)

d991d98 ci(bump-callers): multi-asset WATCHED_ASSETS; watch check-pr-size in the cursor-review fleet (BE-7045)
ecda0a6 ci(bump-callers): make the paths contract read WATCHED_ASSETS exactly as preflight does (BE-7045)
57ab1e4 ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084) (#154)

📊 Changes

7 files changed (+1984 additions, -95 deletions)

View changed files

📝 .github/bump-callers/README.md (+190 -27)
📝 .github/bump-callers/preflight.sh (+577 -52)
📝 .github/bump-callers/tests/test_paths_contract.sh (+456 -13)
📝 .github/bump-callers/tests/test_preflight.sh (+690 -1)
📝 .github/workflows/bump-cursor-review-callers.yml (+42 -1)
📝 .github/workflows/bump-pr-size-callers.yml (+25 -0)
📝 AGENTS.md (+4 -1)

📄 Description

STACKED — merging lands on matt/be-6476-preflight-swap (owned by @mattmillerai, PR #151), NOT main.
This builds directly on #151, which is what puts the Preflight step with WATCHED_ASSETS into bump-cursor-review-callers.yml and adds test_paths_contract.sh. Merge #151 first; GitHub will retarget this PR to main automatically. Do not treat this as safe-to-merge-to-main on its own.

ELI-5

cursor-review doesn't ship the "is this file generated?" classifier inside itself — it builds it at run time from whatever commit of this repo a consumer repo has pinned. So when we change that classifier, every pinned consumer keeps using the old one until something bumps their pin. Nothing did: the bump job for cursor-review wasn't watching the classifier's folder at all.

Just adding the folder to the watch list would have quietly broken a different safety check, so this PR does both halves at once: it widens the watch list and teaches the preflight guard to compare more than one folder.

Why

cursor-review.yml:458-489 checks out and builds scripts/check-pr-size from the caller's pinned workflows_ref — that classifier decides which of a PR's files count as generated and therefore skip review. But bump-cursor-review-callers.yml's push: paths: filter only watched cursor-review.yml + .github/cursor-review/**. A classifier-only commit fired the pr-size fleet and left every pinned cursor-review caller on the stale classifier.

Widening the filter alone would be unsound. preflight.sh's re-point (pin callers to the verified main tip instead of this run's github.sha) is only valid when the compared surface covers every entry in the fleet's trigger — the COUPLED TO THE PATH FILTER note. WATCHED_ASSETS held a single path, so the re-point would have pinned callers to a tip whose check-pr-size content was never compared. Hence both halves in the same commit.

What changed

preflight.sh — WATCHED_ASSETS is now a newline-separated list. Each entry is validated (glob / trailing-slash rejection, same error text), OID-compared against the tip, and decommission-checked independently, with per-entry semantics identical to the old single-asset behavior: any one entry absent at HEAD → decommission; absent at the tip → tip_gone decommission; differing from HEAD → stale run/re-run. Only if every entry matches does the re-point proceed. A single-line value parses to a one-element list, so every existing single-asset fleet is unchanged.
bump-cursor-review-callers.yml — adds scripts/check-pr-size/** to paths: (with a comment mirroring the existing .github/cursor-review/** one) and passes both assets as a block scalar.
test_paths_contract.sh — parse_preflight_env learns the block-scalar shape; actual splits a multi-line value into one entry per line.
test_preflight.sh — a second fixture asset dir plus multi-asset cases.
.github/bump-callers/README.md — the WATCHED_ASSETS row + multi-path section document the list form and name cursor-review as the two-asset fleet.

Verification

Check	Result
`test_preflight.sh`	149 passed, 0 failed (107 pre-existing unchanged + 42 new)
`test_paths_contract.sh`	15 passed, 0 failed
`test_bump_callers.sh`	185 passed, 0 failed
`shellcheck -x` (all 5 scripts)	clean
Python suites (cursor-review / agents-md / groom / refresh-reviewers)	618 passed

Red/green ordering property observed (acceptance criterion): with step 2 applied but steps 1/3 not, test_paths_contract.sh failed exactly as required —

FAIL: bump-cursor-review-callers.yml: `paths:` filter and preflight inputs disagree
      paths:   .github/cursor-review .github/workflows/cursor-review.yml scripts/check-pr-size
      watched: .github/workflows/cursor-review.yml |

(note the stray | — the old single-line parser read the block indicator as the value.) After steps 1+3 it passes.

The new cases prove the second entry is really checked — the failure mode a loop over entry 0 alone would ship. Each "only entry N changed" case is paired with an assertion that the other-entry-only config would wrongly re-point, so the verdict is demonstrably produced by the entry under test: either-entry-changed, either-entry deleted at the tip, either absent at HEAD, both-unchanged re-point, blank/padded lines, and a glob or trailing slash on the second line.

bash 3.2 compatibility verified. "${arr[@]}" on an empty array under set -u is an unbound-variable error on macOS's bash 3.2 (the trap preflight.sh's own comments warn about), so both new loops are guarded with (( ${#asset_dirs[@]} > 0 )). Confirmed empirically that the unguarded form errors and the guarded form does not, and that the parse loop yields the right two entries under 3.2.

Judgment calls

Stacked rather than blocked. The ticket's precondition said to stop and hand back with agent-blocked if #151 hadn't merged. #151 is open — but its branch carries the foundation code (test_paths_contract.sh and the WATCHED_ASSETS preflight step are both present on it), which is the standing always-stack case. Building on the real foundation is strictly better than waiting, and GitHub retargets this PR when #151 merges. Flagging it explicitly since it departs from the ticket's literal instruction.
Per-entry whitespace trimming (small scope addition). A YAML flow scalar can't carry surrounding whitespace, but a block scalar can — and .github/groom resolves to nothing exactly like the .github/groom/** glob validate_path already rejects, i.e. it would silently verify nothing behind a green run. That is a hazard introduced by the new input shape, so each entry is trimmed and the reasoning is in a comment. No existing fleet's value is affected (all 107 pre-existing cases pass unchanged).
scripts/check-pr-size is now watched by two fleets, verified conflict-free. Rather than take this on faith I checked it: bump-callers.sh:65 derives BRANCH="ci/bump-${TAG}", so the fleets push ci/bump-cursor-review vs ci/bump-pr-size, with distinct concurrency groups (bump-cursor-review-callers / bump-pr-size-callers) and distinct rosters (CURSOR_REVIEW_CALLERS / PR_SIZE_CALLERS). bump-pr-size-callers.yml is untouched, per the ticket.
bump-pr-risk-callers.yml keeps its exemption — its ! exclusions are out of scope and it is not migrated here.

Out of scope, but noticed

AGENTS.md is 208 lines, over the checker's 200-line hard ceiling (207 on main, 208 on #151's branch — pre-existing on both, untouched by this PR). It doesn't gate CI because this repo has no self-enrolled ci-agents-md-integrity.yml caller, only test-agents-md-integrity.yml for the checker's unit tests. Worth a separate trim ticket.

Acceptance criteria

All met — both suites pass locally (they are hermetic; CI runs them via test-bump-callers.yml, whose paths: filter already covers bump-cursor-review-callers.yml, so the contract test will run on this PR), the red/green ordering property was observed, pr-risk keeps its exemption, and bump-pr-size-callers.yml was not touched.

Negative-claim falsification: n/a — this change denies no capability. It widens what WATCHED_ASSETS supports. The only rejection paths (glob / trailing-slash) are per-entry extensions of a rejection that already existed, and the premise that those shapes resolve to nothing is empirically covered by both the pre-existing glob_input case and the new multi_glob case rather than asserted.

_{🔄 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/153 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/11/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-6476-preflight-swap` ← **Head:** `matt/be-7045-multi-asset-watched` --- ### 📝 Commits (3) - [`d991d98`](https://github.com/Comfy-Org/github-workflows/commit/d991d98786fb072a1099d3238bc21962dc57a1af) ci(bump-callers): multi-asset WATCHED_ASSETS; watch check-pr-size in the cursor-review fleet (BE-7045) - [`ecda0a6`](https://github.com/Comfy-Org/github-workflows/commit/ecda0a64b05a69512c87bd94b78465753e9ede44) ci(bump-callers): make the paths contract read WATCHED_ASSETS exactly as preflight does (BE-7045) - [`57ab1e4`](https://github.com/Comfy-Org/github-workflows/commit/57ab1e4d484ff79e7c7e6e1cb00f7681aa2d7c76) ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084) (#154) ### 📊 Changes **7 files changed** (+1984 additions, -95 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+190 -27) 📝 `.github/bump-callers/preflight.sh` (+577 -52) 📝 `.github/bump-callers/tests/test_paths_contract.sh` (+456 -13) 📝 `.github/bump-callers/tests/test_preflight.sh` (+690 -1) 📝 `.github/workflows/bump-cursor-review-callers.yml` (+42 -1) 📝 `.github/workflows/bump-pr-size-callers.yml` (+25 -0) 📝 `AGENTS.md` (+4 -1) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-6476-preflight-swap` (owned by @mattmillerai, PR #151), NOT `main`.** > This builds directly on #151, which is what puts the `Preflight` step with `WATCHED_ASSETS` into `bump-cursor-review-callers.yml` and adds `test_paths_contract.sh`. Merge #151 first; GitHub will retarget this PR to `main` automatically. Do **not** treat this as safe-to-merge-to-main on its own. ## ELI-5 `cursor-review` doesn't ship the "is this file generated?" classifier inside itself — it **builds** it at run time from whatever commit of this repo a consumer repo has pinned. So when we change that classifier, every pinned consumer keeps using the *old* one until something bumps their pin. Nothing did: the bump job for cursor-review wasn't watching the classifier's folder at all. Just adding the folder to the watch list would have quietly broken a different safety check, so this PR does both halves at once: it widens the watch list **and** teaches the preflight guard to compare more than one folder. ## Why `cursor-review.yml:458-489` checks out and builds `scripts/check-pr-size` from the caller's pinned `workflows_ref` — that classifier decides which of a PR's files count as generated and therefore **skip review**. But `bump-cursor-review-callers.yml`'s `push:` `paths:` filter only watched `cursor-review.yml` + `.github/cursor-review/**`. A classifier-only commit fired the *pr-size* fleet and left every pinned cursor-review caller on the stale classifier. Widening the filter alone would be **unsound**. `preflight.sh`'s re-point (pin callers to the verified main tip instead of this run's `github.sha`) is only valid when the compared surface covers every entry in the fleet's trigger — the COUPLED TO THE PATH FILTER note. `WATCHED_ASSETS` held a single path, so the re-point would have pinned callers to a tip whose `check-pr-size` content was never compared. Hence both halves in the same commit. ## What changed 1. **`preflight.sh`** — `WATCHED_ASSETS` is now a **newline-separated list**. Each entry is validated (glob / trailing-slash rejection, same error text), OID-compared against the tip, and decommission-checked **independently**, with per-entry semantics identical to the old single-asset behavior: any one entry absent at HEAD → decommission; absent at the tip → `tip_gone` decommission; differing from HEAD → stale run/re-run. Only if *every* entry matches does the re-point proceed. A single-line value parses to a one-element list, so every existing single-asset fleet is unchanged. 2. **`bump-cursor-review-callers.yml`** — adds `scripts/check-pr-size/**` to `paths:` (with a comment mirroring the existing `.github/cursor-review/**` one) and passes both assets as a block scalar. 3. **`test_paths_contract.sh`** — `parse_preflight_env` learns the block-scalar shape; `actual` splits a multi-line value into one entry per line. 4. **`test_preflight.sh`** — a second fixture asset dir plus multi-asset cases. 5. **`.github/bump-callers/README.md`** — the `WATCHED_ASSETS` row + multi-path section document the list form and name cursor-review as the two-asset fleet. ## Verification | Check | Result | |---|---| | `test_preflight.sh` | **149 passed, 0 failed** (107 pre-existing unchanged + 42 new) | | `test_paths_contract.sh` | **15 passed, 0 failed** | | `test_bump_callers.sh` | **185 passed, 0 failed** | | `shellcheck -x` (all 5 scripts) | clean | | Python suites (cursor-review / agents-md / groom / refresh-reviewers) | **618 passed** | **Red/green ordering property observed** (acceptance criterion): with step 2 applied but steps 1/3 not, `test_paths_contract.sh` failed exactly as required — ``` FAIL: bump-cursor-review-callers.yml: `paths:` filter and preflight inputs disagree paths: .github/cursor-review .github/workflows/cursor-review.yml scripts/check-pr-size watched: .github/workflows/cursor-review.yml | ``` (note the stray `|` — the old single-line parser read the block indicator as the value.) After steps 1+3 it passes. **The new cases prove the second entry is really checked** — the failure mode a loop over entry 0 alone would ship. Each "only entry N changed" case is paired with an assertion that the *other-entry-only* config would wrongly re-point, so the verdict is demonstrably produced by the entry under test: either-entry-changed, either-entry deleted at the tip, either absent at HEAD, both-unchanged re-point, blank/padded lines, and a glob **or** trailing slash on the **second** line. **bash 3.2 compatibility verified.** `"${arr[@]}"` on an empty array under `set -u` is an unbound-variable error on macOS's bash 3.2 (the trap `preflight.sh`'s own comments warn about), so both new loops are guarded with `(( ${#asset_dirs[@]} > 0 ))`. Confirmed empirically that the unguarded form errors and the guarded form does not, and that the parse loop yields the right two entries under 3.2. ## Judgment calls - **Stacked rather than blocked.** The ticket's precondition said to stop and hand back with `agent-blocked` if #151 hadn't merged. #151 is open — but its branch **carries the foundation code** (`test_paths_contract.sh` and the `WATCHED_ASSETS` preflight step are both present on it), which is the standing always-stack case. Building on the real foundation is strictly better than waiting, and GitHub retargets this PR when #151 merges. Flagging it explicitly since it departs from the ticket's literal instruction. - **Per-entry whitespace trimming (small scope addition).** A YAML *flow* scalar can't carry surrounding whitespace, but a **block scalar can** — and ` .github/groom` resolves to nothing exactly like the `.github/groom/**` glob `validate_path` already rejects, i.e. it would silently verify nothing behind a green run. That is a hazard introduced by the new input shape, so each entry is trimmed and the reasoning is in a comment. No existing fleet's value is affected (all 107 pre-existing cases pass unchanged). - **`scripts/check-pr-size` is now watched by two fleets, verified conflict-free.** Rather than take this on faith I checked it: `bump-callers.sh:65` derives `BRANCH="ci/bump-${TAG}"`, so the fleets push `ci/bump-cursor-review` vs `ci/bump-pr-size`, with distinct concurrency groups (`bump-cursor-review-callers` / `bump-pr-size-callers`) and distinct rosters (`CURSOR_REVIEW_CALLERS` / `PR_SIZE_CALLERS`). `bump-pr-size-callers.yml` is untouched, per the ticket. - **`bump-pr-risk-callers.yml` keeps its exemption** — its `!` exclusions are out of scope and it is not migrated here. ## Out of scope, but noticed `AGENTS.md` is **208 lines**, over the checker's 200-line hard ceiling (`207` on `main`, `208` on #151's branch — pre-existing on both, untouched by this PR). It doesn't gate CI because this repo has no self-enrolled `ci-agents-md-integrity.yml` caller, only `test-agents-md-integrity.yml` for the checker's unit tests. Worth a separate trim ticket. ## Acceptance criteria All met — both suites pass locally (they are hermetic; CI runs them via `test-bump-callers.yml`, whose `paths:` filter already covers `bump-cursor-review-callers.yml`, so the contract test will run on this PR), the red/green ordering property was observed, pr-risk keeps its exemption, and `bump-pr-size-callers.yml` was not touched. *Negative-claim falsification: n/a — this change denies no capability. It widens what `WATCHED_ASSETS` supports. The only rejection paths (glob / trailing-slash) are per-entry extensions of a rejection that already existed, and the premise that those shapes resolve to nothing is empirically covered by both the pre-existing `glob_input` case and the new `multi_glob` case rather than asserted.* --- <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