Agent JS-fication (1.5.0)#1674
Conversation
🦋 Changeset detectedLatest commit: 57c1634 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 |
…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>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b85bfea91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const inner of tool.tools) { | ||
| addTool(inner); |
There was a problem hiding this comment.
For a dynamic Toolset.create({ tools: () => client.getTools(), setup: ... }) as documented above, this loop snapshots tool.tools into _functionToolsMap when the ToolContext is constructed, before AgentActivity.setupToolsets() runs. If setup() connects/discovers tools, the later realtime/session updates and tool execution still read the stale map, so newly discovered tools are never advertised or executable unless the caller manually calls updateTools() again.
Useful? React with 👍 / 👎.
| /** Code interpreter container ID or configuration. */ | ||
| readonly container: OpenAI.Responses.Tool.CodeInterpreter['container'] | null; | ||
|
|
||
| constructor({ container = null }: CodeInterpreterOptions = {}) { |
There was a problem hiding this comment.
When callers use new CodeInterpreter() (the advertised no-arg constructor), container defaults to null and toToolConfig() omits it, but the OpenAI Responses Code Interpreter tool requires a container value (for example { type: 'auto' } or a container id). As a result, enabling this tool with the default options produces a request that the API rejects before the model can run code.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| get providerTools(): ProviderTool[] { | ||
| return this._providerTools; | ||
| } |
There was a problem hiding this comment.
🟡 providerTools getter returns mutable internal array instead of a defensive copy
The providerTools getter at agents/src/llm/tool_context.ts:371-373 returns this._providerTools directly, despite the JSDoc saying "A copy of all provider tools". Every other collection getter on ToolContext returns a defensive copy: functionTools creates a new object via Object.fromEntries() (line 367), toolsets spreads into a new array [...this._toolsets] (line 377), and tools spreads into [...this._tools] (line 384). Since providerTools returns the raw internal array, any caller that mutates the returned list (e.g. .push(), .splice()) would silently corrupt the ToolContext's internal state, affecting equals() comparisons, flatten() results, and hasTool() lookups.
| get providerTools(): ProviderTool[] { | |
| return this._providerTools; | |
| } | |
| get providerTools(): ProviderTool[] { | |
| return [...this._providerTools]; | |
| } |
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; | ||
| } |
There was a problem hiding this comment.
🔴 onlySingleType guard is unreachable when function tools and provider tools coexist
In toToolsConfig, the error at line 204 ('Gemini does not support mixing function tools and provider tools') fires before the onlySingleType early-return at line 208 can take effect. The Google Chat LLM at plugins/google/src/llm.ts:361 calls toToolsConfig({ ..., onlySingleType: true }) specifically to handle the case where function tools should be preferred over provider tools. But if a user constructs a ToolContext with both function tools and GeminiTool provider tools (e.g. new ToolContext([tool({name:'foo',...}), new GoogleSearch()])), the Chat LLM will throw instead of gracefully returning just the function tools. The onlySingleType check at line 208 should be moved before the mixing error at line 204, so it can short-circuit and return function tools only.
| 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; | |
| } | |
| if (onlySingleType && tools.length > 0) { | |
| return tools; | |
| } | |
| if (tools.length > 0 && providerTools.length > 0) { | |
| throw new Error('Gemini does not support mixing function tools and provider tools'); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
No description provided.