fix(gate): the file-size ratchet judges the change, not the tree (#2004) - #2267
Conversation
`scripts/file-size-baseline.txt` is one shared cell every growing PR must write, and three times running (#1761, #1782, #2003) two PRs that each wrote it CORRECTLY composed into a red `main`. Each regenerates the whole baseline against a snapshot of `main` that does not yet carry the other's growth, so each records a stale ceiling for a file it never touched. The merge is textually clean — the two sides edit different lines — and the result is a ceiling one line below an actual. `main` then stays red and every subsequent PR inherits a failure it did not cause; #1992 sat blocked on exactly this with a tree byte-identical to `main`. Note what did not happen in that composition: the file never grew. Its ceiling moved down underneath it. A guard with one tree to look at cannot tell those apart, so it now asks the per-change question against the base: fail only when current > max(ceiling, size at base) Both halves of the `max` earn their place. `ceiling` alone is the old check and fails the innocent PR. `size at base` alone would pass a file already over its ceiling that THIS change grows further — turning inherited drift into a standing licence to bloat, which is worse than the red main being fixed. Taking the larger fails real growth and is silent about inherited violations. A ceiling raised deliberately via `--update` still passes exactly as before, since the regenerated ceiling equals the current size. The base is the pair check-deleted-tests.sh already uses, for the same reason: on a `pull_request` the checkout is `refs/pull/N/merge`, so HEAD^1 is the base branch tip and the question becomes "does this MERGE grow a god file?". ci.yml already sets the `fetch-depth: 2` that needs. Locally it is the merge base with origin/main; on a linear push it is HEAD^1. When no base resolves — shallow clone, root commit, no origin — the guard falls back to the old whole-tree check, because an unresolvable base must make the ratchet stricter, never weaker. Drift is still reported, on stderr and in the summary line, so the baseline debt stays visible instead of merely being tolerated. It just no longer fails the next PR to walk past it. Witness: scripts/test-file-size.sh gains five hermetic cases that build the skew from two commits in a throwaway repo. B1 (a ceiling lowered under an untouched file) and B2 (drift inherited by an unrelated change) both FAIL on the old guard and pass now. B3, B4 and B5 pass on both, which is the point of them — they constrain the fix rather than being satisfied by it, and B4 in particular pins the case a naive "already over, so ignore it" rule would wave through. Closes #2004
B1-B4 pass the base in by hand, so the rung that resolves it unaided - HEAD^1 on a `refs/pull/N/merge` checkout - was the one rung nothing exercised, and it is the one production depends on. B6 builds a real merge commit whose base branch drifted while the PR was open, and asserts the guard finds its own base. It fails on the old whole-tree guard alongside B1 and B2. Refs #2004
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Reviewer's GuideAdjusts the file-size guard to judge per-change growth against both the baseline ceiling and the size in the base tree, adds hermetic tests for baseline skew scenarios, and updates documentation and Makefile help text to reflect the new behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
`file-size.yml` is the cheap, requirable status context for the ratchet, and
it checked out at the default depth of 1 under a comment stating that full
history was not needed because the guard "reads the working tree and the
committed baseline, never a diff against the base". That was true until the
previous commit and is now false.
At depth 1 the merge commit's parents are not in the clone, so
`resolve_base_commit` finds nothing and falls back to the strict whole-tree
check. The job stays green and reports the ratchet exactly as before, which
is the failure mode worth naming: the fix would have been silently absent
from the one job whose entire purpose is to report this guard cheaply enough
to be required. Found by reading the CI log rather than by a red check,
because there is nothing red to see.
`fetch-depth: 2` is the same trade ci.yml's guards job already makes for
check-deleted-tests.sh — two trees, not two histories, one extra commit.
The guard now also names its mode in the summary line ("Judged against
<sha>" or "No base resolved — strict whole-tree check"). The change-relative
rule is silent by nature, showing itself only when something drifts, so
without that line a too-shallow checkout is indistinguishable from a clean
run in every log it will ever print.
Refs #2004
Review: changes requested — one rung of
|
|
Filed the two residue items so they are handoffs rather than prose: #2310 (nothing retightens the baseline now that inherited drift is non-fatal) and #2311 (the OBSOLETE/STALE classification did not inherit the base-relative reasoning). Reference them from the description and the remaining blocker is the |
1 participant
The bug
scripts/file-size-baseline.txtis one shared cell that every growing PR must write, and three times running (#1761, #1782, #2003) two PRs that each wrote it correctly composed into a redmain. Each regenerates the whole baseline against a snapshot ofmainthat does not yet carry the other's growth, so each records a stale ceiling for a file it never touched. The merge is textually clean — the two sides edit different lines — and the result is a ceiling one line below an actual.mainthen stays red and every subsequent PR inherits a failure it did not cause; #1992 sat blocked on exactly this with a tree byte-identical tomain.The detail that names the fix: in that composition the file never grew. Its ceiling moved down underneath it. A guard with one tree to look at cannot tell those two apart, because "is this tree consistent with this baseline snapshot?" is a whole-tree question and the thing worth preventing is a per-change one.
The fix
The ratchet now judges the change:
Both halves of the
maxearn their place, and that is the whole design:ceilingalone is the old check, and it fails the innocent PR above.size at basealone would pass a file that is already over its ceiling and that this change grows further — turning inherited drift into a standing licence to bloat. That is strictly worse than the redmainbeing fixed here.Taking the larger fails a change that genuinely grows a god file past what it inherited, and stays silent when the violation arrived from somewhere else. A ceiling raised deliberately via
--updatepasses exactly as before, since the regenerated ceiling equals the current size — that path is untouched.Where the base comes from is the same pair
scripts/check-deleted-tests.shalready uses, for the same reason: on apull_requestthe checkout isrefs/pull/N/merge, soHEAD^1is the base branch tip and the question becomes "does this merge grow a god file?" — the question a required check should answer.ci.ymlalready sets thefetch-depth: 2that needs, for that guard. Locally it is the merge base withorigin/main; on a linear push it isHEAD^1.When no base resolves — shallow clone, root commit, no origin — the guard falls back to the old whole-tree check. That direction is deliberate and is pinned by a test: an unresolvable base must make the ratchet stricter, never weaker.
Drift is still reported, on stderr and in the summary line, so the baseline debt stays visible rather than merely tolerated. It just no longer fails the next PR to walk past it.
Witness tests
scripts/test-file-size.shgains six hermetic cases that rebuild the skew from two commits in a throwaway repo — no network, no reliance on this repository's real history. Verified the artisanal way, by running the new suite againstorigin/main's guard:B1, B2, B6 are the witnesses — they fail on the old code and pass on the new. B3, B4, B5 pass on both, which is the point of them: they constrain the fix rather than being satisfied by it. B4 is the one I would ask a reviewer to look at hardest — it is the case a naive "already over at the base, so ignore it" rule would wave through, and it is why the rule takes the
maxrather than either term alone. B6 exists because B1–B4 all pass the base in by hand, which left the rung production actually depends on as the one rung nothing exercised.With the fix in place: 13 passed, 0 failed (the 7 pre-existing language-coverage cases are untouched and still green).
Verification
make gate's Rust tiers are unaffected — this change is shell and markdown only.Definition of done, from the issue
mainlacking the other's growth, compose green — B1/B2, and B6 in the real merge shape.check-god-files.shstill green; guards green.Scope I deliberately did not take
The same shared-cell shape could in principle red an innocent PR through the
OBSOLETEandSTALEbranches (a sibling retires a baseline entry you did not). That has never been observed, and widening the change-relative rule to those paths without a witness would be speculative — the three recorded occurrences are all theGREWbranch. Noted here rather than silently left.Also worth stating plainly, since two issues were recently written on the opposite premise: splitting a god file buys structure, not slack.
--updateretightens every ceiling to its file's current size, so a freshly split file sits at zero headroom again — see my comment on this issue. This PR is what removes the tax; the split does not.Closes #2004
Follow-up found in this PR's own CI log
The first green run exposed something no red check would have:
.github/workflows/file-size.yml— the cheap, requirable status context for this very guard — checked out at the default depth of 1, under a comment stating full history was unnecessary because the guard "reads the working tree and the committed baseline, never a diff against the base."That was true until this PR and is now false. At depth 1 the merge commit's parents are not in the clone, so the base cannot resolve and the guard falls back to the strict whole-tree check. The job stays green and reports the ratchet exactly as before — so the fix would have been silently absent from the one job whose purpose is to report this guard cheaply enough to be required.
ci.yml's guards job was already atfetch-depth: 2and unaffected; this workflow is now too, the same trade it already makes forcheck-deleted-tests.sh.To stop that class of silent non-application recurring, the guard now names its mode in the summary line:
The change-relative rule is silent by nature — it only shows itself when something drifts — so without that line a too-shallow checkout is indistinguishable from a clean run in every log it will ever print. Now it is one glance.