-
Notifications
You must be signed in to change notification settings - Fork 846
Replace Mcp approvals with Function approvals #7104
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 refactors the approval mechanism for tool calls by replacing MCP-specific approval types (McpServerToolApprovalRequestContent and McpServerToolApprovalResponseContent) with generalized FunctionApprovalRequestContent and FunctionApprovalResponseContent types. This allows the function approval system to handle both regular function calls and MCP server tool calls uniformly.
Key Changes:
- Deleted MCP-specific approval content types and consolidated them into the existing Function approval types
- Changed
FunctionApprovalRequestContentandFunctionApprovalResponseContentto acceptAIContentinstead ofFunctionCallContent, enabling support for multiple call types (e.g.,FunctionCallContent,McpServerToolCallContent) - Updated property names from
FunctionCalltoCallContentto reflect the more general nature - Added pattern matching logic in
FunctionInvokingChatClientto handle mixed approval scenarios where onlyFunctionCallContentapprovals are processed - Updated OpenAI response client to map MCP approval items to the generalized approval types
- Added comprehensive tests for mixed approval scenarios
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolApprovalRequestContent.cs |
Deleted - functionality replaced by generalized FunctionApprovalRequestContent |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolApprovalResponseContent.cs |
Deleted - functionality replaced by generalized FunctionApprovalResponseContent |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalRequestContent.cs |
Changed to accept AIContent instead of FunctionCallContent; renamed property FunctionCall → CallContent |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs |
Changed to accept AIContent instead of FunctionCallContent; renamed property FunctionCall → CallContent |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/UserInputRequestContent.cs |
Removed McpServerToolApprovalRequestContent from JSON polymorphic type hierarchy |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/UserInputResponseContent.cs |
Removed McpServerToolApprovalResponseContent from JSON polymorphic type hierarchy |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/AIContent.cs |
Removed commented-out type discriminators for deleted MCP approval types |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Defaults.cs |
Removed JSON serialization registration for deleted MCP approval types |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs |
Added pattern matching to process only FunctionCallContent from approval types; updated struct to be readonly with init properties; fixed spelling error |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponsesChatClient.cs |
Updated to map MCP approval items to generalized FunctionApprovalRequestContent and FunctionApprovalResponseContent with McpServerToolCallContent |
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs |
Changed namespace; updated assertions to use CallContent property; added comprehensive tests for mixed approval scenarios |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientTests.cs |
Updated test assertions to use generalized approval types |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs |
Updated test assertions and approval response creation logic; reordered test execution calls |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionApprovalRequestContentTests.cs |
Updated tests to use CallContent property and added type assertions for deserialized content |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionApprovalResponseContentTests.cs |
Updated tests to use CallContent property and added type assertions for deserialized content |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/UserInputRequestContentTests.cs |
Commented out MCP-specific test case; updated deserialization type |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/UserInputResponseContentTests.cs |
Commented out MCP-specific test case with note about experimental status |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/AIContentTests.cs |
Updated test to use generalized approval types with MCP content |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs
Show resolved
Hide resolved
| switch (content) | ||
| { | ||
| case FunctionApprovalRequestContent farc: | ||
| // Skip if CallContent is not FunctionCallContent |
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.
Can you do this instead with a when clause on the case? (same for the next case below)
| public FunctionApprovalResponseContent Response { get; set; } | ||
| public ChatMessage? RequestMessage { get; set; } | ||
| public FunctionApprovalResponseContent Response { get; init; } | ||
| public FunctionCallContent FunctionCallContent { get; init; } |
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.
Why is this needed? Is that just to make accessing this less verbose? Otherwise isn't this just Response.CallContent?
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.
Because Response.CallContent is now AIContent, I would have to do a cast or type check later if I don't save it here.
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.
Is that just to make accessing this less verbose?
Yes but not for the sake of using less dots/properties.
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 concern is perf, the extra size is very likely more impactful. If you don't want to cast at the call site, I suggest changing the property to access/cast it from Response.
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.
It wasn't for perf, it felt less risky to have the concrete-type property and leverage that the initialization already validated it was a FunctionCallContent. I applied your suggestion anyway since is functionally the same.
Strawman proposal for replacing
McpServerToolApprovalRequestContentandMcpServerToolApprovalResponseContentwith Function approval types.#6492 (comment)
cc @westey-m
Microsoft Reviewers: Open in CodeFlow