feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819) - #87
feat(cursor-review): weekly catalog-drift check for the panel pins (BE-4819)#87mattmillerai wants to merge 3 commits into
Conversation
…E-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.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesCursor catalog drift
Sequence Diagram(s)sequenceDiagram
participant Schedule as GitHub Actions schedule
participant Catalog as cursor-agent models
participant Checker as catalog-drift.py
participant Issue as Sticky GitHub issue
Schedule->>Catalog: Fetch live model catalog
Catalog-->>Checker: Provide catalog output
Checker->>Checker: Extract pins and analyze drift
Checker->>Issue: Create or update findings
Checker->>Issue: Close issue when clean
Checker-->>Schedule: Report urgent status
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
mattmillerai
added
cursor-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/cursor-review/catalog-drift.py:
- Around line 121-148: Update the validation condition in extract_judge_model to
use the idiomatic not-in membership form instead of negating an in expression,
while preserving the existing bare model ID validation behavior.
In @.github/workflows/cursor-review-catalog-drift.yml:
- Around line 120-122: Update the sticky-issue lookup around the numbers and
existing variables to retrieve all open issues through GitHub API pagination
instead of relying on gh issue list --limit 500. Preserve local filtering for
titles beginning with PREFIX, then select the first matching issue number so the
existing duplicate-prevention flow remains unchanged.
- Around line 36-38: Separate the untrusted Cursor CLI installation and review
work from the job that performs issue updates. Pin or verify the CLI artifact
before execution, prevent the installer from influencing later GitHub CLI calls,
and grant issues: write only to the dedicated update job. Replace the gh issue
list --limit 500 lookup with complete pagination or a deterministic exact-title
query so existing sticky issues are always found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc21e0c7-59a2-41af-b8cf-945b6a7da83a
📒 Files selected for processing (7)
.github/cursor-review/README.md.github/cursor-review/catalog-drift.py.github/cursor-review/tests/test_catalog_drift.py.github/workflows/cursor-review-catalog-drift.yml.github/workflows/test-cursor-review-scripts.ymlAGENTS.mdREADME.md
…icky 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.
![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 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…e 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.
|
Review resolution pass — all 13 threads (3 CodeRabbit + 10 cursor-review panel) addressed and resolved across Two findings genuinely changed behavior, not just hardened it:
The workflow's security posture changed shape. Also fixed: the pin extractor now rejects any entry that is not a bare model id on both the strict-JSON and quoted-token paths (a valid JSON array can carry a Two follow-ups proposed rather than fixed here:
Tests: 94, up from 78 — every fix above has a named regression test. One thing to know before merging: the 4-lab × 2-type panel reviewed |
2 participants
ELI-5
The review panel pins four model ids (plus a judge). Cursor keeps swapping models under us: a pinned id gets delisted (every consumer PR then fails the review preflight), or a newer/better model ships and we never notice. The only thing watching for the second case was a hand-written
last checked <date>comment — which went stale in 13 days, and both swaps the BE-4817 audit found (Opus 5 on 07-24, Kimi K3 on ~07-26) landed inside that window and were caught by a human, not by CI.This adds a robot that checks every Monday. It reads the pins out of
cursor-review.ymlitself, asks Cursor what models actually exist, and writes what it finds into one sticky GitHub issue — which it updates in place each week and closes automatically once the drift is gone.What this changes
New
.github/workflows/cursor-review-catalog-drift.yml—schedule(17 6 * * 1) +workflow_dispatch,contents: read+issues: write:cursor-agent models(hard fail on failure — preflight's "better red than silent" stance)..github/workflows/cursor-review.yml— theDefine panel modelsheredoc plus thejudge_modelinput default. Nothing is duplicated, so the checker stays zero-maintenance when the pins are bumped (e.g. by chore(cursor-review): bump panel pins to claude-opus-5-thinking-max and kimi-k3-max (BE-4818) #86).(?<![\w.-])<id>(?![\w.-])):(NO ZDR)) reproduced verbatim and a caution that an unmarked id is not thereby confirmed ZDR-eligible;last checked YYYY-MM-DDolder than 30 days (or missing).[cursor-review catalog drift] …, found by exact title prefix. Raw catalog folded into a<details>block. No findings → the sticky issue is closed and the run is a green no-op.Comparison + rendering live in
.github/cursor-review/catalog-drift.pywith 30 unit tests;cursor-review.ymland the new workflow jointest-cursor-review-scripts.yml's path filter. Docs: a row in the root README table, a paragraph in the cursor-review README's panel section (thelast checkedcomment stays the human-audit record), and anAGENTS.mdlayout line.Verification
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'→ 78 passed (75 pre-existing + the new suite).actionlint(which runs shellcheck over therun:blocks) clean on every workflow;check_agents_md.pypasses.cursor-review.ymlwith synthesized catalogs: a clean catalog reportsno drift; a catalog withkimi-k3-codereplacingkimi-k2.7-codeplus a newclaude-opus-5-thinking-maxreproduces exactly the BE-4817 scenario — 1 delisted pin (withkimi-k3-codenamed as the same-lab alternative), 3 unpinned same-lab ids, stale audit date.cursor-review.yml, so a refactor of the pins block fails a cheap unit run instead of silently making the weekly check report "no drift" forever. Verified against chore(cursor-review): bump panel pins to claude-opus-5-thinking-max and kimi-k3-max (BE-4818) #86's diff: it changes the pin values and the audit date but keeps every anchor, so extraction still works after that lands.cursor-agent modelswithout credentials → exit 1, empty stdout (ran it locally). So the workflow'sif ! cursor-agent modelsguard genuinely catches the auth case rather than proceeding with garbage. A second guard rejects a non-empty but unparseable catalog rather than reporting "all four pins delisted".ci-cursor-review.ymlruns (which need the key for preflight) are succeeding as recently as 2026-07-28, so the org secret does reach this repo and the missing-secret branch is a real-failure path, not the expected one. (Repo-level secret list shows onlyCLOUD_CODE_BOT_PRIVATE_KEY; the org secret list needsadmin:org, which I don't have — the succeeding runs are the evidence.)Not verified, and it cannot be until this merges: the ticket's
workflow_dispatchchecks (dispatch once, dispatch again to confirm the sticky issue updates in place; a fake-id branch run for the delisted path). GitHub only exposesworkflow_dispatchfor workflows present on the default branch, so neither can run from this branch. The dry runs above cover the same logic offline; the first scheduled/dispatched run is the live confirmation.Judgment calls
sed/yqin the workflow. The ticket sketched inline extraction; this repo's convention (AGENTS.md) is that cursor-review logic lives in.github/cursor-review/with unit tests, and inline heredocs in a scheduled-only workflow are otherwise untestable before merge. Behavior is the ticket's; only the location changed.gpt-/claude-/gemini-/kimi-. Today that resolves to exactly that list (it reuses preflight'sre.split(r"[-.]", id, 1)[0]lab derivation), and it keeps working if a pin moves to a differently-branded lab. Consequence, stated deliberately: ids from labs the panel does not pin (e.g.fable-*) are not in the review-me list — they are in the raw-catalog fold, which is where an excluded-by-policy lab belongs.--search. Search is fuzzy and eventually consistent, so a second dispatch minutes after the first could miss its own issue and file a duplicate — precisely the ticket's verification scenario.test-cursor-review-scripts.yml's path filter. Small scope add: the anchor test only protects the extraction if it runs when the file it parses changes.cursor-review.yml, which is the single source of truth for every consumer's pins, so the README row marks it internal.Notes
runs_onandmodelsinputs forcursor-review.ymlthat the workflow does not currently declare.