Fix FICC tool_calls/tool ordering with approvals and service-managed chat history#7617
Conversation
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. |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1501578&view=codecoverage-tab |
jozkee
left a comment
jozkee
left a comment
There was a problem hiding this comment.
I think the fix is pragmatic, but it has a lot of cases to consider, like the ones I mentioned in the PR. I thought that appending at the end was because that was respecting the timing of when the FRC was really obtained, however, that still messed with the timing of the FCC, so we would be basically shifting to "stick with the TARespC position instead" (assuming we also cover the cases I mentioned). The problem is that this is harder than what we currently do.
We tried this before in #7158.
| Tools = | ||
| [ | ||
| new ApprovalRequiredAIFunction(AIFunctionFactory.Create(() => "Result 1", "Func1")), | ||
| AIFunctionFactory.Create((int i) => $"Result 2: {i}", "Func2"), |
There was a problem hiding this comment.
Can we simplify the tests by just having the ApprovalRequiredAIFunction.
| await InvokeAndAssertAsync(options, input, downstreamClientOutput, output, expectedDownstreamClientInput); | ||
|
|
||
| await InvokeAndAssertStreamingAsync(options, input, downstreamClientOutput, output, expectedDownstreamClientInput); | ||
| } |
There was a problem hiding this comment.
Given the following input:
user [approvalResp callId1]
user "MIDDLE"
user [approvalResp callId2]
you will get this downstream:
user "MIDDLE"
tool [FRC callId1, FRC callId2]
This is because the anchor is derived only from the last approval's position. Is that OK?
There was a problem hiding this comment.
This also doesn't consider the case where the message contains TARespContent and some other content:
user [approvalResp callId1, text "please proceed"]
you will get this downstream:
user "please proceed"
tool [FRC callId1]
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