fix(coding-agent): respect plugin scope and handle multiple registry entries
- P1: Use root.scope instead of hardcoded 'user' level in claude-plugins provider - P2: Iterate all registry entries per plugin ID, not just first one - P3: Add home parameter to discoverAgents() for test isolation - P4: Add test coverage for claude-plugins discovery (12 tests) - P5: Cache listClaudePluginRoots results to avoid repeated parsing Addresses issues found in adversarial review of PR #49
This commit is contained in:
@@ -34,7 +34,7 @@ async function loadSkills(ctx: LoadContext): Promise<LoadResult<Skill>> {
|
||||
return loadSkillsFromDir(ctx, {
|
||||
dir: skillsDir,
|
||||
providerId: PROVIDER_ID,
|
||||
level: "user", // Plugin cache is always user-level
|
||||
level: root.scope,
|
||||
});
|
||||
}),
|
||||
);
|
||||
@@ -61,7 +61,7 @@ async function loadSlashCommands(ctx: LoadContext): Promise<LoadResult<SlashComm
|
||||
const results = await Promise.all(
|
||||
roots.map(async root => {
|
||||
const commandsDir = path.join(root.path, "commands");
|
||||
return loadFilesFromDir<SlashCommand>(ctx, commandsDir, PROVIDER_ID, "user", {
|
||||
return loadFilesFromDir<SlashCommand>(ctx, commandsDir, PROVIDER_ID, root.scope, {
|
||||
extensions: ["md"],
|
||||
transform: (name, content, filePath, source) => {
|
||||
const cmdName = name.replace(/\.md$/, "");
|
||||
@@ -69,7 +69,7 @@ async function loadSlashCommands(ctx: LoadContext): Promise<LoadResult<SlashComm
|
||||
name: cmdName,
|
||||
path: filePath,
|
||||
content,
|
||||
level: "user" as const,
|
||||
level: root.scope,
|
||||
_source: source,
|
||||
};
|
||||
},
|
||||
@@ -108,7 +108,7 @@ async function loadHooks(ctx: LoadContext): Promise<LoadResult<Hook>> {
|
||||
const results = await Promise.all(
|
||||
loadTasks.map(async ({ root, hookType }) => {
|
||||
const hooksDir = path.join(root.path, "hooks", hookType);
|
||||
return loadFilesFromDir<Hook>(ctx, hooksDir, PROVIDER_ID, "user", {
|
||||
return loadFilesFromDir<Hook>(ctx, hooksDir, PROVIDER_ID, root.scope, {
|
||||
transform: (name, _content, filePath, source) => {
|
||||
const toolName = name.replace(/\.(sh|bash|zsh|fish)$/, "");
|
||||
return {
|
||||
@@ -116,7 +116,7 @@ async function loadHooks(ctx: LoadContext): Promise<LoadResult<Hook>> {
|
||||
path: filePath,
|
||||
type: hookType,
|
||||
tool: toolName,
|
||||
level: "user" as const,
|
||||
level: root.scope,
|
||||
_source: source,
|
||||
};
|
||||
},
|
||||
@@ -146,13 +146,13 @@ async function loadTools(ctx: LoadContext): Promise<LoadResult<CustomTool>> {
|
||||
const results = await Promise.all(
|
||||
roots.map(async root => {
|
||||
const toolsDir = path.join(root.path, "tools");
|
||||
return loadFilesFromDir<CustomTool>(ctx, toolsDir, PROVIDER_ID, "user", {
|
||||
return loadFilesFromDir<CustomTool>(ctx, toolsDir, PROVIDER_ID, root.scope, {
|
||||
transform: (name, _content, filePath, source) => {
|
||||
const toolName = name.replace(/\.(ts|js|sh|bash|py)$/, "");
|
||||
return {
|
||||
name: toolName,
|
||||
path: filePath,
|
||||
level: "user" as const,
|
||||
level: root.scope,
|
||||
_source: source,
|
||||
};
|
||||
},
|
||||
|
||||
@@ -602,8 +602,15 @@ export function parseClaudePluginsRegistry(content: string): ClaudePluginsRegist
|
||||
/**
|
||||
* List all installed Claude Code plugin roots from the plugin cache.
|
||||
* Reads ~/.claude/plugins/installed_plugins.json and resolves plugin paths.
|
||||
*
|
||||
* Results are cached per home directory to avoid repeated parsing.
|
||||
*/
|
||||
const pluginRootsCache = new Map<string, { roots: ClaudePluginRoot[]; warnings: string[] }>();
|
||||
|
||||
export async function listClaudePluginRoots(home: string): Promise<{ roots: ClaudePluginRoot[]; warnings: string[] }> {
|
||||
const cached = pluginRootsCache.get(home);
|
||||
if (cached) return cached;
|
||||
|
||||
const roots: ClaudePluginRoot[] = [];
|
||||
const warnings: string[] = [];
|
||||
|
||||
@@ -612,13 +619,17 @@ export async function listClaudePluginRoots(home: string): Promise<{ roots: Clau
|
||||
|
||||
if (!content) {
|
||||
// No registry file - not an error, just no plugins
|
||||
return { roots, warnings };
|
||||
const result = { roots, warnings };
|
||||
pluginRootsCache.set(home, result);
|
||||
return result;
|
||||
}
|
||||
|
||||
const registry = parseClaudePluginsRegistry(content);
|
||||
if (!registry) {
|
||||
warnings.push(`Failed to parse Claude Code plugin registry: ${registryPath}`);
|
||||
return { roots, warnings };
|
||||
const result = { roots, warnings };
|
||||
pluginRootsCache.set(home, result);
|
||||
return result;
|
||||
}
|
||||
|
||||
for (const [pluginId, entries] of Object.entries(registry.plugins)) {
|
||||
@@ -634,22 +645,33 @@ export async function listClaudePluginRoots(home: string): Promise<{ roots: Clau
|
||||
const pluginName = pluginId.slice(0, atIndex);
|
||||
const marketplace = pluginId.slice(atIndex + 1);
|
||||
|
||||
// Use the first (most recent) entry
|
||||
const entry = entries[0];
|
||||
if (!entry.installPath || typeof entry.installPath !== "string") {
|
||||
warnings.push(`Plugin ${pluginId} has no installPath`);
|
||||
continue;
|
||||
}
|
||||
// Process all valid entries, not just the first one.
|
||||
// This handles plugins with multiple installs (different scopes/versions).
|
||||
for (const entry of entries) {
|
||||
if (!entry.installPath || typeof entry.installPath !== "string") {
|
||||
warnings.push(`Plugin ${pluginId} entry has no installPath`);
|
||||
continue;
|
||||
}
|
||||
|
||||
roots.push({
|
||||
id: pluginId,
|
||||
marketplace,
|
||||
plugin: pluginName,
|
||||
version: entry.version || "unknown",
|
||||
path: entry.installPath,
|
||||
scope: entry.scope || "user",
|
||||
});
|
||||
roots.push({
|
||||
id: pluginId,
|
||||
marketplace,
|
||||
plugin: pluginName,
|
||||
version: entry.version || "unknown",
|
||||
path: entry.installPath,
|
||||
scope: entry.scope || "user",
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
return { roots, warnings };
|
||||
const result = { roots, warnings };
|
||||
pluginRootsCache.set(home, result);
|
||||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* Clear the plugin roots cache (useful for testing or when plugins change).
|
||||
*/
|
||||
export function clearClaudePluginRootsCache(): void {
|
||||
pluginRootsCache.clear();
|
||||
}
|
||||
|
||||
@@ -55,7 +55,7 @@ async function loadAgentsFromDir(dir: string, source: AgentSource): Promise<Agen
|
||||
*
|
||||
* @param cwd - Current working directory for project agent discovery
|
||||
*/
|
||||
export async function discoverAgents(cwd: string): Promise<DiscoveryResult> {
|
||||
export async function discoverAgents(cwd: string, home: string = os.homedir()): Promise<DiscoveryResult> {
|
||||
const resolvedCwd = path.resolve(cwd);
|
||||
const agentSources = Array.from(new Set(getConfigDirs("", { project: false }).map(entry => entry.source)));
|
||||
|
||||
@@ -88,10 +88,10 @@ export async function discoverAgents(cwd: string): Promise<DiscoveryResult> {
|
||||
}
|
||||
|
||||
// Load agents from Claude Code marketplace plugins
|
||||
const { roots: pluginRoots } = await listClaudePluginRoots(os.homedir());
|
||||
const { roots: pluginRoots } = await listClaudePluginRoots(home);
|
||||
for (const plugin of pluginRoots) {
|
||||
const agentsDir = path.join(plugin.path, "agents");
|
||||
orderedDirs.push({ dir: agentsDir, source: "user" });
|
||||
orderedDirs.push({ dir: agentsDir, source: plugin.scope === "project" ? "project" : "user" });
|
||||
}
|
||||
|
||||
const seen = new Set<string>();
|
||||
|
||||
@@ -0,0 +1,297 @@
|
||||
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { clearCache as clearFsCache } from "@oh-my-pi/pi-coding-agent/capability/fs";
|
||||
import {
|
||||
clearClaudePluginRootsCache,
|
||||
listClaudePluginRoots,
|
||||
parseClaudePluginsRegistry,
|
||||
} from "@oh-my-pi/pi-coding-agent/discovery/helpers";
|
||||
|
||||
describe("parseClaudePluginsRegistry", () => {
|
||||
test("parses valid registry", () => {
|
||||
const content = JSON.stringify({
|
||||
version: 2,
|
||||
plugins: {
|
||||
"my-plugin@marketplace": [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/plugin",
|
||||
version: "1.0.0",
|
||||
installedAt: "2025-01-01T00:00:00Z",
|
||||
lastUpdated: "2025-01-01T00:00:00Z",
|
||||
},
|
||||
],
|
||||
},
|
||||
});
|
||||
|
||||
const result = parseClaudePluginsRegistry(content);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.version).toBe(2);
|
||||
expect(result?.plugins["my-plugin@marketplace"]).toHaveLength(1);
|
||||
});
|
||||
|
||||
test("returns null for invalid JSON", () => {
|
||||
expect(parseClaudePluginsRegistry("not json")).toBeNull();
|
||||
});
|
||||
|
||||
test("returns null for missing version", () => {
|
||||
const content = JSON.stringify({ plugins: {} });
|
||||
expect(parseClaudePluginsRegistry(content)).toBeNull();
|
||||
});
|
||||
|
||||
test("returns null for missing plugins", () => {
|
||||
const content = JSON.stringify({ version: 2 });
|
||||
expect(parseClaudePluginsRegistry(content)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("listClaudePluginRoots", () => {
|
||||
let tempDir: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
clearClaudePluginRootsCache();
|
||||
clearFsCache();
|
||||
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "claude-plugins-test-"));
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
clearClaudePluginRootsCache();
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("returns empty roots when no registry file exists", async () => {
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toEqual([]);
|
||||
expect(result.warnings).toEqual([]);
|
||||
});
|
||||
|
||||
test("parses plugin with user scope", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"test-plugin@test-market": [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/test-plugin",
|
||||
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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toHaveLength(1);
|
||||
expect(result.roots[0]).toEqual({
|
||||
id: "test-plugin@test-market",
|
||||
marketplace: "test-market",
|
||||
plugin: "test-plugin",
|
||||
version: "1.0.0",
|
||||
path: "/path/to/test-plugin",
|
||||
scope: "user",
|
||||
});
|
||||
});
|
||||
|
||||
test("parses plugin with project scope", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"project-plugin@market": [
|
||||
{
|
||||
scope: "project",
|
||||
installPath: "/path/to/project-plugin",
|
||||
version: "2.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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toHaveLength(1);
|
||||
expect(result.roots[0].scope).toBe("project");
|
||||
});
|
||||
|
||||
test("handles multiple entries per plugin ID", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"multi-plugin@market": [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/v2",
|
||||
version: "2.0.0",
|
||||
installedAt: "2025-01-02T00:00:00Z",
|
||||
lastUpdated: "2025-01-02T00:00:00Z",
|
||||
},
|
||||
{
|
||||
scope: "project",
|
||||
installPath: "/path/to/v1",
|
||||
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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
// Should return both entries, not just the first one
|
||||
expect(result.roots).toHaveLength(2);
|
||||
expect(result.roots[0].version).toBe("2.0.0");
|
||||
expect(result.roots[0].scope).toBe("user");
|
||||
expect(result.roots[1].version).toBe("1.0.0");
|
||||
expect(result.roots[1].scope).toBe("project");
|
||||
});
|
||||
|
||||
test("warns on invalid plugin ID format", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"invalid-no-at-symbol": [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/invalid",
|
||||
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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toHaveLength(0);
|
||||
expect(result.warnings).toHaveLength(1);
|
||||
expect(result.warnings[0]).toContain("Invalid plugin ID format");
|
||||
});
|
||||
|
||||
test("warns on entry without installPath", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"no-path@market": [
|
||||
{
|
||||
scope: "user",
|
||||
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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toHaveLength(0);
|
||||
expect(result.warnings).toHaveLength(1);
|
||||
expect(result.warnings[0]).toContain("has no installPath");
|
||||
});
|
||||
|
||||
test("caches results for same home directory", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry: {
|
||||
version: number;
|
||||
plugins: Record<
|
||||
string,
|
||||
Array<{ scope: string; installPath: string; version: string; installedAt: string; lastUpdated: string }>
|
||||
>;
|
||||
} = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"cached-plugin@market": [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/cached",
|
||||
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));
|
||||
|
||||
// First call
|
||||
const result1 = await listClaudePluginRoots(tempDir);
|
||||
expect(result1.roots).toHaveLength(1);
|
||||
|
||||
// Modify the file
|
||||
registry.plugins["new-plugin@market"] = [
|
||||
{
|
||||
scope: "user",
|
||||
installPath: "/path/to/new",
|
||||
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));
|
||||
|
||||
// Second call should return cached result (still 1 plugin)
|
||||
const result2 = await listClaudePluginRoots(tempDir);
|
||||
expect(result2.roots).toHaveLength(1);
|
||||
|
||||
// After clearing cache, should see new plugin
|
||||
clearClaudePluginRootsCache();
|
||||
clearFsCache(); // Also clear fs cache so the file is re-read
|
||||
const result3 = await listClaudePluginRoots(tempDir);
|
||||
expect(result3.roots).toHaveLength(2);
|
||||
});
|
||||
|
||||
test("defaults scope to user when not specified", async () => {
|
||||
const pluginsDir = path.join(tempDir, ".claude", "plugins");
|
||||
await fs.mkdir(pluginsDir, { recursive: true });
|
||||
|
||||
const registry = {
|
||||
version: 2,
|
||||
plugins: {
|
||||
"no-scope@market": [
|
||||
{
|
||||
installPath: "/path/to/no-scope",
|
||||
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));
|
||||
|
||||
const result = await listClaudePluginRoots(tempDir);
|
||||
expect(result.roots).toHaveLength(1);
|
||||
expect(result.roots[0].scope).toBe("user");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user