From 6186fb0fae8af02204e636cac226e3f0e1262474 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 17:23:16 -0700 Subject: [PATCH 1/3] fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/cursor-review/README.md | 2 + .github/cursor-review/post-review.py | 150 ++++++++- .../cursor-review/tests/test_post_review.py | 308 ++++++++++++++++++ .github/workflows/cursor-review.yml | 3 + 4 files changed, 455 insertions(+), 8 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 4531332..6c38b8a 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -77,6 +77,8 @@ With `ledger_prior_review` on (the default), [`build-ledger.py`](build-ledger.py A finding that could not be anchored to a line the reviewed diff carries is **demoted** to the review body, and one that lost its whole review to a failed POST is delivered as prose — neither gets a thread, so neither can be answered and neither costs a slot. But a demoted finding may itself have been a *re-raise*, and that lineage used to disappear with the trailer that was stripped out of its recovered body: the next round saw a fresh, thread-less, cap-exempt finding, so one demoted hop made every later re-raise of the same finding free. The sentinel now carries the ancestor's URL as a field, and the ledger resolves it against the PR's own comments: the trailing `discussion_r` must name a **root** comment of one of *our* consolidated reviews on this PR, so a hallucinated id resolves to nothing. Only that id is read — the owner/repo/PR-number half of the URL is shape-checked but never compared — so a link naming another repo resolves whenever its id matches one of our roots; what the entry renders is always the ancestor's *own* permalink, never the relayed string, and an ancestor with no permalink yields no lineage. The round is recovered the same way, from the review that ancestor belongs to, rather than from the payload. When it resolves, the entry renders `re_raise_of: (round N; ancestor_answers=)` — a key deliberately distinct from the `answers_from_author_or_maintainer` on the entry's own `thread:` line, since the two describe different findings — plus a `re_raise_answer:` excerpt of the ancestor's most recent answering reply, and the repeat rule is applied to that **ancestor's** answer state: with `k >= 1` a further re-raise must carry `repeat_of` and costs a slot, and with `k == 0` nothing changes. Resolution runs over every consolidated review before the round cap, so the ancestor is still found after its own round has aged out of the ledger. A claim that does *not* resolve — a dismissed review, a deleted comment — is not silently rebuilt as a fresh cap-exempt finding: the entry says the lineage is unverified and withholds the exemption. +A `repeat_of` is adjudicated on two independent layers, and neither is the other's backstop. The **writer** ([`post-review.py`](post-review.py)) checks SHAPE — one anchored GitHub discussion permalink, nothing else — and, given `--ledger`, MEMBERSHIP: the URL must be one the ledger the judge was actually shown carried, i.e. an entry's `discussion_url` or the `repeat_of` lineage a demoted re-raise entry renders on its `re_raise_of:` line. The **reader** ([`build-ledger.py`](build-ledger.py)) separately resolves the trailing comment id against this PR's own consolidated-review roots, as above. Membership catches precisely what resolution cannot: an id that resolves but was never SHOWN — aged past the round cap, dropped by the byte cap, or never rendered — which the judge therefore had no basis to cite. A URL naming no shown entry is dropped whole: no trailer, no sentinel field, and no `REPEAT_CAP` slot spent, so a dropped re-raise cannot buy itself the cap exemption. An `empty`, `disabled` or `unknown` ledger carries `entries: []`, and the judge prompt permits `repeat_of` only inside a ledger block, so on those runs the shown set is legitimately empty and every `repeat_of` is dropped. A ledger that cannot be read at all is the one case that must NOT drop anything — no path, no file, unreadable, not an object, no `entries` list — so it degrades to shape-only, exactly as before, and the reader's half still holds. + ### The panel | Lab | Model (Cursor catalog) | diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 63853e4..707407b 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -24,6 +24,21 @@ Falls back to a body-only review (no inline anchors) if GitHub rejects the inline payload anyway — the API is all-or-nothing, so one bad position costs every anchor in the request. + +A judge `repeat_of` is checked on TWO independent layers, and neither is the +other's backstop. Here — the WRITER — it must have the right SHAPE (one anchored +GitHub discussion permalink, nothing else) and, given `--ledger`, it must also be +a MEMBER of the set of thread URLs carried by the very ledger the judge prompt was +rendered from: `discussion_url` on an anchored entry, plus the `repeat_of` lineage +of a demoted re-raise entry (what its `re_raise_of:` line shows). A URL naming no +shown entry is dropped outright, so it costs no `REPEAT_CAP` slot and travels +neither as the rendered trailer nor as the body-only sentinel field. On the +READER, build-ledger.py's `_resolve_lineage` separately resolves the trailing +comment id against this PR's own consolidated-review roots. Membership deliberately +catches only what resolution cannot: an id that is resolvable but was never SHOWN +(aged past the round cap, dropped by the byte cap, or never rendered). Without +`--ledger`, or with one that cannot be read, this degrades to shape-only and the +reader's half still holds. """ import argparse @@ -1432,7 +1447,9 @@ def build_panel_summary(panel: list[dict]) -> str: return "\n\n".join(parts) -def normalize_comments(findings: list[dict]) -> list[dict]: +def normalize_comments( + findings: list[dict], shown_repeat_urls: frozenset[str] | None = None +) -> list[dict]: """Build sorted, severity-tagged inline comments from raw judge findings. Returns a list of {"severity": str, "comment": dict} entries sorted most @@ -1440,6 +1457,9 @@ def normalize_comments(findings: list[dict]) -> list[dict]: (path/line/side/body) with the severity badge prefixed into the body; severity is kept alongside (not inside) so the summary table can count it without leaking an unknown key into the GitHub API request. + + `shown_repeat_urls` is threaded straight to repeat_url_of; None (the default, + which keeps every existing caller unchanged) means shape-only. """ enriched = [] for finding in findings: @@ -1461,7 +1481,11 @@ def normalize_comments(findings: list[dict]) -> list[dict]: continue severity = normalize_severity(finding.get("severity")) badge = f"{SEVERITY_EMOJI[severity]} **{SEVERITY_LABEL[severity]}** — " - repeat_line = render_repeat_of(finding) + # 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) + repeat_line = render_repeat_trailer(finding, repeat_url) enriched.append( { "severity": severity, @@ -1478,7 +1502,7 @@ def normalize_comments(findings: list[dict]) -> list[dict]: # in. Kept beside `repeat_of` rather than replacing it so # enforce_repeat_cap's count and strip_repeat_line's reconstruction are # both untouched. - "repeat_url": repeat_url_of(finding), + "repeat_url": repeat_url, "comment": { "path": path, "line": line_int, @@ -1499,7 +1523,9 @@ def normalize_comments(findings: list[dict]) -> list[dict]: return enriched -def render_repeat_of(finding: dict) -> str: +def render_repeat_of( + finding: dict, shown_repeat_urls: frozenset[str] | None = None +) -> str: """Render the re-raise line for a finding the judge marked as a repeat. `repeat_of` is the prior round's `discussion_url` from the ledger. Showing @@ -1507,13 +1533,87 @@ def render_repeat_of(finding: dict) -> str: record, linked to the thread that already answered it, so the author can see at a glance that this is round N of the same conversation. """ - url = repeat_url_of(finding) + return render_repeat_trailer(finding, repeat_url_of(finding, shown_repeat_urls)) + + +def render_repeat_trailer(finding: dict, url: str) -> str: + """The trailer for an ALREADY-resolved url, so one call decides both fields. + + normalize_comments stores the rendered trailer and the raw URL side by side and + they must never disagree — a trailer whose URL was dropped would spend a + REPEAT_CAP slot on lineage the sentinel does not carry, and would put a live + thread URL back into the prose. Calling repeat_url_of twice would also log the + membership drop twice. + """ if not url: return "" return f"\n\n↩︎ re-raise of {url}{render_repeat_round(finding)}" -def repeat_url_of(finding: dict) -> str: +def load_shown_repeat_urls(path) -> frozenset[str] | None: + """Every thread URL the ledger at `path` showed the judge — or None. + + None means "do not check membership": no path given, or the file is missing, + unreadable, not JSON, not an object, or carries no `entries` list. That is a + DEGRADATION to the shape-only behaviour this had before, and it is the only + correct answer — a guard that cannot read its input must not report an empty + set, which here would drop every `repeat_of` in the run. + + An EMPTY frozenset is a real answer and is NOT that case. `empty` / `disabled` + / `unknown` ledgers all carry `entries: []`, and prompt-judge.md permits + `repeat_of` only when a PRIOR REVIEW LEDGER block appears ("Emit those two + fields on no other finding"), so on such a run the judge was shown no thread + at all and every `repeat_of` it emits is unfounded. + + Two keys per entry, because two kinds of entry can be re-raised. An anchored + entry renders its own `discussion_url:`. A DEMOTED re-raise has no thread of + its own and renders `re_raise_of: ` instead, out of the entry's + `repeat_of` key (BE-12534) — a resolved ancestor permalink, and the judge is + told to carry it forward, so it is legitimately shown even though no entry + calls it a `discussion_url`. + + Never raises: it runs on the posting path, where an exception would cost the + whole review over a file this deliberately treats as optional. + """ + if not path: + print( + "No --ledger path given: judge repeat_of URLs are shape-checked only.", + file=sys.stderr, + ) + return None + try: + with open(path, encoding="utf-8") as f: + data = json.load(f) + # JSONDecodeError and UnicodeDecodeError are both ValueError; RecursionError is + # 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: + print( + f"Could not read the ledger at {path} ({e}): judge repeat_of URLs are " + "shape-checked only.", + file=sys.stderr, + ) + return None + if not isinstance(data, dict) or not isinstance(data.get("entries"), list): + print( + f"Ledger at {path} has no `entries` list: judge repeat_of URLs are " + "shape-checked only.", + file=sys.stderr, + ) + return None + shown = set() + for entry in data["entries"]: + if not isinstance(entry, dict): + continue + for key in ("discussion_url", "repeat_of"): + url = entry.get(key) + if isinstance(url, str) and url.strip(): + shown.add(url.strip()) + return frozenset(shown) + + +def repeat_url_of(finding: dict, shown_repeat_urls: frozenset[str] | None = None) -> str: """The judge's `repeat_of` URL, neutralized and stripped — or `""`. Split out of render_repeat_of (BE-12534) so the RENDERED trailer and the RAW url @@ -1524,11 +1624,34 @@ def repeat_url_of(finding: dict) -> str: The ROUND has no such twin: it stays in the trailer only, because the ledger reads a resolved ancestor's round off that ancestor's own review rather than off the payload, so carrying it structurally would cost sentinel bytes nothing reads. + + `shown_repeat_urls` (BE-12630) is the membership layer: the set of thread URLs + the ledger the judge was shown actually carried, or None to skip the check. + See load_shown_repeat_urls and the module docstring. """ url = finding.get("repeat_of") if not isinstance(url, str) or not url.strip(): return "" - return neutralize_mentions(url.strip()) + url = url.strip() + # Membership in the ledger the judge was actually shown (see the module + # docstring). `shown_repeat_urls is None` — not falsy — is what distinguishes + # "not checked" from an EMPTY shown set, which legitimately drops everything. + # + # Compared BEFORE neutralize_mentions: the set holds raw GitHub permalinks as + # build-ledger.py wrote them, and while neutralize_mentions cannot alter a URL + # of the allowed shape (it has no `@`), comparing the pre-neutralize string + # makes that independence a property of this function rather than of the + # regex someone loosens next. + if shown_repeat_urls is not None and url not in shown_repeat_urls: + # !r, not the bare string: this is relayed model text going to a step log + # that GitHub parses for `::workflow-command::` lines and that is world- + # readable on a public repo, so a newline in it must not start a line. + print( + f"Dropping repeat_of not in the ledger the judge was shown: {url!r}", + file=sys.stderr, + ) + return "" + return neutralize_mentions(url) def coerce_repeat_round(finding: dict): @@ -1695,6 +1818,15 @@ def main(): default=None, help="Banner prepended to the review body (e.g. a judge-failed degradation note).", ) + parser.add_argument( + "--ledger", + default=None, + help=( + "Path to the ledger job's ledger.json — the post-cap entry list the judge " + "prompt was rendered from. Used to drop a judge `repeat_of` naming a thread " + "the judge was not shown. Missing or unreadable degrades to shape-only." + ), + ) parser.add_argument( "--ledger-note", default=None, @@ -1779,7 +1911,9 @@ def main(): raise SystemExit(1) return - enriched = normalize_comments(findings) + # Read once, here rather than at the top of main(): the error-review and + # no-findings paths return before this and adjudicate no repeat_of at all. + enriched = normalize_comments(findings, load_shown_repeat_urls(args.ledger)) enriched, repeats_dropped = enforce_repeat_cap(enriched) # Anchor-aware split. The COUNT below stays the total across both halves — a finding # that lands in the body is still a finding, and a headline that shrank because an diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index d048236..b8f98a1 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -2621,5 +2621,313 @@ def test_strip_severity_badge_leaves_an_unbadged_body_alone(self): self.assertEqual(PR.strip_severity_badge("nonsense", "🟠 **High** — x"), "🟠 **High** — x") +class ShownRepeatUrlsTest(unittest.TestCase): + """The ledger the judge was SHOWN, read back on the posting side (BE-12630). + + `Post review` already downloads the ledger artifact, and `ledger.json` in it is + the post-cap entry list the judge prompt was rendered from. So the writer can + ask a question `_resolve_lineage` structurally cannot: not "does this id name a + root comment of ours" but "was this thread in front of the judge at all". The + gap between the two is real — an entry aged past MAX_ROUNDS, dropped by the byte + cap, or never rendered still resolves. + + The load must never RAISE and must distinguish three states, because two of them + look alike and mean the opposite things: an unreadable ledger is `None` (check + nothing, exactly as before this existed) and an `entries: []` ledger is an empty + frozenset (the judge was shown no thread, so every `repeat_of` is unfounded). + """ + + def _load(self, path): + with contextlib.redirect_stderr(io.StringIO()): + return PR.load_shown_repeat_urls(path) + + def _write(self, text): + tmp = tempfile.mkdtemp() + path = os.path.join(tmp, "ledger.json") + with open(path, "w", encoding="utf-8") as f: + f.write(text) + return path + + def test_no_path_is_none(self): + self.assertIsNone(self._load(None)) + self.assertIsNone(self._load("")) + + def test_a_missing_file_is_none(self): + tmp = tempfile.mkdtemp() + self.assertIsNone(self._load(os.path.join(tmp, "nope.json"))) + # A directory, too: the download step can leave one where the file should be. + self.assertIsNone(self._load(tmp)) + + def test_unreadable_or_invalid_json_is_none(self): + for text in ("", "not json", "[1, 2, 3]", '"a string"', "null", "[" * 3000): + with self.subTest(text=text[:20]): + self.assertIsNone(self._load(self._write(text))) + + def test_a_json_object_without_an_entries_list_is_none(self): + for text in ('{"status": "ok"}', '{"entries": {}}', '{"entries": null}'): + with self.subTest(text=text): + self.assertIsNone(self._load(self._write(text))) + + def test_an_entries_list_that_is_empty_is_an_empty_set_not_none(self): + """The distinction the whole guard turns on. `unknown`/`empty`/`disabled` + ledgers all carry `entries: []`; prompt-judge.md permits `repeat_of` only + inside a ledger block, so on those runs nothing was shown and every + `repeat_of` is unfounded — which an accidental None would silently allow.""" + shown = self._load(self._write('{"status": "unknown", "entries": []}')) + self.assertEqual(shown, frozenset()) + self.assertIsNotNone(shown) + + def test_both_lineage_keys_land_in_the_set(self): + anchored = "https://github.com/o/r/pull/1#discussion_r1" + ancestor = "https://github.com/o/r/pull/1#discussion_r2" + shown = self._load( + self._write( + json.dumps( + { + "status": "ok", + "entries": [ + {"discussion_url": anchored, "anchored": True}, + # A DEMOTED re-raise: no thread of its own, so no + # discussion_url — its lineage is the ancestor permalink + # build-ledger.py resolved, which is what the judge sees + # on the entry's `re_raise_of:` line (BE-12534). + {"discussion_url": "", "repeat_of": ancestor}, + ], + } + ) + ) + ) + self.assertEqual(shown, frozenset({anchored, ancestor})) + + def test_non_string_and_empty_values_are_skipped(self): + url = " https://github.com/o/r/pull/1#discussion_r7 " + shown = self._load( + self._write( + json.dumps( + { + "entries": [ + "not a dict", + None, + {"discussion_url": ""}, + {"discussion_url": " "}, + {"discussion_url": None, "repeat_of": 42}, + {"discussion_url": ["x"]}, + {"discussion_url": url}, + ] + } + ) + ) + ) + # Stripped on the way in, so the comparison is against the same shape + # repeat_url_of strips its candidate to. + self.assertEqual(shown, frozenset({url.strip()})) + + +class RepeatMembershipTest(unittest.TestCase): + """A judge `repeat_of` naming no entry the judge was shown is DROPPED whole. + + Whole is the point: the trailer and the sentinel key come off ONE repeat_url_of + call, so a dropped URL yields neither — and, because `enforce_repeat_cap` counts + the trailer, it also spends no slot. Dropping only the trailer would have left a + live thread URL travelling structurally; dropping only the sentinel key would have + made the demoted re-raise cap-free on the next round, which is the BE-12534 hole. + """ + + URL = "https://github.com/o/r/pull/1#discussion_r99" + + def _repeat_finding(self, url=None, body="still broken"): + raw = finding("a/b.py", 42, severity="high", body=body) + raw["repeat_of"] = url or self.URL + raw["repeat_round"] = 2 + return raw + + def _normalize(self, findings, shown): + with contextlib.redirect_stderr(io.StringIO()): + return PR.normalize_comments(findings, shown) + + def test_a_url_absent_from_the_shown_set_leaves_no_trace(self): + items = self._normalize([self._repeat_finding()], frozenset({"https://github.com/o/r/pull/1#discussion_r1"})) + self.assertEqual(items[0]["repeat_of"], "") + self.assertEqual(items[0]["repeat_url"], "") + self.assertNotIn("re-raise of", items[0]["comment"]["body"]) + self.assertNotIn("discussion_r99", items[0]["comment"]["body"]) + # Not a repeat any more, so it cannot consume one of the REPEAT_CAP slots + # a legitimate re-raise needs. + kept, dropped = PR.enforce_repeat_cap(items, cap=0) + self.assertEqual((len(kept), dropped), (1, 0)) + # …and the demoted copy carries no lineage key either. + payload = json.loads( + PR.render_body_only_sentinel(items)[ + len("") + ].replace("\\u002d", "-") + ) + self.assertNotIn("repeat_of", payload[0]) + + def test_a_url_present_in_the_shown_set_is_byte_identical_to_today(self): + raw = self._repeat_finding() + with_set = self._normalize([raw], frozenset({self.URL})) + without = PR.normalize_comments([raw]) + self.assertEqual(with_set, without) + self.assertIn("re-raise of", with_set[0]["comment"]["body"]) + self.assertEqual(with_set[0]["repeat_url"], self.URL) + + 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.""" + fixtures = [ + self._repeat_finding(), + self._repeat_finding(url="https://evil.example.com/o/r/pull/1#discussion_r99"), + self._repeat_finding(url="not a url"), + finding("a/b.py", 7, body="no lineage at all"), + ] + for raw in fixtures: + with self.subTest(repeat_of=raw.get("repeat_of")): + self.assertEqual( + PR.normalize_comments([raw], None), PR.normalize_comments([raw]) + ) + self.assertEqual( + PR.render_body_only_findings(PR.normalize_comments([raw], None)), + PR.render_body_only_findings(PR.normalize_comments([raw])), + ) + + def test_an_empty_shown_set_drops_every_repeat_of(self): + items = self._normalize( + [self._repeat_finding(), self._repeat_finding(body="also broken")], + frozenset(), + ) + self.assertEqual([i["repeat_of"] for i in items], ["", ""]) + self.assertEqual([i["repeat_url"] for i in items], ["", ""]) + + def test_the_drop_is_announced_on_stderr_without_starting_a_log_line(self): + err = io.StringIO() + with contextlib.redirect_stderr(err): + PR.normalize_comments( + [self._repeat_finding(url=self.URL + "\n::add-mask::x")], frozenset() + ) + printed = err.getvalue() + self.assertIn("Dropping repeat_of not in the ledger the judge was shown:", printed) + # One line. The URL is relayed model text reaching a step log GitHub parses + # for `::workflow-command::` at the start of a line, so it is repr'd. + self.assertEqual(len(printed.strip().splitlines()), 1) + self.assertNotIn("\n::add-mask::", printed) + + def test_a_demoted_re_raises_ancestor_url_is_accepted_from_the_ledger(self): + """The BE-12534 chain, end to end: round N demotes a re-raise, its ancestor + permalink travels structurally, build-ledger.py resolves it onto the entry's + `repeat_of` key — and round N+1's judge, re-raising that same demoted finding, + cites the ancestor URL. It appears nowhere as a `discussion_url`, so a + membership check reading only that key would drop the very lineage BE-12534 + added and hand the chain back its cap exemption.""" + ancestor = "https://github.com/o/r/pull/1#discussion_r42" + ledger = { + "status": "ok", + "entries": [ + {"discussion_url": "https://github.com/o/r/pull/1#discussion_r7", "anchored": True}, + {"discussion_url": "", "anchored": False, "repeat_of": ancestor}, + ], + } + tmp = tempfile.mkdtemp() + path = os.path.join(tmp, "ledger.json") + with open(path, "w", encoding="utf-8") as f: + json.dump(ledger, f) + with contextlib.redirect_stderr(io.StringIO()): + shown = PR.load_shown_repeat_urls(path) + items = self._normalize([self._repeat_finding(url=ancestor)], shown) + self.assertEqual(items[0]["repeat_url"], ancestor) + self.assertIn(f"re-raise of {ancestor}", items[0]["comment"]["body"]) + kept, dropped = PR.enforce_repeat_cap(items, cap=0) + self.assertEqual((len(kept), dropped), (0, 1), "and it still costs a slot") + + def test_the_shown_set_is_read_once_and_reaches_the_single_normalize_call(self): + """`main()` wiring, not the helpers: --ledger must actually be consulted, and + exactly once, on the path that posts a review.""" + raw = self._repeat_finding() + tmp = tempfile.mkdtemp() + ledger_path = os.path.join(tmp, "ledger.json") + with open(ledger_path, "w", encoding="utf-8") as f: + json.dump({"status": "ok", "entries": []}, f) + findings_path = os.path.join(tmp, "findings.json") + with open(findings_path, "w", encoding="utf-8") as f: + json.dump({"findings": [raw], "panel": []}, f) + + seen = [] + real = PR.load_shown_repeat_urls + + def spy(path): + seen.append(path) + return real(path) + + argv = [ + "post-review.py", + "--findings", findings_path, + "--pr-number", "65", + "--repo", "o/r", + "--commit-sha", "deadbee", + "--ledger", ledger_path, + ] + ok = subprocess.CompletedProcess([], 0, stdout="{}", stderr="") + with mock.patch.object(PR, "load_shown_repeat_urls", spy), \ + mock.patch.object(PR.sys, "argv", argv), \ + mock.patch.object(PR, "gh_post_review", return_value=ok) as post, \ + mock.patch.object(PR, "review_already_posted", return_value=False), \ + contextlib.redirect_stderr(io.StringIO()), \ + contextlib.redirect_stdout(io.StringIO()): + PR.main() + self.assertEqual(seen, [ledger_path], "read exactly once, from --ledger") + payload = json.loads(post.call_args[0][2]) + body = json.dumps(payload) + self.assertNotIn("re-raise of", body, "an unshown repeat_of never reaches the PR") + self.assertNotIn("discussion_r99", body) + + +class LedgerFlagWiringTest(unittest.TestCase): + """Every post-review.py invocation in cursor-review.yml passes --ledger. + + Three branches post a review — judge-ok, the raw-panel `--notice` degradation, + and the error review — and only the first is the obvious one. The raw-panel + branch ships PANEL output the judge never adjudicated, which is exactly where an + unfounded `repeat_of` is most likely; missing the flag there would leave the + degraded path on the old shape-only behaviour with nothing to say so. + + The step deliberately does NOT gate on the download's outcome: `ledger_download` + is continue-on-error, and load_shown_repeat_urls degrades on a missing file. + """ + + WORKFLOW = os.path.join( + os.path.dirname(__file__), "..", "..", "workflows", "cursor-review.yml" + ) + INVOCATION = 'python3 "$CURSOR_REVIEW_ASSETS/post-review.py" \\' + + def setUp(self): + with open(self.WORKFLOW, encoding="utf-8") as f: + self.text = f.read() + + def commands(self): + """Each post-review.py invocation, reassembled across its `\\` continuations.""" + out, lines = [], self.text.splitlines() + for i, line in enumerate(lines): + if line.strip() != self.INVOCATION: + continue + cmd, j = line, i + while lines[j].rstrip().endswith("\\"): + j += 1 + cmd += "\n" + lines[j] + out.append(cmd) + return out + + def test_all_three_invocations_carry_the_ledger_flag(self): + cmds = self.commands() + self.assertEqual(len(cmds), 3, "the branch count changed — re-check each one") + for cmd in cmds: + with self.subTest(cmd=cmd.splitlines()[1:3]): + self.assertIn("--ledger /tmp/ledger/ledger.json", cmd) + + def test_the_path_is_the_one_the_download_step_writes(self): + # `ledger_download` unpacks the artifact to /tmp/ledger; no new download step + # exists or is needed, so the two must not drift apart. + self.assertIn("path: /tmp/ledger", self.text) + + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index d52afff..a8a2269 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2393,6 +2393,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ + --ledger /tmp/ledger/ledger.json \ --ledger-note "$LEDGER_NOTE" elif [ "${CONSOLIDATED_COUNT:-0}" != "0" ]; then # No JUDGE_ERROR is read here on purpose: this branch passes --notice with @@ -2410,6 +2411,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ + --ledger /tmp/ledger/ledger.json \ --ledger-note "$LEDGER_NOTE" \ --notice "⚠️ The judge step did not submit a final review through its structured-output tool (status=${JUDGE_STATUS}). The findings below are the raw, un-adjudicated panel output — they may contain duplicates or false positives the judge would normally filter." else @@ -2433,6 +2435,7 @@ jobs: --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ + --ledger /tmp/ledger/ledger.json \ --ledger-note "$LEDGER_NOTE" \ --error-message "Judge call failed (status=${JUDGE_STATUS}): ${JUDGE_ERROR}" fi From 72158a6d69f9b3e7f971b7dd245fb559a655222c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 21:12:44 -0700 Subject: [PATCH 2/3] fix(cursor-review): shape-check the re-raise trailer, charge a rejected lineage a cap slot, gate membership on the JUDGE's ledger (BE-12630) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the BE-12630 membership guard. - repeat_url_of now applies REPEAT_URL_RE + REPEAT_URL_MAX_CHARS itself. They were reached only from render_body_only_sentinel, so the rendered TRAILER — the prose in a bot-authored comment — carried whatever the judge wrote: unbounded (long enough to 422 the inline payload) and spending a REPEAT_CAP slot the sentinel then refused to carry, the trailer/sentinel disagreement render_repeat_trailer exists to prevent. The reader (build-ledger.py) already applied the identical pair, so no URL it would have accepted is newly denied. - enforce_repeat_cap counts a DECLARED re-raise whose URL was refused (`repeat_dropped`). Refunding the slot made a link we drop strictly cheaper than an honest one, so a judge could re-litigate a whole round uncapped by citing threads it was never shown. The finding is still kept — only its lineage is dropped. - New --no-judge-ledger, set by the workflow when JUDGE_LEDGER_OK is false. The judge's and the poster's ledger downloads fail independently and only the judge's says what the judge SAW; on that skew the shown set is empty rather than this job's own intact copy of ledger.json. - load_shown_repeat_urls mirrors the render gates (non-ok status shows nothing; `re_raise_of:` only on a non-anchored entry) and emits a ::warning:: when it degrades to shape-only, so a silently disabled guard is visible. - Deleted render_repeat_of: callerless since normalize_comments took the two-value path, and reintroducing it restores the double-log/desync shape. Module docstring and the cursor-review README's repeat-policy section corrected to match: the old text claimed a dropped re-raise spent no cap slot. --- .github/cursor-review/README.md | 2 +- .github/cursor-review/post-review.py | 162 ++++++++-- .../cursor-review/tests/test_build_ledger.py | 2 +- .../cursor-review/tests/test_post_review.py | 298 ++++++++++++++++-- .github/workflows/cursor-review.yml | 25 +- 5 files changed, 416 insertions(+), 73 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 6c38b8a..20e2b64 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -77,7 +77,7 @@ With `ledger_prior_review` on (the default), [`build-ledger.py`](build-ledger.py A finding that could not be anchored to a line the reviewed diff carries is **demoted** to the review body, and one that lost its whole review to a failed POST is delivered as prose — neither gets a thread, so neither can be answered and neither costs a slot. But a demoted finding may itself have been a *re-raise*, and that lineage used to disappear with the trailer that was stripped out of its recovered body: the next round saw a fresh, thread-less, cap-exempt finding, so one demoted hop made every later re-raise of the same finding free. The sentinel now carries the ancestor's URL as a field, and the ledger resolves it against the PR's own comments: the trailing `discussion_r` must name a **root** comment of one of *our* consolidated reviews on this PR, so a hallucinated id resolves to nothing. Only that id is read — the owner/repo/PR-number half of the URL is shape-checked but never compared — so a link naming another repo resolves whenever its id matches one of our roots; what the entry renders is always the ancestor's *own* permalink, never the relayed string, and an ancestor with no permalink yields no lineage. The round is recovered the same way, from the review that ancestor belongs to, rather than from the payload. When it resolves, the entry renders `re_raise_of: (round N; ancestor_answers=)` — a key deliberately distinct from the `answers_from_author_or_maintainer` on the entry's own `thread:` line, since the two describe different findings — plus a `re_raise_answer:` excerpt of the ancestor's most recent answering reply, and the repeat rule is applied to that **ancestor's** answer state: with `k >= 1` a further re-raise must carry `repeat_of` and costs a slot, and with `k == 0` nothing changes. Resolution runs over every consolidated review before the round cap, so the ancestor is still found after its own round has aged out of the ledger. A claim that does *not* resolve — a dismissed review, a deleted comment — is not silently rebuilt as a fresh cap-exempt finding: the entry says the lineage is unverified and withholds the exemption. -A `repeat_of` is adjudicated on two independent layers, and neither is the other's backstop. The **writer** ([`post-review.py`](post-review.py)) checks SHAPE — one anchored GitHub discussion permalink, nothing else — and, given `--ledger`, MEMBERSHIP: the URL must be one the ledger the judge was actually shown carried, i.e. an entry's `discussion_url` or the `repeat_of` lineage a demoted re-raise entry renders on its `re_raise_of:` line. The **reader** ([`build-ledger.py`](build-ledger.py)) separately resolves the trailing comment id against this PR's own consolidated-review roots, as above. Membership catches precisely what resolution cannot: an id that resolves but was never SHOWN — aged past the round cap, dropped by the byte cap, or never rendered — which the judge therefore had no basis to cite. A URL naming no shown entry is dropped whole: no trailer, no sentinel field, and no `REPEAT_CAP` slot spent, so a dropped re-raise cannot buy itself the cap exemption. An `empty`, `disabled` or `unknown` ledger carries `entries: []`, and the judge prompt permits `repeat_of` only inside a ledger block, so on those runs the shown set is legitimately empty and every `repeat_of` is dropped. A ledger that cannot be read at all is the one case that must NOT drop anything — no path, no file, unreadable, not an object, no `entries` list — so it degrades to shape-only, exactly as before, and the reader's half still holds. +A `repeat_of` is adjudicated on two independent layers, and neither is the other's backstop. The **writer** ([`post-review.py`](post-review.py)) checks SHAPE — one anchored GitHub discussion permalink under 512 characters, nothing else, applied to the rendered trailer and the structural field alike — and, given `--ledger`, MEMBERSHIP: the URL must be one the ledger the judge was actually shown carried, i.e. an *anchored* entry's `discussion_url` or the `repeat_of` lineage a *demoted* re-raise entry renders on its `re_raise_of:` line, mirroring the gates the render itself applies. The **reader** ([`build-ledger.py`](build-ledger.py)) separately resolves the trailing comment id against this PR's own consolidated-review roots, as above. Membership catches precisely what resolution cannot: an id that resolves but was never SHOWN — aged past the round cap, dropped by the byte cap, or never rendered — which the judge therefore had no basis to cite. A URL failing either check is dropped whole — no trailer, no sentinel field, nothing carried into the next round — while the judge's *declaration* that the finding is a re-raise still spends its `REPEAT_CAP` slot. That asymmetry is deliberate: refunding the slot would make a link we reject strictly cheaper than an honest one, so a judge could turn a whole round into uncapped re-litigation simply by citing threads it was never shown. The finding itself is never deleted for a bad link — only its lineage is. An `empty`, `disabled` or `unknown` ledger carries `entries: []`, and the judge prompt permits `repeat_of` only inside a ledger block, so on those runs the shown set is legitimately empty and every `repeat_of` is dropped. A ledger that cannot be read at all is the one case that must NOT drop anything — no path, no file, unreadable, not an object, no `entries` list — so it degrades to shape-only, exactly as before, the reader's half still holds, and the run carries a `::warning::` annotation so a silently disabled guard is visible rather than buried in a step log. The judge's and the poster's ledger downloads fail independently, and only the **judge's** decides what was shown: when that one fails the workflow passes `--no-judge-ledger` and the shown set is empty, because the judge prompt was spliced an empty ledger block however intact this job's own copy of `ledger.json` may be. ### The panel diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 707407b..b2a2302 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -27,18 +27,22 @@ A judge `repeat_of` is checked on TWO independent layers, and neither is the other's backstop. Here — the WRITER — it must have the right SHAPE (one anchored -GitHub discussion permalink, nothing else) and, given `--ledger`, it must also be -a MEMBER of the set of thread URLs carried by the very ledger the judge prompt was -rendered from: `discussion_url` on an anchored entry, plus the `repeat_of` lineage -of a demoted re-raise entry (what its `re_raise_of:` line shows). A URL naming no -shown entry is dropped outright, so it costs no `REPEAT_CAP` slot and travels -neither as the rendered trailer nor as the body-only sentinel field. On the +GitHub discussion permalink under `REPEAT_URL_MAX_CHARS`, nothing else) and, given +`--ledger`, it must also be a MEMBER of the set of thread URLs carried by the very +ledger the judge prompt was rendered from: `discussion_url` on an anchored entry, +plus the `repeat_of` lineage of a demoted re-raise entry (what its `re_raise_of:` +line shows). A URL failing either check is dropped WHOLE — it travels neither as +the rendered trailer nor as the body-only sentinel field — but the judge's +DECLARATION that the finding is a re-raise still costs its `REPEAT_CAP` slot, so a +rejected link cannot be a cheaper way to re-litigate than an honest one. On the READER, build-ledger.py's `_resolve_lineage` separately resolves the trailing comment id against this PR's own consolidated-review roots. Membership deliberately catches only what resolution cannot: an id that is resolvable but was never SHOWN (aged past the round cap, dropped by the byte cap, or never rendered). Without `--ledger`, or with one that cannot be read, this degrades to shape-only and the -reader's half still holds. +reader's half still holds. `--no-judge-ledger` is the third state: the JUDGE's own +download failed, so its prompt carried an EMPTY ledger block and the shown set is +empty — this job's copy of ledger.json is not what the judge saw. """ import argparse @@ -1484,11 +1488,20 @@ def normalize_comments( # 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. + claimed = finding.get("repeat_of") + declared_repeat = isinstance(claimed, str) and bool(claimed.strip()) repeat_url = repeat_url_of(finding, shown_repeat_urls) repeat_line = render_repeat_trailer(finding, repeat_url) enriched.append( { "severity": severity, + # The judge DECLARED a re-raise and only its URL was refused — wrong + # shape, or naming no entry the ledger showed it. The declaration still + # costs a REPEAT_CAP slot (see enforce_repeat_cap): counting only the + # rendered trailer would make a URL we reject strictly CHEAPER than an + # honest one, so a judge could turn a whole round into uncapped + # re-litigation just by citing links the guard drops. + "repeat_dropped": declared_repeat and not repeat_url, # Truthy only for a re-raise of an already-answered finding — # what enforce_repeat_cap counts against REPEAT_CAP. "repeat_of": repeat_line, @@ -1523,22 +1536,16 @@ def normalize_comments( return enriched -def render_repeat_of( - finding: dict, shown_repeat_urls: frozenset[str] | None = None -) -> str: - """Render the re-raise line for a finding the judge marked as a repeat. - - `repeat_of` is the prior round's `discussion_url` from the ledger. Showing - it inline is the whole point of the repeat policy: a re-raise happens on the - record, linked to the thread that already answered it, so the author can see - at a glance that this is round N of the same conversation. - """ - return render_repeat_trailer(finding, repeat_url_of(finding, shown_repeat_urls)) - - def render_repeat_trailer(finding: dict, url: str) -> str: """The trailer for an ALREADY-resolved url, so one call decides both fields. + The SINGLE entry point for rendering the re-raise line. A `render_repeat_of` that + took the finding and resolved the URL itself lived here until BE-12630 removed it: + it had no callers left, and reintroducing one would restore exactly the two-call + shape this signature exists to forbid — a second repeat_url_of over the same + finding, logging the membership drop twice and free to disagree with the + `repeat_url` normalize_comments stored beside the trailer. + normalize_comments stores the rendered trailer and the raw URL side by side and they must never disagree — a trailer whose URL was dropped would spend a REPEAT_CAP slot on lineage the sentinel does not carry, and would put a live @@ -1550,6 +1557,26 @@ def render_repeat_trailer(finding: dict, url: str) -> str: return f"\n\n↩︎ re-raise of {url}{render_repeat_round(finding)}" +def warn_membership_guard_off() -> None: + """Annotate the run when the membership layer turns itself OFF. + + Every ledger download is continue-on-error, so a transient artifact failure + silently degrades this guard to shape-only and an UNPROTECTED run is otherwise + indistinguishable from a protected one — a stderr line nobody opens the log for. + A `::warning::` puts it on the run summary next to the checks. + + The text is a fixed literal: no path, no exception string, nothing relayed. This + line is parsed as a workflow command when it starts one, so nothing that could + carry a newline may be interpolated into it. The detail stays on stderr. + """ + print( + "::warning::cursor-review: the prior-review ledger could not be read on the " + "posting side, so a judge repeat_of is shape-checked only for this run " + "(membership against the ledger the judge was shown is OFF).", + flush=True, + ) + + def load_shown_repeat_urls(path) -> frozenset[str] | None: """Every thread URL the ledger at `path` showed the judge — or None. @@ -1565,12 +1592,14 @@ def load_shown_repeat_urls(path) -> frozenset[str] | None: fields on no other finding"), so on such a run the judge was shown no thread at all and every `repeat_of` it emits is unfounded. - Two keys per entry, because two kinds of entry can be re-raised. An anchored - entry renders its own `discussion_url:`. A DEMOTED re-raise has no thread of - its own and renders `re_raise_of: ` instead, out of the entry's - `repeat_of` key (BE-12534) — a resolved ancestor permalink, and the judge is - told to carry it forward, so it is legitimately shown even though no entry - calls it a `discussion_url`. + One key per entry, chosen by the SAME gates build-ledger.py renders behind, so + the set is what was displayed rather than what the file holds. An anchored entry + renders its own `discussion_url:` and never carries lineage of its own. A DEMOTED + re-raise has no thread and renders `re_raise_of: ` instead, out of the + entry's `repeat_of` key (BE-12534) — a resolved ancestor permalink the judge is + told to carry forward, so it is legitimately shown even though no entry calls it + a `discussion_url`. A ledger whose `status` is not `ok` renders an EMPTY block, so + it shows nothing at all. Never raises: it runs on the posting path, where an exception would cost the whole review over a file this deliberately treats as optional. @@ -1594,6 +1623,7 @@ def load_shown_repeat_urls(path) -> frozenset[str] | None: "shape-checked only.", file=sys.stderr, ) + warn_membership_guard_off() return None if not isinstance(data, dict) or not isinstance(data.get("entries"), list): print( @@ -1601,12 +1631,25 @@ def load_shown_repeat_urls(path) -> frozenset[str] | None: "shape-checked only.", file=sys.stderr, ) + warn_membership_guard_off() return None + # build-ledger.py renders an EMPTY block for any non-ok status, so the judge was + # shown no thread at all and every repeat_of on such a run is unfounded. Latent + # while `unknown` / `empty` / `disabled` all hard-code `entries: []`, but mirrored + # explicitly so the shown set stays equal to what was RENDERED rather than to what + # the file happens to hold, which is what this function's contract claims. + if data.get("status") != "ok": + return frozenset() shown = set() for entry in data["entries"]: if not isinstance(entry, dict): continue - for key in ("discussion_url", "repeat_of"): + # The same gate the renderer applies (build-ledger.py: `discussion_url:` only + # when anchored, `re_raise_of:` only when not). An anchored entry has a thread + # of its own and never carries a `repeat_of` key; reading one anyway would put + # a URL in the shown set that no line of the prompt ever displayed. + keys = ("discussion_url",) if entry.get("anchored", True) else ("repeat_of",) + for key in keys: url = entry.get(key) if isinstance(url, str) and url.strip(): shown.add(url.strip()) @@ -1625,14 +1668,34 @@ def repeat_url_of(finding: dict, shown_repeat_urls: frozenset[str] | None = None a resolved ancestor's round off that ancestor's own review rather than off the payload, so carrying it structurally would cost sentinel bytes nothing reads. - `shown_repeat_urls` (BE-12630) is the membership layer: the set of thread URLs - the ledger the judge was shown actually carried, or None to skip the check. + Two checks, in order. SHAPE first (BE-12630): REPEAT_URL_RE plus + REPEAT_URL_MAX_CHARS, the same pair render_body_only_sentinel applies to the + structural field — applied here so the rendered TRAILER is bounded too, rather + than only the field, which is what let an arbitrary judge string reach a public + review body. Then MEMBERSHIP: `shown_repeat_urls` is the set of thread URLs the + ledger the judge was shown actually carried, or None to skip that half. See load_shown_repeat_urls and the module docstring. """ url = finding.get("repeat_of") if not isinstance(url, str) or not url.strip(): return "" url = url.strip() + # SHAPE, and on THIS path — not only in render_body_only_sentinel (BE-12630). + # Applying it there alone left the TRAILER, the prose a human reads, carrying + # whatever the judge wrote: an arbitrary string rendered after "re-raise of", of + # unbounded length (a long enough one 422s the entire inline payload), spending a + # REPEAT_CAP slot for lineage the sentinel then refused to carry — the exact + # trailer/sentinel disagreement render_repeat_trailer exists to prevent. Both + # halves now live here, so the module docstring's "degrades to shape-only" is + # true of the trailer as well as of the sentinel field. + # + # fullmatch for the same reason the sentinel uses it: `$` also matches just BEFORE + # a trailing newline, and that newline would land at column 0 of the next round's + # prompt. + if len(url) > REPEAT_URL_MAX_CHARS or not REPEAT_URL_RE.fullmatch(url): + # Truncated and !r for the same reason as the membership drop below. + print(f"Dropping malformed repeat_of: {url[:200]!r}", file=sys.stderr) + return "" # Membership in the ledger the judge was actually shown (see the module # docstring). `shown_repeat_urls is None` — not falsy — is what distinguishes # "not checked" from an EMPTY shown set, which legitimately drops everything. @@ -1712,7 +1775,12 @@ def enforce_repeat_cap(enriched: list[dict], cap: int = REPEAT_CAP) -> tuple[lis kept, dropped = [], 0 repeats = 0 for item in enriched: - if item.get("repeat_of"): + # `repeat_dropped` counts too: the judge declared a re-raise and only the URL + # was refused (malformed, or naming no entry it was shown), so the declaration + # spends the budget exactly as an honest one does. Counting the rendered + # trailer alone would leave a round of five fabricated-lineage re-raises + # entirely uncapped — cheaper than five real ones. + if item.get("repeat_of") or item.get("repeat_dropped"): if repeats >= cap: dropped += 1 continue @@ -1827,6 +1895,15 @@ def main(): "the judge was not shown. Missing or unreadable degrades to shape-only." ), ) + parser.add_argument( + "--no-judge-ledger", + action="store_true", + help=( + "The JUDGE's ledger download failed, so its prompt was spliced an empty " + "ledger block. The shown set is then empty regardless of --ledger: this " + "job's own copy of ledger.json is not what the judge saw." + ), + ) parser.add_argument( "--ledger-note", default=None, @@ -1913,7 +1990,22 @@ def main(): # Read once, here rather than at the top of main(): the error-review and # no-findings paths return before this and adjudicate no repeat_of at all. - enriched = normalize_comments(findings, load_shown_repeat_urls(args.ledger)) + if args.no_judge_ledger: + # The two downloads fail independently, and only the JUDGE's decides what the + # judge was shown. When the judge's failed its prompt carried an EMPTY ledger + # block, so the shown set is empty — NOT None, and not this job's own copy of + # ledger.json, whose contents the judge never saw. Reading that copy here would + # make "the very ledger the judge prompt was rendered from" false on exactly + # the degraded run the header already banners. + print( + "The judge's ledger download failed: its prompt carried an empty ledger " + "block, so every judge repeat_of is unfounded and is dropped.", + file=sys.stderr, + ) + shown_repeat_urls = frozenset() + else: + shown_repeat_urls = load_shown_repeat_urls(args.ledger) + enriched = normalize_comments(findings, shown_repeat_urls) enriched, repeats_dropped = enforce_repeat_cap(enriched) # Anchor-aware split. The COUNT below stays the total across both halves — a finding # that lands in the body is still a finding, and a headline that shrank because an @@ -1933,8 +2025,12 @@ def main(): review_head = f"{header}\n\nFound **{len(enriched)}** finding(s)." if repeats_dropped: review_head += ( - f"\n\n_{repeats_dropped} re-raise(s) of already-answered findings were dropped " - f"(cap: {REPEAT_CAP} per review). They are still open on their original threads._" + # "the judge declared", not "of already-answered findings": the cap now + # also counts a declaration whose URL was refused, and such a finding has no + # original thread to still be open on. + f"\n\n_{repeats_dropped} re-raise(s) the judge declared were dropped " + f"(cap: {REPEAT_CAP} per review). Any earlier thread they repeat is " + f"still open._" ) severity_summary = build_severity_summary(enriched) if severity_summary: diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 2f186c5..9230aff 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -477,7 +477,7 @@ def test_repeats_beyond_the_cap_are_dropped_and_noted(self): self.assertIn(50, lines) self.assertNotIn(30, lines) self.assertNotIn(40, lines) - self.assertIn("2 re-raise(s) of already-answered findings were dropped", payload["body"]) + self.assertIn("2 re-raise(s) the judge declared were dropped", payload["body"]) def test_repeat_cap_helper(self): enriched = [ diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index b8f98a1..053baac 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -2690,8 +2690,11 @@ def test_both_lineage_keys_land_in_the_set(self): # A DEMOTED re-raise: no thread of its own, so no # discussion_url — its lineage is the ancestor permalink # build-ledger.py resolved, which is what the judge sees - # on the entry's `re_raise_of:` line (BE-12534). - {"discussion_url": "", "repeat_of": ancestor}, + # on the entry's `re_raise_of:` line (BE-12534). Carries + # `anchored: False`, as _body_only_entries writes it: that + # is the flag the renderer gates `re_raise_of:` on, so it + # is the flag this must gate on too. + {"discussion_url": "", "anchored": False, "repeat_of": ancestor}, ], } ) @@ -2705,6 +2708,9 @@ def test_non_string_and_empty_values_are_skipped(self): self._write( json.dumps( { + # `status: ok` throughout: a ledger the renderer would not + # display shows nothing at all, which the status test covers. + "status": "ok", "entries": [ "not a dict", None, @@ -2723,6 +2729,67 @@ def test_non_string_and_empty_values_are_skipped(self): self.assertEqual(shown, frozenset({url.strip()})) + def test_a_non_ok_status_shows_nothing(self): + """build-ledger.py renders an EMPTY block for any non-ok status, so the judge + saw no thread whatever the file holds. Mirrored here rather than left to the + `entries: []` those statuses happen to hard-code, so the two cannot drift.""" + url = "https://github.com/o/r/pull/1#discussion_r5" + for status in ('"unknown"', '"empty"', '"disabled"', "null"): + with self.subTest(status=status): + shown = self._load( + self._write( + '{"status": %s, "entries": [{"discussion_url": "%s"}]}' + % (status, url) + ) + ) + self.assertEqual(shown, frozenset(), "an empty set, not None") + self.assertIsNotNone(shown) + + def test_an_anchored_entrys_repeat_of_is_never_shown(self): + """The renderer prints `re_raise_of:` only for a NOT-anchored entry (an + anchored one has a thread of its own and _body_only_entries is the only writer + of the lineage keys). Reading it regardless would put a URL in the shown set + that no line of the prompt displayed.""" + own = "https://github.com/o/r/pull/1#discussion_r1" + lineage = "https://github.com/o/r/pull/1#discussion_r2" + shown = self._load( + self._write( + json.dumps( + { + "status": "ok", + "entries": [ + {"discussion_url": own, "anchored": True, "repeat_of": lineage} + ], + } + ) + ) + ) + self.assertEqual(shown, frozenset({own})) + + def test_a_degraded_read_is_annotated_on_the_run(self): + """`ledger_download` is continue-on-error, so a transient artifact failure + turns the membership layer off with nothing but a stderr line — an unprotected + run indistinguishable from a protected one. A ::warning:: puts it on the run + summary. The two cases that are real degradations get it; "no --ledger given" + is a caller's choice, not a failure, and does not.""" + for path in (self._write("not json"), self._write('{"entries": 3}')): + with self.subTest(path=path): + out, err = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + self.assertIsNone(PR.load_shown_repeat_urls(path)) + self.assertTrue( + out.getvalue().startswith("::warning::cursor-review:"), + "the annotation must open the line to be parsed as one", + ) + self.assertEqual(len(out.getvalue().strip().splitlines()), 1) + + out = io.StringIO() + with contextlib.redirect_stdout(out), contextlib.redirect_stderr(io.StringIO()): + PR.load_shown_repeat_urls(None) + PR.load_shown_repeat_urls(self._write('{"status": "ok", "entries": []}')) + self.assertEqual(out.getvalue(), "", "a readable ledger annotates nothing") + + class RepeatMembershipTest(unittest.TestCase): """A judge `repeat_of` naming no entry the judge was shown is DROPPED whole. @@ -2751,10 +2818,13 @@ def test_a_url_absent_from_the_shown_set_leaves_no_trace(self): self.assertEqual(items[0]["repeat_url"], "") self.assertNotIn("re-raise of", items[0]["comment"]["body"]) self.assertNotIn("discussion_r99", items[0]["comment"]["body"]) - # Not a repeat any more, so it cannot consume one of the REPEAT_CAP slots - # a legitimate re-raise needs. + # But the DECLARATION still costs a REPEAT_CAP slot. The judge said this was a + # re-raise and only its URL was refused; if the refusal also refunded the slot, + # citing a link we drop would be strictly cheaper than citing a real one and a + # round of five fabricated re-raises would be entirely uncapped. + self.assertIs(items[0]["repeat_dropped"], True) kept, dropped = PR.enforce_repeat_cap(items, cap=0) - self.assertEqual((len(kept), dropped), (1, 0)) + self.assertEqual((len(kept), dropped), (0, 1)) # …and the demoted copy carries no lineage key either. payload = json.loads( PR.render_body_only_sentinel(items)[ @@ -2772,24 +2842,74 @@ def test_a_url_present_in_the_shown_set_is_byte_identical_to_today(self): self.assertEqual(with_set[0]["repeat_url"], self.URL) 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.""" - fixtures = [ - self._repeat_finding(), + """The default every existing caller still gets: membership OFF, shape ON. + + Asserting `normalize_comments([raw], None) == normalize_comments([raw])` alone + would be VACUOUS — None IS the default, so both sides take one path — and would + have passed just as happily with no shape guard at all. So each fixture states + the OUTCOME it must reach, and a shape guard that goes missing fails here. + """ + good = self._repeat_finding() + bad = [ self._repeat_finding(url="https://evil.example.com/o/r/pull/1#discussion_r99"), self._repeat_finding(url="not a url"), - finding("a/b.py", 7, body="no lineage at all"), + # INTERNAL whitespace, not trailing: repeat_url_of strips first, so a + # trailing newline is a well-shaped URL with slack around it, and the + # fullmatch is what stops one from surviving in the middle. + self._repeat_finding(url="https://github.com/o/r/pull/1#discussion\n_r9"), + self._repeat_finding(url="https://github.com/o/r/pull/1#discussion_r9 x"), + self._repeat_finding( + url="https://github.com/o/r/pull/1#discussion_r" + "9" * 600 + ), ] - for raw in fixtures: - with self.subTest(repeat_of=raw.get("repeat_of")): - self.assertEqual( - PR.normalize_comments([raw], None), PR.normalize_comments([raw]) - ) - self.assertEqual( - PR.render_body_only_findings(PR.normalize_comments([raw], None)), - PR.render_body_only_findings(PR.normalize_comments([raw])), - ) + + kept = self._normalize([good], None) + self.assertEqual(kept[0]["repeat_url"], self.URL, "a well-shaped URL survives") + self.assertIn("re-raise of", kept[0]["comment"]["body"]) + self.assertIs(kept[0]["repeat_dropped"], False) + + for raw in bad: + with self.subTest(repeat_of=raw["repeat_of"][:40]): + items = self._normalize([raw], None) + self.assertEqual(items[0]["repeat_of"], "", "no trailer") + self.assertEqual(items[0]["repeat_url"], "", "no sentinel field") + self.assertNotIn("re-raise of", items[0]["comment"]["body"]) + # Still a declared re-raise, so still capped. + self.assertIs(items[0]["repeat_dropped"], True) + # …and identical to what the parameterless call produces, which is the + # compatibility half the original assertion was reaching for. + self.assertEqual(items, PR.normalize_comments([raw])) + + def test_a_malformed_url_never_reaches_the_rendered_trailer(self): + """The shape pair used to live only in render_body_only_sentinel, so an + arbitrary judge string reached the PROSE — rendered after "re-raise of" in a + bot-authored comment, unbounded in length, and spending a REPEAT_CAP slot for + lineage the sentinel then refused to carry. Both halves are on repeat_url_of + now, so the trailer and the field agree by construction.""" + raw = self._repeat_finding(url="javascript:alert(1) " + "A" * 900) + err = io.StringIO() + with contextlib.redirect_stderr(err): + items = PR.normalize_comments([raw], None) + body = items[0]["comment"]["body"] + self.assertNotIn("re-raise of", body) + self.assertNotIn("javascript:", body) + self.assertEqual(items[0]["repeat_url"], "") + self.assertIn("Dropping malformed repeat_of:", err.getvalue()) + # Bounded and single-line for the same reason the membership drop is. + self.assertEqual(len(err.getvalue().strip().splitlines()), 1) + self.assertLess(len(err.getvalue()), 400) + + def test_a_rejected_declaration_is_capped_exactly_like_an_honest_one(self): + """REPEAT_CAP counts DECLARED re-raises, however their URL fared. Otherwise the + cheapest way to re-litigate a whole round is to cite links the guard drops.""" + honest = [self._repeat_finding(body=f"real {i}") for i in range(2)] + fabricated = [ + self._repeat_finding(url=f"https://github.com/o/r/pull/1#discussion_r{i}") + for i in range(3) + ] + items = self._normalize(honest + fabricated, frozenset({self.URL})) + kept, dropped = PR.enforce_repeat_cap(items, cap=2) + self.assertEqual((len(kept), dropped), (2, 3)) def test_an_empty_shown_set_drops_every_repeat_of(self): items = self._normalize( @@ -2800,17 +2920,29 @@ def test_an_empty_shown_set_drops_every_repeat_of(self): self.assertEqual([i["repeat_url"] for i in items], ["", ""]) def test_the_drop_is_announced_on_stderr_without_starting_a_log_line(self): - err = io.StringIO() - with contextlib.redirect_stderr(err): - PR.normalize_comments( - [self._repeat_finding(url=self.URL + "\n::add-mask::x")], frozenset() - ) - printed = err.getvalue() - self.assertIn("Dropping repeat_of not in the ledger the judge was shown:", printed) - # One line. The URL is relayed model text reaching a step log GitHub parses - # for `::workflow-command::` at the start of a line, so it is repr'd. - self.assertEqual(len(printed.strip().splitlines()), 1) - self.assertNotIn("\n::add-mask::", printed) + """Both drops, because both print relayed model text. Which one fires depends + on the URL: shape runs first, so an injected newline never reaches the + membership branch — and the well-shaped non-member below is what does.""" + cases = [ + (self.URL + "\n::add-mask::x", "Dropping malformed repeat_of:"), + ( + "https://github.com/o/r/pull/1#discussion_r1234", + "Dropping repeat_of not in the ledger the judge was shown:", + ), + ] + for url, expected in cases: + with self.subTest(url=url[:40]): + err = io.StringIO() + with contextlib.redirect_stderr(err): + PR.normalize_comments([self._repeat_finding(url=url)], frozenset()) + printed = err.getvalue() + self.assertIn(expected, printed) + # One line. The URL is relayed model text reaching a step log GitHub + # parses for `::workflow-command::` at the start of a line, so it is + # repr'd — and this module now writes real annotations to stdout, so a + # value able to start a line could forge one. + self.assertEqual(len(printed.strip().splitlines()), 1) + self.assertNotIn("\n::add-mask::", printed) def test_a_demoted_re_raises_ancestor_url_is_accepted_from_the_ledger(self): """The BE-12534 chain, end to end: round N demotes a re-raise, its ancestor @@ -2881,6 +3013,75 @@ def spy(path): self.assertNotIn("discussion_r99", body) +class JudgeLedgerMissingTest(unittest.TestCase): + """`--no-judge-ledger`: the JUDGE's download failed, so nothing was shown. + + The judge's and the poster's ledger downloads fail independently and the workflow + already tracks them apart (`LEDGER_DOWNLOAD_JUDGE` vs `LEDGER_DOWNLOAD`). Only the + judge's says what the judge SAW: when it failed the judge prompt was spliced an + empty ledger block, so every repeat_of it emitted is unfounded — even if this job's + own copy of ledger.json arrived intact and is full of genuine thread URLs. Reading + that copy would make the guard vouch for links the judge never saw, on exactly the + degraded run the header banner already calls out. + """ + + URL = "https://github.com/o/r/pull/1#discussion_r99" + + def _run(self, extra_argv): + raw = finding("a/b.py", 42, severity="high", body="still broken") + raw["repeat_of"] = self.URL + raw["repeat_round"] = 2 + tmp = tempfile.mkdtemp() + ledger_path = os.path.join(tmp, "ledger.json") + with open(ledger_path, "w", encoding="utf-8") as f: + json.dump( + {"status": "ok", "entries": [{"discussion_url": self.URL, "anchored": True}]}, + f, + ) + findings_path = os.path.join(tmp, "findings.json") + with open(findings_path, "w", encoding="utf-8") as f: + json.dump({"findings": [raw], "panel": []}, f) + + seen = [] + real = PR.load_shown_repeat_urls + + def spy(path): + seen.append(path) + return real(path) + + argv = [ + "post-review.py", + "--findings", findings_path, + "--pr-number", "65", + "--repo", "o/r", + "--commit-sha", "deadbee", + "--ledger", ledger_path, + ] + extra_argv + ok = subprocess.CompletedProcess([], 0, stdout="{}", stderr="") + with mock.patch.object(PR, "load_shown_repeat_urls", spy), \ + mock.patch.object(PR.sys, "argv", argv), \ + mock.patch.object(PR, "gh_post_review", return_value=ok) as post, \ + mock.patch.object(PR, "review_already_posted", return_value=False), \ + contextlib.redirect_stderr(io.StringIO()), \ + contextlib.redirect_stdout(io.StringIO()): + PR.main() + return seen, json.dumps(json.loads(post.call_args[0][2])) + + def test_without_the_flag_the_shown_ledger_admits_the_re_raise(self): + """The control: same fixtures, same ledger — the only difference is the flag.""" + seen, body = self._run([]) + self.assertEqual(len(seen), 1, "--ledger was read") + self.assertIn("re-raise of", body) + + def test_with_the_flag_the_ledger_is_not_read_and_the_re_raise_is_dropped(self): + seen, body = self._run(["--no-judge-ledger"]) + self.assertEqual(seen, [], "the poster's copy is not what the judge saw") + self.assertNotIn("re-raise of", body) + self.assertNotIn("discussion_r99", body) + # Dropped, not deleted: the finding itself still reaches the PR. + self.assertIn("still broken", body) + + class LedgerFlagWiringTest(unittest.TestCase): """Every post-review.py invocation in cursor-review.yml passes --ledger. @@ -2890,8 +3091,12 @@ class LedgerFlagWiringTest(unittest.TestCase): unfounded `repeat_of` is most likely; missing the flag there would leave the degraded path on the old shape-only behaviour with nothing to say so. - The step deliberately does NOT gate on the download's outcome: `ledger_download` - is continue-on-error, and load_shown_repeat_urls degrades on a missing file. + `--ledger` itself is deliberately NOT gated on the POSTER's download outcome: + `ledger_download` is continue-on-error and load_shown_repeat_urls degrades on a + missing file, so gating would only swap one graceful degradation for another. The + JUDGE's download is the separate question `--no-judge-ledger` answers — when that + one failed the judge prompt carried an EMPTY block, so nothing was shown and this + job's own (possibly intact) copy of ledger.json must not stand in for it. """ WORKFLOW = os.path.join( @@ -2916,12 +3121,35 @@ def commands(self): out.append(cmd) return out - def test_all_three_invocations_carry_the_ledger_flag(self): + def test_all_three_invocations_carry_the_ledger_flags(self): cmds = self.commands() self.assertEqual(len(cmds), 3, "the branch count changed — re-check each one") for cmd in cmds: with self.subTest(cmd=cmd.splitlines()[1:3]): - self.assertIn("--ledger /tmp/ledger/ledger.json", cmd) + # Quoted and [@]-expanded, so an empty-vs-one-element array cannot + # collapse into a stray empty argv element. + self.assertIn('"${LEDGER_FLAGS[@]}"', cmd) + self.assertNotIn("--ledger /tmp/ledger/ledger.json", cmd) + + def test_the_flag_array_is_built_once_and_always_carries_the_path(self): + self.assertIn("LEDGER_FLAGS=(--ledger /tmp/ledger/ledger.json)", self.text) + self.assertEqual(self.text.count("LEDGER_FLAGS=("), 1) + + def test_the_judge_download_outcome_is_what_disables_membership(self): + """Not the poster's. `JUDGE_LEDGER_OK` is already computed for the header + banner and is the only variable that says what the JUDGE was shown; gating on + `LEDGER_DOWNLOAD` (this job's own fetch) would leave the membership check + vouching for URLs the judge never saw.""" + self.assertIn( + 'if [ "$JUDGE_LEDGER_OK" != "true" ]; then\n' + " LEDGER_FLAGS+=(--no-judge-ledger)", + self.text, + ) + # The array is assembled AFTER the variable it reads is set. + self.assertLess( + self.text.index("JUDGE_LEDGER_OK=true"), + self.text.index("LEDGER_FLAGS=("), + ) def test_the_path_is_the_one_the_download_step_writes(self): # `ledger_download` unpacks the artifact to /tmp/ledger; no new download step diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index a8a2269..b96a935 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2380,6 +2380,25 @@ jobs: fi fi + # What post-review.py adjudicates a judge `repeat_of` against (BE-12630). + # `--ledger` is passed UNCONDITIONALLY — the download is continue-on-error + # and load_shown_repeat_urls degrades on a missing file, so gating on the + # POSTER's download would only convert a graceful degradation into a + # different one. + # + # `--no-judge-ledger` is a different question with a different answer. The + # two downloads fail independently, and it is the JUDGE's that decides what + # the judge was SHOWN: when it failed, the judge prompt was spliced an EMPTY + # ledger block, so every repeat_of it emitted is unfounded — even though this + # job's own copy of ledger.json may have arrived intact and be full of real + # thread URLs. Without this flag the membership check would treat those URLs + # as "shown" and wave the re-raise through on exactly the degraded run + # $LEDGER_NOTE already banners above. + LEDGER_FLAGS=(--ledger /tmp/ledger/ledger.json) + if [ "$JUDGE_LEDGER_OK" != "true" ]; then + LEDGER_FLAGS+=(--no-judge-ledger) + fi + # Three cases: # 1. Judge ok → normal consolidated review. # 2. Judge failed, panel has findings → post the raw per-cell findings @@ -2393,7 +2412,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ - --ledger /tmp/ledger/ledger.json \ + "${LEDGER_FLAGS[@]}" \ --ledger-note "$LEDGER_NOTE" elif [ "${CONSOLIDATED_COUNT:-0}" != "0" ]; then # No JUDGE_ERROR is read here on purpose: this branch passes --notice with @@ -2411,7 +2430,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ - --ledger /tmp/ledger/ledger.json \ + "${LEDGER_FLAGS[@]}" \ --ledger-note "$LEDGER_NOTE" \ --notice "⚠️ The judge step did not submit a final review through its structured-output tool (status=${JUDGE_STATUS}). The findings below are the raw, un-adjudicated panel output — they may contain duplicates or false positives the judge would normally filter." else @@ -2435,7 +2454,7 @@ jobs: --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ - --ledger /tmp/ledger/ledger.json \ + "${LEDGER_FLAGS[@]}" \ --ledger-note "$LEDGER_NOTE" \ --error-message "Judge call failed (status=${JUDGE_STATUS}): ${JUDGE_ERROR}" fi From a27a6959810fa0af86c465e44a3d58e5e58789bf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 21:38:56 -0700 Subject: [PATCH 3/3] docs(cursor-review): a refused repeat link can cost the finding past the cap (BE-12630) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README claimed "The finding itself is never deleted for a bad link — only its lineage is." That holds only while the round is under REPEAT_CAP. enforce_repeat_cap counts `repeat_of or repeat_dropped`, so once the cap is spent the whole item is dropped from `enriched` — it is not moved to the body-only half or the over-the-cap comment, it is gone, disclosed only as a count in the cap banner. test_a_rejected_declaration_is_capped_exactly_like_ an_honest_one already pins that outcome (two kept, three dropped). State the boundary instead of denying it, and tie it back to the asymmetry the paragraph has just justified: the cap counts DECLARATIONS, so it cannot tell a refused link from an honest one, and that indistinguishability is the price of not refunding the slot rather than a second penalty. Raised by CodeRabbit on #279. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 20e2b64..033cbf2 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -77,7 +77,7 @@ With `ledger_prior_review` on (the default), [`build-ledger.py`](build-ledger.py A finding that could not be anchored to a line the reviewed diff carries is **demoted** to the review body, and one that lost its whole review to a failed POST is delivered as prose — neither gets a thread, so neither can be answered and neither costs a slot. But a demoted finding may itself have been a *re-raise*, and that lineage used to disappear with the trailer that was stripped out of its recovered body: the next round saw a fresh, thread-less, cap-exempt finding, so one demoted hop made every later re-raise of the same finding free. The sentinel now carries the ancestor's URL as a field, and the ledger resolves it against the PR's own comments: the trailing `discussion_r` must name a **root** comment of one of *our* consolidated reviews on this PR, so a hallucinated id resolves to nothing. Only that id is read — the owner/repo/PR-number half of the URL is shape-checked but never compared — so a link naming another repo resolves whenever its id matches one of our roots; what the entry renders is always the ancestor's *own* permalink, never the relayed string, and an ancestor with no permalink yields no lineage. The round is recovered the same way, from the review that ancestor belongs to, rather than from the payload. When it resolves, the entry renders `re_raise_of: (round N; ancestor_answers=)` — a key deliberately distinct from the `answers_from_author_or_maintainer` on the entry's own `thread:` line, since the two describe different findings — plus a `re_raise_answer:` excerpt of the ancestor's most recent answering reply, and the repeat rule is applied to that **ancestor's** answer state: with `k >= 1` a further re-raise must carry `repeat_of` and costs a slot, and with `k == 0` nothing changes. Resolution runs over every consolidated review before the round cap, so the ancestor is still found after its own round has aged out of the ledger. A claim that does *not* resolve — a dismissed review, a deleted comment — is not silently rebuilt as a fresh cap-exempt finding: the entry says the lineage is unverified and withholds the exemption. -A `repeat_of` is adjudicated on two independent layers, and neither is the other's backstop. The **writer** ([`post-review.py`](post-review.py)) checks SHAPE — one anchored GitHub discussion permalink under 512 characters, nothing else, applied to the rendered trailer and the structural field alike — and, given `--ledger`, MEMBERSHIP: the URL must be one the ledger the judge was actually shown carried, i.e. an *anchored* entry's `discussion_url` or the `repeat_of` lineage a *demoted* re-raise entry renders on its `re_raise_of:` line, mirroring the gates the render itself applies. The **reader** ([`build-ledger.py`](build-ledger.py)) separately resolves the trailing comment id against this PR's own consolidated-review roots, as above. Membership catches precisely what resolution cannot: an id that resolves but was never SHOWN — aged past the round cap, dropped by the byte cap, or never rendered — which the judge therefore had no basis to cite. A URL failing either check is dropped whole — no trailer, no sentinel field, nothing carried into the next round — while the judge's *declaration* that the finding is a re-raise still spends its `REPEAT_CAP` slot. That asymmetry is deliberate: refunding the slot would make a link we reject strictly cheaper than an honest one, so a judge could turn a whole round into uncapped re-litigation simply by citing threads it was never shown. The finding itself is never deleted for a bad link — only its lineage is. An `empty`, `disabled` or `unknown` ledger carries `entries: []`, and the judge prompt permits `repeat_of` only inside a ledger block, so on those runs the shown set is legitimately empty and every `repeat_of` is dropped. A ledger that cannot be read at all is the one case that must NOT drop anything — no path, no file, unreadable, not an object, no `entries` list — so it degrades to shape-only, exactly as before, the reader's half still holds, and the run carries a `::warning::` annotation so a silently disabled guard is visible rather than buried in a step log. The judge's and the poster's ledger downloads fail independently, and only the **judge's** decides what was shown: when that one fails the workflow passes `--no-judge-ledger` and the shown set is empty, because the judge prompt was spliced an empty ledger block however intact this job's own copy of `ledger.json` may be. +A `repeat_of` is adjudicated on two independent layers, and neither is the other's backstop. The **writer** ([`post-review.py`](post-review.py)) checks SHAPE — one anchored GitHub discussion permalink under 512 characters, nothing else, applied to the rendered trailer and the structural field alike — and, given `--ledger`, MEMBERSHIP: the URL must be one the ledger the judge was actually shown carried, i.e. an *anchored* entry's `discussion_url` or the `repeat_of` lineage a *demoted* re-raise entry renders on its `re_raise_of:` line, mirroring the gates the render itself applies. The **reader** ([`build-ledger.py`](build-ledger.py)) separately resolves the trailing comment id against this PR's own consolidated-review roots, as above. Membership catches precisely what resolution cannot: an id that resolves but was never SHOWN — aged past the round cap, dropped by the byte cap, or never rendered — which the judge therefore had no basis to cite. A URL failing either check is dropped whole — no trailer, no sentinel field, nothing carried into the next round — while the judge's *declaration* that the finding is a re-raise still spends its `REPEAT_CAP` slot. That asymmetry is deliberate: refunding the slot would make a link we reject strictly cheaper than an honest one, so a judge could turn a whole round into uncapped re-litigation simply by citing threads it was never shown. Under the cap a bad link costs the finding its lineage and nothing else. It is only when that declaration is the one that overruns `REPEAT_CAP` that the finding is cut whole — exactly as an honest over-cap re-raise is, since the cap counts *declarations* and so by construction cannot tell the two apart. That is the asymmetry's price, not a second penalty: a refused link never costs a finding its place while the round has slots left. An `empty`, `disabled` or `unknown` ledger carries `entries: []`, and the judge prompt permits `repeat_of` only inside a ledger block, so on those runs the shown set is legitimately empty and every `repeat_of` is dropped. A ledger that cannot be read at all is the one case that must NOT drop anything — no path, no file, unreadable, not an object, no `entries` list — so it degrades to shape-only, exactly as before, the reader's half still holds, and the run carries a `::warning::` annotation so a silently disabled guard is visible rather than buried in a step log. The judge's and the poster's ledger downloads fail independently, and only the **judge's** decides what was shown: when that one fails the workflow passes `--no-judge-ledger` and the shown set is empty, because the judge prompt was spliced an empty ledger block however intact this job's own copy of `ledger.json` may be. ### The panel