-
Notifications
You must be signed in to change notification settings - Fork 849
[WIP] 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
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
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. |
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.
Updated in commit e58a3ae. The example now shows the state after extraction (e.g., [User, FunctionResult(A)]) rather than before, making it clearer what the actual message list contains when this logic runs.
|
|
||
| // 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) |
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.
Fixed in commit e58a3ae. The calculation now only adds the FCC count when !hasConversationId, ensuring rejection messages are inserted at the correct position.
|
cc: @westey-m |
|
@copilot, please address any pending feedback |
…Id and clarify example comment - Only add FCC count to rejectedInsertionIndex when messages were actually inserted (!hasConversationId) - Update example comment to show state after extraction rather than before, making it clearer what the actual message list contains at the point the logic runs Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
| if (functionResultCallIds is { Count: > 0 } && insertionIndex >= 0) | ||
| { | ||
| insertionIndex = messages.Count; | ||
| } |
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, since it fallbacks to append-at-end when there's a FunctionResultContent in the messages, we still have the bug in cases that have FRC prior to the approval request/response pairs.
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.
Fixed in commit 1bf5d84. Now tracks whether any approval responses were skipped (already executed) and only appends at end for those cases, not for unrelated FunctionResultContent items.
| [ | ||
| new ChatMessage(ChatRole.User, "1st message"), | ||
| new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId1", "Func1")]), | ||
| new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Error: Tool call invocation was rejected by user.")]), |
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 the message was recently updated.
| new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Error: Tool call invocation was rejected by user.")]), | |
| new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Tool call invocation rejected.")]), |
…e insertion - Track whether any approval responses were skipped due to already being executed - Only append at end when approval requests in current batch have already been executed - Fix test to use correct rejection message format - Addresses @jozkee feedback about FRC prior to approval request/response pairs Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
|
@stephentoub, @westey-m, I think we need to fix this with a list of indexes, not just the first one it finds. If the FICC receives multiple request/response pairs with messages interleaved, it will cause the same bug. It is a theoretical case, because you will never have messages in between request/response pairs, it would need to be manually added to list of messages. It may be possible if someone is piling up approval requests in top of the FICC and sending them in batch. [Fact]
public async Task MultipleApprovalRequestResponsePairsWithInterleavedUserMessagesPreservesOrderingAsync()
{
// This test verifies that when there are multiple approval request/response pairs
// in a single call with user messages interleaved between them, the message ordering
// is preserved correctly. All approvals are processed in one invocation.
var options = new ChatOptions
{
Tools =
[
new ApprovalRequiredAIFunction(AIFunctionFactory.Create(() => "Result 1", "Func1")),
new ApprovalRequiredAIFunction(AIFunctionFactory.Create(() => "Result 2", "Func2")),
]
};
List<ChatMessage> input =
[
new ChatMessage(ChatRole.User, "1st user message"),
new ChatMessage(ChatRole.Assistant,
[
new FunctionApprovalRequestContent("callId1", new FunctionCallContent("callId1", "Func1"))
]) { MessageId = "resp1" },
new ChatMessage(ChatRole.User,
[
new FunctionApprovalResponseContent("callId1", true, new FunctionCallContent("callId1", "Func1"))
]),
new ChatMessage(ChatRole.User, "2nd user message"),
new ChatMessage(ChatRole.Assistant,
[
new FunctionApprovalRequestContent("callId2", new FunctionCallContent("callId2", "Func2"))
]) { MessageId = "resp2" },
new ChatMessage(ChatRole.User,
[
new FunctionApprovalResponseContent("callId2", true, new FunctionCallContent("callId2", "Func2"))
]),
new ChatMessage(ChatRole.User, "3rd user message"),
];
// The expected input to downstream client should preserve all message ordering:
// 1. User "1st user message" - should remain in place
// 2. Assistant with FunctionCallContent(callId1) - recreated from approval
// 3. Tool with FunctionResultContent(callId1) - from executing approved function
// 4. User "2nd user message" - should remain in place
// 5. Assistant with FunctionCallContent(callId2) - recreated from approval
// 6. Tool with FunctionResultContent(callId2) - from executing approved function
// 7. User "3rd user message" - should remain at the end
List<ChatMessage> expectedDownstreamClientInput =
[
new ChatMessage(ChatRole.User, "1st user message"),
new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId1", "Func1")]),
new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Result 1")]),
new ChatMessage(ChatRole.User, "2nd user message"),
new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId2", "Func2")]),
new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId2", result: "Result 2")]),
new ChatMessage(ChatRole.User, "3rd user message"),
];
List<ChatMessage> downstreamClientOutput =
[
new ChatMessage(ChatRole.Assistant, "Final response"),
];
List<ChatMessage> output =
[
new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId1", "Func1")]),
new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Result 1")]),
new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId2", "Func2")]),
new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId2", result: "Result 2")]),
new ChatMessage(ChatRole.Assistant, "Final response"),
];
await InvokeAndAssertAsync(options, input, downstreamClientOutput, output, expectedDownstreamClientInput);
await InvokeAndAssertStreamingAsync(options, input, downstreamClientOutput, output, expectedDownstreamClientInput);
} |
|
@copilot, address this comment #7158 (comment). |
I've analyzed the issue and added the test case which confirms the bug exists. The current implementation tracks only the first approval request index, causing messages between multiple approval request/response pairs to be incorrectly ordered. The fix requires tracking indices for each approval request by call ID and processing them individually. I've started the implementation but it requires significant refactoring of Working on completing the fix - the key changes are:
Will have the complete fix ready shortly. |
…ndividually - Modified ExtractAndRemoveApprovalRequestsAndResponses to track each approval request's original index by call ID - Refactored ProcessFunctionApprovalResponses to process rejections individually at their correct positions - Process rejections in reverse index order to avoid index shifting issues - Added test case for multiple interleaved approval/rejection pairs - Note: Approved functions still need individual processing (currently uses first index for all) Co-authored-by: jozkee <16040868+jozkee@users.noreply.github.com>
Fix message ordering issue in FunctionInvokingChatClient
Problem Analysis
When
FunctionInvokingChatClientprocessesFunctionApprovalResponseContent, it incorrectly reorders chat messages. User messages added after a function approval rejection are moved before the tool call/result instead of remaining at the end of the conversation.Additionally, when there are multiple approval request/response pairs with messages interleaved between them, only the first approval request index was tracked, causing incorrect ordering for subsequent pairs.
Changes
ExtractAndRemoveApprovalRequestsAndResponsesto track the original index of EACH approval request message by call IDProcessFunctionApprovalResponsesto process rejections individually at their correct positionsInvokeApprovedFunctionApprovalResponsesAsyncImplementation Summary
ExtractAndRemoveApprovalRequestsAndResponsesto track and return a dictionary of insertion indices (one per call ID) where approval requests were originally locatedProcessFunctionApprovalResponsesto process rejections individually in reverse index order, usingInsertRange/Insertto maintain message ordering at each approval's original positionrejectedInsertionIndexdidn't account for whether FCC messages were actually insertedCurrent Status
Note: This is a work in progress. The fix for rejections is complete. The fix for approved functions with multiple interleaved pairs requires additional refactoring to process each approval individually and insert their results at the correct positions while handling asynchronous execution.
All 25 existing approval tests pass. The new test for multiple interleaved pairs demonstrates the remaining issue with approved functions. CodeQL security checks found no issues.
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