Do not double-count usage when Response.ToUpdates re-emits per-message UsageContent#594
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes usage double-counting when converting a completed agent.Response into streaming-style ResponseUpdates and then collecting them back into a Response.
Changes:
- Filter
*message.UsageContentout of per-message updates produced by(*Response).ToUpdates, while still emitting a single trailing aggregate usage update. - Add a regression test ensuring
Collect(resp.ToUpdates()).Usage()matches the real total (no double-counting).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
agent/response.go |
Filters per-message Contents to omit embedded UsageContent so collection doesn’t double-count usage. |
agent/response_test.go |
Adds a round-trip test covering the double-counting scenario and asserting correct aggregate usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // contentsWithoutUsage returns a copy of contents with any *message.UsageContent | ||
| // removed. Per-message usage is re-emitted once as a trailing aggregate update in | ||
| // ToUpdates, so retaining it here would cause Collect to double-count usage. The | ||
| // input slice is not mutated, as the owning Response is caller-owned. |
5c36920 to
fe8b381
Compare
…e UsageContent Response.ToUpdates emitted each message's full Contents (including any UsageContent providers embed there, e.g. anthropicprovider) and then appended a trailing aggregate UsageContent update. Collect merges the trailing update into the last message, so the round trip counted usage twice. Filter UsageContent out of the per-message updates without mutating the caller-owned Response, keeping a single trailing aggregate emission to match .NET ToAgentResponseUpdates semantics.
fe8b381 to
19cf425
Compare
Parity Review: No Cross-Repo Consistency IssuesThis PR fixes a usage double-counting bug in What changed
The fix strips Upstream parityThe PR description correctly identifies the upstream reference: .NET No equivalent Python surface was found for ConclusionThe exported 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.
|
What
(*Response).ToUpdatesnow filters*message.UsageContentout of each per-message update'sContents, while keeping the single trailing aggregateUsageContentupdate.Why
Providers embed usage inside message contents (e.g.
anthropicprovider/agent.goputsUsageContentinmsg.Contents).ToUpdatespreviously emitted the fullmsg.Contents(still holding thatUsageContent) and appended a trailing update carryingresp.Usage(), which itself is the sum of per-message usage.ResponseStream.Collectmerges the empty-keyed trailing update into the last message, soCollect(resp.ToUpdates()).Usage()returnedsum(per-message usage) + aggregate— double the real total.This mirrors .NET
ToAgentResponseUpdates, which emits usage exactly once. The filtering builds a fresh slice and never mutatesmsg.Contents, since theResponseis caller-owned.Testing
Added
TestResponse_ToUpdates_RoundTripDoesNotDoubleCountUsage: builds aResponsewith a message containing aTextContentand aUsageContent{TotalTokenCount:100}, then asserts the per-message update carries noUsageContent, the trailing update reports 100, andCollect(resp.ToUpdates()).Usage().TotalTokenCount == 100(was 200 before the fix). The test fails before the change and passes after.go build ./...,go vet ./agent/..., andgo test ./agent/...all pass.