diff --git a/__tests__/hooks/mutation/use-switch-llm-profile.test.tsx b/__tests__/hooks/mutation/use-switch-llm-profile.test.tsx index fd47773fdb..ef724fd983 100644 --- a/__tests__/hooks/mutation/use-switch-llm-profile.test.tsx +++ b/__tests__/hooks/mutation/use-switch-llm-profile.test.tsx @@ -27,7 +27,12 @@ vi.mock( }), ); -vi.mock("#/hooks/chat/record-model-switch-message", () => ({ +vi.mock("#/hooks/chat/record-model-switch-message", async (importOriginal) => ({ + // Keep the real stampActiveLlmProfile so the metadata-stamp assertions + // below exercise the actual write; only the inline-message recorder is spied. + ...(await importOriginal< + typeof import("#/hooks/chat/record-model-switch-message") + >()), recordModelSwitchMessage: vi.fn(), })); @@ -226,7 +231,10 @@ describe("useSwitchLlmProfile", () => { selected_branch: "main", git_provider: "github", selected_workspace: null, + workspace_mode: null, active_profile: "claude-sonnet-4.6", + // Client-clock ISO timestamp; the mutation path has no event timestamp. + stamped_at: expect.any(String), plugins: null, }); }); diff --git a/src/api/conversation-metadata-store.ts b/src/api/conversation-metadata-store.ts index be059b3682..c2bb299167 100644 --- a/src/api/conversation-metadata-store.ts +++ b/src/api/conversation-metadata-store.ts @@ -35,6 +35,17 @@ export interface ConversationMetadata { * is ambiguous (issue #1082). */ active_profile?: string | null; + /** + * When `active_profile` was stamped (ISO string). Written by the live + * WebSocket handler (the observation's server timestamp) and the user + * `/model` mutation (client clock). The history seed compares the latest + * SwitchLLM observation's timestamp against this so a reload can repair a + * missed agent switch without rolling back a newer manual switch — manual + * `/model` switches go through REST and leave no observation in history. + * Absent on stamps written before this field existed; treated as older + * than any observation so the seed can still repair them. + */ + stamped_at?: string | null; /** Store plugin coordinates only; parameters may contain secrets. */ plugins?: PluginSpec[] | null; } diff --git a/src/contexts/conversation-websocket-context.tsx b/src/contexts/conversation-websocket-context.tsx index 9aa9bd2441..3178190373 100644 --- a/src/contexts/conversation-websocket-context.tsx +++ b/src/contexts/conversation-websocket-context.tsx @@ -69,15 +69,12 @@ import { setConversationState } from "#/utils/conversation-local-storage"; import { recordModelSwitchMessage, seedModelSwitchesFromHistory, + stampActiveLlmProfile, } from "#/hooks/chat/record-model-switch-message"; import { invalidateConversationQueries, updateConversationLlmModelInCache, } from "#/hooks/mutation/conversation-mutation-utils"; -import { - getStoredConversationMetadata, - setStoredConversationMetadata, -} from "#/api/conversation-metadata-store"; export type WebSocketConnectionState = | "CONNECTING" @@ -697,18 +694,14 @@ export function ConversationWebSocketProvider({ // Mirror the user-driven `/model` path: persist the profile so the // chat-header switcher shows the right name after a reload, even - // when several profiles share a model (#1082). - const prevMetadata = getStoredConversationMetadata(conversationId); - setStoredConversationMetadata(conversationId, { - selected_repository: prevMetadata?.selected_repository ?? null, - selected_branch: prevMetadata?.selected_branch ?? null, - git_provider: prevMetadata?.git_provider ?? null, - selected_workspace: prevMetadata?.selected_workspace ?? null, - active_profile: switchLLMObservation.observation.profile_name, - // Full-object replace: carry the plugins snapshot forward so the - // in-conversation plugins view survives a profile switch. - plugins: prevMetadata?.plugins ?? null, - }); + // when several profiles share a model (#1082). Stamp with the + // observation's own timestamp so a later history seed of this same + // event can't roll it back (or needlessly rewrite it). + stampActiveLlmProfile( + conversationId, + switchLLMObservation.observation.profile_name, + switchLLMObservation.timestamp, + ); if (switchLLMObservation.observation.active_model) { updateConversationLlmModelInCache( diff --git a/src/hooks/chat/record-model-switch-message.test.ts b/src/hooks/chat/record-model-switch-message.test.ts index 84b0360742..c53f487d53 100644 --- a/src/hooks/chat/record-model-switch-message.test.ts +++ b/src/hooks/chat/record-model-switch-message.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect, beforeEach } from "vitest"; import { useModelStore } from "#/stores/model-store"; +import { + getStoredConversationMetadata, + setStoredConversationMetadata, +} from "#/api/conversation-metadata-store"; import { OpenHandsEvent } from "#/types/agent-server/core"; import { seedModelSwitchesFromHistory } from "./record-model-switch-message"; @@ -15,10 +19,11 @@ const switchObservation = ( id: string, profileName: string, isError = false, + timestamp = "2024-01-01T00:00:00Z", ): OpenHandsEvent => ({ id, - timestamp: "2024-01-01T00:00:00Z", + timestamp, source: "environment", action_id: `action-${id}`, observation: { @@ -46,9 +51,16 @@ const agentAction = (id: string, kind: string): OpenHandsEvent => const entriesFor = (conversationId: string) => useModelStore.getState().entriesByConversation[conversationId] ?? []; +const activeProfileFor = (conversationId: string) => + useModelStore.getState().activeProfileByConversation[conversationId]; + +const stampedProfileFor = (conversationId: string) => + getStoredConversationMetadata(conversationId)?.active_profile; + describe("seedModelSwitchesFromHistory", () => { beforeEach(() => { useModelStore.getState().clearAll(); + window.localStorage.clear(); }); it("seeds a successful switch anchored to the prior renderable event", () => { @@ -137,4 +149,184 @@ describe("seedModelSwitchesFromHistory", () => { anchorEventId: "u2", }); }); + + it("re-stamps the active profile from a switch in the loaded history", () => { + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(activeProfileFor("c1")).toBe("fast"); + expect(stampedProfileFor("c1")).toBe("fast"); + }); + + it("stamps the latest switch when history holds several", () => { + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + userMessage("u2"), + switchObservation("o2", "architect"), + ]); + + expect(activeProfileFor("c1")).toBe("architect"); + expect(stampedProfileFor("c1")).toBe("architect"); + }); + + it("leaves the stamp untouched when history has no switch", () => { + setStoredConversationMetadata("c1", { + selected_repository: null, + selected_branch: null, + git_provider: null, + selected_workspace: null, + active_profile: "original", + plugins: null, + }); + useModelStore.getState().setActiveProfile("c1", "original"); + + seedModelSwitchesFromHistory("c1", [userMessage("u1")]); + + expect(activeProfileFor("c1")).toBe("original"); + expect(stampedProfileFor("c1")).toBe("original"); + }); + + it("does not stamp from a failed switch", () => { + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("e1", "fast", true), + ]); + + expect(activeProfileFor("c1")).toBeUndefined(); + expect(stampedProfileFor("c1")).toBeUndefined(); + }); + + it("keys the stamp per conversation", () => { + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(activeProfileFor("c2")).toBeUndefined(); + expect(stampedProfileFor("c2")).toBeUndefined(); + }); + + it("preserves the other stored metadata fields when re-stamping", () => { + setStoredConversationMetadata("c1", { + selected_repository: "org/repo", + selected_branch: "main", + git_provider: "github", + selected_workspace: "/ws", + active_profile: "stale", + plugins: [{ source: "s", ref: "r", repo_path: null }], + }); + + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(getStoredConversationMetadata("c1")).toMatchObject({ + selected_repository: "org/repo", + selected_branch: "main", + git_provider: "github", + selected_workspace: "/ws", + active_profile: "fast", + plugins: [{ source: "s", ref: "r", repo_path: null }], + }); + }); + + it("preserves workspace_mode across a stamp (#15520)", () => { + setStoredConversationMetadata("c1", { + selected_repository: null, + selected_branch: null, + git_provider: null, + selected_workspace: "/ws", + workspace_mode: "local_repo", + }); + + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(getStoredConversationMetadata("c1")?.workspace_mode).toBe( + "local_repo", + ); + }); + + it("leaves a newer manual stamp alone (manual switch after a tool switch survives reload)", () => { + // t1: agent tool-switches to "fast" (observation in history). t2: user + // manually switches back to "architect" via /model — a REST call that + // leaves no observation. The reload seed must not roll the stamp back. + setStoredConversationMetadata("c1", { + selected_repository: null, + selected_branch: null, + git_provider: null, + active_profile: "architect", + stamped_at: "2024-06-01T00:00:00Z", + }); + + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(getStoredConversationMetadata("c1")?.active_profile).toBe( + "architect", + ); + expect(getStoredConversationMetadata("c1")?.stamped_at).toBe( + "2024-06-01T00:00:00Z", + ); + // The in-memory stamp takes priority over the persisted one in the pill, + // so it must not be set to the stale profile either. + expect(activeProfileFor("c1")).toBeUndefined(); + }); + + it("repairs a legacy stamp written before stamped_at existed", () => { + setStoredConversationMetadata("c1", { + selected_repository: null, + selected_branch: null, + git_provider: null, + active_profile: "architect", + }); + + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(stampedProfileFor("c1")).toBe("fast"); + expect(getStoredConversationMetadata("c1")?.stamped_at).toBe( + "2024-01-01T00:00:00Z", + ); + }); + + it("re-stamps when the latest observation is newer than the stored stamp", () => { + setStoredConversationMetadata("c1", { + selected_repository: null, + selected_branch: null, + git_provider: null, + active_profile: "architect", + stamped_at: "2023-01-01T00:00:00Z", + }); + + seedModelSwitchesFromHistory("c1", [ + userMessage("u1"), + switchObservation("o1", "fast"), + ]); + + expect(stampedProfileFor("c1")).toBe("fast"); + expect(getStoredConversationMetadata("c1")?.stamped_at).toBe( + "2024-01-01T00:00:00Z", + ); + }); + + it("stamps with the observation timestamp so re-seeding the same history is a no-op", () => { + const events = [userMessage("u1"), switchObservation("o1", "fast")]; + seedModelSwitchesFromHistory("c1", events); + seedModelSwitchesFromHistory("c1", events); + + expect(getStoredConversationMetadata("c1")?.stamped_at).toBe( + "2024-01-01T00:00:00Z", + ); + }); }); diff --git a/src/hooks/chat/record-model-switch-message.ts b/src/hooks/chat/record-model-switch-message.ts index 8c98ee0cbb..0fd23feb24 100644 --- a/src/hooks/chat/record-model-switch-message.ts +++ b/src/hooks/chat/record-model-switch-message.ts @@ -1,5 +1,9 @@ import { getLastRenderableEventId } from "#/hooks/chat/model-command-event-anchor"; import { useModelStore, SeededSwitch } from "#/stores/model-store"; +import { + getStoredConversationMetadata, + setStoredConversationMetadata, +} from "#/api/conversation-metadata-store"; import { OpenHandsEvent } from "#/types/agent-server/core"; import { isSwitchLLMObservationEvent } from "#/types/agent-server/type-guards"; import { shouldRenderEvent } from "#/components/conversation-events/chat/event-content-helpers/should-render-event"; @@ -14,6 +18,45 @@ export function recordModelSwitchMessage( .recordSwitch(conversationId, anchorEventId, profileName); } +/** + * The active-profile stamp for a conversation: the optimistic in-memory entry + * (read first by the chat-input pill) plus the persisted per-conversation + * `active_profile` metadata (survives reloads, round-tripped onto the + * conversation by the agent-server adapter — issue #1082). Every path that + * learns of a successful switch — the user `/model` mutation, the live + * WebSocket handler, and the history seed below — must write through this one + * function so the live and reload paths can't drift. + * + * `stampedAt` is persisted alongside the profile so the history seed can tell + * a stale observation apart from a newer manual switch (see below). The live + * WebSocket handler passes the observation's own (server) timestamp; the user + * `/model` mutation has no event and defaults to the client clock. + */ +export function stampActiveLlmProfile( + conversationId: string, + profileName: string, + stampedAt: string = new Date().toISOString(), +) { + useModelStore.getState().setActiveProfile(conversationId, profileName); + + const prev = getStoredConversationMetadata(conversationId); + setStoredConversationMetadata(conversationId, { + selected_repository: prev?.selected_repository ?? null, + selected_branch: prev?.selected_branch ?? null, + git_provider: prev?.git_provider ?? null, + selected_workspace: prev?.selected_workspace ?? null, + // Carry the attached-workspace mode forward too — the full-object replace + // would otherwise drop it (#15520). This helper is the single write site + // for the profile-switch stamp, so one line covers every path. + workspace_mode: prev?.workspace_mode ?? null, + active_profile: profileName, + stamped_at: stampedAt, + // Full-object replace: carry the plugins snapshot forward so the + // in-conversation plugins view survives a profile switch. + plugins: prev?.plugins ?? null, + }); +} + /** * Rebuilds the inline "Switched to" messages for a conversation from its loaded * history. @@ -35,6 +78,22 @@ export function recordModelSwitchMessage( * Each successful switch is anchored to the last renderable event before it, * matching where the live handler would have placed it. Idempotent: entries are * keyed by the observation event id, so re-seeding on every reload is a no-op. + * + * Also re-derives the active-profile stamp from the latest successful + * SwitchLLM observation. The stamp is otherwise written only by the live + * WebSocket handler and the user `/model` mutation, so a switch that fired + * while the socket was down would never be stamped — after a reload the pill + * showed the stale profile while the panel showed the server's true model. + * + * The re-stamp is gated on the stored `stamped_at`: manual `/model` switches + * go through REST and leave NO observation in history, so an unconditional + * re-stamp would roll a newer manual switch back to an older tool switch. + * The seed therefore only stamps when there is no existing stamp, the stamp + * predates this field (legacy — treated as older than any observation so it + * still gets repaired), or the latest observation is strictly newer. + * Observation timestamps are server-generated while `stamped_at` from the + * mutation path is client-generated, so the cross-clock comparison is a + * heuristic — acceptable here since the alternative is a silent rollback. */ export function seedModelSwitchesFromHistory( conversationId: string, @@ -42,6 +101,7 @@ export function seedModelSwitchesFromHistory( ) { const switches: SeededSwitch[] = []; let lastRenderableId: string | null = null; + let latestSwitch: { profileName: string; timestamp: string } | null = null; for (const event of uiEvents) { if (isSwitchLLMObservationEvent(event) && !event.observation.is_error) { @@ -50,6 +110,10 @@ export function seedModelSwitchesFromHistory( anchorEventId: lastRenderableId, profileName: event.observation.profile_name, }); + latestSwitch = { + profileName: event.observation.profile_name, + timestamp: event.timestamp, + }; } if (shouldRenderEvent(event)) { lastRenderableId = String(event.id); @@ -59,4 +123,15 @@ export function seedModelSwitchesFromHistory( if (switches.length > 0) { useModelStore.getState().seedSwitches(conversationId, switches); } + + if (latestSwitch) { + const { profileName, timestamp } = latestSwitch; + const stored = getStoredConversationMetadata(conversationId); + const stampedAt = stored?.active_profile ? stored.stamped_at : null; + if (!stampedAt || Date.parse(timestamp) > Date.parse(stampedAt)) { + // Stamp with the observation's own timestamp so re-seeding the same + // history on the next reload is a no-op (strictly-newer never matches). + stampActiveLlmProfile(conversationId, profileName, timestamp); + } + } } diff --git a/src/hooks/mutation/use-switch-llm-profile.ts b/src/hooks/mutation/use-switch-llm-profile.ts index b6d64b7cce..d0efc5edc6 100644 --- a/src/hooks/mutation/use-switch-llm-profile.ts +++ b/src/hooks/mutation/use-switch-llm-profile.ts @@ -7,11 +7,10 @@ import { SETTINGS_QUERY_KEYS, } from "#/hooks/query/query-keys"; import { getLastRenderableEventId } from "#/hooks/chat/model-command-event-anchor"; -import { recordModelSwitchMessage } from "#/hooks/chat/record-model-switch-message"; import { - getStoredConversationMetadata, - setStoredConversationMetadata, -} from "#/api/conversation-metadata-store"; + recordModelSwitchMessage, + stampActiveLlmProfile, +} from "#/hooks/chat/record-model-switch-message"; import { displayErrorToast } from "#/utils/custom-toast-handlers"; import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message"; import { I18nKey } from "#/i18n/declaration"; @@ -80,15 +79,7 @@ export const useSwitchLlmProfile = () => { // Keep the per-conversation profile identity fresh so the chat-header // switcher shows the right name after a reload (the agent-server only // round-trips the model string). #1082 - const prev = getStoredConversationMetadata(conversationId); - setStoredConversationMetadata(conversationId, { - selected_repository: prev?.selected_repository ?? null, - selected_branch: prev?.selected_branch ?? null, - git_provider: prev?.git_provider ?? null, - selected_workspace: prev?.selected_workspace ?? null, - active_profile: profileName, - plugins: prev?.plugins ?? null, - }); + stampActiveLlmProfile(conversationId, profileName); } else { // Home-page activate path (same server endpoint as // useActivateLlmProfile): clear the SettingsService cache so the next diff --git a/src/stores/model-store.ts b/src/stores/model-store.ts index 0b6ff5211c..c856a96e50 100644 --- a/src/stores/model-store.ts +++ b/src/stores/model-store.ts @@ -54,6 +54,13 @@ interface ModelActions { * without duplicating, and it preserves live-recorded entries. */ seedSwitches: (conversationId: string, switches: SeededSwitch[]) => void; + /** + * Sets the optimistic active-profile entry for a conversation without + * appending a chat entry — the reload/history path re-derives the stamp + * from past SwitchLLM observations, which seed their chat entries through + * `seedSwitches` instead of `recordSwitch`. + */ + setActiveProfile: (conversationId: string, profileName: string) => void; /** Drops only the optimistic active-profile entry for a conversation. */ clearActiveProfile: (conversationId: string) => void; clear: (conversationId: string) => void; @@ -124,6 +131,13 @@ export const useModelStore = create()( }, }; }), + setActiveProfile: (conversationId, profileName) => + set((s) => ({ + activeProfileByConversation: { + ...s.activeProfileByConversation, + [conversationId]: profileName, + }, + })), clearActiveProfile: (conversationId) => set((s) => { if (!(conversationId in s.activeProfileByConversation)) return s;