From 2e04f762ea5647dbbf2193f71af96cdf0fd7b2ea Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 23 Aug 2026 00:54:06 -0700 Subject: [PATCH] fix(cli): render task validation JSON failures (#127750) --- src/cli/program/register.tasks.test.ts | 37 ++++++--- src/cli/program/register.tasks.ts | 62 ++++----------- test/cli-json-stdout.e2e.test.ts | 101 +++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 59 deletions(-) diff --git a/src/cli/program/register.tasks.test.ts b/src/cli/program/register.tasks.test.ts index e1d87363e765..be1f675f8f8e 100644 --- a/src/cli/program/register.tasks.test.ts +++ b/src/cli/program/register.tasks.test.ts @@ -2,6 +2,7 @@ import { Command } from "commander"; import { createRequireRecord } from "openclaw/plugin-sdk/test-fixtures"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { ExpectedCliError } from "../failure-output.js"; import { registerTasksCommand } from "./register.tasks.js"; const mocks = vi.hoisted(() => ({ @@ -82,10 +83,14 @@ describe("registerTasksCommand", () => { }); it("rejects inherited mutation options before loading task or flow owners", async () => { - await runCli(["tasks", "--json", "cancel", "task-123"]); + await expect(runCli(["tasks", "--json", "cancel", "task-123"])).rejects.toBeInstanceOf( + ExpectedCliError, + ); expect(mocks.tasksModuleLoaded).not.toHaveBeenCalled(); - await runCli(["tasks", "--json", "flow", "cancel", "flow-123"]); + await expect(runCli(["tasks", "--json", "flow", "cancel", "flow-123"])).rejects.toBeInstanceOf( + ExpectedCliError, + ); expect(mocks.flowsModuleLoaded).not.toHaveBeenCalled(); }); @@ -214,12 +219,14 @@ describe("registerTasksCommand", () => { }); it("rejects partially numeric task audit limits before owner action", async () => { - await runCli(["tasks", "audit", "--limit", "5abc"]); + const execution = runCli(["tasks", "audit", "--limit", "5abc"]); - expect(mocks.runtime.error).toHaveBeenCalledWith( + await expect(execution).rejects.toBeInstanceOf(ExpectedCliError); + await expect(execution).rejects.toThrow( "--limit must be a positive integer, for example --limit 25.", ); - expect(mocks.runtime.exit).toHaveBeenCalledWith(1); + expect(mocks.runtime.error).not.toHaveBeenCalled(); + expect(mocks.runtime.exit).not.toHaveBeenCalled(); expect(mocks.tasksAuditCommand).not.toHaveBeenCalled(); }); @@ -280,10 +287,12 @@ describe("registerTasksCommand", () => { flag: "--json", }, ])("rejects $label before owner action", async ({ args, flag }) => { - await runCli(args); + const execution = runCli(args); - expect(mocks.runtime.error).toHaveBeenCalledWith(expect.stringContaining(flag)); - expect(mocks.runtime.exit).toHaveBeenCalledWith(1); + await expect(execution).rejects.toBeInstanceOf(ExpectedCliError); + await expect(execution).rejects.toMatchObject({ message: expect.stringContaining(flag) }); + expect(mocks.runtime.error).not.toHaveBeenCalled(); + expect(mocks.runtime.exit).not.toHaveBeenCalled(); for (const handler of ownerHandlers) { expect(handler).not.toHaveBeenCalled(); } @@ -299,9 +308,10 @@ describe("registerTasksCommand", () => { error: "`tasks cancel` does not support inherited options --json, --runtime.", }, ])("lists only explicitly supplied unsupported flags", async ({ args, error }) => { - await runCli(args); + await expect(runCli(args)).rejects.toThrow(error); - expect(mocks.runtime.error).toHaveBeenCalledWith(error); + expect(mocks.runtime.error).not.toHaveBeenCalled(); + expect(mocks.runtime.exit).not.toHaveBeenCalled(); for (const handler of ownerHandlers) { expect(handler).not.toHaveBeenCalled(); } @@ -320,11 +330,14 @@ describe("registerTasksCommand", () => { }); it("rejects an invalid notify policy before owner action", async () => { - await runCli(["tasks", "notify", "run-123", "sometimes"]); + const execution = runCli(["tasks", "notify", "run-123", "sometimes"]); - expect(mocks.runtime.error).toHaveBeenCalledWith( + await expect(execution).rejects.toBeInstanceOf(ExpectedCliError); + await expect(execution).rejects.toThrow( "Notify policy must be done_only, state_changes, or silent.", ); + expect(mocks.runtime.error).not.toHaveBeenCalled(); + expect(mocks.runtime.exit).not.toHaveBeenCalled(); expect(mocks.tasksNotifyCommand).not.toHaveBeenCalled(); }); diff --git a/src/cli/program/register.tasks.ts b/src/cli/program/register.tasks.ts index 104e2edafba0..a957bd2dc7c1 100644 --- a/src/cli/program/register.tasks.ts +++ b/src/cli/program/register.tasks.ts @@ -15,6 +15,7 @@ import { import { runCommandWithRuntime } from "../cli-utils.js"; import { inheritOptionFromParent } from "../command-options.js"; import { parseCliEnumFilter } from "../enum-filter.js"; +import { ExpectedCliError } from "../failure-output.js"; type TasksParentOption = "json" | "runtime" | "status"; const TASKS_PARENT_OPTIONS = ["json", "runtime", "status"] as const; @@ -56,21 +57,23 @@ function isTaskNotifyPolicy(value: unknown): value is TaskNotifyPolicy { return value === "done_only" || value === "state_changes" || value === "silent"; } +function throwTasksCliError(message: string): never { + throw new ExpectedCliError({ message, humanOutput: message, machineOutput: message }); +} + function resolveTasksLeafOptions( command: Command, leaf: TasksLeaf, -): { json?: boolean; runtime?: string; status?: string } | undefined { +): { json?: boolean; runtime?: string; status?: string } { const supported: readonly TasksParentOption[] = TASKS_LEAF_OPTION_SUPPORT[leaf]; const flags = TASKS_PARENT_OPTIONS.filter( (name) => !supported.includes(name) && inheritOptionFromParent(command, name, "cli") !== undefined, ).map((name) => `--${name}`); if (flags.length > 0) { - defaultRuntime.error( + throwTasksCliError( `\`tasks ${leaf}\` does not support inherited ${flags.length === 1 ? "option" : "options"} ${flags.join(", ")}.`, ); - defaultRuntime.exit(1); - return undefined; } const resolveLocal = (name: TasksParentOption): unknown => { @@ -89,12 +92,10 @@ function resolveTasksLeafOptions( }; } -function parseTasksAuditLimit(limit: unknown): number | null | undefined { +function parseTasksAuditLimit(limit: unknown): number | undefined { const parsed = parseStrictPositiveInteger(limit); if (limit !== undefined && parsed === undefined) { - defaultRuntime.error("--limit must be a positive integer, for example --limit 25."); - defaultRuntime.exit(1); - return null; + throwTasksCliError("--limit must be a positive integer, for example --limit 25."); } return parsed; } @@ -119,9 +120,6 @@ export function registerTasksCommand(program: Command): void { addTasksListOptions(tasksCmd.command("list").description("List tracked background tasks")).action( async (_opts, command) => { const resolved = resolveTasksLeafOptions(command, "list"); - if (!resolved) { - return; - } await runOwner(loadTasksCommands, ({ tasksListCommand }) => tasksListCommand( { @@ -144,13 +142,7 @@ export function registerTasksCommand(program: Command): void { .option("--limit ", "Limit displayed findings") .action(async (opts, command) => { const resolved = resolveTasksLeafOptions(command, "audit"); - if (!resolved) { - return; - } const limit = parseTasksAuditLimit(opts.limit); - if (limit === null) { - return; - } await runOwner(loadTasksCommands, ({ tasksAuditCommand }) => tasksAuditCommand( { @@ -171,9 +163,6 @@ export function registerTasksCommand(program: Command): void { .option("--apply", "Apply reconciliation, cleanup stamping, and pruning", false) .action(async (opts, command) => { const resolved = resolveTasksLeafOptions(command, "maintenance"); - if (!resolved) { - return; - } await runOwner(loadTasksCommands, ({ tasksMaintenanceCommand }) => tasksMaintenanceCommand( { json: Boolean(resolved.json), apply: Boolean(opts.apply) }, @@ -189,9 +178,6 @@ export function registerTasksCommand(program: Command): void { .option("--json", "Output as JSON", false) .action(async (lookup, _opts, command) => { const resolved = resolveTasksLeafOptions(command, "show"); - if (!resolved) { - return; - } await runOwner(loadTasksCommands, ({ tasksShowCommand }) => tasksShowCommand({ lookup, json: Boolean(resolved.json) }, defaultRuntime), ); @@ -203,13 +189,9 @@ export function registerTasksCommand(program: Command): void { .argument("", "Task id, run id, or session key") .argument("", "Notify policy (done_only, state_changes, silent)") .action(async (lookup, notify, _opts, command) => { - if (!resolveTasksLeafOptions(command, "notify")) { - return; - } + resolveTasksLeafOptions(command, "notify"); if (!isTaskNotifyPolicy(notify)) { - defaultRuntime.error("Notify policy must be done_only, state_changes, or silent."); - defaultRuntime.exit(1); - return; + throwTasksCliError("Notify policy must be done_only, state_changes, or silent."); } await runOwner(loadTasksCommands, ({ tasksNotifyCommand }) => tasksNotifyCommand({ lookup, notify }, defaultRuntime), @@ -221,9 +203,7 @@ export function registerTasksCommand(program: Command): void { .description("Cancel a running background task") .argument("", "Task id, run id, or session key") .action(async (lookup, _opts, command) => { - if (!resolveTasksLeafOptions(command, "cancel")) { - return; - } + resolveTasksLeafOptions(command, "cancel"); await runOwner(loadTasksCommands, ({ tasksCancelCommand }) => tasksCancelCommand({ lookup }, defaultRuntime), ); @@ -233,9 +213,7 @@ export function registerTasksCommand(program: Command): void { .command("retry ") .description("Retry delivery for up to 10 blocked subagent completions") .action(async (lookups: string[], _opts, command) => { - if (!resolveTasksLeafOptions(command, "retry")) { - return; - } + resolveTasksLeafOptions(command, "retry"); await runOwner(loadTasksCommands, ({ tasksRetryCommand }) => tasksRetryCommand({ lookups }, defaultRuntime), ); @@ -245,9 +223,7 @@ export function registerTasksCommand(program: Command): void { .command("dismiss ") .description("Dismiss delivery for up to 10 blocked subagent completions") .action(async (lookups: string[], _opts, command) => { - if (!resolveTasksLeafOptions(command, "dismiss")) { - return; - } + resolveTasksLeafOptions(command, "dismiss"); await runOwner(loadTasksCommands, ({ tasksDismissCommand }) => tasksDismissCommand({ lookups }, defaultRuntime), ); @@ -266,9 +242,6 @@ export function registerTasksCommand(program: Command): void { .option("--status ", `Filter by status (${TASK_FLOW_STATUSES.join(", ")})`) .action(async (_opts, command) => { const resolved = resolveTasksLeafOptions(command, "flow list"); - if (!resolved) { - return; - } await runOwner(loadFlowsCommands, ({ flowsListCommand }) => flowsListCommand({ json: Boolean(resolved.json), status: resolved.status }, defaultRuntime), ); @@ -281,9 +254,6 @@ export function registerTasksCommand(program: Command): void { .option("--json", "Output as JSON", false) .action(async (lookup, _opts, command) => { const resolved = resolveTasksLeafOptions(command, "flow show"); - if (!resolved) { - return; - } await runOwner(loadFlowsCommands, ({ flowsShowCommand }) => flowsShowCommand({ lookup, json: Boolean(resolved.json) }, defaultRuntime), ); @@ -294,9 +264,7 @@ export function registerTasksCommand(program: Command): void { .description("Cancel a running TaskFlow") .argument("", "Flow id or owner key") .action(async (lookup, _opts, command) => { - if (!resolveTasksLeafOptions(command, "flow cancel")) { - return; - } + resolveTasksLeafOptions(command, "flow cancel"); await runOwner(loadFlowsCommands, ({ flowsCancelCommand }) => flowsCancelCommand({ lookup }, defaultRuntime), ); diff --git a/test/cli-json-stdout.e2e.test.ts b/test/cli-json-stdout.e2e.test.ts index e89289ea68ee..c76ad27eb314 100644 --- a/test/cli-json-stdout.e2e.test.ts +++ b/test/cli-json-stdout.e2e.test.ts @@ -371,6 +371,107 @@ describe("cli json stdout contract", () => { ); }); + it.each([ + { + name: "audit limit in human mode", + args: ["tasks", "audit", "--limit", "5abc"], + message: "--limit must be a positive integer, for example --limit 25.", + human: true, + }, + { + name: "notify policy in human mode", + args: ["tasks", "notify", "task-123", "sometimes"], + message: "Notify policy must be done_only, state_changes, or silent.", + human: true, + }, + { + name: "routed audit limit with leaf JSON", + args: ["tasks", "audit", "--json", "--limit", "5abc"], + message: "--limit must be a positive integer, for example --limit 25.", + }, + { + name: "routed audit limit with parent JSON", + args: ["tasks", "--json", "audit", "--limit", "5abc"], + message: "--limit must be a positive integer, for example --limit 25.", + }, + { + name: "Commander audit limit with leaf JSON", + args: ["tasks", "audit", "--limit", "5abc", "--json"], + message: "--limit must be a positive integer, for example --limit 25.", + commander: true, + }, + { + name: "Commander audit limit with parent JSON", + args: ["tasks", "--json", "audit", "--limit", "5abc"], + message: "--limit must be a positive integer, for example --limit 25.", + commander: true, + }, + { + name: "routed audit with an inherited runtime", + args: ["tasks", "--json", "--runtime", "cli", "audit"], + message: "`tasks audit` does not support inherited option --runtime.", + }, + { + name: "Commander audit with an inherited status", + args: ["tasks", "--json", "--status", "running", "audit"], + message: "`tasks audit` does not support inherited option --status.", + commander: true, + }, + { + name: "routed maintenance with an inherited runtime", + args: ["tasks", "--runtime", "cli", "maintenance", "--json"], + message: "`tasks maintenance` does not support inherited option --runtime.", + }, + { + name: "routed TaskFlow list with an inherited task status", + args: ["tasks", "--json", "--status", "running", "flow", "list"], + message: "`tasks flow list` does not support inherited option --status.", + }, + { + name: "Commander TaskFlow show with an inherited runtime", + args: ["tasks", "--runtime", "cli", "flow", "--json", "show", "flow-123"], + message: "`tasks flow show` does not support inherited option --runtime.", + commander: true, + }, + { + name: "routed audit limit through dual-TTY finalization", + args: ["tasks", "audit", "--json", "--limit", "5abc"], + message: "--limit must be a positive integer, for example --limit 25.", + tty: true, + }, + ])("renders task registration validation failures for $name", async (testCase) => { + await withTempHome( + async (tempHome) => { + const preload = `data:text/javascript,${encodeURIComponent( + 'Object.defineProperty(process.stdout, "isTTY", { value: true, configurable: true }); Object.defineProperty(process.stderr, "isTTY", { value: true, configurable: true });', + )}`; + const result = runBuiltCli(tempHome, testCase.args, { + OPENCLAW_STATE_DIR: path.join(tempHome, "isolated-state"), + OPENCLAW_CONFIG_PATH: path.join(tempHome, "missing-openclaw.json"), + ...("commander" in testCase ? { OPENCLAW_DISABLE_ROUTE_FIRST: "1" } : {}), + ...("tty" in testCase ? { NODE_OPTIONS: `--import=${preload}`, FORCE_COLOR: "1" } : {}), + }); + + expect(result.status, result.stderr).toBe(1); + expect(result.stdout, result.stderr).not.toMatch(/[\u001B\u0007]/u); + if ("human" in testCase) { + expect(result.stdout).toBe(""); + } else { + expect(JSON.parse(result.stdout)).toEqual({ + ok: false, + error: { type: "cli_error", message: testCase.message }, + }); + } + expect(result.stderr).toContain(testCase.message); + expect(result.stderr.split(testCase.message)).toHaveLength(2); + if ("tty" in testCase) { + expect(result.stderr).toContain("\u001B[?25h"); + } + }, + { prefix: "openclaw-task-registration-json-failure-e2e-" }, + ); + }); + it.each([ { name: "qr", command: ["qr"] }, { name: "clawbot qr", command: ["clawbot", "qr"] },