From d910d878f4d22e6a4d4593ba6e3a5b44a50588b7 Mon Sep 17 00:00:00 2001 From: macodev00 <273427913+macodev00@users.noreply.github.com> Date: Sun, 4 Oct 2026 07:20:53 +0000 Subject: [PATCH] fix(providers): keep in-flight work running when a provider is renamed A display name or accent color edit rebuilt the provider instance and closed its scope, which stopped every thread using it. Update presentation in place, including the first rename that only lifts a legacy config.enabled flag, and ask before deleting a provider that still has an in-flight thread. --- ...rInstanceRegistryLive.presentation.test.ts | 215 ++++++++++++++++++ .../Layers/ProviderInstanceRegistryLive.ts | 90 ++++++-- .../src/provider/Layers/ProviderRegistry.ts | 37 +++ ...ProviderSettingsPanel.environment.test.tsx | 64 +++++- .../settings/ProviderSettingsPanel.tsx | 55 +++++ packages/contracts/src/settings.test.ts | 49 ++++ packages/contracts/src/settings.ts | 45 ++++ 7 files changed, 537 insertions(+), 18 deletions(-) create mode 100644 apps/server/src/provider/Layers/ProviderInstanceRegistryLive.presentation.test.ts diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.presentation.test.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.presentation.test.ts new file mode 100644 index 000000000000..5d09ebef7c01 --- /dev/null +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.presentation.test.ts @@ -0,0 +1,215 @@ +/** + * Presentation edits must not rebuild a live provider instance. + * + * Renaming or recoloring, including the first rename that lifts a legacy + * `config.enabled` flag onto the envelope, keeps the same instance object, + * scope, and in-flight fiber. Deleting the instance or changing runtime + * settings still closes that scope. + */ +import { describe, expect, it } from "@effect/vitest"; +import { ProviderDriverKind, ProviderInstanceId, type ServerProvider } from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; +import * as Fiber from "effect/Fiber"; +import * as PubSub from "effect/PubSub"; +import * as Schema from "effect/Schema"; +import * as Stream from "effect/Stream"; + +import { + defaultProviderContinuationIdentity, + type ProviderDriver, + type ProviderInstance, +} from "../ProviderDriver.ts"; +import { makeProviderInstanceRegistry } from "./ProviderInstanceRegistryLive.ts"; + +const driverKind = ProviderDriverKind.make("codex"); +const instanceId = ProviderInstanceId.make("codex"); + +const makeSnapshot = ( + input: Pick, +): ServerProvider => + ({ + instanceId: input.instanceId, + driver: driverKind, + ...(input.displayName ? { displayName: input.displayName } : { displayName: "Stamped" }), + ...(input.accentColor ? { accentColor: input.accentColor } : { accentColor: "#000000" }), + enabled: input.enabled, + installed: true, + version: null, + status: "ready", + auth: { status: "unknown" }, + checkedAt: "2026-01-01T00:00:00.000Z", + models: [], + slashCommands: [], + skills: [], + }) as ServerProvider; + +const makeDriver = (fibers: Array>) => { + let closed = 0; + const driver = { + driverKind, + metadata: { displayName: "Codex" }, + configSchema: Schema.Unknown as ProviderDriver["configSchema"], + defaultConfig: () => ({}), + create: (input) => + Effect.gen(function* () { + const fiber = yield* Effect.never.pipe(Effect.forkScoped); + fibers.push(fiber); + yield* Effect.addFinalizer(() => + Effect.sync(() => { + closed += 1; + }), + ); + const snapshot = makeSnapshot(input); + const instance: ProviderInstance = { + instanceId: input.instanceId, + driverKind, + continuationIdentity: defaultProviderContinuationIdentity({ + driverKind, + instanceId: input.instanceId, + }), + displayName: input.displayName, + ...(input.accentColor ? { accentColor: input.accentColor } : {}), + enabled: input.enabled, + snapshot: { + resolveMaintenance: () => Effect.die("unused"), + getSnapshot: Effect.succeed(snapshot), + refresh: Effect.succeed(snapshot), + streamChanges: Stream.succeed(snapshot), + applyUsageLimits: () => Effect.void, + }, + snapshotForCwd: () => Effect.succeed(snapshot), + orchestrationAdapter: {} as ProviderInstance["orchestrationAdapter"], + textGeneration: {} as ProviderInstance["textGeneration"], + }; + return instance; + }), + } satisfies ProviderDriver; + return { + driver, + closed: () => closed, + }; +}; + +/** + * `undefined` while the fiber is still running. Effect 4 exposes that as + * `pollUnsafe` rather than an effectful poll. + */ +const fiberExit = (fiber: Fiber.Fiber) => Effect.sync(() => fiber.pollUnsafe()); + +describe("ProviderInstanceRegistryLive presentation", () => { + it.effect("keeps the live scope across a rename and closes it for runtime edits", () => + Effect.gen(function* () { + const fibers: Array> = []; + const { driver, closed } = makeDriver(fibers); + const entry = { + driver: driverKind, + displayName: "Personal", + accentColor: "#123456", + enabled: true, + environment: [{ name: "CODEX_HOME", value: "personal", sensitive: false }], + config: { binaryPath: "codex" }, + }; + const { registry, mutator } = yield* makeProviderInstanceRegistry({ + drivers: [driver], + configMap: { [instanceId]: entry }, + }); + const original = yield* registry.getInstance(instanceId); + expect(original).toBeDefined(); + yield* mutator.reconcile({ [instanceId]: { ...entry } }); + expect(yield* registry.getInstance(instanceId)).toBe(original); + expect(closed()).toBe(0); + expect(yield* fiberExit(fibers[0]!)).toBeUndefined(); + const changes = yield* registry.subscribeChanges; + + yield* mutator.reconcile({ + [instanceId]: { ...entry, displayName: "Work", accentColor: "#654321" }, + }); + const renamed = yield* registry.getInstance(instanceId); + expect(renamed).toBe(original); + expect(renamed?.orchestrationAdapter).toBe(original?.orchestrationAdapter); + expect(renamed?.displayName).toBe("Work"); + expect(renamed?.accentColor).toBe("#654321"); + expect(closed()).toBe(0); + expect(yield* fiberExit(fibers[0]!)).toBeUndefined(); + for (const snapshot of yield* Effect.all([ + renamed!.snapshot.getSnapshot, + renamed!.snapshot.refresh, + renamed!.snapshotForCwd!("/work"), + ])) { + expect(snapshot.displayName).toBe("Work"); + expect(snapshot.accentColor).toBe("#654321"); + } + expect(yield* fiberExit(fibers[0]!)).toBeUndefined(); + yield* PubSub.take(changes); + + yield* mutator.reconcile({ + [instanceId]: { + driver: driverKind, + enabled: true, + environment: entry.environment, + config: { binaryPath: "codex" }, + }, + }); + const cleared = yield* registry.getInstance(instanceId); + expect(cleared).toBe(original); + expect(cleared?.displayName).toBeUndefined(); + expect(cleared?.accentColor).toBeUndefined(); + expect((yield* cleared!.snapshot.getSnapshot).displayName).toBeUndefined(); + expect((yield* cleared!.snapshot.getSnapshot).accentColor).toBeUndefined(); + expect(closed()).toBe(0); + + yield* mutator.reconcile({ + [instanceId]: { + ...entry, + displayName: "Work", + environment: [{ name: "CODEX_HOME", value: "work", sensitive: false }], + }, + }); + const replaced = yield* registry.getInstance(instanceId); + expect(replaced).not.toBe(original); + expect(replaced?.orchestrationAdapter).not.toBe(original?.orchestrationAdapter); + expect(closed()).toBe(1); + const replacedExit = yield* fiberExit(fibers[0]!); + expect(replacedExit !== undefined && Exit.hasInterrupts(replacedExit)).toBe(true); + expect(yield* fiberExit(fibers[1]!)).toBeUndefined(); + + yield* mutator.reconcile({}); + expect(yield* registry.listInstances).toEqual([]); + expect(closed()).toBe(2); + const deletedExit = yield* fiberExit(fibers[1]!); + expect(deletedExit !== undefined && Exit.hasInterrupts(deletedExit)).toBe(true); + }), + ); + + it.effect("keeps a default instance alive when its first rename lifts config.enabled", () => + Effect.gen(function* () { + const fibers: Array> = []; + const { driver, closed } = makeDriver(fibers); + const { registry, mutator } = yield* makeProviderInstanceRegistry({ + drivers: [driver], + configMap: { + [instanceId]: { + driver: driverKind, + config: { enabled: true, binaryPath: "codex", launchArgs: "" }, + }, + }, + }); + const original = yield* registry.getInstance(instanceId); + yield* mutator.reconcile({ + [instanceId]: { + driver: driverKind, + enabled: true, + displayName: "Personal", + config: { binaryPath: "codex", launchArgs: "" }, + }, + }); + const renamed = yield* registry.getInstance(instanceId); + expect(renamed).toBe(original); + expect(renamed?.displayName).toBe("Personal"); + expect(closed()).toBe(0); + expect(yield* fiberExit(fibers[0]!)).toBeUndefined(); + expect((yield* renamed!.snapshot.getSnapshot).displayName).toBe("Personal"); + }), + ); +}); diff --git a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts index 4d59d2f57f98..c75b420b9d86 100644 --- a/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts +++ b/apps/server/src/provider/Layers/ProviderInstanceRegistryLive.ts @@ -23,7 +23,10 @@ * (`Ref`s + `PubSub`) and exposes an internal mutator * (`ProviderInstanceRegistryMutator`) whose `reconcile` method diffs a * fresh config map against the live state, tearing down removed instances - * and building new ones without disturbing unaffected instances. + * and building new ones without disturbing unaffected instances. A display + * name or accent color edit, including the first rename that only lifts a + * legacy `config.enabled` flag onto the envelope, updates the live instance + * in place and leaves its scope running. * * Every live instance runs inside its own child `Scope`. The registry's * own scope owns all child scopes via finalizers, so closing the registry @@ -34,6 +37,7 @@ */ import { providerInstanceConfigEnabledFlag, + providerInstanceRuntimeConfigEqual, ProviderInstanceId, type ProviderInstanceConfig, type ProviderInstanceConfigMap, @@ -64,7 +68,7 @@ import type { AnyProviderDriver, ProviderInstance } from "../ProviderDriver.ts"; interface LiveEntry { readonly instance: ProviderInstance; readonly scope: Scope.Closeable; - readonly entry: ProviderInstanceConfig; + entry: ProviderInstanceConfig; } /** @@ -78,14 +82,61 @@ interface RegistryState { } /** - * Structural equality on `ProviderInstanceConfig` envelopes. Used by - * `reconcile` to skip rebuilds when settings arrive unchanged. Config - * payloads are opaque `unknown` at the envelope layer; `Equal.equals` - * falls back to structural equality for plain records, which matches how - * the schema decode output is constructed. + * Stamp the envelope's current display name and accent color onto a snapshot. + * + * Drivers capture presentation once inside `create`. Reading it from the + * entry `reconcile` keeps up to date lets a rename show up on later reads + * without closing the instance scope. */ -const entryEqual = (a: ProviderInstanceConfig, b: ProviderInstanceConfig): boolean => - Equal.equals(a, b); +const overlayProviderPresentation = ( + entry: ProviderInstanceConfig, + snapshot: ServerProvider, +): ServerProvider => { + const { displayName: _displayName, accentColor: _accentColor, ...rest } = snapshot; + return { + ...rest, + ...(entry.displayName ? { displayName: entry.displayName } : {}), + ...(entry.accentColor ? { accentColor: entry.accentColor } : {}), + }; +}; + +/** + * Return a stable instance whose presentation follows `source.entry`. + * + * The instance object, its adapter, and the scope it was created in stay + * put. `reconcile` assigns the next envelope onto that same entry. + */ +const presentProviderInstance = ( + source: { readonly entry: ProviderInstanceConfig }, + instance: ProviderInstance, +): ProviderInstance => { + const { snapshotForCwd } = instance; + /** + * Read presentation from the entry at call time, including probe emissions + * that still carry the name captured when the driver was created. + */ + const present = (snapshot: ServerProvider) => overlayProviderPresentation(source.entry, snapshot); + return { + ...instance, + get displayName() { + return source.entry.displayName; + }, + get accentColor() { + return source.entry.accentColor; + }, + snapshot: { + ...instance.snapshot, + getSnapshot: instance.snapshot.getSnapshot.pipe(Effect.map(present)), + refresh: instance.snapshot.refresh.pipe(Effect.map(present)), + streamChanges: instance.snapshot.streamChanges.pipe(Stream.map(present)), + }, + ...(snapshotForCwd + ? { + snapshotForCwd: (cwd: string) => snapshotForCwd(cwd).pipe(Effect.map(present)), + } + : {}), + }; +}; /** * Resolve an entry's enabled state. An explicit false on either the @@ -196,12 +247,21 @@ const buildEntry = (input: { }; } + // Presentation getters read this holder, and `reconcile` writes it through + // the live entry's `entry` setter, so a rename updates both together. + const presentation: { entry: ProviderInstanceConfig } = { entry }; return { kind: "live" as const, live: { - instance: createResult.success, scope: childScope, - entry, + instance: presentProviderInstance(presentation, createResult.success), + /** Envelope `reconcile` updates in place so presentation getters follow it. */ + get entry() { + return presentation.entry; + }, + set entry(next: ProviderInstanceConfig) { + presentation.entry = next; + }, }, }; }); @@ -236,7 +296,7 @@ const makeReconcile = (input: { continue; } const nextEntry = configMap[instanceId]; - if (nextEntry !== undefined && !entryEqual(live.entry, nextEntry)) { + if (nextEntry !== undefined && !providerInstanceRuntimeConfigEqual(live.entry, nextEntry)) { replacedIds.add(instanceId); } } @@ -252,6 +312,7 @@ const makeReconcile = (input: { const builtEntries = new Map(); const builtUnavailable = new Map(); let orderChanged = false; + let presentationChanged = false; const previousOrder = [...previousEntries.keys()]; const nextOrder: Array = []; @@ -261,7 +322,9 @@ const makeReconcile = (input: { const existing = previousEntries.get(instanceId); if (existing !== undefined && !replacedIds.has(instanceId)) { - // No-op update: keep the existing live entry and scope. + presentationChanged ||= !Equal.equals(existing.entry, entry); + existing.entry = entry; + // Presentation edits keep the adapter, subscriptions, and scope alive. builtEntries.set(instanceId, existing); continue; } @@ -292,6 +355,7 @@ const makeReconcile = (input: { } const entriesChanged = + presentationChanged || orderChanged || removedIds.length > 0 || replacedIds.size > 0 || diff --git a/apps/server/src/provider/Layers/ProviderRegistry.ts b/apps/server/src/provider/Layers/ProviderRegistry.ts index de476ca0299a..699b21711db2 100644 --- a/apps/server/src/provider/Layers/ProviderRegistry.ts +++ b/apps/server/src/provider/Layers/ProviderRegistry.ts @@ -279,6 +279,20 @@ const haveProvidersChanged = ( nextProviders: ReadonlyArray, ): boolean => !Equal.equals(previousProviders, nextProviders); +/** + * Report whether a retained instance's snapshot now carries a different + * display name or accent color than the aggregated provider list. + * + * A missing current snapshot still counts, so the first read is published. + */ +const providerPresentationChanged = ( + current: Pick | undefined, + next: Pick, +): boolean => + current === undefined || + current.displayName !== next.displayName || + current.accentColor !== next.accentColor; + const correlateSnapshotWithSource = ( source: ProviderSnapshotSource, snapshot: ServerProvider, @@ -768,6 +782,29 @@ export const ProviderRegistryLive = Layer.effect( }).pipe(Effect.ignoreCause({ log: true })), { concurrency: "unbounded", discard: true }, ); + /** + * Publish a retained instance when its display name or accent color + * changed. The subscription and in-flight scope stay in place. + */ + const publishRetainedPresentation = (instance: ProviderInstance) => + Effect.gen(function* () { + if (!carriedOver.has(instance.instanceId)) { + return; + } + const source = buildSnapshotSource(instance); + const provider = yield* source.getSnapshot; + const current = (yield* Ref.get(providersRef)).find( + (candidate) => candidate.instanceId === instance.instanceId, + ); + if (!providerPresentationChanged(current, provider)) { + return; + } + yield* correlateSnapshotWithSource(source, provider).pipe(Effect.flatMap(syncProvider)); + }).pipe(Effect.ignoreCause({ log: true })); + yield* Effect.forEach(instances, publishRetainedPresentation, { + concurrency: "unbounded", + discard: true, + }); yield* upsertProviders(unavailableProviders, { persist: false, replace: true, diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx index dcb4ba7ef7eb..36cb383e2549 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.environment.test.tsx @@ -15,6 +15,12 @@ import { reactHookHarness as hooks } from "../../test/reactHookHarness"; const atoms = vi.hoisted(() => ({ providers: null as ReadonlyArray | null, providersAtom: Symbol("providers"), + threads: [] as ReadonlyArray<{ + providerInstanceId: ProviderInstanceId; + status: string; + activityRunStatus?: string | null; + }>, + threadsAtom: Symbol("threads"), refreshProviders: Symbol("refreshProviders"), updateProvider: Symbol("updateProvider"), uninstallAcpRegistryManagedBinary: Symbol("uninstallAcpRegistryManagedBinary"), @@ -22,12 +28,17 @@ const atoms = vi.hoisted(() => ({ })); const commands = vi.hoisted(() => ({ + confirm: vi.fn(async () => true), refresh: vi.fn(), updateProvider: vi.fn(), uninstall: vi.fn(), acceptUrlAuth: vi.fn(), })); +vi.mock("../../localApi", () => ({ + ensureLocalApi: () => ({ dialogs: { confirm: commands.confirm } }), +})); + const settingsState = vi.hoisted(() => ({ value: null as UnifiedSettings | null, readEnvironmentIds: [] as EnvironmentId[], @@ -71,7 +82,13 @@ vi.mock("react/compiler-runtime", async () => { }); vi.mock("@effect/atom-react", () => ({ - useAtomValue: () => atoms.providers, + useAtomValue: (atom: symbol) => (atom === atoms.threadsAtom ? atoms.threads : atoms.providers), +})); + +vi.mock("../../state/threads", () => ({ + environmentThreadShells: { + environmentThreadsAtom: () => atoms.threadsAtom, + }, })); vi.mock("../../state/server", () => ({ @@ -198,6 +215,8 @@ describe("EnvironmentProviderSettings routing", () => { beforeEach(() => { hooks.reset(); atoms.providers = null; + atoms.threads = []; + commands.confirm.mockReset().mockResolvedValue(true); settingsState.value = DEFAULT_UNIFIED_SETTINGS; settingsState.readEnvironmentIds = []; settingsState.updateEnvironmentIds = []; @@ -436,6 +455,7 @@ describe("EnvironmentProviderSettings routing", () => { }, favorites: [{ provider: customId, model: "favorite" }], }; + atoms.threads = [{ providerInstanceId: codexId, status: "running" }]; let panel = renderPanel(); const customRow = visitElements( panel, @@ -448,9 +468,9 @@ describe("EnvironmentProviderSettings routing", () => { (element) => element.props.instanceId === customId && element.props.mode === "editor", ); expect(customCard).not.toBeNull(); - (customCard?.props.onDelete as (() => void) | undefined)?.(); - await flushPromises(); + await (customCard?.props.onDelete as (() => Promise) | undefined)?.(); + expect(commands.confirm).not.toHaveBeenCalled(); expect(settingsState.mutateProviderInstance).toHaveBeenLastCalledWith({ operation: "remove", instanceId: customId, @@ -510,6 +530,40 @@ describe("EnvironmentProviderSettings routing", () => { ); }); + it("asks before deleting a provider with an in-flight thread and leaves it running on cancel", async () => { + settingsState.value = { + ...DEFAULT_UNIFIED_SETTINGS, + providerInstances: { + [customId]: { + driver: ProviderDriverKind.make("codex"), + enabled: true, + displayName: "Work", + }, + }, + }; + atoms.threads = [ + { providerInstanceId: codexId, status: "running" }, + { providerInstanceId: customId, status: "idle", activityRunStatus: "running" }, + ]; + const panel = renderPanel({ targetInstanceId: customId }); + const card = visitElements( + panel, + (element) => element.props.instanceId === customId && element.props.mode === "editor", + ); + commands.confirm.mockResolvedValueOnce(false); + await (card?.props.onDelete as (() => Promise) | undefined)?.(); + + expect(commands.confirm).toHaveBeenCalledOnce(); + expect(settingsState.mutateProviderInstance).not.toHaveBeenCalled(); + + commands.confirm.mockResolvedValueOnce(true); + await (card?.props.onDelete as (() => Promise) | undefined)?.(); + expect(settingsState.mutateProviderInstance).toHaveBeenCalledWith({ + operation: "remove", + instanceId: customId, + }); + }); + it("lets the server decide managed ACP cleanup after an atomic delete", async () => { const firstId = ProviderInstanceId.make("acpRegistry_kilo_one"); const secondId = ProviderInstanceId.make("acpRegistry_kilo_two"); @@ -536,9 +590,9 @@ describe("EnvironmentProviderSettings routing", () => { panel, (element) => element.props.instanceId === firstId && element.props.mode === "editor", ); - (card?.props.onDelete as (() => void) | undefined)?.(); - await flushPromises(); + await (card?.props.onDelete as (() => Promise) | undefined)?.(); + expect(commands.confirm).not.toHaveBeenCalled(); expect(settingsState.mutateProviderInstance).toHaveBeenCalledWith({ operation: "remove", instanceId: firstId, diff --git a/apps/web/src/components/settings/ProviderSettingsPanel.tsx b/apps/web/src/components/settings/ProviderSettingsPanel.tsx index 0f0f8a337290..4325680b98a6 100644 --- a/apps/web/src/components/settings/ProviderSettingsPanel.tsx +++ b/apps/web/src/components/settings/ProviderSettingsPanel.tsx @@ -13,6 +13,7 @@ import { type AcpRegistryUrlAuthAction, PROVIDER_DISPLAY_NAMES, ProviderDriverKind, + type OrchestrationV2ThreadShell, type ProviderInstanceConfig, type ProviderInstanceId, resolveEnvironmentMachineKind, @@ -41,6 +42,7 @@ import { } from "../../hooks/useSettings"; import { EnvironmentMachineIcon } from "../EnvironmentMachineIcon"; import { cn } from "../../lib/utils"; +import { ensureLocalApi } from "../../localApi"; import { resolveAppModelSelectionState } from "../../modelSelection"; import { useEnvironments, @@ -49,6 +51,7 @@ import { } from "../../state/environments"; import { EMPTY_SERVER_PROVIDERS, serverEnvironment } from "../../state/server"; import { useEnvironmentSessionState } from "../../state/session"; +import { environmentThreadShells } from "../../state/threads"; import { useProjects } from "../../state/entities"; import { useAtomCommand } from "../../state/use-atom-command"; import { getRelativeTimeState } from "../../timestampFormat"; @@ -584,6 +587,42 @@ function AccessGatedProviderSettings({ ); } +const IN_FLIGHT_ORCHESTRATION_STATUSES: ReadonlySet = new Set([ + "preparing", + "queued", + "starting", + "running", + "waiting", +]); + +/** + * Report whether an orchestration v2 shell still has a turn in flight. + * + * Activity status wins when the shell status has already moved on, which is + * how a wake keeps the original run's work visible. Idle and settled shells + * are not in flight. + */ +function orchestrationV2ThreadInFlight( + thread: Pick, +): boolean { + const status = thread.activityRunStatus ?? thread.status; + return IN_FLIGHT_ORCHESTRATION_STATUSES.has(status); +} + +/** + * Report whether this provider instance still owns an in-flight thread. + */ +function providerInstanceHasInFlightThread( + threads: ReadonlyArray< + Pick + >, + instanceId: ProviderInstanceId, +): boolean { + return threads.some( + (thread) => thread.providerInstanceId === instanceId && orchestrationV2ThreadInFlight(thread), + ); +} + export function EnvironmentProviderSettings({ environmentId, environmentLabel, @@ -604,6 +643,7 @@ export function EnvironmentProviderSettings({ readonly readOnly?: boolean; }) { const settings = useEnvironmentSettings(environmentId); + const threads = useAtomValue(environmentThreadShells.environmentThreadsAtom(environmentId)); // Provider instances hold per-machine credentials and binaries, so this // page always edits exactly the environment it displays. const updateSettings = useUpdateEnvironmentSettings(environmentId); @@ -906,7 +946,22 @@ export function EnvironmentProviderSettings({ } }; + /** + * Remove one provider instance. + * + * Ask with the confirm dialog only when that instance has an in-flight + * orchestration v2 thread. Cancel leaves the turn running. An idle + * instance deletes immediately. + */ const deleteProviderInstance = async (row: InstanceRow) => { + if ( + providerInstanceHasInFlightThread(threads, row.instanceId) && + !(await ensureLocalApi().dialogs.confirm( + `Delete this provider from ${environmentLabel}? This stops its running threads. Thread history is kept.`, + )) + ) { + return; + } const updateResult = await persistProviderInstance({ operation: "remove", instanceId: row.instanceId, diff --git a/packages/contracts/src/settings.test.ts b/packages/contracts/src/settings.test.ts index f4a1432f4319..6f94d056808b 100644 --- a/packages/contracts/src/settings.test.ts +++ b/packages/contracts/src/settings.test.ts @@ -7,6 +7,7 @@ import { ClientSettingsPatch, ClaudeSettings, DEFAULT_SERVER_SETTINGS, + providerInstanceRuntimeConfigEqual, resolveProviderInstanceEnabled, ServerSettings, ServerSettingsPatch, @@ -848,6 +849,54 @@ describe("provider enabled defaults", () => { resolveProviderInstanceEnabled({ driver: codex, enabled: false, config: { enabled: true } }), ).toBe(false); }); + + it("treats a display name, accent color, or lifted enabled flag as the same runtime", () => { + const codex = ProviderDriverKind.make("codex"); + const legacy = { + driver: codex, + config: { enabled: true, binaryPath: "codex", launchArgs: "" }, + }; + expect( + providerInstanceRuntimeConfigEqual(legacy, { + ...legacy, + displayName: "Personal", + accentColor: "#112233", + }), + ).toBe(true); + expect( + providerInstanceRuntimeConfigEqual(legacy, { + driver: codex, + enabled: true, + displayName: "Personal", + config: { binaryPath: "codex", launchArgs: "" }, + }), + ).toBe(true); + expect( + providerInstanceRuntimeConfigEqual( + { driver: codex, enabled: true, config: { enabled: false, binaryPath: "codex" } }, + { driver: codex, enabled: true, config: { enabled: true, binaryPath: "codex" } }, + ), + ).toBe(false); + expect( + providerInstanceRuntimeConfigEqual(legacy, { + driver: codex, + enabled: false, + config: { binaryPath: "codex", launchArgs: "" }, + }), + ).toBe(false); + expect( + providerInstanceRuntimeConfigEqual(legacy, { + ...legacy, + environment: [{ name: "CODEX_HOME", value: "/tmp/codex", sensitive: false }], + }), + ).toBe(false); + expect( + providerInstanceRuntimeConfigEqual(legacy, { + driver: ProviderDriverKind.make("grok"), + config: { enabled: true, binaryPath: "codex", launchArgs: "" }, + }), + ).toBe(false); + }); }); describe("ServerSettings worktree defaults", () => { diff --git a/packages/contracts/src/settings.ts b/packages/contracts/src/settings.ts index 082459efc3df..0a830d667170 100644 --- a/packages/contracts/src/settings.ts +++ b/packages/contracts/src/settings.ts @@ -1,6 +1,7 @@ import { SshDeviceHostConfigs } from "./device.ts"; import * as Effect from "effect/Effect"; import * as Duration from "effect/Duration"; +import * as Equal from "effect/Equal"; import * as Schema from "effect/Schema"; import * as SchemaTransformation from "effect/SchemaTransformation"; import { @@ -1460,6 +1461,50 @@ export const resolveProviderInstanceEnabled = ( return instance.enabled ?? configEnabled ?? defaultEnabledForDriver(instance.driver); }; +/** + * Drop a legacy boolean `enabled` flag from a driver config blob. + * + * The envelope owns that flag. Comparing what remains lets a rename that + * only lifts the flag out of `config` look like the same runtime settings. + */ +const providerInstanceConfigWithoutEnabledFlag = (config: unknown): unknown => { + if (providerInstanceConfigEnabledFlag(config) === undefined) { + return config; + } + const { enabled: _enabled, ...rest } = config as Record; + return rest; +}; + +/** + * Enabled value a reconcile should treat as authoritative: the envelope + * flag when present, otherwise the legacy flag inside `config`. + */ +const providerInstanceCanonicalEnabled = ( + instance: Pick, +): boolean | undefined => instance.enabled ?? providerInstanceConfigEnabledFlag(instance.config); + +/** + * Report whether two provider envelopes would start the same instance. + * + * Display name and accent color are ignored. Lifting a legacy + * `config.enabled` flag onto the envelope is also ignored when the resolved + * enabled value does not change, so the first rename of a default provider + * does not rebuild it. Driver, environment, and any other config change + * still count as a runtime edit. + */ +export const providerInstanceRuntimeConfigEqual = ( + left: ProviderInstanceConfig, + right: ProviderInstanceConfig, +): boolean => + left.driver === right.driver && + providerInstanceCanonicalEnabled(left) === providerInstanceCanonicalEnabled(right) && + resolveProviderInstanceEnabled(left) === resolveProviderInstanceEnabled(right) && + Equal.equals(left.environment, right.environment) && + Equal.equals( + providerInstanceConfigWithoutEnabledFlag(left.config), + providerInstanceConfigWithoutEnabledFlag(right.config), + ); + export const ServerSettingsOperation = Schema.Literals([ "normalize", "check-exists",