-
Notifications
You must be signed in to change notification settings - Fork 301
[MCP] Prevent duplicating entities between custom tools and describe_entities #3076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR prevents stored-procedure entities that are already exposed as dedicated custom MCP tools from being duplicated in the describe_entities response.
Changes:
- Added filtering in
DescribeEntitiesToolto skip stored procedures withentity.Mcp.CustomToolEnabled == true. - Added new MSTest coverage validating stored procedure filtering and ensuring tables/views remain unaffected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs | Filters out custom-tool-enabled stored procedures from describe_entities results. |
| src/Service.Tests/Mcp/DescribeEntitiesFilteringTests.cs | Adds tests verifying filtering behavior across stored procedures, tables/views, and nameOnly mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Azure.DataApiBuilder.Mcp/BuiltInTools/DescribeEntitiesTool.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Desired logic
|
| } | ||
| // All entities were filtered because they're custom-tool-only - only return this specific error | ||
| // if ALL configured entities were filtered (not just some). This prevents misleading errors | ||
| // when entities fail to build for other reasons (e.g., exceptions in Build*EntityInfo). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| // when entities fail to build for other reasons (e.g., exceptions in Build*EntityInfo). | |
| // when entities fail to build for other reasons (e.g., exceptions in Build*EntityInfo functions). |
| // All entities were filtered because they're custom-tool-only - only return this specific error | ||
| // if ALL configured entities were filtered (not just some). This prevents misleading errors | ||
| // when entities fail to build for other reasons (e.g., exceptions in Build*EntityInfo). | ||
| else if (filteredCustomToolCount > 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we also want a situation where only some of the entities were filtered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw its purpose is to show the user when all their tools are custom-tools, although it might be a good idea to also give the user the information that their SPs that are custom-tools are in a different place.
| // - custom-tool: true, dml-tools: false → Filtered (appears only in tools/list) | ||
| // - custom-tool: false, dml-tools: false → Filtered (entity fully disabled for MCP) | ||
| // When dml-tools is true (or default), the entity appears in describe_entities. | ||
| if (entity.Source.Type == EntitySourceType.StoredProcedure && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other types of entities (that are not stored procedures), when dmltool enabled is false, do we still need them to be shown in describe_entities?
Or is the dmltoolenabled flag only specific to entities that are stored procs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the dmltoolenabled/Mcp section under entity is non-null only when it is a stored procedure entity, we should not need to check for the type of entity to be stored procedure.
| public async Task DescribeEntities_TablesAndViewsUnaffectedByFiltering() | ||
| { | ||
| // Arrange | ||
| RuntimeConfig config = CreateConfigWithMixedEntityTypes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if some of the tables/views also have dml_tools : false? Shouldnt we filter those out too?
Aniruddh25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on what needs to be done for non stored proc entities
Why make this change?
With custom tools, stored procedure entities could get listed in
describe_entitieseven when they are configured as custom-tool-only (withdml-tools: false). This creates duplication since they already appear intools/list. We need to filter stored procedures withdml-tools: falsefromdescribe_entitiesto avoid this duplication.What is this change?
dml-toolsis explicitly set to falsedescribe_entitiesonly when they are custom-tool-only (not available for DML operations)dml-tools: true(or null/default) continue to appear indescribe_entitieseven if they havecustom-tool: true, allowing dual exposure when neededHow was this tested?
Sample Request(s)