-
Notifications
You must be signed in to change notification settings - Fork 849
Fix message ordering when processing function approval responses #7158
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
…cessing - Track insertion index for approval request messages before removal - Insert reconstructed FunctionCallContent and FunctionResultContent at correct position - Pass insertion index through to ProcessFunctionCallsAsync for approved functions - Handle edge case where already-executed approvals exist - Add tests for rejection and approval scenarios with user messages after responses Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
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 a message ordering bug in FunctionInvokingChatClient where user messages added after function approval responses were incorrectly repositioned before tool call/result messages instead of remaining at the end of the conversation.
Key changes:
- Modified
ExtractAndRemoveApprovalRequestsAndResponsesto track and return the original index position of approval requests - Updated
ProcessFunctionApprovalResponsesto insert reconstructed function call and result messages at their original positions usingInsertRange/Insertinstead of appending withAddRange/Add - Extended
ProcessFunctionCallsAsyncto accept aninsertionIndexparameter for placing approved function results at the correct position
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| FunctionInvokingChatClient.cs | Core fix: tracks approval request positions and uses insertion instead of appending to preserve message ordering |
| FunctionInvokingChatClientApprovalsTests.cs | Adds test coverage for rejection and approval scenarios with user messages following approval responses |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
…nvokingChatClient.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 2 out of 2 changed files in this pull request and generated 2 comments.
| // Before: | ||
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionApprovalRequest(B)] | ||
| // After processing approval for B, if we inserted at the original index of B, we'd get: | ||
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionCall(B), FunctionResult(B)] | ||
| // But if there are already function results present (e.g., for A), we want to append new function calls/results for B at the end: | ||
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionApprovalRequest(B), FunctionCall(B), FunctionResult(B)] | ||
| // This preserves the correct chronological order of function calls and results. |
Copilot
AI
Dec 17, 2025
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.
The example comment is confusing because it shows FunctionApprovalRequest and FunctionApprovalResponse in the message list, but these would have been removed by the time this logic runs. Consider clarifying that the "Before" represents the original user input before extraction, or showing the actual state of the messages list at this point (after extraction but before insertion). For example: "Before extraction: [User, FAReq(A), FARes(A), FAReq(B)]; After extraction: [User, FRes(A)]; After insertion with functionResultCallIds present: [User, FRes(A), FCall(B), FRes(B)]"
| // Before: | |
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionApprovalRequest(B)] | |
| // After processing approval for B, if we inserted at the original index of B, we'd get: | |
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionCall(B), FunctionResult(B)] | |
| // But if there are already function results present (e.g., for A), we want to append new function calls/results for B at the end: | |
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionApprovalRequest(B), FunctionCall(B), FunctionResult(B)] | |
| // This preserves the correct chronological order of function calls and results. | |
| // Before extraction (original user input and approval messages): | |
| // [User, FunctionApprovalRequest(A), FunctionApprovalResponse(A), FunctionResult(A), FunctionApprovalRequest(B)] | |
| // After extraction of approval requests/responses (state of 'messages' at this point, with existing results only): | |
| // [User, FunctionResult(A)] | |
| // After processing approval for B, if we inserted at the original index of B, we'd conceptually place the new call/result | |
| // where B's approval request was, interleaving them with existing messages: | |
| // [User, FunctionResult(A), FunctionCall(B), FunctionResult(B)] | |
| // But if there are already function results present (e.g., for A), we instead append new function calls/results for B at the end | |
| // of the current list of messages to preserve chronological ordering relative to any prior calls/results. |
|
|
||
| // Calculate the insertion position: right after the FCC messages we just inserted | ||
| int rejectedInsertionIndex = insertionIndex >= 0 && insertionIndex <= originalMessages.Count | ||
| ? insertionIndex + (allPreDownstreamCallMessages?.Count ?? 0) |
Copilot
AI
Dec 17, 2025
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.
The calculation of rejectedInsertionIndex doesn't account for whether FCC messages were actually inserted into originalMessages. When hasConversationId is true, FCC messages are not inserted (line 1297-1308), but this calculation still adds allPreDownstreamCallMessages.Count to the index. This causes rejection messages to be inserted at the wrong position when there are messages after the approval response. The calculation should only add the FCC count when !hasConversationId. For example: insertionIndex + (!hasConversationId ? (allPreDownstreamCallMessages?.Count ?? 0) : 0)
| ? insertionIndex + (allPreDownstreamCallMessages?.Count ?? 0) | |
| ? insertionIndex + (!hasConversationId ? (allPreDownstreamCallMessages?.Count ?? 0) : 0) |
When processing
FunctionApprovalResponseContent, the client appends reconstructedFunctionCallContentandFunctionResultContentmessages to the end of the message list, reordering any messages that came after the approval response.Example of the issue:
Changes
ExtractAndRemoveApprovalRequestsAndResponsesnow returns the index where approval requests were originally locatedProcessFunctionApprovalResponsesusesInsertRange/Insertat the tracked position rather thanAddRange/AddProcessFunctionCallsAsyncaccepts aninsertionIndexparameter to place approved function results at the correct positionTests
Added test cases for rejection and approval scenarios with user messages following the approval response.
Original prompt
This pull request was created as a result of the following prompt 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