refactor(frontend): burn the eslint warning ceiling to zero - #7569
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of All checks confirm the commit message's claims are backed by the diff: the gate flips to Design-Verdict: PASS Ratchet-to-zero with a pinned gate is the right end state; every warning class got a reasoned answer, not a bulk suppression, with pixel-level falsification for markup swaps. [DESIGN-REVIEWED] 93fab2d |
UX Review (Fable 5) — ✅ PASSUX-level review of All evidence is in. The UX-Verdict: PASS Markup swaps are pixel-identical (screenshots prove it), labels name outcomes in all 12 locales, and keyboard paths fire the exact click actions. [UX-REVIEWED] 93fab2d |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All surfaces check out against the repo. Final verification of what I established: the zero ceiling is the endpoint the base workflow comment itself mandated ("Ratchet --max-warnings down toward 0"); First-Principles-Verdict: PASS The hard-zero gate is the ratchet's own documented endpoint, and every rider in the diff is the honest resolution of one of the 597 warnings. What this change shipsIntent: make the frontend lint gate reject any new warning by clearing all 597 — a FIX (of gate slack and lint debt).
WatchItems 7–8 are user-visible behavior fixes shipping under a [FIRST-PRINCIPLES-REVIEWED] 93fab2d |
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: |
d1e685c to
472788d
Compare
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: |
|
| jobs | cause | evidence |
|---|---|---|
(3.10, 3), (3.12, 3), (Windows) (3) |
pre-existing log-site census break on main |
reproduced on a pristine origin/main worktree; #7572 fixes it |
(Windows) (2) |
test_irq.py coalescing-window timing flake |
60/60 pass locally; same test flaked on an unrelated branch today |
Worth saying rather than leaving implicit: this is the argument for the flake being
measured rather than reruns being normalised. A test that asserts a window did not
expire, using wall-clock settling on a shared runner, will keep doing this — the
durable fix is to drive the clock rather than to sleep against it. That is out of
scope here, but it is a real finding and I would rather record it than quietly hit
rerun.
472788d to
344fdd5
Compare
344fdd5 to
bf36f58
Compare
|
Audit note — #7745 is being closed in favour of this PRYou are the surviving implementation; #7745 is being closed. What the two sharedVerified independently, not from the earlier finding: #7745's entire diff is 13 lines, one file, one hunk in website/src/test/issueRadarNarrowViewport.test.ts turning What #7745 had that this PR does notPlease pick these up (or say they are not wanted) so they do not disappear with that branch: No code. The one line is already byte-identical inside #7569 (website/src/test/issueRadarNarrowViewport.test.ts, the test 'names the LIST in the Back control, not one item from it': This PR still needs work:
|
c733254 to
8b7e5d5
Compare
The Lint gate carried 597 measured warnings under a 597 ceiling. This clears all
597 and makes the gate a hard zero, so the next warning to land is the next one
a reviewer sees rather than one more entry in a number nobody reads.
No rule was turned off and the ceiling was never raised. `eslint.config.js` gains
coverage rather than losing it: `src/**/*.mjs` is matched by no other block, so
the single such file, `crew-ghost-sprite.gen.mjs`, lints against an EMPTY rule
set. Its `eval()` of a drawing program built as a template literal in that same
file is therefore unreachable by `no-eval`, and a directive naming that rule reads
as UNUSED -- a warning clearable only by deleting a true statement and letting a
real violation return silently later. Enabling `no-eval` for `.mjs` makes the
directive live: the exemption is reviewed rather than an accident of config
coverage, and a second `eval()` in that file would be an error. The comment
asserting that no rule reaches a `.mjs` file under `src/` is replaced by the
directive it describes, because this block makes that assertion false.
What the 597 were, and how each class was answered:
- 383 `no-explicit-any`. Where a shape was already named, the existing name is
used: `Awaited<ReturnType<typeof api.artifactComments>>`, `RootState['chat']`,
`typeof fetch`, `ChatSlot[]`. In `apps/mochi` most were
`api?.getX?.().then((c: any) => ...)`, which threw away the typed seam
`mochiApi.ts` exists to provide -- its header says the pre-migration `any` handle
was the bug -- so the annotation is DELETED and inference supplies the real type.
In test files the diff is type-erasure-only, verified per file by comparing
transpiler output, because `tsconfig.app.json` excludes `src/test` and nothing
typechecks it.
- 116 `jsx-a11y/*`, across 9 rules. Real markup work: labels bound to the controls
they already name (`htmlFor`/`id`), `role` + `tabIndex` + a key handler that
fires the same action as the click, `<Clickable>`/`<button>` where a
`<div onClick>` was a control. 51 are suppressions with a stated reason, and they
are concentrated in three shapes the rule reads as gestures but that no keyboard
can reach: `onLoad` / `onError` resource events on `<img>`/`<iframe>`, handlers
that only `stopPropagation`, and a dialog root's Escape/focus-trap.
- 58 `react-hooks/exhaustive-deps`, decided one at a time. 37 name the missing
dependency, several stabilising it first (`useMemo` over
`commentsQuery.data?.comments`, a module-level `NO_EVENTS`) so the effect does
not re-run per render; 21 keep the exclusion with the invariant written out.
- 21 `no-console`, kept as deliberate diagnostics with the house-style `--`
justification, matching the ~94 already in the tree.
- 19 `no-unused-vars`, all in tests. Where the binding's initializer CALLS
something, the binding is gone and the call stays.
Two new catalog keys are translated into all 12 languages, and the Korean one
ships both 조사 forms (`{{name}}와(과)`), which `koStyle.test.ts` requires because
the particle depends on a value not known until render.
The attachment chip's new image-open button takes the accessible name the three
other `openLightbox` controls already use (`open_image`) rather than `preview`,
which names a different action (`api.previewFile`). One operation, one name, and
no new catalog key: the `<action>: <file name>` shape is the sibling Remove
button's, and the file name is what distinguishes two images in one chip row.
`website/capture/a11y-label-swap.tsx` is added so the three markup swaps that can
move pixels stay falsifiable: captured at both revisions, the frames are identical
outside the attachment chip's alt-text fallback.
The focus-cue gate is diff-scoped, so adding `id`/`aria-labelledby` to two
borderless `<input>`s made this change their owner: both carried `outline-none`
with nothing in its place, which suppressed the global `:focus-visible` outline and
left a keyboard user unable to see them take focus. The suppressor is dropped so
`index.css`'s `2px solid var(--accent)` applies.
`test_eslint_warning_ceiling.py` gains the other half of the invariant. It already
refused a second ceiling in the workflow and a transcribed copy of the number in
prose; it now asserts the ceiling IS zero. The workflow comment asking the next
author not to lift it is prose, and eslint exits 0 under a lifted ceiling either
way, so this is the half that does not depend on the comment being read. The
transcription check narrows to what it was for: a value that can DRIFT. At zero
the value is held by the assertion above rather than by a measurement, so a doc
quoting `--max-warnings 0` cannot go stale -- while scanning prose for the literal
`0` would ban that accurate sentence and fire on any unrelated digit in a
`max-warnings` line.
`docs/ci/ci-and-reviews.md` documents this job, and it described the ceiling as a
ratchet baseline equal to the measured count, so it is updated in the same commit
to describe the hard zero the job now enforces.
8b7e5d5 to
93fab2d
Compare
The mechanism the lane names is real and the proofs it lists genuinely cannot see it. The Why it is inseparable, at the site. Before this change the handler read
So there is no ordering in which a Why the residual risk is bounded rather than merely asserted. // Focus is handed back only when the bar was actually open: ChatPage's
// file/folder-open handlers call close() unconditionally to un-gate the
// dock, and a close that never dismissed anything must not steal focus.
const wasOpen = isOpenRef.currentThe correctness lane reached the same conclusion independently at this head (Opus 4.8: no What did change in response. The lane's point about the title is the part with no code No code change for this item. |
Legitimate, and the mechanism is exactly as described: Fixed at head ceiling = _CEILING.search(_ci_text())
assert ceiling is not None
assert ceiling.group(1) == "0", (
f"ci.yml's eslint ceiling is {ceiling.group(1)}, not 0. ..."
)Verified both directions: it fails with Two notes on where it went, since the lane suggested a workflow grep: The test file, not a shell grep in Also fixed in the same commit, from the UX lane's suggestion: |
Legitimate, and sharper than stated: Fixed at head aria-label={`${i18nT('apps.mochi.chatPanel.open_image')}: ${item.name}`}
Gates re-run on the branch as pushed: |
This is the same finding the lane raised at Re-anchoring it here because that record names an older head and the lane has since re-judged What is genuinely new, and was not answered before, is the sentence the lane ends on: the The subject stays
If a maintainer weighs it the other way, the remedy is a one-line retitle to No code change for this item. |
Re-rebased onto the current
|
buluoray
left a comment
There was a problem hiding this comment.
Reviewed at head SHA 93fab2d1294bfdccd870636f1bf3b0ea5212a511.
Verdict: Approve. Blocking findings: 0.
Method (195 files, not read in full)
I classified the diff by edit shape and read individually only the shapes that can change behaviour. Files/hunks read individually: 8 (ci.yml, eslint.config.js, test_eslint_warning_ceiling.py, and the four production hunks below — usePanelTabs.ts and ChatPage.tsx fetched as full source at head SHA to verify dependency claims, plus RemoteArtifactDetailPage.tsx and ArtifactDeployPage.tsx). The remaining ~187 files were taken in bulk by shape (unused-import removal, any→typed generics, a11y/no-console disable comments), which are behaviourally inert.
Core claim verified
- The ceiling was moved
--max-warnings 597→0in the same commit (.github/workflows/ci.yml), andtest/test_eslint_warning_ceiling.pygainstest_the_ceiling_is_zero, which asserts the ci.yml value is exactly0(would redden on any revert to a non-zero ceiling). - "Frontend Lint & Type Check" (which runs
npx eslint src/ --max-warnings 0) is green on this head SHA. Because the repo requires the ceiling to equal the measured count, a green run authoritatively confirms both that the tree is at zero warnings and that the ceiling is not stale slack.
Behaviour-capable shapes, all checked
- Hook dependency changes (~30): additions are dependency widening (safe; at most extra re-runs). Removals verified against source at head SHA:
ChatPage.switchAgentreads none of the removedinstalledAgents/provider/queryClient(only the comment names them);usePanelTabs.openFilecallsupdate(...), so[upsert]→[update]corrects a stale dep;KiroCrewAgentsPageedit-opener reads only its arga.*, so[defaultAgent]→[]is a genuine unnecessary-dep removal. ??(18): all are either stable module-level empty-constant refactors (preserve nullish semantics) or type-only assertions. No||→??conversion, so no falsy-0/""/falsesemantics change.- 21
exhaustive-depsdisables: each documents a standard intentional pattern (mount-once effect, subscribe-once listener whose per-render closures touch only refs/stable setters, or listing a stable member of a fresh-literal object). Adding the comment is inert; none silence a user-facing bug on the changed path. async/await/void(14) and the one non-null assertion: the ArtifactDeploy recall/destroyasyncwas pre-existing (onlyany→SiteMutationRespchanged); the rest are=> voidtype annotations, test code, or thesave![1]!.body!test mock-inspection. No new promise swallowing or reordering on a production path.no-evalconfig block (src/**/*.mjs): enablesno-evalso the existing directive over a realeval()in the codegen script becomes a reviewed, live exemption; a secondeval()would now error.
Non-blocking observations (advisory, no change required)
eslint.config.js: the newsrc/**/*.mjsblock is deliberately one rule wide (nono-unused-vars/no-undeffor that glob). The comment already flags this as a known gap to widen when a second.mjslands — noting it, not asking for a change.- The subscribe-once
exhaustive-depssuppressions (Mochi sprite walk) are correct today because the captured closures read only refs/stable setters; they would need revisiting if those closures later read render state. Documented in the disable reasons.
Could not verify
- I did not run eslint locally against a full install; the zero-warning count is taken from the green
--max-warnings 0CI job on this SHA rather than an independent local measurement. - I did not open all 187 bulk-classified files line by line; they were grouped by diff shape and spot-checked, not individually read.
AI review lanes on this SHA are consistent: Design / UX / First-Principles PASS; GPT 5.6 and Opus 4.8 report no blocking findings.
eslint's recommended set does not include no-eval, and the base
src/**/*.{ts,tsx} block never enabled it, so eval was unlinted across the
entire application tree. PR #7569 already covered the .mjs generator side
of issue #7699 (the src/**/*.mjs block enabling no-eval, making the
generator's disable directive live) and burned the --max-warnings ceiling
to 0, so the ratchet hunk the issue proposed is obsolete. This adds the
remaining piece: no-eval as a hard-zero 'error' in the base block. The
tree has zero eval sites in .ts/.tsx (all matches are prose in comments),
verified by a clean eslint run (exit 0, 0 warnings).
Closes #7699
eslint's recommended set does not include no-eval, and the base
src/**/*.{ts,tsx} block never enabled it, so eval was unlinted across the
entire application tree. PR #7569 already covered the .mjs generator side
of issue #7699 (the src/**/*.mjs block enabling no-eval, making the
generator's disable directive live) and burned the --max-warnings ceiling
to 0, so the ratchet hunk the issue proposed is obsolete. This adds the
remaining piece: no-eval as a hard-zero 'error' in the base block. The
tree has zero eval sites in .ts/.tsx (all matches are prose in comments),
verified by a clean eslint run (exit 0, 0 warnings).
Closes #7699
Co-authored-by: Zezhen Xu <zezhexu@dev-dsk-zezhexu-2b-15d11a49.us-west-2.amazon.com>
Clears all 597 eslint warnings the frontend Lint gate is carrying and turns
that gate into a hard zero.
The ceiling reads
597, equal to the measured count onmain. After this there isno count to re-measure and no ceiling that can drift above the truth — and
test_eslint_warning_ceiling.pynow asserts the ceiling is0, so lifting itto admit a warning is a failing test rather than a comment someone did not read.
No rule was disabled, and the ceiling was never raised
eslint.config.jsgains coverage rather than losing it, and that is what takesthe count to 0 instead of 1.
src/**/*.mjsis matched by no other config block, so the one.mjsfile undersrc/—crew-ghost-sprite.gen.mjs, a hand-run codegen script — is linted with anempty rule set. Its
// eslint-disable-next-line no-eval, sitting above a realeval(program), was therefore reported as an unused directive: a warningclearable only by deleting a true statement and letting a real
no-evalviolationreturn silently later. That is the path that was taken on
main(#7753 deleted thedirective and left a comment asserting that no rule reaches a
.mjsfile), so thisbranch enables the rule and restores the directive, replacing the comment the
new config block makes false. Enabling
no-evalfor.mjsmakes the directivelive:
So the exemption is now a reviewed one instead of an accident of config coverage,
and a second
eval()in that file would be an error.What the 597 were
@typescript-eslint/no-explicit-anyjsx-a11y/*(9 rules)react-hooks/exhaustive-depsno-console@typescript-eslint/no-unused-varsno-explicit-any. Where a shape was already named, the existing name is usedrather than a new one:
Awaited<ReturnType<typeof api.artifactComments>>,RootState['chat'],typeof fetch,ChatSlot[], and the already-typed siblingmocks in
__mocks__/@radix-ui/as the pattern for the popover double. Inapps/mochimost wereapi?.getX?.().then((c: any) => …)— which throws away thetyped seam
mochiApi.tsexists to provide; its own header says thepre-migration
(window as any).mochihandle beinganywas the bug — so theannotation is deleted and inference supplies the real type. 122 came from one
byte-identical
vi.mock('framer-motion')block copy-pasted across 22 sidebartests, while 14 siblings in the same directory already carried it typed.
jsx-a11y. Labels bound to the controls they already name (htmlFor/id,needing no new string),
role+tabIndex+ a key handler that fires the sameaction as the click,
<Clickable>/<button>where a<div onClick>was really acontrol. The 51 suppressions concentrate in three shapes the rule reads as gestures
but that no keyboard can reach — and each says which:
onLoad/onErroron<img>/<iframe>— resource lifecycle events, not gesturesstopPropagation— no action to activateno-noninteractive-tabindexon a scrollport whose content holds no focusablechild, where removing the tab stop makes overflowing content unreachable by
keyboard: here the rule and WCAG 2.1.1 genuinely disagree and the suppression is
the accessible answer
exhaustive-deps, decided one at a time. 37 name the missing dependency,several stabilising it first so the effect does not re-run every render (
useMemoover
commentsQuery.data?.comments, a module-levelNO_EVENTSconstant). 21 keepthe exclusion with the invariant written out.
Two findings worth a reviewer's attention
1. Test files are typechecked by nothing.
tsconfig.app.jsonexcludessrc/testand**/*.test.ts(x):So
tsc -bcannot catch a wrong type written in a test — which is where most ofthe 383
anylived. The answer was to make every test-file typing changetype-erasure-only and prove it by transpiling both revisions and comparing the
emitted JS:
The 13 test files that differ are the ones that should: 8 removed a dead binding,
5 fixed a11y markup inside an inline test fixture. Every other test file is provably
annotation-only. I also ran an ad-hoc config that does include the test tree as a
per-file delta — it reports 1404 pre-existing errors so it cannot be a gate, but
it answers "did my file get worse?": tree total 1404 → 1402, no file worse. It
went down because the Radix popover mock had two real errors that
anywashiding.
2. A real bug the burndown surfaced, in
ChatPage.tsx.revealAppInPanelguarded its find-pane close on
search.isOpen, andrenderMessageheld thatcallback across renders where the find pane opens. A captured stale
isOpen === falseskipped the close, andisSidePanelHidden({…, searchOpen: true})then kept the dock hidden — so clicking an MCP tool row's "open app" opened a tab
the user could not see, which is exactly what that handler's own comment says the
close exists to prevent.
close()now runs unconditionally, as its three siblinghandlers already do (it only hands focus back
if (wasOpen)), which removes thestaleness and the churn instead of trading one for the other. The behaviour was
pre-existing; what this PR nearly added was a rationale comment that would have
stopped the next reader from fixing it.
Visual evidence
Of 147 changed
.tsx, 130 have an identical render surface — same elementtags, same
classNameliterals, same style bodies — because the fix only addsrole/tabIndex/aria-*/id/onKeyDown. Three swap a styled<div>/<span>/<img>for a real form element, which is the only class of changehere that can move a pixel: a
<label>and a<button>are inline where a<div>is block, so each had to re-state the layout it replaced.website/capture/a11y-label-swap.tsxmounts those three so the claim staysfalsifiable. Captured at this commit and at its parent, same URLs, both themes:
HEAD~1)Pixel diff: 791 of 1,083,760 px differ in dark (0.073%), 717 in light (0.066%),
and every one of them falls in a single band at CSS y 353–389 — inside the
attachment chip, on the alt-text fallback of a bitmap whose
srcis/api/file-raw, which only Electron's own host answers. The box under test isasserted 40×40 in both revisions, and the capture script fails rather than writing
a frame if it is not. Everything else — the
NumberFieldcaptions, thePackInfoHeadercaptions, the toggle row — is pixel-identical.Verification
npx eslint src/ --max-warnings 0npx tsc -bnpx vitest run(full suite)npx jscpd .npm run i18n:check.tsxidenticalTwo new catalog keys are translated into all 12 languages. The Korean one
ships both 조사 forms (
{{name}}와(과) 채팅 열기) —koStyle.test.tsrequires itbecause the particle depends on the final consonant of a value not known until
render, and a bare
과would renderMochi과for half of them. That test caught it.Every one of the ~190 files was reviewed by an adversarial pass whose only job was
to find a dishonest reduction, a dependency that creates a render loop, an
aria-labelthat fires but does nothing, or a hardcoded string that would fail thei18n gate. It raised 2 blocking issues and 21 nits; both blockers and 9 of the nits
are fixed in this commit, and the rest are recorded above or were correctly out of
scope for a lint pass.
One more gate, worth naming
Focus Cue Gatefailed on the first push and it was right to. It is diff-scoped,so adding
id/aria-labelledbyto two borderless<input>s made this changetheir owner — and both carried
outline-nonewith nothing in its place, whichsuppressed the global
:focus-visibleoutline entirely. A keyboard user could notsee either field take focus. The suppressor is dropped so
index.css'soutline: 2px solid var(--accent)applies, and the tree-wide backlog the gatereports stands at 5.
That is the same defect class as the 116 jsx-a11y warnings, found by a gate eslint
cannot express — worth noting because it means
--max-warnings 0is a floor, nota ceiling, on this kind of work.
The one cost of a hard zero, stated plainly
A ceiling of 0 closes the failure mode a stored count cannot — a PR that adds a
warning fails its own CI, with no bookkeeping and no number to re-measure. But it
makes the other failure mode more disruptive, and that is worth knowing before
merging rather than after.
CI validates
refs/pull/N/mergeagainst the base at run time, and nothingre-validates the combination once main moves. Two PRs can each be green and still
produce a warning together — one deletes a suppression as no-longer-needed while
the other adds the code that needed it. With a stored ceiling that lands inside the
slack and goes unnoticed; with 0 there is no slack, so it turns every subsequent
PR red on a gate none of them caused, until someone clears it.
That is not hypothetical here: the same shape has broken
mainthree times thisweek through a different ratchet (
ecab0babe,e7f02aeec,c412c2ff9), and acensus-backed backend gate is doing exactly that to a dozen open PRs right now.
The general fix is a
push: [main]post-merge job that re-runs the ratchets againstthe merged tree.
cd website && npx eslint src/ --max-warnings 0costs ~35s thereand needs no baseline file, which is the argument for driving a ratchet to 0 and
deleting its census wherever the backlog can actually be cleared: a censused ratchet
has two failure modes (the count drifts from the code, and a cross-merge slips
through), a hard zero has only the second. That job is out of scope for this PR, but
it is the thing that makes this gate cheap to live with.
Rebased onto
main, and the ceiling story is now a live exampleRebased again onto current
main. Base moved from63a043a7eto the tip; the onlymaincommit that touched a file this branch also touches is #7753 (4593b7df9,"drop a dead eslint directive and re-equal the warning ceiling"). Diffing this
branch's own patch at the previously-reviewed head against its patch here, the
resolution changed in exactly four files:
ci.yml— fix(ci): drop a dead eslint directive and re-equal the warning ceiling #7753 re-equalled the ceiling 599 → 597 and rewrote the surroundingcomment.
0subsumes 597 and is kept;main's comment text is kept where it isstill true, and every sentence naming a count is gone. Exactly one
--max-warningsliteral remains, whichtest_eslint_warning_ceiling.pypins —note that the pre-existing comment also matched (
Ratchet --max-warnings down toward 0), so rewriting it is what keeps that to one.crew-ghost-sprite.gen.mjs— newly in the diff, and it is the load-bearingone. fix(ci): drop a dead eslint directive and re-equal the warning ceiling #7753 cleared this file's unused-directive warning by deleting the
no-evaldirective aboveeval(program). That was correct onmain, wheresrc/**/*.mjsmatches noeslint.config.jsblock and so lints against an emptyrule set. It is not correct under this PR, whose config block makes
no-evallive for
.mjs: without restoring the directive, this branch turnsmain's treeinto a hard error (
eval can be harmful no-eval), not a warning. Thedirective is restored with a stated reason, and
main's now-false comment(asserting no rule reaches a
.mjsfile undersrc/) is replaced by onedescribing the live directive.
MarkdownRenderer.tsx— the branch's rewrite of the InlineCode chip'ssuppression rationale is dropped; only the two
exhaustive-depsfixes remain.The
role/tabIndex/onKeyDownand theno-noninteractive-element-to-interactive-roledirective with its
--reason were already in the merge base, somainownsthem and this branch was only lengthening prose on a line it otherwise does not
change. The shorter reason also matches the two sibling directives in the same
file.
FindingRow.tsx— dropped from the diff. Its only hunk was a three-linecomment above the row
<div>, whoserole="presentation"and rationale commentwere already in the merge base and already say the same thing.
Nothing else in the branch's 195-file patch changed shape across the rebase — the
i18n catalogues,
ChatPage.tsx,ChatInput.tsx,useWebSocket.ts,App.tsxandissue-radar/context.tsxall differ in content at the two heads only becausemainedited them underneath, not because the resolution moved.And the caveat below stopped being hypothetical while this PR was open. The
ceiling went 609 → 604 → 603 → 599 → 597 while this branch waited, and the 604 was a
raise, made against the instruction in its own comment: #7259 landed a third
exhaustive-depswarning inArtifactsPage.tsxwhile the gate said 603, somainmeasured one over its own ceiling and every open PR touching
website/**went redon a warning its diff never wrote. #7511 is why
main's own runs did not report itfirst, and #7695 carried the restoration.
That is exactly the cross-merge shape described in "The one cost of a hard zero" —
and it happened to this gate, repeatedly, rather than to a hypothetical one. It
cuts toward this PR rather than against it: a stored count has two failure modes
(the number drifts from the code, and a combination no single run can see), a hard
zero has only the second, needs no re-measurement on merge, and offers no slack for
an "unblock the fleet" raise to spend.
ArtifactsPage.tsxis already clean onmain, so this removes the number rather than restoring it.docs/ci/ci-and-reviews.mdis updated in the same commit, since it documented thejob as a ratchet baseline.
test/test_eslint_warning_ceiling.py— which exists tostop the number being transcribed into prose — gains the other half of its
invariant: it now asserts the ceiling is
0. That is the Design lane'ssuggestion taken. The workflow's "NEVER raise this number" comment is prose, eslint
exits 0 under a lifted ceiling either way, and this is the half that does not depend
on the comment being read.
The transcription check narrows in the same commit, to the case it was written for:
a value that can drift. At zero the value is held by the new assertion rather
than by a measurement, so a doc quoting the gate's real invocation
(
--max-warnings 0) cannot go stale — while a prose scan for the literal0wouldban exactly that accurate sentence, and would fire on any unrelated digit in a
max-warningsline. So the docs bullet now names--max-warnings 0, which is thefirst time this file has been able to.
Responses to the review
runModel.ts— fixed, and the reviewer was right. First Principles flagged thatthe
any → unknownconversion there had added runtime coercion (asNumber(spent) ?? 0,a missing
agent_idfolding to''), which changes what a malformed workflow eventrenders. For well-formed events — the actual backend contract — behaviour was
identical, and arguably the coercion was an improvement since it removed a
NaNpath. It still had no business being here: a lint pass should not quietly decide what
an invalid event renders, and nothing tests that path. The two helpers are gone and
the reads are plain assertions carrying exactly the trust the
anydid:The
ChatPage.tsxclose()change does want a human eye, as flagged — it is theone user-observable behaviour change in the PR and it sits under a
refactor:title.The evidence beyond "three siblings do it" is that
useMessageSearch's ownclose()documents this exact call pattern as the supported one:So an unconditional
close()with nothing open is a no-op by design, not atolerated accident. The alternative — suppressing
exhaustive-depsand keeping thesearch.isOpenguard — would have written a rationale comment enshrining the bug,which is what the review agreed is worse. Still: this is the hunk to look at if you
look at only one.
PendingAttachments.tsx— the UX lane's naming suggestion taken. The newimage-open button was labelled
${preview}: ${item.name}while the three siblingopenLightboxcontrols inChatPanel.tsxsayopen_image; worse,previewisalso the name of a different control in that same file (
api.previewFile). It nowuses
open_image, so one operation has one name. The lane's "ideally with the filename interpolated" is kept as-is rather than made a new key: the
<action>: <file name>shape is what the sibling Remove button in the same chip alreadyuses, and it is what distinguishes two images in one chip row — so no thirteenth
catalog entry is needed to get both properties.
Pattern harvest
Rule candidate: a
--max-warnings Nceiling should be driven to 0 and left there, not maintained at N. Every intermediate value needs a re-measurement on each merge, silently absorbs any warning that lands inside it, and makes "did this PR add one?" unanswerable without diffing two JSON reports. At 0 the question is answered by the exit code, and the reviewable unit becomes a one-lineeslint-disable … -- <reason>in the diff rather than a number in a workflow file.Rule candidate: before replacing
anywith a real type, check whether the file is typechecked at all.tsconfig.app.jsonexcludes the whole test tree, sotsc -bvalidates none of it — and that is precisely where mostanylives. The instinct "a wrong type fails the build" is false there. When the compiler is not watching, constrain the diff to type erasure and prove it by comparing transpiler output; that converts an unverifiable change into a verifiable one.Rule candidate:
unknownis not automatically an improvement onany.(globalThis as { ResizeObserver: unknown }).ResizeObserver = xaccepts every value exactly asanydid — the warning goes and the checking never arrives. The review question is "what does this now reject?", not "is the wordanygone?".Not generalizable: enabling
no-evalfor.mjsis specific to a directive whose rule was never configured for that file type. It is worth naming only as a shape — an unused-directive warning can mean the directive is stale, or it can mean the rule is missing coverage, and the two have opposite fixes. Deleting the directive removes a true statement; enabling the rule keeps it and adds enforcement. Reading the suppressed line is the only way to tell which.