diff --git a/src/index.ts b/src/index.ts index e2cf663..7842ec9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -12,7 +12,7 @@ import { createTestCommand } from './commands/test.js'; import { createUsageCommand } from './commands/usage.js'; import { ApiError, CLIError, RequestTimeoutError } from './lib/errors.js'; import { Output, isOutputMode } from './lib/output.js'; -import { rephraseUnknownOption } from './lib/render-error.js'; +import { renderCommanderError, rephraseUnknownOption } from './lib/render-error.js'; import { maybeEmitSkillNudge } from './lib/skill-nudge.js'; import { VERSION } from './version.js'; @@ -77,27 +77,32 @@ program.addCommand(createTestCommand()); program.addCommand(createAgentCommand({})); program.addCommand(createUsageCommand()); -// Propagate exitOverride to every subcommand in the tree. -// Commander's addCommand() does NOT inherit exitOverride from the parent, -// so commands built externally (createTestCommand, createProjectCommand, …) -// and attached via addCommand() still have _exitCallback = null and call -// process.exit directly. Recursively set exitOverride so CommanderError -// bubbles up to our catch block for every leaf subcommand. +// Buffer Commander error messages instead of writing immediately. The catch +// block re-emits in the correct format (JSON or text) once the requested +// output mode is known. Safe because applyExitOverrideDeep ensures every +// command throws CommanderError rather than calling process.exit directly. +let pendingCommanderErrorMsg: string | null = null; + +// Propagate exitOverride AND the buffered outputError config to every +// subcommand in the tree. Commander's addCommand() does NOT inherit either +// from the parent, so commands built externally (createTestCommand, etc.) and +// attached via addCommand() still have _exitCallback = null and a default +// outputError that writes immediately. Both must be applied after the full +// command tree is assembled so every leaf subcommand behaves consistently. function applyExitOverrideDeep(cmd: Command): void { cmd.exitOverride(); + cmd.configureOutput({ + outputError(str, _write) { + const rephrased = rephraseUnknownOption(str); + pendingCommanderErrorMsg = rephrased !== null ? `${rephrased}\n` : str; + }, + }); for (const child of cmd.commands) { applyExitOverrideDeep(child); } } applyExitOverrideDeep(program); -program.configureOutput({ - outputError(str, write) { - const rephrased = rephraseUnknownOption(str); - write(rephrased !== null ? `${rephrased}\n` : str); - }, -}); - /** * Render a leaf command's full path (group + leaf), e.g. `test run` / * `auth whoami`, by walking parents up to (but not including) the root program. @@ -192,9 +197,8 @@ try { process.exit(err.exitCode); } if (err instanceof CommanderError) { - // Commander already wrote the error message (via configureOutput) or the - // help/version text to stdout. Map exit codes per the CLI taxonomy: - // help / version → 0 (user asked for it — no error) + // Map exit codes per the CLI taxonomy: + // help / version → 0 (user asked for it; Commander already wrote the text) // parse errors → 5 (VALIDATION_ERROR family: missing arg, invalid // option, unknown command, etc.) // @@ -213,6 +217,23 @@ try { ) { process.exit(0); } + // For parse errors, write the buffered message in the correct format. + // rawMode from program.opts() is reliable when --output was parsed before + // the error. When the error occurs first (e.g. `testsprite badcmd --output + // json`), rawMode is the default 'text'; scan argv as a best-effort + // fallback so machine consumers still receive a JSON envelope. + const commanderMode = (() => { + if (rawMode === 'json') return 'json' as const; + for (let i = 2; i < process.argv.length; i++) { + const arg = process.argv[i]!; + if (arg === '--output' && process.argv[i + 1] === 'json') return 'json' as const; + if (arg === '--output=json') return 'json' as const; + } + return mode; + })(); + process.stderr.write( + renderCommanderError(pendingCommanderErrorMsg, err.message, commanderMode), + ); process.exit(5); } if (err instanceof CLIError) { diff --git a/src/lib/render-error.test.ts b/src/lib/render-error.test.ts index 643e1bd..2b1ced7 100644 --- a/src/lib/render-error.test.ts +++ b/src/lib/render-error.test.ts @@ -7,7 +7,7 @@ * subcommand name. */ import { describe, expect, it } from 'vitest'; -import { rephraseUnknownOption } from './render-error.js'; +import { renderCommanderError, rephraseUnknownOption } from './render-error.js'; describe('rephraseUnknownOption', () => { it('rephrases --dry-run placed after subcommand', () => { @@ -88,3 +88,60 @@ describe('rephraseUnknownOption', () => { expect(result).toContain('testsprite --endpoint-url '); }); }); + +describe('renderCommanderError', () => { + it('json mode: emits VALIDATION_ERROR envelope', () => { + const out = renderCommanderError("error: unknown command 'foo'\n", 'fallback', 'json'); + const parsed = JSON.parse(out) as { + error: { code: string; message: string; requestId: string; nextAction: string }; + }; + expect(parsed.error.code).toBe('VALIDATION_ERROR'); + expect(parsed.error.message).toBe("error: unknown command 'foo'"); + expect(parsed.error.requestId).toBe('local'); + expect(typeof parsed.error.nextAction).toBe('string'); + expect(parsed.error.nextAction.length).toBeGreaterThan(0); + }); + + it('json mode: uses fallback when pendingMsg is null', () => { + const out = renderCommanderError(null, 'missing required argument', 'json'); + const parsed = JSON.parse(out) as { error: { message: string } }; + expect(parsed.error.message).toBe('missing required argument'); + }); + + it('json mode: trims trailing newline from message', () => { + const out = renderCommanderError('error: bad option\n', 'bad', 'json'); + const parsed = JSON.parse(out) as { error: { message: string } }; + expect(parsed.error.message).toBe('error: bad option'); + }); + + it('json mode: output is valid JSON ending with newline', () => { + const out = renderCommanderError('error: something', 'fallback', 'json'); + expect(out.endsWith('\n')).toBe(true); + expect(() => JSON.parse(out)).not.toThrow(); + }); + + it('text mode: returns pendingMsg as-is', () => { + const out = renderCommanderError('error: bad command\n', 'bad', 'text'); + expect(out).toBe('error: bad command\n'); + }); + + it('text mode: synthesizes from fallback when pendingMsg is null', () => { + const out = renderCommanderError(null, 'missing required argument', 'text'); + expect(out).toContain('missing required argument'); + }); + + it('json mode: rephrased global-flag message is embedded in envelope', () => { + const rephrased = rephraseUnknownOption("error: unknown option '--output'")!; + const out = renderCommanderError(`${rephrased}\n`, 'unknown option', 'json'); + const parsed = JSON.parse(out) as { error: { code: string; message: string } }; + expect(parsed.error.code).toBe('VALIDATION_ERROR'); + expect(parsed.error.message).toContain('--output'); + expect(parsed.error.message).toContain('global flag'); + }); + + it('json mode: envelope has exactly the expected top-level key', () => { + const out = renderCommanderError('error: test', 'test', 'json'); + const parsed = JSON.parse(out) as Record; + expect(Object.keys(parsed)).toEqual(['error']); + }); +}); diff --git a/src/lib/render-error.ts b/src/lib/render-error.ts index 33ecae6..ebb4208 100644 --- a/src/lib/render-error.ts +++ b/src/lib/render-error.ts @@ -4,6 +4,7 @@ * Extracted so the output-interceptor rephrasing logic (P10) is * unit-testable without spawning a full CLI process. */ +import type { OutputMode } from './output.js'; /** * Global flags that belong before the subcommand, not after it. @@ -29,6 +30,44 @@ const GLOBAL_FLAG_ARITY: Record = { * it is an ordinary unknown flag that should fall through to the original * Commander output. */ +/** + * Format a Commander parse-error message for the requested output mode. + * Returns the string to write to stderr; the caller writes it. + * + * pendingMsg: message captured by configureOutput.outputError before the + * CommanderError was thrown (may already be rephrased by + * rephraseUnknownOption). Null only when Commander threw without + * first calling outputError (should not happen for parse errors). + * fallbackMsg: err.message from CommanderError, used when pendingMsg is null. + * mode: the output mode resolved from --output (or argv fallback). + */ +export function renderCommanderError( + pendingMsg: string | null, + fallbackMsg: string, + mode: OutputMode, +): string { + const rawMsg = (pendingMsg ?? fallbackMsg).trim(); + if (mode === 'json') { + return ( + JSON.stringify( + { + error: { + code: 'VALIDATION_ERROR', + message: rawMsg, + nextAction: 'Run testsprite --help or testsprite --help for usage.', + requestId: 'local', + }, + }, + null, + 2, + ) + '\n' + ); + } + // Text mode: emit the buffered message as-is (already formatted by Commander + // or rephrased by rephraseUnknownOption). Synthesize when null. + return pendingMsg ?? `${rawMsg}\n`; +} + export function rephraseUnknownOption(raw: string): string | null { // Commander emits: "error: unknown option '--foo'" const match = /unknown option\s+'--([^']+)'/.exec(raw); diff --git a/test/cli.subprocess.test.ts b/test/cli.subprocess.test.ts index 8cbfa59..86643a2 100644 --- a/test/cli.subprocess.test.ts +++ b/test/cli.subprocess.test.ts @@ -1110,4 +1110,64 @@ describe('[fix-5] Commander parse errors → exit 5; help/version → exit 0', ( expect(result.exitCode).toBe(0); expect(result.stdout.trim()).toBeTruthy(); // version string on stdout }, 30_000); + + it('--output json: missing required arg emits JSON VALIDATION_ERROR envelope, not plain text', async () => { + // `test result` requires a positional . With --output json, the + // CommanderError must be rendered as a machine-readable JSON envelope so + // a coding agent parsing stderr does not receive an unexpected plain-text + // error and crash its JSON.parse. + const result = await runCli(['--output', 'json', 'test', 'result'], { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }); + expect(result.exitCode).toBe(5); + let parsed: { error: { code: string; message: string; requestId: string } }; + expect(() => { + parsed = JSON.parse(result.stderr) as typeof parsed; + }).not.toThrow(); + expect(parsed!.error.code).toBe('VALIDATION_ERROR'); + expect(parsed!.error.requestId).toBe('local'); + expect(parsed!.error.message).toBeTruthy(); + }, 30_000); + + it('--output json: unknown subcommand emits JSON envelope (--output before bad arg)', async () => { + // --output json appears before the unknown subcommand, so Commander parses + // it and program.opts().output is 'json' when the error fires. + const result = await runCli(['--output', 'json', 'test', 'not-a-real-subcommand'], { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }); + expect(result.exitCode).toBe(5); + let parsed: { error: { code: string } }; + expect(() => { + parsed = JSON.parse(result.stderr) as typeof parsed; + }).not.toThrow(); + expect(parsed!.error.code).toBe('VALIDATION_ERROR'); + }, 30_000); + + it('--output json after bad arg: argv fallback detects json mode and emits envelope', async () => { + // The error fires before --output json is parsed, so program.opts().output + // is the default 'text'. The argv fallback scan must still detect json mode. + const result = await runCli(['test', 'not-a-real-subcommand', '--output', 'json'], { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }); + expect(result.exitCode).toBe(5); + let parsed: { error: { code: string } }; + expect(() => { + parsed = JSON.parse(result.stderr) as typeof parsed; + }).not.toThrow(); + expect(parsed!.error.code).toBe('VALIDATION_ERROR'); + }, 30_000); + + it('text mode: Commander parse error still emits plain text (no regression)', async () => { + const result = await runCli(['test', 'result'], { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }); + expect(result.exitCode).toBe(5); + // Must NOT be a JSON envelope in text mode. + expect(() => JSON.parse(result.stderr)).toThrow(); + expect(result.stderr).toContain('test-id'); + }, 30_000); });