AIT-239: Address MCP review feedback

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<typeof vi.spyOn>;
 
 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(

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`,
+        );
       }
     });
 

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' });
+  });
 });

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,26 +35,29 @@ 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',
     );
   }
 }
 
 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,