From 4077d73d2925cfc88487ee767c90d2df450c60e0 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 18:25:44 -0700 Subject: [PATCH 1/3] feat(cursor-review): reuse check-pr-size classifier to skip codegen in the size gate and the reviewed diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cursor-review kept its own hand-maintained diff_excludes pathspec list to decide what codegen to skip — weaker than, and drifting from, the PR-size cap's classifier. It matched filename globs only, so generated files caught by the cap's base-ref linguist-generated attribute or the canonical Go `// Code generated ... DO NOT EDIT.` marker still counted toward the review size cap and got fed to the panel, while blunt directory excludes hid hand-written files that should have been reviewed. Make the shared scripts/check-pr-size tool the single source of truth for "what is codegen" in cursor-review too: - check-pr-size gains an opt-in --generated-out flag that writes the classified generated paths (NUL-separated). The size verdict is unchanged; pr-size.yml is unaffected (additive, default off). - diff-size builds and runs the tool once (GOWORK=off, mirroring pr-size.yml): its over_cap output (counted vs --max = DIFF_SIZE_CAP) drives within_cap, and the generated-paths list drives a single reviewed-diff artifact that excludes every classified generated file. - the panel cells and the judge consume that one artifact instead of each regenerating the diff, so they need no PR checkout — the only PR-authored content they see is the diff text they review. - new extra_generated_globs / extra_lockfiles inputs mirror pr-size and feed the classifier (count + diff). diff_excludes stays as a diff-only, back-compat escape hatch; its old lockfile/vendored default moves into the classifier and extra_generated_globs. Fails open: if the tool errors, over_cap is unset (within_cap=true) and the diff falls back to diff_excludes-only — never a silent skip. Callers that listed generated paths in diff_excludes should move them to extra_generated_globs to keep them out of the size gate too. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/cursor-review.yml | 255 ++++++++++++++++++++-------- README.md | 2 +- scripts/check-pr-size/main.go | 30 ++++ scripts/check-pr-size/main_test.go | 53 ++++++ 4 files changed, 266 insertions(+), 74 deletions(-) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 31cb62f..6d43dc9 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -26,10 +26,14 @@ name: Cursor Review (reusable) # cursor-review: # uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@main # with: -# # Repo-specific pathspecs excluded from the size cap and the diff. -# diff_excludes: >- -# :!**/package-lock.json -# :!data/object_info.json.gz +# # Repo-specific generated/vendored paths. Fed to the shared +# # check-pr-size classifier, so they're excluded from BOTH the size gate +# # and the reviewed diff — the same codegen detection the PR-size cap +# # uses (lockfiles, base-ref linguist-generated, Go markers are already +# # built in). Use diff_excludes only for diff-only, non-generated paths. +# extra_generated_globs: >- +# data/object_info.json.gz +# **/*.snap # # Pin the assets ref to the same ref you pin `uses:` to for # # reproducibility (defaults to main). # workflows_ref: main @@ -66,29 +70,56 @@ on: type: string required: false default: cursor-review - diff_excludes: + extra_generated_globs: description: >- - Whitespace-separated git pathspecs excluded from BOTH the size-budget - count and the diff fed to Cursor. Pass your repo's generated/vendored - paths (lockfiles, snapshots, fixtures) so they don't blow the cap or - waste review signal. Folded scalar collapses newlines to spaces so the - value word-splits into pathspec args. Paths with a leading underscore - need the long-form `:(exclude)` syntax — the short-form `:!` parser - reads `_` as pathspec magic. + Extra glob patterns treated as generated, fed to the shared + check-pr-size classifier. Matching files are excluded from BOTH the + diff-size gate AND the diff handed to the review panel + judge — the + exact same "what is codegen" logic the PR-size cap uses, so the two + workflows can't drift. `*` matches within a path segment, `**` across + segments, `?` one character; a pattern without `/` matches the base + name at any depth. This is the preferred knob for generated/vendored + paths: the classifier already covers dependency lockfiles, files + marked `linguist-generated` in the BASE ref's `.gitattributes` (so a + PR can't exempt itself), and the canonical Go + `// Code generated ... DO NOT EDIT.` marker — so you rarely need more. type: string required: false default: >- - :!**/package-lock.json - :!**/yarn.lock - :!**/pnpm-lock.yaml - :!**/go.sum - :!**/node_modules/** - :!**/.claude/** - :!**/dist/** - :!**/vendor/** - :!**/*.generated.* - :!**/*.min.js - :!**/*.min.css + **/node_modules/** + **/.claude/** + **/dist/** + **/vendor/** + **/*.generated.* + **/*.min.js + **/*.min.css + extra_lockfiles: + description: >- + Extra dependency-lockfile base names to exclude (count + diff), on top + of the classifier's built-ins (go.sum, go.work.sum, package-lock.json, + pnpm-lock.yaml, yarn.lock, Cargo.lock, poetry.lock, uv.lock). + Whitespace- or comma-separated; matched at any directory depth. + type: string + required: false + default: '' + diff_excludes: + description: >- + Additional git pathspecs excluded from the reviewed diff ONLY (not the + size gate), applied verbatim ON TOP of the shared classifier's + generated-file exclusion. Back-compat escape hatch for one-off paths + that aren't "generated" per se; for generated/vendored paths prefer + `extra_generated_globs`, which also keeps them out of the size gate. + Folded scalar collapses newlines to spaces so the value word-splits + into pathspec args. Paths with a leading underscore need the long-form + `:(exclude)` syntax — the short-form `:!` parser reads `_` as pathspec + magic. (Previously this fed both the count and the diff and defaulted + to the lockfile/vendored list; that list now lives in the classifier + and `extra_generated_globs`, so the default here is empty. Callers that + listed generated paths here should move them to `extra_generated_globs` + to keep them out of the size gate too.) + type: string + required: false + default: '' workflows_ref: description: >- Ref of Comfy-Org/github-workflows to load the prompts and scripts @@ -137,6 +168,8 @@ env: REVIEW_LABEL: ${{ inputs.review_label }} JUDGE_MODEL: ${{ inputs.judge_model }} DIFF_EXCLUDES: ${{ inputs.diff_excludes }} + EXTRA_GENERATED_GLOBS: ${{ inputs.extra_generated_globs }} + EXTRA_LOCKFILES: ${{ inputs.extra_lockfiles }} # Where the assets checkout lands (this repo, .github/cursor-review/*). CURSOR_REVIEW_ASSETS: ${{ github.workspace }}/_cursor_review_assets/.github/cursor-review @@ -256,38 +289,128 @@ jobs: runs-on: ubuntu-latest permissions: contents: read + # within_cap is derived from the shared check-pr-size tool's `over_cap` + # output: counted = non-generated changed lines, compared against the review + # cap passed as --max. If the tool fails operationally, over_cap is unset and + # this evaluates true — falling open to "review it" rather than silently + # skipping a PR because the classifier hiccuped. outputs: - within_cap: ${{ steps.count.outputs.within_cap }} + within_cap: ${{ steps.check.outputs.over_cap != 'true' }} steps: - - name: Checkout PR repo - uses: actions/checkout@v6 + - name: Checkout PR head + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: + # Full history so the BASE...HEAD three-dot diff resolves and the + # tool's base-ref reads (linguist-generated via `check-attr --source`, + # deleted-file blob fallbacks) work without a separate fetch. fetch-depth: 0 persist-credentials: false + ref: ${{ github.event.pull_request.head.sha }} + + - 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 + # workflows_ref), never the PR checkout, so a malicious PR can't rewrite + # the logic that decides which of its files skip review. + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + repository: Comfy-Org/github-workflows + ref: ${{ inputs.workflows_ref }} + path: _pr_size_tool + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: _pr_size_tool/scripts/check-pr-size/go.mod + # Dependency-free module (no go.sum) — nothing to cache. + cache: false - - name: Count changed lines - id: count + - name: Build check-pr-size + env: + # See pr-size.yml: the tool is checked out UNDER the consumer repo's + # checkout, so a root go.work would otherwise refuse the build. The + # tool is dependency-free; disable workspace mode and build standalone. + # Quoted "off" — unquoted off is coerced to GOWORK=false by the Actions + # YAML parser, which Go then rejects as a bad workspace-file path. + GOWORK: "off" + run: | + # Drop anything the PR checkout could have pre-seeded under the tool + # path so the build compiles only the pinned sources. + git -C _pr_size_tool clean -ffdx + (cd _pr_size_tool/scripts/check-pr-size && go build -o "${RUNNER_TEMP}/check-pr-size" .) + + - name: Classify generated files and check size + id: check env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - # numstat reports "-\t-" for binary files; treat those as 0 lines so - # binary-only PRs can't bypass the size cap by producing no insertion - # tokens for grep to count. - # $DIFF_EXCLUDES is intentionally unquoted: bash word-splits it into - # separate pathspec args. Pathspecs all start with ":!", so pathname - # expansion finds no matches and leaves them literal. - # shellcheck disable=SC2086 - TOTAL=$(git diff --numstat "$BASE_SHA...$HEAD_SHA" -- . $DIFF_EXCLUDES \ - | awk '{ if ($1 != "-") added += $1; if ($2 != "-") removed += $2 } END { print (added + removed) + 0 }') - echo "Changed lines: $TOTAL (cap: $DIFF_SIZE_CAP)" + # Reuse the PR-size classifier as the single source of truth. --max is + # THIS review's cap (DIFF_SIZE_CAP), so the tool's `over_cap` output + # means "non-generated changed lines exceed the review cap" — exactly + # the gate we want. --mode warn: the tool never exits non-zero on + # overage (this workflow gates via within_cap, not the tool's own + # pass/fail). The same run writes the classified generated paths to + # --generated-out for the diff step below, so classification happens + # ONCE and the count and the reviewed diff can never disagree. + # + # An operational failure (exit 2 — bad ref, git error) leaves over_cap + # unset → within_cap evaluates true → the diff step, finding no + # generated-paths file, falls back to diff_excludes-only (the pre-tool + # behavior). Fail open to "review", never silently skip. + rc=0 + "${RUNNER_TEMP}/check-pr-size" \ + --base "${BASE_SHA}" \ + --head "${HEAD_SHA}" \ + --max "${DIFF_SIZE_CAP}" \ + --mode warn \ + --extra-generated-globs "${EXTRA_GENERATED_GLOBS}" \ + --extra-lockfiles "${EXTRA_LOCKFILES}" \ + --generated-out "${RUNNER_TEMP}/generated-paths" || rc=$? + if [ "$rc" -ne 0 ]; then + echo "::warning::check-pr-size exited ${rc} — reviewing with no generated-file exclusions (diff_excludes still apply)." + rm -f "${RUNNER_TEMP}/generated-paths" + fi - if [ "$TOTAL" -gt "$DIFF_SIZE_CAP" ]; then - echo "Diff exceeds cap — skipping review." - echo "within_cap=false" >> "$GITHUB_OUTPUT" - else - echo "within_cap=true" >> "$GITHUB_OUTPUT" + - name: Build reviewed diff (generated files excluded) + # Skip when over cap: the review is skipped anyway, so don't spend time + # rendering (and uploading) a diff nothing will consume. + if: steps.check.outputs.over_cap != 'true' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + # Exclude every path the classifier flagged generated, plus any caller + # diff_excludes pathspecs (applied verbatim, back-compat). + # :(exclude,literal) disables pathspec magic so a path with + # metacharacters matches literally. An absent/empty generated-paths + # file (tool degraded, or nothing generated) leaves EXCLUDE_ARGS empty, + # so the diff is just the full diff minus diff_excludes. + EXCLUDE_ARGS=() + if [ -s "${RUNNER_TEMP}/generated-paths" ]; then + while IFS= read -r -d '' p; do + if [ -n "$p" ]; then EXCLUDE_ARGS+=(":(exclude,literal)${p}"); fi + done < "${RUNNER_TEMP}/generated-paths" fi + echo "Excluding ${#EXCLUDE_ARGS[@]} generated file(s) classified by check-pr-size." + # $DIFF_EXCLUDES is intentionally unquoted so bash word-splits it into + # pathspec args; the tool-derived excludes are a quoted array. + # shellcheck disable=SC2086 + git diff "${BASE_SHA}...${HEAD_SHA}" -- . $DIFF_EXCLUDES "${EXCLUDE_ARGS[@]}" \ + > "${RUNNER_TEMP}/pr-diff.patch" + echo "Reviewed diff size: $(wc -l < "${RUNNER_TEMP}/pr-diff.patch") lines" + + - name: Upload reviewed diff + # Shared with the panel cells + judge so the diff is generated ONCE, and + # those jobs need no PR checkout of their own. + if: steps.check.outputs.over_cap != 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: pr-diff + path: ${{ runner.temp }}/pr-diff.patch + if-no-files-found: error + retention-days: 7 preflight: # Validate the panel's pinned model ids against the LIVE Cursor catalog @@ -450,12 +573,6 @@ jobs: json.dump(record, f) PY - - name: Checkout PR repo - uses: actions/checkout@v6 - with: - fetch-depth: 0 - persist-credentials: false - - name: Load cursor-review assets # Trusted prompts/scripts come from THIS workflow's repo (public, # pinned via workflows_ref) — never from the PR checkout, so a @@ -468,16 +585,14 @@ jobs: path: _cursor_review_assets persist-credentials: false - - name: Generate diff - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: | - # See diff_excludes input for the rationale and pathspec list. - # shellcheck disable=SC2086 - git diff "$BASE_SHA...$HEAD_SHA" -- . $DIFF_EXCLUDES > /tmp/pr-diff.patch - - echo "Diff size: $(wc -l < /tmp/pr-diff.patch) lines" + - name: Download reviewed diff + # The generated-file-excluded diff is built ONCE in the diff-size job and + # shared here, so this cell needs no PR checkout of its own — the only + # PR-authored content it ever sees is the diff text it reviews. + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: pr-diff + path: /tmp - name: Install Cursor agent CLI run: | @@ -581,12 +696,6 @@ jobs: contents: read pull-requests: write steps: - - name: Checkout PR repo - uses: actions/checkout@v6 - with: - fetch-depth: 0 - persist-credentials: false - - name: Load cursor-review assets uses: actions/checkout@v6 with: @@ -595,14 +704,14 @@ jobs: path: _cursor_review_assets persist-credentials: false - - name: Generate diff - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: | - # See diff_excludes input for the rationale and pathspec list. - # shellcheck disable=SC2086 - git diff "$BASE_SHA...$HEAD_SHA" -- . $DIFF_EXCLUDES > /tmp/pr-diff.patch + - name: Download reviewed diff + # Same generated-file-excluded diff the panel reviewed, built once in + # diff-size — so the judge adjudicates against exactly what the panel + # saw, and this job needs no PR checkout. + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: pr-diff + path: /tmp - name: Install Cursor agent CLI run: | diff --git a/README.md b/README.md index dbebc8e..a05099f 100644 --- a/README.md +++ b/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`, Go generated markers, 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. 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`. | diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 753a814..9022a59 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -40,6 +40,7 @@ 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)") + generatedOut := flag.String("generated-out", "", "if set, also write the NUL-separated list of paths classified as generated to this file (for a downstream consumer to exclude, e.g. the cursor-review diff); does not change the size verdict") flag.Parse() if *base == "" { @@ -92,11 +93,40 @@ func main() { report(res, *modeFlag, *bypassLabel) writeGitHubOutputs(res) + // Emit the classified generated paths for a downstream consumer (the + // cursor-review workflow excludes them from the diff it feeds the review + // models, reusing THIS classifier as the single source of truth for "what + // is codegen"). Written last and best-effort: a write failure is logged but + // never changes the size verdict already reported above, nor fails a check + // that would otherwise pass — the consumer degrades to no exclusions. + if *generatedOut != "" { + if err := writeGeneratedPaths(files, *generatedOut); err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: could not write --generated-out %q: %v\n", *generatedOut, err) + } + } + if shouldFail(res, *modeFlag) { os.Exit(1) } } +// writeGeneratedPaths writes every path classified as generated to path, +// NUL-separated with a trailing NUL after each entry. NUL matches the separator +// git's `-z` numstat/check-attr use and round-trips paths containing spaces or +// newlines (which the -z numstat parser already admits), so a consumer can read +// it back with a NUL-delimited read loop and no quoting hazards. An empty +// generated set writes a zero-byte file — a consumer reads it as "exclude nothing". +func writeGeneratedPaths(files []FileChange, path string) error { + var b bytes.Buffer + for _, f := range files { + if f.Generated { + b.WriteString(f.Path) + b.WriteByte(0) + } + } + return os.WriteFile(path, b.Bytes(), 0o644) +} + // 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. diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index fb195bd..48481c8 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -306,6 +306,59 @@ func TestClassifyHonorsLegitBaseGitattributes(t *testing.T) { } } +// TestWriteGeneratedPaths proves --generated-out serialization: exactly the +// paths marked Generated are emitted, NUL-separated (so a name with spaces or a +// newline round-trips), hand-written paths are omitted, and an all-hand-written +// set yields a zero-byte file (which the cursor-review consumer reads as +// "exclude nothing"). +func TestWriteGeneratedPaths(t *testing.T) { + t.Parallel() + files := []FileChange{ + {Path: "api/service.pb.go", Generated: true}, + {Path: "internal/hand.go", Generated: false}, + {Path: "weird name\nwith newline.json", Generated: true}, + {Path: "web/vendor/lib.js", Generated: true}, + } + out := filepath.Join(t.TempDir(), "gen-paths") + if err := writeGeneratedPaths(files, out); err != nil { + t.Fatalf("writeGeneratedPaths: %v", err) + } + data, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + // Split on NUL; the trailing NUL after the final path yields an empty tail + // element, which the filter drops — mirroring how the consumer reads it. + var got []string + for _, p := range strings.Split(string(data), "\x00") { + if p != "" { + got = append(got, p) + } + } + want := []string{"api/service.pb.go", "weird name\nwith newline.json", "web/vendor/lib.js"} + if len(got) != len(want) { + t.Fatalf("got %d paths %q, want %d %q", len(got), got, len(want), want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("path[%d] = %q, want %q", i, got[i], want[i]) + } + } + + // No generated files → a zero-byte file, not a missing one. + empty := filepath.Join(t.TempDir(), "empty") + if err := writeGeneratedPaths([]FileChange{{Path: "only-hand.go"}}, empty); err != nil { + t.Fatalf("writeGeneratedPaths (empty set): %v", err) + } + info, err := os.Stat(empty) + if err != nil { + t.Fatalf("stat empty output: %v", err) + } + if info.Size() != 0 { + t.Errorf("empty generated set should write a zero-byte file, got size %d", info.Size()) + } +} + // TestClassifyAppliesExtras proves the per-repo extras exclude matching files // without any git attribute or content marker involved. Non-.go paths are used // so contentGenerated never consults git, and attr.trusted is false so the From 3bfe469263b186875ec74f401e90becc68462819 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 20:00:09 -0700 Subject: [PATCH 2/3] feat(check-pr-size): discount blank/comment lines from the size count (opt-in) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AI-generated code now leaves a non-trivial volume of comments, which inflate the changed-line count and can push a PR over the review cap — skipping the review of what may be modest actual logic. Add an opt-in `--ignore-comments` flag (env PR_SIZE_IGNORE_COMMENTS) that excludes blank and comment-only changed lines from the counted total: - annotateDiscounts fetches the patch for the counted (non-generated, non-binary) paths only, and ParseDiscounts tallies each file's blank/comment-only added and removed lines. FileChange.Discounted carries the per-file total; Counted() = Changed() - Discounted (clamped ≥ 0). Default 0, so behavior is unchanged when the flag is off — Counted() == Changed() for every existing path and test. - Heuristic and count-only by design: a per-language, single-line comment-marker table (C-family //, hash #, SQL/Lua --, HTML ) plus single-line block comments and blank lines. Multi-line block-comment bodies and string literals are NOT lexed (documented limitation). A line whose content merely CONTAINS a comment token (e.g. a string literal) still counts. Generated files are never inspected, and the reviewed diff is untouched — only the count changes. - Fails safe: any git/parse error leaves Discounted at 0 (count raw), so a bug can only make the cap stricter, never sneak a large change under it. cursor-review enables it via a new `ignore_comments` input (default true): AI comment volume no longer trips the diff-size gate and skips the review, but the panel still sees the comments — so it can still flag wrong or misleading ones. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/cursor-review.yml | 21 +++- README.md | 2 +- scripts/check-pr-size/main.go | 69 ++++++++++- scripts/check-pr-size/main_test.go | 76 ++++++++++++ scripts/check-pr-size/size.go | 182 ++++++++++++++++++++++++++-- scripts/check-pr-size/size_test.go | 140 +++++++++++++++++++++ 6 files changed, 476 insertions(+), 14 deletions(-) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 6d43dc9..e2d92f4 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -60,11 +60,25 @@ on: default: claude-opus-4-8-thinking-max diff_size_cap: description: >- - Max changed lines (added + removed, after diff_excludes). PRs over - the cap are skipped — too large for a useful single-pass review. + Max counted changed lines. PRs over the cap are skipped — too large + for a useful single-pass review. "Counted" is added + removed lines + after the shared classifier's generated-file exclusion and (when + ignore_comments is true) blank/comment-line discounting. type: number required: false default: 5000 + ignore_comments: + description: >- + Discount blank lines and comment-only lines from the counted total, so + AI-generated comment volume doesn't push a PR over the cap and skip the + review. Count-only: comments are still sent to the review panel (a + reviewer should catch wrong or misleading comments). Heuristic + (per-language, single-line comment markers; multi-line block-comment + bodies and string literals aren't lexed), so it's approximate. Default + true; set false to count every changed line. + type: boolean + required: false + default: true review_label: description: Label whose addition triggers the review. type: string @@ -345,6 +359,9 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + # Read by the tool's --ignore-comments default: discount blank/comment + # lines from the counted total (count-only; the reviewed diff keeps them). + PR_SIZE_IGNORE_COMMENTS: ${{ inputs.ignore_comments }} run: | # Reuse the PR-size classifier as the single source of truth. --max is # THIS review's cap (DIFF_SIZE_CAP), so the tool's `over_cap` output diff --git a/README.md b/README.md index a05099f..4cf3f05 100644 --- a/README.md +++ b/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. 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`, Go generated markers, 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. 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`, Go generated markers, 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`. | diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 9022a59..1b2e659 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -41,6 +41,7 @@ func main() { 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)") generatedOut := flag.String("generated-out", "", "if set, also write the NUL-separated list of paths classified as generated to this file (for a downstream consumer to exclude, e.g. the cursor-review diff); does not change the size verdict") + 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 --generated-out)") flag.Parse() if *base == "" { @@ -74,6 +75,12 @@ func main() { attr.trusted = attrTrusted(attr.useSource, attrModified, *bypassFlag) classify(files, *base, *head, attr, extras) + // 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 { // The check is failing and we ignored linguist-generated exclusions; tell @@ -144,6 +151,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 @@ -335,6 +399,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) } @@ -355,7 +422,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 { diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index 48481c8..37767a2 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -563,3 +563,79 @@ func TestRunGitFoldsStderrIntoError(t *testing.T) { t.Errorf("runGit error should carry git's stderr diagnostic, got: %v", err) } } + +// TestAnnotateDiscountsGit exercises the whole count path against real git: +// diffFiles (numstat) -> classify -> annotateDiscounts -> Evaluate. The PR adds +// significant code alongside comment and blank lines; only the significant lines +// must count toward the cap. +func TestAnnotateDiscountsGit(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "hand.go", "package main\n") + base := commitAll(t, dir, "base") + + // Head adds 6 lines: 1 blank + 2 comments (insignificant) + 3 code (significant). + writeFile(t, dir, "hand.go", "package main\n\n// a comment\n// another comment\nfunc F() int {\n\treturn 41\n}\n") + head := commitAll(t, dir, "head") + t.Chdir(dir) + + files, err := diffFiles(base, head) + if err != nil { + t.Fatalf("diffFiles: %v", err) + } + classify(files, base, head, attrPolicy{}, Extras{}) + annotateDiscounts(files, base, head) + + var hand *FileChange + for i := range files { + if files[i].Path == "hand.go" { + hand = &files[i] + } + } + if hand == nil { + t.Fatal("hand.go not present in diff") + } + if hand.Changed() != 6 { + t.Fatalf("hand.go Changed() = %d, want 6 added lines", hand.Changed()) + } + if hand.Discounted != 3 { + t.Errorf("hand.go Discounted = %d, want 3 (1 blank + 2 comments)", hand.Discounted) + } + if hand.Counted() != 3 { + t.Errorf("hand.go Counted() = %d, want 3 significant lines", hand.Counted()) + } + if res := Evaluate(files, 1000, false); res.Counted != 3 || res.Discounted != 3 { + t.Errorf("Evaluate: Counted=%d Discounted=%d, want 3 and 3", res.Counted, res.Discounted) + } +} + +// TestAnnotateDiscountsSkipsGenerated proves comment-discounting never touches a +// generated file: its lines stay in the generated total (excluded wholesale), +// not the counted or discounted totals. +func TestAnnotateDiscountsSkipsGenerated(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "hand.go", "package main\n") + base := commitAll(t, dir, "base") + + writeFile(t, dir, "gen.go", "// Code generated by tool DO NOT EDIT.\npackage gen\n\n// comment\nvar X = 1\n") + writeFile(t, dir, "hand.go", "package main\n\n// note\nvar Y = 2\n") + head := commitAll(t, dir, "head") + t.Chdir(dir) + + files, err := diffFiles(base, head) + if err != nil { + t.Fatalf("diffFiles: %v", err) + } + classify(files, base, head, attrPolicy{}, Extras{}) + annotateDiscounts(files, base, head) + + for i := range files { + if files[i].Path == "gen.go" { + if !files[i].Generated { + t.Error("gen.go should be classified generated") + } + if files[i].Discounted != 0 { + t.Errorf("generated gen.go must not be discounted, got %d", files[i].Discounted) + } + } + } +} diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 2b41f22..81240f0 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -12,6 +12,7 @@ package main import ( + "bufio" "bytes" "fmt" "io" @@ -49,10 +50,17 @@ type FileChange struct { Deleted int Binary bool Generated bool + // Discounted is the number of this file's changed lines (added + deleted) + // that are blank or comment-only, so excluded from the counted total when + // comment-discounting is on. Zero unless annotateDiscounts populated it (see + // main.go), so Counted() == Changed() for every existing path/test by + // default — the feature only ever subtracts. + Discounted int } -// Changed returns the line count this file contributes to PR size (added + -// deleted). Binary files contribute nothing (they are not lines of code). +// Changed returns the raw line count this file contributes (added + deleted), +// used for reporting a file's size and for the generated-exclusion total. +// Binary files contribute nothing (they are not lines of code). func (f FileChange) Changed() int { if f.Binary { return 0 @@ -60,14 +68,26 @@ func (f FileChange) Changed() int { return f.Added + f.Deleted } +// Counted returns the lines this file contributes to the cap: its changed lines +// minus any blank/comment lines discounted from the count. Never negative +// (Discounted is clamped to Changed() by the caller). Equal to Changed() when +// nothing was discounted, so the default (comment-discounting off) is unchanged. +func (f FileChange) Counted() int { + if c := f.Changed() - f.Discounted; c > 0 { + return c + } + return 0 +} + // Result is the outcome of evaluating a diff against the cap. type Result struct { - Counted int // changed lines from non-generated, non-binary files - Generated int // changed lines excluded because the file is generated - Max int // the configured ceiling - Bypassed bool // a bypass label was present - OK bool // Bypassed OR Counted <= Max - // Files sorted by descending Changed(), for reporting. + Counted int // counted lines from non-generated, non-binary files (after any comment/blank discount) + Generated int // changed lines excluded because the file is generated + Discounted int // changed lines excluded from non-generated files as blank/comment-only (count-only) + Max int // the configured ceiling + Bypassed bool // a bypass label was present + OK bool // Bypassed OR Counted <= Max + // Files sorted by descending Counted(), for reporting. Files []FileChange // Note is an optional human-facing explanation appended to the report (e.g. // why linguist-generated exclusions were skipped). Set by the caller. @@ -288,11 +308,153 @@ func Evaluate(files []FileChange, max int, bypassed bool) Result { res.Generated += f.Changed() continue } - res.Counted += f.Changed() + res.Counted += f.Counted() + res.Discounted += f.Discounted } res.OK = bypassed || res.Counted <= max sort.SliceStable(res.Files, func(i, j int) bool { - return res.Files[i].Changed() > res.Files[j].Changed() + return res.Files[i].Counted() > res.Files[j].Counted() }) return res } + +// commentSyntax describes a language's comment markers for the blank/comment +// discounting heuristic. line holds full-line comment prefixes; blockStart / +// blockEnd bound a block comment. Only SINGLE-LINE block comments (start and +// end on the same trimmed line) are recognized — multi-line block bodies and +// language string literals are deliberately NOT tracked (that would need a real +// per-language lexer). The heuristic is count-only and documented as approximate. +type commentSyntax struct { + line []string + blockStart string + blockEnd string +} + +var ( + cFamily = commentSyntax{line: []string{"//"}, blockStart: "/*", blockEnd: "*/"} // C/Go/JS/TS/Rust/… + hashCmt = commentSyntax{line: []string{"#"}} // Python/Ruby/shell/YAML/… + dashCmt = commentSyntax{line: []string{"--"}} // SQL/Lua/Haskell + mlCmt = commentSyntax{blockStart: ""} // HTML/XML/Markdown/Vue +) + +// extComment maps a lowercased file extension (with dot) to its comment syntax. +// An extension not listed has no comment markers, so only blank lines are ever +// discounted for it — safe (never miscounts real code as a comment). +var extComment = map[string]commentSyntax{ + ".go": cFamily, ".c": cFamily, ".h": cFamily, ".cc": cFamily, ".cpp": cFamily, + ".cxx": cFamily, ".hpp": cFamily, ".hh": cFamily, ".java": cFamily, ".js": cFamily, + ".jsx": cFamily, ".ts": cFamily, ".tsx": cFamily, ".mjs": cFamily, ".cjs": cFamily, + ".rs": cFamily, ".kt": cFamily, ".kts": cFamily, ".swift": cFamily, ".scala": cFamily, + ".cs": cFamily, ".php": cFamily, ".m": cFamily, ".mm": cFamily, ".dart": cFamily, + ".proto": cFamily, ".gradle": cFamily, ".groovy": cFamily, + ".py": hashCmt, ".rb": hashCmt, ".sh": hashCmt, ".bash": hashCmt, ".zsh": hashCmt, + ".yaml": hashCmt, ".yml": hashCmt, ".toml": hashCmt, ".pl": hashCmt, ".pm": hashCmt, + ".r": hashCmt, ".tf": hashCmt, ".tfvars": hashCmt, ".mk": hashCmt, ".ps1": hashCmt, + ".sql": dashCmt, ".lua": dashCmt, ".hs": dashCmt, + ".html": mlCmt, ".htm": mlCmt, ".xml": mlCmt, ".vue": mlCmt, ".svelte": mlCmt, + ".md": mlCmt, ".markdown": mlCmt, +} + +// commentSyntaxFor returns the comment syntax for a path by extension (plus a +// few well-known extensionless names). Unknown → zero value (blank-only). +func commentSyntaxFor(path string) commentSyntax { + switch baseName(path) { + case "Makefile", "makefile", "GNUmakefile", "Dockerfile": + return hashCmt + } + base := baseName(path) + if dot := strings.LastIndex(base, "."); dot >= 0 { + return extComment[strings.ToLower(base[dot:])] + } + return commentSyntax{} +} + +// isInsignificantLine reports whether a changed line's body (the diff line with +// its +/- marker already stripped) is blank or a comment under cs, and so should +// not count toward PR size. A line whose non-whitespace content merely CONTAINS +// a comment token (e.g. a string literal `x = "# not a comment"`) is significant +// — only a line that STARTS (after trimming) with a comment marker is dropped. +func isInsignificantLine(body string, cs commentSyntax) bool { + t := strings.TrimSpace(body) + if t == "" { + return true + } + for _, prefix := range cs.line { + if strings.HasPrefix(t, prefix) { + return true + } + } + if cs.blockStart != "" && strings.HasPrefix(t, cs.blockStart) && strings.Contains(t, cs.blockEnd) { + return true + } + return false +} + +// ParseDiscounts parses a unified diff and returns, per new-file path, the count +// of changed lines (added or removed) that are blank or comment-only under that +// file's language. It is pure (reads from patch) so it unit-tests against literal +// diffs. File/section headers (`+++ `, `--- `) are only honored OUTSIDE a hunk; +// once inside a hunk (after `@@`) a leading `+`/`-` is content — so an added line +// whose own text begins with `+++`/`---` is never mistaken for a header. A path +// git had to quote (spaces/specials; non-ASCII is disabled via core.quotePath in +// the caller) is left as-is and simply won't match the numstat path, so that file +// falls back to its raw count — the discount only ever applies to paths we +// resolved cleanly. +func ParseDiscounts(patch io.Reader) (map[string]int, error) { + result := map[string]int{} + sc := bufio.NewScanner(patch) + // Allow very long lines (e.g. minified/one-line files) rather than erroring; + // 16 MiB is far past any real source line. + sc.Buffer(make([]byte, 0, 64*1024), 16*1024*1024) + + var path string + var cs commentSyntax + var haveFile, inHunk bool + for sc.Scan() { + line := sc.Text() + switch { + case strings.HasPrefix(line, "diff --git "): + haveFile, inHunk, path = false, false, "" + case !inHunk && strings.HasPrefix(line, "--- "): + if p := parseDiffHeaderPath(line[4:]); p != "" { + path, cs, haveFile = p, commentSyntaxFor(p), true + } + case !inHunk && strings.HasPrefix(line, "+++ "): + if p := parseDiffHeaderPath(line[4:]); p != "" { + path, cs, haveFile = p, commentSyntaxFor(p), true + } + case strings.HasPrefix(line, "@@"): + inHunk = true + case inHunk && strings.HasPrefix(line, "+"): + if haveFile && isInsignificantLine(line[1:], cs) { + result[path]++ + } + case inHunk && strings.HasPrefix(line, "-"): + if haveFile && isInsignificantLine(line[1:], cs) { + result[path]++ + } + } + } + if err := sc.Err(); err != nil { + return nil, err + } + return result, nil +} + +// parseDiffHeaderPath extracts the file path from a diff `--- ` / `+++ ` header +// value (the text after the 4-char prefix). Returns "" for /dev/null (added or +// deleted side) and for a git-quoted path (starts with a double quote — left +// unresolved so its file falls back to the raw count). Strips the a/ or b/ prefix +// and a trailing tab-delimited timestamp git may append. +func parseDiffHeaderPath(v string) string { + if i := strings.IndexByte(v, '\t'); i >= 0 { + v = v[:i] + } + if v == "/dev/null" || v == "" || strings.HasPrefix(v, `"`) { + return "" + } + if len(v) >= 2 && (v[:2] == "a/" || v[:2] == "b/") { + return v[2:] + } + return v +} diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 7c25eb9..feff300 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -410,3 +410,143 @@ func TestContentGeneratedRejectsSymlink(t *testing.T) { t.Errorf("contentGenerated must not follow symlinks") } } + +// TestCommentSyntaxFor pins the ext/basename → comment-syntax mapping, including +// the "unknown extension gets no comment markers" (blank-only) safe default. +func TestCommentSyntaxFor(t *testing.T) { + t.Parallel() + tests := []struct { + path string + wantLine []string + wantBlock string // blockStart, "" if none + }{ + {"pkg/foo.go", []string{"//"}, "/*"}, + {"web/app.tsx", []string{"//"}, "/*"}, + {"scripts/deploy.sh", []string{"#"}, ""}, + {"infra/main.tf", []string{"#"}, ""}, + {"db/schema.sql", []string{"--"}, ""}, + {"docs/index.html", nil, "", mlCmt, true}, + {"unknown lang: blank still insignificant", " ", commentSyntax{}, true}, + {"unknown lang: comment-looking code counts", "// still code here", commentSyntax{}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := isInsignificantLine(tt.body, tt.cs); got != tt.want { + t.Errorf("isInsignificantLine(%q) = %v, want %v", tt.body, got, tt.want) + } + }) + } +} + +// TestParseDiscounts feeds a literal unified diff spanning a .go and a .py file +// and checks the per-path blank/comment counts. It also covers the header-vs- +// content ambiguity: an ADDED line whose own text begins with "+++" (inside a +// hunk) must be treated as content, never as a file header. +func TestParseDiscounts(t *testing.T) { + t.Parallel() + patch := strings.Join([]string{ + "diff --git a/pkg/foo.go b/pkg/foo.go", + "index 111..222 100644", + "--- a/pkg/foo.go", + "+++ b/pkg/foo.go", + "@@ -1,3 +1,7 @@", + " package foo", + "+// added comment", // insignificant (go comment) + "+", // insignificant (blank) + "+func F() int {", // significant + "+++ still content", // significant: leading + stripped -> "++ still content", not a header + "-x := 1", // significant (removed code) + "-// old comment", // insignificant (removed comment) + " return 0", + "diff --git a/app.py b/app.py", + "index 333..444 100644", + "--- a/app.py", + "+++ b/app.py", + "@@ -0,0 +1,2 @@", + "+# python comment", // insignificant + "+value = 42", // significant + "", + }, "\n") + + got, err := ParseDiscounts(strings.NewReader(patch)) + if err != nil { + t.Fatalf("ParseDiscounts: %v", err) + } + if got["pkg/foo.go"] != 3 { + t.Errorf("pkg/foo.go discounted = %d, want 3 (comment, blank, removed comment)", got["pkg/foo.go"]) + } + if got["app.py"] != 1 { + t.Errorf("app.py discounted = %d, want 1 (python comment)", got["app.py"]) + } +} + +// TestEvaluateDiscount proves Discounted lowers the counted total (not the +// generated total) and that a fully blank/comment file drops to zero counted. +func TestEvaluateDiscount(t *testing.T) { + t.Parallel() + files := []FileChange{ + {Path: "a.go", Added: 100, Deleted: 0, Discounted: 40}, // 60 counted + {Path: "b.go", Added: 20, Deleted: 0, Discounted: 20}, // all comment → 0 counted + {Path: "gen.pb.go", Added: 900, Generated: true}, // excluded wholesale + } + res := Evaluate(files, 1000, false) + if res.Counted != 60 { + t.Errorf("Counted = %d, want 60", res.Counted) + } + if res.Discounted != 60 { + t.Errorf("Discounted = %d, want 60 (40+20)", res.Discounted) + } + if res.Generated != 900 { + t.Errorf("Generated = %d, want 900 (raw, discount does not touch generated)", res.Generated) + } + // b.go contributes 0 counted lines. + for _, f := range res.Files { + if f.Path == "b.go" && f.Counted() != 0 { + t.Errorf("b.go Counted() = %d, want 0", f.Counted()) + } + } +} From ec880db4ad6395dbed4d793d00397ed523b7ae44 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 21 Jul 2026 21:03:58 -0700 Subject: [PATCH 3/3] fix(cursor-review): resolve panel findings on classifier reuse (BE-1916) Address all 8 cursor-review panel threads on #59: - Go generated marker is now honored only from the BASE blob for the review gate (new --marker-from-base flag): a PR can no longer hide hand-written code from the panel by prepending the one-line marker, matching the base-ref invariant of the linguist-generated path. pr-size.yml (flag off) is unchanged. - The reviewed diff is built by the tool itself (--reviewed-diff-out): one full git diff streamed through a section filter keyed on the classified set (FilterPatch). No per-file argv pathspecs, so an unbounded generated set can't overflow ARG_MAX; a generated rename drops as one section (no orphaned old-path deletion); and a write failure exits 2 so counts are never paired with a missing/partial diff. (git diff has no --pathspec-from-file, hence post-filtering.) - Degraded-path semantics made explicit and accurate: git-level breakage fails the diff-size job (fail closed); tool-level failure falls back to a RAW no-exclusions count gate before building the diff_excludes-only diff, so a degraded run can never feed an unbounded diff to the eight panel jobs. - classify() applies path/attr rules to binary files (content read still skipped), so a binary matching extra_generated_globs (e.g. data/object_info.json.gz) leaves the reviewed diff again. - globRegexp compiles **/ to (?:.*/)? (gitignore semantics): root-level node_modules/, dist/, *.min.js now match the default excludes. - Pin every remaining mutable action tag in cursor-review.yml (checkout v6.0.3, upload-artifact v7.0.1, download-artifact v8.0.1, create-github-app-token v3.2.0). Validation: go test ./... (new FilterPatch/parseDiffGitPaths/ marker-from-base/binary-extras/glob suites), gofmt, go vet, actionlint (incl. shellcheck), python unittest (40 cursor-review tests), plus an end-to-end smoke: head-added-marker attack file is counted AND reviewed; base-generated mock excluded; root node_modules excluded; diff_excludes stays diff-only. Co-Authored-By: Claude Opus 4.8 --- .github/cursor-review/README.md | 7 +- .github/workflows/cursor-review.yml | 116 ++++++++------ README.md | 2 +- scripts/check-pr-size/main.go | 142 +++++++++++++----- scripts/check-pr-size/main_test.go | 141 +++++++++++------ scripts/check-pr-size/size.go | 173 ++++++++++++++++++++- scripts/check-pr-size/size_test.go | 225 +++++++++++++++++++++++++++- 7 files changed, 674 insertions(+), 132 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 7ee26ae..e14a561 100644 --- a/.github/cursor-review/README.md +++ b/.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). | diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index e2d92f4..f3072fe 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -97,6 +97,11 @@ on: marked `linguist-generated` in the BASE ref's `.gitattributes` (so a PR can't exempt itself), and the canonical Go `// Code generated ... DO NOT EDIT.` marker — so you rarely need more. + Because this classification decides what the review panel SEES, the + Go marker is honored only from the BASE blob (same base-ref invariant + as `.gitattributes`): a PR cannot hide a file from review by adding + the marker, and a file NEW in the PR never matches it — cover newly + generated files with a glob here or a base `.gitattributes` rule. type: string required: false default: >- @@ -304,10 +309,13 @@ jobs: permissions: contents: read # within_cap is derived from the shared check-pr-size tool's `over_cap` - # output: counted = non-generated changed lines, compared against the review - # cap passed as --max. If the tool fails operationally, over_cap is unset and - # this evaluates true — falling open to "review it" rather than silently - # skipping a PR because the classifier hiccuped. + # output: counted = non-generated changed lines (minus any comment/blank + # discount), compared against the review cap passed as --max. Failure + # handling is explicit and two-tier (see the classify step): a git-level + # failure fails THIS JOB — fail closed, no review — while a tool-level + # failure falls back to the RAW changed-line count with no exclusions + # (strictly >= the classified count), so a degraded run can still review a + # small PR but can never feed an unbounded diff to the panel. outputs: within_cap: ${{ steps.check.outputs.over_cap != 'true' }} steps: @@ -354,7 +362,7 @@ jobs: git -C _pr_size_tool clean -ffdx (cd _pr_size_tool/scripts/check-pr-size && go build -o "${RUNNER_TEMP}/check-pr-size" .) - - name: Classify generated files and check size + - name: Classify generated files, check size, and build reviewed diff id: check env: BASE_SHA: ${{ github.event.pull_request.base.sha }} @@ -368,53 +376,77 @@ jobs: # means "non-generated changed lines exceed the review cap" — exactly # the gate we want. --mode warn: the tool never exits non-zero on # overage (this workflow gates via within_cap, not the tool's own - # pass/fail). The same run writes the classified generated paths to - # --generated-out for the diff step below, so classification happens - # ONCE and the count and the reviewed diff can never disagree. + # pass/fail). The same run writes the reviewed diff (the PR's patch + # with every generated file's section filtered out, minus any caller + # diff_excludes pathspecs) to --reviewed-diff-out, so classification + # happens ONCE and the count and the reviewed diff can never disagree. + # Filtering the patch — instead of expanding one exclude pathspec per + # generated file onto argv — means an unbounded number of generated + # files (they cost nothing against the cap) can't overflow ARG_MAX. # - # An operational failure (exit 2 — bad ref, git error) leaves over_cap - # unset → within_cap evaluates true → the diff step, finding no - # generated-paths file, falls back to diff_excludes-only (the pre-tool - # behavior). Fail open to "review", never silently skip. + # --marker-from-base: this classification decides what a (potentially + # blocking) review SEES, so the Go generated marker is honored only + # from the BASE blob — a PR can't hide hand-written code from the + # panel by prepending the one-line marker. Matches the base-ref + # invariant the linguist-generated attribute path already enforces. + # + # Failure handling is two-tier, both explicit: + # - git-level breakage (bad ref, missing history): the fallback + # `git diff --numstat` below dies under `set -e` and FAILS this + # job — fail closed, a loud red X, never a silent skip. + # - tool-level failure (classifier bug, patch-write error): fall + # back to the RAW changed-line count with no exclusions — + # strictly >= the classified count, so a degraded run still + # reviews a small PR but can never feed an unbounded diff to the + # eight panel jobs. The next step rebuilds the diff with + # diff_excludes only (the pre-classifier behavior). rc=0 "${RUNNER_TEMP}/check-pr-size" \ --base "${BASE_SHA}" \ --head "${HEAD_SHA}" \ --max "${DIFF_SIZE_CAP}" \ --mode warn \ + --marker-from-base \ --extra-generated-globs "${EXTRA_GENERATED_GLOBS}" \ --extra-lockfiles "${EXTRA_LOCKFILES}" \ - --generated-out "${RUNNER_TEMP}/generated-paths" || rc=$? - if [ "$rc" -ne 0 ]; then - echo "::warning::check-pr-size exited ${rc} — reviewing with no generated-file exclusions (diff_excludes still apply)." - rm -f "${RUNNER_TEMP}/generated-paths" + --diff-excludes "${DIFF_EXCLUDES}" \ + --reviewed-diff-out "${RUNNER_TEMP}/pr-diff.patch" || rc=$? + if [ "$rc" -eq 0 ]; then + if [ -f "${RUNNER_TEMP}/pr-diff.patch" ]; then + echo "Reviewed diff size: $(wc -l < "${RUNNER_TEMP}/pr-diff.patch") lines" + fi + else + echo "::warning::check-pr-size exited ${rc} — gating on the raw line count and reviewing with no generated-file exclusions (diff_excludes still apply)." + rm -f "${RUNNER_TEMP}/pr-diff.patch" + echo "degraded=true" >> "$GITHUB_OUTPUT" + # Raw-count gate: no exclusions, so always >= the classified count. + # Appended after any over_cap the tool already wrote — the last + # value wins, so a raw overage can only tighten the gate. The + # numstat runs as its own statement (not inside a pipeline, whose + # exit status `set -e` would ignore) so git-level breakage fails + # the job here (fail closed, above). + git diff --numstat "${BASE_SHA}...${HEAD_SHA}" > "${RUNNER_TEMP}/raw-numstat" + RAW=$(awk '{ if ($1 != "-") total += $1; if ($2 != "-") total += $2 } END { print total + 0 }' "${RUNNER_TEMP}/raw-numstat") + echo "Raw changed lines (no exclusions): ${RAW} (cap: ${DIFF_SIZE_CAP})" + if [ "${RAW}" -gt "${DIFF_SIZE_CAP}" ]; then + echo "over_cap=true" >> "$GITHUB_OUTPUT" + fi fi - - name: Build reviewed diff (generated files excluded) - # Skip when over cap: the review is skipped anyway, so don't spend time - # rendering (and uploading) a diff nothing will consume. - if: steps.check.outputs.over_cap != 'true' + - name: Build reviewed diff (degraded fallback) + # Only when the classifier tool failed but the raw count is within the + # cap: no generated-file exclusions are available, so build the diff + # with just the caller's diff_excludes (the pre-classifier behavior). + # The raw-count gate above already bounded this diff's size. + if: steps.check.outputs.over_cap != 'true' && steps.check.outputs.degraded == 'true' env: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - # Exclude every path the classifier flagged generated, plus any caller - # diff_excludes pathspecs (applied verbatim, back-compat). - # :(exclude,literal) disables pathspec magic so a path with - # metacharacters matches literally. An absent/empty generated-paths - # file (tool degraded, or nothing generated) leaves EXCLUDE_ARGS empty, - # so the diff is just the full diff minus diff_excludes. - EXCLUDE_ARGS=() - if [ -s "${RUNNER_TEMP}/generated-paths" ]; then - while IFS= read -r -d '' p; do - if [ -n "$p" ]; then EXCLUDE_ARGS+=(":(exclude,literal)${p}"); fi - done < "${RUNNER_TEMP}/generated-paths" - fi - echo "Excluding ${#EXCLUDE_ARGS[@]} generated file(s) classified by check-pr-size." # $DIFF_EXCLUDES is intentionally unquoted so bash word-splits it into - # pathspec args; the tool-derived excludes are a quoted array. + # pathspec args. # shellcheck disable=SC2086 - git diff "${BASE_SHA}...${HEAD_SHA}" -- . $DIFF_EXCLUDES "${EXCLUDE_ARGS[@]}" \ + git diff "${BASE_SHA}...${HEAD_SHA}" -- . $DIFF_EXCLUDES \ > "${RUNNER_TEMP}/pr-diff.patch" echo "Reviewed diff size: $(wc -l < "${RUNNER_TEMP}/pr-diff.patch") lines" @@ -595,7 +627,7 @@ jobs: # pinned via workflows_ref) — never from the PR checkout, so a # malicious PR can't rewrite the prompt to exfiltrate the diff or # smuggle instructions to the model. - uses: actions/checkout@v6 + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: repository: Comfy-Org/github-workflows ref: ${{ inputs.workflows_ref }} @@ -694,7 +726,7 @@ jobs: - name: Upload findings artifact if: always() - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: findings-${{ matrix.review_type }}-${{ matrix.model }} path: /tmp/findings-out/findings.json @@ -714,7 +746,7 @@ jobs: pull-requests: write steps: - name: Load cursor-review assets - uses: actions/checkout@v6 + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: repository: Comfy-Org/github-workflows ref: ${{ inputs.workflows_ref }} @@ -736,7 +768,7 @@ jobs: echo "$HOME/.cursor/bin" >> "$GITHUB_PATH" - name: Download panel findings - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: path: /tmp/panel pattern: findings-* @@ -941,7 +973,7 @@ jobs: - name: Mint bot-identity token (optional) id: bot_token if: ${{ inputs.bot_app_id != '' }} - uses: actions/create-github-app-token@v3 + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 with: app-id: ${{ inputs.bot_app_id }} private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} @@ -998,7 +1030,7 @@ jobs: contents: read steps: - name: Load cursor-review assets - uses: actions/checkout@v6 + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: repository: Comfy-Org/github-workflows ref: ${{ inputs.workflows_ref }} @@ -1052,7 +1084,7 @@ jobs: contents: read steps: - name: Load cursor-review assets - uses: actions/checkout@v6 + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: repository: Comfy-Org/github-workflows ref: ${{ inputs.workflows_ref }} diff --git a/README.md b/README.md index 4cf3f05..e8359a1 100644 --- a/README.md +++ b/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. 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`, Go generated markers, 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.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`. | diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 1b2e659..73145bf 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -1,6 +1,7 @@ package main import ( + "bufio" "bytes" "context" "flag" @@ -40,8 +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)") - generatedOut := flag.String("generated-out", "", "if set, also write the NUL-separated list of paths classified as generated to this file (for a downstream consumer to exclude, e.g. the cursor-review diff); does not change the size verdict") - 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 --generated-out)") + 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 == "" { @@ -73,7 +76,7 @@ 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. @@ -100,15 +103,24 @@ func main() { report(res, *modeFlag, *bypassLabel) writeGitHubOutputs(res) - // Emit the classified generated paths for a downstream consumer (the - // cursor-review workflow excludes them from the diff it feeds the review - // models, reusing THIS classifier as the single source of truth for "what - // is codegen"). Written last and best-effort: a write failure is logged but - // never changes the size verdict already reported above, nor fails a check - // that would otherwise pass — the consumer degrades to no exclusions. - if *generatedOut != "" { - if err := writeGeneratedPaths(files, *generatedOut); err != nil { - fmt.Fprintf(os.Stderr, "check-pr-size: could not write --generated-out %q: %v\n", *generatedOut, err) + // 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") } } @@ -117,21 +129,65 @@ func main() { } } -// writeGeneratedPaths writes every path classified as generated to path, -// NUL-separated with a trailing NUL after each entry. NUL matches the separator -// git's `-z` numstat/check-attr use and round-trips paths containing spaces or -// newlines (which the -z numstat parser already admits), so a consumer can read -// it back with a NUL-delimited read loop and no quoting hazards. An empty -// generated set writes a zero-byte file — a consumer reads it as "exclude nothing". -func writeGeneratedPaths(files []FileChange, path string) error { - var b bytes.Buffer +// 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 { - b.WriteString(f.Path) - b.WriteByte(0) + 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 } - return os.WriteFile(path, b.Bytes(), 0o644) + 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 @@ -235,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 } } @@ -352,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 diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index 37767a2..4c2a0a6 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -246,7 +246,7 @@ func TestClassifyPRAddedGitattributesDoesNotReduceCount(t *testing.T) { } attr := attrPolicy{source: base, useSource: checkAttrSourceSupported(base)} attr.trusted = attrTrusted(attr.useSource, TouchesGitattributes(files), false) - classify(files, base, head, attr, Extras{}) + classify(files, base, head, attr, Extras{}, false) for _, f := range files { if f.Path == "hand.go" && f.Generated { @@ -287,7 +287,7 @@ func TestClassifyHonorsLegitBaseGitattributes(t *testing.T) { if !attr.trusted { t.Fatal("attribute path should be trusted (PR does not touch .gitattributes)") } - classify(files, base, head, attr, Extras{}) + classify(files, base, head, attr, Extras{}, false) var genExcluded, handCounted bool for _, f := range files { @@ -306,56 +306,109 @@ func TestClassifyHonorsLegitBaseGitattributes(t *testing.T) { } } -// TestWriteGeneratedPaths proves --generated-out serialization: exactly the -// paths marked Generated are emitted, NUL-separated (so a name with spaces or a -// newline round-trips), hand-written paths are omitted, and an all-hand-written -// set yields a zero-byte file (which the cursor-review consumer reads as -// "exclude nothing"). -func TestWriteGeneratedPaths(t *testing.T) { - t.Parallel() - files := []FileChange{ - {Path: "api/service.pb.go", Generated: true}, - {Path: "internal/hand.go", Generated: false}, - {Path: "weird name\nwith newline.json", Generated: true}, - {Path: "web/vendor/lib.js", Generated: true}, +// TestWriteReviewedDiff proves the end-to-end --reviewed-diff-out path against +// real git: the emitted patch keeps the hand-written file's section, drops the +// generated file's section entirely, and honors a verbatim exclude pathspec — +// with no per-file argv involved, however many files are excluded. +func TestWriteReviewedDiff(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "hand.go", "package main\n") + writeFile(t, dir, "gen.go", "// Code generated by tool DO NOT EDIT.\npackage main\n") + writeFile(t, dir, "notes.txt", "old\n") + base := commitAll(t, dir, "base") + + writeFile(t, dir, "hand.go", "package main\n\nfunc F() {}\n") + writeFile(t, dir, "gen.go", "// Code generated by tool DO NOT EDIT.\npackage main\n\nvar Regenerated = true\n") + writeFile(t, dir, "notes.txt", "new\n") + head := commitAll(t, dir, "head") + t.Chdir(dir) + + files, err := diffFiles(base, head) + if err != nil { + t.Fatalf("diffFiles: %v", err) } - out := filepath.Join(t.TempDir(), "gen-paths") - if err := writeGeneratedPaths(files, out); err != nil { - t.Fatalf("writeGeneratedPaths: %v", err) + classify(files, base, head, attrPolicy{}, Extras{}, false) + + out := filepath.Join(t.TempDir(), "pr-diff.patch") + if err := writeReviewedDiff(base, head, []string{":(exclude)notes.txt"}, files, out); err != nil { + t.Fatalf("writeReviewedDiff: %v", err) } - data, err := os.ReadFile(out) + patch, err := os.ReadFile(out) if err != nil { t.Fatal(err) } - // Split on NUL; the trailing NUL after the final path yields an empty tail - // element, which the filter drops — mirroring how the consumer reads it. - var got []string - for _, p := range strings.Split(string(data), "\x00") { - if p != "" { - got = append(got, p) - } + got := string(patch) + if !strings.Contains(got, "diff --git a/hand.go b/hand.go") { + t.Errorf("reviewed diff should keep hand.go:\n%s", got) } - want := []string{"api/service.pb.go", "weird name\nwith newline.json", "web/vendor/lib.js"} - if len(got) != len(want) { - t.Fatalf("got %d paths %q, want %d %q", len(got), got, len(want), want) + if strings.Contains(got, "gen.go") { + t.Errorf("reviewed diff must not contain the generated file's section:\n%s", got) } - for i := range want { - if got[i] != want[i] { - t.Errorf("path[%d] = %q, want %q", i, got[i], want[i]) - } + if strings.Contains(got, "notes.txt") { + t.Errorf("reviewed diff must honor the verbatim exclude pathspec:\n%s", got) } +} + +// TestContentGeneratedMarkerFromBase pins the base-blob marker invariant: with +// markerFromBase set, a marker the PR ADDS is ignored (a PR cannot self-exempt +// a file from a consumer's review), while a file already generated at base +// stays classified even if the head strips the marker. +func TestContentGeneratedMarkerFromBase(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "hand.go", "package main\nvar A = 1\n") + writeFile(t, dir, "gen.go", "// Code generated by tool DO NOT EDIT.\npackage main\n") + base := commitAll(t, dir, "base") + + // Head: the attack (marker prepended to the hand-written file), plus the + // inverse (marker stripped from the generated file). + writeFile(t, dir, "hand.go", "// Code generated by tool DO NOT EDIT.\npackage main\nvar A = 2\n") + writeFile(t, dir, "gen.go", "package main\n") + writeFile(t, dir, "new_gen.go", "// Code generated by tool DO NOT EDIT.\npackage main\n") + head := commitAll(t, dir, "head") + t.Chdir(dir) - // No generated files → a zero-byte file, not a missing one. - empty := filepath.Join(t.TempDir(), "empty") - if err := writeGeneratedPaths([]FileChange{{Path: "only-hand.go"}}, empty); err != nil { - t.Fatalf("writeGeneratedPaths (empty set): %v", err) + if contentGenerated("hand.go", base, head, true) { + t.Error("a head-added marker must not classify a file generated when markerFromBase is set") } - info, err := os.Stat(empty) + if !contentGenerated("hand.go", base, head, false) { + t.Error("sanity: without markerFromBase the head content is honored (the size-cap behavior)") + } + if !contentGenerated("gen.go", base, head, true) { + t.Error("a file generated at base should stay classified via the base blob") + } + if contentGenerated("new_gen.go", base, head, true) { + t.Error("a file new in the PR has no base blob, so the marker must not match (conservative)") + } +} + +// TestClassifyBinaryExtras is the regression for binary generated files: a +// binary matching an extra generated glob (e.g. data/object_info.json.gz) must +// be marked Generated — previously classify short-circuited binaries before the +// path rules, so the binary reappeared in the reviewed diff as a binary-differs +// stanza despite the caller excluding it. +func TestClassifyBinaryExtras(t *testing.T) { + t.Parallel() + extras, err := ParseExtras("", "data/object_info.json.gz") if err != nil { - t.Fatalf("stat empty output: %v", err) + t.Fatalf("ParseExtras: %v", err) + } + files := []FileChange{ + {Path: "data/object_info.json.gz", Binary: true}, + {Path: "assets/logo.png", Binary: true}, + {Path: "hand.go", Added: 3}, } - if info.Size() != 0 { - t.Errorf("empty generated set should write a zero-byte file, got size %d", info.Size()) + classify(files, "", "", attrPolicy{}, extras, false) + for _, f := range files { + switch f.Path { + case "data/object_info.json.gz": + if !f.Generated { + t.Error("binary matching an extra generated glob must be classified generated") + } + default: + if f.Generated { + t.Errorf("%s must not be classified generated", f.Path) + } + } } } @@ -375,7 +428,7 @@ func TestClassifyAppliesExtras(t *testing.T) { {Path: "web/snapshots/a/b.snap", Added: 900}, {Path: "web/src/hand.ts", Added: 40}, } - classify(files, "", "", attrPolicy{}, extras) + classify(files, "", "", attrPolicy{}, extras, false) wantGenerated := map[string]bool{ "app/Gemfile.lock": true, @@ -521,7 +574,7 @@ func TestContentGeneratedDeletedFileReadsBlob(t *testing.T) { // path is relative, matching production: f.Path comes from `git diff // --numstat` (repo-root-relative), and the process cwd is the repo root. - if !contentGenerated("gen.go", base, head) { + if !contentGenerated("gen.go", base, head, false) { t.Error("a deleted generated file should still classify as generated via the base blob fallback") } } @@ -582,7 +635,7 @@ func TestAnnotateDiscountsGit(t *testing.T) { if err != nil { t.Fatalf("diffFiles: %v", err) } - classify(files, base, head, attrPolicy{}, Extras{}) + classify(files, base, head, attrPolicy{}, Extras{}, false) annotateDiscounts(files, base, head) var hand *FileChange @@ -625,7 +678,7 @@ func TestAnnotateDiscountsSkipsGenerated(t *testing.T) { if err != nil { t.Fatalf("diffFiles: %v", err) } - classify(files, base, head, attrPolicy{}, Extras{}) + classify(files, base, head, attrPolicy{}, Extras{}, false) annotateDiscounts(files, base, head) for i := range files { diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 81240f0..f2eb73f 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -256,9 +256,12 @@ func (e Extras) Generated(path string) bool { // globRegexp compiles a glob pattern to a regexp: `**` matches any characters // including `/`, `*` matches within a path segment, `?` matches one non-`/` -// character; everything else is literal. Every non-wildcard byte is -// QuoteMeta-escaped, so the built expression is always valid and compilation -// cannot fail. +// character; everything else is literal. `**/` matches ZERO or more leading +// segments (gitignore semantics), so `**/dist/**` also matches a root-level +// `dist/` — compiled as `(?:.*/)?`, not `.*/`, which would demand at least one +// parent directory and silently exempt root-level paths from exclusion. Every +// non-wildcard byte is QuoteMeta-escaped, so the built expression is always +// valid and compilation cannot fail. func globRegexp(pattern string) *regexp.Regexp { var b strings.Builder b.WriteString(`^`) @@ -266,8 +269,13 @@ func globRegexp(pattern string) *regexp.Regexp { switch pattern[i] { case '*': if i+1 < len(pattern) && pattern[i+1] == '*' { - b.WriteString(`.*`) - i++ + if i+2 < len(pattern) && pattern[i+2] == '/' { + b.WriteString(`(?:.*/)?`) + i += 2 + } else { + b.WriteString(`.*`) + i++ + } } else { b.WriteString(`[^/]*`) } @@ -441,6 +449,161 @@ func ParseDiscounts(patch io.Reader) (map[string]int, error) { return result, nil } +// FilterPatch streams a unified git diff from r to w, dropping every per-file +// section for which drop reports true on ANY of the section's paths (old side, +// new side, or rename/copy source/destination). Dropping the WHOLE section — +// rather than excluding paths via git pathspecs — is what lets the caller +// exclude an unbounded number of generated files without argv (ARG_MAX) +// limits, and it removes a generated rename in one piece: the old-path +// deletion never survives as an orphaned full-file removal the way it does +// when only the destination is pathspec-excluded. +// +// Section paths are harvested from the `--- `/`+++ ` headers (via +// parseDiffHeaderPath) and the `rename/copy from/to` lines; when a section has +// neither (binary or mode-only changes), the `diff --git a/X b/Y` line is +// parsed as a fallback (parseDiffGitPaths). A section whose paths cannot be +// parsed at all (e.g. git had to quote them) is KEPT: an unparseable file gets +// reviewed, never silently hidden. Buffering is bounded to a section's header +// (a handful of lines); decided content streams through, so a huge kept file +// never sits in memory. +// +// It returns the number of sections kept and dropped. Text before the first +// `diff --git ` line (git emits none, but be tolerant) passes through. +func FilterPatch(r io.Reader, w io.Writer, drop func(path string) bool) (kept, dropped int, err error) { + br := bufio.NewReaderSize(r, 64*1024) + + var header []string // buffered, undecided section-header lines + var paths []string // candidate paths harvested from those lines + var diffGitLine string + deciding := false // buffering a section header, keep/drop not yet decided + emitting := true // disposition of the current decided section (or preamble) + + decide := func() error { + deciding = false + candidates := paths + if len(candidates) == 0 { + candidates = parseDiffGitPaths(diffGitLine) + } + emitting = true + for _, p := range candidates { + if drop(p) { + emitting = false + break + } + } + if emitting { + kept++ + for _, l := range header { + if _, werr := io.WriteString(w, l); werr != nil { + return werr + } + } + } else { + dropped++ + } + header = nil + return nil + } + + harvest := func(line string) { + l := strings.TrimRight(line, "\n") + add := func(v string) { + // A git-quoted value (leading `"`) is left unresolved — the section + // falls back to other headers or is kept. + if v != "" && !strings.HasPrefix(v, `"`) { + paths = append(paths, v) + } + } + switch { + case strings.HasPrefix(l, "--- "), strings.HasPrefix(l, "+++ "): + if p := parseDiffHeaderPath(l[4:]); p != "" { + paths = append(paths, p) + } + case strings.HasPrefix(l, "rename from "): + add(l[len("rename from "):]) + case strings.HasPrefix(l, "rename to "): + add(l[len("rename to "):]) + case strings.HasPrefix(l, "copy from "): + add(l[len("copy from "):]) + case strings.HasPrefix(l, "copy to "): + add(l[len("copy to "):]) + } + } + + for { + line, rerr := br.ReadString('\n') + if line != "" { + isDiffGit := strings.HasPrefix(line, "diff --git ") + if deciding { + // Header ends at the first hunk, a binary stanza, or the next + // file section; anything else is more header to buffer. + if isDiffGit || strings.HasPrefix(line, "@@") || + strings.HasPrefix(line, "Binary files ") || strings.HasPrefix(line, "GIT binary patch") { + if derr := decide(); derr != nil { + return kept, dropped, derr + } + } else { + header = append(header, line) + harvest(line) + continue + } + } + switch { + case isDiffGit: + deciding = true + header = []string{line} + paths = nil + diffGitLine = strings.TrimRight(line, "\n") + case emitting: + if _, werr := io.WriteString(w, line); werr != nil { + return kept, dropped, werr + } + } + } + if rerr == io.EOF { + break + } + if rerr != nil { + return kept, dropped, rerr + } + } + if deciding { + if derr := decide(); derr != nil { + return kept, dropped, derr + } + } + return kept, dropped, nil +} + +// parseDiffGitPaths extracts path candidates from a `diff --git a/X b/Y` line — +// the fallback for sections with no `---`/`+++` or rename/copy headers (binary +// and mode-only changes). The line is ambiguous when paths contain spaces, so +// resolution is conservative: first try the split point where the a/-side +// equals the b/-side (X == Y — every non-rename, however spacey the name); +// otherwise split at ` b/` only when it appears exactly once. A quoted or +// unresolvable line yields nil, which the caller treats as "keep the section". +func parseDiffGitPaths(line string) []string { + v := strings.TrimPrefix(line, "diff --git ") + if v == line || strings.HasPrefix(v, `"`) || !strings.HasPrefix(v, "a/") { + return nil + } + for j := 0; ; { + i := strings.Index(v[j:], " b/") + if i < 0 { + break + } + j += i + if left, right := v[:j], v[j+3:]; left[2:] == right { + return []string{right} + } + j += 3 + } + if i := strings.Index(v, " b/"); i >= 0 && strings.LastIndex(v, " b/") == i { + return []string{v[2:i], v[i+3:]} + } + return nil +} + // parseDiffHeaderPath extracts the file path from a diff `--- ` / `+++ ` header // value (the text after the 4-char prefix). Returns "" for /dev/null (added or // deleted side) and for a git-quoted path (starts with a double quote — left diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index feff300..3e6239e 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -234,6 +234,11 @@ func TestExtrasGenerated(t *testing.T) { {"path glob anchors to full path", "", "gen/**", "gen/a/b.ts", true}, {"path glob does not float", "", "gen/**", "src/gen/a.ts", false}, {"double star crosses segments", "", "web/**/snapshots/**", "web/a/b/snapshots/x.snap", true}, + {"leading double star matches root level", "", "**/node_modules/**", "node_modules/pkg/index.js", true}, + {"leading double star matches nested", "", "**/node_modules/**", "web/node_modules/pkg/index.js", true}, + {"leading double star star matches root file", "", "**/*.min.js", "app.min.js", true}, + {"leading double star star matches nested file", "", "**/*.min.js", "web/js/app.min.js", true}, + {"interior double star matches zero segments", "", "web/**/snapshots/**", "web/snapshots/x.snap", true}, {"single star does not cross segments", "", "web/*/x.ts", "web/a/b/x.ts", false}, {"question mark matches one char", "", "v?.json", "schema/v1.json", true}, {"question mark needs a char", "", "v?.json", "v.json", false}, @@ -378,7 +383,7 @@ func TestContentGeneratedOnlyTrustsGoFiles(t *testing.T) { if err := os.WriteFile(goFile, marker, 0o644); err != nil { t.Fatal(err) } - if !contentGenerated(goFile, "", "") { + if !contentGenerated(goFile, "", "", false) { t.Errorf("a .go file with the generated marker should be generated") } @@ -388,7 +393,7 @@ func TestContentGeneratedOnlyTrustsGoFiles(t *testing.T) { if err := os.WriteFile(mdFile, marker, 0o644); err != nil { t.Fatal(err) } - if contentGenerated(mdFile, "", "") { + if contentGenerated(mdFile, "", "", false) { t.Errorf("a non-.go file with a pasted marker must not be treated as generated") } } @@ -406,7 +411,7 @@ func TestContentGeneratedRejectsSymlink(t *testing.T) { } // The DoS guard must refuse to follow a symlink, even one pointing at a // legitimately generated file. - if contentGenerated(link, "", "") { + if contentGenerated(link, "", "", false) { t.Errorf("contentGenerated must not follow symlinks") } } @@ -550,3 +555,217 @@ func TestEvaluateDiscount(t *testing.T) { } } } + +// TestFilterPatch exercises the reviewed-diff section filter against literal +// patches: sections drop by old path, new path, or rename source/destination +// (the whole rename goes at once — no orphaned old-path deletion), binary and +// mode-only sections resolve via the `diff --git` fallback, and anything +// unparseable is kept, never hidden. +func TestFilterPatch(t *testing.T) { + t.Parallel() + drop := func(set ...string) func(string) bool { + m := map[string]bool{} + for _, p := range set { + m[p] = true + } + return func(p string) bool { return m[p] } + } + + textSection := func(path, add string) string { + return "diff --git a/" + path + " b/" + path + "\n" + + "index 1111111..2222222 100644\n" + + "--- a/" + path + "\n" + + "+++ b/" + path + "\n" + + "@@ -1,1 +1,2 @@\n line\n+" + add + "\n" + } + + t.Run("drops generated section, keeps hand-written", func(t *testing.T) { + t.Parallel() + patch := textSection("hand.go", "var A = 1") + textSection("gen.pb.go", "var B = 2") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("gen.pb.go")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 1 { + t.Errorf("kept=%d dropped=%d, want 1 and 1", kept, dropped) + } + if got := out.String(); got != textSection("hand.go", "var A = 1") { + t.Errorf("unexpected output:\n%s", got) + } + }) + + t.Run("empty drop set is the identity", func(t *testing.T) { + t.Parallel() + patch := textSection("a.go", "x") + textSection("b.go", "y") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop()) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 2 || dropped != 0 || out.String() != patch { + t.Errorf("identity violated: kept=%d dropped=%d\n%s", kept, dropped, out.String()) + } + }) + + t.Run("rename drops as one piece via destination path", func(t *testing.T) { + t.Parallel() + patch := "diff --git a/old/gen.pb.go b/new/gen.pb.go\n" + + "similarity index 90%\n" + + "rename from old/gen.pb.go\n" + + "rename to new/gen.pb.go\n" + + "index 1111111..2222222 100644\n" + + "--- a/old/gen.pb.go\n" + + "+++ b/new/gen.pb.go\n" + + "@@ -1,1 +1,2 @@\n line\n+more\n" + + textSection("hand.go", "var A = 1") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("new/gen.pb.go")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 1 { + t.Errorf("kept=%d dropped=%d, want 1 and 1", kept, dropped) + } + if got := out.String(); strings.Contains(got, "old/gen.pb.go") || strings.Contains(got, "new/gen.pb.go") { + t.Errorf("rename section should drop entirely (both sides):\n%s", got) + } + }) + + t.Run("pure rename with no hunks drops at section boundary", func(t *testing.T) { + t.Parallel() + patch := "diff --git a/gen.pb.go b/moved/gen.pb.go\n" + + "similarity index 100%\n" + + "rename from gen.pb.go\n" + + "rename to moved/gen.pb.go\n" + + textSection("hand.go", "var A = 1") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("moved/gen.pb.go")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 1 || strings.Contains(out.String(), "rename") { + t.Errorf("kept=%d dropped=%d output:\n%s", kept, dropped, out.String()) + } + }) + + t.Run("binary section drops via diff --git fallback", func(t *testing.T) { + t.Parallel() + patch := "diff --git a/data/object_info.json.gz b/data/object_info.json.gz\n" + + "index 1111111..2222222 100644\n" + + "Binary files a/data/object_info.json.gz and b/data/object_info.json.gz differ\n" + + textSection("hand.go", "var A = 1") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("data/object_info.json.gz")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 1 || strings.Contains(out.String(), "object_info") { + t.Errorf("kept=%d dropped=%d output:\n%s", kept, dropped, out.String()) + } + }) + + t.Run("mode-only section decides at boundary", func(t *testing.T) { + t.Parallel() + patch := "diff --git a/tool.sh b/tool.sh\n" + + "old mode 100644\n" + + "new mode 100755\n" + + textSection("hand.go", "var A = 1") + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("tool.sh")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 1 || strings.Contains(out.String(), "tool.sh") { + t.Errorf("kept=%d dropped=%d output:\n%s", kept, dropped, out.String()) + } + }) + + t.Run("paths with spaces resolve via header equality", func(t *testing.T) { + t.Parallel() + // With core.quotePath=false git leaves a spacey path unquoted and + // appends a tab to the ---/+++ headers; parseDiffHeaderPath strips it. + patch := "diff --git a/my dir/gen file.go b/my dir/gen file.go\n" + + "index 1111111..2222222 100644\n" + + "--- a/my dir/gen file.go\t\n" + + "+++ b/my dir/gen file.go\t\n" + + "@@ -1,1 +1,2 @@\n line\n+more\n" + var out strings.Builder + _, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("my dir/gen file.go")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if dropped != 1 || out.String() != "" { + t.Errorf("spacey generated path should drop, output:\n%s", out.String()) + } + }) + + t.Run("unparseable quoted section is kept, not hidden", func(t *testing.T) { + t.Parallel() + patch := "diff --git \"a/we\\tird.go\" \"b/we\\tird.go\"\n" + + "index 1111111..2222222 100644\n" + + "--- \"a/we\\tird.go\"\n" + + "+++ \"b/we\\tird.go\"\n" + + "@@ -1,1 +1,2 @@\n line\n+more\n" + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, func(string) bool { return true }) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 0 || out.String() != patch { + t.Errorf("unparseable section must be kept verbatim, got kept=%d dropped=%d:\n%s", kept, dropped, out.String()) + } + }) + + t.Run("added content resembling headers stays content", func(t *testing.T) { + t.Parallel() + // Inside a hunk, +/- lines are content: an added line reading + // "+++ b/gen.pb.go" must not rebind the section's path. + patch := "diff --git a/hand.md b/hand.md\n" + + "index 1111111..2222222 100644\n" + + "--- a/hand.md\n" + + "+++ b/hand.md\n" + + "@@ -1,1 +1,3 @@\n line\n++++ b/gen.pb.go\n+diff --git a/x b/x\n" + var out strings.Builder + kept, dropped, err := FilterPatch(strings.NewReader(patch), &out, drop("gen.pb.go")) + if err != nil { + t.Fatalf("FilterPatch: %v", err) + } + if kept != 1 || dropped != 0 || out.String() != patch { + t.Errorf("content lines must not affect sectioning, got kept=%d dropped=%d:\n%s", kept, dropped, out.String()) + } + }) +} + +// TestParseDiffGitPaths pins the fallback parser's conservatism: unambiguous +// lines resolve, the equal-halves rule handles spacey non-renames, and quoted +// or ambiguous lines yield nil (the caller keeps those sections). +func TestParseDiffGitPaths(t *testing.T) { + t.Parallel() + tests := []struct { + name string + line string + want []string + }{ + {"simple", "diff --git a/x.go b/x.go", []string{"x.go"}}, + {"spacey same path", "diff --git a/my dir/f.gz b/my dir/f.gz", []string{"my dir/f.gz"}}, + {"rename single separator", "diff --git a/old.go b/new.go", []string{"old.go", "new.go"}}, + {"pathological b-slash name", "diff --git a/g.gz b/g.gz b/g.gz b/g.gz", []string{"g.gz b/g.gz"}}, + {"quoted gives up", `diff --git "a/we\tird" "b/we\tird"`, nil}, + {"not a diff line", "not a header", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := parseDiffGitPaths(tt.line) + if len(got) != len(tt.want) { + t.Fatalf("parseDiffGitPaths(%q) = %q, want %q", tt.line, got, tt.want) + } + for i := range tt.want { + if got[i] != tt.want[i] { + t.Errorf("parseDiffGitPaths(%q)[%d] = %q, want %q", tt.line, i, got[i], tt.want[i]) + } + } + }) + } +}