Generic per-call RequestOptions across all SDK methods#52
Conversation
Aligns Node, Python, and Go on the same per-call request-options surface
that the rest of the SDK ecosystem has converged on (Stainless-generated
SDKs, Stripe, AWS SDK v3): timeout, cancellation, headers, and
idempotency key controllable on every call rather than only on
construction.
Node:
* New RequestOptions interface ({ signal, timeout, headers, idempotencyKey })
accepted as an optional second argument on send, reply, forward.
* Internal helper composes timeout and caller signal via AbortSignal.any
so either fires AbortError. AbortError and TimeoutError propagate
through unwrapSendResult unchanged instead of being repackaged into
PrimitiveApiError.
* idempotencyKey moves from SendInput to RequestOptions. Pre-1.0
breaking change; matches Stripe/Anthropic/OpenAI conventions.
Python:
* New RequestOptions dataclass plus per-call timeout, extra_headers,
idempotency_key kwargs on send/asend/reply/areply/forward/aforward.
* Internal helper clones AuthenticatedClient via attrs.evolve to apply
per-call options without leaking headers into subsequent calls.
* New PrimitiveClient.with_options(timeout=, extra_headers=) clone
helper for setting many calls at once. idempotency_key intentionally
not on with_options since it is a per-call concern.
Go:
* No code change. Every method already takes ctx context.Context as
the first argument, so per-call timeouts and cancellation work via
context.WithTimeout / context.WithCancel today. README updated to
document the pattern explicitly.
Cross-SDK shared fixture suite still passes. maxRetries is intentionally
out of scope: client-side retries deserve their own design conversation
(safe-to-retry operation classification, idempotency-key requirement,
exponential backoff policy).
Linear: PRI-157
Greptile SummaryThis PR adds a per-call
Confidence Score: 5/5Safe to merge. The request-options surface is well-tested across all three SDKs and the contextvar isolation in Python is correct for both sync and async concurrent use. All changed paths have matching new tests (pre-aborted signal, fired timeout, custom headers, idempotency key on every method, signal+timeout combined, with_options defaults vs per-call override). The single comment is a docstring precision issue that does not affect runtime correctness. sdk-python/src/primitive/client.py — the with_options docstring mildly overstates what timeout=None achieves, but the implementation and tests are correct. Important Files Changed
Reviews (5): Last reviewed commit: "Merge branch 'main' into sdk-per-call-re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Previously, _apply_request_options early-returned when all kwargs were None, so chaining with_options(timeout=5.0) then with_options(timeout=None) would leave the 5s timeout in place. The original api_client was returned unchanged. Introduce a private _NotGiven sentinel for the timeout parameter on with_options, matching the OpenAI/Anthropic Python SDK pattern. The three states are now distinct: omitted leaves the clone unchanged, None clears the timeout (httpx Timeout(None) for no timeout), and a float sets it. extra_headers stays default-None with merge-only semantics: there is no way to remove a header that an earlier with_options call added because header provenance is not tracked. Documented in the docstring; callers needing a clean baseline should construct a fresh client. Use isinstance(timeout, _NotGiven) rather than identity comparison so basedpyright narrows the type properly in the elif/else branches. Linear: PRI-157
|
|
||
| A canceled `ctx` surfaces as `context.Canceled`; a deadline exceeded surfaces | ||
| as `context.DeadlineExceeded`. Both are distinct from API errors returned by | ||
| `*PrimitiveAPIError`, so callers can tell a client-side abort apart from a |
There was a problem hiding this comment.
should be *primitive.APIError
There was a problem hiding this comment.
Fixed in f73757b. README now says *primitive.APIError.
| // Per-call timeout: cancel after 15 seconds. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
| _, err := client.Send(ctx, primitive.SendParams{...}) |
There was a problem hiding this comment.
primitive.SendParams{...}, is not valid go
There was a problem hiding this comment.
Fixed in f73757b. First example now uses a full struct literal (From/To/Subject/BodyText).
| // Per-call cancellation: bail out from another goroutine. | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| go func() { <-userBailoutSignal; cancel() }() | ||
| _, err := client.Send(ctx, primitive.SendParams{...}) |
There was a problem hiding this comment.
primitive.SendParams{...}, is not valid go
There was a problem hiding this comment.
Fixed in f73757b. Second example uses /* params */ so it reads as a placeholder rather than something a reader would copy as-is.
There was a problem hiding this comment.
When using request options, we drop custom HTTP setup. This only happens in the python sdk
There was a problem hiding this comment.
Good catch. The previous attrs.evolve(api_client, ...) per call reset the init=False httpx fields, rebuilding a fresh httpx.Client each call and dropping any client installed via set_httpx_client.
Rewritten in f73757b: per-call options now flow through an httpx request hook driven by a ContextVar. The hook is installed once on the AuthenticatedClient, and with_options stores defaults on PrimitiveClient itself rather than cloning the api_client. client.api_client._client identity is now stable across calls, custom clients survive, and the contextvar keeps it safe across threads and async tasks. New tests in test_client.py cover (a) custom-client identity, (b) no header/timeout leakage between calls, (c) async parity.
There was a problem hiding this comment.
sdk-go doesn't support Idempotency-Key on reply and forward. The other SDKs do.
There was a problem hiding this comment.
Forward now supports it. IdempotencyKey on ForwardParams is threaded into the synthesized SendParams (f73757b), with test coverage in client_test.go.
Reply intentionally drops Idempotency-Key across all three SDKs. The server's reply route synthesizes its own send-mail call and forwards only content-type and authorization, so the header was a silent no-op in Node and Python. Removed from Python reply/areply, and Node rejects it via Omit<RequestOptions, "idempotencyKey"> plus a runtime guard. The SDK surface can be widened non-breakingly when the server route honors it.
| @dataclass(frozen=True, slots=True) | ||
| class RequestOptions: | ||
| """Per-call HTTP options applied to a single client method call.""" | ||
|
|
||
| timeout: float | None = None | ||
| extra_headers: dict[str, str] | None = None | ||
| idempotency_key: str | None = None |
There was a problem hiding this comment.
This is never used anywhere. We only use kwargs
There was a problem hiding this comment.
Right, methods only take kwargs so the public dataclass added no value. Removed from the public surface in f73757b (no longer exported from primitive/__init__.py). Internal _PerCallOptions and _ClientDefaults dataclasses remain as private plumbing for the contextvar hook.
…eanup, Go README fixes
Python: refactor per-call options to a request-time hook driven by a
contextvar, and stop cloning AuthenticatedClient per call. Previously,
attrs.evolve(api_client, ...) reset the init=False httpx client fields
on the clone, which dropped any client a user had installed via
set_httpx_client and rebuilt a fresh httpx.Client on every call,
losing connection pooling. The new approach installs a request hook
once, the hook reads per-call options from a ContextVar, and
with_options stores defaults on PrimitiveClient itself rather than
cloning the api_client. Connection pooling and custom-client identity
are preserved. The previously-exported but unused RequestOptions
dataclass is removed from the public surface.
Reply Idempotency-Key cleanup: the server's reply route synthesizes
its own request to send-mail and only forwards content-type and
authorization, so Idempotency-Key on reply was a silent no-op.
Removing it from reply / areply (Python) and rejecting it via both
the type system (Omit<RequestOptions, "idempotencyKey">) and a
runtime guard on reply (Node). idempotencyKey stays on send and on
forward (which constructs a send under the hood). When the server
adds Idempotency-Key forwarding on the reply route, the SDK surface
can be widened to accept it.
Go: add IdempotencyKey to ForwardParams and thread it through to the
SendParams that Forward synthesizes, matching cross-SDK parity for
the forward path. README example updated with valid Go syntax (the
prior `SendParams{...}` literal was not valid) and the correct error
type name (`*primitive.APIError`).
Linear: PRI-157
The earlier commit on this branch reverted Idempotency-Key support on reply / areply because the server's reply route synthesized its own request to send-mail and dropped any caller-supplied Idempotency-Key header on the way through. The mono-repo PR (PRI-159) closes that gap by also forwarding the header onto the synth request, so the customer- supplied path now reaches send-mail's idempotency machinery. With that gap closed, restore the original cross-SDK shape: * Node: drop the Omit<RequestOptions, "idempotencyKey"> on reply's options parameter and the runtime guard that threw on the field. Replace the rejection test with a positive test that asserts Idempotency-Key is sent on reply. * Python: re-add idempotency_key kwarg on reply / areply (and the _do_reply / _ado_reply internals so it threads through to the contextvar-driven request hook). Replace the rejection tests with positive forwarding tests that assert the header lands on the outgoing request. * Both READMEs: drop the "reply does not accept idempotencyKey" caveat now that it does. Send and forward already accept the field; this restores the originally- intended behavior on the third method. Linear: PRI-159
|
All six comments addressed in f73757b. The substantive one is the Python rewrite: per-call options now flow through a contextvar + httpx request hook installed once on the |
2 participants
Summary
RequestOptionssecond-arg onsend/reply/forwardwithsignal/timeout/headers/idempotencyKey.idempotencyKeymoves fromSendInputtoRequestOptions(pre-1.0 breaking change, matches Stripe/Anthropic/OpenAI shape).timeout/extra_headers/idempotency_keykwargs on every method (send/asend/reply/areply/forward/aforward) plus aclient.with_options(...)clone helper. Go is already idiomatic viactx context.Contextas the first arg; README updated to documentcontext.WithTimeout/context.WithCancelexplicitly.Convention adopted
client.foo(input, options?)whereoptionsisRequestOptionsclient.with_options(...)ctx context.Contextas first arg (no change)Notable design choices
AbortSignal.any([signal, AbortSignal.timeout(ms)])so either firesAbortError.AbortError/TimeoutErrorskip the API-error repackaging path.attrs.evolvedirectly rather than the existingwith_headers/with_timeouthelpers because those mutate the underlying httpx client in place, which would leak per-call headers (includingIdempotency-Key) into subsequent calls.maxRetriesis intentionally out of scope. Client-side retries deserve their own design conversation: which operations are safe to retry, idempotency-key requirement, exponential backoff policy.Test plan
make node-check(lint, typecheck, generated-file-sync, 627 tests)make python-check(ruff, basedpyright, 224 tests)make go-check(gofmt, vet, tests)make shared-check(cross-SDK fixture parity)make node-buildmake python-buildmake go-buildwith_optionsdefaults plus per-call override (Python sync and async)