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 <[email protected]>
2026-10-15 17:50:59 -05:00
parent 467a313c7d
commit fba879f6a1
5 changed files with 79 additions and 9 deletions
@@ -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');
+});
@@ -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 };
  }
@@ -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", () => {
@@ -21,7 +21,9 @@ function parse(args) {
  return out;
 }

-// PATCH is { "revision": <from a read>, "plugins": { "<kind>": <name or [names]> } }.
+// PATCH is { "revision": <from a read>, "plugins": { "<kind>": <name or [names] or null> } }. 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";
@@ -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.