-
Notifications
You must be signed in to change notification settings - Fork 849
Fix approval request/response correlation in FunctionInvokingChatClient #7261
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
Conversation
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.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.
@copilot Update the allApprovalRequestsMessages dictionary to use FunctionApprovalRequestContent.Id and FunctionApprovalResponseContent.Id rather than FunctionCallContent.CallId as the key.
…tent.Id for correlation Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Updated in commit 9034f0e. The dictionary now uses |
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.
We could consider also updating approvalRequestCallIds to be just approvalRequestIds, but I assume that the original function call IDs are more useful for diagnostic purposes than the approval IDs if they ever do happen to be different.
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 fixes approval request/response correlation in FunctionInvokingChatClient so that reconstructed function call messages can reliably reuse the original request message metadata (e.g., MessageId) instead of falling back to synthetic identifiers.
Changes:
- Updated
FunctionInvokingChatClientapproval-response processing to look up the original approval request message byFunctionApprovalResponseContent.Id, matching the dictionary keyFunctionApprovalRequestContent.Id. - Added a focused regression test ensuring that, for both non-streaming and streaming flows, the reconstructed function call message preserves the original request message’s
MessageId.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs | Adds ApprovalResponsePreservesOriginalRequestMessageMetadata regression test verifying that approval processing preserves the original request message MessageId for both buffered and streaming paths. |
| src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs | Adjusts approval response handling to correlate responses with their requests via the shared approval Id, ensuring the original request ChatMessage is recovered and its metadata reused. |
…nctionInvokingChatClientApprovalsTests.cs
...ies/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs
Outdated
Show resolved
Hide resolved
…nctionInvokingChatClientApprovalsTests.cs
Fix approval request/response correlation in FunctionInvokingChatClient
Summary
Fixed an inconsistency in how approval request messages are tracked vs looked up in
FunctionInvokingChatClient.Issue
FunctionApprovalRequestContent.IdapprovalResponse.FunctionCall.CallIdChanges Made
approvalResponse.FunctionCall.CallIdtoapprovalResponse.IdinExtractAndRemoveApprovalRequestsAndResponsesmethodFunctionApprovalRequestContent.Idfor storing approval request messagesApprovalResponsePreservesOriginalRequestMessageMetadatathat validates the fixTest Results
✅ All tests passing:
Original prompt
This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Microsoft Reviewers: Open in CodeFlow