diff --git a/src/commands/auth.ts b/src/commands/auth.ts index de1eda4..c78c2a9 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -2,6 +2,7 @@ import { Command } from 'commander'; import { emitDryRunBanner, makeHttpClient, + parseRequestTimeoutFlag, type CommonOptions as FactoryCommonOptions, } from '../lib/client-factory.js'; import type { ErrorCode } from '../lib/errors.js'; @@ -327,19 +328,6 @@ function resolveCommonOptions(command: Command): CommonOptions { }; } -/** - * Parse the `--request-timeout ` flag value into milliseconds. - * Returns `undefined` when the flag was not supplied (factory falls back to - * the env var / default). Silently clamps out-of-range values — the - * factory applies the same clamp so there is no double-clamp risk. - */ -function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { - if (raw === undefined) return undefined; - const n = Number(raw); - if (!Number.isFinite(n) || n <= 0) return undefined; - return Math.round(n * 1000); // seconds → milliseconds -} - function makeOutput(mode: OutputMode, deps: AuthDeps): Output { return new Output(mode, { stdout: deps.stdout, stderr: deps.stderr }); } diff --git a/src/commands/init.ts b/src/commands/init.ts index dc3effd..04fb8ad 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -11,7 +11,10 @@ */ import { Command } from 'commander'; -import type { CommonOptions as FactoryCommonOptions } from '../lib/client-factory.js'; +import { + parseRequestTimeoutFlag, + 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'; @@ -400,13 +403,6 @@ function resolveCommonOptions(command: Command): CommonOptions { }; } -function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { - if (raw === undefined) return undefined; - const n = Number(raw); - if (!Number.isFinite(n) || n <= 0) return undefined; - return Math.round(n * 1000); -} - const SETUP_DESCRIPTION = 'Set up TestSprite: configure your API key and install the verification skill for your coding agent'; diff --git a/src/commands/project.ts b/src/commands/project.ts index 277f3ca..810bf87 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -3,6 +3,7 @@ import { readFileSync } from 'node:fs'; import { Command } from 'commander'; import { makeHttpClient, + parseRequestTimeoutFlag, type CommonOptions as FactoryCommonOptions, } from '../lib/client-factory.js'; import { ApiError } from '../lib/errors.js'; @@ -503,18 +504,6 @@ function resolveCommonOptions(command: Command): CommonOptions { }; } -/** - * Parse the `--request-timeout ` flag value into milliseconds. - * Returns `undefined` when the flag was not supplied (factory falls back to - * the env var / default). Silently clamps out-of-range values. - */ -function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { - if (raw === undefined) return undefined; - const n = Number(raw); - if (!Number.isFinite(n) || n <= 0) return undefined; - return Math.round(n * 1000); // seconds → milliseconds -} - function makeClient(opts: CommonOptions, deps: ProjectDeps): HttpClient { return makeHttpClient(opts, { env: deps.env, diff --git a/src/commands/test.ts b/src/commands/test.ts index e254958..8a944e4 100644 --- a/src/commands/test.ts +++ b/src/commands/test.ts @@ -6,6 +6,7 @@ import { Command } from 'commander'; import { emitDryRunBanner, makeHttpClient, + parseRequestTimeoutFlag, type CommonOptions as FactoryCommonOptions, } from '../lib/client-factory.js'; import { @@ -7685,18 +7686,6 @@ function resolveCommonOptions(command: Command): CommonOptions { }; } -/** - * Parse the `--request-timeout ` flag value into milliseconds. - * Returns `undefined` when the flag was not supplied (factory falls back to - * the env var / default). Silently clamps out-of-range values. - */ -function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { - if (raw === undefined) return undefined; - const n = Number(raw); - if (!Number.isFinite(n) || n <= 0) return undefined; - return Math.round(n * 1000); // seconds → milliseconds -} - /** D4: headroom added on top of `--timeout` when deriving the per-request window under `--wait`. */ const WAIT_REQUEST_TIMEOUT_CUSHION_MS = 5_000; diff --git a/src/commands/usage.ts b/src/commands/usage.ts index d8a0f21..9975826 100644 --- a/src/commands/usage.ts +++ b/src/commands/usage.ts @@ -17,6 +17,7 @@ import { Command } from 'commander'; import { emitDryRunBanner, makeHttpClient, + parseRequestTimeoutFlag, type CommonOptions as FactoryCommonOptions, } from '../lib/client-factory.js'; import { loadConfig } from '../lib/config.js'; @@ -225,13 +226,6 @@ function resolveCommonOptions(command: Command): CommonOptions { }; } -function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { - if (raw === undefined) return undefined; - const n = Number(raw); - if (!Number.isFinite(n) || n <= 0) return undefined; - return Math.round(n * 1000); -} - function makeOutput(mode: OutputMode, deps: UsageDeps): Output { return new Output(mode, { stdout: deps.stdout, stderr: deps.stderr }); } diff --git a/src/lib/client-factory.test.ts b/src/lib/client-factory.test.ts index f18568e..162d337 100644 --- a/src/lib/client-factory.test.ts +++ b/src/lib/client-factory.test.ts @@ -4,6 +4,7 @@ import { DRY_RUN_BANNER, emitDryRunBanner, makeHttpClient, + parseRequestTimeoutFlag, resetDryRunBannerForTesting, resolveRequestTimeoutMs, } from './client-factory.js'; @@ -12,6 +13,7 @@ import { REQUEST_TIMEOUT_MAX_MS, REQUEST_TIMEOUT_MIN_MS, } from './http.js'; +import { ApiError } from './errors.js'; const NO_CREDS_PATH = '/tmp/testsprite-cli-test-no-such-file-1234.ini'; @@ -211,6 +213,45 @@ describe('resolveRequestTimeoutMs', () => { }); }); +// --------------------------------------------------------------------------- +// parseRequestTimeoutFlag — strict flag parsing (seconds → ms) +// --------------------------------------------------------------------------- + +describe('parseRequestTimeoutFlag', () => { + it('returns undefined when the flag is omitted (factory falls back to env/default)', () => { + expect(parseRequestTimeoutFlag(undefined)).toBeUndefined(); + }); + + it('converts a positive number of seconds to milliseconds', () => { + expect(parseRequestTimeoutFlag('30')).toBe(30_000); + expect(parseRequestTimeoutFlag('1')).toBe(1_000); + expect(parseRequestTimeoutFlag('2.5')).toBe(2_500); + }); + + it('does NOT reject positive out-of-range values — resolveRequestTimeoutMs clamps them', () => { + // 700s is above the 600s cap, but parsing succeeds; the clamp lives in + // resolveRequestTimeoutMs so a large script-supplied value still works. + expect(parseRequestTimeoutFlag('700')).toBe(700_000); + }); + + it.each(['abc', '30s', '0', '-5', 'NaN', 'Infinity', ''])( + 'throws a VALIDATION_ERROR (exit 5) on the invalid flag value %j', + bad => { + let caught: unknown; + try { + parseRequestTimeoutFlag(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('request-timeout'); + }, + ); +}); + // --------------------------------------------------------------------------- // makeHttpClient — requestTimeoutMs propagation // --------------------------------------------------------------------------- diff --git a/src/lib/client-factory.ts b/src/lib/client-factory.ts index fad2fab..b3b3993 100644 --- a/src/lib/client-factory.ts +++ b/src/lib/client-factory.ts @@ -15,7 +15,7 @@ */ import { loadConfig } from './config.js'; import { defaultCredentialsPath } from './credentials.js'; -import { ApiError } from './errors.js'; +import { ApiError, localValidationError } from './errors.js'; import { facadeBaseUrl } from './facade.js'; import type { DebugEvent, FetchImpl } from './http.js'; import { @@ -139,6 +139,38 @@ function clampRequestTimeout(ms: number): number { return Math.min(Math.max(Math.round(ms), REQUEST_TIMEOUT_MIN_MS), REQUEST_TIMEOUT_MAX_MS); } +/** + * Parse the `--request-timeout ` flag value into milliseconds. + * + * Returns `undefined` when the flag was omitted (the factory then falls back to + * the `TESTSPRITE_REQUEST_TIMEOUT_MS` env var, else the 120s default). + * + * A supplied-but-invalid value (non-numeric, zero, or negative) throws a typed + * VALIDATION_ERROR (exit 5) rather than being silently dropped. An explicit + * `--request-timeout 30s` typo previously resolved to `undefined` and the + * command ran with the default 120s deadline — the operator believed they had + * set a timeout but had not, with no signal. Failing loudly here is consistent + * with every other validated flag (`--page-size`, `--output`, `--type`). + * + * Out-of-range but positive values are intentionally NOT rejected — they flow + * through to {@link resolveRequestTimeoutMs}, which clamps to + * `[REQUEST_TIMEOUT_MIN_MS, REQUEST_TIMEOUT_MAX_MS]`. The env-var path stays + * lenient by design (a stray global env var should not hard-fail every + * command); only the explicit per-invocation flag is strict. + * + * This single definition replaces five byte-identical copies that previously + * lived in `auth`, `project`, `usage`, `init`, and `test` — drift between them + * would have silently changed timeout behaviour depending on the command. + */ +export function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { + if (raw === undefined) return undefined; + const n = Number(raw); + if (!Number.isFinite(n) || n <= 0) { + throw localValidationError('request-timeout', 'must be a positive number of seconds'); + } + return Math.round(n * 1000); // seconds → milliseconds +} + export function makeHttpClient(opts: CommonOptions, deps: ClientFactoryDeps = {}): HttpClient { const stderr = deps.stderr ?? ((line: string) => process.stderr.write(`${line}\n`)); const env = deps.env ?? process.env; diff --git a/test/cli.subprocess.test.ts b/test/cli.subprocess.test.ts index 8cbfa59..3be4fbd 100644 --- a/test/cli.subprocess.test.ts +++ b/test/cli.subprocess.test.ts @@ -497,6 +497,24 @@ describe('project list subprocess', () => { const parsed = JSON.parse(result.stderr) as { error: { code: string } }; expect(parsed.error.code).toBe('VALIDATION_ERROR'); }, 30_000); + + it('--request-timeout 30s exits 5 (VALIDATION_ERROR), not a silent fallback to 120s', async () => { + // Previously an invalid flag value resolved to `undefined` and the command + // silently ran with the default 120s deadline — the operator believed they + // had set a timeout but had not. Now the explicit flag is validated like + // every other flag. + const result = await runCli( + ['--output', 'json', '--request-timeout', '30s', 'project', 'list'], + { + TESTSPRITE_API_KEY: 'sk-subproc', + TESTSPRITE_API_URL: baseUrl, + }, + ); + expect(result.exitCode).toBe(5); + const parsed = JSON.parse(result.stderr) as { error: { code: string; nextAction: string } }; + expect(parsed.error.code).toBe('VALIDATION_ERROR'); + expect(parsed.error.nextAction).toContain('request-timeout'); + }, 30_000); }); describe('project get subprocess', () => {