[PR #321] [MERGED] fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff #321

New issue

Closed

opened 2026-10-26 22:38:28 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:28 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/321
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/20/2026
Merged by: @mattmillerai

Base: main ← Head: be-15558-incremental-diff-subset

📝 Commits (2)

da6cbc8 fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff
a2da829 fix(cursor-review): close seven gaps the panel found in the incremental block

📊 Changes

5 files changed (+1095 additions, -7 deletions)

View changed files

📝 .github/cursor-review/README.md (+5 -0)
➕ .github/cursor-review/incremental-diff.py (+282 -0)
➕ .github/cursor-review/tests/test_incremental_diff.py (+687 -0)
📝 .github/workflows/cursor-review.yml (+117 -7)
📝 docs/callers/cursor-review.md (+4 -0)

📄 Description

ELI-5

Each review round after the first gets a bonus block in the prompt saying "here are the hunks that are new since last time — look here first". The prompt calls that block "the subset of the diff above". It wasn't one. It was computed as git diff <last-reviewed-commit>...<head>, and if you merge main into your branch, your last-reviewed commit becomes an ancestor of head — so that range quietly swallows every commit the merge pulled in from main. The reviewer was handed a mountain of other people's code, labelled as your PR.

The fix stops diffing two commits and starts diffing two PR patches: what the last round saw, versus what this round is reviewing. Both are three-dot diffs against the base, so neither can contain a base-branch commit, and every section emitted is copied verbatim out of the diff being reviewed. Then it's checked anyway, and thrown away if the check fails.

What was wrong

diff-size → Build incremental diff since last review built the block with git diff "${LAST_REVIEWED_SHA}...${HEAD_SHA}". With a merge commit at HEAD the merge base of those two commits is LAST_REVIEWED_SHA, so the range expands to the whole merge. Measured on one consumer repo: a round built a 9,800-line block against a 1,234-line reviewed diff, 119 of the block's 133 files outside the PR; two other PRs built ~1.16M-line blocks against 481- and 1,476-line reviewed diffs. Every leg's prompt grew from ~98 KB to ~826 KB, four of six legs timed out, and the two that finished reviewed base-branch files.

What changed

.github/cursor-review/incremental-diff.py (new). build --old --new --out splits both patches into per-file sections on ^diff --git and emits every NEW section whose hunk content differs from the same file's section in OLD, plus every NEW section for a file OLD lacks. A file OLD had and NEW doesn't contributes nothing — it's no longer in the PR. @@ -a,b +c,d @@ headers are normalised to @@ @@ before comparison, so a pure rebase (identical hunks, shifted line numbers) produces an empty block. Lines that track the base blob rather than the branch's own edit — index <blob>..<blob>, the similarity percentages — are excluded for the same reason: they move whenever the base blob does. The pre-hunk metadata that does not (old mode/new mode, rename from/rename to, typechange) is compared, so a round that only adds the executable bit to an already-edited script still shows up; and a binary section keeps its index line, because without --binary the Binary files a/X and b/X differ line is a constant and index is the only content-dependent line git emits. Splitting is LF-only throughout — str.splitlines also breaks on \v, \f, \x85, U+2028 and friends, which would let a content line forge a diff --git section boundary. check --new --full is the fail-safe, returning the foreign-section count and both line counts.

The workflow step. OLD is git -c core.quotePath=false diff "${BASE_SHA}...${LAST_REVIEWED_SHA}" -- . $DIFF_EXCLUDES — the quotePath flag matching writeReviewedDiff, which builds NEW, so a non-ASCII path keys the same on both sides. A build that fails, returns an EMPTY patch, or returns one over 32 MiB is declined rather than used. NEW is pr-diff.patch itself rather than a freshly computed BASE...HEAD diff — see the judgment call below. The helper is loaded from _pr_size_tool/, the pinned checkout of this repo the classifier is already built from, never the PR's own tree. After it runs, the step verifies the result and — if any section of the block is not carried byte for byte by pr-diff.patch, or the block has more lines than pr-diff.patch — truncates it to empty and emits ::warning::Incremental diff discarded: it was not a subset of the reviewed diff (<n> foreign file(s), <a> vs <b> lines). The Incremental diff since round N (<sha>): <n> lines log line is unchanged, so the sweep that sized this problem can be re-run against it.

New incremental_subset output on the incremental step and on the diff-size job. true when the block passed the check or when there is no block at all (round 1, an unreachable last-reviewed SHA, an empty reviewed diff, an over-cap run where the step never ran); false only when a block that had been built was discarded. Written as != 'false' so a skipped step reads as true rather than ''.

Docs. Two paragraphs in the ledger section of .github/cursor-review/README.md plus a scripts-table row, and two in the Gotchas of docs/callers/cursor-review.md — both state that the block is derived from the two PR patches and can never carry a hunk the PR doesn't, and both quote the fail-safe warning verbatim and say what a caller seeing it should conclude (the panel ran on the full diff; nothing was skipped or suppressed).

Judgment calls

NEW is pr-diff.patch, not a fresh git diff BASE...HEAD. The plan I worked from specified the latter. pr-diff.patch is already exactly git diff mergeBase...head -- . <excludes> minus the generated-file sections the classifier stripped (writeReviewedDiff, scripts/check-pr-size/main.go:166), and using it buys two things a fresh diff doesn't. The subset property becomes structural — every emitted section is a verbatim byte-slice of the file the fail-safe compares against — instead of merely checked after the fact. And a generated file touched since the last round can't reappear in the block: with a fresh BASE...HEAD diff it would, it isn't in pr-diff.patch, the fail-safe would count it foreign, and the entire block would be discarded over a lockfile. The fail-safe is kept regardless, as defence in depth.

The two patches can resolve to different merge bases. OLD resolves to merge-base(BASE, LAST), NEW to merge-base(BASE, HEAD), and a round that merged the base branch in moves the second one forward. A file both the base branch and the PR touched then reads as changed even if the author's own edit didn't move. That is over-inclusion strictly within the PR's own files — it costs a little prompt budget and can't break the subset property. Pinning both to one merge base would hide a hunk whose surrounding code really did change under it, which is worse. Noted in the step comment.

Every "couldn't build one" path leaves the block empty and incremental_subset true. Nothing was discarded in those cases; there was simply nothing to prioritize, and the panel behaves as it does on round 1. Reporting false there would make the integrity signal fire on a non-event. A helper crash during build is the one in-between case: it logs its own ::warning::, empties the block and leaves the output true, because again no block was discarded — one was never produced.

No Skip-caller-bump: true trailer. This is a behavioural change inside a watched surface; the fleet should bump.

Verification

Reproduced against real git rather than fixtures. TestMergeCommitHead builds an actual repository — base branch advances by 400 lines plus a new file, the PR branch merges it in and edits its own file — and pins four things: the old formulation really does drag unrelated.py in (424 lines vs a 10-line reviewed diff), the new one emits only app.py, the result passes the subset check, and the old formulation would have failed it. I also extracted the step's run: block straight out of cursor-review.yml and ran it verbatim against that repository for every branch: merge-commit head (10-line block, app.py only, incremental_subset=true), a real git rebase onto the advanced base (empty block), round 1 / LAST == HEAD / unreachable LAST / empty reviewed diff (all skip cleanly), diff_excludes word-splitting through the new wrapper function, a missing helper, and a deliberately sabotaged builder that re-emitted the old commit-range diff — which produced exactly ::warning::Incremental diff discarded: it was not a subset of the reviewed diff (2 foreign file(s), 424 vs 10 lines)., truncated the block to 0 lines, and appended incremental_subset=false.

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' (the command test-cursor-review-scripts.yml runs, and its test_*.py glob picks the new file up): 636 tests, OK — 36 of them new.
shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh: clean. The new step's run: block, extracted and shellchecked separately: clean — the elif chain is why build_old_patch is a function, since a # shellcheck disable= directive is rejected in front of an elif branch.
python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 155 pinned refs. .github/workflow-pins/tests: 499 tests, OK.
python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings — AGENTS.md at 156 lines and no CODEOWNERS; neither touched here).
Other suites, to confirm nothing collateral: groom 405 OK, public-repo-hygiene 204 OK, refresh-reviewers 56 OK, agents-md-integrity OK, linear-ticket OK.
Negative-tested the new drift guard: pointing INCREMENTAL_PY at the PR's own checkout makes test_the_helper_is_loaded_from_the_pinned_checkout_not_the_pr fail as intended.
The new test file is stdlib-only (contextlib, importlib, io, os, shutil, subprocess, tempfile, unittest), matching the repo rule and the sibling workflow suites' explicit note that a yaml import would not run in this job.

Residual

The pytest invocation named in the plan was not run. The plan's verification step is python3 -m pytest .github/cursor-review/tests/; this host has neither pytest nor pip, so I ran the unittest discover command test-cursor-review-scripts.yml actually uses — which AGENTS.md names as the authority, and which is what CI will run. Every new test is a plain unittest.TestCase, so pytest would collect them identically, but I did not empirically confirm that.

The live end-to-end reproduction on a test PR was not performed. The plan asks for it: open a PR here, trigger the review label, push a merge of main into the branch, re-trigger, and read the diff-size log. That needs two labelled review rounds against a real panel (billed cursor-review runs), so it is out of reach of a non-mutating check from this session. What I substituted is described under Verification: the step's own run: block, extracted verbatim from cursor-review.yml, executed against a real repository containing a real merge commit, for each of its branches. That exercises the same shell but not the surrounding job — in particular it does not prove that _pr_size_tool/ is still populated at this point in a real run (a drift guard now asserts the path matches a declared checkout path:, which is the static half of that claim), nor that needs.ledger.outputs.last_reviewed_sha carries what this step assumes on a live PR. This PR itself is the natural place to confirm it: it is labelled cursor-review, so pushing a merge of main into this branch and re-triggering the label would produce exactly the log the plan asks for.

The evidence this change is built on was read, not re-measured. The 9,800-line / ~1.16M-line / ~826 KB figures, the 119-of-133 foreign-file count and the four-of-six-legs-timed-out result come from a prior read-only investigation on a private consumer repository. I have no access to that repository or to those runs, so I could not re-derive any of those numbers; they are quoted in the step comment and the docs as measurements, and this PR does not independently corroborate them. The mechanism they describe is independently reproduced here in TestMergeCommitHead, which is the part the fix actually depends on.

Not swept: how many consumer PRs were affected. The investigation this came from sized the problem on specific PRs; I did not and could not sweep the rest of the fleet's review history to count how many rounds shipped a non-subset block, because those runs and their logs are on repositories this session cannot read. Anyone with that access can now count them directly — the incremental_subset output and the discard warning make it observable going forward, but neither is retroactive.

A consumer already reading diff-size job outputs sees a new one. incremental_subset is additive, so no existing caller breaks, but note that when the whole diff-size job is skipped (gate says don't run) the output is '', not 'true' — the != 'false' default only covers the skipped-step case within a running job. The sibling ticket that turns this into a check-run conclusion should treat '' as "no panel ran" rather than as a verdict.

Two pre-existing agents-md-integrity warnings remain unfixed and are unrelated to this change: AGENTS.md is 156 lines against an aspirational 150 (hard ceiling 200, so it passes), and the repo has no CODEOWNERS file, so AGENTS.md has no DRI. I deliberately did not add to AGENTS.md given the first.

Review round

The panel raised nine findings. Eight are fixed in a2da829; each was reproduced before it was fixed, and each has a regression test.

An empty OLD fell through the fail-closed guard. ! build_old_patch only caught a non-zero exit. BASE...LAST comes back empty — with no error — 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, and the block then became a byte-for-byte copy of the whole reviewed diff: 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 rather than exceeding it. Guard is now ! build_old_patch || [ ! -s "$OLD_PATCH" ].
OLD and NEW disagreed on core.quotePath. NEW comes from check-pr-size's -c core.quotePath=false diff; OLD used the default, so a non-ASCII path arrived C-quoted on one side and plain on the other and the two could never key alike — re-emitting the file in full every round, including a pure rebase. The suite's own _diff helper already passed the flag, which is why nothing caught it.
Pre-hunk metadata was dropped whenever the file also had hunks. A round that only added +x to an already-edited script compared equal and vanished from the block.
str.splitlines let the diff forge its own section boundaries. +x\x0cdiff --git a/lib/auth.py b/lib/auth.py produced two sections, and check learned the forged path from the same bad split, so it did not count as foreign — the guard disarming itself. All splitting is LF-only now.
A changed binary was silently dropped. Excluding index is right for a text file and wrong for a binary, whose other lines are constant. The test that covered this pitted a Binary files … differ stanza against a GIT binary patch one — two formats one git diff invocation never mixes — so it passed while the gap was open.
The fail-safe was weaker than the property the docs assert. It compared path names; "can never contain a hunk the PR does not carry" needs bytes. A fabricated hunk under a path the PR does touch passed as a verified subset. check now requires every section to appear byte for byte in the reviewed diff, matched as a multiset. Both docs now describe it as the byte test it is.
An ambiguous rename header keyed off the wrong split. a/x b/c b/d reads two ways; candidates[0] took ('x', 'c b/d'), which no later round's plain diff --git a/d b/d could match. A new section_paths prefers the section's own rename from/rename to lines — one path per line, so unambiguous. (The finding's first example, a/old name.py b/new name.py, in fact parses correctly today: the split needs a literal b/.)
OLD was unbounded. NEW is capped by diff_size_cap, but BASE...LAST keeps the generated-file sections the classifier strips out of the reviewed diff, and those cost nothing against that cap. Now declined over 32 MiB, on the same fail-closed path as every other "can't build one" branch.

Deferred, with a follow-up filed: BASE_SHA is the current base, not the base the previous round diffed against. After a retarget, BASE...LAST_REVIEWED can carry hunks the previous reviewed diff never showed, and this round then suppresses them — under-inclusion, the one direction the "different merge bases" judgment call above does not accept. It cannot be fixed here: the ledger records last_reviewed_sha and no base SHA, so fixing it means a per-round base SHA in build-ledger.py and in the ledger's rendered-and-reparsed comment format, plus a compatibility path for entries already live on open PRs. Blast radius is bounded — the block is only a prioritization hint and the panel always sees the full reviewed diff beside it, so no hunk goes unreviewed.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 657 passed, 0 failed (57 in test_incremental_diff.py, 21 added this round); .github/workflow-pins/tests: 499 passed, 0 failed; groom 405 / public-repo-hygiene 204 / linear-ticket 116 / refresh-reviewers 56 / agents-md-integrity 46 passed; python3 .github/workflow-pins/check_workflow_pins.py: OK (11 workflows, 155 pinned refs); python3 .github/agents-md-integrity/check_agents_md.py --root .: passed, 2 pre-existing warnings; shellcheck -x on the two cursor-review shell scripts and on the step's extracted run: block: clean, with its four guard branches (failed / empty / oversized / usable OLD) exercised directly; every one of the eight fixed findings reproduced against the pre-fix code before being fixed
Deviations: NEW is pr-diff.patch rather than a fresh git diff BASE...HEAD (rationale under Judgment calls); on the fail-safe finding I took byte-exact section matching rather than the narrower "require the NEW-side path to be carried, relaxing for a rename pair" the reviewer proposed — it subsumes that case and needs no rename special-casing; the base-drift finding is deferred to a follow-up rather than fixed (see Review round); pytest and the live two-round reproduction on a test PR were still not run (both under Residual)

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/321 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `be-15558-incremental-diff-subset` --- ### 📝 Commits (2) - [`da6cbc8`](https://github.com/Comfy-Org/github-workflows/commit/da6cbc8c46eee15a26ba19f4d08465e84bc1fab5) fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff - [`a2da829`](https://github.com/Comfy-Org/github-workflows/commit/a2da829779f99b2fdb0c67862a2737ad173004ac) fix(cursor-review): close seven gaps the panel found in the incremental block ### 📊 Changes **5 files changed** (+1095 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+5 -0) ➕ `.github/cursor-review/incremental-diff.py` (+282 -0) ➕ `.github/cursor-review/tests/test_incremental_diff.py` (+687 -0) 📝 `.github/workflows/cursor-review.yml` (+117 -7) 📝 `docs/callers/cursor-review.md` (+4 -0) </details> ### 📄 Description ## ELI-5 Each review round after the first gets a bonus block in the prompt saying "here are the hunks that are new since last time — look here first". The prompt calls that block "the subset of the diff above". It wasn't one. It was computed as `git diff <last-reviewed-commit>...<head>`, and if you merge `main` into your branch, your last-reviewed commit becomes an *ancestor* of head — so that range quietly swallows every commit the merge pulled in from `main`. The reviewer was handed a mountain of other people's code, labelled as your PR. The fix stops diffing two commits and starts diffing two PR *patches*: what the last round saw, versus what this round is reviewing. Both are three-dot diffs against the base, so neither can contain a base-branch commit, and every section emitted is copied verbatim out of the diff being reviewed. Then it's checked anyway, and thrown away if the check fails. ## What was wrong `diff-size` → *Build incremental diff since last review* built the block with `git diff "${LAST_REVIEWED_SHA}...${HEAD_SHA}"`. With a merge commit at HEAD the merge base of those two commits **is** `LAST_REVIEWED_SHA`, so the range expands to the whole merge. Measured on one consumer repo: a round built a 9,800-line block against a 1,234-line reviewed diff, 119 of the block's 133 files outside the PR; two other PRs built ~1.16M-line blocks against 481- and 1,476-line reviewed diffs. Every leg's prompt grew from ~98 KB to ~826 KB, four of six legs timed out, and the two that finished reviewed base-branch files. ## What changed **`.github/cursor-review/incremental-diff.py` (new).** `build --old --new --out` splits both patches into per-file sections on `^diff --git ` and emits every NEW section whose hunk content differs from the same file's section in OLD, plus every NEW section for a file OLD lacks. A file OLD had and NEW doesn't contributes nothing — it's no longer in the PR. `@@ -a,b +c,d @@` headers are normalised to `@@ @@` before comparison, so a pure rebase (identical hunks, shifted line numbers) produces an empty block. Lines that track the *base* blob rather than the branch's own edit — `index <blob>..<blob>`, the similarity percentages — are excluded for the same reason: they move whenever the base blob does. The pre-hunk metadata that does not (`old mode`/`new mode`, `rename from`/`rename to`, typechange) is compared, so a round that only adds the executable bit to an already-edited script still shows up; and a binary section keeps its `index` line, because without `--binary` the `Binary files a/X and b/X differ` line is a constant and `index` is the only content-dependent line git emits. Splitting is LF-only throughout — `str.splitlines` also breaks on `\v`, `\f`, `\x85`, U+2028 and friends, which would let a content line forge a `diff --git` section boundary. `check --new --full` is the fail-safe, returning the foreign-section count and both line counts. **The workflow step.** OLD is `git -c core.quotePath=false diff "${BASE_SHA}...${LAST_REVIEWED_SHA}" -- . $DIFF_EXCLUDES` — the `quotePath` flag matching `writeReviewedDiff`, which builds NEW, so a non-ASCII path keys the same on both sides. A build that fails, returns an EMPTY patch, or returns one over 32 MiB is declined rather than used. NEW is **`pr-diff.patch` itself** rather than a freshly computed `BASE...HEAD` diff — see the judgment call below. The helper is loaded from `_pr_size_tool/`, the pinned checkout of this repo the classifier is already built from, never the PR's own tree. After it runs, the step verifies the result and — if any section of the block is not carried **byte for byte** by `pr-diff.patch`, or the block has more lines than `pr-diff.patch` — truncates it to empty and emits `::warning::Incremental diff discarded: it was not a subset of the reviewed diff (<n> foreign file(s), <a> vs <b> lines).` The `Incremental diff since round N (<sha>): <n> lines` log line is unchanged, so the sweep that sized this problem can be re-run against it. **New `incremental_subset` output** on the `incremental` step and on the `diff-size` job. `true` when the block passed the check or when there is no block at all (round 1, an unreachable last-reviewed SHA, an empty reviewed diff, an over-cap run where the step never ran); `false` only when a block that had been built was discarded. Written as `!= 'false'` so a skipped step reads as `true` rather than `''`. **Docs.** Two paragraphs in the ledger section of `.github/cursor-review/README.md` plus a scripts-table row, and two in the Gotchas of `docs/callers/cursor-review.md` — both state that the block is derived from the two PR patches and can never carry a hunk the PR doesn't, and both quote the fail-safe warning verbatim and say what a caller seeing it should conclude (the panel ran on the full diff; nothing was skipped or suppressed). ## Judgment calls **NEW is `pr-diff.patch`, not a fresh `git diff BASE...HEAD`.** The plan I worked from specified the latter. `pr-diff.patch` is already exactly `git diff mergeBase...head -- . <excludes>` *minus the generated-file sections the classifier stripped* (`writeReviewedDiff`, `scripts/check-pr-size/main.go:166`), and using it buys two things a fresh diff doesn't. The subset property becomes structural — every emitted section is a verbatim byte-slice of the file the fail-safe compares against — instead of merely checked after the fact. And a generated file touched since the last round can't reappear in the block: with a fresh `BASE...HEAD` diff it would, it isn't in `pr-diff.patch`, the fail-safe would count it foreign, and the *entire* block would be discarded over a lockfile. The fail-safe is kept regardless, as defence in depth. **The two patches can resolve to different merge bases.** OLD resolves to `merge-base(BASE, LAST)`, NEW to `merge-base(BASE, HEAD)`, and a round that merged the base branch in moves the second one forward. A file both the base branch and the PR touched then reads as changed even if the author's own edit didn't move. That is over-inclusion strictly within the PR's own files — it costs a little prompt budget and can't break the subset property. Pinning both to one merge base would hide a hunk whose surrounding code really did change under it, which is worse. Noted in the step comment. **Every "couldn't build one" path leaves the block empty and `incremental_subset` `true`.** Nothing was discarded in those cases; there was simply nothing to prioritize, and the panel behaves as it does on round 1. Reporting `false` there would make the integrity signal fire on a non-event. A helper crash during `build` is the one in-between case: it logs its own `::warning::`, empties the block and leaves the output `true`, because again no block was discarded — one was never produced. **No `Skip-caller-bump: true` trailer.** This is a behavioural change inside a watched surface; the fleet should bump. ## Verification Reproduced against real git rather than fixtures. `TestMergeCommitHead` builds an actual repository — base branch advances by 400 lines plus a new file, the PR branch merges it in and edits its own file — and pins four things: the old formulation really does drag `unrelated.py` in (424 lines vs a 10-line reviewed diff), the new one emits only `app.py`, the result passes the subset check, and the old formulation would have *failed* it. I also extracted the step's `run:` block straight out of `cursor-review.yml` and ran it verbatim against that repository for every branch: merge-commit head (10-line block, `app.py` only, `incremental_subset=true`), a real `git rebase` onto the advanced base (empty block), round 1 / `LAST == HEAD` / unreachable LAST / empty reviewed diff (all skip cleanly), `diff_excludes` word-splitting through the new wrapper function, a missing helper, and a deliberately sabotaged builder that re-emitted the old commit-range diff — which produced exactly `::warning::Incremental diff discarded: it was not a subset of the reviewed diff (2 foreign file(s), 424 vs 10 lines).`, truncated the block to 0 lines, and appended `incremental_subset=false`. - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` (the command `test-cursor-review-scripts.yml` runs, and its `test_*.py` glob picks the new file up): **636 tests, OK** — 36 of them new. - `shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh`: clean. The new step's `run:` block, extracted and shellchecked separately: clean — the `elif` chain is why `build_old_patch` is a function, since a `# shellcheck disable=` directive is rejected in front of an `elif` branch. - `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 155 pinned refs. `.github/workflow-pins/tests`: 499 tests, OK. - `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings — AGENTS.md at 156 lines and no CODEOWNERS; neither touched here). - Other suites, to confirm nothing collateral: groom 405 OK, public-repo-hygiene 204 OK, refresh-reviewers 56 OK, agents-md-integrity OK, linear-ticket OK. - Negative-tested the new drift guard: pointing `INCREMENTAL_PY` at the PR's own checkout makes `test_the_helper_is_loaded_from_the_pinned_checkout_not_the_pr` fail as intended. - The new test file is stdlib-only (`contextlib`, `importlib`, `io`, `os`, `shutil`, `subprocess`, `tempfile`, `unittest`), matching the repo rule and the sibling workflow suites' explicit note that a `yaml` import would not run in this job. ## Residual **The `pytest` invocation named in the plan was not run.** The plan's verification step is `python3 -m pytest .github/cursor-review/tests/`; this host has neither `pytest` nor `pip`, so I ran the `unittest discover` command `test-cursor-review-scripts.yml` actually uses — which AGENTS.md names as the authority, and which is what CI will run. Every new test is a plain `unittest.TestCase`, so pytest would collect them identically, but I did not empirically confirm that. **The live end-to-end reproduction on a test PR was not performed.** The plan asks for it: open a PR here, trigger the review label, push a merge of `main` into the branch, re-trigger, and read the `diff-size` log. That needs two labelled review rounds against a real panel (billed `cursor-review` runs), so it is out of reach of a non-mutating check from this session. What I substituted is described under Verification: the step's own `run:` block, extracted verbatim from `cursor-review.yml`, executed against a real repository containing a real merge commit, for each of its branches. That exercises the same shell but not the surrounding job — in particular it does not prove that `_pr_size_tool/` is still populated at this point in a real run (a drift guard now asserts the path matches a declared checkout `path:`, which is the static half of that claim), nor that `needs.ledger.outputs.last_reviewed_sha` carries what this step assumes on a live PR. **This PR itself is the natural place to confirm it:** it is labelled `cursor-review`, so pushing a merge of `main` into this branch and re-triggering the label would produce exactly the log the plan asks for. **The evidence this change is built on was read, not re-measured.** The 9,800-line / ~1.16M-line / ~826 KB figures, the 119-of-133 foreign-file count and the four-of-six-legs-timed-out result come from a prior read-only investigation on a private consumer repository. I have no access to that repository or to those runs, so I could not re-derive any of those numbers; they are quoted in the step comment and the docs as measurements, and this PR does not independently corroborate them. The *mechanism* they describe is independently reproduced here in `TestMergeCommitHead`, which is the part the fix actually depends on. **Not swept: how many consumer PRs were affected.** The investigation this came from sized the problem on specific PRs; I did not and could not sweep the rest of the fleet's review history to count how many rounds shipped a non-subset block, because those runs and their logs are on repositories this session cannot read. Anyone with that access can now count them directly — the `incremental_subset` output and the discard warning make it observable going forward, but neither is retroactive. **A consumer already reading `diff-size` job outputs sees a new one.** `incremental_subset` is additive, so no existing caller breaks, but note that when the whole `diff-size` job is *skipped* (gate says don't run) the output is `''`, not `'true'` — the `!= 'false'` default only covers the skipped-*step* case within a running job. The sibling ticket that turns this into a check-run conclusion should treat `''` as "no panel ran" rather than as a verdict. **Two pre-existing `agents-md-integrity` warnings remain unfixed** and are unrelated to this change: `AGENTS.md` is 156 lines against an aspirational 150 (hard ceiling 200, so it passes), and the repo has no CODEOWNERS file, so `AGENTS.md` has no DRI. I deliberately did not add to `AGENTS.md` given the first. ## Review round The panel raised nine findings. Eight are fixed in `a2da829`; each was reproduced before it was fixed, and each has a regression test. - **An empty OLD fell through the fail-closed guard.** `! build_old_patch` only caught a non-zero exit. `BASE...LAST` comes back empty — with no error — 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, and the block then became a byte-for-byte copy of the whole reviewed diff: 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` rather than exceeding it. Guard is now `! build_old_patch || [ ! -s "$OLD_PATCH" ]`. - **OLD and NEW disagreed on `core.quotePath`.** NEW comes from check-pr-size's `-c core.quotePath=false` diff; OLD used the default, so a non-ASCII path arrived C-quoted on one side and plain on the other and the two could never key alike — re-emitting the file in full every round, including a pure rebase. The suite's own `_diff` helper already passed the flag, which is why nothing caught it. - **Pre-hunk metadata was dropped whenever the file also had hunks.** A round that only added `+x` to an already-edited script compared equal and vanished from the block. - **`str.splitlines` let the diff forge its own section boundaries.** `+x\x0cdiff --git a/lib/auth.py b/lib/auth.py` produced two sections, and `check` learned the forged path from the same bad split, so it did not count as foreign — the guard disarming itself. All splitting is LF-only now. - **A changed binary was silently dropped.** Excluding `index` is right for a text file and wrong for a binary, whose other lines are constant. The test that covered this pitted a `Binary files … differ` stanza against a `GIT binary patch` one — two formats one `git diff` invocation never mixes — so it passed while the gap was open. - **The fail-safe was weaker than the property the docs assert.** It compared path names; "can never contain a hunk the PR does not carry" needs bytes. A fabricated hunk under a path the PR *does* touch passed as a verified subset. `check` now requires every section to appear byte for byte in the reviewed diff, matched as a multiset. Both docs now describe it as the byte test it is. - **An ambiguous rename header keyed off the wrong split.** `a/x b/c b/d` reads two ways; `candidates[0]` took `('x', 'c b/d')`, which no later round's plain `diff --git a/d b/d` could match. A new `section_paths` prefers the section's own `rename from`/`rename to` lines — one path per line, so unambiguous. (The finding's first example, `a/old name.py b/new name.py`, in fact parses correctly today: the split needs a literal ` b/`.) - **OLD was unbounded.** NEW is capped by `diff_size_cap`, but `BASE...LAST` keeps the generated-file sections the classifier strips out of the reviewed diff, and those cost nothing against that cap. Now declined over 32 MiB, on the same fail-closed path as every other "can't build one" branch. **Deferred, with a follow-up filed: `BASE_SHA` is the current base, not the base the previous round diffed against.** After a retarget, `BASE...LAST_REVIEWED` can carry hunks the previous reviewed diff never showed, and this round then suppresses them — under-inclusion, the one direction the "different merge bases" judgment call above does *not* accept. It cannot be fixed here: the ledger records `last_reviewed_sha` and no base SHA, so fixing it means a per-round base SHA in `build-ledger.py` and in the ledger's rendered-and-reparsed comment format, plus a compatibility path for entries already live on open PRs. Blast radius is bounded — the block is only a prioritization hint and the panel always sees the full reviewed diff beside it, so no hunk goes unreviewed. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 657 passed, 0 failed (57 in `test_incremental_diff.py`, 21 added this round); `.github/workflow-pins/tests`: 499 passed, 0 failed; groom 405 / public-repo-hygiene 204 / linear-ticket 116 / refresh-reviewers 56 / agents-md-integrity 46 passed; `python3 .github/workflow-pins/check_workflow_pins.py`: OK (11 workflows, 155 pinned refs); `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed, 2 pre-existing warnings; `shellcheck -x` on the two cursor-review shell scripts and on the step's extracted `run:` block: clean, with its four guard branches (failed / empty / oversized / usable OLD) exercised directly; every one of the eight fixed findings reproduced against the pre-fix code before being fixed - **Deviations:** NEW is `pr-diff.patch` rather than a fresh `git diff BASE...HEAD` (rationale under Judgment calls); on the fail-safe finding I took byte-exact section matching rather than the narrower "require the NEW-side path to be carried, relaxing for a rename pair" the reviewer proposed — it subsumes that case and needs no rename special-casing; the base-drift finding is deferred to a follow-up rather than fixed (see Review round); `pytest` and the live two-round reproduction on a test PR were still not run (both under Residual) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:28 +00:00