Repository navigation
Python: Filter MCP tool kwargs to declared params via allowlist - #6399
Eduard van Valkenburg (eavanvalkenburg) merged 2 commits into
Conversation
Sorry, something went wrong.
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 Unit Test Overview
|
||||||||||||||||||||||||||||||
Sorry, something went wrong.
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_names constructor arg across MCPTool and 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. |
Sorry, something went wrong.
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_kwargs is sound: declared params pass through (unless denylisted), extras always win over the denylist, and _meta is always extracted separately. The _normalize_additional_tool_argument_names helper properly handles None, bare strings, sequences, and mappings. Both call_tool and call_tool_as_task use 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_names and _prepare_call_kwargs covering 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 exercises load_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 passes some_token but 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_changed triggers load_tools(), previously loaded tools are skipped by the existing_names check (line 1336-1337), so their param names are never added to tool_param_names_by_name. The dict is then fully replaced (line 1386), leaving those tools with an empty declared set. Subsequent call_tool invocations 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
Sorry, something went wrong.
- 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, MCPTool merges the framework runtime
kwargs (from FunctionInvocationContext.kwargs — e.g. thread, conversation_id,
chat_options, tools) with the arguments supplied by the model, then forwards
the 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). Only
declared parameters are forwarded to the server; framework runtime kwargs are
stripped by default.
three transport subclasses (MCPStdioTool, MCPStreamableHTTPTool,
MCPWebsocketTool) lets users opt extra argument names back in. Accepts a
Sequence[str] (applied to every tool) or a Mapping[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.
additionalProperties: true) forwards only the configured extras.
parameters a server declares in its schema; names explicitly opted in via
additional_tool_argument_names always win.
This also tightens an existing leak: in the header_provider flow (see
samples/02-agents/mcp/mcp_api_key_auth.py), a secret passed via
function_invocation_kwargs was previously forwarded to the server as a tool
argument; it is now stripped while header injection continues to work.
Contribution Checklist