fix(ci): burn one eslint warning down to the ratchet ceiling - #7480
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of The entire diff is a single removed unused type import in a test file — no user-facing surface, no strings, no rendered UI. Clean pass. UX-Verdict: PASS One-line unused-import removal in a test file; nothing a user can see, read, or interact with changes. [UX-REVIEWED] ee9aa93 |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Smallest true fix to the drifted ratchet, correctly deferring the systemic skip-lane gap to a named follow-up. Suggestions
[DESIGN-REVIEWED] ee9aa93 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
|
Status: the lane this PR exists for is green -- Frontend Lint & Type Check passes on The one remaining red is a SECOND main-owned ratchet drift, unrelated to this diff and already being fixed elsewhere: This diff touches one file, Worth noting the two drifts are the same failure class from opposite directions -- a one-way ratchet that nobody re-measures when the code moves under it. The eslint ceiling drifted UP past its count; the log-site census stayed ABOVE a count that dropped. This PR's pattern-harvest note argues for measuring on push to main so the commit that moves the number is the one that gets the red; #7492's case is the same argument with the sign flipped, and the census test already says so in its own comment ("Without this the census only ever ratchets one way"). |
The Frontend Lint gate runs `npx eslint src/ --max-warnings 659`, and main currently measures 660 warnings with 0 errors, so the lane fails on every PR whose changed surface reaches it. The ceiling is meant to EQUAL the measured count, and the workflow's own comment says to burn the warning down rather than raise it. Removes an unused `type ReactNode` import from a test file: the smallest true warning in the list, no product code, no behaviour. Measured 660 -> 659, and `npx eslint src/ --max-warnings 659` now exits 0.
6d00bed to
ee9aa93
Compare
|
Rebased onto Rebased rather than re-run: Re-measured against the new main before pushing, since main has moved a lot today and the burn-down would be pointless if the count had already come down: So the drift is still live at 660 and this one-line removal still lands exactly on the ceiling. The diff is unchanged: one unused |
Problem / Motivation
The Frontend Lint gate runs
npx eslint src/ --max-warnings 659. Main currentlymeasures 660 warnings, 0 errors, so the lane fails on every PR whose changed
surface reaches it -- for reasons that have nothing to do with that PR's diff.
Measured on
dd9e002bf(main tip at the time of writing):The same count appears in CI. Observed today: #7462
passed this lane at 03:41Z, and PRs whose surface reached it after that failed
with exactly this 660-vs-659 signature.
Why it matters
It is a false red on unrelated work, and a costly one to diagnose: the failing
PR's author has to prove the warnings are not theirs before they can ignore it
(CI lints the merge with main, so the linted frontend tree is main's even for a
backend-only diff). Meanwhile the readiness gate counts the lane as a failure,
so the PR cannot go green, and
/ai-review overridedoes not apply because thisis a CI lane rather than a review bot.
Most PRs skip the lane, which is why the drift went unnoticed -- it only bites
the occasional PR whose changed surface pulls the frontend in.
What changed (motivation -> approach -> change)
The ceiling is documented as something to burn DOWN, not raise: "The ceiling must
EQUAL the measured count, not sit above it: slack is silent admission ... do not
raise it." So the fix is to remove one real warning, not to touch the gate.
Removed an unused
type ReactNodeimport fromwebsite/src/test/useQueuedMessageActions.test.tsx. It is the smallest truewarning available: a test file, no product code, no behaviour, and the symbol is
referenced nowhere else in the file (
grep ReactNodereturns only the import).Deliberately ONE warning, not a batch: it restores the gate with the smallest
reviewable diff, and it leaves the remaining 659 for a burn-down that can be
judged on its own merits.
eslintreports 8 of them as--fix-able if someonewants to take a bite.
Tests
npx vitest run src/test/useQueuedMessageActions.test.tsx-- 17 tests pass,unchanged.
Gate verification, the exact CI command:
Manual verification
N/A -- a removed unused type import has no runtime surface. The measurement above
is the verification.
Related Issues
None filed; found while diagnosing a red Frontend Lint lane on an unrelated
backend PR (#7435), which
carries the full evidence trail.
Pattern harvest
Rule candidate:
ci.Pattern: "a ratchet whose gate is SKIPPED for most diffs stops being a ratchet --
it becomes a landmine for whoever's changed surface happens to reach it."
The count can only drift upward while the lane is skipped, and the drift is then
charged to an unrelated PR, which is the expensive part: the failing author must
first prove the warnings are not theirs. Two mechanical options, both cheap:
so the commit that adds warning 660 is the one that gets the red.
so a backend-only diff reads "0 added by this PR" instead of a bare
660 > 659.Either turns this class from "diagnose it on someone else's PR" into "the author
who added it sees it".