feat: §16 retry, §17 memo, §18 close(), §19 telemetry (D5) - #34
Merged
Conversation
Implements the four contract-1.8 quality-of-life sections across BOTH the sync and async clients. Re-vendors CONTRACT.md at 1.8.1. §16. This SDK had no bounded read-only retry policy at all — only §9.3's refresh-then-retry-once, which is a different mechanism: it reacts to a 401 by refreshing and deliberately does not loop. §11.2 rule 5 and §14.2 rule 6 had both been requiring "the SDK's existing bounded read-only retry policy" against a policy that did not exist here. _retry.py is that policy: 3 attempts, 200 ms base, 5 s cap, full jitter over [0, backoff], Retry-After as a floor. retry_sync and retry_async share backoff_ms/delay_ms rather than each carrying their own arithmetic. Duplicating it is precisely how eleven SDKs ended up with three different answers, and a sync/async pair is the easiest place for that to happen again unnoticed. The authz call is a POST but changes no server state, so it is retry-eligible: §16.2's test is "changes no server state", NOT "is a GET". Gating on the verb would exclude the single most important operation the policy covers. The §9.3 401-refresh stays *inside* one §16 attempt — a 401 means the token expired, which refreshing fixes, so neither backing off nor spending the transport-failure budget on it would make sense. §17. Opt-in decision memo, off by default, clamped to 5 s, thread-safe (the sync client is commonly shared across a thread pool). Allows and denies memoized identically, because asymmetric caching leaks which outcome occurred through latency. Failures never memoized — structurally, since set() is only reachable after a successful response. Cleared on login/verify_mfa/refresh/ logout, since entries are keyed by subject rather than session. §18. close()/aclose() now set a shutdown flag and clear the memo; every entry point calls _ensure_open() so use-after-close raises instead of silently reconnecting. Neither reaches the network: the server-side session outlives the client object, which is what lets a process restart and resume, and a close() that logged out would end every user's session on each deploy. The test asserts that against the wire, because a logout wired into close() succeeds silently. §19. Frozen event dataclasses with a fixed field set — there is no place to put a token in a payload bound for a metrics backend — and a dispatcher that swallows anything a sink raises, so telemetry cannot fail an authorization check. One RequestStart/RequestEnd pair per ATTEMPT, so callers can count real wire calls; the attempt number is threaded through the retry helper rather than defaulting to 1, which would make a retried call look like a single slow one. tests/test_d5_conformance.py (25 cases) asserts through the public check_access surface and counts requests on the wire, as contract 1.8.1 now requires — that is the assertion that would have caught TypeScript's exported-but-never- called retry helper. Both clients are covered: they are separate classes here, so a fix applied to one and not the other is a real failure mode. Gates: 626 tests pass, mypy clean over 37 files, ruff check and format clean.
CI's lint job runs four gates I had only partly reproduced locally: mypy --strict (I ran plain mypy), ruff check ., ruff format --check . (I scoped mine to src/tests/examples) and interrogate at fail-under = 100. interrogate found six undocumented members in the new modules — dunders and a nested helper that the public-API docs still cover: DecisionMemo.__len__, retry_async._default_sleep, TelemetryDispatcher.__init__, and _RequestSpan's __init__/__enter__/__exit__ 98.5% against a 100% floor. The floor is the point: this SDK publishes its API docs with pdoc, and a dunder with no docstring renders as a bare signature. mypy --strict was already clean. ruff format found one file outside the paths I had been checking.
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
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.
Implements the four contract-1.8 quality-of-life sections across both the sync and async clients. Re-vendors
CONTRACT.mdat 1.8.1.Third of eleven SDKs, after rust#45 and typescript#47.
§16 — a requirement that had gone unmet since it was written
This SDK had no bounded read-only retry policy. It has §9.3's refresh-then-retry-once, which is a different mechanism: it reacts to a
401by refreshing and deliberately does not loop.Meanwhile §11.2 rule 5 and §14.2 rule 6 had both been telling SDKs to retry "under the SDK's existing bounded read-only retry policy" — against a policy that did not exist here.
_retry.pyis that policy: 3 attempts, 200 ms base, 5 s cap, full jitter over[0, backoff],Retry-Afteras a floor.retry_syncandretry_asyncsharebackoff_ms/delay_msrather than each carrying its own arithmetic. Duplicating it is precisely how eleven SDKs ended up with three different answers, and a sync/async pair inside one repo is the easiest place for that to happen again unnoticed.The authz call is a
POST, and it is retry-eligible. §16.2's test is "changes no server state", not "is a GET" — gating on the verb would exclude the single most important operation this policy covers. The §9.3 401-refresh stays inside one §16 attempt: a401means the token expired, which refreshing fixes, so neither backing off nor spending the transport-failure budget on it would make sense.§17 — opt-in, and thread-safe
Off by default (
decision_memo_ttl_ms=0), clamped to 5000 ms. Thread-safe — unlike the TypeScript and Rust equivalents, because the sync client here is commonly shared across a thread pool, and a cache that corrupted under concurrency would be a worse bug than the one it is optimising away.Allows and denies memoized identically (asymmetric caching leaks the outcome through latency). Failures never memoized — structurally, since
set()is only reachable after a successful response. Cleared onlogin/verify_mfa/refresh/logout, since entries are keyed by subject rather than session.§18 — close() and aclose() do not log out
Both set a shutdown flag and clear the memo; every entry point calls
_ensure_open()so use-after-close raises rather than silently reconnecting.Neither reaches the network. The server-side session deliberately outlives the client object — that is what lets a process restart and resume — so a
close()that logged out would silently end every user's session on each deploy. The test asserts that against the wire, because alogoutwired intoclose()succeeds silently and would pass any return-value assertion.§19 — closed event set, per-attempt pairs
Frozen dataclasses with a fixed field set, so there is no place to put a token in a payload bound for a metrics backend — and a test asserting the frozen-ness, since a sink that could mutate a shared event would let one hook corrupt another's input. The dispatcher swallows anything a sink raises: telemetry may not fail an authorization check.
One
RequestStart/RequestEndpair per attempt. The attempt number is threaded through the retry helper rather than defaulting to 1 — that exact bug appeared in the TypeScript PR and would make a retried call indistinguishable from a single slow one.Tests assert on the wire, per contract 1.8.1
tests/test_d5_conformance.py(25 cases) drives the publiccheck_accesssurface throughrespxand counts requests reaching the transport. That is the assertion 1.8.1 now requires, after TypeScript'swithRetryturned out to be exported, unit-tested, green, and called by nothing.Both clients are covered. They are separate classes here (
AxiamClient/AsyncAxiamClient, SDK-Q08), so a fix applied to one and not the other is a real failure mode rather than a hypothetical one.Verification
pytestmypy srcruff check src tests examplesruff format --checkOne
mypyfinding fixed en route:BASE_DELAY_MS * (2 ** (attempt - 1))returnedAnybecauseint ** intwidens — now explicitlyfloat(...).Notes
httpx.MockTransportdirectly and every wire-level case failed with a proxy502: the environment's proxy mounts take precedence over_transport. Rewritten to userespx, which is what the rest of this suite already uses.Generated by Claude Code