Skip to content

refactor(frontend): burn the eslint warning ceiling to zero - #7569

Merged
bolichen97 merged 1 commit into
mainfrom
refactor/eslint-warning-burndown
Sep 3, 2026
Merged

refactor(frontend): burn the eslint warning ceiling to zero#7569
bolichen97 merged 1 commit into
mainfrom
refactor/eslint-warning-burndown

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Clears all 597 eslint warnings the frontend Lint gate is carrying and turns
that gate into a hard zero.

$ npx eslint src/ --max-warnings 0    # exit 0, 0 problems

The ceiling reads 597, equal to the measured count on main. After this there is
no count to re-measure and no ceiling that can drift above the truth — and
test_eslint_warning_ceiling.py now asserts the ceiling is 0, so lifting it
to 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.js gains coverage rather than losing it, and that is what takes
the count to 0 instead of 1.

src/**/*.mjs is matched by no other config block, so the one .mjs file under
src/crew-ghost-sprite.gen.mjs, a hand-run codegen script — is linted with an
empty rule set. Its // eslint-disable-next-line no-eval, sitting above a real
eval(program), was therefore reported as an unused directive: a warning
clearable only by deleting a true statement and letting a real no-eval violation
return silently later. That is the path that was taken on main (#7753 deleted the
directive and left a comment asserting that no rule reaches a .mjs file), so this
branch enables the rule and restores the directive, replacing the comment the
new config block makes false. Enabling no-eval for .mjs makes the directive
live:

$ # with the directive removed:
  173:5  error  eval can be harmful  no-eval
$ # with it present: clean

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

rule n how it was answered
@typescript-eslint/no-explicit-any 383 a real type; in tests, type-erasure-only
jsx-a11y/* (9 rules) 116 markup fixes; 51 justified suppressions
react-hooks/exhaustive-deps 58 37 deps named, 21 exclusions justified
no-console 21 justified as deliberate diagnostics
@typescript-eslint/no-unused-vars 19 dead bindings removed, calls kept

no-explicit-any. Where a shape was already named, the existing name is used
rather than a new one: Awaited<ReturnType<typeof api.artifactComments>>,
RootState['chat'], typeof fetch, ChatSlot[], and the already-typed sibling
mocks in __mocks__/@radix-ui/ as the pattern for the popover double. In
apps/mochi most were api?.getX?.().then((c: any) => …) — which throws away the
typed seam mochiApi.ts exists to provide; its own header says the
pre-migration (window as any).mochi handle being any was the bug — so the
annotation is deleted and inference supplies the real type. 122 came from one
byte-identical vi.mock('framer-motion') block copy-pasted across 22 sidebar
tests, 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 same
action as the click, <Clickable>/<button> where a <div onClick> was really a
control. The 51 suppressions concentrate in three shapes the rule reads as gestures
but that no keyboard can reach — and each says which:

  • onLoad/onError on <img>/<iframe> — resource lifecycle events, not gestures
  • handlers that only stopPropagation — no action to activate
  • a dialog root's Escape + focus-trap — that is the keyboard path
  • no-noninteractive-tabindex on a scrollport whose content holds no focusable
    child, 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 (useMemo
over commentsQuery.data?.comments, a module-level NO_EVENTS constant). 21 keep
the exclusion with the invariant written out.

Two findings worth a reviewer's attention

1. Test files are typechecked by nothing. tsconfig.app.json excludes
src/test and **/*.test.ts(x):

$ npx tsc -b --listFiles | grep -c "src/test/"
0

So tsc -b cannot catch a wrong type written in a test — which is where most of
the 383 any lived. The answer was to make every test-file typing change
type-erasure-only and prove it by transpiling both revisions and comparing the
emitted JS:

type-erasure check: 173 .ts/.tsx changed; 13 test files differ, all accounted for

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 any was
hiding.

2. A real bug the burndown surfaced, in ChatPage.tsx. revealAppInPanel
guarded its find-pane close on search.isOpen, and renderMessage held that
callback across renders where the find pane opens. A captured stale
isOpen === false skipped the close, and isSidePanelHidden({…, 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 sibling
handlers already do (it only hands focus back if (wasOpen)), which removes the
staleness 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 element
tags, same className literals, same style bodies — because the fix only adds
role / tabIndex / aria-* / id / onKeyDown. Three swap a styled
<div>/<span>/<img> for a real form element, which is the only class of change
here 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.tsx mounts those three so the claim stays
falsifiable. Captured at this commit and at its parent, same URLs, both themes:

before (HEAD~1) after
before dark after dark
before light after light

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 src is
/api/file-raw, which only Electron's own host answers. The box under test is
asserted 40×40 in both revisions, and the capture script fails rather than writing
a frame if it is not. Everything else — the NumberField captions, the
PackInfoHeader captions, the toggle row — is pixel-identical.

Verification

Gate Result
npx eslint src/ --max-warnings 0 exit 0, 0 problems
npx tsc -b clean
npx vitest run (full suite) 1,773 files, 27,860 tests pass
npx jscpd . 0 clones
npm run i18n:check exit 0
catalog parity + all 11 language style guards 42 files, 660 tests pass
type-erasure check 13 test files differ, each accounted for individually
render-surface check 131/148 .tsx identical

Two new catalog keys are translated into all 12 languages. The Korean one
ships both 조사 forms ({{name}}와(과) 채팅 열기) — koStyle.test.ts requires it
because the particle depends on the final consonant of a value not known until
render, and a bare would render Mochi과 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-label that fires but does nothing, or a hardcoded string that would fail the
i18n 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 Gate failed on the first push and it was right to. It is diff-scoped,
so adding id/aria-labelledby to two borderless <input>s made this change
their owner — and both carried outline-none with nothing in its place, which
suppressed the global :focus-visible outline entirely. A keyboard user could not
see either field take focus. The suppressor is dropped so index.css's
outline: 2px solid var(--accent) applies, and the tree-wide backlog the gate
reports 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 0 is a floor, not
a 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/merge against the base at run time, and nothing
re-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 main three times this
week through a different ratchet (ecab0babe, e7f02aeec, c412c2ff9), and a
census-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 against
the merged tree. cd website && npx eslint src/ --max-warnings 0 costs ~35s there
and 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 example

Rebased again onto current main. Base moved from 63a043a7e to the tip; the only
main commit 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.ymlfix(ci): drop a dead eslint directive and re-equal the warning ceiling #7753 re-equalled the ceiling 599 → 597 and rewrote the surrounding
    comment. 0 subsumes 597 and is kept; main's comment text is kept where it is
    still true, and every sentence naming a count is gone. Exactly one
    --max-warnings literal remains, which test_eslint_warning_ceiling.py pins —
    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-bearing
    one. 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-eval directive above eval(program). That was correct on main, where
    src/**/*.mjs matches no eslint.config.js block and so lints against an empty
    rule set. It is not correct under this PR, whose config block makes no-eval
    live for .mjs: without restoring the directive, this branch turns main's tree
    into a hard error (eval can be harmful no-eval), not a warning. The
    directive is restored with a stated reason, and main's now-false comment
    (asserting no rule reaches a .mjs file under src/) is replaced by one
    describing the live directive.
  • MarkdownRenderer.tsx — the branch's rewrite of the InlineCode chip's
    suppression rationale is dropped; only the two exhaustive-deps fixes remain.
    The role/tabIndex/onKeyDown and the no-noninteractive-element-to-interactive-role
    directive with its -- reason were already in the merge base, so main owns
    them 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-line
    comment above the row <div>, whose role="presentation" and rationale comment
    were 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.tsx and
issue-radar/context.tsx all differ in content at the two heads only because
main edited 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-deps warning in ArtifactsPage.tsx while the gate said 603, so main
measured one over its own ceiling and every open PR touching website/** went red
on a warning its diff never wrote. #7511 is why main's own runs did not report it
first, 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.tsx is already clean on
main, so this removes the number rather than restoring it.

docs/ci/ci-and-reviews.md is updated in the same commit, since it documented the
job as a ratchet baseline. test/test_eslint_warning_ceiling.py — which exists to
stop 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's
suggestion 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 literal 0 would
ban exactly that accurate sentence, and would fire on any unrelated digit in a
max-warnings line. So the docs bullet now names --max-warnings 0, which is the
first time this file has been able to.

Responses to the review

runModel.ts — fixed, and the reviewer was right. First Principles flagged that
the any → unknown conversion there had added runtime coercion (asNumber(spent) ?? 0,
a missing agent_id folding to ''), which changes what a malformed workflow event
renders. For well-formed events — the actual backend contract — behaviour was
identical, and arguably the coercion was an improvement since it removed a NaN
path. 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 any did:

runModel.ts emitted JS identical to HEAD~1: true

The ChatPage.tsx close() change does want a human eye, as flagged — it is the
one 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 own
close() documents this exact call pattern as the supported one:

// 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.current

So an unconditional close() with nothing open is a no-op by design, not a
tolerated accident. The alternative — suppressing exhaustive-deps and keeping the
search.isOpen guard — 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 new
image-open button was labelled ${preview}: ${item.name} while the three sibling
openLightbox controls in ChatPanel.tsx say open_image; worse, preview is
also the name of a different control in that same file (api.previewFile). It now
uses open_image, so one operation has one name. The lane's "ideally with the file
name 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 already
uses, 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 N ceiling 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-line eslint-disable … -- <reason> in the diff rather than a number in a workflow file.

Rule candidate: before replacing any with a real type, check whether the file is typechecked at all. tsconfig.app.json excludes the whole test tree, so tsc -b validates none of it — and that is precisely where most any lives. 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: unknown is not automatically an improvement on any. (globalThis as { ResizeObserver: unknown }).ResizeObserver = x accepts every value exactly as any did — the warning goes and the checking never arrives. The review question is "what does this now reject?", not "is the word any gone?".

Not generalizable: enabling no-eval for .mjs is 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.

@bolichen97
bolichen97 requested a review from a team September 1, 2026 08:23
@bolichen97
bolichen97 requested a review from a team as a code owner September 1, 2026 08:23
@bolichen97
bolichen97 requested a review from CrysisDeu September 1, 2026 08:23
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 93fab2d1294bfdccd870636f1bf3b0ea5212a511 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks confirm the commit message's claims are backed by the diff: the gate flips to --max-warnings 0 with a test pinning zero, the .mjs config gap is closed narrowly, suppressions carry per-case invariants, behavior-affecting swaps (a11y markup, focus outlines) are documented and falsified via the pre-existing website/capture/ + temp-screenshots/ conventions, and the deps fixes sampled show stabilize-then-depend rather than blind list-appending.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 93fab2d1294bfdccd870636f1bf3b0ea5212a511 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. The outline-none removals are deliberate keyboard-focus restorations explained in the commit; screenshots prove pixel-identity for the markup swaps; new labels (Open image: <file>, Open chat with {{name}}) name outcomes, reuse existing vocabulary, and ship in all 12 locales; every new key handler fires the same action as its click. The dep-list work fixes real stale-UI bugs rather than introducing them. Nothing survives the kill-filter.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 93fab2d1294bfdccd870636f1bf3b0ea5212a511 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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"); website/capture/ (169 files) and temp-screenshots/ (672 files) are established conventions; the two catalog keys are required by the documented i18n invariant; the behavior changes are each the honest resolution of a specific warning and the largest one is declared in its own description section.

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 ships

Intent: make the frontend lint gate reject any new warning by clearing all 597 — a FIX (of gate slack and lint debt).

  1. CI lint gate now fails on any frontend warning (ceiling 597 → 0) — justified
  2. test_the_ceiling_is_zero pins the value; prose-transcription check goes dormant at zero — justified
  3. Hundreds of warnings fixed in place: real types substituted, dead bindings removed — justified
  4. Keyboard/screen-reader users can now operate controls; 3 components swap styled div/span/img for label/button — justified
  5. 51 one-line suppressions, each carrying its reason in the diff — justified
  6. .mjs under src/ now linted for no-eval; the codegen script's directive restored live — justified
  7. "Open app" click while the find pane is open no longer opens a tab behind the hidden dock — rides along, declared
  8. Superseded error card no longer offers a stale Continue (renderMessage deps) — rides along, same rule
  9. Two new catalog strings ("Open image", "Open chat with {{name}}") in 12 languages — justified (i18n invariant)
  10. New capture page + 4 committed before/after screenshots — justified (169- and 672-file existing conventions)

Watch

Items 7–8 are user-visible behavior fixes shipping under a refactor: subject — fully declared in the description, but invisible in git log --oneline, which is what the changelog process reads at release time.

[FIRST-PRINCIPLES-REVIEWED] 93fab2d

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 93fab2d1294bfdccd870636f1bf3b0ea5212a511 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 93fab2d

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 93fab2d1294bfdccd870636f1bf3b0ea5212a511: <one-sentence reason>

@bolichen97
bolichen97 force-pushed the refactor/eslint-warning-burndown branch from d1e685c to 472788d Compare September 1, 2026 08:31
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 93fab2d1294bfdccd870636f1bf3b0ea5212a511 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 93fab2d

Verdict parsed from the review's SHA-scoped output markers for commit 93fab2d1294bfdccd870636f1bf3b0ea5212a511.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 93fab2d1294bfdccd870636f1bf3b0ea5212a511: <one-sentence reason>

@bolichen97

bolichen97 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Backend Tests (3.10, 3) and (Windows) (3) are red on main, not on this branch

Both fail on
test/test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor.
I verified it is not this diff by running it on a throwaway worktree at pristine
origin/main:

$ git worktree add /tmp/probe origin/main
$ cd /tmp/probe && PYTHONPATH=src python -m pytest test/test_security_posture.py -k gate_side_log
1 failed, 29 warnings          # identical on this branch and on pristine main

The assertion is dashboard/handlers/memory.py: 2 sites, census says 0 — a
cross-merge, not a regression: one commit added the log-site census, another added
two redact_and_truncate calls in a file the census never listed, and neither PR's
CI could see the other's half. It lands in shard 3 of 4, so it is failing every open
PR that touches that shard. #7572 fixes it; this branch needs no change and I will
rebase once it lands.

This diff is frontend-only (website/**, one workflow line, four screenshots) and
touches no Python, so it cannot reach that test. Everything frontend is green:
Frontend Lint & Type Check (the gate this PR moves), all four Frontend Tests
shards, Frontend Coverage Merge, Focus Cue Gate, Screenshot Evidence,
PR Readiness, and the Design / UX / First Principles / Code Review lanes.

Worth noting for its own sake: this failure is the same shape as the cost this PR
documents in "The one cost of a hard zero" — a ratchet broken by a combination
neither contributing PR could observe. It is the live example of why the
push: [main] post-merge job described there is worth having.

Update — a second, different red, and it is also not this diff.
Backend Tests (Windows) (2) failed on
test/test_irq.py::test_an_entry_joining_after_a_partial_fire_serves_its_own_floor
(AssertionError: assert False).

That is a timing flake, not a regression. The test drives a coalescing window with
_settle() between _verdict() calls and asserts that a late joiner does NOT
inherit the window's age — so a runner slow enough for _COALESCE to elapse between
two calls closes the window early and the Skip becomes a Report. The whole file
passes locally, twice, on this branch:

$ PYTHONPATH=src python -m pytest test/test_irq.py -q
60 passed in 3.52s

I saw the same test fail once on a Linux shard earlier today on an unrelated
frontend-only branch, which is the pattern of a shared-runner timing flake rather
than of anything either branch changed.

So the four red jobs are two independent causes, neither reachable from a diff that
touches no Python:

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.

@bolichen97
bolichen97 force-pushed the refactor/eslint-warning-burndown branch from 472788d to 344fdd5 Compare September 1, 2026 08:56
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
pepmach
pepmach previously approved these changes Sep 1, 2026
@bolichen97
bolichen97 force-pushed the refactor/eslint-warning-burndown branch from 344fdd5 to bf36f58 Compare September 1, 2026 23:56
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@bolichen97

bolichen97 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Backend Tests (… , 3) + Coverage Gate: still inherited from main after the rebase

Rebased onto a492b653c and re-checked, because the earlier guess at the cause was wrong.

The failing test is unchanged:

test/test_security_posture.py::TestGateSideLogRedactorSpelling
  ::test_no_new_gate_side_log_line_reads_the_baseline_redactor

AssertionError: New gate-side log/audit line(s) reading the BASELINE redactor
  slack/gateway.py: 7 sites, census says 6

Two things worth recording:

  • The earlier hypothesis is disproven. It was attributed to a merge-base predating
    ebc0936f2. That commit is now an ancestor of this branch, and the test still fails —
    so ebc0936f2 was never the fix for this.
  • The real cause is on main and not in this diff. Both files involved are
    byte-identical to origin/main:
    git diff --quiet origin/main -- src/kiro_crew/slack/gateway.py test/test_security_posture.py
    exits 0. The assertion is a whole-tree census, not a diff-scoped gate, so it fails the same
    way on pristine main. A gate-side log site was added to slack/gateway.py on main
    without raising _BASELINE_LOG_SITE_CENSUS, and clearing it means either routing that site
    through redact_log_via_context or bumping the census with a stated reason — a backend
    change, in a file this PR does not touch.

This PR's diff contains zero Python files (195 files: 186 website/src, 2
website/capture, 4 screenshots, website/eslint.config.js, .github/workflows/ci.yml,
docs/ci/ci-and-reviews.md), so it cannot move a backend shard either way. Coverage Gate reads only
backend-test=failure -- failing closed. and clears when the shard does.

The gate this PR actually asserts is green on the rebased tree: npx eslint src/ --max-warnings 0 exits 0 with 0 problems, npx tsc -b clean, npx jscpd . 0 clones,
npm run i18n:check exit 0, and the focus-cue gate exits 0 with the tree-wide backlog at 5.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

Audit note — #7745 is being closed in favour of this PR

You are the surviving implementation; #7745 is being closed.

What the two shared

Verified 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 const s = await shell() into await shell(), and that exact hunk with the same index line (a67439310f..120a8d29e1) sits inside #7569's 194-file diff. git rev-parse on the file gives origin/main a67439310f (the binding is still there), pr/7569 120a8d29e1 and pr/7745 120a8d29e1 -- identical post-images. The purpose is identical on both sides too: remove that one @typescript-eslint/no-unused-vars warning from the count the frontend-lint --max-warnings gate measures. Not a stacked branch: one commit each (c733254 / b855992), different merge-bases (63a043a / 13b0d28), neither ref an ancestor of the other. The relationship is asymmetric -- #7569 landing leaves #7745 with a literally empty diff, #7745 landing leaves #7569 with 193 files and the ceiling change -- which fails the duplicate symmetric bar and meets functional overlap's 'one is a strict superset of the other'. It is not independent (merely co-located), because this is not one file touched twice for independent reasons; it is the same single-line edit made for the same reason producing the same bytes. the first adjudication's ruling and its nomination direction both hold: #7569 does not refuse the capability #7745 exists to deliver, it contains it and 598 more. But #7569 cannot land as it stands, so it is a REBASE rather than a KEEP: its ci.yml hunk deletes a --max-warnings 599 line that no longer exists (main reads 597), its eslint.config.js src/**/*.mjs block exists to make live an eslint-disable-next-line no-eval directive that merged #7753 (4593b7d) deleted from crew-ghost-sprite.gen.mjs, main's FindingRow.tsx:40 already carries the role="presentation" it re-suppresses, its replacement ceiling comment re-adds the SHA-and-PR narration #7753 stripped as AGENTS.md-forbidden, and both human reviews are DISMISSED with the Design lane asking that the ChatPage.tsx close() fix be split into its own fix: PR.

What #7745 had that this PR does not

Please 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': const s = await shell() -> await shell(), both heads resolving to blob 120a8d29e1). Two non-code items to carry before closing #7745: (1) its Pattern-harvest rule candidate -- a count-pinned ratchet (--max-warnings N, baseline ceilings) should also run on a scheduled workflow against main, so a path-filter skip window cannot hide a regression until an unrelated PR inherits it. #7569's body argues the adjacent but different push: [main] post-merge job, so this belongs in whichever PR lands or in a follow-up issue. (2) A conditional: if #7569 is split per the Design lane's request and the split drops the test-file hunk, that one-line removal must be re-made PAIRED with --max-warnings 596, because on today's main the binding is counted inside the green 597 and removing it alone opens the slack the gate's own comment forbids.

This PR still needs work: REBASE

Neither side has merged -- the issue/PR reference check and the open-PR list both show #7569 and #7745 OPEN, so superseded by work already on main does not apply between them. Landed work does however dissolve #7745's stated urgency: merged 4593b7d (#7753) re-measured the gate to --max-warnings 597 with the unused binding still present (git rev-parse 4593b7d:website/src/test/issueRadarNarrowViewport.test.ts = a67439310f, same as main today), so frontend-lint is green on main and #7745's premise that 'no frontend PR can go green' is stale. That is partial coverage of #7745's motivation, not of its diff: the binding itself is still on main, which is precisely why #7569 still carries the hunk.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

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.
@bolichen97
bolichen97 force-pushed the refactor/eslint-warning-burndown branch from 8b7e5d5 to 93fab2d Compare September 2, 2026 20:23
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • The ChatPage.tsx unconditional close() and the dependency-array edits ride under a refactor: title — rebutted, and the demanded remedy rejected

The ChatPage.tsx unconditional close() is a user-visible bug fix, and the 60 dependency-array edits change callback identity and effect re-run timing — none of which the PR's own proofs (type-erasure diff, render-surface diff, pixel diff) can see; they verify markup and types, not scheduling. A regression there bisects to a 194-file commit titled refactor and can't be reverted without dragging the whole lint pass back. Extracting the close() fix (and ideally the dep-array hunks in ChatPage.tsx) into its own fix: PR would make this PR provably behaviour-neutral and the fix independently revertable.

The mechanism the lane names is real and the proofs it lists genuinely cannot see it. The
remedy is what does not survive contact with the code, and it is rejected for a reason
the lane's own sibling lane already stated: the close() change is not separable from the
exhaustive-deps fix, so extracting it means shipping a suppression around a live bug.

Why it is inseparable, at the site. Before this change the handler read search.isOpen
through a stale closure, and the exhaustive-deps fix is what removes that guard's capture.
Keeping the guard requires suppressing the rule and writing a rationale comment that
enshrines the stale read — First Principles put it as:

They are inseparable from the dep-list corrections (deferring them means shipping a suppression around a live bug), so the subject line, not the code, is what a human should weigh.

So there is no ordering in which a fix: PR lands first. A fix: PR containing only the
unconditional close() would still carry the dep-array edit that makes it necessary, i.e.
it would be this hunk under a different title, and the remaining lint PR would then have to
suppress the rule at that exact line and be re-reviewed for it.

Why the residual risk is bounded rather than merely asserted. close() with nothing open is
a total no-op, which is a property of useMessageSearch and not of this PR —
website/src/hooks/useMessageSearch.ts is untouched here, its close() is a useCallback([]),
isOpen === false implies term === '', and its own comment names this call pattern as the
supported one:

// 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.current

The correctness lane reached the same conclusion independently at this head (Opus 4.8: no
blocking findings, having read the focusComposer() gate), and the re-render half was
answered in detail one head earlier — the added entries are booleans, ints, and useCallbacks
whose own dependency arrays are subsets of the enclosing list, so none can change identity on
a render that list survives.

What did change in response. The lane's point about the title is the part with no code
answer, and it is not waved away: the subject stays refactor: because the commit's dominant
content is a lint pass, and the two behaviour-changing hunks are named explicitly in the
commit body and in the PR body's "this is the hunk to look at if you look at only one". That
is the smallest thing that preserves the reviewability the lane is asking for without
splitting a change that cannot be split. If a maintainer would rather have the subject read
fix(frontend):, that is a one-line retitle and I will make it — say so and it is done.

No code change for this item.

@bolichen97

bolichen97 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author
  • NEVER raise this number is prose enforcement; make the ratchet mechanical — fixed

The NEVER raise this number comment is prose enforcement; a one-line CI grep asserting --max-warnings 0 in the workflow (like test_eslint_warning_ceiling.py does for the count) would make the ratchet mechanical instead of exhortative.

Legitimate, and the mechanism is exactly as described: eslint exits 0 under a lifted
ceiling, so nothing in CI reports a raise. The comment was the only thing standing
between the hard zero and a future "unblock the fleet" bump — which is not hypothetical
on this gate, as the 603 → 604 raise in the PR body records.

Fixed at head 93fab2d1294bfdccd870636f1bf3b0ea5212a511 as
test_eslint_warning_ceiling.py::test_the_ceiling_is_zero:

  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 ci.yml's eslint ceiling is 12, not 0 when the
literal is lifted, and the whole module passes on the branch as pushed (56 passed with
test_security_posture.py).

Two notes on where it went, since the lane suggested a workflow grep:

The test file, not a shell grep in ci.yml. That file already owns this gate's
single-source invariant (test_the_lint_gate_declares_a_ceiling refuses a second
--max-warnings literal, so a burn-down cannot leave one behind), and a grep living
inside the workflow it checks can be edited in the same hunk that raises the ceiling.
Keeping both halves in one module outside .github/ means the value and the assertion
cannot move together by accident.
The sibling transcription check is narrowed in the same commit, because the hard
zero changes what it is for. It flagged any doc line containing both max-warnings and
the ceiling's current value — correct while the value was a measurement that could
drift, but at "0" it bans the accurate sentence npx eslint src/ --max-warnings 0
and fires on any unrelated digit in a max-warnings line. It now returns early at
zero, with the reason written into its docstring: the value is held by the assertion
above rather than by a measurement, so it has nothing left to go stale. ci-and-reviews.md
names --max-warnings 0 for the first time as a result.

Also fixed in the same commit, from the UX lane's suggestion: PendingAttachments.tsx's
image-open button now takes the open_image accessible name its three sibling
openLightbox controls use, instead of preview — which names a different control
(api.previewFile) in the same file.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • PendingAttachments.tsx labels its image-open button differently from the three sibling openLightbox controls — fixed

PendingAttachments.tsx labels its image-open button "${preview}: ${item.name}" while the three sibling controls in ChatPanel.tsx doing the identical openLightbox action say open_image ("Open image") — unify on one term, ideally open_image with the file name interpolated, so a screen-reader user hears the same operation named the same way (and can tell multiple images in one transcript apart).

Legitimate, and sharper than stated: apps.mochi.chatPanel.preview is not merely a
different word for the same thing — it is the accessible name of a different control
in the same file, ChatPanel.tsx:1859's api.previewFile button. So the new button was
borrowing another action's name, which is worse than an inconsistency.

Fixed at head 93fab2d1294bfdccd870636f1bf3b0ea5212a511:

aria-label={`${i18nT('apps.mochi.chatPanel.open_image')}: ${item.name}`}

preview keeps its remaining consumer, so no key is orphaned, and none is added: the
lane's "ideally open_image with the file name interpolated" is satisfied by the
<action>: <file name> shape the sibling Remove button in the same chip already uses
(Remove: b.png, pinned by mochiPendingAttachments.test.tsx). That keeps both properties
the lane asked for — one name per operation, and per-image disambiguation — without a
thirteenth catalog entry or a change to the three existing call sites.

Gates re-run on the branch as pushed: eslint src/ --max-warnings 0 exit 0, tsc -b
exit 0, npm run i18n:check exit 0, and the mochi vitest specs 679 passed / 13 files.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Items 7-8 are user-visible behavior changes shipping under a refactor: subject — re-anchored to this head; the code half stands, the subject half answered below

Items 7–8 are user-visible behavior changes shipping under a refactor: subject. AGENTS.md's changelog rule says omissions are systematic when "a change whose subject names one subsystem" touches another — these two fixes will be invisible to the release-notes scan. They are inseparable from the dep-list corrections (deferring them means shipping a suppression around a live bug), so the subject line, not the code, is what a human should weigh.

This is the same finding the lane raised at bf36f587b5e466b546cd7485c8f1ef2e946b0311, where
the code half was answered in detail and is not restated here: see
#7569 (comment) — the added
dependencies are booleans, ints, and useCallbacks whose own dependency arrays are subsets
of the enclosing list, so none can churn renderMessage; and the asymmetry is what makes the
trade correct (a missing dependency is a stale closure and a live bug, a surplus one costs
a re-render of an already-memoised row).

Re-anchoring it here because that record names an older head and the lane has since re-judged
at c733254537402c6500d2c5725cc2ffb19d6cefa2, so the concern otherwise reads as unanswered.

What is genuinely new, and was not answered before, is the sentence the lane ends on: the
subject line is what a human should weigh.
That has no code answer, so here is the position
rather than a deflection.

The subject stays refactor(frontend): for one reason — the commit's dominant content is a
lint pass across 195 files, and a fix: subject would misdescribe 193 of them as much as
refactor: under-describes two. What the lane is really protecting is that the two hunks not
be invisible, and that is addressed where a reader will actually look:

  • the commit body names both, at the site, with the invariant each depends on;
  • the PR body carries them under "Responses to the review" and ends the close() paragraph
    with "this is the hunk to look at if you look at only one";
  • and the release-notes risk the lane cites is mitigated by the same text: AGENTS.md's rule is
    that coverage is verified against the commit range rather than a keyword scan, and this
    commit's body states both behaviour changes in prose a range walk will read.

If a maintainer weighs it the other way, the remedy is a one-line retitle to
fix(frontend): — no code moves and the hygiene gate accepts either prefix. Say so and I
will make it. I am not making it unilaterally because it would then misdescribe the other
193 files, and that trade is exactly the judgement the lane says belongs to a human.

No code change for this item.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Re-rebased onto the current main tip, and re-measured there

Head is now 93fab2d1294bfdccd870636f1bf3b0ea5212a511, one commit on
a030091b405ce206aaa7a2a3cb05ef2fca7c7a3c. The earlier "Green at c733254…" note above
still describes that head accurately; this records what moved since, because the base
advanced three times while the review ran (1a765b88c0e6e460e0a030091b4).

The decisive gate, re-run on the final base: npx eslint src/ --max-warnings 0 exits
0 with empty output. That is the whole risk of this PR — a ceiling of 0 must also cover
whatever main landed underneath — and a030091b4 (#7958) does touch a website/ file
(apps/crew-companion/pet.tsx), so it was re-measured after that rebase specifically rather
than carried over.

Also re-run on the final base, all exit 0: npx tsc -b; npm run jscpd (0 clones);
npm run i18n:check; npm run lint:phantom-classes --test and the gate itself, both scoped
to the new base; test/test_eslint_warning_ceiling.py + test/test_security_posture.py
(56 passed, so the shard-3 redactor census ratchet is clean on this base);
scripts/check_black_formatting.py, flake8, isort on the touched Python file; and
scripts/docs-lint.sh. Vitest for the specs covering the touched files: 41 files / 714 tests
and 13 files / 679 tests, all passing.

What the rebase changed in this branch's own patch, derived by diffing the patch at the
previously-reviewed head against the patch here rather than from the raw two-head diff (which
also contains main's intervening commits and is therefore useless for this question). Four
files, all documented in the PR body's rebase section: ci.yml (#7753 re-equalled 599 → 597;
0 subsumes it and main's comment text is kept where still true),
crew-ghost-sprite.gen.mjs (newly in the diff — #7753 deleted the no-eval directive,
which this PR's config block would turn into a hard error, so it is restored with a stated
reason), and MarkdownRenderer.tsx / FindingRow.tsx (prose-only hunks dropped, because
the directives and rationale they were lengthening were already in the merge base and are
main's to own — a burn-down whose claim is that every hunk derives from a warning it clears
should not carry a comment edit on a line it does not otherwise change).

Three earlier bullets in that section have been corrected rather than reworded, because
they were wrong and a reviewer would have burned a round failing to verify them:
Composer.tsx was never in this PR's diff at all; #7768's role="presentation" landed
before the branch point, so main did not add it "while this was open"; and the claimed
"the directive became unused" in FindingRow.tsx is not reproducible — that hunk contained no
directive.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 3, 2026 03:18

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 5970 in the same commit (.github/workflows/ci.yml), and test/test_eslint_warning_ceiling.py gains test_the_ceiling_is_zero, which asserts the ci.yml value is exactly 0 (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.switchAgent reads none of the removed installedAgents/provider/queryClient (only the comment names them); usePanelTabs.openFile calls update(...), so [upsert][update] corrects a stale dep; KiroCrewAgentsPage edit-opener reads only its arg a.*, 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/""/false semantics change.
  • 21 exhaustive-deps disables: 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/destroy async was pre-existing (only anySiteMutationResp changed); the rest are => void type annotations, test code, or the save![1]!.body! test mock-inspection. No new promise swallowing or reordering on a production path.
  • no-eval config block (src/**/*.mjs): enables no-eval so the existing directive over a real eval() in the codegen script becomes a reviewed, live exemption; a second eval() would now error.

Non-blocking observations (advisory, no change required)

  1. eslint.config.js: the new src/**/*.mjs block is deliberately one rule wide (no no-unused-vars/no-undef for that glob). The comment already flags this as a known gap to widen when a second .mjs lands — noting it, not asking for a change.
  2. The subscribe-once exhaustive-deps suppressions (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 0 CI 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.

@bolichen97
bolichen97 merged commit c71ec9d into main Sep 3, 2026
65 checks passed
@bolichen97
bolichen97 deleted the refactor/eslint-warning-burndown branch September 3, 2026 03:33
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 3, 2026
CrysisDeu pushed a commit that referenced this pull request Sep 4, 2026
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
NicholasRBowers pushed a commit that referenced this pull request Sep 4, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants