[PR #142] [MERGED] feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791) #153

New issue

Closed

opened 2026-10-26 22:36:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented 2026-10-26 22:36:49 +00:00

Owner

Copy link

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/142
Author: @synap5e
Created: 8/7/2026
Status: ✅ Merged
Merged: 8/7/2026
Merged by: @synap5e

Base: main ← Head: synap5e/feat/pr-size-exclude-tests

---

📝 Commits (9)

• 09530f4 feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791)
• 0d43f0f fix(pr-size): comment when the test exclusion is what keeps a PR under the cap
• a40fca1 fix(pr-size): harden the size comment path (review round 2)
• 0247fbf fix(pr-size): align the sticky-comment lookup with find_sticky (review round 3)
• 1658157 fix(pr-size): scope the comment filter to our own bot (review round 4)
• a2bf001 feat(pr-size): annotate a decisive test exclusion so forks are covered too
• 232dfcb fix(pr-size): narrow ambiguous test dirs to the repo root (BE-6791)
• 0502e63 fix(pr-size): anonymise consumer paths; land the elideMiddle fix that did not (BE-6791)
• ab2066a fix(pr-size): close the rename bypass on lockfiles too (BE-6791)

📊 Changes

7 files changed (+1428 additions, -70 deletions)

View changed files

📝 .github/workflows/pr-size.yml (+204 -16)
📝 README.md (+1 -1)
📝 docs/callers/pr-size.md (+97 -1)
📝 scripts/check-pr-size/main.go (+241 -25)
📝 scripts/check-pr-size/main_test.go (+296 -2)
📝 scripts/check-pr-size/size.go (+278 -20)
📝 scripts/check-pr-size/size_test.go (+311 -5)

📄 Description

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.

---

✏️ Correction to the Codex note above

The observation stands — re-verified since: Codex has produced no output on this PR on
any of its three surfaces (pulls/N/reviews, pulls/N/comments, issues/N/comments, all
zero).

The explanation given above does not. It attributed the absence to an exhausted
org-wide Codex quota. That was generalised from an error string on a different PR and has
since been disproved — Codex reviewed another repo PR on 2026-08-10 and returned two
correct findings. So the quota is not the cause, and the real reason Codex never ran here
is simply unknown.

Practical consequence: a @codex review on this PR may well work and is worth trying,
rather than being written off. Left undone deliberately rather than overlooked.

Recording the correction instead of editing the original away, because the wrong reason is
the more useful artifact: it is a true-at-the-time statement that a later event falsified,
sitting in a durable file — the same failure mode this PR's review history repeatedly
turned up.

---

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/142 **Author:** [@synap5e](https://github.com/synap5e) **Created:** 8/7/2026 **Status:** ✅ Merged **Merged:** 8/7/2026 **Merged by:** [@synap5e](https://github.com/synap5e) **Base:** `main` ← **Head:** `synap5e/feat/pr-size-exclude-tests` --- ### 📝 Commits (9) - [`09530f4`](https://github.com/Comfy-Org/github-workflows/commit/09530f435c973411e837b056bd2b27912a8e7a32) feat(pr-size): opt-in exclude_tests so the cap measures production code (BE-6791) - [`0d43f0f`](https://github.com/Comfy-Org/github-workflows/commit/0d43f0fb77d4fff51e000887655cdff03ac067d7) fix(pr-size): comment when the test exclusion is what keeps a PR under the cap - [`a40fca1`](https://github.com/Comfy-Org/github-workflows/commit/a40fca154fb9e210e594808195e463c2433fbc6d) fix(pr-size): harden the size comment path (review round 2) - [`0247fbf`](https://github.com/Comfy-Org/github-workflows/commit/0247fbff17181c837bb44a4108c06b0a1f74031b) fix(pr-size): align the sticky-comment lookup with find_sticky (review round 3) - [`1658157`](https://github.com/Comfy-Org/github-workflows/commit/165815751c4bdf93fa4074aa08b766bbb82928e9) fix(pr-size): scope the comment filter to our own bot (review round 4) - [`a2bf001`](https://github.com/Comfy-Org/github-workflows/commit/a2bf0017f354eb378a6a2c8adbb432ba0bea67b3) feat(pr-size): annotate a decisive test exclusion so forks are covered too - [`232dfcb`](https://github.com/Comfy-Org/github-workflows/commit/232dfcb1e5aa74d361747e5f3dbd42e30ff82566) fix(pr-size): narrow ambiguous test dirs to the repo root (BE-6791) - [`0502e63`](https://github.com/Comfy-Org/github-workflows/commit/0502e63870ff301ab95a1ac525b112ac05d46a41) fix(pr-size): anonymise consumer paths; land the elideMiddle fix that did not (BE-6791) - [`ab2066a`](https://github.com/Comfy-Org/github-workflows/commit/ab2066adbacb132d67b094546b4629a2b6293dc8) fix(pr-size): close the rename bypass on lockfiles too (BE-6791) ### 📊 Changes **7 files changed** (+1428 additions, -70 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-size.yml` (+204 -16) 📝 `README.md` (+1 -1) 📝 `docs/callers/pr-size.md` (+97 -1) 📝 `scripts/check-pr-size/main.go` (+241 -25) 📝 `scripts/check-pr-size/main_test.go` (+296 -2) 📝 `scripts/check-pr-size/size.go` (+278 -20) 📝 `scripts/check-pr-size/size_test.go` (+311 -5) </details> ### 📄 Description ## 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](https://claude.com/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`](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. --- ### ✏️ Correction to the Codex note above The **observation** stands — re-verified since: Codex has produced no output on this PR on any of its three surfaces (`pulls/N/reviews`, `pulls/N/comments`, `issues/N/comments`, all zero). The **explanation** given above does not. It attributed the absence to an exhausted org-wide Codex quota. That was generalised from an error string on a different PR and has since been disproved — Codex reviewed another repo PR on 2026-08-10 and returned two correct findings. So the quota is not the cause, and the real reason Codex never ran here is simply unknown. Practical consequence: a `@codex review` on this PR **may well work** and is worth trying, rather than being written off. Left undone deliberately rather than overlooked. Recording the correction instead of editing the original away, because the wrong reason is the more useful artifact: it is a true-at-the-time statement that a later event falsified, sitting in a durable file — the same failure mode this PR's review history repeatedly turned up. --- _{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

bsakaguchi 2026-10-26 22:36:49 +00:00

• closed this issue
• added the
  pull-request
  label

bsakaguchi referenced this issue 2026-10-26 22:36:53 +00:00

[PR #151] [MERGED] ci(bump-callers): switch every bump-* entrypoint to the shared preflight.sh (BE-6476) #160

bsakaguchi referenced this issue 2026-10-26 22:36:55 +00:00

[PR #154] [MERGED] ci(bump-callers): exclude check-pr-size *_test.go from the pr-size and cursor-review fleets (BE-7084) #161

bsakaguchi referenced this issue 2026-10-26 22:36:55 +00:00

[PR #153] [MERGED] ci(bump-callers): multi-asset WATCHED_ASSETS; watch scripts/check-pr-size in the cursor-review fleet (BE-7045) #162

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

fix/cursor-review-post-retry-5xx

ci/bump-cursor-review

ci/refresh-coderabbit-schema

ci/bump-cursor-cli-pin

dependabot/github_actions/github-actions-43d9b7ee8a

dependabot/pip/dot-github/coderabbit-config/rpds-py-2026.9.1

codex/cursor-review-fields

codex/cursor-review-history-shape

codex/cursor-label-shape

codex/cursor-immediate-handoff

codex/cursor-preserve-rounds

codex/cursor-count-superseded

codex/cursor-three-review

codex/cursor-fallback-finding-identity

codex/readiness-human-identity

codex/readiness-reviewer-permissions

codex/readiness-approval-incomplete

codex/cursor-review-evaluation

codex/readiness-check-failures

codex/readiness-shadow-report

codex/readiness-review-inventory

codex/readiness-from-risk

codex/pr-risk-report-snapshot

codex/pr-risk-report-only

codex/risk-report-base-revision

fix/cursor-review-account-outage-guard

groom/github-workflows-whole-repo-github-cursor-review-39021195499-0

matt/direct-lane-error-visibility

recovery/worktrees/20261008T012922Z-github-workflows-pr178-dirty-3c1b68f78aaf

matt/cursor-review-auto-approve

fix/detect-unreviewed-human-approvals

anupreet-comfy/be-17523-runs-on-input

christian-byrne/cifix-16923-groom-finder-bounds

christian-byrne/fix-linear-ticket-post-merge

chore/comfy-cli-codeowners-team-allowlist

matt/be-4405-groom-extra-denied-paths

cx/risk-dispute-command

cx/risk-label-map-variable

cx/named-risk-tiers

luke/area-label-bump-doc

matt/be-5508-pr-risk-shadow

matt/be-4005-bump-cursor-review-auto-label-callers

agent/cursor-review-xl

matt/be-3160-judge-parse-hardening

matt/auto-label-runs-on

matt/cursor-review-self-hostable

matt/be-3046-cursor-review-callers-var

matt/reviewers-team-slug-expansion

matt/cursor-review-judge-reformat-retry

matt/fix-bump-trailing-newline

matt/bump-kimi-k2.7-code

matt/be-1916-harden-judge-parse

matt/be-1891-cursor-review-blocking

matt/bump-callers-workflow-dispatch

matt/fix-bump-callers-yaml-parse

matt/be-1812-story-11-cursor-review-reusable-workflow-optional-dedicated

fix/cursor-review-fork-pr-403

ci/auto-bump-cursor-review-callers

chore/cursor-review-opus-4-8

matt/auto-label-open-ready

chore/ignore-pycache-cursor-review

docs/cursor-review-readme

feat/cursor-review-auto-label-reusable

chore/cursor-review-deroster

v1

Labels

Clear labels   

cursor-review-catalog-drift

  

groom

  

groom

  

pull-request

Mirrored from GitHub Pull Request

No labels

cursor-review-catalog-drift

groom

groom

pull-request

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#153

No description provided.