MCP: Validate tool name uniqueness across BuiltIn and Custom tools#3110
MCP: Validate tool name uniqueness across BuiltIn and Custom tools#3110
Conversation
Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com>
Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com>
…st comments Co-authored-by: souvikghosh04 <210500244+souvikghosh04@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds validation to ensure MCP tool names are unique across both built-in and custom tool types. Without this validation, custom tools (generated from stored procedure entities) could conflict with built-in tools like create_record or read_records, causing undefined behavior during tool invocation.
Changes:
- Added duplicate tool name detection in
McpToolRegistry.RegisterTool()that throwsDataApiBuilderExceptionwhen a conflict is detected - Implemented comprehensive test suite with 12 test cases covering various duplicate scenarios, case sensitivity, and error messaging
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/Core/McpToolRegistry.cs | Added validation logic to detect and reject duplicate tool names across BuiltIn and Custom tool types |
| src/Service.Tests/Mcp/McpToolRegistryTests.cs | New test file with comprehensive coverage of duplicate detection scenarios, case sensitivity, and edge cases |
| } | ||
|
|
||
| /// <summary> | ||
| /// Test that registering two built-in tools with the same name throws an exception. |
There was a problem hiding this comment.
You have four near-identical “duplicate name throws” tests. Use data rows to compress them while still asserting nuanced messages.
`[DataTestMethod]
// Duplicate BuiltIn vs BuiltIn
[DataRow("duplicate_tool", ToolType.BuiltIn, ToolType.BuiltIn, "built-in tool registered", "register built-in tool with same name")]
// Duplicate Custom vs Custom
[DataRow("my_custom_tool", ToolType.Custom, ToolType.Custom, "custom tool registered", "Cannot register custom tool with the same name")]
// Custom conflicts with BuiltIn
[DataRow("create_record", ToolType.BuiltIn, ToolType.Custom, "built-in tool registered", "Cannot register custom tool with the same name")]
// BuiltIn conflicts with Custom
[DataRow("my_stored_proc", ToolType.Custom, ToolType.BuiltIn, "custom tool registered", "register built-in tool with same name")]`
| // Check for duplicate tool names | ||
| if (_tools.TryGetValue(metadata.Name, out IMcpTool? existingTool)) | ||
| { | ||
| string existingToolType = existingTool.ToolType == ToolType.BuiltIn ? "built-in" : "custom"; |
There was a problem hiding this comment.
How is case sensitivity treated here. Are 2 tools with same names but different cases, treated as same tools (throw error) or different tools (succeed)?
If case sensitivity is allowed, in that case we will have the DML tool create_record and Create_record, which is not right.
There was a problem hiding this comment.
Good point, the dictionary _tools should use a case insensitive comparer for string keys.
| // Check for duplicate tool names | ||
| if (_tools.TryGetValue(metadata.Name, out IMcpTool? existingTool)) | ||
| { | ||
| string existingToolType = existingTool.ToolType == ToolType.BuiltIn ? "built-in" : "custom"; |
There was a problem hiding this comment.
Are empty tool names allowed? This could be dangerours, one of the test below says empty tool names are accepted, if that is the case, then this should be discussed first that if we want to accept empty tools names.
| throw new NotImplementedException(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Add tests for leading/trailing whitespace and invalid characters
This prevents subtle duplication like "tool", " tool", "tool " passing as unique.
If you want to normalize names (e.g., Trim()), then add a test that "tool" and " tool " collide, and enforce trimming in the registry.
| Assert.IsTrue(exception.Message.Contains("Duplicate MCP tool name 'my_custom_tool' detected")); | ||
| Assert.IsTrue(exception.Message.Contains("custom tool with this name is already registered")); | ||
| Assert.IsTrue(exception.Message.Contains("Cannot register custom tool with the same name")); | ||
| Assert.AreEqual(DataApiBuilderException.SubStatusCodes.ErrorInInitialization, exception.SubStatusCode); |
There was a problem hiding this comment.
Right now, you assert only SubStatusCode. If the production exception sets an HTTP status, assert that too.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
|
||
| /// <summary> | ||
| /// Test that tool name comparison is case-sensitive. | ||
| /// Tools with different casing should be allowed. |
There was a problem hiding this comment.
Should they really be allowed?
| IMcpTool tool2 = new MockMcpTool("", ToolType.Custom); | ||
|
|
||
| // Act - Register first tool with empty name | ||
| registry.RegisterTool(tool1); |
There was a problem hiding this comment.
what is the use of an empty tool name?
| /// but this ensures the tool registry properly detects any duplicates that might occur. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void RegisterTool_WithConflictingSnakeCaseNames_DetectsDuplicates() |
There was a problem hiding this comment.
how is this test different than the previous ones which detected duplicate?
| registry.RegisterTool(tool); | ||
|
|
||
| // Verify tool was registered | ||
| bool found = registry.TryGetTool("unique_tool", out IMcpTool? retrievedTool); |
There was a problem hiding this comment.
no need for this single unique tool test, other tests cover this scenario.
| { | ||
| // Arrange | ||
| McpToolRegistry registry = new(); | ||
| IMcpTool tool1 = new MockMcpTool("tool_one", ToolType.BuiltIn); |
There was a problem hiding this comment.
The only way built in/ custom tools can get duplicated is when a stored proc entity is named as create-record or read-records since those are our built in tool names. We should just add tests for that scenario. other cases of 2 multiple same built in tools / multiple same custom tools are not possible since entity names have to be unique by definition.
We should avoid exploding our test suite to speed up checkin time.
Aniruddh25
left a comment
There was a problem hiding this comment.
Reducing the test matrix to what is really possible.
Why make this change?
MCP tool names must be unique. Without validation, custom tools (generated from stored procedure entities) could conflict with built-in tools (
create_record,read_records, etc.) or other custom tools, causing undefined behavior during tool invocation.What is this change?
Added duplicate detection in
McpToolRegistry.RegisterTool()that throwsDataApiBuilderExceptionon conflict:Files Modified:
McpToolRegistry.cs: Added validation inRegisterTool()Files Added:
McpToolRegistryTests.cs: 12 test cases covering duplicate detection across tool types, case sensitivity, and error messagingValidation occurs during
McpToolRegistryInitializer.StartAsync()ensuring fail-fast at service startup.How was this tested?
Sample Request(s)
N/A - Internal validation, no user-facing API changes
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.