fix(document-extract): set standardFontDataUrl for standard-font PDFs#74355
fix(document-extract): set standardFontDataUrl for standard-font PDFs#74355solomonneas wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes PDF text extraction for the common case of PDFs that use any of the 14 standard fonts (Helvetica, Times, Courier, etc.) by resolving and passing Confidence Score: 4/5Safe to merge — fix is minimal, well-tested, and degrades gracefully when the optional dependency is absent. Only P2 style observations; the production logic is correct. Score held at 4 (P2s only). No files require special attention; the one style note is on the test's console.warn monkey-patching pattern. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/document-extract/document-extractor.standard-fonts.test.ts
Line: 27-29
Comment:
**Prefer `vi.spyOn` over manual `console.warn` reassignment**
Directly overwriting `console.warn` works but can suppress warnings from other concurrent test files if Vitest runs them in the same worker. `vi.spyOn(console, 'warn')` is the idiomatic alternative — it wraps the original, captures calls, and is automatically restored after the test (no `finally` needed).
```suggestion
const warnSpy = vi.spyOn(console, "warn").mockImplementation((...args: unknown[]) => {
warnings.push(args.map((arg) => String(arg)).join(" "));
});
try {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/document-extract/document-extractor.standard-fonts.test.ts
Line: 44-46
Comment:
**Restore spy in `finally` with `vi.spyOn` approach**
If you adopt `vi.spyOn`, remove the manual restore below — vitest restores it automatically after the test. If keeping the manual approach, the existing `finally { console.warn = originalWarn; }` is the correct safety net.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Document-extract: address codex review (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| maxPages: 1, | ||
| maxPixels: 2_000_000, | ||
| minTextChars: 5, |
There was a problem hiding this comment.
vi.spyOn over manual console.warn reassignment
Directly overwriting console.warn works but can suppress warnings from other concurrent test files if Vitest runs them in the same worker. vi.spyOn(console, 'warn') is the idiomatic alternative — it wraps the original, captures calls, and is automatically restored after the test (no finally needed).
| maxPages: 1, | |
| maxPixels: 2_000_000, | |
| minTextChars: 5, | |
| const warnSpy = vi.spyOn(console, "warn").mockImplementation((...args: unknown[]) => { | |
| warnings.push(args.map((arg) => String(arg)).join(" ")); | |
| }); | |
| try { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/document-extract/document-extractor.standard-fonts.test.ts
Line: 27-29
Comment:
**Prefer `vi.spyOn` over manual `console.warn` reassignment**
Directly overwriting `console.warn` works but can suppress warnings from other concurrent test files if Vitest runs them in the same worker. `vi.spyOn(console, 'warn')` is the idiomatic alternative — it wraps the original, captures calls, and is automatically restored after the test (no `finally` needed).
```suggestion
const warnSpy = vi.spyOn(console, "warn").mockImplementation((...args: unknown[]) => {
warnings.push(args.map((arg) => String(arg)).join(" "));
});
try {
```
How can I resolve this? If you propose a fix, please make it concise.| const first = await extractor.extract({ | ||
| buffer: helloPdfBuffer, | ||
| mimeType: "application/pdf", |
There was a problem hiding this comment.
finally with vi.spyOn approach
If you adopt vi.spyOn, remove the manual restore below — vitest restores it automatically after the test. If keeping the manual approach, the existing finally { console.warn = originalWarn; } is the correct safety net.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/document-extract/document-extractor.standard-fonts.test.ts
Line: 44-46
Comment:
**Restore spy in `finally` with `vi.spyOn` approach**
If you adopt `vi.spyOn`, remove the manual restore below — vitest restores it automatically after the test. If keeping the manual approach, the existing `finally { console.warn = originalWarn; }` is the correct safety net.
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: needs maintainer review before merge. What this changes: The PR updates the bundled Maintainer follow-up before merge: This is already an open implementation PR with a narrow, evidence-backed fix; the remaining action is maintainer review and merge decision, not a separate automated repair PR. Best possible solution: Land this PR or an equivalent narrow plugin-owned fix that resolves the local Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 542821cd1e60. |
|
Codex review: needs maintainer review before merge. What this changes: The branch resolves Maintainer follow-up before merge: This is a valid implementation PR but overlaps with #70936, so the remaining action is maintainer branch selection and possible test/fix transplant rather than a new automated repair job. Review detailsBest possible solution: Land one narrow plugin-owned fix that resolves the installed Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 8cf724a381a3. |
|
Closing as superseded by #70936, which landed on main as Sorry for the duplicate filing — I missed #70936 sitting open while focusing on the post- |
1 participant
Summary
UnknownErrorException: Ensure that the standardFontDataUrl API parameter is providedper page. Downstream callers then send the model an empty document, the model rejects, and the agent run times out.document-extractplugin and noted "preserving default PDF extraction behavior", but the plugin'sgetDocumentcall and itsPdfJsModuletype both omitstandardFontDataUrl, so the standard fonts pdf.js ships underpdfjs-dist/standard_fonts/are never wired up.extensions/document-extract/document-extractor.tsnow resolves the standard-font directory viacreateRequire(import.meta.url).resolve("pdfjs-dist/package.json")→dirname + "/standard_fonts/" + sep, caches it across calls, types it onPdfJsModule.getDocument, and passes it on every call. A real-PDF integration test using a 518-byte Helvetica fixture proves text extraction now succeeds and that pdf.js no longer warns about the missing parameter.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
document-extractplugin'sPdfJsModule.getDocumentparam type is{ data: Uint8Array; disableWorker?: boolean }and the call site passes onlydataanddisableWorker. pdf.js requiresstandardFontDataUrl(a filesystem path with trailing separator on Node, not afile://URL — pdf.js silently rejects the URL form on Node) to load the standard-font data files. Without it, glyph mapping for the 14 standard fonts is missing andgetTextContent()returns nostritems.src/media/pdf-extract.ts(PR Media: fix PDF text extraction by setting standardFontDataUrl #62175) had a real-PDF integration test that exercised exactly this code path; that test was not carried forward when the surface moved to the plugin in refactor(pdf): move document extraction to plugin #71278, so the regression landed silently.{ data; disableWorker? }), which made it a breaking change against Media: fix PDF text extraction by setting standardFontDataUrl #62175's call shape. With Media: fix PDF text extraction by setting standardFontDataUrl #62175 closed-not-merged a day later, the repo lost the only test that proved standard-font PDFs survive end-to-end extraction.Regression Test Plan (if applicable)
extensions/document-extract/document-extractor.standard-fonts.test.tscreatePdfDocumentExtractor()end-to-end with the actualpdfjs-distmodule — assert non-empty extracted text and nostandardFontDataUrl-related warnings on stderr.pdfjs-distand pass even if the real call site is wrong (this is how the regression was missed). The integration test loads pdf.js for real and exercises both the type signature and the resolved path, including thefile://vs filesystem-path footgun. The fixture is a 518-byte inline base64 PDF, so the test stays fast (~120 ms) and self-contained.document-extractor.test.tsmockspdfjs-distend-to-end, which cannot detect a missing param on the real module.User-visible / Behavior Changes
PDF text extraction now works for PDFs that use any of the 14 standard PDF fonts (Helvetica, Times, Courier, Symbol, ZapfDingbats) — i.e. the common case. Previously these PDFs returned empty text plus per-page warnings; afterwards they return the actual page text. No config or default changes.
Diagram (if applicable)
Security Impact (required)
NoNoNoNoNo—createRequire(import.meta.url).resolve("pdfjs-dist/package.json")only resolves a path inside the plugin's already-bundled optional dependency. The resolved path is read by pdf.js at extraction time, identical to how it loads any other shipped resource.Repro + Verification
Environment
document-extractbundled plugintext)Steps
JVBERi0xLjQK...(518 bytes, deterministic).createPdfDocumentExtractor().extract({ buffer, mimeType: "application/pdf", maxPages: 1, maxPixels: 2_000_000, minTextChars: 5 }).Expected
textcontains the page's text content (e.g. "Hello PDF World").Warning: UnknownErrorException: Ensure that the standardFontDataUrl API parameter is providedon stderr.Actual
text === "", stderr fills withstandardFontDataUrlwarnings (one per page).text === "Hello PDF World", stderr clean. Verified by the new integration test which capturesconsole.warnand asserts nostandardFontDataUrl-mentioning warnings.Evidence
The new test file (
extensions/document-extract/document-extractor.standard-fonts.test.ts) is the proof. Without the production code change, the assertionexpect(result?.text).toContain("Hello PDF World")fails becauseresult.text === "", and thefontWarningsarray picks up thestandardFontDataUrlwarning. With the fix, the test passes in 89 ms.Test run output (clean dev container, this branch):
Targeted typecheck output:
Full build output:
Human Verification (required)
pdfjs-distmodule, not a mock; asserts both content extraction and absence of thestandardFontDataUrlwarning.septrailing separator), not afile://URL — pdf.js silently rejects the URL form on Node, which was the trap that surfaced during PR Media: fix PDF text extraction by setting standardFontDataUrl #62175 review.document-extractor.test.tsstill pass unchanged (the new param is optional in the type, so mock fixtures don't need updating).string | false | nullcache state is exercised in the same process for both calls but the cache hit is internal.)pdfjs-distnot installed →createRequireresolution throws, cache becomes"", function returnsundefined, behavior matches the pre-PR call shape (no regression for the optional-dep path).standardFontDataUrlmechanism, so Helvetica is a representative smoke; happy to add fixtures if reviewers want them.OPENCLAW_LIVE_TEST=1 pnpm test:liveagainst external providers — this PR is local to PDF parsing and does not change provider plumbing.Review Conversations
Compatibility / Migration
Yes—standardFontDataUrlis optional on thegetDocumentparam type (consumers' existing code shape continues to compile); the call site adds it unconditionally so default extraction behavior just gets better.NoNoRisks and Mitigations
createRequire(import.meta.url).resolve("pdfjs-dist/package.json")could fail in environments where the optional dependency is unavailable.try/catch; on failure the cache becomes""and the function returnsundefined. CallinggetDocumentwithstandardFontDataUrl: undefinedis the pre-PR shape, so we degrade to current behavior rather than crash. The existing optional-dependency error path (Optional dependency pdfjs-dist is required for PDF extraction) still owns the actually-missing case.getFactoryUrlProp(pdfjs-dist/legacy/build/pdf.mjs:15024) hard-checksval.endsWith("/")and throwsInvalid factory url ... must include trailing slashotherwise — it's specifically a forward slash, not platform-sep.node:path'sjoin(dirname(pkgPath), "standard_fonts")to compose the directory portion (so the path itself is platform-correct), then unconditionally appending/for the trailing separator pdf.js requires. On Windows this yieldsC:\...\standard_fonts/; Node's fs accepts mixed separators on Windows, and pdf.js concatenates${standardFontDataUrl}${filename}(pdfjs-dist/legacy/build/pdf.worker.mjs:40585) so the final fetch path is well-formed on both platforms.