From 48addd51df89c8298da6558a139307f517a21e88 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Fri, 21 Aug 2026 13:57:52 -0300 Subject: [PATCH] =?UTF-8?q?fix(cli):=20combo=20create=20cannot=20accept=20?= =?UTF-8?q?models=20=E2=80=94=20add=20--models=20option=20(#10954)=20(#110?= =?UTF-8?q?11)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⭐5 — Fix(#10954): `combo create` via CLI sempre criava combos vazios (models: [] hardcoded, sem flag). Adiciona --models/--model com parser próprio (CLI .mjs sem alias @/). TDD RED→GREEN, 22/22 testes verdes. UNSTABLE é o base-red inherited #9985. --- bin/cli/commands/combo.mjs | 28 ++- bin/cli/commands/comboModels.mjs | 142 +++++++++++ .../fixes/10954-combo-create-models.md | 1 + skills/cli-routing/SKILL.md | 5 + .../cli-combo-create-models-10954.test.ts | 224 ++++++++++++++++++ 5 files changed, 398 insertions(+), 2 deletions(-) create mode 100644 bin/cli/commands/comboModels.mjs create mode 100644 changelog.d/fixes/10954-combo-create-models.md create mode 100644 tests/unit/cli-combo-create-models-10954.test.ts diff --git a/bin/cli/commands/combo.mjs b/bin/cli/commands/combo.mjs index 1cd8bb0600..554c1c1d83 100644 --- a/bin/cli/commands/combo.mjs +++ b/bin/cli/commands/combo.mjs @@ -4,6 +4,7 @@ import { withRuntime } from "../runtime.mjs"; import { t } from "../i18n.mjs"; import { apiFetch } from "../api.mjs"; import { emit } from "../output.mjs"; +import { resolveComboModels, collectModel } from "./comboModels.mjs"; const VALID_STRATEGIES = [ "priority", @@ -125,10 +126,31 @@ export function registerCombo(program) { .choices(VALID_STRATEGIES) .default("priority") ) + .option( + "--models ", + "Models for the combo: comma-separated provider/model entries, or a JSON array " + + '(e.g. --models "openai/gpt-4o,anthropic/claude-3-opus" or ' + + '--models \'[{"model":"gpt-4o","providerId":"openai"}]\')' + ) + .option( + "--model ", + "Add one model to the combo (provider/model or bare model id) — repeatable", + collectModel, + [] + ) .action(async (name, opts, cmd) => { const globalOpts = cmd.parent.optsWithGlobals(); + let models; + try { + models = resolveComboModels(opts); + } catch (err) { + console.error(`Error: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + return; + } const exitCode = await runComboCreateCommand(name, opts.strategy, { ...opts, + models, output: globalOpts.output, }); if (exitCode !== 0) process.exit(exitCode); @@ -284,12 +306,14 @@ export async function runComboCreateCommand(name, strategy = "priority", opts = return 1; } + const models = Array.isArray(opts.models) ? opts.models : []; + try { return await withRuntime(async ({ kind, api, db }) => { if (kind === "http") { const res = await api("/api/combos", { method: "POST", - body: { name, strategy, enabled: true, models: [], config: {} }, + body: { name, strategy, enabled: true, models, config: {} }, retry: false, acceptNotOk: true, }); @@ -305,7 +329,7 @@ export async function runComboCreateCommand(name, strategy = "priority", opts = console.error(`Combo '${name}' already exists. Delete it first.`); return 1; } - await db.combos.createCombo({ name, strategy, enabled: true, models: [], config: {} }); + await db.combos.createCombo({ name, strategy, enabled: true, models, config: {} }); } console.log(t("combo.created", { name })); diff --git a/bin/cli/commands/comboModels.mjs b/bin/cli/commands/comboModels.mjs new file mode 100644 index 0000000000..fec9dc8470 --- /dev/null +++ b/bin/cli/commands/comboModels.mjs @@ -0,0 +1,142 @@ +// Parses the `--models` / `--model` options for `omniroute combo create` (#10954). +// +// Root cause of #10954: `combo create` only ever registered `--strategy`; the +// HTTP body (POST /api/combos) and the local-db fallback (db.combos.createCombo) +// both hardcoded `models: []`, so every combo created via the CLI came out +// empty regardless of what the operator intended to route to. +// +// Accepted shapes mirror the server-side Zod union in +// `src/shared/validation/schemas/combo.ts` (`comboModelEntry` / +// `createComboSchema.models`) so a CLI-built payload never gets rejected by +// the API that ultimately validates it: +// - a plain string ("provider/model" or a bare model id) — the server's +// `normalizeComboModels` (src/lib/combos/steps.ts) already splits the +// leading "provider/" segment off a plain string, so passing the raw +// token through is sufficient for the common case; +// - a structured `{ kind?: "model", model, providerId?, provider?, ... }` +// object; +// - a structured `{ kind: "combo-ref", comboName, ... }` object (nested +// combo reference). +// +// The CLI (bin/cli/**) ships as plain `.mjs` with relative-only imports — no +// `@/` path aliases and no TS transpilation at runtime — so importing the +// real Zod schema from `src/shared/validation/schemas/combo.ts` is not +// viable here. This module instead validates the same minimal shape by hand +// and stays a thin, independently testable unit. + +/** + * Validates one already-parsed combo model entry against the shape accepted + * by `comboModelEntry` (string | model-step | combo-ref). Throws with a + * 1-based, human-readable position when the entry does not match. + * + * @param {unknown} entry + * @param {number} index + * @returns {string | Record} + */ +export function validateComboModelEntryShape(entry, index) { + const position = index + 1; + + if (typeof entry === "string") { + const trimmed = entry.trim(); + if (trimmed.length === 0) { + throw new Error(`--models entry #${position}: empty model string`); + } + if (trimmed.length > 300) { + throw new Error(`--models entry #${position}: model string exceeds 300 characters`); + } + return trimmed; + } + + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + throw new Error(`--models entry #${position}: must be a string or a JSON object`); + } + + const kind = entry.kind; + + if (kind === "combo-ref") { + if (typeof entry.comboName !== "string" || entry.comboName.trim().length === 0) { + throw new Error( + `--models entry #${position}: kind "combo-ref" requires a non-empty "comboName"` + ); + } + return entry; + } + + if (kind !== undefined && kind !== "model") { + throw new Error(`--models entry #${position}: unknown "kind" value ${JSON.stringify(kind)}`); + } + + if (typeof entry.model !== "string" || entry.model.trim().length === 0) { + throw new Error(`--models entry #${position}: requires a non-empty "model"`); + } + if (entry.providerId !== undefined && typeof entry.providerId !== "string") { + throw new Error(`--models entry #${position}: "providerId" must be a string`); + } + if (entry.provider !== undefined && typeof entry.provider !== "string") { + throw new Error(`--models entry #${position}: "provider" must be a string`); + } + + return entry; +} + +/** + * Parses one `--models` spec — either a JSON array (`--models '[{"model":"gpt-4o"}]'`) + * or a comma-separated list of provider/model tokens + * (`--models 'openai/gpt-4o,anthropic/claude-3-opus'`) — into an array of + * combo model entries. + * + * @param {string} spec + * @returns {Array>} + */ +export function parseModelsSpec(spec) { + const trimmed = String(spec ?? "").trim(); + if (trimmed.length === 0) return []; + + if (trimmed.startsWith("[")) { + let parsed; + try { + parsed = JSON.parse(trimmed); + } catch (err) { + throw new Error(`--models: invalid JSON array (${err.message})`); + } + if (!Array.isArray(parsed)) { + throw new Error("--models: JSON value must be an array"); + } + return parsed.map((entry, i) => validateComboModelEntryShape(entry, i)); + } + + return trimmed + .split(",") + .map((token) => token.trim()) + .filter((token) => token.length > 0) + .map((token, i) => validateComboModelEntryShape(token, i)); +} + +/** + * Resolves the final `models` array for `combo create` from Commander opts: + * `--models ` and/or repeatable `--model `. + * + * @param {{ models?: string, model?: string[] }} opts + * @returns {Array>} + */ +export function resolveComboModels(opts = {}) { + const result = []; + + if (typeof opts.models === "string" && opts.models.trim().length > 0) { + result.push(...parseModelsSpec(opts.models)); + } + + if (Array.isArray(opts.model)) { + opts.model.forEach((token, i) => { + result.push(validateComboModelEntryShape(String(token).trim(), i)); + }); + } + + return result; +} + +/** Commander `collect`-style reducer for the repeatable `--model` option. */ +export function collectModel(value, previous) { + previous.push(value); + return previous; +} diff --git a/changelog.d/fixes/10954-combo-create-models.md b/changelog.d/fixes/10954-combo-create-models.md new file mode 100644 index 0000000000..0a0638bcea --- /dev/null +++ b/changelog.d/fixes/10954-combo-create-models.md @@ -0,0 +1 @@ +- fix(cli): combo create accepts --models and no longer creates empty combos (#10954) diff --git a/skills/cli-routing/SKILL.md b/skills/cli-routing/SKILL.md index 9485d0a593..aa7591e402 100644 --- a/skills/cli-routing/SKILL.md +++ b/skills/cli-routing/SKILL.md @@ -70,6 +70,11 @@ omniroute combo switch Create a new routing combo +**Flags:** + +- `--models ` +- `--model ` + **Example:** ```bash diff --git a/tests/unit/cli-combo-create-models-10954.test.ts b/tests/unit/cli-combo-create-models-10954.test.ts new file mode 100644 index 0000000000..dc53459d6e --- /dev/null +++ b/tests/unit/cli-combo-create-models-10954.test.ts @@ -0,0 +1,224 @@ +// Regression for #10954: `omniroute combo create` did not accept any way to +// specify models — `bin/cli/commands/combo.mjs` only ever registered +// `--strategy`, and both the HTTP body (POST /api/combos) and the local-db +// fallback (db.combos.createCombo) hardcoded `models: []`. Every combo +// created via the CLI came out empty. +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import type { Command } from "commander"; + +type CapturedOpts = Record; + +interface MockFetchInit { + method?: string; + body?: string; +} + +const ORIGINAL_DATA_DIR = process.env.DATA_DIR; +const ORIGINAL_FETCH = globalThis.fetch; + +function createTempDataDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-cli-combo-models-")); +} + +async function withComboEnv(fn: (dataDir: string) => Promise) { + const dataDir = createTempDataDir(); + process.env.DATA_DIR = dataDir; + // Mock fetch → simulates server offline so withRuntime falls back to DB. + globalThis.fetch = (async () => { + throw new Error("server offline"); + }) as typeof fetch; + + const originalLog = console.log; + console.log = () => {}; + + try { + await fn(dataDir); + } finally { + console.log = originalLog; + globalThis.fetch = ORIGINAL_FETCH; + fs.rmSync(dataDir, { recursive: true, force: true }); + + if (ORIGINAL_DATA_DIR === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = ORIGINAL_DATA_DIR; + } +} + +function makeHealthAndComboFetch(capture: { body: CapturedOpts | null }) { + return (async (url: string, opts?: MockFetchInit) => { + if (String(url).includes("/api/health")) { + return { + ok: true, + status: 200, + json: async () => ({ status: "ok" }), + text: async () => "{}", + headers: new Headers(), + }; + } + if (String(url).includes("/api/combos") && opts?.method === "POST") { + capture.body = opts?.body ? JSON.parse(opts.body) : null; + return { + ok: true, + status: 201, + json: async () => ({ id: "combo-1", ...capture.body }), + text: async () => JSON.stringify(capture.body), + headers: new Headers(), + }; + } + throw new Error(`unexpected fetch: ${url}`); + }) as unknown as typeof fetch; +} + +// RED (on untouched code): `combo create` only registers `--strategy` — an +// unrecognized `--models` option makes Commander (in strict `exitOverride` +// mode) throw "unknown option '--models'" instead of parsing. +test("combo create — parses --models without throwing (Commander option registered)", async () => { + const { registerCombo } = await import("../../bin/cli/commands/combo.mjs"); + const { Command } = await import("commander"); + + const prog = new Command().exitOverride(); + registerCombo(prog); + const comboCmd = prog.commands.find((c: Command) => c.name() === "combo") as Command; + const createCmd = comboCmd.commands.find((c: Command) => c.name() === "create") as Command; + + let capturedOpts: CapturedOpts | null = null; + createCmd.action((_name: string, opts: CapturedOpts) => { + capturedOpts = opts; + }); + + await prog.parseAsync( + ["node", "x", "combo", "create", "my-combo", "--models", "openai/gpt-4o,anthropic/claude-3-opus"], + { from: "node" } + ); + + assert.ok(capturedOpts, "action should have been called"); + assert.equal(capturedOpts.models, "openai/gpt-4o,anthropic/claude-3-opus"); +}); + +test("combo create — repeatable --model is registered and collected", async () => { + const { registerCombo } = await import("../../bin/cli/commands/combo.mjs"); + const { Command } = await import("commander"); + + const prog = new Command().exitOverride(); + registerCombo(prog); + const comboCmd = prog.commands.find((c: Command) => c.name() === "combo") as Command; + const createCmd = comboCmd.commands.find((c: Command) => c.name() === "create") as Command; + + let capturedOpts: CapturedOpts | null = null; + createCmd.action((_name: string, opts: CapturedOpts) => { + capturedOpts = opts; + }); + + await prog.parseAsync( + [ + "node", + "x", + "combo", + "create", + "my-combo", + "--model", + "openai/gpt-4o", + "--model", + "anthropic/claude-3-opus", + ], + { from: "node" } + ); + + assert.deepEqual(capturedOpts.model, ["openai/gpt-4o", "anthropic/claude-3-opus"]); +}); + +test("comboModels.resolveComboModels — parses CSV provider/model tokens", async () => { + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + const models = resolveComboModels({ models: "openai/gpt-4o, anthropic/claude-3-opus" }); + assert.deepEqual(models, ["openai/gpt-4o", "anthropic/claude-3-opus"]); +}); + +test("comboModels.resolveComboModels — parses a JSON array of structured entries", async () => { + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + const models = resolveComboModels({ + models: JSON.stringify([ + { model: "gpt-4o", providerId: "openai" }, + { kind: "combo-ref", comboName: "fallback-combo" }, + ]), + }); + assert.deepEqual(models, [ + { model: "gpt-4o", providerId: "openai" }, + { kind: "combo-ref", comboName: "fallback-combo" }, + ]); +}); + +test("comboModels.resolveComboModels — rejects an invalid JSON entry shape", async () => { + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + assert.throws( + () => resolveComboModels({ models: JSON.stringify([{ providerId: "openai" }]) }), + /requires a non-empty "model"/ + ); +}); + +test("comboModels.resolveComboModels — merges --models and repeated --model", async () => { + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + const models = resolveComboModels({ + models: "openai/gpt-4o", + model: ["anthropic/claude-3-opus"], + }); + assert.deepEqual(models, ["openai/gpt-4o", "anthropic/claude-3-opus"]); +}); + +// GREEN: end-to-end through runComboCreateCommand — local-db fallback path. +test("combo create (db fallback) — stores the parsed --models, no longer creates an empty combo", async () => { + await withComboEnv(async () => { + const { runComboCreateCommand } = await import("../../bin/cli/commands/combo.mjs"); + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + + const models = resolveComboModels({ models: "openai/gpt-4o,anthropic/claude-3-opus" }); + const result = await runComboCreateCommand("models-combo", "priority", { models }); + assert.equal(result, 0); + + const { getComboByName } = await import("../../src/lib/db/combos.ts"); + const combo = await getComboByName("models-combo"); + assert.ok(combo); + // The repository layer (src/lib/db/repositories/sqliteComboRepository.ts) + // normalizes plain "provider/model" strings into structured ComboStep + // objects on write — assert on the normalized shape rather than raw + // string equality, and above all assert the combo is no longer empty + // (the actual #10954 regression). + const storedModels = combo.models as Array>; + assert.equal(storedModels.length, 2, "combo must not be created empty"); + assert.equal(storedModels[0].model, "openai/gpt-4o"); + assert.equal(storedModels[0].providerId, "openai"); + assert.equal(storedModels[1].model, "anthropic/claude-3-opus"); + assert.equal(storedModels[1].providerId, "anthropic"); + }); +}); + +// GREEN: end-to-end through runComboCreateCommand — HTTP path, verifies the +// POST /api/combos body actually carries the parsed models. +test("combo create (HTTP) — POST /api/combos body carries the parsed models", async () => { + const dataDir = createTempDataDir(); + process.env.DATA_DIR = dataDir; + const capture: { body: CapturedOpts | null } = { body: null }; + globalThis.fetch = makeHealthAndComboFetch(capture); + const originalLog = console.log; + console.log = () => {}; + + try { + const { runComboCreateCommand } = await import("../../bin/cli/commands/combo.mjs"); + const { resolveComboModels } = await import("../../bin/cli/commands/comboModels.mjs"); + + const models = resolveComboModels({ models: "openai/gpt-4o,anthropic/claude-3-opus" }); + const result = await runComboCreateCommand("http-models-combo", "priority", { models }); + + assert.equal(result, 0); + assert.ok(capture.body, "POST /api/combos should have been called"); + assert.deepEqual(capture.body.models, ["openai/gpt-4o", "anthropic/claude-3-opus"]); + } finally { + console.log = originalLog; + globalThis.fetch = ORIGINAL_FETCH; + fs.rmSync(dataDir, { recursive: true, force: true }); + if (ORIGINAL_DATA_DIR === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = ORIGINAL_DATA_DIR; + } +});