From 5f7e21e26a91c62ea7d80d180c13176b494bc49e Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 27 May 2026 13:04:39 -0400 Subject: [PATCH] fix(cli): reject malformed timeout values --- src/cli/capability-cli.test.ts | 48 ++++++++++++++- src/cli/capability-cli.ts | 18 ++++-- src/cli/native-hook-relay-cli.test.ts | 58 +++++++++++++++++++ src/cli/native-hook-relay-cli.ts | 17 +++--- src/cli/program/helpers.test.ts | 5 +- src/cli/program/helpers.ts | 16 +---- .../channels.status.command-flow.test.ts | 10 ++++ src/commands/channels/capabilities.test.ts | 26 +++++++++ src/commands/channels/capabilities.ts | 11 +--- src/commands/channels/status.ts | 3 +- src/commands/sessions.test.ts | 20 +++++++ src/commands/sessions.ts | 5 +- 12 files changed, 195 insertions(+), 42 deletions(-) diff --git a/src/cli/capability-cli.test.ts b/src/cli/capability-cli.test.ts index 5f09d5d44f8..9a87b5e5500 100644 --- a/src/cli/capability-cli.test.ts +++ b/src/cli/capability-cli.test.ts @@ -1926,10 +1926,56 @@ describe("capability cli", () => { argv: ["capability", "image", "generate", "--prompt", "portrait", "--timeout-ms", "1000ms"], }), ).rejects.toThrow("exit 1"); - expectRuntimeErrorContains("--timeout-ms must be a finite number"); + expectRuntimeErrorContains("Invalid --timeout. Use a positive millisecond value"); expect(mocks.generateImage).not.toHaveBeenCalled(); }); + it.each([ + [ + "image generate", + ["capability", "image", "generate", "--prompt", "portrait", "--timeout-ms", "1000ms"], + ], + [ + "image edit", + [ + "capability", + "image", + "edit", + "--file", + "photo.png", + "--prompt", + "crop it", + "--timeout-ms", + "1000ms", + ], + ], + [ + "image describe", + ["capability", "image", "describe", "--file", "photo.png", "--timeout-ms", "1000ms"], + ], + [ + "image describe-many", + ["capability", "image", "describe-many", "--file", "photo.png", "--timeout-ms", "1000ms"], + ], + [ + "video generate", + ["capability", "video", "generate", "--prompt", "clip", "--timeout-ms", "1000ms"], + ], + ])("rejects malformed %s timeout before provider dispatch", async (_name, argv) => { + await expect( + runRegisteredCli({ + register: registerCapabilityCli as (program: Command) => void, + argv, + }), + ).rejects.toThrow("exit 1"); + + expectRuntimeErrorContains("Invalid --timeout. Use a positive millisecond value"); + expect(mocks.generateImage).not.toHaveBeenCalled(); + expect(mocks.generateVideo).not.toHaveBeenCalled(); + expect(mocks.describeImageFile).not.toHaveBeenCalled(); + expect(mocks.describeImageFileWithModel).not.toHaveBeenCalled(); + }); + it("routes audio transcribe through transcription, not realtime", async () => { await runRegisteredCli({ register: registerCapabilityCli as (program: Command) => void, diff --git a/src/cli/capability-cli.ts b/src/cli/capability-cli.ts index bf754d06dd8..731184882ef 100644 --- a/src/cli/capability-cli.ts +++ b/src/cli/capability-cli.ts @@ -106,6 +106,7 @@ import { getModelsCommandSecretTargetIds, getTtsCommandSecretTargetIds, } from "./command-secret-targets.js"; +import { parseTimeoutMsWithFallback } from "./parse-timeout.js"; import { removeCommandByName } from "./program/command-tree.js"; import { collectOption } from "./program/helpers.js"; @@ -1179,6 +1180,13 @@ function parseOptionalPositiveInteger(raw: unknown, label: string): number | und return value; } +function parseOptionalTimeoutMs(raw: string | number | undefined): number | undefined { + if (raw === undefined || (typeof raw === "string" && raw.trim() === "")) { + return undefined; + } + return parseTimeoutMsWithFallback(raw, 0, { invalidType: "error" }); +} + function normalizeImageOutputFormat( raw: string | undefined, ): ImageGenerationOutputFormat | undefined { @@ -1961,7 +1969,7 @@ export function registerCapabilityCli(program: Command) { opts.openaiBackground as string | undefined, "--openai-background", ), - timeoutMs: parseOptionalFiniteNumber(opts.timeoutMs, "--timeout-ms"), + timeoutMs: parseOptionalTimeoutMs(opts.timeoutMs), output: opts.output as string | undefined, }); emitJsonOrText(defaultRuntime, Boolean(opts.json), result, formatEnvelopeForText); @@ -2000,7 +2008,7 @@ export function registerCapabilityCli(program: Command) { opts.openaiBackground as string | undefined, "--openai-background", ), - timeoutMs: parseOptionalFiniteNumber(opts.timeoutMs, "--timeout-ms"), + timeoutMs: parseOptionalTimeoutMs(opts.timeoutMs), output: opts.output as string | undefined, }); emitJsonOrText(defaultRuntime, Boolean(opts.json), result, formatEnvelopeForText); @@ -2022,7 +2030,7 @@ export function registerCapabilityCli(program: Command) { files: [String(opts.file)], model: opts.model as string | undefined, prompt: opts.prompt as string | undefined, - timeoutMs: parseOptionalFiniteNumber(opts.timeoutMs, "--timeout-ms"), + timeoutMs: parseOptionalTimeoutMs(opts.timeoutMs), }); emitJsonOrText(defaultRuntime, Boolean(opts.json), result, formatEnvelopeForText); }); @@ -2043,7 +2051,7 @@ export function registerCapabilityCli(program: Command) { files: opts.file as string[], model: opts.model as string | undefined, prompt: opts.prompt as string | undefined, - timeoutMs: parseOptionalFiniteNumber(opts.timeoutMs, "--timeout-ms"), + timeoutMs: parseOptionalTimeoutMs(opts.timeoutMs), }); emitJsonOrText(defaultRuntime, Boolean(opts.json), result, formatEnvelopeForText); }); @@ -2352,7 +2360,7 @@ export function registerCapabilityCli(program: Command) { durationSeconds: parseOptionalFiniteNumber(opts.duration, "--duration"), audio: opts.audio === true ? true : undefined, watermark: opts.watermark === true ? true : undefined, - timeoutMs: parseOptionalFiniteNumber(opts.timeoutMs, "--timeout-ms"), + timeoutMs: parseOptionalTimeoutMs(opts.timeoutMs), }); emitJsonOrText(defaultRuntime, Boolean(opts.json), result, formatEnvelopeForText); }); diff --git a/src/cli/native-hook-relay-cli.test.ts b/src/cli/native-hook-relay-cli.test.ts index c4fbb86e624..8715e81fe63 100644 --- a/src/cli/native-hook-relay-cli.test.ts +++ b/src/cli/native-hook-relay-cli.test.ts @@ -79,6 +79,64 @@ describe("native hook relay CLI", () => { expect(stderr.text()).toBe("err"); }); + it("rejects malformed timeouts before reading relay input", async () => { + const invokeBridge = vi.fn(); + const callGateway = vi.fn(); + const stdout = createWritableTextBuffer(); + const stderr = createWritableTextBuffer(); + + const exitCode = await runNativeHookRelayCli( + { + provider: "codex", + relayId: "relay-1", + generation: "generation-1", + event: "pre_tool_use", + timeout: "5000ms", + }, + { + stdin: createReadableTextStream("{}"), + stdout, + stderr, + invokeBridge: invokeBridge as never, + callGateway: callGateway as never, + }, + ); + + expect(exitCode).toBe(1); + expect(stdout.text()).toBe(""); + expect(stderr.text()).toContain("invalid native hook timeout"); + expect(stderr.text()).toContain('Received: "5000ms"'); + expect(invokeBridge).not.toHaveBeenCalled(); + expect(callGateway).not.toHaveBeenCalled(); + }); + + it("rejects fractional timeouts before gateway fallback", async () => { + const invokeBridge = vi.fn(); + const callGateway = vi.fn(); + const stderr = createWritableTextBuffer(); + + const exitCode = await runNativeHookRelayCli( + { + provider: "codex", + relayId: "relay-1", + generation: "generation-1", + event: "pre_tool_use", + timeout: "1.5", + }, + { + stdin: createReadableTextStream("{}"), + stderr, + invokeBridge: invokeBridge as never, + callGateway: callGateway as never, + }, + ); + + expect(exitCode).toBe(1); + expect(stderr.text()).toContain('Received: "1.5"'); + expect(invokeBridge).not.toHaveBeenCalled(); + expect(callGateway).not.toHaveBeenCalled(); + }); + it("renders unavailable output for legacy relay commands without a generation", async () => { const invokeBridge = vi.fn(async () => { throw new Error("generation must be non-empty string"); diff --git a/src/cli/native-hook-relay-cli.ts b/src/cli/native-hook-relay-cli.ts index 1287007ba83..29651f4325a 100644 --- a/src/cli/native-hook-relay-cli.ts +++ b/src/cli/native-hook-relay-cli.ts @@ -7,6 +7,7 @@ import { } from "../agents/harness/native-hook-relay.js"; import { callGateway } from "../gateway/call.js"; import { ADMIN_SCOPE } from "../gateway/method-scopes.js"; +import { parseTimeoutMsWithFallback } from "./parse-timeout.js"; const MAX_NATIVE_HOOK_STDIN_BYTES = 1024 * 1024; @@ -39,6 +40,13 @@ export async function runNativeHookRelayCli( const relayId = readRequiredOption(opts.relayId, "relay-id"); const generation = opts.generation?.trim() || undefined; const event = readRequiredOption(opts.event, "event"); + let timeoutMs: number; + try { + timeoutMs = parseTimeoutMsWithFallback(opts.timeout, 5_000); + } catch (error) { + writeText(stderr, formatRelayCliError("invalid native hook timeout", error)); + return 1; + } let rawPayload: unknown; try { @@ -57,7 +65,7 @@ export async function runNativeHookRelayCli( event, rawPayload, registrationTimeoutMs: 100, - timeoutMs: normalizeTimeoutMs(opts.timeout), + timeoutMs, }); writeText(stdout, response.stdout); writeText(stderr, response.stderr); @@ -82,7 +90,7 @@ export async function runNativeHookRelayCli( const response = await callGatewayFn({ method: "nativeHook.invoke", params: { provider, relayId, generation, event, rawPayload }, - timeoutMs: normalizeTimeoutMs(opts.timeout), + timeoutMs, scopes: [ADMIN_SCOPE], }); writeText(stdout, response.stdout); @@ -122,11 +130,6 @@ async function readStreamText(stream: NodeJS.ReadableStream, maxBytes: number): return Buffer.concat(chunks, total).toString("utf8"); } -function normalizeTimeoutMs(value: string | undefined): number { - const parsed = Number(value); - return Number.isFinite(parsed) && parsed > 0 ? Math.floor(parsed) : 5_000; -} - function writeText(stream: NodeJS.WritableStream, value: string | undefined): void { if (value) { stream.write(value); diff --git a/src/cli/program/helpers.test.ts b/src/cli/program/helpers.test.ts index d40f606302f..f7892a71fa3 100644 --- a/src/cli/program/helpers.test.ts +++ b/src/cli/program/helpers.test.ts @@ -20,12 +20,13 @@ describe("program helpers", () => { { value: null, expected: undefined }, { value: "", expected: undefined }, { value: 5, expected: 5 }, - { value: 5.9, expected: 5 }, + { value: 5.9, expected: undefined }, { value: 0, expected: undefined }, { value: -1, expected: undefined }, { value: Number.NaN, expected: undefined }, { value: "10", expected: 10 }, - { value: "10ms", expected: 10 }, + { value: "10ms", expected: undefined }, + { value: "1.5", expected: undefined }, { value: "0", expected: undefined }, { value: "nope", expected: undefined }, { value: true, expected: undefined }, diff --git a/src/cli/program/helpers.ts b/src/cli/program/helpers.ts index 077ca54f3dc..e65895152c6 100644 --- a/src/cli/program/helpers.ts +++ b/src/cli/program/helpers.ts @@ -9,21 +9,7 @@ export function parsePositiveIntOrUndefined(value: unknown): number | undefined if (value === undefined || value === null || value === "") { return undefined; } - if (typeof value === "number") { - if (!Number.isFinite(value)) { - return undefined; - } - const parsed = Math.trunc(value); - return parsed > 0 ? parsed : undefined; - } - if (typeof value === "string") { - const parsed = Number.parseInt(value, 10); - if (Number.isNaN(parsed) || parsed <= 0) { - return undefined; - } - return parsed; - } - return undefined; + return parseStrictPositiveInteger(value); } export function parseStrictPositiveIntOrUndefined(value: unknown): number | undefined { diff --git a/src/commands/channels.status.command-flow.test.ts b/src/commands/channels.status.command-flow.test.ts index 8e2a28c9e38..719df809b23 100644 --- a/src/commands/channels.status.command-flow.test.ts +++ b/src/commands/channels.status.command-flow.test.ts @@ -227,6 +227,16 @@ describe("channelsStatusCommand SecretRef fallback flow", () => { }); }); + it("rejects malformed timeouts before gateway status requests", async () => { + const { runtime } = createCapturingTestRuntime(); + + await expect( + channelsStatusCommand({ channel: "imsg", timeout: "10s", probe: true }, runtime as never), + ).rejects.toThrow('Received: "10s"'); + + expect(mocks.callGateway).not.toHaveBeenCalled(); + }); + it("keeps read-only fallback output when SecretRefs are unresolved", async () => { mocks.callGateway.mockRejectedValue(new Error("gateway closed")); mocks.requireValidConfigSnapshot.mockResolvedValue({ secretResolved: false, channels: {} }); diff --git a/src/commands/channels/capabilities.test.ts b/src/commands/channels/capabilities.test.ts index 0ef548c81f9..d6896a8df90 100644 --- a/src/commands/channels/capabilities.test.ts +++ b/src/commands/channels/capabilities.test.ts @@ -178,6 +178,32 @@ describe("channelsCapabilitiesCommand", () => { ]); }); + it("rejects malformed timeouts before capability probes", async () => { + const probeAccount = vi.fn(async () => ({ ok: true })); + const plugin = buildPlugin({ + id: "slack", + account: { + accountId: "default", + botToken: "xoxb-bot", + }, + probe: { ok: true }, + }); + plugin.status = { ...plugin.status, probeAccount }; + vi.mocked(listChannelPlugins).mockReturnValue([plugin]); + vi.mocked(getChannelPlugin).mockReturnValue(plugin); + mocks.resolveInstallableChannelPlugin.mockResolvedValue({ + cfg: { channels: {} }, + channelId: "slack", + plugin, + configChanged: false, + }); + + await expect( + channelsCapabilitiesCommand({ channel: "slack", timeout: "10s" }, runtime), + ).rejects.toThrow('Received: "10s"'); + expect(probeAccount).not.toHaveBeenCalled(); + }); + it("prints Teams Graph permission hints when present", async () => { const plugin = buildPlugin({ id: "msteams", diff --git a/src/commands/channels/capabilities.ts b/src/commands/channels/capabilities.ts index 2249a3494d5..b74089474fe 100644 --- a/src/commands/channels/capabilities.ts +++ b/src/commands/channels/capabilities.ts @@ -12,6 +12,7 @@ import type { } from "../../channels/plugins/types.public.js"; import { formatCliCommand } from "../../cli/command-format.js"; import { formatUnknownChannelMessage } from "../../cli/error-format.js"; +import { parseTimeoutMsWithFallback } from "../../cli/parse-timeout.js"; import { commitConfigWithPendingPluginInstalls } from "../../cli/plugins-install-record-commit.js"; import { refreshPluginRegistryAfterConfigMutation } from "../../cli/plugins-registry-refresh.js"; import { @@ -51,14 +52,6 @@ type ChannelCapabilitiesReport = { diagnostics?: ChannelCapabilitiesDiagnostics; }; -function normalizeTimeout(raw: unknown, fallback = 10_000) { - const value = typeof raw === "string" ? Number(raw) : Number(raw); - if (!Number.isFinite(value) || value <= 0) { - return fallback; - } - return value; -} - function formatSupport(capabilities?: ChannelCapabilities) { if (!capabilities) { return "unknown"; @@ -226,7 +219,7 @@ export async function channelsCapabilitiesCommand( return; } let cfg = loadedCfg; - const timeoutMs = normalizeTimeout(opts.timeout, 10_000); + const timeoutMs = parseTimeoutMsWithFallback(opts.timeout, 10_000); const rawChannel = normalizeLowercaseStringOrEmpty(opts.channel); const rawTarget = normalizeOptionalString(opts.target) ?? ""; diff --git a/src/commands/channels/status.ts b/src/commands/channels/status.ts index ebf9bbdcbb3..9495b40f509 100644 --- a/src/commands/channels/status.ts +++ b/src/commands/channels/status.ts @@ -2,6 +2,7 @@ import { normalizeChannelId } from "../../channels/plugins/index.js"; import { resolveCommandConfigWithSecrets } from "../../cli/command-config-resolution.js"; import { formatCliCommand } from "../../cli/command-format.js"; import { getConfiguredChannelsCommandSecretTargetIds } from "../../cli/command-secret-targets.js"; +import { parseTimeoutMsWithFallback } from "../../cli/parse-timeout.js"; import { withProgress } from "../../cli/progress.js"; import { readConfigFileSnapshot } from "../../config/config.js"; import { callGateway } from "../../gateway/call.js"; @@ -214,7 +215,7 @@ export async function channelsStatusCommand( opts: ChannelsStatusOptions, runtime: RuntimeEnv = defaultRuntime, ) { - const timeoutMs = Number(opts.timeout ?? (opts.probe ? 30_000 : 10_000)); + const timeoutMs = parseTimeoutMsWithFallback(opts.timeout, opts.probe ? 30_000 : 10_000); const requestedChannel = opts.channel ? normalizeChannelId(opts.channel) : null; const statusLabel = opts.probe ? "Checking channel status (probe)…" : "Checking channel status…"; const shouldLogStatus = opts.json !== true && !process.stderr.isTTY; diff --git a/src/commands/sessions.test.ts b/src/commands/sessions.test.ts index 7d0990159d9..882295f2d27 100644 --- a/src/commands/sessions.test.ts +++ b/src/commands/sessions.test.ts @@ -353,6 +353,26 @@ describe("sessionsCommand", () => { fs.rmSync(store); }); + it("rejects partial --active values", async () => { + const store = writeStore( + { + demo: { + sessionId: "demo", + updatedAt: Date.now() - 5 * 60_000, + }, + }, + "sessions-active-partial", + ); + const { runtime, errors } = makeRuntime(); + + await expect(sessionsCommand({ store, active: "10m" }, runtime)).rejects.toThrow("exit 1"); + expect(errors).toStrictEqual([ + "--active must be a positive number of minutes, for example --active 30.", + ]); + + fs.rmSync(store); + }); + it("rejects invalid --limit values", async () => { const store = writeStore( { diff --git a/src/commands/sessions.ts b/src/commands/sessions.ts index b4afde57ee9..9f97f4cd803 100644 --- a/src/commands/sessions.ts +++ b/src/commands/sessions.ts @@ -7,6 +7,7 @@ import { loadSessionStore, resolveSessionTotalTokens } from "../config/sessions. import type { SessionEntry } from "../config/sessions/types.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { info } from "../globals.js"; +import { parseStrictPositiveInteger } from "../infra/parse-finite-number.js"; import { parseAgentSessionKey } from "../routing/session-key.js"; import { type RuntimeEnv, writeRuntimeJson } from "../runtime.js"; import { classifySessionKind, type SessionKind } from "../sessions/classify-session-kind.js"; @@ -329,8 +330,8 @@ export async function sessionsCommand( let activeMinutes: number | undefined; if (opts.active !== undefined) { - const parsed = Number.parseInt(opts.active, 10); - if (Number.isNaN(parsed) || parsed <= 0) { + const parsed = parseStrictPositiveInteger(opts.active); + if (parsed === undefined) { runtime.error("--active must be a positive number of minutes, for example --active 30."); runtime.exit(1); return;