From 8b990d654a385fb6066c55ea6b3d153d47c534af Mon Sep 17 00:00:00 2001
From: Danny Avila <danny@librechat.ai>
Date: Tue, 5 May 2026 14:56:20 -0400
Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20test:=20address=20compr?=
 =?UTF-8?q?ehensive=20review=20NITs=20(initial-emit=20guard=20+=20isStream?=
 =?UTF-8?q?Writable=20coverage)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 .../agents/__tests__/callbacks.spec.js        | 36 +++++++++++++++++++
 api/server/controllers/agents/callbacks.js    | 13 +++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/api/server/controllers/agents/__tests__/callbacks.spec.js b/api/server/controllers/agents/__tests__/callbacks.spec.js
index 3c990bc7a0..0ba20d409c 100644
--- a/api/server/controllers/agents/__tests__/callbacks.spec.js
+++ b/api/server/controllers/agents/__tests__/callbacks.spec.js
@@ -575,3 +575,39 @@ describe('createToolEndCallback', () => {
     });
   });
 });
+
+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 58eda1ed1b..c9612c6b62 100644
--- a/api/server/controllers/agents/callbacks.js
+++ b/api/server/controllers/agents/callbacks.js
@@ -641,8 +641,16 @@ function createToolEndCallback({ req, res, artifactPromises, streamId = null })
           /* 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. */
-          if (streamId || res.headersSent) {
+           * 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
@@ -1017,6 +1025,7 @@ module.exports = {
   agentLogHandlerObj,
   getDefaultHandlers,
   createToolEndCallback,
+  isStreamWritable,
   markSummarizationUsage,
   buildSummarizationHandlers,
   createResponsesToolEndCallback,