🩹 fix: Try office HTML before short-circuiting on category=other

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
This commit is contained in:
Danny Avila 2026-05-04 08:35:39 +09:00
parent 6be1d6f6b7
commit 241551ed1b
2 changed files with 60 additions and 14 deletions

View file

@ -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('<!DOCTYPE html><table><tr><td>1</td></tr></table>');
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('<table>');
});
it('routes extensionless office MIME with parameters through office HTML even when category=other', async () => {
mockOfficeHtml.mockResolvedValueOnce('<!DOCTYPE html><body>x</body></html>');
const text = await extractCodeArtifactText(
Buffer.from('PK'),
'workbook',
'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet; charset=binary',
'other',
);
expect(mockOfficeHtml).toHaveBeenCalled();
expect(text).toContain('<body>');
});
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();
});
});
});

View file

@ -170,28 +170,30 @@ export async function extractCodeArtifactText(
mimeType: string,
category: CodeArtifactCategory,
): Promise<string | null> {
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);