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; };