mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-06 23:32:12 +03:00
fix(vision-bridge): do not select unreachable describe-model when no vision provider is connected (#8430)
This commit is contained in:
committed by
GitHub
parent
b0501642dd
commit
7e55abbc41
3
changelog.d/fixes/8430-fix.plan.md
Normal file
3
changelog.d/fixes/8430-fix.plan.md
Normal file
@@ -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)
|
||||||
@@ -311,6 +311,19 @@ export class VisionBridgeGuardrail extends BaseGuardrail {
|
|||||||
return null;
|
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)
|
// 13. Replace image parts with text descriptions (null → keep original image)
|
||||||
const modifiedBody = replaceImageParts(
|
const modifiedBody = replaceImageParts(
|
||||||
body as Parameters<typeof replaceImageParts>[0],
|
body as Parameters<typeof replaceImageParts>[0],
|
||||||
|
|||||||
@@ -212,11 +212,17 @@ export async function callVisionModel(
|
|||||||
apiKey?: string,
|
apiKey?: string,
|
||||||
routerConfig?: Partial<import("./visionBridgeRouter").VisionBridgeRouterConfig>
|
routerConfig?: Partial<import("./visionBridgeRouter").VisionBridgeRouterConfig>
|
||||||
): Promise<string> {
|
): Promise<string> {
|
||||||
// Auto-select the best vision model if not explicitly configured
|
// Auto-select the best vision model
|
||||||
const modelToUse = await getBestVisionModel({
|
const modelToUse = await getBestVisionModel({
|
||||||
fixedModel: config.model,
|
fixedModel: config.model,
|
||||||
...routerConfig,
|
...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;
|
let lastError: Error | null = null;
|
||||||
|
|
||||||
// Try primary model + fallbacks
|
// Try primary model + fallbacks
|
||||||
|
|||||||
@@ -209,17 +209,29 @@ function selectBestModel(
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Get the best vision model for image description.
|
* 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(
|
export async function getBestVisionModel(
|
||||||
config: Partial<VisionBridgeRouterConfig> = {},
|
config: Partial<VisionBridgeRouterConfig> = {},
|
||||||
deps: VisionBridgeRouterDeps = {}
|
deps: VisionBridgeRouterDeps = {}
|
||||||
): Promise<string> {
|
): Promise<string | null> {
|
||||||
const fullConfig = { ...DEFAULT_ROUTER_CONFIG, ...config };
|
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) {
|
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
|
// Check selection cache — key includes excluded models to prevent cache pollution
|
||||||
@@ -240,8 +252,8 @@ export async function getBestVisionModel(
|
|||||||
const best = selectBestModel(candidates, fullConfig);
|
const best = selectBestModel(candidates, fullConfig);
|
||||||
|
|
||||||
if (!best) {
|
if (!best) {
|
||||||
// Fallback to default
|
// No vision-capable candidate has usable credentials on this instance
|
||||||
return "openai/gpt-4o-mini";
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Cache the selection
|
// Cache the selection
|
||||||
|
|||||||
@@ -64,13 +64,12 @@ test("getBestVisionModel — should exclude specified models", async () => {
|
|||||||
|
|
||||||
test("getBestVisionModel — excludes a candidate with no usable active connection", async () => {
|
test("getBestVisionModel — excludes a candidate with no usable active connection", async () => {
|
||||||
// Every candidate reports a confirmed-unusable connection (`false`) ->
|
// Every candidate reports a confirmed-unusable connection (`false`) ->
|
||||||
// no candidate survives -> the hardcoded last-resort default is returned
|
// no candidate survives -> returns null instead of an unreachable default.
|
||||||
// instead of an unreachable pick.
|
|
||||||
const model = await getBestVisionModel(
|
const model = await getBestVisionModel(
|
||||||
{},
|
{},
|
||||||
{ hasUsableCredentials: async () => false }
|
{ hasUsableCredentials: async () => false }
|
||||||
);
|
);
|
||||||
assert.equal(model, "openai/gpt-4o-mini");
|
assert.equal(model, null);
|
||||||
});
|
});
|
||||||
|
|
||||||
test(
|
test(
|
||||||
|
|||||||
79
tests/unit/repro-8430.test.ts
Normal file
79
tests/unit/repro-8430.test.ts
Normal file
@@ -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<string, unknown> = {
|
||||||
|
visionBridgeEnabled: true,
|
||||||
|
visionBridgePrompt: "Describe this image concisely.",
|
||||||
|
visionBridgeTimeout: 30000,
|
||||||
|
visionBridgeMaxImages: 10,
|
||||||
|
};
|
||||||
|
|
||||||
|
function createGuardrail(options?: Parameters<typeof VisionBridgeGuardrail>[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> = {}): GuardrailContext {
|
||||||
|
return { model: "deepseek/deepseek-v4-pro", log: console, ...o };
|
||||||
|
}
|
||||||
|
|
||||||
|
function createPayload(o: Record<string, unknown> = {}): Record<string, unknown> {
|
||||||
|
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)");
|
||||||
|
});
|
||||||
@@ -1,14 +1,20 @@
|
|||||||
/**
|
/**
|
||||||
* Regression test for #4012 — Nvidia NIM (and any vision-capable model whose
|
* Regression test for #4012 / #8430 — Nvidia NIM (and any vision-capable model
|
||||||
* capability OmniRoute can't prove) via OmniRoute fails to process image inputs.
|
* 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
|
* SEMANTIC CHANGE (#8430): In the combo describe path, when ALL describe calls
|
||||||
* (`null`) vision capability it engages, tries to describe each image with the
|
* fail (no vision-capable provider reachable on this instance), the raw image
|
||||||
* configured vision model, and on a FAILED describe call it replaced the image
|
* is now replaced with an error text stub instead of being preserved. This is
|
||||||
* with the literal text "[Image N]: (unavailable)" — silently destroying the
|
* safe because the combo describe path is only reached for models/targets that
|
||||||
* original image so the (actually vision-capable) upstream answered
|
* are confirmed non-vision-capable — forwarding a raw image to a text-only
|
||||||
* "Image unavailable". A describe failure must NOT be destructive: the original
|
* backend would produce an opaque serde error like `[400] unknown variant
|
||||||
* image must survive so a vision-capable upstream can still see it.
|
* 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 test from "node:test";
|
||||||
import assert from "node:assert/strict";
|
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;
|
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 guardrail = makeGuardrail(true);
|
||||||
const result = await guardrail.preCall(imagePayload(), ctx);
|
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;
|
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");
|
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)"));
|
// The describe stub should contain the unavailable message
|
||||||
assert.equal(unavailable, undefined, "must NOT replace the image with an '(unavailable)' stub");
|
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 () => {
|
test("#4012 successful describe still replaces the image with its text description", async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user