Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #327] [MERGED] ci(bump-callers): exclude catalog-drift.py and schema_drift.py from the cursor-review and coderabbit-config fleet triggers #327

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-15267-bump-exclude-tests-readme ← Head: matt/be-15721-exclude-drift-scripts-from-fleet-triggers

📝 Commits (2)

7d835f9 ci(bump-callers): exclude catalog-drift.py and schema_drift.py from the cursor-review and coderabbit-config fleet triggers
cc07191 test(bump-callers): resolve every paths: exclusion against the tree

📊 Changes

4 files changed (+91 additions, -10 deletions)

View changed files

📝 .github/bump-callers/README.md (+18 -3)
📝 .github/bump-callers/tests/test_paths_contract.sh (+48 -0)
📝 .github/workflows/bump-coderabbit-config-callers.yml (+6 -0)
📝 .github/workflows/bump-cursor-review-callers.yml (+19 -7)

📄 Description

STACKED — merging lands on matt/be-15267-bump-exclude-tests-readme (owned by @mattmillerai, PR #324), NOT main. Merge #324 first; this PR's diff is only the three files below.

ELI-5

Two Python scripts live inside directories that the caller-bump fleets watch, but no consumer ever runs them — they belong to this repo's own weekly maintenance workflows. Today, editing either one mints the org-wide bot token and opens a "bump your pin" PR in every consumer repo, for a change none of those repos can observe. This teaches the two fleets to ignore them. A third script in the same situation, wire-bot-identity.py, deliberately stays watched — the bumper itself executes it and writes its output into caller files, so changing it really does change what consumers get.

What changed

.github/workflows/bump-cursor-review-callers.yml — on.push.paths gains !.github/cursor-review/catalog-drift.py, mirrored into WATCHED_PATHSPECS as :(exclude).github/cursor-review/catalog-drift.py. A comment records why wire-bot-identity.py is NOT excluded alongside it.
.github/workflows/bump-coderabbit-config-callers.yml — same shape for !.github/coderabbit-config/schema_drift.py / :(exclude).github/coderabbit-config/schema_drift.py.
.github/bump-callers/README.md — the two fleets' "Triggers on" cells, plus the "Do not list a file no pinned caller loads" bullet, now record both exclusions and the wire-bot-identity.py carve-out.
.github/bump-callers/tests/test_paths_contract.sh — added after review (cc07191): resolves every paths: exclusion against the tracked tree, so a :(exclude) that stops matching fails the PR instead of silently widening the watched surface back out. Covers all 24 exclusions across all 10 entrypoints, not just the two added here.
.github/bump-callers/README.md — a companion bullet beside the existing "a path that stops resolving is a silent freeze" one, recording why that check lives in the contract test and not in preflight.sh.

WATCHED_ASSETS and WATCHED_EXEC are untouched in both fleets, and the groom fleet is untouched entirely.

Why these two are safe to exclude (empirical sweep, not assertion)

The change's outcome DENIES a bump, so the premise "no pinned caller loads these" was checked against every path in the tree rather than assumed:

Neither reusable a consumer actually calls references either script: grep catalog-drift .github/workflows/cursor-review.yml and grep schema_drift .github/workflows/coderabbit-config-validate.yml both return nothing.
catalog-drift.py's only executor is .github/workflows/cursor-review-catalog-drift.yml, whose triggers are schedule + workflow_dispatch only — no workflow_call, so no caller can reach it at all.
schema_drift.py's only executor is .github/workflows/refresh-coderabbit-schema.yml, likewise schedule + workflow_dispatch, and it checks out ref: main rather than any pinned ref.
git grep -n -E 'catalog-drift|schema_drift' -- .github/cursor-review .github/coderabbit-config ':!*/tests/*' ':!*README.md' hits only each file's own body (usage docstrings and a report footer string).
By contrast wire-bot-identity.py IS reached from a pinned-fleet code path — bump-cursor-review-callers.yml sets WIRE_BOT_SCRIPT: ${{ github.workspace }}/.github/cursor-review/wire-bot-identity.py and bump-callers.sh pipes each caller's YAML through it, staging the file even when the SHA is already current. Excluding it would strand wiring changes; it stays watched, with a comment saying so.

Verified

bash .github/bump-callers/tests/test_paths_contract.sh — 161 passed, 0 failed (151 before the review fix). Both fleets report "WATCHED_PATHSPECS mirrors the paths: filter, exclusions included", each new literal-file negation reports the expected "cannot diverge this way" skip, and every fleet now also reports "all N paths: exclusions still select a tracked path".
The new exclusion-resolution assertion was mutation-tested, not assumed: renaming catalog-drift.py in both lists but not in the tree fails with the exclusion .github/cursor-review/catalog_drift.py selects no tracked path at this commit …; reverted, green again. A sweep of the current tree first confirmed all 24 exclusions resolve today, so it guards the next rename rather than papering over a live break.
bash .github/bump-callers/tests/test_preflight.sh — 445 passed, 0 failed.
bash .github/bump-callers/tests/test_bump_callers.sh — 437 passed, 0 failed.
shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh — clean.
python3 .github/workflow-pins/check_workflow_pins.py — 11 workflows, 156 uses: refs pinned, OK.
bash .github/lint/check-org-repo-literals.sh — OK.

Judgment calls

One comment above WATCHED_ASSETS in bump-cursor-review-callers.yml was reworded even though the plan said to leave WATCHED_ASSETS alone. The input's value is untouched; the comment above it enumerates which excluded paths an OID comparison cannot express, and leaving catalog-drift.py out of that enumeration would have shipped a knowingly stale list. Nothing else near either input moved.
The WATCHED_EXEC "DELIBERATELY ABSENT" note was left as-is. It groups catalog-drift.py and wire-bot-identity.py together, and that grouping stays correct on its own terms — neither is loaded by a caller at run time, which is that input's test. The new paths: comment is where the two now diverge, and it cross-references that note.

Review follow-through

One 🟢 Low finding from the cursor-review panel (raised by 2 of 6 reviewers): the new :(exclude) literals were the one class of hand-written path here that nothing probed for deletion — git ignores a non-matching :(exclude) silently, preflight.sh's resolution loop walks pathspec_positives only, and the contract test's mirror assertion compares the two lists against each other, so a rename applied to neither leaves both stale and agreeing. Verified all three legs and fixed in cc07191.

Deliberately not done the way the finding suggested ("the way WATCHED_EXEC literals already are", i.e. in preflight's decommission probe). A missing WATCHED_EXEC file breaks a pinned caller at run time; a dead exclusion is the opposite sign — it only ever over-watches, comparing more than the filter negates and never less, so it can never produce an unsafe bump. Erroring on it at fleet run time would trade a churn regression for a full fleet outage. The exclusion list is a static, repo-local artifact, so the renaming commit is what should fail, and that commit is a PR in this repo.

Residual

One flaky first run of test_preflight.sh, not reproduced and not diagnosed. The very first invocation on this branch reported 444 passed, 1 failed; 18 consecutive runs afterwards reported 445 passed, 0 failed and no run since printed a failing assertion, so the identity of the failing case was never captured. test_preflight.sh does not read either file this PR edits, so the diff is not a plausible cause — but "a suite that failed once on a shared host and never again" is worth a look on its own, independent of this change.
No live fleet run exercised. The bump fleets only fire on push to main, so nothing here proves the end-to-end behavior — that a real commit touching only catalog-drift.py produces no bump PR. The verification is the contract suite plus the reachability sweep above. The first commit to land on main touching only one of these two files is the real confirmation.
The spike this ticket was written from was not readable from this environment. Its findings — the reachability and per-file call analysis the plan cites as already verified — were re-derived independently here (the sweep above) rather than read; if that spike recorded a case the greps above cannot see, it is unaccounted for.
The prerequisite PR is unmerged and this is stacked on it. If #324 is revised before merge — particularly its WATCHED_PATHSPECS blocks — this branch needs a rebase, and test_paths_contract.sh is what will catch a mismatch.

Provenance

Authored by: agent-work loop
Verified: test_paths_contract.sh 161 passed / 0 failed (mirror assertion, the "cannot diverge" skips, and the new per-fleet exclusion-resolution assertion over all 24 exclusions; the new assertion mutation-tested to confirm it fires); test_preflight.sh 445 passed / 0 failed; test_bump_callers.sh 437 passed / 0 failed; shellcheck -x over bump-callers clean; check_workflow_pins.py OK; check_agents_md.py passed; check-org-repo-literals.sh OK
Deviations: one comment above WATCHED_ASSETS reworded to keep its enumeration of excluded paths accurate — see "Judgment calls"; the review fix was placed in the contract test rather than in preflight.sh as the finding proposed — see "Review follow-through"; no live fleet run exercised, see "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/327 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-15267-bump-exclude-tests-readme` ← **Head:** `matt/be-15721-exclude-drift-scripts-from-fleet-triggers` --- ### 📝 Commits (2) - [`7d835f9`](https://github.com/Comfy-Org/github-workflows/commit/7d835f994233d6a00c238244a966879d184d2389) ci(bump-callers): exclude catalog-drift.py and schema_drift.py from the cursor-review and coderabbit-config fleet triggers - [`cc07191`](https://github.com/Comfy-Org/github-workflows/commit/cc071912681dc418da2b331bf2c2037d524f89ec) test(bump-callers): resolve every `paths:` exclusion against the tree ### 📊 Changes **4 files changed** (+91 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+18 -3) 📝 `.github/bump-callers/tests/test_paths_contract.sh` (+48 -0) 📝 `.github/workflows/bump-coderabbit-config-callers.yml` (+6 -0) 📝 `.github/workflows/bump-cursor-review-callers.yml` (+19 -7) </details> ### 📄 Description **STACKED — merging lands on `matt/be-15267-bump-exclude-tests-readme` (owned by @mattmillerai, PR #324), NOT `main`.** Merge #324 first; this PR's diff is only the three files below. ## ELI-5 Two Python scripts live inside directories that the caller-bump fleets watch, but no consumer ever runs them — they belong to this repo's own weekly maintenance workflows. Today, editing either one mints the org-wide bot token and opens a "bump your pin" PR in every consumer repo, for a change none of those repos can observe. This teaches the two fleets to ignore them. A third script in the same situation, `wire-bot-identity.py`, deliberately stays watched — the bumper itself executes it and writes its output into caller files, so changing it really does change what consumers get. ## What changed - `.github/workflows/bump-cursor-review-callers.yml` — `on.push.paths` gains `!.github/cursor-review/catalog-drift.py`, mirrored into `WATCHED_PATHSPECS` as `:(exclude).github/cursor-review/catalog-drift.py`. A comment records why `wire-bot-identity.py` is NOT excluded alongside it. - `.github/workflows/bump-coderabbit-config-callers.yml` — same shape for `!.github/coderabbit-config/schema_drift.py` / `:(exclude).github/coderabbit-config/schema_drift.py`. - `.github/bump-callers/README.md` — the two fleets' "Triggers on" cells, plus the "Do not list a file no pinned caller loads" bullet, now record both exclusions and the `wire-bot-identity.py` carve-out. - `.github/bump-callers/tests/test_paths_contract.sh` — **added after review** (cc07191): resolves every `paths:` exclusion against the tracked tree, so a `:(exclude)` that stops matching fails the PR instead of silently widening the watched surface back out. Covers all 24 exclusions across all 10 entrypoints, not just the two added here. - `.github/bump-callers/README.md` — a companion bullet beside the existing "a path that stops resolving is a silent freeze" one, recording why that check lives in the contract test and not in `preflight.sh`. `WATCHED_ASSETS` and `WATCHED_EXEC` are untouched in both fleets, and the groom fleet is untouched entirely. ## Why these two are safe to exclude (empirical sweep, not assertion) The change's outcome DENIES a bump, so the premise "no pinned caller loads these" was checked against every path in the tree rather than assumed: - Neither reusable a consumer actually calls references either script: `grep catalog-drift .github/workflows/cursor-review.yml` and `grep schema_drift .github/workflows/coderabbit-config-validate.yml` both return nothing. - `catalog-drift.py`'s only executor is `.github/workflows/cursor-review-catalog-drift.yml`, whose triggers are `schedule` + `workflow_dispatch` only — no `workflow_call`, so no caller can reach it at all. - `schema_drift.py`'s only executor is `.github/workflows/refresh-coderabbit-schema.yml`, likewise `schedule` + `workflow_dispatch`, and it checks out `ref: main` rather than any pinned ref. - `git grep -n -E 'catalog-drift|schema_drift' -- .github/cursor-review .github/coderabbit-config ':!*/tests/*' ':!*README.md'` hits only each file's own body (usage docstrings and a report footer string). - By contrast `wire-bot-identity.py` IS reached from a pinned-fleet code path — `bump-cursor-review-callers.yml` sets `WIRE_BOT_SCRIPT: ${{ github.workspace }}/.github/cursor-review/wire-bot-identity.py` and `bump-callers.sh` pipes each caller's YAML through it, staging the file even when the SHA is already current. Excluding it would strand wiring changes; it stays watched, with a comment saying so. ## Verified - `bash .github/bump-callers/tests/test_paths_contract.sh` — **161 passed, 0 failed** (151 before the review fix). Both fleets report "WATCHED_PATHSPECS mirrors the `paths:` filter, exclusions included", each new literal-file negation reports the expected "cannot diverge this way" skip, and every fleet now also reports "all N `paths:` exclusions still select a tracked path". - The new exclusion-resolution assertion was **mutation-tested, not assumed**: renaming `catalog-drift.py` in both lists but not in the tree fails with `the exclusion .github/cursor-review/catalog_drift.py selects no tracked path at this commit …`; reverted, green again. A sweep of the current tree first confirmed all 24 exclusions resolve today, so it guards the next rename rather than papering over a live break. - `bash .github/bump-callers/tests/test_preflight.sh` — 445 passed, 0 failed. - `bash .github/bump-callers/tests/test_bump_callers.sh` — 437 passed, 0 failed. - `shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh` — clean. - `python3 .github/workflow-pins/check_workflow_pins.py` — 11 workflows, 156 `uses:` refs pinned, OK. - `bash .github/lint/check-org-repo-literals.sh` — OK. ## Judgment calls - **One comment above `WATCHED_ASSETS` in `bump-cursor-review-callers.yml` was reworded** even though the plan said to leave `WATCHED_ASSETS` alone. The input's value is untouched; the comment above it enumerates which excluded paths an OID comparison cannot express, and leaving `catalog-drift.py` out of that enumeration would have shipped a knowingly stale list. Nothing else near either input moved. - **The `WATCHED_EXEC` "DELIBERATELY ABSENT" note was left as-is.** It groups `catalog-drift.py` and `wire-bot-identity.py` together, and that grouping stays correct on its own terms — neither is loaded by a caller at run time, which is that input's test. The new `paths:` comment is where the two now diverge, and it cross-references that note. ## Review follow-through One 🟢 Low finding from the cursor-review panel (raised by 2 of 6 reviewers): the new `:(exclude)` literals were the one class of hand-written path here that nothing probed for deletion — git ignores a non-matching `:(exclude)` silently, `preflight.sh`'s resolution loop walks `pathspec_positives` only, and the contract test's mirror assertion compares the two lists against each *other*, so a rename applied to neither leaves both stale and agreeing. Verified all three legs and fixed in cc07191. Deliberately **not** done the way the finding suggested ("the way `WATCHED_EXEC` literals already are", i.e. in preflight's decommission probe). A missing `WATCHED_EXEC` file breaks a pinned caller at run time; a dead exclusion is the opposite sign — it only ever *over*-watches, comparing more than the filter negates and never less, so it can never produce an unsafe bump. Erroring on it at fleet run time would trade a churn regression for a full fleet outage. The exclusion list is a static, repo-local artifact, so the renaming commit is what should fail, and that commit is a PR in this repo. ## Residual - **One flaky first run of `test_preflight.sh`, not reproduced and not diagnosed.** The very first invocation on this branch reported `444 passed, 1 failed`; 18 consecutive runs afterwards reported `445 passed, 0 failed` and no run since printed a failing assertion, so the identity of the failing case was never captured. `test_preflight.sh` does not read either file this PR edits, so the diff is not a plausible cause — but "a suite that failed once on a shared host and never again" is worth a look on its own, independent of this change. - **No live fleet run exercised.** The bump fleets only fire on `push` to `main`, so nothing here proves the end-to-end behavior — that a real commit touching only `catalog-drift.py` produces no bump PR. The verification is the contract suite plus the reachability sweep above. The first commit to land on `main` touching only one of these two files is the real confirmation. - **The spike this ticket was written from was not readable from this environment.** Its findings — the reachability and per-file call analysis the plan cites as already verified — were re-derived independently here (the sweep above) rather than read; if that spike recorded a case the greps above cannot see, it is unaccounted for. - **The prerequisite PR is unmerged and this is stacked on it.** If #324 is revised before merge — particularly its `WATCHED_PATHSPECS` blocks — this branch needs a rebase, and `test_paths_contract.sh` is what will catch a mismatch. ## Provenance - **Authored by:** agent-work loop - **Verified:** test_paths_contract.sh 161 passed / 0 failed (mirror assertion, the "cannot diverge" skips, and the new per-fleet exclusion-resolution assertion over all 24 exclusions; the new assertion mutation-tested to confirm it fires); test_preflight.sh 445 passed / 0 failed; test_bump_callers.sh 437 passed / 0 failed; shellcheck -x over bump-callers clean; check_workflow_pins.py OK; check_agents_md.py passed; check-org-repo-literals.sh OK - **Deviations:** one comment above `WATCHED_ASSETS` reworded to keep its enumeration of excluded paths accurate — see "Judgment calls"; the review fix was placed in the contract test rather than in preflight.sh as the finding proposed — see "Review follow-through"; no live fleet run exercised, see "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:33 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#327

No description provided.

Rows
Columns