fix(testing): isolate withMockTools per async context (AsyncLocalStorage)#1865
fix(testing): isolate withMockTools per async context (AsyncLocalStorage)#1865toubatbrian wants to merge 5 commits into
Conversation
…age) Replace the module-level `activeMockTools` global with an `AsyncLocalStorage`-backed registry so overlapping/concurrent async tests no longer clobber each other's mock maps. Mirrors Python's per-async-context `ContextVar`. The public `using withMockTools(...)` Disposable API is unchanged; tests now read the active registry via `getActiveMockTools()`. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 225520a The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…calls Co-authored-by: Cursor <cursoragent@cursor.com>
…ler-leak Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Cursor <cursoragent@cursor.com>
| return { | ||
| [Symbol.dispose]() { | ||
| activeMockTools = previous; | ||
| mockToolsStorage.enterWith(previous as MockToolsMap); |
There was a problem hiding this comment.
🟡 Unsafe cast of undefined to MockToolsMap in dispose can pass undefined to enterWith
When withMockTools is called without a pre-existing store, previous is undefined. In the Symbol.dispose handler at line 1037, mockToolsStorage.enterWith(previous as MockToolsMap) passes undefined (cast to MockToolsMap) to enterWith. While AsyncLocalStorage<T>.enterWith() expects a value of type T (here MockToolsMap), it receives undefined. At runtime this happens to work because getStore() returns undefined in both cases, but it violates the enterWith API contract and could break if Node.js ever adds runtime validation. The fix should use mockToolsStorage.disable() when previous is undefined, or change the storage type to AsyncLocalStorage<MockToolsMap | undefined>.
| mockToolsStorage.enterWith(previous as MockToolsMap); | |
| if (previous) { | |
| mockToolsStorage.enterWith(previous); | |
| } else { | |
| mockToolsStorage.disable(); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| it('routes the activity-loop tool execution to a mock installed in the test body', async () => { | ||
| realToolRan = false; | ||
| mockRan = false; | ||
|
|
||
| using _mock = withMockTools(ProbeAgent, { | ||
| theTool: () => { | ||
| mockRan = true; | ||
| return 'MOCKED'; | ||
| }, | ||
| }); | ||
|
|
||
| const result = session.run({ userInput: 'order' }); | ||
| await result.wait(); | ||
|
|
||
| result.expect.containsFunctionCall({ name: 'theTool' }); | ||
| expect(mockRan).toBe(true); | ||
| expect(realToolRan).toBe(false); | ||
| // The tool output is JSON-serialized, so the raw string 'MOCKED' surfaces as '"MOCKED"'. | ||
| result.expect.containsFunctionCallOutput({ output: '"MOCKED"' }); | ||
| }, 30_000); |
There was a problem hiding this comment.
🚩 Mock visibility in activity loop depends on async context inheritance from session.run()
The test at run_result.test.ts:280-299 expects mocks installed via enterWith in the test body to be visible during tool execution in the agent activity loop. This works because session.run() (agent_session.ts:1155) spawns an async IIFE synchronously from the test body's execution context, inheriting the mock store. The IIFE then calls generateReply → createSpeechTask → Task.from which starts the task in the same inherited context. In generation.ts:1171, getMockTool() reads the store. However, this relies on the entire chain NOT introducing an async boundary that breaks the inheritance before getMockTool is called. If the internal scheduling changes (e.g., dispatching through an event emitter or setTimeout), the mock visibility could break silently.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Problem
The
withMockToolstest utility stored its active mock registry in a module-level mutable global (export let activeMockToolsinagents/src/voice/testing/run_result.ts).getMockToolread that global (consumed ingeneration.ts), andwithMockToolsset/restored it on enter/[Symbol.dispose].Because the registry was a single shared global, overlapping or concurrent async tests could clobber each other's mock maps — there was no per-async-context isolation. Python avoids this with a per-async-context
ContextVar(_MockToolsContextVarinlivekit-agents/.../voice/run_result.py).Fix
Move the registry into a module-level
AsyncLocalStorage<MockToolsMap>— the JS analog of Python'sContextVar, and already the pattern used elsewhere in this repo (agentActivityStorage,functionCallStorage,speechHandleStorage).getMockToolnow readsmockToolsStorage.getStore()(same lookup logic).withMockTools(agent, mocks)computesprevious = getStore(), builds the merged map, and callsenterWith(updated);[Symbol.dispose]()restores viaenterWith(previous). UsingenterWithpreserves the synchronous enter/exit ergonomics of theusingdeclaration while giving per-async-context isolation.using withMockTools(...)Disposable API is unchanged.activeMockToolsbinding is removed; tests now use a new/** @internal */ getActiveMockTools()accessor.Tests
run_result.test.tscases pass with thegetActiveMockTools()accessor (they read the active map synchronously right afterwithMockToolsin the same context —enterWithmakes that work).withMockToolsinherits the registry.Validation
pnpm build:agents— success.pnpm --filter @livekit/agents exec vitest run src/voice/testing/run_result.test.ts— 8/8 pass.pnpm --filter @livekit/agents exec vitest run src/voice/generation_tools.test.ts— 10/10 pass.prettier --checkon changed files — clean.@livekit/agentspatch).Known pre-existing noise unrelated to this change:
amd.test.ts,api:checkexport * as, and a fresh-worktree eslint duplicate-plugin env error.