diff --git a/src/node/services/workspaceService.aiSettings.test.ts b/src/node/services/workspaceService.aiSettings.test.ts index 2d3207dbcb7..36ccd0bad97 100644 --- a/src/node/services/workspaceService.aiSettings.test.ts +++ b/src/node/services/workspaceService.aiSettings.test.ts @@ -26,17 +26,20 @@ describe("WorkspaceService sendMessage AI settings persistence", () => { // send unchanged while an otherwise identical user-authored send still updates it. The status // clearing suite above only proves the persistence hook is skipped; this reads the real config. // agentId: a different built-in, a custom agent, and the already-selected agent. + // skipAiSettingsPersistence: the VS Code webview sends it for every send without an explicit pick + // (#4781), so it must leave the remembered agent and settings untouched too. test.each([ - [true, "plan"], - [false, "plan"], - [false, "reviewer"], - [false, "exec"], + [true, "plan", false], + [false, "plan", false], + [false, "reviewer", false], + [false, "exec", false], + [false, "plan", true], ] as const)( - "persists agent and AI settings only for non-synthetic sends (synthetic=%s, agentId=%s)", - async (synthetic, agentId) => { + "persists agent and AI settings only for non-synthetic sends (synthetic=%s, agentId=%s, skip=%s)", + async (synthetic, agentId, skip) => { const { config, historyService, cleanup } = await createTestHistoryService(); try { - const workspaceId = `settings-persistence-${synthetic ? "synthetic" : "manual"}-${agentId}`; + const workspaceId = `settings-persistence-${synthetic ? "synthetic" : "manual"}-${agentId}${skip ? "-skip" : ""}`; const projectPath = "/tmp/settings-persistence-project"; const remembered = { agentId: "exec", @@ -80,7 +83,12 @@ describe("WorkspaceService sendMessage AI settings persistence", () => { const result = await workspaceService.sendMessage( workspaceId, "hello", - { agentId, model: "openai:gpt-5.2", thinkingLevel: "high" }, + { + agentId, + model: "openai:gpt-5.2", + thinkingLevel: "high", + ...(skip ? { skipAiSettingsPersistence: true } : {}), + }, synthetic ? { synthetic: true } : undefined ); @@ -97,7 +105,7 @@ describe("WorkspaceService sendMessage AI settings persistence", () => { aiSettingsByAgent: entry?.aiSettingsByAgent, }; expect(persisted).toEqual( - synthetic + synthetic || skip ? remembered : { agentId, diff --git a/vscode/src/webview/App.test.tsx b/vscode/src/webview/App.test.tsx index a63516ff72a..9cdb730c3cb 100644 --- a/vscode/src/webview/App.test.tsx +++ b/vscode/src/webview/App.test.tsx @@ -510,8 +510,10 @@ describe("vscode webview workspace AI settings", () => { const options = await send(bridge, view); expect(options.agentId).toBe("plan"); expect(String(options.model)).toContain("sonnet"); - // The pick stays local (#4755): no AI-settings write reaches the workspace. + // No pick-time write reaches the workspace; the send carries the pick (#4781). expect(bridge.orpcCalls("workspace.updateAgentAISettings")).toHaveLength(0); + // Settle the persisting send so this webview session has no unresolved one (#4781). + await bridge.answer("workspace.sendMessage", { success: true, data: {} }); }); test("sends a gateway-routed model pick with its gateway ID", async () => { @@ -563,6 +565,7 @@ describe("vscode webview workspace AI settings", () => { const options = await send(bridge, view); expect(String(options.model)).toContain("sonnet"); + await bridge.answer("workspace.sendMessage", { success: true, data: {} }); }); test("shows the actual custom agent instead of mislabeling it as Exec", async () => { @@ -844,3 +847,329 @@ describe("vscode webview app and providers config", () => { expect(bridge.orpcCalls("providers.getConfig")).toHaveLength(1); }); }); + +// #4781: a send persists AI settings only for an explicit, still-current pick, once the workspace's +// settings are loaded, while admin policy allows the stored model, and never while an earlier +// persisting send for the workspace is unresolved. +describe("vscode webview explicit AI-setting persistence", () => { + let cleanupDom: (() => void) | null = null; + + beforeEach(() => { + cleanupDom = installDom(); + resetAiSelectionIntentForTests(); + }); + + afterEach(() => { + cleanup(); + cleanupDom?.(); + cleanupDom = null; + }); + + function mainWorkspace( + plan: { model: string; thinkingLevel: "low" | "medium" | "high" }, + id = WORKSPACE.id + ): UiWorkspace { + return { + ...WORKSPACE, + id, + ai: { + agentId: "plan", + aiSettingsByAgent: { + plan, + exec: { model: "anthropic:claude-opus-5-5", thinkingLevel: "medium" }, + }, + }, + }; + } + + const TERRA_HIGH = { model: "openai:gpt-5.6-terra", thinkingLevel: "high" } as const; + + async function selectById(bridge: TestBridge, workspaceId: string) { + await bridge.emit({ type: "setSelectedWorkspace", workspaceId }); + await bridge.emit({ type: "chatEvent", workspaceId, event: { type: "caught-up" } }); + } + + // `policy: "pending"` leaves policy.get unanswered; by default it answers "no policy". + async function open(workspaces: UiWorkspace[], policy: "none" | "pending" = "none") { + const bridge = new TestBridge(); + const view = render(); + await bridge.emit({ type: "connectionStatus", status: { mode: "api", baseUrl: "http://x" } }); + await bridge.emit({ type: "workspaces", workspaces }); + await selectById(bridge, workspaces[0].id); + if (policy === "none") { + await bridge.answer("policy.get", null); + } + return { bridge, view }; + } + + async function pickModel(view: ReturnType, label: string) { + await act(async () => { + fireEvent.click(view.getByRole("combobox")); + await Promise.resolve(); + }); + await act(async () => { + fireEvent.click(view.getByText(label)); + await Promise.resolve(); + }); + } + + async function pickThinking(view: ReturnType, label: string) { + const trigger = view.container.querySelector("[data-thinking-selector-trigger]"); + if (!trigger) throw new Error("thinking selector did not render"); + await act(async () => { + fireEvent.click(trigger); + await Promise.resolve(); + }); + const option = Array.from( + view.container.querySelectorAll('[role="option"]') + ).find((row) => row.getAttribute("aria-label") === label); + if (!option) throw new Error(`thinking option ${label} did not render`); + await act(async () => { + fireEvent.click(option); + await Promise.resolve(); + }); + } + + function textarea(view: ReturnType): HTMLTextAreaElement { + const element = view.container.querySelector("textarea"); + if (!element) throw new Error("composer textarea did not render"); + return element; + } + + // Types and clicks Send; returns the options of the new sendMessage call. + async function send(bridge: TestBridge, view: ReturnType) { + const before = bridge.orpcCalls("workspace.sendMessage").length; + await typeInto(textarea(view), "hello"); + await act(async () => { + fireEvent.click(view.getByRole("button", { name: "Send message" })); + await Promise.resolve(); + }); + const sends = bridge.orpcCalls("workspace.sendMessage"); + expect(sends).toHaveLength(before + 1); + const input = sends[before].input as { options?: Record }; + if (!input.options) throw new Error("sendMessage carried no options"); + return input.options; + } + + // Plays the host's reply to one sendMessage call (the newest by default). + async function reply(bridge: TestBridge, value: unknown, index = -1) { + const sends = bridge.orpcCalls("workspace.sendMessage"); + const call = sends.at(index); + if (!call) throw new Error("no sendMessage call to answer"); + await bridge.emit({ type: "orpcResponse", requestId: call.requestId, ok: true, kind: "value", value }); + } + + const OK = { success: true, data: {} }; + + test("does not persist a send without an explicit pick", async () => { + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH)]); + const options = await send(bridge, view); + expect(options.skipAiSettingsPersistence).toBe(true); + expect(options.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + }); + + test("persists an explicit model pick once, at the next send", async () => { + // "low" is below Opus 5.5's built-in minimum (MED): the companion thinking level must be sent + // (and so persisted) as stored, not raised to a client-side floor. + const { bridge, view } = await open([ + mainWorkspace({ model: "openai:gpt-5.6-terra", thinkingLevel: "low" }), + ]); + await pickModel(view, "Opus 5.5"); + + const first = await send(bridge, view); + expect(first).toMatchObject({ + agentId: "plan", + model: "anthropic:claude-opus-5-5", + thinkingLevel: "low", + skipAiSettingsPersistence: false, + aiSelectionIntent: { model: true }, + }); + await reply(bridge, OK); + + const second = await send(bridge, view); + expect(second.skipAiSettingsPersistence).toBe(true); + expect(second.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + expect(bridge.orpcCalls("workspace.updateAgentAISettings")).toHaveLength(0); + }); + + test("persists only the last of several rapid picks", async () => { + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH)]); + await pickModel(view, "Sonnet 5"); + await pickModel(view, "Opus 5.5"); + + const options = await send(bridge, view); + expect(options).toMatchObject({ + model: "anthropic:claude-opus-5-5", + skipAiSettingsPersistence: false, + aiSelectionIntent: { model: true }, + }); + await reply(bridge, OK); + }); + + test("persists an explicit thinking pick as selected", async () => { + const { bridge, view } = await open([ + mainWorkspace({ model: "openai:gpt-5.6-terra", thinkingLevel: "medium" }), + ]); + await pickThinking(view, "High"); + + const options = await send(bridge, view); + expect(options).toMatchObject({ + thinkingLevel: "high", + skipAiSettingsPersistence: false, + aiSelectionIntent: { thinkingLevel: true }, + }); + await reply(bridge, OK); + }); + + test("never persists for a workspace without loaded AI settings", async () => { + const { bridge, view } = await open([WORKSPACE]); + await pickModel(view, "Sonnet 5"); + + const options = await send(bridge, view); + expect(String(options.model)).toContain("sonnet"); + expect(options.skipAiSettingsPersistence).toBe(true); + expect(options.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + }); + + test("never persists the admin-policy fallback model", async () => { + // Exec is seeded with Opus 5.5, which the policy excludes. + const workspace: UiWorkspace = { + ...mainWorkspace(TERRA_HIGH), + ai: { ...mainWorkspace(TERRA_HIGH).ai, agentId: "exec" }, + }; + const { bridge, view } = await open([workspace], "pending"); + await bridge.answer("policy.get", { + source: "governor", + status: { state: "enforced" }, + policy: { + policyFormatVersion: "0.1", + providerAccess: [{ id: "openai", allowedModels: ["gpt-5.6-terra"] }], + mcp: { allowUserDefined: { stdio: true, remote: true } }, + runtimes: null, + }, + }); + await bridge.answer("providers.getConfig", { + openai: { apiKeySet: true, isEnabled: true, isConfigured: true }, + }); + try { + await pickThinking(view, "High"); + + const options = await send(bridge, view); + expect(options.model).toBe("openai:gpt-5.6-terra"); + expect(options.skipAiSettingsPersistence).toBe(true); + expect(options.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + } finally { + await clearProvidersConfig(bridge); + } + }); + + test("does not persist while the admin policy is still loading", async () => { + // Until policy.get answers, the model list is unfiltered and the policy looks disabled, so a + // pick could be a model the policy forbids. + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH)], "pending"); + await pickModel(view, "Sonnet 5"); + + const options = await send(bridge, view); + expect(String(options.model)).toContain("sonnet"); + expect(options.skipAiSettingsPersistence).toBe(true); + expect(options.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + }); + + test("persists a locked sub-agent's pick only into its locked agent", async () => { + // agentId was restamped by a recovery send; agentType is the child's creation-time identity. + const { bridge, view } = await open([ + { + ...WORKSPACE, + ai: { + parentWorkspaceId: "ws-parent", + agentId: "plan", + agentType: "exec", + aiSettingsByAgent: { exec: TERRA_HIGH }, + }, + }, + ]); + await pickModel(view, "Sonnet 5"); + + expect((view.getByRole("button", { name: "Exec" }) as HTMLButtonElement).disabled).toBe(true); + const options = await send(bridge, view); + expect(options).toMatchObject({ + agentId: "exec", + skipAiSettingsPersistence: false, + aiSelectionIntent: { model: true }, + }); + expect(String(options.model)).toContain("sonnet"); + await reply(bridge, OK); + }); + + test("keeps one persisting send per workspace in flight; the next send writes the latest pick", async () => { + const other = mainWorkspace(TERRA_HIGH, "ws-2"); + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH), other]); + await pickModel(view, "Sonnet 5"); + const first = await send(bridge, view); + expect(first.skipAiSettingsPersistence).toBe(false); + + // Enter must not start a second send while the first is in flight (the Send button is disabled). + await typeInto(textarea(view), "second"); + await act(async () => { + fireEvent.keyDown(textarea(view), { key: "Enter" }); + await Promise.resolve(); + }); + expect(bridge.orpcCalls("workspace.sendMessage")).toHaveLength(1); + + // Switching workspaces remounts the composer; the first send is still unresolved. + await selectById(bridge, other.id); + await selectById(bridge, WORKSPACE.id); + await pickModel(view, "Opus 5.5"); + const overlapping = await send(bridge, view); + expect(overlapping.model).toBe("anthropic:claude-opus-5-5"); + expect(overlapping.skipAiSettingsPersistence).toBe(true); + expect(overlapping.aiSelectionIntent).toBeUndefined(); + + await reply(bridge, OK, 0); + await reply(bridge, OK, 1); + const next = await send(bridge, view); + expect(next).toMatchObject({ + model: "anthropic:claude-opus-5-5", + skipAiSettingsPersistence: false, + aiSelectionIntent: { model: true }, + }); + await reply(bridge, OK); + }); + + test("stops persisting for a workspace after a send ends without a server result", async () => { + // Own workspace ID: the unknown outcome lasts for this webview session. + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH, "ws-unknown-outcome")]); + await pickModel(view, "Sonnet 5"); + const first = await send(bridge, view); + expect(first.skipAiSettingsPersistence).toBe(false); + const call = bridge.orpcCalls("workspace.sendMessage")[0]; + await bridge.emit({ type: "orpcResponse", requestId: call.requestId, ok: false, error: "network" }); + + await pickModel(view, "Opus 5.5"); + const next = await send(bridge, view); + expect(next.skipAiSettingsPersistence).toBe(true); + expect(next.aiSelectionIntent).toBeUndefined(); + await reply(bridge, OK); + }); + + test("keeps a pick pending after a server-reported send failure", async () => { + const { bridge, view } = await open([mainWorkspace(TERRA_HIGH)]); + await pickModel(view, "Sonnet 5"); + const first = await send(bridge, view); + expect(first.skipAiSettingsPersistence).toBe(false); + await reply(bridge, { success: false, error: { type: "policy_denied", message: "denied" } }); + + const retry = await send(bridge, view); + expect(retry).toMatchObject({ + skipAiSettingsPersistence: false, + aiSelectionIntent: { model: true }, + }); + expect(String(retry.model)).toContain("sonnet"); + await reply(bridge, OK); + }); +}); diff --git a/vscode/src/webview/App.tsx b/vscode/src/webview/App.tsx index a870e0270e5..b6d67dd3662 100644 --- a/vscode/src/webview/App.tsx +++ b/vscode/src/webview/App.tsx @@ -733,6 +733,7 @@ export function App(props: { bridge: VscodeBridge }): JSX.Element { : undefined } aggregator={aggregatorRef.current} + aiSettingsLoaded={selectedWorkspace?.ai != null} onSendComplete={jumpToBottom} onNotice={pushNotice} /> diff --git a/vscode/src/webview/ChatComposer.tsx b/vscode/src/webview/ChatComposer.tsx index 50e23a3fe21..5b60d7edfb8 100644 --- a/vscode/src/webview/ChatComposer.tsx +++ b/vscode/src/webview/ChatComposer.tsx @@ -16,8 +16,14 @@ import { ThinkingProvider } from "xum/browser/contexts/ThinkingContext"; import { usePersistedState, updatePersistedState } from "xum/browser/hooks/usePersistedState"; import { useModelsFromSettings } from "xum/browser/hooks/useModelsFromSettings"; import { useProvidersConfig } from "xum/browser/hooks/useProvidersConfig"; +import { usePolicy } from "xum/browser/contexts/PolicyContext"; import { normalizeSelectedModel } from "xum/common/utils/ai/models"; -import { markAiSelectionIntent } from "xum/browser/utils/aiSelectionIntent"; +import { + consumeAiSelectionIntent, + getAiSelectionIntentForSendOptions, + markAiSelectionIntent, +} from "xum/browser/utils/aiSelectionIntent"; +import assert from "xum/common/utils/assert"; import { useProviderOptions } from "xum/browser/hooks/useProviderOptions"; import { useAutoCompactionSettings } from "xum/browser/hooks/useAutoCompactionSettings"; @@ -42,6 +48,14 @@ import { const SEND_MESSAGE_TIMEOUT_MS = 30_000; +// #4781: at most one AI-settings-persisting send per workspace may be unresolved, so an earlier +// write can never land after a later pick's. The backend saves the settings before sendMessage +// returns, so a server reply (success or failure) settles the entry. "unknown": a persisting send +// ended without a reply (timeout abort or transport error), so its write may still land; later +// sends for that workspace do not persist until the webview reloads (fail closed; the picks still +// apply to the turns). Module scope, because the composer remounts per workspace. +const aiPersistenceByWorkspace = new Map(); + /** * Simple agent toggle for VS Code extension (no agent discovery). * Just toggles between Exec and Plan agents. @@ -117,6 +131,8 @@ function ChatComposerInner(props: { disabled: boolean; disabledReason?: string | undefined; aggregator: StreamingMessageAggregator | null; + /** The workspace's own AI settings are loaded; until then nothing may be persisted (#4781). */ + aiSettingsLoaded: boolean; onSendComplete: () => void; onNotice: (notice: { level: "info" | "error"; message: string }) => void; }): JSX.Element { @@ -165,6 +181,8 @@ function ChatComposerInner(props: { // Until the providers config arrives, the model list is not filtered by provider availability, // so a fallback could pick a provider without credentials; substitute nothing until then. const { config: providersConfig } = useProvidersConfig(); + // Until the first policy.get settles, the policy looks disabled and the model list is unfiltered. + const { loading: policyLoading } = usePolicy(); const policyFallbackModel = storedModelAllowed || providersConfig === null ? null @@ -252,7 +270,7 @@ function ChatComposerInner(props: { {} ); - // #4755: a model change still stays local; sends do not persist AI settings yet (#4781). + // #4781: nothing is written here; the next send persists the pick (desktop parity). }; const cycleModels = customModels.length > 0 ? customModels : models; @@ -272,8 +290,9 @@ function ChatComposerInner(props: { const onSend = async () => { // Re-check at dispatch: the composer can be disabled (e.g. history replay not caught up) - // after the keystroke or click that triggered this send. - if (props.disabled) { + // after the keystroke or click that triggered this send. Like the disabled Send button (and the + // desktop composer), Enter must not start a second send while one is in flight. + if (props.disabled || isSending) { return; } const trimmed = input.trim(); @@ -307,16 +326,37 @@ function ChatComposerInner(props: { controller.abort(); }, SEND_MESSAGE_TIMEOUT_MS); + const baseOptions = { + ...getSendOptionsFromStorage(props.workspaceId), + // The effective agent: for a sub-agent workspace, the locked agent (#4738), not a local pick. + agentId, + }; + // #4781: persist only explicit picks, through the desktop's send-time path (the backend saves the + // sent settings unless skipAiSettingsPersistence; aiSelectionIntent pins them on a sub-agent). + // Never seeded values (no pending pick), never before the workspace's settings are loaded + // (#4755), never before the admin policy has loaded or for a policy-excluded or fallback model + // (#4808), and never while an earlier persisting send for this workspace is unresolved. The + // thinking level is sent as selected; the backend applies the authoritative floor. + const mayPersist = + props.aiSettingsLoaded && + !policyLoading && + storedModelAllowed && + !aiPersistenceByWorkspace.has(props.workspaceId); + const aiSelection = getAiSelectionIntentForSendOptions(props.workspaceId, agentId, { + ...baseOptions, + skipAiSettingsPersistence: !mayPersist, + }); + const persist = aiSelection.intent !== undefined; + assert(!persist || policyFallbackModel === null, "a policy fallback model must never be persisted"); + if (persist) { + aiPersistenceByWorkspace.set(props.workspaceId, "in-flight"); + } + try { const options = { - ...getSendOptionsFromStorage(props.workspaceId), - // The effective agent: for a sub-agent workspace, the locked agent (#4738), not a local pick. - agentId, - // #4755: never persist from the webview. Even with the workspace's settings seeded (#4738), - // saving needs the desktop's selection-intent/gateway-route handling (#4778 review). - // The thinking level is sent as selected: the webview does not load the user's configured - // per-model minimums, so only the backend can apply the authoritative floor. - skipAiSettingsPersistence: true, + ...baseOptions, + skipAiSettingsPersistence: !persist, + ...(persist ? { aiSelectionIntent: aiSelection.intent } : {}), // Only when the stored model is policy-excluded; otherwise keep the stored model string. ...(policyFallbackModel ? { model: policyFallbackModel } : {}), }; @@ -329,6 +369,10 @@ function ChatComposerInner(props: { }, { signal: controller.signal } ); + if (persist) { + // The server replied, so this send's settings write has landed (or was skipped). + aiPersistenceByWorkspace.delete(props.workspaceId); + } if (!result.success) { const errorString = @@ -338,8 +382,15 @@ function ChatComposerInner(props: { return; } + if (persist) { + // A pick made while this send was in flight has a newer token and stays pending. + consumeAiSelectionIntent(props.workspaceId, agentId, aiSelection.attachedTokens); + } props.onSendComplete(); } catch (error) { + if (persist) { + aiPersistenceByWorkspace.set(props.workspaceId, "unknown"); + } if (controller.signal.aborted) { props.onNotice({ level: "error", @@ -481,6 +532,8 @@ export function ChatComposer(props: { disabled: boolean; disabledReason?: string | undefined; aggregator: StreamingMessageAggregator | null; + /** The workspace's own AI settings are loaded; until then nothing may be persisted (#4781). */ + aiSettingsLoaded: boolean; onSendComplete: () => void; onNotice: (notice: { level: "info" | "error"; message: string }) => void; }): JSX.Element { @@ -492,6 +545,7 @@ export function ChatComposer(props: { disabled={props.disabled} disabledReason={props.disabledReason} aggregator={props.aggregator} + aiSettingsLoaded={props.aiSettingsLoaded} onSendComplete={props.onSendComplete} onNotice={props.onNotice} />