From 241551ed1b7f2d4be8d18504e86e1f9450132803 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 08:35:39 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Try=20office=20HTML=20bef?= =?UTF-8?q?ore=20short-circuiting=20on=20category=3Dother?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on #12934 caught that the early `category === 'other'` return short-circuited before `hasOfficeHtmlPath` was checked. The classifier returns 'other' for inputs the new dispatcher can still route — extensionless `application/csv` (CSV MIMEs aren't in the classifier's text-MIME set and don't start with `text/`), and extensionless office MIMEs with parameters like `application/vnd... spreadsheetml.sheet; charset=binary` (the classifier's `isDocumentMime` exact-matches these MIMEs without parameter normalization). Both would route correctly through `officeHtmlBucket` but never reached it. Move the office-HTML attempt above the 'other' early return, and drop the `|| category === 'document' || category === 'pptx'` shortcut now that `hasOfficeHtmlPath` covers the same surface (with parameter normalization) and a wider one. ODT still routes through `extractDocument` unchanged — `hasOfficeHtmlPath` returns false for it and the `category === 'document'` branch below handles it. Adds 3 regression tests: - extensionless `application/csv` + category='other' → office HTML - extensionless parameterized office MIME + category='other' → office HTML - defense check: actual binary 'other' (image/jpeg) still returns null without invoking the office producer --- packages/api/src/files/code/extract.spec.ts | 44 +++++++++++++++++++++ packages/api/src/files/code/extract.ts | 30 +++++++------- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/packages/api/src/files/code/extract.spec.ts b/packages/api/src/files/code/extract.spec.ts index 99407ca1c2..500971d9f1 100644 --- a/packages/api/src/files/code/extract.spec.ts +++ b/packages/api/src/files/code/extract.spec.ts @@ -371,5 +371,49 @@ describe('extractCodeArtifactText', () => { // assertion is that we don't crash and don't leak the bomb error. expect(text).toBe(docxText); }); + + /* Regression for Codex P2 review on PR #12934. The classifier returns + * 'other' for a small but real set of inputs that the new dispatcher + * can still route — extensionless `application/csv`, extensionless + * office MIMEs with parameters, etc. The early `category === 'other'` + * return must NOT short-circuit before `hasOfficeHtmlPath` is checked, + * or those inputs silently lose the rich preview. */ + it('routes extensionless application/csv through office HTML even when category=other', async () => { + mockOfficeHtml.mockResolvedValueOnce('
1
'); + const text = await extractCodeArtifactText( + Buffer.from('a,b\n1,2', 'utf-8'), + 'data', + 'application/csv', + 'other', + ); + expect(mockOfficeHtml).toHaveBeenCalledWith(expect.any(Buffer), 'data', 'application/csv'); + expect(text).toContain(''); + }); + + it('routes extensionless office MIME with parameters through office HTML even when category=other', async () => { + mockOfficeHtml.mockResolvedValueOnce('x'); + const text = await extractCodeArtifactText( + Buffer.from('PK'), + 'workbook', + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet; charset=binary', + 'other', + ); + expect(mockOfficeHtml).toHaveBeenCalled(); + expect(text).toContain(''); + }); + + it('still returns null for true binary "other" files (no MIME match)', async () => { + /* Defense check: the category=other early return is preserved when + * `hasOfficeHtmlPath` returns false. A JPEG should not be handed to + * the office producer. */ + const text = await extractCodeArtifactText( + Buffer.from([0xff, 0xd8, 0xff]), + 'photo.jpg', + 'image/jpeg', + 'other', + ); + expect(mockOfficeHtml).not.toHaveBeenCalled(); + expect(text).toBeNull(); + }); }); }); diff --git a/packages/api/src/files/code/extract.ts b/packages/api/src/files/code/extract.ts index cc349204eb..1168522917 100644 --- a/packages/api/src/files/code/extract.ts +++ b/packages/api/src/files/code/extract.ts @@ -170,28 +170,30 @@ export async function extractCodeArtifactText( mimeType: string, category: CodeArtifactCategory, ): Promise { - if (category === 'other') { - return null; - } if (buffer.length > MAX_TEXT_EXTRACT_BYTES) { return null; } try { - /* Office HTML preview takes precedence for any file the dispatcher - * recognizes — by extension OR by MIME. The MIME fallback matters for - * extensionless tool outputs (e.g. `data` with `text/csv`); without - * it we'd ship raw CSV text to the client's SPREADSHEET bucket which - * expects a full HTML document. The `document`/`pptx` category checks - * keep the path open for sniffed `application/zip`/`octet-stream` MIMEs - * where neither the extension nor the MIME alone is dispositive. */ - if (hasOfficeHtmlPath(name, mimeType) || category === 'document' || category === 'pptx') { + /* Office HTML preview is independent of `category` — `officeHtmlBucket` + * routes by extension OR (parameter-normalized) MIME, and may route + * inputs the legacy classifier labels as 'other'. Concrete cases: + * - extensionless `application/csv` (CSV MIMEs aren't in the + * classifier's text-MIME set and don't start with `text/`) + * - extensionless office MIMEs with parameters like + * `application/vnd...spreadsheetml.sheet; charset=binary` + * Without checking `hasOfficeHtmlPath` BEFORE the `category === 'other'` + * early return, those inputs would silently fall back to download-only + * even though the new dispatcher and the client both expect HTML. */ + if (hasOfficeHtmlPath(name, mimeType)) { const html = await renderOfficeHtml(buffer, name, mimeType); if (html != null) { return html; } - /* HTML producer returned null — fall through to text-only extraction - * for the `document` category (PDFs and ODT still need text), or to - * utf8-text for CSV. PPTX has no text fallback (returns null below). */ + /* HTML producer returned null — fall through to category-based + * dispatch, which may itself return null for unsupported types. */ + } + if (category === 'other') { + return null; } if (category === 'utf8-text') { return extractUtf8(buffer);