From 785db96e3cb0023e0a7b51befba97a6f54c84b8d Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:50:18 -0700 Subject: [PATCH 1/5] Restore mock.module state that leaked across test files Bun swaps a mocked module's exports onto the same namespace object it hands back from import(), so a file that captures "the real module" via a bare import() before mocking is holding a reference that later mutates into the mock. Restoring with that reference in afterAll was therefore a no-op, and files loaded after left every other test in the process running against the mock: authz, verify, secret-guard, and read-file-guard plugins collapsed to their fakes, mcpClientToAgentTools lost several exports, and any file statically importing mcp/client.js after web-search.test.ts lost unwrapToolContent and connectMCPServers entirely. Shallow-copying the captured exports at capture time freezes a real snapshot before the mock overwrites the shared object, so afterAll can actually put the original back. The web-search mock also now spreads the real module instead of replacing it outright, and the mouse-reporting-disabled mock (already spreading real exports) gets the same snapshot fix so its existing afterAll restore is not silently defeated the same way. The dead mock of a nonexistent src/web/plugin.js module is removed along with its unusable real-module capture. --- src/tools/web-search.test.ts | 16 ++++++- .../mouse-reporting-disabled.test.ts | 7 +++- tests/unit/tui/agent-tools.test.ts | 42 ++++++++++++++++--- 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/src/tools/web-search.test.ts b/src/tools/web-search.test.ts index 782019c1c..544f893a4 100644 --- a/src/tools/web-search.test.ts +++ b/src/tools/web-search.test.ts @@ -1,9 +1,19 @@ -import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { afterAll, afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; const calls: { toolName: string; args: Record }[] = []; let connectConfigs: { name: string; url?: string }[] = []; +// Bun mutates the imported namespace object in place when a module is +// mocked, so the capture is shallow-copied immediately -- holding onto the +// live namespace would turn into the mocked exports as soon as mock.module +// below runs, making the afterAll restore a no-op. The mock also needs to +// spread the real module rather than replace it outright, or any other +// export (unwrapToolContent, connectMCPServers) disappears for the rest of +// the process for every file that runs after this one. +const realClient = { ...(await import("../mcp/client.js")) }; + mock.module("../mcp/client.js", () => ({ + ...realClient, connectMCPServer: async (config: { name: string; url?: string }) => { connectConfigs.push(config); return { @@ -21,6 +31,10 @@ mock.module("../mcp/client.js", () => ({ }, })); +afterAll(() => { + mock.module("../mcp/client.js", () => realClient); +}); + const { createWebSearchTool, disposeWebSearchClients, diff --git a/src/tui-opentui/mouse-reporting-disabled.test.ts b/src/tui-opentui/mouse-reporting-disabled.test.ts index 47ef8a8c9..a3b207318 100644 --- a/src/tui-opentui/mouse-reporting-disabled.test.ts +++ b/src/tui-opentui/mouse-reporting-disabled.test.ts @@ -22,7 +22,12 @@ const mountedHarnesses: Harness[] = [] // helpers) does a real `@opentui/core` import, or that import wins the module // cache and the mock never takes effect. Every dependency below is loaded // with a dynamic `import()` after `mock.module` for that reason. -const realCore = await import("@opentui/core") +// +// Bun mutates the imported namespace object in place when a module is +// mocked, so the capture is shallow-copied immediately -- holding onto the +// live namespace would turn into the mocked exports the moment mock.module +// below runs, making the afterAll restore below a no-op. +const realCore = { ...(await import("@opentui/core")) } mock.module("@opentui/core", () => ({ ...realCore, diff --git a/tests/unit/tui/agent-tools.test.ts b/tests/unit/tui/agent-tools.test.ts index 2ca97e68e..c8a04cb06 100644 --- a/tests/unit/tui/agent-tools.test.ts +++ b/tests/unit/tui/agent-tools.test.ts @@ -1,4 +1,4 @@ -import { test, expect, mock } from "bun:test"; +import { afterAll, test, expect, mock } from "bun:test"; import type { ToolDefinition, ToolCall } from "@intx/types/runtime"; import { TOOL_NAMES } from "@intx/tools-posix"; @@ -17,6 +17,27 @@ const mockPosixTools = { dispose: mockDispose, }; +// mock.module replaces the shared module cache for the whole test process, so +// every other file that imports these modules runs against the mock until it +// is put back. Capture the real modules up front and restore them in +// afterAll so this file's mocking is invisible outside its own tests. Bun +// mutates the imported namespace object in place when a module is mocked, so +// each capture is shallow-copied immediately -- holding onto the live +// namespace instead would silently turn into the mocked exports as soon as +// mock.module below runs, making the "restore" a no-op. +const realToolsPosix = { ...(await import("@intx/tools-posix")) }; +const realPosixToolPlugins = { ...(await import("../../../src/agent/posix-tool-plugins.js")) }; +const realMcpPlugin = { ...(await import("../../../src/mcp/plugin.js")) }; +const realPathEscapePlugin = { ...(await import("../../../src/plugins/path-escape-plugin.js")) }; +const realAuthzPlugin = { ...(await import("../../../src/plugins/authz-plugin.js")) }; +const realVerifyPlugin = { ...(await import("../../../src/plugins/verify-plugin.js")) }; +const realPermissionPlugin = { ...(await import("../../../src/plugins/permission-plugin.js")) }; +const realSecretGuardPlugin = { ...(await import("../../../src/plugins/secret-guard-plugin.js")) }; +const realShellGuardPlugin = { ...(await import("../../../src/plugins/shell-guard-plugin.js")) }; +const realReadFileGuardPlugin = { ...(await import("../../../src/plugins/read-file-guard-plugin.js")) }; +const realEditFileLineRange = { ...(await import("../../../src/plugins/edit-file-line-range.js")) }; +const realDirector = { ...(await import("../../../src/agent/director.js")) }; + mock.module("@intx/tools-posix", () => ({ createPosixTools: () => mockPosixTools, TOOL_NAMES, @@ -70,10 +91,6 @@ mock.module("../../../src/plugins/edit-file-line-range.js", () => ({ advertiseEditFileLineRange: (defs: ToolDefinition[]) => defs, })); -mock.module("../../../src/web/plugin.js", () => ({ - webToolsPlugin: () => ({}), -})); - mock.module("../../../src/agent/director.js", () => ({ askOperatorDefinition: { name: "ask_operator", @@ -93,6 +110,21 @@ mock.module("../../../src/agent/director.js", () => ({ createChatDirector: mock(() => ({})), })); +afterAll(() => { + mock.module("@intx/tools-posix", () => realToolsPosix); + mock.module("../../../src/agent/posix-tool-plugins.js", () => realPosixToolPlugins); + mock.module("../../../src/mcp/plugin.js", () => realMcpPlugin); + mock.module("../../../src/plugins/path-escape-plugin.js", () => realPathEscapePlugin); + mock.module("../../../src/plugins/authz-plugin.js", () => realAuthzPlugin); + mock.module("../../../src/plugins/verify-plugin.js", () => realVerifyPlugin); + mock.module("../../../src/plugins/permission-plugin.js", () => realPermissionPlugin); + mock.module("../../../src/plugins/secret-guard-plugin.js", () => realSecretGuardPlugin); + mock.module("../../../src/plugins/shell-guard-plugin.js", () => realShellGuardPlugin); + mock.module("../../../src/plugins/read-file-guard-plugin.js", () => realReadFileGuardPlugin); + mock.module("../../../src/plugins/edit-file-line-range.js", () => realEditFileLineRange); + mock.module("../../../src/agent/director.js", () => realDirector); +}); + const { createAgentToolset } = await import("../../../src/agent/tools.js"); const fakePermissionGate = { From 9eacfeb1cb721852109beadf70e34953d2710b2f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:50:26 -0700 Subject: [PATCH 2/5] Reset the pricing refresh latch before each test, not just after schedulePricingMetadataRefresh guards itself with a module-level one-shot flag shared by the whole test process. Any other file that exercises loadConfig's real bootstrap path leaves that flag set to true and never clears it, so this file's own first test silently inherited "already scheduled" from whichever file ran before it and its offlineFetch was never called. --- src/pricing-metadata.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/pricing-metadata.test.ts b/src/pricing-metadata.test.ts index 25f061a30..e0f46e011 100644 --- a/src/pricing-metadata.test.ts +++ b/src/pricing-metadata.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -15,6 +15,14 @@ import { contextWindowFor } from "./provider/context-window.js"; import { writePricingCache } from "./cost/pricing-fetcher.js"; describe("pricing-metadata", () => { + // refreshScheduled is a module-level one-shot latch shared with every other + // file in this process; another file's real loadConfig() call can leave it + // set before this file's first test ever runs. Reset on both sides so this + // suite's outcome does not depend on what ran before it. + beforeEach(() => { + resetPricingMetadataRefreshForTests(); + }); + afterEach(() => { resetPricingMetadataRefreshForTests(); applyPricingCacheMetadata(null); From ca61df624291cd0bf448b81433fdaccc0e36e6a6 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 00:50:37 -0700 Subject: [PATCH 3/5] Run the suite in randomized order in CI and document the rule A file order that happens to work is not evidence of isolation; only running with --randomize catches a test that leans on another file's side effects. Fixing the leaks this uncovered is only durable if the check keeps running, so add a CI step alongside the canonical run and write the underlying rule into AGENTS.md. --- .github/workflows/ci.yml | 6 ++++++ AGENTS.md | 1 + 2 files changed, 7 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5ecac983..a04eb6d52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,3 +41,9 @@ jobs: # inference package (vendor/) is out of scope for this repo's CI. - name: Test run: bun run test + + # Catches tests that only pass because of the default file order (shared + # module-level state, an unrestored global mock, a leaked env var). The + # seed is fixed so a failure here reproduces locally with the same flag. + - name: Test (randomized order) + run: bun test ./src ./tests ./evals --randomize --seed 424242 diff --git a/AGENTS.md b/AGENTS.md index 0df98dcdb..db0668ccd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,6 +33,7 @@ When refactoring replaces an old path, delete the old one. No back-compat shims, - Add or update tests with every behavior change. - Bug fixes start with a failing test that reproduces the bug. Do not start by patching. - `tests/unit/` shared unit tests and helpers · co-located `src/**/*.test.ts` for module logic · `tests/fixtures/` fixture repos · `tests/integration/` reactor/permission harness. Planned: `tests/e2e/` (fixture-repo runs). +- A test must not depend on another file having run, or on the default file order. It must pass under `bun test ./src ./tests ./evals --randomize`. If a test mutates module-level state or calls `mock.module`, it must restore that state itself (`afterEach`/`afterAll`), not rely on the process happening to reset it. When capturing a module's real exports to restore later, shallow-copy them at capture time (`{ ...(await import(path)) }`) — Bun mutates the live namespace object in place when the module is mocked, so holding a bare reference to it silently turns into the mocked exports. ## Build & Validation From b014735a691a4407d6a385280b23005d217b9acf Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 01:19:31 -0700 Subject: [PATCH 4/5] Fix two more instances of the same live-namespace mock leak config.test.ts captured node:os with a static `import * as nodeOs` and later "restored" it with `mock.module("node:os", () => nodeOs)` -- the same defect as the dynamic-import case already fixed elsewhere in this branch, since Bun mutates that namespace object in place regardless of how it was imported. The capture is now a shallow copy taken before anything mocks node:os. state.test.ts mocked node:fs/promises with no restore at all. Its delegate functions were captured before the mock took effect so the leak was inert here, but the file still violated the isolation rule this branch exists to enforce, so it gets the same shallow-copy capture and an afterAll restore. --- src/session/state.test.ts | 13 +++++++++++-- tests/unit/config.test.ts | 10 ++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/session/state.test.ts b/src/session/state.test.ts index ed28ff369..8c2479aa9 100644 --- a/src/session/state.test.ts +++ b/src/session/state.test.ts @@ -1,9 +1,14 @@ -import { afterEach, beforeEach, expect, mock, test } from "bun:test"; -import * as realFs from "node:fs/promises"; +import { afterAll, afterEach, beforeEach, expect, mock, test } from "bun:test"; import { mkdir, rm } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; +// Bun mutates the imported namespace object in place when a module is +// mocked, so the capture is shallow-copied immediately -- holding onto the +// live namespace would turn into the mocked exports as soon as mock.module +// below runs, making a later restore a no-op. +const realFs = { ...(await import("node:fs/promises")) }; + // Simulates the straggler write's real await point (e.g. cycleRecorder.dispose // during the terminal path) landing its writeFile after a later-issued // terminal write's writeFile, so rename-order alone would let it win. @@ -20,6 +25,10 @@ mock.module("node:fs/promises", () => ({ }, })); +afterAll(() => { + mock.module("node:fs/promises", () => realFs); +}); + const { loadState, saveState } = await import("./state.js"); type RunState = Awaited>; diff --git a/tests/unit/config.test.ts b/tests/unit/config.test.ts index 622f3dee6..671517759 100644 --- a/tests/unit/config.test.ts +++ b/tests/unit/config.test.ts @@ -1,6 +1,12 @@ import { test, expect, mock } from "bun:test"; import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; import * as nodeOs from "node:os"; + +// Bun mutates the imported namespace object in place when a module is +// mocked, so `nodeOs` itself is not safe to hold onto across a mock.module +// call -- capture a shallow copy now, before anything mocks node:os, so the +// snapshot below still reads "real" after the mock/restore round-trip. +const realNodeOs = { ...nodeOs }; import { tmpdir } from "node:os"; import { join } from "node:path"; import { loadConfig } from "../../src/config/index.js"; @@ -226,7 +232,7 @@ test("loadConfig resolves an OAuth-profile provider absent from any settings fil // parameter — so the only way to point it at a synthetic auth store // without touching the real one is to stub node:os for the duration of // this call. - mock.module("node:os", () => ({ ...nodeOs, homedir: () => fakeHome })); + mock.module("node:os", () => ({ ...realNodeOs, homedir: () => fakeHome })); try { const { impl } = offlineFetch(); const config = await loadConfig( @@ -240,7 +246,7 @@ test("loadConfig resolves an OAuth-profile provider absent from any settings fil expect(config.providers.some((p) => p.name === "xai/synthetic")).toBe(true); } } finally { - mock.module("node:os", () => nodeOs); + mock.module("node:os", () => realNodeOs); } } finally { await rm(fakeHome, { recursive: true, force: true }); From 9e3b9db8cc318309b1cec7d0fad8e27e1bcf814e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 7 Aug 2026 01:19:42 -0700 Subject: [PATCH 5/5] Widen the isolation rule and add a rotating random-seed CI job The AGENTS.md wording only mentioned the dynamic-import capture shape, which reads as though that is the only form at risk -- it is not; a static `import * as ns` binding is mutated by mock.module the same way, and config.test.ts had exactly that bug. State the rule in terms of any live namespace binding instead of one idiom. The fixed-seed CI step is deterministic and worth keeping, but it only ever exercises one shuffle of the suite, so a leak that particular order does not disturb stays invisible forever. Add a nightly job with a fresh random seed each run, printed up front so a failure reproduces locally with the exact same --seed. --- .github/workflows/ci.yml | 36 ++++++++++++++++++++++++++++++++++++ AGENTS.md | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a04eb6d52..c4226318a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,11 @@ on: branches: [main] pull_request: workflow_dispatch: + schedule: + # Nightly, a fresh seed each run -- the fixed-seed step above only ever + # exercises one shuffle of the suite, so a leak that this particular + # order does not disturb would otherwise stay invisible forever. + - cron: "17 7 * * *" jobs: check: @@ -47,3 +52,34 @@ jobs: # seed is fixed so a failure here reproduces locally with the same flag. - name: Test (randomized order) run: bun test ./src ./tests ./evals --randomize --seed 424242 + + randomize-nightly: + if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Node + uses: actions/setup-node@v4 + with: + node-version: "24" + + - name: Setup Bun + uses: oven-sh/setup-bun@v2 + with: + bun-version: "1.3.14" + + - name: Install ripgrep + run: sudo apt-get install -y ripgrep + + - name: Install dependencies + run: bun install --frozen-lockfile + + # A fresh seed every run, printed up front so a failure here reproduces + # locally with the exact same `--seed` regardless of which shuffle hit it. + - name: Test (fresh random seed) + run: | + seed=$RANDOM$RANDOM + echo "seed=$seed" + bun test ./src ./tests ./evals --randomize --seed "$seed" diff --git a/AGENTS.md b/AGENTS.md index db0668ccd..9c445f7db 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,7 +33,7 @@ When refactoring replaces an old path, delete the old one. No back-compat shims, - Add or update tests with every behavior change. - Bug fixes start with a failing test that reproduces the bug. Do not start by patching. - `tests/unit/` shared unit tests and helpers · co-located `src/**/*.test.ts` for module logic · `tests/fixtures/` fixture repos · `tests/integration/` reactor/permission harness. Planned: `tests/e2e/` (fixture-repo runs). -- A test must not depend on another file having run, or on the default file order. It must pass under `bun test ./src ./tests ./evals --randomize`. If a test mutates module-level state or calls `mock.module`, it must restore that state itself (`afterEach`/`afterAll`), not rely on the process happening to reset it. When capturing a module's real exports to restore later, shallow-copy them at capture time (`{ ...(await import(path)) }`) — Bun mutates the live namespace object in place when the module is mocked, so holding a bare reference to it silently turns into the mocked exports. +- A test must not depend on another file having run, or on the default file order. It must pass under `bun test ./src ./tests ./evals --randomize`. If a test mutates module-level state or calls `mock.module`, it must restore that state itself (`afterEach`/`afterAll`), not rely on the process happening to reset it. When capturing a module's real exports to restore later, shallow-copy them (`{ ...moduleNamespace }`) at capture time, whether the namespace came from `await import(path)` or a static `import * as ns from "path"` — Bun mutates the live namespace object in place when the module is mocked, so holding a bare reference to it (either form) silently turns into the mocked exports. ## Build & Validation