From 5e1f920a36a24cff1e315e01ff94dae3318005a6 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 13 Jul 2026 05:30:37 -0700 Subject: [PATCH] refactor(cli): use Commander argv for lazy reparses (#106382) --- src/cli/argv.test.ts | 13 +- src/cli/argv.ts | 25 +--- src/cli/program/action-reparse.test.ts | 163 ++--------------------- src/cli/program/action-reparse.ts | 46 +------ src/cli/program/helpers.test.ts | 44 ------ src/cli/program/helpers.ts | 61 +-------- src/cli/program/register-lazy-command.ts | 8 +- 7 files changed, 33 insertions(+), 327 deletions(-) diff --git a/src/cli/argv.test.ts b/src/cli/argv.test.ts index 8ed41195bde5..caf7ed0490d6 100644 --- a/src/cli/argv.test.ts +++ b/src/cli/argv.test.ts @@ -658,21 +658,10 @@ describe("argv helpers", () => { expected: ["bun", "src/entry.ts", "status"], }, ] as const)("builds parse argv from raw args: $name", ({ rawArgs, expected }) => { - const parsed = buildParseArgv({ - programName: "openclaw", - rawArgs: [...rawArgs], - }); + const parsed = buildParseArgv([...rawArgs]); expect(parsed).toEqual([...expected]); }); - it("builds parse argv from fallback args", () => { - const fallbackArgv = buildParseArgv({ - programName: "openclaw", - fallbackArgv: ["status"], - }); - expect(fallbackArgv).toEqual(["node", "openclaw", "status"]); - }); - it.each([ { argv: ["node", "openclaw", "status"], expected: true }, { argv: ["node", "openclaw", "health"], expected: false }, diff --git a/src/cli/argv.ts b/src/cli/argv.ts index c6d70bae50d9..0e1412a5db9a 100644 --- a/src/cli/argv.ts +++ b/src/cli/argv.ts @@ -584,31 +584,20 @@ export function getCommandPositionalsWithRootOptions( return positionals; } -export function buildParseArgv(params: { - programName?: string; - rawArgs?: string[]; - fallbackArgv?: string[]; -}): string[] { - const baseArgv = - params.rawArgs && params.rawArgs.length > 0 - ? params.rawArgs - : params.fallbackArgv && params.fallbackArgv.length > 0 - ? params.fallbackArgv - : process.argv; - const programName = params.programName ?? ""; +export function buildParseArgv(rawArgs: string[], programName = "openclaw"): string[] { const normalizedArgv = - programName && baseArgv[0] === programName - ? baseArgv.slice(1) - : baseArgv[0]?.endsWith("openclaw") - ? baseArgv.slice(1) - : baseArgv; + rawArgs[0] === programName + ? rawArgs.slice(1) + : rawArgs[0]?.endsWith("openclaw") + ? rawArgs.slice(1) + : rawArgs; const looksLikeNode = normalizedArgv.length >= 2 && (isNodeRuntime(normalizedArgv[0] ?? "") || isBunRuntime(normalizedArgv[0] ?? "")); if (looksLikeNode) { return normalizedArgv; } - return ["node", programName || "openclaw", ...normalizedArgv]; + return ["node", programName, ...normalizedArgv]; } export function shouldMigrateStateFromPath(path: string[]): boolean { diff --git a/src/cli/program/action-reparse.test.ts b/src/cli/program/action-reparse.test.ts index d15ccd67ec7b..230ec844cabc 100644 --- a/src/cli/program/action-reparse.test.ts +++ b/src/cli/program/action-reparse.test.ts @@ -1,27 +1,12 @@ // Action reparse tests cover Commander action reparsing for nested CLI commands. import { Command } from "commander"; -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { reparseProgramFromActionArgs } from "./action-reparse.js"; - -const buildParseArgvMock = vi.hoisted(() => vi.fn()); -const resolveCommandOptionArgsMock = vi.hoisted(() => vi.fn()); - -vi.mock("../argv.js", () => ({ - buildParseArgv: buildParseArgvMock, -})); - -vi.mock("./helpers.js", () => ({ - resolveCommandOptionArgs: resolveCommandOptionArgsMock, -})); +import { describe, expect, it, vi } from "vitest"; +import { reparseProgramFromActionCommand } from "./action-reparse.js"; function setRawArgs(command: Command, rawArgs: string[]): void { (command as Command & { rawArgs: string[] }).rawArgs = rawArgs; } -function deleteRawArgs(command: Command): void { - delete (command as Command & { rawArgs?: string[] }).rawArgs; -} - async function expectReparseArgv(params: { parent: Command; action: Command; @@ -33,157 +18,37 @@ async function expectReparseArgv(params: { root = root.parent; } setRawArgs(root, params.argv); - buildParseArgvMock.mockReturnValue(params.argv); const parseAsync = vi.spyOn(root, "parseAsync").mockResolvedValue(root); - await reparseProgramFromActionArgs(params.parent, [params.action]); + await reparseProgramFromActionCommand(params.parent, params.action); expect(parseAsync).toHaveBeenCalledWith(params.expected); } -describe("reparseProgramFromActionArgs", () => { - beforeEach(() => { - vi.clearAllMocks(); - buildParseArgvMock.mockReturnValue(["node", "openclaw", "status"]); - resolveCommandOptionArgsMock.mockReturnValue([]); - }); - - it("uses action command name + args as fallback argv", async () => { - const program = new Command().name("openclaw"); - setRawArgs(program, ["node", "openclaw", "status", "--json"]); - const parseAsync = vi.spyOn(program, "parseAsync").mockResolvedValue(program); - const actionCommand = { - name: () => "status", - parent: program, - args: ["--json"], - } as unknown as Command; - - await reparseProgramFromActionArgs(program, [actionCommand]); - - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: ["node", "openclaw", "status", "--json"], - fallbackArgv: ["status", "--json"], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); - }); - - it("falls back to action args without command name when action has no name", async () => { - const program = new Command().name("openclaw"); - setRawArgs(program, ["node", "openclaw"]); - const parseAsync = vi.spyOn(program, "parseAsync").mockResolvedValue(program); - const actionCommand = { - name: () => "", - parent: program, - args: ["--json"], - } as unknown as Command; - - await reparseProgramFromActionArgs(program, [actionCommand]); - - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: ["node", "openclaw"], - fallbackArgv: ["--json"], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); - }); - - it("preserves explicit parent command options in fallback argv", async () => { - const program = new Command().name("browser"); - const parseAsync = vi.spyOn(program, "parseAsync").mockResolvedValue(program); - const actionCommand = { - name: () => "open", - parent: program, - args: ["about:blank"], - } as unknown as Command; - resolveCommandOptionArgsMock.mockReturnValue(["--json"]); - - await reparseProgramFromActionArgs(program, [actionCommand]); - - expect(resolveCommandOptionArgsMock).toHaveBeenCalledWith(program); - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "browser", - rawArgs: [], - fallbackArgv: ["--json", "open", "about:blank"], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); - }); - +describe("reparseProgramFromActionCommand", () => { it("uses root raw args and reparses the root for nested lazy commands", async () => { const root = new Command().name("openclaw"); setRawArgs(root, ["node", "openclaw", "workspaces", "audit", "export", "--since", "1"]); const workspaces = root.command("workspaces"); const audit = workspaces.command("audit"); const exportCommand = audit.command("export"); - exportCommand.args = ["--since", "1"]; const parseAsync = vi.spyOn(root, "parseAsync").mockResolvedValue(root); const auditParseAsync = vi.spyOn(audit, "parseAsync"); - await reparseProgramFromActionArgs(audit, [exportCommand]); + await reparseProgramFromActionCommand(audit, exportCommand); - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: ["node", "openclaw", "workspaces", "audit", "export", "--since", "1"], - fallbackArgv: ["workspaces", "audit", "export", "--since", "1"], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); + expect(parseAsync).toHaveBeenCalledWith([ + "node", + "openclaw", + "workspaces", + "audit", + "export", + "--since", + "1", + ]); expect(auditParseAsync).not.toHaveBeenCalled(); }); - it("reconstructs the full nested command path when Commander rawArgs is missing", async () => { - // #83893: nested lazy commands still need their ancestor path if - // Commander stops exposing root rawArgs at runtime. - const root = new Command().name("openclaw"); - const workspaces = root.command("workspaces"); - const audit = workspaces.command("audit"); - const exportCommand = audit.command("export"); - exportCommand.args = ["--since", "1"]; - deleteRawArgs(root); - const parseAsync = vi.spyOn(root, "parseAsync").mockResolvedValue(root); - - await reparseProgramFromActionArgs(audit, [exportCommand]); - - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: undefined, - fallbackArgv: ["workspaces", "audit", "export", "--since", "1"], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); - }); - - it("uses program root when action command is missing", async () => { - const program = new Command().name("openclaw"); - const parseAsync = vi.spyOn(program, "parseAsync").mockResolvedValue(program); - - await reparseProgramFromActionArgs(program, []); - - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: [], - fallbackArgv: [], - }); - expect(parseAsync).toHaveBeenCalledWith(["node", "openclaw", "status"]); - }); - - it("falls back to fallbackArgv when Commander rawArgs is missing from the root command", async () => { - // #83893: rawArgs is a Commander runtime field, so the root command must - // still reparse from reconstructed argv if Commander stops exposing it. - const root = new Command().name("openclaw"); - const configCommand = root.command("config"); - configCommand.args = ["set", "key", "value"]; - deleteRawArgs(root); - const parseAsync = vi.spyOn(root, "parseAsync").mockResolvedValue(root); - - await reparseProgramFromActionArgs(root, [configCommand]); - - expect(buildParseArgvMock).toHaveBeenCalledWith({ - programName: "openclaw", - rawArgs: undefined, - fallbackArgv: ["config", "set", "key", "value"], - }); - expect(parseAsync).toHaveBeenCalled(); - }); - it("hoists a trailing lazy-parent option before the loaded command", async () => { const root = new Command().name("openclaw"); const browser = root.command("browser").option("--browser-profile "); diff --git a/src/cli/program/action-reparse.ts b/src/cli/program/action-reparse.ts index 17d38381603e..69d0f5d12301 100644 --- a/src/cli/program/action-reparse.ts +++ b/src/cli/program/action-reparse.ts @@ -1,8 +1,6 @@ -import { expectDefined } from "@openclaw/normalization-core"; // Reparse support for lazy commands after their placeholder has been replaced. import type { Command, Option } from "commander"; import { buildParseArgv } from "../argv.js"; -import { resolveCommandOptionArgs } from "./helpers.js"; function getCommandPathFromRoot(command: Command | undefined): Command[] { const path: Command[] = []; @@ -16,25 +14,6 @@ function getCommandPathFromRoot(command: Command | undefined): Command[] { return path; } -function buildFallbackArgv(program: Command, actionCommand: Command | undefined): string[] { - const actionArgsList = actionCommand?.args ?? []; - const parentOptionArgs = - actionCommand?.parent === program ? resolveCommandOptionArgs(program) : []; - const commandPath = getCommandPathFromRoot(actionCommand).map((command) => command.name()); - if (commandPath.length === 0) { - return [...parentOptionArgs, ...actionArgsList]; - } - return [ - ...commandPath.slice(0, -1), - ...parentOptionArgs, - expectDefined( - commandPath[commandPath.length - 1], - "command path entry at command path.length 1", - ), - ...actionArgsList, - ]; -} - function findRootCommand(cmd: Command): Command { let current: Command = cmd; while (current.parent) { @@ -188,25 +167,14 @@ function hoistLazyParentOptions( : [...argv.slice(0, lazyCommandIndex), ...hoisted, lazyCommandName, ...remaining]; } -/** Rebuild argv from Commander action args and re-run parsing after lazy registration. */ -export async function reparseProgramFromActionArgs( +/** Re-run parsing after replacing a lazy command placeholder. */ +export async function reparseProgramFromActionCommand( program: Command, - actionArgs: unknown[], + actionCommand: Command, ): Promise { - const actionCommand = actionArgs.at(-1) as Command | undefined; - // Use the true root program for argv reconstruction and parsing. - // Commander keeps rawArgs as a JS runtime field, not a typed API; if a - // future version removes it, buildParseArgv falls back to reconstructed argv. - const rootProgram = findRootCommand(actionCommand ?? program); - const rawArgs = (rootProgram as Command & { rawArgs?: string[] }).rawArgs; - const fallbackArgv = buildFallbackArgv(program, actionCommand); - const parseArgv = buildParseArgv({ - programName: rootProgram.name(), - rawArgs, - fallbackArgv, - }); - const normalizedArgv = actionCommand - ? hoistLazyParentOptions(parseArgv, program, actionCommand.name()) - : parseArgv; + const rootProgram = findRootCommand(actionCommand) as Command & { rawArgs: string[] }; + // Commander 15 snapshots the full parse input on the root before actions run. + const parseArgv = buildParseArgv(rootProgram.rawArgs, rootProgram.name()); + const normalizedArgv = hoistLazyParentOptions(parseArgv, program, actionCommand.name()); await rootProgram.parseAsync(normalizedArgv); } diff --git a/src/cli/program/helpers.test.ts b/src/cli/program/helpers.test.ts index cc86700733fd..13570161cd37 100644 --- a/src/cli/program/helpers.test.ts +++ b/src/cli/program/helpers.test.ts @@ -1,12 +1,10 @@ // Program helper tests cover shared command registration and help helpers. -import { Command } from "commander"; import { describe, expect, it } from "vitest"; import { collectOption, parsePositiveIntOrUndefined, parseStrictPositiveIntOption, parseStrictPositiveIntOrUndefined, - resolveCommandOptionArgs, } from "./helpers.js"; describe("program helpers", () => { @@ -61,46 +59,4 @@ describe("program helpers", () => { "--limit must be a positive integer.", ); }); - - it("resolveCommandOptionArgs serializes explicit options", () => { - const command = new Command() - .option("--json", "JSON output", false) - .option("--timeout ", "Timeout", "30000") - .option("--tag ", "Tag", collectOption) - .option("--no-progress", "Disable progress"); - - command.parse([ - "node", - "test", - "--json", - "--timeout", - "10", - "--tag", - "a", - "--tag", - "b", - "--no-progress", - ]); - - expect(resolveCommandOptionArgs(command)).toEqual([ - "--json", - "--timeout", - "10", - "--tag", - "a", - "--tag", - "b", - "--no-progress", - ]); - }); - - it("resolveCommandOptionArgs skips defaults", () => { - const command = new Command() - .option("--json", "JSON output", false) - .option("--timeout ", "Timeout", "30000"); - - command.parse(["node", "test"]); - - expect(resolveCommandOptionArgs(command)).toStrictEqual([]); - }); }); diff --git a/src/cli/program/helpers.ts b/src/cli/program/helpers.ts index 5562ba2079e7..ad8aae2c5c7e 100644 --- a/src/cli/program/helpers.ts +++ b/src/cli/program/helpers.ts @@ -1,5 +1,5 @@ -// Shared Commander registration helpers for repeated options, positive ints, and lazy reparse args. -import { InvalidArgumentError, type Command } from "commander"; +// Shared Commander registration helpers for repeated options and positive integers. +import { InvalidArgumentError } from "commander"; import { parseStrictPositiveInteger } from "../../infra/parse-finite-number.js"; /** Commander option collector for repeatable string flags. */ @@ -28,60 +28,3 @@ export function parseStrictPositiveIntOption(value: string, flag: string): numbe } return parsed; } - -function appendOptionValue(out: string[], flag: string, value: unknown): void { - if (value === undefined) { - return; - } - if (value === false) { - if (flag.startsWith("--no-")) { - out.push(flag); - } - return; - } - if (value === true) { - out.push(flag); - return; - } - const arg = stringifyOptionValue(value); - if (arg !== undefined) { - out.push(flag, arg); - } -} - -function stringifyOptionValue(value: unknown): string | undefined { - if (typeof value === "string") { - return value; - } - if (typeof value === "number" && Number.isFinite(value)) { - return String(value); - } - if (typeof value === "bigint") { - return value.toString(); - } - return undefined; -} - -/** Reconstruct explicit option tokens from a Commander command for lazy reparsing. */ -export function resolveCommandOptionArgs(command: Command): string[] { - const out: string[] = []; - for (const option of command.options) { - const name = option.attributeName(); - if (command.getOptionValueSource(name) === "default") { - continue; - } - const flag = option.long ?? option.short; - if (!flag) { - continue; - } - const value = command.getOptionValue(name); - if (Array.isArray(value)) { - for (const item of value) { - appendOptionValue(out, flag, item); - } - continue; - } - appendOptionValue(out, flag, value); - } - return out; -} diff --git a/src/cli/program/register-lazy-command.ts b/src/cli/program/register-lazy-command.ts index 8a4654b93b95..3c25277293e6 100644 --- a/src/cli/program/register-lazy-command.ts +++ b/src/cli/program/register-lazy-command.ts @@ -1,8 +1,7 @@ // Lazy Commander placeholder registration used to keep CLI startup imports small. import type { Command } from "commander"; -import { reparseProgramFromActionArgs } from "./action-reparse.js"; +import { reparseProgramFromActionCommand } from "./action-reparse.js"; import { removeCommandByName } from "./command-tree.js"; -import { resolveCommandOptionArgs } from "./helpers.js"; type RegisterLazyCommandParams = { program: Command; @@ -32,13 +31,10 @@ export function registerLazyCommand({ placeholder.allowUnknownOption(true).allowExcessArguments(true); placeholder.action(async (...actionArgs) => { const actionCommand = actionArgs.at(-1) as Command; - // Commander separates option values from positional args on placeholders; restore them - // before reparsing so the real command sees the original token order. - actionCommand.args = [...resolveCommandOptionArgs(actionCommand), ...actionCommand.args]; for (const commandName of new Set(removeNames ?? [name])) { removeCommandByName(program, commandName); } await register(); - await reparseProgramFromActionArgs(program, actionArgs); + await reparseProgramFromActionCommand(program, actionCommand); }); }