Refactor to use openai SDK instead of raw httpx#34
Draft
r0x0d wants to merge 1 commit into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates the project from a manual httpx-based integration with OpenAI's Responses API to using the official OpenAI Python SDK. The migration involves replacing custom response models with OpenAI SDK types, updating exception handlers to use OpenAI SDK errors, refactoring client initialization, and rewriting API endpoint logic and tests accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant App as FastAPI App
participant OpenAI as AsyncOpenAI SDK
participant Translator as Response Translator
participant Response as JSON Response
Client->>App: POST /v1/chat/completions
App->>OpenAI: client.responses.create(params)
OpenAI->>OpenAI: Call OpenAI Responses API
alt Streaming Mode
OpenAI-->>App: AsyncIterator[ResponseStreamEvent]
App->>Translator: translate_stream(stream, model)
Translator-->>App: SSE chunks
App-->>Client: Stream SSE events
else Non-Streaming Mode
OpenAI-->>App: Response object
App->>Translator: translate_response(response, model)
Translator-->>App: ChatCompletion
App-->>Client: JSON Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/routers/test_v1.py (1)
165-257:⚠️ Potential issue | 🟡 MinorAdd a regression test for streaming startup failures.
The suite covers streaming success and non-streaming
NotFoundError, but it does not coverstream=Truewhenresponses.create()fails before the first event. That is the new edge introduced by this refactor, and it's where the response format is easiest to regress.As per coding guidelines, Tests cover both success and error paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/test_v1.py` around lines 165 - 257, Add a regression test that exercises the streaming startup failure path by mocking mock_openai_client.responses.create to raise an openai.NotFoundError when called with stream=True and asserting the endpoint returns a non-streaming 404 JSON error; create a new test (e.g., test_chat_completions_streaming_startup_error) that sets mock_openai_client.responses.create.side_effect = openai.NotFoundError(...) with the same httpx.Response body used in test_chat_completions_backend_error, POST to "/v1/chat/completions" with "stream": True and the nonexistent model, then assert resp.status_code == 404 and resp.json()["error"]["message"] == "Model not found".
🧹 Nitpick comments (1)
src/goose_proxy/translators/response.py (1)
27-29: Consider restoring type annotation for better type safety.The parameter
outputlost its type annotation. While the function works correctly viaisinstancechecks, adding a type hint improves code documentation and enables static analysis.♻️ Suggested type annotation
def _extract_tool_calls( - output: list, + output: t.List[t.Any], ) -> t.Optional[t.List[ChatCompletionMessageToolCall]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/goose_proxy/translators/response.py` around lines 27 - 29, Restore a precise type annotation on the _extract_tool_calls function's parameter to improve static checking: change the signature from output: list to something like output: t.List[t.Union[ChatCompletionMessage, ChatCompletionMessageToolCall, dict]] (or the narrower union that matches your message types) so static analyzers know the expected message shapes; update any imports/types used in the annotation if necessary and keep the return type as t.Optional[t.List[ChatCompletionMessageToolCall]].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/goose_proxy/routers/v1.py`:
- Around line 27-32: The upstream client.responses.create(**params) call must be
performed before starting the SSE generator to ensure errors are handled by the
proxy; move the await client.responses.create(**params) out of the async def
generate() and assign its result (e.g., response_stream = await
client.responses.create(**params)) prior to returning StreamingResponse, then
have generate() iterate over translate_stream(response_stream, data.model) and
yield lines; keep the existing StreamingResponse(generate(),
media_type="text/event-stream") and preserve use of translate_stream and
StreamingResponse to match the non-streaming path error handling.
---
Outside diff comments:
In `@tests/routers/test_v1.py`:
- Around line 165-257: Add a regression test that exercises the streaming
startup failure path by mocking mock_openai_client.responses.create to raise an
openai.NotFoundError when called with stream=True and asserting the endpoint
returns a non-streaming 404 JSON error; create a new test (e.g.,
test_chat_completions_streaming_startup_error) that sets
mock_openai_client.responses.create.side_effect = openai.NotFoundError(...) with
the same httpx.Response body used in test_chat_completions_backend_error, POST
to "/v1/chat/completions" with "stream": True and the nonexistent model, then
assert resp.status_code == 404 and resp.json()["error"]["message"] == "Model not
found".
---
Nitpick comments:
In `@src/goose_proxy/translators/response.py`:
- Around line 27-29: Restore a precise type annotation on the
_extract_tool_calls function's parameter to improve static checking: change the
signature from output: list to something like output:
t.List[t.Union[ChatCompletionMessage, ChatCompletionMessageToolCall, dict]] (or
the narrower union that matches your message types) so static analyzers know the
expected message shapes; update any imports/types used in the annotation if
necessary and keep the return type as
t.Optional[t.List[ChatCompletionMessageToolCall]].
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e14104a2-f8ad-4529-a214-c9db61f9dbb5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
pyproject.tomlsrc/goose_proxy/app.pysrc/goose_proxy/exceptions.pysrc/goose_proxy/models/responses.pysrc/goose_proxy/routers/v1.pysrc/goose_proxy/translators/response.pysrc/goose_proxy/translators/streaming.pytests/routers/test_v1.pytests/test_exceptions.pytests/test_models.pytests/translators/test_response.pytests/translators/test_streaming.py
💤 Files with no reviewable changes (2)
- tests/test_models.py
- src/goose_proxy/models/responses.py
r0x0d
force-pushed
the
refactor/use-openai-sdk
branch
from
April 9, 2026 17:57
1f7d35d to
9519444
Compare
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_exceptions.py (2)
137-140: Strengthen timeout test to assert error payload shape.This case currently checks only status code. Also assert parsed body fields (at least
error.type == "api_error") so timeout-path contract regressions are caught.Suggested test assertion expansion
def test_timeout_error_returns_502(self): exc = openai.APITimeoutError(request=httpx.Request("POST", "http://test")) resp = _api_connection_error_handler(_dummy_request(), exc) assert resp.status_code == 502 + body = _parse(resp) + assert body["error"]["type"] == "api_error" + assert body["error"]["code"] == 502As per coding guidelines, "Tests cover both success and error paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_exceptions.py` around lines 137 - 140, Update the test_timeout_error_returns_502 test to also validate the response payload shape returned by _api_connection_error_handler when given an openai.APITimeoutError (created with _dummy_request()); after asserting resp.status_code == 502, parse the response body JSON and assert at minimum that it contains an "error" object and that error["type"] == "api_error" (optionally assert error["message"] or other expected fields to lock the contract).
65-124: Avoid mutating privatehttpx.Response._requestin tests.Using private internals makes these tests fragile across
httpxupdates. Thehttpx.Responseconstructor accepts a publicrequest=parameter; use it instead of post-construction assignment.Proposed refactor
- response = httpx.Response( - status_code=422, - json={"error": {"message": "Invalid parameters"}}, - ) - response._request = httpx.Request("POST", "http://test") + response = httpx.Response( + status_code=422, + json={"error": {"message": "Invalid parameters"}}, + request=httpx.Request("POST", "http://test"), + ) - response = httpx.Response( - status_code=500, - text="Internal Server Error", - ) - response._request = httpx.Request("POST", "http://test") + response = httpx.Response( + status_code=500, + text="Internal Server Error", + request=httpx.Request("POST", "http://test"), + ) - response = httpx.Response( - status_code=503, - json={"detail": "Service unavailable"}, - ) - response._request = httpx.Request("POST", "http://test") + response = httpx.Response( + status_code=503, + json={"detail": "Service unavailable"}, + request=httpx.Request("POST", "http://test"), + ) - response = httpx.Response( - status_code=429, - json={"error": {"message": "Rate limited"}}, - ) - response._request = httpx.Request("POST", "http://test") + response = httpx.Response( + status_code=429, + json={"error": {"message": "Rate limited"}}, + request=httpx.Request("POST", "http://test"), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_exceptions.py` around lines 65 - 124, Replace direct mutation of the private attribute response._request with the public constructor parameter: construct responses using httpx.Response(..., request=httpx.Request("POST", "http://test")) instead of assigning response._request after creation; update all tests in tests/test_exceptions.py (e.g., in test_falls_back_to_str_on_no_body, test_falls_back_to_str_exc_when_no_message_key, test_preserves_status_code, and the earlier 422 test) to pass request=httpx.Request(...) into httpx.Response and remove any response._request = ... assignments so the tests use the public API and remain stable across httpx versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_exceptions.py`:
- Around line 137-140: Update the test_timeout_error_returns_502 test to also
validate the response payload shape returned by _api_connection_error_handler
when given an openai.APITimeoutError (created with _dummy_request()); after
asserting resp.status_code == 502, parse the response body JSON and assert at
minimum that it contains an "error" object and that error["type"] == "api_error"
(optionally assert error["message"] or other expected fields to lock the
contract).
- Around line 65-124: Replace direct mutation of the private attribute
response._request with the public constructor parameter: construct responses
using httpx.Response(..., request=httpx.Request("POST", "http://test")) instead
of assigning response._request after creation; update all tests in
tests/test_exceptions.py (e.g., in test_falls_back_to_str_on_no_body,
test_falls_back_to_str_exc_when_no_message_key, test_preserves_status_code, and
the earlier 422 test) to pass request=httpx.Request(...) into httpx.Response and
remove any response._request = ... assignments so the tests use the public API
and remain stable across httpx versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c174ce2e-5b47-459d-b1ac-39cf04c65858
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
pyproject.tomlsrc/goose_proxy/app.pysrc/goose_proxy/exceptions.pysrc/goose_proxy/models/responses.pysrc/goose_proxy/routers/v1.pysrc/goose_proxy/translators/response.pysrc/goose_proxy/translators/streaming.pytests/routers/test_v1.pytests/test_exceptions.pytests/test_models.pytests/translators/test_response.pytests/translators/test_streaming.py
💤 Files with no reviewable changes (2)
- tests/test_models.py
- src/goose_proxy/models/responses.py
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- src/goose_proxy/translators/response.py
- tests/translators/test_streaming.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/goose_proxy/exceptions.py
- src/goose_proxy/app.py
- tests/translators/test_response.py
r0x0d
force-pushed
the
refactor/use-openai-sdk
branch
2 times, most recently
from
April 14, 2026 17:36
42d795e to
f3cd135
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/goose_proxy/translators/response.py (1)
27-29: Restore type annotation foroutputparameter to improve IDE support and static analysis.The parameter type was weakened from a typed list to bare
list. While runtimeisinstancechecks still work, restoring the proper type hint improves developer experience and type checking tools.+from openai.types.responses import ResponseOutputItem + def _extract_tool_calls( - output: list, + output: t.List[ResponseOutputItem], ) -> t.Optional[t.List[ChatCompletionMessageToolCall]]:
ResponseOutputItemis a union type in the SDK covering all possible output item types that can appear in a Responses API response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/goose_proxy/translators/response.py` around lines 27 - 29, The function _extract_tool_calls currently uses an untyped bare list for the parameter "output"; restore the precise annotation to improve IDE/static-analysis by changing the parameter type to list[ResponseOutputItem] (or typing.List[ResponseOutputItem]) so the signature reads _extract_tool_calls(output: t.List[ResponseOutputItem]) -> t.Optional[t.List[ChatCompletionMessageToolCall]] and update imports if needed to reference ResponseOutputItem; keep the return type as t.Optional[t.List[ChatCompletionMessageToolCall]] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/goose_proxy/app.py`:
- Around line 30-37: The lifespan shutdown currently awaits
openai_client.close() but leaves the custom http_client open; update the
shutdown sequence so after calling AsyncOpenAI.close() on openai_client you also
explicitly close the custom http_client (call its aclose/close method) to avoid
resource leaks—modify the block that creates AsyncOpenAI (symbols: AsyncOpenAI,
openai_client, http_client) to ensure you await http_client.aclose() after
awaiting openai_client.close().
---
Nitpick comments:
In `@src/goose_proxy/translators/response.py`:
- Around line 27-29: The function _extract_tool_calls currently uses an untyped
bare list for the parameter "output"; restore the precise annotation to improve
IDE/static-analysis by changing the parameter type to list[ResponseOutputItem]
(or typing.List[ResponseOutputItem]) so the signature reads
_extract_tool_calls(output: t.List[ResponseOutputItem]) ->
t.Optional[t.List[ChatCompletionMessageToolCall]] and update imports if needed
to reference ResponseOutputItem; keep the return type as
t.Optional[t.List[ChatCompletionMessageToolCall]] unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50836719-05c0-461d-9ae9-00a5c8860a3b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
pyproject.tomlsrc/goose_proxy/app.pysrc/goose_proxy/exceptions.pysrc/goose_proxy/models/responses.pysrc/goose_proxy/translators/response.pysrc/goose_proxy/translators/streaming.pysrc/goose_proxy/v1.pytests/test_exceptions.pytests/test_models.pytests/test_v1.pytests/translators/test_response.pytests/translators/test_streaming.py
💤 Files with no reviewable changes (2)
- tests/test_models.py
- src/goose_proxy/models/responses.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- tests/translators/test_response.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/goose_proxy/exceptions.py
- tests/translators/test_streaming.py
Member
|
The |
r0x0d
marked this pull request as draft
April 15, 2026 11:53
r0x0d
force-pushed
the
refactor/use-openai-sdk
branch
2 times, most recently
from
April 22, 2026 17:39
b6f8835 to
4e77e26
Compare
r0x0d
force-pushed
the
refactor/use-openai-sdk
branch
from
May 4, 2026 12:17
4e77e26 to
a91afdc
Compare
Replace manual HTTP communication, custom Pydantic models for the Responses API, and manual SSE parsing with the openai Python SDK. The SDK handles transport, serialization, streaming, and error typing, reducing ~300 lines of custom code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
r0x0d
force-pushed
the
refactor/use-openai-sdk
branch
from
May 4, 2026 12:21
a91afdc to
1d73b3d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
httpx.AsyncClientwithopenai.AsyncOpenAIfor backend communication, using the SDK'sresponses.create()for both streaming and non-streaming callsmodels/responses.py(~170 lines) — custom Pydantic models for the Responses API are now provided by the SDKrouters/v1.pyby removingcreate_response()andstream_response()helpers — the SDK handles HTTP transport, SSE parsing, and response deserializationopenai.APIStatusError/openai.APIConnectionErrorinstead of httpx error typesTest plan
ruff check)ty check)🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes