fix(models): resolve reasoning capabilities from provider facts - #3296
fix(models): resolve reasoning capabilities from provider facts#3296jackwener wants to merge 1 commit into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at bb7e8d68. The direction is right — resolving reasoning capability from provider facts instead of re-deriving it per call site is the correct consolidation, and the curated metadata table is the right authority for it. Two defects keep it from being correct as written: the picker and the session gate disagree about what a connection supports, and the xAI version comparison is lexical where it needs to be numeric. Both are inline.
The architectural concern is that this PR does not finish the consolidation it starts. thinkingOptionsForConnection treats a heuristic reasoning: false as a hard veto over the curated table, which inverts the authority ordering the rest of the file establishes — the fallback should never overrule the fact. Similarly opencode-free admits models by price rather than by identity, so a provider that lists a model at zero cost silently joins a curated allowlist. Both are places where the new resolution path is still deciding capability from a proxy signal instead of from a fact.
Cross-PR hazard worth flagging before either merges. This PR and #3129 both edit the same exactRecord key allowlist in decodeConnectionModel. exactRecord throws on unknown keys, so if these land in a way that takes one side of that hunk rather than the union, every catalog document written by the other branch becomes undecodable — the failure lands on users who already persisted a document, not in CI. Separately, #3129's normalizeModelFactOverride allowlist does not include thinkingOptions, so an override written after this PR would be dropped there. Whoever merges second should verify the union explicitly rather than trusting a clean textual merge.
Reviewed with Claude Opus as an analysis assistant. Every claim below was verified against the source at this head; reproduction status is stated per finding.
| const declared = relayModelProfile(connection, modelId)?.thinkingLevels; | ||
| if (declared) return declared; | ||
| return thinkingVariantsForModel(connection.providerType, modelId); | ||
| return deriveThinkingChoices(thinkingOptionsForConnection(connection, modelId)); |
There was a problem hiding this comment.
[P1] Make the picker and the session gate read the same connection shape. deriveThinkingChoices is called here with the full connection, so the picker offers thinking levels derived from curated metadata, but session-catalog-coordinator.ts:702-713 builds the connection it passes to the gate without a models field — so the gate resolves against an empty inventory and rejects a level the picker just presented as available. The user selects a thinking level and the request fails, with no signal that the two paths disagreed. Confirmed by reading code at this head. Give the coordinator the same populated connection the picker used, or make deriveThinkingChoices refuse to offer anything the gate cannot admit. Regression test: build a connection whose curated metadata grants a thinking level, run the picker and the session gate over the same input, assert the gate accepts every level the picker offered.
| : 'openai-chat'; | ||
| } | ||
|
|
||
| /** xAI documents the Responses reasoning contract for Grok 4.5 and later text models. */ |
There was a problem hiding this comment.
[P1] Compare xAI model versions numerically, not as integers on the minor component. xAiModelSupportsResponses parses the minor version and compares it as a whole number, so grok-4.20 yields minor 20 and passes a >= 4.5 gate that it must fail — the model is 4.2, not 4.20-as-in-twenty. Any xAI release with a two-digit minor takes the wrong contract path and gets a Responses request it does not support. Confirmed by reading code at this head; not executed. Parse the version as a dotted tuple and compare component-wise, or compare against an explicit model allowlist as the rest of this table does. Regression test: assert grok-4.20 resolves to unsupported and grok-4.5 to supported.
| modelId: string, | ||
| ): ThinkingOptions | undefined { | ||
| const inventory = connection.models?.find((model) => model.id === modelId); | ||
| if (inventory?.capabilities?.reasoning === false) return undefined; |
There was a problem hiding this comment.
[P2] Stop letting a heuristic reasoning: false veto curated metadata. This early return fires whenever the inferred inventory says reasoning === false, before the curated per-model facts are consulted, so a model the table explicitly documents as reasoning-capable is reported as having no thinking options the moment the heuristic guesses wrong — which is exactly the failure mode the curated table exists to correct. The authority ordering is inverted relative to the rest of the resolution path. Confirmed by reading code at this head. Consult curated facts first and treat the heuristic only as a fallback for models the table does not cover. Regression test: a connection whose heuristic inventory says reasoning: false for a model the curated table marks reasoning-capable, assert the curated answer wins.
| // from the same catalog facts as every other provider. A model enters only | ||
| // while upstream declares it active, tool-capable, and zero-cost; retirement | ||
| // therefore cannot leave a hand-maintained id that breaks application startup. | ||
| const opencodeFreeModelIds = Object.entries(GENERATED_MODELS_DEV_METADATA.opencode) |
There was a problem hiding this comment.
[P2] Do not admit models to the opencode-free set by price. This derives the allowlist from every entry whose cost is zero, so any model a provider happens to list at zero cost — a promotional tier, a misconfigured entry, a deliberately mispriced upstream listing — is silently admitted to a curated set that downstream code trusts for capability decisions. Price is not identity. Confirmed by reading code at this head. Enumerate the free models explicitly, as the neighbouring curated sets do. Regression test: add a zero-cost entry for a model that is not part of the free tier, assert it is not admitted.
| 'contextWindow', | ||
| 'maxOutputTokens', | ||
| 'capabilities', | ||
| 'thinkingOptions', |
There was a problem hiding this comment.
[P2] Provide a downgrade path for catalog documents carrying thinkingOptions. Adding the key to this exactRecord allowlist changes the persisted shape, and exactRecord throws on unknown keys — so a document written by this build cannot be decoded by any build that predates it. A user who upgrades, runs once, then rolls back loses their connection catalog with a decode error rather than a degraded read. Confirmed by reading code at this head. Either bump the document version with an explicit migration, or make the decoder tolerant of unknown keys on read. Regression test: encode a catalog at this head, decode it with the allowlist from main, assert a graceful outcome. See the cross-PR note in the review body — #3129 edits this same hunk.
|
Reviewed at The core direction is sound: an account/provider inventory that explicitly advertises There is already a concrete regression from the unrelated free-inventory change: the runtime CI has three failing One correction to the existing version-comparison thread: There is also an additional blocking issue in Together with the existing unresolved findings about inconsistent connection shapes, the |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Blocking and non-blocking findings are inline.
Codex-assisted review performed under the maintainer-approved review workflow.
| // from the same catalog facts as every other provider. A model enters only | ||
| // while upstream declares it active, tool-capable, and zero-cost; retirement | ||
| // therefore cannot leave a hand-maintained id that breaks application startup. | ||
| const opencodeFreeModelIds = Object.entries(GENERATED_MODELS_DEV_METADATA.opencode) |
There was a problem hiding this comment.
[P1] Preserve the catalog per-model adapter override on the free alias
This filter now admits muse-spark-1.2-contributor-free: at this SHA the generated OpenCode row is active, free, and tool-capable, and its generated provider override requires @ai-sdk/openai. However, lookupModelProviderOverride() indexes overrides with opencode-free and does not apply the opencode-free -> opencode alias used by metadata lookup, so resolveModelRuntime() falls back to the generic OpenAI-compatible adapter. I reproduced the divergence at this head: the opencode path resolves the model to the native OpenAI adapter and emits { openai: { reasoningEffort: "high" } }, while opencode-free resolves it to the OpenAI-compatible adapter and emits { "opencode-free": { reasoningEffort: "high" } }, also changing reasoning replay semantics. Resolve model-provider overrides through the same provider alias as metadata, and add a regression test for an active free model with an override.
| git push --force origin "HEAD:$BRANCH" | ||
|
|
||
| if [[ "$(gh pr list --head "$BRANCH" --state open --json number --jq length)" == "0" ]]; then | ||
| gh pr create \ |
There was a problem hiding this comment.
[P2] Bind the existing-PR lookup to this repository branch
gh pr list --head "$BRANCH" filters by branch name and does not support an owner-qualified head. A fork PR whose head is also named automation/models-dev-sync therefore makes this count nonzero; the workflow then force-pushes the repository branch but skips gh pr create, leaving that branch without its own sync PR. Query REST or GraphQL with the repository owner as part of the head identity (and exclude cross-repository PRs) before deciding that the automation PR already exists.
4 participants
Summary
Validation
No full test suite was run.