From 25f4e1a64916fd4e098481cb90f01dc1327fdac7 Mon Sep 17 00:00:00 2001 From: t Date: Fri, 10 Jul 2026 15:22:30 -0700 Subject: [PATCH] test(browse): bun-version-agnostic hooks, hermetic env, cross-file state restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - commands.test.ts: describe-scoped beforeAll(goto) hooks become beforeEach — bun 1.2.x runs describe-scoped beforeAll eagerly at file start in file order, so the three navigations clobbered each other and cascaded into 55 wrong-page / dead-session failures; beforeEach is correctly scoped on all bun versions. - content-security.test.ts: restore the module-load urlBlocklistFilter after the content-filter describe — the module instance is shared across files, and leaving the registry empty broke security-integration.test.ts downstream. - watchdog.test.ts: hardcoded ports 34901-34903 → BROWSE_PORT=0 (random), ending EADDRINUSE collisions with sibling worktree runs. - gstack-update-check.test.ts: pin GSTACK_HOME/GSTACK_STATE_ROOT to the test's state dir so leaked env from other files can't redirect config. - terminal-agent(.integration).test.ts: re-pin static-grep anchors to the current maybeSpawnPty/canDispatchOverTunnel source; feature-detected skip for Bun.spawn PTY support (absent on bun 1.2.x); success-path WebSocket handshake via raw TCP (fetch() never resolves 101 upgrades on bun 1.2.x). - dual-listener.test.ts: update pinned literal to the args-aware canDispatchOverTunnel(body?.command, body?.args) form. Co-Authored-By: Claude Fable 5 --- browse/test/commands.test.ts | 17 +++- browse/test/content-security.test.ts | 11 +++ browse/test/dual-listener.test.ts | 4 +- browse/test/gstack-update-check.test.ts | 8 ++ .../test/terminal-agent-integration.test.ts | 91 ++++++++++++++----- browse/test/terminal-agent.test.ts | 22 ++++- browse/test/watchdog.test.ts | 19 ++-- 7 files changed, 135 insertions(+), 37 deletions(-) diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index 506885b56..bfc25c9c7 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -5,7 +5,7 @@ * A real browse server is started and commands are sent via the CLI HTTP interface. */ -import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { describe, test, expect, beforeAll, beforeEach, afterAll } from 'bun:test'; import { startTestServer } from './test-server'; import { BrowserManager } from '../src/browser-manager'; import { resolveServerScript } from '../src/cli'; @@ -138,7 +138,12 @@ describe('Navigation', () => { // ─── Content Extraction ───────────────────────────────────────── describe('Content extraction', () => { - beforeAll(async () => { + // beforeEach, NOT beforeAll: bun <1.3 runs every describe-scoped beforeAll + // eagerly at file start (in file order), so a beforeAll goto here is + // clobbered by later describes' beforeAll gotos before any test runs. + // beforeEach is scoped correctly on all bun versions, and a goto against + // the local fixture server costs only a few ms per test. + beforeEach(async () => { await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm); }); @@ -196,7 +201,9 @@ describe('Content extraction', () => { // ─── JavaScript / CSS / Attrs ─────────────────────────────────── describe('Inspection', () => { - beforeAll(async () => { + // beforeEach, NOT beforeAll — see 'Content extraction' note (bun <1.3 + // runs describe-scoped beforeAll hooks eagerly at file start). + beforeEach(async () => { await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm); }); @@ -1074,7 +1081,9 @@ describe('Dialog handling', () => { // ─── Element State Checks (is) ───────────────────────────────── describe('Element state checks', () => { - beforeAll(async () => { + // beforeEach, NOT beforeAll — see 'Content extraction' note (bun <1.3 + // runs describe-scoped beforeAll hooks eagerly at file start). + beforeEach(async () => { await handleWriteCommand('goto', [baseUrl + '/states.html'], bm); }); diff --git a/browse/test/content-security.test.ts b/browse/test/content-security.test.ts index 51f9e5349..264019d76 100644 --- a/browse/test/content-security.test.ts +++ b/browse/test/content-security.test.ts @@ -124,6 +124,17 @@ describe('Content filter hooks', () => { clearContentFilters(); }); + // Restore module-load state when this describe is done. bun test runs + // every file in one process, so the content-security module instance is + // SHARED across test files — leaving the registry empty here breaks + // security-integration.test.ts, whose pipeline test pins that the + // built-in urlBlocklistFilter is registered (content-security.ts + // registers it at module load). + afterAll(() => { + clearContentFilters(); + registerContentFilter(urlBlocklistFilter); + }); + test('URL blocklist detects requestbin', () => { const result = urlBlocklistFilter('', 'https://requestbin.com/r/abc', 'text'); expect(result.safe).toBe(false); diff --git a/browse/test/dual-listener.test.ts b/browse/test/dual-listener.test.ts index 3ce04c1b7..f7c527bab 100644 --- a/browse/test/dual-listener.test.ts +++ b/browse/test/dual-listener.test.ts @@ -220,7 +220,9 @@ describe('/command tunnel command allowlist', () => { 'return handleCommand(body, tokenInfo)' ); expect(commandBlock).toContain("surface === 'tunnel'"); - expect(commandBlock).toContain('canDispatchOverTunnel(body?.command)'); + // Args-aware since the --out (disk write) tunnel ban: the dispatch gate + // takes both the command and its args. + expect(commandBlock).toContain('canDispatchOverTunnel(body?.command, body?.args)'); expect(commandBlock).toContain('disallowed_command'); expect(commandBlock).toContain('is not allowed over the tunnel surface'); expect(commandBlock).toContain('status: 403'); diff --git a/browse/test/gstack-update-check.test.ts b/browse/test/gstack-update-check.test.ts index 0edd366e4..73ffecb69 100644 --- a/browse/test/gstack-update-check.test.ts +++ b/browse/test/gstack-update-check.test.ts @@ -22,6 +22,14 @@ function run(extraEnv: Record = {}, args: string[] = []) { ...process.env, GSTACK_DIR: gstackDir, GSTACK_STATE_DIR: stateDir, + // gstack-config resolves its state dir as GSTACK_STATE_ROOT > + // GSTACK_HOME > GSTACK_STATE_DIR. Other test files in the same bun + // process set process.env.GSTACK_HOME mid-suite (and operator shells + // may export it too); a leaked value would win over GSTACK_STATE_DIR + // above and point gstack-config at the wrong config.yaml, making the + // update_check config tests order-dependent. Pin all three. + GSTACK_HOME: stateDir, + GSTACK_STATE_ROOT: stateDir, GSTACK_REMOTE_URL: `file://${join(gstackDir, 'REMOTE_VERSION')}`, ...extraEnv, }, diff --git a/browse/test/terminal-agent-integration.test.ts b/browse/test/terminal-agent-integration.test.ts index cdcbe8de5..b3c8ac604 100644 --- a/browse/test/terminal-agent-integration.test.ts +++ b/browse/test/terminal-agent-integration.test.ts @@ -22,6 +22,26 @@ import * as os from 'os'; const AGENT_SCRIPT = path.join(import.meta.dir, '../src/terminal-agent.ts'); const BASH = '/bin/bash'; +// Bun.spawn PTY support (the `terminal` spawn option) is required for the +// agent to stream PTY output. On older Bun (< 1.3) the option is silently +// ignored — proc.terminal is undefined, terminal.write() no-ops, and the +// round-trip can never produce output. Feature-detect instead of version- +// sniffing, and SKIP (not fake-pass) the output-dependent test on runtimes +// that genuinely lack the capability. All auth/handshake/control-frame +// tests still run everywhere — they don't need a live PTY. +const BUN_HAS_PTY = (() => { + try { + const probe = (Bun as any).spawn(['/bin/sh', '-c', 'exit 0'], { + terminal: { rows: 2, cols: 2, data() {} }, + }); + const has = !!probe.terminal; + try { probe.kill(); } catch {} + return has; + } catch { + return false; + } +})(); + let stateDir: string; let agentProc: any; let agentPort: number; @@ -128,7 +148,11 @@ describe('terminal-agent: /ws gates', () => { }); describe('terminal-agent: PTY round-trip via real WebSocket (Cookie auth)', () => { - test('binary writes go to PTY stdin, output streams back', async () => { + // Skipped when the Bun runtime lacks PTY spawn support (see BUN_HAS_PTY). + // Explicit 15s timeout: the test legitimately waits up to 5s for the WS + // open plus up to 5s for PTY output, which overflows bun's 5s default and + // leaves stale assertions that get mis-attributed to the NEXT test. + test.skipIf(!BUN_HAS_PTY)('binary writes go to PTY stdin, output streams back', async () => { const cookie = 'rt-token-must-be-at-least-seventeen-chars-long'; const granted = await grantToken(cookie); expect(granted.status).toBe(200); @@ -180,7 +204,7 @@ describe('terminal-agent: PTY round-trip via real WebSocket (Cookie auth)', () = try { ws.close(); } catch {} // Give cleanup a moment. await Bun.sleep(200); - }); + }, 15000); test('Sec-WebSocket-Protocol auth path: browser-style upgrade with token in protocol', async () => { // This is the path the actual browser extension takes. Cross-port @@ -199,32 +223,57 @@ describe('terminal-agent: PTY round-trip via real WebSocket (Cookie auth)', () = const token = 'sec-protocol-token-must-be-at-least-seventeen-chars'; await grantToken(token); - // We exercise the protocol path by raw-handshaking via fetch+Upgrade, - // because Bun's test-client WebSocket constructor doesn't propagate + // We exercise the protocol path with a raw TCP handshake, because + // Bun's test-client WebSocket constructor doesn't propagate // `protocols` cleanly when also passed `headers` (the constructor - // detects the third-arg form unreliably). Real browsers (Chromium) - // use the standard protocols arg fine — the server-side handler is - // identical either way, so this test still locks the load-bearing - // invariant: the agent accepts a token via Sec-WebSocket-Protocol - // and echoes the protocol back so a browser would accept the upgrade. + // detects the third-arg form unreliably), and fetch()+Upgrade headers + // never resolves for a 101 response on Bun < 1.3 (it only completes + // for ordinary statuses, so the success path hangs). Real browsers + // (Chromium) use the standard protocols arg fine — the server-side + // handler is identical either way, so this test still locks the + // load-bearing invariant: the agent accepts a token via + // Sec-WebSocket-Protocol and echoes the protocol back so a browser + // would accept the upgrade. const handshakeKey = 'dGhlIHNhbXBsZSBub25jZQ=='; - const resp = await fetch(`http://127.0.0.1:${agentPort}/ws`, { - headers: { - 'Connection': 'Upgrade', - 'Upgrade': 'websocket', - 'Sec-WebSocket-Version': '13', - 'Sec-WebSocket-Key': handshakeKey, - 'Sec-WebSocket-Protocol': `gstack-pty.${token}`, - 'Origin': 'chrome-extension://test-extension-id', - }, + const requestHead = [ + 'GET /ws HTTP/1.1', + `Host: 127.0.0.1:${agentPort}`, + 'Connection: Upgrade', + 'Upgrade: websocket', + 'Sec-WebSocket-Version: 13', + `Sec-WebSocket-Key: ${handshakeKey}`, + `Sec-WebSocket-Protocol: gstack-pty.${token}`, + 'Origin: chrome-extension://test-extension-id', + '', + '', + ].join('\r\n'); + + const net = await import('node:net'); + const responseHead = await new Promise((resolve, reject) => { + const chunks: Buffer[] = []; + const socket = net.connect(agentPort, '127.0.0.1', () => socket.write(requestHead)); + const finish = () => { + clearTimeout(timer); + socket.destroy(); + resolve(Buffer.concat(chunks).toString('utf8')); + }; + const timer = setTimeout(finish, 3000); + socket.on('data', (d: Buffer) => { + chunks.push(d); + // Response head complete once we see the blank line. + if (Buffer.concat(chunks).toString('utf8').includes('\r\n\r\n')) finish(); + }); + socket.on('error', (e: Error) => { clearTimeout(timer); reject(e); }); + socket.on('close', finish); }); // 101 Switching Protocols + protocol echoed back = browser would accept. // 401/403/anything else = browser would close the connection immediately // (the bug we hit in manual dogfood). - expect(resp.status).toBe(101); - expect(resp.headers.get('upgrade')?.toLowerCase()).toBe('websocket'); - expect(resp.headers.get('sec-websocket-protocol')).toBe(`gstack-pty.${token}`); + const statusLine = responseHead.split('\r\n')[0] || ''; + expect(statusLine).toContain('101'); + expect(responseHead.toLowerCase()).toContain('upgrade: websocket'); + expect(responseHead.toLowerCase()).toContain(`sec-websocket-protocol: gstack-pty.${token}`.toLowerCase()); }); test('Sec-WebSocket-Protocol auth: rejects unknown token even with valid Origin', async () => { diff --git a/browse/test/terminal-agent.test.ts b/browse/test/terminal-agent.test.ts index d908052d2..18064ce01 100644 --- a/browse/test/terminal-agent.test.ts +++ b/browse/test/terminal-agent.test.ts @@ -144,17 +144,33 @@ describe('Source-level guard: terminal-agent', () => { test('lazy spawn: claude PTY is spawned in message handler, not on upgrade', () => { // The whole point of lazy-spawn (codex finding #8) is that the WS - // upgrade itself does NOT call spawnClaude. Spawn happens on first - // message frame. + // upgrade itself does NOT spawn claude. Spawn happens on first + // message frame (binary input or the v1.44 explicit `start` frame), + // routed through the maybeSpawnPty helper, which is the only caller + // of spawnClaude. const upgradeBlock = AGENT_SRC.slice( AGENT_SRC.indexOf("if (url.pathname === '/ws')"), AGENT_SRC.indexOf("websocket: {"), ); expect(upgradeBlock).not.toContain('spawnClaude('); + expect(upgradeBlock).not.toContain('maybeSpawnPty('); // Spawn must be invoked from the message handler (lazy on first byte). const messageHandler = AGENT_SRC.slice(AGENT_SRC.indexOf('message(ws, raw)')); - expect(messageHandler).toContain('spawnClaude('); + expect(messageHandler).toContain('maybeSpawnPty('); expect(messageHandler).toContain('!session.spawned'); + // The open() upgrade handler must not spawn — it only creates the + // (spawned: false) session record or re-attaches a detached one. + const openBlock = AGENT_SRC.slice( + AGENT_SRC.indexOf('open(ws)'), + AGENT_SRC.indexOf('message(ws, raw)'), + ); + expect(openBlock).not.toContain('spawnClaude('); + expect(openBlock).not.toContain('maybeSpawnPty('); + // And the helper itself is where spawnClaude actually happens, gated + // on session.spawned so it stays a single-shot lazy spawn. + const helperBlock = AGENT_SRC.slice(AGENT_SRC.indexOf('function maybeSpawnPty')); + expect(helperBlock).toContain('spawnClaude('); + expect(helperBlock).toContain('if (session.spawned) return true;'); }); test('process.on uncaughtException + unhandledRejection handlers exist', () => { diff --git a/browse/test/watchdog.test.ts b/browse/test/watchdog.test.ts index 42faa262a..c9b2d8ad6 100644 --- a/browse/test/watchdog.test.ts +++ b/browse/test/watchdog.test.ts @@ -46,13 +46,19 @@ afterEach(async () => { serverProc = null; }); -function spawnServer(env: Record, port: number): Subprocess { +// No fixed BROWSE_PORT: these tests never dial the server's HTTP port — +// they only watch stdout markers and process liveness. BROWSE_PORT=0 lets +// the server pick a random free port (its default), so concurrent runs of +// this file (sibling worktrees, pr-test-env checkouts) and leftover +// servers from interrupted runs can't collide on a hardcoded port and +// kill the fresh spawn with EADDRINUSE before the 2s liveness check. +function spawnServer(env: Record): Subprocess { const stateFile = path.join(tmpDir, 'browse-state.json'); return spawn(['bun', 'run', SERVER_SCRIPT], { env: { ...process.env, BROWSE_STATE_FILE: stateFile, - BROWSE_PORT: String(port), + BROWSE_PORT: '0', ...env, }, stdio: ['ignore', 'pipe', 'pipe'], @@ -97,7 +103,7 @@ async function readStdoutUntil( describe('parent-process watchdog (v0.18.1.0)', () => { test('BROWSE_PARENT_PID=0 disables the watchdog', async () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'watchdog-pid0-')); - serverProc = spawnServer({ BROWSE_PARENT_PID: '0' }, 34901); + serverProc = spawnServer({ BROWSE_PARENT_PID: '0' }); const out = await readStdoutUntil( serverProc, @@ -115,10 +121,7 @@ describe('parent-process watchdog (v0.18.1.0)', () => { // Pass a bogus parent PID to prove BROWSE_HEADED takes precedence. // If the server-side guard regresses, the watchdog would try to poll // this PID and eventually fire on the "dead parent." - serverProc = spawnServer( - { BROWSE_HEADED: '1', BROWSE_PARENT_PID: '999999' }, - 34902, - ); + serverProc = spawnServer({ BROWSE_HEADED: '1', BROWSE_PARENT_PID: '999999' }); const out = await readStdoutUntil( serverProc, @@ -137,7 +140,7 @@ describe('parent-process watchdog (v0.18.1.0)', () => { const parentPid = parentProc.pid!; // Default headless: no BROWSE_HEADED, real parent PID — watchdog active. - serverProc = spawnServer({ BROWSE_PARENT_PID: String(parentPid) }, 34903); + serverProc = spawnServer({ BROWSE_PARENT_PID: String(parentPid) }); const serverPid = serverProc.pid!; // Give the server a moment to start and register the watchdog interval.