-
Notifications
You must be signed in to change notification settings - Fork 849
Unseal FunctionCallContent/FunctionResultContent; make MCP types extend them #7201
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
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
…ew approval types Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
…ypes Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
jozkee
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.
@copilot please fix errors and address feedback.
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalRequestContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolResultContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponsesChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponsesChatClient.cs
Outdated
Show resolved
Hide resolved
...ies/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs
Show resolved
Hide resolved
…nts properties, fix idToContentMapping Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
…Dictionary, use Result instead of Output Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
All feedback addressed in commits 598f78b, 23d5d20, and 29454f9. Changes include:
|
|
|
||
| /// <summary> | ||
| /// Represents a request for user approval of a function call. | ||
| /// Represents a request for user approval of a call content. |
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.
this must be reverted too
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.
Done in dd30286. Reverted parameter documentation from "requires user approval" back to "requires approval".
|
|
||
| /// <summary> | ||
| /// Represents a response to a function approval request. | ||
| /// Represents a response to an approval request. |
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.
ditto
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.
Done in dd30286. Reverted parameter documentation from "requires user approval" back to "for which approval was requested".
| /// <exception cref="ArgumentException"><paramref name="callId"/> or <paramref name="toolName"/> is empty or composed entirely of whitespace.</exception> | ||
| [JsonConstructor] | ||
| public McpServerToolCallContent(string callId, string toolName, string? serverName) | ||
| : base(callId, toolName, arguments: null) |
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.
@copilot remove arguments.
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.
Done in dd30286. Removed arguments: null parameter from base constructor call.
| message.Contents.Add(new FunctionApprovalRequestContent(mtcari.Id, new McpServerToolCallContent(mtcari.Id, mtcari.ToolName, mtcari.ServerLabel) | ||
| { | ||
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject)!, | ||
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject) as IDictionary<string, object?>, |
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.
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject) as IDictionary<string, object?>, | |
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments, OpenAIJsonContext.Default.IDictionaryStringObject), | |
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.
Done in dd30286. Changed to use OpenAIJsonContext.Default.IDictionaryStringObject and removed .ToMemory().Span and cast.
| yield return CreateUpdate(new FunctionApprovalRequestContent(mtcari.Id, new McpServerToolCallContent(mtcari.Id, mtcari.ToolName, mtcari.ServerLabel) | ||
| { | ||
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject)!, | ||
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject) as IDictionary<string, object?>, |
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.
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject) as IDictionary<string, object?>, | |
| Arguments = JsonSerializer.Deserialize(mtcari.ToolArguments, OpenAIJsonContext.Default.IDictionaryStringObject), |
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.
Done in dd30286. Changed to use OpenAIJsonContext.Default.IDictionaryStringObject and removed .ToMemory().Span and cast.
| mcpCall.CallId, | ||
| mcpCall.ServerName, | ||
| mcpCall.ToolName, | ||
| BinaryData.FromBytes(JsonSerializer.SerializeToUtf8Bytes(mcpCall.Arguments!, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject))); |
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.
@copilot this should serialize as IDictionary<string,object?>, use the same pattern as case 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.
Done in dd30286. Changed to serialize using OpenAIJsonContext.Default.IDictionaryStringObject.
| callContent.Name, | ||
| BinaryData.FromBytes(JsonSerializer.SerializeToUtf8Bytes( | ||
| callContent.Arguments, | ||
| AIJsonUtilities.DefaultOptions.GetTypeInfo(typeof(IDictionary<string, object?>))))); |
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.
@copilot can this use OpenAIJsonContext.Default.IDictionaryStringObject?
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.
Done in dd30286. Changed from AIJsonUtilities.DefaultOptions.GetTypeInfo(typeof(IDictionary<string, object?>)) to OpenAIJsonContext.Default.IDictionaryStringObject.
| contents.Add(new McpServerToolCallContent(mtci.Id, mtci.ToolName, mtci.ServerLabel) | ||
| { | ||
| Arguments = JsonSerializer.Deserialize(mtci.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject)!, | ||
| Arguments = JsonSerializer.Deserialize(mtci.ToolArguments.ToMemory().Span, OpenAIJsonContext.Default.IReadOnlyDictionaryStringObject) as IDictionary<string, object?>, |
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.
@copilot deserialize as IDictionary<string,object?>, use JsonSerializer.Deserialize(utf8json, OpenAIJsonContext.Default.IDictionaryStringObject).
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.
Done in dd30286. Changed to deserialize using OpenAIJsonContext.Default.IDictionaryStringObject and removed .ToMemory().Span and cast.
| { | ||
| Throw.InvalidOperationException( | ||
| $"FunctionApprovalRequestContent found with FunctionCall.CallId(s) '{string.Join(", ", approvalRequestCallIds)}' that have no matching FunctionApprovalResponseContent."); | ||
| $"FunctionApprovalRequestContent found with CallId(s) '{string.Join(", ", approvalRequestCallIds)}' that have no matching FunctionApprovalResponseContent."); |
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.
@copilot undo this since we no longer need to change the property name.
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.
Done in dd30286. Reverted error message from "CallId(s)" back to "FunctionCall.CallId(s)".
| /// <summary> | ||
| /// Gets the FunctionCallContent from the response. | ||
| /// </summary> | ||
| public FunctionCallContent FunctionCallContent => Response.FunctionCall; |
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.
@copilot we can also undo this since we reverted FunctionApprovalResponseContent.FunctionCall to keep using concrete 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.
Done in dd30286. Simplified the struct by removing the XML doc comment and directly accessing Response.FunctionCall.
…ctionaryStringObject Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Undo unneeded changes Updated OpenAIResponsesChatClient Updated tests
Address Review Feedback - All Comments Resolved ✅
All new review comments have been addressed:
Documentation Changes
Constructor Changes
arguments: nullparameter from base constructor callJSON Serialization Changes
✅ Changed all deserializations from
IReadOnlyDictionaryStringObjecttoIDictionaryStringObject:✅ Changed all serializations to use
IDictionaryStringObject:FunctionInvokingChatClient Changes
All changes maintain the breaking changes we accepted (using
IDictionaryinstead ofIReadOnlyDictionary) while simplifying the code.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.
Microsoft Reviewers: Open in CodeFlow