From f3abaf850e2e55e23461a461182104f0395e2831 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 9 Jul 2026 10:41:12 +0200 Subject: [PATCH] fix(providers): keep removal live after config updates --- .../screens/settings/providers-section.tsx | 51 +++++--- .../src/server/agent/agent-manager.test.ts | 41 +++++++ .../server/src/server/agent/agent-manager.ts | 3 + .../agent/provider-snapshot-manager.test.ts | 23 ++++ .../server/agent/provider-snapshot-manager.ts | 27 +++- .../src/server/daemon-config-store.test.ts | 115 ++++++++++++++++++ .../server/src/server/daemon-config-store.ts | 50 +++++++- .../server/src/server/websocket-server.ts | 3 +- 8 files changed, 288 insertions(+), 25 deletions(-) diff --git a/packages/app/src/screens/settings/providers-section.tsx b/packages/app/src/screens/settings/providers-section.tsx index 49bdcc1eda..a21ac56a81 100644 --- a/packages/app/src/screens/settings/providers-section.tsx +++ b/packages/app/src/screens/settings/providers-section.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useMemo, useRef, useState } from "react"; import type { TFunction } from "i18next"; import { useTranslation } from "react-i18next"; import { @@ -95,6 +95,10 @@ interface ProviderActionsMenuProps { providerId: string; providerLabel: string; isRemoving: boolean; + iconSize: number; + foregroundColor: string; + foregroundMutedColor: string; + dangerColor: string; onRemove: (providerId: string, providerLabel: string) => void; } @@ -102,10 +106,13 @@ function ProviderActionsMenu({ providerId, providerLabel, isRemoving, + iconSize, + foregroundColor, + foregroundMutedColor, + dangerColor, onRemove, }: ProviderActionsMenuProps) { const { t } = useTranslation(); - const { theme } = useUnistyles(); const handleRemove = useCallback(() => { onRemove(providerId, providerLabel); }, [onRemove, providerId, providerLabel]); @@ -121,14 +128,12 @@ function ProviderActionsMenu({ ], [], ); - const trashLeading = useMemo( - () => , - [theme.colors.statusDanger], - ); + const trashLeading = useMemo(() => , [dangerColor]); return ( {({ hovered, open }) => ( )} @@ -246,6 +251,10 @@ function ProviderRow({ providerId={def.id} providerLabel={def.label} isRemoving={isRemoving} + iconSize={theme.iconSize.sm} + foregroundColor={theme.colors.foreground} + foregroundMutedColor={theme.colors.foregroundMuted} + dangerColor={theme.colors.statusDanger} onRemove={onRemove} /> ) : null} @@ -312,6 +321,7 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) { const openProviderSettings = useProviderSettingsStore((state) => state.open); const [pendingProviderId, setPendingProviderId] = useState(null); const [removingProviderId, setRemovingProviderId] = useState(null); + const removingProviderIdRef = useRef(null); const [installingProviderId, setInstallingProviderId] = useState(null); const providerDefinitions = useMemo(() => buildProviderDefinitions(entries), [entries]); @@ -343,18 +353,20 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) { const handleRemoveProvider = useCallback( async (providerId: string, providerLabel: string) => { - const confirmed = await confirmDialog({ - title: t("settings.providers.remove.confirmTitle", { name: providerLabel }), - message: t("settings.providers.remove.confirmMessage"), - confirmLabel: t("settings.providers.remove.confirm"), - destructive: true, - }); - if (!confirmed) { - return; - } - + if (removingProviderIdRef.current) return; + removingProviderIdRef.current = providerId; setRemovingProviderId(providerId); try { + const confirmed = await confirmDialog({ + title: t("settings.providers.remove.confirmTitle", { name: providerLabel }), + message: t("settings.providers.remove.confirmMessage"), + confirmLabel: t("settings.providers.remove.confirm"), + destructive: true, + }); + if (!confirmed) { + return; + } + await patchConfig({ removeProviders: [providerId] }); } catch (error) { Alert.alert( @@ -362,6 +374,9 @@ export function ProvidersSection({ serverId }: ProvidersSectionProps) { error instanceof Error ? error.message : String(error), ); } finally { + if (removingProviderIdRef.current === providerId) { + removingProviderIdRef.current = null; + } setRemovingProviderId((current) => (current === providerId ? null : current)); } }, diff --git a/packages/server/src/server/agent/agent-manager.test.ts b/packages/server/src/server/agent/agent-manager.test.ts index 759f9b2f41..98814047e8 100644 --- a/packages/server/src/server/agent/agent-manager.test.ts +++ b/packages/server/src/server/agent/agent-manager.test.ts @@ -2272,6 +2272,47 @@ test("updateProviderRegistry registers a previously unknown provider", async () expect(snapshot.config.provider).toBe("codex"); }); +test("updateProviderRegistry removes providers omitted from the next registry", async () => { + const workdir = mkdtempSync(join(tmpdir(), "agent-manager-test-")); + const storagePath = join(workdir, "agents"); + const storage = new AgentStorage(storagePath, logger); + const removedProvider = "zai-claude" as AgentProvider; + class RemovedProviderClient extends TestAgentClient { + createSessionCalls = 0; + + override async createSession(config: AgentSessionConfig): Promise { + this.createSessionCalls += 1; + return await super.createSession(config); + } + } + + const removedClient = new RemovedProviderClient(); + const manager = new AgentManager({ + clients: { codex: new TestAgentClient(), [removedProvider]: removedClient }, + providerDefinitions: { + codex: { enabled: true }, + [removedProvider]: { enabled: true }, + }, + registry: storage, + logger, + }); + + expect(manager.getRegisteredProviderIds()).toContain(removedProvider); + + manager.updateProviderRegistry({ + providerDefinitions: { codex: { enabled: true } }, + clients: { codex: new TestAgentClient() }, + }); + + expect(manager.getRegisteredProviderIds()).not.toContain(removedProvider); + await expect( + manager.createAgent({ provider: removedProvider, cwd: workdir }, undefined, { + workspaceId: undefined, + }), + ).rejects.toThrow("Unknown provider 'zai-claude'"); + expect(removedClient.createSessionCalls).toBe(0); +}); + test("createAgent passes explicit model strings through to the provider", async () => { const workdir = mkdtempSync(join(tmpdir(), "agent-manager-test-")); const storagePath = join(workdir, "agents"); diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index fa34cbaa16..20d1da8309 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -622,11 +622,14 @@ export class AgentManager { providerDefinitions: ProviderEnabledMap; clients: ProviderClientMap; }): void { + this.providerEnabled.clear(); for (const [provider, definition] of Object.entries(input.providerDefinitions)) { if (definition) { this.providerEnabled.set(provider, definition.enabled); } } + + this.clients.clear(); for (const [provider, client] of Object.entries(input.clients)) { if (client) { this.clients.set(provider, client); diff --git a/packages/server/src/server/agent/provider-snapshot-manager.test.ts b/packages/server/src/server/agent/provider-snapshot-manager.test.ts index 823c3a3431..4b6a3eac56 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.test.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.test.ts @@ -960,6 +960,29 @@ describe("ProviderSnapshotManager applyMutableProviderConfig", () => { } }); + test("removes startup provider overrides from the live registry", () => { + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + providerOverrides: { + "zai-claude": { extends: "claude", label: "ZAI", enabled: true }, + }, + }); + try { + expect(manager.hasProvider("zai-claude")).toBe(true); + + const state = manager.applyMutableProviderConfig({}, { removeProviders: ["zai-claude"] }); + + expect(manager.hasProvider("zai-claude")).toBe(false); + expect(state.providerDefinitions["zai-claude"]).toBeUndefined(); + expect(manager.getSnapshot().some((entry) => entry.provider === "zai-claude")).toBe(false); + + manager.applyMutableProviderConfig({ codex: { enabled: false } }); + expect(manager.hasProvider("zai-claude")).toBe(false); + } finally { + manager.destroy(); + } + }); + test("drops disabled built-in providers from clients while preserving providerDefinitions", () => { const manager = new ProviderSnapshotManager({ logger: createTestLogger(), diff --git a/packages/server/src/server/agent/provider-snapshot-manager.ts b/packages/server/src/server/agent/provider-snapshot-manager.ts index b91b730ec1..f21d919237 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.ts @@ -65,6 +65,22 @@ function resolveDiagnosticTimeoutMs(option: number | undefined, refreshTimeoutMs return Math.max(refreshTimeoutMs, DEFAULT_DIAGNOSTIC_TIMEOUT_MS); } +function omitProviderOverrides( + overrides: Record | undefined, + providers: readonly string[], +): Record | undefined { + if (!overrides || providers.length === 0) { + return overrides; + } + + const nextOverrides = { ...overrides }; + for (const provider of providers) { + delete nextOverrides[provider]; + } + + return Object.keys(nextOverrides).length > 0 ? nextOverrides : undefined; +} + type ProviderSnapshotChangeListener = (entries: ProviderSnapshotEntry[], cwd: string) => void; export interface ProviderSnapshotManagerOptions { @@ -95,6 +111,10 @@ interface ProviderSnapshotReadOptions { wait?: boolean; } +interface ApplyMutableProviderConfigOptions { + removeProviders?: readonly string[]; +} + interface ProviderSnapshotProviderOptions { cwd?: string | null; provider: AgentProvider; @@ -164,7 +184,7 @@ export class ProviderSnapshotManager { private readonly extraClients: Partial>; private runtimeSettings: AgentProviderRuntimeSettingsMap | undefined; private providerOverrides: Record | undefined; - private readonly baseProviderOverrides: Record | undefined; + private baseProviderOverrides: Record | undefined; private providerRegistry: Record; private providerClients: Record; @@ -370,7 +390,12 @@ export class ProviderSnapshotManager { applyMutableProviderConfig( mutableProviders: MutableDaemonConfig["providers"] | undefined, + options: ApplyMutableProviderConfigOptions = {}, ): AgentManagerProviderState { + this.baseProviderOverrides = omitProviderOverrides( + this.baseProviderOverrides, + options.removeProviders ?? [], + ); this.providerOverrides = applyMutableProviderConfigToOverrides( this.baseProviderOverrides, mutableProviders, diff --git a/packages/server/src/server/daemon-config-store.test.ts b/packages/server/src/server/daemon-config-store.test.ts index 5491ca5734..4b4df23a52 100644 --- a/packages/server/src/server/daemon-config-store.test.ts +++ b/packages/server/src/server/daemon-config-store.test.ts @@ -218,6 +218,121 @@ describe("DaemonConfigStore", () => { expect(persisted.agents?.providers).toBeUndefined(); }); + test("patch removes deleted providers from metadata generation", () => { + const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-")); + tempDirs.push(paseoHome); + + const configPath = path.join(paseoHome, "config.json"); + writeFileSync( + configPath, + `${JSON.stringify( + { + version: 1, + agents: { + providers: { + gemini: { + extends: "acp", + label: "Gemini", + command: ["gemini", "--acp"], + }, + claude: { + enabled: false, + }, + }, + metadataGeneration: { + providers: [ + { provider: "gemini", model: "flash" }, + { provider: "claude", model: "haiku" }, + ], + }, + }, + }, + null, + 2, + )}\n`, + ); + + const store = new DaemonConfigStore( + paseoHome, + { + mcp: { injectIntoAgents: false }, + browserTools: { enabled: false }, + providers: { + gemini: {}, + claude: { enabled: false }, + }, + metadataGeneration: { + providers: [ + { provider: "gemini", model: "flash" }, + { provider: "claude", model: "haiku" }, + ], + }, + autoArchiveAfterMerge: false, + enableTerminalAgentHooks: false, + appendSystemPrompt: "", + }, + undefined, + ); + + const next = store.patch({ removeProviders: ["gemini"] }); + + expect(next.metadataGeneration.providers).toEqual([{ provider: "claude", model: "haiku" }]); + const persisted = loadPersistedConfig(paseoHome); + expect(persisted.agents?.metadataGeneration).toEqual({ + providers: [{ provider: "claude", model: "haiku" }], + }); + }); + + test("patch persists provider removal when in-memory config is already clean", () => { + const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-")); + tempDirs.push(paseoHome); + + const configPath = path.join(paseoHome, "config.json"); + writeFileSync( + configPath, + `${JSON.stringify( + { + version: 1, + agents: { + providers: { + gemini: { + extends: "acp", + label: "Gemini", + command: ["gemini", "--acp"], + }, + }, + metadataGeneration: { + providers: [{ provider: "gemini", model: "flash" }], + }, + }, + }, + null, + 2, + )}\n`, + ); + + const store = new DaemonConfigStore( + paseoHome, + { + mcp: { injectIntoAgents: false }, + browserTools: { enabled: false }, + providers: {}, + metadataGeneration: { providers: [] }, + autoArchiveAfterMerge: false, + enableTerminalAgentHooks: false, + appendSystemPrompt: "", + }, + undefined, + ); + + const next = store.patch({ removeProviders: ["gemini"] }); + + expect(next.providers.gemini).toBeUndefined(); + const persisted = loadPersistedConfig(paseoHome); + expect(persisted.agents?.providers).toBeUndefined(); + expect(persisted.agents?.metadataGeneration).toEqual({ providers: [] }); + }); + test("patch persists append system prompt into config.json", () => { const paseoHome = mkdtempSync(path.join(tmpdir(), "paseo-daemon-config-store-")); tempDirs.push(paseoHome); diff --git a/packages/server/src/server/daemon-config-store.ts b/packages/server/src/server/daemon-config-store.ts index 021e151a35..0805355322 100644 --- a/packages/server/src/server/daemon-config-store.ts +++ b/packages/server/src/server/daemon-config-store.ts @@ -20,7 +20,11 @@ interface LoggerLike { info(...args: unknown[]): void; } -type ConfigListener = (config: MutableDaemonConfig) => void; +export interface DaemonConfigChangeDetails { + removedProviders: readonly string[]; +} + +type ConfigListener = (config: MutableDaemonConfig, details: DaemonConfigChangeDetails) => void; type FieldChangeHandler = (value: unknown) => void; function getLogger(logger: LoggerLike | undefined): LoggerLike | undefined { @@ -72,6 +76,30 @@ function omitProvidersFromConfig return changed ? ({ ...config, providers: nextProviders } as T) : config; } +function omitMetadataGenerationProvidersFromConfig< + T extends { metadataGeneration?: { providers?: Array<{ provider?: unknown }> } }, +>(config: T, providers: readonly string[]): T { + if (providers.length === 0 || !config.metadataGeneration?.providers) { + return config; + } + + const removedProviderIds = new Set(providers); + const nextProviders = config.metadataGeneration.providers.filter((entry) => { + return typeof entry.provider !== "string" || !removedProviderIds.has(entry.provider); + }); + if (nextProviders.length === config.metadataGeneration.providers.length) { + return config; + } + + return { + ...config, + metadataGeneration: { + ...config.metadataGeneration, + providers: nextProviders, + }, + } as T; +} + function omitProvidersFromOverrides( overrides: Record | undefined, providers: readonly string[], @@ -148,20 +176,31 @@ export class DaemonConfigStore { public patch(partial: MutableDaemonConfigPatch): MutableDaemonConfig { const parsedPatch = MutableDaemonConfigPatchSchema.parse(partial); const { removeProviders = [], ...configPatch } = parsedPatch; + const removedProviders = Array.from(new Set(removeProviders)); const merged = deepMerge(this.current, configPatch); - const next = MutableDaemonConfigSchema.parse(omitProvidersFromConfig(merged, removeProviders)); + const next = MutableDaemonConfigSchema.parse( + omitMetadataGenerationProvidersFromConfig( + omitProvidersFromConfig(merged, removedProviders), + removedProviders, + ), + ); const changedFieldPaths = Array.from(this.fieldChangeHandlers.keys()).filter((path) => { return !isEqualValue(getValueAtPath(this.current, path), getValueAtPath(next, path)); }); + const configChanged = !isEqualValue(this.current, next); - if (changedFieldPaths.length === 0 && isEqualValue(this.current, next)) { + if (!configChanged && removedProviders.length === 0) { return this.current; } // Persist before updating in-memory state so that if persistence fails, // runtime and disk stay consistent. - this.persistConfig(next, removeProviders); + this.persistConfig(next, removedProviders); + if (!configChanged) { + return this.current; + } + this.current = next; for (const path of changedFieldPaths) { @@ -175,8 +214,9 @@ export class DaemonConfigStore { } } + const changeDetails: DaemonConfigChangeDetails = { removedProviders }; for (const listener of this.changeListeners) { - listener(next); + listener(next, changeDetails); } return next; diff --git a/packages/server/src/server/websocket-server.ts b/packages/server/src/server/websocket-server.ts index 97951f4591..74a890628f 100644 --- a/packages/server/src/server/websocket-server.ts +++ b/packages/server/src/server/websocket-server.ts @@ -554,9 +554,10 @@ export class VoiceAssistantWebSocketServer { this.speech?.onReadinessChange((snapshot) => { this.publishSpeechReadiness(snapshot); }) ?? null; - this.unsubscribeDaemonConfigChange = this.daemonConfigStore.onChange((config) => { + this.unsubscribeDaemonConfigChange = this.daemonConfigStore.onChange((config, details) => { const nextAgentManagerState = this.providerSnapshotManager.applyMutableProviderConfig( config.providers, + { removeProviders: details.removedProviders }, ); this.agentManager.updateProviderRegistry(nextAgentManagerState); this.broadcastDaemonConfigChanged(config);