From 09530f435c973411e837b056bd2b27912a8e7a32 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Thu, 6 Aug 2026 19:42:10 -0700 Subject: [PATCH 1/9] feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The size cap counts added + deleted lines across a PR's net diff and already excludes generated files and dependency lockfiles. Test files were not excluded, so a change that is mostly test coverage trips the cap even when the production diff is small — and the only escape was the `oversized-ok` bypass label, which signals "legitimately large" rather than "mostly tests". Add an `exclude_tests` input (default false) that keeps test-file lines out of the counted total. Test files are recognized by path convention: `*_test.go`; `test_*.py` / `*_test.py` / `conftest.py`; `.test.` / `.spec.` before a JS/TS source extension; and any file under a `test/`, `tests/`, `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` directory segment. Off by default deliberately. `bump-callers.sh` auto-opens SHA-bump PRs into every calling repo, so a default-on change would propagate a weaker guardrail fleet-wide with nobody opting in. Per-repo knobs are the established shape here (`extra_lockfiles`, `extra_generated_globs`, `mode: warn`). Detection is a naming convention, not a proof — unlike the Go generated marker, which must precede the package clause, and `linguist-generated`, read from the base ref so a PR cannot exempt itself. Nothing stops production code being parked in `tests/`. Two mitigations: the opt-in itself, and the excluded total is ALWAYS reported on its own line (`Excluded (tests): N`), so a large test-only PR is visible rather than silently small. When the knob is off, test lines are still broken out of the counted number so the option is discoverable. Matching is on whole path segments and separator-anchored suffixes, so `contest/`, `attestation/`, `latest.go`, `protest.go` and `manifest.ts` are untouched. `spec/` is deliberately NOT a test directory — in this org it holds OpenAPI schemas, which are production artifacts. `Evaluate` now takes a `Policy` struct rather than a third positional bool; adjacent bool parameters are silently swappable at a call site. The three buckets never overlap — a generated file that is also a test counts once, as generated — so counted + generated + test always sums to the diff's non-binary changed lines regardless of policy. Verified by building the tool and running it against the real diff of the internal PR that prompted this: 1678 counted today (over the 1000 cap), 396 counted with the flag on, 1282 reported as test. An OpenAPI schema in that diff correctly stayed counted, confirming the `spec/` decision. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 27 ++++++ README.md | 2 +- docs/callers/pr-size.md | 22 ++++- scripts/check-pr-size/main.go | 33 +++++-- scripts/check-pr-size/main_test.go | 100 ++++++++++++++++++- scripts/check-pr-size/size.go | 150 ++++++++++++++++++++++++++--- scripts/check-pr-size/size_test.go | 137 +++++++++++++++++++++++++- 7 files changed, 446 insertions(+), 25 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 98657e7..c1eb2cf 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -13,6 +13,18 @@ name: PR Size Cap (reusable) # marker before the package clause (this rule simply never matches in # non-Go repos, so it is unconditional) # - files matching `extra_generated_globs` +# - test files, ONLY when `exclude_tests: true` (off by default) — see below +# +# `exclude_tests` (opt-in, per repo): keeps test-file lines out of the counted +# total so a PR is capped on the production code a reviewer must actually reason +# about, not on its test coverage. Test files are recognized by naming +# convention only (`*_test.go`, `test_*.py`/`*_test.py`/`conftest.py`, +# `*.test.ts`/`*.spec.ts` & friends, and paths under `test/`, `tests/`, +# `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/`, +# `__snapshots__/`). Unlike the generated-file rules this is a convention, not a +# proof — nothing stops production code being parked in `tests/` — which is why +# it is opt-in rather than the default. The excluded total is ALWAYS reported on +# its own line, so a large test-only PR cannot pass unremarked. # # 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 @@ -104,6 +116,20 @@ on: type: string required: false default: '' + exclude_tests: + description: >- + Keep test-file lines out of the counted total, so the cap measures + production code rather than test coverage. Test files are matched by + naming convention (`*_test.go`, `test_*.py`, `*.test.ts`, + `*.spec.ts`, and paths under test/, tests/, testing/, testdata/, + e2e/, __tests__/, __mocks__/, __snapshots__/). Off by default: it + loosens the cap, so each repo opts in deliberately. Excluded test + lines are always reported separately, never silently dropped. For a + layout these conventions miss, use `extra_generated_globs` (those + land in the generated bucket instead). + type: boolean + required: false + default: false comment: description: >- Post the sticky over-cap PR comment (needs bot_app_id + @@ -144,6 +170,7 @@ env: PR_SIZE_BYPASS_LABEL: ${{ inputs.bypass_label }} PR_SIZE_EXTRA_LOCKFILES: ${{ inputs.extra_lockfiles }} PR_SIZE_EXTRA_GENERATED_GLOBS: ${{ inputs.extra_generated_globs }} + PR_SIZE_EXCLUDE_TESTS: ${{ inputs.exclude_tests }} jobs: pr-size: diff --git a/README.md b/README.md index cc79724..97caeed 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ complete, copy-pasteable caller. | [`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`. | [cursor-review-auto-label.md](docs/callers/cursor-review-auto-label.md) | | [`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-reviewers.md](docs/callers/assign-reviewers.md) | | [`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`. | [assign-prs-to-author.md](docs/callers/assign-prs-to-author.md) | -| [`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). | [pr-size.md](docs/callers/pr-size.md) | +| [`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`. Opt in to `exclude_tests` to cap production code rather than test coverage (excluded test lines are always reported, never silently dropped). 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). | [pr-size.md](docs/callers/pr-size.md) | | [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, so it is the only surface that needs an extra `checks: write` grant in the caller's block, and only when switched on. Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and its **shape is enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA, so a branch, a tag or a `refs/pull/N/head` is rejected and the grader cannot be loaded from a floating ref after the caller was reviewed. **That is the whole of what is machine-checked, and it is not provenance.** Shape says the ref is immutable, never *which* commit it is: a fork of this public repo shares its object store, so a fork-authored SHA — or a pin left behind when `uses:` moved — is just as well-shaped. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). **Reviewing the caller is what bounds it, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | | [`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`. | [stale.md](docs/callers/stale.md) | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index aca64f0..30fb2e2 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -10,7 +10,8 @@ keep diffs reviewable. In `mode: warn` it reports without failing. Excluded from the count: dependency lockfiles, `linguist-generated` files (read from the **base ref**, so a PR cannot exempt itself by editing `.gitattributes`), Go generated-code markers, and anything you add via -`extra_lockfiles` / `extra_generated_globs`. +`extra_lockfiles` / `extra_generated_globs`. Optionally test files too — see +`exclude_tests` below. The counting logic and its tests live in [`scripts/check-pr-size/`](../../scripts/check-pr-size) and are compiled from @@ -65,6 +66,7 @@ contents: read | `bypass_label` | `oversized-ok` | Waves through a legitimately large change. | | `extra_lockfiles` | `''` | Additional lockfiles to exclude. | | `extra_generated_globs` | `''` | Additional generated-path globs to exclude. | +| `exclude_tests` | `false` | Keep test-file lines out of the count — cap production code, not coverage. Always reported separately. | | `comment` | `true` | Sticky bot comment explaining an overage. | | `bot_app_id` | `''` | Without it, degrades to status + step summary. | | `workflows_ref` | `main` | **Set to your `uses:` SHA** — the tool is built from this ref. | @@ -85,6 +87,24 @@ enforce. alone does not mention `oversized-ok`; the sticky comment is what tells an author the escape hatch exists. Supply the App or expect confused authors. +**`exclude_tests` is a naming convention, not a proof.** Unlike the +generated-file rules — which require Go's marker *before* the package clause, +and read `.gitattributes` from the base ref precisely so a PR cannot exempt +itself — test detection only looks at the path. Nothing stops production code +being parked in `tests/` to duck the cap. That is why it is off by default, and +why the excluded total is always printed on its own line: the report shows +`Excluded (tests): N` next to the counted number, so a 5,000-line "test-only" PR +is visible rather than silently small. Recognized: `*_test.go`; `test_*.py`, +`*_test.py`, `conftest.py`; `*.test.*` / `*.spec.*` for `.js .jsx .mjs .cjs .ts +.tsx .mts .cts`; and any file under a `test/`, `tests/`, `testing/`, +`testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` **directory** +segment. `spec/` is deliberately *not* a test directory — in this org it holds +OpenAPI schemas, which are production artifacts. For a layout these miss, add +`extra_generated_globs` (they land in the generated bucket instead). + +Leaving it off is a real choice, not just the safe one: a 5,000-line test diff +is genuinely slow to review, and the cap is the only thing that says so. + **Go workspaces:** a consumer with a root `go.work` needs `GOWORK=off` for the tool build, since `go build` otherwise discovers the consumer's workspace. diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 753a814..1ed3a6c 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -38,6 +38,7 @@ func main() { 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") + excludeTests := flag.Bool("exclude-tests", envBool("PR_SIZE_EXCLUDE_TESTS"), "keep test-file lines out of the counted total (they are still reported)") 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() @@ -73,7 +74,7 @@ func main() { attr.trusted = attrTrusted(attr.useSource, attrModified, *bypassFlag) classify(files, *base, *head, attr, extras) - res := Evaluate(files, *maxFlag, *bypassFlag) + res := Evaluate(files, Policy{Max: *maxFlag, Bypassed: *bypassFlag, ExcludeTests: *excludeTests}) 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 @@ -139,7 +140,10 @@ func attrTrusted(useSource, attrModified, bypass bool) bool { // 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. +// the canonical Go generated marker in the file's content. It also sets +// FileChange.Test from the path's naming convention (see IsTestPath) — always, +// regardless of policy, so the report can show the test total whether or not the +// caller opted to exclude it. // // The linguist-generated attribute is resolved for every non-binary path in a // SINGLE `git check-attr` pass (attrGeneratedBatch) rather than one subprocess @@ -160,6 +164,7 @@ func classify(files []FileChange, base, head string, attr attrPolicy, extras Ext if f.Binary { continue } + f.Test = IsTestPath(f.Path) if IsLockfile(f.Path) || extras.Generated(f.Path) || attrGen[f.Path] || contentGenerated(f.Path, base, head) { @@ -301,15 +306,29 @@ func renderReport(res Result, mode, bypassLabel string) string { status = "❌ Failed" } } + // What "counted" means depends on the policy, so the label and the over-cap + // sentence both spell it out — a number whose meaning shifted between repos + // (or between runs) is worse than no number. + counted, handWritten := "non-generated", "hand-written" + if res.TestsExcluded { + counted, handWritten = "non-generated, non-test", "hand-written non-test" + } 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, "- Changed lines counted (%s): **%d**\n", counted, res.Counted) fmt.Fprintf(&b, "- Cap: **%d**\n", res.Max) fmt.Fprintf(&b, "- Excluded (generated/lockfiles): %d\n", res.Generated) + // Always surfaced, both ways round: an exclusion nobody can see is how a + // large test-only PR sails through unremarked. + if res.TestsExcluded { + fmt.Fprintf(&b, "- Excluded (tests): %d\n", res.Test) + } else if res.Test > 0 { + fmt.Fprintf(&b, "- Of the counted lines, %d are tests (`exclude_tests` is off)\n", res.Test) + } 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) + fmt.Fprintf(&b, "\n**This PR changes %d lines of %s code, over the %d-line cap.**\n\n", res.Counted, handWritten, res.Max) if mode == modeWarn { b.WriteString("This check runs in `warn` mode, so it will not fail — but consider:\n") } else { @@ -321,11 +340,13 @@ func renderReport(res Result, mode, bypassLabel string) string { if res.Note != "" { fmt.Fprintf(&b, "\n> %s\n", res.Note) } - // Largest contributing files, for quick triage. + // Largest contributing files, for quick triage. Only files that actually + // contribute to Counted are listed, so the list always adds up to the number + // above it. shown := 0 var top strings.Builder for _, f := range res.Files { - if f.Generated || f.Changed() == 0 { + if f.Generated || f.Changed() == 0 || (res.TestsExcluded && f.Test) { continue } if shown == 0 { diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index fb195bd..e9c6620 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -253,7 +253,7 @@ func TestClassifyPRAddedGitattributesDoesNotReduceCount(t *testing.T) { t.Error("hand.go was excluded by a PR-introduced .gitattributes rule") } } - res := Evaluate(files, 1000, false) + res := Evaluate(files, Policy{Max: 1000}) if res.Counted == 0 { t.Errorf("counted lines should include hand.go's changes, got %d", res.Counted) } @@ -335,7 +335,7 @@ func TestClassifyAppliesExtras(t *testing.T) { t.Errorf("%s: Generated = %v, want %v", f.Path, f.Generated, wantGenerated[f.Path]) } } - res := Evaluate(files, 1000, false) + res := Evaluate(files, Policy{Max: 1000}) if res.Counted != 40 { t.Errorf("Counted = %d, want 40", res.Counted) } @@ -453,6 +453,102 @@ func TestRenderReport(t *testing.T) { }) } +// TestRenderReportTestLines is the visibility guarantee for the exclusion: the +// test total must appear in the report BOTH ways round — as an explicit +// exclusion when opted in (so a large test-only PR cannot pass unremarked), and +// as a breakdown of the counted number when not (so the knob is discoverable). +func TestRenderReportTestLines(t *testing.T) { + t.Parallel() + files := []FileChange{ + {Path: "hand.go", Added: 300, Deleted: 36}, + {Path: "hand_test.go", Added: 1200, Deleted: 33, Test: true}, + } + + t.Run("excluded tests are reported and kept out of the file list", func(t *testing.T) { + t.Parallel() + got := renderReport(Evaluate(files, Policy{Max: 1000, ExcludeTests: true}), modeEnforce, "oversized-ok") + for _, want := range []string{ + "✅ Passed", + "Changed lines counted (non-generated, non-test): **336**", + "Excluded (tests): 1233", + } { + if !strings.Contains(got, want) { + t.Errorf("report missing %q:\n%s", want, got) + } + } + // The "largest counted files" list must add up to the counted number, so + // an excluded test file has no business in it. + if strings.Contains(got, "hand_test.go") { + t.Errorf("excluded test file must not appear in the largest-counted list:\n%s", got) + } + }) + + t.Run("counted tests are broken out without changing the verdict", func(t *testing.T) { + t.Parallel() + got := renderReport(Evaluate(files, Policy{Max: 1000}), modeEnforce, "oversized-ok") + for _, want := range []string{ + "❌ Failed", + "Changed lines counted (non-generated): **1569**", + "1233 are tests (`exclude_tests` is off)", + "1569 lines of hand-written code", + } { + if !strings.Contains(got, want) { + t.Errorf("report missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "Excluded (tests)") { + t.Errorf("tests are counted here, so nothing may claim they were excluded:\n%s", got) + } + }) + + t.Run("no test bullet when a PR has no test changes", func(t *testing.T) { + t.Parallel() + got := renderReport(Evaluate(files[:1], Policy{Max: 1000}), modeEnforce, "oversized-ok") + if strings.Contains(got, "are tests") { + t.Errorf("a PR with no test changes should not get a test bullet:\n%s", got) + } + }) +} + +// TestClassifySetsTestRegardlessOfPolicy proves classification is policy-free: +// Test is set from the path alone, so Evaluate can report the total whether or +// not the caller opted to exclude it. Non-.go paths keep git out of the picture +// (contentGenerated never consults it) and attr.trusted is false, so classify +// needs no repo. +func TestClassifySetsTestRegardlessOfPolicy(t *testing.T) { + t.Parallel() + files := []FileChange{ + {Path: "web/src/Button.test.tsx", Added: 400}, + {Path: "web/src/__tests__/render.tsx", Added: 200}, + {Path: "web/src/Button.tsx", Added: 60}, + {Path: "web/src/manifest.ts", Added: 40}, + } + classify(files, "", "", attrPolicy{}, Extras{}) + + wantTest := map[string]bool{ + "web/src/Button.test.tsx": true, + "web/src/__tests__/render.tsx": true, + "web/src/Button.tsx": false, + "web/src/manifest.ts": false, + } + for _, f := range files { + if f.Test != wantTest[f.Path] { + t.Errorf("%s: Test = %v, want %v", f.Path, f.Test, wantTest[f.Path]) + } + if f.Generated { + t.Errorf("%s: nothing here is generated, got Generated = true", f.Path) + } + } + + // Same classification, two policies: only the accounting moves. + if res := Evaluate(files, Policy{Max: 1000}); res.Counted != 700 || res.Test != 600 { + t.Errorf("default policy: Counted = %d (want 700), Test = %d (want 600)", res.Counted, res.Test) + } + if res := Evaluate(files, Policy{Max: 1000, ExcludeTests: true}); res.Counted != 100 || res.Test != 600 { + t.Errorf("exclude policy: Counted = %d (want 100), Test = %d (want 600)", res.Counted, res.Test) + } +} + // 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. diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 2b41f22..6806014 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -3,12 +3,15 @@ // // 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. +// fails if the remaining count exceeds a configurable ceiling. Opting in to +// Policy.ExcludeTests additionally keeps test-file lines out of the count, so a +// mostly-test PR is judged on the production code a reviewer must actually +// reason about. 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. +// and test-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 ( @@ -42,6 +45,48 @@ var lockfileNames = map[string]bool{ "uv.lock": true, } +// testPathSegments are directory names that mark everything beneath them as +// test code. Matching is on whole, slash-delimited path SEGMENTS and never on +// substrings, so `contest/`, `attestation/` and `latest.go` are untouched. Only +// directory segments are considered (never the file name itself), so a +// hand-written file literally named `test` is still counted. +// +// `spec`/`specs` are deliberately absent: in this org those hold API schemas +// (OpenAPI), which are production artifacts. The unambiguous `*.spec.ts` +// file-name convention is handled below instead. +var testPathSegments = map[string]bool{ + "__mocks__": true, // Jest/Vitest manual mocks + "__snapshots__": true, // Jest/Vitest snapshots + "__tests__": true, // Jest/Vitest + "e2e": true, + "test": true, // also covers Maven/Gradle's src/test/... + "testdata": true, // Go's fixture convention + "testing": true, + "tests": true, +} + +// testFileSuffixes are base-name suffixes that mark test code. Each carries its +// own leading separator, so a production file that merely ENDS in the word +// (`latest.go`, `contest.py`) is not a match. +var testFileSuffixes = []string{ + "_test.go", // Go + "_test.py", // Python, suffix style +} + +// testFileNames are exact base names that are test scaffolding wherever they sit. +var testFileNames = map[string]bool{ + "conftest.py": true, // pytest fixture module — no production role +} + +// jsTestExts are the JavaScript/TypeScript source extensions on which the +// `.test.` / `.spec.` infix convention applies (`Button.test.tsx`, +// `api.spec.ts`). The infix is matched on the stem with its leading dot, so a +// hand-written module literally named `spec.ts` is not a match. +var jsTestExts = map[string]bool{ + ".cjs": true, ".cts": true, ".js": true, ".jsx": true, + ".mjs": true, ".mts": true, ".ts": true, ".tsx": true, +} + // FileChange is one file's contribution to the diff. type FileChange struct { Path string @@ -49,6 +94,7 @@ type FileChange struct { Deleted int Binary bool Generated bool + Test bool } // Changed returns the line count this file contributes to PR size (added + @@ -60,13 +106,26 @@ func (f FileChange) Changed() int { return f.Added + f.Deleted } +// Policy is the configuration Evaluate applies to a classified diff. It is a +// struct rather than positional arguments because the knobs are booleans, which +// are silently swappable at a call site. +type Policy struct { + Max int // the ceiling on counted lines + Bypassed bool // a bypass label was present + ExcludeTests bool // subtract test-file lines from the counted total +} + // 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 + Test int // changed lines in test files (excluded only if TestsExcluded) Max int // the configured ceiling Bypassed bool // a bypass label was present OK bool // Bypassed OR Counted <= Max + // TestsExcluded records whether Policy.ExcludeTests was set, so the report + // can say whether Test lines were subtracted from Counted or are part of it. + TestsExcluded bool // Files sorted by descending Changed(), for reporting. Files []FileChange // Note is an optional human-facing explanation appended to the report (e.g. @@ -169,6 +228,60 @@ func baseName(path string) string { return path } +// IsTestPath reports whether a repo-relative path is test code, by naming +// convention: it sits under a test directory segment, or its base name matches a +// per-language test-file convention. +// +// Unlike the generated-file rules this is a CONVENTION check, not a proof — the +// content is never consulted, so nothing stops a contributor from parking +// production code in `tests/`. That is exactly why excluding test lines from the +// cap is opt-in per repo (`exclude_tests`) rather than the default, and why the +// excluded total is always reported rather than silently dropped. +func IsTestPath(path string) bool { + return hasTestSegment(path) || isTestFileName(baseName(path)) +} + +// hasTestSegment reports whether any DIRECTORY segment of path is a test +// directory. The final segment (the file name) is excluded so the rule cannot +// fire on a file that merely shares a name with a test directory. +func hasTestSegment(path string) bool { + dir := path + slash := strings.LastIndex(dir, "/") + if slash < 0 { + return false // no directory part at all + } + for _, seg := range strings.Split(dir[:slash], "/") { + if testPathSegments[seg] { + return true + } + } + return false +} + +// isTestFileName reports whether a file's base name follows a test-file naming +// convention: an exact scaffolding name, a language suffix, Python's `test_` +// prefix, or the JS/TS `.test.`/`.spec.` infix before a source extension. +func isTestFileName(base string) bool { + if testFileNames[base] { + return true + } + for _, suffix := range testFileSuffixes { + if strings.HasSuffix(base, suffix) { + return true + } + } + if strings.HasPrefix(base, "test_") && strings.HasSuffix(base, ".py") { + return true + } + if dot := strings.LastIndex(base, "."); dot > 0 && jsTestExts[base[dot:]] { + stem := base[:dot] + if strings.HasSuffix(stem, ".test") || strings.HasSuffix(stem, ".spec") { + return true + } + } + return false +} + // Extras carries per-repo additions to the exclusion rules, parsed from the // reusable workflow's extra_lockfiles / extra_generated_globs inputs. type Extras struct { @@ -275,22 +388,35 @@ func TouchesGitattributes(files []FileChange) bool { } // 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 { +// the cap. A file's Generated and Test fields must already be set by the caller. +// When Policy.Bypassed is true the result is always OK, but the counts are still +// reported. +// +// The three buckets never overlap: a generated file that is ALSO a test file +// counts once, as generated, so Counted + Generated + Test is always the diff's +// total non-binary changed lines regardless of policy. +func Evaluate(files []FileChange, p Policy) 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} + res := Result{Max: p.Max, Bypassed: p.Bypassed, TestsExcluded: p.ExcludeTests, Files: sorted} for _, f := range sorted { - if f.Generated { + switch { + case f.Generated: res.Generated += f.Changed() - continue + case f.Test: + // Always tallied so the report can show the number either way; only + // kept OUT of Counted when the caller opted in. + res.Test += f.Changed() + if !p.ExcludeTests { + res.Counted += f.Changed() + } + default: + res.Counted += f.Changed() } - res.Counted += f.Changed() } - res.OK = bypassed || res.Counted <= max + res.OK = p.Bypassed || res.Counted <= p.Max sort.SliceStable(res.Files, func(i, j int) bool { return res.Files[i].Changed() > res.Files[j].Changed() }) diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 7c25eb9..559f5eb 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -185,6 +185,73 @@ func TestIsLockfile(t *testing.T) { } } +// TestIsTestPath pins the test-file classification. The false-positive half of +// this table matters more than the true-positive half: a production file +// misclassified as a test silently shrinks the very number the cap protects, +// which is strictly worse than a test file being counted. +func TestIsTestPath(t *testing.T) { + t.Parallel() + tests := []struct { + path string + want bool + }{ + // Go + {"internal/config/feature_rollout_test.go", true}, + {"main_test.go", true}, + {"pkg/testdata/golden.json", true}, + // Python + {"api/tests/test_models.py", true}, + {"api/test_models.py", true}, + {"api/models_test.py", true}, + {"api/conftest.py", true}, + {"conftest.py", true}, + // JS/TS + {"web/src/Button.test.tsx", true}, + {"web/src/api.spec.ts", true}, + {"web/src/util.test.js", true}, + {"web/src/util.spec.mjs", true}, + {"web/src/__tests__/render.tsx", true}, + {"web/src/__snapshots__/Button.test.tsx.snap", true}, + {"web/src/__mocks__/fs.ts", true}, + // Directory conventions, at any depth + {"e2e/checkout.ts", true}, + {"services/checkout/e2e/flow.go", true}, + {"test/helpers.rb", true}, + {"src/test/java/com/x/FooTest.java", true}, + {"internal/testing/harness.go", true}, + + // --- False positives the substring-matching naive version would hit --- + {"contest/leaderboard.go", false}, + {"pkg/contest/entry.py", false}, + {"internal/version/latest.go", false}, + {"protest.go", false}, + {"attestation/verify.go", false}, + {"pkg/attestation/sigstore_test_helpers.go", false}, + {"web/src/manifest.ts", false}, + {"web/src/spec.ts", false}, + {"testify.go", false}, + {"latest_test_results.md", false}, + // `spec/` holds OpenAPI schemas in this org, not RSpec suites. + {"services/checkout/openapi.yaml", false}, + {"spec/openapi.yaml", false}, + {"api/specs/v1.json", false}, + // A file whose own name matches a test DIRECTORY is not a test file. + {"cmd/test", false}, + {"docs/testing", false}, + // Production code that merely lives next to tests. + {"internal/config/feature_rollout.go", false}, + {"services/checkout/server/handlers/assets.go", false}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + t.Parallel() + if got := IsTestPath(tt.path); got != tt.want { + t.Errorf("IsTestPath(%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) { @@ -261,8 +328,10 @@ func TestEvaluate(t *testing.T) { files []FileChange max int bypassed bool + excludeTests bool wantCounted int wantGenerated int + wantTest int wantOK bool }{ { @@ -324,17 +393,79 @@ func TestEvaluate(t *testing.T) { wantCounted: 1000, wantOK: true, }, + { + // Default policy: test lines are reported but still counted, so a + // repo that has not opted in sees exactly today's verdict. + name: "tests counted by default and reported separately", + files: []FileChange{ + {Path: "hand.go", Added: 300, Deleted: 36}, + {Path: "hand_test.go", Added: 1200, Deleted: 33, Test: true}, + }, + max: 1000, + wantCounted: 1569, + wantTest: 1233, + wantOK: false, + }, + { + // The BE-6791 case: the same diff passes once tests are excluded, + // and the excluded total is still reported. + name: "tests excluded when opted in", + files: []FileChange{ + {Path: "hand.go", Added: 300, Deleted: 36}, + {Path: "hand_test.go", Added: 1200, Deleted: 33, Test: true}, + }, + max: 1000, + excludeTests: true, + wantCounted: 336, + wantTest: 1233, + wantOK: true, + }, + { + // Buckets must not overlap: a generated file that also matches a test + // path counts once, as generated, so the three totals still sum to + // the diff's non-binary changed lines. + name: "generated wins over test so buckets never double count", + files: []FileChange{ + {Path: "tests/mock.gen.go", Added: 900, Deleted: 100, Generated: true, Test: true}, + {Path: "hand_test.go", Added: 40, Deleted: 0, Test: true}, + {Path: "hand.go", Added: 10, Deleted: 0}, + }, + max: 1000, + excludeTests: true, + wantCounted: 10, + wantGenerated: 1000, + wantTest: 40, + wantOK: true, + }, + { + // A test-only PR over the cap must still FAIL without the opt-in — + // the exclusion is a per-repo decision, never an implicit one. + name: "test-only PR still fails without the opt-in", + files: []FileChange{ + {Path: "big_test.go", Added: 5000, Deleted: 0, Test: true}, + }, + max: 1000, + wantCounted: 5000, + wantTest: 5000, + wantOK: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := Evaluate(tt.files, tt.max, tt.bypassed) + got := Evaluate(tt.files, Policy{Max: tt.max, Bypassed: tt.bypassed, ExcludeTests: tt.excludeTests}) 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.Test != tt.wantTest { + t.Errorf("Test = %d, want %d", got.Test, tt.wantTest) + } + if got.TestsExcluded != tt.excludeTests { + t.Errorf("TestsExcluded = %v, want %v", got.TestsExcluded, tt.excludeTests) + } if got.OK != tt.wantOK { t.Errorf("OK = %v, want %v", got.OK, tt.wantOK) } @@ -348,7 +479,7 @@ func TestEvaluateSortsFilesByChangedDescending(t *testing.T) { {Path: "small.go", Added: 1, Deleted: 0}, {Path: "big.go", Added: 500, Deleted: 100}, {Path: "mid.go", Added: 50, Deleted: 0}, - }, 1000, false) + }, Policy{Max: 1000}) wantOrder := []string{"big.go", "mid.go", "small.go"} for i, w := range wantOrder { if res.Files[i].Path != w { @@ -363,7 +494,7 @@ func TestEvaluateDoesNotMutateCallerSlice(t *testing.T) { {Path: "small.go", Added: 1}, {Path: "big.go", Added: 500}, } - _ = Evaluate(files, 1000, false) + _ = Evaluate(files, Policy{Max: 1000}) if files[0].Path != "small.go" || files[1].Path != "big.go" { t.Errorf("Evaluate reordered the caller's slice: %+v", files) } From 0d43f0fb77d4fff51e000887655cdff03ac067d7 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Thu, 6 Aug 2026 20:08:00 -0700 Subject: [PATCH 2/9] fix(pr-size): comment when the test exclusion is what keeps a PR under the cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a hole in the central safety claim. The sticky comment posts only when `over_cap` is true — so with `exclude_tests` on, a PR whose 1,200 test lines bring it under the cap goes GREEN and posts NOTHING. The `Excluded (tests): N` line survived only in the Actions step summary, which a reviewer must click into. The number was hidden in exactly the case it exists for, and "a large test-only PR cannot pass unremarked" was not true. Add `Result.ExclusionDecisive()` — tests were excluded, the result is not bypassed, and Counted+Test would exceed Max. It surfaces as a `tests_decisive` GitHub output, rides the artifact, and makes the comment job post on a green check. The report gains a matching paragraph naming the real total. Verified on the diff that motivated this feature: tests_decisive=true, where before the fix that PR would have posted nothing at all. Also from the same review: - Fix a false invariant in Evaluate's doc comment. Counted+Generated+Test is the diff total only when ExcludeTests is set; with it off, Test is a SUBSET of Counted, so the sum double-counts. The existing tests always asserted the correct numbers — only the comment was wrong. - List the largest EXCLUDED test files too. `Excluded (tests): 1282` was unauditable: a reviewer saw a big number with no way to check those files are really tests. Extracted the rendering into `topFiles`. - Set `f.Test` before the binary guard, so a binary fixture under `testdata/` or `__snapshots__/` is reported as the test file it is. No numeric effect — Changed() is 0 for binaries — but classify's doc claimed "always" and it was not. - Match directory segments case-insensitively, so .NET/C#/Unity trees (`Tests/`, `TestData/`, `E2E/`) are recognized. Previously they silently got no exclusion at all. File-name rules stay case-sensitive: `_test.go` and `conftest.py` are lowercase by toolchain definition, so matching other casings would only add false positives. - Stop claiming the largest-counted list "adds up" to Counted — it stops at 10. Not taken: anchoring test-directory matching to the repo root. A production package named `testing/` does get excluded, but that segment is load-bearing for real consumers (one has ~147 non-`*_test.go` helpers under `testing/`), and the opt-in plus the now-mandatory reporting is the intended mitigation. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 36 +++++++++++++---- docs/callers/pr-size.md | 12 ++++-- scripts/check-pr-size/main.go | 60 +++++++++++++++++++++------- scripts/check-pr-size/main_test.go | 50 ++++++++++++++++++++++-- scripts/check-pr-size/size.go | 32 +++++++++++++-- scripts/check-pr-size/size_test.go | 63 ++++++++++++++++++++++++++++++ 6 files changed, 221 insertions(+), 32 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index c1eb2cf..5f61277 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -24,7 +24,10 @@ name: PR Size Cap (reusable) # `__snapshots__/`). Unlike the generated-file rules this is a convention, not a # proof — nothing stops production code being parked in `tests/` — which is why # it is opt-in rather than the default. The excluded total is ALWAYS reported on -# its own line, so a large test-only PR cannot pass unremarked. +# its own line, and when the exclusion is the ONLY reason a PR is under the cap +# the bot comment is posted even though the check is green — otherwise the +# number would be confined to the step summary in exactly the case it exists +# for, and a large test-only PR would pass unremarked. # # 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 @@ -36,7 +39,8 @@ name: PR Size Cap (reusable) # 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 +# surface the bypass label. It also posts on a GREEN check when `exclude_tests` +# is the only thing holding the PR under the cap (see `tests_decisive`). 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. # @@ -239,14 +243,22 @@ jobs: --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 + # The comment job keys off these flags rather than the pr-size job's + # result, so `warn` mode (job green) still comments on overage. They + # ride the artifact, which — unlike job outputs — carries identically # whether this job passed or failed. + # + # tests_decisive covers the mirror-image case: the check is GREEN, but + # only because test lines were excluded. Without it no comment posts + # there, and the excluded total would be visible only in this job's step + # summary — hiding the number in exactly the case it exists for. if: always() env: OVER_CAP: ${{ steps.check.outputs.over_cap }} - run: printf '%s' "${OVER_CAP:-}" > "${RUNNER_TEMP}/pr-size-over-cap" + TESTS_DECISIVE: ${{ steps.check.outputs.tests_decisive }} + run: | + printf '%s' "${OVER_CAP:-}" > "${RUNNER_TEMP}/pr-size-over-cap" + printf '%s' "${TESTS_DECISIVE:-}" > "${RUNNER_TEMP}/pr-size-tests-decisive" # 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 @@ -259,6 +271,7 @@ jobs: path: | ${{ runner.temp }}/pr-size-report.md ${{ runner.temp }}/pr-size-over-cap + ${{ runner.temp }}/pr-size-tests-decisive if-no-files-found: warn retention-days: 7 @@ -338,6 +351,9 @@ jobs: exit 0 fi OVER="$(cat report/pr-size-over-cap 2>/dev/null || true)" + # Green, but only because test lines were excluded — comment anyway, + # so the excluded total is not confined to the step summary. + DECISIVE="$(cat report/pr-size-tests-decisive 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 @@ -349,9 +365,13 @@ jobs: # 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 + elif [ "$OVER" = "true" ] || [ "$DECISIVE" = "true" ]; then gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null - echo "Posted oversize comment." + if [ "$OVER" = "true" ]; then + echo "Posted oversize comment." + else + echo "Under cap only via the test exclusion — posted the size breakdown." + fi else echo "Under cap and no existing comment — nothing to post." fi diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index 30fb2e2..d6fda09 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -93,12 +93,18 @@ and read `.gitattributes` from the base ref precisely so a PR cannot exempt itself — test detection only looks at the path. Nothing stops production code being parked in `tests/` to duck the cap. That is why it is off by default, and why the excluded total is always printed on its own line: the report shows -`Excluded (tests): N` next to the counted number, so a 5,000-line "test-only" PR -is visible rather than silently small. Recognized: `*_test.go`; `test_*.py`, +`Excluded (tests): N` next to the counted number, and names the largest excluded +files so the number can be audited rather than taken on trust. When the +exclusion is the *only* reason a PR is under the cap, the sticky comment posts +even though the check is green — otherwise the number would live solely in the +Actions step summary in precisely the case that matters, and a 5,000-line +"test-only" PR really would pass unremarked. Recognized: `*_test.go`; `test_*.py`, `*_test.py`, `conftest.py`; `*.test.*` / `*.spec.*` for `.js .jsx .mjs .cjs .ts .tsx .mts .cts`; and any file under a `test/`, `tests/`, `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` **directory** -segment. `spec/` is deliberately *not* a test directory — in this org it holds +segment (segment matching is case-insensitive, so `Tests/` and `TestData/` work +too; the file-name rules stay case-sensitive because their toolchains define +them in lowercase). `spec/` is deliberately *not* a test directory — in this org it holds OpenAPI schemas, which are production artifacts. For a layout these miss, add `extra_generated_globs` (they land in the generated bucket instead). diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 1ed3a6c..4eea231 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -161,10 +161,13 @@ func classify(files []FileChange, base, head string, attr attrPolicy, extras Ext } for i := range files { f := &files[i] + // Set before the binary guard so Result.Files reports a binary fixture + // under testdata/ or __snapshots__/ as the test file it is. No numeric + // effect — Changed() is 0 for binaries either way. + f.Test = IsTestPath(f.Path) if f.Binary { continue } - f.Test = IsTestPath(f.Path) if IsLockfile(f.Path) || extras.Generated(f.Path) || attrGen[f.Path] || contentGenerated(f.Path, base, head) { @@ -337,31 +340,55 @@ func renderReport(res Result, mode, bypassLabel string) string { 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) } + // The PR passes ONLY because tests were subtracted — say so unprompted. This + // is the case the whole exclusion-reporting exists for, and it is also the + // one where the check is green and nobody has a reason to look. + if res.OK && res.ExclusionDecisive() { + fmt.Fprintf(&b, "\n**Under the cap only because test lines are excluded.** This PR changes %d lines in total (%d counted + %d test), over the %d-line cap; `exclude_tests` is what brings it under. Expected for a test-heavy change — surfaced so the real size is visible rather than implicit.\n", + res.Counted+res.Test, res.Counted, res.Test, res.Max) + } if res.Note != "" { fmt.Fprintf(&b, "\n> %s\n", res.Note) } - // Largest contributing files, for quick triage. Only files that actually - // contribute to Counted are listed, so the list always adds up to the number - // above it. + // Largest contributors to Counted, for quick triage. Capped at 10, so this + // shows the biggest files rather than accounting for every counted line. + b.WriteString(topFiles(res.Files, "Largest counted files", func(f FileChange) bool { + return !f.Generated && f.Changed() > 0 && !(res.TestsExcluded && f.Test) + })) + // When tests are excluded, name the biggest of them too — otherwise + // `Excluded (tests): N` is an unauditable number: a reviewer sees a large + // exclusion with no way to check the files really are tests. + if res.TestsExcluded { + b.WriteString(topFiles(res.Files, "Largest excluded test files", func(f FileChange) bool { + return !f.Generated && f.Changed() > 0 && f.Test + })) + } + return b.String() +} + +// topFiles renders a collapsed list of up to 10 matching files, largest first +// (res.Files is already sorted by Changed descending). Returns "" when nothing +// matches, so the caller can append unconditionally. +func topFiles(files []FileChange, summary string, keep func(FileChange) bool) string { + var b strings.Builder shown := 0 - var top strings.Builder - for _, f := range res.Files { - if f.Generated || f.Changed() == 0 || (res.TestsExcluded && f.Test) { + for _, f := range files { + if !keep(f) { continue } if shown == 0 { - top.WriteString("\n
Largest counted files\n\n") + fmt.Fprintf(&b, "\n
%s\n\n", summary) } - fmt.Fprintf(&top, "- `%s` (+%d/-%d)\n", f.Path, f.Added, f.Deleted) + fmt.Fprintf(&b, "- `%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()) + if shown == 0 { + return "" } + b.WriteString("\n
\n") return b.String() } @@ -379,7 +406,11 @@ func report(res Result, mode, bypassLabel string) { // 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. +// alone cannot distinguish over from under. tests_decisive drives the same job +// in the opposite case — green, but green only because test lines were +// subtracted — which would otherwise post nothing and leave the excluded total +// visible only to someone who opens the Actions step summary. No-op outside +// GitHub Actions. func writeGitHubOutputs(res Result) { path := os.Getenv("GITHUB_OUTPUT") if path == "" { @@ -390,7 +421,8 @@ func writeGitHubOutputs(res Result) { return } defer f.Close() - fmt.Fprintf(f, "over_cap=%t\ncounted=%d\n", !res.OK, res.Counted) + fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\n", + !res.OK, res.Counted, res.Test, res.ExclusionDecisive()) } // gitTimeout bounds every git invocation so a hung git (a wedged credential diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index e9c6620..7203402 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -453,6 +453,21 @@ func TestRenderReport(t *testing.T) { }) } +// section returns the body of the named
block in a report, or "" if +// absent — so a test can assert which LIST a file appears in, not merely that +// its name occurs somewhere in the markdown. +func section(report, summary string) string { + start := strings.Index(report, ""+summary+"") + if start < 0 { + return "" + } + rest := report[start:] + if end := strings.Index(rest, "
"); end >= 0 { + return rest[:end] + } + return rest +} + // TestRenderReportTestLines is the visibility guarantee for the exclusion: the // test total must appear in the report BOTH ways round — as an explicit // exclusion when opted in (so a large test-only PR cannot pass unremarked), and @@ -476,9 +491,9 @@ func TestRenderReportTestLines(t *testing.T) { t.Errorf("report missing %q:\n%s", want, got) } } - // The "largest counted files" list must add up to the counted number, so - // an excluded test file has no business in it. - if strings.Contains(got, "hand_test.go") { + // An excluded test file has no business in the COUNTED list (it does + // belong in the excluded one, checked separately below). + if strings.Contains(section(got, "Largest counted files"), "hand_test.go") { t.Errorf("excluded test file must not appear in the largest-counted list:\n%s", got) } }) @@ -501,6 +516,35 @@ func TestRenderReportTestLines(t *testing.T) { } }) + t.Run("a green PR saved by the exclusion says so and lists the excluded files", func(t *testing.T) { + t.Parallel() + got := renderReport(Evaluate(files, Policy{Max: 1000, ExcludeTests: true}), modeEnforce, "oversized-ok") + for _, want := range []string{ + "✅ Passed", + "Under the cap only because test lines are excluded", + "changes 1569 lines in total (336 counted + 1233 test)", + // The excluded number must be auditable, not just asserted. + "Largest excluded test files", + "hand_test.go", + } { + if !strings.Contains(got, want) { + t.Errorf("report missing %q:\n%s", want, got) + } + } + }) + + t.Run("a PR that would pass anyway gets no such explanation", func(t *testing.T) { + t.Parallel() + small := []FileChange{ + {Path: "hand.go", Added: 10}, + {Path: "hand_test.go", Added: 20, Test: true}, + } + got := renderReport(Evaluate(small, Policy{Max: 1000, ExcludeTests: true}), modeEnforce, "oversized-ok") + if strings.Contains(got, "only because test lines are excluded") { + t.Errorf("a comfortably-under PR must not claim the exclusion saved it:\n%s", got) + } + }) + t.Run("no test bullet when a PR has no test changes", func(t *testing.T) { t.Parallel() got := renderReport(Evaluate(files[:1], Policy{Max: 1000}), modeEnforce, "oversized-ok") diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 6806014..2dbb5fa 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -51,6 +51,12 @@ var lockfileNames = map[string]bool{ // directory segments are considered (never the file name itself), so a // hand-written file literally named `test` is still counted. // +// Segment matching is case-INSENSITIVE, so .NET/C#/Unity trees (`Tests/`, +// `TestData/`, `E2E/`) are recognized. The file-name rules below stay +// case-sensitive on purpose: `_test.go` and `conftest.py` are spelled in +// lowercase by their toolchains, so a capitalized variant is not the convention +// and matching it would only add false positives. +// // `spec`/`specs` are deliberately absent: in this org those hold API schemas // (OpenAPI), which are production artifacts. The unambiguous `*.spec.ts` // file-name convention is handled below instead. @@ -133,6 +139,22 @@ type Result struct { Note string } +// ExclusionDecisive reports whether the test exclusion is the ONLY reason this +// PR is under the cap — it would be over if test lines counted. +// +// This is the case the reporting exists for. A PR that passes on its own merits +// needs no explanation; one that passes only because 1,200 test lines were +// subtracted is exactly the "large test-only PR sails through unremarked" risk, +// so the workflow uses this to force the sticky comment even when the check is +// green (the comment otherwise posts only on overage, which would hide the +// number in precisely the case it matters most). +// +// Bypassed results are excluded: the label already explains why the PR passed, +// so attributing it to the exclusion would be misleading. +func (r Result) ExclusionDecisive() bool { + return r.TestsExcluded && !r.Bypassed && r.Counted+r.Test > r.Max +} + // 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 @@ -251,7 +273,7 @@ func hasTestSegment(path string) bool { return false // no directory part at all } for _, seg := range strings.Split(dir[:slash], "/") { - if testPathSegments[seg] { + if testPathSegments[strings.ToLower(seg)] { return true } } @@ -392,9 +414,11 @@ func TouchesGitattributes(files []FileChange) bool { // When Policy.Bypassed is true the result is always OK, but the counts are still // reported. // -// The three buckets never overlap: a generated file that is ALSO a test file -// counts once, as generated, so Counted + Generated + Test is always the diff's -// total non-binary changed lines regardless of policy. +// Generated and Test never overlap: a file that is both counts once, as +// generated. Whether Test overlaps Counted depends on policy. With ExcludeTests +// set the three are a partition — Counted + Generated + Test is the diff's total +// non-binary changed lines. WITHOUT it, Test is a SUBSET of Counted, reported +// for information only, so that same sum double-counts the test lines. func Evaluate(files []FileChange, p Policy) Result { // Copy before sorting so we honor the file header's "side-effect-free" // contract and never reorder the caller's slice in place. diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 559f5eb..a00d5ce 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -215,6 +215,10 @@ func TestIsTestPath(t *testing.T) { {"web/src/__mocks__/fs.ts", true}, // Directory conventions, at any depth {"e2e/checkout.ts", true}, + // Segment matching is case-insensitive (.NET/C#/Unity casing). + {"src/Tests/FooTests.cs", true}, + {"src/TestData/golden.json", true}, + {"E2E/Checkout.cs", true}, {"services/checkout/e2e/flow.go", true}, {"test/helpers.rb", true}, {"src/test/java/com/x/FooTest.java", true}, @@ -473,6 +477,65 @@ func TestEvaluate(t *testing.T) { } } +// TestExclusionDecisive pins when the report is forced to explain itself: only +// when the exclusion is the sole reason the PR is under the cap. It drives the +// sticky comment on a GREEN check, so a false positive means a comment on every +// PR and a false negative means the number stays hidden in the step summary. +func TestExclusionDecisive(t *testing.T) { + t.Parallel() + tests := []struct { + name string + res Result + want bool + }{ + { + name: "excluded tests are what keep it under", + res: Result{Counted: 336, Test: 1233, Max: 1000, TestsExcluded: true}, + want: true, + }, + { + name: "would pass anyway — tests are incidental", + res: Result{Counted: 100, Test: 50, Max: 1000, TestsExcluded: true}, + want: false, + }, + { + name: "exactly at the cap without tests is not over", + res: Result{Counted: 900, Test: 100, Max: 1000, TestsExcluded: true}, + want: false, + }, + { + name: "one line over is decisive", + res: Result{Counted: 900, Test: 101, Max: 1000, TestsExcluded: true}, + want: true, + }, + { + name: "policy off — nothing was excluded, so nothing to explain", + res: Result{Counted: 1569, Test: 1233, Max: 1000}, + want: false, + }, + { + // The label already explains why it passed; crediting the exclusion + // would be misleading. + name: "bypassed results are never attributed to the exclusion", + res: Result{Counted: 336, Test: 1233, Max: 1000, TestsExcluded: true, Bypassed: true}, + want: false, + }, + { + name: "no test lines at all", + res: Result{Counted: 500, Max: 1000, TestsExcluded: true}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := tt.res.ExclusionDecisive(); got != tt.want { + t.Errorf("ExclusionDecisive() = %v, want %v", got, tt.want) + } + }) + } +} + func TestEvaluateSortsFilesByChangedDescending(t *testing.T) { t.Parallel() res := Evaluate([]FileChange{ From a40fca154fb9e210e594808195e463c2433fbc6d Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Thu, 6 Aug 2026 20:28:16 -0700 Subject: [PATCH 3/9] fix(pr-size): harden the size comment path (review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 landed three findings on the comment path — the path this feature just made load-bearing, since a green test-heavy PR now has the sticky comment as its only signal. Comment hijack. The sticky-comment lookup matched ANY comment containing ``, with no author filter, and that marker is published in this public workflow file. A PR author could pre-seed a comment carrying the marker; the bot would then PATCH the author's comment instead of posting its own, after which the author can rewrite it freely — suppressing or falsifying the verdict. The lookup now also matches the bot app's own id (compared as a string, so a non-numeric input cannot make jq abort). Report injection. `f.Path` was interpolated into markdown unescaped, and `git diff --numstat -z` emits paths VERBATIM — ParseNumstat's own comment notes paths may contain spaces and newlines. A crafted filename could close the code span and forge lines inside a bot-authored comment (a second "✅ Passed", a fake "Excluded (tests): 0", another marker), and a newline reaching stdout can emit a `::` workflow command. Added sanitizePath: backticks neutralized, control bytes replaced, length bounded. Comment-size DoS. Two 10-entry lists with unbounded paths could push the body past GitHub's 65,536-character limit; the upsert runs under continue-on-error, so it would 422 and degrade SILENTLY to no comment — the same unremarked pass the green-check comment exists to prevent. maxPathDisplay bounds each path. Also: - ExclusionDecisive omitted the "under the cap" half of its own contract. With Counted already over Max, `Counted+Test > Max` is trivially true, so it emitted tests_decisive=true on a RED run. Harmless in today's callers, wrong in a machine-readable output that must stand alone. Added `Counted <= Max`. - The decisive paragraph called `Counted+Test` the PR's "total", omitting Generated — understating a diff that also regenerated a lockfile, in the one sentence whose purpose is showing the real size. Now "non-generated lines". - tests_excluded was written from res.Test unconditionally, asserting an exclusion that never happened under the default policy. Emits 0 there now. - Refreshed Result.Counted/Test field comments (round 1 fixed Evaluate's doc but left these, which the panel re-raised). - Documented that the green-check comment REQUIRES the bot App: opt into exclude_tests without it and you keep the loosening and lose the visibility. Also that extra_generated_globs classifies as generated, not test, so it opts out of this guarantee. Not taken: a concurrency group on the reusable to close the check-then-act race in the upsert. The race is real and pre-existing, but cancel-in-progress on a required check is how a PR ends up BLOCKED behind a cancelled run that looks like a defect — a worse failure than a rare duplicate comment. Belongs upstream of this change, with its own thought. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 18 ++++++- docs/callers/pr-size.md | 17 +++++- scripts/check-pr-size/main.go | 53 ++++++++++++++++-- scripts/check-pr-size/main_test.go | 87 +++++++++++++++++++++++++++++- scripts/check-pr-size/size.go | 29 +++++++--- scripts/check-pr-size/size_test.go | 8 +++ 6 files changed, 198 insertions(+), 14 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 5f61277..448a5eb 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -27,7 +27,10 @@ name: PR Size Cap (reusable) # its own line, and when the exclusion is the ONLY reason a PR is under the cap # the bot comment is posted even though the check is green — otherwise the # number would be confined to the step summary in exactly the case it exists -# for, and a large test-only PR would pass unremarked. +# for, and a large test-only PR would pass unremarked. That green-check comment +# needs `comment: true` plus bot credentials; a repo that sets `exclude_tests` +# without the App keeps the loosening and loses the visibility, so configure +# both together. # # 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 @@ -344,6 +347,7 @@ jobs: GH_TOKEN: ${{ steps.bot.outputs.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + BOT_APP_ID: ${{ inputs.bot_app_id }} run: | set -euo pipefail if [ ! -s report/pr-size-report.md ]; then @@ -358,8 +362,18 @@ jobs: { 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. + # + # The author filter is load-bearing, not defensive dressing: the marker + # is published in this PUBLIC workflow file, so without it a PR author + # can pre-seed a comment containing the marker and we would PATCH + # THEIR comment instead of posting ours — after which they can rewrite + # it freely, suppressing or falsifying the size verdict. Since + # `exclude_tests` makes this comment the ONLY signal for a green + # test-heavy PR, that is a silent hole straight through the guarantee. + # Matching on the app id (compared as a string, so a non-numeric input + # cannot make jq abort) keeps us to comments this app actually wrote. existing="$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ - --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id")" + --jq ".[] | select(((.performed_via_github_app.id // \"\") | tostring) == \"${BOT_APP_ID}\") | 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. diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index d6fda09..7f590f5 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -98,7 +98,22 @@ files so the number can be audited rather than taken on trust. When the exclusion is the *only* reason a PR is under the cap, the sticky comment posts even though the check is green — otherwise the number would live solely in the Actions step summary in precisely the case that matters, and a 5,000-line -"test-only" PR really would pass unremarked. Recognized: `*_test.go`; `test_*.py`, +"test-only" PR really would pass unremarked. + +**That green-check comment needs the bot App.** It requires `comment: true` +(the default) *and* `bot_app_id` + `BOT_APP_PRIVATE_KEY` — all optional. Opt into +`exclude_tests` without the App and you get precisely the outcome the paragraph +above says is prevented: a green check whose excluded total is visible only to +someone who opens the Actions step summary. **If you set `exclude_tests`, +configure the App too** — otherwise you keep the loosening and lose the +visibility that justifies it. + +Note also that `extra_generated_globs` (below) classifies matches as +*generated*, not *test*: they never reach the excluded-test total and never +trigger the green-check comment. A repo leaning on it for an unusual test layout +opts out of this visibility guarantee. + +Recognized: `*_test.go`; `test_*.py`, `*_test.py`, `conftest.py`; `*.test.*` / `*.spec.*` for `.js .jsx .mjs .cjs .ts .tsx .mts .cts`; and any file under a `test/`, `tests/`, `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` **directory** diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 4eea231..a4b209e 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -344,7 +344,11 @@ func renderReport(res Result, mode, bypassLabel string) string { // is the case the whole exclusion-reporting exists for, and it is also the // one where the check is green and nobody has a reason to look. if res.OK && res.ExclusionDecisive() { - fmt.Fprintf(&b, "\n**Under the cap only because test lines are excluded.** This PR changes %d lines in total (%d counted + %d test), over the %d-line cap; `exclude_tests` is what brings it under. Expected for a test-heavy change — surfaced so the real size is visible rather than implicit.\n", + // "non-generated" is load-bearing: res.Generated is deliberately outside + // this sum, so calling it the PR's total would understate a diff that + // also regenerated a lockfile — in the one sentence whose whole job is + // making the real size visible. + fmt.Fprintf(&b, "\n**Under the cap only because test lines are excluded.** This PR changes %d non-generated lines (%d counted + %d test), over the %d-line cap; `exclude_tests` is what brings it under. Expected for a test-heavy change — surfaced so the real size is visible rather than implicit.\n", res.Counted+res.Test, res.Counted, res.Test, res.Max) } if res.Note != "" { @@ -366,6 +370,42 @@ func renderReport(res Result, mode, bypassLabel string) string { return b.String() } +// maxPathDisplay bounds one rendered path. Two 10-entry lists with unbounded +// paths could push the comment body past GitHub's 65,536-character limit, and +// the upsert step runs under continue-on-error — so an over-long body 422s and +// degrades SILENTLY to "no comment posted", which is precisely the unremarked +// pass the decisive-green comment exists to prevent. +const maxPathDisplay = 160 + +// sanitizePath makes a repo-relative path safe to interpolate into the markdown +// report. `git diff --numstat -z` emits paths VERBATIM (see ParseNumstat), so a +// path may legitimately contain backticks, newlines and other control bytes. +// Untreated, a PR author can name a file so the code span closes and forged +// lines land inside a BOT-authored comment — a second "✅ Passed" heading, a +// fake "Excluded (tests): 0", or another sticky-comment marker — and a newline +// reaching stdout can emit a `::` workflow command. Control characters are +// replaced, backticks neutralized, and the result bounded by maxPathDisplay. +func sanitizePath(path string) string { + var b strings.Builder + for _, r := range path { + switch { + case r == '`': + // Would close the code span the caller wraps this in. + b.WriteRune('\'') + case r < 0x20 || r == 0x7f: + // Newlines, CR, and other control bytes: one visible placeholder. + b.WriteRune('?') + default: + b.WriteRune(r) + } + } + s := b.String() + if len(s) > maxPathDisplay { + s = s[:maxPathDisplay] + "…" + } + return s +} + // topFiles renders a collapsed list of up to 10 matching files, largest first // (res.Files is already sorted by Changed descending). Returns "" when nothing // matches, so the caller can append unconditionally. @@ -379,7 +419,7 @@ func topFiles(files []FileChange, summary string, keep func(FileChange) bool) st if shown == 0 { fmt.Fprintf(&b, "\n
%s\n\n", summary) } - fmt.Fprintf(&b, "- `%s` (+%d/-%d)\n", f.Path, f.Added, f.Deleted) + fmt.Fprintf(&b, "- `%s` (+%d/-%d)\n", sanitizePath(f.Path), f.Added, f.Deleted) shown++ if shown >= 10 { break @@ -421,8 +461,15 @@ func writeGitHubOutputs(res Result) { return } defer f.Close() + // tests_excluded reports what was actually EXCLUDED, so it is 0 under the + // default policy — there, res.Test is a subset of counted and naming it an + // exclusion would assert something that did not happen. + testsExcluded := 0 + if res.TestsExcluded { + testsExcluded = res.Test + } fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\n", - !res.OK, res.Counted, res.Test, res.ExclusionDecisive()) + !res.OK, res.Counted, testsExcluded, res.ExclusionDecisive()) } // gitTimeout bounds every git invocation so a hung git (a wedged credential diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index 7203402..f30096f 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -522,7 +522,7 @@ func TestRenderReportTestLines(t *testing.T) { for _, want := range []string{ "✅ Passed", "Under the cap only because test lines are excluded", - "changes 1569 lines in total (336 counted + 1233 test)", + "changes 1569 non-generated lines (336 counted + 1233 test)", // The excluded number must be auditable, not just asserted. "Largest excluded test files", "hand_test.go", @@ -554,6 +554,91 @@ func TestRenderReportTestLines(t *testing.T) { }) } +// TestSanitizePath is the report-injection guard. `git diff --numstat -z` emits +// paths verbatim, so a PR author controls these bytes; untreated they let forged +// lines land inside a BOT-authored comment, which is the one place a reader +// trusts the numbers. +func TestSanitizePath(t *testing.T) { + t.Parallel() + tests := []struct { + name string + path string + want string + }{ + {"ordinary path untouched", "pkg/foo/bar.go", "pkg/foo/bar.go"}, + {"unicode preserved", "café/go.sum", "café/go.sum"}, + { + // Closing the code span and forging a passing verdict. + name: "backtick cannot close the code span", + path: "a`.go\n\n## ✅ Passed — PR size check\n`x", + want: "a'.go??## ✅ Passed — PR size check?'x", + }, + { + // A forged marker would hijack the sticky-comment lookup. + name: "marker injection is defanged", + path: "x`\n\n`y.go", + want: "x'??'y.go", + }, + { + // Newlines reaching stdout can emit workflow commands. + name: "workflow command injection is defanged", + path: "a\n::error::spoofed\nb.go", + want: "a?::error::spoofed?b.go", + }, + {"carriage return replaced", "a\rb.go", "a?b.go"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := sanitizePath(tt.path); got != tt.want { + t.Errorf("sanitizePath(%q) = %q, want %q", tt.path, got, tt.want) + } + }) + } + + t.Run("over-long paths are bounded", func(t *testing.T) { + t.Parallel() + got := sanitizePath(strings.Repeat("a", 5000) + ".go") + if len(got) > maxPathDisplay+len("…") { + t.Errorf("len = %d, want <= %d", len(got), maxPathDisplay+len("…")) + } + if !strings.HasSuffix(got, "…") { + t.Errorf("truncated path should be marked with an ellipsis, got %q", got[len(got)-10:]) + } + }) +} + +// TestRenderReportSanitizesPaths proves the guard is wired into the report, not +// merely available. +// +// The invariant is structural, not textual: markdown only sees a heading at the +// START of a line, so what must be impossible is a hostile path creating a NEW +// line. The forged text surviving inside a code span on the path's own line is +// harmless and expected — asserting its mere absence would test the wrong thing. +func TestRenderReportSanitizesPaths(t *testing.T) { + t.Parallel() + hostile := "evil`.go\n\n## ✅ Passed — PR size check\n\n`x.go" + got := renderReport(Evaluate([]FileChange{ + {Path: hostile, Added: 2000}, + }, Policy{Max: 1000}), modeEnforce, "oversized-ok") + + var headings []string + for _, line := range strings.Split(got, "\n") { + if strings.HasPrefix(line, "## ") { + headings = append(headings, line) + } + } + if len(headings) != 1 { + t.Fatalf("report must carry exactly one status heading, got %d: %q\n%s", len(headings), headings, got) + } + if !strings.Contains(headings[0], "❌ Failed") { + t.Errorf("the surviving heading should be the real failing verdict, got %q", headings[0]) + } + if strings.Contains(got, hostile) { + t.Errorf("raw hostile path reached the report unsanitized:\n%s", got) + } +} + // TestClassifySetsTestRegardlessOfPolicy proves classification is policy-free: // Test is set from the path alone, so Evaluate can report the total whether or // not the caller opted to exclude it. Non-.go paths keep git out of the picture diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 2dbb5fa..99528ed 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -123,12 +123,20 @@ type Policy struct { // 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 - Test int // changed lines in test files (excluded only if TestsExcluded) - Max int // the configured ceiling - Bypassed bool // a bypass label was present - OK bool // Bypassed OR Counted <= Max + // Counted is the number the cap is compared against: changed lines from + // non-binary files that are neither generated nor — when TestsExcluded is + // set — test files. + Counted int + // Generated is changed lines excluded because the file is generated. A + // generated file that is ALSO a test file is tallied here and not in Test. + Generated int + // Test is changed lines in non-generated test files. It is subtracted from + // Counted only when TestsExcluded is set; otherwise it is a subset of + // Counted, reported for information. + Test int + Max int // the configured ceiling + Bypassed bool // a bypass label was present + OK bool // Bypassed OR Counted <= Max // TestsExcluded records whether Policy.ExcludeTests was set, so the report // can say whether Test lines were subtracted from Counted or are part of it. TestsExcluded bool @@ -151,8 +159,15 @@ type Result struct { // // Bypassed results are excluded: the label already explains why the PR passed, // so attributing it to the exclusion would be misleading. +// +// Both halves of "under the cap ONLY because" are checked. Without the +// `Counted <= Max` term a run that is over cap even after the exclusion +// satisfies `Counted+Test > Max` trivially and would report itself decisive on a +// RED run — harmless in today's callers (the report gates on OK, and the comment +// job tests over-cap first) but wrong in the machine-readable output, which must +// stand on its own. func (r Result) ExclusionDecisive() bool { - return r.TestsExcluded && !r.Bypassed && r.Counted+r.Test > r.Max + return r.TestsExcluded && !r.Bypassed && r.Counted <= r.Max && r.Counted+r.Test > r.Max } // ParseNumstat parses the output of `git diff --numstat -z`. Records are diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index a00d5ce..efccbb3 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -508,6 +508,14 @@ func TestExclusionDecisive(t *testing.T) { res: Result{Counted: 900, Test: 101, Max: 1000, TestsExcluded: true}, want: true, }, + { + // Over cap even after the exclusion: `Counted+Test > Max` is + // trivially true, so without the `Counted <= Max` term this would + // claim decisiveness on a RED run. + name: "over cap despite the exclusion is not decisive", + res: Result{Counted: 1500, Test: 500, Max: 1000, TestsExcluded: true}, + want: false, + }, { name: "policy off — nothing was excluded, so nothing to explain", res: Result{Counted: 1569, Test: 1233, Max: 1000}, From 0247fbff17181c837bb44a4108c06b0a1f74031b Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Thu, 6 Aug 2026 20:52:31 -0700 Subject: [PATCH 4/9] fix(pr-size): align the sticky-comment lookup with find_sticky (review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 found a real bug in round 2's own hardening, plus the reason my comment-lookup filter was the wrong shape. This repo already solved these once: `find_sticky` in scripts/pr-risk/publish-risk-surfaces.sh. Round 2 invented a second dialect instead of following it, and got the details wrong. Now aligned. sanitizePath emitted invalid UTF-8. It built the string rune-by-rune then truncated with a BYTE slice, so a cut landing inside a multi-byte rune produced a broken string in the comment body — which GitHub can 422 on, and continue-on-error turns that into the silent no-comment degradation the bound exists to prevent. Truncation now accumulates bytes and only ever cuts at a rune boundary. The old subtest was pure ASCII and could not catch it; the new one sweeps multi-byte runes at every length and asserts utf8.ValidString. Comment-lookup filter, three faults in one line: - Keyed on `.performed_via_github_app.id`, but create-github-app-token also accepts a CLIENT id (`Iv1.…`) for `app-id`. A caller wiring that mints tokens and posts fine, yet never matches — silently stacking a new comment on every push and never flipping a flagged PR to ✅. Now filters on `.user.type == "Bot"`, which is what actually defeats the pre-seed attack (a PR author's comment is type User) without depending on the id's form. - Still `contains`, not `startswith`. The app id is the ORG's shared bot, which also posts cursor-review and groom comments — so a bot comment quoting the marker matched, and the size report would PATCH that review away. An author can induce it by quoting the marker in the PR body. The body renders the marker as its first line, so `startswith` is exact. - Spliced BOT_APP_ID into the jq PROGRAM text. A quote or backslash made the filter a syntax error → non-zero gh → `set -euo pipefail` → silent skip. My comment claiming this "cannot make jq abort" was simply wrong: `tostring` guards the JSON side, not the interpolated literal. The marker is now passed as jq data via --arg and nothing user-controlled reaches the program text. Also: - Reverses round 2's rejection of the duplicate-comment race, because round 3 supplied an option I had not considered. I rejected a `concurrency:` group on the grounds that cancel-in-progress on a required check strands PRs behind cancelled runs. PATCHing every marker match instead of `head -n1` closes the same race with no concurrency group at all — a stale duplicate can no longer sit there displaying a wrong number. - sanitizePath now also replaces unicode.Cf. A bidi override or isolate renders a path as a filename other than the one on disk and can visually reorder the adjacent counts — directly undermining the excluded-files list, whose purpose is letting a reviewer verify the exclusions. - GITHUB_OUTPUT write and close errors are surfaced instead of dropped: a partial write can lose tests_decisive while still exiting 0, and the comment job then reads it as false and skips the green-check comment. - JS/TS matching now tests each dot-separated stem component rather than only the one adjoining the extension, so type tests (`foo.test.d.ts`) match the `*.test.*` convention the docs advertise. Requiring a non-first component still keeps a module named `spec.ts` counted. - Documented that fork and Dependabot PRs never receive the secret, so the green-check comment cannot post there even for a correctly configured caller. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 74 ++++++++++++++++++++++-------- scripts/check-pr-size/main.go | 50 ++++++++++++++------ scripts/check-pr-size/main_test.go | 29 ++++++++++++ scripts/check-pr-size/size.go | 14 ++++-- scripts/check-pr-size/size_test.go | 7 +++ 5 files changed, 138 insertions(+), 36 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 448a5eb..0d01319 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -43,9 +43,14 @@ name: PR Size Cap (reusable) # 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. It also posts on a GREEN check when `exclude_tests` -# is the only thing holding the PR under the cap (see `tests_decisive`). 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. +# is the only thing holding the PR under the cap (see `tests_decisive`). +# +# 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. Note this also covers fork +# and Dependabot PRs, which never receive the secret — so on those the green +# check's excluded-test total lives only in the step summary even for a caller +# that configured the App. # # 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 @@ -347,7 +352,6 @@ jobs: GH_TOKEN: ${{ steps.bot.outputs.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} - BOT_APP_ID: ${{ inputs.bot_app_id }} run: | set -euo pipefail if [ ! -s report/pr-size-report.md ]; then @@ -360,25 +364,55 @@ jobs: DECISIVE="$(cat report/pr-size-tests-decisive 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. + # Find our existing sticky comment(s) by the hidden marker, so we + # update across pushes instead of stacking new ones. Both filters are + # load-bearing, and this mirrors `find_sticky` in + # scripts/pr-risk/publish-risk-surfaces.sh deliberately — same hazards, + # so the same shape rather than a second dialect of it. + # + # .user.type == "Bot" — the marker is published in this PUBLIC + # workflow file, so without an author filter a PR author can + # pre-seed a comment carrying it and we would PATCH THEIRS instead + # of posting ours, after which they rewrite it freely. Since + # `exclude_tests` makes this comment the ONLY signal on a green + # test-heavy PR, that is a hole straight through the guarantee. + # Type is used rather than the app id because + # create-github-app-token also accepts a CLIENT id (`Iv1.…`) for + # `app-id`; a caller wiring that would never match the numeric app + # id and would silently stack a new comment on every push. + # + # startswith, not contains — the body renders the marker as its + # FIRST line, while another bot QUOTING this comment carries the + # marker nested in its own prose. `contains` would PATCH that + # bot's comment away, destroying unrelated review output; the org + # bot posts cursor-review and groom comments too, so an author can + # induce this by quoting the marker in the PR body. # - # The author filter is load-bearing, not defensive dressing: the marker - # is published in this PUBLIC workflow file, so without it a PR author - # can pre-seed a comment containing the marker and we would PATCH - # THEIR comment instead of posting ours — after which they can rewrite - # it freely, suppressing or falsifying the size verdict. Since - # `exclude_tests` makes this comment the ONLY signal for a green - # test-heavy PR, that is a silent hole straight through the guarantee. - # Matching on the app id (compared as a string, so a non-numeric input - # cannot make jq abort) keeps us to comments this app actually wrote. + # The marker is passed as jq DATA (--arg), never spliced into the + # program text, so no value can turn the filter into a syntax error — + # which `set -euo pipefail` plus this step's continue-on-error would + # convert into a silent "no comment posted". existing="$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ - --jq ".[] | select(((.performed_via_github_app.id // \"\") | tostring) == \"${BOT_APP_ID}\") | select(.body | contains(\"${MARKER}\")) | .id")" - existing="$(printf '%s' "$existing" | head -n1)" + | jq -r --arg m "$MARKER" ' + .[] | select((.user.type // "") == "Bot") + | select(((.body // "") | startswith($m))) + | .id')" 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}." + # Update EVERY match, not just the first. The find-then-post is + # check-then-act with no lock, and the documented caller subscribes + # to labeled/unlabeled alongside synchronize — so a push racing a + # label toggle can leave two comments. Taking `head -n1` would then + # pin one forever while the other sat displaying a stale verdict, + # which on a decisive-green PR is a wrong number with equal + # standing. Rewriting all of them keeps every copy truthful, and + # costs one extra API call in the rare case there is a second. + # Unquoted on purpose: the ids are numeric and newline-separated, so + # word splitting is exactly the right parse (and a heredoc's + # terminator cannot be indented inside this YAML block scalar). + for id in $existing; do + gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null + echo "Updated sticky size comment ${id}." + done elif [ "$OVER" = "true" ] || [ "$DECISIVE" = "true" ]; then gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null if [ "$OVER" = "true" ]; then diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index a4b209e..705e88a 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -11,6 +11,8 @@ import ( "strconv" "strings" "time" + "unicode" + "unicode/utf8" ) const ( @@ -385,25 +387,36 @@ const maxPathDisplay = 160 // fake "Excluded (tests): 0", or another sticky-comment marker — and a newline // reaching stdout can emit a `::` workflow command. Control characters are // replaced, backticks neutralized, and the result bounded by maxPathDisplay. +// Unicode format characters (unicode.Cf) are replaced alongside the C0 controls: +// a bidi override (U+202A–U+202E) or isolate (U+2066–U+2069) makes a path render +// as a filename other than the one on disk, and can visually reorder the +// adjacent (+N/-M) counts — which would defeat the "Largest excluded test files" +// list, whose whole purpose is letting a reviewer confirm the excluded lines +// really are tests. +// +// Truncation is bounded by ACCUMULATED BYTES and only ever cuts at a rune +// boundary. Slicing the finished string at a byte offset would split a +// multi-byte rune and emit invalid UTF-8 into the comment body — which GitHub +// can 422 on, and continue-on-error would turn that into the silent no-comment +// degradation maxPathDisplay exists to prevent. func sanitizePath(path string) string { var b strings.Builder for _, r := range path { + out := r switch { case r == '`': // Would close the code span the caller wraps this in. - b.WriteRune('\'') - case r < 0x20 || r == 0x7f: - // Newlines, CR, and other control bytes: one visible placeholder. - b.WriteRune('?') - default: - b.WriteRune(r) + out = '\'' + case r < 0x20 || r == 0x7f || unicode.Is(unicode.Cf, r): + // Newlines, CR, other C0 controls, and invisible format characters. + out = '?' } + if b.Len()+utf8.RuneLen(out) > maxPathDisplay { + return b.String() + "…" + } + b.WriteRune(out) } - s := b.String() - if len(s) > maxPathDisplay { - s = s[:maxPathDisplay] + "…" - } - return s + return b.String() } // topFiles renders a collapsed list of up to 10 matching files, largest first @@ -458,9 +471,9 @@ func writeGitHubOutputs(res Result) { } f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644) if err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: cannot open GITHUB_OUTPUT: %v\n", err) return } - defer f.Close() // tests_excluded reports what was actually EXCLUDED, so it is 0 under the // default policy — there, res.Test is a subset of counted and naming it an // exclusion would assert something that did not happen. @@ -468,8 +481,19 @@ func writeGitHubOutputs(res Result) { if res.TestsExcluded { testsExcluded = res.Test } - fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\n", + // Both errors are surfaced rather than dropped: a partial write can lose + // tests_decisive while the process still exits 0, and the comment job then + // reads the absent flag as false and silently skips the green-check comment + // — the very failure this output exists to close. + _, werr := fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\n", !res.OK, res.Counted, testsExcluded, res.ExclusionDecisive()) + cerr := f.Close() + if werr != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: writing GITHUB_OUTPUT failed: %v\n", werr) + } + if cerr != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: closing GITHUB_OUTPUT failed: %v\n", cerr) + } } // gitTimeout bounds every git invocation so a hung git (a wedged credential diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index f30096f..ff64209 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "unicode/utf8" ) // These tests exercise the git-backed generated-file classification against a @@ -586,6 +587,16 @@ func TestSanitizePath(t *testing.T) { want: "a?::error::spoofed?b.go", }, {"carriage return replaced", "a\rb.go", "a?b.go"}, + { + // Trojan-source class: a bidi override renders the path as a + // filename other than the one on disk, in the very list a reviewer + // uses to confirm the excluded files really are tests. + name: "bidi override is replaced", + path: "src/‮og.tset_x/a.go", + want: "src/?og.tset_x/a.go", + }, + {"bidi isolate is replaced", "a⁦b⁩c.go", "a?b?c.go"}, + {"zero-width joiner is replaced", "a‍b.go", "a?b.go"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -606,6 +617,24 @@ func TestSanitizePath(t *testing.T) { t.Errorf("truncated path should be marked with an ellipsis, got %q", got[len(got)-10:]) } }) + + // The ASCII case above cannot catch a cut landing mid-rune. Multi-byte runes + // at every offset make the byte boundary fall inside a rune for some input + // length, so a byte-slice truncation emits invalid UTF-8 here. + t.Run("truncation never splits a rune", func(t *testing.T) { + t.Parallel() + for _, r := range []string{"é", "☃", "🙂"} { + for n := 1; n <= 200; n++ { + got := sanitizePath(strings.Repeat(r, n) + ".go") + if !utf8.ValidString(got) { + t.Fatalf("sanitizePath(%d×%q) produced invalid UTF-8: %q", n, r, got) + } + if len(got) > maxPathDisplay+len("…") { + t.Fatalf("sanitizePath(%d×%q) len = %d, over bound", n, r, len(got)) + } + } + } + }) } // TestRenderReportSanitizesPaths proves the guard is wired into the report, not diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 99528ed..8987772 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -311,9 +311,17 @@ func isTestFileName(base string) bool { return true } if dot := strings.LastIndex(base, "."); dot > 0 && jsTestExts[base[dot:]] { - stem := base[:dot] - if strings.HasSuffix(stem, ".test") || strings.HasSuffix(stem, ".spec") { - return true + // Any dot-separated component of the stem AFTER the first being exactly + // `test` or `spec` marks a test file. Checking components rather than + // only the one adjoining the extension also catches type tests + // (`foo.test.d.ts`), which match the `*.test.*` convention the docs + // advertise. Requiring a NON-first component is what keeps a + // hand-written module literally named `spec.ts` counted. + parts := strings.Split(base[:dot], ".") + for _, p := range parts[1:] { + if p == "test" || p == "spec" { + return true + } } } return false diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index efccbb3..88f943c 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -211,6 +211,9 @@ func TestIsTestPath(t *testing.T) { {"web/src/util.test.js", true}, {"web/src/util.spec.mjs", true}, {"web/src/__tests__/render.tsx", true}, + // Type tests — the `test` component need not adjoin the extension. + {"web/src/api.test.d.ts", true}, + {"web/src/api.spec.d.ts", true}, {"web/src/__snapshots__/Button.test.tsx.snap", true}, {"web/src/__mocks__/fs.ts", true}, // Directory conventions, at any depth @@ -233,6 +236,10 @@ func TestIsTestPath(t *testing.T) { {"pkg/attestation/sigstore_test_helpers.go", false}, {"web/src/manifest.ts", false}, {"web/src/spec.ts", false}, + {"web/src/test.ts", false}, + // A `test`/`spec` component must not be the FIRST one. + {"web/src/spec.helpers.ts", false}, + {"web/src/manifest.d.ts", false}, {"testify.go", false}, {"latest_test_results.md", false}, // `spec/` holds OpenAPI schemas in this org, not RSpec suites. From 165815751c4bdf93fa4074aa08b766bbb82928e9 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Thu, 6 Aug 2026 21:17:50 -0700 Subject: [PATCH 5/9] fix(pr-size): scope the comment filter to our own bot (review round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 caught two defects introduced by round 3, both from over-correcting. Over-correction on identity. Round 3 replaced the app-id filter with `.user.type == "Bot"`, justified as "an author's comment is type User". That defeats the human pre-seed but drops bot IDENTITY entirely, so the filter meant "any bot on this PR whose body starts with the marker" — any other App or github-actions[bot] workflow in a consumer repo that echoes author-controlled text as its first line would be claimed as ours and PATCHed away, and while such a comment exists the POST branch never runs, suppressing the decisive-green comment outright. find_sticky, which round 3 cited as its model, carries a THIRD filter for exactly this. Now matches `.user.login` against the minted token's own login, taken from create-github-app-token's `app-slug` output — which identifies the installation whether `bot_app_id` is a numeric App ID or a Client ID, so the reason round 3 widened to all bots no longer applies. Abort mid-loop. Round 3's PATCH-every-match loop runs under `set -euo pipefail`, so the first failing PATCH aborted the step and left the remaining copies stale — the exact split-brain the loop was added to prevent. A 404 is expected here: the loop only has multiple ids because duplicates happen, and one being deleted between the list and the write is a real race. Failures are now tolerated per id and reported. Also from round 4: - The round-3 stem-component widening fixed an over-count by introducing an UNDER-count: `api.spec.types.ts`, `openapi.spec.client.ts` classified as test. That collided head-on with the reasoning three comments above it — `spec` is kept out of testPathSegments precisely because it names OpenAPI production artifacts here. `test` now matches in any non-first component (keeping the `foo.test.d.ts` fix); `spec` only as the final stem component. - sanitizePath missed C1 controls (U+0080–U+009F are unicode.Cc, not caught by `r < 0x20`) and the Zl/Zp separators U+2028/U+2029. U+0085 and U+2028 render as line breaks — the same list-escape the function exists to block — and U+009B opens ANSI sequences in the public run log. - sanitizePath truncated the tail, discarding the evidence of classification: `<150 bytes of prefix>/tests/prod.go` rendered as a production-looking path in the "Largest excluded test files" list, defeating what that list is for. Now elides the MIDDLE and keeps the tail. - report() still discarded its GITHUB_STEP_SUMMARY errors, and this PR newly makes that summary the sole carrier on fork and Dependabot PRs, where the bot comment cannot post at all. - strings.ToLower applies Unicode folding, under which U+212A KELVIN SIGN lowercases to `k`, so `__MOCS__` folded to `__mocks__`. The casing tolerance is for .NET/Unity trees, so it now folds ASCII only. - The caller doc claimed the excluded-file list lets the number be "audited" and advertised `*.spec.*` generically. Both overstated the code; hedged and corrected to describe the actual asymmetric rule. Deferred, deliberately, and recorded in the PR description: the comment CREATION race. PATCH-every-match heals duplicates on a later run but two simultaneous runs can still both POST, and round 4 is right that `cancel-in-progress: false` would close it without the stranded-check problem I objected to. It is pre-existing behaviour, the remedy changes queueing semantics for every consumer of this reusable, and it wants its own change and its own review rather than riding along here. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 49 +++++++++++++------ docs/callers/pr-size.md | 15 ++++-- scripts/check-pr-size/main.go | 75 ++++++++++++++++++++++++++---- scripts/check-pr-size/main_test.go | 32 ++++++++++++- scripts/check-pr-size/size.go | 45 ++++++++++++++---- scripts/check-pr-size/size_test.go | 11 ++++- 6 files changed, 187 insertions(+), 40 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 0d01319..3db650b 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -352,6 +352,11 @@ jobs: GH_TOKEN: ${{ steps.bot.outputs.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + # The minted token's OWN bot login. app-slug comes from the token + # mint, so it identifies this installation whether `bot_app_id` was + # given as a numeric App ID or a Client ID (`Iv1.…`) — which is what + # made matching on the app id unreliable. + BOT_LOGIN: ${{ steps.bot.outputs.app-slug }}[bot] run: | set -euo pipefail if [ ! -s report/pr-size-report.md ]; then @@ -370,16 +375,20 @@ jobs: # scripts/pr-risk/publish-risk-surfaces.sh deliberately — same hazards, # so the same shape rather than a second dialect of it. # - # .user.type == "Bot" — the marker is published in this PUBLIC - # workflow file, so without an author filter a PR author can + # .user.login == our own bot — the marker is published in this + # PUBLIC workflow file, so without an author filter a PR author can # pre-seed a comment carrying it and we would PATCH THEIRS instead # of posting ours, after which they rewrite it freely. Since # `exclude_tests` makes this comment the ONLY signal on a green # test-heavy PR, that is a hole straight through the guarantee. - # Type is used rather than the app id because - # create-github-app-token also accepts a CLIENT id (`Iv1.…`) for - # `app-id`; a caller wiring that would never match the numeric app - # id and would silently stack a new comment on every push. + # Matching the LOGIN (not merely `.user.type == "Bot"`) is what + # keeps us to OUR comments: any other App or github-actions[bot] + # workflow in a consumer repo that echoes author-controlled text as + # its first line would otherwise be claimed as ours and PATCHed + # away — and while such a comment exists the POST branch below + # never runs, suppressing the decisive-green comment entirely. + # `.user.type` is kept as a cheap second condition, mirroring + # find_sticky. # # startswith, not contains — the body renders the marker as its # FIRST line, while another bot QUOTING this comment carries the @@ -388,13 +397,14 @@ jobs: # bot posts cursor-review and groom comments too, so an author can # induce this by quoting the marker in the PR body. # - # The marker is passed as jq DATA (--arg), never spliced into the - # program text, so no value can turn the filter into a syntax error — - # which `set -euo pipefail` plus this step's continue-on-error would - # convert into a silent "no comment posted". + # Both the marker and the login are passed as jq DATA (--arg), never + # spliced into the program text, so no value can turn the filter into a + # syntax error — which `set -euo pipefail` plus this step's + # continue-on-error would convert into a silent "no comment posted". existing="$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ - | jq -r --arg m "$MARKER" ' - .[] | select((.user.type // "") == "Bot") + | jq -r --arg m "$MARKER" --arg login "$BOT_LOGIN" ' + .[] | select((.user.login // "") == $login) + | select((.user.type // "") == "Bot") | select(((.body // "") | startswith($m))) | .id')" if [ -n "$existing" ]; then @@ -409,9 +419,20 @@ jobs: # Unquoted on purpose: the ids are numeric and newline-separated, so # word splitting is exactly the right parse (and a heredoc's # terminator cannot be indented inside this YAML block scalar). + # + # Per-id failure is TOLERATED rather than fatal. Under + # `set -euo pipefail` a bare call would abort the loop on the first + # error and leave the remaining copies showing a stale verdict — + # precisely the split-brain this loop exists to prevent. A 404 is an + # expected case: the loop only has more than one id because + # duplicates happen, and a maintainer (or another run) deleting one + # between the list and the write is a real race. for id in $existing; do - gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null - echo "Updated sticky size comment ${id}." + if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null 2>&1; then + echo "Updated sticky size comment ${id}." + else + echo "Could not update sticky size comment ${id} (deleted concurrently?); continuing." >&2 + fi done elif [ "$OVER" = "true" ] || [ "$DECISIVE" = "true" ]; then gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index 7f590f5..6ef36fb 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -93,8 +93,9 @@ and read `.gitattributes` from the base ref precisely so a PR cannot exempt itself — test detection only looks at the path. Nothing stops production code being parked in `tests/` to duck the cap. That is why it is off by default, and why the excluded total is always printed on its own line: the report shows -`Excluded (tests): N` next to the counted number, and names the largest excluded -files so the number can be audited rather than taken on trust. When the +`Excluded (tests): N` next to the counted number, and lists the largest excluded +files — the biggest contributors, not a complete accounting (it stops at 10), but +enough to sanity-check that the exclusion is really tests. When the exclusion is the *only* reason a PR is under the cap, the sticky comment posts even though the check is green — otherwise the number would live solely in the Actions step summary in precisely the case that matters, and a 5,000-line @@ -113,9 +114,13 @@ Note also that `extra_generated_globs` (below) classifies matches as trigger the green-check comment. A repo leaning on it for an unusual test layout opts out of this visibility guarantee. -Recognized: `*_test.go`; `test_*.py`, -`*_test.py`, `conftest.py`; `*.test.*` / `*.spec.*` for `.js .jsx .mjs .cjs .ts -.tsx .mts .cts`; and any file under a `test/`, `tests/`, `testing/`, +Recognized: `*_test.go`; `test_*.py`, `*_test.py`, `conftest.py`; for +`.js .jsx .mjs .cjs .ts .tsx .mts .cts`, a `test` component anywhere in the +filename after the first (so `Button.test.tsx` **and** the type-test form +`api.test.d.ts`) or a `spec` component immediately before the extension +(`api.spec.ts`) — `spec` is deliberately narrower, because `api.spec.types.ts` +and `openapi.spec.client.ts` are OpenAPI *production* files in this org and +excluding them would under-count; and any file under a `test/`, `tests/`, `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` **directory** segment (segment matching is case-insensitive, so `Tests/` and `TestData/` work too; the file-name rules stay case-sensitive because their toolchains define diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 705e88a..628241a 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -407,16 +407,60 @@ func sanitizePath(path string) string { case r == '`': // Would close the code span the caller wraps this in. out = '\'' - case r < 0x20 || r == 0x7f || unicode.Is(unicode.Cf, r): - // Newlines, CR, other C0 controls, and invisible format characters. + case unicode.Is(unicode.Cc, r) || unicode.Is(unicode.Cf, r) || + unicode.Is(unicode.Zl, r) || unicode.Is(unicode.Zp, r): + // Cc covers C0 AND C1 — U+0085 (NEL) is a C1 control that renders as + // a line break, as do the Zl/Zp separators U+2028/U+2029, so all + // three are the same list-escape this function exists to block. Cf + // covers the invisible format characters (bidi overrides/isolates, + // ZWJ). U+009B (CSI) additionally lets ANSI sequences into the + // public run log even though ESC itself is a C0 control. out = '?' } - if b.Len()+utf8.RuneLen(out) > maxPathDisplay { - return b.String() + "…" - } b.WriteRune(out) } - return b.String() + return elideMiddle(b.String(), maxPathDisplay) +} + +// elideMiddle bounds s to max bytes by dropping the MIDDLE, never the tail. +// Truncating the tail would discard the very evidence the report is being read +// for: a long path's classifying segment usually sits at the end, so +// `<150 bytes of prefix>/tests/prod.go` would render as a truncated +// production-looking path in the "Largest excluded test files" list — defeating +// the auditability that list exists to provide — and distinct paths sharing a +// prefix would render identically. Cuts land only on rune boundaries, so the +// result is always valid UTF-8. +func elideMiddle(s string, max int) string { + const ellipsis = "…" + if len(s) <= max { + return s + } + if max <= len(ellipsis) { + return ellipsis + } + budget := max - len(ellipsis) + headBudget, tailBudget := budget/2, budget-budget/2 + + head := 0 + for i := range s { // range over a string yields rune START offsets + if i > headBudget { + break + } + head = i + } + tail := len(s) + for i := len(s); i > 0; { + _, size := utf8.DecodeLastRuneInString(s[:i]) + if len(s)-(i-size) > tailBudget { + break + } + i -= size + tail = i + } + if tail < head { + tail = head + } + return s[:head] + ellipsis + s[tail:] } // topFiles renders a collapsed list of up to 10 matching files, largest first @@ -448,10 +492,23 @@ func topFiles(files []FileChange, summary string, keep func(FileChange) bool) st func report(res Result, mode, bypassLabel string) { summary := renderReport(res, mode, bypassLabel) fmt.Println(summary) + // Errors are surfaced, not swallowed: this PR makes the step summary the + // SOLE carrier of the excluded-test total wherever the bot comment cannot + // post (fork and Dependabot PRs never receive the secret), so a failed write + // silently removes those PRs' only surface. 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) + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644) + if err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: cannot open GITHUB_STEP_SUMMARY: %v\n", err) + return + } + _, werr := fmt.Fprintln(f, summary) + cerr := f.Close() + if werr != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: writing GITHUB_STEP_SUMMARY failed: %v\n", werr) + } + if cerr != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: closing GITHUB_STEP_SUMMARY failed: %v\n", cerr) } } } diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index ff64209..6e0a8b2 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -597,6 +597,13 @@ func TestSanitizePath(t *testing.T) { }, {"bidi isolate is replaced", "a⁦b⁩c.go", "a?b?c.go"}, {"zero-width joiner is replaced", "a‍b.go", "a?b.go"}, + // C1 controls are NOT caught by `r < 0x20`; U+0085 (NEL) renders as a + // line break, and U+009B (CSI) opens ANSI sequences in the public log. + {"C1 next-line is replaced", "a…b.go", "a?b.go"}, + {"C1 CSI is replaced", "a›b.go", "a?b.go"}, + // Zl/Zp separators are line breaks too. + {"line separator is replaced", "a
b.go", "a?b.go"}, + {"paragraph separator is replaced", "a
b.go", "a?b.go"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -613,8 +620,29 @@ func TestSanitizePath(t *testing.T) { if len(got) > maxPathDisplay+len("…") { t.Errorf("len = %d, want <= %d", len(got), maxPathDisplay+len("…")) } - if !strings.HasSuffix(got, "…") { - t.Errorf("truncated path should be marked with an ellipsis, got %q", got[len(got)-10:]) + // The ellipsis sits in the MIDDLE, not at the end — the tail is kept. + if !strings.Contains(got, "…") { + t.Errorf("truncated path should be marked with an ellipsis, got %q", got) + } + if !strings.HasSuffix(got, ".go") { + t.Errorf("the tail should survive elision, got %q", got) + } + }) + + // Truncating the TAIL would discard exactly the segment that explains why a + // file was classified as a test, in the list whose purpose is checking that + // classification. Eliding the middle keeps it. + t.Run("truncation keeps the classifying tail", func(t *testing.T) { + t.Parallel() + got := sanitizePath(strings.Repeat("a", 400) + "/tests/prod.go") + if !strings.HasSuffix(got, "/tests/prod.go") { + t.Errorf("the tail that shows WHY this counted as a test was truncated away: %q", got) + } + if !strings.Contains(got, "…") { + t.Errorf("an elided path should say so: %q", got) + } + if len(got) > maxPathDisplay+len("…") { + t.Errorf("len = %d, over bound", len(got)) } }) diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 8987772..956e001 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -288,13 +288,29 @@ func hasTestSegment(path string) bool { return false // no directory part at all } for _, seg := range strings.Split(dir[:slash], "/") { - if testPathSegments[strings.ToLower(seg)] { + if testPathSegments[asciiLower(seg)] { return true } } return false } +// asciiLower lowercases A–Z only. strings.ToLower applies Unicode simple case +// folding, under which U+212A KELVIN SIGN lowercases to `k` — so a directory +// named `__MOC⟨U+212A⟩S__` would fold to `__mocks__` and drop everything beneath +// it out of the count. The casing tolerance here exists for .NET/Unity trees +// (`Tests/`, `TestData/`), which is an ASCII concern, so ASCII is all it should +// do. +func asciiLower(s string) string { + b := []byte(s) + for i := range b { + if b[i] >= 'A' && b[i] <= 'Z' { + b[i] += 'a' - 'A' + } + } + return string(b) +} + // isTestFileName reports whether a file's base name follows a test-file naming // convention: an exact scaffolding name, a language suffix, Python's `test_` // prefix, or the JS/TS `.test.`/`.spec.` infix before a source extension. @@ -311,15 +327,26 @@ func isTestFileName(base string) bool { return true } if dot := strings.LastIndex(base, "."); dot > 0 && jsTestExts[base[dot:]] { - // Any dot-separated component of the stem AFTER the first being exactly - // `test` or `spec` marks a test file. Checking components rather than - // only the one adjoining the extension also catches type tests - // (`foo.test.d.ts`), which match the `*.test.*` convention the docs - // advertise. Requiring a NON-first component is what keeps a - // hand-written module literally named `spec.ts` counted. + // A dot-separated stem component that is exactly `test` or `spec` marks + // a test file, but the two are NOT symmetric: + // + // `test` matches in any component after the first, which catches type + // tests (`foo.test.d.ts`) — the `*.test.*` convention the docs + // advertise. + // `spec` matches ONLY as the final stem component. In this org `spec` + // names OpenAPI artifacts, so `api.spec.types.ts` and + // `openapi.spec.client.ts` are PRODUCTION files — the same reasoning + // that keeps `spec`/`specs` out of testPathSegments above. Matching + // them would be an under-count, which is the unsafe direction: + // failing to exclude a test file only over-counts, but excluding a + // production file shrinks the number the cap protects. + // + // Requiring a non-first component is what keeps a hand-written module + // literally named `spec.ts` counted. parts := strings.Split(base[:dot], ".") - for _, p := range parts[1:] { - if p == "test" || p == "spec" { + rest := parts[1:] + for i, p := range rest { + if p == "test" || (p == "spec" && i == len(rest)-1) { return true } } diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 88f943c..d614d9c 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -213,7 +213,7 @@ func TestIsTestPath(t *testing.T) { {"web/src/__tests__/render.tsx", true}, // Type tests — the `test` component need not adjoin the extension. {"web/src/api.test.d.ts", true}, - {"web/src/api.spec.d.ts", true}, + {"web/src/api.test.helpers.ts", true}, {"web/src/__snapshots__/Button.test.tsx.snap", true}, {"web/src/__mocks__/fs.ts", true}, // Directory conventions, at any depth @@ -240,6 +240,15 @@ func TestIsTestPath(t *testing.T) { // A `test`/`spec` component must not be the FIRST one. {"web/src/spec.helpers.ts", false}, {"web/src/manifest.d.ts", false}, + // `spec` matches ONLY as the final stem component: in this org these are + // OpenAPI production artifacts, and excluding them would UNDER-count. + {"web/src/api.spec.types.ts", false}, + {"web/src/openapi.spec.client.ts", false}, + {"web/src/payments.spec.gen.ts", false}, + {"web/src/api.spec.d.ts", false}, + // Unicode case folding must not widen the segment match: U+212A KELVIN + // SIGN lowercases to `k` under strings.ToLower. + {"src/__MOC\u212AS__/fs.ts", false}, {"testify.go", false}, {"latest_test_results.md", false}, // `spec/` holds OpenAPI schemas in this org, not RSpec suites. From a2bf0017f354eb378a6a2c8adbb432ba0bea67b3 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Fri, 7 Aug 2026 10:51:52 -0700 Subject: [PATCH 6/9] feat(pr-size): annotate a decisive test exclusion so forks are covered too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised a gap that the docs described honestly but the code did not close: on fork and Dependabot PRs the `exclude_tests` LOOSENING applies while the compensating VISIBILITY does not. Those runs never receive BOT_APP_PRIVATE_KEY, so the green-check comment cannot post even for a caller that configured the App correctly. That inverts the trust gradient — the least-trusted contributions get the least-scrutinized guardrail — and it is silent by construction, because a green check with no comment is indistinguishable from a PR that passed on its own merits. `ExclusionDecisive` was already computed in the read-only job, so the information existed on the trusted side of the split with nowhere visible to go. The size job now emits a `::warning` annotation when the exclusion is decisive. An annotation needs no credentials and no added permissions, so it reaches every PR including forks, and it carries the same figures as the comment. The invariant this feature rests on — the exclusion is never invisible — now holds everywhere rather than only where a bot token happens to exist. The sticky comment remains the richer surface (in-conversation, survives pushes, flips to ✅), not the only one. Corrected the workflow header and caller doc, which both still said the total lives only in the step summary on those PRs — true before this step, not after. The caller doc now states what is genuinely still lost without the App on a fork PR (the sticky comment itself), rather than implying the numbers are hidden. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 30 ++++++++++++++++++++++++++---- docs/callers/pr-size.md | 20 ++++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 3db650b..1bc4cd8 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -47,10 +47,15 @@ name: PR Size Cap (reusable) # # 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. Note this also covers fork -# and Dependabot PRs, which never receive the secret — so on those the green -# check's excluded-test total lives only in the step summary even for a caller -# that configured the App. +# still carry the full report; no comment is posted. This also covers fork and +# Dependabot PRs, which never receive the secret at all. +# +# So that the credential-free path is not a silent hole, a decisive test +# exclusion ALSO emits a `::warning` annotation from the size job itself. That +# needs no secret, so it reaches fork and Dependabot PRs — the ones that would +# otherwise get the loosened cap with none of the visibility justifying it. The +# invariant "the exclusion is never invisible" therefore holds on every PR; +# the bot comment is the richer surface, not the only one. # # 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 @@ -250,6 +255,23 @@ jobs: --base "${BASE_SHA}" \ --head "${HEAD_SHA}" | tee "${RUNNER_TEMP}/pr-size-report.md" + - name: Annotate a decisive test exclusion + # An annotation is the ONE carrier that needs no bot credentials, so it + # is the only one that also reaches fork and Dependabot PRs — which + # never receive BOT_APP_PRIVATE_KEY and would otherwise get the loosened + # cap with none of the visibility that justifies it. That inversion is + # the worst shape this feature can take: the least-trusted contributions + # getting the least-scrutinized guardrail, silently, because a green + # check with no comment is indistinguishable from one that passed on its + # own merits. This fires on every PR regardless of secret availability, + # so the invariant "the exclusion is never invisible" holds everywhere. + if: always() && steps.check.outputs.tests_decisive == 'true' + env: + COUNTED: ${{ steps.check.outputs.counted }} + TESTS: ${{ steps.check.outputs.tests_excluded }} + run: | + echo "::warning title=Under the cap only via the test exclusion::This PR changes $((COUNTED + TESTS)) non-generated lines (${COUNTED} counted + ${TESTS} test) against a ${PR_SIZE_MAX_LINES}-line cap. exclude_tests is what brings it under — expected for a test-heavy change, surfaced so the real size is visible." + - name: Record over-cap flag # The comment job keys off these flags rather than the pr-size job's # result, so `warn` mode (job green) still comments on overage. They diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index 6ef36fb..ef2ca52 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -109,6 +109,26 @@ someone who opens the Actions step summary. **If you set `exclude_tests`, configure the App too** — otherwise you keep the loosening and lose the visibility that justifies it. +**Fork and Dependabot PRs are a blind spot — weigh this before opting in.** Those +runs never receive `BOT_APP_PRIVATE_KEY` (GitHub withholds secrets from them), so +the green-check comment cannot post there *even for a caller that configured the +App correctly*. The consequence is uncomfortable and worth stating bluntly: with +`exclude_tests: true`, a fork PR gets the **weaker cap** and loses the mechanism +that makes the weaker cap safe. The trust gradient inverts — the least-trusted +contributions get the least-scrutinized guardrail — and it is silent, because a +green check with no comment looks exactly like a PR that passed on its own +merits. + +**This is why a decisive exclusion also emits a check annotation.** The +annotation comes from the size job itself and needs no credentials, so it shows +up in the PR's Checks tab on fork and Dependabot PRs too. It carries the same +numbers as the comment (`counted + test` against the cap), so the invariant +holds everywhere — the bot comment is the richer surface, not the only one. + +What you still lose without the App on a fork PR is the *sticky* comment: the +in-conversation explanation that survives pushes and flips to ✅. If that matters +for your outside contributions, leave `exclude_tests` off. + Note also that `extra_generated_globs` (below) classifies matches as *generated*, not *test*: they never reach the excluded-test total and never trigger the green-check comment. A repo leaning on it for an unusual test layout From 232dfcb1e5aa74d361747e5f3dbd42e30ff82566 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Fri, 7 Aug 2026 14:50:52 -0700 Subject: [PATCH 7/9] fix(pr-size): narrow ambiguous test dirs to the repo root (BE-6791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The any-depth directory rule excluded PRODUCTION code, and not hypothetically. A consumer keeps its ArgoCD manifests — cluster RBAC, clusterrolebindings, cert issuers, gateway config — under `infrastructure/argocd/apps/testing/`, where `testing` names the deployment ENVIRONMENT, not test code. With `exclude_tests: true` a PR changing cluster RBAC did not count against the cap. That is close to the worst thing this feature could do. Directory matching is now three cases instead of one: 1. `__tests__/`, `__mocks__/`, `__snapshots__/`, `testdata/` — ANY depth. Nothing else is ever called these, and Go nests testdata by design. 2. `test/`, `tests/`, `testing/`, `e2e/` — repo ROOT only. 3. those four plus `it/` — directly under `src/`, because Maven/Gradle nest tests at `src/test/java` and `src/it` by convention. Without this case those repos would count their whole test tree — the feature quietly UNDER-delivering for them, the mirror of what it did to the ArgoCD tree. Verified against the real consumer, not just fixtures: 227 files under its root-level `testing/` (e2e, integration, smoke, synthetics) stay excluded, and all 31 files under `infrastructure/argocd/apps/testing/` are now counted. Zero misclassified either way. The exact ArgoCD paths are in the table test. This reverses a rejection I made in round 3. The panel proposed root-anchoring and I rejected it, asserting that `testing/` was load-bearing for the consumer so anchoring would break it. I never checked where those files were. They are at the root. The premise was false and the finding was correct. Deliberate cost: a NESTED ambiguous directory such as `services/checkout/e2e/` now counts. That is the safe direction — over-counting starts an argument, under-counting silently shrinks the number the cap protects. Also from round 5: - The PATCH-every-match loop only got half of round 4's promised fix. It tolerated failures but discarded gh's stderr and accumulated no status, so the step exited 0 even if EVERY patch failed, `steps.upsert.outcome` stayed success, the degraded-mode note never fired, and the log blamed every cause on a concurrent delete. Now keeps an rc, lets gh's stderr through, and exits non-zero (continue-on-error still prevents a red check). - The annotation printed the raw `max_lines` input rather than the cap actually applied; envInt falls back to the 1000 default for anything Atoi rejects, so it could name a cap never enforced — and on fork PRs the annotation is the only surface, with no comment to cross-check. The tool now emits the applied cap as `max_lines` and the annotation uses it. - A rename is classified by BOTH paths and counts as a test only if both agree. numstat books a rename's deletions against the destination, so `git mv src/big.go tests/big.go` charged removed production lines to an excluded test path. - elideMiddle returned a 3-byte ellipsis when asked for a bound below that, breaking its own contract. - Narrowed the annotation claim rather than overstating it: an annotation with no file/line reaches the run summary, the Details link and the Checks tab, but NOT the PR conversation or Files changed. On a fork PR the number is findable, not unprompted. Said so in both the workflow header and the caller doc. Not taken: widening the sticky-comment login match to an allow-list. Real (an App rename orphans existing stickies) but it needs a new input, and the failure is visible — a duplicate comment — rather than silent. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 48 +++++++++++--- docs/callers/pr-size.md | 64 +++++++++++++----- scripts/check-pr-size/main.go | 17 ++++- scripts/check-pr-size/size.go | 101 ++++++++++++++++++++--------- scripts/check-pr-size/size_test.go | 63 ++++++++++++++++-- 5 files changed, 229 insertions(+), 64 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 1bc4cd8..e63be45 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -19,9 +19,13 @@ name: PR Size Cap (reusable) # total so a PR is capped on the production code a reviewer must actually reason # about, not on its test coverage. Test files are recognized by naming # convention only (`*_test.go`, `test_*.py`/`*_test.py`/`conftest.py`, -# `*.test.ts`/`*.spec.ts` & friends, and paths under `test/`, `tests/`, -# `testing/`, `testdata/`, `e2e/`, `__tests__/`, `__mocks__/`, -# `__snapshots__/`). Unlike the generated-file rules this is a convention, not a +# `*.test.ts`/`*.spec.ts` & friends, plus test DIRECTORIES matched in three +# cases: `__tests__/`/`__mocks__/`/`__snapshots__/`/`testdata/` at any depth; +# `test/`/`tests/`/`testing/`/`e2e/` at the repo root only; and those four plus +# `it/` directly under `src/` for Maven/Gradle layouts. The root restriction is +# load-bearing — a consumer keeps production ArgoCD manifests under +# `infrastructure/argocd/apps/testing/`, and an any-depth rule excluded cluster +# RBAC from the cap). Unlike the generated-file rules this is a convention, not a # proof — nothing stops production code being parked in `tests/` — which is why # it is opt-in rather than the default. The excluded total is ALWAYS reported on # its own line, and when the exclusion is the ONLY reason a PR is under the cap @@ -53,9 +57,16 @@ name: PR Size Cap (reusable) # So that the credential-free path is not a silent hole, a decisive test # exclusion ALSO emits a `::warning` annotation from the size job itself. That # needs no secret, so it reaches fork and Dependabot PRs — the ones that would -# otherwise get the loosened cap with none of the visibility justifying it. The -# invariant "the exclusion is never invisible" therefore holds on every PR; -# the bot comment is the richer surface, not the only one. +# otherwise get the loosened cap with none of the visibility justifying it. +# +# Be precise about what that buys, because it is less than a comment: an +# annotation carrying no file/line renders on the run-summary page and behind +# the check's Details link, and in the Checks tab — NOT in the PR conversation +# (a passing check collapses to "All checks have passed") and NOT inline on +# Files changed. So on a fork PR the number is recorded and reachable, but it +# still takes a deliberate click; only the sticky comment puts it in front of a +# reviewer unprompted. Where the App is configured, the comment remains the +# real surface and the annotation is the backstop. # # 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 @@ -269,8 +280,15 @@ jobs: env: COUNTED: ${{ steps.check.outputs.counted }} TESTS: ${{ steps.check.outputs.tests_excluded }} + # The APPLIED cap, emitted by the tool — not the raw max_lines input. + # envInt falls back to the 1000 default for anything Atoi rejects (a + # value arriving as `1250.5` from a GitHub expression, or with stray + # whitespace from a forwarded var), so the input can name a cap that + # was never enforced. On fork PRs this annotation is the ONLY surface, + # with no comment to cross-check it against. + CAP: ${{ steps.check.outputs.max_lines }} run: | - echo "::warning title=Under the cap only via the test exclusion::This PR changes $((COUNTED + TESTS)) non-generated lines (${COUNTED} counted + ${TESTS} test) against a ${PR_SIZE_MAX_LINES}-line cap. exclude_tests is what brings it under — expected for a test-heavy change, surfaced so the real size is visible." + echo "::warning title=Under the cap only via the test exclusion::This PR changes $((COUNTED + TESTS)) non-generated lines (${COUNTED} counted + ${TESTS} test) against a ${CAP}-line cap. exclude_tests is what brings it under — expected for a test-heavy change, surfaced so the real size is visible." - name: Record over-cap flag # The comment job keys off these flags rather than the pr-size job's @@ -449,13 +467,25 @@ jobs: # expected case: the loop only has more than one id because # duplicates happen, and a maintainer (or another run) deleting one # between the list and the write is a real race. + rc=0 for id in $existing; do - if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null 2>&1; then + if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null; then echo "Updated sticky size comment ${id}." else - echo "Could not update sticky size comment ${id} (deleted concurrently?); continuing." >&2 + # gh's stderr is deliberately NOT discarded: a 403 from + # permission drift, a 422 on an over-long body and a concurrent + # delete all land here, and swallowing the message would + # misattribute every one of them to "deleted concurrently". + echo "Failed to update sticky size comment ${id} (see gh error above); continuing." >&2 + rc=1 fi done + # Exit non-zero if ANY patch failed, so steps.upsert.outcome is + # 'failure' and the degraded-mode note below actually fires. + # continue-on-error already keeps this from reddening the check — + # without this the step exits 0 even when every patch failed and the + # PR silently keeps a stale verdict on a green run. + [ "$rc" -eq 0 ] || exit 1 elif [ "$OVER" = "true" ] || [ "$DECISIVE" = "true" ]; then gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null if [ "$OVER" = "true" ]; then diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index ef2ca52..cbb799f 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -119,11 +119,18 @@ contributions get the least-scrutinized guardrail — and it is silent, because green check with no comment looks exactly like a PR that passed on its own merits. -**This is why a decisive exclusion also emits a check annotation.** The -annotation comes from the size job itself and needs no credentials, so it shows -up in the PR's Checks tab on fork and Dependabot PRs too. It carries the same -numbers as the comment (`counted + test` against the cap), so the invariant -holds everywhere — the bot comment is the richer surface, not the only one. +**This is why a decisive exclusion also emits a check annotation.** It comes from +the size job itself and needs no credentials, so it reaches fork and Dependabot +PRs, carrying the same numbers as the comment (`counted + test` against the +applied cap). + +Be clear about what it does *not* buy, though. An annotation with no file/line +renders on the run-summary page, behind the check's **Details** link, and in the +**Checks** tab — but **not** in the PR conversation (a passing check collapses to +"All checks have passed") and **not** inline on Files changed. So on a fork PR the +number is recorded and reachable, but still a deliberate click away. Only the +sticky comment puts it in front of a reviewer unprompted. Treat the annotation as +a backstop that makes the number *findable*, not as parity with the comment. What you still lose without the App on a fork PR is the *sticky* comment: the in-conversation explanation that survives pushes and flips to ✅. If that matters @@ -134,19 +141,40 @@ Note also that `extra_generated_globs` (below) classifies matches as trigger the green-check comment. A repo leaning on it for an unusual test layout opts out of this visibility guarantee. -Recognized: `*_test.go`; `test_*.py`, `*_test.py`, `conftest.py`; for -`.js .jsx .mjs .cjs .ts .tsx .mts .cts`, a `test` component anywhere in the -filename after the first (so `Button.test.tsx` **and** the type-test form -`api.test.d.ts`) or a `spec` component immediately before the extension -(`api.spec.ts`) — `spec` is deliberately narrower, because `api.spec.types.ts` -and `openapi.spec.client.ts` are OpenAPI *production* files in this org and -excluding them would under-count; and any file under a `test/`, `tests/`, `testing/`, -`testdata/`, `e2e/`, `__tests__/`, `__mocks__/` or `__snapshots__/` **directory** -segment (segment matching is case-insensitive, so `Tests/` and `TestData/` work -too; the file-name rules stay case-sensitive because their toolchains define -them in lowercase). `spec/` is deliberately *not* a test directory — in this org it holds -OpenAPI schemas, which are production artifacts. For a layout these miss, add -`extra_generated_globs` (they land in the generated bucket instead). +**Recognized as test files.** File names: `*_test.go`; `test_*.py`, `*_test.py`, +`conftest.py`; and for `.js .jsx .mjs .cjs .ts .tsx .mts .cts`, a `test` +component anywhere in the name after the first (`Button.test.tsx`, and the +type-test form `api.test.d.ts`) or a `spec` component immediately before the +extension (`api.spec.ts`) — `spec` is narrower on purpose, because +`api.spec.types.ts` and `openapi.spec.client.ts` are OpenAPI *production* files +here and excluding them would under-count. + +**Directories are matched in three cases, not one**, because the names are not +equally trustworthy: + +| Case | Segments | Where they match | +|---|---|---| +| 1 | `__tests__/`, `__mocks__/`, `__snapshots__/`, `testdata/` | **any depth** — nothing else is ever called these, and Go nests `testdata` by design | +| 2 | `test/`, `tests/`, `testing/`, `e2e/` | **repo root only** | +| 3 | the same four, plus `it/` | **directly under `src/`** — Maven/Gradle nest tests at `src/test/java` and `src/it` | + +The root restriction in case 2 is not fussiness, it is a bug fix. A consumer +keeps production ArgoCD manifests — cluster RBAC, ingress, cert issuers — under +`infrastructure/argocd/apps/testing/`, where `testing` names the deployment +*environment*, not test code. Matching that name at any depth silently excluded +**cluster RBAC changes** from the cap. Root-anchoring keeps all 227 files of that +repo's real `testing/` tree excluded while counting the 31 infrastructure files. + +The cost is deliberate and worth knowing: a **nested** ambiguous directory such as +`services/checkout/e2e/` now counts. That is the safe direction — over-counting +starts an argument, under-counting silently shrinks the number the cap protects — +but if your tests live somewhere the three cases miss, they will be counted. + +Segment matching is case-insensitive over ASCII (so `Tests/` and `TestData/` +work); the file-name rules stay case-sensitive, because their toolchains define +them in lowercase. `spec/` is deliberately *not* a test directory — in this org +it holds OpenAPI schemas, which are production artifacts. For a layout these +miss, add `extra_generated_globs` (they land in the generated bucket instead). Leaving it off is a real choice, not just the safe one: a 5,000-line test diff is genuinely slow to review, and the cap is the only thing that says so. diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 628241a..13e4851 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -166,7 +166,15 @@ func classify(files []FileChange, base, head string, attr attrPolicy, extras Ext // Set before the binary guard so Result.Files reports a binary fixture // under testdata/ or __snapshots__/ as the test file it is. No numeric // effect — Changed() is 0 for binaries either way. - f.Test = IsTestPath(f.Path) + // + // A RENAME is classified by BOTH of its paths and counts as test only if + // both agree. `git diff --numstat` books a rename's deletions against the + // destination, so classifying on the destination alone would let + // `git mv src/big.go tests/big.go` charge removed PRODUCTION lines to an + // excluded test path — a refactor slipping under the cap by moving code + // into a test directory. Requiring both keeps the failure in the + // over-counting direction. + f.Test = IsTestPath(f.Path) && (f.OldPath == "" || IsTestPath(f.OldPath)) if f.Binary { continue } @@ -542,8 +550,11 @@ func writeGitHubOutputs(res Result) { // tests_decisive while the process still exits 0, and the comment job then // reads the absent flag as false and silently skips the green-check comment // — the very failure this output exists to close. - _, werr := fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\n", - !res.OK, res.Counted, testsExcluded, res.ExclusionDecisive()) + // max_lines is the APPLIED cap, so consumers annotate the number actually + // enforced rather than the raw input — envInt silently falls back to the + // default for any value Atoi rejects. + _, werr := fmt.Fprintf(f, "over_cap=%t\ncounted=%d\ntests_excluded=%d\ntests_decisive=%t\nmax_lines=%d\n", + !res.OK, res.Counted, testsExcluded, res.ExclusionDecisive(), res.Max) cerr := f.Close() if werr != nil { fmt.Fprintf(os.Stderr, "check-pr-size: writing GITHUB_OUTPUT failed: %v\n", werr) diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 956e001..21f1bcb 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -45,32 +45,52 @@ var lockfileNames = map[string]bool{ "uv.lock": true, } -// testPathSegments are directory names that mark everything beneath them as -// test code. Matching is on whole, slash-delimited path SEGMENTS and never on -// substrings, so `contest/`, `attestation/` and `latest.go` are untouched. Only -// directory segments are considered (never the file name itself), so a -// hand-written file literally named `test` is still counted. +// Test directories are matched in THREE cases, not one, because the segment +// names are not equally trustworthy. Matching every name at every depth is what +// caused the bug this split exists to fix: a real consumer keeps its production +// ArgoCD manifests — cluster RBAC, ingress, cert issuers — under +// `infrastructure/argocd/apps/testing/`, where `testing` names the target +// ENVIRONMENT, not test code. An any-depth rule silently excluded cluster RBAC +// changes from the cap, which is close to the worst thing this feature could do. // -// Segment matching is case-INSENSITIVE, so .NET/C#/Unity trees (`Tests/`, -// `TestData/`, `E2E/`) are recognized. The file-name rules below stay -// case-sensitive on purpose: `_test.go` and `conftest.py` are spelled in -// lowercase by their toolchains, so a capitalized variant is not the convention -// and matching it would only add false positives. -// -// `spec`/`specs` are deliberately absent: in this org those hold API schemas -// (OpenAPI), which are production artifacts. The unambiguous `*.spec.ts` -// file-name convention is handled below instead. -var testPathSegments = map[string]bool{ +// Matching is on whole, slash-delimited SEGMENTS, never substrings, so +// `contest/`, `attestation/` and `latest.go` are untouched, and it is +// case-insensitive over ASCII so .NET/Unity trees (`Tests/`, `TestData/`) work. +// Only DIRECTORY segments are considered, never the file name itself. + +// unambiguousTestSegments name test code and nothing else, so they are matched +// at ANY depth. Nobody names a production directory `__snapshots__`, and Go's +// `testdata` is defined by the toolchain to be nested. +var unambiguousTestSegments = map[string]bool{ "__mocks__": true, // Jest/Vitest manual mocks "__snapshots__": true, // Jest/Vitest snapshots "__tests__": true, // Jest/Vitest - "e2e": true, - "test": true, // also covers Maven/Gradle's src/test/... - "testdata": true, // Go's fixture convention - "testing": true, - "tests": true, + "testdata": true, // Go's fixture convention, nested by design +} + +// ambiguousTestSegments usually mean tests but also legitimately name an +// environment or product area, so they are matched ONLY at the repo root or +// directly under a source root (see srcRoots). Root-level `testing/` is a test +// tree; `infrastructure/argocd/apps/testing/` is a deployment target. +var ambiguousTestSegments = map[string]bool{ + "e2e": true, + "test": true, + "testing": true, + "tests": true, } +// srcRoots are directories under which an ambiguous segment is still a test +// directory — Maven/Gradle put tests at `src/test/java` and integration tests at +// `src/it`, both nested by convention. Without this case those repos would count +// their entire test tree, so the feature would quietly UNDER-deliver for them, +// which is the mirror of the over-delivery it caused for the ArgoCD tree. +var srcRoots = map[string]bool{"src": true} + +// srcRootOnlyTestSegments are additionally allowed directly under a srcRoot. +// `it` is Maven failsafe's integration-test directory; it is far too generic to +// honour at the repo root. +var srcRootOnlyTestSegments = map[string]bool{"it": true} + // testFileSuffixes are base-name suffixes that mark test code. Each carries its // own leading separator, so a production file that merely ENDS in the word // (`latest.go`, `contest.py`) is not a match. @@ -95,7 +115,13 @@ var jsTestExts = map[string]bool{ // FileChange is one file's contribution to the diff. type FileChange struct { - Path string + Path string + // OldPath is a rename's SOURCE path, empty otherwise. It exists so + // classification can be conservative: `git diff --numstat` books a rename's + // deletions against the destination, so `git mv src/big.go tests/big.go` + // would otherwise charge removed PRODUCTION lines to an excluded test path + // and let the change slip under the cap. See IsTestPath's use in classify. + OldPath string Added int Deleted int Binary bool @@ -190,6 +216,7 @@ func ParseNumstat(r io.Reader) ([]FileChange, error) { if rec == "" { continue // trailing NUL or stray separator } + var oldPath string parts := strings.SplitN(rec, "\t", 3) if len(parts) != 3 { return nil, fmt.Errorf("malformed numstat record: %q", rec) @@ -201,10 +228,10 @@ func ParseNumstat(r io.Reader) ([]FileChange, error) { if i+2 >= len(tokens) || tokens[i+2] == "" { return nil, fmt.Errorf("truncated rename record: %q", rec) } - path = tokens[i+2] + oldPath, path = tokens[i+1], tokens[i+2] i += 2 } - fc := FileChange{Path: path} + fc := FileChange{Path: path, OldPath: oldPath} if parts[0] == "-" || parts[1] == "-" { fc.Binary = true changes = append(changes, fc) @@ -278,20 +305,34 @@ func IsTestPath(path string) bool { return hasTestSegment(path) || isTestFileName(baseName(path)) } -// hasTestSegment reports whether any DIRECTORY segment of path is a test -// directory. The final segment (the file name) is excluded so the rule cannot -// fire on a file that merely shares a name with a test directory. +// hasTestSegment reports whether path sits under a test directory, applying the +// three cases documented on the segment tables above. The final segment (the +// file name) is never considered, so a file merely sharing a name with a test +// directory is still counted. func hasTestSegment(path string) bool { - dir := path - slash := strings.LastIndex(dir, "/") + slash := strings.LastIndex(path, "/") if slash < 0 { return false // no directory part at all } - for _, seg := range strings.Split(dir[:slash], "/") { - if testPathSegments[asciiLower(seg)] { + segs := strings.Split(path[:slash], "/") + for i := range segs { + segs[i] = asciiLower(segs[i]) + } + // Case 1 — unambiguous names, any depth. + for _, seg := range segs { + if unambiguousTestSegments[seg] { return true } } + // Case 2 — ambiguous names, repo root only. + if ambiguousTestSegments[segs[0]] { + return true + } + // Case 3 — ambiguous names (plus `it`) directly under a source root. + if len(segs) > 1 && srcRoots[segs[0]] && + (ambiguousTestSegments[segs[1]] || srcRootOnlyTestSegments[segs[1]]) { + return true + } return false } diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index d614d9c..3149b9f 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -45,9 +45,11 @@ func TestParseNumstat(t *testing.T) { }, { // -z renames: empty path field, then old-path and new-path tokens. - name: "rename resolves to new path", + // The SOURCE is kept too: a rename's deletions are booked against the + // destination, so classification needs both paths to stay conservative. + name: "rename resolves to new path and keeps the source", input: nul("1\t1\t", "dir/old/file.go", "dir/new/file.go"), - want: []FileChange{{Path: "dir/new/file.go", Added: 1, Deleted: 1}}, + want: []FileChange{{Path: "dir/new/file.go", OldPath: "dir/old/file.go", Added: 1, Deleted: 1}}, }, { // A file whose literal name contains " => " must NOT be treated as a @@ -222,10 +224,20 @@ func TestIsTestPath(t *testing.T) { {"src/Tests/FooTests.cs", true}, {"src/TestData/golden.json", true}, {"E2E/Checkout.cs", true}, - {"services/checkout/e2e/flow.go", true}, {"test/helpers.rb", true}, {"src/test/java/com/x/FooTest.java", true}, - {"internal/testing/harness.go", true}, + + // --- Case 2/3: AMBIGUOUS segments only at the root or a source root --- + // Root-level test trees (the real ones in the consumer that drove this). + {"testing/e2e/framework/harness.go", true}, + {"testing/integration/helper.go", true}, + {"testing/smoke/result.go", true}, + {"testing/synthetics/probe.go", true}, + {"tests/helpers.rb", true}, + // Maven/Gradle nest their tests by convention — case 3 keeps them. + {"src/it/java/com/x/FooIT.java", true}, + // Unambiguous names stay matched at ANY depth (case 1). + {"pkg/deep/nested/testdata/golden.json", true}, // --- False positives the substring-matching naive version would hit --- {"contest/leaderboard.go", false}, @@ -255,6 +267,21 @@ func TestIsTestPath(t *testing.T) { {"services/checkout/openapi.yaml", false}, {"spec/openapi.yaml", false}, {"api/specs/v1.json", false}, + // THE REGRESSION THAT DROVE THE THREE-CASE SPLIT: `testing` here names a + // deployment ENVIRONMENT, and these are production ArgoCD manifests — + // cluster RBAC, ingress, cert issuers. They must COUNT against the cap. + {"infrastructure/argocd/apps/testing/charts/ephemeral-ingress/base/templates/clusterrole.yaml", false}, + {"infrastructure/argocd/apps/testing/charts/ephemeral-ingress/base/templates/clusterrolebinding.yaml", false}, + {"infrastructure/argocd/apps/testing/charts/cert-manager/base/values.yaml", false}, + {"infrastructure/argocd/apps/testing/appsets/comfy-cloud-test.yaml", false}, + // Other nested ambiguous segments likewise count (safe direction). + {"services/checkout/e2e/flow.go", false}, + {"internal/testing/harness.go", false}, + {"apps/test/main.go", false}, + // A source root only rescues the segment DIRECTLY beneath it. + {"src/main/java/com/x/Test.java", false}, + {"vendor/src/test/java/x.java", false}, + // A file whose own name matches a test DIRECTORY is not a test file. {"cmd/test", false}, {"docs/testing", false}, @@ -628,3 +655,31 @@ func TestContentGeneratedRejectsSymlink(t *testing.T) { t.Errorf("contentGenerated must not follow symlinks") } } + +// TestRenameClassifiedConservatively is the anti-gaming guard for renames. +// `git diff --numstat` books a rename's deletions against the DESTINATION, so +// moving production code into a test directory would otherwise erase those +// lines from the count. +func TestRenameClassifiedConservatively(t *testing.T) { + t.Parallel() + files := []FileChange{ + // Production code moved INTO a test dir: must still count. + {Path: "tests/big.go", OldPath: "src/big.go", Added: 20, Deleted: 900}, + // A genuine move within the test tree: still a test. + {Path: "tests/b.go", OldPath: "tests/a.go", Added: 5, Deleted: 5}, + // Non-rename test file: unaffected. + {Path: "tests/c.go", Added: 10}, + } + classify(files, "", "", attrPolicy{}, Extras{}) + + want := map[string]bool{"tests/big.go": false, "tests/b.go": true, "tests/c.go": true} + for _, f := range files { + if f.Test != want[f.Path] { + t.Errorf("%s (from %q): Test = %v, want %v", f.Path, f.OldPath, f.Test, want[f.Path]) + } + } + res := Evaluate(files, Policy{Max: 1000, ExcludeTests: true}) + if res.Counted != 920 { + t.Errorf("Counted = %d, want 920 — the 900 deleted production lines must not vanish", res.Counted) + } +} From 0502e63870ff301ab95a1ac525b112ac05d46a41 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Fri, 7 Aug 2026 15:13:17 -0700 Subject: [PATCH 8/9] fix(pr-size): anonymise consumer paths; land the elideMiddle fix that did not (BE-6791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of round 6's findings are about my own reporting rather than the code, and they are the ones worth leading with. I LEAKED A PRIVATE CONSUMER'S LAYOUT INTO THIS PUBLIC REPO — again. The exact deployment path, what runs under it, a specific environment values file, and precise file counts, spread across the workflow header, size.go's comment, docs/callers/pr-size.md and the size_test.go fixtures. AGENTS.md prohibits exactly this, and I had already made and scrubbed the same class of leak earlier in this branch's history. Everything is now anonymised to an illustrative `deploy/envs/testing/` and the file counts are gone; the lesson survives intact because the lesson was never about which repo it was. A FIX I REPORTED AS LANDED HAD NOT LANDED. Round 5's reply said elideMiddle returned "" rather than an over-long ellipsis for a sub-ellipsis bound. The edit silently no-op'd and I never re-read the file, so the reply was false and the review ledger recorded it as closed. It is applied now, with a test that asserts the bound holds for max in 0..3 — the case that had nothing pinning it. The rest of round 6: - The `exclude_tests` input `description:` still advertised the pre-split rule. That string is what GitHub renders and the first thing a consumer reads, so a repo with `packages/foo/tests/` would have opted in expecting an exclusion it will not get. It now states all three cases and says a nested directory counts. - The conservative-rename guard covered `f.Test` but not `extras.Generated`, and Generated outranks Test in Evaluate — so `dist/**` in extra_generated_globs plus `git mv internal/big.go dist/big.go` reopened the same bypass through a different door, with no "largest excluded" list to audit it. Same both-paths conjunction applied. - Case 3 anchored `src` at index 0, so it only rescued SINGLE-module Maven/Gradle. `module-a/src/test/java` and `services/payment/src/it/java` — the standard multi-module shape — fell through and were counted, the exact outcome the case exists to prevent. `src` is now found at any depth. - `it` under a source root now requires a child segment, so a `src/it/` Italian locale tree keeps counting. `it` is ISO-639-1 as well as Maven failsafe, and dropping a locale tree is the unsafe direction. - The upsert exited non-zero on ANY per-id failure, including the 404 the comment above it calls expected — which made the degraded-mode note claim no comment was posted and blame a permission 403, affirmatively false in the case the loop was built for. It now fails only when NOTHING was patched, and reports partial failures without crying wolf. - envInt warns to stderr on an unparseable value instead of silently substituting the default, which for max_lines means silently LOOSENING a cap the caller meant to tighten. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 40 ++++++++++++++++++++++-------- docs/callers/pr-size.md | 8 +++--- scripts/check-pr-size/main.go | 33 ++++++++++++++++++------ scripts/check-pr-size/main_test.go | 12 +++++++++ scripts/check-pr-size/size.go | 31 +++++++++++++++++------ scripts/check-pr-size/size_test.go | 18 ++++++++------ 6 files changed, 106 insertions(+), 36 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index e63be45..413643a 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -23,8 +23,8 @@ name: PR Size Cap (reusable) # cases: `__tests__/`/`__mocks__/`/`__snapshots__/`/`testdata/` at any depth; # `test/`/`tests/`/`testing/`/`e2e/` at the repo root only; and those four plus # `it/` directly under `src/` for Maven/Gradle layouts. The root restriction is -# load-bearing — a consumer keeps production ArgoCD manifests under -# `infrastructure/argocd/apps/testing/`, and an any-depth rule excluded cluster +# load-bearing — a consumer keeps production deployment manifests under +# `deploy/envs/testing/`, and an any-depth rule excluded cluster # RBAC from the cap). Unlike the generated-file rules this is a convention, not a # proof — nothing stops production code being parked in `tests/` — which is why # it is opt-in rather than the default. The excluded total is ALWAYS reported on @@ -148,9 +148,15 @@ on: description: >- Keep test-file lines out of the counted total, so the cap measures production code rather than test coverage. Test files are matched by - naming convention (`*_test.go`, `test_*.py`, `*.test.ts`, - `*.spec.ts`, and paths under test/, tests/, testing/, testdata/, - e2e/, __tests__/, __mocks__/, __snapshots__/). Off by default: it + naming convention: file names (`*_test.go`, `test_*.py`, + `conftest.py`, `*.test.ts`, `*.spec.ts`); the directories + `__tests__/`, `__mocks__/`, `__snapshots__/`, `testdata/` at ANY + depth; and `test/`, `tests/`, `testing/`, `e2e/` only at the REPO + ROOT or under a `src/` directory (Maven/Gradle). The depth limit is + deliberate — those four names also label deployment environments and + product areas, and matching them anywhere excluded production + infrastructure from a real consumer's cap. A nested + `packages/foo/tests/` therefore COUNTS. Off by default: it loosens the cap, so each repo opts in deliberately. Excluded test lines are always reported separately, never silently dropped. For a layout these conventions miss, use `extra_generated_globs` (those @@ -468,23 +474,35 @@ jobs: # duplicates happen, and a maintainer (or another run) deleting one # between the list and the write is a real race. rc=0 + patched=0 + failed=0 for id in $existing; do if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -F body=@body.md >/dev/null; then echo "Updated sticky size comment ${id}." + patched=$((patched + 1)) else + failed=$((failed + 1)) # gh's stderr is deliberately NOT discarded: a 403 from # permission drift, a 422 on an over-long body and a concurrent # delete all land here, and swallowing the message would # misattribute every one of them to "deleted concurrently". echo "Failed to update sticky size comment ${id} (see gh error above); continuing." >&2 - rc=1 fi done - # Exit non-zero if ANY patch failed, so steps.upsert.outcome is - # 'failure' and the degraded-mode note below actually fires. - # continue-on-error already keeps this from reddening the check — - # without this the step exits 0 even when every patch failed and the - # PR silently keeps a stale verdict on a green run. + # Fail the step only when NOTHING was patched. A partial failure is + # the expected case this loop exists for — a duplicate 404s on a + # concurrent delete while the real sticky updates fine — and + # reporting that as a step failure makes the degraded-mode note + # below claim no comment was posted and blame a permission 403, + # which is affirmatively false and dilutes the signal for a genuine + # one. Nothing patched, on the other hand, means the verdict on this + # PR is stale and the note is exactly right. + if [ "$patched" -eq 0 ] && [ "$failed" -gt 0 ]; then + echo "No sticky comment could be updated (${failed} failed); the PR may show a stale verdict." >&2 + rc=1 + elif [ "$failed" -gt 0 ]; then + echo "Updated ${patched} sticky comment(s); ${failed} could not be updated (see errors above)." + fi [ "$rc" -eq 0 ] || exit 1 elif [ "$OVER" = "true" ] || [ "$DECISIVE" = "true" ]; then gh api -X POST "repos/${REPO}/issues/${PR_NUMBER}/comments" -F body=@body.md >/dev/null diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index cbb799f..20f5916 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -159,11 +159,11 @@ equally trustworthy: | 3 | the same four, plus `it/` | **directly under `src/`** — Maven/Gradle nest tests at `src/test/java` and `src/it` | The root restriction in case 2 is not fussiness, it is a bug fix. A consumer -keeps production ArgoCD manifests — cluster RBAC, ingress, cert issuers — under -`infrastructure/argocd/apps/testing/`, where `testing` names the deployment +keeps production deployment manifests — cluster RBAC and ingress config — under +`deploy/envs/testing/`, where `testing` names the deployment *environment*, not test code. Matching that name at any depth silently excluded -**cluster RBAC changes** from the cap. Root-anchoring keeps all 227 files of that -repo's real `testing/` tree excluded while counting the 31 infrastructure files. +**cluster RBAC changes** from the cap. Root-anchoring keeps that repo's whole +root-level `testing/` tree excluded while counting the deployment files. The cost is deliberate and worth knowing: a **nested** ambiguous directory such as `services/checkout/e2e/` now counts. That is the safe direction — over-counting diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 13e4851..12dd857 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -178,7 +178,13 @@ func classify(files []FileChange, base, head string, attr attrPolicy, extras Ext if f.Binary { continue } - if IsLockfile(f.Path) || extras.Generated(f.Path) || + // The same both-paths rule as f.Test above, for the same reason: without + // it, `dist/**` in extra_generated_globs plus + // `git mv internal/big.go dist/big.go` books the removed production + // lines into Generated — which takes precedence over Test in Evaluate + // and has no "largest excluded" list, so it is even less auditable. + extraGen := extras.Generated(f.Path) && (f.OldPath == "" || extras.Generated(f.OldPath)) + if IsLockfile(f.Path) || extraGen || attrGen[f.Path] || contentGenerated(f.Path, base, head) { f.Generated = true @@ -444,7 +450,10 @@ func elideMiddle(s string, max int) string { return s } if max <= len(ellipsis) { - return ellipsis + // Returning the ellipsis would EXCEED the bound this function promises. + // Unreachable at maxPathDisplay, but the helper is general and + // independently tested. + return "" } budget := max - len(ellipsis) headBudget, tailBudget := budget/2, budget-budget/2 @@ -692,13 +701,23 @@ func runGitCapped(maxBytes int64, args ...string) ([]byte, error) { return data, nil } +// envInt reads an int from the environment, falling back to def. An unparseable +// non-empty value WARNS rather than silently substituting: a `max_lines` that +// arrives as `1250.5` (GitHub expressions yield decimals) or with stray +// whitespace from a forwarded var would otherwise become the 1000 default — a +// LOOSER cap than the caller configured, indistinguishable in the report from a +// repo that genuinely meant 1000. func envInt(key string, def int) int { - if v := os.Getenv(key); v != "" { - if n, err := strconv.Atoi(v); err == nil { - return n - } + v := os.Getenv(key) + if v == "" { + return def } - return def + n, err := strconv.Atoi(v) + if err != nil { + fmt.Fprintf(os.Stderr, "check-pr-size: %s=%q is not an integer; falling back to %d\n", key, v, def) + return def + } + return n } func envBool(key string) bool { diff --git a/scripts/check-pr-size/main_test.go b/scripts/check-pr-size/main_test.go index 6e0a8b2..f07e1eb 100644 --- a/scripts/check-pr-size/main_test.go +++ b/scripts/check-pr-size/main_test.go @@ -792,3 +792,15 @@ func TestRunGitFoldsStderrIntoError(t *testing.T) { t.Errorf("runGit error should carry git's stderr diagnostic, got: %v", err) } } + +// TestElideMiddleHonoursTinyBounds pins the case round 5's reply claimed was +// fixed when it was not: asked for a bound below the ellipsis's own length, the +// function must not return something longer than the bound. +func TestElideMiddleHonoursTinyBounds(t *testing.T) { + t.Parallel() + for _, max := range []int{0, 1, 2, 3} { + if got := elideMiddle("some/long/path.go", max); len(got) > max { + t.Errorf("elideMiddle(max=%d) = %q (%d bytes) — exceeds its own bound", max, got, len(got)) + } + } +} diff --git a/scripts/check-pr-size/size.go b/scripts/check-pr-size/size.go index 21f1bcb..12311d1 100644 --- a/scripts/check-pr-size/size.go +++ b/scripts/check-pr-size/size.go @@ -48,8 +48,8 @@ var lockfileNames = map[string]bool{ // Test directories are matched in THREE cases, not one, because the segment // names are not equally trustworthy. Matching every name at every depth is what // caused the bug this split exists to fix: a real consumer keeps its production -// ArgoCD manifests — cluster RBAC, ingress, cert issuers — under -// `infrastructure/argocd/apps/testing/`, where `testing` names the target +// deployment manifests — cluster RBAC and ingress config — under +// `deploy/envs/testing/`, where `testing` names the target // ENVIRONMENT, not test code. An any-depth rule silently excluded cluster RBAC // changes from the cap, which is close to the worst thing this feature could do. // @@ -71,7 +71,7 @@ var unambiguousTestSegments = map[string]bool{ // ambiguousTestSegments usually mean tests but also legitimately name an // environment or product area, so they are matched ONLY at the repo root or // directly under a source root (see srcRoots). Root-level `testing/` is a test -// tree; `infrastructure/argocd/apps/testing/` is a deployment target. +// tree; `deploy/envs/testing/` is a deployment target. var ambiguousTestSegments = map[string]bool{ "e2e": true, "test": true, @@ -328,10 +328,27 @@ func hasTestSegment(path string) bool { if ambiguousTestSegments[segs[0]] { return true } - // Case 3 — ambiguous names (plus `it`) directly under a source root. - if len(segs) > 1 && srcRoots[segs[0]] && - (ambiguousTestSegments[segs[1]] || srcRootOnlyTestSegments[segs[1]]) { - return true + // Case 3 — ambiguous names directly under a source root, at ANY depth. + // `src` is not anchored to index 0 because the standard MULTI-module + // Maven/Gradle shape nests it — `module-a/src/test/java`, + // `services/payment/src/it/java` — and those are the layouts this case + // exists for. The cost is that a vendored `vendor/src/test/...` is also + // excluded; that is the over-counting-is-safer trade running the other way, + // and vendored trees are usually excluded as generated anyway. + for i := 0; i+1 < len(segs); i++ { + if !srcRoots[segs[i]] { + continue + } + next := segs[i+1] + if ambiguousTestSegments[next] { + return true + } + // `it` needs a CHILD segment (Maven failsafe always nests it as + // `src/it/`), because `it` is also the ISO-639-1 + // code for Italian and a bare `src/it/` locale tree must keep counting. + if srcRootOnlyTestSegments[next] && i+2 < len(segs) { + return true + } } return false } diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 3149b9f..2e02aba 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -268,19 +268,23 @@ func TestIsTestPath(t *testing.T) { {"spec/openapi.yaml", false}, {"api/specs/v1.json", false}, // THE REGRESSION THAT DROVE THE THREE-CASE SPLIT: `testing` here names a - // deployment ENVIRONMENT, and these are production ArgoCD manifests — - // cluster RBAC, ingress, cert issuers. They must COUNT against the cap. - {"infrastructure/argocd/apps/testing/charts/ephemeral-ingress/base/templates/clusterrole.yaml", false}, - {"infrastructure/argocd/apps/testing/charts/ephemeral-ingress/base/templates/clusterrolebinding.yaml", false}, - {"infrastructure/argocd/apps/testing/charts/cert-manager/base/values.yaml", false}, - {"infrastructure/argocd/apps/testing/appsets/comfy-cloud-test.yaml", false}, + // deployment ENVIRONMENT, and these are production deployment manifests — + // cluster RBAC and ingress config. They must COUNT against the cap. + {"deploy/envs/testing/charts/ingress/base/templates/clusterrole.yaml", false}, + {"deploy/envs/testing/charts/ingress/base/templates/clusterrolebinding.yaml", false}, + {"deploy/envs/testing/charts/certs/base/values.yaml", false}, + {"deploy/envs/testing/appsets/values.yaml", false}, // Other nested ambiguous segments likewise count (safe direction). {"services/checkout/e2e/flow.go", false}, {"internal/testing/harness.go", false}, {"apps/test/main.go", false}, // A source root only rescues the segment DIRECTLY beneath it. {"src/main/java/com/x/Test.java", false}, - {"vendor/src/test/java/x.java", false}, + // Multi-module Maven/Gradle — the standard shape, now covered by case 3. + {"module-a/src/test/java/com/x/FooTest.java", true}, + {"services/payment/src/it/java/com/x/FooIT.java", true}, + // `it` needs a child segment, so a bare Italian locale tree still counts. + {"src/it/messages.properties", false}, // A file whose own name matches a test DIRECTORY is not a test file. {"cmd/test", false}, From ab2066adbacb132d67b094546b4629a2b6293dc8 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Fri, 7 Aug 2026 15:22:01 -0700 Subject: [PATCH 9/9] fix(pr-size): close the rename bypass on lockfiles too (BE-6791) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit found the third door into the bypass I had closed twice. The both-paths rename rule covered `f.Test` (round 5) and `extras.Generated` (round 6), but `IsLockfile(f.Path)` still classified by destination alone — so `git mv src/big.go go.sum` booked the removed production lines into the generated bucket. Applying a guard to two of three call sites and reporting the class as closed is the same shape of error as the ledger entry round 6 caught. Every exclusion path now requires both paths to agree, with one regression test covering all three doors (lockfile, extra glob, test dir) plus a genuine lockfile churn as the negative control. Two documentation accuracies, both prose-drifted-from-code: - `src/it` requires a CHILD segment (so `src/it/java/FooIT.java` is excluded but `src/it/messages.properties` counts, since `it` is also ISO-639-1 Italian). The workflow input description and the caller doc both implied everything under `src/it/` was excluded. Case 3 also now says `src` is matched at any depth, which the table still described as directly-under-root. - The no-App paragraph said a decisive exclusion without the App is visible only in the step summary, which the annotation section three paragraphs later contradicts. Reconciled: without the App you lose the sticky comment — the only unprompted surface — while the annotation keeps the totals reachable via the Checks tab and Details link. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-size.yml | 4 +++- docs/callers/pr-size.md | 14 ++++++++------ scripts/check-pr-size/main.go | 7 ++++++- scripts/check-pr-size/size_test.go | 29 +++++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 413643a..a895222 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -152,7 +152,9 @@ on: `conftest.py`, `*.test.ts`, `*.spec.ts`); the directories `__tests__/`, `__mocks__/`, `__snapshots__/`, `testdata/` at ANY depth; and `test/`, `tests/`, `testing/`, `e2e/` only at the REPO - ROOT or under a `src/` directory (Maven/Gradle). The depth limit is + ROOT or under a `src/` directory (Maven/Gradle; `it/` there also needs a + child segment, so `src/it/java/...` is excluded but `src/it/x.properties` + counts). The depth limit is deliberate — those four names also label deployment environments and product areas, and matching them anywhere excluded production infrastructure from a real consumer's cap. A nested diff --git a/docs/callers/pr-size.md b/docs/callers/pr-size.md index 20f5916..1c7edfc 100644 --- a/docs/callers/pr-size.md +++ b/docs/callers/pr-size.md @@ -103,11 +103,13 @@ Actions step summary in precisely the case that matters, and a 5,000-line **That green-check comment needs the bot App.** It requires `comment: true` (the default) *and* `bot_app_id` + `BOT_APP_PRIVATE_KEY` — all optional. Opt into -`exclude_tests` without the App and you get precisely the outcome the paragraph -above says is prevented: a green check whose excluded total is visible only to -someone who opens the Actions step summary. **If you set `exclude_tests`, -configure the App too** — otherwise you keep the loosening and lose the -visibility that justifies it. +`exclude_tests` without the App and you lose the sticky comment — the only +surface that puts the number in front of a reviewer unprompted. The check +annotation described below still fires (it needs no credentials), so the totals +remain reachable via the Checks tab and the run's Details link, but reaching them +is a deliberate click. **If you set `exclude_tests`, configure the App too** — +otherwise you keep the loosening and keep only the weaker half of the +visibility. **Fork and Dependabot PRs are a blind spot — weigh this before opting in.** Those runs never receive `BOT_APP_PRIVATE_KEY` (GitHub withholds secrets from them), so @@ -156,7 +158,7 @@ equally trustworthy: |---|---|---| | 1 | `__tests__/`, `__mocks__/`, `__snapshots__/`, `testdata/` | **any depth** — nothing else is ever called these, and Go nests `testdata` by design | | 2 | `test/`, `tests/`, `testing/`, `e2e/` | **repo root only** | -| 3 | the same four, plus `it/` | **directly under `src/`** — Maven/Gradle nest tests at `src/test/java` and `src/it` | +| 3 | the same four, plus `it/` | **under a `src/` directory at any depth** — `module-a/src/test/java` works too. `it/` additionally requires a child segment, so `src/it/java/FooIT.java` is excluded but `src/it/messages.properties` **counts** (`it` is also the ISO-639-1 code for Italian) | The root restriction in case 2 is not fussiness, it is a bug fix. A consumer keeps production deployment manifests — cluster RBAC and ingress config — under diff --git a/scripts/check-pr-size/main.go b/scripts/check-pr-size/main.go index 12dd857..85bed71 100644 --- a/scripts/check-pr-size/main.go +++ b/scripts/check-pr-size/main.go @@ -183,8 +183,13 @@ func classify(files []FileChange, base, head string, attr attrPolicy, extras Ext // `git mv internal/big.go dist/big.go` books the removed production // lines into Generated — which takes precedence over Test in Evaluate // and has no "largest excluded" list, so it is even less auditable. + // EVERY exclusion path needs the both-paths rule, not just the two fixed + // so far: a rename's deletions are booked against the destination, so + // `git mv src/big.go go.sum` would otherwise charge removed production + // lines to the lockfile bucket. Three doors into the same bypass. + lockfile := IsLockfile(f.Path) && (f.OldPath == "" || IsLockfile(f.OldPath)) extraGen := extras.Generated(f.Path) && (f.OldPath == "" || extras.Generated(f.OldPath)) - if IsLockfile(f.Path) || extraGen || + if lockfile || extraGen || attrGen[f.Path] || contentGenerated(f.Path, base, head) { f.Generated = true diff --git a/scripts/check-pr-size/size_test.go b/scripts/check-pr-size/size_test.go index 2e02aba..d866c6b 100644 --- a/scripts/check-pr-size/size_test.go +++ b/scripts/check-pr-size/size_test.go @@ -687,3 +687,32 @@ func TestRenameClassifiedConservatively(t *testing.T) { t.Errorf("Counted = %d, want 920 — the 900 deleted production lines must not vanish", res.Counted) } } + +// TestRenameIntoExclusionBucketsStillCounts covers every exclusion path a +// rename could hide behind. numstat books a rename's deletions against the +// DESTINATION, so moving production code into any excluded bucket would +// otherwise erase those lines from the count. +func TestRenameIntoExclusionBucketsStillCounts(t *testing.T) { + t.Parallel() + extras, err := ParseExtras("", "dist/**") + if err != nil { + t.Fatal(err) + } + files := []FileChange{ + {Path: "go.sum", OldPath: "src/big.go", Added: 0, Deleted: 900}, // into a lockfile + {Path: "dist/big.go", OldPath: "internal/big.go", Deleted: 800}, // into an extra glob + {Path: "tests/big.go", OldPath: "src/big.go", Deleted: 700}, // into a test dir + {Path: "go.sum", OldPath: "go.sum", Added: 10}, // a genuine lockfile churn + } + classify(files, "", "", attrPolicy{}, extras) + + for i, f := range files[:3] { + if f.Generated || f.Test { + t.Errorf("files[%d] %s (from %s): excluded as generated=%v test=%v — production deletions must still count", + i, f.Path, f.OldPath, f.Generated, f.Test) + } + } + if !files[3].Generated { + t.Errorf("a lockfile renamed from itself should stay generated") + } +}