feat(core): support glob patterns in mcp.allowed and mcp.excluded - #6012
Conversation
Add matchesServerPattern/matchesAnyServerPattern helpers that support * (any sequence) and ? (single char) glob syntax. Apply symmetrically to both allow and deny predicates in getMcpServers, isMcpServerDisabled, and getMcpServerUnavailableReason. Existing exact-match configs are unaffected (no glob chars → string equality). Update settings schema descriptions to document the new glob support. Closes #4940 (retargeted to glob matching; the deny-list capability already existed as mcp.excluded).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Thanks for the PR, @DennisYu07! Gate Template: headings aren't a verbatim match for Direction Solves a real admin pain point: when you manage a fleet of MCP servers and want to block or allow a family by name fragment, exact-match forces tedious enumeration. Approach Scope is tight: three files, +156/-6, one concern, no drive-by refactors, no speculative "let's also support character classes / negation / brace expansion." The diff does the minimal thing — add two helpers, wire them into the four existing call sites, update the schema descriptions, add tests. The unit-test matrix (11 tests) covers the helper, regex-special-char safety, and three Config integration paths ( One non-blocking thought before diving into code: Moving on to code review. 🔍 中文说明感谢 @DennisYu07 的 PR! 门禁 模板:章节标题没有严格对齐 方向 解决了一个真实的管理痛点:管理一堆 MCP server 时,想按名字片段批量放行或封禁,精确匹配只能逐个枚举。 方案 范围紧凑:3 个文件、+156/-6、一个关注点、没有顺手重构、没有"顺便支持字符类 / 取反 / 大括号展开"的投机式扩展。diff 做的是最小化改动——加两个 helper、接到 4 个既有调用点、更新 schema 描述、补测试。单测矩阵(11 个)覆盖了 helper 本身、正则特殊字符安全、Config 的三条集成路径( 一个不卡人的小建议: 进入代码审查。🔍 — Qwen Code · qwen3.7-max |
2a. Code reviewIndependent baseline before reading the diff: I would have added a What the PR actually does: almost exactly that, except it rolls its own regex instead of delegating to No correctness blockers. A few observations worth the contributor's consideration:
Code is clean, focused, and well-commented without over-narrating. Tests are colocated and idiomatic. 2b. Real-scenario testingDrove the PR's compiled output directly via Unit tests (PR helpers)All 10 new helper tests pass (6 Config integration tests (PR paths)Covers Compiled-module behavior (scenarios from the PR body)Each line matches the PR's description exactly: VerdictFeature behaves as advertised under the compiled PR code. No regressions in the surrounding Config test block (full 中文说明2a. 代码审查读 diff 之前我自己的方案:加一个 PR 实际做的:几乎一模一样,区别是用自己写的正则而不是 没有正确性阻塞项。几个值得贡献者考虑的观察:
代码干净、聚焦、注释到位不过度。测试按项目惯例就近放置。 2b. 真实场景测试直接驱动 PR 分支的编译产物 + vitest。 单测(PR helper)10 个新 helper 测试全过(6 个 Config 集成测试(PR 路径)覆盖 编译后模块行为(按 PR 正文里的场景)每一行都和 PR 描述一致: 结论在 PR 编译产物上,功能符合预期。周边 Config 测试块没有回归(完整的 — Qwen Code · qwen3.7-max |
|
Stepping back: this is a well-scoped feature that earns its place. The motivation is real (enterprise admins blocking or allowing families of MCP servers), the diff is the minimal change the goal needs, and the compiled PR code produces exactly the outputs the body promises. My independent baseline was "add a The one thing I'd still like the contributor to think about before shipping: Approving. ✅ 中文说明退一步看:这是一个范围拿捏得当、实至名归的功能。动机真实(企业管理员要按族系批量放行或封禁 MCP server),diff 是目标所需的最小改动,PR 的编译产物给出的输出和正文描述完全一致。我独立基线是"加一个 合并前我还想让贡献者再考虑一件事: 通过。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Missed call site in getBlockedMcpServers()
packages/core/src/config/config.ts:5126 still uses this.allowedMcpServers?.includes(key) instead of matchesAnyServerPattern(key, this.allowedMcpServers). All 4 other call sites were correctly migrated, but this one was missed.
When mcp.allowed contains a glob like ["*puppeteer*"], .includes() compares server names against the literal string "*puppeteer*", so no real server name ever matches. The method reports every configured server as "blocked" while getMcpServers() correctly allows them — a visible inconsistency in the UI.
const isAllowed = matchesAnyServerPattern(key, this.allowedMcpServers);
[Suggestion] Pattern: glob-unaware .includes() in enable/disable paths
Three additional call sites still use .includes() on excluded lists, which silently misbehave when glob patterns are present:
packages/core/src/tools/tool-registry.ts:452—currentExcluded.includes(serverName)appends redundant exact entries when a glob already covers the serverpackages/cli/src/acp-integration/acpAgent.ts:5599— enable action's.includes()can't find a server excluded by glob, making re-enable a silent no-oppackages/cli/src/acp-integration/acpAgent.ts:5635,5642— disable action writes redundant exact entries to persistent settings when a glob already covers the server
The disable path (5635/5642) is especially concerning because it persists stale entries to the settings file that survive even after the glob is removed.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] getBlockedMcpServers() at line 5126 still uses this.allowedMcpServers?.includes(key) (exact match) instead of matchesAnyServerPattern(). The PR updated 4 call sites but missed this 5th one. When a user configures mcp.allowed: ["*puppeteer*"], servers that are actively allowed will still appear as "blocked" in the /mcp UI because "*puppeteer*".includes("puppeteer") is false. Fix:
const isAllowed = matchesAnyServerPattern(key, this.allowedMcpServers);[Suggestion] No test covers the allow/exclude precedence interaction with glob patterns. A server matching both an allow glob and an exclude glob should be excluded (per the mcp.excluded description: "Takes precedence over mcp.allowed"). Consider adding:
it('exclude takes precedence over allow with glob patterns', () => {
config.setAllowedMcpServers(['*']);
config.setExcludedMcpServers(['puppeteer']);
expect(config.getMcpServerUnavailableReason('puppeteer')).toBe('excluded');
expect(config.getMcpServerUnavailableReason('playwright')).toBeUndefined();
});…ching - getBlockedMcpServers() used exact match, causing UI inconsistency when mcp.allowed contained glob patterns (critical) - tool-registry.ts disable action added redundant exact entries when a glob already covered the server - acpAgent.ts enable/disable paths silently misbehaved with globs: enable was a no-op, disable persisted stale exact entries Also add test for exclude-takes-precedence-over-allow with glob patterns.
The regex-based glob conversion (*→.*, ?→.) was vulnerable to catastrophic backtracking with pathological patterns like *?*?*?*. Replaced with an iterative two-pointer algorithm that runs in O(n×m) worst case with no backtracking.
qqqys
left a comment
There was a problem hiding this comment.
[Critical] enable now removes every excluded pattern that matches the one server being enabled (packages/cli/src/acp-integration/acpAgent.ts:5605). If an admin has mcp.excluded: ["*puppeteer*"] and a client enables only puppeteer, this code deletes the *puppeteer* rule from user/workspace settings and from the runtime config. That also enables puppeteer-extra, my-puppeteer-server, and any future server covered by the same deny glob.
That turns a per-server enable action into a broad persistent allow-boundary change. The safe behavior is to avoid deleting glob patterns for a single concrete server. Either only remove an exact serverName entry, or introduce an explicit override/allow mechanism that can represent "this one server is enabled despite a broader excluded glob" without dropping the broader deny rule.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Write paths (enable/disable) and hot-reload still use exact string comparison
The PR makes the read paths glob-aware (isMcpServerDisabled, getMcpServerUnavailableReason, getMcpServers) but leaves several write paths using exact string comparison. This creates silent failures when glob patterns are in use:
-
ACP enable action (
acpAgent.ts:5599-5609):currentExcluded.includes(serverName)and.filter(name => name !== serverName)cannot detect or remove glob patterns. Ifmcp.excluded: ["*puppeteer*"]and the user enablespuppeteer, the glob survives the filter,isMcpServerDisabled("puppeteer")still returnstrue, and the function returns{ ok: true, changed: true }— a silent lie. Same issue inMCPManagementDialog.tsx,McpServerActionsView.tsx, andInstalledTab.tsx. -
ACP disable action (
acpAgent.ts:5635, 5642):currentExcluded.includes(serverName)for the dedup guard returnsfalsewhen a glob already covers the server, so a redundant literal entry is appended. Over repeated enable/disable cycles the list accumulates stale entries. -
tool-registry.ts:452: Same dedup guard issue —
currentExcluded.includes(serverName)misses glob coverage. -
hot-reload.ts:101 (
recomputeMcpGating):settingsAllowed.filter(n => bootAllowed.includes(n))intersects settings patterns with the CLI--allowed-mcp-server-namesflag using exact match. Any glob entry like"*puppeteer*"in settings is silently dropped during hot-reload because"*puppeteer*"never equals any literal server name inbootAllowed.
Suggested fix: Use matchesAnyServerPattern / matchesServerPattern in all write-path call sites. For the enable action, filter out patterns that match the target server: currentExcluded.filter(p => !matchesServerPattern(serverName, p)). For the disable dedup guard, use matchesAnyServerPattern(serverName, currentExcluded). For hot-reload, keep a settings pattern if it matches at least one CLI-flag name.
— qwen3.7-max via Qwen Code /review
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
DragonnZhang
left a comment
There was a problem hiding this comment.
No additional findings beyond those already raised. The four open issues are well-covered: (1) getBlockedMcpServers() at config.ts:5126 still uses .includes() instead of matchesAnyServerPattern — already flagged by wenshao and qwen-code-ci-bot; (2) the enable action's filter(!matchesServerPattern(serverName, pattern)) removes broader glob patterns when a single concrete server is enabled, silently expanding what gets enabled — flagged by qqqys; (3) the disable dedup guard still uses .includes() so redundant exact entries accumulate in settings when a glob already covers the server; (4) hot-reload.ts recomputeMcpGating intersects settings patterns with CLI flag names using exact match, silently dropping glob entries. The matchesServerPattern implementation itself (iterative two-pointer, no regex) is correct and the unit tests are comprehensive.
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
Review comment resolutionsCritical fixes (2 bugs fixed)Enable action collateral damage (#3492449022, #3494965422) Disable action wrong changed status (#3492449032) ReDoS (#3492052871) Dangling submodule (#3492449005) Test coverage added (6 gaps filled)
Declined (with reasoning)
|
- Enable action: only remove exact-match patterns from exclusion list, preserving glob patterns to prevent collateral server re-enable - Disable action: return accurate changed status (false when glob already covers the server, no write needed) - Add 8 new test cases: two-pointer edge cases, negative assertions, getBlockedMcpServers glob, exclude-over-allow with both globs, regex special char $ coverage Addresses PR #6012 review feedback.
update
| unregisterGoalHook, | ||
| ToolNames, | ||
| FORK_SUBAGENT_TYPE, | ||
| matchesAnyServerPattern, |
There was a problem hiding this comment.
[Suggestion] This import is used to update 4 call sites in this PR, but 7 additional call sites across 3 UI components still use .includes() for the same exclusion/allowance checks:
MCPManagementDialog.tsx(lines 455, 538, 584)McpServerActionsView.tsx(lines 292, 329)InstalledTab.tsx(lines 569, 584)
This creates a behavioral inconsistency: enabling a glob-excluded server via the ACP API removes the glob pattern (with the collateral-damage issue flagged elsewhere), while the TUI enable at MCPManagementDialog.tsx:455 (currentExcluded.includes(server.name)) fails to find the glob entry and silently no-ops — the server stays excluded with no user feedback. The same user action produces different results depending on which UI surface they use.
Suggested fix: apply the same matchesAnyServerPattern check to all 7 remaining call sites, or extract a shared helper.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] UI disable call sites still use .includes() (3 locations)
McpServerActionsView.tsx:329, InstalledTab.tsx:569, MCPManagementDialog.tsx:538 — all check !excluded.includes(serverName) before adding to the excluded list, but the list is now glob-interpreted. When a glob like *puppeteer* already covers the server, these append a redundant literal entry. The enable branches in these files also use exact-match removal, so glob-based exclusions cannot be toggled from the UI.
Replace .includes(name) with matchesAnyServerPattern(name, excluded) at all three sites.
[Suggestion] Hot-reload bootAllowed intersection uses .includes() — breaks with glob in mcp.allowed
hot-reload.ts:101 — recomputeMcpGating intersects settingsAllowed with CLI --allowed-mcp-server-names via bootAllowed.includes(n). When mcp.allowed contains a glob like ["*puppeteer*"], the filter bootAllowed.includes("*puppeteer*") returns false (bootAllowed has exact names), so the glob is dropped and all glob-allowed servers are silently removed on hot-reload.
? settingsAllowed.filter((n) =>
bootAllowed.includes(n) || matchesAnyServerPattern(n, bootAllowed),
)
— qwen3.7-max via Qwen Code /review
| 'mcp.excluded', | ||
| currentExcluded.filter((name: string) => name !== serverName), | ||
| ); | ||
| const filtered = currentExcluded.filter( |
There was a problem hiding this comment.
[Critical] Enable action uses exact string comparison to filter mcp.excluded, while the disable action (line 5651) uses glob-aware matchesAnyServerPattern. When mcp.excluded contains a glob like ["*puppeteer*"], calling enable for "puppeteer" evaluates "*puppeteer*" !== "puppeteer" → true for all entries, so the glob is never removed. The action returns {ok: true, changed: false} — the server stays excluded but the caller gets a success response.
This is the headline scenario of the PR: an admin excludes servers via glob (the new feature), then can't selectively re-enable one through the normal enable flow. The collateral-damage fix (exact-only removal) solved the opposite problem but created this gap.
| const filtered = currentExcluded.filter( | |
| const filtered = currentExcluded.filter( | |
| (pattern: string) => !matchesServerPattern(serverName, pattern), | |
| ); |
Apply the same change to the runtime filter at line 5614. Trade-off: removing *puppeteer* also un-excludes my-puppeteer — consider documenting this or narrowing to patterns that exclusively match this server.
— qwen3.7-max via Qwen Code /review
| const currentExcluded = this.config.getExcludedMcpServers() || []; | ||
| if (!currentExcluded.includes(serverName)) { | ||
| if (!matchesAnyServerPattern(serverName, currentExcluded)) { | ||
| this.config.setExcludedMcpServers([...currentExcluded, serverName]); |
There was a problem hiding this comment.
[Suggestion] Auto-disable writes raw serverName literals into the excluded list, which is now glob-interpreted. If a server name contains * or ? (legal — the schema imposes no restriction), the appended entry is re-interpreted as a glob on subsequent reads. Example: server test?v2 fails → auto-excluded as ['test?v2'] → matchesServerPattern('testXv2', 'test?v2') returns true, disabling an unrelated server.
Escape glob metacharacters when appending literals, or document the constraint that server names must not contain */?.
— qwen3.7-max via Qwen Code /review
| return { | ||
| serverName, | ||
| action, | ||
| ok: true, |
There was a problem hiding this comment.
[Suggestion] The changed: false return path is now reachable (when the server was already in the desired state) but has zero test coverage. The PR changed both enable and disable to return changed: settingsChanged || runtimeChanged instead of hardcoded true. If changed is consumed by UI or orchestration logic, an incorrect value could cause stale state displays.
Add test cases asserting { changed: false } when: (a) enabling a server not in the excluded list, and (b) disabling a server already matched by an existing glob.
— qwen3.7-max via Qwen Code /review
Replace .includes() with matchesAnyServerPattern() in 4 UI call sites (McpServerActionsView, InstalledTab, MCPManagementDialog) so that disabling a server already covered by a glob pattern does not add a redundant exact-match entry. Addresses wenshao review on PR #6012.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| * `?` matches exactly one character. A pattern without glob characters is | ||
| * compared as an exact string (no behavior change for existing configs). | ||
| * Uses an iterative two-pointer algorithm — O(n×m) worst case, no regex, | ||
| * no backtracking vulnerability. |
There was a problem hiding this comment.
[Suggestion] The JSDoc claims "no backtracking vulnerability" but the two-pointer algorithm does use bounded backtracking via the starPi/starNi save-and-restore mechanism (lines 660–661). This conflates "no catastrophic/exponential backtracking" (true — O(n×m)) with "no backtracking at all" (false). A future maintainer may assume the algorithm never revisits positions and make incorrect modification assumptions.
| * no backtracking vulnerability. | |
| * O(n×m) worst case, no regex, bounded backtracking via saved star position. |
— qwen3.7-max via Qwen Code /review
| expect(matchesServerPattern('', '')).toBe(true); | ||
| }); | ||
|
|
||
| it('handles consecutive * in pattern', () => { |
There was a problem hiding this comment.
[Suggestion] The matchesServerPattern test suite covers * and ? independently but never combines both in a single pattern. The critical algorithmic path where * backtracking interacts with ? advancement is untested. For example, matchesServerPattern('xab', '*?b') forces * to initially match empty, ? to consume x, b to fail against a, then the backtrack branch resets — the most complex branch interaction in the algorithm.
| it('handles consecutive * in pattern', () => { | |
| it('handles consecutive * in pattern', () => { | |
| expect(matchesServerPattern('puppeteer', '**puppeteer**')).toBe(true); | |
| expect(matchesServerPattern('abc', 'a**c')).toBe(true); | |
| }); | |
| it('backtracks through ? after * when literal forces reset', () => { | |
| expect(matchesServerPattern('xab', '*?b')).toBe(true); | |
| expect(matchesServerPattern('xyab', '*?b')).toBe(true); | |
| expect(matchesServerPattern('xb', '*?b')).toBe(true); | |
| expect(matchesServerPattern('b', '*?b')).toBe(false); | |
| }); |
— qwen3.7-max via Qwen Code /review
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
QwenLM#6090) Audit docs/ against the current codebase and correct user-facing drift: - Document glob-pattern support (* and ?) for mcp.allowed / mcp.excluded in settings.md and the MCP feature page (feat QwenLM#6012). - Add missing user-facing settings rows: general.terminalBell, general.preventSystemSleep, general.chatRecording; ui.showStatusInTitle, ui.disableWorkflowKeywordTrigger, ui.enableUserFeedback, ui.compactInline, ui.useTerminalBuffer, ui.hideBuiltinWorktreeIndicator; memory.enableTeamMemory, memory.enableTeamMemorySync; tools.toolSearch.enabled. - Note the QWEN_MODEL alias for OPENAI_MODEL in the auth protocol table. - Document the autonomous (bare /loop) mode in scheduled-tasks (feat QwenLM#5991). Co-authored-by: Claude <noreply@anthropic.com>
What this PR does
Add glob pattern support (
*and?) tomcp.allowedandmcp.excludedsettings. Previously these lists only supported exact server name matching viaArray.includes, requiring administrators to enumerate every server name individually. Now a single pattern like"*puppeteer*"can match all servers whose name containspuppeteer. Two new helpers —matchesServerPatternandmatchesAnyServerPattern— are used symmetrically in 4 call sites across the allow and deny predicates. Patterns without glob characters fall back to exact string equality, so existing configs are unaffected.Why it's needed
Enterprise administrators managing MCP server policies need to block or allow families of related servers without listing each one. For example, blocking all puppeteer-related servers (puppeteer, puppeteer-extra, my-puppeteer-server) currently requires three separate entries. With glob support, one
"*puppeteer*"entry covers all of them. This aligns with Claude Code'sdeniedMcpServerspredicate semantics, which already supports pattern matching.Reviewer Test Plan
How to verify
settings.jsonand configure"mcp": { "excluded": ["*puppeteer*"] }:{ "mcpServers": { "puppeteer": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-puppeteer"] }, "puppeteer-extra": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-puppeteer"] }, "filesystem": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"] }, "playwright": { "command": "npx", "args": ["-y", "@playwright/mcp@latest"] } }, "mcp": { "excluded": ["*puppeteer*"] } }npm run dev, open/mcp—puppeteerandpuppeteer-extrashould show as excluded/blocked;filesystemandplaywrightshould be available."allowed": ["play*"](remove excluded) — onlyplaywrightshould be loaded.cd packages/core && npx vitest run src/config/config.test.ts -t "matchesServerPattern"and-t "matchesAnyServerPattern"and-t "glob pattern".Evidence (Before & After)
Before:

mcp.excludedonly matched exact names —"puppeteer"would not block"puppeteer-extra".After:

mcp.excluded: ["*puppeteer*"]blocks bothpuppeteerandpuppeteer-extrawhile leavingfilesystemandplaywrightunaffected.Tested on
Environment (optional)
npm run devfrom source, Node 22, no sandbox.Risk & Scope
*or?use the same===comparison as before. Regex special characters in server names (.,+,^, etc.) are properly escaped.mcp.allowedandmcp.excludedare affected).Linked Issues
Closes #4940
中文说明
这个 PR 做了什么
为
mcp.allowed和mcp.excluded设置项添加 glob 通配符支持(*和?)。之前这两个列表只支持通过Array.includes精确匹配服务器名称,管理员必须逐条枚举每个服务器名。现在一个"*puppeteer*"模式就能匹配所有名称包含puppeteer的服务器。新增matchesServerPattern和matchesAnyServerPattern两个 helper,在 allow 和 deny 谓词的 4 个调用点对称使用。不含 glob 字符的模式回退到精确字符串比较,现有配置零影响。为什么需要
企业管理员管理 MCP 服务器策略时,需要批量屏蔽或放行一类相关服务器,而不是逐个列名。例如屏蔽所有 puppeteer 相关服务器(puppeteer、puppeteer-extra、my-puppeteer-server),目前需要写三条。有了 glob 支持,一条
"*puppeteer*"就全部覆盖。这也与 Claude Code 的deniedMcpServers谓词语义对齐,后者已支持模式匹配。Reviewer 测试计划
如何验证
settings.json中添加多个 MCP 服务器并配置"mcp": { "excluded": ["*puppeteer*"] }:{ "mcpServers": { "puppeteer": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-puppeteer"] }, "puppeteer-extra": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-puppeteer"] }, "filesystem": { "command": "npx", "args": ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"] }, "playwright": { "command": "npx", "args": ["-y", "@playwright/mcp@latest"] } }, "mcp": { "excluded": ["*puppeteer*"] } }npm run dev,输入/mcp—puppeteer和puppeteer-extra应显示为已排除/已屏蔽;filesystem和playwright应可用。"allowed": ["play*"](移除 excluded)— 只有playwright应被加载。cd packages/core && npx vitest run src/config/config.test.ts -t "matchesServerPattern"和-t "matchesAnyServerPattern"和-t "glob pattern"。证据(改动前后)
改动前:
mcp.excluded只能精确匹配 —"puppeteer"不会屏蔽"puppeteer-extra"。改动后:
mcp.excluded: ["*puppeteer*"]同时屏蔽puppeteer和puppeteer-extra,filesystem和playwright不受影响。测试环境
环境(可选)
从源码
npm run dev运行,Node 22,无沙箱。风险与范围
*或?的模式使用与之前相同的===比较。服务器名中的正则特殊字符(.、+、^等)已正确转义。mcp.allowed和mcp.excluded)。关联 Issue
Closes #4940