[PR #108] [MERGED] feat(groom): raise the pr_size_limit default from 400 to 600 lines (BE-6161) #116

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:36:25 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/108
Author: @mattmillerai
Created: 8/2/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6161-groom-pr-size-limit-600

📝 Commits (3)

3ef62a0 feat(groom): raise the pr_size_limit default from 400 to 600 lines (BE-6161)
5755028 docs(groom): correct two overclaims in the pr_size_limit rationale (BE-6161)
f07fd00 Merge remote-tracking branch 'origin/main' into matt/be-6161-groom-pr-size-limit-600

📊 Changes

1 file changed (+29 additions, -3 deletions)

View changed files

📝 .github/workflows/groom.yml (+29 -3)

📄 Description

ELI-5

Groom's auto-builder refuses to open a PR whose patch changes more than pr_size_limit lines — it files a groom issue instead so nobody has to review a giant machine-written diff. That ceiling was 400. This raises the shared default to 600, in the reusable workflow, so it changes once under review instead of 28 times across callers.

Why 600, and why not 1000

Measured across both builder repos on 2026-08-02: 49 groom issues, 7 bail issues, exactly 1 patch-too-large — and that one was 403 lines changed (> 400). Three lines over. The other 6 bails are CI-privileged-file / security refusals that no size change affects. So 400 was not systematically too tight; it caught one boundary case, and 600 buys headroom for that shape without a large jump.

1000 was rejected deliberately, even though it is the org's human PR cap (pr-size.yml max_lines), for two reasons — both now recorded in the input's description so the next person doesn't "harmonize" the numbers:

Different counting basis. pr-size.yml counts non-generated changed lines — it strips lockfiles and linguist-generated/Code generated output first, and offers an oversized-ok bypass label. Groom counts raw changed lines with no exclusions. 1000-non-generated and 1000-raw are not the same quantity.
Different risk. The human cap is advisory on a diff whose author chose the scope and can defend it in review. pr_size_limit is a hard refusal on a diff with no author to interrogate; the failure mode is a reviewer approving 900 plausible-looking machine-written lines because nothing in it looks wrong. The machine does not get the same benefit of the doubt, on purpose.

What changed

.github/workflows/groom.yml — pr_size_limit default 400 → 600, plus the expanded description above (below-the-human-cap rationale + the measured evidence for the raise).
Same file — a code comment's illustrative value (e.g. 400.0) follows the default to 600.0. The parsing behaviour it documents is unchanged.

What deliberately did NOT change

pr_size_limit stays in _LOCKED_KEYS (.github/groom/config.py). It is the backstop against an unreviewably-large machine patch; a repo raising its own ceiling via GROOM_CONFIG without review defeats it. Raising the shared default here, under review, is the sanctioned act. Already covered by test_pr_size_limit_is_locked.
bail_sink (BE-6157) is untouched. That ticket is about suppressing bail issues; this one is about producing fewer bails in the first place. Complementary, neither replaces the other.
No new test asserting 600. Nothing in .github/groom/tests/ asserted 400 — the number lived in exactly one place (this default) — and a test that re-reads the YAML to assert the literal would recreate the two-places drift the ticket warns about. The invariant actually worth guarding (the key stays locked) is already tested.

Propagation — this does not take effect immediately

Callers are SHA-pinned (uses: ...@<sha> + matching workflows_ref), so a repo keeps 400 until its pin advances past this commit: merge here → bump-groom-callers.yml opens SHA-bump PRs for every repo in GROOM_CALLERS → 600 takes effect per repo as those merge. Two consequences worth stating so nobody is surprised: the 4 callers absent from GROOM_CALLERS (already stranded on older SHAs, BE-6015) will not get this; and the pending builder callers of BE-5920 inherit 600 for free since they pin current main at implementation time.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' → 170 tests, OK.
Parsed groom.yml with PyYAML and read the resolved input back: default is the integer 600, and the folded description renders as the intended three paragraphs (blank-line handling in >- is easy to get wrong, so it was checked rather than assumed).
Cross-checked every claim the new description makes about pr-size.yml against that file: max_lines default 1000, generated/lockfile exclusion, oversized-ok bypass label — all accurate.
Swept pr_size_limit / PR_SIZE_LIMIT / 400 across workflows, .github/groom/, .github/bump-callers/, and both READMEs: no caller in this repo passes pr_size_limit, so no caller-side value silently keeps 400.

Judgment calls

The description now carries the why, at some length, because the "just match the 1000-line human cap" instinct is exactly the thing that would quietly undo this. The literal 600 appears in the prose only in the historical "raised 400 → 600" note; the forward-looking sentence says "the default" so the prose can't drift from the value.
The two acceptance criteria that live past merge — bump PRs opening for registered callers, and confirming 600 in effect on the next builder run via the resolved-knobs line (not the per-job pr_size_limit: input echo, which shows the input default) — cannot be met inside this PR and are unverified here by construction.

_{🔄 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/108 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/2/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6161-groom-pr-size-limit-600` --- ### 📝 Commits (3) - [`3ef62a0`](https://github.com/Comfy-Org/github-workflows/commit/3ef62a07b4071037e6b4525746069ebd984e8540) feat(groom): raise the pr_size_limit default from 400 to 600 lines (BE-6161) - [`5755028`](https://github.com/Comfy-Org/github-workflows/commit/5755028e27628349001eec9044e2e0f20940a4ed) docs(groom): correct two overclaims in the pr_size_limit rationale (BE-6161) - [`f07fd00`](https://github.com/Comfy-Org/github-workflows/commit/f07fd00ece2df9863869c7b72c62ab80214d7c46) Merge remote-tracking branch 'origin/main' into matt/be-6161-groom-pr-size-limit-600 ### 📊 Changes **1 file changed** (+29 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/groom.yml` (+29 -3) </details> ### 📄 Description ## ELI-5 Groom's auto-builder refuses to open a PR whose patch changes more than `pr_size_limit` lines — it files a `groom` issue instead so nobody has to review a giant machine-written diff. That ceiling was 400. This raises the shared default to **600**, in the reusable workflow, so it changes once under review instead of 28 times across callers. ## Why 600, and why not 1000 Measured across both builder repos on 2026-08-02: **49 `groom` issues, 7 bail issues, exactly 1 patch-too-large** — and that one was `403 lines changed (> 400)`. Three lines over. The other 6 bails are CI-privileged-file / security refusals that no size change affects. So 400 was not systematically too tight; it caught one boundary case, and 600 buys headroom for that shape without a large jump. **1000 was rejected deliberately**, even though it is the org's human PR cap (`pr-size.yml` `max_lines`), for two reasons — both now recorded in the input's description so the next person doesn't "harmonize" the numbers: 1. **Different counting basis.** `pr-size.yml` counts *non-generated* changed lines — it strips lockfiles and `linguist-generated`/`Code generated` output first, and offers an `oversized-ok` bypass label. Groom counts raw changed lines with no exclusions. 1000-non-generated and 1000-raw are not the same quantity. 2. **Different risk.** The human cap is advisory on a diff whose author chose the scope and can defend it in review. `pr_size_limit` is a hard refusal on a diff with no author to interrogate; the failure mode is a reviewer approving 900 plausible-looking machine-written lines because nothing in it looks wrong. The machine does not get the same benefit of the doubt, on purpose. ## What changed - `.github/workflows/groom.yml` — `pr_size_limit` default `400` → `600`, plus the expanded description above (below-the-human-cap rationale + the measured evidence for the raise). - Same file — a code comment's illustrative value (`e.g. 400.0`) follows the default to `600.0`. The parsing behaviour it documents is unchanged. ## What deliberately did NOT change - **`pr_size_limit` stays in `_LOCKED_KEYS`** (`.github/groom/config.py`). It is the backstop against an unreviewably-large machine patch; a repo raising its own ceiling via `GROOM_CONFIG` without review defeats it. Raising the shared default here, under review, is the sanctioned act. Already covered by `test_pr_size_limit_is_locked`. - **`bail_sink` (BE-6157) is untouched.** That ticket is about suppressing bail *issues*; this one is about producing fewer bails in the first place. Complementary, neither replaces the other. - **No new test asserting `600`.** Nothing in `.github/groom/tests/` asserted `400` — the number lived in exactly one place (this default) — and a test that re-reads the YAML to assert the literal would recreate the two-places drift the ticket warns about. The invariant actually worth guarding (the key stays locked) is already tested. ## Propagation — this does not take effect immediately Callers are SHA-pinned (`uses: ...@<sha>` + matching `workflows_ref`), so a repo keeps 400 until its pin advances past this commit: merge here → `bump-groom-callers.yml` opens SHA-bump PRs for every repo in `GROOM_CALLERS` → 600 takes effect per repo as those merge. Two consequences worth stating so nobody is surprised: the 4 callers absent from `GROOM_CALLERS` (already stranded on older SHAs, BE-6015) will **not** get this; and the pending builder callers of BE-5920 inherit 600 for free since they pin current main at implementation time. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` → **170 tests, OK**. - Parsed `groom.yml` with PyYAML and read the resolved input back: `default` is the integer `600`, and the folded description renders as the intended three paragraphs (blank-line handling in `>-` is easy to get wrong, so it was checked rather than assumed). - Cross-checked every claim the new description makes about `pr-size.yml` against that file: `max_lines` default `1000`, generated/lockfile exclusion, `oversized-ok` bypass label — all accurate. - Swept `pr_size_limit` / `PR_SIZE_LIMIT` / `400` across workflows, `.github/groom/`, `.github/bump-callers/`, and both READMEs: no caller in this repo passes `pr_size_limit`, so no caller-side value silently keeps 400. ## Judgment calls - The description now carries the *why*, at some length, because the "just match the 1000-line human cap" instinct is exactly the thing that would quietly undo this. The literal `600` appears in the prose only in the historical "raised 400 → 600" note; the forward-looking sentence says "the default" so the prose can't drift from the value. - The two acceptance criteria that live past merge — bump PRs opening for registered callers, and confirming 600 in effect on the next builder run via the **resolved-knobs** line (not the per-job `pr_size_limit:` input echo, which shows the input default) — cannot be met inside this PR and are unverified here by construction. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:25 +00:00