commit 4547869a624f4b0df4ddb2d569e20c12f87564ea Author: Jason Woltje Date: Thu Oct 15 17:32:46 2026 -0500 mosaic plugins set: a patch changes only the kinds it names; null removes a kind (vikunja:19/309, row 145) Jason-approved merge, rebased: git diff f4d7b91a9..0e7d1d8b8 from work/vikunja-19-309, stacked on 305. Business reviewer verdict dfd44af2 approve on 0e7d1d8b8; Jason resolved merge decision 12a20e47 via the CLI. No conflict. packages/plugins/README.md, which refactor also changed, merged with the source commit's +/- lines; the other four paths are the source blobs. Filbert reviews the rebased diff. Co-Authored-By: Claude Opus 5.5 --- packages/bus/tests/plugins-verb.test.mjs | 26 ++++++++++++++++++++ packages/business/src/plugin-settings.mjs | 21 ++++++++++++---- packages/business/tests/plugin-settings.test.mjs | 31 +++++++++++++++++++++++- packages/cli/src/plugins.mjs | 4 ++- packages/plugins/README.md | 6 +++-- 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/packages/bus/tests/plugins-verb.test.mjs b/packages/bus/tests/plugins-verb.test.mjs index aeec1663d..74f8d0a65 100644 --- a/packages/bus/tests/plugins-verb.test.mjs +++ b/packages/bus/tests/plugins-verb.test.mjs @@ -64,3 +64,29 @@ test('runtime: the switch changes the organization file, the receipt carries the assert.deepEqual(log().at(-1).changes.map((c) => [c.path, c.old, c.new]), [['plugins.search-source', 'searxng-url', 'harness-builtin']]); assert.equal(next.plugins['search-source'], 'harness-builtin'); }); + +test('vikunja:19/309 through the broker, a patch switches the kinds it names and keeps the rest; null removes one kind and is trailed', async (t) => { + const { startBroker } = await import('../src/runtime.mjs'); + const { businessDoc, rolesCopy, systemFor, writeJson } = await import('../../business/tests/helpers.mjs'); + const root = mkdtempSync(join(tmpdir(), 'plugins-merge-')); + const rolesDir = rolesCopy(root); + writeJson(join(root, 'businesses/demo.json'), { ...businessDoc(root, 'demo'), plugins: { 'usage-source': ['meter'] } }); + mkdirSync(join(root, 'data'), { mode: 0o700 }); + const bus = await startBroker({ dataRoot: join(root, 'data'), businesses: { demo: { id: 'demo', human: 'jason', arbiters: { technical: 'coder', delivery: 'coder' }, roles: { coder: { authority: { withinRole: [], crossRole: [] } } } } }, organization: { dir: root, rolesDir, system: systemFor(root) } }); + t.after(async () => { await bus.close(); rmSync(root, { recursive: true, force: true }); }); + const human = bus.broker.bindHuman({ business: 'demo', human: 'jason', via: 'cli', outsideAgent: true }); + const log = () => readFileSync(join(root, 'businesses/.history/demo/changes.jsonl'), 'utf8').trim().split('\n').filter(Boolean).map(JSON.parse); + const set = (revision, plugins) => bus.broker.request(human, { verb: 'plugins.set', args: { revision, plugins } }); + let view = bus.broker.request(human, { verb: 'plugins' }); + assert.deepEqual(view.plugins, { 'usage-source': ['meter'] }); + view = set(view.revision, { 'search-source': 'searxng-url' }); + assert.deepEqual(view.plugins, { 'usage-source': ['meter'], 'search-source': 'searxng-url' }, 'usage-source kept'); + assert.deepEqual(log().at(-1).changes.map((c) => c.path), ['plugins.search-source']); + view = set(view.revision, { 'usage-source': null }); + assert.deepEqual(view.plugins, { 'search-source': 'searxng-url' }); + assert.deepEqual(log().at(-1).changes, [{ path: 'plugins.usage-source', old: ['meter'], new: null, oldPresent: true, newPresent: false }]); + const code = (plugins) => { try { set(view.revision, plugins); } catch (e) { return e.code; } return null; }; + assert.equal(code({ 'made-up': null }), 'invalid-plugins'); + assert.equal(code({}), 'invalid-plugins'); + assert.equal(code({ 'search-source': 'no-such-module' }), 'unknown-plugin'); +}); diff --git a/packages/business/src/plugin-settings.mjs b/packages/business/src/plugin-settings.mjs index 6a0264419..e6b76b09a 100644 --- a/packages/business/src/plugin-settings.mjs +++ b/packages/business/src/plugin-settings.mjs @@ -2,10 +2,10 @@ // REQ-ROLE-3). It is the only writer of that key: no broker verb, mosaic // command, Settings form or chief-of-staff write reaches it (sage-0301). // Writes go through the organization writer, which logs old and new names -// like a limit change. A refused write throws a code and logs nothing. +// like a limit change. A write is a patch per kind (see set()): kinds it doesn't name are kept. A refused write throws a code and logs nothing. import { organizationWriter, OrganizationError } from "./organization.mjs"; import { isPlainObject } from "./util.mjs"; -import { PluginError, checkPlugins } from "../../plugins/src/kinds.mjs"; +import { KINDS, PluginError, checkPlugins } from "../../plugins/src/kinds.mjs"; // `exists(kind, name)`, when given, refuses a name no module answers to (the host passes the // loader's hasModule). Without it the setter checks only the shape, as before. @@ -31,21 +31,32 @@ export function pluginSettings({ dir, rolesDir, exists = null }) { if (proto !== Object.prototype && proto !== null) { throw new OrganizationError("invalid-plugins", "plugins must be a plain object with one key per kind"); } + // The patch names the kinds it changes (vikunja:19/309): a kind with a value is switched to it, + // a kind with null is removed, and every kind it doesn't name keeps its setting. An empty patch + // changes nothing and is refused rather than written as a no-op. + const patch = Object.entries(args.plugins); + if (patch.length === 0) throw new OrganizationError("invalid-plugins", "the patch names no kind; to remove a kind give it null"); + const removals = patch.filter(([, setting]) => setting === null).map(([kind]) => kind); + const sets = Object.fromEntries(patch.filter(([, setting]) => setting !== null)); + for (const kind of removals) { + if (!Object.hasOwn(KINDS, kind)) throw new OrganizationError("invalid-plugins", `plugins names unknown kind ${JSON.stringify(kind)}; adding a kind is a core change`); + } try { - checkPlugins(args.plugins, "plugins"); + checkPlugins(sets, "plugins"); } catch (e) { if (e instanceof PluginError) throw new OrganizationError("invalid-plugins", e.message); throw e; } if (exists) { - for (const [kind, setting] of Object.entries(args.plugins)) { + for (const [kind, setting] of Object.entries(sets)) { for (const name of Array.isArray(setting) ? setting : [setting]) { if (!exists(kind, name)) throw new OrganizationError("unknown-plugin", `no ${kind} module named ${name} in this tree`); } } } const next = structuredClone(writer.read(id).document); - next.plugins = structuredClone(args.plugins); + next.plugins = { ...structuredClone(next.plugins ?? {}), ...structuredClone(sets) }; + for (const kind of removals) delete next.plugins[kind]; const receipt = writer.write(id, next, { revision: args.revision, actor }); return { ...read(id), transaction: receipt.transaction }; } diff --git a/packages/business/tests/plugin-settings.test.mjs b/packages/business/tests/plugin-settings.test.mjs index 756c7957e..b770ced6b 100644 --- a/packages/business/tests/plugin-settings.test.mjs +++ b/packages/business/tests/plugin-settings.test.mjs @@ -61,7 +61,36 @@ test("REQ-PLUG-1 REQ-ROLE-3 the setter writes plugins through the organization w // A null-prototype object is still a plain JSON object. const bare = Object.assign(Object.create(null), { "limit-check": ["cap"] }); f.settings.set("acme", { revision: f.settings.read("acme").revision, plugins: bare }, "jason"); - assert.deepEqual(JSON.parse(readFileSync(f.file, "utf8")).plugins, { "limit-check": ["cap"] }); + // The patch names limit-check only, so usage-source keeps its setting (vikunja:19/309). + assert.deepEqual(JSON.parse(readFileSync(f.file, "utf8")).plugins, { "usage-source": ["meter", "other"], "limit-check": ["cap"] }); +}); + +test("vikunja:19/309 a patch changes only the kinds it names; null removes one kind, explicitly and in the trail", () => { + const f = fixture(); + let view = f.settings.read("acme"); + view = f.settings.set("acme", { revision: view.revision, plugins: { "usage-source": ["meter"], "limit-check": ["cap"] } }, "jason"); + view = f.settings.set("acme", { revision: view.revision, plugins: { "limit-check": ["cap", "other"] } }, "jason"); + assert.deepEqual(view.plugins, { "usage-source": ["meter"], "limit-check": ["cap", "other"] }, "usage-source untouched"); + assert.deepEqual(events(f).at(-1).changes.map(c => c.path), ["plugins.limit-check"], "only the named kind is in the receipt"); + view = f.settings.set("acme", { revision: view.revision, plugins: { "usage-source": null } }, "jason"); + assert.deepEqual(view.plugins, { "limit-check": ["cap", "other"] }, "only usage-source removed"); + assert.deepEqual(events(f).at(-1).changes, [{ path: "plugins.usage-source", old: ["meter"], new: null, oldPresent: true, newPresent: false }]); + // Removing a kind that isn't set changes nothing in the file's plugins and is not an error. + view = f.settings.set("acme", { revision: view.revision, plugins: { "usage-source": null } }, "jason"); + assert.deepEqual(view.plugins, { "limit-check": ["cap", "other"] }); +}); + +test("vikunja:19/309 an unknown kind (set or removed), a bad module, an empty patch and null on the wrong shape are refused, nothing written", () => { + const f = fixture(); + const { revision } = f.settings.read("acme"); + const bytes = readFileSync(f.file, "utf8"); + for (const plugins of [{ "made-up": null }, { "made-up": ["x"] }, {}, { "usage-source": null, "made-up": ["x"] }, { "usage-source": ["Bad"], "limit-check": null }]) { + assert.throws(() => f.settings.set("acme", { revision, plugins }, "jason"), e => e instanceof OrganizationError && e.code === "invalid-plugins", JSON.stringify(plugins)); + } + const strict = pluginSettings({ dir: f.root, rolesDir: f.rolesDir, exists: () => false }); + assert.throws(() => strict.set("acme", { revision, plugins: { "usage-source": ["ghost"], "limit-check": null } }, "jason"), e => e.code === "unknown-plugin"); + assert.equal(readFileSync(f.file, "utf8"), bytes); + assert.deepEqual(events(f), []); }); test("REQ-PLUG-1 REQ-ROLE-3 the writer checks the bytes it writes: a Date in plugins is refused there too", () => { diff --git a/packages/cli/src/plugins.mjs b/packages/cli/src/plugins.mjs index 0afb3f522..fc55e624f 100644 --- a/packages/cli/src/plugins.mjs +++ b/packages/cli/src/plugins.mjs @@ -21,7 +21,9 @@ function parse(args) { return out; } -// PATCH is { "revision": , "plugins": { "": } }. +// PATCH is { "revision": , "plugins": { "": } }. It names +// the kinds it changes: a kind with a value is switched to it, a kind with null is removed, and a +// kind it doesn't name keeps its setting (vikunja:19/309). An empty patch is refused. export async function pluginsCommand(args, io, callFor) { const parsed = parse(args); const verb = parsed.positional[0] ?? "view"; diff --git a/packages/plugins/README.md b/packages/plugins/README.md index 1913004e3..8bde4df0b 100644 --- a/packages/plugins/README.md +++ b/packages/plugins/README.md @@ -47,9 +47,11 @@ calls the kind. 5. Switch it on in the organization file's `plugins` object, under its kind: `mosaic plugins` shows the setting and its revision, and `mosaic plugins set --file PATCH` (`{ "revision": ..., "plugins": {...} }`) - changes it through the broker's human-only `plugins` and `plugins.set` + changes it, one kind at a time: the patch names the kinds it changes, a kind + with `null` is removed, and a kind it doesn't name keeps its setting. The + change goes through the broker's human-only `plugins` and `plugins.set` verbs. The setter refuses a bad shape, a name with no module in this tree - (`unknown-plugin`) and a stale revision, and logs actor and old and new + (`unknown-plugin`), an unknown kind, an empty patch and a stale revision, and logs actor and old and new names in `changes.jsonl`. A running broker reads the setting at bus start (REQ-WEB-3's reload will change that), so restart the bus after a switch.