Document-extract: address codex review (Windows trailing slash, cache sentinel, test name)

solomonneas · solomonneas · commit 49bf3e85c9d3 · 2026-04-29T09:28:10.000-04:00
diff --git a/extensions/document-extract/document-extractor.standard-fonts.test.ts b/extensions/document-extract/document-extractor.standard-fonts.test.ts
@@ -39,7 +39,7 @@ describe("PDF document extractor with standard fonts", () => {
     }
   });
 
-  it("reuses the cached standard-font path across calls", async () => {
+  it("extracts the same text on repeated calls without leaking state", async () => {
     const extractor = createPdfDocumentExtractor();
     const first = await extractor.extract({
       buffer: helloPdfBuffer,
diff --git a/extensions/document-extract/document-extractor.ts b/extensions/document-extract/document-extractor.ts
@@ -1,5 +1,5 @@
 import { createRequire } from "node:module";
-import { dirname, join, sep } from "node:path";
+import { dirname, join } from "node:path";
 import type {
   DocumentExtractedImage,
   DocumentExtractionRequest,
@@ -56,24 +56,29 @@ const MAX_RENDER_DIMENSION = 10_000;
 
 let canvasModulePromise: Promise<CanvasModule> | null = null;
 let pdfJsModulePromise: Promise<PdfJsModule> | null = null;
-let standardFontDataUrlCache: string | null = null;
-
-// pdf.js requires a filesystem path (with trailing separator) to resolve the
-// fonts shipped under `pdfjs-dist/standard_fonts/`. Without it, any PDF that
-// references the 14 standard fonts (Helvetica, Times, Courier, Symbol,
-// ZapfDingbats) yields empty text plus a noisy `UnknownErrorException`.
-// pdf.js silently rejects `file://` URLs on Node, so we pass a plain path.
+// `null` = not resolved yet, `false` = resolution failed once, `string` = resolved path.
+let standardFontDataUrlCache: string | false | null = null;
+
+// pdf.js requires a filesystem path that ends in `/` (it throws
+// `Invalid factory url ... must include trailing slash` from
+// `getFactoryUrlProp`). Without it, any PDF that references the 14 standard
+// fonts (Helvetica, Times, Courier, Symbol, ZapfDingbats) yields empty text
+// plus a noisy `UnknownErrorException`. pdf.js silently rejects `file://`
+// URLs on Node, so we pass a plain path.
 function resolveStandardFontDataUrl(): string | undefined {
+  if (standardFontDataUrlCache === false) {
+    return undefined;
+  }
   if (standardFontDataUrlCache !== null) {
-    return standardFontDataUrlCache || undefined;
+    return standardFontDataUrlCache;
   }
   try {
     const requireFn = createRequire(import.meta.url);
     const pkgPath = requireFn.resolve("pdfjs-dist/package.json");
-    standardFontDataUrlCache = join(dirname(pkgPath), "standard_fonts") + sep;
+    standardFontDataUrlCache = `${join(dirname(pkgPath), "standard_fonts")}/`;
     return standardFontDataUrlCache;
   } catch {
-    standardFontDataUrlCache = "";
+    standardFontDataUrlCache = false;
     return undefined;
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ describe("PDF document extractor with standard fonts", () => {`
`39`	`39`	`}`
`40`	`40`	`});`
`41`	`41`
`42`		`- it("reuses the cached standard-font path across calls", async () => {`
	`42`	`+ it("extracts the same text on repeated calls without leaking state", async () => {`
`43`	`43`	`const extractor = createPdfDocumentExtractor();`
`44`	`44`	`const first = await extractor.extract({`
`45`	`45`	`buffer: helloPdfBuffer,`