Fix Response.Update: keep the earliest valid CreatedAt - #668
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts (*Response).Update so CreatedAt is populated only once using the earliest valid timestamp from a stream of updates, matching the .NET behavior and preventing later chunks from overwriting the initial creation time.
Changes:
- Update message-level and response-level
CreatedAtmerge logic to “first valid wins”. - Add
isValidCreatedAthelper to treat zero / epoch-zero timestamps as unset. - Update and extend unit tests to assert earliest-valid timestamp behavior and epoch-zero handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
agent/response.go |
Changes CreatedAt merge semantics and introduces isValidCreatedAt to ignore default/uninitialized timestamps. |
agent/response_test.go |
Updates existing assertions and adds new tests for first-valid-wins and epoch-zero ignored behavior. |
Comments suppressed due to low confidence (1)
agent/response.go:219
- Same issue as message CreatedAt: resp.CreatedAt being epoch-zero (or otherwise invalid) will not be replaced because the guard only checks IsZero(). To fully treat epoch-zero as unset, the "already set" check should use isValidCreatedAt on the current value as well.
if resp.CreatedAt.IsZero() && isValidCreatedAt(update.CreatedAt) {
resp.CreatedAt = update.CreatedAt
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if msg.CreatedAt.IsZero() && isValidCreatedAt(update.CreatedAt) { | ||
| msg.CreatedAt = update.CreatedAt | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in bb29447: the existing-value guard now uses !isValidCreatedAt(...) (both lines 189 and 217) so a pre-existing epoch-zero CreatedAt is treated as unset and a later valid timestamp can replace it. Added a test covering a Response/Message pre-seeded with epoch-zero.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f47335b to
b9c041c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Response.Update overwrote an already-set CreatedAt whenever a later update carried a strictly-later timestamp, so a collected message or response was stamped with the last streamed chunk's time instead of the first valid one. Match .NET ChatResponseExtensions.ProcessUpdate, which keeps the first valid timestamp: only set CreatedAt when it is currently zero and the update's value is valid. Add isValidCreatedAt, mirroring the .NET IsValidCreatedAt helper, so a zero or epoch-zero timestamp is treated as unset and cannot populate CreatedAt (and a later valid value can still take hold).
Guard existing CreatedAt with isValidCreatedAt so an epoch-zero value (e.g. from older data or JSON) can still be replaced by a later valid timestamp, matching the intent of treating epoch-zero as unset.
bb29447 to
a800d6d
Compare
Parity Review: ApprovedThis PR corrects a behavioral divergence in Response.Update by keeping the first valid CreatedAt timestamp rather than the latest, matching the .NET ChatResponseExtensions.ProcessUpdate behavior. Scope: Only agent/response.go and agent/response_test.go are changed. No exported identifiers were added or removed; only the behavior of the existing (*Response).Update method changed. The isValidCreatedAt helper is unexported. Cross-repo consistency: Aligned with .NET (ProcessUpdate keeps first valid timestamp). No equivalent Python stream-merging implementation found for CreatedAt in python/packages/core/agent_framework/, so no Python parity concern. No public API surface change detected — the public-api-change label is not needed. Parity review is green. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Summary
(*Response).Updateoverwrote an already-setCreatedAtwhenever a later update carried a strictly-later timestamp (the guard usedupdate.CreatedAt.After(...)), in both the message-level and response-level branches. As a result a message/response collected from a stream ended up stamped with the last streamed chunk's time instead of the first valid one..NET parity
.NET
ChatResponseExtensions.ProcessUpdatekeeps the first valid timestamp, not the latest:This PR mirrors that:
CreatedAtis set only when it is currently zero and the update's value is valid. It also addsisValidCreatedAt, mirroring the .NETIsValidCreatedAthelper (t.After(time.Unix(0, 0))), so a zero or epoch-zero timestamp is treated as unset — it cannot populateCreatedAt, and a later valid value can still take hold.Tests
agent/response_test.go:TestResponse_Update_CreatedAtandTestResponse_CreatedAt, which previously asserted latest-wins, to assert the first valid timestamp is retained.TestResponse_Update_CreatedAt_FirstValidWins(a strictly-later second update keeps the first timestamp on both message and response) andTestResponse_Update_CreatedAt_EpochZeroIgnored(an epoch-zero timestamp is treated as unset and a later valid value takes hold).Both new tests fail against the previous latest-wins logic and pass with the fix.
Verified with
go build ./...,go vet ./agent/..., andgo test ./...(full suite green).Note
The originally-scoped item also proposed scoping
AdditionalPropertiesinUpdatebyMessageID(matching the same .NETProcessUpdate). That change is intentionally left out of this PR: the Go providers currently surface response-level metadata (e.g. foundryServedModel, openaiEndUserId/Error) by stamping it onto message-bearing updates and relying onUpdatemerging those properties into the response. Scoping byMessageIDin isolation would misroute that metadata onto messages and break provider behavior, so it needs a coordinated provider change and is better handled separately.