-
Notifications
You must be signed in to change notification settings - Fork 748
feat(chat): Add header for execute shell command #6948
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
feat(chat): Add header for execute shell command #6948
Conversation
|
29fffec to
e358157
Compare
| }) | ||
| ) | ||
| // Need separate id for read tool and safe bash command execution as 'run-shell-command' id is required to state in cwChatConnector.ts which will impact generic tool execution. | ||
| if (tool.type === ToolType.ExecuteBash) { |
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.
const actionId = tool.type === ToolType.ExecuteBash
? 'run-shell-command'
: 'generic-tool-execution'
this.dispatcher.sendCustomFormActionMessage(
new CustomFormActionMessage(tabID, { id: actionId })
)
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 do it in next pr.
| buttons: toolUse?.name === ToolType.FsWrite ? undefined : buttons, | ||
| fullWidth: toolUse?.name === ToolType.FsWrite, | ||
| padding: !(toolUse?.name === ToolType.FsWrite), | ||
| buttons: |
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.
We can keep buttons: undefined. Buttons is used only by fsWrite and ExecuteBash
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.
you mean the current check is right, but we can simplify it, right?
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.
if yes, then i can do it in next pr.
| id: 'confirmed-tool-use', | ||
| case 'run-shell-command': | ||
| answer.header = { | ||
| icon: 'code-block' as MynahIconsType, |
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 we don't need this icon and body. Please follow same approach as accept-code-diff.
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.
why we don't need body and icon? or i missed some UI?
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.
Use the tyrone StoreItem to get the body and icon
Problem
Solution
when shell command un-destructive, directly execute the command

when when shell command destructive, customer can decide to run or reject the command execution

-- reject

-- run

feature/xbranches will not be squash-merged at release time.