feat(cursor-review): reuse check-pr-size classifier + discount comments in the size gate

.github/cursor-review/README.md

@@ -191,9 +191,12 @@ descriptions live in the [workflow header](../workflows/cursor-review.yml).
 | Input | Default | What it does |
 |---|---|---|
 | `judge_model` | `claude-opus-4-8-thinking-max` | Model that consolidates panel findings. |
-| `diff_size_cap` | `5000` | Max changed lines (after excludes); larger PRs are skipped. |
+| `diff_size_cap` | `5000` | Max counted changed lines (after generated-file exclusion and comment discounting); larger PRs are skipped. |
+| `ignore_comments` | `true` | Discount blank/comment-only lines from the size count (count-only; the panel still sees them). |
 | `review_label` | `cursor-review` | Label whose addition triggers the review. |
-| `diff_excludes` | lockfiles, `node_modules`, `dist`, `vendor`, minified/generated files | Pathspecs excluded from both the size count and the reviewed diff. |
+| `extra_generated_globs` | `node_modules`, `dist`, `vendor`, minified/`.generated.` files | Extra globs the shared `check-pr-size` classifier treats as generated — excluded from BOTH the size count and the reviewed diff. |
+| `extra_lockfiles` | `''` | Extra lockfile base names for the classifier, on top of its built-ins. |
+| `diff_excludes` | `''` | Pathspecs excluded from the reviewed diff ONLY (not the size count) — back-compat escape hatch; prefer `extra_generated_globs`. |
 | `workflows_ref` | `main` | Ref this directory's prompts/scripts are loaded from. Pin to your `uses:` SHA. |
 | `bot_app_id` | `''` | Optional GitHub App ID; when set (with `BOT_APP_PRIVATE_KEY`), the review posts under that App's identity instead of `github-actions[bot]`. |
 | `blocking` | `false` | Opt-in merge gate. `true` fails the **Blocking gate** check while any cursor-review finding thread is unresolved. See [Make the review blocking](#optional-make-the-review-blocking-merge-gate). |

README.md

@@ -9,7 +9,7 @@ This repo is **public** so any repo — public or private, inside or outside the
 | Workflow | Purpose |
 |---|---|
 | [`detect-unreviewed-merge.yml`](.github/workflows/detect-unreviewed-merge.yml) | SOC 2 compliance — detects PRs merged without prior approval and opens a tracking issue in [`Comfy-Org/unreviewed-merges`](https://github.com/Comfy-Org/unreviewed-merges). |
-| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default; opt in with `blocking: true` to fail a (required-status-check) gate while findings stay unresolved. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Self-hostable via `runs_on` (JSON, default `ubuntu-latest`) and panel models overridable via `models` (JSON array) for accounts lacking a default provider. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). |
+| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default; opt in with `blocking: true` to fail a (required-status-check) gate while findings stay unresolved. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Generated code is excluded from BOTH the diff-size gate and the diff sent to the panel via the shared [`check-pr-size`](scripts/check-pr-size) classifier (base-ref `linguist-generated`, base-blob Go generated markers — a PR cannot self-exempt a file from review, lockfiles, plus `extra_generated_globs` / `extra_lockfiles`) — the same detection as `pr-size.yml`, so codegen never blows the review cap or wastes review signal. Blank and comment-only lines are also discounted from the size count by default (`ignore_comments`) so AI comment volume doesn't trip the cap and skip the review — comments still reach the panel, only the count ignores them. Self-hostable via `runs_on` (JSON, default `ubuntu-latest`) and panel models overridable via `models` (JSON array) for accounts lacking a default provider. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). |
 | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. |
 | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. |
 | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. |

scripts/check-pr-size/main.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"flag"
@@ -40,6 +41,10 @@ func main() {
 	bypassLabel := flag.String("bypass-label", envStr("PR_SIZE_BYPASS_LABEL", defaultBypassLabel), "PR label name the report offers as the bypass")
 	extraLockfiles := flag.String("extra-lockfiles", os.Getenv("PR_SIZE_EXTRA_LOCKFILES"), "extra lockfile base names to exclude (whitespace/comma separated)")
 	extraGlobs := flag.String("extra-generated-globs", os.Getenv("PR_SIZE_EXTRA_GENERATED_GLOBS"), "extra glob patterns treated as generated (whitespace/comma separated)")
+	reviewedDiffOut := flag.String("reviewed-diff-out", "", "if set and the size check passes, also write the PR's unified diff with all generated-file sections excluded to this file (for a downstream review consumer, e.g. cursor-review); a failure writing it exits 2 so the consumer never pairs this run's counts with a missing/partial diff")
+	diffExcludes := flag.String("diff-excludes", "", "extra git pathspecs (whitespace-separated, passed to git verbatim) applied ONLY when building --reviewed-diff-out; never affects the size verdict")
+	markerFromBase := flag.Bool("marker-from-base", false, "read the Go generated-code marker from the BASE blob instead of the PR head, so a PR cannot self-exempt a file by adding the marker; files new in the PR then never match the marker (path- and attribute-based rules still apply), which is strictly conservative — set this when --reviewed-diff-out feeds a review that must not be evadable")
+	ignoreComments := flag.Bool("ignore-comments", envBool("PR_SIZE_IGNORE_COMMENTS"), "exclude blank and comment-only changed lines from the counted total (heuristic, count-only; never affects generated classification or --reviewed-diff-out)")
 	flag.Parse()
 
 	if *base == "" {
@@ -71,7 +76,13 @@ func main() {
 	attr := attrPolicy{source: *base, useSource: checkAttrSourceSupported(*base)}
 	attrModified := TouchesGitattributes(files)
 	attr.trusted = attrTrusted(attr.useSource, attrModified, *bypassFlag)
-	classify(files, *base, *head, attr, extras)
+	classify(files, *base, *head, attr, extras, *markerFromBase)
+
+	// Discount blank/comment-only changed lines from the count (opt-in). Runs
+	// after classify so it only ever inspects non-generated, non-binary files.
+	if *ignoreComments {
+		annotateDiscounts(files, *base, *head)
+	}
 
 	res := Evaluate(files, *maxFlag, *bypassFlag)
 	if !res.OK && !attr.trusted {
@@ -92,11 +103,93 @@ func main() {
 	report(res, *modeFlag, *bypassLabel)
 	writeGitHubOutputs(res)
 
+	// Emit the reviewed diff for a downstream consumer (the cursor-review
+	// workflow feeds it to the review panel + judge): the PR's full patch with
+	// every generated file's section filtered out, reusing THIS classification
+	// as the single source of truth for "what is codegen" — the count and the
+	// reviewed diff come from one process and can never disagree. Written last;
+	// a write failure exits 2 (after removing any partial file) so the consumer
+	// sees a degraded run rather than pairing this run's counts with a
+	// missing/truncated diff. Skipped when over the cap — nothing downstream
+	// consumes a diff the gate is about to skip.
+	if *reviewedDiffOut != "" {
+		if res.OK {
+			if err := writeReviewedDiff(*base, *head, strings.Fields(*diffExcludes), files, *reviewedDiffOut); err != nil {
+				os.Remove(*reviewedDiffOut)
+				fmt.Fprintf(os.Stderr, "check-pr-size: could not write --reviewed-diff-out %q: %v\n", *reviewedDiffOut, err)
+				os.Exit(2)
+			}
+		} else {
+			fmt.Fprintln(os.Stderr, "check-pr-size: over the cap — skipping --reviewed-diff-out")
+		}
+	}
+
 	if shouldFail(res, *modeFlag) {
 		os.Exit(1)
 	}
 }
 
+// writeReviewedDiff streams `git diff base...head` (plus any caller-supplied
+// verbatim exclude pathspecs) through FilterPatch, writing the patch minus
+// every generated file's section to outPath. The generated set is passed to
+// git by FILTERING ITS OUTPUT, never as per-file argv pathspecs, so an
+// unbounded number of generated files (they cost nothing against the cap)
+// cannot overflow ARG_MAX and fail the diff. core.quotePath=false keeps
+// non-ASCII paths verbatim in the headers so they match the numstat-derived
+// classification; a path git still quotes (control chars, quotes) simply won't
+// match and its section is kept — reviewed, never hidden.
+func writeReviewedDiff(base, head string, diffExcludes []string, files []FileChange, outPath string) error {
+	gen := make(map[string]bool)
+	for _, f := range files {
+		if f.Generated {
+			gen[f.Path] = true
+		}
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), gitTimeout)
+	defer cancel()
+	args := append([]string{"-c", "core.quotePath=false", "diff", base + "..." + head, "--", "."}, diffExcludes...)
+	cmd := exec.CommandContext(ctx, "git", args...)
+	cmd.WaitDelay = gitWaitDelay
+	errBuf := &capWriter{cap: maxStderrBytes}
+	cmd.Stderr = errBuf
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		return err
+	}
+	out, err := os.Create(outPath)
+	if err != nil {
+		return err
+	}
+	defer out.Close()
+	if err := cmd.Start(); err != nil {
+		return err
+	}
+	bw := bufio.NewWriter(out)
+	kept, dropped, ferr := FilterPatch(stdout, bw, func(p string) bool { return gen[p] })
+	if ferr != nil {
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+		return ferr
+	}
+	if err := cmd.Wait(); err != nil {
+		if ctx.Err() == context.DeadlineExceeded {
+			err = fmt.Errorf("git %v timed out after %s: %w", args, gitTimeout, ctx.Err())
+		}
+		if s := strings.TrimSpace(errBuf.String()); s != "" {
+			err = fmt.Errorf("%w: %s", err, s)
+		}
+		return err
+	}
+	if err := bw.Flush(); err != nil {
+		return err
+	}
+	if err := out.Close(); err != nil {
+		return err
+	}
+	fmt.Fprintf(os.Stderr, "check-pr-size: reviewed diff written to %s (%d file section(s) kept, %d generated section(s) excluded)\n", outPath, kept, dropped)
+	return nil
+}
+
 // shouldFail reports whether the process should exit non-zero: over the cap
 // (and not bypassed) in enforce mode. Warn mode never fails — it reports and
 // lets the workflow's comment job surface the overage.
@@ -114,6 +207,63 @@ func diffFiles(base, head string) ([]FileChange, error) {
 	return ParseNumstat(strings.NewReader(out))
 }
 
+// annotateDiscounts sets FileChange.Discounted for each non-generated, non-binary
+// file to the number of its changed lines that are blank or comment-only, so the
+// counted total reflects significant lines. Count-only: it never changes which
+// files are generated, nor any diff a consumer builds from --generated-out.
+//
+// Best-effort by design: on any git/parse failure it leaves Discounted at 0, so
+// that file counts raw — a failure can only make the cap STRICTER, never sneak a
+// large change under it. Restricts the patch to the counted paths, so a large
+// generated diff is never fetched or parsed.
+func annotateDiscounts(files []FileChange, base, head string) {
+	idx := make(map[string]*FileChange, len(files))
+	paths := make([]string, 0, len(files))
+	for i := range files {
+		f := &files[i]
+		if f.Generated || f.Binary {
+			continue
+		}
+		paths = append(paths, f.Path)
+		idx[f.Path] = f
+	}
+	if len(paths) == 0 {
+		return
+	}
+	patch, err := runGitDiffPatch(base, head, paths)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "check-pr-size: comment discounting skipped (git diff failed): %v\n", err)
+		return
+	}
+	discounts, err := ParseDiscounts(strings.NewReader(patch))
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "check-pr-size: comment discounting skipped (parse failed): %v\n", err)
+		return
+	}
+	for p, d := range discounts {
+		f, ok := idx[p]
+		if !ok {
+			continue // header path git quoted, or otherwise unmatched — count raw
+		}
+		if d > f.Changed() {
+			d = f.Changed() // clamp: never discount more than the file changed
+		}
+		f.Discounted = d
+	}
+}
+
+// runGitDiffPatch returns the unified-diff patch for the PR's net changes (three
+// dot) restricted to the given paths. core.quotePath=false keeps non-ASCII paths
+// unquoted so ParseDiscounts can read them back; the :(literal) pathspec magic
+// disables wildcards so a filename with glob metacharacters is matched literally.
+func runGitDiffPatch(base, head string, paths []string) (string, error) {
+	args := []string{"-c", "core.quotePath=false", "diff", base + "..." + head, "--"}
+	for _, p := range paths {
+		args = append(args, ":(literal)"+p)
+	}
+	return runGit(args...)
+}
+
 // attrPolicy controls how the linguist-generated git attribute is consulted.
 // source is the tree-ish whose .gitattributes rules are read (the base ref);
 // useSource is whether the installed git honors `check-attr --source` (git
@@ -141,28 +291,29 @@ func attrTrusted(useSource, attrModified, bypass bool) bool {
 // the linguist-generated git attribute (read from the base ref per attr), and
 // the canonical Go generated marker in the file's content.
 //
-// The linguist-generated attribute is resolved for every non-binary path in a
-// SINGLE `git check-attr` pass (attrGeneratedBatch) rather than one subprocess
-// per file, so a large PR pays constant process-creation cost instead of O(N).
-func classify(files []FileChange, base, head string, attr attrPolicy, extras Extras) {
+// Binary files are classified by the path- and attribute-based rules too (none
+// of those read content), so a binary lockfile or a binary matching an extra
+// glob still leaves a --reviewed-diff-out consumer's diff; only the content
+// marker read is skipped for them. Their Changed() is 0 either way, so the
+// size verdict is untouched.
+//
+// The linguist-generated attribute is resolved for every path in a SINGLE
+// `git check-attr` pass (attrGeneratedBatch) rather than one subprocess per
+// file, so a large PR pays constant process-creation cost instead of O(N).
+func classify(files []FileChange, base, head string, attr attrPolicy, extras Extras, markerFromBase bool) {
 	var attrGen map[string]bool
 	if attr.trusted {
 		paths := make([]string, 0, len(files))
 		for i := range files {
-			if !files[i].Binary {
-				paths = append(paths, files[i].Path)
-			}
+			paths = append(paths, files[i].Path)
 		}
 		attrGen = attrGeneratedBatch(paths, attr.source, attr.useSource)
 	}
 	for i := range files {
 		f := &files[i]
-		if f.Binary {
-			continue
-		}
 		if IsLockfile(f.Path) || extras.Generated(f.Path) ||
 			attrGen[f.Path] ||
-			contentGenerated(f.Path, base, head) {
+			(!f.Binary && contentGenerated(f.Path, base, head, markerFromBase)) {
 			f.Generated = true
 		}
 	}
@@ -258,10 +409,25 @@ func isUnknownFlagError(stderr string) bool {
 // non-Go file by pasting the marker at its top. Working-tree reads never follow a
 // symlink and are capped at maxScanBytes, so a PR cannot point a "generated" path
 // at an unbounded/blocking target (e.g. /dev/zero) to hang or OOM the job.
-func contentGenerated(path, base, head string) bool {
+//
+// With markerFromBase set, ONLY the base blob is consulted — mirroring the
+// base-ref invariant the linguist-generated attribute path already enforces.
+// The head-content read is fine for a size cap (self-marking merely shrinks a
+// count a maintainer can eyeball), but when the classification decides what a
+// blocking review SEES, a head-honored marker would let a PR hide arbitrary
+// code by prepending one comment line. Base-only means a file new in the PR
+// never matches the marker (it is counted and reviewed — conservative); files
+// generated at base stay excluded.
+func contentGenerated(path, base, head string, markerFromBase bool) bool {
 	if !strings.HasSuffix(path, ".go") {
 		return false
 	}
+	if markerFromBase {
+		if data, err := runGitCapped(maxScanBytes, "show", base+":"+path); err == nil {
+			return IsGeneratedContent(data)
+		}
+		return false
+	}
 	if info, err := os.Lstat(path); err == nil {
 		if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() {
 			return false
@@ -305,6 +471,9 @@ func renderReport(res Result, mode, bypassLabel string) string {
 	fmt.Fprintf(&b, "- Changed lines counted (non-generated): **%d**\n", res.Counted)
 	fmt.Fprintf(&b, "- Cap: **%d**\n", res.Max)
 	fmt.Fprintf(&b, "- Excluded (generated/lockfiles): %d\n", res.Generated)
+	if res.Discounted > 0 {
+		fmt.Fprintf(&b, "- Excluded (blank/comment lines): %d\n", res.Discounted)
+	}
 	if res.Bypassed {
 		fmt.Fprintf(&b, "- Bypassed via `%s` label ✅\n", bypassLabel)
 	}
@@ -325,7 +494,7 @@ func renderReport(res Result, mode, bypassLabel string) string {
 	shown := 0
 	var top strings.Builder
 	for _, f := range res.Files {
-		if f.Generated || f.Changed() == 0 {
+		if f.Generated || f.Counted() == 0 {
 			continue
 		}
 		if shown == 0 {