From a4f6f53265094495f2dfe05661bb3d70786ee5dd Mon Sep 17 00:00:00 2001 From: cte Date: Mon, 14 Apr 2025 12:04:28 -0700 Subject: [PATCH 1/3] Parse providers individually --- src/core/config/ProviderSettingsManager.ts | 29 ++++++- .../__tests__/ProviderSettingsManager.test.ts | 79 ++++++++++++++++--- 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 35ee6709a0a..60751a0f493 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -1,7 +1,7 @@ import { ExtensionContext } from "vscode" import { z, ZodError } from "zod" -import { providerSettingsSchema, ApiConfigMeta } from "../../schemas" +import { providerSettingsSchema, ApiConfigMeta, ProviderSettings } from "../../schemas" import { Mode, modes } from "../../shared/modes" import { telemetryService } from "../../services/telemetry/TelemetryService" @@ -321,7 +321,32 @@ export class ProviderSettingsManager { private async load(): Promise { try { const content = await this.context.secrets.get(this.secretsKey) - return content ? providerProfilesSchema.parse(JSON.parse(content)) : this.defaultProviderProfiles + + if (!content) { + return this.defaultProviderProfiles + } + + const providerProfiles = providerProfilesSchema + .extend({ + apiConfigs: z.record(z.string(), z.any()), + }) + .parse(JSON.parse(content)) + + const apiConfigs = Object.entries(providerProfiles.apiConfigs).reduce( + (acc, [key, apiConfig]) => { + // Use .strict() to ensure only defined fields are allowed + const result = providerSettingsWithIdSchema.strict().safeParse(apiConfig) + return result.success ? { ...acc, [key]: result.data } : acc + }, + {} as Record, + ) + + return { + ...providerProfiles, + apiConfigs: Object.fromEntries( + Object.entries(apiConfigs).filter(([_, apiConfig]) => apiConfig !== null), + ), + } } catch (error) { if (error instanceof ZodError) { telemetryService.captureSchemaValidationError({ schemaName: "ProviderProfiles", error }) diff --git a/src/core/config/__tests__/ProviderSettingsManager.test.ts b/src/core/config/__tests__/ProviderSettingsManager.test.ts index b1a85075464..a61d5a274bd 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.test.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.test.ts @@ -68,11 +68,17 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { + // Provide a minimal valid structure (missing ID) default: { - config: {}, + apiProvider: "openai", // Example provider + openAiModelId: "gpt-4", // Corresponding model field + rateLimitSeconds: 0, }, + // Provide a minimal valid structure (missing ID) test: { apiProvider: "anthropic", + apiModelId: "claude-instant-1", // Corresponding model field + rateLimitSeconds: 0, }, }, }), @@ -94,19 +100,24 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { + // Valid structure, missing rateLimitSeconds default: { - config: {}, - id: "default", + apiProvider: "openai", + openAiModelId: "gpt-4", + id: "default-id", // Needs an ID for migration test structure rateLimitSeconds: undefined, }, + // Valid structure, missing rateLimitSeconds and ID (ID will be generated) test: { apiProvider: "anthropic", + apiModelId: "claude-instant-1", rateLimitSeconds: undefined, }, + // Valid structure, existing rateLimitSeconds existing: { apiProvider: "anthropic", - // this should not really be possible, unless someone has loaded a hand edited config, - // but we don't overwrite so we'll check that + apiModelId: "claude-instant-1", + id: "existing-id", // Needs an ID rateLimitSeconds: 43, }, }, @@ -422,11 +433,13 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { + // Provide a valid structure for 'test' test: { - config: { - apiProvider: "anthropic", - }, - id: "test-id", + apiProvider: "anthropic", + apiModelId: "claude-instant-1", + apiKey: "test-key", + rateLimitSeconds: 0, + id: "test-id", // Keep ID as loadConfig uses it }, }, }), @@ -437,6 +450,54 @@ describe("ProviderSettingsManager", () => { "Failed to load config: Error: Failed to write provider profiles to secrets: Error: Storage failed", ) }) + + it("should remove invalid profiles during load", async () => { + const invalidConfig = { + currentApiConfigName: "valid", + apiConfigs: { + valid: { + apiProvider: "anthropic", + apiKey: "valid-key", + // id: "valid-id", // ID is added by the manager, not part of the schema input + apiModelId: "claude-3-opus-20240229", // Correct field name for Anthropic model + rateLimitSeconds: 0, // Generic field, should be fine + }, + invalid: { + // Missing required fields like apiProvider or apiKey + id: "invalid-id", + someOtherField: "value", + }, + anotherInvalid: "not an object", // Incorrect type + }, + migrations: { + rateLimitSecondsMigrated: true, + }, + } + + mockSecrets.get.mockResolvedValue(JSON.stringify(invalidConfig)) + + // Load a valid config - this should trigger the cleanup + // Note: loadConfig itself doesn't clean, initialization does. + // We need to call initialize to trigger the load and cleanup. + await providerSettingsManager.initialize() + + // Check that the stored config was updated and invalid profiles removed + // Initialize calls store multiple times potentially (migration, id generation, cleanup) + // We need to find the call that reflects the final cleaned state. + // The last call to store should contain the cleaned config. + const storeCalls = mockSecrets.store.mock.calls + expect(storeCalls.length).toBeGreaterThan(0) // Ensure store was called at least once + const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1] + const storedConfig = JSON.parse(finalStoredConfigJson) + + expect(storedConfig.apiConfigs.valid).toBeDefined() + expect(storedConfig.apiConfigs.invalid).toBeUndefined() + expect(storedConfig.apiConfigs.anotherInvalid).toBeUndefined() + expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid"]) + // Initialize might reset currentApiConfigName if the original was invalid, + // or keep it if it was valid. Let's check it's still 'valid'. + expect(storedConfig.currentApiConfigName).toBe("valid") + }) }) describe("ResetAllConfigs", () => { From a0237b6352212045df818918d3eed49b1da8390d Mon Sep 17 00:00:00 2001 From: cte Date: Mon, 14 Apr 2025 12:06:15 -0700 Subject: [PATCH 2/3] Remove .strict() --- src/core/config/ProviderSettingsManager.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 60751a0f493..f6d115b94df 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -334,8 +334,7 @@ export class ProviderSettingsManager { const apiConfigs = Object.entries(providerProfiles.apiConfigs).reduce( (acc, [key, apiConfig]) => { - // Use .strict() to ensure only defined fields are allowed - const result = providerSettingsWithIdSchema.strict().safeParse(apiConfig) + const result = providerSettingsWithIdSchema.safeParse(apiConfig) return result.success ? { ...acc, [key]: result.data } : acc }, {} as Record, From 13c00efa9157f5129dd621eb74569b1d15b4e809 Mon Sep 17 00:00:00 2001 From: cte Date: Mon, 14 Apr 2025 12:14:37 -0700 Subject: [PATCH 3/3] Clean up tests --- src/core/config/ProviderSettingsManager.ts | 7 +-- .../__tests__/ProviderSettingsManager.test.ts | 58 ++++++------------- 2 files changed, 19 insertions(+), 46 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index f6d115b94df..212a673b953 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -115,20 +115,15 @@ export class ProviderSettingsManager { } if (rateLimitSeconds === undefined) { - // Failed to get the existing value, use the default + // Failed to get the existing value, use the default. rateLimitSeconds = 0 } for (const [name, apiConfig] of Object.entries(providerProfiles.apiConfigs)) { if (apiConfig.rateLimitSeconds === undefined) { - console.log( - `[MigrateRateLimitSeconds] Applying rate limit ${rateLimitSeconds}s to profile: ${name}`, - ) apiConfig.rateLimitSeconds = rateLimitSeconds } } - - console.log(`[MigrateRateLimitSeconds] migration complete`) } catch (error) { console.error(`[MigrateRateLimitSeconds] Failed to migrate rate limit settings:`, error) } diff --git a/src/core/config/__tests__/ProviderSettingsManager.test.ts b/src/core/config/__tests__/ProviderSettingsManager.test.ts index a61d5a274bd..91f5adbdf92 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.test.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.test.ts @@ -68,17 +68,11 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { - // Provide a minimal valid structure (missing ID) default: { - apiProvider: "openai", // Example provider - openAiModelId: "gpt-4", // Corresponding model field - rateLimitSeconds: 0, + config: {}, }, - // Provide a minimal valid structure (missing ID) test: { apiProvider: "anthropic", - apiModelId: "claude-instant-1", // Corresponding model field - rateLimitSeconds: 0, }, }, }), @@ -100,24 +94,19 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { - // Valid structure, missing rateLimitSeconds default: { - apiProvider: "openai", - openAiModelId: "gpt-4", - id: "default-id", // Needs an ID for migration test structure + config: {}, + id: "default", rateLimitSeconds: undefined, }, - // Valid structure, missing rateLimitSeconds and ID (ID will be generated) test: { apiProvider: "anthropic", - apiModelId: "claude-instant-1", rateLimitSeconds: undefined, }, - // Valid structure, existing rateLimitSeconds existing: { apiProvider: "anthropic", - apiModelId: "claude-instant-1", - id: "existing-id", // Needs an ID + // this should not really be possible, unless someone has loaded a hand edited config, + // but we don't overwrite so we'll check that rateLimitSeconds: 43, }, }, @@ -433,13 +422,11 @@ describe("ProviderSettingsManager", () => { JSON.stringify({ currentApiConfigName: "default", apiConfigs: { - // Provide a valid structure for 'test' test: { - apiProvider: "anthropic", - apiModelId: "claude-instant-1", - apiKey: "test-key", - rateLimitSeconds: 0, - id: "test-id", // Keep ID as loadConfig uses it + config: { + apiProvider: "anthropic", + }, + id: "test-id", }, }, }), @@ -458,16 +445,16 @@ describe("ProviderSettingsManager", () => { valid: { apiProvider: "anthropic", apiKey: "valid-key", - // id: "valid-id", // ID is added by the manager, not part of the schema input - apiModelId: "claude-3-opus-20240229", // Correct field name for Anthropic model - rateLimitSeconds: 0, // Generic field, should be fine + apiModelId: "claude-3-opus-20240229", + rateLimitSeconds: 0, }, invalid: { - // Missing required fields like apiProvider or apiKey - id: "invalid-id", - someOtherField: "value", + // Invalid API provider. + id: "x.ai", + apiProvider: "x.ai", }, - anotherInvalid: "not an object", // Incorrect type + // Incorrect type. + anotherInvalid: "not an object", }, migrations: { rateLimitSecondsMigrated: true, @@ -476,26 +463,17 @@ describe("ProviderSettingsManager", () => { mockSecrets.get.mockResolvedValue(JSON.stringify(invalidConfig)) - // Load a valid config - this should trigger the cleanup - // Note: loadConfig itself doesn't clean, initialization does. - // We need to call initialize to trigger the load and cleanup. await providerSettingsManager.initialize() - // Check that the stored config was updated and invalid profiles removed - // Initialize calls store multiple times potentially (migration, id generation, cleanup) - // We need to find the call that reflects the final cleaned state. - // The last call to store should contain the cleaned config. const storeCalls = mockSecrets.store.mock.calls - expect(storeCalls.length).toBeGreaterThan(0) // Ensure store was called at least once + expect(storeCalls.length).toBeGreaterThan(0) // Ensure store was called at least once. const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1] - const storedConfig = JSON.parse(finalStoredConfigJson) + const storedConfig = JSON.parse(finalStoredConfigJson) expect(storedConfig.apiConfigs.valid).toBeDefined() expect(storedConfig.apiConfigs.invalid).toBeUndefined() expect(storedConfig.apiConfigs.anotherInvalid).toBeUndefined() expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid"]) - // Initialize might reset currentApiConfigName if the original was invalid, - // or keep it if it was valid. Let's check it's still 'valid'. expect(storedConfig.currentApiConfigName).toBe("valid") }) })