From cda44b23a4efc2375e1324fa2da5ea3e5bcd172e Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:42:04 +0300 Subject: [PATCH 1/9] fix(cli): resolve Windows command shims so doctor stops failing a healthy install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spawnSync` does no PATHEXT expansion, so a bare `npm`/`npx`/`claude` is ENOENT on Windows even when the tool is installed, and naming the `.cmd` shim directly is EINVAL. doctor reported "not found on PATH" for npm and npx and exited 1 on a working install; `hookmyapp mcp` had the same bug in all five `claude` spawns. Route every external-tool spawn through `runTool()`, which goes via cmd.exe `/c` with separate args on Windows — `shell: true` would concatenate argv and mangle the JSON payload `claude mcp add-json` expects. Also demote npm/npx from hard checks to informational: the CLI never shells out to npm, so a probe that can't see it says nothing about whether the CLI works. A hard gate there blocked agents from completing onboarding on Windows. Reported by a customer on Node 24.14.0 / CLI 0.14.13 (AIT-395, sup_81). --- src/commands/doctor.ts | 10 ++-- src/commands/mcp.ts | 17 +++---- src/lib/__tests__/spawn-tool.test.ts | 71 ++++++++++++++++++++++++++++ src/lib/spawn-tool.ts | 37 +++++++++++++++ 4 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 src/lib/__tests__/spawn-tool.test.ts create mode 100644 src/lib/spawn-tool.ts 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/lib/__tests__/spawn-tool.test.ts b/src/lib/__tests__/spawn-tool.test.ts new file mode 100644 index 0000000..fe1067c --- /dev/null +++ b/src/lib/__tests__/spawn-tool.test.ts @@ -0,0 +1,71 @@ +// 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 } 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', () => { + beforeEach(() => vi.clearAllMocks()); + + 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('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..9a909ab --- /dev/null +++ b/src/lib/spawn-tool.ts @@ -0,0 +1,37 @@ +// 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; + const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; + return output.includes('is not recognized as an internal or external command'); +} From a8a9de10175ff2f0c1cbb50390a45f9d19179fb2 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:42:34 +0300 Subject: [PATCH 2/9] release: 0.14.16 --- CHANGELOG.md | 6 ++++++ package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 125c282..68fe94b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to `@gethookmyapp/cli` are documented here. +## 0.14.16 — 2026-08-14 + +### Fixed + +- `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": { From ec02e15b5c4b0329794123f4c5c8e87c2d9f96c1 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:49:34 +0300 Subject: [PATCH 3/9] fix(cli): stop aborting at teardown on Windows, and run CI there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flushAndExit` called `process.exit()` the moment the telemetry drain resolved. On Windows that aborts the process while libuv handles are still closing: Assertion failed: !(handle->flags & UV_HANDLE_CLOSING), file src\win\async.c Exit code 9, AFTER the command printed its correct output — reported on login, notifications and workspace list, which makes exit codes unusable for scripting and makes a successful login look like a failure. Set `process.exitCode` and let the loop drain instead, and close fetch's keep-alive sockets (Node parks them in a global undici dispatcher that outlives the request) so there is nothing left holding it. An unref'd watchdog still force-exits if some handle we don't own keeps the process alive, which is the only case the old hard exit was buying us. Measured on the built CLI: 24ms between last output and exit. `flushAndExit` no longer terminates, so the --help/--version path in main() returns explicitly instead of falling through to parse-error handling. Add a windows-latest CI job that builds, runs the suite, and asserts `doctor --json` sees npm/npx and exits 0. Both Windows bugs in AIT-395 shipped because every job was ubuntu-only; mocked tests with an injected platform cannot catch either. Refs AIT-395, sup_81. --- .github/workflows/ci.yml | 32 +++++++++++++- src/__tests__/flush-exit.test.ts | 23 +++++----- src/__tests__/sentry-init.test.ts | 27 ++++++------ src/index.ts | 8 +++- .../__tests__/flush-and-exit.test.ts | 36 ++++++++++++++++ src/observability/sentry.ts | 42 ++++++++++++++++--- 6 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 src/observability/__tests__/flush-and-exit.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 734161c..8a2d101 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,8 +28,38 @@ 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 + - 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 + if ($code -ne 0) { throw "doctor exited $code (9 = the libuv teardown abort)" } + $report = $out | ConvertFrom-Json + 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)" } + } + 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/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__/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/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/observability/__tests__/flush-and-exit.test.ts b/src/observability/__tests__/flush-and-exit.test.ts new file mode 100644 index 0000000..2739ce5 --- /dev/null +++ b/src/observability/__tests__/flush-and-exit.test.ts @@ -0,0 +1,36 @@ +import { afterEach, 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', () => { + afterEach(() => { + 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); + + // The force-exit fallback must be unref'd: a ref'd timer would itself keep + // the process alive for the full drain window on every single command. + expect(timer).toHaveBeenCalledOnce(); + expect(unref).toHaveBeenCalledOnce(); + expect(exit).not.toHaveBeenCalled(); + }); +}); diff --git a/src/observability/sentry.ts b/src/observability/sentry.ts index 5b0605d..47a519a 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,41 @@ 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; + +/** + * 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 } | undefined; + await dispatcher?.close?.(); + } catch { + // Best-effort — a teardown detail must never change the exit status. + } } // Test-only helpers — allow the sentry-init + telemetry-consent specs to From c319bfe40fbbb6b52d6edea0f10117c016473f90 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:49:52 +0300 Subject: [PATCH 4/9] docs(changelog): note the Windows teardown crash fix in 0.14.16 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68fe94b..28c2163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to `@gethookmyapp/cli` are documented here. ### 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 From 7e36fe7f976262bb8f688de08f1bdef7fb874f18 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:53:08 +0300 Subject: [PATCH 5/9] test: make the suite pass on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First run of the suite on windows-latest surfaced 7 failures, none of them Windows bugs in the CLI — the tests themselves assumed POSIX: - mcp.test.ts mocked node:child_process and asserted the raw spawnSync argv, which on Windows legitimately carries the cmd.exe /c prefix. Mock the `runTool` seam instead, so the assertions describe what mcp.ts asks to run rather than how the platform spells it. - secrets.test.ts asserted mode 0o600. Windows has no mode bits; the file is protected by the per-user profile ACL. POSIX-only. - eperm-actionable.test.ts builds a read-only dir with chmod, which is a no-op on Windows, so the EPERM it exists to trigger never happens. POSIX-only. - notifications-nudge.test.ts split a path on '/', which yields the whole absolute path on Windows and produced a doubled mkdir target. Use basename. - billing.test.ts's fake-timer poll loop needs more than the default 30s of real time on the Windows runner. Refs AIT-395. --- src/__tests__/notifications-nudge.test.ts | 4 +- src/commands/__tests__/billing.test.ts | 5 ++- src/commands/__tests__/mcp.test.ts | 38 +++++++++++-------- .../__tests__/eperm-actionable.test.ts | 4 +- src/storage/__tests__/secrets.test.ts | 5 ++- 5 files changed, 35 insertions(+), 21 deletions(-) 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/commands/__tests__/billing.test.ts b/src/commands/__tests__/billing.test.ts index 94af3a8..13c89f0 100644 --- a/src/commands/__tests__/billing.test.ts +++ b/src/commands/__tests__/billing.test.ts @@ -415,6 +415,9 @@ describe('billingUpgrade — free tier plan prompt (GET /plans)', () => { expect(paths).not.toContain('/organizations/org_abc12345/billing/checkout'); }); + // 60s budget, not the default 30s: each fake-timer step costs real + // event-loop turns and the Windows runner is slow enough to blow the + // default on this poll loop alone. test('When checkout opens, then upgrade polls the subscription and confirms once the plan flips', async () => { vi.useFakeTimers(); vi.mocked(apiClient) @@ -433,7 +436,7 @@ describe('billingUpgrade — free tier plan prompt (GET /plans)', () => { await run; expect(log.mock.calls.flat().join('\n')).toContain('Upgraded to Scale'); - }); + }, 60_000); test('When a poll tick hits a network blip (apiClient throws NetworkError), then it is swallowed and polling continues to success', async () => { // apiClient wraps a raw fetch failure in its own NetworkError before it 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/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); From 5af6496a47d66c6e80ca4e47d2797759ffc4b9fc Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 08:55:24 +0300 Subject: [PATCH 6/9] test: give the Windows runner a 60s test budget The fake-timer poll tests in billing.test.ts step timers in 100ms increments; the windows-latest runner is slow enough to blow the 30s default doing it. A runner speed difference, not a product bug. One config knob rather than a per-test timeout that the next poll test forgets to copy. Refs AIT-395. --- src/commands/__tests__/billing.test.ts | 5 +---- vitest.config.ts | 4 ++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/commands/__tests__/billing.test.ts b/src/commands/__tests__/billing.test.ts index 13c89f0..94af3a8 100644 --- a/src/commands/__tests__/billing.test.ts +++ b/src/commands/__tests__/billing.test.ts @@ -415,9 +415,6 @@ describe('billingUpgrade — free tier plan prompt (GET /plans)', () => { expect(paths).not.toContain('/organizations/org_abc12345/billing/checkout'); }); - // 60s budget, not the default 30s: each fake-timer step costs real - // event-loop turns and the Windows runner is slow enough to blow the - // default on this poll loop alone. test('When checkout opens, then upgrade polls the subscription and confirms once the plan flips', async () => { vi.useFakeTimers(); vi.mocked(apiClient) @@ -436,7 +433,7 @@ describe('billingUpgrade — free tier plan prompt (GET /plans)', () => { await run; expect(log.mock.calls.flat().join('\n')).toContain('Upgraded to Scale'); - }, 60_000); + }); test('When a poll tick hits a network blip (apiClient throws NetworkError), then it is swallowed and polling continues to success', async () => { // apiClient wraps a raw fetch failure in its own NetworkError before it diff --git a/vitest.config.ts b/vitest.config.ts index 5a43374..cbe76a6 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -7,6 +7,10 @@ export default defineConfig({ // test module loads — prevents tests from clobbering the developer's // real ~/.hookmyapp credentials + active workspace config. setupFiles: ['./vitest.setup.ts'], + // The Windows runner is slow enough that the fake-timer poll tests in + // billing.test.ts blow the 30s default while stepping timers — a runner + // speed difference, not a product bug (AIT-395). + testTimeout: process.platform === 'win32' ? 60_000 : 30_000, coverage: { include: ['src/**'], }, From e514e6c947b9f3dc019003c145801e61897317af Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 09:05:40 +0300 Subject: [PATCH 7/9] =?UTF-8?q?fix(cli):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?locale-safe=20not-found,=20bounded=20agent=20close,=20CI=20hard?= =?UTF-8?q?ening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From CodeRabbit on #59: - `isCommandNotFound` matched only cmd.exe's English diagnostic, so a German or Japanese Windows would be told Claude Code is installed when it isn't. Check status 9009 first — cmd.exe translates the message, not the status. - `dispatcher.close()` waits for in-flight requests and takes no timeout, so one hung fire-and-forget request would have held the CLI open forever where the old hard exit killed it. Race it against 1s, then `destroy()`. - The Windows doctor smoke failed on any runner network blip. Parse the JSON first (a teardown abort emits none — that is the real signal) and only enforce the exit code when the network check itself passed. - `persist-credentials: false` on both checkouts: every later step runs repo code and nothing here pushes. Tests: fake timers in the flushAndExit specs so the unref'd watchdog can't fire against a restored real `process.exit` and kill the worker; restore ComSpec after the spawn-tool cases; cover the 9009 path. Refs AIT-395. --- .github/workflows/ci.yml | 15 ++++++++++++-- src/lib/__tests__/spawn-tool.test.ts | 20 ++++++++++++++++++- src/lib/spawn-tool.ts | 4 ++++ .../__tests__/flush-and-exit.test.ts | 18 ++++++++++++----- src/observability/sentry.ts | 16 +++++++++++++-- 5 files changed, 63 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8a2d101..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' @@ -36,6 +38,8 @@ jobs: runs-on: windows-latest steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-node@v4 with: node-version-file: '.node-version' @@ -51,12 +55,19 @@ jobs: $out = node bin/hookmyapp.js doctor --json 2>&1 | Out-String $code = $LASTEXITCODE Write-Host $out - if ($code -ne 0) { throw "doctor exited $code (9 = the libuv teardown abort)" } - $report = $out | ConvertFrom-Json + # 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, test-windows] diff --git a/src/lib/__tests__/spawn-tool.test.ts b/src/lib/__tests__/spawn-tool.test.ts index fe1067c..6861e7c 100644 --- a/src/lib/__tests__/spawn-tool.test.ts +++ b/src/lib/__tests__/spawn-tool.test.ts @@ -3,7 +3,7 @@ // 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 } from 'vitest'; +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: '' })) })); @@ -13,7 +13,14 @@ 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'); @@ -61,6 +68,17 @@ describe('isCommandNotFound', () => { ).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); }); diff --git a/src/lib/spawn-tool.ts b/src/lib/spawn-tool.ts index 9a909ab..be7ec69 100644 --- a/src/lib/spawn-tool.ts +++ b/src/lib/spawn-tool.ts @@ -32,6 +32,10 @@ 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 index 2739ce5..020fd3a 100644 --- a/src/observability/__tests__/flush-and-exit.test.ts +++ b/src/observability/__tests__/flush-and-exit.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from 'vitest'; +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 @@ -6,7 +6,14 @@ import { flushAndExit } from '../sentry.js'; // `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; }); @@ -27,10 +34,11 @@ describe('flushAndExit teardown', () => { await flushAndExit(0); - // The force-exit fallback must be unref'd: a ref'd timer would itself keep - // the process alive for the full drain window on every single command. - expect(timer).toHaveBeenCalledOnce(); - expect(unref).toHaveBeenCalledOnce(); + // 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 47a519a..f128814 100644 --- a/src/observability/sentry.ts +++ b/src/observability/sentry.ts @@ -339,6 +339,7 @@ export async function flushAndExit(exitCode: number): Promise { } const EXIT_DRAIN_MS = 2000; +const CLOSE_AGENT_MS = 1000; /** * Close fetch's keep-alive sockets. Node parks them in a global undici @@ -350,8 +351,19 @@ async function closeHttpAgent(): Promise { try { const dispatcher = (globalThis as Record)[ Symbol.for('undici.globalDispatcher.1') - ] as { close?: () => Promise } | undefined; - await dispatcher?.close?.(); + ] 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. } From ffe292cdbd502ee2a97af18137a647b7de79ca23 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 09:07:32 +0300 Subject: [PATCH 8/9] test: raise the test budget to 60s on every runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit billing.test.ts's fake-timer poll tests blew the 30s default on ubuntu too, not just Windows — they step timers up to 500 times and CI load decides whether that fits. One budget for both runners. Refs AIT-395. --- vitest.config.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/vitest.config.ts b/vitest.config.ts index cbe76a6..61692d4 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -7,10 +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'], - // The Windows runner is slow enough that the fake-timer poll tests in - // billing.test.ts blow the 30s default while stepping timers — a runner - // speed difference, not a product bug (AIT-395). - testTimeout: process.platform === 'win32' ? 60_000 : 30_000, + // 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/**'], }, From 879c3a1b8905e87868db39a6a3ead2bb6cdd02b6 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Fri, 14 Aug 2026 09:12:43 +0300 Subject: [PATCH 9/9] test: raise the billing suite's own 30s budget, which overrode the config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The describe block in billing.test.ts closes with its own `}, 30_000)`, and a suite-level budget wins over vitest.config.ts — so the previous commit's 60s config never applied to the tests that were timing out. Raise it where it is actually read. Refs AIT-395. --- src/commands/__tests__/billing.test.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) 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);