Repository navigation
fix(catalog): derive built-in auto-combo modalities from the effective target pool - #11987
xiaoyaner0201 wants to merge 7 commits into
Conversation
…-inference#11947) Built-in auto/* rows are emitted with a hardcoded four-key capabilities object and no modality derivation, while the persisted-combo path already intersects per-target modalities. As a result auto/best-vision, auto/pro-vision and auto/vision advertise neither `capabilities.vision` nor `input_modalities`/`output_modalities`, even when every effective target unanimously confirms vision. Add a catalog-boundary RED that drives the real getUnifiedModelsResponse() over a deterministic single-OpenAI pool and encodes the frozen contract: - positive: unanimous vision targets must yield vision:true plus the order-preserving ["text","image"] / ["text"] intersections - fail-closed: empty pool, unresolved targets, missing input evidence and mixed vision verdicts must OMIT the fields rather than emit false/[] - invariant: vision===true iff input_modalities is emitted and contains the exact member "image"; never emit vision:false - non-vision ids must never infer vision from the auto/* name Tests only; no production source is touched. 4 of 6 fail on this base, the first being "auto/best-vision must advertise vision" (undefined !== true).
…e target pool (cheaper-inference#11947) The built-in `auto/*` loop emitted a hardcoded four-key capabilities object with no modality derivation, while the persisted combo path already intersected per-target modalities. So `auto/best-vision` never advertised `vision`, and no `auto/*` id ever advertised `input_modalities` / `output_modalities`. Add a `buildBuiltinAutoModalityMetadata` closure beside the existing target-metadata closures and apply it to `virtualCombo.models`, mapping each entry's `model` to the `modelStr` shape `getComboTargetCatalogMetadata` expects. Fail-closed semantics, all emitted via conditional spreads so an absent verdict is an ABSENT property rather than `false`/`[]`: - any effective target resolving to null metadata omits all three fields — a stricter outer gate than the persisted path, which aggregates its known subset and would otherwise let known targets overstate a partially-unknown pool; - input and output intersections are computed independently via the existing order-preserving `intersectStringArrays()`; - `capabilities.vision === true` iff `input_modalities` is emitted and contains "image", and every target's resolved vision verdict is exactly true; - a non-unanimous vision verdict also suppresses an image-bearing input intersection, preserving that invariant; `vision: false` is never emitted; - `output_modalities` is independent of the vision verdict. Ids, ordering, ownership, token limits and the tool_calling/reasoning/thinking/ temperature baseline are unchanged, and vision is never inferred from an `auto/*` id. Also reflows two pre-existing `shouldHidePaid` call sites that Prettier already flagged on the base tree, so `prettier --check` on this file now exits 0. Focused: 6/6 pass (was 6 tests / 2 pass / 4 fail).
…uild (cheaper-inference#11947) `buildBuiltinAutoModalityMetadata` calls `getComboTargetCatalogMetadata` once per effective target for every one of the ~33 built-in `auto/*` ids. All built-in autos draw from the same candidate pool under different filters, so the distinct target set is ~N while the call count is ~33xN. That made catalog build linear in pool size, silently consuming the ~1.2s/1-vCPU build budget documented at catalog.ts:875-882 with a multiplier. Add two build-scoped memos, keyed on the only four target fields the resolvers read — `providerId ?? provider`, `modelStr`, `connectionId`, `allowedConnectionIds` — via a `JSON.stringify` tuple so no field-split collision is possible: - `comboTargetModelIdMemo` wraps the local `getComboTargetModelId`, which also removes the redundant second resolution at the built-in-auto call site (the same id is already computed inside `getComboTargetCatalogMetadata`). - `comboTargetCatalogMetadataMemo` wraps the expensive resolver itself, so the persisted-combo path benefits too. Both use `Map.has()` before `get()`, so a cached `null` — the fail-closed verdict for an unresolvable target — is a hit rather than a miss. Both `Map`s are `const` locals of `buildUnifiedModelsResponseCore`, sharing the lifetime of `capabilityResolutionSnapshot`, so nothing leaks across builds. Caching returns shared object references. Verified no caller mutates them: the built-in-auto path spreads `metadata` and its nested `capabilities` into fresh objects, the persisted-combo path only reads, and `mergeComboCapabilities` builds a new object (so `delete capabilities.effort_tiers` never touches a cached row). Emitted catalog is unchanged. A 511-row catalog dump over a mixed pool exercising the vision, text-only and fail-closed branches is byte-identical before and after (sha256 2c0730d30cac4f948b1a88b63be6942df2ce1a0d0f7a44cfd64533398610a455). Catalog build median, 7 runs, identical seeded pools, same machine: pool base before after 20 32.7ms 104.8ms 43.9ms 60 35.4ms 249.4ms 46.6ms 120 41.8ms 455.0ms 59.6ms 240 46.9ms — 83.5ms Growth over a 12x pool increase: 1.43x base, 1.90x after, versus 4.32x before across only 6x. The event-loop yield invariant also improves: max gap 19-30ms base, 116-122ms before, 75-81ms after (3 reps each, same session). Focused 6/6, vitest regressions 8/8, node regressions 31/31, full autoCombo 116/116, cheaper-inference#9147 yield 1/1 — all exit 0. prettier --check exit 0; eslint exit 1 with the same 11 findings as the materialized base tree, zero new.
…nference#11947) The cheaper-inference#11947 residual reported each auto target's RAW synced modality evidence (correct — it is what stops cheaper-inference#12046's synthesized `["text","image"]`) but also overwrote the target's vision VERDICT with the raw `synced.attachment` flag. `resolveVisionCapability` applies two authoritative demotions *before* it ever reads `attachment`, and one deliberate promotion after it, so the raw flag bypassed all three for `auto/*` rows: - `customVisionOverride` (cheaper-inference#9195) — the operator's dashboard "Vision capable" toggle, stored as an explicit `false`. - `isKnownTextOnlyDespiteSync()` (cheaper-inference#4071) — a wrong synced `attachment:true` must not win for models the vendor documents as text-only, so an image request can never be routed to a blind model. - `attachment:false` alongside image modalities resolves to vision-capable (cheaper-inference#8250, documented for Kimi K3). Effect on a single-connection deployment whose pool is the denylisted `mimo-v2.5-pro` with a synced row `{attachment:true, modalities_input: ["text","image"]}`: the row advertised `vision:true` where the resolved verdict is `false` — a regression against live, in the unsafe direction. Conversely a unanimous cheaper-inference#8250-shaped pool lost `vision` *and* had its whole input list wiped, including the truthful `"text"`. Drop only the vision half of the override so `allVision` reads the canonical `metadata.capabilities.vision`. The raw modality reporting and the image-evidence gate are unchanged, and `attachment === null` already fell through to the canonical value. Conditioned on the emission precondition (every target has a synced row whose `modalities_input` contains `"image"`), the raw and canonical verdicts differ only in the two cases above, so this preserves cheaper-inference#12046's landed positive behaviour, including its original mixed-pool complaint. Tests: the previous "mixed" case was not heterogeneous under project semantics — both of its targets resolved vision-true — so it pinned the raw-verdict change rather than unanimity. It is rewritten to a genuinely mixed pool (one resolving true, one demoted by the cheaper-inference#4071 denylist), and three cases are added covering the denylist demotion, the operator override demotion and the cheaper-inference#8250 promotion as a positive.
…v3.8.51 @ 718accb Upstream advanced from 7f49b34 to 718accb while R4 was running (two chore commits: stryker test registration cheaper-inference#12170 and shared/components react-hooks lint batch cheaper-inference#12159). Neither touches this change's two paths, and the ESLint suppressions entry for src/app/api/v1/models/catalog.ts is unchanged at no-unused-vars: 10. Ordinary additive two-parent merge so the candidate is bound to the exact current target SHA.
…heaper-inference#11947) The rewritten auto-combo map callback writes `inputModalities` and `outputModalities` as explicit `undefined` keys. Both are declared `?:` on `ComboTargetCatalogMetadata`, so writing them explicitly makes them REQUIRED with type `string[] | undefined` in the callback's inferred return type. The array therefore is not `(ComboTargetCatalogMetadata | null)[]`, and the `(m): m is ComboTargetCatalogMetadata` predicate on the following `.filter` is no longer assignable to its parameter: src/app/api/v1/models/catalog.ts(922,49): error TS2677 Annotating the callback's return type as `ComboTargetCatalogMetadata | null` restores assignability without touching a single runtime expression. The explicit `undefined` keys are deliberately kept. Replacing them with conditional spreads also clears TS2677, but it is NOT behaviour-preserving: when this scope's raw `synced` lookup misses, the spread lets the helper's synthesized `["text","image"]` from `...metadata` survive, which is exactly the raw-evidence bypass this change set exists to prevent. Measured, that variant makes `auto/chaos` and `auto/best-chaos` advertise `input_modalities: ["text","image"]` and `vision: true` off synthesized evidence. Measured with the real compiler (`node node_modules/typescript/lib/tsc.js`, not `npx tsc`) against a complete `git archive` snapshot of upstream release/v3.8.51 @ 718accb: pristine upstream : 289 errors, catalog.ts clean, zero TS2677 before this commit: 290 errors, catalog.ts(922,49) TS2677 after this commit: 289 errors, diagnostic log byte-identical to upstream Behaviour equivalence is proven rather than asserted: the full catalog response was emitted across 12 scenarios (empty pool, vision pool, unresolved target, missing input evidence, mixed verdicts, denylist, operator override, attachment:false-with-image, text-only, user combo, heterogeneous pool, output-only evidence) before and after the change. The two 3.1 MB payloads share sha256 98fe3c4dbda4e425884468523c5d43fdeb3d74f 723bd499eb93dd8419e272329 — identical to the byte.
|
Heads-up @xiaoyaner0201 — #11947 was fixed by #12046, which merged on 2026-08-30 while this PR was still in draft. The two changes address the same root cause (built-in Nothing wrong with your implementation — it is purely a timing collision on parallel work. Closing this to avoid a conflicting second fix; if you spot anything the merged version misses (it covers Sorry for the wasted effort, and thanks for jumping on it. |
|
Thanks — and no issue on the timing. I did one fresh latest-release check before treating this as terminal, and there is one important scope correction: the current #11987 head is already based on top of merged #12046. After #12046 landed, I reconciled the Draft and narrowed it to residual behavior that #12046 does not cover. On exact current
The same two focused probes pass 2/2 on closed Draft head So #12046 covers the positive derivation path, but it does not fully subsume the current two-file #11987 residual. I will not reopen this PR or open a follow-up automatically. If you prefer to keep it closed, this can remain executable evidence for a maintainer-owned follow-up; otherwise the existing reviewed Draft head is already the narrow residual candidate, not a competing second implementation. |
Fixes #11947
Built-in
auto/*rows were emitted with a hardcoded four-keycapabilitiesobject andno modality derivation, so
auto/best-vision,auto/pro-visionandauto/visionadvertised neither
capabilities.visionnorinput_modalities/output_modalitieseven when every effective target unanimously confirmed vision.
Relationship to the merged #12046
#12046 landed the positive half of this contract while this PR was open. This branch now
merges it rather than competing with it, and what remains is the residual gap #12046
left: the auto path synthesized
["text","image"]from a shared helper rather thanreporting what each target actually declares, and the vision verdict could be emitted
without image evidence in the intersection.
All three of #12046's upstream cases (from blob
b9e4552c247ce42cf4a7a71b6f9d9f33ff09ee80)are present and passing here, none weakened into vacuous coverage.
What this changes
src/app/api/v1/models/catalog.ts, one region in the built-in auto loop (~50 lines net):inputModalities/outputModalitiesnow come from the target's ownmodalities_input/modalities_outputrather than the helper's synthesized pair, so an auto row can onlyadvertise image input that some target genuinely declares.
resolveVisionCapabilityviametadata.capabilities.vision— not from the rawsynced.attachmentflag. This matters: the resolver applies the operatorsupportsVisionoverride (fix(providers): manual Vision capable override does not affect Combo routing #9195) and the documented text-only denylist (fix(sse): route image requests only to confirmed-vision combo targets #4071)before it ever reads
attachment, and it deliberately promotesattachment:falsealongside image modalities (fix(providers): Kimi K3 capability metadata conflict — attachment=false but modalities include image/video #8250). Reading the raw flag would bypass all three.
capabilities.vision = trueiff the emittedinput_modalitiescontains the exact member"image"and every effective target'sresolved verdict is exactly
true. A non-unanimous verdict also suppresses animage-bearing input intersection, preserving the invariant in both directions.
omits all three fields, rather than letting the known subset overstate a
partially-unknown pool.
false. All three fields are emitted via conditionalspreads: no
vision: false, no empty arrays.Ids, ordering, ownership, token limits, the
tool_calling/reasoning/thinking/temperature baseline, the persisted-combo path and the
direct path are unchanged. Vision is never inferred from an
auto/*id.No memoization is included. An earlier revision of this branch added per-target memos;
they were dropped deliberately after review found the block adds one id resolution and one
in-memory map lookup per target on an axis the release already pays, the loop still yields,
and no material catalog-build regression reproduced.
Test evidence — real measured counts
Base:
release/v3.8.51. Verified against both718accb03d86d1c421df13bae3b4b911e845a004(the PR's recorded base) and the current tip
78fd3504dd8c8333abbc406bc4cb83443693af06—the three lint batches that landed in between do not touch
src/app/api/**, and the APItypecheck diagnostic log is byte-identical on the two tips.
All typecheck numbers below come from the real compiler, invoked as
node node_modules/typescript/lib/tsc.js --pretty false --noEmit -p tsconfig.typecheck-api.json(TypeScript 6.0.3), on complete
git archivesnapshots. Not vianpx tsc— see Method note.API Route Typecheck
catalog.ts78fd3504d770b5c3739catalog.ts(922,49): error TS2677351a867ebThe fixed tree's diagnostic log is byte-identical to the pristine upstream log (sorted
diff empty) — not merely equal in count. No baseline file was edited;
config/quality/api-typecheck-baseline.jsonis untouched.
The repo's own gate was run both ways as a control:
node scripts/check/check-api-typecheck.mjs770b5c3739✗ src/app/api/v1/models/catalog.ts TS2677 (baseline 0, live 1)— reproduces the CI signature exactly351a867ebapiTypecheckErrors=289, 0 regressionsnode --import tsx/esm --test tests/unit/build/check-api-typecheck.test.ts(the workflow'ssecond step): 8 tests / 8 pass.
Behaviour equivalence
This is a type-level change, and that is proven rather than asserted, two independent ways:
catalog.tswere transpiledwith the same esbuild that
tsxand Next/SWC use. Output sha256eb41bebd089f5ecfe05770be0e90429aa56e6d2eaffc278b0666479f7a923d4f— identical. Everytsconfigin the repo setsnoEmit, so no build artifact is derived from typeannotations at all.
/v1/modelsresponse was capturedacross 12 scenarios — empty pool, vision pool, unresolved target, missing input evidence,
mixed verdicts, denylist-only, operator override,
attachment:false-with-image,text-only, user combo, heterogeneous pool, output-only evidence — before and after.
Both payloads (3.1 MB) hash to
98fe3c4dbda4e425884468523c5d43fdeb3d74f723bd499eb93dd8419e272329. The matrix is notvacuous: it exercises 0/6/28 vision-advertising auto rows depending on scenario.
Test gates
tests/unit/11947-auto-combo-modalities.test.ts*catalog*+*models*filesnpm run test:vitest -- tests/unit/autoCombo/catalog.tsconfig/quality/eslint-suppressions.jsoncatalog.tsis byte-identical to upstreamThe three regression failures are attributed, not assumed. The widened sweep was run on
this branch and on a complete upstream snapshot with the same command: the totals and the
failing set match exactly. They are
runCursorAgent — sigkillFollowupMs(two subtests, apre-existing timing test) and
radar-catalog-capabilities.test.tsx(environmental — packagereact18-json-viewis not installed locally; fails identically on both sides).RED reproduced before the fix. The committed suite was run against complete
git archivesnapshots of live (full-tree snapshots — a single-file copy resolves the wrong config and
gives false results): 9 pass / 2 fail, exit 1. All three of #12046's upstream cases are
present and passing here, none weakened into vacuous coverage.
Probe discrimination. Passing probes alone do not prove closure, so an earlier rejected
revision of this branch (
d425242fc7) was materialized and the review probes run against it:two of them fail there and pass here. On live, 5 of the 11 adversarial probes fail; all 11
pass here.
The TS2677 fix, and why this shape
The previous head introduced one TypeScript error, which CI correctly caught. In the
built-in auto loop the map callback returns
Both keys are optional (
?:) onComboTargetCatalogMetadata, but writing them explicitly —even as
undefined— makes them required asstring[] | undefinedin the callback'sinferred return type. The array is therefore not
(ComboTargetCatalogMetadata | null)[], andthe
(m): m is ComboTargetCatalogMetadatapredicate on the following.filteris no longerassignable to its parameter.
The fix annotates the callback's return type as
ComboTargetCatalogMetadata | null. Noruntime expression is touched.
Three shapes were evaluated, and the two alternatives were rejected on evidence:
...(x ? { inputModalities: x } : {})) also clear TS2677, andmatch the style used at the emission site — but they are not behaviour-preserving here.
When this scope's raw
syncedlookup misses, the spread lets the helper's synthesized["text","image"]from...metadatasurvive instead of being overwritten withundefined.Measured, that variant makes
auto/chaosandauto/best-chaosadvertiseinput_modalities: ["text","image"]andvision: trueoff synthesized evidence — exactlythe bypass this PR exists to close. This is why the explicit keys are deliberately kept,
and the code now carries a comment saying so.
filter(m => m !== null)natively)and is behaviour-equivalent, but it leaves the element type as an anonymous inferred shape,
so the
ComboTargetCatalogMetadatacontract stops being enforced at this boundary. A lateredit could drift out of the declared type silently.
The chosen annotation is also stricter than the status quo, not merely quieter: it enables
excess-property checking on the returned literal, so a typo'd key (TS2353), a wrong value
type (TS2322) or a missing
capabilities(TS2741) now fail at this call site. Verified withtargeted probes.
Method note — why the earlier local evidence was wrong
Worth flagging for other contributors, since it cost this PR a red CI cycle:
scripts/check/check-api-typecheck.mjsshells out tonpx tsc. This repo ships nonode_modules/.bin/tsc, so locallynpxfalls through to the unrelatedtscpackage onnpm (
tsc@2.0.3), which prints "This is not the tsc command you are looking for" andexits 0 without compiling. The script parses the empty output, reports
apiTypecheckErrors=0, and even lists ~163 baselined errors as phantom "improvements" — ona tree that genuinely regresses. On CI
npm ciinstalls the bin, so the gate is correctthere; it is only misleading locally. Any typecheck claim in this PR therefore comes from
node node_modules/typescript/lib/tsc.jsdirectly.That also retires the earlier claim in this body that
catalog.ts"has no typecheckcoverage".
tsconfig.typecheck-api.jsonincludessrc/app/api/**/*.ts; the file is coveredand CI checks it on every PR. The gate was telling the truth and the local evidence was not.
Honest caveats
build, typecheck-core and coverage were not run and are notclaimed. The gates above are what was actually executed.
npx vitest run open-sse/services/autoCombo/is non-green —Test Files 3 failed | 2 passed (5),Tests 23 passed (23),Cannot bundle Node.js built-in "node:sqlite".This is environmental and byte-identical on this branch and on a complete live
snapshot, so it is not attributable to this change.
radar-catalog-capabilities.test.tsxcannot run in this environment at all (missingreact18-json-view); it fails identically on upstream, so it is reported as unattributablerather than passing.
Known follow-ups, deliberately not fixed here
-latesttargets whose synced row is stored under the stripped id.catalog.tsuses the narrow
getSyncedCapability; such a target resolvesvision=true, modalitiesInput=["text","image"]yet emitsvision=undefined, input=undefined. Worth noting this is not a regression: on the current release the samenarrow lookup misses too, so live emits
vision: truewith noinput_modalities—an ungated vision claim. This PR converts that into a clean omission, i.e. it removes a
wrong claim rather than withholding a right one.
getSyncedCapabilityForResolvedis astrict superset and would close it, but widening a lookup adds evidence in the
permissive direction and deserves its own RED/GREEN rather than riding along here.
verified correct today, but the assertion would not fail loudly under one direction of
drift. A two-line tightening.
modalitiesDeclareVisioncounts"video", while emission requires exact"image". Aunanimous video-only pool therefore resolves vision-true but correctly emits no
visionkey here. Always under-delivery, never over-claiming; flagged as a semantic seam rather
than a defect in this diff.
Open questions for maintainers
deliberately stricter than the persisted-combo path, which aggregates its known subset.
Aligning the two either way is a one-line change — this PR chose the safe direction, but
the inconsistency is real and your call.
"video"as vision evidence (follow-up 3), or is exact"image"the intended contract?or to fold follow-up 1 in here, if either is preferred for review.