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 a86b929..d1c8b8f 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 { @@ -7776,18 +7777,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 f9fdc44..186d18a 100644 --- a/src/lib/client-factory.test.ts +++ b/src/lib/client-factory.test.ts @@ -5,6 +5,7 @@ import { assertValidEndpointUrl, emitDryRunBanner, makeHttpClient, + parseRequestTimeoutFlag, resetDryRunBannerForTesting, resolveRequestTimeoutMs, } from './client-factory.js'; @@ -163,6 +164,34 @@ describe('makeHttpClient — getRunFailure dry-run path (M3.3 piece-4)', () => { // resolveRequestTimeoutMs — flag / env / default precedence // --------------------------------------------------------------------------- +describe('parseRequestTimeoutFlag', () => { + it('returns undefined when the flag is absent', () => { + expect(parseRequestTimeoutFlag(undefined)).toBeUndefined(); + }); + + it('converts valid whole seconds to milliseconds', () => { + expect(parseRequestTimeoutFlag('1')).toBe(1_000); + expect(parseRequestTimeoutFlag('120')).toBe(120_000); + expect(parseRequestTimeoutFlag('600')).toBe(600_000); + }); + + it.each(['abc', '0', '-1', '1.5', '601'])( + 'rejects invalid explicit flag value %s with VALIDATION_ERROR', + raw => { + expect(() => parseRequestTimeoutFlag(raw)).toThrow(ApiError); + try { + parseRequestTimeoutFlag(raw); + } catch (err) { + expect(err).toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + details: expect.objectContaining({ field: 'request-timeout' }), + }); + } + }, + ); +}); + describe('resolveRequestTimeoutMs', () => { it('returns the default when no flag or env var is set', () => { expect(resolveRequestTimeoutMs({}, {})).toBe(REQUEST_TIMEOUT_DEFAULT_MS); diff --git a/src/lib/client-factory.ts b/src/lib/client-factory.ts index fa1f8dc..d924d22 100644 --- a/src/lib/client-factory.ts +++ b/src/lib/client-factory.ts @@ -111,10 +111,10 @@ export function resetDryRunBannerForTesting(): void { * is in milliseconds to give scripts sub-second granularity and match the * typical convention for `*_MS` env vars. * - * Values are clamped to [REQUEST_TIMEOUT_MIN_MS, REQUEST_TIMEOUT_MAX_MS]; - * values below the minimum are raised to 1s, values above the maximum are - * lowered to 10m — both silently (no exit 5), since this is a tuning knob, - * not a safety-critical gate. + * Env values and direct factory callers are clamped to + * [REQUEST_TIMEOUT_MIN_MS, REQUEST_TIMEOUT_MAX_MS]. Explicit CLI flag values + * are parsed by {@link parseRequestTimeoutFlag} first, so invalid user input + * exits 5 instead of being silently rounded, clamped, or ignored. */ export function resolveRequestTimeoutMs( opts: Pick, @@ -135,6 +135,27 @@ export function resolveRequestTimeoutMs( return REQUEST_TIMEOUT_DEFAULT_MS; } +/** + * Parse the global `--request-timeout ` flag into milliseconds. + * + * The flag is user-facing and documented as seconds in the inclusive 1-600 + * range, so an explicit invalid value should fail fast instead of silently + * falling back to the default or being rounded/clamped. + */ +export function parseRequestTimeoutFlag(raw: string | undefined): number | undefined { + if (raw === undefined) return undefined; + const seconds = Number(raw); + const minSeconds = REQUEST_TIMEOUT_MIN_MS / 1000; + const maxSeconds = REQUEST_TIMEOUT_MAX_MS / 1000; + if (!Number.isInteger(seconds) || seconds < minSeconds || seconds > maxSeconds) { + throw localValidationError( + 'request-timeout', + `must be an integer between ${minSeconds} and ${maxSeconds} seconds`, + ); + } + return seconds * 1000; +} + function clampRequestTimeout(ms: number): number { return Math.min(Math.max(Math.round(ms), REQUEST_TIMEOUT_MIN_MS), REQUEST_TIMEOUT_MAX_MS); } diff --git a/test/cli.subprocess.test.ts b/test/cli.subprocess.test.ts index 95521a3..97fb3d0 100644 --- a/test/cli.subprocess.test.ts +++ b/test/cli.subprocess.test.ts @@ -497,6 +497,25 @@ 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 abc exits 5 instead of silently using the default', async () => { + const result = await runCli([ + '--output', + 'json', + 'project', + 'list', + '--dry-run', + '--request-timeout', + 'abc', + ]); + expect(result.exitCode).toBe(5); + const parsed = JSON.parse(result.stderr) as { + error: { code: string; nextAction: string; details: { field?: string } }; + }; + expect(parsed.error.code).toBe('VALIDATION_ERROR'); + expect(parsed.error.nextAction).toContain('request-timeout'); + expect(parsed.error.details.field).toBe('request-timeout'); + }, 30_000); }); describe('a malformed --profile is rejected (exit 5), not silently corrupting credentials', () => {