fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it

.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: <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 — `<!-- cursor-review:round v1 {"base":"…","head":"…","merge_base":"…"} -->`, 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 <that merge base> <last reviewed commit>`. 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 (<n> foreign file(s), <a> vs <b> 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.
 

.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 `<!--` in model prose besides), so a round sentinel quoted out of
+# the PR under review can never be at column 0 and can never be this match. The
+# WRITER-side defang covers the error review's fenced text, where imported lines do sit
+# at column 0; `build_ledger` additionally requires the payload's `head` to equal the
+# review's own `commit_id`, which no text a PR can plant knows how to satisfy for a
+# review it did not write.
+ROUND_SENTINEL_OPENER = "<!-- cursor-review:round v1 "
+_ROUND_SENTINEL_RE = re.compile(
+    r"(?:\A|(?<=" + _LINE_SEP_CLASS + r"))"
+    + re.escape(ROUND_SENTINEL_OPENER) + r"(" + _NOT_LINE_SEP_CLASS + r"*?)-->"
+)
+
+# 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 <merge base> <last reviewed>`, 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")