From 7e55abbc418681761df373da09b84979efeba364 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 4 Aug 2026 21:36:34 -0300 Subject: [PATCH] fix(vision-bridge): do not select unreachable describe-model when no vision provider is connected (#8430) --- changelog.d/fixes/8430-fix.plan.md | 3 + src/lib/guardrails/visionBridge.ts | 13 +++ src/lib/guardrails/visionBridgeHelpers.ts | 8 +- src/lib/guardrails/visionBridgeRouter.ts | 24 ++++-- .../guardrails/visionBridgeRouter.test.ts | 5 +- tests/unit/repro-8430.test.ts | 79 +++++++++++++++++++ ...on-bridge-preserve-on-failure-4012.test.ts | 35 +++++--- 7 files changed, 144 insertions(+), 23 deletions(-) create mode 100644 changelog.d/fixes/8430-fix.plan.md create mode 100644 tests/unit/repro-8430.test.ts diff --git a/changelog.d/fixes/8430-fix.plan.md b/changelog.d/fixes/8430-fix.plan.md new file mode 100644 index 0000000000..c184b6ed85 --- /dev/null +++ b/changelog.d/fixes/8430-fix.plan.md @@ -0,0 +1,3 @@ +- fix(vision-bridge): describe-model no longer returns unreachable "openai/gpt-4o-mini" when every vision-capable provider is unreachable on the instance — returns null instead and surfaces a clear error (#8430) +- fix(vision-bridge): validate fixedModel against usable credentials before short-circuiting in getBestVisionModel, so the default "openai/gpt-4o-mini" is not unconditionally selected when no OpenAI connection exists (#8430) +- fix(vision-bridge): in the combo describe path, replace raw images with an error text stub when all describe attempts fail, instead of forwarding images to a confirmed non-vision backend that would reject them with an opaque serde error (#8430) diff --git a/src/lib/guardrails/visionBridge.ts b/src/lib/guardrails/visionBridge.ts index 034b0d8486..11f7555197 100644 --- a/src/lib/guardrails/visionBridge.ts +++ b/src/lib/guardrails/visionBridge.ts @@ -311,6 +311,19 @@ export class VisionBridgeGuardrail extends BaseGuardrail { return null; }); + // 12b. (#8430) When every describe call failed (all null descriptions) in + // the combo describe path, the upstream is a confirmed non-vision model that + // cannot process raw images — replacing them with an "(unavailable)" stub + // is safe here because the upstream can only handle text. The original #4012 + // preserve-raw behavior only applies to paths where the upstream might still + // be vision-capable (reroute path / unknown capability). + const allNull = descriptions.every((d) => d === null); + if (allNull && comboVisionBridgeDecision === "process") { + for (let i = 0; i < descriptions.length; i++) { + descriptions[i] = `[Image ${i + 1}]: (unavailable — no vision-capable provider connected)`; + } + } + // 13. Replace image parts with text descriptions (null → keep original image) const modifiedBody = replaceImageParts( body as Parameters[0], diff --git a/src/lib/guardrails/visionBridgeHelpers.ts b/src/lib/guardrails/visionBridgeHelpers.ts index bce8acf629..207ae43ea0 100644 --- a/src/lib/guardrails/visionBridgeHelpers.ts +++ b/src/lib/guardrails/visionBridgeHelpers.ts @@ -212,11 +212,17 @@ export async function callVisionModel( apiKey?: string, routerConfig?: Partial ): Promise { - // Auto-select the best vision model if not explicitly configured + // Auto-select the best vision model const modelToUse = await getBestVisionModel({ fixedModel: config.model, ...routerConfig, }); + // (#8430) When no vision-capable provider has usable credentials on this + // instance, surface a clear error instead of attempting a describe call that + // would fail with an opaque auth/serde error upstream. + if (!modelToUse) { + throw new Error("No vision-capable provider connected, cannot process image request"); + } let lastError: Error | null = null; // Try primary model + fallbacks diff --git a/src/lib/guardrails/visionBridgeRouter.ts b/src/lib/guardrails/visionBridgeRouter.ts index 3b4bbafd5f..9ea04025e9 100644 --- a/src/lib/guardrails/visionBridgeRouter.ts +++ b/src/lib/guardrails/visionBridgeRouter.ts @@ -209,17 +209,29 @@ function selectBestModel( /** * Get the best vision model for image description. - * Respects fixed model override if configured. + * Respects fixed model override if configured, but validates it has usable + * credentials before short-circuiting — a fixedModel that is confirmed + * unreachable on this instance falls through to auto-selection. + * Returns `null` when no vision-capable candidate has usable credentials. */ export async function getBestVisionModel( config: Partial = {}, deps: VisionBridgeRouterDeps = {} -): Promise { +): Promise { const fullConfig = { ...DEFAULT_ROUTER_CONFIG, ...config }; - // If fixed model is configured, use it + // 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) { - return fullConfig.fixedModel; + const checkCreds = deps.hasUsableCredentials ?? hasUsableCredentialsForModel; + const usable = await checkCreds(fullConfig.fixedModel); + // Only skip credential validation when the check is indeterminate (null). + // A confirmed `false` means fall through to auto-selection. + if (usable !== false) { + return fullConfig.fixedModel; + } } // Check selection cache — key includes excluded models to prevent cache pollution @@ -240,8 +252,8 @@ export async function getBestVisionModel( const best = selectBestModel(candidates, fullConfig); if (!best) { - // Fallback to default - return "openai/gpt-4o-mini"; + // No vision-capable candidate has usable credentials on this instance + return null; } // Cache the selection diff --git a/tests/unit/guardrails/visionBridgeRouter.test.ts b/tests/unit/guardrails/visionBridgeRouter.test.ts index 1712e30d7c..40154fffe1 100644 --- a/tests/unit/guardrails/visionBridgeRouter.test.ts +++ b/tests/unit/guardrails/visionBridgeRouter.test.ts @@ -64,13 +64,12 @@ 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 -> the hardcoded last-resort default is returned - // instead of an unreachable pick. + // no candidate survives -> returns null instead of an unreachable default. const model = await getBestVisionModel( {}, { hasUsableCredentials: async () => false } ); - assert.equal(model, "openai/gpt-4o-mini"); + assert.equal(model, null); }); test( diff --git a/tests/unit/repro-8430.test.ts b/tests/unit/repro-8430.test.ts new file mode 100644 index 0000000000..f64f04617d --- /dev/null +++ b/tests/unit/repro-8430.test.ts @@ -0,0 +1,79 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { VisionBridgeGuardrail } = await import("../../src/lib/guardrails/visionBridge.ts"); +const { resetGuardrailsForTests } = await import("../../src/lib/guardrails/registry.ts"); +const { getBestVisionModel } = await import("../../src/lib/guardrails/visionBridgeRouter.ts"); +import type { GuardrailContext } from "../../src/lib/guardrails/base.ts"; +import type { VisionModelConfig } from "../../src/lib/guardrails/visionBridgeHelpers.ts"; + +const mockSettings: Record = { + visionBridgeEnabled: true, + visionBridgePrompt: "Describe this image concisely.", + visionBridgeTimeout: 30000, + visionBridgeMaxImages: 10, +}; + +function createGuardrail(options?: Parameters[0]) { + return new VisionBridgeGuardrail({ + ...options, + deps: { + getSettings: async () => mockSettings, + callVisionModel: async (_i: string, _c: VisionModelConfig) => { + throw new Error("Vision API error 401: Missing API key"); + }, + hasUsableCredentials: async () => false, + ...(options?.deps ?? {}), + }, + }); +} + +function createContext(o: Partial = {}): GuardrailContext { + return { model: "deepseek/deepseek-v4-pro", log: console, ...o }; +} + +function createPayload(o: Record = {}): Record { + return { + model: "deepseek/deepseek-v4-pro", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is in this image?" }, + { type: "image_url", image_url: { url: "https://example.com/image.png" } }, + ], + }, + ], + ...o, + }; +} + +test.beforeEach(() => { resetGuardrailsForTests({ registerDefaults: false }); }); + +test("8430a: getBestVisionModel returns null when every vision-capable candidate is unusable", async () => { + const model = await getBestVisionModel({}, { hasUsableCredentials: async () => false }); + assert.strictEqual(model, null, `no vision provider reachable, but returned unreachable '${model}'`); +}); + +test("8430b: fixedModel describe-path target must not be an unreachable model", async () => { + const model = await getBestVisionModel( + { fixedModel: "openai/gpt-4o-mini" }, + { hasUsableCredentials: async () => false } + ); + assert.strictEqual(model, null, `fixedModel short-circuit returned unreachable '${model}'`); +}); + +test("8430c: describe path does not forward raw image when no vision provider is reachable", async () => { + const guardrail = createGuardrail({ + deps: { checkModelHasComboMapping: async (_m: string) => true }, + }); + const result = await guardrail.preCall(createPayload(), createContext()); + assert.strictEqual(result.block, false); + assert.ok(result.modifiedPayload, "expected a modified payload"); + const modified = result.modifiedPayload as { + messages: Array<{ content: Array<{ type: string; text?: string }> }>; + }; + const content = modified.messages[0].content; + const imagePart = content.find((p) => p.type === "image_url" || p.type === "image"); + assert.strictEqual(imagePart, undefined, "raw image forwarded with no clear error (ask #2 unimplemented)"); +}); \ No newline at end of file diff --git a/tests/unit/vision-bridge-preserve-on-failure-4012.test.ts b/tests/unit/vision-bridge-preserve-on-failure-4012.test.ts index 0804d8bb9f..ca4bcb0354 100644 --- a/tests/unit/vision-bridge-preserve-on-failure-4012.test.ts +++ b/tests/unit/vision-bridge-preserve-on-failure-4012.test.ts @@ -1,14 +1,20 @@ /** - * Regression test for #4012 — Nvidia NIM (and any vision-capable model whose - * capability OmniRoute can't prove) via OmniRoute fails to process image inputs. + * Regression test for #4012 / #8430 — Nvidia NIM (and any vision-capable model + * whose capability OmniRoute can't prove) via OmniRoute fails to process image + * inputs. * - * The Vision Bridge is enabled by default. For a model with unknown - * (`null`) vision capability it engages, tries to describe each image with the - * configured vision model, and on a FAILED describe call it replaced the image - * with the literal text "[Image N]: (unavailable)" — silently destroying the - * original image so the (actually vision-capable) upstream answered - * "Image unavailable". A describe failure must NOT be destructive: the original - * image must survive so a vision-capable upstream can still see it. + * SEMANTIC CHANGE (#8430): In the combo describe path, when ALL describe calls + * fail (no vision-capable provider reachable on this instance), the raw image + * is now replaced with an error text stub instead of being preserved. This is + * safe because the combo describe path is only reached for models/targets that + * are confirmed non-vision-capable — forwarding a raw image to a text-only + * backend would produce an opaque serde error like `[400] unknown variant + * image_url, expected text`. The original #4012 preserve-raw behavior is + * maintained for the reroute path (not-combo / auto models with unknown vision + * capability), where the upstream model might still be vision-capable. + * + * Previous behavior: describe failure → preserve original image_url part + * Current behavior: total describe failure → replace with error text stub */ import test from "node:test"; import assert from "node:assert/strict"; @@ -52,7 +58,7 @@ function imagePayload() { const ctx = { model: "nvidia/google/diffusiongemma-26b-a4b-it", log: console } as never; -test("#4012 describe failure preserves the original image instead of dropping it", async () => { +test("#4012/#8430 describe failure replaces image with error text stub (combo describe path)", async () => { const guardrail = makeGuardrail(true); const result = await guardrail.preCall(imagePayload(), ctx); @@ -62,11 +68,14 @@ test("#4012 describe failure preserves the original image instead of dropping it }; const content = modified.messages[0].content; + // (#8430) In the combo describe path, total describe failure stubs the image + // instead of preserving it, because the upstream cannot handle raw images. const imagePart = content.find((p) => p.type === "image_url"); - assert.ok(imagePart, "original image_url part must be preserved when the describe call fails"); + assert.equal(imagePart, undefined, "raw image_url must be replaced when no vision provider is reachable"); - const unavailable = content.find((p) => p.type === "text" && p.text?.includes("(unavailable)")); - assert.equal(unavailable, undefined, "must NOT replace the image with an '(unavailable)' stub"); + // The describe stub should contain the unavailable message + const stub = content.find((p) => p.type === "text" && p.text?.includes("unavailable")); + assert.ok(stub, "an error stub should be present when describe fails in the combo path"); }); test("#4012 successful describe still replaces the image with its text description", async () => {