From 8ac4b97dae6bff7258fa5091a908116902fbd635 Mon Sep 17 00:00:00 2001 From: oliver <97427849+flamboh@users.noreply.github.com> Date: Tue, 13 Oct 2026 10:43:56 -0700 Subject: [PATCH] fix(web): settings warn when an environment silently drops a setting (#18334) --- .../settings/scopedSettings.test.ts | 94 ++++++++++++- .../src/components/settings/scopedSettings.ts | 127 ++++++++++++++---- 2 files changed, 191 insertions(+), 30 deletions(-) diff --git a/apps/web/src/components/settings/scopedSettings.test.ts b/apps/web/src/components/settings/scopedSettings.test.ts index e8e5d0a1243ee1e9469dcaa72b153bda2acf7382..dc5b7d74fd9bb80febcd4ac72b99e9475bc30a66 100644 --- a/apps/web/src/components/settings/scopedSettings.test.ts +++ b/apps/web/src/components/settings/scopedSettings.test.ts @@ -230,6 +230,89 @@ describe("scoped settings writes", () => { expect(persistClient).not.toHaveBeenCalled(); }); + describe("an environment that saves but drops keys it does not know", () => { + // Laptop knows every key; Server is an older build that keeps its settings as they were. + const upgradedLaptop = (saved: Partial) => + vi.fn(async ({ environmentId }: { environmentId: EnvironmentId }) => ({ + _tag: "Success" as const, + value: { + ...DEFAULT_SERVER_SETTINGS, + ...(environmentId === laptop.environmentId ? saved : {}), + }, + })); + const outcome = async ( + plan: ReturnType, + persistServer: ReturnType, + ) => { + const result = await persistScopedSettingsPatch(plan, persistServer, vi.fn()); + return { + saved: result.savedEnvironments.map(({ label }) => label), + failed: result.failedEnvironments.map(({ label, message }) => ({ label, message })), + }; + }; + const unsupported = [ + { + label: server.label, + message: "This environment does not support this setting. Update T3 Code on it.", + }, + ]; + + it("reports a dropped toggle", async () => { + expect( + await outcome( + planScopedSettingsPatch(all, environments, { reviewGuidesEnabled: true }), + upgradedLaptop({ reviewGuidesEnabled: true }), + ), + ).toEqual({ saved: [laptop.label], failed: unsupported }); + }); + + it("reports dropped text but accepts the server trimming it", async () => { + expect( + await outcome( + planScopedSettingsPatch(all, environments, { reviewGuideInstructions: " Read tests " }), + upgradedLaptop({ reviewGuideInstructions: "Read tests" }), + ), + ).toEqual({ saved: [laptop.label], failed: unsupported }); + }); + + it("reports a dropped key inside a project override", async () => { + expect( + await outcome( + planScopedSettingsPatch(project, environments, { defaultAutoPull: true }), + upgradedLaptop({ + projectSettingsOverrides: { [laptopProjectId]: { defaultAutoPull: true } }, + }), + ), + ).toEqual({ saved: [laptop.label], failed: unsupported }); + }); + + it("reports a nested key dropped beside one that saved", async () => { + const patch = { defaultAutoPull: true, enableAgentBrowserAccess: false }; + const persistServer = vi.fn(async ({ environmentId }: { environmentId: EnvironmentId }) => ({ + _tag: "Success" as const, + value: { + ...DEFAULT_SERVER_SETTINGS, + projectSettingsOverrides: + environmentId === laptop.environmentId + ? { [laptopProjectId]: patch } + : { [projectId]: { defaultAutoPull: true } }, + }, + })); + expect( + await outcome(planScopedSettingsPatch(project, environments, patch), persistServer), + ).toEqual({ saved: [laptop.label], failed: unsupported }); + }); + + it("accepts a write that matches what the environment already had", async () => { + expect( + await outcome( + planScopedSettingsPatch(all, environments, { reviewGuidesEnabled: false }), + upgradedLaptop({}), + ), + ).toEqual({ saved: [laptop.label, server.label], failed: [] }); + }); + }); + it("persists client keys locally at any scope alongside server keys", async () => { const persistServer = vi.fn().mockResolvedValue({ _tag: "Success" }); const persistClient = vi.fn(); @@ -262,6 +345,7 @@ describe("scoped settings writes", () => { { environmentId: server.environmentId, label: server.label, + previous: expect.anything(), patch: { projectSettingsOverrides: { [projectId]: { enableAgentBrowserAccess: false, defaultAutoPull: true }, @@ -271,6 +355,7 @@ describe("scoped settings writes", () => { { environmentId: laptop.environmentId, label: laptop.label, + previous: expect.anything(), patch: { projectSettingsOverrides: { [laptopProjectId]: { defaultAutoPull: true } } }, }, ]); @@ -391,6 +476,7 @@ describe("scoped settings writes", () => { { environmentId: server.environmentId, label: server.label, + previous: expect.anything(), patch: { projectSettingsOverrides: { [projectId]: { enableAgentBrowserAccess: false } } }, }, ]); @@ -527,6 +613,7 @@ describe("project overrides at environment scope", () => { { environmentId: laptop, label: laptop, + previous: expect.anything(), patch: { projectSettingsOverrides: { [fleet]: { defaultThreadEnvMode: "local" }, [t3]: null }, }, @@ -655,6 +742,11 @@ it("routes legacy model preference keys exclusively to the selected environment" expect(plan.clientPatch).toEqual({}); expect(plan.hasClientWrite).toBe(false); expect(plan.serverWrites).toEqual([ - { environmentId: server.environmentId, label: server.label, patch }, + { + environmentId: server.environmentId, + label: server.label, + previous: expect.anything(), + patch, + }, ]); }); diff --git a/apps/web/src/components/settings/scopedSettings.ts b/apps/web/src/components/settings/scopedSettings.ts index f4fe6e41905cba44ed7dfa759cc4857d580e0c48..dcedf419caf261d1c3e887faa49802afcdc52f3d 100644 --- a/apps/web/src/components/settings/scopedSettings.ts +++ b/apps/web/src/components/settings/scopedSettings.ts @@ -166,6 +166,8 @@ interface ScopedServerWrite { readonly environmentId: EnvironmentId; readonly label: string; readonly patch: ServerSettingsPatch; + /** The environment's settings before the write, to tell a dropped key from a saved one. */ + readonly previous: ServerSettings; } function projectOverrideWrites( @@ -194,6 +196,7 @@ function projectOverrideWrites( writes.set(member.environmentId, { environmentId: member.environmentId, label: environment.label, + previous: settings, patch: { projectSettingsOverrides: { ...existing?.patch.projectSettingsOverrides, @@ -266,29 +269,38 @@ export function planScopedSettingsPatch( return next as ProjectSettingsOverrides; }) : scope.kind === "all" || scope.kind === "environment" - ? connectedEnvironments.map((environment) => ({ - environmentId: environment.environmentId, - label: environment.label, - patch: - environment.serverConfig?.settings.worktreeCleanup != null && - serverPatch.storageCleanup && - Object.keys(serverPatch.storageCleanup).some((key) => key.startsWith("worktree")) - ? { - ...serverPatch, - worktreeCleanup: { - mode: "custom", - rules: { - ...resolveWorktreeCleanup(environment.serverConfig.settings, null), - ...Object.fromEntries( - Object.entries(serverPatch.storageCleanup).filter(([key]) => - key.startsWith("worktree"), - ), - ), - }, - }, - } - : serverPatch, - })) + ? connectedEnvironments.flatMap((environment) => { + const previous = environment.serverConfig?.settings; + if (!previous) return []; + return [ + { + environmentId: environment.environmentId, + label: environment.label, + previous, + patch: + previous.worktreeCleanup != null && + serverPatch.storageCleanup && + Object.keys(serverPatch.storageCleanup).some((key) => + key.startsWith("worktree"), + ) + ? { + ...serverPatch, + worktreeCleanup: { + mode: "custom", + rules: { + ...resolveWorktreeCleanup(previous, null), + ...Object.fromEntries( + Object.entries(serverPatch.storageCleanup).filter(([key]) => + key.startsWith("worktree"), + ), + ), + }, + }, + } + : serverPatch, + }, + ]; + }) : []; const hasClientWrite = Object.keys(clientPatch).length > 0; const hasWrite = hasClientWrite || serverWrites.length > 0; @@ -371,6 +383,7 @@ export function planProjectOverridesClear( writes.set(environmentId, { environmentId, label: environment.label, + previous: settings, patch: { projectSettingsOverrides: { ...existing?.patch.projectSettingsOverrides, @@ -389,13 +402,56 @@ export function planProjectOverridesClear( }; } +/** + * Whether a server kept `before` where `value` asked for a change. Objects are + * compared field by field, so a nested key an older server dropped is caught + * even when a sibling field saved. A null clears the field; strings may come + * back trimmed. + */ +function writeWasDropped(value: unknown, before: unknown, saved: unknown): boolean { + if (value === null) return before != null && Equal.equals(saved, before); + if (isPlainObject(value) && !Array.isArray(before) && !Array.isArray(saved)) { + const field = (source: unknown, key: string) => + isPlainObject(source) ? source[key] : undefined; + return Object.entries(value).some(([key, next]) => + writeWasDropped(next, field(before, key), field(saved, key)), + ); + } + const unchanged = + typeof value === "string" && typeof before === "string" + ? value.trim() === before + : Equal.equals(value, before); + return !unchanged && Equal.equals(saved, before); +} + +/** + * An older server drops keys it does not know, top-level or nested, and still + * succeeds. Left unreported, the row stays mixed forever. + */ +function droppedSettingKeys(write: ScopedServerWrite, saved: ServerSettings | undefined) { + if (saved === undefined) return []; + return Object.entries(write.patch).flatMap(([key, value]) => + writeWasDropped( + value, + write.previous[key as keyof ServerSettings], + saved[key as keyof ServerSettings], + ) + ? [key] + : [], + ); +} + /** Wait for every target so a failed environment does not hide successful or later writes. */ export async function persistScopedSettingsPatch( plan: ReturnType, persistServer: (input: { environmentId: EnvironmentId; input: { patch: ServerSettingsPatch }; - }) => Promise<{ readonly _tag: "Success" | "Failure"; readonly cause?: Cause.Cause }>, + }) => Promise<{ + readonly _tag: "Success" | "Failure"; + readonly cause?: Cause.Cause; + readonly value?: ServerSettings; + }>, persistClient: (patch: ClientSettingsPatch) => void, ) { if (plan.hasClientWrite) persistClient(plan.clientPatch); @@ -404,9 +460,25 @@ export async function persistScopedSettingsPatch( persistServer({ environmentId, input: { patch } }), ), ); + const saved = (index: number) => { + const result = results[index]; + return ( + result?.status === "fulfilled" && + result.value._tag === "Success" && + droppedSettingKeys(plan.serverWrites[index]!, result.value.value).length === 0 + ); + }; const failedEnvironments = plan.serverWrites.flatMap((environment, index) => { + if (saved(index)) return []; const result = results[index]; - if (result?.status === "fulfilled" && result.value._tag === "Success") return []; + if (result?.status === "fulfilled" && result.value._tag === "Success") { + return [ + { + ...environment, + message: "This environment does not support this setting. Update T3 Code on it.", + }, + ]; + } const error: unknown = result?.status === "rejected" ? result.reason @@ -423,10 +495,7 @@ export async function persistScopedSettingsPatch( }, ]; }); - const savedEnvironments = plan.serverWrites.filter((_, index) => { - const result = results[index]; - return result?.status === "fulfilled" && result.value._tag === "Success"; - }); + const savedEnvironments = plan.serverWrites.filter((_, index) => saved(index)); return { failedEnvironments, savedEnvironments, -- 2.54.0