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