mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-27 17:58:07 +00:00
fix(desktop): route live-turn and history actions by the current session, not a stale closure
Redirect/steer, regenerate, restore-checkpoint, edit-message, and change-cwd read `activeSessionId || activeSessionIdRef.current`, which prefers the closure-captured prop whenever it is non-null and only falls back to the ref once the prop is null. That precedence is backwards. 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 keeps whichever closure was current when it last rendered. `activeSessionIdRef` is the authority: it is mirrored during render in use-session-state-cache, and submit.ts / use-session-actions pin it imperatively mid-flight without touching the source prop. The prop is stale by design. `cancelRun` in the same file already reads the ref exclusively and documents exactly this hazard. User-visible effect after switching chats: a typed correction was delivered into the previously focused conversation's live turn (the "session suddenly working on another chat's task" report), and rewinds truncated the wrong session's transcript — real data loss, since a truncating resubmit deletes history after the target ordinal. Nothing crosses over in stored state, which is why a DB/transcript audit of the affected session comes back clean. Also fixes the same defect in `changeSessionCwd`, where a stale target re-anchored another conversation's workspace, pointing that agent's terminal/file tools at the wrong project. The now-unused `activeSessionId` option is dropped from useCwdActions rather than left as a footgun for the next caller. Not changed, verified not affected: model-edit-submenu reads the runtime id via `useStore` (live subscription, re-renders on change), and use-composer-actions guards on `attachedSessionId === activeSessionId`, so a stale value there only skips a detach instead of writing cross-session. Tests: 4 regression cases pin each action to the current session when the prop and the ref disagree. Verified to fail against the pre-fix code (all four reported the stale `rt-abc123` instead of the current session), including a scripted revert of all four sites. Co-authored-by: Drew Donaldson <49219012+Automata-intelligentsia@users.noreply.github.com>
This commit is contained in:
parent
32fd9d65cf
commit
771f1b7f21
5 changed files with 211 additions and 18 deletions
|
|
@ -250,7 +250,6 @@ export function ContribWiring({ children }: { children: ReactNode }) {
|
|||
)
|
||||
|
||||
const { refreshProjectBranch } = useCwdActions({
|
||||
activeSessionId,
|
||||
activeSessionIdRef,
|
||||
onSessionRuntimeInfo: updateActiveSessionRuntimeInfo,
|
||||
requestGateway
|
||||
|
|
|
|||
|
|
@ -37,7 +37,6 @@ function Harness({
|
|||
requestGateway: <T>(method: string, params?: Record<string, unknown>) => Promise<T>
|
||||
}) {
|
||||
const actions = useCwdActions({
|
||||
activeSessionId: activeSessionIdRef.current,
|
||||
activeSessionIdRef,
|
||||
requestGateway
|
||||
})
|
||||
|
|
|
|||
|
|
@ -12,14 +12,12 @@ import {
|
|||
import type { SessionRuntimeInfo } from '@/types/hermes'
|
||||
|
||||
interface CwdActionsOptions {
|
||||
activeSessionId: string | null
|
||||
activeSessionIdRef: MutableRefObject<string | null>
|
||||
onSessionRuntimeInfo?: (info: Pick<SessionRuntimeInfo, 'branch' | 'cwd'>) => void
|
||||
requestGateway: <T = unknown>(method: string, params?: Record<string, unknown>) => Promise<T>
|
||||
}
|
||||
|
||||
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<SessionRuntimeInfo>('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 }
|
||||
|
|
|
|||
|
|
@ -73,6 +73,8 @@ async function actRender(ui: React.ReactElement) {
|
|||
interface HarnessHandle {
|
||||
activeSessionIdRef: MutableRefObject<string | null>
|
||||
cancelRun: () => Promise<void>
|
||||
editMessage: (edited: Parameters<ReturnType<typeof usePromptActions>['editMessage']>[0]) => Promise<void>
|
||||
reloadFromMessage: (parentId: null | string) => Promise<void>
|
||||
restoreToMessage: (messageId: string, target?: { text?: string; userOrdinal?: number | null }) => Promise<void>
|
||||
redirectPrompt: (text: string) => Promise<boolean>
|
||||
/** @deprecated Use `redirectPrompt`. */
|
||||
|
|
@ -174,6 +176,10 @@ function Harness({
|
|||
activeSessionIdRef,
|
||||
cancelRun: (...args: Parameters<typeof actions.cancelRun>) =>
|
||||
act(async () => actions.cancelRun(...args)) as Promise<void>,
|
||||
editMessage: (...args: Parameters<typeof actions.editMessage>) =>
|
||||
act(async () => actions.editMessage(...args)) as Promise<void>,
|
||||
reloadFromMessage: (...args: Parameters<typeof actions.reloadFromMessage>) =>
|
||||
act(async () => actions.reloadFromMessage(...args)) as Promise<void>,
|
||||
restoreToMessage: (...args: Parameters<typeof actions.restoreToMessage>) =>
|
||||
act(async () => actions.restoreToMessage(...args)) as Promise<void>,
|
||||
redirectPrompt: (...args: Parameters<typeof actions.redirectPrompt>) =>
|
||||
|
|
@ -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<string, unknown>?]
|
||||
type GatewayRequestFn = <T>(method: string, params?: Record<string, unknown>, timeoutMs?: number) => Promise<T>
|
||||
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<string | null> = { current: RUNTIME_SESSION_ID }
|
||||
const updated: string[] = []
|
||||
|
||||
if (seedMessages) {
|
||||
setMessages(seedMessages as never)
|
||||
}
|
||||
|
||||
let handle: HarnessHandle | null = null
|
||||
await actRender(
|
||||
<Harness
|
||||
activeSessionId={RUNTIME_SESSION_ID}
|
||||
activeSessionIdRef={activeSessionIdRef}
|
||||
onReady={h => (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)
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -637,7 +637,10 @@ export function usePromptActions({
|
|||
const redirectPrompt = useCallback(
|
||||
async (rawText: string): Promise<boolean> => {
|
||||
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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue