feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791)

[synap5e]

TL;DR

Where to look: IsTestPath in scripts/check-pr-size/size.go, and its table in
size_test.go. Everything else is plumbing.

Why: the cap counts a PR's changed lines and already excludes generated files and
lockfiles. Tests were not excluded, so a mostly-test change trips the cap on its
coverage. The only escape was oversized-ok, which says "legitimately large" — the
wrong signal for a PR whose production diff is small.

Stakes: this is a review guardrail, and the risk runs one way. A production file
misclassified as a test silently shrinks the number the cap protects; a test file left
counted merely annoys someone. So detection is deliberately conservative, the behaviour
is opt-in (exclude_tests, default false — no other repo changes), and the
excluded total is always printed.

What changed: new exclude_tests workflow input; IsTestPath path classifier +
its tests; Evaluate takes a Policy struct instead of a third positional bool;
report gains a test-lines line; README + caller doc.

---

Why opt-in rather than always-on

bump-callers.sh auto-opens SHA-bump PRs into every repo in vars.PR_SIZE_CALLERS.
A default-on change would therefore propagate a weaker guardrail fleet-wide without
a single human opting in. Per-repo knobs are also the established shape here —
extra_lockfiles, extra_generated_globs, mode: warn.

Flipping the default later is a one-line change if the org decides it wants that.

The exclusion is always visible

This is the part that makes the feature safe rather than a hole. A 5,000-line
"test-only" PR must not sail through unremarked, so the report says so either way:

- Changed lines counted (non-generated, non-test): **396**
- Cap: **1000**
- Excluded (generated/lockfiles): 814
- Excluded (tests): 1282

With the knob off, test lines are still broken out of the counted number
(Of the counted lines, 1282 are tests (exclude_tests is off)) — so the option is
discoverable from the failure that motivates it. The "largest counted files" list also
drops excluded test files, so it always adds up to the number above it.

What counts as a test

Rule	Examples
Go suffix	foo_test.go
Python	test_foo.py, foo_test.py, conftest.py
JS/TS infix before .js .jsx .mjs .cjs .ts .tsx .mts .cts	Button.test.tsx, api.spec.ts
Directory segment	test/, tests/, testing/, testdata/, e2e/, __tests__/, __mocks__/, __snapshots__/

Matching is on whole slash-delimited segments and separator-anchored suffixes, never
substrings. The false-positive half of the test table is the important half:
contest/, pkg/contest/entry.py, attestation/verify.go, internal/version/latest.go,
protest.go, manifest.ts, testify.go, sigstore_test_helpers.go all stay counted.
A file whose own name matches a test directory (cmd/test, docs/testing) is not a
test file — only directory segments are considered.

spec/ is deliberately not a test directory. In this org it holds OpenAPI schemas,
which are production artifacts. The unambiguous *.spec.ts file-name convention is
handled instead.

Honest limitation

Unlike the generated-file rules — the Go marker must precede the package clause, and
linguist-generated is read from the base ref so a PR cannot exempt itself — this
only looks at the path. Nothing stops production code being parked in tests/ to duck
the cap. That is called out in the workflow header, the caller doc and the code comment,
and it is the reason for both the opt-in and the always-on reporting.

Verification

gofmt -l . clean, go vet ./... clean, go test ./... passing (mirrors
test-pr-size.yml). agents-md-integrity still passes (its 2 warnings are pre-existing
and untouched).

Built the tool and ran it against the real diff of the internal PR that prompted this —
78% tests, currently carrying oversized-ok:

	counted	generated	test	verdict vs 1000 cap
today	1678	814	(1282, counted)	❌ needs oversized-ok
exclude_tests: true	396	814	1282	✅

An OpenAPI schema file in that diff correctly stays counted, confirming the spec/
decision. Buckets sum consistently both ways (396 + 1282 = 1678).

Follow-up

A separate PR in the consuming repo sets exclude_tests: true and bumps both the
uses: SHA and workflows_ref. No other caller is affected — this is a no-op for every
repo that does not opt in.

🤖 Generated with Claude Code

---

⚠️ The comment hijack is pre-existing on main, not introduced here

origin/main's sticky-comment lookup is select(.body | contains("<!-- ci-pr-size -->"))
— no author filter, and the marker is published in this public workflow file. That is live
today in a workflow every calling repo uses. This PR did not introduce it; round 1 made the
comment load-bearing, which is what caused the panel to look at that line at all.

Severity, stated plainly: moderate, not critical. The check conclusion is set by the
pr-size job, not by the comment, so the gate itself still holds — no code execution, no
credential exposure. What is forgeable is the human-readable explanation, plus collateral:
quoting the marker can make the shared org bot PATCH away an unrelated cursor-review or
groom comment. Review-signal integrity, author-inducible.

Worth knowing when scheduling this: the fix ships here, but the problem exists whether or
not this PR merges.

---

Review round 1 — what changed (0d43f0f)

The panel found a real hole in the central claim of this PR, plus five smaller
inconsistencies. Reasoning lives here rather than in thread replies, since this repo
has no bot identity for me to answer under.

The one that mattered: the exclusion could hide itself

The sticky comment posts only when over_cap is true. So with exclude_tests on, a PR
whose test lines bring it under the cap went green and posted nothing —
Excluded (tests): N survived only in the Actions step summary, which a reviewer has to
click into. The number was invisible in exactly the case it exists for, and
"a large test-only PR cannot pass unremarked" simply was not true as written.

Fixed with Result.ExclusionDecisive() — tests excluded, not bypassed, and
Counted + Test > Max. It surfaces as a tests_decisive output, rides the artifact, and
makes the comment job post on a green check. The report gained a matching paragraph:

Under the cap only because test lines are excluded. This PR changes 1678 lines in
total (396 counted + 1282 test), over the 1000-line cap; exclude_tests is what brings
it under.

Verified against the diff that motivated the feature: tests_decisive=true. Before this
fix, that PR would have posted nothing at all.

Also fixed

Finding	Change
Evaluate's doc claimed Counted + Generated + Test always equals the diff total	False with the knob off — Test is then a subset of Counted, so the sum double-counts. Comment corrected; the tests always asserted the right numbers.
Excluded (tests): N was unauditable	Now lists the largest excluded test files too, so the number can be checked rather than trusted. Rendering extracted into topFiles.
f.Test set after the binary guard	Moved before it, so a binary fixture under testdata/ is reported as a test file. No numeric effect (Changed() is 0 for binaries) — but classify's own doc said "always".
Directory matching was case-sensitive	Now case-insensitive, so Tests/, TestData/, E2E/ (.NET/C#/Unity) are recognized — previously those repos got no exclusion at all. File-name rules stay case-sensitive: _test.go and conftest.py are lowercase by toolchain definition, so other casings would only add false positives.
"largest counted files … always adds up"	It stops at 10. Wording corrected.

Not taken

Anchoring test-directory matching to the repo root. A production package named
testing/ (the client-go/testing idiom) does get excluded, and that is a genuinely
different failure from parking code in tests/ — no author intent required. Rejected
anyway: that segment is load-bearing for real consumers (one has ~147 non-*_test.go
helpers under testing/), and anchoring to the root would break them. The opt-in default
plus the now-mandatory reporting is the intended mitigation, and a repo whose testing/
holds production code should not set exclude_tests.

---

Review round 2 — hardening the comment path (a40fca1)

Round 1 made the sticky comment the only signal for a green test-heavy PR. Round 2
pointed out, reasonably, that this raised the stakes on a path that had never been
treated as security-sensitive. Three findings there, all taken.

Comment hijack

The sticky-comment lookup matched any comment containing <!-- ci-pr-size -->, 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 their comment
instead of posting its own, after which they can rewrite it freely. That suppresses or
falsifies the verdict this PR makes load-bearing. The lookup now also matches the bot
app's own id (compared as a string, so a non-numeric input can't make jq abort).

Report injection

f.Path went into markdown unescaped, and git diff --numstat -z emits paths
verbatim — ParseNumstat's own comment notes they 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. Newlines
reaching stdout can also emit :: workflow commands. Added sanitizePath.

The test for this is structural rather than textual — markdown only sees a heading at the
start of a line, so the invariant is that a hostile path cannot create a new line.
The forged text surviving inside a code span on its own line is harmless and expected.

Comment-size DoS

Two 10-entry lists with unbounded path lengths 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 fixed

Finding	Change
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 "total"	It omits Generated — understating a diff that also regenerated a lockfile, in the one sentence whose job is showing the real size. Now "non-generated lines".
tests_excluded output	Was written from res.Test unconditionally, asserting an exclusion that never happened under the default policy. Emits 0 there now.
Result.Counted / Test field comments	Round 1 fixed Evaluate's doc but left these — correctly re-raised.
The green-check comment silently requires the bot App	Documented: 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 the guarantee.

Not taken

A concurrency: group on the reusable, to close the check-then-act race in the
comment upsert. The race is real and pre-existing. But cancel-in-progress on a
required check is exactly how a PR ends up BLOCKED behind a cancelled run that looks
like a defect — which happened on this very PR during review. That is a worse and more
confusing failure than a rare duplicate comment, and the fix deserves its own change
rather than riding along here.

---

Review round 3 — following the pattern that already existed (0247fbf)

Round 3's most useful contribution was pointing at
find_sticky in scripts/pr-risk/publish-risk-surfaces.sh.
This repo had already solved the sticky-comment problem once, with a comment block naming
the exact hazards. Round 2 invented a second dialect instead of following it, and got the
details wrong. Now aligned.

A real bug in round 2's own hardening

sanitizePath built its string rune-by-rune and then truncated with a byte slice, so
a cut landing inside a multi-byte rune emitted invalid UTF-8 — which GitHub can 422 on,
and continue-on-error converts into the silent no-comment degradation the bound exists
to prevent. Caught by 7 of 8 reviewers. My test couldn't have caught it: it was pure
ASCII. The replacement sweeps multi-byte runes at every length and asserts
utf8.ValidString.

The comment-lookup filter had three faults in one line

Fault	Consequence	Fix
Keyed on .performed_via_github_app.id	create-github-app-token also accepts a Client ID (Iv1.…); a caller wiring that posts fine but never matches — stacking a new comment every push, never flipping to ✅	Filter on .user.type == "Bot", which is what actually defeats the pre-seed (an author's comment is type User) without depending on the id's form
contains, not startswith	The app id is the org's shared bot — it posts cursor-review and groom comments too. A bot comment quoting the marker matched, and the size report would PATCH that review away. An author can induce this by quoting the marker in the PR body	The body renders the marker as its first line, so startswith is exact
BOT_APP_ID spliced into the jq program text	A quote or backslash makes the filter a syntax error → non-zero gh → set -euo pipefail → silent skip. My comment claiming this "cannot make jq abort" was wrong — tostring guards the JSON side, not the interpolated literal	Marker passed as jq data via --arg; nothing user-controlled reaches the program

Verified against fixtures: an attacker's pre-seeded comment and a bot quoting the marker
are both excluded; only genuine marker-first bot comments match; a null body doesn't
crash it.

I reversed a round 2 rejection

I rejected the duplicate-comment race in round 2 because the proposed fix — a
concurrency: group — risks stranding PRs behind cancelled required checks. Round 3
offered an option I hadn't considered: PATCH every marker match instead of head -n1.
That closes the same race with no concurrency group at all, so a stale duplicate can no
longer sit displaying a wrong number. Taken.

Also

• sanitizePath replaces unicode.Cf too — a bidi override renders a path as a filename
  other than the one on disk and can visually reorder the adjacent counts, undermining the
  excluded-files list whose purpose is verification.
• GITHUB_OUTPUT write/close errors are surfaced: a partial write could lose
  tests_decisive while exiting 0, and the comment job reads absent as false.
• JS/TS matching tests each dot-separated stem component rather than only the one adjoining
  the extension, so foo.test.d.ts matches the advertised *.test.*. Requiring a
  non-first component still keeps 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.

---

Review round 4 — final (1658157)

Round 4 caught two defects round 3 introduced, both from over-correcting. Review
stops here: the remaining item is recorded below as known-and-deferred rather than
starting a fifth round.

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 — the filter then 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, from
create-github-app-token's app-slug output. That identifies the installation whether
bot_app_id is a numeric App ID or a Client ID, so the reason round 3 widened 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 the expected case: the loop only has multiple ids
because duplicates happen. Failures are now tolerated per id and reported.

A safe-direction fix that introduced an unsafe one

Round 3's stem-component widening fixed an over-count (foo.test.d.ts) by introducing an
under-count: api.spec.types.ts and openapi.spec.client.ts classified as test. That
collided head-on with the reasoning three comments above it in the same file — spec is
kept out of testPathSegments precisely because it names OpenAPI production artifacts
here. test now matches in any non-first component; spec only as the final stem
component.

Also

Finding	Change
sanitizePath missed C1 controls and Zl/Zp	U+0085 and U+2028 render as line breaks — the same escape the function blocks — and U+009B opens ANSI sequences in the public log. Now Cc || Cf || Zl || Zp.
Truncation discarded the classifying tail	<150 bytes>/tests/prod.go rendered as a production-looking path in the excluded list, defeating its purpose. Now elides the middle.
report() still dropped step-summary errors	And this PR newly makes that summary the sole carrier on fork/Dependabot PRs.
strings.ToLower folds U+212A KELVIN SIGN to k	__MOC⟨U+212A⟩S__ folded to __mocks__. Now ASCII-only folding.
Docs claimed the list allows "auditing", and advertised *.spec.*	Both overstated the code after the narrowing above. Hedged and corrected.

Known and deferred: the comment creation race

Two near-simultaneous runs can both find zero matches and both POST; PATCH-every-match
heals duplicates only on a later run. Round 4 is right that cancel-in-progress: false
would close it — my earlier objection was to cancel-in-progress: **true** stranding
required checks, and it does not survive their version.

Deferred anyway: the race is pre-existing, and adding a concurrency: group to a
reusable workflow changes queueing semantics for every consumer of it, not just this
caller. That wants its own change and its own review rather than riding along in a PR
about test exclusion. Recorded here so it is not lost.

---

Human review — the fork/Dependabot gap (a2bf001)

@wei-hai identified a gap the docs described honestly but the code did not close, and
it is the sharpest framing of it so far: on fork and Dependabot PRs the loosening
applies but 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. The result 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.

Their observation that ExclusionDecisive is already computed in the read-only job is
what makes the fix cheap: the information was on the trusted side of the split the whole
time, with nowhere visible to go.

Taken: their first option. The size job now emits a ::warning annotation when the
exclusion is decisive:

::warning title=Under the cap only via the test exclusion::This PR changes 1678
non-generated lines (396 counted + 1282 test) against a 1000-line cap. …

An annotation needs no credentials and no added permissions, so it reaches every PR
including forks, and 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.

Not taken: fail-closed (their second option — fail the check when tests_decisive and
no comment can post). With the annotation in place the invariant holds without it, and
failing legitimate outside contributions to enforce a reporting property seemed the
wrong trade. Happy to revisit if the annotation proves too quiet in practice.

Docs corrected, not just extended. The workflow header and caller doc both still said
the total lives only in the step summary on those PRs — true before this commit, false
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.

On their check of the sibling pattern: publish-risk-surfaces.sh is the origin of
the find_sticky shape rather than a consumer of it, and it already carries all three
filters — .user.type == "Bot", a login allow-list (allowed_logins, defaulting to
github-actions[bot] plus a configurable STICKY_LOGINS), and startswith($m). It is
this PR that had been taking two of the three; nothing to backport.

---

Review coverage — a gap worth stating

Codex has produced no output on this PR, on any of the three surfaces it can post to
(pulls/N/reviews, pulls/N/comments, issues/N/comments — all zero). So this PR has
no Codex coverage.

The cause is account-level and not something this PR can resolve: the Codex code-review
quota is currently exhausted across Comfy-Org repos, and firing the trigger would return
a refusal that is invisible to CI (there is no Codex check context) while burning a
scarce shared resource. That has been escalated separately.

Recording it rather than papering over it, because "no Codex output" is indistinguishable
from "Codex reviewed and found nothing" — its clean result is a 👍 with no Found N
count. Absence here is genuinely ambiguous, and a reader should not read silence as a
pass.

What this PR does have: four Cursor panel rounds (35 findings, all dispositioned) plus
a human review from @wei-hai, and a fifth panel round on the current head. Those are
documented above.

---

Review round 5 — the one that mattered (232dfcb)

The any-depth directory rule excluded production code

Not hypothetically. A consumer keeps its ArgoCD manifests — cluster RBAC,
clusterrolebinding, 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.

Directory matching is now three cases, not one:

Case	Segments	Where
1	__tests__/, __mocks__/, __snapshots__/, testdata/	any depth
2	test/, tests/, testing/, e2e/	repo root only
3	those four plus it/	directly under src/ (Maven/Gradle)

Case 3 is not decoration: without it, Maven repos would count their entire 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 fixtures. All 227 files under its root-level
testing/ (e2e, integration, smoke, synthetics) stay excluded; all 31 under
infrastructure/argocd/apps/testing/ now count; zero misclassified either way. End to
end on a synthetic diff — 1000 lines of clusterrole.yaml + 500 of testing/smoke + 40
of service code — gives counted=1040, over_cap=true. The same diff counted 40 before.

This reverses my round-3 rejection. The panel proposed root-anchoring; I rejected it,
asserting testing/ was load-bearing for the consumer so anchoring would break it. I
never checked where those files lived. They're at the root. The premise was false and the
finding was right — and the rejection read as considered for two further rounds.

Deliberate cost: a nested ambiguous directory like services/checkout/e2e/ now
counts. Safe direction — over-counting starts an argument, under-counting silently
shrinks the number the cap protects.

Also fixed

Finding	Change
The PATCH loop "tolerated and reported" — only tolerate had landed	It sent gh's stderr to /dev/null and kept no status, so the step exited 0 even if every patch failed and the degraded-mode note never fired. Now keeps an rc, surfaces stderr, exits non-zero.
Annotation printed the raw max_lines input	envInt falls back to 1000 for anything Atoi rejects, so it could name a cap never enforced — and on fork PRs it's the only surface. Tool now emits the applied cap.
A rename's deletions were booked to its destination	git mv src/big.go tests/big.go charged removed production lines to an excluded test path. A rename now counts as test only if both paths do.
elideMiddle returned a 3-byte ellipsis when asked for a smaller bound	Breaks its own contract; returns "" now.
The annotation claim was overstated	An annotation with no file/line reaches the run summary, Details link and Checks tab — but not the PR conversation or Files changed. On a fork PR the number is findable, not unprompted. Narrowed in the workflow header and the caller doc rather than left as-is.

Not taken

Widening the sticky-comment login match to an allow-list. Real — an App rename or a
bot_app_id repoint orphans existing stickies — but it needs a new caller input, and the
failure is visible (a duplicate comment) rather than silent, unlike the cases this PR
closes.

---

⛔ Round 7 — findings acknowledged and deliberately not fixed. Human review needed before more iteration.

Review stops here by decision, not because it converged. Round 7 found 10 findings
(claimed 10 / 0 inline — body-only; caught by the count check, not by the panel
volunteering it). They are recorded below unfixed, so nothing depends on a chat log.

The High (7 of 8 reviewers) — a fourth door, after I certified there were three

0502e63's comment says "EVERY exclusion path needs the both-paths rule … Three doors
into the same bypass." There are four checks in that if. attrGen[f.Path] is still
keyed on the destination alone — attrGeneratedBatch is only ever fed files[i].Path, so
the source's attribute is never resolved.

In a repo whose base .gitattributes marks generated output the standard way
(*.pb.go linguist-generated), git mv internal/big.go internal/big.pb.go plus a
900-line deletion books those removed production lines into Generated — which outranks
Test and has no "largest excluded" list. The attr.trusted gate does not help, because
the attack never touches .gitattributes. And TestRenameIntoExclusionBucketsStillCounts
— which I named for covering every exclusion path — omits this one.

I closed this bypass three times, asserted the class was closed, and missed the fourth
instance inside the same if. That is fixing what I was shown and then certifying the
class.

The rest, unfixed

Sev	Finding
🟡	contentGenerated reads the head tree, so pasting a DO NOT EDIT marker above a package clause moves a hand-written file into Generated. This contradicts a sentence in this PR's own caller doc claiming things are read from the base ref so a PR cannot exempt itself — only the attribute half is base-gated.
🟡	TouchesGitattributes checks only Path, so git mv .gitattributes .gitattributes.bak leaves the fail-closed guard bypassed.
🟡	When every sticky 404s, patched is 0 → the step exits 1 and the POST (an elif) never recreates the comment: no comment at all, while the note blames a 403 that did not occur.
🟡	An empty app-slug makes BOT_LOGIN the literal [bot], which matches nothing — every push then stacks a fresh sticky forever, with no error.
🟢	The workflow header still describes the pre-0502e63 rule. The input description and the doc table were corrected in that same commit; this third copy was not.
🟢	"Vendored trees are usually excluded as generated anyway" — false in this tool; nothing consults linguist-vendored.
🟢	writeGitHubOutputs warns on a partial write but still exits 0, so the failure its own comment describes still happens.
🟢	it/ accepting any child re-opens most of the locale problem (src/it/LC_MESSAGES/, src/it/pages/).
⚪	Off-by-one: max == 3 drops the ellipsis unnecessarily.

Why stopping rather than fixing

Rounds 5, 6 and 7 each found defects in the previous round's fixes. Round 7's High is
the clearest case: a completeness claim I wrote was false about the very if statement it
described. That is a signal about fix quality, not about remaining design risk, and the
answer to it is human review rather than another machine round.

Suggested direction, held loosely: the both-paths rule wants to be applied once,
where classification is decided, rather than repeated at four call sites where a fifth can
be added later and silently miss it. That is a design change, and round 7 is direct
evidence against me making it unsupervised.

---

⚠️ Post-merge record: the rename bypass is live on main, and predates this PR

This PR merged as 46a96b9 carrying the ten round-7 findings above unfixed, including
the 🟠 High. Recording the measured facts here so they are discoverable from the change
that introduced the discussion, rather than only from a chat log.

The bug predates this PR — and this PR made it three-quarters better

main attributes a rename's deletions to the destination path, so moving production
code into any excluded bucket erases those lines from the counted total. Executed against
main's own unmodified sources (extracted to a scratch module, no edits), simulating
git mv src/big.go go.sum with 900 lines removed, as numstat -z emits it:

main parsed the rename as: Path="go.sum" (source discarded)
classified Generated=true  changed=900
=> counted=0  generated=900

All 900 removed production lines fall out of the cap.

Scope, stated conservatively: demonstrated via IsLockfile because that needs no git
fixture. The realistic variant runs through the linguist-generated attribute path — a
repo with *.pb.go linguist-generated in its base .gitattributes, plus
git mv internal/big.go internal/big.pb.go. That is the same mechanism but was not
separately executed. Independent of exclude_tests either way.

Direction of change, because the framing matters:

	both-paths guards
main before this merge	0
main after this merge	3

Three of the four doors are now closed that were all open before; the fourth (attrGen,
still keyed on the destination alone) was already open. This merge did not ship a
regression — it shipped an incomplete fix to a pre-existing hole.

ParseNumstat on pre-merge main already parsed the rename's source token and discarded
it (size.go:108, with a comment naming the path it drops), so the information the fix
needs is already in the parser.

Suggested direction

Apply the both-paths rule once, where classification is decided, rather than at four
call sites where a fifth can be added and silently miss it. Four point fixes and one
false completeness claim is what the repeated-call-site shape produced here.

Also triggered by this merge

bump-pr-size-callers fired 4 seconds after the squash and has opened SHA-bump PRs into
the caller fleet. The opt-in default held as designed — those bumps move the pins only and
do not set exclude_tests — so no repo's cap loosened without a human choosing it.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR adds path-based test classification and optional test-line exclusion to the PR-size checker. It updates evaluation, reports, GitHub outputs, workflow comments, documentation, and tests.

Changes

PR-size test exclusion

Layer / File(s)	Summary
Test classification and evaluation contracts scripts/check-pr-size/size.go, scripts/check-pr-size/size_test.go	The tool classifies test paths independently of policy. Policy, FileChange, and Result track test status, counted lines, excluded lines, rename paths, and decisive exclusions.
Evaluation and report output scripts/check-pr-size/main.go, scripts/check-pr-size/main_test.go	The command passes test exclusion settings to evaluation. Reports show test totals, exclusion status, decisive passes, filtered file lists, and sanitized paths. GitHub outputs include tests_excluded and tests_decisive.
Workflow wiring and documentation .github/workflows/pr-size.yml, README.md, docs/callers/pr-size.md	The workflow adds exclude_tests, maps it to PR_SIZE_EXCLUDE_TESTS, propagates tests_decisive, updates sticky-comment matching and failure handling, and documents detection and reporting behavior.

Sequence Diagram(s)

sequenceDiagram
  participant PRSizeWorkflow
  participant CheckPRSizeMain
  participant SizeEvaluate
  participant CommentJob
  PRSizeWorkflow->>CheckPRSizeMain: pass PR_SIZE_EXCLUDE_TESTS
  CheckPRSizeMain->>SizeEvaluate: pass Policy
  SizeEvaluate->>CheckPRSizeMain: return counted and test totals
  CheckPRSizeMain->>PRSizeWorkflow: write tests_decisive
  PRSizeWorkflow->>CommentJob: provide over-cap and tests_decisive flags
  CommentJob->>CommentJob: find and update matching App bot comments

Loading

Possibly related PRs

• Comfy-Org/github-workflows#80: Shares the pr-size.yml workflow and caller documentation.

Suggested reviewers: mattmillerai

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch synap5e/feat/pr-size-exclude-tests

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch synap5e/feat/pr-size-exclude-tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Found 7 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	2
⚪ Nit	4

Panel: 8/8 reviewers contributed findings.

[github-actions]

🟡 Medium — The header's promise that a large test-only PR "cannot pass unremarked" rests on the sticky comment, but that comment is only created when over_cap is true — exactly when the exclusion brings a PR under the cap, no comment is posted and Excluded (tests): N survives only in the step summary a reviewer has to click into the Actions run to see. writeGitHubOutputs emits no test/excluded count either, so nothing machine-readable carries it. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

[synap5e]

Confirmed and fixed in 0d43f0f. Added Result.ExclusionDecisive() → tests_decisive output → comment job now posts on a green check. Verified end to end: tests_decisive=true on the motivating diff, which previously posted nothing. Detail in the PR description (Review round 1).

[github-actions]

🟢 Low — The new invariant is false under the default policy: with ExcludeTests off the case f.Test branch adds a file's lines to both res.Test and res.Counted, so Counted + Generated + Test overshoots the diff's non-binary total by exactly the test total (the new test expects Counted=700 and Test=600 on a 700-line diff). Qualify the sentence to the opted-in case, and refresh Result.Counted's field comment, which now also means non-test. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case).

[synap5e]

Correct — fixed in 0d43f0f. The invariant holds only with ExcludeTests set; without it Test is a subset of Counted. Comment corrected.

[github-actions]

🟢 Low — hasTestSegment matches a segment at any depth, so with exclude_tests: true a production package named testing/, test/ or e2e/ — a real Go/Python idiom (client-go/testing, django/test/) — silently drops out of the counted total with no author intent at all, which is a different failure from the parking-code-in-tests/ case the docs already acknowledge. Consider anchoring the riskier segments to a known root prefix, or naming this false-positive cost in the caller docs. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).

[synap5e]

Not taken. Rationale in the PR description (Review round 1, "Not taken") — the testing/ segment is load-bearing for real consumers, so root-anchoring would break them.

[github-actions]

⚪ Nit — f.Test is assigned after the if f.Binary { continue } guard, so binary paths never get it set — contradicting this function's own doc claim that Test is set "always, regardless of policy". There is no numeric impact today because Changed() returns 0 for binaries, but a binary fixture under testdata/ or __snapshots__/ is still reported as non-test in Result.Files. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0d43f0f — f.Test now set before the binary guard.

[github-actions]

⚪ Nit — The comment says filtering to contributing files makes the list "always add up to the number above it", but the loop still stops after 10 entries, so any PR touching more than 10 counted files lists strictly less than Counted. Say it shows the largest contributors rather than that it sums. Raised by 1 of 8 reviewers (kimi-k3-max edge-case).

[synap5e]

Fixed in 0d43f0f — wording no longer claims the list sums to Counted.

[github-actions]

⚪ Nit — With exclude_tests on, excluded files are dropped from the largest-counted list and named nowhere else in the report, so Excluded (tests): N cannot be audited from the report alone — a reviewer sees a large excluded number with no way to check whether those files are genuinely tests. Listing the top excluded test files would make the number verifiable. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

[synap5e]

Fixed in 0d43f0f — the report now lists the largest excluded test files.

[github-actions]

⚪ Nit — Segment matching is case-sensitive, so Tests/, TestData/ and E2E/ (the normal casing in .NET/C#/Unity trees) are not recognized. The failure direction is safe — lines are over-counted, cap too strict — but a repo that opts in with that layout gets no exclusion at all and nothing in the report explains why. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0d43f0f — directory segments now match case-insensitively. File-name rules stay case-sensitive by design.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 2 — ledger: 7 prior finding(s) across 1 round(s) (0 never answered).

Found 10 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	6
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

---

scripts/check-pr-size/size.go:155 — 🟡 Medium — ExclusionDecisive omits the "under the cap" half of its own contract: it never checks r.Counted <= r.Max, so on a run that is over cap even after the exclusion (e.g. Counted=1500, Test=500, Max=1000) r.Counted+r.Test > r.Max is trivially true and tests_decisive=true is emitted on a RED run. Harmless today only because renderReport gates on res.OK and the comment job evaluates $OVER first in its ||; add an r.Counted <= r.Max (or r.OK) term so the predicate and the machine-readable output stand on their own. Raised by 6 of 8 reviewers (gemini-3.1-pro edge-case, gemini-3.1-pro adversarial, kimi-k3-max adversarial, kimi-k3-max edge-case, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

.github/workflows/pr-size.yml:361 — 🟡 Medium — The sticky-comment lookup selects ANY comment whose body contains <!-- ci-pr-size --> with no author filter, and that marker is published in this public workflow file. A PR author can pre-seed a comment containing the marker, after which the bot PATCHes that user-owned comment (which the author can then rewrite) instead of creating its own — suppressing or falsifying the green-check comment this diff makes the sole signal for a test-heavy PR. Filter the --jq select on the app's login or .performed_via_github_app. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).

scripts/check-pr-size/main.go:382 — 🟡 Medium — f.Path is interpolated into markdown with no escaping, and git diff --numstat -z deliberately emits paths verbatim — main.go's own parsing comment notes a path may contain spaces and newlines. A PR author can name a file so the code span closes and forged lines are injected into a bot-authored comment (a second ## ✅ Passed heading, a fake Excluded (tests): 0, or another <!-- ci-pr-size --> marker), and newlines in a path can also emit :: workflow commands to stdout. Strip control characters and escape backticks before printing. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).

scripts/check-pr-size/main.go:424 — 🟢 Low — tests_excluded is written from res.Test unconditionally, but with exclude_tests off those lines are included in counted (the case f.Test branch in Evaluate), so the output name asserts an exclusion that did not happen on every default-policy run. Nothing consumes it today; emit 0 when !res.TestsExcluded, or rename it to test_lines. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max edge-case, kimi-k3-max adversarial, kimi-k3-max edge-case).

scripts/check-pr-size/main.go:347 — 🟢 Low — The decisive paragraph reports res.Counted+res.Test as "changes %d lines in total", but that sum omits res.Generated. On a PR with a regenerated lockfile or codegen output the stated "total" understates the real diff by thousands of lines — in the one sentence whose purpose is making the real size visible. Say "non-generated lines" or include res.Generated in the figure. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k3-max adversarial).

docs/callers/pr-size.md:98 — 🟢 Low — The new guarantee that "the sticky comment posts even though the check is green" holds only when comment is true AND bot_app_id + BOT_APP_PRIVATE_KEY are configured — all optional, bot_app_id defaulting to ''. A repo that opts into exclude_tests without a bot App gets exactly the outcome this paragraph says is prevented: a green check with the excluded total living solely in the Actions step summary. Qualify the claim, or state that exclude_tests effectively requires the App. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

scripts/check-pr-size/main.go:362 — 🟢 Low — The report now carries two 10-entry file lists with each path printed at unbounded length, so ~20 deeply nested paths can push the body past GitHub's 65,536-character comment limit. The POST/PATCH then 422s and, because the upsert step is continue-on-error, the run degrades silently to "no comment posted" — the exact unremarked-pass outcome this diff's decisive-green comment exists to prevent. Truncate per path and bound the total body. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

.github/workflows/pr-size.yml:368 — 🟢 Low — The lookup-then-POST upsert is a check-then-act with no lock, and this reusable declares no concurrency: group (the documented caller pattern has none either), so two runs from rapid pushes can both find no marker and both POST; head -n1 then pins one comment forever while the duplicate keeps displaying a stale verdict. Extending the POST to green checks widens the window from over-cap PRs to every test-heavy PR. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).

scripts/check-pr-size/size.go:430 — 🟢 Low — The documented extra_generated_globs fallback for "a layout these conventions miss" puts those files in Generated, never Test, so ExclusionDecisive stays false and a large custom-layout test PR gets no green sticky comment — the fallback silently opts out of the visibility guarantee the rest of this diff builds. Worth saying so in the caller docs where the fallback is recommended. Raised by 1 of 8 reviewers (gpt-5.6-sol-max adversarial).

scripts/check-pr-size/size.go:126 — ⚪ Nit — Round 1's reply on the sum invariant said "Comment corrected", and Evaluate's doc was indeed fixed — but Result.Counted's own field comment, which the same finding also asked to refresh, still reads "changed lines from non-generated, non-binary files" and omits that test lines are out of it under ExcludeTests. The adjacent Test comment ("changed lines in test files") is likewise imprecise: a generated test file lands in Generated and is not tallied here. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

↩︎ re-raise of #142 (comment) (round 1)

(Inline comments could not be anchored to the diff; listed above instead.)

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[synap5e]

Round 2 dispositioned in a40fca1 — 9 of 10 fixed, 1 rejected.

Taken: the comment-hijack (unauthored marker match), report injection via unescaped paths, comment-size 422, ExclusionDecisive missing its Counted <= Max term, the "total" figure omitting generated lines, tests_excluded under the default policy, the re-raised field comments, and both documentation gaps (the green-check comment requires the bot App; extra_generated_globs opts out of the guarantee).

Not taken: a concurrency: group on the reusable. Rationale in the PR description under "Review round 2 → Not taken".

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 3 — ledger: 7 prior finding(s) across 1 round(s) (0 never answered).

Found 9 finding(s).

Severity	Count
🟡 Medium	2
🟢 Low	6
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🟡 Medium — The new filter keys on .performed_via_github_app.id, but the pinned create-github-app-token also accepts a Client ID (Iv1.…/Iv23…) for app-id, and many orgs store that in the variable wired to bot_app_id; the comparison is against the numeric App ID, so such a caller mints tokens and posts fine yet never matches. The failure is silent — existing is empty on every run, so a new comment stacks on each push and a previously-flagged PR never flips to ✅. Consider matching .user.login/.user.type == "Bot" as a fallback (the sibling find_sticky in scripts/pr-risk/publish-risk-surfaces.sh does exactly this), or failing loudly when the id is non-numeric. Raised by 3 of 8 reviewers (kimi-k3-max adversarial, claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).

[synap5e]

Fixed in 0247fbf — now filters on .user.type == "Bot" rather than the app id, so a Client ID caller is unaffected. That is also what actually defeats the pre-seed, since an author's comment is type User.

[github-actions]

🟡 Medium — The marker test is still contains, not startswith, and the app id belongs to the org's shared bot (cursor-review, groom, assign-reviewers all post under it), so the filter means "any comment by our bot app mentioning the marker anywhere" — including a cursor-review comment that quotes PR content containing <!-- ci-pr-size -->. That comment then gets PATCHed away by the size report, which is both a lost review and an author-controlled way to destroy bot output. Since body.md writes the marker as its first line, startswith("$MARKER") is a drop-in fix and is what find_sticky in publish-risk-surfaces.sh already uses. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, gemini-3.1-pro adversarial).

[synap5e]

Fixed in 0247fbf — startswith now, and aligned with find_sticky in scripts/pr-risk/publish-risk-surfaces.sh as you suggested. Verified against fixtures that a bot quoting the marker is excluded.

[synap5e]

Additional context, since it changes how this should be scheduled: this is pre-existing on main, not introduced by this PR. origin/main's lookup is select(.body | contains("<!-- ci-pr-size -->")) — no author filter, contains not startswith — so it is live today in a workflow every calling repo uses. Round 1 made the comment load-bearing, which is what caused the panel to look at that line at all.

Severity as I read it: moderate, not critical. The check conclusion comes from the pr-size job, not the comment, so the gate still holds — no code execution, no credential exposure. Forgeable is the human-readable explanation, plus the collateral you identified (quoting the marker can make the shared bot PATCH away an unrelated review comment). Review-signal integrity, author-inducible.

Noted in the PR description too.

[github-actions]

🟢 Low — BOT_APP_ID is spliced into the jq program as raw source text rather than passed as data, so the value is parsed by jq: a double quote or backslash makes the filter a syntax error, gh exits non-zero, set -euo pipefail kills the step, and continue-on-error turns that into a silent "no comment posted". The adjacent comment's claim that a non-numeric input "cannot make jq abort" only holds for the tostring on the JSON side, not for the interpolated literal — pass it with --jq plus --arg-style data binding or validate that the id is numeric first. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0247fbf — marker passed as jq data via --arg; BOT_APP_ID no longer reaches the program text at all. My comment claiming it "cannot make jq abort" was wrong, as you say: tostring guarded the JSON side, not the interpolated literal.

[github-actions]

🟢 Low — The find-then-POST/PATCH is check-then-act with no concurrency group in the reusable or the documented caller, which subscribes to labeled/unlabeled alongside synchronize — a push plus a label toggle fires two near-simultaneous runs that both see no existing comment and both POST. Later runs always PATCH the head -n1 pick, so the duplicate is permanently orphaned displaying a stale verdict, and this PR widens the blast radius because the comment is now the only signal on a decisive-green PR. A concurrency: pr-size-${{ github.event.pull_request.number }} group (or PATCHing every marker-matching app comment) closes it. Raised by 4 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k3-max adversarial, claude-opus-5-thinking-max adversarial).

[synap5e]

Taken in 0247fbf, reversing my round 2 rejection. I rejected the concurrency: group because cancel-in-progress on a required check strands PRs behind cancelled runs. Your alternative — PATCH every marker match rather than head -n1 — closes the same race without one.

[github-actions]

🟢 Low — sanitizePath builds the string rune-by-rune but truncates with a byte slice, so a path whose 160-byte cut point lands inside a multi-byte rune emits invalid UTF-8 into the comment body and step summary (rendered as U+FFFD, and GitHub can 422 on it — which continue-on-error turns into the silent no-comment degradation maxPathDisplay exists to prevent). The over-long paths are bounded subtest is pure ASCII so it cannot catch this; bound the loop by accumulated byte length instead, and add a multi-byte case to the test. Raised by 7 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max edge-case, kimi-k3-max adversarial, kimi-k3-max edge-case).

[synap5e]

Fixed in 0247fbf — real bug, thank you. Truncation now accumulates bytes and cuts only at a rune boundary. You were right that the subtest could not catch it (pure ASCII); the replacement sweeps multi-byte runes at every length and asserts utf8.ValidString.

[github-actions]

🟢 Low — The control-character filter covers only C0 and DEL, so Unicode format characters pass through: a path with a bidi override (U+202A–U+202E, U+2066–U+2069) or zero-width joiners renders in the bot comment as a filename other than the one on disk, and can visually reorder the adjacent (+N/-M) counts. That directly undercuts the "Largest excluded test files" list, whose stated purpose is letting a reviewer verify the excluded lines really are tests. Extend the replacement to unicode.Cf (and the U+2066–U+2069 isolates). Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).

[synap5e]

Fixed in 0247fbf — sanitizePath now replaces unicode.Cf, covering the bidi overrides and isolates. Test cases added for override, isolate and ZWJ.

[github-actions]

🟢 Low — Fork and Dependabot pull_request runs do not receive BOT_APP_PRIVATE_KEY, so the decisive-exclusion comment cannot post there even for a caller that configured the App correctly — an outside-contributor PR can pass green via exclude_tests with the warning confined to the step summary. The docs' "configure the App too" guidance does not cover this case; worth naming it in docs/callers/pr-size.md alongside the existing App caveat. Raised by 1 of 8 reviewers (gpt-5.6-sol-max adversarial).

[synap5e]

Documented in 0247fbf, in the workflow header alongside the existing App caveat.

[github-actions]

🟢 Low — fmt.Fprintf to the GITHUB_OUTPUT file and the deferred f.Close() both discard their errors, so a partial write (disk full, truncated line) can drop tests_decisive while the process still exits 0. The comment job reads the missing flag as false and silently skips the promised green-check comment — the exact failure mode this PR exists to close. Check both errors and warn to stderr. Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).

[synap5e]

Fixed in 0247fbf — both errors surfaced to stderr.

[github-actions]

⚪ Nit — The .test./.spec. infix only matches when it sits immediately before the final extension, so foo.test.d.ts and Vitest/tsd's foo.test-d.ts type-test files are counted as production code even with exclude_tests: true, despite matching the *.test.* convention docs/callers/pr-size.md advertises. The direction is safe (over-counting), but an opted-in repo gets capped on these files with nothing in the report explaining why. Raised by 1 of 8 reviewers (kimi-k3-max edge-case).

[synap5e]

Fixed in 0247fbf — matching now tests each dot-separated stem component instead of only the one adjoining the extension, so foo.test.d.ts matches. Requiring a non-first component keeps spec.ts counted. Did not take -d.ts, which is not *.test.*.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 4 — ledger: 16 prior finding(s) across 2 round(s) (0 never answered).

Found 9 finding(s).

Severity	Count
🟡 Medium	2
🟢 Low	5
⚪ Nit	2

Panel: 8/8 reviewers contributed findings.

[github-actions]

🟡 Medium — The prior reply justified .user.type == "Bot" on the grounds that "an author's comment is type User" — that defeats the human pre-seed but drops bot identity entirely, so the filter now means "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 comment's first line gets claimed as ours and PATCHed away, and while such a comment exists the elif POST branch never runs — so the decisive-green comment is suppressed. The sibling find_sticky in scripts/pr-risk/publish-risk-surfaces.sh that this block cites as its model carries a third filter for exactly this (select(($allow | index($c.user.login)) != null)); matching .user.login against the minted token's own login (gh api user --jq .login → <slug>[bot]) identifies the installation regardless of numeric-vs-Client app id, so the Client-ID rationale does not require widening to all bots. Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max edge-case, kimi-k3-max adversarial).

[synap5e]

Fixed in 1658157 — you are right that I over-corrected. Round 3 dropped bot identity entirely to dodge the Client-ID problem, when find_sticky's third filter was the answer all along. Now matches .user.login against the minted token's own login via create-github-app-token's app-slug output, which identifies the installation for numeric App IDs and Client IDs alike — so the reason I widened no longer applies. .user.type kept as a cheap second condition.

[github-actions]

🟡 Medium — The new PATCH-every-match loop runs under set -euo pipefail, so the first failing gh api -X PATCH aborts the step mid-iteration and the remaining ids keep a stale verdict — the exact split-brain the loop was added to prevent. A 404 is the expected case here (the loop exists because duplicates happen, and a maintainer or another run deleting one between the list and the write is a real TOCTOU), and continue-on-error: true turns the abort into a silent "no comment posted" with nothing saying which copies were updated. Tolerate per-id failures (|| { echo "..." >&2; rc=1; }) and report at the end instead of aborting. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case, gpt-5.6-sol-max adversarial).

[synap5e]

Fixed in 1658157 — per-id failures are tolerated and reported instead of aborting. Your framing that 404 is the EXPECTED case is what makes this obvious: the loop only has multiple ids because duplicates happen, so a concurrent delete is normal, not exceptional.

[github-actions]

🟢 Low — Widening the stem scan from the component adjoining the extension to any non-first component fixes foo.test.d.ts (an over-count, the direction size_test.go's header calls safe) by introducing an under-count: api.spec.types.ts, openapi.spec.client.ts and payments.spec.gen.ts now classify as test and silently leave Counted under exclude_tests. That collides head-on with the reasoning two comments above in this same file — spec/specs are excluded as directory segments precisely because "in this org those hold API schemas (OpenAPI), which are production artifacts." Restricting the multi-component scan to test (keeping .spec. only where it adjoins the extension), or to the last/second-to-last stem component, keeps the type-test fix without the OpenAPI collision. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max adversarial).

[synap5e]

Fixed in 1658157 — and this is the one I am most glad you caught. I fixed an over-count (safe direction) by introducing an under-count (unsafe direction), contradicting the reasoning three comments above in the same file. test now matches in any non-first component, keeping the foo.test.d.ts fix; spec only as the final stem component, so api.spec.types.ts and openapi.spec.client.ts stay counted.

[github-actions]

🟢 Low — The filter now covers C0, DEL and unicode.Cf, but C1 controls (U+0080–U+009F, which are unicode.Cc and not caught by r < 0x20) and the Unicode line/paragraph separators U+2028/U+2029 still pass through. U+0085 (NEL) and U+2028 introduce line breaks in the rendered markdown — the same list-escape this function exists to block — and U+009B (CSI) can inject ANSI sequences into the public run log despite ESC being filtered. Replace on unicode.Is(unicode.Cc, r) || unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Zl, r) || unicode.Is(unicode.Zp, r). Raised by 3 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max edge-case).

[synap5e]

Fixed in 1658157 — now replaces Cc || Cf || Zl || Zp. Cc covers C1, so U+0085 and U+009B are handled along with U+2028/U+2029. Test cases added for each.

[github-actions]

🟢 Low — The reply accepted PATCH-every-match as closing the race "without" a concurrency group, but that only heals duplicates on a LATER run — the creation side is unchanged: two near-simultaneous runs both see zero matches and both POST. When no further PR event fires before merge (typical for a final push racing an oversized-ok label toggle, which the documented caller subscribes to), the PR permanently shows two contradictory stickies — ❌ over-cap beside ✅ bypassed — and nothing re-heals them. The stranded-required-check objection to concurrency: applies to cancel-in-progress: true; cancel-in-progress: false queues instead of cancelling and closes the creation side. Ordering is the same gap: an older run's PATCH can land after a newer one's, leaving a stale verdict that only a head-SHA guard in the body would prevent. Raised by 3 of 8 reviewers (kimi-k3-max adversarial, kimi-k3-max edge-case, gpt-5.6-sol-max adversarial).

↩︎ re-raise of #142 (comment) (round 3)

[synap5e]

Not taken here, deliberately — and you are right on the substance, including that cancel-in-progress: false sidesteps my stranded-required-check objection. That objection was to cancel-in-progress: true specifically, so it does not survive your version.

Deferring anyway: the creation race is pre-existing, and adding a concurrency: group to a REUSABLE workflow changes queueing semantics for every consumer of it, not just this caller. That deserves its own change and its own review rather than riding along in a PR about test exclusion. Recorded as known-and-deferred in the PR description with your reasoning, so it is not lost.

PATCH-every-match still fixes the healing side, which is what makes a stale duplicate stop showing a wrong number on subsequent runs.

[github-actions]

🟢 Low — Round 3's error-surfacing landed in writeGitHubOutputs but not in its sibling report() just above, which still discards both the os.OpenFile error and the fmt.Fprintln result on GITHUB_STEP_SUMMARY. This PR newly makes that summary the documented sole carrier of the excluded-test total on fork and Dependabot PRs (workflow header lines 50–53), so a failed summary write silently removes the only surface those PRs have, with no stderr trace. Mirror the same two error checks there. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 1658157 — report() now mirrors the same two checks. Your point about this PR newly making the step summary the sole carrier on fork/Dependabot PRs is what makes it more than symmetry.

[github-actions]

🟢 Low — sanitizePath keeps only the leading 160 bytes, so for a long path the evidence of test classification can fall in the discarded tail: <160 bytes of benign prefix>/tests/prod.go renders as a truncated production-looking path in the "Largest excluded test files" list, defeating the auditability that list exists for, and distinct paths sharing a prefix render identically. Preserve the tail (elide the middle) or render which classifier matched alongside the path. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

[synap5e]

Fixed in 1658157 — elideMiddle now drops the middle and keeps the tail, so …/tests/prod.go still shows why it was classified. Test asserts the classifying suffix survives.

[github-actions]

⚪ Nit — strings.ToLower applies Unicode simple case folding, not the ASCII folding the adjacent comment describes for .NET/Unity casing: U+212A KELVIN SIGN lowercases to k, so a directory named __MOC\u212AS__ folds to __mocks__ and everything under it drops out of the count with exclude_tests on. Folding only ASCII (or rejecting non-ASCII segments before comparing) keeps the intended casing tolerance without the widening. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 1658157 — asciiLower folds A-Z only. The casing tolerance exists for .NET/Unity trees, which is an ASCII concern, so Unicode folding was pure surface.

[github-actions]

⚪ Nit — "names the largest excluded files so the number can be audited" overstates what topFiles produces — it stops after 10 entries, so a PR touching more than 10 excluded test files shows a list that cannot account for Excluded (tests): N. Round 1 forced the same correction on the counted list (now worded "the biggest files rather than accounting for every counted line"); give this sentence the same hedge, since "audited" implies a completeness the list does not provide. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 1658157 — hedged to "the biggest contributors, not a complete accounting (it stops at 10)". Also corrected the same file's *.spec.* claim, which now overstated the rule after the spec narrowing above.

[wei-hai]

Review: well-designed feature, and the safety reasoning is right. One gap I'd want closed before it ships.

The framing is what makes this reviewable: the risk runs one way, so detection is conservative, the behavior is opt-in, and the excluded total is always printed. That's the correct set of three, and each one is doing real work rather than being a hedge.

Opt-in specifically because the bump fleet auto-propagates changes into every roster repo is the right reason. A default-on version would weaken a review guardrail across the org without a single human deciding to weaken it — and the people who'd notice are the ones who'd never see the PR.

Making the exclusion always visible is what turns this from a hole into a feature. Printing the test breakdown even when the knob is off (Of the counted lines, 1282 are tests (exclude_tests is off)) is a nice touch — the option becomes discoverable from the failure that motivates it, rather than from docs nobody reads at that moment. Dropping excluded files from the "largest counted files" list so it adds up to the number above it is the kind of detail that stops people distrusting the report.

ExclusionDecisive forcing a comment on a green check is the single most important part of the design. Without it the number lives only in the step summary in exactly the case it exists for, and a 5,000-line test-only PR passes unremarked.

---

The gap: on fork and Dependabot PRs, the loosening applies but the compensating visibility does not.

The workflow header is honest about this:

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.

So for a repo with exclude_tests: true, PRs from forks get the weaker cap and lose the mechanism that makes the weaker cap safe. That inverts the trust gradient — the least-trusted contributions get the least-scrutinized guardrail — and it's silent, because a green check with no comment looks identical to a PR that passed on its own merits.

ExclusionDecisive is already computed in the job that has the read-only token, so the information exists on the trusted side of the split; it just has nowhere visible to go without the App. Some options, roughly in order of how much I'd like them:

• Put the excluded-test total into the check run's output/summary or title, which is visible without bot credentials. That preserves the invariant on every PR regardless of secret availability.
• Fail the check when tests_decisive is true and no comment can be posted — fail-closed, so the loosening never applies invisibly.
• At minimum, document in the caller doc that exclude_tests on a repo taking fork contributions has this blind spot, next to the existing "configure both together" advice.

The header already says a repo that sets exclude_tests without the App "keeps the loosening and loses the visibility," which is the right diagnosis — I'd just rather the code enforced it than the comment warned about it, since the failure is invisible by construction.

---

On the classifier itself — the conservatism is well judged.

Whole-segment, slash-delimited matching so contest/, attestation/ and latest.go are untouched is the right call, and worth having as an explicit comment since substring matching here would be a silent under-count of production code.

Excluding spec/specs because they hold OpenAPI schemas in this org, while still honoring the unambiguous *.spec.ts file-name convention, is exactly the distinction I'd want — those really are production artifacts and would have been a meaningful hole.

Case-insensitive on directory segments (so .NET/Unity Tests/, TestData/ are caught) but case-sensitive on file-name rules (because _test.go and conftest.py are lowercase by toolchain convention, so a capitalized variant isn't the convention) is a distinction most implementations would get wrong in one direction or the other. Good.

Generated taking precedence over Test in the tally, documented on the field, avoids the double-count question entirely.

On the sticky-comment author filter: matching the bot's own login rather than .user.type == "Bot" is a real security fix, not hardening. The marker is published in a public workflow file, so without an author filter a PR author can pre-seed a comment carrying it and get the workflow to PATCH theirs — and since exclude_tests makes that comment the only signal on a green test-heavy PR, that's a hole straight through the guarantee this feature rests on. Deliberately mirroring the existing find_sticky shape rather than inventing a second dialect is the right instinct; worth confirming the other consumer of that pattern got the login fix too, since the same hazard applies there.

Minor: Policy as a struct rather than a third positional bool — agreed, and the reason you gave (booleans are silently swappable at a call site) is the real one.

[synap5e]

Gap closed in a2bf001, using your first option.

The size job now emits a ::warning annotation when tests_decisive is true:

::warning title=Under the cap only via the test exclusion::This PR changes 1678
non-generated lines (396 counted + 1282 test) against a 1000-line cap. …

No credentials and no added permissions, so it reaches fork and Dependabot PRs and carries the same figures as the comment.

Not taken: fail-closed. With the annotation the invariant holds without failing outside contributions to enforce a reporting property. Reasoning in the PR description under "Human review — the fork/Dependabot gap", along with why the sticky comment is still worth having where it can post.

Also corrected the workflow header and caller doc, which both still said the total lives only in the step summary on those PRs — true before a2bf001, false after.

On your check of the sibling: scripts/pr-risk/publish-risk-surfaces.sh is the origin of the find_sticky shape rather than a consumer, and already has all three filters — .user.type == "Bot" (line 391), the allowed_logins allow-list (lines 354-357, 377, 394), and startswith($m) (line 395). This PR was the one taking two of three. Nothing to backport.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/callers/pr-size.md (2)

43-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a valid commit SHA in the caller example.

@<full-commit-sha> and workflows_ref: <same-full-commit-sha> are placeholders, so the example cannot be copied and executed. Replace both values with the same merged-main commit SHA and update them together when upgrading the workflow.

As per coding guidelines, docs/callers/*.md must provide a complete, copy-pasteable caller for each reusable workflow.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/callers/pr-size.md` around lines 43 - 45, Replace the placeholder values
in the caller example with one valid merged-main commit SHA, using the identical
SHA for both the workflow reference and workflows_ref input. Ensure the example
remains copy-pasteable and both references are updated together when upgrading
the workflow.

Source: Coding guidelines

---

99-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the warning annotation as the no-credential fallback.

The workflow emits a warning annotation whenever tests_decisive is true, even when BOT_APP_PRIVATE_KEY is unavailable. Therefore, the excluded total is not limited to the Actions step summary. Keep the App requirement for the richer sticky comment, but describe the annotation as the fallback. No secret, still a signal.

Suggested wording change

-Without the App, the excluded total is visible only in the Actions step summary.
+Without the App, the workflow still emits a warning annotation with the excluded total, but it cannot post the sticky comment.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/callers/pr-size.md` around lines 99 - 110, Update the documentation
around the exclude_tests App configuration to state that when tests_decisive is
true and BOT_APP_PRIVATE_KEY is unavailable, the workflow still emits a warning
annotation showing the excluded total. Keep the requirement for bot_app_id and
BOT_APP_PRIVATE_KEY specifically for the richer sticky comment, and describe the
annotation as the no-credential fallback.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@docs/callers/pr-size.md`:
- Around line 43-45: Replace the placeholder values in the caller example with
one valid merged-main commit SHA, using the identical SHA for both the workflow
reference and workflows_ref input. Ensure the example remains copy-pasteable and
both references are updated together when upgrading the workflow.
- Around line 99-110: Update the documentation around the exclude_tests App
configuration to state that when tests_decisive is true and BOT_APP_PRIVATE_KEY
is unavailable, the workflow still emits a warning annotation showing the
excluded total. Keep the requirement for bot_app_id and BOT_APP_PRIVATE_KEY
specifically for the richer sticky comment, and describe the annotation as the
no-credential fallback.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 29b9c704-655f-415b-90e2-f24aa52be03b

📥 Commits

Reviewing files that changed from the base of the PR and between 1658157 and a2bf001.

📒 Files selected for processing (2)

• .github/workflows/pr-size.yml
• docs/callers/pr-size.md

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 5 — ledger: 18 prior finding(s) across 2 round(s) (0 never answered).

Found 7 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	3
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🟡 Medium — Round 4's reply said per-id failures are now "tolerated and reported", but only the tolerate half landed: the loop sends gh's stderr to /dev/null and accumulates no failure status, so the step exits 0 even when EVERY PATCH fails (a 403 from permission drift — the one this file documents as proven live on Jul-24 — a 422 on an over-long body, or a 429/5xx). steps.upsert.outcome therefore stays success, the Note degraded mode step keyed on == 'failure' never fires, and the PR keeps a stale verdict on a green run; the log also misattributes every cause to "deleted concurrently?" because the real error was discarded. Keep an rc and exit non-zero after the loop (continue-on-error already prevents a red check) and let gh's stderr through. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k3-max edge-case).

↩︎ re-raise of #142 (comment) (round 4)

[synap5e]

Fixed in 232dfcb — correct, and my round-4 reply overclaimed. Only the tolerate half had landed: stderr went to /dev/null and no status accumulated, so the step exited 0 even if every patch failed. Now keeps an rc, lets gh's stderr through, and exits non-zero so the degraded-mode note fires.

[github-actions]

🟡 Medium — The annotation prints the raw PR_SIZE_MAX_LINES input rather than the cap the tool actually applied (res.Max), and envInt silently falls back to defaultMaxLines (1000) for any value strconv.Atoi rejects — a max_lines that arrives as a decimal (GitHub expressions yield 1250.5 for 2501/2) or with stray whitespace from a forwarded var. The annotation then names a cap that was never enforced, disagreeing with the report's Cap: line, and on fork and Dependabot PRs the annotation is the ONLY surface, so there is no comment to cross-check it against. Emit the applied cap as a step output alongside counted and use that here. Raised by 3 of 8 reviewers (gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, gemini-3.1-pro edge-case).

[synap5e]

Fixed in 232dfcb — the tool now emits the applied cap as a max_lines output and the annotation uses that instead of the raw input.

[github-actions]

🟡 Medium — ParseNumstat keeps only a rename's destination path, so this new f.Test = IsTestPath(f.Path) classifies a rename's DELETIONS by where the file landed: git mv src/big.go tests/big.go plus edits books all the removed production lines as excluded test lines under exclude_tests: true, letting a large production refactor pass the cap. The Largest excluded test files list shows only tests/big.go, so the audit trail the list exists for cannot reveal it either. Classify a rename by the more conservative of its two paths, or count a renamed file's deletions against the source path. Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).

[synap5e]

Fixed in 232dfcb — ParseNumstat now keeps the rename source in OldPath, and a rename counts as test only if BOTH paths do. Test asserts the 900 deleted production lines still count.

[github-actions]

🟢 Low — The comment justifies this as mirroring find_sticky, but find_sticky matches an allow-LIST of logins (github-actions[bot] plus a caller-supplied STICKY_LOGINS) precisely because the posting identity is plural over time, whereas this derives exactly one login from the current mint. Any identity change — an App rename, since the slug and therefore the <slug>[bot] login follow the App name, or a caller repointing bot_app_id — makes every existing sticky invisible, so a fresh one is POSTed and the old copy is orphaned permanently displaying a stale verdict. That is the same split-brain the PATCH-every-match loop below exists to prevent, reached through a different door, with no knob to widen the match. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).

[synap5e]

Not taken. Real — an App rename or a bot_app_id repoint orphans existing stickies — but the fix needs a new caller input, and the failure is visible (a duplicate comment) rather than silent, unlike the cases this PR is closing. Recorded in the PR description rather than actioned.

[github-actions]

🟢 Low — Eliding the middle preserves the tail, but test directory segments match at ANY depth, so the segment that caused the classification can itself land in the discarded middle: <long prefix>/tests/<160-byte filename>.go renders with tests/ gone, showing a production-looking path in the "Largest excluded test files" list — the auditability that list exists for. The new truncation keeps the classifying tail subtest only covers the case where the segment is near the end. Render the matched classifier (segment or filename rule) alongside the path rather than relying on the path's shape. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

[synap5e]

Partly dissolved by 232dfcb: ambiguous segments (test, tests, testing, e2e) now match only at the repo root or under src/, so a deep <prefix>/tests/<file> no longer classifies at all. Unambiguous segments can still be nested, so the residual case stands — not fixing further for now.

[github-actions]

🟢 Low — An annotation emitted with no file=/line= renders only at the top of the run-summary page and behind the check run's Details link — it does not appear in the PR conversation (a passing check collapses into "All checks have passed") nor inline on Files changed. That is essentially the same click-through the docs call insufficient for the step summary, so "the exclusion is never invisible ... on every PR" overstates what this adds. Either attach file/line metadata so it lands on the Files changed tab, or narrow the claim here and in the matching paragraph of docs/callers/pr-size.md. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

[synap5e]

Fixed in 232dfcb by narrowing the claim rather than the code. You are right that a file/line-less annotation reaches the run summary, the Details link and the Checks tab, but not the PR conversation or Files changed. Both the workflow header and the caller doc now say the number is findable on a fork PR, not unprompted.

[github-actions]

⚪ Nit — elideMiddle breaks its own "bounds s to max bytes" contract when max <= len(ellipsis): it returns the 3-byte ellipsis, which exceeds the requested bound. Unreachable at maxPathDisplay = 160, but this is an otherwise general, independently testable helper — returning an empty string (or the largest rune-bounded prefix that fits) would make the bound hold unconditionally. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, gemini-3.1-pro edge-case).

[synap5e]

Fixed in 232dfcb — returns "" rather than an over-long ellipsis when the bound is below its length.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 6 — ledger: 25 prior finding(s) across 3 round(s) (0 never answered).

Found 8 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	4
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🟡 Medium — The exclude_tests input description: still advertises the pre-split rule — "paths under test/, tests/, testing/, testdata/, e2e/, tests/, mocks/, snapshots/" with no depth qualifier — while the implementation now matches test/tests/testing/e2e only at the repo root or directly under src/. This description is what GitHub renders for the input and the first thing a consumer reads before opting in, and both the workflow header (lines 22-28) and docs/callers/pr-size.md were rewritten for the three-case split in this same commit, so a repo with packages/foo/tests/ or services/checkout/e2e/ opts in expecting an exclusion it will not get. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max adversarial).

[synap5e]

Fixed in 0502e63 — the input description: now states all three cases and says explicitly that a nested packages/foo/tests/ counts. You are right that this string is the one a consumer actually reads before opting in.

[github-actions]

🟡 Medium — The conservative-rename guard applies only to f.Test; extras.Generated(f.Path) a few lines below still classifies by destination alone, and Generated takes precedence over Test in Evaluate. With dist/** in extra_generated_globs, git mv internal/big.go dist/big.go plus a 900-line deletion books those removed production lines into the Generated bucket — the same bypass just closed for tests, and less auditable because there is no "Largest excluded generated files" list. Extend the same (f.OldPath == "" || …) conjunction to the glob check. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0502e63 — same both-paths conjunction applied to the glob check. Your point that Generated outranks Test and has no "largest excluded" list makes it strictly worse than the case I had just closed.

[github-actions]

🟡 Medium — Case 3 requires src to be segs[0], so it only rescues single-module Maven/Gradle layouts: module-a/src/test/java/FooTest.java and services/payment/src/it/… — the standard multi-module shape, and the one Gradle's own docs use — fall through all three cases and are counted, which is exactly the outcome srcRoots' comment says it exists to prevent ("Without this case those repos would count their entire test tree"). Scan the segment list for any src immediately followed by an ambiguous segment rather than anchoring src to index 0; the vendor/src/test/java/x.java test case would need revisiting as the deliberate cost. Raised by 2 of 8 reviewers (gemini-3.1-pro adversarial, claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0502e63 — src is now found at any depth rather than anchored at index 0, so module-a/src/test/java and services/payment/src/it/java are covered. Took the vendor/src/test/... cost you flagged and documented it at the rule.

[github-actions]

🟢 Low — The new [ "$rc" -eq 0 ] || exit 1 fires on ANY per-id failure, including the 404 the comment twelve lines above calls "an expected case", so a partial run — one duplicate 404s on a concurrent delete while the real sticky PATCHes fine — makes steps.upsert.outcome failure and the "Note degraded mode" step then prints "No bot comment posted: the sticky comment upsert failed at runtime … a residual permission 403". That is affirmatively false in the case the loop was added to handle, and it dilutes the signal for the genuine 403 the note exists to catch. Track patched-vs-failed separately so the note can say "some copies failed", or exclude a 404 from rc. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k3-max edge-case).

[synap5e]

Fixed in 0502e63 — the step now fails only when NOTHING was patched, and reports partial failures separately. You are right that reporting an expected 404 as a permission 403 is affirmatively false and dilutes the genuine signal.

[github-actions]

🟢 Low — Emitting the applied cap as max_lines fixed the reporting mismatch but not the silent substitution it documents: envInt discards strconv.Atoi's error, so a max_lines arriving as 1250.5 or with stray whitespace from a forwarded vars. value becomes the 1000 default — a LOOSER cap than the caller configured when they meant 500 — with no stderr line, and Cap: 1000 in the report is indistinguishable from a repo that genuinely meant 1000. Warn to stderr when PR_SIZE_MAX_LINES is non-empty but unparseable, so a caller typo cannot quietly raise the cap. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).

[synap5e]

Fixed in 0502e63 — envInt now warns to stderr on an unparseable non-empty value. Worth stating the direction plainly: the silent substitution LOOSENED a cap the caller meant to tighten.

[github-actions]

🟢 Low — This paragraph publishes a private consumer's internal layout in a public repo: the exact path infrastructure/argocd/apps/testing/, what lives under it (cluster RBAC, ingress, cert issuers), and precise file counts (227 vs 31). The same detail is repeated in the pr-size.yml header, size.go's comment, and size_test.go fixtures like infrastructure/argocd/apps/testing/appsets/comfy-cloud-test.yaml, against AGENTS.md's rule to keep private repo paths and detail out of workflow files and docs. Anonymizing to something like deploy/envs/testing/ keeps the entire lesson. Raised by 1 of 8 reviewers (kimi-k3-max adversarial).

[synap5e]

Fixed in 0502e63, and this is a repeat offence on my part rather than an oversight — I made and scrubbed the same class of leak earlier in this branch. Anonymised to an illustrative deploy/envs/testing/ across the workflow header, size.go, the caller doc and the test fixtures, and dropped the file counts. The lesson survives because it was never about which repo it was.

[github-actions]

🟢 Low — srcRootOnlyTestSegments honours it directly under src/, but it is also the ISO-639-1 code for Italian, so a plausible src/it/ locale or content tree drops out of the count under exclude_tests — the under-counting direction this file repeatedly names as the unsafe one. Maven/failsafe always nests it as src/it/<java|resources|…>/, so requiring such a child segment (or dropping it entirely) keeps the Maven support without the collision. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

[synap5e]

Fixed in 0502e63 — it under a source root now requires a child segment, so a bare src/it/ locale tree keeps counting. Test case added.

[github-actions]

⚪ Nit — Round 5's reply says this was "Fixed in 232dfcb — returns "" rather than an over-long ellipsis when the bound is below its length", but the tree still reads if max <= len(ellipsis) { return ellipsis }, and elideMiddle is absent from the hunks new since round 5 while 232dfcb's other changes (rename OldPath, the max_lines output, the segment split) are all present — so that specific fix did not land. Impact is unchanged and unreachable at maxPathDisplay = 160; the problem is that the ledger now records it as fixed. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k3-max edge-case).

[synap5e]

You are right and this is the important one. The edit silently no-op'd, I never re-read the file, and my round-5 reply reported it as fixed — so the ledger recorded a closed finding that was still open. Actually applied in 0502e63, with a test asserting the bound holds for max in 0..3. Thank you for diffing the claim against the tree rather than trusting it.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/pr-size.yml:
- Around line 151-159: Update the test-directory documentation at
.github/workflows/pr-size.yml lines 151-159 and docs/callers/pr-size.md lines
155-159 to state that it/ is recognized only when it has a child path, such as
src/it/java/..., while src/it/messages.properties or src/it/<locale-file> is
counted; ensure the wording does not imply every path beneath src/it/ is
excluded.

In `@docs/callers/pr-size.md`:
- Around line 122-133: Update the earlier no-App and fork guidance to
distinguish the unavailable sticky PR comment from the still-available
credential-free warning annotation for decisive exclusions. Document that the
annotation exposes totals in the run summary, Checks, and Details views, but is
not shown in the PR conversation or inline on Files changed.

In `@scripts/check-pr-size/main.go`:
- Around line 181-187: Update the exclusion condition in the file classification
logic around extraGen so IsLockfile also validates both paths for renames: a
file is lockfile-excluded only when the destination and, when present, the old
path are both lockfiles. Add a regression test covering a production-file rename
to a lockfile and verify its deletion lines are not excluded as generated.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e6907dcb-f94d-4f08-8761-851b6da68014

📥 Commits

Reviewing files that changed from the base of the PR and between a2bf001 and 0502e63.

📒 Files selected for processing (6)

• .github/workflows/pr-size.yml
• docs/callers/pr-size.md
• scripts/check-pr-size/main.go
• scripts/check-pr-size/main_test.go
• scripts/check-pr-size/size.go
• scripts/check-pr-size/size_test.go

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @synap5e.

Round 7 — ledger: 24 prior finding(s) across 3 round(s) (0 never answered).

Found 10 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	4
🟢 Low	4
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

---

scripts/check-pr-size/main.go:193 — 🟠 High — The new comment asserts "EVERY exclusion path needs the both-paths rule ... Three doors into the same bypass", but there are four checks in this if and attrGen[f.Path] is still keyed on the destination alone — attrGeneratedBatch is only ever fed files[i].Path, so the source path's attribute is never resolved. In a repo whose base-ref .gitattributes marks generated output (*.pb.go linguist-generated, vendor/** linguist-generated — the standard pattern this code path exists to honour, and the attr.trusted gate does not help because the attack never touches .gitattributes), git mv internal/big.go internal/big.pb.go plus a 900-line deletion books those removed production lines into Generated, which outranks Test in Evaluate and has no "largest excluded" list. Collect f.OldPath into the batch and apply the same (f.OldPath == "" || attrGen[f.OldPath]) conjunction; TestRenameIntoExclusionBucketsStillCounts claims to cover "every exclusion path" but omits this one because it needs a git repo. Raised by 7 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case).

scripts/check-pr-size/main.go:194 — 🟡 Medium — contentGenerated(f.Path, base, head) reads the PR HEAD tree, so this rule is head-controlled: adding // Code generated by x. DO NOT EDIT. above the package clause of a hand-written .go file moves all of its changed lines into Generated, and a rename into a .go destination carrying that line does the same for the source's deletions (no OldPath conjunction here either). That contradicts the paragraph this PR adds to docs/callers/pr-size.md — "read .gitattributes from the base ref precisely so a PR cannot exempt itself" — since only the attribute half is base-gated, and the package-clause requirement stops mid-file pasting, not top-of-file pasting. Honour the marker only when the BASE blob also carries it (a genuinely new generated file then over-counts, the safe direction). Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max edge-case).

scripts/check-pr-size/size.go:513 — 🟡 Medium — ParseNumstat now populates OldPath, but TouchesGitattributes still inspects only Path, so git mv .gitattributes .gitattributes.bak (or renaming it into place) leaves attrModified false and attr.trusted true — the fail-closed policy that whole guard exists for is bypassed while attribute-excluded files are edited in the same PR. Check both paths, the same conjunction just applied to the test/lockfile/glob doors. Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).

.github/workflows/pr-size.yml:502 — 🟡 Medium — When every listed sticky 404s — the concurrent delete the comment 25 lines above calls "an expected case" — patched is 0, so this branch exits 1 and, because the POST lives in an elif, never falls through to recreate the comment. On a decisive-green or over-cap PR that leaves no comment at all while the degraded-mode note blames "a residual permission 403", which is affirmatively false. An all-404 outcome means "gone" and is recoverable by posting fresh, unlike a 403/422: capture gh api's status (or --silent + -i) and re-POST when every failure was a 404. Separately, the partial-failure elif only echoes to stdout, so a duplicate left showing a stale verdict has no signal outside the step log — emit ::warning there. Raised by 4 of 8 reviewers (gemini-3.1-pro edge-case, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial).

.github/workflows/pr-size.yml:407 — 🟡 Medium — If steps.bot.outputs.app-slug is ever empty (the pinned create-github-app-token SHA predating the output, or a rename), BOT_LOGIN becomes the literal [bot], which matches no real .user.login. existing is then empty on every run, so the POST branch fires each push and stacks a fresh sticky comment forever — silently defeating the entire find-then-PATCH design with no error anywhere. Guard it: [ "$BOT_LOGIN" = "[bot]" ] && { echo "::error::app-slug missing"; exit 1; } turns silent degradation into a loud misconfiguration. Raised by 1 of 8 reviewers (kimi-k3-max edge-case).

scripts/check-pr-size/main.go:574 — 🟢 Low — This comment states the hazard exactly — "a partial write can lose tests_decisive while the process still exits 0 ... silently skips the green-check comment" — but writing the error to stderr does not change the exit code, so the failure it names still happens. writeGitHubOutputs should return the error so main can exit non-zero rather than letting the comment job proceed on an absent flag. Raised by 1 of 8 reviewers (gemini-3.1-pro edge-case).

.github/workflows/pr-size.yml:25 — 🟢 Low — This header's three-case summary still describes the pre-0502e63 rule: it says the four ambiguous segments plus it/ match "directly under src/", whereas hasTestSegment now scans for a src segment at ANY depth (so module-a/src/test/java is excluded), and it omits that it/ requires a child segment (so src/it/x.properties counts). The input description: at lines 148-160 and case 3 of the table in docs/callers/pr-size.md were both corrected for exactly these two points in this same commit; this block, the canonical in-file description, was not. Raised by 2 of 8 reviewers (kimi-k3-max adversarial, claude-opus-5-thinking-max edge-case).

scripts/check-pr-size/size.go:337 — 🟢 Low — The stated mitigation for un-anchoring src — "vendored trees are usually excluded as generated anyway" — does not hold in this tool: lockfileNames covers no vendored path, extra_generated_globs is opt-in, and the attribute path reads only linguist-generated (GitHub's vendoring convention is linguist-vendored, never consulted here). So any src/test, src/tests, src/testing, src/e2e inside a committed node_modules, third_party or vendored SDK drops silently out of the count — the under-count direction this file repeatedly names as unsafe, and the deleted vendor/src/test/java/x.java case was the test pinning it. Requiring a recognized language child segment (src/test/<java|kotlin|scala|resources>), as case 3 already does for it, keeps the Maven/Gradle layouts without the widening. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).

scripts/check-pr-size/size.go:349 — 🟢 Low — The it child-segment requirement was added so a bare src/it/ Italian locale tree keeps counting, but accepting ANY child re-opens most of it: src/it/LC_MESSAGES/messages.po and src/it/pages/index.md are production locale content and are still classified as test. Maven failsafe always nests src/it/<java|kotlin|resources|scala>, so requiring one of those recognized source-set children keeps the integration-test support without swallowing locale trees. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).

scripts/check-pr-size/main.go:457 — ⚪ Nit — The bound is now honoured, but the boundary is off by one: at max == len(ellipsis) == 3 the ellipsis fits exactly, yet max <= len(ellipsis) returns "" and drops the truncation indicator. Use max < len(ellipsis); TestElideMiddleHonoursTinyBounds passes either way since it only asserts len(got) <= max. Raised by 2 of 8 reviewers (gemini-3.1-pro edge-case, gpt-5.6-sol-max edge-case).

(Inline comments could not be anchored to the diff; listed above instead.)

[synap5e]

Round 7 acknowledged: 10 findings, deliberately not fixed, recorded in the PR description under "Round 7".

Stopping review here by decision rather than convergence. Rounds 5, 6 and 7 each found defects in the previous round's fixes, and round 7's High is a completeness claim I wrote that was false about the same if statement it described — there is a fourth exclusion path (attrGen, keyed on the destination alone) after I asserted there were three.

That is a signal about fix quality rather than remaining design risk, so this wants human review before more machine iteration. The findings are recorded so nothing depends on a chat log.

Not merging.