🛠️ refactor: address codex audit findings (wire-shape parity, DRY, defensive catch)

Comprehensive audit on PR #12957. Resolves all valid findings:

- **MAJOR #1 — Wire-shape parity**: phase-1 ships the full `fileMetadata`
  record over SSE; phase-2 was using a tight `attachmentFromFileMetadata`
  projection. Drop the projection and have phase-2 spread `{...updated,
  messageId, toolCallId}` so both events match the long-standing
  legacy phase-1 shape clients depend on.

- **MAJOR #2 — DRY**: extract `runPhase2Finalize({ finalize, fileId,
  onResolved })` into `process.js` (alongside `processCodeOutput` whose
  contract it pairs with). Both `callbacks.js` paths and `tools.js`
  now flow through it. Single catch path eliminates divergence
  surface — the fix landed in 01704d4f0 (cross-turn messageId routing)
  was a symptom of this duplication risk.

- **MINOR #3 — JSDoc accuracy**: `finalizePreview`'s buffer is bounded
  by `fileSizeLimit`, not the 1MB extractor cap. Updated and added a
  note about peak heap from queued buffers.

- **MINOR #4 — Defensive catch**: `runPhase2Finalize`'s catch attempts
  a best-effort `updateFile({ status: 'failed', previewError:
  'unexpected' })` for the file_id, so a programming bug in
  `finalizePreview` doesn't leave the record stuck `'pending'` until
  the next boot-time orphan sweep.

- **NIT #6 — Stale PR refs**: 12952 → 12957 in 3 places.

- **NIT #7 — Schema bound**: `previewError` capped at `maxlength: 200`
  to prevent a future codepath from accidentally persisting a stack
  trace.

Skipped per audit verdict (non-blocking):
- #5 (memory pressure): documented in JSDoc; impl change was reviewer's
  "consider", not actionable.
- #8 (double DB query per poll): low cost, indexed by_id, polling is
  gated narrow.
- #9 (TAttachment cast): the union type is intentional; the casts are
  safe widening, refactoring TAttachment is invasive and out of scope.

Tests: 11 new (7 `runPhase2Finalize` unit tests covering happy path,
null-finalize, throws, double-fail, no-fileId, no-onResolved; +4
wire-shape parity assertions in the existing cross-turn test). 328
backend tests pass; 528 frontend tests pass; lint and typecheck clean.
This commit is contained in:
Danny Avila 2026-05-05 13:46:01 -04:00
parent e80c4ba21c
commit 68ff60c820
9 changed files with 247 additions and 106 deletions

View file

@ -28,6 +28,28 @@ jest.mock('~/server/services/Files/Citations', () => ({
jest.mock('~/server/services/Files/Code/process', () => ({
processCodeOutput: jest.fn(),
/* `runPhase2Finalize` is the runtime pairing for `finalize` (defined
* alongside processCodeOutput in process.js). The callback wires
* phase-2 through it; reproduce the basic happy-path here so the
* SSE-emit assertions still work. The catch/defensive-updateFile
* branch is unit-tested directly against the real helper in
* process.spec.js — exercising it here would add test coupling
* without coverage benefit. */
runPhase2Finalize: ({ finalize, onResolved }) => {
if (typeof finalize !== 'function') {
return;
}
finalize()
.then((updated) => {
if (!updated || !onResolved) {
return;
}
onResolved(updated);
})
.catch(() => {
/* swallowed in the mock — see process.spec.js for catch coverage */
});
},
}));
jest.mock('~/server/services/Tools/credentials', () => ({
@ -427,6 +449,15 @@ describe('createToolEndCallback', () => {
expect(phase2.status).toBe('ready');
expect(phase2.text).toBe('<table></table>');
expect(phase2.toolCallId).toBe('tool-2');
/* Wire-shape parity with phase-1: phase-2 emits the full updated
* record so the client doesn't see one shape on phase-1 and a
* narrower projection on phase-2. (Codex audit on PR #12957
* Finding 1.) */
expect(phase2.filename).toBe('output.csv');
expect(phase2.filepath).toBe('/uploads/output.csv');
expect(phase2.type).toBe('text/csv');
expect(phase2.conversationId).toBe('thread789');
expect(phase2.textFormat).toBe('html');
});
it('phase-2 emit is skipped when finalize resolves to null (no DB update happened)', async () => {

View file

@ -15,7 +15,7 @@ const {
createToolExecuteHandler,
} = require('@librechat/api');
const { processFileCitations } = require('~/server/services/Files/Citations');
const { processCodeOutput } = require('~/server/services/Files/Code/process');
const { processCodeOutput, runPhase2Finalize } = require('~/server/services/Files/Code/process');
const { saveBase64Image } = require('~/server/services/Files/process');
class ModelEndHandler {
@ -422,52 +422,6 @@ function writeAttachmentUpdate(res, streamId, attachment) {
writeAttachment(res, streamId, attachment);
}
/**
* Build the wire-format attachment payload from a phase-1 or phase-2
* 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, runtimeMessageId) {
return {
file_id: fileMetadata.file_id,
filename: fileMetadata.filename,
filepath: fileMetadata.filepath,
type: fileMetadata.type,
width: fileMetadata.width,
height: fileMetadata.height,
conversationId: fileMetadata.conversationId,
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.
* Pass null explicitly (rather than undefined) so the wire format
* is stable for the empty-text gate on the client. */
text: fileMetadata.text ?? null,
/* Trust signal so the client routes office types to the HTML
* bucket only when the backend produced sanitized full-document
* HTML (Codex P1 review on PR #12934). */
textFormat: fileMetadata.textFormat ?? null,
/* Two-phase preview lifecycle: phase-1 emits 'pending', phase-2
* patches to 'ready' or 'failed'. Absent for files that don't
* expect a preview (images, plain text). */
status: fileMetadata.status,
previewError: fileMetadata.previewError,
};
}
/**
*
* @param {Object} params
@ -671,32 +625,29 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null })
* after the HTTP response closes. If the stream is still open
* when phase-2 lands, push an `attachment` update event so
* the UI patches in place; otherwise React Query polling on
* `/api/files/:file_id/preview` picks it up. */
if (typeof finalize === 'function') {
finalize()
.then((updated) => {
if (!updated) {
return;
}
writeAttachmentUpdate(
res,
streamId,
/* 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) => {
logger.error('Error in phase-2 preview extraction:', error);
* `/api/files/:file_id/preview` picks it up.
*
* Spread the full updated record (mirroring the phase-1 emit
* shape) and overlay `messageId`/`toolCallId` from the
* current run. The DB record preserves the original
* `messageId` across cross-turn filename reuse so
* `getCodeGeneratedFiles` can trace the file back to its
* original assistant message; 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 while
* turn-1's already-resolved attachment got re-merged.
* (Codex P1 review on PR #12957.) */
runPhase2Finalize({
finalize,
fileId: fileMetadata.file_id,
onResolved: (updated) => {
writeAttachmentUpdate(res, streamId, {
...updated,
messageId: metadata.run_id,
toolCallId,
});
}
},
});
return fileMetadata;
})().catch((error) => {
logger.error('Error processing code output:', error);
@ -937,26 +888,21 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises })
* so the client merges the resolved record over the pending
* placeholder. Fire-and-forget — survives response close;
* polling covers the post-close gap. */
if (typeof finalize === 'function') {
finalize()
.then((updated) => {
if (!updated) {
return;
}
if (!res || res.writableEnded || !res.headersSent) {
return;
}
writeResponsesAttachment(
res,
tracker,
buildResponsesAttachment(updated, toolCallId),
metadata,
);
})
.catch((error) => {
logger.error('Error in phase-2 preview extraction:', error);
});
}
runPhase2Finalize({
finalize,
fileId: fileMetadata.file_id,
onResolved: (updated) => {
if (!res || res.writableEnded || !res.headersSent) {
return;
}
writeResponsesAttachment(
res,
tracker,
buildResponsesAttachment(updated, toolCallId),
metadata,
);
},
});
return fileMetadata;
})().catch((error) => {

View file

@ -10,7 +10,7 @@ const {
} = require('librechat-data-provider');
const { getRoleByName, createToolCall, getToolCallsByConvo, getMessage } = require('~/models');
const { processFileURL, uploadImageBuffer } = require('~/server/services/Files/process');
const { processCodeOutput } = require('~/server/services/Files/Code/process');
const { processCodeOutput, runPhase2Finalize } = require('~/server/services/Files/Code/process');
const { loadAuthValues } = require('~/server/services/Tools/credentials');
const { loadTools } = require('~/app/clients/tools/util');
@ -208,14 +208,11 @@ const callTool = async (req, res) => {
}
/* This endpoint is non-streaming and its contract is "give
* me the artifacts" — return the phase-1 record immediately
* (with `status: 'pending'` for office buckets) and run phase-2
* extraction in the background. The client polls
* `/api/files/:file_id/preview` for the resolved record. */
if (typeof finalize === 'function') {
finalize().catch((error) => {
logger.error('Error in phase-2 preview extraction:', error);
});
}
* (with `status: 'pending'` for office buckets) and run
* phase-2 extraction in the background. The client polls
* `/api/files/:file_id/preview` for the resolved record.
* No `onResolved` — there's no live stream to write to here. */
runPhase2Finalize({ finalize, fileId: fileMetadata.file_id });
return fileMetadata;
})().catch((error) => {
logger.error('Error processing code output:', error);

View file

@ -179,7 +179,7 @@ describe('GET /files/:file_id/preview', () => {
});
it('defaults to status:ready for legacy records with no status field (back-compat)', async () => {
/* Records pre-dating PR #12952 have no `status` field. The endpoint
/* Records pre-dating PR #12957 have no `status` field. The endpoint
* MUST treat that as 'ready' so legacy code-execution attachments
* don't suddenly poll forever. If text exists, surface it. */
mockGetFiles.mockResolvedValueOnce([

View file

@ -98,8 +98,16 @@ const PHASE_TWO_TIMEOUT_MS = 60_000;
* thing that can lose progress — covered by the boot-time orphan sweep.
*
* @param {object} params
* @param {Buffer} params.buffer - The file contents (held in closure;
* bounded by `MAX_TEXT_EXTRACT_BYTES = 1MB` from the extractor).
* @param {Buffer} params.buffer - The full downloaded file contents,
* bounded by the server's `fileSizeLimit` config (defaults far above
* the 1MB extractor cap). The buffer is captured by the closure
* returned in `{ finalize }`, so when many office files queue behind
* the inner concurrency limiter (cap 2), all queued buffers stay
* resident until each one's slot frees. For a tool result emitting
* N office files, peak heap usage from this path is up to
* `N * fileSizeLimit`. Acceptable for typical agent runs (a handful
* of files at a few hundred KB each); pathological cases are bounded
* by the inner per-file 12s timeout and the outer 60s phase-2 cap.
* @param {string} params.leafName - Basename for classification.
* @param {string} params.mimeType - Detected/inferred MIME.
* @param {string} params.category - Classifier output.
@ -155,6 +163,66 @@ const finalizePreview = async ({ buffer, leafName, mimeType, category, file_id }
}
};
/**
* Run the background phase-2 thunk returned by `processCodeOutput` and
* route the resolved record to the caller's emit logic. Shared between
* `callbacks.js` (chat-completions + Open Responses) and `tools.js`
* (direct tool endpoint) so the fire-and-forget pattern doesn't drift
* across callsites.
*
* `onResolved` receives the post-update DB record and is the only piece
* that varies — chat-completions writes the legacy `attachment` SSE
* event, Open Responses writes the spec-shaped `librechat:attachment`
* event with a sequence number, and the direct tool endpoint has no
* stream to write to (caller passes a no-op).
*
* The catch path is the safety net for unexpected programming errors
* inside `finalizePreview`. The function is designed to never throw
* (extraction and DB failures are translated to `status: 'failed'`
* inside it), but a ref error or future regression would otherwise
* leave the DB record stuck at `'pending'` until the boot-time orphan
* sweep — potentially hours away on a stable server. We attempt a
* best-effort `updateFile` to mark the record `'failed'` with
* `previewError: 'unexpected'` so the UI stops polling and the
* next-turn LLM context surfaces the failure.
*
* @param {object} params
* @param {(() => Promise<object | null>) | undefined} params.finalize - The
* thunk returned by `processCodeOutput`. No-op when undefined.
* @param {string | undefined} params.fileId - DB key for the failure
* marker; if absent the catch only logs.
* @param {(updated: object) => void} [params.onResolved] - Called once
* on success with the post-update record.
*/
const runPhase2Finalize = ({ finalize, fileId, onResolved }) => {
if (typeof finalize !== 'function') {
return;
}
finalize()
.then((updated) => {
if (!updated || !onResolved) {
return;
}
onResolved(updated);
})
.catch((error) => {
logger.error('Error in phase-2 preview extraction:', error);
if (!fileId) {
return;
}
updateFile({
file_id: fileId,
status: 'failed',
previewError: 'unexpected',
}).catch((updateErr) => {
logger.error(
`[runPhase2Finalize] also failed to mark ${fileId} as failed after error:`,
updateErr,
);
});
});
};
/**
* Process code execution output files — downloads and saves both images
* and non-image files. All files are saved to local storage with
@ -782,4 +850,5 @@ module.exports = {
getSessionInfo,
processCodeOutput,
readSandboxFile,
runPhase2Finalize,
};

View file

@ -973,6 +973,98 @@ describe('Code Process', () => {
});
});
describe('runPhase2Finalize', () => {
/* The runtime pairing for `processCodeOutput`'s `finalize` thunk.
* `finalizePreview` is designed to never throw (translates errors
* to `status: 'failed'` internally). The helper's catch is the
* safety net for unexpected programming errors that would
* otherwise leave the DB record stuck at `status: 'pending'`
* forever — we attempt a best-effort `updateFile` to mark it
* `'failed'` with `previewError: 'unexpected'` so the UI stops
* polling and the next-turn LLM context surfaces the failure.
* (Codex audit on PR #12957 Finding 4.) */
const { runPhase2Finalize } = require('./process');
const { updateFile } = require('~/models');
beforeEach(() => {
updateFile.mockReset();
updateFile.mockResolvedValue({});
});
it('is a no-op when finalize is undefined (non-office files)', () => {
expect(() =>
runPhase2Finalize({ finalize: undefined, fileId: 'fid-1', onResolved: jest.fn() }),
).not.toThrow();
expect(updateFile).not.toHaveBeenCalled();
});
it('calls onResolved with the resolved record on success', async () => {
const onResolved = jest.fn();
const finalize = jest
.fn()
.mockResolvedValue({ file_id: 'fid-1', status: 'ready', text: '<x/>' });
runPhase2Finalize({ finalize, fileId: 'fid-1', onResolved });
await new Promise((resolve) => setImmediate(resolve));
expect(onResolved).toHaveBeenCalledWith(
expect.objectContaining({ file_id: 'fid-1', status: 'ready' }),
);
expect(updateFile).not.toHaveBeenCalled();
});
it('skips onResolved when finalize resolves to null (DB write failed inside finalizePreview)', async () => {
const onResolved = jest.fn();
const finalize = jest.fn().mockResolvedValue(null);
runPhase2Finalize({ finalize, fileId: 'fid-1', onResolved });
await new Promise((resolve) => setImmediate(resolve));
expect(onResolved).not.toHaveBeenCalled();
});
it('marks the record as failed (previewError: "unexpected") when finalize throws', async () => {
const onResolved = jest.fn();
const finalize = jest.fn().mockRejectedValue(new Error('unexpected ref error'));
runPhase2Finalize({ finalize, fileId: 'fid-boom', onResolved });
await new Promise((resolve) => setImmediate(resolve));
await new Promise((resolve) => setImmediate(resolve));
expect(onResolved).not.toHaveBeenCalled();
expect(updateFile).toHaveBeenCalledWith({
file_id: 'fid-boom',
status: 'failed',
previewError: 'unexpected',
});
expect(logger.error).toHaveBeenCalledWith(
'Error in phase-2 preview extraction:',
expect.any(Error),
);
});
it('logs but does not throw when the defensive updateFile itself fails', async () => {
const onResolved = jest.fn();
const finalize = jest.fn().mockRejectedValue(new Error('original error'));
updateFile.mockRejectedValueOnce(new Error('mongo down'));
runPhase2Finalize({ finalize, fileId: 'fid-doublefail', onResolved });
await new Promise((resolve) => setImmediate(resolve));
await new Promise((resolve) => setImmediate(resolve));
expect(onResolved).not.toHaveBeenCalled();
// Two logger.error calls: one for the original throw, one for the failed mark.
expect(logger.error.mock.calls.some((c) => /also failed to mark/.test(c[0]))).toBe(true);
});
it('does not attempt the defensive updateFile when fileId is missing', async () => {
const finalize = jest.fn().mockRejectedValue(new Error('boom'));
runPhase2Finalize({ finalize, fileId: undefined });
await new Promise((resolve) => setImmediate(resolve));
expect(updateFile).not.toHaveBeenCalled();
});
it('skips onResolved gracefully when caller omits it (e.g., tools.js direct endpoint)', async () => {
const finalize = jest.fn().mockResolvedValue({ file_id: 'fid-1', status: 'ready' });
// No onResolved — non-streaming caller.
expect(() => runPhase2Finalize({ finalize, fileId: 'fid-1' })).not.toThrow();
await new Promise((resolve) => setImmediate(resolve));
expect(updateFile).not.toHaveBeenCalled();
});
});
describe('readSandboxFile', () => {
/**
* `readSandboxFile` shells `cat <file_path>` through the sandbox

View file

@ -286,7 +286,7 @@ export const fileDownload = (userId: string, fileId: string) =>
`${BASE_URL}/api/files/download/${userId}/${fileId}`;
/* Two-phase code-execution preview lifecycle endpoint. Returns
* `{ status, text?, textFormat?, previewError? }` so the frontend can
* poll while phase-2 HTML extraction is in flight. See PR #12952. */
* poll while phase-2 HTML extraction is in flight. See PR #12957. */
export const filePreview = (fileId: string) =>
`${BASE_URL}/api/files/${encodeURIComponent(fileId)}/preview`;
export const fileConfig = () => `${BASE_URL}/api/files/config`;

View file

@ -412,7 +412,7 @@ export const getFiles = (): Promise<f.TFile[]> => {
* - `previewError` only when `status === 'failed'`.
*
* Called from `useFilePreview` with React Query's `refetchInterval`
* gated on `status === 'pending'` and `isSubmitting`. See PR #12952.
* gated on `status === 'pending'` and `isSubmitting`. See PR #12957.
*/
export const getFilePreview = (fileId: string): Promise<f.TFilePreview> => {
return request.get(endpoints.filePreview(fileId));

View file

@ -75,6 +75,12 @@ const file: Schema<IMongoFile> = new Schema(
},
previewError: {
type: String,
/* Bounded to short machine-readable reasons (`'timeout'`,
* `'parser-error'`, `'orphaned'`, `'unexpected'`). Cap prevents a
* future codepath from accidentally persisting a stack trace or
* full error message — would bloat documents and ship a wall of
* text into the UI tooltip. */
maxlength: 200,
},
context: {
type: String,