From 522b4bc67eaf0e8cd2fd89940515fd51029a4e4d Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 25 Aug 2026 13:40:36 +0200 Subject: [PATCH] fix(ai-sessions): correct payload stripping and turn-end reporting Co-Authored-By: Claude Opus 5 --- .../copilot/chat/AIChatManager.svelte.ts | 19 ++++++++- .../components/sessions/sessionMirror.test.ts | 39 ++++++++++++++++++ .../sessions/sessionMirrorPayload.ts | 31 ++++++++++++++ .../sessions/sessionRuntime.svelte.ts | 40 +++++++------------ 4 files changed, 101 insertions(+), 28 deletions(-) create mode 100644 frontend/src/lib/components/sessions/sessionMirror.test.ts create mode 100644 frontend/src/lib/components/sessions/sessionMirrorPayload.ts diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index d12677406b..6254366f5b 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -701,6 +701,14 @@ export class AIChatManager { * session runtime. */ mirroringRemoteRun = $state(false) + /** Whether the turn that just finished is one a follow-up should be sent + * after: it committed, or the user deliberately stopped it. False through a + * provider error, an empty-response rollback, or a programmatic cancel — the + * states this manager deliberately keeps its own queued message through. The + * session runtime reports it to the other tabs, whose queues follow the same + * rule. */ + lastTurnAcceptsFollowUp = false + // Workspace items the CURRENT chat modified via AI tool calls, as // `${UserDraftItemKind}:${storagePath}` keys (see modifiedItemsMask.ts). // undefined = untracked: only the global side-panel chat (never initialised), @@ -781,7 +789,10 @@ export class AIChatManager { // turn-end save. #maskPersistQueue: Promise = Promise.resolve() #persistModifiedItems(): Promise { - // Runs outside any turn, so the run guard never sees it. + // Runs outside any turn, so the run guard never sees it. Dropped rather + // than deferred: the re-read when the run ends reseeds the mask from the + // driving tab's record, so a write held back here would be overwritten by + // it anyway — and writing now would put a mismatched pair in the record. if (this.mirroringRemoteRun) return this.#maskPersistQueue this.#maskPersistQueue = this.#maskPersistQueue.then(() => this.historyManager @@ -2925,6 +2936,9 @@ export class AIChatManager { // send exits before install. Kept in a mutable local so every exit path // releases the right key. let reservationKey = options.resendReservationKey + // Cleared up front so an exit before the verdict below (a refused mode, a + // pre-flight throw) reports this turn rather than the previous one's. + this.lastTurnAcceptsFollowUp = false const requestedMode = options.mode ?? this.mode if (!isAIModeVisible(requestedMode)) { this.#releaseOutgoingReservation(reservationKey) @@ -3812,7 +3826,8 @@ export class AIChatManager { // empty-response rollback, or a programmatic cancel (panel teardown, // save-and-clear) leaves it in place as a card so it isn't fired into a // failed or torn-down turn. - if (turnCommittedCleanly || this.wasCancelledByUser()) { + this.lastTurnAcceptsFollowUp = turnCommittedCleanly || this.wasCancelledByUser() + if (this.lastTurnAcceptsFollowUp) { await this.flushQueuedMessage() } // A background job may have finished mid-turn: its note missed this turn's diff --git a/frontend/src/lib/components/sessions/sessionMirror.test.ts b/frontend/src/lib/components/sessions/sessionMirror.test.ts new file mode 100644 index 0000000000..8d24bc4fd4 --- /dev/null +++ b/frontend/src/lib/components/sessions/sessionMirror.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' +import { withoutHeavyPayloads } from './sessionMirrorPayload' +import type { DisplayMessage } from '$lib/components/copilot/chat/shared' + +// Frames go out several times a second for the whole turn, so anything left in +// them is re-cloned and re-broadcast on every tick. The caps allow megabytes per +// image and per pasted file, and a structural typecheck cannot catch a field +// name that no longer exists — the message union is cast on the way out. +describe('withoutHeavyPayloads', () => { + it('strips the bytes and keeps what the transcript renders', () => { + const messages = [ + { + role: 'user', + content: 'look at this', + images: [ + { dataUrl: 'data:image/png;base64,AAAA', mediaType: 'image/png', name: 'shot.png' } + ], + files: [{ name: 'notes.md', content: 'a very long pasted file', id: 'f1' }] + }, + { role: 'tool', content: 'took a screenshot', imageUrl: 'data:image/png;base64,BBBB' } + ] as unknown as DisplayMessage[] + + const stripped = withoutHeavyPayloads(messages) + + expect(JSON.stringify(stripped)).not.toContain('data:image') + expect(JSON.stringify(stripped)).not.toContain('a very long pasted file') + // The file chip is labelled from the name, so that has to survive. + expect((stripped[0] as any).files).toEqual([{ name: 'notes.md', content: '', id: 'f1' }]) + // Images are dropped whole: the bubble renders one per entry with no + // per-image guard, so an emptied url would show a broken image instead. + expect((stripped[0] as any).images).toBeUndefined() + expect((stripped[0] as any).content).toBe('look at this') + }) + + it('passes through a message with nothing heavy in it', () => { + const messages = [{ role: 'assistant', content: 'plain reply' }] as unknown as DisplayMessage[] + expect(withoutHeavyPayloads(messages)[0]).toBe(messages[0]) + }) +}) diff --git a/frontend/src/lib/components/sessions/sessionMirrorPayload.ts b/frontend/src/lib/components/sessions/sessionMirrorPayload.ts new file mode 100644 index 0000000000..1635659601 --- /dev/null +++ b/frontend/src/lib/components/sessions/sessionMirrorPayload.ts @@ -0,0 +1,31 @@ +import type { DisplayMessage } from '$lib/components/copilot/chat/shared' + +/** + * Drop the attachment bytes from the transcript a mirror frame carries. + * + * A frame goes out several times a second for the whole turn, and the message + * that holds an image or a pasted file sits in its tail unchanged throughout — + * so shipping the payloads means re-cloning and re-broadcasting the same + * megabytes on every tick (the caps allow roughly 16MB of base64 image plus 5MB + * of file content on a single message). Watchers render the transcript without + * them and receive the real thing when the turn ends and they re-read the + * record the driving tab saved. + */ +export function withoutHeavyPayloads(messages: DisplayMessage[]): DisplayMessage[] { + return messages.map((message) => { + const images = 'images' in message ? message.images : undefined + const files = 'files' in message ? message.files : undefined + const imageUrl = 'imageUrl' in message ? message.imageUrl : undefined + if (!images?.length && !files?.length && !imageUrl) return message + const stripped: Record = { ...message } + // Dropped rather than blanked: the bubble renders one per entry with + // no per-image guard, so an emptied dataUrl would show a broken image where + // the real one is about to appear. + if (images?.length) delete stripped.images + // Kept, minus the bytes: the chip is labelled from the name. + if (files?.length) stripped.files = files.map((f) => ({ ...f, content: '' })) + // Guarded at the render site, so an empty string renders nothing. + if (imageUrl) stripped.imageUrl = '' + return stripped as DisplayMessage + }) +} diff --git a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts index 62dbf15b48..b0c0fdfd44 100644 --- a/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionRuntime.svelte.ts @@ -93,6 +93,7 @@ import { getNonStreamingMetadataCompletion } from '$lib/components/copilot/lib' import { sendUserToast } from '$lib/toast' import { pendingUserAction, type DisplayMessage } from '$lib/components/copilot/chat/shared' import type { ChatCompletionMessageParam } from 'openai/resources/index.mjs' +import { withoutHeavyPayloads } from './sessionMirrorPayload' import { broadcastMirror, broadcastTurnEnd, @@ -937,19 +938,27 @@ async function initRuntime(runtime: SessionRuntime, session: Session) { // turns interleave into one chat id. manager.runGuard = async (body) => { const outcome = await withSessionRunLock(session.id, async () => { + // The whole turn protocol rides on ownership, not just the frames: a + // turn-end announced without it would reach tabs that were never + // mirroring and end a run of their own that is still going. + if (!RUN_OWNERSHIP_AVAILABLE) return body() // The first frame doubles as the "a run started here" signal: it is // posted immediately and carries the chat id the watchers need. startMirroring(session.id) - let committed = false try { - const result = await body() - committed = result !== false - return result + return await body() } finally { stopMirroring(session.id) // The chat id is re-read here, not reused from above: the turn may // have rotated it, and the listeners key their IndexedDB re-read on it. - broadcastTurnEnd(session.id, manager.historyManager.getCurrentChatId(), committed) + broadcastTurnEnd( + session.id, + manager.historyManager.getCurrentChatId(), + // Not "did the send return truthy": that means the input was + // consumed, and stays true through provider errors and rollbacks. + // The manager reports the rule it applies to its own queue. + manager.lastTurnAcceptsFollowUp + ) } }) if (outcome === 'busy') { @@ -1008,27 +1017,6 @@ async function initRuntime(runtime: SessionRuntime, session: Session) { // that reactive tracking of the array root would miss. const mirrorTimers = new Map>() -// Attachments a frame does not carry. They are bounded only by the upload caps -// (megabytes of base64 per image, and again per pasted file), they sit in the -// tail unchanged for the whole turn, and a frame goes out several times a -// second — so shipping them would re-clone and re-post the same bytes on every -// tick. Watchers render a placeholder until the turn ends, where the IndexedDB -// re-read hands them the real thing. -function withoutHeavyPayloads(messages: DisplayMessage[]): DisplayMessage[] { - return messages.map((message) => { - const images = 'images' in message ? message.images : undefined - const files = 'files' in message ? message.files : undefined - const imageUrl = 'imageUrl' in message ? message.imageUrl : undefined - if (!images?.length && !files?.length && !imageUrl) return message - return { - ...message, - ...(images?.length ? { images: images.map((i) => ({ ...i, url: '' })) } : {}), - ...(files?.length ? { files: files.map((f) => ({ ...f, content: '' })) } : {}), - ...(imageUrl ? { imageUrl: '' } : {}) - } as DisplayMessage - }) -} - // The driver's transcript length at its last frame, so a shrink is detectable. const lastSentTotals = new Map()