From 08532462c037ab557792b3931c8eafd89f351d1f Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 15 Aug 2026 10:44:09 -0700 Subject: [PATCH] test: adapt main's diagnostics tests to the merged designs cli-lock asserts typed ServerLockError (errno + lock path) instead of the log-and-return shape the merge didn't keep, dropping only the one duplicate of server-lock-errors coverage; the liveness tripwire exempts error-handling.ts as the sanctioned tasklist site; snapshot and compare-board wrappers pass the now-mandatory browser-manager arg; background.js's test pins that the retired sidebar-command type is rejected pre-gate with no response fields. --- browse/test/cli-lock.test.ts | 76 +++++++++++++------- browse/test/compare-board.test.ts | 2 +- browse/test/extension-sender-auth.test.ts | 16 ++++- browse/test/gstack-config.test.ts | 18 +++-- browse/test/gstack-update-check.test.ts | 21 ++++-- browse/test/process-liveness-windows.test.ts | 19 +++-- browse/test/snapshot.test.ts | 2 +- 7 files changed, 108 insertions(+), 46 deletions(-) diff --git a/browse/test/cli-lock.test.ts b/browse/test/cli-lock.test.ts index 79f4df8d3..9c6e04c19 100644 --- a/browse/test/cli-lock.test.ts +++ b/browse/test/cli-lock.test.ts @@ -1,8 +1,22 @@ +/** + * #1084 diagnostics — merged-design shape. + * + * Main's smell wave pinned a log-and-return-null acquireServerLock; this + * branch keeps the typed ServerLockError + bounded-retry design (fully pinned + * in server-lock-errors.test.ts). This file re-expresses the non-redundant + * assertion intents from the wave's test against the kept design: + * - unexpected open failures surface the REAL errno + lock path (typed + * throw), never phantom "another process holds the lock" contention; + * - holder-PID read failures surface errno + lock path the same way; + * - genuine live contention stays SILENT (null return, no stderr noise). + * Exact duplicates of server-lock-errors.test.ts coverage (stale-lock + * reacquire, ENOENT self-heal, EACCES throw) are deliberately not repeated. + */ import { describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; -import { acquireServerLock } from '../src/cli'; +import { acquireServerLock, ServerLockError } from '../src/cli'; function withTempDir(fn: (dir: string) => T): T { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-lock-')); @@ -27,14 +41,27 @@ function captureErrors(fn: () => T): { result: T; messages: string[] } { } describe('browse CLI server lock diagnostics (#1084)', () => { - test('logs non-EEXIST open failures instead of reporting phantom lock contention', () => { + test('unexpected open failures throw ServerLockError with the real errno — not phantom lock contention', () => { + if (process.platform === 'win32') return; // ENOTDIR errno mapping differs on Windows withTempDir((dir) => { - const lockPath = path.join(dir, 'missing-parent', 'browse.json.lock'); - const { result, messages } = captureErrors(() => acquireServerLock(lockPath)); + // A FILE where a directory is expected: open('wx') fails ENOTDIR — an + // errno that is neither contention (EEXIST) nor the self-healing + // missing-dir case (ENOENT). The old code's bare catch would have + // reported "another process holds the lock" forever. + const blocker = path.join(dir, 'blocker'); + fs.writeFileSync(blocker, 'not a directory\n'); + const lockPath = path.join(blocker, 'browse.json.lock'); - expect(result).toBeNull(); - expect(messages.join('\n')).toContain('unexpected ENOENT while opening'); - expect(messages.join('\n')).toContain(lockPath); + let thrown: any = null; + try { + acquireServerLock(lockPath); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(ServerLockError); + expect(thrown.code).toBe('ENOTDIR'); + expect(thrown.message).toContain('E_SERVER_LOCK (ENOTDIR)'); + expect(thrown.message).toContain(lockPath); }); }); @@ -50,30 +77,25 @@ describe('browse CLI server lock diagnostics (#1084)', () => { }); }); - test('logs holder PID read failures with code and lock path', () => { + test('holder PID read failures throw ServerLockError with code and lock path', () => { withTempDir((dir) => { + // Lock path exists but is a DIRECTORY: open('wx') → EEXIST (looks like + // contention), then the holder-PID read fails EISDIR. The kept design + // surfaces that errno + path in a typed error instead of retrying or + // reporting phantom contention. const lockPath = path.join(dir, 'browse.json.lock'); fs.mkdirSync(lockPath); - const { result, messages } = captureErrors(() => acquireServerLock(lockPath)); - - expect(result).toBeNull(); - expect(messages.join('\n')).toContain('unexpected EISDIR while reading holder PID from'); - expect(messages.join('\n')).toContain(lockPath); - }); - }); - - test('removes stale lock and reacquires it', () => { - withTempDir((dir) => { - const lockPath = path.join(dir, 'browse.json.lock'); - fs.writeFileSync(lockPath, 'not-a-pid\n'); - - const release = acquireServerLock(lockPath); - - expect(release).toBeFunction(); - expect(fs.readFileSync(lockPath, 'utf-8').trim()).toBe(String(process.pid)); - release?.(); - expect(fs.existsSync(lockPath)).toBe(false); + let thrown: any = null; + try { + acquireServerLock(lockPath); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(ServerLockError); + expect(thrown.code).toBe('EISDIR'); + expect(thrown.message).toContain('E_SERVER_LOCK (EISDIR)'); + expect(thrown.message).toContain(lockPath); }); }); }); diff --git a/browse/test/compare-board.test.ts b/browse/test/compare-board.test.ts index b9007af7e..90e1c9459 100644 --- a/browse/test/compare-board.test.ts +++ b/browse/test/compare-board.test.ts @@ -16,7 +16,7 @@ import { handleReadCommand as _handleReadCommand } from '../src/read-commands'; import { handleWriteCommand as _handleWriteCommand } from '../src/write-commands'; const handleReadCommand = (cmd: string, args: string[], b: BrowserManager) => - _handleReadCommand(cmd, args, b.getActiveSession()); + _handleReadCommand(cmd, args, b.getActiveSession(), b); const handleWriteCommand = (cmd: string, args: string[], b: BrowserManager) => _handleWriteCommand(cmd, args, b.getActiveSession(), b); import { generateCompareHtml } from '../../design/src/compare'; diff --git a/browse/test/extension-sender-auth.test.ts b/browse/test/extension-sender-auth.test.ts index df9fc1cda..ba5d4781c 100644 --- a/browse/test/extension-sender-auth.test.ts +++ b/browse/test/extension-sender-auth.test.ts @@ -32,9 +32,13 @@ const CONTENT_SCRIPT_SENDER = { id: OWN_ID, url: 'https://evil.example/page', ta const FOREIGN_SENDER = { id: FOREIGN_ID, url: `chrome-extension://${FOREIGN_ID}/background.html` }; const NO_URL_SENDER = { id: OWN_ID }; +// 'sidebar-command' is no longer a message type at all — the chat-queue path +// was ripped along with the /sidebar-command endpoint, so background.js now +// rejects it pre-gate as an unknown type (no response, nothing to leak). It is +// pinned separately below as a representative unknown type. const PRIVILEGED = [ 'getPort', 'setPort', 'getServerUrl', 'getToken', 'fetchRefs', - 'command', 'sidebar-command', 'getTabState', + 'command', 'getTabState', ]; // Content-script-originated flows that must keep working. const CONTENT_SCRIPT_TYPES = ['openSidePanel', 'elementPicked', 'pickerCancelled', 'inspectResult']; @@ -208,6 +212,16 @@ describe('background.js onMessage listener (behavioral)', () => { } }); + test('retired sidebar-command type is rejected pre-gate with no response and no leaks', () => { + // Even from the most-trusted sender shape, a type outside ALLOWED_TYPES + // never reaches a handler: no sendResponse, no token/port fields possible. + for (const sender of [PAGE_SENDER, CONTENT_SCRIPT_SENDER, FOREIGN_SENDER, NO_URL_SENDER]) { + const r = dispatch(listener, { type: 'sidebar-command', message: 'hi' }, sender); + expect(r.responded).toBe(false); + expectDenied(r); + } + }); + test('denied setPort never persists the attacker port', () => { const before = calls.storageSet.length; const r = dispatch(listener, { type: 'setPort', port: 6666 }, CONTENT_SCRIPT_SENDER); diff --git a/browse/test/gstack-config.test.ts b/browse/test/gstack-config.test.ts index 51adbe813..bb8da531a 100644 --- a/browse/test/gstack-config.test.ts +++ b/browse/test/gstack-config.test.ts @@ -15,12 +15,20 @@ const SCRIPT = join(import.meta.dir, '..', '..', 'bin', 'gstack-config'); let stateDir: string; function run(args: string[] = [], extraEnv: Record = {}) { + // The script resolves its state dir as GSTACK_STATE_ROOT > GSTACK_HOME > + // GSTACK_STATE_DIR > $HOME/.gstack. Strip the higher-precedence vars so a + // stray value in the harness env (another test file's leftovers, operator + // shell) can never outrank the per-test GSTACK_STATE_DIR isolation. + const env: Record = { + ...process.env, + GSTACK_STATE_DIR: stateDir, + }; + delete env.GSTACK_STATE_ROOT; + delete env.GSTACK_HOME; + Object.assign(env, extraEnv); // per-test overrides always win, deliberately + const result = Bun.spawnSync(['bash', SCRIPT, ...args], { - env: { - ...process.env, - GSTACK_STATE_DIR: stateDir, - ...extraEnv, - }, + env, stdout: 'pipe', stderr: 'pipe', }); diff --git a/browse/test/gstack-update-check.test.ts b/browse/test/gstack-update-check.test.ts index 104a2e04b..16bf7b851 100644 --- a/browse/test/gstack-update-check.test.ts +++ b/browse/test/gstack-update-check.test.ts @@ -17,14 +17,21 @@ let gstackDir: string; let stateDir: string; function run(extraEnv: Record = {}, args: string[] = []) { + // gstack-config (which this script shells out to for update_check) resolves + // state as GSTACK_STATE_ROOT > GSTACK_HOME > GSTACK_STATE_DIR > ~/.gstack. + // Strip the higher-precedence vars so harness-env leftovers can never + // outrank the per-test GSTACK_STATE_DIR isolation. + const env: Record = { + ...process.env, + GSTACK_DIR: gstackDir, + GSTACK_STATE_DIR: stateDir, + GSTACK_REMOTE_URL: `file://${join(gstackDir, 'REMOTE_VERSION')}`, + }; + delete env.GSTACK_STATE_ROOT; + delete env.GSTACK_HOME; + Object.assign(env, extraEnv); // per-test overrides always win, deliberately const result = Bun.spawnSync(['bash', SCRIPT, ...args], { - env: { - ...process.env, - GSTACK_DIR: gstackDir, - GSTACK_STATE_DIR: stateDir, - GSTACK_REMOTE_URL: `file://${join(gstackDir, 'REMOTE_VERSION')}`, - ...extraEnv, - }, + env, stdout: 'pipe', stderr: 'pipe', }); diff --git a/browse/test/process-liveness-windows.test.ts b/browse/test/process-liveness-windows.test.ts index e0fe1ff5a..d474473dc 100644 --- a/browse/test/process-liveness-windows.test.ts +++ b/browse/test/process-liveness-windows.test.ts @@ -54,10 +54,16 @@ describe('process liveness probe (Windows terminal-agent leak)', () => { expect(isProcessAlive(2147483646)).toBe(false); }); - test('3. isProcessAlive spawns NO subprocess', () => { + test('3. isProcessAlive spawns NO subprocess on POSIX (signal-0 path)', () => { // The heart of the bug: a liveness probe that forks is slow enough to // time out, and a timed-out probe silently answers "dead". Signal 0 // cannot time out because it never leaves the process. + // + // Merged design note: on win32 the helper DOES keep a single hardened + // tasklist probe (windowsHide, bounded timeout, quoted-CSV PID match) + // because Bun's process.kill(pid, 0) throws ESRCH for live Windows PIDs + // in compiled binaries. The POSIX path stays subprocess-free. + if (process.platform === 'win32') return; const origSpawn = (Bun as any).spawn; const origSpawnSync = (Bun as any).spawnSync; const spawns: string[] = []; @@ -73,11 +79,16 @@ describe('process liveness probe (Windows terminal-agent leak)', () => { } }); - test('4. no source file probes liveness via tasklist', () => { - // Static tripwire: re-introducing a tasklist-based existence check - // anywhere in src/ resurrects the false-negative class. + test('4. no source file probes liveness via tasklist outside the central helper', () => { + // Static tripwire: ad-hoc tasklist existence checks scattered across src/ + // resurrect the false-negative class (each call site re-invents the + // timeout/parse handling and gets it subtly wrong). The ONE sanctioned + // site is error-handling.ts's isProcessAlive win32 branch — centralized, + // windowsHide, bounded timeout, quoted-CSV `"${pid}"` match. Every other + // file must route through the helper. const offenders: string[] = []; for (const { file, content } of readAllSourceFiles()) { + if (file === 'error-handling.ts') continue; // the canonical helper const code = stripComments(content); // `PID eq` is the existence-probe form specifically. Other tasklist // uses (e.g. IMAGENAME filters for browser detection) are unaffected. diff --git a/browse/test/snapshot.test.ts b/browse/test/snapshot.test.ts index d3c012eaf..7f509eed0 100644 --- a/browse/test/snapshot.test.ts +++ b/browse/test/snapshot.test.ts @@ -14,7 +14,7 @@ import { handleMetaCommand } from '../src/meta-commands'; import * as fs from 'fs'; const handleReadCommand = (cmd: string, args: string[], b: BrowserManager) => - _handleReadCommand(cmd, args, b.getActiveSession()); + _handleReadCommand(cmd, args, b.getActiveSession(), b); const handleWriteCommand = (cmd: string, args: string[], b: BrowserManager) => _handleWriteCommand(cmd, args, b.getActiveSession(), b);