-
Notifications
You must be signed in to change notification settings - Fork 37k
Add error handling properties to language model tool results #284549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds error handling properties (toolResultError and hasError) to language model tool results, enabling better communication of error states between tools and the chat system. The changes span the API definition layer, type conversion utilities, service interfaces, and the main thread implementation.
Key Changes
- Added
toolResultErrorstring property toExtendedLanguageModelToolResultAPI - Added
hasErrorboolean flag toIToolResultinterface and API - Updated type converters to handle the new error properties in both directions (to/from extension API)
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/vscode-dts/vscode.proposed.chatParticipantPrivate.d.ts |
Added toolResultError property to ExtendedLanguageModelToolResult API definition |
src/vs/workbench/contrib/chat/common/languageModelToolsService.ts |
Added hasError property to IToolResult interface |
src/vs/workbench/api/common/extHostTypeConverters.ts |
Updated LanguageModelToolResult and LanguageModelToolResult2 converters to handle toolResultError, toolResultMessage, toolMetadata, and hasError properties |
src/vs/workbench/api/browser/mainThreadLanguageModelTools.ts |
Updated $invokeTool to pass toolResultMessage, toolResultError, toolMetadata, and hasError to extension host |
da1ba7c to
c987897
Compare
| /** Whether to ask the user to confirm these tool results. Overrides {@link IToolConfirmationMessages.confirmResults}. */ | ||
| confirmResults?: boolean; | ||
| /** Whether there was an error calling the tool. The tool may still have partially succeeded. */ | ||
| hasError?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about how hasError/toolResultError are used today, are those redundant? The tool can also throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok, we can run it past @connor4312 that it is correct and useful to share these props with the extension for logging from eval runs
Introduce additional properties to handle error states in language model tool results. This enhancement allows for better communication of errors and messages from the tool, improving overall robustness and user experience.