Install Playwright OS dependencies before launching browsers - #57951
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…erences Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add integration tests for this script - in a separate job |
There was a problem hiding this comment.
🟡 Changes recommended
Dependency-resolution failure currently skips required setup and permits broken browsers at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Installs Playwright browser OS dependencies before downloading browser binaries.
Changes:
- Normalizes requested browser names.
- Resolves Playwright’s bundled CLI and runs
install-deps. - Retains retry logic for browser downloads.
File summaries
| File | Description |
|---|---|
actions/setup/sh/install_playwright_browsers.sh |
Adds system dependency provisioning. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| echo "Installing Playwright system dependencies for: ${normalized_browsers[*]}" | ||
| node "$playwright_js" install-deps "${normalized_browsers[@]}" | ||
| else | ||
| echo "::warning::Could not locate playwright/cli.js next to playwright-cli; skipping system dependency install" |
There was a problem hiding this comment.
Fixed in ecca38d — the dependency install now fails closed. There is no warn-and-continue path left:
playwright-climissing on PATH →::error::+exit 1- bundled
playwrightCLI unresolvable →::error::+exit 1 install-depsitself failing →::error::+exit 1
All three exit before playwright-cli install-browser runs, so a failed dependency install can no longer leave downloaded-but-unlaunchable browsers behind. Covered by Test 2 in install_playwright_browsers_integration_test.sh, which asserts non-zero exit, the error message, and that PLAYWRIGHT_BROWSERS_PATH was never created.
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR (only actions/setup/sh/install_playwright_browsers.sh changed). Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a solid root-cause fix (installs OS-level Playwright deps via install-deps before browser install, addressing the actual missing-shared-library failure rather than retrying downloads). One gap: no regression test.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed: the fix targets missing shared libraries, not just symptom retries.
- Missing regression test: sibling scripts in
actions/setup/sh/follow a<script>_test.shpattern; this PR doesn't add one, so a future path-resolution regression could silently reintroduce the original failure (browsers download but can't launch). - Silent fallback preserved (pre-existing note by Copilot review): if
playwright_jsresolution fails, the script only emits::warning::and continues — this matches the author's stated intent to avoid hard-failing on layout differences, but is worth confirming is deliberate given the bug this PR fixes was exactly this kind of silent gap.
Positive Highlights
- ✅ Clear root-cause diagnosis and fix, well explained in the PR description with before/after.
- ✅ Reasonable defensive path resolution across two npm layout candidates.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 27.2 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| playwright_js="$candidate" | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
[/diagnosing-bugs] This bug fix (missing OS deps causing runtime browser launch failures) has no regression test guarding the new path-resolution/install-deps logic, even though sibling scripts in this dir follow a <script>_test.sh convention. @copilot please address this.
Why this matters
The root cause here was silent: browsers downloaded fine but failed to launch. Without a test, a future refactor of the npm/playwright-cli layout (or removal of this block) could silently reintroduce the exact same failure mode, and CI would not catch it until a workflow run fails at runtime.
Consider adding install_playwright_browsers_test.sh that stubs a fake playwright-cli/node_modules/playwright/cli.js layout (both package-root and one-level-up cases) and asserts install-deps is invoked with the normalized browser list, plus a case where resolution fails and the warning path is exercised.
There was a problem hiding this comment.
Added in b529a43 / ecca38d: actions/setup/sh/install_playwright_browsers_integration_test.sh, run by a dedicated sh-playwright-browsers CI job (matrix: chromium, firefox, webkit).
It guards both new code paths:
- unsupported browser names are rejected
- the resolution/
install-depspath fails closed and downloads nothing when it cannot run - system deps and browser binaries are installed on the happy path
- the installed browser actually launches and renders a page — the direct regression test for the missing shared libraries, since the download succeeds even when they are absent
Verified locally for chromium: 9/9 assertions pass.
There was a problem hiding this comment.
A small simplification is available in the new Playwright CLI path lookup logic. net: -2 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57951 · codex · mai10 · 5.76 AIC · ⌖ 0.267 AIC · ⊞ 12.8K
Comment /ponytail to run again
| # node_modules/. Depending on npm layout, the resolved playwright-cli script | ||
| # may live at the package root (sibling of node_modules) or one level deeper | ||
| # in a bin/ subdirectory, so check both candidate locations. | ||
| search_dir="$(dirname "$playwright_cli_real")" |
There was a problem hiding this comment.
L33-40: shrink: manual two-location search for the bundled Playwright CLI. Resolve the sibling playwright/cli.js directly from the resolved playwright-cli path instead of checking both candidate directories.
There was a problem hiding this comment.
Done in ecca38d. The two-candidate directory loop is gone; the bundled CLI is now resolved in one step by letting Node do the node_modules walk-up from the resolved playwright-cli path and reading the entry point from the playwright package's own bin field. The same simplification was applied to the launch step in the integration test.
…e CI job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Request changes
The new dependency-install path still has a fail-open branch, so this does not reliably fix the runner setup.
Blocking theme
If playwright/cli.js is not found in one of the two guessed relative locations, the script emits a warning and keeps going. That recreates the exact broken behavior this PR is trying to remove: browser downloads succeed, but runtime still fails later with missing system libraries. Setup needs to fail closed when the dependency installer cannot be resolved.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 14.5 AIC · ⌖ 7.38 AIC · ⊞ 23.5K
Comment /review to run again
| echo "Installing Playwright system dependencies for: ${normalized_browsers[*]}" | ||
| node "$playwright_js" install-deps "${normalized_browsers[@]}" | ||
| else | ||
| echo "::warning::Could not locate playwright/cli.js next to playwright-cli; skipping system dependency install" |
There was a problem hiding this comment.
This fallback makes the setup step non-deterministic again: if playwright/cli.js is not found in one of those two guessed paths, the script only emits a warning and continues downloading browsers that still cannot start, so the original runtime failure survives under a slightly different npm layout.
💡 The dependency-install step needs to fail closed, not silently degrade.
install-deps is the entire fix here. Treating lookup failure as a warning means any packaging drift in @playwright/cli puts us right back in the broken state this PR is meant to eliminate, but without failing setup loudly.
A safer pattern is to abort when the bundled Playwright CLI cannot be resolved, or to resolve it from a source that is guaranteed by the install step instead of hard-coding two relative paths. For example:
if [ -z "$playwright_js" ]; then
echo "::error::Could not locate playwright/cli.js required for Playwright system dependency installation"
exit 1
fi
node "$playwright_js" install-deps "${normalized_browsers[@]}"That keeps the workflow honest: either dependencies were installed, or setup fails before later jobs hit missing shared-library errors.
There was a problem hiding this comment.
Fixed in ecca38d, along the lines you suggested. The lookup failure is now fatal rather than a warning, and the resolution no longer hard-codes relative paths:
playwright_js="$(node -e '
const path = require("path");
const pkgPath = require.resolve("playwright/package.json", { paths: [process.argv[1]] });
const pkg = require(pkgPath);
const bin = typeof pkg.bin === "string" ? pkg.bin : pkg.bin && pkg.bin.playwright;
if (!bin) { process.exit(1); }
process.stdout.write(path.resolve(path.dirname(pkgPath), bin));
' "$(dirname "$playwright_cli_real")" 2>/dev/null || true)"
if [ ! -f "$playwright_js" ]; then
echo "::error::Could not resolve the playwright CLI bundled with ${playwright_cli_real}; Playwright system dependencies cannot be installed"
exit 1
fiNode performs the node_modules walk-up itself and the entry point comes from the package's own bin field, so npm layout drift is tolerated — and if resolution still fails, setup aborts before any browser is downloaded. install-deps returning non-zero is now also fatal.
Note require.resolve("playwright/cli.js", ...) cannot be used directly: cli.js is not listed in the playwright package's exports, so resolving package.json and reading bin is the supported route.
There was a problem hiding this comment.
Reviewed via Impeccable-informed lens (harden/audit — this is a bash setup script, not a UI change, so most Impeccable modes don't apply directly).
Outstanding blocking issue (already flagged, not duplicating inline): the fallback path when playwright_js can't be resolved only prints ::warning:: and then proceeds to install browsers anyway (see existing comment on line 47). This silently reintroduces the exact failure mode this PR fixes — browsers get downloaded but can't launch due to missing OS deps — with no clear signal at runtime beyond a warning easy to miss in CI logs. Recommend failing hard (exit 1) in that branch, or at minimum surfacing it as a workflow-level error/annotation.
No other correctness or security issues found in the diff; the rest of the path-resolution and browser install logic looks sound.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 19.5 AIC · ⌖ 8.08 AIC · ⊞ 8.3K
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in b529a43: The test runs the real install path and asserts:
Verified locally for chromium (7/7 assertions pass). |
|
@copilot Please review the current PR state, address the latest review feedback, refresh the branch if needed, and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…uditor-tool Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Reconciled. Summary of this pass:
Remaining blockers (human-only):
Notes: I could not resolve the review threads (no GraphQL token in this environment), so they remain open despite being addressed. The |
|
@copilot Please review the current PR state, verify whether the addressed review feedback can now be resolved, refresh the branch if needed, and run the
|
Ran
Correction to my previous summary: I flagged the Remaining blockers — both human-only:
One caveat worth stating plainly: I still cannot resolve the review threads — no GraphQL token is available in this environment — so they will stay open on the PR despite being addressed. That is a tooling limit, not an outstanding code concern. |
|
@copilot Please verify the current PR state, confirm the addressed Playwright setup review feedback is fully closed on the latest head, and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Ran No code changes this pass. The branch was refreshed again by an earlier run (merge
Remaining blockers, both human-only:
One caveat, unchanged: no GraphQL token is available in this environment, so I cannot mark the four threads resolved — they will stay visibly open despite being addressed and outdated. That is a tooling limitation on my end, not an outstanding code concern; the underlying feedback is closed. |
|
🎉 This pull request is included in a new release. Release: |
Blog Auditor (and any workflow using
tools.playwright) failed at runtime because all three bundled browser engines (Chromium, Firefox, WebKit) were non-functional — their shared library dependencies (libnspr4,libnss3,libatk-bridge2.0-0, etc.) were never installed on the runner.Root cause
install_playwright_browsers.shdownloaded browser binaries viaplaywright-cli install-browserbut never provisioned the OS-level packages those binaries need to actually launch.Fix
actions/setup/sh/install_playwright_browsers.sh: before installing each requested browser, resolve theplaywrightpackage bundled alongside the globally-installed@playwright/cliand invoke itsinstall-deps <browsers>command, which installs missing apt packages viasudo(matching other setup scripts that already do system package installs this way).playwright-cli's bin is symlinked into the global npm bin dir (notplaywright's own CLI), the script resolves the real path ofplaywright-cliand locatesnode_modules/playwright/cli.jsrelative to it, checking both the package-root and one-level-up locations to tolerate npm layout differences.Run: https://github.com/github/gh-aw/actions/runs/33641605189
Run reference: https://github.com/github/gh-aw/actions/runs/33648331948
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.