diff --git a/changelog.d/fixes/8530-combo-model-name-collision.md b/changelog.d/fixes/8530-combo-model-name-collision.md new file mode 100644 index 0000000000..65e70c5524 --- /dev/null +++ b/changelog.d/fixes/8530-combo-model-name-collision.md @@ -0,0 +1 @@ +- fix(api): surface a non-blocking warning + startup scan when a combo name shadows a real model id, instead of silently routing with zero signal (#8530) diff --git a/docs/routing/AUTO-COMBO.md b/docs/routing/AUTO-COMBO.md index 07ecf36f24..424926507a 100644 --- a/docs/routing/AUTO-COMBO.md +++ b/docs/routing/AUTO-COMBO.md @@ -128,6 +128,37 @@ Auto-scoring selects best provider/model per request | `src/sse/handlers/chat.ts` | Integration: auto prefix short-circuit | | `src/shared/constants/providers.ts` | `SYSTEM_PROVIDERS.auto` system entry | +## Combo Names That Match a Real Model Id + +A combo whose `name` is identical to a bare model id (e.g. a combo named +`gpt-5.5`) is an **intentional, supported pattern**, not a bug: it is the +mechanism for per-model-id provider fallback documented in +[#6940](https://github.com/diegosouzapw/OmniRoute/issues/6940). Because combo +resolution is checked before bare-model-id resolution +(`getComboForModel()` in `src/sse/services/model.ts`), a request for the bare +id `gpt-5.5` is routed through the combo's targets (e.g. +`acme-responses/gpt-5.5`, `backup-responses/gpt-5.5`) instead of straight to +a single provider — this reuses the combo-before-rewrite precedence built for +[#3227/#3233](https://github.com/diegosouzapw/OmniRoute/issues/3227) and is +regression-tested by `tests/unit/responses-combo-resolution-3227.test.ts` and +`tests/unit/combo-name-codex-responses-rewrite.test.ts`. + +Creating or renaming a combo to a name that shadows a real model id is +**never rejected** — doing so would break this documented workflow. Instead +(#8530), `POST /api/combos` and `PUT /api/combos/[id]` attach a non-blocking +`warning` field to the response when the (new) name collides with a real +model id: + +```json +{ "warning": { "code": "COMBO_NAME_SHADOWS_MODEL", "modelId": "gpt-5.5", "providerId": "openai" } } +``` + +At boot, `scanComboModelNameCollisionsAtBoot()` +(`src/instrumentation-node.ts`) also logs a one-line `[STARTUP]` warning +enumerating every existing combo that shadows a model id, so operators who +hit this by accident (rather than intentionally, per #6940) have a signal. +The detection helper lives in `src/lib/combos/modelNameCollision.ts`. + ## How It Works (Persisted Auto-Combos) The Auto-Combo Engine dynamically selects the best provider/model for each request using a **12-factor scoring function** (defined in `open-sse/services/autoCombo/scoring.ts` → `DEFAULT_WEIGHTS`). All weights sum to **1.0**. diff --git a/src/app/api/combos/[id]/route.ts b/src/app/api/combos/[id]/route.ts index 5ec69f1385..a1ba03ee1b 100644 --- a/src/app/api/combos/[id]/route.ts +++ b/src/app/api/combos/[id]/route.ts @@ -18,6 +18,7 @@ import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; import { QUOTA_MODEL_PREFIX } from "@/lib/quota/quotaModelNaming"; import { comboErrorResponse } from "@/lib/api/comboErrorResponse"; import { ComboInvariantError } from "@/lib/combos/invariants"; +import { buildComboNameCollisionWarning } from "@/lib/combos/modelNameCollision"; // Minimal shape for the fields we read off a combo row in this route. // `getComboById` returns a structurally `JsonRecord`-typed object, so we @@ -249,7 +250,13 @@ export async function PUT(request, { params }) { // Auto sync to Cloud if enabled await syncToCloudIfEnabled(); - return NextResponse.json(combo); + // #8530: a combo renamed to a real model id is a supported pattern + // (#6940 — bare-model-id provider fallback), so it is never rejected. + // Surface it as a non-blocking warning instead of silently shadowing it. + const warning = comboName + ? buildComboNameCollisionWarning(String(comboName)) + : null; + return NextResponse.json(warning ? { ...combo, warning } : combo); } catch (error) { if (error instanceof ComboInvariantError) { return comboErrorResponse("COMBO_008", 400, { reason: error.message }, request); diff --git a/src/app/api/combos/route.ts b/src/app/api/combos/route.ts index 7bd3baebef..c13466a800 100644 --- a/src/app/api/combos/route.ts +++ b/src/app/api/combos/route.ts @@ -17,6 +17,7 @@ import { requireManagementAuth } from "@/lib/api/requireManagementAuth"; import { comboErrorResponse } from "@/lib/api/comboErrorResponse"; import { computeComboContextLength } from "@/lib/combos/comboContext"; import { ComboInvariantError } from "@/lib/combos/invariants"; +import { buildComboNameCollisionWarning } from "@/lib/combos/modelNameCollision"; // GET /api/combos - Get all combos export async function GET(request: Request) { @@ -120,7 +121,12 @@ export async function POST(request) { // Auto sync to Cloud if enabled await syncToCloudIfEnabled(); - return NextResponse.json(combo, { status: 201 }); + // #8530: a combo named after a real model id is a supported pattern + // (#6940 — bare-model-id provider fallback), so it is never rejected. + // Surface it as a non-blocking warning so the dashboard/API caller can + // confirm it was intentional instead of silently shadowing the model. + const warning = buildComboNameCollisionWarning(name); + return NextResponse.json(warning ? { ...combo, warning } : combo, { status: 201 }); } catch (error) { if (error instanceof ComboInvariantError) { return comboErrorResponse("COMBO_008", 400, { reason: error.message }, request); diff --git a/src/instrumentation-node.ts b/src/instrumentation-node.ts index 92ec377cf7..f0498e4239 100755 --- a/src/instrumentation-node.ts +++ b/src/instrumentation-node.ts @@ -212,6 +212,36 @@ export async function warmModelCatalogCache(): Promise { } } +/** + * #8530: enumerate existing combos whose name shadows a real model id and + * log a startup warning. Never rejects/throws — #6940 documents a combo + * named after a bare model id as the supported mechanism for per-model + * provider fallback, so a collision here is expected in some deployments; + * this only gives operators who hit it accidentally a signal. + * + * Exported (rather than left inline in registerNodejs()) so it can be unit + * tested directly without exercising the rest of the startup sequence. + */ +export async function scanComboModelNameCollisionsAtBoot(): Promise { + try { + const [{ getCombos }, { scanCombosForModelCollisions }] = await Promise.all([ + import("@/lib/db/combos"), + import("@/lib/combos/modelNameCollision"), + ]); + const collisions = scanCombosForModelCollisions(await getCombos()); + if (collisions.length > 0) { + console.warn( + `[STARTUP] ${collisions.length} combo(s) share a name with a real model id (#8530) — ` + + "intentional per #6940 bare-model-id fallback, but confirm each is expected: " + + collisions.map((c) => `${c.comboName}→${c.providerId}/${c.modelId}`).join(", ") + ); + } + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + console.warn("[STARTUP] Could not scan combos for model-name collisions (non-fatal):", msg); + } +} + export async function registerNodejs(): Promise { markServerStarting(); @@ -264,6 +294,8 @@ export async function registerNodejs(): Promise { console.warn("[STARTUP] Could not clear stale crash cooldowns (non-fatal):", msg); } + await scanComboModelNameCollisionsAtBoot(); + const [ { initGracefulShutdown }, { initApiBridgeServer }, diff --git a/src/lib/combos/modelNameCollision.ts b/src/lib/combos/modelNameCollision.ts new file mode 100644 index 0000000000..a0733a63a0 --- /dev/null +++ b/src/lib/combos/modelNameCollision.ts @@ -0,0 +1,95 @@ +/** + * Combo-name / model-id collision detection (#8530). + * + * #6940 (closed by the maintainer) documents a combo named identically to a + * bare model id — e.g. combo `gpt-5.5` fanning out to + * `acme-responses/gpt-5.5`, `backup-responses/gpt-5.5` — as THE supported + * mechanism for per-model provider fallback on bare Responses model ids + * (reusing the #3227/#3233 combo-before-rewrite precedence, which is + * regression-tested in `tests/unit/responses-combo-resolution-3227.test.ts` + * and `tests/unit/combo-name-codex-responses-rewrite.test.ts`). + * + * So a colliding name is NOT rejected — it is a supported, intentional + * pattern. This module only makes the collision *observable*: callers use it + * to attach a non-blocking warning to the create/rename response and to the + * boot-time scan, instead of silently shadowing the model with zero signal. + */ +import { PROVIDER_MODELS } from "@/shared/constants/models"; + +export interface ComboModelCollision { + providerId: string; + modelId: string; +} + +let cachedIndex: Map | null = null; + +/** + * Bare model id -> first provider that registers it. Built once from the + * (lazily-generated, then cached) provider registry and memoized for the + * process lifetime — the registry is static compiled-in config, not + * runtime/DB state, so it never needs invalidation. + */ +function getModelIdIndex(): Map { + if (cachedIndex) return cachedIndex; + const index = new Map(); + for (const [providerId, models] of Object.entries(PROVIDER_MODELS)) { + for (const model of models) { + if (!index.has(model.id)) { + index.set(model.id, { providerId, modelId: model.id }); + } + } + } + cachedIndex = index; + return index; +} + +/** Test-only: force the memoized registry index to rebuild on next call. */ +export function __resetModelNameCollisionCacheForTest(): void { + cachedIndex = null; +} + +/** + * Returns the provider that registers `name` as a bare model id, or `null` + * when `name` does not collide with any known real model id. + */ +export function findCollidingModel(name: string): ComboModelCollision | null { + if (!name) return null; + return getModelIdIndex().get(name) ?? null; +} + +/** Machine-readable warning payload attached to POST/PUT combo responses. */ +export function buildComboNameCollisionWarning( + name: string +): { code: "COMBO_NAME_SHADOWS_MODEL"; modelId: string; providerId: string } | null { + const collision = findCollidingModel(name); + if (!collision) return null; + return { + code: "COMBO_NAME_SHADOWS_MODEL", + modelId: collision.modelId, + providerId: collision.providerId, + }; +} + +/** Shape of the minimal combo record the boot-time scan needs. */ +export interface ComboLike { + name?: unknown; +} + +/** + * Boot-time scan (see `src/instrumentation-node.ts`): enumerates existing + * combos whose name shadows a real model id, for a startup log — never a + * hard failure, since the shadowing pattern is intentional per #6940. + */ +export function scanCombosForModelCollisions( + combos: readonly ComboLike[] +): Array<{ comboName: string; providerId: string; modelId: string }> { + const results: Array<{ comboName: string; providerId: string; modelId: string }> = []; + for (const combo of combos) { + if (typeof combo.name !== "string" || combo.name.length === 0) continue; + const collision = findCollidingModel(combo.name); + if (collision) { + results.push({ comboName: combo.name, ...collision }); + } + } + return results; +} diff --git a/tests/unit/combo-model-name-collision-8530.test.ts b/tests/unit/combo-model-name-collision-8530.test.ts new file mode 100644 index 0000000000..fcdf09a827 --- /dev/null +++ b/tests/unit/combo-model-name-collision-8530.test.ts @@ -0,0 +1,190 @@ +// #8530 — combo name / model id collision guard. +// +// #6940 (closed by the maintainer) documents a combo named identically to a +// bare model id (e.g. combo `gpt-5.5`) as THE supported mechanism for +// per-model provider fallback — see `tests/unit/responses-combo-resolution-3227.test.ts` +// and `tests/unit/combo-name-codex-responses-rewrite.test.ts` for the +// combo-before-rewrite precedence this relies on. So creation/rename must +// NEVER hard-reject a colliding name (that would regress #6940); it must +// only make the collision observable via a non-blocking `warning` field. +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"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-combo-collision-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const combosDb = await import("../../src/lib/db/combos.ts"); +const createRoute = await import("../../src/app/api/combos/route.ts"); +const comboRoute = await import("../../src/app/api/combos/[id]/route.ts"); +const collision = await import("../../src/lib/combos/modelNameCollision.ts"); + +// A model id that really is registered by multiple providers (verified via +// PROVIDER_MODELS at write time — see open-sse/config/providers/*). +const REAL_MODEL_ID = "gpt-5.5"; +// Not a registered bare model id anywhere in the provider registry. +const NON_COLLIDING_NAME = "not-a-real-model-8530-guard-probe"; + +interface ComboResponseBody { + name?: string; + warning?: { code: string; modelId: string; providerId: string }; +} + +async function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +function makeCreateRequest(body: unknown) { + return new Request("http://localhost/api/combos", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +function makeUpdateRequest(body: unknown) { + return new Request("http://localhost/api/combos/combo-1", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("POST /api/combos: name colliding with a real model id is created (#6940 pattern), with a warning", async () => { + const response = await createRoute.POST( + makeCreateRequest({ + name: REAL_MODEL_ID, + strategy: "priority", + models: [ + { providerId: "codex", model: REAL_MODEL_ID }, + { providerId: "openai", model: REAL_MODEL_ID }, + ], + }) + ); + const body = (await response.json()) as ComboResponseBody; + + // Legitimate/sanctioned shape MUST still pass — never a 4xx (would regress #6940). + assert.equal(response.status, 201); + assert.equal(body.name, REAL_MODEL_ID); + const stored = await combosDb.getComboByName(REAL_MODEL_ID); + assert.equal(stored?.name, REAL_MODEL_ID); + + // But it is now observable via a non-blocking warning. + assert.equal(body.warning?.code, "COMBO_NAME_SHADOWS_MODEL"); + assert.equal(body.warning?.modelId, REAL_MODEL_ID); + assert.equal(typeof body.warning?.providerId, "string"); +}); + +test("POST /api/combos: non-colliding name is created with no warning field at all", async () => { + const response = await createRoute.POST( + makeCreateRequest({ + name: NON_COLLIDING_NAME, + strategy: "priority", + models: [{ providerId: "claude", model: "claude-sonnet-4-6" }], + }) + ); + const body = (await response.json()) as ComboResponseBody; + + assert.equal(response.status, 201); + assert.equal(body.name, NON_COLLIDING_NAME); + assert.equal("warning" in body, false); +}); + +test("PUT /api/combos/[id]: renaming to a real model id is applied (#6940 pattern), with a warning", async () => { + const combo = await combosDb.createCombo({ + name: "claude-plain", + models: [{ provider: "claude", model: "claude-sonnet-4-6" }], + }); + + const response = await comboRoute.PUT(makeUpdateRequest({ name: REAL_MODEL_ID }), { + params: Promise.resolve({ id: combo.id }), + }); + const body = (await response.json()) as ComboResponseBody; + + // Legitimate/sanctioned rename MUST still pass — never a 4xx. + assert.equal(response.status, 200); + assert.equal(body.name, REAL_MODEL_ID); + const stored = await combosDb.getComboByName(REAL_MODEL_ID); + assert.equal(stored?.id, combo.id); + + assert.equal(body.warning?.code, "COMBO_NAME_SHADOWS_MODEL"); + assert.equal(body.warning?.modelId, REAL_MODEL_ID); +}); + +test("PUT /api/combos/[id]: renaming to a non-colliding name has no warning field", async () => { + const combo = await combosDb.createCombo({ + name: "claude-plain", + models: [{ provider: "claude", model: "claude-sonnet-4-6" }], + }); + + const response = await comboRoute.PUT(makeUpdateRequest({ name: NON_COLLIDING_NAME }), { + params: Promise.resolve({ id: combo.id }), + }); + const body = (await response.json()) as ComboResponseBody; + + assert.equal(response.status, 200); + assert.equal(body.name, NON_COLLIDING_NAME); + assert.equal("warning" in body, false); +}); + +test("scanCombosForModelCollisions: reports existing combos that shadow a real model id", () => { + const results = collision.scanCombosForModelCollisions([ + { name: REAL_MODEL_ID }, + { name: NON_COLLIDING_NAME }, + { name: "" }, + ]); + assert.equal(results.length, 1); + assert.equal(results[0].comboName, REAL_MODEL_ID); + assert.equal(results[0].modelId, REAL_MODEL_ID); +}); + +test("findCollidingModel: returns null for a name with no real-model-id collision", () => { + assert.equal(collision.findCollidingModel(NON_COLLIDING_NAME), null); +}); + +test("scanComboModelNameCollisionsAtBoot: logs a startup warning enumerating existing collisions", async () => { + await combosDb.createCombo({ + name: REAL_MODEL_ID, + models: [{ provider: "codex", model: REAL_MODEL_ID }], + }); + await combosDb.createCombo({ + name: NON_COLLIDING_NAME, + models: [{ provider: "claude", model: "claude-sonnet-4-6" }], + }); + + const { scanComboModelNameCollisionsAtBoot } = await import("../../src/instrumentation-node.ts"); + const originalWarn = console.warn; + const warnings: unknown[][] = []; + console.warn = (...args: unknown[]) => { + warnings.push(args); + }; + try { + await scanComboModelNameCollisionsAtBoot(); + } finally { + console.warn = originalWarn; + } + + const collisionWarning = warnings.find((args) => + String(args[0]).includes("share a name with a real model id (#8530)") + ); + assert.ok(collisionWarning, "expected a startup warning about the model-name collision"); + assert.ok(String(collisionWarning![0]).includes(REAL_MODEL_ID)); + assert.ok( + !String(collisionWarning![0]).includes(NON_COLLIDING_NAME), + "non-colliding combo must not be reported" + ); +});