-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: implement a tool permission store #1516
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
|
|
||
| let file = File::open(file_path)?; | ||
| let mut permissions: ToolPermissionStore = serde_json::from_reader(file)?; | ||
| permissions.permissions_dir = store.permissions_dir; |
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 clean up the expired entries?
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.
yes, good call
| if let Ok(tool_call) = request.tool_call.clone() { | ||
| if let Some(allowed) = store.check_permission(request) { | ||
| if allowed { | ||
| let output = capabilities.dispatch_tool_call(tool_call).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 output something to let users know the permission is skipped due to cache?
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.
possibly, it could be a bit noisy imo
| // This helps identify when the same tool is being used in a different context | ||
| let mut hasher = Hasher::new(); | ||
| hasher.update( | ||
| serde_json::to_string(&tool_request.tool_call.as_ref().unwrap().arguments) |
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 want to hash on some argument params? I am wondering whether we will have low hit rate if we hash all, including argument param and value, like write file, the name can be different, but they are super similar
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.
Similar thought - I was thinking to hash by tool name only at first, but this would lump all bash commands in one hash.
Any ideas about how to do the hash at the right level of granularity?
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, kind of tricky, introducing some cosine similarity check may be overkilled. I am wondering whether we can have a set of normalized keys? for example in bash case, we have "command":write, "file_path": xxx, and we maintain a set containing "file_path", if it is in the normalized key set, like "file_path", we replace the value with "normalized_value" before hashing. Even with such case, we cannot handle all argument keys, we can just start with bash? how do you think?
|
Overall, lgtm, thanks for the improvment |
* main: feat: parallel processing in approve mode (#1575) Feat: support auto-including dirs in binary/bench-work-dir (#1576) refactor models component (#1535) docs: Add running Goose in CI tutorial (#1426) chore: remove logging of oauth config, just log where we output (#1573) feat: implement a tool permission store (#1516) minor typo (#1569) fix: open new session in working dir when hotkey is pressed (#1570) feat: store working directory for sessions (#1559) docs: extension timeout (#1567) fix: update openrouter referer website to point to github page site (#1566) feat(cli): add --debug flag to goose session / run (#1564)
~/.config/goose/tool_permissions.json