From 806a37fca87672ddd1ba82d65753d9fb82ff7e9c Mon Sep 17 00:00:00 2001 From: Alix-007 Date: Sat, 20 Jun 2026 03:33:24 +0800 Subject: [PATCH] fix(cli): reject present-but-invalid --timeout on status/health fast path (#92996) Merged via squash. Prepared head SHA: eda96f9f80f0af4e1a1b0fe97f971c7cb95fb845 Co-authored-by: Alix-007 <267018309+Alix-007@users.noreply.github.com> Co-authored-by: steipete <58493+steipete@users.noreply.github.com> Reviewed-by: @steipete --- src/cli/argv.test.ts | 36 ++++++++++++++++++++++--- src/cli/argv.ts | 21 +++++++++++---- src/cli/program/route-args.test.ts | 42 ++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 8 deletions(-) diff --git a/src/cli/argv.test.ts b/src/cli/argv.test.ts index 4a8d9fd29eb..e5c863d340a 100644 --- a/src/cli/argv.test.ts +++ b/src/cli/argv.test.ts @@ -561,6 +561,16 @@ describe("argv helpers", () => { argv: ["node", "openclaw", "--", "--timeout=99"], expected: undefined, }, + { + name: "repeated flag uses final value", + argv: ["node", "openclaw", "status", "--timeout", "100", "--timeout=200"], + expected: "200", + }, + { + name: "missing repeated value remains invalid", + argv: ["node", "openclaw", "status", "--timeout", "--timeout", "200"], + expected: null, + }, ])("extracts flag values: $name", ({ argv, expected }) => { expect(getFlagValue(argv, "--timeout")).toBe(expected); }); @@ -597,17 +607,37 @@ describe("argv helpers", () => { { name: "invalid integer", argv: ["node", "openclaw", "status", "--timeout", "nope"], - expected: undefined, + expected: null, }, { name: "non-decimal integer", argv: ["node", "openclaw", "status", "--timeout", "0x10"], - expected: undefined, + expected: null, }, { name: "partial integer", argv: ["node", "openclaw", "status", "--timeout", "5s"], - expected: undefined, + expected: null, + }, + { + name: "zero", + argv: ["node", "openclaw", "status", "--timeout", "0"], + expected: null, + }, + { + name: "negative integer", + argv: ["node", "openclaw", "status", "--timeout", "-5"], + expected: null, + }, + { + name: "repeated value uses final valid integer", + argv: ["node", "openclaw", "status", "--timeout", "nope", "--timeout", "5000"], + expected: 5000, + }, + { + name: "repeated value rejects final invalid integer", + argv: ["node", "openclaw", "status", "--timeout", "5000", "--timeout", "nope"], + expected: null, }, ])("parses positive integer flag values: $name", ({ argv, expected }) => { expect(getPositiveIntFlagValue(argv, "--timeout")).toBe(expected); diff --git a/src/cli/argv.ts b/src/cli/argv.ts index 50d00ed0ff0..81c309a92a6 100644 --- a/src/cli/argv.ts +++ b/src/cli/argv.ts @@ -403,6 +403,7 @@ export function normalizeRootLogLevelArgv( export function getFlagValue(argv: string[], name: string): string | null | undefined { const args = argv.slice(2); + let value: string | undefined; for (let i = 0; i < args.length; i += 1) { const arg = args[i]; if (arg === FLAG_TERMINATOR) { @@ -410,14 +411,22 @@ export function getFlagValue(argv: string[], name: string): string | null | unde } if (arg === name) { const next = args[i + 1]; - return isValueToken(next) ? next : null; + if (!isValueToken(next)) { + return null; + } + value = next; + i += 1; + continue; } if (arg.startsWith(`${name}=`)) { - const value = arg.slice(name.length + 1); - return value ? value : null; + const assigned = arg.slice(name.length + 1); + if (!assigned) { + return null; + } + value = assigned; } } - return undefined; + return value; } export function getVerboseFlag(argv: string[], options?: { includeDebug?: boolean }): boolean { @@ -435,7 +444,9 @@ export function getPositiveIntFlagValue(argv: string[], name: string): number | if (raw === null || raw === undefined) { return raw; } - return parsePositiveInt(raw); + // Keep absent distinct from present-but-invalid so route-first callers can + // defer invalid input to Commander instead of silently applying defaults. + return parsePositiveInt(raw) ?? null; } export function getCommandPathWithRootOptions(argv: string[], depth = 2): string[] { diff --git a/src/cli/program/route-args.test.ts b/src/cli/program/route-args.test.ts index 1a9e00fb670..7960b60ac7c 100644 --- a/src/cli/program/route-args.test.ts +++ b/src/cli/program/route-args.test.ts @@ -44,6 +44,48 @@ describe("route-args", () => { expect(parseStatusRouteArgs(["node", "openclaw", "status", "--timeout"])).toBeNull(); }); + it("defers status/health --timeout with a present-but-invalid value to Commander", () => { + // Regression: the route-first fast path used to silently accept invalid + // --timeout values (0, negative, non-numeric, unit-suffixed) and run with + // the default timeout, diverging from the full Commander path which rejects + // them with a non-zero exit. Returning null defers to Commander so both + // paths share the same validation. + for (const bad of ["0", "-5", "nope", "5s"]) { + expect(parseStatusRouteArgs(["node", "openclaw", "status", "--timeout", bad])).toBeNull(); + expect(parseHealthRouteArgs(["node", "openclaw", "health", "--timeout", bad])).toBeNull(); + } + expect( + parseStatusRouteArgs([ + "node", + "openclaw", + "status", + "--timeout", + "5000", + "--timeout", + "nope", + ]), + ).toBeNull(); + expect( + parseHealthRouteArgs([ + "node", + "openclaw", + "health", + "--timeout", + "nope", + "--timeout", + "5000", + ]), + ).toMatchObject({ timeoutMs: 5000 }); + // A valid positive integer still parses on the fast path. + expect(parseStatusRouteArgs(["node", "openclaw", "status", "--timeout", "5000"])).toMatchObject( + { timeoutMs: 5000 }, + ); + // No --timeout flag at all still uses the fast path (undefined timeout). + expect(parseStatusRouteArgs(["node", "openclaw", "status"])).toMatchObject({ + timeoutMs: undefined, + }); + }); + it("parses gateway status route args and rejects probe-only ssh flags", () => { expect( parseGatewayStatusRouteArgs([