Skip to content

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

Merged
mishushakov merged 3 commits into
mainfrom
cursor/author-pr-claiming-e227
Aug 19, 2026
Merged

ci(js-sdk): install Playwright Chromium without --with-deps#1699
mishushakov merged 3 commits into
mainfrom
cursor/author-pr-claiming-e227

Conversation

@cursor

@cursor cursor Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Supersedes #1698 (claimed via /sdk claim by @mishushakov). Please close #1698 in favour of this PR — I have no write access to close it myself.

This is a straight clone: the commit f65f602 from #1698 is applied here unmodified (original authorship and the Co-authored-by: Mish Ushakov trailer preserved), with origin/main merged in so the branch is current — main had moved one commit ahead (#1693), which touches none of the two files in this PR. The diff against main is identical to the original: .github/workflows/js_sdk_tests.yml and packages/js-sdk/package.json. Per the claim instructions, nothing was reviewed or changed.

The original description follows, verbatim.


Closes SDK-339. Related: SDK-292, which introduced the browser project this install serves.

Problem

packages/js-sdk/package.json had a pretest hook running npx playwright install --with-deps chromium. --with-deps shells out to apt on Linux, and to a DISM Media Foundation enable on Windows, on every invocation — regardless of whether the workflow's Playwright browser cache hit. On one Test JS SDK run that hook was 90% of the Node leg:

leg step time
node / ubuntu-22.04 Run Node tests total 23m21s
pretest (--with-deps) 20m57s
vitest run (101 files, 100 passed) 2m23s
node / windows-latest pretest DISM Media Foundation enable 4m31s

The browser cache worked fine (Cache hit for: playwright-Linux-1.55.1, restored in 3s). The time went to apt: apt-get update 1m42s, then 18.4 MB fetched in 18m59s at 16.1 kB/s off a stalling Azure Ubuntu mirror (fonts-wqy-zenhei alone stalled 7m49s).

The mirror stall is transient; being on that path at all is the structural problem. Every shared library Chromium needs (libnss3, libgbm1, libdrm2, libcairo2, xvfb, …) was already already the newest version on the runner image — the only 9 new packages were CJK/Cyrillic fonts (fonts-wqy-zenhei, fonts-ipafont-gothic, xfonts-*) that the single headless browser test never renders. For comparison, in the same run the bun (2m44s), deno (2m41s) and cloudflare (2m1s) legs run the same test code with no Playwright pretest.

Change

  • packages/js-sdk/package.json: replace the pretest hook with an explicit playwright:install script (playwright install chromium, no --with-deps).
  • .github/workflows/js_sdk_tests.yml: run it as its own step gated on matrix.runtime == 'node', right after the existing browser-cache step, with a comment recording why --with-deps is omitted.

Moving it out of pretest also keeps it off every local pnpm test, including for contributors who never touch the browser project.

Usage

CI installs the browser as a distinct, cache-backed step:

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

Locally, the browser project needs Chromium once per Playwright version:

cd packages/js-sdk
pnpm run playwright:install   # ~7s cold, ~0.8s once installed
pnpm test

Without it, the browser project fails with Playwright's own "Executable doesn't exist … run playwright install" message; the other projects (unit, template, connectionConfig) are unaffected.

Verification

Run on this branch with no prior Playwright deps installed on the machine:

  • pnpm run playwright:install: 6.5s cold (Chromium headless shell + ffmpeg, no apt), 0.78s as a no-op afterwards.
  • pnpm exec vitest run --project browser: 1 passed. Chromium launches and drives a real sandbox without any --with-deps packages, confirming the fonts and libs weren't load-bearing.
  • pnpm build + full pnpm test: 101 files, 99 passed / 1 skipped in 2m34s. The one failure is tests/sandbox/network.test.ts > injected header is reflected by the httpbin sidecar, which fails with 404: template 'httpbin' not found — it needs a prebuilt httpbin template that this agent's API key doesn't have, unrelated to this change.
  • pnpm run format, pnpm run lint, pnpm run typecheck clean for packages/js-sdk (the recursive root scripts fail only in packages/python-sdk, where uv isn't installed in this environment).
  • pnpm run check-deps (knip) reports no new findings; playwright is still resolved as a used devDependency through the new script.

No changeset: this touches only dev tooling and CI, with no change to published behavior (the pretest/playwright:install scripts are inert for consumers of the package). The commit that originally added the hook, #977, likewise shipped without one.

Open in Web View Automation 

cursoragent and others added 2 commits August 19, 2026 16:54
The `pretest` hook ran `npx playwright install --with-deps chromium`,
which shells out to apt on Linux and to a DISM Media Foundation enable on
Windows on every invocation, even when the cached browser is restored.
That dominated the Node legs: 20m57s of a 23m21s step on ubuntu-22.04
(18m59s of it downloading 18.4 MB of CJK/Cyrillic fonts at 16 kB/s) and
4m31s per run on windows-latest, while vitest itself takes ~2m20s.

Every shared library Chromium needs is already on the runner images, and
the headless `browser` project renders none of the fonts `--with-deps`
adds, so drop it. Moving the install from `pretest` into a Node-only
workflow step also keeps it out of every local `pnpm test`.

Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
…ming-e227

Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
@linear-code

linear-code Bot commented Aug 19, 2026

Copy link
Copy Markdown

SDK-339

@changeset-bot

changeset-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 818bc91

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Package Artifacts

Built from 38c5de1. Download artifacts from this workflow run.

JS SDK (e2b@2.41.1-cursor-author-pr-claiming-e227.0):

npm install ./e2b-2.41.1-cursor-author-pr-claiming-e227.0.tgz

CLI (@e2b/cli@2.16.3-cursor-author-pr-claiming-e227.0):

npm install ./e2b-cli-2.16.3-cursor-author-pr-claiming-e227.0.tgz

Python SDK (e2b==2.41.0+cursor.author.pr.claiming.e227):

pip install ./e2b-2.41.0+cursor.author.pr.claiming.e227-py3-none-any.whl

Comment thread .github/workflows/js_sdk_tests.yml Outdated
Comment on lines +99 to +105
# `--with-deps` is deliberately omitted. It shells out to apt on Linux and
# to a DISM Media Foundation enable on Windows on every run, cache hit or
# not, which cost ~21m and ~4m30s respectively. The runner images already
# ship every shared library Chromium needs; the only packages
# `--with-deps` pulled in were CJK/Cyrillic fonts that the headless
# `browser` project never renders. Without it this step is a no-op once
# the cache above hits.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove the comment

@mishushakov

Copy link
Copy Markdown
Member

@cursor check

@cursor

cursor Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback: removed the 7-line comment above the Install Playwright Chromium step in .github/workflows/js_sdk_tests.yml (the block explaining why --with-deps is omitted).

Pushed as 818bc91d9 on cursor/author-pr-claiming-e227. The workflow step is unchanged; only the comment is gone.

Open in Web Open in Cursor 

Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
@mishushakov
mishushakov marked this pull request as ready for review August 19, 2026 17:06

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 818bc91d90

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/js-sdk/package.json
@mishushakov
mishushakov enabled auto-merge (squash) August 19, 2026 17:12

@cursor cursor Bot left a comment

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.

No TASTE.md surface here; the change itself verifies clean, with one worthwhile follow-up

The diff is one workflow step plus one scripts entry — no src/ file, no export, no docstring — so the SDK design principles have nothing to bite on. I spent the run verifying the change instead.

What checked out

In this PR's own CI (run 32279418801, both Node legs green):

leg Install Playwright Chromium Run Node tests browser project
node / ubuntu-22.04 2s (Cache hit for: playwright-Linux-1.55.1) 2m22s browser (chromium) run.test.tsx (1 test) 840ms; 101 files passed, 1 skipped
node / windows-latest 5s 2m29s passed in 3.6s; 101 files passed, 1 skipped

Baselines from runs on concurrent branches that don't carry this change: Run Node tests (which still included the pretest hook) took 8m30s and 3m43s on ubuntu, 5m23s and 5m57s on windows. So the saving is real even when apt behaves — roughly 1m20s to 6m on Linux and about 3m on Windows, the latter being the deterministic DISM cost. The 20m57s in the description is the tail case rather than the expected win; worth keeping straight before someone cites it as the norm.

Locally, on a bare Ubuntu 24.04 VM with an empty ~/.cache/ms-playwright and playwright install-deps never run:

  • pnpm run playwright:install took 7s cold and 0s warm (description says 6.5s / 0.78s).
  • The browser project then passes, which is a second independent confirmation that the apt fonts and libs weren't load-bearing — the Windows leg passing without the Media Foundation enable is the first.
  • With no browser present, pnpm exec vitest run --project browser exits 1 and prints Playwright's own boxed remedy. Losing the auto-install fails loudly instead of silently skipping the suite, which was the main risk I wanted to rule out.
  • pnpm run check-deps produces byte-identical output on base and HEAD (both exit 1 on the same pre-existing findings), and playwright is not among the four flagged unused devDependencies, so the knip claim holds. Nothing in CI gates knip, for what it's worth.

On completeness of the pretest removal: js_sdk_tests.yml is the only workflow that mentions Playwright, and its only callers are sdk_tests.yml, release.yml and release-candidate.yml, so every CI path that runs the browser project now installs the browser explicitly. test:bun, test:deno and test:cf pass explicit --project lists that exclude browser, and the logs confirm those legs skip the new step. No pretest reference is left anywhere in the tree.

Shipping without a changeset matches precedent: ci(...)-scoped PRs #1662, #1646, #1632, #1622 and #1588 all merged with zero .changeset/ files (#1639 is the lone exception and it was itself about changesets). The already-resolved P1 from chatgpt-codex-connector doesn't apply — npm never runs a dependency's pretest, so renaming a dev script is invisible to consumers.

Worth acting on

The browser project never launches full Chromium, so this install can be a third of its size — details and measurements in the inline comment.

The documented way to run tests now fails on a fresh clone. AGENTS.md / CLAUDE.md tells contributors to run tests with pnpm run test; the root script is pnpm test --recursive --if-present, which reaches js-sdk's vitest run and therefore the browser project. CONTRIBUTING.md (2 lines) and DEV.md (3 lines) say nothing about tests, so this PR description is the only record of the new step. When it fails, Playwright's message says to run pnpm exec playwright install, which fetches all three browsers rather than the chromium-only script added here. One line in packages/js-sdk/README.md or AGENTS.md would close the gap — flagging it rather than insisting, since dropping the workflow comment suggests the minimalism is deliberate.

Coordination with #1609 (test(js-sdk): run the full unit test suite in a browser, open since July): it edits the same pretest line, moving it to a pretest:browser hook behind a dedicated test:browser script and matrix leg, which keeps the self-provisioning this PR gives up. Measured with git merge-tree: #1609 already conflicts with main in three files (js_sdk_tests.yml, tests/sandbox/network.test.ts, pnpm-lock.yaml), and this PR adds a fourth in packages/js-sdk/package.json.

Nits

  • The description is stale in two spots: the workflow step no longer lands "with a comment recording why --with-deps is omitted" (removed in 818bc91 at @mishushakov's request), and #1698 was already closed at 17:02 UTC, before the request to close it was written.
  • The one failing check is JS SDK - cloudflare on tests/sandbox/network.test.ts > maskRequestHost (cat: /tmp/headers.txt: No such file or directory). That leg skips all three Playwright steps, and the earlier run on this same branch — differing only by the removed YAML comment — was fully green, so it is unrelated flake.
Open in Web View Automation 

Sent by Cursor Automation: /check SDK complies with TASTE.md

"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",

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 left a comment

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.

SDK test coverage: zero delta, and the browser project earns its keep

The diff contains no file under src/ or tests/ in either SDK, so the coverage delta is exactly zero by construction — a base/head A/B would only measure noise (repeat runs of the same commit move ±2 lines in template/buildApi.ts). Absolute numbers on 818bc91d9, E2B_API_KEY present so the live suites really ran:

package statements branches lines tests
js-sdk (unit+connectionConfig+template) 82.06% (2214/2698) 73.26% (1337/1825) 81.96% (2164/2640) 631 pass / 1 fail / 32 skip
js-sdk browser project alone 16.58% (448/2701) 10.73% (196/1825) 16.60% (439/2644) 1 pass
python-sdk 69.44% (10304/14838) 46.69% (1328/2844) 950 pass / 2 fail / 57 skip
cli 17.40% (247/1419) 21.72% (161/741) 17.05% (236/1384) 109 pass

All 3 failures are the pre-existing httpbin-template environmental ones, identical on main. python splits into 83.58% hand-written (5268/6303) and 59.00% generated (5036/8535); the cli's 17% is an artifact of its tests driving the built CLI as a subprocess, which v8 cannot instrument.

Since the percentages cannot say anything about this PR, I spent the run on the question they cannot answer: does the suite this PR reprovisions still run, and is it worth running?

It still runs, and it fails loudly when it can't

  • pretest really was live, and not for the reason it looks like: pnpm does not honour pre/post hooks by default, /workspace/.npmrc turns them on with enable-pre-post-scripts=true. Probed it on pnpm 10.33.3 — pretesttestposttest all fire. So dropping the hook is a real behaviour change, exactly as the description says.
  • pnpm run playwright:install resolves (playwright is a direct devDep) and installs Chromium plus the headless shell in 8s.
  • With Chromium present and playwright install-deps never run on this bare Ubuntu 24.04 VM, --project browser passes in 1.8s. Dropping --with-deps holds up.
  • With Chromium absent the project fails: exit 1, Executable doesn't exist at …/chromium_headless_shell-1193/…. Not a silent skip — which is the property that actually matters for coverage, because it means CI cannot lose this suite quietly.
  • CI wiring is complete. js-sdk's pnpm test has exactly one call site in .github/ (this workflow, line 110), and release.yml, release-candidate.yml and sdk_tests.yml all reach it via uses: ./.github/workflows/js_sdk_tests.yml, so they inherit the new step; cli_tests.yml's pnpm test runs in packages/cli. The if: matrix.runtime == 'node' gate is right, since test:bun/test:deno/test:cf pass explicit --project lists that exclude browser.

Its 6 unique lines badly understate its value

Measured rather than argued: the browser project executes 439 lines, but only 6 lines and 5 branch arms that the three node projects don't — utils.ts:45-46, api/metadata.ts:25, and three module-level import statements. On line coverage alone it looks deletable.

It isn't. utils.ts:45-46 is the typeof window !== 'undefined' arm of getRuntime(), and this project is the only thing in the repo that reaches it (zero hits everywhere else). Mutating runtime: 'browser''unknown' there fails the browser test with SandboxError: 2: [unknown] Failed to fetch, because connectionConfig.ts:471 then selects https://sandbox.<domain>, which CORS-blocks in a browser — the exact case that line's comment exists for. One live test is the only guard on that choice, and every other test in the repo is blind to it. Keeping it cheap to run is worth doing.

Two follow-ups inline: a latent browser-upload defect that this coverage gap is hiding, and the local prerequisite now being undocumented. Neither is a blocker for this PR.

Open in Web View Automation 

Sent by Cursor Automation: /coverage SDK Test Coverage Report

"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",

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.


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

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 testvitest 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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved — this is a straightforward CI tooling change that drops --with-deps from the Playwright install to avoid the apt mirror stalls described in the PR. One nit was flagged inline about a latent Windows Media-Foundation dependency risk on a future cache miss; it's speculative, CI-only, and doesn't affect published SDK behavior, so it doesn't block approval.

Extended reasoning...

Overview

This PR touches only .github/workflows/js_sdk_tests.yml and packages/js-sdk/package.json. It replaces the pretest hook (npx playwright install --with-deps chromium) with an explicit playwright:install script (playwright install chromium, no --with-deps) invoked as its own CI step gated on matrix.runtime == 'node'. The change is well-motivated by the PR's own timing data showing the apt-based --with-deps install stalling for ~21 minutes on a slow Ubuntu mirror, fetching only CJK/Cyrillic fonts that the single headless browser test never renders. It also moves the install out of pretest so it no longer runs on every local pnpm test invocation for contributors who don't touch the browser project.

Security risks

None. This is CI/dev-tooling only — it does not change any published SDK code path, and playwright install chromium without --with-deps is a narrower operation (fewer packages fetched) than what it replaces, not a broader one.

Level of scrutiny

Low. This is a CI workflow and devDependency script change with no effect on runtime behavior of the published e2b package. The PR author (via the linked #1698 clone) did verify the change locally: Chromium launches and drives a real browser test without the dropped packages, and format/lint/typecheck/check-deps all pass for packages/js-sdk.

Other factors

The one prior human review comment ("remove the comment") was addressed in the latest commit (818bc91), and no changeset is expected here per project convention (the original hook in #977 also shipped without one, and this is inert for consumers). The inline nit about a possible Windows Media-Foundation regression on a future Playwright-version/cache-eviction cache miss is worth having on record, but it's conditional on a future event rather than a current failure, and the fix (re-adding --with-deps or the DISM step on the Windows leg only) is cheap to apply later if it materializes.

"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",

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.

@mishushakov

Copy link
Copy Markdown
Member

@cursor check

@cursor

cursor Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Check complete — PR is ready

Review feedback is addressed and CI is green on 818bc91d9.

Review feedback

  • "remove the comment" — done in 818bc91d9. The --with-deps explanation block is gone; the Install Playwright Chromium step is unchanged.

CI status

All 29 checks passing, including:

  • JS SDK - node (ubuntu + windows): browser project passes
  • Install Playwright Chromium: 2s (ubuntu cache hit), 5s (windows)
  • Lint, typecheck, generated files, SDK Tests Status, CLA

Change summary

File Change
packages/js-sdk/package.json pretestplaywright:install (no --with-deps)
.github/workflows/js_sdk_tests.yml Explicit install step after browser cache

Non-blocking nits (no action taken)

These came from automation, not from @mishushakov:

  1. Changeset (Codex P1) — not needed; this is CI/dev-tooling only, same as Add a 'pretest' step to install chromium #977 and other ci(...) PRs.
  2. --only-shell — could shrink cache ~3×; browser project only uses headless shell.
  3. Local docsplaywright:install isn't mentioned in AGENTS.md/CONTRIBUTING.md; fresh clones need it before pnpm test.
  4. Windows Media Foundation — latent risk on a future cache miss; Linux apt stall was the real problem this PR fixes.
  5. PR description — still mentions the removed workflow comment and asks to close ci(js-sdk): install Playwright Chromium without --with-deps #1698 (already closed).

Approvals

  • @claude approved — straightforward CI tooling change; Windows MF nit is speculative.

Nothing further to implement unless you want one of the follow-ups above (e.g. --only-shell, docs line, or Windows-only --with-deps).

Open in Web Open in Cursor 

@mishushakov
mishushakov merged commit 43c28b1 into main Aug 19, 2026
46 of 48 checks passed
@mishushakov
mishushakov deleted the cursor/author-pr-claiming-e227 branch August 19, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants