Repository navigation
fix(bidi): emit OpenAI usage before response stop [replica of harness-sdk#4889] - #30
poshinchen wants to merge 1 commit into
Conversation
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v3-reason (optimized) review
Assessment: approve
Assessment: Approve
A focused, well-scoped bug fix that correctly reorders usage emission to precede BidiResponseStopEvent in the OpenAI bidi adapter, matching OpenAI's response.done payload semantics. Tests assert on full event-list equality (not per-field), which guards against regressions, and docs/diagram are updated consistently.
Review Categories
- Correctness: Core reordering in
_complete_responseis sound; theif usage :=guard correctly skips emission when no usage is present. - Testing: Good use of whole-object list equality; one suggested gap is a cancelled response with no
usagekey. - Behavior change: Ordering shift is observable to consumers — worth verifying downstream aggregators and noting the change for integrators.
- Docs: Event timeline, ordering steps, and the Nova Sonic timing note are clear and accurate to the diff.
Nicely contained fix with matching tests and documentation.
|
|
||
| if usage := response.get("usage"): | ||
| events.append(self._convert_usage_metadata(usage)) | ||
| events.append(BidiResponseStopEvent(response_id=response_id)) |
There was a problem hiding this comment.
Suggestion
Issue: When response.get("usage") is absent (e.g., some cancelled responses), no BidiUsageEvent is emitted and only BidiResponseStopEvent follows, which is correct — but the new ordering means consumers that previously relied on usage arriving after stop will now receive it before.
Suggestion: Please verify this ordering change is reflected wherever downstream consumers aggregate usage (and consider noting the behavior change in a changelog entry if the project maintains one), so the fix doesn't surprise existing integrations.
| @@ -171,9 +188,11 @@ async def test_receive_preserves_native_order_with_late_transcription(model, moc | |||
| BidiTranscriptStartEvent("user", content_id="user-1"), | |||
There was a problem hiding this comment.
Suggestion
Issue: test_receive_preserves_native_order_with_late_transcription now emits BidiUsageEvent(0, 0, 0) for cancelled responses, which exercises the zero-token path but doesn't cover a cancelled response that carries no usage key.
Suggestion: Consider adding a case where a cancelled response.done omits usage to confirm only BidiResponseStopEvent is emitted (no usage event), locking in the if usage := guard behavior.
GitHub Actions
Failure
Documentation Preview FailedThe documentation deployment encountered an error. Please check the deployment logs for more details. |
Replica of
https://github.com/strands-agents/harness-sdk/pull/4889(URL in backticks to avoid a cross-reference on the original PR) for reviewer evaluation. Branches mirror the original PR's base and head commits.The review below was produced by the optimized Strandly reviewer (variant
v3-reason, optimization runaeb3ee, agent model Opus 4.8) during validation on PRs it never saw in training. Compare with the human and Strandly reviews on the original PR.