Sfoglia il codice sorgente

fix: sanitize removed/invalid API providers to prevent infinite loop (#9869)

Hannes Rudolph 3 settimane fa
parent
commit
9f4dcfc0e6

+ 50 - 2
src/core/config/ContextProxy.ts

@@ -15,6 +15,7 @@ import {
 	providerSettingsSchema,
 	providerSettingsSchema,
 	globalSettingsSchema,
 	globalSettingsSchema,
 	isSecretStateKey,
 	isSecretStateKey,
+	isProviderName,
 } from "@roo-code/types"
 } from "@roo-code/types"
 import { TelemetryService } from "@roo-code/telemetry"
 import { TelemetryService } from "@roo-code/telemetry"
 
 
@@ -88,9 +89,33 @@ export class ContextProxy {
 		// Migration: Check for old nested image generation settings and migrate them
 		// Migration: Check for old nested image generation settings and migrate them
 		await this.migrateImageGenerationSettings()
 		await this.migrateImageGenerationSettings()
 
 
+		// Migration: Sanitize invalid/removed API providers
+		await this.migrateInvalidApiProvider()
+
 		this._isInitialized = true
 		this._isInitialized = true
 	}
 	}
 
 
+	/**
+	 * Migrates invalid/removed apiProvider values by clearing them from storage.
+	 * This handles cases where a user had a provider selected that was later removed
+	 * from the extension (e.g., "glama").
+	 */
+	private async migrateInvalidApiProvider() {
+		try {
+			const apiProvider = this.stateCache.apiProvider
+			if (apiProvider !== undefined && !isProviderName(apiProvider)) {
+				logger.info(`[ContextProxy] Found invalid provider "${apiProvider}" in storage - clearing it`)
+				// Clear the invalid provider from both cache and storage
+				this.stateCache.apiProvider = undefined
+				await this.originalContext.globalState.update("apiProvider", undefined)
+			}
+		} catch (error) {
+			logger.error(
+				`Error during invalid API provider migration: ${error instanceof Error ? error.message : String(error)}`,
+			)
+		}
+	}
+
 	/**
 	/**
 	 * Migrates old nested openRouterImageGenerationSettings to the new flattened structure
 	 * Migrates old nested openRouterImageGenerationSettings to the new flattened structure
 	 */
 	 */
@@ -266,15 +291,38 @@ export class ContextProxy {
 	public getProviderSettings(): ProviderSettings {
 	public getProviderSettings(): ProviderSettings {
 		const values = this.getValues()
 		const values = this.getValues()
 
 
+		// Sanitize invalid/removed apiProvider values before parsing
+		// This handles cases where a user had a provider selected that was later removed
+		// from the extension (e.g., "glama"). We sanitize here to avoid repeated
+		// schema validation errors that can cause infinite loops in telemetry.
+		const sanitizedValues = this.sanitizeProviderValues(values)
+
 		try {
 		try {
-			return providerSettingsSchema.parse(values)
+			return providerSettingsSchema.parse(sanitizedValues)
 		} catch (error) {
 		} catch (error) {
 			if (error instanceof ZodError) {
 			if (error instanceof ZodError) {
 				TelemetryService.instance.captureSchemaValidationError({ schemaName: "ProviderSettings", error })
 				TelemetryService.instance.captureSchemaValidationError({ schemaName: "ProviderSettings", error })
 			}
 			}
 
 
-			return PROVIDER_SETTINGS_KEYS.reduce((acc, key) => ({ ...acc, [key]: values[key] }), {} as ProviderSettings)
+			return PROVIDER_SETTINGS_KEYS.reduce(
+				(acc, key) => ({ ...acc, [key]: sanitizedValues[key] }),
+				{} as ProviderSettings,
+			)
+		}
+	}
+
+	/**
+	 * Sanitizes provider values by resetting invalid/removed apiProvider values.
+	 * This prevents schema validation errors for removed providers.
+	 */
+	private sanitizeProviderValues(values: RooCodeSettings): RooCodeSettings {
+		if (values.apiProvider !== undefined && !isProviderName(values.apiProvider)) {
+			logger.info(`[ContextProxy] Sanitizing invalid provider "${values.apiProvider}" - resetting to undefined`)
+			// Return a new values object without the invalid apiProvider
+			const { apiProvider, ...restValues } = values
+			return restValues as RooCodeSettings
 		}
 		}
+		return values
 	}
 	}
 
 
 	public async setProviderSettings(values: ProviderSettings) {
 	public async setProviderSettings(values: ProviderSettings) {

+ 31 - 1
src/core/config/ProviderSettingsManager.ts

@@ -11,6 +11,7 @@ import {
 	DEFAULT_CONSECUTIVE_MISTAKE_LIMIT,
 	DEFAULT_CONSECUTIVE_MISTAKE_LIMIT,
 	getModelId,
 	getModelId,
 	type ProviderName,
 	type ProviderName,
+	isProviderName,
 } from "@roo-code/types"
 } from "@roo-code/types"
 import { TelemetryService } from "@roo-code/telemetry"
 import { TelemetryService } from "@roo-code/telemetry"
 
 
@@ -598,7 +599,10 @@ export class ProviderSettingsManager {
 
 
 			const apiConfigs = Object.entries(providerProfiles.apiConfigs).reduce(
 			const apiConfigs = Object.entries(providerProfiles.apiConfigs).reduce(
 				(acc, [key, apiConfig]) => {
 				(acc, [key, apiConfig]) => {
-					const result = providerSettingsWithIdSchema.safeParse(apiConfig)
+					// First, sanitize invalid apiProvider values before parsing
+					// This handles removed providers (like "glama") gracefully
+					const sanitizedConfig = this.sanitizeProviderConfig(apiConfig)
+					const result = providerSettingsWithIdSchema.safeParse(sanitizedConfig)
 					return result.success ? { ...acc, [key]: result.data } : acc
 					return result.success ? { ...acc, [key]: result.data } : acc
 				},
 				},
 				{} as Record<string, ProviderSettingsWithId>,
 				{} as Record<string, ProviderSettingsWithId>,
@@ -622,6 +626,32 @@ export class ProviderSettingsManager {
 		}
 		}
 	}
 	}
 
 
+	/**
+	 * Sanitizes a provider config by resetting invalid/removed apiProvider values.
+	 * This handles cases where a user had a provider selected that was later removed
+	 * from the extension (e.g., "glama").
+	 */
+	private sanitizeProviderConfig(apiConfig: unknown): unknown {
+		if (typeof apiConfig !== "object" || apiConfig === null) {
+			return apiConfig
+		}
+
+		const config = apiConfig as Record<string, unknown>
+
+		// Check if apiProvider is set and if it's still valid
+		if (config.apiProvider !== undefined && !isProviderName(config.apiProvider)) {
+			console.log(
+				`[ProviderSettingsManager] Sanitizing invalid provider "${config.apiProvider}" - resetting to undefined`,
+			)
+			// Return a new config object without the invalid apiProvider
+			// This effectively resets the profile so the user can select a valid provider
+			const { apiProvider, ...restConfig } = config
+			return restConfig
+		}
+
+		return apiConfig
+	}
+
 	private async store(providerProfiles: ProviderProfiles) {
 	private async store(providerProfiles: ProviderProfiles) {
 		try {
 		try {
 			await this.context.secrets.store(this.secretsKey, JSON.stringify(providerProfiles, null, 2))
 			await this.context.secrets.store(this.secretsKey, JSON.stringify(providerProfiles, null, 2))

+ 75 - 0
src/core/config/__tests__/ContextProxy.spec.ts

@@ -428,4 +428,79 @@ describe("ContextProxy", () => {
 			expect(initializeSpy).toHaveBeenCalledTimes(1)
 			expect(initializeSpy).toHaveBeenCalledTimes(1)
 		})
 		})
 	})
 	})
+
+	describe("invalid apiProvider migration", () => {
+		it("should clear invalid apiProvider from storage during initialization", async () => {
+			// Reset and create a new proxy with invalid provider in state
+			vi.clearAllMocks()
+			mockGlobalState.get.mockImplementation((key: string) => {
+				if (key === "apiProvider") {
+					return "invalid-removed-provider" // Invalid/removed provider
+				}
+				return undefined
+			})
+
+			const proxyWithInvalidProvider = new ContextProxy(mockContext)
+			await proxyWithInvalidProvider.initialize()
+
+			// Should have cleared the invalid apiProvider
+			expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", undefined)
+		})
+
+		it("should not modify valid apiProvider during initialization", async () => {
+			// Reset and create a new proxy with valid provider in state
+			vi.clearAllMocks()
+			mockGlobalState.get.mockImplementation((key: string) => {
+				if (key === "apiProvider") {
+					return "anthropic" // Valid provider
+				}
+				return undefined
+			})
+
+			const proxyWithValidProvider = new ContextProxy(mockContext)
+			await proxyWithValidProvider.initialize()
+
+			// Should NOT have called update for apiProvider (it's valid)
+			const updateCalls = mockGlobalState.update.mock.calls
+			const apiProviderUpdateCalls = updateCalls.filter((call: any[]) => call[0] === "apiProvider")
+			expect(apiProviderUpdateCalls.length).toBe(0)
+		})
+	})
+
+	describe("getProviderSettings", () => {
+		it("should sanitize invalid apiProvider before parsing", async () => {
+			// Set an invalid provider in state
+			await proxy.updateGlobalState("apiProvider", "invalid-removed-provider" as any)
+			await proxy.updateGlobalState("apiModelId", "some-model")
+
+			const settings = proxy.getProviderSettings()
+
+			// The invalid apiProvider should be sanitized (removed)
+			expect(settings.apiProvider).toBeUndefined()
+			// Other settings should still be present
+			expect(settings.apiModelId).toBe("some-model")
+		})
+
+		it("should pass through valid apiProvider", async () => {
+			// Set a valid provider in state
+			await proxy.updateGlobalState("apiProvider", "anthropic")
+			await proxy.updateGlobalState("apiModelId", "claude-3-opus-20240229")
+
+			const settings = proxy.getProviderSettings()
+
+			// Valid provider should be returned
+			expect(settings.apiProvider).toBe("anthropic")
+			expect(settings.apiModelId).toBe("claude-3-opus-20240229")
+		})
+
+		it("should handle undefined apiProvider gracefully", async () => {
+			// Ensure no provider is set
+			await proxy.updateGlobalState("apiProvider", undefined)
+
+			const settings = proxy.getProviderSettings()
+
+			// Should not throw and should return undefined
+			expect(settings.apiProvider).toBeUndefined()
+		})
+	})
 })
 })

+ 63 - 6
src/core/config/__tests__/ProviderSettingsManager.spec.ts

@@ -705,7 +705,55 @@ describe("ProviderSettingsManager", () => {
 			)
 			)
 		})
 		})
 
 
-		it("should remove invalid profiles during load", async () => {
+		it("should sanitize invalid/removed providers by resetting apiProvider to undefined", async () => {
+			// This tests the fix for the infinite loop issue when a provider is removed
+			const configWithRemovedProvider = {
+				currentApiConfigName: "valid",
+				apiConfigs: {
+					valid: {
+						apiProvider: "anthropic",
+						apiKey: "valid-key",
+						apiModelId: "claude-3-opus-20240229",
+						id: "valid-id",
+					},
+					removedProvider: {
+						// Provider that was removed from the extension (e.g., "invalid-removed-provider")
+						id: "removed-id",
+						apiProvider: "invalid-removed-provider",
+						apiKey: "some-key",
+						apiModelId: "some-model",
+					},
+				},
+				migrations: {
+					rateLimitSecondsMigrated: true,
+					diffSettingsMigrated: true,
+					openAiHeadersMigrated: true,
+					consecutiveMistakeLimitMigrated: true,
+					todoListEnabledMigrated: true,
+				},
+			}
+
+			mockSecrets.get.mockResolvedValue(JSON.stringify(configWithRemovedProvider))
+
+			await providerSettingsManager.initialize()
+
+			const storeCalls = mockSecrets.store.mock.calls
+			expect(storeCalls.length).toBeGreaterThan(0)
+			const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1]
+
+			const storedConfig = JSON.parse(finalStoredConfigJson)
+			// The valid provider should be untouched
+			expect(storedConfig.apiConfigs.valid).toBeDefined()
+			expect(storedConfig.apiConfigs.valid.apiProvider).toBe("anthropic")
+
+			// The config with the removed provider should have its apiProvider reset to undefined
+			// but still be present (not filtered out entirely)
+			expect(storedConfig.apiConfigs.removedProvider).toBeDefined()
+			expect(storedConfig.apiConfigs.removedProvider.apiProvider).toBeUndefined()
+			expect(storedConfig.apiConfigs.removedProvider.id).toBe("removed-id")
+		})
+
+		it("should sanitize invalid providers and remove non-object profiles during load", async () => {
 			const invalidConfig = {
 			const invalidConfig = {
 				currentApiConfigName: "valid",
 				currentApiConfigName: "valid",
 				apiConfigs: {
 				apiConfigs: {
@@ -715,12 +763,12 @@ describe("ProviderSettingsManager", () => {
 						apiModelId: "claude-3-opus-20240229",
 						apiModelId: "claude-3-opus-20240229",
 						rateLimitSeconds: 0,
 						rateLimitSeconds: 0,
 					},
 					},
-					invalid: {
-						// Invalid API provider.
+					invalidProvider: {
+						// Invalid API provider - should be sanitized (kept but apiProvider reset to undefined)
 						id: "x.ai",
 						id: "x.ai",
 						apiProvider: "x.ai",
 						apiProvider: "x.ai",
 					},
 					},
-					// Incorrect type.
+					// Incorrect type - should be completely removed
 					anotherInvalid: "not an object",
 					anotherInvalid: "not an object",
 				},
 				},
 				migrations: {
 				migrations: {
@@ -737,10 +785,19 @@ describe("ProviderSettingsManager", () => {
 			const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1]
 			const finalStoredConfigJson = storeCalls[storeCalls.length - 1][1]
 
 
 			const storedConfig = JSON.parse(finalStoredConfigJson)
 			const storedConfig = JSON.parse(finalStoredConfigJson)
+			// Valid config should be untouched
 			expect(storedConfig.apiConfigs.valid).toBeDefined()
 			expect(storedConfig.apiConfigs.valid).toBeDefined()
-			expect(storedConfig.apiConfigs.invalid).toBeUndefined()
+			expect(storedConfig.apiConfigs.valid.apiProvider).toBe("anthropic")
+
+			// Invalid provider config should be sanitized - kept but apiProvider reset to undefined
+			expect(storedConfig.apiConfigs.invalidProvider).toBeDefined()
+			expect(storedConfig.apiConfigs.invalidProvider.apiProvider).toBeUndefined()
+			expect(storedConfig.apiConfigs.invalidProvider.id).toBe("x.ai")
+
+			// Non-object config should be completely removed
 			expect(storedConfig.apiConfigs.anotherInvalid).toBeUndefined()
 			expect(storedConfig.apiConfigs.anotherInvalid).toBeUndefined()
-			expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid"])
+
+			expect(Object.keys(storedConfig.apiConfigs)).toEqual(["valid", "invalidProvider"])
 			expect(storedConfig.currentApiConfigName).toBe("valid")
 			expect(storedConfig.currentApiConfigName).toBe("valid")
 		})
 		})
 	})
 	})