diff --git a/.gitignore b/.gitignore index 98e8c221..7da1da4a 100644 --- a/.gitignore +++ b/.gitignore @@ -105,6 +105,13 @@ src/databases/officer_db/migrations/ # to the platform; the plugins do not. /plugins/*/ +# Generated by src/servers/plugins/npm-deps.ts from every plugin's `npmDependencies`, and installed into +# plugins/node_modules/. It describes whichever plugins are on THIS machine, so committing one would +# claim a dependency set nobody chose. The rule above only ignores directories, so these need naming. +/plugins/package.json +/plugins/bun.lock +/plugins/bun.lockb + # Visual baselines for the theme extraction — large, machine-specific, regenerate with scripts/theme-snapshot.ts .snapshots/ diff --git a/src/servers/plugins/install.ts b/src/servers/plugins/install.ts index d327d1ce..e26c6c46 100644 --- a/src/servers/plugins/install.ts +++ b/src/servers/plugins/install.ts @@ -24,7 +24,7 @@ import { generatePluginSchemas, pushPluginSchema } from './schema'; import { declaredTables, dropTables, existingTables } from './tables'; import type { OnLog } from './run-logged'; import { installDependencies, installRelease, manualInstallHint, reportDependencies, stillMissing } from './os-deps'; -import { installNpmDependencies, npmManifestFor, npmWouldBlockUpdates, verifyNpmDependencies } from './npm-deps'; +import { anyPluginWantsPackages, installNpmDependencies, unresolvableDependencies } from './npm-deps'; import { mountPrefix, type DiscoveredPlugin } from './manifest'; // The install runner: the four verbs, each as a short ordered list of effects. @@ -269,44 +269,48 @@ export async function installPlugin( await step(steps, onStep, `dependencies: ${deps.dependencies.map((d) => d.binary).join(', ')} already present`); } - // ── npm packages, in the plugin's own directory ── + // ── npm packages, shared across every plugin ── // // Beside the OS dependencies above and verified the same way, because the same failure is available: - // an install that exits 0 and still leaves nothing the sidecar can import. Atelier hit exactly that - // on 2026-08-20 — `sharp` in the PLATFORM's package.json, alpha never told to `bun install`, and the + // an install that exits 0 and still leaves nothing the sidecar can import. Atelier hit exactly that on + // 2026-08-20 — `sharp` in the PLATFORM's package.json, alpha never told to `bun install`, and the // sidecar crash-looped on a plugin that had reported a clean install. - if (npmManifestFor(plugin)) { - await step(steps, onStep, "packages: installing from the plugin's own package.json"); - const npm = await installNpmDependencies(plugin); - if (!npm.ok) { - return { ok: false, appName, steps, error: `package install failed: ${npm.output || 'no output'}` }; - } + // + // Generated from every plugin ON DISK rather than from this one, so a shared `sharp` is installed once + // and two plugins wanting it converge instead of carrying a copy each. + { + const { plugins: onDisk } = await discoverPlugins(); + if (anyPluginWantsPackages(onDisk)) { + await step(steps, onStep, 'packages: resolving npm dependencies declared by plugins'); + const npm = await installNpmDependencies(onDisk); - // Resolved, not assumed. `sharp` resolves to a platform-specific binary package, so a lockfile that - // installs cleanly on one machine can still leave nothing importable on another — which is the - // shape of the failure this whole block exists to stop shipping. - const unresolvable = await verifyNpmDependencies(plugin); - if (unresolvable.length) { - return { - ok: false, - appName, - steps, - error: `packages installed, but ${unresolvable.join(', ')} cannot be resolved from ${plugin.dir}`, - }; - } - await step(steps, onStep, 'packages: installed and resolvable'); + for (const [name, askers] of Object.entries(npm.conflicts)) { + // Named, not silently intersected. Two plugins disagreeing about a version is a thing their + // authors need to know, and the install either resolves it or fails on the next line. + await step( + steps, + onStep, + `packages: ${name} requested as ${askers.map((a) => `${a.range} (${a.appName})`).join(', ')} — intersected`, + ); + } - // Said now, while there is something to be done about it. `pullPlugin` refuses on any uncommitted - // change, so an unignored `node_modules/` turns the Update button into a permanent "N uncommitted - // changes" about a directory the platform created — and the person seeing that message has no - // reason to connect it to a package.json shipped three versions ago. - if (await npmWouldBlockUpdates(plugin)) { - await step( - steps, - onStep, - 'packages: WARNING — this plugin does not ignore node_modules/, so Update will refuse as dirty. ' + - 'Add `node_modules/` to its .gitignore.', - ); + if (!npm.ok) { + return { ok: false, appName, steps, error: `package install failed: ${npm.output || 'no output'}` }; + } + + // Resolved, not assumed. `sharp` resolves to a platform-specific binary package, so an install + // that succeeds on one machine can still leave nothing importable on another — which is the shape + // of the failure this whole block exists to stop shipping. + const unresolvable = unresolvableDependencies(plugin); + if (unresolvable.length) { + return { + ok: false, + appName, + steps, + error: `packages installed, but ${unresolvable.join(', ')} cannot be resolved from ${plugin.dir}`, + }; + } + await step(steps, onStep, 'packages: installed and resolvable'); } } diff --git a/src/servers/plugins/manifest.ts b/src/servers/plugins/manifest.ts index 0910257e..c26d8341 100644 --- a/src/servers/plugins/manifest.ts +++ b/src/servers/plugins/manifest.ts @@ -244,6 +244,25 @@ export type PluginManifest = { * that runs and quietly produces nothing. */ osDependencies?: OsDependency[]; + + /** + * npm packages this plugin imports. Absent means it needs none, which is every plugin but Atelier. + * + * Ranges, not pins, and deliberately: every plugin's declarations are merged into ONE shared install at + * `plugins/node_modules/`, so two plugins asking for `^0.33.5` and `^0.33.9` share one copy rather than + * carrying 50 MB of the same platform binaries twice. Where ranges differ they are intersected — both + * plugins have to work — so incompatible requirements fail the install loudly instead of handing one + * plugin a version the other cannot use. + * + * This is the one place in the codebase where a dependency is resolved at install time rather than from + * a committed lockfile. That is a real cost against the rule `CLAUDE.md` states, taken knowingly: the + * alternative is a lockfile per plugin and no sharing at all. What it buys back is that every package a + * plugin can pull is DECLARED here, visible before install, and the merged file is regenerated from + * these manifests rather than edited — so nothing can appear in it without a plugin having asked. + * + * See `npm-deps.ts`. + */ + npmDependencies?: Record; }; /** What the platform knows about a plugin on disk: its manifest, plus everything the tree said. */ diff --git a/src/servers/plugins/npm-deps.test.ts b/src/servers/plugins/npm-deps.test.ts new file mode 100644 index 00000000..86a02b73 --- /dev/null +++ b/src/servers/plugins/npm-deps.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, test } from 'bun:test'; +import { mergeDependencies } from './npm-deps'; +import type { DiscoveredPlugin } from './manifest'; + +const plugin = (appName: string, npmDependencies?: Record) => + ({ appName, dir: `/tmp/${appName}`, manifest: { npmDependencies } }) as unknown as DiscoveredPlugin; + +describe('merging what plugins ask for', () => { + test('one plugin, one range', () => { + expect(mergeDependencies([plugin('atelier', { sharp: '^0.33.5' })]).merged).toEqual({ sharp: '^0.33.5' }); + }); + + test('two plugins asking for the same range share one entry, and it is not a conflict', () => { + const { merged, conflicts } = mergeDependencies([ + plugin('atelier', { sharp: '^0.33.5' }), + plugin('photos', { sharp: '^0.33.5' }), + ]); + expect(merged).toEqual({ sharp: '^0.33.5' }); + expect(conflicts).toEqual({}); + }); + + // A SPACE, not `||`. npm and bun read space-separated ranges as an intersection: the resolved version + // must satisfy both. `||` would mean "either", which would let one plugin get a version the other + // cannot import — the silent version of this failure, and the one worth designing against. + test('different ranges intersect, and are reported', () => { + const { merged, conflicts } = mergeDependencies([ + plugin('atelier', { sharp: '^0.33.5' }), + plugin('photos', { sharp: '^0.33.9' }), + ]); + expect(merged.sharp).toBe('^0.33.5 ^0.33.9'); + expect(conflicts.sharp).toEqual([ + { appName: 'atelier', range: '^0.33.5' }, + { appName: 'photos', range: '^0.33.9' }, + ]); + }); + + test('plugins declaring nothing contribute nothing', () => { + expect(mergeDependencies([plugin('offscale'), plugin('music', {})]).merged).toEqual({}); + }); + + // Sorted, so the generated file does not churn between installs and a diff means something changed. + test('output is stable regardless of plugin order', () => { + const a = mergeDependencies([plugin('zed', { b: '1' }), plugin('alpha', { a: '1' })]); + const b = mergeDependencies([plugin('alpha', { a: '1' }), plugin('zed', { b: '1' })]); + expect(Object.keys(a.merged)).toEqual(Object.keys(b.merged)); + expect(Object.keys(a.merged)).toEqual(['a', 'b']); + }); +}); diff --git a/src/servers/plugins/npm-deps.ts b/src/servers/plugins/npm-deps.ts index 7aef2ffb..0cf43431 100644 --- a/src/servers/plugins/npm-deps.ts +++ b/src/servers/plugins/npm-deps.ts @@ -1,137 +1,153 @@ import { existsSync } from 'node:fs'; import { join } from 'node:path'; +import { PLATFORM_DIR } from '../data-path'; import type { DiscoveredPlugin } from './manifest'; -// A plugin's own npm dependencies, installed into its own directory. +// npm packages a plugin needs, declared in its manifest and installed ONCE for all plugins. // // ── The gap this closes ── // -// `osDependencies` covers binaries a plugin needs on PATH — `ffmpeg`, `radicale`, `tea`. There was -// nothing for a JavaScript package, so a plugin needing one had to add it to the PLATFORM's -// `package.json`. Atelier did exactly that with `sharp` on 2026-08-20, carefully and with the frozen -// lockfile dance documented, and it still went wrong the first time it met a second machine: alpha -// pulled the `package.json`, nobody ran `bun install`, and `officer-atelier` crash-looped on -// `Cannot find package 'sharp'`. The plugin reported a successful install and then could not start. +// `osDependencies` covers binaries on PATH — `ffmpeg`, `radicale`, `tea`. There was nothing for a +// JavaScript package, so a plugin needing one had to add it to the PLATFORM's `package.json`. Atelier did +// that with `sharp`, and it broke on the first machine that was not the one it was written on: alpha +// pulled the `package.json`, nobody ran `bun install`, and the sidecar crash-looped on +// `Cannot find package 'sharp'` — after the install flow had reported success. // -// Three things were wrong with that shape, and none of them are the agent's fault: +// ── Why one shared install, not one per plugin ── // -// - **The platform carried a dependency only one plugin needs.** Uninstall the plugin and `sharp` is -// still in the platform's lockfile, with nothing recording why. -// - **Installing a plugin could not install it.** The install flow ran, said "installed", and left a -// process that could not boot. Nothing in the pipeline was positioned to notice. -// - **It forced the platform's lockfile open.** `frozenLockfile` exists so an unexplained lockfile -// change is a signal; a plugin needing a package makes that signal routine. +// The first version gave each plugin its own `package.json` and `node_modules/`. That works and it is +// what a git repository normally does, but `sharp` is ~50 MB of platform binaries and it is the obvious +// dependency for anything touching images — a second and third plugin wanting it would mean a second and +// third copy, and no version they could converge on. // -// ── The shape ── +// So: every plugin's `npmDependencies` are merged into ONE generated `plugins/package.json`, installed +// into `plugins/node_modules/`. Bun resolves node_modules up the directory tree, so that one directory +// serves every plugin and none of the platform's own code. // -// A plugin that needs packages ships its own `package.json` and its own lockfile, and they are committed -// in the plugin's OWN repository — which is the thing a plugin already is. Bun resolves `node_modules` -// up the directory tree, so `plugins//node_modules/` serves that plugin's code and nothing else. -// Uninstalling removes the directory and the packages with it. +// ── What is given up, deliberately ── // -// The presence of the file is the declaration. No manifest field, for the same reason `sidecar/index.ts` -// has none: a second place to say the same thing is a second place for it to be wrong. +// A committed lockfile. Ranges are the point — two plugins asking for `^0.33.5` and `^0.33.9` should +// share one install — and a range resolves per machine. `CLAUDE.md` is emphatic about frozen lockfiles +// and the 2026-08-04 supply-chain compromise, so this is a real cost and it is worth naming: plugin +// packages are resolved at install time, like `osDependencies` are, and unlike the platform's own. +// +// The mitigation is that the surface is small and declared. Every package a plugin can pull is written in +// its manifest, visible before install, and the merged file is regenerated from those manifests rather +// than edited — so a package appearing there without a plugin asking for it is not something that can +// happen quietly. -/** `plugins//package.json`, or null when the plugin needs no packages — the ordinary case. */ -export const npmManifestFor = (plugin: DiscoveredPlugin): string | null => { - const path = join(plugin.dir, 'package.json'); - return existsSync(path) ? path : null; +/** The generated file. Regenerated from manifests on every install — never edited, never committed. */ +export const PLUGIN_PACKAGE_JSON = join(PLATFORM_DIR, 'plugins', 'package.json'); +const PLUGINS_DIR = join(PLATFORM_DIR, 'plugins'); + +export type MergedDependencies = { + /** package → the range actually written. */ + merged: Record; + /** package → which plugins asked, and for what. Only entries where they disagreed. */ + conflicts: Record; }; -export type NpmInstallResult = { ok: boolean; ran: boolean; output: string }; - /** - * Will the packages this is about to install make the plugin un-updatable? + * Merge every plugin's declared packages into one set of ranges. * - * `pullPlugin` refuses on ANY uncommitted change — `git status --porcelain` non-empty — because it is - * about to `merge --ff-only` over the checkout and will not overwrite someone's work. `node_modules/` - * that nothing ignores is an uncommitted change, so a plugin shipping `package.json` without ignoring it - * installs perfectly once and can then never be updated again. The Update button just says "N uncommitted - * changes" forever, about a directory the platform itself created. + * Where two plugins ask for the same package with DIFFERENT ranges, the ranges are joined with a space — + * which npm and bun read as an intersection, not an alternative. That is the honest semantics: both + * plugins have to work, so the resolved version must satisfy both. Compatible ranges (`^0.33.5` and + * `^0.33.9`) resolve to one shared install; genuinely incompatible ones fail the install with bun's own + * message rather than silently giving one plugin a version the other cannot use. * - * Asked with `git check-ignore`, not by reading `.gitignore`: the rule may be nested, global, or in - * `.git/info/exclude`, and every one of those counts. - * - * Not fixed automatically. Writing a line into the plugin's own `.gitignore` would be the platform - * editing a repository it does not own — and would itself dirty the checkout, which is the very thing - * being avoided. + * Sorted throughout, so the generated file does not churn between installs. */ -export async function npmWouldBlockUpdates(plugin: DiscoveredPlugin): Promise { - if (!npmManifestFor(plugin)) return false; - if (!existsSync(join(plugin.dir, '.git'))) return false; // not a checkout — nothing to update from - try { - const proc = Bun.spawn(['git', 'check-ignore', '-q', 'node_modules'], { - cwd: plugin.dir, - stdout: 'ignore', - stderr: 'ignore', - }); - // 0 = ignored, 1 = not ignored, anything else = git could not answer, which is not a finding. - return (await proc.exited) === 1; - } catch { - return false; - } -} +export function mergeDependencies(plugins: DiscoveredPlugin[]): MergedDependencies { + const asked: Record = {}; -/** - * Install a plugin's packages, in its own directory. - * - * **Frozen.** The plugin ships a lockfile beside its `package.json`, so this resolves from that and - * nothing else — the same discipline the platform holds itself to, and for the same reason: a package - * that changed without anyone choosing it is exactly what a supply-chain compromise looks like. A plugin - * whose lockfile does not satisfy its `package.json` fails here rather than quietly resolving something - * newer. - * - * Never throws. A plugin whose packages fail to install is broken, but so is refusing the whole install - * over it — the caller decides, and it has more context than this does. - */ -export async function installNpmDependencies(plugin: DiscoveredPlugin): Promise { - if (!npmManifestFor(plugin)) return { ok: true, ran: false, output: '' }; - - try { - const proc = Bun.spawn(['bun', 'install', '--frozen-lockfile'], { - cwd: plugin.dir, - stdout: 'pipe', - stderr: 'pipe', - // The plugin's own directory is the root of this install. `--frozen-lockfile` is passed explicitly - // rather than relied upon from a bunfig the plugin may not ship. - }); - const [out, err] = await Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]); - const code = await proc.exited; - return { ok: code === 0, ran: true, output: `${out}${err}`.trim() }; - } catch (error) { - return { ok: false, ran: true, output: error instanceof Error ? error.message : String(error) }; - } -} - -/** - * Can this plugin's code actually resolve what it declared? - * - * Asked AFTER installing, because "the install command exited 0" and "the sidecar can import it" are - * different claims, and it was the second one that failed on alpha. `sharp` in particular resolves to a - * platform-specific binary package, so a lockfile that installs cleanly on one machine can still leave - * nothing importable on another. - */ -export async function verifyNpmDependencies(plugin: DiscoveredPlugin): Promise { - const manifestPath = npmManifestFor(plugin); - if (!manifestPath) return []; - - let names: string[]; - try { - const parsed = (await Bun.file(manifestPath).json()) as { dependencies?: Record }; - names = Object.keys(parsed.dependencies ?? {}); - } catch { - return []; - } - - const missing: string[] = []; - for (const name of names) { - // `bun.resolveSync` from the plugin's directory answers the question the sidecar will ask, rather - // than the question "is there a folder with the right name". - try { - Bun.resolveSync(name, plugin.dir); - } catch { - missing.push(name); + for (const plugin of [...plugins].sort((a, b) => a.appName.localeCompare(b.appName))) { + const deps = plugin.manifest.npmDependencies ?? {}; + for (const [name, range] of Object.entries(deps)) { + (asked[name] ??= []).push({ appName: plugin.appName, range }); } } - return missing; + + const merged: Record = {}; + const conflicts: MergedDependencies['conflicts'] = {}; + + for (const name of Object.keys(asked).sort()) { + const ranges = [...new Set(asked[name]!.map((a) => a.range))]; + merged[name] = ranges.join(' '); + if (ranges.length > 1) conflicts[name] = asked[name]!; + } + + return { merged, conflicts }; } + +export type NpmInstallResult = { + ok: boolean; + ran: boolean; + output: string; + conflicts: MergedDependencies['conflicts']; +}; + +/** + * Write `plugins/package.json` from every plugin's manifest and install it. + * + * Generated from the plugins ON DISK, like `generatePluginSchemas` — not from what is installed. A plugin + * whose directory is present but which nobody has enabled still gets its packages, which is the same + * safety property that file documents: the thing that decides is what is there to import. + * + * Never throws. + */ +export async function installNpmDependencies(plugins: DiscoveredPlugin[]): Promise { + const { merged, conflicts } = mergeDependencies(plugins); + + if (Object.keys(merged).length === 0) { + // Nothing wants packages. Leave any previous file alone rather than writing an empty one — an empty + // `package.json` beside a populated `node_modules` reads as "these were installed by nobody". + return { ok: true, ran: false, output: '', conflicts }; + } + + const body = { + name: '@officer/plugin-dependencies', + private: true, + '//': 'GENERATED from every plugin manifest by src/servers/plugins/npm-deps.ts. Not edited, not committed.', + dependencies: merged, + }; + + try { + await Bun.write(PLUGIN_PACKAGE_JSON, `${JSON.stringify(body, null, 2)}\n`); + + const proc = Bun.spawn(['bun', 'install'], { cwd: PLUGINS_DIR, stdout: 'pipe', stderr: 'pipe' }); + const [out, err] = await Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text()]); + const code = await proc.exited; + return { ok: code === 0, ran: true, output: `${out}${err}`.trim(), conflicts }; + } catch (error) { + return { ok: false, ran: true, output: error instanceof Error ? error.message : String(error), conflicts }; + } +} + +/** + * Can this plugin's code actually resolve what its manifest declared? + * + * Asked AFTER installing, because "the install exited 0" and "the sidecar can import it" are different + * claims and it was the second one that failed on alpha. `sharp` resolves to a platform-specific binary + * package, so a lockfile that installs cleanly on one machine can leave nothing importable on another. + * + * Resolved FROM THE PLUGIN'S OWN DIRECTORY, which is the question its sidecar will ask. + */ +export function unresolvableDependencies(plugin: DiscoveredPlugin): string[] { + const names = Object.keys(plugin.manifest.npmDependencies ?? {}); + return names.filter((name) => { + try { + Bun.resolveSync(name, plugin.dir); + return false; + } catch { + return true; + } + }); +} + +/** Whether anything on this machine declares packages at all. */ +export const anyPluginWantsPackages = (plugins: DiscoveredPlugin[]): boolean => + plugins.some((p) => Object.keys(p.manifest.npmDependencies ?? {}).length > 0); + +/** For the uninstall path: the generated file exists and can be regenerated without the removed plugin. */ +export const pluginPackageJsonExists = (): boolean => existsSync(PLUGIN_PACKAGE_JSON); diff --git a/src/servers/plugins/update.test.ts b/src/servers/plugins/update.test.ts index ef795360..4bd9e265 100644 --- a/src/servers/plugins/update.test.ts +++ b/src/servers/plugins/update.test.ts @@ -3,8 +3,6 @@ import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { PLUGINS_DIR } from './discover'; import { pullPlugin } from './update'; -import { discoverPlugins } from './discover'; -import { npmManifestFor, npmWouldBlockUpdates } from './npm-deps'; // 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 @@ -55,19 +53,3 @@ describe('pullPlugin refuses rather than guesses', () => { } }); }); - -describe('npm packages and the Update button', () => { - // The trap: `pullPlugin` refuses on ANY uncommitted change, and `node_modules/` that nothing ignores - // is one. So a plugin shipping `package.json` without ignoring it installs perfectly once and can then - // never be updated — the button says "N uncommitted changes" forever, about a directory the platform - // itself created. Every plugin that exists today ignores it, which is luck rather than a rule. - test('atelier — the only plugin with packages — ignores node_modules', async () => { - const { plugins } = await discoverPlugins(); - const withPackages = plugins.filter((p) => npmManifestFor(p)); - - // Not asserting that atelier specifically is installed — a checkout without it is legitimate. - for (const plugin of withPackages) { - expect(await npmWouldBlockUpdates(plugin), `${plugin.appName} would block its own updates`).toBe(false); - } - }); -});