From 1cc0d8b35369c3362b4d90c3497b47b83403e0f7 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 28 Jul 2026 01:59:30 -0700 Subject: [PATCH 1/3] fix(cursor-review): survive a rebranded model family in the drift check (BE-4852) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` 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. --- .github/cursor-review/README.md | 8 +- .github/cursor-review/catalog-drift.py | 151 +++++++++++++++--- .../cursor-review/tests/test_catalog_drift.py | 112 +++++++++++-- .../workflows/cursor-review-catalog-drift.yml | 4 + README.md | 2 +- 5 files changed, 242 insertions(+), 35 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index e32e7a7..695139f 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -86,7 +86,11 @@ reclassified non-ZDR — is machine-checked weekly by [`cursor-review-catalog-drift.yml`](../workflows/cursor-review-catalog-drift.yml), which diffs the pins against `cursor-agent models` and files one sticky `[cursor-review catalog drift]` issue listing delisted pins, **pins whose catalog -line now says NO-ZDR**, unpinned same-lab catalog ids, and an audit date older +line now says NO-ZDR**, unpinned same-lab catalog ids, catalog ids from families +the panel pins nothing from (a quieter, collapsed catch-all — a lab equates to an +id's first `-`/`.`-separated token, so a lab the panel *does* pin can only +surface there after rebranding under a new prefix, e.g. OpenAI's `o` series +alongside `gpt-*`), and an audit date older than 30 days. It reports; it never bumps a pin — picking the newest highest-reasoning **ZDR-eligible** model stays a human call (Cursor marks only NO-ZDR models inline, e.g. Fable's `(NO ZDR)` suffix). That marker is the one @@ -107,7 +111,7 @@ it has gone stale. | [`extract-findings.py`](extract-findings.py) | Parses a cell's raw `cursor-agent` output into a normalized findings record. Always emits structured JSON — even on empty output or parse failure — so the consolidate step has uniform input. | | [`post-review.py`](post-review.py) | Reads the judge's consolidated findings and posts **one** PR review with line-anchored inline comments and severity badges. | | [`gate-unresolved.py`](gate-unresolved.py) | The opt-in blocking gate (`blocking: true`). Queries the PR's review threads and exits non-zero while any cursor-review finding thread is unresolved. | -| [`catalog-drift.py`](catalog-drift.py) | Backs the weekly catalog-drift check. Extracts the pins from `cursor-review.yml`, diffs them against raw `cursor-agent models` output, and renders the sticky issue title + body (delisted pins, pins marked NO-ZDR, unpinned same-lab ids, stale audit date). Reports only — it never edits a pin. | +| [`catalog-drift.py`](catalog-drift.py) | Backs the weekly catalog-drift check. Extracts the pins from `cursor-review.yml`, diffs them against raw `cursor-agent models` output, and renders the sticky issue title + body (delisted pins, pins marked NO-ZDR, unpinned same-lab ids, catalog ids from unpinned families, stale audit date). Reports only — it never edits a pin. | | [`slack-notify.sh`](slack-notify.sh) | Sends the start/complete Slack DMs to the triggerer (no-ops without a token). | ## Adopt it in your repo diff --git a/.github/cursor-review/catalog-drift.py b/.github/cursor-review/catalog-drift.py index 278a2ca..6357d45 100644 --- a/.github/cursor-review/catalog-drift.py +++ b/.github/cursor-review/catalog-drift.py @@ -10,7 +10,7 @@ landed in that window and were caught by hand, not by CI. This script is the machine half of that audit. Given the workflow file and the -raw output of `cursor-agent models`, it reports four kinds of drift: +raw output of `cursor-agent models`, it reports five kinds of drift: * **delisted pin** — a pinned id (panel or judge) is absent from the live catalog. Urgent: consumer PRs will start failing preflight. @@ -24,6 +24,11 @@ picking "newest highest-reasoning ZDR-eligible" needs human judgment, and ZDR especially — Cursor only marks NO-ZDR inline (e.g. a `(NO ZDR)` suffix), so any such marker on the line is surfaced verbatim rather than interpreted. + * **unpinned model families** — catalog ids whose family prefix matches no pin + at all. Quieter than the above (mostly labs the panel will never pin, so it + renders collapsed and is never `urgent`), but it is the ONLY place a lab the + panel already pins can surface after rebranding under a new prefix — see + `lab_of` and the catch-all in `analyze`. * **stale audit date** — the `last checked YYYY-MM-DD` comment is older than `--stale-days` (or missing entirely). @@ -71,6 +76,12 @@ # carries at least one `-` or `.` — that separator requirement is what keeps # prose lines ("Available models:") out of the parsed id list. _ID_TOKEN = re.compile(r"^[a-z0-9][a-z0-9._-]*$") +# Every model id a lab has shipped to Cursor's catalog carries a version number +# somewhere (`gpt-5.6-sol-max`, `claude-opus-4-8-thinking-max`, `kimi-k2.7-code`, +# `o5-pro`), and a hyphenated PROSE word at the head of a catalog line does not +# ("`gpt-based` models are …"). Requiring a digit is what separates the two — see +# `_is_model_id`. +_HAS_DIGIT = re.compile(r"[0-9]") _BULLET = re.compile(r"^[-*>•\s]+") # The ONE catalog marker worth interpreting rather than merely reproducing: a # PINNED model reclassified non-ZDR means private review diffs are flowing to a @@ -93,17 +104,36 @@ class ExtractionError(Exception): def _is_model_id(token): """True for a bare catalog/model id — see `_ID_TOKEN`. - The letter requirement keeps a numbered-list marker (`1.` in a hypothetical - `1. gpt-5.6-sol-max` catalog line) from parsing as an id. That matters more - than it looks: `present()` trusts this parse, so a catalog of ['1.', '2.'] - would look perfectly valid while reporting every real pin as delisted. - Parsing nothing instead routes it to `main`'s diagnostic "no ids could be - parsed — the format may have changed" hard exit. + Three requirements beyond the character class, each closing a different + misparse: + + * a `-`/`.` **separator**, which keeps prose lines ("Available models:") + out of the parsed id list; + * a **letter**, which keeps a numbered-list marker (`1.` in a hypothetical + `1. gpt-5.6-sol-max` catalog line) from parsing as an id. That matters + more than it looks: `present()` trusts this parse, so a catalog of + ['1.', '2.'] would look perfectly valid while reporting every real pin as + delisted; + * a **digit**, which keeps a hyphenated prose word at the head of a catalog + line ("`gpt-based` models are …") from being admitted as a bogus id — it + would otherwise add noise to the review-me list and, since BE-4852, mint + a phantom `gpt-based`-style entry in the unpinned-families section. + + Parsing nothing at all instead routes a garbled catalog to `main`'s + diagnostic "no ids could be parsed — the format may have changed" hard exit. + + All three bind the PIN validator too (`extract_panel_models` / + `extract_judge_model` call this): a shape the catalog parser would never emit + must not be accepted as a pin, or that pin reads as delisted every run. If a + lab ever ships a separator-less or digit-less id, relax the rule HERE — one + function, both sides — and `test_pin_and_catalog_id_shape_rules_agree` keeps + them honest. """ return ( bool(_ID_TOKEN.match(token)) and re.search(r"[-.]", token) is not None and re.search(r"[a-z]", token) is not None + and _HAS_DIGIT.search(token) is not None ) @@ -163,10 +193,10 @@ def extract_panel_models(workflow_text): if bad: raise ExtractionError( f"the /tmp/models.json heredoc yielded entries that are not bare model ids: {bad!r} " - "(expected a single lowercase token containing a `-` or `.`, e.g. " - "`gpt-5.6-sol-max`). If a lab has shipped a separator-less id, relax `_is_model_id` " - "here AND in the catalog parser together — they must agree, or the pin will read as " - "delisted every run." + "(expected a single lowercase token containing a `-` or `.` AND a digit, e.g. " + "`gpt-5.6-sol-max`). If a lab has shipped a separator-less or digit-less id, relax " + "`_is_model_id` — it is the ONE rule the pin validator and the catalog parser share, " + "and they must agree, or the pin will read as delisted every run." ) return models @@ -281,7 +311,15 @@ def present(model_id, catalog_ids): def lab_of(model_id): - """Lab prefix of an id — `gpt-5.6-sol-max` -> `gpt` (preflight's split).""" + """Lab prefix of an id — `gpt-5.6-sol-max` -> `gpt` (preflight's split). + + A *family* prefix, strictly — not a vendor. One lab can ship under several + (OpenAI's `o` series alongside `gpt-*` is the precedent), and nothing in a + bare id says the two belong together. Mapping them would need a hand-kept + alias table, which is exactly the maintenance burden deriving labs from the + pins avoids; `analyze` handles the rebrand case with the unpinned-families + catch-all instead (BE-4852). + """ return re.split(r"[-.]", model_id, maxsplit=1)[0].lower() @@ -332,6 +370,7 @@ def roles_of(model_id): if lab not in labs: labs.append(lab) pinned_set = set(pinned) + pinned_labs = set(labs) unpinned = [] for lab in labs: candidates = [ @@ -346,6 +385,28 @@ def roles_of(model_id): } ) + # The catch-all (BE-4852): catalog ids whose family prefix matches NO pin. + # `lab_of` equates a lab with an id's first token, so an existing lab + # shipping under a NEW family prefix — OpenAI's `o` series alongside + # `gpt-*` is the precedent — resolves to a "lab" nobody pins and would drop + # out of the review-me list above entirely. That is precisely the "a newer + # model shipped and nobody noticed" case this whole check exists to catch, so + # it gets its own quieter section rather than a hand-maintained prefix→lab + # alias table (which is the maintenance burden deriving labs from the pins + # exists to avoid). Most of what lands here is genuinely uninteresting — + # labs Comfy will never pin — hence: a finding, but never `urgent`, and + # rendered collapsed. + unpinned_labs = [] + for model_id, note in entries: + lab = lab_of(model_id) + if lab in pinned_labs: + continue + group = next((g for g in unpinned_labs if g["lab"] == lab), None) + if group is None: + group = {"lab": lab, "candidates": []} + unpinned_labs.append(group) + group["candidates"].append({"id": model_id, "note": note}) + audit = { "last_checked": last_checked.isoformat() if last_checked else None, "age_days": (today - last_checked).days if last_checked else None, @@ -365,11 +426,19 @@ def roles_of(model_id): "delisted": delisted, "zdr_risk": zdr_risk, "unpinned": unpinned, + "unpinned_labs": unpinned_labs, "audit": audit, # `urgent` is what reddens the weekly run: a pin the preflight is about - # to reject, or a pin that is no longer ZDR-eligible. + # to reject, or a pin that is no longer ZDR-eligible. `unpinned_labs` is + # deliberately NOT here — it is a standing watchlist, not a breakage. "urgent": bool(delisted or zdr_risk), - "has_findings": bool(delisted or zdr_risk or unpinned or audit["stale"]), + # `unpinned_labs` DOES count, or a rebranded family would render into a + # body nobody ever sees (a clean `has_findings` closes the sticky issue). + # It does not newly pin the issue open: `unpinned` alone already makes + # this true on any real catalog, which lists more tiers per pinned lab + # than the panel pins — the auto-close path was already reserved for a + # catalog trimmed to exactly the pins. + "has_findings": bool(delisted or zdr_risk or unpinned or unpinned_labs or audit["stale"]), } @@ -390,6 +459,13 @@ def summary_line(report): count = sum(len(g["candidates"]) for g in report["unpinned"]) if count: bits.append(f"{count} unpinned same-lab id{'s' if count != 1 else ''}") + families = report.get("unpinned_labs") or [] + if families: + ids = sum(len(g["candidates"]) for g in families) + bits.append( + f"{len(families)} unpinned famil{'y' if len(families) == 1 else 'ies'} " + f"({ids} id{'s' if ids != 1 else ''})" + ) audit = report["audit"] if audit["stale"]: if audit["last_checked"] is None: @@ -426,6 +502,14 @@ def _inline_code(text): return f"{delim}{pad}{flat}{pad}{delim}" +def _candidate_list(candidates): + """Bullet list of `{id, note}` rows — shared by both unpinned sections.""" + return "\n".join( + f"- `{c['id']}`" + (f" — {_inline_code(c['note'])}" if c["note"] else "") + for c in candidates + ) + + def render_body(report, catalog_text, run_url=None, checked_at=None): """Render the sticky issue body (also used as the run's step summary).""" pins = report["pins"] @@ -441,8 +525,9 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): ) else: out.append( - "No drift: every pinned model id is present in Cursor's live catalog, no unpinned " - "same-lab ids are available, and the audit date is current." + "No drift: every pinned model id is present in Cursor's live catalog, the catalog " + "offers no unpinned ids — from a pinned lab or an unpinned family — and the audit " + "date is current." ) meta = [] @@ -493,12 +578,34 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): for group in report["unpinned"]: pinned_now = ", ".join(f"`{m}`" for m in group["pinned"]) or "_none_" out.append(f"**`{group['lab']}`** (pinned: {pinned_now})") - out.append( - "\n".join( - f"- `{c['id']}`" + (f" — {_inline_code(c['note'])}" if c["note"] else "") - for c in group["candidates"] - ) - ) + out.append(_candidate_list(group["candidates"])) + + families = report.get("unpinned_labs") or [] + if families: + # Collapsed on purpose: most of this is labs the panel will never pin, so + # it must not crowd out the same-lab review-me list above. It exists for + # the one row that matters — a lab already on the panel shipping under a + # new family prefix, which `lab_of` cannot tell from a new vendor. + detail = [ + "
", + f"Catalog ids from unpinned model families " + f"({', '.join('`' + g['lab'] + '`' for g in families)})", + "", + "Families the panel pins **nothing** from. Usually just labs Comfy does not use — but " + "the lab of an id is its first `-`/`.`-separated token, so a lab the panel DOES pin " + "shipping under a new family prefix (OpenAI's `o` series alongside `gpt-*` is the " + "precedent) lands here rather than in the review-me list above. Scan for a familiar lab " + "wearing an unfamiliar prefix; ignore the rest. Same caveats as above — notes are " + "verbatim, an unmarked id is **not** thereby confirmed ZDR-eligible.", + ] + for group in families: + detail.append("") + detail.append(f"**`{group['lab']}`**") + detail.append("") + detail.append(_candidate_list(group["candidates"])) + detail.append("") + detail.append("
") + out.append("\n".join(detail)) if audit["stale"]: if audit["last_checked"] is None: diff --git a/.github/cursor-review/tests/test_catalog_drift.py b/.github/cursor-review/tests/test_catalog_drift.py index 2b2bc53..dc6e764 100644 --- a/.github/cursor-review/tests/test_catalog_drift.py +++ b/.github/cursor-review/tests/test_catalog_drift.py @@ -139,17 +139,26 @@ def test_quoted_token_fallback_rejects_a_quoted_non_id(self): def test_pin_and_catalog_id_shape_rules_agree(self): # The pin validator and the catalog parser MUST accept the same shapes: # a pin the catalog parser would never emit reads as delisted every run. - # Both currently require a `-`/`.`, so a separator-less id (`o3`) is - # rejected at extraction — a loud checker-defect signal, not a false - # claim about the catalog. If that rule is ever relaxed, relax it in - # both places; this test is the tripwire. - for token in ["o3", "sonnet", "gpt5"]: + # Both go through `_is_model_id`, which requires a `-`/`.` AND a digit — + # so a separator-less id (`o3`) or a digit-less one (`code-supernova`) is + # rejected at extraction: a loud checker-defect signal, not a false claim + # about the catalog. If that rule is ever relaxed, relaxing the one + # function relaxes both sides; this test is the tripwire. + for token in ["o3", "sonnet", "gpt5", "code-supernova", "gpt-based"]: self.assertFalse(cd._is_model_id(token), token) self.assertEqual(cd.catalog_entries(token + "\n"), []) - for token in ["gpt-5.6-sol-max", "kimi-k2.7-code", "gemini-3.1-pro"]: + for token in ["gpt-5.6-sol-max", "kimi-k2.7-code", "gemini-3.1-pro", "o5-pro"]: self.assertTrue(cd._is_model_id(token), token) self.assertEqual([m for m, _ in cd.catalog_entries(token + "\n")], [token]) + def test_a_digit_less_pin_is_rejected_loudly_rather_than_read_as_delisted(self): + # The other half of the tripwire: because `_is_model_id` gates pins too, + # tightening it to require a digit must surface as an ExtractionError + # (red run, actionable message), never as a silent "delisted pin". + text = WORKFLOW.replace('"kimi-k2.7-code"', '"code-supernova"') + with self.assertRaises(cd.ExtractionError): + cd.extract_panel_models(text) + def test_valid_json_with_a_placeholder_entry_raises(self): # Strict JSON is not enough — a placeholder would be "checked" as a pin. text = WORKFLOW.replace( @@ -212,6 +221,15 @@ def test_parses_ids_and_notes(self): self.assertIn("gpt-5.6-sol", entries) self.assertEqual(entries["fable-5-max"], "(NO ZDR)") + def test_a_hyphenated_prose_word_is_not_admitted_as_an_id(self): + # `gpt-based` is lowercase, hyphenated and has letters, so before the + # digit requirement it parsed as a catalog id — noise in the review-me + # list, and a phantom family in the unpinned-families section. + entries = cd.catalog_entries( + "gpt-based models are listed below\ngpt-5.6-sol-max\nself-hosted options: none\n" + ) + self.assertEqual([m for m, _ in entries], ["gpt-5.6-sol-max"]) + def test_a_numbered_list_marker_is_not_mistaken_for_an_id(self): # `1.` satisfies "lowercase token with a separator", so without the # letter requirement a numbered catalog format would parse as the ids @@ -315,11 +333,56 @@ def test_unpinned_same_lab_ids_are_grouped_and_listed(self): self.assertTrue(report["has_findings"]) self.assertFalse(report["urgent"]) - def test_ids_from_unpinned_labs_are_not_listed_as_candidates(self): - # `fable-*` is a lab the panel does not pin — it belongs in the raw - # catalog fold, not in the review-me list. + def test_ids_from_unpinned_labs_are_not_listed_as_same_lab_candidates(self): + # `fable-*` is a lab the panel does not pin — it belongs in the quieter + # unpinned-families catch-all, never in the same-lab review-me list. report = analyze() self.assertNotIn("fable", [g["lab"] for g in report["unpinned"]]) + self.assertEqual( + [g["lab"] for g in report["unpinned_labs"]], ["fable"] + ) + self.assertEqual( + report["unpinned_labs"][0]["candidates"], [{"id": "fable-5-max", "note": "(NO ZDR)"}] + ) + + def test_a_rebranded_family_from_a_pinned_lab_is_caught_by_the_catch_all(self): + # The BE-4852 case: OpenAI ships `o5-pro` alongside `gpt-*`. `lab_of` + # reads its family as `o5` (the first `-`/`.`-separated token), which no + # pin uses, so the same-lab review-me list cannot see it — the catch-all + # is the only thing standing between "a newer model shipped" and silence. + report = analyze(catalog=CATALOG + "o5-pro\no5-pro-thinking\n") + self.assertEqual(cd.lab_of("o5-pro"), "o5") + self.assertNotIn("o5", [g["lab"] for g in report["unpinned"]]) + families = {g["lab"]: [c["id"] for c in g["candidates"]] for g in report["unpinned_labs"]} + self.assertEqual(families["o5"], ["o5-pro", "o5-pro-thinking"]) + self.assertTrue(report["has_findings"]) + + def test_an_unpinned_family_is_a_finding_but_never_urgent(self): + # It is a standing watchlist, not breakage: it must gate the sticky issue + # (or a rebranded family renders into a body nobody sees) but must not + # redden the weekly run. + catalog = "\n".join(PANEL) + "\nfable-5-max\n" + report = analyze(catalog=catalog) + self.assertTrue(report["has_findings"]) + self.assertFalse(report["urgent"]) + self.assertIn("unpinned famil", cd.summary_line(report)) + + def test_a_catalog_of_exactly_the_pins_still_has_no_unpinned_families(self): + # The auto-close path: the catch-all must not make `has_findings` true + # unconditionally. + report = analyze(catalog="\n".join(PANEL) + "\n") + self.assertEqual(report["unpinned_labs"], []) + self.assertFalse(report["has_findings"]) + + def test_a_delisted_pins_lab_still_counts_as_pinned_for_the_catch_all(self): + # A lab whose only pin was just delisted is emphatically still "a lab the + # panel pins" — its successors belong in the same-lab review-me list with + # the delisted-pin context, not demoted into the quiet fold. + catalog = CATALOG.replace("kimi-k2.7-code\n", "kimi-k3-code\n") + report = analyze(catalog=catalog) + self.assertNotIn("kimi", [g["lab"] for g in report["unpinned_labs"]]) + kimi = [g for g in report["unpinned"] if g["lab"] == "kimi"][0] + self.assertEqual([c["id"] for c in kimi["candidates"]], ["kimi-k3-code"]) def test_a_newly_shipped_same_lab_model_shows_up(self): report = analyze(catalog=CATALOG + "claude-opus-5-thinking-max\n") @@ -384,11 +447,23 @@ def test_body_fence_survives_backticks_in_the_catalog(self): self.assertIn("````text", body) def test_oversized_catalog_is_truncated(self): - catalog = CATALOG + ("gpt-filler-x\n" * 6000) + # `gpt-filler-1x` is a parseable same-lab id on purpose (a digit-less + # `gpt-filler-x` is no longer admitted, which would make this exercise + # only the raw-fold clamp and not the report above it). + catalog = CATALOG + ("gpt-filler-1x\n" * 6000) body = cd.render_body(analyze(catalog=catalog), catalog) self.assertIn("truncated", body) self.assertLess(len(body), 65536) + def test_a_catalog_of_many_distinct_unpinned_families_still_fits_the_body_cap(self): + # GitHub rejects an oversized body outright (422), so the section added + # for BE-4852 must not be able to push the report past the clamp. + catalog = CATALOG + "".join(f"lab{n}-9-max\n" for n in range(4000)) + report = analyze(catalog=catalog) + self.assertGreater(len(report["unpinned_labs"]), 100) + body = cd.render_body(report, catalog) + self.assertLess(len(body), 65536) + def test_a_backtick_in_a_catalog_note_cannot_break_out_of_its_code_span(self): # Notes are unconstrained third-party text reproduced in a bot-authored # issue in a PUBLIC repo; a bare single-backtick span would let one inject @@ -406,6 +481,23 @@ def test_a_pinned_no_zdr_marker_is_called_out_in_the_body(self): self.assertIn("`gemini-3.1-pro`", body) self.assertIn("confidentiality regression", body) + def test_body_lists_unpinned_families_in_a_collapsed_section(self): + catalog = CATALOG + "o5-pro\n" + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertIn("unpinned model families", body) + self.assertIn("`o5-pro`", body) + self.assertIn("`o5`", body) + # Collapsed, and below the same-lab review-me list it must not crowd out. + self.assertLess(body.index("review-me list"), body.index("unpinned model families")) + self.assertIn("Catalog ids from unpinned model families", body) + + def test_a_backtick_in_an_unpinned_family_note_cannot_break_out(self): + # Same public-repo injection surface as the same-lab list — these notes + # go through `_inline_code` too, not a bare single-backtick span. + catalog = CATALOG + "fable-6-max `@everyone` see docs\n" + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertIn("`` `@everyone` see docs ``", body) + def test_clean_body_says_no_drift(self): catalog = "\n".join(PANEL) + "\n" body = cd.render_body(analyze(catalog=catalog), catalog) diff --git a/.github/workflows/cursor-review-catalog-drift.yml b/.github/workflows/cursor-review-catalog-drift.yml index 028c905..44d6cc3 100644 --- a/.github/workflows/cursor-review-catalog-drift.yml +++ b/.github/workflows/cursor-review-catalog-drift.yml @@ -21,6 +21,10 @@ name: Cursor Review — catalog drift check # that may retain them; # * unpinned same-lab catalog ids — a REVIEW-ME list, not a recommendation # (newest highest-reasoning ZDR-eligible is a human call); +# * catalog ids from families the panel pins nothing from — quieter still +# (collapsed, never urgent), but it is the only place a lab already on the +# panel can surface after rebranding under a new prefix, e.g. OpenAI's +# `o` series alongside `gpt-*` (BE-4852); # * a `last checked` audit date older than 30 days (or future-dated). # # The `last checked` comment stays the human-audit record — this check does not diff --git a/README.md b/README.md index c8165d8..ea54fea 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. | -| [`cursor-review-catalog-drift.yml`](.github/workflows/cursor-review-catalog-drift.yml) | **Internal to this repo — not `workflow_call`able.** Weekly (Mon 06:17 UTC) + `workflow_dispatch` drift check between the `cursor-review.yml` model pins and Cursor's live `cursor-agent models` catalog. Reads the pins *out of* `cursor-review.yml` (panel heredoc + `judge_model` default) rather than duplicating them, then reports: a **delisted pin** (urgent — consumer PRs are about to fail the review preflight; this also fails the run), a **pin whose catalog line now says NO-ZDR** (also urgent, and the quieter failure — nothing breaks, private review diffs just keep flowing to a model that may retain them), **unpinned same-lab catalog ids** (a review-me list, never an auto-recommendation — "newest highest-reasoning ZDR-eligible" is a human call, and NO-ZDR markers are surfaced verbatim), and a **`last checked` audit date** older than 30 days (or future-dated). Findings land in one sticky issue (`[cursor-review catalog drift]`, label `cursor-review-catalog-drift`, updated in place, closed automatically on a clean run) with the raw catalog folded in. Least privilege by construction: the job that pipes Cursor's installer into bash holds only `contents: read` and hands its rendered report to a separate `issues: write` job via an artifact. Comparison logic + tests live in [`.github/cursor-review/catalog-drift.py`](.github/cursor-review/catalog-drift.py). Requires `CURSOR_API_KEY`. | +| [`cursor-review-catalog-drift.yml`](.github/workflows/cursor-review-catalog-drift.yml) | **Internal to this repo — not `workflow_call`able.** Weekly (Mon 06:17 UTC) + `workflow_dispatch` drift check between the `cursor-review.yml` model pins and Cursor's live `cursor-agent models` catalog. Reads the pins *out of* `cursor-review.yml` (panel heredoc + `judge_model` default) rather than duplicating them, then reports: a **delisted pin** (urgent — consumer PRs are about to fail the review preflight; this also fails the run), a **pin whose catalog line now says NO-ZDR** (also urgent, and the quieter failure — nothing breaks, private review diffs just keep flowing to a model that may retain them), **unpinned same-lab catalog ids** (a review-me list, never an auto-recommendation — "newest highest-reasoning ZDR-eligible" is a human call, and NO-ZDR markers are surfaced verbatim), **catalog ids from families the panel pins nothing from** (a quieter collapsed catch-all, never urgent — a lab is an id's first `-`/`.`-separated token, so a lab the panel *does* pin can only surface there once it rebrands under a new prefix, e.g. OpenAI's `o` series alongside `gpt-*`), and a **`last checked` audit date** older than 30 days (or future-dated). Findings land in one sticky issue (`[cursor-review catalog drift]`, label `cursor-review-catalog-drift`, updated in place, closed automatically on a clean run) with the raw catalog folded in. Least privilege by construction: the job that pipes Cursor's installer into bash holds only `contents: read` and hands its rendered report to a separate `issues: write` job via an artifact. Comparison logic + tests live in [`.github/cursor-review/catalog-drift.py`](.github/cursor-review/catalog-drift.py). Requires `CURSOR_API_KEY`. | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | ## Usage From 89d7bb6beda3d696a8e78a8a78b30e0e1868bb86 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 28 Jul 2026 02:21:38 -0700 Subject: [PATCH 2/3] fix(cursor-review): drop the digit id rule, close the judge-pin shape hole (BE-4852) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `
` 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. --- .github/cursor-review/catalog-drift.py | 112 ++++++++++++------ .../cursor-review/tests/test_catalog_drift.py | 97 ++++++++++++--- 2 files changed, 158 insertions(+), 51 deletions(-) diff --git a/.github/cursor-review/catalog-drift.py b/.github/cursor-review/catalog-drift.py index 6357d45..3d6082b 100644 --- a/.github/cursor-review/catalog-drift.py +++ b/.github/cursor-review/catalog-drift.py @@ -65,6 +65,13 @@ # raw-catalog fold rather than losing the whole body to a 422. MAX_CATALOG_CHARS = 40000 MAX_BODY_CHARS = 60000 +# Per-section budget for the unpinned-families fold, so `MAX_BODY_CHARS` (a blunt +# `body[:N]`) can never cut into its markup. Generous next to a real catalog — +# a few dozen models across a handful of families — while still bounding a +# pathological one. The section is a "scan for a familiar lab wearing an +# unfamiliar prefix" list, so the first rows of each family carry its signal. +MAX_FAMILY_LABS = 40 +MAX_FAMILY_IDS = 25 _HEREDOC_START = re.compile(r"cat\s*>\s*/tmp/models\.json\s*<<\s*'?JSON'?") _HEREDOC_END = re.compile(r"^\s*JSON\s*$") @@ -76,12 +83,6 @@ # carries at least one `-` or `.` — that separator requirement is what keeps # prose lines ("Available models:") out of the parsed id list. _ID_TOKEN = re.compile(r"^[a-z0-9][a-z0-9._-]*$") -# Every model id a lab has shipped to Cursor's catalog carries a version number -# somewhere (`gpt-5.6-sol-max`, `claude-opus-4-8-thinking-max`, `kimi-k2.7-code`, -# `o5-pro`), and a hyphenated PROSE word at the head of a catalog line does not -# ("`gpt-based` models are …"). Requiring a digit is what separates the two — see -# `_is_model_id`. -_HAS_DIGIT = re.compile(r"[0-9]") _BULLET = re.compile(r"^[-*>•\s]+") # The ONE catalog marker worth interpreting rather than merely reproducing: a # PINNED model reclassified non-ZDR means private review diffs are flowing to a @@ -104,7 +105,7 @@ class ExtractionError(Exception): def _is_model_id(token): """True for a bare catalog/model id — see `_ID_TOKEN`. - Three requirements beyond the character class, each closing a different + Two requirements beyond the character class, each closing a different misparse: * a `-`/`.` **separator**, which keeps prose lines ("Available models:") @@ -113,27 +114,34 @@ def _is_model_id(token): `1. gpt-5.6-sol-max` catalog line) from parsing as an id. That matters more than it looks: `present()` trusts this parse, so a catalog of ['1.', '2.'] would look perfectly valid while reporting every real pin as - delisted; - * a **digit**, which keeps a hyphenated prose word at the head of a catalog - line ("`gpt-based` models are …") from being admitted as a bogus id — it - would otherwise add noise to the review-me list and, since BE-4852, mint - a phantom `gpt-based`-style entry in the unpinned-families section. + delisted. Parsing nothing at all instead routes a garbled catalog to `main`'s diagnostic "no ids could be parsed — the format may have changed" hard exit. - All three bind the PIN validator too (`extract_panel_models` / - `extract_judge_model` call this): a shape the catalog parser would never emit + Deliberately NOT also requiring a digit, though every id the panel pins today + happens to carry one. Cursor ships digit-less ids for real (`code-supernova`), + and the rule is not free in that direction: a digit-less id is dropped by + `catalog_entries` BEFORE parsing, so a newly shipped digit-less family never + reaches the BE-4852 unpinned-families catch-all and the run reports clean — + the exact "a new model shipped and nobody noticed" silence this whole check + exists to end. What the digit bought was keeping a hyphenated PROSE word at + the head of a catalog line ("`gpt-based` models are …") out of the id list; + what it cost was dropping real ids. That trade is the wrong way round, and in + the same direction `present` already argues for explicitly: over-report + (one bogus row in a collapsed, never-urgent section) rather than + under-report (a real family silently missing). + + Both rules bind the PIN validator too — `extract_panel_models` and + `extract_judge_model` call this: a shape the catalog parser would never emit must not be accepted as a pin, or that pin reads as delisted every run. If a - lab ever ships a separator-less or digit-less id, relax the rule HERE — one - function, both sides — and `test_pin_and_catalog_id_shape_rules_agree` keeps - them honest. + lab ever ships a separator-less id, relax the rule HERE — one function, both + sides — and `test_pin_and_catalog_id_shape_rules_agree` keeps them honest. """ return ( bool(_ID_TOKEN.match(token)) and re.search(r"[-.]", token) is not None and re.search(r"[a-z]", token) is not None - and _HAS_DIGIT.search(token) is not None ) @@ -193,10 +201,10 @@ def extract_panel_models(workflow_text): if bad: raise ExtractionError( f"the /tmp/models.json heredoc yielded entries that are not bare model ids: {bad!r} " - "(expected a single lowercase token containing a `-` or `.` AND a digit, e.g. " - "`gpt-5.6-sol-max`). If a lab has shipped a separator-less or digit-less id, relax " - "`_is_model_id` — it is the ONE rule the pin validator and the catalog parser share, " - "and they must agree, or the pin will read as delisted every run." + "(expected a single lowercase token containing a `-` or `.`, e.g. " + "`gpt-5.6-sol-max`). If a lab has shipped a separator-less id, relax `_is_model_id` " + "— it is the ONE rule the pin validator and the catalog parser share, and they must " + "agree, or the pin will read as delisted every run." ) return models @@ -222,10 +230,21 @@ def extract_judge_model(workflow_text): # `>-`, which would report as delisted on every single run. value = re.split(r"\s+#", default.group(1).strip(), maxsplit=1)[0].strip() value = value.strip("\"'") - if value and value[0] not in ">|" and not re.search(r"\s", value): + # …and then the SAME shape rule the catalog parser applies. The + # scalar/whitespace checks above are strictly weaker than + # `_is_model_id`, so without this the judge pin was the one hole + # in the "both sides agree" contract: a default the catalog + # parser would never emit (`o3` — no separator) extracted + # cleanly here and then failed `present()`, reporting a phantom + # URGENT "delisted pin" every Monday against a model that is + # sitting right there in the catalog. + if value and value[0] not in ">|" and _is_model_id(value): return value raise ExtractionError( - f"the `judge_model` input's default is not a bare model id: {value!r}" + f"the `judge_model` input's default is not a bare model id: {value!r} " + "(expected a single lowercase token containing a `-` or `.`, e.g. " + "`claude-opus-4-8-thinking-max`). Relax `_is_model_id` if a lab ships a " + "separator-less id — it gates the catalog parser too, and the two must agree." ) break raise ExtractionError("could not find the `judge_model` input's `default:` value") @@ -396,14 +415,17 @@ def roles_of(model_id): # exists to avoid). Most of what lands here is genuinely uninteresting — # labs Comfy will never pin — hence: a finding, but never `urgent`, and # rendered collapsed. + # Grouped through a dict keyed by lab (not a linear scan per entry) so this + # stays O(N) on a catalog whose ids all have distinct prefixes. unpinned_labs = [] + by_lab = {} for model_id, note in entries: lab = lab_of(model_id) if lab in pinned_labs: continue - group = next((g for g in unpinned_labs if g["lab"] == lab), None) + group = by_lab.get(lab) if group is None: - group = {"lab": lab, "candidates": []} + group = by_lab[lab] = {"lab": lab, "candidates": []} unpinned_labs.append(group) group["candidates"].append({"id": model_id, "note": note}) @@ -502,12 +524,20 @@ def _inline_code(text): return f"{delim}{pad}{flat}{pad}{delim}" -def _candidate_list(candidates): - """Bullet list of `{id, note}` rows — shared by both unpinned sections.""" - return "\n".join( - f"- `{c['id']}`" + (f" — {_inline_code(c['note'])}" if c["note"] else "") - for c in candidates - ) +def _candidate_list(candidates, limit=None): + """Bullet list of `{id, note}` rows — shared by both unpinned sections. + + `limit` caps the rows and names the remainder instead of dropping it + silently; the same-lab list above is bounded by the pins so it passes None. + """ + shown = candidates if limit is None else candidates[:limit] + rows = [ + f"- `{c['id']}`" + (f" — {_inline_code(c['note'])}" if c["note"] else "") for c in shown + ] + hidden = len(candidates) - len(shown) + if hidden: + rows.append(f"- _… and {hidden} more — see the raw catalog fold below._") + return "\n".join(rows) def render_body(report, catalog_text, run_url=None, checked_at=None): @@ -586,10 +616,22 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): # it must not crowd out the same-lab review-me list above. It exists for # the one row that matters — a lab already on the panel shipping under a # new family prefix, which `lab_of` cannot tell from a new vendor. + # Budgeted HERE rather than left to `MAX_BODY_CHARS` below. That clamp is + # a blunt `body[:N]`: on a catalog with thousands of distinct prefixes it + # would cut INTO this block, leaving the `
`/inline-code markup + # unterminated (which swallows the rest of the issue in GitHub's + # renderer) and dropping the stale-audit and raw-catalog sections that + # follow. Truncating with an explicit count keeps the body well-formed + # and says out loud what was dropped. + shown = families[:MAX_FAMILY_LABS] + hidden = len(families) - len(shown) + labs_listed = ", ".join("`" + g["lab"] + "`" for g in shown) + if hidden: + labs_listed += f", +{hidden} more" detail = [ "
", f"Catalog ids from unpinned model families " - f"({', '.join('`' + g['lab'] + '`' for g in families)})", + f"({labs_listed})
", "", "Families the panel pins **nothing** from. Usually just labs Comfy does not use — but " "the lab of an id is its first `-`/`.`-separated token, so a lab the panel DOES pin " @@ -598,11 +640,11 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): "wearing an unfamiliar prefix; ignore the rest. Same caveats as above — notes are " "verbatim, an unmarked id is **not** thereby confirmed ZDR-eligible.", ] - for group in families: + for group in shown: detail.append("") detail.append(f"**`{group['lab']}`**") detail.append("") - detail.append(_candidate_list(group["candidates"])) + detail.append(_candidate_list(group["candidates"], limit=MAX_FAMILY_IDS)) detail.append("") detail.append("") out.append("\n".join(detail)) diff --git a/.github/cursor-review/tests/test_catalog_drift.py b/.github/cursor-review/tests/test_catalog_drift.py index dc6e764..92876a7 100644 --- a/.github/cursor-review/tests/test_catalog_drift.py +++ b/.github/cursor-review/tests/test_catalog_drift.py @@ -139,25 +139,48 @@ def test_quoted_token_fallback_rejects_a_quoted_non_id(self): def test_pin_and_catalog_id_shape_rules_agree(self): # The pin validator and the catalog parser MUST accept the same shapes: # a pin the catalog parser would never emit reads as delisted every run. - # Both go through `_is_model_id`, which requires a `-`/`.` AND a digit — - # so a separator-less id (`o3`) or a digit-less one (`code-supernova`) is - # rejected at extraction: a loud checker-defect signal, not a false claim - # about the catalog. If that rule is ever relaxed, relaxing the one - # function relaxes both sides; this test is the tripwire. - for token in ["o3", "sonnet", "gpt5", "code-supernova", "gpt-based"]: + # All three paths go through `_is_model_id`, which requires a `-`/`.` and + # a letter — so a separator-less id (`o3`) is rejected at extraction: a + # loud checker-defect signal, not a false claim about the catalog. If + # that rule is ever relaxed, relaxing the one function relaxes every + # side; this test is the tripwire. + for token in ["o3", "sonnet", "gpt5"]: self.assertFalse(cd._is_model_id(token), token) self.assertEqual(cd.catalog_entries(token + "\n"), []) for token in ["gpt-5.6-sol-max", "kimi-k2.7-code", "gemini-3.1-pro", "o5-pro"]: self.assertTrue(cd._is_model_id(token), token) self.assertEqual([m for m, _ in cd.catalog_entries(token + "\n")], [token]) - def test_a_digit_less_pin_is_rejected_loudly_rather_than_read_as_delisted(self): - # The other half of the tripwire: because `_is_model_id` gates pins too, - # tightening it to require a digit must surface as an ExtractionError - # (red run, actionable message), never as a silent "delisted pin". + def test_a_digit_less_id_is_accepted_on_both_sides(self): + # Cursor ships digit-less ids for real (`code-supernova`). Requiring a + # digit dropped them in `catalog_entries` BEFORE parsing, so a newly + # shipped digit-less family never reached the BE-4852 catch-all and the + # run reported clean — the silence this check exists to end — and a + # digit-less PIN hard-failed extraction every run. + self.assertTrue(cd._is_model_id("code-supernova")) + self.assertEqual( + [m for m, _ in cd.catalog_entries("code-supernova\n")], ["code-supernova"] + ) text = WORKFLOW.replace('"kimi-k2.7-code"', '"code-supernova"') + self.assertIn("code-supernova", cd.extract_panel_models(text)) + + def test_a_digit_less_family_reaches_the_unpinned_families_catch_all(self): + # The end-to-end of the above: a digit-less family the panel pins nothing + # from must surface as a finding, not vanish. + report = analyze(catalog=CATALOG + "code-supernova\n") + families = {g["lab"]: [c["id"] for c in g["candidates"]] for g in report["unpinned_labs"]} + self.assertEqual(families["code"], ["code-supernova"]) + self.assertTrue(report["has_findings"]) + + def test_a_judge_default_the_catalog_parser_would_reject_raises(self): + # The judge pin was the one hole in the "both sides agree" contract: + # `extract_judge_model` checked only for whitespace/scalar markers, so a + # separator-less default (`o3`) extracted cleanly and then failed + # `present()` — a phantom URGENT "delisted pin" every Monday against a + # model sitting right there in the catalog. + text = WORKFLOW.replace("default: claude-opus-4-8-thinking-max", "default: o3") with self.assertRaises(cd.ExtractionError): - cd.extract_panel_models(text) + cd.extract_judge_model(text) def test_valid_json_with_a_placeholder_entry_raises(self): # Strict JSON is not enough — a placeholder would be "checked" as a pin. @@ -221,14 +244,32 @@ def test_parses_ids_and_notes(self): self.assertIn("gpt-5.6-sol", entries) self.assertEqual(entries["fable-5-max"], "(NO ZDR)") - def test_a_hyphenated_prose_word_is_not_admitted_as_an_id(self): - # `gpt-based` is lowercase, hyphenated and has letters, so before the - # digit requirement it parsed as a catalog id — noise in the review-me - # list, and a phantom family in the unpinned-families section. + def test_a_hyphenated_prose_word_is_admitted_and_that_is_the_accepted_trade(self): + # `gpt-based` is lowercase, hyphenated and has letters, so it parses as + # an id: one bogus row in a never-urgent list. Screening it out by + # requiring a digit was tried and reverted — it also dropped real + # digit-less ids (`code-supernova`) before parsing, which is the + # under-report direction this checker refuses (see `_is_model_id` and + # `present`). Over-reporting a prose word is the cheaper failure, and + # this test pins that choice so it is not silently re-tightened. entries = cd.catalog_entries( "gpt-based models are listed below\ngpt-5.6-sol-max\nself-hosted options: none\n" ) - self.assertEqual([m for m, _ in entries], ["gpt-5.6-sol-max"]) + self.assertEqual( + [m for m, _ in entries], ["gpt-based", "gpt-5.6-sol-max", "self-hosted"] + ) + # What actually matters is that the noise is inert. `gpt-based` reads as + # lab `gpt`, so it shows up as one extra row in the same-lab review-me + # list — a list that is explicitly "review me, not a recommendation" and + # never `urgent`. It cannot redden a run, and it cannot mask a real pin, + # because `delisted` is computed from the pins, not from this list. + report = analyze(catalog="\n".join(PANEL) + f"\n{JUDGE}\ngpt-based models are listed\n") + self.assertFalse(report["urgent"]) + self.assertEqual(report["delisted"], []) + self.assertEqual( + [(g["lab"], [c["id"] for c in g["candidates"]]) for g in report["unpinned"]], + [("gpt", ["gpt-based"])], + ) def test_a_numbered_list_marker_is_not_mistaken_for_an_id(self): # `1.` satisfies "lowercase token with a separator", so without the @@ -463,6 +504,30 @@ def test_a_catalog_of_many_distinct_unpinned_families_still_fits_the_body_cap(se self.assertGreater(len(report["unpinned_labs"]), 100) body = cd.render_body(report, catalog) self.assertLess(len(body), 65536) + # Length alone is not the property that matters — the section must be + # budgeted rather than sliced by the blunt `MAX_BODY_CHARS` clamp, which + # would leave the markup unterminated (swallowing the rest of the issue + # in GitHub's renderer) and drop the sections below it. + self.assertEqual(body.count("
"), body.count("
")) + self.assertIn("more", body) + # The sections that follow the fold survive. + self.assertIn("Raw cursor-agent models output", body) + self.assertIn("This issue is sticky", body) + + def test_the_unpinned_families_fold_names_what_it_truncated(self): + # Silent truncation would read as "these are all the families" — the one + # reading that could hide the rebranded family this section exists for. + catalog = "\n".join(PANEL) + "".join(f"\nlab{n}-9-max" for n in range(cd.MAX_FAMILY_LABS + 5)) + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertIn("+5 more", body) + self.assertEqual(body.count("
"), body.count("
")) + + def test_a_single_family_with_many_ids_is_capped_and_says_so(self): + ids = "".join(f"\nsolo-{n}-max" for n in range(cd.MAX_FAMILY_IDS + 3)) + catalog = "\n".join(PANEL) + ids + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertIn("… and 3 more", body) + self.assertEqual(body.count("
"), body.count("
")) def test_a_backtick_in_a_catalog_note_cannot_break_out_of_its_code_span(self): # Notes are unconstrained third-party text reproduced in a bot-authored From 03dbf7b8809fa755a521a2c8bf26c1f347f0df1a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 28 Jul 2026 11:28:53 -0700 Subject: [PATCH 3/3] fix(cursor-review): budget every drift-report list, admit bare o-series ids (BE-4852) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/cursor-review/catalog-drift.py | 193 +++++++++++------- .../cursor-review/tests/test_catalog_drift.py | 114 +++++++++-- 2 files changed, 224 insertions(+), 83 deletions(-) diff --git a/.github/cursor-review/catalog-drift.py b/.github/cursor-review/catalog-drift.py index 3d6082b..15a900b 100644 --- a/.github/cursor-review/catalog-drift.py +++ b/.github/cursor-review/catalog-drift.py @@ -61,17 +61,29 @@ STICKY_TITLE_PREFIX = "[cursor-review catalog drift]" DEFAULT_STALE_DAYS = 30 -# GitHub caps an issue body at 65536 chars; leave room for the report above the -# raw-catalog fold rather than losing the whole body to a 422. +# GitHub caps an issue body at 65536 chars. MAX_CATALOG_CHARS is only the raw +# fold's CEILING — `render_body` grants the fold whatever MAX_BODY_CHARS the +# report sections have not used, so the two budgets cannot overlap into a 422. MAX_CATALOG_CHARS = 40000 MAX_BODY_CHARS = 60000 -# Per-section budget for the unpinned-families fold, so `MAX_BODY_CHARS` (a blunt -# `body[:N]`) can never cut into its markup. Generous next to a real catalog — -# a few dozen models across a handful of families — while still bounding a -# pathological one. The section is a "scan for a familiar lab wearing an -# unfamiliar prefix" list, so the first rows of each family carry its signal. +# Budgets for the report's candidate lists, so `MAX_BODY_CHARS` (a blunt +# `body[:N]`) can never cut into their markup. MAX_FAMILY_IDS caps EVERY +# per-lab id list (the same-lab review-me list, a delisted pin's alternatives, +# the families fold); MAX_FAMILY_LABS plus the CHARS budget bound the families +# fold, the one list whose group count the catalog controls — row caps alone +# bound its rows, not its chars (40 labs × 25 capped-note rows is still ~4× the +# body cap). All generous next to a real catalog — a few dozen models across a +# handful of families — and every list leads with its highest-signal rows, so +# the first rows carry it. MAX_FAMILY_LABS = 40 MAX_FAMILY_IDS = 25 +MAX_FAMILY_FOLD_CHARS = 15000 +# Cap on a rendered note. Notes are third-party free text repeated across +# dozens of rows; unbounded, a single note could eat the whole body budget. +MAX_NOTE_CHARS = 200 +# Scaffolding reserve when computing the raw fold's remaining budget: the +#
wrapper, the code-fence lines, and a truncation notice. +_FOLD_OVERHEAD = 400 _HEREDOC_START = re.compile(r"cat\s*>\s*/tmp/models\.json\s*<<\s*'?JSON'?") _HEREDOC_END = re.compile(r"^\s*JSON\s*$") @@ -80,8 +92,9 @@ _DEFAULT_KEY = re.compile(r"^\s*default\s*:\s*(.+?)\s*$") _LAST_CHECKED = re.compile(r"last checked\s+(\d{4}-\d{2}-\d{2})", re.IGNORECASE) # A catalog id is lowercase alphanumeric with `.`/`-`/`_` separators, and always -# carries at least one `-` or `.` — that separator requirement is what keeps -# prose lines ("Available models:") out of the parsed id list. +# carries a `-`/`.` or a digit — that requirement is what keeps prose lines +# ("models available:") out of the parsed id list while still admitting a bare +# id like `o3`. _ID_TOKEN = re.compile(r"^[a-z0-9][a-z0-9._-]*$") _BULLET = re.compile(r"^[-*>•\s]+") # The ONE catalog marker worth interpreting rather than merely reproducing: a @@ -108,39 +121,43 @@ def _is_model_id(token): Two requirements beyond the character class, each closing a different misparse: - * a `-`/`.` **separator**, which keeps prose lines ("Available models:") - out of the parsed id list; + * a `-`/`.` separator **or a digit**, which keeps bare prose words + ("models", "available") out of the parsed id list; * a **letter**, which keeps a numbered-list marker (`1.` in a hypothetical - `1. gpt-5.6-sol-max` catalog line) from parsing as an id. That matters - more than it looks: `present()` trusts this parse, so a catalog of - ['1.', '2.'] would look perfectly valid while reporting every real pin as - delisted. + `1. gpt-5.6-sol-max` catalog line — or a bare year in prose) from + parsing as an id. That matters more than it looks: `present()` trusts + this parse, so a catalog of ['1.', '2.'] would look perfectly valid + while reporting every real pin as delisted. Parsing nothing at all instead routes a garbled catalog to `main`'s diagnostic "no ids could be parsed — the format may have changed" hard exit. - Deliberately NOT also requiring a digit, though every id the panel pins today - happens to carry one. Cursor ships digit-less ids for real (`code-supernova`), - and the rule is not free in that direction: a digit-less id is dropped by - `catalog_entries` BEFORE parsing, so a newly shipped digit-less family never - reaches the BE-4852 unpinned-families catch-all and the run reports clean — - the exact "a new model shipped and nobody noticed" silence this whole check - exists to end. What the digit bought was keeping a hyphenated PROSE word at - the head of a catalog line ("`gpt-based` models are …") out of the id list; - what it cost was dropping real ids. That trade is the wrong way round, and in - the same direction `present` already argues for explicitly: over-report - (one bogus row in a collapsed, never-urgent section) rather than - under-report (a real family silently missing). + Deliberately NOT requiring a digit (Cursor ships digit-less ids for real — + `code-supernova`) and NOT requiring a separator either (OpenAI ships bare + o-series ids — `o3`). Each rule was tried and dropped for the same reason: + an id it rejects is dropped by `catalog_entries` BEFORE parsing, so a newly + shipped family of that shape never reaches the BE-4852 unpinned-families + catch-all and the run reports clean — the exact "a new model shipped and + nobody noticed" silence this whole check exists to end, and for the + separator rule that silence hit exactly the bare o-series rebrand the + catch-all was built for. What each rule bought was keeping some prose at + the head of a catalog line out of the id list ("`gpt-based` models …", or + "`v2` models …" for the separator); what it cost was dropping real ids. + That trade is the wrong way round, and in the same direction `present` + already argues for explicitly: over-report (one bogus row in a collapsed, + never-urgent section) rather than under-report (a real family silently + missing). Both rules bind the PIN validator too — `extract_panel_models` and `extract_judge_model` call this: a shape the catalog parser would never emit must not be accepted as a pin, or that pin reads as delisted every run. If a - lab ever ships a separator-less id, relax the rule HERE — one function, both - sides — and `test_pin_and_catalog_id_shape_rules_agree` keeps them honest. + lab ever ships an id this rule rejects (a bare digit-less word), relax it + HERE — one function, both sides — and + `test_pin_and_catalog_id_shape_rules_agree` keeps them honest. """ return ( bool(_ID_TOKEN.match(token)) - and re.search(r"[-.]", token) is not None + and re.search(r"[-.0-9]", token) is not None and re.search(r"[a-z]", token) is not None ) @@ -201,10 +218,10 @@ def extract_panel_models(workflow_text): if bad: raise ExtractionError( f"the /tmp/models.json heredoc yielded entries that are not bare model ids: {bad!r} " - "(expected a single lowercase token containing a `-` or `.`, e.g. " - "`gpt-5.6-sol-max`). If a lab has shipped a separator-less id, relax `_is_model_id` " - "— it is the ONE rule the pin validator and the catalog parser share, and they must " - "agree, or the pin will read as delisted every run." + "(expected a single lowercase token carrying a `-`, `.`, or digit, e.g. " + "`gpt-5.6-sol-max` or `o3`). If a lab has shipped an id of a new shape, relax " + "`_is_model_id` — it is the ONE rule the pin validator and the catalog parser share, " + "and they must agree, or the pin will read as delisted every run." ) return models @@ -234,17 +251,17 @@ def extract_judge_model(workflow_text): # scalar/whitespace checks above are strictly weaker than # `_is_model_id`, so without this the judge pin was the one hole # in the "both sides agree" contract: a default the catalog - # parser would never emit (`o3` — no separator) extracted - # cleanly here and then failed `present()`, reporting a phantom - # URGENT "delisted pin" every Monday against a model that is - # sitting right there in the catalog. + # parser would never emit (`sonnet` — a bare digit-less word) + # extracted cleanly here and then failed `present()`, reporting + # a phantom URGENT "delisted pin" every Monday against a model + # that is sitting right there in the catalog. if value and value[0] not in ">|" and _is_model_id(value): return value raise ExtractionError( f"the `judge_model` input's default is not a bare model id: {value!r} " - "(expected a single lowercase token containing a `-` or `.`, e.g. " - "`claude-opus-4-8-thinking-max`). Relax `_is_model_id` if a lab ships a " - "separator-less id — it gates the catalog parser too, and the two must agree." + "(expected a single lowercase token carrying a `-`, `.`, or digit, e.g. " + "`claude-opus-4-8-thinking-max`). Relax `_is_model_id` if a lab ships an id " + "of a new shape — it gates the catalog parser too, and the two must agree." ) break raise ExtractionError("could not find the `judge_model` input's `default:` value") @@ -515,9 +532,14 @@ def _inline_code(text): a bot-authored issue in a PUBLIC repo. A bare single-backtick span would let a note carrying a backtick break out and inject markdown — or an `@mention` that notifies real people — so the delimiter is sized to the note (per - CommonMark) and padded when the note starts or ends with a backtick. + CommonMark) and padded when the note starts or ends with a backtick. The + note is also capped at MAX_NOTE_CHARS — it is repeated across dozens of + rows, and unbounded it could eat the whole body budget by itself — with the + cap applied BEFORE the delimiter is sized, so it sizes what is emitted. """ flat = re.sub(r"\s+", " ", text).strip() + if len(flat) > MAX_NOTE_CHARS: + flat = flat[:MAX_NOTE_CHARS].rstrip("`") + " …" longest = max((len(m) for m in re.findall(r"`+", flat)), default=0) delim = "`" * (longest + 1) pad = " " if flat.startswith("`") or flat.endswith("`") else "" @@ -528,7 +550,8 @@ def _candidate_list(candidates, limit=None): """Bullet list of `{id, note}` rows — shared by both unpinned sections. `limit` caps the rows and names the remainder instead of dropping it - silently; the same-lab list above is bounded by the pins so it passes None. + silently. Every caller passes one: the number of GROUPS in the same-lab + list is bounded by the pins, but the ids per group come from the catalog. """ shown = candidates if limit is None else candidates[:limit] rows = [ @@ -577,11 +600,13 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): ) for item in report["delisted"]: roles = "/".join(item["roles"]) or "pin" - same_lab = ( - ", ".join(f"`{m}`" for m in item["same_lab_available"]) - if item["same_lab_available"] - else "_(no same-lab id in the catalog)_" - ) + alts = item["same_lab_available"] + if alts: + same_lab = ", ".join(f"`{m}`" for m in alts[:MAX_FAMILY_IDS]) + if len(alts) > MAX_FAMILY_IDS: + same_lab += f", +{len(alts) - MAX_FAMILY_IDS} more" + else: + same_lab = "_(no same-lab id in the catalog)_" out.append(f"- `{item['id']}` ({roles}) — available for lab `{item['lab']}`: {same_lab}") if report.get("zdr_risk"): @@ -608,7 +633,7 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): for group in report["unpinned"]: pinned_now = ", ".join(f"`{m}`" for m in group["pinned"]) or "_none_" out.append(f"**`{group['lab']}`** (pinned: {pinned_now})") - out.append(_candidate_list(group["candidates"])) + out.append(_candidate_list(group["candidates"], limit=MAX_FAMILY_IDS)) families = report.get("unpinned_labs") or [] if families: @@ -622,10 +647,27 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): # unterminated (which swallows the rest of the issue in GitHub's # renderer) and dropping the stale-audit and raw-catalog sections that # follow. Truncating with an explicit count keeps the body well-formed - # and says out loud what was dropped. - shown = families[:MAX_FAMILY_LABS] + # and says out loud what was dropped. Budgeted in CHARS as well as rows: + # the row caps bound how many rows render, but 40 labs × 25 capped-note + # rows still overruns the whole body budget, so groups stop when the + # fold's char budget is spent (the first group always renders). + shown = [] + used = 0 + for group in families: + if len(shown) >= MAX_FAMILY_LABS: + break + block = "\n**`{lab}`**\n\n{rows}".format( + lab=group["lab"], + rows=_candidate_list(group["candidates"], limit=MAX_FAMILY_IDS), + ) + if shown and used + len(block) > MAX_FAMILY_FOLD_CHARS: + break + shown.append(block) + used += len(block) + if used > MAX_FAMILY_FOLD_CHARS: + break hidden = len(families) - len(shown) - labs_listed = ", ".join("`" + g["lab"] + "`" for g in shown) + labs_listed = ", ".join("`" + g["lab"] + "`" for g in families[: len(shown)]) if hidden: labs_listed += f", +{hidden} more" detail = [ @@ -640,11 +682,7 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): "wearing an unfamiliar prefix; ignore the rest. Same caveats as above — notes are " "verbatim, an unmarked id is **not** thereby confirmed ZDR-eligible.", ] - for group in shown: - detail.append("") - detail.append(f"**`{group['lab']}`**") - detail.append("") - detail.append(_candidate_list(group["candidates"], limit=MAX_FAMILY_IDS)) + detail.extend(shown) detail.append("") detail.append("
") out.append("\n".join(detail)) @@ -674,23 +712,40 @@ def render_body(report, catalog_text, run_url=None, checked_at=None): f"pins and refresh the `last checked` comment in `cursor-review.yml`." ) - catalog = catalog_text.rstrip("\n") - if len(catalog) > MAX_CATALOG_CHARS: - catalog = catalog[:MAX_CATALOG_CHARS] + "\n… truncated — see the workflow run log for the full catalog." - out.append( - "
\nRaw cursor-agent models output\n\n" - + _fenced(catalog) - + "\n
" - ) - out.append( + footer = ( "_Filed by the weekly `cursor-review-catalog-drift` check. This issue is sticky — it is " "updated in place each run and closed automatically once a run finds no drift._" ) + # The raw fold gets whatever body budget the report has NOT used, up to the + # MAX_CATALOG_CHARS ceiling. A fixed 40K assumed the report stayed inside + # ~20K; a large catalog can exceed that even with every list capped, and the + # blunt `body[:N]` clamp below would then slice mid-fold — unterminated + # markup that swallows the footer in GitHub's renderer. + catalog = catalog_text.rstrip("\n") + remaining = MAX_BODY_CHARS - sum(len(section) + 2 for section in out) - len(footer) + budget = min(MAX_CATALOG_CHARS, remaining - _FOLD_OVERHEAD) + if budget < 500: + # Not enough room left for a useful excerpt — say so instead of folding + # a fragment (or overshooting into the clamp). + out.append( + "_Raw `cursor-agent models` output omitted — the report above used the body " + "budget; see the workflow run log for the full catalog._" + ) + else: + if len(catalog) > budget: + catalog = catalog[:budget] + "\n… truncated — see the workflow run log for the full catalog." + out.append( + "
\nRaw cursor-agent models output\n\n" + + _fenced(catalog) + + "\n
" + ) + out.append(footer) body = "\n\n".join(out) + "\n" if len(body) > MAX_BODY_CHARS: - # Last-resort clamp: GitHub rejects an oversized body outright (422), and - # a failed issue write would lose the whole report. A truncated report - # still names the delisted pins, which lead the body. + # Last-resort clamp — every section above is bounded, so reaching this + # takes pathological ids, but GitHub rejects an oversized body outright + # (422) and a failed issue write would lose the whole report. A + # truncated report still names the delisted pins, which lead the body. body = body[:MAX_BODY_CHARS] + "\n\n_… report truncated — see the workflow run log._\n" return body diff --git a/.github/cursor-review/tests/test_catalog_drift.py b/.github/cursor-review/tests/test_catalog_drift.py index 92876a7..69aa876 100644 --- a/.github/cursor-review/tests/test_catalog_drift.py +++ b/.github/cursor-review/tests/test_catalog_drift.py @@ -139,15 +139,15 @@ def test_quoted_token_fallback_rejects_a_quoted_non_id(self): def test_pin_and_catalog_id_shape_rules_agree(self): # The pin validator and the catalog parser MUST accept the same shapes: # a pin the catalog parser would never emit reads as delisted every run. - # All three paths go through `_is_model_id`, which requires a `-`/`.` and - # a letter — so a separator-less id (`o3`) is rejected at extraction: a - # loud checker-defect signal, not a false claim about the catalog. If - # that rule is ever relaxed, relaxing the one function relaxes every - # side; this test is the tripwire. - for token in ["o3", "sonnet", "gpt5"]: + # All three paths go through `_is_model_id`, which requires a letter + # plus a `-`/`.` or a digit — so a bare prose word (`sonnet`) is + # rejected at extraction: a loud checker-defect signal, not a false + # claim about the catalog. If that rule is ever relaxed, relaxing the + # one function relaxes every side; this test is the tripwire. + for token in ["sonnet", "models", "available"]: self.assertFalse(cd._is_model_id(token), token) self.assertEqual(cd.catalog_entries(token + "\n"), []) - for token in ["gpt-5.6-sol-max", "kimi-k2.7-code", "gemini-3.1-pro", "o5-pro"]: + for token in ["gpt-5.6-sol-max", "kimi-k2.7-code", "gemini-3.1-pro", "o5-pro", "o3", "gpt5"]: self.assertTrue(cd._is_model_id(token), token) self.assertEqual([m for m, _ in cd.catalog_entries(token + "\n")], [token]) @@ -172,13 +172,28 @@ def test_a_digit_less_family_reaches_the_unpinned_families_catch_all(self): self.assertEqual(families["code"], ["code-supernova"]) self.assertTrue(report["has_findings"]) + def test_a_bare_separator_less_id_is_accepted_on_both_sides(self): + # OpenAI ships bare o-series ids for real (`o3`). Requiring a `-`/`.` + # dropped them in `catalog_entries` BEFORE parsing — the same pre-parse + # silence that got the digit rule reverted, and it hit exactly the bare + # o-series rebrand the BE-4852 catch-all was built for: the id never + # reached it and the run reported clean. + report = analyze(catalog=CATALOG + "o3\n") + families = {g["lab"]: [c["id"] for c in g["candidates"]] for g in report["unpinned_labs"]} + self.assertEqual(families["o3"], ["o3"]) + self.assertTrue(report["has_findings"]) + # …and the pin side of the shared rule accepts it too. + text = WORKFLOW.replace("default: claude-opus-4-8-thinking-max", "default: o3") + self.assertEqual(cd.extract_judge_model(text), "o3") + def test_a_judge_default_the_catalog_parser_would_reject_raises(self): # The judge pin was the one hole in the "both sides agree" contract: # `extract_judge_model` checked only for whitespace/scalar markers, so a - # separator-less default (`o3`) extracted cleanly and then failed - # `present()` — a phantom URGENT "delisted pin" every Monday against a - # model sitting right there in the catalog. - text = WORKFLOW.replace("default: claude-opus-4-8-thinking-max", "default: o3") + # default the catalog parser drops (`sonnet` — a bare digit-less word) + # extracted cleanly and then failed `present()` — a phantom URGENT + # "delisted pin" every Monday against a model sitting right there in + # the catalog. + text = WORKFLOW.replace("default: claude-opus-4-8-thinking-max", "default: sonnet") with self.assertRaises(cd.ExtractionError): cd.extract_judge_model(text) @@ -258,11 +273,12 @@ def test_a_hyphenated_prose_word_is_admitted_and_that_is_the_accepted_trade(self self.assertEqual( [m for m, _ in entries], ["gpt-based", "gpt-5.6-sol-max", "self-hosted"] ) - # What actually matters is that the noise is inert. `gpt-based` reads as + # What usually matters is that the noise is inert. `gpt-based` reads as # lab `gpt`, so it shows up as one extra row in the same-lab review-me # list — a list that is explicitly "review me, not a recommendation" and - # never `urgent`. It cannot redden a run, and it cannot mask a real pin, - # because `delisted` is computed from the pins, not from this list. + # never `urgent` — and a token that matches no pin cannot redden a run + # or mask one. The exception — a prose token that exactly EQUALS a pin — + # is pinned separately below. report = analyze(catalog="\n".join(PANEL) + f"\n{JUDGE}\ngpt-based models are listed\n") self.assertFalse(report["urgent"]) self.assertEqual(report["delisted"], []) @@ -271,6 +287,25 @@ def test_a_hyphenated_prose_word_is_admitted_and_that_is_the_accepted_trade(self [("gpt", ["gpt-based"])], ) + def test_a_prose_line_leading_with_a_pinned_id_reads_as_that_pin(self): + # The known residual of admitting prose tokens: a catalog line whose + # FIRST token exactly equals a pinned id ("kimi-k2.7-code was removed…") + # is shape-indistinguishable from a listing of that id with a note, so + # `present()` counts the pin as listed and the urgent delisted finding + # is suppressed. Telling the two apart means interpreting the note text, + # which this checker refuses by design for everything except NO-ZDR (see + # `catalog_entries`) — a "removed"/"deprecated" word denylist would be + # guessing at phrasing Cursor has never committed to, with a false match + # crying delisted-wolf about a live pin. Pinned here so the trade stays + # explicit rather than accidental: if Cursor ever ships prose like this, + # the fix is note interpretation, not tighter token screening. + catalog = CATALOG.replace( + "kimi-k2.7-code\n", "kimi-k2.7-code was removed from the catalog\n" + ) + report = analyze(catalog=catalog) + self.assertEqual(report["delisted"], []) + self.assertFalse(report["urgent"]) + def test_a_numbered_list_marker_is_not_mistaken_for_an_id(self): # `1.` satisfies "lowercase token with a separator", so without the # letter requirement a numbered catalog format would parse as the ids @@ -529,6 +564,57 @@ def test_a_single_family_with_many_ids_is_capped_and_says_so(self): self.assertIn("… and 3 more", body) self.assertEqual(body.count("
"), body.count("
")) + def test_worst_case_lists_and_notes_never_reach_the_blunt_clamp(self): + # The row caps alone bound COUNTS, not chars: max-rows-everywhere with + # long notes used to blow through MAX_BODY_CHARS and land in the blunt + # `body[:N]` clamp, which slices mid-markup — exactly the corruption the + # per-section budgets exist to prevent. The one-id-per-family test above + # never reaches that worst case, so this one does: long notes, hundreds + # of same-lab ids, and dozens of families × dozens of ids at once. The + # body must come in UNDER the cap via budgeting (notes capped, lists + # capped, families fold char-budgeted, raw fold given only the leftover + # budget) — not via the clamp. + note = "context " * 60 # ~480 chars, > MAX_NOTE_CHARS + catalog = ( + CATALOG + + "".join(f"gpt-tier-{n} {note}\n" for n in range(300)) + + "".join(f"newlab{n}-tier-{m} {note}\n" for n in range(60) for m in range(30)) + ) + report = analyze(catalog=catalog) + body = cd.render_body(report, catalog) + self.assertLessEqual(len(body), cd.MAX_BODY_CHARS) + self.assertNotIn("report truncated", body) + self.assertEqual(body.count("
"), body.count("
")) + # The sections after the big lists survive, well-formed. + self.assertIn("This issue is sticky", body) + self.assertIn("cursor-agent models", body) + + def test_the_raw_fold_shrinks_to_the_budget_the_report_left_over(self): + # A 40K report + a 40K raw fold is 80K — over GitHub's 65536 limit even + # though each half respects its own constant. The fold must take only + # what MAX_BODY_CHARS has left, and the whole body must stay well-formed. + catalog = CATALOG + "".join(f"lab{n}-9-max\n" for n in range(4000)) + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertLessEqual(len(body), cd.MAX_BODY_CHARS) + self.assertNotIn("report truncated", body) + self.assertIn("truncated — see the workflow run log", body) + + def test_a_delisted_pins_alternatives_list_is_capped(self): + extra = cd.MAX_FAMILY_IDS + 8 + catalog = CATALOG.replace("kimi-k2.7-code\n", "") + "".join( + f"kimi-k{n}-code\n" for n in range(extra) + ) + body = cd.render_body(analyze(catalog=catalog), catalog) + self.assertIn("Delisted pin", body) + self.assertIn("+8 more", body) + + def test_an_oversized_note_is_capped_in_its_row(self): + catalog = CATALOG + "gpt-5.7-preview " + ("x" * 1000) + "\n" + body = cd.render_body(analyze(catalog=catalog), catalog) + row = [ln for ln in body.splitlines() if ln.startswith("- `gpt-5.7-preview`")][0] + self.assertLess(len(row), cd.MAX_NOTE_CHARS + 100) + self.assertIn("…", row) + def test_a_backtick_in_a_catalog_note_cannot_break_out_of_its_code_span(self): # Notes are unconstrained third-party text reproduced in a bot-authored # issue in a PUBLIC repo; a bare single-backtick span would let one inject