Skip to content

feat(sdk-python): enable mcp_servers support via initialize control request #6463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
juhuan wants to merge 1 commit into from
Closed

feat(sdk-python): enable mcp_servers support via initialize control request #6463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/sdk-python/src/qwen_code_sdk/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ async def _ensure_started(self) -> None:
async def _initialize(self) -> None:
try:
payload: dict[str, Any] = {"hooks": None}
if self._options.mcp_servers:

@wenshao wenshao Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

payload["mcpServers"] = self._options.mcp_servers

@qwen-code-ci-bot qwen-code-ci-bot Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

await self._send_control_request("initialize", payload)
except Exception as exc:
await self._finish_with_error(exc)
Expand Down
6 changes: 0 additions & 6 deletions packages/sdk-python/src/qwen_code_sdk/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,6 @@ def validate_query_options(options: QueryOptions) -> None:
):
raise ValidationError("path_to_qwen_executable cannot be empty")

@qwen-code-ci-bot qwen-code-ci-bot Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

if options.mcp_servers:
raise ValidationError(
"mcp_servers is not supported in Python SDK v1. "
"Remove the mcp_servers option or use the TypeScript SDK."
)


def _validate_optional_callable(
value: object,
Expand Down
9 changes: 4 additions & 5 deletions packages/sdk-python/tests/unit/test_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,7 @@ def test_timeout_rejects_boolean_value() -> None:
TimeoutOptions.from_mapping({"stream_close": True})


def test_rejects_mcp_servers() -> None:
with pytest.raises(ValidationError, match="mcp_servers is not supported"):
validate_query_options(
QueryOptions(mcp_servers={"my-server": {"command": "node", "args": []}})
)
def test_accepts_mcp_servers() -> None:
validate_query_options(
QueryOptions(mcp_servers={"my-server": {"command": "node", "args": []}})
)
Loading