Skip to content

Commit 77463ec

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 15e95de commit 77463ec

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,
@@ -757,7 +757,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
757757
const globals = command.optsWithGlobals() as Partial<CommonOptions>;
758758
return {
759759
profile: globals.profile ?? 'default',
760-
output: globals.output ?? 'text',
760+
output: resolveOutputMode(globals.output),
761761
endpointUrl: globals.endpointUrl,
762762
debug: globals.debug ?? false,
763763
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';
@@ -391,7 +391,7 @@ function resolveCommonOptions(command: Command): CommonOptions {
391391
};
392392
return {
393393
profile: globals.profile ?? 'default',
394-
output: globals.output ?? 'text',
394+
output: resolveOutputMode(globals.output),
395395
endpointUrl: globals.endpointUrl,
396396
debug: globals.debug ?? false,
397397
verbose: globals.verbose ?? false,

src/commands/project.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import {
88
import { ApiError } from '../lib/errors.js';
99
import type { FetchImpl } from '../lib/http.js';
1010
import type { HttpClient } from '../lib/http.js';
11-
import { GLOBAL_OPTS_HINT, Output, type OutputMode } from '../lib/output.js';
11+
import { GLOBAL_OPTS_HINT, Output, resolveOutputMode, type OutputMode } from '../lib/output.js';
1212
import { assertNotLocal } from '../lib/target-url.js';
1313
import { assertIdempotencyKey } from '../lib/validate.js';
1414
import {
@@ -488,13 +488,9 @@ function resolveCommonOptions(command: Command): CommonOptions {
488488
requestTimeout?: string;
489489
};
490490
// P2-8: validate --output before allowing silent fallback to 'text'.
491-
const rawOutput = globals.output;
492-
if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') {
493-
throw localValidationError('--output must be one of: json, text');
494-
}
495491
return {
496492
profile: globals.profile ?? 'default',
497-
output: (globals.output as OutputMode | undefined) ?? 'text',
493+
output: resolveOutputMode(globals.output),
498494
endpointUrl: globals.endpointUrl,
499495
debug: globals.debug ?? false,
500496
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,
@@ -7761,13 +7761,9 @@ function resolveCommonOptions(command: Command): CommonOptions {
77617761
// P2-8: validate --output before allowing silent fallback to 'text'.
77627762
// An invalid value (e.g. `--output yaml`) must exit 5 with a clear error
77637763
// rather than silently treating the request as text mode.
7764-
const rawOutput = globals.output;
7765-
if (rawOutput !== undefined && rawOutput !== 'json' && rawOutput !== 'text') {
7766-
throw localValidationError('output', 'must be one of: json, text', ['json', 'text']);
7767-
}
77687764
return {
77697765
profile: globals.profile ?? 'default',
7770-
output: (globals.output as OutputMode | undefined) ?? 'text',
7766+
output: resolveOutputMode(globals.output),
77717767
dryRun: globals.dryRun ?? false,
77727768
endpointUrl: globals.endpointUrl,
77737769
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 (raw === 'json' || raw === 'text') 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
@@ -499,6 +499,33 @@ describe('project list subprocess', () => {
499499
}, 30_000);
500500
});
501501

502+
describe('invalid --output is rejected uniformly (exit 5)', () => {
503+
// Regression: previously only `test` and `project` validated `--output`;
504+
// `auth`, `usage`, `agent`, and `init` silently coerced an unknown value to
505+
// text mode. An agent that asked for `--output json` but mistyped it then
506+
// received a text payload it could not parse, with no signal as to why. Every
507+
// command group now routes through resolveOutputMode (exit 5 on bad input).
508+
509+
// Note: when `--output` itself is the invalid value, the requested mode is
510+
// unusable for the error envelope, so it is rendered in text mode (the catch
511+
// block in index.ts falls back to text for an unrecognised --output).
512+
513+
it('agent list --output josn exits 5 with an actionable message (offline command)', async () => {
514+
const result = await runCli(['--output', 'josn', 'agent', 'list'], {});
515+
expect(result.exitCode).toBe(5);
516+
expect(result.stderr).toContain('must be one of: json, text');
517+
}, 30_000);
518+
519+
it('auth status --output yaml exits 5 before any network call', async () => {
520+
const result = await runCli(['--output', 'yaml', 'auth', 'status'], {
521+
TESTSPRITE_API_KEY: 'sk-subproc',
522+
TESTSPRITE_API_URL: baseUrl,
523+
});
524+
expect(result.exitCode).toBe(5);
525+
expect(result.stderr).toContain('must be one of: json, text');
526+
}, 30_000);
527+
});
528+
502529
describe('a malformed --profile is rejected (exit 5), not silently corrupting credentials', () => {
503530
// A profile name becomes an INI section header (`[name]`). `prod]` would
504531
// serialise to `[prod]]`, which the parser cannot read back — `setup` would

0 commit comments

Comments
 (0)