diff --git a/src/auth/oauthServer.ts b/src/auth/oauthServer.ts index 61f07def..37123e89 100644 --- a/src/auth/oauthServer.ts +++ b/src/auth/oauthServer.ts @@ -113,8 +113,11 @@ export class OAuthServer { // popup takes both away. Going through the helper also puts this call // behind CAPY_WEB_NO_OPEN for the first time; before, a suite or CI run // that reached authentication opened the developer's own browser. - void openScreen(authUrl, { kind: 'handoff' }).then((plan) => { - if (plan.via !== 'suppressed') console.log(`✓ Opened browser for authentication`); + // Only claim the browser opened once it actually did. When it could not + // (no `xdg-open` in a VM or container), say nothing more: the link is + // already on screen above, and the callback server keeps waiting for it. + void openScreen(authUrl, { kind: 'handoff' }).then((result) => { + if (result.opened) console.log(`✓ Opened browser for authentication`); }); const timeout = setTimeout(() => { diff --git a/src/ui/openScreen.ts b/src/ui/openScreen.ts index a4d15fba..e8089b10 100644 --- a/src/ui/openScreen.ts +++ b/src/ui/openScreen.ts @@ -340,29 +340,91 @@ export function planOpen(url: string, opts: PlanOptions): OpenPlan { * caller before this runs, so a browser that will not start is an inconvenience * and not a failure of the flow. */ -export async function openScreen(url: string, opts: PlanOptions): Promise { +export async function openScreen( + url: string, + opts: PlanOptions, + deps: OpenDeps = {}, +): Promise { const plan = planOpen(url, opts); + const opened = await carryOut(plan, url, deps); + return { ...plan, opened }; +} + +/** + * A started process, as far as this module needs to know: Node's ChildProcess + * fits. `once` is optional because a stand-in (a test double, or a launcher + * that hands back nothing useful) may not have it; that counts as started. + */ +export interface Launchable { + readonly pid?: number; + once?(event: 'error', listener: (err: Error) => void): unknown; + unref?(): unknown; +} + +/** Injection seams, so tests can stand in for a missing launcher without starting one. */ +export interface OpenDeps { + readonly spawnApp?: (exec: string, args: readonly string[]) => Launchable; + readonly openDefault?: (url: string) => Promise; +} + +/** + * True when the process started, false when it could not be started (ENOENT: + * no `xdg-open` in a VM or container, no Chrome binary). + * + * Attaching the `error` listener is the load-bearing part: Node reports a + * failed spawn as an `error` event on the next tick, and an `error` event with + * no listener throws after every surrounding try/catch has returned, taking the + * whole CLI down. The listener stays attached, so a later error is harmless too. + * + * Whether it started is read from `pid`, which Node leaves undefined when the + * spawn failed — known synchronously, with no wait on an event. A stand-in with + * no `pid` field at all counts as started. + */ +export function launched(child: Launchable): boolean { + child?.once?.('error', () => { + /* reported through the return value; see above */ + }); + if (!child || !('pid' in child)) return true; + return typeof child.pid === 'number'; +} + +async function carryOut(plan: OpenPlan, url: string, deps: OpenDeps): Promise { + if (plan.via === 'suppressed') return false; + if (plan.via === 'default-browser') return fallback(url, deps); + // Detached: the window outlives the CLI process, which is the whole point + // for an ending page served just before exit. + const started = await tryLaunch(() => + (deps.spawnApp ?? spawnDetached)(plan.exec, plan.args), + ); + return started ? true : fallback(url, deps); +} + +function spawnDetached(exec: string, args: readonly string[]): Launchable { + return spawn(exec, [...args], { detached: true, stdio: 'ignore' }); +} + +async function tryLaunch(start: () => Launchable | Promise): Promise { try { - if (plan.via === 'app-window') { - // Detached: the window outlives the CLI process, which is the whole point - // for an ending page served just before exit. - const child = spawn(plan.exec, plan.args, { detached: true, stdio: 'ignore' }); - child.on('error', () => void fallback(url)); - child.unref(); - } else if (plan.via === 'default-browser') { - await fallback(url); - } + // No `await` between a synchronous spawn and `launched`: the failure + // arrives on the next tick, and an await here could let it land before the + // listener does. An async starter (the `open` package) returns its child + // in the same microtask drain, which still runs ahead of that tick. + const started = start(); + const child = started instanceof Promise ? await started : started; + const ok = launched(child); + if (ok) child.unref?.(); + return ok; } catch { /* best-effort; the printed URL is the real fallback */ + return false; } - return plan; } -async function fallback(url: string): Promise { - try { - const open = (await import('open')).default; - await open(url); - } catch { - /* best-effort */ - } +async function defaultOpen(url: string): Promise { + const open = (await import('open')).default; + return open(url); +} + +function fallback(url: string, deps: OpenDeps): Promise { + return tryLaunch(() => (deps.openDefault ?? defaultOpen)(url)); } diff --git a/tests/ui/openScreen.test.ts b/tests/ui/openScreen.test.ts index 47839af7..dfa82dd0 100644 --- a/tests/ui/openScreen.test.ts +++ b/tests/ui/openScreen.test.ts @@ -9,7 +9,9 @@ * thin part that carries the plan out. */ import { describe, test, expect } from 'bun:test'; -import { planOpen, type DefaultBrowser } from '../../src/ui/openScreen'; +import { spawn } from 'node:child_process'; +import { EventEmitter } from 'node:events'; +import { planOpen, openScreen, launched, type DefaultBrowser, type Launchable } from '../../src/ui/openScreen'; const CHROME: DefaultBrowser = { family: 'chromium', @@ -92,3 +94,75 @@ describe('which window a screen opens in', () => { expect(plan.args.filter((a) => a.includes('127.0.0.1'))).toHaveLength(1); }); }); + +/** + * Carrying the plan out when there is no browser to start — a VM, a container, + * a server over SSH. The defect being guarded is Node's own: a failed spawn is + * reported as an `error` event a tick later, and an `error` event with no + * listener throws after every try/catch has returned and kills the CLI. + */ +describe('opening when no browser can start', () => { + const MISSING = 'capy-test-no-such-launcher-7f3a'; + const dialog = { kind: 'dialog' as const, browser: CHROME, env: env() }; + const handoff = { kind: 'handoff' as const, env: env() }; + + /** + * How Node reports a missing executable: no throw, `pid` left undefined, and + * an `error` event on the next tick. An EventEmitter's unhandled `error` + * throws, so if the listener were missing this would fail the run exactly as + * it killed the CLI. Built by hand because Bun (which runs this suite) throws + * synchronously from `spawn` instead, which is not the path the CLI takes. + */ + const nodeStyleMissing = (): Launchable => { + const child = Object.assign(new EventEmitter(), { pid: undefined }); + process.nextTick(() => + child.emit('error', Object.assign(new Error(`spawn ${MISSING} ENOENT`), { code: 'ENOENT' })), + ); + return child; + }; + /** Mirrors the `open` package: start inside an async function, return the child. */ + const openMissing = async (): Promise => { + await Promise.resolve(); + return nodeStyleMissing(); + }; + + test('a missing launcher resolves as not opened instead of crashing', async () => { + const result = await openScreen(URL, handoff, { openDefault: openMissing }); + expect(result.via).toBe('default-browser'); + expect(result.opened).toBe(false); + }); + + test('a missing app-window binary falls back, and reports not opened when that fails too', async () => { + const result = await openScreen(URL, dialog, { + spawnApp: nodeStyleMissing, + openDefault: openMissing, + }); + expect(result.via).toBe('app-window'); + expect(result.opened).toBe(false); + }); + + test('a launcher that throws while starting is not opened', async () => { + const result = await openScreen(URL, handoff, { + openDefault: () => Promise.reject(new Error('boom')), + }); + expect(result.opened).toBe(false); + }); + + test('a launcher that starts is opened', async () => { + const result = await openScreen(URL, handoff, { + openDefault: async () => spawn(process.execPath, ['-e', ''], { stdio: 'ignore' }), + }); + expect(result.opened).toBe(true); + }); + + test('suppressed never starts anything and is not opened', async () => { + const result = await openScreen(URL, { kind: 'handoff', env: env({ CAPY_WEB_NO_OPEN: '1' }) }, { + openDefault: () => Promise.reject(new Error('must not be called')), + }); + expect(result).toEqual({ via: 'suppressed', opened: false }); + }); + + test('launched() treats an object without events as started', async () => { + expect(launched({})).toBe(true); + }); +});