mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(desktop): pin composer draft scope to the swap-effect owner, not the render ref
Fixes #54527 — a message typed into one TUI session could be silently misrouted into (or overwritten by) another concurrently-open session. Root cause: activeQueueSessionKeyRef is written on every render, but the debounced draft-persist timer, the pagehide flush, and dispatchSubmit's reject-restore path all read it lazily at async-resolve time instead of capturing the scope that was active when the operation started. A session switch landing between capture and resolve relabels one session's text under the other session's key. A large paste widens the window (slower synchronous render), which matches the original report. Fix: introduce draftScopeRef, written only by the draft-swap effect (so it always reflects the session whose text is actually loaded in the editor) and read it instead of the render-time ref at both async write sites. dispatchSubmit's restore() now uses the submittedScope already captured at dispatch instead of re-reading the live ref. Also adds isPendingDraftPersistCurrent as defense-in-depth: before the debounce timer commits a write, it verifies its captured {scope, text} pair is still the one on file. This is a no-op under the fix above (a session swap or a newer keystroke already clears/replaces the pending entry via clearTimeout), but turns any future regression that reintroduces a stale/live-ref read at this call site into a dropped write instead of a silent cross-session misroute. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
0b2907f586
commit
2afa92c74f
4 changed files with 105 additions and 6 deletions
|
|
@ -1,7 +1,14 @@
|
|||
import type { Unstable_TriggerItem } from '@assistant-ui/core'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { pickPlaceholder, slashArgStage, slashChipKindForItem, slashCommandToken } from './composer-utils'
|
||||
import {
|
||||
isPendingDraftPersistCurrent,
|
||||
type PendingDraftPersist,
|
||||
pickPlaceholder,
|
||||
slashArgStage,
|
||||
slashChipKindForItem,
|
||||
slashCommandToken
|
||||
} from './composer-utils'
|
||||
|
||||
const item = (group: string): Unstable_TriggerItem =>
|
||||
({ id: 'x', type: 'slash', label: 'x', metadata: { group } }) as unknown as Unstable_TriggerItem
|
||||
|
|
@ -38,3 +45,36 @@ describe('pickPlaceholder', () => {
|
|||
expect(pool).toContain(pickPlaceholder(pool))
|
||||
})
|
||||
})
|
||||
|
||||
describe('isPendingDraftPersistCurrent (#54527 integrity guard)', () => {
|
||||
it('accepts a write when the pending entry still matches what was captured', () => {
|
||||
const entry: PendingDraftPersist = { scope: 'session-a', text: 'hello' }
|
||||
|
||||
expect(isPendingDraftPersistCurrent(entry, entry)).toBe(true)
|
||||
expect(isPendingDraftPersistCurrent({ scope: 'session-a', text: 'hello' }, entry)).toBe(true)
|
||||
})
|
||||
|
||||
it('rejects when the pending slot was cleared (session swap / newer flush already committed)', () => {
|
||||
const entry: PendingDraftPersist = { scope: 'session-a', text: 'hello' }
|
||||
|
||||
expect(isPendingDraftPersistCurrent(null, entry)).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects when the pending slot now belongs to a different session (the #54527 misroute shape)', () => {
|
||||
const captured: PendingDraftPersist = { scope: 'session-a', text: 'carefully composed prompt' }
|
||||
const supersededBy: PendingDraftPersist = { scope: 'session-b', text: 'different draft' }
|
||||
|
||||
expect(isPendingDraftPersistCurrent(supersededBy, captured)).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects when the pending slot was replaced by a newer keystroke in the same session', () => {
|
||||
const captured: PendingDraftPersist = { scope: 'session-a', text: 'first draft' }
|
||||
const supersededBy: PendingDraftPersist = { scope: 'session-a', text: 'first draft continued' }
|
||||
|
||||
expect(isPendingDraftPersistCurrent(supersededBy, captured)).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects when nothing was ever captured', () => {
|
||||
expect(isPendingDraftPersistCurrent(null, null)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -58,3 +58,28 @@ export interface QueueEditState {
|
|||
}
|
||||
|
||||
export const cloneAttachments = (attachments: ComposerAttachment[]) => attachments.map(a => ({ ...a }))
|
||||
|
||||
export interface PendingDraftPersist {
|
||||
scope: string | null
|
||||
text: string
|
||||
}
|
||||
|
||||
/**
|
||||
* Defense-in-depth for #54527: the debounce timer and the `pagehide` flush
|
||||
* both write a captured `{ scope, text }` pair some time after it was
|
||||
* scheduled. Before either commits the write, this checks the pair is still
|
||||
* the one currently on file — i.e. nothing cleared or replaced it in the
|
||||
* meantime (a session swap, a newer keystroke). The scope-capture fix
|
||||
* upstream (`draftScopeRef`) already makes every captured pair correct by
|
||||
* construction; this guard exists so that if a future change reintroduces a
|
||||
* stale/live-ref read at one of these call sites, the write is dropped
|
||||
* instead of silently filing one session's text under another session's key.
|
||||
*/
|
||||
export function isPendingDraftPersistCurrent(
|
||||
pending: PendingDraftPersist | null,
|
||||
expected: PendingDraftPersist | null
|
||||
): boolean {
|
||||
return (
|
||||
pending !== null && expected !== null && pending.scope === expected.scope && pending.text === expected.text
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -5,7 +5,12 @@ import { SLASH_COMMAND_RE } from '@/lib/chat-runtime'
|
|||
import { $composerAttachments, type ComposerAttachment, stashSessionDraft, takeSessionDraft } from '@/store/composer'
|
||||
import { isBrowsingHistory } from '@/store/composer-input-history'
|
||||
|
||||
import { cloneAttachments, DRAFT_PERSIST_DEBOUNCE_MS, type QueueEditState } from '../composer-utils'
|
||||
import {
|
||||
cloneAttachments,
|
||||
DRAFT_PERSIST_DEBOUNCE_MS,
|
||||
isPendingDraftPersistCurrent,
|
||||
type QueueEditState
|
||||
} from '../composer-utils'
|
||||
import {
|
||||
type ComposerInsertMode,
|
||||
focusComposerInput,
|
||||
|
|
@ -77,6 +82,13 @@ export function useComposerDraft({
|
|||
const draftPersistTimerRef = useRef<number | undefined>(undefined)
|
||||
const activeQueueSessionKeyRef = useRef(activeQueueSessionKey)
|
||||
activeQueueSessionKeyRef.current = activeQueueSessionKey
|
||||
// Owned only by the swap effect below — unlike activeQueueSessionKeyRef this
|
||||
// does NOT update on every render, so it always reflects the session whose
|
||||
// text is actually loaded in the editor. Async work (debounce timers,
|
||||
// pagehide flush) must persist against this, not the render-time ref, or a
|
||||
// session switch mid-flight files one session's draft under another's key
|
||||
// (#54527).
|
||||
const draftScopeRef = useRef(activeQueueSessionKey)
|
||||
const sessionIdRef = useRef(sessionId)
|
||||
sessionIdRef.current = sessionId
|
||||
const queueEditStateRef = useRef<QueueEditState | null>(queueEditRef.current)
|
||||
|
|
@ -222,10 +234,20 @@ export function useComposerDraft({
|
|||
return
|
||||
}
|
||||
|
||||
const scope = activeQueueSessionKeyRef.current
|
||||
pendingDraftPersistRef.current = { scope, text }
|
||||
const scope = draftScopeRef.current
|
||||
const entry = { scope, text }
|
||||
pendingDraftPersistRef.current = entry
|
||||
window.clearTimeout(draftPersistTimerRef.current)
|
||||
draftPersistTimerRef.current = window.setTimeout(() => {
|
||||
// Integrity guard (defense-in-depth, #54527): only commit if this is
|
||||
// still the pending write on file. A session swap or a newer
|
||||
// keystroke clears/replaces it before firing in the normal case; this
|
||||
// catches any future call site that skips that bookkeeping instead of
|
||||
// silently filing text under the wrong session.
|
||||
if (!isPendingDraftPersistCurrent(pendingDraftPersistRef.current, entry)) {
|
||||
return
|
||||
}
|
||||
|
||||
pendingDraftPersistRef.current = null
|
||||
stashAt(scope, text)
|
||||
}, DRAFT_PERSIST_DEBOUNCE_MS)
|
||||
|
|
@ -285,6 +307,14 @@ export function useComposerDraft({
|
|||
// never clears composer state; this effect alone stashes on leave, restores
|
||||
// on enter. Keyed writes are idempotent, so no skip-sentinel.
|
||||
useEffect(() => {
|
||||
// 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
|
||||
// fire later would just clobber with an older snapshot.
|
||||
window.clearTimeout(draftPersistTimerRef.current)
|
||||
pendingDraftPersistRef.current = null
|
||||
draftScopeRef.current = activeQueueSessionKey
|
||||
|
||||
const { attachments, text } = takeSessionDraft(activeQueueSessionKey)
|
||||
loadIntoComposer(text, attachments)
|
||||
|
||||
|
|
@ -304,7 +334,7 @@ export function useComposerDraft({
|
|||
// inside the debounce/rAF window would drop trailing keystrokes without this.
|
||||
useEffect(() => {
|
||||
const flushPendingDraftPersist = () => {
|
||||
const scope = activeQueueSessionKeyRef.current
|
||||
const scope = draftScopeRef.current
|
||||
const editing = queueEditStateRef.current
|
||||
|
||||
if (editing?.sessionKey === scope || isBrowsingHistory(sessionIdRef.current)) {
|
||||
|
|
|
|||
|
|
@ -79,7 +79,11 @@ export function useComposerSubmit({
|
|||
|
||||
const restore = () => {
|
||||
loadIntoComposer(text, submittedAttachments)
|
||||
stashAt(activeQueueSessionKeyRef.current, text, submittedAttachments)
|
||||
// Use the scope captured at dispatch, not whatever session is focused
|
||||
// now — the gateway can reject well after the user has switched away,
|
||||
// and re-stashing into the currently-focused session would overwrite
|
||||
// its draft with the rejected text from a different session (#54527).
|
||||
stashAt(submittedScope, text, submittedAttachments)
|
||||
}
|
||||
|
||||
void Promise.resolve(attachments ? onSubmit(text, { attachments }) : onSubmit(text))
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue