diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2d7ad6da5..98b991833 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed Claude plugin slash commands and skills silently vanishing when the plugin manifest declares `commands`/`slash-commands`/`skills` as a JSON array — the shape the Claude plugins reference documents and real plugins like `addyosmani/agent-skills` ship. `resolvePluginDir` in `packages/coding-agent/src/discovery/claude-plugins.ts` typed those fields as `string` and dropped array values on the floor; it now normalizes both shapes, loads every in-root entry, and reports one out-of-plugin-root warning per bad entry so misconfigured paths remain visible. ([#4609](https://github.com/can1357/oh-my-pi/issues/4609)) +- Fixed Claude plugin slash commands and skills silently vanishing when the plugin manifest declares `commands`/`slash-commands`/`skills` as a JSON array — the shape the Claude plugins reference documents and real plugins like `addyosmani/agent-skills` ship. `resolvePluginDir` in `packages/coding-agent/src/discovery/claude-plugins.ts` typed those fields as `string` and dropped array values on the floor; it now normalizes both shapes, loads every in-root entry, and reports one out-of-plugin-root warning per bad entry. The resolver also now honours Claude's per-field merge semantic — `skills` adds to the default `skills/` scan; `commands`/`slash-commands` replace the default `commands/` — so plugins like `{"skills":["./extra-skills"]}` no longer lose their default `skills/` folder while `{"commands":["./admin"]}` still replaces `commands/` as documented. ([#4609](https://github.com/can1357/oh-my-pi/issues/4609)) - Fixed macOS Backspace on empty search not deleting sessions in the `/resume` picker; Fn+Backspace terminals that deliver `\x7f` instead of `\e[3~` now reach the delete confirmation dialog. ([#4580](https://github.com/can1357/oh-my-pi/pull/4580) by [@JagravNaik](https://github.com/JagravNaik)) - Fixed `/rename` title arguments treating `#` prompt-action tokens as autocomplete triggers instead of literal session title text. ([#4600](https://github.com/can1357/oh-my-pi/issues/4600)) - Fixed empty session `.jsonl` files accumulating in `~/.omp/agent/sessions//` after a draft-then-clear exit cycle. `SessionManager.saveDraft(text)` materializes the session file so the draft sidecar has a parent; a subsequent `saveDraft("")` unlinked the sidecar but left the metadata-only JSONL behind (title slot + session header + startup selector entries, ~500–750 B), and `#shouldHaveSessionFile()` could no longer prune it once `#fileIsCurrent`/`#forceFileCreation` were latched. `SessionManager.close()` now drops only draft-owned metadata-only sessions with no saved draft sidecar to reattach to, while keeping real conversations, meaningful non-message entries such as handoff custom messages, explicit `ensureOnDisk()` sessions, drafts still pending for `--resume`, and never-materialized sessions untouched ([#4571](https://github.com/can1357/oh-my-pi/issues/4571)). diff --git a/packages/coding-agent/src/discovery/claude-plugins.ts b/packages/coding-agent/src/discovery/claude-plugins.ts index 8c46b7112..f9647c124 100644 --- a/packages/coding-agent/src/discovery/claude-plugins.ts +++ b/packages/coding-agent/src/discovery/claude-plugins.ts @@ -61,21 +61,33 @@ function isWithinPluginRoot(rootPath: string, targetPath: string): boolean { /** * Resolve a manifest-declared directory field to absolute paths within the - * plugin root. The Claude plugin manifest allows path fields to be either a - * single string or an array of strings - * (https://code.claude.com/docs/en/plugins-reference#path-behavior-rules), so - * both shapes are normalized here. + * plugin root. * - * The first `manifestKeys` entry that supplies at least one non-empty path wins - * (later keys are ignored — used for the `commands` > `slash-commands` legacy - * fallback). When no key is set the plugin's default subdirectory (`fallback`) - * is used. Entries that resolve outside the plugin root are dropped with a - * warning so misconfigured manifests are visible without traversal escape. + * Manifest path fields may be `string` or `string[]` + * (https://code.claude.com/docs/en/plugins-reference#path-behavior-rules); + * both shapes are normalized here. The first `manifestKeys` entry that + * supplies at least one non-empty path wins (later keys are ignored — used for + * the `commands` > `slash-commands` legacy fallback). + * + * `fallback` is the default subdirectory (e.g. `skills/`, `commands/`) and + * `includeFallback` controls the Claude-documented merge semantic per field: + * + * - `skills` **adds to** the default: `fallback` is always scanned, and any + * manifest entries load alongside it. Callers pass `includeFallback: true`. + * - `commands` / `slash-commands` **replace** the default: an explicit + * manifest key means the default `commands/` directory is not scanned. + * Callers pass `includeFallback: false` (the manifest itself may still + * list `./commands` explicitly to keep it). + * + * When no matching key is set, the fallback is used regardless. Entries that + * resolve outside the plugin root are dropped with a warning so misconfigured + * manifests remain observable and cannot escape via traversal. */ async function resolvePluginDir( root: ClaudePluginRoot, manifestKeys: ReadonlyArray, fallback: string, + includeFallback: boolean, ): Promise { const manifest = await readPluginManifest(root); const fallbackDir = path.join(root.path, fallback); @@ -106,17 +118,28 @@ async function resolvePluginDir( return { dirs: [fallbackDir], warnings: [] }; } + // Dedup preserves order: default entry (when included) first, then declared + // entries in manifest order. Deduping the paths themselves means a plugin + // author can still list `./commands` explicitly when they want the default + // alongside extras without producing double-loads. + const seen = new Set(); const dirs: string[] = []; const warnings: string[] = []; + if (includeFallback) { + seen.add(fallbackDir); + dirs.push(fallbackDir); + } for (const entry of configured) { const resolved = path.resolve(root.path, entry); - if (isWithinPluginRoot(root.path, resolved)) { - dirs.push(resolved); - } else { + if (!isWithinPluginRoot(root.path, resolved)) { warnings.push( `[claude-plugins] Ignoring ${String(matchedKey)} path outside plugin root for ${root.id}: ${entry}`, ); + continue; } + if (seen.has(resolved)) continue; + seen.add(resolved); + dirs.push(resolved); } return { dirs, warnings }; @@ -133,7 +156,12 @@ async function loadSkills(ctx: LoadContext): Promise> { warnings.push(...rootWarnings); const results = await Promise.all( roots.map(async root => { - const { dirs: skillsDirs, warnings: resolveWarnings } = await resolvePluginDir(root, ["skills"], "skills"); + const { dirs: skillsDirs, warnings: resolveWarnings } = await resolvePluginDir( + root, + ["skills"], + "skills", + true, + ); const scanResults = await Promise.all( skillsDirs.map(dir => scanSkillsFromDir(ctx, { @@ -178,6 +206,7 @@ async function loadSlashCommands(ctx: LoadContext): Promise diff --git a/packages/coding-agent/test/discovery/claude-plugins.test.ts b/packages/coding-agent/test/discovery/claude-plugins.test.ts index 31a0cd867..f2a4c5d85 100644 --- a/packages/coding-agent/test/discovery/claude-plugins.test.ts +++ b/packages/coding-agent/test/discovery/claude-plugins.test.ts @@ -771,6 +771,92 @@ describe("listClaudePluginRoots", () => { expect(result.all.find(s => s.name === "alpha")).toBeDefined(); expect(result.all.find(s => s.name === "beta")).toBeDefined(); }); + + test("manifest skills field merges with default skills/ directory (adds, not replaces)", async () => { + // Per Claude plugins reference "Path behavior rules": + // `skills` adds to the default `skills/` scan; the default is always loaded + // alongside any manifest-declared directories. + const pluginsDir = path.join(tempDir, ".claude", "plugins"); + const pluginPath = path.join(tempDir, "plugins", "manifest-skills-merge"); + await fs.mkdir(pluginsDir, { recursive: true }); + await fs.mkdir(path.join(pluginPath, ".claude-plugin"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, "skills", "default-skill"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, "extra-skills", "extra-skill"), { recursive: true }); + + const registry = { + version: 2, + plugins: { + "manifest-skills-merge@market": [ + { + scope: "user", + installPath: pluginPath, + version: "1.0.0", + installedAt: "2025-01-01T00:00:00Z", + lastUpdated: "2025-01-01T00:00:00Z", + }, + ], + }, + }; + await fs.writeFile(path.join(pluginsDir, "installed_plugins.json"), JSON.stringify(registry)); + await fs.writeFile( + path.join(pluginPath, ".claude-plugin", "plugin.json"), + JSON.stringify({ skills: ["./extra-skills"] }), + ); + await fs.writeFile( + path.join(pluginPath, "skills", "default-skill", "SKILL.md"), + "---\nname: default-skill\ndescription: Default skill\n---\nBody\n", + ); + await fs.writeFile( + path.join(pluginPath, "extra-skills", "extra-skill", "SKILL.md"), + "---\nname: extra-skill\ndescription: Extra skill\n---\nBody\n", + ); + + const result = await loadCapability("skills", { cwd: tempDir }); + expect(result.warnings).toEqual([]); + expect(result.all.find(s => s.name === "default-skill")).toBeDefined(); + expect(result.all.find(s => s.name === "extra-skill")).toBeDefined(); + }); + + test("manifest commands field replaces default commands/ directory (Claude replace semantics)", async () => { + // Per Claude plugins reference "Path behavior rules": + // `commands` REPLACES the default `commands/` scan when the manifest key is set. + // A plugin that wants both must list `./commands` explicitly. + const pluginsDir = path.join(tempDir, ".claude", "plugins"); + const pluginPath = path.join(tempDir, "plugins", "manifest-commands-replace"); + await fs.mkdir(pluginsDir, { recursive: true }); + await fs.mkdir(path.join(pluginPath, ".claude-plugin"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, "commands"), { recursive: true }); + await fs.mkdir(path.join(pluginPath, "admin-commands"), { recursive: true }); + + const registry = { + version: 2, + plugins: { + "manifest-commands-replace@market": [ + { + scope: "user", + installPath: pluginPath, + version: "1.0.0", + installedAt: "2025-01-01T00:00:00Z", + lastUpdated: "2025-01-01T00:00:00Z", + }, + ], + }, + }; + await fs.writeFile(path.join(pluginsDir, "installed_plugins.json"), JSON.stringify(registry)); + await fs.writeFile( + path.join(pluginPath, ".claude-plugin", "plugin.json"), + JSON.stringify({ commands: ["./admin-commands"] }), + ); + // This file lives under the default commands/ dir and MUST NOT load once the + // manifest declares `commands` (Claude's documented "replaces default" semantic). + await fs.writeFile(path.join(pluginPath, "commands", "default.md"), "Default\n"); + await fs.writeFile(path.join(pluginPath, "admin-commands", "admin.md"), "Admin\n"); + + const result = await loadCapability("slash-commands", { cwd: tempDir }); + expect(result.warnings).toEqual([]); + expect(result.all.find(c => c.name === "manifest-commands-replace:admin")).toBeDefined(); + expect(result.all.find(c => c.name === "manifest-commands-replace:default")).toBeUndefined(); + }); }); describe("discoverAgents plugin precedence", () => {