fix(pdf): provide standardFontDataUrl for Node fallback rendering by anyech · Pull Request #51465 · openclaw/openclaw

anyech · 2026-03-21T06:26:33Z

Summary

pass standardFontDataUrl into the Node PDF extraction-fallback renderer
use the working filesystem-path form (.pathname) instead of file://...href
add focused regression coverage for the fallback render path

Closes #51455.

Problem

The non-native PDF extraction fallback could emit repeated PDF.js warnings during page-image rendering in Node:

UnknownErrorException: Ensure that the standardFontDataUrl API parameter is provided.

The analysis still succeeded, but the renderer was missing the standard-font configuration PDF.js expects.

Root cause

The fallback path called getDocument({ data, disableWorker: true }) without standardFontDataUrl before later calling page.render(...).

Fix

Provide:

const standardFontDataUrl = new URL(
  "../node_modules/pdfjs-dist/standard_fonts/",
  import.meta.url,
).pathname;

and pass it into getDocument(...).

Why `.pathname`

Local verification showed that .href changed the failure into font-load warnings in this Node runtime path, while .pathname eliminated the warning both locally and in a live post-restart check.

Testing

corepack pnpm exec vitest run --config vitest.config.ts src/media/pdf-extract.test.ts src/agents/tools/pdf-tool.test.ts
- 52 passed

Notes

AI-assisted change
Narrow PR: PDF extraction-fallback only
No full repo pnpm test run

greptile-apps · 2026-03-21T06:30:02Z

Greptile Summary

This PR addresses the UnknownErrorException: Ensure that the standardFontDataUrl API parameter is provided warning emitted by PDF.js during Node fallback rendering by wiring standardFontDataUrl into the getDocument call and adding a regression test. The concept is correct, but the relative path used to locate the standard fonts directory has a depth error that will prevent fonts from actually loading at runtime.

Path traversal is one level too shallow: "../node_modules/pdfjs-dist/standard_fonts/" resolves from the compiled output at dist/media/pdf-extract.js to dist/node_modules/pdfjs-dist/standard_fonts/ — a directory that does not exist. The correct path is "../../node_modules/pdfjs-dist/standard_fonts/" to reach the repo-root node_modules/.
The original warning is suppressed but fonts may still not load: providing any truthy string for standardFontDataUrl silences the "parameter not provided" error, so local testing may appear clean while font fetches silently 404 at the wrong path.
Test does not guard against path regressions: getDocument is fully mocked and path existence is never verified with fs.existsSync, so the test passes regardless of whether the resolved directory exists.
The type declaration update in pdfjs-dist-legacy.d.ts is clean and correct.

Confidence Score: 2/5

Not safe to merge — the relative path resolves to a non-existent directory at runtime, meaning standard fonts will still fail to load despite the fix.
The fix correctly identifies the missing standardFontDataUrl parameter and the approach of computing a filesystem path from import.meta.url is sound. However, the relative path "../node_modules/..." is one ../ short of the repo root when evaluated from dist/media/pdf-extract.js at runtime. This means font file fetches will silently 404, leaving the underlying rendering problem present even though the specific error message is suppressed. The regression test also doesn't guard against this because it fully mocks getDocument and never checks the path's existence on disk. These together mean the PR does not fully achieve its stated goal and needs a one-character fix before merging.
src/media/pdf-extract.ts (line 52) — fix the relative path from "../node_modules/..." to "../../node_modules/..."

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/media/pdf-extract.ts
Line: 52-53

Comment:
**Wrong depth in relative path — fonts won't be found at runtime**

The relative path `"../node_modules/pdfjs-dist/standard_fonts/"` is one level too shallow.

The compiled file lives at `dist/media/pdf-extract.js`, so `import.meta.url` at runtime resolves to `file:///…/dist/media/pdf-extract.js`. Resolving `../` from `dist/media/` gives `dist/`, not the repo root. The full resolved path would be:

```
/path/to/openclaw/dist/node_modules/pdfjs-dist/standard_fonts/
```

…which does not exist. The actual package is installed at `/path/to/openclaw/node_modules/pdfjs-dist/standard_fonts/`.

Reaching the repo root from `dist/media/` requires two levels up:

```suggestion
  const standardFontDataUrl = new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)
    .pathname;
```

This silently causes PDF.js to fail to load standard fonts even though the parameter is now wired in. The specific `"standardFontDataUrl API parameter is provided"` warning disappears (because the param is now truthy), but font fetches will 404 and PDF rendering quality may still be degraded. The test does not catch this because `getDocument` is fully mocked and the path's existence is never verified.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/media/pdf-extract.test.ts
Line: 55-58

Comment:
**Test does not verify the resolved path exists on disk**

The assertions check that `standardFontDataUrl` is a non-empty string containing `"standard_fonts"`, but they never verify the path actually points to an existing directory. This means the test passes even when the path resolves to a non-existent location (e.g., `dist/node_modules/pdfjs-dist/standard_fonts/`).

Consider adding a `fs.existsSync` guard to catch path regressions:

```ts
import { existsSync } from "node:fs";
// …
expect(existsSync(params.standardFontDataUrl)).toBe(true);
```

This would have caught the depth-mismatch in the relative path before the PR landed.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "Fix PDF fallback sta..."}

greptile-apps · 2026-03-21T06:30:05Z

+  const standardFontDataUrl = new URL("../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)
+    .pathname;


Wrong depth in relative path — fonts won't be found at runtime

The relative path "../node_modules/pdfjs-dist/standard_fonts/" is one level too shallow.

The compiled file lives at dist/media/pdf-extract.js, so import.meta.url at runtime resolves to file:///…/dist/media/pdf-extract.js. Resolving ../ from dist/media/ gives dist/, not the repo root. The full resolved path would be:

/path/to/openclaw/dist/node_modules/pdfjs-dist/standard_fonts/

…which does not exist. The actual package is installed at /path/to/openclaw/node_modules/pdfjs-dist/standard_fonts/.

Reaching the repo root from dist/media/ requires two levels up:

Suggested change

const standardFontDataUrl = new URL("../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)

.pathname;

const standardFontDataUrl = new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)

.pathname;

This silently causes PDF.js to fail to load standard fonts even though the parameter is now wired in. The specific "standardFontDataUrl API parameter is provided" warning disappears (because the param is now truthy), but font fetches will 404 and PDF rendering quality may still be degraded. The test does not catch this because getDocument is fully mocked and the path's existence is never verified.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/media/pdf-extract.ts Line: 52-53 Comment: **Wrong depth in relative path — fonts won't be found at runtime** The relative path `"../node_modules/pdfjs-dist/standard_fonts/"` is one level too shallow. The compiled file lives at `dist/media/pdf-extract.js`, so `import.meta.url` at runtime resolves to `file:///…/dist/media/pdf-extract.js`. Resolving `../` from `dist/media/` gives `dist/`, not the repo root. The full resolved path would be: ``` /path/to/openclaw/dist/node_modules/pdfjs-dist/standard_fonts/ ``` …which does not exist. The actual package is installed at `/path/to/openclaw/node_modules/pdfjs-dist/standard_fonts/`. Reaching the repo root from `dist/media/` requires two levels up: ```suggestion const standardFontDataUrl = new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url) .pathname; ``` This silently causes PDF.js to fail to load standard fonts even though the parameter is now wired in. The specific `"standardFontDataUrl API parameter is provided"` warning disappears (because the param is now truthy), but font fetches will 404 and PDF rendering quality may still be degraded. The test does not catch this because `getDocument` is fully mocked and the path's existence is never verified. How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-21T06:30:06Z

+    });
+    expect(typeof params.standardFontDataUrl).toBe("string");
+    expect(params.standardFontDataUrl).toContain("standard_fonts");
+    expect(params.standardFontDataUrl.endsWith("/standard_fonts/")).toBe(true);


Test does not verify the resolved path exists on disk

The assertions check that standardFontDataUrl is a non-empty string containing "standard_fonts", but they never verify the path actually points to an existing directory. This means the test passes even when the path resolves to a non-existent location (e.g., dist/node_modules/pdfjs-dist/standard_fonts/).

Consider adding a fs.existsSync guard to catch path regressions:

import { existsSync } from "node:fs"; // … expect(existsSync(params.standardFontDataUrl)).toBe(true);

This would have caught the depth-mismatch in the relative path before the PR landed.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/media/pdf-extract.test.ts Line: 55-58 Comment: **Test does not verify the resolved path exists on disk** The assertions check that `standardFontDataUrl` is a non-empty string containing `"standard_fonts"`, but they never verify the path actually points to an existing directory. This means the test passes even when the path resolves to a non-existent location (e.g., `dist/node_modules/pdfjs-dist/standard_fonts/`). Consider adding a `fs.existsSync` guard to catch path regressions: ```ts import { existsSync } from "node:fs"; // … expect(existsSync(params.standardFontDataUrl)).toBe(true); ``` This would have caught the depth-mismatch in the relative path before the PR landed. How can I resolve this? If you propose a fix, please make it concise.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8e89c656d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-21T06:31:13Z

+  const standardFontDataUrl = new URL("../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)
+    .pathname;


Resolve standardFontDataUrl from both source and dist layouts

When this code runs from source rather than a built install, import.meta.url is .../src/media/pdf-extract.ts, so ../node_modules/pdfjs-dist/standard_fonts/ resolves to src/node_modules/... instead of the repo root. The main dev entrypoints (pnpm openclaw / pnpm dev via scripts/run-node.mjs) still execute source files, so scanned/image-only PDFs in those workflows will keep failing once pdf.js tries to load fallback fonts. The added test only asserts that some standard_fonts string is passed, so it won't catch this broken path.

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4174400354

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-26T20:40:29Z

+    });
+    expect(typeof params.standardFontDataUrl).toBe("string");
+    expect(params.standardFontDataUrl).toContain("standard_fonts");
+    expect(params.standardFontDataUrl.endsWith("standard_fonts/")).toBe(true);


Make path assertion separator-agnostic

fileURLToPath(...) returns Windows-style paths (ending in \standard_fonts\), so this hardcoded "standard_fonts/" suffix check fails on Windows even when standardFontDataUrl is correct. Because this PR touches src/**, the Windows shard in .github/workflows/ci.yml runs pnpm test, so this assertion can block CI for a non-bug in runtime behavior; please make the suffix check accept both separators (or assert via path.basename/dirname).

Useful? React with 👍 / 👎.

anyech · 2026-04-19T07:32:12Z

Thanks — I checked the current failing CI lane by looking at the check-run annotations first rather than starting with the full workflow logs.

The concrete failing annotation on the current head points to:

src/cli/cron-cli/shared.test.ts:173
getCronChannelOptions > falls back to a generic channel placeholder when no plugins are loaded
expected: last|<channel-id>
received: last|telegram|signal

I then rechecked that seam in a disposable checkout of this PR branch and ran the narrowest local verification I could exercise:

corepack pnpm exec vitest run --config vitest.config.ts src/cli/cron-cli/shared.test.ts

That targeted test passes locally in the disposable checkout.

I also diffed this PR branch against the current mainline. The branch only touches:

src/media/pdf-extract.test.ts
src/media/pdf-extract.ts
src/types/pdfjs-dist-legacy.d.ts

It does not touch src/cli/cron-cli/shared.ts or src/cli/cron-cli/shared.test.ts, so at the moment this looks more like an adjacent or stale CI failure than something introduced by the standardFontDataUrl change itself.

Caveat: this is not a clean latest-version stage repro of the Windows failure path. It is a narrower annotation-first read plus isolated disposable-checkout verification.

Given that, I would prefer not to widen this PR to fix an apparently unrelated cron-cli test failure. If helpful, I can rerun the Windows lane or handle that as a separate follow-up, but I would rather keep this PR scoped to the PDF fix.

anyech · 2026-04-19T07:52:57Z

Thanks — I checked the current failing CI lane by looking at the check-run annotations first rather than starting with the full workflow logs.

The concrete failing annotation on the current head points to:

src/cli/cron-cli/shared.test.ts:173
getCronChannelOptions > falls back to a generic channel placeholder when no plugins are loaded
expected: last|<channel-id>
received: last|telegram|signal

I then rechecked that seam in a disposable checkout of this PR branch and ran the narrowest local verification I could exercise:

corepack pnpm exec vitest run --config vitest.config.ts src/cli/cron-cli/shared.test.ts

That targeted test passes locally in the disposable checkout.

I also diffed this PR branch against the current mainline. The branch only touches:

src/media/pdf-extract.test.ts
src/media/pdf-extract.ts
src/types/pdfjs-dist-legacy.d.ts

It does not touch src/cli/cron-cli/shared.ts or src/cli/cron-cli/shared.test.ts, so from this narrow check it looks more likely to be adjacent than introduced by the standardFontDataUrl change itself.

Caveat: this is not a clean latest-version stage repro of the Windows failure path. It is a narrower annotation-first read plus isolated disposable-checkout verification.

Given that, I would keep this PR scoped to the PDF fix unless stronger evidence ties the cron-cli failure to this change. If helpful, I can rerun the Windows lane or handle that as a separate follow-up.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e6534e5c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-19T09:02:25Z

-  const pdf = await getDocument({ data: new Uint8Array(buffer), disableWorker: true }).promise;
+  const pdfJsModule = await loadPdfJsModule();
+  const standardFontDataUrl = fileURLToPath(
+    new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url),


Resolve standard-font path from package location

standardFontDataUrl is computed with new URL("../../node_modules/...", import.meta.url), which only works when the emitted file stays two directories below the package root. This repo’s build uses tsdown without unbundle: true (tsdown.config.ts), so bundled output can place this code in root-level entry/chunk files; in that layout ../../node_modules resolves outside the OpenClaw package and the font directory is missing, so Node fallback rendering still emits the same PDF.js standard-font warnings in built installs. Resolve from pdfjs-dist/package.json (e.g. via require.resolve) instead of assuming a fixed relative depth.

Useful? React with 👍 / 👎.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e6534e5c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-04-19T09:03:03Z

-  const pdf = await getDocument({ data: new Uint8Array(buffer), disableWorker: true }).promise;
+  const pdfJsModule = await loadPdfJsModule();
+  const standardFontDataUrl = fileURLToPath(
+    new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url),


Resolve standard font path from pdfjs module location

Computing standardFontDataUrl via new URL("../../node_modules/...", import.meta.url) is still layout-dependent and can miss in bundled output: this repo builds with bundled mode (not unbundle), and we already expect hashed top-level dist/*.js chunks in tests (for example src/plugin-sdk/root-alias.test.ts), so if this module is emitted at dist/*.js the resulting path becomes <package-parent>/node_modules/node_modules/pdfjs-dist/standard_fonts, which does not exist. In that case PDF.js cannot load standard fonts and the fallback renderer reverts to the same warning-prone behavior this change is trying to fix.

Useful? React with 👍 / 👎.

…edup provider-hook prose list, progressive-disclose bundled-provider examples

@andrejtr

Load Feishu setup surfaces through a setup-only barrel so onboarding does not import the Lark SDK before bundled runtime deps are staged.\n\nThanks @andrejtr.\n\nCo-authored-by: andrejtr <[email protected]>

openclaw-barnacle · 2026-04-24T01:07:24Z

Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch.

openclaw-barnacle · 2026-04-24T01:07:42Z

Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch.

chatgpt-codex-connector · 2026-04-24T01:10:14Z

💡 Codex Review

openclaw/src/media/pdf-extract.ts

Lines 96 to 97 in 77d3966

    
           const standardFontDataUrl = fileURLToPath( 
        
             new URL("../../node_modules/pdfjs-dist/standard_fonts/", import.meta.url),

Resolve standard fonts path from package root

Compute standardFontDataUrl without assuming this file stays two directories under the package root. The bundled build emits root-level dist/*.js entry/chunk files, and when this code runs from those artifacts ../../node_modules/pdfjs-dist/standard_fonts/ resolves to the parent package tree (for example .../node_modules/node_modules/...), so the directory is missing and PDF.js still cannot load standard fonts during fallback rendering. Resolving from the installed pdfjs-dist package location avoids this layout-dependent failure.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

openclaw-barnacle Bot added the size: S label Mar 21, 2026

greptile-apps Bot reviewed Mar 21, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Mar 21, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Mar 26, 2026

View reviewed changes

anyech force-pushed the fix/pdf-standard-font-data-url branch from 4174400 to c76b9e5 Compare March 31, 2026 07:08

chatgpt-codex-connector Bot reviewed Apr 19, 2026

View reviewed changes

Takhoffman and others added 19 commits April 23, 2026 12:36

fix(gateway): disarm wrapper timeout before teardown (openclaw#70704)

d39e34d

test: remove canvas reload sleep

bff9f10

docs(plugins): architecture — sentence-case title, CardGroup intro, d…

12de62b

…edup provider-hook prose list, progressive-disclose bundled-provider examples

ci: add duplicate PR cleanup workflow

184c4e3

fix(feishu): keep setup entry off runtime SDK (openclaw#70339)

23c90ae

Load Feishu setup surfaces through a setup-only barrel so onboarding does not import the Lark SDK before bundled runtime deps are staged.\n\nThanks @andrejtr.\n\nCo-authored-by: andrejtr <[email protected]>

perf: reduce installer version parsing overhead

7c974ca

test: use lightweight install version fixture

604ce85

fix(qqbot): require auth for bot-approve

9694a45

test: dedupe Anthropic cache policy fixtures

359a656

test: dedupe session lock symlink fixtures

844f1dd

test: dedupe subagent announcement fixtures

ce681be

test: dedupe coding tool session store fixtures

961ab16

test: reuse channel plugin fixture

fd3bb6f

test: share temp plugin extension fixtures

9d8690d

test: share gateway live env helpers

bd2553e

test: share CLI runtime mock helper

140a8cf

test: reuse CLI runtime mock in gateway tests

42c6e2d

test: reuse CLI runtime mock in daemon tests

d0303e2

test: stabilize full-suite flakes

310b1ee

openclaw-barnacle Bot closed this Apr 24, 2026

openclaw-barnacle Bot added size: XL and removed size: S labels Apr 24, 2026

anyech mentioned this pull request Apr 24, 2026

fix(pdf): resolve standard fonts from pdfjs package root #70936

Merged

solomonneas mentioned this pull request Apr 26, 2026

Media: fix PDF text extraction by setting standardFontDataUrl #62175

Closed

7 tasks

clawsweeper Bot mentioned this pull request Apr 29, 2026

bug: pdf extraction fallback renderer omits standardFontDataUrl and emits PDF.js warnings in Node #51455

Closed

		const standardFontDataUrl = new URL("../node_modules/pdfjs-dist/standard_fonts/", import.meta.url)
		.pathname;

Uh oh!

Uh oh!

Conversation

anyech commented Mar 21, 2026

Summary

Problem

Root cause

Fix

Why .pathname

Testing

Notes

Uh oh!

greptile-apps Bot commented Mar 21, 2026

Greptile Summary

Confidence Score: 2/5

Uh oh!

greptile-apps Bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

anyech commented Apr 19, 2026

Uh oh!

anyech commented Apr 19, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

openclaw-barnacle Bot commented Apr 24, 2026

Uh oh!

openclaw-barnacle Bot commented Apr 24, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 24, 2026

💡 Codex Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

Why `.pathname`