From 20333f81ea8514c3cf58bb5e8cab1ee3f78a0cb4 Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Sat, 25 Jul 2026 22:25:41 +0300 Subject: [PATCH 1/4] refactor(shared): use canonical profile provider identifiers --- src/shared/ProfileValidator.ts | 42 ++++----- src/shared/__tests__/ProfileValidator.spec.ts | 88 ++++++++++++++++++- 2 files changed, 108 insertions(+), 22 deletions(-) diff --git a/src/shared/ProfileValidator.ts b/src/shared/ProfileValidator.ts index bb56718a7d..923582bd06 100644 --- a/src/shared/ProfileValidator.ts +++ b/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 } diff --git a/src/shared/__tests__/ProfileValidator.spec.ts b/src/shared/__tests__/ProfileValidator.spec.ts index efeb7a4820..4abbb5843e 100644 --- a/src/shared/__tests__/ProfileValidator.spec.ts +++ b/src/shared/__tests__/ProfileValidator.spec.ts @@ -1,11 +1,97 @@ // 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 identifiers = providerIdentifiers as Record + const originalIdentifier = identifiers[identifierKey] + const canonicalIdentifier = `canonical-${identifierKey}` + const modelId = "model" + + try { + identifiers[identifierKey] = canonicalIdentifier + + 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) + } finally { + identifiers[identifierKey] = originalIdentifier + } + }) + + it.each([ + { identifierKey: "fakeAi", profileSettings: {}, modelId: undefined, expected: false }, + { + identifierKey: "fakeAi", + profileSettings: {}, + modelId: undefined, + expected: true, + providerAllowAll: true, + }, + ])( + "preserves canonical fallback behavior when provider allowAll is $providerAllowAll", + ({ identifierKey, profileSettings, modelId, expected, providerAllowAll = false }) => { + const identifiers = providerIdentifiers as Record + const originalIdentifier = identifiers[identifierKey] + const canonicalIdentifier = `canonical-${identifierKey}` + + try { + identifiers[identifierKey] = canonicalIdentifier + + const profile = { + apiProvider: canonicalIdentifier, + ...profileSettings, + } as unknown as ProviderSettings + const allowList: OrganizationAllowList = { + allowAll: false, + providers: { + [canonicalIdentifier]: { + allowAll: providerAllowAll, + models: modelId ? [modelId] : undefined, + }, + }, + } + + expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(expected) + } finally { + identifiers[identifierKey] = originalIdentifier + } + }, + ) + it("should allow any profile when allowAll is true", () => { const allowList: OrganizationAllowList = { allowAll: true, From 3187ecc2f27976704bc21f5003035b78dbc3d941 Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Sun, 26 Jul 2026 20:02:17 +0300 Subject: [PATCH 2/4] test(shared): strengthen profile validator coverage --- src/shared/__tests__/ProfileValidator.spec.ts | 93 ++++++++----------- 1 file changed, 41 insertions(+), 52 deletions(-) diff --git a/src/shared/__tests__/ProfileValidator.spec.ts b/src/shared/__tests__/ProfileValidator.spec.ts index 4abbb5843e..84a9354858 100644 --- a/src/shared/__tests__/ProfileValidator.spec.ts +++ b/src/shared/__tests__/ProfileValidator.spec.ts @@ -47,48 +47,37 @@ describe("ProfileValidator", () => { } 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) } finally { identifiers[identifierKey] = originalIdentifier } }) it.each([ - { identifierKey: "fakeAi", profileSettings: {}, modelId: undefined, expected: false }, - { - identifierKey: "fakeAi", - profileSettings: {}, - modelId: undefined, - expected: true, - providerAllowAll: true, - }, + { providerAllowAll: false, expected: false }, + { providerAllowAll: true, expected: true }, ])( - "preserves canonical fallback behavior when provider allowAll is $providerAllowAll", - ({ identifierKey, profileSettings, modelId, expected, providerAllowAll = false }) => { - const identifiers = providerIdentifiers as Record - const originalIdentifier = identifiers[identifierKey] - const canonicalIdentifier = `canonical-${identifierKey}` - - try { - identifiers[identifierKey] = canonicalIdentifier - - const profile = { - apiProvider: canonicalIdentifier, - ...profileSettings, - } as unknown as ProviderSettings - const allowList: OrganizationAllowList = { - allowAll: false, - providers: { - [canonicalIdentifier]: { - allowAll: providerAllowAll, - models: modelId ? [modelId] : undefined, - }, - }, - } - - expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(expected) - } finally { - identifiers[identifierKey] = originalIdentifier + "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) }, ) @@ -254,17 +243,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) => { @@ -276,7 +265,7 @@ describe("ProfileValidator", () => { }, } const profile: ProviderSettings = { - apiProvider: provider as any, // Type assertion needed here + apiProvider: provider, apiModelId: "test-model", } @@ -289,11 +278,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", } @@ -304,11 +293,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" }, } @@ -364,11 +353,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) From 5b5e8c28d1f94f4f10a47a6a642b4fdc90ea1253 Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Sun, 26 Jul 2026 20:04:45 +0300 Subject: [PATCH 3/4] test(shared): avoid mutating provider identifiers --- src/shared/__tests__/ProfileValidator.spec.ts | 46 ++++++++----------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/src/shared/__tests__/ProfileValidator.spec.ts b/src/shared/__tests__/ProfileValidator.spec.ts index 84a9354858..ace96c9fd0 100644 --- a/src/shared/__tests__/ProfileValidator.spec.ts +++ b/src/shared/__tests__/ProfileValidator.spec.ts @@ -27,38 +27,30 @@ describe("ProfileValidator", () => { ["requesty", { requestyModelId: "model" }], ["unbound", { unboundModelId: "model" }], ])("resolves %s model fields through canonical identifiers", (identifierKey, profileSettings) => { - const identifiers = providerIdentifiers as Record - const originalIdentifier = identifiers[identifierKey] - const canonicalIdentifier = `canonical-${identifierKey}` + const canonicalIdentifier = providerIdentifiers[identifierKey as keyof typeof providerIdentifiers] const modelId = "model" - try { - identifiers[identifierKey] = canonicalIdentifier - - 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 profile = { + apiProvider: canonicalIdentifier, + ...profileSettings, + } as ProviderSettings + const allowList: OrganizationAllowList = { + allowAll: false, + providers: { + [canonicalIdentifier]: { allowAll: false, models: [modelId] }, + }, + } - const negativeAllowList: OrganizationAllowList = { - allowAll: false, - providers: { - [canonicalIdentifier]: { allowAll: false, models: ["other-model"] }, - }, - } + expect(ProfileValidator.isProfileAllowed(profile, allowList)).toBe(true) - expect(ProfileValidator.isProfileAllowed(profile, negativeAllowList)).toBe(false) - } finally { - identifiers[identifierKey] = originalIdentifier + const negativeAllowList: OrganizationAllowList = { + allowAll: false, + providers: { + [canonicalIdentifier]: { allowAll: false, models: ["other-model"] }, + }, } + + expect(ProfileValidator.isProfileAllowed(profile, negativeAllowList)).toBe(false) }) it.each([ From 48687d6c4da84a198414b53216aadd39243f6494 Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Sun, 26 Jul 2026 20:12:54 +0300 Subject: [PATCH 4/4] chore(lint): prune profile validator suppressions --- src/eslint-suppressions.json | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index a0f00c04e3..ba02185cdb 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1689,11 +1689,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