feat(groom): raise the pr_size_limit default from 400 to 600 lines (BE-6161) - #108
feat(groom): raise the pr_size_limit default from 400 to 600 lines (BE-6161)#108mattmillerai wants to merge 2 commits into
Conversation
…E-6161) Measured across both builder repos on 2026-08-02: 49 groom issues, 7 bails, exactly one patch-too-large — and that one was 403 lines changed (> 400). The other six bails are CI-privileged-file / security refusals no size change affects. So 400 was not systematically tight; it caught one boundary case, and 600 buys headroom for that shape without a large jump. 1000 (the org's human PR cap in pr-size.yml) is deliberately NOT copied: that cap counts non-generated lines with an oversized-ok bypass and is advisory on an author-defended diff, while this is a hard refusal on raw changed lines with no author to interrogate. The input description now records both reasons. pr_size_limit stays a _LOCKED_KEY — a repo must not raise its own ceiling via GROOM_CONFIG; raising the shared default under review is the sanctioned act.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 2 finding(s).
| Severity | Count |
|---|---|
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…E-6161) Review found the new prose overstates two things about the size gate. - The `_LOCKED_KEY` note said review here is "the only way it moves". The lock covers the GROOM_CONFIG/config layers only and explicitly exempts caller defaults (config.py), and the builder job reads `inputs.pr_size_limit` directly — so a consumer caller can still raise its own ceiling via `with:`. Say what the lock actually buys (no un-reviewed variable-layer raise) and point an auditor at the caller. - "raw changed lines with no exclusions" oversells the bound: the count sums `git diff --cached --numstat`, which reports `-` for binary files (counted as 0), and counts lines regardless of length. Name both gaps so the basis is not read as a byte budget.
2 participants
ELI-5
Groom's auto-builder refuses to open a PR whose patch changes more than
pr_size_limitlines — it files agroomissue 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
groomissues, 7 bail issues, exactly 1 patch-too-large — and that one was403 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.ymlmax_lines), for two reasons — both now recorded in the input's description so the next person doesn't "harmonize" the numbers:pr-size.ymlcounts non-generated changed lines — it strips lockfiles andlinguist-generated/Code generatedoutput first, and offers anoversized-okbypass label. Groom counts raw changed lines with no exclusions. 1000-non-generated and 1000-raw are not the same quantity.pr_size_limitis 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_limitdefault400→600, plus the expanded description above (below-the-human-cap rationale + the measured evidence for the raise).e.g. 400.0) follows the default to600.0. The parsing behaviour it documents is unchanged.What deliberately did NOT change
pr_size_limitstays in_LOCKED_KEYS(.github/groom/config.py). It is the backstop against an unreviewably-large machine patch; a repo raising its own ceiling viaGROOM_CONFIGwithout review defeats it. Raising the shared default here, under review, is the sanctioned act. Already covered bytest_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.600. Nothing in.github/groom/tests/asserted400— 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>+ matchingworkflows_ref), so a repo keeps 400 until its pin advances past this commit: merge here →bump-groom-callers.ymlopens SHA-bump PRs for every repo inGROOM_CALLERS→ 600 takes effect per repo as those merge. Two consequences worth stating so nobody is surprised: the 4 callers absent fromGROOM_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.groom.ymlwith PyYAML and read the resolved input back:defaultis the integer600, 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).pr-size.ymlagainst that file:max_linesdefault1000, generated/lockfile exclusion,oversized-okbypass label — all accurate.pr_size_limit/PR_SIZE_LIMIT/400across workflows,.github/groom/,.github/bump-callers/, and both READMEs: no caller in this repo passespr_size_limit, so no caller-side value silently keeps 400.Judgment calls
600appears 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.pr_size_limit:input echo, which shows the input default) — cannot be met inside this PR and are unverified here by construction.