test: eliminate remaining cross-file mock.module poisoning in client tests (closes #977)

.claude/rules/testing.md

@@ -21,6 +21,12 @@ Test file re-implements production logic instead of importing it. Signs: test-on
 ### 2. Module-Level Mocking
 Using `mock.module()` or `vi.mock()` instead of fetch-level mocks. Problems: bypasses actual API function logic, `mock.module()` is permanent in bun:test, tests pass even when integration is broken. Has caused production incidents (mocking `config.js` broke 26+ unrelated tests). **Preferred: dependency injection over module mocking.**
 
+**Never `mock.module()` a target that any other test file imports for real.** `bun:test`'s `mock.module()` is process-global and irreversible for the life of the test process, so it poisons every test file loaded afterward in the same process — and bun runs test files in directory readdir order (not CLI-arg order), which differs across operating systems. A suite that is green on one OS and red on another (or green locally and red in CI) is the signature of this failure mode (Issue #970, PR #976, Issue #977).
+
+- **Prohibited example:** `mock.module('../../routes/__root', () => ({ useWorktreeCreationTasksContext: () => ({ ... }) }))`. `routes/__root` is the route root, so multiple other test files (route tests, sibling component tests) import it for real; a mock factory that only re-declares a subset of its exports silently breaks any file that loads afterward and needs an export the factory omitted.
+- **Permitted example:** a module consumed exclusively by the one test file mocking it (e.g. a component's own tightly-scoped internal helper with no other importer) may still use `mock.module()`, but confirm the exclusivity first — grep the repo for other real importers before relying on this exception.
+- **Before adding a new `mock.module()` call**, grep the repository for other files that import the target module without mocking it. If any exist, do not use `mock.module()` — use one of, in order of preference: (1) a DI seam (prop / injected factory) on the component or hook under test, (2) `spyOn()` on the module's named export (restorable per-test via `.mockRestore()` in `afterEach`), (3) fetch-level request stubbing, (4) a real store/context with an injected fake value. See `test-standards` skill for worked conversion patterns.
+
 ### 3. Private Method Testing
 Attempting to test internal/private methods directly. Test through public interface instead, or extract to a separate module if complexity warrants it.
 
@@ -74,3 +80,4 @@ Before writing tests, verify:
 - [ ] Not following existing bad patterns blindly
 - [ ] Not changing production code just for testing without discussion
 - [ ] **Target code is mockable via DI** — check if the code under test imports module-level singletons. If it does, DI refactoring is required before the test can be written safely. Do NOT use `mock.module()` to work around missing DI. See Anti-Pattern #2.
+- [ ] **If a new `mock.module()` call is unavoidable**, grep the repo for other real importers of the target module first. See Anti-Pattern #2's cross-file-imported-target prohibition.

.claude/skills/test-standards/test-standards.md

@@ -168,6 +168,105 @@ describe('Client-Server Boundary', () => {
 
 This gives you a single test file that exercises the full round-trip: user event → form serialization → HTTP body → server handler → persisted state. If any step drops or mistransforms data, the test fails.
 
+## Converting Cross-File `mock.module()` Poisoning
+
+See [rules/testing.md](../../rules/testing.md) Anti-Pattern #2 for the prohibition (never `mock.module()` a target another test file real-imports) and when the exception applies. This section is the conversion how-to, with three worked patterns in order of preference. Before converting, grep the repo for other real importers of the target module to confirm the poisoning classification (Issue #977 Phase 1).
+
+### Pattern 1 — DI seam (prop / injected factory)
+
+Best when the component itself resolves the dependency via a module-level singleton call. Add an optional prop defaulting to the real function; production and other consumers are unaffected because the prop is optional.
+
+```typescript
+// Production: TerminalAdapter.tsx
+export function TerminalAdapter({
+  // ...
+  createInstance = getOrCreateTerminal,   // optional DI seam, defaults to the real store
+}: TerminalProps & { createInstance?: typeof getOrCreateTerminal }) {
+  const instance = useMemo(() => createInstance(sessionId, workerId, opts), [createInstance, sessionId, workerId, opts]);
+  // ...
+}
+
+// Test: TerminalAdapter.test.tsx
+const mockGetOrCreateTerminal = mock((_sessionId: string, _workerId: string): TerminalInstance => stubInstance);
+render(<TerminalAdapter sessionId="s" workerId="w" createInstance={mockGetOrCreateTerminal} />);
+```
+
+(Worked example: PR #976, `packages/client/src/components/terminal/TerminalAdapter.tsx` + its test.)
+
+### Pattern 2 — `spyOn()` on a named export
+
+Best for a hook or function the component-under-test imports and calls directly, when adding a DI prop is not warranted. `spyOn` is restorable per-test (unlike `mock.module()`), so it must be paired with `.mockRestore()` in `afterEach` — without that, the spy leaks into the next test in the same file (not other files, since `spyOn` targets a specific module-namespace object each test file imports independently).
+
+```typescript
+import * as useAppWsModule from '../../hooks/useAppWs';
+
+let useAppWsEventSpy: ReturnType<typeof spyOn>;
+
+beforeEach(() => {
+  useAppWsEventSpy = spyOn(useAppWsModule, 'useAppWsEvent').mockImplementation(() => undefined);
+});
+
+afterEach(() => {
+  useAppWsEventSpy.mockRestore();
+});
+```
+
+A generic function (`useAppWsState<T>(selector: (state) => T): T`) needs an explicit generic on the mock implementation so the cast is not silently `any`:
+
+```typescript
+useAppWsStateSpy = spyOn(useAppWsModule, 'useAppWsState').mockImplementation(<T,>() => false as T);
+```
+
+(Worked examples: `routes/__tests__/index.test.tsx`, `__tests__/routes/agents/index.test.tsx`, `components/sessions/hooks/__tests__/useSessionPageState.test.ts`, `components/worktrees/__tests__/QuickWorktreeDialog.test.tsx`.)
+
+### Pattern 3 — Real module/context + injected fake value
+
+Best when the module already exposes a designed seam — a context Provider, or a setter function — instead of hard-coding a return value via `mock.module()`. This is the least invasive option: zero production code changes, and the "fake" data flows through the real module's real code path.
+
+```typescript
+// lib/capabilities.ts already exposes a real setter for its module-level cache:
+import { setCapabilities } from '../../lib/capabilities';
+
+beforeEach(() => {
+  setCapabilities({ vscode: false, vscodeOpenMode: 'local-spawn', vscodeRemoteHost: null });
+});
+```
+
+```typescript
+// A React context consumed via useContext(): provide a REAL Provider with a fake value
+// instead of mock.module()-replacing the hook that reads it.
+import { WorktreeDeletionTasksContext } from '../../contexts/root-contexts';
+
+render(
+  <WorktreeDeletionTasksContext.Provider value={mockDeletionTasks}>
+    <SessionSettings {...props} />
+  </WorktreeDeletionTasksContext.Provider>
+);
+```
+
+Note the context is imported from its owning module (`contexts/root-contexts.ts`), not from the barrel that re-exports it (`routes/__root.tsx`) — importing from the owning module avoids pulling in the barrel's heavier dependency surface (router registration, layout components) and is the same object reference, so `useContext` inside the real hook resolves correctly either way.
+
+(Worked examples: `__tests__/routes/WorktreeRow.test.tsx` (Pattern 3a, setter), `hooks/__tests__/useCreateWorktree.test.ts` and `components/__tests__/SessionSettings.test.tsx` (Pattern 3b, Provider).)
+
+### Which pattern to reach for
+
+1. Does the target module already expose a public setter or a context Provider for the exact state the test needs? → **Pattern 3**.
+2. Is the dependency resolved via a direct function/hook call the component makes itself, with no existing seam? → **Pattern 2** (`spyOn`) is usually less code than adding a new prop; reach for **Pattern 1** (DI prop) only when the component needs the seam for a reason beyond testing (e.g. a legitimate caller-supplied override).
+3. Never fall back to `mock.module()` to avoid picking one of the above — see rules/testing.md Anti-Pattern #2.
+
+### `mock.module()` merges, it does not replace
+
+When classifying whether a `mock.module()` call site is a cross-file poisoner (Issue #977 Phase 1), do not assume the factory's return value fully replaces the module's export namespace for other importers. Empirically (Bun 1.3.10), `mock.module(specifier, factory)` **merges** `factory()`'s return value onto the real module's exports — an export the factory does not declare falls through to the real implementation for *every* importer, not just the ones that ran before the mock. Only the properties the factory *does* declare are overridden, and that override is what leaks cross-file.
+
+Two consequences for classification:
+
+- **A partial-override factory does not "break" untouched exports.** `lib/capabilities`'s poisoner overrode `hasVSCode` / `getVSCodeOpenMode` / `getVSCodeRemoteHost` but not `setCapabilities` — a victim reading `setCapabilities` sees the real function regardless of poisoning order. Do not conclude "benign" from this: the *overridden* exports still leak (verified below).
+- **"Does the victim's assertion still pass?" is not a reliable signal.** A victim can read a poisoned export, receive the wrong value, and still pass all its own assertions if it happens to be structurally tolerant of the substitution (e.g. it re-derives its own Provider/consumer pair from the same poisoned reference, so both sides stay internally consistent even though neither is talking to the real module). Classify by whether the *specific overridden export* is provably read by another file — via a reference/identity check or by inspecting the resolved function's source (`fn.toString()`) — not by whether that file's test suite currently goes red.
+
+**Verification technique used to classify all 5 call sites in PR #977:** force deterministic load order across two real files (CLI-arg order is not respected by Bun's scheduler — see the load-order note below) by placing a lexicographically-earlier-sorting temp copy of the poisoner in `src/`, then read the victim's imported binding's `.toString()` (for functions) or compare `===` identity (for objects/contexts) against a value stashed on `globalThis` by the poisoner. A source string matching the poisoner's factory body, or an identity match against the poisoner's locally-created object, is unambiguous proof of the leak — independent of whether the victim's own assertions happen to still pass.
+
+**Load order is not controllable via CLI argument order or simple filename convention.** Passing files in a specific order to `bun test fileA fileB` does not guarantee `fileA` loads first — Bun applies its own internal scan order regardless of argument order, and that order is not simple alphabetical (it appeared to depend on more than just the basename in ad-hoc testing). The only reliable lever found: bun evaluates a *directory tree* scan in some deterministic-but-opaque order, and paths starting with a double-underscore directory (`src/__polarity_X`) reliably sort ahead of ordinary `src/<lowercase-dir>/...` paths in practice — but always verify with an explicit `console.error` load marker in each candidate file rather than trusting the naming convention alone.
+
 ## Bun mock typing for `calls[N][M]` access
 
 Bun's `mock(async () => {})` infers the mock's `args` type as `[]` when no parameters are declared, even when the call site passes arguments. Reading `mock.calls[0][1]` then fails type-checking with `TS2493: Tuple type '[]' of length '0' has no element at index '1'`.

packages/client/src/__tests__/routes/WorktreeRow.test.tsx

@@ -1,20 +1,20 @@
-import { describe, it, expect, mock, afterEach } from 'bun:test';
+import { describe, it, expect, mock, beforeEach, afterEach } from 'bun:test';
 import { screen, cleanup } from '@testing-library/react';
 import { renderWithRouter } from '../../test/renderWithRouter';
-import { WorktreeDeletionTasksContext } from '../../routes/__root';
+import { WorktreeDeletionTasksContext } from '../../contexts/root-contexts';
+import { setCapabilities } from '../../lib/capabilities';
+import { WorktreeRow, type WorktreeRowProps, type SessionWithActivity } from '../../routes/index';
 import type { UseWorktreeDeletionTasksReturn } from '../../hooks/useWorktreeDeletionTasks';
 import type { Worktree, WorktreeSession, WorktreeDeletionTask, Session } from '@agent-console/shared';
 
-// Mock capabilities - reads from a module-level cache set during app initialization,
-// so module-level mocking is appropriate here (no business logic or fetch involved).
-mock.module('../../lib/capabilities', () => ({
-  hasVSCode: () => false,
-  getVSCodeOpenMode: () => 'local-spawn',
-  getVSCodeRemoteHost: () => null,
-}));
-
-// Import WorktreeRow AFTER mock.module calls to ensure mocks are applied
-import { WorktreeRow, type WorktreeRowProps, type SessionWithActivity } from '../../routes/index';
+// lib/capabilities reads from a module-level cache populated at app boot via the
+// real setCapabilities() setter. Using the setter instead of `mock.module` avoids
+// process-global poisoning of other test files that real-import lib/capabilities
+// in the same process (testing.md Anti-Pattern #2; e.g. routes/__tests__/index.test.tsx
+// spies on the real module's hasVSCode export).
+beforeEach(() => {
+  setCapabilities({ vscode: false, vscodeOpenMode: 'local-spawn', vscodeRemoteHost: null });
+});
 
 afterEach(cleanup);
 

packages/client/src/components/__tests__/SessionSettings.test.tsx

@@ -2,8 +2,8 @@ import { describe, it, expect, mock, beforeEach, afterEach } from 'bun:test';
 import { screen, fireEvent, waitFor, act, cleanup, render } from '@testing-library/react';
 import { createRootRoute, createRouter, createMemoryHistory, RouterProvider } from '@tanstack/react-router';
 import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
-import { createContext, useContext } from 'react';
 import { SessionSettings } from '../SessionSettings';
+import { WorktreeDeletionTasksContext } from '../../contexts/root-contexts';
 import type { UseWorktreeDeletionTasksReturn } from '../../hooks/useWorktreeDeletionTasks';
 
 // Helper to create mock Response
@@ -24,9 +24,6 @@ const prLinkResponse = createMockResponse({
   orgRepo: 'org/repo',
 });
 
-// Mock the WorktreeDeletionTasksContext
-const MockWorktreeDeletionTasksContext = createContext<UseWorktreeDeletionTasksReturn | null>(null);
-
 // Create mock deletion tasks context
 function createMockDeletionTasks(): UseWorktreeDeletionTasksReturn {
   return {
@@ -58,13 +55,17 @@ async function renderWithRouterAndContext(
     },
   });
 
-  // Mock the import of __root to use our mock context
-  // We need to replace the actual context with our mock
+  // Provide the deletion tasks context via the REAL WorktreeDeletionTasksContext
+  // (re-exported by routes/__root) instead of a `mock.module`-replaced module --
+  // mock.module is process-global in bun:test and would poison every other test
+  // file that real-imports routes/__root in the same process (testing.md
+  // Anti-Pattern #2). SessionSettings renders DeleteWorktreeDialog, which calls
+  // useWorktreeDeletionTasksContext() and requires a Provider ancestor.
   const rootRoute = createRootRoute({
     component: () => (
-      <MockWorktreeDeletionTasksContext.Provider value={deletionTasks}>
+      <WorktreeDeletionTasksContext.Provider value={deletionTasks}>
         {ui}
-      </MockWorktreeDeletionTasksContext.Provider>
+      </WorktreeDeletionTasksContext.Provider>
     ),
   });
   const memoryHistory = createMemoryHistory({
@@ -89,23 +90,6 @@ async function renderWithRouterAndContext(
   return { ...result, router, queryClient };
 }
 
-// mock.module replaces the entire module permanently in bun:test.
-// The mock must:
-// 1. Export WorktreeDeletionTasksContext so other test files can wrap with Provider
-// 2. Have useWorktreeDeletionTasksContext read from the context via useContext,
-//    so Provider-wrapped tests get the provided value (not always empty tasks)
-// 3. Fall back to default mock when no Provider is present (for SessionSettings tests)
-mock.module('../../routes/__root', () => ({
-  useWorktreeDeletionTasksContext: () => {
-    const context = useContext(MockWorktreeDeletionTasksContext);
-    if (!context) {
-      return createMockDeletionTasks();
-    }
-    return context;
-  },
-  WorktreeDeletionTasksContext: MockWorktreeDeletionTasksContext,
-}));
-
 describe('SessionSettings', () => {
   let originalFetch: typeof fetch;
   let mockFetch: ReturnType<typeof mock<(url: string, init?: RequestInit) => Promise<Response>>>;

packages/client/src/components/sessions/hooks/__tests__/useSessionPageState.test.ts

@@ -1,12 +1,20 @@
 import { describe, it, expect, mock, beforeEach, afterEach, afterAll, spyOn } from 'bun:test'
 import type { Session, Worker, AgentActivityState, WorkerActivityInfo, WorkerMessage } from '@agent-console/shared'
+import { renderHook, act } from '@testing-library/react'
+import { createElement, type ReactNode } from 'react'
+import { SessionDataContext, type SessionDataContextValue } from '../../../../contexts/root-contexts'
+import { useSessionPageState, type UseSessionPageStateOptions } from '../useSessionPageState'
+import * as useAppWsModule from '../../../../hooks/useAppWs'
 
-// --- useAppWsEvent mock ---
+// --- useAppWsEvent spy ---
 //
-// We mock useAppWsEvent to capture the callbacks the hook registers.
-// This avoids coupling to the WebSocket transport layer (already tested in useAppWs.test.ts)
-// and prevents interference from other test files that mock the same module via mock.module
-// (e.g., __root.test.tsx).
+// We replace useAppWsEvent per-test via `spyOn` (NOT `mock.module`, which is
+// process-global in bun:test and would poison every other test file that
+// real-imports hooks/useAppWs in the same process -- testing.md Anti-Pattern #2;
+// routes/__tests__/index.test.tsx and __tests__/routes/agents/index.test.tsx both
+// real-import this module for the same spyOn pattern) to capture the callbacks the
+// hook registers. This avoids coupling to the WebSocket transport layer (already
+// tested in useAppWs.test.ts).
 
 interface CapturedCallbacks {
   onSessionsSync?: (sessions: Session[], activityStates: WorkerActivityInfo[]) => void
@@ -21,18 +29,8 @@ interface CapturedCallbacks {
 
 let capturedCallbacks: CapturedCallbacks = {}
 
-mock.module('../../../../hooks/useAppWs', () => ({
-  useAppWsEvent: (options: CapturedCallbacks) => {
-    capturedCallbacks = options
-  },
-  useAppWsState: () => false,
-}))
-
-// Must import AFTER mock.module
-import { renderHook, act } from '@testing-library/react'
-import { createElement, type ReactNode } from 'react'
-import { SessionDataContext, type SessionDataContextValue } from '../../../../contexts/root-contexts'
-import { useSessionPageState, type UseSessionPageStateOptions } from '../useSessionPageState'
+let useAppWsEventSpy: ReturnType<typeof spyOn>
+let useAppWsStateSpy: ReturnType<typeof spyOn>
 
 // --- Fetch-level mocking ---
 
@@ -154,6 +152,16 @@ describe('useSessionPageState', () => {
 
   beforeEach(() => {
     consoleErrorSpy = spyOn(console, 'error').mockImplementation(() => {})
+    useAppWsEventSpy = spyOn(useAppWsModule, 'useAppWsEvent').mockImplementation(
+      (options = {}) => {
+        capturedCallbacks = options as CapturedCallbacks
+      },
+    )
+    // useAppWsState<T>(selector) is generic; useSessionPageState does not call it
+    // directly today, so the cast-returned value is never observed by production code.
+    useAppWsStateSpy = spyOn(useAppWsModule, 'useAppWsState').mockImplementation(
+      <T,>() => false as T
+    )
 
     capturedCallbacks = {}
     mockFetch.mockClear()
@@ -162,6 +170,8 @@ describe('useSessionPageState', () => {
 
   afterEach(() => {
     consoleErrorSpy.mockRestore()
+    useAppWsEventSpy.mockRestore()
+    useAppWsStateSpy.mockRestore()
   })
 
   describe('initial load', () => {