fix: repair pnpm run lint and gate it in CI - #108
JohnMcLear wants to merge 3 commits into
Conversation
`typescript: ^7.0.2` resolved to TypeScript 7, the native port, which no longer exposes the legacy compiler API. `ts-api-utils` (pulled in by `@typescript-eslint` through `eslint-config-etherpad`) throws `Cannot read properties of undefined (reading 'Intrinsic')` on load, taking the whole ESLint config down before any file is checked. Pin `typescript` to `~6.0.3` and bump `eslint-config-etherpad` to `^5.0.0`, which declares `typescript: ">=4.8.4 <6.1.0"` so a future TypeScript major fails loudly at install time instead of silently breaking lint. Add a reusable `lint.yml` workflow and wire it into `test-and-release.yml` so lint actually runs in CI and gates the release job — the reason this rotted unnoticed is that it never ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw
With lint restored, ESLint reports findings that were latent while it could not run at all. These are mechanical and behaviour-preserving: partly `eslint --fix`, partly by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoRestore ESLint compatibility and gate releases on lint
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history |
Two points from the automated review on this PR: * The reusable lint job declared no `permissions`, so it inherited `contents: write` and `id-token: write` from test-and-release.yml while running `eslint .` — which executes `.eslintrc.cjs` and every installed ESLint plugin. It only needs to read the checkout, so it now pins `permissions: contents: read`. The release job keeps its own elevated block. * `eslint-config-etherpad@5` pulls in `eslint-visitor-keys@5.0.1`, whose engines are `^20.19.0 || ^22.13.0 || >=24`, while this package still advertised `node: >=18.0.0`. Under an engine-strict installer a contributor on an advertised Node 18 could no longer install the dev dependencies. `engines.node` is raised to `>=22.0.0`, the current ether plugin floor, so the manifest states something true again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw
|
Thanks — went through the review. Two of the findings were real and are fixed in Fixed — "lint code can modify the repository" (High). Correct. The reusable lint job declared no lint:
name: ESLint
runs-on: ubuntu-latest
permissions:
contents: readThe release job keeps its own elevated block. Note this applies to the shared plugin lint workflow, not just this repo, so it needs propagating to the other Fixed — "strict installs reject declared node". Also correct, and caused by this PR: Declined — "the fix adds no regression test". The regression test is the new Declined here — "fork pull requests skip lint". True, but it is a property of the whole ether plugin CI template rather than of this change: 🤖 Generated with Claude Code |
The bug
pnpm run lintfails on the default branch of this repo (and of ~80 otherether/ep_*plugins):Root cause
Dependabot bumped
typescriptto^7.0.2. TypeScript 7 is the native (Go) port and no longer exposes the legacy compiler API surface, sots.TypeFlagsisundefined.ts-api-utilsevaluates this at module load:…which throws, taking
@typescript-eslint— and therefore all ofeslint-config-etherpad— down with it. ESLint exits without linting a single file.No released
@typescript-eslintsupports TypeScript 7.8.70.0and every8.70.1-alpha.*declaretypescript: ">=4.8.4 <6.1.0", so this cannot be fixed by upgrading the lint toolchain today;typescripthas to be held inside the supported range.The fix
typescript→~6.0.3, the newest line@typescript-eslintsupports and an exact match for its<6.1.0ceiling.eslint-config-etherpad→^5.0.0. 5.x declares that sametypescriptrange itself, so a future TypeScript major fails resolution loudly instead of breaking lint silently. The.eslintrc.cjsentry points are unchanged, so this repo's existing config keeps working.The lockfile diff is large because TypeScript 7 ships ~20 per-platform native binary packages (
@typescript/typescript-linux-x64,-darwin-arm64, …) that TypeScript 6 doesn't have.Why it rotted: lint ran in no CI job
That's the real defect — the breakage sat on the default branch unnoticed. This PR adds a reusable
.github/workflows/lint.ymland wires it intotest-and-release.yml:Because
automerge.ymlonly auto-merges Dependabot PRs when the Node.js Package run succeeds, a future dependency bump that breaks lint now turns that run red and is held back instead of landing silently. No Dependabot ignore rule is added — CI is the gate.Lint findings
Four errors, fixed in a separate commit so the dependency/CI commit stays reviewable on its own — all in one backend spec:
static/tests/backend/specs/popup_overflow.jsmocha/consistent-spacing-between-blockseslint --fixstatic/tests/backend/specs/popup_overflow.jsmocha/no-synchronous-tests×3beforehook and the twoitbodiesasyncNo assertions changed. (Verified the same findings appear with
eslint-config-etherpad@4.0.5, so they are pre-existing debt, not something the^5.0.0bump introduced.)Three
camelcase/no-use-before-definewarnings remain (hook_name,diffUsingJS); warnings don't fail the build.Verification
Review follow-up
A third commit (
fix: least-privilege lint job and an honest engines.node) addresses two findings from the automated review:permissions: contents: readon the lint job. It declared nopermissions, so it inheritedcontents: write/id-token: writefromtest-and-release.ymlwhile runningeslint .— which executes.eslintrc.cjsand every installed ESLint plugin. This is a property of the shared plugin lint workflow and should be propagated to the otherether/ep_*repos that already took it.engines.node→>=22.0.0.eslint-config-etherpad@5pulls ineslint-visitor-keys@5.0.1(^20.19.0 || ^22.13.0 || >=24), so advertising>=18.0.0would break an engine-strict dev install.>=22.0.0is the current ether plugin floor.Two further findings were declined, with reasons, in the review thread: the "no regression test" one (the new
lintjob is the regression test) and the "fork PRs skip lint" one (a property of the whole plugin CI template —backendandfrontendalready miss fork PRs identically — so it belongs in a template-wide change).🤖 Generated with Claude Code
https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw