[PR #22] [MERGED] fix(cursor-review): fail loud when a panel model id is delisted #33

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:35:35 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/22
Author: @mattmillerai
Created: 7/7/2026
Status: ✅ Merged
Merged: 7/21/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-2627-fail-loud-delisted-models

📝 Commits (3)

b0af3d5 fix(cursor-review): bump dead kimi-k2.5 model id to kimi-k2.7-code
f1800d7 fix(cursor-review): fail loud when a panel model id is delisted
720a7e5 Merge remote-tracking branch 'origin/main' into matt/be-2627-fail-loud-delisted-models

📊 Changes

3 files changed (+311 additions, -17 deletions)

View changed files

📝 .github/cursor-review/extract-findings.py (+82 -0)
📝 .github/cursor-review/tests/test_extract_findings.py (+89 -2)
📝 .github/workflows/cursor-review.yml (+140 -15)

📄 Description

ELI-5

The review panel pins one exact model name per AI lab. When a lab quietly
retires (delists) one of those names, the panel used to just… skip that model
and say nothing — so we'd run a review with 3 labs instead of 4 for weeks and
never notice. This PR makes that mistake loud: the run now checks every
pinned model against the live catalog before it starts, and fails the whole
run (with a clear "this model id is gone, here's the closest one" message) if
any pin is dead. As a backup, even if that check is bypassed, a dead model now
shows up as (error) in the panel summary instead of a silent "found nothing".

What changed

1. Preflight catalog check (fail-loud, primary). A new preflight job runs
cursor-agent models once, before the 8-cell fan-out, and hard-fails the run if
any pinned panel model id is absent from the live catalog — naming the missing
id(s) plus the closest same-lab hint. This job is also the single source of
truth for the matrix model list (the review matrix now consumes it via
fromJSON), so the list we validate and the list we review can never drift
apart. Fork PRs / runs without CURSOR_API_KEY skip the check cleanly, matching
the gate's existing fork handling (forks already yield should_run=false).

2. Defense-in-depth in extract-findings.py (backup). The per-cell run step
now forwards the cursor-agent exit code + stderr into extract-findings.py. A
cell whose call exited non-zero or whose stderr matches
Cannot use this model: is now classified status=error (which post-review.py
already renders as (error) and counts as a failed cell, firing the existing
degraded-panel path) instead of a silent empty. A non-zero exit that still
produced valid findings is left untouched, so real findings are never discarded.

Testing

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' —
27 tests, all green. New coverage: ClassifyRunErrorTest, ParseExitCodeTest,
and end-to-end test_delisted_model_is_error_not_empty (the core regression).
Workflow YAML validated (parses; the review matrix expands from the preflight
output).

Notes / judgment calls

Stacked on #21 (base = matt/bump-kimi-k2.7-code). That PR carries the
live kimi-k2.7-code pin; this PR is the durability half. GitHub retargets to
main once #21 merges. Review sees only this net diff.
Single source of truth over a duplicated list. Rather than hardcode the
model list a second time in the preflight (a new drift vector — the whole bug
this fixes), the preflight owns the list and the matrix derives from it.
Judge model is out of scope. The preflight validates the 4 matrix pins, not
the separate judge_model input. Its default equals a panel pin (so it's
covered transitively), and a delisted judge already degrades via consolidate's
judge-failed banner rather than silently. A custom, delisted judge_model
override would not be caught here — noted as a possible follow-up.
The weekly scheduled drift-check (auto-diff the matrix vs the catalog and open a
bump ticket) is intentionally not included — fail-loud on the next PR run is
enough for now.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/22 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/7/2026 **Status:** ✅ Merged **Merged:** 7/21/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2627-fail-loud-delisted-models` --- ### 📝 Commits (3) - [`b0af3d5`](https://github.com/Comfy-Org/github-workflows/commit/b0af3d56b324d255569bfdc59cf7e07d9a7e9a40) fix(cursor-review): bump dead kimi-k2.5 model id to kimi-k2.7-code - [`f1800d7`](https://github.com/Comfy-Org/github-workflows/commit/f1800d76dd88f16be4b650dc10eddb23bfb15dde) fix(cursor-review): fail loud when a panel model id is delisted - [`720a7e5`](https://github.com/Comfy-Org/github-workflows/commit/720a7e5afdfc9fbd4f818428462e872974de4dca) Merge remote-tracking branch 'origin/main' into matt/be-2627-fail-loud-delisted-models ### 📊 Changes **3 files changed** (+311 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/extract-findings.py` (+82 -0) 📝 `.github/cursor-review/tests/test_extract_findings.py` (+89 -2) 📝 `.github/workflows/cursor-review.yml` (+140 -15) </details> ### 📄 Description ## ELI-5 The review panel pins one exact model name per AI lab. When a lab quietly retires (delists) one of those names, the panel used to just… skip that model and say nothing — so we'd run a review with 3 labs instead of 4 for weeks and never notice. This PR makes that mistake **loud**: the run now checks every pinned model against the live catalog *before* it starts, and fails the whole run (with a clear "this model id is gone, here's the closest one" message) if any pin is dead. As a backup, even if that check is bypassed, a dead model now shows up as `(error)` in the panel summary instead of a silent "found nothing". ## What changed **1. Preflight catalog check (fail-loud, primary).** A new `preflight` job runs `cursor-agent models` once, before the 8-cell fan-out, and hard-fails the run if any pinned panel model id is absent from the live catalog — naming the missing id(s) plus the closest same-lab hint. This job is *also* the single source of truth for the matrix model list (the `review` matrix now consumes it via `fromJSON`), so the list we validate and the list we review can never drift apart. Fork PRs / runs without `CURSOR_API_KEY` skip the check cleanly, matching the gate's existing fork handling (forks already yield `should_run=false`). **2. Defense-in-depth in `extract-findings.py` (backup).** The per-cell run step now forwards the cursor-agent exit code + stderr into `extract-findings.py`. A cell whose call exited non-zero **or** whose stderr matches `Cannot use this model:` is now classified `status=error` (which `post-review.py` already renders as `(error)` and counts as a failed cell, firing the existing degraded-panel path) instead of a silent `empty`. A non-zero exit that still produced valid findings is left untouched, so real findings are never discarded. ## Testing - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — 27 tests, all green. New coverage: `ClassifyRunErrorTest`, `ParseExitCodeTest`, and end-to-end `test_delisted_model_is_error_not_empty` (the core regression). - Workflow YAML validated (parses; the `review` matrix expands from the preflight output). ## Notes / judgment calls - **Stacked on #21** (base = `matt/bump-kimi-k2.7-code`). That PR carries the live `kimi-k2.7-code` pin; this PR is the durability half. GitHub retargets to `main` once #21 merges. Review sees only this net diff. - **Single source of truth over a duplicated list.** Rather than hardcode the model list a second time in the preflight (a new drift vector — the whole bug this fixes), the preflight *owns* the list and the matrix derives from it. - **Judge model is out of scope.** The preflight validates the 4 matrix pins, not the separate `judge_model` input. Its default equals a panel pin (so it's covered transitively), and a delisted judge already degrades via consolidate's judge-failed banner rather than silently. A custom, delisted `judge_model` override would not be caught here — noted as a possible follow-up. - The weekly scheduled drift-check (auto-diff the matrix vs the catalog and open a bump ticket) is intentionally **not** included — fail-loud on the next PR run is enough for now. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:35:35 +00:00