Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions OpenClaw-Query-Submit
Submodule OpenClaw-Query-Submit added at be6657
45 changes: 33 additions & 12 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ import {
unregisterGoalHook,
ToolNames,
FORK_SUBAGENT_TYPE,
matchesAnyServerPattern,

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

} from '@qwen-code/qwen-code-core';
Comment thread
DennisYu07 marked this conversation as resolved.
import { randomUUID } from 'node:crypto';
import type {
Expand Down Expand Up @@ -5596,23 +5597,34 @@ class QwenAgent implements Agent {

if (action === 'enable') {
const settings = loadSettings(this.config.getTargetDir());
let settingsChanged = false;
for (const scope of [SettingScope.User, SettingScope.Workspace]) {
const scopeSettings = settings.forScope(scope).settings;
const currentExcluded = scopeSettings.mcp?.excluded || [];
if (currentExcluded.includes(serverName)) {
settings.setValue(
scope,
'mcp.excluded',
currentExcluded.filter((name: string) => name !== serverName),
);
const filtered = currentExcluded.filter(

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

Suggested change
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

(pattern: string) => pattern !== serverName,
);
if (filtered.length !== currentExcluded.length) {
settings.setValue(scope, 'mcp.excluded', filtered);
settingsChanged = true;
}
}
const currentExcluded = this.config.getExcludedMcpServers() || [];
this.config.setExcludedMcpServers(
currentExcluded.filter((name: string) => name !== serverName),
const runtimeFiltered = currentExcluded.filter(
(pattern: string) => pattern !== serverName,
);
let runtimeChanged = false;
if (runtimeFiltered.length !== currentExcluded.length) {
this.config.setExcludedMcpServers(runtimeFiltered);
runtimeChanged = true;
}
await toolRegistry.discoverToolsForServer(serverName);
return { serverName, action, ok: true, changed: true };
return {
serverName,
action,
ok: true,

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

changed: settingsChanged || runtimeChanged,
};
}

if (action === 'disable') {
Expand All @@ -5635,18 +5647,27 @@ class QwenAgent implements Agent {
}
const scopeSettings = settings.forScope(targetScope).settings;
const currentExcluded = scopeSettings.mcp?.excluded || [];
if (!currentExcluded.includes(serverName)) {
let settingsChanged = false;
if (!matchesAnyServerPattern(serverName, currentExcluded)) {
Comment thread
DennisYu07 marked this conversation as resolved.
settings.setValue(targetScope, 'mcp.excluded', [
...currentExcluded,
serverName,
]);
settingsChanged = true;
}
const runtimeExcluded = this.config.getExcludedMcpServers() || [];
if (!runtimeExcluded.includes(serverName)) {
let runtimeChanged = false;
if (!matchesAnyServerPattern(serverName, runtimeExcluded)) {
this.config.setExcludedMcpServers([...runtimeExcluded, serverName]);
runtimeChanged = true;
}
await toolRegistry.disableMcpServer(serverName);
return { serverName, action, ok: true, changed: true };
return {
serverName,
action,
ok: true,
changed: settingsChanged || runtimeChanged,
};
}

if (action === 'clear-auth') {
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/config/settingsSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2416,7 +2416,8 @@ const SETTINGS_SCHEMA = {
// restart-required.
requiresRestart: false,
default: undefined as string[] | undefined,
description: 'A list of MCP servers to allow.',
description:
'A list of MCP servers to allow. Supports glob patterns (e.g. "*puppeteer*").',
showInDialog: false,
mergeStrategy: MergeStrategy.CONCAT,
},
Expand All @@ -2429,7 +2430,8 @@ const SETTINGS_SCHEMA = {
// restart-required.
requiresRestart: false,
default: undefined as string[] | undefined,
description: 'A list of MCP servers to exclude.',
description:
'A list of MCP servers to exclude. Supports glob patterns (e.g. "*puppeteer*"). Takes precedence over mcp.allowed.',
showInDialog: false,
mergeStrategy: MergeStrategy.CONCAT,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
mcpServerRequiresOAuth,
MCPOAuthTokenStorage,
createDebugLogger,
matchesAnyServerPattern,
} from '@qwen-code/qwen-code-core';
import {
loadSettings,
Expand Down Expand Up @@ -566,7 +567,7 @@ export const InstalledTab = ({
// Disable: add to excluded + disconnect.
const excluded =
settings.forScope(targetScope).settings.mcp?.excluded ?? [];
if (!excluded.includes(item.name)) {
if (!matchesAnyServerPattern(item.name, excluded)) {
settings.setValue(targetScope, 'mcp.excluded', [
...excluded,
item.name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
DiscoveredMCPTool,
MCPOAuthTokenStorage,
createDebugLogger,
matchesAnyServerPattern,
} from '@qwen-code/qwen-code-core';
import { loadSettings, SettingScope } from '../../../../config/settings.js';
import { getErrorMessage } from '../../../../utils/errors.js';
Expand Down Expand Up @@ -326,7 +327,7 @@ export const McpServerActionsView = ({
: SettingScope.User;
const settings = loadSettings();
const excluded = settings.forScope(scope).settings.mcp?.excluded || [];
if (!excluded.includes(serverName)) {
if (!matchesAnyServerPattern(serverName, excluded)) {
settings.setValue(scope, 'mcp.excluded', [...excluded, serverName]);
}
await toolRegistry?.disableMcpServer(serverName);
Expand Down
9 changes: 5 additions & 4 deletions packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
type AnyDeclarativeTool,
type DiscoveredMCPPrompt,
createDebugLogger,
matchesAnyServerPattern,
} from '@qwen-code/qwen-code-core';
import { loadSettings, SettingScope } from '../../../config/settings.js';
import { loadMcpApprovals } from '../../../config/mcpApprovals.js';
Expand Down Expand Up @@ -534,8 +535,8 @@ export const MCPManagementDialog: React.FC<MCPManagementDialogProps> = ({
).settings;
const currentExcluded = scopeSettings.mcp?.excluded || [];

// If server is not in exclusion list, add it
if (!currentExcluded.includes(server.name)) {
// If server is not already covered by an exclusion pattern, add it
if (!matchesAnyServerPattern(server.name, currentExcluded)) {
const newExcluded = [...currentExcluded, server.name];
settings.setValue(
targetScope === 'user' ? SettingScope.User : SettingScope.Workspace,
Expand Down Expand Up @@ -580,8 +581,8 @@ export const MCPManagementDialog: React.FC<MCPManagementDialogProps> = ({
).settings;
const currentExcluded = scopeSettings.mcp?.excluded || [];

// If server is not in exclusion list, add it
if (!currentExcluded.includes(server.name)) {
// If server is not already covered by an exclusion pattern, add it
if (!matchesAnyServerPattern(server.name, currentExcluded)) {
const newExcluded = [...currentExcluded, server.name];
settings.setValue(
scope === 'user' ? SettingScope.User : SettingScope.Workspace,
Expand Down
207 changes: 207 additions & 0 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
APPROVAL_MODE_INFO,
MCPServerConfig,
TrustGateError,
matchesServerPattern,
matchesAnyServerPattern,
} from './config.js';
import { Storage } from './storage.js';
import * as fs from 'node:fs';
Expand Down Expand Up @@ -344,6 +346,92 @@ vi.mock('../core/toolHookTriggers.js', () => ({
fireNotificationHook: vi.fn().mockResolvedValue({}),
}));

describe('matchesServerPattern', () => {
it('exact match when no glob characters', () => {
expect(matchesServerPattern('puppeteer', 'puppeteer')).toBe(true);
expect(matchesServerPattern('puppeteer', 'playwright')).toBe(false);
});

it('* matches any sequence including empty', () => {
expect(matchesServerPattern('puppeteer', '*puppeteer*')).toBe(true);
expect(matchesServerPattern('my-puppeteer-server', '*puppeteer*')).toBe(
true,
);
expect(matchesServerPattern('playwright', '*puppeteer*')).toBe(false);
expect(matchesServerPattern('anything', '*')).toBe(true);
expect(matchesServerPattern('prefix-suffix', 'prefix*')).toBe(true);
expect(matchesServerPattern('prefix-suffix', '*suffix')).toBe(true);
});

it('? matches exactly one character', () => {
expect(matchesServerPattern('abc', 'a?c')).toBe(true);
expect(matchesServerPattern('ac', 'a?c')).toBe(false);
expect(matchesServerPattern('axc', 'a?c')).toBe(true);
});

it('escapes regex special characters', () => {
expect(matchesServerPattern('my.server', 'my.server')).toBe(true);
expect(matchesServerPattern('myXserver', 'my.server')).toBe(false);
expect(matchesServerPattern('a+b', 'a+b')).toBe(true);
expect(matchesServerPattern('a^b', 'a^b')).toBe(true);
expect(matchesServerPattern('a$b', 'a$b')).toBe(true);
expect(matchesServerPattern('aXb', 'a$b')).toBe(false);
});

it('combines glob with exact segments', () => {
expect(matchesServerPattern('foo-bar-baz', 'foo-*-baz')).toBe(true);
Comment thread
DennisYu07 marked this conversation as resolved.
expect(matchesServerPattern('foo-bar-qux', 'foo-*-baz')).toBe(false);
});

it('handles empty name', () => {
expect(matchesServerPattern('', '*')).toBe(true);
expect(matchesServerPattern('', '?')).toBe(false);
expect(matchesServerPattern('', '')).toBe(true);
});

it('handles consecutive * in pattern', () => {

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

Suggested change
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

expect(matchesServerPattern('puppeteer', '**puppeteer**')).toBe(true);
expect(matchesServerPattern('abc', 'a**c')).toBe(true);
});

it('handles ? at pattern boundaries', () => {
expect(matchesServerPattern('abc', '?bc')).toBe(true);
expect(matchesServerPattern('abc', 'ab?')).toBe(true);
expect(matchesServerPattern('abc', '???')).toBe(true);
expect(matchesServerPattern('ab', '???')).toBe(false);
});

it('rejects when pattern is longer than name', () => {
expect(matchesServerPattern('ab', 'a*b*c')).toBe(false);
expect(matchesServerPattern('abc', 'a*b*c')).toBe(true);
});
});
Comment thread
DennisYu07 marked this conversation as resolved.

describe('matchesAnyServerPattern', () => {
it('returns false for undefined or empty list', () => {
expect(matchesAnyServerPattern('puppeteer', undefined)).toBe(false);
expect(matchesAnyServerPattern('puppeteer', [])).toBe(false);
});

it('matches if any pattern matches', () => {
expect(
matchesAnyServerPattern('puppeteer', ['playwright', '*puppeteer*']),
).toBe(true);
expect(
matchesAnyServerPattern('chrome', ['playwright', '*puppeteer*']),
).toBe(false);
});

it('works with mixed exact and glob patterns', () => {
expect(
matchesAnyServerPattern('playwright', ['playwright', '*puppeteer*']),
).toBe(true);
expect(
matchesAnyServerPattern('my-puppeteer', ['playwright', '*puppeteer*']),
).toBe(true);
Comment thread
DennisYu07 marked this conversation as resolved.
});
});

describe('Server Config (config.ts)', () => {
const MODEL = 'qwen3-coder-plus';

Expand Down Expand Up @@ -1060,6 +1148,125 @@ describe('Server Config (config.ts)', () => {
});
expect(config.getAllowedMcpServers()).toEqual(['y']);
});

it('getMcpServers filters by glob pattern in allowedMcpServers', async () => {
const config = new Config({
...baseParams,
mcpServers: {
puppeteer: srvA,
'my-puppeteer-server': srvB,
playwright: srvA,
},
});
config.setAllowedMcpServers(['*puppeteer*']);
const result = config.getMcpServers();
expect(Object.keys(result!)).toEqual([
Comment thread
DennisYu07 marked this conversation as resolved.
'puppeteer',
'my-puppeteer-server',
]);
expect(Object.keys(result!)).not.toContain('playwright');
});

it('isMcpServerDisabled supports glob patterns in excludedMcpServers', () => {
Comment thread
DennisYu07 marked this conversation as resolved.
const config = new Config({
...baseParams,
mcpServers: {
puppeteer: srvA,
'my-puppeteer': srvA,
playwright: srvB,
},
});
config.setExcludedMcpServers(['*puppeteer*']);
expect(config.isMcpServerDisabled('puppeteer')).toBe(true);
expect(config.isMcpServerDisabled('my-puppeteer')).toBe(true);
expect(config.isMcpServerDisabled('playwright')).toBe(false);
expect(config.getMcpServers()!['puppeteer']).toBeDefined();
expect(config.getMcpServers()!['my-puppeteer']).toBeDefined();
});

it('getMcpServerUnavailableReason classifies by glob match', async () => {
const config = new Config({
...baseParams,
mcpServers: {
puppeteer: srvA,
Comment thread
DennisYu07 marked this conversation as resolved.
playwright: srvB,
chrome: srvA,
},
});
await config.reinitializeMcpServers({
puppeteer: srvA,
playwright: srvB,
chrome: srvA,
});

config.setAllowedMcpServers(['play*']);
expect(config.getMcpServerUnavailableReason('puppeteer')).toBe(
'not_allowed',
);
expect(
config.getMcpServerUnavailableReason('playwright'),
).toBeUndefined();

// Clear allow-list so the excluded check is reached.
config.setAllowedMcpServers(undefined);
config.setExcludedMcpServers(['*chrome*']);
expect(config.getMcpServerUnavailableReason('chrome')).toBe('excluded');
});

it('exclude takes precedence over allow with glob patterns', async () => {
const config = new Config({
...baseParams,
mcpServers: { puppeteer: srvA, playwright: srvB },
});
await config.reinitializeMcpServers({
puppeteer: srvA,
playwright: srvB,
});

config.setAllowedMcpServers(['*']);
config.setExcludedMcpServers(['puppeteer']);
expect(config.getMcpServerUnavailableReason('puppeteer')).toBe(
'excluded',
);
expect(
config.getMcpServerUnavailableReason('playwright'),
).toBeUndefined();
});

it('exclude takes precedence when both lists use globs', async () => {
const config = new Config({
...baseParams,
mcpServers: { puppeteer: srvA, playwright: srvB },
});
await config.reinitializeMcpServers({
puppeteer: srvA,
playwright: srvB,
});

config.setAllowedMcpServers(['*puppeteer*']);
config.setExcludedMcpServers(['puppeteer']);
expect(config.getMcpServerUnavailableReason('puppeteer')).toBe(
'excluded',
);
expect(config.isMcpServerDisabled('puppeteer')).toBe(true);
});

it('getBlockedMcpServers returns servers not matching allowed glob', () => {
const config = new Config({
...baseParams,
mcpServers: {
puppeteer: srvA,
'my-puppeteer': srvA,
playwright: srvB,
},
});
config.setAllowedMcpServers(['*puppeteer*']);
const blocked = config.getBlockedMcpServers();
const blockedNames = blocked.map((s) => s.name);
expect(blockedNames).toContain('playwright');
expect(blockedNames).not.toContain('puppeteer');
expect(blockedNames).not.toContain('my-puppeteer');
});
});

describe('MemoryPressureMonitor isolation', () => {
Expand Down
Loading
Loading