-
Notifications
You must be signed in to change notification settings - Fork 849
Fix ToChatResponse to use first appropriate CreatedAt instead of last #7193
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
…f last Changed the logic in ProcessUpdate to prefer the first valid CreatedAt value (non-null and after Unix epoch) rather than the latest one. This fixes the issue where UsageContent updates with later timestamps were incorrectly overwriting the message CreatedAt with a value that doesn't reflect when the actual message content was generated. Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
…ixEpoch under #if NET - Updated comments in ProcessUpdate to clarify that CreatedAt prefers first valid value - Used DateTimeOffset.UnixEpoch under #if NET instead of custom _unixEpoch field 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 ToChatResponse{Async} was incorrectly using the timestamp from the last update rather than the first update when constructing a ChatResponse. This caused inaccurate CreatedAt values when usage updates arrive significantly after content updates (e.g., in the Mistral API where usage updates can be delayed by several minutes).
Key Changes:
- Modified timestamp handling to use "first valid value wins" behavior instead of "last value wins"
- Added
IsValidCreatedAthelper that treats values ≤ Unix epoch (1970-01-01) as null/invalid - Updated all affected tests to reflect the new first-wins behavior and use valid timestamps (years ≥ 1970)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs |
Changed ProcessUpdate to set CreatedAt only when null (first-wins), added IsValidCreatedAt helper and Unix epoch constant |
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs |
Updated test timestamps from invalid years (1, 2) to valid ones (2024, 2025), updated expected values to match first-wins behavior, and corrected comments to reflect new semantics |
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/ChatCompletion/ChatResponseExtensions.cs
Outdated
Show resolved
Hide resolved
...crosoft.Extensions.AI.Abstractions.Tests/ChatCompletion/ChatResponseUpdateExtensionsTests.cs
Show resolved
Hide resolved
Simplified the IsValidCreatedAt helper to use direct comparison (createdAt > UnixEpoch) instead of pattern matching first. The comparison returns false when the nullable value is null. Co-authored-by: stephentoub <[email protected]>
Added test cases to validate that datetime values less than Unix epoch (not just equal to it) are treated the same as null: - Added test cases to ToChatResponse_TimestampFolding_MemberData for 1969 and 1960 dates - Updated ToChatResponse_AlternativeTimestamps to include a before-epoch timestamp Co-authored-by: stephentoub <[email protected]>
ToChatResponse{Async}should use the first update's CreatedAt instead of the lastChatResponseExtensions.cslines 521-524 (message) and 566-569 (response)DateTimeOffset.UnixEpochunder#if NETinstead of custom fieldIsValidCreatedAtto use direct nullable comparisonSummary
Changed the
ToChatResponse{Async}implementation so that theCreatedAtproperty reflects the first appropriate update'sCreatedAtproperty rather than the last. Values that are less than or equal to the Unix epoch are treated as null (invalid). This ensures that when streaming responses from providers like Mistral, theCreatedAtaccurately reflects when the message content was generated, not when the usage update was received.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Microsoft Reviewers: Open in CodeFlow