feat: Add allowed_tools and require_approval to option transforms#328
feat: Add allowed_tools and require_approval to option transforms#328dark-panda wants to merge 1 commit intoactiveagents:mainfrom
allowed_tools and require_approval to option transforms#328Conversation
Some LLMs will not execute tools unless explicitly told that they are allowed to do so. This feature normalizes the options and transforms them for specific LLMs as necessary.
There was a problem hiding this comment.
Pull request overview
Adds support for MCP tool allowlisting and approval requirements during provider-specific option normalization, improving interoperability with LLMs that require explicit tool enablement.
Changes:
- OpenAI Responses: propagate
require_approvalandallowed_toolsthrough MCP server normalization. - Anthropic: introduce
normalize_mcp_toolsand auto-extracttools(asmcp_toolset) from MCP servers’allowed_toolswhentoolsis not provided. - Add/extend provider transform test coverage around these new MCP options.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/active_agent/providers/open_ai/responses/transforms.rb | Passes require_approval and allowed_tools through OpenAI MCP server normalization. |
| lib/active_agent/providers/anthropic/transforms.rb | Extracts MCP allowed_tools into Anthropic tools (mcp_toolset) during param normalization. |
| test/providers/open_ai/responses/transforms_test.rb | Adds tests for OpenAI MCP normalization with require_approval and allowed_tools. |
| test/providers/anthropic/transforms_test.rb | Adds tests for Anthropic MCP tool extraction and normalize_params integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| server_hash = server.is_a?(Hash) ? server.deep_symbolize_keys : server | ||
|
|
||
| if server_hash[:allowed_tools].present? | ||
| { | ||
| type: "mcp_toolset", | ||
| mcp_server_name: server_hash[:name], | ||
| default_config: { | ||
| enabled: false | ||
| }, | ||
| configs: server_hash[:allowed_tools].to_h { |tool| | ||
| [ tool[:name], { enabled: true } ] | ||
| } | ||
| } | ||
| end |
There was a problem hiding this comment.
normalize_mcp_tools assumes allowed_tools is an array of hashes with :name (uses tool[:name]). Elsewhere (e.g., MCP docs/examples) allowed_tools is shown as an array of strings, which would raise a TypeError here. Consider supporting both formats (string tool names and {name: ...} hashes), and ignore/validate unexpected elements so tool extraction doesn't crash on valid common-format input.
| server_hash = server.is_a?(Hash) ? server.deep_symbolize_keys : server | |
| if server_hash[:allowed_tools].present? | |
| { | |
| type: "mcp_toolset", | |
| mcp_server_name: server_hash[:name], | |
| default_config: { | |
| enabled: false | |
| }, | |
| configs: server_hash[:allowed_tools].to_h { |tool| | |
| [ tool[:name], { enabled: true } ] | |
| } | |
| } | |
| end | |
| # Only hash servers are supported for MCP tool extraction | |
| next unless server.is_a?(Hash) | |
| server_hash = server.deep_symbolize_keys | |
| allowed_tools = server_hash[:allowed_tools] | |
| next unless allowed_tools.present? && allowed_tools.is_a?(Array) | |
| configs_array = allowed_tools.filter_map do |tool| | |
| case tool | |
| when String, Symbol | |
| name = tool.to_s | |
| next if name.empty? | |
| [name, { enabled: true }] | |
| when Hash | |
| tool_hash = tool.respond_to?(:deep_symbolize_keys) ? tool.deep_symbolize_keys : tool | |
| name = tool_hash[:name] || tool_hash["name"] | |
| next unless name | |
| name_str = name.to_s | |
| next if name_str.empty? | |
| [name_str, { enabled: true }] | |
| else | |
| # Ignore unsupported tool formats | |
| next | |
| end | |
| end | |
| next if configs_array.empty? | |
| { | |
| type: "mcp_toolset", | |
| mcp_server_name: server_hash[:name], | |
| default_config: { | |
| enabled: false | |
| }, | |
| configs: configs_array.to_h | |
| } |
| test "normalize_mcp_tools returns empty array for empty array" do | ||
| result = transforms.normalize_mcp_tools([]) | ||
|
|
||
| assert_nil result | ||
| end |
There was a problem hiding this comment.
This test name says it "returns empty array for empty array" but the assertion expects nil. Please align the test description with the expected behavior (or adjust the behavior if [] is actually intended), so failures are easier to interpret.
| params[:mcp_servers] = normalize_mcp_servers(mcps) | ||
|
|
||
| if params[:tools].nil? # If tools not already provided, extract from mcps | ||
| params[:tools] = normalize_mcp_tools(mcps) |
There was a problem hiding this comment.
In normalize_params, when :tools is nil and MCPs are provided, params[:tools] = normalize_mcp_tools(mcps) will set :tools to nil when no allowed_tools are present (because normalize_mcp_tools returns nil via .presence). This introduces a tools: nil keyword arg that previously wouldn’t be sent and may serialize as "tools": null / fail Anthropic model validation. Consider only assigning :tools when toolsets were actually extracted (or deleting the key when the result is nil).
| params[:tools] = normalize_mcp_tools(mcps) | |
| mcp_tools = normalize_mcp_tools(mcps) | |
| params[:tools] = mcp_tools if mcp_tools |
| test "normalize_mcp_tools converts allowed_tools to mcp_toolset format" do | ||
| mcp_servers = [ | ||
| { | ||
| name: "stripe", | ||
| url: "https://mcp.stripe.com", | ||
| allowed_tools: [ | ||
| { name: "create_payment" }, | ||
| { name: "get_payment" } | ||
| ] | ||
| } |
There was a problem hiding this comment.
The new allowed_tools behavior is only tested with allowed_tools: [{ name: ... }], but docs/examples show allowed_tools as an array of tool-name strings. Add coverage for the string-array form (and/or mixed forms) to ensure common-format input doesn’t break normalize_mcp_tools.
Some LLMs will not execute tools unless explicitly told that they are allowed to do so. This feature normalizes the options and transforms them for specific LLMs as necessary.