fix(voice): preserve toolsets during LLM tool sync - #1752
rosetta-livekit-bot[bot] wants to merge 11 commits into
Conversation
…1525) Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com> Co-authored-by: u9g <jason.lernerman@livekit.io>
Agent.llmNode now returns ReadableStream<ChatChunk | string | FlushSentinel>, but the agent_v2 hook overrides and AgentHookAdapter still declared the narrower ChatChunk | string union, so passing super.llmNode as the fallback failed to type-check. Widen the override return types and the adapter's fallback/return signatures to include FlushSentinel. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Brian Yin <brian.yin@livekit.io> Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com> Co-authored-by: u9g <jason.lernerman@livekit.io>
Catch end-call close listener errors to avoid unhandled rejections during shutdown, and make public tool type guards return false for null inputs.
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
Co-authored-by: rosetta-livekit-bot[bot] <282703043+rosetta-livekit-bot[bot]@users.noreply.github.com>
🦋 Changeset detectedLatest commit: c3880bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 34 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 |
| _syncFlattened( | ||
| tools: readonly Tool[], | ||
| structuredTools: readonly ToolContextEntry<UserData>[] = this._tools, | ||
| ): void { | ||
| const current = new ToolContext<UserData>(structuredTools).flatten(); | ||
| const currentTools = new Set(current); | ||
| const nextTools = new Set(tools); | ||
| const added = tools.filter((tool) => !currentTools.has(tool)); | ||
| const removed = current.filter((tool) => !nextTools.has(tool)); | ||
|
|
||
| const structured = structuredTools.filter((tool) => !removed.some((r) => tool === r)); | ||
| this._updateTools([...structured, ...(added as ToolContextEntry<UserData>[])], removed); | ||
| } |
There was a problem hiding this comment.
🚩 _tools inconsistency with _functionToolsMap after exclusion-based sync
After _syncFlattened excludes Toolset members (the removal scenario), _tools still contains Toolsets whose .tools arrays include the excluded members, but _functionToolsMap does not. This means new ToolContext(toolCtx.tools) would re-include excluded tools and new ToolContext(toolCtx.tools).equals(toolCtx) would return false. This inconsistency is explicitly documented in the _syncFlattened docstring ("removed Toolset members stay owned by their toolset for lifecycle purposes, but are excluded from this context's callable/provider lookups"). However, it could interact with onToolsetToolsChanged at agents/src/voice/agent_activity.ts:4210 — that method creates new ToolContext(current.tools) and checks .equals(current), so it would detect a difference and call updateTools(current.tools), potentially re-adding tools that were intentionally excluded. This is only triggered when a dynamic Toolset pushes new tools, which is unlikely to happen during active LLM inference, but worth being aware of.
Was this helpful? React with 👍 or 👎 to provide feedback.
| _syncFlattened( | ||
| tools: readonly Tool[], | ||
| structuredTools: readonly ToolContextEntry<UserData>[] = this._tools, | ||
| ): void { | ||
| const current = new ToolContext<UserData>(structuredTools).flatten(); | ||
| const currentTools = new Set(current); | ||
| const nextTools = new Set(tools); | ||
| const added = tools.filter((tool) => !currentTools.has(tool)); | ||
| const removed = current.filter((tool) => !nextTools.has(tool)); | ||
|
|
||
| const structured = structuredTools.filter((tool) => !removed.some((r) => tool === r)); | ||
| this._updateTools([...structured, ...(added as ToolContextEntry<UserData>[])], removed); | ||
| } |
There was a problem hiding this comment.
🚩 No test coverage for _syncFlattened
The new _syncFlattened method has no unit tests in tool_context.test.ts. The method handles non-trivial logic (identity-based diff, Toolset preservation, exclusion mechanism) and would benefit from test cases covering: (1) no-op when tools unchanged, (2) tool added by LLM node, (3) tool removed from a Toolset, (4) top-level tool removed, (5) tool replaced with different instance same name. The existing test suite thoroughly covers ToolContext and Toolset behavior but doesn't exercise _syncFlattened or the exclude parameter of _updateTools.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private async setupToolsetList(toolsets: readonly Toolset[]): Promise<void> { | ||
| const outputs = await Promise.allSettled( | ||
| toolsets.map((ts) => | ||
| ts.setup({ | ||
| // A dynamic toolset pushes a changed tool list here; re-flatten and re-advertise it. | ||
| updateTools: (tools) => { | ||
| ts._setTools(tools); | ||
| void this.onToolsetToolsChanged().catch((error) => | ||
| this.logger.error({ error }, 'error re-advertising toolset tools'), | ||
| ); | ||
| }, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🚩 Dynamic toolset pushes can race if multiple pushes arrive before the first settles
In agents/src/voice/agent_activity.ts:4220-4224, when a toolset pushes new tools via ToolsetContext.updateTools, the handler calls ts._setTools(tools) synchronously and then fires void this.onToolsetToolsChanged().catch(...) as fire-and-forget. If a toolset pushes tools multiple times in rapid succession (e.g. an MCP server emitting tool_list_changed events), multiple onToolsetToolsChanged calls can overlap. Each call goes through this.updateTools(current.tools) which acquires no lock. The updateTools method itself does setupToolsetList and closeToolsetList for added/removed toolsets, but since the same toolset entries are being re-used (same toolset, different inner tools), the setup/close steps should be no-ops. The final state should converge correctly, but intermediate states could briefly advertise stale tool lists to the realtime session.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (tools.length > 0 && providerTools.length > 0) { | ||
| throw new Error('Gemini does not support mixing function tools and provider tools'); | ||
| } | ||
|
|
||
| if (onlySingleType && tools.length > 0) { | ||
| return tools; | ||
| } | ||
|
|
||
| tools.push(...providerTools); | ||
|
|
||
| return tools.length > 0 ? tools : undefined; | ||
| } |
There was a problem hiding this comment.
🚩 Google LLM plugin's onlySingleType flag silently drops provider tools when function tools are present
In plugins/google/src/utils.ts:204-208, when onlySingleType is true and function tools exist, the function returns early with only the function tools array, silently discarding any provider tools (GeminiTool instances). This is called from plugins/google/src/llm.ts:362-366 for the non-realtime LLM path. The error at line 204 ('Gemini does not support mixing function tools and provider tools') only fires when both are present and onlySingleType is false (the realtime path). So for the non-realtime LLM, if a user passes both function tools and Gemini provider tools (e.g. GoogleSearch), the provider tools are silently ignored rather than raising an error. This asymmetry between the realtime and non-realtime paths could surprise users.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Testing
Notes
Ported from livekit/agents#6049
Original PR description
Two related fixes for async toolset (
AsyncToolset) routing/teardown around agent handoff.1. Session-scoped AsyncToolset cancelled at handoff (primary)
A session-scoped
AsyncToolset(AgentSession(tools=[...])) should survive an agent handoff: a long-runningctx.update()tool started under agent A keeps running and delivers under agent B. In the pipeline path it didn't — the in-flight tool was cancelled at handoff and its deferred result lost.Root cause: the tool->executor routing map was built from
tool_ctx.toolsets. But the LLM inference step shares the sametool_ctxand runs first; to hand the model a flat function list it flattens the toolsets and writes the flat list back intotool_ctxin place (_llm_inference_task:tools = tool_ctx.flatten()thentool_ctx.update_tools(tools)). After thattool_ctx.toolsetsis empty, so a session-scoped tool was routed to the activity executor and cancelled when the activity drained on handoff.Fix: build the routing map from the registered toolsets directly (
session.tools+ the current agent's tools + MCP), which still carry theAsyncToolsetgrouping, instead of the post-inferencetool_ctx.2. Agent-scoped AsyncToolset hard-cancelled at handoff
Exposed by fix #(1) (and flagged in review). Agent-scoped
AsyncToolsets (Agent(tools=[...])) run on their own executors, butaclose()onlydrain()ed the activity executor; the agent toolset executors wereaclose()d (cancel-all). So a non-cancellable agent-scoped async tool that had returned from its firstctx.update()was hard-cancelled at handoff instead of being awaited, losing its result.Fix: drain the agent-scoped
AsyncToolsetexecutors alongside the activity executor (cancel cancellable, await non-cancellable) before the aclose() sweep. Session-scoped toolsets are owned by the session and intentionally not drained here.Verification
Reproduced and verified end-to-end with
agents-cliagainst real inference (text + voice):CANCELLEDat handoff; after, it logsCOMPLETED(awaited to completion).This matches the JS implementation, which keeps
toolCtx.toolsetsintact and routes off it. A deeper follow-up could stop the inference step from destructively flattening the sharedtool_ctxin place; these fixes address routing/teardown at the consumer with minimal blast radius.