-
Notifications
You must be signed in to change notification settings - Fork 849
ToChatResponse: Merge AdditionalProperties into ChatMessage instead of ChatResponse #7194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…instead of ChatResponse Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where AdditionalProperties from ChatResponseUpdate objects were incorrectly being merged into the top-level ChatResponse.AdditionalProperties instead of the individual ChatMessage.AdditionalProperties. This caused properties from different messages to collide when they had the same keys.
Key Changes:
- Moved
AdditionalPropertiesmerging logic fromChatResponselevel toChatMessagelevel inProcessUpdatemethod - Updated existing test expectations to reflect that properties now belong to messages
- Added comprehensive new test to verify properties are correctly isolated per message
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.Abstractions/ChatCompletion/ChatResponseExtensions.cs |
Relocated the AdditionalProperties merging logic (lines 538-548) to apply to message instead of response, enabling per-message property isolation |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs |
Updated existing test to expect AdditionalProperties on messages (lines 60-65) and added new test ToChatResponse_AdditionalPropertiesGoToMessages (lines 449-496) to verify the corrected behavior with multiple messages |
|
@copilot, there's a test failure, e.g. |
…sent Co-authored-by: stephentoub <[email protected]>
Head branch was pushed to by a user without write access
Fixed in 04ddd18. The issue was that my initial change unconditionally moved |
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
Co-authored-by: stephentoub <[email protected]>
|
cc: @westey-m |
ToChatResponse{Async}should mergeChatResponseUpdate.AdditionalPropertiesinto the correspondingChatMessage.AdditionalPropertiesrather thanChatResponse.AdditionalPropertiesProcessUpdatemethod inChatResponseExtensions.csto addAdditionalPropertiesbased on MessageId presenceChatResponseUpdateExtensionsTests.csto expect the new behaviorToChatResponse_AdditionalPropertiesGoToMessagesto verify the correct behaviorToChatResponse_AdditionalPropertiesRoutingBasedOnMessageIdfor MessageId-based routingSummary
This PR updates
ToChatResponseandToChatResponseAsyncmethods to useMessageIdas the discriminator for routingAdditionalProperties:Routing logic:
MessageId→AdditionalPropertiesgo tomessage.AdditionalProperties(message-scoped)MessageId→AdditionalPropertiesgo toresponse.AdditionalProperties(response-scoped)Benefits:
MessageIdsystem_fingerprint) is preserved correctly for telemetryMessageIdOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Microsoft Reviewers: Open in CodeFlow