From eb5e65b84617f154bd7133872ad177d557e1c239 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Mon, 21 Sep 2026 06:53:08 +0900 Subject: [PATCH 01/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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/26] 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 f779686b94b935439f9e680c6c79c5f4819384cb Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 03:18:36 +0900 Subject: [PATCH 17/26] fix(core): gate the failed-restore reconcile's fallback activation What: `reconcileActiveProfileAfterFailedRestore` now activates a fallback only when the current selection is not a newer one, i.e. a name the restore was not trying to restore that the manager still holds. The `hasConfigById`-guarded mode re-applies and the mode-map mirror stay ungated. Why: when a restore times out, the queue advances while the restore body is still pending. If that body later resumes, `applyProviderProfileRestore` returns false and the caller reconciled with no gate at all, activating `listConfig()[0]` and replacing a profile the user had selected in the meantime. Impact: an aborted restore can no longer replace a newer selection, while it still reconciles the mode bindings in both stores (the convergence writes are over ids that provably exist, so they need no gate). Gating them on the abort is what would break that contract, so the gate is on the fallback activation only. --- src/core/webview/ClineProvider.ts | 43 ++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f9bf286ee2..55a0c640fb 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2658,29 +2658,42 @@ export class ClineProvider * 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. * + * The fallback activation is the only write here that can *replace* a profile, so it is gated on + * the selection being a newer one: a name the restore was not trying to restore, which the + * manager still holds, was picked by someone else while the restore was pending and needs no + * replacement. The `hasConfigById`-guarded re-applies and the mirror below are convergence + * writes over ids that provably exist, so they stay ungated: an aborted restore must still + * reconcile the mode bindings, which is the contract the head spec pins. */ private async reconcileActiveProfileAfterFailedRestore( removedModeConfigs: Record = {}, + previousContextValues: Pick = {}, ): Promise { try { - const remainingProfiles = await this.providerSettingsManager.listConfig() - const fallback = remainingProfiles[0]?.name + const selectedName = this.contextProxy.getValues().currentApiConfigName + const selectionIsNewer = !!selectedName && selectedName !== previousContextValues.currentApiConfigName + const selectionSurvives = selectionIsNewer && (await this.providerSettingsManager.hasConfig(selectedName)) - if (!fallback) { - return - } + if (!selectionSurvives) { + 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 }) + const { + name: _fallbackName, + id: _fallbackId, + ...fallbackSettings + } = await this.providerSettingsManager.activateProfile({ name: fallback }) - await this.contextProxy.setProviderSettings(fallbackSettings, fallbackSettings.apiProvider) - await this.contextProxy.setValues({ - currentApiConfigName: fallback, - listApiConfigMeta: remainingProfiles, - }) + await this.contextProxy.setProviderSettings(fallbackSettings, fallbackSettings.apiProvider) + await this.contextProxy.setValues({ + 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. A binding is only // re-applied when its id still names a profile: the restore's `saveConfig` can reject, From 7c3feed1585c4e335b104c5cbc837eb1bbbbdf9e Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 03:21:40 +0900 Subject: [PATCH 18/26] fix(core): post the rollback state, widen the keep-set, surface a failed purge What: three delete-path defects the review found, plus the per-thread regression tests for T1-T7. - T3: the queued rollback posts state at the end of its body, after both the restore and the reconcile branches, so the webview cannot keep showing the replacement profile after the rollback re-pointed both stores. - T6: `getSurvivingProviderSecretKeys` unions ownership over *every* remaining profile's provider instead of resolving only the active one. A profile shares one SecretStorage slot per key name with every profile of the same provider, so a remaining OpenRouter profile that is not active still reads the slot an inactive delete would otherwise purge. A key no remaining provider owns is still purged (S-3 cross-provider delete), and an unreadable remaining profile contributes nothing, which keeps the fail-closed direction. - T7: a rejected secret purge is retried once and then surfaced with a warning. The delete stays successful because the profile is already gone from the manager. The retry matters because `storeSecret` clears the cache before it awaits `secrets.delete`: a rejected delete leaves the cache empty while SecretStorage keeps the value, and the ownership clear-out skips keys absent from the cache, so nothing else retries them in this session. Why: without these, a rollback left the webview naming a profile the stores no longer had (T3), an inactive delete could delete a non-active profile's live credential (T6), and a failed purge was invisible to the caller and unrecoverable until restart (T7). Impact: the keep-set only widens (to all remaining profiles), the fail-closed fallback and the S-3 cross-provider purge are preserved, and every T1-T7 test in `ClineProvider.spec.ts` fails on 0b6b93457 and passes here. --- src/core/webview/ClineProvider.ts | 220 ++++---- .../webview/__tests__/ClineProvider.spec.ts | 505 +++++++++++++++--- 2 files changed, 559 insertions(+), 166 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 55a0c640fb..3196c08eb2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2254,16 +2254,13 @@ export class ClineProvider 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. + // Fenced behind the abort, and read from the manager rather than the + // `listApiConfigMeta` cache, which never observed a profile created while the + // purge held the lock. await this.contextProxy.setValues({ listApiConfigMeta: await this.providerSettingsManager.listConfig(), modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), }) - return } @@ -2273,29 +2270,27 @@ export class ClineProvider // cache and in SecretStorage until some unrelated future write happens to name a // provider that does not own them, which may never happen. // - // 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. + // The keep-set comes from the providers of the profiles that remain, not from the + // active profile's 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 a + // remaining profile share a provider and a key name, the slot they would purge + // *is* that profile's credential. await this.purgeDeletedProfileSecrets( undoConfig, - await this.getSurvivingProviderSecretKeys(globalSettings.currentApiConfigName, undoConfig), + await this.getSurvivingProviderSecretKeys(profileToDelete.name, 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. + // 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. 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. + // Read the list from the manager rather than the `listApiConfigMeta` cache: the + // cache only observes writes this provider made. await this.contextProxy.setValues({ listApiConfigMeta: (await this.providerSettingsManager.listConfig()).filter( ({ name }) => name !== profileToDelete.name, @@ -2359,10 +2354,12 @@ export class ClineProvider // 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. + // replacement's live credential from the shared SecretStorage slot. It spans every + // remaining profile, so a same-provider profile that is not the replacement keeps its + // credential too. await this.purgeDeletedProfileSecrets( deletedProfileConfig, - await this.getSurvivingProviderSecretKeys(profileToActivate, deletedProfileConfig), + await this.getSurvivingProviderSecretKeys(profileToDelete.name, deletedProfileConfig), ) // The activation above only rebinds the *current* mode, so every other mode that @@ -2398,44 +2395,54 @@ export class ClineProvider } /** - * The deleted profile's secret keys that the *surviving* profile's provider also owns. + * The deleted profile's secret keys that any *remaining* 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 + * same answer: is a key about to be deleted still in use by a profile that survives? That + * cannot be answered from a 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. + * The answer spans *every* remaining profile, not just the one that ends up active. A profile + * shares one SecretStorage slot per key name with every profile of the same provider, so a + * remaining OpenRouter profile that is not active still reads the slot an inactive delete would + * otherwise purge. Keeping a key is the correct semantics, not a leak, because nothing is + * orphaned: a key no remaining provider owns is still purged, so a cross-provider delete keeps + * nothing. * - * An unreadable survivor yields an empty set, so the purge removes every key the deleted profile - * declared. That is the confidentiality-safe direction. + * A remaining profile that cannot be read contributes nothing, so its keys are purged. That is + * the confidentiality-safe direction, and it is also what an unresolvable survivor set does: + * the keep-set is empty and every key the deleted profile declared is removed. */ private async getSurvivingProviderSecretKeys( - survivingProfileName: string | undefined, + deletedName: string, deletedConfig: (ProviderSettingsWithId & { name: string }) | undefined, ): Promise> { - if (!survivingProfileName || !deletedConfig) { - return new Set() - } - - let survivingProvider: unknown + const deletedKeys = deletedConfig ? getProfileSecretKeys(deletedConfig) : [] + const keep = new Set() try { - const { apiProvider } = await this.providerSettingsManager.getProfile({ name: survivingProfileName }) - survivingProvider = apiProvider + for (const { name } of await this.providerSettingsManager.listConfig()) { + if (name === deletedName || keep.size === deletedKeys.length) { + continue + } + + try { + const { apiProvider } = await this.providerSettingsManager.getProfile({ name }) + for (const key of deletedKeys) { + if (providerOwnsSecretKey(apiProvider, key) === true) keep.add(key) + } + } catch { + // An unreadable remaining profile contributes nothing, so its keys are purged. + } + } } catch { return new Set() } - return new Set( - getProfileSecretKeys(deletedConfig).filter((key) => providerOwnsSecretKey(survivingProvider, key) === true), - ) + return keep } /** @@ -2446,12 +2453,16 @@ 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 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. + * `keep` names the keys a remaining profile's provider still owns: those carry a live + * credential (profiles of one provider share one SecretStorage slot per key name), so removing + * them would delete a 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. + * The delete itself stays successful: the profile is already gone from the manager, so a purge + * failure is retried once and then surfaced, never rethrown. A retry is needed because + * `storeSecret` clears the cache before it awaits `secrets.delete`: a rejected delete leaves the + * cache empty while SecretStorage still holds the value, and the ownership clear-out skips keys + * absent from the cache, so nothing else retries them in this session. The keys are logged by + * name only, never by value. */ private async purgeDeletedProfileSecrets( config: (ProviderSettingsWithId & { name: string }) | undefined, @@ -2468,14 +2479,22 @@ export class ClineProvider } try { - await this.contextProxy.purgeProviderSecrets(secretKeys) + try { + await this.contextProxy.purgeProviderSecrets(secretKeys) + } catch { + // The cache is already cleared for these keys, so a retry re-issues the storage delete + // that failed rather than relying on a later clear-out that would skip them. + 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) - }`, - ) + const message = `Failed to purge the secrets of the deleted profile '${config.name}': ${error instanceof Error ? error.message : String(error)}` + + this.log(message) + // The delete succeeded, so this must not reject it; the user still has to learn that a + // credential may remain in SecretStorage until the next successful purge or restart. + void vscode.window.showWarningMessage(message) } } @@ -2519,13 +2538,19 @@ export class ClineProvider if (restored) { this.log(`Restored provider profile '${name}' after a failed replacement activation`) - return + } else { + // The restore did not finish, so both stores may still name a profile the + // manager 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, previousContextValues) } - // The restore did not finish, so both stores may still name a profile the manager - // 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) + // The replacement activation posted state before this rollback landed, and the + // rollback's own writes (the restore slice or the reconcile's fallback) change what + // the webview must show. Without a post here the webview keeps naming the + // replacement while both stores name the restored or reconciled profile. + await this.postStateToWebview() }, { runToCompletion: true, abortOnTimeout: true }, ) @@ -2565,12 +2590,8 @@ export class ClineProvider // 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`, - ) + if ((await this.providerSettingsManager.saveConfigIfAbsent(name, providerSettings)) === undefined) { + this.log(`Skipped undoing the purge for '${name}': a same-name profile was saved after the timeout`) return false } @@ -2606,45 +2627,48 @@ export class ClineProvider removedModeConfigs: Record, signal: AbortSignal, ): Promise { - if (!config) { - return false - } + 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) - // 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 (signal.aborted) return false - if (signal.aborted) return false + await this.providerSettingsManager.activateProfile({ name }) - await this.providerSettingsManager.activateProfile({ name }) + 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) - // 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 + // `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) + } - // `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 + } - await this.contextProxy.setValues(previousContextValues) - return true + // The inactive branch never captures a config, so there is nothing to restore: writing the + // `previousContextValues` snapshot would re-mirror a list that predates any profile created + // while the purge held the lock. + return false } /** @@ -2671,10 +2695,12 @@ export class ClineProvider ): Promise { try { const selectedName = this.contextProxy.getValues().currentApiConfigName - const selectionIsNewer = !!selectedName && selectedName !== previousContextValues.currentApiConfigName - const selectionSurvives = selectionIsNewer && (await this.providerSettingsManager.hasConfig(selectedName)) + const keepSelection = + !!selectedName && + selectedName !== previousContextValues.currentApiConfigName && + (await this.providerSettingsManager.hasConfig(selectedName)) - if (!selectionSurvives) { + if (!keepSelection) { const remainingProfiles = await this.providerSettingsManager.listConfig() const fallback = remainingProfiles[0]?.name diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index a484d06428..9ac0b85814 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,336 @@ 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("T2: reconcile after an aborted restore does not replace a newer user selection", async () => { + vi.useFakeTimers() + + try { + const { managerState } = mockProviderSettingsManager() + + // A third profile, so the reconcile's fallback (`listConfig()[0]`) is neither the + // deleted profile nor the one the user picks: with only two profiles the fallback + // would coincide with the user's selection and the clobber would be invisible. + const thirdProfile: ProviderSettingsEntry = { + name: "third-config", + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + } + vi.mocked(provider.providerSettingsManager.listConfig).mockImplementation(async () => [ + thirdProfile, + otherProfile, + ]) + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementation( + async (params: { name: string } | { id: string }) => { + const name = "name" in params ? params.name : thirdProfile.name + managerState.currentApiConfigName = name + return { ...thirdProfile, name } + }, + ) + + // The replacement activation fails fast so the rollback starts, then the restore's + // own `saveConfig` hangs past the restore timeout: the abort fires while the restore + // body is pending and the queue advances (probe-B shape). + vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( + new Error("Failed to activate profile: storage error"), + ) + let releaseRestore!: () => void + const pendingRestore = new Promise((resolve) => { + releaseRestore = resolve + }) + vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { + await pendingRestore + return currentProfile.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()) + + // The user selects the surviving profile while the restore is still pending. The + // manager holds that name, so the reconcile's gate sees a live newer selection. + await provider.contextProxy.setValue("currentApiConfigName", "other-config") + managerState.currentApiConfigName = "other-config" + + // Cross the restore timeout: the abort fires and the queue advances. + await vi.advanceTimersByTimeAsync(ClineProvider.PENDING_OPERATION_TIMEOUT_MS + 1) + releaseRestore() + await deletionResult + await vi.advanceTimersByTimeAsync(0) + + // The reconcile must not activate its fallback over the user's newer selection. + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("other-config") + expect(managerState.currentApiConfigName).toBe("other-config") + } finally { + vi.useRealTimers() + } + }) + + test("T3: posts state after the rollback restores the profile", async () => { + await provider.resolveWebviewView(mockWebviewView) + mockProviderSettingsManager() + mockPostMessage.mockClear() + + // Fail the replacement activation so the rollback restores the deleted profile. + 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 replacement activation posted state before the rollback landed, so the rollback + // needs its own post: without it the webview keeps naming the replacement. + const postedStates = mockPostMessage.mock.calls.filter( + ([message]: [ExtensionMessage]) => message.type === "state", + ) + const lastState = postedStates.at(-1)?.[0].state as ExtensionState | undefined + expect(lastState?.currentApiConfigName).toBe("current-config") + }) + + 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() + } + }) + + test("T6: inactive delete keeps a credential a non-active remaining profile owns", async () => { + // Three profiles: A is active (Anthropic), B is inactive (OpenRouter), C is deleted + // (OpenRouter). B and C share the single `openRouterApiKey` SecretStorage slot, so + // purging it would delete B's live credential even though B is not the active profile. + const thirdProfile: ProviderSettingsEntry = { + name: "third-config", + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + } + mockProviderSettingsManager() + const configs = new Map>([ + [currentProfile.name, { ...currentProfile, apiKey: "current-key" }], + [thirdProfile.name, { ...thirdProfile, openRouterModelId: "third-model" }], + // The profile being deleted is readable and declares the shared OpenRouter key, so + // the purge actually targets that key: an unreadable deleted profile would make the + // purge a no-op and the test would pass without proving anything. + [ + "deleted-config", + { + name: "deleted-config", + id: "deleted-id", + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "shared-live-key", + }, + ], + ]) + vi.mocked(provider.providerSettingsManager.listConfig).mockImplementation(async () => [ + currentProfile, + thirdProfile, + ]) + vi.mocked(provider.providerSettingsManager.getProfile).mockImplementation(async (params) => { + const name = "name" in params ? params.name : "" + const config = configs.get(name) + if (!config) { + throw new Error(`Config with name '${name}' not found`) + } + return { ...config, name } + }) + + // A (Anthropic) is active; the deleted profile C is OpenRouter. + await provider.contextProxy.setValue("currentApiConfigName", "current-config") + await provider.contextProxy.setValue("listApiConfigMeta", [currentProfile, thirdProfile]) + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.openrouter, + openRouterApiKey: "shared-live-key", + }) + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBe("shared-live-key") + + await provider.deleteProviderProfile({ + name: "deleted-config", + id: "deleted-id", + apiProvider: providerIdentifiers.openrouter, + }) + + // B still owns the slot, so the credential must survive even though the active profile + // is Anthropic and owns none of the deleted keys. + expect(provider.contextProxy.getSecret("openRouterApiKey")).toBe("shared-live-key") + expect(mockContext.secrets.delete).not.toHaveBeenCalledWith("openRouterApiKey") + }) + + test("T7: retries a rejected secret purge once and surfaces it without failing the delete", async () => { + const { profiles } = mockProviderSettingsManager() + + // The profile being deleted declares `openRouterApiKey`; the survivor is Anthropic, so + // the keep-set is empty and the purge targets that 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", openRouterApiKey: "deleted-key" } + } + return { ...currentProfile, name: "current-config", apiKey: "current-key" } + }) + + const purgeSpy = vi + .spyOn(provider.contextProxy, "purgeProviderSecrets") + .mockRejectedValueOnce(new Error("secrets.delete failed")) + .mockRejectedValueOnce(new Error("secrets.delete failed")) + + // The delete itself must still succeed: the profile is already gone from the manager. + await expect(provider.deleteProviderProfile(otherProfile)).resolves.toBeUndefined() + + expect(profiles.map(({ name }) => name)).toEqual(["current-config"]) + // One retry, then the failure is surfaced instead of silently swallowed. + expect(purgeSpy).toHaveBeenCalledTimes(2) + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( + expect.stringContaining("Failed to purge the secrets of the deleted profile 'other-config'"), + ) + }) }) }) From 311e784aed5582e2f7cf5861b855a74559b7f45f Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 04:53:00 +0900 Subject: [PATCH 19/26] 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() + } + }) }) }) From 773ef7c0620997cc9ee89ed06d5b359d63f324dd Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 05:03:32 +0900 Subject: [PATCH 20/26] refactor(core): trim the profile-purge delta to the mutation budget What: rewrap the comment blocks that sit inside counted call spans, restore the early-return shape of `applyProviderProfileRestore` (the `if (config) { ... }` form re-indented a 15-line body into the changed set), and drop the redundant `keep.size === deletedKeys.length` early exit in `getSurvivingProviderSecretKeys`, which only skipped work the loop already skips. Why: the cumulative mutation gate measured 507 changed executable lines against the merge base (limit 500). Each of these is behavior-preserving: the guard removal only drops an optimization, and the rest is comment wrapping plus indentation. Impact: gate 507 -> 498 (-9). No keep-set semantics, fail-closed fallback, discriminator, retry, or fence placement changed. The 7 T1-T7 regressions and the 351-test targeted suite stay green. --- src/core/webview/ClineProvider.ts | 138 ++++++++++++++---------------- 1 file changed, 64 insertions(+), 74 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3196c08eb2..c2321d19ce 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2254,9 +2254,8 @@ export class ClineProvider removedModeConfigs, ) - // Fenced behind the abort, and read from the manager rather than the - // `listApiConfigMeta` cache, which never observed a profile created while the - // purge held the lock. + // Fenced behind the abort, and read from the manager rather than the cache, + // which never observed a profile created while the purge held the lock. await this.contextProxy.setValues({ listApiConfigMeta: await this.providerSettingsManager.listConfig(), modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), @@ -2266,18 +2265,15 @@ export class ClineProvider // 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. + // clear-out: without an explicit purge the secrets stay in the cache and in + // SecretStorage until some unrelated future write names a provider that does not + // own them, which may never happen. // // The keep-set comes from the providers of the profiles that remain, not from the - // active profile's 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 a - // remaining profile share a provider and a key name, the slot they would purge - // *is* that profile's credential. + // active blob: that round-trip strips secret fields while keeping `apiProvider`, so + // a surviving OpenRouter blob declares no `openRouterApiKey` even though the slot + // holds its live value. A shared provider+key means the slot they would purge *is* + // that profile's credential. await this.purgeDeletedProfileSecrets( undoConfig, await this.getSurvivingProviderSecretKeys(profileToDelete.name, undoConfig), @@ -2289,8 +2285,8 @@ export class ClineProvider return } - // Read the list from the manager rather than the `listApiConfigMeta` cache: the - // cache only observes writes this provider made. + // Read the list from the manager, not the cache: the cache only observes this + // provider's own writes. await this.contextProxy.setValues({ listApiConfigMeta: (await this.providerSettingsManager.listConfig()).filter( ({ name }) => name !== profileToDelete.name, @@ -2308,8 +2304,8 @@ export class ClineProvider ({ 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. + // 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 @@ -2332,31 +2328,29 @@ export class ClineProvider 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. + // 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) - // 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. + // 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'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. + // 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. // // 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. It spans every - // remaining profile, so a same-provider profile that is not the replacement keeps its - // credential too. + // blob-derived set is empty when the replacement's blob came from a secrets-stripped + // snapshot, and purging then deletes its live credential from the shared slot. It spans + // every remaining profile, so a same-provider profile that is not the replacement keeps + // its credential too. await this.purgeDeletedProfileSecrets( deletedProfileConfig, await this.getSurvivingProviderSecretKeys(profileToDelete.name, deletedProfileConfig), @@ -2425,9 +2419,7 @@ export class ClineProvider try { for (const { name } of await this.providerSettingsManager.listConfig()) { - if (name === deletedName || keep.size === deletedKeys.length) { - continue - } + if (name === deletedName) continue try { const { apiProvider } = await this.providerSettingsManager.getProfile({ name }) @@ -2539,17 +2531,15 @@ export class ClineProvider if (restored) { this.log(`Restored provider profile '${name}' after a failed replacement activation`) } else { - // The restore did not finish, so both stores may still name a profile the - // manager 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. + // The restore did not finish, so both stores may still name a profile the manager + // 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, previousContextValues) } // The replacement activation posted state before this rollback landed, and the - // rollback's own writes (the restore slice or the reconcile's fallback) change what - // the webview must show. Without a post here the webview keeps naming the - // replacement while both stores name the restored or reconciled profile. + // rollback's writes change what the webview must show: without a post it keeps + // naming the replacement while both stores name the restored profile. await this.postStateToWebview() }, { runToCompletion: true, abortOnTimeout: true }, @@ -2627,48 +2617,48 @@ 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) { + // The inactive branch never captures a config, so there is nothing to restore: writing + // the `previousContextValues` snapshot would re-mirror a list that predates any profile + // created while the purge held the lock. + 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.contextProxy.setValues(previousContextValues) - return true + await this.providerSettingsManager.setModeConfig(mode as Mode, configId) } - // The inactive branch never captures a config, so there is nothing to restore: writing the - // `previousContextValues` snapshot would re-mirror a list that predates any profile created - // while the purge held the lock. - return false + if (signal.aborted) return false + + await this.contextProxy.setValues(previousContextValues) + return true } /** From 20d0812942eb473934915afb5a914a72c54964fb Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 07:28:28 +0900 Subject: [PATCH 21/26] fix(core): close the profile-purge reconcile races CodeRabbit flagged What: - ProviderSettingsManager.setModeConfig takes `onlyIfUnbound`, performing the profile-existence check, the mode-still-unbound check and the write under one lock. `hasConfigById` is deleted; its only caller was the reconcile. - reconcileActiveProfileAfterFailedRestore gates the fallback activation on the deleted profile's name instead of the pre-delete selection, so a failed inactive delete keeps the unchanged active profile. - The inactive delete's list mirror drops its redundant name filter: `listConfig()` already excludes the deleted name, and the filter dropped a same-name profile a direct manager write saved during the purge. - The timed-out inactive branch mirrors the manager only when its view diverged from the cache, instead of always writing a snapshot that can be older than a newer mutation's write. Why: CodeRabbit's round-3 review opened 7 threads on #1735/#2266. Five describe real races; two prescriptions were falsified by probe (purging owned secrets on a complete activation re-breaks the round-4 same-provider data loss; skipping the aborted mirror disables the T5 convergence contract). Impact: Both stores stay convergent through a timed-out delete and a failed restore, and a newer mode binding or selection is never overwritten by a rollback. Gate stays at 498/500 measured, with no defense removed. Adds T8-T11 plus four manager-level tests; all are red on the previous head. --- src/core/config/ProviderSettingsManager.ts | 32 ++-- .../__tests__/ProviderSettingsManager.spec.ts | 52 ++++++ src/core/webview/ClineProvider.ts | 72 ++++---- .../webview/__tests__/ClineProvider.spec.ts | 164 +++++++++++++++++- 4 files changed, 264 insertions(+), 56 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index f3809c8dbe..65a7220a95 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -589,28 +589,27 @@ 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. + * + * `onlyIfUnbound` makes the write conditional, for the rollback that re-applies the bindings a + * purge dropped. That rollback must not overwrite a mode a newer selection has already bound + * (its queue advances at its timeout, so a newer binding can commit while the aborted rollback + * still runs), and it must not re-apply a binding whose profile no longer exists (the restore's + * `saveConfig` can reject, and a dangling id makes the next switch to that mode resolve nothing + * and silently keep the current profile). Both checks and the write share this one lock, so + * neither can straddle a concurrent purge or rename. Returns whether the write happened. */ - public async setModeConfig(mode: Mode, configId: string) { + public async setModeConfig(mode: Mode, configId: string, { onlyIfUnbound = false } = {}): Promise { try { return await this.lock(async () => { const providerProfiles = await this.load() + + if (onlyIfUnbound) { + const exists = Object.values(providerProfiles.apiConfigs).some((config) => config.id === configId) + if (!exists || providerProfiles.modeApiConfigs?.[mode] !== undefined) return false + } + // Ensure the per-mode config map exists if (!providerProfiles.modeApiConfigs) { providerProfiles.modeApiConfigs = {} @@ -618,6 +617,7 @@ export class ProviderSettingsManager { // Assign the chosen config ID to this mode providerProfiles.modeApiConfigs[mode] = configId await this.store(providerProfiles) + return true }) } catch (error) { throw new Error(`Failed to set mode config: ${error}`) diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index 9b1a903ae6..1212788443 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -1456,6 +1456,58 @@ describe("ProviderSettingsManager", () => { }) }) + describe("setModeConfig with onlyIfUnbound", () => { + // The reconcile's re-apply is the only caller. Both skips are the point of the option: a + // newer selection may have bound the mode while the rollback ran, and the binding's profile + // may no longer exist because the restore's `saveConfig` rejected. + const profilesWith = (modeApiConfigs?: Record): ProviderProfiles => ({ + currentApiConfigName: "default", + apiConfigs: { + default: { id: "default" }, + test: { apiProvider: providerIdentifiers.anthropic, id: "test-id" }, + }, + modeApiConfigs, + migrations: { rateLimitSecondsMigrated: false }, + }) + + const storedProfiles = () => JSON.parse(mockSecrets.store.mock.calls.at(-1)![1] as string) as ProviderProfiles + + it("binds the mode when it is unbound and the profile exists", async () => { + mockSecrets.get.mockResolvedValue(JSON.stringify(profilesWith())) + + await expect( + providerSettingsManager.setModeConfig("code" as never, "test-id", { onlyIfUnbound: true }), + ).resolves.toBe(true) + expect(storedProfiles().modeApiConfigs).toEqual({ code: "test-id" }) + }) + + it("leaves a newer selection's binding alone", async () => { + mockSecrets.get.mockResolvedValue(JSON.stringify(profilesWith({ code: "newer-id" }))) + + await expect( + providerSettingsManager.setModeConfig("code" as never, "test-id", { onlyIfUnbound: true }), + ).resolves.toBe(false) + // Nothing was written, so the newer binding survives. + expect(mockSecrets.store).not.toHaveBeenCalled() + }) + + it("skips a binding whose profile no longer exists", async () => { + mockSecrets.get.mockResolvedValue(JSON.stringify(profilesWith())) + + await expect( + providerSettingsManager.setModeConfig("code" as never, "gone-id", { onlyIfUnbound: true }), + ).resolves.toBe(false) + expect(mockSecrets.store).not.toHaveBeenCalled() + }) + + it("still overwrites unconditionally without the option", async () => { + mockSecrets.get.mockResolvedValue(JSON.stringify(profilesWith({ code: "newer-id" }))) + + await expect(providerSettingsManager.setModeConfig("code" as never, "test-id")).resolves.toBe(true) + expect(storedProfiles().modeApiConfigs).toEqual({ code: "test-id" }) + }) + }) + describe("syncCloudProfiles", () => { it("should add new cloud profiles without secret keys", async () => { const existingConfig: ProviderProfiles = { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index c2321d19ce..6710bee96c 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -210,6 +210,9 @@ type GetStateOptions = { includeTaskHistory?: boolean } +/** The ContextProxy snapshot a failed delete rolls back to. */ +type ProviderProfileSnapshot = Pick + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -2184,10 +2187,7 @@ 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< - RooCodeSettings, - "currentApiConfigName" | "listApiConfigMeta" | "modeApiConfigs" - > = {} + let previousContextValues: ProviderProfileSnapshot = {} // The mode bindings the purge dropped from the manager, so the rollback can re-apply them. let removedModeConfigs: Record = {} @@ -2248,18 +2248,18 @@ export class ClineProvider // 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, - ) - - // Fenced behind the abort, and read from the manager rather than the cache, - // which never observed a profile created while the purge held the lock. - await this.contextProxy.setValues({ - listApiConfigMeta: await this.providerSettingsManager.listConfig(), - modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), - }) + await this.undoCommittedProviderProfilePurge(profileToDelete.name, undoConfig, removedModeConfigs) + + // The undo restored the pre-delete profile, so the manager's view needs mirroring + // only when it holds a profile the cache never observed. An unconditional write + // would land a snapshot older than a newer mutation's write and replace it. + const listApiConfigMeta = await this.providerSettingsManager.listConfig() + if (listApiConfigMeta.length !== (this.contextProxy.getValues().listApiConfigMeta?.length ?? 0)) { + await this.contextProxy.setValues({ + listApiConfigMeta, + modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), + }) + } return } @@ -2286,11 +2286,10 @@ export class ClineProvider } // Read the list from the manager, not the cache: the cache only observes this - // provider's own writes. + // provider's own writes. `listConfig()` already excludes the deleted name, so a + // filter here would drop a same-name profile saved during the purge. await this.contextProxy.setValues({ - listApiConfigMeta: (await this.providerSettingsManager.listConfig()).filter( - ({ name }) => name !== profileToDelete.name, - ), + listApiConfigMeta: await this.providerSettingsManager.listConfig(), modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), }) @@ -2498,7 +2497,7 @@ export class ClineProvider private async restoreDeletedProviderProfile( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: ProviderProfileSnapshot, removedModeConfigs: Record, ): Promise { // Run the rollback through the mutation queue: it used to race queued activations, and its @@ -2534,7 +2533,7 @@ export class ClineProvider // The restore did not finish, so both stores may still name a profile the manager // 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, previousContextValues) + await this.reconcileActiveProfileAfterFailedRestore(name, removedModeConfigs) } // The replacement activation posted state before this rollback landed, and the @@ -2613,7 +2612,7 @@ export class ClineProvider private async applyProviderProfileRestore( name: string, config: (ProviderSettingsWithId & { name: string }) | undefined, - previousContextValues: Pick, + previousContextValues: ProviderProfileSnapshot, removedModeConfigs: Record, signal: AbortSignal, ): Promise { @@ -2673,21 +2672,20 @@ export class ClineProvider * keep pointing a mode at the deleted profile after the purge dropped the binding in the manager. * * The fallback activation is the only write here that can *replace* a profile, so it is gated on - * the selection being a newer one: a name the restore was not trying to restore, which the - * manager still holds, was picked by someone else while the restore was pending and needs no - * replacement. The `hasConfigById`-guarded re-applies and the mirror below are convergence - * writes over ids that provably exist, so they stay ungated: an aborted restore must still - * reconcile the mode bindings, which is the contract the head spec pins. + * the deleted name: any other selection the manager still holds was never part of this delete, + * or was picked by someone else while the restore was pending. The `onlyIfUnbound` re-applies + * and the mirror below are convergence writes over ids that exist and modes no newer selection + * has bound, so they stay ungated: an aborted restore must still reconcile the mode bindings. */ private async reconcileActiveProfileAfterFailedRestore( + deletedName: string, removedModeConfigs: Record = {}, - previousContextValues: Pick = {}, ): Promise { try { const selectedName = this.contextProxy.getValues().currentApiConfigName const keepSelection = !!selectedName && - selectedName !== previousContextValues.currentApiConfigName && + selectedName !== deletedName && (await this.providerSettingsManager.hasConfig(selectedName)) if (!keepSelection) { @@ -2711,15 +2709,13 @@ export class ClineProvider }) } // 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. 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. + // them now so the manager's map matches the pre-delete state again. The check and the + // write share one manager lock: a newer selection can bind the mode while this + // rollback runs, and a binding whose profile no longer exists must not be re-applied. + // 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)) { - if (await this.providerSettingsManager.hasConfigById(configId)) { - await this.providerSettingsManager.setModeConfig(mode as Mode, configId) - } + await this.providerSettingsManager.setModeConfig(mode as Mode, configId, { onlyIfUnbound: true }) } await this.contextProxy.setValue("modeApiConfigs", await this.providerSettingsManager.getModeApiConfigs()) } catch (reconcileError) { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 9ac0b85814..29b7903b12 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3804,9 +3804,21 @@ describe("ClineProvider", () => { return { ...config, name } }), ), - setModeConfig: vi.fn().mockImplementation((mode: string, configId: string) => + // The single-lock form the reconcile uses: the unbound check, the existence check + // and the write are one step, so neither a newer binding nor a purge can land + // between them. + setModeConfig: vi.fn().mockImplementation((mode: string, configId: string, options?: { onlyIfUnbound?: boolean }) => withLock(async () => { + if (options?.onlyIfUnbound) { + if (modeApiConfigs[mode] !== undefined) { + return false + } + if (!Object.values(Object.fromEntries(configs)).some((config) => config.id === configId)) { + return false + } + } modeApiConfigs[mode] = configId + return true }), ), getModeConfigId: vi @@ -4528,6 +4540,7 @@ describe("ClineProvider", () => { }) vi.mocked(provider.providerSettingsManager.setModeConfig).mockImplementationOnce(async () => { await pendingModeWrite + return true }) const deletion = provider.deleteProviderProfile(currentProfile) @@ -4797,7 +4810,7 @@ describe("ClineProvider", () => { // 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(await provider.providerSettingsManager.listConfig()).toEqual([otherProfile]) expect(modeApiConfigs).toEqual({}) expect(provider.contextProxy.getValues().modeApiConfigs).toEqual({}) }) @@ -4948,6 +4961,153 @@ describe("ClineProvider", () => { expect.stringContaining("Failed to purge the secrets of the deleted profile 'other-config'"), ) }) + + // --- Round-8 per-thread regressions (CodeRabbit #1735 round-3 threads) --- + + test("T8: a failed inactive delete keeps the unchanged active profile instead of activating another", async () => { + // The inactive branch never captures a config, so `applyProviderProfileRestore` reports + // false and the reconcile runs. Its fallback (`listConfig()[0]`) is a *different* profile + // here: without a gate on the deleted name the reconcile treats the unchanged active + // selection as "not newer than the pre-delete one" and switches the user away from it. + const { managerState } = mockProviderSettingsManager() + + // `listConfig()[0]` is the profile the reconcile would wrongly activate. + const thirdProfile: ProviderSettingsEntry = { + name: "third-config", + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + } + vi.mocked(provider.providerSettingsManager.listConfig).mockImplementation(async () => [ + thirdProfile, + currentProfile, + ]) + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementation( + async (params: { name: string } | { id: string }) => { + const name = "name" in params ? params.name : thirdProfile.name + managerState.currentApiConfigName = name + return { ...thirdProfile, name } + }, + ) + + // The purge fails on an inactive delete, so the rollback runs with no captured config. + vi.mocked(provider.providerSettingsManager.deleteConfig).mockRejectedValueOnce( + new Error("Failed to delete config: storage error"), + ) + + await expect(provider.deleteProviderProfile(otherProfile)).rejects.toThrow( + "Failed to delete config: storage error", + ) + + // The active profile was never part of this delete, so it must still be selected. + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(managerState.currentApiConfigName).toBe("current-config") + }) + + test("T9: the inactive delete's list mirror does not filter out a same-name profile saved during the purge", async () => { + // `listConfig()` is read *after* the purge, so it already excludes the deleted name. A + // name filter on top of it drops a profile that a direct manager write re-created under + // the same name during the purge: the manager keeps it, the webview loses it. + const { profiles, withLock, deleteConfigUnlocked } = mockProviderSettingsManager() + + // The purge succeeds, and a same-name profile is saved while it holds the lock: the + // manager's list therefore contains the re-created `other-config`. The write goes + // straight into the harness's arrays because a `saveConfig` call would re-enter the + // lock this step already holds. + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce((name: string) => + withLock(async () => { + const result = await deleteConfigUnlocked(name) + profiles.push({ name, id: "recreated-id", apiProvider: providerIdentifiers.openrouter }) + return result + }), + ) + + await provider.deleteProviderProfile(otherProfile) + + // The manager holds the re-created profile, so the webview's list must too. + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + expect( + provider + .getProviderProfileEntries() + .map(({ name }) => name) + .sort(), + ).toEqual(["current-config", "other-config"]) + }) + + test("T10: the reconcile re-applies a dropped mode binding through the single-lock guarded method", async () => { + // The rollback queue advances at the restore timeout, so a newer mode binding can commit + // while the aborted restore still runs. Re-applying the pre-delete binding with a plain + // `setModeConfig` would overwrite that newer selection with a stale id, and a separate + // existence check would let a purge land between the check and the write. The reconcile + // must therefore route every re-apply through the one locked, guarded manager method. + // Its semantics (skip when bound, skip when the id is gone) are proven against the real + // manager in ProviderSettingsManager.spec.ts. + const { modeApiConfigs } = mockProviderSettingsManager() + modeApiConfigs.code = currentProfile.id + await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) + + // The replacement activation fails and the restore's own `saveConfig` rejects, so the + // reconcile runs with the purge's dropped binding. + 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 guarded method received the purge's dropped binding, and no unguarded write ran. + expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalledWith("code", currentProfile.id, { + onlyIfUnbound: true, + }) + }) + + test("T11: a timed-out inactive delete does not re-mirror a list it already converged", async () => { + // The aborted branch used to re-read the manager and write the cache unconditionally. + // Those reads sit behind the manager lock, so a newer mutation can write between them + // and the `setValues`, and the late write then replaces the newer metadata with the + // older snapshot. When the undo restored the pre-delete profile, the manager's view is + // exactly what the cache already holds, so the write must not happen at all. + vi.useFakeTimers() + + try { + const { profiles, withLock, deleteConfigUnlocked } = mockProviderSettingsManager() + const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") + + // Hold the purge past the mutation window so the caller is told the delete timed out + // and the abort fires while the purge still holds the lock. + 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) + + releasePurge() + for (let i = 0; i < 50; i++) { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(0) + } + await deletionResult + + // The undo restored the profile, so the manager and the cache already agree: no late + // `setValues` may land after the abort. + expect(profiles.map(({ name }) => name).sort()).toEqual(["current-config", "other-config"]) + expect(setValuesSpy).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) }) }) From c23dde1642a504b665d9bd0459a6a993609ccf94 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 08:54:22 +0900 Subject: [PATCH 22/26] style(core): satisfy prettier on the fix-8 delta --- src/core/webview/ClineProvider.ts | 40 +++++++++---------- .../webview/__tests__/ClineProvider.spec.ts | 32 +++++++++------ 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6710bee96c..7733097713 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2204,8 +2204,7 @@ export class ClineProvider let deleteBodySettled: Promise = Promise.resolve() try { 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. + // Read inside the queue: a queued activation may have changed the active profile. const globalSettings = this.contextProxy.getValues() const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName @@ -2216,18 +2215,15 @@ 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. + // A timeout may have fired while queued or while an earlier manager call held + // the lock. Purging now would delete the profile after the caller was told it 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. + // Captured before the purge so a timeout firing under the manager lock can undo a + // committed purge; the inactive branch activates no replacement to converge the + // two stores. Best-effort: a manager that cannot produce it leaves nothing to undo. let undoConfig: (ProviderSettingsWithId & { name: string }) | undefined try { undoConfig = await this.providerSettingsManager.getProfile({ name: profileToDelete.name }) @@ -2239,22 +2235,27 @@ export class ClineProvider return } - // Purge first so a failing purge fails fast before any ContextProxy write, - // matching the ordering the webview delete path established. + // 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 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. + // A timeout may have fired under the manager lock. The purge committed, so the + // caller must not be left with a deleted profile it was told survived: undo it. if (signal.aborted) { - await this.undoCommittedProviderProfilePurge(profileToDelete.name, undoConfig, removedModeConfigs) + await this.undoCommittedProviderProfilePurge( + profileToDelete.name, + undoConfig, + removedModeConfigs, + ) // The undo restored the pre-delete profile, so the manager's view needs mirroring // only when it holds a profile the cache never observed. An unconditional write // would land a snapshot older than a newer mutation's write and replace it. const listApiConfigMeta = await this.providerSettingsManager.listConfig() - if (listApiConfigMeta.length !== (this.contextProxy.getValues().listApiConfigMeta?.length ?? 0)) { + if ( + listApiConfigMeta.length !== (this.contextProxy.getValues().listApiConfigMeta?.length ?? 0) + ) { await this.contextProxy.setValues({ listApiConfigMeta, modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), @@ -2328,9 +2329,8 @@ export class ClineProvider .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. + // config, and current-task wiring match a user activating the replacement from the + // webview. The unlocked variant is used because this already runs in 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 diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 29b7903b12..3891824cc4 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3807,20 +3807,26 @@ describe("ClineProvider", () => { // The single-lock form the reconcile uses: the unbound check, the existence check // and the write are one step, so neither a newer binding nor a purge can land // between them. - setModeConfig: vi.fn().mockImplementation((mode: string, configId: string, options?: { onlyIfUnbound?: boolean }) => - withLock(async () => { - if (options?.onlyIfUnbound) { - if (modeApiConfigs[mode] !== undefined) { - return false - } - if (!Object.values(Object.fromEntries(configs)).some((config) => config.id === configId)) { - return false + setModeConfig: vi + .fn() + .mockImplementation((mode: string, configId: string, options?: { onlyIfUnbound?: boolean }) => + withLock(async () => { + if (options?.onlyIfUnbound) { + if (modeApiConfigs[mode] !== undefined) { + return false + } + if ( + !Object.values(Object.fromEntries(configs)).some( + (config) => config.id === configId, + ) + ) { + return false + } } - } - modeApiConfigs[mode] = configId - return true - }), - ), + modeApiConfigs[mode] = configId + return true + }), + ), getModeConfigId: vi .fn() .mockImplementation((mode: string) => withLock(async () => modeApiConfigs[mode])), From e9046d8d812f745eebd6d72dbf6482838698720f Mon Sep 17 00:00:00 2001 From: tiebreak-probe Date: Sat, 31 Oct 2026 12:58:40 +0900 Subject: [PATCH 23/26] fix(core): complete the profile-purge compensation path The queued delete body committed a purge before its second abort check and returned without undoing it, so a timeout during the secret purge left the profile deleted while the caller was told the delete failed. The same body deleted by name without revalidating the id the caller selected, and its post-purge tail awaited the manager reads after the abort check, so a timeout landing during them still wrote a stale list. Both abort checks now share one undo closure that re-reads the manager's list behind the lock and mirrors it only when the entries differ, so a same-name recreation is caught and a converged cache is left alone. The restore uses the conditional save, the undo re-applies dropped bindings with onlyIfUnbound, and the timeout log branches on the same predicate as the abort decision. Specs follow the restore's switch to saveConfigIfAbsent and the pre-purge profile read. --- src/core/webview/ClineProvider.ts | 116 +++++++++--------- .../webview/__tests__/ClineProvider.spec.ts | 50 +++++--- 2 files changed, 93 insertions(+), 73 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 7733097713..383dc59fdd 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -213,6 +213,10 @@ type GetStateOptions = { /** The ContextProxy snapshot a failed delete rolls back to. */ type ProviderProfileSnapshot = Pick +/** Whether two profile lists name the same profiles with the same ids, so a mirror would be a no-op. */ +const entriesEqual = (left: ProviderSettingsEntry[], right: ProviderSettingsEntry[] = []): boolean => + left.length === right.length && left.every((entry, i) => entry.name === right[i]?.name && entry.id === right[i]?.id) + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -310,14 +314,16 @@ export class ClineProvider () => fn(controller.signal), ) const callerResult = this.withProviderProfileMutationTimeout(run, () => { - if (!options.runToCompletion || options.abortOnTimeout) { - controller.abort() - } + // The abort and the queue advance both treat `abortOnTimeout` as "the body stops here", + // so the log must too: branching on `runToCompletion` alone reports that the queue is + // waiting for a body that was just aborted. + const abortsBody = !options.runToCompletion || options.abortOnTimeout + if (abortsBody) controller.abort() 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", + abortsBody + ? "Provider profile mutation timed out; aborting in-flight mutation" + : "Provider profile mutation timed out; waiting for it to finish before the next mutation", ) }) @@ -2214,6 +2220,33 @@ export class ClineProvider modeApiConfigs: globalSettings.modeApiConfigs, } + // Read the stored profile once, before either branch purges: it is both the undo + // snapshot and the identity check. A failed read must reject the delete, because a + // delete that purges without the blob leaves the profile's credentials behind. A + // same-name profile saved after the caller picked this name carries a different id, + // so deleting by name alone would destroy that newer profile. + const storedProfile = await this.providerSettingsManager.getProfile({ name: profileToDelete.name }) + if (profileToDelete.id && storedProfile.id !== profileToDelete.id) return + + // Both abort checks below undo a committed purge the same way. The mirror is + // conditional on the entries, not on the abort: this runs *because* the body was + // aborted, so an abort guard here would make the mirror dead code and strand the + // cache on a list that predates a profile saved while the purge held the lock. + // Comparing name+id instead of the length catches a same-name recreation, which + // keeps the length and changes the id. + const undoPurge = async () => { + await this.undoCommittedProviderProfilePurge( + profileToDelete.name, + storedProfile, + removedModeConfigs, + ) + const listApiConfigMeta = await this.providerSettingsManager.listConfig() + const modeApiConfigs = await this.providerSettingsManager.getModeApiConfigs() + if (!entriesEqual(listApiConfigMeta, this.contextProxy.getValues().listApiConfigMeta)) { + await this.contextProxy.setValues({ listApiConfigMeta, modeApiConfigs }) + } + } + if (!deletedActiveProfile) { // A timeout may have fired while queued or while an earlier manager call held // the lock. Purging now would delete the profile after the caller was told it failed. @@ -2221,20 +2254,6 @@ export class ClineProvider return } - // Captured before the purge so a timeout firing under the manager lock can undo a - // committed purge; the inactive branch activates no replacement to converge the - // two stores. Best-effort: a manager that cannot produce it 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)) @@ -2243,24 +2262,7 @@ export class ClineProvider // A timeout may have fired under the manager lock. The purge committed, so the // caller must not be left with a deleted profile it was told survived: undo it. if (signal.aborted) { - await this.undoCommittedProviderProfilePurge( - profileToDelete.name, - undoConfig, - removedModeConfigs, - ) - - // The undo restored the pre-delete profile, so the manager's view needs mirroring - // only when it holds a profile the cache never observed. An unconditional write - // would land a snapshot older than a newer mutation's write and replace it. - const listApiConfigMeta = await this.providerSettingsManager.listConfig() - if ( - listApiConfigMeta.length !== (this.contextProxy.getValues().listApiConfigMeta?.length ?? 0) - ) { - await this.contextProxy.setValues({ - listApiConfigMeta, - modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), - }) - } + await undoPurge() return } @@ -2276,23 +2278,24 @@ export class ClineProvider // holds its live value. A shared provider+key means the slot they would purge *is* // that profile's credential. await this.purgeDeletedProfileSecrets( - undoConfig, - await this.getSurvivingProviderSecretKeys(profileToDelete.name, undoConfig), + storedProfile, + await this.getSurvivingProviderSecretKeys(profileToDelete.name, storedProfile), ) - // 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. + // The purge above can outlast the mutation timeout, and the reads below sit behind + // the manager lock, so the abort can fire during either: re-check once after both, + // then undo the purge this body already committed rather than landing a late + // `setValues` after a newer queued mutation. Reading before the check is safe: + // both calls are side-effect free. + const listApiConfigMeta = await this.providerSettingsManager.listConfig() + const modeApiConfigs = await this.providerSettingsManager.getModeApiConfigs() + if (signal.aborted) { + await undoPurge() return } - // Read the list from the manager, not the cache: the cache only observes this - // provider's own writes. `listConfig()` already excludes the deleted name, so a - // filter here would drop a same-name profile saved during the purge. - await this.contextProxy.setValues({ - listApiConfigMeta: await this.providerSettingsManager.listConfig(), - modeApiConfigs: await this.providerSettingsManager.getModeApiConfigs(), - }) + await this.contextProxy.setValues({ listApiConfigMeta, modeApiConfigs }) await this.postStateToWebview() return @@ -2315,9 +2318,7 @@ export class ClineProvider } // Capture the profile before purging so a failed active-profile switch can restore it. - deletedProfileConfig = await this.providerSettingsManager.getProfile({ - name: profileToDelete.name, - }) + deletedProfileConfig = storedProfile // 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. @@ -2354,7 +2355,6 @@ export class ClineProvider deletedProfileConfig, await this.getSurvivingProviderSecretKeys(profileToDelete.name, deletedProfileConfig), ) - // 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 @@ -2584,8 +2584,10 @@ export class ClineProvider return false } + // A newer selection can bind the mode while the purge holds the lock, so the re-apply + // must not overwrite it: the guarded form skips a mode that is already bound. for (const [mode, configId] of Object.entries(removedModeConfigs)) { - await this.providerSettingsManager.setModeConfig(mode as Mode, configId) + await this.providerSettingsManager.setModeConfig(mode as Mode, configId, { onlyIfUnbound: true }) } this.log(`Undid a provider profile purge for '${name}' that committed after the caller timed out`) @@ -2624,8 +2626,12 @@ export class ClineProvider } // Keep `id`: the manager reuses it for the mode associations that point at this profile. + // The conditional save makes the check and the write one lock: a same-name profile saved + // outside the mutation queue during the replacement activation must not be overwritten by + // the pre-delete snapshot. When the name is taken the restore is skipped and the reconcile + // converges from the manager's view instead. const { name: _name, ...providerSettings } = config - await this.providerSettingsManager.saveConfig(name, providerSettings) + if ((await this.providerSettingsManager.saveConfigIfAbsent(name, providerSettings)) === undefined) return false if (signal.aborted) return false diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 3891824cc4..a6f61badfb 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3595,6 +3595,13 @@ describe("ClineProvider", () => { Object.assign(provider, { providerSettingsManager: { + // The queued delete body reads the stored profile first: it is both the undo + // snapshot and the identity check against the entry the caller selected. + getProfile: vi.fn().mockResolvedValue({ + name: "deleted-config", + id: "deleted-id", + apiProvider: providerIdentifiers.anthropic, + }), deleteConfig: vi.fn().mockImplementation(async () => { const removedModeConfigs = { code: modeApiConfigs.code } delete modeApiConfigs.code @@ -4121,6 +4128,11 @@ describe("ClineProvider", () => { test("fails fast without writing ContextProxy state when the config purge rejects", async () => { Object.assign(provider, { providerSettingsManager: { + // The queued body reads the stored profile before it purges, so the double has + // to answer that read for the purge below to be the call that rejects. + getProfile: vi + .fn() + .mockResolvedValue({ ...otherProfile, openRouterModelId: "other-model", apiKey: "other-key" }), deleteConfig: vi.fn().mockRejectedValue(new Error("Config 'other-config' not found")), listConfig: vi.fn().mockResolvedValue([currentProfile, otherProfile]), }, @@ -4157,8 +4169,10 @@ describe("ClineProvider", () => { ) // 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( + // including the credential: a restore that dropped `apiKey` fails here. The restore + // goes through the conditional save, so a same-name profile saved during the window + // is not overwritten by the pre-delete snapshot. + expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalledWith( "current-config", expect.objectContaining({ id: "current-id", @@ -4204,7 +4218,7 @@ describe("ClineProvider", () => { // The timeout path restores the purged profile instead of leaving ContextProxy // naming a profile the manager no longer has. The credential must survive too. - expect(provider.providerSettingsManager.saveConfig).toHaveBeenCalledWith( + expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalledWith( "current-config", expect.objectContaining({ id: "current-id", @@ -4257,14 +4271,14 @@ describe("ClineProvider", () => { // 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() + expect(provider.providerSettingsManager.saveConfigIfAbsent).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( + expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalledWith( "current-config", expect.objectContaining({ id: "current-id" }), ) @@ -4365,14 +4379,14 @@ describe("ClineProvider", () => { vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( new Error("Failed to activate profile: storage error"), ) - vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).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()) + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalled()) // Queue a newer mutation while the rollback is still pending. const queued = provider.activateProviderProfile({ name: "other-config" }) @@ -4473,19 +4487,19 @@ describe("ClineProvider", () => { 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 + // own `saveConfigIfAbsent` 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( + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).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()) + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalled()) // Queue a newer mutation behind the hung rollback. const queued = provider.activateProviderProfile({ name: "other-config" }) @@ -4744,14 +4758,14 @@ describe("ClineProvider", () => { const pendingRestore = new Promise((resolve) => { releaseRestore = resolve }) - vi.mocked(provider.providerSettingsManager.saveConfig).mockImplementationOnce(async () => { + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).mockImplementationOnce(async () => { await pendingRestore return currentProfile.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()) + await vi.waitFor(() => expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalled()) // The user selects the surviving profile while the restore is still pending. The // manager holds that name, so the reconcile's gate sees a live newer selection. @@ -4801,12 +4815,12 @@ describe("ClineProvider", () => { 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). + // The replacement activation fails, and the restore's own `saveConfigIfAbsent` 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( + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).mockRejectedValueOnce( new Error("Failed to save config: storage error"), ) @@ -5051,12 +5065,12 @@ describe("ClineProvider", () => { modeApiConfigs.code = currentProfile.id await provider.contextProxy.setValue("modeApiConfigs", { ...modeApiConfigs }) - // The replacement activation fails and the restore's own `saveConfig` rejects, so the - // reconcile runs with the purge's dropped binding. + // The replacement activation fails and the restore's own `saveConfigIfAbsent` rejects, so + // the reconcile runs with the purge's dropped binding. vi.mocked(provider.providerSettingsManager.activateProfile).mockRejectedValueOnce( new Error("Failed to activate profile: storage error"), ) - vi.mocked(provider.providerSettingsManager.saveConfig).mockRejectedValueOnce( + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).mockRejectedValueOnce( new Error("Failed to save config: storage error"), ) From a7301f3963c6ef1ffa4308c72854365a2af2e000 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 21:16:14 +0900 Subject: [PATCH 24/26] fix(core): keep the active profile when a delete fails before its purge What: the reconcile's keep gate now also checks that the deleted name still exists in the manager, so a selection equal to the deleted name is kept when the delete never committed. Why: a rejected pre-purge `getProfile` read, or a rejected `deleteConfig` after the capture, reaches the rollback with nothing purged. The gate tested only name inequality, so it forced the fallback and activated `listConfig()[0]` for a delete that never happened. When that fallback is a different provider, its `setProviderSettings` ownership clear-out deletes the surviving profile's live SecretStorage credential. Impact: no spurious profile switch or credential loss on a failed delete. Net 0 counted executable lines. Tests: T12 (pre-purge read rejection) and T13 (deleteConfig rejection) drive both triggers; the two tests that asserted the old fallback behavior now assert the kept selection. --- src/core/webview/ClineProvider.ts | 9 +- .../webview/__tests__/ClineProvider.spec.ts | 118 ++++++++++++++++-- 2 files changed, 113 insertions(+), 14 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 383dc59fdd..3a6c9081cf 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2678,8 +2678,11 @@ export class ClineProvider * keep pointing a mode at the deleted profile after the purge dropped the binding in the manager. * * The fallback activation is the only write here that can *replace* a profile, so it is gated on - * the deleted name: any other selection the manager still holds was never part of this delete, - * or was picked by someone else while the restore was pending. The `onlyIfUnbound` re-applies + * the deleted name *and* on whether that name still exists: a delete that failed before its + * purge (a rejected pre-purge read, or a rejected `deleteConfig`) leaves the profile in the + * manager, so the unchanged selection is not a stale pointer to a deleted profile and must not + * be replaced. Any other selection the manager still holds was never part of this delete, or + * was picked by someone else while the restore was pending. The `onlyIfUnbound` re-applies * and the mirror below are convergence writes over ids that exist and modes no newer selection * has bound, so they stay ungated: an aborted restore must still reconcile the mode bindings. */ @@ -2691,7 +2694,7 @@ export class ClineProvider const selectedName = this.contextProxy.getValues().currentApiConfigName const keepSelection = !!selectedName && - selectedName !== deletedName && + (selectedName !== deletedName || (await this.providerSettingsManager.hasConfig(deletedName))) && (await this.providerSettingsManager.hasConfig(selectedName)) if (!keepSelection) { diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index d6ec6019c8..7573709ff9 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -4323,16 +4323,21 @@ describe("ClineProvider", () => { apiProvider: providerIdentifiers.anthropic, apiKey: "current-key", }) - // The replacement activation fails, and so does the restore, so the fallback runs. + // The replacement activation fails, and the restore fails before it can re-save the + // profile, so the name is genuinely gone and the fallback must run. (A restore that + // fails *after* re-saving leaves the profile in the manager, which the keep gate now + // treats as a selection that must not be replaced.) // 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" } }) + vi.mocked(provider.providerSettingsManager.saveConfigIfAbsent).mockRejectedValueOnce( + new Error("Failed to restore profile: storage error"), + ) await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( "Failed to activate profile: storage error", @@ -4350,9 +4355,10 @@ describe("ClineProvider", () => { // 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 }) + // The purge dropped the bindings and the restore never re-saved the profile, so its id is + // dangling: the guarded re-apply refuses it and both stores agree on the empty map rather + // than pointing a mode at a profile the manager no longer has. + expect(modeApiConfigs).toEqual({}) expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) }) @@ -4566,13 +4572,14 @@ describe("ClineProvider", () => { 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") + // The restore was aborted at the re-apply fence, but it had already re-saved the profile + // under its original id, so the selection is not a stale pointer: the reconcile keeps it + // instead of switching the user to another profile, and re-applies the purge's dropped + // bindings atomically so the mode map agrees in both stores. + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") expect(provider.contextProxy.getProviderSettings()).toMatchObject({ - apiProvider: providerIdentifiers.openrouter, - openRouterModelId: "other-model", + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", }) expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) @@ -5121,6 +5128,95 @@ describe("ClineProvider", () => { vi.useRealTimers() } }) + + test("T12: a delete that fails before any purge keeps the still-present active profile", async () => { + // The pre-purge read rejects, so nothing was deleted: the profile is still in the manager + // and still the user's selection. The rollback's reconcile used to test only name + // inequality, so `selectedName === deletedName` forced the fallback and switched the user + // to `listConfig()[0]` for a delete that never happened. When that fallback is a different + // provider, its `setProviderSettings` clear-out deletes the surviving profile's live + // SecretStorage credential. + const { managerState } = mockProviderSettingsManager() + + // `listConfig()[0]` is the profile a spurious fallback would activate, and it must be + // activatable: a fallback that rejects is caught and logged by the reconcile, which would + // make the test pass without ever exercising the switch. + const thirdProfile: ProviderSettingsEntry = { + name: "third-config", + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + } + vi.mocked(provider.providerSettingsManager.listConfig).mockImplementation(async () => [ + thirdProfile, + currentProfile, + ]) + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementation( + async (params: { name: string } | { id: string }) => { + const name = "name" in params ? params.name : thirdProfile.name + managerState.currentApiConfigName = name + return { ...thirdProfile, name } + }, + ) + vi.mocked(provider.providerSettingsManager.getProfile).mockRejectedValueOnce( + new Error("Failed to read provider profiles from secrets: storage error"), + ) + + await provider.contextProxy.setProviderSettings({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }) + expect(provider.contextProxy.getSecret("apiKey")).toBe("current-key") + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to read provider profiles from secrets: storage error", + ) + + // Nothing was purged, so the delete is a no-op in both stores. + expect(managerState.currentApiConfigName).toBe("current-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(provider.contextProxy.getProviderSettings()).toMatchObject({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "current-key", + }) + expect(mockContext.secrets.delete).not.toHaveBeenCalledWith("apiKey") + }) + + test("T13: a delete whose purge rejects after the capture keeps the still-present active profile", async () => { + // The second trigger: the active branch captured the config before `deleteConfig` + // rejected, so the restore runs with a real config but `saveConfigIfAbsent` finds the name + // still present and reports false. The reconcile then sees the unchanged selection and, + // without the existence conjunct, activates the fallback for a profile that was never + // deleted. + const { managerState } = mockProviderSettingsManager() + + const thirdProfile: ProviderSettingsEntry = { + name: "third-config", + id: "third-id", + apiProvider: providerIdentifiers.openrouter, + } + vi.mocked(provider.providerSettingsManager.listConfig).mockImplementation(async () => [ + thirdProfile, + currentProfile, + ]) + vi.mocked(provider.providerSettingsManager.activateProfile).mockImplementation( + async (params: { name: string } | { id: string }) => { + const name = "name" in params ? params.name : thirdProfile.name + managerState.currentApiConfigName = name + return { ...thirdProfile, name } + }, + ) + vi.mocked(provider.providerSettingsManager.deleteConfig).mockRejectedValueOnce( + new Error("Failed to delete config: storage error"), + ) + + await expect(provider.deleteProviderProfile(currentProfile)).rejects.toThrow( + "Failed to delete config: storage error", + ) + + expect(managerState.currentApiConfigName).toBe("current-config") + expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") + expect(provider.getProviderProfileEntries().map(({ name }) => name)).toContain("current-config") + }) }) }) From 3b2f07376f7daf2b000a425675df21a3014f20f8 Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 21:17:50 +0900 Subject: [PATCH 25/26] fix(core): compare the whole profile entry in the undo mirror gate What: `entriesEqual` now serializes both lists instead of comparing only name and id per index. Why: a save under an existing name reuses that profile's id, so a changed `apiProvider` or `modelId` compared equal on name+id. The undo mirror was then skipped and the webview kept the old provider/model for that profile until the next activation re-mirrored the list. Impact: the mirror runs whenever any entry field differs. Both sides are always `listConfig()`-shaped, so key order is stable and the comparison is exact. Net 0 counted executable lines. Tests: T14 re-saves the same name under the same id with a different provider during a timed-out purge and asserts the cache converges to the manager. --- src/core/webview/ClineProvider.ts | 10 +++- .../webview/__tests__/ClineProvider.spec.ts | 57 +++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 3a6c9081cf..a734543b81 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -213,9 +213,15 @@ type GetStateOptions = { /** The ContextProxy snapshot a failed delete rolls back to. */ type ProviderProfileSnapshot = Pick -/** Whether two profile lists name the same profiles with the same ids, so a mirror would be a no-op. */ +/** + * Whether two profile lists are the same list, so a mirror would be a no-op. + * + * Compares the whole entry, not just name+id: a save under an existing name reuses that profile's + * id, so a changed provider or model compares equal on those two fields and a mirror gated on them + * would leave the webview showing the old provider for that profile. + */ const entriesEqual = (left: ProviderSettingsEntry[], right: ProviderSettingsEntry[] = []): boolean => - left.length === right.length && left.every((entry, i) => entry.name === right[i]?.name && entry.id === right[i]?.id) + left.length === right.length && JSON.stringify(left) === JSON.stringify(right) export class ClineProvider extends EventEmitter diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 7573709ff9..b204aed27f 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -5217,6 +5217,63 @@ describe("ClineProvider", () => { expect(provider.contextProxy.getValues().currentApiConfigName).toBe("current-config") expect(provider.getProviderProfileEntries().map(({ name }) => name)).toContain("current-config") }) + + test("T14: the undo mirror catches a same-id provider change instead of stranding the cache", async () => { + // A queued save lands while the timed-out purge holds the lock and re-saves the same name + // under the *same* id (the manager preserves the id on update) with a different provider. + // The undo is skipped because the name exists, and the mirror gate used to compare only + // name+id: it saw "equal", wrote nothing, and left the webview showing the old provider + // for that profile until some later activation re-mirrored the list. + vi.useFakeTimers() + + try { + const { profiles, withLock, deleteConfigUnlocked } = mockProviderSettingsManager() + + 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) + + // Same name, same id, different provider: exactly what an update through `saveConfig` + // produces, since the manager reuses the existing id. + const recreated = provider.providerSettingsManager.saveConfig("other-config", { + id: otherProfile.id, + apiProvider: providerIdentifiers.anthropic, + apiKey: "new-user-key", + }) + + releasePurge() + for (let i = 0; i < 50; i++) { + await Promise.resolve() + await vi.advanceTimersByTimeAsync(0) + } + + await recreated + await deletionResult + + // The manager holds the new provider, so the cache must too; name+id equality is not + // enough to conclude the two views agree. + expect(profiles.find(({ name }) => name === "other-config")?.apiProvider).toBe( + providerIdentifiers.anthropic, + ) + expect(provider.getProviderProfileEntries().find(({ name }) => name === "other-config")).toMatchObject({ + id: otherProfile.id, + apiProvider: providerIdentifiers.anthropic, + }) + } finally { + vi.useRealTimers() + } + }) }) }) From afb629e636a7e304e53398449d35d7158e9ae26f Mon Sep 17 00:00:00 2001 From: myk1yt Date: Sat, 31 Oct 2026 21:21:51 +0900 Subject: [PATCH 26/26] fix(core): derive a profile's secret keys from its provider as well as its blob What: `getProfileSecretKeys` now returns the union of the keys the blob declares and the keys the profile's own `apiProvider` owns, both drawn from `PROVIDER_SECRET_KEYS`. Why: `api.getConfiguration()` strips secret fields, so a profile saved from that round trip declares no secret while its live credential is still in SecretStorage. Deleting that profile purged zero keys and left the credential behind indefinitely. Impact: a deleted profile's own credentials are removed even when its blob is secret-less. A provider with no ownership branch keeps the presence-based answer, and the surviving-profile keep-set is derived from the same candidate set, so it can only grow. Net 0 counted executable lines. Tests: three cases pin the union, the declared-key path, and the retired-provider fallback. --- src/core/config/ContextProxy.ts | 9 +++- .../config/__tests__/ContextProxy.spec.ts | 42 ++++++++++++++++++- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index e5e8cdb25e..b22aa49b81 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -93,18 +93,23 @@ export const providerOwnsSecretKey = (apiProvider: unknown, key: string): boolea } /** - * The secret keys a stored profile blob declares. + * The secret keys a stored profile uses. * * 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. * + * The blob alone is not enough: `api.getConfiguration()` strips secret fields, so a profile + * saved from that round trip declares none while its live credential sits in SecretStorage. + * The profile's own `apiProvider` still names the keys it owns, so the candidates are the + * union of both. A provider with no ownership branch keeps the presence-based answer. + * * 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)) + [...PROVIDER_SECRET_KEYS].filter((key) => key in config || providerOwnsSecretKey(config.apiProvider, key) === true) const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, diff --git a/src/core/config/__tests__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index 5d3cfaa9aa..e8c1fb7aa1 100644 --- a/src/core/config/__tests__/ContextProxy.spec.ts +++ b/src/core/config/__tests__/ContextProxy.spec.ts @@ -7,7 +7,7 @@ import { GLOBAL_STATE_KEYS, SECRET_STATE_KEYS, GLOBAL_SECRET_KEYS } from "@roo-c import { clearAllMocks } from "../../../test-utils/reset" import { makeExtensionContext, makeUri } from "../../../test-utils/vscode" -import { ContextProxy } from "../ContextProxy" +import { ContextProxy, getProfileSecretKeys } from "../ContextProxy" import { providerIdentifiers, retiredProviderIdentifiers } from "@roo-code/types/provider-identifiers" vi.mock("vscode", () => ({ @@ -576,6 +576,46 @@ describe("ContextProxy", () => { }) }) + describe("getProfileSecretKeys", () => { + it("includes the keys the profile's own provider owns even when the blob omits them", async () => { + // A profile written from an `api.getConfiguration()` snapshot has its secret fields + // stripped while keeping `apiProvider`. Deriving the purge candidates from the blob alone + // therefore declares no secret, so deleting that profile would leave its live + // SecretStorage credential behind. Ownership answers what the omission cannot. + expect( + getProfileSecretKeys({ + name: "stripped-profile", + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "openai/gpt-4.1", + }), + ).toEqual(["openRouterApiKey"]) + }) + + it("keeps a declared key and drops nothing the provider does not own", async () => { + // The union must not turn into "every key the provider owns": a key the blob declares + // stays, and a non-secret setting is never a candidate. + expect( + getProfileSecretKeys({ + apiProvider: providerIdentifiers.anthropic, + apiKey: "declared-key", + apiModelId: "claude-sonnet-4-5", + }), + ).toEqual(["apiKey"]) + }) + + it("stays blob-only for a profile whose provider has no ownership branch", async () => { + // A retired provider has no branch in the discriminated union, so ownership is unknown. + // Guessing would either drop a declared key or invent one; the presence-based answer is + // the only safe one. + expect( + getProfileSecretKeys({ + apiProvider: retiredProviderIdentifiers.groq, + apiKey: "retired-key", + }), + ).toEqual(["apiKey"]) + }) + }) + describe("resetAllState", () => { it("should clear all in-memory caches", async () => { // Setup initial state in caches