fix(cursor-review): fail loud when a panel model id is delisted

.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: <id>`) — 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: <id>" 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:

.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: <id>" 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()