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 1250a7baca..b22aa49b81 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -13,7 +13,9 @@ import { type GlobalState, type RooCodeSettings, providerSettingsSchema, + providerSettingsSchemaDiscriminated, globalSettingsSchema, + codebaseIndexProviderSchema, isSecretStateKey, isProviderName, isRetiredProvider, @@ -32,6 +34,83 @@ 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)) + +// 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". +// 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. + */ +export 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 +} + +/** + * 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[] => + [...PROVIDER_SECRET_KEYS].filter((key) => key in config || providerOwnsSecretKey(config.apiProvider, key) === true) + const globalSettingsExportSchema = globalSettingsSchema.omit({ taskHistory: true, listApiConfigMeta: true, @@ -507,7 +586,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 @@ -523,14 +613,61 @@ 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`. + // + // 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 owner = expectedProvider ?? values.apiProvider + + const isStale = (key: string) => { + if (!isSecretStateKey(key)) { + return !!this.stateCache[key as GlobalStateKey] + } + + if (!this.secretCache[key as SecretStateKey]) { + return false + } + + return providerOwnsSecretKey(owner, key) !== true + } + 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, }) } + /** + * 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/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 01695d7994..65a7220a95 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 { @@ -503,9 +531,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 +549,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}`) @@ -542,11 +591,25 @@ export class ProviderSettingsManager { /** * 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 = {} @@ -554,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}`) @@ -574,6 +638,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__/ContextProxy.spec.ts b/src/core/config/__tests__/ContextProxy.spec.ts index b4d3b1b8aa..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", () => ({ @@ -405,6 +405,215 @@ 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 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. + 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") + }) + + 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") + }) + + 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("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", () => { diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index a1fc9ac7de..1212788443 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", () => { @@ -1387,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 869a96f65d..a734543b81 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, @@ -111,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, providerOwnsSecretKey } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" import { PendingActionSettlementError, Task } from "../task/Task" @@ -209,6 +210,19 @@ type GetStateOptions = { includeTaskHistory?: boolean } +/** The ContextProxy snapshot a failed delete rolls back to. */ +type ProviderProfileSnapshot = Pick + +/** + * 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 && JSON.stringify(left) === JSON.stringify(right) + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -272,28 +286,51 @@ 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. + * 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 } = {}, + 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), ) 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 - } + // 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() - controller.abort() - this.log("Provider profile mutation timed out; aborting in-flight mutation") + this.log( + abortsBody + ? "Provider profile mutation timed out; aborting in-flight mutation" + : "Provider profile mutation timed out; waiting for it to finish before the next mutation", + ) }) void run.then( @@ -313,13 +350,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 { @@ -338,6 +378,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 @@ -2070,6 +2143,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. @@ -2082,10 +2166,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", @@ -2112,26 +2196,550 @@ export class ClineProvider } async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) { - const globalSettings = this.contextProxy.getValues() - let profileToActivate: string | undefined = globalSettings.currentApiConfigName + // 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: ProviderProfileSnapshot = {} + // 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 + // 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, 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 { + const enqueued = this.enqueueProviderProfileMutationWithBody(async (signal) => { + // Read inside the queue: a queued activation may have changed the active profile. + const globalSettings = this.contextProxy.getValues() + const deletedActiveProfile = profileToDelete.name === globalSettings.currentApiConfigName + + previousContextValues = { + currentApiConfigName: globalSettings.currentApiConfigName, + listApiConfigMeta: globalSettings.listApiConfigMeta, + 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. + 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 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 undoPurge() + 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 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 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( + storedProfile, + await this.getSurvivingProviderSecretKeys(profileToDelete.name, storedProfile), + ) + + // 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 + } + + await this.contextProxy.setValues({ listApiConfigMeta, modeApiConfigs }) + + await this.postStateToWebview() + return + } - if (profileToDelete.name === profileToActivate) { - profileToActivate = this.getProviderProfileEntries().find(({ name }) => name !== profileToDelete.name)?.name + // 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 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) { + throw new Error("You cannot delete the last profile") + } + + // Capture the profile before purging so a failed active-profile switch can restore it. + 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. + if (signal.aborted) { + return + } + + 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 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 + // 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 keep-set is ownership-derived for the same reason as the inactive branch: a + // 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), + ) + // 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() + }) + 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, 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, + previousContextValues, + removedModeConfigs, + ) + throw error + } + } + + /** + * 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 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. + * + * 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. + * + * 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( + deletedName: string, + deletedConfig: (ProviderSettingsWithId & { name: string }) | undefined, + ): Promise> { + const deletedKeys = deletedConfig ? getProfileSecretKeys(deletedConfig) : [] + const keep = new Set() + + try { + for (const { name } of await this.providerSettingsManager.listConfig()) { + if (name === deletedName) 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() } - if (!profileToActivate) { - throw new Error("You cannot delete the last profile") + return keep + } + + /** + * 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 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. + * + * 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, + keep: Set = new Set(), + ): Promise { + if (!config) { + return } - const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name) + const secretKeys = getProfileSecretKeys(config).filter((key) => !keep.has(key)) - await this.contextProxy.setValues({ - ...globalSettings, - currentApiConfigName: profileToActivate, - listApiConfigMeta: entries, - }) + if (secretKeys.length === 0) { + return + } - await this.postStateToWebview() + try { + 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) { + 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) + } + } + + /** + * 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, + previousContextValues: ProviderProfileSnapshot, + 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 + // 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.enqueueProviderProfileMutation( + async (signal) => { + let restored = false + + try { + restored = await this.applyProviderProfileRestore( + name, + config, + previousContextValues, + removedModeConfigs, + 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`) + } 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(name, removedModeConfigs) + } + + // The replacement activation posted state before this rollback landed, and the + // 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 }, + ) + } catch (error) { + this.log( + `Provider profile restore for '${name}' did not complete: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + + /** + * 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. + * + * 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 { + if (!config) { + return false + } + + try { + const { name: _name, ...providerSettings } = config + + // 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. + 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 + } + + // 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, { onlyIfUnbound: true }) + } + + 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, + config: (ProviderSettingsWithId & { name: string }) | undefined, + previousContextValues: ProviderProfileSnapshot, + removedModeConfigs: Record, + signal: AbortSignal, + ): Promise { + 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 + } + + // 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 + if ((await this.providerSettingsManager.saveConfigIfAbsent(name, providerSettings)) === undefined) return false + + if (signal.aborted) return false + + 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. 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 + + // `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 + } + + /** + * 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. + * + * 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. + * + * The fallback activation is the only write here that can *replace* a profile, so it is gated on + * 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. + */ + private async reconcileActiveProfileAfterFailedRestore( + deletedName: string, + removedModeConfigs: Record = {}, + ): Promise { + try { + const selectedName = this.contextProxy.getValues().currentApiConfigName + const keepSelection = + !!selectedName && + (selectedName !== deletedName || (await this.providerSettingsManager.hasConfig(deletedName))) && + (await this.providerSettingsManager.hasConfig(selectedName)) + + if (!keepSelection) { + 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.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. 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)) { + await this.providerSettingsManager.setModeConfig(mode as Mode, configId, { onlyIfUnbound: true }) + } + await this.contextProxy.setValue("modeApiConfigs", await this.providerSettingsManager.getModeApiConfigs()) + } catch (reconcileError) { + this.log( + `Failed to reconcile the active profile after a failed restore: ${ + reconcileError instanceof Error ? reconcileError.message : String(reconcileError) + }`, + ) + } } private async persistStickyProviderProfileToCurrentTask( @@ -2195,20 +2803,31 @@ 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), ]) } + // 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 }) @@ -2218,6 +2837,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..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", () => { @@ -745,6 +775,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 779313d385..b204aed27f 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -435,6 +435,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(), } @@ -3578,6 +3580,1701 @@ 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: { + // 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 + 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) + + // 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" }) + + // 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" }) + }) + + 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", () => { + 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] + + // Full configs keyed by name, so `getProfile`/`saveConfig` behave like the real manager. + // 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" }], + ]) + + // 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 } + + // 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 = {} + + // 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((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 { ...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))), + ), + 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 } + }), + ), + // 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 + .fn() + .mockImplementation((mode: string) => withLock(async () => modeApiConfigs[mode])), + getModeApiConfigs: vi.fn().mockImplementation(() => withLock(async () => ({ ...modeApiConfigs }))), + }, + }) + + return { profiles, managerState, modeApiConfigs, withLock, deleteConfigUnlocked } + } + + 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") + // Deleting a non-active profile must not trigger activation side effects. + expect(provider.providerSettingsManager.activateProfile).not.toHaveBeenCalled() + 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("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() + + 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 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"]) + }) + + 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() + + 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("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) + + // 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 + // replacement's effective settings, not the deleted profile's. + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( + expect.objectContaining({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: "other-model", + }), + ) + // 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 () => { + 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]), + }, + }) + + const setValuesSpy = vi.spyOn(provider.contextProxy, "setValues") + + await expect(provider.deleteProviderProfile(otherProfile)).rejects.toThrow( + "Config 'other-config' not found", + ) + + expect(provider.providerSettingsManager.deleteConfig).toHaveBeenCalledWith("other-config") + // 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 () => { + 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"), + ) + + 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, + // 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", + 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() + }) + + test("restores the deleted profile when the mutation timeout aborts the activation", async () => { + vi.useFakeTimers() + + try { + 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 + }) + // 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" } + }) + + 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 + // naming a profile the manager no longer has. The credential must survive too. + expect(provider.providerSettingsManager.saveConfigIfAbsent).toHaveBeenCalledWith( + "current-config", + 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 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") + + // 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.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.saveConfigIfAbsent).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() + } + }) + + 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, 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 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")) + .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", + ) + + // 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", + }) + // 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 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) + }) + + test("keeps a queued activation's write when the rollback settles late", async () => { + vi.useFakeTimers() + + try { + const { managerState } = mockProviderSettingsManager() + let resolveRestore!: () => void + const pendingRestore = new Promise((resolve) => { + resolveRestore = resolve + }) + + // 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.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.saveConfigIfAbsent).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'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", + }) + } finally { + vi.useRealTimers() + } + }) + + 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("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("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 `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.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.saveConfigIfAbsent).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 + 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("restore abort at the mode re-apply still reconciles the mode map in both stores", async () => { + vi.useFakeTimers() + + try { + 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 + return true + }) + + 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, 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.anthropic, + apiKey: "current-key", + }) + expect(modeApiConfigs).toEqual({ code: currentProfile.id, architect: currentProfile.id }) + expect(provider.contextProxy.getValues().modeApiConfigs).toEqual(modeApiConfigs) + } finally { + vi.useRealTimers() + } + }) + + 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. 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[] = [] + const harnessDeleteConfig = vi + .mocked(provider.providerSettingsManager.deleteConfig) + .getMockImplementation()! + const recordingDeleteConfig = async (name: string) => { + deleteConfigCalls.push(name) + 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 + // purge commit after the failure was reported. + let releasePurge!: () => void + const purgeGate = new Promise((resolve) => { + releasePurge = resolve + }) + vi.mocked(provider.providerSettingsManager.deleteConfig).mockImplementationOnce(async (name) => { + await purgeGate + return recordingDeleteConfig(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") + + // 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 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 }) + // 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.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.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. + 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 `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.saveConfigIfAbsent).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.listConfig()).toEqual([otherProfile]) + 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'"), + ) + }) + + // --- 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 `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.saveConfigIfAbsent).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() + } + }) + + 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") + }) + + 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() + } + }) + }) }) describe("webviewMessageHandler no-floating-promises coverage", () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index bd7c589958..851d3f187b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -2512,24 +2512,45 @@ 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)}`, ) vscode.window.showErrorMessage(t("common:errors.delete_api_config")) + } finally { + // `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", + await provider.providerSettingsManager.getModeApiConfigs(), + ) + } catch (mirrorError) { + provider.log( + `Error mirroring mode configs after delete api configuration: ${ + mirrorError instanceof Error ? mirrorError.message : String(mirrorError) + }`, + ) + } } } break