Skip to content

fix(setup): pin Playwright browser install to bundled version so headed browse connect works (#1829)#1838

Open
jbetala7 wants to merge 1 commit into
garrytan:mainfrom
jbetala7:oss/fix-1829-playwright-pin
Open

fix(setup): pin Playwright browser install to bundled version so headed browse connect works (#1829)#1838
jbetala7 wants to merge 1 commit into
garrytan:mainfrom
jbetala7:oss/fix-1829-playwright-pin

Conversation

@jbetala7

@jbetala7 jbetala7 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #1829

Problem

On a clean install/upgrade, headed browse connect fails with a missing full Chromium build:

Executable doesn't exist at .../ms-playwright/chromium-1208/.../Google Chrome for Testing
    at async launchHeaded (browse/src/browser-manager.ts)

even though ./setup reported success and headless browse works fine.

Root cause (two issues in setup)

  1. Install/runtime version drift. The browse binary is compiled (setup ~line 390) against the lockfile-pinned Playwright (playwright@1.58.2 → Chromium revision 1208). But the browser-install step ran bunx playwright install chromium, which resolves the latest Playwright at runtime (currently 1.60.0 → revision 1223) and downloads a different full build than the binary expects. The cache ends up with chromium-1223 (wrong) and chromium_headless_shell-1208 but not chromium-1208 (full) — so headed launch fails.
  2. Headless-only verify masks it. ensure_playwright_browser() only ran a default chromium.launch() (headless), which passes off the cached chromium_headless_shell even when the full Chrome-for-Testing build is absent. So setup reports success.

Fix

  • Install browsers with the Playwright version bundled in node_modules (read from playwright/package.json) instead of bunx-latest, so the installed Chromium revision always matches the compiled binary and the two can never drift. Falls back to the old behavior only if the version can't be resolved.
  • Harden ensure_playwright_browser() to assert chromium.executablePath() exists before treating the browser as ready (both the Node/Windows and Bun/Unix branches), so a missing full build fails setup instead of being silently masked by the cached headless shell.

This matches the maintainer-suggested direction in the issue (pin the install to the bundled version; make verify catch a missing full build).

Testing

  • bun test test/setup-playwright-pin.test.ts — new static-source regression test (no browser download), 3 pass.
  • bun test test/setup-windows-fallback.test.ts test/setup-emoji-font.test.ts — existing setup tests still green (20 pass).
  • bash -n setup — syntax OK.
  • Verified locally that the bundled version resolves to 1.58.2 and chromium.executablePath() returns the chromium-1208 full-build path the issue reports as missing.

Relationship to other open Playwright PRs (not duplicates)

…rrytan#1829)

Headed `browse connect` fails with a missing full Chromium build (e.g.
`chromium-1208/.../Google Chrome for Testing`) after a "successful" ./setup,
while headless browse works fine. Two root causes in setup:

1. The browse binary is compiled against the lockfile-pinned Playwright
   (1.58.2 -> Chromium 1208), but `bunx playwright install chromium` resolves
   the *latest* Playwright at runtime (1.60.0 -> Chromium 1223) and downloads a
   different full build than the binary expects.
2. `ensure_playwright_browser()` only does a headless `chromium.launch()`, which
   passes off the cached headless shell even when the full Chrome-for-Testing
   build that headed mode needs is missing — so the mismatch is never caught.

Fix: install browsers with the Playwright version bundled in node_modules (so
install and runtime can never drift), and assert `chromium.executablePath()`
exists before treating the browser as ready so a missing full build fails setup
instead of being masked.

Adds test/setup-playwright-pin.test.ts (static source assertions, no browser
download) guarding both invariants.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Jun 2, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

DavidMiserak added a commit to DavidMiserak/gstack that referenced this pull request Jun 8, 2026
… exit 1

Replaces both `exit 1` paths in the Playwright install/verify block with a
$_PW_FAIL_REASON accumulator that prints a named warning to stderr and
returns control. Previously a Chromium download failure (network blip,
locked-down corporate machine, missing Node.js on Windows) would abort
./setup mid-run with no skills registered, no hooks installed, no
migrations applied — and a re-run would start over from the top.

Now the failure only disables browser-driven features (/qa,
/design-review, make-pdf, sidebar, /pair-agent); skills, hooks, and
migrations land normally. The warning names which sub-step failed
(chromium-install / windows-no-node / windows-node-modules /
post-install-launch) so users and bug reports can be specific, and
tells the user to re-run ./setup to retry just this step.

Tests (5) exercise the actual block from setup with stubbed
ensure_playwright_browser and bunx, simulate install + post-install
failures, and assert the script exits 0 with the named warning. One
side-by-side test inlines the OLD bug shape and confirms it returned
non-zero, so the exit-code difference is proven, not just asserted.

Coordinates with garrytan#1838 (pins Playwright install version): both touch the
same code block, so whichever lands first needs the other to rebase. The
changes are complementary — garrytan#1838 fixes which Chromium build gets
installed; this PR fixes what happens when the install fails.

Carved out from garrytan#1883 per @jbetala7's review request.
DavidMiserak added a commit to DavidMiserak/gstack that referenced this pull request Jun 8, 2026
…ability

setup used to interleave trivial filesystem ops, expensive compute,
settings.json mutations, and network/sudo steps in whatever order the
script grew. A Playwright Chromium download failing at step 2 would
abort before skills got registered at step 4, hooks landed at step 10,
or migrations ran at step 8.

This commit groups work by where failures actually come from:

  Phase A — filesystem (always succeeds): ~/.gstack/, welcome, GBrain detection
  Phase B — local compute (low fail): bun build, skill linking, migrations, regen
  Phase C — settings.json mutation (medium fail, reversible): team-mode + plan-tune hooks
  Phase D — external network/sudo/package managers (highest fail): coreutils, font, Playwright

Each phase is bracketed with a header comment; steps within are
renumbered (A1, A2, B1, B2a...).

The biggest behavior shift is the GBrain detect/regen split that the
new order requires: detection moves to A3 (filesystem scan only, no
dependencies), regen moves to B6 (runs after the binary is built and
skill docs are generated). Detection persists to
~/.gstack/gbrain-detection.json so B6 can read it.

A safety guard for that split: when gstack-gbrain-detect exits non-zero,
A3 removes both the temp file AND any pre-existing detection file from a
prior successful run. Without the guard, a stale "ok" detection file
would survive a failed detect and silently apply ~250 tokens of overhead
per planning skill to a user whose gbrain may no longer be installed.

Behavior preservation:
- bun_cmd routing in link_{codex,factory,opencode}_skill_dirs is
  unchanged (still buggy here). The fix is in garrytan#1898.
- gen:skill-docs:user still pipes through `tail -3` in B6 (still buggy
  here). The fix is in garrytan#1898.
- Playwright Chromium install/verify in D3 still exits 1 on failure
  (unchanged). The warn-don't-exit conversion is in garrytan#1900 and
  coordinates with garrytan#1838.

Splitting those keeps this PR reviewable as "a move plus the gbrain
split that the move requires," per @jbetala7's request on garrytan#1883.
DavidMiserak added a commit to DavidMiserak/gstack that referenced this pull request Jun 8, 2026
… exit 1

Replaces both `exit 1` paths in the Playwright install/verify block with a
$_PW_FAIL_REASON accumulator that prints a named warning to stderr and
returns control. Previously a Chromium download failure (network blip,
locked-down corporate machine, missing Node.js on Windows) would abort
./setup mid-run with no skills registered, no hooks installed, no
migrations applied — and a re-run would start over from the top.

Now the failure only disables browser-driven features (/qa,
/design-review, make-pdf, sidebar, /pair-agent); skills, hooks, and
migrations land normally. The warning names which sub-step failed
(chromium-install / windows-no-node / windows-node-modules /
post-install-launch) so users and bug reports can be specific, and
tells the user to re-run ./setup to retry just this step.

Tests (5) exercise the actual block from setup with stubbed
ensure_playwright_browser and bunx, simulate install + post-install
failures, and assert the script exits 0 with the named warning. One
side-by-side test inlines the OLD bug shape and confirms it returned
non-zero, so the exit-code difference is proven, not just asserted.

Coordinates with garrytan#1838 (pins Playwright install version): both touch the
same code block, so whichever lands first needs the other to rebase. The
changes are complementary — garrytan#1838 fixes which Chromium build gets
installed; this PR fixes what happens when the install fails.

Carved out from garrytan#1883 per @jbetala7's review request.
DavidMiserak added a commit to DavidMiserak/gstack that referenced this pull request Jun 8, 2026
… exit 1

Replaces both `exit 1` paths in the Playwright install/verify block with a
$_PW_FAIL_REASON accumulator that prints a named warning to stderr and
returns control. Previously a Chromium download failure (network blip,
locked-down corporate machine, missing Node.js on Windows) would abort
./setup mid-run with no skills registered, no hooks installed, no
migrations applied — and a re-run would start over from the top.

Now the failure only disables browser-driven features (/qa,
/design-review, make-pdf, sidebar, /pair-agent); skills, hooks, and
migrations land normally. The warning names which sub-step failed
(chromium-install / windows-no-node / windows-node-modules /
post-install-launch) so users and bug reports can be specific, and
tells the user to re-run ./setup to retry just this step.

Tests (5) exercise the actual block from setup with stubbed
ensure_playwright_browser and bunx, simulate install + post-install
failures, and assert the script exits 0 with the named warning. One
side-by-side test inlines the OLD bug shape and confirms it returned
non-zero, so the exit-code difference is proven, not just asserted.

Coordinates with garrytan#1838 (pins Playwright install version): both touch the
same code block, so whichever lands first needs the other to rebase. The
changes are complementary — garrytan#1838 fixes which Chromium build gets
installed; this PR fixes what happens when the install fails.

Carved out from garrytan#1883 per @jbetala7's review request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

browse connect (headed) fails: setup installs wrong Chromium revision vs compiled binary's pinned Playwright

1 participant