From 8b5a92a7d62ae5779e240ec25aacbe015fd9c0dd Mon Sep 17 00:00:00 2001 From: davidson3556 Date: Tue, 23 Jun 2026 17:40:22 -0700 Subject: [PATCH] fix(cli): validate --output uniformly across all command groups Only `test` and `project` validated the global `--output` flag. The `auth`, `usage`, `agent`, and `init` command groups resolved it with `globals.output ?? 'text'`, so an unrecognised value (e.g. a typo like `--output josn`) was silently coerced to text mode instead of being rejected. A coding agent that asked for `--output json` then received a human-readable text payload and failed to parse it as JSON, with no signal as to why. Extract the validation into a shared `resolveOutputMode` helper in `lib/output.js` and route every command group's `resolveCommonOptions` through it. Invalid values now throw a typed VALIDATION_ERROR (exit 5) with an actionable message everywhere. This also unifies the error wording, which previously differed between `test` ("Flag `--output` is invalid: must be one of: json, text.") and `project` ("--output must be one of: json, text"). --- src/commands/agent.ts | 4 ++-- src/commands/auth.ts | 4 ++-- src/commands/init.ts | 4 ++-- src/commands/project.ts | 8 ++------ src/commands/test.ts | 8 ++------ src/commands/usage.ts | 4 ++-- src/lib/output.test.ts | 33 ++++++++++++++++++++++++++++++++- src/lib/output.ts | 22 ++++++++++++++++++++++ test/cli.subprocess.test.ts | 27 +++++++++++++++++++++++++++ 9 files changed, 93 insertions(+), 21 deletions(-) diff --git a/src/commands/agent.ts b/src/commands/agent.ts index c27e2b8..3c3e93c 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -4,7 +4,7 @@ import { Command } from 'commander'; import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factory.js'; import { CLIError, localValidationError } from '../lib/errors.js'; import type { OutputMode } from '../lib/output.js'; -import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js'; import { promptText } from '../lib/prompt.js'; import { type AgentTarget, @@ -757,7 +757,7 @@ function resolveCommonOptions(command: Command): CommonOptions { const globals = command.optsWithGlobals() as Partial; return { profile: globals.profile ?? 'default', - output: globals.output ?? 'text', + output: resolveOutputMode(globals.output), endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, verbose: globals.verbose ?? false, diff --git a/src/commands/auth.ts b/src/commands/auth.ts index de1eda4..5ce6a90 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -18,7 +18,7 @@ import { import { loadConfig } from '../lib/config.js'; import { emitDeprecationNotice } from '../lib/deprecate.js'; import type { OutputMode } from '../lib/output.js'; -import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js'; import { promptSecret } from '../lib/prompt.js'; export interface MeResponse { @@ -318,7 +318,7 @@ function resolveCommonOptions(command: Command): CommonOptions { }; return { profile: globals.profile ?? 'default', - output: globals.output ?? 'text', + output: resolveOutputMode(globals.output), endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, verbose: globals.verbose ?? false, diff --git a/src/commands/init.ts b/src/commands/init.ts index dc3effd..13e8331 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -14,7 +14,7 @@ import { Command } from 'commander'; import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factory.js'; import { emitDeprecationNotice } from '../lib/deprecate.js'; import { CLIError } from '../lib/errors.js'; -import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js'; import type { AuthDeps, MeResponse } from './auth.js'; import { runConfigure, runWhoami } from './auth.js'; import type { AgentDeps, AgentFs, InstallResult } from './agent.js'; @@ -391,7 +391,7 @@ function resolveCommonOptions(command: Command): CommonOptions { }; return { profile: globals.profile ?? 'default', - output: globals.output ?? 'text', + output: resolveOutputMode(globals.output), endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, verbose: globals.verbose ?? false, diff --git a/src/commands/project.ts b/src/commands/project.ts index 277f3ca..9e88e07 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -8,7 +8,7 @@ import { import { ApiError } from '../lib/errors.js'; import type { FetchImpl } from '../lib/http.js'; import type { HttpClient } from '../lib/http.js'; -import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js'; import { assertNotLocal } from '../lib/target-url.js'; import { assertIdempotencyKey } from '../lib/validate.js'; import { @@ -488,13 +488,9 @@ function resolveCommonOptions(command: Command): CommonOptions { requestTimeout?: string; }; // P2-8: validate --output before allowing silent fallback to 'text'. - const rawOutput = globals.output; - if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') { - throw localValidationError('--output must be one of: json, text'); - } return { profile: globals.profile ?? 'default', - output: (globals.output as OutputMode | undefined) ?? 'text', + output: resolveOutputMode(globals.output), endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, verbose: globals.verbose ?? false, diff --git a/src/commands/test.ts b/src/commands/test.ts index e254958..b775d99 100644 --- a/src/commands/test.ts +++ b/src/commands/test.ts @@ -34,7 +34,7 @@ import { import { REQUEST_TIMEOUT_DEFAULT_MS, REQUEST_TIMEOUT_MAX_MS } from '../lib/http.js'; import type { FetchImpl } from '../lib/http.js'; import type { HttpClient } from '../lib/http.js'; -import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js'; import { fetchSinglePage, paginate, @@ -7670,13 +7670,9 @@ function resolveCommonOptions(command: Command): CommonOptions { // P2-8: validate --output before allowing silent fallback to 'text'. // An invalid value (e.g. `--output yaml`) must exit 5 with a clear error // rather than silently treating the request as text mode. - const rawOutput = globals.output; - if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') { - throw localValidationError('output', 'must be one of: json, text', ['json', 'text']); - } return { profile: globals.profile ?? 'default', - output: (globals.output as OutputMode | undefined) ?? 'text', + output: resolveOutputMode(globals.output), dryRun: globals.dryRun ?? false, endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, diff --git a/src/commands/usage.ts b/src/commands/usage.ts index d8a0f21..a539261 100644 --- a/src/commands/usage.ts +++ b/src/commands/usage.ts @@ -22,7 +22,7 @@ import { import { loadConfig } from '../lib/config.js'; import { resolvePortalBase } from '../lib/facade.js'; import type { FetchImpl } from '../lib/http.js'; -import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js'; +import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js'; /** * Usage/balance response from `/me` (when the backend supplies it) or a future @@ -216,7 +216,7 @@ function resolveCommonOptions(command: Command): CommonOptions { }; return { profile: globals.profile ?? 'default', - output: globals.output ?? 'text', + output: resolveOutputMode(globals.output), endpointUrl: globals.endpointUrl, debug: globals.debug ?? false, verbose: globals.verbose ?? false, diff --git a/src/lib/output.test.ts b/src/lib/output.test.ts index b572bda..bab3b53 100644 --- a/src/lib/output.test.ts +++ b/src/lib/output.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { Output, isOutputMode } from './output.js'; +import { Output, isOutputMode, resolveOutputMode } from './output.js'; +import { ApiError } from './errors.js'; describe('isOutputMode', () => { it('accepts json and text', () => { @@ -15,6 +16,36 @@ describe('isOutputMode', () => { }); }); +describe('resolveOutputMode', () => { + it('returns the mode verbatim for valid values', () => { + expect(resolveOutputMode('json')).toBe('json'); + expect(resolveOutputMode('text')).toBe('text'); + }); + + it('defaults to text when the flag is omitted (undefined)', () => { + expect(resolveOutputMode(undefined)).toBe('text'); + }); + + it('throws a typed VALIDATION_ERROR (exit 5) instead of silently falling back to text', () => { + // The footgun this guards against: an agent that asks for `--output json` + // but mistypes it would otherwise receive a text payload and fail to parse + // it as JSON with no signal. Every command group must reject, not coerce. + for (const bad of ['josn', 'yaml', 'JSON', 'Text', '']) { + let caught: unknown; + try { + resolveOutputMode(bad); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(ApiError); + const apiErr = caught as ApiError; + expect(apiErr.code).toBe('VALIDATION_ERROR'); + expect(apiErr.exitCode).toBe(5); + expect(apiErr.nextAction).toContain('must be one of: json, text'); + } + }); +}); + describe('Output', () => { let logSpy: ReturnType; let errorSpy: ReturnType; diff --git a/src/lib/output.ts b/src/lib/output.ts index 2b2db39..9713f24 100644 --- a/src/lib/output.ts +++ b/src/lib/output.ts @@ -1,3 +1,5 @@ +import { localValidationError } from './errors.js'; + export type OutputMode = 'json' | 'text'; /** @@ -13,6 +15,26 @@ export function isOutputMode(value: unknown): value is OutputMode { return value === 'json' || value === 'text'; } +/** + * Resolve a raw `--output` flag value to a concrete {@link OutputMode}. + * + * `undefined` (flag omitted) resolves to the default `'text'`. Any other + * value that is not `'json'` or `'text'` throws a typed VALIDATION_ERROR + * (exit 5) with an actionable message. + * + * The alternative — silently falling back to `'text'` — is a footgun for the + * CLI's primary consumer (coding agents): a caller that asks for + * `--output json` but mistypes it (`--output josn`) would otherwise receive a + * human-readable text payload and fail to parse it as JSON, with no signal as + * to why. Every command group routes its global-option resolution through this + * helper so the validation is uniform. + */ +export function resolveOutputMode(raw: unknown): OutputMode { + if (raw === undefined) return 'text'; + if (raw === 'json' || raw === 'text') return raw; + throw localValidationError('output', 'must be one of: json, text', ['json', 'text']); +} + export interface OutputStreams { /** * Line-oriented stdout writer. Each call is one logical line; the diff --git a/test/cli.subprocess.test.ts b/test/cli.subprocess.test.ts index 8cbfa59..16a6106 100644 --- a/test/cli.subprocess.test.ts +++ b/test/cli.subprocess.test.ts @@ -499,6 +499,33 @@ describe('project list subprocess', () => { }, 30_000); }); +describe('invalid --output is rejected uniformly (exit 5)', () => { + // Regression: previously only `test` and `project` validated `--output`; + // `auth`, `usage`, `agent`, and `init` silently coerced an unknown value to + // text mode. An agent that asked for `--output json` but mistyped it then + // received a text payload it could not parse, with no signal as to why. Every + // command group now routes through resolveOutputMode (exit 5 on bad input). + + // Note: when `--output` itself is the invalid value, the requested mode is + // unusable for the error envelope, so it is rendered in text mode (the catch + // block in index.ts falls back to text for an unrecognised --output). + + it('agent list --output josn exits 5 with an actionable message (offline command)', async () => { + const result = await runCli(['--output', 'josn', 'agent', 'list'], {}); + expect(result.exitCode).toBe(5); + expect(result.stderr).toContain('must be one of: json, text'); + }, 30_000); + + it('auth status --output yaml exits 5 before any network call', async () => { + const result = await runCli(['--output', 'yaml', 'auth', 'status'], { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }); + expect(result.exitCode).toBe(5); + expect(result.stderr).toContain('must be one of: json, text'); + }, 30_000); +}); + describe('project get subprocess', () => { it('--output json returns the §6.1 Project shape', async () => { const result = await runCli(['--output', 'json', 'project', 'get', 'project_subproc'], {