fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown - #279
fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown#279mattmillerai wants to merge 1 commit into
Conversation
… judge was shown The judge's `repeat_of` was only ever SHAPE-checked on the writing side: any well-formed GitHub discussion permalink was rendered as a re-raise trailer, spent a REPEAT_CAP slot and travelled structurally through the body-only sentinel. The reader's `_resolve_lineage` catches a fabricated id, but it cannot catch an id that is genuinely one of ours and was nevertheless never SHOWN to the judge — a thread aged past the round cap, dropped by the ledger's byte cap, or never rendered at all. That is exactly the id the judge has no basis to cite. The `Post review` job already downloads the ledger artifact, and its ledger.json is the post-cap entry list the judge prompt was rendered from, so the check needs no new artifact and no new job output. post-review.py now loads the set of thread URLs that ledger carried — each entry's `discussion_url`, plus the `repeat_of` lineage a demoted re-raise entry renders on its `re_raise_of:` line — and drops a `repeat_of` that names none of them. Both lineage fields come off one repeat_url_of call, so a dropped URL yields neither the trailer nor the sentinel key and consumes no cap slot; dropping only one of the two would have reopened the cap-free chain. An unreadable ledger degrades to today's shape-only behaviour and says so on stderr: a guard that cannot read its input must not report an empty set. An `entries: []` ledger is a real EMPTY set, not that case — the judge prompt permits repeat_of only inside a ledger block, so on an empty/disabled/unknown ledger every repeat_of is unfounded. All three post-review.py invocations pass --ledger. The step deliberately does not gate on the download's outcome. build-ledger.py is untouched: the two layers are independent, not each other's backstop.
mattmillerai
added
cursor-review
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 102 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
| # ONE repeat_url_of call decides both lineage fields below, so a URL the | ||
| # ledger never showed the judge produces neither the trailer nor the | ||
| # sentinel key — and therefore consumes no REPEAT_CAP slot either. | ||
| repeat_url = repeat_url_of(finding, shown_repeat_urls) |
There was a problem hiding this comment.
🟠 High — On a membership miss repeat_url_of returns "" but the finding is still kept, and enforce_repeat_cap counts only a truthy repeat trailer — so the finding silently becomes an ordinary, cap-exempt one. A judge that cites a non-member URL on every re-raise therefore gets unlimited re-litigation with no ↩︎ re-raise of disclosure to the reader and no lineage recorded next round, which is the opposite of the README's claim that a dropped re-raise "cannot buy itself the cap exemption". Either reject such a finding outright or keep a separate rejected-repeat marker for cap accounting.
Raised by 3 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| --commit-sha "$HEAD_SHA" \ | ||
| --triggered-by "$TRIGGERED_BY" \ | ||
| --diff /tmp/pr-diff.patch \ | ||
| --ledger /tmp/ledger/ledger.json \ |
There was a problem hiding this comment.
🟡 Medium — This path is the POST job's own ledger download, but whether the JUDGE prompt received a ledger is tracked separately in LEDGER_DOWNLOAD_JUDGE / JUDGE_LEDGER_OK (lines 2359-2368) and used only for the header banner. When the judge's continue-on-error download failed while the poster's succeeded, the judge was spliced an empty ledger block yet every URL in ledger.json still counts as "shown" — so the guard's stated premise ("the very ledger the judge prompt was rendered from") is false on exactly the degraded run this workflow already detects. Gate the flag on JUDGE_LEDGER_OK and pass an empty shown set otherwise.
Raised by 5 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k3-high edge-case).
| file=sys.stderr, | ||
| ) | ||
| return "" | ||
| return neutralize_mentions(url) |
There was a problem hiding this comment.
🟡 Medium — repeat_url_of does isinstance/strip/membership plus neutralize_mentions and nothing else — REPEAT_URL_RE and REPEAT_URL_MAX_CHARS are applied only in render_body_only_sentinel — so the "degrades to shape-only" fallback this docstring and the README promise is in fact no shape validation at all on the trailer path. With shown_repeat_urls is None (missing or unreadable ledger, whose download is continue-on-error), an arbitrary judge string such as not a url is rendered verbatim after ↩︎ re-raise of in a bot-authored comment and consumes a REPEAT_CAP slot, and since the trailer is not length-clamped a long one can 422 the whole inline payload. It is also the trailer/sentinel disagreement render_repeat_trailer's docstring says must never happen.
Raised by 4 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| def test_none_reproduces_todays_shape_only_behaviour_exactly(self): | ||
| """The default every existing caller still gets. Run the same fixtures through | ||
| both signatures and demand equality — including a malformed `repeat_of`, which | ||
| the shape guard must still reject on its own.""" |
There was a problem hiding this comment.
🟢 Low — test_none_reproduces_todays_shape_only_behaviour_exactly only asserts normalize_comments([raw], None) == normalize_comments([raw]), and None is the parameter default, so both sides take the same path and the assertion is vacuously true. The "not a url" and evil.example.com fixtures are therefore never checked for rejection despite the docstring claiming "the shape guard must still reject on its own"; assert on the actual output (no trailer, repeat_url == "") and the missing guard surfaces as a failure.
Raised by 3 of 6 reviewers (claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial, claude-opus-5-thinking-max edge-case).
| # the one non-ValueError json.load raises on input it cannot parse (deep nesting) | ||
| # and is caught by name rather than by a bare `except`, which would also swallow | ||
| # a KeyboardInterrupt or a genuine bug in this module. | ||
| except (OSError, ValueError, RecursionError) as e: |
There was a problem hiding this comment.
🟢 Low — When the ledger cannot be read the membership layer deactivates with only a stderr line, and the workflow's ledger_download is continue-on-error, so a transient artifact failure makes an unprotected run indistinguishable from a protected one. Emit a ::warning:: annotation or fold the degradation into the existing --ledger-note banner so a silently disabled guard is visible.
Raised by 1 of 6 reviewers (kimi-k3-high adversarial).
|
|
||
|
|
||
| def render_repeat_of(finding: dict) -> str: | ||
| def render_repeat_of( |
There was a problem hiding this comment.
🟢 Low — render_repeat_of gains a shown_repeat_urls parameter but loses its last caller in the same change: normalize_comments now calls repeat_url_of + render_repeat_trailer directly. It is also precisely the two-call shape render_repeat_trailer's docstring exists to prevent, so a future caller reintroduces the double membership log and the trailer/repeat_url desync — delete it or make it the single entry point again.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
| for entry in data["entries"]: | ||
| if not isinstance(entry, dict): | ||
| continue | ||
| for key in ("discussion_url", "repeat_of"): |
There was a problem hiding this comment.
⚪ Nit — The loader adds every entry's discussion_url and repeat_of regardless of the ledger's status or the entry's anchored flag, while build-ledger.py renders an empty block for any non-ok status and shows re_raise_of: only when the entry is not anchored. Latent today because unknown/empty/disabled ledgers hard-code entries: [], but mirroring both gates here would keep the shown set equal to what was actually rendered, as this function's docstring claims.
Raised by 2 of 6 reviewers (kimi-k3-high edge-case, claude-opus-5-thinking-max edge-case).
2 participants
ELI-5
The judge is shown a list of earlier findings on this PR (the "prior-review ledger") and may re-raise one of them by quoting that thread's link. Until now we only checked the link looked right. Now we also check it is actually one of the links the judge was shown. A link it was never shown gets dropped — no re-raise line, nothing carried forward, and no re-raise budget spent on it.
What changed
.github/cursor-review/post-review.pyload_shown_repeat_urls(path) -> frozenset[str] | None. Reads the ledger job'sledger.json— the post-cap entry list the judge prompt was rendered from — and returns every thread URL it carried: each entry'sdiscussion_url, plus therepeat_oflineage key a demoted re-raise entry renders on itsre_raise_of:line. Non-string / empty values are skipped; values are.strip()ed.entrieslist →None(degrade to today's shape-only behaviour) with one stderr line saying why. It never raises — an exception here would cost the whole review over a file that is deliberately optional.entries: []→ a real empty frozenset, notNone.empty/disabled/unknownledgers all carryentries: [], andprompt-judge.mdpermitsrepeat_ofonly inside a ledger block ("Emit those two fields on no other finding"), so on those runs the judge was shown no thread and everyrepeat_ofis unfounded.repeat_url_of(finding, shown_repeat_urls=None)drops a URL absent from the set (one stderr line) after the existing type/strip step, comparing the stripped, pre-neutralize string.render_repeat_of(...)/normalize_comments(...)thread the parameter through, keyword-defaulted so every existing caller and test is unchanged. Bothrepeat_of(the trailerenforce_repeat_capcounts) andrepeat_url(the sentinel key) now come off onerepeat_url_ofcall via a newrender_repeat_trailerhelper, so a dropped URL produces neither and spends noREPEAT_CAPslot. Dropping only one of the two would have reopened the cap-free chain this is meant to close.main()gains--ledger(defaultNone) and calls the loader once, feeding the singlenormalize_commentscall..github/workflows/cursor-review.yml—--ledger /tmp/ledger/ledger.jsonon all threepost-review.pyinvocations (judge-ok, raw-panel--notice, error). No new download step:ledger_downloadalready places the file there, and the step deliberately does not gate on that download's outcome — the loader degrades.Docs — module docstring and the README's repeat-policy section now state the two-layer contract: writer = shape + membership in the shown set; reader =
build-ledger.py's_resolve_lineageroot check. Independent layers, not each other's backstop.build-ledger.pyis untouched.Why membership, and why not a heuristic
A membership check cannot catch "a real root of ours belonging to a different finding" — that root is itself in the shown set. It is not meant to: it catches only resolvable-but-not-shown ids (aged past
MAX_ROUNDS, dropped by the byte cap, or never rendered). A path/relation heuristic was deliberately not added — a false negative there would make the re-raise cap-exempt, which is the hole this closes.Falsification of the capability this denies
This change denies a capability (a
repeat_ofnaming an unshown thread), so I went looking for a legitimate URL the judge could be shown that the set would not contain. There are exactly two placesbuild-ledger.pyrenders a permalink into the prompt block, and both are covered:discussion_url: <url>(anchored entries) ←entry["discussion_url"]re_raise_of: <url>andMUST carry repeat_of: <url>(demoted re-raise entries) ←entry["repeat_of"]Verified by grepping every
html_url/url/https://occurrence inbuild-ledger.py— no third source exists. Two adjacent checks: the caps mutateentriesbefore the dict is returned, soledger.jsonis genuinely the post-cap list; and_defang_fencesrewrites only lines opening with a run of=, so a rendered permalink is byte-identical to the string inledger.json. The ledger the panel reads is rendered from the sameentrieslist, which is what makes the flag correct on the raw-panel branch too.Verification
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'— 478 passed, 0 failed (16 new, acrossShownRepeatUrlsTest,RepeatMembershipTest,LedgerFlagWiringTest).--ledgerfrom just the error branch failstest_all_three_invocations_carry_the_ledger_flag; restored afterwards.python3 .github/workflow-pins/check_workflow_pins.py— OK, 11 workflows, 0 exempt.python3 .github/agents-md-integrity/check_agents_md.py --root .— passed (2 pre-existing warnings: AGENTS.md 155 lines, no CODEOWNERS).python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'— 316 passed.bash .github/bump-callers/tests/*.sh(each separately) — 3/3 pass.actionlint .github/workflows/cursor-review.yml— one pre-existingjob.workflow_shaproperty warning at line 548, untouched by this diff.Judgment calls
{url!r}, not the bare URL. The spec wrote…shown: <url>. This is relayed model text reaching a step log that GitHub parses for::workflow-command::at the start of a line, and that log is world-readable on a public repo — a newline in the value must not be able to start a line.repr()keeps it to one line and matches the file's existing{path!r}/{finding!r}prints. A test asserts the message prefix, single-line-ness, and that an embedded\n::add-mask::cannot start a line.normalize_commentssite, not the top ofmain(). The error-review and no-findings paths return earlier and adjudicate norepeat_ofat all, so reading the file there would be work (and a stderr line) with nothing to decide.except (OSError, ValueError, RecursionError)rather than a bareexcept.JSONDecodeErrorandUnicodeDecodeErrorare bothValueError;RecursionErroris the one non-ValueErrorjson.loadraises on input it cannot parse (confirmed locally: 1M-deep nesting raises it, 100k does not). A bareexceptwould also swallowKeyboardInterruptand genuine bugs in the module.Residual
repeat_ofthat happens to name a real shown thread would pass both layers. The blast radius is small (a re-raise citing a genuine thread, costing a cap slot) and the workflow already banners that degradation on the review, so no attempt was made to reconcile the two downloads' outcomes here. Doing so would mean gating the flag onledger_download's outcome, which the spec explicitly rules out.cursor-review.yml; nocursor-reviewworkflow run was executed against a real PR with a multi-round ledger, and none of the fleet's consumer repos were exercised. The first live exercise will be this PR's own review, whose ledger is a first round (entries: []) and therefore only exercises the empty-set branch.Provenance
unittestcursor-review suite: 478 passed, 0 failed; workflow-pins suite: 316 passed;check_workflow_pins.py: OK (11 workflows);check_agents_md.py: passed (2 pre-existing warnings); bump-callers shell tests: 3/3 pass;actionlint: no new findingsrepr()'d rather than bare (rationale above); no other acceptance criterion was skipped