From d2e53fb8b12d5c5d148eddd7f309bfa716fc2355 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 18 Jul 2026 15:09:18 -0700 Subject: [PATCH 1/2] feat: add reusable PR-size cap workflow with per-repo inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the PR-size guardrail as a workflow_call workflow (pr-size.yml) plus its self-contained Go module (scripts/check-pr-size), so any repo can cap PR size with a thin SHA-pinned caller. Inputs (all optional): max_lines (1000), mode (enforce|warn — warn reports and comments but never fails, for gradual rollout), bypass_label (oversized-ok), extra_lockfiles, extra_generated_globs, comment, bot_app_id, workflows_ref. Secrets: BOT_APP_PRIVATE_KEY (pass-through app key for the sticky comment; absent credentials degrade to status + step summary only). The two-job security split is preserved: the size job runs against PR code with a read-only token and builds the checker from this repo's pinned checkout (no PR-authored code executes); the comment job holds the bot write token, checks out no PR code, and receives the report + over-cap flag as an artifact. Unit tests ported and extended (extras parsing, glob matching, warn/enforce contract, report rendering); zizmor and actionlint clean on both new workflows. --- .github/workflows/pr-size.yml | 303 +++++++++++++++++++++ .github/workflows/test-pr-size.yml | 55 ++++ README.md | 1 + scripts/check-pr-size/go.mod | 3 + scripts/check-pr-size/main.go | 330 +++++++++++++++++++++++ scripts/check-pr-size/main_test.go | 357 +++++++++++++++++++++++++ scripts/check-pr-size/size.go | 298 +++++++++++++++++++++ scripts/check-pr-size/size_test.go | 412 +++++++++++++++++++++++++++++ 8 files changed, 1759 insertions(+) create mode 100644 .github/workflows/pr-size.yml create mode 100644 .github/workflows/test-pr-size.yml create mode 100644 scripts/check-pr-size/go.mod create mode 100644 scripts/check-pr-size/main.go create mode 100644 scripts/check-pr-size/main_test.go create mode 100644 scripts/check-pr-size/size.go create mode 100644 scripts/check-pr-size/size_test.go diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml new file mode 100644 index 0000000..f2587a7 --- /dev/null +++ b/.github/workflows/pr-size.yml @@ -0,0 +1,303 @@ +name: PR Size Cap (reusable) + +# Reusable PR-size guardrail: counts a pull request's changed lines (added + +# deleted) across its net diff, excluding generated files and dependency +# lockfiles, and fails the check when the remainder exceeds `max_lines` — so +# diffs stay reviewable for humans and AI agents alike. +# +# What's excluded from the count: +# - dependency lockfiles (go.sum, package-lock.json, ... plus `extra_lockfiles`) +# - files marked `linguist-generated` in .gitattributes — read from the BASE +# ref only, so a PR cannot mark its own files generated to shrink its count +# - Go files carrying the canonical `// Code generated ... DO NOT EDIT.` +# marker before the package clause (this rule simply never matches in +# non-Go repos, so it is unconditional) +# - files matching `extra_generated_globs` +# +# Bypass: add the `bypass_label` (default `oversized-ok`) to the PR for a +# legitimately large change. The check still runs (so it always posts a +# status) and reports green. +# +# Rollout: `mode: warn` reports (and comments) on overage but never fails the +# check — use it to trial a cap on a repo before flipping to `enforce`. +# +# When a PR is over the cap and bot credentials are configured, the bot posts a +# single sticky comment explaining the overage and the bypass label, then flips +# it to ✅ once the PR is trimmed or labeled — the failing status alone does not +# surface the bypass label. Without `bot_app_id` + BOT_APP_PRIVATE_KEY, or with +# `comment: false`, the workflow degrades gracefully: the check status and the +# size job's step summary still carry the full report; no comment is posted. +# +# Two-job security split: the `pr-size` job runs against PR code with a +# read-only token; the `comment` job holds the bot write token, checks out NO +# PR code, and receives the report as an artifact — so a write-scoped token is +# never present in a job that touched PR-authored content. +# +# The counting logic + its unit tests live in scripts/check-pr-size/ in THIS +# repo; the size job builds it from the `workflows_ref` checkout, so consumer +# repos carry only a thin caller and there is no logic to drift. +# +# Caller pattern (place in consumer repo at .github/workflows/ci-pr-size.yml): +# +# name: CI - PR Size Cap +# on: +# pull_request: +# branches: [main] +# # labeled/unlabeled are included (beyond the default opened/synchronize/ +# # reopened) so toggling the bypass label re-runs the check immediately — +# # no dummy push needed to clear a failed run. +# types: [opened, synchronize, reopened, labeled, unlabeled] +# permissions: +# contents: read +# jobs: +# pr-size: +# uses: Comfy-Org/github-workflows/.github/workflows/pr-size.yml@ # v1 +# with: +# max_lines: 1000 +# # Pin the tool ref to the same ref you pin `uses:` to for +# # reproducibility (defaults to main). +# workflows_ref: +# # Optional: post the sticky comment under your GitHub App. +# bot_app_id: ${{ vars.APP_ID }} +# secrets: +# BOT_APP_PRIVATE_KEY: ${{ secrets.CLOUD_CODE_BOT_PRIVATE_KEY }} + +on: + workflow_call: + inputs: + max_lines: + description: Max non-generated changed lines (added + deleted) allowed. + type: number + required: false + default: 1000 + mode: + description: >- + `enforce` fails the check when a PR is over the cap; `warn` still + reports (and comments) on overage but never fails the check — for + gradual per-repo rollout. + type: string + required: false + default: enforce + bypass_label: + description: >- + PR label that bypasses the cap for a legitimately large change. + Create it in the consumer repo before enforcing. + type: string + required: false + default: oversized-ok + extra_lockfiles: + description: >- + Extra dependency-lockfile base names to exclude from the count, on + top of the 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: '' + extra_generated_globs: + description: >- + Extra glob patterns treated as generated (excluded from the count). + Whitespace- or comma-separated. `*` matches within a path segment, + `**` across segments, `?` one character. A pattern without `/` is + matched against the file's base name at any depth; one with `/` + against the full repo-relative path. + type: string + required: false + default: '' + comment: + description: >- + Post the sticky over-cap PR comment (needs bot_app_id + + BOT_APP_PRIVATE_KEY). Set false for status + step summary only. + type: boolean + required: false + default: true + bot_app_id: + description: >- + GitHub App ID used to post the sticky comment (paired with the + BOT_APP_PRIVATE_KEY secret). App IDs aren't secret, so this is an + input. Optional — when absent, the comment job degrades to status + + step summary only. + type: string + required: false + default: '' + workflows_ref: + description: >- + Ref of Comfy-Org/github-workflows to load the check-pr-size tool + from. Pin this to the same ref you pin `uses:` to for + reproducibility. + type: string + required: false + default: main + secrets: + BOT_APP_PRIVATE_KEY: + description: >- + PEM private key matching bot_app_id. Required only when bot_app_id + is set and comment is true. + required: false + +# Mapped from `inputs` here so the run steps below read them without per-step +# plumbing; the check-pr-size tool reads the PR_SIZE_* variables directly. +env: + PR_SIZE_MAX_LINES: ${{ inputs.max_lines }} + PR_SIZE_MODE: ${{ inputs.mode }} + PR_SIZE_BYPASS: ${{ contains(github.event.pull_request.labels.*.name, inputs.bypass_label) }} + PR_SIZE_BYPASS_LABEL: ${{ inputs.bypass_label }} + PR_SIZE_EXTRA_LOCKFILES: ${{ inputs.extra_lockfiles }} + PR_SIZE_EXTRA_GENERATED_GLOBS: ${{ inputs.extra_generated_globs }} + +jobs: + pr-size: + name: Cap PR size (LoC) + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + - name: Checkout PR head + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + # Full history for all refs, so the base SHA and the merge-base + # three-dot diff resolve without a separate authenticated fetch. + fetch-depth: 0 + persist-credentials: false + ref: ${{ github.event.pull_request.head.sha }} + + - name: Load check-pr-size tool + # The tool comes from THIS workflow's repo (public, pinned via + # workflows_ref) — never from the PR checkout, so no PR-authored code + # runs in this job. + 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 + # The module is dependency-free (no go.sum) — nothing to cache. + cache: false + + - name: Build check-pr-size + run: | + # Drop anything the PR checkout could have pre-seeded under the tool + # path (such files are untracked in the tool repo, which checkout + # does not remove), 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: Check PR size + id: check + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -o pipefail + "${RUNNER_TEMP}/check-pr-size" \ + --base "${BASE_SHA}" \ + --head "${HEAD_SHA}" | tee "${RUNNER_TEMP}/pr-size-report.md" + + - name: Record over-cap flag + # The comment job keys off this flag rather than the pr-size job's + # result, so `warn` mode (job green) still comments on overage. It + # rides the artifact, which — unlike job outputs — carries identically + # whether this job passed or failed. + if: always() + env: + OVER_CAP: ${{ steps.check.outputs.over_cap }} + run: printf '%s' "${OVER_CAP:-}" > "${RUNNER_TEMP}/pr-size-over-cap" + + # Hand the rendered report to the isolated `comment` job below. Uploaded + # as an artifact (not a job output) so the job that holds the bot + # write-token never runs in the same context as the PR checkout. + - name: Upload size report + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: pr-size-report + path: | + ${{ runner.temp }}/pr-size-report.md + ${{ runner.temp }}/pr-size-over-cap + if-no-files-found: warn + retention-days: 7 + + # Posts the size report as a single sticky PR comment when a PR is over the + # cap (and flips it to ✅ once the PR is trimmed or bypass-labeled), so + # contributors see WHY the check is red and how to clear it. Posting is keyed + # off the over-cap flag carried in the artifact, not the size job's result, + # so `warn` mode (job green) still comments on overage. + # + # Isolation: this job checks out NO PR code and only downloads the report + # artifact, so the bot's write-scoped token is never present in a job that + # held a PR checkout. + comment: + name: Comment when oversize + needs: pr-size + # always(): the interesting case is exactly when pr-size failed. + if: always() && inputs.comment && needs.pr-size.result != 'cancelled' && needs.pr-size.result != 'skipped' + runs-on: ubuntu-latest + permissions: + contents: read + env: + # The `secrets` context is unavailable in `if` conditions, so the + # credentials-present test is evaluated here (job env allows it) and the + # steps below gate on the result. With no bot credentials they no-op and + # the check status + step summary stay the only outputs. + BOT_CONFIGURED: ${{ inputs.bot_app_id != '' && secrets.BOT_APP_PRIVATE_KEY != '' }} + steps: + - name: Download size report + id: dl + continue-on-error: true + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: pr-size-report + path: report + + - name: Mint bot token + if: steps.dl.outcome == 'success' && env.BOT_CONFIGURED == 'true' + id: bot + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + app-id: ${{ inputs.bot_app_id }} + private-key: ${{ secrets.BOT_APP_PRIVATE_KEY }} + # Scope the minted token to comment upserts only, not the app's full + # installation permissions (PR comments ride the issues API). + permission-issues: write + permission-pull-requests: write + + - name: Upsert sticky size comment + if: steps.bot.outcome == 'success' + env: + GH_TOKEN: ${{ steps.bot.outputs.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + if [ ! -s report/pr-size-report.md ]; then + echo "Report artifact empty — the size tool crashed before rendering; leaving any existing comment untouched." + exit 0 + fi + OVER="$(cat report/pr-size-over-cap 2>/dev/null || true)" + MARKER='' + { printf '%s\n\n' "$MARKER"; cat report/pr-size-report.md; } > body.md + # Find our existing sticky comment (if any) by the hidden marker, so + # we update one comment across pushes instead of stacking new ones. + existing="$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ + --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id")" + existing="$(printf '%s' "$existing" | head -n1)" + if [ -n "$existing" ]; then + # Update in place — flips to ✅ when a previously-flagged PR is fixed. + gh api -X PATCH "repos/${REPO}/issues/comments/${existing}" -F body=@body.md >/dev/null + echo "Updated sticky size comment ${existing}." + elif [ "$OVER" = "true" ]; then + gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null + echo "Posted oversize comment." + else + echo "Under cap and no existing comment — nothing to post." + fi + + - name: Note degraded mode + if: steps.dl.outcome == 'success' && steps.bot.outcome == 'skipped' + run: echo "bot_app_id/BOT_APP_PRIVATE_KEY not configured — no PR comment; the report is in the pr-size job's step summary." diff --git a/.github/workflows/test-pr-size.yml b/.github/workflows/test-pr-size.yml new file mode 100644 index 0000000..2fb736e --- /dev/null +++ b/.github/workflows/test-pr-size.yml @@ -0,0 +1,55 @@ +name: Test pr-size script + +# Runs the Go unit tests (+ gofmt/vet) for the PR-size checker behind +# pr-size.yml. The checker gates every consumer repo's PRs, so a counting +# regression silently blocks (or waves through) PRs org-wide — cheap to guard +# with a unit run on change. + +on: + pull_request: + paths: + - 'scripts/check-pr-size/**' + - '.github/workflows/test-pr-size.yml' + push: + branches: [main] + paths: + - 'scripts/check-pr-size/**' + - '.github/workflows/test-pr-size.yml' + +permissions: + contents: read + +jobs: + test: + name: go test + runs-on: ubuntu-latest + defaults: + run: + working-directory: scripts/check-pr-size + steps: + - name: Checkout + uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + with: + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: scripts/check-pr-size/go.mod + # The module is dependency-free (no go.sum) — nothing to cache. + cache: false + + - name: gofmt + run: | + UNFORMATTED="$(gofmt -l .)" + if [ -n "$UNFORMATTED" ]; then + echo "gofmt needed on:" + echo "$UNFORMATTED" + exit 1 + fi + + - name: go vet + run: go vet ./... + + - name: go test + run: go test ./... diff --git a/README.md b/README.md index 216cb65..d49d5dd 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`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`. | +| [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | ## Usage diff --git a/scripts/check-pr-size/go.mod b/scripts/check-pr-size/go.mod new file mode 100644 index 0000000..de9f9a8 --- /dev/null +++ b/scripts/check-pr-size/go.mod @@ -0,0 +1,3 @@ +module github.com/Comfy-Org/github-workflows/scripts/check-pr-size + +go 1.26.4 diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go new file mode 100644 index 0000000..72ff355 --- /dev/null +++ b/scripts/check-pr-size/main.go @@ -0,0 +1,330 @@ +package main + +import ( + "flag" + "fmt" + "io" + "os" + "os/exec" + "strconv" + "strings" +) + +const ( + defaultMaxLines = 1000 + defaultBypassLabel = "oversized-ok" + + modeEnforce = "enforce" + modeWarn = "warn" + + // maxScanBytes bounds how much of a file we read looking for the generated + // marker (which sits at the top). It caps memory/time so a PR cannot point a + // "generated" path at an unbounded or blocking target and hang the job. + maxScanBytes = 4 << 20 // 4 MiB +) + +func main() { + base := flag.String("base", "", "base git ref to diff against (e.g. origin/main); required") + head := flag.String("head", "HEAD", "head git ref of the PR") + maxFlag := flag.Int("max", envInt("PR_SIZE_MAX_LINES", defaultMaxLines), "max non-generated changed lines allowed") + bypassFlag := flag.Bool("bypass", envBool("PR_SIZE_BYPASS"), "bypass the cap (set when the bypass label is present)") + modeFlag := flag.String("mode", envStr("PR_SIZE_MODE", modeEnforce), "'enforce' exits non-zero when over the cap; 'warn' reports without failing") + 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)") + flag.Parse() + + if *base == "" { + fmt.Fprintln(os.Stderr, "check-pr-size: --base is required") + os.Exit(2) + } + if *modeFlag != modeEnforce && *modeFlag != modeWarn { + fmt.Fprintf(os.Stderr, "check-pr-size: --mode must be %q or %q, got %q\n", modeEnforce, modeWarn, *modeFlag) + os.Exit(2) + } + extras, err := ParseExtras(*extraLockfiles, *extraGlobs) + if err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: %v\n", err) + os.Exit(2) + } + + files, err := diffFiles(*base, *head) + if err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: %v\n", err) + os.Exit(2) + } + + // The linguist-generated attribute is read from the BASE ref, never the PR + // head, so a PR cannot add `* linguist-generated=true` to .gitattributes and + // shrink its own counted LoC. Two defense-in-depth conditions disable the + // attribute path entirely (unless the bypass label is present): git too old + // to honor `check-attr --source`, or the PR diff touching .gitattributes at + // all. See attrGenerated / TouchesGitattributes. + attr := attrPolicy{source: *base, useSource: checkAttrSourceSupported(*base)} + attrModified := TouchesGitattributes(files) + attr.trusted = attrTrusted(attr.useSource, attrModified, *bypassFlag) + classify(files, *base, *head, attr, extras) + + res := Evaluate(files, *maxFlag, *bypassFlag) + if !res.OK && !attr.trusted { + // The check is failing and we ignored linguist-generated exclusions; tell + // the contributor every reason so that dropping one (e.g. the .gitattributes + // edit) does not surprise them with the next (an old-git runner). + var reasons []string + if attrModified { + reasons = append(reasons, "this PR modifies `.gitattributes`") + } + if !attr.useSource { + reasons = append(reasons, "this CI runner's git is too old to read `linguist-generated` from the base ref") + } + if len(reasons) > 0 { + res.Note = fmt.Sprintf("`linguist-generated` exclusions were not applied because %s. Add the `%s` label if this large change is intentional.", strings.Join(reasons, " and "), *bypassLabel) + } + } + report(res, *modeFlag, *bypassLabel) + writeGitHubOutputs(res) + + if shouldFail(res, *modeFlag) { + os.Exit(1) + } +} + +// 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. +func shouldFail(res Result, mode string) bool { + return !res.OK && mode == modeEnforce +} + +// diffFiles runs `git diff --numstat` for the PR's net changes (three-dot: from +// the merge-base of base and head, to head) and parses the result. +func diffFiles(base, head string) ([]FileChange, error) { + out, err := runGit("diff", "--numstat", "-z", base+"..."+head) + if err != nil { + return nil, fmt.Errorf("git diff failed: %w", err) + } + return ParseNumstat(strings.NewReader(out)) +} + +// 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 +// >=2.40); trusted is whether attribute-based exclusion may be applied at all +// (false disables it, so a PR-introduced rule cannot shrink the count). +type attrPolicy struct { + source string + useSource bool + trusted bool +} + +// attrTrusted decides whether linguist-generated attribute exclusions may be +// applied. It always requires useSource, so attributes are only ever read from +// the base ref via `--source` and never from the PR-head working tree. The +// bypass label may override the .gitattributes-touched gate, but must NOT +// re-enable head-controlled reading on a runner too old for `--source` — doing +// so would make the reported excluded/counted numbers head-manipulable, +// breaking the base-only invariant even though the check passes on bypass. +func attrTrusted(useSource, attrModified, bypass bool) bool { + return useSource && (!attrModified || bypass) +} + +// classify sets FileChange.Generated for each file using, in order: the +// lockfile name lists (built-in + extras), the caller-supplied generated globs, +// the linguist-generated git attribute (read from the base ref per attr), and +// the canonical Go generated marker in the file's content. +func classify(files []FileChange, base, head string, attr attrPolicy, extras Extras) { + for i := range files { + f := &files[i] + if f.Binary { + continue + } + if IsLockfile(f.Path) || extras.Generated(f.Path) || + (attr.trusted && attrGenerated(f.Path, attr.source, attr.useSource)) || + contentGenerated(f.Path, base, head) { + f.Generated = true + } + } +} + +// attrGenerated reports whether .gitattributes marks the path linguist-generated. +// When useSource is set, the attribute rules are read from source (the base ref) +// via `git check-attr --source`, so .gitattributes rules introduced by the PR +// head are never consulted — a PR cannot mark arbitrary hand-written files +// generated to escape the size count. `--source` needs git >= 2.40 (GitHub +// runners ship newer); on older git useSource is false and the attribute path is +// left untrusted (see attrPolicy) rather than reading the PR checkout. +func attrGenerated(path, source string, useSource bool) bool { + args := []string{"check-attr", "linguist-generated"} + if useSource { + args = append(args, "--source", source) + } + args = append(args, "--", path) + out, err := runGit(args...) + if err != nil { + return false + } + // Format: ": linguist-generated: ". git reports "true" for an + // explicit "=true" and "set" for a bare attribute; both mean generated. + s := strings.TrimSpace(out) + return strings.HasSuffix(s, ": true") || strings.HasSuffix(s, ": set") +} + +// checkAttrSourceSupported reports whether the installed git honors +// `check-attr --source` (added in git 2.40). Older git rejects the flag as an +// unknown option and exits non-zero, so we probe once rather than parse the +// version string. A false result forces the attribute path onto its untrusted +// fallback (see attrPolicy). The probe path need not exist; check-attr resolves +// rules for arbitrary path strings. +func checkAttrSourceSupported(source string) bool { + _, err := runGit("check-attr", "--source", source, "linguist-generated", "--", ".gitattributes") + return err == nil +} + +// contentGenerated reports whether a .go file carries Go's canonical generated +// marker before its package clause. Only .go files are consulted: the marker's +// anti-gaming guarantee relies on the package-clause gate (see IsGeneratedContent), +// which non-Go files lack, so a contributor cannot exclude a large hand-written +// 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 { + if !strings.HasSuffix(path, ".go") { + return false + } + if info, err := os.Lstat(path); err == nil { + if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { + return false + } + f, err := os.Open(path) + if err != nil { + return false + } + defer f.Close() + data, err := io.ReadAll(io.LimitReader(f, maxScanBytes)) + if err != nil { + return false + } + return IsGeneratedContent(data) + } + // Not in the working tree (e.g. deleted): read the head/base blob so a + // shrinking regen of a generated file is still classified as generated. + for _, ref := range []string{head, base} { + if out, err := runGit("show", ref+":"+path); err == nil { + return IsGeneratedContent([]byte(out)) + } + } + return false +} + +// renderReport builds the markdown report for stdout, the step summary, and the +// sticky PR comment. +func renderReport(res Result, mode, bypassLabel string) string { + var b strings.Builder + status := "✅ Passed" + if !res.OK { + if mode == modeWarn { + status = "⚠️ Over cap (warn-only)" + } else { + status = "❌ Failed" + } + } + fmt.Fprintf(&b, "## %s — PR size check\n\n", status) + 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.Bypassed { + fmt.Fprintf(&b, "- Bypassed via `%s` label ✅\n", bypassLabel) + } + if !res.OK { + fmt.Fprintf(&b, "\n**This PR changes %d lines of hand-written code, over the %d-line cap.**\n\n", res.Counted, res.Max) + if mode == modeWarn { + b.WriteString("This check runs in `warn` mode, so it will not fail — but consider:\n") + } else { + b.WriteString("Options:\n") + } + b.WriteString("- Split it into smaller, independently reviewable PRs (stacked PRs help).\n") + fmt.Fprintf(&b, "- If the size is justified, add the `%s` label to bypass this check.\n", bypassLabel) + } + if res.Note != "" { + fmt.Fprintf(&b, "\n> %s\n", res.Note) + } + // Largest contributing files, for quick triage. + shown := 0 + var top strings.Builder + for _, f := range res.Files { + if f.Generated || f.Changed() == 0 { + continue + } + if shown == 0 { + top.WriteString("\n
Largest counted files\n\n") + } + fmt.Fprintf(&top, "- `%s` (+%d/-%d)\n", f.Path, f.Added, f.Deleted) + shown++ + if shown >= 10 { + break + } + } + if shown > 0 { + top.WriteString("\n
\n") + b.WriteString(top.String()) + } + return b.String() +} + +func report(res Result, mode, bypassLabel string) { + summary := renderReport(res, mode, bypassLabel) + fmt.Println(summary) + if path := os.Getenv("GITHUB_STEP_SUMMARY"); path != "" { + if f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644); err == nil { + defer f.Close() + fmt.Fprintln(f, summary) + } + } +} + +// writeGitHubOutputs exposes the evaluation to later workflow steps via +// GITHUB_OUTPUT. over_cap drives the sticky-comment job, which must post on +// overage even in warn mode, where this process exits 0 and the job result +// alone cannot distinguish over from under. No-op outside GitHub Actions. +func writeGitHubOutputs(res Result) { + path := os.Getenv("GITHUB_OUTPUT") + if path == "" { + return + } + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644) + if err != nil { + return + } + defer f.Close() + fmt.Fprintf(f, "over_cap=%t\ncounted=%d\n", !res.OK, res.Counted) +} + +func runGit(args ...string) (string, error) { + cmd := exec.Command("git", args...) + out, err := cmd.Output() + if err != nil { + return "", err + } + return string(out), nil +} + +func envInt(key string, def int) int { + if v := os.Getenv(key); v != "" { + if n, err := strconv.Atoi(v); err == nil { + return n + } + } + return def +} + +func envBool(key string) bool { + b, _ := strconv.ParseBool(os.Getenv(key)) + return b +} + +func envStr(key, def string) string { + if v := os.Getenv(key); v != "" { + return v + } + return def +} diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go new file mode 100644 index 0000000..d30d9fe --- /dev/null +++ b/scripts/check-pr-size/main_test.go @@ -0,0 +1,357 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// These tests exercise the git-backed generated-file classification against a +// real, throwaway repository so the `.gitattributes` / `check-attr --source` +// behavior is verified end to end rather than mocked. They are not +// t.Parallel(): they t.Chdir into the temp repo (the production helpers resolve +// git against the process working directory), and t.Chdir forbids parallel use. + +func gitRun(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v failed: %v\n%s", args, err, out) + } + return string(out) +} + +// initTestRepo creates an empty git repo with a committer identity configured. +func initTestRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + gitRun(t, dir, "init", "-q") + gitRun(t, dir, "config", "user.email", "test@example.com") + gitRun(t, dir, "config", "user.name", "Test") + gitRun(t, dir, "config", "commit.gpgsign", "false") + return dir +} + +// writeFile writes name (relative to dir), creating parent directories. +func writeFile(t *testing.T, dir, name, content string) { + t.Helper() + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// commitAll stages everything and commits, returning the new commit's SHA. +func commitAll(t *testing.T, dir, msg string) string { + t.Helper() + gitRun(t, dir, "add", "-A") + gitRun(t, dir, "commit", "-q", "-m", msg) + return strings.TrimSpace(gitRun(t, dir, "rev-parse", "HEAD")) +} + +func TestTouchesGitattributes(t *testing.T) { + t.Parallel() + tests := []struct { + name string + files []FileChange + want bool + }{ + {"root gitattributes", []FileChange{{Path: ".gitattributes"}}, true}, + {"nested gitattributes", []FileChange{{Path: "vendor/.gitattributes"}}, true}, + {"no gitattributes", []FileChange{{Path: "main.go"}, {Path: "dir/x.go"}}, false}, + {"lookalike suffix is not a match", []FileChange{{Path: "my.gitattributes"}}, false}, + {"empty", nil, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := TouchesGitattributes(tt.files); got != tt.want { + t.Errorf("TouchesGitattributes(%+v) = %v, want %v", tt.files, got, tt.want) + } + }) + } +} + +// TestAttrGeneratedSourceIsolatesBase proves attrGenerated reads .gitattributes +// from the base tree (useSource=true), not the working tree (the PR head). It +// checks both directions: a base rule the head removed is still honored via +// --source, and a rule only the head adds is ignored via --source but WOULD +// have been honored reading the working tree. +func TestAttrGeneratedSourceIsolatesBase(t *testing.T) { + // Direction 1: base HAS the rule, head REMOVED it. + t.Run("base rule honored despite head removal", func(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, ".gitattributes", "foo.go linguist-generated=true\n") + writeFile(t, dir, "foo.go", "package foo\n") + base := commitAll(t, dir, "base with rule") + writeFile(t, dir, ".gitattributes", "") // head drops the rule + commitAll(t, dir, "head drops rule") // working tree now at head + t.Chdir(dir) + + if !checkAttrSourceSupported(base) { + t.Skip("git too old for check-attr --source") + } + if !attrGenerated("foo.go", base, true) { + t.Error("attrGenerated should read the base rule via --source") + } + // Reading the working tree (head) must NOT see the removed rule. + if attrGenerated("foo.go", base, false) { + t.Error("attrGenerated without --source read the head tree, expected no rule") + } + }) + + // Direction 2: base has NO rule, head ADDED one (the attack). --source must + // ignore it; reading the working tree would (unsafely) honor it. + t.Run("head-added rule ignored via source", func(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "foo.go", "package foo\n") + base := commitAll(t, dir, "base no rule") + writeFile(t, dir, ".gitattributes", "*.go linguist-generated=true\n") + commitAll(t, dir, "head adds rule") // working tree now at head + t.Chdir(dir) + + if !checkAttrSourceSupported(base) { + t.Skip("git too old for check-attr --source") + } + if attrGenerated("foo.go", base, true) { + t.Error("attrGenerated via --source must not see the head-introduced rule") + } + if !attrGenerated("foo.go", base, false) { + t.Error("sanity: reading the working tree should see the head rule (the vulnerability)") + } + }) +} + +// TestClassifyPRAddedGitattributesDoesNotReduceCount is the anti-gaming +// regression: a PR that adds `*.go linguist-generated=true` to .gitattributes +// must not shrink the counted lines of its hand-written .go changes. +func TestClassifyPRAddedGitattributesDoesNotReduceCount(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "hand.go", "package main\n\nfunc F() {}\n") + base := commitAll(t, dir, "base") + + // PR head: sneak in an attribute rule AND a large hand-written change. + writeFile(t, dir, ".gitattributes", "*.go linguist-generated=true\n") + writeFile(t, dir, "hand.go", "package main\n\n"+strings.Repeat("// padding line\n", 40)+"func F() {}\n") + head := commitAll(t, dir, "head") + t.Chdir(dir) + + files, err := diffFiles(base, head) + if err != nil { + t.Fatalf("diffFiles: %v", err) + } + attr := attrPolicy{source: base, useSource: checkAttrSourceSupported(base)} + attr.trusted = attrTrusted(attr.useSource, TouchesGitattributes(files), false) + classify(files, base, head, attr, Extras{}) + + for _, f := range files { + if f.Path == "hand.go" && f.Generated { + t.Error("hand.go was excluded by a PR-introduced .gitattributes rule") + } + } + res := Evaluate(files, 1000, false) + if res.Counted == 0 { + t.Errorf("counted lines should include hand.go's changes, got %d", res.Counted) + } +} + +// TestClassifyHonorsLegitBaseGitattributes proves the base-only reading does +// not break legitimate attribute-based exclusion: a rule already present in the +// base ref (and not modified by the PR) still excludes a matching file. +func TestClassifyHonorsLegitBaseGitattributes(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, ".gitattributes", "generated/** linguist-generated=true\n") + writeFile(t, dir, "generated/api.go", "package generated\n") + writeFile(t, dir, "hand.go", "package main\n") + base := commitAll(t, dir, "base") + + // PR head changes only source files, NOT .gitattributes. + writeFile(t, dir, "generated/api.go", "package generated\n\n"+strings.Repeat("// gen line\n", 30)) + writeFile(t, dir, "hand.go", "package main\n\n"+strings.Repeat("// hand line\n", 5)) + head := commitAll(t, dir, "head") + t.Chdir(dir) + + if !checkAttrSourceSupported(base) { + t.Skip("git too old for check-attr --source") + } + files, err := diffFiles(base, head) + if err != nil { + t.Fatalf("diffFiles: %v", err) + } + attr := attrPolicy{source: base, useSource: true} + attr.trusted = !TouchesGitattributes(files) + if !attr.trusted { + t.Fatal("attribute path should be trusted (PR does not touch .gitattributes)") + } + classify(files, base, head, attr, Extras{}) + + var genExcluded, handCounted bool + for _, f := range files { + if f.Path == "generated/api.go" && f.Generated { + genExcluded = true + } + if f.Path == "hand.go" && !f.Generated { + handCounted = true + } + } + if !genExcluded { + t.Error("generated/api.go should be excluded by the legit base .gitattributes rule") + } + if !handCounted { + t.Error("hand.go should still be counted") + } +} + +// 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 +// attribute path is skipped — classify needs no repo. +func TestClassifyAppliesExtras(t *testing.T) { + t.Parallel() + extras, err := ParseExtras("Gemfile.lock", "*.gen.ts web/snapshots/**") + if err != nil { + t.Fatalf("ParseExtras: %v", err) + } + files := []FileChange{ + {Path: "app/Gemfile.lock", Added: 500}, + {Path: "web/src/api.gen.ts", Added: 800}, + {Path: "web/snapshots/a/b.snap", Added: 900}, + {Path: "web/src/hand.ts", Added: 40}, + } + classify(files, "", "", attrPolicy{}, extras) + + wantGenerated := map[string]bool{ + "app/Gemfile.lock": true, + "web/src/api.gen.ts": true, + "web/snapshots/a/b.snap": true, + "web/src/hand.ts": false, + } + for _, f := range files { + if f.Generated != wantGenerated[f.Path] { + t.Errorf("%s: Generated = %v, want %v", f.Path, f.Generated, wantGenerated[f.Path]) + } + } + res := Evaluate(files, 1000, false) + if res.Counted != 40 { + t.Errorf("Counted = %d, want 40", res.Counted) + } + if res.Generated != 2200 { + t.Errorf("Generated = %d, want 2200", res.Generated) + } +} + +// TestAttrTrusted pins the base-only invariant: attribute exclusions are trusted +// only when git can read attributes from the base ref (useSource), and the bypass +// label may lift the .gitattributes-touched gate but must NEVER re-enable +// head-controlled reading on an old-git runner (useSource=false). +func TestAttrTrusted(t *testing.T) { + tests := []struct { + name string + useSource bool + attrModified bool + bypass bool + want bool + }{ + {"modern git, clean", true, false, false, true}, + {"modern git, gitattributes edited", true, true, false, false}, + {"modern git, gitattributes edited, bypass overrides gate", true, true, true, true}, + {"modern git, clean, bypass", true, false, true, true}, + {"old git never trusted", false, false, false, false}, + // The regression: bypass must not re-enable head reading on old git. + {"old git, bypass does NOT re-enable head reading", false, false, true, false}, + {"old git, gitattributes edited, bypass", false, true, true, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := attrTrusted(tt.useSource, tt.attrModified, tt.bypass); got != tt.want { + t.Errorf("attrTrusted(useSource=%v, attrModified=%v, bypass=%v) = %v, want %v", + tt.useSource, tt.attrModified, tt.bypass, got, tt.want) + } + }) + } +} + +// TestShouldFail pins the mode contract: enforce fails on overage, warn never +// fails, and a bypassed result never fails in either mode. +func TestShouldFail(t *testing.T) { + t.Parallel() + tests := []struct { + name string + res Result + mode string + want bool + }{ + {"enforce over cap fails", Result{OK: false}, modeEnforce, true}, + {"enforce under cap passes", Result{OK: true}, modeEnforce, false}, + {"warn over cap does not fail", Result{OK: false}, modeWarn, false}, + {"warn under cap does not fail", Result{OK: true}, modeWarn, false}, + {"enforce bypassed does not fail", Result{OK: true, Bypassed: true}, modeEnforce, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := shouldFail(tt.res, tt.mode); got != tt.want { + t.Errorf("shouldFail(%+v, %q) = %v, want %v", tt.res, tt.mode, got, tt.want) + } + }) + } +} + +// TestRenderReport checks the mode- and label-sensitive report copy: warn mode +// shows the warning header and never the failure one, the bypass label name is +// the configured one, and the over-cap guidance appears whenever over. +func TestRenderReport(t *testing.T) { + t.Parallel() + over := Result{Counted: 1500, Max: 1000, OK: false, Files: []FileChange{{Path: "big.go", Added: 1500}}} + under := Result{Counted: 10, Max: 1000, OK: true} + bypassed := Result{Counted: 1500, Max: 1000, OK: true, Bypassed: true} + + t.Run("enforce over cap", func(t *testing.T) { + t.Parallel() + got := renderReport(over, modeEnforce, "huge-ok") + for _, want := range []string{"❌ Failed", "over the 1000-line cap", "`huge-ok` label", "Largest counted files"} { + if !strings.Contains(got, want) { + t.Errorf("report missing %q:\n%s", want, got) + } + } + }) + t.Run("warn over cap", func(t *testing.T) { + t.Parallel() + got := renderReport(over, modeWarn, "oversized-ok") + if !strings.Contains(got, "⚠️ Over cap (warn-only)") { + t.Errorf("warn report missing warn header:\n%s", got) + } + if strings.Contains(got, "❌ Failed") { + t.Errorf("warn report must not claim failure:\n%s", got) + } + if !strings.Contains(got, "`warn` mode") { + t.Errorf("warn report should explain warn mode:\n%s", got) + } + }) + t.Run("under cap", func(t *testing.T) { + t.Parallel() + got := renderReport(under, modeEnforce, "oversized-ok") + if !strings.Contains(got, "✅ Passed") { + t.Errorf("under-cap report missing pass header:\n%s", got) + } + if strings.Contains(got, "Options:") { + t.Errorf("under-cap report must not include over-cap guidance:\n%s", got) + } + }) + t.Run("bypassed", func(t *testing.T) { + t.Parallel() + got := renderReport(bypassed, modeEnforce, "oversized-ok") + for _, want := range []string{"✅ Passed", "Bypassed via `oversized-ok` label"} { + if !strings.Contains(got, want) { + t.Errorf("bypassed report missing %q:\n%s", want, got) + } + } + }) +} diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go new file mode 100644 index 0000000..2b41f22 --- /dev/null +++ b/scripts/check-pr-size/size.go @@ -0,0 +1,298 @@ +// Command check-pr-size caps a pull request's size, measured in lines of code +// changed, so diffs stay reviewable for humans and AI agents alike. +// +// It counts added + deleted lines across the PR diff, EXCLUDING generated files +// (codegen can emit huge amounts of code that would trip the cap unfairly), and +// fails if the remaining count exceeds a configurable ceiling. A PR label +// provides an explicit bypass for legitimate large changes. +// +// This file holds the pure, side-effect-free logic (diff parsing, generated-file +// classification, cap evaluation) so it can be unit tested without a git repo; +// main.go wires it to git and the CI environment. +package main + +import ( + "bytes" + "fmt" + "io" + "regexp" + "sort" + "strconv" + "strings" + "unicode" +) + +// generatedMarker matches Go's canonical generated-file header. A file counts as +// generated only when this appears BEFORE its package clause — the same rule the +// go toolchain uses, so a contributor cannot opt hand-written code out of the +// count by pasting the marker mid-file. +var generatedMarker = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.$`) + +// lockfileNames are dependency lockfiles: machine-maintained, frequently huge, +// and not hand-reviewed line by line. They rarely carry a generated marker, so +// they are matched by base name. Per-repo additions come in via Extras. +var lockfileNames = map[string]bool{ + "go.sum": true, + "go.work.sum": true, + "package-lock.json": true, + "pnpm-lock.yaml": true, + "yarn.lock": true, + "Cargo.lock": true, + "poetry.lock": true, + "uv.lock": true, +} + +// FileChange is one file's contribution to the diff. +type FileChange struct { + Path string + Added int + Deleted int + Binary bool + Generated bool +} + +// Changed returns the line count this file contributes to PR size (added + +// deleted). Binary files contribute nothing (they are not lines of code). +func (f FileChange) Changed() int { + if f.Binary { + return 0 + } + return f.Added + f.Deleted +} + +// 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. + 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. + Note string +} + +// ParseNumstat parses the output of `git diff --numstat -z`. Records are +// NUL-delimited and, crucially, paths are emitted verbatim (no C-style quoting +// of spaces/UTF-8, unlike the newline form), so a lockfile or generated file +// with an unusual name is classified correctly. Each record is +// "\t\t", with "-\t-" counts marking a binary file. A +// rename/copy is emitted as "\t\t" (empty path field) followed +// by two extra NUL-terminated tokens — the old path then the new path; the new +// path is kept so classification reads the file at its post-diff location. +func ParseNumstat(r io.Reader) ([]FileChange, error) { + data, err := io.ReadAll(r) + if err != nil { + return nil, err + } + tokens := strings.Split(string(data), "\x00") + var changes []FileChange + for i := 0; i < len(tokens); i++ { + rec := tokens[i] + if rec == "" { + continue // trailing NUL or stray separator + } + parts := strings.SplitN(rec, "\t", 3) + if len(parts) != 3 { + return nil, fmt.Errorf("malformed numstat record: %q", rec) + } + path := parts[2] + if path == "" { + // Rename/copy: the following two tokens are old path, then new path. + // A missing/empty new-path token means a truncated stream. + if i+2 >= len(tokens) || tokens[i+2] == "" { + return nil, fmt.Errorf("truncated rename record: %q", rec) + } + path = tokens[i+2] + i += 2 + } + fc := FileChange{Path: path} + if parts[0] == "-" || parts[1] == "-" { + fc.Binary = true + changes = append(changes, fc) + continue + } + added, err := strconv.Atoi(parts[0]) + if err != nil { + return nil, fmt.Errorf("bad added count in %q: %w", rec, err) + } + deleted, err := strconv.Atoi(parts[1]) + if err != nil { + return nil, fmt.Errorf("bad deleted count in %q: %w", rec, err) + } + fc.Added = added + fc.Deleted = deleted + changes = append(changes, fc) + } + return changes, nil +} + +// IsGeneratedContent reports whether Go source carries the canonical generated +// marker before its package clause. The content is scanned from memory with no +// line-length limit, so a very long line before the package clause cannot cause +// a false negative. A nil/empty read returns false. Callers restrict this to +// .go files (see contentGenerated in main.go): the package-clause gate is what +// stops a contributor opting a hand-written file out of the count by pasting the +// marker mid-file, and non-Go files have no package clause to anchor that. +func IsGeneratedContent(content []byte) bool { + for len(content) > 0 { + var line []byte + if i := bytes.IndexByte(content, '\n'); i >= 0 { + line, content = content[:i], content[i+1:] + } else { + line, content = content, nil + } + s := strings.TrimRight(string(line), "\r") + if strings.HasPrefix(s, "package ") { + return false + } + if generatedMarker.MatchString(s) { + return true + } + } + return false +} + +// IsLockfile reports whether the path is one of the built-in dependency +// lockfiles. Per-repo additions are handled by Extras.Generated. +func IsLockfile(path string) bool { + return lockfileNames[baseName(path)] +} + +// baseName returns the final path segment of a slash-separated path. +func baseName(path string) string { + if slash := strings.LastIndex(path, "/"); slash >= 0 { + return path[slash+1:] + } + return path +} + +// Extras carries per-repo additions to the exclusion rules, parsed from the +// reusable workflow's extra_lockfiles / extra_generated_globs inputs. +type Extras struct { + lockfiles map[string]bool + globs []extraGlob +} + +type extraGlob struct { + re *regexp.Regexp + // baseOnly marks a pattern with no '/': it matches the file's base name at + // any depth (like a .gitignore basename pattern) instead of the full path. + baseOnly bool +} + +// splitList splits a workflow-input list on whitespace and commas, so folded +// YAML scalars and comma lists both work. +func splitList(s string) []string { + return strings.FieldsFunc(s, func(r rune) bool { + return r == ',' || unicode.IsSpace(r) + }) +} + +// ParseExtras parses the extra_lockfiles and extra_generated_globs inputs. +// Lockfile entries must be base names — matching mirrors the built-in list, +// which excludes a lockfile at any directory depth, so a path would silently +// never match. +func ParseExtras(lockfiles, globs string) (Extras, error) { + var e Extras + for _, name := range splitList(lockfiles) { + if strings.Contains(name, "/") { + return Extras{}, fmt.Errorf("extra lockfile %q must be a base name, not a path", name) + } + if e.lockfiles == nil { + e.lockfiles = map[string]bool{} + } + e.lockfiles[name] = true + } + for _, pattern := range splitList(globs) { + e.globs = append(e.globs, extraGlob{ + re: globRegexp(pattern), + baseOnly: !strings.Contains(pattern, "/"), + }) + } + return e, nil +} + +// Generated reports whether path matches the per-repo extra exclusion rules: +// its base name is an extra lockfile, or it matches an extra generated glob. +func (e Extras) Generated(path string) bool { + base := baseName(path) + if e.lockfiles[base] { + return true + } + for _, g := range e.globs { + target := path + if g.baseOnly { + target = base + } + if g.re.MatchString(target) { + return true + } + } + return false +} + +// 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. +func globRegexp(pattern string) *regexp.Regexp { + var b strings.Builder + b.WriteString(`^`) + for i := 0; i < len(pattern); i++ { + switch pattern[i] { + case '*': + if i+1 < len(pattern) && pattern[i+1] == '*' { + b.WriteString(`.*`) + i++ + } else { + b.WriteString(`[^/]*`) + } + case '?': + b.WriteString(`[^/]`) + default: + b.WriteString(regexp.QuoteMeta(pattern[i : i+1])) + } + } + b.WriteString(`$`) + return regexp.MustCompile(b.String()) +} + +// TouchesGitattributes reports whether any changed file is a .gitattributes file +// (at the repo root or in any subdirectory). A PR that edits .gitattributes can +// introduce linguist-generated rules, so this drives the attribute path's +// defense-in-depth fallback in main.go (see attrPolicy). +func TouchesGitattributes(files []FileChange) bool { + for _, f := range files { + if baseName(f.Path) == ".gitattributes" { + return true + } + } + return false +} + +// Evaluate sums the changed lines of non-generated files and compares against +// max. A file's Generated field must already be set by the caller. When bypassed +// is true the result is always OK, but the counts are still reported. +func Evaluate(files []FileChange, max int, bypassed bool) Result { + // Copy before sorting so we honor the file header's "side-effect-free" + // contract and never reorder the caller's slice in place. + sorted := make([]FileChange, len(files)) + copy(sorted, files) + res := Result{Max: max, Bypassed: bypassed, Files: sorted} + for _, f := range sorted { + if f.Generated { + res.Generated += f.Changed() + continue + } + res.Counted += f.Changed() + } + 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 +} diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go new file mode 100644 index 0000000..7c25eb9 --- /dev/null +++ b/scripts/check-pr-size/size_test.go @@ -0,0 +1,412 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// nul builds a NUL-terminated `git diff --numstat -z` stream from records. Each +// record is a preformatted "\t\t" (or the two extra +// old/new path tokens of a rename); every token is terminated by a NUL. +func nul(records ...string) string { + if len(records) == 0 { + return "" + } + return strings.Join(records, "\x00") + "\x00" +} + +func TestParseNumstat(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input string + want []FileChange + wantErr bool + }{ + { + name: "simple add and delete", + input: nul("10\t5\tpkg/foo/foo.go", "0\t3\tbar.go"), + want: []FileChange{ + {Path: "pkg/foo/foo.go", Added: 10, Deleted: 5}, + {Path: "bar.go", Added: 0, Deleted: 3}, + }, + }, + { + name: "binary file", + input: nul("-\t-\tassets/logo.png"), + want: []FileChange{{Path: "assets/logo.png", Binary: true}}, + }, + { + name: "stray empty tokens ignored", + input: "\x00" + nul("2\t2\ta.go"), + want: []FileChange{{Path: "a.go", Added: 2, Deleted: 2}}, + }, + { + // -z renames: empty path field, then old-path and new-path tokens. + name: "rename resolves to new path", + input: nul("1\t1\t", "dir/old/file.go", "dir/new/file.go"), + want: []FileChange{{Path: "dir/new/file.go", Added: 1, Deleted: 1}}, + }, + { + // A file whose literal name contains " => " must NOT be treated as a + // rename (regression: the old newline parser rewrote it to "go.sum"). + name: "literal arrow in filename is preserved", + input: nul("1\t0\tnotes => go.sum"), + want: []FileChange{{Path: "notes => go.sum", Added: 1, Deleted: 0}}, + }, + { + // Braces in a legit filename must not trigger rename rewriting. + name: "literal braces in filename are preserved", + input: nul("3\t0\tconfig/{settings}.go"), + want: []FileChange{{Path: "config/{settings}.go", Added: 3, Deleted: 0}}, + }, + { + // -z emits UTF-8/space paths verbatim (no C-style quoting). + name: "unicode path kept verbatim", + input: nul("1\t0\tcafé/go.sum"), + want: []FileChange{{Path: "café/go.sum", Added: 1, Deleted: 0}}, + }, + { + name: "malformed record errors", + input: nul("not-a-numstat-record"), + wantErr: true, + }, + { + name: "non-numeric count errors", + input: nul("x\t2\tfoo.go"), + wantErr: true, + }, + { + name: "truncated rename errors", + input: nul("1\t1\t", "only-old-path"), + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ParseNumstat(strings.NewReader(tt.input)) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != len(tt.want) { + t.Fatalf("got %d files, want %d: %+v", len(got), len(tt.want), got) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("file %d = %+v, want %+v", i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestIsGeneratedContent(t *testing.T) { + t.Parallel() + tests := []struct { + name string + body string + want bool + }{ + { + name: "oapi-codegen header before package", + body: "// Package api ...\n//\n// Code generated by oapi-codegen version v2.5.0 DO NOT EDIT.\npackage api\n", + want: true, + }, + { + name: "ent header on first line", + body: "// Code generated by ent, DO NOT EDIT.\n\npackage database\n", + want: true, + }, + { + name: "marker after package clause does not count", + body: "package foo\n\n// Code generated by evil DO NOT EDIT.\nfunc F() {}\n", + want: false, + }, + { + name: "hand-written file", + body: "package foo\n\nfunc Bar() {}\n", + want: false, + }, + { + name: "CRLF line endings still match", + body: "// Code generated by ent, DO NOT EDIT.\r\n\r\npackage database\r\n", + want: true, + }, + { + name: "empty content", + body: "", + want: false, + }, + { + // A line longer than bufio.Scanner's default 64KB token before the + // package clause must not cause a false negative (regression). + name: "very long line before marker still matches", + body: "// " + strings.Repeat("x", 100*1024) + "\n// Code generated by tool DO NOT EDIT.\npackage foo\n", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := IsGeneratedContent([]byte(tt.body)); got != tt.want { + t.Errorf("IsGeneratedContent() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsLockfile(t *testing.T) { + t.Parallel() + tests := []struct { + path string + want bool + }{ + {"go.sum", true}, + {"go.work.sum", true}, + {"pkg/foo/go.sum", true}, + {"frontend/package-lock.json", true}, + {"pnpm-lock.yaml", true}, + {"go.mod", false}, + {"pkg/foo/main.go", false}, + } + for _, tt := range tests { + if got := IsLockfile(tt.path); got != tt.want { + t.Errorf("IsLockfile(%q) = %v, want %v", tt.path, got, tt.want) + } + } +} + +func TestParseExtras(t *testing.T) { + t.Parallel() + t.Run("empty inputs yield inert extras", func(t *testing.T) { + t.Parallel() + e, err := ParseExtras("", "") + if err != nil { + t.Fatalf("ParseExtras: %v", err) + } + if e.Generated("anything.go") || e.Generated("Gemfile.lock") { + t.Error("empty extras must match nothing") + } + }) + t.Run("whitespace and comma separated lists both parse", func(t *testing.T) { + t.Parallel() + e, err := ParseExtras("Gemfile.lock, composer.lock\nPipfile.lock", "*.pb.go, gen/**") + if err != nil { + t.Fatalf("ParseExtras: %v", err) + } + for _, p := range []string{"Gemfile.lock", "sub/dir/composer.lock", "Pipfile.lock", "a/b/x.pb.go", "gen/deep/file.ts"} { + if !e.Generated(p) { + t.Errorf("Generated(%q) = false, want true", p) + } + } + }) + t.Run("lockfile path is rejected", func(t *testing.T) { + t.Parallel() + if _, err := ParseExtras("vendor/Gemfile.lock", ""); err == nil { + t.Error("expected error for a path-shaped lockfile entry") + } + }) +} + +func TestExtrasGenerated(t *testing.T) { + t.Parallel() + tests := []struct { + name string + lockfiles string + globs string + path string + want bool + }{ + {"extra lockfile at root", "Gemfile.lock", "", "Gemfile.lock", true}, + {"extra lockfile at depth", "Gemfile.lock", "", "app/Gemfile.lock", true}, + {"extra lockfile no substring match", "Gemfile.lock", "", "Gemfile.lock.bak", false}, + {"basename glob matches at any depth", "", "*.gen.ts", "web/src/api.gen.ts", true}, + {"basename glob star stays in segment", "", "*.gen.ts", "web/api.gen.ts/other.txt", false}, + {"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}, + {"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}, + {"regex metachars are literal", "", "a+b.txt", "a+b.txt", true}, + {"regex metachars do not repeat", "", "a+b.txt", "aab.txt", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e, err := ParseExtras(tt.lockfiles, tt.globs) + if err != nil { + t.Fatalf("ParseExtras: %v", err) + } + if got := e.Generated(tt.path); got != tt.want { + t.Errorf("Generated(%q) = %v, want %v", tt.path, got, tt.want) + } + }) + } +} + +func TestEvaluate(t *testing.T) { + t.Parallel() + tests := []struct { + name string + files []FileChange + max int + bypassed bool + wantCounted int + wantGenerated int + wantOK bool + }{ + { + name: "under cap passes", + files: []FileChange{ + {Path: "a.go", Added: 100, Deleted: 50}, + {Path: "b.go", Added: 20, Deleted: 0}, + }, + max: 1000, + wantCounted: 170, + wantOK: true, + }, + { + name: "over cap fails", + files: []FileChange{ + {Path: "a.go", Added: 800, Deleted: 400}, + }, + max: 1000, + wantCounted: 1200, + wantOK: false, + }, + { + name: "generated files excluded from count", + files: []FileChange{ + {Path: "svc.gen.go", Added: 5000, Deleted: 2000, Generated: true}, + {Path: "hand.go", Added: 100, Deleted: 10}, + }, + max: 1000, + wantCounted: 110, + wantGenerated: 7000, + wantOK: true, + }, + { + name: "binary files contribute nothing", + files: []FileChange{ + {Path: "logo.png", Binary: true}, + {Path: "hand.go", Added: 30, Deleted: 0}, + }, + max: 1000, + wantCounted: 30, + wantOK: true, + }, + { + name: "bypass label passes even when over cap", + files: []FileChange{ + {Path: "a.go", Added: 9000, Deleted: 0}, + }, + max: 1000, + bypassed: true, + wantCounted: 9000, + wantOK: true, + }, + { + name: "exactly at cap passes", + files: []FileChange{ + {Path: "a.go", Added: 1000, Deleted: 0}, + }, + max: 1000, + wantCounted: 1000, + wantOK: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := Evaluate(tt.files, tt.max, tt.bypassed) + if got.Counted != tt.wantCounted { + t.Errorf("Counted = %d, want %d", got.Counted, tt.wantCounted) + } + if got.Generated != tt.wantGenerated { + t.Errorf("Generated = %d, want %d", got.Generated, tt.wantGenerated) + } + if got.OK != tt.wantOK { + t.Errorf("OK = %v, want %v", got.OK, tt.wantOK) + } + }) + } +} + +func TestEvaluateSortsFilesByChangedDescending(t *testing.T) { + t.Parallel() + res := Evaluate([]FileChange{ + {Path: "small.go", Added: 1, Deleted: 0}, + {Path: "big.go", Added: 500, Deleted: 100}, + {Path: "mid.go", Added: 50, Deleted: 0}, + }, 1000, false) + wantOrder := []string{"big.go", "mid.go", "small.go"} + for i, w := range wantOrder { + if res.Files[i].Path != w { + t.Errorf("Files[%d] = %q, want %q", i, res.Files[i].Path, w) + } + } +} + +func TestEvaluateDoesNotMutateCallerSlice(t *testing.T) { + t.Parallel() + files := []FileChange{ + {Path: "small.go", Added: 1}, + {Path: "big.go", Added: 500}, + } + _ = Evaluate(files, 1000, false) + if files[0].Path != "small.go" || files[1].Path != "big.go" { + t.Errorf("Evaluate reordered the caller's slice: %+v", files) + } +} + +func TestContentGeneratedOnlyTrustsGoFiles(t *testing.T) { + t.Parallel() + dir := t.TempDir() + marker := []byte("// Code generated by tool DO NOT EDIT.\npackage x\n") + + goFile := filepath.Join(dir, "x.go") + if err := os.WriteFile(goFile, marker, 0o644); err != nil { + t.Fatal(err) + } + if !contentGenerated(goFile, "", "") { + t.Errorf("a .go file with the generated marker should be generated") + } + + // A non-Go file with the same marker pasted at the top must NOT be excluded: + // it has no package clause, so the anti-gaming guarantee does not hold. + mdFile := filepath.Join(dir, "x.md") + if err := os.WriteFile(mdFile, marker, 0o644); err != nil { + t.Fatal(err) + } + if contentGenerated(mdFile, "", "") { + t.Errorf("a non-.go file with a pasted marker must not be treated as generated") + } +} + +func TestContentGeneratedRejectsSymlink(t *testing.T) { + t.Parallel() + dir := t.TempDir() + target := filepath.Join(dir, "real.go") + if err := os.WriteFile(target, []byte("// Code generated by tool DO NOT EDIT.\npackage x\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.go") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unsupported: %v", err) + } + // The DoS guard must refuse to follow a symlink, even one pointing at a + // legitimately generated file. + if contentGenerated(link, "", "") { + t.Errorf("contentGenerated must not follow symlinks") + } +} From 2eecbcdeb0489537696776e3db2a6a54b2277c43 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 20 Jul 2026 23:36:08 -0700 Subject: [PATCH 2/2] fix(check-pr-size): cap the deleted-file blob read in contentGenerated The working-tree read path was capped at maxScanBytes to guard against an oversized/blocking target, but the git-blob fallback for a deleted file used runGit (unbounded cmd.Output()), contradicting that guarantee. Add runGitCapped and use it for the fallback. --- scripts/check-pr-size/main.go | 41 ++++++++++++++++++++++++++++-- scripts/check-pr-size/main_test.go | 40 +++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 72ff355..60d69e2 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -208,9 +208,11 @@ func contentGenerated(path, base, head string) bool { } // Not in the working tree (e.g. deleted): read the head/base blob so a // shrinking regen of a generated file is still classified as generated. + // Capped at maxScanBytes just like the working-tree read above, so a + // deleted path pointing at an oversized blob can't balloon job memory. for _, ref := range []string{head, base} { - if out, err := runGit("show", ref+":"+path); err == nil { - return IsGeneratedContent([]byte(out)) + if data, err := runGitCapped(maxScanBytes, "show", ref+":"+path); err == nil { + return IsGeneratedContent(data) } } return false @@ -308,6 +310,41 @@ func runGit(args ...string) (string, error) { return string(out), nil } +// runGitCapped runs git and reads at most maxBytes of its stdout, killing the +// process rather than blocking on Wait() if it had more to write. Used for +// reads of ref-addressed blobs (e.g. `git show :`) whose size we +// don't control, so a single oversized blob can't spike job memory the way an +// unbounded cmd.Output() would. +func runGitCapped(maxBytes int64, args ...string) ([]byte, error) { + cmd := exec.Command("git", args...) + stdout, err := cmd.StdoutPipe() + if err != nil { + return nil, err + } + if err := cmd.Start(); err != nil { + return nil, err + } + data, readErr := io.ReadAll(io.LimitReader(stdout, maxBytes)) + if readErr != nil { + _ = cmd.Process.Kill() + _ = cmd.Wait() + return nil, readErr + } + // Drain-probe for leftover output beyond the cap; if the process still has + // more to write, kill it instead of letting Wait() block on a full pipe. + extra := make([]byte, 1) + n, _ := stdout.Read(extra) + if n > 0 { + _ = cmd.Process.Kill() + _ = cmd.Wait() + return data, nil + } + if err := cmd.Wait(); err != nil { + return nil, err + } + return data, nil +} + func envInt(key string, def int) int { if v := os.Getenv(key); v != "" { if n, err := strconv.Atoi(v); err == nil { diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index d30d9fe..9f847ab 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -355,3 +355,43 @@ func TestRenderReport(t *testing.T) { } }) } + +// TestContentGeneratedDeletedFileReadsBlob proves the deleted-file fallback +// (the file no longer exists in the working tree, so contentGenerated falls +// back to reading the head/base git blob) still classifies correctly. +func TestContentGeneratedDeletedFileReadsBlob(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "gen.go", "// Code generated by tool DO NOT EDIT.\npackage x\n") + base := commitAll(t, dir, "add generated file") + if err := os.Remove(filepath.Join(dir, "gen.go")); err != nil { + t.Fatal(err) + } + head := commitAll(t, dir, "delete generated file") + t.Chdir(dir) + + // 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) { + t.Error("a deleted generated file should still classify as generated via the base blob fallback") + } +} + +// TestRunGitCappedLimitsBlobRead is the regression for the deleted-file +// fallback's DoS guard: contentGenerated's working-tree read is capped at +// maxScanBytes, and the git-blob fallback (for a path no longer in the +// working tree) must be capped the same way rather than buffering an +// unbounded blob via a plain cmd.Output(). +func TestRunGitCappedLimitsBlobRead(t *testing.T) { + dir := initTestRepo(t) + writeFile(t, dir, "big.txt", strings.Repeat("x", 10_000)) + sha := commitAll(t, dir, "add big file") + t.Chdir(dir) + + data, err := runGitCapped(100, "show", sha+":big.txt") + if err != nil { + t.Fatalf("runGitCapped: %v", err) + } + if len(data) != 100 { + t.Errorf("len(data) = %d, want 100 (capped read)", len(data)) + } +}