feat(review): add local ESLint quality findings

[LarryHu0217]

Summary

• run the project's existing local ESLint only for changed JavaScript and TypeScript files
• map ESLint JSON findings into the shared review finding contract and reuse the existing sidebar/send-to-agent flow
• degrade silently when no ESLint config applies, while surfacing actionable provider failures
• add an explicit Refresh findings control for the current diff

Validation

• npm run check
• npm test (116 files passed, 2 skipped; 1,861 tests passed, 23 skipped)
• Electron IPC finding tests and client review tests

Fixes #260

[johannesjo]

Thanks for this — the shape is right: clean split between the main-process runner, the IPC boundary and a thin renderer adapter, args validated at the handler, parser defensive about unexpected JSON, no new dependencies. npm run typecheck, eslint --max-warnings 0 and prettier --check are all clean on the touched files, and I verified the happy path end-to-end by importing the real module and running it against a file with a genuine violation:

{ "status": "available", "findings": [ { "id": "eslint:src/__lint_probe.ts:1:21:@typescript-eslint/no-explicit-any", "severity": "error", "location": { "filePath": "src/__lint_probe.ts", "startLine": 1, "startColumn": 21, "endLine": 1, "endColumn": 24 }, ... } ] }

So the core feature works, including the exit-code-1 recovery (ESLint exits non-zero whenever it finds errors, so that catch branch is the normal path, not the exceptional one).

My concerns are concentrated in error classification — the part #260's Notes section asked to be strict about.

1. Two common setups produce a generic error that force-opens the sidebar on every diff open

Both verified by importing electron/ipc/eslint-quality-findings.ts and calling loadEslintQualityFindings directly.

a) ESLint config present, ESLint not resolvable.

$ npx --no-install eslint --format json src/a.ts
npm error npx canceled due to missing packages and no YES option: ["eslint@10.8.1"]

Exit 1, empty stdout. In classifyEslintError (eslint-quality-findings.ts:164) error.code is the numeric exit code 1, not 'ENOENT', and the text matches none of the three regexes, so it falls through to the generic branch:

{ "status": "unavailable", "message": "ESLint findings could not be loaded. Try again after checking the project lint command." }

b) Legacy .eslintrc* project on ESLint 9. hasEslintConfig (:137) accepts .eslintrc*, but ESLint ≥9 ignores those without ESLINT_USE_FLAT_CONFIG=false:

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

Same generic result — confirmed by running the module against a .eslintrc.json-only project with ESLint 9 on the path.

In both cases unavailable makes the renderer provider throw, which sets findingsError → hasReviewSidebarState() goes truthy (ReviewSidebarPanel.tsx:19) and setSidebarOpen(true) fires (ReviewProvider.tsx:330). That's an error banner force-opening the sidebar on every diff open — the #258 pattern the issue explicitly asked to avoid, and the message doesn't tell the user what to do about it.

Exposure is narrower than I first thought — node_modules is a default-checked symlink candidate (git.ts:144), so most worktree tasks do have ESLint available. It still bites when the box is unchecked, when deps aren't installed yet, on Yarn PnP, and (independently of any of that) on every legacy-eslintrc project.

Suggestion that fixes both and simplifies the runner: resolve <worktree>/node_modules/.bin/eslint directly (with a short walk up for monorepos) instead of going through npx, and treat a missing binary as not-applicable — silent. That matches the issue's non-goal ("Running or bundling a linter the project doesn't already have") better than an error banner does, and it drops npm's overhead from the happy path. Worth knowing: --no-install does correctly refuse to install, but npm still does a registry round trip to resolve the spec first — note it resolved eslint@10.8.1 above. That's a network call per diff open, and offline it sits in npm's fetch retries against the 30s timeout.

For (b), either drop the .eslintrc* names from detection or map "couldn't find an eslint.config" to not-applicable.

2. Error mapping has no tests

The acceptance criteria call for it ("Parsing and error mapping have tests"). Parsing is well covered; classifyEslintError isn't exported and exec isn't injectable, so neither the classification branches nor the exit-1-with-stdout path can be exercised. That gap is what let #1 through. Injecting the exec function (like git-exclude.ts does with execFileSyncImpl) would make all of it testable.

Also uncovered: the renderer mapping in src/lib/eslint-quality-findings.ts, and refreshFindings — ReviewProvider.client.test.tsx already has a deferred-provider harness that makes that a short test.

3. Missing -- before the file list

validateRelativePath (register.ts:222) rejects absolute and .. paths but permits a leading dash, and the paths are spread straight into argv at :196. Verified — a file named --version in the repo:

$ eslint --format json --no-error-on-unmatched-pattern "--version"
v9.39.3                                    # parsed as a flag
$ eslint --format json --no-error-on-unmatched-pattern -- "--version"
[{"filePath":".../--version","messages":[...]}]   # linted as a file

A file named --fix would have ESLint rewrite the user's files — an explicit non-goal. execFile blocks shell injection but not argv injection; adding '--' before the spread is a one-element fix.

4. Findings outside the changed hunks land in the sidebar

File-level scoping is what the issue asked for, but ESLint reports the whole file, and reconcileQualityFindings marks anything not on a diff line stale. openFindings() doesn't filter by freshness, so those entries render in the list and count toward the Review (N) badge (ReviewSidebar.tsx:198) while being unselectable and unsubmittable. They are labelled "Stale" (QualityFindingSidebarItem.tsx:21), so it's not silent — but "stale" is a misleading word for "pre-existing, outside the diff", and on a codebase with lint debt it buries the actual signal. Worth deciding deliberately: filter to findings landing on the diff's lines in the provider, or make them collapsible.

Smaller things

• ENOENT message is wrong (:167): ENOENT means npx isn't on PATH — a machine-level problem — reported as "ESLint is not available in this project."
• Timeouts misreport: at 30s the child is killed; partial stdout → "ESLint returned malformed JSON output.", empty stdout → the generic message. Neither says "timed out", though error.killed/signal is available.
• No child-process cancellation: invalidateFindingLoad() only ignores the result, so rapid commit navigation stacks concurrent ESLint runs (~2s each, measured on a single file in this repo).
• Refresh button is now always visible (DiffViewerDialog.tsx:394): with the ESLint provider wired in, findingProvider is never undefined, so a Python project gets a permanent "Refresh findings" button that does nothing.
• Refresh before the diff finishes loading is a silent no-op — activeReviewDiff is still null while the button looks enabled.
• Finding id omits endLine/endColumn, so two messages from the same rule at the same start position collide on the key.
• Very large diffs spread every changed path into argv; a few thousand files risks E2BIG.

Summary

#1 is the one I'd want fixed before merge — it turns the first real provider into a recurring, non-actionable error banner for the setups it does hit, which is the specific failure #260 was written to prevent. #2 and #3 are cheap and worth folding into the same round. #4 is a design call better made now than discovered after merge.

[LarryHu0217]

Implemented the requested follow-up in commit 212c8fa7:

• Resolve the local node_modules/.bin/eslint binary (walking up a few monorepo levels) and return not-applicable when it is not installed, without an npx registry/path lookup.
• Stop treating legacy .eslintrc* files as active ESLint 9 config, and map the missing-flat-config error to not-applicable.
• Inject the executor for error-mapping tests and pass -- before changed paths.

Validation: 7 focused tests passed; changed-file ESLint passed; Prettier check passed; strict TypeScript check for the provider passed.

[johannesjo]

Thank you very much! <3