fix(openai): guard against AsyncAPIResponse without .id in async responses wrapper#4078
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughparse_response now recognizes OpenAI ChangesResponses parsing and tests
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
2 suggestions toward a simpler/more robust shape before this lands: 1. The same
|
| Line | Function | Guarded? |
|---|---|---|
| 570 | responses_get_or_create_wrapper (sync) |
❌ |
| 737 | async_responses_get_or_create_wrapper |
✅ (this PR) |
| 827 | responses_cancel_wrapper |
❌ |
| 859 | async_responses_cancel_wrapper |
❌ |
| ~1050 | ResponseStream finalize path |
❌ |
The reporter only hit the async path because the cs-agents-demo is async, but the sync caller using client.responses.with_raw_response.create(...) trips the identical AttributeError on line 572.
2. The cleanest fix is in parse_response itself
parse_response (line 236) currently only unwraps LegacyAPIResponse:
def parse_response(response):
if isinstance(response, LegacyAPIResponse):
return response.parse()
return responseBut APIResponse and AsyncAPIResponse (the non-legacy variants returned by with_raw_response()) also expose a .parse() method that yields the typed Response — which has the id everything downstream needs. So the one-line fix is to unwrap them too:
def parse_response(response):
if hasattr(response, "parse") and callable(getattr(response, "parse", None)):
return response.parse()
return responseor, more explicit:
from openai._response import APIResponse, AsyncAPIResponse # alongside LegacyAPIResponse
def parse_response(response):
if isinstance(response, (LegacyAPIResponse, APIResponse, AsyncAPIResponse)):
return response.parse()
return responseBenefits over the per-site hasattr guard:
- Fixes all 5 call sites at once.
- Preserves telemetry — the current early-return drops the span entirely, so users who instrument with
with_raw_responsesilently get no observability for those calls. Unwrapping at the source means they get a proper span with the realid. - Matches the function's declared return type (
-> Response).
If unwrapping in parse_response feels too broad for this PR, then at minimum please mirror the hasattr guard to the other 4 sites — a guard in only one of five identical places is harder to reason about than no guard at all.
|
@doronkopit5 You are absolutely right. made that change plus fixed the below suggested code rabbit change |
f4224e5 to
c41e14f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.py`:
- Around line 237-239: The current parse_response function calls
AsyncAPIResponse.parse() synchronously which returns a coroutine; split into two
helpers: keep parse_response for sync responses (LegacyAPIResponse, APIResponse
-> call .parse() and return result) and add parse_response_async that is async
and awaits AsyncAPIResponse.parse() (handle Response passthrough similarly),
then update async wrappers async_responses_cancel_wrapper and
async_responses_get_or_create_wrapper to call and await
parse_response_async(response) instead of calling parse_response so the
coroutine is awaited before accessing attributes like .id.
In
`@packages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py`:
- Around line 839-855: The test incorrectly mocks AsyncAPIResponse.parse as a
synchronous method; update test_parse_response_unwraps_async_api_response to
model async behavior by making wrapper.parse an async coroutine that returns
inner (or patch it with an async function) and call/await parse_response
accordingly (use asyncio.run or mark the test async) so the
AsyncAPIResponse.parse contract is respected; reference AsyncAPIResponse,
parse_response, and wrapper.parse when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87b2f966-53c6-4c37-ab1c-60f3a1f95c90
⛔ Files ignored due to path filters (2)
packages/opentelemetry-instrumentation-openai/uv.lockis excluded by!**/*.lockpackages/sample-app/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/responses_wrappers.pypackages/opentelemetry-instrumentation-openai/tests/traces/test_responses.py
1e27c6c to
80231dd
Compare
Fixes #4058 — OpenAI Agents crash when AsyncAPIResponse without .id is returned by parse_response
Summary by CodeRabbit
Bug Fixes
Tests