-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add support for search in workspace #15493
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
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.
Very cool, thank you! Works already pretty well. I think we'll need a bit of prompt tweaking to help the LLM making best use of this. But this is something we probably need to play with in more detail over time (observing multiple search scenarios).
}, | ||
required: ['query', 'useRegExp'] | ||
}, | ||
handler: (argString: string) => this.handleSearch(argString) |
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.
It might be worth looking for a cancellation token to ensure long running searches can be canceled by the user if the user cancels the response:
handler: async (args: string, ctx?: MutableChatRequestModel): Promise<string> => {
const token = ctx?.response?.cancellationToken;
...
return { | ||
id: SEARCH_IN_WORKSPACE_ID, | ||
name: SEARCH_IN_WORKSPACE_ID, | ||
description: 'Searches the workspace for files containing the provided query string or regular expression.', |
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 had a few cases where the AI tried to search for multi-word search strings. So we should probably guide the AI here a bit better how to use this search tool. Maybe the following works better?
description: 'Searches the workspace for files containing the provided query string or regular expression.', | |
description: 'Searches the workspace for files matching the given search term (parameter `query`), using case-insensitive full-string or regex matching. Multi-word patterns must match exactly (including spaces, but case-insensitive). For best results, prefer multiple queries with alternative search terms, over single long or complex ones.', |
But I didn't deeply test this.
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.
LGTM (with Philips feedback)
}; | ||
} | ||
|
||
private async handleSearch(argString: string, cancellationToken: CancellationToken): Promise<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.
private async handleSearch(argString: string, cancellationToken: CancellationToken): Promise<string> { | |
private async handleSearch(argString: string, cancellationToken?: CancellationToken): Promise<string> { |
this.searchService.search(args.query, callbacks, options) | ||
.then(id => { | ||
expectedSearchId = id; | ||
cancellationToken.onCancellationRequested(() => { |
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.
cancellationToken.onCancellationRequested(() => { | |
cancellationToken?.onCancellationRequested(() => { |
fixed as part of #15504 |
What it does
fixes #15467
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers