From fc7ba0e91468d2dfcd9f75555a3ecace51d519b9 Mon Sep 17 00:00:00 2001 From: Hunter Senft-Grupp Date: Fri, 17 Jul 2026 22:17:15 +0000 Subject: [PATCH 1/4] feat(cursor-review): collect structured output through MCP tools --- .github/cursor-review/README.md | 13 +- .github/cursor-review/extract-findings.py | 275 ----------------- .github/cursor-review/post-review.py | 4 +- .github/cursor-review/prompt-adversarial.md | 27 +- .github/cursor-review/prompt-edge-case.md | 27 +- .github/cursor-review/prompt-judge.md | 27 +- .github/cursor-review/review-output-mcp.py | 271 +++++++++++++++++ .../tests/test_extract_findings.py | 284 ------------------ .../tests/test_review_output_mcp.py | 143 +++++++++ .github/workflows/cursor-review.yml | 231 ++++++++------ .../workflows/test-cursor-review-scripts.yml | 8 +- 11 files changed, 610 insertions(+), 700 deletions(-) delete mode 100644 .github/cursor-review/extract-findings.py create mode 100644 .github/cursor-review/review-output-mcp.py delete mode 100644 .github/cursor-review/tests/test_extract_findings.py create mode 100644 .github/cursor-review/tests/test_review_output_mcp.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 7ee26ae..99f6714 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -35,7 +35,7 @@ PR gets the `cursor-review` label │ Google ▢ ▢ │ │ Moonshot ▢ ▢ │ │ │ - │ each cell: build prompt → run cursor-agent → extract-findings.py │ + │ each cell: cursor-agent records findings through a stdio MCP tool │ └───────────────────────────────┬───────────────────────────────────────┘ │ 8 findings artifacts ▼ @@ -71,9 +71,12 @@ Each model runs **two review types**: [`prompt-edge-case.md`](prompt-edge-case.md). A single **judge** model ([`prompt-judge.md`](prompt-judge.md)) then adjudicates -all 8 cells' findings into the final review. If a cell fails (checkout, agent, -extraction), it still shows up in the panel summary tagged `error` rather than -silently vanishing — the review tells you what didn't run. +all 8 cells' findings and submits the final review through the same slim stdio +MCP server. Model prose is never parsed for results: tool schemas validate the +records before writing them, which removes formatting drift, markdown fences, +truncated JSON, and reformat retries from the result path. If a cell fails +(checkout, agent, or tool submission), it still shows up in the panel summary +tagged `error` rather than silently vanishing. ## What's in this directory @@ -82,7 +85,7 @@ silently vanishing — the review tells you what didn't run. | [`prompt-adversarial.md`](prompt-adversarial.md) | Prompt for the security/reliability review pass. | | [`prompt-edge-case.md`](prompt-edge-case.md) | Prompt for the correctness/logic review pass. | | [`prompt-judge.md`](prompt-judge.md) | Prompt the judge model uses to consolidate panel findings into one review. | -| [`extract-findings.py`](extract-findings.py) | Parses a cell's raw `cursor-agent` output into a normalized findings record. Always emits structured JSON — even on empty output or parse failure — so the consolidate step has uniform input. | +| [`review-output-mcp.py`](review-output-mcp.py) | Dependency-free stdio MCP server. Reviewers record individual findings and finish; the judge submits the final findings array. It validates and atomically writes the normalized records consumed by later jobs. | | [`post-review.py`](post-review.py) | Reads the judge's consolidated findings and posts **one** PR review with line-anchored inline comments and severity badges. | | [`gate-unresolved.py`](gate-unresolved.py) | The opt-in blocking gate (`blocking: true`). Queries the PR's review threads and exits non-zero while any cursor-review finding thread is unresolved. | | [`slack-notify.sh`](slack-notify.sh) | Sends the start/complete Slack DMs to the triggerer (no-ops without a token). | diff --git a/.github/cursor-review/extract-findings.py b/.github/cursor-review/extract-findings.py deleted file mode 100644 index daff2f4..0000000 --- a/.github/cursor-review/extract-findings.py +++ /dev/null @@ -1,275 +0,0 @@ -#!/usr/bin/env python3 -"""Parse raw cursor-agent output into a normalized findings record. - -Used by per-cell matrix steps AND by the judge/consolidate step. Each caller -converts the model's raw stdout into a JSON file the next step can ingest. The -output is always structured — even on parse failures or empty output — so the -downstream step has a uniform input. - -Output shape: - { - "model": str, - "review_type": str, - "status": "ok" | "empty" | "error" | "parse_error", - "findings": [{"file": str, "line": int, "side": "RIGHT", "body": str}, ...], - "error": str # only when status != "ok" - } -""" - -import argparse -import json -import re - - -def _try_load(snippet: str): - """json.loads `snippet`, returning the value only if it's a list or dict. - - A bare number/string/bool is never a findings payload, so reject it — this - keeps the brace-scan below from "succeeding" on a stray scalar. - """ - try: - value = json.loads(snippet) - except (json.JSONDecodeError, ValueError): - return None - return value if isinstance(value, (list, dict)) else None - - -def _iter_json_candidates(text: str): - """Yield each top-level balanced {...} / [...] region embedded in `text`. - - String- and escape-aware: braces or brackets inside JSON string literals - don't throw off the nesting count, so prose like `... the findings […] are` - surrounding a real array doesn't corrupt the match the way a naive - first-`[`/last-`]` slice does. Regions are yielded in document order; the - caller parses each and keeps the last that is findings-shaped. - """ - openers = {"{", "["} - closers = {"}", "]"} - i, n = 0, len(text) - while i < n: - if text[i] not in openers: - i += 1 - continue - depth = 0 - in_str = False - escape = False - j = i - while j < n: - c = text[j] - if in_str: - if escape: - escape = False - elif c == "\\": - escape = True - elif c == '"': - in_str = False - elif c == '"': - in_str = True - elif c in openers: - depth += 1 - elif c in closers: - depth -= 1 - if depth == 0: - yield text[i : j + 1] - break - j += 1 - # Resume scanning after this region (or after an unbalanced opener). - i = j + 1 - - -def parse_json_findings(raw_text: str): - """Extract the findings JSON value from raw model output. - - Tolerates surrounding prose and markdown fences. Returns the parsed value - (a findings array, or a `{"findings": [...]}` wrapper), or None if no - findings-shaped JSON could be located. - - Crucially this scans for a *findings-shaped* region, not merely the first - thing that parses as JSON, and prefers the LAST such region. The judge - (esp. on verification-heavy diffs, BE-3160) opens with prose that quotes - individual finding OBJECTS or scalar lists inline while reasoning, then - emits the real array LAST. Taking the first parseable region there yields - an un-coercible object (→ spurious parse_error) or a bogus scalar list, - while the genuine findings array sits further down. Layered so a clean - response still takes the fast path: - - 1. The whole output is the findings JSON. - 2. A fenced ```json (or bare ```) block holds it — last valid block wins. - 3. A balanced {...}/[...] region embedded in prose — last valid wins. - """ - text = raw_text.strip() - - # Fast path: the whole response is the findings payload. - whole = _try_load(text) - if coerce_findings_list(whole) is not None: - return whole - - # Fenced blocks: prose/verification precedes the answer, so the last - # findings-shaped fence is the real one. - best = None - for match in re.finditer(r"```(?:json)?\s*\n(.*?)```", text, re.DOTALL): - parsed = _try_load(match.group(1).strip()) - if coerce_findings_list(parsed) is not None: - best = parsed - if best is not None: - return best - - # Bare balanced regions embedded in prose: keep the LAST findings-shaped - # one so an inline finding object / scalar list quoted mid-reasoning never - # shadows the real array that follows it. - for candidate in _iter_json_candidates(text): - parsed = _try_load(candidate) - if coerce_findings_list(parsed) is not None: - best = parsed - return best - - -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. - - A findings list is a JSON array of finding OBJECTS (an empty array is - allowed — "no findings"), or an object wrapping such an array under a - findings-like key. The panel cells and judge are asked for a bare JSON - array, but a model intermittently wraps it as `{"findings": [...]}` (or a - near-synonym key); unwrap those so a well-formed-but-wrapped response - parses instead of being discarded as a parse_error. - - Requiring the elements to be objects is what lets the extractor above skip - a scalar list the judge quotes in prose (e.g. `["contains", "startswith"]` - while narrating jq builtins) and keep scanning for the real findings array. - """ - if isinstance(parsed, list): - return parsed if all(isinstance(item, dict) for item in parsed) else None - if isinstance(parsed, dict): - for key in ("findings", "results", "items", "reviews"): - value = parsed.get(key) - if isinstance(value, list) and all(isinstance(item, dict) for item in value): - return value - return None - - -def main(): - parser = argparse.ArgumentParser() - parser.add_argument("--raw", required=True, help="Path to raw cursor-agent output") - 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() - except (OSError, UnicodeDecodeError) as e: - record.update(status="error", error=f"Could not read raw output: {e}", findings=[]) - with open(args.out, "w", encoding="utf-8") as f: - 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: - json.dump(record, f) - return - - findings = coerce_findings_list(parse_json_findings(raw)) - - if findings is None: - # Truncate raw so artifacts stay small even on chatty parse failures. - record.update( - status="parse_error", - error=f"Could not parse JSON findings from output. First 500 chars:\n{raw[:500]}", - findings=[], - ) - else: - record.update(status="ok", findings=findings) - - with open(args.out, "w", encoding="utf-8") as f: - json.dump(record, f) - - -if __name__ == "__main__": - main() diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 9a83bfe..a08b1ae 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -12,7 +12,7 @@ ... ], "panel": [ - {"model": str, "review_type": str, "status": "ok"|"empty"|"error"|"parse_error"}, + {"model": str, "review_type": str, "status": "ok"|"error"}, ... ] } @@ -28,7 +28,7 @@ import sys # Severity scale, ordered most → least urgent. Drives sort order, the inline -# comment prefix, and the summary table. The judge is instructed to emit one +# comment prefix, and the summary table. The judge tool accepts one # of these strings per finding (see prompt-judge.md); anything missing or # unrecognized falls back to DEFAULT_SEVERITY so a malformed value can never # drop a finding — it just lands in the middle bucket. diff --git a/.github/cursor-review/prompt-adversarial.md b/.github/cursor-review/prompt-adversarial.md index 2b0a4e3..0875649 100644 --- a/.github/cursor-review/prompt-adversarial.md +++ b/.github/cursor-review/prompt-adversarial.md @@ -19,25 +19,22 @@ Do NOT flag: - Performance micro-optimizations unless they create a DoS vector - Issues in test files unless the test itself is masking a real bug -Review the following diff and report every finding. You MUST respond with ONLY a JSON -array — no prose, no markdown fences, no explanation outside the array. - -Each element must be an object with exactly these keys: -- "file": string — the file path relative to the repo root -- "line": integer — the line number in the NEW side of the diff where the issue exists -- "side": "RIGHT" — always RIGHT since findings are on the new code -- "severity": string — one of "critical", "high", "medium", "low", "nit" +Review the following diff and record every finding with the +`cursor_review_record_finding` tool. Call it once per distinct issue using: +- `file`: the file path relative to the repo root +- `line`: the line number in the NEW side of the diff where the issue exists +- `side`: `RIGHT` since findings are on the new code +- `severity`: one of `critical`, `high`, `medium`, `low`, `nit` ("critical" = exploitable hole / data loss / crash on a normal path; "high" = real bug on a plausible input; "medium" = bug on an edge path; "low" = minor security/reliability concern; "nit" = very low-impact security/reliability concern) -- "body": string — a concise description of the issue (1-3 sentences) +- `body`: a concise description of the issue (1-3 sentences) -If you find no issues, return an empty array: [] +After recording all findings, call `cursor_review_finish` exactly once. Call it +even if you found no issues. Do not put findings in your final response: only +tool calls are collected. -Example response: -[ - {"file": "internal/api/handler.go", "line": 42, "side": "RIGHT", "severity": "critical", "body": "User-supplied `filename` is passed to `os.Open` without path-traversal validation. An attacker can read arbitrary files with `../../etc/passwd`."}, - {"file": "internal/worker/upload.go", "line": 118, "side": "RIGHT", "severity": "high", "body": "The goroutine captures `ctx` from the outer scope but the parent function returns and cancels the context before the upload completes, causing silent data loss."} -] +Example `cursor_review_record_finding` arguments: +`{"file":"internal/api/handler.go","line":42,"side":"RIGHT","severity":"critical","body":"User-supplied filename is passed to os.Open without path-traversal validation."}` === BEGIN DIFF === diff --git a/.github/cursor-review/prompt-edge-case.md b/.github/cursor-review/prompt-edge-case.md index a506868..a2e9625 100644 --- a/.github/cursor-review/prompt-edge-case.md +++ b/.github/cursor-review/prompt-edge-case.md @@ -21,25 +21,22 @@ Do NOT flag: - Performance optimizations unless they cause correctness issues - Issues in test files unless the test itself is masking a real bug -Review the following diff and report every finding. You MUST respond with ONLY a JSON -array — no prose, no markdown fences, no explanation outside the array. - -Each element must be an object with exactly these keys: -- "file": string — the file path relative to the repo root -- "line": integer — the line number in the NEW side of the diff where the issue exists -- "side": "RIGHT" — always RIGHT since findings are on the new code -- "severity": string — one of "critical", "high", "medium", "low", "nit" +Review the following diff and record every finding with the +`cursor_review_record_finding` tool. Call it once per distinct issue using: +- `file`: the file path relative to the repo root +- `line`: the line number in the NEW side of the diff where the issue exists +- `side`: `RIGHT` since findings are on the new code +- `severity`: one of `critical`, `high`, `medium`, `low`, `nit` ("critical" = data loss / crash on a normal path; "high" = real bug on a plausible input; "medium" = bug on an edge path; "low" = minor correctness issue; "nit" = very low-impact correctness/clarity issue) -- "body": string — a concise description of the issue (1-3 sentences) +- `body`: a concise description of the issue (1-3 sentences) -If you find no issues, return an empty array: [] +After recording all findings, call `cursor_review_finish` exactly once. Call it +even if you found no issues. Do not put findings in your final response: only +tool calls are collected. -Example response: -[ - {"file": "pkg/retry/retrier.go", "line": 55, "side": "RIGHT", "severity": "high", "body": "When `maxRetries` is 0 the loop body never executes, so the function returns `nil` instead of running the operation once. The guard should be `i <= maxRetries`."}, - {"file": "internal/router/router.go", "line": 203, "side": "RIGHT", "severity": "medium", "body": "The `default` branch of the select sends on `errCh` without checking if the channel is full. If two goroutines hit this path simultaneously the second send blocks forever, leaking the goroutine."} -] +Example `cursor_review_record_finding` arguments: +`{"file":"pkg/retry/retrier.go","line":55,"side":"RIGHT","severity":"high","body":"When maxRetries is 0 the loop never executes, so the operation is not attempted."}` === BEGIN DIFF === diff --git a/.github/cursor-review/prompt-judge.md b/.github/cursor-review/prompt-judge.md index dbe96e5..84af0c3 100644 --- a/.github/cursor-review/prompt-judge.md +++ b/.github/cursor-review/prompt-judge.md @@ -1,13 +1,8 @@ -RESPOND WITH ONLY A JSON ARRAY. Your entire response must be a single JSON -array of finding objects (or `[]` if none) — no prose, no preamble, no -explanation, no markdown code fences before or after it. Do not narrate your -reasoning. The exact element schema is specified at the end of this prompt. - You have NO shell, filesystem, or web/search tools in this environment. Do not attempt to use them and do not narrate attempts to (e.g. "shell execution isn't available here", "let me confirm via documentation", "verification changes my adjudication"). Adjudicate solely from the panel findings and diff provided -below, and emit ONLY the JSON array — any prose preamble breaks the contract. +below, then submit the result through the final-review tool. You are a senior software engineer adjudicating findings from a panel of AI code reviewers. The panel ran a 4-lab × 2-review-type matrix (8 cells total): @@ -34,17 +29,17 @@ Selection guidance: - Cap the final list at 10 findings. Below 10 is fine if there genuinely aren't more. -Output: a JSON array, no prose, no markdown fences. Each element is an -object with exactly: -- "file": string — repo-relative path -- "line": integer — a line number that appears on the RIGHT (new) side of +Submit the result exactly once with the `cursor_review_submit_final` tool. Its +`findings` argument contains each kept finding using: +- `file`: repo-relative path +- `line`: a line number that appears on the RIGHT (new) side of one of the diff hunks below. Lines that aren't in any hunk cannot be anchored as inline comments — GitHub will reject them. If a finding's natural anchor isn't shown in the diff, RETARGET it to the nearest RIGHT-side line that IS in a hunk, or DROP the finding. -- "side": "RIGHT" — always -- "severity": string — exactly one of "critical", "high", "medium", "low", - "nit". Use this rubric: +- `side`: `RIGHT` — always +- `severity`: exactly one of `critical`, `high`, `medium`, `low`, `nit`. + Use this rubric: - "critical": exploitable security hole, data loss/corruption, or a crash on a normal path. Ship-blocker. - "high": a real bug that will misbehave on a plausible input, or a serious @@ -53,11 +48,13 @@ object with exactly: a blocker. - "low": minor correctness or robustness issue with limited impact. - "nit": style, naming, or polish — optional to address. -- "body": string — concise (1-3 sentences). Do NOT prefix the body with a +- `body`: concise (1-3 sentences). Do NOT prefix the body with a severity word or emoji; the severity field drives the rendered badge. END with attribution like `_Raised by 3 of 8 reviewers (gpt-5.6-sol-max adversarial, claude-opus-4-8-thinking-max edge-case, gemini-3.1-pro adversarial)._` -Order the array most-severe first. If no findings rise to the bar, return []. +Order findings most-severe first. If no findings rise to the bar, submit an +empty `findings` array. Do not put the result in your final response: only the +tool call is collected. === BEGIN PANEL FINDINGS === diff --git a/.github/cursor-review/review-output-mcp.py b/.github/cursor-review/review-output-mcp.py new file mode 100644 index 0000000..15a631c --- /dev/null +++ b/.github/cursor-review/review-output-mcp.py @@ -0,0 +1,271 @@ +#!/usr/bin/env python3 +"""Minimal stdio MCP server for durable Cursor Review output.""" + +import argparse +import json +import os +import posixpath +import sys +import tempfile + +SEVERITIES = ("critical", "high", "medium", "low", "nit") +PROTOCOL_VERSION = "2025-06-18" +SUPPORTED_PROTOCOL_VERSIONS = { + "2024-11-05", + "2025-03-26", + "2025-06-18", + "2025-11-25", +} + +FINDING_SCHEMA = { + "type": "object", + "additionalProperties": False, + "required": ["file", "line", "side", "severity", "body"], + "properties": { + "file": {"type": "string", "minLength": 1, "maxLength": 1024}, + "line": {"type": "integer", "minimum": 1}, + "side": {"type": "string", "enum": ["RIGHT"]}, + "severity": {"type": "string", "enum": list(SEVERITIES)}, + "body": {"type": "string", "minLength": 1, "maxLength": 20000}, + }, +} + + +def write_record(path, record): + directory = os.path.dirname(os.path.abspath(path)) + os.makedirs(directory, exist_ok=True) + fd, temporary_path = tempfile.mkstemp(dir=directory, prefix=".review-output-") + try: + with os.fdopen(fd, "w", encoding="utf-8") as output: + json.dump(record, output) + os.replace(temporary_path, path) + except BaseException: + try: + os.unlink(temporary_path) + except FileNotFoundError: + pass + raise + + +def initial_record(args): + record = { + "status": "error", + "error": "Cursor agent did not submit structured review output.", + "findings": [], + } + if args.model: + record["model"] = args.model + if args.review_type: + record["review_type"] = args.review_type + return record + + +def read_record(args): + try: + with open(args.out, encoding="utf-8") as source: + return json.load(source) + except (OSError, json.JSONDecodeError): + return initial_record(args) + + +def validate_finding(value): + if not isinstance(value, dict): + raise ValueError("finding must be an object") + expected = {"file", "line", "side", "severity", "body"} + if set(value) != expected: + raise ValueError(f"finding keys must be exactly {sorted(expected)}") + + path = value["file"] + if not isinstance(path, str) or not path or len(path) > 1024: + raise ValueError("file must be a non-empty string of at most 1024 characters") + if path.startswith("/") or "\\" in path or "\x00" in path: + raise ValueError("file must be a repo-relative POSIX path") + normalized = posixpath.normpath(path) + if normalized in (".", "..") or normalized.startswith("../"): + raise ValueError("file must stay inside the repository") + + line = value["line"] + if isinstance(line, bool) or not isinstance(line, int) or line < 1: + raise ValueError("line must be a positive integer") + if value["side"] != "RIGHT": + raise ValueError("side must be RIGHT") + if value["severity"] not in SEVERITIES: + raise ValueError(f"severity must be one of {', '.join(SEVERITIES)}") + body = value["body"] + if not isinstance(body, str) or not body or len(body) > 20000: + raise ValueError("body must be a non-empty string of at most 20000 characters") + + return {**value, "file": normalized} + + +def tools_for(mode): + if mode == "reviewer": + return [ + { + "name": "cursor_review_record_finding", + "description": ( + "Record one code-review finding. Call once per distinct issue. " + "Only findings recorded with this tool count." + ), + "inputSchema": FINDING_SCHEMA, + }, + { + "name": "cursor_review_finish", + "description": ( + "Finish this review after recording every finding. Call exactly once, " + "including when there are no findings." + ), + "inputSchema": { + "type": "object", + "additionalProperties": False, + "properties": {}, + }, + }, + ] + return [ + { + "name": "cursor_review_submit_final", + "description": ( + "Submit the final adjudicated review. Call exactly once with every finding " + "that should be posted, or an empty findings array. This tool is the only " + "channel used for the final result." + ), + "inputSchema": { + "type": "object", + "additionalProperties": False, + "required": ["findings"], + "properties": { + "findings": { + "type": "array", + "maxItems": 10, + "items": FINDING_SCHEMA, + } + }, + }, + } + ] + + +def call_tool(args, name, arguments): + if not isinstance(arguments, dict): + raise ValueError("arguments must be an object") + + if args.mode == "reviewer" and name == "cursor_review_record_finding": + finding = validate_finding(arguments) + record = read_record(args) + if record.get("status") == "ok": + raise ValueError("review is already finished") + findings = record.get("findings") + if not isinstance(findings, list): + findings = [] + record.update( + status="error", + error="Cursor agent recorded findings but did not finish the review.", + findings=[*findings, finding], + ) + write_record(args.out, record) + return f"Recorded {finding['severity']} finding on {finding['file']}:{finding['line']}." + + if args.mode == "reviewer" and name == "cursor_review_finish": + if arguments: + raise ValueError("cursor_review_finish takes no arguments") + record = read_record(args) + if record.get("status") == "ok": + raise ValueError("review is already finished") + findings = record.get("findings") + record.update(status="ok", findings=findings if isinstance(findings, list) else []) + record.pop("error", None) + write_record(args.out, record) + return f"Finished review with {len(record['findings'])} finding(s)." + + if args.mode == "judge" and name == "cursor_review_submit_final": + if read_record(args).get("status") == "ok": + raise ValueError("final review is already submitted") + if set(arguments) != {"findings"} or not isinstance(arguments["findings"], list): + raise ValueError("findings must be an array and the only argument") + if len(arguments["findings"]) > 10: + raise ValueError("final review is capped at 10 findings") + findings = [validate_finding(finding) for finding in arguments["findings"]] + record = initial_record(args) + record.update(status="ok", findings=findings) + record.pop("error", None) + write_record(args.out, record) + return f"Submitted final review with {len(findings)} finding(s)." + + raise ValueError(f"unknown tool for {args.mode} mode: {name}") + + +def result(request_id, value): + return {"jsonrpc": "2.0", "id": request_id, "result": value} + + +def error(request_id, code, message): + return {"jsonrpc": "2.0", "id": request_id, "error": {"code": code, "message": message}} + + +def handle(args, message): + request_id = message.get("id") + method = message.get("method") + if method == "initialize": + params = message.get("params") or {} + requested = params.get("protocolVersion") if isinstance(params, dict) else None + return result(request_id, { + "protocolVersion": ( + requested if requested in SUPPORTED_PROTOCOL_VERSIONS else PROTOCOL_VERSION + ), + "capabilities": {"tools": {"listChanged": False}}, + "serverInfo": {"name": "cursor-review-output", "version": "1.0.0"}, + }) + if method == "ping": + return result(request_id, {}) + if method == "tools/list": + return result(request_id, {"tools": tools_for(args.mode)}) + if method == "tools/call": + params = message.get("params") or {} + try: + if not isinstance(params, dict): + raise ValueError("params must be an object") + text = call_tool(args, params.get("name"), params.get("arguments") or {}) + return result(request_id, { + "content": [{"type": "text", "text": text}], + "isError": False, + }) + except (KeyError, TypeError, ValueError) as exc: + return result(request_id, { + "content": [{"type": "text", "text": str(exc)}], + "isError": True, + }) + if method and method.startswith("notifications/"): + return None + return error(request_id, -32601, f"unknown method: {method}") + + +def serve(args): + for line in sys.stdin: + try: + message = json.loads(line) + if not isinstance(message, dict): + raise ValueError("request must be an object") + response = handle(args, message) + except (json.JSONDecodeError, ValueError) as exc: + response = error(None, -32700, str(exc)) + if response is not None: + print(json.dumps(response, separators=(",", ":")), flush=True) + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("--mode", choices=("reviewer", "judge"), required=True) + parser.add_argument("--out", required=True) + parser.add_argument("--model") + parser.add_argument("--review-type") + parser.add_argument("--init", action="store_true") + args = parser.parse_args() + if args.init: + write_record(args.out, initial_record(args)) + return + serve(args) + + +if __name__ == "__main__": + main() diff --git a/.github/cursor-review/tests/test_extract_findings.py b/.github/cursor-review/tests/test_extract_findings.py deleted file mode 100644 index 7b71a99..0000000 --- a/.github/cursor-review/tests/test_extract_findings.py +++ /dev/null @@ -1,284 +0,0 @@ -#!/usr/bin/env python3 -"""Regression tests for extract-findings.py JSON recovery. - -The judge/consolidate step intermittently returns valid findings JSON wrapped -in a sentence or two of prose. Discarding the whole run on that (the BE-1916 -parse_error class — hit on ComfyUI #487 and Alex's PR) is the bug these tests -guard against: prose-wrapped JSON must be recovered, not thrown away. - -Run: python3 .github/cursor-review/tests/test_extract_findings.py -""" - -import importlib.util -import json -import os -import tempfile -import unittest - -# extract-findings.py has a hyphen, so import it by path rather than `import`. -_MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "extract-findings.py") -_spec = importlib.util.spec_from_file_location("extract_findings", _MODULE_PATH) -ef = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(ef) - - -# A real-finding payload reused across cases. -FINDINGS = [ - { - "file": "internal/api/handler.go", - "line": 42, - "side": "RIGHT", - "severity": "high", - "body": "User-supplied filename reaches os.Open without traversal checks.", - }, - { - "file": "internal/worker/upload.go", - "line": 118, - "side": "RIGHT", - "severity": "medium", - "body": "Context is cancelled before the upload completes [see hunk], losing data.", - }, -] - - -def _findings(raw): - """Mirror main()'s parse → coerce pipeline.""" - return ef.coerce_findings_list(ef.parse_json_findings(raw)) - - -class ParseJsonFindingsTest(unittest.TestCase): - def test_bare_array(self): - self.assertEqual(_findings(json.dumps(FINDINGS)), FINDINGS) - - def test_empty_array(self): - self.assertEqual(_findings("[]"), []) - - def test_prose_wrapped_array_487(self): - # The #487 failure shape: a prose verdict, THEN the JSON array. The - # prose even contains a bracket pair, which is what broke the old naive - # first-`[`/last-`]` slice. - raw = ( - "Based on my review of the actual code, I can adjudicate the two " - "findings [both raised by multiple reviewers] as follows. Here is " - "the consolidated result:\n\n" + json.dumps(FINDINGS) + "\n\n" - "These are the only items that rise to the bar." - ) - self.assertEqual(_findings(raw), FINDINGS) - - def test_fenced_json_block(self): - raw = ( - "Sure — here are the findings:\n\n```json\n" - + json.dumps(FINDINGS) - + "\n```\nLet me know if you need more." - ) - self.assertEqual(_findings(raw), FINDINGS) - - def test_fenced_block_no_lang(self): - raw = "Result:\n\n```\n" + json.dumps(FINDINGS) + "\n```\n" - self.assertEqual(_findings(raw), FINDINGS) - - def test_object_wrapped_findings(self): - raw = "Here you go:\n" + json.dumps({"findings": FINDINGS}) - self.assertEqual(_findings(raw), FINDINGS) - - def test_brackets_inside_string_literals(self): - # Brackets inside a body string must not confuse the balanced scanner. - payload = [{"file": "a.py", "line": 1, "side": "RIGHT", "body": "uses arr[i] and m['k']"}] - raw = "Verdict below.\n" + json.dumps(payload) - self.assertEqual(_findings(raw), payload) - - def test_genuinely_malformed_returns_none(self): - self.assertIsNone(_findings("I reviewed the code and found nothing actionable.")) - - def test_truncated_json_returns_none(self): - # An unterminated array can't be recovered — must fail, not half-parse. - self.assertIsNone(_findings('[{"file": "a.py", "line": 1, "body": "oops"')) - - def test_scalar_is_not_findings(self): - # A bare number is valid JSON but never a findings payload. - self.assertIsNone(_findings("42")) - - def test_object_without_findings_key(self): - self.assertIsNone(_findings('{"summary": "looks good", "count": 0}')) - - def test_scalar_list_is_not_findings(self): - # A list of scalars is valid JSON but never a findings payload — it - # must not be mistaken for one (the PR-146 jq-builtins prose shape). - self.assertIsNone(_findings('["contains", "startswith", "ascii_downcase"]')) - - def test_verification_prose_with_inline_object_then_array_be3160(self): - # PR-145 shape: verification prose that quotes a single finding OBJECT - # inline, THEN the real array. The old first-parseable-region logic - # returned the un-coercible inline object → spurious parse_error. - raw = ( - "Verification changes my adjudication significantly. Three panel " - 'findings turn out to be misreads. For instance {"file": "b.go", ' - '"line": 3, "severity": "low"} does not hold on inspection.\n\n' - "Final consolidated findings:\n" + json.dumps(FINDINGS) - ) - self.assertEqual(_findings(raw), FINDINGS) - - def test_tool_narration_prose_with_scalar_list_then_array_be3160(self): - # PR-146 shape: tool-attempt narration containing an inline scalar list, - # THEN the real array. The old logic returned the scalar list as bogus - # findings; extraction must recover the genuine array instead. - raw = ( - "Shell execution isn't available here. Let me confirm the jq builtin " - 'set via documentation. The relevant keys are ["contains", ' - '"startswith", "ascii_downcase"].\n\n' + json.dumps(FINDINGS) - ) - self.assertEqual(_findings(raw), FINDINGS) - - def test_last_findings_array_wins(self): - # When multiple findings-shaped arrays appear, the LAST (the real answer - # after prose) wins over an earlier draft the judge second-guessed. - earlier = [{"file": "old.go", "line": 1, "side": "RIGHT", "body": "superseded"}] - raw = ( - "My first pass produced:\n" + json.dumps(earlier) + "\n\nOn " - "reflection that was wrong. The final answer is:\n" + json.dumps(FINDINGS) - ) - self.assertEqual(_findings(raw), FINDINGS) - - -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, 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") - with open(raw_path, "w", encoding="utf-8") as f: - f.write(raw_text) - import 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: - sys.argv = argv - with open(out_path, encoding="utf-8") as f: - return json.load(f) - - def test_prose_wrapped_parses_ok(self): - raw = "After review, my verdict is:\n\n" + json.dumps(FINDINGS) - record = self._run(raw) - self.assertEqual(record["status"], "ok") - self.assertEqual(record["findings"], FINDINGS) - - def test_verification_prose_then_array_is_ok_be3160(self): - # Acceptance #1: a judge output containing valid JSON after prose must - # consolidate (status ok), not fail as parse_error. - raw = ( - "Verification changes my adjudication significantly. Three panel " - "findings turn out to be misreads.\n\n" + json.dumps(FINDINGS) - ) - record = self._run(raw) - self.assertEqual(record["status"], "ok") - self.assertEqual(record["findings"], FINDINGS) - - def test_malformed_is_parse_error(self): - record = self._run("I could not find anything worth flagging.") - self.assertEqual(record["status"], "parse_error") - self.assertEqual(record["findings"], []) - - 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/cursor-review/tests/test_review_output_mcp.py b/.github/cursor-review/tests/test_review_output_mcp.py new file mode 100644 index 0000000..59995cf --- /dev/null +++ b/.github/cursor-review/tests/test_review_output_mcp.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Contract tests for the Cursor Review stdio MCP output tools.""" + +import importlib.util +import json +import os +import subprocess +import sys +import tempfile +import unittest + +MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "review-output-mcp.py") +SPEC = importlib.util.spec_from_file_location("review_output_mcp", MODULE_PATH) +MCP = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(MCP) + +FINDING = { + "file": "internal/api/handler.go", + "line": 42, + "side": "RIGHT", + "severity": "high", + "body": "User-supplied filename reaches os.Open without traversal checks.", +} + + +class ValidationTest(unittest.TestCase): + def test_finding_is_normalized(self): + finding = {**FINDING, "file": "internal/api/../api/handler.go"} + self.assertEqual(MCP.validate_finding(finding)["file"], FINDING["file"]) + + def test_rejects_path_traversal(self): + with self.assertRaisesRegex(ValueError, "inside the repository"): + MCP.validate_finding({**FINDING, "file": "../secret"}) + + def test_rejects_unknown_fields(self): + with self.assertRaisesRegex(ValueError, "keys must be exactly"): + MCP.validate_finding({**FINDING, "confidence": 0.9}) + + +class StdioServerTest(unittest.TestCase): + def run_server(self, mode, messages): + with tempfile.TemporaryDirectory() as directory: + output_path = os.path.join(directory, "findings.json") + subprocess.run( + [sys.executable, MODULE_PATH, "--mode", mode, "--out", output_path, + "--model", "test-model", "--review-type", mode, "--init"], + check=True, + ) + process = subprocess.run( + [sys.executable, MODULE_PATH, "--mode", mode, "--out", output_path, + "--model", "test-model", "--review-type", mode], + input="".join(json.dumps(message) + "\n" for message in messages), + text=True, + capture_output=True, + check=True, + ) + with open(output_path, encoding="utf-8") as source: + record = json.load(source) + responses = [json.loads(line) for line in process.stdout.splitlines()] + return record, responses + + def test_reviewer_records_findings_and_finishes(self): + record, responses = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "initialize", "params": {}}, + {"jsonrpc": "2.0", "id": 2, "method": "tools/call", "params": { + "name": "cursor_review_record_finding", "arguments": FINDING, + }}, + {"jsonrpc": "2.0", "id": 3, "method": "tools/call", "params": { + "name": "cursor_review_finish", "arguments": {}, + }}, + ]) + self.assertEqual(record["status"], "ok") + self.assertEqual(record["findings"], [FINDING]) + self.assertEqual([response["id"] for response in responses], [1, 2, 3]) + + def test_reviewer_must_finish_empty_review(self): + record, _ = self.run_server("reviewer", []) + self.assertEqual(record["status"], "error") + self.assertEqual(record["findings"], []) + + def test_recorded_finding_is_incomplete_until_finish(self): + record, _ = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { + "name": "cursor_review_record_finding", "arguments": FINDING, + }}, + ]) + self.assertEqual(record["status"], "error") + self.assertEqual(record["findings"], [FINDING]) + + def test_cannot_overwrite_finished_review(self): + record, responses = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { + "name": "cursor_review_finish", "arguments": {}, + }}, + {"jsonrpc": "2.0", "id": 2, "method": "tools/call", "params": { + "name": "cursor_review_record_finding", "arguments": FINDING, + }}, + ]) + self.assertEqual(record["findings"], []) + self.assertTrue(responses[1]["result"]["isError"]) + + def test_judge_submits_empty_final_review(self): + record, _ = self.run_server("judge", [ + {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { + "name": "cursor_review_submit_final", "arguments": {"findings": []}, + }}, + ]) + self.assertEqual(record["status"], "ok") + self.assertEqual(record["findings"], []) + + def test_invalid_tool_input_does_not_replace_output(self): + record, responses = self.run_server("judge", [ + {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { + "name": "cursor_review_submit_final", + "arguments": {"findings": [{**FINDING, "line": 0}]}, + }}, + ]) + self.assertEqual(record["status"], "error") + self.assertTrue(responses[0]["result"]["isError"]) + + def test_negotiates_only_supported_protocol(self): + _, responses = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "initialize", "params": { + "protocolVersion": "2099-01-01", + }}, + ]) + self.assertEqual( + responses[0]["result"]["protocolVersion"], MCP.PROTOCOL_VERSION + ) + + def test_negotiates_current_protocol(self): + _, responses = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "initialize", "params": { + "protocolVersion": "2025-11-25", + }}, + ]) + self.assertEqual( + responses[0]["result"]["protocolVersion"], "2025-11-25" + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 31cb62f..51c2d8f 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -5,7 +5,7 @@ name: Cursor Review (reusable) # judge model consolidates the findings into ONE review posted on the PR, # with per-finding severity badges (critical/high/medium/low/nit). # -# Single source of truth: the prompts and the extract/post/notify scripts +# Single source of truth: the prompts and the output/post/notify scripts # live in THIS repo under .github/cursor-review/. Consumer repos add only a # thin caller — they don't carry copies of the review logic, so there's no # drift to keep in sync. @@ -428,8 +428,8 @@ jobs: - name: Seed default findings artifact # Pre-seed a minimal artifact tagged status=error so this cell # ALWAYS shows up in the panel summary, even if a later step - # (checkout, Cursor install, prompt build, extract) fails. The - # `Extract findings` step overwrites this on success. Without + # (checkout, Cursor install, prompt build, tool submission) fails. The + # MCP output tool overwrites this on success. Without # this, an early-failing cell silently disappears from the panel # and the consolidated review undercounts the matrix. env: @@ -492,6 +492,56 @@ jobs: # would just ship malicious bits as a "new version" we'd then bump to. run: cursor-agent --version + - name: Configure structured review output + env: + MODEL: ${{ matrix.model }} + REVIEW_TYPE: ${{ matrix.review_type }} + run: | + python3 "$CURSOR_REVIEW_ASSETS/review-output-mcp.py" \ + --mode reviewer \ + --out /tmp/findings-out/findings.json \ + --model "$MODEL" \ + --review-type "$REVIEW_TYPE" \ + --init + + mkdir -p .cursor + python3 - <<'PY' + import json, os, sys, tempfile + + if os.path.islink(".cursor"): + raise SystemExit("Refusing to write Cursor config through a symlinked .cursor directory") + + config = { + "mcpServers": { + "cursor-review-output": { + "type": "stdio", + "command": sys.executable, + "args": [ + os.path.join(os.environ["CURSOR_REVIEW_ASSETS"], "review-output-mcp.py"), + "--mode", "reviewer", + "--out", "/tmp/findings-out/findings.json", + "--model", os.environ["MODEL"], + "--review-type", os.environ["REVIEW_TYPE"], + ], + } + } + } + cli_config = { + "permissions": { + "allow": [ + "Mcp(cursor-review-output:cursor_review_record_finding)", + "Mcp(cursor-review-output:cursor_review_finish)", + ], + "deny": [], + } + } + for filename, value in (("mcp.json", config), ("cli.json", cli_config)): + fd, temporary = tempfile.mkstemp(dir=".cursor", prefix=f".{filename}-") + with os.fdopen(fd, "w", encoding="utf-8") as output: + json.dump(value, output) + os.replace(temporary, os.path.join(".cursor", filename)) + PY + - name: Build prompt env: REVIEW_TYPE: ${{ matrix.review_type }} @@ -515,14 +565,13 @@ jobs: # protection — and as of cursor-agent 2026.05.05 it fails to start # on ubuntu-latest, producing empty output. # - # Capture the exit code instead of swallowing it with `|| true`: a - # delisted/unavailable model exits non-zero with empty stdout, and - # the extract step uses the code + stderr to tag the cell `error` - # (loud) rather than `empty` (silent). The `&& ... || ...` idiom - # records the code without tripping the shell's `set -e`. + # Capture the exit code without tripping the shell's `set -e`. If the + # agent fails before finishing its MCP submission, the pre-seeded + # structured artifact remains status=error. cursor-agent \ --print \ --trust \ + --approve-mcps \ --model "$MODEL" \ < /tmp/prompt.txt \ > /tmp/review-raw.txt 2>/tmp/review-stderr.txt \ @@ -536,27 +585,10 @@ jobs: echo "=== Stderr ===" cat /tmp/review-stderr.txt - - name: Extract findings + - name: Show structured findings if: always() - env: - MODEL: ${{ matrix.model }} - 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" \ - --exit-code "$CURSOR_EXIT" \ - --stderr /tmp/review-stderr.txt - - echo "=== Extracted findings ===" + echo "=== Structured findings ===" cat /tmp/findings-out/findings.json echo "" @@ -573,7 +605,7 @@ jobs: # Adjudicates the 8 cell artifacts into one consolidated review. # Calls cursor-agent one more time as the judge, with prompt-judge.md. name: Consolidate panel - needs: [gate, diff-size, review] + needs: [gate, diff-size, preflight, review] if: always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' runs-on: ubuntu-latest timeout-minutes: 15 @@ -615,8 +647,56 @@ jobs: path: /tmp/panel pattern: findings-* + - name: Configure structured final review + run: | + python3 "$CURSOR_REVIEW_ASSETS/review-output-mcp.py" \ + --mode judge \ + --out /tmp/judge-findings.json \ + --model "$JUDGE_MODEL" \ + --review-type judge \ + --init + + mkdir -p .cursor + python3 - <<'PY' + import json, os, sys, tempfile + + if os.path.islink(".cursor"): + raise SystemExit("Refusing to write Cursor config through a symlinked .cursor directory") + + config = { + "mcpServers": { + "cursor-review-output": { + "type": "stdio", + "command": sys.executable, + "args": [ + os.path.join(os.environ["CURSOR_REVIEW_ASSETS"], "review-output-mcp.py"), + "--mode", "judge", + "--out", "/tmp/judge-findings.json", + "--model", os.environ["JUDGE_MODEL"], + "--review-type", "judge", + ], + } + } + } + cli_config = { + "permissions": { + "allow": [ + "Mcp(cursor-review-output:cursor_review_submit_final)", + ], + "deny": [], + } + } + for filename, value in (("mcp.json", config), ("cli.json", cli_config)): + fd, temporary = tempfile.mkstemp(dir=".cursor", prefix=f".{filename}-") + with os.fdopen(fd, "w", encoding="utf-8") as output: + json.dump(value, output) + os.replace(temporary, os.path.join(".cursor", filename)) + PY + - name: Aggregate panel findings id: aggregate + env: + PANEL_MODELS: ${{ needs.preflight.outputs.models }} run: | # Each artifact directory contains one findings.json. Combine into # a single panel.json array preserving cell metadata. @@ -628,6 +708,24 @@ jobs: with open(path, encoding="utf-8") as f: panel.append(json.load(f)) + expected = { + (model, review_type) + for model in json.loads(os.environ["PANEL_MODELS"]) + for review_type in ("adversarial", "edge-case") + } + present = { + (cell.get("model"), cell.get("review_type")) + for cell in panel + } + for model, review_type in sorted(expected - present): + panel.append({ + "model": model, + "review_type": review_type, + "status": "error", + "error": "Cell findings artifact was not uploaded.", + "findings": [], + }) + with open("/tmp/panel.json", "w", encoding="utf-8") as f: json.dump(panel, f, indent=2) @@ -662,16 +760,13 @@ jobs: env: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} run: | - run_judge() { - cursor-agent \ - --print \ - --trust \ - --model "$JUDGE_MODEL" \ - < "$1" \ - > /tmp/judge-raw.txt 2>/tmp/judge-stderr.txt || true - } - - run_judge /tmp/judge-prompt.txt + cursor-agent \ + --print \ + --trust \ + --approve-mcps \ + --model "$JUDGE_MODEL" \ + < /tmp/judge-prompt.txt \ + > /tmp/judge-raw.txt 2>/tmp/judge-stderr.txt || true echo "=== Judge raw output (first 1000 chars) ===" head -c 1000 /tmp/judge-raw.txt @@ -679,68 +774,34 @@ jobs: echo "=== Judge stderr ===" cat /tmp/judge-stderr.txt - # LLMs intermittently wrap the findings array in prose. extract-findings.py - # recovers most of that (fenced block / embedded brace-match), but if it - # still can't parse, retry the judge ONCE with a terse strict-JSON - # reminder appended — a single retry clears the large majority of these - # transient parse failures. The retry overwrites /tmp/judge-raw.txt, so - # the parse step below reads whichever attempt the judge last produced. - python3 "$CURSOR_REVIEW_ASSETS/extract-findings.py" \ - --raw /tmp/judge-raw.txt \ - --out /tmp/judge-probe.json \ - --model "$JUDGE_MODEL" \ - --review-type judge - PROBE_STATUS=$(python3 -c "import json; print(json.load(open('/tmp/judge-probe.json')).get('status','?'))") - - if [ "$PROBE_STATUS" != "ok" ]; then - echo "Judge output did not parse (status=$PROBE_STATUS) — retrying once with a strict-JSON reminder." - { - cat /tmp/judge-prompt.txt - echo "" - echo "=== REMINDER ===" - echo "Your previous response could not be parsed as JSON. Return ONLY the JSON array of findings — no prose, no explanation, no markdown code fences. Your entire response must be a single JSON array (return [] if there are no findings). You have no shell, filesystem, or web tools here, so do not narrate attempts to use them or your verification reasoning; emit only the array." - } > /tmp/judge-prompt-retry.txt - run_judge /tmp/judge-prompt-retry.txt - echo "=== Judge retry raw output (first 1000 chars) ===" - head -c 1000 /tmp/judge-raw.txt - echo "" - echo "=== Judge retry stderr ===" - cat /tmp/judge-stderr.txt - fi + echo "=== Structured final review ===" + cat /tmp/judge-findings.json + echo "" - name: Build consolidated findings file id: consolidated env: OK_COUNT: ${{ steps.aggregate.outputs.ok_count }} run: | - # If at least one cell produced findings, parse the judge output - # into a findings file. Otherwise short-circuit — post-review.py - # will render a "no contributors" review with the panel summary. + # If no cells completed, short-circuit — post-review.py will render + # a "no contributors" review with the panel summary. Otherwise the + # judge's MCP tool has already written /tmp/judge-findings.json. if [ "$OK_COUNT" = "0" ]; then - echo "No panel cells contributed findings — skipping judge parse." + echo "No panel cells contributed findings — skipping judge." echo '{"status": "ok", "findings": []}' > /tmp/judge-findings.json - else - python3 "$CURSOR_REVIEW_ASSETS/extract-findings.py" \ - --raw /tmp/judge-raw.txt \ - --out /tmp/judge-findings.json \ - --model "$JUDGE_MODEL" \ - --review-type judge fi - # If the judge errored or its output couldn't be parsed, surface - # that to the next step so we post an honest error review rather - # than silently claiming "no findings" when the panel did produce - # content. + # If the judge did not call its final-review tool, surface that to + # the next step rather than silently claiming "no findings". JUDGE_STATUS=$(python3 -c "import json,sys; print(json.load(open('/tmp/judge-findings.json')).get('status','?'))") echo "Judge status: $JUDGE_STATUS" echo "judge_status=$JUDGE_STATUS" >> "$GITHUB_OUTPUT" # Combine judge findings with panel metadata (model, review_type, # status only — drop full per-cell findings to keep the post payload - # small) for the post-review script. If the judge FAILED (still no - # parseable verdict after the retry), fall back to the raw per-cell - # findings so the run surfaces what the panel found instead of a bare - # "Review failed". + # small) for the post-review script. If the judge did not submit, + # fall back to the raw per-cell findings so the run still surfaces + # what the panel found. python3 - <<'PY' import json, os @@ -851,7 +912,7 @@ jobs: --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ - --notice "⚠️ The judge step could not produce a parseable verdict after a retry (status=${JUDGE_STATUS}). The findings below are the raw, un-adjudicated panel output — they may contain duplicates or false positives the judge would normally filter." + --notice "⚠️ The judge step did not submit a final review through its structured-output tool (status=${JUDGE_STATUS}). The findings below are the raw, un-adjudicated panel output — they may contain duplicates or false positives the judge would normally filter." else JUDGE_ERROR=$(python3 -c "import json; print(json.load(open('/tmp/judge-findings.json')).get('error','(no error message)'))") python3 "$CURSOR_REVIEW_ASSETS/post-review.py" \ diff --git a/.github/workflows/test-cursor-review-scripts.yml b/.github/workflows/test-cursor-review-scripts.yml index fde5e6e..3714f00 100644 --- a/.github/workflows/test-cursor-review-scripts.yml +++ b/.github/workflows/test-cursor-review-scripts.yml @@ -1,9 +1,9 @@ name: Test cursor-review scripts -# Runs the Python unit tests for the cursor-review helper scripts (the -# extract-findings.py JSON parser in particular). These scripts drive the -# review panel + judge consolidation, so a parser regression silently breaks -# every consumer repo's review — cheap to guard with a unit run on change. +# Runs the Python unit tests for the cursor-review helper scripts, especially +# the stdio MCP output contract used by reviewers and the judge. These scripts +# drive panel consolidation for every consumer repo, so they are cheap to +# guard with a unit run on change. on: pull_request: From 72d4c3fba99218937dac836f8621350c8df01308 Mon Sep 17 00:00:00 2001 From: Hunter Senft-Grupp Date: Sat, 18 Jul 2026 00:51:30 +0000 Subject: [PATCH 2/4] fix(cursor-review): address structured output review feedback --- .github/cursor-review/review-output-mcp.py | 2 +- .../tests/test_review_output_mcp.py | 19 ++++++++++++++++--- .github/workflows/cursor-review.yml | 6 ++++-- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/.github/cursor-review/review-output-mcp.py b/.github/cursor-review/review-output-mcp.py index 15a631c..6184c1a 100644 --- a/.github/cursor-review/review-output-mcp.py +++ b/.github/cursor-review/review-output-mcp.py @@ -225,7 +225,7 @@ def handle(args, message): try: if not isinstance(params, dict): raise ValueError("params must be an object") - text = call_tool(args, params.get("name"), params.get("arguments") or {}) + text = call_tool(args, params.get("name"), params.get("arguments", {})) return result(request_id, { "content": [{"type": "text", "text": text}], "isError": False, diff --git a/.github/cursor-review/tests/test_review_output_mcp.py b/.github/cursor-review/tests/test_review_output_mcp.py index 59995cf..f0774ff 100644 --- a/.github/cursor-review/tests/test_review_output_mcp.py +++ b/.github/cursor-review/tests/test_review_output_mcp.py @@ -38,7 +38,7 @@ def test_rejects_unknown_fields(self): class StdioServerTest(unittest.TestCase): - def run_server(self, mode, messages): + def run_server(self, mode, messages, include_initial=False): with tempfile.TemporaryDirectory() as directory: output_path = os.path.join(directory, "findings.json") subprocess.run( @@ -46,6 +46,8 @@ def run_server(self, mode, messages): "--model", "test-model", "--review-type", mode, "--init"], check=True, ) + with open(output_path, encoding="utf-8") as source: + initial_record = json.load(source) process = subprocess.run( [sys.executable, MODULE_PATH, "--mode", mode, "--out", output_path, "--model", "test-model", "--review-type", mode], @@ -57,6 +59,8 @@ def run_server(self, mode, messages): with open(output_path, encoding="utf-8") as source: record = json.load(source) responses = [json.loads(line) for line in process.stdout.splitlines()] + if include_initial: + return record, responses, initial_record return record, responses def test_reviewer_records_findings_and_finishes(self): @@ -109,10 +113,19 @@ def test_judge_submits_empty_final_review(self): self.assertEqual(record["findings"], []) def test_invalid_tool_input_does_not_replace_output(self): - record, responses = self.run_server("judge", [ + record, responses, initial_record = self.run_server("judge", [ {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { "name": "cursor_review_submit_final", - "arguments": {"findings": [{**FINDING, "line": 0}]}, + "arguments": {"findings": [FINDING, {**FINDING, "line": 0}]}, + }}, + ], include_initial=True) + self.assertEqual(record, initial_record) + self.assertTrue(responses[0]["result"]["isError"]) + + def test_rejects_falsy_non_object_tool_arguments(self): + record, responses = self.run_server("reviewer", [ + {"jsonrpc": "2.0", "id": 1, "method": "tools/call", "params": { + "name": "cursor_review_finish", "arguments": [], }}, ]) self.assertEqual(record["status"], "error") diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 51c2d8f..45c2728 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -504,7 +504,8 @@ jobs: --review-type "$REVIEW_TYPE" \ --init - mkdir -p .cursor + rm -rf -- .cursor + mkdir -m 700 .cursor python3 - <<'PY' import json, os, sys, tempfile @@ -656,7 +657,8 @@ jobs: --review-type judge \ --init - mkdir -p .cursor + rm -rf -- .cursor + mkdir -m 700 .cursor python3 - <<'PY' import json, os, sys, tempfile From eb24bfcc6ec25437a16dd3737afd1f0790bf358b Mon Sep 17 00:00:00 2001 From: Hunter Senft-Grupp Date: Sat, 18 Jul 2026 00:52:54 +0000 Subject: [PATCH 3/4] fix(cursor-review): integrate model preflight after rebase Amp-Thread-ID: https://ampcode.com/threads/T-019f7201-5122-7674-9389-5610ebeb5d9d Co-authored-by: Amp --- .github/workflows/cursor-review.yml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 45c2728..e5e5938 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -163,7 +163,7 @@ jobs: run: | # PRs from forks can't run this review. The pull_request event grants # fork PRs no access to secrets — so CURSOR_API_KEY is empty and every - # panel cell produces empty output — and a read-only GITHUB_TOKEN, so + # panel cell fails before submitting output — and a read-only GITHUB_TOKEN, so # posting the consolidated review returns HTTP 403. The review can # neither analyze nor post, so skip cleanly instead of burning the # 8-cell matrix + judge and failing red on every external contribution. @@ -292,11 +292,9 @@ jobs: 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 + # per-cell `cursor-agent --model ` call cannot submit structured output. + # Failing here turns that drift into a loud, visible job failure before the + # fan-out starts. 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 @@ -443,7 +441,7 @@ jobs: "model": os.environ["MODEL"], "review_type": os.environ["REVIEW_TYPE"], "status": "error", - "error": "Cell did not reach the extraction step.", + "error": "Cell did not complete structured output submission.", "findings": [], } with open("/tmp/findings-out/findings.json", "w", encoding="utf-8") as f: From 364e9a3089d86d1793d6204aee53be2dbd83e2e7 Mon Sep 17 00:00:00 2001 From: Hunter Senft-Grupp Date: Tue, 21 Jul 2026 18:00:32 +0000 Subject: [PATCH 4/4] fix(cursor-review): harden structured output failures Amp-Thread-ID: https://ampcode.com/threads/T-019f7201-5122-7674-9389-5610ebeb5d9d Co-authored-by: Amp --- .github/cursor-review/review-output-mcp.py | 7 ++--- .../tests/test_review_output_mcp.py | 27 +++++++++++++++++++ .github/workflows/cursor-review.yml | 13 ++++++--- .../workflows/test-cursor-review-scripts.yml | 2 ++ 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/.github/cursor-review/review-output-mcp.py b/.github/cursor-review/review-output-mcp.py index 6184c1a..1a6a25b 100644 --- a/.github/cursor-review/review-output-mcp.py +++ b/.github/cursor-review/review-output-mcp.py @@ -63,7 +63,8 @@ def initial_record(args): def read_record(args): try: with open(args.out, encoding="utf-8") as source: - return json.load(source) + record = json.load(source) + return record if isinstance(record, dict) else initial_record(args) except (OSError, json.JSONDecodeError): return initial_record(args) @@ -230,7 +231,7 @@ def handle(args, message): "content": [{"type": "text", "text": text}], "isError": False, }) - except (KeyError, TypeError, ValueError) as exc: + except (KeyError, TypeError, ValueError, OSError) as exc: return result(request_id, { "content": [{"type": "text", "text": str(exc)}], "isError": True, @@ -247,7 +248,7 @@ def serve(args): if not isinstance(message, dict): raise ValueError("request must be an object") response = handle(args, message) - except (json.JSONDecodeError, ValueError) as exc: + except (json.JSONDecodeError, ValueError, OSError) as exc: response = error(None, -32700, str(exc)) if response is not None: print(json.dumps(response, separators=(",", ":")), flush=True) diff --git a/.github/cursor-review/tests/test_review_output_mcp.py b/.github/cursor-review/tests/test_review_output_mcp.py index f0774ff..16c9a66 100644 --- a/.github/cursor-review/tests/test_review_output_mcp.py +++ b/.github/cursor-review/tests/test_review_output_mcp.py @@ -7,7 +7,9 @@ import subprocess import sys import tempfile +import types import unittest +from unittest import mock MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "review-output-mcp.py") SPEC = importlib.util.spec_from_file_location("review_output_mcp", MODULE_PATH) @@ -36,6 +38,31 @@ def test_rejects_unknown_fields(self): with self.assertRaisesRegex(ValueError, "keys must be exactly"): MCP.validate_finding({**FINDING, "confidence": 0.9}) + def test_non_object_record_falls_back_to_initial_state(self): + with tempfile.TemporaryDirectory() as directory: + output_path = os.path.join(directory, "findings.json") + with open(output_path, "w", encoding="utf-8") as output: + json.dump([], output) + args = types.SimpleNamespace( + out=output_path, + model="test-model", + review_type="reviewer", + ) + self.assertEqual(MCP.read_record(args), MCP.initial_record(args)) + + def test_tool_io_error_returns_json_rpc_error(self): + args = types.SimpleNamespace(mode="reviewer") + message = { + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": {"name": "cursor_review_finish", "arguments": {}}, + } + with mock.patch.object(MCP, "call_tool", side_effect=OSError("disk full")): + response = MCP.handle(args, message) + self.assertTrue(response["result"]["isError"]) + self.assertEqual(response["result"]["content"][0]["text"], "disk full") + class StdioServerTest(unittest.TestCase): def run_server(self, mode, messages, include_initial=False): diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index e5e5938..bb150af 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -705,8 +705,13 @@ jobs: panel = [] for path in sorted(glob.glob("/tmp/panel/findings-*/findings.json")): - with open(path, encoding="utf-8") as f: - panel.append(json.load(f)) + try: + with open(path, encoding="utf-8") as f: + cell = json.load(f) + except (OSError, json.JSONDecodeError): + continue + if isinstance(cell, dict): + panel.append(cell) expected = { (model, review_type) @@ -766,7 +771,9 @@ jobs: --approve-mcps \ --model "$JUDGE_MODEL" \ < /tmp/judge-prompt.txt \ - > /tmp/judge-raw.txt 2>/tmp/judge-stderr.txt || true + > /tmp/judge-raw.txt 2>/tmp/judge-stderr.txt \ + && JUDGE_EXIT=0 || JUDGE_EXIT=$? + echo "cursor-agent (judge) exit code: $JUDGE_EXIT" echo "=== Judge raw output (first 1000 chars) ===" head -c 1000 /tmp/judge-raw.txt diff --git a/.github/workflows/test-cursor-review-scripts.yml b/.github/workflows/test-cursor-review-scripts.yml index 3714f00..54f25bb 100644 --- a/.github/workflows/test-cursor-review-scripts.yml +++ b/.github/workflows/test-cursor-review-scripts.yml @@ -4,6 +4,8 @@ name: Test cursor-review scripts # the stdio MCP output contract used by reviewers and the judge. These scripts # drive panel consolidation for every consumer repo, so they are cheap to # guard with a unit run on change. +# Inputs/secrets: none. Triggers: path-filtered pull requests and pushes to main. +# Caller pattern: none; this is repository-local CI, not a reusable workflow. on: pull_request: