From a13f4d29ee7be0cd89a8009fc7a28f44aa3e2d5a Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 26 Aug 2026 17:58:12 -0700 Subject: [PATCH] refactor(cli): collapse model fallback command wrappers (#130536) --- src/cli/models-cli.test.ts | 16 +-- src/cli/models-cli.ts | 55 ++++---- src/commands/models.set.e2e.test.ts | 130 ++++++++++++++----- src/commands/models/fallbacks-shared.test.ts | 117 ++++++++--------- src/commands/models/fallbacks-shared.ts | 6 +- src/commands/models/fallbacks.ts | 47 ------- src/commands/models/image-fallbacks.ts | 47 ------- 7 files changed, 188 insertions(+), 230 deletions(-) delete mode 100644 src/commands/models/fallbacks.ts delete mode 100644 src/commands/models/image-fallbacks.ts diff --git a/src/cli/models-cli.test.ts b/src/cli/models-cli.test.ts index 3392afd5b5dd..b5a4c3da823d 100644 --- a/src/cli/models-cli.test.ts +++ b/src/cli/models-cli.test.ts @@ -77,17 +77,11 @@ vi.mock("../commands/models/aliases.js", () => ({ modelsAliasesListCommand: mocks.modelsAliasesListCommand, modelsAliasesRemoveCommand: mocks.modelsAliasesRemoveCommand, })); -vi.mock("../commands/models/fallbacks.js", () => ({ - modelsFallbacksAddCommand: mocks.noopAsync, - modelsFallbacksClearCommand: mocks.noopAsync, - modelsFallbacksListCommand: mocks.noopAsync, - modelsFallbacksRemoveCommand: mocks.noopAsync, -})); -vi.mock("../commands/models/image-fallbacks.js", () => ({ - modelsImageFallbacksAddCommand: mocks.noopAsync, - modelsImageFallbacksClearCommand: mocks.noopAsync, - modelsImageFallbacksListCommand: mocks.noopAsync, - modelsImageFallbacksRemoveCommand: mocks.noopAsync, +vi.mock("../commands/models/fallbacks-shared.js", () => ({ + addFallbackCommand: mocks.noopAsync, + clearFallbacksCommand: mocks.noopAsync, + listFallbacksCommand: mocks.noopAsync, + removeFallbackCommand: mocks.noopAsync, })); vi.mock("../commands/models/scan.js", () => ({ modelsScanCommand: mocks.modelsScanCommand, diff --git a/src/cli/models-cli.ts b/src/cli/models-cli.ts index 1260313a91e5..21eb450610bc 100644 --- a/src/cli/models-cli.ts +++ b/src/cli/models-cli.ts @@ -21,10 +21,7 @@ const loadModelsStatusCommands = createModuleLoader( ); const loadModelsAliasesCommands = createModuleLoader(() => import("../commands/models/aliases.js")); const loadModelsFallbacksCommands = createModuleLoader( - () => import("../commands/models/fallbacks.js"), -); -const loadModelsImageFallbacksCommands = createModuleLoader( - () => import("../commands/models/image-fallbacks.js"), + () => import("../commands/models/fallbacks-shared.js"), ); const loadModelsAuthCommands = createModuleLoader(() => import("../commands/models/auth.js")); const loadModelsAuthOrderCommands = createModuleLoader( @@ -216,34 +213,25 @@ export function registerModelsCli(program: Command) { modelType: "model", noun: "fallback", article: "a", - load: async () => { - const commands = await loadModelsFallbacksCommands(); - return { - list: commands.modelsFallbacksListCommand, - add: commands.modelsFallbacksAddCommand, - remove: commands.modelsFallbacksRemoveCommand, - clear: commands.modelsFallbacksClearCommand, - }; - }, + key: "model", + label: "Fallbacks", + notFoundLabel: "Fallback", + clearedMessage: "Fallback list cleared.", }, { name: "image-fallbacks", modelType: "image model", noun: "image fallback", article: "an", - load: async () => { - const commands = await loadModelsImageFallbacksCommands(); - return { - list: commands.modelsImageFallbacksListCommand, - add: commands.modelsImageFallbacksAddCommand, - remove: commands.modelsImageFallbacksRemoveCommand, - clear: commands.modelsImageFallbacksClearCommand, - }; - }, + key: "imageModel", + label: "Image fallbacks", + notFoundLabel: "Image fallback", + clearedMessage: "Image fallback list cleared.", }, ] as const; - for (const { name, modelType, noun, article, load } of fallbackGroups) { + for (const params of fallbackGroups) { + const { name, modelType, noun, article } = params; const group = models.command(name).description(`Manage ${modelType} fallback list`); group @@ -253,20 +241,27 @@ export function registerModelsCli(program: Command) { .option("--plain", "Plain output", false) .action(async (opts) => { await withModelsRuntime(async ({ defaultRuntime }) => { - const commands = await load(); - await commands.list({ ...opts, json: hasJsonOutput(opts) }, defaultRuntime); + const { listFallbacksCommand } = await loadModelsFallbacksCommands(); + await listFallbacksCommand( + params, + { ...opts, json: hasJsonOutput(opts) }, + defaultRuntime, + ); }); }); - for (const action of ["add", "remove"] as const) { + for (const [action, handler] of [ + ["add", "addFallbackCommand"], + ["remove", "removeFallbackCommand"], + ] as const) { group .command(action) .description(`${action === "add" ? "Add" : "Remove"} ${article} ${noun} model`) .argument("", "Model id or alias") .action(async (model: string) => { await withModelsRuntime(async ({ defaultRuntime }) => { - const commands = await load(); - await commands[action](model, defaultRuntime); + const commands = await loadModelsFallbacksCommands(); + await commands[handler](params, model, defaultRuntime); }); }); } @@ -276,8 +271,8 @@ export function registerModelsCli(program: Command) { .description(`Clear all ${noun} models`) .action(async () => { await withModelsRuntime(async ({ defaultRuntime }) => { - const commands = await load(); - await commands.clear(defaultRuntime); + const { clearFallbacksCommand } = await loadModelsFallbacksCommands(); + await clearFallbacksCommand(params, defaultRuntime); }); }); } diff --git a/src/commands/models.set.e2e.test.ts b/src/commands/models.set.e2e.test.ts index 3abc124b334e..9a3bb1a8a9a8 100644 --- a/src/commands/models.set.e2e.test.ts +++ b/src/commands/models.set.e2e.test.ts @@ -1,8 +1,11 @@ // Models set e2e tests cover persisted model selection updates through command handlers. -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { createModelVisibilityPolicy } from "../agents/model-visibility-policy.js"; +import { registerModelsCli } from "../cli/models-cli.js"; import { stampConfigWriteMetadata } from "../config/io.meta.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { defaultRuntime } from "../runtime.js"; +import { runRegisteredCli } from "../test-utils/command-runner.js"; const mocks = vi.hoisted(() => ({ currentConfig: {} as Record, @@ -26,7 +29,6 @@ vi.mock("../config/config.js", async () => { }; }); -import { modelsFallbacksAddCommand } from "./models/fallbacks.js"; import { modelsSetImageCommand } from "./models/set-image.js"; import { modelsSetCommand } from "./models/set.js"; @@ -56,12 +58,29 @@ function expectWrittenPrimaryModel(model: string) { }); } +const fallbackGroups = [ + { name: "fallbacks", key: "model", label: "Fallbacks", singular: "Fallback" }, + { + name: "image-fallbacks", + key: "imageModel", + label: "Image fallbacks", + singular: "Image fallback", + }, +] as const; + +async function runFallbackCommand(name: string, ...args: string[]) { + await runRegisteredCli({ register: registerModelsCli, argv: ["models", name, ...args] }); +} + describe("models set + fallbacks", () => { beforeEach(() => { mocks.currentConfig = {}; mocks.writtenConfig = undefined; + vi.spyOn(defaultRuntime, "log").mockImplementation(() => {}); }); + afterEach(() => vi.restoreAllMocks()); + it("normalizes z.ai provider in models set", async () => { mockConfigSnapshot({}); const runtime = makeRuntime(); @@ -149,38 +168,87 @@ describe("models set + fallbacks", () => { expect(policy.allows({ provider: "openai", model: "gpt-5.6-sol" })).toBe(true); }); - it("normalizes z-ai provider in models fallbacks add", async () => { - mockConfigSnapshot({ agents: { defaults: { model: { fallbacks: [] } } } }); - const runtime = makeRuntime(); + it.each(fallbackGroups)( + "normalizes z-ai provider in models $name add", + async ({ name, key, label }) => { + mockConfigSnapshot({ agents: { defaults: { [key]: { fallbacks: [] } } } }); - await modelsFallbacksAddCommand("z-ai/glm-4.7", runtime); + await runFallbackCommand(name, "add", "z-ai/glm-4.7"); - const written = getWrittenConfig(); - expect(written.agents).toEqual({ - defaults: { - model: { fallbacks: ["zai/glm-4.7"] }, - models: { "zai/glm-4.7": {} }, - }, - }); - }); - - it("preserves primary when adding fallbacks to string defaults.model", async () => { - mockConfigSnapshot({ agents: { defaults: { model: "openai/gpt-4.1-mini" } } }); - const runtime = makeRuntime(); - - await modelsFallbacksAddCommand("anthropic/claude-opus-4-6", runtime); - - const written = getWrittenConfig(); - expect(written.agents).toEqual({ - defaults: { - model: { - primary: "openai/gpt-4.1-mini", - fallbacks: ["anthropic/claude-opus-4-6"], + const written = getWrittenConfig(); + expect(written.agents).toEqual({ + defaults: { + [key]: { fallbacks: ["zai/glm-4.7"] }, + models: { "zai/glm-4.7": {} }, }, - models: { "anthropic/claude-opus-4-6": {} }, - }, - }); - }); + }); + expect(defaultRuntime.log).toHaveBeenLastCalledWith(`${label}: zai/glm-4.7`); + }, + ); + + it.each(fallbackGroups)( + "preserves string primary when adding models $name", + async ({ name, key }) => { + mockConfigSnapshot({ agents: { defaults: { [key]: "openai/gpt-4.1-mini" } } }); + + await runFallbackCommand(name, "add", "anthropic/claude-opus-4-6"); + + const written = getWrittenConfig(); + expect(written.agents).toEqual({ + defaults: { + [key]: { + primary: "openai/gpt-4.1-mini", + fallbacks: ["anthropic/claude-opus-4-6"], + }, + models: { "anthropic/claude-opus-4-6": {} }, + }, + }); + }, + ); + + it.each(fallbackGroups)( + "removes aliases and clears only models $name", + async ({ name, key, label, singular }) => { + const siblingKey = key === "model" ? "imageModel" : "model"; + const primary = "openai/gpt-5.6-luna"; + const sibling = { primary, fallbacks: [primary] }; + mockConfigSnapshot({ + agents: { + defaults: { + [key]: { primary, fallbacks: ["backup", primary] }, + [siblingKey]: sibling, + models: { "zai/glm-4.7": { alias: "backup" } }, + }, + }, + }); + + await runFallbackCommand(name, "remove", "z-ai/glm-4.7"); + + expect(getWrittenConfig().agents?.defaults?.[key]).toEqual({ primary, fallbacks: [primary] }); + expect(getWrittenConfig().agents?.defaults?.[siblingKey]).toEqual(sibling); + expect(defaultRuntime.log).toHaveBeenLastCalledWith(`${label}: ${primary}`); + mocks.currentConfig = getWrittenConfig(); + + await runFallbackCommand(name, "clear"); + + expect(getWrittenConfig().agents?.defaults?.[key]).toEqual({ primary, fallbacks: [] }); + expect(getWrittenConfig().agents?.defaults?.[siblingKey]).toEqual(sibling); + expect(defaultRuntime.log).toHaveBeenLastCalledWith(`${singular} list cleared.`); + + mockConfigSnapshot(getWrittenConfig()); + const error = vi.spyOn(defaultRuntime, "error").mockImplementation(() => {}); + const exit = vi.spyOn(defaultRuntime, "exit").mockImplementation(() => { + throw new Error("CLI exit"); + }); + await expect(runFallbackCommand(name, "remove", "backup")).rejects.toThrow("CLI exit"); + expect(error).toHaveBeenCalledWith( + expect.stringContaining(`${singular} not found: zai/glm-4.7.`), + ); + expect(error).toHaveBeenCalledWith(expect.stringContaining(`models ${name} list`)); + expect(exit).toHaveBeenCalledWith(1); + expect(mocks.writtenConfig).toBeUndefined(); + }, + ); it("normalizes provider casing in models set", async () => { mockConfigSnapshot({}); diff --git a/src/commands/models/fallbacks-shared.test.ts b/src/commands/models/fallbacks-shared.test.ts index dacb32511f56..b84fe2c50a6c 100644 --- a/src/commands/models/fallbacks-shared.test.ts +++ b/src/commands/models/fallbacks-shared.test.ts @@ -1,6 +1,7 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; -import type { RuntimeEnv } from "../../runtime.js"; -import { listFallbacksCommand } from "./fallbacks-shared.js"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerModelsCli } from "../../cli/models-cli.js"; +import { defaultRuntime } from "../../runtime.js"; +import { runRegisteredCli } from "../../test-utils/command-runner.js"; const mocks = vi.hoisted(() => ({ loadModelsConfig: vi.fn(), @@ -10,25 +11,22 @@ vi.mock("./load-config.js", () => ({ loadModelsConfig: mocks.loadModelsConfig, })); -describe("listFallbacksCommand", () => { +describe.each([ + { + name: "fallbacks", + label: "Fallbacks", + key: "model" as const, + model: "anthropic/claude-sonnet-4-6", + }, + { + name: "image-fallbacks", + label: "Image fallbacks", + key: "imageModel" as const, + model: "openai/gpt-image-1", + }, +])("models $name list", (testCase) => { beforeEach(() => { mocks.loadModelsConfig.mockReset(); - }); - - it.each([ - { - label: "Fallbacks", - key: "model" as const, - commandName: "models fallbacks list", - model: "anthropic/claude-sonnet-4-6", - }, - { - label: "Image fallbacks", - key: "imageModel" as const, - commandName: "models image-fallbacks list", - model: "openai/gpt-image-1", - }, - ])("attributes $label diagnostics to the real CLI command", async (testCase) => { mocks.loadModelsConfig.mockResolvedValue({ agents: { defaults: { @@ -36,54 +34,53 @@ describe("listFallbacksCommand", () => { }, }, }); - const runtime = { - log: vi.fn(), - error: vi.fn(), - exit: vi.fn(), - } satisfies RuntimeEnv; + vi.spyOn(defaultRuntime, "log").mockImplementation(() => {}); + vi.spyOn(defaultRuntime, "writeStdout").mockImplementation(() => {}); + vi.spyOn(defaultRuntime, "writeJson").mockImplementation(() => {}); + }); - await listFallbacksCommand( - { label: testCase.label, key: testCase.key }, - { json: true }, - runtime, - ); + afterEach(() => vi.restoreAllMocks()); + + it.each([ + ["--json", testCase.name, "list"], + [testCase.name, "list", "--json"], + ])("writes JSON and attributes diagnostics for %s %s %s", async (...args) => { + await runRegisteredCli({ register: registerModelsCli, argv: ["models", ...args] }); expect(mocks.loadModelsConfig).toHaveBeenCalledWith({ - commandName: testCase.commandName, - runtime, - }); - expect(runtime.log).toHaveBeenCalledOnce(); - expect(JSON.parse(runtime.log.mock.calls[0]?.[0] as string)).toEqual({ - fallbacks: [testCase.model], + commandName: `models ${testCase.name} list`, + runtime: defaultRuntime, }); + expect(vi.mocked(defaultRuntime.writeJson).mock.calls.map(([value]) => value)).toEqual([ + { + fallbacks: [testCase.model], + }, + ]); + expect(defaultRuntime.log).not.toHaveBeenCalled(); }); - it.each([ - { label: "Fallbacks", key: "model" as const, model: "anthropic/claude-sonnet-4-6" }, - { label: "Image fallbacks", key: "imageModel" as const, model: "openai/gpt-image-1" }, - ])("writes populated plain $label directly to stdout", async (testCase) => { - mocks.loadModelsConfig.mockResolvedValue({ - agents: { - defaults: { - [testCase.key]: { fallbacks: [testCase.model] }, - }, - }, + it("writes populated plain output directly to stdout", async () => { + await runRegisteredCli({ + register: registerModelsCli, + argv: ["models", testCase.name, "list", "--plain"], }); - const runtime = { - log: vi.fn(), - error: vi.fn(), - exit: vi.fn(), - writeStdout: vi.fn(), - writeJson: vi.fn(), - }; - await listFallbacksCommand( - { label: testCase.label, key: testCase.key }, - { plain: true }, - runtime, - ); + expect(defaultRuntime.writeStdout).toHaveBeenCalledExactlyOnceWith(testCase.model); + expect(defaultRuntime.log).not.toHaveBeenCalled(); + }); - expect(runtime.writeStdout).toHaveBeenCalledExactlyOnceWith(testCase.model); - expect(runtime.log).not.toHaveBeenCalled(); + it.each([false, true])("preserves human output (empty: %s)", async (empty) => { + if (empty) { + mocks.loadModelsConfig.mockResolvedValue({}); + } + await runRegisteredCli({ + register: registerModelsCli, + argv: ["models", testCase.name, "list"], + }); + + expect(vi.mocked(defaultRuntime.log).mock.calls).toEqual([ + [`${testCase.label} (${empty ? 0 : 1}):`], + [empty ? "- none" : `- ${testCase.model}`], + ]); }); }); diff --git a/src/commands/models/fallbacks-shared.ts b/src/commands/models/fallbacks-shared.ts index b3fb8187d960..fa2affe67982 100644 --- a/src/commands/models/fallbacks-shared.ts +++ b/src/commands/models/fallbacks-shared.ts @@ -85,7 +85,6 @@ export async function addFallbackCommand( params: { label: string; key: DefaultsFallbackKey; - logPrefix: string; }, modelRaw: string, runtime: RuntimeEnv, @@ -110,7 +109,7 @@ export async function addFallbackCommand( }); logConfigUpdated(runtime); - runtime.log(`${params.logPrefix}: ${getFallbacks(updated, params.key).join(", ")}`); + runtime.log(`${params.label}: ${getFallbacks(updated, params.key).join(", ")}`); } /** Removes a fallback model by resolving aliases to the canonical provider/model key. */ @@ -119,7 +118,6 @@ export async function removeFallbackCommand( label: string; key: DefaultsFallbackKey; notFoundLabel: string; - logPrefix: string; }, modelRaw: string, runtime: RuntimeEnv, @@ -156,7 +154,7 @@ export async function removeFallbackCommand( }); logConfigUpdated(runtime); - runtime.log(`${params.logPrefix}: ${getFallbacks(updated, params.key).join(", ")}`); + runtime.log(`${params.label}: ${getFallbacks(updated, params.key).join(", ")}`); } /** Clears all fallback model refs for the selected defaults key. */ diff --git a/src/commands/models/fallbacks.ts b/src/commands/models/fallbacks.ts deleted file mode 100644 index cdd5882fd32e..000000000000 --- a/src/commands/models/fallbacks.ts +++ /dev/null @@ -1,47 +0,0 @@ -/** Commands for managing default text model fallbacks. */ -import type { RuntimeEnv } from "../../runtime.js"; -import { - addFallbackCommand, - clearFallbacksCommand, - listFallbacksCommand, - removeFallbackCommand, -} from "./fallbacks-shared.js"; - -/** Lists configured text model fallbacks. */ -export async function modelsFallbacksListCommand( - opts: { json?: boolean; plain?: boolean }, - runtime: RuntimeEnv, -) { - return await listFallbacksCommand({ label: "Fallbacks", key: "model" }, opts, runtime); -} - -/** Adds a text model fallback. */ -export async function modelsFallbacksAddCommand(modelRaw: string, runtime: RuntimeEnv) { - return await addFallbackCommand( - { label: "Fallbacks", key: "model", logPrefix: "Fallbacks" }, - modelRaw, - runtime, - ); -} - -/** Removes a text model fallback. */ -export async function modelsFallbacksRemoveCommand(modelRaw: string, runtime: RuntimeEnv) { - return await removeFallbackCommand( - { - label: "Fallbacks", - key: "model", - notFoundLabel: "Fallback", - logPrefix: "Fallbacks", - }, - modelRaw, - runtime, - ); -} - -/** Clears all text model fallbacks. */ -export async function modelsFallbacksClearCommand(runtime: RuntimeEnv) { - return await clearFallbacksCommand( - { key: "model", clearedMessage: "Fallback list cleared." }, - runtime, - ); -} diff --git a/src/commands/models/image-fallbacks.ts b/src/commands/models/image-fallbacks.ts deleted file mode 100644 index 819bd528398c..000000000000 --- a/src/commands/models/image-fallbacks.ts +++ /dev/null @@ -1,47 +0,0 @@ -/** Commands for managing default image model fallbacks. */ -import type { RuntimeEnv } from "../../runtime.js"; -import { - addFallbackCommand, - clearFallbacksCommand, - listFallbacksCommand, - removeFallbackCommand, -} from "./fallbacks-shared.js"; - -/** Lists configured image model fallbacks. */ -export async function modelsImageFallbacksListCommand( - opts: { json?: boolean; plain?: boolean }, - runtime: RuntimeEnv, -) { - return await listFallbacksCommand({ label: "Image fallbacks", key: "imageModel" }, opts, runtime); -} - -/** Adds an image model fallback. */ -export async function modelsImageFallbacksAddCommand(modelRaw: string, runtime: RuntimeEnv) { - return await addFallbackCommand( - { label: "Image fallbacks", key: "imageModel", logPrefix: "Image fallbacks" }, - modelRaw, - runtime, - ); -} - -/** Removes an image model fallback. */ -export async function modelsImageFallbacksRemoveCommand(modelRaw: string, runtime: RuntimeEnv) { - return await removeFallbackCommand( - { - label: "Image fallbacks", - key: "imageModel", - notFoundLabel: "Image fallback", - logPrefix: "Image fallbacks", - }, - modelRaw, - runtime, - ); -} - -/** Clears all image model fallbacks. */ -export async function modelsImageFallbacksClearCommand(runtime: RuntimeEnv) { - return await clearFallbacksCommand( - { key: "imageModel", clearedMessage: "Image fallback list cleared." }, - runtime, - ); -}