From da6cbc8c46eee15a26ba19f4d08465e84bc1fab5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 20:16:25 +0000 Subject: [PATCH 1/5] fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panel prompt introduces the "hunks new since round N" block as "the subset of the diff above". Built as `git diff LAST_REVIEWED...HEAD` it was not one: when HEAD is a merge commit that pulled the base branch into the branch, LAST is an ancestor of HEAD, the merge base of the two IS LAST, and the range therefore carries every commit that merge brought in. 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 other 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 base-branch files instead of the PR. Build it from two PR patches instead — `git diff BASE...LAST_REVIEWED` against the reviewed diff this round is running on. Both are three-dot diffs against the base, so neither can name a base-branch commit, and every emitted section is a verbatim slice of the reviewed diff, so the subset property holds by construction. Hunk comparison normalises `@@ -a,b +c,d @@` to `@@ @@`, so a pure rebase yields an empty block rather than re-reviewing the whole PR. The logic moves into .github/cursor-review/incremental-diff.py so it is testable, loaded from the same pinned checkout of this repo the classifier comes from — never the PR's own tree. Its `check` subcommand is the fail-safe the step runs afterwards: a block naming a path the reviewed diff lacks, or longer than the reviewed diff, is discarded whole and annotated, and the panel runs on the full diff alone. The new `incremental_subset` job output reports that verdict. --- .github/cursor-review/README.md | 5 + .github/cursor-review/incremental-diff.py | 224 +++++++++ .../tests/test_incremental_diff.py | 452 ++++++++++++++++++ .github/workflows/cursor-review.yml | 102 +++- docs/callers/cursor-review.md | 4 + 5 files changed, 780 insertions(+), 7 deletions(-) create mode 100755 .github/cursor-review/incremental-diff.py create mode 100644 .github/cursor-review/tests/test_incremental_diff.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index d30461d2..95ccb8eb 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -89,6 +89,10 @@ 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. + +The build is then **verified, not trusted**. If the block names a path the reviewed diff does not carry, or 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. + ### The panel | Lab | Model (Cursor catalog) | @@ -134,6 +138,7 @@ tagged `error` rather than silently vanishing. | [`install-cursor-cli.sh`](install-cursor-cli.sh) | Installs the Cursor agent CLI from the versioned, sha256-pinned release artifact — not `curl cursor.com/install \| bash`. Used by all three CLI-using jobs; the pin (`CURSOR_CLI_VERSION` / `CURSOR_CLI_SHA256`) lives in `cursor-review.yml`'s top-level `env:`. | | [`build-ledger.py`](build-ledger.py) | Builds the **prior-review ledger** — what earlier rounds raised on this PR and how the author answered — and splices it into the panel/judge prompts. Untrusted prose is defanged twice over: fence-opening lines are rewritten, and continuation lines of quoted prose are prefixed ` |` so no imported text can sit at a field's indent and forge one. Also the prompt splicer, so the no-ledger path is byte-identical to the pre-ledger prompt. | | [`fence-diff.py`](fence-diff.py) | Wraps the reviewed diff (plus the incremental hunks, and the judge's panel-findings block) in `=== BEGIN/END DIFF ===` fences. The diff is attacker-authored PR bytes, so static literal fences are not a control; the nonce is what a PR cannot forge. Each prompt-build step mints its OWN nonce (`mint`), into a shell variable rather than a step `env:` or job output — Actions dumps a step's env map into the public run log, and a per-prompt value means a leak in one job cannot forge a fence in another. Copies the body through **byte for byte** — it never defangs or normalizes the payload. | +| [`incremental-diff.py`](incremental-diff.py) | Builds the incremental "hunks new since the last reviewed round" block as the difference between two PR patches (`build`), and verifies the result really is a subset of the reviewed diff (`check`) so the workflow can discard it if it is not. Never a commit range: `git diff LAST_REVIEWED...HEAD` carries every commit a merge of the base branch brought in — see [the ledger section](#the-prior-review-ledger-and-the-repeat-policy). | | [`catalog-drift.py`](catalog-drift.py) | Backs the weekly catalog-drift check. Extracts the pins from `cursor-review.yml`, diffs them against raw `cursor-agent models` output, and renders the sticky issue title + body (delisted pins, pins marked NO-ZDR, unpinned same-lab ids, catalog ids from unpinned families, stale audit date). Reports only — it never edits a pin. | ## Adopt it in your repo diff --git a/.github/cursor-review/incremental-diff.py b/.github/cursor-review/incremental-diff.py new file mode 100755 index 00000000..61008f0e --- /dev/null +++ b/.github/cursor-review/incremental-diff.py @@ -0,0 +1,224 @@ +#!/usr/bin/env python3 +"""Build the "hunks new since the last reviewed round" block (BE-15558). + +The panel prompt calls this block "the subset of the diff above". It used to be +built as `git diff LAST_REVIEWED_SHA...HEAD_SHA`, which is not a subset of +anything: when HEAD is a merge commit that pulled the base branch into the +branch, LAST is an ancestor of HEAD, the merge base of the two IS LAST, and the +range therefore contains every commit that merge brought in. Measured on one +consumer repo, round 2 of a PR built a 9,800-line block against a 1,234-line +reviewed diff with 119 of its 133 files outside the PR, and two PRs built +~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`). + +`build` emits every NEW file section whose hunk content differs from the same +file's section in OLD, plus every NEW section for a file OLD does not have. A +file OLD had and NEW does not contributes nothing: it is no longer in the PR, so +there is nothing to prioritize. Because every emitted section is copied verbatim +out of NEW, the output is a subset of NEW by construction. + +Hunk content is compared with the `@@ -a,b +c,d @@` headers normalised to +`@@ @@`, so a pure rebase — identical hunks at shifted line numbers — produces +an empty block rather than re-reviewing the whole PR. `index ..` +lines are excluded from the comparison for the same reason: they change whenever +the base blob moves, even when the branch's own edit did not. + +`check` is the fail-safe the workflow runs afterwards, against the full reviewed +diff, so the subset property is *verified* and not merely intended — the block +is discarded whole (and the panel runs on the full diff alone, which is always +correct) if it names a file the reviewed diff does not carry or is longer than +the reviewed diff. + +Subcommands: + + build --old --new --out + check --new --full # exit 0 subset, 2 not a subset + +Run: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' +""" + +import argparse +import sys + +_DIFF_HEADER = "diff --git " + + +def parse_paths(header: str): + """Return the (old, new) paths named by a `diff --git a/X b/Y` line. + + git does not escape the separator, so `a/my file b/my file` is ambiguous on + its face. Resolve it the way git's own readers do: every candidate ` b/` + split is tried and the one whose halves are consistent wins, preferring the + common `a/X b/X` case. A header that cannot be parsed yields `()` — the + caller treats that as "unknown path", which the fail-safe counts as foreign + rather than waving through. + """ + rest = header[len(_DIFF_HEADER):].rstrip("\n") + if not rest.startswith("a/"): + return () + candidates = [] + idx = rest.find(" b/") + while idx != -1: + old = rest[2:idx] + new = rest[idx + 3:] + if old and new: + candidates.append((old, new)) + idx = rest.find(" b/", idx + 1) + if not candidates: + return () + for old, new in candidates: + if old == new: + return (old, new) + # A rename: no split is self-confirming, so take the first, which is what + # git produces for the unambiguous case. + return candidates[0] + + +def split_sections(text: str): + """Split a unified diff into `(header_line, [lines...])` file sections. + + Anything before the first `diff --git ` line (git emits none, but a caller's + file could) is dropped rather than silently attached to the first section. + """ + sections = [] + current = None + for line in text.splitlines(keepends=True): + if line.startswith(_DIFF_HEADER): + current = (line, [line]) + sections.append(current) + elif current is not None: + current[1].append(line) + return sections + + +def hunk_signature(lines): + """The comparable content of one file section. + + From the first `@@` onward, with each hunk header collapsed to `@@ @@` so a + pure line shift (and a changed trailing function context) does not read as a + change. A section with no `@@` at all — a binary file, a mode-only change, a + rename with no edit — falls back to every line after the header except + `index`, which is base-blob dependent and would otherwise make every rebase + look like a change. + """ + signature = [] + seen_hunk = False + for line in lines[1:]: + if line.startswith("@@"): + seen_hunk = True + signature.append("@@ @@") + elif seen_hunk: + signature.append(line.rstrip("\n")) + if seen_hunk: + return tuple(signature) + return tuple( + line.rstrip("\n") + for line in lines[1:] + if not line.startswith("index ") + ) + + +def build(old_text: str, new_text: str) -> str: + """Return the sections of NEW that OLD lacks or that changed since OLD.""" + old_by_path = {} + for header, lines in split_sections(old_text): + paths = parse_paths(header) + # An unparseable header is keyed by its raw line: it can still match the + # identical header on the NEW side, and it can never collide with a path. + key = paths[1] if paths else header + old_by_path[key] = hunk_signature(lines) + out = [] + for header, lines in split_sections(new_text): + paths = parse_paths(header) + key = paths[1] if paths else header + if key in old_by_path and old_by_path[key] == hunk_signature(lines): + continue + out.extend(lines) + return "".join(out) + + +def _paths_in(text: str): + """Every path a patch's `diff --git` headers name, old and new sides.""" + found = set() + for header, _lines in split_sections(text): + paths = parse_paths(header) + if paths: + found.update(paths) + else: + found.add(header.rstrip("\n")) + return found + + +def check(new_text: str, full_text: str): + """Return `(foreign_count, new_lines, full_lines)` for the fail-safe. + + `foreign_count` counts the file sections in the incremental block naming a + path the full reviewed diff does not carry. A block is a subset when that is + zero AND it is no longer than the reviewed diff. + """ + full_paths = _paths_in(full_text) + foreign = 0 + for header, _lines in split_sections(new_text): + paths = parse_paths(header) + names = set(paths) if paths else {header.rstrip("\n")} + # Foreign only when NOTHING it names is in the reviewed diff: a rename + # the classifier rewrote should not read as foreign on one side alone. + if not (names & full_paths): + foreign += 1 + return foreign, _count_lines(new_text), _count_lines(full_text) + + +def _count_lines(text: str) -> int: + """Line count matching `wc -l`, so the workflow's numbers agree with the log.""" + return text.count("\n") + + +def _read(path: str) -> str: + with open(path, "r", encoding="utf-8", errors="surrogateescape", newline="") as fh: + return fh.read() + + +def cmd_build(args) -> int: + old_text = _read(args.old) + new_text = _read(args.new) + with open(args.out, "w", encoding="utf-8", errors="surrogateescape", newline="") as fh: + fh.write(build(old_text, new_text)) + return 0 + + +def cmd_check(args) -> int: + foreign, new_lines, full_lines = check(_read(args.new), _read(args.full)) + print(f"foreign={foreign}") + print(f"new_lines={new_lines}") + print(f"full_lines={full_lines}") + return 0 if foreign == 0 and new_lines <= full_lines else 2 + + +def main(argv=None) -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + sub = parser.add_subparsers(dest="cmd", required=True) + + build_p = sub.add_parser("build", help="emit the sections of NEW that changed since OLD") + build_p.add_argument("--old", required=True, help="the patch the last round reviewed (BASE...LAST)") + build_p.add_argument("--new", required=True, help="the patch this round reviews (the reviewed diff)") + build_p.add_argument("--out", required=True, help="where to write the incremental block") + build_p.set_defaults(func=cmd_build) + + check_p = sub.add_parser("check", help="verify the block is a subset of the reviewed diff") + check_p.add_argument("--new", required=True, help="the incremental block to verify") + check_p.add_argument("--full", required=True, help="the reviewed diff it must be a subset of") + check_p.set_defaults(func=cmd_check) + + args = parser.parse_args(argv) + return args.func(args) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/cursor-review/tests/test_incremental_diff.py b/.github/cursor-review/tests/test_incremental_diff.py new file mode 100644 index 00000000..b9044d42 --- /dev/null +++ b/.github/cursor-review/tests/test_incremental_diff.py @@ -0,0 +1,452 @@ +#!/usr/bin/env python3 +"""Regression tests for the incremental "hunks new since last round" block (BE-15558). + +The prompt tells the panel this block "is the subset of the diff above". Built +as `git diff LAST_REVIEWED...HEAD` it was not: with a merge commit at HEAD, LAST +is an ancestor of HEAD, the merge base of the two IS LAST, and the range carries +every commit the merge pulled in from the base branch. The property pinned here +is the one that was missing — **the block only ever contains hunks the PR itself +carries** — plus the behaviours that must survive the rewrite: a non-merge round +still shows only what changed since the last round, a pure rebase shows nothing, +and a file dropped from the PR contributes nothing. + +`TestMergeCommitHead` is the real repro, run against an actual git repository +with a real merge commit, so it fails against the old commit-range formulation +rather than only against a hand-written fixture. + +Run: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' +""" + +import contextlib +import importlib.util +import io +import os +import shutil +import subprocess +import tempfile +import unittest + +_HERE = os.path.dirname(os.path.abspath(__file__)) +_ASSETS = os.path.join(_HERE, "..") + + +def _load(name, filename): + spec = importlib.util.spec_from_file_location(name, os.path.join(_ASSETS, filename)) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +inc = _load("incremental_diff", "incremental-diff.py") + + +def _patch(path, hunk_header, body, index="1111111..2222222 100644"): + """One file section of a unified diff, in git's own shape.""" + return ( + f"diff --git a/{path} b/{path}\n" + f"index {index}\n" + f"--- a/{path}\n" + f"+++ b/{path}\n" + f"{hunk_header}\n" + f"{body}" + ) + + +# --------------------------------------------------------------------------- # +# 1. The headline case: a merge commit at HEAD # +# --------------------------------------------------------------------------- # + + +def _git(repo, *args): + return subprocess.run( + ["git", "-C", repo, *args], + check=True, capture_output=True, text=True, + ).stdout + + +def _rev(repo, ref): + return _git(repo, "rev-parse", ref).strip() + + +def _write(repo, name, text): + with open(os.path.join(repo, name), "w", encoding="utf-8") as fh: + fh.write(text) + + +class TestMergeCommitHead(unittest.TestCase): + """A real repo, a real merge of the base branch into the PR branch.""" + + @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-repo-") + 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, "unrelated.py", "base\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "base") + cls.fork_point = _rev(repo, "HEAD") + + # The PR branch: round 1 touches app.py only. + _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") + + # Meanwhile main moves — a file the PR never touches. + _git(repo, "checkout", "-q", "main") + _write(repo, "unrelated.py", "base\nmain moved on\nand on\nand on\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "main advances") + cls.base_sha = _rev(repo, "HEAD") + + # Round 2: the author merges main into the branch and edits app.py. + _git(repo, "checkout", "-q", "pr") + _git(repo, "merge", "-q", "--no-ff", "-m", "merge main", "main") + _write(repo, "app.py", "one\nTWO\nTHREE\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "pr round 2") + cls.head_sha = _rev(repo, "HEAD") + + @classmethod + def tearDownClass(cls): + shutil.rmtree(cls.repo, ignore_errors=True) + + def _diff(self, a, b): + return _git(self.repo, "-c", "core.quotePath=false", "diff", f"{a}...{b}", "--", ".") + + def test_old_formulation_pulled_in_base_branch_files(self): + """The bug, pinned: the commit range carries main's own commits.""" + stale = self._diff(self.last_reviewed, self.head_sha) + self.assertIn("unrelated.py", stale) + + def test_block_contains_only_pr_files(self): + old = self._diff(self.base_sha, self.last_reviewed) + new = self._diff(self.base_sha, self.head_sha) + block = inc.build(old, new) + self.assertIn("app.py", block) + self.assertNotIn("unrelated.py", block) + self.assertIn("+THREE", block) + + def test_block_passes_the_subset_fail_safe(self): + old = self._diff(self.base_sha, self.last_reviewed) + new = self._diff(self.base_sha, self.head_sha) + block = inc.build(old, new) + foreign, new_lines, full_lines = inc.check(block, new) + self.assertEqual(foreign, 0) + self.assertLessEqual(new_lines, full_lines) + + def test_old_formulation_fails_the_subset_fail_safe(self): + """The fail-safe would have caught the shipped bug on its own.""" + stale = self._diff(self.last_reviewed, self.head_sha) + reviewed = self._diff(self.base_sha, self.head_sha) + foreign, _new_lines, _full_lines = inc.check(stale, reviewed) + self.assertGreater(foreign, 0) + + def test_unchanged_file_is_not_re_emitted_after_a_merge(self): + """A merge that brings in no PR-file change yields an empty block.""" + merge_only = _rev(self.repo, f"{self.head_sha}^") + old = self._diff(self.base_sha, self.last_reviewed) + new = self._diff(self.base_sha, merge_only) + self.assertEqual(inc.build(old, new), "") + + +# --------------------------------------------------------------------------- # +# 2. The behaviours the rewrite must preserve # +# --------------------------------------------------------------------------- # + + +class TestBuild(unittest.TestCase): + def test_non_merge_head_shows_only_what_changed_since_last_round(self): + touched = _patch("a.py", "@@ -1,2 +1,3 @@", " one\n+two\n+three\n") + untouched = _patch("b.py", "@@ -1 +1,2 @@", " x\n+y\n") + old = _patch("a.py", "@@ -1,2 +1,2 @@", " one\n+two\n") + untouched + new = touched + untouched + block = inc.build(old, new) + self.assertIn("a/a.py", block) + self.assertNotIn("a/b.py", block) + + def test_file_dropped_since_last_round_contributes_nothing(self): + gone = _patch("gone.py", "@@ -1 +1 @@", "-x\n+y\n") + kept = _patch("kept.py", "@@ -1 +1 @@", "-p\n+q\n") + self.assertEqual(inc.build(gone + kept, kept), "") + + def test_pure_rebase_yields_an_empty_block(self): + """Identical hunks at shifted line numbers, over a moved base blob.""" + old = _patch("a.py", "@@ -10,3 +10,4 @@ def f():", " x\n+new\n y\n", index="aaaaaaa..bbbbbbb 100644") + new = _patch("a.py", "@@ -84,3 +85,4 @@ def f():", " x\n+new\n y\n", index="ccccccc..ddddddd 100644") + self.assertEqual(inc.build(old, new), "") + + def test_a_real_edit_at_a_shifted_line_still_shows(self): + old = _patch("a.py", "@@ -10,3 +10,4 @@", " x\n+new\n y\n") + new = _patch("a.py", "@@ -84,3 +85,4 @@", " x\n+newer\n y\n") + self.assertIn("+newer", inc.build(old, new)) + + def test_a_file_new_this_round_is_emitted_whole(self): + old = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + added = ( + "diff --git a/n.py b/n.py\n" + "new file mode 100644\n" + "index 0000000..3333333\n" + "--- /dev/null\n" + "+++ b/n.py\n" + "@@ -0,0 +1,2 @@\n" + "+hello\n" + "+world\n" + ) + block = inc.build(old, old + added) + self.assertEqual(block, added) + + def test_emitted_sections_are_byte_for_byte_slices_of_new(self): + new = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + self.assertIn(inc.build("", new), new) + + def test_empty_new_yields_an_empty_block(self): + self.assertEqual(inc.build(_patch("a.py", "@@ -1 +1 @@", "-x\n+y\n"), ""), "") + + def test_a_binary_file_changed_since_last_round_is_emitted(self): + old = ( + "diff --git a/i.png b/i.png\n" + "index 1111111..2222222 100644\n" + "Binary files a/i.png and b/i.png differ\n" + ) + new = ( + "diff --git a/i.png b/i.png\n" + "index 1111111..4444444 100644\n" + "GIT binary patch\n" + "literal 4\n" + "Lc$@\n" + ) + self.assertIn("GIT binary patch", inc.build(old, new)) + + def test_a_mode_only_change_new_this_round_is_emitted(self): + new = ( + "diff --git a/s.sh b/s.sh\n" + "old mode 100644\n" + "new mode 100755\n" + ) + self.assertEqual(inc.build("", new), new) + + +# --------------------------------------------------------------------------- # +# 3. Header parsing — the identity the whole comparison keys on # +# --------------------------------------------------------------------------- # + + +class TestParsePaths(unittest.TestCase): + def test_plain_path(self): + self.assertEqual(inc.parse_paths("diff --git a/x.py b/x.py\n"), ("x.py", "x.py")) + + def test_path_with_spaces(self): + self.assertEqual( + inc.parse_paths("diff --git a/my file.txt b/my file.txt\n"), + ("my file.txt", "my file.txt"), + ) + + def test_path_containing_the_separator(self): + self.assertEqual( + inc.parse_paths("diff --git a/x b/y.txt b/x b/y.txt\n"), + ("x b/y.txt", "x b/y.txt"), + ) + + def test_rename(self): + self.assertEqual(inc.parse_paths("diff --git a/o.py b/n.py\n"), ("o.py", "n.py")) + + def test_unparseable_header_yields_nothing(self): + self.assertEqual(inc.parse_paths("diff --git nonsense\n"), ()) + + def test_a_section_with_an_unparseable_header_is_still_compared(self): + weird = "diff --git nonsense\n@@ -1 +1 @@\n-x\n+y\n" + self.assertEqual(inc.build(weird, weird), "") + + +# --------------------------------------------------------------------------- # +# 4. The fail-safe # +# --------------------------------------------------------------------------- # + + +class TestCheck(unittest.TestCase): + def test_a_subset_passes(self): + a = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + b = _patch("b.py", "@@ -1 +1 @@", "-p\n+q\n") + self.assertEqual(inc.check(a, a + b), (0, a.count("\n"), (a + b).count("\n"))) + + def test_a_foreign_file_is_counted(self): + a = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + foreign = _patch("elsewhere.py", "@@ -1 +1 @@", "-p\n+q\n") + count, _n, _f = inc.check(a + foreign, a) + self.assertEqual(count, 1) + + def test_an_empty_block_is_a_subset(self): + self.assertEqual(inc.check("", _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n"))[0], 0) + + def test_a_rename_matching_on_one_side_is_not_foreign(self): + """The classifier's patch may name the file under only one of its paths.""" + block = "diff --git a/o.py b/n.py\n@@ -1 +1 @@\n-x\n+y\n" + full = _patch("n.py", "@@ -1 +1 @@", "-x\n+y\n") + self.assertEqual(inc.check(block, full)[0], 0) + + def test_line_counts_match_wc_l(self): + text = "diff --git a/a.py b/a.py\n@@ -1 +1 @@\n-x\n+y\n" + self.assertEqual(inc.check(text, text)[1], 4) + + +# --------------------------------------------------------------------------- # +# 5. The CLI the workflow step actually calls # +# --------------------------------------------------------------------------- # + + +class TestCli(unittest.TestCase): + def _main(self, argv): + """Run the CLI with its key=value report captured, not dumped into the suite.""" + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + rc = inc.main(argv) + return rc, buf.getvalue() + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix="inc-diff-cli-") + self.addCleanup(shutil.rmtree, self.tmp, True) + + def _file(self, name, text): + path = os.path.join(self.tmp, name) + with open(path, "w", encoding="utf-8") as fh: + fh.write(text) + return path + + def test_build_writes_the_block(self): + old = self._file("old.patch", _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n")) + new_text = _patch("a.py", "@@ -1 +1 @@", "-x\n+z\n") + new = self._file("new.patch", new_text) + out = os.path.join(self.tmp, "out.patch") + self.assertEqual(inc.main(["build", "--old", old, "--new", new, "--out", out]), 0) + with open(out, encoding="utf-8") as fh: + self.assertEqual(fh.read(), new_text) + + def test_build_truncates_a_pre_existing_out_file(self): + """A second round must never append onto the previous round's block.""" + same = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + old = self._file("old.patch", same) + new = self._file("new.patch", same) + out = self._file("out.patch", "STALE\n") + inc.main(["build", "--old", old, "--new", new, "--out", out]) + with open(out, encoding="utf-8") as fh: + self.assertEqual(fh.read(), "") + + def test_check_exit_codes_and_output(self): + a = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + full = self._file("full.patch", a) + ok = self._file("ok.patch", a) + rc, out = self._main(["check", "--new", ok, "--full", full]) + self.assertEqual(rc, 0) + self.assertIn("foreign=0", out) + bad = self._file("bad.patch", a + _patch("z.py", "@@ -1 +1 @@", "-p\n+q\n")) + rc, out = self._main(["check", "--new", bad, "--full", full]) + self.assertEqual(rc, 2) + # The three numbers the workflow's ::warning:: interpolates. + self.assertIn("foreign=1", out) + self.assertRegex(out, r"new_lines=\d+") + self.assertRegex(out, r"full_lines=\d+") + + def test_check_rejects_a_block_longer_than_the_reviewed_diff(self): + """Same paths, more lines — the second half of the subset property.""" + full = self._file("full.patch", _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n")) + long_block = _patch("a.py", "@@ -1,9 +1,9 @@", "".join(f"+l{i}\n" for i in range(20))) + bad = self._file("bad.patch", long_block) + rc, out = self._main(["check", "--new", bad, "--full", full]) + self.assertEqual(rc, 2) + self.assertIn("foreign=0", out) + + def test_non_utf8_bytes_survive_a_round_trip(self): + """PR bytes are attacker-authored; the helper must not die on them.""" + raw = b"diff --git a/b.bin b/b.bin\n@@ -1 +1 @@\n-\xff\xfe\n+\xfe\xff\n" + old = os.path.join(self.tmp, "old.patch") + new = os.path.join(self.tmp, "new.patch") + out = os.path.join(self.tmp, "out.patch") + with open(old, "wb") as fh: + fh.write(b"") + with open(new, "wb") as fh: + fh.write(raw) + self.assertEqual(inc.main(["build", "--old", old, "--new", new, "--out", out]), 0) + with open(out, "rb") as fh: + self.assertEqual(fh.read(), raw) + + +# --------------------------------------------------------------------------- # +# 6. The workflow wiring — the helper is worthless if the step drifts off it # +# --------------------------------------------------------------------------- # + + +_WORKFLOW = os.path.join(_HERE, "..", "..", "workflows", "cursor-review.yml") + + +class TestWorkflowWiring(unittest.TestCase): + @classmethod + def setUpClass(cls): + with open(_WORKFLOW, encoding="utf-8") as fh: + cls.text = fh.read() + + def _assert_has(self, needle): + # assertIn would dump the whole 2,500-line workflow into the failure. + self.assertTrue(needle in self.text, f"cursor-review.yml no longer contains {needle!r}") + + def test_the_step_calls_the_helper(self): + self._assert_has("incremental-diff.py") + + def test_the_commit_range_formulation_is_gone(self): + """`git diff LAST...HEAD` is the bug — it must not come back.""" + needle = 'git diff "${LAST_REVIEWED_SHA}...${HEAD_SHA}"' + self.assertFalse(needle in self.text, f"cursor-review.yml is back on {needle!r}") + + def test_the_sweepable_log_line_survives(self): + self._assert_has("Incremental diff since round") + + def test_the_fail_safe_warning_is_present(self): + self._assert_has("::warning::Incremental diff discarded") + + def test_incremental_subset_is_a_job_output(self): + self._assert_has("incremental_subset:") + + def test_the_helper_is_loaded_from_the_pinned_checkout_not_the_pr(self): + """The path the step calls must be one a pinned checkout actually writes. + + The helper decides what the panel is shown, so it has to come from THIS + repo at `workflows_ref` — never the PR's own tree, which the PR under + review can rewrite. That only holds while the hardcoded path in the step + matches the `path:` some checkout of `Comfy-Org/github-workflows` + declares, and nothing else in the workflow ties the two together. + + Parsed without PyYAML, for the reason the sibling workflow suites give: + this repo is stdlib-only and this job's CI installs no requirements. + """ + checkout_paths = set() + repo_seen = False + for raw in self.text.splitlines(): + line = raw.strip() + if line.startswith("- name:") or line.startswith("- uses:"): + repo_seen = False + if line == "repository: Comfy-Org/github-workflows": + repo_seen = True + elif repo_seen and line.startswith("path:"): + checkout_paths.add(line.split(":", 1)[1].strip()) + self.assertTrue(checkout_paths, "no pinned checkout of this repo found in the workflow") + + called = [ + line for line in self.text.splitlines() + if "incremental-diff.py" in line and "=" in line and "#" not in line + ] + self.assertTrue(called, "the step no longer resolves incremental-diff.py by path") + self.assertTrue( + any(f"{prefix}/" in called[0] for prefix in checkout_paths), + f"the step loads the helper from {called[0].strip()!r}, which is not " + f"under any pinned checkout of this repo {sorted(checkout_paths)}", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 56af0f19..3bf731a7 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -754,6 +754,16 @@ jobs: # so calling it "after generated-file exclusion" would send an author # hunting for a lockfile that was, in fact, counted. degraded: ${{ steps.check.outputs.degraded }} + # Whether the incremental "hunks new since last round" block really was + # the subset of the reviewed diff the prompt calls it (BE-15558). + # `true` both when the block passed the fail-safe and when there is no + # block at all (round 1, an unresolvable last-reviewed SHA, an over-cap + # run where this step never ran) — in none of those cases was anything + # dropped. `false` ONLY when the fail-safe fired and discarded a block + # that had been built, which is the one case a panel-integrity signal + # 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' }} steps: - name: Checkout PR head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -954,21 +964,99 @@ jobs: # ride the same shared artifact instead of every consumer re-deriving it. # Best-effort: an unresolvable SHA (force-push, base-branch rewrite) # simply produces no incremental block. + # + # It is built by DIFFING TWO PR PATCHES, never a commit range (BE-15558). + # `git diff LAST_REVIEWED...HEAD` looks like the obvious formulation and + # is wrong: when HEAD is a merge commit that pulled the base branch into + # the branch, LAST is an ancestor of HEAD, the merge base of the two IS + # LAST, and the range therefore contains every commit that merge brought + # in. Measured on a consumer repo, one round built a 9,800-line block + # 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. + # + # NEW is `pr-diff.patch` ITSELF — the diff this round is actually + # reviewing — not a freshly-computed BASE...HEAD diff. Every emitted + # section is then a verbatim slice of the reviewed diff, so the subset + # property the prompt asserts holds by construction, and a generated + # 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. + 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 }} LEDGER_ROUNDS: ${{ needs.ledger.outputs.rounds }} run: | - : > "${RUNNER_TEMP}/pr-diff-new.patch" - if [ -n "$LAST_REVIEWED_SHA" ] && [ "$LAST_REVIEWED_SHA" != "$HEAD_SHA" ] \ - && git cat-file -e "${LAST_REVIEWED_SHA}^{commit}" 2>/dev/null; then + NEW_PATCH="${RUNNER_TEMP}/pr-diff-new.patch" + OLD_PATCH="${RUNNER_TEMP}/pr-diff-old.patch" + FULL_PATCH="${RUNNER_TEMP}/pr-diff.patch" + # The helper comes from the pinned checkout of THIS repo (same one the + # classifier was built from), never the PR checkout — a PR must not be + # able to rewrite the logic that decides what the panel is shown. + INCREMENTAL_PY="_pr_size_tool/.github/cursor-review/incremental-diff.py" + : > "$NEW_PATCH" + # Wrapped in a function so the shellcheck directive sits in front of a + # complete command — it is rejected in front of an `elif` branch. + build_old_patch() { + # $DIFF_EXCLUDES is intentionally unquoted so bash word-splits it + # into pathspec args. # shellcheck disable=SC2086 - git diff "${LAST_REVIEWED_SHA}...${HEAD_SHA}" -- . $DIFF_EXCLUDES \ - > "${RUNNER_TEMP}/pr-diff-new.patch" || : > "${RUNNER_TEMP}/pr-diff-new.patch" - echo "Incremental diff since round ${LEDGER_ROUNDS:-?} (${LAST_REVIEWED_SHA}): $(wc -l < "${RUNNER_TEMP}/pr-diff-new.patch") lines" - else + git diff "${BASE_SHA}...${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 + # append the raw-count fallback above uses. + echo "incremental_subset=true" >> "$GITHUB_OUTPUT" + # Every "can't build one" case below leaves the block EMPTY and + # `incremental_subset` true: nothing was discarded, there was simply + # nothing to prioritize, and the panel reviews the full diff as it + # does on round 1. Only the fail-safe at the bottom writes `false`. + if [ -z "$LAST_REVIEWED_SHA" ] || [ "$LAST_REVIEWED_SHA" = "$HEAD_SHA" ]; then 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 [ ! -s "$FULL_PATCH" ]; then + echo "The reviewed diff is empty — skipping the incremental diff block." + elif ! build_old_patch; then + # Fail CLOSED to "no block". Falling through with an empty OLD would + # make every file read as new and emit the whole reviewed diff a + # second time — a doubled prompt that prioritizes nothing. + echo "Could not build the last-reviewed patch (${LAST_REVIEWED_SHA}) — skipping the incremental diff block." + elif ! python3 "$INCREMENTAL_PY" build \ + --old "$OLD_PATCH" --new "$FULL_PATCH" --out "$NEW_PATCH"; then + echo "::warning::Could not build the incremental diff block — the panel will run on the full reviewed diff alone." + : > "$NEW_PATCH" + else + echo "Incremental diff since round ${LEDGER_ROUNDS:-?} (${LAST_REVIEWED_SHA}): $(wc -l < "$NEW_PATCH") lines" + # Fail-safe. The prompt tells the panel this block "is the subset of + # the diff above", so VERIFY it rather than trusting the builder: + # any path the reviewed diff does not carry, or more lines than the + # reviewed diff, and the block is discarded whole. The panel then + # runs on the full diff alone, which is always correct. + if SUBSET_OUT="$(python3 "$INCREMENTAL_PY" check --new "$NEW_PATCH" --full "$FULL_PATCH")"; then + echo "Incremental block verified as a subset of the reviewed diff." + else + FOREIGN="$(printf '%s\n' "$SUBSET_OUT" | sed -n 's/^foreign=//p')" + NEW_LINES="$(printf '%s\n' "$SUBSET_OUT" | sed -n 's/^new_lines=//p')" + FULL_LINES="$(printf '%s\n' "$SUBSET_OUT" | sed -n 's/^full_lines=//p')" + : > "$NEW_PATCH" + echo "::warning::Incremental diff discarded: it was not a subset of the reviewed diff (${FOREIGN:-unknown} foreign file(s), ${NEW_LINES:-unknown} vs ${FULL_LINES:-unknown} lines)." + echo "incremental_subset=false" >> "$GITHUB_OUTPUT" + fi fi - name: Upload reviewed diff diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 7033bf58..73d4a564 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -144,6 +144,10 @@ 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 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 the block named files the reviewed diff does not carry (or came out longer than it) and 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. + **Dependabot PRs are not covered by the fork skip.** Dependabot's branches live in the base repo, so the gate's cross-repo check treats them as ordinary PRs — but Dependabot-triggered runs read the *Dependabot* secret store, not Actions secrets. From a2da829779f99b2fdb0c67862a2737ad173004ac Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 20:50:40 +0000 Subject: [PATCH 2/5] fix(cursor-review): close seven gaps the panel found in the incremental block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the BE-15558 rewrite. Each one is a case where the block either dropped a hunk it should have shown, showed the whole diff twice, or passed a fail-safe that was weaker than the property the docs assert. Builder (`incremental-diff.py`): * Split on LF only. `str.splitlines` also breaks on a lone CR, `\v`, `\f`, `\x1c`-`\x1e`, `\x85`, U+2028 and U+2029, none of which git treats as a line break — so a content line `+x\x0cdiff --git a/lib/auth.py b/lib/auth.py` forged a file-section boundary, putting a real file's remaining hunks under a header the PR chose, and taught `check` the forged path from the same bad split so the section did not count as foreign. * Fold the pre-hunk metadata into the signature even when the file has hunks. Starting at the first `@@` hid `old mode`/`new mode`, `rename from`/`to` and typechange lines whenever the file was also edited — so a round that only added the executable bit to an already-edited script compared equal and was dropped. Only the mode-ONLY case used to survive. * Keep the `index` line for a binary section. Without `--binary`, `Binary files a/X and b/X differ` is a CONSTANT, so excluding `index` (right for a text file, whose blob id moves on every rebase) left a changed binary with an identical signature on both sides. The test that covered this pitted a `Binary files ... differ` stanza against a `GIT binary patch` one — two formats a single `git diff` invocation never mixes — so it passed while the gap was open. * Key a rename off its own `rename from`/`rename to` lines. One path per line, so they settle the ambiguity the header cannot: `a/x b/c b/d` splits two ways and `parse_paths` took the wrong one, keying the section under a path no later round's plain header could match. * `check` now verifies BYTES, not path names. The README and the caller guide say the block "can never contain a hunk the PR does not carry"; counting only foreign paths waved through fabricated, reordered or duplicated hunks riding under a path the PR does touch. Every section must now appear byte for byte in the reviewed diff, matched as a multiset. The length arm is kept as a redundant second gate. Workflow step: * Build OLD with `-c core.quotePath=false`, matching check-pr-size's `writeReviewedDiff`, which builds NEW. Under the default a non-ASCII path arrived C-quoted on one side and plain on the other, so the two could never key alike and every such file was re-emitted in full on every round — including a pure rebase, the exact over-inclusion this rewrite exists to stop. * Decline a successful-but-EMPTY OLD, not just a failed build. `BASE...LAST` is empty whenever merge-base(BASE, LAST) is LAST itself (the base advanced past the last-reviewed commit, e.g. a retarget) or `$DIFF_EXCLUDES` filters everything out; git reports no error, so the old guard fell through and every file read as new — emitting the whole reviewed diff a second time, the doubled prompt the guard's own comment says it prevents. The fail-safe could not catch it either: nothing foreign, and new_lines EQUALS full_lines. * Bound OLD at 32 MiB. NEW is bounded by `diff_size_cap`; OLD is bounded by nothing, since it keeps the generated-file sections the classifier strips out of the reviewed diff and those cost nothing against that cap. Docs follow the check: both places that described the fail-safe as a path test now describe it as the byte test it is. 657 tests pass (57 in this file, 21 new). workflow-pins 499, groom 405, public-repo-hygiene 204, linear-ticket 116, refresh-reviewers 56, agents-md-integrity 46 — all OK. `check_workflow_pins.py` OK (11 workflows, 155 pinned refs); `check_agents_md.py` passed with its 2 pre-existing warnings; shellcheck clean on the cursor-review scripts and on the step's extracted `run:` block, whose four guard branches were exercised directly. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/README.md | 2 +- .github/cursor-review/incremental-diff.py | 174 ++++++++---- .../tests/test_incremental_diff.py | 261 +++++++++++++++++- .github/workflows/cursor-review.yml | 34 ++- docs/callers/cursor-review.md | 2 +- 5 files changed, 394 insertions(+), 79 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 95ccb8eb..a4507acc 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -91,7 +91,7 @@ Those per-entry lines are the ledger's own structure, so imported prose must not 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. -The build is then **verified, not trusted**. If the block names a path the reviewed diff does not carry, or 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. +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. ### The panel diff --git a/.github/cursor-review/incremental-diff.py b/.github/cursor-review/incremental-diff.py index 61008f0e..2deb3de2 100755 --- a/.github/cursor-review/incremental-diff.py +++ b/.github/cursor-review/incremental-diff.py @@ -18,23 +18,27 @@ * OLD — `git diff BASE...LAST_REVIEWED` — what the last round saw. * NEW — the reviewed diff this round is running on (`pr-diff.patch`). -`build` emits every NEW file section whose hunk content differs from the same -file's section in OLD, plus every NEW section for a file OLD does not have. A -file OLD had and NEW does not contributes nothing: it is no longer in the PR, so -there is nothing to prioritize. Because every emitted section is copied verbatim -out of NEW, the output is a subset of NEW by construction. - -Hunk content is compared with the `@@ -a,b +c,d @@` headers normalised to -`@@ @@`, so a pure rebase — identical hunks at shifted line numbers — produces -an empty block rather than re-reviewing the whole PR. `index ..` -lines are excluded from the comparison for the same reason: they change whenever -the base blob moves, even when the branch's own edit did not. +`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 +had and NEW does not contributes nothing: it is no longer in the PR, so there is +nothing to prioritize. Because every emitted section is copied verbatim out of +NEW, the output is a subset of NEW by construction. + +Sections are compared with the `@@ -a,b +c,d @@` headers normalised to `@@ @@`, +so a pure rebase — identical hunks at shifted line numbers — produces an empty +block rather than re-reviewing the whole PR. Lines whose content tracks the BASE +blob rather than the branch's own edit (`index ..`, the similarity +percentages) are excluded for the same reason: they move whenever the base blob +moves, even when the branch's edit did not. `check` is the fail-safe the workflow runs afterwards, against the full reviewed -diff, so the subset property is *verified* and not merely intended — the block -is discarded whole (and the panel runs on the full diff alone, which is always -correct) if it names a file the reviewed diff does not carry or is longer than -the reviewed diff. +diff, so the subset property is *verified* and not merely intended — every +section of the block must appear BYTE FOR BYTE in the reviewed diff, and the +block is discarded whole if any does not (or if it is longer than the reviewed +diff). Comparing bytes rather than path names is what makes the README's "can +never contain a hunk the PR does not carry" a checked property: a path-only +check waves through fabricated, reordered or duplicated hunks as long as they +are carried under some path the PR does touch. Subcommands: @@ -46,9 +50,36 @@ import argparse import sys +from collections import Counter _DIFF_HEADER = "diff --git " +# Lines whose content tracks the BASE blob rather than the branch's own edit. +# They move on every rebase even when the author changed nothing, so they are +# excluded from the comparison. A binary section is the one exception — see +# `hunk_signature`. +_BASE_VOLATILE = ("index ", "similarity index ", "dissimilarity index ") + + +def _lf_lines(text: str): + r"""Split on LF only, keeping the terminator. + + `str.splitlines` is wrong here: it also breaks on a lone `\r`, `\v`, `\f`, + `\x1c`-`\x1e`, `\x85`, U+2028 and U+2029, none of which git treats as a line + break. A patch is attacker-authored PR bytes, so a content line + `+x\x0cdiff --git a/lib/auth.py b/lib/auth.py` would otherwise forge a file + section boundary, putting a real file's remaining hunks under a header of + the PR's choosing — and `check` would learn the forged path from the same + bad split and not count the section foreign. + """ + if not text: + return [] + parts = text.split("\n") + lines = [part + "\n" for part in parts[:-1]] + if parts[-1]: + lines.append(parts[-1]) + return lines + def parse_paths(header: str): """Return the (old, new) paths named by a `diff --git a/X b/Y` line. @@ -57,8 +88,11 @@ def parse_paths(header: str): its face. Resolve it the way git's own readers do: every candidate ` b/` split is tried and the one whose halves are consistent wins, preferring the common `a/X b/X` case. A header that cannot be parsed yields `()` — the - caller treats that as "unknown path", which the fail-safe counts as foreign - rather than waving through. + caller treats that as "unknown path" and keys the section by its raw header. + + A rename has no self-confirming split, so this can still guess wrong on a + header like `a/x b/c b/d`; `section_paths` is what resolves those, from the + section's own one-path-per-line `rename from`/`rename to` lines. """ rest = header[len(_DIFF_HEADER):].rstrip("\n") if not rest.startswith("a/"): @@ -81,6 +115,30 @@ def parse_paths(header: str): return candidates[0] +def section_paths(header: str, lines): + """The (old, new) paths one file section names. + + Prefers the section's own `rename from` / `rename to` lines over the + `diff --git` header. They carry ONE path per line, so they are unambiguous + exactly where the header is not: `a/x b/c b/d` has two readings and the + header alone cannot tell them apart, which used to key the section under a + path (`x` -> `c b/d`) that no later round's plain header could match, + re-emitting the whole file every time. Falls back to the header when the + section carries no rename pair, which is every non-rename section. + """ + old = new = None + for line in lines[1:]: + if line.startswith("@@"): + break + if line.startswith("rename from "): + old = line[len("rename from "):].rstrip("\n") + elif line.startswith("rename to "): + new = line[len("rename to "):].rstrip("\n") + if old and new: + return (old, new) + return parse_paths(header) + + def split_sections(text: str): """Split a unified diff into `(header_line, [lines...])` file sections. @@ -89,7 +147,7 @@ def split_sections(text: str): """ sections = [] current = None - for line in text.splitlines(keepends=True): + for line in _lf_lines(text): if line.startswith(_DIFF_HEADER): current = (line, [line]) sections.append(current) @@ -99,44 +157,51 @@ def split_sections(text: str): def hunk_signature(lines): - """The comparable content of one file section. - - From the first `@@` onward, with each hunk header collapsed to `@@ @@` so a - pure line shift (and a changed trailing function context) does not read as a - change. A section with no `@@` at all — a binary file, a mode-only change, a - rename with no edit — falls back to every line after the header except - `index`, which is base-blob dependent and would otherwise make every rebase - look like a change. + """The comparable content of one file section. Two parts, both load-bearing: + + * The pre-hunk metadata — `old mode`/`new mode`, `rename from`/`rename to`, + `deleted file mode`, a typechange — minus the base-volatile lines. + Dropping it whenever the file also had hunks hid a round that only added + the executable bit to an already-edited script: small, high-signal, and + exactly the kind of change this block exists to surface. + * The hunks, from the first `@@` onward, each hunk header collapsed to + `@@ @@` so a pure line shift (and a changed trailing function context) + does not read as a change. + + A section with no `@@` at all — a binary file, a mode-only change, a rename + with no edit — is all metadata. For the `Binary files a/X and b/X differ` + form git emits without `--binary`, that line is a CONSTANT and + `index ..` is the section's only content-dependent line, so the + base-volatile exclusion is lifted for a binary section; otherwise a binary + whose bytes changed since the last round compares equal and is dropped. """ + body = lines[1:] + binary = any(line.startswith("Binary files ") for line in body) signature = [] seen_hunk = False - for line in lines[1:]: + for line in body: if line.startswith("@@"): seen_hunk = True signature.append("@@ @@") elif seen_hunk: signature.append(line.rstrip("\n")) - if seen_hunk: - return tuple(signature) - return tuple( - line.rstrip("\n") - for line in lines[1:] - if not line.startswith("index ") - ) + elif binary or not line.startswith(_BASE_VOLATILE): + signature.append(line.rstrip("\n")) + return tuple(signature) def build(old_text: str, new_text: str) -> str: """Return the sections of NEW that OLD lacks or that changed since OLD.""" old_by_path = {} for header, lines in split_sections(old_text): - paths = parse_paths(header) + paths = section_paths(header, lines) # An unparseable header is keyed by its raw line: it can still match the # identical header on the NEW side, and it can never collide with a path. key = paths[1] if paths else header old_by_path[key] = hunk_signature(lines) out = [] for header, lines in split_sections(new_text): - paths = parse_paths(header) + paths = section_paths(header, lines) key = paths[1] if paths else header if key in old_by_path and old_by_path[key] == hunk_signature(lines): continue @@ -144,33 +209,26 @@ def build(old_text: str, new_text: str) -> str: return "".join(out) -def _paths_in(text: str): - """Every path a patch's `diff --git` headers name, old and new sides.""" - found = set() - for header, _lines in split_sections(text): - paths = parse_paths(header) - if paths: - found.update(paths) - else: - found.add(header.rstrip("\n")) - return found - - def check(new_text: str, full_text: str): """Return `(foreign_count, new_lines, full_lines)` for the fail-safe. - `foreign_count` counts the file sections in the incremental block naming a - path the full reviewed diff does not carry. A block is a subset when that is - zero AND it is no longer than the reviewed diff. + `foreign_count` counts the file sections of the incremental block that do + not appear BYTE FOR BYTE in the full reviewed diff. That is the property the + README and the caller guide assert — "can never contain a hunk the PR does + not carry" — checked directly, rather than the weaker "every section names + SOME path the reviewed diff also names", which waves through fabricated, + reordered or duplicated hunks under a path the PR does happen to touch. + + Counted as a multiset, so a section the block emits twice is foreign on its + second appearance even though the reviewed diff carries it once. """ - full_paths = _paths_in(full_text) + remaining = Counter("".join(lines) for _header, lines in split_sections(full_text)) foreign = 0 - for header, _lines in split_sections(new_text): - paths = parse_paths(header) - names = set(paths) if paths else {header.rstrip("\n")} - # Foreign only when NOTHING it names is in the reviewed diff: a rename - # the classifier rewrote should not read as foreign on one side alone. - if not (names & full_paths): + for _header, lines in split_sections(new_text): + section = "".join(lines) + if remaining[section] > 0: + remaining[section] -= 1 + else: foreign += 1 return foreign, _count_lines(new_text), _count_lines(full_text) diff --git a/.github/cursor-review/tests/test_incremental_diff.py b/.github/cursor-review/tests/test_incremental_diff.py index b9044d42..25d5f3d8 100644 --- a/.github/cursor-review/tests/test_incremental_diff.py +++ b/.github/cursor-review/tests/test_incremental_diff.py @@ -210,19 +210,59 @@ def test_empty_new_yields_an_empty_block(self): self.assertEqual(inc.build(_patch("a.py", "@@ -1 +1 @@", "-x\n+y\n"), ""), "") def test_a_binary_file_changed_since_last_round_is_emitted(self): + """One `git diff` invocation emits ONE binary format, not two. + + Without `--binary` — which this workflow does not pass — a changed + binary is a header, an `index`, and the CONSTANT line + `Binary files a/X and b/X differ`. So `index` is the section's only + content-dependent line, and excluding it (right for a text file, whose + blob id moves on every rebase) silently dropped every binary change. + The fixture this replaced pitted a `Binary files ... differ` stanza + against a `GIT binary patch` one — two formats a single `git diff` run + never mixes — so it passed while the gap was open. + """ old = ( "diff --git a/i.png b/i.png\n" "index 1111111..2222222 100644\n" "Binary files a/i.png and b/i.png differ\n" ) - new = ( + new = old.replace("..2222222", "..4444444") + self.assertEqual(inc.build(old, new), new) + + def test_an_unchanged_binary_file_is_not_re_emitted(self): + same = ( "diff --git a/i.png b/i.png\n" - "index 1111111..4444444 100644\n" - "GIT binary patch\n" - "literal 4\n" - "Lc$@\n" + "index 1111111..2222222 100644\n" + "Binary files a/i.png and b/i.png differ\n" ) - self.assertIn("GIT binary patch", inc.build(old, new)) + self.assertEqual(inc.build(same, same), "") + + def test_a_mode_change_on_an_already_edited_file_is_emitted(self): + """`+x` on a script the PR already edits. + + The mode lines sit BEFORE the first `@@`, so a signature that began at + the first `@@` compared the two rounds equal and dropped the section — + losing exactly the small, high-signal privilege change the block exists + to surface. Only the mode-ONLY case (no hunks at all) used to survive. + """ + old = _patch("s.sh", "@@ -1,2 +1,2 @@", "-a\n+b\n") + new = old.replace( + "diff --git a/s.sh b/s.sh\n", + "diff --git a/s.sh b/s.sh\nold mode 100644\nnew mode 100755\n", + ) + self.assertEqual(inc.build(old, new), new) + + def test_a_moved_base_blob_still_yields_no_block(self): + """Folding pre-hunk metadata in must not undo the rebase-quiet property. + + `index` and the similarity percentage track the BASE blob, so they move + on a rebase the author had no part in; they stay out of the signature. + """ + old = _patch("a.py", "@@ -1,3 +1,3 @@", "-x\n+y\n") + new = _patch( + "a.py", "@@ -41,3 +41,3 @@", "-x\n+y\n", index="9999999..8888888 100644" + ) + self.assertEqual(inc.build(old, new), "") def test_a_mode_only_change_new_this_round_is_emitted(self): new = ( @@ -285,17 +325,183 @@ def test_a_foreign_file_is_counted(self): def test_an_empty_block_is_a_subset(self): self.assertEqual(inc.check("", _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n"))[0], 0) - def test_a_rename_matching_on_one_side_is_not_foreign(self): - """The classifier's patch may name the file under only one of its paths.""" - block = "diff --git a/o.py b/n.py\n@@ -1 +1 @@\n-x\n+y\n" - full = _patch("n.py", "@@ -1 +1 @@", "-x\n+y\n") - self.assertEqual(inc.check(block, full)[0], 0) + def test_a_rename_section_copied_verbatim_is_not_foreign(self): + """A rename names two paths; copied verbatim it is still a subset.""" + full = ( + "diff --git a/o.py b/n.py\n" + "similarity index 90%\n" + "rename from o.py\n" + "rename to n.py\n" + "index 1111111..2222222 100644\n" + "--- a/o.py\n" + "+++ b/n.py\n" + "@@ -1 +1 @@\n-x\n+y\n" + ) + self.assertEqual(inc.check(full, full)[0], 0) + + def test_a_fabricated_hunk_under_a_carried_path_is_foreign(self): + """The property the README asserts, checked directly. + + Counting only path names waved this through: `a.py` IS in the reviewed + diff, so a section carrying a hunk the PR never wrote passed as a + verified subset. The bytes are what the panel reads, so the bytes are + what the fail-safe compares. + """ + full = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + forged = _patch("a.py", "@@ -1 +1 @@", "-x\n+subprocess.run(EXFIL)\n") + self.assertEqual(inc.check(forged, full)[0], 1) + + def test_a_duplicated_section_is_foreign_on_its_second_copy(self): + """Sections are matched as a multiset: the diff carries this one once.""" + a = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + self.assertEqual(inc.check(a + a, a)[0], 1) + + def test_a_reordered_block_is_still_a_subset(self): + """Order is not part of the property — verbatim content is.""" + a = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + b = _patch("b.py", "@@ -1 +1 @@", "-p\n+q\n") + self.assertEqual(inc.check(b + a, a + b)[0], 0) + + def test_what_build_emits_always_passes(self): + """build copies sections out of NEW, so check can only trip on a + builder bug — which is the whole reason it runs.""" + old = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + new = ( + _patch("a.py", "@@ -1 +1 @@", "-x\n+z\n") + + _patch("b.py", "@@ -1 +1 @@", "-p\n+q\n") + ) + self.assertEqual(inc.check(inc.build(old, new), new)[0], 0) def test_line_counts_match_wc_l(self): text = "diff --git a/a.py b/a.py\n@@ -1 +1 @@\n-x\n+y\n" self.assertEqual(inc.check(text, text)[1], 4) +class TestLineSplitting(unittest.TestCase): + """A git patch is LF-delimited. `str.splitlines` is not. + + It also breaks on a lone `\r`, `\v`, `\f`, `\x1c`-`\x1e`, `\x85`, U+2028 + and U+2029 — none of which git treats as a line break, and all of which a + PR can put in a content line. Splitting on them let the diff's own payload + forge a `diff --git` section boundary. + """ + + _FORGED = "diff --git a/lib/auth.py b/lib/auth.py" + + def test_a_form_feed_in_content_cannot_forge_a_section(self): + text = ( + "diff --git a/n.txt b/n.txt\n" + "index 1111111..2222222 100644\n" + "--- a/n.txt\n" + "+++ b/n.txt\n" + "@@ -0,0 +1 @@\n" + f"+x\x0c{self._FORGED}\n" + ) + self.assertEqual( + [header for header, _lines in inc.split_sections(text)], + ["diff --git a/n.txt b/n.txt\n"], + ) + + def test_the_forged_path_is_not_learned_by_the_fail_safe(self): + """The same bad split taught `check` the forged path, so a block + carrying it did not count as foreign — the guard disarming itself.""" + full = ( + "diff --git a/n.txt b/n.txt\n" + "@@ -0,0 +1 @@\n" + f"+x\x0c{self._FORGED}\n" + ) + block = f"{self._FORGED}\n@@ -1 +1 @@\n-secret\n+leaked\n" + self.assertEqual(inc.check(block, full)[0], 1) + + def test_no_other_unicode_break_splits_a_section(self): + for sep in ("\r", "\x0b", "\x0c", "\x1c", "\x1d", "\x1e", "\x85", "\u2028", "\u2029"): + with self.subTest(sep=repr(sep)): + text = ( + "diff --git a/a.py b/a.py\n" + "@@ -1 +1 @@\n" + f"+z{sep}diff --git a/forged b/forged\n" + ) + self.assertEqual(len(inc.split_sections(text)), 1) + + def test_a_patch_with_no_trailing_newline_keeps_its_last_line(self): + text = "diff --git a/a.py b/a.py\n@@ -1 +1 @@\n-x\n+y" + _header, lines = inc.split_sections(text)[0] + self.assertEqual("".join(lines), text) + + +class TestSectionPaths(unittest.TestCase): + """`rename from`/`rename to` carry ONE path per line, so they settle the + header ambiguity `parse_paths` can only guess at.""" + + _AMBIGUOUS = ( + "diff --git a/x b/c b/d\n" + "similarity index 90%\n" + "rename from x b/c\n" + "rename to d\n" + "index 1111111..2222222 100644\n" + "--- a/x b/c\n" + "+++ b/d\n" + "@@ -1 +1 @@\n-p\n+q\n" + ) + + def test_the_header_alone_takes_the_wrong_reading(self): + """`a/x b/c b/d` splits two ways and the header cannot say which.""" + self.assertEqual(inc.parse_paths("diff --git a/x b/c b/d\n"), ("x", "c b/d")) + + def test_the_rename_lines_settle_it(self): + header, lines = inc.split_sections(self._AMBIGUOUS)[0] + self.assertEqual(inc.section_paths(header, lines), ("x b/c", "d")) + + def test_a_non_rename_section_falls_back_to_the_header(self): + header, lines = inc.split_sections(_patch("a.py", "@@ -1 +1 @@", "-x\n+y\n"))[0] + self.assertEqual(inc.section_paths(header, lines), ("a.py", "a.py")) + + def test_a_rename_to_line_in_content_is_not_read_as_metadata(self): + """Only the pre-hunk region is metadata; `+rename to x` in a hunk body + is content, and must not redirect the section's key.""" + body = "-p\n+rename to /etc/shadow\n" + header, lines = inc.split_sections(_patch("a.py", "@@ -1 +1 @@", body))[0] + self.assertEqual(inc.section_paths(header, lines), ("a.py", "a.py")) + + def test_the_ambiguous_rename_keys_on_the_path_a_later_round_uses(self): + """Round N renames the file; round N+1 edits it in place and emits the + plain `diff --git a/d b/d`. Keyed off the header's wrong guess + (`c b/d`) the two never matched, so the file was re-emitted whole every + round after the rename.""" + header, lines = inc.split_sections(self._AMBIGUOUS)[0] + later = _patch("d", "@@ -1 +1 @@", "-p\n+q\n") + later_header, later_lines = inc.split_sections(later)[0] + self.assertEqual( + inc.section_paths(header, lines)[1], + inc.section_paths(later_header, later_lines)[1], + ) + + +class TestEmptyOldPatch(unittest.TestCase): + """Why the workflow step refuses to call the builder with an empty OLD. + + The builder cannot tell "nothing was reviewed last round" from "the OLD + patch came out empty", and must not: an empty OLD legitimately means every + file is new. The guard therefore belongs in the step, and these two tests + are what it is guarding against. + """ + + def test_an_empty_old_reproduces_the_whole_reviewed_diff(self): + full = ( + _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + + _patch("b.py", "@@ -1 +1 @@", "-p\n+q\n") + ) + self.assertEqual(inc.build("", full), full) + + def test_and_the_fail_safe_cannot_catch_that(self): + """Nothing is foreign and the block EQUALS the diff, so neither arm + trips: the panel just gets the same diff twice, prioritizing nothing.""" + full = _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n") + foreign, new_lines, full_lines = inc.check(inc.build("", full), full) + self.assertEqual(foreign, 0) + self.assertEqual(new_lines, full_lines) + + # --------------------------------------------------------------------------- # # 5. The CLI the workflow step actually calls # # --------------------------------------------------------------------------- # @@ -354,13 +560,16 @@ def test_check_exit_codes_and_output(self): self.assertRegex(out, r"full_lines=\d+") def test_check_rejects_a_block_longer_than_the_reviewed_diff(self): - """Same paths, more lines — the second half of the subset property.""" + """Same path, more lines. Both arms of the fail-safe trip here now: the + section is not carried verbatim AND the block outgrows the diff. The + length arm is kept as a redundant second gate, and its two numbers are + what the workflow's ::warning:: interpolates.""" full = self._file("full.patch", _patch("a.py", "@@ -1 +1 @@", "-x\n+y\n")) long_block = _patch("a.py", "@@ -1,9 +1,9 @@", "".join(f"+l{i}\n" for i in range(20))) bad = self._file("bad.patch", long_block) rc, out = self._main(["check", "--new", bad, "--full", full]) self.assertEqual(rc, 2) - self.assertIn("foreign=0", out) + self.assertIn("foreign=1", out) def test_non_utf8_bytes_survive_a_round_trip(self): """PR bytes are attacker-authored; the helper must not die on them.""" @@ -412,6 +621,32 @@ def test_the_fail_safe_warning_is_present(self): def test_incremental_subset_is_a_job_output(self): self._assert_has("incremental_subset:") + def test_the_step_declines_an_empty_last_reviewed_patch(self): + """A successful-but-EMPTY OLD must not reach the builder. + + `BASE...LAST` comes out empty whenever merge-base(BASE, LAST) is LAST + itself, or whenever $DIFF_EXCLUDES filters every file out. git reports + no error, so only an explicit `-s` test stops the fall-through that + re-emits the entire reviewed diff as the "incremental" block. + """ + self._assert_has('! build_old_patch || [ ! -s "$OLD_PATCH" ]') + + 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.""" + self._assert_has( + 'git -c core.quotePath=false diff "${BASE_SHA}...${LAST_REVIEWED_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 + reviewed diff, which cost nothing against that cap.""" + self._assert_has("OLD_PATCH_MAX_BYTES") + self._assert_has('[ "$(wc -c < "$OLD_PATCH")" -gt "$OLD_PATCH_MAX_BYTES" ]') + def test_the_helper_is_loaded_from_the_pinned_checkout_not_the_pr(self): """The path the step calls must be one a pinned checkout actually writes. diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 3bf731a7..c6f27cca 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -1009,13 +1009,26 @@ jobs: # able to rewrite the logic that decides what the panel is shown. 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 + # 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 + # big enough to burn the runner's memory on. Declining fails closed to + # "no block", the same as every other case below. + OLD_PATCH_MAX_BYTES=33554432 # 32 MiB # Wrapped in a function so the shellcheck directive sits in front of a # complete command — it is rejected in front of an `elif` branch. build_old_patch() { + # core.quotePath=false to match check-pr-size's `writeReviewedDiff`, + # which builds the NEW side. Under the default (quoting ON) a + # 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. # $DIFF_EXCLUDES is intentionally unquoted so bash word-splits it # into pathspec args. # shellcheck disable=SC2086 - git diff "${BASE_SHA}...${LAST_REVIEWED_SHA}" -- . $DIFF_EXCLUDES > "$OLD_PATCH" + git -c core.quotePath=false diff "${BASE_SHA}...${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 @@ -1031,11 +1044,20 @@ jobs: echo "Last-reviewed SHA ${LAST_REVIEWED_SHA} is unreachable (force-push or base-branch rewrite) — 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; then - # Fail CLOSED to "no block". Falling through with an empty OLD would - # make every file read as new and emit the whole reviewed diff a - # second time — a doubled prompt that prioritizes nothing. - echo "Could not build the last-reviewed patch (${LAST_REVIEWED_SHA}) — skipping the incremental diff block." + elif ! build_old_patch || [ ! -s "$OLD_PATCH" ]; then + # Fail CLOSED to "no block" — for a FAILED build and an EMPTY one + # alike. Falling through with an empty OLD would make every file + # read as new and emit the whole reviewed diff a second time: a + # 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 + # 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 + echo "The last-reviewed patch is larger than ${OLD_PATCH_MAX_BYTES} bytes — skipping the incremental diff block rather than parsing it." elif ! python3 "$INCREMENTAL_PY" build \ --old "$OLD_PATCH" --new "$FULL_PATCH" --out "$NEW_PATCH"; then echo "::warning::Could not build the incremental diff block — the panel will run on the full reviewed diff alone." diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 73d4a564..d4a57494 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -146,7 +146,7 @@ see the spend warning below). **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 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 the block named files the reviewed diff does not carry (or came out longer than it) and 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. +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. **Dependabot PRs are not covered by the fork skip.** Dependabot's branches live in the base repo, so the gate's cross-repo check treats them as ordinary PRs — but From 74bbcff853483c935a3e477b98b8027812db3ee6 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 21:36:29 +0000 Subject: [PATCH 3/5] fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incremental "hunks new since the last reviewed round" block built its OLD side as `git diff BASE...LAST_REVIEWED`, which is right only while BASE still resolves to the merge base that round 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, everything the branch inherited from the old one lands on both sides, and hunks the panel has never seen are subtracted as "already reviewed". Nothing downstream catches it — the block is merely smaller, and a smaller block still passes the subset fail-safe. Every consolidated review now carries a round sentinel directly under its header recording the commit it reviewed and the merge base it was diffed against. The next round's ledger reads the merge base back out of the LAST round's sentinel — accepting it only when its `head` is that review's own `commit_id` and its SHAs are full lowercase hex — and the incremental step builds OLD as a two-dot diff against exactly that commit. With no usable recorded merge base it emits no block at all and logs which case it hit, rather than falling back to the current base, which is the bug. Every live PR sees one block-less round after this ships. --- .github/cursor-review/README.md | 2 + .github/cursor-review/build-ledger.py | 110 ++++++++ .github/cursor-review/post-review.py | 97 ++++++- .../cursor-review/tests/test_build_ledger.py | 177 +++++++++++++ .../tests/test_incremental_diff.py | 176 ++++++++++++- .../cursor-review/tests/test_post_review.py | 249 +++++++++++++++++- .github/workflows/cursor-review.yml | 118 +++++++-- docs/callers/cursor-review.md | 2 + 8 files changed, 907 insertions(+), 24 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index a4507acc..2db72d61 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -91,6 +91,8 @@ Those per-entry lines are the ledger's own structure, so imported prose must not 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. +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 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 its `head` is that review's own `commit_id` and its `merge_base` is a full lowercase hex SHA. `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. ### The panel diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py index 92f623e0..a00595f1 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -217,6 +217,36 @@ 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. +_FULL_SHA_RE = re.compile(r"^[0-9a-f]{40}$") + # 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 +491,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 +894,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 +912,38 @@ 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. + # + # Three gates, all of which must hold, and all of which fail to "" rather than to a + # guess. 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 = "" + round_sentinel = _parse_round_sentinel(consolidated[-1].get("body") or "") + 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 +1144,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 +1175,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 +1196,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 +1612,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/post-review.py b/.github/cursor-review/post-review.py index 8d82c41d..ea9fc077 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -139,6 +139,26 @@ # 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. +_ROUND_SHA_RE = re.compile(r"^[0-9a-f]{40}$") + # --- 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 @@ -1874,6 +1894,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. @@ -1907,6 +1951,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), ) @@ -2673,6 +2727,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( @@ -2710,15 +2782,30 @@ 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. `post_error_review` gets the plain one: a round that + # failed reviewed nothing, so recording what it "diffed against" would be a claim + # about a panel that never ran — and build-ledger.py refuses the error-review shape + # outright anyway, so a sentinel there could only ever be misleading. + 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) @@ -2750,13 +2837,13 @@ def main(): all_failed = bool(panel) and all(c.get("status") != "ok" for c in panel) if all_failed: body_text = ( - f"{header}\n\n⚠️ **Panel did not produce any findings.**\n\n" + f"{review_header}\n\n⚠️ **Panel did not produce any findings.**\n\n" "Every reviewer in the matrix failed to contribute — see the " "panel summary for which cells errored, and the run logs for " "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( @@ -2816,7 +2903,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 252b6675..451ca43b 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -2439,3 +2439,180 @@ 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_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 25d5f3d8..c4b600c1 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 908e2255..d93c17e6 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_panel_produced_nothing_body_carries_it(self): + payload = self.run_main( + [], + commit_sha=HEAD_40, + extra_argv=self.ARGV, + panel=[{"model": "m", "review_type": "adversarial", "status": "error"}], + )[0] + self.assertIn("Panel did not produce any 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 one body 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"]) + + # -- 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 c6f27cca..b8527d9e 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -592,6 +592,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 @@ -764,6 +771,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 @@ -799,6 +814,31 @@ jobs: echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" 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 }} + 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 @@ -985,20 +1025,31 @@ 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. + # + # 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" @@ -1010,7 +1061,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 @@ -1025,10 +1076,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 @@ -1042,6 +1098,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 @@ -1051,9 +1123,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 @@ -2310,7 +2382,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 @@ -2540,6 +2616,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 }} @@ -2629,6 +2711,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[@]}" \ @@ -2647,6 +2731,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[@]}" \ @@ -2672,6 +2758,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 d4a57494..a69e1392 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -146,6 +146,8 @@ see the spend warning below). **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. +**A retargeted PR gets one round with no block, on purpose.** Each consolidated review now carries a hidden *round sentinel* recording the commit it reviewed and the merge base it was diffed against, 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. **Dependabot PRs are not covered by the fork skip.** Dependabot's branches live in From 65f04d933d4510c15c63dece3a71a072a896e4c5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 22:09:37 +0000 Subject: [PATCH 4/5] fix(cursor-review): refuse a round sentinel from a round that reviewed nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel review of BE-15598. Four fixes, one documentation pass. 1. `build_ledger` now refuses the ERROR-REVIEW shape before parsing the round sentinel, exactly as `_body_only_entries` already does 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 satisfy the line anchor the sentinel's containment argument rests on. The writer-side defang added alongside it only reaches bodies THIS version wrote, while consumers stay pinned to older SHAs and every error review they have already posted sits on their PRs undefanged. The `head == commit_id` gate does not close that window either — the reviewed head SHA is public to the PR author — and the workflow's `cat-file -e` / `merge-base --is-ancestor` checks prove ancestry, not that a commit was the merge base round N used. A comment in post-review.py already asserted this reader-side refusal existed; now it does. 2. The all-panel-cells-failed body takes the PLAIN header, so it writes no sentinel. It carries no "Review failed" heading, so (1) does not reach it, and it is posted with `delivers=False` precisely because nothing was reviewed — yet it was recording a merge base the next round would have subtracted hunks against. Same rule as the error review, enforced at the writer because that is the only place it can be. 3. `_FULL_SHA_RE` / `_ROUND_SHA_RE` terminate with `\Z`, not `$`. Python's `$` also matches just before a final newline, so `^[0-9a-f]{40}$` accepted a 41-character value ending in one — and that gate is the single control keeping a line break out of the `key=value` `_write_outputs` appends to $GITHUB_OUTPUT. The invariant the surrounding code relies on is now the one the regex provides. 4. A test that actually pins the column-0 containment. The two existing forgery tests could not fail for the reason they claim: `search` returns the FIRST match and a genuine sentinel sits ahead of the forgery, so both pass with the line anchor removed entirely — the same regression the module comments record happening once already on the body-only sentinel. The new case carries no real sentinel at all, across every indent a rendered finding can sit behind. Docs: both ledger sections stated the superseded `git diff BASE...LAST_REVIEWED` formulation in a paragraph directly above the one that reverses it, so a reader going top-down got the pre-BE-15598 behaviour first. Folded into the new formulation, and both now say which rounds record no sentinel at all. Two panel nits are answered in comments rather than code, since neither is reachable: the `diff-size` merge-base step needs no line-break guard (`git merge-base` on two commits prints one line and `$( )` strips the newline), and the doubled-prompt shape on the retarget path is a known prompt-budget waste whose fix needs a threshold nothing here has grounds to pick. Verified: 705 tests, 0 failures; each new test confirmed to fail against the un-fixed code. check_workflow_pins.py OK; check_agents_md.py passed; shellcheck -S warning clean over both edited run blocks. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/README.md | 4 +- .github/cursor-review/build-ledger.py | 35 ++++++++-- .github/cursor-review/post-review.py | 30 +++++++-- .../cursor-review/tests/test_build_ledger.py | 65 +++++++++++++++++++ .../cursor-review/tests/test_post_review.py | 43 ++++++++---- .github/workflows/cursor-review.yml | 18 +++++ docs/callers/cursor-review.md | 4 +- 7 files changed, 174 insertions(+), 25 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 2db72d61..da504938 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -89,9 +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 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 its `head` is that review's own `commit_id` and its `merge_base` is a full lowercase hex SHA. `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. +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 a00595f1..3d80bab1 100644 --- a/.github/cursor-review/build-ledger.py +++ b/.github/cursor-review/build-ledger.py @@ -245,7 +245,13 @@ def _load_gate_unresolved(): # 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. -_FULL_SHA_RE = re.compile(r"^[0-9a-f]{40}$") +# +# 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 @@ -919,15 +925,36 @@ def build_ledger( # already reviewed and drop them — a silent loss the subset fail-safe cannot catch, # since a smaller block is still a subset. # - # Three gates, all of which must hold, and all of which fail to "" rather than to a - # guess. Only the LAST round is read (it is the only one the next block diffs + # 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 = "" - round_sentinel = _parse_round_sentinel(consolidated[-1].get("body") or "") + 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 diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index ea9fc077..89fe7682 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -157,7 +157,12 @@ # "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. -_ROUND_SHA_RE = re.compile(r"^[0-9a-f]{40}$") +# +# `\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 @@ -2787,10 +2792,13 @@ def main(): # 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. `post_error_review` gets the plain one: a round that - # failed reviewed nothing, so recording what it "diffed against" would be a claim - # about a panel that never ran — and build-ledger.py refuses the error-review shape - # outright anyway, so a sentinel there could only ever be misleading. + # 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) ) @@ -2836,8 +2844,18 @@ 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"{review_header}\n\n⚠️ **Panel did not produce any findings.**\n\n" + f"{header}\n\n⚠️ **Panel did not produce any findings.**\n\n" "Every reviewer in the matrix failed to contribute — see the " "panel summary for which cells errored, and the run logs for " "the underlying cause." diff --git a/.github/cursor-review/tests/test_build_ledger.py b/.github/cursor-review/tests/test_build_ledger.py index 451ca43b..2394f2af 100644 --- a/.github/cursor-review/tests/test_build_ledger.py +++ b/.github/cursor-review/tests/test_build_ledger.py @@ -2561,6 +2561,71 @@ def test_a_sentinel_behind_a_blockquote_marker_is_not_the_match(self): 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]): diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index d93c17e6..9b45f56b 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -5195,16 +5195,6 @@ def test_the_no_findings_body_carries_it(self): self.assertIn("✅ No high-signal findings.", payload["body"]) self._assert_directly_under_the_header(payload["body"]) - def test_the_panel_produced_nothing_body_carries_it(self): - payload = self.run_main( - [], - commit_sha=HEAD_40, - extra_argv=self.ARGV, - panel=[{"model": "m", "review_type": "adversarial", "status": "error"}], - )[0] - self.assertIn("Panel did not produce any 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)], @@ -5216,7 +5206,7 @@ def test_the_wholesale_fallback_body_carries_it_too(self): self.assertEqual(len(posted), 2) self._assert_directly_under_the_header(posted[1]["body"]) - # -- and the one body that must NOT ------------------------------------- # + # -- 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 @@ -5231,6 +5221,37 @@ def test_the_error_review_body_carries_no_sentinel(self): 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): diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index b8527d9e..33096afc 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -830,6 +830,14 @@ jobs: 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 @@ -1035,6 +1043,16 @@ jobs: # 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 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index a69e1392..5514cddc 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -144,9 +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 now carries a hidden *round sentinel* recording the commit it reviewed and the merge base it was diffed against, 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. +**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. From cec0e332bc7c51448f737356fd7a3920da20e934 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 20 Sep 2026 07:28:27 +0000 Subject: [PATCH 5/5] fix(cursor-review): trim the superseded three-dot prose, and stop re-collecting the end-to-end suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit findings from the round on #322. Both prose sites that still introduced OLD as a three-dot diff against the base named the formulation BE-15598 replaced as the reason the subset property holds, which is the argument that invited the bug. `incremental-diff.py`'s module docstring now describes OLD as the two-dot diff against the merge base round N recorded — and why it is read back rather than recomputed — and `cursor-review.yml`'s comment no longer claims both patches are three-dot, which the same comment already contradicted forty lines further down. `RoundSentinelTest` subclassed `EndToEndPostTest` to reach its stubbed-`gh` harness, so discovery collected that class's five `test_*` methods a second time under the child and CI ran them twice (measured: 19 collected, 14 its own). The harness — `run_main`, the class's only non-test member — moves to a `PostHarness` mixin that is not a `TestCase`, and both suites mix it in. Discovery now collects RoundSentinelTest's 14 and EndToEndPostTest's 5 exactly once each; the suite goes 720 OK, down from 725 by precisely the five duplicates, with no case lost. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/incremental-diff.py | 20 ++++++++++++----- .../cursor-review/tests/test_post_review.py | 22 ++++++++++++++----- .github/workflows/cursor-review.yml | 6 ++--- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/.github/cursor-review/incremental-diff.py b/.github/cursor-review/incremental-diff.py index 2deb3de2..f531df52 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/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 377fd697..9efb9958 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -230,8 +230,15 @@ def test_a_real_diff_returns_a_map(self): _UNSET = object() -class EndToEndPostTest(unittest.TestCase): - """Drive main() with a stubbed `gh` and read the payload it would have sent.""" +class PostHarness: + """The stubbed-`gh` harness both end-to-end suites drive main() through. + + A plain object, NOT a `unittest.TestCase`: when this lived on + `EndToEndPostTest` and `RoundSentinelTest` subclassed that class to reach it, + discovery collected the parent's five `test_*` methods a second time under the + child and CI ran them twice. Carrying the shared helpers on a mixin with no + `test_*` methods of its own keeps every case collected exactly once. + """ def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summaries=None, panel=None, existing_reviews=None, list_returncode=0, list_calls=None, @@ -375,6 +382,10 @@ def fake_summary(markdown, note=None): outputs[key] = value return posted + +class EndToEndPostTest(PostHarness, unittest.TestCase): + """Drive main() with a stubbed `gh` and read the payload it would have sent.""" + def test_the_field_regression_nine_anchor_one_lands_in_the_body(self): # The observed shape: ten findings, one citing a line outside every hunk. findings = [finding("app.py", ln) for ln in (10, 11, 12, 13, 81, 82, 83)] @@ -5314,7 +5325,7 @@ def test_the_path_is_the_one_the_download_step_writes(self): MERGE_BASE_40 = "c" * 40 -class RoundSentinelTest(EndToEndPostTest): +class RoundSentinelTest(PostHarness, unittest.TestCase): """The round sentinel: what this round reviewed, and what it diffed against (BE-15598). The ledger already records WHICH commit the last round reviewed. Without what it @@ -5324,8 +5335,9 @@ class RoundSentinelTest(EndToEndPostTest): from the incremental block. The subset fail-safe cannot catch a block that is merely too small, so the record has to be written down at the time. - Subclasses EndToEndPostTest for its stubbed-`gh` harness; the cases below are the - only ones that pass a real 40-hex `commit_sha`. + Mixes in `PostHarness` for the stubbed-`gh` harness rather than subclassing + `EndToEndPostTest`, which would re-collect that class's own `test_*` methods here; + the cases below are the only ones that pass a real 40-hex `commit_sha`. """ ARGV = ["--base-sha", BASE_40, "--merge-base-sha", MERGE_BASE_40] diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 37a8f0e3..8855d920 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -1083,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