mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(desktop): stop model-switch dup + route recovery resumes to the owning profile
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>
This commit is contained in:
parent
dc861964fc
commit
5fccc9aae9
7 changed files with 249 additions and 6 deletions
|
|
@ -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<string, unknown> }[] = []
|
||||
let submitAttempts = 0
|
||||
|
||||
const requestGateway = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
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(
|
||||
<Harness
|
||||
onReady={h => (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<string, unknown> }[] = []
|
||||
let submitAttempts = 0
|
||||
|
||||
const requestGateway = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
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(
|
||||
<Harness
|
||||
onReady={h => (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<string, unknown> }[] = []
|
||||
let submitAttempts = 0
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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<Record<string, unknown>>()),
|
||||
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<boolean>) | null =
|
||||
null
|
||||
render(<BranchHarness onReady={branch => (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) ─────────
|
||||
|
|
|
|||
|
|
@ -1177,7 +1177,13 @@ export function useSessionActions({
|
|||
async (storedSessionId: string, sessionProfile?: string | null): Promise<boolean> => {
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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<ChatMessage['role'], number>()
|
||||
|
||||
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<Ses
|
|||
return undefined
|
||||
}
|
||||
|
||||
/**
|
||||
* The profile that owns a stored session, resolved through the same
|
||||
* cache → active-backend → cross-profile ladder as `resolveStoredSession`.
|
||||
*
|
||||
* Recovery `session.resume` calls (stale runtime id, session-not-found, wedged
|
||||
* loop) must re-register the conversation on ITS backend, not on whichever
|
||||
* profile happens to be live. Omitting the profile lets the gateway fall back to
|
||||
* the launch-profile DB (tui_gateway/server.py), which is how a session bleeds
|
||||
* from one profile into another (#67603, second symptom). A cache-only lookup
|
||||
* misses any session outside the paginated sidebar window, so route through the
|
||||
* resolver, which probes uncached ids across profiles.
|
||||
*/
|
||||
export async function resolveSessionProfile(storedSessionId: null | string): Promise<string | undefined> {
|
||||
if (!storedSessionId) {
|
||||
return undefined
|
||||
}
|
||||
|
||||
const profile = (await resolveStoredSession(storedSessionId))?.profile?.trim()
|
||||
|
||||
return profile || undefined
|
||||
}
|
||||
|
||||
type SessionRuntimeStatePatch = Partial<
|
||||
Pick<
|
||||
ClientSessionState,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue