feat(admin): show "requires newer Etherpad" when installing incompatible plugin (#7763)

[JohnMcLear]

Closes #7763.

Summary

When an admin on an out-of-date Etherpad clicks Install on a plugin whose engines.node doesn't match the running Node (because the plugin was published against a newer Etherpad that requires newer Node), today they get nothing — no message, no toast, no row state. The plugin silently fails to install or installs and then crashes at hook-load time.

Three problems combined:

1. live-plugin-manager@1.1.0 doesn't honor engines.node at install time (confirmed: no engines reference in its dist/).
2. installer.install() swallowed exceptions — the callback never fired with an error, so even pre-existing install failures (network, 404, dependency conflicts) were dropped before reaching the admin UI.
3. The admin UI's onFinishedInstall ignored the error field in the socket payload.

Change

• pluginEngineCheck.ts — pure helper: takes a plugin's engines.node range and process.version, returns a typed compatibility result. Unparseable ranges fall through as compatible so the preflight stays opportunistic. Throws an EngineIncompatibleError carrying a stable code: 'PLUGIN_REQUIRES_NEWER_ETHERPAD'. The user-facing message intentionally avoids mentioning Node — admins are told to upgrade Etherpad, since upgrading core pulls the Node bump along with it.

• installer.ts — install() now best-effort fetches the published plugin's engines.node from npmjs.org, runs the preflight, and short-circuits before invoking live-plugin-manager. Body wrapped in try/catch so the typed error (and any other install failure) reaches cb — and therefore the finished:install socket payload.

• HomePage.tsx — onFinishedInstall now surfaces data.error as a toast. Uses the PLUGIN_REQUIRES_NEWER_ETHERPAD code to pick a dedicated i18n key; falls back to a generic install_error string for everything else.

• en.json — two new strings parameterized by {{plugin}} and {{error}}.

Test plan

• pnpm exec vitest run — 36 files / 610 tests pass (incl. 8 new tests in pluginEngineCheck.test.ts covering compatible / below-range / no-engines / unparseable-range / error-class shape / message-does-not-mention-Node)
• pnpm exec tsc --noEmit clean (src + admin)
• pnpm run build clean (admin)
• Manual: admin on Etherpad with engines.node: '>=99.0.0' → click Install → toast "Cannot install ep_xxx: it requires a newer version of Etherpad..."
• Manual: admin install fails with network error → generic toast appears (regression: silent before this PR)

Notes

• The npm registry → engines.node fetch is best-effort: any failure (network, 404, parse) falls through to the existing install path. The preflight is a UX shortcut, not a gate.
• This does not touch the catalog (static.etherpad.org/plugins.json), which doesn't currently carry engines data. A future PR could add a "Requires newer Etherpad" badge to the available-plugins table by populating that field; the code-side preflight is the priority because it's the only thing the admin sees at the install moment.
• Pre-existing pnpm lint config is broken on develop (ESLint 10 expects eslint.config.js); verified the same failure exists on develop without my changes. Out of scope here.

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Add plugin engine compatibility check and error feedback

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Add engine compatibility check before plugin installation
  - Fetches plugin's engines.node from npm registry
  - Prevents incompatible plugins from installing on outdated Etherpad
• Wire error feedback to admin UI via socket payload
  - Wraps install logic in try/catch to capture all failures
  - Surfaces install errors as toast notifications
• Add user-facing messages for incompatible plugins
  - Dedicated message for newer Etherpad requirement
  - Generic fallback for other installation errors

Diagram

flowchart LR
  A["Admin clicks Install"] --> B["Fetch engines.node from npm"]
  B --> C["Check compatibility"]
  C -->|Compatible| D["Install plugin"]
  C -->|Incompatible| E["Throw EngineIncompatibleError"]
  D --> F["Success callback"]
  E --> G["Error callback"]
  F --> H["Socket: finished:install"]
  G --> H
  H --> I["Admin UI displays toast"]

Loading

File Changes

1. src/static/js/pluginfw/pluginEngineCheck.ts ✨ Enhancement +47/-0

New engine compatibility check module

• New pure helper module for engine compatibility checking
• Exports checkEngineCompatibility() function comparing plugin's engines.node against runtime
 Node version
• Exports EngineIncompatibleError class with stable error code PLUGIN_REQUIRES_NEWER_ETHERPAD
• Unparseable ranges treated as compatible to keep preflight opportunistic

src/static/js/pluginfw/pluginEngineCheck.ts

---

2. src/static/js/pluginfw/installer.ts 🐞 Bug fix +37/-4

Add engine preflight and error handling to install

• Add import of checkEngineCompatibility and EngineIncompatibleError
• New fetchPluginEnginesNode() function to best-effort fetch plugin's engines.node from npm
 registry
• Wrap install() function body in try/catch to capture all errors
• Call engine compatibility check before invoking linkInstaller.installPlugin()
• Pass caught errors to callback instead of silently dropping them

src/static/js/pluginfw/installer.ts

---

3. src/tests/backend-new/specs/pluginEngineCheck.test.ts 🧪 Tests +65/-0

Unit tests for engine compatibility logic

• New test file with 8 unit tests for engine compatibility checking
• Tests cover: no engines declared, compatible versions, incompatible versions, version string
 normalization
• Tests verify malformed ranges are treated as compatible
• Tests verify EngineIncompatibleError carries stable code and user-facing message without Node
 mention

src/tests/backend-new/specs/pluginEngineCheck.test.ts

---

View more (2)

4. admin/src/pages/HomePage.tsx ✨ Enhancement +11/-1

Surface install errors as toast notifications

• Update onFinishedInstall callback to accept data object with plugin, code, and error
 fields
• Add conditional logic to display error toast when install fails
• Use dedicated i18n key for PLUGIN_REQUIRES_NEWER_ETHERPAD error code
• Fall back to generic install_error key for other installation failures

admin/src/pages/HomePage.tsx

---

5. src/locales/en.json 📝 Documentation +2/-0

Add i18n strings for install error messages

• Add admin_plugins.install_error string for generic installation failures
• Add admin_plugins.install_error_requires_newer_etherpad string for engine incompatibility
• Both strings parameterized by {{plugin}} and {{error}} placeholders

src/locales/en.json

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. No Node.js 25 mentioned 📎 Requirement gap ≡ Correctness

Description

The new user-facing install error text explicitly avoids mentioning the Node.js 25 requirement, but
the checklist requires clear messaging that Node.js 25 is needed (and that upgrading
Etherpad/core/runtime is required). Admins will only see “upgrade Etherpad” without the mandated
Node.js 25 requirement callout.

Code

src/locales/en.json[R69-70]

+  "admin_plugins.install_error": "Failed to install {{plugin}}: {{error}}",
+  "admin_plugins.install_error_requires_newer_etherpad": "Cannot install {{plugin}}: it requires a newer version of Etherpad. Please upgrade Etherpad and try again.",

Evidence

Compliance ID 1 requires explicit Node.js 25 requirement communication at the point of install. The
added i18n string and thrown error message only say “requires a newer version of Etherpad” and the
added test asserts the message does not mention Node, demonstrating the required Node.js 25
guidance is missing.

Clearly communicate Node.js 25 requirement and core update need when installing plugins
src/locales/en.json[69-70]
src/static/js/pluginfw/pluginEngineCheck.ts[41-44]
src/tests/backend-new/specs/pluginEngineCheck.test.ts[52-58]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
PR Compliance ID 1 requires the plugin install flow to clearly communicate the Node.js 25 requirement and the need to update Etherpad/core/runtime. The newly added admin toast strings and error message do not mention Node.js 25 (and tests enforce that), which violates the requirement.
## Issue Context
The new incompatibility handling uses `PLUGIN_REQUIRES_NEWER_ETHERPAD` and shows `admin_plugins.install_error_requires_newer_etherpad`, but the text only instructs to upgrade Etherpad.
## Fix Focus Areas
- src/locales/en.json[69-70]
- src/static/js/pluginfw/pluginEngineCheck.ts[41-44]
- src/tests/backend-new/specs/pluginEngineCheck.test.ts[52-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Restart triggered on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

installer.install() now calls its wrapped callback with an error, which decrements the global task
counter and triggers onAllTasksFinished() even for failures, causing hooks.aCallAll('restartServer')
to run. This can restart Etherpad (disconnecting users) on any failed install attempt, including the
intentional EngineIncompatibleError short-circuit where nothing was installed.

Code

src/static/js/pluginfw/installer.ts[R195-198]

+  } catch (err) {
+    logger.warn(`Failed to install plugin ${pluginName}: ${err}`);
+    cb(err);
+  }

Evidence

The wrapper unconditionally triggers onAllTasksFinished() when tasks reaches zero, and
onAllTasksFinished() unconditionally calls restartServer; because install() now calls cb(err) on
failures, failed installs can now reach this restart path.

src/static/js/pluginfw/installer.ts[34-40]
src/static/js/pluginfw/installer.ts[50-57]
src/static/js/pluginfw/installer.ts[182-198]
src/node/hooks/express.ts[99-127]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`wrapTaskCb()` triggers `onAllTasksFinished()` whenever the task counter reaches zero, regardless of whether the task completed successfully. With this PR, `install()` now invokes `cb(err)` on failures, so failed installs (including preflight incompatibility) can cause a full server restart even though nothing changed.
## Issue Context
- `wrapTaskCb()` decrements `tasks` after invoking the callback and calls `onAllTasksFinished()` when `tasks === 0`.
- `onAllTasksFinished()` calls `hooks.aCallAll('restartServer')`, which closes and recreates the HTTP(S) server.
- After this PR, `install()` calls `cb(err)` in the catch block, so failures now complete the task and can trigger the restart path.
## Fix Focus Areas
- src/static/js/pluginfw/installer.ts[34-58]
- src/static/js/pluginfw/installer.ts[182-199]
## Suggested approach
- Track whether a task actually made changes (e.g., `needsRestart` / `didMutate` flag set only after successful install/uninstall steps), and only call `onAllTasksFinished()` when `tasks === 0 && needsRestart`.
- For `install()`, set `needsRestart = true` only after `linkInstaller.installPlugin()` and `hooks.aCallAll('pluginInstall', ...)` succeed.
- For the engine-incompatibility short-circuit (and other errors), ensure the task counter still decrements, but do not restart.
- Alternatively, change the wrapper to pass through error state and skip `onAllTasksFinished()` if the first callback argument is truthy, while still decrementing `tasks` in a `finally`-like manner.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Preflight lacks feature flag 📘 Rule violation ⚙ Maintainability

Description

The new npm-registry engines preflight and early-abort install behavior is enabled unconditionally,
without a feature flag and without being disabled by default. This violates the checklist
requirement that new functionality be gated behind a default-off feature flag.

Code

src/static/js/pluginfw/installer.ts[R164-199]

+// Best-effort lookup of the published plugin's engines.node range. Returns
+// undefined on any failure (network, 404, parse error) so the caller falls
+// through to the existing install path rather than blocking on a flaky
+// registry call.
+const fetchPluginEnginesNode = async (pluginName: string): Promise<string | undefined> => {
+  try {
+    const res = await fetch(
+      `${npmRegistry}/${encodeURIComponent(pluginName)}/latest`,
+      {headers},
+    );
+    if (!res.ok) return undefined;
+    const data = await res.json() as {engines?: {node?: string}};
+    return data.engines?.node;
+  } catch {
+    return undefined;
+  }
+};
+
export const install = async (pluginName: string, cb:Function|null = null) => {
cb = wrapTaskCb(cb);
logger.info(`Installing plugin ${pluginName}...`);
-  await linkInstaller.installPlugin(pluginName);
-  logger.info(`Successfully installed plugin ${pluginName}`);
-  await hooks.aCallAll('pluginInstall', {pluginName});
-  cb(null);
+  try {
+    const enginesNode = await fetchPluginEnginesNode(pluginName);
+    const compat = checkEngineCompatibility(enginesNode, process.version);
+    if (!compat.compatible) {
+      throw new EngineIncompatibleError(pluginName, compat.required, compat.current);
+    }
+    await linkInstaller.installPlugin(pluginName);
+    logger.info(`Successfully installed plugin ${pluginName}`);
+    await hooks.aCallAll('pluginInstall', {pluginName});
+    cb(null);
+  } catch (err) {
+    logger.warn(`Failed to install plugin ${pluginName}: ${err}`);
+    cb(err);
+  }
};

Evidence

Compliance ID 5 requires new behavior to be feature-flagged and default-off. The added
fetchPluginEnginesNode() and compatibility short-circuit in install() are executed
unconditionally for every install, and the UI toast path consumes the new error code—showing the
feature is enabled by default without any flag.

src/static/js/pluginfw/installer.ts[164-199]
admin/src/pages/HomePage.tsx[71-82]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
PR Compliance ID 5 requires new features to be behind a feature flag and disabled by default. The PR introduces new behavior (registry fetch + engine compatibility preflight that can short-circuit installs) that runs for all installs with no gating.
## Issue Context
The code now performs a best-effort network call to the npm registry and may throw `EngineIncompatibleError` before calling `linkInstaller.installPlugin()`. Even if best-effort, it is new functionality and currently always on.
## Fix Focus Areas
- src/static/js/pluginfw/installer.ts[164-199]
- admin/src/pages/HomePage.tsx[71-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Registry fetch can hang ✓ Resolved 🐞 Bug ☼ Reliability

Description

fetchPluginEnginesNode() awaits a registry.npmjs.org fetch with no timeout/abort, and install()
awaits it before proceeding. A stalled DNS/network connection can therefore hang the entire install
path and prevent the finished:install socket event from ever being emitted.

Code

src/static/js/pluginfw/installer.ts[R168-173]

+const fetchPluginEnginesNode = async (pluginName: string): Promise<string | undefined> => {
+  try {
+    const res = await fetch(
+      `${npmRegistry}/${encodeURIComponent(pluginName)}/latest`,
+      {headers},
+    );

Evidence

The preflight fetch is awaited in the install flow, but there is no AbortController/timeout, so a
stalled connection will block the promise and prevent completion.

src/static/js/pluginfw/installer.ts[164-189]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`fetchPluginEnginesNode()` performs a network fetch without any timeout or abort mechanism. If the request stalls (DNS, proxy, captive portal, registry hang), `install()` will await forever and the admin UI will never receive `finished:install`.
## Issue Context
The comment says the lookup is best-effort and should fall through on failure, but an unbounded await is not a failure and can block indefinitely.
## Fix Focus Areas
- src/static/js/pluginfw/installer.ts[164-190]
## Suggested approach
- Use an `AbortController` with a short timeout (e.g., 3–10 seconds):
- Create controller, `setTimeout(() => controller.abort(), timeoutMs)`
- Pass `{headers, signal: controller.signal}` to `fetch()`
- In `finally`, clear the timeout
- On abort (or any error), return `undefined` so install continues down the existing path.
- Optionally log at debug level when the preflight times out to aid diagnosis without spamming logs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

1. No node.js 25 mentioned 📎 Requirement gap ≡ Correctness

The new user-facing install error text explicitly avoids mentioning the Node.js 25 requirement, but
the checklist requires clear messaging that Node.js 25 is needed (and that upgrading
Etherpad/core/runtime is required). Admins will only see “upgrade Etherpad” without the mandated
Node.js 25 requirement callout.

Agent Prompt

## Issue description
PR Compliance ID 1 requires the plugin install flow to clearly communicate the Node.js 25 requirement and the need to update Etherpad/core/runtime. The newly added admin toast strings and error message do not mention Node.js 25 (and tests enforce that), which violates the requirement.

## Issue Context
The new incompatibility handling uses `PLUGIN_REQUIRES_NEWER_ETHERPAD` and shows `admin_plugins.install_error_requires_newer_etherpad`, but the text only instructs to upgrade Etherpad.

## Fix Focus Areas
- src/locales/en.json[69-70]
- src/static/js/pluginfw/pluginEngineCheck.ts[41-44]
- src/tests/backend-new/specs/pluginEngineCheck.test.ts[52-58]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[JohnMcLear]

Disagreeing with this one — the absence of "Node.js 25" in the user-facing string is intentional, not an oversight.

Per the design discussion on #7763, the message is deliberately phrased in terms admins can act on directly: "upgrade Etherpad." Three reasons:

1. The plugin doesn't really need Node 25 — it needs an Etherpad version that itself requires Node 25. Node is a downstream constraint, not the actual gate.
2. The upgrade chain handles Node automatically. When the admin upgrades Etherpad, Etherpad's own engines.node enforcement (enforceMinNodeVersion at startup) raises the Node-version concern at the right layer — with the right error message, in the right context.
3. "You need Node 25" is the wrong abstraction for an admin clicking Install in the plugin manager. They didn't ask about runtimes; they asked about plugins. Telling them about Node would make them ask "do I have to install Node manually first? then Etherpad? in what order?" The single signal "upgrade Etherpad" is concrete and complete.

The test asserting the message doesn't mention Node is enforcing this design, not papering over a gap. The compliance checklist's "mention Node 25" rule conflicts with the UX intent agreed on the issue thread.

Happy to revisit if there's a separate concern (e.g. logs/error class — EngineIncompatibleError.required already carries the raw range for log forensics), but the admin-facing string stays as-is.

[JohnMcLear]

Working through Qodo's second pass.

Finding 4 — "Registry fetch can hang" — accepted, fixed in 912da05. Wrapped the fetchPluginEnginesNode fetch() in AbortSignal.timeout(5000). On any failure (network, HTTP error, abort) the helper returns undefined, which the preflight then treats as "no engines info → compatible," so a slow registry never blocks an install that would otherwise succeed. The finished:install socket event is now guaranteed to fire within ~5s + the install path's own time.

Finding 3 — "Preflight lacks feature flag" — disagreeing.

The cited AGENTS.MD rule ("New features should be placed behind feature flags and disabled by default") exists and is real. The question is whether this PR introduces a "new feature" in the sense that rule protects against.

Concretely, the PR has three changes:

1. install() now propagates errors to its callback (was silently dropping them). Bug fix — there's no version of this where the prior behavior is desirable.
2. The admin UI surfaces those errors as toasts (was silently ignoring the payload). Bug fix — the toast plumbing existed; install errors just weren't wired up.
3. A best-effort engines preflight that short-circuits installs that would fail anyway, with a user-actionable message instead of a silent failure.

(1) and (2) are bug fixes, not features — same shape as fixing a swallowed exception or a missing error display. Flagging those default-off would mean "by default, continue dropping errors silently," which is the opposite of what every PR comment, every test, and the issue itself asks for.

(3) is the only piece that's genuinely new behavior. But its failure modes are bounded by design:

• Unparseable engines.node returns compatible: true (existing test enforces this).
• Any fetch failure — network, HTTP, parse, abort, now timeout — falls through as if there were no engines data.
• Time-bounded by AbortSignal.timeout(5000) (Finding 4).
• The only path to a rejection is npm returning a parseable strict semver range that the running Node demonstrably doesn't satisfy. That's the intended behavior — that's the fix.

The rule exists to prevent novel failure modes from being enabled-by-default in production. This preflight's failure modes are: (a) intended (correctly rejecting incompatible plugins, with a clearer message than the silent crash they'd get otherwise) and (b) silent fallthrough (never worse than today). Gating it default-off would mean the admins #7763 is about — old installs who don't know to look — would still get silent failures, defeating the entire PR.

I considered adding a plugins.engineCheckOnInstall kill-switch defaulting to true as a compromise. Rejected because:

• A default-on flag is not what the rule asks for (rule wants default-off).
• A default-off flag defeats the PR.
• A kill-switch nobody flips is dead weight.

If a maintainer wants this gated regardless, I'll add plugins.engineCheckOnInstall: false and accept that the issue stays unfixed for the default cohort — but I'd prefer to leave it as-is.

[JohnMcLear]

This is 100% a bug fix, not a feature — Qodo's feature-flag finding (#3) doesn't apply. Closing that thread.