fix(cli): validate --request-timeout flag and de-duplicate its parser (#17)

`parseRequestTimeoutFlag` was copy-pasted byte-for-byte into five command
files (auth, project, usage, init, test). Every copy silently returned
`undefined` for an invalid value, so an explicit `--request-timeout 30s`
(a natural "30 seconds" typo) 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.

Hoist a single definition into client-factory.ts (next to
resolveRequestTimeoutMs and the REQUEST_TIMEOUT_* constants) and make the
flag strict: a non-numeric, zero, or negative value now throws a typed
VALIDATION_ERROR (exit 5), consistent with every other validated flag
(--page-size, --output, --type). Positive out-of-range values are still
accepted and clamped by resolveRequestTimeoutMs, and the
TESTSPRITE_REQUEST_TIMEOUT_MS env-var path stays lenient by design (a
stray global env var should not hard-fail every command).

Adds unit coverage for parseRequestTimeoutFlag and a subprocess
regression that `--request-timeout 30s` exits 5 instead of falling back
to 120s.

Author: Davidson3556

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';
@@ -337,19 +338,6 @@ function resolveCommonOptions(command: Command): CommonOptions {
   };
 }
 
-/**
- * Parse the `--request-timeout <seconds>` 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 });
 }

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, resolveOutputMode } from '../lib/output.js';
@@ -433,13 +436,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 TestSprite agent skills for your coding agent';
 

src/commands/project.ts

@@ -4,6 +4,7 @@ import { Command } from 'commander';
 import {
   emitDryRunBanner,
   makeHttpClient,
+  parseRequestTimeoutFlag,
   type CommonOptions as FactoryCommonOptions,
 } from '../lib/client-factory.js';
 import { ApiError } from '../lib/errors.js';
@@ -531,18 +532,6 @@ function resolveCommonOptions(command: Command): CommonOptions {
   };
 }
 
-/**
- * Parse the `--request-timeout <seconds>` 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,

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 {
@@ -7838,18 +7839,6 @@ function resolveCommonOptions(command: Command): CommonOptions {
   };
 }
 
-/**
- * Parse the `--request-timeout <seconds>` 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;
 

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 });
 }

src/lib/client-factory.test.ts

@@ -5,6 +5,7 @@ import {
   assertValidEndpointUrl,
   emitDryRunBanner,
   makeHttpClient,
+  parseRequestTimeoutFlag,
   resetDryRunBannerForTesting,
   resolveRequestTimeoutMs,
 } from './client-factory.js';
@@ -213,6 +214,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
 // ---------------------------------------------------------------------------

src/lib/client-factory.ts

@@ -180,6 +180,43 @@ export function assertValidEndpointUrl(rawUrl: string): void {
   }
 }
 
+/**
+ * Parse the `--request-timeout <seconds>` 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) {
+    // Surface the offending value in the message (same as assertValidEndpointUrl)
+    // so the operator sees exactly what they typed.
+    throw localValidationError(
+      'request-timeout',
+      `"${raw}" is not valid — 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;

test/cli.subprocess.test.ts

@@ -499,6 +499,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('malformed --endpoint-url is rejected (exit 5), not retried as a network error', () => {