-
Notifications
You must be signed in to change notification settings - Fork 396
fix: consolidate tool permission logic for consistent display and exe… #2975
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
Before change Behavior:Example ScenarioConfiguration: in Agent mcp config file On running /tools displays MCP tools from
|
After fix Behavior:Example ScenarioConfiguration: in Agent mcp config file On running /tools displays MCP tools from
|
Another After fix Behavior:Example ScenarioConfiguration: in Agent mcp config file On running /tools displays MCP tools from
|
kkashilk
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.
Can you confirm that you've run -
cargo clippy --locked --workspace --color always -- -D warnings
cargo +nightly fmt --check -- --color always
cargo +nightly fmt
| // Native tool | ||
| None => { | ||
| let patterns = filter_patterns(|p| !p.starts_with('@')); | ||
| info!("Native patterns: {:?}", patterns); |
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.
nit: change info! to debug! unless necessary.
You can run Q_LOG_LEVEL=debug cargo run... to get these logs if necessary.
|
|
||
| /// Checks if a tool is allowed based on the agent's allowed_tools configuration. | ||
| /// This function handles both native tools and MCP tools with wildcard pattern support. | ||
| pub fn is_tool_allowed( |
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 replace the call to matches_any_pattern in native tool validation as well
For example -
| let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); |
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.
Good callout
crates/chat-cli/src/cli/agent/mod.rs
Outdated
| ToolOrigin::Native => None, | ||
| _ => Some(<ToolOrigin as Borrow<str>>::borrow(origin)), | ||
| }; | ||
| is_tool_allowed(&a.allowed_tools, tool_name, server_name) |
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.
nit: change method name to is_tool_in_allowlist.
Whether a tool is allowed or not is a bit more complicated depending on the tool
aws#2975) * fix: consolidate tool permission logic for consistent display and execution * fix: centralize tool permission checking logic --------- Co-authored-by: Niraj Chowdhary <[email protected]>
Issue #, if available: None
Overview
This change consolidates tool permission checking logic to ensure consistent behavior between the /tools display and actual tool execution in Q CLI.
Background
The CLI had two separate implementations for checking tool permissions:
Display path: Used for showing trust status in /tools command
Execution path: Used when actually running tools
These implementations had different wildcard pattern support, leading to inconsistent user experience.
Documentation Reference
https://github.com/aws/amazon-q-developer-cli/blob/main/docs/agent-format.md#allowedtools-field
According to the allowedTools specification:
Pattern Types:
Native tools: "fs_read", "fs_*"
MCP tools: "@server", "@server/tool", "@quip"
Key Rules:
Patterns starting with @ apply to MCP tools
Patterns without @ apply to native tools
Both support wildcard patterns (*, ?)
Solution
Created a centralized tool_permission_checker utility that:
Provides single source of truth for permission logic
Properly separates native vs MCP pattern matching
Ensures consistent wildcard support across all code paths
Stays consistent with Q cli documentation on allowedTools instructions
Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.