From a57f1e5f001f819a7b81afa19bf81657a5417efc Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 5 Sep 2026 10:41:23 -0700 Subject: [PATCH 1/3] feat(cursor-review): carry the 422 fallback's findings into the ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wholesale fallback posts every finding of the round as prose when the inline review POST fails, and carried the body-only prose MARKER with no sentinel — so the next round's ledger disclosed the loss loudly and recovered zero entries, including for the findings that anchored perfectly well and lost their thread only to the failed POST. The fallback body now composes head-first (marker note → sentinel → findings prose, the same load-bearing order render_body_only_findings uses), so the ledger regex sees the marker line immediately above the sentinel and a tail clamp eats the least-urgent prose rather than the JSON. Findings that came from the inline half are tagged `lost_to_fallback: true`; already-unanchorable ones are not, since the POST outcome changed nothing for them. The reader keeps the same disposition for both (anchored false, no discussion_url, answered_count 0, cap-exempt — all factually true of a finding with no thread) and changes only the presentation: `[post-failed]` on the header line, a note saying the finding DID anchor and was lost to an API failure, plus one steering bullet each for the panel and the judge so neither is told to prefer not re-raising a finding that will anchor again. The size guard drops the sentinel entirely when it would not fit ahead of the first finding, which reproduces today's disclosed degradation as the floor. It reserves the clamp's own truncation note rather than testing the raw limit: the clamp cuts at `limit - len(note)`, so a head+sentinel fitting the limit by less than that was cut mid-JSON and drop_unterminated_comment then removed the sentinel AND every finding after it — a ~120-character window in which the review collapsed from 60,000 characters of findings to a 494-character header. No sentinel version bump: `lost_to_fallback` is an optional per-item key an older parser ignores, and it is emitted only for an item that carries it, so a success-path payload stays byte-identical. --- .github/cursor-review/build-ledger.py | 88 +++++--- .github/cursor-review/post-review.py | 105 +++++++-- .../cursor-review/tests/test_build_ledger.py | 100 ++++++++ .../cursor-review/tests/test_post_review.py | 213 ++++++++++++++++-- 4 files changed, 434 insertions(+), 72 deletions(-) diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index b5bf61f9..9a1cc7ea 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 @@ -833,6 +841,10 @@ 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 anchored to the diff correctly and was lost to an API failure that\n" + " delivered the whole review as prose. Re-raise it freely if it still applies.\n" ) _JUDGE_STEERING = ( @@ -858,6 +870,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 anchor to\n" + " the diff — its review was lost to an API failure and delivered as prose — so\n" + " the preference above does not apply: re-raise it freely if it still holds,\n" + " and it should anchor normally this round.\n" ) @@ -906,6 +923,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 +940,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 +972,17 @@ 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 DID anchor, so the steering that asks the panel to prefer + # not re-raising an unanchorable one would be wrong about it. + lines.append( + " (review POST failed — this finding anchored to the diff but its " + "review was delivered body-only, so no thread exists and nobody could " + "answer it; re-raising it needs no repeat_of and it should anchor " + "normally next round)\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. diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 9043014d..a4df5950 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -144,6 +144,17 @@ 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._" +) def normalize_severity(value) -> str: @@ -801,10 +812,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 +961,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,8 +987,9 @@ def render_body_only_sentinel(items: list) -> str: ) ), } - for item in items - ] + if item.get("lost_to_fallback"): + entry["lost_to_fallback"] = True + payload.append(entry) encoded = json.dumps(payload, ensure_ascii=False, separators=(",", ":")) escaped = encoded.replace("-", "\\u002d") return f"" @@ -1013,7 +1030,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 @@ -1366,11 +1383,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 +1453,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 +1473,55 @@ 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 in it: the ones from `inline_items` tagged + # `lost_to_fallback`, the ones already unanchorable left untagged, since the POST + # outcome changed nothing for them. + # + # 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. + lost_ids = {id(item) for item in inline_items} + sentinel_items = [ + {**item, "lost_to_fallback": True} if id(item) in lost_ids else item + for item in enriched + ] + fallback_head_with_sentinel = ( + f"{fallback_head}\n\n{render_body_only_sentinel(sentinel_items)}" + ) + # Size guard: the sentinel is a best-effort addition, never a reason for the clamp + # to reach the note above it. If it would not fit ahead of the first finding — so + # many findings that their JSON overruns the limit by itself, or a huge panel + # summary — drop it and post exactly what this path posted before, which the ledger + # still reads as a disclosed truncation rather than a silent round. + # + # The clamp's own note is RESERVED here, 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 is 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. Reserving + # the note closes it: the sentinel is either posted whole or not posted at all. + if ( + len(fallback_head_with_sentinel) + len(FINDINGS_SEPARATOR) + > MAX_REVIEW_BODY_CHARS - len(CLAMP_TRUNCATION_NOTE) + ): + print( + "Review: the fallback's body-only sentinel does not fit 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..585b5543 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -1410,5 +1410,105 @@ 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) + self.assertEqual(ledger["unanchorable_count"], 2) + self.assertEqual(ledger["unanswered_count"], 0, "nobody could have answered either") + + 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) + self.assertIn( + "(review POST failed — this finding anchored to the diff but its review was " + "delivered body-only, so no thread exists and nobody could answer it; " + "re-raising it needs no repeat_of and it should anchor normally next round)", + 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("anchored", steering) + self.assertIn("raise it 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..afe9a63d 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -46,11 +46,33 @@ # 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], [], []) + # Two files, so a per-file anchor set has something to be wrong about. `app.py` has two # hunks; `util.py` has one. Line numbers are the NEW side, exactly what a finding cites. DIFF = """diff --git a/app.py b/app.py @@ -487,10 +509,15 @@ def test_the_clamp_cuts_the_least_urgent_finding_in_the_fallback(self): # The fallback body used to be review_body (ending with the DEMOTED half) plus # the inline half appended after it, so clamping from the end dropped the # findings that had just lost their anchors while a demoted nit survived. - findings = [finding("app.py", 11, severity="critical", body="C" * 19000)] - findings += [finding("elsewhere.py", 7, severity="low", body="L" * 19000)] - findings += [finding("app.py", 12, severity="high", body="H" * 19000)] - findings += [finding("elsewhere.py", 8, severity="medium", body="M" * 19000)] + # + # Bodies are sized to fill the budget with the SENTINEL in place (BE-10002), + # which takes up to BODY_ONLY_SENTINEL_BODY_CHARS per finding out of it. The + # property pinned is the ordering, not the exact capacity: what the clamp takes + # is still the least urgent finding, and never the head above them. + findings = [finding("app.py", 11, severity="critical", body="C" * 18000)] + findings += [finding("elsewhere.py", 7, severity="low", body="L" * 18000)] + findings += [finding("app.py", 12, severity="high", body="H" * 18000)] + findings += [finding("elsewhere.py", 8, severity="medium", body="M" * 18000)] posted = EndToEndPostTest().run_main( findings, post_returncode=1, stderr="gh: Unprocessable Entity (HTTP 422)" ) @@ -499,22 +526,30 @@ def test_the_clamp_cuts_the_least_urgent_finding_in_the_fallback(self): # Severity order now holds across BOTH halves, so the cut lands on the low. # Before, the demoted half came first and the inline half was appended after # it, so the high (inline) was dropped whole while the low (demoted) survived. - self.assertIn("C" * 19000, body, "the critical survives") - self.assertIn("H" * 19000, body, "so does the high") - self.assertIn("M" * 19000, body, "and the medium") - self.assertNotIn("L" * 19000, body, "the low is what gets cut") - - def test_the_fallback_round_degrades_loudly_in_the_next_round_s_ledger(self): - """The fallback body carries no sentinel — it is not rendered through - render_body_only_findings, and adding bytes to a request that just 422'd is the - wrong move. But on this path EVERY finding is body-only, so without the prose - marker next round's build_ledger saw neither entries nor a degradation: the - round read as a review that found nothing, and the round after it looked like a - first round. The marker costs one sentence and keeps the disclosure honest. + self.assertIn("C" * 18000, body, "the critical survives") + self.assertIn("H" * 18000, body, "so does the high") + self.assertIn("M" * 18000, body, "and the medium") + self.assertNotIn("L" * 18000, body, "the low is what gets cut") + # …and the low is still recoverable from the sentinel above the cut, which is + # the ONLY copy of it that survives on this path. + ledger = ledger_from_posted_body(body) + self.assertEqual(ledger["entry_count"], 4) + self.assertTrue( + any(e["finding"].startswith("L" * 100) for e in ledger["entries"]), + "the cut finding is still recoverable from the sentinel above the cut", + ) + + def test_the_fallback_round_carries_its_findings_into_the_next_round_s_ledger(self): + """The fallback body used to carry the prose marker and NO sentinel, so the + next round's ledger disclosed the loss loudly and recovered zero entries — for + every finding of the round, including the ones that anchored perfectly well and + lost their thread only to the failed POST. Since BE-10002 it carries a sentinel + too, and a lost-to-the-POST finding is marked so the render can say which of the + two things happened to it. PROSE_MARKER is pinned to build-ledger.py's BODY_ONLY_PROSE_MARKER by test_build_ledger.py, which also pins that a marker with no readable sentinel - degrades loudly — so those two plus this one are the whole chain. + still degrades loudly — the floor this sits on top of. """ # One anchorable finding, so there IS an inline half to drop and the wholesale # fallback actually runs; one off-diff, so the round has a demoted half too. @@ -526,7 +561,115 @@ def test_the_fallback_round_degrades_loudly_in_the_next_round_s_ledger(self): fallback = posted[1] self.assertNotIn("comments", fallback, "this is the wholesale fallback") self.assertIn(PROSE_MARKER, fallback["body"]) - self.assertNotIn(PR.BODY_ONLY_SENTINEL_PREFIX, fallback["body"], "no sentinel here") + self.assertIn(PR.BODY_ONLY_SENTINEL_PREFIX, fallback["body"], "and now a sentinel") + + ledger = ledger_from_posted_body(fallback["body"]) + self.assertEqual(ledger["entry_count"], 2, "both findings reach the ledger") + by_line = {e["line"]: e for e in ledger["entries"]} + # The one that anchored: its thread died with the POST, not with the diff. + self.assertTrue(by_line[11]["lost_to_fallback"]) + self.assertFalse(by_line[11]["anchored"], "it still has no thread") + self.assertEqual(by_line[11]["discussion_url"], "") + self.assertEqual(by_line[11]["thread"]["answered_count"], 0, "so it is cap-exempt") + # The one that never could have anchored is unchanged — the POST outcome + # told us nothing about it. + self.assertNotIn("lost_to_fallback", by_line[999]) + + rendered = BL.render_ledger_markdown(ledger, "judge") + self.assertIn("* app.py:11 [medium] [post-failed]", rendered) + self.assertIn("* app.py:999 [medium] [unanchorable]", rendered) + self.assertIn("this finding anchored to the diff", rendered) + + def test_a_fallback_too_large_for_a_sentinel_still_degrades_loudly(self): + """The size guard's floor. When the head plus the note plus the sentinel would + not fit under the limit on their own, the sentinel is dropped entirely and this + path posts exactly what it posted before BE-10002 — the marker alone, which the + next round reads as a disclosed truncation rather than as a silent round. + + Reachable through the sentinel's own size: it carries up to + BODY_ONLY_SENTINEL_BODY_CHARS per finding and the panel path has no count cap, + so a round with enough findings overruns MAX_REVIEW_BODY_CHARS on the JSON. + """ + findings = [finding("app.py", 11, body="anchorable " + "z" * 700)] + findings += [ + finding("app.py", 900 + n, body=f"demoted {n} " + "z" * 700) for n in range(120) + ] + posted = EndToEndPostTest().run_main( + findings, post_returncode=1, stderr="gh: Unprocessable Entity (HTTP 422)" + ) + fallback = posted[1]["body"] + self.assertNotIn( + PR.BODY_ONLY_SENTINEL_PREFIX, fallback, "no sentinel fits, so none is posted" + ) + self.assertIn(PROSE_MARKER, fallback, "but the disclosure still is") + self.assertLessEqual(len(fallback), PR.MAX_REVIEW_BODY_CHARS) + + ledger = ledger_from_posted_body(fallback) + self.assertEqual(ledger["entry_count"], 0, "today's behaviour, preserved as the floor") + self.assertEqual(ledger["unrecovered_rounds"], 1) + self.assertIn("could not recover the finding(s)", BL.ledger_note(ledger)) + + def test_the_sentinel_is_never_posted_where_the_clamp_would_cut_it(self): + """The size guard reserves the clamp's own note, not just the limit. + + The clamp cuts at `limit - len(note)`, so a head+sentinel that fits the limit + by less than that gets cut mid-JSON — and drop_unterminated_comment then takes + the sentinel back to its opener AND every finding after it. Measured before the + reserve: a ~120-character window in which this path posted a 494-character + header instead of 60,000 characters of findings. Swept across the boundary + rather than pinned to the one fixture that lands in it. + """ + for pad in range(0, 400, 80): + with self.subTest(path_padding=pad): + findings = [finding("app.py", 11, body="anchorable " + "z" * 700)] + findings += [ + finding("app.py", 900 + i, body=f"demoted {i} " + "z" * 700) + for i in range(88) + ] + findings += [finding("p" * (pad + 1) + ".py", 8000, body="z" * 700)] + body = EndToEndPostTest().run_main( + findings, post_returncode=1, stderr="gh: Unprocessable Entity (HTTP 422)" + )[1]["body"] + self.assertIn(PROSE_MARKER, body, "the disclosure is never optional") + self.assertGreater( + len(body), PR.MAX_REVIEW_BODY_CHARS // 2, + "the review never collapses to a bare header — findings still render", + ) + ledger = ledger_from_posted_body(body) + if PR.BODY_ONLY_SENTINEL_PREFIX in body: + self.assertGreater(ledger["entry_count"], 0, "a posted sentinel parses") + else: + self.assertEqual(ledger["unrecovered_rounds"], 1, "…or it degrades loudly") + + def test_the_fallback_s_sentinel_survives_the_clamp_that_cuts_the_findings(self): + """Head-first ordering, the same property render_body_only_findings has: the + sentinel sits directly under the note, ahead of every finding, so a clamp big + enough to cut prose still leaves the machine-readable copy whole. Without it the + round that most needs the ledger — every finding lost, and the body too long to + show them all — is the one the ledger cannot read.""" + big = "x" * 19000 + findings = [finding("app.py", 11, body="anchorable " + big)] + findings += [finding("app.py", 900 + n, body=f"demoted {n} " + big) for n in range(5)] + posted = EndToEndPostTest().run_main( + findings, post_returncode=1, stderr="gh: Unprocessable Entity (HTTP 422)" + ) + fallback = posted[1]["body"] + self.assertEqual(len(fallback), PR.MAX_REVIEW_BODY_CHARS, "the clamp really fired") + self.assertIn("truncated here", fallback) + self.assertLess( + fallback.index(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) From fb3edb8628c51014e040353c07204a154399340c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 08:48:05 -0700 Subject: [PATCH 2/3] fix(cursor-review): keep a prose floor under the fallback's sentinel (BE-10002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-panel findings on the BE-10002 PR. - Prose floor (High). The sentinel duplicates every finding as JSON at close to the length of the prose entry below it, and the size guard only asked whether it fit ahead of finding one. Measured: 89 findings of ~700 chars posted 58,720 characters of comment and rendered ZERO findings, while the same round one finding LARGER dropped the sentinel whole and rendered 79 — the cliff ran backwards. The sentinel now gets at most half the body and fit_sentinel_items 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. Same round now: 39 findings visible AND 45 recovered, at every size. - Tag only what was checked (Medium). With `anchors is None` partition_by_anchor fails open and calls every finding inline without testing one, so tagging them `lost_to_fallback` asserted a check nobody ran — and on a 422, whose typical cause is an anchor GitHub refused, asserted it backwards. Gated on a real anchor parse; untagged findings stay [unanchorable], the conservative reading. - Steering no longer over-claims (Medium). "It anchored to the diff" / "should anchor normally this round" / "re-raise it freely" became "it passed the diff-anchor check" and "re-raise it if it still holds": the writer tags this from its own parse, and a [post-failed] entry carries no repeat lineage, so waving the panel through amplifies the one bypass the repeat cap cannot see. - Aggregate no longer contradicts the detail (Low, 6/6 reviewers). post-failed entries had folded into unanchorable_count, so the header said "N unanchorable, so never answerable at all" of findings the entry line two rows below reports matched the diff. Split into post_failed_count with its own clause. - Test measured the wrong thing (Medium). `len(body)` counts the sentinel's HTML comment, which renders as nothing, so the assertion held on exactly the body that showed no findings. Measured on the visible body now. - Writer emits `lost_to_fallback` on `is True`, symmetric with the reader. The residual the reviewers asked for is documented where the tag is set: a nonzero `gh` result is not proof the review was not committed server-side. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/build-ledger.py | 56 ++++-- .github/cursor-review/post-review.py | 143 ++++++++++++--- .../cursor-review/tests/test_build_ledger.py | 45 ++++- .../cursor-review/tests/test_post_review.py | 172 ++++++++++++++++-- 4 files changed, 352 insertions(+), 64 deletions(-) diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index 9a1cc7ea..7b72bf12 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -747,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", @@ -758,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 @@ -786,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, @@ -804,6 +820,7 @@ def disabled_ledger() -> dict: "entry_count": 0, "unanswered_count": 0, "unanchorable_count": 0, + "post_failed_count": 0, "notes": [], } @@ -843,8 +860,9 @@ def disabled_ledger() -> dict: " 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 anchored to the diff correctly and was lost to an API failure that\n" - " delivered the whole review as prose. Re-raise it freely if it still applies.\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 = ( @@ -871,10 +889,10 @@ def disabled_ledger() -> dict: " 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 anchor to\n" - " the diff — its review was lost to an API failure and delivered as prose — so\n" - " the preference above does not apply: re-raise it freely if it still holds,\n" - " and it should anchor normally this round.\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" ) @@ -899,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 " @@ -974,13 +999,15 @@ def render_ledger_markdown(ledger: dict, audience: str = "panel") -> str: lines.append(f" reply from {who}{tag}: {_defang_fences(reply['text'])}\n") if not anchored and lost_to_fallback: # Same "nobody could have answered it" as below, but the reason matters: - # this finding DID anchor, so the steering that asks the panel to prefer - # not re-raising an unanchorable one would be wrong about it. + # 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 anchored to the diff but its " - "review was delivered body-only, so no thread exists and nobody could " - "answer it; re-raising it needs no repeat_of and it should anchor " - "normally next round)\n" + " (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 @@ -1039,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 a4df5950..dc36d1b5 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -155,6 +155,18 @@ def emit_delivery(delivered: bool, gated: int = 0, ungated: int = 0) -> None: "\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: @@ -987,7 +999,11 @@ def render_body_only_sentinel(items: list) -> str: ) ), } - if item.get("lost_to_fallback"): + # `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=(",", ":")) @@ -995,6 +1011,40 @@ def render_body_only_sentinel(items: list) -> str: 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: @@ -1352,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 @@ -1480,41 +1534,80 @@ def main(): # 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 in it: the ones from `inline_items` tagged - # `lost_to_fallback`, the ones already unanchorable left untagged, since the POST - # outcome changed nothing for them. + # 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. - lost_ids = {id(item) for item in inline_items} + # + # 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 ] - fallback_head_with_sentinel = ( - f"{fallback_head}\n\n{render_body_only_sentinel(sentinel_items)}" - ) - # Size guard: the sentinel is a best-effort addition, never a reason for the clamp - # to reach the note above it. If it would not fit ahead of the first finding — so - # many findings that their JSON overruns the limit by itself, or a huge panel - # summary — drop it and post exactly what this path posted before, which the ledger - # still reads as a disclosed truncation rather than a silent round. + # 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 here, 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 is a + # 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. Reserving - # the note closes it: the sentinel is either posted whole or not posted at all. - if ( - len(fallback_head_with_sentinel) + len(FINDINGS_SEPARATOR) - > MAX_REVIEW_BODY_CHARS - len(CLAMP_TRUNCATION_NOTE) - ): + # 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: the fallback's body-only sentinel does not fit under the size " + "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, diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 585b5543..8a7ade33 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -1450,21 +1450,50 @@ def test_the_flag_survives_the_round_trip_through_the_real_writer(self): self.assertFalse(entry["anchored"]) self.assertEqual(entry["discussion_url"], "") self.assertEqual(entry["thread"]["answered_count"], 0) - self.assertEqual(ledger["unanchorable_count"], 2) + # 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 anchored to the diff but its review was " - "delivered body-only, so no thread exists and nobody could answer it; " - "re-raising it needs no repeat_of and it should anchor normally next round)", + "(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") @@ -1477,8 +1506,12 @@ def test_both_audiences_are_told_what_post_failed_means(self): "--- ROUND 1", 1 )[0] self.assertIn("[post-failed]", steering) - self.assertIn("anchored", steering) - self.assertIn("raise it freely", steering.lower()) + 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 diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index afe9a63d..f441bf98 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -73,6 +73,20 @@ def ledger_from_posted_body(body): } 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"