fix(smoke): guard chromium.launch() at both sites (#56) #60

Merged

forgejo-admin merged 2 commits from guard-browser-launch into main

2026-08-18 15:29:38 +00:00

forgejo-admin commented

2026-08-18 08:45:11 +00:00

Owner

Closes #56.

The defect

In runSpaSmoke the module load is guarded and the smoke run is guarded. The launch() between them was bare:

chromium = await loadChromium();          // guarded → warn
const browser = await chromium.launch();  // NOT guarded
try { await runSpaSmokeCheck(...) }       // guarded → fail

Anything launch() threw propagated out of runSpaSmoke, out of lunacycleHealthCheck, and out of the hook. So no smoke_spa item was recorded, smoke_api and smoke_ws never ran, and the operator got a raw browserType.launch: Executable doesn't exist at … string in place of a check name. The same hook backs both the 15-minute monitor and deploy verification, so it took out both.

Live today for any launch failure — a missing revision, a sandbox/seccomp refusal, OOM, a stale cache after a disk sweep. Not bump-specific.

The fix

fail, not warn, per the issue's own reasoning: unlike "playwright-core is not installed at all", a browser that is present and will not start is a real regression on a host that is supposed to be able to run this.

smoke-handler.ts had the same defect in a worse form, and it is fixed too — the dynamic import('playwright-core') was unguarded as well, there is no addCheck on that path at all, and max_attempts: 1 means no retry either. Both now sit inside one guard that:

records the failure in the log with the same detail a named check item would carry (a subscriber's only outputs are its log and its job status);
falls through rather than returning, so the API and WebSocket smokes below still run — that is the part an operator would most want to still have, and in that file they are inline, so a bare throw genuinely took them with it;
throws at the end, so the job status stays honest and all smoke checks passed is not logged after a failure.

The test

Exactly the one #56 predicted, using the loadChromium seam PR #55 added — a stub that loads fine but whose launch() rejects.

It asserts the check list, not merely that the hook threw, because "it threw" is precisely what the bug already did:

expect(checkNames(logger)).toEqual(['smoke_login', 'smoke_spa', 'smoke_api', 'smoke_ws'])

Confirmed red before the fix (Rule 7.6): with the guard reverted, the playwright message escapes and the run stops at smoke_login — 18 pass, 1 fail. With the guard, 19 pass.

Gates

bun run test:hooks 66 pass · bun run typecheck:hooks exit 0 · bun run lint exit 0 (pre-existing warnings only, exit code read rather than inferred).

Deliberately NOT in this PR

Passing an explicit executablePath and moving artifacts off /tmp/lunacycle-smoke. Both need resolveBrowser() and moduleArtifactDir() from @celilo/capabilities, which are additive in celilo#897 and not yet published — so that half cannot typecheck here today. It is a clean second pass: celilo/scripts already depends on ^2.0.0, so the release picks up with no dependency change.

This half is worth landing alone regardless. It is what converts a browser failure from an escaped exception into an attributable check, which is the thing that makes the missing-browser case survivable at all — and it is independent of how the platform question settled.

🤖 Generated with Claude Code

Closes #56. ## The defect In `runSpaSmoke` the module **load** is guarded and the smoke **run** is guarded. The `launch()` between them was bare: ```ts chromium = await loadChromium(); // guarded → warn const browser = await chromium.launch(); // NOT guarded try { await runSpaSmokeCheck(...) } // guarded → fail ``` Anything `launch()` threw propagated out of `runSpaSmoke`, out of `lunacycleHealthCheck`, and out of the hook. So no `smoke_spa` item was recorded, `smoke_api` and `smoke_ws` never ran, and the operator got a raw `browserType.launch: Executable doesn't exist at …` string in place of a check name. The same hook backs both the 15-minute monitor and deploy verification, so it took out both. Live today for any launch failure — a missing revision, a sandbox/seccomp refusal, OOM, a stale cache after a disk sweep. Not bump-specific. ## The fix `fail`, not `warn`, per the issue's own reasoning: unlike "playwright-core is not installed at all", a browser that is present and will not start is a real regression on a host that is supposed to be able to run this. `smoke-handler.ts` had the same defect in a worse form, and it is fixed too — the dynamic `import('playwright-core')` was unguarded as well, there is no `addCheck` on that path at all, and `max_attempts: 1` means no retry either. Both now sit inside one guard that: - records the failure in the log with the same detail a named check item would carry (a subscriber's only outputs are its log and its job status); - **falls through rather than returning**, so the API and WebSocket smokes below still run — that is the part an operator would most want to still have, and in that file they are inline, so a bare throw genuinely took them with it; - throws at the end, so the job status stays honest and `all smoke checks passed` is not logged after a failure. ## The test Exactly the one #56 predicted, using the `loadChromium` seam PR #55 added — a stub that loads fine but whose `launch()` rejects. It asserts the check **list**, not merely that the hook threw, because "it threw" is precisely what the bug already did: ``` expect(checkNames(logger)).toEqual(['smoke_login', 'smoke_spa', 'smoke_api', 'smoke_ws']) ``` **Confirmed red before the fix** (Rule 7.6): with the guard reverted, the playwright message escapes and the run stops at `smoke_login` — 18 pass, 1 fail. With the guard, 19 pass. ## Gates `bun run test:hooks` 66 pass · `bun run typecheck:hooks` exit 0 · `bun run lint` exit 0 (pre-existing warnings only, exit code read rather than inferred). ## Deliberately NOT in this PR Passing an explicit `executablePath` and moving artifacts off `/tmp/lunacycle-smoke`. Both need `resolveBrowser()` and `moduleArtifactDir()` from `@celilo/capabilities`, which are additive in celilo#897 and **not yet published** — so that half cannot typecheck here today. It is a clean second pass: `celilo/scripts` already depends on `^2.0.0`, so the release picks up with no dependency change. This half is worth landing alone regardless. It is what converts a browser failure from an escaped exception into an attributable check, which is the thing that makes the missing-browser case survivable at all — and it is independent of how the platform question settled. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

forgejo-admin added 1 commit

2026-08-18 08:45:11 +00:00

fix(smoke): guard chromium.launch() at both sites (#56 )

pr-validate / validate (pull_request) Successful in 41s

Details

aeb346e520

In runSpaSmoke the module LOAD was guarded and the smoke RUN was guarded;
the launch() between them was bare. Anything it threw propagated out of
runSpaSmoke, out of lunacycleHealthCheck, and out of the hook — so no
smoke_spa item was recorded, smoke_api and smoke_ws never ran, and the
operator saw a raw "browserType.launch: Executable doesn't exist at …"
instead of a check name. The same hook backs the 15m monitor and deploy
verification, so it took out both.

Recorded as fail, not warn: unlike "playwright-core is not installed at
all", a browser that is present and will not start is a real regression on
a host that is supposed to be able to run this.

smoke-handler.ts had it worse — the dynamic import was unguarded too, there
is no check list on that path, and max_attempts is 1 so there is no retry.
Both now sit inside one guard that records the failure, falls through rather
than returning so the API and WS smokes still run, and throws at the end so
the job status stays honest.

The test asserts the check LIST rather than merely that the hook threw, since
"it threw" is exactly what the bug already did. Confirmed red before the fix:
without the guard the playwright message escapes and the run stops at
smoke_login.

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

forgejo-admin added 1 commit

2026-08-18 08:52:31 +00:00

docs(smoke): correct two comments that outlived their facts

pr-validate / validate (pull_request) Successful in 37s

Details

c07741bec8

Both verified against the source of truth rather than taken on report.

smoke-handler.ts's docblock illustrates the subscription wiring with
timeout_ms: 120000; manifest.yml:203 says 90000. A docblock that
paraphrases a manifest and then drifts from it is worse than no docblock,
because it reads as authoritative.

spa-smoke.ts justified `state: 'attached'` with "celilo-mgr has no fonts
installed at all (no /usr/share/fonts, no fontconfig)". That was true when
written and is measurably false now — the box has fontconfig, libfreetype6
and DejaVu, and fc-list returns 8 faces. The claim has since misled readers
into treating a host-packaging accident as the reason for the policy.

The POLICY is unchanged and now leads, because it never depended on the
font claim: presence is what this check wants, since it exists to prove the
SPA rendered data it had to fetch, and layout is the e2e suite's job. The
stale version is recorded rather than deleted so nobody restores it.

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

forgejo-admin referenced this pull request from celilo/celilo

2026-08-18 08:54:03 +00:00

docs(openspec): correct three managed-browser-runtime claims against measurement #899

forgejo-admin merged commit b6733d4fda into main

2026-08-18 15:29:38 +00:00

forgejo-admin referenced this pull request from a commit

2026-08-18 15:29:40 +00:00

Merge pull request 'fix(smoke): guard chromium.launch() at both sites (#56)' (#60) from guard-browser-launch into main

forgejo-admin referenced this pull request

2026-08-18 17:37:31 +00:00

feat(smoke): launch celilo's provisioned browser, and keep the artifacts #62