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.
This commit is contained in:
Garry Tan 2026-08-15 10:44:09 -07:00
parent 1e0fd96c15
commit 08532462c0
No known key found for this signature in database
GPG Key ID: C1F69E85C74EFE1D
7 changed files with 108 additions and 46 deletions

View File

@ -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<T>(fn: (dir: string) => T): T {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-lock-'));
@ -27,14 +41,27 @@ function captureErrors<T>(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);
});
});
});

View File

@ -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';

View File

@ -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);

View File

@ -15,12 +15,20 @@ const SCRIPT = join(import.meta.dir, '..', '..', 'bin', 'gstack-config');
let stateDir: string;
function run(args: string[] = [], extraEnv: Record<string, string> = {}) {
// 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<string, string | undefined> = {
...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',
});

View File

@ -17,14 +17,21 @@ let gstackDir: string;
let stateDir: string;
function run(extraEnv: Record<string, string> = {}, 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<string, string | undefined> = {
...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',
});

View File

@ -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.

View File

@ -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);