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