-
Notifications
You must be signed in to change notification settings - Fork 851
Update MCP contents to extend Function contents #7245
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 simplifies the MCP server tool invocation approval mechanism by consolidating approval types. Instead of having separate McpServerToolApprovalRequestContent and McpServerToolApprovalResponseContent classes, it makes McpServerToolCallContent and McpServerToolResultContent inherit from FunctionCallContent and FunctionResultContent respectively, and reuses the existing FunctionApprovalRequestContent and FunctionApprovalResponseContent for MCP approvals.
Changes:
- Removed
McpServerToolApprovalRequestContentandMcpServerToolApprovalResponseContentclasses - Made
McpServerToolCallContentextendFunctionCallContentandMcpServerToolResultContentextendFunctionResultContent - Added
InvocationRequiredproperty toFunctionCallContent(defaults totrue, set tofalsefor MCP tools) - Updated OpenAI integration and tests to work with the unified approval model
- Added API compatibility suppressions for breaking changes
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
McpServerToolApprovalRequestContent.cs |
Deleted - approval now handled through FunctionApprovalRequestContent |
McpServerToolApprovalResponseContent.cs |
Deleted - approval now handled through FunctionApprovalResponseContent |
McpServerToolCallContent.cs |
Now extends FunctionCallContent, inherits Name, CallId, and Arguments properties, sets InvocationRequired = false |
McpServerToolResultContent.cs |
Now extends FunctionResultContent, simplified to only add CallId through base constructor |
FunctionCallContent.cs |
Added InvocationRequired property and polymorphic JSON serialization for McpServerToolCallContent |
FunctionResultContent.cs |
Added polymorphic JSON serialization for McpServerToolResultContent |
UserInputRequestContent.cs |
Removed McpServerToolApprovalRequestContent from JSON derived types |
UserInputResponseContent.cs |
Removed McpServerToolApprovalResponseContent from JSON derived types |
AIContent.cs |
Updated comments to reflect removed types |
OpenAIResponsesChatClient.cs |
Updated to create FunctionApprovalRequestContent wrapping McpServerToolCallContent instead of separate MCP approval types, updated error handling to use Result property |
OpenAIJsonContext.cs |
Removed IReadOnlyDictionary<string, object?> serialization (now uses IDictionary) |
AIJsonUtilities.Defaults.cs |
Removed MCP approval types from JSON serialization registration |
FunctionInvokingChatClient.cs |
Updated approval filtering to check InvocationRequired property on function calls |
CompatibilitySuppressions.xml |
Added suppressions for removed types and changed properties across all target frameworks |
| Test files | Updated to use new API, added tests for mixed MCP/function approval scenarios, fixed namespace |
...ies/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs
Outdated
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionResultContent.cs
Outdated
Show resolved
Hide resolved
|
@copilot please revise the PR summary based on the current state of the PR |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Updated the PR summary to accurately reflect the current state of the PR. The summary now correctly describes that |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…alization and tests
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 26 out of 26 changed files in this pull request and generated no new comments.
| Output = [mtci.Error is not null ? | ||
| new ErrorContent(mtci.Error.ToString()) : | ||
| new TextContent(mtci.ToolOutput)], | ||
| Result = mtci.Error ?? (object)mtci.ToolOutput, |
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.
I think this is not a big concern for McpServerToolResultContent because it's content intended for the LLM and just included in the messages for information reasons.
There's no clear way to identify results as errors. Using ErrorContent was also somewhat of a bug because OpenAI's McpToolCallItem.Error is BinaryData so we were turning that into string in order to pack it in ErrorContent.
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.
Using ErrorContent was also somewhat of a bug
Can you elaborate? McpToolCallItem.Error for whatever reason is modeled in the client as BinaryData, but it's a string in OpenAI's docs:
https://platform.openai.com/docs/api-reference/responses/object#responses-object-output-mcp_tool_call-error
which just means the client library is choosing to represent it as bytes.
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.
OpenAI docs are wrong, in my testing, error is returned as object. See also openai/openai-dotnet#596 (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.
If it's a json object that can still be represented as a string. The binary data is effectively just a utf8 string.
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.
OK, that's fair. FICC sets Result as "Error: Function failed" when an AIFunction throws, do we want to
- align this with that and return
$"Error: {mtci.Error.ToString()}"or - Keep wrapping with ErrorContent to keep fidelity with previous behavior?
If we go with the second, I don't think we would want to change FICC to wrap errors in ErrorContent since it may be too disruptive.
Summary
This PR consolidates MCP server tool approval flow by making
McpServerToolCallContentandMcpServerToolResultContentinherit fromFunctionCallContentandFunctionResultContentrespectively, instead of having separate approval types.Changes
McpServerToolCallContentextendFunctionCallContentandMcpServerToolResultContentextendFunctionResultContentMcpServerToolApprovalRequestContentandMcpServerToolApprovalResponseContentclasses - MCP approvals now use the existingFunctionApprovalRequestContentandFunctionApprovalResponseContentInvocationRequiredproperty toFunctionCallContent(defaults totrue). ForMcpServerToolCallContent, this defaults tofalsesince MCP tool calls are informationalOpenAIResponsesChatClientto work with the unified approval modelCompatibilitySuppressions.xmlImpact
This reduces complexity by reusing existing function call/result abstractions rather than maintaining parallel MCP-specific approval types. The approval mechanism now works consistently for both regular function calls and MCP server tool calls.
Original prompt
This pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Microsoft Reviewers: Open in CodeFlow