diff --git a/changelog.d/fixes/12373-vision-bridge-auto-combo-guard.md b/changelog.d/fixes/12373-vision-bridge-auto-combo-guard.md new file mode 100644 index 0000000000..033324bfc5 --- /dev/null +++ b/changelog.d/fixes/12373-vision-bridge-auto-combo-guard.md @@ -0,0 +1 @@ +- **fix(guardrails):** keep `auto`/`auto/*` virtual combos exempt from the Vision Bridge `fixedModel` credential guard so a combo target is passed through instead of silently falling back to global auto-selection ([#12237](https://github.com/diegosouzapw/OmniRoute/issues/12237)) diff --git a/src/lib/guardrails/visionBridgeRouter.ts b/src/lib/guardrails/visionBridgeRouter.ts index 5660f1e557..c2dbbc9121 100644 --- a/src/lib/guardrails/visionBridgeRouter.ts +++ b/src/lib/guardrails/visionBridgeRouter.ts @@ -231,7 +231,9 @@ async function getVisionCapableModels( }; }); - return candidates.filter((candidate): candidate is VisionModelCandidate => candidate !== null); + return candidates.filter( + (candidate): candidate is VisionModelCandidate => candidate !== null + ); }) ); @@ -271,6 +273,51 @@ function selectBestModel( return scored[0]; } +/** + * (#12237) `auto` / `auto/*` ids are VIRTUAL combos: there is no provider + * row for "auto", so the credential check always reports `false` for them. + * Member-level credentials are enforced downstream when the combo + * dispatches (mirrors the reroute guard in visionBridge.ts), so a virtual + * combo must not be discarded by the #8430 short-circuit — otherwise the + * combo silently falls through to auto-selection and never rotates. It is + * still subject to the pool check in `getBestVisionModel`: when the ENTIRE + * vision pool is unusable there is nothing the combo could dispatch to, and + * returning the combo id would let a raw image reach a text-only backend + * (#8430). + * + * Returns the combo id when `fixedModel` is virtual, `undefined` otherwise. + */ +function resolveVirtualCombo(fixedModel: string | undefined): string | undefined { + return fixedModel === "auto" || fixedModel?.startsWith("auto/") ? fixedModel : undefined; +} + +/** + * Resolve a live selection-cache entry for `cacheKey`. + * + * Returns the id to hand back: the cached member for a concrete target, or + * `virtualCombo` once the cached member proves it still has usable + * credentials (the cache never re-validates credentials, and the caller + * exempts virtual combos from that check). A missing or expired entry yields + * `null`; an entry whose member is no longer available or usable is dropped + * so the pool is rescanned. + */ +async function resolveCachedSelection( + cacheKey: string, + virtualCombo: string | undefined, + deps: VisionBridgeRouterDeps +): Promise { + const cached = selectionCache.get(cacheKey); + if (!cached || cached.expiresAt <= Date.now()) return null; + + if (await cachedModelRemainsAvailable(cached.modelId, deps)) { + if (!virtualCombo) return cached.modelId; + const checkCreds = deps.hasUsableCredentials ?? hasUsableCredentialsForModel; + if ((await checkCreds(cached.modelId)) !== false) return virtualCombo; + } + selectionCache.delete(cacheKey); + return null; +} + /** * Get the best vision model for image description. * Respects fixed model override if configured, but validates it has usable @@ -283,12 +330,15 @@ export async function getBestVisionModel( deps: VisionBridgeRouterDeps = {} ): Promise { const fullConfig = { ...DEFAULT_ROUTER_CONFIG, ...config }; + const virtualCombo = resolveVirtualCombo(fullConfig.fixedModel); // If fixed model is configured, validate it has usable credentials first. // (#8430) An unreachable fixedModel (e.g. the default "openai/gpt-4o-mini" // on an instance with no OpenAI connection/key) must not short-circuit the // credential check — fall through to auto-selection instead. - if (fullConfig.fixedModel) { + // (#12237) A virtual combo is exempt here and goes through the pool + // selection below instead; see `resolveVirtualCombo`. + if (fullConfig.fixedModel && !virtualCombo) { const checkCreds = deps.hasUsableCredentials ?? hasUsableCredentialsForModel; const usable = await checkCreds(fullConfig.fixedModel); // Only skip credential validation when the check is indeterminate (null). @@ -304,13 +354,8 @@ export async function getBestVisionModel( fullConfig.excludedModels.length > 0 ? `excl:${[...fullConfig.excludedModels].sort().join(",")}` : "default"; - const cached = selectionCache.get(cacheKey); - if (cached && cached.expiresAt > Date.now()) { - if (await cachedModelRemainsAvailable(cached.modelId, deps)) { - return cached.modelId; - } - selectionCache.delete(cacheKey); - } + const cachedPick = await resolveCachedSelection(cacheKey, virtualCombo, deps); + if (cachedPick) return cachedPick; // Get all vision-capable candidates const candidates = await getVisionCapableModels(deps); @@ -329,7 +374,9 @@ export async function getBestVisionModel( expiresAt: Date.now() + fullConfig.selectionCacheTtlMs, }); - return best.fullName; + // A virtual combo is returned as-is once the pool proves at least one + // vision-capable member is usable; it rotates its own members downstream. + return virtualCombo ?? best.fullName; } /** diff --git a/tests/unit/guardrails/visionBridgeRouter.test.ts b/tests/unit/guardrails/visionBridgeRouter.test.ts index c520aa834d..7c7f75052a 100644 --- a/tests/unit/guardrails/visionBridgeRouter.test.ts +++ b/tests/unit/guardrails/visionBridgeRouter.test.ts @@ -80,8 +80,65 @@ test("getBestVisionModel — should exclude specified models", async () => { test("getBestVisionModel — excludes a candidate with no usable active connection", async () => { // Every candidate reports a confirmed-unusable connection (`false`) -> // no candidate survives -> returns null instead of an unreachable default. + const model = await getBestVisionModel({}, { hasUsableCredentials: async () => false }); + assert.equal(model, null); +}); + +// `auto` / `auto/*` ids are VIRTUAL combos: there is no provider row for +// "auto", so hasUsableCredentialsForModel reports a confirmed `false` for the +// combo id itself while the pool members remain usable (indeterminate here). +const virtualComboOnlyUnusable = async (fullModelId: string) => + fullModelId === "auto" || fullModelId.startsWith("auto/") ? false : null; + +test("getBestVisionModel — keeps an auto/* virtual-combo fixedModel when its credential check is false (#12237)", async () => { + // The #8430 short-circuit must not discard the combo — member credentials + // are enforced downstream when the combo dispatches (same exemption as the + // reroute guard in visionBridge.ts). + const fixedModel = "auto/vision"; const model = await getBestVisionModel( - {}, + { fixedModel }, + { hasUsableCredentials: virtualComboOnlyUnusable } + ); + assert.equal(model, fixedModel); +}); + +test('getBestVisionModel — keeps a bare "auto" fixedModel when its credential check is false (#12237)', async () => { + const model = await getBestVisionModel( + { fixedModel: "auto" }, + { hasUsableCredentials: virtualComboOnlyUnusable } + ); + assert.equal(model, "auto"); +}); + +test("getBestVisionModel — keeps an auto/* virtual-combo fixedModel on a cached pool selection (#12237)", async () => { + // Warm the selection cache with a pool pick, then ask for the combo: the + // cache-hit branch must still hand back the combo, not the cached member. + const warm = await getBestVisionModel({}, { hasUsableCredentials: virtualComboOnlyUnusable }); + assert.ok(warm); + const model = await getBestVisionModel( + { fixedModel: "auto/vision" }, + { hasUsableCredentials: virtualComboOnlyUnusable } + ); + assert.equal(model, "auto/vision"); +}); + +test("getBestVisionModel — discards an auto/* virtual-combo fixedModel when the ENTIRE vision pool is unusable (#8430)", async () => { + // The exemption only bypasses the credential check on the virtual id. With + // no usable vision-capable member anywhere, the combo has nothing to + // dispatch to and must fall through to `null` so the caller describes + // instead of forwarding a raw image to a text-only backend. + const model = await getBestVisionModel( + { fixedModel: "auto/vision" }, + { hasUsableCredentials: async () => false } + ); + assert.equal(model, null); +}); + +test("getBestVisionModel — still falls through when a concrete fixedModel has no usable credentials (#8430)", async () => { + // Regression guard for the exemption above: a non-virtual fixedModel with + // a confirmed-unusable credential check must still be discarded. + const model = await getBestVisionModel( + { fixedModel: "openai/gpt-4o-mini" }, { hasUsableCredentials: async () => false } ); assert.equal(model, null); @@ -105,20 +162,17 @@ test("getBestVisionModel — does not query live catalogs for providers without assert.equal(catalogCalls, 0); }); -test( - "getBestVisionModel — selects a credentialed candidate over an uncredentialed higher-priority one", - async () => { - // openai (priority 50, would normally win) has no usable connection; - // every other vision-capable provider does. - const model = await getBestVisionModel( - {}, - { - hasUsableCredentials: async (fullModelId) => fullModelId.split("/")[0] !== "openai", - } - ); - assert.equal(model.startsWith("openai/"), false); - } -); +test("getBestVisionModel — selects a credentialed candidate over an uncredentialed higher-priority one", async () => { + // openai (priority 50, would normally win) has no usable connection; + // every other vision-capable provider does. + const model = await getBestVisionModel( + {}, + { + hasUsableCredentials: async (fullModelId) => fullModelId.split("/")[0] !== "openai", + } + ); + assert.equal(model.startsWith("openai/"), false); +}); test("getBestVisionModel — excludes static models missing from an authoritative live catalog", async () => { const model = await getBestVisionModel( @@ -188,17 +242,14 @@ test("getFallbackModels — should respect max fallback attempts", async () => { assert.ok(fallbacks.length <= 2); }); -test( - "getFallbackModels — does not include candidates with a confirmed-unusable connection", - async () => { - const fallbacks = await getFallbackModels( - "openai/gpt-4o-mini", - {}, - { hasUsableCredentials: async (fullModelId) => fullModelId.split("/")[0] !== "anthropic" } - ); - assert.ok(!fallbacks.some((m) => m.startsWith("anthropic/"))); - } -); +test("getFallbackModels — does not include candidates with a confirmed-unusable connection", async () => { + const fallbacks = await getFallbackModels( + "openai/gpt-4o-mini", + {}, + { hasUsableCredentials: async (fullModelId) => fullModelId.split("/")[0] !== "anthropic" } + ); + assert.ok(!fallbacks.some((m) => m.startsWith("anthropic/"))); +}); test("getFallbackModels — excludes fallbacks missing from an authoritative live catalog", async () => { const fallbacks = await getFallbackModels(