[PR #196] [MERGED] fix(pr-size): require a rename's SOURCE to be linguist-generated too (BE-8137) #205

New issue

Closed

opened 2026-10-26 22:37:20 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:20 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8137-rename-source-linguist-generated

📝 Commits (2)

2b2d253 fix(pr-size): require a rename's SOURCE to be linguist-generated too (BE-8137)
76bf67a fix(pr-size): pair renames in annotateDiscounts so the guard isn't cancelled (BE-8137)

📊 Changes

2 files changed (+211 additions, -4 deletions)

View changed files

📝 scripts/check-pr-size/main.go (+54 -4)
📝 scripts/check-pr-size/main_test.go (+157 -0)

📄 Description

ELI-5

The PR-size checker decides which files are "generated" so they skip both the line count and the review panel's patch. For a renamed file it asked "is the file's NEW path marked linguist-generated?" and nothing else. So you could git mv a hand-written file into a generated tree, edit it in the same commit, and the edit became invisible — it counted zero lines and never reached the reviewers. The three exclusion buckets sitting on the lines right next to it already asked about the OLD path too; this one just didn't. Now it does.

What changed

classify() in scripts/check-pr-size/main.go, two edits:

The batched git check-attr lookup now resolves a rename's OldPath as well as its Path, so the source path's attribute is actually available.
The attribute bucket now reads attrGen[f.Path] && (f.OldPath == "" || attrGen[f.OldPath]), mirroring the IsTestPath / IsLockfile / extras.Generated conjuncts immediately above it.

The classify() header comment's "a RENAME is classified on BOTH its old and new path for every exclusion bucket" list now names the attribute bucket, and spells out that this bucket's hole costs the reviewed diff as well as the count.

Chesterton's Fence

The destination-only form was an omission, not a design choice. git log -L on those lines lands on ab2066a (fix(pr-size): close the rename bypass on lockfiles too), which introduced the lockfile and extraGen both-paths conjuncts in exactly this if and left attrGen[f.Path] bare on the line between them. The change is also strictly narrowing — every case it newly declines to exclude gets counted and reviewed, never the reverse — so the failure direction stays the safe one. The base-ref invariant is untouched: attrGeneratedBatch still resolves via git check-attr -z --stdin --source <base>, which works for arbitrary path strings whether or not they exist at that ref, and a rename's source path does exist there.

Test

TestClassifyRenameIntoAttrGeneratedTreeStillCounts (new, main_test.go) builds a throwaway repo whose base carries gen/** linguist-generated=true, then in one head commit renames a hand-written file into gen/ with an injected edit and renames a genuinely generated file within gen/. It asserts, in that order: rename detection actually fired (OldPath is the source), the moved hand-written file is NOT Generated, the intra-gen/ rename still IS, Evaluate counts the injected line, the injected marker appears in the --reviewed-diff-out patch, and the generated rename does not.

I falsified the test three ways against the unfixed code before trusting it — reverting the attrGenOK conjunct alone, reverting the OldPath batch-lookup alone, and reverting both. Each revert fails the assertion it is supposed to guard, and reverting both reproduces the ticket's reported numbers exactly: Counted = 0, everything excluded, zero occurrences of the injected marker in the reviewed diff (0 file section(s) kept, 2 generated section(s) excluded). The OldPath batch-lookup edit is load-bearing on its own: without it attrGen[f.OldPath] is always false and legitimate moves inside a generated tree would start counting.

Sweep of the buckets this PR does NOT change

classify() makes five rename-sensitive decisions. Four were already sound and I left them alone: f.Test, lockfile and extraGen carry the explicit both-paths conjunct, and the content-marker bucket fails closed on renames by construction. I probed that last one rather than reasoning about it, since it was the only non-obvious case — with markerFromBase set it reads base:<destination>, which for a true rename does not exist, so it returns false and the file is counted. Review sharpened this: the fail-closed behavior holds only under --marker-from-base, which the sole --reviewed-diff-out call site (cursor-review.yml:672) does pass. Without the flag the head read can be self-marked — but that reduces to pasting the marker in place, needs no rename, and is the already-documented count-only tradeoff, so the bucket stays destination-only with the reasoning now written down. The adjacent worry (moving a hand-written .go on top of a file that was generated at base) is not a rename at all: git reports it as a separate modify plus delete with an empty OldPath, so the hand-written deletions stay booked against their own non-generated path and still count. One hole out of five, and it is the one fixed here.

Propagation

None by hand. main.go is inside the scripts/check-pr-size/** path filter of both bump-cursor-review-callers.yml and bump-pr-size-callers.yml, so merging fans SHA-bump PRs to the pinned callers automatically. The main_test.go half is stripped by the :(exclude)scripts/check-pr-size/*_test.go negation, which only strips the paths it matches — the non-test change still triggers the bump. No consumer's ci-cursor-review.yml pin is touched in this PR.

Judgment calls and unexercised artifacts

Out of scope, and the reason the ticket should stay open: initTestRepo (main_test.go) does not pin the throwaway repo's initial branch, so it inherits the host's init.defaultBranch. On any machine where that is main, TestResolveMergeBaseWithAdvancedBaseBranch fails with fatal: a branch named 'main' already exists — pre-existing on the base commit, unrelated to this change, and invisible in CI because the runner's git still defaults to master. I did not fix it: it means touching an unrelated test, and this diff is scoped to the reported defect. Because it is an unfixed defect on a file the ticket names, the reference below is Refs, not Closes.
Not exercised: the two short SHAs the report cites as carrying the bug are not fetchable from this remote (couldn't find remote ref), so I confirmed the bare attrGen[f.Path] || form on the current origin/main tip instead, where it was still present. The private consumer-repo PR the original reproduction ran against is in another repository and out of bounds for this change; nothing here relies on it, since the repro is reproduced in-repo by the new test. The tracking ticket's own thread and any linked material were not readable from this environment.
full-autonomy is deliberately absent: the source ticket does not carry it, and the label does not exist on this repo.

Review round

The panel raised 5 findings; 3 were fixed and 2 documented as residuals (76bf67a).

The one that mattered — the fix was being cancelled downstream. annotateDiscounts built its patch with a pathspec naming only the DESTINATION, so git could not pair a rename and re-emitted it as a whole-file addition. Every blank/comment line then became a discount and the d > Changed() clamp drove Counted() to 0. I reproduced it end to end before touching anything: with PR_SIZE_IGNORE_COMMENTS=1 (which cursor-review's ignore_comments input feeds), git mv handwritten/big.go gen/big.go plus a one-line edit reported Counted = 0 — i.e. the count half of this PR's guard was fully nullified for that caller. It now passes OldPath as an extra pathspec, mirroring the check-attr change, and the same run reports Counted = 1. Discounts stay keyed by destination, so an unpaired old-path section finds no entry and the file counts raw.

Test hardening. The two base files in the new attribute test were byte-identical, leaving gen/keep2.json's exact-rename pairing to git's hashmap tie-break — it could have paired with the hand-written source and inverted both OldPath assertions. They now carry distinct bodies. Separately the check-attr --source probe ran before t.Chdir, where base is not a valid object; the helper reports that class of failure as supported, so the skip could never fire and an old-git runner would have died on kept.Generated with a misleading message. Probe moved after the chdir.

Documented, not changed. The content-marker bucket is destination-only by design (fails closed under --marker-from-base; without it a rename grants nothing an in-place marker paste does not). And the both-paths rule inherits whatever pairing git chooses, which a same-content generated file planted at base can bias — a KNOWN LIMIT paragraph on classify now records it. Neither is reachable as a new defect under any current caller, so neither got a follow-up ticket.

Refs BE-8137

Provenance

Authored by: agent-work loop
Verified: gofmt -l . clean; go vet ./... clean; cd scripts/check-pr-size && go test ./... — ok, 0 failures (run with a neutral GIT_CONFIG_GLOBAL, see the out-of-scope note above). Falsification: the original fix reverted three ways (each partial revert fails the assertion it guards), and the new TestAnnotateDiscountsPairsRenamesSoTheEditStillCounts re-run against the reverted annotateDiscounts change, which fails with Discounted = 1, want 0 and Counted = 0, want 1. Both the original defect and the discount cancellation were also reproduced end to end through the built binary against a throwaway repo, before and after each fix.
Deviations: none against the plan — both classify() edits, the header-comment update, and the test are implemented. The review round then added one behavior fix outside the original plan (annotateDiscounts rename pairing), taken on because it silently cancelled this PR's own fix rather than being adjacent cleanup. Two review findings were answered with documentation rather than code, each with the reasoning on its thread. The unrelated initTestRepo branch-name fragility described above remains a deliberate omission.

_{🔄 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/196 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8137-rename-source-linguist-generated` --- ### 📝 Commits (2) - [`2b2d253`](https://github.com/Comfy-Org/github-workflows/commit/2b2d253c9afeb239958ee8a5cb9be2cc8ce0cb1f) fix(pr-size): require a rename's SOURCE to be linguist-generated too (BE-8137) - [`76bf67a`](https://github.com/Comfy-Org/github-workflows/commit/76bf67ab10448b8f4d0245781c02edb1281b2b81) fix(pr-size): pair renames in annotateDiscounts so the guard isn't cancelled (BE-8137) ### 📊 Changes **2 files changed** (+211 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `scripts/check-pr-size/main.go` (+54 -4) 📝 `scripts/check-pr-size/main_test.go` (+157 -0) </details> ### 📄 Description ## ELI-5 The PR-size checker decides which files are "generated" so they skip both the line count and the review panel's patch. For a renamed file it asked "is the file's NEW path marked `linguist-generated`?" and nothing else. So you could `git mv` a hand-written file into a generated tree, edit it in the same commit, and the edit became invisible — it counted zero lines and never reached the reviewers. The three exclusion buckets sitting on the lines right next to it already asked about the OLD path too; this one just didn't. Now it does. ## What changed `classify()` in `scripts/check-pr-size/main.go`, two edits: 1. The batched `git check-attr` lookup now resolves a rename's `OldPath` as well as its `Path`, so the source path's attribute is actually available. 2. The attribute bucket now reads `attrGen[f.Path] && (f.OldPath == "" || attrGen[f.OldPath])`, mirroring the `IsTestPath` / `IsLockfile` / `extras.Generated` conjuncts immediately above it. The `classify()` header comment's "a RENAME is classified on BOTH its old and new path for every exclusion bucket" list now names the attribute bucket, and spells out that this bucket's hole costs the reviewed diff as well as the count. ## Chesterton's Fence The destination-only form was an omission, not a design choice. `git log -L` on those lines lands on `ab2066a` (`fix(pr-size): close the rename bypass on lockfiles too`), which introduced the `lockfile` and `extraGen` both-paths conjuncts in exactly this `if` and left `attrGen[f.Path]` bare on the line between them. The change is also strictly narrowing — every case it newly declines to exclude gets counted and reviewed, never the reverse — so the failure direction stays the safe one. The base-ref invariant is untouched: `attrGeneratedBatch` still resolves via `git check-attr -z --stdin --source <base>`, which works for arbitrary path strings whether or not they exist at that ref, and a rename's source path does exist there. ## Test `TestClassifyRenameIntoAttrGeneratedTreeStillCounts` (new, `main_test.go`) builds a throwaway repo whose base carries `gen/** linguist-generated=true`, then in one head commit renames a hand-written file into `gen/` with an injected edit and renames a genuinely generated file within `gen/`. It asserts, in that order: rename detection actually fired (`OldPath` is the source), the moved hand-written file is NOT `Generated`, the intra-`gen/` rename still IS, `Evaluate` counts the injected line, the injected marker appears in the `--reviewed-diff-out` patch, and the generated rename does not. I falsified the test three ways against the unfixed code before trusting it — reverting the `attrGenOK` conjunct alone, reverting the `OldPath` batch-lookup alone, and reverting both. Each revert fails the assertion it is supposed to guard, and reverting both reproduces the ticket's reported numbers exactly: `Counted = 0`, everything excluded, zero occurrences of the injected marker in the reviewed diff (`0 file section(s) kept, 2 generated section(s) excluded`). The `OldPath` batch-lookup edit is load-bearing on its own: without it `attrGen[f.OldPath]` is always false and legitimate moves inside a generated tree would start counting. ## Sweep of the buckets this PR does NOT change `classify()` makes five rename-sensitive decisions. Four were already sound and I left them alone: `f.Test`, `lockfile` and `extraGen` carry the explicit both-paths conjunct, and the content-marker bucket fails closed on renames by construction. I probed that last one rather than reasoning about it, since it was the only non-obvious case — with `markerFromBase` set it reads `base:<destination>`, which for a true rename does not exist, so it returns false and the file is counted. Review sharpened this: the fail-closed behavior holds only *under* `--marker-from-base`, which the sole `--reviewed-diff-out` call site (`cursor-review.yml:672`) does pass. Without the flag the head read can be self-marked — but that reduces to pasting the marker in place, needs no rename, and is the already-documented count-only tradeoff, so the bucket stays destination-only with the reasoning now written down. The adjacent worry (moving a hand-written `.go` on top of a file that was generated at base) is not a rename at all: git reports it as a separate modify plus delete with an empty `OldPath`, so the hand-written deletions stay booked against their own non-generated path and still count. One hole out of five, and it is the one fixed here. ## Propagation None by hand. `main.go` is inside the `scripts/check-pr-size/**` path filter of both `bump-cursor-review-callers.yml` and `bump-pr-size-callers.yml`, so merging fans SHA-bump PRs to the pinned callers automatically. The `main_test.go` half is stripped by the `:(exclude)scripts/check-pr-size/*_test.go` negation, which only strips the paths it matches — the non-test change still triggers the bump. No consumer's `ci-cursor-review.yml` pin is touched in this PR. ## Judgment calls and unexercised artifacts - **Out of scope, and the reason the ticket should stay open:** `initTestRepo` (`main_test.go`) does not pin the throwaway repo's initial branch, so it inherits the host's `init.defaultBranch`. On any machine where that is `main`, `TestResolveMergeBaseWithAdvancedBaseBranch` fails with `fatal: a branch named 'main' already exists` — pre-existing on the base commit, unrelated to this change, and invisible in CI because the runner's git still defaults to `master`. I did not fix it: it means touching an unrelated test, and this diff is scoped to the reported defect. Because it is an unfixed defect on a file the ticket names, the reference below is `Refs`, not `Closes`. - **Not exercised:** the two short SHAs the report cites as carrying the bug are not fetchable from this remote (`couldn't find remote ref`), so I confirmed the bare `attrGen[f.Path] ||` form on the current `origin/main` tip instead, where it was still present. The private consumer-repo PR the original reproduction ran against is in another repository and out of bounds for this change; nothing here relies on it, since the repro is reproduced in-repo by the new test. The tracking ticket's own thread and any linked material were not readable from this environment. - `full-autonomy` is deliberately absent: the source ticket does not carry it, and the label does not exist on this repo. ## Review round The panel raised 5 findings; 3 were fixed and 2 documented as residuals (76bf67a). **The one that mattered — the fix was being cancelled downstream.** `annotateDiscounts` built its patch with a pathspec naming only the DESTINATION, so git could not pair a rename and re-emitted it as a whole-file addition. Every blank/comment line then became a discount and the `d > Changed()` clamp drove `Counted()` to 0. I reproduced it end to end before touching anything: with `PR_SIZE_IGNORE_COMMENTS=1` (which `cursor-review`'s `ignore_comments` input feeds), `git mv handwritten/big.go gen/big.go` plus a one-line edit reported `Counted = 0` — i.e. the count half of this PR's guard was fully nullified for that caller. It now passes `OldPath` as an extra pathspec, mirroring the check-attr change, and the same run reports `Counted = 1`. Discounts stay keyed by destination, so an unpaired old-path section finds no entry and the file counts raw. **Test hardening.** The two base files in the new attribute test were byte-identical, leaving `gen/keep2.json`'s exact-rename pairing to git's hashmap tie-break — it could have paired with the hand-written source and inverted both `OldPath` assertions. They now carry distinct bodies. Separately the `check-attr --source` probe ran *before* `t.Chdir`, where `base` is not a valid object; the helper reports that class of failure as supported, so the skip could never fire and an old-git runner would have died on `kept.Generated` with a misleading message. Probe moved after the chdir. **Documented, not changed.** The content-marker bucket is destination-only by design (fails closed under `--marker-from-base`; without it a rename grants nothing an in-place marker paste does not). And the both-paths rule inherits whatever pairing git chooses, which a same-content generated file planted at base can bias — a `KNOWN LIMIT` paragraph on `classify` now records it. Neither is reachable as a new defect under any current caller, so neither got a follow-up ticket. Refs BE-8137 ## Provenance - **Authored by:** agent-work loop - **Verified:** `gofmt -l .` clean; `go vet ./...` clean; `cd scripts/check-pr-size && go test ./...` — ok, 0 failures (run with a neutral `GIT_CONFIG_GLOBAL`, see the out-of-scope note above). Falsification: the original fix reverted three ways (each partial revert fails the assertion it guards), and the new `TestAnnotateDiscountsPairsRenamesSoTheEditStillCounts` re-run against the reverted `annotateDiscounts` change, which fails with `Discounted = 1, want 0` and `Counted = 0, want 1`. Both the original defect and the discount cancellation were also reproduced end to end through the built binary against a throwaway repo, before and after each fix. - **Deviations:** none against the plan — both `classify()` edits, the header-comment update, and the test are implemented. The review round then added one behavior fix outside the original plan (`annotateDiscounts` rename pairing), taken on because it silently cancelled this PR's own fix rather than being adjacent cleanup. Two review findings were answered with documentation rather than code, each with the reasoning on its thread. The unrelated `initTestRepo` branch-name fragility described above remains a deliberate omission. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:20 +00:00