From bc2bafe24afb8e359936866ee873c7ffe9e21abb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 14:19:31 +0000 Subject: [PATCH] a queue can no longer be delivered to the wrong conversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My fix was worse than the bug it replaced. Queue in one chat, switch to another, and the messages were SENT there — into a conversation the person had not written them in. `ChatDetailPanel` mounts `` and passed no panel id, so `queueKey` fell through to `screen:new` for every conversation it showed. One key, every chat. Switching restored the queue into the chat being opened, and the mount-drain I added — which exists so a returning queue is not left sitting there — then delivered it. Two of my changes, each defensible alone, combining into the worst outcome available. Three things, in order of what actually protects the user: **A key that cannot be shared.** `queueKey` returns null when neither a panel nor a session identifies the mount, and null persists nothing. Then an unidentifiable chat behaves exactly as it did before any of this: queue local to the mount, lost on a switch. That is the original complaint, and it is strictly better than delivering to the wrong place. A shared key is worse than no key. **The drain refuses a null key too.** Belt and braces: nothing was restored, so nothing may be sent. **`ChatDetailPanel` now says which conversation it is**, from `resumeSessionId`, which it already had in scope. The identity was one line away the whole time. Three tests pin it, including the collision itself: two resumed conversations must not produce the same key, and a null key must never persist. The lesson I would rather write down than repeat: I added the mount-drain reasoning about a queue's promise, and never asked what it would do if the key were wrong. A mechanism that sends messages needs its failure mode considered before its happy path. tsgo clean, frontend builds, 901 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 --- .../Chat/EmbeddableChat/queue-store.test.ts | 16 +++++++++++ .../apps/Chat/EmbeddableChat/queue-store.ts | 27 +++++++++++++++---- .../Chat/EmbeddableChat/useEmbeddableChat.ts | 5 +++- .../src/apps/ChatHistory/ChatDetailPanel.tsx | 4 +++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.test.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.test.ts index d1723799..8d026db3 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.test.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.test.ts @@ -12,6 +12,22 @@ const KEYS = ['p1:s1', 'p2:s1', 'p1:new', 'p2:new', 'screen:s1']; beforeEach(() => KEYS.forEach((k) => writeQueue(k, []))); describe('queueKey', () => { + // The bug the first version shipped: `ChatDetailPanel` mounts with `sessionId={undefined}`, so every + // conversation fell back to one shared key. Queue in one chat, open another, and the queue was restored + // there — and the mount-drain SENT it. Somebody's message delivered to a chat they did not write it in. + test('an unidentifiable mount gets NO key, so it cannot share one', () => { + expect(queueKey(undefined, undefined)).toBeNull(); + }); + + test('a null key never persists anything', () => { + writeQueue(null, [prompt('must not survive')]); + expect(readQueue(null)).toEqual([]); + }); + + test('two resumed conversations in the same surface do not collide', () => { + expect(queueKey('chat:AAA', undefined)).not.toBe(queueKey('chat:BBB', undefined)); + }); + test('a panel and a session together identify a queue', () => { expect(queueKey('p1', 's1')).toBe('p1:s1'); }); diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.ts index 32ea6bee..b0abead6 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/queue-store.ts @@ -28,14 +28,31 @@ import type { QueuedPrompt } from './useEmbeddableChat'; */ const queues = new Map(); -/** A queue belongs to one panel's view of one conversation. */ -export const queueKey = (panelId: string | undefined, sessionId: string | undefined): string => - `${panelId ?? 'screen'}:${sessionId ?? 'new'}`; +/** + * A queue belongs to one panel's view of one conversation — or to nothing at all. + * + * **Null when neither identifies it, and that is the important case.** The first version fell back to + * `screen:new`, so every conversation opened without a session id shared one queue: `ChatDetailPanel` + * mounts with `sessionId={undefined}`, so queueing in one chat and opening another restored the queue + * there — and the mount-drain then SENT it into the wrong conversation. Somebody's message, delivered to + * a chat they did not write it in. + * + * A shared key is worse than no key. With null the queue simply stays local to the mount and is lost on a + * switch, which is where this started — bad, but not wrong. + * + * A panel alone is enough: one panel holds one new chat, so `panel-3:new` is unambiguous. A session alone + * is enough: that IS the conversation. Neither is not. + */ +export function queueKey(panelId: string | undefined, sessionId: string | undefined): string | null { + if (!panelId && !sessionId) return null; + return `${panelId ?? 'screen'}:${sessionId ?? 'new'}`; +} -export const readQueue = (key: string): QueuedPrompt[] => queues.get(key) ?? []; +export const readQueue = (key: string | null): QueuedPrompt[] => (key ? (queues.get(key) ?? []) : []); /** Empty is the same as absent — an entry per conversation ever opened would grow without bound. */ -export function writeQueue(key: string, items: QueuedPrompt[]): void { +export function writeQueue(key: string | null, items: QueuedPrompt[]): void { + if (!key) return; if (items.length === 0) queues.delete(key); else queues.set(key, items); } diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts index 13a936fb..f4d68e47 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts @@ -102,7 +102,10 @@ export function useEmbeddableChat(params: UseEmbeddableChatParams, onMessageComp useEffect(() => { if (restoredRef.current) return; restoredRef.current = true; - if (!isGenerating && queuedRef.current.length > 0) void drainQueueRef.current(); + // `storeKey` being null means this mount cannot identify its conversation, so nothing was restored and + // nothing may be sent — see `queue-store`. Sending here on a shared key is what delivered a message + // into the chat somebody had switched TO. + if (storeKey && !isGenerating && queuedRef.current.length > 0) void drainQueueRef.current(); }, []); // Detect when agent finishes a message (isGenerating: true → false) diff --git a/src/workspaces/officerdev/src/apps/ChatHistory/ChatDetailPanel.tsx b/src/workspaces/officerdev/src/apps/ChatHistory/ChatDetailPanel.tsx index 67978aa4..dc0af42e 100644 --- a/src/workspaces/officerdev/src/apps/ChatHistory/ChatDetailPanel.tsx +++ b/src/workspaces/officerdev/src/apps/ChatHistory/ChatDetailPanel.tsx @@ -200,6 +200,10 @@ function NewChat(props: NewChatProps) {