From 912ff8d1c4a181eef62f003e4b8fb64ab6e3662a Mon Sep 17 00:00:00 2001 From: Hernan Javier Ardila Sanchez Date: Thu, 9 Jul 2026 22:15:53 +0200 Subject: [PATCH] fix(vision-bridge): auto-reroute non-vision models to fastest vision model when images detected (#6640) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(electron): bump electron 42→43 + build better-sqlite3 from source (ABI 148) (#6605) fix(electron): bump electron 42→43 + rebuild better-sqlite3 from source against the Electron ABI (148). Electron 43 raises NODE_MODULE_VERSION to 148; better-sqlite3@12.11.1 has no electron-v148 prebuild, so the packaged app died with 'Nenhum driver SQLite disponível'. prepare-electron-standalone now compiles better-sqlite3 from source against the electron headers into build/Release (where 'bindings' resolves it). Validated by Electron Package Smoke (green) + local (node_register_module_v148). Supersedes #6378. (--admin: the only reds are SonarQube/SonarCloud failing on a coverage-report artifact digest-mismatch — a GitHub Actions infra flake, not this diff; Sonar is green on main and the diff touches only the electron build.) * deps: bump the development group across 1 directory with 6 updates (#6588) deps: bump the development group (6 updates). Rebased onto current main; all checks green after the electron-smoke fix (#6605). * fix(proxy): force CONNECT tunnel for HTTP proxied requests (undici 8.7) + production deps bump (#6620) fix(proxy): force CONNECT tunnel for HTTP proxied requests (undici 8.7) + production deps bump. undici 8.6+ changed ProxyAgent to forward plain-HTTP via request-proxy instead of CONNECT, breaking OAuth refresh through a connection proxy (501). proxyDispatcher now passes proxyTunnel:true. Validated: Unit Tests 3/8 (the OAuth-proxy test) green, new regression test green (fails without the fix on undici 8.7), SonarQube green. Supersedes #6380. (--admin: the only red is Electron Package Smoke failing on a next-build artifact 'digest-mismatch' — a GitHub Actions infra flake corrupting the asar ('file data stream has unexpected number of bytes'); the better-sqlite3 rebuild itself succeeded (gyp ok) and the electron path is unchanged from #6605 which passed the smoke. Not this diff.) * fix(vision-bridge): auto-reroute non-vision models to fastest vision model when images detected The VisionBridgeGuardrail was describing images as text via a vision model and sending text to the original (non-vision) model. This defeated the purpose when the final target was already vision-capable (auto/vision, combos with vision targets) and never actually rerouted requests to a vision model. Changes: - Individual non-vision models + images → reroute to the fastest available vision-capable model (via getBestVisionModel), keeping images intact - Auto/ prefix models (auto/vision, auto) → skip guardrail entirely, letting the auto-combo resolver handle vision-capable model selection - Combo mappings with non-vision targets → keep existing describe behavior (fallback path via checkModelHasComboMapping) - chat.ts: sync modelStr from body.model after guardrail execution so downstream routing uses the rerouted model * fix(vision-bridge): use getBestVisionModel auto-routing instead of fixed model Address Gemini review feedback: getBestVisionConfig({}) with empty object bypassed auto-routing by always defaulting to a fixed model. Auto-select the best vision model from available providers instead. * fix: compact modelStr sync to stay under file-size cap (1632) * fix: remove debug log, orphaned brace to keep file under cap * chore: trigger CI re-run with file-size fix and PR evidence * chore: rebaseline chat.ts frozen cap to 1754 (PR #6640 +3 lines) * fix(auto-combo): respect hidden models from dashboard toggle getHiddenModelsByProvider() only queried modelCompatOverrides and customModels namespaces, missing the hiddenModels namespace used by the dashboard hide/unhide toggle. Auto-combo candidates now filter out models the user explicitly hid. * fix(changelog): restore CHANGELOG bullets eaten by release sync Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: herjarsa Co-authored-by: Diego Rodrigues de Sa e Souza --- CHANGELOG.md | 1 + config/quality/file-size-baseline.json | 33 +- src/lib/guardrails/visionBridge.ts | 52 ++- src/sse/handlers/chat.ts | 18 + tests/unit/guardrails/visionBridge.test.ts | 396 ++++++++++++------ .../vision-bridge-policy-reroute-6640.test.ts | 149 +++++++ 6 files changed, 490 insertions(+), 159 deletions(-) create mode 100644 tests/unit/vision-bridge-policy-reroute-6640.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f209f9bcad..d39001371f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral ### 🐛 Bug Fixes - **fix(cli):** per-agent AgentBridge DNS toggle was broken for 8 of the 9 supported agents, and a failed MITM startup step could orphan the spawned proxy child — `addDNSEntry`/`removeDNSEntry` (`src/mitm/dns/dnsConfig.ts`) always resolved the legacy Antigravity default hosts regardless of which agent's toggle was flipped, so enabling DNS for Cursor/Codex/Claude Code/etc. silently added only `daily-cloudcode-pa.googleapis.com` while the DB recorded `dns_enabled=true` for the selected agent. Both functions now accept an optional `agentId` and resolve hosts via `ALL_TARGETS`; `POST /api/tools/agent-bridge/agents/[id]/dns` passes the route's `id` through and now returns 404 for an id that doesn't match a known target instead of silently falling back. Separately, `startMitmInternal()` (`src/mitm/manager.ts`) now wraps `generateCert()` (log + rethrow), the `provisionDnsEntries()` call, and the PID-file write in try/catch so a mid-startup failure can't orphan the already-spawned MITM child process. On Windows, `addDNSEntries`/`removeDNSEntries` also batch every missing/present entry into a single elevated PowerShell invocation instead of one UAC prompt per host line. Regression guard: `tests/unit/dns-config-generic.test.ts` (agent-specific resolution + batching), `tests/unit/agent-bridge-dns-route-validation.test.ts` (404 for unknown agent id). ([#6338](https://github.com/diegosouzapw/OmniRoute/pull/6338) — thanks @hamsa0x7) +- **fix(guardrails):** Vision Bridge's individual-model auto-reroute (route an image-bearing request straight to a vision-capable model instead of describe-then-forward) could bypass a policy-restricted API key's model allowlist/budget ([#6640](https://github.com/diegosouzapw/OmniRoute/pull/6640)) — `VisionBridgeGuardrail.preCall()` (`src/lib/guardrails/visionBridge.ts`) swaps `body.model` to the best available vision-capable model, but that swap happens in the guardrail pipeline AFTER `chat.ts` already called `enforceApiKeyPolicy()` against the ORIGINAL model, so a key scoped to a narrow `allowedModels` list could still execute against an unvetted (and possibly costlier) vision model the reroute picked. `chat.ts` now re-validates any guardrail-driven model change against the same per-key allowlist (`isModelAllowedForKey`) before honoring it, falling back to the original already-approved model when the reroute target is not allowed. The reroute path also now honors an explicit `settings.visionBridgeModel` operator override (previously ignored, unlike the combo/describe path a few lines below it, which already respects it via `getVisionBridgeConfig`). Regression guard: `tests/unit/guardrails/visionBridge.test.ts` (22 tests). (thanks @herjarsa) - **fix(auth):** an API key restricted via `allowedModels`/`allowedCombos` could bypass that restriction entirely over the Codex Responses-over-WebSocket bridge ([#6564](https://github.com/diegosouzapw/OmniRoute/issues/6564)) — `prepare()` in `src/app/api/internal/codex-responses-ws/route.ts` authenticated the WS bridge's API key (`authenticate()`/`authorizeWebSocketHandshake()`) and honored `allowedConnections`, but never called `enforceApiKeyPolicy()`, the same model/combo policy gate the HTTP `/v1/responses` path enforces via `handleChat()` — so a key scoped to e.g. `combo/model-1.0` could still reach a direct Codex model like `gpt-5.5` through this transport, as long as an eligible Codex OAuth connection existed. The bridge's WS auth token arrives via query params (`api_key`/`token`/`access_token`), not a normal `Authorization` header, so a new `enforceCodexWsApiKeyPolicy()` builds an equivalent `Request` carrying an explicit `Authorization: Bearer ` header and calls `enforceApiKeyPolicy()` against the CLIENT-requested model, before any Codex-specific model remapping or credential selection. Regression guard: `tests/unit/codex-ws-policy-enforcement-6564.test.ts` (a model-restricted key is rejected 403 before reaching credential selection; a combo-restricted key is rejected 403 requesting a disallowed combo; a key that DOES allow the requested model still proceeds past policy). - **fix(security):** loopback-gate `/api/middleware/*` so a leaked JWT over a tunnel can't install or trigger a middleware hook — middleware hooks compile + run arbitrary JS via `new vm.Script` on the request hot path (`src/lib/middleware/registry.ts`), the same RCE class as the already-gated `/api/plugins/*`; `/api/middleware/` is now in `LOCAL_ONLY_API_PREFIXES` so loopback enforcement runs unconditionally before any auth check (Hard Rules #15 + #17). Regression guard: `tests/unit/route-guard-middleware-local-only.test.ts`. ([#6541](https://github.com/diegosouzapw/OmniRoute/pull/6541)) — see PR. (thanks @developerjillur) - **fix(startup):** AgentBridge's MITM server no longer fails to start with `ROUTER_API_KEY is required` on a normal install ([#6403](https://github.com/diegosouzapw/OmniRoute/issues/6403)) — `POST /api/tools/agent-bridge/server` resolved the spawned MITM child's router key from only an explicit `apiKey` body field (never sent by the AgentBridge UI — the schema has no such field) and the `ROUTER_API_KEY` env var (unset by default), so `startMitm()` always received `""` and the child hard-exited, even though OmniRoute already had a usable API key in its own DB. A new `resolveRouterApiKey()` now falls back to `pickApiKeyForInternalUse()` (the same DB-backed selector the combo-health-check / cloud-sync internal probes use), resolving in order: explicit key → `ROUTER_API_KEY` env → an existing DB key. Regression guard: `tests/unit/agentbridge-mitm-router-key-6403.test.ts`. diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index ca1153119e..98f688bfbd 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -259,7 +259,7 @@ "src/shared/services/cliRuntime.ts": 1110, "src/shared/validation/schemas.ts": 2523, "_rebaseline_2026_06_28_5275_correlation_id_extract": "Extraction of the safe CorrelationId subset of #5275 (hartmark) — request correlation id stored in call_logs (migration 109) and returned via the X-Correlation-Id response header, WITHOUT the combo/resilience or build/lazy-loading changes (those stay in #5275). Own growth: callLogs.ts 975->985 (correlation_id column on CallLogSummaryRow + read/map), usageHistory.ts 983->988 (correlationId metadata normalize), chat.ts 1575->1632 (withCorrelationId response wiring + combo-failure log carrying correlationId), chatHelpers.ts new 811 (withCorrelationId helper + reqId threading; was 791873 (+3: antigravity projectId assertion + 50ms tick for the now fire-and-forget onboarding, matching the no-PKCE/no-openid flow).", + "_rebaseline_2026_07_02_5928_base_red": "web-cookie-providers-new.test.ts 845->850: #5928 (test(security) Kimi Web URL host parse, CodeQL #689) grew the file +5 lines and merged into release/v3.8.44 WITHOUT rebaselining, leaving a fast-gates base-red that blocked every subsequent PR->release. Test growth is legitimate (a security regression test); maintainer absorbs the drift here. Frozen at 850.", + "_rebaseline_basered_codebuddy_cn": "Base-red fix (#4664 CodeBuddy CN): oauth-providers-config.test.ts 867->870 (+3) to align the EXPECTED provider list/config with the codebuddy-cn provider that #4664 added to the registry without updating this test (it asserts 'exactly once').", + "_rebaseline_pr4561_qwen_oauth_url": "Reconcile #4561 (port decolua/9router#683) already-merged growth: oauth-providers-config.test.ts 855->867 (+12, qwen.ai URL regression-pin test). Fast-gate PR->release does not run check:file-size, so this surfaced post-merge.", + "_rebaseline_pr4613_compatible_provider_groups": "Reconcile #4613 already-merged growth: providers-page-utils.test.ts 1004->1052 (+48, buildCompatibleProviderGroups partition unit test). Fast-gate PR->release does not run check:file-size, so this surfaced post-merge.", + "tests/integration/chat-pipeline.test.ts": 1601, "tests/integration/chatcore-compression-integration.test.ts": 1111, "tests/integration/skills-pipeline.test.ts": 918, "tests/unit/account-fallback-service.test.ts": 1572, @@ -285,29 +290,27 @@ "tests/unit/chatcore-sanitization.test.ts": 831, "tests/unit/chatcore-translation-paths.test.ts": 2810, "tests/unit/chatgpt-web.test.ts": 3170, - "tests/unit/combo-routing-engine.test.ts": 3213, + "tests/unit/combo-config.test.ts": 881, + "tests/unit/combo-routing-engine.test.ts": 3209, "tests/unit/combo-strategy-fallbacks.test.ts": 880, "tests/unit/db-core-init.test.ts": 877, "tests/unit/db-migration-runner.test.ts": 1491, "tests/unit/db-settings-crud.test.ts": 941, "tests/unit/deepseek-web.test.ts": 1092, "tests/unit/executor-antigravity.test.ts": 942, - "tests/unit/executor-codex.test.ts": 1347, + "tests/unit/executor-codex.test.ts": 1340, "tests/unit/executor-default-base.test.ts": 1523, "tests/unit/grok-web.test.ts": 2437, "tests/unit/image-generation-handler.test.ts": 2019, "tests/unit/model-sync-route.test.ts": 1016, "tests/unit/models-catalog-route.test.ts": 1605, - "_rebaseline_pr4561_qwen_oauth_url": "Reconcile #4561 (port decolua/9router#683) already-merged growth: oauth-providers-config.test.ts 855->867 (+12, qwen.ai URL regression-pin test). Fast-gate PR->release does not run check:file-size, so this surfaced post-merge.", - "_rebaseline_basered_codebuddy_cn": "Base-red fix (#4664 CodeBuddy CN): oauth-providers-config.test.ts 867->870 (+3) to align the EXPECTED provider list/config with the codebuddy-cn provider that #4664 added to the registry without updating this test (it asserts 'exactly once').", - "_rebaseline_2026_06_27_5193_antigravity_test": "#5193 own test growth: oauth-providers-config.test.ts 870->873 (+3: antigravity projectId assertion + 50ms tick for the now fire-and-forget onboarding, matching the no-PKCE/no-openid flow).", - "tests/unit/oauth-providers-config.test.ts": 873, + "tests/unit/oauth-providers-config.test.ts": 842, "tests/unit/perplexity-web.test.ts": 999, "tests/unit/provider-models-route.test.ts": 1752, - "tests/unit/provider-validation-specialty.test.ts": 2874, - "_rebaseline_pr4613_compatible_provider_groups": "Reconcile #4613 already-merged growth: providers-page-utils.test.ts 1004->1052 (+48, buildCompatibleProviderGroups partition unit test). Fast-gate PR->release does not run check:file-size, so this surfaced post-merge.", + "tests/unit/provider-validation-specialty.test.ts": 2856, "tests/unit/providers-page-utils.test.ts": 1109, "tests/unit/reasoning-cache.test.ts": 980, + "tests/unit/response-sanitizer.test.ts": 1063, "tests/unit/route-edge-coverage.test.ts": 1241, "tests/unit/search-handler-extended.test.ts": 1124, "tests/unit/sse-auth.test.ts": 1600, @@ -316,15 +319,12 @@ "tests/unit/translator-friendly-test-bench.test.tsx": 848, "tests/unit/translator-helper-branches.test.ts": 870, "tests/unit/translator-openai-responses-req.test.ts": 1172, - "tests/unit/translator-openai-to-gemini.test.ts": 1579, + "tests/unit/translator-openai-to-gemini.test.ts": 1541, "tests/unit/translator-openai-to-kiro.test.ts": 1234, "tests/unit/translator-resp-gemini-to-openai.test.ts": 1234, - "tests/unit/usage-service-hardening.test.ts": 1633, + "tests/unit/usage-service-hardening.test.ts": 1503, "tests/unit/vscode-token-routes.test.ts": 1285, - "tests/unit/combo-config.test.ts": 881, - "_rebaseline_2026_07_02_5928_base_red": "web-cookie-providers-new.test.ts 845->850: #5928 (test(security) Kimi Web URL host parse, CodeQL #689) grew the file +5 lines and merged into release/v3.8.44 WITHOUT rebaselining, leaving a fast-gates base-red that blocked every subsequent PR->release. Test growth is legitimate (a security regression test); maintainer absorbs the drift here. Frozen at 850.", - "tests/unit/web-cookie-providers-new.test.ts": 890, - "tests/unit/response-sanitizer.test.ts": 1063 + "tests/unit/web-cookie-providers-new.test.ts": 890 }, "_rebaseline_2026_06_09": "Re-baseline consciente pre-release v3.8.19: 9 arquivos cresceram durante o ciclo (features mergeadas: RequestLoggerV2 +281 request-logger rework, stream +101, combo +73, chatCore +45, catalog +32 fable-5/catalog-flag, callLogs +4, accountFallback +2, usageHistory novo 840) + core.ts +7 (fix resetAllDbModuleState, PR 3536). A catraca segue valendo destes valores — proximo crescimento falha. Decisao: encolher (esp. RequestLoggerV2/chatCore) e a issue #3501 ficam para o ciclo seguinte.", "_rebaseline_2026_06_11_phase1f": "Phase 1f (#3501): ProviderDetailPageClient.tsx 4948→4062 (-886 LOC); 3 novos hooks extraídos. useProviderConnections.ts=954 acima do cap=800 — justificado: extração direta do god-component (zero lógica nova), própria redução do cliente supera o custo. useProviderSettings.ts=263 e useProviderModels.ts=154 já abaixo do cap.", @@ -383,6 +383,7 @@ "_rebaseline_2026_07_06_6118_zed_oauthmodal": "PR #6118 own growth: OAuthModal.tsx 989->993 (+4 = Zed hosted native-app sign-in modal branch). Cohesive UI growth for the zed-hosted OAuth provider; not extractable. The prior 6118 comment set the note but left the frozen value at 989.", "_rebaseline_2026_07_06_6351_glm_team_quota": "PR #6351 own growth (GLM team-plan quota fields threaded through the connection modals; new GlmTeamQuotaFields.tsx extracted): AddApiKeyModal.tsx ->951 (+9), EditConnectionModal.tsx ->1277 (+18). Absorbs the pre-existing session base-red on these frozen modals; release captain rebaseline-at-release supersedes.", "_rebaseline_2026_07_06_6499_unique_default_name": "PR #6499 own growth: AddApiKeyModal.tsx 952->959 (+7 = a unique default connection name so a second API key for the same provider does not reuse 'main' and trigger the backend name-based upsert that silently overwrote the first connection). The pure name derivation was extracted to computeConnectionDefaultName.ts (unit-tested) to keep the growth minimal; the contributor's original full-form-reset rewrite was trimmed to a spread reset to avoid dropping the GLM team-quota fields #6351 added and to hold the frozen god-file growth down. Release captain rebaseline-at-release supersedes.", + "_rebaseline_2026_07_08_vb_reroute": "PR #6640 (Vision Bridge reroute) own growth, re-measured post-merge with origin/release/v3.8.47 + /implement-prs mandatory pre-merge fixes (wc -l + 1): chat.ts 1778->1796 (+18, stacking on #6515/#6525 chirag growth already frozen at 1778) = the original +3 guardrail modelStr sync block PLUS +15 for the policy re-validation added during review (a guardrail-driven model change is now re-checked against isModelAllowedForKey before being honored, closing an allowlist-bypass gap; see tests/unit/vision-bridge-policy-reroute-6640.test.ts). Irreducible wiring at the guardrail post-execution/policy chokepoint; covered by tests/unit/guardrails/visionBridge.test.ts (22 tests) + the 3 new policy-reroute regression tests.", "_rebaseline_2026_07_07_6523_chirag_cooldown_body": "PR #6523 (@chirag127, #6460) own growth: chatHelpers.ts 860->866 (+6 = retryAfterAt/credentialsCoolingCount fields on modelCooldownResponse) and auth.ts 2447->2448 (+1 = connectionsCount threaded through no-credentials fallback). Owner-approved rebaseline (file-size cap for contributor PR). Frozen (cannot grow further); release captain's rebaseline-at-release supersedes.", "_rebaseline_2026_07_07_6526_chirag_modal_1080p": "PR #6526 (@chirag127, #6265): AddApiKeyModal.tsx ->961 (1080p sizing). Owner-approved. Frozen.", "_rebaseline_2026_07_07_6515_chirag": "PR #6515 (@chirag127) own growth: src/sse/handlers/chat.ts ->1763. Owner-approved rebaseline. Frozen.", diff --git a/src/lib/guardrails/visionBridge.ts b/src/lib/guardrails/visionBridge.ts index 1c721b8cd9..4e5b45f966 100644 --- a/src/lib/guardrails/visionBridge.ts +++ b/src/lib/guardrails/visionBridge.ts @@ -1,7 +1,8 @@ /** * Vision Bridge Guardrail. - * Intercepts image-bearing requests to non-vision models, - * extracts descriptions via vision model, and replaces images with text. + * Intercepts image-bearing requests to non-vision models. + * For individual non-vision models: reroutes to the fastest available vision-capable model. + * For combos with non-vision targets: extracts descriptions via vision model and replaces images with text. */ import { BaseGuardrail, type GuardrailContext, type GuardrailResult } from "./base"; @@ -17,6 +18,7 @@ import { getVisionBridgeConfig, isVisionBridgeForcedModel, } from "@/shared/constants/visionBridgeDefaults"; +import { getBestVisionModel } from "./visionBridgeRouter"; type ComboVisionBridgeDecision = "process" | "skip" | "not-combo"; @@ -121,6 +123,11 @@ export class VisionBridgeGuardrail extends BaseGuardrail { return { block: false }; } + // 3b. Auto/ prefix → skip guardrail (auto-combo resolver handles vision-capable model selection) + if (model === "auto" || model.startsWith("auto/")) { + return { block: false }; + } + const forceVisionBridge = isVisionBridgeForcedModel(model); // 4. Check if model supports vision @@ -176,7 +183,40 @@ export class VisionBridgeGuardrail extends BaseGuardrail { return { block: false }; } - // 9. Get configuration + // 9. Individual non-combo model with images → REROUTE to best vision-capable model + // instead of describing images through an intermediate vision call. + // This lets a downstream vision model process the image natively. + if (comboVisionBridgeDecision === "not-combo" && !forceVisionBridge) { + // Honor an explicit operator override from the Vision Bridge settings tab + // (settings.visionBridgeModel) as the fixed reroute target, for consistency + // with the combo/describe path below (step 10) which always honors it via + // getVisionBridgeConfig. When unset, auto-select the fastest available + // vision-capable model from available providers. + const configuredModel = + typeof settings.visionBridgeModel === "string" && settings.visionBridgeModel.trim() + ? settings.visionBridgeModel.trim() + : undefined; + const bestModel = getBestVisionModel({ fixedModel: configuredModel }); + if (bestModel && bestModel !== model) { + const modifiedBody = { + ...(body as Record), + model: bestModel, + }; + return { + block: false, + modifiedPayload: modifiedBody as unknown, + meta: { + rerouted: true, + fromModel: model, + toModel: bestModel, + imagesKept: imageParts.length, + }, + }; + } + // Fall through: if no vision model found, describe images as text instead + } + + // 10. Get configuration const config = getVisionBridgeConfig({ visionBridgeEnabled: settings.visionBridgeEnabled as boolean | undefined, visionBridgeModel: settings.visionBridgeModel as string | undefined, @@ -185,10 +225,10 @@ export class VisionBridgeGuardrail extends BaseGuardrail { visionBridgeMaxImages: settings.visionBridgeMaxImages as number | undefined, }); - // 10. Limit images + // 11. Limit images const limitedParts = imageParts.slice(0, config.maxImages); - // 11. Call vision model for each image in parallel (injectable for testing) + // 12. Call vision model for each image in parallel (injectable for testing) const callVision = this.deps.callVisionModel ?? defaultCallVisionModel; const logger = context.log; const startTime = Date.now(); @@ -215,7 +255,7 @@ export class VisionBridgeGuardrail extends BaseGuardrail { return null; }); - // 12. Replace image parts with text descriptions (null → keep original image) + // 13. Replace image parts with text descriptions (null → keep original image) const modifiedBody = replaceImageParts( body as Parameters[0], descriptions diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 8f67fc95a0..1646f0a81d 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -55,6 +55,7 @@ import { checkAndRefreshToken } from "../services/tokenRefresh"; import { createHookContext, runHooks, initPreRequestRegistry } from "@/lib/middleware/registry"; import { deleteHandoff, getHandoff } from "@/lib/db/contextHandoffs"; import { updateCombo } from "@/lib/db/combos"; +import { isModelAllowedForKey } from "@/lib/db/apiKeys"; import { promoteSuccessfulComboModel } from "@/lib/combos/autoPromote"; import { deleteSessionAccountAffinity, @@ -464,6 +465,23 @@ export async function handleChat( ); } body = preCallGuardrails.payload; + if (body?.model && typeof body.model === "string" && body.model !== modelStr) { + const rerouteModel = body.model; + // A guardrail (e.g. Vision Bridge auto-reroute) can swap body.model AFTER + // enforceApiKeyPolicy already validated modelStr's allowlist/budget above. + // Re-check the new target against the same per-key allowlist so a + // policy-restricted key cannot be silently routed to an unchecked model. + const rerouteAllowed = await isModelAllowedForKey(apiKey, rerouteModel); + if (!rerouteAllowed) { + log.warn( + "POLICY", + `Guardrail reroute to "${rerouteModel}" rejected by API key policy (key=${apiKeyInfo?.id || "unknown"}); keeping original model "${modelStr}"` + ); + body = { ...body, model: modelStr }; + } else { + modelStr = rerouteModel; + } + } telemetry.endPhase(); // T08: per-key active session limit (0 = unlimited). diff --git a/tests/unit/guardrails/visionBridge.test.ts b/tests/unit/guardrails/visionBridge.test.ts index 847c9d6115..c59713b371 100644 --- a/tests/unit/guardrails/visionBridge.test.ts +++ b/tests/unit/guardrails/visionBridge.test.ts @@ -190,12 +190,19 @@ test("VB-S02b: respects native vision support for GPT-family models", async () = const result = await guardrail.preCall(payload, createContext({ model })); assert.strictEqual(result.block, false, `expected passthrough for ${model}`); - assert.strictEqual( - result.modifiedPayload, - undefined, - `expected unmodified payload for ${model}` - ); assert.strictEqual(visionCallCount, 0, `expected no bridge call for ${model}`); + + // If supportsVision is true, payload should be unmodified. + // If supportsVision is null, the guardrail reroutes (modifiedPayload defined, model changed). + // Both are correct behavior — the key invariant is no describe call. + const caps = getResolvedModelCapabilities(model); + if (caps.supportsVision === true) { + assert.strictEqual( + result.modifiedPayload, + undefined, + `expected unmodified payload for ${model}` + ); + } } }); @@ -225,10 +232,60 @@ test("VB-S04: passthroughs when messages array is empty", async () => { assert.strictEqual(result.block, false); }); -// ── VB-S01: Single image processing ───────────────────────────────────────── +// ── VB-S12: Auto-prefix skip ──────────────────────────────────────────────── -test("VB-S01: replaces image with description for non-vision model", async () => { - mockVisionResponse = "A beautiful sunset over the ocean"; +test("VB-S12: skips guardrail for auto/ prefix model (auto/vision)", async () => { + const guardrail = createGuardrail(); + + const payload = createPayload({ + model: "auto/vision", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is in this image?" }, + { + type: "image_url", + image_url: { url: "https://example.com/image.png" }, + }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "auto/vision" })); + assert.strictEqual(result.block, false); + assert.strictEqual(result.modifiedPayload, undefined, "auto/vision should passthrough"); + assert.strictEqual(visionCallCount, 0, "should NOT call vision API for auto prefix"); +}); + +test("VB-S12b: skips guardrail for bare auto prefix", async () => { + const guardrail = createGuardrail(); + + const payload = createPayload({ + model: "auto", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is in this image?" }, + { + type: "image_url", + image_url: { url: "https://example.com/image.png" }, + }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "auto" })); + assert.strictEqual(result.block, false); + assert.strictEqual(result.modifiedPayload, undefined, "auto should passthrough"); +}); + +// ── VB-S01: Single image → reroute (individual non-vision model) ─────────── + +test("VB-S01: reroutes non-vision model with images to best vision model", async () => { const guardrail = createGuardrail(); const payload = createPayload({ @@ -252,37 +309,40 @@ test("VB-S01: replaces image with description for non-vision model", async () => assert.strictEqual(result.block, false); assert.ok(result.modifiedPayload); + // Model should be rerouted to best vision-capable model (auto-selected from providers) const modified = result.modifiedPayload as { + model?: string; messages: Array<{ content: unknown[] }>; }; - const content = modified.messages[0].content as Array<{ - type: string; - text?: string; - }>; + assert.ok(modified.model, "rerouted model should be set"); + assert.notStrictEqual( + modified.model, + "minimax/minimax-01", + "model should be different from original" + ); + // Images should be KEPT since the vision model handles them natively + const content = modified.messages[0].content as Array<{ type: string; [key: string]: unknown }>; const imagePart = content.find((p) => p.type === "image_url"); - assert.strictEqual(imagePart, undefined); + assert.ok(imagePart, "original image_url part must be preserved for rerouted vision model"); - const descriptionPart = content.find((p) => p.type === "text" && p.text?.includes("sunset")); - assert.ok(descriptionPart); + // Meta should indicate reroute occurred + const meta = result.meta as Record; + assert.strictEqual(meta.rerouted, true); + assert.strictEqual(meta.fromModel, "minimax/minimax-01"); + assert.ok( + typeof meta.toModel === "string" && meta.toModel.length > 0, + "toModel should be a non-empty string" + ); + assert.notStrictEqual(meta.toModel, "minimax/minimax-01", "toModel should differ from original"); + assert.strictEqual(meta.imagesKept, 1); + assert.strictEqual(visionCallCount, 0, "should NOT call vision API for description"); }); -// ── VB-S04: Multiple images ───────────────────────────────────────────────── +// ── VB-S13: Reroute preserves multiple images ────────────────────────────── -test("VB-S04: processes multiple images and concatenates descriptions", async () => { - let callIdx = 0; - const descriptions = ["A cute cat", "A playful dog", "A colorful bird"]; - - const guardrail = new VisionBridgeGuardrail({ - deps: { - getSettings: async () => mockSettings, - callVisionModel: async () => { - const desc = descriptions[callIdx] || "Unknown image"; - callIdx++; - return desc; - }, - }, - }); +test("VB-S13: reroutes with multiple images, all preserved", async () => { + const guardrail = createGuardrail(); const payload = createPayload({ model: "minimax/minimax-01", @@ -312,106 +372,21 @@ test("VB-S04: processes multiple images and concatenates descriptions", async () assert.strictEqual(result.block, false); assert.ok(result.modifiedPayload); - assert.strictEqual(callIdx, 3); + // All 3 images should be present in the rerouted payload const modified = result.modifiedPayload as { + model?: string; messages: Array<{ content: unknown[] }>; }; - const content = modified.messages[0].content as Array<{ - type: string; - text?: string; - }>; - - assert.ok(content.some((p) => p.type === "text" && p.text?.includes("[Image 1]"))); - assert.ok(content.some((p) => p.type === "text" && p.text?.includes("[Image 2]"))); - assert.ok(content.some((p) => p.type === "text" && p.text?.includes("[Image 3]"))); + const content = modified.messages[0].content as Array<{ type: string; [key: string]: unknown }>; + const images = content.filter((p) => p.type === "image_url"); + assert.strictEqual(images.length, 3, "all 3 images should be preserved"); + assert.strictEqual(visionCallCount, 0, "should NOT call vision API"); }); -// ── VB-S03: Fail-open on vision error ────────────────────────────────────── +// ── VB-S07: Base64 image format → reroute ────────────────────────────────── -test("VB-S03: preserves the original image when the vision API fails (#4012)", async () => { - shouldVisionFail = true; - const guardrail = createGuardrail(); - - const payload = createPayload({ - model: "minimax/minimax-01", - messages: [ - { - role: "user", - content: [ - { type: "text", text: "What is this?" }, - { - type: "image_url", - image_url: { url: "https://example.com/image.png" }, - }, - ], - }, - ], - }); - - const result = await guardrail.preCall(payload, createContext({ model: "minimax/minimax-01" })); - - assert.strictEqual(result.block, false); - - const modified = (result.modifiedPayload ?? payload) as { - messages: Array<{ content: unknown[] }>; - }; - const content = modified.messages[0].content as Array<{ - type: string; - text?: string; - }>; - - // #4012: a failed describe must NOT replace the image with an "(unavailable)" - // stub — the original image is preserved so a vision-capable upstream can see it. - const imagePart = content.find((p) => p.type === "image_url"); - assert.ok(imagePart, "original image_url part must be preserved on describe failure"); - const unavailPart = content.find((p) => p.type === "text" && p.text?.includes("unavailable")); - assert.strictEqual(unavailPart, undefined); -}); - -test("VB-S03: logs warning when vision API fails", async () => { - shouldVisionFail = true; - let warningLogged = false; - const guardrail = createGuardrail(); - - const payload = createPayload({ - model: "minimax/minimax-01", - messages: [ - { - role: "user", - content: [ - { - type: "image_url", - image_url: { url: "https://example.com/image.png" }, - }, - ], - }, - ], - }); - - const mockLog = { - warn: (_tag: string, msg: string) => { - if (msg.includes("Failed to get description")) { - warningLogged = true; - } - }, - }; - - await guardrail.preCall( - payload, - createContext({ - model: "minimax/minimax-01", - log: mockLog as GuardrailContext["log"], - }) - ); - - assert.strictEqual(warningLogged, true); -}); - -// ── VB-S07: Base64 image format ───────────────────────────────────────────── - -test("VB-S07: handles base64 image format", async () => { - mockVisionResponse = "An image description"; +test("VB-S07: reroutes base64 image to vision model", async () => { const guardrail = createGuardrail(); const payload = createPayload({ @@ -437,13 +412,115 @@ test("VB-S07: handles base64 image format", async () => { assert.strictEqual(result.block, false); assert.ok(result.modifiedPayload); + const modified = result.modifiedPayload as { model?: string }; + assert.ok(modified.model, "rerouted model should be set"); + assert.notStrictEqual( + modified.model, + "minimax/minimax-01", + "model should be different from original" + ); + // Don't assert a specific model — auto-router picks the best available vision model + assert.strictEqual(visionCallCount, 0, "should NOT call vision API"); }); -// ── VB-S09: Image count limit ─────────────────────────────────────────────── +// ── VB-S03: Fail-open on vision error (via combo mapping path) ──────────── -test("VB-S09: respects maxImages setting", async () => { +test("VB-S03: preserves the original image when the vision API fails (#4012)", async () => { + shouldVisionFail = true; + const guardrail = createGuardrail({ + deps: { + checkModelHasComboMapping: async (_model: string) => true, + }, + }); + + const payload = createPayload({ + model: "openai/gpt-4o", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is this?" }, + { + type: "image_url", + image_url: { url: "https://example.com/image.png" }, + }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "openai/gpt-4o" })); + + assert.strictEqual(result.block, false); + + const modified = (result.modifiedPayload ?? payload) as { + messages: Array<{ content: unknown[] }>; + }; + const content = modified.messages[0].content as Array<{ + type: string; + text?: string; + }>; + + // #4012: a failed describe must NOT replace the image with an "(unavailable)" + // stub — the original image is preserved so a vision-capable upstream can see it. + const imagePart = content.find((p) => p.type === "image_url"); + assert.ok(imagePart, "original image_url part must be preserved on describe failure"); + const unavailPart = content.find((p) => p.type === "text" && p.text?.includes("unavailable")); + assert.strictEqual(unavailPart, undefined); +}); + +test("VB-S03: logs warning when vision API fails (via combo mapping)", async () => { + shouldVisionFail = true; + let warningLogged = false; + const guardrail = createGuardrail({ + deps: { + checkModelHasComboMapping: async (_model: string) => true, + }, + }); + + const payload = createPayload({ + model: "openai/gpt-4o", + messages: [ + { + role: "user", + content: [ + { + type: "image_url", + image_url: { url: "https://example.com/image.png" }, + }, + ], + }, + ], + }); + + const mockLog = { + warn: (_tag: string, msg: string) => { + if (msg.includes("Failed to get description")) { + warningLogged = true; + } + }, + }; + + await guardrail.preCall( + payload, + createContext({ + model: "openai/gpt-4o", + log: mockLog as GuardrailContext["log"], + }) + ); + + assert.strictEqual(warningLogged, true); +}); + +// ── VB-S09: Image count limit (via combo mapping) ────────────────────────── + +test("VB-S09: respects maxImages setting in combo mapping path", async () => { mockSettings.visionBridgeMaxImages = 2; - const guardrail = createGuardrail(); + const guardrail = createGuardrail({ + deps: { + checkModelHasComboMapping: async (_model: string) => true, + }, + }); const images = Array.from({ length: 5 }, (_, i) => ({ type: "image_url" as const, @@ -451,7 +528,7 @@ test("VB-S09: respects maxImages setting", async () => { })); const payload = createPayload({ - model: "minimax/minimax-01", + model: "openai/gpt-4o", messages: [ { role: "user", @@ -460,16 +537,15 @@ test("VB-S09: respects maxImages setting", async () => { ], }); - await guardrail.preCall(payload, createContext({ model: "minimax/minimax-01" })); + await guardrail.preCall(payload, createContext({ model: "openai/gpt-4o" })); // Should only call vision API for 2 images (maxImages=2) assert.strictEqual(visionCallCount, 2); }); -// ── VB-S10: Meta information returned ─────────────────────────────────────── +// ── VB-S10: Meta information returned (reroute path) ─────────────────────── -test("VB-S10: returns meta with imagesProcessed count", async () => { - mockVisionResponse = "A test description"; +test("VB-S10: returns meta with reroute info for individual non-vision model", async () => { const guardrail = createGuardrail(); const payload = createPayload({ @@ -498,11 +574,57 @@ test("VB-S10: returns meta with imagesProcessed count", async () => { assert.ok(typeof result.meta === "object"); const meta = result.meta as Record; - assert.strictEqual(meta.imagesProcessed, 2); - assert.ok(Array.isArray(meta.descriptions)); - assert.strictEqual((meta.descriptions as string[]).length, 2); - assert.strictEqual(typeof meta.processingTimeMs, "number"); - assert.strictEqual(meta.visionModel, "openai/gpt-4o-mini"); + assert.strictEqual(meta.rerouted, true); + assert.strictEqual(meta.fromModel, "minimax/minimax-01"); + assert.ok( + typeof meta.toModel === "string" && meta.toModel.length > 0, + "toModel should be a non-empty string" + ); + assert.notStrictEqual(meta.toModel, "minimax/minimax-01", "toModel should differ from original"); + assert.strictEqual(meta.imagesKept, 2); +}); + +// ── VB-S01b: Describe images via combo mapping path ──────────────────────── + +test("VB-S01b: describes images when combo mapping forces process path", async () => { + mockVisionResponse = "A cat sitting on a windowsill"; + const guardrail = createGuardrail({ + deps: { + checkModelHasComboMapping: async (_model: string) => true, + }, + }); + + const payload = createPayload({ + model: "openai/gpt-4o", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is this?" }, + { + type: "image_url", + image_url: { url: "https://example.com/cat.png" }, + }, + ], + }, + ], + }); + + const result = await guardrail.preCall(payload, createContext({ model: "openai/gpt-4o" })); + + assert.strictEqual(result.block, false); + assert.ok(result.modifiedPayload); + + const modified = result.modifiedPayload as { messages: Array<{ content: unknown[] }> }; + const content = modified.messages[0].content as Array<{ type: string; text?: string }>; + + // Images should be replaced with text descriptions (combo path) + const imagePart = content.find((p) => p.type === "image_url"); + assert.strictEqual(imagePart, undefined, "image should be replaced by description"); + + const descriptionPart = content.find((p) => p.type === "text" && p.text?.includes("cat")); + assert.ok(descriptionPart, "description should be present"); + assert.ok(visionCallCount > 0, "vision API should have been called for description"); }); // ── VB-S11: Combo mapping forces vision processing despite vision-capable model ── @@ -568,7 +690,7 @@ test("VB-S11b: passthroughs when vision-capable model has NO combo mapping", asy const result = await guardrail.preCall(payload, createContext({ model: "openai/gpt-4o" })); - // Vision bridge should skip (passthrough) since model supports vision and no combo mapping + // Vision bridge should skip (passthrough) since model supports vision + no combo mapping assert.strictEqual(result.block, false); assert.strictEqual(result.modifiedPayload, undefined); assert.strictEqual(visionCallCount, 0); diff --git a/tests/unit/vision-bridge-policy-reroute-6640.test.ts b/tests/unit/vision-bridge-policy-reroute-6640.test.ts new file mode 100644 index 0000000000..62a5ae0d4f --- /dev/null +++ b/tests/unit/vision-bridge-policy-reroute-6640.test.ts @@ -0,0 +1,149 @@ +// Regression tests for PR #6640 review findings — Vision Bridge's individual-model +// auto-reroute (route an image-bearing request straight to a vision-capable model +// instead of describe-then-forward) swaps `body.model` INSIDE the guardrail +// pipeline, which runs AFTER `chat.ts` already called `enforceApiKeyPolicy()` +// against the original model. Without a re-check, a key restricted via +// `allowedModels` could silently execute against an unvetted reroute target. +// +// These tests exercise the real `handleChat()` pipeline end-to-end (real DB, +// real guardrail registry, mocked upstream fetch) to prove: +// 1. A guardrail-driven reroute to a model NOT in the key's `allowedModels` +// is rejected — the request falls back to the original, already-approved +// model instead of silently escaping the policy. +// 2. A guardrail-driven reroute to a model that DOES pass the allowlist is +// still honored (the fix must not break the legitimate reroute). +// 3. The reroute honors an explicit `settings.visionBridgeModel` operator +// override, consistent with the combo/describe path. + +import test from "node:test"; +import assert from "node:assert/strict"; + +import { createChatPipelineHarness } from "../integration/_chatPipelineHarness.ts"; + +const harness = await createChatPipelineHarness("vision-bridge-policy-reroute-6640"); +const { handleChat, buildRequest, buildOpenAIResponse, resetStorage, seedConnection, seedApiKey, settingsDb } = + harness; + +function imageBearingBody(model: string) { + return { + model, + stream: false, + messages: [ + { + role: "user", + content: [ + { type: "text", text: "What is in this image?" }, + { type: "image_url", image_url: { url: "https://example.com/image.png" } }, + ], + }, + ], + }; +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + await harness.cleanup(); +}); + +test("#6640: guardrail reroute to a model outside allowedModels is rejected — falls back to the original allowed model", async () => { + await seedConnection("openai", { apiKey: "sk-openai-primary" }); + // Vision Bridge auto-reroutes non-vision models with images to the best + // available vision-capable model; pin it to a DIFFERENT model than the one + // the key is allowed to use, so a real reroute is guaranteed to happen and + // to conflict with the allowlist. + await settingsDb.updateSettings({ visionBridgeModel: "openai/gpt-4o-mini" }); + + const apiKey = await seedApiKey({ allowedModels: ["openai/gpt-3.5-turbo"] }); + + const fetchCalls: Array<{ body: Record | null }> = []; + globalThis.fetch = async (_url: string | URL | Request, init: RequestInit = {}) => { + fetchCalls.push({ body: init.body ? JSON.parse(String(init.body)) : null }); + return buildOpenAIResponse("described"); + }; + + const response = await handleChat( + buildRequest({ + authKey: apiKey.key, + body: imageBearingBody("openai/gpt-3.5-turbo"), + }) + ); + + // The reroute target (gpt-4o-mini) is not in allowedModels — the request + // must NOT be silently executed against it. It must either fall back to the + // original, already-approved model (gpt-3.5-turbo) or be rejected outright, + // but it must never reach the upstream with the disallowed model. + if (response.status === 200) { + assert.equal(fetchCalls.length, 1, "exactly one upstream call expected"); + assert.equal( + fetchCalls[0].body?.model, + "gpt-3.5-turbo", + "must fall back to the original allowed model, not silently execute the disallowed reroute target" + ); + } else { + assert.equal(fetchCalls.length, 0, "a rejected request must never reach the upstream"); + } +}); + +test("#6640: guardrail reroute to a model inside allowedModels is honored (no regression)", async () => { + await seedConnection("openai", { apiKey: "sk-openai-primary" }); + await settingsDb.updateSettings({ visionBridgeModel: "openai/gpt-4o-mini" }); + + // This key allows BOTH the original model and the reroute target. + const apiKey = await seedApiKey({ + allowedModels: ["openai/gpt-3.5-turbo", "openai/gpt-4o-mini"], + }); + + const fetchCalls: Array<{ body: Record | null }> = []; + globalThis.fetch = async (_url: string | URL | Request, init: RequestInit = {}) => { + fetchCalls.push({ body: init.body ? JSON.parse(String(init.body)) : null }); + return buildOpenAIResponse("described"); + }; + + const response = await handleChat( + buildRequest({ + authKey: apiKey.key, + body: imageBearingBody("openai/gpt-3.5-turbo"), + }) + ); + + assert.equal(response.status, 200); + assert.equal(fetchCalls.length, 1); + assert.equal( + fetchCalls[0].body?.model, + "gpt-4o-mini", + "reroute to an allowed vision model must still be honored" + ); +}); + +test("#6640: reroute honors an explicit settings.visionBridgeModel override (consistency with the describe path)", async () => { + await seedConnection("openai", { apiKey: "sk-openai-primary" }); + await settingsDb.updateSettings({ visionBridgeModel: "openai/gpt-4o-mini" }); + + // No allowlist restriction — nothing to enforce here, this proves the + // settings threading itself (independent of the policy re-check above). + const apiKey = await seedApiKey(); + + const fetchCalls: Array<{ body: Record | null }> = []; + globalThis.fetch = async (_url: string | URL | Request, init: RequestInit = {}) => { + fetchCalls.push({ body: init.body ? JSON.parse(String(init.body)) : null }); + return buildOpenAIResponse("described"); + }; + + const response = await handleChat( + buildRequest({ + authKey: apiKey.key, + body: imageBearingBody("openai/gpt-3.5-turbo"), + }) + ); + + assert.equal(response.status, 200); + assert.equal(fetchCalls.length, 1); + assert.equal( + fetchCalls[0].body?.model, + "gpt-4o-mini", + "the configured settings.visionBridgeModel must be honored as the reroute target" + ); +});