From a33fb7c4e6b5022ec597c8e4274c05ebc5b743df Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Thu, 6 Aug 2026 01:45:43 -0300 Subject: [PATCH] fix(mcp): give the audit tests a loader seam createRequire cannot hide from (#9559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): give the audit tests a loader seam createRequire cannot hide from Since #8959 the audit DB loads better-sqlite3 via createRequire() (so Electron/global-install resolution works) — which vi.doMock cannot intercept: it only patches Vitest's ESM module graph. The audit.test.ts better-sqlite3 mock therefore never engaged; the tests opened a REAL empty sqlite file in the temp DATA_DIR ('no such table: mcp_tool_audit' on stderr) and every mock assertion counted zero calls. The 3 failures are deterministic (reproduced 3/3 locally), redding Vitest (fast-path) for the entire PR queue — long misdiagnosed as a flake (#9095 merge notes call it 'pre-existing audit.test.ts flake'). - Shutdown tests inject the mock through the audit connection cache (globalThis.__omnirouteMcpAuditDb) — the module's own seam. - The node:sqlite fallback test drives __setBetterSqliteLoaderForTests, a test-only loader override; the production createRequire path is untouched (node:sqlite itself is import()'d, so its doMock still works). 3/3 red -> 3/3 green; full open-sse/mcp-server vitest suite 88/88. * chore: align changelog slug with the PR number (9559) --------- Co-authored-by: diegosouzapw --- .../fixes/9559-mcp-audit-vitest-seam.md | 1 + open-sse/mcp-server/__tests__/audit.test.ts | 69 +++++++++---------- open-sse/mcp-server/audit.ts | 24 +++++-- 3 files changed, 52 insertions(+), 42 deletions(-) create mode 100644 changelog.d/fixes/9559-mcp-audit-vitest-seam.md diff --git a/changelog.d/fixes/9559-mcp-audit-vitest-seam.md b/changelog.d/fixes/9559-mcp-audit-vitest-seam.md new file mode 100644 index 0000000000..2117839f0b --- /dev/null +++ b/changelog.d/fixes/9559-mcp-audit-vitest-seam.md @@ -0,0 +1 @@ +- **fix(mcp):** the 3 `audit.test.ts` shutdown/fallback tests fail deterministically since #8959 switched the audit DB loader to `createRequire("better-sqlite3")` — `vi.doMock` only patches Vitest's ESM module graph, so the old better-sqlite3 mock never engaged and the tests hit a real empty sqlite file ("no such table: mcp_tool_audit"), redding the `Vitest (fast-path)` job on every PR (long misdiagnosed as a flake). Shutdown tests now inject the mock through the audit connection cache (`globalThis.__omnirouteMcpAuditDb`), and the node:sqlite fallback test drives a new test-only loader seam (`__setBetterSqliteLoaderForTests`) — the production `createRequire` path is unchanged. 3/3 red → 3/3 green; full `open-sse/mcp-server` vitest suite 88/88. diff --git a/open-sse/mcp-server/__tests__/audit.test.ts b/open-sse/mcp-server/__tests__/audit.test.ts index 19f69ffb8d..829acaf4af 100644 --- a/open-sse/mcp-server/__tests__/audit.test.ts +++ b/open-sse/mcp-server/__tests__/audit.test.ts @@ -18,6 +18,13 @@ function createStatementMock() { }; } +// #8959 made the production loader use createRequire() (Electron/global-install +// resolution), which vi.doMock CANNOT intercept — it only patches Vitest's ESM +// module graph. The old better-sqlite3 doMock therefore never engaged: the code +// opened a REAL sqlite file in the temp DATA_DIR ("no such table" on stderr) +// and every mock assertion counted 0 calls. The shutdown tests now inject the +// mock through the audit connection cache (globalThis.__omnirouteMcpAuditDb), +// and the fallback test uses the __setBetterSqliteLoaderForTests seam. describe("MCP audit shutdown", () => { let dataDir: string; let dbFile: string; @@ -46,15 +53,10 @@ describe("MCP audit shutdown", () => { close: vi.fn(), open: true, }; - const MockDatabase = vi.fn(function MockDatabase() { - return mockDb; - }); - - vi.doMock("better-sqlite3", () => ({ - default: MockDatabase, - })); const audit = await import("../audit.ts"); + // Inject through the connection cache — the seam the module itself uses. + globalThis.__omnirouteMcpAuditDb = mockDb as unknown as typeof globalThis.__omnirouteMcpAuditDb; await audit.logToolCall("omniroute_get_health", { ok: true }, { ok: true }, 12, true); expect(mockDb.prepare).toHaveBeenCalledTimes(1); @@ -80,15 +82,9 @@ describe("MCP audit shutdown", () => { close: vi.fn(), open: true, }; - const MockDatabase = vi.fn(function MockDatabase() { - return mockDb; - }); - - vi.doMock("better-sqlite3", () => ({ - default: MockDatabase, - })); const audit = await import("../audit.ts"); + globalThis.__omnirouteMcpAuditDb = mockDb as unknown as typeof globalThis.__omnirouteMcpAuditDb; await audit.logToolCall("omniroute_get_health", {}, {}, 5, true); expect(audit.closeAuditDb()).toBe(true); @@ -103,26 +99,16 @@ describe("MCP audit shutdown", () => { // Simulate a global-install scenario where the bundled native binary // never landed in dist/node_modules/better-sqlite3/build/Release/. + // Thrown from the loader seam because the real load path is + // createRequire("better-sqlite3"), unreachable by vi.doMock. const bindingErr = new Error( "Could not locate the bindings file. Tried: …/better_sqlite3.node" ) as Error & { code?: string }; bindingErr.code = "MODULE_NOT_FOUND"; - // Simulate the binding-missing failure as the better-sqlite3 default - // constructor throwing — this matches reality (`new Database()` throws - // "Could not locate the bindings file" when the prebuilt .node is absent) - // and reaches the adapter's `catch (nativeErr)`. A factory that itself - // throws is reported by vitest as a mock-setup error and never reaches - // the code under test. - const ThrowingDatabase = vi.fn(function ThrowingDatabase() { - throw bindingErr; - }); - vi.doMock("better-sqlite3", () => ({ - default: ThrowingDatabase, - })); - // node:sqlite's DatabaseSync does not expose a boolean `open` property, - // so the mock intentionally omits it — the adapter tracks open state in - // a local closure and exposes it via a getter. + // node:sqlite IS loaded via dynamic import(), so doMock works for it. + // Its DatabaseSync does not expose a boolean `open` property — the + // adapter tracks open state in a local closure. const mockNodeDb = { prepare: vi.fn(() => createStatementMock()), exec: vi.fn(), @@ -134,17 +120,24 @@ describe("MCP audit shutdown", () => { vi.doMock("node:sqlite", () => ({ DatabaseSync })); const audit = await import("../audit.ts"); + audit.__setBetterSqliteLoaderForTests(() => { + throw bindingErr; + }); - await audit.logToolCall("omniroute_get_health", { ok: true }, { ok: true }, 4, true); - expect(DatabaseSync).toHaveBeenCalledWith(dbFile); - expect(mockNodeDb.prepare).toHaveBeenCalled(); + try { + await audit.logToolCall("omniroute_get_health", { ok: true }, { ok: true }, 4, true); + expect(DatabaseSync).toHaveBeenCalledWith(dbFile); + expect(mockNodeDb.prepare).toHaveBeenCalled(); - expect(audit.closeAuditDb()).toBe(true); - expect(mockNodeDb.exec).toHaveBeenCalledWith("PRAGMA wal_checkpoint(TRUNCATE)"); - expect(mockNodeDb.close).toHaveBeenCalledTimes(1); + expect(audit.closeAuditDb()).toBe(true); + expect(mockNodeDb.exec).toHaveBeenCalledWith("PRAGMA wal_checkpoint(TRUNCATE)"); + expect(mockNodeDb.close).toHaveBeenCalledTimes(1); - // Cache is cleared after close, so a second close is a no-op. - expect(audit.closeAuditDb()).toBe(false); - expect(mockNodeDb.close).toHaveBeenCalledTimes(1); + // Cache is cleared after close, so a second close is a no-op. + expect(audit.closeAuditDb()).toBe(false); + expect(mockNodeDb.close).toHaveBeenCalledTimes(1); + } finally { + audit.__setBetterSqliteLoaderForTests(null); + } }); }); diff --git a/open-sse/mcp-server/audit.ts b/open-sse/mcp-server/audit.ts index da1e4eaf8f..a658d3af7f 100644 --- a/open-sse/mcp-server/audit.ts +++ b/open-sse/mcp-server/audit.ts @@ -206,11 +206,27 @@ function toString(value: unknown): string { return typeof value === "string" ? value : ""; } +/** + * Test-only seam: the production load path uses `createRequire()` (so the + * Electron/global-install resolution works — #8959), which `vi.doMock` cannot + * intercept (it only patches Vitest's ESM module graph). Tests inject a + * throwing/mocked loader here to exercise the node:sqlite fallback. + */ +let betterSqliteLoaderForTests: (() => unknown) | null = null; +export function __setBetterSqliteLoaderForTests(loader: (() => unknown) | null): void { + betterSqliteLoaderForTests = loader; +} + async function openBetterSqliteAuditDb(dbPath: string): Promise { - const { createRequire } = await import("node:module"); - const _require = createRequire(import.meta.url); - const mod = _require("better-sqlite3"); - const Database = (mod?.default || mod) as unknown as new ( + let mod: unknown; + if (betterSqliteLoaderForTests) { + mod = betterSqliteLoaderForTests(); + } else { + const { createRequire } = await import("node:module"); + const _require = createRequire(import.meta.url); + mod = _require("better-sqlite3"); + } + const Database = ((mod as { default?: unknown })?.default || mod) as unknown as new ( dbPath: string ) => AuditDatabase; return new Database(dbPath);