[PR #88] [MERGED] fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) #100

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-4819-cursor-review-catalog-drift ← Head: matt/be-4852-catalog-drift-lab-heuristics

📝 Commits (3)

1cc0d8b fix(cursor-review): survive a rebranded model family in the drift check (BE-4852)
89d7bb6 fix(cursor-review): drop the digit id rule, close the judge-pin shape hole (BE-4852)
03dbf7b fix(cursor-review): budget every drift-report list, admit bare o-series ids (BE-4852)

📊 Changes

5 files changed (+514 additions, -59 deletions)

View changed files

📝 .github/cursor-review/README.md (+6 -2)
📝 .github/cursor-review/catalog-drift.py (+250 -46)
📝 .github/cursor-review/tests/test_catalog_drift.py (+253 -10)
📝 .github/workflows/cursor-review-catalog-drift.yml (+4 -0)
📝 README.md (+1 -1)

📄 Description

STACKED — merging lands on matt/be-4819-cursor-review-catalog-drift (owned by @mattmillerai, PR #87), NOT main. The file this PR edits does not exist on main yet; #87 introduces it. Merge #87 first, then this.

ELI-5

The weekly checker looks at Cursor's model list and tells us if one of our four review models disappeared, or if a newer one showed up that we might want to switch to. It figures out "which lab is this?" by taking the first chunk of the model's name — gpt-5.6-sol-max → gpt. That works right up until a lab renames its family: when OpenAI ships o5-pro next to gpt-*, the checker reads the lab as o5, sees that nobody pins o5, and quietly drops it. So the one thing the check exists for — "a newer model shipped and nobody noticed" — is exactly the thing it could miss. This PR adds a second, quieter list for models from families we pin nothing from, so a renamed family shows up instead of vanishing. It also stops the parser treating a hyphenated English word at the start of a line (gpt-based models are …) as if it were a model name.

What changed

Both fixes were deferred out of #87 by the review panel (thread) — they change the design, not just a regex, so they were out of scope there.

1. Unpinned-families catch-all (analyze → report["unpinned_labs"]). Catalog ids whose family prefix matches no pin at all now get their own section instead of being dropped. This is the ticket's first proposed option, chosen over an explicit LAB_ALIASES = {"gpt": "openai", "o": "openai", …} table: the alias table is more precise but reintroduces exactly the hand-maintained list that deriving labs from the pins exists to avoid, and it needs a human edit before the rebrand it is supposed to catch. The catch-all costs one section and stays zero-maintenance.

Deliberate placement, per the ticket's "keep it out of urgent" note:

Not urgent — it is a standing watchlist, not breakage, so it never reddens the weekly run.
Counted in has_findings — it has to be, or a rebranded family renders into a body nobody ever sees (a clean has_findings closes the sticky issue). See the judgment call below on auto-close.
Rendered collapsed, below the same-lab review-me list, so the labs Comfy will never pin cannot crowd out the list that actually needs a decision.

A lab whose only pin was just delisted still counts as pinned, so its successors stay in the same-lab list (with the delisted-pin context) rather than being demoted into the quiet fold — covered by a test.

2. Model ids must carry a digit (_is_model_id). gpt-based is lowercase, hyphenated and has letters, so it parsed as a catalog id and added noise to the review-me list — and, with change 1 in place, would have minted a phantom gpt-based family. Every id a lab has actually shipped carries a version number somewhere; a prose word does not. This is the ticket's first proposed option for (2); "require the token to appear in more than one catalog line" was rejected because a real catalog lists each id exactly once.

The rule lives in the single _is_model_id that the pin validator and the catalog parser both call, so the two cannot drift apart — relaxing it is one edit, not two. test_pin_and_catalog_id_shape_rules_agree (the tripwire the ticket names) is extended rather than weakened: it now also pins code-supernova/gpt-based as rejected and o5-pro as accepted, on both sides.

Verification

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 103 passed (the exact command test-cursor-review-scripts.yml runs). check_agents_md.py --root . passes (one pre-existing CODEOWNERS warning). No shell changed, so shellcheck is unaffected.

New tests: the rebranded-family catch-all (o5-pro → family o5, invisible to the same-lab list), finding-but-never-urgent, a catalog of exactly the pins still having no unpinned families (the auto-close path is not unconditionally broken), a delisted pin's lab still counting as pinned, prose-word rejection, digit-less pin rejected loudly rather than read as delisted, the collapsed-section render, backtick-injection safety on the new section's notes, and a 4000-family catalog still fitting under GitHub's body cap.

Judgment calls

The tightened id rule binds pins too, on purpose. _is_model_id gates extraction, so a digit-less pin (code-supernova is a real Cursor id shape) now fails extraction with an actionable message and a red run, rather than being silently reported as delisted every Monday. I checked this against the live pins rather than assuming: gpt-5.6-sol-max, claude-opus-4-8-thinking-max, gemini-3.1-pro, kimi-k2.7-code and the judge_model default all carry digits and extract cleanly from the real cursor-review.yml (asserted by test_extraction_works_against_the_real_cursor_review_workflow). The cost of the tightening is that a digit-less catalog id would no longer reach the review-me list — it is still reproduced verbatim in the raw-catalog fold, which is where a human reads it anyway.
The sticky issue's auto-close, which the ticket asked me to consider. Folding unpinned_labs into has_findings does not newly pin the issue open: unpinned alone already makes has_findings true on any real catalog, since Cursor lists more tiers per pinned lab than the panel pins four of. The auto-close path was already reserved for a catalog trimmed to exactly the pins, and still is. I chose not to add a separate "worth an issue" gate — it would reintroduce the miss (a rebranded family with no other drift would file nothing), and splitting the two flags is a bigger design change than this deferred follow-up should carry. Flagging it in case you'd rather the catch-all be step-summary-only.
Not done, deliberately: no LAB_ALIASES table (see above), and lab_of itself is unchanged — its docstring now says plainly that it returns a family prefix, not a vendor, and points at the catch-all.

_{🔄 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/88 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/28/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-4819-cursor-review-catalog-drift` ← **Head:** `matt/be-4852-catalog-drift-lab-heuristics` --- ### 📝 Commits (3) - [`1cc0d8b`](https://github.com/Comfy-Org/github-workflows/commit/1cc0d8b35369c3362b4d90c3497b47b83403e0f7) fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) - [`89d7bb6`](https://github.com/Comfy-Org/github-workflows/commit/89d7bb6beda3d696a8e78a8a78b30e0e1868bb86) fix(cursor-review): drop the digit id rule, close the judge-pin shape hole (BE-4852) - [`03dbf7b`](https://github.com/Comfy-Org/github-workflows/commit/03dbf7b8809fa755a521a2c8bf26c1f347f0df1a) fix(cursor-review): budget every drift-report list, admit bare o-series ids (BE-4852) ### 📊 Changes **5 files changed** (+514 additions, -59 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+6 -2) 📝 `.github/cursor-review/catalog-drift.py` (+250 -46) 📝 `.github/cursor-review/tests/test_catalog_drift.py` (+253 -10) 📝 `.github/workflows/cursor-review-catalog-drift.yml` (+4 -0) 📝 `README.md` (+1 -1) </details> ### 📄 Description **STACKED — merging lands on `matt/be-4819-cursor-review-catalog-drift` (owned by @mattmillerai, PR #87), NOT `main`.** The file this PR edits does not exist on `main` yet; #87 introduces it. Merge #87 first, then this. ## ELI-5 The weekly checker looks at Cursor's model list and tells us if one of our four review models disappeared, or if a newer one showed up that we might want to switch to. It figures out "which lab is this?" by taking the first chunk of the model's name — `gpt-5.6-sol-max` → `gpt`. That works right up until a lab renames its family: when OpenAI ships `o5-pro` next to `gpt-*`, the checker reads the lab as `o5`, sees that nobody pins `o5`, and quietly drops it. So the one thing the check exists for — "a newer model shipped and nobody noticed" — is exactly the thing it could miss. This PR adds a second, quieter list for models from families we pin *nothing* from, so a renamed family shows up instead of vanishing. It also stops the parser treating a hyphenated English word at the start of a line (`gpt-based models are …`) as if it were a model name. ## What changed Both fixes were deferred out of #87 by the review panel ([thread](https://github.com/Comfy-Org/github-workflows/pull/87#discussion_r3662377669)) — they change the *design*, not just a regex, so they were out of scope there. **1. Unpinned-families catch-all (`analyze` → `report["unpinned_labs"]`).** Catalog ids whose family prefix matches no pin at all now get their own section instead of being dropped. This is the ticket's first proposed option, chosen over an explicit `LAB_ALIASES = {"gpt": "openai", "o": "openai", …}` table: the alias table is more precise but reintroduces exactly the hand-maintained list that deriving labs from the pins exists to avoid, and it needs a human edit *before* the rebrand it is supposed to catch. The catch-all costs one section and stays zero-maintenance. Deliberate placement, per the ticket's "keep it out of `urgent`" note: - **Not `urgent`** — it is a standing watchlist, not breakage, so it never reddens the weekly run. - **Counted in `has_findings`** — it has to be, or a rebranded family renders into a body nobody ever sees (a clean `has_findings` closes the sticky issue). See the judgment call below on auto-close. - **Rendered collapsed**, below the same-lab review-me list, so the labs Comfy will never pin cannot crowd out the list that actually needs a decision. A lab whose only pin was just delisted still counts as pinned, so its successors stay in the same-lab list (with the delisted-pin context) rather than being demoted into the quiet fold — covered by a test. **2. Model ids must carry a digit (`_is_model_id`).** `gpt-based` is lowercase, hyphenated and has letters, so it parsed as a catalog id and added noise to the review-me list — and, with change 1 in place, would have minted a phantom `gpt-based` family. Every id a lab has actually shipped carries a version number somewhere; a prose word does not. This is the ticket's first proposed option for (2); "require the token to appear in more than one catalog line" was rejected because a real catalog lists each id exactly once. The rule lives in the single `_is_model_id` that the pin validator *and* the catalog parser both call, so the two cannot drift apart — relaxing it is one edit, not two. `test_pin_and_catalog_id_shape_rules_agree` (the tripwire the ticket names) is extended rather than weakened: it now also pins `code-supernova`/`gpt-based` as rejected and `o5-pro` as accepted, on both sides. ## Verification `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — **103 passed** (the exact command `test-cursor-review-scripts.yml` runs). `check_agents_md.py --root .` passes (one pre-existing CODEOWNERS warning). No shell changed, so `shellcheck` is unaffected. New tests: the rebranded-family catch-all (`o5-pro` → family `o5`, invisible to the same-lab list), finding-but-never-urgent, a catalog of exactly the pins still having no unpinned families (the auto-close path is not unconditionally broken), a delisted pin's lab still counting as pinned, prose-word rejection, digit-less pin rejected loudly rather than read as delisted, the collapsed-section render, backtick-injection safety on the new section's notes, and a 4000-family catalog still fitting under GitHub's body cap. ## Judgment calls - **The tightened id rule binds pins too, on purpose.** `_is_model_id` gates extraction, so a digit-less pin (`code-supernova` is a real Cursor id shape) now fails extraction with an actionable message and a red run, rather than being silently reported as delisted every Monday. I checked this against the live pins rather than assuming: `gpt-5.6-sol-max`, `claude-opus-4-8-thinking-max`, `gemini-3.1-pro`, `kimi-k2.7-code` and the `judge_model` default all carry digits and extract cleanly from the real `cursor-review.yml` (asserted by `test_extraction_works_against_the_real_cursor_review_workflow`). The cost of the tightening is that a digit-less *catalog* id would no longer reach the review-me list — it is still reproduced verbatim in the raw-catalog fold, which is where a human reads it anyway. - **The sticky issue's auto-close, which the ticket asked me to consider.** Folding `unpinned_labs` into `has_findings` does not newly pin the issue open: `unpinned` alone already makes `has_findings` true on any real catalog, since Cursor lists more tiers per pinned lab than the panel pins four of. The auto-close path was already reserved for a catalog trimmed to exactly the pins, and still is. I chose not to add a separate "worth an issue" gate — it would reintroduce the miss (a rebranded family with no other drift would file nothing), and splitting the two flags is a bigger design change than this deferred follow-up should carry. Flagging it in case you'd rather the catch-all be step-summary-only. - Not done, deliberately: no `LAB_ALIASES` table (see above), and `lab_of` itself is unchanged — its docstring now says plainly that it returns a *family* prefix, not a vendor, and points at the catch-all. --- <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:13 +00:00