diff --git a/changelog.d/fixes/13927-combo-ref-vision-bridge.md b/changelog.d/fixes/13927-combo-ref-vision-bridge.md new file mode 100644 index 0000000000..d12977c8ab --- /dev/null +++ b/changelog.d/fixes/13927-combo-ref-vision-bridge.md @@ -0,0 +1 @@ +- fix(guardrails): resolve nested `combo-ref` steps to their real leaf models when deciding vision-bridge behavior, so a pass-through combo pointing at an all-vision-capable inner combo skips the describe-and-replace path instead of stripping raw images (#13927) diff --git a/src/lib/guardrails/visionBridge.ts b/src/lib/guardrails/visionBridge.ts index cdf4028cc0..e87a7c376e 100644 --- a/src/lib/guardrails/visionBridge.ts +++ b/src/lib/guardrails/visionBridge.ts @@ -31,11 +31,79 @@ import { isProviderConnectionUsable, hasUsableCredentialsForModel, } from "./visionBridgeCredentials"; +import { MAX_COMBO_DEPTH } from "@omniroute/open-sse/services/combo/comboPredicates.ts"; export { isProviderConnectionUsable, hasUsableCredentialsForModel }; type ComboVisionBridgeDecision = "process" | "skip" | "not-combo" | "no-vision"; +type LeafVisionTally = { hasVision: boolean; hasNonVision: boolean }; + +/// Evaluate a single `kind: "model"` step's proven vision capability. +/// Returns null when the step lacks a valid model string (malformed step). +function evaluateModelStepCapability(s: Record): "vision" | "non-vision" | null { + const targetModel = s.model; + if (typeof targetModel !== "string") return null; + const provider = + typeof s.providerId === "string" + ? s.providerId + : typeof s.provider === "string" + ? s.provider + : null; + const caps = getResolvedModelCapabilities({ provider, model: targetModel }); + return caps.supportsVision === true ? "vision" : "non-vision"; +} + +/// Recursively resolve a `combo-ref` step to its real leaf models' vision +/// capability, reusing the same MAX_COMBO_DEPTH guard as the flatten dispatch +/// path (open-sse/services/combo/comboStructure.ts) plus a visited-set cycle +/// guard, so this request-hot-path lookup can never recurse unbounded or loop +/// on a cyclic combo-ref chain. +/// +/// Unresolvable cases (combo not found, empty/invalid models, depth exceeded, +/// or a cycle) fall back to treating the combo-ref step as a single +/// non-vision-capable leaf -- conservative, but no longer forces the WHOLE +/// outer combo to "process" the way the old unconditional shortcut did. +async function resolveComboRefVisionCapability( + comboName: string, + visited: Set, + depth: number +): Promise { + const fallback: LeafVisionTally = { hasVision: false, hasNonVision: true }; + if (depth > MAX_COMBO_DEPTH || visited.has(comboName)) return fallback; + + const { getComboByName } = await import("@/lib/db/combos"); + const nestedCombo = await getComboByName(comboName); + if (!nestedCombo) return fallback; + + const nestedVisited = new Set(visited); + nestedVisited.add(comboName); + + const nestedRawModels = (nestedCombo as Record).models; + if (!Array.isArray(nestedRawModels) || nestedRawModels.length === 0) return fallback; + + const tally: LeafVisionTally = { hasVision: false, hasNonVision: false }; + let hasLeaf = false; + for (const step of nestedRawModels) { + const s = step as Record; + if (s.kind === "combo-ref" && typeof s.comboName === "string") { + hasLeaf = true; + const nested = await resolveComboRefVisionCapability(s.comboName, nestedVisited, depth + 1); + tally.hasVision = tally.hasVision || nested.hasVision; + tally.hasNonVision = tally.hasNonVision || nested.hasNonVision; + continue; + } + if (s.kind === "model") { + hasLeaf = true; + const capability = evaluateModelStepCapability(s); + if (capability === "vision") tally.hasVision = true; + else tally.hasNonVision = true; + } + } + + return hasLeaf ? tally : fallback; +} + export function resolveVisionComboName(mapping: Record): string | null { const comboName = mapping.comboName ?? mapping.name ?? null; return typeof comboName === "string" && comboName.length > 0 ? comboName : null; @@ -75,37 +143,43 @@ export async function getComboVisionBridgeDecision( if (!Array.isArray(rawModels)) return "process"; // 4. Check each target for vision support - // combo-ref → conservative (process images) + // combo-ref → recursively resolve the referenced combo's real leaf + // models (depth/cycle-guarded); unresolvable → conservative non-vision leaf // model step with no native vision → process images // all model steps with native vision → safe to skip // zero vision-capable model steps → "no-vision" (reroute-eligible) let hasModelStep = false; let hasVisionCapableStep = false; let hasNonVisionStep = false; + const rootComboName = + typeof (combo as Record).name === "string" + ? ((combo as Record).name as string) + : model; for (const step of rawModels) { const s = step as Record; - if (s.kind === "combo-ref") return "process"; + if (s.kind === "combo-ref") { + hasModelStep = true; + if (typeof s.comboName !== "string") { + hasNonVisionStep = true; + continue; + } + const nested = await resolveComboRefVisionCapability( + s.comboName, + new Set([rootComboName]), + 1 + ); + if (nested.hasVision) hasVisionCapableStep = true; + if (nested.hasNonVision) hasNonVisionStep = true; + continue; + } if (s.kind === "model") { hasModelStep = true; - const targetModel = s.model; - if (typeof targetModel === "string") { - const provider = - typeof s.providerId === "string" - ? s.providerId - : typeof s.provider === "string" - ? s.provider - : null; - const caps = getResolvedModelCapabilities({ - provider, - model: targetModel, - }); - if (caps.supportsVision === true) { - hasVisionCapableStep = true; - } else { - hasNonVisionStep = true; - } + const capability = evaluateModelStepCapability(s); + if (capability === null) return "process"; + if (capability === "vision") { + hasVisionCapableStep = true; } else { - return "process"; + hasNonVisionStep = true; } } } diff --git a/tests/unit/combo-ref-vision-bridge-13927.test.ts b/tests/unit/combo-ref-vision-bridge-13927.test.ts new file mode 100644 index 0000000000..6bb4fed682 --- /dev/null +++ b/tests/unit/combo-ref-vision-bridge-13927.test.ts @@ -0,0 +1,148 @@ +/** + * Repro probe for #13927: combo-ref hop strips image parts. + * + * getComboVisionBridgeDecision() treats ANY top-level `combo-ref` step as an + * unconditional "process" (describe images as text) without ever resolving + * the nested combo to see whether its actual leaf targets support vision. + * For a pass-through combo whose single member is a combo-ref to another + * combo made entirely of vision-capable models, this wrongly triggers the + * describe-and-replace path instead of "skip" (pass the image through raw), + * which is what happens for the flat inner combo or the leaf model called + * directly. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +process.env.DATA_DIR = `/tmp/omniroute-test-13927-${Date.now()}`; + +const { getComboVisionBridgeDecision } = await import("../../src/lib/guardrails/visionBridge.ts"); +const combosDb = await import("../../src/lib/db/combos.ts"); +const core = await import("../../src/lib/db/core.ts"); + +test.after(() => { + core.resetDbInstance(); +}); + +test("#13927: nested combo-ref to an all-vision-capable inner combo should resolve to skip, not process", async () => { + // Inner combo: flat, all leaf targets are proven vision-capable + // (command-code/gpt-5.5 is asserted vision-capable elsewhere, e.g. + // tests/unit/vision-bridge-native-skip.test.ts). + await combosDb.createCombo({ + name: "inner-vision-combo-13927", + models: [{ providerId: "command-code", model: "gpt-5.5", weight: 1 }], + }); + + // Outer combo: single member is a combo-ref to the inner (all-vision) combo + // -- exactly the "preset/default -> preset/deepseek-flash-latest" shape from + // the issue. + await combosDb.createCombo({ + name: "outer-passthrough-combo-13927", + models: [{ kind: "combo-ref", comboName: "inner-vision-combo-13927" }], + }); + + const innerDecision = await getComboVisionBridgeDecision("inner-vision-combo-13927"); + assert.equal( + innerDecision, + "skip", + "sanity: the flat inner combo (all vision-capable targets) must resolve to skip" + ); + + const outerDecision = await getComboVisionBridgeDecision("outer-passthrough-combo-13927"); + assert.equal( + outerDecision, + "skip", + "BUG #13927: outer combo whose only member is a combo-ref to an all-vision-capable " + + "combo must ALSO resolve to skip (image should pass through raw), not 'process' " + + "(which describes the image as text and drops the raw bytes before the leaf ever sees them)" + ); +}); + +test("#13927: combo-ref to a mixed-capability inner combo resolves to process", async () => { + // Inner combo mixes one vision-capable and one non-vision-capable leaf. + await combosDb.createCombo({ + name: "inner-mixed-combo-13927", + models: [ + { providerId: "command-code", model: "gpt-5.5", weight: 1 }, + { providerId: "mistral", model: "mistral-large-latest", weight: 1 }, + ], + }); + + await combosDb.createCombo({ + name: "outer-mixed-ref-combo-13927", + models: [{ kind: "combo-ref", comboName: "inner-mixed-combo-13927" }], + }); + + const decision = await getComboVisionBridgeDecision("outer-mixed-ref-combo-13927"); + assert.equal( + decision, + "process", + "a combo-ref to a mixed inner combo must resolve real leaf capabilities and still " + + "describe images (some real leaves cannot see them)" + ); +}); + +test("#13927: combo-ref to a zero-vision inner combo resolves to no-vision", async () => { + await combosDb.createCombo({ + name: "inner-zero-vision-combo-13927", + models: [{ providerId: "mistral", model: "mistral-large-latest", weight: 1 }], + }); + + await combosDb.createCombo({ + name: "outer-zero-vision-ref-combo-13927", + models: [{ kind: "combo-ref", comboName: "inner-zero-vision-combo-13927" }], + }); + + const decision = await getComboVisionBridgeDecision("outer-zero-vision-ref-combo-13927"); + assert.equal( + decision, + "no-vision", + "a combo-ref to an all-non-vision inner combo must behave like a text-only model " + + "(reroute-eligible), matching the flat-combo 'no-vision' outcome" + ); +}); + +test("#13927: circular combo-ref chain falls back safely and never throws", async () => { + // A -> B -> A. Neither combo has a real leaf model, only cyclic combo-refs, + // so both sides of the cycle hit the visited-set guard. + await combosDb.createCombo({ + name: "circular-a-13927", + models: [{ kind: "combo-ref", comboName: "circular-b-13927" }], + }); + await combosDb.createCombo({ + name: "circular-b-13927", + models: [{ kind: "combo-ref", comboName: "circular-a-13927" }], + }); + + await assert.doesNotReject(async () => { + const decision = await getComboVisionBridgeDecision("circular-a-13927"); + // Implementation-plan contract: an unresolvable/circular combo-ref step is + // folded in as a non-vision-capable leaf rather than forcing the whole + // combo back to "process" -- with no other real leaf present, that lands + // on "no-vision" (safe, reroute-eligible), never a crash. + assert.equal(decision, "no-vision"); + }); +}); + +test("#13927: circular combo-ref mixed with a real vision-capable leaf resolves to process", async () => { + // A real vision-capable leaf plus an unresolvable circular combo-ref: the + // circular hop still contributes a conservative non-vision leaf, so the + // combo is "mixed" and must describe images, not skip. + await combosDb.createCombo({ + name: "circular-c-13927", + models: [{ kind: "combo-ref", comboName: "circular-d-13927" }], + }); + await combosDb.createCombo({ + name: "circular-d-13927", + models: [{ kind: "combo-ref", comboName: "circular-c-13927" }], + }); + await combosDb.createCombo({ + name: "outer-circular-mixed-combo-13927", + models: [ + { providerId: "command-code", model: "gpt-5.5", weight: 1 }, + { kind: "combo-ref", comboName: "circular-c-13927" }, + ], + }); + + const decision = await getComboVisionBridgeDecision("outer-circular-mixed-combo-13927"); + assert.equal(decision, "process"); +});