Skip to content

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

Merged
mattmillerai merged 1 commit into
mainfrom
matt/be-3160-judge-parse-hardening
Jul 17, 2026
Merged

fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160)#31
mattmillerai merged 1 commit into
mainfrom
matt/be-3160-judge-parse-hardening

Conversation

@mattmillerai

Copy link
Copy Markdown
Contributor

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.

…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.
@mattmillerai mattmillerai added agent-coded Authored by the agent-work loop cursor-review Multi-model cursor review labels Jul 16, 2026
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 53 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 444a44f9-97fd-422b-909d-90ed1abe97f1

📥 Commits

Reviewing files that changed from the base of the PR and between 3ea5dce and 8449eb2.

📒 Files selected for processing (4)
  • .github/cursor-review/extract-findings.py
  • .github/cursor-review/prompt-judge.md
  • .github/cursor-review/tests/test_extract_findings.py
  • .github/workflows/cursor-review.yml
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3160-judge-parse-hardening
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3160-judge-parse-hardening

Comment @coderabbitai help to get the list of available commands.

@mattmillerai
mattmillerai merged commit 3aead23 into main Jul 17, 2026
4 checks passed
mattmillerai added a commit that referenced this pull request Jul 27, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded Authored by the agent-work loop cursor-review Multi-model cursor review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants