From eb5e65b84617f154bd7133872ad177d557e1c239 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Mon, 21 Sep 2026 06:53:08 +0900 Subject: [PATCH 01/17] fix(core): purge deleted provider profiles from ProviderSettingsManager --- src/core/webview/ClineProvider.ts | 2 + .../webview/__tests__/ClineProvider.spec.ts | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 86ce5d8e67..8aee878c30 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1959,6 +1959,8 @@ export class ClineProvider const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + await this.providerSettingsManager.deleteConfig(profileToDelete.name) + await this.contextProxy.setValues({ ...globalSettings, currentApiConfigName: profileToActivate, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 97c4dd877e..1e60dd886f 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2939,6 +2939,64 @@ describe("ClineProvider", () => { ]) }) }) + + describe("deleteProviderProfile", () => { + const currentProfile: ProviderSettingsEntry = { + name: "current-config", + id: "current-id", + apiProvider: providerIdentifiers.anthropic, + } + const otherProfile: ProviderSettingsEntry = { + name: "other-config", + id: "other-id", + apiProvider: providerIdentifiers.openrouter, + } + + const mockProviderSettingsManager = () => { + const profiles: ProviderSettingsEntry[] = [currentProfile, otherProfile] + + Object.assign(provider, { + providerSettingsManager: { + deleteConfig: vi.fn().mockImplementation(async (name: string) => { + const index = profiles.findIndex((profile) => profile.name === name) + if (index !== -1) { + profiles.splice(index, 1) + } + }), + listConfig: vi.fn().mockImplementation(async () => profiles), + }, + }) + + return profiles + } + + beforeEach(async () => { + await provider.contextProxy.setValue("currentApiConfigName", "current-config") + await provider.contextProxy.setValue("listApiConfigMeta", [currentProfile, otherProfile]) + }) + + test("purges the deleted profile from ProviderSettingsManager", async () => { + mockProviderSettingsManager() + + await provider.deleteProviderProfile(otherProfile) + + expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("other-config") + expect(await provider.providerSettingsManager.listConfig()).toEqual([currentProfile]) + expect(provider.getProviderProfileEntries()).toEqual([currentProfile]) + expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "current-config") + }) + + test("switches the active profile when deleting it and purges it from ProviderSettingsManager", async () => { + mockProviderSettingsManager() + + await provider.deleteProviderProfile(currentProfile) + + expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("current-config") + expect(await provider.providerSettingsManager.listConfig()).toEqual([otherProfile]) + expect(provider.getProviderProfileEntries()).toEqual([otherProfile]) + expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "other-config") + }) + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { From 3a0747d05d41e54be660180eb27214e00093a2c9 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sun, 27 Sep 2026 22:28:01 +0900 Subject: [PATCH 02/17] test(core): cover fail-fast ordering when profile config purge rejects What: add a negative spec to the deleteProviderProfile suite that makes the mocked providerSettingsManager.deleteConfig reject with the manager's not-found error and asserts the error propagates out of deleteProviderProfile while no ContextProxy write occurs (contextProxy.setValues never called, globalState.update call count unchanged). Why: the PR body claims the manager's not-found/last-config guards fail fast before any ContextProxy state is written, but no test pinned that ordering, so a future refactor that moved the purge after the ContextProxy write or swallowed the manager error would pass the whole suite. Verified the new spec fails under an ordering mutation (setValues before deleteConfig) and passes at head. --- .../webview/__tests__/ClineProvider.spec.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 1e60dd886f..94e2f4119b 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2996,6 +2996,26 @@ describe("ClineProvider", () => { expect(provider.getProviderProfileEntries()).toEqual([otherProfile]) expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "other-config") }) + + test("fails fast without writing ContextProxy state when the config purge rejects", async () => { + Object.assign(provider, { + providerSettingsManager: { + deleteConfig: vi.fn().mockRejectedValue(new Error("Config 'other-config' not found")), + listConfig: vi.fn().mockResolvedValue([currentProfile, otherProfile]), + }, + }) + + const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") + const updateCallsBefore = vi.mocked(mockContext.globalState.update).mock.calls.length + + await expect(provider.deleteProviderProfile(otherProfile)).rejects.toThrow( + "Config 'other-config' not found", + ) + + expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("other-config") + expect(setValuesSpy).not.toHaveBeenCalled() + expect(vi.mocked(mockContext.globalState.update).mock.calls.length).toBe(updateCallsBefore) + }) }) }) From 4b54f4fb2274b057614cdb7f7dc5b0f847461404 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sun, 27 Sep 2026 23:06:18 +0900 Subject: [PATCH 03/17] fix(core): activate replacement via standard path when deleting active profile What: deleting the active provider profile now routes the replacement through the existing activateProviderProfile path (the same routine the webview uses when a user switches profiles) instead of only rewriting the currentApiConfigName and listApiConfigMeta pointers in ContextProxy. Why: the pointer-only update left the effective provider settings, the mode config, and current-task wiring stale, so the session kept running on the deleted profile's configuration. Activation through the standard path applies the replacement's provider settings, persists the mode config, and re-syncs task wiring, ending in the same state as a user activation. Impact: the secrets purge (providerSettingsManager.deleteConfig) still runs and fails fast before any ContextProxy write; deleting a non-active profile is unchanged. Active-deletion specs now assert the effective provider settings and mode-config persistence, not just the name pointers. --- src/core/webview/ClineProvider.ts | 19 +++++++++++----- .../webview/__tests__/ClineProvider.spec.ts | 22 +++++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 8aee878c30..418a71b697 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1947,11 +1947,10 @@ export class ClineProvider async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() - let profileToActivate: string | undefined = globalSettings.currentApiConfigName - - if (profileToDelete.name === profileToActivate) { - profileToActivate = this.getProviderProfileEntries().find(({ name }) => name !== profileToDelete.name)?.name - } + const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName + const profileToActivate = deletedActiveProfile + ? this.getProviderProfileEntries().find(({ name }) => name !== profileToDelete.name)?.name + : globalSettings.currentApiConfigName if (!profileToActivate) { throw new Error("You cannot delete the last profile") @@ -1959,8 +1958,18 @@ export class ClineProvider const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + // Purge first so a failing purge fails fast before any ContextProxy write, + // matching the ordering the webview delete path established. await this.providerSettingsManager.deleteConfig(profileToDelete.name) + if (deletedActiveProfile) { + // Route through the standard activation path so the effective provider + // settings, mode config, and current-task wiring all end up identical to + // a user activating the replacement profile from the webview. + await this.activateProviderProfile({ name: profileToActivate }) + return + } + await this.contextProxy.setValues({ ...globalSettings, currentApiConfigName: profileToActivate, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 94e2f4119b..84c21d17f8 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2964,6 +2964,16 @@ describe("ClineProvider", () => { } }), listConfig: vi.fn().mockImplementation(async () => profiles), + activateProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { + const profile = profiles.find((entry) => entry.name === name) + if (!profile) { + throw new Error(`Config '${name}' not found`) + } + // Include a distinguishing setting so tests can assert the + // effective provider settings, not just the name pointers. + return { ...profile, openRouterModelId: "other-model" } + }), + setModeConfig: vi.fn(), }, }) @@ -2984,6 +2994,9 @@ describe("ClineProvider", () => { expect(await provider.providerSettingsManager.listConfig()).toEqual([currentProfile]) expect(provider.getProviderProfileEntries()).toEqual([currentProfile]) expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "current-config") + // Deleting a non-active profile must not trigger activation side effects. + expect(provider.providerSettingsManager.activateProfile).not.toHaveBeenCalled() + expect(provider.providerSettingsManager.setModeConfig).not.toHaveBeenCalled() }) test("switches the active profile when deleting it and purges it from ProviderSettingsManager", async () => { @@ -2992,9 +3005,18 @@ describe("ClineProvider", () => { await provider.deleteProviderProfile(currentProfile) expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("current-config") + // The replacement is activated through the standard activation path. + expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledWith({ name: "other-config" }) expect(await provider.providerSettingsManager.listConfig()).toEqual([otherProfile]) expect(provider.getProviderProfileEntries()).toEqual([otherProfile]) expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "other-config") + // The effective provider settings match the replacement, not just the name pointers. + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }) + // Same side effects as the webview activation path: the mode config is persisted. + expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalledWith("code", "other-id") }) test("fails fast without writing ContextProxy state when the config purge rejects", async () => { From 4a370fdd4d8e4e60cf15d53e6514ba738ef6678c Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 09:09:59 +0900 Subject: [PATCH 04/17] chore: retrigger automated review after thread response From 9cc37bd1601d11385cb6f45b9587f9a96bf237c6 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 09:51:52 +0900 Subject: [PATCH 05/17] fix(core): make active-profile deletion recoverable Pick the replacement profile from ProviderSettingsManager.listConfig() instead of ContextProxy metadata, so the activated name is guaranteed to exist in the manager. Capture the deleted profile before purging and restore it when the replacement activation rejects or times out, keeping the manager and ContextProxy naming the same profile so a retry no longer fails with "Config not found". Adds failure-path tests for activation rejection and the mutation timeout. --- src/core/webview/ClineProvider.ts | 51 +++++++++-- .../webview/__tests__/ClineProvider.spec.ts | 86 ++++++++++++++++++- 2 files changed, 129 insertions(+), 8 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 418a71b697..2ae46e230c 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -17,6 +17,7 @@ import { type GlobalState, type ProviderName, type ProviderSettings, + type ProviderSettingsWithId, type RooCodeSettings, type ProviderSettingsEntry, type StaticAppProperties, @@ -1948,8 +1949,10 @@ export class ClineProvider async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName + // Pick the replacement from the manager, not from ContextProxy metadata: the chosen name + // must exist in the manager or the activation below rejects with "not found". const profileToActivate = deletedActiveProfile - ? this.getProviderProfileEntries().find(({ name }) => name !== profileToDelete.name)?.name + ? (await this.providerSettingsManager.listConfig()).find(({ name }) => name !== profileToDelete.name)?.name : globalSettings.currentApiConfigName if (!profileToActivate) { @@ -1958,15 +1961,28 @@ export class ClineProvider const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + // Capture the profile before purging so a failed active-profile switch can restore it. + const deletedProfileConfig = deletedActiveProfile + ? await this.providerSettingsManager.getProfile({ name: profileToDelete.name }) + : undefined + // Purge first so a failing purge fails fast before any ContextProxy write, // matching the ordering the webview delete path established. await this.providerSettingsManager.deleteConfig(profileToDelete.name) if (deletedActiveProfile) { - // Route through the standard activation path so the effective provider - // settings, mode config, and current-task wiring all end up identical to - // a user activating the replacement profile from the webview. - await this.activateProviderProfile({ name: profileToActivate }) + try { + // Route through the standard activation path so the effective provider + // settings, mode config, and current-task wiring all end up identical to + // a user activating the replacement profile from the webview. + await this.activateProviderProfile({ name: profileToActivate }) + } catch (error) { + // Activation leaves ContextProxy naming the deleted profile, so the purge has to + // be undone: otherwise the manager no longer has the active profile and a retry + // fails in `deleteConfig` with "not found". + await this.restoreDeletedProviderProfile(profileToDelete.name, deletedProfileConfig) + throw error + } return } @@ -1979,6 +1995,31 @@ export class ClineProvider await this.postStateToWebview() } + /** + * Undo the purge of an active profile whose replacement activation failed, so the manager and + * ContextProxy keep naming the same profile. Best-effort: a failed restore is logged rather + * than replacing the original activation error. + */ + private async restoreDeletedProviderProfile( + name: string, + config: (ProviderSettingsWithId & { name: string }) | undefined, + ): Promise { + if (!config) return + + try { + const { name: _name, ...providerSettings } = config + await this.providerSettingsManager.saveConfig(name, providerSettings) + await this.providerSettingsManager.activateProfile({ name }) + this.log(`Restored provider profile '${name}' after a failed replacement activation`) + } catch (error) { + this.log( + `Failed to restore provider profile '${name}' after a failed replacement activation: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + private async persistStickyProviderProfileToCurrentTask( apiConfigName: string, options: { skipCurrentTaskRebuild?: boolean } = {}, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 84c21d17f8..770c66e680 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2955,6 +2955,11 @@ describe("ClineProvider", () => { const mockProviderSettingsManager = () => { const profiles: ProviderSettingsEntry[] = [currentProfile, otherProfile] + // Full configs keyed by name, so `getProfile`/`saveConfig` behave like the real manager. + const configs = new Map( + profiles.map((profile) => [profile.name, { ...profile, openRouterModelId: "other-model" }]), + ) + Object.assign(provider, { providerSettingsManager: { deleteConfig: vi.fn().mockImplementation(async (name: string) => { @@ -2962,16 +2967,31 @@ describe("ClineProvider", () => { if (index !== -1) { profiles.splice(index, 1) } + configs.delete(name) }), listConfig: vi.fn().mockImplementation(async () => profiles), + getProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { + const config = configs.get(name) + if (!config) { + throw new Error(`Config with name '${name}' not found`) + } + return { ...config, name } + }), + saveConfig: vi.fn().mockImplementation(async (name: string, config: ProviderSettingsEntry) => { + configs.set(name, { ...config, name }) + if (!profiles.some((profile) => profile.name === name)) { + profiles.push({ name, id: config.id, apiProvider: config.apiProvider }) + } + return config.id + }), activateProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { - const profile = profiles.find((entry) => entry.name === name) - if (!profile) { + const config = configs.get(name) + if (!config) { throw new Error(`Config '${name}' not found`) } // Include a distinguishing setting so tests can assert the // effective provider settings, not just the name pointers. - return { ...profile, openRouterModelId: "other-model" } + return { ...config, name } }), setModeConfig: vi.fn(), }, @@ -3038,6 +3058,66 @@ describe("ClineProvider", () => { expect(setValuesSpy).not.toHaveBeenCalled() expect(vi.mocked(mockContext.globalState.update).mock.calls.length).toBe(updateCallsBefore) }) + + test("restores the deleted profile when replacement activation rejects", async () => { + const profiles = mockProviderSettingsManager() + // Reject only the replacement activation; the restore's own activation still succeeds. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to activate profile: storage error", + ) + + // The purge is undone so the manager and ContextProxy still name the same profile. + expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( + "current-config", + expect.objectContaining({ id: "current-id", apiProvider: providerIdentifiers.anthropic }), + ) + expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledWith({ name: "current-config" }) + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + // A retry is now possible: the profile exists again, so the purge no longer throws "not found". + await expect(provider.deleteProviderProfile(currentProfile)).resolves.toBeUndefined() + }) + + test("restores the deleted profile when the mutation timeout aborts the activation", async () => { + vi.useFakeTimers() + + try { + const profiles = mockProviderSettingsManager() + let resolveActivation!: () => void + const pendingActivation = new Promise((resolve) => { + resolveActivation = resolve + }) + // Hang only the replacement activation; the restore's own activation still succeeds. + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + await pendingActivation + // The abandoned activation still stores the replacement in the manager. + return { ...otherProfile, openRouterModelId: "other-model" } + }) + + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + await deletionResult + + // The timeout path restores the purged profile instead of leaving ContextProxy + // naming a profile the manager no longer has. + expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( + "current-config", + expect.objectContaining({ id: "current-id", apiProvider: providerIdentifiers.anthropic }), + ) + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + + // A late completion of the abandoned activation must not resurrect a second state. + resolveActivation() + await vi.advanceTimersByTimeAsync(0) + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + } finally { + vi.useRealTimers() + } + }) }) }) From c30971c2c6010b2bfa0d85debe43cacfe5728534 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 09:56:18 +0900 Subject: [PATCH 06/17] test(core): cover task rewiring when the active profile is deleted Assert that deleting the active provider profile reaches the live task's updateApiConfiguration with the replacement's effective settings, so a change that skipped the rebuild only on the deletion path would fail. --- .../webview/__tests__/ClineProvider.spec.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 770c66e680..27299eb479 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3039,6 +3039,31 @@ describe("ClineProvider", () => { expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalledWith("code", "other-id") }) + test("rewires the live task to the replacement settings when deleting the active profile", async () => { + mockProviderSettingsManager() + + const mockTask = new Task({ + ...defaultTaskOptions, + apiConfiguration: { + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }, + }) + mockTask.updateApiConfiguration = vi.fn() + await provider.addClineToStack(mockTask) + + await provider.deleteProviderProfile(currentProfile) + + // The deletion path must not skip the task rebuild: the running task receives the + // replacement's effective settings, not the deleted profile's. + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( + expect.objectContaining({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }), + ) + }) + test("fails fast without writing ContextProxy state when the config purge rejects", async () => { Object.assign(provider, { providerSettingsManager: { From 4315fdcf420573e6999756a1e9850cb19d3fc006 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 10:27:32 +0900 Subject: [PATCH 07/17] fix(core): serialize active-profile deletion and bound its rollback Run the purge and replacement activation as one queued profile mutation so a queued upsert cannot resurrect the deleted profile, and call the unlocked activation to avoid nesting the queue. Roll back both the manager and ContextProxy to their pre-delete values when the replacement activation fails, bound that rollback with an independent timeout, and reconcile currentApiConfigName to an existing profile when the restore itself fails. Extends the tests with credential and manager-current-profile assertions. --- src/core/webview/ClineProvider.ts | 128 ++++++++++++------ .../webview/__tests__/ClineProvider.spec.ts | 46 +++++-- 2 files changed, 123 insertions(+), 51 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2ae46e230c..5fe737e520 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1949,50 +1949,69 @@ export class ClineProvider async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { const globalSettings = this.contextProxy.getValues() const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName - // Pick the replacement from the manager, not from ContextProxy metadata: the chosen name - // must exist in the manager or the activation below rejects with "not found". - const profileToActivate = deletedActiveProfile - ? (await this.providerSettingsManager.listConfig()).find(({ name }) => name !== profileToDelete.name)?.name - : globalSettings.currentApiConfigName - if (!profileToActivate) { - throw new Error("You cannot delete the last profile") + if (!deletedActiveProfile) { + // Purge first so a failing purge fails fast before any ContextProxy write, + // matching the ordering the webview delete path established. + await this.providerSettingsManager.deleteConfig(profileToDelete.name) + + await this.contextProxy.setValues({ + ...globalSettings, + listApiConfigMeta: this.getProviderProfileEntries().filter( + ({ name }) => name !== profileToDelete.name, + ), + }) + + await this.postStateToWebview() + return } - const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + // Captured inside the queued mutation so the rollback below can restore them even when the + // mutation times out (the timeout rejects the outer wrapper, so an inner catch never runs). + let deletedProfileConfig: (ProviderSettingsWithId & { name: string }) | undefined + const previousContextValues: Pick = { + currentApiConfigName: globalSettings.currentApiConfigName, + listApiConfigMeta: globalSettings.listApiConfigMeta, + } + + // The purge and the replacement switch run as one queued mutation: otherwise a queued + // `upsertProviderProfile` for the same name can save the profile back after the purge and + // make it reappear in `listApiConfigMeta` after this call has already resolved. + try { + await this.enqueueProviderProfileMutation(async (signal) => { + // Pick the replacement from the manager, not from ContextProxy metadata: the chosen + // name must exist in the manager or the activation below rejects with "not found". + const profileToActivate = (await this.providerSettingsManager.listConfig()).find( + ({ name }) => name !== profileToDelete.name, + )?.name + + if (!profileToActivate) { + throw new Error("You cannot delete the last profile") + } - // Capture the profile before purging so a failed active-profile switch can restore it. - const deletedProfileConfig = deletedActiveProfile - ? await this.providerSettingsManager.getProfile({ name: profileToDelete.name }) - : undefined + // Capture the profile before purging so a failed active-profile switch can restore it. + deletedProfileConfig = await this.providerSettingsManager.getProfile({ + name: profileToDelete.name, + }) - // Purge first so a failing purge fails fast before any ContextProxy write, - // matching the ordering the webview delete path established. - await this.providerSettingsManager.deleteConfig(profileToDelete.name) + await this.providerSettingsManager.deleteConfig(profileToDelete.name) - if (deletedActiveProfile) { - try { // Route through the standard activation path so the effective provider // settings, mode config, and current-task wiring all end up identical to - // a user activating the replacement profile from the webview. - await this.activateProviderProfile({ name: profileToActivate }) - } catch (error) { - // Activation leaves ContextProxy naming the deleted profile, so the purge has to - // be undone: otherwise the manager no longer has the active profile and a retry - // fails in `deleteConfig` with "not found". - await this.restoreDeletedProviderProfile(profileToDelete.name, deletedProfileConfig) - throw error - } - return + // a user activating the replacement profile from the webview. The unlocked + // variant is used because this already runs inside the mutation queue. + await this.activateProviderProfileUnlocked({ name: profileToActivate }, undefined, signal) + }) + } catch (error) { + // Either the manager or ContextProxy can be left naming the deleted profile, so both + // are rolled back to their pre-delete state. + await this.restoreDeletedProviderProfile( + profileToDelete.name, + deletedProfileConfig, + previousContextValues, + ) + throw error } - - await this.contextProxy.setValues({ - ...globalSettings, - currentApiConfigName: profileToActivate, - listApiConfigMeta: entries, - }) - - await this.postStateToWebview() } /** @@ -2003,13 +2022,23 @@ export class ClineProvider private async restoreDeletedProviderProfile( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, + previousContextValues: Pick, ): Promise { - if (!config) return - + // Bound the rollback independently: the failed activation may still be holding the + // manager lock, and the caller must not wait on it indefinitely. try { - const { name: _name, ...providerSettings } = config - await this.providerSettingsManager.saveConfig(name, providerSettings) - await this.providerSettingsManager.activateProfile({ name }) + await this.withProviderProfileMutationTimeout( + (async () => { + if (config) { + const { name: _name, ...providerSettings } = config + await this.providerSettingsManager.saveConfig(name, providerSettings) + await this.providerSettingsManager.activateProfile({ name }) + } + + await this.contextProxy.setValues(previousContextValues) + })(), + () => this.log(`Provider profile restore for '${name}' timed out`), + ) this.log(`Restored provider profile '${name}' after a failed replacement activation`) } catch (error) { this.log( @@ -2017,6 +2046,25 @@ export class ClineProvider error instanceof Error ? error.message : String(error) }`, ) + + // The restore failed, so ContextProxy may still name a profile the manager no longer + // has. Point it at a profile that does exist so a retry can proceed. + try { + const fallback = (await this.providerSettingsManager.listConfig())[0]?.name + + if (fallback) { + await this.contextProxy.setValues({ + currentApiConfigName: fallback, + listApiConfigMeta: await this.providerSettingsManager.listConfig(), + }) + } + } catch (reconcileError) { + this.log( + `Failed to reconcile the active profile after a failed restore: ${ + reconcileError instanceof Error ? reconcileError.message : String(reconcileError) + }`, + ) + } } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 27299eb479..1b6f1fc34a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -426,6 +426,8 @@ describe("ClineProvider", () => { setTaskNumber: vi.fn(), setParentTask: vi.fn(), setRootTask: vi.fn(), + // Mirrors Task's sticky-profile setter so tests can assert the persisted name. + setTaskApiConfigName: vi.fn(), taskId: options?.historyItem?.id || "test-task-id", emit: vi.fn(), } @@ -2956,10 +2958,18 @@ describe("ClineProvider", () => { const profiles: ProviderSettingsEntry[] = [currentProfile, otherProfile] // Full configs keyed by name, so `getProfile`/`saveConfig` behave like the real manager. - const configs = new Map( - profiles.map((profile) => [profile.name, { ...profile, openRouterModelId: "other-model" }]), + // Each carries a distinct `apiKey` so a restore that drops credentials fails the test. + const configs = new Map( + [ + [currentProfile.name, { ...currentProfile, apiKey: "current-key" }], + [otherProfile.name, { ...otherProfile, openRouterModelId: "other-model", apiKey: "other-key" }], + ], ) + // Mirrors `ProviderSettingsManager.currentApiConfigName` so tests can assert the + // manager's own notion of the active profile, not just ContextProxy's. + const managerState = { currentApiConfigName: currentProfile.name } + Object.assign(provider, { providerSettingsManager: { deleteConfig: vi.fn().mockImplementation(async (name: string) => { @@ -2989,6 +2999,7 @@ describe("ClineProvider", () => { if (!config) { throw new Error(`Config '${name}' not found`) } + managerState.currentApiConfigName = name // Include a distinguishing setting so tests can assert the // effective provider settings, not just the name pointers. return { ...config, name } @@ -2997,7 +3008,7 @@ describe("ClineProvider", () => { }, }) - return profiles + return { profiles, managerState } } beforeEach(async () => { @@ -3062,6 +3073,8 @@ describe("ClineProvider", () => { openRouterModelId: "other-model", }), ) + // The sticky profile name is persisted too, so removing that update fails the test. + expect(mockTask.setTaskApiConfigName).toHaveBeenCalledWith("other-config") }) test("fails fast without writing ContextProxy state when the config purge rejects", async () => { @@ -3085,7 +3098,7 @@ describe("ClineProvider", () => { }) test("restores the deleted profile when replacement activation rejects", async () => { - const profiles = mockProviderSettingsManager() + const { profiles, managerState } = mockProviderSettingsManager() // Reject only the replacement activation; the restore's own activation still succeeds. vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( new Error("Failed to activate profile: storage error"), @@ -3095,12 +3108,19 @@ describe("ClineProvider", () => { "Failed to activate profile: storage error", ) - // The purge is undone so the manager and ContextProxy still name the same profile. + // The purge is undone so the manager and ContextProxy still name the same profile, + // including the credential: a restore that dropped `apiKey` fails here. expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( "current-config", - expect.objectContaining({ id: "current-id", apiProvider: providerIdentifiers.anthropic }), + expect.objectContaining({ + id: "current-id", + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }), ) expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledWith({ name: "current-config" }) + expect(managerState.currentApiConfigName).toBe("current-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) // A retry is now possible: the profile exists again, so the purge no longer throws "not found". await expect(provider.deleteProviderProfile(currentProfile)).resolves.toBeUndefined() @@ -3110,7 +3130,7 @@ describe("ClineProvider", () => { vi.useFakeTimers() try { - const profiles = mockProviderSettingsManager() + const { profiles } = mockProviderSettingsManager() let resolveActivation!: () => void const pendingActivation = new Promise((resolve) => { resolveActivation = resolve @@ -3118,7 +3138,6 @@ describe("ClineProvider", () => { // Hang only the replacement activation; the restore's own activation still succeeds. vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { await pendingActivation - // The abandoned activation still stores the replacement in the manager. return { ...otherProfile, openRouterModelId: "other-model" } }) @@ -3128,14 +3147,19 @@ describe("ClineProvider", () => { await deletionResult // The timeout path restores the purged profile instead of leaving ContextProxy - // naming a profile the manager no longer has. + // naming a profile the manager no longer has. The credential must survive too. expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( "current-config", - expect.objectContaining({ id: "current-id", apiProvider: providerIdentifiers.anthropic }), + expect.objectContaining({ + id: "current-id", + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }), ) expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) - // A late completion of the abandoned activation must not resurrect a second state. + // A late completion of the abandoned activation must not move ContextProxy: the + // aborted signal makes `activateProviderProfileUnlocked` return before writing. resolveActivation() await vi.advanceTimersByTimeAsync(0) expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") From fc5c21e243be43c5ac2031c31959d7db71c11d75 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 10:30:24 +0900 Subject: [PATCH 08/17] style(core): apply prettier formatting to profile deletion changes The `compile` check runs `prettier --check` before lint and type checks, and both touched files failed it. --- src/core/webview/ClineProvider.ts | 10 ++-------- src/core/webview/__tests__/ClineProvider.spec.ts | 10 ++++------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 5fe737e520..753616058b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1957,9 +1957,7 @@ export class ClineProvider await this.contextProxy.setValues({ ...globalSettings, - listApiConfigMeta: this.getProviderProfileEntries().filter( - ({ name }) => name !== profileToDelete.name, - ), + listApiConfigMeta: this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name), }) await this.postStateToWebview() @@ -2005,11 +2003,7 @@ export class ClineProvider } catch (error) { // Either the manager or ContextProxy can be left naming the deleted profile, so both // are rolled back to their pre-delete state. - await this.restoreDeletedProviderProfile( - profileToDelete.name, - deletedProfileConfig, - previousContextValues, - ) + await this.restoreDeletedProviderProfile(profileToDelete.name, deletedProfileConfig, previousContextValues) throw error } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 1b6f1fc34a..d779f5b89e 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2959,12 +2959,10 @@ describe("ClineProvider", () => { // Full configs keyed by name, so `getProfile`/`saveConfig` behave like the real manager. // Each carries a distinct `apiKey` so a restore that drops credentials fails the test. - const configs = new Map( - [ - [currentProfile.name, { ...currentProfile, apiKey: "current-key" }], - [otherProfile.name, { ...otherProfile, openRouterModelId: "other-model", apiKey: "other-key" }], - ], - ) + const configs = new Map([ + [currentProfile.name, { ...currentProfile, apiKey: "current-key" }], + [otherProfile.name, { ...otherProfile, openRouterModelId: "other-model", apiKey: "other-key" }], + ]) // Mirrors `ProviderSettingsManager.currentApiConfigName` so tests can assert the // manager's own notion of the active profile, not just ContextProxy's. From 70ba377f512f50adebc19e0a4bd1ea039926dc3f Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 11:01:27 +0900 Subject: [PATCH 09/17] fix(core): read deletion state inside the queue and prefer configured profiles Read the active-profile snapshot inside the queued mutation so a queued activation cannot make the deletion branch on stale state, and run the inactive-profile purge inside the same queue so a queued upsert cannot re-add the profile after the delete resolves. Skip profiles without an `apiProvider` when choosing the replacement, matching the mode-switch path, and abort the queued purge when the caller already timed out. Roll back provider settings as well as the name pointers, and apply the fallback profile's settings when the restore itself fails. Adds a history-write assertion to the active-deletion test. --- src/core/webview/ClineProvider.ts | 82 +++++++++++++------ .../webview/__tests__/ClineProvider.spec.ts | 36 +++++++- 2 files changed, 88 insertions(+), 30 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 753616058b..d9319891ae 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1947,40 +1947,54 @@ export class ClineProvider } async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { - const globalSettings = this.contextProxy.getValues() - const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName - - if (!deletedActiveProfile) { - // Purge first so a failing purge fails fast before any ContextProxy write, - // matching the ordering the webview delete path established. - await this.providerSettingsManager.deleteConfig(profileToDelete.name) - - await this.contextProxy.setValues({ - ...globalSettings, - listApiConfigMeta: this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name), - }) - - await this.postStateToWebview() - return - } - // Captured inside the queued mutation so the rollback below can restore them even when the // mutation times out (the timeout rejects the outer wrapper, so an inner catch never runs). let deletedProfileConfig: (ProviderSettingsWithId & { name: string }) | undefined - const previousContextValues: Pick = { - currentApiConfigName: globalSettings.currentApiConfigName, - listApiConfigMeta: globalSettings.listApiConfigMeta, - } + let previousContextValues: Pick = + {} - // The purge and the replacement switch run as one queued mutation: otherwise a queued + // The purge and any replacement switch run as one queued mutation: otherwise a queued // `upsertProviderProfile` for the same name can save the profile back after the purge and // make it reappear in `listApiConfigMeta` after this call has already resolved. try { await this.enqueueProviderProfileMutation(async (signal) => { + // Read the settings inside the queue: a queued activation may have changed the + // active profile since this call was made. + const globalSettings = this.contextProxy.getValues() + const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName + + previousContextValues = { + currentApiConfigName: globalSettings.currentApiConfigName, + listApiConfigMeta: globalSettings.listApiConfigMeta, + apiProvider: globalSettings.apiProvider, + } + + if (!deletedActiveProfile) { + // Purge first so a failing purge fails fast before any ContextProxy write, + // matching the ordering the webview delete path established. + await this.providerSettingsManager.deleteConfig(profileToDelete.name) + + await this.contextProxy.setValues({ + ...globalSettings, + listApiConfigMeta: this.getProviderProfileEntries().filter( + ({ name }) => name !== profileToDelete.name, + ), + }) + + await this.postStateToWebview() + return + } + // Pick the replacement from the manager, not from ContextProxy metadata: the chosen // name must exist in the manager or the activation below rejects with "not found". - const profileToActivate = (await this.providerSettingsManager.listConfig()).find( + const remainingProfiles = (await this.providerSettingsManager.listConfig()).filter( ({ name }) => name !== profileToDelete.name, + ) + + // Prefer a configured profile, mirroring the mode-switch path which skips entries + // with no `apiProvider`. Fall back to any remaining entry so deletion still works. + const profileToActivate = ( + remainingProfiles.find(({ apiProvider }) => !!apiProvider) ?? remainingProfiles[0] )?.name if (!profileToActivate) { @@ -1992,6 +2006,12 @@ export class ClineProvider name: profileToDelete.name, }) + // A timeout may have already fired while the manager lock was held. Purging now + // would delete the profile after the caller was told the deletion failed. + if (signal.aborted) { + return + } + await this.providerSettingsManager.deleteConfig(profileToDelete.name) // Route through the standard activation path so the effective provider @@ -2016,7 +2036,7 @@ export class ClineProvider private async restoreDeletedProviderProfile( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: Pick, ): Promise { // Bound the rollback independently: the failed activation may still be holding the // manager lock, and the caller must not wait on it indefinitely. @@ -2029,6 +2049,8 @@ export class ClineProvider await this.providerSettingsManager.activateProfile({ name }) } + // Restore the settings too, not just the name pointers: a partial activation + // may have left the replacement's provider settings under the deleted name. await this.contextProxy.setValues(previousContextValues) })(), () => this.log(`Provider profile restore for '${name}' timed out`), @@ -2042,14 +2064,20 @@ export class ClineProvider ) // The restore failed, so ContextProxy may still name a profile the manager no longer - // has. Point it at a profile that does exist so a retry can proceed. + // has. Point it at a profile that does exist, including that profile's settings, so a + // retry can proceed and the webview cannot show a name with the wrong settings. try { - const fallback = (await this.providerSettingsManager.listConfig())[0]?.name + const remainingProfiles = await this.providerSettingsManager.listConfig() + const fallback = remainingProfiles[0]?.name if (fallback) { + const fallbackProfile = await this.providerSettingsManager.getProfile({ name: fallback }) + const { name: _fallbackName, ...fallbackSettings } = fallbackProfile + await this.contextProxy.setValues({ + ...fallbackSettings, currentApiConfigName: fallback, - listApiConfigMeta: await this.providerSettingsManager.listConfig(), + listApiConfigMeta: remainingProfiles, }) } } catch (reconcileError) { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index d779f5b89e..856f96577d 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -87,6 +87,7 @@ vi.mock("../../../utils/storage", () => ({ getSettingsDirectoryPath: vi.fn().mockResolvedValue("/test/settings/path"), getTaskDirectoryPath: vi.fn().mockResolvedValue("/test/task/path"), getGlobalStoragePath: vi.fn().mockResolvedValue("/test/storage/path"), + getStorageBasePath: vi.fn().mockReturnValue("/test/storage/path"), })) vi.mock("@modelcontextprotocol/sdk/types.js", () => ({ @@ -3061,6 +3062,21 @@ describe("ClineProvider", () => { mockTask.updateApiConfiguration = vi.fn() await provider.addClineToStack(mockTask) + // Seed the legacy global-state history so `persistStickyProviderProfileToCurrentTask` + // finds a history item and performs the write. + await provider.contextProxy.setValue("taskHistory", [ + { + id: mockTask.taskId, + ts: Date.now(), + task: "Test task", + number: 1, + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + }, + ]) + const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([]) + await provider.deleteProviderProfile(currentProfile) // The deletion path must not skip the task rebuild: the running task receives the @@ -3073,6 +3089,13 @@ describe("ClineProvider", () => { ) // The sticky profile name is persisted too, so removing that update fails the test. expect(mockTask.setTaskApiConfigName).toHaveBeenCalledWith("other-config") + // And the history write happens, so removing it from the deletion path also fails. + expect(updateTaskHistorySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: mockTask.taskId, + apiConfigName: "other-config", + }), + ) }) test("fails fast without writing ContextProxy state when the config purge rejects", async () => { @@ -3084,15 +3107,22 @@ describe("ClineProvider", () => { }) const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") - const updateCallsBefore = vi.mocked(mockContext.globalState.update).mock.calls.length await expect(provider.deleteProviderProfile(otherProfile)).rejects.toThrow( "Config 'other-config' not found", ) expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("other-config") - expect(setValuesSpy).not.toHaveBeenCalled() - expect(vi.mocked(mockContext.globalState.update).mock.calls.length).toBe(updateCallsBefore) + // The purge failed, so no successful-path state write may run. The only `setValues` + // allowed is the rollback, which restores the pre-delete values verbatim. + for (const [values] of setValuesSpy.mock.calls) { + expect(values).toEqual( + expect.objectContaining({ + currentApiConfigName: "current-config", + listApiConfigMeta: [currentProfile, otherProfile], + }), + ) + } }) test("restores the deleted profile when replacement activation rejects", async () => { From 257eb9c7e6ba775f0391e3cad4e785f1deeb31ae Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 12:41:31 +0900 Subject: [PATCH 10/17] fix(core): purge deleted profile secrets and serialize the deletion rollback What: - ContextProxy.setProviderSettings now clears stale secret keys by consulting secretCache, so a deleted profile's differently-named secrets (openRouterApiKey when the replacement uses apiKey) no longer survive the switch in SecretStorage and keep being merged back by getValues. - Codebase-index credentials are excluded from that clear-out: they are both PROVIDER_SETTINGS_KEYS and SECRET_STATE_KEYS but are never part of a profile blob, so wiping them would delete the user's index credentials on every profile switch. - The deletion rollback runs inside the provider profile mutation queue with a new abortOnTimeout option, and re-checks the abort signal before each write. - The failed-restore fallback activates the fallback profile in the manager, so both stores name the same profile instead of leaving the manager pointing at the deleted one. - The inactive deletion branch checks signal.aborted before its write and writes only listApiConfigMeta instead of a stale full settings snapshot. - The restore writes the captured profile's full provider slice, not just the name pointers, so a failure after the replacement's settings write no longer leaves the replacement's settings under the restored name. Why: Verification of PR #1735 upheld four defects: retained secrets after cross-provider active deletion (P1 security), a divergent manager/ContextProxy pair after a failed restore, a rollback that raced queued mutations, and a partial rollback that left mixed provider settings. Each is reachable from the normal delete-provider-profile flow. Impact: Deleting a provider profile now removes its secrets from SecretStorage, leaves the manager and ContextProxy naming the same profile on every failure path, and cannot have its rollback interleaved with a newer queued mutation. Adds 7 regression tests; each was verified to fail with its fix reverted. --- src/core/config/ContextProxy.ts | 18 +- .../config/__tests__/ContextProxy.spec.ts | 41 ++++ src/core/webview/ClineProvider.ts | 177 ++++++++++++------ .../webview/__tests__/ClineProvider.spec.ts | 174 ++++++++++++++++- 4 files changed, 350 insertions(+), 60 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 1250a7baca..4984c34929 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -14,6 +14,7 @@ import { type RooCodeSettings, providerSettingsSchema, globalSettingsSchema, + codebaseIndexProviderSchema, isSecretStateKey, isProviderName, isRetiredProvider, @@ -32,6 +33,12 @@ const PASS_THROUGH_STATE_KEYS = ["taskHistory"] export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.includes(key) +// Codebase-index credentials are part of `ProviderSettings` but are not owned by a +// provider profile: the index settings handler writes them flat and `saveConfig`'s +// discriminated parse strips them from every profile blob. Clearing them here would +// delete the user's index credentials on every profile switch. +const FLAT_ONLY_PROVIDER_SECRET_KEYS = new Set(Object.keys(codebaseIndexProviderSchema.shape)) + const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, @@ -523,9 +530,16 @@ export class ContextProxy { } } + // Secret keys live in `secretCache`, not `stateCache`, so they need their own + // presence check. Without this a deleted profile's differently-named secrets + // (`openRouterApiKey` when the replacement uses `apiKey`) survive the switch and + // keep being merged back by `getValues`. + const isStale = (key: string) => + isSecretStateKey(key) ? !!this.secretCache[key as SecretStateKey] : !!this.stateCache[key as GlobalStateKey] + await this.setValues({ - ...PROVIDER_SETTINGS_KEYS.filter((key) => !isSecretStateKey(key)) - .filter((key) => !!this.stateCache[key]) + ...PROVIDER_SETTINGS_KEYS.filter((key) => !FLAT_ONLY_PROVIDER_SECRET_KEYS.has(key)) + .filter(isStale) .reduce((acc, key) => ({ ...acc, [key]: undefined }), {} as ProviderSettings), ...values, }) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index b4d3b1b8aa..5824c010ff 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -405,6 +405,47 @@ describe("ContextProxy", () => { expect(proxy.getGlobalState("apiModelId")).toBeUndefined() expect(proxy.getGlobalState("openAiBaseUrl")).toBeUndefined() }) + + it("clears a stale secret whose key the incoming configuration does not use", async () => { + // Anthropic -> OpenRouter: different secret key names, so the incoming configuration + // cannot overwrite the stale secret by name collision. + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "anthropic-secret", + }) + expect(proxy.getSecret("apiKey")).toBe("anthropic-secret") + + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "openrouter-secret", + }) + + // The stale secret must be deleted from both the cache and SecretStorage. A clear-out + // that only enumerates non-secret keys leaves it here and `getValues` keeps merging it. + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(proxy.getValues().apiKey).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + // The incoming secret still lands, so this is a targeted purge, not a blanket wipe. + expect(proxy.getSecret("openRouterApiKey")).toBe("openrouter-secret") + expect(mockSecrets.store).toHaveBeenCalledWith("openRouterApiKey", "openrouter-secret") + }) + + it("keeps the global image-generation secret and codebase-index credentials", async () => { + // Neither is owned by a provider profile: the image key is a global secret, and the + // index credentials are written flat by the index settings handler. + await proxy.storeSecret("openRouterImageApiKey", "image-secret") + await proxy.storeSecret("codeIndexOpenAiKey", "index-secret") + + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "anthropic-secret", + }) + + expect(proxy.getSecret("openRouterImageApiKey")).toBe("image-secret") + expect(proxy.getSecret("codeIndexOpenAiKey")).toBe("index-secret") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterImageApiKey") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("codeIndexOpenAiKey") + }) }) describe("resetAllState", () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b2d58d71cf..26d1efefa5 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -275,11 +275,13 @@ export class ClineProvider /** * With `runToCompletion`, a timeout only rejects the caller: the mutation is not aborted and the * queue advances once it settles. Use it for multi-write mutations that must not stop halfway - * (storage saved but not activated) or overlap with the next mutation. + * (storage saved but not activated) or overlap with the next mutation. Add `abortOnTimeout` when + * the remaining writes must not land after the caller was told the mutation failed: the body is + * aborted, but the queue still waits for it (and its rollback) to settle. */ private enqueueProviderProfileMutation( fn: (signal: AbortSignal) => Promise, - options: { runToCompletion?: boolean } = {}, + options: { runToCompletion?: boolean; abortOnTimeout?: boolean } = {}, ): Promise { const controller = new AbortController() // Run fn after either outcome so a rejected mutation never poisons the queue. @@ -288,13 +290,15 @@ export class ClineProvider () => fn(controller.signal), ) const callerResult = this.withProviderProfileMutationTimeout(run, () => { - if (options.runToCompletion) { - this.log("Provider profile mutation timed out; waiting for it to finish before the next mutation") - return + if (!options.runToCompletion || options.abortOnTimeout) { + controller.abort() } - controller.abort() - this.log("Provider profile mutation timed out; aborting in-flight mutation") + this.log( + options.runToCompletion + ? "Provider profile mutation timed out; waiting for it to finish before the next mutation" + : "Provider profile mutation timed out; aborting in-flight mutation", + ) }) void run.then( @@ -2116,12 +2120,15 @@ export class ClineProvider // Captured inside the queued mutation so the rollback below can restore them even when the // mutation times out (the timeout rejects the outer wrapper, so an inner catch never runs). let deletedProfileConfig: (ProviderSettingsWithId & { name: string }) | undefined - let previousContextValues: Pick = - {} + let previousContextValues: Pick = {} // The purge and any replacement switch run as one queued mutation: otherwise a queued // `upsertProviderProfile` for the same name can save the profile back after the purge and // make it reappear in `listApiConfigMeta` after this call has already resolved. + // + // The body keeps the default timeout semantics (abort, then let the queue advance): holding + // the queue here would deadlock the rollback below, which is itself queued, whenever the + // activation hangs. The rollback is what gets the hold-and-abort treatment. try { await this.enqueueProviderProfileMutation(async (signal) => { // Read the settings inside the queue: a queued activation may have changed the @@ -2132,7 +2139,6 @@ export class ClineProvider previousContextValues = { currentApiConfigName: globalSettings.currentApiConfigName, listApiConfigMeta: globalSettings.listApiConfigMeta, - apiProvider: globalSettings.apiProvider, } if (!deletedActiveProfile) { @@ -2140,12 +2146,16 @@ export class ClineProvider // matching the ordering the webview delete path established. await this.providerSettingsManager.deleteConfig(profileToDelete.name) - await this.contextProxy.setValues({ - ...globalSettings, - listApiConfigMeta: this.getProviderProfileEntries().filter( - ({ name }) => name !== profileToDelete.name, - ), - }) + // A timeout may have already fired while the manager lock was held. Writing + // the snapshot now would clobber whatever a newer mutation has since stored. + if (signal.aborted) { + return + } + + await this.contextProxy.setValue( + "listApiConfigMeta", + this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name), + ) await this.postStateToWebview() return @@ -2188,7 +2198,8 @@ export class ClineProvider }) } catch (error) { // Either the manager or ContextProxy can be left naming the deleted profile, so both - // are rolled back to their pre-delete state. + // are rolled back to their pre-delete state. The rollback is queued too, so no other + // mutation can interleave with its writes. await this.restoreDeletedProviderProfile(profileToDelete.name, deletedProfileConfig, previousContextValues) throw error } @@ -2202,57 +2213,115 @@ export class ClineProvider private async restoreDeletedProviderProfile( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: Pick, ): Promise { - // Bound the rollback independently: the failed activation may still be holding the - // manager lock, and the caller must not wait on it indefinitely. + // Run the rollback through the mutation queue: it used to race queued activations, and its + // late writes could overwrite a newer selection. `runToCompletion` keeps the queue held + // until every restore write has landed, and the aborted signal stops those writes once the + // caller has been told the restore timed out. The fallback reconcile runs inside the same + // queued body for the same reason. A restore timeout is logged, never rethrown: the caller + // must still see the original activation error. try { - await this.withProviderProfileMutationTimeout( - (async () => { - if (config) { - const { name: _name, ...providerSettings } = config - await this.providerSettingsManager.saveConfig(name, providerSettings) - await this.providerSettingsManager.activateProfile({ name }) + await this.enqueueProviderProfileMutation( + async (signal) => { + let restored = false + + try { + restored = await this.applyProviderProfileRestore(name, config, previousContextValues, signal) + } catch (error) { + this.log( + `Failed to restore provider profile '${name}' after a failed replacement activation: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + + if (restored) { + this.log(`Restored provider profile '${name}' after a failed replacement activation`) + return } - // Restore the settings too, not just the name pointers: a partial activation - // may have left the replacement's provider settings under the deleted name. - await this.contextProxy.setValues(previousContextValues) - })(), - () => this.log(`Provider profile restore for '${name}' timed out`), + // The restore did not finish, so both stores may still name a profile the manager + // no longer has. + await this.reconcileActiveProfileAfterFailedRestore() + }, + { runToCompletion: true, abortOnTimeout: true }, ) - this.log(`Restored provider profile '${name}' after a failed replacement activation`) } catch (error) { this.log( - `Failed to restore provider profile '${name}' after a failed replacement activation: ${ + `Provider profile restore for '${name}' did not complete: ${ error instanceof Error ? error.message : String(error) }`, ) + } + } - // The restore failed, so ContextProxy may still name a profile the manager no longer - // has. Point it at a profile that does exist, including that profile's settings, so a - // retry can proceed and the webview cannot show a name with the wrong settings. - try { - const remainingProfiles = await this.providerSettingsManager.listConfig() - const fallback = remainingProfiles[0]?.name + /** + * Rewrite the purged profile into the manager and ContextProxy. Returns false when the caller + * already timed out, so the caller can reconcile instead of leaving a half-applied restore. + */ + private async applyProviderProfileRestore( + name: string, + config: (ProviderSettingsWithId & { name: string }) | undefined, + previousContextValues: Pick, + signal: AbortSignal, + ): Promise { + if (config) { + // Keep `id`: the manager reuses it for the mode associations that point at this profile. + const { name: _name, ...providerSettings } = config + await this.providerSettingsManager.saveConfig(name, providerSettings) - if (fallback) { - const fallbackProfile = await this.providerSettingsManager.getProfile({ name: fallback }) - const { name: _fallbackName, ...fallbackSettings } = fallbackProfile + if (signal.aborted) return false - await this.contextProxy.setValues({ - ...fallbackSettings, - currentApiConfigName: fallback, - listApiConfigMeta: remainingProfiles, - }) - } - } catch (reconcileError) { - this.log( - `Failed to reconcile the active profile after a failed restore: ${ - reconcileError instanceof Error ? reconcileError.message : String(reconcileError) - }`, - ) + await this.providerSettingsManager.activateProfile({ name }) + + if (signal.aborted) return false + + // Restore the whole provider slice, not just the name pointers: a partial activation may + // have left the replacement's provider settings under the deleted name. `id` is manager + // metadata, not a setting, so it must not reach `ContextProxy`. `apiProvider` belongs to + // this slice, so the snapshot below deliberately does not carry it: writing it again + // would overwrite the restored provider with the pre-delete pointer. + const { id: _id, ...settings } = providerSettings + await this.contextProxy.setProviderSettings(settings) + } + + await this.contextProxy.setValues(previousContextValues) + return true + } + + /** + * Point both stores at a profile that still exists, including its settings, so a retry can + * proceed and the webview cannot show a name with the wrong settings. The manager is activated + * too: reconciling ContextProxy alone would leave the manager's own `currentApiConfigName` + * naming the deleted profile. + */ + private async reconcileActiveProfileAfterFailedRestore(): Promise { + try { + const remainingProfiles = await this.providerSettingsManager.listConfig() + const fallback = remainingProfiles[0]?.name + + if (!fallback) { + return } + + const { + name: _fallbackName, + id: _fallbackId, + ...fallbackSettings + } = await this.providerSettingsManager.activateProfile({ name: fallback }) + + await this.contextProxy.setValues({ + ...fallbackSettings, + currentApiConfigName: fallback, + listApiConfigMeta: remainingProfiles, + }) + } catch (reconcileError) { + this.log( + `Failed to reconcile the active profile after a failed restore: ${ + reconcileError instanceof Error ? reconcileError.message : String(reconcileError) + }`, + ) } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 88114cd8ad..1f6dab8a25 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3597,7 +3597,9 @@ describe("ClineProvider", () => { const profiles: ProviderSettingsEntry[] = [currentProfile, otherProfile] // Full configs keyed by name, so `getProfile`/`saveConfig` behave like the real manager. - // Each carries a distinct `apiKey` so a restore that drops credentials fails the test. + // The two profiles deliberately use *different* secret key names (`apiKey` for Anthropic, + // `openRouterApiKey` for OpenRouter): a same-named fixture lets a same-key overwrite hide + // a deleted profile's secret surviving the switch. const configs = new Map([ [currentProfile.name, { ...currentProfile, apiKey: "current-key" }], [otherProfile.name, { ...otherProfile, openRouterModelId: "other-model", apiKey: "other-key" }], @@ -3796,13 +3798,17 @@ describe("ClineProvider", () => { vi.useFakeTimers() try { - const { profiles } = mockProviderSettingsManager() + const { profiles, managerState } = mockProviderSettingsManager() let resolveActivation!: () => void const pendingActivation = new Promise((resolve) => { resolveActivation = resolve }) // Hang only the replacement activation; the restore's own activation still succeeds. + // The manager's pointer is written before the hang, which is where the real + // `activateProfile` sets it (inside the lock, before `store`): a rollback that only + // guards ContextProxy would leave the manager naming the replacement. vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + managerState.currentApiConfigName = otherProfile.name await pendingActivation return { ...otherProfile, openRouterModelId: "other-model" } }) @@ -3824,11 +3830,171 @@ describe("ClineProvider", () => { ) expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) - // A late completion of the abandoned activation must not move ContextProxy: the - // aborted signal makes `activateProviderProfileUnlocked` return before writing. + // A late completion of the abandoned activation must not move ContextProxy or the + // manager: the aborted signal makes `activateProviderProfileUnlocked` return before + // writing, and the rollback has already re-pointed the manager at the restored + // profile. resolveActivation() await vi.advanceTimersByTimeAsync(0) expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(managerState.currentApiConfigName).toBe("current-config") + } finally { + vi.useRealTimers() + } + }) + + test("purges the deleted profile's differently-named secret on an active cross-provider delete", async () => { + // Anthropic -> OpenRouter: the two profiles carry different secret key names, so the + // replacement's write cannot overwrite the deleted profile's secret by name collision. + const { profiles } = mockProviderSettingsManager() + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + return { ...otherProfile, openRouterModelId: "other-model", openRouterApiKey: "other-key" } + }) + + // Seed the deleted profile's secret exactly as an earlier activation of it would have. + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }) + expect(provider.contextProxy.getSecret("apiKey")).toBe("current-key") + + await provider.deleteProviderProfile(currentProfile) + + // The deleted profile's secret must be gone from the cache and from SecretStorage, not + // merely absent from the manager blob. Without the secret-aware clear-out it survives. + expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() + expect(mockContext.secrets.delete).toHaveBeenCalledWith("apiKey") + // The replacement's own secret is still stored, so the purge is not a blanket wipe. + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBe("other-key") + expect(provider.contextProxy.getValues().apiKey).toBeUndefined() + expect(profiles.map(({ name }) => name)).toEqual(["other-config"]) + }) + + test("reconciles the manager's active pointer when the restore fails", async () => { + const { managerState } = mockProviderSettingsManager() + // The replacement activation fails, and so does the restore, so the fallback runs. + vi.mocked(provider.providerSettingsManager.activateProfile) + .mockRejectedValueOnce(new Error("Failed to activate profile: storage error")) + .mockRejectedValueOnce(new Error("Failed to restore profile: storage error")) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to activate profile: storage error", + ) + + // ContextProxy alone is not enough: the manager would keep naming the deleted profile, + // so the next activation would disagree with what the webview shows. + expect(managerState.currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }) + }) + + test("keeps a queued activation's write when the rollback settles late", async () => { + const { managerState } = mockProviderSettingsManager() + let resolveRestore!: () => void + const pendingRestore = new Promise((resolve) => { + resolveRestore = resolve + }) + + // Fail the replacement activation, then hang the restore's own `saveConfig` so the + // rollback is still in flight while the next mutation is queued behind it. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { + await pendingRestore + return "current-id" + }) + + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Failed to activate profile: storage error") + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalled()) + + // Queue a newer mutation while the rollback is still pending. + const queued = provider.activateProviderProfile({ name: "other-config" }) + + // The rollback must still hold the queue, so the newer activation cannot write yet. + // Without the queued rollback this activation runs immediately and the late rollback + // then overwrites it. + await vi.waitFor(async () => { + expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledTimes(1) + }) + expect(managerState.currentApiConfigName).toBe("current-config") + + // Let the rollback finish late; only then may the queued activation write. + resolveRestore() + await deletionResult + await queued + + // The rollback's late completion must not undo the queued activation's selection. + expect(managerState.currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }) + }) + + test("restores the deleted profile's full settings when activation fails after its settings write", async () => { + mockProviderSettingsManager() + + // Fail after the replacement's provider settings have been written, the way a + // `setModeConfig` rejection does: the name pointers and the settings then disagree. + vi.mocked(provider.providerSettingsManager.setModeConfig).mockRejectedValueOnce( + new Error("Failed to set mode config: storage error"), + ) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to set mode config: storage error", + ) + + // Restoring only the name pointers would leave OpenRouter's `openRouterModelId` under + // the restored Anthropic name. The whole provider slice must come back. + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }) + expect(provider.contextProxy.getProviderSettings().openRouterModelId).toBeUndefined() + }) + + test("does not purge an inactive profile after the caller already timed out", async () => { + vi.useFakeTimers() + + try { + const { profiles } = mockProviderSettingsManager() + let resolvePurge!: () => void + const pendingPurge = new Promise((resolve) => { + resolvePurge = resolve + }) + // Stall the inactive purge past the mutation timeout, then let it complete. + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce(async () => { + await pendingPurge + const index = profiles.findIndex((profile) => profile.name === otherProfile.name) + profiles.splice(index, 1) + }) + + const setValueSpy = vi.spyOn(provider.contextProxy, "setValue") + const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") + + const deletion = provider.deleteProviderProfile(otherProfile) + const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") + + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + await deletionResult + + // The caller was told the deletion failed, so the stale snapshot must not be written + // over whatever a newer mutation has since stored. Clear the earlier calls (setup and + // the purge attempt itself) so only the post-abort writes are observed; this branch's + // only write is `listApiConfigMeta`. + setValueSpy.mockClear() + setValuesSpy.mockClear() + resolvePurge() + await vi.advanceTimersByTimeAsync(0) + expect(setValueSpy).not.toHaveBeenCalled() + expect(setValuesSpy).not.toHaveBeenCalled() } finally { vi.useRealTimers() } From 69a7d7df8e7223295db6b5c7b6bba50831294b13 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 13:18:35 +0900 Subject: [PATCH 11/17] fix(core): drop orphaned mode bindings when a provider profile is deleted What: - `ProviderSettingsManager.deleteConfig` now drops every `modeApiConfigs` entry that points at the deleted profile inside the same lock and the same single `store()` as the purge, and returns the dropped entries as `removedModeConfigs`. An id that a remaining profile still carries is left alone, so `renameApiConfiguration` (which saves the new name with the old id before deleting the old name) keeps its bindings. - New `ProviderSettingsManager.getModeApiConfigs()` returns a copy of the manager's map, so callers that mirror it into `ContextProxy` cannot drift. - `ClineProvider.deleteProviderProfile` mirrors that map into ContextProxy in both branches: the inactive branch folds it into its existing `setValues`, the active branch writes it after the replacement activation and posts state again (the activation posts before the mirror lands). - The rollback re-applies `removedModeConfigs` through `setModeConfig` and restores the pre-delete `modeApiConfigs` snapshot, symmetric with the full-slice restore. - The webview `deleteApiConfiguration` twin mirrors the map too. Why: `deleteConfig` removed only `apiConfigs[name]`, so `modeApiConfigs` kept the dead id. `handleModeSwitchUnlocked` resolves a mode through `getModeConfigId` and then looks the id up in `listApiConfig`; when the id is gone the lookup misses and the "no saved config for this mode" branch runs, so the switch silently keeps the current profile instead of moving to a real one. Reachable from both delete entry points, in both the active and the inactive branch. Round-1 finding, two seats (K31PRO P1 conf99, MLARGE P2 conf85); deferred by the fix1 unit. Impact: A mode bound to a deleted profile is cleared rather than rebound: an unbound mode already has documented fallbacks (mode switch saves the current profile as that mode's default; delegation inherits the parent task's config), while rebinding would reassign modes the user never chose. Six regression tests cover both branches, the rename guard, the rollback and the webview twin; each was verified by reverting only its fix. --- src/core/config/ProviderSettingsManager.ts | 40 +++++- .../__tests__/ProviderSettingsManager.spec.ts | 69 ++++++++++ src/core/webview/ClineProvider.ts | 68 ++++++++-- .../webview/__tests__/ClineProvider.spec.ts | 123 +++++++++++++++++- src/core/webview/webviewMessageHandler.ts | 8 ++ 5 files changed, 293 insertions(+), 15 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 01695d7994..3795ef0e43 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -503,9 +503,12 @@ export class ProviderSettingsManager { } /** - * Delete a config by name. + * Delete a config by name. The per-mode bindings that point at the deleted profile are dropped + * with it: a surviving binding makes the next switch to that mode resolve no profile and + * silently keep the current one. The dropped bindings are returned so a caller that rolls the + * deletion back can re-apply them. */ - public async deleteConfig(name: string) { + public async deleteConfig(name: string): Promise<{ removedModeConfigs: Record }> { try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -518,8 +521,26 @@ export class ProviderSettingsManager { throw new Error(`Cannot delete the last remaining configuration`) } + const deletedId = providerProfiles.apiConfigs[name].id delete providerProfiles.apiConfigs[name] + + const removedModeConfigs: Record = {} + // A rename saves the profile under its new name with the same `id` before deleting + // the old one, so only an id no remaining profile carries is an orphan. + const idStillInUse = + !!deletedId && Object.values(providerProfiles.apiConfigs).some((config) => config.id === deletedId) + + if (deletedId && !idStillInUse) { + for (const [mode, configId] of Object.entries(providerProfiles.modeApiConfigs ?? {})) { + if (configId === deletedId) { + removedModeConfigs[mode] = configId + delete providerProfiles.modeApiConfigs![mode] + } + } + } + await this.store(providerProfiles) + return { removedModeConfigs } }) } catch (error) { throw new Error(`Failed to delete config: ${error}`) @@ -574,6 +595,21 @@ export class ProviderSettingsManager { } } + /** + * Get every per-mode config binding. Callers that mirror this map into `ContextProxy` read it + * here rather than recomputing it, so the two stores cannot drift. + */ + public async getModeApiConfigs(): Promise> { + try { + return await this.lock(async () => { + const { modeApiConfigs } = await this.load() + return { ...modeApiConfigs } + }) + } catch (error) { + throw new Error(`Failed to get mode configs: ${error}`) + } + } + public async export() { try { return await this.lock(async () => { diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index a1fc9ac7de..9b1a903ae6 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -994,6 +994,75 @@ describe("ProviderSettingsManager", () => { "Failed to delete config: Error: Cannot delete the last remaining configuration", ) }) + + it("should drop the mode bindings that pointed at the deleted config", async () => { + const existingConfig: ProviderProfiles = { + currentApiConfigName: "default", + apiConfigs: { + default: { + id: "default", + }, + test: { + apiProvider: providerIdentifiers.anthropic, + id: "test-id", + }, + }, + modeApiConfigs: { + code: "test-id", + architect: "default", + ask: "test-id", + }, + migrations: { + rateLimitSecondsMigrated: false, + }, + } + + mockSecrets.get.mockResolvedValue(JSON.stringify(existingConfig)) + + const { removedModeConfigs } = await providerSettingsManager.deleteConfig("test") + + // A surviving binding would make the next switch to that mode resolve a profile that no + // longer exists, so the mode silently keeps the current profile instead of switching. + const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1]) + expect(storedConfig.modeApiConfigs).toEqual({ architect: "default" }) + // The dropped bindings come back so a rollback can re-apply them. + expect(removedModeConfigs).toEqual({ code: "test-id", ask: "test-id" }) + }) + + it("should keep the mode bindings when the deleted config's id is reused by a rename", async () => { + // `renameApiConfiguration` saves the new name with the old `id` and then deletes the old + // name, so the id is still live and its mode bindings must survive the purge. + const existingConfig: ProviderProfiles = { + currentApiConfigName: "default", + apiConfigs: { + default: { + id: "default", + }, + renamed: { + apiProvider: providerIdentifiers.anthropic, + id: "test-id", + }, + test: { + apiProvider: providerIdentifiers.anthropic, + id: "test-id", + }, + }, + modeApiConfigs: { + code: "test-id", + }, + migrations: { + rateLimitSecondsMigrated: false, + }, + } + + mockSecrets.get.mockResolvedValue(JSON.stringify(existingConfig)) + + const { removedModeConfigs } = await providerSettingsManager.deleteConfig("test") + + const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1]) + expect(storedConfig.modeApiConfigs).toEqual({ code: "test-id" }) + expect(removedModeConfigs).toEqual({}) + }) }) describe("LoadConfig", () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 26d1efefa5..2bc940636c 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2120,7 +2120,12 @@ export class ClineProvider // Captured inside the queued mutation so the rollback below can restore them even when the // mutation times out (the timeout rejects the outer wrapper, so an inner catch never runs). let deletedProfileConfig: (ProviderSettingsWithId & { name: string }) | undefined - let previousContextValues: Pick = {} + let previousContextValues: Pick< + RooCodeSettings, + "currentApiConfigName" | "listApiConfigMeta" | "modeApiConfigs" + > = {} + // The mode bindings the purge dropped from the manager, so the rollback can re-apply them. + let removedModeConfigs: Record = {} // The purge and any replacement switch run as one queued mutation: otherwise a queued // `upsertProviderProfile` for the same name can save the profile back after the purge and @@ -2139,12 +2144,14 @@ export class ClineProvider previousContextValues = { currentApiConfigName: globalSettings.currentApiConfigName, listApiConfigMeta: globalSettings.listApiConfigMeta, + modeApiConfigs: globalSettings.modeApiConfigs, } if (!deletedActiveProfile) { // Purge first so a failing purge fails fast before any ContextProxy write, // matching the ordering the webview delete path established. - await this.providerSettingsManager.deleteConfig(profileToDelete.name) + removedModeConfigs = (await this.providerSettingsManager.deleteConfig(profileToDelete.name)) + .removedModeConfigs // A timeout may have already fired while the manager lock was held. Writing // the snapshot now would clobber whatever a newer mutation has since stored. @@ -2152,10 +2159,12 @@ export class ClineProvider return } - await this.contextProxy.setValue( - "listApiConfigMeta", - this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name), - ) + await this.contextProxy.setValues({ + listApiConfigMeta: this.getProviderProfileEntries().filter( + ({ name }) => name !== profileToDelete.name, + ), + modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), + }) await this.postStateToWebview() return @@ -2188,19 +2197,38 @@ export class ClineProvider return } - await this.providerSettingsManager.deleteConfig(profileToDelete.name) + removedModeConfigs = (await this.providerSettingsManager.deleteConfig(profileToDelete.name)) + .removedModeConfigs // Route through the standard activation path so the effective provider // settings, mode config, and current-task wiring all end up identical to // a user activating the replacement profile from the webview. The unlocked // variant is used because this already runs inside the mutation queue. await this.activateProviderProfileUnlocked({ name: profileToActivate }, undefined, signal) + + // The activation above only rebinds the *current* mode, so every other mode that + // pointed at the deleted profile would keep a dangling id and silently no-op on the + // next switch. The manager dropped those bindings with the purge; mirror its map into + // ContextProxy so the two stores agree. + await this.contextProxy.setValue( + "modeApiConfigs", + await this.providerSettingsManager.getModeApiConfigs(), + ) + + // The activation posted state before this mirror landed, so post again: the webview + // must not keep a map that names a mode bound to a profile the manager no longer has. + await this.postStateToWebview() }) } catch (error) { // Either the manager or ContextProxy can be left naming the deleted profile, so both // are rolled back to their pre-delete state. The rollback is queued too, so no other // mutation can interleave with its writes. - await this.restoreDeletedProviderProfile(profileToDelete.name, deletedProfileConfig, previousContextValues) + await this.restoreDeletedProviderProfile( + profileToDelete.name, + deletedProfileConfig, + previousContextValues, + removedModeConfigs, + ) throw error } } @@ -2213,7 +2241,8 @@ export class ClineProvider private async restoreDeletedProviderProfile( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: Pick, + removedModeConfigs: Record, ): Promise { // Run the rollback through the mutation queue: it used to race queued activations, and its // late writes could overwrite a newer selection. `runToCompletion` keeps the queue held @@ -2227,7 +2256,13 @@ export class ClineProvider let restored = false try { - restored = await this.applyProviderProfileRestore(name, config, previousContextValues, signal) + restored = await this.applyProviderProfileRestore( + name, + config, + previousContextValues, + removedModeConfigs, + signal, + ) } catch (error) { this.log( `Failed to restore provider profile '${name}' after a failed replacement activation: ${ @@ -2263,7 +2298,8 @@ export class ClineProvider private async applyProviderProfileRestore( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: Pick, + removedModeConfigs: Record, signal: AbortSignal, ): Promise { if (config) { @@ -2284,6 +2320,16 @@ export class ClineProvider // would overwrite the restored provider with the pre-delete pointer. const { id: _id, ...settings } = providerSettings await this.contextProxy.setProviderSettings(settings) + + // `saveConfig` above reused the profile's `id`, so the mode bindings the purge dropped + // name a profile that exists again. Re-apply them in the manager, then mirror the + // pre-delete map into ContextProxy: the snapshot below restores every mode, including + // the ones the purge never touched. + for (const [mode, configId] of Object.entries(removedModeConfigs)) { + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + } + + if (signal.aborted) return false } await this.contextProxy.setValues(previousContextValues) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 1f6dab8a25..8849e35cd5 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3581,6 +3581,44 @@ describe("ClineProvider", () => { }) }) + describe("deleteApiConfiguration", () => { + test("mirrors the mode bindings the purge dropped into ContextProxy", async () => { + await provider.resolveWebviewView(mockWebviewView) + const messageHandler = mockWebviewView.webview.onDidReceiveMessage.mock.calls[0][0] + + const survivingProfile: ProviderSettingsEntry = { + name: "other-config", + id: "other-id", + apiProvider: providerIdentifiers.openrouter, + } + const modeApiConfigs: Record = { code: "deleted-id", ask: "other-id" } + + Object.assign(provider, { + providerSettingsManager: { + deleteConfig: vi.fn().mockImplementation(async () => { + const removedModeConfigs = { code: modeApiConfigs.code } + delete modeApiConfigs.code + return { removedModeConfigs } + }), + listConfig: vi.fn().mockResolvedValue([survivingProfile]), + getModeApiConfigs: vi.fn().mockImplementation(async () => ({ ...modeApiConfigs })), + }, + }) + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + "answers.yes" as unknown as vscode.MessageItem, + ) + vi.spyOn(provider, "activateProviderProfile").mockResolvedValue(undefined) + + await provider.contextProxy.setValue("modeApiConfigs", { code: "deleted-id", ask: "other-id" }) + + await messageHandler({ type: "deleteApiConfiguration", text: "deleted-config" }) + + // The webview delete path shares the purge, so it shares the orphan: a mode left on the + // deleted id silently no-ops on the next switch instead of moving to a real profile. + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual({ ask: "other-id" }) + }) + }) + describe("deleteProviderProfile", () => { const currentProfile: ProviderSettingsEntry = { name: "current-config", @@ -3609,14 +3647,29 @@ describe("ClineProvider", () => { // manager's own notion of the active profile, not just ContextProxy's. const managerState = { currentApiConfigName: currentProfile.name } + // Mirrors the manager's own `modeApiConfigs`, so a test can prove the purge drops the + // bindings that pointed at the deleted profile instead of leaving a dangling id. + const modeApiConfigs: Record = {} + Object.assign(provider, { providerSettingsManager: { deleteConfig: vi.fn().mockImplementation(async (name: string) => { + const removedModeConfigs: Record = {} + const deletedId = configs.get(name)?.id const index = profiles.findIndex((profile) => profile.name === name) if (index !== -1) { profiles.splice(index, 1) } configs.delete(name) + + for (const [mode, configId] of Object.entries(modeApiConfigs)) { + if (configId === deletedId) { + removedModeConfigs[mode] = configId + delete modeApiConfigs[mode] + } + } + + return { removedModeConfigs } }), listConfig: vi.fn().mockImplementation(async () => profiles), getProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { @@ -3643,11 +3696,15 @@ describe("ClineProvider", () => { // effective provider settings, not just the name pointers. return { ...config, name } }), - setModeConfig: vi.fn(), + setModeConfig: vi.fn().mockImplementation(async (mode: string, configId: string) => { + modeApiConfigs[mode] = configId + }), + getModeConfigId: vi.fn().mockImplementation(async (mode: string) => modeApiConfigs[mode]), + getModeApiConfigs: vi.fn().mockImplementation(async () => ({ ...modeApiConfigs })), }, }) - return { profiles, managerState } + return { profiles, managerState, modeApiConfigs } } beforeEach(async () => { @@ -3960,6 +4017,67 @@ describe("ClineProvider", () => { expect(provider.contextProxy.getProviderSettings().openRouterModelId).toBeUndefined() }) + test("clears the mode bindings that pointed at a deleted inactive profile", async () => { + const { modeApiConfigs } = mockProviderSettingsManager() + // Two modes are bound to the profile being deleted; a third stays on the surviving one. + modeApiConfigs.code = otherProfile.id + modeApiConfigs.architect = otherProfile.id + modeApiConfigs.ask = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + await provider.deleteProviderProfile(otherProfile) + + // A binding that survives the purge points a mode at a profile the manager no longer + // has, so the next switch to that mode resolves nothing and silently keeps the current + // profile. Both stores must drop it instead. + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual({ ask: currentProfile.id }) + expect(modeApiConfigs).toEqual({ ask: currentProfile.id }) + }) + + test("rebinds the deleted active profile's other modes to the replacement", async () => { + await provider.resolveWebviewView(mockWebviewView) + const { modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + modeApiConfigs.ask = otherProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + await provider.deleteProviderProfile(currentProfile) + + // The activation rebinds only the current mode (`code`); every other mode that pointed + // at the deleted profile must not keep a dangling id either. + expect(modeApiConfigs).toEqual({ code: otherProfile.id, ask: otherProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) + + // The activation posts state before this mirror lands, so the mirror needs its own post. + // Without it the webview keeps a mode bound to a profile the manager no longer has. + const postedStates = mockPostMessage.mock.calls.filter( + ([message]: [ExtensionMessage]) => message.type === "state", + ) + expect(postedStates.length).toBeGreaterThanOrEqual(2) + }) + + test("re-applies the mode bindings when the deletion rolls back", async () => { + const { modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // Fail the replacement activation so the purge is rolled back. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to activate profile: storage error", + ) + + // The restore reuses the profile's `id`, so the bindings the purge dropped name a live + // profile again and must come back in both stores, not just ContextProxy. + expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) + }) + test("does not purge an inactive profile after the caller already timed out", async () => { vi.useFakeTimers() @@ -3974,6 +4092,7 @@ describe("ClineProvider", () => { await pendingPurge const index = profiles.findIndex((profile) => profile.name === otherProfile.name) profiles.splice(index, 1) + return { removedModeConfigs: {} } }) const setValueSpy = vi.spyOn(provider.contextProxy, "setValue") diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index bd7c589958..a8b587c9f1 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2524,6 +2524,14 @@ export const webviewMessageHandler = async ( try { await provider.providerSettingsManager.deleteConfig(oldName) await provider.activateProviderProfile({ name: newName }) + + // The purge drops the mode bindings that pointed at the deleted profile, and the + // activation only rebinds the current mode. Mirror the manager's map so the + // webview state cannot keep pointing a mode at a profile that no longer exists. + await provider.contextProxy.setValue( + "modeApiConfigs", + await provider.providerSettingsManager.getModeApiConfigs(), + ) } catch (error) { provider.log( `Error delete api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`, From 3f0e5819fc3edc3c3337e5570a94b77d692b10c9 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 16:48:35 +0900 Subject: [PATCH 12/17] fix(core): bound the profile-mutation queue and make the delete rollback atomic What: - `enqueueProviderProfileMutation` now delegates to a single `enqueueProviderProfileMutationWithBody` instead of duplicating the whole queue primitive. The queue tail advances from the raw body only for a `runToCompletion` body WITHOUT `abortOnTimeout`; an `abortOnTimeout` body advances on the timeout-bounded result, so a storage call that never settles cannot pin the queue for every later mutation. - The delete rollback waits for the abandoned delete body through a new `settleProviderProfileMutationBody`, bounded to one mutation window. Waiting unbounded would re-introduce the pin the bound exists to remove; after the bound the body's own abort fences keep its late writes out. - The inactive deletion branch captures the profile before the purge and, when a timeout fires while the manager lock is held, undoes the purge that already committed (`undoCommittedProviderProfilePurge`). Previously the purge landed after the caller was told the deletion failed, and the inactive branch never activates a replacement, so nothing converged the two stores. - `reconcileActiveProfileAfterFailedRestore` re-applies the purge's dropped mode bindings BEFORE the single `modeApiConfigs` mirror write, so ContextProxy never holds a map the manager disagrees with. Why: Round-2 review left three P1s and five P2s on this delta. Two of them were regressions introduced by the round-2 fix itself: the queue advanced on `callerResult` for every `runToCompletion` body even though only `abortOnTimeout` bodies get a fired signal, so `upsertProviderProfile` and `updateProfileModel` could have their remaining writes land after the next mutation started; and the failed-restore reconcile wrote the mode map before re-applying the dropped bindings. The remaining items were the inactive-branch purge committing after a reported failure, the rollback racing the abandoned body, the fallback merging instead of replacing the provider slice, and the webview twin mirroring only on success. Impact: A hung SecretStorage write can no longer pin the profile-mutation queue, an inactive deletion that times out no longer leaves the profile deleted, and the rollback cannot be overwritten by the body it compensates. The inactive-branch regression test now asserts the profile really survives instead of only watching spies, and the previously inverted `apiHandlerRebuild` assertion is restored to the hold contract that applies to a non-aborting body. The e2e-mock job is still red. The three failing subtasks tests reproduce locally at this head, but they never delete a profile and the CI logs contain zero profile-mutation activity, so the reported queue-pin trace is disproved; that failure is not addressed here. --- src/core/webview/ClineProvider.ts | 206 +++++++++-- .../ClineProvider.apiHandlerRebuild.spec.ts | 2 + .../webview/__tests__/ClineProvider.spec.ts | 336 ++++++++++++++---- src/core/webview/webviewMessageHandler.ts | 25 +- 4 files changed, 475 insertions(+), 94 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2bc940636c..6032863946 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -273,18 +273,35 @@ export class ClineProvider } /** - * With `runToCompletion`, a timeout only rejects the caller: the mutation is not aborted and the - * queue advances once it settles. Use it for multi-write mutations that must not stop halfway - * (storage saved but not activated) or overlap with the next mutation. Add `abortOnTimeout` when - * the remaining writes must not land after the caller was told the mutation failed: the body is - * aborted, but the queue still waits for it (and its rollback) to settle. + * With `runToCompletion`, a timeout only rejects the caller: the queue advances once the body + * settles. Use it for multi-write mutations that must not stop halfway (storage saved but not + * activated) or overlap with the next mutation. Add `abortOnTimeout` when the remaining writes + * must not land after the caller was told the mutation failed: the body is aborted, and the + * queue may advance on the timeout instead of waiting for it. + * + * A `runToCompletion` body without `abortOnTimeout` keeps the queue until it settles, because + * nothing would stop its remaining writes from landing after the next mutation started. With + * `abortOnTimeout` the queue advances on the timeout-bounded result, so a body hung on storage + * (e.g. a never-settling SecretStorage write behind the manager lock) cannot pin the queue: + * every write in such a body re-checks its AbortSignal, which the timeout fires. */ private enqueueProviderProfileMutation( fn: (signal: AbortSignal) => Promise, options: { runToCompletion?: boolean; abortOnTimeout?: boolean } = {}, ): Promise { + return this.enqueueProviderProfileMutationWithBody(fn, options).callerResult + } + + /** + * The queue primitive behind `enqueueProviderProfileMutation`. It also returns the raw body + * promise so a caller can wait for an abandoned body to settle (e.g. before running its + * rollback) without being subject to the mutation timeout. + */ + private enqueueProviderProfileMutationWithBody( + fn: (signal: AbortSignal) => Promise, + options: { runToCompletion?: boolean; abortOnTimeout?: boolean } = {}, + ): { callerResult: Promise; body: Promise } { const controller = new AbortController() - // Run fn after either outcome so a rejected mutation never poisons the queue. const run = this.providerProfileMutationQueue.then( () => fn(controller.signal), () => fn(controller.signal), @@ -318,13 +335,16 @@ export class ClineProvider }, ) - // Advance from the timeout-bounded result. Each fn checks its AbortSignal before - // writing state, so advancing the queue on timeout cannot produce stale overwrites. - this.providerProfileMutationQueue = (options.runToCompletion ? run : callerResult).then( + // Advance from the raw body unless the timeout aborted it: an aborted body's remaining + // writes are fenced by its own signal checks, so the queue can move on without waiting for + // a hung storage call. A body that was never aborted still holds the queue, or its late + // writes would land after the next mutation. + const queueTail = options.runToCompletion && !options.abortOnTimeout ? run : callerResult + this.providerProfileMutationQueue = queueTail.then( () => undefined, () => undefined, ) - return callerResult + return { callerResult, body: run } } private withProviderProfileMutationTimeout(operation: Promise, onTimeout: () => void): Promise { @@ -343,6 +363,39 @@ export class ClineProvider }) } + /** + * Wait for an abandoned mutation body to settle, but never longer than one mutation window. + * + * A rollback must not run while the body it compensates can still write, so it waits for that + * body first. That wait has to be bounded: a body hung on a never-settling SecretStorage write + * holds the manager lock, and an unbounded wait would pin the rollback (and the whole mutation + * queue behind it) forever. After the bound the body's own abort fences are what keep its late + * writes out, so the rollback proceeds instead of waiting indefinitely. + */ + private async settleProviderProfileMutationBody(body: Promise): Promise { + let timeoutId: ReturnType | undefined + const bound = new Promise((resolve) => { + timeoutId = setTimeout(() => { + this.log("Abandoned provider profile mutation still running; rolling back without it") + resolve() + }, ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + }) + + try { + await Promise.race([ + body.then( + () => undefined, + () => undefined, + ), + bound, + ]) + } finally { + if (timeoutId) { + clearTimeout(timeoutId) + } + } + } + private readonly pendingEditOperations: PendingEditOperationStore private cloudOrganizationsCache: CloudOrganizationMembership[] | null = null @@ -2133,9 +2186,13 @@ export class ClineProvider // // The body keeps the default timeout semantics (abort, then let the queue advance): holding // the queue here would deadlock the rollback below, which is itself queued, whenever the - // activation hangs. The rollback is what gets the hold-and-abort treatment. + // activation hangs. The rollback is what gets the hold-and-abort treatment, and it waits for + // this body to settle first so its writes can never race the abandoned body's late writes. + // The body promise is tracked separately because the queue enqueuer returns the + // timeout-bounded caller result, not the raw body. + let deleteBodySettled: Promise = Promise.resolve() try { - await this.enqueueProviderProfileMutation(async (signal) => { + const enqueued = this.enqueueProviderProfileMutationWithBody(async (signal) => { // Read the settings inside the queue: a queued activation may have changed the // active profile since this call was made. const globalSettings = this.contextProxy.getValues() @@ -2148,14 +2205,43 @@ export class ClineProvider } if (!deletedActiveProfile) { + // A timeout may have already fired while this body was queued or while an + // earlier manager call held the lock. Purging now would delete the profile + // after the caller was told the deletion failed. + if (signal.aborted) { + return + } + + // Captured before the purge so a timeout that fires while the manager lock is + // held can undo a purge that already committed. Without it the manager would + // drop the profile while the caller was told the deletion failed, and the + // inactive branch never activates a replacement to converge the two stores. + // Best-effort: a manager that cannot produce the profile leaves nothing to undo. + let undoConfig: (ProviderSettingsWithId & { name: string }) | undefined + try { + undoConfig = await this.providerSettingsManager.getProfile({ name: profileToDelete.name }) + } catch { + undoConfig = undefined + } + + if (signal.aborted) { + return + } + // Purge first so a failing purge fails fast before any ContextProxy write, // matching the ordering the webview delete path established. removedModeConfigs = (await this.providerSettingsManager.deleteConfig(profileToDelete.name)) .removedModeConfigs - // A timeout may have already fired while the manager lock was held. Writing - // the snapshot now would clobber whatever a newer mutation has since stored. + // A timeout may have fired while the manager lock was held. The purge committed, + // so the caller must not be left with a deleted profile it was told survived: + // undo it instead of returning with the two stores disagreeing. if (signal.aborted) { + await this.undoCommittedProviderProfilePurge( + profileToDelete.name, + undoConfig, + removedModeConfigs, + ) return } @@ -2206,6 +2292,13 @@ export class ClineProvider // variant is used because this already runs inside the mutation queue. await this.activateProviderProfileUnlocked({ name: profileToActivate }, undefined, signal) + // A timeout may have fired while the activation was in flight. The queued + // rollback is restoring the pre-delete state, so these tail writes must not + // land after it. + if (signal.aborted) { + return + } + // The activation above only rebinds the *current* mode, so every other mode that // pointed at the deleted profile would keep a dangling id and silently no-op on the // next switch. The manager dropped those bindings with the purge; mirror its map into @@ -2219,10 +2312,15 @@ export class ClineProvider // must not keep a map that names a mode bound to a profile the manager no longer has. await this.postStateToWebview() }) + deleteBodySettled = enqueued.body + await enqueued.callerResult } catch (error) { // Either the manager or ContextProxy can be left naming the deleted profile, so both // are rolled back to their pre-delete state. The rollback is queued too, so no other - // mutation can interleave with its writes. + // mutation can interleave with its writes, and it first waits (bounded) for the + // abandoned delete body to settle so no late write of that body can land after the + // restoration. + await this.settleProviderProfileMutationBody(deleteBodySettled) await this.restoreDeletedProviderProfile( profileToDelete.name, deletedProfileConfig, @@ -2277,8 +2375,9 @@ export class ClineProvider } // The restore did not finish, so both stores may still name a profile the manager - // no longer has. - await this.reconcileActiveProfileAfterFailedRestore() + // no longer has. The reconcile also re-applies the purge's dropped mode bindings + // and mirrors the mode map, so an aborted restore cannot silently lose them. + await this.reconcileActiveProfileAfterFailedRestore(removedModeConfigs) }, { runToCompletion: true, abortOnTimeout: true }, ) @@ -2291,6 +2390,42 @@ export class ClineProvider } } + /** + * Undo a purge that committed after the caller was already told the deletion timed out. + * + * The inactive branch has no replacement activation, so nothing else would converge the two + * stores: the manager would be missing the profile while `listApiConfigMeta` still lists it. + * Re-saving the captured config restores the profile under its original `id`, which makes the + * mode bindings the purge dropped name a live profile again. Best-effort: a failure here is + * logged rather than replacing the timeout the caller already received. + */ + private async undoCommittedProviderProfilePurge( + name: string, + config: (ProviderSettingsWithId & { name: string }) | undefined, + removedModeConfigs: Record, + ): Promise { + if (!config) { + return + } + + try { + const { name: _name, ...providerSettings } = config + await this.providerSettingsManager.saveConfig(name, providerSettings) + + for (const [mode, configId] of Object.entries(removedModeConfigs)) { + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + } + + this.log(`Undid a provider profile purge for '${name}' that committed after the caller timed out`) + } catch (error) { + this.log( + `Failed to undo the provider profile purge for '${name}' after a timeout: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + /** * Rewrite the purged profile into the manager and ContextProxy. Returns false when the caller * already timed out, so the caller can reconcile instead of leaving a half-applied restore. @@ -2321,11 +2456,14 @@ export class ClineProvider const { id: _id, ...settings } = providerSettings await this.contextProxy.setProviderSettings(settings) + if (signal.aborted) return false + // `saveConfig` above reused the profile's `id`, so the mode bindings the purge dropped - // name a profile that exists again. Re-apply them in the manager, then mirror the - // pre-delete map into ContextProxy: the snapshot below restores every mode, including - // the ones the purge never touched. + // name a profile that exists again. Re-apply them in the manager with a per-write abort + // check, then mirror the pre-delete map into ContextProxy: the snapshot below restores + // every mode, including the ones the purge never touched. for (const [mode, configId] of Object.entries(removedModeConfigs)) { + if (signal.aborted) return false await this.providerSettingsManager.setModeConfig(mode as Mode, configId) } @@ -2341,8 +2479,15 @@ export class ClineProvider * proceed and the webview cannot show a name with the wrong settings. The manager is activated * too: reconciling ContextProxy alone would leave the manager's own `currentApiConfigName` * naming the deleted profile. + * + * The fallback's provider slice is written through `setProviderSettings` so the stale-clear + * semantics apply: a raw `setValues` merge would leave the restored profile's differently-named + * secrets behind under the fallback name. The mode map is reconciled too, so ContextProxy cannot + * keep pointing a mode at the deleted profile after the purge dropped the binding in the manager. */ - private async reconcileActiveProfileAfterFailedRestore(): Promise { + private async reconcileActiveProfileAfterFailedRestore( + removedModeConfigs: Record = {}, + ): Promise { try { const remainingProfiles = await this.providerSettingsManager.listConfig() const fallback = remainingProfiles[0]?.name @@ -2357,11 +2502,18 @@ export class ClineProvider ...fallbackSettings } = await this.providerSettingsManager.activateProfile({ name: fallback }) + await this.contextProxy.setProviderSettings(fallbackSettings) await this.contextProxy.setValues({ - ...fallbackSettings, currentApiConfigName: fallback, listApiConfigMeta: remainingProfiles, }) + // The purge may have dropped mode bindings the restore could not re-apply; re-apply + // them now so the manager's map matches the pre-delete state again. The mirror below + // runs after the re-apply, so ContextProxy never holds a map the manager disagrees with. + for (const [mode, configId] of Object.entries(removedModeConfigs)) { + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + } + await this.contextProxy.setValue("modeApiConfigs", await this.providerSettingsManager.getModeApiConfigs()) } catch (reconcileError) { this.log( `Failed to reconcile the active profile after a failed restore: ${ @@ -2440,12 +2592,20 @@ export class ClineProvider ]) } + // A timeout may have fired while the writes above were in flight. Every later write is + // skipped so an abandoned activation body cannot clobber a queued rollback's restoration. + if (signal?.aborted) return + const { mode } = await this.getState() + if (signal?.aborted) return + if (id && persistModeConfig) { await this.providerSettingsManager.setModeConfig(mode, id) } + if (signal?.aborted) return + // Change the provider for the current task. this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true, skipCurrentTaskRebuild }) @@ -2455,6 +2615,8 @@ export class ClineProvider await this.persistStickyProviderProfileToCurrentTask(name, { skipCurrentTaskRebuild }) } + if (signal?.aborted) return + if (!skipCurrentTaskRebuild) { await this.postStateToWebview() } diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index c2aec90dcd..c291acf5fd 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -745,6 +745,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await update const next = provider.activateProviderProfile({ name: "other-profile" }) + // The model update was not aborted on timeout, so its remaining writes still hold the + // queue: letting the next mutation start would let those writes land after it. await vi.advanceTimersByTimeAsync(0) expect(manager.activateProfile).not.toHaveBeenCalled() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 8849e35cd5..871d2aae24 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3855,7 +3855,10 @@ describe("ClineProvider", () => { vi.useFakeTimers() try { - const { profiles, managerState } = mockProviderSettingsManager() + const { profiles, managerState, modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) let resolveActivation!: () => void const pendingActivation = new Promise((resolve) => { resolveActivation = resolve @@ -3873,6 +3876,7 @@ describe("ClineProvider", () => { const deletion = provider.deleteProviderProfile(currentProfile) const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + resolveActivation() await deletionResult // The timeout path restores the purged profile instead of leaving ContextProxy @@ -3895,6 +3899,55 @@ describe("ClineProvider", () => { await vi.advanceTimersByTimeAsync(0) expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") expect(managerState.currentApiConfigName).toBe("current-config") + + // The rollback also re-applies the mode bindings the purge dropped, in both stores, + // and the abandoned body's tail mirror/postState never lands after the abort. + expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) + } finally { + vi.useRealTimers() + } + }) + + test("rollback waits for the abandoned delete body before restoring", async () => { + vi.useFakeTimers() + + try { + const { profiles, managerState } = mockProviderSettingsManager() + let resolveActivation!: () => void + const pendingActivation = new Promise((resolve) => { + resolveActivation = resolve + }) + // Hang the replacement activation past the timeout, then let it complete late. + // The rollback must not start until this body has fully settled, so the body's + // late writes cannot land after the restoration. + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + managerState.currentApiConfigName = otherProfile.name + await pendingActivation + return { ...otherProfile, openRouterModelId: "other-model" } + }) + + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + + // The rollback is gated on the abandoned body's settlement: while the body is + // still hung past the timeout, no restore write may be observable yet. + await vi.advanceTimersByTimeAsync(0) + expect(provider.providerSettingsManager.saveConfig).not.toHaveBeenCalled() + + resolveActivation() + await deletionResult + await vi.advanceTimersByTimeAsync(0) + + // Only after the body settled does the rollback restore the purged profile. + expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( + "current-config", + expect.objectContaining({ id: "current-id" }), + ) + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(managerState.currentApiConfigName).toBe("current-config") } finally { vi.useRealTimers() } @@ -3928,11 +3981,28 @@ describe("ClineProvider", () => { }) test("reconciles the manager's active pointer when the restore fails", async () => { - const { managerState } = mockProviderSettingsManager() + const { managerState, modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + // Seed the deleted profile's secret so the test can prove the fallback's write + // replaces the provider slice instead of merging over the restored one. The + // replacement (OpenRouter) profile carries `openRouterApiKey`, so a differently-named + // stale `apiKey` cannot be overwritten by a same-key merge. + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }) // The replacement activation fails, and so does the restore, so the fallback runs. + // The fallback profile is OpenRouter, so its activation carries `openRouterApiKey`. vi.mocked(provider.providerSettingsManager.activateProfile) .mockRejectedValueOnce(new Error("Failed to activate profile: storage error")) .mockRejectedValueOnce(new Error("Failed to restore profile: storage error")) + .mockImplementationOnce(async (params: { name: string } | { id: string }) => { + const resolvedName = "name" in params ? params.name : otherProfile.name + managerState.currentApiConfigName = resolvedName + return { ...otherProfile, openRouterModelId: "other-model", openRouterApiKey: "other-key" } + }) await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( "Failed to activate profile: storage error", @@ -3946,52 +4016,69 @@ describe("ClineProvider", () => { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "other-model", }) + // The fallback REPLACES the provider slice through setProviderSettings: the restored + // Anthropic profile's differently-named secret must not survive under the fallback name. + expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() + expect(provider.contextProxy.getValues().apiKey).toBeUndefined() + // The fallback also re-applies the purge's dropped mode bindings and mirrors the map, + // so both stores agree again. + expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) }) test("keeps a queued activation's write when the rollback settles late", async () => { - const { managerState } = mockProviderSettingsManager() - let resolveRestore!: () => void - const pendingRestore = new Promise((resolve) => { - resolveRestore = resolve - }) - - // Fail the replacement activation, then hang the restore's own `saveConfig` so the - // rollback is still in flight while the next mutation is queued behind it. - vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( - new Error("Failed to activate profile: storage error"), - ) - vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { - await pendingRestore - return "current-id" - }) + vi.useFakeTimers() - const deletion = provider.deleteProviderProfile(currentProfile) - const deletionResult = expect(deletion).rejects.toThrow("Failed to activate profile: storage error") - await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalled()) + try { + const { managerState } = mockProviderSettingsManager() + let resolveRestore!: () => void + const pendingRestore = new Promise((resolve) => { + resolveRestore = resolve + }) - // Queue a newer mutation while the rollback is still pending. - const queued = provider.activateProviderProfile({ name: "other-config" }) + // Fail the replacement activation, then hang the restore's own `saveConfig` past the + // mutation timeout, so the rollback's caller result rejects while its body is still + // in flight and the next mutation is already queued behind it. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { + await pendingRestore + return "current-id" + }) - // The rollback must still hold the queue, so the newer activation cannot write yet. - // Without the queued rollback this activation runs immediately and the late rollback - // then overwrites it. - await vi.waitFor(async () => { - expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledTimes(1) - }) - expect(managerState.currentApiConfigName).toBe("current-config") + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Failed to activate profile: storage error") + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalled()) + + // Queue a newer mutation while the rollback is still pending. + const queued = provider.activateProviderProfile({ name: "other-config" }) + + // The rollback's timeout fires while its body is hung: the queue advances (bounded + // hold), so the newer activation starts instead of waiting forever on the hung + // storage write. + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + await vi.waitFor(async () => { + expect(provider.providerSettingsManager.activateProfile).toHaveBeenCalledTimes(2) + }) + expect(managerState.currentApiConfigName).toBe("other-config") - // Let the rollback finish late; only then may the queued activation write. - resolveRestore() - await deletionResult - await queued + // Let the rollback's hung save settle late; the queued activation has already run + // and the rollback's aborted signal stops its remaining writes. + resolveRestore() + await deletionResult + await queued - // The rollback's late completion must not undo the queued activation's selection. - expect(managerState.currentApiConfigName).toBe("other-config") - expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") - expect(provider.contextProxy.getProviderSettings()).toMatchObject({ - apiProvider: providerIdentifiers.openrouter, - openRouterModelId: "other-model", - }) + // The rollback's late completion must not undo the queued activation's selection. + expect(managerState.currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }) + } finally { + vi.useRealTimers() + } }) test("restores the deleted profile's full settings when activation fails after its settings write", async () => { @@ -4057,6 +4144,42 @@ describe("ClineProvider", () => { expect(postedStates.length).toBeGreaterThanOrEqual(2) }) + test("does not pin the mutation queue on a rollback body that never settles", async () => { + vi.useFakeTimers() + + try { + const { managerState } = mockProviderSettingsManager() + // Fail the replacement activation so the rollback starts, then hang the rollback's + // own `saveConfig` forever: this is the manager lock held by a never-settling + // SecretStorage write, the one case a queue-level escape hatch has to cover. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce( + () => new Promise(() => {}), + ) + + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Failed to activate profile: storage error") + await vi.advanceTimersByTimeAsync(0) + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalled()) + + // Queue a newer mutation behind the hung rollback. + const queued = provider.activateProviderProfile({ name: "other-config" }) + + // The rollback's timeout fires and aborts it, so the queue advances instead of + // waiting on the hung storage call forever. + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + await vi.waitFor(() => expect(managerState.currentApiConfigName).toBe("other-config")) + + await deletionResult + await queued + expect(managerState.currentApiConfigName).toBe("other-config") + } finally { + vi.useRealTimers() + } + }) + test("re-applies the mode bindings when the deletion rolls back", async () => { const { modeApiConfigs } = mockProviderSettingsManager() modeApiConfigs.code = currentProfile.id @@ -4078,42 +4201,127 @@ describe("ClineProvider", () => { expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) }) - test("does not purge an inactive profile after the caller already timed out", async () => { + test("restore abort at the mode re-apply still reconciles the mode map in both stores", async () => { vi.useFakeTimers() try { - const { profiles } = mockProviderSettingsManager() - let resolvePurge!: () => void - const pendingPurge = new Promise((resolve) => { - resolvePurge = resolve + const { modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // The replacement activation fails fast so the rollback starts. The restore's + // first `setModeConfig` re-apply is then hung past the restore timeout, so the + // abort fires while the loop is in flight and the per-write fence drops the + // remaining re-apply. + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + let releaseModeWrite!: () => void + const pendingModeWrite = new Promise((resolve) => { + releaseModeWrite = resolve + }) + vi.mocked(provider.providerSettingsManager.setModeConfig).mockImplementationOnce(async () => { + await pendingModeWrite }) - // Stall the inactive purge past the mutation timeout, then let it complete. - vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce(async () => { - await pendingPurge - const index = profiles.findIndex((profile) => profile.name === otherProfile.name) - profiles.splice(index, 1) - return { removedModeConfigs: {} } + + const deletion = provider.deleteProviderProfile(currentProfile) + const deletionResult = expect(deletion).rejects.toThrow("Failed to activate profile: storage error") + await vi.waitFor(() => expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalled()) + + // Fire the restore's timeout while the re-apply is hung: the abort flag is set. + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + releaseModeWrite() + await deletionResult + await vi.advanceTimersByTimeAsync(0) + + // The restore was aborted at the re-apply fence, so the fallback reconcile ran: it + // points both stores at the remaining profile and re-applies the purge's dropped + // bindings atomically, so the mode map agrees in both stores. + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", }) + expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) + } finally { + vi.useRealTimers() + } + }) - const setValueSpy = vi.spyOn(provider.contextProxy, "setValue") - const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") + test("undoes an inactive purge that committed after the caller already timed out", async () => { + vi.useFakeTimers() + + try { + const { profiles, modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = otherProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // The purge splices for real, so "the profile survived" is asserted against the + // manager's own profile list rather than against a spy. + const deleteConfigCalls: string[] = [] + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementation(async (name) => { + deleteConfigCalls.push(name) + const deletedId = profiles.find((profile) => profile.name === name)?.id + const index = profiles.findIndex((profile) => profile.name === name) + if (index !== -1) { + profiles.splice(index, 1) + } + + const removedModeConfigs: Record = {} + for (const [mode, configId] of Object.entries(modeApiConfigs)) { + if (configId === deletedId) { + removedModeConfigs[mode] = configId + delete modeApiConfigs[mode] + } + } + + return { removedModeConfigs } + }) + + // Hold the purge past the mutation window: the caller is told the deletion timed out + // while the manager lock is still held, which is exactly the ordering that lets a + // purge commit after the failure was reported. + let releasePurge!: () => void + const purgeGate = new Promise((resolve) => { + releasePurge = resolve + }) + const realDeleteConfig = provider.providerSettingsManager.deleteConfig + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce(async (name) => { + await purgeGate + return realDeleteConfig(name) + }) const deletion = provider.deleteProviderProfile(otherProfile) + // Attach the rejection expectation before the timeout fires: the promise rejects with + // the timeout error, and an unattached handler would surface as an unhandled rejection. const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") - await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + // Cross the mutation window: the caller result is rejected and the abort fires while + // the purge is still holding the manager lock. + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + expect(deleteConfigCalls).toEqual([]) + + // The purge now commits, after the caller was already told the deletion timed out. + // The delete body sees the aborted signal and compensates instead of returning with + // the profile gone. + releasePurge() + for (let i = 0; i < 50; i++) { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(0) + } + await deletionResult - // The caller was told the deletion failed, so the stale snapshot must not be written - // over whatever a newer mutation has since stored. Clear the earlier calls (setup and - // the purge attempt itself) so only the post-abort writes are observed; this branch's - // only write is `listApiConfigMeta`. - setValueSpy.mockClear() - setValuesSpy.mockClear() - resolvePurge() - await vi.advanceTimersByTimeAsync(0) - expect(setValueSpy).not.toHaveBeenCalled() - expect(setValuesSpy).not.toHaveBeenCalled() + // The purge ran, and the compensation restored the profile under its original id, + // with the mode binding the purge dropped back in the manager's map. + expect(deleteConfigCalls).toEqual(["other-config"]) + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + expect(profiles.find(({ name }) => name === "other-config")?.id).toBe(otherProfile.id) + expect(modeApiConfigs).toEqual({ code: otherProfile.id }) + // ContextProxy still lists both profiles, so the two stores agree. + expect(provider.getProviderProfileEntries()).toEqual([currentProfile, otherProfile]) } finally { vi.useRealTimers() } diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index a8b587c9f1..6d0bd3999f 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2524,20 +2524,29 @@ export const webviewMessageHandler = async ( try { await provider.providerSettingsManager.deleteConfig(oldName) await provider.activateProviderProfile({ name: newName }) - - // The purge drops the mode bindings that pointed at the deleted profile, and the - // activation only rebinds the current mode. Mirror the manager's map so the - // webview state cannot keep pointing a mode at a profile that no longer exists. - await provider.contextProxy.setValue( - "modeApiConfigs", - await provider.providerSettingsManager.getModeApiConfigs(), - ) } catch (error) { provider.log( `Error delete api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`, ) vscode.window.showErrorMessage(t("common:errors.delete_api_config")) + } finally { + // The purge drops the mode bindings that pointed at the deleted profile, and the + // activation only rebinds the current mode. Mirror the manager's map in a finally + // so the webview state cannot keep pointing a mode at a profile that no longer + // exists, even when the activation above failed after the purge. + try { + await provider.contextProxy.setValue( + "modeApiConfigs", + await provider.providerSettingsManager.getModeApiConfigs(), + ) + } catch (mirrorError) { + provider.log( + `Error mirroring mode configs after delete api configuration: ${ + mirrorError instanceof Error ? mirrorError.message : String(mirrorError) + }`, + ) + } } } break From 1b2b0084e45d5a6b05f83f8b08c07e5be680293c Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 20:53:49 +0900 Subject: [PATCH 13/17] fix(core): keep a provider's own secrets when a partial profile write omits them What: - ContextProxy.setProviderSettings derives secret-key ownership from providerSettingsSchemaDiscriminated and treats a cached secret as stale only when the provider named by the write does not own it. - Adds three ContextProxy specs: the partial-write shape, the empty-write semantics, and per-key (not all-or-nothing) application of the rule. Why: The 257eb9c7e stale-secret purge classified a secret as stale whenever the incoming slice omitted it. Partial writes legitimately omit secrets: api.getConfiguration() strips them, so the e2e profile test's setConfiguration(priorConfiguration) then setActiveProfile("default") sequence rewrote the default profile from a secret-less snapshot and deleted openRouterApiKey from SecretStorage. Every task rehydrated afterwards ran without an API key and hung until its 30 s timeout, which is the deterministic 9/3 mocked-e2e failure. Impact: Fixes the deterministic e2e-mock failure. Preserves the secret-purge security intent: a genuine cross-provider switch still deletes the old provider's differently-named secret, because that key is owned by a different provider. Empty writes and the codebase-index carve-out are unchanged. --- src/core/config/ContextProxy.ts | 65 ++++++++++++++++++- .../config/__tests__/ContextProxy.spec.ts | 63 ++++++++++++++++++ 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 4984c34929..59689ca53c 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -13,6 +13,7 @@ import { type GlobalState, type RooCodeSettings, providerSettingsSchema, + providerSettingsSchemaDiscriminated, globalSettingsSchema, codebaseIndexProviderSchema, isSecretStateKey, @@ -39,6 +40,51 @@ export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.in // delete the user's index credentials on every profile switch. const FLAT_ONLY_PROVIDER_SECRET_KEYS = new Set(Object.keys(codebaseIndexProviderSchema.shape)) +// Which provider owns each secret key, read from the same discriminated union that +// `saveConfig` parses profiles with: a provider branch declares the keys its own +// profile blob carries, so ownership is exactly "this key appears in this branch". +// Every provider-owned secret key has exactly one owner (verified across all 37 +// branches); the codebase-index credentials have none, which is why they are the +// FLAT_ONLY carve-out above. +const PROVIDER_SECRET_KEY_OWNERS = new Map>() + +for (const branch of providerSettingsSchemaDiscriminated.options) { + const literal = branch.shape.apiProvider + + // The union's last branch is the `apiProvider: undefined` default, which owns nothing. + if (!("value" in literal) || typeof literal.value !== "string") continue + + for (const key of Object.keys(branch.shape)) { + if (!isSecretStateKey(key)) continue + + const owners = PROVIDER_SECRET_KEY_OWNERS.get(key) ?? new Set() + owners.add(literal.value) + PROVIDER_SECRET_KEY_OWNERS.set(key, owners) + } +} + +/** + * Whether the provider named by a write is the one that owns `key`. + * + * A partial write can legitimately omit a secret the writing provider still uses: + * `api.getConfiguration()` strips secrets (`src/extension/api.ts`), so a + * save-then-reactivate round trip rewrites a profile from a snapshot that has no + * secret in it. Treating that omission as "this profile uses none of these" deleted + * the live profile's credentials from SecretStorage and broke every task rehydrated + * afterwards. Ownership answers the question the omission cannot. + * + * Returns `undefined` when ownership is unknown (no provider named, or a retired + * provider, whose branch is not in the union), so callers keep the old + * presence-based semantics instead of guessing. + */ +const providerOwnsSecretKey = (apiProvider: unknown, key: string): boolean | undefined => { + if (typeof apiProvider !== "string" || !isProviderName(apiProvider)) { + return undefined + } + + return PROVIDER_SECRET_KEY_OWNERS.get(key)?.has(apiProvider) ?? false +} + const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, @@ -534,8 +580,23 @@ export class ContextProxy { // presence check. Without this a deleted profile's differently-named secrets // (`openRouterApiKey` when the replacement uses `apiKey`) survive the switch and // keep being merged back by `getValues`. - const isStale = (key: string) => - isSecretStateKey(key) ? !!this.secretCache[key as SecretStateKey] : !!this.stateCache[key as GlobalStateKey] + // + // A secret the writing provider still owns is not stale, even when the write omits + // it: partial writes are normal (`api.getConfiguration()` strips secrets, so a + // save-then-reactivate round trip rewrites a profile from a secret-less snapshot). + // Purging on omission alone deleted the live profile's credentials, and every task + // rehydrated afterwards ran without an API key. + const isStale = (key: string) => { + if (!isSecretStateKey(key)) { + return !!this.stateCache[key as GlobalStateKey] + } + + if (!this.secretCache[key as SecretStateKey]) { + return false + } + + return providerOwnsSecretKey(values.apiProvider, key) !== true + } await this.setValues({ ...PROVIDER_SETTINGS_KEYS.filter((key) => !FLAT_ONLY_PROVIDER_SECRET_KEYS.has(key)) diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 5824c010ff..cf14414ef7 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -430,6 +430,31 @@ describe("ContextProxy", () => { expect(mockSecrets.store).toHaveBeenCalledWith("openRouterApiKey", "openrouter-secret") }) + it("keeps a secret the writing provider still owns when a partial write omits it", async () => { + // The e2e/API restore shape: `api.getConfiguration()` strips secrets, so a + // save-then-reactivate round trip rewrites the active profile from a snapshot that + // carries `apiProvider` plus the non-secret settings and no secret at all. Purging on + // omission alone deleted `openRouterApiKey` from SecretStorage, and every task + // rehydrated afterwards hung without an API key. + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "live-secret", + openRouterModelId: "openai/gpt-4.1", + }) + expect(proxy.getSecret("openRouterApiKey")).toBe("live-secret") + + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4.1", + }) + + // The provider named by the write is the one that owns `openRouterApiKey`, so the + // omission is a partial write, not a declaration that the profile uses no secret. + expect(proxy.getSecret("openRouterApiKey")).toBe("live-secret") + expect(proxy.getValues().openRouterApiKey).toBe("live-secret") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterApiKey") + }) + it("keeps the global image-generation secret and codebase-index credentials", async () => { // Neither is owned by a provider profile: the image key is a global secret, and the // index credentials are written flat by the index settings handler. @@ -446,6 +471,44 @@ describe("ContextProxy", () => { expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterImageApiKey") expect(mockSecrets.delete).not.toHaveBeenCalledWith("codeIndexOpenAiKey") }) + + it("still purges every secret when the write names no provider", async () => { + // Ownership is unknown without a provider, so the presence-based clear-out keeps its + // old meaning: an empty write is a full reset, not a partial one. + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "live-secret", + }) + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "anthropic-secret", + }) + + await proxy.setProviderSettings({}) + + expect(proxy.getSecret("openRouterApiKey")).toBeUndefined() + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("openRouterApiKey") + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + }) + + it("applies the ownership rule per key, not to the whole secret set", async () => { + // A partial write naming OpenRouter omits OpenRouter's own secret while a different + // provider's secret is still cached. The owned key survives and the foreign one is + // purged, so the rule is decided per key rather than as an all-or-nothing wipe. + await proxy.storeSecret("apiKey", "anthropic-secret") + await proxy.storeSecret("openRouterApiKey", "openrouter-secret") + + await proxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4.1", + }) + + expect(proxy.getSecret("openRouterApiKey")).toBe("openrouter-secret") + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterApiKey") + }) }) describe("resetAllState", () => { From 99a95bc3494ec7601bbbe0b4dcf3c7c459449f3b Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 22:02:13 +0900 Subject: [PATCH 14/17] fix(core): purge deleted profile secrets explicitly and route the webview delete through the queue What: - Route the webview deleteApiConfiguration handler through deleteProviderProfile so it shares the mutation queue, the rollback, and the secret purge the extension API path already had. - Resolve the secret-ownership discriminator from the manager instead of the incoming slice's apiProvider: setProviderSettings takes an optional expectedProvider, and every activation/persist path passes the provider it read from the stored blob. - Purge a deleted profile's own secret keys explicitly on both delete branches, instead of relying on a later write's ownership inference. Keys the replacement profile's own blob declares are kept, so a same-provider succession cannot delete the replacement's live credential. - Add a packages/types invariant test for one owner per secret key, fallback tests for retired/unknown providers, and regression tests for each fix. Why: The webview twin deleted outside the mutation queue with no rollback, so a queued upsert could resurrect the profile and a failed activation left a ghost entry. The ownership discriminator trusted a caller-supplied field, so a crafted partial write could name the owner of a secret it wanted preserved. The inactive delete branch ran no replacement write, so its clear-out never fired and the deleted profile's credentials stayed in SecretStorage, and a same-provider succession preserved the deleted key by design. Impact: A deleted profile's credentials are removed on every delete path. Partial writes still preserve a provider's own secrets, and the replacement's own credential survives a same-provider succession. The global image-generation key and the codebase-index credentials are structurally excluded from the purge. --- .../src/__tests__/provider-settings.test.ts | 52 ++++++ src/core/config/ContextProxy.ts | 61 ++++++- .../config/__tests__/ContextProxy.spec.ts | 65 +++++++ src/core/webview/ClineProvider.ts | 110 +++++++++++- .../ClineProvider.apiHandlerRebuild.spec.ts | 30 ++++ .../webview/__tests__/ClineProvider.spec.ts | 164 ++++++++++++++++++ src/core/webview/webviewMessageHandler.ts | 28 +-- 7 files changed, 488 insertions(+), 22 deletions(-) diff --git a/packages/types/src/__tests__/provider-settings.test.ts b/packages/types/src/__tests__/provider-settings.test.ts index c5a79002a4..56f0172bb9 100644 --- a/packages/types/src/__tests__/provider-settings.test.ts +++ b/packages/types/src/__tests__/provider-settings.test.ts @@ -1,4 +1,6 @@ import { ANTHROPIC_API_PROTOCOL, OPENAI_API_PROTOCOL, providerIdentifiers, providerNames } from "../index.js" +import { codebaseIndexProviderSchema } from "../codebase-index.js" +import { isSecretStateKey } from "../global-settings.js" import { getApiProtocol, OPEN_AI_CODEX_SERVICE_TIER_KEY, @@ -21,6 +23,56 @@ describe("provider settings discriminated union", () => { it.each(providerNames)("accepts the %s provider branch", (apiProvider) => { expect(providerSettingsSchemaDiscriminated.safeParse({ apiProvider }).success).toBe(true) }) + + it("assigns every provider-owned secret key to exactly one provider", () => { + // Secret-key ownership is derived from this union and decides which credentials survive a + // profile write, so a key declared by two branches would make that decision ambiguous: a + // write naming either provider would preserve the other's secret. The invariant is one + // owner per key, and it has to hold for the whole union, not just today's branches. + const owners = new Map() + + for (const branch of providerSettingsSchemaDiscriminated.options) { + const literal = branch.shape.apiProvider + + // The union's last branch is the `apiProvider: undefined` default, which owns nothing. + if (!("value" in literal) || typeof literal.value !== "string") continue + + for (const key of Object.keys(branch.shape)) { + if (!isSecretStateKey(key)) continue + + owners.set(key, [...(owners.get(key) ?? []), literal.value]) + } + } + + const shared = [...owners].filter(([, providers]) => providers.length > 1) + + expect(shared).toEqual([]) + // Guard against the map silently going empty (e.g. a renamed `apiProvider` field), which + // would make the assertion above pass while proving nothing. + expect(owners.size).toBeGreaterThan(0) + }) + + it("leaves the flat codebase-index credentials owned by no provider", () => { + // The index credentials are part of `ProviderSettings` but belong to no profile: they are + // written flat and every profile's discriminated parse strips them. A provider branch that + // started declaring one would make a profile switch delete the user's index credentials. + const ownedByProvider = new Set() + + for (const branch of providerSettingsSchemaDiscriminated.options) { + const literal = branch.shape.apiProvider + + if (!("value" in literal) || typeof literal.value !== "string") continue + + for (const key of Object.keys(branch.shape)) { + ownedByProvider.add(key) + } + } + + const indexSecretKeys = Object.keys(codebaseIndexProviderSchema.shape).filter((key) => isSecretStateKey(key)) + + expect(indexSecretKeys.length).toBeGreaterThan(0) + expect(indexSecretKeys.filter((key) => ownedByProvider.has(key))).toEqual([]) + }) }) describe("OpenAI-compatible extra body settings", () => { diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 59689ca53c..b587dfa4f1 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -40,6 +40,13 @@ export const isPassThroughStateKey = (key: string) => PASS_THROUGH_STATE_KEYS.in // delete the user's index credentials on every profile switch. const FLAT_ONLY_PROVIDER_SECRET_KEYS = new Set(Object.keys(codebaseIndexProviderSchema.shape)) +// The secret keys a provider profile can own: `PROVIDER_SETTINGS_KEYS ∩ SECRET_STATE_KEYS`, +// minus the FLAT_ONLY carve-out. The global image-generation key is in `SECRET_STATE_KEYS` but +// not in `ProviderSettings`, so it is excluded here exactly as the clear-out loop excludes it. +const PROVIDER_SECRET_KEYS = new Set( + PROVIDER_SETTINGS_KEYS.filter((key) => isSecretStateKey(key) && !FLAT_ONLY_PROVIDER_SECRET_KEYS.has(key)), +) + // Which provider owns each secret key, read from the same discriminated union that // `saveConfig` parses profiles with: a provider branch declares the keys its own // profile blob carries, so ownership is exactly "this key appears in this branch". @@ -85,6 +92,20 @@ const providerOwnsSecretKey = (apiProvider: unknown, key: string): boolean | und return PROVIDER_SECRET_KEY_OWNERS.get(key)?.has(apiProvider) ?? false } +/** + * The secret keys a stored profile blob declares. + * + * A profile blob is the record of which credentials that profile uses: `saveConfig`'s + * discriminated parse keeps only the branch's own keys, so the keys present in the blob + * are exactly the ones the profile owns. Used to purge a deleted profile's credentials + * explicitly instead of inferring them from a later write. + * + * Membership is `PROVIDER_SETTINGS_KEYS ∩ SECRET_STATE_KEYS`, the same set the clear-out + * loop iterates: it excludes the global image-generation key, which no profile blob owns. + */ +export const getProfileSecretKeys = (config: Record): string[] => + Object.keys(config).filter((key) => PROVIDER_SECRET_KEYS.has(key)) + const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, @@ -560,7 +581,18 @@ export class ContextProxy { return sanitizedValues } - public async setProviderSettings(values: ProviderSettings) { + /** + * Write a provider slice, clearing out the settings it no longer carries. + * + * `expectedProvider` is the provider the caller resolved from the manager for the profile + * being written. It is the ownership discriminator, and it must not be read off `values`: + * `values.apiProvider` is whatever the caller put in the slice, so a crafted partial write + * could name the owner of a secret it wants preserved and keep a credential that belongs to + * a different profile. Callers that activate or persist a stored profile pass the provider + * from the manager blob; callers with no manager record (import, tests) omit it and fall + * back to the slice's own field. + */ + public async setProviderSettings(values: ProviderSettings, expectedProvider?: string) { // Explicitly clear out any old API configuration values before that // might not be present in the new configuration. // If a value is not present in the new configuration, then it is assumed @@ -586,6 +618,8 @@ export class ContextProxy { // save-then-reactivate round trip rewrites a profile from a secret-less snapshot). // Purging on omission alone deleted the live profile's credentials, and every task // rehydrated afterwards ran without an API key. + const owner = expectedProvider ?? values.apiProvider + const isStale = (key: string) => { if (!isSecretStateKey(key)) { return !!this.stateCache[key as GlobalStateKey] @@ -595,7 +629,7 @@ export class ContextProxy { return false } - return providerOwnsSecretKey(values.apiProvider, key) !== true + return providerOwnsSecretKey(owner, key) !== true } await this.setValues({ @@ -606,6 +640,29 @@ export class ContextProxy { }) } + /** + * Delete the named secrets from the cache and from SecretStorage. + * + * This is the explicit form of the clear-out above: the caller names the keys, so the + * decision does not depend on what a later write happens to omit. A profile delete uses it + * to remove exactly the credentials the deleted profile's own blob declared, which is the + * only way to cover the inactive branch (no replacement write runs) and a same-provider + * succession (ownership would preserve the deleted profile's key by design). + * + * Keys outside `PROVIDER_SETTINGS_KEYS ∩ SECRET_STATE_KEYS` are ignored, as are the flat + * codebase-index credentials: the global image-generation secret and the index keys belong + * to no profile, so a profile delete must not take them with it. + */ + public async purgeProviderSecrets(keys: Iterable): Promise { + const purgeable = [...keys].filter((key) => PROVIDER_SECRET_KEYS.has(key)) + + if (purgeable.length === 0) { + return + } + + await this.setValues(purgeable.reduce((acc, key) => ({ ...acc, [key]: undefined }), {} as RooCodeSettings)) + } + /** * RooCodeSettings */ diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index cf14414ef7..5d3cfaa9aa 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -509,6 +509,71 @@ describe("ContextProxy", () => { expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterApiKey") }) + + it("resolves ownership from the provider the caller resolved, not the slice's own field", async () => { + // The slice names OpenRouter, which owns `openRouterApiKey`, but the profile actually + // being written is Anthropic. Trusting the slice would let a crafted partial write name + // the owner of a secret it wants preserved and keep a credential that belongs to a + // different profile; the caller-resolved provider is the authority. + await proxy.storeSecret("openRouterApiKey", "foreign-secret") + await proxy.storeSecret("apiKey", "anthropic-secret") + + await proxy.setProviderSettings( + { apiProvider: providerIdentifiers.openrouter, openRouterModelId: "openai/gpt-4.1" }, + providerIdentifiers.anthropic, + ) + + // Anthropic owns `apiKey`, so it survives the partial write; OpenRouter's key does not, + // even though the slice named OpenRouter. + expect(proxy.getSecret("apiKey")).toBe("anthropic-secret") + expect(proxy.getSecret("openRouterApiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("openRouterApiKey") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("apiKey") + }) + + it("purges exactly the named secrets, leaving global and flat-only keys alone", async () => { + // The explicit purge is the delete path's tool: a profile's own keys go, but the global + // image-generation secret and the flat codebase-index credentials belong to no profile. + await proxy.storeSecret("apiKey", "anthropic-secret") + await proxy.storeSecret("openRouterImageApiKey", "image-secret") + await proxy.storeSecret("codeIndexOpenAiKey", "index-secret") + + await proxy.purgeProviderSecrets(["apiKey", "openRouterImageApiKey", "codeIndexOpenAiKey"]) + + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + expect(proxy.getSecret("openRouterImageApiKey")).toBe("image-secret") + expect(proxy.getSecret("codeIndexOpenAiKey")).toBe("index-secret") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("openRouterImageApiKey") + expect(mockSecrets.delete).not.toHaveBeenCalledWith("codeIndexOpenAiKey") + }) + + it("falls back to presence-based clearing for a retired provider", async () => { + // A retired provider has no branch in the discriminated union, so ownership is unknown. + // The fallback must stay the pre-ownership behavior: an omitted secret is stale and is + // cleared. Guessing "owned" here would leak a retired profile's credential forever. + await proxy.storeSecret("apiKey", "retired-secret") + await proxy.storeSecret("openRouterApiKey", "stale-secret") + + await proxy.setProviderSettings({ apiProvider: retiredProviderIdentifiers.groq }) + + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(proxy.getSecret("openRouterApiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + expect(mockSecrets.delete).toHaveBeenCalledWith("openRouterApiKey") + }) + + it("falls back to presence-based clearing for an unknown provider string", async () => { + // Same fallback for a provider name that is neither active nor retired (a removed + // provider still in storage): ownership is unknown, so the conservative clear-out runs + // rather than preserving a credential no live provider claims. + await proxy.storeSecret("apiKey", "unknown-provider-secret") + + await proxy.setProviderSettings({ apiProvider: "not-a-real-provider" as never }) + + expect(proxy.getSecret("apiKey")).toBeUndefined() + expect(mockSecrets.delete).toHaveBeenCalledWith("apiKey") + }) }) describe("resetAllState", () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6032863946..0a8899a03b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -112,7 +112,7 @@ import { t } from "../../i18n" import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" -import { ContextProxy } from "../config/ContextProxy" +import { ContextProxy, getProfileSecretKeys } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" @@ -2128,6 +2128,17 @@ export class ClineProvider if (signal.aborted) return id + // Read the stored entries once: they are the manager's authoritative view of each + // profile, including the provider `saveConfig` pinned into the blob. The ownership + // discriminator below must not be taken from `providerSettings.apiProvider`, which is + // the caller's own field and can disagree with what was stored (a legacy router config + // is downgraded to no provider at all). + const storedProfiles = await this.providerSettingsManager.listConfig() + + if (signal.aborted) return id + + const storedProvider = storedProfiles.find((profile) => profile.name === name)?.apiProvider + // These promises do the following: // 1. Adds or updates the list of provider profiles. // 2. Sets the current provider profile. @@ -2140,10 +2151,10 @@ export class ClineProvider // I left the original implementation in just to be safe. // `allSettled` so a caller that rolls back after a failure never races a write still in flight. const activationResults = await Promise.allSettled([ - this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()), + this.updateGlobalState("listApiConfigMeta", storedProfiles), this.updateGlobalState("currentApiConfigName", name), this.providerSettingsManager.setModeConfig(mode, id), - this.contextProxy.setProviderSettings(providerSettings), + this.contextProxy.setProviderSettings(providerSettings, storedProvider), ]) const failedActivation = activationResults.find( (result): result is PromiseRejectedResult => result.status === "rejected", @@ -2245,6 +2256,13 @@ export class ClineProvider return } + // The delete is committed, so the profile's credentials go with it. This branch + // activates no replacement, so no `setProviderSettings` write runs the ownership + // clear-out: without an explicit purge the deleted profile's secrets stay in the + // cache and in SecretStorage until some unrelated future write happens to name a + // provider that does not own them, which may never happen. + await this.purgeDeletedProfileSecrets(undoConfig) + await this.contextProxy.setValues({ listApiConfigMeta: this.getProviderProfileEntries().filter( ({ name }) => name !== profileToDelete.name, @@ -2299,6 +2317,20 @@ export class ClineProvider return } + // The activation's clear-out is ownership-based, which deliberately preserves a + // secret the replacement provider owns. When the replacement is the same provider as + // the deleted profile, that rule keeps the deleted profile's credential alive under + // the replacement's name. The delete is explicit intent, so the deleted profile's own + // keys are removed here, after the activation has written the replacement's slice. + // + // A key the replacement's own blob declares is excluded: the replacement wrote its + // value through the activation above, and purging by name would delete the live + // profile's credential, which is the round-4 data-loss shape. + await this.purgeDeletedProfileSecrets( + deletedProfileConfig, + await this.getReplacementSecretKeys(profileToActivate), + ) + // The activation above only rebinds the *current* mode, so every other mode that // pointed at the deleted profile would keep a dangling id and silently no-op on the // next switch. The manager dropped those bindings with the purge; mirror its map into @@ -2331,6 +2363,63 @@ export class ClineProvider } } + /** + * The secret keys the profile that replaced a deleted one declares. + * + * Used to keep the delete purge from removing a credential the replacement wrote itself. A + * manager that cannot produce the profile (it was just activated, so it should exist) yields an + * empty set: the purge then falls back to removing every key the deleted profile declared, which + * is the confidentiality-safe direction. + */ + private async getReplacementSecretKeys(name: string): Promise> { + try { + const { name: _name, ...config } = await this.providerSettingsManager.getProfile({ name }) + return new Set(getProfileSecretKeys(config)) + } catch { + return new Set() + } + } + + /** + * Delete the credentials a purged profile's own blob declared. + * + * The ownership clear-out in `setProviderSettings` infers staleness from a *later* write, so + * it cannot cover two cases: the inactive branch runs no replacement write at all, and a + * same-provider succession is exactly the case ownership preserves. A delete is explicit + * intent, so the deleted profile's keys are removed by name here instead of being inferred. + * + * `keep` names the keys the replacement profile declares: those carry the replacement's own + * value, so removing them would delete a live credential. + * + * Best-effort: the profile is already gone from the manager, so a failure here must not turn + * a successful delete into an error. The keys are logged by name only, never by value. + */ + private async purgeDeletedProfileSecrets( + config: (ProviderSettingsWithId & { name: string }) | undefined, + keep: Set = new Set(), + ): Promise { + if (!config) { + return + } + + const secretKeys = getProfileSecretKeys(config).filter((key) => !keep.has(key)) + + if (secretKeys.length === 0) { + return + } + + try { + await this.contextProxy.purgeProviderSecrets(secretKeys) + this.log(`Purged ${secretKeys.length} secret(s) of the deleted profile '${config.name}'`) + } catch (error) { + this.log( + `Failed to purge the secrets of the deleted profile '${config.name}': ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + /** * Undo the purge of an active profile whose replacement activation failed, so the manager and * ContextProxy keep naming the same profile. Best-effort: a failed restore is logged rather @@ -2452,9 +2541,11 @@ export class ClineProvider // have left the replacement's provider settings under the deleted name. `id` is manager // metadata, not a setting, so it must not reach `ContextProxy`. `apiProvider` belongs to // this slice, so the snapshot below deliberately does not carry it: writing it again - // would overwrite the restored provider with the pre-delete pointer. + // would overwrite the restored provider with the pre-delete pointer. It is still passed + // as the ownership discriminator, because the profile being restored is the one whose + // own secrets must survive this write. const { id: _id, ...settings } = providerSettings - await this.contextProxy.setProviderSettings(settings) + await this.contextProxy.setProviderSettings(settings, config.apiProvider) if (signal.aborted) return false @@ -2502,7 +2593,7 @@ export class ClineProvider ...fallbackSettings } = await this.providerSettingsManager.activateProfile({ name: fallback }) - await this.contextProxy.setProviderSettings(fallbackSettings) + await this.contextProxy.setProviderSettings(fallbackSettings, fallbackSettings.apiProvider) await this.contextProxy.setValues({ currentApiConfigName: fallback, listApiConfigMeta: remainingProfiles, @@ -2584,11 +2675,14 @@ export class ClineProvider const skipCurrentTaskRebuild = options?.skipCurrentTaskRebuild ?? false if (!skipCurrentTaskRebuild) { - // See `upsertProviderProfile` for a description of what this is doing. + // See `upsertProviderProfile` for a description of what this is doing. The slice comes + // from the manager's own blob, so its `apiProvider` is the stored provider rather than a + // caller-supplied field; passing it explicitly keeps the ownership discriminator pinned + // to the profile being activated. await Promise.all([ this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()), this.contextProxy.setValue("currentApiConfigName", name), - this.contextProxy.setProviderSettings(providerSettings), + this.contextProxy.setProviderSettings(providerSettings, providerSettings.apiProvider), ]) } diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index c291acf5fd..9793be0b40 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -425,6 +425,36 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { // Should not call buildApiHandler when there's no task expect(buildApiHandlerMock).not.toHaveBeenCalled() }) + + test("discriminates secret ownership by the stored provider, not the slice's own field", async () => { + // The slice claims OpenRouter while the manager stores Anthropic for this name. The + // ownership rule decides which cached secrets survive the write, so reading the provider + // off the caller's slice would let a crafted partial write preserve a foreign credential. + // A cached OpenRouter secret must be purged even though the slice names OpenRouter. + provider["providerSettingsManager"].listConfig = vi.fn().mockResolvedValue([ + { + name: "test-config", + id: "test-id", + apiProvider: providerIdentifiers.anthropic, + }, + ]) + await provider.contextProxy.storeSecret("openRouterApiKey", "foreign-secret") + await provider.contextProxy.storeSecret("apiKey", "stored-secret") + + await provider.upsertProviderProfile( + "test-config", + { + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4", + }, + true, + ) + + // The stored provider is Anthropic, so its own `apiKey` survives the partial write and + // the OpenRouter key does not. + expect(provider.contextProxy.getSecret("apiKey")).toBe("stored-secret") + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBeUndefined() + }) }) describe("updateProfileModel", () => { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 871d2aae24..f885dec5db 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3609,6 +3609,12 @@ describe("ClineProvider", () => { ) vi.spyOn(provider, "activateProviderProfile").mockResolvedValue(undefined) + // The handler resolves the profile from ContextProxy metadata before deleting, so the + // entry it deletes has to be listed there, exactly as the webview saw it. + await provider.contextProxy.setValue("listApiConfigMeta", [ + { name: "deleted-config", id: "deleted-id", apiProvider: providerIdentifiers.anthropic }, + survivingProfile, + ]) await provider.contextProxy.setValue("modeApiConfigs", { code: "deleted-id", ask: "other-id" }) await messageHandler({ type: "deleteApiConfiguration", text: "deleted-config" }) @@ -3617,6 +3623,62 @@ describe("ClineProvider", () => { // deleted id silently no-ops on the next switch instead of moving to a real profile. expect(provider.contextProxy.getValues().modeApiConfigs).toEqual({ ask: "other-id" }) }) + + test("routes the delete through the queued provider-profile policy", async () => { + // The twin used to call `deleteConfig` + `activateProviderProfile` itself, outside the + // mutation queue and without a rollback. Routing it through `deleteProviderProfile` is + // what gives the webview path the purge, the queue, and the compensation the API path + // already had. + await provider.resolveWebviewView(mockWebviewView) + const messageHandler = mockWebviewView.webview.onDidReceiveMessage.mock.calls[0][0] + + const deletedProfile: ProviderSettingsEntry = { + name: "deleted-config", + id: "deleted-id", + apiProvider: providerIdentifiers.anthropic, + } + + await provider.contextProxy.setValue("listApiConfigMeta", [deletedProfile]) + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + "answers.yes" as unknown as vscode.MessageItem, + ) + + const deleteProviderProfile = vi.spyOn(provider, "deleteProviderProfile").mockResolvedValue(undefined) + const rawDelete = vi.fn() + Object.assign(provider.providerSettingsManager, { deleteConfig: rawDelete }) + const activate = vi.spyOn(provider, "activateProviderProfile").mockResolvedValue(undefined) + + await messageHandler({ type: "deleteApiConfiguration", text: "deleted-config" }) + + // The entry handed to the policy method is the one the webview named, so the queued body + // reads the profile from the manager instead of trusting a webview-supplied snapshot. + expect(deleteProviderProfile).toHaveBeenCalledWith(deletedProfile) + // Neither raw step runs: they are the unqueued, un-rollback-able shape being replaced. + expect(rawDelete).not.toHaveBeenCalled() + expect(activate).not.toHaveBeenCalled() + }) + + test("does not delete anything when the named profile is not listed", async () => { + // `deleteProviderProfile` takes an entry, so the handler must resolve one first. A name the + // webview no longer has must not reach the manager as a raw name-based purge. + await provider.resolveWebviewView(mockWebviewView) + const messageHandler = mockWebviewView.webview.onDidReceiveMessage.mock.calls[0][0] + + await provider.contextProxy.setValue("listApiConfigMeta", []) + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + "answers.yes" as unknown as vscode.MessageItem, + ) + + const deleteProviderProfile = vi.spyOn(provider, "deleteProviderProfile").mockResolvedValue(undefined) + const rawDelete = vi.fn() + Object.assign(provider.providerSettingsManager, { deleteConfig: rawDelete }) + + await messageHandler({ type: "deleteApiConfiguration", text: "ghost-config" }) + + expect(deleteProviderProfile).not.toHaveBeenCalled() + expect(rawDelete).not.toHaveBeenCalled() + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.delete_api_config") + }) }) describe("deleteProviderProfile", () => { @@ -3726,6 +3788,108 @@ describe("ClineProvider", () => { expect(provider.providerSettingsManager.setModeConfig).not.toHaveBeenCalled() }) + test("purges an inactive profile's own secrets, which no replacement write would clear", async () => { + // The inactive branch activates no replacement, so the ownership clear-out in + // `setProviderSettings` never runs. Without an explicit purge the deleted profile's + // differently-named secret (`apiKey` here, while the active profile is OpenRouter) stays + // in the cache and in SecretStorage, and `getValues` keeps merging it back. + mockProviderSettingsManager() + + // The active profile is the OpenRouter one, so the Anthropic profile being deleted is + // inactive and carries the secret that must go. + await provider.contextProxy.setValue("currentApiConfigName", "other-config") + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "inactive-key", + }) + expect(provider.contextProxy.getSecret("apiKey")).toBe("inactive-key") + + await provider.deleteProviderProfile(currentProfile) + + expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() + expect(provider.contextProxy.getValues().apiKey).toBeUndefined() + expect(mockContext.secrets.delete).toHaveBeenCalledWith("apiKey") + // The surviving profile is untouched: this is a targeted purge of one profile's keys. + expect(provider.getProviderProfileEntries()).toEqual([otherProfile]) + }) + + test("purges the deleted profile's secret when the replacement is the same provider", async () => { + // Ownership deliberately preserves a secret the *writing* provider owns, so a + // same-provider succession (Anthropic -> Anthropic) keeps the deleted profile's `apiKey` + // alive under the replacement's name. A delete is explicit intent: the deleted profile's + // own keys go, whether or not the replacement shares its provider. + const { profiles } = mockProviderSettingsManager() + + // The replacement is Anthropic too, and its blob is secret-less: the normal shape of a + // profile written from an `api.getConfiguration()` snapshot, which strips secrets. The + // ownership rule reads that omission as a partial write and preserves `apiKey`, so only + // an explicit purge removes the deleted profile's credential. + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + return { ...otherProfile, apiProvider: providerIdentifiers.anthropic } + }) + vi.mocked(provider.providerSettingsManager.getProfile).mockImplementation(async (params) => { + const name = "name" in params ? params.name : "" + + if (name === "other-config") { + return { ...otherProfile, name: "other-config", apiProvider: providerIdentifiers.anthropic } + } + + return { ...currentProfile, name: "current-config", apiKey: "deleted-key" } + }) + + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "deleted-key", + }) + expect(provider.contextProxy.getSecret("apiKey")).toBe("deleted-key") + + await provider.deleteProviderProfile(currentProfile) + + // The deleted profile's credential is gone rather than surviving under the replacement. + expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() + expect(provider.contextProxy.getValues().apiKey).toBeUndefined() + expect(mockContext.secrets.delete).toHaveBeenCalledWith("apiKey") + expect(profiles.map(({ name }) => name)).toEqual(["other-config"]) + }) + + test("keeps the replacement's own secret when it declares the same key", async () => { + // Same-provider succession where the replacement carries its own credential. The + // activation writes the replacement's value, so purging the shared key by name would + // delete the live profile's credential: the round-4 data-loss shape. Only the deleted + // profile's keys the replacement does not declare may be purged. + const { profiles } = mockProviderSettingsManager() + + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { + return { ...otherProfile, apiProvider: providerIdentifiers.anthropic, apiKey: "replacement-key" } + }) + vi.mocked(provider.providerSettingsManager.getProfile).mockImplementation(async (params) => { + const name = "name" in params ? params.name : "" + + if (name === "other-config") { + return { + ...otherProfile, + name: "other-config", + apiProvider: providerIdentifiers.anthropic, + apiKey: "replacement-key", + } + } + + return { ...currentProfile, name: "current-config", apiKey: "deleted-key" } + }) + + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "deleted-key", + }) + + await provider.deleteProviderProfile(currentProfile) + + // The replacement's credential survives; the deleted profile's did not. + expect(provider.contextProxy.getSecret("apiKey")).toBe("replacement-key") + expect(mockContext.secrets.delete).not.toHaveBeenCalledWith("apiKey") + expect(profiles.map(({ name }) => name)).toEqual(["other-config"]) + }) + test("switches the active profile when deleting it and purges it from ProviderSettingsManager", async () => { mockProviderSettingsManager() diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 6d0bd3999f..851d3f187b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2512,18 +2512,22 @@ export const webviewMessageHandler = async ( const oldName = message.text - const newName = (await provider.providerSettingsManager.listConfig()).filter( - (c) => c.name !== oldName, - )[0]?.name - - if (!newName) { + // Route through the same policy entry point the extension API uses. The twin used to + // call `deleteConfig` + `activateProviderProfile` directly, which left it outside the + // mutation queue (a queued upsert could save the profile back after the purge) and + // without the rollback that restores the profile when the replacement activation + // fails (the webview then listed a profile the manager no longer had). The queued + // path also purges the deleted profile's own secrets, which the raw call never did + // on the inactive branch. + const entry = provider.getProviderProfileEntry(oldName) + + if (!entry) { vscode.window.showErrorMessage(t("common:errors.delete_api_config")) - return + break } try { - await provider.providerSettingsManager.deleteConfig(oldName) - await provider.activateProviderProfile({ name: newName }) + await provider.deleteProviderProfile(entry) } catch (error) { provider.log( `Error delete api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`, @@ -2531,10 +2535,10 @@ export const webviewMessageHandler = async ( vscode.window.showErrorMessage(t("common:errors.delete_api_config")) } finally { - // The purge drops the mode bindings that pointed at the deleted profile, and the - // activation only rebinds the current mode. Mirror the manager's map in a finally - // so the webview state cannot keep pointing a mode at a profile that no longer - // exists, even when the activation above failed after the purge. + // `deleteProviderProfile` mirrors the manager's mode map on both branches, but a + // failed delete rolls back to the pre-delete map through a queued restore. Mirroring + // here keeps the webview's map equal to the manager's whatever the outcome, so a + // mode can never keep pointing at a profile the manager no longer has. try { await provider.contextProxy.setValue( "modeApiConfigs", From 0b6b93457c5025fc3cd28e4bba6a6d14934c34a6 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Fri, 30 Oct 2026 23:54:02 +0900 Subject: [PATCH 15/17] fix(core): derive the delete purge keep-set from the surviving provider's ownership What: - Both delete branches now pass an ownership-derived keep-set to purgeDeletedProfileSecrets, via a new getSurvivingProviderSecretKeys helper that resolves the surviving profile's provider from the manager and keeps the deleted profile's keys that provider owns. - Exports providerOwnsSecretKey from ContextProxy. - Replaces the active branch's blob-derived keep-set with the same helper, and retitles the round-5 same-provider spec to assert the corrected semantics. - Adds three inactive-branch specs: same-provider preservation, cross-provider purge, and the unreadable-survivor purge fallback. Why: The inactive branch called purgeDeletedProfileSecrets with no keep-set, so deleting a profile whose blob declared a key the surviving active profile also uses deleted the live credential. The e2e profile test's finally deletes subtask-child-profile and subtask-parent-profile while default is active; all three are OpenRouter, so they share the single openRouterApiKey SecretStorage slot. The purge deleted default's credential and every rehydrated task hung until its 30 s timeout, which is the deterministic 9/3 mocked-e2e failure. A blob-derived keep-set cannot fix this: the api.getConfiguration() round-trip rewrites a profile without its secret fields while keeping apiProvider, so the surviving profile's blob is secret-less exactly when the live credential needs protecting. Provider ownership comes from the schema, which no round-trip can strip. The active branch had the same hazard and is aligned. Impact: Fixes the deterministic e2e-mock failure: the local mocked subtasks suite is 12/12. Preserves the S-3 security closure, because a cross-provider delete keeps nothing (the survivor owns none of the deleted keys) and an unreadable survivor falls back to purging every key the deleted profile declared. --- src/core/config/ContextProxy.ts | 2 +- src/core/webview/ClineProvider.ts | 69 ++++++++--- .../webview/__tests__/ClineProvider.spec.ts | 117 ++++++++++++++++-- 3 files changed, 158 insertions(+), 30 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index b587dfa4f1..e5e8cdb25e 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -84,7 +84,7 @@ for (const branch of providerSettingsSchemaDiscriminated.options) { * provider, whose branch is not in the union), so callers keep the old * presence-based semantics instead of guessing. */ -const providerOwnsSecretKey = (apiProvider: unknown, key: string): boolean | undefined => { +export const providerOwnsSecretKey = (apiProvider: unknown, key: string): boolean | undefined => { if (typeof apiProvider !== "string" || !isProviderName(apiProvider)) { return undefined } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0a8899a03b..6bd570255b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -112,7 +112,7 @@ import { t } from "../../i18n" import { buildApiHandler } from "../../api" import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/providers/fetchers/lmstudio" -import { ContextProxy, getProfileSecretKeys } from "../config/ContextProxy" +import { ContextProxy, getProfileSecretKeys, providerOwnsSecretKey } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" @@ -2261,7 +2261,18 @@ export class ClineProvider // clear-out: without an explicit purge the deleted profile's secrets stay in the // cache and in SecretStorage until some unrelated future write happens to name a // provider that does not own them, which may never happen. - await this.purgeDeletedProfileSecrets(undoConfig) + // + // The keep-set comes from the *surviving* profile's provider, not from its blob. + // A blob-derived set is empty exactly when the live credential needs protecting: + // the `api.getConfiguration()` round-trip rewrites a profile without its secret + // fields while keeping `apiProvider`, so a surviving OpenRouter profile's blob + // declares no `openRouterApiKey` even though that SecretStorage slot holds its + // live value. When the deleted profile and the survivor share a provider and a key + // name, the slot they would purge *is* the survivor's credential. + await this.purgeDeletedProfileSecrets( + undoConfig, + await this.getSurvivingProviderSecretKeys(globalSettings.currentApiConfigName, undoConfig), + ) await this.contextProxy.setValues({ listApiConfigMeta: this.getProviderProfileEntries().filter( @@ -2323,12 +2334,13 @@ export class ClineProvider // the replacement's name. The delete is explicit intent, so the deleted profile's own // keys are removed here, after the activation has written the replacement's slice. // - // A key the replacement's own blob declares is excluded: the replacement wrote its - // value through the activation above, and purging by name would delete the live - // profile's credential, which is the round-4 data-loss shape. + // The keep-set is ownership-derived for the same reason as the inactive branch: a + // blob-derived set is empty when the replacement's blob was written from an + // `api.getConfiguration()` snapshot (secrets stripped), and purging then deletes the + // replacement's live credential from the shared SecretStorage slot. await this.purgeDeletedProfileSecrets( deletedProfileConfig, - await this.getReplacementSecretKeys(profileToActivate), + await this.getSurvivingProviderSecretKeys(profileToActivate, deletedProfileConfig), ) // The activation above only rebinds the *current* mode, so every other mode that @@ -2364,20 +2376,44 @@ export class ClineProvider } /** - * The secret keys the profile that replaced a deleted one declares. + * The deleted profile's secret keys that the *surviving* profile's provider also owns. + * + * Both delete branches purge the deleted profile's credentials explicitly, and both need the + * same answer: is a key about to be deleted still in use by the profile that survives? That + * cannot be answered from the surviving profile's blob. The `api.getConfiguration()` round-trip + * rewrites a profile without its secret fields while keeping `apiProvider`, so a surviving + * OpenRouter profile's blob declares no `openRouterApiKey` even though that SecretStorage slot + * holds its live value. Provider ownership answers it from the schema instead, which no + * round-trip can strip. + * + * A same-provider succession (the deleted profile and the survivor both OpenRouter) shares one + * SecretStorage slot per key name, so "the deleted profile's key" *is* the survivor's credential: + * keeping it is the correct semantics, not a leak, because nothing is orphaned to purge. A + * cross-provider delete keeps nothing, because the survivor owns none of the deleted keys. * - * Used to keep the delete purge from removing a credential the replacement wrote itself. A - * manager that cannot produce the profile (it was just activated, so it should exist) yields an - * empty set: the purge then falls back to removing every key the deleted profile declared, which - * is the confidentiality-safe direction. + * An unreadable survivor yields an empty set, so the purge removes every key the deleted profile + * declared. That is the confidentiality-safe direction. */ - private async getReplacementSecretKeys(name: string): Promise> { + private async getSurvivingProviderSecretKeys( + survivingProfileName: string | undefined, + deletedConfig: (ProviderSettingsWithId & { name: string }) | undefined, + ): Promise> { + if (!survivingProfileName || !deletedConfig) { + return new Set() + } + + let survivingProvider: unknown + try { - const { name: _name, ...config } = await this.providerSettingsManager.getProfile({ name }) - return new Set(getProfileSecretKeys(config)) + const { apiProvider } = await this.providerSettingsManager.getProfile({ name: survivingProfileName }) + survivingProvider = apiProvider } catch { return new Set() } + + return new Set( + getProfileSecretKeys(deletedConfig).filter((key) => providerOwnsSecretKey(survivingProvider, key) === true), + ) } /** @@ -2388,8 +2424,9 @@ export class ClineProvider * same-provider succession is exactly the case ownership preserves. A delete is explicit * intent, so the deleted profile's keys are removed by name here instead of being inferred. * - * `keep` names the keys the replacement profile declares: those carry the replacement's own - * value, so removing them would delete a live credential. + * `keep` names the keys the surviving profile's provider still owns: those carry a live + * credential (a same-provider succession shares one SecretStorage slot per key name), so + * removing them would delete the surviving profile's own key. * * Best-effort: the profile is already gone from the manager, so a failure here must not turn * a successful delete into an error. The keys are logged by name only, never by value. diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index f885dec5db..a484d06428 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3813,17 +3813,108 @@ describe("ClineProvider", () => { expect(provider.getProviderProfileEntries()).toEqual([otherProfile]) }) - test("purges the deleted profile's secret when the replacement is the same provider", async () => { - // Ownership deliberately preserves a secret the *writing* provider owns, so a - // same-provider succession (Anthropic -> Anthropic) keeps the deleted profile's `apiKey` - // alive under the replacement's name. A delete is explicit intent: the deleted profile's - // own keys go, whether or not the replacement shares its provider. + test("keeps the surviving profile's credential on an inactive same-provider delete", async () => { + // The exact e2e trio shape: both profiles are OpenRouter, so they share the single + // `openRouterApiKey` SecretStorage slot, and the surviving `default` profile's blob was + // written from an `api.getConfiguration()` snapshot, which strips secrets. The inactive + // branch activates no replacement, so nothing rewrites the slot: purging the deleted + // profile's key would delete the live credential and every rehydrated task would hang. + const { profiles } = mockProviderSettingsManager() + + // Both profiles are OpenRouter with the same key name; the survivor's blob is secret-less. + vi.mocked(provider.providerSettingsManager.getProfile).mockImplementation(async (params) => { + const name = "name" in params ? params.name : "" + + if (name === "other-config") { + return { ...otherProfile, name: "other-config", apiProvider: providerIdentifiers.openrouter } + } + + return { + ...currentProfile, + name: "current-config", + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "live-key", + } + }) + + // The survivor is active, so deleting the other profile is an inactive deletion. + await provider.contextProxy.setValue("currentApiConfigName", "other-config") + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "live-key", + }) + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBe("live-key") + + await provider.deleteProviderProfile(currentProfile) + + // The slot is the surviving OpenRouter profile's credential, so it must survive. + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBe("live-key") + expect(mockContext.secrets.delete).not.toHaveBeenCalledWith("openRouterApiKey") + expect(profiles.map(({ name }) => name)).toEqual(["other-config"]) + }) + + test("still purges on an inactive cross-provider delete, which owns none of the deleted keys", async () => { + // The confidentiality case S-3 exists for: the survivor is OpenRouter, the deleted profile + // is Anthropic. The survivor owns none of the deleted profile's keys, so the keep-set is + // empty and the purge removes the orphaned credential. + mockProviderSettingsManager() + + await provider.contextProxy.setValue("currentApiConfigName", "other-config") + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "orphaned-key", + }) + expect(provider.contextProxy.getSecret("apiKey")).toBe("orphaned-key") + + await provider.deleteProviderProfile(currentProfile) + + expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() + expect(provider.contextProxy.getValues().apiKey).toBeUndefined() + expect(mockContext.secrets.delete).toHaveBeenCalledWith("apiKey") + }) + + test("purges on an inactive delete when the surviving profile cannot be read", async () => { + // Confidentiality-safe fallback: an unreadable survivor yields an empty keep-set, so the + // purge removes every key the deleted profile declared instead of silently retaining them. + mockProviderSettingsManager() + + await provider.contextProxy.setValue("currentApiConfigName", "other-config") + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "orphaned-key", + }) + + vi.mocked(provider.providerSettingsManager.getProfile).mockImplementation(async (params) => { + const name = "name" in params ? params.name : "" + + if (name === "other-config") { + throw new Error("Failed to read provider profiles from secrets: storage error") + } + + return { + ...currentProfile, + name: "current-config", + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "orphaned-key", + } + }) + + await provider.deleteProviderProfile(currentProfile) + + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBeUndefined() + expect(mockContext.secrets.delete).toHaveBeenCalledWith("openRouterApiKey") + }) + + test("keeps the shared slot when the replacement is the same provider and declares no secret", async () => { + // A same-provider succession where the replacement's blob is secret-less: the normal + // shape of a profile written from an `api.getConfiguration()` snapshot, which strips + // secrets. Two profiles of one provider necessarily share the same SecretStorage slot + // (each secret key has exactly one owning provider), so the slot holds the *replacement's* + // live credential. Purging it would delete the surviving profile's key, which is the + // round-4 data-loss shape and the deterministic e2e failure. Nothing is orphaned here, so + // keeping the slot is the correct semantics rather than a leak. const { profiles } = mockProviderSettingsManager() - // The replacement is Anthropic too, and its blob is secret-less: the normal shape of a - // profile written from an `api.getConfiguration()` snapshot, which strips secrets. The - // ownership rule reads that omission as a partial write and preserves `apiKey`, so only - // an explicit purge removes the deleted profile's credential. vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementationOnce(async () => { return { ...otherProfile, apiProvider: providerIdentifiers.anthropic } }) @@ -3845,10 +3936,10 @@ describe("ClineProvider", () => { await provider.deleteProviderProfile(currentProfile) - // The deleted profile's credential is gone rather than surviving under the replacement. - expect(provider.contextProxy.getSecret("apiKey")).toBeUndefined() - expect(provider.contextProxy.getValues().apiKey).toBeUndefined() - expect(mockContext.secrets.delete).toHaveBeenCalledWith("apiKey") + // The slot survives because the surviving Anthropic profile owns `apiKey`: it is that + // profile's credential, not the deleted profile's orphan. + expect(provider.contextProxy.getSecret("apiKey")).toBe("deleted-key") + expect(mockContext.secrets.delete).not.toHaveBeenCalledWith("apiKey") expect(profiles.map(({ name }) => name)).toEqual(["other-config"]) }) From 057100896fb5f1e4f619d164aedd0d79ad00eb80 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 03:16:15 +0900 Subject: [PATCH 16/17] fix(core): make the timed-out purge compensation atomic and store-derived What: the inactive/timed-out purge compensation now (a) restores the purged profile through `ProviderSettingsManager.saveConfigIfAbsent`, which checks and writes under one manager lock and reports whether it wrote, (b) skips that undo when a same-name profile was saved during the window instead of overwriting it with the pre-delete snapshot, (c) re-applies a dropped mode binding only when its id still names a profile (`hasConfigById`), and (d) fences the branch-tail metadata write behind the abort and reads the list from the manager instead of the `listApiConfigMeta` cache. Why: a timed-out purge commits after the caller is told the deletion failed. The undo then re-saved a stale snapshot over a newer same-name profile (T1), the reconcile re-applied bindings whose profile no longer existed (T4), and the late metadata write dropped a profile created during the window from `listApiConfigMeta` while the manager kept it (T5). `applyProviderProfileRestore` also wrote the stale `previousContextValues` snapshot on the inactive path, where no config was ever captured, which was the actual writer behind T5. Impact: after a timed-out purge the two stores converge to the manager's current view and never overwrite a newer write. Same-provider succession, the fail-closed fallback, and the S-3 cross-provider purge are unchanged. --- src/core/config/ProviderSettingsManager.ts | 43 ++++++++ src/core/webview/ClineProvider.ts | 115 +++++++++++++++------ 2 files changed, 126 insertions(+), 32 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 3795ef0e43..f3809c8dbe 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -443,6 +443,34 @@ export class ProviderSettingsManager { } } + /** + * Save a config only when no config with that name exists, as one locked step. + * + * A caller that has to restore a profile it is not sure still belongs there (the undo of a + * purge that committed after the caller timed out) cannot check-then-write: a save for the + * same name can land between the check and the write, and the restore would then overwrite + * it with a stale snapshot. The existence check and the write share one lock here, so a + * concurrent `saveConfig` either lands before the check (and this call skips) or after the + * write (and wins). Returns the written id, or `undefined` when the name was taken. + * + * The blob is stored verbatim: the caller restores a config this manager produced, so + * re-parsing it would only strip fields (a retired provider's legacy keys) it already holds. + */ + public async saveConfigIfAbsent(name: string, config: ProviderSettingsWithId): Promise { + return await this.lock(async () => { + const providerProfiles = await this.load() + + if (providerProfiles.apiConfigs[name]) { + return undefined + } + + const id = config.id || this.generateId() + providerProfiles.apiConfigs[name] = { ...config, id } + await this.store(providerProfiles) + return id + }) + } + public async getProfile( params: { name: string } | { id: string }, ): Promise { @@ -561,6 +589,21 @@ export class ProviderSettingsManager { } } + /** + * Check if a config exists by its manager-assigned id. + * + * The mode bindings a purge dropped name a profile by `id`, and a rollback that failed to + * restore the profile must not re-apply them: a binding whose id no profile carries makes the + * next switch to that mode resolve nothing and silently keep the current profile. Checking + * under the lock keeps the answer from straddling a concurrent purge or rename. + */ + public async hasConfigById(id: string) { + return await this.lock(async () => { + const providerProfiles = await this.load() + return Object.values(providerProfiles.apiConfigs).some((config) => config.id === id) + }) + } + /** * Set the API config for a specific mode. */ diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6bd570255b..f9bf286ee2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2253,6 +2253,17 @@ export class ClineProvider undoConfig, removedModeConfigs, ) + + // The write below is fenced behind the abort, so this abandoned body cannot + // land a late `setValues` after a newer queued mutation has written. It still + // converges: the list is the manager's current view, not the `listApiConfigMeta` + // cache, which never observed a profile created while the purge held the lock + // and would drop it from the webview's list while the manager keeps it. + await this.contextProxy.setValues({ + listApiConfigMeta: await this.providerSettingsManager.listConfig(), + modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), + }) + return } @@ -2274,8 +2285,19 @@ export class ClineProvider await this.getSurvivingProviderSecretKeys(globalSettings.currentApiConfigName, undoConfig), ) + // The purge above can outlast the mutation timeout. Once the abort fired, this + // abandoned body must not land a late `setValues` after a newer queued mutation + // has written: a profile created during the window would be dropped from + // `listApiConfigMeta` while the manager still holds it. + if (signal.aborted) { + return + } + + // Read the list from the manager rather than filtering the `listApiConfigMeta` + // cache: the cache only observes writes this provider made, so a profile created + // by a concurrent mutation would be missing from the filtered result. await this.contextProxy.setValues({ - listApiConfigMeta: this.getProviderProfileEntries().filter( + listApiConfigMeta: (await this.providerSettingsManager.listConfig()).filter( ({ name }) => name !== profileToDelete.name, ), modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), @@ -2524,37 +2546,58 @@ export class ClineProvider * Re-saving the captured config restores the profile under its original `id`, which makes the * mode bindings the purge dropped name a live profile again. Best-effort: a failure here is * logged rather than replacing the timeout the caller already received. + * + * Returns false when a profile with that name exists again, i.e. when the undo was skipped: + * the caller must then converge from the manager's view instead of a stale snapshot. */ private async undoCommittedProviderProfilePurge( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, removedModeConfigs: Record, - ): Promise { + ): Promise { if (!config) { - return + return false } try { const { name: _name, ...providerSettings } = config - await this.providerSettingsManager.saveConfig(name, providerSettings) + + // The check and the write share one manager lock: a queued save for the same name can + // land while the purge holds that lock, and re-saving the pre-delete snapshot over it + // would silently revert the newer profile's credentials. + const restoredId = await this.providerSettingsManager.saveConfigIfAbsent(name, providerSettings) + + if (restoredId === undefined) { + this.log( + `Skipped undoing the provider profile purge for '${name}': a profile with that name was saved after the timeout`, + ) + return false + } for (const [mode, configId] of Object.entries(removedModeConfigs)) { await this.providerSettingsManager.setModeConfig(mode as Mode, configId) } this.log(`Undid a provider profile purge for '${name}' that committed after the caller timed out`) + return true } catch (error) { this.log( `Failed to undo the provider profile purge for '${name}' after a timeout: ${ error instanceof Error ? error.message : String(error) }`, ) + return false } } /** * Rewrite the purged profile into the manager and ContextProxy. Returns false when the caller * already timed out, so the caller can reconcile instead of leaving a half-applied restore. + * + * With no captured config there is nothing to restore (the inactive branch never captures one), + * so this writes nothing and reports false: writing the `previousContextValues` snapshot would + * re-mirror a `listApiConfigMeta` that predates any profile created while the purge held the + * lock, dropping that profile from the webview's list while the manager keeps it. */ private async applyProviderProfileRestore( name: string, @@ -2563,41 +2606,43 @@ export class ClineProvider removedModeConfigs: Record, signal: AbortSignal, ): Promise { - if (config) { - // Keep `id`: the manager reuses it for the mode associations that point at this profile. - const { name: _name, ...providerSettings } = config - await this.providerSettingsManager.saveConfig(name, providerSettings) + if (!config) { + return false + } - if (signal.aborted) return false + // Keep `id`: the manager reuses it for the mode associations that point at this profile. + const { name: _name, ...providerSettings } = config + await this.providerSettingsManager.saveConfig(name, providerSettings) - await this.providerSettingsManager.activateProfile({ name }) + if (signal.aborted) return false - if (signal.aborted) return false + await this.providerSettingsManager.activateProfile({ name }) - // Restore the whole provider slice, not just the name pointers: a partial activation may - // have left the replacement's provider settings under the deleted name. `id` is manager - // metadata, not a setting, so it must not reach `ContextProxy`. `apiProvider` belongs to - // this slice, so the snapshot below deliberately does not carry it: writing it again - // would overwrite the restored provider with the pre-delete pointer. It is still passed - // as the ownership discriminator, because the profile being restored is the one whose - // own secrets must survive this write. - const { id: _id, ...settings } = providerSettings - await this.contextProxy.setProviderSettings(settings, config.apiProvider) + if (signal.aborted) return false - if (signal.aborted) return false + // Restore the whole provider slice, not just the name pointers: a partial activation may + // have left the replacement's provider settings under the deleted name. `id` is manager + // metadata, not a setting, so it must not reach `ContextProxy`. `apiProvider` belongs to + // this slice, so the snapshot below deliberately does not carry it: writing it again + // would overwrite the restored provider with the pre-delete pointer. It is still passed + // as the ownership discriminator, because the profile being restored is the one whose + // own secrets must survive this write. + const { id: _id, ...settings } = providerSettings + await this.contextProxy.setProviderSettings(settings, config.apiProvider) - // `saveConfig` above reused the profile's `id`, so the mode bindings the purge dropped - // name a profile that exists again. Re-apply them in the manager with a per-write abort - // check, then mirror the pre-delete map into ContextProxy: the snapshot below restores - // every mode, including the ones the purge never touched. - for (const [mode, configId] of Object.entries(removedModeConfigs)) { - if (signal.aborted) return false - await this.providerSettingsManager.setModeConfig(mode as Mode, configId) - } + if (signal.aborted) return false + // `saveConfig` above reused the profile's `id`, so the mode bindings the purge dropped + // name a profile that exists again. Re-apply them in the manager with a per-write abort + // check, then mirror the pre-delete map into ContextProxy: the snapshot below restores + // every mode, including the ones the purge never touched. + for (const [mode, configId] of Object.entries(removedModeConfigs)) { if (signal.aborted) return false + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) } + if (signal.aborted) return false + await this.contextProxy.setValues(previousContextValues) return true } @@ -2612,6 +2657,7 @@ export class ClineProvider * semantics apply: a raw `setValues` merge would leave the restored profile's differently-named * secrets behind under the fallback name. The mode map is reconciled too, so ContextProxy cannot * keep pointing a mode at the deleted profile after the purge dropped the binding in the manager. + * */ private async reconcileActiveProfileAfterFailedRestore( removedModeConfigs: Record = {}, @@ -2636,10 +2682,15 @@ export class ClineProvider listApiConfigMeta: remainingProfiles, }) // The purge may have dropped mode bindings the restore could not re-apply; re-apply - // them now so the manager's map matches the pre-delete state again. The mirror below - // runs after the re-apply, so ContextProxy never holds a map the manager disagrees with. + // them now so the manager's map matches the pre-delete state again. A binding is only + // re-applied when its id still names a profile: the restore's `saveConfig` can reject, + // and a re-applied dangling id makes the next switch to that mode resolve nothing and + // silently keep the current profile. The mirror below runs after the re-apply, so + // ContextProxy never holds a map the manager disagrees with. for (const [mode, configId] of Object.entries(removedModeConfigs)) { - await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + if (await this.providerSettingsManager.hasConfigById(configId)) { + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + } } await this.contextProxy.setValue("modeApiConfigs", await this.providerSettingsManager.getModeApiConfigs()) } catch (reconcileError) { From 311e784aed5582e2f7cf5861b855a74559b7f45f Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 04:53:00 +0900 Subject: [PATCH 17/17] test(core): back-port the profile-purge spec harness and the T1/T4/T5 regressions What: the deleteProviderProfile spec harness now mirrors the real manager's single lock (`withLock` + a lock-free `deleteConfigUnlocked`), mocks `saveConfigIfAbsent`, `hasConfigById` and `hasConfig`, and the legacy inactive-undo recorder delegates to `getMockImplementation()` instead of re-splicing the profile array. Adds the T1, T4 and T5 regression tests, whose fixes are in this PR. Why: this PR's source calls `ProviderSettingsManager.saveConfigIfAbsent` and `hasConfigById`, but the spec harness at this commit predates both, so three deleteProviderProfile specs failed against correct source. The recorder also had to stop re-implementing the splice: the harness keeps profiles in a private `configs` map, so a recorder that only spliced the array left that map holding the purged name and `saveConfigIfAbsent` saw it present, silently skipping the undo. Impact: spec-only, no source change. `__tests__/` is in the mutation gate's EXCLUDED_PATH_SEGMENTS, so this commit costs 0 gate lines. The T2/T3/T6/T7 regressions stay with the follow-up PR because they need its source changes. --- .../webview/__tests__/ClineProvider.spec.ts | 329 ++++++++++++++---- 1 file changed, 260 insertions(+), 69 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index a484d06428..765341c248 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3713,60 +3713,110 @@ describe("ClineProvider", () => { // bindings that pointed at the deleted profile instead of leaving a dangling id. const modeApiConfigs: Record = {} + // The real manager serializes every read/write through one promise chain, and the + // timed-out-purge races are all about who holds that lock. Without it the harness cannot + // express the ordering: a `saveConfig` issued while a purge is in flight would land + // before the purge instead of behind it. + let managerLock = Promise.resolve() + const withLock = (step: () => Promise): Promise => { + const next = managerLock.then(step) + managerLock = next.then( + () => undefined, + () => undefined, + ) + return next + } + + // The purge's body, without the lock: a test that needs to hold the manager lock across + // a gate wraps this in `withLock` itself, so a concurrent `saveConfig` queues behind the + // purge instead of landing before it. + const deleteConfigUnlocked = async (name: string) => { + const removedModeConfigs: Record = {} + const deletedId = configs.get(name)?.id + const index = profiles.findIndex((profile) => profile.name === name) + if (index !== -1) { + profiles.splice(index, 1) + } + configs.delete(name) + + for (const [mode, configId] of Object.entries(modeApiConfigs)) { + if (configId === deletedId) { + removedModeConfigs[mode] = configId + delete modeApiConfigs[mode] + } + } + + return { removedModeConfigs } + } + + const writeConfig = (name: string, config: ProviderSettingsEntry) => { + configs.set(name, { ...config, name }) + if (!profiles.some((profile) => profile.name === name)) { + profiles.push({ name, id: config.id, apiProvider: config.apiProvider }) + } + return config.id + } + Object.assign(provider, { providerSettingsManager: { - deleteConfig: vi.fn().mockImplementation(async (name: string) => { - const removedModeConfigs: Record = {} - const deletedId = configs.get(name)?.id - const index = profiles.findIndex((profile) => profile.name === name) - if (index !== -1) { - profiles.splice(index, 1) - } - configs.delete(name) - - for (const [mode, configId] of Object.entries(modeApiConfigs)) { - if (configId === deletedId) { - removedModeConfigs[mode] = configId - delete modeApiConfigs[mode] + deleteConfig: vi + .fn() + .mockImplementation((name: string) => withLock(() => deleteConfigUnlocked(name))), + listConfig: vi.fn().mockImplementation(() => withLock(async () => profiles)), + getProfile: vi.fn().mockImplementation(({ name }: { name: string }) => + withLock(async () => { + const config = configs.get(name) + if (!config) { + throw new Error(`Config with name '${name}' not found`) } - } - - return { removedModeConfigs } - }), - listConfig: vi.fn().mockImplementation(async () => profiles), - getProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { - const config = configs.get(name) - if (!config) { - throw new Error(`Config with name '${name}' not found`) - } - return { ...config, name } - }), - saveConfig: vi.fn().mockImplementation(async (name: string, config: ProviderSettingsEntry) => { - configs.set(name, { ...config, name }) - if (!profiles.some((profile) => profile.name === name)) { - profiles.push({ name, id: config.id, apiProvider: config.apiProvider }) - } - return config.id - }), - activateProfile: vi.fn().mockImplementation(async ({ name }: { name: string }) => { - const config = configs.get(name) - if (!config) { - throw new Error(`Config '${name}' not found`) - } - managerState.currentApiConfigName = name - // Include a distinguishing setting so tests can assert the - // effective provider settings, not just the name pointers. - return { ...config, name } - }), - setModeConfig: vi.fn().mockImplementation(async (mode: string, configId: string) => { - modeApiConfigs[mode] = configId - }), - getModeConfigId: vi.fn().mockImplementation(async (mode: string) => modeApiConfigs[mode]), - getModeApiConfigs: vi.fn().mockImplementation(async () => ({ ...modeApiConfigs })), + return { ...config, name } + }), + ), + saveConfig: vi + .fn() + .mockImplementation((name: string, config: ProviderSettingsEntry) => + withLock(async () => writeConfig(name, config)), + ), + // Mirrors the real manager's single-lock check-and-write: the existence check and + // the write share one step, so a concurrent `saveConfig` cannot slip between them. + saveConfigIfAbsent: vi + .fn() + .mockImplementation((name: string, config: ProviderSettingsEntry) => + withLock(async () => (configs.has(name) ? undefined : writeConfig(name, config))), + ), + hasConfigById: vi + .fn() + .mockImplementation((id: string) => + withLock(async () => + Object.values(Object.fromEntries(configs)).some((config) => config.id === id), + ), + ), + hasConfig: vi.fn().mockImplementation((name: string) => withLock(async () => configs.has(name))), + activateProfile: vi.fn().mockImplementation(({ name }: { name: string }) => + withLock(async () => { + const config = configs.get(name) + if (!config) { + throw new Error(`Config '${name}' not found`) + } + managerState.currentApiConfigName = name + // Include a distinguishing setting so tests can assert the + // effective provider settings, not just the name pointers. + return { ...config, name } + }), + ), + setModeConfig: vi.fn().mockImplementation((mode: string, configId: string) => + withLock(async () => { + modeApiConfigs[mode] = configId + }), + ), + getModeConfigId: vi + .fn() + .mockImplementation((mode: string) => withLock(async () => modeApiConfigs[mode])), + getModeApiConfigs: vi.fn().mockImplementation(() => withLock(async () => ({ ...modeApiConfigs }))), }, }) - return { profiles, managerState, modeApiConfigs } + return { profiles, managerState, modeApiConfigs, withLock, deleteConfigUnlocked } } beforeEach(async () => { @@ -4514,26 +4564,20 @@ describe("ClineProvider", () => { await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) // The purge splices for real, so "the profile survived" is asserted against the - // manager's own profile list rather than against a spy. + // manager's own profile list rather than against a spy. The recorder delegates to the + // harness implementation instead of re-implementing the splice: the harness keeps its + // profiles in a private `configs` map too, and a recorder that only spliced the array + // left that map holding the purged name, so `saveConfigIfAbsent` saw it present and + // the undo silently skipped. const deleteConfigCalls: string[] = [] - vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementation(async (name) => { + const harnessDeleteConfig = vi + .mocked(provider.providerSettingsManager.deleteConfig) + .getMockImplementation()! + const recordingDeleteConfig = async (name: string) => { deleteConfigCalls.push(name) - const deletedId = profiles.find((profile) => profile.name === name)?.id - const index = profiles.findIndex((profile) => profile.name === name) - if (index !== -1) { - profiles.splice(index, 1) - } - - const removedModeConfigs: Record = {} - for (const [mode, configId] of Object.entries(modeApiConfigs)) { - if (configId === deletedId) { - removedModeConfigs[mode] = configId - delete modeApiConfigs[mode] - } - } - - return { removedModeConfigs } - }) + return harnessDeleteConfig(name) + } + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementation(recordingDeleteConfig) // Hold the purge past the mutation window: the caller is told the deletion timed out // while the manager lock is still held, which is exactly the ordering that lets a @@ -4542,10 +4586,9 @@ describe("ClineProvider", () => { const purgeGate = new Promise((resolve) => { releasePurge = resolve }) - const realDeleteConfig = provider.providerSettingsManager.deleteConfig vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce(async (name) => { await purgeGate - return realDeleteConfig(name) + return recordingDeleteConfig(name) }) const deletion = provider.deleteProviderProfile(otherProfile) @@ -4575,12 +4618,160 @@ describe("ClineProvider", () => { expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) expect(profiles.find(({ name }) => name === "other-config")?.id).toBe(otherProfile.id) expect(modeApiConfigs).toEqual({ code: otherProfile.id }) - // ContextProxy still lists both profiles, so the two stores agree. + // The list comes from the manager's view, so the two stores agree. expect(provider.getProviderProfileEntries()).toEqual([currentProfile, otherProfile]) } finally { vi.useRealTimers() } }) + + // --- Round-7 per-thread regressions (CodeRabbit #1735 T1-T7) --- + + test("T1: skips the undo when a same-name profile was saved while the timed-out purge held the lock", async () => { + vi.useFakeTimers() + + try { + const { profiles, modeApiConfigs, withLock, deleteConfigUnlocked } = mockProviderSettingsManager() + modeApiConfigs.code = otherProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // Hold the purge *inside* the manager lock past the mutation window, so the caller is + // told the deletion timed out while the purge still holds the lock (probe-A shape). + // A `saveConfig` issued after this gate queues behind the purge, exactly like the real + // manager's FIFO chain. + let releasePurge!: () => void + const purgeGate = new Promise((resolve) => { + releasePurge = resolve + }) + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce((name: string) => + withLock(async () => { + await purgeGate + return deleteConfigUnlocked(name) + }), + ) + + const deletion = provider.deleteProviderProfile(otherProfile) + const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") + + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + + // The user re-creates the same name while the abandoned purge still holds the lock. + // A raw `saveConfig` (not `upsertProviderProfile`, which pins the queue with + // `runToCompletion` and would never land before the undo's existence check). + const recreated = provider.providerSettingsManager.saveConfig("other-config", { + id: "recreated-id", + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "brand-new-user-key", + openRouterModelId: "brand-new-model", + }) + + releasePurge() + for (let i = 0; i < 50; i++) { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(0) + } + + await recreated + await deletionResult + + // The undo must not re-save the pre-delete snapshot over the newer profile. + expect(await provider.providerSettingsManager.getProfile({ name: "other-config" })).toMatchObject({ + id: "recreated-id", + openRouterApiKey: "brand-new-user-key", + openRouterModelId: "brand-new-model", + }) + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + // The skipped undo is why the mode binding stays dropped: the re-created profile has a + // different id, so re-applying the old binding would leave a dangling id. + expect(modeApiConfigs).toEqual({}) + } finally { + vi.useRealTimers() + } + }) + + test("T4: failed-restore reconcile does not re-apply bindings whose profile no longer exists", async () => { + const { modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + modeApiConfigs.architect = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // The replacement activation fails, and the restore's own `saveConfig` rejects, so the + // deleted profile never comes back (probe-C shape). + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + vi.mocked(provider.providerSettingsManager.saveConfig).mockRejectedValueOnce( + new Error("Failed to save config: storage error"), + ) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to activate profile: storage error", + ) + + // The deleted profile is gone, so its id is dangling: re-applying it would make the next + // switch to that mode resolve nothing and silently keep the current profile. + expect(await provider.providerSettingsManager.hasConfigById(currentProfile.id)).toBe(false) + expect(modeApiConfigs).toEqual({}) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual({}) + }) + + test("T5: timed-out inactive rollback cannot drop a profile saved during the window from listApiConfigMeta", async () => { + vi.useFakeTimers() + + try { + const { profiles, withLock, deleteConfigUnlocked } = mockProviderSettingsManager() + + // Hold the purge *inside* the manager lock past the mutation window (probe-D shape). + let releasePurge!: () => void + const purgeGate = new Promise((resolve) => { + releasePurge = resolve + }) + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce((name: string) => + withLock(async () => { + await purgeGate + return deleteConfigUnlocked(name) + }), + ) + + const deletion = provider.deleteProviderProfile(otherProfile) + const deletionResult = expect(deletion).rejects.toThrow("Provider profile mutation timed out") + + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + + // A different-name profile is created during the window. The `listApiConfigMeta` + // cache never observes it, so a cache-derived or snapshot-derived write drops it. + const created = provider.providerSettingsManager.saveConfig("third-config", { + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "third-key", + }) + + releasePurge() + for (let i = 0; i < 50; i++) { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(0) + } + + await created + await deletionResult + + // The abandoned body's compensation must converge to the manager's view, so the + // mid-window profile stays listed in both stores. + expect(profiles.map(({ name }) => name).sort()).toEqual([ + "current-config", + "other-config", + "third-config", + ]) + expect( + provider + .getProviderProfileEntries() + .map(({ name }) => name) + .sort(), + ).toEqual(["current-config", "other-config", "third-config"]) + } finally { + vi.useRealTimers() + } + }) }) })