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",