diff --git a/packages/types/src/__tests__/provider-model-id.test.ts b/packages/types/src/__tests__/provider-model-id.test.ts index 69d77122aa..24d7783c73 100644 --- a/packages/types/src/__tests__/provider-model-id.test.ts +++ b/packages/types/src/__tests__/provider-model-id.test.ts @@ -1,33 +1,20 @@ import { retiredProviderIdentifiers } from "../provider-identifiers.js" +import { providerDefinitionList } from "../providers/index.js" import { getModelId, - modelIdKeys, providerIdentifiers, providerSettingsSchema, + PROVIDER_SETTINGS_KEYS, type ProviderSettings, } from "../index.js" -const expectedModelIdKeys = [ - "apiModelId", - "openRouterModelId", - "openAiModelId", - "ollamaModelId", - "lmStudioModelId", - "lmStudioDraftModelId", - "requestyModelId", - "unboundModelId", - "litellmModelId", - "vercelAiGatewayModelId", - "opencodeGoModelId", - "kenariModelId", - "nanoGptModelId", - "ioIntelligenceModelId", - "zooGatewayModelId", -] as const - -describe("modelIdKeys", () => { - it("preserves every model ID setting and its legacy precedence order", () => { - expect(modelIdKeys).toEqual(expectedModelIdKeys) +describe("provider model ID fields", () => { + it.each( + providerDefinitionList.flatMap((definition) => + definition.modelIdKey ? [[definition.apiProvider, definition.modelIdKey] as const] : [], + ), + )("declares a persisted ProviderSettings key for %s", (_apiProvider, modelIdKey) => { + expect(PROVIDER_SETTINGS_KEYS).toContain(modelIdKey) }) }) @@ -115,14 +102,28 @@ describe("getModelId", () => { expect(getModelId({})).toBeUndefined() }) - it("preserves legacy model ID precedence for retired providers", () => { - const settings: ProviderSettings = { - apiProvider: retiredProviderIdentifiers.groq, - lmStudioDraftModelId: "draft-model", - requestyModelId: "requesty-model", - } + it.each([ + [retiredProviderIdentifiers.cerebras, "apiModelId"], + [retiredProviderIdentifiers.chutes, "apiModelId"], + [retiredProviderIdentifiers.deepinfra, "deepInfraModelId"], + [retiredProviderIdentifiers.doubao, "apiModelId"], + [retiredProviderIdentifiers.featherless, "apiModelId"], + [retiredProviderIdentifiers.groq, "apiModelId"], + [retiredProviderIdentifiers.huggingface, "huggingFaceModelId"], + [retiredProviderIdentifiers.roo, "apiModelId"], + ] as const)("reads the retired provider's own model ID field for %s", (apiProvider, modelIdField) => { + const settings = { + apiProvider, + [modelIdField]: "retired-model", + // A field belonging to a different provider must not win. + requestyModelId: "other-provider-model", + } as ProviderSettings + + expect(getModelId(settings)).toBe("retired-model") + }) - expect(getModelId(settings)).toBe("draft-model") + it("returns undefined for a retired provider without a model ID", () => { + expect(getModelId({ apiProvider: retiredProviderIdentifiers.groq })).toBeUndefined() }) it("resolves a model ID for every provider definition without throwing", () => { diff --git a/packages/types/src/provider-settings.ts b/packages/types/src/provider-settings.ts index ebb48b041b..fba709aad5 100644 --- a/packages/types/src/provider-settings.ts +++ b/packages/types/src/provider-settings.ts @@ -294,58 +294,26 @@ export type ProviderSettingsWithId = z.infer - -/** - * @deprecated Use `getModelId()` to resolve the model ID for the active provider. - */ -export const modelIdKeys = [ - "apiModelId", - "openRouterModelId", - "openAiModelId", - "ollamaModelId", - "lmStudioModelId", - "lmStudioDraftModelId", - "requestyModelId", - "unboundModelId", - "litellmModelId", - "vercelAiGatewayModelId", - "opencodeGoModelId", - "kenariModelId", - "nanoGptModelId", - "ioIntelligenceModelId", - "zooGatewayModelId", -] as const satisfies readonly ModelIdKey[] - -/** - * @deprecated Provider categories should use the specific provider type guards. - */ -export type TypicalProvider = Exclude - -/** - * @deprecated Use the specific provider type guards instead. - */ -export const isTypicalProvider = (key: unknown): key is TypicalProvider => - isProviderName(key) && !isInternalProvider(key) && !isCustomProvider(key) && !isFauxProvider(key) - -/** - * @deprecated Use `getModelId()` instead. This map is retained for API compatibility. + * Model ID field each retired provider used before it was removed. + * + * Retired profiles are preserved verbatim (see `ProviderSettingsManager`), so + * `getModelId()` still needs to read the model the user had selected. */ -export const modelIdKeysByProvider = Object.fromEntries( - providerDefinitionList.flatMap((definition) => { - if (!isTypicalProvider(definition.apiProvider)) { - return [] - } - - if (!definition.modelIdKey) { - throw new Error(`Missing model ID key for provider definition: ${definition.apiProvider}`) - } +const retiredProviderModelIdKeys: Record = { + [retiredProviderIdentifiers.cerebras]: "apiModelId", + [retiredProviderIdentifiers.chutes]: "apiModelId", + [retiredProviderIdentifiers.deepinfra]: "deepInfraModelId", + [retiredProviderIdentifiers.doubao]: "apiModelId", + [retiredProviderIdentifiers.featherless]: "apiModelId", + [retiredProviderIdentifiers.groq]: "apiModelId", + [retiredProviderIdentifiers.huggingface]: "huggingFaceModelId", + [retiredProviderIdentifiers.roo]: "apiModelId", +} - return [[definition.apiProvider, definition.modelIdKey] as const] - }), -) as Record +const readStringField = (settings: ProviderSettings, field: string): string | undefined => { + const value = (settings as Record)[field] + return typeof value === "string" ? value : undefined +} export function getModelId(settings: ProviderSettings): string | undefined { if (isProviderName(settings.apiProvider)) { @@ -353,8 +321,7 @@ export function getModelId(settings: ProviderSettings): string | undefined { } if (typeof settings.apiProvider === "string" && isRetiredProvider(settings.apiProvider)) { - const modelIdKey = modelIdKeys.find((key) => settings[key]) - return modelIdKey ? settings[modelIdKey] : undefined + return readStringField(settings, retiredProviderModelIdKeys[settings.apiProvider]) } return undefined diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 636c022f62..0d75379ec7 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -71,7 +71,7 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, getModelId, isRetiredProvider, - modelIdKeys, + providerDefinitionList, providerIdentifiers, } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" @@ -204,7 +204,12 @@ function scheduleTask( } // Keys a model-selector patch may touch: model IDs plus settings reset on model change. -const PROFILE_MODEL_ID_PATCH_KEYS: ReadonlySet = new Set(modelIdKeys) +// Derived from the provider definitions so a new provider's model field is patchable +// without editing this list. Providers without a `modelIdKey` (VS Code LM and GitHub +// Copilot, which use the nested `vsCodeLmModelSelector`) contribute nothing. +const PROFILE_MODEL_ID_PATCH_KEYS: ReadonlySet = new Set( + providerDefinitionList.flatMap((definition) => (definition.modelIdKey ? [definition.modelIdKey] : [])), +) const BEDROCK_CUSTOM_ARN_MODEL_ID = "custom-arn" diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index c2aec90dcd..ef94f22481 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -5,7 +5,7 @@ import * as vscode from "vscode" import { makeCompositeDisposable } from "../../../test-utils/vscode" import { TelemetryService } from "@roo-code/telemetry" -import { getModelId, RooCodeEventName } from "@roo-code/types" +import { getModelId, providerDefinitionList, RooCodeEventName } from "@roo-code/types" import { ContextProxy } from "../../config/ContextProxy" import type { Mode } from "../../../shared/modes" @@ -538,6 +538,33 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { expect(vscode.window.showErrorMessage).toHaveBeenCalled() }) + test.each( + providerDefinitionList.flatMap((definition) => + definition.modelIdKey ? [[definition.apiProvider, definition.modelIdKey] as const] : [], + ), + )("accepts the model ID field declared by the %s provider definition", async (apiProvider, modelIdKey) => { + provider["providerSettingsManager"].getProfile = vi + .fn() + .mockResolvedValue({ name: "test-config", id: "test-id", apiProvider }) + + await provider.updateProfileModel("test-config", apiProvider, { [modelIdKey]: "patched-model" }) + + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() + const saveConfig = vi.mocked(provider["providerSettingsManager"].saveConfig) + expect(saveConfig).toHaveBeenCalledTimes(1) + expect(saveConfig.mock.calls[0][1]).toMatchObject({ [modelIdKey]: "patched-model" }) + }) + + test("rejects a model ID field that no provider definition declares", async () => { + // `lmStudioDraftModelId` is a speculative-decoding setting, not the provider's selected model. + await provider.updateProfileModel("test-config", providerIdentifiers.openrouter, { + lmStudioDraftModelId: "draft-model", + }) + + expect(provider["providerSettingsManager"].saveConfig).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalled() + }) + test("only allows reset keys to be cleared, so a patch cannot set a Bedrock custom ARN", async () => { await provider.updateProfileModel("test-config", providerIdentifiers.openrouter, { openRouterModelId: "anthropic/claude", diff --git a/src/shared/__tests__/ProfileValidator.spec.ts b/src/shared/__tests__/ProfileValidator.spec.ts index 6e8f404872..65bf8128cc 100644 --- a/src/shared/__tests__/ProfileValidator.spec.ts +++ b/src/shared/__tests__/ProfileValidator.spec.ts @@ -416,10 +416,9 @@ describe("ProfileValidator", () => { ).toBe(false) }) - it("enforces the model allow-list for retired providers using legacy model ID precedence", () => { - // The removed local switch returned `undefined` for retired providers, so they - // could only pass via a provider-level `allowAll`. `getModelId()` resolves them - // through the legacy `modelIdKeys` precedence, so the model list now applies. + it("enforces the model allow-list for retired providers using their own model ID field", () => { + // `getModelId()` resolves retired providers through the retired-provider model ID + // map, so the model list applies instead of only the provider-level `allowAll`. const allowList: OrganizationAllowList = { allowAll: false, providers: { @@ -429,13 +428,29 @@ describe("ProfileValidator", () => { expect( ProfileValidator.isProfileAllowed( - { apiProvider: retiredProviderIdentifiers.groq, requestyModelId: "allowed-model" }, + { apiProvider: retiredProviderIdentifiers.groq, apiModelId: "allowed-model" }, allowList, ), ).toBe(true) expect( ProfileValidator.isProfileAllowed( - { apiProvider: retiredProviderIdentifiers.groq, requestyModelId: "other-model" }, + { apiProvider: retiredProviderIdentifiers.groq, apiModelId: "other-model" }, + allowList, + ), + ).toBe(false) + }) + + it("does not borrow another provider's model ID field for a retired provider", () => { + const allowList: OrganizationAllowList = { + allowAll: false, + providers: { + [retiredProviderIdentifiers.groq]: { allowAll: false, models: ["allowed-model"] }, + }, + } + + expect( + ProfileValidator.isProfileAllowed( + { apiProvider: retiredProviderIdentifiers.groq, requestyModelId: "allowed-model" }, allowList, ), ).toBe(false)