Skip to content

Commit 18b97aa

Browse files
abhipatel12abhipatel12
committed
refactor: standardize MCP tool naming to mcp_ FQN format
1 parent 7d31d5f commit 18b97aa

16 files changed

Lines changed: 465 additions & 351 deletions

‎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

‎packages/core/src/core/loggingContentGenerator.test.ts‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -709,7 +709,7 @@ describe('estimateContextBreakdown', () => {
709709
{
710710
functionDeclarations: [
711711
{
712-
name: 'myserver__search',
712+
name: 'mcp_myserver_search',
713713
description: 'Search via MCP',
714714
parameters: {},
715715
},
@@ -747,8 +747,7 @@ describe('estimateContextBreakdown', () => {
747747
expect(builtinOnly.mcp_servers).toBe(0);
748748
});
749749

750-
it('should not classify tools with __ in the middle of a segment as MCP', () => {
751-
// "__" at start or end (not a valid server__tool pattern) should not be MCP
750+
it('should not classify tools without mcp_ prefix as MCP', () => {
752751
const config = {
753752
tools: [
754753
{
@@ -842,7 +841,7 @@ describe('estimateContextBreakdown', () => {
842841
functionDeclarations: [
843842
{ name: 'read_file', description: 'Read', parameters: {} },
844843
{
845-
name: 'myserver__search',
844+
name: 'mcp_myserver_search',
846845
description: 'MCP search',
847846
parameters: {},
848847
},
@@ -858,7 +857,7 @@ describe('estimateContextBreakdown', () => {
858857
expect(result.history).toBeGreaterThan(0);
859858
// tool_calls should only contain non-MCP tools
860859
expect(result.tool_calls['read_file']).toBeGreaterThan(0);
861-
expect(result.tool_calls['myserver__search']).toBeUndefined();
860+
expect(result.tool_calls['mcp_myserver_search']).toBeUndefined();
862861
// MCP tokens are only in mcp_servers
863862
expect(result.mcp_servers).toBeGreaterThan(0);
864863
});
@@ -870,7 +869,7 @@ describe('estimateContextBreakdown', () => {
870869
parts: [
871870
{
872871
functionCall: {
873-
name: 'myserver__search',
872+
name: 'mcp_myserver_search',
874873
args: { query: 'test' },
875874
},
876875
},
@@ -881,7 +880,7 @@ describe('estimateContextBreakdown', () => {
881880
parts: [
882881
{
883882
functionResponse: {
884-
name: 'myserver__search',
883+
name: 'mcp_myserver_search',
885884
response: { results: [] },
886885
},
887886
},
@@ -890,7 +889,7 @@ describe('estimateContextBreakdown', () => {
890889
];
891890
const result = estimateContextBreakdown(contents);
892891
// MCP tool calls should NOT appear in tool_calls
893-
expect(result.tool_calls['myserver__search']).toBeUndefined();
892+
expect(result.tool_calls['mcp_myserver_search']).toBeUndefined();
894893
// MCP call tokens should only be counted in mcp_servers
895894
expect(result.mcp_servers).toBeGreaterThan(0);
896895
});
@@ -908,7 +907,7 @@ describe('estimateContextBreakdown', () => {
908907
},
909908
{
910909
functionCall: {
911-
name: 'myserver__search',
910+
name: 'mcp_myserver_search',
912911
args: { q: 'hello' },
913912
},
914913
},
@@ -919,7 +918,7 @@ describe('estimateContextBreakdown', () => {
919918
// Non-MCP tools should be in tool_calls
920919
expect(result.tool_calls['read_file']).toBeGreaterThan(0);
921920
// MCP tools should NOT be in tool_calls
922-
expect(result.tool_calls['myserver__search']).toBeUndefined();
921+
expect(result.tool_calls['mcp_myserver_search']).toBeUndefined();
923922
// MCP tool calls should only be in mcp_servers
924923
expect(result.mcp_servers).toBeGreaterThan(0);
925924
});

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -478,9 +478,13 @@ describe('Core System Prompt (prompts.ts)', () => {
478478
const prompt = getCoreSystemPrompt(mockConfig);
479479
expect(prompt).toContain('# Active Approval Mode: Plan');
480480
// Read-only MCP tool should appear with server name
481-
expect(prompt).toContain('`read_data` (readonly-server)');
481+
expect(prompt).toContain(
482+
'`mcp_readonly-server_read_data` (readonly-server)',
483+
);
482484
// Non-read-only MCP tool should not appear (excluded by policy)
483-
expect(prompt).not.toContain('`write_data` (nonreadonly-server)');
485+
expect(prompt).not.toContain(
486+
'`mcp_nonreadonly-server_write_data` (nonreadonly-server)',
487+
);
484488
expect(prompt).toMatchSnapshot();
485489
});
486490

@@ -498,8 +502,12 @@ describe('Core System Prompt (prompts.ts)', () => {
498502

499503
const prompt = getCoreSystemPrompt(mockConfig);
500504

501-
expect(prompt).toContain('`read_data` (readonly-server)');
502-
expect(prompt).not.toContain('`write_data` (nonreadonly-server)');
505+
expect(prompt).toContain(
506+
'`mcp_readonly-server_read_data` (readonly-server)',
507+
);
508+
expect(prompt).not.toContain(
509+
'`mcp_nonreadonly-server_write_data` (nonreadonly-server)',
510+
);
503511
});
504512

505513
it('should only list available tools in PLAN mode', () => {

0 commit comments

Comments
 (0)