diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index f35101b..20d40f9 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -191,6 +191,32 @@ def _load_gate_unresolved(): re.DOTALL | re.MULTILINE, ) +# post-review.py's companion to the line above, emitted only when a size budget cut the +# payload down to a PREFIX of the round's demoted findings. Pinned to the same +# single-spaced OPENER, so a spelling the writer's defang lets through cannot satisfy +# this reader either. What follows the opener is NOT pinned, because nothing here reads +# it: the presence of the line is the whole claim ("findings were dropped"), and pinning +# `kept=N total=N` would let a shape this reader did not expect turn a DISCLOSED loss +# back into a silent one — the failure the companion exists to remove. The counts are +# for a human reading the raw body. +# +# Anchored to a LINE START for the same reason the sentinel is, and it is the sentinel's +# containment argument — not the writer-side defang — that carries the weight here. +# `defang_body_only_contract` runs only inside `post_error_review`, and `_body_only_entries` +# refuses an error review before it ever reaches this pattern, so the defang gives this +# line ZERO coverage on the success and 422-fallback bodies where the companion is +# actually read. The line anchor is what does: a demoted finding's prose renders as a +# blockquote, so a `` quoted into a +# finding body sits behind a `> ` and can never be the match. Unanchored — with a bare +# `search` over the whole body — that literal was plantable from the PR under review and +# flipped a FULLY RECOVERED round to `degraded`, fabricating an `unrecovered_rounds` +# entry and a "could not be recovered" note in the next round's prompt. +BODY_ONLY_TRUNCATED_OPENER = "" +) + # post_error_review's shape, as its own f-string renders it. See _body_only_entries: # this is the one consolidated body whose imported text sits at column 0, and the # writer-side defang that protects it only exists in bodies written by THIS version. @@ -452,6 +478,31 @@ def _body_only_text(value) -> str: return _FIELD_LINE_BREAK_RE.sub(" ", str(value or "")) +def _body_only_truncated(body: str) -> bool: + """Whether the round's sentinel says it carries only a PREFIX of its findings. + + Scoped the two ways the sentinel itself is scoped, because a false POSITIVE here is + not cosmetic: it fabricates an `unrecovered_rounds` entry and a "could not be + recovered — they may repeat" note in the next round's prompt off a round that lost + nothing. + + 1. The companion must begin its LINE (see `_BODY_ONLY_TRUNCATED_RE`), which is what + keeps a blockquoted copy quoted out of a finding body from matching. + 2. It must sit BELOW the sentinel it annotates. That is where post-review.py writes + it on BOTH budgeted paths — the success section and the 422 fallback — and it + narrows the line anchor further: a body with no readable sentinel has nothing for + this line to be a companion TO, and such a round is already degraded by the + missing sentinel rather than by this. + + Searching from `sentinel.end()` rather than slicing, so the pattern's line-start + lookbehind still sees the `\n` that precedes the companion. + """ + sentinel = _BODY_ONLY_SENTINEL_RE.search(body or "") + if sentinel is None: + return False + return bool(_BODY_ONLY_TRUNCATED_RE.search(body or "", sentinel.end())) + + def _resolve_lineage(url, by_id: dict, replies_by_root: dict, round_by_review: dict, pr_author=None): """Resolve a sentinel `repeat_of` to the ANCESTOR thread it names, or None. @@ -561,8 +612,11 @@ def _resolve_lineage(url, by_id: dict, replies_by_root: dict, round_by_review: d def _body_only_entries(review: dict, meta: dict, max_body: int, resolve_lineage=None): """(entries, degraded) for one consolidated review's demoted findings. - ``degraded`` is True when the review says it demoted findings but the sentinel - could not be read — the caller discloses that as a truncation note. + ``degraded`` is True when the review says it demoted findings that are not in the + returned entries — because the sentinel could not be read at all, or because the + writer's size budget cut it to a prefix and said so. The caller discloses either as + a truncation note. Entries and ``degraded`` are INDEPENDENT: a prefix payload + returns both real entries and True. An ERROR review is refused outright, before either half is looked at. It is the one consolidated body that renders unbounded judge/CLI text in a FENCE rather than a @@ -585,6 +639,15 @@ def _body_only_entries(review: dict, meta: dict, max_body: int, resolve_lineage= parsed = _parse_body_only_sentinel(review.get("body") or "") if parsed is None: return [], BODY_ONLY_PROSE_MARKER in (review.get("body") or "") + # A sentinel the writer's size budget cut down to a PREFIX parses perfectly — it is + # valid JSON, just not all of it — so the entries recovered below are real AND the + # round is degraded at the same time. Without this the omitted findings vanish with + # no `unrecovered_rounds` entry and no note, which is strictly worse than the + # all-or-nothing rule the budget replaced: THAT one degraded loudly, because a + # dropped sentinel does not parse. Absent on a body an older writer posted, which + # reads as "not truncated" — the same answer that writer's all-or-nothing payload + # actually warranted. + truncated = _body_only_truncated(review.get("body") or "") entries = [] for item in parsed: entry = { @@ -648,7 +711,7 @@ def _body_only_entries(review: dict, meta: dict, max_body: int, resolve_lineage= # asserting it would let a forged, unresolvable URL spend a repeat slot. entry["repeat_unresolved"] = True entries.append(entry) - return entries, False + return entries, truncated def _resolve_root_id(comment: dict, by_id: dict): diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 63853e4..c8decb4 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -105,6 +105,19 @@ # lets the reader reject a payload it does not understand instead of guessing. BODY_ONLY_SENTINEL_PREFIX = "cursor-review:body-only-findings v1" +# The sentinel's companion, emitted ONLY when a size budget cut the payload down to a +# PREFIX of the round's demoted findings. A truncated payload is still VALID JSON, so +# without this line build-ledger.py reads 12-of-89 as a complete recovery: the other 77 +# vanish with no `unrecovered_rounds` entry and no note, and the only record is a line +# in a public run log nobody reads. The all-or-nothing rule this budget replaced was +# self-disclosing by accident — a dropped sentinel does not parse, so the round degraded +# LOUDLY — and a partial one has to say so on purpose. Deliberately a SECOND comment +# rather than a key inside the payload: the reader pins the sentinel to a single-spaced +# opener immediately below the prose marker (see build-ledger.py), so anything inserted +# between them breaks the recovery it is meant to annotate, and a reader pinned to an +# older SHA ignores an unknown trailing comment instead of failing to parse the findings. +BODY_ONLY_TRUNCATED_PREFIX = "cursor-review:body-only-truncated v1" + # --- the blocking gate's delivery signal (BE-4691) ------------------------- # `needs.post-review.result == 'success'` cannot stand in for "a review carrying # resolvable finding threads landed on the PR": this script exits 0 after a @@ -209,18 +222,20 @@ def emit_delivery( "\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 +# The share of a finding-carrying body the sentinel may take, on BOTH such paths — the +# success path's demoted-findings section (render_body_only_findings) and the wholesale +# 422 fallback. 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). +SENTINEL_MAX_CHARS = MAX_REVIEW_BODY_CHARS // 2 def normalize_severity(value) -> str: @@ -1069,6 +1084,65 @@ def load_anchors(diff_path): return anchors +# CommonMark's start condition for an HTML block opened by `` to close it. @@ -1078,21 +1152,40 @@ def drop_unterminated_comment(cut: str) -> str: clamp_review_body's own "as much of it as fits is in the job summary" note. The review then renders as a header with no visible findings and no explanation of why. - Fixed HERE rather than by giving the sentinel a byte budget at render time, because - a budget cannot actually promise this: whether the sentinel survives depends on how - much body precedes it, which render_body_only_findings does not know. The clamp is - the one place that knows where the cut lands, and closing it here covers every HTML - comment in every posted body rather than the one we happen to be thinking about. + The BUDGET is the primary guard, and both finding-carrying paths now compute one in + main() from the head they just measured — the success path's demoted-findings + section and the wholesale 422 fallback alike — so on either of them the sentinel is + posted whole, or as its most-urgent prefix, or not at all, and never where the cut + lands. This function is the BACKSTOP behind that arithmetic: it is the one place + that knows where the cut actually landed, so it covers any HTML comment in any + posted body — a head measured wrong, a comment some future path adds — rather than + the one we happen to be thinking about. Dropping the fragment is safe on its own terms: the section's prose marker sits ABOVE the sentinel, so a cut deep enough to reach it still leaves build-ledger.py the evidence that findings WERE demoted, and that round degrades loudly instead of reading as a round that found nothing. + + Two things keep the rewind from costing more than the fragment. Model-supplied prose + can no longer carry an opener at all: render_finding_entry neutralizes ``, so once one is left dangling every byte after it + is already inside that comment — including any later `" in cut[opener:]: - return cut - return cut[:opener].rstrip() + for offset in html_block_openers(cut): + if cut.find("-->", offset) == -1: + return cut[:offset].rstrip() + return cut def clamp_review_body(body: str, limit: int = MAX_REVIEW_BODY_CHARS) -> str: @@ -1139,6 +1232,23 @@ def render_code_ref(path, line) -> str: return f"{fence}{pad}{text}{pad}{fence}" +# The one construct a blockquote does NOT contain. cmark-gfm strips the `> ` marker +# before parsing a blockquote's contents, so `> `. Measured against GitHub's own +# /markdown render: one finding carrying `` back at +# column 0 does not undo it, because that one is markdown-escaped to `-->` and closes +# nothing — the opener has to die at the WRITER. +# +# A zero-width space defeats CommonMark's start condition while the text still reads +# exactly as it arrived, the same trick and the same house style as +# defang_body_only_contract. Applied to EVERY `" +def render_body_only_truncation(kept: int, total: int) -> str: + """Disclose, machine-readably, that the sentinel above carries only `kept` of `total`. + + Counts rather than a bare flag, so the next round's prompt can say how much it lost + rather than only that it lost something. Both are plain integers from `len()`, so + nothing model-supplied reaches this line and it needs no escaping of its own. + """ + return f"" + + def fit_sentinel_items(items: list, budget: int) -> list: """The longest leading run of `items` whose rendered sentinel fits `budget` chars. @@ -1376,31 +1506,128 @@ def fit_sentinel_items(items: list, budget: int) -> list: return items[:lo] -def render_body_only_findings(items: list) -> str: - """Render findings that could not be anchored, for inclusion in the review body.""" +def sentinel_share(available: int, prose_len: int) -> int: + """How much of `available` pre-cut space the sentinel may take, given its prose. + + Two terms, and the SMALLER wins: + + * `SENTINEL_MAX_CHARS`, half the whole body — the ceiling. + * what the prose does not need, floored at half of `available`. + + That second term is what makes the ceiling hold at EVERY head size. Passing + `available` straight into `min(SENTINEL_MAX_CHARS, available)` buys the ceiling only + while the first term wins: once the head grows past roughly half the limit, + `available` is itself under the ceiling, the `min` stops binding, and the sentinel is + free to take all of the space that is left — reproducing on a big-head round the + zero-visible-findings collapse the ceiling exists to prevent. Both heads are + caller-shaped (`--notice`, `--ledger-note`, the panel summary), so that is a size a + consumer repo can reach without touching this file. + + `available - prose_len` BEFORE the floor, so a round whose prose is small is not + charged a floor it does not need: the sentinel may use whatever the prose leaves, + and the split only becomes one-half-each when the prose wants more than half. + + The ceiling bounds the FLOOR — the room the sentinel takes over the prose's + objection — and NOT the leftover the prose never wanted. Capping the whole `max` + charged the ceiling on rounds with no size pressure behind it: a 44,000-char + sentinel beside 17,000 chars of prose in ~59,000 of space was handed 30,000 instead + of the ~42,000 that fit, dropping roughly a hundred ledger entries to reserve space + the prose had no use for. The prose keeps its guarantee either way, because the + `available // 2` floor already delivers it: the prose gets + `min(prose_len, available // 2)` at every head size, which is "everything it wants, + up to half" — exactly what SENTINEL_MAX_CHARS was introduced to promise. + """ + return max(available - prose_len, min(SENTINEL_MAX_CHARS, available // 2)) + + +def render_body_only_findings(items: list, budget: int | None = None) -> str: + """Render findings that could not be anchored, for inclusion in the review body. + + `budget` is the number of characters this whole section may occupy before the + clamp's cut point — i.e. what is left of MAX_REVIEW_BODY_CHARS once the clamp's own + note, the review head above this section, and the separator between them are + subtracted. The caller computes it because the caller is the only one that has + measured the head; documenting it here keeps that arithmetic explained in one + place. `None` means "unbudgeted": the sentinel carries every item, which is what + every non-`main()` caller (and every round small enough for it not to matter) wants. + + Order is load-bearing, and it is marker → sentinel → prose. + + clamp_review_body cuts the TAIL, so the machine-readable copy sits as near the + head of the section as it can and stays recoverable for as long as any of the + section survives. But it cannot be first: a clamp landing INSIDE the JSON takes + the closing `-->` with it, and with the marker below that it took the evidence + too — build-ledger.py saw neither a parseable sentinel nor the marker, and a + fully-demoted round read as a review that found nothing. That is the one cut that + actually happens, and it was the silent one. + + One short line above the sentinel costs ~140 chars of recoverability and makes + every such cut LOUD. It is also the sentinel's required predecessor on the read + side, which is what scopes build-ledger.py's search to this section. + + The prose renders ALL `items` whatever the budget does to the sentinel: the budget + governs which findings the LEDGER recovers, never which ones a reader is shown. + Prose that overruns is handled by the tail clamp, as it always was. + + A section that FITS pays no budget at all — the clamp will not cut it, so there is + no size pressure to justify dropping a ledger entry. That is not a theoretical case: + render_body_only_sentinel escapes every `-` to six characters where the prose below + spends one, so a round on hyphen-rich paths can push the sentinel past + SENTINEL_MAX_CHARS while sentinel-plus-prose stays well under the limit. Charging + the ceiling there would drop findings out of the ledger to make room nobody needed. + + "Fits" is measured against the RAW limit, which is `budget` plus the clamp note the + caller subtracted out of it. `budget` is the cut POINT — where the clamp starts + trimming once it has decided to trim — but clamp_review_body leaves any body up to + MAX_REVIEW_BODY_CHARS untouched and never reaches for its note at all. Testing + `len(whole) <= budget` therefore truncated and degraded a section sitting in the + ~140-char window between the two, with no clamp behind it to justify the loss. The + 422 fallback's own guard compares against MAX_REVIEW_BODY_CHARS for this reason; + this is the same comparison, expressed in what this function was handed. + """ if not items: return "" - # Order is load-bearing, and it is marker → sentinel → prose. - # - # clamp_review_body cuts the TAIL, so the machine-readable copy sits as near the - # head of the section as it can and stays recoverable for as long as any of the - # section survives. But it cannot be first: a clamp landing INSIDE the JSON takes - # the closing `-->` with it, and with the marker below that it took the evidence - # too — build-ledger.py saw neither a parseable sentinel nor the marker, and a - # fully-demoted round read as a review that found nothing. That is the one cut that - # actually happens, and it was the silent one. - # - # One short line above the sentinel costs ~140 chars of recoverability and makes - # every such cut LOUD. It is also the sentinel's required predecessor on the read - # side, which is what scopes build-ledger.py's search to this section. - md = ( + marker = ( f"_The finding(s) below {BODY_ONLY_PROSE_MARKER}, so they are reported here " "instead of inline:_\n\n" - f"{render_body_only_sentinel(items)}\n\n" ) - for item in items: - md += render_finding_entry(item["comment"]) + "\n\n" - return md.rstrip("\n") + prose = "".join(render_finding_entry(item["comment"]) + "\n\n" for item in items) + whole = f"{marker}{render_body_only_sentinel(items)}\n\n{prose}" + if budget is None or len(whole) <= budget + len(CLAMP_TRUNCATION_NOTE): + return whole.rstrip("\n") + # Room for the truncation companion, measured at its longest: `kept` is strictly + # less than `total` here, so it can never carry more digits than `total` does. + notice_reserve = ( + len(render_body_only_truncation(len(items), len(items))) + len("\n\n") + ) + available = budget - len(marker) - len("\n\n") - notice_reserve + kept = fit_sentinel_items(items, sentinel_share(available, len(prose))) + if kept: + md = f"{marker}{render_body_only_sentinel(kept)}\n\n" + if len(kept) < len(items): + # The loss, serialized. A prefix payload is still valid JSON, so without + # this line next round's ledger reads it as a complete recovery. + md += f"{render_body_only_truncation(len(kept), len(items))}\n\n" + print( + f"Review: the body-only sentinel carries the {len(kept)} most urgent of " + f"{len(items)} demoted finding(s) — the rest would have displaced the " + "findings a reader can see.", + file=sys.stderr, + ) + else: + # Nothing fits: emit exactly what this section carried before the sentinel + # existed. The marker still discloses that findings WERE demoted, so next + # round's ledger reads a truncation rather than a round that found nothing — + # no companion needed, because a missing sentinel does not parse and is + # already the loud case. + print( + "Review: no part of the 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, + ) + md = marker + return f"{md}{prose}".rstrip("\n") def render_findings_markdown(review_body: str, comments: list[dict]) -> str: @@ -1811,7 +2038,21 @@ def main(): review_head += "\n\n_(All findings had invalid file/line references and were dropped.)_" review_body = review_head - body_only_md = render_body_only_findings(body_only_items) + # The section's size guard, computed HERE because `review_head` is the only thing + # that decides where the clamp lands and this is the only place it has been + # measured. What the section may occupy before the cut point: the limit, less the + # clamp's own note (the clamp cuts at `limit - len(note)`), less the head above it, + # less the separator between them. `review_body` ends up as + # `review_head + FINDINGS_SEPARATOR + marker + sentinel + "\n\n" + prose`, so with + # this budget the sentinel's closing `-->` always sits before the cut: it is emitted + # whole, or as its most-urgent prefix, or not at all — never where the clamp cuts. + body_only_md = render_body_only_findings( + body_only_items, + budget=MAX_REVIEW_BODY_CHARS + - len(CLAMP_TRUNCATION_NOTE) + - len(review_head) + - len(FINDINGS_SEPARATOR), + ) if body_only_md: # A demoted finding still carries no THREAD — there is no place to answer or # resolve it — but since BE-9565 it does reach the next round's ledger: the @@ -2041,20 +2282,49 @@ def finish_posted_review(): # ~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), + # + # Both parts are skipped outright for a body that FITS: nothing will be cut, so + # there is no size pressure to justify dropping a ledger entry. Same rule, and the + # same `sentinel_share` split, as the success path's section above. + prose_only = render_findings_markdown("", [i["comment"] for i in enriched]) + whole_fallback_len = ( + len(fallback_head) + + len("\n\n") + + len(render_body_only_sentinel(sentinel_items)) + + len(prose_only) ) - kept = fit_sentinel_items(sentinel_items, sentinel_budget) + if whole_fallback_len <= MAX_REVIEW_BODY_CHARS: + kept = sentinel_items + else: + notice_reserve = ( + len(render_body_only_truncation(len(sentinel_items), len(sentinel_items))) + + len("\n\n") + ) + available = ( + MAX_REVIEW_BODY_CHARS + - len(CLAMP_TRUNCATION_NOTE) + - len(fallback_head) + - len("\n\n") + - len(FINDINGS_SEPARATOR) + - notice_reserve + ) + # `prose_only` opens with FINDINGS_SEPARATOR, which `available` already + # reserved; counting it twice would understate what the prose leaves over. + kept = fit_sentinel_items( + sentinel_items, + sentinel_share(available, max(0, len(prose_only) - len(FINDINGS_SEPARATOR))), + ) if kept: fallback_head_with_sentinel = ( f"{fallback_head}\n\n{render_body_only_sentinel(kept)}" ) if len(kept) < len(sentinel_items): + # The loss, serialized — see render_body_only_truncation. A prefix payload + # is still valid JSON, so next round's ledger would otherwise read it as a + # complete recovery of a round that lost most of its findings. + fallback_head_with_sentinel += ( + f"\n\n{render_body_only_truncation(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 " diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 2f186c5..bbe5681 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -1017,6 +1017,104 @@ def test_a_finding_that_merely_quotes_the_error_heading_is_still_recovered(self) self.assertEqual(ledger["entry_count"], 1, "the round was NOT refused") self.assertEqual(ledger["entries"][0]["path"], "post-review.py") + def test_a_prefix_payload_is_recovered_AND_degraded(self): + """The reader half of BE-12535's truncation companion. + + A payload the writer's size budget cut to a prefix is valid JSON, so it parses + and its entries are real — but the findings it left out are gone, and without + the companion this round reads exactly like one that demoted only what it + carried. Entries and `degraded` are independent here for that reason: the round + contributes what it recovered AND says what it lost. + """ + section = body_only_section([demoted("far.py", 900)]) + section += f"\n\n" + ledger = bl.build_ledger([review_with_demoted(101, 1, [], section=section)], [], []) + self.assertEqual(ledger["entry_count"], 1, "what fit is still recovered") + self.assertEqual(ledger["entries"][0]["path"], "far.py") + self.assertEqual(ledger["unrecovered_rounds"], 1, "…and the rest is disclosed") + self.assertTrue(any("could not be recovered" in n for n in ledger["notes"])) + + def test_a_body_without_the_companion_is_not_reported_as_truncated(self): + """Absent on every body an older writer posted, and on every round that fit. It + must read as "not truncated" there — inventing a loss would drive a "findings + may repeat" warning into every prompt on a PR that lost nothing.""" + ledger = bl.build_ledger( + [review_with_demoted(101, 1, [demoted("far.py", 900)])], [], [] + ) + self.assertEqual(ledger["entry_count"], 1) + self.assertEqual(ledger["unrecovered_rounds"], 0) + self.assertEqual(ledger["notes"], []) + + def test_the_companion_is_pinned_to_one_spelling(self): + """One exact literal, not a whitespace-tolerant one. Nothing but post-review.py + legitimately writes this line, so tolerance buys nothing and costs the only + property that matters: a spelling looser than the one the writer emits is one + more shape a forger can reach for.""" + section = body_only_section([demoted("far.py", 900)]) + real = f"{section}\n\n" + self.assertTrue(bl._body_only_truncated(real), "our own render is read") + for spelling in ( + f"", + "", + ): + with self.subTest(spelling=spelling): + self.assertFalse(bl._body_only_truncated(f"{section}\n\n{spelling}")) + # …but what FOLLOWS the pinned opener is deliberately not pinned: the line's + # presence is the claim, so an unexpected count shape still discloses the loss + # rather than silently reading as a complete recovery. + self.assertTrue( + bl._body_only_truncated( + f"{section}\n\n" + ) + ) + + def test_a_companion_quoted_into_a_finding_body_is_refused(self): + """The forgery that is actually reachable, mirroring the sentinel's own test. + + `defang_body_only_contract` runs ONLY inside `post_error_review`, and + `_body_only_entries` refuses an error review before it ever reaches this line — + so the writer-side defang gives the companion zero coverage on the success and + 422-fallback bodies where it is read. The LINE ANCHOR is the whole control, and + without it this literal was plantable by putting it in the PR under review: a + model quotes it back into a finding body, it matches straight through the `> ` + blockquote prefix, and a fully recovered round flips to `degraded` — fabricating + an `unrecovered_rounds` entry and a "may repeat" warning in the next prompt. + """ + planted = f"" + section = body_only_section( + [demoted("far.py", 900, body=f"quoting the PR:\n{planted}")] + ) + self.assertIn("body-only-truncated", section, "the forgery is still reported") + self.assertFalse( + bl._body_only_truncated(section), "…but it is quoted, so it claims nothing" + ) + ledger = bl.build_ledger([review_with_demoted(101, 1, [], section=section)], [], []) + self.assertEqual(ledger["entry_count"], 1, "the round is recovered in full") + self.assertEqual(ledger["unrecovered_rounds"], 0, "and nothing is invented") + self.assertEqual(ledger["notes"], []) + + # Pinned at the READER as well, independently of that defang. Two writers reach + # this parser: consumer repos stay on older pinned SHAs, so every success body + # they posted before `render_finding_entry` learned to neutralize an opener is + # still sitting on their PRs with a raw one in it. The line anchor is the half + # that covers those, and it is the half no writer-side change can outrun. + raw = f"{body_only_section([demoted('far.py', 900)])}\n> {planted}" + self.assertIn(planted, raw, "the opener really is raw in this fixture") + self.assertFalse(bl._body_only_truncated(raw), "the blockquote prefix is not matched through") + + def test_a_companion_above_the_sentinel_is_refused(self): + """The second half of the scoping: the companion annotates the sentinel, so it + has to sit BELOW it — which is where post-review.py writes it on both budgeted + paths. A line-anchored match alone would still accept one planted at column 0 in + some other part of a consolidated body.""" + section = body_only_section([demoted("far.py", 900)]) + above = f"\n\n{section}" + self.assertFalse(bl._body_only_truncated(above)) + # And with no readable sentinel there is nothing for it to be a companion to. + self.assertFalse( + bl._body_only_truncated(f"") + ) + def test_a_deeply_nested_payload_degrades_instead_of_raising(self): """`json.loads` raises RecursionError — a RuntimeError, not a ValueError — on a few KB of `[[[[…`, which fits a review body many times over. Uncaught it escapes diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index d048236..46cb6b9 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -74,6 +74,22 @@ def ledger_from_posted_body(body): return BL.build_ledger([review], [], []) +def sentinel_payload(body): + """The findings the body's body-only sentinel actually carries. + + Read off the POSTed line rather than through build_ledger, which re-sorts its + entries: the prefix the writer chose only survives here. + """ + lines = [ + ln for ln in body.splitlines() + if ln.startswith(f"")] + ) + + def visible(body): """`body` with the sentinel comment line removed. @@ -85,6 +101,7 @@ def visible(body): ln for ln in body.splitlines() if not ln.startswith(f"` always sits ahead of the cut point. Without that reserve there is a window + `len(CLAMP_TRUNCATION_NOTE)` wide in which the sentinel fits the raw limit but + not the clamp's cut — posted, cut mid-JSON, rewound to its opener, taking the + prose below it. Swept across the boundary with a padded `path` (model output, + length-checked nowhere) rather than pinned to 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)[0]["body"] + self.assertIn(PROSE_MARKER, body, "the disclosure is never optional") + # Measured on the VISIBLE body: `len(body)` counts tens of thousands of + # characters of HTML comment that render as nothing, so it stays large + # on exactly the body that shows a reader no findings. + self.assertGreater( + visible(body).count("demoted "), 20, + "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_success_path_sentinel_keeps_the_most_urgent_prefix(self): + """A prefix, not a sample — the success path's half of + test_the_sentinel_keeps_the_most_urgent_findings_when_it_cannot_keep_all. + + `enriched` is severity-sorted and `partition_by_anchor` preserves that order, so + the demoted findings the budget keeps are the ones next round most needs back, + in the same order as the prose below them. The critical is cited LAST in the + input so the assertion tests the severity sort rather than the input order. + """ + findings = [ + finding("app.py", 900 + i, severity="low", body=f"low {i} " + "z" * 700) + for i in range(99) + ] + findings += [finding("app.py", 999, severity="critical", body="C " + "z" * 700)] + body = EndToEndPostTest().run_main(findings)[0]["body"] + # Read the PAYLOAD, not the ledger: build_ledger re-sorts its entries, so the + # order the sentinel was WRITTEN in — the thing under test — only survives here. + payload = sentinel_payload(body) + self.assertGreater(len(payload), 0) + self.assertLess(len(payload), 100, "not all of them fit") + self.assertEqual(payload[0]["severity"], "critical", "the most urgent is kept") + # The kept set is the leading run of the severity-sorted order, not a scatter + # through it: the critical, then the lows in the order they were cited. + self.assertEqual( + [e["line"] for e in payload], + [999] + [900 + i for i in range(len(payload) - 1)], + ) + + def test_render_body_only_findings_without_a_budget_is_unchanged(self): + """`budget=None` is today's behaviour, byte-identical. Every non-`main()` reader + of this function gets what it always got, and the round-trip tests above keep + pinning the unbudgeted render.""" + items = PR.normalize_comments( + [finding("app.py", 900 + i, body=f"demoted {i}") for i in range(12)] + ) + self.assertEqual( + PR.render_body_only_findings(items), + PR.render_body_only_findings(items, budget=None), + ) + rendered = PR.render_body_only_findings(items) + self.assertIn( + PR.render_body_only_sentinel(items), rendered, + "the unbudgeted sentinel carries every item", + ) + + def test_a_budget_nothing_fits_posts_the_marker_alone_on_the_success_path(self): + """The floor, on the success path: when not even the first finding's JSON fits, + the sentinel is dropped whole and the section carries what it carried before the + sentinel existed — the marker, which next round reads as a disclosed truncation + rather than as a round that found nothing. The PROSE is never what gets dropped. + """ + marker = ( + f"_The finding(s) below {PROSE_MARKER}, so they are reported here " + "instead of inline:_\n\n" + ) + items = PR.normalize_comments( + [finding("app.py", 900 + i, body=f"demoted {i}") for i in range(12)] + ) + rendered = PR.render_body_only_findings(items, budget=len(marker) + 10) + self.assertIn(PROSE_MARKER, rendered, "the disclosure survives") + self.assertNotIn(PR.BODY_ONLY_SENTINEL_PREFIX, rendered, "the sentinel does not") + for i in range(12): + self.assertIn(f"demoted {i}", rendered, "and every finding is still shown") + + def test_a_truncated_sentinel_discloses_the_loss_to_the_next_round_s_ledger(self): + """A PREFIX payload is still valid JSON, so it must say it is a prefix (BE-12535). + + The all-or-nothing rule this budget replaced was self-disclosing by accident: + a dropped sentinel does not parse, `_parse_body_only_sentinel` returns None, and + the round degraded LOUDLY as an `unrecovered_rounds` entry the next prompt calls + out. A budget-cut payload parses perfectly, so left unannotated it reads to the + next round as a COMPLETE recovery — the omitted findings vanish with no note at + all, and the only record is a line in a public run log. That is strictly worse + than what it replaced, on exactly the rounds with the most to report. + """ + 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(120) + ] + body = EndToEndPostTest().run_main(findings)[0]["body"] + payload = sentinel_payload(body) + self.assertLess(len(payload), 120, "the budget did cut the payload") + self.assertIn( + f"", + body, + "…and the cut is serialized, not left to the run log", + ) + ledger = ledger_from_posted_body(body) + self.assertEqual(len(payload), ledger["entry_count"], "what fit is recovered") + self.assertEqual( + ledger["unrecovered_rounds"], 1, "…and what did not is disclosed" + ) + self.assertTrue( + any("could not be recovered" in n for n in ledger["notes"]), + "the next round's prompt is told, in words", + ) + + def test_a_truncated_fallback_sentinel_discloses_the_loss_too(self): + """The same companion on the 422 path, where partial payloads came first. + + The two paths share one reader: a disclosure the success path emits and the + fallback does not would make `unrecovered_rounds` mean different things + depending on which body a round happened to post. + """ + findings = [ + finding("app.py", 11 + (i % 3), body=f"lost {i} " + "z" * 700) + for i in range(120) + ] + body = EndToEndPostTest().run_main( + findings, post_returncode=1, stderr="gh: Unprocessable Entity (HTTP 422)" + )[1]["body"] + payload = sentinel_payload(body) + self.assertLess(len(payload), 120) + self.assertIn( + f"", + body, + ) + self.assertEqual(ledger_from_posted_body(body)["unrecovered_rounds"], 1) + + def test_a_whole_sentinel_never_claims_a_truncation(self): + """The companion is emitted ONLY on a real cut, so the note it drives into the + next round's prompt is never a false alarm — and a round that recovered + everything is never reported as one that lost findings.""" + findings = [finding("app.py", 11)] + findings += [finding("app.py", 900 + i, body=f"demoted {i}") for i in range(6)] + body = EndToEndPostTest().run_main(findings)[0]["body"] + self.assertEqual(len(sentinel_payload(body)), 6, "all six fit") + self.assertNotIn(PR.BODY_ONLY_TRUNCATED_PREFIX, body) + ledger = ledger_from_posted_body(body) + self.assertEqual(ledger["entry_count"], 6) + self.assertEqual(ledger["unrecovered_rounds"], 0) + + def test_the_prose_floor_holds_when_the_head_eats_the_ceiling(self): + """SENTINEL_MAX_CHARS alone is not the floor it reads as (BE-12535). + + `min(SENTINEL_MAX_CHARS, available)` buys the half-the-body ceiling only while + the FIRST term wins. Once the review head grows past roughly half the limit — + and the head is built from caller-supplied `--notice`, `--ledger-note` and the + panel summary, so a consumer repo can reach that size without touching this + file — `available` is itself under the ceiling, the `min` stops binding, and the + sentinel is free to take every character left before the cut. That reproduces + the zero-visible-findings collapse the ceiling exists to prevent, on the rounds + where the head is already crowding the findings out. + + Swept across the head size at which the second term takes over rather than + pinned to one point past it. + """ + for head_share in (0.3, 0.5, 0.6, 0.75, 0.9): + with self.subTest(head_share=head_share): + pad = int(PR.MAX_REVIEW_BODY_CHARS * head_share) + 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(60) + ] + body = EndToEndPostTest().run_main( + findings, + panel=[{"model": "m" * pad, "review_type": "adversarial", + "status": "error"}], + )[0]["body"] + self.assertLessEqual(len(body), PR.MAX_REVIEW_BODY_CHARS) + self.assertIn(PROSE_MARKER, body, "the disclosure is never optional") + # The sentinel never takes more than half of what the head left. + sentinel = [ + ln for ln in body.splitlines() + if ln.startswith(f"`. Checked against GitHub's own /markdown render: one finding + carrying `` back at column 0 does not undo it + either — that one is escaped to `-->` and closes nothing. + + So it dies at the writer, and the clamp's rewind never has to reason about it. + It is plantable exactly as you would expect: ``, so once one is left + dangling every byte after it — including any later `\n\n" + ) + + def test_a_blockquoted_opener_below_a_dangling_one_does_not_misdirect_the_rewind(self): + """The other half of the scoping, as a contract test on the function. + + An unscoped `rfind` took the LAST `