Skip to content

fix: repair pnpm run lint and gate it in CI - #108

Open
JohnMcLear wants to merge 3 commits into
mainfrom
ci/repair-lint
Open

JohnMcLear wants to merge 3 commits into
mainfrom
ci/repair-lint

Conversation

@JohnMcLear

@JohnMcLear JohnMcLear commented Sep 20, 2026

Copy link
Copy Markdown
Member

The bug

pnpm run lint fails on the default branch of this repo (and of ~80 other ether/ep_* plugins):

$ pnpm run lint
Oops! Something went wrong! :(
ESLint: 8.57.1
TypeError: Failed to load plugin '@typescript-eslint' declared in
'.eslintrc.cjs » eslint-config-etherpad/plugin#overrides[0] » ./node.js » ./index.js#overrides[0]':
Cannot read properties of undefined (reading 'Intrinsic')
    at .../ts-api-utils@1.4.3_typescript@7.0.2/node_modules/ts-api-utils/lib/index.cjs:779:57

Root cause

Dependabot bumped typescript to ^7.0.2. TypeScript 7 is the native (Go) port and no longer exposes the legacy compiler API surface, so ts.TypeFlags is undefined. ts-api-utils evaluates this at module load:

var IntrinsicTypeFlags = ts.TypeFlags.Intrinsic ?? ...

…which throws, taking @typescript-eslint — and therefore all of eslint-config-etherpad — down with it. ESLint exits without linting a single file.

No released @typescript-eslint supports TypeScript 7. 8.70.0 and every 8.70.1-alpha.* declare typescript: ">=4.8.4 <6.1.0", so this cannot be fixed by upgrading the lint toolchain today; typescript has to be held inside the supported range.

The fix

  • typescript~6.0.3, the newest line @typescript-eslint supports and an exact match for its <6.1.0 ceiling.
  • eslint-config-etherpad^5.0.0. 5.x declares that same typescript range itself, so a future TypeScript major fails resolution loudly instead of breaking lint silently. The .eslintrc.cjs entry 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.yml and wires it into test-and-release.yml:

jobs:
  lint:
    uses: ./.github/workflows/lint.yml
    secrets: inherit
  backend: ...
  frontend: ...
  release:
    needs:
      - lint
      - backend
      - frontend

Because automerge.yml only 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:

File Rule Fix
static/tests/backend/specs/popup_overflow.js mocha/consistent-spacing-between-blocks eslint --fix
static/tests/backend/specs/popup_overflow.js mocha/no-synchronous-tests ×3 marked the before hook and the two it bodies async

No 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.0 bump introduced.)

Three camelcase / no-use-before-define warnings remain (hook_name, diffUsingJS); warnings don't fail the build.

Verification

$ pnpm install && pnpm run lint
> ep_timesliderdiff@0.0.68 lint
> eslint .
✖ 3 problems (0 errors, 3 warnings)

$ echo $?
0

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: read on the lint job. It declared no permissions, so it inherited contents: write / id-token: write from test-and-release.yml while running eslint . — which executes .eslintrc.cjs and every installed ESLint plugin. This is a property of the shared plugin lint workflow and should be propagated to the other ether/ep_* repos that already took it.
  • engines.node>=22.0.0. eslint-config-etherpad@5 pulls in eslint-visitor-keys@5.0.1 (^20.19.0 || ^22.13.0 || >=24), so advertising >=18.0.0 would break an engine-strict dev install. >=22.0.0 is the current ether plugin floor.

Two further findings were declined, with reasons, in the review thread: the "no regression test" one (the new lint job is the regression test) and the "fork PRs skip lint" one (a property of the whole plugin CI template — backend and frontend already miss fork PRs identically — so it belongs in a template-wide change).

🤖 Generated with Claude Code

https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw

JohnMcLear and others added 2 commits September 20, 2026 18:56
`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-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Restore ESLint compatibility and gate releases on lint

🐞 Bug fix ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Restores ESLint compatibility by pinning TypeScript 6 and upgrading Etherpad's lint configuration.
• Runs lint through reusable CI and blocks releases when lint fails.
• Resolves latent Mocha lint errors without changing assertions.
Diagram

graph TD
  EV["Push Event"] --> CI["CI Orchestrator"] --> LW["Lint Workflow"] --> TOOL["Pinned Toolchain"] --> ESL["ESLint"] --> REL["Release"]
  CI --> BT["Backend Tests"] --> REL
  CI --> FT["Frontend Tests"] --> REL
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Ignore TypeScript 7 in Dependabot
  • ➕ Prevents the known incompatible major from being proposed.
  • ➕ Requires only a small dependency-management configuration change.
  • ➖ Does not detect other dependency changes that break lint.
  • ➖ Requires manual maintenance as compatibility ranges evolve.
2. Inline lint in the main workflow
  • ➕ Keeps the complete CI definition in one file.
  • ➕ Avoids an additional reusable workflow boundary.
  • ➖ Reduces reuse across Etherpad plugin repositories.
  • ➖ Makes the test-and-release orchestrator more verbose.

Recommendation: Keep the PR's compatible TypeScript constraint and reusable lint gate. A Dependabot ignore rule would only mask one upgrade path, while upgrading the lint toolchain cannot currently support TypeScript 7; CI enforcement provides broader protection against future regressions.

Files changed (5) +308 / -477

Tests (1) +4 / -3
popup_overflow.jsClear latent Mocha lint failures +4/-3

Clear latent Mocha lint failures

• Adds consistent block spacing and marks the setup hook and test callbacks asynchronous to satisfy Mocha lint rules. Assertions and tested behavior remain unchanged.

static/tests/backend/specs/popup_overflow.js

Other (4) +304 / -474
lint.ymlAdd reusable ESLint workflow +35/-0

Add reusable ESLint workflow

• Adds a callable GitHub Actions workflow that configures Node.js and pnpm, restores the dependency cache, installs without lifecycle scripts, and runs ESLint.

.github/workflows/lint.yml

test-and-release.ymlGate releases on successful lint +4/-0

Gate releases on successful lint

• Invokes the reusable lint workflow alongside backend and frontend tests. Adds lint to the release job dependencies so publication cannot proceed after lint failures.

.github/workflows/test-and-release.yml

package.jsonRestore a compatible TypeScript lint toolchain +2/-2

Restore a compatible TypeScript lint toolchain

• Pins TypeScript to the supported 6.0 line and upgrades eslint-config-etherpad to 5.x, which declares an explicit TypeScript compatibility ceiling.

package.json

pnpm-lock.yamlResolve the updated ESLint and TypeScript dependency graph +263/-472

Resolve the updated ESLint and TypeScript dependency graph

• Regenerates the lockfile for eslint-config-etherpad 5.0.2 and TypeScript 6.0.3, including the newer TypeScript ESLint packages. Removes TypeScript 7's platform-specific native packages and obsolete transitive dependencies.

pnpm-lock.yaml

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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
@JohnMcLear

Copy link
Copy Markdown
Member Author

Thanks — went through the review. Two of the findings were real and are fixed in fix: least-privilege lint job and an honest engines.node; two are declined, with reasons.

Fixed — "lint code can modify the repository" (High). Correct. 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 now pins:

  lint:
    name: ESLint
    runs-on: ubuntu-latest
    permissions:
      contents: read

The 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 ether/ep_* repos that already took it.

Fixed — "strict installs reject declared node". Also correct, and caused by this PR: eslint-config-etherpad@5 pulls in eslint-visitor-keys@5.0.1, whose engines are ^20.19.0 || ^22.13.0 || >=24, while the manifest still advertised >=18.0.0. engines.node is now >=22.0.0, the current ether plugin floor. (That range still nominally admits 22.0–22.12, which eslint-visitor-keys excludes; tightening further would mean departing from the project-wide floor for a devDependency, so it's left at the policy value.)

Declined — "the fix adds no regression test". The regression test is the new lint job: before this PR pnpm run lint exited non-zero without linting a file, and the job would have been red. A separate unit test that shells out to ESLint to assert exit 0 would only restate what the CI job already asserts, and would itself break on the next toolchain bump for reasons unrelated to this repo.

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: test-and-release.yml triggers on push/workflow_dispatch only, so the existing backend and frontend jobs already miss fork PRs in exactly the same way. Adding a pull_request trigger changes run semantics for all three jobs and interacts with automerge.yml, so it belongs in a template-wide change across the ~85 plugin repos, not in this one-repo lint repair.

🤖 Generated with Claude Code

https://claude.ai/code/session_013S4pYSjwUsiZtdtMMpW7bw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant