fix(bidi): emit gemini usage metadata alongside content events - #3725
fix(bidi): emit gemini usage metadata alongside content events#3725mehtarac wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| """ | ||
| events: list[BidiOutputEvent] = [] | ||
|
|
||
| if server_content.interrupted: |
There was a problem hiding this comment.
Issue: The switch from early-return to list accumulation changes emission from exclusive (one event per message) to cumulative. That's the right fix for usageMetadata, but it also changes content semantics: an interrupted message now co-emits with any output_transcription/model_turn text in the same server_content, whereas before the interruption short-circuited everything.
Suggestion: This looks intentional (and test_interruption_emitted_alongside_other_server_content locks it in), so no change needed if it is — just confirming it's deliberate that partial model text is surfaced after an interruption rather than dropped. Worth a one-line note in the PR description since it's a subtle behavior change beyond the usage fix.
There was a problem hiding this comment.
Yes, intentional. Per the Live API reference, interrupted, outputTranscription, and modelTurn are all fields on the same BidiGenerateContentServerContent object, not mutually exclusive message types — the old early-return dropped whichever fields came after the first match.
|
|
||
| assert [type(event) for event in events] == [BidiUsageEvent] | ||
| usage_event = events[0] | ||
| assert usage_event.modality_details == [{"modality": "audio", "input_tokens": 7, "output_tokens": 9}] |
There was a problem hiding this comment.
Issue: This test only asserts modality_details and cache_read_input_tokens, but the fixture also populates input_tokens/output_tokens/total_tokens (10/20/30). Since those fields aren't asserted, a regression that corrupts the token counts while this message shape is in play would pass silently.
Suggestion: BidiUsageEvent is a dict subclass, so you can assert the whole event in one equality and cover every field at once:
assert events == [
BidiUsageEvent(
input_tokens=10,
output_tokens=20,
total_tokens=30,
modality_details=[{"modality": "audio", "input_tokens": 7, "output_tokens": 9}],
cache_read_input_tokens=4,
)
]The same pattern applies to test_usage_metadata_emitted_alongside_audio (lines 591–593), which asserts the three token fields individually.
There was a problem hiding this comment.
Confirmed resolved — both test_usage_metadata_modality_details and test_usage_metadata_emitted_alongside_audio now assert the full BidiUsageEvent in a single equality, so every field (including the token counts) is covered. Thanks for tightening these. 👍
|
Assessment: Comment (approve-leaning) Solid, focused bug fix. Restructuring Review notes
Nice cleanup overall — the new structure also clearly sets up the follow-up for the missing response start/complete events. |
|
@strandly-the-agent do a fresh review of this PR |
|
Assessment: Approve Fresh review of the Verification
Clean, well-tested fix that also sets up the follow-up for the missing response start/complete events. 🚀 |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Fresh review at 3bfda5ec — verdict: comment. No blocker. The usage fix is right, and the restructure quietly recovers two other silent drops (an interrupted turn's inputTranscription, and audio riding with an outputTranscription). Two new co-emission orderings deserve a decision, and one premise deserves confirming.
Verified on this branch (real google.genai.types, not mocks; differential vs base 11ad6366):
- ✅
27 passed—python -m pytest tests/strands/experimental/bidi/models/test_gemini_live.py -q(google-genai 2.17.0) - ✅ the fix works:
serverContent(audio) + usageMetadata→['BidiAudioStreamEvent', 'BidiUsageEvent']at head vs['BidiAudioStreamEvent']on base - ✅ floor is safe:
LiveServerContent/UsageMetadataand every field read exist atgoogle-genai==1.32.0 - ✅ shape matches repo convention (
openai_realtime.py:562-609already accumulates a list with usage appended last) - 🟡 2 new behaviors reproduced end-to-end through the real
_BidiAudioOutput/_BidiAgentLoop(inline) - 🟡 mutation check: emitting audio before the
server_contentevents still passes all 27 tests → event order is unpinned
❓ Question (worth settling before merge)
Is Gemini Live's usageMetadata a per-message delta or a running total? loop.py:340-344 sums every BidiUsageEvent. If it's cumulative, then emitting more usage events — precisely what this PR does — would make session accounting overcount rather than fix an undercount, inverting the premise. I couldn't settle it: the Live API reference documents the fields but never says which; the official capabilities-guide example doesn't sum, it prints each message standalone as "Used {usage.total_token_count} tokens in total" under the comment "the server will periodically send messages that include UsageMetadata"; and several developer-forum reports describe promptTokenCount as cumulative/growing per turn. One captured live session settles it — do you have one handy? Happy to be wrong here.
Coverage gaps (non-blocking) — 3
- No test pairs
usage_metadatawith atool_callor with a transcript, though the new docstring's own claim is that usage "can accompany any other field" — and usage landing with a tool-call message is arguably more common than with a session-resumption update. test_usage_metadata_modality_detailsusesAUDIOfor both prompt and response details, so only the merge branch (gemini_live.py:369-370) runs; the new-modality append branch (:372-374, e.g. audio-in/text-out) is never exercised.if detail.modality and detail.token_count:(:354,:365) silently drops a legitimately reportedtoken_count == 0. Intentional?
Appendix — non-blocking (5)
Two pre-existing bugs, verified byte-identical on base — they belong in issues, not this PR. I tried to file them and my token lacks issues:write here, so they're written up below for someone with permissions (or say the word and I'll draft them as an issue body you can paste).
-
⚪
modality_detailsemits"mediamodality.audio", violating its own type contract.gemini_live.py:357,367dostr(detail.modality).lower(); on the wiredetail.modalityis aMediaModalityenum with no__str__override, so:>>> str(t.MediaModality.AUDIO).lower() -> 'mediamodality.audio' >>> t.MediaModality.AUDIO.value.lower() -> 'audio'Real-types run against this branch emits
{'modality': 'mediamodality.audio', 'input_tokens': 7, 'output_tokens': 0}, butModalityUsage.modalityisLiteral["text","audio","image","cached"](types/events.py:460) — any consumer branching on it silently fails to match. Same at floor 1.32.0 and at 2.17.0. Fix isdetail.modality.value.lower();openai_realtime.py:608may share the pattern. -
⚪
part.thoughtisn't filtered, so model reasoning can leak into transcripts and history.gemini_live.py:326takes every part with.text; google-genai's own.textproperty explicitly skips thought parts. Real-types repro: parts[Part(text="reasoning: they want the capital", thought=True), Part(text="Paris.")]→ transcript"reasoning: they want the capital Paris.", wheremessage.textgives"Paris.". Pre-existing, but the change in the second inline comment widens how often the path fires. -
⚪
message.datais now evaluated twice per message (:245and:248). It's a computed property that runspart.model_dump()per part and logs the "non-data parts" warning for mixed parts — at the 1.32 floor there's no once-per-process dedup, so a mixed message warns twice, which sits oddly beside the comment at:323. Measured cost is negligible (3.9 µs/chunk), so this is purely ahas_data = bool(message.data)tidy-up. -
⚪
test_event_conversionis still one 113-line multi-scenario test — pre-existing, and the PR re-pointed it at the new fixtures without splitting it, even though it introduces exactly that per-scenario pattern next door. -
⚪ Pre-existing ruff
UP035ongemini_live.py:18(AsyncGeneratorfromtyping) — also on base, not yours.
How this review ran
Staged pipeline: routing triage → context build → parallel specialist passes (correctness/safety, adversarial-repro, test-quality) → aggregation with de-dup and a reachability gate → this post. Each pass was an independent fresh-context agent; the adversarial pass ran warm (no nested spawn available) against its own role rubric. I re-verified every load-bearing finding myself with real google.genai types before posting. Two threads from the earlier automated review are deliberately not re-raised: the exclusive→cumulative shift (you answered it) and the whole-object BidiUsageEvent assertions (fixed in 3bfda5ec).
AI review — solid work for a human to approve, not a gate.
| await model.start() | ||
|
|
||
| prompt_detail = unittest.mock.Mock() | ||
| prompt_detail.modality = "AUDIO" |
There was a problem hiding this comment.
🟡 This fixture makes the assertion pass on a value the real API never produces.
modality is set to the plain string "AUDIO", so production's str(detail.modality).lower() (gemini_live.py:357) coincidentally yields "audio" and the test goes green. On the wire detail.modality is a MediaModality enum, and str(MediaModality.AUDIO).lower() is "mediamodality.audio" — which violates ModalityUsage.modality: Literal["text","audio","image","cached"] (types/events.py:460). So the one new test covering modality details certifies the wrong string and can't catch the real bug.
The production bug is pre-existing (see the appendix on the summary comment), so fixing it isn't your obligation here — but the fixture is new, and it's a two-character fix in a line this PR already moves:
| prompt_detail.modality = "AUDIO" | |
| prompt_detail.modality = genai_types.MediaModality.AUDIO |
With the real enum, "modality": "audio" only passes once _convert_usage_metadata uses detail.modality.value.lower(). Same for response_detail at :643. If you'd rather keep this PR scoped to the usage fix, that's fine — just leave the fixture out of it rather than pinning a string the wire can't produce.
| ): | ||
| """Session resumption tracks the handle and still emits co-attached usage metadata. | ||
|
|
||
| Guards https://github.com/strands-agents/harness-sdk/issues/3745 — this branch previously |
There was a problem hiding this comment.
🟡 Dangling issue link (×3), and this docstring narrates the diff.
https://github.com/strands-agents/harness-sdk/issues/3745 doesn't resolve — I checked via the API, and it's neither an issue nor a PR (the newest issue in the repo is ~3721). Same link at :577 and :675, and the PR's own "Related Issues" section is empty. docs/TESTING.md:82 draws the line as: a regression test for a discovered bug links the issue it guards; a test written as part of feature development carries no issue reference. These ship in the same PR as the fix, so unless there's a real issue to point at, dropping the links is the rule-conformant move.
Separately, "this branch previously returned early, discarding usage outright" narrates how the code changed, which root AGENTS.md explicitly forbids ("Never narrate how the code changed or what it used to be") — that reasoning belongs in the PR description.
| Guards https://github.com/strands-agents/harness-sdk/issues/3745 — this branch previously | |
| """Session resumption tracks the handle and still emits co-attached usage metadata. | |
| usageMetadata sits outside the messageType union, so it can accompany any other field, | |
| including a session-resumption update. | |
| """ |
Read the reference — verdictYou were right that the docs settle it. Net: two of my findings withdrawn (replied on both threads — the interrupt/audio ordering and the duplicate-transcript one), and the usage question resolves to something real but narrower than I framed it — and not this PR's bug. Usage semantics — answered: a per-generation-request snapshot, not a per-message delta. The reference defines So the bug is
One design note for whoever picks that up: Verdict: approve-leaning. What's left from my review is two test nits — the fixture that pins |
Description
Gemini's Live API sends token usage in a usageMetadata field that sits outside the messageType union. Per the Gemini Live Api reference:
BidiGeminiLiveModel._convert_gemini_live_event's implementation was such that any message with both content and usage lost the usage. The loss is silent and cumulative:_BidiAgentLoop._run_modelaccumulates session token counters solely from BidiUsageEvent, so every dropped event undercounts the session with no error. This restructures the converter to accumulate into a list. Control messages stay exclusive content messages append, and usage is evaluated unconditionally at the end.The list-accumulating shape also unblocks emitting multiple events per message, needed for the missing BidiResponseStartEvent / BidiResponseCompleteEvent for google gemini live model provider change left to a follow-up.
Related Issues
Documentation PR
Type of Change
Bug fix
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.