Skip to content

Commit f82d872

Browse files
abhipatel12abhipatel12
authored
refactor(core): standardize MCP tool naming to mcp_ FQN format (#21425)
1 parent 4513b4a commit f82d872

19 files changed

Lines changed: 694 additions & 497 deletions

‎integration-tests/extensions-reload.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ describe('extension reloading', () => {
104104
return (
105105
output.includes(
106106
'test-server (from test-extension) - Ready (1 tool)',
107-
) && output.includes('- hello')
107+
) && output.includes('- mcp_test-server_hello')
108108
);
109109
},
110110
30000, // 30s timeout
@@ -148,7 +148,7 @@ describe('extension reloading', () => {
148148
return (
149149
output.includes(
150150
'test-server (from test-extension) - Ready (1 tool)',
151-
) && output.includes('- goodbye')
151+
) && output.includes('- mcp_test-server_goodbye')
152152
);
153153
},
154154
30000,

‎integration-tests/policy-headless.test.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ async function verifyToolExecution(
6868
promptCommand: PromptCommand,
6969
result: string,
7070
expectAllowed: boolean,
71+
expectedDenialString?: string,
7172
) {
7273
const log = await waitForToolCallLog(
7374
rig,
@@ -78,10 +79,13 @@ async function verifyToolExecution(
7879
if (expectAllowed) {
7980
expect(log!.toolRequest.success).toBe(true);
8081
expect(result).not.toContain('Tool execution denied by policy');
82+
expect(result).not.toContain(`Tool "${promptCommand.tool}" not found`);
8183
expect(result).toContain(promptCommand.expectedSuccessResult);
8284
} else {
8385
expect(log!.toolRequest.success).toBe(false);
84-
expect(result).toContain('Tool execution denied by policy');
86+
expect(result).toContain(
87+
expectedDenialString || 'Tool execution denied by policy',
88+
);
8589
expect(result).toContain(promptCommand.expectedFailureResult);
8690
}
8791
}
@@ -92,6 +96,7 @@ interface TestCase {
9296
promptCommand: PromptCommand;
9397
policyContent?: string;
9498
expectAllowed: boolean;
99+
expectedDenialString?: string;
95100
}
96101

97102
describe('Policy Engine Headless Mode', () => {
@@ -125,7 +130,13 @@ describe('Policy Engine Headless Mode', () => {
125130
approvalMode: 'default',
126131
});
127132

128-
await verifyToolExecution(rig, tc.promptCommand, result, tc.expectAllowed);
133+
await verifyToolExecution(
134+
rig,
135+
tc.promptCommand,
136+
result,
137+
tc.expectAllowed,
138+
tc.expectedDenialString,
139+
);
129140
};
130141

131142
const testCases = [
@@ -134,6 +145,7 @@ describe('Policy Engine Headless Mode', () => {
134145
responsesFile: 'policy-headless-shell-denied.responses',
135146
promptCommand: ECHO_PROMPT,
136147
expectAllowed: false,
148+
expectedDenialString: 'Tool "run_shell_command" not found',
137149
},
138150
{
139151
name: 'should allow ASK_USER tools in headless mode if explicitly allowed via policy file',
@@ -178,6 +190,7 @@ describe('Policy Engine Headless Mode', () => {
178190
priority = 100
179191
`,
180192
expectAllowed: false,
193+
expectedDenialString: 'Tool execution denied by policy',
181194
},
182195
];
183196

‎packages/cli/src/config/policy-engine.integration.test.ts‎

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -93,45 +93,45 @@ describe('Policy Engine Integration Tests', () => {
9393
// Tools from allowed server should be allowed
9494
// Tools from allowed server should be allowed
9595
expect(
96-
(await engine.check({ name: 'allowed-server__tool1' }, undefined))
96+
(await engine.check({ name: 'mcp_allowed-server_tool1' }, undefined))
9797
.decision,
9898
).toBe(PolicyDecision.ALLOW);
9999
expect(
100100
(
101101
await engine.check(
102-
{ name: 'allowed-server__another_tool' },
102+
{ name: 'mcp_allowed-server_another_tool' },
103103
undefined,
104104
)
105105
).decision,
106106
).toBe(PolicyDecision.ALLOW);
107107

108108
// Tools from trusted server should be allowed
109109
expect(
110-
(await engine.check({ name: 'trusted-server__tool1' }, undefined))
110+
(await engine.check({ name: 'mcp_trusted-server_tool1' }, undefined))
111111
.decision,
112112
).toBe(PolicyDecision.ALLOW);
113113
expect(
114114
(
115115
await engine.check(
116-
{ name: 'trusted-server__special_tool' },
116+
{ name: 'mcp_trusted-server_special_tool' },
117117
undefined,
118118
)
119119
).decision,
120120
).toBe(PolicyDecision.ALLOW);
121121

122122
// Tools from blocked server should be denied
123123
expect(
124-
(await engine.check({ name: 'blocked-server__tool1' }, undefined))
124+
(await engine.check({ name: 'mcp_blocked-server_tool1' }, undefined))
125125
.decision,
126126
).toBe(PolicyDecision.DENY);
127127
expect(
128-
(await engine.check({ name: 'blocked-server__any_tool' }, undefined))
128+
(await engine.check({ name: 'mcp_blocked-server_any_tool' }, undefined))
129129
.decision,
130130
).toBe(PolicyDecision.DENY);
131131

132132
// Tools from unknown servers should use default
133133
expect(
134-
(await engine.check({ name: 'unknown-server__tool' }, undefined))
134+
(await engine.check({ name: 'mcp_unknown-server_tool' }, undefined))
135135
.decision,
136136
).toBe(PolicyDecision.ASK_USER);
137137
});
@@ -151,12 +151,16 @@ describe('Policy Engine Integration Tests', () => {
151151

152152
// ANY tool with a server name should be allowed
153153
expect(
154-
(await engine.check({ name: 'mcp-server__tool' }, 'mcp-server'))
154+
(await engine.check({ name: 'mcp_mcp-server_tool' }, 'mcp-server'))
155155
.decision,
156156
).toBe(PolicyDecision.ALLOW);
157157
expect(
158-
(await engine.check({ name: 'another-server__tool' }, 'another-server'))
159-
.decision,
158+
(
159+
await engine.check(
160+
{ name: 'mcp_another-server_tool' },
161+
'another-server',
162+
)
163+
).decision,
160164
).toBe(PolicyDecision.ALLOW);
161165

162166
// Built-in tools should NOT be allowed by the MCP wildcard
@@ -171,7 +175,7 @@ describe('Policy Engine Integration Tests', () => {
171175
allowed: ['my-server'],
172176
},
173177
tools: {
174-
exclude: ['my-server__dangerous-tool'],
178+
exclude: ['mcp_my-server_dangerous-tool'],
175179
},
176180
};
177181

@@ -184,20 +188,24 @@ describe('Policy Engine Integration Tests', () => {
184188
// MCP server allowed (priority 4.1) provides general allow for server
185189
// MCP server allowed (priority 4.1) provides general allow for server
186190
expect(
187-
(await engine.check({ name: 'my-server__safe-tool' }, undefined))
191+
(await engine.check({ name: 'mcp_my-server_safe-tool' }, undefined))
188192
.decision,
189193
).toBe(PolicyDecision.ALLOW);
190194
// But specific tool exclude (priority 4.4) wins over server allow
191195
expect(
192-
(await engine.check({ name: 'my-server__dangerous-tool' }, undefined))
193-
.decision,
196+
(
197+
await engine.check(
198+
{ name: 'mcp_my-server_dangerous-tool' },
199+
undefined,
200+
)
201+
).decision,
194202
).toBe(PolicyDecision.DENY);
195203
});
196204

197205
it('should handle complex mixed configurations', async () => {
198206
const settings: Settings = {
199207
tools: {
200-
allowed: ['custom-tool', 'my-server__special-tool'],
208+
allowed: ['custom-tool', 'mcp_my-server_special-tool'],
201209
exclude: ['glob', 'dangerous-tool'],
202210
},
203211
mcp: {
@@ -242,21 +250,21 @@ describe('Policy Engine Integration Tests', () => {
242250
(await engine.check({ name: 'custom-tool' }, undefined)).decision,
243251
).toBe(PolicyDecision.ALLOW);
244252
expect(
245-
(await engine.check({ name: 'my-server__special-tool' }, undefined))
253+
(await engine.check({ name: 'mcp_my-server_special-tool' }, undefined))
246254
.decision,
247255
).toBe(PolicyDecision.ALLOW);
248256

249257
// MCP server tools
250258
expect(
251-
(await engine.check({ name: 'allowed-server__tool' }, undefined))
259+
(await engine.check({ name: 'mcp_allowed-server_tool' }, undefined))
252260
.decision,
253261
).toBe(PolicyDecision.ALLOW);
254262
expect(
255-
(await engine.check({ name: 'trusted-server__tool' }, undefined))
263+
(await engine.check({ name: 'mcp_trusted-server_tool' }, undefined))
256264
.decision,
257265
).toBe(PolicyDecision.ALLOW);
258266
expect(
259-
(await engine.check({ name: 'blocked-server__tool' }, undefined))
267+
(await engine.check({ name: 'mcp_blocked-server_tool' }, undefined))
260268
.decision,
261269
).toBe(PolicyDecision.DENY);
262270

@@ -483,7 +491,7 @@ describe('Policy Engine Integration Tests', () => {
483491
expect(blockedToolRule?.priority).toBe(4.4); // Command line exclude
484492

485493
const blockedServerRule = rules.find(
486-
(r) => r.toolName === 'blocked-server__*',
494+
(r) => r.toolName === 'mcp_blocked-server_*',
487495
);
488496
expect(blockedServerRule?.priority).toBe(4.9); // MCP server exclude
489497

@@ -493,11 +501,13 @@ describe('Policy Engine Integration Tests', () => {
493501
expect(specificToolRule?.priority).toBe(4.3); // Command line allow
494502

495503
const trustedServerRule = rules.find(
496-
(r) => r.toolName === 'trusted-server__*',
504+
(r) => r.toolName === 'mcp_trusted-server_*',
497505
);
498506
expect(trustedServerRule?.priority).toBe(4.2); // MCP trusted server
499507

500-
const mcpServerRule = rules.find((r) => r.toolName === 'mcp-server__*');
508+
const mcpServerRule = rules.find(
509+
(r) => r.toolName === 'mcp_mcp-server_*',
510+
);
501511
expect(mcpServerRule?.priority).toBe(4.1); // MCP allowed server
502512

503513
const readOnlyToolRule = rules.find((r) => r.toolName === 'glob');
@@ -509,18 +519,19 @@ describe('Policy Engine Integration Tests', () => {
509519
(await engine.check({ name: 'blocked-tool' }, undefined)).decision,
510520
).toBe(PolicyDecision.DENY);
511521
expect(
512-
(await engine.check({ name: 'blocked-server__any' }, undefined))
522+
(await engine.check({ name: 'mcp_blocked-server_any' }, undefined))
513523
.decision,
514524
).toBe(PolicyDecision.DENY);
515525
expect(
516526
(await engine.check({ name: 'specific-tool' }, undefined)).decision,
517527
).toBe(PolicyDecision.ALLOW);
518528
expect(
519-
(await engine.check({ name: 'trusted-server__any' }, undefined))
529+
(await engine.check({ name: 'mcp_trusted-server_any' }, undefined))
520530
.decision,
521531
).toBe(PolicyDecision.ALLOW);
522532
expect(
523-
(await engine.check({ name: 'mcp-server__any' }, undefined)).decision,
533+
(await engine.check({ name: 'mcp_mcp-server_any' }, undefined))
534+
.decision,
524535
).toBe(PolicyDecision.ALLOW);
525536
expect((await engine.check({ name: 'glob' }, undefined)).decision).toBe(
526537
PolicyDecision.ALLOW,
@@ -549,7 +560,7 @@ describe('Policy Engine Integration Tests', () => {
549560

550561
// Exclusion (195) should win over trust (90)
551562
expect(
552-
(await engine.check({ name: 'conflicted-server__tool' }, undefined))
563+
(await engine.check({ name: 'mcp_conflicted-server_tool' }, undefined))
553564
.decision,
554565
).toBe(PolicyDecision.DENY);
555566
});
@@ -560,7 +571,7 @@ describe('Policy Engine Integration Tests', () => {
560571
excluded: ['my-server'], // Priority 195 - DENY
561572
},
562573
tools: {
563-
allowed: ['my-server__special-tool'], // Priority 100 - ALLOW
574+
allowed: ['mcp_my-server_special-tool'], // Priority 100 - ALLOW
564575
},
565576
};
566577

@@ -573,11 +584,11 @@ describe('Policy Engine Integration Tests', () => {
573584
// Server exclusion (195) wins over specific tool allow (100)
574585
// This might be counterintuitive but follows the priority system
575586
expect(
576-
(await engine.check({ name: 'my-server__special-tool' }, undefined))
587+
(await engine.check({ name: 'mcp_my-server_special-tool' }, undefined))
577588
.decision,
578589
).toBe(PolicyDecision.DENY);
579590
expect(
580-
(await engine.check({ name: 'my-server__other-tool' }, undefined))
591+
(await engine.check({ name: 'mcp_my-server_other-tool' }, undefined))
581592
.decision,
582593
).toBe(PolicyDecision.DENY);
583594
});
@@ -647,13 +658,13 @@ describe('Policy Engine Integration Tests', () => {
647658
const tool3Rule = rules.find((r) => r.toolName === 'tool3');
648659
expect(tool3Rule?.priority).toBe(4.4); // Excluded tools (user tier)
649660

650-
const server2Rule = rules.find((r) => r.toolName === 'server2__*');
661+
const server2Rule = rules.find((r) => r.toolName === 'mcp_server2_*');
651662
expect(server2Rule?.priority).toBe(4.9); // Excluded servers (user tier)
652663

653664
const tool1Rule = rules.find((r) => r.toolName === 'tool1');
654665
expect(tool1Rule?.priority).toBe(4.3); // Allowed tools (user tier)
655666

656-
const server1Rule = rules.find((r) => r.toolName === 'server1__*');
667+
const server1Rule = rules.find((r) => r.toolName === 'mcp_server1_*');
657668
expect(server1Rule?.priority).toBe(4.1); // Allowed servers (user tier)
658669

659670
const globRule = rules.find((r) => r.toolName === 'glob');

‎packages/core/src/agents/local-executor.test.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,7 @@ import { debugLogger } from '../utils/debugLogger.js';
1717
import { LocalAgentExecutor, type ActivityCallback } from './local-executor.js';
1818
import { makeFakeConfig } from '../test-utils/config.js';
1919
import { ToolRegistry } from '../tools/tool-registry.js';
20-
import {
21-
DiscoveredMCPTool,
22-
MCP_QUALIFIED_NAME_SEPARATOR,
23-
} from '../tools/mcp-tool.js';
20+
import { DiscoveredMCPTool } from '../tools/mcp-tool.js';
2421
import { LSTool } from '../tools/ls.js';
2522
import { LS_TOOL_NAME, READ_FILE_TOOL_NAME } from '../tools/tool-names.js';
2623
import {
@@ -503,7 +500,7 @@ describe('LocalAgentExecutor', () => {
503500
it('should automatically qualify MCP tools in agent definitions', async () => {
504501
const serverName = 'mcp-server';
505502
const toolName = 'mcp-tool';
506-
const qualifiedName = `${serverName}${MCP_QUALIFIED_NAME_SEPARATOR}${toolName}`;
503+
const qualifiedName = `mcp_${serverName}_${toolName}`;
507504

508505
const mockMcpTool = {
509506
tool: vi.fn(),

‎packages/core/src/core/__snapshots__/prompts.test.ts.snap‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ The following tools are available in Plan Mode:
105105
<tool>\`exit_plan_mode\`</tool>
106106
<tool>\`write_file\`</tool>
107107
<tool>\`replace\`</tool>
108-
<tool>\`read_data\` (readonly-server)</tool>
108+
<tool>\`mcp_readonly-server_read_data\` (readonly-server)</tool>
109109
</available_tools>
110110

111111
## Rules
@@ -275,7 +275,7 @@ The following tools are available in Plan Mode:
275275
<tool>\`exit_plan_mode\`</tool>
276276
<tool>\`write_file\`</tool>
277277
<tool>\`replace\`</tool>
278-
<tool>\`read_data\` (readonly-server)</tool>
278+
<tool>\`mcp_readonly-server_read_data\` (readonly-server)</tool>
279279
</available_tools>
280280

281281
## Rules
@@ -564,7 +564,7 @@ The following tools are available in Plan Mode:
564564
<tool>\`exit_plan_mode\`</tool>
565565
<tool>\`write_file\`</tool>
566566
<tool>\`replace\`</tool>
567-
<tool>\`read_data\` (readonly-server)</tool>
567+
<tool>\`mcp_readonly-server_read_data\` (readonly-server)</tool>
568568
</available_tools>
569569

570570
## Rules

0 commit comments

Comments
 (0)