-
Notifications
You must be signed in to change notification settings - Fork 37.5k
Add optional chatSessionResource parameter for read operations in ManageTodoListTool #289290
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
Conversation
3f0cc60 to
1d651db
Compare
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 a chatSessionResource parameter to the IManageTodoListToolInputParams interface to enable read operations when the session resource is not available through the invocation context. The change provides a fallback mechanism for obtaining the chat session resource specifically for read operations.
Changes:
- Added optional
chatSessionResourcestring parameter toIManageTodoListToolInputParamsinterface - Modified the
invokemethod to use the parameter as a fallback wheninvocation.context?.sessionResourceis undefined and the operation is 'read'
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/common/tools/builtinTools/manageTodoListTool.ts:100
- The new fallback logic for chatSessionResource when operation is 'read' lacks test coverage. Consider adding tests that verify this behavior, particularly testing scenarios where invocation.context.sessionResource is undefined and args.chatSessionResource is provided, as well as cases with invalid URI strings to ensure proper error handling.
const chatSessionResource = invocation.context?.sessionResource ?? (args.operation === 'read' && args.chatSessionResource ? URI.parse(args.chatSessionResource) : undefined);
src/vs/workbench/contrib/chat/common/tools/builtinTools/manageTodoListTool.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/common/tools/builtinTools/manageTodoListTool.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/common/tools/builtinTools/manageTodoListTool.ts
Outdated
Show resolved
Hide resolved
…TodoListTool.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| status: 'not-started' | 'in-progress' | 'completed'; | ||
| }>; | ||
| // used for todo read only | ||
| chatSessionResource?: string; |
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.
Can't it be a URI?
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.
No. This is coming from an non llm tool call and I can't pass the sessionResource string back from the chat extension as a URI
No description provided.