fix: render PDF pages with self-contained fonts so text survives fontless containers

[aliasunder]

What does this PR do?

Fixes vault_read_file raw: true returning PDF page images with all text silently missing — vector graphics (rules, underlines, bullets) rendered while every glyph was dropped. Reported against a real vault PDF (a Chrome print-to-PDF cover letter); reproduced byte-for-byte against production.

Root cause (three stacked failures):

1. unpdf bundles a serverless/edge pdfjs build and resolves its documented Node defaults (disableFontFace: true, standardFontDataUrl, cMaps) via import.meta.resolve("pdfjs-dist/package.json") inside a silent catch. pdfjs-dist is only unpdf's devDependency, so in this repo's install the resolve throws and every Node default vanishes.
2. What remains is useSystemFonts: true + CSS-font-path rendering, which draws text via canvas fillText with host fonts instead of the PDF's own glyph outlines. The production container (node:24-trixie-slim) ships zero fonts, so Skia draws nothing. Verified both directions: the identical code renders text on a host with fonts and a blank page after defonting the host.
3. Even configured, the edge build can't load font/cMap data (no node:fs, and pdfjs's Node fetch treats file:// strings as literal paths), and it calls Math.sumPrecise, which Node 24 lacks.

The fix — new src/utils/pdf-engine.ts, the single entry point for every PDF read:

• Injects @napi-rs/canvas constructors into globalThis (overwriting — pdfjs's own path2d polyfill objects are rejected by napi-rs contexts, and unpdf's set-if-undefined injection loses that race), then swaps in pdfjs-dist/legacy/build/pdf.mjs via unpdf's definePDFJSModule (the Node-targeted build with runtime polyfills).
• Creates every document proxy with disableFontFace: true + useSystemFonts: false + bundled standard fonts and cMaps as plain paths — glyphs always render from font data (embedded, or pdfjs-dist's bundled base-14 set), never from the host font stack.
• Single-flight memoized init that retries after a failed attempt; both the text-extraction and page-render paths now share one configured proxy (also dropping the per-page document re-parse), and the proxy is disposed with loadingTask.destroy() rather than cleanup().
• Proper cMaps also fix latent text-extraction garbage for CJK/CID-keyed PDFs.

Also fixes the pre-existing fit-image-to-byte-budget alpha-WebP test flake (fixture shrunk 2000px → 800px; the test timed out at its 5s limit on slow runners, including on untouched main).

Verification: end-to-end through the built output in a defonted environment — an embedded-font Chrome print-to-PDF and a worst-case non-embedded base-14 Helvetica PDF both render full text and still extract text correctly. The new integration test renders the repo's non-embedded-Helvetica fixture and asserts glyph pixels — that test fails without this fix even on font-rich CI (with useSystemFonts: false, standard-font loading is the only way base-14 text renders), and mutation runs confirmed it fails when standardFontDataUrl is dropped, when the init-retry reset is removed, and when the engine points at a wrong font directory.

Type of change

• Bug fix

Checklist

• npm test passes
• npm run lint passes
• npm run prettier:check passes
• npm run build succeeds
• New MCP tools follow the naming and description conventions in AGENTS.md (n/a — no new tools)
• README or ARCHITECTURE.md updated (ARCHITECTURE.md PDF section; AGENTS.md structure tree; .devin/wiki.json page purpose)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved PDF text extraction and page rendering across environments, including systems without installed fonts.
  • Added bundled font and character-map support for more consistent PDF output.
  • Improved PDF resource cleanup after successful or failed extraction and rendering.

• Documentation

  • Updated architecture and module documentation to describe the PDF compatibility behavior and supported file handling.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	pdfjs-dist@​6.2.108

View full report

[umm-actually]

umm-actually re-reviewed at b41ded2

1 new finding(s) posted (13 tracked finding(s) across all runs).

---

umm-actually · deepseek/deepseek-v4-pro

[aliasunder]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds a shared PDF.js Node engine with bundled font and cMap support. PDF asset extraction and rendering now use a shared document proxy and loading-task destruction. Tests cover rendering, extraction, initialization retries, configuration, cleanup, and asset resolution.

Changes

PDF handling

Layer / File(s)	Summary
PDF engine bootstrap and configuration package.json, src/utils/pdf-engine.ts, AGENTS.md, ARCHITECTURE.md, .devin/wiki.json	Adds pdfjs-dist, configures the legacy Node build with bundled fonts and cMaps, injects canvas globals, and documents the new utility.
PDF engine integration tests src/utils/__tests__/pdf-engine.test.ts	Tests bundled-font rendering, text extraction, PDF.js options, initialization retry behavior, and resolved asset paths.
Asset operation proxy flow src/vault-mcp/vault-operations/asset-operations.ts, src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts	Creates document proxies through createPdfDocumentProxy, passes them to page rendering, uses the shared canvas implementation, and destroys loading tasks during cleanup. Tests cover extraction, rendering, failures, and cleanup.
Estimated code review effort: 3 (Moderate)	~20 minutes

Possibly related PRs

• aliasunder/vault-cortex#351: Introduces the asset-handling PDF flow extended by this change.
• aliasunder/vault-cortex#352: Modifies the same PDF handling path in asset-operations.ts.
• aliasunder/vault-cortex#356: Uses the related PDF page-rendering flow replaced here by the shared PDF engine.

Suggested labels: Review effort 2/5

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: PDF rendering now preserves text in containers without host fonts.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/vault-bootstrap-setup-n0o48p

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts (1)

40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the shared canvas importer identity.

expect.any(Function) accepts an unrelated importer. Export a hoisted mockCanvasImport and assert that renderPageAsImage receives that exact mock. This verifies the shared canvas integration contract.

As per coding guidelines, tests must use exact assertions.

Proposed test update

+  const mockCanvasImport = vi.fn()
   return {
+    mockCanvasImport,
     mockDestroy,
     mockCreatePdfDocumentProxy: vi.fn(() => ({
       loadingTask: { destroy: mockDestroy },
       numPages: 1,
     })),
   }
 })

 vi.mock("../../../utils/pdf-engine.js", () => ({
   createPdfDocumentProxy: mockCreatePdfDocumentProxy,
-  canvasImport: vi.fn(),
+  canvasImport: mockCanvasImport,
 }))

     expect(mockRenderPageAsImage).toHaveBeenCalledWith(configuredProxy, 1, {
-      canvasImport: expect.any(Function),
+      canvasImport: mockCanvasImport,
       scale: 2,
     })

Also applies to: 725-730

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts` around
lines 40 - 43, Update the PDF engine mock to export a hoisted mockCanvasImport,
then revise the renderPageAsImage assertions to verify it receives that exact
mock instead of expect.any(Function). Apply the same identity assertion to the
additional covered case.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/utils/__tests__/pdf-engine.test.ts`:
- Around line 105-107: Update the createPdfDocumentProxy rejection assertion to
match the complete deterministic error message rather than using the partial
string matcher. Preserve the existing “transient init failure” contract while
ensuring wrapped prefixes or suffixes do not satisfy the assertion.

In `@src/utils/pdf-engine.ts`:
- Around line 104-107: Update the initialization flow around pdfEnginePromise so
the catch handler that clears the memoized promise also re-throws the original
initialization error. Memoize that re-throwing catch result instead of attaching
a detached observer to initAttempt, preserving retry behavior while keeping the
rejection observable.

---

Nitpick comments:
In `@src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts`:
- Around line 40-43: Update the PDF engine mock to export a hoisted
mockCanvasImport, then revise the renderPageAsImage assertions to verify it
receives that exact mock instead of expect.any(Function). Apply the same
identity assertion to the additional covered case.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fc84d622-ea3c-4dfb-a6dc-69227d3db042

📥 Commits

Reviewing files that changed from the base of the PR and between 7d7d96c and a157f6c.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (8)

• .devin/wiki.json
• AGENTS.md
• ARCHITECTURE.md
• package.json
• src/utils/__tests__/pdf-engine.test.ts
• src/utils/pdf-engine.ts
• src/vault-mcp/vault-operations/__tests__/asset-operations.test.ts
• src/vault-mcp/vault-operations/asset-operations.ts

[umm-actually]

Log the raw error object instead of String(error) to preserve stack traces
Low severity · correctness · medium confidence

src/vault-mcp/vault-operations/asset-operations.ts:200 — beyond the diff's line ranges, in code the changes touch or depend on.

renderPdfPages logs logger.warn("pdf_page_render_failed", { page, error: String(error) }) when a page render fails, but String(error) discards the error's .stack, .cause, and any structured properties. A PDF page failure is likely a native binding crash or an unexpected format issue — these are the hardest failures to diagnose, and the most information-rich part (stack trace, cause chain) is the part being stripped.

Failure scenario: A PDF with an unusual font encoding causes renderPageAsImage to throw an error from the native @napi-rs/canvas binding with a stack trace pointing to the specific C++ call. The warn log records only "Error: memory access out of bounds" — no stack, no file path. The operator has no way to distinguish this from a benign allocator hiccup and cannot file a meaningful bug report.

Suggested fix

Use `describeError(error)` (the project's own utility for extracting messages from unknown throws) and also log `error` as a structured field so the logger's own error serialization preserves the stack: `logger.warn("pdf_page_render_failed", { page, error })`. The logger should handle Error objects natively — passing `error` directly lets it capture the stack.

[aliasunder]

Re the PR-level finding on String(error) in renderPdfPages' warn log: fixed in ef5f04a — the site now uses describeError, aligning the one outlier with the codebase-wide idiom (15+ existing call sites). The second half of the suggestion (also log the raw error object so the logger captures the stack) is declined: the structured logger JSON-serializes fields and has no Error-object serialization path — a raw Error would log as {} — and no existing site in the codebase passes raw error objects; describeError's [name]: message is the established contract.

---

Generated by Claude Code

[aliasunder]

Phase 1: PR Review

2 findings across 2 files.

The core implementation is sound: the pdf-engine bootstrap correctly injects canvas constructors before the pdfjs import, swaps in the legacy Node build, configures font-independent document proxies, and handles init failure with a retry-safe single-flight pattern. The asset-operations changes — proxy reuse, shared canvasImport, proper destroy() cleanup, describeError — are all correct and consistent. Module placement in utils/ satisfies both admission gates (zero domain knowledge, complete standalone primitive). AGENTS.md structure tree, wiki.json, and package.json changes are accurate.

Findings are both docstring/doc precision issues — no code bugs found.

Verdict: ship-with-minor-fixes

---

🔍 ship-check · pr-review · opus

[aliasunder]

Phase 2: Code Quality

1 finding across 5 files.

---

🔍 ship-check · code-quality · opus

[umm-actually]

Clamp maxPdfRenderPages to at least 1 at the data layer
Low severity · correctness · medium confidence

src/vault-mcp/vault-operations/asset-operations.ts:435 — beyond the diff's line ranges, in code the changes touch or depend on.

The maxPdfRenderPages is taken from params.maxPdfRenderPages without a floor — while the tool schema enforces .min(1), the data-layer function readAssetContent doesn't enforce it. If called programmatically with maxPdfRenderPages: 0, pagesToRender would be 0 and the explicit guard throws a useful error. However, MAX_PDF_RENDER_PAGES defaults to 5 in config and the env-var parser validates with .min(1), so this is only a concern for programmatic callers that bypass the tool layer. The explicit guard is already present (the if (pagesToRender === 0) block), so no behavioral gap — but the guard could be tightened to <= 0 for clarity and defense against negative values.

Failure scenario: Programmatic caller passes maxPdfRenderPages: -1 — Math.min(1, -1) yields -1. renderPdfPages loops for (let pageNumber = 1; pageNumber <= -1; pageNumber++), which never enters the loop, returning an empty array. The guard if (pages.length === 0) catches it and throws "no pages could be rendered", which is a correct error but the root cause (negative page cap) is obscured.

Suggested fix

Add `Math.max(1, params.maxPdfRenderPages)` to guard against negative or zero caps at the data layer — the tool schema handles the normal path, but the data layer should be resilient to programmatic misuse.

[aliasunder]

Re the PR-level finding on clamping maxPdfRenderPages at the data layer: declined. The value is unreachable below 1 through every real entry point — the env-var parser and the tool schema both enforce a minimum of 1, and readAssetContent is only called with config-derived values (the finding itself concedes "no behavioral gap"). More to the point, the suggested Math.max(1, …) is silent normalization, which the repo's conventions reject at boundaries: if an invalid cap ever became reachable, the right move per AGENTS.md would be explicit rejection with a clear error, not quietly rendering one page for a caller who asked for −1.

---

Generated by Claude Code

[aliasunder]

Phase 3: Test Audit

1 finding across 2 files.

pdf-engine.test.ts (NEW, 5 tests): 4 of 5 tests pass all audit dimensions cleanly. The integration tests are well-designed — spy mode keeps the real unpdf implementation while verifying configuration, the fixture uses non-embedded base-14 Helvetica to prove font-independent rendering, and the retry test uses vi.resetModules() for a clean memo. One mild assertion quality finding on the text extraction test.

asset-operations.test.ts (modified): All changes are mechanical and correct — mock renames (cleanup → destroy, getDocumentProxy → createPdfDocumentProxy) match the production API change, test names updated to reflect the new behavior, and the new "renders through the configured document proxy" test is a strict improvement over the removed "passes canvasImport and scale" test (exact reference assertions replace expect.any(Uint8Array) and expect.objectContaining). No coverage regression from the removed test.

Coverage gaps: None. Both production files have adequate test coverage for all changed and new code paths. The canvasImport export is trivially thin (one-line delegation) and tested via mock in asset-operations. The describeError swap in logging is a format detail exercised by existing catch-branch tests.

---

🔍 ship-check · test-audit · opus

[aliasunder]

Phase 4: Bug Check

1 finding across 5 files.

---

🔍 ship-check · bug-check · opus