fix(cli): emit JSON envelope for Commander parse errors under --output json (#22)

When a subcommand fired a parse error (unknown command, missing required
argument, invalid option), Commander's outputError callback wrote plain
text to stderr immediately and the catch block exited 5 with no further
output. A machine consumer that always parses stderr as JSON received an
unexpected plain-text string and crashed its JSON.parse.

Root cause: configureOutput was only applied to the root program, not to
subcommands. Each subcommand retained the default outputError that calls
write(str) directly. applyExitOverrideDeep now also propagates
configureOutput to every leaf so the message is buffered instead of
written.

In the CommanderError catch block, a resolved output mode is used to
either write a VALIDATION_ERROR JSON envelope or the buffered plain-text
message. An argv scan fallback handles the edge case where --output json
appears after the bad argument and was not yet parsed when the error fired.

The renderCommanderError helper is extracted to src/lib/render-error.ts
(alongside the existing rephraseUnknownOption helper) so it is unit-testable
without a subprocess. Eight unit tests cover JSON/text output, null fallback,
message trimming, and rephrased global-flag embedding. Four subprocess
regression tests in the [fix-5] block cover missing-arg, unknown subcommand,
argv-fallback, and text-mode no-regression paths.

Co-authored-by: zeshi-du <zeshi@testsprite.com>

Author: crypticsaiyan

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) {

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 <value> <subcommand>');
   });
 });
+
+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<string, unknown>;
+    expect(Object.keys(parsed)).toEqual(['error']);
+  });
+});

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<string, 'boolean' | 'value'> = {
  * 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 <command> --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);

test/cli.subprocess.test.ts

@@ -1156,4 +1156,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 <test-id>. 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);
 });