From 8770f98dcd7bc551b36c53ac9f31073e508cc283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 23 Aug 2026 23:19:11 +0000 Subject: [PATCH] capabilities: read a plugin's where they live, stop copying them in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four publishers copied a plugin's affordances, notifications and skills into capabilities/ at install. That directory is meant to be forked — core's work plus the owner's — so a fork accumulated files it did not author with nothing on disk saying which, and there was no good answer: commit them and carry another plugin's work, leave them and git status stays dirty. Copying was also wrong in two ways unrelated to git. A copy goes stale, because plugins:update pulls the plugin's directory and the copy keeps describing the old version. And a copy survived plugins:disable, so a disabled plugin's skill stayed in the agent's index — off, and still being announced. Now the loaders take a list of roots: the owner's store, plus each installed AND enabled plugin's own directory. capability-sources.ts resolves that list. No namespace is needed to keep them apart, because separate directories do it structurally rather than by convention. The owner's own win a skill name collision — their root is first and the merge keeps the first of each id — so a plugin cannot take over a name already in use. Affordances are deliberately NOT de-duplicated: several can be true at once. Notifications needed more than a root list, and it would have failed silently. Their codes were derived from the COPY'S PATH, //file.yaml. Read in place the path is just notifications/catalogue.yaml and carries no identity, so every plugin code would have vanished with nothing raised. loadPluginEntries takes the identity from the manifest instead, which is better provenance anyway — publisher and appName are validated at discovery, where a path was only ever a directory somebody chose. A yaml key still cannot claim another namespace, and that is pinned by test. createSession is async now, only because resolving enabled plugins reads the database. Once per session; its single caller already awaited. Deletes all four publishers including api/tasks/publish.ts from earlier today, superseded before it shipped. app-store/assets stays — images served over HTTP, where the copy is what makes them reachable. NOT done: tasks. That store is read-write, so reads must span sources while writes stay in the owner's directory, and a plugin's task has to be read-only in the UI. Next. Two things the design predicted as orphans turned out not to be, and both were checked before deleting rather than after. capabilities/extensions/tool-loader is written by the platform's own bootstrap every boot, not published by a plugin. And skills/email-db has no source to fall back on — the email plugin ships no skills directory, so that file exists in exactly one place and was hand-written. Neither was touched. 1422 pass, tsgo clean, officer restarts and serves. Co-Authored-By: Claude Opus 5 Officer-Session: 656f5c85-e49a-4f1f-b9f3-042a04b4d043 --- src/servers/affordances/load.ts | 20 ++- src/servers/affordances/publish.test.ts | 80 ------------ src/servers/affordances/publish.ts | 62 ---------- src/servers/api/tasks/publish.test.ts | 97 --------------- src/servers/api/tasks/publish.ts | 73 ----------- src/servers/capability-sources.test.ts | 121 +++++++++++++++++++ src/servers/capability-sources.ts | 97 +++++++++++++++ src/servers/notifications/catalogue.ts | 61 +++++++++- src/servers/notifications/publish.ts | 58 --------- src/servers/plugins/install.ts | 55 --------- src/servers/sidecar/claude/claude-manager.ts | 13 +- src/servers/sidecar/notify/dispatch.ts | 3 +- src/servers/sidecar/notify/index.ts | 3 +- src/servers/skills/load.ts | 30 ++++- src/servers/skills/publish.ts | 52 -------- 15 files changed, 335 insertions(+), 490 deletions(-) delete mode 100644 src/servers/affordances/publish.test.ts delete mode 100644 src/servers/affordances/publish.ts delete mode 100644 src/servers/api/tasks/publish.test.ts delete mode 100644 src/servers/api/tasks/publish.ts create mode 100644 src/servers/capability-sources.test.ts create mode 100644 src/servers/capability-sources.ts delete mode 100644 src/servers/notifications/publish.ts delete mode 100644 src/servers/skills/publish.ts diff --git a/src/servers/affordances/load.ts b/src/servers/affordances/load.ts index be0db927..876c8fd0 100644 --- a/src/servers/affordances/load.ts +++ b/src/servers/affordances/load.ts @@ -1,6 +1,7 @@ import { readdirSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { OFFICER_ITEMS_DIR } from '../data-path'; +import { capabilityRoots } from '../capability-sources'; // What the platform tells an agent about the surface it is speaking through. // @@ -107,6 +108,21 @@ export function compileAffordances(affordances: Affordance[]): string | null { } /** Read and compile in one step, for callers that only want the string. */ -export function affordanceBlock(root: string = AFFORDANCES_DIR): string | null { - return compileAffordances(loadAffordances(root)); +/** + * Every affordance in force: the owner's own, plus each installed and enabled plugin's. + * + * Read in place rather than copied in — see `capability-sources.ts`. Unlike skills these are NOT + * de-duplicated by name: an affordance is a paragraph of guidance, several can be true at once, and two + * plugins describing the same surface differently is information rather than a collision. + */ +export function loadAffordancesAcross(roots: string[]): Affordance[] { + return roots.flatMap((root) => loadAffordances(root)); +} + +/** + * Read and compile in one step. Async because resolving which plugins are enabled reads the database; + * called once per session, which is what makes that affordable. + */ +export async function affordanceBlock(): Promise { + return compileAffordances(loadAffordancesAcross(await capabilityRoots('affordances'))); } diff --git a/src/servers/affordances/publish.test.ts b/src/servers/affordances/publish.test.ts deleted file mode 100644 index 29ff22e8..00000000 --- a/src/servers/affordances/publish.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { describe, expect, test, afterEach } from 'bun:test'; -import { mkdirSync, rmSync, writeFileSync, existsSync } from 'node:fs'; -import { join } from 'node:path'; -import { tmpdir } from 'node:os'; -import { affordanceDirFor, affordanceSourceFor, publishAffordances, unpublishAffordances } from './publish'; -import { AFFORDANCES_DIR } from './load'; -import type { DiscoveredPlugin } from '../plugins/manifest'; - -// The failure that matters is not a copy going wrong — it is uninstall taking somebody else's files with -// it. Every first-party plugin publishes under one `officerdev/` directory, so removing the wrong level -// would strip them all and nobody would notice until an agent stopped knowing something. - -const root = join(tmpdir(), `aff-pub-${process.pid}`); - -const plugin = (appName: string, publisher = 'officerdev'): DiscoveredPlugin => - ({ appName, dir: join(root, appName), manifest: { publisher } }) as DiscoveredPlugin; - -const withAffordance = (appName: string, publisher = 'officerdev') => { - const p = plugin(appName, publisher); - mkdirSync(affordanceSourceFor(p), { recursive: true }); - writeFileSync(join(affordanceSourceFor(p), `${appName}.md`), `---\nname: ${appName}\n---\n\nbody\n`); - return p; -}; - -afterEach(() => { - rmSync(root, { recursive: true, force: true }); - for (const pub of ['officerdev', 'alice']) rmSync(join(AFFORDANCES_DIR, pub), { recursive: true, force: true }); -}); - -describe('publishing', () => { - test('lands under /, mirroring mountPrefix', () => { - const p = withAffordance('test-gitea'); - expect(affordanceDirFor(p)).toBe(join(AFFORDANCES_DIR, 'officerdev', 'test-gitea')); - }); - - test('copies what the plugin ships and counts it', async () => { - expect(await publishAffordances(withAffordance('test-gitea'))).toBe(1); - }); - - test('a plugin shipping none is not a failure — most ship none', async () => { - const bare = plugin('test-bare'); - mkdirSync(bare.dir, { recursive: true }); - expect(await publishAffordances(bare)).toBe(0); - }); - - test('re-publishing overwrites rather than erroring, so re-install works', async () => { - const p = withAffordance('test-gitea'); - expect(await publishAffordances(p)).toBe(1); - expect(await publishAffordances(p)).toBe(1); - }); - - test('two publishers with the same plugin name do not collide', async () => { - await publishAffordances(withAffordance('test-gitea', 'officerdev')); - await publishAffordances(withAffordance('test-gitea', 'alice')); - expect(existsSync(join(AFFORDANCES_DIR, 'officerdev', 'test-gitea', 'test-gitea.md'))).toBe(true); - expect(existsSync(join(AFFORDANCES_DIR, 'alice', 'test-gitea', 'test-gitea.md'))).toBe(true); - }); -}); - -describe('unpublishing', () => { - test('removes the plugin', async () => { - const p = withAffordance('test-gitea'); - await publishAffordances(p); - await unpublishAffordances(p); - expect(existsSync(affordanceDirFor(p))).toBe(false); - }); - - test('leaves the PUBLISHER alone — the thing that would silently strip every sibling', async () => { - const a = withAffordance('test-gitea'); - const b = withAffordance('test-music'); - await publishAffordances(a); - await publishAffordances(b); - await unpublishAffordances(a); - expect(existsSync(affordanceDirFor(b))).toBe(true); - }); - - test('is safe when nothing was ever published', async () => { - await unpublishAffordances(plugin('test-never')); - }); -}); diff --git a/src/servers/affordances/publish.ts b/src/servers/affordances/publish.ts deleted file mode 100644 index 5755307d..00000000 --- a/src/servers/affordances/publish.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { cp, readdir, rm } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; -import { join } from 'node:path'; -import { AFFORDANCES_DIR } from './load'; -import type { DiscoveredPlugin } from '../plugins/manifest'; - -// Copying a plugin's affordances into the store the agent reads. -// -// A plugin is a new SURFACE arriving at runtime, which is the case hand-written prose could never cover: -// install gitea and no agent learns that `tea` is authenticated, or that `git` is the better tool for -// what git already does. The plugin knows; nothing was carrying that across. -// -// Same shape as `publishAssets` — a directory in the plugin repository, copied to a platform-managed -// place on install, removed on uninstall. Deliberately so: two mechanisms for "the plugin ships some -// files" would drift, and this one is already understood. -// -// ── Why the destination is namespaced ── -// -// `//`, mirroring `mountPrefix`. Two plugins called `gitea` from different publishers -// must not overwrite each other's affordances, and an affordance in the tree should say whose it is -// without opening it. `core` occupies the appName slot for the platform's own. - -/** Where a plugin's affordances land. `officerdev/gitea/`, beside `officer/core/`. */ -export const affordanceDirFor = (plugin: DiscoveredPlugin): string => - join(AFFORDANCES_DIR, plugin.manifest.publisher, plugin.appName); - -/** What the plugin ships, if anything. */ -export const affordanceSourceFor = (plugin: DiscoveredPlugin): string => join(plugin.dir, 'affordances'); - -/** - * Publish a plugin's affordances. Returns the number of files, or 0 when it ships none — which is the - * ordinary case and not a failure. - * - * Idempotent by overwriting, so re-running an install republishes rather than erroring. It does NOT - * remove files the plugin has since dropped; that is what uninstall is for, and deleting on every - * install would make a half-finished copy destructive. - */ -export async function publishAffordances(plugin: DiscoveredPlugin): Promise { - const source = affordanceSourceFor(plugin); - if (!existsSync(source)) return 0; - - const dest = affordanceDirFor(plugin); - await cp(source, dest, { recursive: true, force: true }); - - try { - return ((await readdir(dest, { recursive: true })) as string[]).filter((f) => f.endsWith('.md')).length; - } catch { - return 0; - } -} - -/** - * Drop a plugin's affordances. - * - * Removes the plugin's own directory, never the publisher's: `officerdev/` holds every first-party - * plugin's, and taking it out because one was uninstalled would silently strip the rest. - * - * Safe to call when nothing was ever published. - */ -export async function unpublishAffordances(plugin: DiscoveredPlugin): Promise { - await rm(affordanceDirFor(plugin), { recursive: true, force: true }); -} diff --git a/src/servers/api/tasks/publish.test.ts b/src/servers/api/tasks/publish.test.ts deleted file mode 100644 index b8bddc76..00000000 --- a/src/servers/api/tasks/publish.test.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { afterEach, describe, expect, test } from 'bun:test'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync, existsSync, readFileSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { itemsDir } from '@@/data-path'; -import { publishTasks, taskSourceFor, unpublishTasks } from './publish'; -import type { DiscoveredPlugin } from '../../plugins/manifest'; - -/** - * A plugin shipping a task is how it reaches the file browser's context menu, so these are the two - * properties that matter: it arrives on install, and uninstalling takes back only what the plugin brought. - * - * The second is the one worth a test. `tasks/` is the OWNER'S directory — their own work lives beside - * whatever a plugin published, with nothing on disk marking which is which. A `rm -rf` of the wrong scope - * here destroys work no plugin ever created, which is not a failure a type checker can catch. - * - * Everything is prefixed `zz-publish-test-` and removed afterwards, because these write into the real - * items store rather than a fixture — `itemsDir` is derived from the install layout and cannot be pointed - * elsewhere for a test. - */ - -const PREFIX = 'zz-publish-test-'; -const made: string[] = []; - -const plugin = (dir: string): DiscoveredPlugin => ({ appName: `${PREFIX}plugin`, dir }) as DiscoveredPlugin; - -const seedPluginWithTasks = (names: string[]): string => { - const dir = mkdtempSync(join(tmpdir(), 'zz-plugin-')); - made.push(dir); - for (const name of names) { - const t = join(dir, 'tasks', name); - mkdirSync(t, { recursive: true }); - writeFileSync(join(t, 'TASK.md'), `---\nname: ${name}\n---\n`); - } - return dir; -}; - -afterEach(() => { - for (const d of made.splice(0)) rmSync(d, { recursive: true, force: true }); - // Anything this file could have written into the real store, by name. - for (const n of [`${PREFIX}one`, `${PREFIX}two`, `${PREFIX}owners-own`]) { - rmSync(join(itemsDir('tasks'), n), { recursive: true, force: true }); - } -}); - -describe('publishTasks', () => { - test('a plugin with no tasks directory publishes nothing and does not fail', async () => { - const dir = mkdtempSync(join(tmpdir(), 'zz-plugin-')); - made.push(dir); - expect(await publishTasks(plugin(dir))).toBe(0); - }); - - test('tasks arrive in the items store, contents and all', async () => { - const dir = seedPluginWithTasks([`${PREFIX}one`, `${PREFIX}two`]); - expect(await publishTasks(plugin(dir))).toBe(2); - - const landed = join(itemsDir('tasks'), `${PREFIX}one`, 'TASK.md'); - expect(existsSync(landed)).toBe(true); - expect(readFileSync(landed, 'utf8')).toContain(`name: ${PREFIX}one`); - }); - - test('publishing twice overwrites rather than failing', async () => { - const dir = seedPluginWithTasks([`${PREFIX}one`]); - await publishTasks(plugin(dir)); - expect(await publishTasks(plugin(dir))).toBe(1); - }); -}); - -describe('unpublishTasks leaves the owner alone', () => { - // The whole reason this file exists. - test("removes only what the plugin shipped, never the owner's own task", async () => { - const ownersOwn = join(itemsDir('tasks'), `${PREFIX}owners-own`); - mkdirSync(ownersOwn, { recursive: true }); - writeFileSync(join(ownersOwn, 'TASK.md'), '---\nname: hand written\n---\n'); - - const dir = seedPluginWithTasks([`${PREFIX}one`]); - await publishTasks(plugin(dir)); - await unpublishTasks(plugin(dir)); - - expect(existsSync(join(itemsDir('tasks'), `${PREFIX}one`))).toBe(false); - expect(existsSync(join(ownersOwn, 'TASK.md'))).toBe(true); - }); - - test('a plugin whose source is already gone removes nothing', async () => { - const ownersOwn = join(itemsDir('tasks'), `${PREFIX}owners-own`); - mkdirSync(ownersOwn, { recursive: true }); - - // The directory the plugin was cloned into no longer exists — deleted before uninstall ran. - await unpublishTasks(plugin(join(tmpdir(), 'zz-plugin-does-not-exist'))); - - expect(existsSync(ownersOwn)).toBe(true); - }); - - test('the source path is the plugin directory, not the store', () => { - expect(taskSourceFor(plugin('/somewhere/plugins/x'))).toBe('/somewhere/plugins/x/tasks'); - }); -}); diff --git a/src/servers/api/tasks/publish.ts b/src/servers/api/tasks/publish.ts deleted file mode 100644 index bd9c7a18..00000000 --- a/src/servers/api/tasks/publish.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { cp, readdir, rm } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; -import { join } from 'node:path'; -import { itemsDir } from '@@/data-path'; -import type { DiscoveredPlugin } from '../../plugins/manifest'; - -// Publishing a plugin's tasks, the fourth of these and the same shape as assets, affordances, -// notification catalogues and skills. -// -// ── Why a plugin needs to ship a task at all ── -// -// A task is how something reaches the FILE BROWSER'S CONTEXT MENU. `TriggerConfig` is -// `{ type: 'file'; extensions: string[] } | { type: 'directory' }`, and `matchesTrigger` in -// `officerdev/src/apps/FileBrowser/useTasks.ts` is what decides which entries appear on a right-click. So -// a plugin that wants to offer "transcribe this" on an mp3 does not need a platform change, a menu -// registry or a new frontend hook — it needs a task with the right trigger, and that already works. -// -// What was missing was only the delivery: every other publishable thing had a publisher and tasks did -// not, so the capability had to be installed by hand alongside the plugin and removed by hand after. -// -// ── Flat, not namespaced by publisher ── -// -// Same trade as skills, for the same reason and with the same limit. A task's directory name is its -// identity everywhere — `taskDirName` in a job row, the path a script runner materialises, the thing the -// owner sees in a run history. Wrapping that in `officerdev/whisper/` would put a publisher in every one -// of those places for the benefit of a collision nobody has had. Collisions resolve first-come, which is -// the rule the items store has always used. -// -// That is acceptable because a task claims no authority — it is an entry in a menu, gated by the same -// permission the tasks surface already carries. It is NOT acceptable for affordances, which is why those -// are namespaced: an affordance that could claim `officer/core:` would be claiming the platform's voice. - -const tasksDir = () => itemsDir('tasks'); - -/** What the plugin ships, if anything. */ -export const taskSourceFor = (plugin: DiscoveredPlugin): string => join(plugin.dir, 'tasks'); - -/** Publish a plugin's tasks. Returns the number of directories, or 0 when it ships none. */ -export async function publishTasks(plugin: DiscoveredPlugin): Promise { - const source = taskSourceFor(plugin); - if (!existsSync(source)) return 0; - - await cp(source, tasksDir(), { recursive: true, force: true }); - - try { - return (await readdir(source, { withFileTypes: true })).filter((e) => e.isDirectory()).length; - } catch { - return 0; - } -} - -/** - * Drop a plugin's tasks. - * - * Removes only the directories THIS plugin shipped, read from its own source tree — never everything - * under `tasks/`, which holds the owner's own work. A task the owner wrote by hand has no marker saying - * so, so the plugin's source tree is the only honest record of what came from the plugin. - * - * The consequence is worth stating: if the owner EDITS a task a plugin shipped, uninstalling still - * removes it, because it is still one of the names the plugin published. That matches skills and is the - * behaviour to keep — the alternative is orphans nobody can attribute. - */ -export async function unpublishTasks(plugin: DiscoveredPlugin): Promise { - const source = taskSourceFor(plugin); - if (!existsSync(source)) return; - let dirs: string[]; - try { - dirs = (await readdir(source, { withFileTypes: true })).filter((e) => e.isDirectory()).map((e) => e.name); - } catch { - return; - } - for (const dir of dirs) await rm(join(tasksDir(), dir), { recursive: true, force: true }); -} diff --git a/src/servers/capability-sources.test.ts b/src/servers/capability-sources.test.ts new file mode 100644 index 00000000..6cd785cc --- /dev/null +++ b/src/servers/capability-sources.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, test } from 'bun:test'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { loadSkillsAcross } from './skills/load'; +import { loadAffordancesAcross } from './affordances/load'; +import { loadPluginEntries } from './notifications/catalogue'; + +/** + * A plugin's capabilities are READ where they live rather than copied into `capabilities/` + * (docs/plugin-capabilities.md). These pin the three properties that were not obvious. + * + * `capabilityRoots` itself is not tested here — it reads the real database and the real plugin + * directory, so a test of it would assert whatever this machine happens to have installed. What is worth + * pinning is what the loaders do with a list once they have one, and that is pure. + */ + +const seed = (files: Record): string => { + const dir = mkdtempSync(join(tmpdir(), 'zz-caps-')); + for (const [rel, body] of Object.entries(files)) { + const abs = join(dir, rel); + mkdirSync(join(abs, '..'), { recursive: true }); + writeFileSync(abs, body); + } + return dir; +}; + +const skill = (name: string, desc: string) => `---\nname: ${name}\ndescription: ${desc}\n---\n\nbody\n`; + +describe('skills merge across sources', () => { + test('a plugin directory contributes its skills', () => { + const owner = seed({ 'mine/SKILL.md': skill('Mine', 'the owner wrote this') }); + const plugin = seed({ 'theirs/SKILL.md': skill('Theirs', 'a plugin shipped this') }); + try { + const ids = loadSkillsAcross([owner, plugin]).map((s) => s.id).sort(); + expect(ids).toEqual(['mine', 'theirs']); + } finally { + rmSync(owner, { recursive: true, force: true }); + rmSync(plugin, { recursive: true, force: true }); + } + }); + + // Precedence, and the reason for it: a plugin must not be able to take over the meaning of a name its + // owner is already using, just by picking the same directory name. + test("the owner's own wins a name collision", () => { + const owner = seed({ 'deploy/SKILL.md': skill('Deploy', 'the owner version') }); + const plugin = seed({ 'deploy/SKILL.md': skill('Deploy', 'the plugin version') }); + try { + const found = loadSkillsAcross([owner, plugin]); + expect(found).toHaveLength(1); + expect(found[0]!.description).toBe('the owner version'); + } finally { + rmSync(owner, { recursive: true, force: true }); + rmSync(plugin, { recursive: true, force: true }); + } + }); + + // Which is what makes disable mean disable: the caller simply stops passing that root. + test('dropping a source drops its skills', () => { + const owner = seed({ 'mine/SKILL.md': skill('Mine', 'kept') }); + const plugin = seed({ 'theirs/SKILL.md': skill('Theirs', 'gone when disabled') }); + try { + expect(loadSkillsAcross([owner, plugin])).toHaveLength(2); + expect(loadSkillsAcross([owner])).toHaveLength(1); + } finally { + rmSync(owner, { recursive: true, force: true }); + rmSync(plugin, { recursive: true, force: true }); + } + }); + + test('a source that does not exist is skipped, not fatal', () => { + const owner = seed({ 'mine/SKILL.md': skill('Mine', 'kept') }); + try { + expect(loadSkillsAcross([owner, join(tmpdir(), 'zz-caps-nope')])).toHaveLength(1); + } finally { + rmSync(owner, { recursive: true, force: true }); + } + }); +}); + +describe('affordances are not de-duplicated', () => { + // Unlike skills: an affordance is guidance, several can be true at once, and two plugins describing the + // same surface differently is information rather than a collision. + test('both sources contribute even with the same file name', () => { + const a = seed({ 'x/one.md': '---\nname: A\ndescription: from a\n---\nbody a\n' }); + const b = seed({ 'y/one.md': '---\nname: B\ndescription: from b\n---\nbody b\n' }); + try { + expect(loadAffordancesAcross([a, b])).toHaveLength(2); + } finally { + rmSync(a, { recursive: true, force: true }); + rmSync(b, { recursive: true, force: true }); + } + }); +}); + +describe('notification codes come from the manifest, not the path', () => { + // The whole reason loadPluginEntries exists. Copied to capabilities/ the path carried + // /; read in place it carries nothing, so the identity has to travel separately. + test('a plugin in its own directory still gets a namespaced code', () => { + const dir = seed({ 'notifications/catalogue.yaml': "nowPlaying:\n title: 'Now playing'\n" }); + try { + const entries = loadPluginEntries({ appName: 'music', dir, manifest: { publisher: 'officerdev' } }); + expect(Object.keys(entries)).toEqual(['officerdev/music:nowPlaying']); + expect(entries['officerdev/music:nowPlaying']!.thread).toBe('officerdev/music'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + // The property that must survive the move: a plugin cannot claim the platform's voice by writing a + // core-looking key into its own yaml. + test('a yaml key cannot claim another namespace', () => { + const dir = seed({ 'notifications/c.yaml': "officer/core:test:\n title: 'Nice try'\n" }); + try { + const keys = Object.keys(loadPluginEntries({ appName: 'x', dir, manifest: { publisher: 'someone' } })); + expect(keys).toEqual(['someone/x:officer/core:test']); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/servers/capability-sources.ts b/src/servers/capability-sources.ts new file mode 100644 index 00000000..6808e0d1 --- /dev/null +++ b/src/servers/capability-sources.ts @@ -0,0 +1,97 @@ +import { existsSync } from 'node:fs'; +import { join } from 'node:path'; +import { itemsDir, type ItemType } from './data-path'; +import { snapshotPlugins } from './plugins/mount'; + +/** + * Where capabilities are read from — the owner's own store, plus every installed and enabled plugin's. + * + * ── Why plugins are read in place rather than copied in ── + * + * `capabilities/` is a git repository, and the point of it is that anybody can fork it: core's tasks and + * skills, plus their own on top. That only holds if the directory contains exactly two things, what core + * ships and what its owner wrote. Publishing a plugin's files into it broke that — a fork accumulated + * files it did not author, with nothing on disk saying which, so committing them carried another plugin's + * work and not committing them left `git status` permanently dirty. + * + * Copying was wrong a second way that has nothing to do with git. A copy goes STALE: `plugins:update` is + * a `git pull` in the plugin's directory, and a capability copied out of it keeps describing the previous + * version until something republishes. And a copy survives `plugins:disable`, so a disabled plugin's + * skill stayed in the agent's index — the plugin off, the agent still told it was there. + * + * Reading in place fixes all of that by construction, and needs no namespace to do it. Scoping the paths + * would have made provenance READABLE; separate directories make it STRUCTURAL, which is the stronger + * property and the cheaper one. + * + * See `docs/plugin-capabilities.md`. + * + * ── Enabled, not merely installed ── + * + * A disabled plugin contributes nothing. That is the whole meaning of disable — routes down, sidecar + * stopped — and a capability that outlived it was the one piece that did not comply. + * + * ── Cost ── + * + * This reads the database, so it is async, and it must not be called per turn. Its callers resolve it + * once per session (`skillsBlock`, `affordanceBlock`) or per request on surfaces that are already doing + * database work. A missing directory is skipped rather than erroring: most plugins ship no capabilities + * at all, and that is the ordinary case rather than a fault. + */ +export async function capabilityRoots(type: ItemType | 'affordances' | 'notifications'): Promise { + const roots = [itemsDir(type as ItemType)]; + + let states: Awaited>['states'] = []; + try { + ({ states } = await snapshotPlugins()); + } catch { + // The owner's own capabilities still load. A database that cannot be read is a much louder problem + // somewhere else, and losing the agent's whole skill index to it would only obscure that. + return roots; + } + + for (const state of states) { + if (!state.install?.enabled) continue; + const dir = join(state.plugin.dir, type); + if (existsSync(dir)) roots.push(dir); + } + return roots; +} + +/** + * The same list, for a caller that already holds a snapshot. + * + * Exists so a request doing several of these — the items rescan reads five types — pays for one database + * read rather than five. + */ +export function capabilityRootsFrom( + states: Awaited>['states'], + type: ItemType | 'affordances' | 'notifications', +): string[] { + const roots = [itemsDir(type as ItemType)]; + for (const state of states) { + if (!state.install?.enabled) continue; + const dir = join(state.plugin.dir, type); + if (existsSync(dir)) roots.push(dir); + } + return roots; +} + +/** + * Installed and enabled plugins, in the shape the notification catalogue wants. + * + * Notifications are the one capability whose namespace cannot be read off a path once the file stays in + * the plugin's own directory — see `notifications/catalogue.ts` → `loadPluginEntries`. So the source list + * carries the identity with it. + */ +export async function pluginCatalogueSources(): Promise< + { appName: string; dir: string; manifest: { publisher: string } }[] +> { + try { + const { states } = await snapshotPlugins(); + return states + .filter((s) => s.install?.enabled) + .map((s) => ({ appName: s.plugin.appName, dir: s.plugin.dir, manifest: { publisher: s.plugin.manifest.publisher } })); + } catch { + return []; + } +} diff --git a/src/servers/notifications/catalogue.ts b/src/servers/notifications/catalogue.ts index acc30e0d..3aed15bc 100644 --- a/src/servers/notifications/catalogue.ts +++ b/src/servers/notifications/catalogue.ts @@ -44,6 +44,8 @@ export type NotificationEntry = { thread?: string; }; +export type PluginCatalogueSource = { appName: string; dir: string; manifest: { publisher: string } }; + export type Catalogue = { /** * Changes whenever the aggregate does. A CONTENT HASH, not a counter. @@ -195,6 +197,58 @@ export function loadPublishedEntries(root: string = NOTIFICATIONS_DIR): Record/` from the path, which worked while installing COPIED a + * plugin's yaml to `capabilities/notifications///`. Read in place the path is just + * `notifications/catalogue.yaml` and carries no identity at all — so the codes would silently vanish, + * which is the failure this exists to prevent. + * + * The manifest supplies it instead, and that is strictly better provenance: `publisher` and `appName` are + * validated at discovery, where the path was only ever a directory somebody chose. The property that + * matters is unchanged — the platform imposes the namespace and the yaml cannot claim one. Writing + * `officer/core:test` as a key still produces `/:officer/core:test`. + */ +export function loadPluginEntries(plugin: { + appName: string; + dir: string; + manifest: { publisher: string }; +}): Record { + const root = join(plugin.dir, 'notifications'); + let files: string[]; + try { + files = (readdirSync(root, { recursive: true }) as string[]).filter((f) => /\.ya?ml$/.test(f)); + } catch { + return {}; // shipping none is the ordinary case + } + + const { publisher } = plugin.manifest; + const entries: Record = {}; + + for (const rel of files.sort()) { + let doc: unknown; + try { + doc = YAML.parse(readFileSync(join(root, rel), 'utf8')); + } catch { + continue; + } + if (doc === null || typeof doc !== 'object') continue; + + for (const [key, value] of Object.entries(doc as Record)) { + const entry = coerce(value); + if (!entry) continue; + entries[`${publisher}/${plugin.appName}:${key}`] = { + ...entry, + thread: entry.thread ?? `${publisher}/${plugin.appName}`, + }; + } + } + + return entries; +} + /** * The whole catalogue: the platform's own plus everything installed plugins publish. * @@ -202,8 +256,11 @@ export function loadPublishedEntries(root: string = NOTIFICATIONS_DIR): Record//`, mirroring `mountPrefix`, -// so a plugin's codes are stamped with its provenance by WHERE IT LANDS rather than by anything it -// declares. See `catalogue.ts`. - -/** Where a plugin's catalogue lands. `officerdev/music/`, beside `officer/core`'s built-in entries. */ -export const notificationDirFor = (plugin: DiscoveredPlugin): string => - join(NOTIFICATIONS_DIR, plugin.manifest.publisher, plugin.appName); - -/** What the plugin ships, if anything. */ -export const notificationSourceFor = (plugin: DiscoveredPlugin): string => join(plugin.dir, 'notifications'); - -/** - * Publish a plugin's catalogue. Returns the number of files, or 0 when it ships none — the ordinary case. - * - * Idempotent by overwriting, so re-running an install republishes rather than erroring. It does NOT remove - * files the plugin has since dropped; that is uninstall's job, and deleting on every install would make a - * half-finished copy destructive. - */ -export async function publishNotifications(plugin: DiscoveredPlugin): Promise { - const source = notificationSourceFor(plugin); - if (!existsSync(source)) return 0; - - const dest = notificationDirFor(plugin); - await cp(source, dest, { recursive: true, force: true }); - - try { - return ((await readdir(dest, { recursive: true })) as string[]).filter((f) => /\.ya?ml$/.test(f)).length; - } catch { - return 0; - } -} - -/** - * Drop a plugin's catalogue. - * - * Removes the plugin's own directory, never the publisher's: `officerdev/` holds every first-party - * plugin's, and taking it out because one was uninstalled would strip the rest. - * - * The codes then stop resolving, which is a defined state rather than a broken one — a device holding a - * queued push for a departed plugin renders the unknown-code fallback. See docs/push-notifications.md. - * - * Safe to call when nothing was ever published. - */ -export async function unpublishNotifications(plugin: DiscoveredPlugin): Promise { - await rm(notificationDirFor(plugin), { recursive: true, force: true }); -} diff --git a/src/servers/plugins/install.ts b/src/servers/plugins/install.ts index 5e297454..6afcdf88 100644 --- a/src/servers/plugins/install.ts +++ b/src/servers/plugins/install.ts @@ -17,10 +17,6 @@ import { discoverPlugins } from './discover'; import { fetchPlugin } from './fetch'; import { revokeGrantsForPermissions } from 'officerdb'; import { publishAssets, unpublishAssets } from '../app-store/assets'; -import { publishAffordances, unpublishAffordances } from '../affordances/publish'; -import { publishNotifications, unpublishNotifications } from '../notifications/publish'; -import { publishSkills, unpublishSkills } from '../skills/publish'; -import { publishTasks, unpublishTasks } from '../api/tasks/publish'; import { refreshPluginMounts, snapshotPlugins } from './mount'; import { publishExtension, unpublishExtension, extensionFileName } from './zip'; import { generatePluginSchemas, pushPluginSchema } from './schema'; @@ -403,53 +399,6 @@ export async function installPlugin( await step(steps, onStep, `extension: published to /plugins/${appName}/${file}`); } - // What this plugin needs an AGENT to know about itself. A plugin is a new surface arriving at - // runtime, and nothing else carries that across: install gitea and no agent learns that `tea` is - // authenticated unless the plugin says so. Same shape as the assets above — a directory in the - // repository, copied to a platform-managed place. - // - // Not fatal. An affordance that failed to copy costs an agent some knowledge; refusing the install - // over it would cost the owner the plugin. - try { - const published = await publishAffordances(plugin); - if (published) { - await step(steps, onStep, `affordances: ${published} published to capabilities/affordances/`); - } - } catch (err) { - await step(steps, onStep, `affordances: NOT published — ${err instanceof Error ? err.message : err}`); - } - - // Same bargain for the notification catalogue: a plugin that cannot publish its codes still installs, - // and its pushes render the unknown-code fallback on the device rather than nothing at all. - try { - const published = await publishNotifications(plugin); - if (published) { - await step(steps, onStep, `notifications: ${published} published to capabilities/notifications/`); - } - } catch (err) { - await step(steps, onStep, `notifications: NOT published — ${err instanceof Error ? err.message : err}`); - } - - // Same bargain again: a plugin that cannot publish its skills still installs. The agent simply does - // not learn the reference exists, which is the state every plugin was in before this. - try { - const published = await publishSkills(plugin); - if (published) await step(steps, onStep, `skills: ${published} published to capabilities/skills/`); - } catch (err) { - await step(steps, onStep, `skills: NOT published — ${err instanceof Error ? err.message : err}`); - } - - // And the same for tasks, which is how a plugin reaches the FILE BROWSER'S CONTEXT MENU. A task with a - // file trigger appears on a right-click with no platform change at all — `matchesTrigger` in - // officerdev/src/apps/FileBrowser/useTasks.ts already does that work. What was missing was only the - // delivery, so the capability had to be installed by hand beside the plugin and removed by hand after. - try { - const published = await publishTasks(plugin); - if (published) await step(steps, onStep, `tasks: ${published} published to capabilities/tasks/`); - } catch (err) { - await step(steps, onStep, `tasks: NOT published — ${err instanceof Error ? err.message : err}`); - } - // The tile, for the person who just installed it. `plugin.web` because `pluginDockManifests` only // offers a tile for a plugin that has a frontend — a backend-only plugin has no screen to pin. if (plugin.web && forUserId !== undefined) { @@ -547,12 +496,8 @@ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?: // Same reasoning: a copy, with the original still in the plugin's directory. Removed so an agent stops // being told about a surface that is no longer there — an affordance for an uninstalled plugin is // worse than none, because it reads as true. - if (plugin) await unpublishAffordances(plugin).catch(() => {}); // And its notification codes, which stop resolving rather than resolving to something stale. A device // still holding a queued push renders the unknown-code fallback, which is a defined state. - if (plugin) await unpublishNotifications(plugin).catch(() => {}); - if (plugin) await unpublishSkills(plugin).catch(() => {}); - if (plugin) await unpublishTasks(plugin).catch(() => {}); await step(steps, onStep, 'tables and data: untouched (published assets, affordances, notifications, skills and tasks removed)'); return { ok: true, appName, steps }; } diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index a7158c8e..3ca3cef2 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -552,11 +552,14 @@ function armIdle(session: PersistentSession): void { }, IDLE_TIMEOUT_MS); } -function createSession( +// Async only because the preamble is: resolving which plugins are installed and enabled, so their +// capabilities can be read where they live, reads the database. Once per session — see +// `capability-sources.ts`. Its single caller already awaits. +async function createSession( params: ClaudeSpawnStreamingParams, onEvent: (event: ChatEvent) => void, onNotify: (kind: 'turn-ended' | 'input-needed') => void, -): PersistentSession { +): Promise { const { sessionKey } = params; const input = makeInputQueue(); const abort = new AbortController(); @@ -625,11 +628,11 @@ function createSession( // Read here rather than at module load: a plugin installed an hour ago should be live on the next // message, and this costs a few kilobytes once per session. - const affordances = affordanceBlock(); + const affordances = await affordanceBlock(); // The skills INDEX — names, descriptions and paths, never bodies. See skills/load.ts for why this is // the inverse of an affordance. Read here rather than at module load for the same reason: a skill // written an hour ago should be offered on the next session. - const skills = skillsBlock(); + const skills = await skillsBlock(); /* And, for a commander, the two paths only this session can be told. @@ -916,7 +919,7 @@ export async function spawnClaudeStreaming( await applyModelChange(session, params.model); await applyPlanModeChange(session, params.planMode); } else { - session = createSession(params, onEvent, onNotify); + session = await createSession(params, onEvent, onNotify); } session.pushTurn(params.prompt, params.images); } diff --git a/src/servers/sidecar/notify/dispatch.ts b/src/servers/sidecar/notify/dispatch.ts index 6f74cce1..28a4fe5b 100644 --- a/src/servers/sidecar/notify/dispatch.ts +++ b/src/servers/sidecar/notify/dispatch.ts @@ -1,5 +1,6 @@ import type { Channel, DeliveryResult, Notification } from './types'; import { loadCatalogue } from '../../notifications/catalogue'; +import { pluginCatalogueSources } from '../../capability-sources'; import { discordChannel } from './discord'; import { apnsChannel } from './apns'; import { fcmChannel } from './fcm'; @@ -28,7 +29,7 @@ export async function dispatch(n: Notification): Promise { // Read ONCE, here — not per channel and not per device. `version` travels with the push as `mv`, and // every copy of one notification must carry the same one; two channels reading disk either side of a // plugin install would otherwise tell two devices different things about the same event. - const ctx = { catalogue: loadCatalogue() }; + const ctx = { catalogue: loadCatalogue(undefined, await pluginCatalogueSources()) }; return Promise.all( active.map(async (c) => { diff --git a/src/servers/sidecar/notify/index.ts b/src/servers/sidecar/notify/index.ts index 5c13c66f..1b327cef 100644 --- a/src/servers/sidecar/notify/index.ts +++ b/src/servers/sidecar/notify/index.ts @@ -3,6 +3,7 @@ import { createSidecarConnector } from '../connect'; import { dispatch, configuredChannels } from './dispatch'; import { handleDeviceRoute } from './devices'; import { loadCatalogue } from '../../notifications/catalogue'; +import { pluginCatalogueSources } from '../../capability-sources'; import { resolveNotifyUser } from './resolve-user'; import { closeApnsSessions } from './apns'; import type { Notification } from './types'; @@ -76,7 +77,7 @@ const server = Bun.serve({ // it would present as "the new plugin's notifications don't work" — the hardest kind of bug to place. // It is a few kilobytes off local disk against a request the app makes about once per launch. if (url.pathname === '/mapping' && req.method === 'GET') { - const catalogue = loadCatalogue(); + const catalogue = loadCatalogue(undefined, await pluginCatalogueSources()); return Response.json(catalogue, { // The client decides by `version`, so a cached copy would defeat the sync it exists to drive. headers: { 'Cache-Control': 'no-store' }, diff --git a/src/servers/skills/load.ts b/src/servers/skills/load.ts index 5fae2725..16b76e8a 100644 --- a/src/servers/skills/load.ts +++ b/src/servers/skills/load.ts @@ -1,6 +1,7 @@ import { readdirSync, readFileSync, existsSync } from 'node:fs'; import { join } from 'node:path'; import { OFFICER_ITEMS_DIR } from '../data-path'; +import { capabilityRoots } from '../capability-sources'; // Skills: reference the agent reads WHEN IT NEEDS IT, not on every turn. // @@ -104,6 +105,31 @@ export function compileSkills(skills: Skill[]): string | null { } /** Read and compile in one step, for callers that only want the string. */ -export function skillsBlock(root: string = SKILLS_DIR): string | null { - return compileSkills(loadSkills(root)); +/** + * Every skill on offer: the owner's own, plus each installed and enabled plugin's own directory. + * + * A plugin's skills are read WHERE THEY LIVE rather than copied into `capabilities/` — see + * `capability-sources.ts` for why, and `docs/plugin-capabilities.md` for the whole argument. + * + * **The owner's own win a name collision**, because `capabilityRoots` puts their directory first and this + * keeps the first of each id. That is the right precedence: a skill somebody wrote deliberately should + * not be displaced by a plugin that happened to pick the same directory name, and a plugin cannot + * silently take over the meaning of a name already in use. + */ +export function loadSkillsAcross(roots: string[]): Skill[] { + const byId = new Map(); + for (const root of roots) { + for (const skill of loadSkills(root)) if (!byId.has(skill.id)) byId.set(skill.id, skill); + } + return [...byId.values()].sort((a, b) => a.id.localeCompare(b.id)); +} + +/** + * Read and compile in one step, for callers that only want the string. + * + * Async because resolving which plugins are enabled reads the database. Called once per session, which is + * what makes that affordable — see the note in `capability-sources.ts`. + */ +export async function skillsBlock(): Promise { + return compileSkills(loadSkillsAcross(await capabilityRoots('skills'))); } diff --git a/src/servers/skills/publish.ts b/src/servers/skills/publish.ts deleted file mode 100644 index 31c1d5ca..00000000 --- a/src/servers/skills/publish.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { cp, readdir, rm } from 'node:fs/promises'; -import { existsSync } from 'node:fs'; -import { join } from 'node:path'; -import { SKILLS_DIR } from './load'; -import type { DiscoveredPlugin } from '../plugins/manifest'; - -// Publishing a plugin's skills, the same shape as affordances and notification catalogues. -// -// Deliberately NOT namespaced by publisher, unlike those two. A skill's directory name is what appears in -// the index and what the agent reads a path to, and `officerdev/gitea/SKILL.md` in a prompt is noise for -// a human and for a model. Collisions are handled by the loader taking whichever sorts first, which is -// the same rule the items store has always used — one flat namespace, first come. -// -// That trade is only acceptable because a skill is REFERENCE. An affordance's namespace matters because a -// plugin must not be able to claim `officer/core:`; a skill has no authority to usurp. - -export const skillDirFor = (plugin: DiscoveredPlugin): string => join(SKILLS_DIR, plugin.appName); - -/** What the plugin ships, if anything. */ -export const skillSourceFor = (plugin: DiscoveredPlugin): string => join(plugin.dir, 'skills'); - -/** Publish a plugin's skills. Returns the number of directories, or 0 when it ships none. */ -export async function publishSkills(plugin: DiscoveredPlugin): Promise { - const source = skillSourceFor(plugin); - if (!existsSync(source)) return 0; - - await cp(source, SKILLS_DIR, { recursive: true, force: true }); - - try { - return (await readdir(source, { withFileTypes: true })).filter((e) => e.isDirectory()).length; - } catch { - return 0; - } -} - -/** - * Drop a plugin's skills. - * - * Removes only the directories THIS plugin shipped, read from its own source tree — not everything under - * `skills/`, which holds the platform's own and every other plugin's. - */ -export async function unpublishSkills(plugin: DiscoveredPlugin): Promise { - const source = skillSourceFor(plugin); - if (!existsSync(source)) return; - let dirs: string[]; - try { - dirs = (await readdir(source, { withFileTypes: true })).filter((e) => e.isDirectory()).map((e) => e.name); - } catch { - return; - } - for (const dir of dirs) await rm(join(SKILLS_DIR, dir), { recursive: true, force: true }); -}