Skip to content

Commit 45c16b1

Browse files
committed
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").
1 parent 3ab8136 commit 45c16b1

9 files changed

Lines changed: 93 additions & 21 deletions

File tree

src/commands/agent.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { Command } from 'commander';
44
import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factory.js';
55
import { CLIError, localValidationError } from '../lib/errors.js';
66
import type { OutputMode } from '../lib/output.js';
7-
import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js';
7+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js';
88
import { promptText } from '../lib/prompt.js';
99
import {
1010
type AgentTarget,
@@ -852,7 +852,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
852852
const globals = command.optsWithGlobals() as Partial<CommonOptions>;
853853
return {
854854
profile: globals.profile ?? 'default',
855-
output: globals.output ?? 'text',
855+
output: resolveOutputMode(globals.output),
856856
endpointUrl: globals.endpointUrl,
857857
debug: globals.debug ?? false,
858858
verbose: globals.verbose ?? false,

src/commands/auth.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
import { loadConfig } from '../lib/config.js';
1919
import { emitDeprecationNotice } from '../lib/deprecate.js';
2020
import type { OutputMode } from '../lib/output.js';
21-
import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js';
21+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js';
2222
import { promptSecret } from '../lib/prompt.js';
2323

2424
export interface MeResponse {
@@ -318,7 +318,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
318318
};
319319
return {
320320
profile: globals.profile ?? 'default',
321-
output: globals.output ?? 'text',
321+
output: resolveOutputMode(globals.output),
322322
endpointUrl: globals.endpointUrl,
323323
debug: globals.debug ?? false,
324324
verbose: globals.verbose ?? false,

src/commands/init.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { Command } from 'commander';
1414
import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factory.js';
1515
import { emitDeprecationNotice } from '../lib/deprecate.js';
1616
import { CLIError } from '../lib/errors.js';
17-
import { GLOBAL_OPTS_HINT, Output } from '../lib/output.js';
17+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode } from '../lib/output.js';
1818
import type { AuthDeps, MeResponse } from './auth.js';
1919
import { runConfigure, runWhoami } from './auth.js';
2020
import type { AgentDeps, AgentFs, InstallResult } from './agent.js';
@@ -424,7 +424,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
424424
};
425425
return {
426426
profile: globals.profile ?? 'default',
427-
output: globals.output ?? 'text',
427+
output: resolveOutputMode(globals.output),
428428
endpointUrl: globals.endpointUrl,
429429
debug: globals.debug ?? false,
430430
verbose: globals.verbose ?? false,

src/commands/project.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
import { ApiError } from '../lib/errors.js';
1010
import type { FetchImpl } from '../lib/http.js';
1111
import type { HttpClient } from '../lib/http.js';
12-
import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js';
12+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js';
1313
import { assertNotLocal } from '../lib/target-url.js';
1414
import { assertIdempotencyKey } from '../lib/validate.js';
1515
import {
@@ -494,13 +494,9 @@ function resolveCommonOptions(command: Command): CommonOptions {
494494
requestTimeout?: string;
495495
};
496496
// P2-8: validate --output before allowing silent fallback to 'text'.
497-
const rawOutput = globals.output;
498-
if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') {
499-
throw localValidationError('--output must be one of: json, text');
500-
}
501497
return {
502498
profile: globals.profile ?? 'default',
503-
output: (globals.output as OutputMode | undefined) ?? 'text',
499+
output: resolveOutputMode(globals.output),
504500
endpointUrl: globals.endpointUrl,
505501
debug: globals.debug ?? false,
506502
verbose: globals.verbose ?? false,

src/commands/test.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import {
3434
import { REQUEST_TIMEOUT_DEFAULT_MS, REQUEST_TIMEOUT_MAX_MS } from '../lib/http.js';
3535
import type { FetchImpl } from '../lib/http.js';
3636
import type { HttpClient } from '../lib/http.js';
37-
import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js';
37+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js';
3838
import {
3939
fetchSinglePage,
4040
paginate,
@@ -7806,13 +7806,9 @@ function resolveCommonOptions(command: Command): CommonOptions {
78067806
// P2-8: validate --output before allowing silent fallback to 'text'.
78077807
// An invalid value (e.g. `--output yaml`) must exit 5 with a clear error
78087808
// rather than silently treating the request as text mode.
7809-
const rawOutput = globals.output;
7810-
if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') {
7811-
throw localValidationError('output', 'must be one of: json, text', ['json', 'text']);
7812-
}
78137809
return {
78147810
profile: globals.profile ?? 'default',
7815-
output: (globals.output as OutputMode | undefined) ?? 'text',
7811+
output: resolveOutputMode(globals.output),
78167812
dryRun: globals.dryRun ?? false,
78177813
endpointUrl: globals.endpointUrl,
78187814
debug: globals.debug ?? false,

src/commands/usage.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
import { loadConfig } from '../lib/config.js';
2323
import { resolvePortalBase } from '../lib/facade.js';
2424
import type { FetchImpl } from '../lib/http.js';
25-
import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js';
25+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js';
2626

2727
/**
2828
* Usage/balance response from `/me` (when the backend supplies it) or a future
@@ -216,7 +216,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
216216
};
217217
return {
218218
profile: globals.profile ?? 'default',
219-
output: globals.output ?? 'text',
219+
output: resolveOutputMode(globals.output),
220220
endpointUrl: globals.endpointUrl,
221221
debug: globals.debug ?? false,
222222
verbose: globals.verbose ?? false,

src/lib/output.test.ts

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
2-
import { Output, isOutputMode } from './output.js';
2+
import { Output, isOutputMode, resolveOutputMode } from './output.js';
3+
import { ApiError } from './errors.js';
34

45
describe('isOutputMode', () => {
56
it('accepts json and text', () => {
@@ -15,6 +16,36 @@ describe('isOutputMode', () => {
1516
});
1617
});
1718

19+
describe('resolveOutputMode', () => {
20+
it('returns the mode verbatim for valid values', () => {
21+
expect(resolveOutputMode('json')).toBe('json');
22+
expect(resolveOutputMode('text')).toBe('text');
23+
});
24+
25+
it('defaults to text when the flag is omitted (undefined)', () => {
26+
expect(resolveOutputMode(undefined)).toBe('text');
27+
});
28+
29+
it('throws a typed VALIDATION_ERROR (exit 5) instead of silently falling back to text', () => {
30+
// The footgun this guards against: an agent that asks for `--output json`
31+
// but mistypes it would otherwise receive a text payload and fail to parse
32+
// it as JSON with no signal. Every command group must reject, not coerce.
33+
for (const bad of ['josn', 'yaml', 'JSON', 'Text', '']) {
34+
let caught: unknown;
35+
try {
36+
resolveOutputMode(bad);
37+
} catch (err) {
38+
caught = err;
39+
}
40+
expect(caught).toBeInstanceOf(ApiError);
41+
const apiErr = caught as ApiError;
42+
expect(apiErr.code).toBe('VALIDATION_ERROR');
43+
expect(apiErr.exitCode).toBe(5);
44+
expect(apiErr.nextAction).toContain('must be one of: json, text');
45+
}
46+
});
47+
});
48+
1849
describe('Output', () => {
1950
let logSpy: ReturnType<typeof vi.spyOn>;
2051
let errorSpy: ReturnType<typeof vi.spyOn>;

src/lib/output.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { localValidationError } from './errors.js';
2+
13
export type OutputMode = 'json' | 'text';
24

35
/**
@@ -13,6 +15,26 @@ export function isOutputMode(value: unknown): value is OutputMode {
1315
return value === 'json' || value === 'text';
1416
}
1517

18+
/**
19+
* Resolve a raw `--output` flag value to a concrete {@link OutputMode}.
20+
*
21+
* `undefined` (flag omitted) resolves to the default `'text'`. Any other
22+
* value that is not `'json'` or `'text'` throws a typed VALIDATION_ERROR
23+
* (exit 5) with an actionable message.
24+
*
25+
* The alternative — silently falling back to `'text'` — is a footgun for the
26+
* CLI's primary consumer (coding agents): a caller that asks for
27+
* `--output json` but mistypes it (`--output josn`) would otherwise receive a
28+
* human-readable text payload and fail to parse it as JSON, with no signal as
29+
* to why. Every command group routes its global-option resolution through this
30+
* helper so the validation is uniform.
31+
*/
32+
export function resolveOutputMode(raw: unknown): OutputMode {
33+
if (raw === undefined) return 'text';
34+
if (isOutputMode(raw)) return raw;
35+
throw localValidationError('output', 'must be one of: json, text', ['json', 'text']);
36+
}
37+
1638
export interface OutputStreams {
1739
/**
1840
* Line-oriented stdout writer. Each call is one logical line; the

test/cli.subprocess.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,33 @@ describe('malformed --endpoint-url is rejected (exit 5), not retried as a networ
530530
}, 30_000);
531531
});
532532

533+
describe('invalid --output is rejected uniformly (exit 5)', () => {
534+
// Regression: previously only `test` and `project` validated `--output`;
535+
// `auth`, `usage`, `agent`, and `init` silently coerced an unknown value to
536+
// text mode. An agent that asked for `--output json` but mistyped it then
537+
// received a text payload it could not parse, with no signal as to why. Every
538+
// command group now routes through resolveOutputMode (exit 5 on bad input).
539+
540+
// Note: when `--output` itself is the invalid value, the requested mode is
541+
// unusable for the error envelope, so it is rendered in text mode (the catch
542+
// block in index.ts falls back to text for an unrecognised --output).
543+
544+
it('agent list --output josn exits 5 with an actionable message (offline command)', async () => {
545+
const result = await runCli(['--output', 'josn', 'agent', 'list'], {});
546+
expect(result.exitCode).toBe(5);
547+
expect(result.stderr).toContain('must be one of: json, text');
548+
}, 30_000);
549+
550+
it('auth status --output yaml exits 5 before any network call', async () => {
551+
const result = await runCli(['--output', 'yaml', 'auth', 'status'], {
552+
TESTSPRITE_API_KEY: 'sk-subproc',
553+
TESTSPRITE_API_URL: baseUrl,
554+
});
555+
expect(result.exitCode).toBe(5);
556+
expect(result.stderr).toContain('must be one of: json, text');
557+
}, 30_000);
558+
});
559+
533560
describe('a malformed --profile is rejected (exit 5), not silently corrupting credentials', () => {
534561
// A profile name becomes an INI section header (`[name]`). `prod]` would
535562
// serialise to `[prod]]`, which the parser cannot read back — `setup` would

0 commit comments

Comments
 (0)