-
Notifications
You must be signed in to change notification settings - Fork 849
Add content to represent service-side actions #7109
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 introduces ServiceActionContent as a new base class for content types representing service-side actions. It refactors CodeInterpreterToolCallContent, CodeInterpreterToolResultContent, McpServerToolCallContent, and McpServerToolResultContent to extend this base class, consolidating the common Id property (previously CallId). This change improves the type hierarchy and provides a cleaner abstraction for service-hosted operations.
Key Changes
- Introduces
ServiceActionContentbase class with a requiredIdproperty - Updates code interpreter and MCP server tool content classes to inherit from
ServiceActionContent - Standardizes property naming from
CallIdtoIdacross all service action content types - Makes
ida required constructor parameter (previously nullable/optional)
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/ServiceActionContent.cs |
New base class for service-side actions with required Id property |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CodeInterpreterToolCallContent.cs |
Updated to extend ServiceActionContent and require id in constructor |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/CodeInterpreterToolResultContent.cs |
Updated to extend ServiceActionContent and require id in constructor |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolCallContent.cs |
Updated to extend ServiceActionContent, removing duplicate CallId property |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolResultContent.cs |
Updated to extend ServiceActionContent, removing duplicate CallId property |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/AIContent.cs |
Added ServiceActionContent to polymorphic type comments |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Defaults.cs |
Registered ServiceActionContent for JSON serialization |
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs |
Updated coalescing logic to use Id property |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponsesChatClient.cs |
Updated to use Id property and new constructors |
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantsChatClient.cs |
Updated to use new constructors with required id |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/ServiceActionContentTests.cs |
New comprehensive test suite for ServiceActionContent |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/CodeInterpreterToolCallContentTests.cs |
Updated tests for constructor changes and added serialization test |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/CodeInterpreterToolResultContentTests.cs |
Updated tests for constructor changes and added validation test |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/McpServerToolCallContentTests.cs |
Updated tests for property renaming and added serialization test |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/McpServerToolResultContentTests.cs |
Updated tests for property renaming |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/AIContentTests.cs |
Added ServiceActionContent to polymorphic serialization test |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientTests.cs |
Updated integration tests to use Id property |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIResponseClientIntegrationTests.cs |
Updated integration tests to use Id property |
test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIAssistantChatClientIntegrationTests.cs |
Simplified assertions to use required Id property |
|
@jozkee, thanks. Does this base class alone solve much? The only property it's consolidating is Id. Are there situations where we'd special-case / type-check for this base class? In a hypothetical future when we add support for a server-side OpenAPI tool, does this save us from needing to introduce a full set of types like those for MCP? |
Only be the recommended type to use as base
No, I was thinking more of this as useful to avoid that the FICC invokes already-processed FCCs. I realize now that is still cumbersome without a
I think a ServiceFuncionCallContent would. |
|
Could we achieve the same thing with #7126 ? What if we were to unseal FCC/FRC so that types like the MCP ones could derive from them, or is that conflating too much? |
Yes, FCC.Exception would be meaningless and FCC.Name is odd in MCP content since we have two kinds of names, ServerName and ToolName. Maybe if we moved some things from FCC to an abstract |
Name is tool name. That one makes sense to me. If you were to use an McpClientTool client side, the Name you get back would match.
It would just always be null, since nothing is invoked. It will similarly be null if the function doesn't throw, or if the FCC represents a call handled by the IChatClient itself, or if the FCC represents another server-side invocation (which is just a subset of the IChatClient handling the invocation itself). |
You mean the Overall, it sounds fine to me then. Would we also unseal FunctionResultContent and extend McpServerToolResultContent? The result type is simpler. |
Introduce ServiceActionContent and implement it in CodeInterpreter and McpServerTool contents.
Naming considerations:
Contributes to #6492 (comment).
Microsoft Reviewers: Open in CodeFlow