Skip to content

Commit 10fb2a1

Browse files
committed
fix(core): [CRITICAL] Fix silent conversation memory wipe on user interruption and compaction
1 parent 4ebe140 commit 10fb2a1

3 files changed

Lines changed: 138 additions & 3 deletions

File tree

cli/src/hooks/__tests__/use-send-message.test.tsx

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,3 +186,127 @@ describe('useSendMessage continuation state', () => {
186186
}
187187
})
188188
})
189+
190+
// Regression tests for the syncRunState fix (#1054).
191+
//
192+
// Both scenarios previously caused a follow-up message to lose all conversation
193+
// context because previousRunStateRef was not updated before client.run()
194+
// settled. The tests below drive the real hook and assert that the snapshot
195+
// passed to onStateSnapshot() is the one carried into the next run's
196+
// previousRun, verifying the actual wiring — not a reimplemented proxy.
197+
describe('useSendMessage syncRunState regression (#1054)', () => {
198+
test('abort path: latestRunStateSnapshot is committed to previousRunStateRef before client.run() settles', async () => {
199+
// This exercises use-send-message.ts line ~355:
200+
// syncRunState(latestRunStateSnapshot) ← inside registerActiveRun callback
201+
// Calling stopActiveRun fires the real abort callback synchronously, before
202+
// client.run()'s promise resolves. The follow-up run must receive the
203+
// snapshot that was live at abort time, not an empty/null state.
204+
const { setup, root } = await mountHost()
205+
const runs: Promise<void>[] = []
206+
207+
try {
208+
runs.push(
209+
sendMessageFromHost!({ content: 'first message', agentMode: 'DEFAULT' }),
210+
)
211+
await waitFor('first run registered', () => runCalls.length === 1)
212+
213+
const liveSnapshot = makeRunState('mid-stream')
214+
// Simulate a partial streaming state update arriving before Esc.
215+
runCalls[0].runConfig.onStateSnapshot(liveSnapshot)
216+
// User presses Esc — fires the real registerActiveRun abort callback.
217+
stopActiveRun('user-interrupt')
218+
219+
runs.push(
220+
sendMessageFromHost!({ content: 'follow-up after abort', agentMode: 'DEFAULT' }),
221+
)
222+
await waitFor('second run registered', () => runCalls.length === 2)
223+
224+
// The real hook must have assigned liveSnapshot into previousRunStateRef
225+
// via syncRunState before we got here — not the blank sentinel.
226+
expect(runCalls[1].runConfig.previousRun).toBe(liveSnapshot)
227+
} finally {
228+
settlePendingRuns()
229+
await Promise.all(runs)
230+
flushSync(() => root.unmount())
231+
setup.renderer.destroy()
232+
}
233+
})
234+
235+
test('error path: latestRunStateSnapshot is committed to previousRunStateRef when client.run() rejects', async () => {
236+
// This exercises use-send-message.ts line ~752:
237+
// syncRunState(latestRunStateSnapshot) ← inside catch (error) block
238+
// When client.run() throws (network error, session expiry, gate error),
239+
// the catch block must persist the last received snapshot so the user's
240+
// conversation context survives the failure.
241+
const { setup, root } = await mountHost()
242+
const runs: Promise<void>[] = []
243+
244+
try {
245+
runs.push(
246+
sendMessageFromHost!({ content: 'first message', agentMode: 'DEFAULT' }),
247+
)
248+
await waitFor('first run registered', () => runCalls.length === 1)
249+
250+
const lastSnapshot = makeRunState('before-error')
251+
// Simulate a snapshot arriving mid-stream, then a network / gate error.
252+
runCalls[0].runConfig.onStateSnapshot(lastSnapshot)
253+
runCalls[0].reject(new Error('session expired'))
254+
await runs[0]
255+
256+
runs.push(
257+
sendMessageFromHost!({ content: 'continue', agentMode: 'DEFAULT' }),
258+
)
259+
await waitFor('second run registered', () => runCalls.length === 2)
260+
261+
// The catch block must have called syncRunState(latestRunStateSnapshot),
262+
// making lastSnapshot available to the next run.
263+
expect(runCalls[1].runConfig.previousRun).toBe(lastSnapshot)
264+
} finally {
265+
settlePendingRuns()
266+
await Promise.all(runs)
267+
flushSync(() => root.unmount())
268+
setup.renderer.destroy()
269+
}
270+
})
271+
test('abort path: falls back to prior run state when client.run() is aborted before any snapshot arrives', async () => {
272+
// latestRunStateSnapshot is initialized from previousRunStateRef.current (line ~313).
273+
// If the user presses Esc immediately — before the SDK emits any onStateSnapshot —
274+
// syncRunState is called with that initial value, which is the prior completed run's
275+
// state. This directly answers the question: "is latestRunStateSnapshot guaranteed
276+
// to be populated at the abort callsite?" Yes — it is never null.
277+
const { setup, root } = await mountHost()
278+
const runs: Promise<void>[] = []
279+
280+
try {
281+
// Run 1: complete successfully so there IS a known prior state.
282+
runs.push(
283+
sendMessageFromHost!({ content: 'first message', agentMode: 'DEFAULT' }),
284+
)
285+
await waitFor('first run registered', () => runCalls.length === 1)
286+
const priorState = makeRunState('completed')
287+
runCalls[0].resolve(priorState)
288+
await runs[0]
289+
290+
// Run 2: abort immediately, before any onStateSnapshot arrives.
291+
runs.push(
292+
sendMessageFromHost!({ content: 'second message', agentMode: 'DEFAULT' }),
293+
)
294+
await waitFor('second run registered', () => runCalls.length === 2)
295+
// Deliberately NO onStateSnapshot call — simulates Esc before any streaming progress.
296+
stopActiveRun('user-interrupt')
297+
298+
// Run 3 should carry priorState (from run 1), not a blank/null sentinel.
299+
runs.push(
300+
sendMessageFromHost!({ content: 'follow-up', agentMode: 'DEFAULT' }),
301+
)
302+
await waitFor('third run registered', () => runCalls.length === 3)
303+
304+
expect(runCalls[2].runConfig.previousRun).toBe(priorState)
305+
} finally {
306+
settlePendingRuns()
307+
await Promise.all(runs)
308+
flushSync(() => root.unmount())
309+
setup.renderer.destroy()
310+
}
311+
})
312+
})

cli/src/hooks/helpers/send-message.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,8 @@ export const setupStreamingContext = (params: {
305305
abortController.signal.addEventListener('abort', () => {
306306
// Abort means the user stopped streaming; update UI with an interruption notice.
307307
// Release the chain lock immediately so new messages can be sent directly instead
308-
// of being queued.
308+
// of being queued. registerActiveRun updates previousRunStateRef synchronously
309+
// with the latest snapshot so immediate follow-ups retain preserved context.
309310
streamRefs.setters.setWasAbortedByUser(true)
310311
setIsRetrying(false)
311312
timerController.stop('aborted')

cli/src/hooks/use-send-message.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,12 @@ export const useSendMessage = ({
329329
clearActiveRun(runOwnerId)
330330
}
331331

332+
const syncRunState = (state: RunState) => {
333+
if (!runChatIsCurrent()) return
334+
previousRunStateRef.current = state
335+
setRunState(state)
336+
}
337+
332338
registerActiveRun(runOwnerId, (reason) => {
333339
if (abortController.signal.aborted) return
334340

@@ -344,6 +350,10 @@ export const useSendMessage = ({
344350
if (isProcessingQueueRef) isProcessingQueueRef.current = false
345351
}
346352

353+
// Keep in-memory previousRunStateRef fresh so immediate follow-up
354+
// messages carry the latest snapshot even before client.run settles.
355+
syncRunState(latestRunStateSnapshot)
356+
347357
// Capture the old chat's array now. Context-changing callers reset the
348358
// store immediately after stopActiveRun returns.
349359
scheduleCheckpointSave(
@@ -687,8 +697,7 @@ export const useSendMessage = ({
687697
// same chat, so the interrupted turn is still saved as before.)
688698
if (!abortController.signal.aborted && runChatIsCurrent()) {
689699
// Finalize: persist state and mark complete
690-
previousRunStateRef.current = runState
691-
setRunState(runState)
700+
syncRunState(runState)
692701
setIsRetrying(false)
693702

694703
// Drop any queued/in-flight async checkpoint first so a stale write
@@ -740,6 +749,7 @@ export const useSendMessage = ({
740749
// first so a stale write can't clobber this one. Skipped after a
741750
// mid-run chat switch — the store's messages belong to the new chat.
742751
if (runChatIsCurrent()) {
752+
syncRunState(latestRunStateSnapshot)
743753
await settleCheckpointSave()
744754
saveChatState(
745755
latestRunStateSnapshot,

0 commit comments

Comments
 (0)