From f1800d76dd88f16be4b650dc10eddb23bfb15dde Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 7 Jul 2026 14:34:03 -0700 Subject: [PATCH] fix(cursor-review): fail loud when a panel model id is delisted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor delisting a pinned catalog model id used to degrade the panel silently: the per-cell `cursor-agent --model ` call exits non-zero with 0 bytes of stdout, which the extractor tagged `empty` — indistinguishable from "the model ran and found nothing" — so the panel ran a lab short for weeks, undetected. Two changes make catalog drift fail loud: - New preflight job runs `cursor-agent models` once before the 8-cell fan-out and hard-fails the run if any pinned id is absent from the live catalog, naming the missing id(s) and the closest same-lab hint. Fork PRs (no CURSOR_API_KEY) skip it, matching the gate's existing fork handling. The job is also the single source of truth for the matrix model list, so the validated list and the reviewed list cannot drift apart. - Defense-in-depth in extract-findings.py: the run step now forwards the cursor-agent exit code + stderr, and a cell whose call exited non-zero or whose stderr matches `Cannot use this model:` is classified `error` (which post-review.py reports as `(error)` and the existing degraded-panel path fires) instead of a silent `empty`. --- .github/cursor-review/extract-findings.py | 82 ++++++++++ .../tests/test_extract_findings.py | 91 ++++++++++- .github/workflows/cursor-review.yml | 145 ++++++++++++++++-- 3 files changed, 305 insertions(+), 13 deletions(-) diff --git a/.github/cursor-review/extract-findings.py b/.github/cursor-review/extract-findings.py index 42d0a08..221e4fb 100644 --- a/.github/cursor-review/extract-findings.py +++ b/.github/cursor-review/extract-findings.py @@ -107,6 +107,58 @@ def parse_json_findings(raw_text: str): return None +def parse_exit_code(value): + """Coerce the --exit-code argument to an int, or None if unknown. + + The workflow passes the captured cursor-agent exit status through as a + string that may be blank (the run step didn't record one) or absent, so + treat anything non-integer as "unknown" rather than erroring. + """ + if value is None: + return None + value = value.strip() + if not value: + return None + try: + return int(value) + except ValueError: + return None + + +# A delisted / unavailable model makes cursor-agent print this to stderr and +# exit non-zero with zero bytes on stdout. Matching it lets us tag the cell as +# a loud `error` instead of an `empty` that reads as "ran and found nothing". +_MODEL_UNAVAILABLE_RE = re.compile(r"Cannot use this model:.*", re.IGNORECASE) + + +def classify_run_error(exit_code, stderr_text, raw): + """Return an error message if the cursor-agent call clearly failed, else None. + + Two signals: + + * stderr names an unusable model (`Cannot use this model: `) — this is + definitive (the model never ran), so it wins even when stdout has content. + * a non-zero exit code AND empty stdout — the call failed and produced + nothing, which the caller would otherwise misread as an `empty` + (found-nothing) cell. A non-zero exit that still yielded parseable + findings is left to the normal parse path so real findings are never + discarded. + """ + stderr_text = stderr_text or "" + match = _MODEL_UNAVAILABLE_RE.search(stderr_text) + if match: + return match.group(0).strip() + + if exit_code not in (None, 0) and not (raw or "").strip(): + msg = f"cursor-agent exited with status {exit_code} and produced no output." + tail = [line.strip() for line in stderr_text.splitlines() if line.strip()] + if tail: + msg += f" Last stderr: {tail[-1]}" + return msg + + return None + + def coerce_findings_list(parsed): """Reduce a parsed JSON value to the findings list, or None if it isn't one. @@ -131,10 +183,28 @@ def main(): parser.add_argument("--out", required=True, help="Path to write the findings JSON file") parser.add_argument("--model", required=True) parser.add_argument("--review-type", required=True) + parser.add_argument( + "--exit-code", + default=None, + help="cursor-agent process exit status (blank/absent = unknown).", + ) + parser.add_argument( + "--stderr", + default=None, + help="Path to the cursor-agent stderr capture, used to classify run errors.", + ) args = parser.parse_args() record = {"model": args.model, "review_type": args.review_type} + stderr_text = "" + if args.stderr: + try: + with open(args.stderr, encoding="utf-8", errors="replace") as f: + stderr_text = f.read() + except OSError: + stderr_text = "" + try: with open(args.raw, encoding="utf-8") as f: raw = f.read() @@ -144,6 +214,18 @@ def main(): json.dump(record, f) return + # Defense-in-depth against silent catalog drift: a delisted/unavailable + # model exits non-zero with a "Cannot use this model: " stderr and no + # stdout. Tag that as a loud `error` (which post-review.py reports as + # `(error)` and counts as a failed cell) rather than an `empty` that is + # indistinguishable from "the model ran and found nothing". + run_error = classify_run_error(parse_exit_code(args.exit_code), stderr_text, raw) + if run_error is not None: + record.update(status="error", error=run_error, findings=[]) + with open(args.out, "w", encoding="utf-8") as f: + json.dump(record, f) + return + if not raw.strip(): record.update(status="empty", error="Cursor agent produced empty output.", findings=[]) with open(args.out, "w", encoding="utf-8") as f: diff --git a/.github/cursor-review/tests/test_extract_findings.py b/.github/cursor-review/tests/test_extract_findings.py index e735138..aad9888 100644 --- a/.github/cursor-review/tests/test_extract_findings.py +++ b/.github/cursor-review/tests/test_extract_findings.py @@ -102,10 +102,59 @@ def test_object_without_findings_key(self): self.assertIsNone(_findings('{"summary": "looks good", "count": 0}')) +class ClassifyRunErrorTest(unittest.TestCase): + """Unit-cover the delisted-model / failed-invocation classifier.""" + + def test_cannot_use_model_stderr_wins(self): + # The delisted-model fingerprint: non-zero exit, empty stdout, and a + # "Cannot use this model: " stderr. Must classify as error, and + # surface the specific stderr line. + msg = ef.classify_run_error(1, "Cannot use this model: kimi-k2.5\n", "") + self.assertEqual(msg, "Cannot use this model: kimi-k2.5") + + def test_cannot_use_model_wins_even_with_stdout(self): + # The marker is definitive (the model never ran), so it wins even if + # some stray text landed on stdout. + msg = ef.classify_run_error(1, "error: Cannot use this model: gone-model", "noise") + self.assertEqual(msg, "Cannot use this model: gone-model") + + def test_nonzero_exit_and_empty_stdout_is_error(self): + msg = ef.classify_run_error(2, "some transient failure\n", " ") + self.assertIsNotNone(msg) + self.assertIn("status 2", msg) + self.assertIn("some transient failure", msg) + + def test_nonzero_exit_but_findings_present_is_not_error(self): + # A non-zero exit that still produced usable output must NOT be + # discarded — leave it to the normal parse path. + self.assertIsNone(ef.classify_run_error(1, "", json.dumps(FINDINGS))) + + def test_zero_exit_empty_is_not_error(self): + # A clean exit with empty output is a genuine "found nothing" — stays + # empty, not error. + self.assertIsNone(ef.classify_run_error(0, "", "")) + + def test_unknown_exit_is_not_error(self): + self.assertIsNone(ef.classify_run_error(None, "", "")) + + +class ParseExitCodeTest(unittest.TestCase): + def test_integer_string(self): + self.assertEqual(ef.parse_exit_code("1"), 1) + + def test_blank_and_none_are_unknown(self): + self.assertIsNone(ef.parse_exit_code("")) + self.assertIsNone(ef.parse_exit_code(" ")) + self.assertIsNone(ef.parse_exit_code(None)) + + def test_non_integer_is_unknown(self): + self.assertIsNone(ef.parse_exit_code("not-a-number")) + + class MainEndToEndTest(unittest.TestCase): """Drive main() the way the workflow does, asserting the status field.""" - def _run(self, raw_text): + def _run(self, raw_text, exit_code=None, stderr_text=None): with tempfile.TemporaryDirectory() as d: raw_path = os.path.join(d, "raw.txt") out_path = os.path.join(d, "out.json") @@ -114,13 +163,21 @@ def _run(self, raw_text): import sys argv = sys.argv - sys.argv = [ + new_argv = [ "extract-findings.py", "--raw", raw_path, "--out", out_path, "--model", "judge-model", "--review-type", "judge", ] + if exit_code is not None: + new_argv += ["--exit-code", str(exit_code)] + if stderr_text is not None: + stderr_path = os.path.join(d, "stderr.txt") + with open(stderr_path, "w", encoding="utf-8") as f: + f.write(stderr_text) + new_argv += ["--stderr", stderr_path] + sys.argv = new_argv try: ef.main() finally: @@ -143,6 +200,36 @@ def test_empty_is_empty(self): record = self._run(" \n ") self.assertEqual(record["status"], "empty") + def test_delisted_model_is_error_not_empty(self): + # The core regression: a delisted model (empty stdout + non-zero exit + + # "Cannot use this model:" stderr) must be `error`, never `empty`. + record = self._run( + "", + exit_code=1, + stderr_text="Cannot use this model: kimi-k2.5\n", + ) + self.assertEqual(record["status"], "error") + self.assertIn("Cannot use this model: kimi-k2.5", record["error"]) + self.assertEqual(record["findings"], []) + + def test_nonzero_exit_empty_output_is_error(self): + record = self._run("", exit_code=137, stderr_text="killed\n") + self.assertEqual(record["status"], "error") + self.assertIn("status 137", record["error"]) + + def test_empty_without_error_signals_stays_empty(self): + # Passing the args but with a clean exit and no stderr must not change + # the genuine found-nothing classification. + record = self._run("", exit_code=0, stderr_text="") + self.assertEqual(record["status"], "empty") + + def test_findings_survive_nonzero_exit(self): + # A non-zero exit that still produced findings must not be discarded. + raw = json.dumps(FINDINGS) + record = self._run(raw, exit_code=1, stderr_text="") + self.assertEqual(record["status"], "ok") + self.assertEqual(record["findings"], FINDINGS) + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index c17f998..d1b8a30 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -283,11 +283,121 @@ jobs: echo "within_cap=true" >> "$GITHUB_OUTPUT" fi + preflight: + # Validate the panel's pinned model ids against the LIVE Cursor catalog + # once, before the 8-cell fan-out. When Cursor delists a pinned id the + # per-cell `cursor-agent --model ` call fails with 0 bytes of stdout, + # which downstream reads as an `empty` cell — indistinguishable from "the + # model ran and found nothing", so the panel silently runs a lab short for + # weeks. Failing here turns that drift into a loud, visible job failure on + # the very next run. This job is ALSO the single source of truth for the + # matrix model list (emitted as an output the `review` matrix consumes), so + # the validated list and the reviewed list can never drift apart. + name: Preflight — validate model catalog + needs: [gate, diff-size] + if: needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + outputs: + models: ${{ steps.models.outputs.models }} + steps: + - name: Define panel models + id: models + # Single source of truth for the per-lab model pins. The `review` + # matrix consumes this list via fromJSON, and the catalog check below + # validates every entry — so a bump only ever touches this one list. + # Keep in sync with the README model table. + run: | + cat > /tmp/models.json <<'JSON' + [ + "gpt-5.3-codex-xhigh", + "claude-opus-4-8-thinking-xhigh", + "gemini-3.1-pro", + "kimi-k2.7-code" + ] + JSON + echo "models=$(jq -c . /tmp/models.json)" >> "$GITHUB_OUTPUT" + + - name: Install Cursor agent CLI + env: + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} + run: | + # Fork PRs (and any run without the secret) get no CURSOR_API_KEY, so + # `cursor-agent models` can't authenticate. Skip cleanly — matches the + # gate's fork handling, which already sets should_run=false for forks. + if [ -z "$CURSOR_API_KEY" ]; then + echo "No CURSOR_API_KEY (fork PR or unset secret) — skipping catalog preflight." + exit 0 + fi + curl https://cursor.com/install -fsSL | bash + echo "$HOME/.cursor/bin" >> "$GITHUB_PATH" + + - name: Validate model pins against live catalog + env: + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} + run: | + if [ -z "$CURSOR_API_KEY" ]; then + echo "No CURSOR_API_KEY — skipping catalog preflight (matches fork handling)." + exit 0 + fi + + # Capture the live catalog. A failure here (auth/network) is itself a + # hard fail — better red than a silently degraded panel. + if ! cursor-agent models > /tmp/catalog.txt 2>/tmp/catalog-stderr.txt; then + echo "::error::'cursor-agent models' failed — cannot verify the panel model pins." + cat /tmp/catalog-stderr.txt + exit 1 + fi + echo "=== Cursor catalog ===" + cat /tmp/catalog.txt + + python3 - <<'PY' + import json + import re + import sys + + with open("/tmp/models.json", encoding="utf-8") as f: + pins = json.load(f) + with open("/tmp/catalog.txt", encoding="utf-8") as f: + catalog_raw = f.read() + + catalog_lines = [ln.strip() for ln in catalog_raw.splitlines() if ln.strip()] + + def present(model_id): + # Match the id as a whole token so `kimi-k2.5` doesn't match inside + # `kimi-k2.55`; ids contain `.`/`-`, so bound on those too. + pattern = r"(? /tmp/review-raw.txt 2>/tmp/review-stderr.txt || true + > /tmp/review-raw.txt 2>/tmp/review-stderr.txt \ + && CURSOR_EXIT=0 || CURSOR_EXIT=$? + echo "$CURSOR_EXIT" > /tmp/review-exit-code + echo "cursor-agent exit code: $CURSOR_EXIT" echo "=== Raw output (first 500 chars) ===" head -c 500 /tmp/review-raw.txt @@ -415,11 +531,18 @@ jobs: REVIEW_TYPE: ${{ matrix.review_type }} run: | mkdir -p /tmp/findings-out + # Pass the run's exit code + stderr so a delisted-model failure is + # classified `error`, not `empty`. Both are best-effort: a blank + # exit code (run step skipped) reads as "unknown", and a missing + # stderr file is tolerated by extract-findings.py. + CURSOR_EXIT="$(cat /tmp/review-exit-code 2>/dev/null || true)" python3 "$CURSOR_REVIEW_ASSETS/extract-findings.py" \ --raw /tmp/review-raw.txt \ --out /tmp/findings-out/findings.json \ --model "$MODEL" \ - --review-type "$REVIEW_TYPE" + --review-type "$REVIEW_TYPE" \ + --exit-code "$CURSOR_EXIT" \ + --stderr /tmp/review-stderr.txt echo "=== Extracted findings ===" cat /tmp/findings-out/findings.json