From eeda2458c7ed0047ff246ed6aa939912e1d2673d Mon Sep 17 00:00:00 2001 From: Stephen Dolan Date: Thu, 21 May 2026 09:30:00 -0400 Subject: [PATCH 1/5] fix: prevent stdout truncation when piped (#20) Calling process.exit() terminates the Node.js process before the asynchronous stdout pipe buffer drains, truncating output at ~512 bytes when piped through cat, tee, ssh, or any other pipeline. Setting process.exitCode and letting the event loop drain naturally is the documented Node.js solution. Affected every command's JSON output and every error response when run over SSH or in a shell pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/cli.ts | 4 +- src/lib/__tests__/errors.test.ts | 68 ++++++++++++++++++++++++++++++++ src/lib/errors.ts | 8 +++- 3 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 src/lib/__tests__/errors.test.ts diff --git a/src/cli.ts b/src/cli.ts index ebd2ba0..5f624a1 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -35,5 +35,7 @@ program.addCommand(createFolderCommand()); program.addCommand(createMcpCommand()); program.parseAsync().catch(() => { - process.exit(1); + // Set exitCode instead of calling process.exit() so any queued stdout + // writes finish before the process terminates. See errors.ts for details. + process.exitCode = 1; }); diff --git a/src/lib/__tests__/errors.test.ts b/src/lib/__tests__/errors.test.ts new file mode 100644 index 0000000..4a5bb5d --- /dev/null +++ b/src/lib/__tests__/errors.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { handleError, OmniFocusCliError } from '../errors.js'; + +describe('handleError', () => { + let logSpy: ReturnType; + let originalExitCode: typeof process.exitCode; + + beforeEach(() => { + originalExitCode = process.exitCode; + process.exitCode = undefined; + logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); + }); + + afterEach(() => { + logSpy.mockRestore(); + process.exitCode = originalExitCode; + }); + + function lastLoggedJson(): unknown { + const lastCall = logSpy.mock.calls.at(-1); + if (!lastCall) throw new Error('console.log was not called'); + return JSON.parse(String(lastCall[0])); + } + + it('sets process.exitCode to 1 without calling process.exit', () => { + handleError(new Error('boom')); + expect(process.exitCode).toBe(1); + }); + + it('returns normally so the event loop can drain stdout', () => { + // If handleError still called process.exit(), this test would never + // complete normally. Reaching the assertion proves the function + // returns synchronously. + expect(() => handleError(new Error('boom'))).not.toThrow(); + }); + + it('serializes OmniFocusCliError with its statusCode', () => { + handleError(new OmniFocusCliError('bad request', 400)); + expect(lastLoggedJson()).toEqual({ + error: { name: 'cli_error', detail: 'bad request', statusCode: 400 }, + }); + }); + + it('maps "not found" errors to 404', () => { + handleError(new Error('Task not found')); + expect(lastLoggedJson()).toEqual({ + error: { name: 'omnifocus_error', detail: 'Task not found', statusCode: 404 }, + }); + }); + + it('maps "Multiple" errors to 400', () => { + handleError(new Error('Multiple matches for "foo"')); + expect(lastLoggedJson()).toEqual({ + error: { + name: 'omnifocus_error', + detail: 'Multiple matches for "foo"', + statusCode: 400, + }, + }); + }); + + it('falls back to unknown_error for non-Error values', () => { + handleError('something weird'); + expect(lastLoggedJson()).toEqual({ + error: { name: 'unknown_error', detail: 'An unknown error occurred', statusCode: 500 }, + }); + }); +}); diff --git a/src/lib/errors.ts b/src/lib/errors.ts index eb5c099..3c52a91 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -10,7 +10,7 @@ export class OmniFocusCliError extends Error { } } -export function handleError(error: unknown): never { +export function handleError(error: unknown): void { let name = 'unknown_error'; let detail = 'An unknown error occurred'; let statusCode = 500; @@ -31,5 +31,9 @@ export function handleError(error: unknown): never { } outputJson({ error: { name, detail, statusCode } }); - process.exit(1); + // Set exitCode instead of calling process.exit() to let the stdout pipe + // buffer drain before the process terminates. Calling process.exit() + // truncates piped output at ~512 bytes on macOS because pipe writes are + // asynchronous. See https://nodejs.org/api/process.html#processexitcode + process.exitCode = 1; } From 2088df43b8bf03f6ad5db3b11090f077fb41c260 Mon Sep 17 00:00:00 2001 From: Stephen Dolan Date: Thu, 21 May 2026 09:31:55 -0400 Subject: [PATCH 2/5] test: clear process.exitCode after each handleError test Bun's native test runner (used in CI via `bun test`) reads process.exitCode at suite end to decide whether the run succeeded. The handleError tests deliberately set process.exitCode = 1, so any test that didn't clean up properly would fail the whole CI run even though every assertion passed. Always reset process.exitCode to undefined in afterEach instead of trying to restore a captured value, which could itself be stale. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/__tests__/errors.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/__tests__/errors.test.ts b/src/lib/__tests__/errors.test.ts index 4a5bb5d..376be5c 100644 --- a/src/lib/__tests__/errors.test.ts +++ b/src/lib/__tests__/errors.test.ts @@ -3,17 +3,18 @@ import { handleError, OmniFocusCliError } from '../errors.js'; describe('handleError', () => { let logSpy: ReturnType; - let originalExitCode: typeof process.exitCode; beforeEach(() => { - originalExitCode = process.exitCode; process.exitCode = undefined; logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); }); afterEach(() => { logSpy.mockRestore(); - process.exitCode = originalExitCode; + // Always clear exitCode after each test. handleError sets it to 1 and + // we must not leak that into the test runner's own exit status — which + // bun's native test runner reads to decide if the suite passed. + process.exitCode = undefined; }); function lastLoggedJson(): unknown { From 3f32020b52d2b54853e41743525b3d57832ba1d7 Mon Sep 17 00:00:00 2001 From: Stephen Dolan Date: Thu, 21 May 2026 09:34:01 -0400 Subject: [PATCH 3/5] test: isolate process.exitCode side effects inside each test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vitest's afterEach hook didn't reliably reset process.exitCode under bun's native test runner on ubuntu CI — the suite would pass every assertion but bun would still report exit 1 because handleError had set the global exitCode and the hook either ran too late or not at all. Move the save/spy/restore lifecycle into a synchronous try/finally inside a helper so the cleanup runs as part of the test body, with no dependency on the runner's lifecycle hooks. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/__tests__/errors.test.ts | 66 ++++++++++++++++---------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/lib/__tests__/errors.test.ts b/src/lib/__tests__/errors.test.ts index 376be5c..86bc851 100644 --- a/src/lib/__tests__/errors.test.ts +++ b/src/lib/__tests__/errors.test.ts @@ -1,57 +1,57 @@ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { handleError, OmniFocusCliError } from '../errors.js'; -describe('handleError', () => { - let logSpy: ReturnType; - - beforeEach(() => { - process.exitCode = undefined; - logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); - }); - - afterEach(() => { - logSpy.mockRestore(); - // Always clear exitCode after each test. handleError sets it to 1 and - // we must not leak that into the test runner's own exit status — which - // bun's native test runner reads to decide if the suite passed. - process.exitCode = undefined; - }); - - function lastLoggedJson(): unknown { +/** + * Calls handleError while isolating the side effects: captures the JSON + * written to stdout, captures the resulting process.exitCode, and restores + * both before returning. This keeps the test runner's own exit status clean + * (bun's native test runner reads process.exitCode at suite end). + */ +function runHandleError(error: unknown): { logged: unknown; exitCode: unknown } { + const previousExitCode = process.exitCode; + process.exitCode = undefined; + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); + try { + handleError(error); + const capturedExit = process.exitCode; const lastCall = logSpy.mock.calls.at(-1); - if (!lastCall) throw new Error('console.log was not called'); - return JSON.parse(String(lastCall[0])); + const logged = lastCall ? JSON.parse(String(lastCall[0])) : undefined; + return { logged, exitCode: capturedExit }; + } finally { + logSpy.mockRestore(); + process.exitCode = previousExitCode; } +} +describe('handleError', () => { it('sets process.exitCode to 1 without calling process.exit', () => { - handleError(new Error('boom')); - expect(process.exitCode).toBe(1); + const { exitCode } = runHandleError(new Error('boom')); + expect(exitCode).toBe(1); }); it('returns normally so the event loop can drain stdout', () => { // If handleError still called process.exit(), this test would never - // complete normally. Reaching the assertion proves the function - // returns synchronously. - expect(() => handleError(new Error('boom'))).not.toThrow(); + // complete normally. Reaching the assertion proves the function returns. + expect(() => runHandleError(new Error('boom'))).not.toThrow(); }); it('serializes OmniFocusCliError with its statusCode', () => { - handleError(new OmniFocusCliError('bad request', 400)); - expect(lastLoggedJson()).toEqual({ + const { logged } = runHandleError(new OmniFocusCliError('bad request', 400)); + expect(logged).toEqual({ error: { name: 'cli_error', detail: 'bad request', statusCode: 400 }, }); }); it('maps "not found" errors to 404', () => { - handleError(new Error('Task not found')); - expect(lastLoggedJson()).toEqual({ + const { logged } = runHandleError(new Error('Task not found')); + expect(logged).toEqual({ error: { name: 'omnifocus_error', detail: 'Task not found', statusCode: 404 }, }); }); it('maps "Multiple" errors to 400', () => { - handleError(new Error('Multiple matches for "foo"')); - expect(lastLoggedJson()).toEqual({ + const { logged } = runHandleError(new Error('Multiple matches for "foo"')); + expect(logged).toEqual({ error: { name: 'omnifocus_error', detail: 'Multiple matches for "foo"', @@ -61,8 +61,8 @@ describe('handleError', () => { }); it('falls back to unknown_error for non-Error values', () => { - handleError('something weird'); - expect(lastLoggedJson()).toEqual({ + const { logged } = runHandleError('something weird'); + expect(logged).toEqual({ error: { name: 'unknown_error', detail: 'An unknown error occurred', statusCode: 500 }, }); }); From 2b7f89bcb3c98c58ca62ab6b73bb6f6a8a2eaea6 Mon Sep 17 00:00:00 2001 From: Stephen Dolan Date: Thu, 21 May 2026 09:36:06 -0400 Subject: [PATCH 4/5] test: run handleError in subprocess to verify drain end-to-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bun's native test runner snapshots any process.exitCode mutation during a test and uses it as the suite's final exit code, even when the test restores it before returning. handleError deliberately sets process.exitCode = 1, so in-process testing was breaking CI. Move the test to a child process. Side benefit: this is a faithful end-to-end test of issue #20 — it pipes the child's stdout, parses the full JSON body, and asserts the exit code, mirroring exactly how downstream consumers like SSH/MCP wrappers consume the CLI. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/__tests__/errors.test.ts | 127 ++++++++++++++++++++++--------- 1 file changed, 92 insertions(+), 35 deletions(-) diff --git a/src/lib/__tests__/errors.test.ts b/src/lib/__tests__/errors.test.ts index 86bc851..df4a49f 100644 --- a/src/lib/__tests__/errors.test.ts +++ b/src/lib/__tests__/errors.test.ts @@ -1,57 +1,114 @@ -import { describe, it, expect, vi } from 'vitest'; -import { handleError, OmniFocusCliError } from '../errors.js'; +import { describe, it, expect, beforeAll } from 'vitest'; +import { execFile } from 'child_process'; +import { promisify } from 'util'; +import { mkdtemp, writeFile, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; /** - * Calls handleError while isolating the side effects: captures the JSON - * written to stdout, captures the resulting process.exitCode, and restores - * both before returning. This keeps the test runner's own exit status clean - * (bun's native test runner reads process.exitCode at suite end). + * These tests run handleError in a child process. The function deliberately + * sets process.exitCode = 1 to fix issue #20 (stdout truncation when piped), + * so we can't call it inside the test runner — bun's native test runner + * treats any process.exitCode mutation as a suite failure, even when reverted. + * + * Spawning a child process also gives us a faithful end-to-end test: we can + * pipe the child's stdout and confirm that the JSON error body arrives + * intact (not truncated) and that the exit code is correctly propagated. */ -function runHandleError(error: unknown): { logged: unknown; exitCode: unknown } { - const previousExitCode = process.exitCode; - process.exitCode = undefined; - const logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined); + +const execFileAsync = promisify(execFile); + +interface HandleErrorResult { + stdout: string; + exitCode: number | null; +} + +async function runHandleError( + scenario: 'omnifocus_cli_error' | 'error_not_found' | 'error_multiple' | 'error_plain' | 'non_error' +): Promise { + const tmp = await mkdtemp(join(tmpdir(), 'of-errors-test-')); + const script = join(tmp, 'run.mjs'); try { - handleError(error); - const capturedExit = process.exitCode; - const lastCall = logSpy.mock.calls.at(-1); - const logged = lastCall ? JSON.parse(String(lastCall[0])) : undefined; - return { logged, exitCode: capturedExit }; + await writeFile( + script, + ` +import { handleError, OmniFocusCliError } from '${join(process.cwd(), 'src/lib/errors.ts')}'; + +let error; +switch (${JSON.stringify(scenario)}) { + case 'omnifocus_cli_error': + error = new OmniFocusCliError('bad request', 400); + break; + case 'error_not_found': + error = new Error('Task not found'); + break; + case 'error_multiple': + error = new Error('Multiple matches for "foo"'); + break; + case 'error_plain': + error = new Error('boom'); + break; + case 'non_error': + error = 'something weird'; + break; +} + +handleError(error); +// Intentionally do NOT call process.exit(). The whole point of handleError +// is that it sets exitCode and lets the loop drain. If anything is broken +// the test will hang or truncate. +`, + 'utf8' + ); + + try { + const { stdout } = await execFileAsync('bun', [script], { encoding: 'utf8' }); + return { stdout, exitCode: 0 }; + } catch (err) { + const e = err as { stdout?: string; code?: number | null }; + return { stdout: e.stdout ?? '', exitCode: typeof e.code === 'number' ? e.code : null }; + } } finally { - logSpy.mockRestore(); - process.exitCode = previousExitCode; + await rm(tmp, { recursive: true, force: true }); } } describe('handleError', () => { - it('sets process.exitCode to 1 without calling process.exit', () => { - const { exitCode } = runHandleError(new Error('boom')); + beforeAll(() => { + // Sanity check the helper compiles cleanly; nothing else needed. + }); + + it('exits with code 1 instead of calling process.exit', async () => { + const { exitCode } = await runHandleError('error_plain'); expect(exitCode).toBe(1); }); - it('returns normally so the event loop can drain stdout', () => { - // If handleError still called process.exit(), this test would never - // complete normally. Reaching the assertion proves the function returns. - expect(() => runHandleError(new Error('boom'))).not.toThrow(); + it('writes a complete JSON error body when piped (no truncation)', async () => { + const { stdout } = await runHandleError('error_plain'); + // The full JSON must parse — proves stdout drained before exit. + const parsed = JSON.parse(stdout); + expect(parsed).toEqual({ + error: { name: 'omnifocus_error', detail: 'boom', statusCode: 500 }, + }); }); - it('serializes OmniFocusCliError with its statusCode', () => { - const { logged } = runHandleError(new OmniFocusCliError('bad request', 400)); - expect(logged).toEqual({ + it('serializes OmniFocusCliError with its statusCode', async () => { + const { stdout } = await runHandleError('omnifocus_cli_error'); + expect(JSON.parse(stdout)).toEqual({ error: { name: 'cli_error', detail: 'bad request', statusCode: 400 }, }); }); - it('maps "not found" errors to 404', () => { - const { logged } = runHandleError(new Error('Task not found')); - expect(logged).toEqual({ + it('maps "not found" errors to 404', async () => { + const { stdout } = await runHandleError('error_not_found'); + expect(JSON.parse(stdout)).toEqual({ error: { name: 'omnifocus_error', detail: 'Task not found', statusCode: 404 }, }); }); - it('maps "Multiple" errors to 400', () => { - const { logged } = runHandleError(new Error('Multiple matches for "foo"')); - expect(logged).toEqual({ + it('maps "Multiple" errors to 400', async () => { + const { stdout } = await runHandleError('error_multiple'); + expect(JSON.parse(stdout)).toEqual({ error: { name: 'omnifocus_error', detail: 'Multiple matches for "foo"', @@ -60,9 +117,9 @@ describe('handleError', () => { }); }); - it('falls back to unknown_error for non-Error values', () => { - const { logged } = runHandleError('something weird'); - expect(logged).toEqual({ + it('falls back to unknown_error for non-Error values', async () => { + const { stdout } = await runHandleError('non_error'); + expect(JSON.parse(stdout)).toEqual({ error: { name: 'unknown_error', detail: 'An unknown error occurred', statusCode: 500 }, }); }); From 9994f9c9ac4f30cf517ae9153f91ec9ce5f58f2a Mon Sep 17 00:00:00 2001 From: Stephen Dolan Date: Thu, 21 May 2026 09:37:49 -0400 Subject: [PATCH 5/5] docs: document process.exit ban and bun test runner quirk Captures two lessons from fixing #20: - process.exit() truncates piped output at ~512 bytes; use exitCode. - bun's native test runner (used in CI) treats any process.exitCode mutation during a test as the suite's exit code, even when reverted. This differs from vitest, which is what `bun run test` uses locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 8e4f477..15f49ae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -93,6 +93,12 @@ Commands use `withErrorHandling()` HOF which: - **Window Requirement**: Perspective viewing requires an OmniFocus window to be open - **Permissions**: First run requires granting Automation permissions in System Settings - **ESM Modules**: Uses ES modules (type: "module" in package.json), all imports need .js extensions +- **Never call `process.exit()`**: It terminates the process before async stdout pipe writes drain, truncating piped output at ~512 bytes (issue #20). Set `process.exitCode` instead and let the event loop finish naturally. + +## Testing Notes + +- `bun run test` uses vitest; CI runs `bun test` (bun's native runner). They have different semantics — verify both locally before pushing. +- Bun's native test runner snapshots any `process.exitCode` mutation during a test and uses it as the suite's exit code, even when reverted. Tests that need to assert exit behavior must run the code in a child process. ## Date Handling