[refactor] Remove deprecated functionality in packages/types/src/provider-settings.ts

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", () => {

packages/types/src/provider-settings.ts

@@ -294,67 +294,34 @@
 export const PROVIDER_SETTINGS_KEYS = providerSettingsSchema.keyof().options
 
 /**
- * @deprecated Use `getModelId()` to resolve the model ID for the active provider.
- */
-export type ModelIdKey = Extract<keyof ProviderSettingsShape, `${string}ModelId`>
-
-/**
- * @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<ProviderName, InternalProvider | CustomProvider | FauxProvider>
-
-/**
- * @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<RetiredProviderName, string> = {
+	[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<TypicalProvider, ModelIdKey>
+const readStringField = (settings: ProviderSettings, field: string): string | undefined => {
+	const value = (settings as Record<string, unknown>)[field]
+	return typeof value === "string" ? value : undefined
+}
 
 export function getModelId(settings: ProviderSettings): string | undefined {
 	if (isProviderName(settings.apiProvider)) {
 		return providerDefinitions[settings.apiProvider]?.getModelId(settings)
 	}
 
 	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

src/core/webview/ClineProvider.ts

@@ -71,7 +71,7 @@
 	DEFAULT_CHECKPOINT_TIMEOUT_SECONDS,
 	getModelId,
 	isRetiredProvider,
-	modelIdKeys,
+	providerDefinitionList,
 	providerIdentifiers,
 } from "@roo-code/types"
 import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock"
@@ -204,7 +204,12 @@
 }
 
 // Keys a model-selector patch may touch: model IDs plus settings reset on model change.
-const PROFILE_MODEL_ID_PATCH_KEYS: ReadonlySet<string> = 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<string> = new Set(
+	providerDefinitionList.flatMap((definition) => (definition.modelIdKey ? [definition.modelIdKey] : [])),
+)
 
 const BEDROCK_CUSTOM_ARN_MODEL_ID = "custom-arn"
 

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",

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)