Skip to content

ci(js-sdk): install Playwright Chromium without --with-deps #1699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mishushakov merged 3 commits into from
Aug 19, 2026
Merged

ci(js-sdk): install Playwright Chromium without --with-deps #1699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f65f602
ci(js-sdk): install Playwright Chromium without --with-deps
cursoragent Aug 19, 2026
84b5fc3
Merge remote-tracking branch 'origin/main' into cursor/author-pr-clai…
cursoragent Aug 19, 2026
818bc91
ci(js-sdk): remove Playwright install step comment per review
cursoragent Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/js_sdk_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ jobs:
path: ${{ matrix.os == 'windows-latest' && '~/AppData/Local/ms-playwright' || '~/.cache/ms-playwright' }}
key: playwright-${{ runner.os }}-${{ steps.playwright-version.outputs.version }}

- name: Install Playwright Chromium
if: matrix.runtime == 'node'
run: pnpm run playwright:install

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is wired correctly — I checked every pnpm test call site in .github/, and the reusable-workflow callers inherit this step. The loose end is local, and it's a documentation gap rather than a disagreement with the design: keeping the install off every contributor's pnpm test is the right call.

Nothing in the repo mentions playwright:install except the package.json line that defines it. Meanwhile AGENTS.md/CLAUDE.md tell contributors and coding agents to run tests with pnpm run test, and the root script is pnpm test --recursive --if-present, which reaches js-sdk's test → vitest run → the browser project. With enable-pre-post-scripts=true in .npmrc that path used to self-provision; now a fresh clone gets Playwright's own error, whose remediation hint is the generic npx playwright install (all three browsers) rather than this repo's pnpm run playwright:install.

Mirroring the description's local snippet into CONTRIBUTING.md or DEV.md would close it.


# The unit bundle test and the Cloudflare deploy config fail in CI when
# the build output is missing.
- name: Test build
Expand Down
2 changes: 1 addition & 1 deletion packages/js-sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"generate:volume-api": "openapi-typescript ../../spec/openapi-volumecontent.yml -x api_key --array-length --alphabetize --output src/volume/schema.gen.ts",
"generate:mcp": "json2ts -i ./../../spec/mcp-server.json -o src/sandbox/mcp.d.ts --unreachableDefinitions --style.singleQuote --no-style.semi",
"check-deps": "knip",
"pretest": "npx playwright install --with-deps chromium",
"playwright:install": "playwright install chromium",

Check warning on line 38 in packages/js-sdk/package.json

View check run for this annotation

Claude / Claude Code Review

Dropping --with-deps may break Windows Chromium launch on cache miss 

Removing `--with-deps` from the Playwright install (packages/js-sdk/package.json:38) also drops `install_media_pack.ps1`'s `Install-WindowsFeature Server-Media-Foundation` step on Windows, which Chromium's launch-time host validation on Windows Server hard-fails without. This is currently masked because the `actions/cache`-backed Playwright cache still holds a validation marker from before this change, but on the next cache miss (Playwright version bump, cache eviction, or new runner image) the
Comment thread
mishushakov marked this conversation as resolved.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

playwright install chromium fetches full Chromium (594MB) and the headless shell (321MB), but the browser project only ever launches the shell: vitest.config.mts sets headless: true, and moving ~/.cache/ms-playwright/chromium-1193 out of the way leaves the project passing (1 passed). The missing-browser error names it too — Executable doesn't exist at .../chromium_headless_shell-1193/chrome-linux/headless_shell.

Measured on a clean cache here:

command cold time on disk
playwright install chromium 7s 920MB
playwright install --only-shell chromium 2s 326MB

The browser project passes with only the shell installed, so --only-shell would cut ~594MB from the download and shrink the actions/cache artifact about 3x, which speeds up restore on a hit as well (the restore step was 3s on this run's ubuntu leg).

The trade-off: headed local debugging (--browser.headless=false) needs full Chromium, and a contributor would have to re-run without the flag. If you'd rather keep that available with one command, leaving this as-is is defensible — the CI win here is already the bulk of it.

@cursor cursor Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gap in the suite this script serves — and a real defect hiding in it. Follow-up material, not something to fix here.

The browser project is the SDK's only source of runtime === 'browser', but its single test does commands.run + files.read only, so the places the SDK actually behaves differently in a browser are never executed there. Branch counts from the node run:

  • utils.ts:233 cond-expr [0, 11] — the runtime === 'browser' arm of toUploadBody's gzip path (buffer to a Blob instead of streaming) has zero hits.
  • utils.ts:77 if [0, 58] — dynamicImport's browser guard is called 58 times and throws never.
  • In the browser run, if 224, cond-expr 233 and if 238 are all [0, 0]: the one browser test never calls toUploadBody at all.

tests/utils.test.ts is already written as the contract test for precisely this — line 9 is const streams = runtime !== 'browser' and five assertions read expect(streamed).toBe(streams) — but the project's include is tests/runtimes/browser/**/*.{test,spec}.tsx, so it only ever runs under node, where streams is permanently true and the test.skipIf(!streams) never skips. That is the gap SDK-292 is aiming at.

So I ran it in the browser to see what it would say: copied to tests/runtimes/browser/, two import paths fixed, no other edits. It reports 1 failed / 15 passed / 1 skipped:

FAIL toUploadBody leaves an async-iterable foreign stream alone
AssertionError: expected '[object ReadableStream]' to be 'hello'

The premise in that test ("the platform accepts any async iterable as a body") and in foreignPlatformObjects.ts:54 is true of undici, not of browsers — BodyInit has no async-iterable member. Verified directly in HeadlessChrome 140:

in a browser result
new Response(nativeStream).text() 'hello'
new Response(foreignAsyncIterableStream).text() '[object ReadableStream]'
Symbol.asyncIterator in nativeStream true (Chrome ≥ 124)

toDispatchableStream's Symbol.asyncIterator in stream clause is supposed to recognize a stream the platform will accept, so in a browser it waves a foreign/polyfilled stream through and toBlob buffers the stringified tag. A browser caller doing files.write(path, polyfilledStream) uploads the literal [object ReadableStream] instead of the data — the failure mode utils.ts:139-141 says the function exists to prevent.

I don't have a clean one-liner to offer. Gating the clause on runtime !== 'browser' fixes this case and keeps the node suite at 17/17, but it turns toUploadBody does not re-wrap a native stream when the global class was replaced red in the browser, because Chromium's native streams are async-iterable and that same clause is what rescues the replaced-global case there. The two contracts the file encodes need a nativeness probe that isn't async-iterability. Scratch files deleted; nothing committed.

@claude claude Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Removing --with-deps from the Playwright install (packages/js-sdk/package.json:38) also drops install_media_pack.ps1's Install-WindowsFeature Server-Media-Foundation step on Windows, which Chromium's launch-time host validation on Windows Server hard-fails without. This is currently masked because the actions/cache-backed Playwright cache still holds a validation marker from before this change, but on the next cache miss (Playwright version bump, cache eviction, or new runner image) the node/windows-latest browser vitest project could fail outright; consider keeping --with-deps (or an explicit Media Foundation install) on the Windows leg.

Extended reasoning...

What the bug is

packages/js-sdk/package.json:38 replaces playwright install --with-deps chromium with plain playwright install chromium, and the new step in .github/workflows/js_sdk_tests.yml runs it unconditionally for both the ubuntu-22.04 and windows-latest legs of the node matrix entry. On Linux, --with-deps just apt-installs shared libraries and fonts, and the PR's own verification shows those werent load-bearing (already the newest version). On Windows, however, --with-deps does something functionally different: it runs install_media_pack.ps1, which calls Install-WindowsFeature Server-Media-Foundation on Windows Server. That feature supplies mf.dll, mfplat.dll, msmpeg2vdec.dll, evr.dll, and avrt.dll, which Chromiums Windows build links against.

The code path that triggers it

Playwrights browser-launch path (playwright-core/lib/server/registry/index.js, _validateHostRequirementsForExecutableIfNeeded -> validateDependenciesWindows in dependencies.js) inspects the installed Chromium binarys DLL imports and throws a hard Host system is missing dependencies! error (pointing the user at Install-WindowsFeature Server-Media-Foundation) if those DLLs arent resolvable. This is not a soft warning about missing codecs — it prevents chromium.launch() from succeeding at all, which would fail the browser vitest project (packages/js-sdk/vitest.config.mts) that pnpm test runs unconditionally on the node matrix legs, including Windows.

Why nothing currently prevents it

The validation is skipped only when a DEPENDENCIES_VALIDATED marker file already exists inside the executables cache directory and is less than 30 days old. That directory is exactly the one keyed and persisted by the workflows actions/cache step (playwright-${{ runner.os }}-${{ steps.playwright-version.outputs.version }}). Because earlier CI runs (pre-PR) used --with-deps and therefore both enabled the Windows feature and produced a valid marker, todays cache hits skip revalidation and the leg passes green. That is precisely why this regression is invisible in the PRs own CI run.

Step-by-step proof of the failure condition

  1. Playwright is bumped (or the cache is evicted, or GitHub rolls a new windows-latest image) -> the cache key changes -> cache miss on windows-latest.
  2. pnpm run playwright:install runs playwright install chromium fresh, with no --with-deps, so install_media_pack.ps1 never runs and Server-Media-Foundation is never enabled on this ephemeral runner.
  3. pnpm test starts the browser vitest project, which calls chromium.launch().
  4. Playwright validates host requirements, finds no recent DEPENDENCIES_VALIDATED marker (fresh extraction), and inspects the DLL imports.
  5. If Server-Media-Foundation is not part of the runners base image, the missing DLLs cause validateDependenciesWindows to throw, browser fails, and the required node/windows-latest leg (which gates SDK Tests Status and the release publish step) goes red — for reasons unrelated to any actual product change.

The PRs own timing table is suggestive here: it attributes 4m31s on windows-latest to a "DISM Media Foundation enable" step, which is consistent with the feature not being preinstalled on that runner image (though, as one reviewer pointed out, Install-WindowsFeature can also be slow even as a no-op, so this data point alone isnt fully conclusive).

Why this is worth flagging despite the uncertainty

The PRs verification section only exercised the change on Linux/apt; the Windows Media-Foundation path was never tested end-to-end with a real cache miss, so the risk is unverified either way. The blast radius is limited to CI/dev-tooling (no effect on published SDK behavior), and the failure is conditional on a future cache miss rather than immediate — which is why this is a nit rather than blocking. But the fix is cheap and asymmetric: keep --with-deps (or a Install-WindowsFeature Server-Media-Foundation step) gated to the Windows leg only, while keeping the Linux leg on the lighter playwright install chromium. That preserves the PRs actual goal (killing the 20-minute apt stall on Linux) without introducing a latent, hard-to-diagnose failure mode on Windows that only surfaces after the cache masking wears off.

"test:bun": "bunx --bun vitest run --project unit --project connectionConfig --project template",
"test:cf": "vitest run --config tests/runtimes/cloudflare/vitest.config.mts",
"test:cf:deploy": "vitest run --config tests/runtimes/cloudflare-deploy/vitest.config.mts",
Expand Down
Loading