feat: option to disable named tool choice#322
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a configuration flag to avoid sending named/object-form tool_choice to OpenAI-compatible APIs, improving compatibility with providers that only accept string tool_choice values (e.g., required).
Changes:
- Introduce
disableNamedToolChoiceinLLMConfig, with defaulting inparseLLMConfig. - Update
OpenAIClientto fall back totool_choice: "required"when named tool choice is disabled. - Expose the option in the extension settings UI and document it on the website.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/website/src/pages/docs/advanced/page-agent-core/page.tsx | Documents the new disableNamedToolChoice config option in the LLM config reference. |
| packages/llms/src/types.ts | Adds disableNamedToolChoice to LLMConfig (but the new JSDoc needs to match actual behavior). |
| packages/llms/src/index.ts | Ensures disableNamedToolChoice is normalized to false by default in parseLLMConfig. |
| packages/llms/src/OpenAIClient.ts | Implements the runtime behavior: use named tool choice only when enabled, otherwise use "required". |
| packages/extension/src/components/ConfigPanel.tsx | Adds a settings toggle to control disableNamedToolChoice. |
| packages/extension/src/agent/useAgent.ts | Plumbs the new toggle through extension config persistence (currently stored under advancedConfig). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * remove the tool_choice field from the request. | ||
| * @note fix "Invalid tool_choice type: 'object'" for some LLMs. |
There was a problem hiding this comment.
The JSDoc for disableNamedToolChoice is misleading: it says this option removes the tool_choice field, but the implementation keeps tool_choice and uses the string value "required" (and only disables the named/object form when toolChoiceName is provided). Please update the comment to accurately describe the behavior and when it applies.
| * remove the tool_choice field from the request. | |
| * @note fix "Invalid tool_choice type: 'object'" for some LLMs. | |
| * Disable the named/object tool_choice form when forcing a specific tool. | |
| * When used together with `toolChoiceName`, the client sends `tool_choice: "required"` | |
| * instead of `{ type: "function", function: { name: toolChoiceName } }`, keeping the | |
| * `tool_choice` field but avoiding the object form that some LLMs reject with | |
| * errors like "Invalid tool_choice type: 'object'". |
| experimentalLlmsTxt, | ||
| disableNamedToolChoice, | ||
| ...llmConfig | ||
| }: ExtConfig) => { | ||
| await chrome.storage.local.set({ llmConfig }) |
There was a problem hiding this comment.
disableNamedToolChoice is part of LLMConfig, but it’s destructured out here and therefore not included in the llmConfig object persisted to storage. That duplicates config concerns (it then gets stored under advancedConfig) and means anything reading only llmConfig will miss the flag. Consider keeping disableNamedToolChoice within llmConfig and out of AdvancedConfig so it persists with the other LLM settings.
What
Some LLMs do not support named
tool_choiceasin
Give an option to use "required" as
tool_choiceType
Testing
Closes
Requirements / 要求