-
Notifications
You must be signed in to change notification settings - Fork 749
feat(chat): add tool utils to delegate execution #6863
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
|
|
/retryBuilds |
| | { type: ToolType.FsWrite; tool: FsWrite } | ||
| | { type: ToolType.ExecuteBash; tool: ExecuteBash } | ||
|
|
||
| export class ToolUtils { |
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.
what's the intent of this class?
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.
The idea was to have this be a helper class to interact with all the tools and to sort of enforce a common interface (requiresAcceptance, invoke, etc) so that we don't need to keep writing if-else or switch statements just to handle tools. This is how the CLI does it as well
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.
Seems like a case where class-based inheritance would fit.
| import { ToolResult, ToolResultContentBlock, ToolResultStatus, ToolUse } from '@amzn/codewhisperer-streaming' | ||
| import { InvokeOutput } from './toolShared' | ||
|
|
||
| export enum ToolType { |
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 feel like this code is not extensible for when we support MCP-based tools. Can we revisit this after the prototype?
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.
Sure, I need to think about how MCP will work here. Happy to revisit later
| \`\`\` | ||
| using the \`executeBash\` tool.` | ||
| } | ||
| if (toolUse.name === 'fsWrite') { |
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 know you didn't change this - but I feel like we should refactor this to have a cleaner interface. Having a giant if/else block doesn't seem like the right solution.
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.
Yeah that was the goal of this PR to have the common interface across all the tools (I just didn't get to refactoring this part yet since @laileni-aws will be integrating his UI changes here)
mr-lee
left a comment
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 approving to unblock - but I also think we need to refactor here.
| "type": "shell", | ||
| "command": "npm", | ||
| "args": ["run", "watch"], | ||
| "type": "npm", |
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.
is there a reason why this was changed? if it needs to be changed can you make it on master
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 accidentally committed this change to this branch in a previous commit. Here I'm reverting it back to what it is in master.
On another note, my vscode keeps trying to run watch using yarn instead of npm even in master. I noticed this happening for others as well so I'll make this change in master
Problem
Tool APIs are not consistent.
Solution
Create a tool util wrapper class around tools to delegate validation, execution, and previews of tools.
So now to use the tools we can do something like:
feature/xbranches will not be squash-merged at release time.