[PR #17] [MERGED] fix(cursor-review): harden judge against parse_error (lenient extraction + retry + panel fallback) (BE-1916) #27

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/17
Author: @mattmillerai
Created: 6/29/2026
Status: ✅ Merged
Merged: 6/29/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-1916-harden-judge-parse

📝 Commits (1)

6d4a388 fix(cursor-review): harden judge against parse_error (lenient extraction + retry + panel fallback) (BE-1916)

📊 Changes

6 files changed (+394 additions, -49 deletions)

View changed files

📝 .github/cursor-review/extract-findings.py (+101 -32)
📝 .github/cursor-review/post-review.py (+9 -0)
📝 .github/cursor-review/prompt-judge.md (+5 -0)
➕ .github/cursor-review/tests/test_extract_findings.py (+148 -0)
📝 .github/workflows/cursor-review.yml (+93 -17)
➕ .github/workflows/test-cursor-review-scripts.yml (+38 -0)

📄 Description

ELI-5

Sometimes the "judge" model that combines all the reviewers' notes answers in
plain English ("Based on my review, I think…") and then pastes the JSON we
need. Our old parser only knew a couple of rigid ways to find that JSON, so it
choked on the prose, threw everything away, and the whole PR review showed up as
a scary "Review failed" — even when the judge's answer was actually correct
(this happened on ComfyUI #487 and Alex's PR). This PR teaches the pipeline to
(1) dig the JSON out of the prose, (2) politely ask the judge once more if it
still can't, and (3) if even that fails, show the raw reviewer notes instead of
a blank failure.

What changed

Three layers, cheapest first, all in the cursor-review judge path:

Lenient extraction (extract-findings.py) — parse_json_findings now
recovers JSON wrapped in prose via a string/escape-aware balanced
brace-scan for the first {…}/[…] region that actually parses, instead
of the old naive first-[/last-] slice (which broke the moment the prose
contained a stray bracket — exactly the #487 shape). New coerce_findings_list
also unwraps a {"findings": […]}-style object. The fast path (clean JSON)
and the fenced-json path are unchanged.
Retry once (cursor-review.yml → Run judge) — after the judge runs we
probe-parse its output; on a hard parse failure we re-run the judge exactly
once with a terse "Return ONLY the JSON array, no prose" reminder appended.
No loop — one retry, then we stop.
Tighten the judge prompt (prompt-judge.md) — a bare-JSON-array demand is
now the first line of the prompt.

Plus the degraded fallback: if the judge still can't be parsed after the
retry and the panel produced findings, we post the raw, de-duplicated per-cell
findings under a warning banner — never a bare "Review failed" when there
are real findings to show. A genuine total failure (no panel findings either)
still surfaces the honest error review.

Acceptance criteria

Prose-wrapped valid JSON is parsed successfully, not failed — parse_json_findings + balanced scan; regression-tested.
A malformed judge response triggers exactly one retry; only a second failure surfaces as an error, and even then posts the per-cell findings (deduped, with a banner) rather than a bare "Review failed".
Regression test feeding a #487-style prose-wrapped-JSON sample through the parser — .github/cursor-review/tests/test_extract_findings.py (test_prose_wrapped_array_487 + 13 more).

Testing

python3 -m unittest discover -s .github/cursor-review/tests → 14 tests, all green. New CI workflow test-cursor-review-scripts.yml runs them on any change under .github/cursor-review/**.
Simulated the consolidate step's three branches locally (judge-ok / judge-failed-with-panel-findings / no-contributors) and dry-ran post-review.py --notice against a stub gh: the degraded path posts a real review with inline comments + banner, not "Review failed".

Judgment calls / not done

JSON / structured-output mode (plan item 3, second half) — intentionally NOT
enabled. The judge is invoked through the cursor-agent CLI, which has no
reliable per-model knob to force the findings array shape; --output-format json wraps stdout in cursor-agent's own envelope, which would break the
existing parser. Flipping it blind is a net regression risk, so I relied on the
prompt tightening + the strict-JSON retry (which is structured-output pressure
at the prompt level) + the lenient parser. The acceptance criteria are met
without it. Happy to revisit if a confirmed JSON-mode flag exists.
Brace-scan first-match limitation: if the judge's prose contains a
standalone valid JSON array of scalars before the real findings array, the
scanner could pick the wrong one. This is contrived (covered case: prose with a
non-JSON […] recovers correctly, tested), strictly better than the old
behavior, and backstopped by the retry + prompt. Not over-engineered.

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

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/17 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 6/29/2026 **Status:** ✅ Merged **Merged:** 6/29/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-1916-harden-judge-parse` --- ### 📝 Commits (1) - [`6d4a388`](https://github.com/Comfy-Org/github-workflows/commit/6d4a388550bb6effbaabbee56bee72357acc7e5d) fix(cursor-review): harden judge against parse_error (lenient extraction + retry + panel fallback) (BE-1916) ### 📊 Changes **6 files changed** (+394 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/extract-findings.py` (+101 -32) 📝 `.github/cursor-review/post-review.py` (+9 -0) 📝 `.github/cursor-review/prompt-judge.md` (+5 -0) ➕ `.github/cursor-review/tests/test_extract_findings.py` (+148 -0) 📝 `.github/workflows/cursor-review.yml` (+93 -17) ➕ `.github/workflows/test-cursor-review-scripts.yml` (+38 -0) </details> ### 📄 Description ## ELI-5 Sometimes the "judge" model that combines all the reviewers' notes answers in plain English ("Based on my review, I think…") **and then** pastes the JSON we need. Our old parser only knew a couple of rigid ways to find that JSON, so it choked on the prose, threw everything away, and the whole PR review showed up as a scary **"Review failed"** — even when the judge's answer was actually correct (this happened on ComfyUI #487 and Alex's PR). This PR teaches the pipeline to (1) dig the JSON out of the prose, (2) politely ask the judge once more if it still can't, and (3) if even that fails, show the raw reviewer notes instead of a blank failure. ## What changed Three layers, cheapest first, all in the cursor-review judge path: 1. **Lenient extraction** (`extract-findings.py`) — `parse_json_findings` now recovers JSON wrapped in prose via a **string/escape-aware balanced brace-scan** for the first `{…}`/`[…]` region that actually parses, instead of the old naive first-`[`/last-`]` slice (which broke the moment the prose contained a stray bracket — exactly the #487 shape). New `coerce_findings_list` also unwraps a `{"findings": […]}`-style object. The fast path (clean JSON) and the fenced-```json``` path are unchanged. 2. **Retry once** (`cursor-review.yml` → *Run judge*) — after the judge runs we probe-parse its output; on a hard parse failure we re-run the judge **exactly once** with a terse "Return ONLY the JSON array, no prose" reminder appended. No loop — one retry, then we stop. 3. **Tighten the judge prompt** (`prompt-judge.md`) — a bare-JSON-array demand is now the first line of the prompt. Plus the **degraded fallback**: if the judge still can't be parsed after the retry *and* the panel produced findings, we post the raw, de-duplicated per-cell findings under a warning banner — **never a bare "Review failed"** when there are real findings to show. A genuine total failure (no panel findings either) still surfaces the honest error review. ## Acceptance criteria - [x] Prose-wrapped valid JSON is parsed successfully, not failed — `parse_json_findings` + balanced scan; regression-tested. - [x] A malformed judge response triggers **exactly one** retry; only a second failure surfaces as an error, and even then posts the **per-cell findings** (deduped, with a banner) rather than a bare "Review failed". - [x] Regression test feeding a #487-style prose-wrapped-JSON sample through the parser — `.github/cursor-review/tests/test_extract_findings.py` (`test_prose_wrapped_array_487` + 13 more). ## Testing - `python3 -m unittest discover -s .github/cursor-review/tests` → **14 tests, all green**. New CI workflow `test-cursor-review-scripts.yml` runs them on any change under `.github/cursor-review/**`. - Simulated the consolidate step's three branches locally (judge-ok / judge-failed-with-panel-findings / no-contributors) and dry-ran `post-review.py --notice` against a stub `gh`: the degraded path posts a real review with inline comments + banner, not "Review failed". ## Judgment calls / not done - **JSON / structured-output mode (plan item 3, second half) — intentionally NOT enabled.** The judge is invoked through the `cursor-agent` CLI, which has no reliable per-model knob to force the *findings array* shape; `--output-format json` wraps stdout in cursor-agent's own envelope, which would **break** the existing parser. Flipping it blind is a net regression risk, so I relied on the prompt tightening + the strict-JSON retry (which is structured-output pressure at the prompt level) + the lenient parser. The acceptance criteria are met without it. Happy to revisit if a confirmed JSON-mode flag exists. - **Brace-scan first-match limitation:** if the judge's *prose* contains a standalone valid JSON array of scalars *before* the real findings array, the scanner could pick the wrong one. This is contrived (covered case: prose with a non-JSON `[…]` recovers correctly, tested), strictly better than the old behavior, and backstopped by the retry + prompt. Not over-engineered. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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