feat(clients): add Python/Rust HTTP client SDKs and OpenAPI codegen pipeline - #539
Merged
Merged
Conversation
github-actions
Bot
added
dependencies
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds generated SDK tooling and new client crates for Python and Rust, an OpenAPI generator, Makefile targets for generation, extensive schemars::JsonSchema derives across protocol types, client transports with retry/backoff and streaming/SSE support, error hierarchies, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Client as SmgClient/AsyncSmgClient
participant Transport as SyncTransport/AsyncTransport
participant HTTP as HTTP Client (httpx/reqwest)
participant Server as SMG Server
App->>Client: call create(request)
Client->>Transport: request("POST","/v1/...", json=request)
Transport->>HTTP: send request (with retry/backoff)
HTTP->>Server: HTTP request
Server-->>HTTP: response (200 / 4xx / 429/5xx)
HTTP-->>Transport: Response
alt 200
Transport->>Client: return response
Client-->>App: parsed model
else 429/500/503
Transport->>Transport: backoff & retry
else 4xx
Transport-->>Client: mapped error
Client-->>App: raise
end
sequenceDiagram
participant App as Application
participant Client as SmgClient
participant Transport as Transport
participant HTTP as HTTP Client
participant SSE as SSE Parser
participant Stream as TypedStream / Pydantic
App->>Client: create_stream(request)
Client->>Transport: request(..., stream=True)
Transport->>HTTP: POST (stream)
HTTP-->>Transport: streaming response (bytes)
Transport->>SSE: pass bytes
loop per SSE chunk
SSE->>Stream: emit SseEvent
Stream->>Stream: deserialize event.data -> typed chunk
Stream-->>App: yield chunk
end
Note over SSE: [DONE] sentinel ends stream
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
Contributor
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Contributor
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the developer experience by introducing robust, auto-generated client SDKs for Python and Rust. The core innovation lies in a new OpenAPI codegen pipeline that ensures protocol types are consistently and automatically reflected across different language clients, thereby reducing maintenance overhead and the potential for discrepancies. This change streamlines the process of integrating with SMG, making it easier for developers to build applications using their preferred language while benefiting from up-to-date and accurate type definitions. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39fbe82cbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an excellent pull request that adds comprehensive and well-structured HTTP client SDKs for both Python and Rust. The introduction of an OpenAPI-based code generation pipeline is a fantastic strategy for maintaining type safety and consistency between the server and clients, significantly reducing the potential for manual errors. The clients are feature-rich, including support for streaming, error handling, and automatic retries. My feedback includes a couple of critical suggestions to improve resource management in the Python client's transport layer, a minor maintainability improvement for the OpenAPI generator, and a note on a potential performance optimization in the Rust client.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protocols/src/embedding.rs (1)
66-72:⚠️ Potential issue | 🟠 MajorNew required fields
modelandusageare a breaking deserialization change.Adding non-optional fields to
EmbeddingResponsemeans any existing serialized payloads (tests, fixtures, stored data) that lackmodelorusagewill now fail to deserialize. If any backend returns an embedding response without these fields, this is a runtime error. Verify that all downstream consumers and test fixtures have been updated.#!/bin/bash # Search for any existing serialized EmbeddingResponse payloads or test fixtures # that may not include the new `model` and `usage` fields. rg -rn 'EmbeddingResponse' --type=rust --type=json -C 3#!/bin/bash # Check if there are any tests deserializing EmbeddingResponse from JSON rg -rn 'EmbeddingResponse' -g '*.rs' -g '*.json' -g '*.py' -C 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@protocols/src/embedding.rs` around lines 66 - 72, The new non-optional fields on the EmbeddingResponse struct (model: String and usage: UsageInfo) are a breaking deserialization change; change these fields to be backward-compatible by making them optional or supplying serde defaults (e.g., model: Option<String> and usage: Option<UsageInfo> or add #[serde(default)] with sensible defaults), update the EmbeddingResponse definition to use those optional/defaulted types, and then update any tests/fixtures to include the fields where appropriate (or keep them omitted if optional) so deserialization of older payloads and current backends won't fail; locate and modify the EmbeddingResponse struct and any code that reads .model or .usage to handle the Option/Default safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/openapi-gen/src/main.rs`:
- Around line 84-107: The fixup_schema function currently converts any
Value::Bool(true) inside Value::Array to an empty object, which corrupts
non-combinator arrays and also omits converting a top-level true; update
fixup_schema to be key-aware: change the Value::Array handling so it only treats
true as schema-combinator placeholders when the parent key is one of "anyOf",
"oneOf", or "allOf" (i.e., only convert true->{} for those arrays), otherwise
recurse into array items without changing booleans; additionally, add handling
in the top-level match to convert a top-level Value::Bool(true) into
Value::Object(Map::new()) so standalone boolean schemas are fixed (refer to the
function name fixup_schema and the Value::Array / Value::Bool(true) branches to
locate edits).
In `@clients/python/smg_client/__init__.py`:
- Around line 28-35: The module currently re-exports ConnectionError and
TimeoutError (alongside other errors) which shadow Python builtins; update
clients/python/smg_client/__init__.py to add a brief explanatory comment or note
in the module docstring near the error imports (mentioning ConnectionError and
TimeoutError) stating that the shadowing is intentional and these names are the
client-specific exception classes being exported so future maintainers don't
mistake them for the builtins; ensure the comment references the exported
symbols ConnectionError and TimeoutError and remains concise.
In `@clients/python/smg_client/_client.py`:
- Around line 105-109: The example incorrectly uses "async with
client.chat.completions.create_stream(...)" on the coroutine returned by async
def create_stream; instead await the coroutine to get the AsyncStream and then
use it as an async context manager — i.e., call await
client.chat.completions.create_stream(...) to obtain the stream (AsyncStream)
and then "async with" that stream before iterating (references:
client.chat.completions.create_stream, stream/AsyncStream).
In `@clients/python/smg_client/_errors.py`:
- Around line 62-67: The module currently defines ConnectionError and
TimeoutError which shadow Python built-ins; either explicitly declare and export
these names via an __all__ list to make the shadowing intentional (e.g., add
"__all__ = ['SmgError', 'ConnectionError', 'TimeoutError', ...]") or rename the
classes to SmgConnectionError and SmgTimeoutError and update all
references/usages accordingly (search for ConnectionError and TimeoutError in
the package to update imports and exception handling). Ensure the chosen
approach is consistent across the package and update any tests or docs
referencing those symbols.
- Around line 92-110: The Anthropic-style branch (the condition checking
data.get("type") == "error") is shadowed by the prior check for "error" in data
being a dict; to fix, reorder the parsing logic in
clients/python/smg_client/_errors.py so you first check if data.get("type") ==
"error" and extract err = data.get("error", {}) -> message, error_type, param,
code, then fall back to the existing branches that handle "error" as a dict or
string; update references to variables message, error_type, param, and code
accordingly to ensure Anthropic responses ({"type":"error","error":{...}}) are
parsed correctly.
In `@clients/python/smg_client/_sse.py`:
- Around line 37-55: The SSE parsing loop currently yields one SseEvent per
"data:" line which fragments multi-line SSE payloads; modify the logic in the
generator that iterates response.iter_lines() so it accumulates consecutive
"data:" lines into a buffer (e.g., append each stripped data line), and only
when an empty line (event boundary) is seen join the buffered lines with "\n"
into a single payload, check for the "[DONE]" sentinel on the combined payload,
then yield SseEvent(data=combined_data, event=event_type) and reset both the
buffer and event_type; apply the same buffered-join change to the second block
handling lines 64-81 so multi-line SSE events are emitted as one SseEvent.
In `@clients/python/smg_client/_streaming.py`:
- Around line 89-93: The example usage in the docstring for
client.messages.create_stream incorrectly uses attribute access on events while
the stream iterator's __next__ returns a dict; update the example to access keys
instead (e.g., use event["type"] and event["delta"]["text"] rather than
event.type and event.delta.text) so the sample loop over the stream matches the
dict-returning iterator implementation.
- Around line 54-56: The example incorrectly uses the coroutine returned by
client.chat.completions.create_stream(req) directly as an async context manager;
await the coroutine first to get the AsyncStream object and then use that object
with the async with statement. Update the snippet that calls
client.chat.completions.create_stream(req) so you either await it into a
variable (e.g., stream = await client.chat.completions.create_stream(req)) and
then do async with that stream (async with stream: ...) or use an await
expression before the context manager, ensuring the AsyncStream (from
create_stream) is awaited before entering the async context.
In `@clients/python/smg_client/_transport.py`:
- Line 73: The current assignment body = resp.read().decode() can raise
UnicodeDecodeError for non-UTF-8 responses; change it to use resp.text when
available (to leverage encoding negotiation) or fall back to decoding bytes with
errors="replace" to avoid exceptions. Concretely, replace the direct
resp.read().decode() call (the code that sets the variable body) with a safe
branch that prefers resp.text if the response object exposes it, otherwise
decodes resp.read() with .decode("utf-8", errors="replace") (or wrap the decode
in a try/except UnicodeDecodeError to replace invalid bytes).
- Around line 68-79: The streaming branch leaks the context manager because you
call response = self._client.stream(...) and response.__enter__() to get resp
but never call response.__exit__(); fix by not returning the raw resp—either
return the context manager itself so callers can use `with response:` (replace
`return resp` with `return response`) or explicitly manage enter/exit here (use
try/finally around response.__enter__()/response.__exit__() and only return the
context manager on success). Update references in this block
(self._client.stream, response, resp) and keep existing retry logic
(_should_retry, _retry_delay, raise_for_status) intact.
- Around line 138-148: The streaming branch leaks the async context manager by
calling response.__aenter__() without __aexit__; replace the use of
self._client.stream(...).__aenter__() with a direct send call that requests
streaming (e.g. await self._client.send(..., stream=True)) so you get a response
object without entering a context manager, then keep the existing error
handling: await resp.aread(), await resp.aclose() on error, use
_should_retry(resp.status_code, attempt, self._config.max_retries) and
_retry_delay(attempt, resp) for retries, and return resp on success; reference
the existing symbols self._client.stream, self._client.send, _should_retry,
_retry_delay, and raise_for_status to locate and update the code.
In `@clients/python/smg_client/api/messages.py`:
- Around line 26-28: The create() implementations incorrectly use
kwargs.setdefault("stream", False) which allows callers to pass stream=True;
change both Messages.create (sync) and AsyncMessages.create (async) to force
non-streaming by assigning kwargs["stream"] = False (matching the create_stream
pattern) before calling self._transport.request so the response is non-streaming
and safe for Message.model_validate_json to parse.
In `@clients/python/tests/test_errors.py`:
- Around line 1-62: Add tests in clients/python/tests/test_errors.py that call
raise_for_status to exercise HTTP 503 and 403 paths: create one test similar to
test_raise_for_status_500 that calls raise_for_status(503, "Service
Unavailable") and asserts that ServiceUnavailableError is raised, and another
test similar to test_raise_for_status_401 or 404 that calls
raise_for_status(403, '{"error": {"message": "Forbidden"}}') (and also try plain
text "Forbidden") and asserts PermissionDeniedError is raised; reference the
raise_for_status function and the exported exception classes
ServiceUnavailableError and PermissionDeniedError in your assertions.
In `@clients/rust/src/api/chat.rs`:
- Around line 22-29: The create method currently assumes non-stream JSON and
create_stream assumes SSE, so first guard the request.stream flag in both
functions: in create (function create) check that request.stream is false and
return a clear SmgError (or map to an API error) if true; in create_stream
(function create_stream) check that request.stream is true and return an
explicit SmgError if false; update the error messages to indicate the mismatched
streaming mode (include the field name request.stream and the expected mode) so
callers get a clear, actionable error instead of opaque parse/stream failures
(apply the same guard to the analogous code block around create_stream described
in the comment).
In `@clients/rust/src/error.rs`:
- Around line 76-106: The match in clients/rust/src/error.rs currently falls
through unknown 4xx codes to Self::Server; update the default branch in that
match (the block constructing error variants) to detect 400..=499 and return a
client-side error variant (e.g., Self::Client { message, status, body: parsed })
instead of Server, and only treat non-4xx codes as Server; if a Client variant
does not exist on the error enum, add a dedicated variant (e.g., Client) and
construct it from this match so unrecognized 4xx statuses like 422 are
classified as client errors.
In `@clients/rust/src/streaming/mod.rs`:
- Around line 8-18: The function __test_sse_stream is exported publicly even
though it's meant as a test helper; make it non-public by removing the pub
visibility (or if other crate-local tests need it, change to pub(crate)) so it
no longer becomes part of the crate's public semver surface; update the
signature at the function definition that currently reads pub fn
__test_sse_stream(... ) to either fn __test_sse_stream(...) or pub(crate) fn
__test_sse_stream(...) and keep the call to sse_stream(...) unchanged.
In `@clients/rust/src/streaming/sse.rs`:
- Around line 78-97: The check that treats data == "[DONE]" as a sentinel
currently skips the event rather than terminating the stream; update the code by
adding a brief clarifying comment next to the sentinel handling in try_parse
(around the current_data/current_event and data == "[DONE]" branch) stating that
“[DONE] is skipped here and does not close the stream — stream termination is
driven by the underlying transport/connection closing, not by this sentinel,” so
readers understand this is intentional and no additional termination logic is
required in SseEvent/try_parse/unfold.
- Around line 114-131: The flush method currently ignores the passed-in buffer
and thus drops any partial line when the stream ends; change fn flush(&mut self,
_buffer: &mut String) -> Option<SseEvent> to use the provided buffer (remove the
underscore) and process any remaining content: append buffer contents (trim a
trailing '\n' if present) to self.current_data (or feed it into the same parsing
logic used by try_parse), clear the buffer, then build and return the final
SseEvent using self.current_event and the joined self.current_data (preserving
the existing "[DONE]" check to return None), and clear
current_data/current_event afterwards so no partial data is lost on stream
close.
In `@clients/rust/src/streaming/typed_stream.rs`:
- Around line 12-22: Remove the redundant #[pin] on TypedStream::inner (which is
a Pin<Box<dyn Stream<...> + Send>>) and update any projection/uses in poll_next
to access &mut self.inner directly (no pin projection macro needed for that
field); specifically, delete #[pin] before inner, and in the impl for
Stream/TypedStream::poll_next switch from projected_inner or
self.project().inner to working with &mut self.inner (and call Pin::as_mut or
Pin::new(self.inner.as_mut()) only if you need a Pin<&mut _> for calling
Stream::poll_next).
In `@clients/rust/src/transport.rs`:
- Around line 63-71: Update the doc comment for post_stream to explicitly
explain that retries are disabled once the response body begins because
server-side effects (e.g., token generation) cannot be replayed, but note that
connection-level failures before any bytes are received are safe to retry;
modify post_stream (and related SmgError handling) to distinguish
connection-level errors that occur before the response stream starts from errors
occurring after the response has begun: perform a short retry loop around
self.client.post(self.url(path)).json(body).send().await to handle transient
connection failures (mapping those failures into retryable
SmgError::Connection), while ensuring that once check_status(resp).await is
called (or any response body is consumed) no retries are attempted; keep
check_status usage and preserve existing error mapping for non-retryable
streaming failures.
- Around line 17-39: Add a dedicated error variant to SmgError (e.g.,
SmgError::Config(String)) and replace the catch-all SmgError::Stream(...) usages
in the transport constructor with it: in the Transport::new function (the new
method that builds the reqwest Client) change the mapping for header parsing and
client building errors to return SmgError::Config with descriptive messages
instead of SmgError::Stream; also search for other non-stream error mappings
(e.g., retry/construction sites currently using SmgError::Stream) and adjust
them or document why Stream is appropriate so configuration/construction
failures are distinguishable from stream errors.
- Around line 138-142: The backoff_delay function currently returns a
deterministic exponential delay; update backoff_delay(attempt: u32) to add
randomized jitter: compute the exponential base_ms as before (500ms *
2^attempt), then generate a small random jitter (e.g., using
rand::thread_rng().gen_range(0..=(base_ms / 2)) or ±50% as preferred) and add it
to the base, finally clamp the result to the 30_000 ms maximum and return
Duration::from_millis(clamped_value); import rand::Rng and keep the function
signature unchanged so callers of backoff_delay continue to work.
In `@clients/rust/tests/test_error.rs`:
- Around line 39-58: Add a test that verifies SmgError::from_status treats an
unmapped 4xx (e.g., 422) as the BadRequest fallback: create an error via
SmgError::from_status(422, "unprocessable entity"), match on
SmgError::BadRequest { message, body, .. } and assert the message equals
"unprocessable entity" and body.is_none(), otherwise panic; reference
SmgError::from_status and the SmgError::BadRequest variant in the new test
(e.g., test_error_from_status_unmapped_4xx).
In `@clients/rust/tests/test_sse.rs`:
- Around line 5-11: The current bytes_stream helper returns the whole payload as
a single Bytes chunk so the SSE parser never sees split boundaries; create a
multi-chunk variant (or overload bytes_stream) that yields at least two chunks
by splitting the input mid-line (for example split between the "data: " prefix
and the JSON body) and use that in a new test to feed the SSE parser (the test
exercising the same parsing path as existing tests). Update or add a test that
calls the SSE consumer with this multi-chunk stream to verify buffering and
reassembly across chunk boundaries, referencing the bytes_stream helper and the
test function that invokes the SSE parser.
- Line 17: The test currently swallows parse errors by using
stream.filter_map(|r| async { r.ok() }).collect().await which turns Err items
into missing events and leads to misleading assertions; instead collect the
stream of Result<SseEvent, E> (the variable stream) into a Vec<Result<_, _>> or
into a Result<Vec<SseEvent>, _> first (e.g. collect::<Result<Vec<_>,
_>>().await), then unwrap or assert on the Results so any parse error surfaces
with its error message; apply the same change in both tests referenced (the one
using SseEvent and the test_sse_parses_anthropic_format) so assertions operate
on actual SseEvent values after explicit Result handling.
In `@Makefile`:
- Around line 141-151: The Makefile target invoking the generator uses an
unpinned datamodel-codegen binary (the uvx line that starts with "--from
datamodel-code-generator datamodel-codegen"), causing non-deterministic outputs;
pin the generator to an exact published version by replacing the plain
"datamodel-codegen" token with an exact package spec like
"datamodel-codegen==X.Y.Z" (or otherwise ensure uvx installs a fixed version),
update the uvx invocation in that Makefile rule accordingly, and document the
chosen version so CI and local runs use the same generator binary.
---
Outside diff comments:
In `@protocols/src/embedding.rs`:
- Around line 66-72: The new non-optional fields on the EmbeddingResponse struct
(model: String and usage: UsageInfo) are a breaking deserialization change;
change these fields to be backward-compatible by making them optional or
supplying serde defaults (e.g., model: Option<String> and usage:
Option<UsageInfo> or add #[serde(default)] with sensible defaults), update the
EmbeddingResponse definition to use those optional/defaulted types, and then
update any tests/fixtures to include the fields where appropriate (or keep them
omitted if optional) so deserialization of older payloads and current backends
won't fail; locate and modify the EmbeddingResponse struct and any code that
reads .model or .usage to handle the Option/Default safely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (54)
.gitignoreCargo.tomlMakefileclients/openapi-gen/Cargo.tomlclients/openapi-gen/src/main.rsclients/python/pyproject.tomlclients/python/smg_client/__init__.pyclients/python/smg_client/_client.pyclients/python/smg_client/_config.pyclients/python/smg_client/_errors.pyclients/python/smg_client/_sse.pyclients/python/smg_client/_streaming.pyclients/python/smg_client/_transport.pyclients/python/smg_client/api/__init__.pyclients/python/smg_client/api/chat.pyclients/python/smg_client/api/completions.pyclients/python/smg_client/api/embeddings.pyclients/python/smg_client/api/messages.pyclients/python/smg_client/api/models.pyclients/python/smg_client/api/rerank.pyclients/python/smg_client/types/__init__.pyclients/python/tests/__init__.pyclients/python/tests/test_errors.pyclients/python/tests/test_sse.pyclients/python/tests/test_types.pyclients/rust/Cargo.tomlclients/rust/src/api/chat.rsclients/rust/src/api/completions.rsclients/rust/src/api/embeddings.rsclients/rust/src/api/messages.rsclients/rust/src/api/mod.rsclients/rust/src/api/models.rsclients/rust/src/api/rerank.rsclients/rust/src/client.rsclients/rust/src/config.rsclients/rust/src/error.rsclients/rust/src/lib.rsclients/rust/src/streaming/mod.rsclients/rust/src/streaming/sse.rsclients/rust/src/streaming/typed_stream.rsclients/rust/src/transport.rsclients/rust/tests/test_error.rsclients/rust/tests/test_sse.rsprotocols/Cargo.tomlprotocols/src/chat.rsprotocols/src/common.rsprotocols/src/completion.rsprotocols/src/embedding.rsprotocols/src/generate.rsprotocols/src/messages.rsprotocols/src/rerank.rsprotocols/src/responses.rsprotocols/src/sampling_params.rsprotocols/src/tokenize.rs
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 25, 2026 19:03
39fbe82 to
f32751e
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32751e7fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (7)
clients/python/smg_client/_streaming.py (2)
49-79: Async stream example and implementation are correct.The docstring at Line 54 now correctly shows
stream = await client.chat.completions.create_stream(req)before entering the async context manager. The__anext__implementation properly awaits the async SSE iterator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_streaming.py` around lines 49 - 79, The AsyncStream implementation and docstring are correct; no code changes required—keep AsyncStream.__anext__, __aiter__, __aenter__, __aexit__, and aclose as implemented, and retain the example usage showing await client.chat.completions.create_stream(req) before entering the async context manager to ensure correct async iteration behavior.
82-120: Anthropic sync stream — doc example now correctly uses dict access.Lines 92-94 use
event.get("type")andevent.get("delta", {})matching thedictreturn type of__next__. Thetypefield injection logic at Lines 109-110 is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_streaming.py` around lines 82 - 120, The implementation of AnthropicSyncStream.__next__ correctly returns a dict (using event.json()) and the example usage using dict access is appropriate; keep the SSE-to-JSON type injection logic (if event.event and "type" not in data: data["type"] = event.event) as-is and do not change the return type or the example usage referencing event.get("type") and event.get("delta", {}); no code modifications required to iter_sse_sync, __next__, or the class doc example.clients/rust/src/transport.rs (3)
142-154: Jitter implementation is reasonable for avoiding aranddependency.Using
SystemTimesubsec nanos provides sufficient variation for retry jitter in a client SDK. The prior review asking for jitter has been addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/transport.rs` around lines 142 - 154, The review notes that backoff_delay's jitter implementation (function backoff_delay) is acceptable and approves the change, but the comment is duplicated; no code changes needed—leave function backoff_delay as-is and remove the duplicate review comment/annotation so only a single approval comment remains.
63-75: Streaming requests now have clear documentation for why retries are skipped.The doc comment explains that once token generation begins, the response can't be replayed. This addresses the prior review feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/transport.rs` around lines 63 - 75, The doc comment for post_stream documents why streaming requests aren't retried, but the PR/comment contains duplicate review markers ([approve_code_changes] and [duplicate_comment]) — remove the duplicate/erroneous annotation from the submission (leave the clarified doc comment above pub(crate) async fn post_stream intact) so only the intended approval/comment remains and no duplicate markers are present.
17-39:SmgError::Streamis still used as a catch-all for configuration/construction errors.Lines 26, 33 map header-parse and client-build failures to
SmgError::Stream, which semantically describes streaming errors. This conflation was flagged in the prior review and remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/transport.rs` around lines 17 - 39, The new() constructor in transport.rs is mapping header parse and client build failures to SmgError::Stream which is semantically wrong; update the two map_err() calls in Client::builder().timeout(...) -> HeaderValue::from_str(...) and builder.build() to return a configuration/construction-specific error variant (e.g. SmgError::Config or SmgError::TransportInit) instead of SmgError::Stream, and if that variant does not exist add it to the SmgError enum with appropriate display/debug messages so header-parse and client-build failures are clearly distinguished from streaming errors.clients/python/smg_client/api/messages.py (1)
26-28:kwargs["stream"] = Falseis now correctly used, consistent with other API modules.This addresses the prior review feedback about
setdefaultallowing callers to passstream=Trueintocreate().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/api/messages.py` around lines 26 - 28, Ensure the create message call forces non-streaming by assigning kwargs["stream"] = False (not using setdefault) before sending the request; specifically, in the function that calls self._transport.request("POST", "/v1/messages", json=kwargs) set kwargs["stream"]=False so callers cannot pass stream=True, then parse the response with Message.model_validate_json(resp.content).clients/openapi-gen/src/main.rs (1)
82-113:fixup_schemanow correctly scopes boolean-to-object conversion to combinator arrays.The implementation properly limits the
true→{}replacement to arrays underanyOf/oneOf/allOfkeys, and recurses normally for all other values. This addresses the prior review feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` around lines 82 - 113, The fixup_schema function already scopes the boolean-to-object conversion correctly by only replacing Value::Bool(true) within arrays found under keys "anyOf", "oneOf", or "allOf" and otherwise recursion handles other nodes; ensure this implementation in fn fixup_schema remains as-is (retain the match on Value::Object that checks keys with matches!(k.as_str(), "anyOf" | "oneOf" | "allOf"), the inner Value::Array handling that replaces true with an empty Object, and the recursive calls for other branches) — no further changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/openapi-gen/src/main.rs`:
- Around line 117-143: collect_schema currently inserts entries into the schemas
map with schemas.insert(name.clone(), value) and schemas.insert(title.clone(),
root_value) which silently overwrite existing entries; update collect_schema to
check for an existing entry before inserting (use schemas.get(&key) to compare
existing value vs. new Value) and if they differ, emit a clear stderr warning
(e.g., via eprintln!) naming the conflicting key (name or title), indicating
which value is being replaced; then proceed to replace or skip per desired
behavior so collisions are no longer silent.
- Around line 213-324: The wildcard `use` declarations inside main (e.g., `use
openai_protocol::chat::*`, `use openai_protocol::completion::*`, etc.) obscure
the origins of types like `ListModelsResponse` used with
`collect_schema`/`schema_for!`; replace these glob imports with explicit imports
at the top of the file (for example import `ChatCompletionRequest`,
`ChatCompletionResponse`, `CompletionRequest`, `CompletionResponse`,
`EmbeddingRequest`, `EmbeddingResponse`, `ListModelsResponse`, etc.) so each
type's module is clear, or if you prefer minimal change add a short inline
comment next to less-obvious uses (e.g., `ListModelsResponse`) stating its
module; update references in `main()` to rely on those explicit top-level
imports and remove the in-function `use ...::*` lines.
- Line 350: The code currently calls serde_yaml::to_string(&doc) (producing the
variable yaml), but serde_yaml is archived and unmaintained; update the
dependency to use the maintained fork serde_yaml_ng while keeping the serde_yaml
import path by changing your Cargo.toml dependency to: serde_yaml = { package =
"serde_yaml_ng", version = "0.10" } so existing uses of serde_yaml::to_string
and other serde_yaml symbols remain unchanged and no source changes are
required.
In `@clients/python/smg_client/_sse.py`:
- Around line 28-30: The json() method's return annotation is too narrow; update
the return type of json(self) in clients/python/smg_client/_sse.py from dict to
a broader type (e.g., Any or Union[dict, list, str, int, float, bool, None]) to
reflect json.loads() possible returns; import Any (or the chosen Union types)
from typing and adjust the signature of json() accordingly while keeping the
implementation using json.loads(self.data).
In `@clients/python/smg_client/api/chat.py`:
- Around line 20-31: The create (and create_stream) methods currently accept
**kwargs: Any and blindly forward them to _transport.request, which allows
silent typos; change the signature of create in
clients/python/smg_client/api/chat.py to accept a typed request object (e.g.,
ChatCompletionRequest) or a TypedDict plus an optional **overrides,
validate/convert that model to a dict before calling
self._transport.request("POST", "/v1/chat/completions", json=payload), and keep
returning ChatCompletionResponse.model_validate_json(resp.content); apply the
same pattern to the corresponding create/create_stream methods in completions,
embeddings, messages, and rerank modules so callers get IDE type safety and
early validation instead of runtime server errors.
In `@clients/python/smg_client/types/__init__.py`:
- Around line 46-50: The current except ImportError as _e in
clients/python/smg_client/types/__init__.py is too broad and can mask real
import errors; change it to only convert a missing smg_client.types._generated
module into the friendly message: catch ModuleNotFoundError (or inspect _e.name)
and if _e.name == "smg_client.types._generated" raise the custom ImportError
message from _e, otherwise re-raise the original exception so other ImportError
causes are not swallowed; reference the existing except ImportError as _e line
and the target module name smg_client.types._generated when implementing this
narrowing logic.
In `@clients/python/tests/test_types.py`:
- Around line 77-79: Replace fragile exact float/list equality checks with
pytest.approx: in the test that calls EmbeddingResponse.model_validate and
asserts resp.data[0].embedding == [0.1, 0.2, 0.3], change that assertion to
compare using pytest.approx (e.g., resp.data[0].embedding == pytest.approx([0.1,
0.2, 0.3])). Do the same for the other floating assertions around lines ~110-112
(e.g., any assertions comparing usage or scalar floats to 0.95) so they use
pytest.approx; add an import for pytest.approx if needed. Ensure you update all
occurrences mentioned so floating comparisons are tolerant and
intention-revealing.
- Line 127: The test is coupled to an implementation detail by accessing the
Enum `.value` on msg.content[0].type; change the assertion to compare the actual
string (e.g., assert msg.content[0].type == "text") so it works whether `type`
is a str or enum, or if `type` must remain an Enum keep the `.value` but add a
clarifying comment next to the assertion explaining that `type` is intentionally
an Enum and `.value` is required for the string comparison (reference: the
assertion using msg.content[0].type.value).
In `@clients/rust/src/client.rs`:
- Around line 46-73: Accessor methods chat(), completions(), embeddings(),
messages(), models(), and rerank() each clone the Transport and return a new
wrapper (e.g., Chat::new(self.transport.clone())), which can allocate repeatedly
in hot loops; update the doc comments on these methods to explicitly state that
cloning is cheap but callers should cache the returned wrapper (for example, let
chat = client.chat(); reuse chat for multiple calls) to avoid repeated
allocation, and include a short example showing preferred vs non-preferred usage
so users know to reuse the Chat/Completions/Embeddings/Messages/Models/Rerank
instances rather than calling client.*() repeatedly.
In `@clients/rust/src/streaming/sse.rs`:
- Around line 85-104: When encountering an empty line in the SSE parser where
self.current_data.is_empty() is true, clear the stale event type by resetting
self.current_event (e.g., set to None) before continuing so the next non-empty
data frame doesn't inherit an old event; update the branch in the method
handling empty lines (the one that currently checks self.current_data,
constructs SseEvent, compares data == "[DONE]" to set self.done, and continues)
to explicitly clear self.current_event when skipping keep-alive/empty-boundary
frames.
In `@clients/rust/src/streaming/typed_stream.rs`:
- Around line 32-33: Change the comment on the Unpin impl for TypedStream<T> to
use the repository's safe-code marker: replace the "SAFETY:" prefix with
"INVARIANT:" and keep the rest of the text intact (i.e., document that
TypedStream is Unpin because Pin<Box<...>> is always Unpin) next to the impl<T>
Unpin for TypedStream<T> {} declaration so it follows repo conventions for
safe-code assumptions.
In `@clients/rust/src/transport.rs`:
- Around line 77-129: Add a brief inline comment inside send_with_retry next to
the Retry-After parsing (the block that converts header to f64 and maps to
Duration or falls back to backoff_delay) explaining that Retry-After may be an
HTTP-date (e.g., "Wed, 21 Oct 2015 07:28:00 GMT") which will fail f64 parsing
and intentionally fall back to exponential backoff; reference the
variables/funcs involved (headers().get("retry-after"), parse::<f64>(),
std::time::Duration::from_secs_f64, backoff_delay) so future readers understand
the silent fallback behavior.
---
Duplicate comments:
In `@clients/openapi-gen/src/main.rs`:
- Around line 82-113: The fixup_schema function already scopes the
boolean-to-object conversion correctly by only replacing Value::Bool(true)
within arrays found under keys "anyOf", "oneOf", or "allOf" and otherwise
recursion handles other nodes; ensure this implementation in fn fixup_schema
remains as-is (retain the match on Value::Object that checks keys with
matches!(k.as_str(), "anyOf" | "oneOf" | "allOf"), the inner Value::Array
handling that replaces true with an empty Object, and the recursive calls for
other branches) — no further changes are required.
In `@clients/python/smg_client/_streaming.py`:
- Around line 49-79: The AsyncStream implementation and docstring are correct;
no code changes required—keep AsyncStream.__anext__, __aiter__, __aenter__,
__aexit__, and aclose as implemented, and retain the example usage showing await
client.chat.completions.create_stream(req) before entering the async context
manager to ensure correct async iteration behavior.
- Around line 82-120: The implementation of AnthropicSyncStream.__next__
correctly returns a dict (using event.json()) and the example usage using dict
access is appropriate; keep the SSE-to-JSON type injection logic (if event.event
and "type" not in data: data["type"] = event.event) as-is and do not change the
return type or the example usage referencing event.get("type") and
event.get("delta", {}); no code modifications required to iter_sse_sync,
__next__, or the class doc example.
In `@clients/python/smg_client/api/messages.py`:
- Around line 26-28: Ensure the create message call forces non-streaming by
assigning kwargs["stream"] = False (not using setdefault) before sending the
request; specifically, in the function that calls
self._transport.request("POST", "/v1/messages", json=kwargs) set
kwargs["stream"]=False so callers cannot pass stream=True, then parse the
response with Message.model_validate_json(resp.content).
In `@clients/rust/src/transport.rs`:
- Around line 142-154: The review notes that backoff_delay's jitter
implementation (function backoff_delay) is acceptable and approves the change,
but the comment is duplicated; no code changes needed—leave function
backoff_delay as-is and remove the duplicate review comment/annotation so only a
single approval comment remains.
- Around line 63-75: The doc comment for post_stream documents why streaming
requests aren't retried, but the PR/comment contains duplicate review markers
([approve_code_changes] and [duplicate_comment]) — remove the
duplicate/erroneous annotation from the submission (leave the clarified doc
comment above pub(crate) async fn post_stream intact) so only the intended
approval/comment remains and no duplicate markers are present.
- Around line 17-39: The new() constructor in transport.rs is mapping header
parse and client build failures to SmgError::Stream which is semantically wrong;
update the two map_err() calls in Client::builder().timeout(...) ->
HeaderValue::from_str(...) and builder.build() to return a
configuration/construction-specific error variant (e.g. SmgError::Config or
SmgError::TransportInit) instead of SmgError::Stream, and if that variant does
not exist add it to the SmgError enum with appropriate display/debug messages so
header-parse and client-build failures are clearly distinguished from streaming
errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (53)
.gitignoreCargo.tomlMakefileclients/openapi-gen/Cargo.tomlclients/openapi-gen/src/main.rsclients/python/pyproject.tomlclients/python/smg_client/__init__.pyclients/python/smg_client/_client.pyclients/python/smg_client/_config.pyclients/python/smg_client/_errors.pyclients/python/smg_client/_sse.pyclients/python/smg_client/_streaming.pyclients/python/smg_client/_transport.pyclients/python/smg_client/api/__init__.pyclients/python/smg_client/api/chat.pyclients/python/smg_client/api/completions.pyclients/python/smg_client/api/embeddings.pyclients/python/smg_client/api/messages.pyclients/python/smg_client/api/models.pyclients/python/smg_client/api/rerank.pyclients/python/smg_client/types/__init__.pyclients/python/tests/__init__.pyclients/python/tests/test_errors.pyclients/python/tests/test_sse.pyclients/python/tests/test_types.pyclients/rust/Cargo.tomlclients/rust/src/api/chat.rsclients/rust/src/api/completions.rsclients/rust/src/api/embeddings.rsclients/rust/src/api/messages.rsclients/rust/src/api/mod.rsclients/rust/src/api/models.rsclients/rust/src/api/rerank.rsclients/rust/src/client.rsclients/rust/src/config.rsclients/rust/src/error.rsclients/rust/src/lib.rsclients/rust/src/streaming/mod.rsclients/rust/src/streaming/sse.rsclients/rust/src/streaming/typed_stream.rsclients/rust/src/transport.rsclients/rust/tests/test_error.rsprotocols/Cargo.tomlprotocols/src/chat.rsprotocols/src/common.rsprotocols/src/completion.rsprotocols/src/embedding.rsprotocols/src/generate.rsprotocols/src/messages.rsprotocols/src/rerank.rsprotocols/src/responses.rsprotocols/src/sampling_params.rsprotocols/src/tokenize.rs
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 25, 2026 19:43
f32751e to
0b115dc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b115dcc14
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
clients/openapi-gen/src/main.rs (3)
350-350:serde_yamlis archived and unmaintained — still not migrated toserde_yaml_ng.This was flagged in a prior review.
serde_yamlwas archived by David Tolnay in March 2024. The recommended drop-in replacement isserde_yaml_ng:serde_yaml = { package = "serde_yaml_ng", version = "0.10" }This requires no source code changes since the import path stays
serde_yaml::*.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` at line 350, Replace the archived serde_yaml dependency with the maintained serde_yaml_ng package in the crate manifest so calls like serde_yaml::to_string(&doc) (the usage that assigns to variable yaml) continue to work without source changes; update Cargo.toml to depend on serde_yaml_ng (using the package override so the import path remains serde_yaml) and run cargo update to refresh the lockfile.
319-324:ListModelsResponseorigin is unclear due to glob imports.
ListModelsResponseon Line 320 is not covered by any of the explicitusestatements visible inmain(). It compiles only because one of the glob imports (use openai_protocol::*::*) happens to re-export it. This makes it fragile — a refactor of the protocol crate's module structure could break this silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` around lines 319 - 324, The reference to ListModelsResponse is relying on a glob re-export (openai_protocol::*::*) and is fragile; explicitly import or fully-qualify the type so the dependency is obvious and robust. Update main.rs to replace the implicit symbol usage in the collect_schema/schema_for! call by adding an explicit use for ListModelsResponse from its defining module in the protocol crate or by calling it with its full path (e.g., openai_protocol::<module>::ListModelsResponse) so collect_schema(&schema_for!(ListModelsResponse), ...) no longer depends on glob imports.
117-143:collect_schemastill silently overwrites duplicate schema names.This was flagged in a prior review and not yet addressed. If two types produce the same
title, the earlier entry is silently replaced — potentially dropping types from the generated spec with no indication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` around lines 117 - 143, collect_schema currently overwrites existing entries in the schemas BTreeMap when duplicate keys (from root.definitions names or the computed title) occur; change it to detect duplicates and return a descriptive error instead of silently replacing values. Specifically, before inserting into schemas in the loop over root.definitions (referencing the name variable) and before inserting the root entry (referencing the title variable), check schemas.contains_key(key) and bail/return Err(anyhow!("duplicate schema name: {}", key)) with context so callers of collect_schema receive a failure instead of silent loss; keep fixup_schema usage unchanged.clients/python/smg_client/_sse.py (1)
28-30:json()return type is still too narrow.
json.loads()can returnlist,str,int,float,bool, orNone— not justdict. This was flagged in a prior review and not yet addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_sse.py` around lines 28 - 30, The json() method currently types its return as dict but json.loads can return many types; update the type annotation for json() in class/method named json to reflect broader JSON return types (use typing.Any or a Union/TypedAlias like JSONType = Union[dict, list, str, int, float, bool, None]) and adjust imports to include typing.Any or the specific Union/TypedAlias so callers and type checkers accept all possible json.loads results.clients/python/smg_client/types/__init__.py (1)
46-50:⚠️ Potential issue | 🟠 MajorNarrow the import exception handling to avoid masking real failures.
Line 46 catches all
ImportError, which can hide unrelated import/dependency errors from insidesmg_client.types._generated. Only rewrite the missing-module case, and re-raise everything else.🛠️ Proposed fix
-except ImportError as _e: - raise ImportError( - "smg_client.types._generated not found. " - "Run 'make generate-clients' to generate the types module." - ) from _e +except ModuleNotFoundError as _e: + if _e.name != "smg_client.types._generated": + raise + raise ImportError( + "smg_client.types._generated not found. " + "Run 'make generate-clients' to generate the types module." + ) from _eUse this read-only check to confirm the current handler is broad and verify after the fix:
#!/bin/bash python - <<'PY' import ast from pathlib import Path path = Path("clients/python/smg_client/types/__init__.py") tree = ast.parse(path.read_text()) for node in ast.walk(tree): if isinstance(node, ast.Try): for h in node.handlers: t = h.type if isinstance(t, ast.Name): print(f"except {t.id} at line {h.lineno}") PYExpected: currently reports
except ImportError at line 46; after fix it should reportexcept ModuleNotFoundError ....🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/types/__init__.py` around lines 46 - 50, The current except ImportError handler in smg_client.types.__init__ is too broad and masks other import failures; change it to catch ModuleNotFoundError (the specific failure when a module is missing) and inside the handler check the exception's name attribute equals "smg_client.types._generated" before raising the custom ImportError with the existing message, otherwise re-raise the original exception so unrelated ImportError/ModuleNotFoundError cases inside smg_client.types._generated are not swallowed.clients/rust/src/streaming/typed_stream.rs (1)
32-33: UseINVARIANT:for this safe-code assumption comment.This is the same convention issue previously raised: the marker should be
INVARIANT:rather thanSAFETY:for non-unsafe reasoning.Based on learnings: In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code and reserve SAFETY: for unsafe blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/streaming/typed_stream.rs` around lines 32 - 33, Change the comment marker above the Unpin impl for TypedStream from "SAFETY:" to "INVARIANT:" because this documents a safe-code assumption (not an unsafe block); update the line preceding impl<T> Unpin for TypedStream<T> {} to use "INVARIANT: `TypedStream` is `Unpin` because `Pin<Box<...>>` is always `Unpin`." so the repository convention is followed.clients/rust/src/transport.rs (1)
26-33: Non-stream failures are still classified asSmgError::Stream.This concern was already raised earlier and is still present in constructor/retry paths.
Also applies to: 90-95, 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/transport.rs` around lines 26 - 33, The code currently wraps non-stream failures (e.g., errors from builder.build and header parsing) as SmgError::Stream; change these map_err closures to return a more appropriate SmgError variant (e.g., SmgError::Transport, SmgError::Http, or add a SmgError::ClientBuild variant) instead of SmgError::Stream so non-stream errors are classified correctly; update all occurrences around the HTTP client construction and retry/constructor paths (the map_err on builder.build, header parsing, and similar closures at the other noted sites) to construct and use the chosen enum variant and adjust the SmgError enum if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/python/smg_client/_errors.py`:
- Line 114: The conditional `if "type" in data and data.get("type") == "error":`
is redundant; replace it with `if data.get("type") == "error":` so the
missing-key case is handled by get() and behavior remains the same—locate the
check (the `if` using `data` and `"type"`) and simplify it accordingly.
In `@clients/python/smg_client/_sse.py`:
- Around line 57-58: The SSE parsing uses line[len("data:") :].lstrip(), which
removes all leading whitespace contrary to the SSE spec; change the logic in the
branches that check line.startswith("data:") (and the analogous branch at the
other occurrence around lines 91-92) to remove at most a single leading space
after the field name—i.e., take the substring after "data:" and if its first
character is a space drop exactly one space, otherwise keep it as-is, then
append that result to data_lines (same change for the other field handling).
In `@clients/python/smg_client/_streaming.py`:
- Around line 107-110: The SSE event field should always be authoritative:
replace the conditional checks that only set data["type"] when missing with
logic that overwrites data["type"] whenever event.event is present (i.e., use
event.event to set data["type"] unconditionally if event.event exists). Apply
this change to both places where data["type"] is set from event.event (the
blocks referencing event.event and data["type"] around the current checks at
~lines 109 and 136) so the SSE event field wins even when the JSON body contains
a differing "type".
In `@clients/python/smg_client/_transport.py`:
- Line 21: Replace the hardcoded User-Agent string in _transport.py with a
dynamic one sourced from the package version: import importlib.metadata.version
(or use a shared __version__ constant) and construct
"smg-client-python/{version}" when building the headers (the existing headers
dict or function that returns headers). Ensure you handle
LookupError/PackageNotFoundError by falling back to a default like
"smg-client-python/unknown" so header construction (where "User-Agent" is set)
never fails at runtime.
In `@clients/rust/src/transport.rs`:
- Around line 146-155: In backoff_delay, the code currently caps base_ms before
adding jitter which allows the final delay to exceed the intended max; update
the function (backoff_delay) to compute the jitter, add it to base_ms, then
clamp the resulting total to a max (e.g., 30_000 ms) so the returned Duration
never exceeds the bound; keep the jitter-generation approach (using
SystemTime::now().subsec_nanos()) but compute jitter relative to base or an
appropriate range and apply final min(total_ms, MAX_MS) before constructing
Duration.
---
Duplicate comments:
In `@clients/openapi-gen/src/main.rs`:
- Line 350: Replace the archived serde_yaml dependency with the maintained
serde_yaml_ng package in the crate manifest so calls like
serde_yaml::to_string(&doc) (the usage that assigns to variable yaml) continue
to work without source changes; update Cargo.toml to depend on serde_yaml_ng
(using the package override so the import path remains serde_yaml) and run cargo
update to refresh the lockfile.
- Around line 319-324: The reference to ListModelsResponse is relying on a glob
re-export (openai_protocol::*::*) and is fragile; explicitly import or
fully-qualify the type so the dependency is obvious and robust. Update main.rs
to replace the implicit symbol usage in the collect_schema/schema_for! call by
adding an explicit use for ListModelsResponse from its defining module in the
protocol crate or by calling it with its full path (e.g.,
openai_protocol::<module>::ListModelsResponse) so
collect_schema(&schema_for!(ListModelsResponse), ...) no longer depends on glob
imports.
- Around line 117-143: collect_schema currently overwrites existing entries in
the schemas BTreeMap when duplicate keys (from root.definitions names or the
computed title) occur; change it to detect duplicates and return a descriptive
error instead of silently replacing values. Specifically, before inserting into
schemas in the loop over root.definitions (referencing the name variable) and
before inserting the root entry (referencing the title variable), check
schemas.contains_key(key) and bail/return Err(anyhow!("duplicate schema name:
{}", key)) with context so callers of collect_schema receive a failure instead
of silent loss; keep fixup_schema usage unchanged.
In `@clients/python/smg_client/_sse.py`:
- Around line 28-30: The json() method currently types its return as dict but
json.loads can return many types; update the type annotation for json() in
class/method named json to reflect broader JSON return types (use typing.Any or
a Union/TypedAlias like JSONType = Union[dict, list, str, int, float, bool,
None]) and adjust imports to include typing.Any or the specific Union/TypedAlias
so callers and type checkers accept all possible json.loads results.
In `@clients/python/smg_client/types/__init__.py`:
- Around line 46-50: The current except ImportError handler in
smg_client.types.__init__ is too broad and masks other import failures; change
it to catch ModuleNotFoundError (the specific failure when a module is missing)
and inside the handler check the exception's name attribute equals
"smg_client.types._generated" before raising the custom ImportError with the
existing message, otherwise re-raise the original exception so unrelated
ImportError/ModuleNotFoundError cases inside smg_client.types._generated are not
swallowed.
In `@clients/rust/src/streaming/typed_stream.rs`:
- Around line 32-33: Change the comment marker above the Unpin impl for
TypedStream from "SAFETY:" to "INVARIANT:" because this documents a safe-code
assumption (not an unsafe block); update the line preceding impl<T> Unpin for
TypedStream<T> {} to use "INVARIANT: `TypedStream` is `Unpin` because
`Pin<Box<...>>` is always `Unpin`." so the repository convention is followed.
In `@clients/rust/src/transport.rs`:
- Around line 26-33: The code currently wraps non-stream failures (e.g., errors
from builder.build and header parsing) as SmgError::Stream; change these map_err
closures to return a more appropriate SmgError variant (e.g.,
SmgError::Transport, SmgError::Http, or add a SmgError::ClientBuild variant)
instead of SmgError::Stream so non-stream errors are classified correctly;
update all occurrences around the HTTP client construction and retry/constructor
paths (the map_err on builder.build, header parsing, and similar closures at the
other noted sites) to construct and use the chosen enum variant and adjust the
SmgError enum if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
.gitignoreMakefileclients/openapi-gen/src/main.rsclients/python/smg_client/__init__.pyclients/python/smg_client/_client.pyclients/python/smg_client/_errors.pyclients/python/smg_client/_sse.pyclients/python/smg_client/_streaming.pyclients/python/smg_client/_transport.pyclients/python/smg_client/api/chat.pyclients/python/smg_client/api/completions.pyclients/python/smg_client/api/messages.pyclients/python/smg_client/types/__init__.pyclients/python/tests/test_errors.pyclients/rust/Cargo.tomlclients/rust/src/api/chat.rsclients/rust/src/api/completions.rsclients/rust/src/api/messages.rsclients/rust/src/error.rsclients/rust/src/streaming/mod.rsclients/rust/src/streaming/sse.rsclients/rust/src/streaming/typed_stream.rsclients/rust/src/transport.rsclients/rust/tests/test_error.rs
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 25, 2026 21:15
0b115dc to
0064364
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0064364df6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
clients/openapi-gen/src/main.rs (2)
117-144:collect_schemasilently overwrites duplicate schema names.
schemas.insert()at lines 132 and 141 will silently overwrite existing entries if two types produce the same name. For a codegen tool, this can silently drop schemas. Consider logging a warning on collision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` around lines 117 - 144, collect_schema currently uses schemas.insert() in the loop over root.definitions and when adding the root (calls to schemas.insert at the top-level) which will silently overwrite an existing entry if two schemas produce the same title/name; change those insertions to first check for an existing key (e.g., using schemas.contains_key or schemas.entry) and if present, emit a warning (including the conflicting name and context — e.g., the definition key or "root" and the originating schema id/title) instead of overwriting; keep the existing schema on collision or otherwise explicitly decide merge/rename behavior so no schema is silently dropped.
213-324: Glob imports insidemain()reduce type traceability.
ListModelsResponseon line 320 has no obvious module origin, making it unclear which protocol module defines it. This was flagged in a prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/openapi-gen/src/main.rs` around lines 213 - 324, The code uses many local glob imports inside main(), which hides the origin of types like ListModelsResponse and reduces traceability; update the file to remove in-function glob imports and instead either add explicit top-level imports (e.g. use openai_protocol::models::ListModelsResponse) or fully qualify types when calling collect_schema/schema_for! (e.g. collect_schema(&schema_for!(openai_protocol::models::ListModelsResponse), ...)); apply this to other ambiguous types used in main() (referenced by collect_schema, schema_for!, and paths.insert calls) so each type's module is explicit and obvious.clients/python/smg_client/_transport.py (1)
18-26: Hardcoded version string in User-Agent will drift from package version.
"smg-client-python/0.1.0"is a magic string that will become stale. Consider reading fromimportlib.metadataor a shared__version__constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_transport.py` around lines 18 - 26, The User-Agent in _build_headers is hardcoded ("smg-client-python/0.1.0") and will become stale; change _build_headers to build the User-Agent dynamically by importing the package version (e.g., using importlib.metadata.version("smg-client-python") or a central __version__ constant) and format the header as f"smg-client-python/{version}" before returning headers; keep the existing header keys and Authorization logic intact and ensure fallback behavior if the version lookup fails.clients/python/smg_client/_sse.py (1)
57-58:lstrip()strips all leading whitespace, not just the single space per SSE spec.Per the SSE specification, only a single leading space character after the field name should be removed.
lstrip()will strip all leading whitespace, which could corrupt payloads with intentional leading spaces. For JSON payloads this is benign, but for spec correctness:Proposed fix
- data_lines.append(line[len("data:") :].lstrip()) + value = line[len("data:") :] + data_lines.append(value[1:] if value.startswith(" ") else value)Also applies to line 92.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_sse.py` around lines 57 - 58, The code handling SSE data fields uses lstrip() (seen in the branch checking line.startswith("data:") and the similar occurrence around line 92), which removes all leading whitespace; change this to only drop a single leading space per the SSE spec by slicing off the first character only if it is exactly a space (e.g., take line[len("data:"):] and if that string startswith(" ") then use [1:] else use the string as-is), and apply the same change to the other occurrence so only one leading space is removed rather than all whitespace.clients/rust/src/transport.rs (1)
146-156:⚠️ Potential issue | 🟠 MajorClamp after adding jitter to enforce the documented max backoff.
At Line 155,
capped_ms + jitter_mscan exceed 30_000ms, so the retry delay can violate the stated cap.🛠️ Proposed fix
fn backoff_delay(attempt: u32) -> std::time::Duration { let base_ms = 500u64 * 2u64.saturating_pow(attempt); let capped_ms = base_ms.min(30_000); // Simple jitter using system time to avoid adding a `rand` dependency. let nanos = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() .subsec_nanos() as u64; let jitter_ms = nanos % (capped_ms / 2 + 1); - std::time::Duration::from_millis(capped_ms + jitter_ms) + let delay_ms = capped_ms.saturating_add(jitter_ms).min(30_000); + std::time::Duration::from_millis(delay_ms) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/rust/src/transport.rs` around lines 146 - 156, In backoff_delay, the computed delay uses capped_ms + jitter_ms which can exceed the documented 30_000ms cap; update the function (backoff_delay) to add jitter first and then clamp the final value to 30_000ms (use saturating arithmetic to avoid overflow), and ensure the jitter calculation cannot divide/modulo by zero by bounding the jitter modulus with a max(1, capped_ms/2 + 1) before taking nanos % modulus so the final Duration is min(30_000, capped_ms.saturating_add(jitter_ms)).clients/python/smg_client/_streaming.py (1)
104-111: SSEevent:field should be authoritative — currently only injected when"type"is missing.The comment on Line 107-108 says the SSE event field is authoritative, but the code at Line 109 only sets
data["type"]when the key is absent. If both exist and diverge, the JSON value wins silently. Same issue exists inAnthropicAsyncStreamat Line 136.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/_streaming.py` around lines 104 - 111, In __next__ of the synchronous stream (clients/python/smg_client/_streaming.py) and the corresponding method in AnthropicAsyncStream, always treat the SSE event field as authoritative: if event.event is truthy, set data["type"] = event.event unconditionally (do not only set when "type" is missing) so the SSE event value overrides any conflicting "type" inside the JSON payload; update both __next__ and the async counterpart to assign the SSE event into the parsed dict whenever present.clients/python/smg_client/types/__init__.py (1)
46-50:⚠️ Potential issue | 🟡 Minor
ModuleNotFoundErrorcatch is still too broad — check_e.nameto avoid masking transitive import failures.If
_generated.pyexists but imports a missing transitive dependency (e.g.,pydanticnot installed), the current catch will swallow that error and misleadingly tell the user to runmake generate-clients. Narrowing with an_e.namecheck prevents this.Proposed fix
-except ModuleNotFoundError as _e: - raise ModuleNotFoundError( +except ModuleNotFoundError as _e: + if _e.name != "smg_client.types._generated": + raise + raise ModuleNotFoundError( "smg_client.types._generated not found. " "Run 'make generate-clients' to generate the types module." ) from _e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/types/__init__.py` around lines 46 - 50, The except block catching ModuleNotFoundError around importing smg_client.types._generated is too broad; modify the handler in __init__.py to inspect the caught exception's name (variable _e) and only raise the custom ModuleNotFoundError with the "Run 'make generate-clients'" message when _e.name equals "smg_client.types._generated"; otherwise re-raise the original exception so transitive import errors (e.g., missing pydantic) are not masked. Ensure you reference the import of smg_client.types._generated and the caught exception variable _e in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/openapi-gen/src/main.rs`:
- Around line 335-339: The OpenAPI Info.version currently uses
env!("CARGO_PKG_VERSION") which ties the API spec to the codegen crate; change
Info { version: env!("CARGO_PKG_VERSION").to_string(), ... } to pull the API
version from a dedicated source (e.g., a new constant like OPENAPI_VERSION, a
CLI argument parsed in main and passed into where Info is constructed, or read
from the gateway's runtime/version provider) and wire that value into the Info
struct construction so the API version is independent of the openapi-gen crate
version.
In `@clients/python/smg_client/_errors.py`:
- Around line 113-129: The parsed JSON `data` can be a non-dict (e.g.,
null/number/boolean), so guard the dict-style key checks by verifying the type
before using "in" — wrap the branches that check `"type" in data`, `"error" in
data` etc. with an `if isinstance(data, dict):` check inside the function that
processes the response (the block around the existing checks in
clients/python/smg_client/_errors.py), and remove KeyError from the exception
handler so it only catches json.JSONDecodeError; this prevents TypeError from
non-dict JSON from escaping and lets the function fall back to the generic error
path as intended.
In `@clients/python/smg_client/_streaming.py`:
- Around line 104-111: Wrap the direct calls to event.json() in the __next__
method (and the similar call around lines 133-138) with a try/except that
catches json.JSONDecodeError and raises a clearer exception (e.g., ValueError or
a custom StreamingParseError) that includes the SSE event type (event.event) and
the raw event data (event.data) in the message so callers get a user-friendly
explanation of malformed/non-JSON SSE payloads; update both places where
event.json() is used (the __next__ implementation and the other occurrence) to
follow this pattern.
In `@clients/python/smg_client/_transport.py`:
- Around line 68-80: The sync streaming error path is accessing resp.text on a
streaming response (created via self._client.send(req, stream=True)), which can
raise ResponseNotRead; fix by reading the body before using text: call
resp.read() (or resp.content) inside the try block, decode to text as needed,
then close the response in finally; keep the existing retry logic
(_should_retry, _retry_delay) and error raising (raise_for_status) but pass the
read body to raise_for_status instead of accessing resp.text. Ensure you modify
the block that uses self._client.build_request and self._client.send to read
resp.read() before inspecting/raising on resp.status_code.
In `@clients/python/tests/test_errors.py`:
- Around line 75-78: Add a regression test in
clients/python/tests/test_errors.py that calls raise_for_status with JSON
scalar/null bodies to ensure the fallback preserves the raw body; e.g., use
pytest.raises(BadRequestError) for raise_for_status(400, "null") and
raise_for_status(400, "123") and assert exc_info.value.message equals "null" and
"123" respectively so raise_for_status and BadRequestError keep the raw scalar
string.
In `@clients/rust/Cargo.toml`:
- Around line 16-19: The Cargo features rustls and native-tls are not mutually
exclusive; add a compile-time guard so both can't be enabled together—for
example, add a build script (build.rs) that checks the env vars
CARGO_FEATURE_RUSTLS and CARGO_FEATURE_NATIVE_TLS and fails the build if both
are set, or add a top-level cfg check using #[cfg(all(feature = "rustls",
feature = "native-tls"))] compile_error!("enable only one of the features:
rustls or native-tls");; target the [features] section and the symbols "rustls"
and "native-tls" when implementing the guard so the build will error on
conflicting feature combinations.
In `@clients/rust/src/api/messages.rs`:
- Around line 58-70: inject_event_type currently only inserts "type" into the
event JSON when the key is missing, but the SSE `event:` field is supposed to be
authoritative; update inject_event_type to unconditionally set/overwrite the
"type" entry when event.event is Some — parse event.data into a
serde_json::Value, ensure it's an object (as_object_mut), and call
obj.insert("type".to_string(), serde_json::json!(event_type)) without checking
contains_key, then set event.data = value.to_string(); apply the same
unconditional-overwrite fix in the Python stream handlers (AnthropicSyncStream
and AnthropicAsyncStream in _streaming.py) so the SSE event field always
overrides any existing "type" in the JSON body.
---
Duplicate comments:
In `@clients/openapi-gen/src/main.rs`:
- Around line 117-144: collect_schema currently uses schemas.insert() in the
loop over root.definitions and when adding the root (calls to schemas.insert at
the top-level) which will silently overwrite an existing entry if two schemas
produce the same title/name; change those insertions to first check for an
existing key (e.g., using schemas.contains_key or schemas.entry) and if present,
emit a warning (including the conflicting name and context — e.g., the
definition key or "root" and the originating schema id/title) instead of
overwriting; keep the existing schema on collision or otherwise explicitly
decide merge/rename behavior so no schema is silently dropped.
- Around line 213-324: The code uses many local glob imports inside main(),
which hides the origin of types like ListModelsResponse and reduces
traceability; update the file to remove in-function glob imports and instead
either add explicit top-level imports (e.g. use
openai_protocol::models::ListModelsResponse) or fully qualify types when calling
collect_schema/schema_for! (e.g.
collect_schema(&schema_for!(openai_protocol::models::ListModelsResponse), ...));
apply this to other ambiguous types used in main() (referenced by
collect_schema, schema_for!, and paths.insert calls) so each type's module is
explicit and obvious.
In `@clients/python/smg_client/_sse.py`:
- Around line 57-58: The code handling SSE data fields uses lstrip() (seen in
the branch checking line.startswith("data:") and the similar occurrence around
line 92), which removes all leading whitespace; change this to only drop a
single leading space per the SSE spec by slicing off the first character only if
it is exactly a space (e.g., take line[len("data:"):] and if that string
startswith(" ") then use [1:] else use the string as-is), and apply the same
change to the other occurrence so only one leading space is removed rather than
all whitespace.
In `@clients/python/smg_client/_streaming.py`:
- Around line 104-111: In __next__ of the synchronous stream
(clients/python/smg_client/_streaming.py) and the corresponding method in
AnthropicAsyncStream, always treat the SSE event field as authoritative: if
event.event is truthy, set data["type"] = event.event unconditionally (do not
only set when "type" is missing) so the SSE event value overrides any
conflicting "type" inside the JSON payload; update both __next__ and the async
counterpart to assign the SSE event into the parsed dict whenever present.
In `@clients/python/smg_client/_transport.py`:
- Around line 18-26: The User-Agent in _build_headers is hardcoded
("smg-client-python/0.1.0") and will become stale; change _build_headers to
build the User-Agent dynamically by importing the package version (e.g., using
importlib.metadata.version("smg-client-python") or a central __version__
constant) and format the header as f"smg-client-python/{version}" before
returning headers; keep the existing header keys and Authorization logic intact
and ensure fallback behavior if the version lookup fails.
In `@clients/python/smg_client/types/__init__.py`:
- Around line 46-50: The except block catching ModuleNotFoundError around
importing smg_client.types._generated is too broad; modify the handler in
__init__.py to inspect the caught exception's name (variable _e) and only raise
the custom ModuleNotFoundError with the "Run 'make generate-clients'" message
when _e.name equals "smg_client.types._generated"; otherwise re-raise the
original exception so transitive import errors (e.g., missing pydantic) are not
masked. Ensure you reference the import of smg_client.types._generated and the
caught exception variable _e in your change.
In `@clients/rust/src/transport.rs`:
- Around line 146-156: In backoff_delay, the computed delay uses capped_ms +
jitter_ms which can exceed the documented 30_000ms cap; update the function
(backoff_delay) to add jitter first and then clamp the final value to 30_000ms
(use saturating arithmetic to avoid overflow), and ensure the jitter calculation
cannot divide/modulo by zero by bounding the jitter modulus with a max(1,
capped_ms/2 + 1) before taking nanos % modulus so the final Duration is
min(30_000, capped_ms.saturating_add(jitter_ms)).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
.gitignoreMakefileclients/openapi-gen/Cargo.tomlclients/openapi-gen/src/main.rsclients/python/smg_client/__init__.pyclients/python/smg_client/_client.pyclients/python/smg_client/_errors.pyclients/python/smg_client/_sse.pyclients/python/smg_client/_streaming.pyclients/python/smg_client/_transport.pyclients/python/smg_client/api/chat.pyclients/python/smg_client/api/completions.pyclients/python/smg_client/api/messages.pyclients/python/smg_client/types/__init__.pyclients/python/tests/test_errors.pyclients/rust/Cargo.tomlclients/rust/src/api/chat.rsclients/rust/src/api/completions.rsclients/rust/src/api/messages.rsclients/rust/src/error.rsclients/rust/src/streaming/mod.rsclients/rust/src/streaming/sse.rsclients/rust/src/streaming/typed_stream.rsclients/rust/src/transport.rsclients/rust/tests/test_error.rs
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 25, 2026 23:14
0064364 to
7df6a69
Compare
slin1237
added a commit
that referenced
this pull request
Feb 25, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 25, 2026 23:17
7df6a69 to
d2dc505
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2dc50520b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
clients/python/smg_client/types/__init__.py (1)
46-50:⚠️ Potential issue | 🟠 MajorException handling still masks nested import failures inside
_generated.This
except ModuleNotFoundErrorblock catches both missing module and missing transitive imports. It should only rewrite the error when_e.name == "smg_client.types._generated".Proposed fix
-except ModuleNotFoundError as _e: - raise ModuleNotFoundError( - "smg_client.types._generated not found. " - "Run 'make generate-clients' to generate the types module." - ) from _e +except ModuleNotFoundError as _e: + if _e.name != "smg_client.types._generated": + raise + raise ModuleNotFoundError( + "smg_client.types._generated not found. " + "Run 'make generate-clients' to generate the types module." + ) from _e#!/bin/bash set -euo pipefail FILE="clients/python/smg_client/types/__init__.py" echo "=== current exception handler ===" sed -n '40,55p' "$FILE" echo echo "=== check for _e.name guard ===" rg -n '_e\.name\s*!=' "$FILE" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/python/smg_client/types/__init__.py` around lines 46 - 50, The except ModuleNotFoundError handler in smg_client.types.__init__ is too broad and masks import errors from inside _generated; update the handler that currently reads "except ModuleNotFoundError as _e:" to check if getattr(_e, "name", None) == "smg_client.types._generated" (or _e.name == "smg_client.types._generated") and only rewrite/raise the friendly message in that case; otherwise re-raise the original exception (raise) so nested import failures inside _generated are not masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/python/smg_client/_errors.py`:
- Around line 110-111: Move the "import json" out of the runtime try block and
place it at module level in clients/python/smg_client/_errors.py so the stdlib
import is executed once at import time; remove the in-function or in-block
import and the misleading implication that json might raise ImportError, keeping
the rest of the try/except logic unchanged and referencing the existing "import
json" occurrence to locate the change.
In `@clients/python/smg_client/_streaming.py`:
- Around line 129-131: Add a short usage example to the AnthropicAsyncStream
class docstring mirroring the sync example from AnthropicSyncStream: show how to
instantiate the stream, await the async generator, and iterate with "async for
event in AnthropicAsyncStream(...)" (or await .stream()) to process events and
handle completion/errors; update the AnthropicAsyncStream docstring to include a
minimal async snippet demonstrating "async with" or "async for" usage so callers
can discover the correct async pattern.
- Around line 36-38: The __next__ (and __anext__) methods call
self._model_cls.model_validate_json(event.data) and allow
pydantic.ValidationError or AttributeError to bubble up; wrap the model
validation in a try/except that catches pydantic.ValidationError and
AttributeError and re-raises a SmgError with a descriptive message that includes
the offending event payload (use event.json() or event.data) and the original
exception as context, mirroring how AnthropicSyncStream wraps errors; apply the
same change to AsyncStream.__anext__ and reference the symbols __next__,
__anext__, _model_cls, model_validate_json, and SmgError so the fix is
implemented consistently.
- Around line 69-71: Replace direct calls to the iterator's private __anext__()
with the anext() builtin: in the async __anext__ method where you currently do
"event = await self._iterator.__anext__()", change it to use "event = await
anext(self._iterator)"; make the same change in the analogous method on
AnthropicAsyncStream so both async iterators use the idiomatic anext() dispatch.
Ensure you import/built-in use anext (Python ≥3.10) and leave the subsequent
model validation call (model_validate_json) unchanged.
- Line 15: The TypeVar T is currently unbound which allows callers to pass types
that don't implement model_validate_json; update T to be constrained by a
Protocol or a Pydantic base so static checkers can verify model_cls has
model_validate_json. Define a small Protocol (e.g.,
supports_model_validate_json) declaring `@classmethod` model_validate_json and use
TypeVar("T", bound=supports_model_validate_json) (or set bound=BaseModel if you
depend on Pydantic) and update the signature that accepts model_cls and any
usages in _streaming.py to use this constrained T so mypy/pyright will catch
incorrect types at call sites.
In `@clients/python/smg_client/_transport.py`:
- Around line 33-42: The _retry_delay function currently returns
float(retry_after) unvalidated; update _retry_delay to parse Retry-After safely
by converting to float/int then sanitizing: reject negative values (treat as 0),
clamp the value to a sensible max (e.g., 30.0 seconds), and only return the
sanitized value when it's within bounds; otherwise fall back to the
exponential-backoff calculation (min(2**attempt * 0.5, 30.0)). Ensure the
validation covers non-numeric strings (catch ValueError) and extremely large
values by applying the same cap before returning.
In `@clients/rust/src/error.rs`:
- Around line 11-16: The BadRequest variant's display attribute hardcodes "400"
which is misleading for other 4xx statuses; update the thiserror attribute on
the BadRequest enum variant (BadRequest { message: String, status: u16, body:
Option<ErrorResponse> }) to interpolate the actual status (e.g. #[error("bad
request ({status}): {message}")]) so the displayed message uses the status field
instead of the literal 400.
In `@clients/rust/src/streaming/sse.rs`:
- Around line 43-46: Replace the per-chunk lossy UTF-8 decoding: stop using
buffer as a String and calling
buffer.push_str(&String::from_utf8_lossy(&chunk)); instead make buffer a byte
buffer (Vec<u8> or BytesMut), append raw chunk bytes, scan the byte buffer for
complete line boundaries (e.g., b'\n' or SSE double-newline sequences) and only
decode each complete line with String::from_utf8 (or from_utf8_lossy if you
must) before parsing; leave any trailing partial UTF-8 sequence bytes in buffer
for the next chunk. Ensure you update the code paths that reference buffer to
use byte-buffer append and line-splitting logic and only convert bytes->String
when a full line is extracted.
In `@clients/rust/src/transport.rs`:
- Around line 147-148: The backoff calculation in transport.rs currently
multiplies 500u64 by 2u64.saturating_pow(attempt) which can overflow before the
min cap is applied; update the computation of base_ms to use a saturating
multiplication (e.g., compute the power into an intermediate and call
saturating_mul) so that 500 is multiplied safely by the power without wrapping,
then apply the existing min(30_000) to produce capped_ms; refer to the variables
base_ms, capped_ms and the attempt power calculation when making the change.
- Around line 104-110: The code currently parses the "retry-after" header into
an f64 and directly calls Duration::from_secs_f64, which will panic on negative,
non-finite or too-large values; change the chain that produces delay so you
validate the parsed f64 (e.g., check value.is_finite() && value >= 0.0 && value
<= some sane_max) before calling Duration::from_secs_f64, and if the value fails
validation fall back to backoff_delay(attempt); keep the existing
.headers().get("retry-after") lookup and the final
unwrap_or_else(backoff_delay(attempt)) behavior but insert the guard between
.parse::<f64>().ok() and .map(Duration::from_secs_f64).
---
Duplicate comments:
In `@clients/python/smg_client/types/__init__.py`:
- Around line 46-50: The except ModuleNotFoundError handler in
smg_client.types.__init__ is too broad and masks import errors from inside
_generated; update the handler that currently reads "except ModuleNotFoundError
as _e:" to check if getattr(_e, "name", None) == "smg_client.types._generated"
(or _e.name == "smg_client.types._generated") and only rewrite/raise the
friendly message in that case; otherwise re-raise the original exception (raise)
so nested import failures inside _generated are not masked.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
.gitignoreMakefileclients/openapi-gen/Cargo.tomlclients/openapi-gen/src/main.rsclients/python/smg_client/__init__.pyclients/python/smg_client/_client.pyclients/python/smg_client/_errors.pyclients/python/smg_client/_sse.pyclients/python/smg_client/_streaming.pyclients/python/smg_client/_transport.pyclients/python/smg_client/api/chat.pyclients/python/smg_client/api/completions.pyclients/python/smg_client/api/messages.pyclients/python/smg_client/types/__init__.pyclients/python/tests/test_errors.pyclients/rust/Cargo.tomlclients/rust/src/api/chat.rsclients/rust/src/api/completions.rsclients/rust/src/api/messages.rsclients/rust/src/error.rsclients/rust/src/streaming/mod.rsclients/rust/src/streaming/sse.rsclients/rust/src/streaming/typed_stream.rsclients/rust/src/transport.rsclients/rust/tests/test_error.rs
slin1237
added a commit
that referenced
this pull request
Feb 26, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 26, 2026 02:42
d2dc505 to
4229a90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4229a90160
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
slin1237
added a commit
that referenced
this pull request
Feb 26, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 26, 2026 03:08
4229a90 to
6f9d8e0
Compare
slin1237
added a commit
that referenced
this pull request
Feb 26, 2026
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 26, 2026 04:54
6f9d8e0 to
dffccf0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dffccf09f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
|
|
||
| // ---- Models ---- | ||
| let resp_name = collect_schema(&schema_for!(ListModelsResponse), &mut schemas)?; |
There was a problem hiding this comment.
This binds /v1/models to ListModelsResponse, which comes from protocols/src/messages.rs and requires has_more, but the OpenAI router currently returns only {"object":"list","data":[...]} in get_models (model_gateway/src/routers/openai/router.rs, lines 587-590). As a result, OpenAPI-generated clients can reject valid /v1/models responses from the default OpenAI route, causing deserialization failures for a common endpoint.
Useful? React with 👍 / 👎.
Contributor
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
…ipeline
Add multi-language HTTP client SDKs for SMG with an OpenAPI-based type
generation pipeline. Python types are auto-generated from Rust protocol
types via an intermediate OpenAPI 3.1 spec, eliminating hand-written
type duplication.
What changed:
- clients/openapi-gen: new Rust binary that collects schemars JSON
schemas from openai-protocol types and emits an OpenAPI 3.1 YAML spec.
Includes fixup_schema() to rewrite #/definitions/ refs to
#/components/schemas/ and replace boolean `true` with `{}` for
datamodel-codegen compatibility. Uses anyhow for error handling to
satisfy workspace clippy lints.
- clients/python: new smg-client Python package with sync/async clients,
SSE streaming, Anthropic Messages API support, and auto-generated
Pydantic v2 types. API modules (chat, completions, embeddings,
messages, models, rerank) are hand-written for HTTP routing and
streaming; types/__init__.py re-exports directly from _generated.py.
17 tests covering types, SSE parsing, and error handling.
- clients/rust: new smg-client Rust crate with async reqwest-based
transport, retry logic, typed SSE streaming for OpenAI/Anthropic/
Responses protocols, and comprehensive error mapping. Re-exports types
from openai-protocol (no duplication).
- protocols/Cargo.toml: make schemars a required (non-optional)
dependency, remove jsonschema feature flag entirely.
- protocols/src/*.rs: remove all 215 occurrences of
cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema)) and
merge schemars::JsonSchema directly into #[derive()] lines. Remove
cfg_attr wrappers from schemars(rename) attributes and cfg guard from
FunctionCall manual impl in common.rs.
- Cargo.toml: add schemars 0.8 to workspace deps, add clients/rust and
clients/openapi-gen to workspace members.
- Makefile: add generate-openapi, generate-python-types, and
generate-clients targets for the codegen pipeline.
- .gitignore: add uv.lock entry.
- clients/python/pyproject.toml: align with bindings/python standard
(authors, license table format, readme path, classifiers, structured
pytest config).
Why:
- Eliminate manual type synchronization between Rust protocol types and
client SDKs. Previously, any protocol change required updating types
in every client language by hand.
- Remove cfg_attr boilerplate (266 lines) that was error-prone — 7 files
with 109 types were already missed. Since schemars::JsonSchema has no
runtime cost, the feature flag added complexity with no benefit.
- Provide ready-to-use client SDKs for Python and Rust consumers of SMG.
How:
- openapi-gen uses schemars::schema_for!() to collect JSON schemas for
all request/response types, then assembles them into an OpenAPI 3.1
document with paths for each endpoint.
- Python types are generated via datamodel-code-generator (invoked as
uvx) from the OpenAPI YAML, producing Pydantic v2 models with
annotated fields and union operators.
- Rust client re-exports openai-protocol types directly, avoiding
duplication entirely.
- `make generate-clients` runs the full pipeline: cargo run openapi-gen
then datamodel-codegen.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…SDKs
Critical fixes:
- _transport.py: replace stream().__enter__() with build_request()+send(stream=True)
to fix context manager leak in both sync and async transports
- _transport.py: add try/finally for resource cleanup on error body read
- _transport.py: use decode("utf-8", errors="replace") for non-UTF-8 safety
Major fixes:
- _sse.py: buffer data: lines until event boundary (empty line) before
yielding, fixing multi-line SSE payload fragmentation
- sse.rs: [DONE] sentinel now sets a done flag that terminates the stream
instead of just skipping the event (prevents hanging)
- sse.rs: flush() now processes remaining partial line in the buffer
- error.rs: unmapped 4xx statuses (e.g. 422) now map to BadRequest, not Server
- main.rs: fixup_schema is now key-aware, only replacing true->{} inside
anyOf/oneOf/allOf arrays instead of all arrays
- main.rs: use env!("CARGO_PKG_VERSION") instead of hardcoded version
- streaming/mod.rs: remove __test_sse_stream public export, move SSE tests
to unit tests inside sse.rs
- chat.rs/completions.rs/messages.rs: guard stream flag in create vs
create_stream to give clear errors on mismatch
- Makefile: pin datamodel-code-generator==0.54.0
Minor fixes:
- _errors.py: reorder Anthropic error check before OpenAI check (was shadowed)
- _errors.py: add __all__ and document intentional builtin shadowing
- _client.py/_streaming.py: fix async examples to await create_stream()
- _streaming.py: fix docstring to use dict access instead of attribute access
- messages.py/chat.py/completions.py: force stream=False in create()
- types/__init__.py: wrap import in try/except with helpful error message
- __init__.py: add comment about intentional builtin shadowing
- test_errors.py: add tests for 403 and 503 status codes
- test_error.rs: add test for unmapped 4xx (422)
- typed_stream.rs: remove unnecessary pin-project-lite, implement Unpin
- transport.rs: add jitter to backoff delay, document streaming retry rationale
- .gitignore: add generated files (smg-openapi.yaml, _generated.py)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
slin/client
branch
from
February 26, 2026 08:25
dffccf0 to
452c5de
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 452c5dede2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+149
to
+153
| let line = buffer.trim_end_matches(['\r', '\n']); | ||
| if let Some(value) = line | ||
| .strip_prefix("data: ") | ||
| .or_else(|| line.strip_prefix("data:")) | ||
| { |
There was a problem hiding this comment.
If the server closes a stream without sending the final blank-line delimiter, flush() treats the entire remaining buffer as one line and only runs a single data:/event: prefix check. A tail like event: message_stop\ndata: {...} therefore matches neither prefix, leaves current_data empty, and silently drops the last event. This can truncate the final chunk for Anthropic/Responses streams when connections end immediately after the last payload.
Useful? React with 👍 / 👎.
This was referenced Feb 26, 2026
5 tasks
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
1 participant
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
Add multi-language HTTP client SDKs for SMG with an OpenAPI-based type generation pipeline. Python types are auto-generated from Rust protocol types via an intermediate OpenAPI 3.1 spec, eliminating hand-written type duplication. Refs: #489
What changed
New crates & packages
clients/openapi-gen— Rust binary that collectsschemarsJSON schemas fromopenai-protocoltypes and emits an OpenAPI 3.1 YAML spec. Includesfixup_schema()to rewrite#/definitions/refs to#/components/schemas/and replace booleantruewith{}fordatamodel-codegencompatibility.clients/python(smg-client) — Python SDK with sync/async clients, SSE streaming, Anthropic Messages API support, and auto-generated Pydantic v2 types. 17 tests covering types, SSE parsing, and error handling.clients/rust(smg-client) — Rust SDK with async reqwest-based transport, retry logic with exponential backoff, typed SSE streaming for OpenAI/Anthropic/Responses protocols, and comprehensive error mapping. Re-exports types directly fromopenai-protocol(zero duplication).Protocol changes
protocols/Cargo.toml:schemarsis now a required (non-optional) dependency. Removed thejsonschemafeature flag entirely.protocols/src/*.rs: Removed all 215 occurrences of#[cfg_attr(feature = "jsonschema", derive(schemars::JsonSchema))]and mergedschemars::JsonSchemadirectly into#[derive()]lines. Removedcfg_attrwrappers fromschemars(rename = ...)attributes and the#[cfg(feature = "jsonschema")]guard fromFunctionCallmanual impl incommon.rs.Build & infra
Cargo.toml: Addedschemars = "0.8"to workspace deps, addedclients/rustandclients/openapi-gento workspace members.Makefile: Addedgenerate-openapi,generate-python-types, andgenerate-clientstargets..gitignore: Addeduv.lock.Why
make generate-clientsregenerates everything from the single Rust source of truth.cfg_attrwere error-prone — 7 files with 109 types were already missed. Sinceschemars::JsonSchemahas no runtime cost, the feature flag added complexity with no benefit.How
openapi-genusesschemars::schema_for!()to collect JSON schemas for all request/response types, assembles them into an OpenAPI 3.1 document with paths for each endpoint.datamodel-code-generatorfrom the OpenAPI YAML, producing Pydantic v2 models.openai-protocoltypes directly.make generate-clientsruns the full pipeline.Test plan
cargo clippy -p openapi-gen --all-targets -- -D warnings— passes cleancargo clippy --all-targets --all-features -- -D warnings— passes (only pre-existing workspace warnings)make generate-clients— generates OpenAPI spec and Python types end-to-endcd clients/python && uv run pytest— all 17 tests passcargo test -p smg-client— Rust client tests passSummary by CodeRabbit
New Features
Improvements
Tests
Chores