fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) - #88
Merged
mattmillerai merged 3 commits intoAug 4, 2026
Conversation
…ck (BE-4852)
Two heuristics in the catalog-drift checker distorted the "unpinned same-lab
ids" review-me list. Neither could produce a false green, but both undercut the
headline feature — noticing that a newer model shipped.
1. `lab_of()` equates a lab with an id's first `-`/`.`-separated token, and labs
are derived from the pins themselves (what keeps the checker
zero-maintenance). An existing lab shipping under a NEW family prefix —
OpenAI's `o<n>` series alongside `gpt-*` is the precedent — therefore
resolves to a lab nobody pins and vanished from the review-me list. Add a
quieter catch-all section listing catalog ids whose family prefix matches no
pin at all, so a rebranded family surfaces instead of dropping out. It is a
finding (or a rebranded family would render into a body nobody sees) but
never `urgent`, and renders collapsed below the same-lab list.
2. `catalog_entries()` admitted any lowercase hyphenated first token with a
letter, so a prose line ("gpt-based models are …") parsed as a bogus id.
Require a digit as well: every id a lab has shipped carries a version number,
a prose word does not. The rule lives in the one `_is_model_id` the pin
validator and the catalog parser share, so the two cannot drift apart —
`test_pin_and_catalog_id_shape_rules_agree` stays the tripwire, and all five
live pins satisfy it.
mattmillerai
added
agent-coded
mattmillerai
marked this pull request as ready for review
July 28, 2026 09:00
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
… hole (BE-4852) Review panel findings on the BE-4852 catch-all: * `_HAS_DIGIT` gated the CATALOG parser as well as the pin validator, so a real digit-less id — Cursor ships `code-supernova` — was dropped by `catalog_entries` before parsing. A newly shipped digit-less family therefore never reached the unpinned-families catch-all this PR adds and the run reported clean: exactly the "a new model shipped and nobody noticed" silence the check exists to end. It also hard-failed extraction every run if such an id were ever pinned. Removed: over-reporting a hyphenated prose word (one row in a collapsed, never-urgent list) is the cheaper failure, and the direction `present` already argues for. * `extract_judge_model` never called `_is_model_id`, though the docstring claimed it did. Its scalar/whitespace checks are strictly weaker, so a separator-less judge default (`o3`) extracted cleanly and then failed `present()` — a phantom URGENT "delisted pin" every Monday. It now applies the same shared rule, making the "both sides agree" contract true. * The unpinned-families fold had no per-section budget, so the blunt `MAX_BODY_CHARS` clamp could cut into it, leaving `<details>` unterminated and dropping the stale-audit and raw-catalog sections below. Budgeted at `MAX_FAMILY_LABS`/`MAX_FAMILY_IDS`, naming what it truncated. * Grouping now keys a dict by lab instead of a linear scan per entry (O(N)). Tests updated to match: the two that encoded the digit rule now assert the accepted trade, plus new coverage for the judge shape hole, a digit-less family reaching the catch-all, and the fold staying well-formed under truncation.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
…es ids (BE-4852) Review-panel fixes on the drift check: - Body budget (medium): cap every per-lab id list (same-lab review-me, delisted alternatives) at MAX_FAMILY_IDS, cap rendered notes at MAX_NOTE_CHARS, char-budget the families fold (row caps bound counts, not chars), and grant the raw-catalog fold only the MAX_BODY_CHARS the report has not used instead of a fixed 40K — so the blunt body[:N] clamp can no longer slice mid-markup. Worst-case test added. - Bare separator-less ids (low): `_is_model_id` now requires a `-`/`.` OR a digit (plus a letter), so a real bare id (`o3`) reaches the unpinned-families catch-all instead of being dropped before parsing — the same silence that got the digit rule reverted. - Prose-token masking (low): the test comment overclaimed that admitted prose "cannot mask a real pin"; a line whose first token equals a pin does mask its delisted finding. Comment corrected and the limitation pinned by an explicit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mattmillerai
merged commit Aug 4, 2026
3167ec1
into
matt/be-4819-cursor-review-catalog-drift
4 checks passed
Contributor
Author
|
Merged without pre-merge approval — SOC 2 justification Justification: Advisory CI-only change to this repo's own reusable workflows, merged by the author to clear a backlog that was going stale and self-conflicting; no consumer repo can adopt it without a separate, separately-reviewed SHA-bump PR, so the peer-approval gate still applies downstream before this change reaches any other repo's CI. Detail:
Post-merge peer review remains welcome and will be tracked on the corresponding tracking issue. |
mattmillerai
added a commit
that referenced
this pull request
Aug 4, 2026
…ck (BE-4852) (#88) * fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) Two heuristics in the catalog-drift checker distorted the "unpinned same-lab ids" review-me list. Neither could produce a false green, but both undercut the headline feature — noticing that a newer model shipped. 1. `lab_of()` equates a lab with an id's first `-`/`.`-separated token, and labs are derived from the pins themselves (what keeps the checker zero-maintenance). An existing lab shipping under a NEW family prefix — OpenAI's `o<n>` series alongside `gpt-*` is the precedent — therefore resolves to a lab nobody pins and vanished from the review-me list. Add a quieter catch-all section listing catalog ids whose family prefix matches no pin at all, so a rebranded family surfaces instead of dropping out. It is a finding (or a rebranded family would render into a body nobody sees) but never `urgent`, and renders collapsed below the same-lab list. 2. `catalog_entries()` admitted any lowercase hyphenated first token with a letter, so a prose line ("gpt-based models are …") parsed as a bogus id. Require a digit as well: every id a lab has shipped carries a version number, a prose word does not. The rule lives in the one `_is_model_id` the pin validator and the catalog parser share, so the two cannot drift apart — `test_pin_and_catalog_id_shape_rules_agree` stays the tripwire, and all five live pins satisfy it. * fix(cursor-review): drop the digit id rule, close the judge-pin shape hole (BE-4852) Review panel findings on the BE-4852 catch-all: * `_HAS_DIGIT` gated the CATALOG parser as well as the pin validator, so a real digit-less id — Cursor ships `code-supernova` — was dropped by `catalog_entries` before parsing. A newly shipped digit-less family therefore never reached the unpinned-families catch-all this PR adds and the run reported clean: exactly the "a new model shipped and nobody noticed" silence the check exists to end. It also hard-failed extraction every run if such an id were ever pinned. Removed: over-reporting a hyphenated prose word (one row in a collapsed, never-urgent list) is the cheaper failure, and the direction `present` already argues for. * `extract_judge_model` never called `_is_model_id`, though the docstring claimed it did. Its scalar/whitespace checks are strictly weaker, so a separator-less judge default (`o3`) extracted cleanly and then failed `present()` — a phantom URGENT "delisted pin" every Monday. It now applies the same shared rule, making the "both sides agree" contract true. * The unpinned-families fold had no per-section budget, so the blunt `MAX_BODY_CHARS` clamp could cut into it, leaving `<details>` unterminated and dropping the stale-audit and raw-catalog sections below. Budgeted at `MAX_FAMILY_LABS`/`MAX_FAMILY_IDS`, naming what it truncated. * Grouping now keys a dict by lab instead of a linear scan per entry (O(N)). Tests updated to match: the two that encoded the digit rule now assert the accepted trade, plus new coverage for the judge shape hole, a digit-less family reaching the catch-all, and the fold staying well-formed under truncation. * fix(cursor-review): budget every drift-report list, admit bare o-series ids (BE-4852) Review-panel fixes on the drift check: - Body budget (medium): cap every per-lab id list (same-lab review-me, delisted alternatives) at MAX_FAMILY_IDS, cap rendered notes at MAX_NOTE_CHARS, char-budget the families fold (row caps bound counts, not chars), and grant the raw-catalog fold only the MAX_BODY_CHARS the report has not used instead of a fixed 40K — so the blunt body[:N] clamp can no longer slice mid-markup. Worst-case test added. - Bare separator-less ids (low): `_is_model_id` now requires a `-`/`.` OR a digit (plus a letter), so a real bare id (`o3`) reaches the unpinned-families catch-all instead of being dropped before parsing — the same silence that got the digit rule reverted. - Prose-token masking (low): the test comment overclaimed that admitted prose "cannot mask a real pin"; a line whose first token equals a pin does mask its delisted finding. Comment corrected and the limitation pinned by an explicit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
mattmillerai
added a commit
that referenced
this pull request
Aug 4, 2026
…E-4819) (#87) * feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819) The per-PR preflight fails loud when a PINNED model id is delisted, but only at review time, on somebody's PR, after the pin has already gone bad — and by design it never notices a NEWER model shipping. The only guard against that second case was a `last checked <date>` comment above the pins, which went stale inside 13 days: both catalog swaps found by the BE-4817 audit landed in that window and were caught by hand, not by CI. Add a scheduled (Mon 06:17 UTC) + dispatchable check that reads the pins OUT of cursor-review.yml — panel heredoc plus the judge_model input default, never a duplicated list — diffs them against `cursor-agent models`, and reports delisted pins, unpinned same-lab catalog ids (a review-me list, not a recommendation; NO-ZDR markers surfaced verbatim), and a `last checked` date older than 30 days through one sticky issue that is updated in place and closed automatically on a clean run. Comparison + rendering live in .github/cursor-review/catalog-drift.py with unit tests, including one that runs the extractors against the real cursor-review.yml so a refactor there can't silently blind the check; cursor-review.yml joins that test job's path filter for the same reason. * fix(cursor-review): least-privilege the drift check + paginate the sticky lookup (BE-4819) Addresses the CodeRabbit review on #87. * Split the sticky-issue write out of the job that runs Cursor's installer. `curl https://cursor.com/install | bash` is a mutable third-party artifact Cursor ships no checksummed/versioned build of, and it ran in the same job that held `issues: write` — so every later `gh` call was downstream of code Cursor can change at will. The `drift` job now holds only `contents: read` and hands its rendered report to a new credential-free-of-installers `sticky` job via an artifact; `issues: write` exists in exactly one job that runs no third-party code. Same boundary groom.yml draws around its agent jobs, and it drops the workflow-level `permissions:` block that granted the write scope to every job (zizmor's excess-permissions finding). * Paginate the sticky-issue lookup. `gh issue list --limit 500` silently truncates, and a sticky that falls off the end of that page gets filed a second time. Walk every open issue with `gh api --paginate`, filtering out PRs (`/issues` returns them; `gh issue list` did not). * `value[0] not in ">|"` instead of `not value[0] in ">|"` (Ruff E713). No behavior change to the drift report itself; the 78 script tests still pass. * fix(cursor-review): close the drift check's silent-miss paths from the review panel (BE-4819) Addresses the cursor-review panel findings on #87. * HIGH — `present()` scanned the RAW catalog text with a whole-token regex, so a delisted pin still mentioned anywhere in the output was treated as present and never reported. That is not an edge case: a delist normally ships WITH a note naming the id it replaces ("kimi-k3-code (replaces kimi-k2.7-code)"), so the urgent path would have stayed silent in exactly the scenario it exists for, while consumer PRs went red at preflight. Now compares against the PARSED catalog ids, the same source `unpinned` already used. The trade-off is deliberate: a garbled catalog over-reports (loud) instead of under-reporting. * HIGH — the installer step no longer receives `CURSOR_API_KEY`. The presence check moved to its own step, so the org secret is not in scope while an unpinned remote script runs. (Pinning the installer itself spans all four call sites in this repo — proposed as a follow-up.) * MEDIUM — a new finding class: a pinned model whose catalog line now carries a NO-ZDR marker. Nothing breaks, so nothing flagged it, and private review diffs kept flowing to a model that may retain them. This is the one marker the checker interprets rather than reproduces, and only for a pin already in service; it reddens the run like a delisted pin. * MEDIUM — the quoted-token fallback (and the strict-JSON path) now reject any entry that is not a bare model id, instead of adopting a quoted comment or a "TODO pick a fifth" placeholder as a pin. A guessed pin set means false delisted alarms every Monday, or a real pin dropped and never monitored. * MEDIUM — the raw catalog is never echoed unfenced. `cursor-agent` output is semi-trusted and this repo is public, so a line beginning `::` would be executed by the runner (`::add-mask::`, `::stop-commands::`). The shell echo is wrapped in a `::stop-commands::<random>` fence; the script's unparseable-catalog dump prefixes every line with `| `. * MEDIUM — the sticky key is now label + title prefix, not the public title alone, so the bot cannot adopt and rewrite an issue anyone opened with that title. (Pagination landed in the previous commit.) * LOW — `last checked` is read only within 40 lines above the panel heredoc, so an unrelated match elsewhere can't shadow the real audit date; and a future-dated typo now counts as stale instead of reading as fresh and suppressing the alert until 30 days past that future date. * LOW — merge catalog notes on a repeated id instead of keeping the first, so a later `(NO ZDR)` on a duplicate line is not discarded. * NIT — catalog notes render in a length-sized, padded inline code span. They are unconstrained third-party text reproduced in a bot-authored issue in a PUBLIC repo; a bare single-backtick span let a note break out and inject markdown or an `@mention` that notifies real people. Also requires a model id to contain a letter, so a numbered catalog format parses as nothing (routing to the diagnostic "format may have changed" exit) rather than as the ids ['1.', '2.'] with every real pin reading as delisted. The `lab_of` prefix heuristic dropping a lab's rebranded family, and prose words admitted as catalog ids, are left as-is and proposed as a follow-up — both distort the review-me list, neither can produce a false green. 94 tests (was 78); actionlint + shellcheck clean. * fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) (#88) * fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) Two heuristics in the catalog-drift checker distorted the "unpinned same-lab ids" review-me list. Neither could produce a false green, but both undercut the headline feature — noticing that a newer model shipped. 1. `lab_of()` equates a lab with an id's first `-`/`.`-separated token, and labs are derived from the pins themselves (what keeps the checker zero-maintenance). An existing lab shipping under a NEW family prefix — OpenAI's `o<n>` series alongside `gpt-*` is the precedent — therefore resolves to a lab nobody pins and vanished from the review-me list. Add a quieter catch-all section listing catalog ids whose family prefix matches no pin at all, so a rebranded family surfaces instead of dropping out. It is a finding (or a rebranded family would render into a body nobody sees) but never `urgent`, and renders collapsed below the same-lab list. 2. `catalog_entries()` admitted any lowercase hyphenated first token with a letter, so a prose line ("gpt-based models are …") parsed as a bogus id. Require a digit as well: every id a lab has shipped carries a version number, a prose word does not. The rule lives in the one `_is_model_id` the pin validator and the catalog parser share, so the two cannot drift apart — `test_pin_and_catalog_id_shape_rules_agree` stays the tripwire, and all five live pins satisfy it. * fix(cursor-review): drop the digit id rule, close the judge-pin shape hole (BE-4852) Review panel findings on the BE-4852 catch-all: * `_HAS_DIGIT` gated the CATALOG parser as well as the pin validator, so a real digit-less id — Cursor ships `code-supernova` — was dropped by `catalog_entries` before parsing. A newly shipped digit-less family therefore never reached the unpinned-families catch-all this PR adds and the run reported clean: exactly the "a new model shipped and nobody noticed" silence the check exists to end. It also hard-failed extraction every run if such an id were ever pinned. Removed: over-reporting a hyphenated prose word (one row in a collapsed, never-urgent list) is the cheaper failure, and the direction `present` already argues for. * `extract_judge_model` never called `_is_model_id`, though the docstring claimed it did. Its scalar/whitespace checks are strictly weaker, so a separator-less judge default (`o3`) extracted cleanly and then failed `present()` — a phantom URGENT "delisted pin" every Monday. It now applies the same shared rule, making the "both sides agree" contract true. * The unpinned-families fold had no per-section budget, so the blunt `MAX_BODY_CHARS` clamp could cut into it, leaving `<details>` unterminated and dropping the stale-audit and raw-catalog sections below. Budgeted at `MAX_FAMILY_LABS`/`MAX_FAMILY_IDS`, naming what it truncated. * Grouping now keys a dict by lab instead of a linear scan per entry (O(N)). Tests updated to match: the two that encoded the digit rule now assert the accepted trade, plus new coverage for the judge shape hole, a digit-less family reaching the catch-all, and the fold staying well-formed under truncation. * fix(cursor-review): budget every drift-report list, admit bare o-series ids (BE-4852) Review-panel fixes on the drift check: - Body budget (medium): cap every per-lab id list (same-lab review-me, delisted alternatives) at MAX_FAMILY_IDS, cap rendered notes at MAX_NOTE_CHARS, char-budget the families fold (row caps bound counts, not chars), and grant the raw-catalog fold only the MAX_BODY_CHARS the report has not used instead of a fixed 40K — so the blunt body[:N] clamp can no longer slice mid-markup. Worst-case test added. - Bare separator-less ids (low): `_is_model_id` now requires a `-`/`.` OR a digit (plus a letter), so a real bare id (`o3`) reaches the unpinned-families catch-all instead of being dropped before parsing — the same silence that got the digit rule reverted. - Prose-token masking (low): the test comment overclaimed that admitted prose "cannot mask a real pin"; a line whose first token equals a pin does mask its delisted finding. Comment corrected and the limitation pinned by an explicit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
STACKED — merging lands on
matt/be-4819-cursor-review-catalog-drift(owned by @mattmillerai, PR #87), NOTmain. The file this PR edits does not exist onmainyet; #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 shipso5-pronext togpt-*, the checker reads the lab aso5, sees that nobody pinso5, 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 explicitLAB_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:urgent— it is a standing watchlist, not breakage, so it never reddens the weekly run.has_findings— it has to be, or a rebranded family renders into a body nobody ever sees (a cleanhas_findingscloses the sticky issue). See the judgment call below on auto-close.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-basedis 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 phantomgpt-basedfamily. 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_idthat 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 pinscode-supernova/gpt-basedas rejected ando5-proas accepted, on both sides.Verification
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'— 103 passed (the exact commandtest-cursor-review-scripts.ymlruns).check_agents_md.py --root .passes (one pre-existing CODEOWNERS warning). No shell changed, soshellcheckis unaffected.New tests: the rebranded-family catch-all (
o5-pro→ familyo5, 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
_is_model_idgates extraction, so a digit-less pin (code-supernovais 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-codeand thejudge_modeldefault all carry digits and extract cleanly from the realcursor-review.yml(asserted bytest_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.unpinned_labsintohas_findingsdoes not newly pin the issue open:unpinnedalone already makeshas_findingstrue 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.LAB_ALIASEStable (see above), andlab_ofitself is unchanged — its docstring now says plainly that it returns a family prefix, not a vendor, and points at the catch-all.