fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160) - #31
Conversation
…N region (BE-3160) The judge (claude-opus-4-8-thinking-xhigh) drifts into verification / tool-attempt prose on verification-heavy diffs, emitting the real findings array only AFTER the prose. extract-findings.py returned the FIRST region that merely parsed as JSON and stopped, so an inline finding object quoted mid-reasoning (a dict with no findings key) yielded a spurious parse_error, and an inline scalar list (e.g. jq builtins) yielded bogus findings — the genuine array further down was never reached. On PRs 145/146 this reproduced across a label cycle because the diffs reliably re-trigger the prose mode, so the BE-1916 judge retry re-hit the same extraction bug. - parse_json_findings now scans ALL embedded JSON regions and keeps the LAST findings-shaped one, instead of the first that parses. - coerce_findings_list requires array elements to be objects, so a scalar list quoted in prose no longer masquerades as findings. - prompt-judge.md + the retry reminder tell the judge it has no shell/fs/web tools and must not narrate verification/tool attempts — cutting the drift at the source. Adds regression tests for both the PR-145 (inline object) and PR-146 (scalar list) prose shapes, plus last-block-wins and scalar-list-rejection.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
…ards (BE-4680) Second review round. The panel caught a real regression behind the "phantom blocking input" story, plus errors in my own round-1 prose. The blocking gate was NOT "never declared" — it shipped in #16 (BE-1891) as a `blocking:` input plus a fail-closed "Blocking gate" job, and #31 (a judge-extraction fix) deleted both from cursor-review.yml while leaving gate-unresolved.py and all its documentation behind. So: - .github/cursor-review/README.md still told callers to pass `blocking: true` — a zero-job startup_failure for anyone who copied it. Replaced with a regression note; dropped the `blocking` row from the knobs table (and added the missing `run_without_label` row); marked gate-unresolved.py orphaned in the file inventory. This was the missed call site of this PR's own cleanup. - AGENTS.md: corrected "never declared" to what actually happened, and made the durable lesson "deleting an input is a docs change too — grep for its name in the same commit." Withdrew my round-1 merge-gate advice. Marking `Consolidate panel` required does NOT gate anything: GitHub counts a skipped required check as passing, and that job is if:-gated on five conditions (no trigger label, dedupe hit, over diff_size_cap, fork PR, panel skipped) — so it goes green in exactly the cases where no review ran. Documented that as a trap instead. Also fixed my own false claim that "pushing a commit always re-runs": a push clears the dedupe but delivers no event to a caller on `types: [labeled, unlabeled]`, so you still toggle the label. Dependabot is not covered by the fork guards I added in round 1 — its branches are same-repo, but its runs read the Dependabot secret store, not Actions secrets, so the App-token mint still fails. Added `&& github.actor != 'dependabot[bot]'` to the assign-reviewers and cursor-review-auto-label examples and explained why assign-reviewers' own bot-author skip does not save it (that check runs after the mint has failed). Noted the same hazard for cursor-review under run_without_label/synchronize. SECURITY.md claimed the AI workflows run model steps with no write credentials. True for groom and for cursor-review's 8 panel cells (contents: read), but NOT for the judge: Consolidate panel runs the judge model and posts the review in one pull-requests: write job. Scoped the claim so that boundary reads as in-scope for reports rather than known-and-accepted. assign-prs-to-author.md said "Inputs: None" while the workflow exposes `skip-bots` (default true) — the only supported way to assign bot PRs. Tests: go + all four python suites pass; AGENTS.md gate passes; no broken links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.pyparse_json_findingsnow scans every embedded JSON region and keeps thelast 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_listnow requires array elements to be objects, so ascalar list the judge quotes in prose (e.g.
["contains","startswith"]whilenarrating 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.ymlverification 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.pyarray) 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_findingsreturned the firstlist/dictthatjson.loadsaccepted and stopped. Reproduced against the two captured openings:
{...}then real arrayparse_error(dict had nofindingskey)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
test_verification_prose_then_array_is_ok_be3160(assertsstatus == ok).the failure notice — ✅ the retry (BE-1916,
cursor-review.yml"Run judge")is preserved; genuinely un-parseable output still probes
!= okand retriesonce, now with a stricter no-tools reminder. This change is what makes the
retry effective on the 145/146 class.
⏳ post-merge runtime step, not verifiable from this PR: it requires
consumer repo
mattmillerai/agent-workto pin itsworkflows_refto a refcontaining this change (or
@main) and a label cycle on those PRs. Once thismerges, cycle
cursor-reviewon #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_compileon the script + YAML parse of the workflow: clean.Notes / judgment calls
extract-findings.pyis shared by theper-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_listtightening now rejects a list containing non-objectelements (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.