-
Notifications
You must be signed in to change notification settings - Fork 841
Update ToChatResponse{Async} to also factor in AuthorName #6910
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
Originally we were only factoring in Role. Then we also augmented it to factor in MessageId, since updates from different messages are by definition not part of the same message. Now it also makes sense to factor in AuthorName, as different text from different speakers / agents are also by definition not part of the same message.
|
cc: @westey-m |
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 updates the ToChatResponse and ToChatResponseAsync methods to include AuthorName as an additional boundary condition for determining when to create new chat messages from streaming updates. Previously, the methods only considered Role and MessageId changes as triggers for new message boundaries. Now, changes in AuthorName will also dictate message boundaries, ensuring that text from different speakers/agents are properly separated into distinct messages.
Key changes:
- Enhanced boundary detection logic to include
AuthorNamecomparison alongside existingRoleandMessageIdchecks - Comprehensive test coverage for various
AuthorNameboundary scenarios - Updated existing tests to reflect the new behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs |
Refactored message boundary detection logic to include AuthorName comparison and added helper methods for clean boundary checking |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs |
Added extensive test coverage for AuthorName boundary scenarios and updated existing tests to accommodate the new behavior |
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Show resolved
Hide resolved
* Update ToChatResponse{Async} to also factor in AuthorName
Originally we were only factoring in Role. Then we also augmented it to factor in MessageId, since updates from different messages are by definition not part of the same message. Now it also makes sense to factor in AuthorName, as different text from different speakers / agents are also by definition not part of the same message.
* Fix useAsync inversion in tests
* Update ToChatResponse{Async} to also factor in AuthorName
Originally we were only factoring in Role. Then we also augmented it to factor in MessageId, since updates from different messages are by definition not part of the same message. Now it also makes sense to factor in AuthorName, as different text from different speakers / agents are also by definition not part of the same message.
* Fix useAsync inversion in tests
* Update ToChatResponse{Async} to also factor in AuthorName
Originally we were only factoring in Role. Then we also augmented it to factor in MessageId, since updates from different messages are by definition not part of the same message. Now it also makes sense to factor in AuthorName, as different text from different speakers / agents are also by definition not part of the same message.
* Fix useAsync inversion in tests
Originally we were only factoring in Role. Then we also augmented it to factor in MessageId, since updates from different messages are by definition not part of the same message. Now it also makes sense to factor in AuthorName, as different text from different speakers / agents are also by definition not part of the same message.
Microsoft Reviewers: Open in CodeFlow