Skip to content

feat: Add allowed_tools and require_approval to option transforms - #328

Open
dark-panda wants to merge 1 commit into
activeagents:mainfrom
dark-panda:feat/allowed-tools-and-require-approval
Open

feat: Add allowed_tools and require_approval to option transforms#328
dark-panda wants to merge 1 commit into
activeagents:mainfrom
dark-panda:feat/allowed-tools-and-require-approval

Conversation

@dark-panda

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_approval and allowed_tools through MCP server normalization.
  • Anthropic: introduce normalize_mcp_tools and auto-extract tools (as mcp_toolset) from MCP servers’ allowed_tools when tools is 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.

Comment thread lib/active_agent/providers/anthropic/transforms.rb
Comment thread test/providers/anthropic/transforms_test.rb
Comment thread lib/active_agent/providers/anthropic/transforms.rb
Comment thread test/providers/anthropic/transforms_test.rb
@dark-panda
dark-panda force-pushed the feat/allowed-tools-and-require-approval branch from 623a217 to 43a4fed Compare June 9, 2026 19:24
@TonsOfFun

Copy link
Copy Markdown
Contributor

I think we may need to update the VCR cassettes here

@dark-panda
dark-panda force-pushed the feat/allowed-tools-and-require-approval branch from 43a4fed to 957bd29 Compare July 15, 2026 13:52
@TonsOfFun
TonsOfFun self-requested a review July 29, 2026 01:15

Copy link
Copy Markdown
Contributor

I dug into the CI failures on this PR. There are two separate causes, and I've pushed a fixed version of this change to the claude/pr-328-test-failures-xihiq8 branch (this PR's commit plus a fix commit) with the full suite green locally (1287 runs, 0 failures, 0 errors) and bin/rubocop clean.

Why the 50 test failures happen

Every failing request body contains "tools":null — including plain requests with no MCP configuration at all:

POST https://api.anthropic.com/v1/messages with body
'{"model":"claude-sonnet-5","messages":[...],"max_tokens":1024,...,"tools":null}'

The chain is:

  1. Anthropic::Request#apply_defaults sets mcp_servers: [] on every request (it's in DEFAULTS, see lib/active_agent/providers/anthropic/request.rb).
  2. An empty array is truthy in Ruby, so the new if params[:mcps] || params[:mcp_servers] branch in normalize_params now runs unconditionally — even for requests that never touched MCP.
  3. normalize_mcp_tools([]) returns nil (via .compact.presence), and params[:tools] = nil puts an explicit tools key into the gem model's data, which serializes as "tools": null in the request body.
  4. Every WebMock body expectation in the integration suite then fails to match, since the expected bodies don't include a tools key. (Copilot's review also spotted this exact hazard.)

The fix is to only assign :tools when toolsets were actually extracted:

if params[:tools].nil?
  mcp_tools = normalize_mcp_tools(mcps)
  params[:tools] = mcp_tools if mcp_tools.present?
end

Why the lint job fails

The repo's RuboCop config (rails-omakase) enforces Layout/SpaceInsideArrayLiteralBrackets — array literals need spaces inside the brackets ([ "create_payment" ], not ["create_payment"]). The new tests in this PR have 28 such offenses. bin/rubocop -A on the touched files fixes them all.

Also included on the fix branch

While fixing the above I also addressed the Copilot review feedback:

  • normalize_mcp_tools now accepts allowed_tools entries as tool-name strings/symbols as well as { name: ... } hashes (previously tool[:name] would raise TypeError on the string form shown in the MCP docs), ignoring entries in any other format, with test coverage for the string and mixed forms.
  • normalize_mcp_tools now consistently returns nil whenever nothing is extracted (nil/non-array/empty input, or no allowed_tools), and the test names that said "returns empty array" but asserted nil were aligned with the actual behavior.

Feel free to cherry-pick 414821c from that branch onto this PR, or apply the equivalent changes directly.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants