mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-06 15:03:43 +08:00
fix(chat): re-derive active profile stamp from event history on reload (#16439)
Co-authored-by: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com>
This commit is contained in:
co-authored by
Vasco Schiavo
parent
b50c60c672
commit
226a6d2e68
@@ -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,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<ModelStore>()(
|
||||
},
|
||||
};
|
||||
}),
|
||||
setActiveProfile: (conversationId, profileName) =>
|
||||
set((s) => ({
|
||||
activeProfileByConversation: {
|
||||
...s.activeProfileByConversation,
|
||||
[conversationId]: profileName,
|
||||
},
|
||||
})),
|
||||
clearActiveProfile: (conversationId) =>
|
||||
set((s) => {
|
||||
if (!(conversationId in s.activeProfileByConversation)) return s;
|
||||
|
||||
Reference in New Issue
Block a user