diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 58fd9af..ad271ba 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -89,7 +89,9 @@ A `repeat_of` is adjudicated on two independent layers, and neither is the other Those per-entry lines are the ledger's own structure, so imported prose must not be able to write one. An entry's fields sit at a two-space indent, and a finding body or an author reply keeps its line breaks — so every line of quoted prose *after its first* is prefixed ` | ` (a blank line renders as a bare ` |`, since no rendered line carries trailing whitespace), and the block header tells both audiences that a two-space line without `|` is a field this workflow wrote. The entry HEADER line takes the other half of the same contract: a `path` — which git permits a line break inside, and which a thread-derived entry takes straight from the review comment — is flattened to one line before it is interpolated there. A reply from any GitHub account that contains `\n thread: … answers_from_author_or_maintainer=1` or `\n discussion_url: ` therefore renders as visibly quoted text rather than as a field the judge would follow to grant itself a repeat slot, or a URL `post-review.py` would publish unvalidated. The prose is still shown in full, and single-line prose renders exactly as it did before. -Alongside the ledger, each round after the first is shown an **incremental block** — the hunks new since the last reviewed commit — introduced by the line "the subset of the diff above that changed since the last reviewed commit". That claim is now a property of how the block is built, not an aspiration: it is derived from two PR patches, `git diff BASE...LAST_REVIEWED` and the reviewed diff this round is running on, and every section it emits is a verbatim slice of the latter. It therefore **can never contain a hunk the PR does not carry**. The formulation it replaced (BE-15558) was `git diff LAST_REVIEWED...HEAD`, a commit range; with a merge commit at HEAD, `LAST_REVIEWED` is an ancestor of HEAD and the merge base of the two IS `LAST_REVIEWED`, so the range swallowed every commit that merge pulled in from the base branch — one measured round built a 9,800-line block against a 1,234-line reviewed diff with 119 of its 133 files outside the PR, two others built ~1.16M-line blocks, and the prompt grew from ~98 KB to ~826 KB, timing most legs out and pointing the survivors at base-branch files. Hunk comparison normalises the `@@ -a,b +c,d @@` headers to `@@ @@`, so a pure rebase — identical hunks at shifted line numbers — produces an empty block rather than re-reviewing the whole PR. +Alongside the ledger, each round after the first is shown an **incremental block** — the hunks new since the last reviewed commit — introduced by the line "the subset of the diff above that changed since the last reviewed commit". That claim is now a property of how the block is built, not an aspiration: it is derived from two PR patches — the diff the previous round actually reviewed, pinned to the merge base *that* round recorded (see below), and the reviewed diff this round is running on — and every section it emits is a verbatim slice of the latter. It therefore **can never contain a hunk the PR does not carry**. The formulation it replaced (BE-15558) was `git diff LAST_REVIEWED...HEAD`, a commit range; with a merge commit at HEAD, `LAST_REVIEWED` is an ancestor of HEAD and the merge base of the two IS `LAST_REVIEWED`, so the range swallowed every commit that merge pulled in from the base branch — one measured round built a 9,800-line block against a 1,234-line reviewed diff with 119 of its 133 files outside the PR, two others built ~1.16M-line blocks, and the prompt grew from ~98 KB to ~826 KB, timing most legs out and pointing the survivors at base-branch files. Hunk comparison normalises the `@@ -a,b +c,d @@` headers to `@@ @@`, so a pure rebase — identical hunks at shifted line numbers — produces an empty block rather than re-reviewing the whole PR. + +Both sides of that comparison are pinned to the merge base **their own round** used, and the previous round's is read back rather than recomputed (BE-15598). Every consolidated review that actually reviewed something carries a **round sentinel** directly under its header — ``, an HTML comment that renders as nothing — and the next round's ledger reads the merge base out of the LAST one, accepting it only when that body is not the *Review failed* shape, its `head` is that review's own `commit_id`, and its `merge_base` is a full lowercase hex SHA. A round that reviewed nothing — the error review, and the body posted when every panel cell errored — deliberately writes no sentinel, so it records no merge base and the round after it takes the fail-closed path below rather than subtracting hunks no panel ever saw. `OLD` is then `git diff `. Recomputing it as `git diff BASE...LAST_REVIEWED`, as the first version did, is correct only while `BASE` still resolves to the same merge base: **retarget the PR, or rewrite its base branch, and it does not**. The three-dot form silently re-resolves to a different merge base, everything the branch inherited from the old one appears on both sides, and hunks the panel has never seen are subtracted as "already reviewed". Nothing downstream catches that — the block is merely *smaller*, and a smaller block is still a subset, so the fail-safe below passes. Hence the pin, and hence it **fails closed**: with no recorded merge base (a round reviewed before the sentinel existed, a sentinel that did not parse, a recorded commit this checkout cannot reach or that is no longer an ancestor of the reviewed commit) the run logs which case it hit and emits **no block at all** rather than falling back to the current base. Every live PR sees one block-less round after this ships — the round it compares against predates the sentinel — and the block returns on the round after. The merge base recorded the other way round, for *this* round, comes from a plain `git merge-base` in the `diff-size` job rather than from `check-pr-size`, so it exists on the degraded raw-numstat path too. Nothing reads `base` back; it is there so a human can tell after the fact that the base moved between two rounds. The build is then **verified, not trusted**, and against the reviewed diff's own bytes rather than its file names: every section of the block must appear **byte for byte** in the reviewed diff. A path-only check would have waved through a fabricated, reordered or duplicated hunk as long as it rode under some path the PR does touch, which is weaker than the property asserted above. If any section fails that, or the block is longer than the reviewed diff, it is discarded whole and the run logs `::warning::Incremental diff discarded: it was not a subset of the reviewed diff ( foreign file(s), vs lines).` Seeing that warning in a consumer's log means the panel ran on the **full reviewed diff alone** — always correct, just without the prioritization hint — and not that the review was degraded or skipped. The `diff-size` job reports the same fact as its `incremental_subset` output, `false` only when a block that had been built was discarded. diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index 92f623e..3d80bab 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -217,6 +217,42 @@ def _load_gate_unresolved(): + re.escape(BODY_ONLY_TRUNCATED_OPENER) + _NOT_LINE_SEP_CLASS + r"*?-->" ) +# The round sentinel (BE-15598): what the round that posted this review DIFFED AGAINST. +# Emitted by post-review.py directly under the review header, on every success body and +# on none of the error ones. +# +# Pinned to the SINGLE-SPACED opener, the exact byte-for-byte string post-review.py's +# f-string emits, for the same reason the body-only sentinel is: the writer's defang +# replaces one exact literal, and a reader more tolerant than that defang is a reader +# the defang does not fully cover. `v1` is part of the literal, so a future `v2` payload +# does not match at all — which is the intended "reject what you do not understand". +# +# Anchored to a LINE START, which is what stops a finding from forging one. Every line +# of a rendered finding sits behind a `> ` blockquote marker (and post-review.py +# neutralizes a bare `" +) + +# The shape every SHA in that payload must have before anything here believes it. Kept +# strict (lowercase full hex) so it agrees exactly with post-review.py's writer-side +# validation: a value this rejects reaches the workflow as "", which fails closed to +# "no incremental block" rather than to a `git diff` against an attacker-chosen ref. +# +# Terminated with `\Z`, NOT `$`: Python's `$` also matches just before a FINAL newline, +# so `^[0-9a-f]{40}$` accepts a 41-character value ending in one. `_write_outputs` +# appends the accepted value to $GITHUB_OUTPUT as `key=value`, and this gate is the +# single control keeping a line break out of it — an invariant `$` does not provide. +# post-review.py's `_ROUND_SHA_RE` is the writer-side twin and is anchored the same way. +_FULL_SHA_RE = re.compile(r"^[0-9a-f]{40}\Z") + # 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. @@ -461,6 +497,38 @@ def _parse_body_only_sentinel(body: str): return items +def _parse_round_sentinel(body: str): + """Recover the round sentinel post-review.py wrote into a review body (BE-15598). + + Returns the payload dict, or ``None`` when there is nothing to trust — no sentinel, + a version this reader does not know (the opener pins `v1`, so `v2` simply does not + match), a payload the tail clamp cut mid-JSON, or a shape that is not an object with + string `head` and `merge_base`. Never raises: the caller's fallback is "" for both + recorded SHAs, which fails closed to "no incremental block next round". + + `base` is deliberately NOT required. Nothing builds a diff from it — it is recorded + for a human reading the raw body, and for whoever has to reconstruct what a round + was looking at — so a payload missing it is still perfectly usable for the one thing + this record exists to do. + """ + match = _ROUND_SENTINEL_RE.search(body or "") + if not match: + return None + try: + payload = json.loads(match.group(1).strip()) + except (ValueError, TypeError, RecursionError): + # RecursionError for the same reason _parse_body_only_sentinel catches it: it is + # a RuntimeError, not a ValueError, so a few KB of `[[[[…` would otherwise escape + # into cmd_build's blanket except and cost the ENTIRE ledger where this promises + # one unreadable sentinel degrades to "no recorded merge base". + return None + if not isinstance(payload, dict): + return None + if not all(isinstance(payload.get(key), str) for key in ("head", "merge_base")): + return None + return payload + + def _body_only_line(value): """Coerce a sentinel's ``line`` to an int, or None. @@ -832,6 +900,8 @@ def build_ledger( "rounds": 0, "total_rounds": 0, "last_reviewed_sha": "", + "last_reviewed_merge_base": "", + "last_reviewed_base_sha": "", "entries": [], "entry_count": 0, "unanswered_count": 0, @@ -848,6 +918,59 @@ def build_ledger( total_rounds = len(consolidated) last_reviewed_sha = consolidated[-1].get("commit_id") or "" + # What the LAST round diffed against (BE-15598). Read from that round's own review + # body rather than recomputed, because the answer is not derivable after the fact: + # a retarget or a base-branch rewrite moves merge-base(base, last_reviewed), and the + # next round's incremental block would then treat hunks the panel never saw as + # already reviewed and drop them — a silent loss the subset fail-safe cannot catch, + # since a smaller block is still a subset. + # + # Four gates, all of which must hold, and all of which fail to "" rather than to a + # guess. The ERROR-REVIEW shape is refused outright first, exactly as + # `_body_only_entries` refuses it and for the identical reason: `post_error_review` + # renders imported judge/CLI text inside a FENCE, so it is the one consolidated body + # whose foreign lines sit at column 0 and can therefore satisfy the line anchor the + # sentinel's containment argument rests on. post-review.py's writer-side defang + # covers that text, but only in bodies THIS version wrote — consumers stay pinned to + # older SHAs, and every error review they have already posted is sitting on their PRs + # undefanged, a body no writer-side change can reach. Nor does the `head` gate below + # close it: the reviewed head SHA is public to the PR author, so a forged payload can + # name it, and the merge base it then claims would pass the workflow's `cat-file -e` + # and `merge-base --is-ancestor` checks (those prove ANCESTRY, not that the commit is + # the merge base round N actually used). Refusing the shape here is the half that + # cannot be outrun by a slow fleet, and it costs nothing real: `post_error_review` + # deliberately writes no sentinel, so a genuine error review never had one to lose. + # + # It also makes the workflow's "No recorded merge base" log honest for the case its + # message omits — the last round was an error review, which carries no sentinel at all. + # + # Then: only the LAST round is read (it is the only one the next block diffs + # against). `head` must equal that review's own `commit_id`, so a sentinel copied + # from another round or another PR is refused, and so is one left behind by a body + # whose review was re-posted against a different commit. And `merge_base` must be a + # full lowercase hex SHA before it is allowed anywhere near a `git diff` argument. + last_reviewed_merge_base = "" + last_reviewed_base_sha = "" + last_body = consolidated[-1].get("body") or "" + round_sentinel = ( + None if _ERROR_REVIEW_RE.search(last_body) else _parse_round_sentinel(last_body) + ) + if ( + round_sentinel is not None + and last_reviewed_sha + and round_sentinel.get("head") == last_reviewed_sha + and _FULL_SHA_RE.match(round_sentinel.get("merge_base") or "") + ): + last_reviewed_merge_base = round_sentinel["merge_base"] + # `base` is diagnostic, but it is held to the SAME shape as the merge base, and + # not because anything diffs against it: `_write_outputs` appends it to + # $GITHUB_OUTPUT, where a value carrying a newline is an output-injection + # vector. The writer only ever emits hex-or-empty, so this costs nothing real + # and keeps a single control on the whole payload. + recorded_base = round_sentinel.get("base", "") + if isinstance(recorded_base, str) and _FULL_SHA_RE.match(recorded_base): + last_reviewed_base_sha = recorded_base + comments = [c for c in (comments or []) if isinstance(c, dict)] by_id = {c.get("id"): c for c in comments} flags = _thread_flags_by_root(threads) @@ -1048,6 +1171,8 @@ def _size(items): "rounds": len(rounds_present), "total_rounds": total_rounds, "last_reviewed_sha": last_reviewed_sha, + "last_reviewed_merge_base": last_reviewed_merge_base, + "last_reviewed_base_sha": last_reviewed_base_sha, "entries": entries, "entry_count": len(entries), "unanswered_count": unanswered, @@ -1077,6 +1202,8 @@ def unknown_ledger(call: str, reason: str) -> dict: "rounds": 0, "total_rounds": 0, "last_reviewed_sha": "", + "last_reviewed_merge_base": "", + "last_reviewed_base_sha": "", "entries": [], "entry_count": 0, "unanswered_count": 0, @@ -1096,6 +1223,8 @@ def disabled_ledger() -> dict: "rounds": 0, "total_rounds": 0, "last_reviewed_sha": "", + "last_reviewed_merge_base": "", + "last_reviewed_base_sha": "", "entries": [], "entry_count": 0, "unanswered_count": 0, @@ -1510,6 +1639,14 @@ def _write_outputs(ledger: dict) -> None: f.write(f"status={ledger.get('status', 'unknown')}\n") f.write(f"rounds={ledger.get('total_rounds', 0)}\n") f.write(f"last_reviewed_sha={ledger.get('last_reviewed_sha', '')}\n") + # Consumed by the `incremental` step in the diff-size job (BE-15598): the OLD + # side of the incremental block is `git diff `, and + # an EMPTY value here is what makes that step skip the block outright instead of + # falling back to the current base, which is the bug this record exists to fix. + f.write(f"last_reviewed_merge_base={ledger.get('last_reviewed_merge_base', '')}\n") + # Diagnostic only — nothing builds a diff from it. It is what makes a shrunken + # block explicable after the fact ("the base moved between these two rounds"). + f.write(f"last_reviewed_base_sha={ledger.get('last_reviewed_base_sha', '')}\n") f.write(f"entry_count={ledger.get('entry_count', 0)}\n") diff --git a/.github/cursor-review/incremental-diff.py b/.github/cursor-review/incremental-diff.py index 2deb3de..f531df5 100755 --- a/.github/cursor-review/incremental-diff.py +++ b/.github/cursor-review/incremental-diff.py @@ -11,12 +11,20 @@ ~1.16M-line blocks; the prompt grew from ~98 KB to ~826 KB, most legs timed out, and the legs that finished reviewed files from the base branch instead of the PR. -The fix is to stop diffing two commits and start diffing two PR *patches*. Both -are three-dot diffs against the base, so each contains only the branch's own -changes and neither can carry a base-branch commit: - -* OLD — `git diff BASE...LAST_REVIEWED` — what the last round saw. -* NEW — the reviewed diff this round is running on (`pr-diff.patch`). +The fix is to stop diffing two commits and start diffing two PR *patches*. Each +is taken against a MERGE BASE rather than along a commit range, so each contains +only the branch's own changes and neither can carry a base-branch commit: + +* OLD — `git diff LAST_REVIEWED` — what the + last round saw. Two-dot, and pinned to the merge base that round itself + diffed against, read back from the round sentinel in its review rather than + recomputed from the CURRENT base (BE-15598): retarget the PR or rewrite its + base branch and a recomputed merge base moves, which puts everything the + branch inherited from the old base on both sides and silently subtracts, as + "already reviewed", hunks the panel has never seen. The caller supplies no + OLD patch at all when it has no recorded merge base to pin to. +* NEW — the reviewed diff this round is running on (`pr-diff.patch`), which + `check-pr-size` builds as `mergeBase...head` for THIS round. `build` emits every NEW file section whose content differs from the same file's section in OLD, plus every NEW section for a file OLD does not have. A file OLD diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 05c701b..431fb5c 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -139,6 +139,31 @@ # 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 round sentinel (BE-15598). The ledger already records WHICH commit the last round +# reviewed (`last_reviewed_sha`); this records what that round diffed it AGAINST. The +# next round's incremental block rebuilds the OLD side as "what round N saw", and the +# only way to do that faithfully is to diff the tree round N actually used — its merge +# base. Recomputing it from the CURRENT base is what BE-15597 measured wrong: after a +# retarget or a base-branch rewrite the merge base moves, hunks the panel never saw read +# as already-reviewed, and they are dropped from the block. The block still passes the +# subset fail-safe (it is a subset), so nothing downstream catches it — hence a written +# record rather than a derivation. Same version-suffix discipline as the sentinels above: +# a reader that does not understand the payload rejects it instead of guessing. +ROUND_SENTINEL_PREFIX = "cursor-review:round v1" + +# Every SHA the round sentinel carries is validated against this before it is written. +# A field that does not match is emitted as "" rather than dropped: the reader then sees +# a sentinel that parses and is missing the one thing it needs, which fails closed to +# "no incremental block", where a MISSING key would be indistinguishable from a sentinel +# this writer never wrote. It also keeps the payload free of `-->`, `"` and newlines by +# construction, so the comment cannot be broken out of by whatever produced the value. +# +# `\Z`, not `$`, and for that last clause specifically: Python's `$` matches before a +# FINAL newline too, so `$` here would let a 41-character value whose last byte is `\n` +# through and put a line break inside the HTML comment. build-ledger.py's `_FULL_SHA_RE` +# is the reader-side twin and carries the same anchor. +_ROUND_SHA_RE = re.compile(r"^[0-9a-f]{40}\Z") + # --- 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 @@ -2004,6 +2029,30 @@ def strip_severity_badge(severity: str, body: str) -> str: return body[len(badge):] if body.startswith(badge) else body +def render_round_sentinel(head: str, base: str, merge_base: str) -> str: + """The round sentinel: what THIS round reviewed, and what it diffed against. + + Read back by build-ledger.py from the last consolidated review, and used by the + next round's incremental block to pin the OLD patch to the merge base this round + used rather than recomputing one from a base that may since have moved. + + Every field is a 40-hex commit SHA or the empty string — see `_ROUND_SHA_RE`. The + JSON is sorted-key and separator-tight so the line is byte-stable across rounds, + which is what lets a reader pin the opener as one exact literal. + """ + + def field(value) -> str: + text = (value or "").strip() + return text if _ROUND_SHA_RE.match(text) else "" + + payload = json.dumps( + {"base": field(base), "head": field(head), "merge_base": field(merge_base)}, + sort_keys=True, + separators=(",", ":"), + ) + return f"" + + def defang_body_only_contract(text: str) -> str: """Break both halves of the body-only sentinel contract inside imported text. @@ -2037,6 +2086,16 @@ def defang_body_only_contract(text: str) -> str: # "findings were lost" note in the next round's prompt off text we quoted. BODY_ONLY_TRUNCATED_PREFIX, BODY_ONLY_TRUNCATED_PREFIX.replace(":", ":\u200b", 1), + ).replace( + # The round sentinel (BE-15598), which is worth rather more than the other two: + # forged, it names a merge base of the attacker's choosing, and the next round + # builds its OLD patch — "what the panel already saw" — against that tree. A + # merge base equal to the last-reviewed commit makes OLD empty; the step fails + # closed there, but a merge base pointing at a LATER tree would suppress real + # hunks from the block. The reader is line-anchored and checks `head` against + # the review's own `commit_id`, so this is the third control, not the only one. + ROUND_SENTINEL_PREFIX, + ROUND_SENTINEL_PREFIX.replace(":", ":\u200b", 1), ) @@ -2803,6 +2862,24 @@ def main(): "or unreadable means every finding is sent inline (pre-existing behaviour)." ), ) + parser.add_argument( + "--base-sha", + default="", + help=( + "The PR's base-branch tip for this round, recorded in the round sentinel. " + "Diagnostic only — nothing reads it back to build a diff." + ), + ) + parser.add_argument( + "--merge-base-sha", + default="", + help=( + "merge-base(base, head) for this round, recorded in the round sentinel so " + "the NEXT round can pin its 'already reviewed' patch to the tree this " + "round actually diffed against. Empty (unresolvable) is recorded as empty, " + "and the next round then skips its incremental block rather than guessing." + ), + ) parser.add_argument("--triggered-by", default=None) parser.add_argument("--error-message", default=None, help="If set, post an error review with this message") parser.add_argument( @@ -2840,15 +2917,33 @@ def main(): attribution = f"\n\n_Triggered by @{args.triggered_by}._" if args.triggered_by else "" header = f"## 🔍 Cursor Review — Consolidated panel{attribution}" + # The round sentinel goes on its own line directly under the header and above + # everything else (BE-15598), for the same reason the body-only sentinel sits near + # the top of its section: `clamp_review_body` cuts the TAIL, so a record this short + # at this height survives every cut that leaves a body at all. + # + # Two headers, deliberately, and the plain one goes to every body whose round + # reviewed NOTHING: `post_error_review`, and the all-panel-cells-failed branch + # below. Recording what such a round "diffed against" would be a claim about a panel + # that never ran, and the next round would build its "already reviewed" side from it + # — subtracting hunks nobody looked at. build-ledger.py refuses the error-review + # shape outright besides, so a sentinel there could only ever be misleading; the + # all-failed body carries no such shape, which is why withholding it is the control. + review_header = "{}\n{}".format( + header, render_round_sentinel(args.commit_sha, args.base_sha, args.merge_base_sha) + ) + banners = "" if args.notice: # Surface a degradation banner (judge failed → raw panel findings) right # under the title so every rendered body carries it. - header += f"\n\n{neutralize_mentions(args.notice)}" + banners += f"\n\n{neutralize_mentions(args.notice)}" if args.ledger_note and args.ledger_note.strip(): # Either "Round N — ledger: …" or the ledger-unavailable banner. The # banner case matters most: a re-review that ran WITHOUT prior context # must never look identical to a genuine first-round review. - header += f"\n\n_{neutralize_mentions(args.ledger_note.strip())}_" + banners += f"\n\n_{neutralize_mentions(args.ledger_note.strip())}_" + header += banners + review_header += banners if args.error_message: post_error_review(args.repo, args.pr_number, args.commit_sha, header, args.error_message) @@ -2879,6 +2974,16 @@ def main(): # misleading on (2), so check the panel metadata explicitly. all_failed = bool(panel) and all(c.get("status") != "ok" for c in panel) if all_failed: + # The PLAIN header, no round sentinel — same rule as post_error_review, and + # the same reason (BE-15598). Every reviewer errored, so this round diffed + # nothing and judged nothing; recording what it "diffed against" would let + # the NEXT round build its "already reviewed" side out of it and subtract + # hunks no panel ever saw. build-ledger.py still counts this body as a round + # for `last_reviewed_sha` — it IS a review of that commit — but with no + # sentinel it records no merge base, so the next round fails closed to "no + # incremental block" and the panel sees the full diff. That is the correct + # trade: the block is a prioritization hint, and losing it costs prompt + # budget where trusting this round costs coverage. body_text = ( f"{header}\n\n⚠️ **Panel did not produce any findings.**\n\n" "Every reviewer in the matrix failed to contribute — see the " @@ -2886,7 +2991,7 @@ def main(): "the underlying cause." ) else: - body_text = f"{header}\n\n✅ No high-signal findings." + body_text = f"{review_header}\n\n✅ No high-signal findings." if panel_summary: body_text += f"\n\n{panel_summary}" payload = json.dumps( @@ -2946,7 +3051,7 @@ def main(): # fallback) can list ALL findings once, in severity order, instead of appending the # inline half AFTER a block that already ends with the demoted half — which put a # demoted nit ahead of a lost critical and made the size clamp cut the wrong end. - review_head = f"{header}\n\nFound **{len(enriched)}** finding(s)." + review_head = f"{review_header}\n\nFound **{len(enriched)}** finding(s)." if repeats_dropped: review_head += ( # "the judge declared", not "of already-answered findings": the cap now diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 252b667..2394f2a 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -2439,3 +2439,245 @@ def test_both_audiences_are_told_what_the_marker_means(self): if __name__ == "__main__": unittest.main() + + +# --------------------------------------------------------------------------- # +# The round sentinel: what the last round diffed against (BE-15598) # +# --------------------------------------------------------------------------- # + +_HEAD = "a" * 40 +_BASE = "b" * 40 +_MERGE_BASE = "c" * 40 + + +def round_body(head=_HEAD, base=_BASE, merge_base=_MERGE_BASE, extra=""): + """A consolidated review body carrying a round sentinel, rendered by the WRITER. + + Through `pr.render_round_sentinel` rather than a literal here, so the two spellings + can never drift: a change to the writer's f-string fails this file rather than being + quietly carried along by a copy of the old one. + """ + return ( + f"{MARKER}\n{pr.render_round_sentinel(head, base, merge_base)}" + f"\n\nFound **1** finding(s).{extra}" + ) + + +class TestRoundSentinel(unittest.TestCase): + """The ledger records `last_reviewed_sha`; this records what that round diffed it + against, so the next round's incremental block can pin its "already reviewed" patch + to the merge base round N really used instead of recomputing one from a base that + has since moved (BE-15597). Every refusal below lands on "" for both keys, which + makes the next round skip its block — never fall back to the current base.""" + + def test_the_writers_own_render_round_trips(self): + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=round_body())], [], []) + self.assertEqual(ledger["last_reviewed_sha"], _HEAD) + self.assertEqual(ledger["last_reviewed_merge_base"], _MERGE_BASE) + self.assertEqual(ledger["last_reviewed_base_sha"], _BASE) + + def test_only_the_last_round_is_read(self): + """The next block diffs against the LAST round, so an earlier round's sentinel + is not merely irrelevant — using it would name the wrong tree entirely.""" + older = "d" * 40 + reviews = [ + review(1, 1, sha=older, body=round_body(head=older, merge_base="e" * 40)), + review(2, 2, sha=_HEAD, body=round_body()), + ] + ledger = bl.build_ledger(reviews, [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], _MERGE_BASE) + + def test_a_last_round_without_a_sentinel_records_nothing(self): + """Every live PR's first round after rollout, and the fail-closed case that + costs that round its incremental block rather than shrinking it.""" + reviews = [ + review(1, 1, sha="f" * 40, body=round_body(head="f" * 40)), + review(2, 2, sha=_HEAD), + ] + ledger = bl.build_ledger(reviews, [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + self.assertEqual(ledger["last_reviewed_base_sha"], "") + + def test_a_sentinel_naming_a_different_head_is_refused(self): + """`head` must be the review's own `commit_id`. A sentinel copied off another + round — or another PR — names a merge base that was never this commit's.""" + body = round_body(head="9" * 40) + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + + def test_a_review_with_no_commit_id_is_refused(self): + """`"" == ""` must not be how a sentinel gets accepted.""" + body = round_body(head="") + ledger = bl.build_ledger([review(1, 1, sha="", body=body)], [], []) + self.assertEqual(ledger["last_reviewed_sha"], "") + self.assertEqual(ledger["last_reviewed_merge_base"], "") + + def test_an_unknown_version_bad_json_or_a_non_hex_sha_is_refused(self): + cases = { + "v2": f'{MARKER}\n', + "bad json": f"{MARKER}\n", + "cut mid-payload": f'{MARKER}\n', + "merge_base not a string": f'{MARKER}\n', + "short sha": f'{MARKER}\n', + "uppercase sha": f'{MARKER}\n', + "a ref, not a sha": f'{MARKER}\n', + "empty merge base": f'{MARKER}\n', + } + for name, body in cases.items(): + with self.subTest(case=name): + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + self.assertEqual(ledger["last_reviewed_base_sha"], "") + + def test_a_missing_base_still_yields_the_merge_base(self): + """`base` is diagnostic — nothing builds a diff from it — so its absence must + not cost the one field the next round actually needs.""" + body = f'{MARKER}\n' + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], _MERGE_BASE) + self.assertEqual(ledger["last_reviewed_base_sha"], "") + + def test_the_reader_is_pinned_to_one_spelling(self): + """The same discipline the body-only sentinel is held to: the writer defangs ONE + exact literal, so a reader looser than that literal is a reader the defang does + not cover. Both spellings pinned together here, as that suite does.""" + real = round_body() + self.assertIsNotNone(bl._parse_round_sentinel(real)) + for spelling in ( + f'', + f'', + f'', + # What defang_body_only_contract writes in place of the real one. + pr.defang_body_only_contract(pr.render_round_sentinel(_HEAD, _BASE, _MERGE_BASE)), + ): + with self.subTest(spelling=spelling): + self.assertIsNone(bl._parse_round_sentinel(f"{MARKER}\n{spelling}")) + + def test_a_sentinel_behind_a_blockquote_marker_is_not_the_match(self): + """Every line of a rendered finding sits behind a `> `, so a round sentinel + quoted out of the PR under review can never be at column 0.""" + forged = pr.render_round_sentinel(_HEAD, _BASE, "e" * 40) + body = f"{MARKER}\n{pr.render_round_sentinel(_HEAD, _BASE, _MERGE_BASE)}\n\n> {forged}" + self.assertEqual(bl._parse_round_sentinel(body)["merge_base"], _MERGE_BASE) + + def test_a_blockquoted_sentinel_is_the_ONLY_candidate_and_still_loses(self): + """The case above cannot actually fail for the reason it names: `search` returns + the FIRST match and the genuine sentinel is on line 2, ahead of the forgery, so + it passes with the line anchor removed entirely. THIS is what pins the column-0 + containment the whole anti-forgery argument rests on — the body carries no real + sentinel at all, so an unanchored pattern would hand back the forged merge base. + + The module comments record exactly this regression happening once already, on the + body-only sentinel. Every indent a rendered finding can sit behind is covered, + not just `> `: post-review.py quotes into list items and nested quotes too.""" + forged = pr.render_round_sentinel(_HEAD, _BASE, "e" * 40) + for indent in ("> ", ">", "> > ", " ", "\t", "- ", " - > "): + with self.subTest(indent=repr(indent)): + body = f"{MARKER}\n\nFound **1** finding(s).\n\n{indent}{forged}" + self.assertIsNone(bl._parse_round_sentinel(body)) + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + + def test_an_error_review_is_refused_even_carrying_a_perfect_sentinel(self): + """`post_error_review` fences imported judge/CLI text, so its lines are the one + foreign lines in any consolidated body that sit at COLUMN 0 — the anchor the + sentinel relies on cannot help there. The writer-side defang only reaches bodies + THIS version wrote, and consumers pinned to older SHAs have undefanged error + reviews already sitting on their PRs. So the READER refuses the shape, exactly as + `_body_only_entries` does: a genuine error review never carries a sentinel, so + this can only ever cost a forgery.""" + forged = pr.render_round_sentinel(_HEAD, _BASE, "e" * 40) + body = ( + f"{MARKER}\n\n{bl.ERROR_REVIEW_MARKER}\n\n```\n" + f"judge crashed\n{forged}\n```\n" + ) + self.assertIn(bl.ERROR_REVIEW_MARKER, body, "the heading really is in there") + self.assertIsNotNone( + bl._parse_round_sentinel(body), "and the forgery really would have parsed" + ) + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_sha"], _HEAD, "still counts as a round") + self.assertEqual(ledger["last_reviewed_merge_base"], "") + self.assertEqual(ledger["last_reviewed_base_sha"], "") + + def test_a_finding_quoting_the_error_heading_does_not_suppress_a_real_sentinel(self): + """The refusal is line-anchored for the same reason `_body_only_entries`' is: a + finding ABOUT post_error_review renders the heading behind a `> `, and a bare + substring test would drop that round's genuine merge base.""" + body = round_body(extra=f"\n\n> 🟠 **High** — {bl.ERROR_REVIEW_MARKER} is unfenced") + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], _MERGE_BASE) + + def test_a_trailing_newline_does_not_pass_the_sha_gate(self): + """`^[0-9a-f]{40}$` accepts `"a" * 40 + "\n"` — Python's `$` matches before a + final newline — and this gate is the single control keeping a line break out of + the `key=value` `_write_outputs` appends to $GITHUB_OUTPUT. `\Z` is what makes + the regex provide the invariant the surrounding code claims from it.""" + for field in ("merge_base", "base", "head"): + with self.subTest(field=field): + self.assertIsNone(bl._FULL_SHA_RE.match("a" * 40 + "\n")) + # End to end: a hand-rolled payload (the writer can no longer emit one) whose + # merge base carries the newline must reach the ledger as "". + payload = ( + '{"base":"%s","head":"%s","merge_base":"%s\\n"}' % (_BASE, _HEAD, _MERGE_BASE) + ) + body = f"{MARKER}\n\n\nFound **1** finding(s)." + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + + def test_the_parser_never_raises(self): + for body in (None, "", MARKER, ""): + with self.subTest(body=str(body)[:40]): + self.assertIsNone(bl._parse_round_sentinel(body)) + + def test_both_keys_are_present_on_every_degraded_ledger(self): + """A consumer reads `ledger["last_reviewed_merge_base"]` unconditionally; a + KeyError on the degraded shapes would take down the whole ledger job.""" + for name, ledger in ( + ("empty", bl.build_ledger([], [], [])), + ("unknown", bl.unknown_ledger("GET reviews", "HTTP 502")), + ("disabled", bl.disabled_ledger()), + ("ok", bl.build_ledger([review(1, 1, sha=_HEAD, body=round_body())], [], [])), + ): + with self.subTest(status=name): + self.assertIn("last_reviewed_merge_base", ledger) + self.assertIn("last_reviewed_base_sha", ledger) + + def test_both_keys_reach_github_output(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "out") + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=round_body())], [], []) + with mock.patch.dict(os.environ, {"GITHUB_OUTPUT": path}, clear=False): + bl._write_outputs(ledger) + with open(path, encoding="utf-8") as f: + written = dict( + line.split("=", 1) for line in f.read().splitlines() if "=" in line + ) + self.assertEqual(written["last_reviewed_merge_base"], _MERGE_BASE) + self.assertEqual(written["last_reviewed_base_sha"], _BASE) + self.assertEqual(written["last_reviewed_sha"], _HEAD) + + def test_a_degraded_ledger_writes_both_keys_empty(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "out") + with mock.patch.dict(os.environ, {"GITHUB_OUTPUT": path}, clear=False): + bl._write_outputs(bl.unknown_ledger("GET reviews", "HTTP 502")) + with open(path, encoding="utf-8") as f: + written = dict( + line.split("=", 1) for line in f.read().splitlines() if "=" in line + ) + self.assertEqual(written["last_reviewed_merge_base"], "") + self.assertEqual(written["last_reviewed_base_sha"], "") + + def test_a_non_hex_base_is_dropped_rather_than_written_through(self): + """`base` is diagnostic, but `_write_outputs` appends it to $GITHUB_OUTPUT — + where a value carrying a newline is an output-injection vector. One shape check + over the whole payload, not two.""" + body = ( + f'{MARKER}\n' + ) + ledger = bl.build_ledger([review(1, 1, sha=_HEAD, body=body)], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], _MERGE_BASE) + self.assertEqual(ledger["last_reviewed_base_sha"], "") diff --git a/.github/cursor-review/tests/test_incremental_diff.py b/.github/cursor-review/tests/test_incremental_diff.py index 25d5f3d..c4b600c 100644 --- a/.github/cursor-review/tests/test_incremental_diff.py +++ b/.github/cursor-review/tests/test_incremental_diff.py @@ -14,6 +14,13 @@ with a real merge commit, so it fails against the old commit-range formulation rather than only against a hand-written fixture. +`TestRetargetedBase` is the second real repro (BE-15598), for the half the rewrite +left open: OLD was still `git diff BASE...LAST_REVIEWED`, which re-resolves its merge +base from the CURRENT base, so a retarget made hunks the panel had never seen read as +already reviewed and dropped them from the block — under a `check` that passes, since +a block that is too small is still a subset. OLD is now pinned to the merge base the +previous round recorded in its own review. + Run: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' """ @@ -156,6 +163,134 @@ def test_unchanged_file_is_not_re_emitted_after_a_merge(self): self.assertEqual(inc.build(old, new), "") +# --------------------------------------------------------------------------- # +# 1b. The second headline case: the PR is RETARGETED between rounds # +# --------------------------------------------------------------------------- # + + +class TestRetargetedBase(unittest.TestCase): + """A real repo, a real retarget: the OLD side must be pinned, not recomputed. + + `git diff BASE...LAST_REVIEWED` is right only while BASE resolves to the merge base + round N actually used. Retarget the PR (or rewrite its base branch) and it does not: + the three-dot form silently re-resolves to a DIFFERENT merge base, and everything + that entered the branch from the old base now shows up on both sides — so hunks the + panel has never seen are subtracted as "already reviewed" and dropped from the + block. Nothing downstream catches it: the block is smaller, and a smaller block is + still a subset, so `check` passes (BE-15597). + + The repo below is the minimum that reproduces it. + + ROOT ──────────────── release (B2: the NEW base, after the retarget) + └── B1 (main edits lib.py) + └── L (round 1's reviewed head: edits app.py) + └── H (round 2's head: edits app.py again) + + Round 1 ran with base=B1, so it reviewed `B1...L` — app.py only; the panel has never + been shown lib.py. Round 2 retargets onto `release`, so its reviewed diff is + `ROOT...H`, which DOES carry lib.py (it came into the branch with the fork point). + Rebuilding OLD as `ROOT...L` puts that same lib.py section on both sides. + """ + + @classmethod + def setUpClass(cls): + if shutil.which("git") is None: # pragma: no cover - CI always has git + raise unittest.SkipTest("git not available") + cls.repo = tempfile.mkdtemp(prefix="inc-diff-retarget-") + repo = cls.repo + _git(repo, "init", "-q", "-b", "main") + _git(repo, "config", "user.email", "test@example.invalid") + _git(repo, "config", "user.name", "Test") + _write(repo, "app.py", "one\ntwo\nthree\n") + _write(repo, "lib.py", "lib one\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "root") + cls.root = _rev(repo, "HEAD") + + # The branch the PR is retargeted ONTO, left at ROOT. + _git(repo, "branch", "release") + + # main moves first: it edits lib.py. B1 is round 1's base. + _write(repo, "lib.py", "lib one\nlib two from main\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "main edits lib.py") + cls.base_round_1 = _rev(repo, "HEAD") + + # The PR forks from B1 and edits app.py. Round 1 reviews exactly this. + _git(repo, "checkout", "-q", "-b", "pr") + _write(repo, "app.py", "one\nTWO\nthree\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "pr round 1") + cls.last_reviewed = _rev(repo, "HEAD") + + # Round 2: the PR is retargeted onto `release` (still ROOT) and gains a commit. + _write(repo, "app.py", "one\nTWO\nTHREE\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "pr round 2") + cls.head = _rev(repo, "HEAD") + cls.base_round_2 = _rev(repo, "release") + + # What round 1 recorded in its own review: merge-base(B1, L) — which is B1. + cls.recorded_merge_base = _git( + repo, "merge-base", cls.base_round_1, cls.last_reviewed + ).strip() + + @classmethod + def tearDownClass(cls): + shutil.rmtree(cls.repo, ignore_errors=True) + + def _three_dot(self, a, b): + return _git(self.repo, "-c", "core.quotePath=false", "diff", f"{a}...{b}", "--", ".") + + def _two_dot(self, a, b): + return _git(self.repo, "-c", "core.quotePath=false", "diff", a, b, "--", ".") + + def test_the_retarget_really_moved_the_merge_base(self): + """The precondition. Without it the rest of this class proves nothing.""" + self.assertEqual(self.recorded_merge_base, self.base_round_1) + self.assertEqual( + _git(self.repo, "merge-base", self.base_round_2, self.head).strip(), self.root + ) + self.assertNotEqual(self.recorded_merge_base, self.root) + + def test_round_one_never_saw_lib_py(self): + """It is not in round 1's reviewed diff, so the panel has never been shown it.""" + self.assertNotIn("lib.py", self._three_dot(self.base_round_1, self.last_reviewed)) + + def test_round_two_reviews_it(self): + self.assertIn("lib.py", self._three_dot(self.base_round_2, self.head)) + + def test_the_old_formulation_drops_a_hunk_the_panel_never_saw(self): + """The bug, pinned: OLD rebuilt against the CURRENT base hides lib.py.""" + old = self._three_dot(self.base_round_2, self.last_reviewed) + new = self._three_dot(self.base_round_2, self.head) + self.assertNotIn("lib.py", inc.build(old, new)) + + def test_and_the_subset_fail_safe_cannot_catch_that(self): + """Which is why the fix has to be the pin, not another check: a block that is + too SMALL is still a subset of the reviewed diff.""" + old = self._three_dot(self.base_round_2, self.last_reviewed) + new = self._three_dot(self.base_round_2, self.head) + foreign, new_lines, full_lines = inc.check(inc.build(old, new), new) + self.assertEqual(foreign, 0) + self.assertLessEqual(new_lines, full_lines) + + def test_pinning_old_to_the_recorded_merge_base_keeps_it(self): + old = self._two_dot(self.recorded_merge_base, self.last_reviewed) + new = self._three_dot(self.base_round_2, self.head) + block = inc.build(old, new) + self.assertIn("lib.py", block) + self.assertIn("lib two from main", block) + self.assertIn("app.py", block, "this round's own edit is still prioritized") + + def test_the_pinned_block_still_passes_the_fail_safe(self): + old = self._two_dot(self.recorded_merge_base, self.last_reviewed) + new = self._three_dot(self.base_round_2, self.head) + foreign, new_lines, full_lines = inc.check(inc.build(old, new), new) + self.assertEqual(foreign, 0) + self.assertLessEqual(new_lines, full_lines) + + # --------------------------------------------------------------------------- # # 2. The behaviours the rewrite must preserve # # --------------------------------------------------------------------------- # @@ -635,11 +770,48 @@ def test_the_old_patch_is_built_with_quotepath_off(self): """It must match the NEW side, which check-pr-size builds with `-c core.quotePath=false`; under the default a non-ASCII path arrives C-quoted on one side and plain on the other, so the two never key - alike and the file is re-emitted in full on every round.""" + alike and the file is re-emitted in full on every round. + + Two-dot against the RECORDED merge base since BE-15598 — the exact left tree + round N diffed — which is the same two-tree diff check-pr-size's own + `mergeBase...head` resolves to on the NEW side.""" self._assert_has( - 'git -c core.quotePath=false diff "${BASE_SHA}...${LAST_REVIEWED_SHA}"' + 'git -c core.quotePath=false diff "${LAST_REVIEWED_MERGE_BASE}" "${LAST_REVIEWED_SHA}"' ) + def test_the_step_never_diffs_old_against_the_current_base(self): + """The BE-15598 bug, pinned. After a retarget or a base-branch rewrite the + current base resolves to a merge base round N never used, so hunks the panel + has never seen read as already-reviewed and vanish from the block — and the + subset fail-safe cannot catch it, because a block that is too SMALL is still + a subset.""" + needle = '"${BASE_SHA}...${LAST_REVIEWED_SHA}"' + self.assertFalse(needle in self.text, f"cursor-review.yml is back on {needle!r}") + + def test_the_step_fails_closed_without_a_recorded_merge_base(self): + """No recorded merge base means no honest OLD patch, so there is no block — + never a fall back to ${BASE_SHA}, which is the bug above.""" + self._assert_has('[ -z "$LAST_REVIEWED_MERGE_BASE" ]') + self._assert_has("No recorded merge base for round") + self._assert_has("LAST_REVIEWED_MERGE_BASE: ${{ needs.ledger.outputs.last_reviewed_merge_base }}") + self._assert_has("last_reviewed_merge_base: ${{ steps.build.outputs.last_reviewed_merge_base }}") + + def test_the_recorded_merge_base_is_checked_before_it_is_diffed(self): + """Reachable in THIS checkout, and still an ancestor of the commit it was the + merge base OF — otherwise `git diff ` succeeds and + returns something that is not what round N saw at all.""" + self._assert_has('git cat-file -e "${LAST_REVIEWED_MERGE_BASE}^{commit}"') + self._assert_has('git merge-base --is-ancestor "$LAST_REVIEWED_MERGE_BASE" "$LAST_REVIEWED_SHA"') + self._assert_has("is unreachable or not an ancestor of the last-reviewed commit") + + def test_the_round_merge_base_is_resolved_and_published(self): + """In SHELL, in the diff-size job — not out of check-pr-size, which is skipped + entirely on the degraded raw-numstat fallback path. A round that records no + merge base costs the NEXT round its whole block.""" + self._assert_has('MERGE_BASE="$(git merge-base "$BASE_SHA" "$HEAD_SHA" 2>/dev/null || true)"') + self._assert_has("merge_base_sha: ${{ steps.merge_base.outputs.merge_base_sha }}") + self._assert_has('--merge-base-sha "$MERGE_BASE_SHA"') + def test_the_old_patch_is_size_bounded(self): """NEW is bounded by diff_size_cap; OLD is bounded by nothing — it keeps the generated-file sections the classifier strips out of the diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 3e7ade2..9efb995 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -103,6 +103,7 @@ def visible(body): for ln in body.splitlines() if not ln.startswith(f"")]) + + def _assert_directly_under_the_header(self, body): + lines = body.splitlines() + self.assertTrue( + lines[0].startswith("## 🔍 Cursor Review — Consolidated panel"), + f"the header is still the first line: {lines[0]!r}", + ) + self.assertEqual( + lines[1], self._sentinel_line(body), + "the sentinel is the SECOND line — the clamp cuts the tail, so this height " + "is what makes it survive every cut that leaves a body at all", + ) + + # -- the three success bodies ------------------------------------------- # + + def test_the_findings_body_carries_it_directly_under_the_header(self): + payload = self.run_main( + [finding("app.py", 11)], commit_sha=HEAD_40, extra_argv=self.ARGV + )[0] + self._assert_directly_under_the_header(payload["body"]) + self.assertEqual( + self._payload(payload["body"]), + {"base": BASE_40, "head": HEAD_40, "merge_base": MERGE_BASE_40}, + ) + + def test_the_no_findings_body_carries_it(self): + payload = self.run_main([], commit_sha=HEAD_40, extra_argv=self.ARGV)[0] + self.assertIn("✅ No high-signal findings.", payload["body"]) + self._assert_directly_under_the_header(payload["body"]) + + def test_the_wholesale_fallback_body_carries_it_too(self): + posted = self.run_main( + [finding("app.py", 11), finding("app.py", 999)], + commit_sha=HEAD_40, + extra_argv=self.ARGV, + post_returncode=1, + stderr="gh: Unprocessable Entity (HTTP 422)", + ) + self.assertEqual(len(posted), 2) + self._assert_directly_under_the_header(posted[1]["body"]) + + # -- and the two bodies that must NOT ----------------------------------- # + + def test_the_error_review_body_carries_no_sentinel(self): + """A round that failed reviewed nothing, so it has nothing to record about + what it diffed. build-ledger.py refuses the error-review shape outright, so a + sentinel there could only ever mislead a human reading the raw body.""" + posted = self.run_main( + [], + commit_sha=HEAD_40, + extra_argv=self.ARGV + ["--error-message", "judge exploded"], + ) + self.assertEqual(len(posted), 1) + self.assertIn("⚠️ **Review failed**", posted[0]["body"]) + self.assertNotIn(PR.ROUND_SENTINEL_PREFIX, posted[0]["body"]) + + def test_the_all_panel_cells_failed_body_carries_no_sentinel_either(self): + """Same rule, and the case the error review does NOT cover: every reviewer + errored, so this round judged nothing — but the body carries no "Review failed" + heading, so build-ledger.py's reader-side refusal does not reach it. Withholding + the sentinel at the WRITER is therefore the only control, and without it the next + round would rebuild its "already reviewed" side from a panel that never ran and + subtract hunks nobody looked at. It is posted with `delivers=False` for the very + same reason.""" + posted = self.run_main( + [], + commit_sha=HEAD_40, + extra_argv=self.ARGV, + panel=[{"model": "m", "review_type": "adversarial", "status": "error"}], + ) + self.assertEqual(len(posted), 1) + self.assertIn("Panel did not produce any findings", posted[0]["body"]) + self.assertNotIn(PR.ROUND_SENTINEL_PREFIX, posted[0]["body"]) + self.assertNotIn(BL.ERROR_REVIEW_MARKER, posted[0]["body"], + "and the reader-side refusal genuinely does not cover it") + # Driven through the REAL parser, not a copy: it is still a round, so + # `last_reviewed_sha` advances — and it records no merge base, so the next + # round fails closed to "no incremental block" rather than to a bad one. + ledger = BL.build_ledger( + [{"id": 101, "state": "COMMENTED", "commit_id": HEAD_40, + "submitted_at": "2026-07-01T00:00:00Z", "body": posted[0]["body"], + "user": {"login": "github-actions[bot]", "type": "Bot"}}], + [], [], + ) + self.assertEqual(ledger["last_reviewed_sha"], HEAD_40) + self.assertEqual(ledger["last_reviewed_merge_base"], "") + + # -- the banners still render, and still render BELOW it ---------------- # + + def test_the_notice_and_ledger_banners_still_follow_it(self): + payload = self.run_main( + [finding("app.py", 11)], + commit_sha=HEAD_40, + extra_argv=self.ARGV + [ + "--triggered-by", "someone", + "--notice", "judge degraded", + "--ledger-note", "Round 2 — ledger: 1 entry", + ], + )[0] + body = payload["body"] + self.assertIn("_Triggered by @someone._", body) + self.assertLess( + body.index("_Triggered by @someone._"), + body.index(PR.ROUND_SENTINEL_PREFIX), + "attribution is part of the header line block, so it precedes the sentinel", + ) + for banner in ("judge degraded", "Round 2 — ledger: 1 entry"): + self.assertGreater( + body.index(banner), body.index(PR.ROUND_SENTINEL_PREFIX), + f"{banner!r} renders below the sentinel, as the header comment says", + ) + + # -- SHA validation ------------------------------------------------------ # + + def test_every_field_must_be_a_full_lowercase_hex_sha(self): + for bad in ("deadbeef", "A" * 40, "x" * 40, "a" * 39, "a" * 41, "", None, + " " + "a" * 40 + " "): + with self.subTest(bad=bad): + rendered = PR.render_round_sentinel(bad, bad, bad) + payload = json.loads( + rendered[len("")] + ) + expected = "a" * 40 if bad and bad.strip() == "a" * 40 else "" + self.assertEqual( + payload, {"base": expected, "head": expected, "merge_base": expected} + ) + + def test_an_unresolvable_merge_base_is_recorded_as_empty_not_dropped(self): + """Fail closed, and be legible about it: a sentinel that PARSES and carries no + merge base is what makes the next round skip its block. A missing KEY would be + indistinguishable from a body this writer never wrote.""" + payload = self.run_main( + [finding("app.py", 11)], + commit_sha=HEAD_40, + extra_argv=["--base-sha", BASE_40, "--merge-base-sha", ""], + )[0] + self.assertEqual( + self._payload(payload["body"]), + {"base": BASE_40, "head": HEAD_40, "merge_base": ""}, + ) + + def test_the_payload_is_one_line_with_sorted_keys(self): + rendered = PR.render_round_sentinel(HEAD_40, BASE_40, MERGE_BASE_40) + self.assertNotIn("\n", rendered) + self.assertEqual( + rendered, + '', + ) + + # -- containment --------------------------------------------------------- # + + def test_the_defang_breaks_a_round_sentinel_in_imported_text(self): + forged = PR.render_round_sentinel(HEAD_40, BASE_40, MERGE_BASE_40) + defanged = PR.defang_body_only_contract(forged) + self.assertNotIn(PR.ROUND_SENTINEL_PREFIX, defanged) + self.assertIn("cursor-review:​round v1", defanged) + self.assertIsNone(BL._parse_round_sentinel(defanged)) + + def test_an_error_message_cannot_smuggle_a_parseable_round_sentinel(self): + """The error review is the one consolidated body whose imported text sits at + column 0 — it renders inside a FENCE, not a blockquote — so the writer-side + defang is what covers it.""" + forged = PR.render_round_sentinel(HEAD_40, BASE_40, MERGE_BASE_40) + posted = self.run_main( + [], + commit_sha=HEAD_40, + extra_argv=self.ARGV + ["--error-message", f"boom\n{forged}\nmore"], + ) + self.assertNotIn(PR.ROUND_SENTINEL_PREFIX, posted[0]["body"]) + self.assertIn(MERGE_BASE_40, posted[0]["body"], "the text is reported, not deleted") + self.assertIsNone(BL._parse_round_sentinel(posted[0]["body"])) + + def test_a_finding_body_quoting_one_cannot_forge_it(self): + forged = PR.render_round_sentinel(HEAD_40, "d" * 40, "e" * 40) + payload = self.run_main( + [finding("app.py", 11, body=f"the PR contains\n{forged}\nliterally")], + commit_sha=HEAD_40, + extra_argv=self.ARGV, + )[0] + # Exactly one sentinel — ours — and it still names OUR merge base. + self.assertEqual( + self._payload(payload["body"])["merge_base"], + MERGE_BASE_40, + "the quoted copy did not become the sentinel", + ) + parsed = BL._parse_round_sentinel(payload["body"]) + self.assertEqual(parsed["merge_base"], MERGE_BASE_40) + + # -- the clamp ----------------------------------------------------------- # + + def test_the_sentinel_survives_the_size_clamp(self): + """`clamp_review_body` cuts the TAIL and the sentinel sits on line two, so it + is never what a cut takes — which is the whole reason for that placement.""" + # Unanchorable on purpose: they are demoted into the BODY, which is the only + # way one round's prose reaches the size limit at all. + findings = [ + finding("elsewhere.py", 100 + i, body="x" * 900) for i in range(200) + ] + payload = self.run_main(findings, commit_sha=HEAD_40, extra_argv=self.ARGV)[0] + body = payload["body"] + self.assertIn("truncated here", body, "the clamp really did fire") + self.assertEqual(len(body), PR.MAX_REVIEW_BODY_CHARS) + self._assert_directly_under_the_header(body) + self.assertEqual( + BL._parse_round_sentinel(body), + {"base": BASE_40, "head": HEAD_40, "merge_base": MERGE_BASE_40}, + ) + + def test_a_clamped_body_still_hands_the_next_round_its_merge_base(self): + """End to end through the real reader, not just the regex.""" + payload = self.run_main( + [finding("elsewhere.py", 100 + i, body="y" * 900) for i in range(200)], + commit_sha=HEAD_40, + extra_argv=self.ARGV, + )[0] + review = { + "id": 7, + "state": "COMMENTED", + "commit_id": HEAD_40, + "submitted_at": "2026-09-01T00:00:00Z", + "body": payload["body"], + "user": {"login": "github-actions[bot]", "type": "Bot"}, + } + ledger = BL.build_ledger([review], [], []) + self.assertEqual(ledger["last_reviewed_merge_base"], MERGE_BASE_40) + self.assertEqual(ledger["last_reviewed_base_sha"], BASE_40) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 1ab36db..8855d92 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -594,6 +594,13 @@ jobs: status: ${{ steps.build.outputs.status || steps.fallback.outputs.status }} rounds: ${{ steps.build.outputs.rounds || steps.fallback.outputs.rounds }} last_reviewed_sha: ${{ steps.build.outputs.last_reviewed_sha }} + # The merge base the LAST round recorded in its own review (BE-15598), read back + # out of the round sentinel. The `incremental` step in `diff-size` builds its OLD + # patch against exactly this commit, and skips the block outright when it is empty + # — there is no fallback to the current base, which is what made a retargeted PR + # drop hunks the panel had never seen. Like `last_reviewed_sha` this has no + # fallback-step reading: a degraded ledger legitimately records nothing. + last_reviewed_merge_base: ${{ steps.build.outputs.last_reviewed_merge_base }} steps: - name: Resolve the asset ref # This job must never fail (see the job comment), so the fail-closed @@ -794,6 +801,14 @@ jobs: # should report. Written with `!= 'false'` rather than read straight # through so the skipped-step empty string reads as `true`, not `''`. incremental_subset: ${{ steps.incremental.outputs.incremental_subset != 'false' }} + # The merge base THIS round's reviewed diff was taken against — the exact left + # tree `check-pr-size` resolves `BASE_SHA...HEAD_SHA` to. Consumed by `post-review` + # (BE-15598), which writes it into the consolidated review's round sentinel so the + # NEXT round can rebuild "what the panel already saw" against this tree rather than + # recomputing a merge base from a base that may have moved since (a retarget, a + # base-branch rewrite). Empty when git could not resolve it, which the next round + # reads as "no recorded merge base" and fails closed to no incremental block. + merge_base_sha: ${{ steps.merge_base.outputs.merge_base_sha }} steps: - name: Checkout PR head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -860,6 +875,39 @@ jobs: exit 1 fi + - name: Resolve this round's merge base + # Recorded in the consolidated review by `post-review`, and read back by the + # NEXT round to pin the OLD side of its incremental block (BE-15598). Plain + # shell rather than a check-pr-size output on purpose: the tool does resolve + # the same commit internally, but it is skipped entirely on the degraded + # raw-numstat fallback path, and a round that records no merge base costs the + # NEXT round its whole incremental block. This step runs on both paths. + # + # Best-effort by construction — it must never fail the job. `git merge-base` + # exits non-zero when the two commits share no history (an unrelated-histories + # base, a base ref the fetch-depth:0 checkout still cannot see), and there is + # nothing to do about that except record nothing and say so. + id: merge_base + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # + # No line-break guard on the value, unlike `resolveMergeBase` in + # scripts/check-pr-size/main.go, which rejects a result containing \r or \n. The + # divergence is deliberate, not an oversight: `git merge-base` with two commits + # and no --all prints exactly ONE line, and `$( )` strips every trailing newline + # off it, so there is no value this step can emit that the Go check would refuse. + # post-review.py's `field()` re-validates the shape before it reaches the + # sentinel besides, and build-ledger.py's `_FULL_SHA_RE` again on the way back. + run: | + MERGE_BASE="$(git merge-base "$BASE_SHA" "$HEAD_SHA" 2>/dev/null || true)" + if [ -n "$MERGE_BASE" ]; then + echo "Round merge base: ${MERGE_BASE}" + else + echo "::warning::Could not resolve the merge base of ${BASE_SHA} and ${HEAD_SHA} — this round records none, so the next round will skip its incremental diff block rather than diff against the wrong tree." + fi + echo "merge_base_sha=${MERGE_BASE}" >> "$GITHUB_OUTPUT" + - name: Load check-pr-size tool # The SAME classifier the PR-size cap uses — the single source of truth # for "what is codegen". Comes from THIS repo (public, pinned via @@ -1035,9 +1083,9 @@ jobs: # against a 1,234-line reviewed diff (119 of its 133 files outside the # PR) and two PRs built ~1.16M-line blocks; the prompt went from ~98 KB # to ~826 KB, most legs timed out, and the legs that finished reviewed - # files from the base branch rather than the PR. Both patches here are - # three-dot diffs against the base, so each carries only the branch's - # own changes and neither can name a base-branch commit. + # files from the base branch rather than the PR. Each patch here is taken + # against a MERGE BASE instead, so each carries only the branch's own + # changes and neither can name a base-branch commit. # # NEW is `pr-diff.patch` ITSELF — the diff this round is actually # reviewing — not a freshly-computed BASE...HEAD diff. Every emitted @@ -1046,20 +1094,42 @@ jobs: # file the classifier stripped from the reviewed diff can never reappear # in the block (which would trip the fail-safe and discard the lot). # - # The two patches can resolve to DIFFERENT merge bases — OLD to - # merge-base(BASE, LAST), NEW to merge-base(BASE, HEAD) — when the round - # merged or rebased the base branch in. A file the base branch AND the - # branch both touched then reads as changed even if the author's own - # edit did not move. That is over-inclusion strictly WITHIN the PR's own - # files, which costs a little prompt budget and never breaks the subset - # property; the alternative (pinning both to one merge base) would hide - # a hunk whose surrounding code really did change under it. + # Each patch is pinned to the merge base ITS OWN round used: OLD to the one + # round N recorded in its review's round sentinel, NEW (via check-pr-size) to + # merge-base(BASE, HEAD) for this round. Recomputing OLD's from the CURRENT + # base is the bug BE-15598 fixes — after a retarget or a base-branch rewrite + # `BASE...LAST_REVIEWED` diffs against a merge base round N never used, so + # hunks the panel has never seen read as "already reviewed" and are dropped + # from the block. The subset fail-safe cannot catch that: a block that is too + # SMALL is still a subset. With no recorded merge base there is no honest + # answer, so the step emits no block at all rather than guessing one. + # + # KNOWN, and not fixed here: on the retarget path this pin exists FOR, the two + # merge bases can be nearly disjoint, so almost nothing in NEW is in OLD and the + # block comes out close to the full reviewed diff — which the prompt then carries + # twice. `check` cannot catch it (`foreign == 0` and `new_lines <= full_lines` + # both hold), because the block really is a faithful subset; it is just a useless + # one. The cost is prompt budget on an uncommon path, never coverage, and the + # alternative — declining a block whose length approaches `full_lines` — needs a + # threshold nothing here has grounds to pick yet. Left as a known waste rather + # than guessed at. + # + # The two merge bases still differ whenever the base branch was merged in + # between rounds. A file the base branch AND the branch both touched then reads + # as changed even if the author's own edit did not move. That is over-inclusion + # strictly WITHIN the PR's own files, which costs a little prompt budget and + # never breaks the subset property; it is the accepted direction, because the + # alternative hides a hunk whose surrounding code really did change under it. id: incremental if: steps.check.outputs.over_cap != 'true' env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} LAST_REVIEWED_SHA: ${{ needs.ledger.outputs.last_reviewed_sha }} + # The merge base round N recorded, NOT this round's — see above. Empty for + # every round reviewed before the round sentinel existed, and for one whose + # sentinel did not parse or did not name the reviewed commit. + LAST_REVIEWED_MERGE_BASE: ${{ needs.ledger.outputs.last_reviewed_merge_base }} LEDGER_ROUNDS: ${{ needs.ledger.outputs.rounds }} run: | NEW_PATCH="${RUNNER_TEMP}/pr-diff-new.patch" @@ -1071,7 +1141,7 @@ jobs: INCREMENTAL_PY="_pr_size_tool/.github/cursor-review/incremental-diff.py" : > "$NEW_PATCH" # An OLD patch larger than this is declined rather than parsed. NEW is - # bounded by diff_size_cap; OLD is NOT — `git diff BASE...LAST` keeps + # bounded by diff_size_cap; OLD is NOT — the last-reviewed patch keeps # the generated-file sections the classifier strips out of the # reviewed diff, and those cost nothing against that cap — so a PR # comfortably under the review cap can still hand this step a patch @@ -1086,10 +1156,15 @@ jobs: # non-ASCII path arrives C-quoted here and PLAIN there, so the two # sides key differently and every such file is re-emitted in full on # every round — including a pure rebase. + # TWO-DOT, against the RECORDED merge base: that commit is the exact left + # tree round N diffed, so this is the same two-tree diff its own + # `mergeBase...head` resolved to — and check-pr-size builds NEW the same + # way. Three dots here would re-resolve a merge base from a ref that has + # already moved, which is precisely the bug (BE-15598). # $DIFF_EXCLUDES is intentionally unquoted so bash word-splits it # into pathspec args. # shellcheck disable=SC2086 - git -c core.quotePath=false diff "${BASE_SHA}...${LAST_REVIEWED_SHA}" -- . $DIFF_EXCLUDES > "$OLD_PATCH" + git -c core.quotePath=false diff "${LAST_REVIEWED_MERGE_BASE}" "${LAST_REVIEWED_SHA}" -- . $DIFF_EXCLUDES > "$OLD_PATCH" } # Default the output to the honest "nothing was discarded" value, and # let the fail-safe below append 'false' over it — same last-value-wins @@ -1103,6 +1178,22 @@ jobs: echo "No usable last-reviewed SHA — skipping the incremental diff block." elif ! git cat-file -e "${LAST_REVIEWED_SHA}^{commit}" 2>/dev/null; then echo "Last-reviewed SHA ${LAST_REVIEWED_SHA} is unreachable (force-push or base-branch rewrite) — skipping the incremental diff block." + elif [ -z "$LAST_REVIEWED_MERGE_BASE" ]; then + # Fail CLOSED, and do NOT fall back to ${BASE_SHA}: that fallback IS the bug + # (BE-15598). Every live PR sees this once after rollout — the round it is + # comparing against was posted before the sentinel existed — and then the + # block comes back on the round after, which is the first one with a + # recorded merge base to pin to. + echo "No recorded merge base for round ${LEDGER_ROUNDS:-?} (reviewed before the round sentinel existed, or the sentinel did not parse) — skipping the incremental diff block." + elif ! git cat-file -e "${LAST_REVIEWED_MERGE_BASE}^{commit}" 2>/dev/null \ + || ! git merge-base --is-ancestor "$LAST_REVIEWED_MERGE_BASE" "$LAST_REVIEWED_SHA" 2>/dev/null; then + # The recorded commit has to be reachable in THIS checkout and has to still + # be an ancestor of the commit it was the merge base of. A base-branch + # rewrite can drop the commit entirely, and a rewritten PR branch can leave + # it reachable but no longer in the last-reviewed commit's history — in + # which case `git diff ` succeeds and returns + # something that is not "what round N saw" at all. + echo "Recorded merge base ${LAST_REVIEWED_MERGE_BASE} is unreachable or not an ancestor of the last-reviewed commit ${LAST_REVIEWED_SHA} — skipping the incremental diff block." elif [ ! -s "$FULL_PATCH" ]; then echo "The reviewed diff is empty — skipping the incremental diff block." elif ! build_old_patch || [ ! -s "$OLD_PATCH" ]; then @@ -1112,9 +1203,9 @@ jobs: # doubled prompt that prioritizes nothing, which the fail-safe # below cannot catch either (nothing is foreign, and new_lines # EQUALS full_lines rather than exceeding it). An empty OLD is not - # an error git reports — it is simply what `BASE...LAST` yields - # whenever merge-base(BASE, LAST) is LAST itself (the base advanced - # past the last-reviewed commit, e.g. after a retarget), or + # an error git reports — it is simply what the recorded merge base + # against the last-reviewed commit yields whenever round N's own + # patch was empty (the recorded merge base IS that commit), or # whenever $DIFF_EXCLUDES filters every file out. echo "Could not build a non-empty last-reviewed patch (${LAST_REVIEWED_SHA}) — skipping the incremental diff block." elif [ "$(wc -c < "$OLD_PATCH")" -gt "$OLD_PATCH_MAX_BYTES" ]; then @@ -2464,7 +2555,11 @@ jobs: # checkout, and the diff comes from the `pr-diff` artifact `diff-size` # published rather than from the copy the judge job had shell access to. name: Post review - needs: [consolidate, ledger] + # `diff-size` for its `merge_base_sha` alone (BE-15598) — the round sentinel this + # job writes has to record the merge base THIS round's reviewed diff was taken + # against. Already a transitive dependency (consolidate → review → diff-size), so + # this adds no ordering, only the outputs. + needs: [consolidate, ledger, diff-size] # `!cancelled()` rather than `always()`, for the same reason # `over-cap-comment` documents at its own `if:`: the documented caller sets # `cancel-in-progress: true`, and `always()` runs a job even when the RUN is @@ -2725,6 +2820,12 @@ jobs: PR_NUMBER: ${{ github.event.pull_request.number }} REPO: ${{ github.repository }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # The two halves of the round sentinel post-review.py writes under the review + # header (BE-15598). BASE_SHA is diagnostic; MERGE_BASE_SHA is the one the + # NEXT round pins its "already reviewed" patch to, and an empty value is + # recorded as empty so that round skips its block rather than guessing. + BASE_SHA: ${{ github.event.pull_request.base.sha }} + MERGE_BASE_SHA: ${{ needs.diff-size.outputs.merge_base_sha }} TRIGGERED_BY: ${{ steps.meta.outputs.triggered_by }} JUDGE_STATUS: ${{ steps.meta.outputs.judge_status }} CONSOLIDATED_COUNT: ${{ steps.meta.outputs.consolidated_count }} @@ -2814,6 +2915,8 @@ jobs: --pr-number "$PR_NUMBER" \ --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ + --base-sha "$BASE_SHA" \ + --merge-base-sha "$MERGE_BASE_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ "${LEDGER_FLAGS[@]}" \ @@ -2832,6 +2935,8 @@ jobs: --pr-number "$PR_NUMBER" \ --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ + --base-sha "$BASE_SHA" \ + --merge-base-sha "$MERGE_BASE_SHA" \ --triggered-by "$TRIGGERED_BY" \ --diff /tmp/pr-diff.patch \ "${LEDGER_FLAGS[@]}" \ @@ -2857,6 +2962,8 @@ jobs: --pr-number "$PR_NUMBER" \ --repo "$REPO" \ --commit-sha "$HEAD_SHA" \ + --base-sha "$BASE_SHA" \ + --merge-base-sha "$MERGE_BASE_SHA" \ --triggered-by "$TRIGGERED_BY" \ "${LEDGER_FLAGS[@]}" \ --ledger-note "$LEDGER_NOTE" \ diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 92bc9b4..47c1cf3 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -144,7 +144,9 @@ see the spend warning below). **An over-cap PR gets no review, and now says so.** When the counted diff exceeds `diff_size_cap` the panel is skipped and the run still goes green — nothing about it is a failure. So the skip announces itself in three places instead: a `::warning::` annotation and a step-summary block on the *Diff size check* job (both credential-free, so they still show on Dependabot PRs, whose runs can't read Actions secrets), plus a sticky PR comment naming the counted total and the cap. Get the PR under the cap and **re-apply the label** — with the label-gated caller above a push alone starts no run — and that comment flips to ✅. The comment posts as your bot app when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are set and as `github-actions[bot]` otherwise, so it works out of the box; if the write fails it degrades to the annotation and the summary and the job log says why. The comment path is best-effort throughout — it never reddens the run. Note that **fork PRs get neither half**: the gate skips a cross-repo head before the size check runs, so a fork PR is skipped for being a fork, whatever its size. **Under `blocking: true` an over-cap PR does not go green** — the Blocking gate holds it red, because diff size is author-controlled and "too big to review" is not evidence a PR is clean; see [the blocking-gate gotchas](#blocking-gate-gotchas). -**The "hunks new since round N" block is always a subset of the diff being reviewed.** From round 2 onward the panel prompt carries a second block — the hunks new since the last reviewed commit — introduced as "the subset of the diff above". It is derived from two PR patches (`git diff BASE...LAST_REVIEWED` versus the reviewed diff this round is running on), each of which is a three-dot diff against the base and so contains only your branch's own changes, and every section it shows is copied verbatim out of the reviewed diff. It can therefore never contain a hunk your PR does not carry — in particular, merging the base branch into your branch no longer drags that branch's commits into the block (BE-15558; the old `git diff LAST_REVIEWED...HEAD` formulation did, because with a merge commit at HEAD the merge base of those two commits *is* `LAST_REVIEWED`). A pure rebase, which shifts line numbers without changing a hunk, produces no block at all rather than re-flagging the whole PR. +**The "hunks new since round N" block is always a subset of the diff being reviewed.** From round 2 onward the panel prompt carries a second block — the hunks new since the last reviewed commit — introduced as "the subset of the diff above". It is derived from two PR patches — the diff the previous round actually reviewed, taken against the merge base *that* round recorded (see the next note), versus the reviewed diff this round is running on — each of which is a diff against a merge base and so contains only your branch's own changes, and every section it shows is copied verbatim out of the reviewed diff. It can therefore never contain a hunk your PR does not carry — in particular, merging the base branch into your branch no longer drags that branch's commits into the block (BE-15558; the old `git diff LAST_REVIEWED...HEAD` formulation did, because with a merge commit at HEAD the merge base of those two commits *is* `LAST_REVIEWED`). A pure rebase, which shifts line numbers without changing a hunk, produces no block at all rather than re-flagging the whole PR. + +**A retargeted PR gets one round with no block, on purpose.** Each consolidated review that actually reviewed something now carries a hidden *round sentinel* recording the commit it reviewed and the merge base it was diffed against (a round that failed outright, or in which every reviewer errored, records none — so the round after it takes the fail-closed path below), and the next round rebuilds the "already reviewed" side against **that recorded merge base** rather than recomputing one from your PR's current base (BE-15598). It has to: change the PR's base branch — or rewrite that branch — and the recomputed merge base moves, so everything your branch inherited from the old base appears on both sides and hunks the panel has never seen are quietly subtracted as already reviewed. That loss is invisible to the subset check below, since a block that is merely too small is still a subset. So the step **fails closed** instead: when there is no usable recorded merge base — the previous round predates this change, its sentinel did not parse, or the recorded commit is unreachable or no longer an ancestor of the reviewed commit — the *Diff size check* job logs which case it hit (`No recorded merge base for round N …` / `Recorded merge base … is unreachable or not an ancestor …`) and the panel simply reviews the full diff with no prioritization block. Nothing is skipped and no finding is suppressed. Every open PR sees exactly one such round after this rolls out, because the round it is comparing against was posted before the sentinel existed; the block comes back on the round after that. The block is verified after it is built, and **discarded whole if the check fails**. If you see `::warning::Incremental diff discarded: it was not a subset of the reviewed diff ( foreign file(s), vs lines).` on the *Diff size check* job, it means some section of the block was not carried **byte for byte** by the reviewed diff — a file it does not have, or a hunk that did not match verbatim — or the block came out longer than it, and the whole thing was thrown away: the panel reviewed the **full diff alone**, which is always correct — it just lost the hint about where to spend budget first. Nothing was skipped and no finding was suppressed. The job also reports this as its `incremental_subset` output, which is `false` only in that discard case.