mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-25 17:18:11 +00:00
fix(desktop): close cross-session leak windows in composer + session refs (#59305)
Two React passive-effect timing bugs let a session switch land in the wrong chat: activeSessionIdRef/selectedStoredSessionIdRef (use-session-state-cache) and the composer's attachment-scope swap (use-composer-draft) both mirrored their source props via useEffect, which fires one commit AFTER the new session's view has already painted — a synchronous read/submit in that window observed the outgoing session's ids/attachments. - use-session-state-cache.ts: mirror the session refs synchronously during render instead of a useEffect, guarded to fire only when the prop itself changed (not unconditionally) so an imperative pin from submit.ts / use-session-actions (e.g. a freshly resumed runtime id, intentionally not synced to the source atom) survives an unrelated re-render. - use-composer-draft.ts: the per-thread attachment-scope-swap effect is now a useLayoutEffect, closing the window before paint. - submit.ts / session-context-drift.ts: add a 3rd drift prong comparing the composer's loaded scope (SubmitTextOptions.composerScope) against the submit target, resolved into the same lineage-root domain (resolveComposerSessionKey) the composer itself uses — comparing against the raw tip id would false-positive-abort every submit into any session that has ever auto-compressed. - routes.ts / chat/index.tsx: the primary composer's durable scope key now prefers the route over a possibly-stale store selection (primaryRouteSelectedSessionId). - use-composer-draft.ts: redacted [composer-rehydrate] diagnostic log (counts/kinds/scope only, never raw refs) for future reports in this class. - chat-runtime.ts: normalize attachment id values (url/path) before hashing so a re-attach with a trailing slash or backslash path dedupes correctly. 16 files, 286 tests across the touched/dependent suites (17 files) green, including new regression coverage for each fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
86084d03ba
commit
d083d9dacd
16 changed files with 650 additions and 27 deletions
|
|
@ -0,0 +1,128 @@
|
|||
import { act, cleanup, render } from '@testing-library/react'
|
||||
import { useLayoutEffect } from 'react'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { mainComposerScope, stashSessionDraft, type ComposerAttachment } from '@/store/composer'
|
||||
|
||||
import type { QueueEditState } from '../composer-utils'
|
||||
import { useComposerDraft } from './use-composer-draft'
|
||||
|
||||
const mockComposerApi = { setText: vi.fn() }
|
||||
|
||||
vi.mock('@assistant-ui/react', () => ({
|
||||
useAui: () => ({ composer: () => mockComposerApi }),
|
||||
useAuiState: (selector: (state: { composer: { text: string } }) => unknown) =>
|
||||
selector({ composer: { text: '' } }),
|
||||
useComposerRuntime: () => ({
|
||||
getState: () => ({ text: '' }),
|
||||
subscribe: () => () => undefined
|
||||
})
|
||||
}))
|
||||
|
||||
interface ProbeHarnessProps {
|
||||
activeQueueSessionKey: string | null
|
||||
onLayoutSnapshot: (attachments: ComposerAttachment[]) => void
|
||||
sessionId: string
|
||||
}
|
||||
|
||||
function ProbeHarness({ activeQueueSessionKey, onLayoutSnapshot, sessionId }: ProbeHarnessProps) {
|
||||
useComposerDraft({
|
||||
activeQueueSessionKey,
|
||||
focusKey: null,
|
||||
inputDisabled: false,
|
||||
queueEditRef: { current: null as QueueEditState | null },
|
||||
sessionId
|
||||
})
|
||||
|
||||
// useLayoutEffect fires synchronously right after the DOM commit, BEFORE
|
||||
// the hook's per-thread scope-swap useEffect (a passive effect) has a
|
||||
// chance to swap attachmentScope.$attachments over to the new session. A
|
||||
// synchronous read here — the same read ChatBar's `attachments` prop
|
||||
// performs at render time — observes the OUTGOING session's attachments.
|
||||
useLayoutEffect(() => {
|
||||
onLayoutSnapshot(mainComposerScope.$attachments.get())
|
||||
})
|
||||
|
||||
return null
|
||||
}
|
||||
|
||||
describe('useComposerDraft — attachment scope stays coherent with the committed session on switch (#59305)', () => {
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
mainComposerScope.clear()
|
||||
})
|
||||
|
||||
it('clears the outgoing session attachments by the layout phase right after switching sessions', () => {
|
||||
const attachmentA: ComposerAttachment = { id: 'url-A', kind: 'url', label: 'A' }
|
||||
stashSessionDraft('session-A', 'hi from A', [attachmentA])
|
||||
|
||||
const snapshots: ComposerAttachment[][] = []
|
||||
|
||||
const { rerender } = render(
|
||||
<ProbeHarness
|
||||
activeQueueSessionKey="session-A"
|
||||
onLayoutSnapshot={s => snapshots.push(s)}
|
||||
sessionId="session-A"
|
||||
/>
|
||||
)
|
||||
|
||||
// Mount loads session A's stashed attachment into the (module-level) main
|
||||
// scope — confirms the fixture actually seeded the leak precondition.
|
||||
expect(mainComposerScope.$attachments.get()).toEqual([attachmentA])
|
||||
|
||||
snapshots.length = 0 // drop the initial-mount snapshot; only the switch matters
|
||||
|
||||
act(() => {
|
||||
rerender(
|
||||
<ProbeHarness
|
||||
activeQueueSessionKey="session-B"
|
||||
onLayoutSnapshot={s => snapshots.push(s)}
|
||||
sessionId="session-B"
|
||||
/>
|
||||
)
|
||||
})
|
||||
|
||||
// By the layout phase the scope must already be B's (empty) — a submit
|
||||
// fired the instant B renders must never ship session A's attachment.
|
||||
expect(snapshots[0]).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('useComposerDraft — rehydrate diagnostic log stays redacted', () => {
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
mainComposerScope.clear()
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('logs counts/kinds/scope on restore but never the raw url, refText, or label', () => {
|
||||
const secretUrl = 'https://secret.example.com/private-workspace-path'
|
||||
const attachment: ComposerAttachment = {
|
||||
id: 'url-secret',
|
||||
kind: 'url',
|
||||
label: 'do-not-leak-label',
|
||||
refText: `@url:${secretUrl}`
|
||||
}
|
||||
stashSessionDraft('session-secret', '', [attachment])
|
||||
|
||||
const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => undefined)
|
||||
|
||||
render(
|
||||
<ProbeHarness activeQueueSessionKey="session-secret" onLayoutSnapshot={() => undefined} sessionId="session-secret" />
|
||||
)
|
||||
|
||||
const rehydrateCalls = debugSpy.mock.calls.filter(call => call[0] === '[composer-rehydrate]')
|
||||
expect(rehydrateCalls.length).toBeGreaterThan(0)
|
||||
|
||||
const serialized = JSON.stringify(rehydrateCalls)
|
||||
expect(serialized).not.toContain(secretUrl)
|
||||
expect(serialized).not.toContain(attachment.label)
|
||||
expect(serialized).not.toContain(attachment.refText)
|
||||
|
||||
expect(rehydrateCalls[0]?.[1]).toMatchObject({
|
||||
attachmentCount: 1,
|
||||
attachmentKinds: ['url'],
|
||||
scope: 'session-secret'
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -1,5 +1,5 @@
|
|||
import { useAui, useAuiState, useComposerRuntime } from '@assistant-ui/react'
|
||||
import { type RefObject, useCallback, useEffect, useRef, useState } from 'react'
|
||||
import { type RefObject, useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react'
|
||||
|
||||
import { SLASH_COMMAND_RE } from '@/lib/chat-runtime'
|
||||
import { type ComposerAttachment, stashSessionDraft, takeSessionDraft } from '@/store/composer'
|
||||
|
|
@ -20,7 +20,7 @@ import {
|
|||
onComposerInsertRequest
|
||||
} from '../focus'
|
||||
import { type InlineRefInput, insertInlineRefsIntoEditor } from '../inline-refs'
|
||||
import { composerPlainText, placeCaretEnd, renderComposerContents } from '../rich-editor'
|
||||
import { composerPlainText, placeCaretEnd, REF_RE, renderComposerContents } from '../rich-editor'
|
||||
import { useComposerScope } from '../scope'
|
||||
import type { ChatBarProps } from '../types'
|
||||
|
||||
|
|
@ -189,6 +189,21 @@ export function useComposerDraft({
|
|||
stashSessionDraft(scope, text, attachments)
|
||||
|
||||
const loadIntoComposer = (text: string, attachments: ComposerAttachment[]) => {
|
||||
// Diagnostic breadcrumb for #59305-class reports: identifies WHAT kind of
|
||||
// state got restored into the composer (session switch, queue-edit
|
||||
// restore, history browse) without logging any raw content. REF_RE has the
|
||||
// global flag — testing against a throwaway clone avoids mutating the
|
||||
// shared instance's lastIndex, which would otherwise corrupt this check on
|
||||
// the next call.
|
||||
if (attachments.length > 0 || new RegExp(REF_RE.source, REF_RE.flags).test(text)) {
|
||||
console.debug('[composer-rehydrate]', {
|
||||
attachmentCount: attachments.length,
|
||||
attachmentKinds: attachments.map(a => a.kind),
|
||||
hasTextRefs: new RegExp(REF_RE.source, REF_RE.flags).test(text),
|
||||
scope: activeQueueSessionKeyRef.current
|
||||
})
|
||||
}
|
||||
|
||||
attachmentScope.$attachments.set(cloneAttachments(attachments))
|
||||
paintDraft(text, false)
|
||||
}
|
||||
|
|
@ -318,7 +333,16 @@ export function useComposerDraft({
|
|||
// Per-thread draft swap — the composer's only session coupling. Lifecycle
|
||||
// never clears composer state; this effect alone stashes on leave, restores
|
||||
// on enter. Keyed writes are idempotent, so no skip-sentinel.
|
||||
useEffect(() => {
|
||||
//
|
||||
// MUST be a layout effect, not a passive one: it swaps attachmentScope's
|
||||
// module-level $attachments atom, and a passive effect fires only after the
|
||||
// browser paints the new session's view — leaving a window where the DOM
|
||||
// already shows session B while $attachments (and therefore ChatBar's
|
||||
// `attachments` prop) still holds session A's chips. A submit fired in that
|
||||
// window (e.g. a fast session switch immediately followed by Enter) would
|
||||
// ship A's attachments into B's turn (#59305). useLayoutEffect closes the
|
||||
// window by running before paint.
|
||||
useLayoutEffect(() => {
|
||||
// A pending debounce timer from the outgoing session is now stale — its
|
||||
// scope was correct when scheduled, but the authoritative stash below
|
||||
// (and the cleanup on the way out) already covers that text. Letting it
|
||||
|
|
|
|||
|
|
@ -113,7 +113,9 @@ describe('useComposerSubmit busy-turn routing', () => {
|
|||
hook.result.current.submitDraft()
|
||||
})
|
||||
|
||||
await waitFor(() => expect(onSubmit).toHaveBeenCalledWith('/compress preserve context'))
|
||||
await waitFor(() =>
|
||||
expect(onSubmit).toHaveBeenCalledWith('/compress preserve context', { composerScope: 'stored-session' })
|
||||
)
|
||||
expect(clearDraft).toHaveBeenCalledTimes(1)
|
||||
expect(onSteer).not.toHaveBeenCalled()
|
||||
expect(queueCurrentDraft).not.toHaveBeenCalled()
|
||||
|
|
@ -159,9 +161,26 @@ describe('useComposerSubmit busy-turn routing', () => {
|
|||
hook.result.current.submitDraft()
|
||||
})
|
||||
|
||||
await waitFor(() => expect(onSubmit).toHaveBeenCalledWith('ordinary question', { attachments: [] }))
|
||||
await waitFor(() =>
|
||||
expect(onSubmit).toHaveBeenCalledWith('ordinary question', {
|
||||
attachments: [],
|
||||
composerScope: 'stored-session'
|
||||
})
|
||||
)
|
||||
expect(onSteer).not.toHaveBeenCalled()
|
||||
expect(queueCurrentDraft).not.toHaveBeenCalled()
|
||||
expect(onCancel).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('threads the loaded composer scope through onSubmit for the #59305 submit-time guard', async () => {
|
||||
const { hook, onSubmit } = renderSubmitHook({ text: 'hello' })
|
||||
|
||||
act(() => {
|
||||
hook.result.current.submitDraft()
|
||||
})
|
||||
|
||||
await waitFor(() =>
|
||||
expect(onSubmit).toHaveBeenCalledWith('hello', expect.objectContaining({ composerScope: 'stored-session' }))
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -89,7 +89,11 @@ export function useComposerSubmit({
|
|||
stashAt(submittedScope, text, submittedAttachments)
|
||||
}
|
||||
|
||||
void Promise.resolve(attachments ? onSubmit(text, { attachments }) : onSubmit(text))
|
||||
void Promise.resolve(
|
||||
attachments
|
||||
? onSubmit(text, { attachments, composerScope: submittedScope })
|
||||
: onSubmit(text, { composerScope: submittedScope })
|
||||
)
|
||||
.then(accepted => void (accepted === false ? restore() : clearSessionDraft(submittedScope)))
|
||||
.catch(restore)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -42,7 +42,7 @@ import {
|
|||
import { isSecondaryWindow, isWatchWindow } from '@/store/windows'
|
||||
import type { ModelOptionsResponse } from '@/types/hermes'
|
||||
|
||||
import { routeSessionId } from '../routes'
|
||||
import { primaryRouteSelectedSessionId, routeSessionId } from '../routes'
|
||||
import { titlebarHeaderBaseClass, titlebarHeaderShadowClass, titlebarHeaderTitleClass } from '../shell/titlebar'
|
||||
|
||||
import { ChatDropOverlay } from './chat-drop-overlay'
|
||||
|
|
@ -285,11 +285,18 @@ export function ChatView({
|
|||
const resumeExhaustedSessionId = useStore($resumeExhaustedSessionId)
|
||||
|
||||
// Durable composer/queue scope (lineage root) so auto-compression tip rotation
|
||||
// does not wipe an in-progress draft or orphan /queue entries.
|
||||
const queueSessionKey = useMemo(
|
||||
() => resolveComposerSessionKey(selectedSessionId, sessions),
|
||||
[selectedSessionId, sessions]
|
||||
)
|
||||
// does not wipe an in-progress draft or orphan /queue entries. For the
|
||||
// primary view, the route is authoritative over the store selection — the
|
||||
// latter can be momentarily null/stale mid-switch, which used to leak into
|
||||
// the composer's scope key (#59305). A tile has no route, so it always uses
|
||||
// its own selection directly.
|
||||
const queueSessionKey = useMemo(() => {
|
||||
const effectiveSelectedSessionId = isPrimary
|
||||
? primaryRouteSelectedSessionId(location.pathname, selectedSessionId)
|
||||
: selectedSessionId
|
||||
|
||||
return resolveComposerSessionKey(effectiveSelectedSessionId, sessions)
|
||||
}, [isPrimary, location.pathname, selectedSessionId, sessions])
|
||||
|
||||
// When the tip row arrives after compression, migrate any tip-keyed stash onto
|
||||
// the durable lineage key before the composer remounts onto that key.
|
||||
|
|
|
|||
30
apps/desktop/src/app/routes.test.ts
Normal file
30
apps/desktop/src/app/routes.test.ts
Normal file
|
|
@ -0,0 +1,30 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { NEW_CHAT_ROUTE, primaryRouteSelectedSessionId, sessionRoute, SETTINGS_ROUTE } from './routes'
|
||||
|
||||
const SESS_A = 'sess-a'
|
||||
const SESS_B = 'sess-b'
|
||||
|
||||
describe('primaryRouteSelectedSessionId', () => {
|
||||
it('prefers the routed session id over a stale/different store selection (#59305)', () => {
|
||||
// The route already committed to B while the store selection hasn't
|
||||
// caught up yet (still reads A) — the route wins.
|
||||
expect(primaryRouteSelectedSessionId(sessionRoute(SESS_B), SESS_A)).toBe(SESS_B)
|
||||
})
|
||||
|
||||
it('returns null on the new-chat route even with a leftover selection from the previous chat', () => {
|
||||
expect(primaryRouteSelectedSessionId(NEW_CHAT_ROUTE, SESS_A)).toBeNull()
|
||||
})
|
||||
|
||||
it('falls back to the store selection on a non-chat route (settings, overlays)', () => {
|
||||
expect(primaryRouteSelectedSessionId(SETTINGS_ROUTE, SESS_A)).toBe(SESS_A)
|
||||
})
|
||||
|
||||
it('falls back to the store selection when the route matches the same session', () => {
|
||||
expect(primaryRouteSelectedSessionId(sessionRoute(SESS_A), SESS_A)).toBe(SESS_A)
|
||||
})
|
||||
|
||||
it('returns null on a non-chat route with no store selection', () => {
|
||||
expect(primaryRouteSelectedSessionId(SETTINGS_ROUTE, null)).toBeNull()
|
||||
})
|
||||
})
|
||||
|
|
@ -147,6 +147,23 @@ export function routeSessionId(pathname: string): string | null {
|
|||
return id && !id.includes('/') ? decodeURIComponent(id) : null
|
||||
}
|
||||
|
||||
/**
|
||||
* The primary composer's durable scope key candidate: the route is the source
|
||||
* of truth for which chat is on screen, so prefer its (stable) stored session
|
||||
* id over a store selection that can be momentarily null/stale mid-switch
|
||||
* (#59305). A genuine new-chat route always wins with `null`, never falling
|
||||
* back to a leftover selection from the chat just left. A non-chat route
|
||||
* (settings, an overlay) has no session opinion, so the store selection passes
|
||||
* through unchanged.
|
||||
*/
|
||||
export function primaryRouteSelectedSessionId(pathname: string, storeSelectedSessionId: string | null): string | null {
|
||||
if (isNewChatRoute(pathname)) {
|
||||
return null
|
||||
}
|
||||
|
||||
return routeSessionId(pathname) ?? storeSelectedSessionId
|
||||
}
|
||||
|
||||
export function sessionRoute(sessionId: string): string {
|
||||
return `${SESSION_ROUTE_PREFIX}${encodeURIComponent(sessionId)}`
|
||||
}
|
||||
|
|
|
|||
|
|
@ -122,4 +122,77 @@ describe('sessionContextDrift', () => {
|
|||
|
||||
expect(reason).toBe('route:__new__->sess-b')
|
||||
})
|
||||
|
||||
it('does not drift when composerScope is omitted (non-composer submits: queue drain, steer)', () => {
|
||||
const reason = sessionContextDrift({
|
||||
startRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
nowRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
startSelectedStoredId: SESS_A,
|
||||
nowSelectedStoredId: SESS_A,
|
||||
submitTargetStoredId: SESS_A
|
||||
})
|
||||
|
||||
expect(reason).toBeNull()
|
||||
})
|
||||
|
||||
it('does not drift when composerScope matches the resolved (lineage) submit target', () => {
|
||||
const reason = sessionContextDrift({
|
||||
startRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
nowRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
startSelectedStoredId: SESS_A,
|
||||
nowSelectedStoredId: SESS_A,
|
||||
submitTargetStoredId: SESS_A,
|
||||
composerScope: SESS_A,
|
||||
submitTargetComposerScope: SESS_A
|
||||
})
|
||||
|
||||
expect(reason).toBeNull()
|
||||
})
|
||||
|
||||
it('drifts (composer prong) when the loaded composer scope disagrees with the resolved submit target (#59305)', () => {
|
||||
const reason = sessionContextDrift({
|
||||
startRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
nowRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
startSelectedStoredId: SESS_A,
|
||||
nowSelectedStoredId: SESS_A,
|
||||
submitTargetStoredId: SESS_A,
|
||||
composerScope: SESS_B,
|
||||
submitTargetComposerScope: SESS_A
|
||||
})
|
||||
|
||||
expect(reason).toBe('composer:sess-b->sess-a')
|
||||
})
|
||||
|
||||
it('checks the composer prong before the route/selection prongs', () => {
|
||||
const reason = sessionContextDrift({
|
||||
startRouteToken: routeToken(sessionRoute(SESS_A)),
|
||||
nowRouteToken: routeToken(sessionRoute(SESS_B)),
|
||||
startSelectedStoredId: SESS_A,
|
||||
nowSelectedStoredId: SESS_B,
|
||||
submitTargetStoredId: SESS_A,
|
||||
composerScope: SESS_B,
|
||||
submitTargetComposerScope: SESS_A
|
||||
})
|
||||
|
||||
expect(reason).toBe('composer:sess-b->sess-a')
|
||||
})
|
||||
|
||||
it('does not drift when the session has rotated via compression (composerScope is the lineage root, submitTargetStoredId is the live tip)', () => {
|
||||
const ROOT_ID = 'stored-root'
|
||||
const TIP_ID = 'stored-tip-after-compression'
|
||||
|
||||
const reason = sessionContextDrift({
|
||||
startRouteToken: routeToken(sessionRoute(TIP_ID)),
|
||||
nowRouteToken: routeToken(sessionRoute(TIP_ID)),
|
||||
startSelectedStoredId: TIP_ID,
|
||||
nowSelectedStoredId: TIP_ID,
|
||||
submitTargetStoredId: TIP_ID,
|
||||
composerScope: ROOT_ID,
|
||||
// What submit.ts actually passes: resolveComposerSessionKey(TIP_ID, sessions),
|
||||
// which resolves to the lineage root for a session that has compressed.
|
||||
submitTargetComposerScope: ROOT_ID
|
||||
})
|
||||
|
||||
expect(reason).toBeNull()
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -37,6 +37,25 @@ interface SessionContextDriftArgs {
|
|||
* a real chat as drift.
|
||||
*/
|
||||
submitTargetStoredId?: string | null
|
||||
/**
|
||||
* The composer scope that was actually loaded when the text was submitted
|
||||
* (SubmitTextOptions.composerScope). The composer and the session-side refs
|
||||
* live in separate React subtrees and can each be internally consistent yet
|
||||
* still disagree with each other at the instant of send — this prong catches
|
||||
* that cross-component drift (#59305). Omit for non-composer submits.
|
||||
*/
|
||||
composerScope?: string | null
|
||||
/**
|
||||
* resolveComposerSessionKey(submitTargetStoredId, sessions) — the durable
|
||||
* lineage-root form of the submit target, in the SAME domain as
|
||||
* composerScope. Compared against composerScope instead of the raw
|
||||
* submitTargetStoredId: the composer keys drafts/attachments on the lineage
|
||||
* root (stable across auto-compression tip rotation) while
|
||||
* submitTargetStoredId tracks the live tip — comparing composerScope
|
||||
* directly against the tip would false-positive-abort every submit into any
|
||||
* session that has ever compressed.
|
||||
*/
|
||||
submitTargetComposerScope?: string | null
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -56,8 +75,21 @@ export function sessionContextDrift({
|
|||
nowRouteToken,
|
||||
startSelectedStoredId,
|
||||
nowSelectedStoredId,
|
||||
submitTargetStoredId
|
||||
submitTargetStoredId,
|
||||
composerScope,
|
||||
submitTargetComposerScope
|
||||
}: SessionContextDriftArgs): string | null {
|
||||
// Composer prong: the composer's loaded scope disagrees with the resolved
|
||||
// submit target. Not a start/now comparison like the two prongs below — the
|
||||
// composer only hands us one snapshot per submit — but it belongs in the
|
||||
// same fail-closed gate since it's exactly the same "wrong session" failure
|
||||
// mode. Compared against submitTargetComposerScope (lineage-pinned), NOT
|
||||
// submitTargetStoredId (live tip) — see the field doc on
|
||||
// SessionContextDriftArgs for why those two must not be conflated.
|
||||
if (composerScope !== undefined && composerScope !== null && composerScope !== submitTargetComposerScope) {
|
||||
return `composer:${composerScope}->${submitTargetComposerScope}`
|
||||
}
|
||||
|
||||
const targetStart = routeTargetFromToken(startRouteToken)
|
||||
const targetNow = routeTargetFromToken(nowRouteToken)
|
||||
|
||||
|
|
|
|||
|
|
@ -2458,6 +2458,7 @@ describe('usePromptActions submit session-context isolation (#54527)', () => {
|
|||
afterEach(() => {
|
||||
cleanup()
|
||||
vi.restoreAllMocks()
|
||||
setSessions(() => [])
|
||||
})
|
||||
|
||||
it('aborts submit when the user switches sessions during session.resume (no misroute)', async () => {
|
||||
|
|
@ -2515,6 +2516,100 @@ describe('usePromptActions submit session-context isolation (#54527)', () => {
|
|||
})
|
||||
})
|
||||
|
||||
it('does not false-positive-abort when the session has rotated via compression (lineage root vs tip)', async () => {
|
||||
// The composer keys drafts/attachments on the DURABLE lineage root
|
||||
// (resolveComposerSessionKey / sessionPinId — survives auto-compression
|
||||
// tip rotation), but selectedStoredSessionIdRef tracks the CURRENT TIP.
|
||||
// For any session that has compressed at least once, root !== tip — if
|
||||
// composerScope is compared against the raw tip, every legitimate submit
|
||||
// into that session would look like drift.
|
||||
const ROOT_ID = 'stored-root-original'
|
||||
const TIP_ID = 'stored-tip-after-compression'
|
||||
|
||||
setSessions(() => [sessionInfo({ id: TIP_ID, _lineage_root_id: ROOT_ID })])
|
||||
|
||||
const calls: { method: string; params?: Record<string, unknown> }[] = []
|
||||
|
||||
const requestGateway = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
calls.push({ method, params })
|
||||
|
||||
return {} as never
|
||||
})
|
||||
|
||||
let handle: HarnessHandle | null = null
|
||||
await actRender(
|
||||
<Harness
|
||||
onReady={h => (handle = h)}
|
||||
refreshSessions={async () => undefined}
|
||||
requestGateway={requestGateway}
|
||||
storedSessionId={TIP_ID}
|
||||
/>
|
||||
)
|
||||
|
||||
// The composer's scope is the lineage root (what resolveComposerSessionKey
|
||||
// actually returns for this session) — a legitimate, non-drifted submit.
|
||||
const ok = await handle!.submitText('message into the rotated session', { composerScope: ROOT_ID })
|
||||
|
||||
expect(ok).toBe(true)
|
||||
expect(calls.some(c => c.method === 'prompt.submit')).toBe(true)
|
||||
})
|
||||
|
||||
it('aborts submit when the composer scope disagrees with the resolved target (#59305)', async () => {
|
||||
// The composer (ChatBar) and the session-side refs live in separate React
|
||||
// subtrees; each can be internally consistent yet still disagree with each
|
||||
// other at the instant of send if the two updated on different commits.
|
||||
// composerScope carries the composer's own snapshot of "what session was
|
||||
// loaded" into submit.ts, which must refuse to send when it disagrees with
|
||||
// the session the submit is actually about to target.
|
||||
const calls: { method: string; params?: Record<string, unknown> }[] = []
|
||||
|
||||
const requestGateway = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
calls.push({ method, params })
|
||||
|
||||
return {} as never
|
||||
})
|
||||
|
||||
let handle: HarnessHandle | null = null
|
||||
await actRender(
|
||||
<Harness
|
||||
onReady={h => (handle = h)}
|
||||
refreshSessions={async () => undefined}
|
||||
requestGateway={requestGateway}
|
||||
storedSessionId={STORED_SESSION_A}
|
||||
/>
|
||||
)
|
||||
|
||||
const ok = await handle!.submitText('typed while B was on screen', { composerScope: STORED_SESSION_B })
|
||||
|
||||
expect(ok).toBe(false)
|
||||
expect(calls.some(c => c.method === 'prompt.submit')).toBe(false)
|
||||
})
|
||||
|
||||
it('submits normally when the composer scope agrees with the resolved target', async () => {
|
||||
const calls: { method: string; params?: Record<string, unknown> }[] = []
|
||||
|
||||
const requestGateway = vi.fn(async (method: string, params?: Record<string, unknown>) => {
|
||||
calls.push({ method, params })
|
||||
|
||||
return {} as never
|
||||
})
|
||||
|
||||
let handle: HarnessHandle | null = null
|
||||
await actRender(
|
||||
<Harness
|
||||
onReady={h => (handle = h)}
|
||||
refreshSessions={async () => undefined}
|
||||
requestGateway={requestGateway}
|
||||
storedSessionId={STORED_SESSION_A}
|
||||
/>
|
||||
)
|
||||
|
||||
const ok = await handle!.submitText('typed while A was on screen', { composerScope: STORED_SESSION_A })
|
||||
|
||||
expect(ok).toBe(true)
|
||||
expect(calls.some(c => c.method === 'prompt.submit')).toBe(true)
|
||||
})
|
||||
|
||||
it('aborts recovery submit when the user switches sessions during timeout resume', async () => {
|
||||
const calls: { method: string; params?: Record<string, unknown> }[] = []
|
||||
let submitAttempts = 0
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ import {
|
|||
} from '@/store/composer'
|
||||
import { clearNotifications, notify, notifyError } from '@/store/notifications'
|
||||
import { requestDesktopOnboarding } from '@/store/onboarding'
|
||||
import { setAwaitingResponse, setBusy, setMessages } from '@/store/session'
|
||||
import { $sessions, resolveComposerSessionKey, setAwaitingResponse, setBusy, setMessages } from '@/store/session'
|
||||
|
||||
import type { ClientSessionState } from '../../../types'
|
||||
import { sessionContextDrift } from '../session-context-drift'
|
||||
|
|
@ -210,7 +210,15 @@ export function useSubmitPrompt(deps: SubmitPromptDeps) {
|
|||
nowRouteToken: getRouteToken(),
|
||||
startSelectedStoredId: startingStoredSessionId,
|
||||
nowSelectedStoredId: selectedStoredSessionIdRef.current,
|
||||
submitTargetStoredId: startingStoredSessionId
|
||||
submitTargetStoredId: startingStoredSessionId,
|
||||
composerScope: options?.composerScope,
|
||||
// The composer keys drafts/attachments on the durable lineage
|
||||
// root (survives auto-compression tip rotation), while
|
||||
// startingStoredSessionId is the live tip — resolve the target
|
||||
// into the same lineage-root domain before comparing, or every
|
||||
// submit into a session that has ever compressed would
|
||||
// false-positive-abort.
|
||||
submitTargetComposerScope: resolveComposerSessionKey(startingStoredSessionId, $sessions.get())
|
||||
})
|
||||
: null
|
||||
|
||||
|
|
|
|||
|
|
@ -359,6 +359,14 @@ export function visibleUserIndexAtOrdinal(messages: readonly ChatMessage[], targ
|
|||
|
||||
export interface SubmitTextOptions {
|
||||
attachments?: ComposerAttachment[]
|
||||
/** The composer scope key that was actually loaded when this text was
|
||||
* submitted (see use-composer-draft's activeQueueSessionKeyRef). Compared
|
||||
* against the resolved submit target in sessionContextDrift — a mismatch
|
||||
* means the composer and the session-side refs disagreed about which
|
||||
* session this send belongs to (#59305). Omit for non-composer submits
|
||||
* (queue drain, steer, external submit requests): the check is a no-op
|
||||
* without it. */
|
||||
composerScope?: string | null
|
||||
fromQueue?: boolean
|
||||
/** Runtime session id to submit into. Queue drains pass this so a
|
||||
* backgrounded/source session cannot be replaced by the current foreground
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { act, cleanup, render } from '@testing-library/react'
|
||||
import type { MutableRefObject } from 'react'
|
||||
import { type MutableRefObject, useLayoutEffect } from 'react'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { ChatMessage } from '@/lib/chat-messages'
|
||||
|
|
@ -260,6 +260,111 @@ describe('useSessionStateCache — per-session turn timer', () => {
|
|||
})
|
||||
})
|
||||
|
||||
interface LayoutProbeHarnessProps {
|
||||
activeSessionId: string | null
|
||||
onLayoutSnapshot: (snapshot: { active: string | null; selected: string | null }) => void
|
||||
onReady: (cache: Cache) => void
|
||||
selectedStoredSessionId: string | null
|
||||
}
|
||||
|
||||
function LayoutProbeHarness({
|
||||
activeSessionId,
|
||||
onLayoutSnapshot,
|
||||
onReady,
|
||||
selectedStoredSessionId
|
||||
}: LayoutProbeHarnessProps) {
|
||||
const busyRef: MutableRefObject<boolean> = { current: false }
|
||||
|
||||
const cache = useSessionStateCache({
|
||||
activeSessionId,
|
||||
busyRef,
|
||||
selectedStoredSessionId,
|
||||
setAwaitingResponse: () => undefined,
|
||||
setBusy: () => undefined,
|
||||
setMessages: () => undefined
|
||||
})
|
||||
|
||||
onReady(cache)
|
||||
|
||||
// useLayoutEffect fires synchronously right after the DOM commit, BEFORE
|
||||
// the hook's own useEffect (a passive effect) has a chance to mirror the
|
||||
// new props into activeSessionIdRef/selectedStoredSessionIdRef. Anything
|
||||
// that reads the refs in this window — including a synchronous DOM event
|
||||
// handler firing against the just-committed view — observes the outgoing
|
||||
// session's ids.
|
||||
useLayoutEffect(() => {
|
||||
onLayoutSnapshot({
|
||||
active: cache.activeSessionIdRef.current,
|
||||
selected: cache.selectedStoredSessionIdRef.current
|
||||
})
|
||||
})
|
||||
|
||||
return null
|
||||
}
|
||||
|
||||
describe('useSessionStateCache — refs stay coherent with the committed session on switch (#59305)', () => {
|
||||
afterEach(() => cleanup())
|
||||
|
||||
it('reflects the new session ids from the layout phase right after switching to a new session', () => {
|
||||
let cache!: Cache
|
||||
const snapshots: Array<{ active: string | null; selected: string | null }> = []
|
||||
|
||||
const { rerender } = render(
|
||||
<LayoutProbeHarness
|
||||
activeSessionId="runtime-A"
|
||||
onLayoutSnapshot={s => snapshots.push(s)}
|
||||
onReady={c => (cache = c)}
|
||||
selectedStoredSessionId="stored-A"
|
||||
/>
|
||||
)
|
||||
|
||||
void cache
|
||||
snapshots.length = 0 // drop the initial-mount snapshot; only the switch matters
|
||||
|
||||
rerender(
|
||||
<LayoutProbeHarness
|
||||
activeSessionId="runtime-B"
|
||||
onLayoutSnapshot={s => snapshots.push(s)}
|
||||
onReady={c => (cache = c)}
|
||||
selectedStoredSessionId="stored-B"
|
||||
/>
|
||||
)
|
||||
|
||||
// The refs must already reflect B by the layout phase — a callback firing
|
||||
// in this window must never observe the outgoing session's ids.
|
||||
expect(snapshots[0]).toEqual({ active: 'runtime-B', selected: 'stored-B' })
|
||||
})
|
||||
|
||||
it('does not clobber an imperative ref pin on a re-render that leaves the props unchanged (#54527-class)', () => {
|
||||
// submit.ts pins activeSessionIdRef.current to a freshly resumed runtime id
|
||||
// WITHOUT updating the source atom that feeds the activeSessionId prop (by
|
||||
// design — see submit.ts's "pin the foreground session context" comment).
|
||||
// The prop-mirroring here must only fire when the prop itself changes; an
|
||||
// unconditional resync would silently undo that pin on the next incidental
|
||||
// render (wiring.tsx re-renders constantly during an active turn).
|
||||
let cache!: Cache
|
||||
|
||||
const { rerender } = render(
|
||||
<Harness activeSessionId="runtime-A" onReady={c => (cache = c)} selectedStoredSessionId="stored-A" />
|
||||
)
|
||||
|
||||
// Simulate submit.ts's imperative pin: a resume swapped in a new runtime
|
||||
// id without touching the prop.
|
||||
cache.activeSessionIdRef.current = 'runtime-resumed'
|
||||
|
||||
// A re-render with the SAME props (e.g. an unrelated $busy/$messages
|
||||
// change elsewhere in the tree) must not touch the pinned ref.
|
||||
rerender(<Harness activeSessionId="runtime-A" onReady={c => (cache = c)} selectedStoredSessionId="stored-A" />)
|
||||
|
||||
expect(cache.activeSessionIdRef.current).toBe('runtime-resumed')
|
||||
|
||||
// A genuine prop change (a real navigation/selection move) still wins.
|
||||
rerender(<Harness activeSessionId="runtime-B" onReady={c => (cache = c)} selectedStoredSessionId="stored-B" />)
|
||||
|
||||
expect(cache.activeSessionIdRef.current).toBe('runtime-B')
|
||||
})
|
||||
})
|
||||
|
||||
function userMessage(id: string, text: string): ChatMessage {
|
||||
return { id, role: 'user', parts: [{ type: 'text', text }] }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -51,8 +51,32 @@ export function useSessionStateCache({
|
|||
setMessages
|
||||
}: SessionStateCacheOptions) {
|
||||
const busy = useStore($busy)
|
||||
const activeSessionIdRef = useRef<string | null>(null)
|
||||
const selectedStoredSessionIdRef = useRef<string | null>(null)
|
||||
const activeSessionIdRef = useRef<string | null>(activeSessionId)
|
||||
const selectedStoredSessionIdRef = useRef<string | null>(selectedStoredSessionId)
|
||||
|
||||
// Mirror the latest prop into its ref synchronously during render — not via
|
||||
// a passive useEffect, which only fires a frame after paint and left the
|
||||
// ref pointing at the outgoing session for one commit (#59305). Guarded to
|
||||
// fire only when the PROP itself changed since the last render (the same
|
||||
// condition a `useEffect(..., [activeSessionId])` dependency array already
|
||||
// enforced) rather than unconditionally: submit.ts and use-session-actions
|
||||
// pin these refs imperatively mid-flight (e.g. to a just-resumed runtime id)
|
||||
// without updating the source atom in lockstep, and wiring.tsx re-renders
|
||||
// constantly during an active turn — an unconditional resync would silently
|
||||
// clobber that pin on the next incidental render (#54527-class regression).
|
||||
const activeSessionIdPropRef = useRef(activeSessionId)
|
||||
|
||||
if (activeSessionIdPropRef.current !== activeSessionId) {
|
||||
activeSessionIdPropRef.current = activeSessionId
|
||||
activeSessionIdRef.current = activeSessionId
|
||||
}
|
||||
|
||||
const selectedStoredSessionIdPropRef = useRef(selectedStoredSessionId)
|
||||
|
||||
if (selectedStoredSessionIdPropRef.current !== selectedStoredSessionId) {
|
||||
selectedStoredSessionIdPropRef.current = selectedStoredSessionId
|
||||
selectedStoredSessionIdRef.current = selectedStoredSessionId
|
||||
}
|
||||
const sessionStateByRuntimeIdRef = useRef(new Map<string, ClientSessionState>())
|
||||
const runtimeIdByStoredSessionIdRef = useRef(new Map<string, string>())
|
||||
const pendingViewStateRef = useRef<{ sessionId: string; state: ClientSessionState } | null>(null)
|
||||
|
|
@ -61,18 +85,10 @@ export function useSessionStateCache({
|
|||
// flush below tell a same-session refresh from a thread switch.
|
||||
const viewSessionIdRef = useRef<string | null>(null)
|
||||
|
||||
useEffect(() => {
|
||||
activeSessionIdRef.current = activeSessionId
|
||||
}, [activeSessionId])
|
||||
|
||||
useEffect(() => {
|
||||
setMutableRef(busyRef, busy)
|
||||
}, [busy, busyRef])
|
||||
|
||||
useEffect(() => {
|
||||
selectedStoredSessionIdRef.current = selectedStoredSessionId
|
||||
}, [selectedStoredSessionId])
|
||||
|
||||
const ensureSessionState = useCallback((sessionId: string, storedSessionId?: string | null) => {
|
||||
const existing = sessionStateByRuntimeIdRef.current.get(sessionId)
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import type { ComposerAttachment } from '@/store/composer'
|
|||
|
||||
import {
|
||||
attachmentDisplayText,
|
||||
attachmentId,
|
||||
coerceThinkingText,
|
||||
optimisticAttachmentRef,
|
||||
parseCommandDispatch,
|
||||
|
|
@ -150,3 +151,30 @@ describe('parseSlashCommand', () => {
|
|||
expect(parseSlashCommand('/ some words')).toEqual({ arg: '', name: '' })
|
||||
})
|
||||
})
|
||||
|
||||
describe('attachmentId', () => {
|
||||
it('normalizes a trailing slash on a url so a re-attach dedupes (#59305 P2)', () => {
|
||||
expect(attachmentId('url', 'https://example.com/a')).toBe(attachmentId('url', 'https://example.com/a/'))
|
||||
})
|
||||
|
||||
it('falls back to the trimmed raw value for a malformed url instead of throwing', () => {
|
||||
expect(() => attachmentId('url', 'not a url')).not.toThrow()
|
||||
expect(attachmentId('url', ' not a url ')).toBe(attachmentId('url', 'not a url'))
|
||||
})
|
||||
|
||||
it('normalizes backslash path separators so a Windows and posix path dedupe', () => {
|
||||
expect(attachmentId('file', 'a\\b.ts')).toBe(attachmentId('file', 'a/b.ts'))
|
||||
})
|
||||
|
||||
it('normalizes a trailing slash on a folder path', () => {
|
||||
expect(attachmentId('folder', 'src/app/')).toBe(attachmentId('folder', 'src/app'))
|
||||
})
|
||||
|
||||
it('does not collapse a bare root path to an empty id', () => {
|
||||
expect(attachmentId('folder', '/')).toBe('folder:/')
|
||||
})
|
||||
|
||||
it('keeps distinct urls distinct', () => {
|
||||
expect(attachmentId('url', 'https://example.com/a')).not.toBe(attachmentId('url', 'https://example.com/b'))
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -149,8 +149,37 @@ export function contextPath(path: string, cwd: string): string {
|
|||
return path.startsWith(normalizedCwd) ? path.slice(normalizedCwd.length) : path
|
||||
}
|
||||
|
||||
// IDs are content-derived (`kind:value`), not uuids, so upsertAttachment's
|
||||
// exact-match dedupe only catches a re-attach when the raw value matches
|
||||
// byte-for-byte. Normalize the value first so a trailing slash, a `\` path
|
||||
// separator, etc. don't slip past dedupe as a "different" attachment.
|
||||
function normalizeAttachmentValue(kind: ComposerAttachment['kind'], value: string): string {
|
||||
const trimmed = value.trim()
|
||||
|
||||
if (kind === 'url') {
|
||||
try {
|
||||
// The WHATWG URL parser only collapses an EMPTY path to '/' (bare
|
||||
// origin) — it does not treat '/a' and '/a/' as equivalent, so strip a
|
||||
// trailing slash ourselves once the URL is otherwise canonicalized
|
||||
// (scheme/host case, default ports, etc.).
|
||||
return new URL(trimmed).toString().replace(/\/+$/, '')
|
||||
} catch {
|
||||
return trimmed
|
||||
}
|
||||
}
|
||||
|
||||
if (kind === 'file' || kind === 'folder' || kind === 'image') {
|
||||
const posix = trimmed.replace(/\\/g, '/')
|
||||
|
||||
// Don't collapse a bare root ('/' or 'C:/') down to an empty string.
|
||||
return posix.length > 1 ? posix.replace(/\/+$/, '') : posix
|
||||
}
|
||||
|
||||
return trimmed
|
||||
}
|
||||
|
||||
export function attachmentId(kind: ComposerAttachment['kind'], value: string): string {
|
||||
return `${kind}:${value}`
|
||||
return `${kind}:${normalizeAttachmentValue(kind, value)}`
|
||||
}
|
||||
|
||||
export function pathLabel(path: string): string {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue