fix(tests): repair 7 pre-existing failures (env pollution + stale markers)

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) <noreply@anthropic.com>

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;
+      }
     }
   });
 

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<string, string> = { ... }`
+    // 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

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"]

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<string, string | undefined> = {};
+
+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;
+    }
+  }
+});

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

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(),