Skip to content

fix: add SDK mock to worker-mcp test to survive module isolation#992

Open
lsm wants to merge 1 commit intodevfrom
fix/sdk-mock-module-isolation
Open

fix: add SDK mock to worker-mcp test to survive module isolation#992
lsm wants to merge 1 commit intodevfrom
fix/sdk-mock-module-isolation

Conversation

@lsm
Copy link
Copy Markdown
Owner

@lsm lsm commented Mar 26, 2026

Problem

Bun's module isolation between test files invalidates the mock.module() from setup.ts. When room-runtime-service-worker-mcp.test.ts uses dynamic await import(...) to load source modules that transitively import createSdkMcpServer from @anthropic-ai/claude-agent-sdk, the real module gets resolved before the preload mock is re-applied, causing SyntaxError on CI (Ubuntu) where SDK 0.2.81 doesn't always export createSdkMcpServer at the ESM level.

This causes 8 test failures and 46 "Unhandled error between tests" SyntaxErrors in the unit test suite.

Fix

Re-declare the SDK mock at the top of room-runtime-service-worker-mcp.test.ts so it survives Bun's module isolation between test files.

Scope

Single file change: packages/daemon/tests/unit/room/room-runtime-service-worker-mcp.test.ts

Bun's module isolation between test files can invalidate the
mock.module() from setup.ts. When room-runtime-service-worker-mcp.test.ts
uses dynamic `await import(...)` to load source modules that transitively
import createSdkMcpServer from the SDK, the real module gets resolved
before the preload mock is re-applied, causing SyntaxError on CI where
SDK 0.2.81 doesn't always export createSdkMcpServer at the ESM level.

Re-declare the SDK mock at the top of this test file so it survives
Bun's module isolation between test files.
Copy link
Copy Markdown
Owner Author

@lsm lsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by glm-5-turbo (GLM)

Model: glm-5-turbo | Client: NeoKai | Provider: GLM

Verdict: APPROVE

The fix is correct and necessary.

Analysis

Why the bug occurs: The test file uses dynamic await import(...) to load room-runtime-service.ts, which transitively imports createRoomAgentMcpServer from room-agent-tools.ts. That function calls createSdkMcpServer from the SDK. On CI where SDK 0.2.81 does not reliably export createSdkMcpServer at the ESM level, the real module gets resolved before the preload-level mock from setup.ts is re-applied, causing a SyntaxError.

Why the fix works: Re-declaring mock.module('@anthropic-ai/claude-agent-sdk', ...) at the top of the test file (lines 24-40) ensures the mock is in place before any dynamic import in that file triggers SDK resolution. This follows the same pattern already used in session-lifecycle-sdk-title.test.ts and explained in its comment: "Only mock.module calls for EXTERNAL packages are placed at the top level."

Mock correctness:

  • Returns createSdkMcpServer with the minimal shape required by createRoomAgentMcpServer (returns { type: 'sdk', name, version, tools, instance: { connect, disconnect } }) — adequate for the test's spy-based assertions
  • query, interrupt, supportedModels, tool all return appropriate stubs
  • Comment at lines 18-23 clearly documents the purpose and failure mode

Test results: All 8 tests pass cleanly (128.00ms, 0 failures).

No issues found.


P0: 0 | P1: 0 | P2: 0 | P3: 0

Copy link
Copy Markdown
Owner Author

@lsm lsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by claude-opus-4-6 (Anthropic)

Model: claude-opus-4-6 | Client: NeoKai | Provider: Anthropic

Verdict: APPROVE

The fix is correct and minimal. No issues found.


Analysis

Root cause (correct): Bun's test runner isolates module state between test files even when they share a --preload setup file. When room-runtime-service-worker-mcp.test.ts uses await import('../../../src/lib/room/runtime/room-runtime-service') (line 133 of the test), it triggers a transitive import chain: room-runtime-serviceroom-agent-tools@anthropic-ai/claude-agent-sdk. If the real SDK (v0.2.81 on CI) has already been resolved by a prior test file in the same process, the preload mock from setup.ts is not re-applied before the dynamic import resolves, causing a SyntaxError.

Fix correctness (correct): mock.module() in Bun is hoisted to the top of the module evaluation order, so placing it after the static type imports (lines 14-16) but before the buildSessionFactory helper (which contains the first dynamic import) guarantees the mock is active before any import chain can resolve the real SDK.

Mock completeness (sufficient): The mock provides all symbols that any transitive source module could need:

  • query, interrupt, supportedModels — agent session SDK surface
  • createSdkMcpServer — used by room-agent-tools.ts:1240, planner-agent.ts:568, leader-agent.ts:614
  • tool — used by room-agent-tools.ts, planner-agent.ts, leader-agent.ts

The createSdkMcpServer mock returns the expected { type: 'sdk', name, version, tools, instance: { connect, disconnect } } shape. The tools parameter is not captured because the test never exercises createRoomAgentMcpServer's internals — it only verifies that setRuntimeMcpServers is called with the correct merged map — but this is fine; unused parameters being discarded is the intended behavior.

Scope (minimal): Single-file change, no side effects on other test files. The other 9 files that import room-runtime-service use static imports at module load time, where the preload mock from setup.ts is already in effect. Only this file (which uses await import(...) inside helper functions) needs the re-declaration.


P0/P1/P2/P3

  • P0: 0
  • P1: 0
  • P2: 0
  • P3: 0

No issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant