-
Notifications
You must be signed in to change notification settings - Fork 616
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: permission before tool call #1313
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.
Looks great! Have a few tweaks below but excited to try this
} | ||
} | ||
}, | ||
"chat" => { |
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 instead of skipping tool calls in the loop, we really want to not advertise the tools to the agent at all? e.g. remove them from the call to provider.complete
- otherwise i could see this leading to confusing dialogue
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.
Subtle detail, but the ToolConfirmationRequest
is not added to the agent loop and is not visible to the agent. Added some context in the chat mode to allow the agent to frame the tool call requests as plans.
|
||
// Wait for confirmation response through the channel | ||
let mut rx = self.confirmation_rx.lock().await; | ||
if let Some((req_id, confirmed)) = rx.recv().await { |
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.
do we need to add any waiting deadline?
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.
for cli, we shouldn't add a timeout imo, but for GUI, it should be considered
// Wait for confirmation response through the channel | ||
let mut rx = self.confirmation_rx.lock().await; | ||
if let Some((req_id, confirmed)) = rx.recv().await { | ||
if req_id == request.id { |
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.
curious, will we have the case that req_id != request.id
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, the identical message id should be passed back.
Internal testing instructions:
pull the branch
use env var
use config
# add GOOSE_MODE=approve in ~/.config/goose/config.yaml # auto, approve,chat ./target/debug/goose session