feat: support loading MCP server configurations from JSON config files#2108
feat: support loading MCP server configurations from JSON config files#2108
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/strands/tools/mcp/mcp_config.py
Outdated
| from mcp.client.stdio import stdio_client | ||
| from mcp.client.streamable_http import streamablehttp_client | ||
|
|
||
| from .mcp_client import MCPClient, ToolFilters, _ToolMatcher |
There was a problem hiding this comment.
Issue: Importing private type _ToolMatcher from mcp_client.py.
_ToolMatcher is prefixed with _ indicating it's a private/internal type, but it's now used as a cross-module dependency.
Suggestion: Either promote _ToolMatcher to a public type (remove the underscore and export it) since it's now part of the contract between modules, or re-define the type locally in mcp_config.py.
There was a problem hiding this comment.
Good catch. I'll redefine the type locally in mcp_config.py to avoid cross-module private dependency.
src/strands/tools/mcp/mcp_config.py
Outdated
| Returns: | ||
| True if the string appears to be a regex pattern, False for plain strings. | ||
| """ | ||
| return any(c in _REGEX_SPECIAL_CHARS for c in s) |
There was a problem hiding this comment.
Issue: The _is_regex_pattern heuristic is fragile and can produce false positives.
Any string containing characters like ., (, [, $, etc. will be treated as a regex pattern. For example, a tool filter value like "my.tool" or "tools(v2)" would be silently compiled as a regex instead of being treated as a literal match, leading to unexpected behavior.
Suggestion: Consider a more explicit opt-in mechanism. Options include:
- A prefix/suffix convention, e.g.
"/search_.*/"for regex (similar to JavaScript notation) - Always treat as regex (since exact strings are also valid regex)
- A structured format:
{"pattern": "search_.*", "type": "regex"}vs{"pattern": "echo", "type": "exact"}
Option 2 (always compile as regex) is the simplest — exact strings like "echo" work fine as regex patterns since they match themselves. This avoids the ambiguity entirely.
There was a problem hiding this comment.
Agreed, option 2 (always compile as regex) is the simplest and eliminates the ambiguity entirely. Implementing this now.
src/strands/tools/mcp/mcp_config.py
Outdated
| return result if result else None | ||
|
|
||
|
|
||
| def create_mcp_client_from_config(server_name: str, config: dict[str, Any]) -> MCPClient: |
There was a problem hiding this comment.
Issue: create_mcp_client_from_config and parse_tool_filters are public functions (no underscore prefix) but are not included in __all__ in __init__.py.
Suggestion: Either add them to __all__ for discoverability, or prefix them with _ if they're intended to be internal implementation details. Given that create_mcp_client_from_config could be useful for users who want to create a single client from config, consider making it explicitly public.
There was a problem hiding this comment.
I'll prefix both create_mcp_client_from_config and parse_tool_filters with _ since they're internal implementation details. load_mcp_clients_from_config is the intended public entry point.
| raise FileNotFoundError(f"MCP configuration file not found: {file_path}") | ||
|
|
||
| with open(config_path) as f: | ||
| config_dict: dict[str, Any] = json.load(f) |
There was a problem hiding this comment.
Issue: PR description mentions Claude Desktop / Cursor interoperability, but the function doesn't support the standard Claude Desktop config wrapper format.
Claude Desktop configs typically nest servers under a "mcpServers" key: {"mcpServers": {"server1": {...}}}. The current implementation requires the unwrapped format (just the server dict). Users copying their claude_desktop_config.json would get unexpected results.
Suggestion: Consider auto-detecting and unwrapping the "mcpServers" wrapper key when it's the only top-level key, or when the config is loaded from a file. This would align with the "Embrace common standards" tenet and the interoperability use case from the PR description.
There was a problem hiding this comment.
Good point. I'll auto-detect and unwrap the "mcpServers" wrapper key to support copy-pasting from Claude Desktop config files.
| agent_kwargs["tools"] = tools_list | ||
| elif "mcp_servers" in config_dict: | ||
| # Empty dict case - still call to maintain consistent behavior | ||
| load_mcp_clients_from_config(config_dict["mcp_servers"]) |
There was a problem hiding this comment.
Issue: The elif branch for empty mcp_servers dict is unnecessary and confusing.
When mcp_servers is {}, calling load_mcp_clients_from_config({}) just returns {} with no side effects. The comment says "still call to maintain consistent behavior" but there's no behavior to maintain — it's a no-op.
Suggestion: Simplify to a single if block:
if config_dict.get("mcp_servers"):
mcp_clients = load_mcp_clients_from_config(config_dict["mcp_servers"])
tools_list = agent_kwargs.get("tools", [])
if not isinstance(tools_list, list):
tools_list = list(tools_list)
tools_list.extend(mcp_clients.values())
agent_kwargs["tools"] = tools_listThere was a problem hiding this comment.
Agreed, simplifying to a single if block.
|
|
||
| # Extract common MCPClient parameters | ||
| prefix = config.get("prefix") | ||
| startup_timeout = config.get("startup_timeout", 30) |
There was a problem hiding this comment.
Issue: Missing startup_timeout type annotation and validation.
The startup_timeout value is read from JSON config as config.get("startup_timeout", 30) but there's no validation that it's actually a number. A string value like "30" would silently pass through and potentially cause a type error later in MCPClient.
Suggestion: Add basic type validation for startup_timeout (and optionally prefix) in create_mcp_client_from_config, or add JSON schema validation for the individual server config entries.
There was a problem hiding this comment.
Adding type validation for startup_timeout and invalid regex patterns.
src/strands/tools/mcp/mcp_config.py
Outdated
| env=config.get("env"), | ||
| cwd=config.get("cwd"), | ||
| ) | ||
| transport_callable = lambda: stdio_client(params) # noqa: E731 |
There was a problem hiding this comment.
Issue: The noqa: E731 comments for lambda assignments suggest these should be proper functions.
The linter rule E731 flags lambda assignments for good reason — they're harder to debug (stack traces show <lambda> instead of a descriptive name) and can't have docstrings.
Suggestion: Use nested def statements instead:
if transport == "stdio":
params = StdioServerParameters(...)
def transport_callable() -> Any:
return stdio_client(params)This eliminates the need for noqa suppressions and produces better stack traces.
There was a problem hiding this comment.
Replacing lambdas with named def statements for better stack traces.
|
Assessment: Request Changes This PR adds a well-structured feature for loading MCP server configurations from JSON files, addressing a real user need for declarative multi-server setup. The overall architecture is sound — separate config module, integration with existing However, there are a few issues that need to be addressed before merge: Review Categories
The feature design aligns well with the SDK tenets, particularly "Simple at any scale" and "Embrace common standards." |
|
/strands delete the *.log files |
Add declarative MCP server configuration via agent config files and dicts. Supports stdio, SSE, and streamable-HTTP transports with JSON schema validation, tool filters, and auto-detection of transport type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Unshure
force-pushed
the
agent-tasks/482
branch
from
08bae00 to
dad40f5
Compare
|
|
||
| _SCHEMA_PATH = Path(__file__).parent / "mcp_server_config.schema.json" | ||
| with open(_SCHEMA_PATH) as _f: | ||
| MCP_SERVER_CONFIG_SCHEMA: dict[str, Any] = json.load(_f) |
There was a problem hiding this comment.
Issue: Module-level file I/O at import time is a new pattern in this codebase and introduces side effects.
Both mcp_config.py and agent_config.py now read JSON files at the module level. Since agent_config.py imports from mcp_config.py, importing anything from strands.experimental.agent_config triggers two file reads. The original agent_config.py had the schema inline as a Python dict with no file I/O.
Suggestion: Consider either:
- Keep schemas as Python dicts inline (consistent with the original pattern and avoids file I/O at import time)
- Lazy-load the schemas on first use (e.g., via
functools.lru_cacheon a loader function) - If external JSON files are preferred for editor/tooling support, use
importlib.resourcesinstead ofPath(__file__).parent— this is the recommended approach for reading package data files and works correctly even in zipped distributions.
| ) | ||
|
|
||
|
|
||
| def load_mcp_clients_from_config(config: str | dict[str, Any]) -> dict[str, MCPClient]: |
There was a problem hiding this comment.
Issue: load_mcp_clients_from_config is not exported from strands.experimental.__init__.py.
The PR description shows the import path as from strands.tools.mcp import load_mcp_clients_from_config, but the function has been moved to strands.experimental.mcp_config. Users currently must use from strands.experimental.mcp_config import load_mcp_clients_from_config, which is an internal module path.
Suggestion: Add load_mcp_clients_from_config to strands.experimental.__init__.py's __all__ and imports. This makes the function discoverable via the established experimental namespace and gives users a stable import path:
from strands.experimental import load_mcp_clients_from_config
| "mcp_servers": { | ||
| "description": "MCP server configurations. Each key is a server name and the value is a server configuration object with transport-specific settings.", | ||
| "type": "object", | ||
| "additionalProperties": { "$ref": "mcp_server_config.schema.json" } |
There was a problem hiding this comment.
Issue: The $ref in this JSON schema is not resolved by the JSON schema validator — it's manually replaced in Python code at agent_config.py:31.
This means the JSON file is misleading when read standalone: the $ref looks like a standard JSON Schema reference, but it won't work with any standard JSON Schema tooling. Anyone opening this file in an IDE or schema validator would get a resolution error.
Suggestion: Either:
- Inline the MCP server schema directly in this file (removes the misleading
$ref) - Add a comment in the JSON file explaining the
$refis resolved programmatically - If keeping separate files, consider using the
jsonschemaRefResolverto properly resolve$refs
| def _parse_tool_filters(config: dict[str, Any] | None) -> ToolFilters | None: | ||
| """Parse a tool filter configuration into a ToolFilters instance. | ||
|
|
||
| All filter strings are compiled as regex patterns. Exact-match strings like ``"^echo$"`` |
There was a problem hiding this comment.
Issue: Docstring says "^echo$" but the typical user pattern from config will just be "echo".
Since all strings are compiled as regex and re.match is used (not re.fullmatch), "echo" matches "echo_extra" as well. The docstring mentions "^echo$" as an example of exact-match, but users likely won't think to add anchors from a JSON config file.
Suggestion: Update the docstring to be clearer about this behavior:
All filter strings are compiled as regex patterns and matched using ``re.match``
(prefix match). Use ``"^echo$"`` for exact matching, or ``"echo"`` to match any
tool name starting with "echo".
|
Issue: The PR description import path is outdated. The PR description still shows: from strands.tools.mcp import load_mcp_clients_from_configBut the function now lives in |
|
Assessment: Request Changes Great progress from the previous iteration — all prior feedback was addressed thoughtfully. The A few items remain before this is ready: Review Categories
The overall design is clean and the test coverage is thorough across unit, integration, and schema validation layers. |
- Move test file to tests/strands/experimental/ to mirror src/ structure - Export load_mcp_clients_from_config from strands.experimental.__init__ - Convert JSON schema files to inline Python dicts (eliminates module-level file I/O) - Remove hack by directly referencing MCP_SERVER_CONFIG_SCHEMA in agent config - Delete unused .schema.json files - Update _parse_tool_filters docstring to clarify re.match prefix-match semantics
|
|
||
| import json | ||
| from pathlib import Path | ||
| import logging |
There was a problem hiding this comment.
Issue: Bug — from pathlib import Path was removed from agent_config.py, breaking file-based config loading.
The original agent_config.py imported Path on line 15. This PR replaced it with import logging but Path is still used on line 107 (config_path = Path(file_path)). This means config_to_agent("path/to/config.json") — an existing, working feature — will raise NameError: name 'Path' is not defined.
Suggestion: Add the import back:
import json
import logging
from pathlib import Path
from typing import Any
| if file_path.startswith("file://"): | ||
| file_path = file_path[7:] | ||
|
|
||
| config_path = Path(file_path) |
There was a problem hiding this comment.
Issue: Bug — Path is used on line 250 but never imported.
load_mcp_clients_from_config("path/to/config.json") will raise NameError: name 'Path' is not defined because pathlib.Path is not imported in this module.
Suggestion: Add to the imports at the top of the file:
from pathlib import Path
|
Issue: The PR description still shows the outdated import path. The "Public API Changes" section shows: from strands.tools.mcp import load_mcp_clients_from_configBut the function is now at from strands.experimental import load_mcp_clients_from_config |
|
Assessment: Request Changes The PR has improved significantly across three iterations — schemas are now clean inline Python dicts, test files are correctly placed, exports are properly configured, and the API surface is well-designed. However, two critical bugs and an unrelated change need to be addressed. Review Categories
The underlying feature design is solid and all prior feedback (13 threads) has been addressed well. Once the |
1 participant
Motivation
Users currently must programmatically instantiate each
MCPClientwith transport callables, which becomes verbose when managing multiple MCP servers. The Claude Desktop / Cursor / VS Code ecosystem has established a convention for declarativemcpServersJSON configuration. This change brings that same pattern to Strands, letting users define multiple MCP servers in a single JSON config file and load them automatically.Resolves: #482
Public API Changes
New
load_mcp_clients_from_config()function for standalone usage:config_to_agent()now accepts anmcp_serversfield:Transport type is auto-detected from config fields (
command→ stdio,url→ sse) or specified explicitly viatransport. Per-server options includeprefix,startup_timeout,tool_filters(with regex pattern support),env,cwd, andheaders.Key implementation decisions:
"echo"are valid regex matching themselves), avoiding fragile auto-detection heuristicsmcpServerswrapper key from Claude Desktop/Cursor configs is auto-detected and unwrapped for interoperability_create_mcp_client_from_config,_parse_tool_filters) are private; onlyload_mcp_clients_from_configis the public entry pointUse Cases
mcpServersJSON config from Claude Desktop or Cursor setups