From f9c224b70abcb4237d166d93fd10677791719b51 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 03:28:28 +0900 Subject: [PATCH] fix(routing): resolve model-keyed lookups the way the runtime resolves them Two evidence surfaces read per-model config with a bare map lookup while the runtime resolves the same keys through modelRecordValue, so family and case overrides were invisible to them and a prototype-shaped id resolved an Object.prototype member instead of missing. Routing capability evidence therefore gave gpt-oss:120b the provider-wide 8k window instead of the gpt-oss family's 131072, and ignored noVisionModels - values that select candidates, not just logs. The Lab behavior report missed the same overrides, and "constructor" resolved to Object.prototype.constructor, which made jcsStringify throw and silently dropped Lab subjects. Exact-own maps (modelPreferHostedTools, modelOpenRouterRouting) deliberately do not family-spread; that boundary is pinned by tests. Both patches are @ntdatt812's work from #2100 and #2077, applied unchanged. Closes #2100 Closes #2077 --- src/routing/capability.ts | 35 ++-- src/routing/compatibility/behavior.ts | 44 ++++- .../routing-capability-model-matching.test.ts | 152 ++++++++++++++++++ ...uting-compatibility-model-matching.test.ts | 126 +++++++++++++++ 4 files changed, 345 insertions(+), 12 deletions(-) create mode 100644 tests/routing-capability-model-matching.test.ts diff --git a/src/routing/capability.ts b/src/routing/capability.ts index b49cac897e..0681f64860 100644 --- a/src/routing/capability.ts +++ b/src/routing/capability.ts @@ -10,7 +10,7 @@ * how that affects eligibility. */ -import type { OcxConfig } from "../types"; +import { modelInList, type OcxConfig } from "../types"; import { isCanonicalOpenAiForwardProvider, OPENAI_CODEX_PROVIDER_ID } from "../providers/openai-tiers"; import { serviceTierSupportForModel } from "../providers/service-tier"; import { PROVIDER_REGISTRY } from "../providers/registry"; @@ -21,6 +21,7 @@ import { nativeReasoningEfforts, } from "../codex/catalog/metadata"; import { readCatalog, readCodexCatalogPath } from "../codex/catalog/parsing"; +import { modelRecordValue } from "../reasoning-effort"; import { statSync } from "node:fs"; import type { RouteCapabilityEvidence } from "./trace"; @@ -159,9 +160,14 @@ export function candidateCapabilityEvidence( const catalogRow = cachedCatalogModels().find(model => model.provider === providerName && model.id === modelId); const isNative = providerName === OPENAI_CODEX_PROVIDER_ID && !modelId.includes("/"); - const rawContextWindow = provider?.modelContextWindows?.[modelId] + // `modelRecordValue`, not a bare lookup: every runtime reader of these three maps + // resolves them that way, so a `gpt-oss` entry covers `gpt-oss:120b`. Reading raw + // made the evidence disagree with the resolver it claims to describe — and for the + // window it did not even degrade to unknown, it fell through to the provider-wide + // value, which is a definite wrong answer rather than an absent one. + const rawContextWindow = modelRecordValue(provider?.modelContextWindows, modelId) ?? provider?.contextWindow - ?? registryEntry?.modelContextWindows?.[modelId] + ?? modelRecordValue(registryEntry?.modelContextWindows, modelId) ?? catalogRow?.contextWindow ?? (isNative ? nativeOpenAiContextWindow(modelId, nativeContextLimits(config)) : undefined); // Native rows go through the accessor (raise-to-ceiling + opt-in). Routed rows keep @@ -170,10 +176,21 @@ export function candidateCapabilityEvidence( ? (nativeOpenAiContextWindow(modelId, nativeContextLimits(config)) ?? rawContextWindow) : rawContextWindow; - const modalities = provider?.modelInputModalities?.[modelId] - ?? registryEntry?.modelInputModalities?.[modelId] - ?? catalogRow?.inputModalities - ?? (isNative ? nativeInputModalities(modelId) : undefined); + // `noVisionModels` is checked before the modality chain because that is the order + // `isModelTextOnly` uses: it matches the no-vision list and returns true before it + // ever reads `modelInputModalities` (`src/vision/index.ts:32`). So a `gpt-oss` + // no-vision entry beats an exact `gpt-oss:120b` entry that lists "image", and + // deriving `image` from the modality chain alone reported vision on a model the + // runtime refuses it for. That matters more here than on the CLI surface fixed in + // #2086: routing *acts* on this evidence, so it would select the candidate for image + // work that execution then rejects. + const noVision = modelInList(provider?.noVisionModels, modelId); + const modalities = noVision + ? ["text"] + : (modelRecordValue(provider?.modelInputModalities, modelId) + ?? modelRecordValue(registryEntry?.modelInputModalities, modelId) + ?? catalogRow?.inputModalities + ?? (isNative ? nativeInputModalities(modelId) : undefined)); const image = Array.isArray(modalities) ? modalities.includes("image") : undefined; @@ -196,8 +213,8 @@ export function candidateCapabilityEvidence( || provider?.parallelToolCalls === true || undefined; - const reasoningEfforts = provider?.modelReasoningEfforts?.[modelId] - ?? registryEntry?.modelReasoningEfforts?.[modelId] + const reasoningEfforts = modelRecordValue(provider?.modelReasoningEfforts, modelId) + ?? modelRecordValue(registryEntry?.modelReasoningEfforts, modelId) ?? (isNative ? nativeReasoningEfforts(modelId) : undefined); const tierSupport = provider diff --git a/src/routing/compatibility/behavior.ts b/src/routing/compatibility/behavior.ts index 5abf08f17d..58e02f62fb 100644 --- a/src/routing/compatibility/behavior.ts +++ b/src/routing/compatibility/behavior.ts @@ -1,3 +1,4 @@ +import { modelRecordValue } from "../../reasoning-effort"; import { modelInList } from "../../types"; import type { OcxConfig, OcxProviderConfig } from "../../types"; import { PROVIDER_REGISTRY } from "../../providers/registry"; @@ -51,8 +52,45 @@ function includesModel(list: string[] | undefined, modelId: string): boolean { return modelInList(list, modelId); } +/** + * Per-model override lookup for the nine family-aware report rows. + * + * Delegates to modelRecordValue so the report reads these maps the way the + * runtime does -- own properties only, then the pre-colon family, then a + * case-folded key. A bare index disagreed on all three: it missed the + * `gpt-oss` entry ollama-cloud's `gpt-oss:120b` actually resolves, missed a + * differently-cased key, and walked the prototype chain, so a routed model id + * of `constructor`/`toString` yielded an Object.prototype function. That last + * one made buildBehaviorFingerprintV1 throw ("unsupported value type + * function"); the caller catches it (`src/routing/compatibility/subject.ts:125`) + * and returns no route, so the subject is silently dropped -- and the linker + * contract says implementations do not throw. + * + * Not every override map belongs here. `modelPreferHostedTools` and + * `modelOpenRouterRouting` are exact-own at runtime and go through + * `exactOwnValue` below; widening those to the family would be this same bug + * with the sign flipped. + */ function modelValue(map: Record | undefined, modelId: string): T | undefined { - return map?.[modelId]; + return modelRecordValue(map, modelId); +} + +/** + * Exact, own-property lookup for the two maps the runtime resolves that way. + * + * `modelPreferHostedTools` and `modelOpenRouterRouting` are deliberately exact: the + * adapter reads the first through `hasOwnProperty` + * (`src/adapters/openai-responses.ts:1001`) and the second through `Object.hasOwn` + * (`src/providers/openrouter-routing.ts:89`), and the type documents the first as + * "Exact-model hosted tools" (`src/types.ts:1584`). Sending them through + * `modelRecordValue` would make the report say a `gpt-oss` entry applies to + * `gpt-oss:120b` when the adapter will never apply it -- the same divergence this + * file exists to remove, pointed the other way. A bare index is not the answer + * either: it walks the prototype chain, which is the bug `modelValue` just fixed. + * Neither existing primitive is right for these two, so this is the third one. + */ +function exactOwnValue(map: Record | undefined, modelId: string): T | undefined { + return map !== undefined && Object.hasOwn(map, modelId) ? map[modelId] : undefined; } const CREDENTIAL_HEADER = /(authorization|api[-_]?key|token|secret|credential|cookie)/i; @@ -69,7 +107,7 @@ function nonCredentialHeaderDigest( } function effectiveOpenRouterRouting(effective: OcxProviderConfig, modelId: string) { - return effective.modelOpenRouterRouting?.[modelId] ?? effective.openRouterRouting; + return exactOwnValue(effective.modelOpenRouterRouting, modelId) ?? effective.openRouterRouting; } /** @@ -184,7 +222,7 @@ export function resolveProductionBehaviorValues( effective.parallelToolCalls ?? (upstreamProtocol === "openai-chat"), ), "tools.hostedPreference": behaviorRow("provider_config", { - tools: modelValue(effective.modelPreferHostedTools, modelId) ?? [], + tools: exactOwnValue(effective.modelPreferHostedTools, modelId) ?? [], }), "tools.builtinNameEscaping": behaviorRow("provider_config", effective.escapeBuiltinToolNames === true), "cache.forwarding": behaviorRow("provider_config", effective.promptCacheKey === true), diff --git a/tests/routing-capability-model-matching.test.ts b/tests/routing-capability-model-matching.test.ts new file mode 100644 index 0000000000..d8839b1f98 --- /dev/null +++ b/tests/routing-capability-model-matching.test.ts @@ -0,0 +1,152 @@ +import { describe, expect, test } from "bun:test"; +import { candidateCapabilityEvidence } from "../src/routing/capability"; +import { PROVIDER_REGISTRY } from "../src/providers/registry"; +import { modelRecordValue } from "../src/reasoning-effort"; +import { isModelTextOnly } from "../src/vision"; +import type { OcxConfig, OcxProviderConfig } from "../src/types"; + +/** + * `candidateCapabilityEvidence` describes what the resolver will do with a candidate, + * so it has to match the resolver. Every runtime reader of `modelContextWindows`, + * `modelInputModalities` and `modelReasoningEfforts` goes through `modelRecordValue` + * (`src/reasoning-effort.ts:108`, `src/server/effort-policy.ts:122`, + * `src/vision/index.ts:34`, `src/codex/catalog/provider-fetch.ts:612`), which accepts a + * family entry for a tagged id. This file pins the evidence to that same rule. + * + * The window matters most: a bare lookup did not degrade to unknown there, it fell + * through to the provider-wide `contextWindow` — a definite wrong answer, which the + * module's own "unknown is not zero" contract is written to avoid. + */ + +function providerWithFamilyEntries(): OcxProviderConfig { + return { + adapter: "openai-chat", + baseUrl: "https://example.test/v1", + contextWindow: 8_000, + models: ["gpt-oss:120b"], + modelContextWindows: { "gpt-oss": 131_072 }, + modelInputModalities: { "gpt-oss": ["text"] }, + modelReasoningEfforts: { "gpt-oss": ["low", "high"] }, + } as unknown as OcxProviderConfig; +} + +function configFor(provider: OcxProviderConfig): OcxConfig { + return { providers: { custom: provider } } as unknown as OcxConfig; +} + +describe("candidateCapabilityEvidence model matching", () => { + test("a family entry covers its tagged siblings, as the resolver does", () => { + const provider = providerWithFamilyEntries(); + + // Ground truth first: what the runtime itself resolves off this config. + expect(modelRecordValue(provider.modelContextWindows, "gpt-oss:120b")).toBe(131_072); + expect(isModelTextOnly(provider, "gpt-oss:120b")).toBe(true); + + const evidence = candidateCapabilityEvidence(configFor(provider), "custom", "gpt-oss:120b"); + expect(evidence.contextWindow).toBe(131_072); + expect(evidence.image).toBe(false); + expect(evidence.reasoningEfforts).toEqual(["low", "high"]); + }); + + test("the window does not fall through to the provider-wide value", () => { + // The specific regression: the provider-wide 8_000 is not "unknown", it is a + // definite answer belonging to a different model, and routing would act on it. + // Asserted as the exact expected number rather than `not.toBe(8_000)`, which + // would also pass for `undefined` or any other wrong value. + const evidence = candidateCapabilityEvidence( + configFor(providerWithFamilyEntries()), + "custom", + "gpt-oss:120b", + ); + expect(evidence.contextWindow).toBe(131_072); + }); + + test("an exact entry still wins over the family entry", () => { + const provider = { + ...providerWithFamilyEntries(), + modelContextWindows: { "gpt-oss": 131_072, "gpt-oss:20b": 32_000 }, + } as unknown as OcxProviderConfig; + expect(candidateCapabilityEvidence(configFor(provider), "custom", "gpt-oss:20b").contextWindow) + .toBe(32_000); + }); + + test("an unrelated model still falls back to the provider-wide window", () => { + const evidence = candidateCapabilityEvidence( + configFor(providerWithFamilyEntries()), + "custom", + "some-other-model", + ); + expect(evidence.contextWindow).toBe(8_000); + expect(evidence.reasoningEfforts).toBeUndefined(); + }); + + test("a registry entry covers its tagged siblings with no provider configured", () => { + // The three registry lookups (capability.ts lines 170/180/206) are a separate branch + // from the configured-provider ones above: they are only reached when the provider is + // absent from the config, which every other case here supplies. + const registryEntry = PROVIDER_REGISTRY.find(entry => entry.id === "xai"); + if (!registryEntry) throw new Error("fixture drift: no `xai` entry in PROVIDER_REGISTRY"); + + // Pin the fixture's shape rather than its values, so registry churn does not turn + // into a false failure here while real drift still does. + const family = "grok-4.6"; + expect(registryEntry.modelContextWindows?.[family]).toBeNumber(); + expect(registryEntry.modelInputModalities?.[family]).toBeArray(); + expect(registryEntry.modelReasoningEfforts?.[family]).toBeArray(); + + const emptyConfig = { providers: {} } as unknown as OcxConfig; + const evidence = candidateCapabilityEvidence(emptyConfig, "xai", `${family}:latest`); + + expect(evidence.contextWindow).toBe(registryEntry.modelContextWindows![family]); + expect(evidence.image).toBe(registryEntry.modelInputModalities![family].includes("image")); + expect(evidence.reasoningEfforts).toEqual(registryEntry.modelReasoningEfforts![family]); + }); + + test("noVisionModels beats an exact modality entry, as isModelTextOnly does", () => { + // `isModelTextOnly` matches the no-vision list and returns true before it ever + // reads `modelInputModalities`, so the `gpt-oss` no-vision entry wins over an + // exact `gpt-oss:120b` entry listing "image". Evidence that disagrees here is + // worse than a wrong window: routing selects the candidate for image work and + // execution then refuses it. + const provider = { + ...providerWithFamilyEntries(), + noVisionModels: ["gpt-oss"], + modelInputModalities: { "gpt-oss:120b": ["text", "image"] }, + } as unknown as OcxProviderConfig; + + // Ground truth first: the resolver this evidence claims to describe says text-only. + expect(isModelTextOnly(provider, "gpt-oss:120b")).toBe(true); + + const evidence = candidateCapabilityEvidence(configFor(provider), "custom", "gpt-oss:120b"); + expect(evidence.image).toBe(false); + }); + + test("a model outside noVisionModels keeps its declared image modality", () => { + // The negative half: the no-vision check must not spread to models the list does + // not cover, or the fix would trade a false positive for a false negative. + const provider = { + ...providerWithFamilyEntries(), + models: ["gpt-oss:120b", "llava:13b"], + noVisionModels: ["gpt-oss"], + modelInputModalities: { "llava:13b": ["text", "image"] }, + } as unknown as OcxProviderConfig; + + expect(isModelTextOnly(provider, "llava:13b")).toBe(false); + expect(candidateCapabilityEvidence(configFor(provider), "custom", "llava:13b").image).toBe(true); + }); + + test("a prototype-shaped model id resolves nothing", () => { + // modelRecordValue uses hasOwnProperty; a bare lookup would return Object.prototype + // members here and hand routing a function as evidence. + for (const modelId of ["constructor", "toString", "valueOf", "hasOwnProperty"]) { + const evidence = candidateCapabilityEvidence( + configFor(providerWithFamilyEntries()), + "custom", + modelId, + ); + expect(evidence.contextWindow).toBe(8_000); + expect(evidence.reasoningEfforts).toBeUndefined(); + expect(evidence.image).toBeUndefined(); + } + }); +}); diff --git a/tests/routing-compatibility-model-matching.test.ts b/tests/routing-compatibility-model-matching.test.ts index 1dee27be25..f3a26f39dd 100644 --- a/tests/routing-compatibility-model-matching.test.ts +++ b/tests/routing-compatibility-model-matching.test.ts @@ -7,6 +7,9 @@ import { describe, expect, test } from "bun:test"; import { resolveProductionBehaviorValues } from "../src/routing/compatibility/behavior"; import { createOpenAIChatAdapter } from "../src/adapters/openai-chat"; +import { buildBehaviorFingerprintV1 } from "../src/lab/subject/behavior-fingerprint"; +import { resolveOpenRouterRouting } from "../src/providers/openrouter-routing"; +import { modelRecordValue } from "../src/reasoning-effort"; import type { OcxConfig, OcxParsedRequest, OcxProviderConfig } from "../src/types"; // ollama-cloud ships `gpt-oss:120b` verbatim (src/providers/registry.ts) and the same @@ -72,3 +75,126 @@ describe("behavior report must agree with the wire the adapter actually builds", expect(v["reasoning.budgetMode"]!.value).toBe(false); }); }); + +// The list-shaped options above are one half of the report. The other half is the +// per-model override maps, which the runtime reads through modelRecordValue: own +// properties, then the pre-colon family, then a case-folded key. + +const OVERRIDES: OcxProviderConfig = { + adapter: "openai-chat", + baseUrl: "https://ollama.com/v1", + apiKey: "sk-test", + authMode: "key", + modelMaxOutputTokens: { "gpt-oss": 1234 }, + modelContextWindows: { "GPT-OSS": 55_555 }, +}; + +const overrideConfig = { providers: { "ollama-cloud": OVERRIDES } } as unknown as OcxConfig; + +const overrideValues = (modelId: string) => + resolveProductionBehaviorValues(overrideConfig, "ollama-cloud", modelId, OVERRIDES, "salt")!; + +describe("behavior report reads per-model overrides the way the runtime does", () => { + test("the adapter really applies the bare-family override to the :tag model (ground truth)", () => { + const parsed: OcxParsedRequest = { + modelId: MODEL, + context: { messages: [{ role: "user", content: "hi", timestamp: 0 }] }, + stream: false, + options: {}, + }; + const body = JSON.parse(createOpenAIChatAdapter(OVERRIDES).buildRequest(parsed).body as string); + expect(body.max_tokens).toBe(1234); + }); + + test("report agrees: limits.maxOutputTokens for the :tag model", () => { + expect(overrideValues(MODEL)["limits.maxOutputTokens"]!.value).toBe(1234); + }); + + test("a case-folded key still resolves", () => { + expect(overrideValues("gpt-oss")["limits.contextWindow"]!.value).toBe(55_555); + }); + + test("an unrelated model gets no override (control)", () => { + expect(overrideValues("glm-5.3")["limits.maxOutputTokens"]!.value).toBeNull(); + }); +}); + +// Model ids are operator-controlled, so one can collide with Object.prototype. +// openai-responses.ts already guards modelPreferHostedTools for exactly this. +describe("a prototype-shaped model id resolves to no override", () => { + test.each(["constructor", "toString", "valueOf", "hasOwnProperty"])( + "%s yields null rather than an inherited function", + (modelId) => { + const v = overrideValues(modelId); + expect(v["limits.contextWindow"]!.value).toBeNull(); + expect(v["limits.maxOutputTokens"]!.value).toBeNull(); + expect(typeof v["modalities.input"]!.value).not.toBe("function"); + }, + ); + + test("so the behavior fingerprint stays computable", () => { + // jcsStringify rejects a function, and resolvePassiveRouteSubjectId swallows the + // throw -- the subject would silently never link. + expect(() => buildBehaviorFingerprintV1(overrideValues("constructor"))).not.toThrow(); + expect(buildBehaviorFingerprintV1(overrideValues("constructor"))) + .toBe(buildBehaviorFingerprintV1(overrideValues("toString"))); + }); +}); + +// Not every override map is family-aware, and the two that are not must stay that way. +// The adapter reads `modelPreferHostedTools` through `hasOwnProperty` +// (`src/adapters/openai-responses.ts:1001`) and `resolveOpenRouterRouting` reads +// `modelOpenRouterRouting` through `Object.hasOwn` (`src/providers/openrouter-routing.ts:89`); +// the type calls the first "Exact-model hosted tools" (`src/types.ts:1584`). Sending +// these through modelRecordValue would be the divergence above with the sign flipped: +// the report would claim an override applies that the adapter will never apply. +const EXACT_ONLY: OcxProviderConfig = { + adapter: "openai-responses", + baseUrl: "https://openrouter.ai/api/v1", + apiKey: "sk-test", + authMode: "key", + modelPreferHostedTools: { "gpt-oss": ["image_generation"] }, + modelOpenRouterRouting: { "gpt-oss": { order: ["fireworks"] } }, +} as unknown as OcxProviderConfig; + +const exactConfig = { providers: { "ollama-cloud": EXACT_ONLY } } as unknown as OcxConfig; + +const exactValues = (modelId: string) => + resolveProductionBehaviorValues(exactConfig, "ollama-cloud", modelId, EXACT_ONLY, "salt")!; + +describe("exact-own override maps must not spread to the family", () => { + test("the runtime really does not apply the bare-family entry to the :tag model (ground truth)", () => { + // openRouter routing is resolvable directly, so this half is executable rather than cited. + expect(resolveOpenRouterRouting(EXACT_ONLY, "gpt-oss")).toEqual({ order: ["fireworks"] }); + expect(resolveOpenRouterRouting(EXACT_ONLY, MODEL)).toBeUndefined(); + + // And the divergence is real rather than theoretical: the family-aware primitive + // resolves the entry that the adapter's own-property guard does not see. + expect(modelRecordValue(EXACT_ONLY.modelPreferHostedTools, MODEL)).toEqual(["image_generation"]); + expect(Object.hasOwn(EXACT_ONLY.modelPreferHostedTools!, MODEL)).toBe(false); + }); + + test("report agrees: the :tag model gets no hosted-tool preference", () => { + expect(exactValues(MODEL)["tools.hostedPreference"]!.value).toEqual({ tools: [] }); + }); + + test("report agrees: the :tag model gets no openrouter routing", () => { + expect(exactValues(MODEL)["openrouter.order"]!.value).toEqual([]); + }); + + test("an exact key still resolves on both maps (control)", () => { + const v = exactValues("gpt-oss"); + expect(v["tools.hostedPreference"]!.value).toEqual({ tools: ["image_generation"] }); + expect(v["openrouter.order"]!.value).toEqual(["fireworks"]); + }); + + test("a prototype-shaped id resolves neither map", () => { + // The bare index this replaced walked the prototype chain here too, so these two + // maps had the original bug and must not simply inherit the family-aware fix. + for (const modelId of ["constructor", "toString", "valueOf"]) { + const v = exactValues(modelId); + expect(v["tools.hostedPreference"]!.value).toEqual({ tools: [] }); + expect(v["openrouter.order"]!.value).toEqual([]); + } + }); +});