diff --git a/changelog.d/fixes/13951-combo-update-invariants.md b/changelog.d/fixes/13951-combo-update-invariants.md new file mode 100644 index 0000000000..1ca2676849 --- /dev/null +++ b/changelog.d/fixes/13951-combo-update-invariants.md @@ -0,0 +1 @@ +- fix(combos): synchronize allowedProviders and allow invariant override when updating combos from dashboard ([#13951](https://github.com/diegosouzapw/OmniRoute/pull/13951)) diff --git a/src/app/(dashboard)/dashboard/combos/page.tsx b/src/app/(dashboard)/dashboard/combos/page.tsx index fc30b8b037..d93a2d3442 100644 --- a/src/app/(dashboard)/dashboard/combos/page.tsx +++ b/src/app/(dashboard)/dashboard/combos/page.tsx @@ -84,6 +84,8 @@ import { } from "@/lib/combos/intelligentRouting"; import { getComboStepTarget } from "@/lib/combos/steps"; import { DEAD_COMBO_CONFIG_KEYS } from "@/lib/combos/deadConfigKeys"; +import { modelFamily } from "@/lib/combos/invariants"; +import { resolveCanonicalProviderModel } from "@omniroute/open-sse/services/model.ts"; import { resolveServerErrorMessage } from "@/lib/api/serverErrorMessage"; import { useTranslations } from "next-intl"; @@ -653,6 +655,55 @@ function normalizeModelEntry(entry) { }; } +/** + * On an existing-combo edit, work out how the dashboard save should synchronize + * `allowedProviders` / `allowedModelFamilies` against the combo's new step list so + * adding a step across providers never triggers COMBO_008 (#13951). Both restrictions + * are only ever WIDENED or left untouched here — never synthesized from no restriction, + * and never wiped just because the combo happens to have one. + */ +function computeAllowedRestrictionSync( + isEdit: boolean, + combo: { allowedProviders?: unknown; allowedModelFamilies?: unknown } | null | undefined, + models: Array<{ providerId?: string; model?: string }> +): { allowedProviders?: string[]; allowedModelFamilies?: null; overrideAllowedProviders?: true } { + if (!isEdit) return {}; + const result: { + allowedProviders?: string[]; + allowedModelFamilies?: null; + overrideAllowedProviders?: true; + } = { overrideAllowedProviders: true }; + + const existingProviders = Array.isArray(combo?.allowedProviders) ? combo.allowedProviders : []; + if (existingProviders.length > 0) { + const stepProviders = models + .map((m) => { + if (m.providerId) return m.providerId; + if (typeof m.model !== "string" || !m.model.includes("/")) return ""; + const [aliasOrProvider, ...rest] = m.model.split("/"); + return resolveCanonicalProviderModel(aliasOrProvider, rest.join("/")).provider || ""; + }) + .filter((p): p is string => Boolean(p)); + result.allowedProviders = Array.from(new Set([...existingProviders, ...stepProviders])); + } + + // Only clear the family restriction when a new step actually violates it (#13951). + const existingFamilies = Array.isArray(combo?.allowedModelFamilies) + ? combo.allowedModelFamilies + : []; + if (existingFamilies.length > 0) { + const allowedFamilies = new Set(existingFamilies); + const stepViolatesFamilies = models.some((m) => { + const family = typeof m.model === "string" ? modelFamily(m.model) : null; + return !family || !allowedFamilies.has(family); + }); + if (stepViolatesFamilies) result.allowedModelFamilies = null; + } + + return result; +} + + function getModelString(entry) { if (typeof entry === "string") return entry; if (entry?.kind === "combo-ref") return entry.comboName; @@ -3027,6 +3078,10 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo strategy, }; + // When editing an existing combo from the dashboard form, synchronize allowedProviders + // and clear legacy family restrictions so adding steps across providers never triggers COMBO_008 + Object.assign(saveData, computeAllowedRestrictionSync(isEdit, combo, models)); + // Per-combo description (#5005). Free-text, optional, persisted in combo data. if (description.trim()) { saveData.description = description.trim(); diff --git a/src/app/api/combos/[id]/route.ts b/src/app/api/combos/[id]/route.ts index 04f39a35b1..4c6d0d5bba 100644 --- a/src/app/api/combos/[id]/route.ts +++ b/src/app/api/combos/[id]/route.ts @@ -6,6 +6,7 @@ import { syncToCloud } from "@/lib/cloudSync"; import { validateCompositeTiersConfig } from "@/lib/combos/compositeTiers"; import { normalizeComboModels } from "@/lib/combos/steps"; import { validateComboDAG, clampComboDepth } from "@omniroute/open-sse/services/combo.ts"; +import { resolveCanonicalProviderModel } from "@omniroute/open-sse/services/model.ts"; import { updateComboSchema } from "@/shared/validation/schemas"; import { requiresQuotaOnlyComboRefExecute } from "@/shared/validation/schemas/combo"; import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; @@ -137,6 +138,32 @@ export async function PUT(request, { params }) { }), } : normalizedUpdate; + + if (body.overrideAllowedProviders === true) { + delete body.overrideAllowedProviders; + const currentProviders = Array.isArray(currentCombo.allowedProviders) + ? currentCombo.allowedProviders + : []; + // Only widen an EXISTING restriction (#13951/COMBO_008). When the combo + // currently has no allowedProviders restriction, currentProviders is + // empty and unioning it with the new step providers would synthesize a + // brand-new allowlist out of nothing — the opposite of "no restriction". + if (body.models && body.allowedProviders === undefined && currentProviders.length > 0) { + const stepProviders = ( + body.models as Array<{ providerId?: string; provider?: string; model?: string }> + ) + .map((m) => { + if (m.providerId) return m.providerId; + if (m.provider) return m.provider; + if (typeof m.model !== "string" || !m.model.includes("/")) return ""; + const [aliasOrProvider, ...rest] = m.model.split("/"); + return resolveCanonicalProviderModel(aliasOrProvider, rest.join("/")).provider || ""; + }) + .filter((p): p is string => Boolean(p)); + body.allowedProviders = Array.from(new Set([...currentProviders, ...stepProviders])); + } + } + const nextComboState = { ...currentCombo, ...body, diff --git a/src/lib/combos/invariants.ts b/src/lib/combos/invariants.ts index 302bf61296..6759443249 100644 --- a/src/lib/combos/invariants.ts +++ b/src/lib/combos/invariants.ts @@ -14,10 +14,19 @@ const FAMILY_PATTERNS: ReadonlyArray<[string, RegExp]> = [ ]; function strings(value: unknown): string[] { - return Array.isArray(value) ? value.filter((item): item is string => typeof item === "string") : []; + return Array.isArray(value) + ? value.filter((item): item is string => typeof item === "string") + : []; } -function modelFamily(model: string): string | null { +/** + * Detect the model "family" (gpt/claude/gemini/...) from a bare or + * provider-prefixed model id. Exported for callers that need to know + * whether a candidate step would actually violate an existing + * `allowedModelFamilies` restriction (#13951) rather than only the + * `validateComboInvariant` throw path below. + */ +export function modelFamily(model: string): string | null { const bare = model.slice(model.lastIndexOf("/") + 1); return FAMILY_PATTERNS.find(([, pattern]) => pattern.test(bare))?.[0] ?? null; } diff --git a/src/shared/validation/schemas/combo.ts b/src/shared/validation/schemas/combo.ts index 306b8da498..51ed4072ae 100644 --- a/src/shared/validation/schemas/combo.ts +++ b/src/shared/validation/schemas/combo.ts @@ -432,8 +432,9 @@ export const updateComboSchema = z // so the one endpoint a client can flip it through stripped the field and // a visibility-only update was rejected as empty. #12836 isHidden: z.boolean().optional(), - allowedProviders: z.array(z.string().trim().min(1).max(200)).max(100).optional(), - allowedModelFamilies: z.array(z.string().trim().min(1).max(100)).max(100).optional(), + allowedProviders: z.array(z.string().trim().min(1).max(200)).max(100).optional().nullable(), + allowedModelFamilies: z.array(z.string().trim().min(1).max(100)).max(100).optional().nullable(), + overrideAllowedProviders: z.boolean().optional(), // Nullable like `description` and `context_length` above: an absent field means // "leave unchanged" because updateCombo merges over the stored record, so clearing // one needs an explicit null for updateCombo's null-means-delete pass (#12158). diff --git a/tests/unit/combo-put-route-allowed-providers.test.ts b/tests/unit/combo-put-route-allowed-providers.test.ts new file mode 100644 index 0000000000..0fa837ce0c --- /dev/null +++ b/tests/unit/combo-put-route-allowed-providers.test.ts @@ -0,0 +1,85 @@ +// #13951 — route-level regression coverage for the PUT /api/combos/[id] +// overrideAllowedProviders sync path. tests/unit/combo-update-invariants.test.ts +// only exercises combosDb.updateCombo() directly, bypassing the PUT route +// branch this test targets. +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-put-route-")); +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 comboRoute = await import("../../src/app/api/combos/[id]/route.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +function put(id: string, body: Record) { + return new Request(`http://localhost/api/combos/${id}`, { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +test("PUT with overrideAllowedProviders on a combo with NO prior restriction stays unrestricted", async () => { + const combo = await combosDb.createCombo({ + name: "unrestricted-combo", + strategy: "priority", + models: [{ provider: "claude", model: "claude-sonnet-5" }], + }); + assert.ok(combo?.id); + assert.equal((combo as { allowedProviders?: string[] }).allowedProviders, undefined); + + const response = await comboRoute.PUT( + put(combo.id, { + name: "unrestricted-combo", + models: [ + { provider: "claude", model: "claude-sonnet-5" }, + { provider: "openai", model: "gpt-5" }, + ], + overrideAllowedProviders: true, + }), + { params: Promise.resolve({ id: combo.id }) } + ); + assert.equal(response.status, 200); + + const stored = (await combosDb.getComboById(combo.id)) as { allowedProviders?: string[] }; + // The combo had no restriction before the edit — it must still have none + // afterwards. Synthesizing allowedProviders=["claude","openai"] here would + // be the #13951 regression: a later add-a-provider update would start + // failing COMBO_008 where it previously succeeded. + assert.equal(stored.allowedProviders, undefined); +}); + +test("PUT with overrideAllowedProviders on a combo with an EXISTING restriction unions the new step providers", async () => { + const combo = await combosDb.createCombo({ + name: "restricted-combo", + strategy: "priority", + allowedProviders: ["claude"], + models: [{ provider: "claude", model: "claude-sonnet-5" }], + }); + assert.ok(combo?.id); + + const response = await comboRoute.PUT( + put(combo.id, { + name: "restricted-combo", + models: [ + { provider: "claude", model: "claude-sonnet-5" }, + { provider: "openai", model: "gpt-5" }, + ], + overrideAllowedProviders: true, + }), + { params: Promise.resolve({ id: combo.id }) } + ); + assert.equal(response.status, 200); + + const stored = (await combosDb.getComboById(combo.id)) as { allowedProviders?: string[] }; + assert.deepEqual([...(stored.allowedProviders ?? [])].sort(), ["claude", "openai"]); +}); diff --git a/tests/unit/combo-update-invariants.test.ts b/tests/unit/combo-update-invariants.test.ts new file mode 100644 index 0000000000..342b397e69 --- /dev/null +++ b/tests/unit/combo-update-invariants.test.ts @@ -0,0 +1,89 @@ +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 { updateComboSchema } from "../../src/shared/validation/schemas/combo.ts"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-combo-invariants-")); +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"); + +async function resetStorage() { + core.resetDbInstance(); + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + core.resetDbInstance(); + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } +}); + +test("updateComboSchema accepts nullable allowedProviders, allowedModelFamilies, and overrideAllowedProviders", () => { + const parsedNulls = updateComboSchema.safeParse({ + allowedProviders: null, + allowedModelFamilies: null, + overrideAllowedProviders: true, + }); + assert.equal(parsedNulls.success, true); + if (parsedNulls.success) { + assert.equal(parsedNulls.data.allowedProviders, null); + assert.equal(parsedNulls.data.allowedModelFamilies, null); + assert.equal(parsedNulls.data.overrideAllowedProviders, true); + } + + const parsedArray = updateComboSchema.safeParse({ + allowedProviders: ["claude", "antigravity"], + allowedModelFamilies: ["claude"], + }); + assert.equal(parsedArray.success, true); +}); + +test("updateCombo allows updating allowedProviders and clearing with null", async () => { + const combo = await combosDb.createCombo({ + name: "claude-combo", + allowedProviders: ["claude"], + models: [{ provider: "claude", model: "claude-sonnet-5" }], + }); + assert.ok(combo?.id); + + // Updating models to include a new provider with expanded allowedProviders succeeds + const updated = await combosDb.updateCombo(String(combo.id), { + allowedProviders: ["claude", "antigravity"], + models: [ + { provider: "claude", model: "claude-sonnet-5" }, + { provider: "antigravity", model: "claude-sonnet-4-6" }, + ], + }); + assert.ok(updated); + const typedUpdated = updated as { + allowedProviders?: string[]; + models: Array<{ providerId?: string }>; + }; + assert.deepEqual(typedUpdated.allowedProviders, ["claude", "antigravity"]); + assert.equal(typedUpdated.models.length, 2); + + // Clearing allowedProviders with null succeeds and removes the invariant restriction + const cleared = await combosDb.updateCombo(String(combo.id), { + allowedProviders: null, + models: [{ provider: "openrouter", model: "nvidia/nemotron-3.5-lightning:free" }], + }); + assert.ok(cleared); + const typedCleared = cleared as { + allowedProviders?: string[]; + models: Array<{ providerId?: string }>; + }; + assert.equal(typedCleared.allowedProviders, undefined); + assert.equal(typedCleared.models[0]?.providerId, "openrouter"); +});