diff --git a/src/cli/src/commands/skills.ts b/src/cli/src/commands/skills.ts index 8a68667..627799d 100644 --- a/src/cli/src/commands/skills.ts +++ b/src/cli/src/commands/skills.ts @@ -26,6 +26,10 @@ import { removeManagedHooks, type HooksByEvent, } from '../utils/hooks-materialiser.js'; +import { + attachSkillMcpServers, + parseMcpServerDeps, +} from '../utils/mcpservers-materialiser.js'; import { ApiError } from '../api-client.js'; /** @@ -70,8 +74,7 @@ interface SyncedSkillMetadata { postInstall?: unknown; postInstallTimeoutSec?: unknown; hooks?: unknown; - // mcpServers auto-attach lives in a follow-up — straightforward - // wrapper around the existing `mcpctl create serverattachment` path. + mcpServers?: unknown; } export interface SyncOpts { @@ -99,6 +102,7 @@ export interface SyncResult { postInstallsRan: string[]; // skills whose postInstall executed in this sync postInstallsSkipped: string[]; // skills with postInstall but unchanged hash → no rerun hooksApplied: string[]; // skills whose hooks were registered/updated in ~/.claude/settings.json + mcpServersAttached: string[]; // ":" tuples that landed in this sync errors: Array<{ skill: string; error: string }>; exitCode: 0 | 1 | 2; } @@ -125,6 +129,7 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise 0 || result.installed.length > 0 || result.updated.length > 0 || result.removed.length > 0 || result.postInstallsRan.length > 0) { + const anythingHappened = + result.errors.length > 0 || + result.installed.length > 0 || + result.updated.length > 0 || + result.removed.length > 0 || + result.postInstallsRan.length > 0 || + result.hooksApplied.length > 0 || + result.mcpServersAttached.length > 0; + if (!opts.quiet || anythingHappened) { const parts: string[] = []; if (result.installed.length) parts.push(`${String(result.installed.length)} installed`); if (result.updated.length) parts.push(`${String(result.updated.length)} updated`); @@ -250,11 +263,13 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise 0 && projectName) { + try { + const att = await attachSkillMcpServers(client, projectName, mcpServerDeps, warn); + for (const srv of att.attached) { + result.mcpServersAttached.push(`${v.name}:${srv}`); + } + for (const e of att.errors) { + result.errors.push({ + skill: v.name, + error: `mcpServers attach '${e.server}': ${e.error}`, + }); + } + } catch (err: unknown) { + warn(`mcpctl: failed to attach mcpServers for skill '${v.name}': ${err instanceof Error ? err.message : String(err)}`); + } + } else if (mcpServerDeps.length > 0) { + warn(`mcpctl: skill '${v.name}' declares mcpServers but sync is running global-only; skipping attach`); + } + // ── postInstall: run metadata.postInstall when present ── // Hash-pinned: only execute when the script's sha256 differs from // what state recorded. Failures DO NOT update the recorded hash so diff --git a/src/cli/src/utils/mcpservers-materialiser.ts b/src/cli/src/utils/mcpservers-materialiser.ts new file mode 100644 index 0000000..657c80e --- /dev/null +++ b/src/cli/src/utils/mcpservers-materialiser.ts @@ -0,0 +1,176 @@ +/** + * Auto-attach the MCP server dependencies a skill declares to the + * project that's syncing. Per the corporate-appliance trust model, + * publishing a skill that says "this project depends on my-grafana" + * is enough — the client takes mcpd at its word and asks mcpd to + * attach the server to the project. + * + * What this function does NOT do (deliberately): + * - Auto-create the server from a template if it's missing. + * Provisioning infrastructure from a skill push is a separate + * decision that needs explicit operator consent. v1 just warns + * when the named server doesn't exist and skips that dep. + * - Detach servers that a skill removed from its mcpServers list. + * Detach is destructive (the project loses access) and the + * `attach` itself is idempotent on the server side, so we err + * on the side of leaving things attached. PR-7 can revisit if + * a use case shows up. + * + * The mcpServers field is per-project: a skill's declared deps only + * get attached to the project the sync is running for. Global skills + * (no projectName context) skip this step entirely — there's no + * project to attach to. + */ +import type { ApiClient } from '../api-client.js'; +import { ApiError } from '../api-client.js'; + +export interface McpServerDep { + name: string; + fromTemplate?: string; + project?: string; +} + +export interface AttachResult { + attached: string[]; + alreadyAttached: string[]; + missing: string[]; + errors: Array<{ server: string; error: string }>; +} + +/** + * Resolve project name → id, list its currently-attached servers, + * then attach each declared dep that isn't already there. Idempotent + * by virtue of the existing-attachment check. + * + * Failures per-server are collected, not thrown — sync continues. + */ +export async function attachSkillMcpServers( + client: ApiClient, + projectName: string, + deps: McpServerDep[], + warn: (msg: string) => void = () => {}, +): Promise { + const result: AttachResult = { + attached: [], + alreadyAttached: [], + missing: [], + errors: [], + }; + if (deps.length === 0) return result; + + // Resolve project → id (the attach endpoint is keyed by id, not name). + let projectId: string; + try { + const projects = await client.get>('/api/v1/projects'); + const match = projects.find((p) => p.name === projectName); + if (!match) { + // No project to attach to — surface every dep as an error so the + // operator can see something is mis-configured. + for (const dep of deps) { + result.errors.push({ server: dep.name, error: `Project '${projectName}' not found` }); + } + return result; + } + projectId = match.id; + } catch (err: unknown) { + for (const dep of deps) { + result.errors.push({ + server: dep.name, + error: `Failed to resolve project: ${err instanceof Error ? err.message : String(err)}`, + }); + } + return result; + } + + // Inspect current attachments. The /api/v1/projects/:id/servers POST + // endpoint is idempotent server-side, but we still pre-check so we + // can report alreadyAttached vs newly-attached cleanly. + let attached = new Set(); + try { + const project = await client.get<{ servers?: Array<{ server?: { name: string } }> }>(`/api/v1/projects/${projectId}`); + attached = new Set( + (project.servers ?? []) + .map((s) => s.server?.name) + .filter((n): n is string => typeof n === 'string'), + ); + } catch (err: unknown) { + warn(`mcpctl: failed to read current attachments for project '${projectName}': ${err instanceof Error ? err.message : String(err)}`); + // Fall through with an empty set — we'll attempt attaches and let + // server-side idempotency cover any duplicates. + } + + // Optionally narrow the existing-server set so we can warn loudly on + // unknown server names. (Server attaches against a non-existent + // server would 404 anyway, but a clearer warning is friendlier.) + let existingServers = new Set(); + try { + const servers = await client.get>('/api/v1/servers'); + existingServers = new Set(servers.map((s) => s.name)); + } catch { + // Best-effort; if listing fails we still try the attach. + } + + for (const dep of deps) { + // Honour an explicit `project` on the dep — defensive, normally + // matches the active project anyway. Skip mismatches so a skill + // can declare deps for a different project without collateral + // damage during this sync. + if (dep.project && dep.project !== projectName) { + continue; + } + + if (attached.has(dep.name)) { + result.alreadyAttached.push(dep.name); + continue; + } + + if (existingServers.size > 0 && !existingServers.has(dep.name)) { + // Server doesn't exist on mcpd. v1 doesn't auto-create; warn and continue. + const detail = dep.fromTemplate + ? ` (skill suggests creating it via template '${dep.fromTemplate}')` + : ''; + warn(`mcpctl: skill mcpServers dep '${dep.name}' not found on mcpd${detail}; skipping attach`); + result.missing.push(dep.name); + continue; + } + + try { + await client.post(`/api/v1/projects/${projectId}/servers`, { server: dep.name }); + result.attached.push(dep.name); + } catch (err: unknown) { + // Idempotency: 409 (already attached) is success. + if (err instanceof ApiError && err.status === 409) { + result.alreadyAttached.push(dep.name); + continue; + } + // 404 means either the project or the server vanished mid-sync. + if (err instanceof ApiError && err.status === 404) { + result.missing.push(dep.name); + continue; + } + result.errors.push({ + server: dep.name, + error: err instanceof Error ? err.message : String(err), + }); + } + } + + return result; +} + +/** Type-narrow the metadata.mcpServers field. Tolerant of garbage. */ +export function parseMcpServerDeps(value: unknown): McpServerDep[] { + if (!Array.isArray(value)) return []; + const out: McpServerDep[] = []; + for (const v of value) { + if (v === null || typeof v !== 'object') continue; + const obj = v as Record; + const name = obj['name']; + if (typeof name !== 'string' || name.length === 0) continue; + const dep: McpServerDep = { name }; + if (typeof obj['fromTemplate'] === 'string') dep.fromTemplate = obj['fromTemplate']; + if (typeof obj['project'] === 'string') dep.project = obj['project']; + out.push(dep); + } + return out; +} diff --git a/src/cli/tests/utils/mcpservers-materialiser.test.ts b/src/cli/tests/utils/mcpservers-materialiser.test.ts new file mode 100644 index 0000000..356b08c --- /dev/null +++ b/src/cli/tests/utils/mcpservers-materialiser.test.ts @@ -0,0 +1,227 @@ +import { describe, it, expect, vi } from 'vitest'; +import { attachSkillMcpServers, parseMcpServerDeps } from '../../src/utils/mcpservers-materialiser.js'; +import type { ApiClient } from '../../src/api-client.js'; +import { ApiError } from '../../src/api-client.js'; + +interface MockClient { + get: ReturnType; + post: ReturnType; + put: ReturnType; + delete: ReturnType; +} + +function makeClient(): MockClient { + return { + get: vi.fn(), + post: vi.fn(async () => ({})), + put: vi.fn(async () => ({})), + delete: vi.fn(async () => undefined), + }; +} + +function apiError(status: number, body = 'err'): ApiError { + return new ApiError(status, body); +} + +describe('mcpservers-materialiser', () => { + describe('parseMcpServerDeps', () => { + it('returns [] for non-arrays', () => { + expect(parseMcpServerDeps(null)).toEqual([]); + expect(parseMcpServerDeps('foo')).toEqual([]); + expect(parseMcpServerDeps({})).toEqual([]); + }); + + it('keeps valid entries and drops garbage', () => { + const out = parseMcpServerDeps([ + { name: 'good', fromTemplate: 't', project: 'p' }, + { name: '', fromTemplate: 't' }, // empty name → drop + { fromTemplate: 'no-name' }, // no name → drop + { name: 'bare' }, // valid, minimal + 'string', // not an object → drop + ]); + expect(out).toEqual([ + { name: 'good', fromTemplate: 't', project: 'p' }, + { name: 'bare' }, + ]); + }); + }); + + describe('attachSkillMcpServers', () => { + it('attaches a new server when not already present', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [] }; + if (path === '/api/v1/servers') return [{ name: 'my-grafana' }]; + throw new Error(`unexpected GET ${path}`); + }); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana', fromTemplate: 'grafana' }], + ); + + expect(result.attached).toEqual(['my-grafana']); + expect(result.alreadyAttached).toEqual([]); + expect(result.missing).toEqual([]); + expect(result.errors).toEqual([]); + expect(client.post).toHaveBeenCalledWith('/api/v1/projects/proj-1/servers', { server: 'my-grafana' }); + }); + + it('reports alreadyAttached without re-posting', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [{ server: { name: 'my-grafana' } }] }; + if (path === '/api/v1/servers') return [{ name: 'my-grafana' }]; + throw new Error(`unexpected GET ${path}`); + }); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana' }], + ); + + expect(result.alreadyAttached).toEqual(['my-grafana']); + expect(result.attached).toEqual([]); + expect(client.post).not.toHaveBeenCalled(); + }); + + it('warns + skips when server does not exist on mcpd', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [] }; + if (path === '/api/v1/servers') return [{ name: 'something-else' }]; + throw new Error(`unexpected GET ${path}`); + }); + + const warnings: string[] = []; + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana', fromTemplate: 'grafana' }], + (m) => warnings.push(m), + ); + + expect(result.missing).toEqual(['my-grafana']); + expect(result.attached).toEqual([]); + expect(client.post).not.toHaveBeenCalled(); + expect(warnings.some((w) => w.includes('my-grafana') && w.includes('grafana'))).toBe(true); + }); + + it('errors-out when the project does not exist', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return []; // no projects + throw new Error(`unexpected GET ${path}`); + }); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'no-such-project', + [{ name: 'my-grafana' }], + ); + + expect(result.errors).toHaveLength(1); + expect(result.errors[0]?.error).toContain('Project'); + expect(client.post).not.toHaveBeenCalled(); + }); + + it('treats 409 from POST as alreadyAttached (idempotent server-side)', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + // attachments listing fails — fall back to attempting + handling 409 + if (path === '/api/v1/projects/proj-1') throw apiError(500, 'flake'); + if (path === '/api/v1/servers') return [{ name: 'my-grafana' }]; + throw new Error(`unexpected GET ${path}`); + }); + client.post.mockRejectedValueOnce(apiError(409, 'already attached')); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana' }], + ); + + expect(result.alreadyAttached).toEqual(['my-grafana']); + expect(result.errors).toEqual([]); + }); + + it('treats 404 from POST as missing (server vanished mid-sync)', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [] }; + if (path === '/api/v1/servers') return [{ name: 'my-grafana' }]; // existed when we listed + throw new Error(`unexpected GET ${path}`); + }); + // …but vanished by the time we POSTed. + client.post.mockRejectedValueOnce(apiError(404, 'gone')); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana' }], + ); + + expect(result.missing).toEqual(['my-grafana']); + expect(result.errors).toEqual([]); + }); + + it('skips deps that target a different project', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [] }; + if (path === '/api/v1/servers') return [{ name: 'my-grafana' }]; + throw new Error(`unexpected GET ${path}`); + }); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'my-grafana', project: 'other-project' }], + ); + + expect(result.attached).toEqual([]); + expect(result.missing).toEqual([]); + expect(client.post).not.toHaveBeenCalled(); + }); + + it('continues past per-server errors', async () => { + const client = makeClient(); + client.get.mockImplementation(async (path: string) => { + if (path === '/api/v1/projects') return [{ id: 'proj-1', name: 'demo' }]; + if (path === '/api/v1/projects/proj-1') return { servers: [] }; + if (path === '/api/v1/servers') return [{ name: 'a' }, { name: 'b' }]; + throw new Error(`unexpected GET ${path}`); + }); + client.post.mockImplementation(async (path: string, body) => { + if ((body as { server: string }).server === 'a') throw apiError(500, 'boom'); + return {}; + }); + + const result = await attachSkillMcpServers( + client as unknown as ApiClient, + 'demo', + [{ name: 'a' }, { name: 'b' }], + ); + + expect(result.errors).toHaveLength(1); + expect(result.errors[0]?.server).toBe('a'); + expect(result.attached).toEqual(['b']); + }); + + it('returns empty on empty deps without making any calls', async () => { + const client = makeClient(); + const result = await attachSkillMcpServers(client as unknown as ApiClient, 'demo', []); + expect(result).toEqual({ attached: [], alreadyAttached: [], missing: [], errors: [] }); + expect(client.get).not.toHaveBeenCalled(); + expect(client.post).not.toHaveBeenCalled(); + }); + }); +});