refactor(shared): use canonical profile provider identifiers (#1019)

* refactor(shared): use canonical profile provider identifiers

* test(shared): strengthen profile validator coverage

* test(shared): avoid mutating provider identifiers

* chore(lint): prune profile validator suppressions

Author: WebMad

src/eslint-suppressions.json

@@ -1684,11 +1684,6 @@
 			"count": 2
 		}
 	},
-	"shared/__tests__/ProfileValidator.spec.ts": {
-		"@typescript-eslint/no-explicit-any": {
-			"count": 2
-		}
-	},
 	"shared/__tests__/api.spec.ts": {
 		"@typescript-eslint/no-explicit-any": {
 			"count": 8

src/shared/ProfileValidator.ts

@@ -1,4 +1,4 @@
-import type { ProviderSettings, OrganizationAllowList } from "@roo-code/types"
+import { providerIdentifiers, type ProviderSettings, type OrganizationAllowList } from "@roo-code/types"
 
 export class ProfileValidator {
 	public static isProfileAllowed(profile: ProviderSettings, allowList: OrganizationAllowList): boolean {
@@ -51,36 +51,36 @@ export class ProfileValidator {
 
 	private static getModelIdFromProfile(profile: ProviderSettings): string | undefined {
 		switch (profile.apiProvider) {
-			case "openai":
+			case providerIdentifiers.openai:
 				return profile.openAiModelId
-			case "anthropic":
-			case "openai-native":
-			case "bedrock":
-			case "vertex":
-			case "gemini":
-			case "mistral":
-			case "deepseek":
-			case "xai":
-			case "sambanova":
-			case "fireworks":
-			case "friendli":
+			case providerIdentifiers.anthropic:
+			case providerIdentifiers.openaiNative:
+			case providerIdentifiers.bedrock:
+			case providerIdentifiers.vertex:
+			case providerIdentifiers.gemini:
+			case providerIdentifiers.mistral:
+			case providerIdentifiers.deepseek:
+			case providerIdentifiers.xai:
+			case providerIdentifiers.sambanova:
+			case providerIdentifiers.fireworks:
+			case providerIdentifiers.friendli:
 				return profile.apiModelId
-			case "litellm":
+			case providerIdentifiers.litellm:
 				return profile.litellmModelId
-			case "lmstudio":
+			case providerIdentifiers.lmstudio:
 				return profile.lmStudioModelId
-			case "vscode-lm":
+			case providerIdentifiers.vscodeLm:
 				// We probably need something more flexible for this one, if we need to really support it here.
 				return profile.vsCodeLmModelSelector?.id
-			case "openrouter":
+			case providerIdentifiers.openrouter:
 				return profile.openRouterModelId
-			case "ollama":
+			case providerIdentifiers.ollama:
 				return profile.ollamaModelId
-			case "requesty":
+			case providerIdentifiers.requesty:
 				return profile.requestyModelId
-			case "unbound":
+			case providerIdentifiers.unbound:
 				return profile.unboundModelId
-			case "fake-ai":
+			case providerIdentifiers.fakeAi:
 			default:
 				return undefined
 		}

src/shared/__tests__/ProfileValidator.spec.ts

@@ -1,11 +1,78 @@
 // npx vitest run src/shared/__tests__/ProfileValidator.spec.ts
 
-import type { ProviderSettings, OrganizationAllowList } from "@roo-code/types"
+import { providerIdentifiers, type ProviderSettings, type OrganizationAllowList } from "@roo-code/types"
 
 import { ProfileValidator } from "../ProfileValidator"
 
 describe("ProfileValidator", () => {
 	describe("isProfileAllowed", () => {
+		it.each([
+			["openai", { openAiModelId: "model" }],
+			["anthropic", { apiModelId: "model" }],
+			["openaiNative", { apiModelId: "model" }],
+			["bedrock", { apiModelId: "model" }],
+			["vertex", { apiModelId: "model" }],
+			["gemini", { apiModelId: "model" }],
+			["mistral", { apiModelId: "model" }],
+			["deepseek", { apiModelId: "model" }],
+			["xai", { apiModelId: "model" }],
+			["sambanova", { apiModelId: "model" }],
+			["fireworks", { apiModelId: "model" }],
+			["friendli", { apiModelId: "model" }],
+			["litellm", { litellmModelId: "model" }],
+			["lmstudio", { lmStudioModelId: "model" }],
+			["vscodeLm", { vsCodeLmModelSelector: { id: "model" } }],
+			["openrouter", { openRouterModelId: "model" }],
+			["ollama", { ollamaModelId: "model" }],
+			["requesty", { requestyModelId: "model" }],
+			["unbound", { unboundModelId: "model" }],
+		])("resolves %s model fields through canonical identifiers", (identifierKey, profileSettings) => {
+			const canonicalIdentifier = providerIdentifiers[identifierKey as keyof typeof providerIdentifiers]
+			const modelId = "model"
+
+			const profile = {
+				apiProvider: canonicalIdentifier,
+				...profileSettings,
+			} as ProviderSettings
+			const allowList: OrganizationAllowList = {
+				allowAll: false,
+				providers: {
+					[canonicalIdentifier]: { allowAll: false, models: [modelId] },
+				},
+			}
+
+			expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(true)
+
+			const negativeAllowList: OrganizationAllowList = {
+				allowAll: false,
+				providers: {
+					[canonicalIdentifier]: { allowAll: false, models: ["other-model"] },
+				},
+			}
+
+			expect(ProfileValidator.isProfileAllowed(profile, negativeAllowList)).toBe(false)
+		})
+
+		it.each([
+			{ providerAllowAll: false, expected: false },
+			{ providerAllowAll: true, expected: true },
+		])(
+			"preserves missing-model fallback behavior when provider allowAll is $providerAllowAll",
+			({ providerAllowAll, expected }) => {
+				const profile: ProviderSettings = {
+					apiProvider: providerIdentifiers.openai,
+				}
+				const allowList: OrganizationAllowList = {
+					allowAll: false,
+					providers: {
+						[providerIdentifiers.openai]: { allowAll: providerAllowAll },
+					},
+				}
+
+				expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(expected)
+			},
+		)
+
 		it("should allow any profile when allowAll is true", () => {
 			const allowList: OrganizationAllowList = {
 				allowAll: true,
@@ -168,17 +235,17 @@ describe("ProfileValidator", () => {
 
 		// Test specific providers that use apiModelId
 		const apiModelProviders = [
-			"anthropic",
-			"openai-native",
-			"bedrock",
-			"vertex",
-			"gemini",
-			"mistral",
-			"deepseek",
-			"xai",
-			"sambanova",
-			"fireworks",
-			"friendli",
+			providerIdentifiers.anthropic,
+			providerIdentifiers.openaiNative,
+			providerIdentifiers.bedrock,
+			providerIdentifiers.vertex,
+			providerIdentifiers.gemini,
+			providerIdentifiers.mistral,
+			providerIdentifiers.deepseek,
+			providerIdentifiers.xai,
+			providerIdentifiers.sambanova,
+			providerIdentifiers.fireworks,
+			providerIdentifiers.friendli,
 		]
 
 		apiModelProviders.forEach((provider) => {
@@ -190,7 +257,7 @@ describe("ProfileValidator", () => {
 					},
 				}
 				const profile: ProviderSettings = {
-					apiProvider: provider as any, // Type assertion needed here
+					apiProvider: provider,
 					apiModelId: "test-model",
 				}
 
@@ -203,11 +270,11 @@ describe("ProfileValidator", () => {
 			const allowList: OrganizationAllowList = {
 				allowAll: false,
 				providers: {
-					litellm: { allowAll: false, models: ["test-model"] },
+					[providerIdentifiers.litellm]: { allowAll: false, models: ["test-model"] },
 				},
 			}
 			const profile: ProviderSettings = {
-				apiProvider: "litellm" as any,
+				apiProvider: providerIdentifiers.litellm,
 				litellmModelId: "test-model",
 			}
 
@@ -218,11 +285,11 @@ describe("ProfileValidator", () => {
 			const allowList: OrganizationAllowList = {
 				allowAll: false,
 				providers: {
-					"vscode-lm": { allowAll: false, models: ["copilot-gpt-3.5"] },
+					[providerIdentifiers.vscodeLm]: { allowAll: false, models: ["copilot-gpt-3.5"] },
 				},
 			}
 			const profile: ProviderSettings = {
-				apiProvider: "vscode-lm",
+				apiProvider: providerIdentifiers.vscodeLm,
 				vsCodeLmModelSelector: { id: "copilot-gpt-3.5" },
 			}
 
@@ -278,11 +345,11 @@ describe("ProfileValidator", () => {
 			const allowList: OrganizationAllowList = {
 				allowAll: false,
 				providers: {
-					"fake-ai": { allowAll: false },
+					[providerIdentifiers.fakeAi]: { allowAll: false },
 				},
 			}
 			const profile: ProviderSettings = {
-				apiProvider: "fake-ai",
+				apiProvider: providerIdentifiers.fakeAi,
 			}
 
 			expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(false)