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

build: add ESLint to script/lint — make check cannot currently catch undefined identifiers #152

New Issue
Open
opened 2026-08-09 03:42:24 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:42:24 +02:00
Collaborator
Copy Link

Problem

script/lint runs prettier --check . (package.json:11), which is
byte-identical to script/fmt-check (package.json:13). So make check runs
the same formatting check twice and performs no static analysis at all.
This repo is a cryptocurrency wallet and has no linter.

Two hard, shipped, user-facing crashes on main are the direct consequence,
both of the same class (identifier used but not imported):

  • AddToken is entirely unreachable (addToken.js:24 uses showView, never
    imported) — filed separately.
  • TransactionDetail crashes for every ERC-20 transfer
    (transactionDetail.js:136 uses addressDotHtml, never imported) — filed
    separately.

make check is green on main despite both. A linter with no-undef would
have caught each one at commit time. The CI workflow cannot catch them either,
because nothing exercises the views.

There is also a substantial tail of unused imports that a linter would flag:
src/popup/index.js:15 (clearViewStack); confirmTx.js (formatUnits,
showFlash, flashCopyFeedback, escapeHtml, currentNetwork);
settings.js:13 (NETWORKS, SUPPORTED_CHAIN_IDS); addressToken.js
(currentAddress, getAddressValueUsd); addressDetail.js
(currentAddress); approval.js (currentNetwork); home.js
(flashCopyFeedback); send.js (escapeHtml); txStatus.js (saveState);
helpers.js:7 (getAddressValueUsd).

Implementation requirements

  • Add ESLint as a devDependency via yarn (never npm). Use the modern flat
    config (eslint.config.js). Per the global convention, prefer the widely
    used, well maintained standard: @eslint/js recommended as the base.
  • Rules that must be enabled and must fail the build:
    • no-undef (the rule that catches both live crashes)
    • no-unused-vars (catches the dead-import tail above)
  • Configure the right environments/globals so there are no false positives:
    the popup and content scripts are browser context, src/background/ is a
    service worker, the sources use CommonJS require/module.exports, and
    tests/ is jest. build.js is Node. Set ecmaVersion appropriately.
  • Wire it into script/lint in addition to the existing prettier check —
    do not drop formatting enforcement. Keep script/fmt-check as it is.
    package.json's lint script should invoke eslint too, so
    yarn run lint and make lint agree.
  • Fix every violation the new linter reports. Removing genuinely unused
    imports is in scope for this issue. If a rule produces a large volume of
    unrelated churn, narrow the rule rather than mass-editing unrelated code,
    and say so in the PR.
  • Do not use scripted search-and-replace to strip the unused imports; edit the
    files directly.
  • Respect the ordering: the two point-fix crash issues may land before or
    after this, but after this issue lands, both classes must be
    linter-detectable. If they have already landed, confirm the linter stays
    green; if not, this issue is expected to surface them.
  • make check must remain non-mutating (REPO_POLICIES) — use --fix only
    manually, never inside script/lint.

Definition of done

  • eslint.config.js exists, ESLint is a pinned devDependency in
    package.json with a matching yarn.lock integrity entry.
  • make lint runs both ESLint and prettier and fails on either.
  • no-undef and no-unused-vars are active and error-level.
  • Deliberately introducing foo() with no import into any src/ file
    makes make lint fail. Demonstrate this in the PR description.
  • Zero ESLint violations remain in src/, tests/, and build.js.
  • make check passes and modifies no files in the working tree.
  • README "Entrypoints" section still accurately describes script/lint.
  • TODO.md updated in the same commit.
## Problem `script/lint` runs `prettier --check .` (`package.json:11`), which is byte-identical to `script/fmt-check` (`package.json:13`). So `make check` runs the same formatting check twice and performs **no static analysis at all**. This repo is a cryptocurrency wallet and has no linter. Two hard, shipped, user-facing crashes on `main` are the direct consequence, both of the same class (identifier used but not imported): - AddToken is entirely unreachable (`addToken.js:24` uses `showView`, never imported) — filed separately. - TransactionDetail crashes for every ERC-20 transfer (`transactionDetail.js:136` uses `addressDotHtml`, never imported) — filed separately. `make check` is green on `main` despite both. A linter with `no-undef` would have caught each one at commit time. The CI workflow cannot catch them either, because nothing exercises the views. There is also a substantial tail of unused imports that a linter would flag: `src/popup/index.js:15` (`clearViewStack`); `confirmTx.js` (`formatUnits`, `showFlash`, `flashCopyFeedback`, `escapeHtml`, `currentNetwork`); `settings.js:13` (`NETWORKS`, `SUPPORTED_CHAIN_IDS`); `addressToken.js` (`currentAddress`, `getAddressValueUsd`); `addressDetail.js` (`currentAddress`); `approval.js` (`currentNetwork`); `home.js` (`flashCopyFeedback`); `send.js` (`escapeHtml`); `txStatus.js` (`saveState`); `helpers.js:7` (`getAddressValueUsd`). ## Implementation requirements - Add ESLint as a devDependency via `yarn` (never `npm`). Use the modern flat config (`eslint.config.js`). Per the global convention, prefer the widely used, well maintained standard: `@eslint/js` recommended as the base. - Rules that must be enabled and must fail the build: - `no-undef` (the rule that catches both live crashes) - `no-unused-vars` (catches the dead-import tail above) - Configure the right environments/globals so there are no false positives: the popup and content scripts are browser context, `src/background/` is a service worker, the sources use CommonJS `require`/`module.exports`, and `tests/` is jest. `build.js` is Node. Set `ecmaVersion` appropriately. - Wire it into `script/lint` **in addition to** the existing prettier check — do not drop formatting enforcement. Keep `script/fmt-check` as it is. `package.json`'s `lint` script should invoke eslint too, so `yarn run lint` and `make lint` agree. - Fix every violation the new linter reports. Removing genuinely unused imports is in scope for this issue. If a rule produces a large volume of unrelated churn, narrow the rule rather than mass-editing unrelated code, and say so in the PR. - Do not use scripted search-and-replace to strip the unused imports; edit the files directly. - Respect the ordering: the two point-fix crash issues may land before or after this, but after this issue lands, both classes must be linter-detectable. If they have already landed, confirm the linter stays green; if not, this issue is expected to surface them. - `make check` must remain non-mutating (REPO_POLICIES) — use `--fix` only manually, never inside `script/lint`. ## Definition of done - [ ] `eslint.config.js` exists, ESLint is a pinned devDependency in `package.json` with a matching `yarn.lock` integrity entry. - [ ] `make lint` runs both ESLint and prettier and fails on either. - [ ] `no-undef` and `no-unused-vars` are active and error-level. - [ ] Deliberately introducing `foo()` with no import into any `src/` file makes `make lint` fail. Demonstrate this in the PR description. - [ ] Zero ESLint violations remain in `src/`, `tests/`, and `build.js`. - [ ] `make check` passes and modifies no files in the working tree. - [ ] README "Entrypoints" section still accurately describes `script/lint`. - [ ] `TODO.md` updated in the same commit.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:42:24 +02:00
clawbot commented 2026-08-12 10:24:34 +02:00
Author
Collaborator
Copy Link

Scope note, found while landing #206: script/lint is yarn run lint (prettier --check .) on the host and does not containerize. The only containerized path is script/cibuild, whose Dockerfile runs make check.

That is tolerable while lint is prettier-only, but it stops being tolerable when this issue puts ESLint behind that target: results would then depend on whichever ESLint the host happens to have, which is the exact class of drift the docker-only lint policy exists to prevent.

So this unit should containerize script/lint as well as add ESLint to it, and make check must reach the containerized path rather than the host one.

Scope note, found while landing https://git.eeqj.de/sneak/AutistMask/pulls/206: `script/lint` is `yarn run lint` (`prettier --check .`) on the **host** and does not containerize. The only containerized path is `script/cibuild`, whose Dockerfile runs `make check`. That is tolerable while lint is prettier-only, but it stops being tolerable when this issue puts ESLint behind that target: results would then depend on whichever ESLint the host happens to have, which is the exact class of drift the docker-only lint policy exists to prevent. So this unit should containerize `script/lint` as well as add ESLint to it, and `make check` must reach the containerized path rather than the host one.
clawbot commented 2026-08-14 06:01:28 +02:00
Author
Collaborator
Copy Link

Plan, on issue-152-eslint:

  1. yarn add --dev pinned eslint + @eslint/js (exact versions, yarn.lock integrity entries).
  2. eslint.config.js (flat), @eslint/js recommended as base, sourceType: "commonjs", ecmaVersion current. Per-tree globals so there are no false positives: src/popup/** + src/content/** browser, src/background/** service worker, src/shared/** the intersection both actually use, tests/** jest + node, build.js + tests/e2e/** node. no-undef and no-unused-vars error-level; dist/, node_modules/, src/shared/phishingBlocklist.json ignored.
  3. Fix every violation by hand, file by file — no scripted rewrites. Expected shape is the unused-import tail in the body plus whatever no-undef finds beyond it. If a rule turns out to generate churn unrelated to this issue I will narrow the rule and say so in the PR rather than mass-edit.
  4. package.json lint becomes eslint + the existing prettier --check ., so yarn run lint and make lint agree. script/fmt-check untouched, no --fix anywhere in the lint path, make check stays non-mutating.
  5. Per the scope note above, script/lint gets containerized: a lint stage in the Dockerfile on the already-pinned node base, script/lint running docker build --target lint from the host, and an explicit env guard set in the image so the make check that runs inside the CI build takes the native path instead of recursing into docker. make check therefore reaches the containerized linter, not the host's ESLint.
  6. Demonstrate the linter is load-bearing: add an unimported foo() call to a src/ file, capture the failing make lint output into the PR body, revert.
  7. README Entrypoints line for script/lint updated to say what it now runs and that it containerizes; TODO.md moved on in the same commit.
Plan, on `issue-152-eslint`: 1. `yarn add --dev` pinned `eslint` + `@eslint/js` (exact versions, `yarn.lock` integrity entries). 2. `eslint.config.js` (flat), `@eslint/js` recommended as base, `sourceType: "commonjs"`, `ecmaVersion` current. Per-tree globals so there are no false positives: `src/popup/**` + `src/content/**` browser, `src/background/**` service worker, `src/shared/**` the intersection both actually use, `tests/**` jest + node, `build.js` + `tests/e2e/**` node. `no-undef` and `no-unused-vars` error-level; `dist/`, `node_modules/`, `src/shared/phishingBlocklist.json` ignored. 3. Fix every violation by hand, file by file — no scripted rewrites. Expected shape is the unused-import tail in the body plus whatever `no-undef` finds beyond it. If a rule turns out to generate churn unrelated to this issue I will narrow the rule and say so in the PR rather than mass-edit. 4. `package.json` `lint` becomes eslint + the existing `prettier --check .`, so `yarn run lint` and `make lint` agree. `script/fmt-check` untouched, no `--fix` anywhere in the lint path, `make check` stays non-mutating. 5. Per the scope note above, `script/lint` gets containerized: a `lint` stage in the `Dockerfile` on the already-pinned node base, `script/lint` running `docker build --target lint` from the host, and an explicit env guard set in the image so the `make check` that runs *inside* the CI build takes the native path instead of recursing into docker. `make check` therefore reaches the containerized linter, not the host's ESLint. 6. Demonstrate the linter is load-bearing: add an unimported `foo()` call to a `src/` file, capture the failing `make lint` output into the PR body, revert. 7. README Entrypoints line for `script/lint` updated to say what it now runs and that it containerizes; `TODO.md` moved on in the same commit.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#152
Delete Branch "%!s()"

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?