Skip to content

fix: correctness, resource-handling, and documentation fixes across the SDK#5

Merged
OmarAlJarrah merged 16 commits into
mainfrom
fix/sdk-correctness-hardening
Jun 10, 2026
Merged

fix: correctness, resource-handling, and documentation fixes across the SDK#5
OmarAlJarrah merged 16 commits into
mainfrom
fix/sdk-correctness-hardening

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

A pass over the SDK fixing correctness and resource-handling bugs that the type
checker and existing tests did not catch, together with a documentation refresh
so the prose matches the shipped API. No new runtime dependencies. The public
surface is unchanged apart from three deliberate narrowings called out below.

Each fix lands with a regression test, and the commits are grouped by subsystem
so they can be reviewed independently.

Notable fixes

HTTP layer

  • Importing the request package before the response package raised
    ImportError from a module cycle; an annotation-only import now sits under
    TYPE_CHECKING.
  • The SSE parser split a single event in two when a CRLF terminator landed on
    a read-chunk boundary; a trailing CR is now held until the next byte
    disambiguates it.
  • Control-character validation ran only in the Headers constructor, so a
    CR/LF value could be injected through Request.with_header; every mutator
    now validates. A directly-constructed HttpHeaderName is lower-cased so it
    matches the case-insensitive store.
  • Async request bodies now validate chunk_size, FileRequestBody
    content-length is clamped to the bytes actually available, form encoding
    honours the requested charset, and the async stream body's close is shielded
    against a second cancellation.
  • Non-UTC aware datetimes are normalised before formatting an HTTP date,
    ETag.parse rejects an embedded quote, suffix byte-ranges share the public
    HttpRange type so they compose in format_many, and the paginator wrappers
    take explicit typed parameters.

Pipeline

  • Status-driven retries and non-reissuable redirects closed neither the in-hand
    response nor its connection; both now close before continuing. The
    synchronous retry's status-path sleep moved outside the try so a deadline
    timeout propagates instead of being reclassified and retried (matching the
    async policy), and the injected clock now reaches the async policy's inner
    configuration so an HTTP-date Retry-After is deterministic.
  • The per-call ContextStore entry is evicted once run completes, so the
    store no longer grows without bound across calls with distinct trace ids.
  • StagedPipelineBuilder.from_pipeline raises a clear error on a bare-callable
    step it cannot rehydrate instead of an opaque AttributeError.

Auth

  • Digest declines (rather than raising) on credentials it cannot encode under
    the negotiated charset and on a session-variant algorithm offered without
    qop; it no longer emits cnonce/nc on a qop-less response, and
    preferred_algorithms now acts as an allow-list.
  • AsyncBearerTokenPolicy gained the challenge-handler and 401/407 handling its
    synchronous counterpart already had.

Webhooks — a non-ASCII signature candidate is skipped instead of raising out
of verify, an empty decoded secret is rejected at construction, and the
timestamp must be a plain ASCII integer.

Serialization — mapping keys are encoded symmetrically with the decode side
so a dict keyed by an enum/date/UUID survives a round trip, a bare Tristate
annotation keeps its tri-state meaning, and a caller-supplied json default
chains behind the built-in datetime/bytes encoders instead of replacing them.

Instrumentation / config / errors — the URL redactor replaces the whole
parameter (key included) so a bare token carried as a query key is no longer
logged; non-finite durations are rejected; NO_PROXY uses conventional suffix
matching; SdkError keeps a coherent traceback when given an explicit cause;
and a loggable response body re-raises a base exception after recording it
rather than replaying a partial body as complete.

Transports — connect- and read-phase timeouts are classified consistently
across the adapters (so the retry policy never auto-retries a non-idempotent
request whose response may be in flight), the asyncio transport maps read-phase
errors into the SDK hierarchy and stops overwriting a caller's Host header,
and a status code outside the known set releases its connection before raising.
The aiohttp client refuses use after close instead of opening a fresh session.

Documentation

The README, CHANGELOG, project guide, and docs pages now reflect the real
default pipeline order and the subsystems that had shipped without mention
(webhook verification, the pagination package, the idempotency and
client-identity policies, correlation, and the reconnecting SSE client). A
broken import in the pipelines guide, a dangling reference in the architecture
guide, an inverted claim about retry body buffering, a reference to a
non-existent error type, and several stale value-object/auth descriptions are
corrected.

Conventions

Docstrings use the project's plain-backtick convention throughout, and a new
test scans every package source and test file so Sphinx cross-reference roles
cannot reappear (neither ruff nor mypy inspects docstring prose).

Public surface changes (intentional)

  • Removed RetryConfig — it was exported but unwired; RetryPolicy is the
    real configuration surface.
  • HttpRange.suffix now returns HttpRange (previously a private type) so a
    suffix range composes with format_many.
  • CallContext is now an abc.ABC (it declares no abstract methods, so
    existing subclasses are unaffected).

The static API-surface baseline is regenerated to match.

Testing

  • uv run ruff check and uv run ruff format --check: clean
  • uv run mypy --strict: clean (227 source files)
  • uv run pytest: 1141 passing (+128), with a regression test for each fix

OmarAlJarrah and others added 16 commits June 10, 2026 09:34
Resolve a module import cycle that made importing the request package before
the response package raise ImportError, by moving an annotation-only import
under TYPE_CHECKING.

Hold a trailing carriage return in the SSE line reader so an event whose CRLF
terminator is split across two read chunks is parsed as one event rather than
being split in two.

Enforce control-character validation on every Headers mutator (with_added,
with_set, with_merged), not only the constructor, so a CR/LF-bearing value can
no longer be injected through Request.with_header. Normalise HttpHeaderName
values to lower case so a directly-constructed instance still matches the
case-insensitive store.

Validate chunk_size on async request bodies (matching the sync and response
bodies), clamp FileRequestBody.content_length to the bytes actually available,
and thread the requested charset through form-body percent encoding. Route the
async stream body's close through a shared shielded-cleanup helper so a second
cancellation cannot leak the stream.

Normalise non-UTC aware datetimes before formatting an HTTP date, reject an
embedded double quote in ETag.parse, let suffix byte-ranges share the public
HttpRange type so they compose in format_many, and give the paginator wrappers
explicit typed parameters instead of *args/**kwargs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Decline (rather than raise UnicodeEncodeError) when credentials cannot be
encoded under the negotiated charset, and decline a session-variant algorithm
offered without qop, since such a response would fold cnonce into HA1 yet omit
it from the header, leaving the server unable to verify it.

Stop emitting cnonce and nc on a qop-less (RFC 2069) response, where RFC 7616
forbids them, and treat preferred_algorithms as an allow-list so a caller can
actually refuse weaker algorithms. Build the parsed challenge's parameters once
and expose them as a read-only mapping.

Give AsyncBearerTokenPolicy the challenge-handler support and 401/407 handling
its sync counterpart already had, so proxy and Digest/Basic negotiation work on
the async path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Skip a signature candidate that is not ASCII-encodable instead of letting
UnicodeEncodeError escape verify(), so an attacker-supplied signature header
cannot turn into an unhandled error. Reject an empty decoded secret at
construction rather than silently building a verifier with an empty key, and
require the timestamp to be a plain ASCII integer.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…en builders

Close the in-hand response before re-sending on a status-driven retry and
before raising when a redirect cannot be reissued, so retries and failed
redirects no longer leak pooled connections. Move the synchronous retry's
status-path sleep outside the try block so a budget-deadline timeout
propagates immediately instead of being reclassified and retried, matching the
async policy, and thread the injected clock into the async policy's inner
configuration so an HTTP-date Retry-After is measured deterministically.

Evict the per-call ContextStore entry once Pipeline.run / AsyncPipeline.run
completes (after in-chain observers have read it), so the store no longer grows
without bound across calls with distinct trace ids.

Raise a clear error from StagedPipelineBuilder.from_pipeline when a pipeline
contains a bare-callable sans-io step that cannot be rehydrated, instead of an
opaque AttributeError. Remove the unused, unwired RetryConfig from the public
surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…stom defaults

Encode mapping keys symmetrically with the decode side so a field typed as a
dict keyed by an enum, date, or UUID survives a decode/encode round trip
instead of raising on re-serialisation. Detect a bare (non-parameterised)
Tristate annotation so an absent or null value keeps its tri-state meaning.
Chain a caller-supplied json default behind the built-in datetime and bytes
encoders instead of replacing them, so supplying one no longer silently breaks
the built-in handling.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…O_PROXY matching

Redact the whole parameter for a non-allowlisted query item, including the key,
so a bare token carried as the key of a callback URL is no longer logged in the
clear. Reject inf and nan durations in configuration parsing so a non-finite
value can never disable deadline enforcement. Match NO_PROXY entries with
conventional suffix semantics (host equality or a dot-delimited subdomain),
matching curl, requests, and Go, rather than as whole-string globs.

Fall back to the cause's traceback when an SdkError is constructed with an
explicit cause outside an active except block, so its exc_info triple is
coherent. Re-raise a base exception from a drained loggable response body after
recording it, so a partly-drained body is never replayed as a complete one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s on error

Classify connect-phase and read-phase timeouts consistently across the
adapters: a read timeout maps to a response timeout and a connect timeout to a
request timeout, so the retry policy charges them to the right budget and never
auto-retries a non-idempotent request whose response may already be in flight.
Map the asyncio transport's read-phase timeouts and stream errors into the SDK
error hierarchy instead of leaking raw stdlib exceptions, and stop overwriting a
caller-supplied Host header.

Release the underlying connection when a response carries a status code outside
the known set before raising, so an unrecognised code no longer leaks a pooled
connection, and make the aiohttp client refuse use after close instead of
silently opening a fresh session.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a test that scans every package source and test file and fails if a Sphinx
cross-reference role appears, enforcing the project's plain-backtick docstring
convention. Neither ruff nor mypy inspects docstring prose, so this is the only
mechanical guard keeping the convention from rotting back in.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ped surface

Correct the documented default pipeline order and add the subsystems that had
shipped without mention (webhook verification, the pagination package, the
idempotency and client-identity policies, correlation, and the reconnecting SSE
client) across the README, CHANGELOG, project guide, and docs pages. Fix a
broken import in the pipelines guide, a dangling reference and a misplaced
transport in the architecture guide, an inverted claim about retry body
buffering, a reference to a non-existent error type, and several value-object
and auth descriptions that no longer matched the code.

Regenerate the public API surface baseline to reflect the now-narrower surface
(the suffix-range type, the abstract CallContext base, and the removed
RetryConfig).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The codec advertised UUID support in its docstrings but had no encode or
decode branch for it, so a `UUID` field or a `dict[UUID, ...]` raised
SerializationError on encode and was never reconstructed on decode.

Encode a UUID to its canonical string and decode it back through the same
type-driven path used for enums and datetimes, so UUID values and keys now
survive a full round trip. Clarify the mapping-key docstrings while here:
enum, temporal, and UUID keys reconstruct to their declared type, while bare
scalar (int/float/bool) keys follow the codec's existing no-coercion rule and
come back as their wire string.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bearer-token policies re-sent the request over the previous 401/407
response without closing it first, stranding the pooled connection on every
challenge or handler retry. Close the rejected response before each re-issue
on both the challenge-handler and on_challenge paths, in the sync and async
policies, matching the convention the retry and redirect policies follow.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ETag.parse accepted a literal space (0x20) inside the opaque tag. Per
RFC 7232 section 2.3 the entity-tag character set excludes everything at or
below SP and DEL, so tighten the guard to reject 0x20 alongside the control
characters and DQUOTE already rejected, while keeping obs-text (0x80-0xFF)
valid.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bypasses_proxy compared the full candidate string, so a port-qualified host
(api.example.com:443) never matched a bare example.com entry, and a
conventional host:port bypass entry was unsupported -- despite the module
documenting curl/requests/Go parity. Strip a trailing :port (and IPv6
brackets) from both the candidate and the entry before matching, so
comparison is host-only as those tools do.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three adapter inconsistencies in how a request-versus-response failure is
reported, which matters because the retry policy must never blind-retry a
non-idempotent request whose response may already be in flight:

- The aiohttp client configured only a total timeout, so a connect-phase
  stall raised a bare TimeoutError indistinguishable from a read timeout and
  was misclassified as a response error. Configure sock_connect/sock_read so
  connect raises ConnectionTimeoutError (request error) and read raises
  SocketTimeoutError (response error), matching the per-operation timeouts
  the other adapters already use.
- The requests client did not wrap response-body streaming, so a mid-body
  ChunkedEncodingError, connection drop, or read timeout escaped as the raw
  library exception. Map a body-read Timeout to ServiceResponseTimeoutError
  and any other RequestException to ServiceResponseError.
- Document why the asyncio reference client deliberately classifies a
  post-connect OSError as a response error rather than the request error the
  other adapters use for their generic transport bucket.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Unreleased notes claimed no public symbol was removed, but RetryConfig
was dropped from the pipeline surface. Correct the preamble, add a Removed
entry for RetryConfig, and document the HttpRange.suffix return-type change
and CallContext becoming an abc.ABC under Changed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove leftover per-test tracking tags from a few test docstrings and
comments; they carried no information for readers of the suite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@OmarAlJarrah OmarAlJarrah merged commit 526ecc7 into main Jun 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant