From da5fc64853b47c2e8baedd1baf17bb1bcba2f8b5 Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Tue, 24 Feb 2026 22:28:08 -1000 Subject: [PATCH 1/3] feat(cli): multi-provider skill installation Add provider abstraction so `mpak skill install` and `mpak skill list` target the correct skills directory for Claude, Cursor, Copilot, Codex, Gemini, Goose, and OpenCode. Resolution priority: --provider flag > MPAK_PROVIDER env > config > auto-detect (single=use, zero=default claude, multiple=error). New commands: `mpak provider list|set|show` Fix: isValidProvider used `in` operator which traversed the prototype chain (constructor, toString returned true). Switched to Object.hasOwn. --- packages/cli/src/commands/provider.test.ts | 162 +++++++ packages/cli/src/commands/provider.ts | 72 +++ packages/cli/src/commands/skills/install.ts | 18 +- packages/cli/src/commands/skills/list.ts | 21 +- packages/cli/src/program.test.ts | 61 +++ packages/cli/src/program.ts | 44 +- packages/cli/src/utils/config-manager.test.ts | 87 ++++ packages/cli/src/utils/config-manager.ts | 23 + packages/cli/src/utils/providers.test.ts | 425 ++++++++++++++++++ packages/cli/src/utils/providers.ts | 137 ++++++ 10 files changed, 1025 insertions(+), 25 deletions(-) create mode 100644 packages/cli/src/commands/provider.test.ts create mode 100644 packages/cli/src/commands/provider.ts create mode 100644 packages/cli/src/utils/providers.test.ts create mode 100644 packages/cli/src/utils/providers.ts diff --git a/packages/cli/src/commands/provider.test.ts b/packages/cli/src/commands/provider.test.ts new file mode 100644 index 0000000..23845f1 --- /dev/null +++ b/packages/cli/src/commands/provider.test.ts @@ -0,0 +1,162 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +// ─── Mocks ────────────────────────────────────────────────────────────────── + +const detectProvidersMock = vi.fn(); +const resolveProviderMock = vi.fn(); +const setProviderMock = vi.fn(); + +vi.mock("../utils/providers.js", () => ({ + detectProviders: (...args: unknown[]) => detectProvidersMock(...args), + getProviderNames: () => + ["claude", "cursor", "copilot", "codex", "gemini", "goose", "opencode"], + getSkillsDir: (name: string) => `/home/user/.${name}/skills`, + isValidProvider: (name: string) => + ["claude", "cursor", "copilot", "codex", "gemini", "goose", "opencode"].includes(name), + resolveProvider: (...args: unknown[]) => resolveProviderMock(...args), +})); + +vi.mock("../utils/config-manager.js", () => ({ + ConfigManager: class { + setProvider(name: string) { + setProviderMock(name); + } + }, +})); + +import { + handleProviderList, + handleProviderSet, + handleProviderShow, +} from "./provider.js"; + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +let logOutput: string[]; +let stderrOutput: string[]; + +beforeEach(() => { + vi.clearAllMocks(); + logOutput = []; + stderrOutput = []; + vi.spyOn(console, "log").mockImplementation((...args) => { + logOutput.push(args.join(" ")); + }); + vi.spyOn(process.stderr, "write").mockImplementation((chunk) => { + stderrOutput.push(String(chunk)); + return true; + }); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +// ─── handleProviderList ───────────────────────────────────────────────────── + +describe("handleProviderList", () => { + it("marks detected providers with a checkmark", async () => { + detectProvidersMock.mockReturnValue(["claude", "cursor"]); + + await handleProviderList(); + + const output = logOutput.join("\n"); + // claude should have checkmark, codex should not + expect(output).toMatch(/✓\s+claude/); + expect(output).toMatch(/✓\s+cursor/); + expect(output).toMatch(/\s{2}\s+copilot/); // space, not checkmark + }); + + it("shows all 7 providers even when none detected", async () => { + detectProvidersMock.mockReturnValue([]); + + await handleProviderList(); + + const output = logOutput.join("\n"); + for (const name of ["claude", "cursor", "copilot", "codex", "gemini", "goose", "opencode"]) { + expect(output).toContain(name); + } + expect(output).toContain("No providers detected"); + }); + + it("shows detected summary when providers found", async () => { + detectProvidersMock.mockReturnValue(["gemini"]); + + await handleProviderList(); + + const output = logOutput.join("\n"); + expect(output).toContain("Detected: gemini"); + }); +}); + +// ─── handleProviderSet ────────────────────────────────────────────────────── + +describe("handleProviderSet", () => { + it("persists a valid provider to config", async () => { + await handleProviderSet("cursor"); + + expect(setProviderMock).toHaveBeenCalledWith("cursor"); + expect(logOutput.join("\n")).toContain("cursor"); + }); + + it("rejects an invalid provider name", async () => { + const exitError = new Error("process.exit"); + const exitSpy = vi + .spyOn(process, "exit") + .mockImplementation(() => { + throw exitError; + }); + + await expect(handleProviderSet("vscode")).rejects.toThrow("process.exit"); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(stderrOutput.join("")).toContain("Unknown provider"); + expect(stderrOutput.join("")).toContain("vscode"); + expect(setProviderMock).not.toHaveBeenCalled(); + + exitSpy.mockRestore(); + }); + + it("shows the skills directory in confirmation", async () => { + await handleProviderSet("claude"); + + const output = logOutput.join("\n"); + // Should show both the name and the path + expect(output).toContain("claude"); + expect(output).toContain(".claude/skills"); + }); +}); + +// ─── handleProviderShow ───────────────────────────────────────────────────── + +describe("handleProviderShow", () => { + it("displays the resolved provider and directory", async () => { + resolveProviderMock.mockReturnValue({ + provider: "cursor", + skillsDir: "/home/user/.cursor/skills", + }); + + await handleProviderShow(); + + const output = logOutput.join("\n"); + expect(output).toContain("cursor"); + expect(output).toContain("/home/user/.cursor/skills"); + }); + + it("exits with error when resolution fails (e.g. ambiguous)", async () => { + resolveProviderMock.mockImplementation(() => { + throw new Error("Multiple providers detected: claude, cursor"); + }); + + const exitSpy = vi + .spyOn(process, "exit") + .mockImplementation(() => undefined as never); + + await handleProviderShow(); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(stderrOutput.join("")).toContain("Multiple providers detected"); + + exitSpy.mockRestore(); + }); +}); diff --git a/packages/cli/src/commands/provider.ts b/packages/cli/src/commands/provider.ts new file mode 100644 index 0000000..77ef309 --- /dev/null +++ b/packages/cli/src/commands/provider.ts @@ -0,0 +1,72 @@ +import { ConfigManager } from "../utils/config-manager.js"; +import { + detectProviders, + getProviderNames, + getSkillsDir, + isValidProvider, + resolveProvider, +} from "../utils/providers.js"; + +/** + * List all supported providers and indicate which are detected + */ +export async function handleProviderList(): Promise { + const detected = detectProviders(); + const all = getProviderNames(); + + console.log(""); + console.log("Supported providers:"); + console.log(""); + + for (const name of all) { + const isDetected = detected.includes(name); + const marker = isDetected ? "\u2713" : " "; + const dir = getSkillsDir(name); + console.log(` ${marker} ${name.padEnd(10)} ${dir}`); + } + + console.log(""); + if (detected.length > 0) { + console.log(`Detected: ${detected.join(", ")}`); + } else { + console.log("No providers detected. Defaulting to claude."); + } +} + +/** + * Set the default provider + */ +export async function handleProviderSet(name: string): Promise { + if (!isValidProvider(name)) { + process.stderr.write( + `Error: Unknown provider: ${name}\n`, + ); + process.stderr.write( + `Valid providers: ${getProviderNames().join(", ")}\n`, + ); + process.exit(1); + } + + const configManager = new ConfigManager(); + configManager.setProvider(name); + + console.log( + `Default provider set to: ${name} (${getSkillsDir(name)})`, + ); +} + +/** + * Show the current resolved provider and skills directory + */ +export async function handleProviderShow(): Promise { + try { + const { provider, skillsDir } = resolveProvider(); + console.log(`Provider: ${provider}`); + console.log(`Skills directory: ${skillsDir}`); + } catch (err) { + process.stderr.write( + `${err instanceof Error ? err.message : String(err)}\n`, + ); + process.exit(1); + } +} diff --git a/packages/cli/src/commands/skills/install.ts b/packages/cli/src/commands/skills/install.ts index b414b6f..1cf1785 100644 --- a/packages/cli/src/commands/skills/install.ts +++ b/packages/cli/src/commands/skills/install.ts @@ -1,16 +1,10 @@ import { existsSync, mkdirSync, writeFileSync, rmSync } from "fs"; import { join, basename } from "path"; -import { homedir, tmpdir } from "os"; +import { tmpdir } from "os"; import { execFileSync } from "child_process"; import { formatSize, fmtError } from "../../utils/format.js"; import { createClient } from "../../utils/client.js"; - -/** - * Get the Claude Code skills directory - */ -function getSkillsDir(): string { - return join(homedir(), ".claude", "skills"); -} +import { resolveProvider } from "../../utils/providers.js"; /** * Parse skill spec into name and version @@ -45,6 +39,7 @@ function getShortName(scopedName: string): string { export interface InstallOptions { force?: boolean; json?: boolean; + provider?: string; } /** @@ -57,13 +52,15 @@ export async function handleSkillInstall( try { const { name, version } = parseSkillSpec(skillSpec); + // Resolve target provider + const { provider, skillsDir } = resolveProvider(options.provider); + // Get download info const client = createClient(); const downloadInfo = version ? await client.getSkillVersionDownload(name, version) : await client.getSkillDownload(name); const shortName = getShortName(downloadInfo.skill.name); - const skillsDir = getSkillsDir(); const installPath = join(skillsDir, shortName); // Check if already installed @@ -138,6 +135,7 @@ export async function handleSkillInstall( shortName, version: downloadInfo.skill.version, path: installPath, + provider, }, null, 2, @@ -148,7 +146,7 @@ export async function handleSkillInstall( console.log(`\u2713 Installed: ${shortName}`); console.log(""); console.log( - "Skill available in Claude Code. Restart to activate.", + `Skill available in ${provider}. Restart to activate.`, ); } } catch (err) { diff --git a/packages/cli/src/commands/skills/list.ts b/packages/cli/src/commands/skills/list.ts index 80bdc37..a0bd5f9 100644 --- a/packages/cli/src/commands/skills/list.ts +++ b/packages/cli/src/commands/skills/list.ts @@ -1,14 +1,7 @@ import { existsSync, readdirSync, readFileSync, statSync } from "fs"; import { join } from "path"; -import { homedir } from "os"; import matter from "gray-matter"; - -/** - * Get the Claude Code skills directory - */ -function getSkillsDir(): string { - return join(homedir(), ".claude", "skills"); -} +import { resolveProvider } from "../../utils/providers.js"; interface InstalledSkill { name: string; @@ -20,9 +13,7 @@ interface InstalledSkill { /** * List all installed skills */ -function listInstalledSkills(): InstalledSkill[] { - const skillsDir = getSkillsDir(); - +function listInstalledSkills(skillsDir: string): InstalledSkill[] { if (!existsSync(skillsDir)) { return []; } @@ -72,6 +63,7 @@ function listInstalledSkills(): InstalledSkill[] { export interface ListOptions { json?: boolean; + provider?: string; } /** @@ -80,7 +72,8 @@ export interface ListOptions { export async function handleSkillList( options: ListOptions, ): Promise { - const skills = listInstalledSkills(); + const { provider, skillsDir } = resolveProvider(options.provider); + const skills = listInstalledSkills(skillsDir); if (options.json) { console.log(JSON.stringify(skills, null, 2)); @@ -91,7 +84,7 @@ export async function handleSkillList( console.log("No skills installed."); console.log(""); console.log("Install skills with: mpak skill install "); - console.log("Or create your own in ~/.claude/skills/"); + console.log(`Or create your own in ${skillsDir}/`); return; } @@ -122,6 +115,6 @@ export async function handleSkillList( console.log(""); console.log( - `${skills.length} skill(s) installed in ${getSkillsDir()}`, + `${skills.length} skill(s) installed in ${skillsDir} (${provider})`, ); } diff --git a/packages/cli/src/program.test.ts b/packages/cli/src/program.test.ts index 58b1d0f..545f770 100644 --- a/packages/cli/src/program.test.ts +++ b/packages/cli/src/program.test.ts @@ -1,5 +1,18 @@ import { describe, it, expect } from "vitest"; import { createProgram } from "./program.js"; +import type { Command } from "commander"; + +/** + * Walk the commander tree to find a subcommand by dotted path. + * e.g. findCommand(program, "skill", "install") finds `mpak skill install` + */ +function findCommand(root: Command, ...path: string[]): Command | undefined { + let cmd: Command | undefined = root; + for (const name of path) { + cmd = cmd?.commands.find((c) => c.name() === name); + } + return cmd; +} describe("createProgram", () => { it("should create a program with correct name", () => { @@ -21,4 +34,52 @@ describe("createProgram", () => { ); expect(versionOption).toBeDefined(); }); + + describe("provider command registration", () => { + it("registers provider subcommand group", () => { + const program = createProgram(); + const provider = findCommand(program, "provider"); + expect(provider).toBeDefined(); + }); + + it("registers provider list, set, and show subcommands", () => { + const program = createProgram(); + const provider = findCommand(program, "provider"); + const subNames = provider?.commands.map((c) => c.name()); + expect(subNames).toContain("list"); + expect(subNames).toContain("set"); + expect(subNames).toContain("show"); + }); + }); + + describe("--provider flag wiring", () => { + it("skill install accepts --provider / -p", () => { + const program = createProgram(); + const install = findCommand(program, "skill", "install"); + const opt = install?.options.find( + (o) => o.long === "--provider", + ); + expect(opt).toBeDefined(); + expect(opt?.short).toBe("-p"); + }); + + it("skill list accepts --provider / -p", () => { + const program = createProgram(); + const list = findCommand(program, "skill", "list"); + const opt = list?.options.find( + (o) => o.long === "--provider", + ); + expect(opt).toBeDefined(); + expect(opt?.short).toBe("-p"); + }); + + it("skill search does NOT accept --provider (not applicable)", () => { + const program = createProgram(); + const search = findCommand(program, "skill", "search"); + const opt = search?.options.find( + (o) => o.long === "--provider", + ); + expect(opt).toBeUndefined(); + }); + }); }); diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 4daaef9..6094ffa 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -21,6 +21,11 @@ import { handleSkillInstall, handleSkillList, } from "./commands/skills/index.js"; +import { + handleProviderList, + handleProviderSet, + handleProviderShow, +} from "./commands/provider.js"; /** * Creates and configures the CLI program @@ -173,9 +178,13 @@ export function createProgram(): Command { skill .command("install ") - .description("Install a skill to ~/.claude/skills/") + .description("Install a skill to the target provider's skills directory") .option("--force", "Overwrite existing installation") .option("--json", "Output as JSON") + .option( + "-p, --provider ", + "Target provider (claude, cursor, copilot, codex, gemini, goose, opencode)", + ) .action(async (name, options) => { await handleSkillInstall(name, options); }); @@ -184,6 +193,10 @@ export function createProgram(): Command { .command("list") .description("List installed skills") .option("--json", "Output as JSON") + .option( + "-p, --provider ", + "Target provider (claude, cursor, copilot, codex, gemini, goose, opencode)", + ) .action(async (options) => { await handleSkillList(options); }); @@ -226,6 +239,35 @@ export function createProgram(): Command { await handleConfigClear(packageName, key); }); + // ========================================================================== + // Provider commands + // ========================================================================== + + const providerCmd = program + .command("provider") + .description("Manage skill provider (claude, cursor, copilot, etc.)"); + + providerCmd + .command("list") + .description("List supported providers and show which are detected") + .action(async () => { + await handleProviderList(); + }); + + providerCmd + .command("set ") + .description("Set the default provider") + .action(async (name) => { + await handleProviderSet(name); + }); + + providerCmd + .command("show") + .description("Show the current resolved provider and skills directory") + .action(async () => { + await handleProviderShow(); + }); + // ========================================================================== // Shell completion // ========================================================================== diff --git a/packages/cli/src/utils/config-manager.test.ts b/packages/cli/src/utils/config-manager.test.ts index 536ae2c..bd5eeec 100644 --- a/packages/cli/src/utils/config-manager.test.ts +++ b/packages/cli/src/utils/config-manager.test.ts @@ -461,5 +461,92 @@ describe("ConfigManager", () => { config.packages?.["@scope/pkg"]?.["api_key"], ).toBe("secret"); }); + + it("should throw ConfigCorruptedError when provider is not a string", () => { + writeFileSync( + testConfigFile, + JSON.stringify({ + version: "1.0.0", + lastUpdated: "2024-01-01T00:00:00Z", + provider: 12345, + }), + { mode: 0o600 }, + ); + + const manager = new ConfigManager(); + expect(() => manager.loadConfig()).toThrow( + ConfigCorruptedError, + ); + expect(() => manager.loadConfig()).toThrow( + /provider must be a string/, + ); + }); + + it("should load config with valid provider field", () => { + writeFileSync( + testConfigFile, + JSON.stringify({ + version: "1.0.0", + lastUpdated: "2024-01-01T00:00:00Z", + provider: "cursor", + }), + { mode: 0o600 }, + ); + + const manager = new ConfigManager(); + const config = manager.loadConfig(); + expect(config.provider).toBe("cursor"); + }); + }); + + describe("provider", () => { + it("should persist provider to disk across instances", () => { + // Instance 1 writes + const writer = new ConfigManager(); + writer.setProvider("cursor"); + + // Instance 2 reads from disk (fresh, no cache) + const reader = new ConfigManager(); + expect(reader.getProvider()).toBe("cursor"); + }); + + it("should return undefined when no provider is set", () => { + const manager = new ConfigManager(); + expect(manager.getProvider()).toBeUndefined(); + }); + + it("should overwrite existing provider and persist the latest", () => { + const manager = new ConfigManager(); + manager.setProvider("cursor"); + manager.setProvider("copilot"); + + // Verify latest value persisted to disk + const reader = new ConfigManager(); + expect(reader.getProvider()).toBe("copilot"); + }); + + it("should accept arbitrary strings (no validation at storage layer)", () => { + // ConfigManager.setProvider does NOT validate against known providers. + // resolveProvider() is responsible for validation on read. + // This test documents that design decision. + const manager = new ConfigManager(); + manager.setProvider("invented-provider"); + expect(manager.getProvider()).toBe("invented-provider"); + }); + + it("should not interfere with other config fields", () => { + const manager = new ConfigManager(); + manager.setRegistryUrl("https://custom.example.com"); + manager.setPackageConfigValue("@scope/pkg", "key", "value"); + manager.setProvider("cursor"); + + // All three fields should coexist + const reader = new ConfigManager(); + expect(reader.getProvider()).toBe("cursor"); + expect(reader.getRegistryUrl()).toBe("https://custom.example.com"); + expect(reader.getPackageConfigValue("@scope/pkg", "key")).toBe( + "value", + ); + }); }); }); diff --git a/packages/cli/src/utils/config-manager.ts b/packages/cli/src/utils/config-manager.ts index 6ba586f..c6dfc14 100644 --- a/packages/cli/src/utils/config-manager.ts +++ b/packages/cli/src/utils/config-manager.ts @@ -21,6 +21,7 @@ export interface MpakConfig { version: string; lastUpdated: string; registryUrl?: string; + provider?: string; packages?: Record; } @@ -77,6 +78,16 @@ function validateConfig(data: unknown, configPath: string): MpakConfig { ); } + if ( + obj["provider"] !== undefined && + typeof obj["provider"] !== "string" + ) { + throw new ConfigCorruptedError( + "Config field provider must be a string", + configPath, + ); + } + if (obj["packages"] !== undefined) { if (typeof obj["packages"] !== "object" || obj["packages"] === null) { throw new ConfigCorruptedError( @@ -114,6 +125,7 @@ function validateConfig(data: unknown, configPath: string): MpakConfig { "version", "lastUpdated", "registryUrl", + "provider", "packages", ]); for (const key of Object.keys(obj)) { @@ -213,6 +225,17 @@ export class ConfigManager { ); } + setProvider(name: string): void { + const config = this.loadConfig(); + config.provider = name; + this.saveConfig(); + } + + getProvider(): string | undefined { + const config = this.loadConfig(); + return config.provider; + } + /** * Get all stored config values for a package */ diff --git a/packages/cli/src/utils/providers.test.ts b/packages/cli/src/utils/providers.test.ts new file mode 100644 index 0000000..aa544a5 --- /dev/null +++ b/packages/cli/src/utils/providers.test.ts @@ -0,0 +1,425 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { join } from "path"; +import { homedir } from "os"; + +// Mock fs.existsSync before importing the module +vi.mock("fs", async () => { + const actual = await vi.importActual("fs"); + return { + ...(actual as object), + existsSync: vi.fn( + (actual as Record).existsSync as () => boolean, + ), + }; +}); + +const getProviderMock = vi.fn(); + +// Mock config-manager with a proper class +vi.mock("./config-manager.js", () => { + return { + ConfigManager: class MockConfigManager { + getProvider() { + return getProviderMock(); + } + loadConfig() { + return { + version: "1.0.0", + lastUpdated: new Date().toISOString(), + }; + } + }, + }; +}); + +import { existsSync } from "fs"; +import { + detectProviders, + getProviderNames, + getSkillsDir, + isValidProvider, + resolveProvider, +} from "./providers.js"; + +const mockedExistsSync = vi.mocked(existsSync); + +describe("providers", () => { + beforeEach(() => { + vi.clearAllMocks(); + delete process.env["MPAK_PROVIDER"]; + }); + + afterEach(() => { + delete process.env["MPAK_PROVIDER"]; + }); + + // ========================================================================= + // Structural invariants — catch real bugs when someone edits the maps + // ========================================================================= + + describe("provider map alignment", () => { + it("every provider in PROVIDERS has a corresponding detection parent", () => { + // If someone adds a provider to PROVIDERS but forgets PROVIDER_PARENTS, + // detectProviders() will never find it. This test catches that. + const names = getProviderNames(); + for (const name of names) { + // detectProviders checks PROVIDER_PARENTS, so simulate "all exist" + mockedExistsSync.mockReturnValue(true); + const detected = detectProviders(); + expect( + detected, + `Provider "${name}" exists in PROVIDERS but not in PROVIDER_PARENTS`, + ).toContain(name); + } + }); + + it("every provider has a skills dir ending in /skills", () => { + for (const name of getProviderNames()) { + const dir = getSkillsDir(name); + expect( + dir.endsWith("/skills") || dir.endsWith("\\skills"), + `${name}'s skills dir "${dir}" doesn't end with /skills`, + ).toBe(true); + } + }); + + it("every provider's skills dir is under homedir", () => { + const home = homedir(); + for (const name of getProviderNames()) { + const dir = getSkillsDir(name); + expect( + dir.startsWith(home), + `${name}'s skills dir "${dir}" is not under ${home}`, + ).toBe(true); + } + }); + }); + + // ========================================================================= + // isValidProvider — boundary cases + // ========================================================================= + + describe("isValidProvider", () => { + it("rejects names with wrong casing", () => { + expect(isValidProvider("Claude")).toBe(false); + expect(isValidProvider("CURSOR")).toBe(false); + }); + + it("rejects names that are substrings of valid providers", () => { + expect(isValidProvider("claud")).toBe(false); + expect(isValidProvider("cursors")).toBe(false); + }); + + it("rejects prototype pollution keys", () => { + expect(isValidProvider("constructor")).toBe(false); + expect(isValidProvider("__proto__")).toBe(false); + expect(isValidProvider("toString")).toBe(false); + }); + }); + + // ========================================================================= + // detectProviders — filesystem interaction + // ========================================================================= + + describe("detectProviders", () => { + it("returns empty when nothing exists", () => { + mockedExistsSync.mockReturnValue(false); + expect(detectProviders()).toEqual([]); + }); + + it("returns all providers when all parent dirs exist", () => { + mockedExistsSync.mockReturnValue(true); + const detected = detectProviders(); + expect(detected).toEqual(getProviderNames()); + }); + + it("checks the parent dir, not the skills dir itself", () => { + // goose skills dir is ~/.config/agents/skills + // detection should check ~/.config/agents, NOT ~/.config/agents/skills + const calledPaths: string[] = []; + mockedExistsSync.mockImplementation((p) => { + calledPaths.push(String(p)); + return false; + }); + detectProviders(); + + // Verify no checked path ends with /skills + for (const p of calledPaths) { + expect( + p.endsWith("/skills") || p.endsWith("\\skills"), + `detectProviders checked skills dir directly: ${p}`, + ).toBe(false); + } + }); + + it("only checks the expected number of paths (one per provider)", () => { + mockedExistsSync.mockReturnValue(false); + detectProviders(); + expect(mockedExistsSync).toHaveBeenCalledTimes( + getProviderNames().length, + ); + }); + }); + + // ========================================================================= + // resolveProvider — priority chain semantics + // ========================================================================= + + describe("resolveProvider priority chain", () => { + it("explicit flag beats everything else", () => { + process.env["MPAK_PROVIDER"] = "copilot"; + getProviderMock.mockReturnValue("gemini"); + mockedExistsSync.mockReturnValue(true); // all providers detected + + const result = resolveProvider("cursor"); + expect(result.provider).toBe("cursor"); + }); + + it("env var beats config and detection", () => { + process.env["MPAK_PROVIDER"] = "copilot"; + getProviderMock.mockReturnValue("gemini"); + mockedExistsSync.mockReturnValue(true); + + const result = resolveProvider(); + expect(result.provider).toBe("copilot"); + }); + + it("config beats detection", () => { + getProviderMock.mockReturnValue("gemini"); + // Multiple providers detected — would normally error, but config wins + mockedExistsSync.mockReturnValue(true); + + const result = resolveProvider(); + expect(result.provider).toBe("gemini"); + }); + }); + + // ========================================================================= + // resolveProvider — result consistency + // ========================================================================= + + describe("resolveProvider result consistency", () => { + it("skillsDir in result always matches getSkillsDir(provider)", () => { + // Test across all resolution paths + const scenarios: Array<{ + label: string; + setup: () => void; + explicit?: string; + }> = [ + { + label: "explicit flag", + setup: () => {}, + explicit: "cursor", + }, + { + label: "env var", + setup: () => { + process.env["MPAK_PROVIDER"] = "copilot"; + }, + }, + { + label: "config", + setup: () => { + getProviderMock.mockReturnValue("gemini"); + }, + }, + { + label: "auto-detect single", + setup: () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockImplementation( + (p) => p === join(homedir(), ".codex"), + ); + }, + }, + { + label: "fallback to claude", + setup: () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockReturnValue(false); + }, + }, + ]; + + for (const { label, setup, explicit } of scenarios) { + vi.clearAllMocks(); + delete process.env["MPAK_PROVIDER"]; + setup(); + + const result = resolveProvider(explicit); + expect( + result.skillsDir, + `${label}: skillsDir doesn't match getSkillsDir(${result.provider})`, + ).toBe(getSkillsDir(result.provider)); + } + }); + }); + + // ========================================================================= + // resolveProvider — edge cases + // ========================================================================= + + describe("resolveProvider edge cases", () => { + it("treats empty string explicit as no explicit (falls through)", () => { + // Empty string is falsy, so resolveProvider("") should behave like resolveProvider() + getProviderMock.mockReturnValue("gemini"); + const result = resolveProvider(""); + expect(result.provider).toBe("gemini"); + }); + + it("defaults to claude when zero providers detected", () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockReturnValue(false); + + const result = resolveProvider(); + expect(result.provider).toBe("claude"); + expect(result.skillsDir).toBe(getSkillsDir("claude")); + }); + + it("uses the sole detected provider without error", () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockImplementation( + (p) => p === join(homedir(), ".cursor"), + ); + + const result = resolveProvider(); + expect(result.provider).toBe("cursor"); + }); + }); + + // ========================================================================= + // resolveProvider — error message quality + // ========================================================================= + + describe("resolveProvider error messages", () => { + it("invalid explicit provider lists valid names in error", () => { + try { + resolveProvider("vscode"); + expect.fail("should have thrown"); + } catch (err) { + const msg = (err as Error).message; + // Must include the bad name AND at least some valid names + expect(msg).toContain("vscode"); + expect(msg).toContain("claude"); + expect(msg).toContain("cursor"); + } + }); + + it("invalid env var provider identifies the source in error", () => { + process.env["MPAK_PROVIDER"] = "bad-name"; + try { + resolveProvider(); + expect.fail("should have thrown"); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toContain("MPAK_PROVIDER"); + expect(msg).toContain("bad-name"); + } + }); + + it("invalid config provider identifies the source in error", () => { + getProviderMock.mockReturnValue("stale-value"); + try { + resolveProvider(); + expect.fail("should have thrown"); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toContain("config"); + expect(msg).toContain("stale-value"); + } + }); + + it("ambiguous detection error lists the detected provider names", () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockImplementation((p) => { + return ( + p === join(homedir(), ".claude") || + p === join(homedir(), ".gemini") + ); + }); + + try { + resolveProvider(); + expect.fail("should have thrown"); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toContain("claude"); + expect(msg).toContain("gemini"); + // Should NOT mention providers that weren't detected + expect(msg).not.toMatch(/\bcursor\b/); + } + }); + + it("ambiguous detection error suggests remediation commands", () => { + getProviderMock.mockReturnValue(undefined); + mockedExistsSync.mockImplementation((p) => { + return ( + p === join(homedir(), ".claude") || + p === join(homedir(), ".cursor") + ); + }); + + try { + resolveProvider(); + expect.fail("should have thrown"); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toContain("mpak provider set"); + expect(msg).toContain("--provider"); + } + }); + }); + + // ========================================================================= + // resolveProvider — each layer validates independently + // ========================================================================= + + describe("resolveProvider validates at each layer", () => { + it("explicit: rejects known-bad names even when env/config would succeed", () => { + process.env["MPAK_PROVIDER"] = "claude"; + getProviderMock.mockReturnValue("claude"); + + expect(() => resolveProvider("nope")).toThrow(/Unknown provider: nope/); + }); + + it("env: rejects known-bad names even when config would succeed", () => { + process.env["MPAK_PROVIDER"] = "nope"; + getProviderMock.mockReturnValue("claude"); + + expect(() => resolveProvider()).toThrow(/MPAK_PROVIDER/); + }); + + it("config: rejects known-bad names even when detection would succeed", () => { + getProviderMock.mockReturnValue("nope"); + mockedExistsSync.mockImplementation( + (p) => p === join(homedir(), ".claude"), + ); + + expect(() => resolveProvider()).toThrow(/config/); + }); + }); + + // ========================================================================= + // resolveProvider — exercising every provider through every path + // ========================================================================= + + describe("resolveProvider works for all providers", () => { + const allProviders = [ + "claude", + "cursor", + "copilot", + "codex", + "gemini", + "goose", + "opencode", + ] as const; + + for (const name of allProviders) { + it(`resolves ${name} via explicit flag`, () => { + const result = resolveProvider(name); + expect(result.provider).toBe(name); + expect(result.skillsDir).toBe(getSkillsDir(name)); + }); + } + }); +}); diff --git a/packages/cli/src/utils/providers.ts b/packages/cli/src/utils/providers.ts new file mode 100644 index 0000000..1d361b0 --- /dev/null +++ b/packages/cli/src/utils/providers.ts @@ -0,0 +1,137 @@ +import { existsSync } from "fs"; +import { homedir } from "os"; +import { join } from "path"; +import { ConfigManager } from "./config-manager.js"; + +/** + * Provider name → default skills directory + */ +const PROVIDERS = { + claude: () => join(homedir(), ".claude", "skills"), + cursor: () => join(homedir(), ".cursor", "skills"), + copilot: () => join(homedir(), ".copilot", "skills"), + codex: () => join(homedir(), ".codex", "skills"), + gemini: () => join(homedir(), ".gemini", "skills"), + goose: () => join(homedir(), ".config", "agents", "skills"), + opencode: () => join(homedir(), ".config", "opencode", "skills"), +} as const; + +export type ProviderName = keyof typeof PROVIDERS; + +/** + * Provider parent directories used for detection + */ +const PROVIDER_PARENTS: Record string> = { + claude: () => join(homedir(), ".claude"), + cursor: () => join(homedir(), ".cursor"), + copilot: () => join(homedir(), ".copilot"), + codex: () => join(homedir(), ".codex"), + gemini: () => join(homedir(), ".gemini"), + goose: () => join(homedir(), ".config", "agents"), + opencode: () => join(homedir(), ".config", "opencode"), +}; + +/** + * List all valid provider names + */ +export function getProviderNames(): ProviderName[] { + return Object.keys(PROVIDERS) as ProviderName[]; +} + +/** + * Check if a string is a valid provider name + */ +export function isValidProvider(name: string): name is ProviderName { + return Object.hasOwn(PROVIDERS, name); +} + +/** + * Get the skills directory for a specific provider + */ +export function getSkillsDir(provider: ProviderName): string { + return PROVIDERS[provider](); +} + +/** + * Detect which providers are present by checking if their parent directories exist + */ +export function detectProviders(): ProviderName[] { + const detected: ProviderName[] = []; + for (const [name, parentDir] of Object.entries(PROVIDER_PARENTS)) { + if (existsSync(parentDir())) { + detected.push(name as ProviderName); + } + } + return detected; +} + +/** + * Resolution result from resolveProvider + */ +export interface ResolvedProvider { + provider: ProviderName; + skillsDir: string; +} + +/** + * Resolve the target provider using the priority chain: + * 1. Explicit --provider flag + * 2. MPAK_PROVIDER env var + * 3. provider field in ~/.mpak/config.json + * 4. Auto-detect: exactly 1 → use it, 0 → default to claude, multiple → error + */ +export function resolveProvider(explicit?: string): ResolvedProvider { + // 1. Explicit --provider flag + if (explicit) { + if (!isValidProvider(explicit)) { + throw new Error( + `Unknown provider: ${explicit}\nValid providers: ${getProviderNames().join(", ")}`, + ); + } + return { provider: explicit, skillsDir: getSkillsDir(explicit) }; + } + + // 2. MPAK_PROVIDER env var + const envProvider = process.env["MPAK_PROVIDER"]; + if (envProvider) { + if (!isValidProvider(envProvider)) { + throw new Error( + `Unknown provider in MPAK_PROVIDER: ${envProvider}\nValid providers: ${getProviderNames().join(", ")}`, + ); + } + return { provider: envProvider, skillsDir: getSkillsDir(envProvider) }; + } + + // 3. Config file + const configManager = new ConfigManager(); + const configProvider = configManager.getProvider(); + if (configProvider) { + if (!isValidProvider(configProvider)) { + throw new Error( + `Unknown provider in config: ${configProvider}\nValid providers: ${getProviderNames().join(", ")}`, + ); + } + return { + provider: configProvider, + skillsDir: getSkillsDir(configProvider), + }; + } + + // 4. Auto-detect + const detected = detectProviders(); + + if (detected.length === 1) { + const provider = detected[0]!; + return { provider, skillsDir: getSkillsDir(provider) }; + } + + if (detected.length === 0) { + // Default to claude when nothing is detected + return { provider: "claude", skillsDir: getSkillsDir("claude") }; + } + + // Multiple providers detected — ambiguous + throw new Error( + `Multiple providers detected: ${detected.join(", ")}\nSet a default: mpak provider set \nOr specify: mpak skill install --provider `, + ); +} From 6c617456dc17076296925f59c5227a99c0f02c5c Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 26 Feb 2026 04:16:24 -1000 Subject: [PATCH 2/3] fix(cli): default to claude on ambiguous detection, improve goose detection - Multiple detected providers now defaults to claude with a stderr warning instead of throwing, preserving backward compatibility - Goose detection uses ~/.config/goose instead of generic ~/.config/agents - Test mocks use vi.importActual to stay in sync with real provider list - Add test for empty MPAK_PROVIDER env var fallthrough --- packages/cli/src/commands/provider.test.ts | 25 ++++---- packages/cli/src/utils/providers.test.ts | 66 +++++++++++++++------- packages/cli/src/utils/providers.ts | 12 ++-- 3 files changed, 65 insertions(+), 38 deletions(-) diff --git a/packages/cli/src/commands/provider.test.ts b/packages/cli/src/commands/provider.test.ts index 23845f1..ffe21cb 100644 --- a/packages/cli/src/commands/provider.test.ts +++ b/packages/cli/src/commands/provider.test.ts @@ -6,15 +6,16 @@ const detectProvidersMock = vi.fn(); const resolveProviderMock = vi.fn(); const setProviderMock = vi.fn(); -vi.mock("../utils/providers.js", () => ({ - detectProviders: (...args: unknown[]) => detectProvidersMock(...args), - getProviderNames: () => - ["claude", "cursor", "copilot", "codex", "gemini", "goose", "opencode"], - getSkillsDir: (name: string) => `/home/user/.${name}/skills`, - isValidProvider: (name: string) => - ["claude", "cursor", "copilot", "codex", "gemini", "goose", "opencode"].includes(name), - resolveProvider: (...args: unknown[]) => resolveProviderMock(...args), -})); +vi.mock("../utils/providers.js", async () => { + const actual = await vi.importActual("../utils/providers.js") as Record; + return { + detectProviders: (...args: unknown[]) => detectProvidersMock(...args), + getProviderNames: actual["getProviderNames"], + getSkillsDir: actual["getSkillsDir"], + isValidProvider: actual["isValidProvider"], + resolveProvider: (...args: unknown[]) => resolveProviderMock(...args), + }; +}); vi.mock("../utils/config-manager.js", () => ({ ConfigManager: class { @@ -143,9 +144,9 @@ describe("handleProviderShow", () => { expect(output).toContain("/home/user/.cursor/skills"); }); - it("exits with error when resolution fails (e.g. ambiguous)", async () => { + it("exits with error when resolution fails (e.g. invalid config)", async () => { resolveProviderMock.mockImplementation(() => { - throw new Error("Multiple providers detected: claude, cursor"); + throw new Error("Unknown provider in config: stale-value"); }); const exitSpy = vi @@ -155,7 +156,7 @@ describe("handleProviderShow", () => { await handleProviderShow(); expect(exitSpy).toHaveBeenCalledWith(1); - expect(stderrOutput.join("")).toContain("Multiple providers detected"); + expect(stderrOutput.join("")).toContain("Unknown provider in config"); exitSpy.mockRestore(); }); diff --git a/packages/cli/src/utils/providers.test.ts b/packages/cli/src/utils/providers.test.ts index aa544a5..092aacb 100644 --- a/packages/cli/src/utils/providers.test.ts +++ b/packages/cli/src/utils/providers.test.ts @@ -135,7 +135,7 @@ describe("providers", () => { it("checks the parent dir, not the skills dir itself", () => { // goose skills dir is ~/.config/agents/skills - // detection should check ~/.config/agents, NOT ~/.config/agents/skills + // detection should check ~/.config/goose, NOT ~/.config/agents/skills const calledPaths: string[] = []; mockedExistsSync.mockImplementation((p) => { calledPaths.push(String(p)); @@ -267,6 +267,14 @@ describe("providers", () => { expect(result.provider).toBe("gemini"); }); + it("treats empty MPAK_PROVIDER as unset (falls through to config)", () => { + process.env["MPAK_PROVIDER"] = ""; + getProviderMock.mockReturnValue("copilot"); + + const result = resolveProvider(); + expect(result.provider).toBe("copilot"); + }); + it("defaults to claude when zero providers detected", () => { getProviderMock.mockReturnValue(undefined); mockedExistsSync.mockReturnValue(false); @@ -329,7 +337,7 @@ describe("providers", () => { } }); - it("ambiguous detection error lists the detected provider names", () => { + it("ambiguous detection defaults to claude and warns with detected names", () => { getProviderMock.mockReturnValue(undefined); mockedExistsSync.mockImplementation((p) => { return ( @@ -338,19 +346,28 @@ describe("providers", () => { ); }); - try { - resolveProvider(); - expect.fail("should have thrown"); - } catch (err) { - const msg = (err as Error).message; - expect(msg).toContain("claude"); - expect(msg).toContain("gemini"); - // Should NOT mention providers that weren't detected - expect(msg).not.toMatch(/\bcursor\b/); - } + const stderrChunks: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk) => { + stderrChunks.push(String(chunk)); + return true; + }); + + const result = resolveProvider(); + expect(result.provider).toBe("claude"); + expect(result.skillsDir).toBe(getSkillsDir("claude")); + + const warning = stderrChunks.join(""); + expect(warning).toContain("claude"); + expect(warning).toContain("gemini"); + // Should NOT mention providers that weren't detected + expect(warning).not.toMatch(/\bcursor\b/); + + spy.mockRestore(); }); - it("ambiguous detection error suggests remediation commands", () => { + it("ambiguous detection warning suggests remediation commands", () => { getProviderMock.mockReturnValue(undefined); mockedExistsSync.mockImplementation((p) => { return ( @@ -359,14 +376,21 @@ describe("providers", () => { ); }); - try { - resolveProvider(); - expect.fail("should have thrown"); - } catch (err) { - const msg = (err as Error).message; - expect(msg).toContain("mpak provider set"); - expect(msg).toContain("--provider"); - } + const stderrChunks: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk) => { + stderrChunks.push(String(chunk)); + return true; + }); + + resolveProvider(); + + const warning = stderrChunks.join(""); + expect(warning).toContain("mpak provider set"); + expect(warning).toContain("--provider"); + + spy.mockRestore(); }); }); diff --git a/packages/cli/src/utils/providers.ts b/packages/cli/src/utils/providers.ts index 1d361b0..baa8e4c 100644 --- a/packages/cli/src/utils/providers.ts +++ b/packages/cli/src/utils/providers.ts @@ -27,7 +27,7 @@ const PROVIDER_PARENTS: Record string> = { copilot: () => join(homedir(), ".copilot"), codex: () => join(homedir(), ".codex"), gemini: () => join(homedir(), ".gemini"), - goose: () => join(homedir(), ".config", "agents"), + goose: () => join(homedir(), ".config", "goose"), opencode: () => join(homedir(), ".config", "opencode"), }; @@ -78,7 +78,7 @@ export interface ResolvedProvider { * 1. Explicit --provider flag * 2. MPAK_PROVIDER env var * 3. provider field in ~/.mpak/config.json - * 4. Auto-detect: exactly 1 → use it, 0 → default to claude, multiple → error + * 4. Auto-detect: exactly 1 → use it, 0 → default to claude, multiple → default to claude with warning */ export function resolveProvider(explicit?: string): ResolvedProvider { // 1. Explicit --provider flag @@ -130,8 +130,10 @@ export function resolveProvider(explicit?: string): ResolvedProvider { return { provider: "claude", skillsDir: getSkillsDir("claude") }; } - // Multiple providers detected — ambiguous - throw new Error( - `Multiple providers detected: ${detected.join(", ")}\nSet a default: mpak provider set \nOr specify: mpak skill install --provider `, + // Multiple providers detected — default to claude with a warning + process.stderr.write( + `Warning: Multiple providers detected (${detected.join(", ")}). Defaulting to claude.\n` + + `Run 'mpak provider set ' to set a default, or use --provider .\n`, ); + return { provider: "claude", skillsDir: getSkillsDir("claude") }; } From 6b736699fddc2f76c9671e05ed5a888e0bb82f0f Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Tue, 17 Mar 2026 10:29:04 -1000 Subject: [PATCH 3/3] fix(cli): align goose skills dir with detection dir Use ~/.config/goose/skills/ (goose-specific) instead of ~/.config/agents/skills/ (cross-agent portable) so the skills directory is consistent with the detection parent (~/.config/goose). --- packages/cli/src/utils/providers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/utils/providers.ts b/packages/cli/src/utils/providers.ts index baa8e4c..c99a263 100644 --- a/packages/cli/src/utils/providers.ts +++ b/packages/cli/src/utils/providers.ts @@ -12,7 +12,7 @@ const PROVIDERS = { copilot: () => join(homedir(), ".copilot", "skills"), codex: () => join(homedir(), ".codex", "skills"), gemini: () => join(homedir(), ".gemini", "skills"), - goose: () => join(homedir(), ".config", "agents", "skills"), + goose: () => join(homedir(), ".config", "goose", "skills"), opencode: () => join(homedir(), ".config", "opencode", "skills"), } as const;