From 5fccc9aae9c3a698efb2bf37b8b9607c75eaac73 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 22 Jul 2026 23:42:43 -0500 Subject: [PATCH] fix(desktop): stop model-switch dup + route recovery resumes to the owning profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two Desktop session-reconciliation symptoms from #67603. Symptom 1 — duplicated user bubble after a model switch. The gateway persists model-switch / personality notices as role=user `[System: …]` rows (tui_gateway/server.py) so strict OpenAI-compatible providers don't reject a non-leading system message (#48338). `preserveLocalPendingTurnMessages` paired local optimistic rows with the stored transcript by user-role ordinal, so a marker between two real user turns shifted every later ordinal and the optimistic row was re-appended at the bottom. The single trailing-marker case is already covered by the compression-era `latestAuthoritativeUser` guard, but two switches around one turn (marker before AND after the committed prompt) still duplicated it. Exclude `[System:` bookkeeping markers from ordinal pairing on both sides. Symptom 2 — a session appearing under two profiles. The main resume path already resolves a session's owning profile via `resolveStoredSession` (cache → active backend → cross-profile probe), but the recovery `session.resume` calls (stale runtime id, session-not-found, wedged loop, redirect) omitted `profile`, so the gateway fell back to the launch-profile DB and forked the conversation into the wrong profile. Route every recovery resume — and an uncached right-click branch — through the same resolver so the profile is carried even for sessions outside the paginated sidebar window (the cache-miss gap). Tests: discriminating two-switch marker test (fails before, passes after); cache-hit + cross-profile cache-miss coverage for the recovery resume and for branching an uncached session. Supersedes #68665 and #63590. Closes #67603. Co-authored-by: Dolverin <5910064+Dolverin@users.noreply.github.com> Co-authored-by: oliviaaaa7788 <274182427+oliviaaaa7788@users.noreply.github.com> --- .../hooks/use-prompt-actions/index.test.tsx | 95 +++++++++++++++++++ .../session/hooks/use-prompt-actions/index.ts | 11 ++- .../hooks/use-prompt-actions/submit.ts | 13 ++- .../hooks/use-session-actions.test.tsx | 36 ++++++- .../hooks/use-session-actions/index.ts | 8 +- .../hooks/use-session-actions/utils.test.ts | 52 ++++++++++ .../hooks/use-session-actions/utils.ts | 40 ++++++++ 7 files changed, 249 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx index f27fc5b750a5..a7de88bf4de3 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx @@ -3,6 +3,7 @@ import type { MutableRefObject } from 'react' import { useEffect, useRef } from 'react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { getSession } from '@/hermes' import { textPart } from '@/lib/chat-messages' import { $composerAttachments, $composerDraft, type ComposerAttachment, setComposerDraft } from '@/store/composer' import { $notifications, clearNotifications } from '@/store/notifications' @@ -25,6 +26,7 @@ import { uploadComposerAttachment, usePromptActions } from '.' vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), + getSession: vi.fn(), PROMPT_SUBMIT_REQUEST_TIMEOUT_MS: 1_800_000, setApiRequestProfile: vi.fn(), transcribeAudio: vi.fn() @@ -1834,6 +1836,99 @@ describe('usePromptActions sleep/wake session recovery', () => { expect(calls[2]?.params).toEqual({ session_id: RECOVERED_SESSION_ID, text: 'message after wake' }) }) + // #67603 (second symptom): a recovery resume must re-register on the session's + // OWNING profile. Resuming on whichever profile is live forks the conversation + // into the wrong profile's DB — the session then appears under both profiles. + it('carries the owning profile from the cache into the recovery resume', async () => { + setSessions(() => [sessionInfo({ id: STORED_SESSION_ID, profile: 'work' })]) + + const calls: { method: string; params?: Record }[] = [] + let submitAttempts = 0 + + const requestGateway = vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params }) + + if (method === 'prompt.submit') { + submitAttempts += 1 + + if (submitAttempts === 1) { + throw new Error('session not found') + } + + return {} as never + } + + if (method === 'session.resume') { + return { session_id: RECOVERED_SESSION_ID } as never + } + + return {} as never + }) + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + storedSessionId={STORED_SESSION_ID} + /> + ) + + expect(await handle!.submitText('message after wake')).toBe(true) + expect(calls[1]?.params).toEqual({ session_id: STORED_SESSION_ID, source: 'desktop', profile: 'work' }) + + setSessions(() => []) + }) + + // The session lives on another profile and is outside the paginated sidebar + // cache: resolve it by id across profiles rather than resuming profile-blind. + it('resolves the owning profile across profiles when the session is not cached', async () => { + // module-factory vi.fn is not reset by restoreAllMocks — reset explicitly in + // the finally below so this resolved value never leaks into sibling tests. + setSessions(() => []) + vi.mocked(getSession).mockResolvedValue(sessionInfo({ id: STORED_SESSION_ID, profile: 'work' })) + + const calls: { method: string; params?: Record }[] = [] + let submitAttempts = 0 + + const requestGateway = vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params }) + + if (method === 'prompt.submit') { + submitAttempts += 1 + + if (submitAttempts === 1) { + throw new Error('session not found') + } + + return {} as never + } + + if (method === 'session.resume') { + return { session_id: RECOVERED_SESSION_ID } as never + } + + return {} as never + }) + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + storedSessionId={STORED_SESSION_ID} + /> + ) + + expect(await handle!.submitText('message after wake')).toBe(true) + expect(calls[1]?.params).toEqual({ session_id: STORED_SESSION_ID, source: 'desktop', profile: 'work' }) + + vi.mocked(getSession).mockReset() + setSessions(() => []) + }) + it('background queue resume uses the queued stored id and leaves foreground runtime selected', async () => { const calls: { method: string; params?: Record }[] = [] let submitAttempts = 0 diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts index f7edf24f14c1..1e314fe714a8 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts @@ -43,6 +43,7 @@ import type { ImageAttachResponse, SessionRedirectResponse } from '../../../types' +import { resolveSessionProfile } from '../use-session-actions/utils' import { applyBranchVisibility, @@ -601,9 +602,12 @@ export function usePromptActions({ if (isSessionNotFoundError(err) && selectedStoredSessionIdRef.current) { try { + const resumeProfile = await resolveSessionProfile(selectedStoredSessionIdRef.current) + const resumed = await requestGateway<{ session_id: string }>('session.resume', { session_id: selectedStoredSessionIdRef.current, - source: 'desktop' + source: 'desktop', + ...(resumeProfile ? { profile: resumeProfile } : {}) }) const recoveredId = resumed?.session_id @@ -700,9 +704,12 @@ export function usePromptActions({ // correction right after a reconnect isn't lost to the race. if (isSessionNotFoundError(err) && selectedStoredSessionIdRef.current) { try { + const resumeProfile = await resolveSessionProfile(selectedStoredSessionIdRef.current) + const resumed = await requestGateway<{ session_id: string }>('session.resume', { session_id: selectedStoredSessionIdRef.current, - source: 'desktop' + source: 'desktop', + ...(resumeProfile ? { profile: resumeProfile } : {}) }) const recoveredId = resumed?.session_id diff --git a/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts b/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts index deee5b0683ba..398a72f3be4a 100644 --- a/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts +++ b/apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts @@ -24,6 +24,7 @@ import { setAwaitingResponse, setBusy, setMessages } from '@/store/session' import type { ClientSessionState } from '../../../types' import { sessionContextDrift } from '../session-context-drift' +import { resolveSessionProfile } from '../use-session-actions/utils' import { _submitInFlight, @@ -384,9 +385,14 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) { // background queue drain only has the durable id). Continue that target // conversation; only a genuine new-chat draft may create a new session. try { + // Re-register on the session's OWNING profile — resuming on whichever + // profile is live would fork the conversation into the wrong DB (#67603). + const resumeProfile = await resolveSessionProfile(targetStoredSessionId) + const resumed = await requestGateway<{ session_id: string }>('session.resume', { session_id: targetStoredSessionId, - source: 'desktop' + source: 'desktop', + ...(resumeProfile ? { profile: resumeProfile } : {}) }) const resumeDrift = sessionDriftReason() @@ -528,9 +534,12 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) { // backend loop (#55578 symptom d) rejects the submit even though // the stored session is fine — resume + retry instead of erroring // out and losing the session binding. + const resumeProfile = await resolveSessionProfile(recoverStoredSessionId) + const resumed = await requestGateway<{ session_id: string }>('session.resume', { session_id: recoverStoredSessionId, - source: 'desktop' + source: 'desktop', + ...(resumeProfile ? { profile: resumeProfile } : {}) }) const resumeRetryDrift = sessionDriftReason() diff --git a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx index 5188a11f8d26..f0a4139f7990 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx @@ -3,7 +3,7 @@ import type { MutableRefObject } from 'react' import { useEffect } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' -import { getSessionMessages, type SessionInfo } from '@/hermes' +import { getSession, getSessionMessages, type SessionInfo } from '@/hermes' import { createClientSessionState } from '@/lib/chat-runtime' import { clearSessionDraft, stashSessionDraft, takeSessionDraft } from '@/store/composer' import { $activeGatewayProfile, $newChatProfile, ensureGatewayProfile } from '@/store/profile' @@ -43,6 +43,7 @@ import { useSessionActions } from './use-session-actions' vi.mock('@/hermes', async importOriginal => ({ ...(await importOriginal>()), deleteSession: vi.fn(), + getSession: vi.fn(), getSessionMessages: vi.fn(), listAllProfileSessions: vi.fn(), setApiRequestProfile: vi.fn(), @@ -1088,6 +1089,39 @@ describe('branchStoredSession desktop source tagging', () => { source: 'desktop' }) }) + + // #67603: right-clicking a session outside the paginated sidebar window is a + // cache miss. Resolve its owning profile (cache → active → cross-profile) and + // swap to it before reading the transcript / creating the branch, so the fork + // is not created on whichever profile happens to be live. + it('resolves and swaps to the parent profile when the branched session is not cached', async () => { + setSessions([]) + vi.mocked(getSession).mockResolvedValue(storedSession({ id: 'stored-parent', message_count: 1, profile: 'work' })) + vi.mocked(getSessionMessages).mockResolvedValue({ + messages: [{ content: 'branch me', role: 'user', timestamp: 1 }], + session_id: 'stored-parent' + } as never) + + const requestGateway = vi.fn(async (method: string) => { + if (method === 'session.create') { + return { session_id: 'branch-runtime', stored_session_id: 'branch-stored' } as never + } + + return {} as never + }) + + let branchStoredSession: ((storedSessionId: string, sessionProfile?: string | null) => Promise) | null = + null + render( (branchStoredSession = branch)} requestGateway={requestGateway} />) + await waitFor(() => expect(branchStoredSession).not.toBeNull()) + + await expect(branchStoredSession!('stored-parent')).resolves.toBe(true) + + expect(ensureGatewayProfile).toHaveBeenCalledWith('work') + expect(getSessionMessages).toHaveBeenCalledWith('stored-parent', 'work') + + vi.mocked(getSession).mockReset() + }) }) // ── Warm-cache mapping integrity (the "open chat A, chat B loads" bug) ───────── diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index 75aaf8cd6cab..a4ac8ec4ad72 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -1177,7 +1177,13 @@ export function useSessionActions({ async (storedSessionId: string, sessionProfile?: string | null): Promise => { clearNotifications() - const stored = $sessions.get().find(session => sessionMatchesStoredId(session, storedSessionId)) + // Right-clicking a session outside the paginated sidebar window is a cache + // miss: resolve it (cache → active backend → cross-profile) so the branch + // is created on the parent's OWNING profile, not whichever is live (#67603). + const stored = + $sessions.get().find(session => sessionMatchesStoredId(session, storedSessionId)) ?? + (sessionProfile ? undefined : await resolveStoredSession(storedSessionId)) + const profile = sessionProfile ?? stored?.profile try { diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts b/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts index d2fab21de6ca..2f0d7c05f5f9 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts @@ -361,6 +361,58 @@ describe('preserveLocalPendingTurnMessages', () => { expect(preserveLocalPendingTurnMessages(compressedAuthority, pollutedWarmCache)).toBe(compressedAuthority) }) + + // #67603: the gateway persists model-switch / personality notices as role=user + // ([System: …], tui_gateway/server.py). A single trailing marker is already + // handled by the latestAuthoritativeUser guard above, but TWO switches around + // one turn put a marker BEFORE the committed prompt (shifting its ordinal) and + // another AFTER it (so the prompt is no longer the last user row, so the text + // guard can't rescue it). Naive ordinal pairing then pairs the optimistic row + // against a marker, treats it as uncommitted, and re-appends it — the + // duplicated user bubble stacked at the bottom of the chat. + it('does not duplicate the optimistic prompt when markers bracket it (two model switches)', () => { + const marker = (name: string) => `[System: The active model for this chat has changed to ${name}.]` + + const previous = [ + msg('1-user', 'user', 'first'), + msg('2-assistant', 'assistant', 'first answer'), + msg('user-optimistic', 'user', 'second question') + ] + + const next = [ + msg('s1-user', 'user', 'first'), + msg('s2-assistant', 'assistant', 'first answer'), + msg('s3-marker', 'user', marker('k2')), + msg('s4-user', 'user', 'second question'), + msg('s5-assistant', 'assistant', 'second answer'), + msg('s6-marker', 'user', marker('k3')) + ] + + expect(preserveLocalPendingTurnMessages(next, previous)).toBe(next) + }) + + it('still keeps a genuinely uncommitted optimistic turn when a marker is present', () => { + const previous = [ + msg('1-user', 'user', 'first'), + msg('2-assistant', 'assistant', 'first answer'), + msg('user-optimistic', 'user', 'new question') + ] + + // The marker is persisted but the new prompt has not committed yet — the + // optimistic row must survive (marker exclusion must not over-correct). + const next = [ + msg('1-user-stored', 'user', 'first'), + msg('2-assistant-stored', 'assistant', 'first answer'), + msg('3-marker-stored', 'user', '[System: The active model for this chat has changed to k3.]') + ] + + expect(preserveLocalPendingTurnMessages(next, previous).map(message => message.id)).toEqual([ + '1-user-stored', + '2-assistant-stored', + '3-marker-stored', + 'user-optimistic' + ]) + }) }) describe('appendLiveSessionProjection', () => { diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts b/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts index f724a8f3e2cf..55781c3bcb7a 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts @@ -240,7 +240,17 @@ export function reconcileResumeMessages(nextMessages: ChatMessage[], previousMes * history, not in-flight work. The latest authoritative user confirms whether * that tail has persisted; any authoritative assistant at the same ordinal * supersedes the local stream. + * + * Gateway bookkeeping markers (the model-switch / personality notices written + * by tui_gateway/server.py) are persisted as role=user but are not user turns. + * They must not take part in ordinal pairing on either side: a stored marker + * between two real user turns shifts every later user ordinal, so the optimistic + * row misses its committed copy and is appended a second time at the end of the + * transcript — the duplicated user bubble of #67603. */ +const isGatewaySystemMarker = (message: ChatMessage): boolean => + message.role === 'user' && chatMessageText(message).trimStart().startsWith('[System:') + export function preserveLocalPendingTurnMessages( nextMessages: ChatMessage[], previousMessages: ChatMessage[] @@ -253,6 +263,10 @@ export function preserveLocalPendingTurnMessages( const nextRoleCounts = new Map() for (const message of nextMessages) { + if (isGatewaySystemMarker(message)) { + continue + } + const ordinal = nextRoleCounts.get(message.role) ?? 0 nextRoleCounts.set(message.role, ordinal + 1) nextByRoleOrdinal.set(`${message.role}:${ordinal}`, message) @@ -269,6 +283,10 @@ export function preserveLocalPendingTurnMessages( const preserved: ChatMessage[] = [] for (const message of previousMessages) { + if (isGatewaySystemMarker(message)) { + continue + } + const ordinal = previousRoleCounts.get(message.role) ?? 0 previousRoleCounts.set(message.role, ordinal + 1) @@ -497,6 +515,28 @@ export async function resolveStoredSession(storedSessionId: string): Promise { + if (!storedSessionId) { + return undefined + } + + const profile = (await resolveStoredSession(storedSessionId))?.profile?.trim() + + return profile || undefined +} + type SessionRuntimeStatePatch = Partial< Pick< ClientSessionState,