fix(cli): close third review — token collision, migration, ownership

Round 2 fixed the first review but introduced regressions of its own, all of which only bite against state written by the previously installed build. `config prime-agent`: - Mint each credential under a unique `prime-agent-<stamp>` name again. `McpToken` is unique on (name, projectId) and revoke is a soft delete, so round 2's fixed `prime-agent` name could only ever be minted once per project — and the revoke-first ordering destroyed the working credential before discovering the mint would fail. - Provision the credential BEFORE touching settings.json. Registering the new project unmounts the previously active one, so a failed mint must not be able to leave prime-agent with no working project at all. The command now aborts with settings.json untouched. - Retire only the token this auth.json actually held, once its replacement is stored. Sweeping every `prime-agent*` token for the project would revoke the credential another install (or a custom --output run) is using; anything else that looks orphaned is reported, not deleted. - Validate a pre-existing credential instead of trusting its presence: a revoked or expired token used to short-circuit provisioning and leave prime-agent broken while the command reported success. Matched by tokenPrefix against the project's active tokens, so the secret is never sent. Fails open when the API can't be consulted. - Actually write auth.json 0600. `writeFile`'s mode is ignored for an existing file and prime-agent creates auth.json itself at 0644, so chmod after writing. - Recognise the untagged mcpServers entries older CLIs wrote (canonical proxy URL + an `mcp:<name>` mcpctl PAT in auth.json) so a switch unmounts them instead of leaving two gateways live. Hand-configured servers have no such credential and are still preserved. Same rule in the `/mcpctl` switcher's active-project lookup. - Add `--skip-marker`, and pass it from the `/mcpctl` switcher: the extension runs from whatever directory prime-agent was started in, and was silently re-scoping that repo's `.mcpctl-project`. `skills sync --agent prime-agent`: - Record ownership from the skill's own scope, not the syncing project's. Globals were being pinned to whichever project happened to sync them, after which every other project refused to update them forever. - Never adopt legacy, ownership-less state into the current scope. Round 2 did, which deleted the other project's skills on the first sync after upgrading. Such entries are attributed to the project that last wrote the state file, and left alone when that isn't the project syncing now. - Close the overwrite-guard bypass: a sync with no project, or a global landing on a project-owned name, could still clobber and re-own a tracked skill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB
2026-08-08 12:01:52 +01:00
parent fa7055ac5e
commit 170dc06496
9 changed files with 629 additions and 81 deletions
--- a/src/cli/tests/commands/prime-agent.test.ts
+++ b/src/cli/tests/commands/prime-agent.test.ts
@@ -1,5 +1,5 @@
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
-import { writeFileSync, readFileSync, mkdtempSync, rmSync, existsSync, statSync } from 'node:fs';
+import { writeFileSync, readFileSync, mkdtempSync, rmSync, existsSync, statSync, chmodSync } from 'node:fs';
 import { join } from 'node:path';
 import { tmpdir, homedir } from 'node:os';
 import { createConfigCommand } from '../../src/commands/config.js';
@@ -297,6 +297,231 @@ describe('config prime-agent', () => {
    expect(written.mcpServers['sre']).toBeDefined();               // untagged preserved
  });

+  it('adopts an untagged entry an older CLI wrote, keeping hand-configured ones', async () => {
+    // Written by a CLI that predates `mcpctlManaged`: an untagged entry whose
+    // URL is canonical AND a matching mcp:<project> PAT in auth.json.
+    const settingsPath = join(tmpDir, 'settings.json');
+    writeFileSync(settingsPath, JSON.stringify({
+      mcpServers: {
+        legacy: { type: 'http', url: `${DEFAULT_MCPCTL_GATEWAY_URL}/projects/legacy/mcp` },
+        websearch: { type: 'http', url: 'https://search.example/mcp' }, // hand-configured
+        sre: { type: 'http', url: `${DEFAULT_MCPCTL_GATEWAY_URL}/projects/sre/mcp` }, // canonical URL, no credential
+      },
+    }));
+    writeFileSync(join(tmpDir, 'auth.json'), JSON.stringify({
+      itaz: { type: 'api_key', key: 'sk-provider' },
+      'mcp:legacy': { type: 'api_key', key: 'mcpctl_pat_legacytoken1234' },
+    }));
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'labctl', '-o', settingsPath, '--skip-skills', '--skip-extension', '--token', 'mcpctl_pat_x'], { from: 'user' });
+
+    const written = JSON.parse(readFileSync(settingsPath, 'utf-8'));
+    expect(written.mcpServers['legacy']).toBeUndefined();  // adopted + unmounted
+    expect(written.mcpServers['websearch']).toBeDefined(); // unrelated, preserved
+    expect(written.mcpServers['sre']).toBeDefined();       // no PAT → hand-set, preserved
+    expect(written.mcpServers['labctl'].mcpctlManaged).toBe(true);
+  });
+
+  it('mints each credential under a unique name (never a fixed one)', async () => {
+    const settingsPath = join(tmpDir, 'settings.json');
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'p', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    const body = client.post.mock.calls.find((c) => c[0] === '/api/v1/mcptokens')?.[1] as { name: string };
+    // A fixed name can only ever be minted once: McpToken is unique on
+    // (name, projectId) and revoke is a soft delete.
+    expect(body.name).not.toBe('prime-agent');
+    expect(body.name).toMatch(/^prime-agent-[a-z0-9-]+$/);
+  });
+
+  it('revokes the token it replaced, only after the replacement is stored', async () => {
+    const settingsPath = join(tmpDir, 'settings.json');
+    const authPath = join(tmpDir, 'auth.json');
+    writeFileSync(authPath, JSON.stringify({
+      'mcp:p': { type: 'api_key', key: 'mcpctl_pat_oldtoken00000' },
+    }));
+    const order: string[] = [];
+    const api = {
+      get: vi.fn(async (url: string) => {
+        order.push(`get ${url}`);
+        return [
+          { id: 'tok-old', name: 'prime-agent-abc', status: 'active', tokenPrefix: 'mcpctl_pat_oldto' },
+          { id: 'tok-other', name: 'ci-runner', status: 'active', tokenPrefix: 'mcpctl_pat_ci000' },
+        ];
+      }),
+      post: vi.fn(async (url: string) => {
+        order.push(`post ${url}`);
+        return {};
+      }),
+      put: vi.fn(async () => ({})),
+      delete: vi.fn(async () => {}),
+    } as unknown as ApiClient;
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client: api, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    // Explicitly replace the stored credential.
+    await cmd.parseAsync(['prime-agent', '--project', 'p', '-o', settingsPath, '--skip-skills', '--skip-extension', '--token', 'mcpctl_pat_supplied00000'], { from: 'user' });
+
+    // The new credential landed on disk...
+    expect(JSON.parse(readFileSync(authPath, 'utf-8'))['mcp:p'].key).toBe('mcpctl_pat_supplied00000');
+    // ...before the token it replaced was revoked — never the other way round.
+    const revokeAt = order.indexOf('post /api/v1/mcptokens/tok-old/revoke');
+    expect(revokeAt).toBeGreaterThanOrEqual(0);
+    expect(statSync(authPath).mtimeMs).toBeGreaterThan(0);
+    // Tokens this auth.json never held are reported, never revoked.
+    expect(order).not.toContain('post /api/v1/mcptokens/tok-other/revoke');
+  });
+
+  it('never revokes a token this auth.json did not hold', async () => {
+    // A run against a custom --output (or a second machine) must not touch the
+    // credential the real install is using.
+    const settingsPath = join(tmpDir, 'settings.json');
+    const api = {
+      get: vi.fn(async () => [
+        { id: 'tok-elsewhere', name: 'prime-agent-abc', status: 'active', tokenPrefix: 'mcpctl_pat_elsew' },
+      ]),
+      post: vi.fn(async (url: string) => (url === '/api/v1/mcptokens' ? { token: 'mcpctl_pat_brandnew0000' } : {})),
+      put: vi.fn(async () => ({})),
+      delete: vi.fn(async () => {}),
+    } as unknown as ApiClient;
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client: api, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'p', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    const revokes = api.post.mock.calls.filter((c) => String(c[0]).includes('/revoke'));
+    expect(revokes).toEqual([]);
+    // ...but the user is told about it rather than left guessing.
+    expect(output.join('\n')).toContain('prime-agent-abc');
+  });
+
+  it('leaves settings.json untouched when the credential cannot be provisioned', async () => {
+    // The active project must keep working when a switch fails: registering the
+    // new project unmounts the old one, so it may not run before the mint.
+    const settingsPath = join(tmpDir, 'settings.json');
+    const before = JSON.stringify({
+      mcpServers: {
+        homeautomation: { type: 'http', url: `${DEFAULT_MCPCTL_GATEWAY_URL}/projects/homeautomation/mcp`, mcpctlManaged: true },
+      },
+    });
+    writeFileSync(settingsPath, before);
+    const badClient = { ...client, get: vi.fn(async () => []), post: vi.fn(async () => ({})) } as unknown as ApiClient;
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client: badClient, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'labctl', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    expect(process.exitCode).toBe(1);
+    expect(readFileSync(settingsPath, 'utf-8')).toBe(before);
+  });
+
+  it('re-mints when the stored credential is no longer active', async () => {
+    const settingsPath = join(tmpDir, 'settings.json');
+    writeFileSync(join(tmpDir, 'auth.json'), JSON.stringify({
+      'mcp:p': { type: 'api_key', key: 'mcpctl_pat_revoked000000' },
+    }));
+    const api = {
+      get: vi.fn(async () => [
+        { id: 'tok-1', name: 'prime-agent-old', status: 'revoked', tokenPrefix: 'mcpctl_pat_revo' },
+      ]),
+      post: vi.fn(async () => ({ token: 'mcpctl_pat_fresh0000000' })),
+      put: vi.fn(async () => ({})),
+      delete: vi.fn(async () => {}),
+    } as unknown as ApiClient;
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client: api, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'p', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    const auth = JSON.parse(readFileSync(join(tmpDir, 'auth.json'), 'utf-8'));
+    expect(auth['mcp:p'].key).toBe('mcpctl_pat_fresh0000000');
+    expect(process.exitCode).toBe(0);
+  });
+
+  it('keeps a stored credential that is still active', async () => {
+    const settingsPath = join(tmpDir, 'settings.json');
+    writeFileSync(join(tmpDir, 'auth.json'), JSON.stringify({
+      'mcp:p': { type: 'api_key', key: 'mcpctl_pat_liveaaaaaaaa' },
+    }));
+    const api = {
+      get: vi.fn(async () => [
+        // mcpd records the first 16 chars of the raw token as tokenPrefix.
+        { id: 'tok-1', name: 'prime-agent-x', status: 'active', tokenPrefix: 'mcpctl_pat_livea' },
+      ]),
+      post: vi.fn(async () => ({ token: 'should-not-be-minted' })),
+      put: vi.fn(async () => ({})),
+      delete: vi.fn(async () => {}),
+    } as unknown as ApiClient;
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client: api, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'p', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    expect(api.post).not.toHaveBeenCalled();
+    const auth = JSON.parse(readFileSync(join(tmpDir, 'auth.json'), 'utf-8'));
+    expect(auth['mcp:p'].key).toBe('mcpctl_pat_liveaaaaaaaa');
+  });
+
+  it('tightens a pre-existing 0644 auth.json to 0600', async () => {
+    // prime-agent creates auth.json itself with the default umask; writeFile's
+    // `mode` is ignored for an existing file, so the write must chmod.
+    const settingsPath = join(tmpDir, 'settings.json');
+    const authPath = join(tmpDir, 'auth.json');
+    writeFileSync(authPath, JSON.stringify({ itaz: { type: 'api_key', key: 'sk-x' } }), { mode: 0o644 });
+    chmodSync(authPath, 0o644);
+
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'm', '-o', settingsPath, '--skip-skills', '--skip-extension'], { from: 'user' });
+
+    expect(statSync(authPath).mode & 0o777).toBe(0o600);
+    // The provider credential is still there.
+    expect(JSON.parse(readFileSync(authPath, 'utf-8')).itaz.key).toBe('sk-x');
+  });
+
+  it('--skip-marker leaves the current directory alone', async () => {
+    // The /mcpctl switcher runs from whatever directory prime-agent started in.
+    const settingsPath = join(tmpDir, 'settings.json');
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'sre', '-o', settingsPath, '--skip-skills', '--skip-extension', '--skip-marker', '--token', 'mcpctl_pat_x'], { from: 'user' });
+
+    expect(exceptionSafeRead(join(tmpDir, '.mcpctl-project'))).toBeNull();
+  });
+
+  it('the installed switcher extension passes --skip-marker', async () => {
+    const settingsPath = join(tmpDir, 'settings.json');
+    const cmd = createConfigCommand(
+      { configDeps: { configDir: tmpDir }, log },
+      { client, credentialsDeps: { configDir: tmpDir }, log },
+    );
+    await cmd.parseAsync(['prime-agent', '--project', 'ha', '-o', settingsPath, '--skip-skills', '--token', 'mcpctl_pat_x'], { from: 'user' });
+
+    const ext = readFileSync(join(tmpDir, 'extensions', 'mcpctl-switch.ts'), 'utf-8');
+    expect(ext).toContain("'--skip-extension', '--skip-marker'");
+  });
+
  it('merges a re-configured project entry, preserving user-added fields', async () => {
    const settingsPath = join(tmpDir, 'settings.json');
    writeFileSync(settingsPath, JSON.stringify({
--- a/src/cli/tests/utils/prime-agent-skills.test.ts
+++ b/src/cli/tests/utils/prime-agent-skills.test.ts
@@ -207,6 +207,101 @@ describe('runPrimeAgentSkillsSync', () => {
    expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toBe('# version-a\n');
  });

+  it('does not delete legacy, ownership-less state belonging to another project', async () => {
+    // State written by a CLI that predates the `project` field: the skill has
+    // no recorded owner and the file records projA as the last syncing project.
+    const legacyDir = join(installRoot, 'legacy-skill');
+    mkdirSync(legacyDir, { recursive: true });
+    writeFileSync(join(legacyDir, 'SKILL.md'), '# legacy\n', 'utf-8');
+    writeFileSync(statePath, JSON.stringify({
+      schemaVersion: 1,
+      lastSync: '2026-01-01T00:00:00.000Z',
+      lastSyncProject: 'projA',
+      skills: {
+        'legacy-skill': {
+          id: 'l-1', semver: '1.0.0', contentHash: 'sha256:l', scope: 'project',
+          installDir: legacyDir, files: {}, postInstallHash: null,
+          lastSyncedAt: '2026-01-01T00:00:00.000Z',
+          // note: no `project` field
+        },
+      },
+    }), 'utf-8');
+
+    // First sync after upgrading, for a *different* project.
+    const client = mockClient({ visible: [], full: {} });
+    const result = await runPrimeAgentSkillsSync({ project: 'projB', installRoot, statePath }, deps(client));
+
+    expect(result.removed).toEqual([]);
+    expect(existsSync(legacyDir)).toBe(true);
+  });
+
+  it('cleans up legacy state once the owning project syncs again', async () => {
+    const legacyDir = join(installRoot, 'legacy-skill');
+    mkdirSync(legacyDir, { recursive: true });
+    writeFileSync(join(legacyDir, 'SKILL.md'), '# legacy\n', 'utf-8');
+    writeFileSync(statePath, JSON.stringify({
+      schemaVersion: 1,
+      lastSync: '2026-01-01T00:00:00.000Z',
+      lastSyncProject: 'projA',
+      skills: {
+        'legacy-skill': {
+          id: 'l-1', semver: '1.0.0', contentHash: 'sha256:l', scope: 'project',
+          installDir: legacyDir, files: {}, postInstallHash: null,
+          lastSyncedAt: '2026-01-01T00:00:00.000Z',
+        },
+      },
+    }), 'utf-8');
+
+    const client = mockClient({ visible: [], full: {} });
+    const result = await runPrimeAgentSkillsSync({ project: 'projA', installRoot, statePath }, deps(client));
+
+    expect(result.removed).toContain('legacy-skill');
+    expect(existsSync(legacyDir)).toBe(false);
+  });
+
+  it('keeps global skills updatable after switching projects', async () => {
+    // A global installed while projA was active must not be pinned to projA —
+    // globals are visible from every project.
+    const gv = (hash: string) => [
+      { id: 'g-1', name: 'shared-global', description: 'd', semver: '1.0.0', contentHash: hash, metadata: {}, scope: 'global' },
+    ];
+    const gf = (hash: string, body: string) => ({
+      'g-1': { id: 'g-1', name: 'shared-global', description: 'd', semver: '1.0.0', contentHash: hash, content: body, files: {} },
+    });
+
+    const clientA = mockClient({ visible: gv('sha256:v1'), full: gf('sha256:v1', '# v1\n') });
+    await runPrimeAgentSkillsSync({ project: 'projA', installRoot, statePath }, deps(clientA));
+    expect((await loadState(statePath)).skills['shared-global']?.project).toBeNull();
+
+    // Switch to projB; the global has been updated server-side.
+    const clientB = mockClient({ visible: gv('sha256:v2'), full: gf('sha256:v2', '# v2\n') });
+    const resultB = await runPrimeAgentSkillsSync({ project: 'projB', installRoot, statePath }, deps(clientB));
+
+    expect(resultB.updated).toContain('shared-global');
+    expect(resultB.preserved).toEqual([]);
+    expect(readFileSync(join(installRoot, 'shared-global', 'SKILL.md'), 'utf-8')).toBe('# v2\n');
+  });
+
+  it('does not let a global-only sync clobber a project-owned skill', async () => {
+    const av = [
+      { id: 'a-1', name: 'x-skill', description: 'd', semver: '1.0.0', contentHash: 'sha256:a', metadata: {}, scope: 'project' },
+    ];
+    const af = { 'a-1': { id: 'a-1', name: 'x-skill', description: 'd', semver: '1.0.0', contentHash: 'sha256:a', content: '# version-a\n', files: {} } };
+    await runPrimeAgentSkillsSync({ project: 'projA', installRoot, statePath }, deps(mockClient({ visible: av, full: af })));
+
+    // A global of the same name shows up on a global-only sync.
+    const gv = [
+      { id: 'g-9', name: 'x-skill', description: 'd', semver: '1.0.0', contentHash: 'sha256:g', metadata: {}, scope: 'global' },
+    ];
+    const gf = { 'g-9': { id: 'g-9', name: 'x-skill', description: 'd', semver: '1.0.0', contentHash: 'sha256:g', content: '# global\n', files: {} } };
+    const empty = join(tmpDir, 'empty3');
+    mkdirSync(empty, { recursive: true });
+    const result = await runPrimeAgentSkillsSync({ cwd: empty, installRoot, statePath }, deps(mockClient({ visible: gv, full: gf })));
+
+    expect(result.preserved).toContain('x-skill');
+    expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toBe('# version-a\n');
+  });
+
  it('removes global orphans on a global-only sync', async () => {
    // First sync a global skill.
    const v = [