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.
|
Reworked the positioning into a single rule rather than the earlier front-insert/relocate approach: the reconstructed tool-call/result block is inserted just before the last message carrying approval content, so residual/trailing caller content stays after it while preserved (MCP / informational-only) approval content stays before it. Mixed |
…vice-chat-history
|
Pushed another round addressing the latest review:
All 681 On the broader concern about complexity vs. #7158: the positioning is now a single rule — insert the reconstructed block just before the last message carrying function-approval content — with interleaved/leading content among approval responses explicitly documented as an unsupported scenario rather than something we try to handle. Happy to discuss if you'd prefer a different tradeoff there. Ready for another look. |
I think this is fine, it appears there is nothing keeping us with the current approach other than simplicity and this approach enables your scenario to succeed. If this becomes something that needs to change per scenario, we can consider introducing a knob. |
…chat history (#7617) * Address fcc/frc ordering bug when using approvals and service stored chat history * Address PR comments * Address PR comments
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