diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index 899429440e..4ca9052355 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -990,6 +990,11 @@ function modelInputModalities( if (capabilityRecord?.vision === false) return ["text"]; if (capabilityRecord?.vision === true || capabilities?.some(value => ( value === "vision" || value === "image-input" || value === "image_input" + // llama.cpp and Ollama-compatible servers report vision as "multimodal" — + // it is the only image signal those servers emit (#1797). Mapped to the + // closed `text|image` enum rather than passed through: an out-of-enum + // modality makes Codex reject the entire catalog file. + || value === "multimodal" ))) { return ["text", "image"]; } diff --git a/src/providers/model-discovery.ts b/src/providers/model-discovery.ts index bd3795105f..ada0bd2aec 100644 --- a/src/providers/model-discovery.ts +++ b/src/providers/model-discovery.ts @@ -331,12 +331,83 @@ export function extractModelEnvelopeRows( } /** Validate, bound, deduplicate, and declaratively filter OpenAI `{data:[...]}` or top-level arrays (Together `#617`). */ +/** + * Metadata a sibling `models[]` array may contribute to an ALREADY-ADMITTED + * `data[]` row (#1797). + * + * llama.cpp serves a dual-envelope body: an Ollama-style `models[]` array + * carrying `capabilities` alongside the OpenAI-style `data[]` array carrying + * `meta`, so the two halves of one model's metadata never meet and a server + * that truthfully advertises "multimodal" produced an image-blind row. + * + * Two boundaries make this safe, and both were added after review found the + * first attempt unsound: + * + * 1. It runs AFTER admission filtering. Enriching first let a sibling supply + * the exact field a provider filter requires — reproduced against the real + * Chutes policy, where a row lacking `supported_features: ["tools"]` was + * admitted once a same-id sibling provided it. Enrichment may change what is + * KNOWN about a model, never WHICH models are published. + * 2. Only the capability keys #1797 needs are copied. A blanket "fill every + * absent key" made the untrusted `models[]` array a way into any field the + * pipeline consumes. + * + * The list deliberately EXCLUDES `supported_features` and `features`, even + * though both are capability-shaped: they are the two keys real provider + * filters test (`registry.ts:1591` requires `supported_features` to contain + * "tools"; `registry.ts:1883` tests `features.tool_use`). Ordering already + * prevents a sibling from flipping an admission verdict, but a key that is + * both enrichable and filter-relevant is one refactor away from becoming a + * bypass again. #1797 does not need them. + */ +const SIBLING_ENRICHABLE_KEYS = new Set(["capabilities", "modalities", "input_modalities"]); + +type SiblingIndex = Map | null>; + +function buildSiblingIndex(value: unknown, limit: number): SiblingIndex | null { + const record = plainObject(value); + const sibling = record?.models; + if (!Array.isArray(sibling) || sibling.length === 0 || sibling.length > limit) return null; + + const byId: SiblingIndex = new Map(); + for (const raw of sibling) { + const entry = plainObject(raw); + if (!entry) continue; + for (const key of ["id", "model", "name"]) { + const id = entry[key]; + if (typeof id !== "string" || id.length === 0) continue; + // `null` marks an ambiguous id: two sibling entries claim it, so neither + // can be attributed with confidence and both are ignored. + byId.set(id, byId.has(id) && byId.get(id) !== entry ? null : entry); + } + } + return byId.size > 0 ? byId : null; +} + +function enrichAdmittedModel(item: ProviderModelsApiItem, siblings: SiblingIndex): ProviderModelsApiItem { + const extra = siblings.get(item.id); + if (!extra) return item; + let merged: Record | null = null; + for (const key of SIBLING_ENRICHABLE_KEYS) { + if (!(key in extra) || key in item) continue; + merged ??= { ...(item as Record) }; + merged[key] = extra[key]; + } + return (merged ?? item) as ProviderModelsApiItem; +} + +function plainObject(value: unknown): Record | null { + return typeof value === "object" && value !== null && !Array.isArray(value) + ? value as Record + : null; +} export function extractProviderModelItems( value: unknown, discovery: ResolvedProviderModelDiscovery, ): ProviderModelItemsResult { const limit = positiveIntegerAtMost(discovery.maxModels, MODEL_DISCOVERY_MAX_MODELS); let data: unknown[]; + let siblings: SiblingIndex | null = null; if (Array.isArray(value)) { // Together-style top-level /models arrays. Catalog discovery must not treat a stray // `models` key on openai-chat responses as valid — only `data` envelopes or top-level arrays. @@ -346,6 +417,7 @@ export function extractProviderModelItems( const envelope = extractModelEnvelopeRows(value, discovery.maxModels, ["data"]); if (!envelope.ok) return envelope; data = envelope.rows; + siblings = buildSiblingIndex(value, limit); } const items: ProviderModelsApiItem[] = []; @@ -363,9 +435,15 @@ export function extractProviderModelItems( if (!isValidModelDiscoveryModelId(finalId)) continue; } const item = finalId === id ? raw as ProviderModelsApiItem : { ...(raw as ProviderModelsApiItem), id: finalId }; + // Admission is decided on the ORIGINAL `data[]` row, before any sibling + // enrichment. Merging first let a `models[]` entry supply the very field a + // provider filter requires — reproduced against the real Chutes policy, + // where a row lacking `supported_features: ["tools"]` was admitted once a + // same-id sibling provided it. Enrichment must never change WHICH models + // are published, only what is known about an already-admitted one. if (!providerModelMatchesDiscoveryFilter(item, discovery.spec?.filter) || seen.has(finalId)) continue; seen.add(finalId); - items.push(item); + items.push(siblings ? enrichAdmittedModel(item, siblings) : item); } return { ok: true, items, rawCount: data.length }; } diff --git a/tests/catalog-llamacpp-capabilities.test.ts b/tests/catalog-llamacpp-capabilities.test.ts index 153999128e..1eb433fd80 100644 --- a/tests/catalog-llamacpp-capabilities.test.ts +++ b/tests/catalog-llamacpp-capabilities.test.ts @@ -3,16 +3,17 @@ import { catalogHintsFromModelsApiItem } from "../src/codex/catalog/provider-fet import { extractProviderModelItems } from "../src/providers/model-discovery"; /** - * Regression coverage for the context half of #1797. + * Regression coverage for #1797 (llama.cpp dual-envelope metadata). * - * A llama.cpp server reports its served context under `meta.n_ctx`, which was in - * none of the recognized context fields, so a correct local server produced no - * context evidence at all. + * A llama.cpp server splits one model across two arrays: an Ollama-style + * `models[]` carrying `capabilities` and an OpenAI-style `data[]` carrying + * `meta`. Discovery reads only `data[]`, so the served context (`meta.n_ctx`) + * and the image signal (`multimodal`) never met and a correct server produced + * a context-unknown, image-blind row. * - * The image half of #1797 is NOT fixed here and is characterized below: the - * `multimodal` token lives in the Ollama-style `models[]` array while discovery - * deliberately reads only `data[]`, and even a merged item would stay - * image-unknown because `multimodal` is not a recognized capability string. + * Both halves now resolve. The tests below also pin the boundary that makes + * the join safe: admission is decided on the original `data[]` row before any + * enrichment, and only capability keys are copied. */ const VERBATIM_LLAMACPP_BODY = { @@ -62,9 +63,10 @@ describe("llama.cpp served context ingestion (#1797)", () => { expect(hints.contextWindow).toBe(32768); }); - test("the dual-envelope body yields context but still no image evidence", () => { - // Characterization of the KNOWN remaining gap in #1797, so the follow-up fix - // has a live witness and a test to flip rather than a prose claim. + test("the dual-envelope body now yields BOTH context and image evidence", () => { + // Was a characterization of the #1797 gap: the multimodal token lived in + // models[] while discovery read only data[], so the row stayed image-blind. + // Both halves are now joined on exact id, and multimodal maps to image. const extracted = extractProviderModelItems(VERBATIM_LLAMACPP_BODY, { maxModels: 100, } as never); @@ -74,7 +76,67 @@ describe("llama.cpp served context ingestion (#1797)", () => { const hints = catalogHintsFromModelsApiItem("lidge", items[0] as never); expect(hints.contextWindow).toBe(262144); - // The "multimodal" token was discarded with models[]; unknown, never false. - expect(hints.inputModalities).toBeUndefined(); + expect(hints.inputModalities).toEqual(["text", "image"]); + }); + + test("a data[] value is never overridden by its sibling", () => { + // models[] is exactly the key discovery refuses to trust as a source of + // models. Enrichment fills only ABSENT keys, so an authoritative data[] + // entry always wins. + const extracted = extractProviderModelItems({ + models: [{ id: "m", context_length: 999 }], + data: [{ id: "m", context_length: 111 }], + }, { maxModels: 100 } as never); + const items = (extracted as { ok: true; items: Array> }).items; + expect(items[0]!.context_length).toBe(111); + }); + + test("a model present only in the sibling array is still ignored", () => { + // Membership is decided entirely by data[]; the conservative boundary that + // refuses a stray models key is preserved. + const extracted = extractProviderModelItems({ + models: [{ id: "ghost" }], + data: [{ id: "real" }], + }, { maxModels: 100 } as never); + const items = (extracted as { ok: true; items: Array> }).items; + expect(items.map(i => i.id)).toEqual(["real"]); + }); + + test("an ambiguous sibling id is skipped rather than guessed", () => { + const extracted = extractProviderModelItems({ + models: [{ id: "m", context_length: 999 }, { id: "m", context_length: 555 }], + data: [{ id: "m" }], + }, { maxModels: 100 } as never); + const items = (extracted as { ok: true; items: Array> }).items; + expect(items[0]!.context_length).toBeUndefined(); + }); + test("sibling metadata cannot admit a model the provider filter rejects", () => { + // Enrichment used to run BEFORE admission filtering, so a models[] entry + // could supply the exact field a filter required. Reproduced against the + // real Chutes policy: a row without supported_features:["tools"] was + // admitted once a same-id sibling provided it. Enrichment may change what + // is KNOWN about a model, never WHICH models are published. + const extracted = extractProviderModelItems({ + models: [{ id: "not-proven-tool-capable", supported_features: ["tools"] }], + data: [{ id: "not-proven-tool-capable" }], + }, { + maxModels: 100, + spec: { filter: { allOf: [{ path: ["supported_features"], containsAny: ["tools"] }] } }, + } as never); + + const items = (extracted as { ok: true; items: Array> }).items; + expect(items).toEqual([]); + }); + + test("only capability keys are enriched, not arbitrary fields", () => { + // A blanket fill-every-absent-key made the untrusted models[] array a way + // into any field the pipeline consumes. + const extracted = extractProviderModelItems({ + models: [{ id: "m", context_length: 999, owned_by: "hostile" }], + data: [{ id: "m" }], + }, { maxModels: 100 } as never); + + const items = (extracted as { ok: true; items: Array> }).items; + expect(items[0]).toEqual({ id: "m" }); }); });