diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 734161c..e89d7e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-node@v4 with: node-version-file: '.node-version' @@ -28,8 +30,47 @@ jobs: - run: npm test - run: node bin/hookmyapp.js --version + # AIT-395: both Windows bugs a customer reported (bare `spawnSync` never + # resolving `.cmd` shims, and the libuv abort at teardown) shipped because + # every job here was ubuntu-only. Mocked unit tests with an injected platform + # cannot catch either — only actually running the CLI on Windows can. + test-windows: + runs-on: windows-latest + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + - uses: actions/setup-node@v4 + with: + node-version-file: '.node-version' + cache: 'npm' + - run: npm ci + - run: node build.mjs + - run: npm test + # Exercises the exit path itself: a clean 0, not the exit-9 abort. + - run: node bin/hookmyapp.js --version + - name: doctor sees the npm/npx shims and exits cleanly + shell: pwsh + run: | + $out = node bin/hookmyapp.js doctor --json 2>&1 | Out-String + $code = $LASTEXITCODE + Write-Host $out + # Parse first: a teardown abort produces no JSON at all, so this is + # what actually distinguishes "crashed" from "ran and reported". + try { $report = $out | ConvertFrom-Json } + catch { throw "doctor produced no JSON report (exit $code) — likely the libuv teardown abort" } + foreach ($id in @('npm', 'npx')) { + $check = $report.checks | Where-Object { $_.id -eq $id } + if (-not $check.ok) { throw "doctor could not see $id on Windows: $($check.detail)" } + } + # An unreachable API is a runner network blip, not a regression: only + # fail on a non-zero exit that the network check does not explain. + $network = $report.checks | Where-Object { $_.id -eq 'network' } + if ($code -ne 0 -and $network.ok) { throw "doctor exited $code with every hard check passing (9 = the libuv teardown abort)" } + if (-not $network.ok) { Write-Host "note: API unreachable from this runner; exit-code assertion skipped" } + notify-failure: - needs: [test] + needs: [test, test-windows] if: always() && github.event_name == 'push' && github.ref == 'refs/heads/main' && (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) runs-on: ubuntu-latest timeout-minutes: 2 diff --git a/CHANGELOG.md b/CHANGELOG.md index 125c282..28c2163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ All notable changes to `@gethookmyapp/cli` are documented here. +## 0.14.16 — 2026-08-14 + +### Fixed + +- Commands no longer crash on Windows after finishing their work. Every command that made a network call printed the right output and then died with an assertion from Node's event loop and exit code 9, which made successful runs look like failures and made exit codes unusable in scripts and CI (AIT-395). +- `hookmyapp doctor` no longer reports `npm` and `npx` as missing on Windows when both are installed and working, and no longer exits 1 on a healthy install. `hookmyapp mcp` had the same problem locating Claude Code on Windows and is fixed with it. npm and npx are now reported for information only: the CLI runs on Node alone and never calls them, so they cannot block setup (AIT-395). + ## 0.14.15 — 2026-08-13 ### Changed diff --git a/package-lock.json b/package-lock.json index 6b042b6..3487c70 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@gethookmyapp/cli", - "version": "0.14.15", + "version": "0.14.16", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@gethookmyapp/cli", - "version": "0.14.15", + "version": "0.14.16", "license": "MIT", "dependencies": { "@inquirer/prompts": "^7.0.0", diff --git a/package.json b/package.json index 1f2ce4c..4b50c3e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@gethookmyapp/cli", - "version": "0.14.15", + "version": "0.14.16", "description": "HookMyApp CLI, No BS. Just go live.", "type": "module", "bin": { diff --git a/src/__tests__/flush-exit.test.ts b/src/__tests__/flush-exit.test.ts index e26d698..a041404 100644 --- a/src/__tests__/flush-exit.test.ts +++ b/src/__tests__/flush-exit.test.ts @@ -50,11 +50,10 @@ describe('flushAndExit awaits PostHog AND Sentry in parallel', () => { it('calls posthog.shutdown(2000) when PostHog client is initialised', async () => { process.env.HOOKMYAPP_POSTHOG_TOKEN = 'phc_test'; await initPostHogLazy(); // initialise - const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { - throw new Error(`__test_exit_${code ?? 0}__`); - }) as never); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); try { - await expect(flushAndExit(0)).rejects.toThrow('__test_exit_0__'); + await flushAndExit(0); + expect(process.exitCode).toBe(0); expect(fakeShutdown).toHaveBeenCalledTimes(1); expect(fakeShutdown).toHaveBeenCalledWith(2000); } finally { @@ -66,12 +65,12 @@ describe('flushAndExit awaits PostHog AND Sentry in parallel', () => { process.env.HOOKMYAPP_POSTHOG_TOKEN = 'phc_test'; fakeShutdown.mockRejectedValueOnce(new Error('posthog drop')); await initPostHogLazy(); - const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { - throw new Error(`__test_exit_${code ?? 0}__`); - }) as never); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); try { - await expect(flushAndExit(2)).rejects.toThrow('__test_exit_2__'); - expect(exitSpy).toHaveBeenCalledWith(2); + await flushAndExit(2); + // AIT-395: status set, process left to drain — never a hard exit. + expect(process.exitCode).toBe(2); + expect(exitSpy).not.toHaveBeenCalled(); } finally { exitSpy.mockRestore(); } @@ -81,11 +80,9 @@ describe('flushAndExit awaits PostHog AND Sentry in parallel', () => { process.env.HOOKMYAPP_TELEMETRY = 'off'; process.env.HOOKMYAPP_POSTHOG_TOKEN = 'phc_test'; await initPostHogLazy(); // returns null — no client - const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { - throw new Error(`__test_exit_${code ?? 0}__`); - }) as never); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); try { - await expect(flushAndExit(0)).rejects.toThrow('__test_exit_0__'); + await flushAndExit(0); expect(fakeShutdown).not.toHaveBeenCalled(); } finally { exitSpy.mockRestore(); diff --git a/src/__tests__/notifications-nudge.test.ts b/src/__tests__/notifications-nudge.test.ts index 970989b..772b7dd 100644 --- a/src/__tests__/notifications-nudge.test.ts +++ b/src/__tests__/notifications-nudge.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { basename, join } from 'node:path'; vi.mock('node:child_process', () => ({ spawn: vi.fn(), @@ -256,7 +256,7 @@ describe('maybeNudge banner', () => { describe('cache namespacing', () => { it('readCache falls back to the pre-rename notices-nudge filename after an upgrade', () => { const file = cacheFilePath(getEffectiveApiUrl(), 'agc_legacy'); - const legacy = join(dir, file.split('/').pop()!.replace('notifications-nudge-', 'notices-nudge-')); + const legacy = join(dir, basename(file).replace('notifications-nudge-', 'notices-nudge-')); writeCacheAtomic(legacy, unreadCache); expect(readCache(file)?.hasUnread).toBe(true); // unread state survives the rename }); diff --git a/src/__tests__/sentry-init.test.ts b/src/__tests__/sentry-init.test.ts index 41b0f61..b29a6b3 100644 --- a/src/__tests__/sentry-init.test.ts +++ b/src/__tests__/sentry-init.test.ts @@ -190,27 +190,30 @@ describe('shouldCaptureToSentry filter — capture every non-null error', () => }); describe('flushAndExit', () => { - it('calls process.exit with the provided code when Sentry is not initialized (fast path)', async () => { - const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { - throw new Error(`__test_exit_${code ?? 0}__`); - }) as never); + // AIT-395: the contract changed from "hard exit" to "set the status and let + // the loop drain" — process.exit() on top of a closing libuv handle aborts + // the process on Windows (exit 9) after the command already succeeded. + it('sets the provided exit code when Sentry is not initialized (fast path)', async () => { + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); try { - await expect(flushAndExit(2)).rejects.toThrow('__test_exit_2__'); - expect(exitSpy).toHaveBeenCalledWith(2); + await flushAndExit(2); + expect(process.exitCode).toBe(2); + expect(exitSpy).not.toHaveBeenCalled(); } finally { exitSpy.mockRestore(); + process.exitCode = undefined; } }); - it('exits with 0 when passed 0', async () => { - const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { - throw new Error(`__test_exit_${code ?? 0}__`); - }) as never); + it('sets 0 when passed 0', async () => { + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); try { - await expect(flushAndExit(0)).rejects.toThrow('__test_exit_0__'); - expect(exitSpy).toHaveBeenCalledWith(0); + await flushAndExit(0); + expect(process.exitCode).toBe(0); + expect(exitSpy).not.toHaveBeenCalled(); } finally { exitSpy.mockRestore(); + process.exitCode = undefined; } }); }); diff --git a/src/commands/__tests__/billing.test.ts b/src/commands/__tests__/billing.test.ts index 94af3a8..ca6ecf8 100644 --- a/src/commands/__tests__/billing.test.ts +++ b/src/commands/__tests__/billing.test.ts @@ -476,8 +476,9 @@ describe('billingUpgrade — free tier plan prompt (GET /plans)', () => { await assertion; }); // advanceUntilSettled drives up to 500 real event-loop turns per poll test. - // On a loaded CI runner that can exceed the 5s default and time out even - // though nothing is wrong — it already timed out once on this branch. The - // block-level budget keeps these tests honest without making each one - // declare its own. -}, 30_000); + // On a loaded CI runner that can exceed the default and time out even + // though nothing is wrong. The block-level budget keeps these tests honest + // without making each one declare its own — and it OVERRIDES the config's + // testTimeout, so raising the budget means raising it here (AIT-395: 30s + // still wasn't enough on either runner). +}, 60_000); diff --git a/src/commands/__tests__/mcp.test.ts b/src/commands/__tests__/mcp.test.ts index 70dbd98..ff24417 100644 --- a/src/commands/__tests__/mcp.test.ts +++ b/src/commands/__tests__/mcp.test.ts @@ -1,10 +1,16 @@ import { beforeEach, describe, expect, test, vi } from 'vitest'; -import { spawnSync } from 'node:child_process'; +import { runTool } from '../../lib/spawn-tool.js'; import { Command } from 'commander'; import { resolve } from 'node:path'; import { getValidAccessToken } from '../../api/client.js'; -vi.mock('node:child_process', () => ({ spawnSync: vi.fn() })); +// Mock the tool-spawn seam, not node:child_process: `runTool` adds the +// cmd.exe /c prefix on Windows, so asserting raw runTool argv here is a +// platform-dependent test, not a contract test (AIT-395). +vi.mock('../../lib/spawn-tool.js', async (importOriginal) => ({ + ...(await importOriginal()), + runTool: vi.fn(), +})); vi.mock('../../api/client.js', () => ({ getValidAccessToken: vi.fn() })); vi.mock('../../config/env-profiles.js', () => ({ getEffectiveApiUrl: () => 'https://api.hookmyapp.com', @@ -33,11 +39,11 @@ describe('MCP setup', () => { }); test('installs a user-scoped Claude headersHelper without storing a token', () => { - vi.mocked(spawnSync).mockReturnValue({ status: 0 } as never); + vi.mocked(runTool).mockReturnValue({ status: 0 } as never); installClaudeMcp(); - const [, args] = vi.mocked(spawnSync).mock.calls[0]; + const [, args] = vi.mocked(runTool).mock.calls[0]; expect(args).toEqual([ 'mcp', 'add-json', @@ -54,14 +60,14 @@ describe('MCP setup', () => { }); test('skips automatic setup when Claude Code is absent', () => { - vi.mocked(spawnSync).mockReturnValueOnce({ + vi.mocked(runTool).mockReturnValueOnce({ status: null, error: Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), } as never); maybeInstallClaudeMcp(true); - expect(spawnSync).toHaveBeenCalledOnce(); + expect(runTool).toHaveBeenCalledOnce(); }); test('shell-quotes helper paths without expanding metacharacters', () => { @@ -72,30 +78,30 @@ describe('MCP setup', () => { }); test('replaces an existing Claude entry', () => { - vi.mocked(spawnSync) + vi.mocked(runTool) .mockReturnValueOnce({ status: 1, stderr: 'already exists' } as never) .mockReturnValueOnce({ status: 0 } as never) .mockReturnValueOnce({ status: 0 } as never); installClaudeMcp(); - expect(spawnSync).toHaveBeenCalledTimes(3); - expect(vi.mocked(spawnSync).mock.calls[1][1]).toEqual(['mcp', 'remove', '--scope', 'user', 'hookmyapp']); + expect(runTool).toHaveBeenCalledTimes(3); + expect(vi.mocked(runTool).mock.calls[1][1]).toEqual(['mcp', 'remove', '--scope', 'user', 'hookmyapp']); }); test('removes only the user-scoped HookMyApp entry', () => { - vi.mocked(spawnSync).mockReturnValue({ status: 0 } as never); + vi.mocked(runTool).mockReturnValue({ status: 0 } as never); removeClaudeMcp(true); - expect(spawnSync).toHaveBeenCalledWith('claude', ['mcp', 'remove', '--scope', 'user', 'hookmyapp'], { + expect(runTool).toHaveBeenCalledWith('claude', ['mcp', 'remove', '--scope', 'user', 'hookmyapp'], { encoding: 'utf8', timeout: 10_000, }); }); test('treats missing Claude as successful cleanup', () => { - vi.mocked(spawnSync).mockReturnValue({ + vi.mocked(runTool).mockReturnValue({ status: null, error: Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), } as never); @@ -104,24 +110,24 @@ describe('MCP setup', () => { }); test('treats an absent MCP entry as successful cleanup', () => { - vi.mocked(spawnSync).mockReturnValue({ status: 1, stderr: 'MCP server hookmyapp not found' } as never); + vi.mocked(runTool).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({ + vi.mocked(runTool).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 }); + expect(vi.mocked(runTool).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); + vi.mocked(runTool).mockReturnValue({ status: 0 } as never); const write = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); const program = new Command().option('--json'); registerMcpCommand(program); diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 3d26a95..0cd848f 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,5 +1,5 @@ import type { Command } from 'commander'; -import { spawnSync } from 'node:child_process'; +import { runTool } from '../lib/spawn-tool.js'; import { readCredentials } from '../auth/store.js'; import { apiClient } from '../api/client.js'; import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; @@ -18,7 +18,7 @@ function parseMajor(v: string): number { return Number(v.replace(/^v/, '').split function toolVersion(cmd: string): string | null { try { - const r = spawnSync(cmd, ['-v'], { encoding: 'utf8' }); + const r = runTool(cmd, ['-v'], { encoding: 'utf8' }); return r.status === 0 ? (r.stdout || '').trim() : null; } catch { return null; } } @@ -33,11 +33,13 @@ export async function collectDoctorReport( checks.push({ id: 'node', label: 'Node.js >= 20', ok: nodeOk, hard: true, detail: nodeOk ? nodeV : `${nodeV} — upgrade at https://nodejs.org` }); // npm/npx presence (best-effort; gated so unit tests stay hermetic). + // Reported, never a gate: the CLI never shells out to npm, so a probe that + // can't see it says nothing about whether the CLI works (AIT-395). if (opts.checkTools !== false) { const npm = toolVersion('npm'); - checks.push({ id: 'npm', label: 'npm', ok: npm !== null, hard: true, detail: npm ?? 'not found on PATH' }); + checks.push({ id: 'npm', label: 'npm', ok: npm !== null, hard: false, detail: npm ?? 'not found on PATH' }); const npx = toolVersion('npx'); - checks.push({ id: 'npx', label: 'npx', ok: npx !== null, hard: true, detail: npx ?? 'not found on PATH' }); + checks.push({ id: 'npx', label: 'npx', ok: npx !== null, hard: false, detail: npx ?? 'not found on PATH' }); const mcp = getClaudeMcpStatus(); checks.push({ id: 'mcp', label: 'HookMyApp MCP (Claude)', ok: mcp.ok, hard: false, detail: mcp.detail }); } diff --git a/src/commands/mcp.ts b/src/commands/mcp.ts index c794d86..b23aec9 100644 --- a/src/commands/mcp.ts +++ b/src/commands/mcp.ts @@ -1,9 +1,9 @@ -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'; +import { isCommandNotFound, runTool } from '../lib/spawn-tool.js'; const MCP_NAME = 'hookmyapp'; const CLAUDE_OPTIONS = { encoding: 'utf8' as const, timeout: 10_000 }; @@ -38,14 +38,14 @@ export function installClaudeMcp(): void { headersHelper: headersHelper(), }); const args = ['mcp', 'add-json', '--scope', 'user', MCP_NAME, config]; - let result = spawnSync('claude', args, CLAUDE_OPTIONS); + let result = runTool('claude', args, CLAUDE_OPTIONS); const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; if (result.status !== 0 && output.includes('already exists')) { 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); + result = runTool('claude', args, CLAUDE_OPTIONS); } if (result.error || result.status !== 0) { throw new ConfigurationError( @@ -57,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'], CLAUDE_OPTIONS); + const probe = runTool('claude', ['--version'], CLAUDE_OPTIONS); if (probe.error || probe.status !== 0) return; try { installClaudeMcp(); @@ -71,8 +71,9 @@ export function maybeInstallClaudeMcp(force = false): void { 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 }; + const result = runTool('claude', ['mcp', 'remove', '--scope', 'user', MCP_NAME], CLAUDE_OPTIONS); + // Claude Code isn't installed — nothing to clean up, not a failure. + if (isCommandNotFound(result)) 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')) { @@ -87,9 +88,9 @@ export function removeClaudeMcp(force = false): { ok: boolean; detail?: string } } export function getClaudeMcpStatus(): { ok: boolean; detail: string } { - const result = spawnSync('claude', ['mcp', 'get', MCP_NAME], CLAUDE_OPTIONS); + const result = runTool('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')) { + if (isCommandNotFound(result)) { return { ok: false, detail: 'Claude Code not found' }; } const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; diff --git a/src/index.ts b/src/index.ts index 2e00c4c..51e4ff9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -309,7 +309,13 @@ async function main(): Promise { process.stderr.write('\n' + err.stack + '\n'); } } else if (err instanceof CommanderError) { - if (err.exitCode === 0) await flushAndExit(0); // --help, --version + // --help, --version. `flushAndExit` no longer terminates the process + // (AIT-395), so this path must return instead of falling through into + // the parse-error handling below. + if (err.exitCode === 0) { + await flushAndExit(0); + return; + } // Emit cli_parse_error for non-zero parse failures BEFORE the // emitCommandInvoked early-return swallows the signal (invokedCommand // is null on parse failures because the action handler never ran). diff --git a/src/lib/__tests__/spawn-tool.test.ts b/src/lib/__tests__/spawn-tool.test.ts new file mode 100644 index 0000000..6861e7c --- /dev/null +++ b/src/lib/__tests__/spawn-tool.test.ts @@ -0,0 +1,89 @@ +// AIT-395 — Windows can't resolve `.cmd` shims from a bare command name. +// The customer's own probe, which this helper is built against: +// spawnSync('claude', ['--version']) → ENOENT (no PATHEXT expansion) +// spawnSync('claude.cmd', ['--version']) → EINVAL (.cmd needs cmd.exe) +// shell: true → concatenates, mangling JSON args +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { spawnSync } from 'node:child_process'; + +vi.mock('node:child_process', () => ({ spawnSync: vi.fn(() => ({ status: 0, stdout: '', stderr: '' })) })); + +import { isCommandNotFound, runTool } from '../spawn-tool.js'; + +const OPTIONS = { encoding: 'utf8' as const, timeout: 10_000 }; + +describe('runTool', () => { + const originalComSpec = process.env.ComSpec; + beforeEach(() => vi.clearAllMocks()); + // These cases set and delete ComSpec; the rest of the worker must not + // inherit whichever one ran last. + afterEach(() => { + if (originalComSpec === undefined) delete process.env.ComSpec; + else process.env.ComSpec = originalComSpec; + }); + + it('spawns the command directly on posix', () => { + runTool('claude', ['--version'], OPTIONS, 'darwin'); + expect(spawnSync).toHaveBeenCalledWith('claude', ['--version'], OPTIONS); + }); + + it('routes through cmd.exe /c on win32 so .cmd shims resolve', () => { + process.env.ComSpec = 'C:\\WINDOWS\\system32\\cmd.exe'; + runTool('claude', ['--version'], OPTIONS, 'win32'); + expect(spawnSync).toHaveBeenCalledWith( + 'C:\\WINDOWS\\system32\\cmd.exe', + ['/c', 'claude', '--version'], + OPTIONS, + ); + }); + + it('falls back to cmd.exe when ComSpec is unset', () => { + delete process.env.ComSpec; + runTool('npm', ['-v'], OPTIONS, 'win32'); + expect(spawnSync).toHaveBeenCalledWith('cmd.exe', ['/c', 'npm', '-v'], OPTIONS); + }); + + it('keeps a JSON argument as ONE argv entry (shell:true would concatenate it)', () => { + const json = JSON.stringify({ type: 'http', url: 'https://api.hookmyapp.com/mcp' }); + runTool('claude', ['mcp', 'add-json', '--scope', 'user', 'hookmyapp', json], OPTIONS, 'win32'); + const [, args, options] = vi.mocked(spawnSync).mock.calls[0]; + expect(args).toEqual(['/c', 'claude', 'mcp', 'add-json', '--scope', 'user', 'hookmyapp', json]); + // shell:true is what mangles the payload — it must never be set. + expect(options).not.toHaveProperty('shell'); + }); +}); + +describe('isCommandNotFound', () => { + it('detects the posix ENOENT shape', () => { + const err = Object.assign(new Error('spawnSync claude ENOENT'), { code: 'ENOENT' }); + expect(isCommandNotFound({ error: err } as never)).toBe(true); + }); + + it('detects cmd.exe reporting a missing command (status 1, no error object)', () => { + expect( + isCommandNotFound({ + status: 1, + stderr: "'claude' is not recognized as an internal or external command,\r\n", + } as never), + ).toBe(true); + }); + + it('detects a missing command on a non-English Windows via status 9009', () => { + // cmd.exe translates its diagnostic but not its exit status, so a German + // or Japanese Windows must not be told "Claude Code is installed". + expect( + isCommandNotFound({ + status: 9009, + stderr: '"claude" ist entweder falsch geschrieben oder konnte nicht gefunden werden.\r\n', + } as never), + ).toBe(true); + }); + + it('is false for a command that ran and failed on its own terms', () => { + expect(isCommandNotFound({ status: 1, stderr: 'No MCP server found with name: hookmyapp' } as never)).toBe(false); + }); + + it('is false for a successful run', () => { + expect(isCommandNotFound({ status: 0, stdout: '2.1.231' } as never)).toBe(false); + }); +}); diff --git a/src/lib/spawn-tool.ts b/src/lib/spawn-tool.ts new file mode 100644 index 0000000..be7ec69 --- /dev/null +++ b/src/lib/spawn-tool.ts @@ -0,0 +1,41 @@ +// One place where the CLI shells out to another tool (AIT-395). +// +// Windows never resolves a bare command name to its `.cmd` shim: `spawnSync` +// does no PATHEXT expansion, so `spawnSync('claude', …)` is ENOENT even with +// claude on PATH, and naming the shim directly is EINVAL (`.cmd` needs an +// interpreter). `shell: true` is NOT the fix — it concatenates argv into one +// command line instead of escaping it, which mangles the JSON payload +// `claude mcp add-json` expects. Spawning cmd.exe with `/c` and separate args +// keeps every argument intact and lets cmd.exe do the shim lookup. +// +// ponytail: no caret-escaping of cmd.exe metacharacters (& | < > ^). Our +// arguments are our own API URL + a JSON config; if a caller ever passes +// user-controlled text with those characters, escape here. +import { spawnSync, type SpawnSyncOptionsWithStringEncoding, type SpawnSyncReturns } from 'node:child_process'; + +export function runTool( + command: string, + args: string[], + options: SpawnSyncOptionsWithStringEncoding, + platform: NodeJS.Platform = process.platform, +): SpawnSyncReturns { + if (platform !== 'win32') return spawnSync(command, args, options); + return spawnSync(process.env.ComSpec || 'cmd.exe', ['/c', command, ...args], options); +} + +/** + * "The tool isn't installed" — ENOENT on posix, where the spawn itself fails, + * OR cmd.exe's own complaint on Windows, where the spawn SUCCEEDS (cmd.exe + * runs fine) and the missing command only shows up in its output. + */ +export function isCommandNotFound(result: SpawnSyncReturns): boolean { + if ((result.error as NodeJS.ErrnoException | undefined)?.code === 'ENOENT') return true; + if (result.error?.message.includes('ENOENT')) return true; + if (result.status === 0) return false; + // 9009 is cmd.exe's own "command not found" status, and unlike its message + // it is the same on a German or Japanese Windows. Check it first; the + // English text stays as a fallback for shells that don't set the status. + if (result.status === 9009) return true; + const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; + return output.includes('is not recognized as an internal or external command'); +} diff --git a/src/observability/__tests__/flush-and-exit.test.ts b/src/observability/__tests__/flush-and-exit.test.ts new file mode 100644 index 0000000..020fd3a --- /dev/null +++ b/src/observability/__tests__/flush-and-exit.test.ts @@ -0,0 +1,44 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { flushAndExit } from '../sentry.js'; + +// AIT-395: `flushAndExit` used to call `process.exit()` outright. On Windows +// that aborts the process while libuv handles are still closing — the +// `src\win\async.c` assertion + exit code 9 a customer hit on EVERY networked +// command, after the command had already printed the right output. +describe('flushAndExit teardown', () => { + // Fake timers so the unref'd watchdog that flushAndExit schedules cannot + // outlive the test, fire against the by-then-restored real process.exit, + // and take the vitest worker down with it. + beforeEach(() => vi.useFakeTimers()); + + afterEach(() => { + vi.clearAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); + process.exitCode = undefined; + }); + + it('sets the exit code instead of killing the process', async () => { + const exit = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + + await flushAndExit(3); + + expect(process.exitCode).toBe(3); + expect(exit).not.toHaveBeenCalled(); + }); + + it('leaves no timer holding the loop open', async () => { + const exit = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + const unref = vi.fn(); + const timer = vi.spyOn(globalThis, 'setTimeout').mockReturnValue({ unref } as never); + + await flushAndExit(0); + + // EVERY timer this path schedules must be unref'd — the force-exit + // watchdog and the agent-close race alike. A single ref'd timer would keep + // the process alive for its full window on every command. + expect(timer.mock.calls.length).toBeGreaterThan(0); + expect(unref).toHaveBeenCalledTimes(timer.mock.calls.length); + expect(exit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/observability/sentry.ts b/src/observability/sentry.ts index 5b0605d..f128814 100644 --- a/src/observability/sentry.ts +++ b/src/observability/sentry.ts @@ -307,7 +307,7 @@ export async function captureError(err: unknown): Promise { * is initialized this is essentially a no-op + immediate exit (zero added * latency on the happy path for telemetry-off users). */ -export async function flushAndExit(exitCode: number): Promise { +export async function flushAndExit(exitCode: number): Promise { await Promise.allSettled([ (async (): Promise => { if (initialized && sentryModule) { @@ -320,11 +320,53 @@ export async function flushAndExit(exitCode: number): Promise { })(), shutdownPostHog(2000), ]); - process.exit(exitCode); - // Unreachable after process.exit; satisfies the `Promise` return type - // for TS without a raw `throw new Error` (AppError discipline — see the - // monorepo CLAUDE.md rule the CLI mirrors). - return undefined as never; + await closeHttpAgent(); + + // Do NOT `process.exit()` here. On Windows that aborts the process the + // moment any libuv handle is mid-close — `Assertion failed: + // !(handle->flags & UV_HANDLE_CLOSING), file src\win\async.c` with exit + // code 9, AFTER the command has already printed its correct output + // (AIT-395). Setting `exitCode` lets the loop drain and exit on its own + // terms with the same status. + process.exitCode = exitCode; + + // Safety net for a handle we failed to close: an unref'd timer cannot keep + // the process alive, so this only ever fires when something else is still + // holding the loop open — the situation where the old hard exit was the + // only way out anyway. + const bail = setTimeout(() => process.exit(exitCode), EXIT_DRAIN_MS); + bail.unref(); +} + +const EXIT_DRAIN_MS = 2000; +const CLOSE_AGENT_MS = 1000; + +/** + * Close fetch's keep-alive sockets. Node parks them in a global undici + * dispatcher that outlives the request, so without this the loop still holds + * live handles at exit — the thing that makes networked commands (and only + * networked commands) crash on Windows. + */ +async function closeHttpAgent(): Promise { + try { + const dispatcher = (globalThis as Record)[ + Symbol.for('undici.globalDispatcher.1') + ] as { close?: () => Promise; destroy?: () => Promise } | undefined; + if (!dispatcher?.close) return; + // `close()` drains gracefully — it waits for in-flight requests and takes + // no timeout, so one hung fire-and-forget request would hold the CLI open + // forever. Give it a second, then cut the sockets outright. + let closed = false; + await Promise.race([ + dispatcher.close().then(() => { + closed = true; + }), + new Promise((r) => setTimeout(r, CLOSE_AGENT_MS).unref()), + ]); + if (!closed) await dispatcher.destroy?.(); + } catch { + // Best-effort — a teardown detail must never change the exit status. + } } // Test-only helpers — allow the sentry-init + telemetry-consent specs to diff --git a/src/storage/__tests__/eperm-actionable.test.ts b/src/storage/__tests__/eperm-actionable.test.ts index 13d23a2..9cc422b 100644 --- a/src/storage/__tests__/eperm-actionable.test.ts +++ b/src/storage/__tests__/eperm-actionable.test.ts @@ -5,7 +5,9 @@ import { join } from 'node:path'; import { setPersistedTelemetry } from '../../observability/telemetry.js'; import { ConfigWriteForbiddenError } from '../errors.js'; -describe('config writers translate EPERM to ConfigWriteForbiddenError', () => { +// POSIX-only: `chmodSync(dir, 0o500)` does not make a directory read-only on +// Windows, so the EPERM this suite exists to trigger never happens there. +describe.skipIf(process.platform === 'win32')('config writers translate EPERM to ConfigWriteForbiddenError', () => { let dir: string; let originalConfigDir: string | undefined; diff --git a/src/storage/__tests__/secrets.test.ts b/src/storage/__tests__/secrets.test.ts index 574f0ab..7e4ae3b 100644 --- a/src/storage/__tests__/secrets.test.ts +++ b/src/storage/__tests__/secrets.test.ts @@ -31,7 +31,10 @@ describe('secrets (file-only storage)', () => { expect(await readSecrets()).toEqual(FIXTURE); }); - it('writes the file with mode 0o600', async () => { + // POSIX-only: Windows has no mode bits, chmod is a no-op there and the file + // is protected by the per-user profile ACL instead. Asserting 0o600 on + // Windows tests nothing about the OS. + it.skipIf(process.platform === 'win32')('writes the file with mode 0o600', async () => { await writeSecrets(FIXTURE); const mode = statSync(join(dir, 'credentials.json')).mode & 0o777; expect(mode).toBe(0o600); diff --git a/vitest.config.ts b/vitest.config.ts index 5a43374..61692d4 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -7,6 +7,11 @@ export default defineConfig({ // test module loads — prevents tests from clobbering the developer's // real ~/.hookmyapp credentials + active workspace config. setupFiles: ['./vitest.setup.ts'], + // billing.test.ts's fake-timer poll tests step timers up to 500 times, and + // under full-suite load on a CI runner that blows the 30s default — seen + // on both windows-latest and ubuntu-latest. Runner speed, not a product + // bug (AIT-395). + testTimeout: 60_000, coverage: { include: ['src/**'], },