From d877ae26c804aed1ab86d6aa5ba7ed73d616da36 Mon Sep 17 00:00:00 2001 From: Shakker Date: Sun, 2 Aug 2026 21:56:42 +0100 Subject: [PATCH] fix: fence global model picker ownership --- ui/src/lib/sessions/list-options.test.ts | 46 ++++++++++++++++- ui/src/lib/sessions/patch.ts | 2 + ui/src/lib/sessions/session-mutations.ts | 11 ++-- ui/src/pages/chat/chat-session.ts | 11 ++-- ui/src/pages/chat/chat-settings-patches.ts | 3 ++ ui/src/pages/chat/chat-view.test.ts | 60 ++++++++++++++++++++++ 6 files changed, 125 insertions(+), 8 deletions(-) diff --git a/ui/src/lib/sessions/list-options.test.ts b/ui/src/lib/sessions/list-options.test.ts index 5ba807dc535c..4a50f3f610e7 100644 --- a/ui/src/lib/sessions/list-options.test.ts +++ b/ui/src/lib/sessions/list-options.test.ts @@ -15,10 +15,12 @@ function sessionsResult(sessions: SessionsListResult["sessions"], ts: number): S function deferred() { let resolve: (value: T) => void = () => undefined; - const promise = new Promise((next) => { + let reject: (error: unknown) => void = () => undefined; + const promise = new Promise((next, fail) => { resolve = next; + reject = fail; }); - return { promise, resolve }; + return { promise, reject, resolve }; } function createSessions(client: GatewayBrowserClient, key: string) { @@ -328,4 +330,44 @@ describe("session list replacement options", () => { expect(sessions.state.modelOverrides[key]).toBe("openai/gpt-old"); sessions.dispose(); }); + + it.each(["resolve", "reject"] as const)( + "does not %s an optimistic model patch into a replacement UI owner", + async (outcome) => { + const pendingPatch = deferred(); + const request = vi.fn(async (method: string) => { + if (method === "sessions.patch") { + return await pendingPatch.promise; + } + throw new Error(`Unexpected request: ${method}`); + }); + const key = "global"; + const sessions = createSessions({ request } as unknown as GatewayBrowserClient, key); + let ownsModelOverride = true; + sessions.setModelOverride(key, "openai/gpt-old"); + + const operation = sessions.patch( + key, + { model: "openai/gpt-agent-a" }, + { + deferListRefresh: true, + ownsModelOverride: () => ownsModelOverride, + }, + ); + expect(sessions.state.modelOverrides[key]).toBe("openai/gpt-agent-a"); + + ownsModelOverride = false; + sessions.setModelOverride(key, "openai/gpt-agent-b"); + if (outcome === "resolve") { + pendingPatch.resolve({ ok: true, path: "", key, entry: {} }); + await expect(operation).resolves.toMatchObject({ ok: true, key }); + } else { + pendingPatch.reject(new Error("agent A patch failed")); + await expect(operation).rejects.toThrow("agent A patch failed"); + } + + expect(sessions.state.modelOverrides[key]).toBe("openai/gpt-agent-b"); + sessions.dispose(); + }, + ); }); diff --git a/ui/src/lib/sessions/patch.ts b/ui/src/lib/sessions/patch.ts index 6dc15355c048..9dfd2fcd04f2 100644 --- a/ui/src/lib/sessions/patch.ts +++ b/ui/src/lib/sessions/patch.ts @@ -27,6 +27,8 @@ export type SessionPatchOptions = { agentId?: string; /** Let a caller with stricter lifecycle ownership publish the resolved model value. */ deferModelOverride?: boolean; + /** Keep optimistic model state bound to the UI owner that initiated the patch. */ + ownsModelOverride?: () => boolean; /** Capture the current connection now, but dispatch only after this tail settles. */ waitFor?: Promise; /** diff --git a/ui/src/lib/sessions/session-mutations.ts b/ui/src/lib/sessions/session-mutations.ts index 10f863efe6b1..8a28293c27fd 100644 --- a/ui/src/lib/sessions/session-mutations.ts +++ b/ui/src/lib/sessions/session-mutations.ts @@ -166,8 +166,9 @@ export function createSessionMutations(host: SessionMutationsHost) { let previousModelOverride: string | null | undefined; let modelPatchStarted = false; const modelPatchToken = Symbol(); + const ownsModelOverride = () => options.ownsModelOverride?.() !== false; const startModelPatch = () => { - if (!managesModelOverride || modelPatchStarted) { + if (!managesModelOverride || modelPatchStarted || !ownsModelOverride()) { return; } const pendingModelPatch = pendingModelPatches.get(normalizedKey); @@ -187,7 +188,9 @@ export function createSessionMutations(host: SessionMutationsHost) { const restoreModelOverride = () => { if (modelPatchStarted && pendingModelPatches.get(normalizedKey)?.token === modelPatchToken) { pendingModelPatches.delete(normalizedKey); - setModelOverride(key, previousModelOverride); + if (ownsModelOverride()) { + setModelOverride(key, previousModelOverride); + } } }; try { @@ -216,7 +219,9 @@ export function createSessionMutations(host: SessionMutationsHost) { pendingModelPatches.get(normalizedKey)?.token === modelPatchToken ) { pendingModelPatches.delete(normalizedKey); - setModelOverride(key, patchParams.model); + if (ownsModelOverride()) { + setModelOverride(key, patchParams.model); + } } return result; } catch (error) { diff --git a/ui/src/pages/chat/chat-session.ts b/ui/src/pages/chat/chat-session.ts index 860793c3548c..79e33fa74875 100644 --- a/ui/src/pages/chat/chat-session.ts +++ b/ui/src/pages/chat/chat-session.ts @@ -364,7 +364,10 @@ export async function switchChatModel( if (currentOverride === nextModel) { return true; } - const previousModelOverride = host.sessions.state.modelOverrides[targetSessionKey]; + const modelOwnerAgentId = scopedAgentParamsForSession(host, targetSessionKey).agentId; + const ownsModelOverride = () => + !isUiGlobalSessionKey(targetSessionKey) || + scopedAgentParamsForSession(host, targetSessionKey).agentId === modelOwnerAgentId; setChatError(host, null, true); const switchPromiseRef: { current?: Promise } = {}; const clearPendingSwitch = () => { @@ -384,6 +387,7 @@ export async function switchChatModel( }, { ...scopedAgentParamsForSession(host, targetSessionKey), + ownsModelOverride, reconcile: async () => { await host.onModelChanged?.(); await refreshCurrentChatSessionList(host); @@ -395,8 +399,9 @@ export async function switchChatModel( } return true; } catch (err) { - host.sessions.setModelOverride(targetSessionKey, previousModelOverride); - setChatError(host, `Failed to set model: ${String(err)}`, true); + if (ownsModelOverride()) { + setChatError(host, `Failed to set model: ${String(err)}`, true); + } return false; } finally { clearPendingSwitch(); diff --git a/ui/src/pages/chat/chat-settings-patches.ts b/ui/src/pages/chat/chat-settings-patches.ts index 948c17f7cf3b..766123c32cda 100644 --- a/ui/src/pages/chat/chat-settings-patches.ts +++ b/ui/src/pages/chat/chat-settings-patches.ts @@ -116,6 +116,7 @@ export function patchChatSessionSettings( options: { agentId?: string; deferModelOverride?: boolean; + ownsModelOverride?: () => boolean; reconcile?: (result: SessionsPatchResult) => Promise | void; } = {}, ): Promise { @@ -127,6 +128,7 @@ export function patchChatSessionSettings( const result = await host.sessions.patch(sessionKey, patch, { agentId: options.agentId, deferModelOverride: options.deferModelOverride, + ownsModelOverride: options.ownsModelOverride, waitFor: previous, }); if (result) { @@ -168,6 +170,7 @@ export async function patchChatCommandSessionSettings( patch: SessionPatch, options: { deferModelOverride?: boolean; + ownsModelOverride?: () => boolean; reconcile?: (result: SessionsPatchResult) => Promise | void; } = {}, ): Promise>>> { diff --git a/ui/src/pages/chat/chat-view.test.ts b/ui/src/pages/chat/chat-view.test.ts index aee892ed2da3..bbe009e53fac 100644 --- a/ui/src/pages/chat/chat-view.test.ts +++ b/ui/src/pages/chat/chat-view.test.ts @@ -6031,6 +6031,66 @@ describe("chat model controls", () => { expect(host.chatThinkingLevel).toBe("high"); }); + it("does not restore or report a failed global model switch after the selected agent changes", async () => { + const modelPatch = createDeferred(); + const modelOverrides: Record = { + global: "openai/gpt-agent-a-old", + }; + let patchOptions: SessionPatchOptions | undefined; + const sessions = { + state: { modelOverrides }, + patch: vi.fn( + async (_key: string, _patch: Record, options?: SessionPatchOptions) => { + patchOptions = options; + return await modelPatch.promise; + }, + ), + refresh: async () => {}, + setModelOverride: vi.fn((key: string, value: string | null | undefined) => { + if (value === undefined) { + delete modelOverrides[key]; + } else { + modelOverrides[key] = value; + } + }), + patchRowLocal: vi.fn(), + }; + const host = { + assistantAgentId: "work", + agentsList: { defaultId: "main", scope: "global" }, + client: {}, + connected: true, + sessionKey: "global", + chatModelCatalog: [], + chatModelSwitchPromises: {}, + chatThinkingLevel: null, + sessions, + sessionsResult: createSessionsResultFromRows([ + { + key: "global", + kind: "direct", + updatedAt: 1, + model: "gpt-agent-a-old", + modelProvider: "openai", + }, + ]), + } as unknown as Parameters[0]; + + const switching = switchChatModel(host, "openai/gpt-agent-a-new"); + await waitForFast(() => expect(patchOptions).toBeDefined()); + expect(patchOptions?.ownsModelOverride?.()).toBe(true); + + host.assistantAgentId = "main"; + sessions.setModelOverride("global", "openai/gpt-agent-b"); + modelPatch.reject(new Error("agent A patch failed")); + + await expect(switching).resolves.toBe(false); + expect(patchOptions?.ownsModelOverride?.()).toBe(false); + expect(modelOverrides.global).toBe("openai/gpt-agent-b"); + expect(host.lastError ?? null).toBeNull(); + expect(host.chatError ?? null).toBeNull(); + }); + it("keeps the newest speed selection when an older patch fails late", async () => { const pendingPatches: Array<{ resolve: () => void; reject: (error: Error) => void }> = []; // Minimal host: the factory's mock gateway rebuilds session rows on every