authorgravatar for 97427849+flamboh@users.noreply.github.comoliver <97427849+flamboh@users.noreply.github.com> 2026-10-13 10:43:56-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-13 12:43:56-05:00
log8ac4b97dae6bff7258fa5091a908116902fbd635
treecbd33a3162532043054bc76bf0789d4712ed37d4
parent4292720fd927f4c81c21da4e596bdfcedbde32ce
signature Signed by PGP key B5690EEEBB952194

fix(web): settings warn when an environment silently drops a setting (#18334)

2 files changed, 191 insertions(+), 30 deletions(-)

apps/web/src/components/settings/scopedSettings.test.ts+93-1
...@@ -230,6 +230,89 @@ describe("scoped settings writes", () => {...@@ -230,6 +230,89 @@ describe("scoped settings writes", () => {
230 expect(persistClient).not.toHaveBeenCalled();230 expect(persistClient).not.toHaveBeenCalled();
231 });231 });
232232
233 describe("an environment that saves but drops keys it does not know", () => {
234 // Laptop knows every key; Server is an older build that keeps its settings as they were.
235 const upgradedLaptop = (saved: Partial<ServerSettings>) =>
236 vi.fn(async ({ environmentId }: { environmentId: EnvironmentId }) => ({
237 _tag: "Success" as const,
238 value: {
239 ...DEFAULT_SERVER_SETTINGS,
240 ...(environmentId === laptop.environmentId ? saved : {}),
241 },
242 }));
243 const outcome = async (
244 plan: ReturnType<typeof planScopedSettingsPatch>,
245 persistServer: ReturnType<typeof upgradedLaptop>,
246 ) => {
247 const result = await persistScopedSettingsPatch(plan, persistServer, vi.fn());
248 return {
249 saved: result.savedEnvironments.map(({ label }) => label),
250 failed: result.failedEnvironments.map(({ label, message }) => ({ label, message })),
251 };
252 };
253 const unsupported = [
254 {
255 label: server.label,
256 message: "This environment does not support this setting. Update T3 Code on it.",
257 },
258 ];
259
260 it("reports a dropped toggle", async () => {
261 expect(
262 await outcome(
263 planScopedSettingsPatch(all, environments, { reviewGuidesEnabled: true }),
264 upgradedLaptop({ reviewGuidesEnabled: true }),
265 ),
266 ).toEqual({ saved: [laptop.label], failed: unsupported });
267 });
268
269 it("reports dropped text but accepts the server trimming it", async () => {
270 expect(
271 await outcome(
272 planScopedSettingsPatch(all, environments, { reviewGuideInstructions: " Read tests " }),
273 upgradedLaptop({ reviewGuideInstructions: "Read tests" }),
274 ),
275 ).toEqual({ saved: [laptop.label], failed: unsupported });
276 });
277
278 it("reports a dropped key inside a project override", async () => {
279 expect(
280 await outcome(
281 planScopedSettingsPatch(project, environments, { defaultAutoPull: true }),
282 upgradedLaptop({
283 projectSettingsOverrides: { [laptopProjectId]: { defaultAutoPull: true } },
284 }),
285 ),
286 ).toEqual({ saved: [laptop.label], failed: unsupported });
287 });
288
289 it("reports a nested key dropped beside one that saved", async () => {
290 const patch = { defaultAutoPull: true, enableAgentBrowserAccess: false };
291 const persistServer = vi.fn(async ({ environmentId }: { environmentId: EnvironmentId }) => ({
292 _tag: "Success" as const,
293 value: {
294 ...DEFAULT_SERVER_SETTINGS,
295 projectSettingsOverrides:
296 environmentId === laptop.environmentId
297 ? { [laptopProjectId]: patch }
298 : { [projectId]: { defaultAutoPull: true } },
299 },
300 }));
301 expect(
302 await outcome(planScopedSettingsPatch(project, environments, patch), persistServer),
303 ).toEqual({ saved: [laptop.label], failed: unsupported });
304 });
305
306 it("accepts a write that matches what the environment already had", async () => {
307 expect(
308 await outcome(
309 planScopedSettingsPatch(all, environments, { reviewGuidesEnabled: false }),
310 upgradedLaptop({}),
311 ),
312 ).toEqual({ saved: [laptop.label, server.label], failed: [] });
313 });
314 });
315
233 it("persists client keys locally at any scope alongside server keys", async () => {316 it("persists client keys locally at any scope alongside server keys", async () => {
234 const persistServer = vi.fn().mockResolvedValue({ _tag: "Success" });317 const persistServer = vi.fn().mockResolvedValue({ _tag: "Success" });
235 const persistClient = vi.fn();318 const persistClient = vi.fn();
...@@ -262,6 +345,7 @@ describe("scoped settings writes", () => {...@@ -262,6 +345,7 @@ describe("scoped settings writes", () => {
262 {345 {
263 environmentId: server.environmentId,346 environmentId: server.environmentId,
264 label: server.label,347 label: server.label,
348 previous: expect.anything(),
265 patch: {349 patch: {
266 projectSettingsOverrides: {350 projectSettingsOverrides: {
267 [projectId]: { enableAgentBrowserAccess: false, defaultAutoPull: true },351 [projectId]: { enableAgentBrowserAccess: false, defaultAutoPull: true },
...@@ -271,6 +355,7 @@ describe("scoped settings writes", () => {...@@ -271,6 +355,7 @@ describe("scoped settings writes", () => {
271 {355 {
272 environmentId: laptop.environmentId,356 environmentId: laptop.environmentId,
273 label: laptop.label,357 label: laptop.label,
358 previous: expect.anything(),
274 patch: { projectSettingsOverrides: { [laptopProjectId]: { defaultAutoPull: true } } },359 patch: { projectSettingsOverrides: { [laptopProjectId]: { defaultAutoPull: true } } },
275 },360 },
276 ]);361 ]);
...@@ -391,6 +476,7 @@ describe("scoped settings writes", () => {...@@ -391,6 +476,7 @@ describe("scoped settings writes", () => {
391 {476 {
392 environmentId: server.environmentId,477 environmentId: server.environmentId,
393 label: server.label,478 label: server.label,
479 previous: expect.anything(),
394 patch: { projectSettingsOverrides: { [projectId]: { enableAgentBrowserAccess: false } } },480 patch: { projectSettingsOverrides: { [projectId]: { enableAgentBrowserAccess: false } } },
395 },481 },
396 ]);482 ]);
...@@ -527,6 +613,7 @@ describe("project overrides at environment scope", () => {...@@ -527,6 +613,7 @@ describe("project overrides at environment scope", () => {
527 {613 {
528 environmentId: laptop,614 environmentId: laptop,
529 label: laptop,615 label: laptop,
616 previous: expect.anything(),
530 patch: {617 patch: {
531 projectSettingsOverrides: { [fleet]: { defaultThreadEnvMode: "local" }, [t3]: null },618 projectSettingsOverrides: { [fleet]: { defaultThreadEnvMode: "local" }, [t3]: null },
532 },619 },
...@@ -655,6 +742,11 @@ it("routes legacy model preference keys exclusively to the selected environment"...@@ -655,6 +742,11 @@ it("routes legacy model preference keys exclusively to the selected environment"
655 expect(plan.clientPatch).toEqual({});742 expect(plan.clientPatch).toEqual({});
656 expect(plan.hasClientWrite).toBe(false);743 expect(plan.hasClientWrite).toBe(false);
657 expect(plan.serverWrites).toEqual([744 expect(plan.serverWrites).toEqual([
658 { environmentId: server.environmentId, label: server.label, patch },745 {
746 environmentId: server.environmentId,
747 label: server.label,
748 previous: expect.anything(),
749 patch,
750 },
659 ]);751 ]);
660});752});
apps/web/src/components/settings/scopedSettings.ts+98-29
...@@ -166,6 +166,8 @@ interface ScopedServerWrite {...@@ -166,6 +166,8 @@ interface ScopedServerWrite {
166 readonly environmentId: EnvironmentId;166 readonly environmentId: EnvironmentId;
167 readonly label: string;167 readonly label: string;
168 readonly patch: ServerSettingsPatch;168 readonly patch: ServerSettingsPatch;
169 /** The environment's settings before the write, to tell a dropped key from a saved one. */
170 readonly previous: ServerSettings;
169}171}
170172
171function projectOverrideWrites(173function projectOverrideWrites(
...@@ -194,6 +196,7 @@ function projectOverrideWrites(...@@ -194,6 +196,7 @@ function projectOverrideWrites(
194 writes.set(member.environmentId, {196 writes.set(member.environmentId, {
195 environmentId: member.environmentId,197 environmentId: member.environmentId,
196 label: environment.label,198 label: environment.label,
199 previous: settings,
197 patch: {200 patch: {
198 projectSettingsOverrides: {201 projectSettingsOverrides: {
199 ...existing?.patch.projectSettingsOverrides,202 ...existing?.patch.projectSettingsOverrides,
...@@ -266,29 +269,38 @@ export function planScopedSettingsPatch(...@@ -266,29 +269,38 @@ export function planScopedSettingsPatch(
266 return next as ProjectSettingsOverrides;269 return next as ProjectSettingsOverrides;
267 })270 })
268 : scope.kind === "all" || scope.kind === "environment"271 : scope.kind === "all" || scope.kind === "environment"
269 ? connectedEnvironments.map((environment) => ({272 ? connectedEnvironments.flatMap((environment) => {
270 environmentId: environment.environmentId,273 const previous = environment.serverConfig?.settings;
271 label: environment.label,274 if (!previous) return [];
272 patch:275 return [
273 environment.serverConfig?.settings.worktreeCleanup != null &&276 {
274 serverPatch.storageCleanup &&277 environmentId: environment.environmentId,
275 Object.keys(serverPatch.storageCleanup).some((key) => key.startsWith("worktree"))278 label: environment.label,
276 ? {279 previous,
277 ...serverPatch,280 patch:
278 worktreeCleanup: {281 previous.worktreeCleanup != null &&
279 mode: "custom",282 serverPatch.storageCleanup &&
280 rules: {283 Object.keys(serverPatch.storageCleanup).some((key) =>
281 ...resolveWorktreeCleanup(environment.serverConfig.settings, null),284 key.startsWith("worktree"),
282 ...Object.fromEntries(285 )
283 Object.entries(serverPatch.storageCleanup).filter(([key]) =>286 ? {
284 key.startsWith("worktree"),287 ...serverPatch,
285 ),288 worktreeCleanup: {
286 ),289 mode: "custom",
287 },290 rules: {
288 },291 ...resolveWorktreeCleanup(previous, null),
289 }292 ...Object.fromEntries(
290 : serverPatch,293 Object.entries(serverPatch.storageCleanup).filter(([key]) =>
291 }))294 key.startsWith("worktree"),
295 ),
296 ),
297 },
298 },
299 }
300 : serverPatch,
301 },
302 ];
303 })
292 : [];304 : [];
293 const hasClientWrite = Object.keys(clientPatch).length > 0;305 const hasClientWrite = Object.keys(clientPatch).length > 0;
294 const hasWrite = hasClientWrite || serverWrites.length > 0;306 const hasWrite = hasClientWrite || serverWrites.length > 0;
...@@ -371,6 +383,7 @@ export function planProjectOverridesClear(...@@ -371,6 +383,7 @@ export function planProjectOverridesClear(
371 writes.set(environmentId, {383 writes.set(environmentId, {
372 environmentId,384 environmentId,
373 label: environment.label,385 label: environment.label,
386 previous: settings,
374 patch: {387 patch: {
375 projectSettingsOverrides: {388 projectSettingsOverrides: {
376 ...existing?.patch.projectSettingsOverrides,389 ...existing?.patch.projectSettingsOverrides,
...@@ -389,13 +402,56 @@ export function planProjectOverridesClear(...@@ -389,13 +402,56 @@ export function planProjectOverridesClear(
389 };402 };
390}403}
391404
405/**
406 * Whether a server kept `before` where `value` asked for a change. Objects are
407 * compared field by field, so a nested key an older server dropped is caught
408 * even when a sibling field saved. A null clears the field; strings may come
409 * back trimmed.
410 */
411function writeWasDropped(value: unknown, before: unknown, saved: unknown): boolean {
412 if (value === null) return before != null && Equal.equals(saved, before);
413 if (isPlainObject(value) && !Array.isArray(before) && !Array.isArray(saved)) {
414 const field = (source: unknown, key: string) =>
415 isPlainObject(source) ? source[key] : undefined;
416 return Object.entries(value).some(([key, next]) =>
417 writeWasDropped(next, field(before, key), field(saved, key)),
418 );
419 }
420 const unchanged =
421 typeof value === "string" && typeof before === "string"
422 ? value.trim() === before
423 : Equal.equals(value, before);
424 return !unchanged && Equal.equals(saved, before);
425}
426
427/**
428 * An older server drops keys it does not know, top-level or nested, and still
429 * succeeds. Left unreported, the row stays mixed forever.
430 */
431function droppedSettingKeys(write: ScopedServerWrite, saved: ServerSettings | undefined) {
432 if (saved === undefined) return [];
433 return Object.entries(write.patch).flatMap(([key, value]) =>
434 writeWasDropped(
435 value,
436 write.previous[key as keyof ServerSettings],
437 saved[key as keyof ServerSettings],
438 )
439 ? [key]
440 : [],
441 );
442}
443
392/** Wait for every target so a failed environment does not hide successful or later writes. */444/** Wait for every target so a failed environment does not hide successful or later writes. */
393export async function persistScopedSettingsPatch(445export async function persistScopedSettingsPatch(
394 plan: ReturnType<typeof planScopedSettingsPatch>,446 plan: ReturnType<typeof planScopedSettingsPatch>,
395 persistServer: (input: {447 persistServer: (input: {
396 environmentId: EnvironmentId;448 environmentId: EnvironmentId;
397 input: { patch: ServerSettingsPatch };449 input: { patch: ServerSettingsPatch };
398 }) => Promise<{ readonly _tag: "Success" | "Failure"; readonly cause?: Cause.Cause<unknown> }>,450 }) => Promise<{
451 readonly _tag: "Success" | "Failure";
452 readonly cause?: Cause.Cause<unknown>;
453 readonly value?: ServerSettings;
454 }>,
399 persistClient: (patch: ClientSettingsPatch) => void,455 persistClient: (patch: ClientSettingsPatch) => void,
400) {456) {
401 if (plan.hasClientWrite) persistClient(plan.clientPatch);457 if (plan.hasClientWrite) persistClient(plan.clientPatch);
...@@ -404,9 +460,25 @@ export async function persistScopedSettingsPatch(...@@ -404,9 +460,25 @@ export async function persistScopedSettingsPatch(
404 persistServer({ environmentId, input: { patch } }),460 persistServer({ environmentId, input: { patch } }),
405 ),461 ),
406 );462 );
463 const saved = (index: number) => {
464 const result = results[index];
465 return (
466 result?.status === "fulfilled" &&
467 result.value._tag === "Success" &&
468 droppedSettingKeys(plan.serverWrites[index]!, result.value.value).length === 0
469 );
470 };
407 const failedEnvironments = plan.serverWrites.flatMap((environment, index) => {471 const failedEnvironments = plan.serverWrites.flatMap((environment, index) => {
472 if (saved(index)) return [];
408 const result = results[index];473 const result = results[index];
409 if (result?.status === "fulfilled" && result.value._tag === "Success") return [];474 if (result?.status === "fulfilled" && result.value._tag === "Success") {
475 return [
476 {
477 ...environment,
478 message: "This environment does not support this setting. Update T3 Code on it.",
479 },
480 ];
481 }
410 const error: unknown =482 const error: unknown =
411 result?.status === "rejected"483 result?.status === "rejected"
412 ? result.reason484 ? result.reason
...@@ -423,10 +495,7 @@ export async function persistScopedSettingsPatch(...@@ -423,10 +495,7 @@ export async function persistScopedSettingsPatch(
423 },495 },
424 ];496 ];
425 });497 });
426 const savedEnvironments = plan.serverWrites.filter((_, index) => {498 const savedEnvironments = plan.serverWrites.filter((_, index) => saved(index));
427 const result = results[index];
428 return result?.status === "fulfilled" && result.value._tag === "Success";
429 });
430 return {499 return {
431 failedEnvironments,500 failedEnvironments,
432 savedEnvironments,501 savedEnvironments,