[PR #31] [MERGED] fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160) #44

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-3160-judge-parse-hardening

📝 Commits (1)

8449eb2 fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160)

📊 Changes

4 files changed (+133 additions, -169 deletions)

View changed files

📝 .github/cursor-review/extract-findings.py (+47 -24)
📝 .github/cursor-review/prompt-judge.md (+6 -0)
📝 .github/cursor-review/tests/test_extract_findings.py (+49 -0)
📝 .github/workflows/cursor-review.yml (+31 -145)

📄 Description

ELI-5

The "judge" AI that combines our 8 code-review opinions into one is supposed to
answer with only a JSON list. On some PRs it instead starts talking first
("Verification changes my adjudication…", "Shell execution isn't available
here…") and puts the real JSON list after that chatter. Our parser grabbed the
first JSON-ish thing it saw and quit — which was a stray object or list from
the chatter — so it declared "couldn't parse" and posted "Review failed"
even though the real answer was sitting right below. This teaches the parser to
keep looking and take the last real findings list, and tells the judge to
stop narrating. Now the review lands instead of failing.

What changed

.github/cursor-review/extract-findings.py

parse_json_findings now scans every embedded JSON region and keeps the
last findings-shaped one, instead of returning the first region that
merely parses. This is the core fix: on verification-heavy diffs the judge
emits prose first and the array last, so "first that parses" grabbed the wrong
thing.
coerce_findings_list now requires array elements to be objects, so a
scalar list the judge quotes in prose (e.g. ["contains","startswith"] while
narrating jq builtins — the PR-146 shape) can't masquerade as findings.

.github/cursor-review/prompt-judge.md + the retry reminder in
.github/workflows/cursor-review.yml

Tell the judge it has no shell/filesystem/web tools and must not narrate
verification or tool attempts — cutting the prose drift at the source. (The
judge's own PR-146 output, "Shell execution isn't available here", is the
evidence the tools genuinely aren't present in this stdin-piped run.)

.github/cursor-review/tests/test_extract_findings.py

Regression tests for the PR-145 (inline finding object in prose, then real
array) and PR-146 (inline scalar list in prose, then real array) shapes,
plus last-block-wins and scalar-list-rejection. Suite: 19 tests, all green.

Root cause

parse_json_findings returned the first list/dict that json.loads
accepted and stopped. Reproduced against the two captured openings:

Prose shape (from ticket)	Old result	New result
PR 145: inline finding object `{...}` then real array	`parse_error` (dict had no `findings` key)	recovers the array
PR 146: inline scalar list then real array	bogus scalar-list findings	recovers the array

The BE-1916 judge retry already existed but didn't help 145/146: those diffs
reliably re-trigger the prose mode, so the retry re-hit the same extraction bug.
With extraction fixed, both the first attempt and the retry now succeed.

Acceptance criteria

Valid JSON after prose consolidates instead of failing — ✅ covered by
test_verification_prose_then_array_is_ok_be3160 (asserts status == ok).
Genuinely malformed output triggers an automatic judge-stage retry before
the failure notice — ✅ the retry (BE-1916, cursor-review.yml "Run judge")
is preserved; genuinely un-parseable output still probes != ok and retries
once, now with a stricter no-tools reminder. This change is what makes the
retry effective on the 145/146 class.
PRs 145/146 re-reviewed successfully once the fix lands (label cycle) —
⏳ post-merge runtime step, not verifiable from this PR: it requires
consumer repo mattmillerai/agent-work to pin its workflows_ref to a ref
containing this change (or @main) and a label cycle on those PRs. Once this
merges, cycle cursor-review on #145/#146 to confirm.

Testing

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'
→ 19 passed (the CI command in test-cursor-review-scripts.yml).
python3 -m py_compile on the script + YAML parse of the workflow: clean.

Notes / judgment calls

Scope of the extraction change: extract-findings.py is shared by the
per-cell panel steps too, not just the judge. The stricter "findings are
arrays of objects" + "last valid wins" behavior is equally correct for cells
(their prompts also demand a JSON array of objects), so this is a net
robustness gain across the board, not a judge-only patch.
coerce_findings_list tightening now rejects a list containing non-object
elements (previously returned as-is). A mixed/scalar list is malformed for
this schema anyway; rejecting it routes to the retry rather than posting
garbage — the intended behavior.

_{🔄 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/31 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/16/2026 **Status:** ✅ Merged **Merged:** 7/17/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-3160-judge-parse-hardening` --- ### 📝 Commits (1) - [`8449eb2`](https://github.com/Comfy-Org/github-workflows/commit/8449eb241d0eb84dfa59e15915fa268ce99abe28) fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160) ### 📊 Changes **4 files changed** (+133 additions, -169 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/extract-findings.py` (+47 -24) 📝 `.github/cursor-review/prompt-judge.md` (+6 -0) 📝 `.github/cursor-review/tests/test_extract_findings.py` (+49 -0) 📝 `.github/workflows/cursor-review.yml` (+31 -145) </details> ### 📄 Description ## ELI-5 The "judge" AI that combines our 8 code-review opinions into one is supposed to answer with **only** a JSON list. On some PRs it instead starts talking first ("Verification changes my adjudication…", "Shell execution isn't available here…") and puts the real JSON list *after* that chatter. Our parser grabbed the **first** JSON-ish thing it saw and quit — which was a stray object or list from the chatter — so it declared "couldn't parse" and posted **"Review failed"** even though the real answer was sitting right below. This teaches the parser to keep looking and take the **last** real findings list, and tells the judge to stop narrating. Now the review lands instead of failing. ## What changed `.github/cursor-review/extract-findings.py` - `parse_json_findings` now scans **every** embedded JSON region and keeps the **last findings-shaped** one, instead of returning the first region that merely parses. This is the core fix: on verification-heavy diffs the judge emits prose first and the array last, so "first that parses" grabbed the wrong thing. - `coerce_findings_list` now requires array elements to be **objects**, so a scalar list the judge quotes in prose (e.g. `["contains","startswith"]` while narrating jq builtins — the PR-146 shape) can't masquerade as findings. `.github/cursor-review/prompt-judge.md` + the retry reminder in `.github/workflows/cursor-review.yml` - Tell the judge it has **no shell/filesystem/web tools** and must not narrate verification or tool attempts — cutting the prose drift at the source. (The judge's own PR-146 output, "Shell execution isn't available here", is the evidence the tools genuinely aren't present in this stdin-piped run.) `.github/cursor-review/tests/test_extract_findings.py` - Regression tests for the PR-145 (inline finding **object** in prose, then real array) and PR-146 (inline **scalar list** in prose, then real array) shapes, plus last-block-wins and scalar-list-rejection. Suite: 19 tests, all green. ## Root cause `parse_json_findings` returned the first `list`/`dict` that `json.loads` accepted and stopped. Reproduced against the two captured openings: | Prose shape (from ticket) | Old result | New result | |---|---|---| | PR 145: inline finding object `{...}` then real array | `parse_error` (dict had no `findings` key) | recovers the array | | PR 146: inline scalar list then real array | bogus scalar-list findings | recovers the array | The BE-1916 judge **retry** already existed but didn't help 145/146: those diffs reliably re-trigger the prose mode, so the retry re-hit the same extraction bug. With extraction fixed, both the first attempt and the retry now succeed. ## Acceptance criteria 1. **Valid JSON after prose consolidates instead of failing** — ✅ covered by `test_verification_prose_then_array_is_ok_be3160` (asserts `status == ok`). 2. **Genuinely malformed output triggers an automatic judge-stage retry before the failure notice** — ✅ the retry (BE-1916, `cursor-review.yml` "Run judge") is preserved; genuinely un-parseable output still probes `!= ok` and retries once, now with a stricter no-tools reminder. This change is what makes the retry *effective* on the 145/146 class. 3. **PRs 145/146 re-reviewed successfully once the fix lands (label cycle)** — ⏳ **post-merge runtime step**, not verifiable from this PR: it requires consumer repo `mattmillerai/agent-work` to pin its `workflows_ref` to a ref containing this change (or `@main`) and a label cycle on those PRs. Once this merges, cycle `cursor-review` on #145/#146 to confirm. ## Testing - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` → **19 passed** (the CI command in `test-cursor-review-scripts.yml`). - `python3 -m py_compile` on the script + YAML parse of the workflow: clean. ## Notes / judgment calls - **Scope of the extraction change:** `extract-findings.py` is shared by the per-cell panel steps too, not just the judge. The stricter "findings are arrays of objects" + "last valid wins" behavior is equally correct for cells (their prompts also demand a JSON array of objects), so this is a net robustness gain across the board, not a judge-only patch. - **`coerce_findings_list` tightening** now rejects a list containing non-object elements (previously returned as-is). A mixed/scalar list is malformed for this schema anyway; rejecting it routes to the retry rather than posting garbage — the intended behavior. --- <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:40 +00:00