Fix: Mark both TARC and TAResp FunctionCallContent as InformationalOnly after approval processing#7468
Fix: Mark both TARC and TAResp FunctionCallContent as InformationalOnly after approval processing#7468westey-m wants to merge 2 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an approval-processing edge case in FunctionInvokingChatClient where ToolApprovalRequestContent (TARC) and ToolApprovalResponseContent (TAResp) can end up with inconsistent FunctionCallContent.InformationalOnly values after serialization/deserialization, leading to validation failures.
Changes:
- Track the original
ToolApprovalRequestContentalongside approval responses so both request/responseFunctionCallContentinstances can be updated consistently. - Mark both request and response
FunctionCallContentasInformationalOnly=truewhen handling approvals/rejections to avoid mismatches across serialization boundaries. - Add a regression test covering the “separate FCC instances with same CallId” rejection scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs | Carries request content through approval processing and marks both request/response FCCs as informational after approval handling. |
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs | Adds a test ensuring rejection processing marks InformationalOnly on both request and response FCC instances. |
| // We mark both the response and request FunctionCallContent to ensure consistency | ||
| // across serialization boundaries where they may be separate object instances. | ||
| m.ResponseFunctionCallContent.InformationalOnly = true; | ||
| _ = m.RequestFunctionCallContent?.InformationalOnly = true; |
There was a problem hiding this comment.
m.RequestFunctionCallContent?.InformationalOnly = true uses the null-conditional operator on the left-hand side of an assignment, which is not valid C# and will fail to compile. Update this to an explicit null check (e.g., store the value in a local and set the property when non-null).
| _ = m.RequestFunctionCallContent?.InformationalOnly = true; | |
| var requestFunctionCallContent = m.RequestFunctionCallContent; | |
| if (requestFunctionCallContent is not null) | |
| { | |
| requestFunctionCallContent.InformationalOnly = true; | |
| } |
| // across serialization boundaries where they may be separate object instances. | ||
| foreach (var approval in notInvokedApprovals) | ||
| { | ||
| _ = approval.RequestFunctionCallContent?.InformationalOnly = true; |
There was a problem hiding this comment.
approval.RequestFunctionCallContent?.InformationalOnly = true uses the null-conditional operator on the left-hand side of an assignment, which is not valid C# and will fail to compile. Use an explicit null check before setting InformationalOnly.
| _ = approval.RequestFunctionCallContent?.InformationalOnly = true; | |
| if (approval.RequestFunctionCallContent is not null) | |
| { | |
| approval.RequestFunctionCallContent.InformationalOnly = true; | |
| } |
…nctionInvokingChatClientApprovalsTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| _ = approvalRequestCallIds?.Remove(tarc.ToolCall.CallId); | ||
| (allApprovalResponses ??= []).Add(tarc); | ||
| break; | ||
|
|
There was a problem hiding this comment.
The fix prevents the inconsistency from being created going forward, do we also want to handle stored conversations serialized before the fix? With something like this:
| case ToolApprovalResponseContent tarc when tarc.ToolCall is FunctionCallContent { InformationalOnly: true }: | |
| // Remove from validation set to handle sessions serialized before the fix | |
| // for https://github.com/dotnet/extensions/pull/7468. | |
| _ = approvalRequestCallIds?.Remove(tarc.ToolCall.CallId); | |
| goto default; |
This should also allow us to add a test with
var requestFcc = new FunctionCallContent("callId1", "Func1");
var responseFcc = new FunctionCallContent("callId1", "Func1") { InformationalOnly = true };
Problem
Fixes microsoft/agent-framework#5189
FunctionInvokingChatClient.ExtractAndRemoveApprovalRequestsAndResponsesthrowsInvalidOperationExceptionwhen conversation history contains aToolApprovalRequestContent(TARC) /ToolApprovalResponseContent(TAResp) pair whoseFunctionCallContentobjects have inconsistentInformationalOnlyflags — a situation that reliably occurs after session serialization/deserialization.Root Cause
FICC relies on shared object references between the
FunctionCallContent(FCC) inside a TARC and the FCC inside the corresponding TAResp. WhenGenerateRejectedFunctionResultssetsFCC.InformationalOnly = true, it only mutates the TAResp's FCC. Without serialization this works because TARC and TAResp share the same FCC object. After serialization, they are separate instances and the TARC's FCC retainsInformationalOnly = false.Turn-by-turn reproduction
Turn 1 — User sends a message. Model returns
FunctionCallContent("call1"). FICC wraps it inToolApprovalRequestContentand returns it. End-of-run persistence stores[user_msg, TARC(FCC)].Session serialize → deserialize — The stored TARC now contains a new FCC object (
FCC_D,InformationalOnly = false). The caller still holds the original FCC (FCC_OLD) from the response.Turn 2 — User rejects the approval using the original TARC reference, creating
TAResp(FCC_OLD). Messages sent to FICC:TARC(from deserialized history)FCC_DfalseTAResp(from user input)FCC_OLDfalseFICC processes successfully — both match the
InformationalOnly: falsepattern.GenerateRejectedFunctionResultssetsFCC_OLD.InformationalOnly = true. ButFCC_Dis a different object and is NOT mutated. Model retries → returns newFCC("call2")→ wrapped inTARC2.End-of-run persistence stores:
TARC(call1)FCC_Dfalse(never mutated!)TAResp(call1)FCC_OLDtrue(mutated)FRC(call1, "rejected")TARC2(call2)falseTurn 3 — User rejects call2. FICC's
ExtractAndRemoveApprovalRequestsAndResponsesprocesses history:TARC(FCC_D, InformationalOnly=false)→ matches pattern → adds"call1"toapprovalRequestCallIdsTAResp(FCC_OLD, InformationalOnly=true)→ SKIPPED (doesn't matchInformationalOnly: falseguard)TARC2(call2, InformationalOnly=false)→ matches → adds"call2"TAResp2(call2, InformationalOnly=false)→ matches → removes"call2"Result:
approvalRequestCallIds = {"call1"}— validation throws:Fix
Mark both the request and response
FunctionCallContentasInformationalOnly = trueat every approval processing point, ensuring consistency regardless of object identity:ApprovalResultWithRequestMessage— Now stores theToolApprovalRequestContentalongside the response, exposing bothResponseFunctionCallContentandRequestFunctionCallContent.ExtractAndRemoveApprovalRequestsAndResponses— The approval request dictionary now stores the TARC content alongside the message, and passes it through toApprovalResultWithRequestMessage.Request.GenerateRejectedFunctionResults— Marks bothResponseFunctionCallContentandRequestFunctionCallContentasInformationalOnly = true.InvokeApprovedFunctionApprovalResponsesAsync— Also marks the request FCC for the approved path, since the same identity split exists after serialization.Tests —
CloneInputnow deep-clonesFunctionCallContent,ToolApprovalRequestContent, andToolApprovalResponseContent, simulating the serialization effect (separate FCC instances) so existing tests exercise the previously broken codepath.CC @stephentoub