From 2a559533877156c70020c634e9f003739c97ff35 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 23 May 2026 19:25:06 -0700 Subject: [PATCH] fix(tests): repair 7 pre-existing failures (env pollution + stale markers) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 7 failures existed on main before this branch — verified via `git stash` round-trip. Bundling them into the long-lived-sidebar PR because we kept tripping over them while running `bun test` to verify Commit 0. * Global afterEach restores `process.env.PATH` (new bunfig.toml + test-setup.ts). browser-skill-commands.test.ts sets `PATH = '/test/bin:/usr/bin'` to exercise a scrubbed-env fixture and used the broken `process.env = origEnv` reassignment pattern that swaps the proxy reference; the underlying env stayed mutated and leaked downstream. Fixed three call sites in that file and added a narrow PATH-only global guardrail so a future polluter can't bring the bug back. Killed: pair-agent-tunnel-eval (bun ENOENT), security.test.ts > resolveBashBinary (Bun.which('bash') null), server-no-import-side-effects (bun ENOENT). * server-auth.test.ts: two `sliceBetween` markers referenced strings deleted when sidebar-agent.ts was ripped — `'Sidebar agent started'` → `'Terminal agent started'`, `'Sidebar endpoints'` → `'Batch endpoint'`. Also fixed the pair-agent BROWSE_PARENT_PID assertion (the literal `serverEnv.BROWSE_PARENT_PID` never existed in source; the actual contract is the object-literal `BROWSE_PARENT_PID: '0'` inside the `const serverEnv` declaration). * test/upgrade-migration-v1.test.ts: also overrides HOME in the spawn env. The migration shells out to `${HOME}/.claude/skills/gstack/bin/gstack-config` and a developer's real config with `explain_level` set causes the script to take the "user already decided" branch and skip writing the pending-prompt flag the test asserts on. * test/setup-codesign.test.ts: replaced fragile `bun run build` string-match (which hit a comment 700 lines later) with the actual invocation `bun_cmd run build` used in the setup script. Net: full suite is now green; CI no longer trips on bash/bun-ENOENT from PATH pollution or on test markers that drifted with the codebase. Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/test/browser-skill-commands.test.ts | 29 ++++++++++-- browse/test/server-auth.test.ts | 19 +++++--- bunfig.toml | 8 ++++ test-setup.ts | 53 ++++++++++++++++++++++ test/setup-codesign.test.ts | 9 ++-- test/upgrade-migration-v1.test.ts | 6 ++- 6 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 bunfig.toml create mode 100644 test-setup.ts diff --git a/browse/test/browser-skill-commands.test.ts b/browse/test/browser-skill-commands.test.ts index 5dc1b6afb..889c2d46e 100644 --- a/browse/test/browser-skill-commands.test.ts +++ b/browse/test/browser-skill-commands.test.ts @@ -178,7 +178,17 @@ describe('buildSpawnEnv', () => { process.env.LANG = 'en_US.UTF-8'; }); afterEach(() => { - process.env = origEnv; + // process.env = origEnv replaces only the reference; the underlying + // env stays mutated and leaks to later test files in the same Bun + // process (e.g., breaks Bun.which('bash') in security.test.ts and + // bun-spawn in pair-agent-tunnel-eval.test.ts). Delete every current + // key then re-assign from the snapshot — restores the actual env. + for (const k of Object.keys(process.env)) { + if (!(k in origEnv)) delete process.env[k]; + } + for (const [k, v] of Object.entries(origEnv)) { + if (v !== undefined) process.env[k] = v; + } }); it('untrusted: drops $HOME and secrets', () => { @@ -293,7 +303,15 @@ describe.skipIf(SKIP_SPAWN)('spawnSkill: lifecycle', () => { expect(parsed.gh).toBeNull(); expect(parsed.gstack).toBeNull(); } finally { - process.env = origEnv; + // See afterEach comment in `buildSpawnEnv` describe — direct + // reassignment of process.env doesn't actually restore the + // underlying env in Bun. Delete + re-assign instead. + for (const k of Object.keys(process.env)) { + if (!(k in origEnv)) delete process.env[k]; + } + for (const [k, v] of Object.entries(origEnv)) { + if (v !== undefined) process.env[k] = v; + } } }); @@ -312,7 +330,12 @@ describe.skipIf(SKIP_SPAWN)('spawnSkill: lifecycle', () => { const parsed = JSON.parse(result.stdout); expect(parsed.home).toBe('/Users/test-user'); } finally { - process.env = origEnv; + for (const k of Object.keys(process.env)) { + if (!(k in origEnv)) delete process.env[k]; + } + for (const [k, v] of Object.entries(origEnv)) { + if (v !== undefined) process.env[k] = v; + } } }); diff --git a/browse/test/server-auth.test.ts b/browse/test/server-auth.test.ts index 8732866b5..2469a121b 100644 --- a/browse/test/server-auth.test.ts +++ b/browse/test/server-auth.test.ts @@ -63,13 +63,13 @@ describe('Server auth security', () => { // Test 4: /activity/history requires auth via validateAuth test('/activity/history requires authentication', () => { - const historyBlock = sliceBetween(SERVER_SRC, "url.pathname === '/activity/history'", 'Sidebar endpoints'); + const historyBlock = sliceBetween(SERVER_SRC, "url.pathname === '/activity/history'", 'Batch endpoint'); expect(historyBlock).toContain('validateAuth'); }); // Test 5: /activity/history has no wildcard CORS header test('/activity/history has no wildcard CORS header', () => { - const historyBlock = sliceBetween(SERVER_SRC, "url.pathname === '/activity/history'", 'Sidebar endpoints'); + const historyBlock = sliceBetween(SERVER_SRC, "url.pathname === '/activity/history'", 'Batch endpoint'); expect(historyBlock).not.toContain("'*'"); }); @@ -314,7 +314,7 @@ describe('Server auth security', () => { // Regression: connect command crashed with "domains is not defined" because // a stray `domains,` variable was in the status fetch body (cli.ts:852). test('connect command status fetch body has no undefined variable references', () => { - const connectBlock = sliceBetween(CLI_SRC, 'Launching headed Chromium', 'Sidebar agent started'); + const connectBlock = sliceBetween(CLI_SRC, 'Launching headed Chromium', 'Terminal agent started'); // The status fetch should use a clean JSON body expect(connectBlock).toContain("command: 'status'"); // Must NOT contain a bare `domains` reference in the fetch body @@ -335,10 +335,15 @@ describe('Server auth security', () => { // The connect subprocess env must override BROWSE_PARENT_PID expect(pairBlock).toContain("BROWSE_PARENT_PID"); expect(pairBlock).toContain("'0'"); - // The connect command must propagate BROWSE_PARENT_PID=0 to serverEnv - const connectBlock = sliceBetween(CLI_SRC, 'Launching headed Chromium', 'Sidebar agent started'); - expect(connectBlock).toContain("BROWSE_PARENT_PID"); - expect(connectBlock).toContain("serverEnv.BROWSE_PARENT_PID"); + // The connect command must propagate BROWSE_PARENT_PID=0 via the + // serverEnv object literal passed to startServer. The literal text + // `serverEnv.BROWSE_PARENT_PID` is NOT in source — the value is + // assigned via object-literal syntax (`BROWSE_PARENT_PID: '0'`) + // inside the `const serverEnv: Record = { ... }` + // declaration. Assert both pieces appear in the connect block. + const connectBlock = sliceBetween(CLI_SRC, 'Launching headed Chromium', 'Terminal agent started'); + expect(connectBlock).toContain("const serverEnv"); + expect(connectBlock).toContain("BROWSE_PARENT_PID: '0'"); }); // Regression: newtab returned 403 for scoped tokens because the tab ownership diff --git a/bunfig.toml b/bunfig.toml new file mode 100644 index 000000000..c53535f07 --- /dev/null +++ b/bunfig.toml @@ -0,0 +1,8 @@ +[test] +# Snapshot + restore process.env after every test. Centralizes the +# defense against per-file env mutations leaking into subsequent files +# (e.g., PATH = '/test/bin:/usr/bin' from a security-test fixture +# causing `Bun.which('bash')` to return null in unrelated downstream +# tests, or `Bun.spawn(['bun', ...])` to ENOENT when looking for bun +# via the polluted PATH). +preload = ["./test-setup.ts"] diff --git a/test-setup.ts b/test-setup.ts new file mode 100644 index 000000000..fb742ab2d --- /dev/null +++ b/test-setup.ts @@ -0,0 +1,53 @@ +/** + * Global test preload (Bun: `[test] preload` in bunfig.toml). + * + * Snapshots `process.env` once at preload time, then restores it after + * every test. Defends against the recurring pollution class where one + * test file mutates `process.env.PATH` / `HOME` / etc. and leaks into + * unrelated subsequent files in the same Bun process — surfaces as + * `Executable not found in $PATH: "bun"` or `Bun.which('bash')` returning + * null in tests that have no business touching env. + * + * `process.env = X` reassignment does work in Bun (it swaps the underlying + * proxy), but several test files use the broken pattern of + * `origEnv = {...process.env}` followed by per-test mutation without a + * matching restore inside try/finally. Centralizing the safety net here + * means new tests don't have to remember the dance, and the bug class + * stays dead. + */ +import { afterEach, beforeAll } from 'bun:test'; + +// Narrowly restore PATH after every test. Defends against the recurring +// pollution class where one test sets `process.env.PATH = '/test/bin:/usr/bin'` +// to exercise a scrubbed-env fixture and either forgets to restore or uses +// the broken `process.env = origEnv` reassignment, then a downstream test +// (security.test.ts > resolveBashBinary, pair-agent-tunnel-eval, or +// server-no-import-side-effects) sees the wrong PATH and either has +// `Bun.which('bash')` return null or `Bun.spawn(['bun', ...])` ENOENT. +// +// Deliberately narrow: snapshotting + restoring all of process.env breaks +// tests that legitimately set per-file env at module top-level (e.g., +// domain-skills-storage.test.ts assigns `process.env.GSTACK_HOME` at +// import time so the loaded module reads the test sandbox path on first +// invocation — wiping that on afterEach would route reads at the user's +// real ~/.gstack and the test would assert on the wrong filesystem). +// +// If a future test pollutes a different variable in the same broken way, +// add it to RESTORE_KEYS rather than widening the snapshot scope. +const RESTORE_KEYS = ['PATH', 'Path'] as const; +const baseline: Record = {}; + +beforeAll(() => { + for (const k of RESTORE_KEYS) baseline[k] = process.env[k]; +}); + +afterEach(() => { + for (const k of RESTORE_KEYS) { + const want = baseline[k]; + if (want === undefined) { + if (process.env[k] !== undefined) delete process.env[k]; + } else if (process.env[k] !== want) { + process.env[k] = want; + } + } +}); diff --git a/test/setup-codesign.test.ts b/test/setup-codesign.test.ts index 1ac7a4982..506ce5cae 100644 --- a/test/setup-codesign.test.ts +++ b/test/setup-codesign.test.ts @@ -33,9 +33,12 @@ describe('setup: Apple Silicon codesign', () => { test('codesign block is inside the NEEDS_BUILD=1 branch', () => { const content = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); - // The codesign block should appear after `bun run build` and before the - // `if [ ! -x "$BROWSE_BIN" ]` guard that checks the build succeeded. - const buildIdx = content.indexOf('bun run build'); + // The codesign block should appear after the build command and before the + // `if [ ! -x "$BROWSE_BIN" ]` guard that checks the build succeeded. The + // setup script invokes the build via `bun_cmd run build` (not literal + // `bun run build`) so the wrapper can route through asdf/volta/etc; + // matching the wrapped form keeps this test stable across that indirection. + const buildIdx = content.indexOf('bun_cmd run build'); const codesignIdx = content.indexOf('codesign --remove-signature'); const browseCheckIdx = content.indexOf('gstack setup failed: browse binary missing'); expect(buildIdx).toBeGreaterThan(-1); diff --git a/test/upgrade-migration-v1.test.ts b/test/upgrade-migration-v1.test.ts index edef6ee3a..09fdaf2c2 100644 --- a/test/upgrade-migration-v1.test.ts +++ b/test/upgrade-migration-v1.test.ts @@ -26,9 +26,13 @@ afterEach(() => { }); function run(): { stdout: string; stderr: string; status: number } { + // Override HOME too — the migration reads `${HOME}/.claude/skills/gstack/bin/gstack-config`, + // and the developer's real config may have `explain_level` set, which the + // migration interprets as "user already decided" and short-circuits without + // writing the pending-prompt flag (breaking these tests). const res = spawnSync('bash', [MIGRATION], { encoding: 'utf-8', - env: { ...process.env, GSTACK_HOME: tmpHome }, + env: { ...process.env, GSTACK_HOME: tmpHome, HOME: tmpHome }, }); return { stdout: (res.stdout ?? '').trim(),