diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index 6688107aa827..90b8569b19f2 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -250,7 +250,6 @@ export function ContribWiring({ children }: { children: ReactNode }) { ) const { refreshProjectBranch } = useCwdActions({ - activeSessionId, activeSessionIdRef, onSessionRuntimeInfo: updateActiveSessionRuntimeInfo, requestGateway diff --git a/apps/desktop/src/app/session/hooks/use-cwd-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-cwd-actions.test.tsx index 4ce81fd226c1..213c5d2054d6 100644 --- a/apps/desktop/src/app/session/hooks/use-cwd-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-cwd-actions.test.tsx @@ -37,7 +37,6 @@ function Harness({ requestGateway: (method: string, params?: Record) => Promise }) { const actions = useCwdActions({ - activeSessionId: activeSessionIdRef.current, activeSessionIdRef, requestGateway }) diff --git a/apps/desktop/src/app/session/hooks/use-cwd-actions.ts b/apps/desktop/src/app/session/hooks/use-cwd-actions.ts index 8226a2f59501..cf0e469cc50c 100644 --- a/apps/desktop/src/app/session/hooks/use-cwd-actions.ts +++ b/apps/desktop/src/app/session/hooks/use-cwd-actions.ts @@ -12,14 +12,12 @@ import { import type { SessionRuntimeInfo } from '@/types/hermes' interface CwdActionsOptions { - activeSessionId: string | null activeSessionIdRef: MutableRefObject onSessionRuntimeInfo?: (info: Pick) => void requestGateway: (method: string, params?: Record) => Promise } export function useCwdActions({ - activeSessionId, activeSessionIdRef, onSessionRuntimeInfo, requestGateway @@ -59,7 +57,13 @@ export function useCwdActions({ return } - if (!activeSessionId) { + // Ref, not the closure-captured prop: this hook's consumers are memoized + // on a stable actions object, so the prop can still name the previously + // focused chat. Re-anchoring the wrong session's workspace would point + // that agent's terminal/file tools at another conversation's project. + const sessionId = activeSessionIdRef.current + + if (!sessionId) { setCurrentCwd(trimmed) const workspaceGeneration = setNewChatWorkspaceTarget(trimmed) @@ -92,7 +96,7 @@ export function useCwdActions({ try { const info = await requestGateway('session.cwd.set', { - session_id: activeSessionId, + session_id: sessionId, cwd: trimmed }) @@ -117,7 +121,7 @@ export function useCwdActions({ }) } }, - [activeSessionId, activeSessionIdRef, copy, onSessionRuntimeInfo, requestGateway] + [activeSessionIdRef, copy, onSessionRuntimeInfo, requestGateway] ) return { changeSessionCwd, refreshProjectBranch } 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 68f0505eba80..0eb73bdd98e8 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 @@ -73,6 +73,8 @@ async function actRender(ui: React.ReactElement) { interface HarnessHandle { activeSessionIdRef: MutableRefObject cancelRun: () => Promise + editMessage: (edited: Parameters['editMessage']>[0]) => Promise + reloadFromMessage: (parentId: null | string) => Promise restoreToMessage: (messageId: string, target?: { text?: string; userOrdinal?: number | null }) => Promise redirectPrompt: (text: string) => Promise /** @deprecated Use `redirectPrompt`. */ @@ -174,6 +176,10 @@ function Harness({ activeSessionIdRef, cancelRun: (...args: Parameters) => act(async () => actions.cancelRun(...args)) as Promise, + editMessage: (...args: Parameters) => + act(async () => actions.editMessage(...args)) as Promise, + reloadFromMessage: (...args: Parameters) => + act(async () => actions.reloadFromMessage(...args)) as Promise, restoreToMessage: (...args: Parameters) => act(async () => actions.restoreToMessage(...args)) as Promise, redirectPrompt: (...args: Parameters) => @@ -186,6 +192,8 @@ function Harness({ }) }, [ actions.cancelRun, + actions.editMessage, + actions.reloadFromMessage, actions.restoreToMessage, actions.redirectPrompt, actions.steerPrompt, @@ -3388,3 +3396,176 @@ describe('uploadComposerAttachment remote read failures', () => { ).rejects.toThrow('ENOENT: no such file') }) }) + +// The actions bag is a STABLE ref that wiring.tsx mutates in place +// (Object.assign), and the pane surfaces are memoized on that stable ref — so a +// surface does NOT re-render when the active session changes and its props keep +// holding whichever `usePromptActions` closure was current when it last +// rendered. `activeSessionIdRef` is therefore the authority (mirrored during +// render in use-session-state-cache, and pinned imperatively mid-flight by +// submit.ts / use-session-actions without touching the source prop), while the +// closure-captured `activeSessionId` prop is stale by design. +// +// Every action below used `activeSessionId || activeSessionIdRef.current`, which +// prefers the STALE value whenever it is non-null and only consults the fresh +// ref once the prop is null. That routed history-mutating writes and live-turn +// corrections into the previously-focused chat: content the user typed in chat B +// reached chat A's agent, and rewinds truncated the wrong session's transcript. +// `cancelRun` in the same file already reads the ref exclusively and documents +// exactly this hazard. +describe('usePromptActions stale-closure session routing', () => { + const RUNTIME_SESSION_B = 'rt-session-b-current' + + beforeEach(() => { + // Earlier suites in this file leave `$busy` true (it is a module-level + // store, shared across tests). reloadFromMessage bails on a busy session, + // so without this reset the regeneration case never reaches its routing + // decision and would pass vacuously. + $busy.set(false) + }) + + afterEach(() => { + cleanup() + setMessages([]) + $busy.set(false) + vi.restoreAllMocks() + }) + + type GatewayCall = [string, Record?] + type GatewayRequestFn = (method: string, params?: Record, timeoutMs?: number) => Promise + type GatewayMock = GatewayRequestFn & { mock: { calls: unknown[][] } } + + function gatewayCalls(requestGateway: GatewayMock): GatewayCall[] { + return requestGateway.mock.calls as unknown as GatewayCall[] + } + + // Renders with `activeSessionId` (the prop) pinned to session A, then moves + // the ref to session B — the exact split a memoized surface holds after the + // user switches chats. Every action must target B. + // + // The rewind/reload planners read the GLOBAL `$messages` store (the view + // transcript), not the harness's per-session state, so the fixture has to + // seed both or the action bails on a null plan and the test would pass + // vacuously without ever reaching the routing decision. + async function renderWithStaleClosure(requestGateway: GatewayMock, seedMessages?: unknown[]) { + const activeSessionIdRef: MutableRefObject = { current: RUNTIME_SESSION_ID } + const updated: string[] = [] + + if (seedMessages) { + setMessages(seedMessages as never) + } + + let handle: HarnessHandle | null = null + await actRender( + (handle = h)} + onUpdateState={sessionId => updated.push(sessionId)} + refreshSessions={async () => undefined} + requestGateway={requestGateway} + seedMessages={seedMessages} + selectedStoredSessionIdRef={{ current: null }} + /> + ) + + // The user switches to session B. The ref follows; the prop captured in the + // memoized surface's closure still says session A. + activeSessionIdRef.current = RUNTIME_SESSION_B + + return { handle: handle!, updated } + } + + it('redirects the live turn into the CURRENT session, not the stale closure session', async () => { + const requestGateway = vi.fn(async () => ({ status: 'redirected' }) as never) as unknown as GatewayMock + const { handle } = await renderWithStaleClosure(requestGateway) + + await handle.redirectPrompt('actually use Postgres') + + // A redirect reaches the model mid-turn. Sent to the stale session, the + // correction lands in a conversation the user is no longer looking at — + // this is the observed "session suddenly working on another chat's task". + expect(requestGateway).toHaveBeenCalledWith('session.redirect', { + session_id: RUNTIME_SESSION_B, + text: 'actually use Postgres' + }) + expect(requestGateway).not.toHaveBeenCalledWith( + 'session.redirect', + expect.objectContaining({ session_id: RUNTIME_SESSION_ID }) + ) + }) + + it('regenerates against the CURRENT session, not the stale closure session', async () => { + const requestGateway = vi.fn(async () => ({}) as never) as unknown as GatewayMock + + const { handle, updated } = await renderWithStaleClosure(requestGateway, [ + { id: 'u1', parts: [textPart('original prompt')], role: 'user', timestamp: 0 }, + { id: 'a1', parts: [textPart('reply')], role: 'assistant', timestamp: 1 } + ]) + + await handle.reloadFromMessage('u1') + + // prompt.submit with a truncate ordinal DELETES history after that point. + // Aimed at the stale session it destroys the wrong transcript. + expect(requestGateway).toHaveBeenCalledWith( + 'prompt.submit', + expect.objectContaining({ session_id: RUNTIME_SESSION_B }), + expect.anything() + ) + expect(requestGateway).not.toHaveBeenCalledWith( + 'prompt.submit', + expect.objectContaining({ session_id: RUNTIME_SESSION_ID }), + expect.anything() + ) + expect(updated).toContain(RUNTIME_SESSION_B) + expect(updated).not.toContain(RUNTIME_SESSION_ID) + }) + + it('restores a checkpoint in the CURRENT session, not the stale closure session', async () => { + const requestGateway = vi.fn(async () => ({}) as never) as unknown as GatewayMock + + const { handle, updated } = await renderWithStaleClosure(requestGateway, [ + { id: 'u1', parts: [textPart('first prompt')], role: 'user', timestamp: 0 }, + { id: 'a1', parts: [textPart('first reply')], role: 'assistant', timestamp: 1 }, + { id: 'u2', parts: [textPart('second prompt')], role: 'user', timestamp: 2 } + ]) + + await handle.restoreToMessage('u2') + + // A rewind is destructive; the optimistic truncation must also be applied + // to the session that actually receives the rewind. + expect(updated).toContain(RUNTIME_SESSION_B) + expect(updated).not.toContain(RUNTIME_SESSION_ID) + + for (const [, params] of gatewayCalls(requestGateway)) { + if (params && 'session_id' in params) { + expect(params.session_id).toBe(RUNTIME_SESSION_B) + } + } + }) + + it('edits a message in the CURRENT session, not the stale closure session', async () => { + const requestGateway = vi.fn(async () => ({}) as never) as unknown as GatewayMock + + const { handle, updated } = await renderWithStaleClosure(requestGateway, [ + { id: 'u1', parts: [textPart('original prompt')], role: 'user', timestamp: 0 }, + { id: 'a1', parts: [textPart('reply')], role: 'assistant', timestamp: 1 } + ]) + + await handle.editMessage({ + content: [{ text: 'edited prompt', type: 'text' }], + parentId: null, + role: 'user', + sourceId: 'u1' + } as never) + + expect(updated).toContain(RUNTIME_SESSION_B) + expect(updated).not.toContain(RUNTIME_SESSION_ID) + + for (const [, params] of gatewayCalls(requestGateway)) { + if (params && 'session_id' in params) { + expect(params.session_id).toBe(RUNTIME_SESSION_B) + } + } + }) +}) 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 18b080bd3b54..3904d8235b2e 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 @@ -637,7 +637,10 @@ export function usePromptActions({ const redirectPrompt = useCallback( async (rawText: string): Promise => { const text = sanitizeComposerInput(rawText).trim() - const sessionId = activeSessionId || activeSessionIdRef.current + // Ref, not the closure-captured prop — see cancelRun above. A redirect + // reaches the live model mid-turn, so a stale target delivers the user's + // correction into a conversation they are no longer looking at. + const sessionId = activeSessionIdRef.current if (!text || !sessionId) { return false @@ -730,7 +733,6 @@ export function usePromptActions({ return false }, [ - activeSessionId, activeSessionIdRef, appendSessionTextMessage, requestGateway, @@ -741,7 +743,11 @@ export function usePromptActions({ const reloadFromMessage = useCallback( async (parentId: string | null) => { - if (!activeSessionId || $busy.get()) { + // Ref, not the closure-captured prop — a truncating resubmit aimed at a + // stale session deletes the wrong transcript. + const sessionId = activeSessionIdRef.current + + if (!sessionId || $busy.get()) { return } @@ -752,20 +758,20 @@ export function usePromptActions({ } clearNotifications() - updateSessionState(activeSessionId, state => applyReloadOptimistic(state, plan)) + updateSessionState(sessionId, state => applyReloadOptimistic(state, plan)) try { await requestGateway( 'prompt.submit', { - session_id: activeSessionId, + session_id: sessionId, text: plan.text, ...truncateSubmitParams(plan.truncateOrdinal) }, PROMPT_SUBMIT_REQUEST_TIMEOUT_MS ) } catch (err) { - updateSessionState(activeSessionId, state => ({ + updateSessionState(sessionId, state => ({ ...state, busy: false, awaitingResponse: false @@ -773,7 +779,7 @@ export function usePromptActions({ notifyError(err, copy.regenerateFailed) } }, - [activeSessionId, copy.regenerateFailed, requestGateway, updateSessionState] + [activeSessionIdRef, copy.regenerateFailed, requestGateway, updateSessionState] ) // Cursor-style "restore checkpoint": rewind the conversation to a past user @@ -793,7 +799,9 @@ export function usePromptActions({ const restoreToMessage = useCallback( async (messageId: string, target?: RestoreMessageTarget) => { - const sessionId = activeSessionId || activeSessionIdRef.current + // Ref, not the closure-captured prop — a rewind is destructive, so a + // stale target truncates a conversation the user did not ask to rewind. + const sessionId = activeSessionIdRef.current if (!sessionId) { throw new Error('No active session to restore.') @@ -834,12 +842,14 @@ export function usePromptActions({ throw err } }, - [activeSessionId, activeSessionIdRef, busyRef, submitRewindPrompt, updateSessionState] + [activeSessionIdRef, busyRef, submitRewindPrompt, updateSessionState] ) const editMessage = useCallback( async (edited: AppendMessage) => { - const sessionId = activeSessionId || activeSessionIdRef.current + // Ref, not the closure-captured prop — an edit rewinds and resubmits, so + // a stale target rewrites the wrong session's history. + const sessionId = activeSessionIdRef.current const messages = $messages.get() const plan = sessionId ? planEdit(messages, edited) : null @@ -890,7 +900,7 @@ export function usePromptActions({ notifyError(surfaced, copy.editFailed) } }, - [activeSessionId, activeSessionIdRef, busyRef, copy.editFailed, submitRewindPrompt, updateSessionState] + [activeSessionIdRef, busyRef, copy.editFailed, submitRewindPrompt, updateSessionState] ) const handleThreadMessagesChange = useCallback(