fix(voice): preserve toolsets during LLM tool sync#1752
fix(voice): preserve toolsets during LLM tool sync#1752rosetta-livekit-bot[bot] wants to merge 1 commit into
Conversation
🦋 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| _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.
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.