diff --git a/src/servers/api/plugins/router.ts b/src/servers/api/plugins/router.ts index 8e8d4bb7..46448ff7 100644 --- a/src/servers/api/plugins/router.ts +++ b/src/servers/api/plugins/router.ts @@ -14,7 +14,7 @@ import { purgePlugin, type PluginActionResult, } from '../../plugins/install'; -import { updatePlugin, syncCapabilities } from '../../plugins/update'; +import { updatePlugin, enablePlugin, syncCapabilities } from '../../plugins/update'; // /api/plugins — what is on this machine, what is installed, and the four verbs that change it. // @@ -233,8 +233,10 @@ pluginsRouter.post('/:appName/purge', async (ctx) => { return ctx.json(await purgePlugin(ctx.req.param('appName'), { dropData: true })); }); +// Enabling refreshes the checkout first — see `enablePlugin`. A disabled plugin's directory is what the +// next start runs, so turning one back on used to run whatever was current the day it was switched off. pluginsRouter.post('/:appName/enable', async (ctx) => { - const result = await setPluginRunning(ctx.req.param('appName'), true); + const result = await enablePlugin(ctx.req.param('appName'), undefined, ctx.get('user').id); return ctx.json(result, result.ok ? 200 : 400); }); @@ -265,7 +267,7 @@ type VerbFn = ( const VERBS = { install: (appName, onStep, userId, onLog) => installPlugin(appName, onStep, userId, onLog), - enable: (appName, onStep) => setPluginRunning(appName, true, onStep), + enable: (appName, onStep, userId, onLog) => enablePlugin(appName, onStep, userId, onLog), disable: (appName, onStep) => setPluginRunning(appName, false, onStep), // Same function, and the verb IS the flag — see the two routes above. delete: (appName, onStep, _userId, onLog) => purgePlugin(appName, { dropData: false }, onStep, onLog), diff --git a/src/servers/plugins/update.test.ts b/src/servers/plugins/update.test.ts index 4bd9e265..c2ed8fbd 100644 --- a/src/servers/plugins/update.test.ts +++ b/src/servers/plugins/update.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test, afterAll } from 'bun:test'; import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { PLUGINS_DIR } from './discover'; -import { pullPlugin } from './update'; +import { pullPlugin, enablePlugin } from './update'; // The REFUSALS are what this test is for. A pull that works is easy to believe; a pull that quietly // discarded somebody's uncommitted work is the failure that matters, and every plugin directory on a @@ -53,3 +53,27 @@ describe('pullPlugin refuses rather than guesses', () => { } }); }); + +// ── Enabling must survive a refusal to pull ── +// +// `enablePlugin` refreshes the checkout before turning a plugin on, so that a plugin which sat disabled +// for a week does not come back running the code it was switched off with. The risk in that is obvious: +// the pull is refused for reasons that say nothing about whether the plugin can run — a dirty tree, an +// unreachable forge, a directory that was copied in rather than cloned — and enabling is meant to be the +// reversible half of a pair with disable. +// +// So this asserts the pull refusal is REPORTED and then stepped past. The enable itself still fails here, +// because a scratch directory has no install row — and that is the point: the failure that comes back is +// about being installed, not about git, which is only possible if the refusal did not short-circuit. +describe('enablePlugin steps past a pull it cannot do', () => { + test('a directory that is not a git checkout still reaches the enable', async () => { + // Named without the word "git" in it, so the last assertion below is about the error and not about + // the fixture's own name. + scratch('test-enable-plain'); + const r = await enablePlugin('test-enable-plain'); + + expect(r.steps.some((s) => s.includes('not refreshed') && s.includes('not a git checkout'))).toBe(true); + expect(r.error).toContain('not installed'); + expect(r.error).not.toContain('git'); + }); +}); diff --git a/src/servers/plugins/update.ts b/src/servers/plugins/update.ts index 9d6c227a..a2862f2f 100644 --- a/src/servers/plugins/update.ts +++ b/src/servers/plugins/update.ts @@ -2,7 +2,7 @@ import { existsSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; import { PLUGINS_DIR } from './discover'; import { OFFICER_ITEMS_DIR, PLATFORM_DIR } from '../data-path'; -import { installPlugin } from './install'; +import { installPlugin, setPluginRunning } from './install'; import type { OnStep, PluginActionResult } from './install'; import type { OnLog } from './run-logged'; @@ -124,6 +124,65 @@ export async function updatePlugin( return { ...result, steps: [...steps, ...result.steps] }; } +/** + * Enable a plugin, refreshing it first. + * + * ── Why enabling pulls ── + * + * A disabled plugin keeps its directory, and the directory is what the next start actually runs. So the + * longer something stays disabled the more certainly it is stale, and turning it back on would quietly + * run whatever was current on the day it was switched off. Deleting no longer has that problem — it takes + * the directory with it, so installing is always a fresh clone — which leaves disable/enable as the one + * remaining way to run old code without being told. + * + * That is not hypothetical either. It is the same shape as the jellyfin install on alpha, which re-applied + * a two-day-old checkout and reported success at every step. + * + * ── The pull cannot be allowed to fail the enable ── + * + * `updatePlugin` returns `ok: false` when the pull is refused, and it is refused for reasons that have + * nothing to do with whether the plugin can run: a dirty working tree (every plugin directory on a + * developer's machine is one), a forge that cannot be reached, a checkout that is not a git repository at + * all. Enabling is the reversible half of a pair with disable, and a switch that will not turn back on + * because a network is down is worse than a plugin running last week's code. + * + * So the refresh is best-effort and SAYS which of the three happened, and the enable runs either way. The + * re-apply is likewise allowed to fail without taking the enable with it — the code already on disk was + * running before it was disabled, so falling back to it is a known-good state rather than a guess. + */ +export async function enablePlugin( + appName: string, + onStep?: OnStep, + userId?: number, + onLog?: OnLog, +): Promise { + const steps: string[] = []; + const say = async (line: string) => { + steps.push(line); + await onStep?.(line); + }; + + const pulled = await pullPlugin(appName, onLog); + const short = (sha: string | null) => sha?.slice(0, 7) ?? '?'; + + if (!pulled.ok) { + // Not an error here, only a fact worth stating: this is running the code that is on disk. + await say(`not refreshed — ${pulled.error}`); + } else if (pulled.updated) { + await say(`pulled ${short(pulled.before)} → ${short(pulled.after)}`); + const applied = await installPlugin(appName, onStep, userId, onLog); + steps.push(...applied.steps); + if (!applied.ok) await say(`re-apply failed (${applied.error}) — enabling what is on disk instead`); + } else { + await say('already at the latest commit'); + } + + // Always. `recordPluginInstall` deliberately never touches `enabled`, so the re-apply above cannot have + // switched it on as a side effect — which is correct, and means this line is the one that enables. + const enabled = await setPluginRunning(appName, true, onStep); + return { ...enabled, steps: [...steps, ...enabled.steps] }; +} + // ── The capabilities repository ── // // `capabilities/` is the file-based item store, and most of what is in it belongs to the owner. A small diff --git a/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx b/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx index cea195fe..a2893dc4 100644 --- a/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx +++ b/src/workspaces/officerdev/src/apps/Plugins/PluginDetail.tsx @@ -540,7 +540,7 @@ export const PluginDetail = () => { rather than leaving someone to infer it from two buttons sitting next to each other. */}

{plugin.installed - ? 'Disabling unmounts its routes and stops its sidecar, and changes nothing else — it is reversible in one click. Deleting needs it disabled first.' + ? 'Disabling unmounts its routes and stops its sidecar, and changes nothing else. Enabling pulls the latest version first, so a plugin that was off for a while does not come back running old code. Deleting needs it disabled first.' : 'Not installed, but the code is still on this machine — which is why the marketplace does not offer it. Installing again reuses this directory as it stands; deleting gets you a fresh clone.'}