Fix FICC tool_calls/tool ordering with approvals and service-managed chat history#7617
Open
westey-m wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an ordering bug in FunctionInvokingChatClient when resuming approval-gated tool calls in service-managed history mode (ConversationId set): tool results are now inserted ahead of any caller-supplied trailing messages so providers that require strict assistant(tool_calls) -> tool(result) adjacency don’t reject the request.
Changes:
- Add an insertion-index path for generated tool result messages so they can be placed before trailing caller messages rather than always appended.
- Compute and thread an “approved result insert index” through the approval-response handling (streaming + non-streaming) to keep tool results adjacent to their tool-call.
- Add regression tests for both service-managed and client-managed history modes when trailing messages are present after approval responses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs | Inserts reconstructed tool-call / tool-result messages at the approval anchor (before trailing messages) and threads an insert index through approval execution to preserve tool_calls→tool adjacency. |
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs | Adds streaming and non-streaming regression tests validating correct tool result placement with trailing messages in both service-managed and client-managed history modes. |
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1501578&view=codecoverage-tab |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7616
Summary
When
FunctionInvokingChatClient(FICC) resumes an approval-gated function call in service-managed history mode (a non-emptyConversationIdis set), it omits the reconstructedassistant(tool_calls)message (the service already holds it) but still appends the executed tool result to the tail of the outgoing message list. If the caller supplies any message after theToolApprovalResponseContenton the continuation turn, that trailing message ends up wedged between the service-heldassistant(tool_calls)and itstoolresult:Providers enforcing
tool_calls→tooladjacency (e.g. OpenAI) reject this with HTTP 400.Fix
Instead of always appending the reconstructed tool-call/tool-result messages at the tail, they are now inserted just before any caller-supplied trailing messages, keeping the tool result adjacent to the
assistantmessage that owns the matchingtool_call_id:ProcessFunctionCallsAsyncgains an optionalint? insertIndex— when set, results are inserted at that index rather than appended.ProcessFunctionApprovalResponsescomputes the trailing-message count (messages after the last approval-content message) and inserts the reconstructedassistant(tool_calls)+ rejected results just before them, returning the running insert index.InvokeApprovedFunctionApprovalResponsesAsyncso approved tool results land adjacent to the tool-call.When there are no trailing messages the insert position degrades to the original tail-append behavior, so existing scenarios are unaffected. The invoked function still receives the full input (trailing messages remain in
context.Messagesduring invocation; only the result placement changes).Resulting valid ordering:
Tests
Added regression tests (streaming + non-streaming) covering trailing messages after an approval response in both service-managed and client-managed history modes:
ApprovedResponsesStayAdjacentToToolCallWhenTrailingMessagesPresentWithServiceThreadsAsyncApprovedResponsesStayAdjacentToToolCallWhenTrailingMessagesPresentWithClientHistoryAsyncAll existing
FunctionInvokingChatClientapproval tests continue to pass.Microsoft Reviewers: Open in CodeFlow
CC @jozkee