From 6c6c72def764765687950235aefaa48acad45aa3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 6 May 2026 03:04:19 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9A=80=20feat:=20Decouple=20File=20Attach?= =?UTF-8?q?ment=20Persistence=20from=20Preview=20Rendering=20(#12957)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🗂️ feat: add `status` lifecycle to file records for two-phase previews Schema and model foundation for decoupling the agent's final response from CPU-heavy office-format HTML extraction. - `MongoFile.status: 'pending' | 'ready' | 'failed'` (indexed) and `previewError?: string` mirror the lifecycle: phase-1 emits the file record at `pending` so the response is unblocked; phase-2 transitions to `ready` (with text/textFormat) or `failed` (with previewError) in the background. Absent for legacy records — clients treat that as `ready` for back-compat. - Mirror types added to `TFile` in data-provider so frontend cache consumers see the new fields. - New `sweepOrphanedPreviews(maxAgeMs)` method on the file model recovers stale `pending` records left behind by a process restart mid-extraction; transitions them to `failed` with `previewError: 'orphaned'`. Cheap because `status` is indexed. * ⚡ feat: two-phase code-execution preview flow (unblocks final response) The agent's final response no longer waits on CPU-heavy office HTML extraction. Phase-1 (download + storage save + DB record at `status: 'pending'`) is awaited as before; phase-2 (extract + `updateFile`) runs in the background with a hard 60s ceiling. Three flows, all funneling through `processCodeOutput` and updated to the new `{ file, finalize? }` return shape: - `callbacks.js` (chat-completions + Open Responses streaming): emit the phase-1 attachment immediately (carries `status: 'pending'` for office buckets so the UI shows "preparing preview…"), then fire-and-forget `finalize()`. If the SSE stream is still open when phase-2 lands, push an `attachment` update event with the same `file_id` so the client merges over the placeholder in place. - `tools.js` direct endpoint: same split — return the phase-1 metadata immediately, run extraction in the background. Client polls for the resolved record. `finalize()` wraps the existing 12s per-render timeout in a 60s outer `withTimeout`. The HTML-or-null contract from #12934 is preserved: office types that fail extraction transition to `status: 'failed'` with `previewError: 'parser-error' | 'timeout'` rather than falling back to plain text (would be an XSS vector). Promises continue running after the HTTP response closes (Node doesn't kill them). The boot-time orphan sweep covers the only case that loses progress — actual process restart mid-extraction. `primeFiles` annotates the agent's `toolContext` line for prior-turn files: `(preview not yet generated)` for pending, `(preview unavailable: )` for failed. The model can volunteer "you can still download it" instead of pretending the preview is fine. `hasOfficeHtmlPath` exported from `@librechat/api` so `processCodeOutput` can decide whether a file expects a preview at all. * 🔍 feat: `GET /api/files/:file_id/preview` endpoint and boot orphan sweep - New `GET /api/files/:file_id/preview` route returns `{ status, text?, textFormat?, previewError? }`. The frontend's `useFilePreview` React Query hook polls this while phase-2 is in flight, then auto-stops on terminal status. ACL identical to the download route (reuses `fileAccess` middleware). Defaults `status` to `'ready'` for legacy records so back-compat is implicit. `text` only included when `status === 'ready'` and non-null — preserves the HTML-or-null security contract from #12934. - `sweepOrphanedPreviews()` invoked on boot in both `server/index.js` and `server/experimental.js`. Recovers any `pending` records left behind by a process restart mid-extraction (the only case the in-process two-phase flow can't handle on its own). Fire-and-forget so a transient sweep failure doesn't block startup. * 🖥️ feat: frontend two-phase preview consumer (polling + UI states) Wires the React side to the new lifecycle so the user sees what's happening with their file while phase-2 extraction runs in the background and after the response stream closes. - `useAttachmentHandler` upserts by `file_id` (was append-only) so the phase-2 SSE update event merges over the pending placeholder in place. Lightweight attachments without a `file_id` (web_search / file_search citations) keep the legacy append path. - `useFilePreview(file_id)` React Query hook with `refetchInterval: (data) => data?.status === 'pending' ? 2500 : false` so polling auto-stops on the first terminal response without the caller having to flip `enabled`. - `useAttachmentPreviewSync(attachment)` bridges polled data into `messageAttachmentsMap`. Polling enabled iff `status === 'pending' && isAnySubmitting` — per the design ask: active polling while the LLM is still generating, then quiet. Process-restart and post-stream cases are covered by polling on the next interaction. - `Attachment.tsx` renders a small `PreviewStatusIndicator` (spinner + "Preparing preview…" for pending, alert icon + "Preview unavailable" for failed) inside `FileAttachment`. Download button stays fully functional in both states. Two new English locale keys. - Data-provider scaffolding: `TFilePreview` type, `endpoints.filePreview`, `dataService.getFilePreview`, `QueryKeys.filePreview`. * 🧪 fix: stub `useAttachmentPreviewSync` in pre-existing Attachment test mocks The new `useAttachmentPreviewSync` hook is called unconditionally inside `FileAttachment` (added in the prior commit). Two pre-existing test files mock `~/hooks` to provide `useLocalize` only — the un-mocked preview hook reference resolved to undefined and crashed render with `(0 , _hooks.useAttachmentPreviewSync) is not a function` on the Ubuntu/Windows CI runners. Fix is local to the test mocks: add a no-op stub that returns `{ status: 'ready' }` so the component renders the legacy chip path. The two-phase preview behavior itself has its own dedicated suites (`useAttachmentHandler.spec.tsx`, `useAttachmentPreviewSync.spec.tsx`). * 🐛 fix: route phase-2 attachment update to current-run messageId 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. * 🛠️ 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. * 🛡️ refactor: address codex P1+P2 + rename to drop phase-1/2 jargon Codex round 2 review on PR #12957 caught two race conditions and one recovery gap, all triggered by cross-turn filename reuse (`claimCodeFile` intentionally returns the same `file_id` for the same `(filename, conversationId)` across turns). Plus naming cleanup the user requested — internal "phase 1 / phase 2" vocabulary leaks across sprints, replace it everywhere with terms describing what's actually happening. P1 — stale render overwrites newer revision (process.js) Two turns reusing `output.csv` share a `file_id`. If turn-1's background render resolves AFTER turn-2's persist step, the unconditional `updateFile` writes turn-1's stale text/status over turn-2's pending placeholder. Fix: stamp a fresh `previewRevision` UUID on every emit, thread it through `finalizePreview`, and make the commit conditional via a new optional `extraFilter` argument on `updateFile` (`{ previewRevision: }`). The defensive `updateFile` in `runPreviewFinalize`'s catch uses the same guard so a programming error from an older render also can't override a newer turn. P1 — stale React Query cache on pending remount (queries.ts) Same root cause from the frontend side. Cache key `[QueryKeys.filePreview, file_id]` may hold a prior turn's `'ready'` payload; with `refetchOnMount: false` and the polling gate on `pending`, polling never starts for the new placeholder. Fix: `useAttachmentHandler` invalidates that query whenever an attachment with a `file_id` arrives. Both initial-emit and update events trigger invalidation — uniform gate. P2 — quick-restart orphans skipped by boot sweep (files.js) Boot `sweepOrphanedPreviews` uses a 5-min cutoff for multi-instance safety. A crash + restart inside the cutoff leaves `pending` records that never get touched again. Fix: lazy sweep inside the preview endpoint — if a polled record is `pending` and `updatedAt` is older than 5 min, mark it `failed:orphaned` on the spot before responding. Conditional on the same `updatedAt` we observed so a concurrent legitimate update wins. Cheap, bounded by user activity. Naming cleanup - `runPhase2Finalize` → `runPreviewFinalize` - `PHASE_TWO_TIMEOUT_MS` → `PREVIEW_FINALIZE_TIMEOUT_MS` - All `phase-1` / `phase-2` / `two-phase` prose replaced with "the immediate emit", "the deferred render", "the persist step", "the deferred preview", etc. Skill-feature `phase 1/2` references (different feature) left alone. Tests: 10 new (4 lazy-sweep × preview endpoint, 3 cache-invalidation × useAttachmentHandler, 3 extraFilter × updateFile data-schemas). Backend 332/332, frontend 531/531, data-schemas 37/37, lint clean. * 🛠️ refactor: address comprehensive review (round 3) — stale-cache MAJOR + 3 minors Comprehensive review on PR #12957 caught a P1 follow-on bug from the prior `invalidateQueries` fix, plus 3 maintainability findings. MAJOR: stale React Query cache not actually fixed by `invalidateQueries` The previous fix called `invalidateQueries` to flush stale cached preview data on cross-turn filename reuse. But `useFilePreview` had `refetchOnMount: false`, which made the new observer read the stale-marked 'ready' data without refetching. The polling `refetchInterval` then evaluated against stale 'ready' → returned `false` → polling never started → user stuck on stale content. Fix (belt-and-suspenders): a) `useAttachmentHandler` switched to `removeQueries` — drops the cache entry entirely so the next mount has nothing to read and must fetch. b) `useFilePreview` no longer sets `refetchOnMount: false`, so the React Query default (`true`) kicks in — second line of defense if any future codepath observes stale data before the handler has a chance to evict. MINOR: `finalizePreview` JSDoc missing `previewRevision` param Added with explanation of the conditional update guard. MINOR: asymmetric stream-writable guard between SSE protocols Chat-completions delegated the gate to `writeAttachmentUpdate`; Open Responses inlined `!res.writableEnded && res.headersSent`. Extracted `isStreamWritable(res, streamId)` predicate; both paths + `writeAttachmentUpdate` now share the single source of truth. NIT: `(data as Partial).file_id` cast repeated 4 times Extracted to a `fileId` local at the top of the handler. Tests: existing 9 invalidate-tests rewritten as remove-tests; +1 new lock-in test asserts removeQueries is called and invalidateQueries is NOT (regression guard against round-3 finding). 332 backend pass, 532 frontend pass, lint clean. Skipped findings (deferred / acceptable): - MINOR: post-submission pending state has no auto-recovery — the `isAnySubmitting` polling gate was the user's explicit design; LLM context surfaces failed/pending so the model can volunteer. Worth a follow-up if real users hit it. - NIT: double DB query per preview poll — reviewer marked acceptable; changing `fileAccess` middleware is out of scope. * 🛡️ test: address comprehensive review NITs (initial-emit guard + isStreamWritable coverage) NIT — chat-completions initial emit skips writableEnded check The Open Responses initial emit was switched to use the new `isStreamWritable` predicate in the round-3 commit, but the chat-completions initial emit kept the older narrower check (`streamId || res.headersSent`). On a client disconnect mid-stream (`writableEnded === true`) it would still hit `res.write` and raise `ERR_STREAM_WRITE_AFTER_END` — caught by the outer IIFE catch but logged as noise. Switch this site to `isStreamWritable` too so both initial-emit paths share the same gate as the deferred update emits. NIT — `isStreamWritable` not directly unit-tested The predicate was only covered indirectly via the deferred-preview SSE tests (writableEnded skip, headersSent check). Export from `callbacks.js` and add 5 parametric tests pinning down each branch (streamId truthy, res null, !headersSent, writableEnded, happy path) so a future condition addition can't silently regress. * 🐛 fix: stuck "Preparing preview…" + inline the chip subtitle Two related fixes for a stuck-spinner bug a user reported in manual testing of PR #12957. **Stuck spinner (the bug)** The deferred preview render can complete a few seconds AFTER the SSE stream closes (typical case: PPTX render finishes ~3s after the LLM emits FINAL). When that happens, the SSE update is silently dropped (`isStreamWritable` returns false on a closed stream) and polling is the only recovery path. The earlier polling gate was `status === 'pending' && isAnySubmitting`, which mirrored the original design intent ("only query while the LLM is still generating"). But `isAnySubmitting` flips false the moment the model emits FINAL — milliseconds before the deferred render commits. Polling never runs, the chip stays "Preparing preview…" forever even though the DB has `status: 'ready'` with valid HTML. Drop the `isAnySubmitting` part of the gate. `useFilePreview`'s `refetchInterval` is already a function-form that returns `false` on the first terminal response, so polling auto-stops within one tick of resolution. The server-side render ceiling (60s) plus the lazy sweep in the preview endpoint cap the worst case to ~24 polls per pending attachment. Polling itself never blocks UX — the gate's purpose was "don't waste cycles", and capping by terminal status is the correct expression of that. **Inline the chip subtitle (the visual)** The previous design rendered "Preparing preview…" as a loose-feeling spinner+text BELOW the file chip. The chip itself looked done while a floating annotation said it wasn't. `FileContainer` gains an optional `subtitle?: ReactNode` prop that overrides the default file-type label. `Attachment.tsx` passes a `PreviewStatusSubtitle` (spinner + "Preparing preview…" / alert + "Preview unavailable") into that slot when the file's preview is pending or failed. The chip footprint stays identical to its `'ready'` form — just the second row swaps from "PowerPoint Presentation" to the status indicator. No floating element, no layout shift. Tests: regression test pinning down "polling stays enabled after the LLM finishes" so a future revert can't reintroduce the stuck-spinner bug. Existing FileContainer tests pass unchanged (subtitle override is opt-in). 522 frontend tests pass; lint clean. * 🐛 fix: deferred-preview survives reload + matches artifact card chrome Fixes the remaining stuck-pending case after the polling gate fix: on a reloaded conversation, message.attachments come from the DB frozen at the immediate-persist `status: 'pending'`, but `messageAttachmentsMap` is empty because no SSE handler ever fired for that messageId. Polling now INSERTS a new live entry when no record matches the file_id, and `useAttachments` merges live entries onto DB entries by file_id so the resolved text/textFormat reach `artifactTypeForAttachment` and the chip routes through the proper PanelArtifact card. Also replaces the small file chip used during the pending state with a PreviewPlaceholderCard that mirrors ToolArtifactCard chrome, so the transition to the resolved PanelArtifact no longer reshapes the UI. * ✨ feat: auto-open panel when deferred preview resolves pending→ready The legacy auto-open path is gated only on `isSubmitting`, so an office-file preview that resolves *after* the SSE stream closes would render in place but never auto-open the panel — even though that's exactly the moment the result becomes meaningful to the user. Adds a per-file_id one-shot signal that `useAttachmentPreviewSync` flips on the pending→ready edge; `ToolArtifactCard` consumes it on mount and auto-opens regardless of submission state. The signal is *only* set on the actual transition (history loads of pre-resolved files don't trigger it) and is consumed once (panel close + reopen on the same card stays user-controlled). * 🐛 fix: drop placeholder Terminal overlay + scope auto-open to fresh resolutions Two fixes for issues spotted in manual testing of the deferred-preview auto-open feature: 1. PreviewPlaceholderCard was passing `file={attachment}` to FilePreview, which triggered SourceIcon's Terminal overlay (`metadata.fileIdentifier` is set on every code-execution file). The artifact card itself doesn't show that overlay; the placeholder shouldn't either, so the pending→resolved transition is visually seamless. 2. The `previewJustResolved` flag flipped on every pending→ready transition observed by the polling hook — including stale-pending DB records that resolve via the first poll on a *history load*. Conversations whose immediate-persist snapshot left attachments at `status: 'pending'` would yank the panel open every revisit. Adds `mountedDuringStreamRef` to the hook (mirroring ToolArtifactCard) so the flag fires only when the hook itself was mounted during an active turn — preserving the pre-PR contract that the panel only auto-opens for results the user is actively waiting on, never for history. * 🐛 fix: don't downgrade preview to failed when only the SSE emit throws Codex P2 finding on PR #12957: the original chain placed `.catch` after `.then(onResolved)`, so a throw inside `onResolved` (transport-side errors — SSE write race after stream close, an emitter listener throwing) would propagate into the finalize catch and persist `status: 'failed'` / `previewError: 'unexpected'`. That surfaced "preview unavailable" in the UI for a perfectly valid file, and degraded next-turn LLM context to reflect a non-existent failure. Wraps `onResolved` in its own try/catch so emit errors are logged but do not affect the file's persisted status. Extraction success and emit success are now independent: if extraction succeeds and `finalizePreview` writes the terminal status, the polling layer / next page load surfaces the resolved preview even if this turn's SSE emit didn't land. * 🛡️ fix: run boot-time orphan sweep under system tenant context Codex P2 finding on PR #12957: `File` is tenant-isolated, so under `TENANT_ISOLATION_STRICT=true` the boot-time `sweepOrphanedPreviews` threw `[TenantIsolation] Query attempted without tenant context in strict mode` and the recovery path silently failed every restart. Stale `status: 'pending'` records would be stuck until a user happened to poll the preview endpoint and trigger the lazy sweep — which only covers the file the user is currently looking at, not the bulk candidate set the boot sweep is designed to recover. Wraps the sweep in `runAsSystem(...)` in both boot paths (`api/server/index.js` and `api/server/experimental.js`) and pins the contract with regression tests in `file.spec.ts` — one test asserts the bare call throws under strict mode, the other asserts the `runAsSystem`-wrapped call succeeds. * 🧹 chore: trim verbose comments from previous commit * 🧹 chore: address review findings (dead branch, lazy-sweep cutoff, stale JSDoc) - finalizePreview: drop unreachable !isOfficeBucket branch (caller already gates on hasOfficeHtmlPath, so this path is always office) - preview endpoint: drop lazy-sweep cutoff from 5min to 2min — anything past the 60s render ceiling is definitively orphaned, and per-request sweep can be tighter than the per-instance boot sweep - strip stale `isSubmitting` references from JSDoc in 3 spots (the client-side gate was removed in 9a65840) Skipped: function-length (#3) and client-side polling cap (#4) — refactors without correctness/perf wins; remaining NITs. * 🧹 fix: trim 1 query off pending polls + clear stale lifecycle on cross-shape updates - Preview endpoint: reuse fileAccess middleware's record for the lifecycle check; only re-fetch with text on the terminal ready response. Cuts the typical poll lifecycle from 2(N+1) to N+1 queries, since the vast majority of polls hit while pending and don't need text at all. - processCodeOutput non-office branch: explicitly null out status, previewError, previewRevision (codex P2). Without this, an update at the same (filename, conversationId) where the prior emit was an office file leaves stale lifecycle fields and the client renders the wrong state for the now non-office artifact. - Tests: rewire preview.spec mocks for the new shape, add boundary test pinning the 2min cutoff, add regression test for the cross-shape update. * 🐛 fix: keep polling on transient errors but cap permanently-broken endpoint Codex P2: the previous `data?.status === 'pending' ? 2500 : false` gate killed polling on the first transient error. With `retry: false`, a 500 left `data` undefined, the callback returned false, and the chip was stuck "Preparing preview…" forever — exactly the bug the polling layer was supposed to recover from. Inverts the gate: stop on terminal success (`ready`/`failed`) or after 5 consecutive errors. Transient errors keep retrying; a permanently broken endpoint caps at ~12.5s instead of polling forever. Predicate extracted as `previewRefetchInterval` for direct unit testing without fighting React Query's timer machinery. * ✨ feat: render pending-preview files in their own row Pending deferred-preview chips now bucket into a separate row above the resolved attachments — reads as "this is still happening" rather than mixing with completed downloads. Once status flips to ready, the chip re-buckets into panelArtifacts; failed re-buckets into the file row alongside other downloads. * 🎨 fix: render pending-preview chips in the panel-artifact row, not the file row Previous bucketing put pending chips in the file row (since `artifactTypeForAttachment` returns null for empty-text records). The pending placeholder is a future panel artifact — sharing the row keeps the chip in place when it resolves instead of jumping rows. Plain files still get their own row. * 🐛 fix: phase-1 SSE replay must not regress a resolved attachment Codex P1: useEventHandlers.finalHandler iterates responseMessage.attachments at stream end and dispatches each through the attachment handler. Those records are the immediate-persist snapshot (status:pending, text:null) — if a deferred update has already moved the same file_id to ready/failed, the existing merge let the pending fields win and downgraded the resolved record. Result: chip flickers back to pending and polling restarts until the lazy sweep corrects. Pin the terminal lifecycle fields (status, text, textFormat, previewError) when existing is ready/failed and incoming is pending. Other field updates still go through. * 🐛 fix: track preview-poll error cap outside React Query state Codex P2: the previous cap relied on `query.state.fetchFailureCount`, but React Query v4's reducer resets that to 0 on every fetch dispatch (the `'fetch'` action). With `retry: false`, each failed poll left count at 1 and the next dispatch reset it back to 0, so the `>= 5` branch never fired and a permanently-broken endpoint polled forever. Track consecutive errors in a module-level Map keyed by file_id, incremented in a thin `fetchFilePreview` wrapper around the data service call. The Map is cleared on success and on cap-stop, so memory is bounded by in-flight pending file_ids per session. --- .../agents/__tests__/callbacks.spec.js | 284 ++++++++++ api/server/controllers/agents/callbacks.js | 194 +++++-- api/server/controllers/tools.js | 19 +- api/server/experimental.js | 14 +- api/server/index.js | 14 +- api/server/routes/files/files.js | 80 +++ api/server/routes/files/preview.spec.js | 372 +++++++++++++ .../Code/__tests__/process-traversal.spec.js | 7 + api/server/services/Files/Code/process.js | 401 ++++++++++++-- .../services/Files/Code/process.spec.js | 496 +++++++++++++++++- .../Chat/Input/Files/FileContainer.tsx | 21 +- .../Messages/Content/Parts/Attachment.tsx | 165 +++++- .../Content/Parts/ToolArtifactCard.tsx | 54 +- .../Parts/__tests__/ArtifactRouting.test.tsx | 139 +++++ .../Parts/__tests__/TextAttachment.test.tsx | 4 + .../__tests__/previewRefetchInterval.spec.ts | 97 ++++ client/src/data-provider/Files/queries.ts | 80 +++ .../useAttachmentPreviewSync.spec.tsx | 442 ++++++++++++++++ client/src/hooks/Files/index.ts | 1 + .../hooks/Files/useAttachmentPreviewSync.ts | 197 +++++++ .../__tests__/useAttachments.spec.tsx | 129 +++++ client/src/hooks/Messages/useAttachments.ts | 43 +- .../__tests__/useAttachmentHandler.spec.tsx | 279 ++++++++++ client/src/hooks/SSE/useAttachmentHandler.ts | 54 ++ client/src/locales/en/translation.json | 2 + client/src/store/artifacts.ts | 26 + packages/api/src/files/code/extract.ts | 7 +- packages/data-provider/src/api-endpoints.ts | 5 + packages/data-provider/src/data-service.ts | 16 + packages/data-provider/src/keys.ts | 1 + packages/data-provider/src/types/files.ts | 36 ++ .../data-schemas/src/methods/file.spec.ts | 214 ++++++++ packages/data-schemas/src/methods/file.ts | 51 +- packages/data-schemas/src/schema/file.ts | 30 ++ packages/data-schemas/src/types/file.ts | 29 + 35 files changed, 3861 insertions(+), 142 deletions(-) create mode 100644 api/server/routes/files/preview.spec.js create mode 100644 client/src/data-provider/Files/__tests__/previewRefetchInterval.spec.ts create mode 100644 client/src/hooks/Files/__tests__/useAttachmentPreviewSync.spec.tsx create mode 100644 client/src/hooks/Files/useAttachmentPreviewSync.ts create mode 100644 client/src/hooks/Messages/__tests__/useAttachments.spec.tsx create mode 100644 client/src/hooks/SSE/__tests__/useAttachmentHandler.spec.tsx diff --git a/api/server/controllers/agents/__tests__/callbacks.spec.js b/api/server/controllers/agents/__tests__/callbacks.spec.js index 8bd711f9c6..0ba20d409c 100644 --- a/api/server/controllers/agents/__tests__/callbacks.spec.js +++ b/api/server/controllers/agents/__tests__/callbacks.spec.js @@ -28,6 +28,28 @@ jest.mock('~/server/services/Files/Citations', () => ({ jest.mock('~/server/services/Files/Code/process', () => ({ processCodeOutput: jest.fn(), + /* `runPreviewFinalize` is the runtime pairing for `finalize` (defined + * alongside processCodeOutput in process.js). The callback wires + * the deferred render 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. */ + runPreviewFinalize: ({ 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', () => ({ @@ -326,4 +348,266 @@ describe('createToolEndCallback', () => { expect(res.write).not.toHaveBeenCalled(); }); }); + + describe('code execution deferred-preview emit', () => { + /* The deferred-preview code-execution flow emits the attachment twice over + * SSE: the initial emit with `status: 'pending'` and the current run's + * messageId, the deferred render with the resolved record. The preview update 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 preview-update 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('the preview update 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: the initial emit (pending) and the deferred render (ready). + expect(res.write).toHaveBeenCalledTimes(2); + const phase1 = parseSseAttachment(res.write.mock.calls[0]); + const phase2 = parseSseAttachment(res.write.mock.calls[1]); + + // Initial emit already used the runtime messageId (sourced from result.file). + expect(phase1.messageId).toBe('turn-2-current-run'); + expect(phase1.status).toBe('pending'); + + /* The preview update MUST also route to the current run's messageId so the + * frontend's `useAttachmentHandler` upserts under the same + * messageAttachmentsMap slot as the initial emit. 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'); + /* Wire-shape parity with the initial emit: preview update emits the full updated + * record so the client doesn't see one shape on the initial emit and a + * narrower projection on the deferred render. (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('the preview update 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 the initial emit fired; preview update noop'd because finalize returned null. + expect(res.write).toHaveBeenCalledTimes(1); + }); + + it('the preview update 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 the initial emit fires but BEFORE + // the deferred render 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)); + + // Initial emit wrote; preview update 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 the deferred render 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); + }); + }); +}); + +describe('isStreamWritable', () => { + /* Direct parametric coverage of the predicate that gates SSE writes + * in both the chat-completions and Open Responses callbacks. The + * existing deferred-preview tests cover this indirectly via the + * `writeAttachmentUpdate` writableEnded path; these tests pin down + * each individual branch so a future modification (e.g. adding a + * new condition) can't silently regress. + * (Comprehensive review NIT on PR #12957.) */ + const { isStreamWritable } = require('../callbacks'); + + it('returns true when streamId is truthy regardless of res state', () => { + /* Resumable mode writes go to the job emitter; res state is + * irrelevant. Even a closed res with no headers should not block. */ + expect(isStreamWritable(null, 'stream-1')).toBe(true); + expect(isStreamWritable({ headersSent: false, writableEnded: true }, 'stream-1')).toBe(true); + expect(isStreamWritable(undefined, 'stream-1')).toBe(true); + }); + + it('returns false when streamId is falsy and res is null/undefined', () => { + expect(isStreamWritable(null, null)).toBe(false); + expect(isStreamWritable(undefined, null)).toBe(false); + }); + + it('returns false when headers have not been sent yet', () => { + expect(isStreamWritable({ headersSent: false, writableEnded: false }, null)).toBe(false); + }); + + it('returns false when the stream has already ended', () => { + expect(isStreamWritable({ headersSent: true, writableEnded: true }, null)).toBe(false); + }); + + it('returns true on the happy path: headers sent, not ended, no streamId', () => { + expect(isStreamWritable({ headersSent: true, writableEnded: false }, null)).toBe(true); + }); }); diff --git a/api/server/controllers/agents/callbacks.js b/api/server/controllers/agents/callbacks.js index dc8f6cc65a..c9612c6b62 100644 --- a/api/server/controllers/agents/callbacks.js +++ b/api/server/controllers/agents/callbacks.js @@ -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, runPreviewFinalize } = require('~/server/services/Files/Code/process'); const { saveBase64Image } = require('~/server/services/Files/process'); class ModelEndHandler { @@ -397,6 +397,55 @@ function writeAttachment(res, streamId, attachment) { } } +/** + * Predicate: is it safe to push an SSE write to the caller right now? + * + * In `streamId` (resumable) mode, writes go to the job emitter and the + * `res` state is irrelevant — always writable. + * + * In standard mode, the caller's `res` must have headers sent (the + * stream has been opened) and not yet be `writableEnded` (the response + * hasn't closed). Writing to a closed stream raises + * `ERR_STREAM_WRITE_AFTER_END`. + * + * Used by deferred preview emits in both `createToolEndCallback` + * (chat-completions) and `createResponsesToolEndCallback` (Open + * Responses) so the gate logic stays in one place. (Comprehensive + * review #3 on PR #12957.) + */ +function isStreamWritable(res, streamId) { + if (streamId) { + return true; + } + return !!res && res.headersSent && !res.writableEnded; +} + +/** + * Emit an update for an attachment that was previously sent with + * `status: 'pending'`. Fire-and-forget: if the response stream has + * already closed (the agent finished generating before the deferred + * preview resolved) the frontend's React Query polling on + * `/api/files/:file_id/preview` picks up the resolved record on its + * next tick. Skipping the write in that case avoids + * `ERR_STREAM_WRITE_AFTER_END`. + * + * Reuses the `attachment` SSE event name with a discriminated payload: + * the frontend's `useAttachmentHandler` upserts by `file_id`, so a + * second event with the same id and `status: 'ready' | 'failed'` + * overwrites the pending placeholder in place. No new event type, no + * new client listener. + * + * @param {ServerResponse} res + * @param {string | null} streamId + * @param {Object} attachment - Updated attachment payload (must carry `file_id`). + */ +function writeAttachmentUpdate(res, streamId, attachment) { + if (!isStreamWritable(res, streamId)) { + return; + } + writeAttachment(res, streamId, attachment); +} + /** * * @param {Object} params @@ -556,14 +605,15 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null }) continue; } const { id, name } = file; + const toolCallId = output.tool_call_id; artifactPromises.push( (async () => { - const fileMetadata = await processCodeOutput({ + const result = await processCodeOutput({ req, id, name, messageId: metadata.run_id, - toolCallId: output.tool_call_id, + toolCallId, conversationId: metadata.thread_id, /** * Use the FILE's session_id (storage session), not the @@ -583,15 +633,54 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null }) */ session_id: file.session_id ?? output.artifact.session_id, }); - if (!streamId && !res.headersSent) { - return fileMetadata; - } - + const fileMetadata = result?.file ?? null; + const finalize = result?.finalize; if (!fileMetadata) { return null; } - - writeAttachment(res, streamId, fileMetadata); + /* Initial emit: ship the attachment to the client immediately + * (carries `status: 'pending'` for office buckets so the UI + * shows "preparing preview…"). The agent's response stops + * blocking on extraction here. + * + * Use the shared `isStreamWritable` predicate rather than the + * narrower `streamId || res.headersSent` check that lived + * here before — a client disconnect mid-stream + * (`res.writableEnded`) would otherwise hit `res.write` and + * raise `ERR_STREAM_WRITE_AFTER_END` (caught by the outer + * IIFE catch but logged as noise). Same gate the Responses + * path uses below. */ + if (isStreamWritable(res, streamId)) { + writeAttachment(res, streamId, fileMetadata); + } + /* Deferred preview rendering: extraction continues running + * even after the HTTP response closes. If the stream is still + * open when the preview resolves, push an `attachment` + * update event so the UI patches in place; otherwise React + * Query polling on `/api/files/:file_id/preview` picks it up. + * + * Spread the full updated record (mirroring the initial 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 update SSE 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.) */ + runPreviewFinalize({ + finalize, + fileId: fileMetadata.file_id, + previewRevision: result?.previewRevision, + onResolved: (updated) => { + writeAttachmentUpdate(res, streamId, { + ...updated, + messageId: metadata.run_id, + toolCallId, + }); + }, + }); return fileMetadata; })().catch((error) => { logger.error('Error processing code output:', error); @@ -782,14 +871,15 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises }) continue; } const { id, name } = file; + const toolCallId = output.tool_call_id; artifactPromises.push( (async () => { - const fileMetadata = await processCodeOutput({ + const result = await processCodeOutput({ req, id, name, messageId: metadata.run_id, - toolCallId: output.tool_call_id, + toolCallId, conversationId: metadata.thread_id, /** * Use the FILE's session_id (storage session), not the @@ -809,38 +899,45 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises }) */ session_id: file.session_id ?? output.artifact.session_id, }); - + const fileMetadata = result?.file ?? null; + const finalize = result?.finalize; if (!fileMetadata) { return null; } - // For Responses API, emit attachment during streaming - if (res.headersSent && !res.writableEnded) { - const attachment = { - file_id: fileMetadata.file_id, - filename: fileMetadata.filename, - type: fileMetadata.type, - url: fileMetadata.filepath, - width: fileMetadata.width, - height: fileMetadata.height, - tool_call_id: output.tool_call_id, - /* Inline text / sanitized HTML preview from - * `extractCodeArtifactText` — 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 can route .docx/.csv/.xlsx/ - * .pptx attachments to the office HTML buckets only when - * the backend produced sanitized full-document HTML. - * RAG-uploaded plain text from mammoth.extractRawText - * arrives without this flag and must NOT be injected as - * HTML — Codex P1 review on PR #12934. */ - textFormat: fileMetadata.textFormat ?? null, - }; - writeResponsesAttachment(res, tracker, attachment, metadata); + /* Initial emit (Open Responses extension format). The agent's + * response no longer blocks on extraction. */ + if (isStreamWritable(res, null)) { + writeResponsesAttachment( + res, + tracker, + buildResponsesAttachment(fileMetadata, toolCallId), + metadata, + ); } + /* Deferred preview rendering: extract HTML in the background + * and emit a follow-up `librechat:attachment` with the same + * `file_id` so the client merges the resolved record over the + * pending placeholder. Fire-and-forget — survives response + * close; polling covers the post-close gap. */ + runPreviewFinalize({ + finalize, + fileId: fileMetadata.file_id, + previewRevision: result?.previewRevision, + onResolved: (updated) => { + if (!isStreamWritable(res, null)) { + return; + } + writeResponsesAttachment( + res, + tracker, + buildResponsesAttachment(updated, toolCallId), + metadata, + ); + }, + }); + return fileMetadata; })().catch((error) => { logger.error('Error processing code output:', error); @@ -851,6 +948,28 @@ function createResponsesToolEndCallback({ req, res, tracker, artifactPromises }) }; } +/** + * Project a file metadata record into the Open Responses attachment + * shape. Mirrors the legacy inline projection but adds `status` and + * `previewError` so deferred preview updates carry the lifecycle + * signal the client uses to upsert by `file_id`. + */ +function buildResponsesAttachment(fileMetadata, toolCallId) { + return { + file_id: fileMetadata.file_id, + filename: fileMetadata.filename, + type: fileMetadata.type, + url: fileMetadata.filepath, + width: fileMetadata.width, + height: fileMetadata.height, + tool_call_id: toolCallId, + text: fileMetadata.text ?? null, + textFormat: fileMetadata.textFormat ?? null, + status: fileMetadata.status, + previewError: fileMetadata.previewError, + }; +} + const ALLOWED_LOG_LEVELS = new Set(['debug', 'info', 'warn', 'error']); function agentLogHandler(_event, data) { @@ -906,6 +1025,7 @@ module.exports = { agentLogHandlerObj, getDefaultHandlers, createToolEndCallback, + isStreamWritable, markSummarizationUsage, buildSummarizationHandlers, createResponsesToolEndCallback, diff --git a/api/server/controllers/tools.js b/api/server/controllers/tools.js index 8124894584..07be1210c1 100644 --- a/api/server/controllers/tools.js +++ b/api/server/controllers/tools.js @@ -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, runPreviewFinalize } = require('~/server/services/Files/Code/process'); const { loadAuthValues } = require('~/server/services/Tools/credentials'); const { loadTools } = require('~/app/clients/tools/util'); @@ -192,7 +192,7 @@ const callTool = async (req, res) => { const { id, name } = file; artifactPromises.push( (async () => { - const fileMetadata = await processCodeOutput({ + const result = await processCodeOutput({ req, id, name, @@ -201,11 +201,22 @@ const callTool = async (req, res) => { conversationId, session_id: artifact.session_id, }); - + const fileMetadata = result?.file ?? null; + const finalize = result?.finalize; if (!fileMetadata) { return null; } - + /* This endpoint is non-streaming and its contract is "give + * me the artifacts" — return the persisted record immediately + * (with `status: 'pending'` for office buckets) and run the + * preview render 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. */ + runPreviewFinalize({ + finalize, + fileId: fileMetadata.file_id, + previewRevision: result?.previewRevision, + }); return fileMetadata; })().catch((error) => { logger.error('Error processing code output:', error); diff --git a/api/server/experimental.js b/api/server/experimental.js index cd594c1696..b12b9deffe 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -10,7 +10,7 @@ const express = require('express'); const passport = require('passport'); const compression = require('compression'); const cookieParser = require('cookie-parser'); -const { logger } = require('@librechat/data-schemas'); +const { logger, runAsSystem } = require('@librechat/data-schemas'); const mongoSanitize = require('express-mongo-sanitize'); const { isEnabled, @@ -26,7 +26,12 @@ const initializeOAuthReconnectManager = require('./services/initializeOAuthRecon const createValidateImageRequest = require('./middleware/validateImageRequest'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); const { updateInterfacePermissions: updateInterfacePerms } = require('@librechat/api'); -const { getRoleByName, updateAccessPermissions, seedDatabase } = require('~/models'); +const { + getRoleByName, + updateAccessPermissions, + seedDatabase, + sweepOrphanedPreviews, +} = require('~/models'); const { checkMigrations } = require('./services/start/migration'); const initializeMCPs = require('./services/initializeMCPs'); const configureSocialLogins = require('./socialLogins'); @@ -220,6 +225,11 @@ if (cluster.isMaster) { /** Seed database (idempotent) */ await seedDatabase(); + /* Mirrors `server/index.js`; `runAsSystem` for tenant-isolated File. */ + runAsSystem(sweepOrphanedPreviews).catch((err) => { + logger.error('[sweepOrphanedPreviews] Background sweep failed:', err); + }); + /** Initialize app configuration */ const appConfig = await getAppConfig(); initializeFileStorage(appConfig); diff --git a/api/server/index.js b/api/server/index.js index d798f1a166..6bc4a131e6 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -25,7 +25,12 @@ const { } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); const initializeOAuthReconnectManager = require('./services/initializeOAuthReconnectManager'); -const { getRoleByName, updateAccessPermissions, seedDatabase } = require('~/models'); +const { + getRoleByName, + updateAccessPermissions, + seedDatabase, + sweepOrphanedPreviews, +} = require('~/models'); const { capabilityContextMiddleware } = require('./middleware/roles/capabilities'); const createValidateImageRequest = require('./middleware/validateImageRequest'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); @@ -69,6 +74,13 @@ const startServer = async () => { } await runAsSystem(seedDatabase); + /* Recover stuck `status: 'pending'` records from a crash mid-render. + * `runAsSystem` is required — `File` is tenant-isolated and strict + * mode rejects unscoped queries. Lazy sweep in the preview endpoint + * covers anything younger than the boot cutoff. */ + runAsSystem(sweepOrphanedPreviews).catch((err) => { + logger.error('[sweepOrphanedPreviews] Background sweep failed:', err); + }); const appConfig = await getAppConfig({ baseOnly: true }); initializeFileStorage(appConfig); await runAsSystem(async () => { diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 5c26f65b81..285f783909 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -295,6 +295,86 @@ router.get('/code/download/:session_id/:fileId', async (req, res) => { } }); +/* Lazy-sweep cutoff: pending records older than this are marked failed + * on the next poll. 2min is well past the 60s render ceiling, so any + * `pending` past it is definitively orphaned. Tighter than the boot + * sweep (5min) since this runs per-request, not per-instance. */ +const PREVIEW_LAZY_SWEEP_CUTOFF_MS = 2 * 60 * 1000; + +/** + * Poll the lifecycle status of a code-execution file's inline preview. + * + * Deferred-preview flow: the immediate persist step writes the file + * record at `status: 'pending'`; the background render transitions + * it to `'ready'` (with `text` + `textFormat`) or `'failed'` (with + * `previewError`). The frontend's `useFilePreview` React Query hook + * polls this endpoint at ~2.5s intervals while `status === 'pending'`, + * then auto-stops on terminal status. + * + * Returns the smallest viable shape: + * - `status` always present (defaults to `'ready'` for legacy records + * that never had the field — clients treat absent as ready). + * - `text` and `textFormat` only when status is 'ready' AND text + * is non-null (preserves the security contract from PR #12934 — + * office bucket files MUST NOT receive plain-text fallbacks). + * - `previewError` only when status is 'failed'. + * + * Lazy-sweeps stale `pending` records on the spot — see + * `PREVIEW_LAZY_SWEEP_CUTOFF_MS` for the rationale. + * + * Reuses the `fileAccess` middleware so ACL is identical to download. + * + * @route GET /files/:file_id/preview + */ +router.get('/:file_id/preview', fileAccess, async (req, res) => { + try { + const { file_id } = req.params; + /* `fileAccess` already fetched the record (sans `text`, the default + * projection drops it). Reuse for the lifecycle check; only re-fetch + * with `text` on a terminal ready response — the typical lifecycle + * is N pending polls + 1 ready, so this avoids ~N redundant text + * reads per file. */ + let file = req.fileAccess.file; + /* Lazy sweep: if stuck `pending` past the cutoff, mark `failed` + * conditional on the observed `updatedAt` (concurrent legitimate + * updates win). */ + if (file.status === 'pending' && file.updatedAt instanceof Date) { + const ageMs = Date.now() - file.updatedAt.getTime(); + if (ageMs > PREVIEW_LAZY_SWEEP_CUTOFF_MS) { + const swept = await db.updateFile( + { file_id, status: 'failed', previewError: 'orphaned' }, + { status: 'pending', updatedAt: file.updatedAt }, + ); + if (swept) { + file = swept; + logger.info( + `[/files/:file_id/preview] Lazy-swept orphaned pending record ${file_id} (age ${Math.round(ageMs / 1000)}s)`, + ); + } + } + } + /* Default to 'ready' for back-compat: legacy records pre-date the + * field, and non-office files never get a status set on persist. */ + const status = file.status ?? 'ready'; + const payload = { file_id, status }; + if (status === 'ready') { + const withText = await db.findFileById(file_id); + if (withText?.text != null) { + payload.text = withText.text; + payload.textFormat = withText.textFormat ?? null; + } + } else if (status === 'failed' && file.previewError) { + payload.previewError = file.previewError; + } + return res.status(200).json(payload); + } catch (error) { + logger.error('[/files/:file_id/preview] Error fetching preview status:', error); + return res + .status(500) + .json({ error: 'Internal Server Error', message: 'Failed to fetch preview status' }); + } +}); + router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { try { const { userId, file_id } = req.params; diff --git a/api/server/routes/files/preview.spec.js b/api/server/routes/files/preview.spec.js new file mode 100644 index 0000000000..f86c128d4b --- /dev/null +++ b/api/server/routes/files/preview.spec.js @@ -0,0 +1,372 @@ +/** + * Coverage for the new GET /files/:file_id/preview endpoint. + * + * Deferred-preview code-execution flow: the immediate persist step + * emits a file record at `status: 'pending'`; the background render + * transitions it to `'ready'` (with text) or `'failed'` (with + * previewError). The frontend polls this endpoint until status is + * terminal. This suite asserts the response shape across all four + * states (pending, ready, failed, legacy/back-compat) and the auth + * boundary (404 vs 403). + */ + +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn(), debug: jest.fn(), error: jest.fn(), info: jest.fn() }, + SystemCapabilities: {}, +})); + +jest.mock('@librechat/api', () => ({ + refreshS3FileUrls: jest.fn(), + resolveUploadErrorMessage: jest.fn(), + verifyAgentUploadPermission: jest.fn(), +})); + +const mockFindFileById = jest.fn(); +const mockGetFiles = jest.fn(); +const mockUpdateFile = jest.fn(); +jest.mock('~/models', () => ({ + findFileById: (...args) => mockFindFileById(...args), + getFiles: (...args) => mockGetFiles(...args), + updateFile: (...args) => mockUpdateFile(...args), + getAgents: jest.fn().mockResolvedValue([]), + batchUpdateFiles: jest.fn(), +})); + +jest.mock('~/server/services/Files/process', () => ({ + filterFile: jest.fn(), + processFileUpload: jest.fn(), + processDeleteRequest: jest.fn(), + processAgentFileUpload: jest.fn(), +})); + +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: jest.fn(() => ({})), +})); + +jest.mock('~/server/controllers/assistants/helpers', () => ({ + getOpenAIClient: jest.fn(), +})); + +jest.mock('~/server/middleware/roles/capabilities', () => ({ + hasCapability: jest.fn(() => (_req, _res, next) => next()), +})); + +jest.mock('~/server/services/PermissionService', () => ({ + checkPermission: jest.fn(() => (_req, _res, next) => next()), + getEffectivePermissions: jest.fn().mockResolvedValue(0), +})); + +jest.mock('~/server/services/Files', () => ({ + hasAccessToFilesViaAgent: jest.fn(), +})); + +jest.mock('~/server/utils/files', () => ({ + cleanFileName: (name) => name, +})); + +jest.mock('~/cache', () => ({ + getLogStores: jest.fn(() => ({ get: jest.fn(), set: jest.fn() })), +})); + +const express = require('express'); +const request = require('supertest'); +const filesRouter = require('./files'); + +/** + * Mount the router with a per-request user injector so we can simulate + * a logged-in user without spinning up the full auth stack. + */ +function buildApp({ user = { id: 'user-123', role: 'user' } } = {}) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = user; + req.config = { fileStrategy: 'local' }; + next(); + }); + app.use('/files', filesRouter); + return app; +} + +const OWNER_USER_ID = 'user-123'; + +describe('GET /files/:file_id/preview', () => { + beforeEach(() => { + mockFindFileById.mockReset(); + mockGetFiles.mockReset(); + mockUpdateFile.mockReset(); + }); + + it('returns 404 when the file does not exist (auth check fails first via fileAccess)', async () => { + /* `fileAccess` middleware does its own getFiles lookup and returns + * 404 before our handler ever runs. This test asserts the boundary + * lives there, not that the handler duplicates the check. */ + mockGetFiles.mockResolvedValueOnce([]); + const res = await request(buildApp()).get('/files/missing-id/preview'); + expect(res.status).toBe(404); + expect(res.body).toMatchObject({ error: 'Not Found' }); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('returns 403 when the requester does not own the file and has no agent-based access', async () => { + /* fileAccess returns 403 — the file exists but belongs to someone + * else and no agent grants access. The preview handler should + * never run. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'someone-elses', user: 'other-user', filename: 'x.xlsx' }, + ]); + const res = await request(buildApp()).get('/files/someone-elses/preview'); + expect(res.status).toBe(403); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('returns status:pending without text/textFormat while the deferred render is in flight', async () => { + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-pending', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'pending', + }, + ]); + const res = await request(buildApp()).get('/files/fid-pending/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-pending', status: 'pending' }); + /* Pending must NOT leak `text` and must NOT trigger the text re-fetch. */ + expect(res.body).not.toHaveProperty('text'); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('returns status:ready with text + textFormat when the deferred render succeeded', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-ready', user: OWNER_USER_ID, filename: 'data.xlsx', status: 'ready' }, + ]); + /* Text is fetched only on the terminal ready response. */ + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-ready', + text: '
1
', + textFormat: 'html', + }); + const res = await request(buildApp()).get('/files/fid-ready/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-ready', + status: 'ready', + text: '
1
', + textFormat: 'html', + }); + }); + + it('returns status:failed with previewError when the deferred render errored', async () => { + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-failed', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'failed', + previewError: 'parser-error', + }, + ]); + const res = await request(buildApp()).get('/files/fid-failed/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-failed', + status: 'failed', + previewError: 'parser-error', + }); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('defaults to status:ready for legacy records with no status field (back-compat)', async () => { + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-legacy', + user: OWNER_USER_ID, + filename: 'old.csv', + // status intentionally absent + }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-legacy', + text: 'csv,header\n1,2', + textFormat: 'text', + }); + const res = await request(buildApp()).get('/files/fid-legacy/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-legacy', + status: 'ready', + text: 'csv,header\n1,2', + textFormat: 'text', + }); + }); + + it('returns status:ready with no text when the record is ready but text is null (binary/oversized)', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-binary', user: OWNER_USER_ID, filename: 'image.bin' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-binary', + text: null, + textFormat: null, + }); + const res = await request(buildApp()).get('/files/fid-binary/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-binary', status: 'ready' }); + }); + + it('returns ready with no text when ready record was deleted between fileAccess and text fetch', async () => { + /* `fileAccess` saw the record but the concurrent delete removed it + * before the text fetch. Surface ready-without-text rather than + * 500 — the client routes to download-only and stops polling. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-race', user: OWNER_USER_ID, filename: 'data.xlsx', status: 'ready' }, + ]); + mockFindFileById.mockResolvedValueOnce(null); + const res = await request(buildApp()).get('/files/fid-race/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-race', status: 'ready' }); + }); + + it('returns 500 with a stable shape if the text fetch throws unexpectedly', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-boom', user: OWNER_USER_ID, filename: 'data.xlsx', status: 'ready' }, + ]); + mockFindFileById.mockRejectedValueOnce(new Error('mongo down')); + const res = await request(buildApp()).get('/files/fid-boom/preview'); + expect(res.status).toBe(500); + expect(res.body).toMatchObject({ error: 'Internal Server Error' }); + }); + + describe('lazy sweep for stale pending records', () => { + /* The boot-time `sweepOrphanedPreviews` only runs once at startup + * with a 5-min cutoff. A backend crash + quick restart can leave + * `pending` records younger than 5 min that never get touched + * again. This endpoint sweeps them on the spot whenever a polling + * request lands on one — the user is exactly the consumer who + * cares, so on-demand sweep is the right shape. (Codex P2 review + * on PR #12957.) */ + const STALE_MS = 6 * 60 * 1000; + const FRESH_MS = 30 * 1000; + + it('marks a stale pending record as failed:orphaned and returns the swept state', async () => { + const updatedAt = new Date(Date.now() - STALE_MS); + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-stale', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'pending', + updatedAt, + }, + ]); + mockUpdateFile.mockResolvedValueOnce({ + file_id: 'fid-stale', + status: 'failed', + previewError: 'orphaned', + }); + + const res = await request(buildApp()).get('/files/fid-stale/preview'); + + expect(mockUpdateFile).toHaveBeenCalledWith( + { file_id: 'fid-stale', status: 'failed', previewError: 'orphaned' }, + { status: 'pending', updatedAt }, + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-stale', + status: 'failed', + previewError: 'orphaned', + }); + }); + + it('does NOT sweep a fresh pending record (within the cutoff window)', async () => { + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-fresh', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'pending', + updatedAt: new Date(Date.now() - FRESH_MS), + }, + ]); + + const res = await request(buildApp()).get('/files/fid-fresh/preview'); + + expect(mockUpdateFile).not.toHaveBeenCalled(); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-fresh', status: 'pending' }); + }); + + it('sweeps a record past the 2min cutoff but below the 5min boot-sweep threshold', async () => { + /* Pins the cutoff change from 5min to 2min — without this, a + * future revert wouldn't fail the suite. */ + const updatedAt = new Date(Date.now() - 3 * 60 * 1000); + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-mid', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'pending', + updatedAt, + }, + ]); + mockUpdateFile.mockResolvedValueOnce({ + file_id: 'fid-mid', + status: 'failed', + previewError: 'orphaned', + }); + + const res = await request(buildApp()).get('/files/fid-mid/preview'); + + expect(mockUpdateFile).toHaveBeenCalled(); + expect(res.body).toEqual({ + file_id: 'fid-mid', + status: 'failed', + previewError: 'orphaned', + }); + }); + + it('does NOT sweep a stale ready record (only pending qualifies)', async () => { + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-ready', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'ready', + updatedAt: new Date(Date.now() - STALE_MS), + }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-ready', + text: 'final', + textFormat: 'html', + }); + + const res = await request(buildApp()).get('/files/fid-ready/preview'); + + expect(mockUpdateFile).not.toHaveBeenCalled(); + expect(res.body).toMatchObject({ status: 'ready', text: 'final' }); + }); + + it('falls through to the original pending payload if the conditional sweep loses the race', async () => { + const updatedAt = new Date(Date.now() - STALE_MS); + mockGetFiles.mockResolvedValueOnce([ + { + file_id: 'fid-race', + user: OWNER_USER_ID, + filename: 'data.xlsx', + status: 'pending', + updatedAt, + }, + ]); + mockUpdateFile.mockResolvedValueOnce(null); + + const res = await request(buildApp()).get('/files/fid-race/preview'); + + expect(mockUpdateFile).toHaveBeenCalled(); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-race', status: 'pending' }); + }); + }); +}); diff --git a/api/server/services/Files/Code/__tests__/process-traversal.spec.js b/api/server/services/Files/Code/__tests__/process-traversal.spec.js index 099794821d..b6fcc2636f 100644 --- a/api/server/services/Files/Code/__tests__/process-traversal.spec.js +++ b/api/server/services/Files/Code/__tests__/process-traversal.spec.js @@ -32,6 +32,13 @@ jest.mock('@librechat/api', () => { * mock returns null in lockstep with the null `text` above so * downstream consumers don't see a phantom format. */ getExtractedTextFormat: jest.fn(() => null), + /* Pass-through `withTimeout`: this suite asserts traversal sanitization, + * not deferred preview timing. */ + withTimeout: async (promise) => promise, + /* These traversal cases all use non-office filenames — keep the + * inline (non-finalize) path so existing assertions on a single + * createFile call hold. */ + hasOfficeHtmlPath: jest.fn(() => false), codeServerHttpAgent: new http.Agent({ keepAlive: false }), codeServerHttpsAgent: new https.Agent({ keepAlive: false }), }; diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 4348d2766f..4efc938223 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -3,8 +3,10 @@ const { v4 } = require('uuid'); const { logger } = require('@librechat/data-schemas'); const { getCodeBaseURL } = require('@librechat/agents'); const { + withTimeout, getBasePath, logAxiosError, + hasOfficeHtmlPath, sanitizeArtifactPath, flattenArtifactPath, createAxiosInstance, @@ -69,8 +71,235 @@ const createDownloadFallback = ({ }; /** - * Process code execution output files - downloads and saves both images and non-image files. - * All files are saved to local storage with fileIdentifier metadata for code env re-upload. + * Hard ceiling on the deferred preview rendering (HTML extraction + DB + * update). The inner office-render path already has its own 12s timeout + * and a concurrency-limited queue; this is the outer guard that catches + * pathological cases where queue wait + render + DB write would + * otherwise hang the file in `status: 'pending'` indefinitely. + * + * If the timeout fires the record is updated to `status: 'failed'` + * with `previewError: 'timeout'` and the UI shows download-only. + */ +const PREVIEW_FINALIZE_TIMEOUT_MS = 60_000; + +/** + * Render the inline HTML preview for a code-execution file (or plain + * text for non-office buckets that still benefit from caching), then + * atomically transition the DB record to `status: 'ready'` (with + * `text`/`textFormat`) or `status: 'failed'` (with `previewError`). + * + * Decoupled from `processCodeOutput` so the agent's final response is + * not blocked on potentially slow office rendering. The caller fires + * this without awaiting; promises continue running after the HTTP + * response closes (Node doesn't kill them) and the frontend learns of + * completion via the `attachment` update SSE event (if the stream is + * still open) or via React Query polling otherwise. Process restart + * is the only thing that can lose progress — covered by the boot-time + * orphan sweep. + * + * @param {object} params + * @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 render cap. + * @param {string} params.leafName - Basename for classification. + * @param {string} params.mimeType - Detected/inferred MIME. + * @param {string} params.category - Classifier output. + * @param {string} params.file_id - The DB record key for the update. + * @param {string} [params.previewRevision] - Generation marker stamped + * by the immediate persist step. The DB commit is conditional on + * this — if a newer emit (cross-turn filename reuse) has rotated + * the revision before this render finishes, `updateFile` returns + * null and the stale render is silently discarded rather than + * overwriting the newer record. + * @returns {Promise} The post-update record on + * success; `null` if the DB update itself failed (extraction failure + * is reflected as `status: 'failed'`, not a thrown error) or if the + * `previewRevision` guard rejected the write. + */ +const finalizePreview = async ({ + buffer, + leafName, + mimeType, + category, + file_id, + previewRevision, +}) => { + let text = null; + let previewError; + try { + text = await withTimeout( + extractCodeArtifactText(buffer, leafName, mimeType, category), + PREVIEW_FINALIZE_TIMEOUT_MS, + `Preview extraction exceeded ${PREVIEW_FINALIZE_TIMEOUT_MS}ms`, + ); + } catch (_error) { + /* `extractCodeArtifactText` swallows its own errors and returns null, + * so the only way to reach here is a `withTimeout` rejection — i.e. + * the queue + render combined exceeded the outer 60s ceiling. */ + previewError = 'timeout'; + logger.warn( + `[finalizePreview] ${file_id}: extraction timed out after ${PREVIEW_FINALIZE_TIMEOUT_MS}ms`, + ); + } + /* HTML-or-null contract (PR #12934): null result on an office file + * must NOT fall back to plain text — surface as failed. Caller gates + * on `hasOfficeHtmlPath`, so reaching here always means office. */ + const textFormat = getExtractedTextFormat(leafName, mimeType, text); + const failed = text == null; + const status = failed ? 'failed' : 'ready'; + if (failed && !previewError) { + previewError = 'parser-error'; + } + try { + /* Conditional update: commit only if `previewRevision` still + * matches what the immediate persist step stamped. If a newer + * emit has rotated the revision (cross-turn filename reuse), + * `updateFile` returns null and the stale render is silently + * discarded. (Codex P1 review on PR #12957.) */ + const updated = await updateFile( + { + file_id, + text, + textFormat, + status, + previewError: failed ? previewError : null, + }, + previewRevision ? { previewRevision } : undefined, + ); + if (!updated && previewRevision) { + logger.debug( + `[finalizePreview] ${file_id}: stale render skipped — newer emit has superseded revision ${previewRevision}`, + ); + } + return updated; + } catch (error) { + logger.error( + `[finalizePreview] ${file_id}: failed to persist preview result: ${error?.message ?? error}`, + ); + return null; + } +}; + +/** + * Run the background `finalize` 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` ONLY. 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. + * + * `onResolved` errors are deliberately isolated in their own try/catch. + * Without that isolation, a transient transport-side failure (SSE write + * race after the stream closed, an emitter listener throwing) would + * propagate into the finalize catch and downgrade an *already-resolved* + * record to `failed` with `previewError: 'unexpected'` — surfacing + * "preview unavailable" in the UI even though extraction succeeded + * and the file is on disk. The emit failure is logged but the DB + * record stays at whatever `finalizePreview` wrote (typically + * `'ready'`), so the polling layer / next page load still sees the + * resolved preview. + * + * @param {object} params + * @param {(() => Promise) | 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 {string | undefined} [params.previewRevision] - Generation + * marker stamped by the immediate persist step. The defensive + * `updateFile` in the catch is conditional on this — if a newer + * emit has rotated the revision, the stale failure marker is + * silently discarded so a programming error from an older render + * doesn't override a newer turn's record. + * @param {(updated: object) => void} [params.onResolved] - Called once + * on success with the post-update record. + */ +const runPreviewFinalize = ({ finalize, fileId, previewRevision, onResolved }) => { + if (typeof finalize !== 'function') { + return; + } + finalize() + .then((updated) => { + if (!updated || !onResolved) { + return; + } + /* Isolated try/catch — a throw inside `onResolved` (transport-side + * SSE write race, emitter listener error) MUST NOT propagate to + * the outer `.catch`, which would downgrade an already-resolved + * record to `failed` with `previewError: 'unexpected'`. + * Extraction succeeded at this point and `finalizePreview` has + * already persisted the terminal status; the polling layer / next + * page load will surface the resolved preview even if this turn's + * SSE emit didn't land. */ + try { + onResolved(updated); + } catch (emitError) { + logger.error( + `[runPreviewFinalize] onResolved threw for ${fileId}; record stays at the finalized status:`, + emitError, + ); + } + }) + .catch((error) => { + logger.error('Error rendering deferred preview:', error); + if (!fileId) { + return; + } + updateFile( + { + file_id: fileId, + status: 'failed', + previewError: 'unexpected', + }, + previewRevision ? { previewRevision } : undefined, + ).catch((updateErr) => { + logger.error( + `[runPreviewFinalize] 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 + * `fileIdentifier` metadata for code env re-upload. + * + * Returns a two-part shape so callers can ship the attachment to the + * client immediately and run preview extraction in the background: + * - `file`: persisted metadata (file is on disk, downloadable, and + * has `status: 'pending'` if a preview is still being rendered). + * - `finalize` (optional): a thunk returning the deferred preview + * result promise. Present only when an inline HTML preview is + * expected (office buckets — DOCX/XLSX/XLS/ODS/CSV/PPTX). Caller + * decides whether to await or fire-and-forget. + * + * Existing fallback paths (size limit, missing storage strategy, error + * catch) return `{ file }` with no `finalize` — there's nothing to + * extract. + * * @param {ServerRequest} params.req - The Express request object. * @param {string} params.id - The file ID from the code environment. * @param {string} params.name - The filename. @@ -78,7 +307,7 @@ const createDownloadFallback = ({ * @param {string} params.session_id - The code execution session ID. * @param {string} params.conversationId - The current conversation ID. * @param {string} params.messageId - The current message ID. - * @returns {Promise} The file metadata or undefined if an error occurs. + * @returns {Promise<{ file: MongoFile & { messageId: string, toolCallId: string }, finalize?: () => Promise }>} */ const processCodeOutput = async ({ req, @@ -123,15 +352,17 @@ const processCodeOutput = async ({ logger.warn( `[processCodeOutput] File "${name}" (${(buffer.length / megabyte).toFixed(2)} MB) exceeds size limit of ${(fileSizeLimit / megabyte).toFixed(2)} MB, falling back to download URL`, ); - return createDownloadFallback({ - id, - name, - messageId, - toolCallId, - session_id, - conversationId, - expiresAt: currentDate.getTime() + 86400000, - }); + return { + file: createDownloadFallback({ + id, + name, + messageId, + toolCallId, + session_id, + conversationId, + expiresAt: currentDate.getTime() + 86400000, + }), + }; } const fileIdentifier = `${session_id}/${id}`; @@ -206,7 +437,7 @@ const processCodeOutput = async ({ metadata: { fileIdentifier }, }; await createFile(file, true); - return Object.assign(file, { messageId, toolCallId }); + return { file: Object.assign(file, { messageId, toolCallId }) }; } const { saveBuffer } = getStrategyFunctions(appConfig.fileStrategy); @@ -214,15 +445,17 @@ const processCodeOutput = async ({ logger.warn( `[processCodeOutput] saveBuffer not available for strategy ${appConfig.fileStrategy}, falling back to download URL`, ); - return createDownloadFallback({ - id, - name, - messageId, - toolCallId, - session_id, - conversationId, - expiresAt: currentDate.getTime() + 86400000, - }); + return { + file: createDownloadFallback({ + id, + name, + messageId, + toolCallId, + session_id, + conversationId, + expiresAt: currentDate.getTime() + 86400000, + }), + }; } const detectedType = await determineFileType(buffer, true); @@ -271,18 +504,17 @@ const processCodeOutput = async ({ * what it would have gotten with the old flat-name flow. */ const leafName = path.basename(safeName); const category = classifyCodeArtifact(leafName, mimeType); - const text = await extractCodeArtifactText(buffer, leafName, mimeType, category); - /* `textFormat` accompanies `text` so the client can gate - * office-HTML-bucket routing on a trusted signal — clients MUST - * NOT inject `text` into the iframe as HTML unless `textFormat === - * 'html'`. RAG-uploaded `.docx` etc. arrive with plain text from - * mammoth.extractRawText and would otherwise be hijacked by the - * extension-based office routing into the HTML-injection path - * (Codex P1 review on PR #12934). null on extract failure — the - * client treats absence as 'text' for safety. */ - const textFormat = getExtractedTextFormat(leafName, mimeType, text); - const file = { + /* Office-bucket files (DOCX/XLSX/XLS/ODS/CSV/PPTX) route through + * `bufferToOfficeHtml` which is CPU-heavy. Persist the record now + * with `status: 'pending'` and `text: null` so the agent's response + * isn't blocked, then return a `finalize` thunk the caller can run + * in the background. Non-office files have cheap or no extraction + * — run it inline so the caller gets a fully-resolved record + * without juggling a finalize step. */ + const expectsPreview = hasOfficeHtmlPath(leafName, mimeType); + + const baseFile = { file_id, filepath, messageId: persistedMessageId, @@ -298,16 +530,70 @@ const processCodeOutput = async ({ context: FileContext.execute_code, usage: isUpdate ? (claimed.usage ?? 0) + 1 : 1, createdAt: isUpdate ? claimed.createdAt : formattedDate, - // Always set `text` explicitly (string or null) so that an update which - // produces a binary or oversized artifact clears any previously cached - // text — `createFile` uses findOneAndUpdate with $set semantics, which - // would otherwise leave a stale value behind. + }; + + if (expectsPreview) { + /* Persist with `status: 'pending'` and explicit + * `text: null` / `textFormat: null` so an update that previously + * had cached text gets cleared. The deferred finalize transitions + * to 'ready' (with text/textFormat) or 'failed' (with + * previewError). + * + * `previewRevision` is a fresh UUID stamped on every emit. The + * deferred finalize's `updateFile` is conditional on this — if + * a newer turn (cross-turn filename reuse) has rotated the + * revision before this render finishes, the stale render is + * silently discarded rather than overwriting the newer record. + * (Codex P1 review on PR #12957.) */ + const previewRevision = v4(); + const file = { + ...baseFile, + text: null, + textFormat: null, + status: 'pending', + previewError: null, + previewRevision, + }; + await createFile(file, true); + return { + file: Object.assign(file, { messageId, toolCallId }), + finalize: () => + finalizePreview({ buffer, leafName, mimeType, category, file_id, previewRevision }), + previewRevision, + }; + } + + /* Non-office path: extraction is cheap (utf8 decode, parseDocument + * for PDF/ODT, or null for binaries). Run inline and return a + * fully-resolved record — no `finalize` needed. */ + const text = await extractCodeArtifactText(buffer, leafName, mimeType, category); + /* `textFormat` accompanies `text` so the client can gate + * office-HTML-bucket routing on a trusted signal — clients MUST + * NOT inject `text` into the iframe as HTML unless `textFormat === + * 'html'`. RAG-uploaded `.docx` etc. arrive with plain text from + * mammoth.extractRawText and would otherwise be hijacked by the + * extension-based office routing into the HTML-injection path + * (Codex P1 review on PR #12934). null on extract failure — the + * client treats absence as 'text' for safety. */ + const textFormat = getExtractedTextFormat(leafName, mimeType, text); + const file = { + ...baseFile, + // Always set explicitly so an update which produces a binary or + // oversized artifact clears any previously cached text — createFile + // uses findOneAndUpdate with $set semantics. text: text ?? null, textFormat: textFormat ?? null, + // Clear deferred-preview lifecycle fields in case the prior emit + // at this (filename, conversationId) was an office file — + // otherwise stale `pending`/`failed` would persist and the client + // would render the wrong state for the now non-office artifact. + status: null, + previewError: null, + previewRevision: null, }; await createFile(file, true); - return Object.assign(file, { messageId, toolCallId }); + return { file: Object.assign(file, { messageId, toolCallId }) }; } catch (error) { if (error?.message === 'Path traversal detected in filename') { logger.warn( @@ -320,15 +606,17 @@ const processCodeOutput = async ({ }); // Fallback for download errors - return download URL so user can still manually download - return createDownloadFallback({ - id, - name, - messageId, - toolCallId, - session_id, - conversationId, - expiresAt: currentDate.getTime() + 86400000, - }); + return { + file: createDownloadFallback({ + id, + name, + messageId, + toolCallId, + session_id, + conversationId, + expiresAt: currentDate.getTime() + 86400000, + }), + }; } }; @@ -465,7 +753,23 @@ const primeFiles = async (options) => { } const entity_id = overrideEntityId ?? queryParams.entity_id; - toolContext += `\n\t- /mnt/data/${file.filename}${fileSuffix}`; + + /* Surface the preview lifecycle so the LLM knows when a + * prior-turn artifact's rich preview didn't materialize. The + * file blob is always available (`processCodeOutput` persists + * it before returning), so the model can still tell the user + * "you can download it" even when the preview never resolved. + * Absent status means legacy or non-office — render normally. */ + let previewSuffix = ''; + if (file.status === 'pending') { + previewSuffix = ' (preview not yet generated)'; + } else if (file.status === 'failed') { + previewSuffix = file.previewError + ? ` (preview unavailable: ${file.previewError})` + : ' (preview unavailable)'; + } + + toolContext += `\n\t- /mnt/data/${file.filename}${fileSuffix}${previewSuffix}`; files.push({ id: overrideId ?? id, session_id: overrideSessionId ?? session_id, @@ -624,4 +928,5 @@ module.exports = { getSessionInfo, processCodeOutput, readSandboxFile, + runPreviewFinalize, }; diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index 8b12278c9a..98cbbf9fff 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -44,6 +44,15 @@ mockAxios.isAxiosError = jest.fn(() => false); const mockClassifyCodeArtifact = jest.fn(() => 'other'); const mockExtractCodeArtifactText = jest.fn(async () => null); const mockGetExtractedTextFormat = jest.fn((_name, _mime, text) => (text == null ? null : 'text')); +/* `hasOfficeHtmlPath` gates the persist-then-render split: when true, processCodeOutput + * returns `{ file, finalize }` with the file persisted at `status: 'pending'` + * and `finalize` runs the background extraction. Default false here so the + * legacy single-phase tests below (txt/png/etc) exercise the inline path + * unchanged. The dedicated office/finalize describe block toggles it on. */ +const mockHasOfficeHtmlPath = jest.fn(() => false); +/* Pass-through `withTimeout`: tests don't drive timeouts here (those live + * in promise.spec.ts and the finalizePreview unit tests below). */ +const passthroughWithTimeout = async (promise) => promise; jest.mock('@librechat/api', () => { const http = require('http'); const https = require('https'); @@ -53,6 +62,8 @@ jest.mock('@librechat/api', () => { sanitizeArtifactPath: jest.fn((name) => name), flattenArtifactPath: jest.fn((name) => name.replace(/\//g, '__')), createAxiosInstance: jest.fn(() => mockAxios), + withTimeout: (...args) => passthroughWithTimeout(...args), + hasOfficeHtmlPath: (...args) => mockHasOfficeHtmlPath(...args), /** * Arrow-function indirection (vs. a direct `jest.fn()` reference) so * tests can per-case `mockReturnValueOnce` / `mockImplementationOnce` @@ -178,7 +189,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(mockClaimCodeFile).toHaveBeenCalledWith({ filename: 'test-file.txt', @@ -201,7 +212,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.file_id).toBe('mock-uuid-1234'); expect(result.usage).toBe(1); @@ -221,7 +232,7 @@ describe('Code Process', () => { }; convertImage.mockResolvedValue(convertedFile); - const result = await processCodeOutput(imageParams); + const { file: result } = await processCodeOutput(imageParams); expect(convertImage).toHaveBeenCalledWith( mockReq, @@ -246,7 +257,7 @@ describe('Code Process', () => { mockAxios.mockResolvedValue({ data: imageBuffer }); convertImage.mockResolvedValue({ filepath: '/images/user-123/existing-img-id.webp' }); - const result = await processCodeOutput(imageParams); + const { file: result } = await processCodeOutput(imageParams); expect(convertImage).toHaveBeenCalledWith( mockReq, @@ -272,7 +283,7 @@ describe('Code Process', () => { getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer }); determineFileType.mockResolvedValue({ mime: 'text/plain' }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(mockSaveBuffer).toHaveBeenCalledWith({ userId: 'user-123', @@ -299,7 +310,7 @@ describe('Code Process', () => { const mockSaveBuffer = jest.fn().mockResolvedValue('/uploads/saved.txt'); getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer }); - const result = await processCodeOutput({ + const { file: result } = await processCodeOutput({ ...baseParams, name: 'test_folder/test_file.txt', }); @@ -378,7 +389,7 @@ describe('Code Process', () => { mockAxios.mockResolvedValue({ data: smallBuffer }); determineFileType.mockResolvedValue({ mime: 'application/pdf' }); - const result = await processCodeOutput({ ...baseParams, name: 'document.pdf' }); + const { file: result } = await processCodeOutput({ ...baseParams, name: 'document.pdf' }); expect(determineFileType).toHaveBeenCalledWith(smallBuffer, true); expect(result.type).toBe('application/pdf'); @@ -389,7 +400,7 @@ describe('Code Process', () => { mockAxios.mockResolvedValue({ data: smallBuffer }); determineFileType.mockResolvedValue(null); - const result = await processCodeOutput({ ...baseParams, name: 'unknown.xyz' }); + const { file: result } = await processCodeOutput({ ...baseParams, name: 'unknown.xyz' }); expect(result.type).toBe('application/octet-stream'); }); @@ -403,7 +414,7 @@ describe('Code Process', () => { mockClassifyCodeArtifact.mockReturnValueOnce('utf8-text'); mockExtractCodeArtifactText.mockResolvedValueOnce('hello world\n'); - const result = await processCodeOutput({ ...baseParams, name: 'note.txt' }); + const { file: result } = await processCodeOutput({ ...baseParams, name: 'note.txt' }); expect(mockClassifyCodeArtifact).toHaveBeenCalledWith('note.txt', 'text/plain'); expect(mockExtractCodeArtifactText).toHaveBeenCalledWith( @@ -426,7 +437,7 @@ describe('Code Process', () => { mockClassifyCodeArtifact.mockReturnValueOnce('other'); mockExtractCodeArtifactText.mockResolvedValueOnce(null); - const result = await processCodeOutput({ ...baseParams, name: 'archive.zip' }); + const { file: result } = await processCodeOutput({ ...baseParams, name: 'archive.zip' }); expect(result.text).toBeNull(); const createCall = createFile.mock.calls[0][0]; @@ -465,6 +476,31 @@ describe('Code Process', () => { expect(mockClassifyCodeArtifact).not.toHaveBeenCalled(); expect(mockExtractCodeArtifactText).not.toHaveBeenCalled(); }); + + it('clears deferred-preview lifecycle fields so a prior office record at this file_id stops looking pending', async () => { + /* Codex P2: same (filename, conversationId) was previously an + * office artifact, leaving status/previewError/previewRevision + * populated. The non-office update must reset them or the + * client renders the wrong state for the now non-office file. */ + mockClaimCodeFile.mockResolvedValueOnce({ + file_id: 'reused-id', + filename: 'output.txt', + usage: 1, + createdAt: '2024-01-01T00:00:00.000Z', + }); + mockAxios.mockResolvedValue({ data: Buffer.from('hello') }); + determineFileType.mockResolvedValue({ mime: 'text/plain' }); + mockClassifyCodeArtifact.mockReturnValueOnce('text'); + mockHasOfficeHtmlPath.mockReturnValueOnce(false); + mockExtractCodeArtifactText.mockResolvedValueOnce('hello'); + + await processCodeOutput({ ...baseParams, name: 'output.txt' }); + + const createCall = createFile.mock.calls[0][0]; + expect(createCall).toHaveProperty('status', null); + expect(createCall).toHaveProperty('previewError', null); + expect(createCall).toHaveProperty('previewRevision', null); + }); }); describe('file size limit enforcement', () => { @@ -475,7 +511,7 @@ describe('Code Process', () => { const largeBuffer = Buffer.alloc(5000); // 5KB - exceeds 1KB limit mockAxios.mockResolvedValue({ data: largeBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('exceeds size limit')); expect(result.filepath).toContain('/api/files/code/download/session-123/file-id-123'); @@ -494,7 +530,7 @@ describe('Code Process', () => { mockAxios.mockResolvedValue({ data: smallBuffer }); getStrategyFunctions.mockReturnValue({ saveBuffer: null }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(logger.warn).toHaveBeenCalledWith( expect.stringContaining('saveBuffer not available'), @@ -506,7 +542,7 @@ describe('Code Process', () => { it('should fallback to download URL on axios error', async () => { mockAxios.mockRejectedValue(new Error('Network error')); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.filepath).toContain('/api/files/code/download/session-123/file-id-123'); expect(result.conversationId).toBe('conv-123'); @@ -520,7 +556,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.usage).toBe(1); }); @@ -534,7 +570,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.usage).toBe(6); }); @@ -547,7 +583,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.usage).toBe(1); }); @@ -558,7 +594,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.metadata).toEqual({ fileIdentifier: 'session-123/file-id-123', @@ -569,7 +605,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.context).toBe(FileContext.execute_code); }); @@ -578,7 +614,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput(baseParams); + const { file: result } = await processCodeOutput(baseParams); expect(result.toolCallId).toBe('tool-call-123'); expect(result.messageId).toBe('msg-123'); @@ -718,7 +754,7 @@ describe('Code Process', () => { const smallBuffer = Buffer.alloc(100); mockAxios.mockResolvedValue({ data: smallBuffer }); - const result = await processCodeOutput({ + const { file: result } = await processCodeOutput({ ...baseParams, name: 'sentinel.txt', messageId: 'turn-2-current-run-msg', @@ -789,6 +825,329 @@ describe('Code Process', () => { expect(callConfig.httpsAgent.keepAlive).toBe(false); }); }); + + describe('deferred-preview flow (office-bucket files)', () => { + /* Office-bucket files (DOCX/XLSX/etc.) split into: + * the initial emit (await): persist `text: null, status: 'pending'`, + * return `{ file, finalize }` so the caller can ship the + * attachment to the client immediately; + * the deferred render (background): finalize() invokes the extractor and + * transitions the record to 'ready' (with text/textFormat) or + * 'failed' (with previewError). The agent's final response + * never blocks on the deferred render. + * + * The `hasOfficeHtmlPath` mock is the gate. Other tests keep it + * at `false` (legacy single-phase path); we flip it on here. */ + const { updateFile } = require('~/models'); + + beforeEach(() => { + mockHasOfficeHtmlPath.mockReturnValue(true); + updateFile.mockResolvedValue({ file_id: 'mock-uuid-1234', status: 'ready' }); + }); + + afterEach(() => { + mockHasOfficeHtmlPath.mockReturnValue(false); + }); + + it('persists the initial emit with status:pending and text:null, deferring extraction', async () => { + mockAxios.mockResolvedValue({ data: Buffer.alloc(100) }); + determineFileType.mockResolvedValue({ + mime: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + }); + + const result = await processCodeOutput({ ...baseParams, name: 'data.xlsx' }); + + expect(result.file).toMatchObject({ + file_id: 'mock-uuid-1234', + filename: 'data.xlsx', + status: 'pending', + text: null, + textFormat: null, + }); + expect(typeof result.finalize).toBe('function'); + // Extractor MUST NOT have been called yet — that's deferred preview work. + expect(mockExtractCodeArtifactText).not.toHaveBeenCalled(); + // Persisted record with the pending status. + expect(createFile).toHaveBeenCalledWith( + expect.objectContaining({ status: 'pending', text: null, textFormat: null }), + true, + ); + }); + + it('finalize() runs the extractor, transitions to ready with text+textFormat on success', async () => { + mockAxios.mockResolvedValue({ data: Buffer.alloc(100) }); + determineFileType.mockResolvedValue({ + mime: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + }); + mockExtractCodeArtifactText.mockResolvedValueOnce('
1
'); + mockGetExtractedTextFormat.mockReturnValueOnce('html'); + + const { finalize } = await processCodeOutput({ ...baseParams, name: 'data.xlsx' }); + await finalize(); + + expect(mockExtractCodeArtifactText).toHaveBeenCalledTimes(1); + /* Update is conditional on `previewRevision` so an older render + * can't overwrite a newer turn's record on cross-turn filename + * reuse. The uuid mock returns the same value for every v4() + * call, so file_id and previewRevision happen to coincide here + * — what matters is the second arg carries the revision filter. */ + expect(updateFile).toHaveBeenCalledWith( + { + file_id: 'mock-uuid-1234', + text: '
1
', + textFormat: 'html', + status: 'ready', + previewError: null, + }, + { previewRevision: 'mock-uuid-1234' }, + ); + }); + + it('finalize() transitions to failed with previewError when extractor returns null (HTML-or-null contract)', async () => { + mockAxios.mockResolvedValue({ data: Buffer.alloc(100) }); + determineFileType.mockResolvedValue({ + mime: 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + }); + mockExtractCodeArtifactText.mockResolvedValueOnce(null); + // Office bucket + null text → must be 'failed', NEVER raw text fallback + // (PR #12934 SEC fix: prevents