From 57c9a5066e0b7f53117cd9bf685ba557c21388a0 Mon Sep 17 00:00:00 2001 From: Catfish-75 Date: Sat, 23 May 2026 14:09:38 +0300 Subject: [PATCH] Fix Codex browse runtime on Windows --- browse/src/cli.ts | 81 ++++++++++++++++++++++++++++++------ browse/test/commands.test.ts | 2 +- browse/test/config.test.ts | 26 +++++++++++- setup | 40 +++++++++++++++--- test/gen-skill-docs.test.ts | 17 ++++++++ 5 files changed, 146 insertions(+), 20 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 4f523bea7a..8e9c8ca3c4 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; +import { spawnSync } from 'node:child_process'; import { safeUnlink, safeUnlinkQuiet, safeKill, isProcessAlive } from './error-handling'; import { writeSecureFile, mkdirSecure } from './file-permissions'; import { resolveConfig, ensureStateDir, readVersionHash } from './config'; @@ -19,7 +20,9 @@ import { redactProxyUrl } from './proxy-redact'; const config = resolveConfig(); const IS_WINDOWS = process.platform === 'win32'; -const MAX_START_WAIT = IS_WINDOWS ? 15000 : (process.env.CI ? 30000 : 8000); // Node+Chromium takes longer on Windows +const DEFAULT_START_WAIT = IS_WINDOWS ? 45000 : (process.env.CI ? 30000 : 8000); // Node+Chromium takes longer on Windows +const MAX_START_WAIT = Number.parseInt(process.env.BROWSE_START_WAIT_MS || '', 10) || DEFAULT_START_WAIT; +let startedServerThisRun = false; export function resolveServerScript( env: Record = process.env, @@ -229,16 +232,15 @@ async function startServer(extraEnv?: Record): Promise): Promise { const start = Date.now(); while (Date.now() - start < MAX_START_WAIT) { const freshState = readState(); - if (freshState && await isServerHealthy(freshState.port)) return freshState; + if (freshState && await isServerHealthy(freshState.port)) { + startedServerThisRun = true; + return freshState; + } await Bun.sleep(200); } throw new Error('Timed out waiting for another instance to start the server'); @@ -394,6 +400,7 @@ async function ensureServer(flags?: GlobalFlags): Promise { // Re-read state under lock in case another process just started the server const freshState = readState(); if (freshState && await isServerHealthy(freshState.port)) { + startedServerThisRun = true; return freshState; } @@ -405,8 +412,6 @@ async function ensureServer(flags?: GlobalFlags): Promise { console.error(`[browse] Starting server with proxy ${flags.redactedProxyUrl}${flags.headed ? ' (headed)' : ''}...`); } else if (flags?.headed) { console.error('[browse] Starting server in headed mode...'); - } else { - console.error('[browse] Starting server...'); } return await startServer(extraEnv); } finally { @@ -469,10 +474,8 @@ async function sendCommand(state: ServerState, command: string, args: string[], } const text = await resp.text(); - if (resp.ok) { - process.stdout.write(text); - if (!text.endsWith('\n')) process.stdout.write('\n'); + await writeStdout(text); } else { // Try to parse as JSON error try { @@ -489,8 +492,15 @@ async function sendCommand(state: ServerState, command: string, args: string[], console.error('[browse] Command timed out after 30s'); process.exit(1); } - // Connection error — server may have crashed + // `stop` intentionally tears the daemon down. On Windows/Node the socket + // can close before the response body reaches the CLI; treat that as a + // successful stop instead of triggering the generic crash-restart path. if (err.code === 'ECONNREFUSED' || err.code === 'ECONNRESET' || err.message?.includes('fetch failed')) { + if (command === 'stop' && !(await isServerHealthy(state.port))) { + safeUnlinkQuiet(config.stateFile); + await writeStdout('Server stopped'); + return; + } if (retries >= 1) throw new Error('[browse] Server crashed twice in a row — aborting'); console.error('[browse] Server connection lost. Restarting...'); // Kill the old server to avoid orphaned chromium processes @@ -513,6 +523,32 @@ async function sendCommand(state: ServerState, command: string, args: string[], } } +async function writeStdout(text: string): Promise { + const output = text.endsWith('\n') ? text : `${text}\n`; + fs.writeSync(1, output); +} + +async function handleStopCommand(commandArgs: string[]): Promise { + const state = readState(); + if (!state) { + await writeStdout('Server not running'); + return; + } + + if (await isServerHealthy(state.port)) { + await sendCommand(state, 'stop', commandArgs); + return; + } + + if (state.pid && isProcessAlive(state.pid)) { + await killServer(state.pid); + await writeStdout('Server stopped'); + } else { + await writeStdout('Server not running'); + } + safeUnlinkQuiet(config.stateFile); +} + // Module-level reference to the resolved global flags from main(). Used by // sendCommand's crash-retry path so a daemon restart after ECONNRESET doesn't // silently drop --proxy / --headed. @@ -1144,6 +1180,15 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: process.exit(0); } + // stop must never auto-start a daemon. The generic command path calls + // ensureServer(), which is correct for normal browser commands but wrong for + // shutdown: `browse stop` from a clean state should be a no-op, not a + // start-then-stop cycle that can leave a detached Windows process behind. + if (command === 'stop') { + await handleStopCommand(commandArgs); + process.exit(0); + } + // Special case: chain reads from stdin if (command === 'chain' && commandArgs.length === 0) { const stdin = await Bun.stdin.text(); @@ -1152,6 +1197,18 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: let state = await ensureServer(globalFlags); + if (startedServerThisRun && process.env.BROWSE_SKIP_REEXEC_AFTER_START !== '1') { + const result = spawnSync(process.execPath, process.argv.slice(2), { + stdio: ['ignore', 'pipe', 'pipe'], + encoding: 'utf8', + env: { ...process.env, BROWSE_SKIP_REEXEC_AFTER_START: '1' }, + }); + if (result.error) throw result.error; + if (result.stdout) fs.writeSync(1, result.stdout); + if (result.stderr) fs.writeSync(2, result.stderr); + process.exit(result.status ?? 1); + } + // ─── Pair-Agent (post-server, pre-dispatch) ────────────── if (command === 'pair-agent') { // Ensure headed mode — the user should see the browser window diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index b3870c0ccf..ebfdcdd0c6 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -33,7 +33,7 @@ beforeAll(async () => { bm = new BrowserManager(); await bm.launch(); -}); +}, 30000); afterAll(() => { // Force kill browser instead of graceful close (avoids hang) diff --git a/browse/test/config.test.ts b/browse/test/config.test.ts index 8daa27c3eb..7e6b8108cc 100644 --- a/browse/test/config.test.ts +++ b/browse/test/config.test.ts @@ -315,6 +315,30 @@ describe('startup error log', () => { }); }); +describe('cli command dispatch', () => { + const cliSource = fs.readFileSync(path.resolve(__dirname, '../src/cli.ts'), 'utf-8'); + + test('handles stop before ensureServer so shutdown never auto-starts a daemon', () => { + const stopDispatch = cliSource.indexOf('await handleStopCommand(commandArgs)'); + const ensureServerCall = cliSource.indexOf('let state = await ensureServer(globalFlags)'); + + expect(stopDispatch).toBeGreaterThan(-1); + expect(ensureServerCall).toBeGreaterThan(-1); + expect(stopDispatch).toBeLessThan(ensureServerCall); + }); + + test('cold-start re-exec preserves command stdout on stdout', () => { + expect(cliSource).toContain('if (result.stdout) fs.writeSync(1, result.stdout)'); + expect(cliSource).not.toContain('IS_WINDOWS ? 2 : 1, result.stdout'); + }); + + test('default headless cold-start does not print a delayed startup banner', () => { + expect(cliSource).not.toContain("console.error('[browse] Starting server...')"); + expect(cliSource).toContain('Starting server in headed mode'); + expect(cliSource).toContain('Starting server with proxy'); + }); +}); + describe('resolveGstackHome', () => { test('honors GSTACK_HOME env var when set', () => { const orig = process.env.GSTACK_HOME; @@ -367,7 +391,7 @@ describe('resolveChromiumProfile', () => { delete process.env.CHROMIUM_PROFILE; process.env.GSTACK_HOME = '/tmp/fallback-gstack'; try { - expect(resolveChromiumProfile()).toBe('/tmp/fallback-gstack/chromium-profile'); + expect(resolveChromiumProfile()).toBe(path.join('/tmp/fallback-gstack', 'chromium-profile')); } finally { if (origEnv !== undefined) process.env.CHROMIUM_PROFILE = origEnv; if (origHome === undefined) delete process.env.GSTACK_HOME; diff --git a/setup b/setup index 631b84003c..7c1052ea08 100755 --- a/setup +++ b/setup @@ -648,6 +648,39 @@ create_agents_sidecar() { done } +link_browse_runtime_assets() { + local gstack_dir="$1" + local runtime_root="$2" + + mkdir -p "$runtime_root/browse" "$runtime_root/node_modules" + + if [ -d "$gstack_dir/browse/dist" ]; then + _link_or_copy "$gstack_dir/browse/dist" "$runtime_root/browse/dist" + fi + # The compiled Windows CLI still resolves browse/src/server.ts at startup + # before it delegates to the Node-compatible server bundle. + if [ -d "$gstack_dir/browse/src" ]; then + _link_or_copy "$gstack_dir/browse/src" "$runtime_root/browse/src" + fi + if [ -d "$gstack_dir/browse/bin" ]; then + _link_or_copy "$gstack_dir/browse/bin" "$runtime_root/browse/bin" + fi + + # server-node.mjs intentionally externalizes these runtime packages. + for dep in playwright playwright-core diff; do + if [ -d "$gstack_dir/node_modules/$dep" ]; then + _link_or_copy "$gstack_dir/node_modules/$dep" "$runtime_root/node_modules/$dep" + fi + done + if [ -d "$gstack_dir/node_modules/@ngrok" ]; then + mkdir -p "$runtime_root/node_modules/@ngrok" + for scoped_dep in "$gstack_dir/node_modules/@ngrok"/*; do + [ -e "$scoped_dep" ] || continue + _link_or_copy "$scoped_dep" "$runtime_root/node_modules/@ngrok/$(basename "$scoped_dep")" + done + fi +} + # ─── Helper: create a minimal ~/.codex/skills/gstack runtime root ─────────── # Codex scans ~/.codex/skills recursively. Exposing the whole repo here causes # duplicate skills because source SKILL.md files and generated Codex skills are @@ -673,12 +706,7 @@ create_codex_runtime_root() { if [ -d "$gstack_dir/bin" ]; then _link_or_copy "$gstack_dir/bin" "$codex_gstack/bin" fi - if [ -d "$gstack_dir/browse/dist" ]; then - _link_or_copy "$gstack_dir/browse/dist" "$codex_gstack/browse/dist" - fi - if [ -d "$gstack_dir/browse/bin" ]; then - _link_or_copy "$gstack_dir/browse/bin" "$codex_gstack/browse/bin" - fi + link_browse_runtime_assets "$gstack_dir" "$codex_gstack" if [ -f "$agents_dir/gstack-upgrade/SKILL.md" ]; then _link_or_copy "$agents_dir/gstack-upgrade/SKILL.md" "$codex_gstack/gstack-upgrade/SKILL.md" fi diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index c594ea4bcd..659c584726 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -2224,6 +2224,23 @@ describe('setup script validation', () => { expect(setupContent).toContain('create_agents_sidecar "$SOURCE_GSTACK_DIR"'); }); + test('Codex runtime root includes browse source and externalized Node deps', () => { + const buildScript = fs.readFileSync(path.join(ROOT, 'browse', 'scripts', 'build-node-server.sh'), 'utf-8'); + const externals = [...buildScript.matchAll(/--external\s+"?([^"\\\s]+)"?/g)] + .map((match) => match[1]) + .filter((dep) => dep !== 'bun:sqlite'); + const fnStart = setupContent.indexOf('link_browse_runtime_assets()'); + const fnEnd = setupContent.indexOf('# ─── Helper: create a minimal ~/.codex/skills/gstack runtime root', fnStart); + const fnBody = setupContent.slice(fnStart, fnEnd); + + expect(fnBody).toContain('browse/src'); + for (const dep of externals) { + const root = dep.startsWith('@') ? dep.split('/')[0] : dep; + expect(fnBody).toContain(root); + } + expect(setupContent).toContain('link_browse_runtime_assets "$gstack_dir" "$codex_gstack"'); + }); + test('link_codex_skill_dirs reads from .agents/skills/', () => { // The Codex link function must reference .agents/skills for generated Codex skills const fnStart = setupContent.indexOf('link_codex_skill_dirs()');