History

fix(server): drop stale text generation options when resetting text-gen model selection (#2076)

Utkarsh Patil committed 7 months ago

Commit 7a08fcf1 parent 19d47405 files · +141 −5Browse files

Unified · Split

5 files
  1. apps/server/src/serverSettings.test.ts +29 −0
  2. apps/server/src/serverSettings.ts +18 −2
  3. apps/web/src/hooks/useSettings.ts +2 −2
  4. packages/shared/src/serverSettings.test.ts +59 −0
  5. packages/shared/src/serverSettings.ts +33 −1
apps/server/src/serverSettings.test.ts+29−0
@@ -142,6 +142,35 @@ it.layer(NodeServices.layer)("server settings", (it) => { · 141 unmodified lines
142 }).pipe(Effect.provide(makeServerSettingsLayer())),142 }).pipe(Effect.provide(makeServerSettingsLayer())),
143 );143 );
144 144
145 it.effect("drops stale text generation options when resetting model selection", () =>
146 Effect.gen(function* () {
147 const serverSettings = yield* ServerSettingsService;
148
149 yield* serverSettings.updateSettings({
150 textGenerationModelSelection: {
151 provider: "codex",
152 model: DEFAULT_SERVER_SETTINGS.textGenerationModelSelection.model,
153 options: {
154 reasoningEffort: "high",
155 fastMode: true,
156 },
157 },
158 });
159
160 const next = yield* serverSettings.updateSettings({
161 textGenerationModelSelection: {
162 provider: DEFAULT_SERVER_SETTINGS.textGenerationModelSelection.provider,
163 model: DEFAULT_SERVER_SETTINGS.textGenerationModelSelection.model,
164 },
165 });
166
167 assert.deepEqual(next.textGenerationModelSelection, {
168 provider: DEFAULT_SERVER_SETTINGS.textGenerationModelSelection.provider,
169 model: DEFAULT_SERVER_SETTINGS.textGenerationModelSelection.model,
170 });
171 }).pipe(Effect.provide(makeServerSettingsLayer())),
172 );
173
145 it.effect("trims provider path settings when updates are applied", () =>174 it.effect("trims provider path settings when updates are applied", () =>
146 Effect.gen(function* () {175 Effect.gen(function* () {
147 const serverSettings = yield* ServerSettingsService;176 const serverSettings = yield* ServerSettingsService;
apps/server/src/serverSettings.ts+18−2
@@ -42,6 +42,7 @@ import * as Semaphore from "effect/Semaphore"; · 41 unmodified lines
42import { ServerConfig } from "./config";42import { ServerConfig } from "./config";
43import { type DeepPartial, deepMerge } from "@t3tools/shared/Struct";43import { type DeepPartial, deepMerge } from "@t3tools/shared/Struct";
44import { fromLenientJson } from "@t3tools/shared/schemaJson";44import { fromLenientJson } from "@t3tools/shared/schemaJson";
45import { applyServerSettingsPatch } from "@t3tools/shared/serverSettings";
45 46
46export interface ServerSettingsShape {47export interface ServerSettingsShape {
47 /** Start the settings runtime and attach file watching. */48 /** Start the settings runtime and attach file watching. */
@@ -80,7 +81,20 @@ export class ServerSettingsService extends Context.Service< · 32 unmodified lines
80 getSettings: Ref.get(currentSettingsRef),81 getSettings: Ref.get(currentSettingsRef),
81 updateSettings: (patch) =>82 updateSettings: (patch) =>
82 Ref.get(currentSettingsRef).pipe(83 Ref.get(currentSettingsRef).pipe(
83 Effect.map((currentSettings) => deepMerge(currentSettings, patch)),84 Effect.flatMap((currentSettings) =>
85 Schema.decodeEffect(ServerSettings)(
86 applyServerSettingsPatch(currentSettings, patch),
87 ).pipe(
88 Effect.mapError(
89 (cause) =>
90 new ServerSettingsError({
91 settingsPath: "<memory>",
92 detail: `failed to normalize server settings: ${SchemaIssue.makeFormatterDefault()(cause.issue)}`,
93 cause,
94 }),
95 ),
96 ),
97 ),
84 Effect.tap((nextSettings) => Ref.set(currentSettingsRef, nextSettings)),98 Effect.tap((nextSettings) => Ref.set(currentSettingsRef, nextSettings)),
85 ),99 ),
86 streamChanges: Stream.empty,100 streamChanges: Stream.empty,
@@ -314,7 +328,9 @@ const makeServerSettings = Effect.gen(function* () { · 227 unmodified lines
314 writeSemaphore.withPermits(1)(328 writeSemaphore.withPermits(1)(
315 Effect.gen(function* () {329 Effect.gen(function* () {
316 const current = yield* getSettingsFromCache;330 const current = yield* getSettingsFromCache;
317 const next = yield* Schema.decodeEffect(ServerSettings)(deepMerge(current, patch)).pipe(331 const next = yield* Schema.decodeEffect(ServerSettings)(
332 applyServerSettingsPatch(current, patch),
333 ).pipe(
318 Effect.mapError(334 Effect.mapError(
319 (cause) =>335 (cause) =>
320 new ServerSettingsError({336 new ServerSettingsError({
apps/web/src/hooks/useSettings.ts+2−2
@@ -19,7 +19,7 @@ import { · 18 unmodified lines
19} from "@t3tools/contracts/settings";19} from "@t3tools/contracts/settings";
20import { ensureLocalApi } from "~/localApi";20import { ensureLocalApi } from "~/localApi";
21import { Struct } from "effect";21import { Struct } from "effect";
22import { deepMerge } from "@t3tools/shared/Struct";22import { applyServerSettingsPatch } from "@t3tools/shared/serverSettings";
23import { applySettingsUpdated, getServerConfig, useServerSettings } from "~/rpc/serverState";23import { applySettingsUpdated, getServerConfig, useServerSettings } from "~/rpc/serverState";
24 24
25const CLIENT_SETTINGS_PERSISTENCE_ERROR_SCOPE = "[CLIENT_SETTINGS]";25const CLIENT_SETTINGS_PERSISTENCE_ERROR_SCOPE = "[CLIENT_SETTINGS]";
@@ -154,7 +154,7 @@ export function useUpdateSettings() { · 128 unmodified lines
154 if (Object.keys(serverPatch).length > 0) {154 if (Object.keys(serverPatch).length > 0) {
155 const currentServerConfig = getServerConfig();155 const currentServerConfig = getServerConfig();
156 if (currentServerConfig) {156 if (currentServerConfig) {
157 applySettingsUpdated(deepMerge(currentServerConfig.settings, serverPatch));157 applySettingsUpdated(applyServerSettingsPatch(currentServerConfig.settings, serverPatch));
158 }158 }
159 // Fire-and-forget RPC — push will reconcile on success159 // Fire-and-forget RPC — push will reconcile on success
160 void ensureLocalApi().server.updateSettings(serverPatch);160 void ensureLocalApi().server.updateSettings(serverPatch);
packages/shared/src/serverSettings.test.ts+59−0
@@ -1,5 +1,7 @@
1import { DEFAULT_SERVER_SETTINGS } from "@t3tools/contracts";
1import { describe, expect, it } from "vitest";2import { describe, expect, it } from "vitest";
2import {3import {
4 applyServerSettingsPatch,
3 extractPersistedServerObservabilitySettings,5 extractPersistedServerObservabilitySettings,
4 normalizePersistedServerSettingString,6 normalizePersistedServerSettingString,
5 parsePersistedServerObservabilitySettings,7 parsePersistedServerObservabilitySettings,
@@ -50,4 +52,61 @@ describe("serverSettings helpers", () => { · 44 unmodified lines
50 otlpMetricsUrl: undefined,52 otlpMetricsUrl: undefined,
51 });53 });
52 });54 });
55
56 it("replaces text generation selection when provider/model are provided", () => {
57 const current = {
58 ...DEFAULT_SERVER_SETTINGS,
59 textGenerationModelSelection: {
60 provider: "codex" as const,
61 model: "gpt-5.4-mini",
62 options: {
63 reasoningEffort: "high" as const,
64 fastMode: true,
65 },
66 },
67 };
68
69 expect(
70 applyServerSettingsPatch(current, {
71 textGenerationModelSelection: {
72 provider: "codex",
73 model: "gpt-5.4-mini",
74 },
75 }).textGenerationModelSelection,
76 ).toEqual({
77 provider: "codex",
78 model: "gpt-5.4-mini",
79 });
80 });
81
82 it("still deep merges text generation selection when only options are provided", () => {
83 const current = {
84 ...DEFAULT_SERVER_SETTINGS,
85 textGenerationModelSelection: {
86 provider: "codex" as const,
87 model: "gpt-5.4-mini",
88 options: {
89 reasoningEffort: "high" as const,
90 fastMode: true,
91 },
92 },
93 };
94
95 expect(
96 applyServerSettingsPatch(current, {
97 textGenerationModelSelection: {
98 options: {
99 fastMode: false,
100 },
101 },
102 }).textGenerationModelSelection,
103 ).toEqual({
104 provider: "codex",
105 model: "gpt-5.4-mini",
106 options: {
107 reasoningEffort: "high",
108 fastMode: false,
109 },
110 });
111 });
53});112});
packages/shared/src/serverSettings.ts+33−1
@@ -1,5 +1,6 @@
1import { ServerSettings } from "@t3tools/contracts";1import { ServerSettings, type ServerSettingsPatch } from "@t3tools/contracts";
2import { Schema } from "effect";2import { Schema } from "effect";
3import { deepMerge } from "./Struct";
3import { fromLenientJson } from "./schemaJson";4import { fromLenientJson } from "./schemaJson";
4 5
5const ServerSettingsJson = fromLenientJson(ServerSettings);6const ServerSettingsJson = fromLenientJson(ServerSettings);
@@ -38,3 +39,34 @@ export function parsePersistedServerObservabilitySettings( · 32 unmodified lines
38 return { otlpTracesUrl: undefined, otlpMetricsUrl: undefined };39 return { otlpTracesUrl: undefined, otlpMetricsUrl: undefined };
39 }40 }
40}41}
42
43function shouldReplaceTextGenerationModelSelection(
44 patch: ServerSettingsPatch["textGenerationModelSelection"] | undefined,
45): boolean {
46 return Boolean(patch && (patch.provider !== undefined || patch.model !== undefined));
47}
48
49/**
50 * Applies a server settings patch while treating textGenerationModelSelection as
51 * replace-on-provider/model updates. This prevents stale nested options from
52 * surviving a reset patch that intentionally omits options.
53 */
54export function applyServerSettingsPatch(
55 current: ServerSettings,
56 patch: ServerSettingsPatch,
57): ServerSettings {
58 const selectionPatch = patch.textGenerationModelSelection;
59 const next = deepMerge(current, patch);
60 if (!selectionPatch || !shouldReplaceTextGenerationModelSelection(selectionPatch)) {
61 return next;
62 }
63
64 return {
65 ...next,
66 textGenerationModelSelection: {
67 provider: selectionPatch.provider ?? current.textGenerationModelSelection.provider,
68 model: selectionPatch.model ?? current.textGenerationModelSelection.model,
69 ...(selectionPatch.options ? { options: selectionPatch.options } : {}),
70 },
71 };
72}