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 35c11e8..daff69e 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). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | 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..60d69e2 --- /dev/null +++ b/scripts/check-pr-size/main.go @@ -0,0 +1,367 @@ +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. + // 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 data, err := runGitCapped(maxScanBytes, "show", ref+":"+path); err == nil { + return IsGeneratedContent(data) + } + } + 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 +} + +// 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 { + 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..9f847ab --- /dev/null +++ b/scripts/check-pr-size/main_test.go @@ -0,0 +1,397 @@ +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) + } + } + }) +} + +// 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)) + } +} 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") + } +}