[Fix] upsertProviderProfile: enforce org allow-list and roll back on failure

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.
 	 */

src/core/config/__tests__/ProviderSettingsManager.spec.ts

@@ -1352,6 +1352,47 @@ describe("ProviderSettingsManager", () => {
 		})
 	})
 
+	describe("ClearModeConfig", () => {
+		const profilesWithModes = (modeApiConfigs?: Record<string, string>) =>
+			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 = {

src/core/webview/ClineProvider.ts

@@ -1885,21 +1885,111 @@
 		providerSettings: ProviderSettings,
 		activate: boolean = true,
 	): Promise<string | undefined> {
+		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) {
 			this.log(
 				`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<Awaited<ReturnType<ProviderSettingsManager["getProfile"]>>, "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<ReturnType<ClineProvider["snapshotProviderProfileState"]>>,
+		activate: boolean,
+	): Promise<void> {
+		// 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.

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,