fix(core): purge deleted provider profiles from ProviderSettingsManager

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<string, string[]>()
+
+		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<string>()
+
+		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", () => {

src/core/config/ContextProxy.ts

@@ -13,7 +13,9 @@ import {
 	type GlobalState,
 	type RooCodeSettings,
 	providerSettingsSchema,
+	providerSettingsSchemaDiscriminated,
 	globalSettingsSchema,
+	codebaseIndexProviderSchema,
 	isSecretStateKey,
 	isProviderName,
 	isRetiredProvider,
@@ -32,6 +34,78 @@ 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<string>(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<string>(
+	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<string, Set<string>>()
+
+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<string>()
+		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 blob declares.
+ *
+ * A profile blob is the record of which credentials that profile uses: `saveConfig`'s
+ * discriminated parse keeps only the branch's own keys, so the keys present in the blob
+ * are exactly the ones the profile owns. Used to purge a deleted profile's credentials
+ * explicitly instead of inferring them from a later write.
+ *
+ * Membership is `PROVIDER_SETTINGS_KEYS ∩ SECRET_STATE_KEYS`, the same set the clear-out
+ * loop iterates: it excludes the global image-generation key, which no profile blob owns.
+ */
+export const getProfileSecretKeys = (config: Record<string, unknown>): string[] =>
+	Object.keys(config).filter((key) => PROVIDER_SECRET_KEYS.has(key))
+
 const globalSettingsExportSchema = globalSettingsSchema.omit({
 	taskHistory: true,
 	listApiConfigMeta: true,
@@ -507,7 +581,18 @@ export class ContextProxy {
 		return sanitizedValues
 	}
 
-	public async setProviderSettings(values: ProviderSettings) {
+	/**
+	 * Write a provider slice, clearing out the settings it no longer carries.
+	 *
+	 * `expectedProvider` is the provider the caller resolved from the manager for the profile
+	 * being written. It is the ownership discriminator, and it must not be read off `values`:
+	 * `values.apiProvider` is whatever the caller put in the slice, so a crafted partial write
+	 * could name the owner of a secret it wants preserved and keep a credential that belongs to
+	 * a different profile. Callers that activate or persist a stored profile pass the provider
+	 * from the manager blob; callers with no manager record (import, tests) omit it and fall
+	 * back to the slice's own field.
+	 */
+	public async setProviderSettings(values: ProviderSettings, expectedProvider?: string) {
 		// Explicitly clear out any old API configuration values before that
 		// might not be present in the new configuration.
 		// If a value is not present in the new configuration, then it is assumed
@@ -523,14 +608,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<string>): Promise<void> {
+		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
 	 */

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<string | undefined> {
+		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<ProviderSettingsWithId & { name: string }> {
@@ -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<string, string> }> {
 		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<string, string> = {}
+				// 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}`)
@@ -540,6 +589,21 @@ export class ProviderSettingsManager {
 		}
 	}
 
+	/**
+	 * Check if a config exists by its manager-assigned id.
+	 *
+	 * The mode bindings a purge dropped name a profile by `id`, and a rollback that failed to
+	 * restore the profile must not re-apply them: a binding whose id no profile carries makes the
+	 * next switch to that mode resolve nothing and silently keep the current profile. Checking
+	 * under the lock keeps the answer from straddling a concurrent purge or rename.
+	 */
+	public async hasConfigById(id: string) {
+		return await this.lock(async () => {
+			const providerProfiles = await this.load()
+			return Object.values(providerProfiles.apiConfigs).some((config) => config.id === id)
+		})
+	}
+
 	/**
 	 * Set the API config for a specific mode.
 	 */
@@ -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<Record<string, string>> {
+		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 () => {