From 832b8f7bc7eea6176ed42f441e4d0d2c7eb0a4e3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 17:40:49 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20CI=20=E2=80=94=20preserve?= =?UTF-8?q?=20legacy=20download=20path=20for=20empty-text=20office=20attac?= =?UTF-8?q?hments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regressions surfaced after the textFormat security gate landed. 1. **Client** (`LogContent.test.tsx` "falls back to the legacy download branch for an office file with no extracted text"): When the security gate downgraded an office type without `textFormat: 'html'` to PLAIN_TEXT, the lenient empty-text gate on PLAIN_TEXT then accepted a missing `text` field and rendered a half-empty panel card. The historical contract is "office type + no text → legacy download UI"; the downgrade should only fire when there's actual plain text that needs safe-escaping. Fix in `detectArtifactTypeFromFile`: short-circuit to null when the office type lands in the security-gate branch with no text. The PLAIN_TEXT downgrade still fires for legacy attachments that DO carry plain text. 2. **API** (`process.spec.js` + `process-traversal.spec.js`): the `@librechat/api` mocks didn't expose `getExtractedTextFormat`, so `processCodeOutput` called `undefined(...)` → TypeError → tests got undefined results. Added the helper to both mocks with a faithful default (returns 'text' for non-null extractor output, null otherwise). Tests: new regression in `artifacts.test.ts` pinning the empty-text + no-textFormat → null contract for all four office types (.docx/.csv/.xlsx/.pptx), so a future refactor can't silently re-introduce the half-empty card. --- .../Code/__tests__/process-traversal.spec.js | 5 +++++ .../services/Files/Code/process.spec.js | 10 +++++++++ client/src/utils/__tests__/artifacts.test.ts | 21 +++++++++++++++++++ client/src/utils/artifacts.ts | 13 +++++++++++- 4 files changed, 48 insertions(+), 1 deletion(-) 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 016f3a71c6..099794821d 100644 --- a/api/server/services/Files/Code/__tests__/process-traversal.spec.js +++ b/api/server/services/Files/Code/__tests__/process-traversal.spec.js @@ -27,6 +27,11 @@ jest.mock('@librechat/api', () => { createAxiosInstance: jest.fn(() => mockAxios), classifyCodeArtifact: jest.fn(() => 'other'), extractCodeArtifactText: jest.fn(async () => null), + /* `processCodeOutput` calls this to derive the trust flag persisted + * on `IMongoFile.textFormat` — Codex P1 review on PR #12934. The + * mock returns null in lockstep with the null `text` above so + * downstream consumers don't see a phantom format. */ + getExtractedTextFormat: jest.fn(() => null), codeServerHttpAgent: new http.Agent({ keepAlive: false }), codeServerHttpsAgent: new https.Agent({ keepAlive: false }), }; diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index e0d7b08fd3..8b12278c9a 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -43,6 +43,7 @@ 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')); jest.mock('@librechat/api', () => { const http = require('http'); const https = require('https'); @@ -63,6 +64,15 @@ jest.mock('@librechat/api', () => { */ classifyCodeArtifact: (...args) => mockClassifyCodeArtifact(...args), extractCodeArtifactText: (...args) => mockExtractCodeArtifactText(...args), + /* `processCodeOutput` derives the `textFormat` trust flag for + * `IMongoFile` from this helper — Codex P1 review on PR #12934. + * The mock returns 'text' for non-null extractor output and null + * otherwise so the downstream `file.textFormat` field is set to + * a believable shape without modeling the office-HTML branch + * (the dispatcher under test isn't exercising that path). Per- + * test overrides via `mockGetExtractedTextFormat.mockReturnValue` + * if a case needs to assert the 'html' value. */ + getExtractedTextFormat: (...args) => mockGetExtractedTextFormat(...args), codeServerHttpAgent: new http.Agent({ keepAlive: false }), codeServerHttpsAgent: new https.Agent({ keepAlive: false }), }; diff --git a/client/src/utils/__tests__/artifacts.test.ts b/client/src/utils/__tests__/artifacts.test.ts index 455e518afa..b664bc3456 100644 --- a/client/src/utils/__tests__/artifacts.test.ts +++ b/client/src/utils/__tests__/artifacts.test.ts @@ -225,6 +225,27 @@ describe('detectArtifactTypeFromFile', () => { ).toBe(TOOL_ARTIFACT_TYPES.PLAIN_TEXT); }); + /* Regression: an office attachment with no `text` AND no `textFormat` + * must NOT downgrade to PLAIN_TEXT (which has the lenient empty-text + * gate and would render as a half-empty panel card). The historical + * contract for office types with missing extraction is "fall through + * to the legacy download UI"; the security gate's empty-text exception + * preserves that. CI regression on PR #12934 — `LogContent.test.tsx` + * "falls back to the legacy download branch for an office file with + * no extracted text" was the canary. */ + it.each([ + ['report.docx', 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'], + ['data.csv', 'text/csv'], + ['workbook.xlsx', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'], + ['slides.pptx', 'application/vnd.openxmlformats-officedocument.presentationml.presentation'], + ])( + '%s with no text and no textFormat returns null (preserves legacy download path)', + (filename, type) => { + expect(detectArtifactTypeFromFile({ filename, type, text: '' })).toBeNull(); + expect(detectArtifactTypeFromFile({ filename, type, text: undefined })).toBeNull(); + }, + ); + it('falls back to MIME when the extension is unknown', () => { expect(detectArtifactTypeFromFile({ filename: 'noext', type: 'text/html', text: 'x' })).toBe( TOOL_ARTIFACT_TYPES.HTML, diff --git a/client/src/utils/artifacts.ts b/client/src/utils/artifacts.ts index 11d360a94f..c51e83cfa3 100644 --- a/client/src/utils/artifacts.ts +++ b/client/src/utils/artifacts.ts @@ -719,8 +719,19 @@ export function detectArtifactTypeFromFile( * pre-flag period and the safe default ('text' bucket) is OK for * those — they were rendering correctly before this flag, and the * markdown viewer escaping HTML is a safety upgrade, not a - * regression. Codex P1 review on PR #12934. */ + * regression. Codex P1 review on PR #12934. + * + * Empty-text exception: if the office attachment has NO text at all, + * downgrading to PLAIN_TEXT would route an empty-text-tolerant + * bucket onto the panel (a half-rendered card showing only the + * filename). The historical contract for office types with missing + * text is "fall through to the legacy download path"; preserve that + * by returning null here. The downgrade only matters when there's + * actual text content that needs safe-escaping. */ if (OFFICE_HTML_BUCKETS.has(type) && attachment.textFormat !== 'html') { + if (!attachment.text) { + return null; + } return TOOL_ARTIFACT_TYPES.PLAIN_TEXT; } if (