Repository navigation
feat(models): accept a pre-configured client in OpenAIResponsesModel [replica of harness-sdk#4967] - #13
feat(models): accept a pre-configured client in OpenAIResponsesModel [replica of harness-sdk#4967]#13poshinchen wants to merge 1 commit into
Conversation
Mirrors OpenAIModel's client parameter: an injected client is reused for every request and never closed by the model. With an injected client, stream() reads the rest of the body after the terminal event so the connection returns to the pool, and always closes the response. Resolves strands-agents#4544
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v3-reason (optimized) review
Assessment: comment
Assessment: Comment
This PR cleanly mirrors OpenAIModel's injected-client pattern for OpenAIResponsesModel, with sensible validation and well-reasoned connection-reuse draining. The approach and test coverage (parametrized injected vs. default paths, error-path draining) are solid.
Review Categories
- API review: A new public
__init__parameter changes the model's public API contract — please confirm theapi/needs-reviewlabel is applied. - Dependencies: Direct
httpximport should have a declared, upper-bounded direct dependency (please verify). - Error handling: Confirm "best-effort" drain/close logic catches a narrow exception set and surfaces missing
close/drain failures at debug level rather than silently. - Docs consistency: TS example uses
OpenAIModelwhile Python usesOpenAIResponsesModel. - Tests: A couple of injected-client tests could assert full event/request shape instead of only call counts.
Nice work pairing the connection-reuse fix with the client injection — the draining rationale is clearly documented.
File-level comments:
strands-py/src/strands/models/openai_responses.py: Suggestion
Issue: _close_response_stream uses getattr(response, "close", None) and only calls it if present, silently doing nothing when close is absent — this could mask a connection leak if the response shape changes.
Suggestion: Please verify the response object is always expected to have a close; if so, consider logging at debug level when close is missing so leaks are observable rather than silent.
strands-py/src/strands/models/openai_responses.py: Important
Issue: The new httpx import at module level is added for exception handling in _drain_for_connection_reuse (per the diff). httpx is a transitive dependency of openai, so importing it directly adds an undeclared direct dependency.
Suggestion: Please verify httpx is declared as a direct dependency with a supported upper bound in the package metadata; otherwise catch the exceptions via openai's re-exported types or guard the import.
strands-py/tests/strands/models/test_openai_responses.py: Suggestion
Issue: test_count_tokens_with_injected_client asserts only tru_tokens == 42 plus client-lifecycle calls, but not that responses.input_tokens.count was awaited with the expected request shape.
Suggestion: Consider adding injected_client.responses.input_tokens.count.assert_awaited_once() and asserting the forwarded kwargs so a regression in request construction is caught, consistent with the other injected-client tests.
strands-py/tests/strands/models/test_openai_responses.py: Suggestion
Issue: test_stream_with_injected_client asserts await_count == 2 and lifecycle non-calls, but does not assert the emitted events match an expected full sequence.
Suggestion: Consider asserting the full event list (as test_stream_reads_past_terminal_event_only_with_injected_client does) so the injected-client path's output shape is verified in a single equality check rather than only the call count.
| client_args: dict[str, Any] | None = None, | ||
| bedrock_mantle_config: BedrockMantleConfig | None = None, | ||
| *, | ||
| client: Client | None = None, |
There was a problem hiding this comment.
Important
Issue: The docstring for client lists use cases ("Injecting custom client wrappers (e.g., GuardrailsAsyncOpenAI)") but the PR description states client is keyword-only because client_args is positional. The parameter ordering places client after bedrock_mantle_config, which is reasonable, but new public API surface likely requires API review.
Suggestion: Please confirm this PR carries the api/needs-review label — a new public __init__ parameter on a customer-facing model class changes the public API contract.
| // Custom client | ||
| { | ||
| // --8<-- [start:custom_client] | ||
| const client = new OpenAI({ apiKey: '<KEY>' }) |
There was a problem hiding this comment.
Suggestion
Issue: The TypeScript doc example under custom_client instantiates OpenAIModel, but this PR is about OpenAIResponsesModel. The Python tab uses OpenAIResponsesModel, creating an inconsistency on the OpenAI Responses provider page.
Suggestion: Please verify whether the TS SDK exposes a Responses-specific model; if so, use it here for parity with the Python example to avoid confusing readers.
| @@ -210,6 +258,45 @@ def _resolve_client_args(self) -> dict[str, Any]: | |||
| ) | |||
There was a problem hiding this comment.
Important
Issue: _drain_for_connection_reuse (and the error-path draining) catch broad exceptions as "best effort" per the docstring. The parametrized test covers httpx.RemoteProtocolError and openai.APIError, but the catch scope isn't fully visible in the diff.
Suggestion: Please verify the drain catches a narrow, intentional exception set rather than bare Exception, so genuine bugs during draining aren't silently swallowed. If broad catching is intended, log at debug level for observability.
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/4967(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.