Skip to content

feat(sdk): add allowed_mcp_server_names option to both SDKs - #6474

Closed
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-mcp-options
Closed

feat(sdk): add allowed_mcp_server_names option to both SDKs#6474
juhuan wants to merge 1 commit into
QwenLM:mainfrom
juhuan:feat/sdk-mcp-options

Conversation

@juhuan

@juhuan juhuan commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Add allowed_mcp_server_names (Python) / allowedMcpServerNames (TS) option that maps to CLI's --allowed-mcp-server-names flag, allowing users to whitelist which MCP servers are available for a session.

Note: strict_mcp_config was not implemented as the CLI does not have this flag.

Test plan

  • Python SDK tests pass (pytest — 58 passed)
  • TypeScript SDK typecheck passes (tsc --noEmit)
  • TypeScript SDK tests pass (vitest run — 1163 passed)

Add `allowed_mcp_server_names` (Python) / `allowedMcpServerNames` (TS)
option that maps to CLI's `--allowed-mcp-server-names` flag, allowing
users to whitelist which MCP servers are available for a session.

Note: `strict_mcp_config` was not implemented as the CLI does not have
this flag.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

Hi @juhuan — thanks for the PR! The code change itself looks clean and follows the existing SDK patterns perfectly. However, the PR body doesn't follow our pull request template.

The template requires these sections, which are currently missing:

  • What this PR does / Why it's needed — the "Summary" section covers what, but the why (motivation, use case) is absent. Why do SDK users need to whitelist MCP servers?
  • Reviewer Test Plan — how a reviewer can verify the change works end-to-end (not just "tests pass"). A short scenario like "create a query with allowedMcpServerNames: ['server-a'], confirm only server-a's tools are available" would suffice.
  • Tested on — which OS(es) you verified on.
  • Risk & Scope — even "minimal, additive-only, no breaking changes" is useful.
  • Linked Issues — is there an issue this addresses?

Could you update the PR description to follow the template? Happy to re-review once that's done.

中文说明

你好 @juhuan —— 感谢提交!代码改动本身很干净,完全遵循了 SDK 现有的模式。但 PR 正文没有按照我们的 PR 模板 填写。

模板要求以下章节,目前缺失:

  • What this PR does / Why it's needed —— "Summary" 说明了做了什么,但缺少为什么(动机、使用场景)。SDK 用户为什么需要白名单 MCP servers?
  • Reviewer Test Plan —— 审查者如何端到端验证这个改动(不只是"测试通过")。一个简短的场景就够了,比如"用 allowedMcpServerNames: ['server-a'] 创建 query,确认只有 server-a 的工具可用"。
  • Tested on —— 在哪些操作系统上验证过。
  • Risk & Scope —— 即使是"最小改动、纯新增、无破坏性变更"也有用。
  • Linked Issues —— 是否有关联的 issue?

请按模板更新 PR 描述,完成后我会重新审查。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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] Missing test coverage for the new allowed_mcp_server_names / allowedMcpServerNames option in both SDKs.

  • packages/sdk-python/tests/unit/test_transport.py:25test_build_cli_arguments_maps_supported_options tests allowed_tools but not allowed_mcp_server_names. Add allowed_mcp_server_names=["my-mcp-server"] to the QueryOptions(...) constructor and "--allowed-mcp-server-names", "my-mcp-server" to the expected args list.
  • packages/sdk-typescript/test/unit/ProcessTransport.test.ts:180 — "should build CLI arguments correctly with all options" tests coreTools/excludeTools but not allowedMcpServerNames. Add allowedMcpServerNames: ['my-mcp-server'] to the options object and '--allowed-mcp-server-names', 'my-mcp-server' to the expect.arrayContaining([...]) assertion.

[Suggestion] Both SDK READMEs document all existing options but were not updated for the new option. Add allowedMcpServerNames to the options table in packages/sdk-typescript/README.md and allowed_mcp_server_names to the options list in packages/sdk-python/README.md.

— qwen3.7-max via Qwen Code /review

args.extend(["--allowed-tools", ",".join(options.allowed_tools)])

if options.allowed_mcp_server_names:
args.extend(["--allowed-mcp-server-names", ",".join(options.allowed_mcp_server_names)])

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] This line is 95 characters, exceeding the project's ruff line-length = 88 configured in pyproject.toml. This will fail ruff check.

Suggested change
args.extend(["--allowed-mcp-server-names", ",".join(options.allowed_mcp_server_names)])
args.extend(
["--allowed-mcp-server-names", ",".join(options.allowed_mcp_server_names)]
)

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

💡 Suggestion: Consolidate SDK PRs

Hi @juhuan, thanks for the comprehensive SDK work! We noticed you have 15 open PRs that all modify the same core files (transport.py, types.py, queryOptionsSchema.ts, types.ts, ProcessTransport.ts, createQuery.ts) and were created on the same day.

The problem

  • Merge conflicts: Since all 15 PRs touch the same files, whichever merges first will cause conflicts in the remaining 14.
  • Review overhead: Reviewing 15 near-identical PRs separately is inefficient and risks fatigue.
  • CI cost: 15 separate CI runs for the same lint/typecheck passes.

Suggestion: regroup into 2 PRs

We recommend closing the current 15 PRs and reopening them as 2 consolidated PRs:

PR 1 — feat(sdk): expose transport and query options in both SDKs

Covers pure SDK-side option additions (~9 current PRs):

PR 2 — feat(sdk): add control request methods to both SDKs

Covers features that also involve CLI-side ControlDispatcher changes (~4 current PRs):

This keeps a reasonable separation of concerns while eliminating the merge-conflict chain and making review much more manageable.

/cc @juhuan

@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Closing in favor of consolidated PRs (see suggestion comment above). Please reopen as 2 grouped PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants