-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Give auto-chain fallback models the call options they would have received as primary #4327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,238 @@ | ||
| import type { | ||
| LanguageModelV3, | ||
| LanguageModelV3CallOptions, | ||
| LanguageModelV3StreamPart, | ||
| } from "@ai-sdk/provider"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { createFallback } from "./fallback_ai_model"; | ||
|
|
||
| /** | ||
| * The regression under test: call options (temperature) are resolved for the | ||
| * PRIMARY model before the request; forwarding them verbatim to a fallback of | ||
| * a different provider produced a hard 400 (Anthropic rejects an explicit | ||
| * temperature for thinking models), turning a recoverable stream blip into a | ||
| * fatal error. On any non-primary model the fallback wrapper must drop | ||
| * `temperature` and let the provider default apply. | ||
| */ | ||
|
|
||
| function textStream(): ReadableStream<LanguageModelV3StreamPart> { | ||
| return new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue({ type: "stream-start", warnings: [] } as any); | ||
| controller.enqueue({ type: "text-delta", id: "1", delta: "ok" } as any); | ||
| controller.close(); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| function fakeModel(params: { | ||
| modelId: string; | ||
| behavior: "succeed" | "reject-retryable"; | ||
| seen: LanguageModelV3CallOptions[]; | ||
| }): LanguageModelV3 { | ||
| return { | ||
| specificationVersion: "v3", | ||
| provider: "fake", | ||
| modelId: params.modelId, | ||
| supportedUrls: {}, | ||
| async doGenerate() { | ||
| throw new Error("not used"); | ||
| }, | ||
| async doStream(options: LanguageModelV3CallOptions) { | ||
| params.seen.push(options); | ||
| if (params.behavior === "reject-retryable") { | ||
| // Matches RETRYABLE_ERROR_PATTERNS ("service unavailable"). | ||
| throw new Error("service unavailable"); | ||
| } | ||
| return { stream: textStream() }; | ||
| }, | ||
| } as unknown as LanguageModelV3; | ||
| } | ||
|
|
||
| async function drain(stream: ReadableStream<LanguageModelV3StreamPart>) { | ||
| const reader = stream.getReader(); | ||
| while (!(await reader.read()).done) { | ||
| // consume | ||
| } | ||
| } | ||
|
|
||
| describe("fallback model call options", () => { | ||
| it("passes temperature to the primary model untouched", async () => { | ||
| const seen: LanguageModelV3CallOptions[] = []; | ||
| const model = createFallback({ | ||
| models: [fakeModel({ modelId: "primary", behavior: "succeed", seen })], | ||
| }) as unknown as LanguageModelV3; | ||
|
|
||
| const result = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 1, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(result.stream); | ||
|
|
||
| expect(seen).toHaveLength(1); | ||
| expect(seen[0].temperature).toBe(1); | ||
| }); | ||
|
|
||
| it("drops temperature when failing over to a non-primary model", async () => { | ||
| const primarySeen: LanguageModelV3CallOptions[] = []; | ||
| const fallbackSeen: LanguageModelV3CallOptions[] = []; | ||
| const model = createFallback({ | ||
| models: [ | ||
| fakeModel({ | ||
| modelId: "primary", | ||
| behavior: "reject-retryable", | ||
| seen: primarySeen, | ||
| }), | ||
| fakeModel({ | ||
| modelId: "fallback", | ||
| behavior: "succeed", | ||
| seen: fallbackSeen, | ||
| }), | ||
| ], | ||
| }) as unknown as LanguageModelV3; | ||
|
|
||
| const result = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 1, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(result.stream); | ||
|
|
||
| // Primary was tried with the caller's options... | ||
| expect(primarySeen.length).toBeGreaterThanOrEqual(1); | ||
| expect(primarySeen[0].temperature).toBe(1); | ||
| // ...the fallback's provider default must apply instead. | ||
| expect(fallbackSeen).toHaveLength(1); | ||
| expect(fallbackSeen[0].temperature).toBeUndefined(); | ||
| // Everything else survives the strip. | ||
| expect(fallbackSeen[0].prompt).toEqual([]); | ||
| }); | ||
|
|
||
| it("applies a fallback model's own call options as if it were primary", async () => { | ||
| const primarySeen: LanguageModelV3CallOptions[] = []; | ||
| const fallbackSeen: LanguageModelV3CallOptions[] = []; | ||
| const model = createFallback({ | ||
| models: [ | ||
| fakeModel({ | ||
| modelId: "primary", | ||
| behavior: "reject-retryable", | ||
| seen: primarySeen, | ||
| }), | ||
| fakeModel({ | ||
| modelId: "fallback", | ||
| behavior: "succeed", | ||
| seen: fallbackSeen, | ||
| }), | ||
| ], | ||
| modelCallOptions: [ | ||
| undefined, // primary always gets the caller's options | ||
| { | ||
| temperature: 1, | ||
| maxOutputTokens: 32_000, | ||
| providerOptions: { | ||
| anthropic: { thinking: { type: "adaptive" } }, | ||
| }, | ||
| }, | ||
| ], | ||
| }) as unknown as LanguageModelV3; | ||
|
|
||
| const result = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 0.2, | ||
| maxOutputTokens: 128_000, | ||
| providerOptions: { | ||
| "dyad-engine": { dyadRequestId: "req-1" }, | ||
| openai: { reasoningEffort: "medium" }, | ||
| }, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(result.stream); | ||
|
|
||
| expect(fallbackSeen).toHaveLength(1); | ||
| const seen = fallbackSeen[0]; | ||
| // The model-derived subset is the fallback's own... | ||
| expect(seen.temperature).toBe(1); | ||
| expect(seen.maxOutputTokens).toBe(32_000); | ||
| expect((seen.providerOptions as any).anthropic).toEqual({ | ||
| thinking: { type: "adaptive" }, | ||
| }); | ||
| // ...request-scoped options pass through untouched. | ||
| expect((seen.providerOptions as any)["dyad-engine"]).toEqual({ | ||
| dyadRequestId: "req-1", | ||
| }); | ||
| expect(seen.prompt).toEqual([]); | ||
| }); | ||
|
|
||
| it("unsets temperature when the fallback's own options have none", async () => { | ||
| const fallbackSeen: LanguageModelV3CallOptions[] = []; | ||
| const model = createFallback({ | ||
| models: [ | ||
| fakeModel({ | ||
| modelId: "primary", | ||
| behavior: "reject-retryable", | ||
| seen: [], | ||
| }), | ||
| fakeModel({ | ||
| modelId: "fallback", | ||
| behavior: "succeed", | ||
| seen: fallbackSeen, | ||
| }), | ||
| ], | ||
| modelCallOptions: [ | ||
| undefined, | ||
| // catalog had no temperature/cap for this model: undefined means | ||
| // "unset", never "inherit the primary's" | ||
| { providerOptions: { anthropic: { thinking: { type: "adaptive" } } } }, | ||
| ], | ||
| }) as unknown as LanguageModelV3; | ||
|
|
||
| const result = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 0.2, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(result.stream); | ||
|
|
||
| expect(fallbackSeen).toHaveLength(1); | ||
| expect(fallbackSeen[0].temperature).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("drops temperature on a sticky non-primary index without a same-request failover", async () => { | ||
| // After a failover the index stays on the fallback for modelResetInterval; | ||
| // a FRESH request's first call then already targets the fallback while its | ||
| // options were still computed for the primary selection. | ||
| const primarySeen: LanguageModelV3CallOptions[] = []; | ||
| const fallbackSeen: LanguageModelV3CallOptions[] = []; | ||
| const model = createFallback({ | ||
| models: [ | ||
| fakeModel({ | ||
| modelId: "primary", | ||
| behavior: "reject-retryable", | ||
| seen: primarySeen, | ||
| }), | ||
| fakeModel({ | ||
| modelId: "fallback", | ||
| behavior: "succeed", | ||
| seen: fallbackSeen, | ||
| }), | ||
| ], | ||
| }) as unknown as LanguageModelV3; | ||
|
|
||
| // First request fails over primary -> fallback. | ||
| const first = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 1, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(first.stream); | ||
|
|
||
| // Second request starts on the sticky fallback index. | ||
| const second = await model.doStream({ | ||
| prompt: [], | ||
| temperature: 1, | ||
| } as unknown as LanguageModelV3CallOptions); | ||
| await drain(second.stream); | ||
|
|
||
| expect(fallbackSeen).toHaveLength(2); | ||
| expect(fallbackSeen[1].temperature).toBeUndefined(); | ||
| // The primary saw only the first request's attempt. | ||
| expect(primarySeen.every((o) => o.temperature === 1)).toBe(true); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,29 @@ import { DyadError, DyadErrorKind } from "@/errors/dyad_error"; | |
| const logger = log.scope("fallback_model"); | ||
|
|
||
| // Types | ||
|
|
||
| /** | ||
| * The model-derived subset of call options — what a model should receive | ||
| * because of what it is, as opposed to what the request is. Everything else | ||
| * (prompt, tools, headers, request metadata) passes through unchanged. | ||
| */ | ||
| export interface FallbackModelCallOptions { | ||
| temperature?: number; | ||
| maxOutputTokens?: number; | ||
| providerOptions?: Record<string, unknown>; | ||
| } | ||
|
|
||
| interface FallbackSettings { | ||
| models: Array<LanguageModel>; | ||
| /** | ||
| * Per-model call-option overrides, parallel to `models`. The caller's | ||
| * options are computed for the PRIMARY selection and encode that model's | ||
| * constraints; an entry here expresses what the model at the same index | ||
| * would have received had it been selected as primary. Models without an | ||
| * entry get the conservative default on failover: `temperature` stripped | ||
| * (valid on every provider), everything else forwarded. | ||
| */ | ||
| modelCallOptions?: Array<FallbackModelCallOptions | undefined>; | ||
| } | ||
|
|
||
| interface RetryState { | ||
|
|
@@ -177,6 +198,59 @@ class FallbackModel implements LanguageModelV3 { | |
| return model; | ||
| } | ||
|
|
||
| /** | ||
| * Call options are resolved for the PRIMARY model before the request is made | ||
| * (e.g. `getTemperature(settings.selectedModel)`), so they encode that | ||
| * model's constraints, not the fallback's. Forwarding them verbatim across a | ||
| * provider switch produces hard 400s — observed: a gpt-5.6 stream error | ||
| * failed over to an Anthropic thinking model, which rejected the forwarded | ||
| * `temperature` ("`temperature` may only be set to 1 when thinking is | ||
| * enabled"), converting a recoverable blip into a fatal stream error. | ||
| * | ||
| * When calling any model other than the primary, apply that model's | ||
| * `modelCallOptions` entry — the options it would have received as the | ||
| * primary selection, computed at chain-build time where catalog access | ||
| * exists. Without an entry, fall back to stripping `temperature` (an absent | ||
| * temperature is valid on every provider). Applies to sticky-index first | ||
| * attempts too — after a failover, `currentModelIndex` stays non-zero for | ||
| * `modelResetInterval`, so a fresh request's first call can already target a | ||
| * fallback model. | ||
| */ | ||
| private optionsForCurrentModel( | ||
| options: LanguageModelV3CallOptions, | ||
| ): LanguageModelV3CallOptions { | ||
| if (this.currentModelIndex === 0) return options; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM Primary chain entry still gets the auto pseudo-model's temperature
💡 Suggestion: Either apply |
||
| const overrides = this.settings.modelCallOptions?.[this.currentModelIndex]; | ||
| if (!overrides) { | ||
| // No per-model knowledge: strip temperature (an absent temperature is | ||
| // valid on every provider) and forward the rest. | ||
| if (options.temperature === undefined) return options; | ||
| const { temperature: _dropped, ...rest } = options; | ||
| return rest; | ||
| } | ||
| // Rebuild the model-derived subset as if this model had been primary. | ||
| // temperature is REPLACED (undefined in the overrides means "unset", not | ||
| // "keep the primary's"); maxOutputTokens falls back to the caller's when | ||
| // the catalog had none; providerOptions merge at the provider-family key — | ||
| // request-scoped keys (e.g. dyad-engine) pass through, the fallback's | ||
| // family entry is added, and a stale foreign family key is inert because | ||
| // providers only read their own key. | ||
| const { temperature: _replaced, ...rest } = options; | ||
| return { | ||
| ...rest, | ||
| ...(overrides.temperature !== undefined | ||
| ? { temperature: overrides.temperature } | ||
| : {}), | ||
| ...(overrides.maxOutputTokens !== undefined | ||
| ? { maxOutputTokens: overrides.maxOutputTokens } | ||
| : {}), | ||
| providerOptions: { | ||
| ...options.providerOptions, | ||
| ...(overrides.providerOptions ?? {}), | ||
| } as LanguageModelV3CallOptions["providerOptions"], | ||
| }; | ||
| } | ||
|
|
||
| private checkAndResetModel(): void { | ||
| // Only reset if we're not currently in a retry cycle | ||
| if (this.isRetrying) return; | ||
|
|
@@ -269,7 +343,9 @@ class FallbackModel implements LanguageModelV3 { | |
| this.checkAndResetModel(); | ||
|
|
||
| return this.retry(async (retryState) => { | ||
| const result = await this.getUnderlyingModel().doStream(options); | ||
| const result = await this.getUnderlyingModel().doStream( | ||
| this.optionsForCurrentModel(options), | ||
| ); | ||
|
|
||
| // Create a wrapped stream that handles errors gracefully | ||
| const wrappedStream = this.createWrappedStream( | ||
|
|
@@ -380,7 +456,7 @@ class FallbackModel implements LanguageModelV3 { | |
| try { | ||
| const nextResult = await fallbackModel | ||
| .getUnderlyingModel() | ||
| .doStream(options); | ||
| .doStream(fallbackModel.optionsForCurrentModel(options)); | ||
| await processStream(nextResult.stream); | ||
| } catch (nextError) { | ||
| controller.error(nextError); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ vi.mock("./model_effort", () => ({ | |
| })); | ||
|
|
||
| vi.mock("../shared/language_model_helpers", () => ({ | ||
| // The auto chain now computes each fallback model's own call options | ||
| // (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 () => []), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM New per-model call-option wiring has no test coverage The only test touching the new get_model_client code path mocks 💡 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). |
||
| getLanguageModelProviders: vi.fn(async () => [ | ||
| { | ||
| id: "auto", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.