diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 01695d7994..8d4c0b5c9d 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -560,6 +560,23 @@ export class ProviderSettingsManager { } } + /** + * Remove the API config binding of a specific mode, e.g. to undo a binding created by a failed upsert. + */ + public async clearModeConfig(mode: Mode) { + try { + return await this.lock(async () => { + const providerProfiles = await this.load() + if (providerProfiles.modeApiConfigs && mode in providerProfiles.modeApiConfigs) { + delete providerProfiles.modeApiConfigs[mode] + await this.store(providerProfiles) + } + }) + } catch (error) { + throw new Error(`Failed to clear mode config: ${error}`) + } + } + /** * Get the API config ID for a specific mode. */ diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index a1fc9ac7de..07019ea637 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -1352,6 +1352,47 @@ describe("ProviderSettingsManager", () => { }) }) + describe("ClearModeConfig", () => { + const profilesWithModes = (modeApiConfigs?: Record) => + JSON.stringify({ + currentApiConfigName: "default", + apiConfigs: { default: { id: "default" } }, + modeApiConfigs, + migrations: { rateLimitSecondsMigrated: true }, + }) + + const lastStored = () => { + const calls = mockSecrets.store.mock.calls + return JSON.parse(calls[calls.length - 1][1]) + } + + it("removes only the binding of the given mode", async () => { + mockSecrets.get.mockResolvedValue(profilesWithModes({ code: "code-id", architect: "architect-id" })) + + await providerSettingsManager.clearModeConfig("code") + + expect(lastStored().modeApiConfigs).toEqual({ architect: "architect-id" }) + }) + + it.each([ + { name: "the mode has no binding", modeApiConfigs: { architect: "architect-id" } }, + { name: "no mode map is stored", modeApiConfigs: undefined }, + ])("does not write when $name", async ({ modeApiConfigs }) => { + mockSecrets.get.mockResolvedValue(profilesWithModes(modeApiConfigs)) + mockSecrets.store.mockClear() + + await providerSettingsManager.clearModeConfig("code") + + expect(mockSecrets.store).not.toHaveBeenCalled() + }) + + it("should throw error if secrets storage fails", async () => { + mockSecrets.get.mockRejectedValue(new Error("Storage failed")) + + await expect(providerSettingsManager.clearModeConfig("code")).rejects.toThrow("Failed to clear mode config") + }) + }) + describe("HasConfig", () => { it("should return true for existing config", async () => { const existingConfig: ProviderProfiles = { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 28a42913f3..66e402c06b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1885,9 +1885,39 @@ export class ClineProvider providerSettings: ProviderSettings, activate: boolean = true, ): Promise { + let partialState = false + try { return await this.enqueueProviderProfileMutation( - (signal) => this.persistProviderProfile(name, providerSettings, activate, signal), + async (signal) => { + const { organizationAllowList, mode } = await this.getState() + + // Reject before touching disk or in-memory state. + if (!ProfileValidator.isProfileAllowed(providerSettings, organizationAllowList)) { + throw new OrganizationAllowListViolationError( + t("common:errors.violated_organization_allowlist"), + ) + } + + const snapshot = await this.snapshotProviderProfileState(name, mode) + + try { + return await this.persistProviderProfile(name, providerSettings, activate, signal) + } catch (persistError) { + // Save, activate and propagate are separate writes; put everything back so + // storage, context settings, the mode binding and the task agree again. + try { + await this.restoreProviderProfileState(name, mode, snapshot, activate) + } catch (restoreError) { + partialState = true + this.log( + `Error restoring profile state after failed upsert: ${JSON.stringify(restoreError, Object.getOwnPropertyNames(restoreError), 2)}`, + ) + } + + throw persistError + } + }, { runToCompletion: true }, ) } catch (error) { @@ -1895,11 +1925,71 @@ export class ClineProvider `Error create new api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`, ) - vscode.window.showErrorMessage(t("common:errors.create_api_config")) + vscode.window.showErrorMessage( + partialState + ? t("common:errors.update_profile_model_partial") + : error instanceof OrganizationAllowListViolationError + ? error.message + : t("common:errors.create_api_config"), + ) return undefined } } + /** Captures every value `persistProviderProfile` can change, so a failed upsert can be undone. */ + private async snapshotProviderProfileState(name: string, mode: Mode) { + let previousProfile: Omit>, "name"> | undefined + + if (await this.providerSettingsManager.hasConfig(name)) { + const { name: _name, ...stored } = await this.providerSettingsManager.getProfile({ name }) + previousProfile = stored + } + + // A delegated child can run with a handoff configuration that differs from the global + // settings, so rollback rebuilds this task from its own captured configuration. + const task = this.getCurrentTask() + + return { + previousProfile, + listApiConfigMeta: this.getGlobalState("listApiConfigMeta"), + currentApiConfigName: this.getGlobalState("currentApiConfigName"), + modeConfigId: await this.providerSettingsManager.getModeConfigId(mode), + providerSettings: this.contextProxy.getProviderSettings(), + task, + taskApiConfiguration: task?.apiConfiguration ? structuredClone(task.apiConfiguration) : undefined, + } + } + + private async restoreProviderProfileState( + name: string, + mode: Mode, + snapshot: Awaited>, + activate: boolean, + ): Promise { + // Run every step even if one fails, then report the first failure. + const results = await Promise.allSettled([ + snapshot.previousProfile + ? this.providerSettingsManager.saveConfig(name, snapshot.previousProfile) + : this.providerSettingsManager.hasConfig(name).then(async (exists) => { + if (exists) await this.providerSettingsManager.deleteConfig(name) + }), + this.updateGlobalState("listApiConfigMeta", snapshot.listApiConfigMeta), + this.updateGlobalState("currentApiConfigName", snapshot.currentApiConfigName), + // A mode with no previous binding must not stay bound to a profile that is being removed. + snapshot.modeConfigId + ? this.providerSettingsManager.setModeConfig(mode, snapshot.modeConfigId) + : this.providerSettingsManager.clearModeConfig(mode), + this.contextProxy.setProviderSettings(snapshot.providerSettings), + ]) + + if (activate && snapshot.task && snapshot.taskApiConfiguration && this.getCurrentTask() === snapshot.task) { + this.updateTaskApiHandlerIfNeeded(snapshot.taskApiConfiguration, { forceRebuild: true }) + } + + const failed = results.find((result): result is PromiseRejectedResult => result.status === "rejected") + if (failed) throw failed.reason + } + /** * Applies a model change to the latest stored profile inside the mutation queue, * so a stale webview snapshot cannot overwrite newer profile settings. diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index 3121fe1046..a036651924 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -243,6 +243,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { // Mock providerSettingsManager ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + deleteConfig: vi.fn().mockResolvedValue(undefined), saveConfig: vi.fn().mockResolvedValue("test-id"), listConfig: vi.fn().mockResolvedValue([ { @@ -253,6 +255,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }, ]), setModeConfig: vi.fn(), + clearModeConfig: vi.fn().mockResolvedValue(undefined), getModeConfigId: vi.fn().mockResolvedValue(undefined), activateProfile: vi.fn().mockResolvedValue({ name: "test-config", @@ -338,6 +341,164 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { expect((mockTask as any).apiConfiguration.modelTemperature).toBe(0.7) }) + test("rejects a profile the organization allow-list forbids without touching any state", async () => { + const realGetState = provider.getState.bind(provider) + vi.spyOn(provider, "getState").mockImplementation(async () => ({ + ...(await realGetState()), + organizationAllowList: { + allowAll: false, + providers: { + [providerIdentifiers.openrouter]: { allowAll: false, models: ["openai/gpt-4"] }, + }, + }, + })) + const setProviderSettings = vi.spyOn(provider.contextProxy, "setProviderSettings") + + const result = await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + + expect(result).toBeUndefined() + expect(provider["providerSettingsManager"].saveConfig).not.toHaveBeenCalled() + expect(provider["providerSettingsManager"].setModeConfig).not.toHaveBeenCalled() + expect(setProviderSettings).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.violated_organization_allowlist") + }) + + test("accepts a profile the organization allow-list permits", async () => { + const realGetState = provider.getState.bind(provider) + vi.spyOn(provider, "getState").mockImplementation(async () => ({ + ...(await realGetState()), + organizationAllowList: { + allowAll: false, + providers: { + [providerIdentifiers.openrouter]: { allowAll: false, models: ["openai/gpt-4"] }, + }, + }, + })) + + const result = await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4", + }) + + expect(result).toBe("test-id") + }) + + test("restores the previous profile, mode binding and settings when activation fails", async () => { + const manager = provider["providerSettingsManager"] + const previousProfile = { + id: "test-id", + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4", + } + vi.mocked(manager.hasConfig).mockResolvedValue(true) + vi.mocked(manager.getProfile).mockResolvedValue({ name: "test-config", ...previousProfile }) + vi.mocked(manager.getModeConfigId).mockResolvedValue("previous-mode-id") + const previousSettings = provider.contextProxy.getProviderSettings() + const { mode } = await provider.getState() + const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: previousSettings }) + await provider.addClineToStack(mockTask) + const setProviderSettings = vi + .spyOn(provider.contextProxy, "setProviderSettings") + .mockRejectedValueOnce(new Error("context write failed")) + + const result = await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + + expect(result).toBeUndefined() + // First the new profile is saved, then the snapshot is written back. + expect(manager.saveConfig).toHaveBeenNthCalledWith(1, "test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + expect(manager.saveConfig).toHaveBeenLastCalledWith("test-config", previousProfile) + expect(manager.setModeConfig).toHaveBeenLastCalledWith(mode, "previous-mode-id") + expect(setProviderSettings).toHaveBeenLastCalledWith(previousSettings) + // The task's API handler is rebuilt from the prior settings. + expect(mockTask.updateApiConfiguration).toHaveBeenLastCalledWith(previousSettings) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.create_api_config") + }) + + test("removes a mode binding that did not exist before when activation fails", async () => { + const manager = provider["providerSettingsManager"] + const { mode } = await provider.getState() + vi.mocked(manager.getModeConfigId).mockResolvedValue(undefined) + vi.spyOn(provider.contextProxy, "setProviderSettings").mockRejectedValueOnce(new Error("write failed")) + + await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + + expect(manager.clearModeConfig).toHaveBeenCalledWith(mode) + expect(manager.setModeConfig).not.toHaveBeenLastCalledWith(mode, undefined) + }) + + test("rebuilds a delegated task from its own configuration, not the global settings, when activation fails", async () => { + const manager = provider["providerSettingsManager"] + const childConfiguration = { + apiProvider: providerIdentifiers.anthropic, + apiModelId: "claude-handoff-model", + } + expect(provider.contextProxy.getProviderSettings()).not.toMatchObject(childConfiguration) + const childTask = new Task({ ...defaultTaskOptions, apiConfiguration: childConfiguration }) + await provider.addClineToStack(childTask) + vi.mocked(manager.hasConfig).mockResolvedValue(true) + vi.mocked(manager.getProfile).mockResolvedValue({ name: "test-config", id: "test-id" }) + vi.spyOn(provider.contextProxy, "setProviderSettings").mockRejectedValueOnce(new Error("write failed")) + + await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + + expect(childTask.updateApiConfiguration).toHaveBeenLastCalledWith(childConfiguration) + }) + + test("reports a partial state, after running every rollback step, when the rollback itself fails", async () => { + const manager = provider["providerSettingsManager"] + vi.mocked(manager.hasConfig).mockResolvedValue(true) + vi.mocked(manager.getProfile).mockResolvedValue({ name: "test-config", id: "test-id" }) + vi.mocked(manager.getModeConfigId).mockResolvedValue("previous-mode-id") + const previousSettings = provider.contextProxy.getProviderSettings() + const setProviderSettings = vi + .spyOn(provider.contextProxy, "setProviderSettings") + .mockRejectedValueOnce(new Error("activation write failed")) + // Activation writes the mode binding first, then the rollback fails to restore it. + vi.mocked(manager.setModeConfig) + .mockResolvedValueOnce(undefined) + .mockRejectedValue(new Error("restore failed")) + + const result = await provider.upsertProviderProfile("test-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "anthropic/claude", + }) + + expect(result).toBeUndefined() + // The failed mode restore did not stop the other steps. + expect(manager.saveConfig).toHaveBeenLastCalledWith("test-config", { id: "test-id" }) + expect(setProviderSettings).toHaveBeenLastCalledWith(previousSettings) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.update_profile_model_partial") + expect(vscode.window.showErrorMessage).not.toHaveBeenCalledWith("errors.create_api_config") + }) + + test("deletes a profile that did not exist before when activation fails", async () => { + const manager = provider["providerSettingsManager"] + vi.mocked(manager.hasConfig).mockResolvedValueOnce(false).mockResolvedValue(true) + vi.spyOn(provider.contextProxy, "setProviderSettings").mockRejectedValueOnce(new Error("write failed")) + + await provider.upsertProviderProfile("new-config", { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4", + }) + + expect(manager.deleteConfig).toHaveBeenCalledWith("new-config") + }) + test("calls updateApiConfiguration when provider changes", async () => { const mockTask = new Task({ ...defaultTaskOptions, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 93543435e2..12049773eb 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2684,6 +2684,8 @@ describe("ClineProvider", () => { const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as any).mock.calls[0][0] ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), listConfig: vi .fn() .mockResolvedValue([ @@ -3458,7 +3460,10 @@ describe("ClineProvider", () => { const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as any).mock.calls[0][0] ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), setModeConfig: vi.fn(), + clearModeConfig: vi.fn().mockResolvedValue(undefined), saveConfig: vi.fn().mockResolvedValue(undefined), listConfig: vi .fn() @@ -3494,6 +3499,9 @@ describe("ClineProvider", () => { test("handles buildApiHandler error in updateApiConfiguration", async () => { await provider.resolveWebviewView(mockWebviewView) + // Seed the active profile so the rollback has a known name to put back. + await provider.setValue("currentApiConfigName", "current-config") + vi.mocked(mockContext.globalState.update).mockClear() const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as any).mock.calls[0][0] // Mock buildApiHandler to throw an error @@ -3503,7 +3511,10 @@ describe("ClineProvider", () => { throw new Error("API handler error") }) ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), setModeConfig: vi.fn(), + clearModeConfig: vi.fn().mockResolvedValue(undefined), saveConfig: vi.fn().mockResolvedValue(undefined), listConfig: vi .fn() @@ -3514,6 +3525,13 @@ describe("ClineProvider", () => { // Setup Task instance with auto-mock from the top of the file const mockCline = new Task(defaultTaskOptions) // Create a new mocked instance + // The first rebuild (activation) fails; the rollback's rebuild succeeds. + mockCline.updateApiConfiguration = vi + .fn() + .mockImplementationOnce(() => { + throw new Error("API handler error") + }) + .mockImplementation(() => {}) await provider.addClineToStack(mockCline) const testApiConfig = { @@ -3534,11 +3552,14 @@ describe("ClineProvider", () => { ) expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.create_api_config") - // Verify state was still updated - expect(mockContext.globalState.update).toHaveBeenCalledWith("listApiConfigMeta", [ - { name: "test-config", id: "test-id", apiProvider: providerIdentifiers.anthropic }, - ]) - expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "test-config") + // The failed upsert is rolled back: the new profile is removed from storage and the + // previously active profile name is put back. + expect(provider.providerSettingsManager.hasConfig).toHaveBeenCalledWith("test-config") + const nameWrites = vi + .mocked(mockContext.globalState.update) + .mock.calls.filter(([key]) => key === "currentApiConfigName") + expect(nameWrites.at(0)?.[1]).toBe("test-config") + expect(nameWrites.at(-1)?.[1]).toBe("current-config") }) test("handles successful saveApiConfiguration", async () => { @@ -3546,7 +3567,10 @@ describe("ClineProvider", () => { const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as any).mock.calls[0][0] ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), setModeConfig: vi.fn(), + clearModeConfig: vi.fn().mockResolvedValue(undefined), saveConfig: vi.fn().mockResolvedValue(undefined), listConfig: vi .fn() @@ -5907,6 +5931,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) const postMessageSpy = vi.spyOn(provider, "postMessageToWebview").mockResolvedValue(undefined) ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), listConfig: vi.fn().mockResolvedValue([]), } @@ -5938,6 +5964,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { const saveConfig = vi.fn().mockResolvedValue(undefined) vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) ;(provider as any).providerSettingsManager = { + hasConfig: vi.fn().mockResolvedValue(false), + getModeConfigId: vi.fn().mockResolvedValue(undefined), listConfig: vi.fn().mockResolvedValue([ { name: "Zoo Gateway", apiProvider: providerIdentifiers.zooGateway }, { name: "Backup Zoo", apiProvider: providerIdentifiers.zooGateway },