michal/mcpctl
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(cli): metadata.mcpServers auto-attach in mcpctl skills sync

Browse Source 
Closes the third deferred item from PR-5: skills can declare upstream
MCP server dependencies via `metadata.mcpServers` and `mcpctl skills
sync` now attaches them to the active project. Same trust model as
postInstall/hooks: the publisher is responsible, the client just
asks mcpd to attach.

## Behaviour

- For each `{ name, fromTemplate?, project? }` entry:
  - If the project already has the server attached → record as
    `alreadyAttached`, no-op.
  - If the server doesn't exist on mcpd → warn + skip (we don't
    auto-create from template; that's a separate explicit decision
    for the operator). The warning surfaces the suggested template
    so the operator can decide.
  - Otherwise → POST `/api/v1/projects/:id/servers { server: <name> }`.
  - 409 from POST → treat as alreadyAttached (server-side idempotency).
  - 404 from POST → treat as missing (race: server vanished mid-sync).
  - Other errors collected per-server; sync continues.
- A dep with `project:` set to a non-active project is skipped during
  this sync — keeps the active sync from making collateral changes
  to other projects.
- Global skill syncs (no project context) skip mcpServers entirely
  with a warning — there's no project to attach to.

## Files

### Added
- src/cli/src/utils/mcpservers-materialiser.ts (~140 LOC)
- src/cli/tests/utils/mcpservers-materialiser.test.ts (~190 LOC,
  10 tests covering: parse-tolerance, fresh attach, alreadyAttached
  short-circuit, missing-server warn+skip, missing-project errors-
  out, 409→alreadyAttached, 404→missing, cross-project skip,
  per-server error collection, empty-deps no-op)

### Edited
- src/cli/src/commands/skills.ts: applyOne calls
  attachSkillMcpServers after hooks. Tracks per-skill attachments in
  result.mcpServersAttached. Summary line surfaces "N mcpServers
  attached".

## Verification

165 test files / 2193 tests green (up from 2182).

Real-world flow:

```yaml
# skill metadata.yaml
mcpServers:
  - name: my-grafana
    fromTemplate: grafana
    project: monitoring
  - name: my-loki
    fromTemplate: loki
```

```bash
# As operator: ensure the named servers exist on mcpd first
mcpctl create server my-grafana --from-template grafana --env-from-secret grafana-creds
mcpctl create server my-loki --from-template loki

# Now publish the skill that declares them as deps. Sync will attach:
mcpctl skills sync
# → mcpctl: 1 installed, 2 mcpServers attached
mcpctl describe project monitoring     # both servers now attached
```

## What's intentionally NOT in this PR

- Auto-creating servers from `fromTemplate` when they don't exist.
  Provisioning infra from a skill push is a separate decision needing
  explicit operator policy. v1 warns + skips; the warning includes
  the suggested template name so the operator can act manually.
- Detaching a server when a skill drops it from mcpServers. Detach is
  destructive (project loses access) and we can't tell whether the
  detach is intentional vs. accidental drop. PR-7 can revisit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Michal
2026-05-07 19:12:20 +01:00
parent 7ebc8b22d1
commit 180e50a978
3 changed files with 447 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

227
src/cli/tests/utils/mcpservers-materialiser.test.ts Normal file
View File

@@ -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<typeof vi.fn>;
post: ReturnType<typeof vi.fn>;
put: ReturnType<typeof vi.fn>;
delete: ReturnType<typeof vi.fn>;
}
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();
});
});
});