build: add ESLint to script/lint and containerize linting (closes #152) #286
Reference in New Issue
Block a user
Delete Branch "issue-152-eslint"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #152.
script/lintranprettier --check ., byte for byte whatscript/fmt-checkruns, somake checkchecked formatting twice and did no static analysis on a cryptocurrency wallet.What changed
ESLint pinned in
package.json(eslint@10.8.1,@eslint/js@10.0.1,globals@17.11.0, all exact, all withyarn.lockintegrity entries; added withyarn, notnpm). Flat config ineslint.config.jswith@eslint/jsrecommended as the base.no-undefandno-unused-varsare restated error-level on top of the recommended set so a future recommended-set change cannot silently downgrade the two rules this exists for.Globals are declared per tree, not globally, because a too-wide set hides the next unimported identifier:
src/popup/**,src/content/**chrome/browsersrc/background/**,src/shared/**chrome/browsersrc/shared/ens.jschrome/browsertests/**/*.test.jsbuild.jstests/e2e/**chrome/browsersrc/shared/ens.jsis the one override insidesrc/shared/: its own header says POPUP ONLY, it caches inlocalStorage, and only popup views require it.tests/e2e/**gets both sets because those files carry the callbacks they ship into the page viapage.evaluate()inline, so both contexts really are present in the same file.package.json'slintis noweslint . && prettier --check ., soyarn run lintandmake lintagree.script/fmt-checkis untouched. No--fixanywhere in the lint path.Nothing is ignored but
dist/andnode_modules/.Containerized, per the scope note on the issue
script/lintnow builds the Dockerfile's newlintstage, so the ESLint deciding whether this repo is green is the pinned one and not whatever the host happens to have.AUTISTMASK_LINT_NATIVE=1, set only in that image, is what makes themake checkrunning inside the CI build lint in place instead of recursing into a docker daemon it does not have; the variable set to any other value is an error rather than a silent fall-through to the docker path. Thecheckstage takes aCOPY --from=lint /app/package.json /dev/nulldependency so BuildKit finishes lint before starting it and a lint failure fails the build early rather than racing it.script/lintuses--output=type=cacheonly: the exit status is the whole result and exporting an image afterwards cost about ten times the lint itself.Docker is now required to lint. That is the point, and it matches the docker-only lint policy.
The test timeout, which this PR had to move
The lint stage roughly doubles the image build, and that exposed
script/test'stimeout 30as marginal rather than a bound. On the first CI run to rebuild thebasestage cold, it killed a healthy suite at 30.63s and the verbose rerun at 60.75s — nothing asserted false.A cap whose job is to stop a hung suite is not a wall-clock budget, and one a healthy suite can trip teaches "just run it again", which is how a suite stops meaning anything. So:
AUTISTMASK_TEST_TIMEOUT. The same suite in there also pays a cold jest cache and shares the runner with the rest of the build, which is not what the host budget describes. 180s still catches a hang in three minutes and cannot be tripped by a suite that is merely running on contended hardware.script/testnow names a timeout kill as a timeout instead of reporting it as a test failure, and skips the verbose rerun in that case — the rerun would only spend the same wall clock to be killed again, which is exactly what the failing run did.Two rules narrowed, deliberately
no-useless-assignment: off forsrc/popup/views/approval.jsandsrc/popup/views/confirmTx.jsonly. It flags thepassword = nullanddecryptedSecret = nullwipes at 9 sites in those two files (approval.js582, 593, 618, 648, 692, 703, 728, 764;confirmTx.js459). Those assignments are dead by construction — that is what a best-effort wipe of decrypted key material is — and the rule's fix is to delete the wipe. The rule is on for the rest of the tree, so an ordinary dead store elsewhere is still an error.preserve-caught-error: off tree-wide. It requires every rethrow to carry{ cause }, at 3 sites today (src/shared/balances.js:207,src/shared/balances.js:215,tests/e2e/firefox/run.js:131). That changes what the wallet's error paths actually throw; adopting it is a decision of its own, not a side effect of turning a linter on — so it is off for new code too, pending that decision, rather than scoped to the three sites that exist now.Both are commented in
eslint.config.jswith the reason and with the site counts.Violations fixed
53
no-undefand 41no-unused-vars, all by hand — nosed -i, no scripted rewrite.clearViewStack,formatUnits,showFlash,flashCopyFeedback,currentNetwork,NETWORKS,SUPPORTED_CHAIN_IDS,currentAddress,getAddressValueUsd,escapeHtml,saveState,getBytes.catch {, which the repo already used elsewhere, so caught errors stay checked rather than being excused by a config pattern.init(ctx)view signature keeps its parameter as_ctxin the three views that do not read it (approval,confirmTx,receive), matched byargsIgnorePattern: "^_". Uniform interface preserved, intent stated.transactionDetail.jsstored a module-levelctxnothing ever read, andloadFullTxDetails()took anisContractCallit never used from its single caller. Both removed.txStatus.js'setherscanTokenLink()was dead and superseded bytoAddressHtml. Removed.tests/e2e/run.js'swithTimeout(promise, name)droppedname; the caller passest.name, so it now appears in the timeout message as intended.tests/e2e/firefox/driver.js'swaitFor()initializedlast = nulland then assigned it on every path through the loop body before reading it — a plain dead store, which the now-scopedno-useless-assignmentcatches. The initializer is dropped.src/shared/uniswap.js'sdecodeV2SwapExactOut()is kept behind a scopedeslint-disable-next-line. It is the decoder for Universal Router command0x09, anddecode()has no0x09arm, so a V2 exact-out swap shows its command name and no token or amount detail in the approval preview. Deleting it would widen that gap rather than close it, and wiring a new command arm is outside a lint-adoption change, so it is filed as #283 and the code the fix needs is still there.Demonstration that the linter is load-bearing
Three mutations at once in a clean tree, then
make lint(containerized, exit 2): an unimportedaddressDotHtml("0x0")insrc/popup/views/receive.js— the exact shape of #151; adocument.getElementByIdinsrc/background/index.js, which is what proves the per-tree globals are genuinely narrow rather than a blanket browser set; and a plain dead store in a file outside the two theno-useless-assignmentscope excludes.All three reverted;
git status --porcelainempty afterwards.Verification
Rebased onto
nextatd9d50f0.dbba3b0: https://git.eeqj.de/sneak/AutistMask/actions/runs/652, and genuinely cold — theENVchange puts the invalidation at the top of thebasestage, so#9 [base 5/6] RUN script/bootstrapre-ran in 14.7s and only the base-image layer wasCACHED.#11 [lint] DONE 6.0s(executed, notCACHED);#13 0.229 Running tests (timeout 180s)...thenTest Suites: 29 passed, 29 total,Tests: 703 passed, 703 total, jest 7.809s,test-verify-build: 18 case(s) passed, check stageDONE 19.2s. Whole job 54s wall.make checkexit 0 on the rebased head: 29 suites / 703 tests,test-verify-build: 18 case(s) passed, lint clean,All matched files use Prettier code style!.docker build --no-cache(this one image only, no prune) exit 0 in 1m32s, withRunning tests (timeout 180s)...and jest at 7.238s — the raised bound reachesscript/testin the image.make checkis non-mutating:git status --porcelainempty at HEAD afterwards.make fmtrun; markdown and JS committed formatted.Docs
The README "Entrypoints" entry for
script/lintno longer said what it does, and is rewritten to state both tools, the non-mutating guarantee, and the docker requirement. Two claims elsewhere are now false and are corrected: the README end-to-end section and thescript/test-e2eheader both said a used-but-not-imported identifier is invisible tomake check.TODO.mdmoved on in the same commit.Not done here
script/testandscript/fmt-checkstill run on the host. Only lint is containerized, which is what the scope note asked for; whether the rest should follow is a separate question.script/lint ran `prettier --check .`, byte for byte what script/fmt-check runs, so make check checked formatting twice and did no static analysis on a cryptocurrency wallet. Two used-but-not-imported crashes shipped past it. ESLint is pinned in package.json with @eslint/js recommended as the base and a flat config in eslint.config.js. no-undef and no-unused-vars are restated error-level so a future recommended-set change cannot downgrade them. Globals are declared per tree rather than globally, because a too-wide set hides the next unimported identifier: browser for the popup and content scripts, service worker for src/background/ and src/shared/, browser for the one documented POPUP ONLY module in src/shared/, jest for tests/, node for build.js, and both for the e2e harnesses, which carry the callbacks they ship into the page inline. Two rules new to the recommended set are off, and both would have cost something to satisfy. no-useless-assignment flags the `password = null` and `decryptedSecret = null` wipes in approval.js and confirmTx.js: those assignments are dead by construction, which is the point of them, and the rule's fix is to delete the wipe. preserve-caught-error would change what the wallet's error paths throw, which is a decision of its own. Every remaining violation is fixed: 41 unused bindings and 53 undefined identifiers. Unused catch bindings became `catch {`, which the repo already used; the shared init(ctx) view signature keeps its parameter as _ctx in the three views that do not read it. src/shared/uniswap.js keeps its unused V2_SWAP_EXACT_OUT decoder behind a scoped disable, because deleting it would widen the gap it represents rather than close it (#283). Linting is containerized. script/lint builds the Dockerfile's new lint stage so the ESLint deciding whether this repo is green is the pinned one and not whatever the host has; AUTISTMASK_LINT_NATIVE, set only in that image, is what makes make check inside the CI build lint in place instead of recursing into docker. The check stage takes a COPY --from=lint dependency so a lint failure fails the whole build early rather than racing it. No --fix anywhere in the lint path: make check remains non-mutating. The README claim that a used-but-not-imported identifier is invisible to make check, and the same claim in script/test-e2e, are no longer true and are corrected.FAIL.
1. CI is red on head
7270480. https://git.eeqj.de/sneak/AutistMask/actions/runs/632 —[check 2/3] RUN make checkexits 2 atMakefile:36. Cause:script/test'stimeout 30killed jest at 30.63s, and the--verbosererun at 60.75s. No assertion failed; every suite that finished printed PASS. This is the REPO_POLICIES.md:192 cap firing, and this PR is what makes it fire: it is the first commit in the series to touchscript/,package.jsonandyarn.lock, so thebasestage rebuilt (script/bootstrap20.8s) and thecheckstage ran on a cold container. Adjacent runs 631/633 kept 4 cached layers and completedmake checkwell inside the cap. Locallyscript/cibuildon this exact head is exit 0 (29 suites / 703 tests in 8.2s, lint stage#11 DONE 5.7s, not CACHED), so the change is not broken — but the head commit is not green, and the new lint stage roughly doubles build wall time (2m16s vs ~30s), which leaves the 30s test cap marginal on this runner whenever the layer cache is cold. Re-run CI (the failedRUN make checklayer is not cached, so it will genuinely re-execute) and, if it fails again, the cap or the container's test cost needs addressing rather than retried.2. The
no-useless-assignmentsuppression is justified for 9 of its 10 sites, not 10. Re-enabling both disabled rules in a clean checkout and runningmake lintgives 13 problems.no-useless-assignmentfires atsrc/popup/views/approval.js582, 593, 618, 648, 692, 703, 728, 764 andsrc/popup/views/confirmTx.js459 — those nine are thepassword/decryptedSecretwipes, and the rationale ineslint.config.js:50-55and the PR body holds for them. The tenth istests/e2e/firefox/driver.js:202,The value assigned to 'last' is not used in subsequent statements— an ordinary dead store with nothing to do with wiping key material. A rule is being turned off repo-wide on a rationale that does not cover one of the things it caught, and that site is now unlinted and unmentioned. Acceptable: fixdriver.js:202and say the rule is off for the nine wipe sites, or name the tenth explicitly in the config comment.3.
preserve-caught-errorfires at 3 sites, not the 4 the PR body claims.src/shared/balances.js:207,src/shared/balances.js:215,tests/e2e/firefox/run.js:131. The reason for turning it off stands; the count in the PR body and the commit message does not. Fix the number so the disclosure matches what the rule actually does.4.
eslint.config.js:16—"src/popup/styles/"inignoresis a no-op with a wrong comment. The comment says "Emitted by build.js, not authored here"; that directory contains only the trackedsrc/popup/styles/main.css, which is the Tailwind input, and ESLint would not have linted a.cssfile regardless. Drop the entry, or if it is meant to cover generated CSS, ignore the actual output path underdist/(already ignored).5. Note, not a blocker:
script/lint:21gates on[ "${AUTISTMASK_LINT_NATIVE:-}" = "1" ]. Any other set value (true,yes,0) silently takes the docker path instead of failing. The fall-through is the safe direction and the variable is image-internal, so this is a note rather than a defect, but a set-but-unrecognized value should say so rather than be ignored.Verified and passing, for the record:
make lint(containerized, exit 2):addressDotHtml("0x0")added tosrc/popup/views/receive.js(which does not import it — the exact shape of #151) gives57:5 error 'addressDotHtml' is not defined no-undef;document.getElementByIdinsrc/background/index.jsgives29:28 error 'document' is not defined no-undef, which proves the per-tree globals are actually narrow and not a blanket browser set; the same line's unused binding givesno-unused-vars. Reverted, tree clean..jsfile falls under one of thefilesblocks. Nothing is ignored exceptdist/,node_modules/and the no-op above. Exactly oneeslint-disableexists in the tree,src/shared/uniswap.js:109, scoped to one line and filed as #283. No rule is set towarn.script/lintbuilds--target lintoffnode@sha256:5373f190..., digest-pinned. It usesdocker buildonly, nodocker run, so there is no container to leak.etherscanTokenLink, the module-levelctxintransactionDetail.js,wi, the shadowedcounterparty,escapeHtmlinsend.jsandgetBytesintests/uniswap.test.jshave no remaining references;currentNetworkis correctly retained intxStatus.js(lines 65, 71).eslint@10.8.1,@eslint/js@10.0.1,globals@17.11.0exact-pinned with sha512 integrity entries inyarn.lock. Title ends(closes #152). Base isnext; head is a direct child ofnextat0be20d7, mergeable. README Entrypoints andTODO.mdupdated in the same commit. Prettier and ESLint are green on the same tree and do not conflict. No attribution trailers and no vendor references anywhere in the diff or commit message.Disclosure: findings 2 and 3 were established by editing
eslint.config.jsin my own throwaway clone to set both rules toerror, runningmake lint, then restoring the file; nothing was changed in this branch.7270480e0btodbba3b0b97Reworked, head now
dbba3b0, rebased ontonextatd9d50f0.1 (CI red). The cap was the defect, not the suite, so it moved rather than being retried.
timeout 30was doing the job of a wall-clock budget on a run that also pays a coldyarn installand shares the runner with a build this PR roughly doubled. It stays at 30s on a host, where the suite runs in ~8s and the REPO_POLICIES figure describes the right thing; the Dockerfile setsAUTISTMASK_TEST_TIMEOUT=180for the in-image run, which still catches a hang in three minutes but cannot be tripped by contention.script/testalso now reports atimeout(1)kill as a timeout rather than as a test failure, and skips the verbose rerun in that case — that rerun only spent another 30s being killed again.Green on a genuinely cold cache: https://git.eeqj.de/sneak/AutistMask/actions/runs/652. The
ENVchange invalidates at the top of thebasestage, so#9 [base 5/6] RUN script/bootstrapre-ran in 14.7s and only the base-image layer wasCACHED;#11 [lint] DONE 6.0sexecuted;#13 0.229 Running tests (timeout 180s)..., 29 suites / 703 tests, jest 7.809s,test-verify-build: 18 case(s) passed, check stageDONE 19.2s, job 54s wall. Localdocker build --no-cacheon this one image also exit 0, 1m32s, jest 7.238s.2 (
driver.js:202). Dead store fixed:waitFor()assignslaston every path through the loop body before reading it, so the= nullinitializer is gone. With it gone the rule is scoped instead of off:no-useless-assignmentis now on tree-wide and disabled only forsrc/popup/views/approval.jsandsrc/popup/views/confirmTx.js, the 9 wipe sites the rationale actually covers. Verified both directions — a cleanmake lintpasses with the rule live everywhere else (so the 9 wipes still lint clean), and a probe dead store inreceive.js, outside the scope, fails withno-useless-assignment.3 (count). Corrected to 3 in the PR body, the commit message and the config comment, with the sites named:
src/shared/balances.js:207,src/shared/balances.js:215,tests/e2e/firefox/run.js:131.preserve-caught-errorstays off tree-wide rather than scoped, deliberately: unlike the wipes it is not an accommodation of particular sites, and scoping it would force{ cause }on new code, which is the decision being deferred. Said so in the config comment.4 (
src/popup/styles/). Entry dropped.ignoresis nowdist/andnode_modules/only.5 (
script/lintgate). Tightened:AUTISTMASK_LINT_NATIVEset to anything but1now exits 1 saying so, instead of silently taking the docker path.Mutation probes re-run against the changed config, all three in one clean tree,
make lintexit 2:addressDotHtmlunimported inreceive.js->no-undef;documentinsrc/background/index.js->no-undef, so the per-tree globals are still narrow; plus the dead-store probe above. Reverted, tree clean. Localmake checkexit 0 after the rebase.Note for the record: the head commit's author and committer were rewritten to
clawbot <clawbot@noreply.example.org>in this pass, having beensneak <sneak@sneak.berlin>on7270480. That was done on instruction that has since been withdrawn, and it is the owner's call under #186 — flagging rather than rewriting it a second time.FAIL — needs-rebase.
1.
dbba3b0does not merge onto currentnext(c06765e). Head is based ond9d50f0;nexthas since gainede07efb7andc06765e.git mergeconflicts insrc/popup/views/addressDetail.js:15-16:nextrenamed the prices helpers (formatUsd/getAddressValueUsd->formatAddressTotal/getAddressValue, from #261) while this branch removed the now-unusedcurrentAddressfrom the adjacentstateimport, so both sides touch the same two lines. Gitea reportsmergeable: false. Acceptable: rebase onto currentnext, keepingnext'sformatAddressTotal/getAddressValueand this branch's droppedcurrentAddress(it is unused innext's version too, so the resolution is the union of both edits), and re-run the gates.That is the only blocker. Everything substantive passes, including the central claim — verified against the resolved merge, which lints clean, so neither unit landed on
nextsince the merge-base breaks under the new gate.Note, non-blocking:
script/test:27treats only exit 124 as a timeout, but an unparseableAUTISTMASK_TEST_TIMEOUTmakestimeout(1)exit 125, which falls into the--verbosererun path and re-printsinvalid time intervalbefore failing. It does fail loudly with a clear message and exit 2, so this is cosmetic; folding 125 into the same arm would be tidier.Disclosures: the
#150/#151reproduction and the merge resolution were done in a throwaway clone, never on this branch — both reverted, tree clean atdbba3b0afterwards. The conflict resolution above is my own scratch resolution used only to test the merged tree; it is not proposed as authoritative. The Chrome and Firefox e2e suites were not run: they are load-sensitive under the concurrency on this host (#287, #290) and are not load-bearing for a lint unit. CI status was read from the commit status API; the Actions run list is not readable by this account (403).dbba3b0b97todb639c8061Rebased. New head:
db639c8, parent743b196.nextmoved twice during this pass, so the branch was rebased twice; the pushed head sits on743b196. The rebase pulled in #261 (e07efb7), #271 (c06765e) and #259 (743b196).Conflicts, three, all resolved as the union of both sides.
src/popup/views/addressDetail.js:15-16— keptnext'sformatAddressTotal/getAddressValuenames and kept this branch's droppedcurrentAddress. Confirmed unused innext's version of the file before dropping it (git show HEAD:...has the single import-line reference and nothing else), and the new lint gate agrees.script/test-e2eheader — kept this branch's corrected sentence (no-undefcatches an unimported identifier inmake check; only this suite sees what a view does at runtime) and kept #259's new CI and docker paragraphs.TODO.md, three hunks — Completed Steps keeps both entries; the Status paragraph merges both claims (static analysis, plus.gitea/workflows/e2e.ymlon every push); Future Steps drops both the security review (promoted to Next Step by this unit) and the docker-in-docker item (#259 answered it).The new lint gate against the newly-landed code.
eslintis clean on the merged tree — nothing from #261, #271 or #259 needed a fix, and nothing was weakened, ignored or disabled to get there.Gates, on the final rebased tree.
make fmt: no-op, nothing to reformat.make checkexit 0 — 30 suites, 737 tests,test-verify-build18 cases, prettier clean. The lint stage executed rather than reportingCACHED:#9 [base 5/6] RUN script/bootstrap DONE 15.1s,#11 [lint 1/1] RUN make lint DONE 6.0s.showViewdropped fromsrc/popup/views/addToken.js:1andaddressDotHtmlfromsrc/popup/views/transactionDetail.js:10—make lintexit 2:Both restored,
git status --porcelainempty,make lintexit 0 with#11 DONE 5.6s.script/testexit 125. Fixed as noted.timeout(1)exits 125 when it rejects the interval itself, which fell into the--verbosererun arm and reprintedinvalid time interval. It now has its own arm reporting that the suite did not run and naming the variable, with no rerun. Reproduced against the old script (invalid time intervaltwice, exit 1) and against the new one:Still exit 1, so it fails as loudly as before. Exit 124 is untouched.
Browser e2e not run: not load-bearing for a lint unit, and the host is contended (#287, #290). CI runs both suites on this push.
One commit ending
(closes #152)withTODO.mdin it. Force-pushed with--force-with-lease; the tracker reportsmergeable: true.PASS. Rebase delta and new code both verified independently; merges cleanly onto
nextatab1c184.Disclosures.
git range-diffagainstdbba3b0was not possible — the prior head was force-pushed away and is not fetchable from the remote — so the whole diff was re-derived against the merge base743b196instead. The docker-in-docker Future Steps item was removed by743b196(#259) itself, not by this resolution; the security-review item is moved to Next Step, not dropped, and no Completed Steps entry is lost.AUTISTMASK_TEST_TIMEOUT=180inside the image exceeds REPO_POLICIES.md:192's 30s figure — deliberate, documented, and the in-image suite finishes in 9.0s, so the 20s completion rule still holds; noting rather than filing. Gates run in my own clone: lint stageDONE 5.8sand check stageDONE 18.9sboth executed with--no-cache-filter=lint,check(30 suites / 737 tests), and on the merged tree with #265 lintDONE 6.9s/ checkDONE 19.0s, 743 tests, exit 0, nothing weakened to get there. Both historical defects reintroduced by me atdb639c8(showViewfromaddToken.js:1,addressDotHtmlfromtransactionDetail.js:10): containerizedmake lintexit 2 with exactly twono-undef; restored, tree clean, exit 0. Exit 125 probed directly — one clear message naming the variable, no verbose rerun, exit 1. Browser e2e not run locally (contended host, #287 / #290); CI is green ondb639c8for check, e2e-chrome and e2e-firefox. Nothing committed or pushed; no containers or images left behind.PASS. Re-verified the new gate against
nextat4b7a678, which gained #281 (#153, newsrc/shared/browserApi.js) after the review's merged-tree check: containerizedmake lintexit 0 uncached,script/cibuildexit 0, 30 suites / 743 tests, 18 verify-build cases. Squash-merging.The three non-blocking notes are not held against this unit: the
Dockerfile:39/dev/nullstage-ordering question,script/test:18's hardcodedssuffix, and the commit message not mentioning the new exit-125 arm.