From e80c4ba21c63faee26c2435e3f0bf355b5075440 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 5 May 2026 13:20:21 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix:=20route=20phase-2=20attachm?= =?UTF-8?q?ent=20update=20to=20current-run=20messageId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 review on PR #12957. `processCodeOutput` intentionally preserves the original DB `messageId` across cross-turn filename reuse so `getCodeGeneratedFiles` can still trace a file back to the assistant message that originally produced it. The phase-1 SSE emit already routes by the current run's messageId — `processCodeOutput` runtime-overlays it via `Object.assign(file, { messageId, toolCallId })` and the callback writes `result.file` directly. Phase-2 was passing the raw `updateFile` return through `attachmentFromFileMetadata`, which read `messageId` straight off the DB record. On a turn-N run that re-emitted a filename from turn-1 (e.g. agent writes `output.csv` again), the phase-2 SSE update routed to `turn-1-msg` instead of `turn-N-msg`. Frontend's `useAttachmentHandler` upserts under the wrong messageAttachmentsMap slot — turn-N's pending chip stays stuck at "preparing preview…" while turn-1's already-resolved attachment gets re-merged. Fix: thread `runtimeMessageId` through `attachmentFromFileMetadata` and pass `metadata.run_id` from the phase-2 emit site. Mirrors how phase-1 sources its messageId. Tests cover the cross-turn reuse case plus the writableEnded / null-finalize / no-finalize paths to lock in the broader phase-2 emit contract. --- .../agents/__tests__/callbacks.spec.js | 217 ++++++++++++++++++ api/server/controllers/agents/callbacks.js | 27 ++- 2 files changed, 241 insertions(+), 3 deletions(-) diff --git a/api/server/controllers/agents/__tests__/callbacks.spec.js b/api/server/controllers/agents/__tests__/callbacks.spec.js index 8bd711f9c6..ea942a751e 100644 --- a/api/server/controllers/agents/__tests__/callbacks.spec.js +++ b/api/server/controllers/agents/__tests__/callbacks.spec.js @@ -326,4 +326,221 @@ describe('createToolEndCallback', () => { expect(res.write).not.toHaveBeenCalled(); }); }); + + describe('code execution two-phase preview emit', () => { + /* The two-phase code-execution flow emits the attachment twice over + * SSE: phase-1 with `status: 'pending'` and the current run's + * messageId, phase-2 with the resolved record. The phase-2 emit + * must use the CURRENT run's messageId (not the persisted DB one) + * because `processCodeOutput` intentionally preserves the original + * `messageId` on cross-turn filename reuse — `getCodeGeneratedFiles` + * needs that for prior-turn priming. + * + * Codex P1 review on PR #12957: shipping `updated.messageId` + * straight from the DB record routed phase-2 patches to the wrong + * message slot, leaving the current turn's pending chip stuck. */ + + const { processCodeOutput } = require('~/server/services/Files/Code/process'); + + function makeCodeExecutionEvent({ runId, threadId, toolCallId, fileId, name }) { + return { + output: { + name: 'execute_code', + tool_call_id: toolCallId, + artifact: { + session_id: 'sess-1', + files: [{ id: fileId, name, session_id: 'sess-1' }], + }, + }, + metadata: { run_id: runId, thread_id: threadId }, + }; + } + + /** Parse the SSE frame `res.write` produces back to a payload object. */ + function parseSseAttachment(call) { + const frame = call[0]; + const dataLine = frame.split('\n').find((l) => l.startsWith('data: ')); + return JSON.parse(dataLine.slice('data: '.length)); + } + + it('phase-2 emit uses the current run messageId, not the persisted DB messageId (cross-turn filename reuse)', async () => { + /* Simulate turn-2 reusing `output.csv` from turn-1. The DB record + * surfaced by `updateFile` carries the original `turn-1-msg` + * messageId; the runtime emit must rewrite to `turn-2-msg`. */ + res.headersSent = true; + const finalize = jest.fn().mockResolvedValue({ + file_id: 'fid-shared', + filename: 'output.csv', + filepath: '/uploads/output.csv', + type: 'text/csv', + conversationId: 'thread789', + messageId: 'turn-1-original-msg', // persisted DB id (older turn) + status: 'ready', + text: '
', + textFormat: 'html', + }); + processCodeOutput.mockResolvedValue({ + file: { + file_id: 'fid-shared', + filename: 'output.csv', + filepath: '/uploads/output.csv', + type: 'text/csv', + conversationId: 'thread789', + messageId: 'turn-2-current-run', // runtime overlay (current turn) + toolCallId: 'tool-2', + status: 'pending', + text: null, + textFormat: null, + }, + finalize, + }); + + const toolEndCallback = createToolEndCallback({ req, res, artifactPromises }); + const event = makeCodeExecutionEvent({ + runId: 'turn-2-current-run', + threadId: 'thread789', + toolCallId: 'tool-2', + fileId: 'fid-shared', + name: 'output.csv', + }); + await toolEndCallback({ output: event.output }, event.metadata); + await Promise.all(artifactPromises); + // Wait one more tick so the fire-and-forget finalize() chain settles. + await new Promise((resolve) => setImmediate(resolve)); + + // Two SSE writes: phase-1 (pending) and phase-2 (ready). + expect(res.write).toHaveBeenCalledTimes(2); + const phase1 = parseSseAttachment(res.write.mock.calls[0]); + const phase2 = parseSseAttachment(res.write.mock.calls[1]); + + // Phase-1 already used the runtime messageId (sourced from result.file). + expect(phase1.messageId).toBe('turn-2-current-run'); + expect(phase1.status).toBe('pending'); + + /* Phase-2 MUST also route to the current run's messageId so the + * frontend's `useAttachmentHandler` upserts under the same + * messageAttachmentsMap slot as phase-1. Routing to + * `turn-1-original-msg` would land the patch on a stale message + * and leave turn-2's pending chip stuck. */ + expect(phase2.messageId).toBe('turn-2-current-run'); + expect(phase2.file_id).toBe('fid-shared'); + expect(phase2.status).toBe('ready'); + expect(phase2.text).toBe('
'); + expect(phase2.toolCallId).toBe('tool-2'); + }); + + it('phase-2 emit is skipped when finalize resolves to null (no DB update happened)', async () => { + res.headersSent = true; + processCodeOutput.mockResolvedValue({ + file: { + file_id: 'fid-1', + filename: 'data.xlsx', + filepath: '/uploads/data.xlsx', + type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + messageId: 'run-1', + toolCallId: 'tool-1', + status: 'pending', + }, + finalize: jest.fn().mockResolvedValue(null), + }); + + const toolEndCallback = createToolEndCallback({ req, res, artifactPromises }); + const event = makeCodeExecutionEvent({ + runId: 'run-1', + threadId: 'thread-1', + toolCallId: 'tool-1', + fileId: 'fid-1', + name: 'data.xlsx', + }); + await toolEndCallback({ output: event.output }, event.metadata); + await Promise.all(artifactPromises); + await new Promise((resolve) => setImmediate(resolve)); + + // Only phase-1 fired; phase-2 noop'd because finalize returned null. + expect(res.write).toHaveBeenCalledTimes(1); + }); + + it('phase-2 emit is skipped when the response stream has already closed', async () => { + res.headersSent = true; + /* Hand-rolled deferred so we can hold finalize() open until + * AFTER setting `res.writableEnded = true`. Otherwise the mock + * resolves synchronously, the .then() runs in the same microtask + * queue as the artifactPromises await, and writableEnded is set + * too late. */ + let resolveFinalize; + const finalizeDeferred = new Promise((resolve) => { + resolveFinalize = resolve; + }); + processCodeOutput.mockResolvedValue({ + file: { + file_id: 'fid-1', + filename: 'data.xlsx', + filepath: '/uploads/data.xlsx', + type: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + messageId: 'run-1', + toolCallId: 'tool-1', + status: 'pending', + }, + finalize: jest.fn().mockReturnValue(finalizeDeferred), + }); + + const toolEndCallback = createToolEndCallback({ req, res, artifactPromises }); + const event = makeCodeExecutionEvent({ + runId: 'run-1', + threadId: 'thread-1', + toolCallId: 'tool-1', + fileId: 'fid-1', + name: 'data.xlsx', + }); + await toolEndCallback({ output: event.output }, event.metadata); + await Promise.all(artifactPromises); + // Simulate the response closing AFTER phase-1 fires but BEFORE + // phase-2 lands. The frontend's polling path will catch the + // resolved record on its next tick. + res.writableEnded = true; + // Now resolve finalize and let the .then() chain run. + resolveFinalize({ + file_id: 'fid-1', + filename: 'data.xlsx', + messageId: 'run-1', + status: 'ready', + text: '', + textFormat: 'html', + }); + await new Promise((resolve) => setImmediate(resolve)); + + // Phase-1 wrote; phase-2 noop'd because writableEnded. + expect(res.write).toHaveBeenCalledTimes(1); + }); + + it('does not call finalize for a non-office file (no preview expected)', async () => { + res.headersSent = true; + processCodeOutput.mockResolvedValue({ + file: { + file_id: 'fid-txt', + filename: 'note.txt', + filepath: '/uploads/note.txt', + type: 'text/plain', + messageId: 'run-1', + toolCallId: 'tool-1', + // No status — non-office files skip phase-2 entirely. + }, + // No finalize key — caller should not call anything. + }); + + const toolEndCallback = createToolEndCallback({ req, res, artifactPromises }); + const event = makeCodeExecutionEvent({ + runId: 'run-1', + threadId: 'thread-1', + toolCallId: 'tool-1', + fileId: 'fid-txt', + name: 'note.txt', + }); + await toolEndCallback({ output: event.output }, event.metadata); + await Promise.all(artifactPromises); + await new Promise((resolve) => setImmediate(resolve)); + + expect(res.write).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/api/server/controllers/agents/callbacks.js b/api/server/controllers/agents/callbacks.js index e7101edd20..544b2dfd7a 100644 --- a/api/server/controllers/agents/callbacks.js +++ b/api/server/controllers/agents/callbacks.js @@ -427,8 +427,20 @@ function writeAttachmentUpdate(res, streamId, attachment) { * file metadata record. Centralizes the field projection so phase-1 * (pending), phase-2 (ready/failed), and pre-existing legacy callers * all emit the same shape. + * + * `runtimeMessageId` overrides whatever messageId the record carries + * — required for phase-2 emits because `processCodeOutput` intentionally + * preserves the original DB messageId across cross-turn filename reuse + * (so `getCodeGeneratedFiles` can still trace the file back to the + * assistant message that originally produced it). Routing the SSE + * update by the persisted id would land the patch on a stale message + * slot — turn-N's pending placeholder would stay stuck pending while + * turn-1's already-resolved attachment got re-merged. The phase-1 + * emit path mirrors this by reading `result.file.messageId`, which + * `processCodeOutput` runtime-overlays via `Object.assign`. (Codex P1 + * review on PR #12957.) */ -function attachmentFromFileMetadata(fileMetadata, toolCallId) { +function attachmentFromFileMetadata(fileMetadata, toolCallId, runtimeMessageId) { return { file_id: fileMetadata.file_id, filename: fileMetadata.filename, @@ -437,7 +449,7 @@ function attachmentFromFileMetadata(fileMetadata, toolCallId) { width: fileMetadata.width, height: fileMetadata.height, conversationId: fileMetadata.conversationId, - messageId: fileMetadata.messageId, + messageId: runtimeMessageId ?? fileMetadata.messageId, toolCallId: toolCallId ?? fileMetadata.toolCallId, /* Inline text / sanitized HTML preview from the extractor — drives * the file artifact panel's rich preview for DOCX/XLSX/CSV/PPTX. @@ -669,7 +681,16 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null }) writeAttachmentUpdate( res, streamId, - attachmentFromFileMetadata(updated, toolCallId), + /* Pass `metadata.run_id` as the runtime messageId + * override — see attachmentFromFileMetadata's JSDoc. + * `updated` carries the DB-persisted messageId which, + * on cross-turn filename reuse (e.g. agent emits + * `output.csv` again on turn N), is the original + * turn's id rather than the current one. Without this + * override the SSE update routes to the wrong + * message slot and the current turn's pending chip + * stays stuck. */ + attachmentFromFileMetadata(updated, toolCallId, metadata.run_id), ); }) .catch((error) => {