Skip to content

Commit 0a6c5bf

Browse files
committed
fix(setup): Playwright Chromium failure becomes best-effort warn, not 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.
1 parent d8c91c6 commit 0a6c5bf

3 files changed

Lines changed: 228 additions & 26 deletions

File tree

CHANGELOG.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,90 @@ behavior. Nothing about how you invoke these skills changed.
179179
- Redaction, taxonomy, and parity content tests now read the skeleton+sections union so relocated prose still counts toward coverage.
180180
- Real-session section-read canary deferred to TODOS (the deterministic guards ship first).
181181

182+
## [1.56.2.0] - 2026-06-07
183+
184+
## **A failed Playwright Chromium install no longer aborts `./setup` mid-run. Skills, hooks, and migrations are already in place; you get a named warning and a retry hint.**
185+
186+
The Playwright Chromium install is the riskiest step in `./setup`. It needs
187+
network, it talks to a Microsoft CDN, on Windows it bounces through Node.js
188+
because of a Bun pipe bug, and on locked-down machines it may not even be
189+
allowed to run. Until now, any failure in that step was fatal: `exit 1`, and
190+
the run dies before skills get registered, before hooks land, before
191+
migrations run. Re-running `./setup` would start over from the top.
192+
193+
That's the wrong shape. The user already has a working bun, a built browse
194+
binary, and a tree of skill templates ready to go. The thing that failed —
195+
Chromium availability — only affects browser-driven features (`/qa`,
196+
`/design-review`, `make-pdf`, sidebar, `/pair-agent`). Everything else can
197+
still work. So make the failure non-fatal.
198+
199+
This release replaces both `exit 1` paths in the Playwright install/verify
200+
block with a `$_PW_FAIL_REASON` accumulator. On install failure the script
201+
prints a named warning telling you which sub-step failed, names the
202+
affected features, gives Windows users the specific known-issue link, and
203+
tells you to re-run `./setup` to retry — at which point only the Playwright
204+
step needs to succeed, because everything else is already done.
205+
206+
### The numbers that matter
207+
208+
Source: `bun test test/setup-playwright-warn-not-exit.test.ts` on this branch.
209+
210+
| Metric | Before | After | Δ |
211+
|--------|--------|-------|---|
212+
| Playwright failure aborts `./setup` | yes | no | warn instead of exit |
213+
| Skills registered when Chromium download fails | sometimes | always | depends on which step failed first |
214+
| Named failure reason in warning | no | yes | `chromium-install`, `windows-no-node`, `windows-node-modules`, `post-install-launch` |
215+
| Retry guidance in failure output | no | yes | "Re-run ./setup to retry" |
216+
217+
The four named reasons matter. When a Playwright install fails it's usually
218+
not obvious whether the download died, Chromium was downloaded but won't
219+
launch, or Node.js isn't on PATH (Windows). The warning calls it by name
220+
so users and bug reports can be specific.
221+
222+
### What this means for you
223+
224+
If your network blinks during `./setup`, or you're on a corporate machine
225+
that won't let `bunx playwright install chromium` finish, you'll now get
226+
gstack installed with everything except browser features, plus a clear
227+
warning. Re-run `./setup` when the network is back to enable browser
228+
features. If you're on Windows and hit the Bun-on-Windows pipe bug, the
229+
warning tells you exactly which `node -e` check to verify.
230+
231+
### Itemized changes
232+
233+
#### Changed
234+
235+
- `setup`: Playwright Chromium install/verify no longer aborts on failure.
236+
The two `exit 1` paths in the install block are replaced with a
237+
`$_PW_FAIL_REASON` accumulator (values: `chromium-install`,
238+
`windows-no-node`, `windows-node-modules`, `post-install-launch`). When
239+
set, the script prints a named warning to stderr, lists the affected
240+
browser-driven features, includes the Bun-on-Windows known-issue link
241+
if applicable, and tells the user to re-run `./setup` to retry. All
242+
other setup steps complete normally.
243+
244+
#### For contributors
245+
246+
- `test/setup-playwright-warn-not-exit.test.ts` — 5 tests:
247+
- Two source-anchored checks confirm no `exit 1` is reachable from the
248+
Playwright block and the `_PW_FAIL_REASON` accumulator pattern is in
249+
place.
250+
- Two functional tests execute the live block from `setup` with stubbed
251+
`ensure_playwright_browser` and `bunx`, simulate install + post-install
252+
failures, and assert the script exits 0 with the named warning.
253+
- One side-by-side test inlines the OLD `exit 1` bug shape and confirms
254+
it returned non-zero — proves the exit-code difference is real, not a
255+
quoting illusion.
256+
257+
#### Coordination
258+
259+
- This PR is one of three carved out from #1883 per @jbetala7's review
260+
request. It touches the same code block as #1838 (which pins the
261+
Playwright install version), so whichever of #1838 or this PR lands
262+
first will need a small rebase on the other. The two changes are
263+
complementary — #1838 fixes which Chromium build gets installed; this
264+
PR fixes what happens when the install fails.
265+
182266
## [1.56.1.0] - 2026-06-03
183267

184268
## **`/sync-gbrain` can no longer delete your repo. Cleanup now refuses any directory it cannot prove it created.**

setup

Lines changed: 39 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -475,44 +475,57 @@ if [ "$INSTALL_OPENCODE" -eq 1 ] && [ "$NEEDS_BUILD" -eq 0 ]; then
475475
)
476476
fi
477477

478-
# 2. Ensure Playwright's Chromium is available
478+
# 2. Ensure Playwright's Chromium is available (best-effort: warn on failure).
479+
#
480+
# When Chromium download or launch fails (network issue, missing sudo,
481+
# offline mirror, Bun-on-Windows pipe bug), browser-driven features
482+
# (/qa, /design-review, make-pdf, sidebar, /pair-agent) are unavailable
483+
# until the user retries. All other gstack functionality (skill
484+
# registration, hooks, migrations) is already installed by earlier steps.
485+
# Previously this step was fatal (exit 1) and would abort the whole run
486+
# mid-setup, leaving skills unregistered. Now it sets $_PW_FAIL_REASON,
487+
# prints a named warning, and returns control. Re-running ./setup retries.
488+
_PW_FAIL_REASON=""
489+
479490
if ! ensure_playwright_browser; then
480491
echo "Installing Playwright Chromium..."
481-
(
482-
cd "$SOURCE_GSTACK_DIR"
483-
bunx playwright install chromium
484-
)
492+
if ! ( cd "$SOURCE_GSTACK_DIR" && bunx playwright install chromium ); then
493+
_PW_FAIL_REASON="chromium-install"
494+
fi
485495

486-
if [ "$IS_WINDOWS" -eq 1 ]; then
496+
if [ -z "$_PW_FAIL_REASON" ] && [ "$IS_WINDOWS" -eq 1 ]; then
487497
# On Windows, Node.js launches Chromium (not Bun — see oven-sh/bun#4253).
488498
# Ensure playwright is importable by Node from the gstack directory.
489499
if ! command -v node >/dev/null 2>&1; then
490-
echo "gstack setup failed: Node.js is required on Windows (Bun cannot launch Chromium due to a pipe bug)" >&2
491-
echo " Install Node.js: https://nodejs.org/" >&2
492-
exit 1
500+
_PW_FAIL_REASON="windows-no-node"
501+
else
502+
echo "Windows detected — verifying Node.js can load Playwright..."
503+
if ! ( cd "$SOURCE_GSTACK_DIR" && \
504+
# Bun's node_modules already has playwright; verify Node can require it
505+
{ node -e "require('playwright')" 2>/dev/null || npm install --no-save playwright; } && \
506+
# @ngrok/ngrok is externalized in server-node.mjs and resolved at runtime.
507+
# Verify the platform-specific native binary is installed so /pair-agent
508+
# tunnels don't fail later with a cryptic module-not-found error.
509+
{ node -e "require('@ngrok/ngrok')" 2>/dev/null || npm install --no-save @ngrok/ngrok; } ); then
510+
_PW_FAIL_REASON="windows-node-modules"
511+
fi
493512
fi
494-
echo "Windows detected — verifying Node.js can load Playwright..."
495-
(
496-
cd "$SOURCE_GSTACK_DIR"
497-
# Bun's node_modules already has playwright; verify Node can require it
498-
node -e "require('playwright')" 2>/dev/null || npm install --no-save playwright
499-
# @ngrok/ngrok is externalized in server-node.mjs and resolved at runtime.
500-
# Verify the platform-specific native binary is installed so /pair-agent
501-
# tunnels don't fail later with a cryptic module-not-found error.
502-
node -e "require('@ngrok/ngrok')" 2>/dev/null || npm install --no-save @ngrok/ngrok
503-
)
504513
fi
505514
fi
506515

507-
if ! ensure_playwright_browser; then
516+
if [ -z "$_PW_FAIL_REASON" ] && ! ensure_playwright_browser; then
517+
_PW_FAIL_REASON="post-install-launch"
518+
fi
519+
520+
if [ -n "$_PW_FAIL_REASON" ]; then
521+
echo "" >&2
522+
echo " warning: Playwright Chromium is unavailable ($_PW_FAIL_REASON)." >&2
523+
echo " Browser features (/qa, /design-review, make-pdf, sidebar, /pair-agent) are unavailable." >&2
508524
if [ "$IS_WINDOWS" -eq 1 ]; then
509-
echo "gstack setup failed: Playwright Chromium could not be launched via Node.js" >&2
510-
echo " This is a known issue with Bun on Windows (oven-sh/bun#4253)." >&2
511-
echo " Ensure Node.js is installed and 'node -e \"require('playwright')\"' works." >&2
512-
else
513-
echo "gstack setup failed: Playwright Chromium could not be launched" >&2
525+
echo " Known Bun-on-Windows issue (oven-sh/bun#4253). Ensure Node.js is installed," >&2
526+
echo " then verify: node -e \"require('playwright')\"" >&2
514527
fi
515-
exit 1
528+
echo " Re-run ./setup to retry. Skills, hooks, and migrations are already installed." >&2
516529
fi
517530

518531
# 2b. Ensure a color-emoji font is installed so make-pdf emoji render (Linux).
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import { describe, test, expect } from 'bun:test';
2+
import { spawnSync } from 'child_process';
3+
import * as fs from 'fs';
4+
import * as path from 'path';
5+
6+
const ROOT = path.resolve(import.meta.dir, '..');
7+
const SETUP_SRC = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8');
8+
9+
function runBash(script: string): { stdout: string; stderr: string; status: number } {
10+
const r = spawnSync('bash', ['-c', script], { encoding: 'utf-8' });
11+
return { stdout: r.stdout || '', stderr: r.stderr || '', status: r.status ?? -1 };
12+
}
13+
14+
// Extract the live Playwright install block from setup. Source-anchored so
15+
// the test is resilient to line-number drift.
16+
function extractPlaywrightBlock(): string {
17+
const start = SETUP_SRC.indexOf("# 2. Ensure Playwright's Chromium");
18+
expect(start).toBeGreaterThan(-1);
19+
// Block ends just before the next top-level section comment.
20+
const end = SETUP_SRC.indexOf('\n# 2b.', start);
21+
expect(end).toBeGreaterThan(start);
22+
return SETUP_SRC.slice(start, end);
23+
}
24+
25+
describe('setup: Playwright failure is best-effort warn, not fatal exit', () => {
26+
const block = extractPlaywrightBlock();
27+
28+
test('no `exit 1` is reachable from the Playwright install/verify path', () => {
29+
// The bug shape used `exit 1` twice. The fix replaces both with a
30+
// $_PW_FAIL_REASON accumulator that prints a named warning. Either an
31+
// explicit `exit 1` OR `exit 1` (any spaces) inside the block is the bug.
32+
expect(block).not.toMatch(/^\s*exit\s+1\b/m);
33+
});
34+
35+
test('uses the $_PW_FAIL_REASON accumulator + named warning', () => {
36+
expect(block).toContain('_PW_FAIL_REASON=""');
37+
expect(block).toContain('_PW_FAIL_REASON="chromium-install"');
38+
expect(block).toContain('_PW_FAIL_REASON="post-install-launch"');
39+
expect(block).toMatch(/warning: Playwright Chromium is unavailable/);
40+
// Tell users this is recoverable, not a hard fail.
41+
expect(block).toMatch(/Re-run \.\/setup to retry/);
42+
});
43+
44+
test('functional: install failure leaves the script with exit 0 and prints the warning', () => {
45+
// Run a stripped harness that mimics the live block:
46+
// - ensure_playwright_browser stubbed to always fail
47+
// - bunx stubbed to fail (simulates Chromium download failure)
48+
// Assert: exit 0, warning printed, NOT an exit-1 path.
49+
const r = runBash(`
50+
set +e
51+
IS_WINDOWS=0
52+
SOURCE_GSTACK_DIR=/tmp
53+
ensure_playwright_browser() { return 1; } # browser missing
54+
bunx() { return 1; } # install fails
55+
56+
${block}
57+
58+
RC=$?
59+
echo "FINAL_RC=$RC"
60+
`);
61+
62+
// Script must NOT exit non-zero (the bug shape would).
63+
expect(r.stdout).toContain('FINAL_RC=0');
64+
// Named warning fires with the expected reason.
65+
expect(r.stderr).toContain('warning: Playwright Chromium is unavailable (chromium-install)');
66+
// User-actionable retry hint.
67+
expect(r.stderr).toContain('Re-run ./setup to retry');
68+
});
69+
70+
test('functional: bug shape (with `exit 1`) would have exited non-zero', () => {
71+
// Inline the BUGGY shape — confirms the difference is real, not an
72+
// illusion of bash quoting. This is the form we removed.
73+
const buggy = `
74+
ensure_playwright_browser() { return 1; }
75+
bunx() { return 1; }
76+
if ! ensure_playwright_browser; then
77+
( cd /tmp && bunx playwright install chromium )
78+
fi
79+
if ! ensure_playwright_browser; then
80+
echo "gstack setup failed: Playwright Chromium could not be launched" >&2
81+
exit 1
82+
fi
83+
`;
84+
const r = runBash(`set +e; ( ${buggy} ); echo "FINAL_RC=$?"`);
85+
expect(r.stdout).toContain('FINAL_RC=1');
86+
});
87+
88+
test('functional: post-install verify failure also degrades to warn', () => {
89+
// Simulate the install succeeding but the post-install launch still failing.
90+
// The fixed shape sets _PW_FAIL_REASON="post-install-launch" and warns.
91+
const r = runBash(`
92+
set +e
93+
IS_WINDOWS=0
94+
SOURCE_GSTACK_DIR=/tmp
95+
ensure_playwright_browser() { return 1; }
96+
bunx() { return 0; } # install succeeds
97+
98+
${block}
99+
100+
echo "FINAL_RC=$?"
101+
`);
102+
expect(r.stdout).toContain('FINAL_RC=0');
103+
expect(r.stderr).toContain('warning: Playwright Chromium is unavailable (post-install-launch)');
104+
});
105+
});

0 commit comments

Comments
 (0)