Remove backward-compat InformationalOnly case from FICC; suggest middleware workaround#7538
Open
jozkee wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes legacy backward-compat handling from FunctionInvokingChatClient for mixed InformationalOnly approval history and demonstrates a test-only middleware workaround for normalizing old serialized sessions before FICC processes them.
Changes:
- Removed the special
ToolApprovalResponseContent/InformationalOnly: truevalidation case from FICC. - Added
ApprovalHistoryNormalizingChatClienttest middleware to normalize matching approval request flags. - Updated the mixed
InformationalOnlyapproval test to use the middleware workaround.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs |
Removes the legacy validation bypass for processed approval responses. |
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/ApprovalHistoryNormalizingChatClient.cs |
Adds test-only middleware that marks matching approval requests as informational. |
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs |
Updates the legacy mixed-state test to exercise the middleware workaround. |
…leware workaround Remove the special case in ExtractAndRemoveApprovalRequestsAndResponses that handled ToolApprovalResponseContent with InformationalOnly=true for sessions serialized before PR dotnet#7468. Instead, provide a middleware workaround (ApprovalHistoryNormalizingChatClient) that normalizes the InformationalOnly flags on TARC/TAResp pairs before FICC processes them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jozkee
force-pushed
the
agents/i-m-having-second-thoughts-on-https-github-27472a3e
branch
from
40436b6 to
283dde9
Compare
jozkee
deleted the
agents/i-m-having-second-thoughts-on-https-github-27472a3e
branch
jozkee
restored the
agents/i-m-having-second-thoughts-on-https-github-27472a3e
branch
Collaborator
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1439185&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.
Summary
Removes the backward-compat special case in
ExtractAndRemoveApprovalRequestsAndResponses(added in #7468) that handledToolApprovalResponseContentwithInformationalOnly: truefor sessions serialized before the fix.Instead, a middleware workaround (
ApprovalHistoryNormalizingChatClient) is provided in the test project as a sample that normalizesInformationalOnlyflags on TARC/TAResp pairs before FICC processes them. This keeps FICC simpler and pushes the legacy compat concern to the consumer.Changes
FunctionInvokingChatClient.cs: Removed theToolApprovalResponseContentwithInformationalOnly: truecase from the switch inExtractAndRemoveApprovalRequestsAndResponses.ApprovalHistoryNormalizingChatClient.cs(new, test-only): Middleware that uses LINQ to find TAResp call IDs whereInformationalOnlyis already true, then sets the matching TARC FCCs to true as well.FunctionInvokingChatClientApprovalsTests.cs: Updated test to use the middleware workaround, proving the approach works across all TFMs (streaming and non-streaming).Ref: #7468 (comment)
cc @westey-m