Give auto-chain fallback models the call options they would have received as primary - #4327
Give auto-chain fallback models the call options they would have received as primary#4327keppo-bot[bot] wants to merge 2 commits into
Conversation
Call options are resolved for the selected model before a request is made, so they encode that model's constraints. The fallback wrapper reused them verbatim when switching models mid-stream or between attempts, so a chain that crosses providers forwarded the primary's temperature to a model that rejects it — observed as a gpt-5.6 stream blip failing over to an Anthropic thinking model, whose 400 (temperature may only be set to 1 when thinking is enabled) turned a recoverable error into a fatal one. On any non-primary model, drop temperature and let the provider default apply: an absent temperature is valid everywhere, and this layer has no catalog access to recompute a per-model value. Covers the sticky-index case too — after a failover the index stays on the fallback for three minutes, so a fresh request's first call can already target a fallback model.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
| private optionsForCurrentModel( | ||
| options: LanguageModelV3CallOptions, | ||
| ): LanguageModelV3CallOptions { | ||
| if (this.currentModelIndex === 0) return options; |
There was a problem hiding this comment.
🟡 MEDIUM
Temperature is stripped even on same-provider fallback chains
The guard keys on currentModelIndex rather than on whether the fallback actually crosses providers, so the strip also fires on the two chains that never leave OpenRouter: the auto/free chain (FREE_OPENROUTER_MODEL_NAMES) and getOpenRouterAutoFallbackModelClient in get_model_client.ts:275, which builds [primaryModel, openrouter/free]. Both of those catalog entries specify temperature: 0 (language_model_constants.ts, auto/free and the OpenRouter model entries), so a failover inside a single provider silently swaps deterministic decoding for the provider default (typically 1.0) with none of the cross-provider 400 hazard that motivates the change. The user sees noticeably more variable code generation from a fallback that was configured to be deterministic, and nothing in the logs explains why.
💡 Suggestion: Strip only when the provider actually changes, e.g. compare the primary model's provider against getUnderlyingModel().provider and return options unchanged when they match. If dropping unconditionally is the deliberate choice, say so in the comment so the single-provider chains are visibly in scope.
| ): LanguageModelV3CallOptions { | ||
| if (this.currentModelIndex === 0) return options; | ||
| if (options.temperature === undefined) return options; | ||
| const { temperature: _dropped, ...rest } = options; |
There was a problem hiding this comment.
🟡 MEDIUM
maxOutputTokens has the same cross-provider hazard and is forwarded
The PR description asks reviewers to weigh 'temperature only, deliberately' but argues only about providerOptions being provider-keyed and therefore inert. maxOutputTokens comes from the identical resolution path: local_agent_handler.ts:1030 calls getMaxTokens(settings.selectedModel) on the line directly above the getTemperature call this comment cites, and both are resolved against the selected model's catalog entry before the request. It is then forwarded verbatim across a provider switch, and a value above the fallback model's own output cap is a hard 400 in exactly the same way temperature was. Today's auto chain happens to be safe because auto/auto pins a conservative 32k, but that is incidental to a catalog value that can change, and OpenRouter chains inherit the user-selected model's cap instead.
💡 Suggestion: Either extend the strip to maxOutputTokens (an absent value falls back to the provider default the same way temperature does), or amend the comment and PR description to state that maxOutputTokens is knowingly left forwarded and why the current catalog values make it safe.
| } from "@ai-sdk/provider"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { createFallback } from "./fallback_ai_model"; |
There was a problem hiding this comment.
🟡 MEDIUM
New unit test does not mock electron-log unlike every sibling test
fallback_ai_model.ts imports electron-log at module scope and calls log.scope() at import time, and defaultShouldRetryThisError calls logger.info on every retryable-error check, which both failover tests hit. All 26 existing test files under src/ipc/utils that transitively pull in electron-log declare vi.mock('electron-log', ...) first, including get_model_client.test.ts:15 which imports this very module; the unit project in vitest.config.ts has no setupFiles, so there is no shared mock to inherit. I could not run the suite to confirm a failure because node_modules is not installed in this checkout, so this is a convention/risk observation rather than an observed break, but the fix is a no-op if the import already works.
💡 Suggestion: Add the standard three-line mock used across this directory: vi.mock("electron-log", () => ({ default: { scope: () => ({ debug: vi.fn(), info: vi.fn(), log: vi.fn(), warn: vi.fn(), error: vi.fn() }) } })), importing vi from vitest.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues The core fix is correct and narrowly scoped: Three things worth weighing before merge, none of them blockers. The most substantive is that the strip is keyed on index, not on provider change, so it also fires on the two fallback chains that never cross providers — Confidence notes: the diff is complete and not truncated. I could not execute the new test file — Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
…ceived as primary Dropping temperature made the failover request valid but still wrong: the Anthropic fallback ran without its adaptive-thinking providerOptions (the chain client is built with builtinProviderId 'openai', so only providerOptions.openai was on the request), and the primary's maxOutputTokens is a latent 400 for models with smaller caps. At chain-build time — where catalog access exists — compute each entry's own temperature and output cap (getTemperature/getMaxTokens) and its provider family's thinking/reasoning options at the user's chosen effort (getModelScopedProviderOptions, kept in lockstep with getProviderOptions' family branches and pinned to them by test). The wrapper applies the current model's entry on any non-primary call: temperature and cap are replaced (an undefined override means unset, never 'inherit the primary's'), family providerOptions merge over the request-scoped keys, which pass through. Models without an entry keep the conservative default: temperature stripped.
keppo-bot
Bot
changed the title
| providerOptions: getModelScopedProviderOptions({ | ||
| providerId: resolvedModel.providerId, | ||
| modelName: resolvedModel.apiName, | ||
| modelSelection: model, |
There was a problem hiding this comment.
🔴 HIGH
Auto chain passes a model without effortLevel as ModelSelection
getProModelClient receives model: LargeLanguageModel (see the signature at line 289 and the call site at line 160, which passes getAutoSidekickRuntimeModel(selectedModel)), and LargeLanguageModel has no effortLevel. getModelScopedProviderOptions declares modelSelection: ModelSelection, where effortLevel is required, so this argument is a type error and tsc should reject the build. Even if the type were widened, getModelEffort reads modelSelection.effortLevel, so every chain entry would be built with reasoningEffort: undefined (OpenAI) and effort: undefined (Anthropic). Because the new optionsForCurrentModel replaces the whole providerOptions.openai / providerOptions.anthropic key rather than deep-merging, that undefined effort would also overwrite the correct effort the caller computed via getProviderOptions - so a fallback would silently lose the user's chosen reasoning effort. The surrounding scope in getModelClient already has a real ModelSelection (modelSelection, line 94) that is not threaded into getProModelClient.
💡 Suggestion: Thread the resolved ModelSelection (getModelClient's modelSelection) into getProModelClient and pass that to getModelScopedProviderOptions instead of model.
| private optionsForCurrentModel( | ||
| options: LanguageModelV3CallOptions, | ||
| ): LanguageModelV3CallOptions { | ||
| if (this.currentModelIndex === 0) return options; |
There was a problem hiding this comment.
🟡 MEDIUM
Primary chain entry still gets the auto pseudo-model's temperature
optionsForCurrentModel short-circuits on index 0, so the first chain entry keeps the caller's options. In auto mode those options are not the primary model's: local_agent_handler computes getTemperature(settings.selectedModel) with selectedModel = {provider: "auto", name: "auto"}, and the catalog's auto/auto entry has temperature: 0, while the resolved index-0 OpenAI models are marked in the catalog as requiring temperature 1. So the exact mismatch this PR fixes for indexes 1..n is left in place for index 0, and modelCallOptions[0] is computed in get_model_client (a catalog lookup per request) but can never be read. Applying the entry's own options at index 0 too - or documenting explicitly why index 0 must not be touched - would make the mechanism consistent and remove the dead entry.
💡 Suggestion: Either apply modelCallOptions[index] uniformly (including index 0) or stop computing/passing an entry for index 0 and note why the primary is exempt.
| * Kept in lockstep with the family branches of getProviderOptions below — | ||
| * update both when a provider's thinking config changes. | ||
| */ | ||
| export function getModelScopedProviderOptions({ |
There was a problem hiding this comment.
🟡 MEDIUM
Provider family options duplicated between two functions
getModelScopedProviderOptions re-implements the OpenAI, Anthropic and Google/Vertex branches of getProviderOptions verbatim, and the doc comment acknowledges the two must be kept 'in lockstep' by hand. The new provider_options.test.ts lockstep assertions cover only three provider/model combinations at one effort level, so a future change (a new provider family, a new gemini gating rule, or the reasoningEffortProviderId branch which the scoped version omits entirely) can drift without any test failing. This is exactly the kind of duplication that produces the next cross-provider option bug.
💡 Suggestion: Have getProviderOptions build its family slice by calling getModelScopedProviderOptions so there is a single source of truth, rather than maintaining two copies.
| if (providerId === "anthropic") { | ||
| return { anthropic: getAnthropicProviderOptions(modelSelection) }; | ||
| } | ||
| if (providerId === "google" || providerId === "vertex") { |
There was a problem hiding this comment.
🟡 MEDIUM
Google chain entry's providerOptions likely ignored by engine model
The Google/Vertex chain entry is created with provider(resolvedModelId, ...), i.e. an OpenAICompatibleChatLanguageModel whose provider name is dyad-engine. AI SDK OpenAI-compatible models only forward the providerOptions key matching their own provider name - which is why llm_engine_provider.ts reads dyad options out of the dyad-engine key and injects Google thinking config through getExtraProviderOptionsForEngine in the custom fetch instead. If that holds, the new providerOptions.google = { thinkingConfig } for the Google entry never reaches the request and is dead configuration, while the engine fetch wrapper already supplies the equivalent per-entry thinking options based on chatParams.providerId. I could not run the app or inspect node_modules in this environment, so please confirm against the installed AI SDK before keeping the branch.
💡 Suggestion: Verify whether the OpenAI-compatible engine model reads providerOptions.google; if not, drop the google/vertex branch (or the whole scoped-providerOptions addition for non-Anthropic entries) rather than shipping inert config.
| // (temperature/maxOutputTokens) via findLanguageModel -> getLanguageModels. | ||
| // An empty catalog means "no per-model data", which exercises the | ||
| // conservative path without inventing model entries these tests don't need. | ||
| getLanguageModels: vi.fn(async () => []), |
There was a problem hiding this comment.
🟡 MEDIUM
New per-model call-option wiring has no test coverage
The only test touching the new get_model_client code path mocks getLanguageModels to return an empty catalog, so temperature and maxOutputTokens resolve to undefined and nothing asserts that modelCallOptions is built or handed to createFallback in the right order. The fallback_ai_model tests exercise modelCallOptions only with hand-written literals. As a result no test covers the actual chain-building logic - including the missing effortLevel defect above, or a future off-by-one between the models and modelCallOptions arrays.
💡 Suggestion: Add a get_model_client test with a non-empty mocked catalog that asserts createFallback receives per-index call options (temperature, maxOutputTokens, and the family providerOptions with the user's effort level).
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The core diagnosis is right and the fallback wrapper change is well-tested: call options are resolved once for the primary selection, and forwarding The blocker is on the other side of the wiring: the auto chain builds each entry's provider options from a Note also that the PR title and description describe only "stop forwarding the primary model's temperature", while the implementation additionally forwards per-model The diff is complete (not truncated). I could not run Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results✅ All tests passed!
Total: 289 tests passed (12 skipped) |
1 participant
Summary
The auto-mode fallback chain reused the original request's call options when it
switched models, and those options were resolved against the selected model's
constraints before the request began. Observed in practice: a transient gpt-5.6
stream error failed over to the chain's Anthropic model, which 400'd on the
forwarded
temperature— the resilience mechanism converted a recoverable blipinto a fatal stream error.
The deeper problem is that the fallback got an OpenAI-shaped request: the chain
client is built with
builtinProviderId: "openai", so the request carriedproviderOptions.openaiand noproviderOptions.anthropic— no adaptivethinking at all (with which the temperature would have been legal). The
primary's
maxOutputTokensis the same class of latent 400 for models withsmaller caps.
Fix, in two layers:
get_model_client.ts, where catalog access exists): eachchain entry gets the call options it would have received had it been the
primary selection — its own
temperatureand output cap from the catalog,and its provider family's thinking/reasoning options at the user's chosen
effort (
getModelScopedProviderOptions).fallback_ai_model.ts): on any non-primary call, apply thatmodel's entry — temperature/cap replaced, family
providerOptionsmergedover the request-scoped keys, which pass through untouched. Models without an
entry keep a conservative default:
temperaturestripped (absent temperatureis valid on every provider), everything else forwarded.
Decisions a reviewer should weigh:
prompt,tools,headers, and the
dyad-enginerequest metadata are request-scoped and passthrough unchanged.
undefinedin an override means unset, never "inheritthe primary's" — inheriting is precisely the bug.
getModelScopedProviderOptionsduplicates the family branches ofgetProviderOptionsrather than refactoring the latter, to keep this PR'sblast radius out of the primary request path. A test pins the two in
lockstep: it asserts the scoped slice equals what
getProviderOptionsemitsfor the same family, so drift breaks CI. The google branch keys off the
resolved model's API name where
getProviderOptionsuses the selectedmodel's name — for chain entries the resolved name is the correct source.
createFallbacksites (openrouter chains) pass nooverrides. They are same-provider chains where forwarded options are
homogeneous; they get the conservative temperature-strip on failover, a
minor behaviour change (provider default instead of forwarded value) that is
safe by construction.
fallback model for three minutes, so a fresh request's first attempt can
target a non-primary model with primary-computed options — same bug without
a same-request failover.
optionsForCurrentModelkeys on the current index,not on "did we just switch". Tested.
get_model_clienttests mock the catalog as empty, which exercisesthe conservative no-entry path; the override path is tested at the wrapper
layer with explicit entries.