You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #201 (issue #117) fixed the pdfjs optional-Canvas/DOM warnings and added a regression test that simulates a canvas-free install in a child process. That closed every acceptance criterion from #117 except the last one:
CI covers at least one optional-dependency-absent or packaged-bundle target in addition to a normal install.
This issue tracks that remaining item.
Current state
.github/workflows/release.yml is the only workflow in the repo. It:
triggers on v* tags and workflow_dispatch only,
installs with a plain npm ci,
and never runs the test suite (vitest does not appear in it).
So there is no pull-request CI at all today: no npm test, and no install variant with optional dependencies omitted. Verification currently depends on a maintainer running the suite locally, on a machine where @napi-rs/canvas is usually present — which is exactly the environment that hides the class of bug #117 described.
Why it matters
@napi-rs/canvas is an optional transitive dependency of pdfjs-dist. A normal dev install has it, so the regression it guards against is invisible locally by default. The environments that lack it are the ones users actually get:
npm ci --omit=optional,
any install where the native optional package fails to build,
the SEA bundle, which does not ship it (scripts/bundle-sea.ts externalizes only better-sqlite3 and playwright-core).
The regression test added in #201 (src/tools/os/read-document/extractors/pdf-extractor.canvas-warnings.test.ts) builds its own canvas-free sandbox and runs pdfjs in a fresh child process, so it does catch a regression under a normal install. But nothing runs it automatically on a PR, and no job exercises a genuinely optional-dependency-absent tree.
Proposed change
Add a test workflow that runs on pull requests and pushes to main, with two install variants:
Both should run npx tsc -p tsconfig.json --noEmit and the test suite.
Worth deciding as part of this:
Flaky tests. A full npx vitest run on main currently has a handful of environment-dependent failures — timing-sensitive tests (src/agent/parallel-tool-calls.integration.test.ts, src/sidecar/send-message-concurrency.test.ts), some src/tools/os/git/* tests, and src/tools/os/fs-glob-real.test.ts, which searches $HOME for a specific personal document and cannot pass on a clean runner. These were confirmed failing on unmodified main, independent of Suppress pdfjs Canvas/DOM warnings in text-only PDF extraction #201. A green PR gate needs them quarantined, made hermetic, or excluded first — otherwise the job will be red from day one and get ignored.
Scope. Running the whole suite on every PR may be more than is wanted initially. A narrower first step is to run the --omit=optional variant over src/tools/os/read-document/ only, which is what Suppress pdfjs Canvas/DOM warnings in text-only PDF extraction #117 concerns, and widen later.
Cost. Whether to run the matrix on all platforms in release.yml or just ubuntu-latest for the test job.
Acceptance criteria
A workflow runs on pull requests targeting main.
It includes at least one job installing with optional dependencies omitted (or building the packaged bundle) in addition to a normal install.
Summary
PR #201 (issue #117) fixed the pdfjs optional-Canvas/DOM warnings and added a regression test that simulates a canvas-free install in a child process. That closed every acceptance criterion from #117 except the last one:
This issue tracks that remaining item.
Current state
.github/workflows/release.ymlis the only workflow in the repo. It:v*tags andworkflow_dispatchonly,npm ci,vitestdoes not appear in it).So there is no pull-request CI at all today: no
npm test, and no install variant with optional dependencies omitted. Verification currently depends on a maintainer running the suite locally, on a machine where@napi-rs/canvasis usually present — which is exactly the environment that hides the class of bug #117 described.Why it matters
@napi-rs/canvasis an optional transitive dependency ofpdfjs-dist. A normal dev install has it, so the regression it guards against is invisible locally by default. The environments that lack it are the ones users actually get:npm ci --omit=optional,scripts/bundle-sea.tsexternalizes onlybetter-sqlite3andplaywright-core).The regression test added in #201 (
src/tools/os/read-document/extractors/pdf-extractor.canvas-warnings.test.ts) builds its own canvas-free sandbox and runs pdfjs in a fresh child process, so it does catch a regression under a normal install. But nothing runs it automatically on a PR, and no job exercises a genuinely optional-dependency-absent tree.Proposed change
Add a test workflow that runs on pull requests and pushes to
main, with two install variants:npm ci— the normal install.npm ci --omit=optional— the deployment shape that reproduces Suppress pdfjs Canvas/DOM warnings in text-only PDF extraction #117.Both should run
npx tsc -p tsconfig.json --noEmitand the test suite.Worth deciding as part of this:
npx vitest runonmaincurrently has a handful of environment-dependent failures — timing-sensitive tests (src/agent/parallel-tool-calls.integration.test.ts,src/sidecar/send-message-concurrency.test.ts), somesrc/tools/os/git/*tests, andsrc/tools/os/fs-glob-real.test.ts, which searches$HOMEfor a specific personal document and cannot pass on a clean runner. These were confirmed failing on unmodifiedmain, independent of Suppress pdfjs Canvas/DOM warnings in text-only PDF extraction #201. A green PR gate needs them quarantined, made hermetic, or excluded first — otherwise the job will be red from day one and get ignored.--omit=optionalvariant oversrc/tools/os/read-document/only, which is what Suppress pdfjs Canvas/DOM warnings in text-only PDF extraction #117 concerns, and widen later.release.ymlor justubuntu-latestfor the test job.Acceptance criteria
main.main, so a red result is a real signal.