diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index b5bf61f9..7b72bf12 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -459,32 +459,40 @@ def _body_only_entries(review: dict, meta: dict, max_body: int): return [], BODY_ONLY_PROSE_MARKER in (review.get("body") or "") entries = [] for item in parsed: - entries.append( - { - "round": meta["round"], - "commit": meta["commit"], - "posted_at": meta["posted_at"], - "path": _body_only_text(item.get("path")), - "line": _body_only_line(item.get("line")), - "severity": _body_only_text(item.get("severity")), - "finding": _truncate(item.get("body") or "", max_body), - # Permanently unanswered, by construction: there is no thread to - # reply on. answered_count=0 is what makes these cap-EXEMPT, matching - # the existing rule that only an ANSWERED finding costs a repeat slot. - "thread": { - "resolved": False, - "outdated": False, - "reply_count": 0, - "answered_count": 0, - }, - "replies": [], - "dropped_replies": 0, - # No thread means no permalink. Rendered as an omitted line rather - # than an empty one, so the judge can never emit it as a `repeat_of`. - "discussion_url": "", - "anchored": False, - } - ) + entry = { + "round": meta["round"], + "commit": meta["commit"], + "posted_at": meta["posted_at"], + "path": _body_only_text(item.get("path")), + "line": _body_only_line(item.get("line")), + "severity": _body_only_text(item.get("severity")), + "finding": _truncate(item.get("body") or "", max_body), + # Permanently unanswered, by construction: there is no thread to + # reply on. answered_count=0 is what makes these cap-EXEMPT, matching + # the existing rule that only an ANSWERED finding costs a repeat slot. + "thread": { + "resolved": False, + "outdated": False, + "reply_count": 0, + "answered_count": 0, + }, + "replies": [], + "dropped_replies": 0, + # No thread means no permalink. Rendered as an omitted line rather + # than an empty one, so the judge can never emit it as a `repeat_of`. + "discussion_url": "", + "anchored": False, + } + # BE-10002: the writer marks a finding that anchored to the diff and lost its + # thread to a failed review POST, not to the diff. Everything mechanical above + # is still correct for it — there is no thread, so no discussion_url and no + # answer — and the flag changes only how the render explains it. Matched with + # `is True` rather than truthiness because the payload is model-adjacent text + # travelling through a public review body: a stray `"lost_to_fallback": "no"` + # must not read as the flag being set. + if item.get("lost_to_fallback") is True: + entry["lost_to_fallback"] = True + entries.append(entry) return entries, False @@ -739,7 +747,21 @@ def _size(items): for e in entries if e.get("anchored", True) and e["thread"]["answered_count"] == 0 ) - unanchorable = sum(1 for e in entries if not e.get("anchored", True)) + # Counted apart from `post_failed` for the same reason `unanswered` is counted + # apart from both: the block header is the first thing the model reads, and + # "N unanchorable, so never answerable at all" said of a finding whose own entry + # line two rows below reports that it DID anchor is the aggregate contradicting + # the detail. On a wholesale-fallback round that would be every finding of it. + unanchorable = sum( + 1 + for e in entries + if not e.get("anchored", True) and e.get("lost_to_fallback") is not True + ) + post_failed = sum( + 1 + for e in entries + if not e.get("anchored", True) and e.get("lost_to_fallback") is True + ) return { "status": "ok", @@ -750,6 +772,7 @@ def _size(items): "entry_count": len(entries), "unanswered_count": unanswered, "unanchorable_count": unanchorable, + "post_failed_count": post_failed, "notes": notes, # How many rounds demoted findings we could not read back, and how many notes # a SIZE cap produced. Both kept structurally rather than sniffed out of @@ -778,6 +801,7 @@ def unknown_ledger(call: str, reason: str) -> dict: "entry_count": 0, "unanswered_count": 0, "unanchorable_count": 0, + "post_failed_count": 0, "notes": [], "failed_call": call, "reason": reason, @@ -796,6 +820,7 @@ def disabled_ledger() -> dict: "entry_count": 0, "unanswered_count": 0, "unanchorable_count": 0, + "post_failed_count": 0, "notes": [], } @@ -833,6 +858,11 @@ def disabled_ledger() -> dict: " thread, so nobody COULD have answered it and the first bullet above does\n" " not apply to it. Re-raising it is legitimate; prefer not to unless its\n" " severity warrants, and say in the body that it repeats unanchored.\n" + "- An entry marked [post-failed] is like [unanchorable] for repeat purposes —\n" + " no thread exists, so nobody could have answered it — but UNLIKE it, the\n" + " finding passed the diff-anchor check and lost its thread to an API failure\n" + " that delivered the whole review as prose. Re-raise it if it still applies;\n" + " the \"prefer not to\" above is about unanchorable findings and not about it.\n" ) _JUDGE_STEERING = ( @@ -858,6 +888,11 @@ def disabled_ledger() -> dict: " could have answered it. If the same unanchorable finding appears in several\n" " recent rounds, prefer NOT re-raising it unless its severity warrants; if you\n" " do re-raise it, say in the body that it repeats unanchored.\n" + "- An entry marked [post-failed] has NO discussion_url and never takes repeat_of\n" + " either, and costs no repeat slot. But unlike [unanchorable] it DID pass the\n" + " diff-anchor check — its review was lost to an API failure and delivered as\n" + " prose — so the preference above does not apply to it:\n" + " re-raise it if it still holds.\n" ) @@ -882,9 +917,16 @@ def render_ledger_markdown(ledger: dict, audience: str = "panel") -> str: # their own clause, because saying "N never answered" of a finding nobody could # answer contradicts the per-entry line right below it. unanchorable = ledger.get('unanchorable_count') or 0 + post_failed = ledger.get('post_failed_count') or 0 counts = f"{ledger['unanswered_count']} never answered" if unanchorable: counts += f"; {unanchorable} unanchorable, so never answerable at all" + if post_failed: + # Its own clause, not folded into `unanchorable`: these findings DID pass the + # diff-anchor check, and the entry lines below say so. + counts += ( + f"; {post_failed} lost to a failed review POST, so never answerable either" + ) lines.append( f"Ledger: {ledger['entry_count']} prior finding(s) across " f"{ledger['rounds']} round(s) of {ledger['total_rounds']} total on this PR " @@ -906,6 +948,11 @@ def render_ledger_markdown(ledger: dict, audience: str = "panel") -> str: # Pre-BE-9565 entries (and every thread-derived one) are anchored; only a # finding recovered from a review body is not, so default to True. anchored = entry.get("anchored", True) + # Same disposition as any other thread-less entry; only the explanation + # differs. A forward-compatibility property too: a v1 payload written before + # BE-10002 (or by a consumer still pinned to an older SHA) carries no flag and + # renders exactly as it always did. + lost_to_fallback = entry.get("lost_to_fallback") is True # path/severity are defanged like the prose below. For a thread-derived entry # they came from GitHub, but a body-only entry relays them from model output # through the sentinel, and both land on the HEADER line. `_body_only_text` @@ -918,7 +965,7 @@ def render_ledger_markdown(ledger: dict, audience: str = "panel") -> str: if entry["severity"]: header += f" [{_defang_fences(entry['severity'])}]" if not anchored: - header += " [unanchorable]" + header += " [post-failed]" if lost_to_fallback else " [unanchorable]" # entry['path'], entry['severity'], entry['finding'] and reply['text'] are all # imported prose — the untrusted fields in this block. Defanged so none can # forge the fence that makes the block DATA. See _defang_fences. @@ -950,7 +997,19 @@ def render_ledger_markdown(ledger: dict, audience: str = "panel") -> str: # judge must not treat it as one. tag = " (third party — NOT an answer)" lines.append(f" reply from {who}{tag}: {_defang_fences(reply['text'])}\n") - if not anchored: + if not anchored and lost_to_fallback: + # Same "nobody could have answered it" as below, but the reason matters: + # this finding passed the diff-anchor check, so the steering that asks the + # panel to prefer not re-raising an unanchorable one would be wrong about + # it. Stated as the check it passed rather than as a promise about next + # round: the writer tags this from ITS parse of the diff, and the POST that + # failed may well have failed because GitHub refused an anchor anyway. + lines.append( + " (review POST failed — this finding matched a line in the reviewed " + "diff but its review was delivered body-only, so no thread exists and " + "nobody could answer it; re-raising it needs no repeat_of)\n" + ) + elif not anchored: # Stronger than "never answered": nobody COULD have answered it. Said # explicitly so the judge does not read a bare answered_count=0 as an # author who ignored the finding. @@ -1007,9 +1066,12 @@ def ledger_note(ledger: dict) -> str: # "3 prior finding(s) … (0 never answered)" — i.e. as though the author had answered # every one of them, when not one of them had a thread to answer. unanchorable = ledger.get('unanchorable_count') or 0 + post_failed = ledger.get('post_failed_count') or 0 counts = f"{ledger['unanswered_count']} never answered" if unanchorable: counts += f"; {unanchorable} unanchorable" + if post_failed: + counts += f"; {post_failed} lost to a failed review POST" return ( f"Round {ledger['total_rounds'] + 1} — ledger: {ledger['entry_count']} prior " f"finding(s) across {ledger['rounds']} round(s) " diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 9043014d..dc36d1b5 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -144,6 +144,29 @@ def emit_delivery(delivered: bool, gated: int = 0, ungated: int = 0) -> None: # round demoted nothing", so it is a constant here rather than a literal in the render. BODY_ONLY_PROSE_MARKER = "could not be anchored to a line the reviewed diff carries" BODY_ONLY_TRUNCATION_MARKER = " …[truncated]" +# What render_findings_markdown puts between the head and the first finding. A +# constant because the wholesale fallback's size guard has to measure everything that +# precedes finding one, and a guard that re-spelled this separator would drift from it. +FINDINGS_SEPARATOR = "\n\n---\n\n" +# What clamp_review_body appends in place of what it cut. Also a constant because the +# same size guard has to RESERVE it: the clamp cuts at `limit - len(note)`, so a head +# that merely fits under the limit can still have its tail — the sentinel — taken. +CLAMP_TRUNCATION_NOTE = ( + "\n\n_…truncated here: the review body reached GitHub's size limit. As much " + "of it as fits is in the job summary of this run._" +) +# The share of the fallback body the sentinel may take. It has TWO readers and the +# HUMAN comes first: the prose findings are the review a person actually reads on the +# PR, and the sentinel is a best-effort machine-readable copy for next round's ledger. +# Uncapped, the sentinel wins that contest — its per-finding JSON is nearly as long as +# the prose entry it duplicates, so it can consume the whole budget ahead of finding +# one and leave the clamp nothing but the head to keep. Measured before this cap: 89 +# findings of ~700 chars posted 58,720 characters of JSON and rendered ZERO findings, +# while the same round at 90 findings — one over the all-or-nothing guard, so the +# sentinel was dropped whole — rendered 79 of them. The cliff ran the wrong way. +# Half the budget is the prose FLOOR; the sentinel takes the most-urgent prefix of the +# findings that fits the other half (see fit_sentinel_items). +FALLBACK_SENTINEL_MAX_CHARS = MAX_REVIEW_BODY_CHARS // 2 def normalize_severity(value) -> str: @@ -801,10 +824,7 @@ def clamp_review_body(body: str, limit: int = MAX_REVIEW_BODY_CHARS) -> str: body = encodable(body) if len(body) <= limit: return body - note = ( - "\n\n_…truncated here: the review body reached GitHub's size limit. As much " - "of it as fits is in the job summary of this run._" - ) + note = CLAMP_TRUNCATION_NOTE if limit <= len(note): # Degenerate limit (tests, a future tightening): the cut still has to hold. return body[:limit] @@ -953,9 +973,17 @@ def render_body_only_sentinel(items: list) -> str: That blanket replace is safe because the only JSON tokens outside string literals here are `[`, `]`, `{`, `}`, `,`, `:` and the digits of `line` — normalize_comments guarantees `line` is a POSITIVE int, so no `-` can appear as a number's sign. + + `lost_to_fallback` (BE-10002) is emitted ONLY for an item that carries it, so a + success-path payload stays byte-identical to what this rendered before the key + existed. It marks a finding that anchored fine and lost its thread to the failed + POST rather than to the diff — presentation only on the reading side, since the + mechanical consequences of `anchored: false` are correct for it either way. The + key name carries no `-`, so the escape above already covers it. """ - payload = [ - { + payload = [] + for item in items: + entry = { # neutralize_mentions, like render_code_ref does for the prose half. The # body was already neutralized in normalize_comments, but `path` is raw # model output until it is rendered — and this render is still a POSTed @@ -971,13 +999,52 @@ def render_body_only_sentinel(items: list) -> str: ) ), } - for item in items - ] + # `is True`, matching build-ledger.py's reader exactly. It reads the key that + # way so a stray `"lost_to_fallback": "no"` in a relayed payload cannot count + # as set; emitting on mere truthiness here would normalize such a value to + # JSON `true` and defeat that guard from the writing side. + if item.get("lost_to_fallback") is True: + entry["lost_to_fallback"] = True + payload.append(entry) encoded = json.dumps(payload, ensure_ascii=False, separators=(",", ":")) escaped = encoded.replace("-", "\\u002d") return f"" +def fit_sentinel_items(items: list, budget: int) -> list: + """The longest leading run of `items` whose rendered sentinel fits `budget` chars. + + A PREFIX rather than a subset, because `items` arrives most → least urgent: the + findings kept are the ones next round most needs back, and they stay in the same + order as the prose below them. `[]` means "nothing fits" — the caller drops the + sentinel and posts the prose marker alone, which is what this path did before the + sentinel existed and which the ledger still reads as a disclosed truncation. + + Recovering SOME findings is strictly better than the all-or-nothing rule it + replaces: that rule made the sentinel free to eat the whole body budget as long as + it fit at all, so the round with the most findings to report was the one that + rendered none of them. A partial sentinel is not a partial truth on the reading + side either — the findings it leaves out are simply absent from the ledger, exactly + as all of them were when the whole sentinel was dropped. + + Binary search: the render grows monotonically with the prefix length, so the + boundary is well defined and found in ~log2(n) renders rather than n. + """ + if budget <= 0 or not items: + return [] + if len(render_body_only_sentinel(items)) <= budget: + return items + # Invariant: `lo` fits (or is 0, the drop case the caller handles), `hi` does not. + lo, hi = 0, len(items) + while lo < hi - 1: + mid = (lo + hi) // 2 + if len(render_body_only_sentinel(items[:mid])) <= budget: + lo = mid + else: + hi = mid + return items[:lo] + + def render_body_only_findings(items: list) -> str: """Render findings that could not be anchored, for inclusion in the review body.""" if not items: @@ -1013,7 +1080,7 @@ def render_findings_markdown(review_body: str, comments: list[dict]) -> str: """ md = review_body if comments: - md += "\n\n---\n\n" + md += FINDINGS_SEPARATOR for c in comments: md += render_finding_entry(c) + "\n\n" return md @@ -1335,7 +1402,11 @@ def main(): # 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 # anchor missed would misreport the review. - inline_items, body_only_items = partition_by_anchor(enriched, load_anchors(args.diff)) + # `anchors` is kept, not just consumed: None means the diff could not be read and + # partition_by_anchor failed OPEN without testing a single finding, which the + # wholesale fallback below has to know before it can claim anything anchored. + anchors = load_anchors(args.diff) + inline_items, body_only_items = partition_by_anchor(enriched, anchors) comments = [item["comment"] for item in inline_items] # The head is every finding-independent part of the review. Kept separate from the @@ -1366,11 +1437,10 @@ def main(): # build-ledger.py parses back out of this body, so a fully-demoted round can no # longer read as a review that found nothing. Those entries are permanently # UNANSWERED, hence cap-exempt, which is the same rule an unanswered thread - # already gets. What is still open: the WHOLESALE fallback body (the 422 path - # below) carries no sentinel, because it is not rendered through - # render_body_only_findings — so its findings do not reach the ledger. It does - # carry the prose marker, so that round degrades loudly rather than silently. - review_body += f"\n\n---\n\n{body_only_md}" + # already gets. Since BE-10002 the WHOLESALE fallback body (the 422 path below) + # carries a sentinel of its own too, so a round whose findings were lost to a + # failed POST reaches the ledger as well. + review_body += f"{FINDINGS_SEPARATOR}{body_only_md}" # Every finding, most → least urgent, for any render that has no inline half. prose_body = render_findings_markdown(review_head, [i["comment"] for i in enriched]) @@ -1437,13 +1507,11 @@ def main(): # Fallback: same findings without inline anchors. Typical cause is line # numbers that fall outside the diff context — often the model picked # a line near the change but not on the change. - # The note carries BODY_ONLY_PROSE_MARKER deliberately. This body has no sentinel — - # it is not rendered through render_body_only_findings, and adding one is the wrong - # move on a request that just 422'd for being unacceptable (see the PR's Residual). - # But without the marker, next round's build_ledger saw neither entries NOR a - # degradation for a round on which EVERY finding is body-only, so a fallback-posted - # round read as a review that found nothing and the round after it looked like a - # first round. The marker alone costs a sentence and keeps the disclosure honest. + # The note carries BODY_ONLY_PROSE_MARKER deliberately. Without it, next round's + # build_ledger saw neither entries NOR a degradation for a round on which EVERY + # finding is body-only, so a fallback-posted round read as a review that found + # nothing and the round after it looked like a first round. The marker alone costs a + # sentence and keeps the disclosure honest. # # It goes in the HEAD, for exactly the reason the section marker had to move above # the sentinel: clamp_review_body cuts the TAIL. Appended after the findings the @@ -1459,8 +1527,94 @@ def main(): "below instead. None of them has a review thread, so there is nowhere to " "reply to one or resolve it.)_" ) + # …and the sentinel goes DIRECTLY under that note (BE-10002), in the same + # marker → sentinel → prose order render_body_only_findings uses and for the same + # two reasons: build-ledger.py accepts a sentinel only when the marker line sits + # immediately above it, and a tail clamp then eats the least-urgent PROSE rather + # than the JSON. Until this, the fallback posted the marker alone, so the ledger + # disclosed the degradation loudly and recovered ZERO entries — including for the + # findings that anchored perfectly well and lost their thread only to the failed + # POST. Every finding of the round is OFFERED to it — the ones from `inline_items` + # tagged `lost_to_fallback`, the ones already unanchorable left untagged, since the + # POST outcome changed nothing for them — and the size guard below decides how many + # of them the body can actually afford to carry. + # + # Residual (BE-10002): a nonzero `gh` result is not PROOF the review was not + # committed server-side — the `not comments` branch above declines the fallback for + # exactly that reason. If the first POST did land, its findings have real threads + # and also land in this sentinel, so next round's ledger carries each of them + # twice: once with its thread and any reply on it, once as a cap-exempt + # [post-failed] entry saying nobody could have answered it. Keying the tag on a + # confirmed-absent thread would need a read of the PR's reviews this script does + # not do, so it is written down here rather than fixed. + # + # Tagged by identity, not by value: `inline_items` and `body_only_items` hold the + # very objects `enriched` does, and two findings can be equal without being the + # same one. Iterating `enriched` is what keeps the sentinel in the same most → + # least urgent order as the prose below it. + # + # And tagged only where the anchors were actually CHECKED. With `anchors is None` + # partition_by_anchor put every finding inline without testing one, so + # `inline_items` is not evidence of anything — least of all on a 422, whose typical + # cause IS an anchor GitHub would not take. Untagged, those findings render as + # [unanchorable]: the conservative reading, and the one this path gave them before + # BE-10002. The claim the flag makes is "this passed the diff-anchor check", and + # that is a claim only a real check can make. + lost_ids = {id(item) for item in inline_items} if anchors is not None else set() + sentinel_items = [ + {**item, "lost_to_fallback": True} if id(item) in lost_ids else item + for item in enriched + ] + # Size guard, in two parts. + # + # A PROSE FLOOR first. The sentinel duplicates the findings in JSON at nearly the + # length of the prose entries below it, so left to take whatever fits it displaces + # the review a human reads: measured at 89 findings it posted 58,720 characters of + # comment and rendered no findings at all, where the same round one finding larger + # dropped the sentinel and rendered 79. The sentinel gets at most half the body; + # fit_sentinel_items then keeps the most-urgent prefix that fits, so a round too big + # for a whole sentinel recovers part of one instead of none of it. + # + # The clamp's own note is RESERVED in that budget, not merely the limit tested. The + # clamp cuts at `limit - len(note)`, so a head+sentinel that fits the limit by less + # than that can still be cut mid-JSON — and drop_unterminated_comment then removes + # the sentinel back to its opener, taking every finding after it with it. That was a + # ~120-char window (measured: 89 findings, one long path) in which the review + # collapsed from 60,000 characters of findings to a 494-character header. The + # sentinel is posted whole or not at all; it is never posted where the clamp cuts. + sentinel_budget = min( + FALLBACK_SENTINEL_MAX_CHARS, + MAX_REVIEW_BODY_CHARS + - len(CLAMP_TRUNCATION_NOTE) + - len(fallback_head) + - len("\n\n") + - len(FINDINGS_SEPARATOR), + ) + kept = fit_sentinel_items(sentinel_items, sentinel_budget) + if kept: + fallback_head_with_sentinel = ( + f"{fallback_head}\n\n{render_body_only_sentinel(kept)}" + ) + if len(kept) < len(sentinel_items): + print( + f"Review: the fallback's body-only sentinel carries the " + f"{len(kept)} most urgent of {len(sentinel_items)} finding(s) — the " + "rest would have displaced the findings a reader can see.", + file=sys.stderr, + ) + else: + # Nothing fits: post exactly what this path posted before the sentinel existed. + # The prose marker is still in the head, so next round's ledger reads a + # disclosed truncation rather than a round that found nothing. + print( + "Review: no part of the fallback's body-only sentinel fits under the size " + "limit — posting the marker alone, so next round's ledger discloses the " + "loss instead of recovering the findings.", + file=sys.stderr, + ) + fallback_head_with_sentinel = fallback_head fallback_body = render_findings_markdown( - fallback_head, [i["comment"] for i in enriched] + fallback_head_with_sentinel, [i["comment"] for i in enriched] ) clamped_fallback = clamp_review_body(fallback_body) fallback_payload = json.dumps( diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 6715596e..8a7ade33 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -1410,5 +1410,138 @@ def test_the_prose_fallback_marker_still_matches_what_post_review_renders(self): ) +class TestLostToFallbackEntries(unittest.TestCase): + """The wholesale-fallback half of the body-only channel (BE-10002). + + When the review POST fails, post-review.py re-posts every finding as prose — the + ones that anchored fine included. They reach the ledger through the same sentinel + as a demoted finding and get the same disposition (no thread, no discussion_url, + answered_count 0, cap-exempt), because all of that is factually true of them. What + is NOT true of them is the steering that asks the panel to prefer not re-raising an + unanchorable finding: these anchored, and will anchor again. Hence one flag, and a + render that says which of the two happened. + """ + + def _section(self, findings, lost_lines=()): + """The demoted-findings section, as post-review.py renders it, with the named + lines tagged the way the fallback branch tags them.""" + items = pr.normalize_comments(findings) + tagged = [ + {**item, "lost_to_fallback": True} + if item["comment"]["line"] in lost_lines + else item + for item in items + ] + return pr.render_body_only_findings(tagged) + + def _ledger(self, findings, lost_lines=()): + section = self._section(findings, lost_lines) + return bl.build_ledger( + [review_with_demoted(101, 1, findings, section=section)], [], [] + ) + + def test_the_flag_survives_the_round_trip_through_the_real_writer(self): + ledger = self._ledger([demoted("app.py", 11), demoted("far.py", 900)], lost_lines=(11,)) + by_line = {e["line"]: e for e in ledger["entries"]} + self.assertTrue(by_line[11]["lost_to_fallback"]) + self.assertNotIn("lost_to_fallback", by_line[900]) + # Everything mechanical is the SAME for both — the flag is presentation only. + for entry in ledger["entries"]: + self.assertFalse(entry["anchored"]) + self.assertEqual(entry["discussion_url"], "") + self.assertEqual(entry["thread"]["answered_count"], 0) + # Counted apart, never merged: the block header reports each in its own + # clause, so it can never say "unanchorable, so never answerable at all" of + # the finding whose entry line below reports that it matched the diff. + self.assertEqual(ledger["unanchorable_count"], 1) + self.assertEqual(ledger["post_failed_count"], 1) + self.assertEqual(ledger["unanswered_count"], 0, "nobody could have answered either") + + def test_the_two_thread_less_totals_are_reported_separately(self): + ledger = self._ledger( + [demoted("app.py", 11), demoted("far.py", 900)], lost_lines=(11,) + ) + header = bl.render_ledger_markdown(ledger, "judge").split("--- ROUND 1", 1)[0] + self.assertIn("1 unanchorable, so never answerable at all", header) + self.assertIn("1 lost to a failed review POST, so never answerable either", header) + self.assertNotIn("2 unanchorable", header) + note = bl.ledger_note(ledger) + self.assertIn("1 unanchorable", note) + self.assertIn("1 lost to a failed review POST", note) + + def test_a_round_with_no_post_failed_entry_reads_exactly_as_before(self): + """The new clause is added, never substituted: an ordinary demoted round's + header and note keep the wording they had.""" + ledger = self._ledger([demoted("far.py", 900)]) + header = bl.render_ledger_markdown(ledger, "judge").split("--- ROUND 1", 1)[0] + self.assertIn("1 unanchorable, so never answerable at all", header) + self.assertNotIn("failed review POST", header) + self.assertNotIn("failed review POST", bl.ledger_note(ledger)) + + def test_it_renders_post_failed_and_says_the_finding_did_anchor(self): + rendered = bl.render_ledger_markdown( + self._ledger([demoted("app.py", 11, severity="low")], lost_lines=(11,)), "judge" + ) + self.assertIn("* app.py:11 [low] [post-failed]", rendered) + self.assertNotIn("* app.py:11 [low] [unanchorable]", rendered) + # Worded as the check the finding PASSED, not as a promise about next round: + # the writer tags this from its own parse of the diff, and the POST that failed + # may have failed because GitHub refused an anchor anyway. + self.assertIn( + "(review POST failed — this finding matched a line in the reviewed diff " + "but its review was delivered body-only, so no thread exists and nobody " + "could answer it; re-raising it needs no repeat_of)", + rendered, + ) + self.assertNotIn("should anchor normally", rendered) + # The unanchorable note is the OTHER branch — an entry gets one, never both. + self.assertNotIn("demoted to the review body, no thread exists", rendered) + self.assertNotIn("discussion_url:", rendered, "still no thread to point at") + + def test_both_audiences_are_told_what_post_failed_means(self): + ledger = self._ledger([demoted("app.py", 11)], lost_lines=(11,)) + for audience in ("panel", "judge"): + with self.subTest(audience=audience): + steering = bl.render_ledger_markdown(ledger, audience).split( + "--- ROUND 1", 1 + )[0] + self.assertIn("[post-failed]", steering) + self.assertIn("diff-anchor check", steering) + self.assertIn("re-raise it if it still", steering.lower()) + # Not "freely": a [post-failed] entry carries no repeat lineage, so + # steering that waves the panel through amplifies the one bypass the + # repeat cap cannot see. + self.assertNotIn("freely", steering.lower()) + + def test_a_v1_payload_without_the_flag_renders_exactly_as_before(self): + """Forward compatibility in the direction that actually happens: consumers stay + pinned to older SHAs, so bodies written without the key keep arriving.""" + findings = [demoted("far.py", 900, severity="low")] + plain = bl.render_ledger_markdown(self._ledger(findings), "judge") + self.assertIn("* far.py:900 [low] [unanchorable]", plain) + self.assertNotIn("[post-failed]", plain.split("--- ROUND 1", 1)[1]) + self.assertIn("cannot be answered or resolved; re-raising needs no repeat_of", plain) + + def test_only_a_real_true_sets_the_flag(self): + """The payload is model-adjacent text travelling through a public review body, + so a near-miss value must not read as the flag being set.""" + for value in ("true", "false", 1, 0, None, [], {"a": 1}): + with self.subTest(value=value): + section = replace_payload( + body_only_section([demoted("far.py", 900)]), + json.dumps( + [{"path": "far.py", "line": 900, "severity": "low", + "body": "x", "lost_to_fallback": value}] + ).replace("-", "\\u002d"), + ) + ledger = bl.build_ledger( + [review(101, 1, body=f"{MARKER}\n\nFound **1** finding(s).\n\n---\n\n{section}")], + [], [], + ) + self.assertEqual(ledger["entry_count"], 1, "the entry is still recovered") + self.assertNotIn("lost_to_fallback", ledger["entries"][0]) + self.assertIn("[unanchorable]", bl.render_ledger_markdown(ledger, "judge")) + + if __name__ == "__main__": unittest.main() diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index a5e650dd..b0be1524 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -46,11 +46,47 @@ # The sentence build-ledger.py keys on to tell "this round demoted findings and the # sentinel is unreadable" apart from "this round demoted nothing". Duplicated as a -# literal on purpose: test_build_ledger.py pins it against PROSE_MARKER, -# so a reword that breaks the contract fails there, and this file stays free of a -# build-ledger import it otherwise has no use for. +# literal on purpose: test_build_ledger.py pins it against PROSE_MARKER, so a reword +# that breaks the contract fails there rather than being quietly carried along here. PROSE_MARKER = "could not be anchored to a line the reviewed diff carries" +# build-ledger.py, for the fallback round-trips below (BE-10002). What the wholesale +# fallback body is FOR, once it carries a sentinel, is what the next round's ledger can +# read back out of it — so those tests drive the real parser rather than a copy of it +# living here, which would pin only itself. +_BL_SPEC = importlib.util.spec_from_file_location( + "build_ledger", os.path.join(os.path.dirname(__file__), "..", "build-ledger.py") +) +BL = importlib.util.module_from_spec(_BL_SPEC) +_BL_SPEC.loader.exec_module(BL) + + +def ledger_from_posted_body(body): + """Build the ledger a NEXT round would see from one posted review body.""" + review = { + "id": 101, + "state": "COMMENTED", + "commit_id": "abc1234567", + "submitted_at": "2026-07-01T00:00:00Z", + "body": body, + "user": {"login": "github-actions[bot]", "type": "Bot"}, + } + return BL.build_ledger([review], [], []) + + +def visible(body): + """`body` with the sentinel comment line removed. + + The sentinel renders as NOTHING on the PR, so any assertion about what a reader + sees has to drop it first — `len(body)` counts tens of thousands of characters of + HTML comment and stays large on exactly the body that shows no findings at all. + """ + return "\n".join( + ln + for ln in body.splitlines() + if not ln.startswith(f"") + ].replace("\\u002d", "-") + ) + self.assertIs(payload[0]["lost_to_fallback"], True) + # …and the key itself carries no `-`, so the dash escape still leaves the + # comment unclosable. + self.assertNotIn("--", PR.render_body_only_sentinel(tagged)[len("")]) + def test_no_sentinel_when_nothing_was_demoted(self): posted = EndToEndPostTest().run_main([finding("app.py", 11)])[0]["body"] self.assertNotIn(PR.BODY_ONLY_SENTINEL_PREFIX, posted)