tool can give alternative definition by model #1400
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds model-aware alternativeDefinition support so tools can provide different LanguageModelToolInformation depending on the selected model.
- Extends ICopilotTool.alternativeDefinition to accept the current model.
- Applies alternativeDefinition in three getEnabledTools implementations before filtering.
- Updates interface documentation to mention model-based customization.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/base/extHostContext/simulationExtHostToolsService.ts | Adds model-aware mapping to substitute tool definitions before filtering. |
| src/extension/tools/vscode-node/toolsService.ts | Introduces model-based alternativeDefinition application prior to existing enable/disable logic. |
| src/extension/tools/node/test/testToolsService.ts | Mirrors production logic for model-driven alternativeDefinition in test service. |
| src/extension/tools/common/toolsRegistry.ts | Updates ICopilotTool interface and docs to accept model info in alternativeDefinition. |
| return this.tools | ||
| .map(tool => { | ||
| // Apply model-specific alternative if available via alternativeDefinition | ||
| const owned = this.copilotTools.get(getToolName(tool.name) as ToolName); | ||
| if (owned?.alternativeDefinition) { | ||
| const alternative = owned.alternativeDefinition(request.model); | ||
| if (alternative) { | ||
| return alternative; | ||
| } | ||
| } | ||
| return tool; | ||
| }) | ||
| .filter(tool => filter?.(tool) ?? (!this._disabledTools.has(getToolName(tool.name)) && packageJsonTools.has(tool.name))); |
There was a problem hiding this comment.
[nitpick] The model-specific alternativeDefinition mapping logic is duplicated in three services; extracting a shared helper (e.g., applyAlternativeToolDefinition(tools, request, lookupFn)) would reduce repetition and risk of divergence if the logic changes.
Copilot uses AI. Check for mistakes.
| return this.tools | ||
| .map(tool => { | ||
| // Apply model-specific alternative if available via alternativeDefinition | ||
| const owned = this._copilotTools.get(getToolName(tool.name) as ToolName); | ||
| if (owned?.value?.alternativeDefinition) { | ||
| const alternative = owned.value.alternativeDefinition(request.model); | ||
| if (alternative) { | ||
| return alternative; | ||
| } | ||
| } | ||
| return tool; | ||
| }) | ||
| .filter(tool => { |
There was a problem hiding this comment.
[nitpick] This mapping block repeats the same alternativeDefinition substitution pattern found in other services; consider consolidating into a reusable utility to minimize duplicated logic and simplify future changes.
Copilot uses AI. Check for mistakes.
| * can be driven by EXP for example, or customized based on the current model. | ||
| * ⚠️ A tool using an alternative definition MUST still accept its default | ||
| * parameters because the alternative definition will only be applied within | ||
| * the Copilot extension, not other extensions' usages via `vscode.lm.tools`. | ||
| * | ||
| * @param modelInfo Optional information about the currently selected language model. | ||
| * If provided, allows customizing the tool definition per model. | ||
| */ | ||
| alternativeDefinition?(): vscode.LanguageModelToolInformation | undefined; | ||
| alternativeDefinition?(modelInfo?: vscode.LanguageModelChat): vscode.LanguageModelToolInformation | undefined; |
There was a problem hiding this comment.
[nitpick] The documentation specifies parameter acceptance but does not clarify expectations about immutable fields (e.g., tool name); if name or signature changes are disallowed, explicitly stating that the alternativeDefinition must preserve the original tool's name and parameter schema would prevent misuse.
Copilot uses AI. Check for mistakes.
| // Apply model-specific alternative if available via alternativeDefinition | ||
| const owned = this._copilotTools.value.get(getToolName(tool.name) as ToolName); | ||
| if (owned?.alternativeDefinition) { | ||
| const alternative = owned.alternativeDefinition(endpoint); |
There was a problem hiding this comment.
We call alternativeDefinition in the tools getter, would it make sense to change that to be model-specific? Like getTools(model)? Or should it stay generic and we only apply the model-specific stuff here?
There was a problem hiding this comment.
Hmm, feels like tools consumers should all call getEnabledTools, then it is better the tools getter stay generic - also consider there is a cache - I wonder if we should not call alternativeDefinition in the tools getter as we will call it later in getEnabledTools.
2 participants
Pass model info into the
alternativeDefinitionmethod ofICopilotToolto allow tools to provide alternative definition based on different models passing in.