sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

build: add ESLint to script/lint and containerize linting (closes #152) #286

Open
clawbot wants to merge 1 commits from issue-152-eslint into next
pull from: issue-152-eslint
merge into: sneak:next
Conversation 0 Commits 1 Files Changed 32 +647 -83
clawbot commented 2026-08-14 06:18:46 +02:00
Collaborator
Copy Link

Closes #152.

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.

What changed

ESLint pinned in package.json (eslint@10.8.1, @eslint/js@10.0.1, globals@17.11.0, all exact, all with yarn.lock integrity entries; added with yarn, not npm). Flat config in eslint.config.js with @eslint/js recommended as the base. no-undef and no-unused-vars are 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:

tree globals
src/popup/**, src/content/** browser + chrome/browser
src/background/**, src/shared/** service worker + chrome/browser
src/shared/ens.js browser + chrome/browser
tests/**/*.test.js node + jest
build.js node
tests/e2e/** node + browser + chrome/browser

src/shared/ens.js is the one override inside src/shared/: its own header says POPUP ONLY, it caches in localStorage, and only popup views require it. tests/e2e/** gets both sets because those files carry the callbacks they ship into the page via page.evaluate() inline, so both contexts really are present in the same file.

package.json's lint is now eslint . && prettier --check ., so yarn run lint and make lint agree. script/fmt-check is untouched. No --fix anywhere in the lint path.

Containerized, per the scope note on the issue

script/lint now 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 happens to have. AUTISTMASK_LINT_NATIVE=1, set only in that image, is what makes the make check running inside the CI build lint in place instead of recursing into a docker daemon it does not have. The check stage takes a COPY --from=lint /app/package.json /dev/null dependency so BuildKit finishes lint before starting it and a lint failure fails the build early rather than racing it. script/lint uses --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.

Two rules narrowed, deliberately

  • no-useless-assignment: off. It flags the password = null and decryptedSecret = null wipes in approval.js and confirmTx.js (10 sites). 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.
  • preserve-caught-error: off. It requires every rethrow to carry { cause } (4 sites). 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.

Both are commented in eslint.config.js with the reason.

Violations fixed

53 no-undef and 41 no-unused-vars, all by hand — no sed -i, no scripted rewrite.

  • Unused imports, including the whole tail the issue enumerates: clearViewStack, formatUnits, showFlash, flashCopyFeedback, currentNetwork, NETWORKS, SUPPORTED_CHAIN_IDS, currentAddress, getAddressValueUsd, escapeHtml, saveState, getBytes.
  • Unused catch bindings became catch {, which the repo already used elsewhere, so caught errors stay checked rather than being excused by a config pattern.
  • The shared init(ctx) view signature keeps its parameter as _ctx in the three views that do not read it (approval, confirmTx, receive), matched by argsIgnorePattern: "^_". Uniform interface preserved, intent stated.
  • transactionDetail.js stored a module-level ctx nothing ever read, and loadFullTxDetails() took an isContractCall it never used from its single caller. Both removed.
  • txStatus.js's etherscanTokenLink() was dead and superseded by toAddressHtml. Removed.
  • tests/e2e/run.js's withTimeout(promise, name) dropped name; the caller passes t.name, so it now appears in the timeout message as intended.
  • src/shared/uniswap.js's decodeV2SwapExactOut() is kept behind a scoped eslint-disable-next-line. It is the decoder for Universal Router command 0x09, and decode() has no 0x09 arm, 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

foo() added to renderTotalValue() in src/popup/views/home.js, then make lint:

#11 [lint 1/1] RUN make lint
#11 0.169 Linting...
#11 0.310 yarn run v1.22.22
#11 0.339 $ eslint . && prettier --check .
#11 1.748
#11 1.748 /app/src/popup/views/home.js
#11 1.748   49:5  error  'foo' is not defined  no-undef
#11 1.748
#11 1.748 ✖ 1 problem (1 error, 0 warnings)
#11 1.748
#11 1.832 error Command failed with exit code 1.
#11 2.106 make: *** [Makefile:27: lint] Error 1
#11 ERROR: process "/bin/sh -c make lint" did not complete successfully: exit code: 2
make: *** [Makefile:27: lint] Error 1

And the same probe through script/cibuild, to show the gate holds for CI and not just for make lint:

#11 2.591   49:5  error  'foo' is not defined  no-undef
#11 2.591 ✖ 1 problem (1 error, 0 warnings)
#11 2.921 make: *** [Makefile:27: lint] Error 1
#11 ERROR: process "/bin/sh -c make lint" did not complete successfully: exit code: 2
ERROR: failed to build: failed to solve: process "/bin/sh -c make lint" did not complete successfully: exit code: 2
CIBUILD_EXIT=1

The probe was reverted; git diff on src/popup/views/home.js in this branch shows only the flashCopyFeedback import removal.

Verification

Rebased onto next at 0be20d7 (which landed src/popup/viewRouter.js, a file the linter had never seen), then re-run:

  • make check exit 0: 29 suites / 703 tests passed, test-verify-build: 18 case(s) passed, lint clean, All matched files use Prettier code style!. The lint layer executed rather than reporting CACHED — the run is timestamped #11 0.170 Linting... through #11 7.405 All matched files use Prettier code style!.
  • make check is non-mutating: git diff HEAD hashed identical before and after, and git status --porcelain empty at HEAD.
  • script/cibuild exit 0 on the restructured multi-stage Dockerfile, through make check, make build and verify-build: 4 bundle(s) verified autistmask-build-debug=off.
  • make fmt run; markdown and JS committed formatted.

Docs

The README "Entrypoints" entry for script/lint no 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 the script/test-e2e header both said a used-but-not-imported identifier is invisible to make check. TODO.md moved on in the same commit.

Not done here

  • script/test and script/fmt-check still 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.
Closes https://git.eeqj.de/sneak/AutistMask/issues/152. `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. ## What changed ESLint pinned in `package.json` (`eslint@10.8.1`, `@eslint/js@10.0.1`, `globals@17.11.0`, all exact, all with `yarn.lock` integrity entries; added with `yarn`, not `npm`). Flat config in `eslint.config.js` with `@eslint/js` recommended as the base. `no-undef` and `no-unused-vars` are 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: | tree | globals | | --- | --- | | `src/popup/**`, `src/content/**` | browser + `chrome`/`browser` | | `src/background/**`, `src/shared/**` | service worker + `chrome`/`browser` | | `src/shared/ens.js` | browser + `chrome`/`browser` | | `tests/**/*.test.js` | node + jest | | `build.js` | node | | `tests/e2e/**` | node + browser + `chrome`/`browser` | `src/shared/ens.js` is the one override inside `src/shared/`: its own header says POPUP ONLY, it caches in `localStorage`, and only popup views require it. `tests/e2e/**` gets both sets because those files carry the callbacks they ship into the page via `page.evaluate()` inline, so both contexts really are present in the same file. `package.json`'s `lint` is now `eslint . && prettier --check .`, so `yarn run lint` and `make lint` agree. `script/fmt-check` is untouched. No `--fix` anywhere in the lint path. ## Containerized, per the scope note on the issue `script/lint` now 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 happens to have. `AUTISTMASK_LINT_NATIVE=1`, set only in that image, is what makes the `make check` running *inside* the CI build lint in place instead of recursing into a docker daemon it does not have. The `check` stage takes a `COPY --from=lint /app/package.json /dev/null` dependency so BuildKit finishes lint before starting it and a lint failure fails the build early rather than racing it. `script/lint` uses `--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. ## Two rules narrowed, deliberately - **`no-useless-assignment`: off.** It flags the `password = null` and `decryptedSecret = null` wipes in `approval.js` and `confirmTx.js` (10 sites). 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. - **`preserve-caught-error`: off.** It requires every rethrow to carry `{ cause }` (4 sites). 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. Both are commented in `eslint.config.js` with the reason. ## Violations fixed 53 `no-undef` and 41 `no-unused-vars`, all by hand — no `sed -i`, no scripted rewrite. - Unused imports, including the whole tail the issue enumerates: `clearViewStack`, `formatUnits`, `showFlash`, `flashCopyFeedback`, `currentNetwork`, `NETWORKS`, `SUPPORTED_CHAIN_IDS`, `currentAddress`, `getAddressValueUsd`, `escapeHtml`, `saveState`, `getBytes`. - Unused catch bindings became `catch {`, which the repo already used elsewhere, so caught errors stay checked rather than being excused by a config pattern. - The shared `init(ctx)` view signature keeps its parameter as `_ctx` in the three views that do not read it (`approval`, `confirmTx`, `receive`), matched by `argsIgnorePattern: "^_"`. Uniform interface preserved, intent stated. - `transactionDetail.js` stored a module-level `ctx` nothing ever read, and `loadFullTxDetails()` took an `isContractCall` it never used from its single caller. Both removed. - `txStatus.js`'s `etherscanTokenLink()` was dead and superseded by `toAddressHtml`. Removed. - `tests/e2e/run.js`'s `withTimeout(promise, name)` dropped `name`; the caller passes `t.name`, so it now appears in the timeout message as intended. - `src/shared/uniswap.js`'s `decodeV2SwapExactOut()` is **kept** behind a scoped `eslint-disable-next-line`. It is the decoder for Universal Router command `0x09`, and `decode()` has no `0x09` arm, 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 https://git.eeqj.de/sneak/AutistMask/issues/283 and the code the fix needs is still there. ## Demonstration that the linter is load-bearing `foo()` added to `renderTotalValue()` in `src/popup/views/home.js`, then `make lint`: ``` #11 [lint 1/1] RUN make lint #11 0.169 Linting... #11 0.310 yarn run v1.22.22 #11 0.339 $ eslint . && prettier --check . #11 1.748 #11 1.748 /app/src/popup/views/home.js #11 1.748 49:5 error 'foo' is not defined no-undef #11 1.748 #11 1.748 ✖ 1 problem (1 error, 0 warnings) #11 1.748 #11 1.832 error Command failed with exit code 1. #11 2.106 make: *** [Makefile:27: lint] Error 1 #11 ERROR: process "/bin/sh -c make lint" did not complete successfully: exit code: 2 make: *** [Makefile:27: lint] Error 1 ``` And the same probe through `script/cibuild`, to show the gate holds for CI and not just for `make lint`: ``` #11 2.591 49:5 error 'foo' is not defined no-undef #11 2.591 ✖ 1 problem (1 error, 0 warnings) #11 2.921 make: *** [Makefile:27: lint] Error 1 #11 ERROR: process "/bin/sh -c make lint" did not complete successfully: exit code: 2 ERROR: failed to build: failed to solve: process "/bin/sh -c make lint" did not complete successfully: exit code: 2 CIBUILD_EXIT=1 ``` The probe was reverted; `git diff` on `src/popup/views/home.js` in this branch shows only the `flashCopyFeedback` import removal. ## Verification Rebased onto `next` at `0be20d7` (which landed `src/popup/viewRouter.js`, a file the linter had never seen), then re-run: - `make check` exit 0: 29 suites / 703 tests passed, `test-verify-build: 18 case(s) passed`, lint clean, `All matched files use Prettier code style!`. The lint layer executed rather than reporting `CACHED` — the run is timestamped `#11 0.170 Linting...` through `#11 7.405 All matched files use Prettier code style!`. - `make check` is non-mutating: `git diff HEAD` hashed identical before and after, and `git status --porcelain` empty at HEAD. - `script/cibuild` exit 0 on the restructured multi-stage Dockerfile, through `make check`, `make build` and `verify-build: 4 bundle(s) verified autistmask-build-debug=off`. - `make fmt` run; markdown and JS committed formatted. ## Docs The README "Entrypoints" entry for `script/lint` no 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 the `script/test-e2e` header both said a used-but-not-imported identifier is invisible to `make check`. `TODO.md` moved on in the same commit. ## Not done here - `script/test` and `script/fmt-check` still 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.
clawbot added 1 commit 2026-08-14 06:18:47 +02:00
build: add ESLint to script/lint and containerize linting (closes #152)
Some checks failed
check / check (push) Failing after 2m16s
Details
7270480e0b 
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.
clawbot added the needs-review label 2026-08-14 06:18:52 +02:00
clawbot self-assigned this 2026-08-14 06:18:53 +02:00
Some checks failed
check / check (push) Failing after 2m16s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-152-eslint:issue-152-eslint
git checkout issue-152-eslint
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#286
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?