fix(discovery): honour per-field Claude plugin path merge semantics
Review feedback on #4610: array-form skills was silently replacing the default `skills/` scan when the manifest declared any explicit entries. Per the Claude plugins reference "Path behavior rules" (https://code.claude.com/docs/en/plugins-reference#path-behavior-rules): - `skills` ADDS to the default `skills/` scan - `commands` / `slash-commands` REPLACE the default `commands/` scan `resolvePluginDir` now takes an explicit `includeFallback` flag. `loadSkills` passes `true` (fallback + declared entries, deduped by resolved absolute path so a manifest may still list `./skills` alongside extras without double-load); `loadSlashCommands` passes `false` (replace semantic preserved). Deduplication keeps the fallback first and declared entries in manifest order. Regression tests cover both semantics: skills-array merges with default `skills/`; commands-array replaces default `commands/` so a stray `commands/default.md` no longer loads once the manifest declares an alternative — matching Claude's documented behavior.
This commit is contained in:
roboomp
@@ -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/<cwd>/` 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)).
|
||||
|
||||
@@ -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<keyof ClaudePluginManifest>,
|
||||
fallback: string,
|
||||
includeFallback: boolean,
|
||||
): Promise<ResolvedPluginDir> {
|
||||
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<string>();
|
||||
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<LoadResult<Skill>> {
|
||||
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<LoadResult<SlashComm
|
||||
root,
|
||||
["commands", "slash-commands"],
|
||||
"commands",
|
||||
false,
|
||||
);
|
||||
const commandResults = await Promise.all(
|
||||
commandsDirs.map(dir =>
|
||||
|
||||
@@ -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<Skill>("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<SlashCommand>("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", () => {
|
||||
|
||||
Reference in New Issue
Block a user