feat(sdk-python): enable mcp_servers support via initialize control request - #6463
feat(sdk-python): enable mcp_servers support via initialize control request#6463juhuan wants to merge 1 commit into
Conversation
…equest Remove the validation that rejected mcp_servers in the Python SDK. CLI MCP server configs are now sent to the CLI during the initialize control request, matching the TypeScript SDK's pattern. The CLI's SystemController already handles external MCP server configs received through this channel.
|
Thanks for the PR! Template looks good ✓ Problem: This is a real feature gap, not theoretical hardening. The Python SDK currently raises Direction: Aligned. MCP server configuration is a core SDK capability, and the Python SDK is the primary SDK for programmatic access. Bringing it to parity with the TypeScript SDK on CLI-managed MCP servers is clearly within scope. The PR correctly draws the line at in-process SDK MCP servers (tracked in #4889). Size: 8 production lines (2 added in Approach: Minimal and focused. Two surgical edits: remove the validation block, wire Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是真实的功能缺口，不是理论性加固。Python SDK 目前在用户尝试配置 MCP 服务器时抛出 方向：对齐。MCP 服务器配置是 SDK 的核心能力，Python SDK 是编程访问的主要 SDK。在 CLI 管理的 MCP 服务器方面与 TS SDK 对齐，完全在范围内。PR 正确地将进程内 SDK MCP 服务器排除在外（跟踪于 #4889）。 规模：8 行生产代码（ 方案：最小且聚焦。两处精确修改：移除验证块，将 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: To enable MCP server support in the Python SDK, I would: (1) remove the Diff comparison: The implementation is correct and matches the TypeScript SDK pattern ( Reuse check: No new code added — just removing a guard and wiring an existing field. Nothing to reuse. Issues found: None. No correctness bugs, no security holes, no convention violations. Before/AfterThis is an SDK behavior change (no TUI impact). Verified with a Python script that exercises the validation path: Before (main)After (this PR)Unit TestsAll 58 tests pass (including the renamed 中文说明代码审查独立方案： 要在 Python SDK 中启用 MCP 服务器支持，我会：(1) 移除 Diff 对比： 实现正确，与 TypeScript SDK 的模式一致。Python SDK 直接将 问题： 无。无正确性 bug、无安全漏洞、无规范违反。 Before/After这是 SDK 行为变更（无 TUI 影响）。通过 Python 脚本验证了验证路径： Before： 全部 58 个测试通过。 — Qwen Code · qwen3.7-max |
|
This is one of the cleanest fork PRs I've reviewed. Eight production lines that do exactly what they say: remove a guard, wire a field. The before/after confirms the behavior change, all 58 tests pass, and the implementation mirrors the TypeScript SDK's established pattern. The problem is real — I verified it by running the validation on No reservations. The scope is tight, the diff is minimal, the test is updated. Ships the feature cleanly. Approving. ✅ 中文说明这是我审查过的最干净的 fork PR 之一。8 行生产代码精确地完成了声明的功能：移除守卫，接入字段。Before/after 确认了行为变更，全部 58 个测试通过，实现与 TypeScript SDK 已建立的模式一致。 问题是真实的——我在 没有顾虑。范围紧凑，diff 最小，测试已更新。干净地交付了功能。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| try: | ||
| payload: dict[str, Any] = {"hooks": None} | ||
| if self._options.mcp_servers: | ||
| payload["mcpServers"] = self._options.mcp_servers |
There was a problem hiding this comment.
[Suggestion] The 2 new production lines that wire mcpServers into the initialize payload have no corresponding unit test. test_accepts_mcp_servers only verifies that validation passes — it does not verify the field actually reaches the CLI process.
A regression here (typo in key name, wrong variable reference) would silently break MCP server support with no test catching it.
Consider adding a test in test_query_core.py:
@pytest.mark.asyncio
async def test_initialize_includes_mcp_servers_in_payload() -> None:
transport = FakeTransport()
servers = {"my-server": {"command": "node", "args": []}}
query = Query(
transport=transport,
options=QueryOptions(mcp_servers=servers, timeout=TimeoutOptions(...)),
prompt="hello",
session_id=VALID_UUID,
)
await query._ensure_started()
init_request = await _wait_for_request(transport, "initialize")
assert init_request["request"]["mcpServers"] == servers
await query.close()Also worth testing the negative case: when mcp_servers is None, mcpServers should not appear in the payload.
— qwen3.7-max via Qwen Code /review
| @@ -64,12 +64,6 @@ def validate_query_options(options: QueryOptions) -> None: | |||
| ): | |||
| raise ValidationError("path_to_qwen_executable cannot be empty") | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] The PR removes the reject-all validation for mcp_servers without adding any positive validation on inner dict values. The TypeScript SDK validates server configs against CLIMcpServerConfigSchema (Zod); the Python SDK now accepts any dict[str, dict[str, Any]] — including {"server": {}} (empty config, no transport type) or {"server": {"command": 123}} (wrong type).
Malformed configs will pass through silently and be dropped by the CLI's normalizeMcpServerConfig with only a debug-level log — the user gets no error, their MCP server simply doesn't appear.
Consider adding minimal validation:
if options.mcp_servers:
for name, cfg in options.mcp_servers.items():
if not isinstance(cfg, dict):
raise ValidationError(f"mcp_servers['{name}'] must be a dict")
if not any(k in cfg for k in ("command", "url", "httpUrl", "tcp")):
raise ValidationError(
f"mcp_servers['{name}'] must include at least one of: "
"command, url, httpUrl, tcp"
)— qwen3.7-max via Qwen Code /review
| async def _initialize(self) -> None: | ||
| try: | ||
| payload: dict[str, Any] = {"hooks": None} | ||
| if self._options.mcp_servers: |
There was a problem hiding this comment.
[Critical] The 2 new lines adding mcpServers to the initialize payload have no test coverage. The existing test_accepts_mcp_servers only verifies that validation doesn't raise — it doesn't assert the wire payload. A regression that drops or renames the key would go undetected.
Add a unit test in test_query_core.py that constructs a Query with mcp_servers set and asserts the captured initialize request payload contains "mcpServers":
options = QueryOptions(mcp_servers={"my-server": {"command": "node", "args": []}})
# ... start query, capture initialize request via FakeTransport ...
assert init_request["mcpServers"] == {"my-server": {"command": "node", "args": []}}— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
💡 Suggestion: Consolidate SDK PRsHi @juhuan, thanks for the comprehensive SDK work! We noticed you have 15 open PRs that all modify the same core files ( The problem
Suggestion: regroup into 2 PRsWe recommend closing the current 15 PRs and reopening them as 2 consolidated PRs: PR 1 — Covers pure SDK-side option additions (~9 current PRs):
PR 2 — Covers features that also involve CLI-side
This keeps a reasonable separation of concerns while eliminating the merge-conflict chain and making review much more manageable. /cc @juhuan |
|
Closing in favor of consolidated PRs (see suggestion comment above). Please reopen as 2 grouped PRs. |
3 participants
What this PR does
Removes the validation that explicitly rejected
mcp_serversin the Python SDK. CLI MCP server configs are now sent to the CLI during theinitializecontrol request, matching the TypeScript SDK's pattern.The Python SDK already had
mcp_servers: dict[str, dict[str, Any]]inQueryOptionsandQueryOptionsDict, andfrom_mappingalready parsed it correctly. The only blocker was:validation.pyraised aValidationErrorwhenmcp_serverswas setquery.py's_initialize()method did not includemcpServersin the initialize payloadBoth are now fixed. The CLI's
SystemController.handleInitialize()already handles external MCP server configs received through themcpServersfield in the initialize request.What's supported
CLI MCP servers (external servers managed by the CLI process): stdio, SSE, streamable HTTP, WebSocket. These are the same configs the TypeScript SDK supports via
CLIMcpServerConfigSchema.What's not supported
In-process SDK MCP servers (
type: 'sdk'with a server instance). The TypeScript SDK supports these viaSdkMcpServerConfig, but this requires a Python MCP server runtime that doesn't exist yet. This is tracked separately in #4889.Changes
Python SDK (
packages/sdk-python):validation.py— removed themcp_serversrejection blockquery.py—_initialize()now includesmcpServersin the initialize payload when configuredtest_validation.py— replacedtest_rejects_mcp_serverswithtest_accepts_mcp_serversWhy it's needed
The Python SDK is the primary SDK for programmatic access to qwen-code, but it couldn't configure MCP servers — a core feature for extending the CLI with custom tools. Users had to use the TypeScript SDK or configure MCP servers in settings files instead of programmatically. This unblocks the most common use case: defining MCP servers in code when creating a query.
Reviewer Test Plan
How to verify
cd packages/sdk-python && PYTHONPATH=src python3 -m pytest tests/unit/test_validation.py tests/unit/test_transport.py -v— 34 tests pass, includingtest_accepts_mcp_serversEvidence (Before & After)
N/A — SDK option change, no user-visible TUI change
Tested on
Environment
Unit tests only: Python 3.11 with pytest 9.1
Risk & Scope
ValidationErrorformcp_serverswill no longer trigger. This is the intended behavior change.mcp_serversrejection should update to handle the new behavior.Linked Issues
Closes #4889 (partially — CLI MCP servers only, not in-process SDK MCP servers)
中文说明
本 PR 做了什么
移除了 Python SDK 中明确拒绝
mcp_servers的验证逻辑。CLI MCP 服务器配置现在通过initialize控制请求发送给 CLI，与 TypeScript SDK 的模式一致。Python SDK 此前已有
mcp_servers字段在QueryOptions和QueryOptionsDict中，from_mapping也能正确解析。唯一的阻塞点是：validation.py在设置mcp_servers时抛出ValidationErrorquery.py的_initialize()方法未在初始化载荷中包含mcpServers两者均已修复。CLI 的
SystemController.handleInitialize()已经能处理通过mcpServers字段接收的外部 MCP 服务器配置。支持范围
CLI MCP 服务器（由 CLI 进程管理的外部服务器）：stdio、SSE、streamable HTTP、WebSocket。
不支持
进程内 SDK MCP 服务器（
type: 'sdk'带服务器实例），需要 Python MCP 服务器运行时，跟踪于 #4889。风险与范围
mcp_serversValidationError的代码不再触发。这是预期的行为变更。mcp_servers被拒绝的代码需更新以处理新行为。