Python: Filter MCP tool kwargs to declared params via allowlist#6399
Conversation
Previously MCPTool combined framework runtime kwargs (from FunctionInvocationContext.kwargs) with the LLM-supplied arguments and stripped only a hardcoded denylist of known framework keys before forwarding to the MCP server. Any new framework-injected kwarg leaked to the server unless the denylist was updated. Switch to an allowlist built from each tool's declared parameters (inputSchema.properties). Only declared params are forwarded; everything else is stripped. Add an `additional_tool_argument_names` constructor argument so users can opt extra names back in, globally (Sequence[str]) and/or per remote tool name (Mapping with reserved "*" global key). The existing denylist is kept as a safety net for framework-named params a server declares in its schema; explicitly opted-in extras always win. The reserved _meta handling is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR hardens the Python MCP tool-calling path by switching from a fragile denylist of framework-injected kwargs to an allowlist derived from each tool’s declared inputSchema.properties, with a controlled escape hatch (additional_tool_argument_names) for explicitly permitting extra names.
Changes:
- Add allowlist-based kwarg filtering in
MCPTool._prepare_call_kwargs, using tool parameter names captured at tool-load time. - Introduce
additional_tool_argument_namesconstructor arg acrossMCPTooland transport subclasses to opt specific names back into forwarding. - Add unit tests and update core package developer docs to describe the new behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_mcp.py | Implements allowlist filtering and adds additional_tool_argument_names plumbing and normalization helper. |
| python/packages/core/tests/core/test_mcp.py | Adds tests covering normalization and kwarg filtering behavior end-to-end. |
| python/packages/core/AGENTS.md | Documents the allowlist behavior and the new additional_tool_argument_names option. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90%
✓ Correctness
This PR correctly replaces a fragile denylist with an allowlist approach for filtering MCP tool kwargs. The filtering logic in
_prepare_call_kwargsis sound: declared params pass through (unless denylisted), extras always win over the denylist, and_metais always extracted separately. The_normalize_additional_tool_argument_nameshelper properly handles None, bare strings, sequences, and mappings. Bothcall_toolandcall_tool_as_taskuse the same filtering path. The transport subclasses all forward the new parameter to the base class. No correctness issues found.
✓ Security Reliability
This PR significantly improves security by replacing a fragile denylist with an allowlist approach for MCP tool kwargs. The implementation is sound: only declared parameters (from inputSchema.properties) plus explicitly user-configured extras are forwarded; framework runtime kwargs are stripped by default. The denylist is retained as a safety net for schema-declared names that collide with framework internals. The construction-time-only configuration of additional_tool_argument_names prevents model-issued tool calls from influencing the filter. One minor reliability concern: string values in the mapping form of additional_tool_argument_names would be silently split into individual characters rather than treated as single names, unlike the outer parameter which has explicit str handling.
✓ Test Coverage
The PR adds solid unit tests for
_normalize_additional_tool_argument_namesand_prepare_call_kwargscovering the main paths: stripping undeclared args, global/per-tool extras, denylist guarding, zero-arg tools, unknown tools, and _meta extraction. An end-to-end test exercisesload_tools+call_tool. However, two claims in the PR rationale lack dedicated test coverage: (1) the 'extras always win over the denylist' behavior when a denylisted name is both declared in the schema AND opted in via extras, and (2) the header_provider secret-leak fix (existing test at line 4638 passessome_tokenbut never asserts it's stripped from forwarded arguments).
✓ Failure Modes
The allowlist filtering logic has a placement bug that causes silent argument loss on tool reload. When
notifications/tools/list_changedtriggersload_tools(), previously loaded tools are skipped by theexisting_namescheck (line 1336-1337), so their param names are never added totool_param_names_by_name. The dict is then fully replaced (line 1386), leaving those tools with an emptydeclaredset. Subsequentcall_toolinvocations silently drop all model-supplied arguments. The meta and task_support registrations (lines 1325-1330) are correctly placed BEFORE the skip check, but the new param names code was placed AFTER it.
✓ Design Approach
I found one blocking design regression in the new allowlist cache: background or repeated tool reloads erase the declared-parameter allowlist for already-loaded tools, so subsequent MCP calls can silently drop model-suplied arguments. The rest of the approach looks aligned with the PR rationale.
Automated review by eavanvalkenburg's agents
- Fix pyright reportUnknownArgumentType in _load_tools (cast schema properties). - Register declared param names before the existing-tool skip guard so that tool-list reloads preserve the allowlist for already-loaded tools (previously unchanged tools silently dropped all declared args after a background reload). - Handle bare-string values in an additional_tool_argument_names mapping instead of iterating their characters. - Clarify the framework denylist comment: explicit extras override the denylist. - Make the extras-override-denylist test unambiguous (opt in a denylisted name). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
When the framework calls an MCP tool,
MCPToolmerges the framework runtimekwargs (from
FunctionInvocationContext.kwargs— e.g.thread,conversation_id,chat_options,tools) with the arguments supplied by the model, then forwardsthe merged dict to the server. Until now this was cleaned up with a hardcoded
denylist of known framework keys. That approach is fragile: any new
framework-injected kwarg leaks to the MCP server as a tool argument unless the
denylist is updated to match.
Description
Replace the denylist with an allowlist derived from each tool's actual
declared parameters (
inputSchema.properties, captured at tool-load time). Onlydeclared parameters are forwarded to the server; framework runtime kwargs are
stripped by default.
additional_tool_argument_namesonMCPTooland allthree transport subclasses (
MCPStdioTool,MCPStreamableHTTPTool,MCPWebsocketTool) lets users opt extra argument names back in. Accepts aSequence[str](applied to every tool) or aMapping[str, Sequence[str]]keyed by remote tool name, where the reserved key
"*"denotes global extras.It is configured only in user code at construction — there is no per-call
override, so a model-issued tool call cannot change which names pass through.
properties(including schemas withadditionalProperties: true) forwards only the configured extras.parameters a server declares in its schema; names explicitly opted in via
additional_tool_argument_namesalways win._metahandling is unchanged.This also tightens an existing leak: in the
header_providerflow (seesamples/02-agents/mcp/mcp_api_key_auth.py), a secret passed viafunction_invocation_kwargswas previously forwarded to the server as a toolargument; it is now stripped while header injection continues to work.
Contribution Checklist