diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml
index 98657e7..a895222 100644
--- a/.github/workflows/pr-size.yml
+++ b/.github/workflows/pr-size.yml
@@ -13,6 +13,28 @@ 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, 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 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
+# 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. 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
@@ -24,9 +46,27 @@ 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
-# `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.
+# 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. 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.
+#
+# 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
@@ -104,6 +144,28 @@ 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: 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; `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
+ `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
+ 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 +206,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:
@@ -211,15 +274,47 @@ 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 }}
+ # 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 ${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 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
@@ -232,6 +327,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
@@ -304,6 +400,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
@@ -311,20 +412,107 @@ 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
- # 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.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.
+ # 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
+ # 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.
+ #
+ # 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 ".[] | select(.body | contains(\"${MARKER}\")) | .id")"
- existing="$(printf '%s' "$existing" | head -n1)"
+ | 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
- # 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
+ # 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).
+ #
+ # 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.
+ 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
+ fi
+ done
+ # 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
- 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/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..1c7edfc 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,100 @@ 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, 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
+"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 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
+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.** 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
+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
+opts out of this visibility guarantee.
+
+**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/` | **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
+`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 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
+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.
+
**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..85bed71 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 (
@@ -38,6 +40,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 +76,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 +142,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
@@ -157,10 +163,33 @@ 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.
+ //
+ // 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
}
- 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.
+ // 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 lockfile || extraGen ||
attrGen[f.Path] ||
contentGenerated(f.Path, base, head) {
f.Generated = true
@@ -301,15 +330,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 {
@@ -318,39 +361,176 @@ 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() {
+ // "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 != "" {
fmt.Fprintf(&b, "\n> %s\n", res.Note)
}
- // Largest contributing files, for quick triage.
+ // 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()
+}
+
+// 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.
+// 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.
+ out = '\''
+ 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 = '?'
+ }
+ b.WriteRune(out)
+ }
+ 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) {
+ // 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
+
+ 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
+// (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 {
+ for _, f := range files {
+ if !keep(f) {
continue
}
if shown == 0 {
- top.WriteString("\nLargest 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", sanitizePath(f.Path), f.Added, f.Deleted)
shown++
if shown >= 10 {
break
}
}
- if shown > 0 {
- top.WriteString("\n 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
+// 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)
+ }
+ }
+ // 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)
+ }
+ })
+
+ 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("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 non-generated lines (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")
+ if strings.Contains(got, "are tests") {
+ t.Errorf("a PR with no test changes should not get a test bullet:\n%s", got)
+ }
+ })
+}
+
+// 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"},
+ {
+ // 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", "abc.go", "a?b?c.go"},
+ {"zero-width joiner is replaced", "ab.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", "ab.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) {
+ 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("…"))
+ }
+ // 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))
+ }
+ })
+
+ // 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
+// 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
+// (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.
@@ -510,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 2b41f22..12311d1 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,13 +45,88 @@ var lockfileNames = map[string]bool{
"uv.lock": true,
}
+// 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
+// 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.
+//
+// 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
+ "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; `deploy/envs/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.
+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
+ 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
Generated bool
+ Test bool
}
// Changed returns the line count this file contributes to PR size (added +
@@ -60,13 +138,34 @@ 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
- 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
// Files sorted by descending Changed(), for reporting.
Files []FileChange
// Note is an optional human-facing explanation appended to the report (e.g.
@@ -74,6 +173,29 @@ 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.
+//
+// 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.Max && 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
@@ -94,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)
@@ -105,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)
@@ -169,6 +292,126 @@ 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 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 {
+ slash := strings.LastIndex(path, "/")
+ if slash < 0 {
+ return false // no directory part at all
+ }
+ 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 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
+}
+
+// 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.
+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:]] {
+ // 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], ".")
+ rest := parts[1:]
+ for i, p := range rest {
+ if p == "test" || (p == "spec" && i == len(rest)-1) {
+ 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 +518,37 @@ 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.
+//
+// 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.
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..d866c6b 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
@@ -185,6 +187,122 @@ 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},
+ // Type tests — the `test` component need not adjoin the extension.
+ {"web/src/api.test.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
+ {"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},
+ {"test/helpers.rb", true},
+ {"src/test/java/com/x/FooTest.java", 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},
+ {"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},
+ {"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},
+ // `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.
+ {"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 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},
+ // 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},
+ {"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 +379,10 @@ func TestEvaluate(t *testing.T) {
files []FileChange
max int
bypassed bool
+ excludeTests bool
wantCounted int
wantGenerated int
+ wantTest int
wantOK bool
}{
{
@@ -324,17 +444,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)
}
@@ -342,13 +524,80 @@ 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,
+ },
+ {
+ // 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},
+ 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{
{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 +612,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)
}
@@ -410,3 +659,60 @@ 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)
+ }
+}
+
+// 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")
+ }
+}