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. |
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