From c6106762036e69c0b91005e30673fe5a44a52f30 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Fri, 4 Sep 2026 23:27:44 +0200 Subject: [PATCH] fix(providers): preserve state across configuration reloads (#4332) A provider configuration edit invalidated every cached catalog and could turn healthy providers into false availability errors under the resulting probe burst. Preserve unchanged registry entries, clients, loads, and snapshots, and keep the submitted draft snapshot stable while creation is pending. --- .../src/composer/draft/create-flow.test.ts | 104 ++++++++++++ .../app/src/composer/draft/create-flow.ts | 75 +++++---- .../src/server/agent/plugin-provider.ts | 1 + .../src/server/agent/provider-registry.ts | 4 + .../agent/provider-snapshot-manager.test.ts | 157 +++++++++++++++++- .../server/agent/provider-snapshot-manager.ts | 65 ++++++-- 6 files changed, 359 insertions(+), 47 deletions(-) diff --git a/packages/app/src/composer/draft/create-flow.test.ts b/packages/app/src/composer/draft/create-flow.test.ts index 471d1fc8b9..8587fa3db6 100644 --- a/packages/app/src/composer/draft/create-flow.test.ts +++ b/packages/app/src/composer/draft/create-flow.test.ts @@ -88,6 +88,110 @@ describe("useDraftAgentCreateFlow", () => { expect(onCreateSuccess).toHaveBeenCalledTimes(1); }); + it("keeps the submitted preview when the pending provider selection disappears", () => { + const attempt: DraftCreateAttempt = { + clientMessageId: "msg-provider-handoff", + text: "build this", + timestamp: new Date("2026-05-25T00:00:00.000Z"), + }; + const { result, rerender } = renderHook( + ({ provider }: { provider: string | null }) => + useDraftAgentCreateFlow({ + draftId: "draft-handoff", + getPendingServerId: () => "server-1", + initialAttempt: attempt, + buildDraftAgent: () => { + if (!provider) throw new Error("Select a model"); + return { provider, model: "selected-model" }; + }, + createRequest: async () => ({ agentId: "agent-1", result: { id: "agent-1" } }), + onCreateSuccess: () => undefined, + }), + { initialProps: { provider: "codex" } as { provider: string | null } }, + ); + expect(result.current.draftAgent).toEqual({ provider: "codex", model: "selected-model" }); + rerender({ provider: null }); + expect(result.current.isSubmitting).toBe(true); + expect(result.current.draftAgent).toEqual({ provider: "codex", model: "selected-model" }); + }); + + it("shows an invalid prepared selection as a form error instead of throwing in render", () => { + const createRequest = vi.fn(async () => ({ agentId: "agent-1", result: { id: "agent-1" } })); + const { result } = renderHook(() => + useDraftAgentCreateFlow({ + draftId: "draft-invalid", + getPendingServerId: () => "server-1", + initialAttempt: { + clientMessageId: "msg-invalid", + text: "build this", + timestamp: new Date(0), + }, + buildDraftAgent: () => { + throw new Error("Select a model"); + }, + createRequest, + onCreateSuccess: () => undefined, + }), + ); + expect(result.current.isSubmitting).toBe(false); + expect(result.current.draftAgent).toBeNull(); + expect(result.current.formErrorMessage).toBe("Select a model"); + expect(createRequest).not.toHaveBeenCalled(); + }); + + it("keeps a manual submission stable and lets a failed request retry with a new selection", async () => { + let failRequest!: (error: Error) => void; + const request = new Promise((_resolve, reject) => { + failRequest = reject; + }); + let requestCount = 0; + const { result, rerender } = renderHook( + ({ provider }: { provider: string | null }) => + useDraftAgentCreateFlow({ + draftId: "draft-manual", + getPendingServerId: () => "server-1", + buildDraftAgent: () => { + if (!provider) throw new Error("Select a model"); + return { provider }; + }, + createRequest: async () => { + requestCount++; + if (requestCount === 1) return await request; + return { agentId: "agent-1", result: { id: "agent-1" } }; + }, + onCreateSuccess: () => undefined, + }), + { initialProps: { provider: "codex" } as { provider: string | null } }, + ); + let submission!: Promise; + const captureError = (error: unknown) => error; + await act(async () => { + submission = result.current + .handleCreateFromInput({ text: "build this", attachments: [], cwd: "/repo" }) + .catch(captureError); + }); + rerender({ provider: null }); + expect(result.current.draftAgent).toEqual({ provider: "codex" }); + await act(async () => { + failRequest(new Error("Provider unavailable")); + await submission; + }); + expect(result.current.isSubmitting).toBe(false); + expect(result.current.draftAgent).toBeNull(); + expect(result.current.formErrorMessage).toBe("Provider unavailable"); + rerender({ provider: "claude" }); + await act(async () => { + await result.current.handleCreateFromInput({ + text: "try again", + attachments: [], + cwd: "/repo", + }); + }); + expect(result.current.draftAgent).toEqual({ provider: "claude" }); + expect(result.current.formErrorMessage).toBe(""); + expect(requestCount).toBe(2); + }); + it("allows retrying an empty prompt when the draft still has context attachments", async () => { const attachment = { kind: "chat_history", diff --git a/packages/app/src/composer/draft/create-flow.ts b/packages/app/src/composer/draft/create-flow.ts index 89a237e786..d1cfce1161 100644 --- a/packages/app/src/composer/draft/create-flow.ts +++ b/packages/app/src/composer/draft/create-flow.ts @@ -27,23 +27,23 @@ interface CreateAttempt { attachments?: AgentAttachment[]; } -type DraftAgentMachineState = +type DraftAgentMachineState = | { tag: "draft"; errorMessage: string } - | { tag: "creating"; attempt: CreateAttempt }; + | { tag: "creating"; attempt: CreateAttempt; draftAgent: TDraftAgent }; -type DraftAgentMachineEvent = +type DraftAgentMachineEvent = | { type: "DRAFT_SET_ERROR"; message: string } - | { type: "SUBMIT"; attempt: CreateAttempt } + | { type: "SUBMIT"; attempt: CreateAttempt; draftAgent: TDraftAgent } | { type: "CREATE_FAILED"; message: string }; function assertNever(value: never): never { throw new Error(`Unhandled state: ${JSON.stringify(value)}`); } -function reducer( - state: DraftAgentMachineState, - event: DraftAgentMachineEvent, -): DraftAgentMachineState { +function reducer( + state: DraftAgentMachineState, + event: DraftAgentMachineEvent, +): DraftAgentMachineState { switch (event.type) { case "DRAFT_SET_ERROR": { if (state.tag !== "draft") { @@ -52,7 +52,7 @@ function reducer( return { ...state, errorMessage: event.message }; } case "SUBMIT": { - return { tag: "creating", attempt: event.attempt }; + return { tag: "creating", attempt: event.attempt, draftAgent: event.draftAgent }; } case "CREATE_FAILED": { if (state.tag !== "creating") { @@ -65,6 +65,17 @@ function reducer( } } +function prepareCreateAttempt( + attempt: CreateAttempt, + buildDraftAgent: (attempt: CreateAttempt) => TDraftAgent, +): DraftAgentMachineState { + try { + return { tag: "creating", attempt, draftAgent: buildDraftAgent(attempt) }; + } catch (error) { + return { tag: "draft", errorMessage: error instanceof Error ? error.message : String(error) }; + } +} + interface CreateRequestResult { agentId: string | null; result: TCreateResult; @@ -113,11 +124,11 @@ export function useDraftAgentCreateFlow({ }: UseDraftAgentCreateFlowOptions) { const { t } = useTranslation(); const [machine, dispatch] = useReducer( - reducer, + reducer, initialAttempt, - (attempt): DraftAgentMachineState => + (attempt): DraftAgentMachineState => attempt - ? { tag: "creating", attempt } + ? prepareCreateAttempt(attempt, buildDraftAgent) : { tag: "draft", errorMessage: "", @@ -163,12 +174,18 @@ export function useDraftAgentCreateFlow({ ]; }, [machine]); - const draftAgent = useMemo(() => { - if (machine.tag !== "creating") { - return null; - } - return buildDraftAgent(machine.attempt); - }, [buildDraftAgent, machine]); + const draftAgent = machine.tag === "creating" ? machine.draftAgent : null; + const startCreateAttempt = useCallback( + (attempt: CreateAttempt) => { + const prepared = prepareCreateAttempt(attempt, buildDraftAgent); + if (prepared.tag === "draft") { + dispatch({ type: "DRAFT_SET_ERROR", message: prepared.errorMessage }); + throw new Error(prepared.errorMessage); + } + dispatch({ type: "SUBMIT", attempt, draftAgent: prepared.draftAgent }); + }, + [buildDraftAgent], + ); const runCreateAttempt = useCallback( async ({ attempt, cwd }: { attempt: CreateAttempt; cwd: string }) => { @@ -179,15 +196,14 @@ export function useDraftAgentCreateFlow({ throw error; } - await onBeforeSubmit?.({ - attempt, - text: attempt.text, - images: attempt.images, - attachments: attempt.attachments, - cwd, - }); - try { + await onBeforeSubmit?.({ + attempt, + text: attempt.text, + images: attempt.images, + attachments: attempt.attachments, + cwd, + }); const createResult = await createRequest({ attempt, text: attempt.text, @@ -287,6 +303,7 @@ export function useDraftAgentCreateFlow({ ...(wirePayload.attachments.length > 0 ? { attachments: wirePayload.attachments } : {}), }; + startCreateAttempt(attempt); setPendingCreateAttempt({ draftId, serverId: pendingServerId, @@ -300,7 +317,6 @@ export function useDraftAgentCreateFlow({ : {}), }); - dispatch({ type: "SUBMIT", attempt }); onCreateStart?.(); await runCreateAttempt({ attempt, cwd }); }, @@ -312,6 +328,7 @@ export function useDraftAgentCreateFlow({ onCreateStart, runCreateAttempt, setPendingCreateAttempt, + startCreateAttempt, t, validateBeforeSubmit, ], @@ -320,11 +337,11 @@ export function useDraftAgentCreateFlow({ const continueCreateFromAttempt = useCallback( async ({ attempt, cwd }: { attempt: CreateAttempt; cwd: string }) => { if (!isSubmitting) { - dispatch({ type: "SUBMIT", attempt }); + startCreateAttempt(attempt); } await runCreateAttempt({ attempt, cwd }); }, - [isSubmitting, runCreateAttempt], + [isSubmitting, runCreateAttempt, startCreateAttempt], ); return { diff --git a/packages/server/src/server/agent/plugin-provider.ts b/packages/server/src/server/agent/plugin-provider.ts index c2eb5e08de..a9dd271220 100644 --- a/packages/server/src/server/agent/plugin-provider.ts +++ b/packages/server/src/server/agent/plugin-provider.ts @@ -805,6 +805,7 @@ function createPluginProviderDefinition( ): ProviderDefinition { return { id: registration.id, + configuration: null, label: registration.label, description: registration.description ?? `Plugin provider ${registration.label}`, iconSvg: registration.icon, diff --git a/packages/server/src/server/agent/provider-registry.ts b/packages/server/src/server/agent/provider-registry.ts index 5c9231f095..ac70c936cb 100644 --- a/packages/server/src/server/agent/provider-registry.ts +++ b/packages/server/src/server/agent/provider-registry.ts @@ -70,6 +70,8 @@ export type { AgentProviderDefinition }; export { AGENT_PROVIDER_DEFINITIONS, getAgentProviderDefinition }; export interface ProviderDefinition extends AgentProviderDefinition { + /** Effective inputs after overrides and inheritance; plugin registrations are owned separately. */ + configuration: Omit | null; iconSvg?: string; enabled: boolean; /** @@ -608,8 +610,10 @@ function createRegistryEntry( const hasStaticModes = resolved.definition.modes.length > 0; + const { createBaseClient: _createBaseClient, contract: _contract, ...configuration } = resolved; return { ...resolved.definition, + configuration, enabled: resolved.enabled, derivedFromProviderId: resolved.derivedFromProviderId, optionsSchema: resolved.contract.optionsSchema, 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 21f8f51626..f30cd6e92c 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.test.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.test.ts @@ -1458,6 +1458,155 @@ describe("ProviderSnapshotManager applyMutableProviderConfig", () => { } }); + test("changing one provider preserves other catalogs and clients across directories", async () => { + const calls = { claude: 0, codex: 0 }; + const clients = Object.fromEntries( + (["claude", "codex"] as const).map((provider) => [ + provider, + createExtraClient(provider, { + async isAvailable() { + return true; + }, + async fetchCatalog() { + calls[provider]++; + return { models: [], modes: [] }; + }, + }), + ]), + ); + const config = { + claude: { enabled: true }, + codex: { enabled: true, command: ["codex"], env: { TEST_SETTING: "same" } }, + copilot: { enabled: false }, + opencode: { enabled: false }, + pi: { enabled: false }, + omp: { enabled: false }, + }; + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + providerOverrides: config, + runtimeSettings: { + codex: { command: { mode: "replace", argv: ["codex"] }, env: { TEST_SETTING: "same" } }, + }, + extraClients: clients, + }); + const cwds = [resolve("/tmp/catalog-a"), resolve("/tmp/catalog-b")]; + try { + for (const cwd of cwds) await manager.warmUpSnapshotForCwd({ cwd }); + const getCodexEntry = (cwd: string) => + manager.getSnapshot(cwd).find((entry) => entry.provider === "codex"); + const before = cwds.map(getCodexEntry); + const definition = manager.getAgentManagerProviderState().providerDefinitions.codex; + manager.applyMutableProviderConfig( + { ...config, claude: { enabled: true, label: "Renamed" } }, + { replace: true }, + ); + expect(cwds.map(getCodexEntry)).toEqual(before); + expect(manager.getAgentManagerProviderState().providerDefinitions.codex).toEqual(definition); + for (const cwd of cwds) await manager.warmUpSnapshotForCwd({ cwd }); + expect(calls).toEqual({ claude: 4, codex: 2 }); + const listener = vi.fn(); + manager.on("change", listener); + manager.applyMutableProviderConfig( + { ...config, claude: { label: "Renamed", enabled: true } }, + { replace: true }, + ); + for (const cwd of cwds) await manager.warmUpSnapshotForCwd({ cwd }); + expect(calls).toEqual({ claude: 4, codex: 2 }); + expect(listener).not.toHaveBeenCalled(); + } finally { + manager.destroy(); + } + }); + + test("reload preserves an unchanged lookup in flight through commit and rollback", async () => { + let finish!: () => void; + const gate = new Promise((complete) => { + finish = complete; + }); + let calls = 0; + const config = { + claude: { enabled: false }, + codex: { enabled: true }, + copilot: { enabled: false }, + opencode: { enabled: false }, + pi: { enabled: false }, + omp: { enabled: false }, + }; + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + providerOverrides: config, + extraClients: { + codex: createExtraClient("codex", { + async isAvailable() { + return true; + }, + async fetchCatalog() { + calls++; + await gate; + return { models: [], modes: [] }; + }, + }), + }, + }); + const cwd = resolve("/tmp/catalog-pending"); + try { + const pending = manager.warmUpSnapshotForCwd({ cwd }); + await vi.waitFor(() => expect(calls).toBe(1)); + manager.applyMutableProviderConfig( + { ...config, claude: { enabled: false, label: "Changed" } }, + { replace: true }, + ); + const staged = manager.stageMutableProviderConfig( + { ...config, claude: { enabled: false, label: "Rollback" } }, + { replace: true }, + ); + staged.rollback(); + finish(); + await pending; + expect(manager.getSnapshot(cwd).find((e) => e.provider === "codex")?.status).toBe("ready"); + expect(calls).toBe(1); + } finally { + finish(); + manager.destroy(); + } + }); + + test("reload replaces derived clients only when their provider configuration changes", () => { + const config = { + claude: { enabled: true }, + codex: { enabled: true }, + "codex-2": { extends: "codex", label: "Codex 2", enabled: true }, + copilot: { enabled: false }, + opencode: { enabled: false }, + pi: { enabled: false }, + omp: { enabled: false }, + }; + const manager = new ProviderSnapshotManager({ + logger: createTestLogger(), + providerOverrides: config, + }); + try { + const before = manager.getAgentManagerProviderState().clients; + const unchanged = manager.applyMutableProviderConfig(config, { replace: true }).clients; + expect(unchanged.codex).toBe(before.codex); + expect(unchanged["codex-2"]).toBe(before["codex-2"]); + const changed = manager.applyMutableProviderConfig( + { ...config, codex: { enabled: true, env: { CODEX_HOME: "/tmp/other-codex-home" } } }, + { replace: true }, + ).clients; + expect(changed.claude).toBe(before.claude); + expect(changed.codex).not.toBe(before.codex); + expect(changed["codex-2"]).not.toBe(before["codex-2"]); + const { "codex-2": _removedProvider, ...withoutDerived } = config; + const removed = manager.applyMutableProviderConfig(withoutDerived, { replace: true }).clients; + expect(removed["codex-2"]).toBeUndefined(); + expect(removed.claude).toBe(before.claude); + } finally { + manager.destroy(); + } + }); + test("stages provider state without events, then publishes or rolls back", () => { const manager = new ProviderSnapshotManager({ logger: createTestLogger(), @@ -1597,12 +1746,12 @@ describe("ProviderSnapshotManager lifecycle", () => { const listener = vi.fn(); manager.on("change", listener); manager.getSnapshot("/tmp/project"); - manager.applyMutableProviderConfig({}); + manager.applyMutableProviderConfig({ claude: { enabled: false, label: "Changed" } }); const firstCallCount = listener.mock.calls.length; - expect(firstCallCount).toBeGreaterThan(0); + expect(firstCallCount).toBe(1); manager.off("change", listener); - manager.applyMutableProviderConfig({}); + manager.applyMutableProviderConfig({ claude: { enabled: false, label: "Changed again" } }); expect(listener.mock.calls.length).toBe(firstCallCount); } finally { manager.destroy(); @@ -1724,7 +1873,7 @@ describe("ProviderSnapshotManager cwd routing", () => { const listener = vi.fn(); manager.on("change", listener); manager.getSnapshot(); - manager.applyMutableProviderConfig({}); + manager.applyMutableProviderConfig({ claude: { enabled: false, label: "Changed" } }); const cwds = listener.mock.calls.map((call) => call[1]); expect(cwds).toContain(GLOBAL_PROVIDER_SNAPSHOT_KEY); } finally { diff --git a/packages/server/src/server/agent/provider-snapshot-manager.ts b/packages/server/src/server/agent/provider-snapshot-manager.ts index 5b60f68cca..5217a1c738 100644 --- a/packages/server/src/server/agent/provider-snapshot-manager.ts +++ b/packages/server/src/server/agent/provider-snapshot-manager.ts @@ -1,6 +1,7 @@ import { EventEmitter } from "node:events"; import { homedir } from "node:os"; import { resolve } from "node:path"; +import { isDeepStrictEqual } from "node:util"; import type { Logger } from "pino"; @@ -546,28 +547,38 @@ export class ProviderSnapshotManager { ); // The mutable config is the complete provider source after startup. Keeping // startup-derived runtime settings here would retain removed command/env fields. - if (options.replace) this.runtimeSettings = undefined; - this.providerRegistry = this.buildRegistry(); - this.providerClients = { - ...this.extraClients, - ...this.pluginProviders.clients(), - } as Record; + const nextRegistry = this.buildRegistry(); + const changedProviders = this.findChangedProviders(previous, nextRegistry); - for (const cwd of this.snapshots.keys()) { - this.providerLoads.delete(cwd); - this.snapshots.set(cwd, this.reconcileSnapshotForRegistry(cwd)); + this.providerRegistry = nextRegistry; + this.providerClients = { ...previous.providerClients }; + for (const provider of changedProviders) delete this.providerClients[provider]; + for (const provider of Object.keys(nextRegistry)) { + if (!changedProviders.has(provider)) { + nextRegistry[provider] = previous.providerRegistry[provider]!; + } + } + Object.assign(this.providerClients, this.extraClients, this.pluginProviders.clients()); + const providersToRefresh = [...changedProviders].filter((provider) => nextRegistry[provider]); + + for (const cwd of snapshotCwds) { + const loads = new Map(this.providerLoads.get(cwd)); + for (const provider of changedProviders) loads.delete(provider); + this.providerLoads.set(cwd, loads); + this.snapshots.set(cwd, this.reconcileSnapshotForRegistry(cwd, changedProviders)); } return { agentManagerState: this.getAgentManagerProviderState(), publish: () => { + if (changedProviders.size === 0) return; for (const cwd of snapshotCwds) { this.emitChange(cwd); const target = cwd === GLOBAL_PROVIDER_SNAPSHOT_KEY ? createGlobalSnapshotTarget() : createWorkspaceSnapshotTarget(cwd); - const providers = this.resolveProvidersToWarm(cwd); + const providers = this.resolveProvidersToWarm(cwd, providersToRefresh); if (providers.length > 0) void this.warmUp(target, providers); } }, @@ -579,6 +590,25 @@ export class ProviderSnapshotManager { } } + private findChangedProviders( + previous: MutableProviderState, + nextRegistry: Record, + ): Set { + const providers = new Set([ + ...Object.keys(previous.providerRegistry), + ...Object.keys(nextRegistry), + ]); + const changed = new Set(); + for (const provider of providers) { + const before = previous.providerRegistry[provider]; + const after = nextRegistry[provider]; + if (!before || !after || !isDeepStrictEqual(before.configuration, after.configuration)) { + changed.add(provider); + } + } + return changed; + } + private captureMutableProviderState(): MutableProviderState { return { baseProviderOverrides: this.baseProviderOverrides, @@ -779,7 +809,7 @@ export class ProviderSnapshotManager { const definition = this.providerRegistry[provider]; entries.set(provider, { provider, - status: "loading", + status: definition?.enabled === false ? "unavailable" : "loading", enabled: definition?.enabled ?? true, source: this.getProviderSource(provider), label: definition?.label, @@ -791,13 +821,20 @@ export class ProviderSnapshotManager { return entries; } - private reconcileSnapshotForRegistry(cwd: string): Map { + private reconcileSnapshotForRegistry( + cwd: string, + changedProviders?: ReadonlySet, + ): Map { const existing = this.snapshots.get(cwd); const entries = new Map(); for (const provider of this.getProviderIds()) { const definition = this.providerRegistry[provider]; const current = existing?.get(provider); + if (current && changedProviders && !changedProviders.has(provider)) { + entries.set(provider, current); + continue; + } const metadata = { provider, enabled: definition?.enabled ?? true, @@ -967,7 +1004,6 @@ export class ProviderSnapshotManager { force: boolean; }): Promise { const { snapshotCwd, catalogScope, provider, definition, load, force } = options; - const snapshot = this.getOrCreateSnapshot(snapshotCwd); const base = { provider, source: this.getProviderSource(provider), @@ -980,7 +1016,8 @@ export class ProviderSnapshotManager { if (!this.isCurrentProviderLoad(snapshotCwd, provider, load)) { return false; } - snapshot.set(provider, entry); + // A config transaction may replace the map while this unchanged provider is loading. + this.getOrCreateSnapshot(snapshotCwd).set(provider, entry); this.emitChange(snapshotCwd); return true; };