diff --git a/src/auth/__tests__/logout.test.ts b/src/auth/__tests__/logout.test.ts index a2a4c5e..9320dc4 100644 --- a/src/auth/__tests__/logout.test.ts +++ b/src/auth/__tests__/logout.test.ts @@ -5,6 +5,11 @@ import { join } from 'node:path'; import { Command } from 'commander'; import { logoutCommand } from '../logout.js'; +const removeClaudeMcpMock = vi.hoisted(() => + vi.fn<() => { ok: boolean; detail?: string }>(() => ({ ok: true })), +); +vi.mock('../../commands/mcp.js', () => ({ removeClaudeMcp: removeClaudeMcpMock })); + // logout against the real file-backed store in a temp HOOKMYAPP_CONFIG_DIR // (same seam as agent-refresh.test.ts / the storage tests). @@ -13,6 +18,7 @@ const SAVED = process.env.HOOKMYAPP_CONFIG_DIR; let logSpy: ReturnType; beforeEach(() => { + removeClaudeMcpMock.mockReset().mockReturnValue({ ok: true }); DIR = mkdtempSync(join(tmpdir(), 'hma-logout-')); process.env.HOOKMYAPP_CONFIG_DIR = DIR; logSpy = vi.spyOn(console, 'log').mockReturnValue(undefined); @@ -60,11 +66,27 @@ describe('logout', () => { expect(existsSync(credsPath)).toBe(false); const written = stdoutSpy.mock.calls.map((c) => String(c[0])).join(''); - expect(JSON.parse(written.trim())).toEqual({ status: 'logged_out', revoked: false }); + expect(JSON.parse(written.trim())).toEqual({ + status: 'logged_out', + revoked: false, + mcpCleanup: { ok: true }, + }); // The human check line must NOT be printed in --json mode. expect(logSpy.mock.calls.flat().join('')).not.toMatch(/Logged out/); }); + test('reports MCP cleanup failure after credentials are removed', async () => { + removeClaudeMcpMock.mockReturnValue({ ok: false, detail: 'Claude MCP cleanup timed out' }); + const stdoutSpy = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + + await runLogout(['--json']); + + expect(JSON.parse(String(stdoutSpy.mock.calls[0][0]))).toMatchObject({ + status: 'logged_out_with_warning', + mcpCleanup: { ok: false, detail: 'Claude MCP cleanup timed out' }, + }); + }); + test('agent credential → self-revokes server-side before clearing local creds (AIT-153)', async () => { const credsPath = join(DIR, 'credentials.json'); writeFileSync( diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 0c502f9..eeb8db4 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -31,14 +31,22 @@ export function logoutCommand(program: Command): void { } await deleteCredentials(); - removeClaudeMcp(); + const mcpCleanup = removeClaudeMcp(); if (json) { process.stdout.write( - JSON.stringify({ status: 'logged_out', revoked }) + '\n', + JSON.stringify({ + status: mcpCleanup.ok ? 'logged_out' : 'logged_out_with_warning', + revoked, + mcpCleanup, + }) + '\n', ); } else { - console.log('\n✓ Logged out\n'); + console.log( + mcpCleanup.ok + ? '\n✓ Logged out\n' + : `\n✓ Logged out\n⚠ ${mcpCleanup.detail}\n`, + ); } }); diff --git a/src/commands/__tests__/mcp.test.ts b/src/commands/__tests__/mcp.test.ts index 115e994..70dbd98 100644 --- a/src/commands/__tests__/mcp.test.ts +++ b/src/commands/__tests__/mcp.test.ts @@ -1,5 +1,7 @@ import { beforeEach, describe, expect, test, vi } from 'vitest'; import { spawnSync } from 'node:child_process'; +import { Command } from 'commander'; +import { resolve } from 'node:path'; import { getValidAccessToken } from '../../api/client.js'; vi.mock('node:child_process', () => ({ spawnSync: vi.fn() })); @@ -8,7 +10,14 @@ vi.mock('../../config/env-profiles.js', () => ({ getEffectiveApiUrl: () => 'https://api.hookmyapp.com', })); -import { installClaudeMcp, maybeInstallClaudeMcp, printMcpHeaders, removeClaudeMcp } from '../mcp.js'; +import { + installClaudeMcp, + maybeInstallClaudeMcp, + printMcpHeaders, + registerMcpCommand, + removeClaudeMcp, + shellQuote, +} from '../mcp.js'; describe('MCP setup', () => { beforeEach(() => vi.clearAllMocks()); @@ -38,7 +47,7 @@ describe('MCP setup', () => { JSON.stringify({ type: 'http', url: 'https://api.hookmyapp.com/mcp', - headersHelper: 'hookmyapp mcp-headers', + headersHelper: `${shellQuote(process.execPath)} ${shellQuote(resolve(process.argv[1]))} mcp-headers`, }), ]); expect(JSON.stringify(args)).not.toContain('Bearer'); @@ -55,6 +64,13 @@ describe('MCP setup', () => { expect(spawnSync).toHaveBeenCalledOnce(); }); + test('shell-quotes helper paths without expanding metacharacters', () => { + expect(shellQuote('/tmp/$(`unsafe`)/it\'s', 'linux')).toBe(`'/tmp/$(\`unsafe\`)/it'"'"'s'`); + expect(shellQuote('C:\\Program Files\\nodejs\\node.exe', 'win32')).toBe( + '"C:\\Program Files\\nodejs\\node.exe"', + ); + }); + test('replaces an existing Claude entry', () => { vi.mocked(spawnSync) .mockReturnValueOnce({ status: 1, stderr: 'already exists' } as never) @@ -74,6 +90,44 @@ describe('MCP setup', () => { expect(spawnSync).toHaveBeenCalledWith('claude', ['mcp', 'remove', '--scope', 'user', 'hookmyapp'], { encoding: 'utf8', + timeout: 10_000, }); }); + + test('treats missing Claude as successful cleanup', () => { + vi.mocked(spawnSync).mockReturnValue({ + status: null, + error: Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), + } as never); + + expect(removeClaudeMcp(true)).toEqual({ ok: true }); + }); + + test('treats an absent MCP entry as successful cleanup', () => { + vi.mocked(spawnSync).mockReturnValue({ status: 1, stderr: 'MCP server hookmyapp not found' } as never); + + expect(removeClaudeMcp(true)).toEqual({ ok: true }); + }); + + test('reports a bounded Claude status timeout', async () => { + vi.mocked(spawnSync).mockReturnValue({ + status: null, + error: Object.assign(new Error('timed out'), { code: 'ETIMEDOUT' }), + } as never); + const { getClaudeMcpStatus } = await import('../mcp.js'); + + expect(getClaudeMcpStatus()).toEqual({ ok: false, detail: 'Claude MCP check timed out' }); + expect(vi.mocked(spawnSync).mock.calls[0][2]).toMatchObject({ timeout: 10_000 }); + }); + + test('emits JSON for mcp install in global JSON mode', async () => { + vi.mocked(spawnSync).mockReturnValue({ status: 0 } as never); + const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + const program = new Command().option('--json'); + registerMcpCommand(program); + + await program.parseAsync(['node', 'hookmyapp', '--json', 'mcp', 'install', '--agent', 'claude']); + + expect(JSON.parse(String(write.mock.calls.at(-1)?.[0]))).toEqual({ status: 'configured', agent: 'claude' }); + }); }); diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index 18a07c7..c794d86 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -1,10 +1,25 @@ import { spawnSync } from 'node:child_process'; +import { resolve } from 'node:path'; import type { Command } from 'commander'; import { getEffectiveApiUrl } from '../config/env-profiles.js'; import { ConfigurationError } from '../output/error.js'; import { addExamples } from '../output/help.js'; const MCP_NAME = 'hookmyapp'; +const CLAUDE_OPTIONS = { encoding: 'utf8' as const, timeout: 10_000 }; + +function headersHelper(): string { + return `${shellQuote(process.execPath)} ${shellQuote(resolve(process.argv[1]))} mcp-headers`; +} + +export function shellQuote(value: string, platform = process.platform): string { + if (platform === 'win32') return `"${value}"`; + return `'${value.replace(/'/g, `'"'"'`)}'`; +} + +function timedOut(error: Error | undefined): boolean { + return (error as NodeJS.ErrnoException | undefined)?.code === 'ETIMEDOUT'; +} function mcpUrl(): string { return `${getEffectiveApiUrl().replace(/\/$/, '')}/mcp`; @@ -20,18 +35,21 @@ export function installClaudeMcp(): void { const config = JSON.stringify({ type: 'http', url: mcpUrl(), - headersHelper: 'hookmyapp mcp-headers', + headersHelper: headersHelper(), }); const args = ['mcp', 'add-json', '--scope', 'user', MCP_NAME, config]; - let result = spawnSync('claude', args, { encoding: 'utf8' }); + let result = spawnSync('claude', args, CLAUDE_OPTIONS); const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; if (result.status !== 0 && output.includes('already exists')) { - removeClaudeMcp(true); - result = spawnSync('claude', args, { encoding: 'utf8' }); + const cleanup = removeClaudeMcp(true); + if (!cleanup.ok) { + throw new ConfigurationError(cleanup.detail ?? 'Claude MCP cleanup failed', 'MCP_INSTALL_FAILED'); + } + result = spawnSync('claude', args, CLAUDE_OPTIONS); } if (result.error || result.status !== 0) { throw new ConfigurationError( - result.error?.message || result.stderr.trim() || 'Claude Code MCP setup failed', + result.error?.message || (result.stderr ?? '').trim() || 'Claude Code MCP setup failed', 'MCP_INSTALL_FAILED', ); } @@ -39,7 +57,7 @@ export function installClaudeMcp(): void { export function maybeInstallClaudeMcp(force = false): void { if (!force && process.env.NODE_ENV === 'test') return; - const probe = spawnSync('claude', ['--version'], { encoding: 'utf8' }); + const probe = spawnSync('claude', ['--version'], CLAUDE_OPTIONS); if (probe.error || probe.status !== 0) return; try { installClaudeMcp(); @@ -51,17 +69,26 @@ export function maybeInstallClaudeMcp(force = false): void { } } -export function removeClaudeMcp(force = false): void { - if (!force && process.env.NODE_ENV === 'test') return; - spawnSync('claude', ['mcp', 'remove', '--scope', 'user', MCP_NAME], { - encoding: 'utf8', - }); +export function removeClaudeMcp(force = false): { ok: boolean; detail?: string } { + if (!force && process.env.NODE_ENV === 'test') return { ok: true }; + const result = spawnSync('claude', ['mcp', 'remove', '--scope', 'user', MCP_NAME], CLAUDE_OPTIONS); + if ((result.error as NodeJS.ErrnoException | undefined)?.code === 'ENOENT') return { ok: true }; + if (!result.error && result.status === 0) return { ok: true }; + const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`.toLowerCase(); + if (output.includes('not found') || output.includes('does not exist') || output.includes('no mcp server')) { + return { ok: true }; + } + return { + ok: false, + detail: timedOut(result.error) + ? 'Claude MCP cleanup timed out' + : result.error?.message || (result.stderr ?? '').trim() || 'Claude MCP cleanup failed', + }; } export function getClaudeMcpStatus(): { ok: boolean; detail: string } { - const result = spawnSync('claude', ['mcp', 'get', MCP_NAME], { - encoding: 'utf8', - }); + const result = spawnSync('claude', ['mcp', 'get', MCP_NAME], CLAUDE_OPTIONS); + if (timedOut(result.error)) return { ok: false, detail: 'Claude MCP check timed out' }; if (result.error?.message.includes('ENOENT')) { return { ok: false, detail: 'Claude Code not found' }; } @@ -88,7 +115,11 @@ export function registerMcpCommand(program: Command): void { throw new ConfigurationError(`Unsupported agent "${opts.agent}". Supported: claude`, 'MCP_AGENT_UNSUPPORTED'); } installClaudeMcp(); - process.stdout.write('HookMyApp MCP configured for Claude Code.\n'); + process.stdout.write( + program.opts().json + ? JSON.stringify({ status: 'configured', agent: 'claude' }) + '\n' + : 'HookMyApp MCP configured for Claude Code.\n', + ); }); addExamples( install,