[PR #87] [MERGED] feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819) #97

New issue

Closed

opened 2026-10-26 22:36:12 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:12 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/87
Author: @mattmillerai
Created: 7/28/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4819-cursor-review-catalog-drift

📝 Commits (4)

40d5f0a feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819)
c36a347 fix(cursor-review): least-privilege the drift check + paginate the sticky lookup (BE-4819)
1c47f59 fix(cursor-review): close the drift check's silent-miss paths from the review panel (BE-4819)
4d7efb9 fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) (#88)

📊 Changes

7 files changed (+1838 additions, -1 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
➕ .github/cursor-review/catalog-drift.py (+849 -0)
➕ .github/cursor-review/tests/test_catalog_drift.py (+736 -0)
➕ .github/workflows/cursor-review-catalog-drift.yml (+238 -0)
📝 .github/workflows/test-cursor-review-scripts.yml (+9 -0)
📝 AGENTS.md (+4 -1)
📝 README.md (+1 -0)

📄 Description

ELI-5

The review panel pins four model ids (plus a judge). Cursor keeps swapping models under us: a pinned id gets delisted (every consumer PR then fails the review preflight), or a newer/better model ships and we never notice. The only thing watching for the second case was a hand-written last checked <date> comment — which went stale in 13 days, and both swaps the BE-4817 audit found (Opus 5 on 07-24, Kimi K3 on ~07-26) landed inside that window and were caught by a human, not by CI.

This adds a robot that checks every Monday. It reads the pins out of cursor-review.yml itself, asks Cursor what models actually exist, and writes what it finds into one sticky GitHub issue — which it updates in place each week and closes automatically once the drift is gone.

What this changes

New .github/workflows/cursor-review-catalog-drift.yml — schedule (17 6 * * 1) + workflow_dispatch, contents: read + issues: write:

Checkout, install the Cursor CLI exactly the way preflight does, run cursor-agent models (hard fail on failure — preflight's "better red than silent" stance).
Extract the pins from .github/workflows/cursor-review.yml — the Define panel models heredoc plus the judge_model input default. Nothing is duplicated, so the checker stays zero-maintenance when the pins are bumped (e.g. by #86).
Compare, using the same whole-token regex preflight uses ((?<![\w.-])<id>(?![\w.-])):
- delisted pin — the urgent case, reported first in the issue and it fails the run (see judgment calls);
- unpinned same-lab ids — grouped by lab, an explicit review-me list rather than a recommendation, with any inline catalog marker (e.g. Fable's (NO ZDR)) reproduced verbatim and a caution that an unmarked id is not thereby confirmed ZDR-eligible;
- stale audit date — last checked YYYY-MM-DD older than 30 days (or missing).
Open / update / close one sticky issue titled [cursor-review catalog drift] …, found by exact title prefix. Raw catalog folded into a <details> block. No findings → the sticky issue is closed and the run is a green no-op.

Comparison + rendering live in .github/cursor-review/catalog-drift.py with 30 unit tests; cursor-review.yml and the new workflow join test-cursor-review-scripts.yml's path filter. Docs: a row in the root README table, a paragraph in the cursor-review README's panel section (the last checked comment stays the human-audit record), and an AGENTS.md layout line.

Verification

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' → 78 passed (75 pre-existing + the new suite). actionlint (which runs shellcheck over the run: blocks) clean on every workflow; check_agents_md.py passes.
End-to-end dry runs of the script against the real cursor-review.yml with synthesized catalogs: a clean catalog reports no drift; a catalog with kimi-k3-code replacing kimi-k2.7-code plus a new claude-opus-5-thinking-max reproduces exactly the BE-4817 scenario — 1 delisted pin (with kimi-k3-code named as the same-lab alternative), 3 unpinned same-lab ids, stale audit date.
One test runs the extractors against the checked-in cursor-review.yml, so a refactor of the pins block fails a cheap unit run instead of silently making the weekly check report "no drift" forever. Verified against #86's diff: it changes the pin values and the audit date but keeps every anchor, so extraction still works after that lands.
Falsification of the two hard-fail paths (they deny a capability, so they were checked empirically, not just unit-tested):
- cursor-agent models without credentials → exit 1, empty stdout (ran it locally). So the workflow's if ! cursor-agent models guard genuinely catches the auth case rather than proceeding with garbage. A second guard rejects a non-empty but unparseable catalog rather than reporting "all four pins delisted".
- "CURSOR_API_KEY is not available here" → falsified: this repo's own ci-cursor-review.yml runs (which need the key for preflight) are succeeding as recently as 2026-07-28, so the org secret does reach this repo and the missing-secret branch is a real-failure path, not the expected one. (Repo-level secret list shows only CLOUD_CODE_BOT_PRIVATE_KEY; the org secret list needs admin:org, which I don't have — the succeeding runs are the evidence.)

Not verified, and it cannot be until this merges: the ticket's workflow_dispatch checks (dispatch once, dispatch again to confirm the sticky issue updates in place; a fake-id branch run for the delisted path). GitHub only exposes workflow_dispatch for workflows present on the default branch, so neither can run from this branch. The dry runs above cover the same logic offline; the first scheduled/dispatched run is the live confirmation.

Judgment calls

Logic lives in a tested Python script, not inline sed/yq in the workflow. The ticket sketched inline extraction; this repo's convention (AGENTS.md) is that cursor-review logic lives in .github/cursor-review/ with unit tests, and inline heredocs in a scheduled-only workflow are otherwise untestable before merge. Behavior is the ticket's; only the location changed.
A delisted pin fails the run (non-urgent findings leave it green). The ticket says "no findings → green no-op" but calls delisted the urgent case to "report prominently" — a red weekly run plus the failure notification is the loudest channel available, and it self-heals the moment the pin is fixed.
Lab prefixes are derived from the pins rather than hardcoded as gpt-/claude-/gemini-/kimi-. Today that resolves to exactly that list (it reuses preflight's re.split(r"[-.]", id, 1)[0] lab derivation), and it keeps working if a pin moves to a differently-branded lab. Consequence, stated deliberately: ids from labs the panel does not pin (e.g. fable-*) are not in the review-me list — they are in the raw-catalog fold, which is where an excluded-by-policy lab belongs.
The sticky lookup lists open issues (limit 500) instead of --search. Search is fuzzy and eventually consistent, so a second dispatch minutes after the first could miss its own issue and file a duplicate — precisely the ticket's verification scenario.
Added to test-cursor-review-scripts.yml's path filter. Small scope add: the anchor test only protects the extraction if it runs when the file it parses changes.
Not a reusable workflow — it reads this repo's cursor-review.yml, which is the single source of truth for every consumer's pins, so the README row marks it internal.

Notes

Merge order vs #86 (BE-4818, the pin bump) does not matter; the two touch different lines and my README paragraph was deliberately placed clear of #86's table hunk.
Pre-existing doc inaccuracy left alone (out of scope): the root README describes runs_on and models inputs for cursor-review.yml that the workflow does not currently declare.

_{🔄 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/87 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/28/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4819-cursor-review-catalog-drift` --- ### 📝 Commits (4) - [`40d5f0a`](https://github.com/Comfy-Org/github-workflows/commit/40d5f0aa8b949654849a078dbda939d31b86f8fa) feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819) - [`c36a347`](https://github.com/Comfy-Org/github-workflows/commit/c36a3479731095ddb6c37577fcf1fb4895c992ec) fix(cursor-review): least-privilege the drift check + paginate the sticky lookup (BE-4819) - [`1c47f59`](https://github.com/Comfy-Org/github-workflows/commit/1c47f59fddaf1cd3f3b51731bb214b39fcce8144) fix(cursor-review): close the drift check's silent-miss paths from the review panel (BE-4819) - [`4d7efb9`](https://github.com/Comfy-Org/github-workflows/commit/4d7efb9a0396085a6948ea077b35e04a7de33e4f) fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) (#88) ### 📊 Changes **7 files changed** (+1838 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) ➕ `.github/cursor-review/catalog-drift.py` (+849 -0) ➕ `.github/cursor-review/tests/test_catalog_drift.py` (+736 -0) ➕ `.github/workflows/cursor-review-catalog-drift.yml` (+238 -0) 📝 `.github/workflows/test-cursor-review-scripts.yml` (+9 -0) 📝 `AGENTS.md` (+4 -1) 📝 `README.md` (+1 -0) </details> ### 📄 Description ## ELI-5 The review panel pins four model ids (plus a judge). Cursor keeps swapping models under us: a pinned id gets delisted (every consumer PR then fails the review preflight), or a newer/better model ships and we never notice. The only thing watching for the second case was a hand-written `last checked <date>` comment — which went stale in 13 days, and both swaps the BE-4817 audit found (Opus 5 on 07-24, Kimi K3 on ~07-26) landed inside that window and were caught by a human, not by CI. This adds a robot that checks every Monday. It reads the pins out of `cursor-review.yml` itself, asks Cursor what models actually exist, and writes what it finds into one sticky GitHub issue — which it updates in place each week and closes automatically once the drift is gone. ## What this changes New `.github/workflows/cursor-review-catalog-drift.yml` — `schedule` (`17 6 * * 1`) + `workflow_dispatch`, `contents: read` + `issues: write`: 1. Checkout, install the Cursor CLI exactly the way preflight does, run `cursor-agent models` (hard fail on failure — preflight's "better red than silent" stance). 2. Extract the pins **from `.github/workflows/cursor-review.yml`** — the `Define panel models` heredoc plus the `judge_model` input default. Nothing is duplicated, so the checker stays zero-maintenance when the pins are bumped (e.g. by #86). 3. Compare, using the same whole-token regex preflight uses (`(?<![\w.-])<id>(?![\w.-])`): - **delisted pin** — the urgent case, reported first in the issue **and** it fails the run (see judgment calls); - **unpinned same-lab ids** — grouped by lab, an explicit review-me list rather than a recommendation, with any inline catalog marker (e.g. Fable's `(NO ZDR)`) reproduced verbatim and a caution that an unmarked id is *not* thereby confirmed ZDR-eligible; - **stale audit date** — `last checked YYYY-MM-DD` older than 30 days (or missing). 4. Open / update / close one sticky issue titled `[cursor-review catalog drift] …`, found by exact title prefix. Raw catalog folded into a `<details>` block. No findings → the sticky issue is closed and the run is a green no-op. Comparison + rendering live in `.github/cursor-review/catalog-drift.py` with 30 unit tests; `cursor-review.yml` and the new workflow join `test-cursor-review-scripts.yml`'s path filter. Docs: a row in the root README table, a paragraph in the cursor-review README's panel section (the `last checked` comment stays the human-audit record), and an `AGENTS.md` layout line. ## Verification - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` → **78 passed** (75 pre-existing + the new suite). `actionlint` (which runs shellcheck over the `run:` blocks) clean on every workflow; `check_agents_md.py` passes. - End-to-end dry runs of the script against the **real** `cursor-review.yml` with synthesized catalogs: a clean catalog reports `no drift`; a catalog with `kimi-k3-code` replacing `kimi-k2.7-code` plus a new `claude-opus-5-thinking-max` reproduces exactly the BE-4817 scenario — 1 delisted pin (with `kimi-k3-code` named as the same-lab alternative), 3 unpinned same-lab ids, stale audit date. - One test runs the extractors against the checked-in `cursor-review.yml`, so a refactor of the pins block fails a cheap unit run instead of silently making the weekly check report "no drift" forever. Verified against #86's diff: it changes the pin values and the audit date but keeps every anchor, so extraction still works after that lands. - Falsification of the two hard-fail paths (they deny a capability, so they were checked empirically, not just unit-tested): - `cursor-agent models` without credentials → **exit 1, empty stdout** (ran it locally). So the workflow's `if ! cursor-agent models` guard genuinely catches the auth case rather than proceeding with garbage. A second guard rejects a non-empty but unparseable catalog rather than reporting "all four pins delisted". - "CURSOR_API_KEY is not available here" → falsified: this repo's own `ci-cursor-review.yml` runs (which need the key for preflight) are **succeeding** as recently as 2026-07-28, so the org secret does reach this repo and the missing-secret branch is a real-failure path, not the expected one. (Repo-level secret list shows only `CLOUD_CODE_BOT_PRIVATE_KEY`; the org secret list needs `admin:org`, which I don't have — the succeeding runs are the evidence.) **Not verified, and it cannot be until this merges:** the ticket's `workflow_dispatch` checks (dispatch once, dispatch again to confirm the sticky issue updates in place; a fake-id branch run for the delisted path). GitHub only exposes `workflow_dispatch` for workflows present on the default branch, so neither can run from this branch. The dry runs above cover the same logic offline; the first scheduled/dispatched run is the live confirmation. ## Judgment calls 1. **Logic lives in a tested Python script, not inline `sed`/`yq` in the workflow.** The ticket sketched inline extraction; this repo's convention (AGENTS.md) is that cursor-review logic lives in `.github/cursor-review/` with unit tests, and inline heredocs in a scheduled-only workflow are otherwise untestable before merge. Behavior is the ticket's; only the location changed. 2. **A delisted pin fails the run** (non-urgent findings leave it green). The ticket says "no findings → green no-op" but calls delisted the urgent case to "report prominently" — a red weekly run plus the failure notification is the loudest channel available, and it self-heals the moment the pin is fixed. 3. **Lab prefixes are derived from the pins** rather than hardcoded as `gpt-`/`claude-`/`gemini-`/`kimi-`. Today that resolves to exactly that list (it reuses preflight's `re.split(r"[-.]", id, 1)[0]` lab derivation), and it keeps working if a pin moves to a differently-branded lab. Consequence, stated deliberately: ids from labs the panel does *not* pin (e.g. `fable-*`) are not in the review-me list — they are in the raw-catalog fold, which is where an excluded-by-policy lab belongs. 4. **The sticky lookup lists open issues (limit 500) instead of `--search`.** Search is fuzzy *and* eventually consistent, so a second dispatch minutes after the first could miss its own issue and file a duplicate — precisely the ticket's verification scenario. 5. **Added to `test-cursor-review-scripts.yml`'s path filter.** Small scope add: the anchor test only protects the extraction if it runs when the file it parses changes. 6. **Not a reusable workflow** — it reads *this* repo's `cursor-review.yml`, which is the single source of truth for every consumer's pins, so the README row marks it internal. ## Notes - Merge order vs #86 (BE-4818, the pin bump) does not matter; the two touch different lines and my README paragraph was deliberately placed clear of #86's table hunk. - Pre-existing doc inaccuracy left alone (out of scope): the root README describes `runs_on` and `models` inputs for `cursor-review.yml` that the workflow does not currently declare. --- <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:12 +00:00