diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 4531332..033cbf2 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 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 | Lab | Model (Cursor catalog) | diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 63853e4..b2a2302 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -24,6 +24,25 @@ 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 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. `--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 @@ -1432,7 +1451,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 +1461,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,10 +1485,23 @@ 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. + 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, @@ -1478,7 +1515,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,21 +1536,127 @@ def normalize_comments(findings: list[dict]) -> list[dict]: return enriched -def render_repeat_of(finding: dict) -> str: - """Render the re-raise line for a finding the judge marked as a repeat. +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. - `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. + 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. """ - url = repeat_url_of(finding) if not url: return "" return f"\n\n↩︎ re-raise of {url}{render_repeat_round(finding)}" -def repeat_url_of(finding: dict) -> str: +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. + + 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. + + 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. + """ + 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, + ) + warn_membership_guard_off() + 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, + ) + 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 + # 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()) + 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 +1667,54 @@ 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. + + 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 "" - return neutralize_mentions(url.strip()) + 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. + # + # 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): @@ -1589,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 @@ -1695,6 +1886,24 @@ 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( + "--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, @@ -1779,7 +1988,24 @@ 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. + 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 @@ -1799,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 d048236..053baac 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -2621,5 +2621,541 @@ 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). 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}, + ], + } + ) + ) + ) + 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( + { + # `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, + {"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()})) + + + 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. + + 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"]) + # 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), (0, 1)) + # …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: 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"), + # 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 + ), + ] + + 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( + [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): + """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 + 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 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. + + 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. + + `--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( + 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_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]): + # 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 + # 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..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,6 +2412,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ + "${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 @@ -2410,6 +2430,7 @@ jobs: --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ + "${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 @@ -2433,6 +2454,7 @@ jobs: --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ --triggered-by "$TRIGGERED_BY" \ + "${LEDGER_FLAGS[@]}" \ --ledger-note "$LEDGER_NOTE" \ --error-message "Judge call failed (status=${JUDGE_STATUS}): ${JUDGE_ERROR}" fi