[TexasCoding]

Wave 1-A of the audit umbrella. 7 issues; HTTP transport hardening.

Changes

• Retry: include Cloudflare 5xx (520–524) and 408 in RETRYABLE_STATUS_CODES #192 Retry: widen RETRYABLE_STATUS_CODES to include 408/425/520–524 (Cloudflare 5xx). POST/DELETE still never retry.
• Retry: cap total wall-clock budget; no per-request deadline today (~180s worst case) #193 KalshiConfig.total_timeout: wall-clock deadline (None = legacy unbounded).
• Error mapping: 422 / 409 / 408 / 410 fall through to bare KalshiError (+ tests for fallback) #201 _map_error: 422 → KalshiValidationError, 409 → KalshiConflictError. Body capped at 16KB via Content-Length; message truncated to 1024 chars.
• Config: validate base_url includes /trade-api/v2 path component (silent sign-path corruption) #202 KalshiConfig.__post_init__ validates base_url path == /trade-api/v2.
• Errors: cap _map_error response body buffering (memory + log volume) #203 _map_error body buffering bounded (memory + log volume).
• Errors: add KalshiTimeoutError + KalshiPoolExhaustedError (typed timeout semantics) #204 httpx.PoolTimeout → KalshiPoolExhaustedError (always retryable, even POST). httpx.TimeoutException → KalshiTimeoutError.
• Auth: KalshiClient.close() shouldn't shut down externally-provided KalshiAuth executor #210 KalshiClient/AsyncKalshiClient.close() only shut down KalshiAuth when client owns it (_auth_owned).

Tests

15 new test classes / suites; 183 tests pass; ruff + mypy --strict clean on 7 changed files.

Closes #192, Closes #193, Closes #201, Closes #202, Closes #203, Closes #204, Closes #210
Refs #224

[claude]

Code Review — Wave 1-A Transport Hardening

Overall: solid, well-motivated PR with good test coverage across all 7 issues. A few things worth addressing before merge.

---

Bugs / Correctness

1. _map_error body guard is misleading about what it protects (minor, _base_client.py)

The inline comment says "short-circuit before httpx materialises it into memory", but httpx.Client.request() buffers the full response body before returning — the body is already in RAM by the time _map_error is called. The Content-Length guard's real value is bounding log line / exception message size and skipping JSON parsing on huge responses, which is still worth having. The comment should say that rather than imply memory is being protected at read time:

# #203: skip JSON parsing and bound the error message for large error bodies.
# Note: httpx has already read the body; this prevents spending CPU on
# parsing and caps log/Sentry payload size.

If actual memory protection is desired, the transport would need to switch to streaming responses with a read limit — that's a bigger refactor, not required now, but worth a follow-up issue.

2. from_env ownership override is a logic inversion trap (client.py, async_client.py)

# __init__
self._auth_owned: bool = auth is None      # True when WE build it

# from_env override
client._auth_owned = auth is not None      # True when try_from_env() returned auth

These two lines use opposite boolean expressions to set the same flag, which is correct today but easy to break on a future refactor. The disconnect is that __init__ treats auth is None as "I will build it", while from_env treats auth is not None as "I did build it". Functionally fine, but a cleaner fix is a private keyword arg:

# __init__
self._auth_owned: bool = kwargs.pop("_auth_owned", auth is None)

Or just document the inversion explicitly with a # NOTE: comment near each site so the relationship is obvious.

3. total_timeout accepts negative values (config.py)

KalshiConfig(total_timeout=-1.0) silently creates an object where every retry attempt will immediately fail the budget check, since (elapsed + delay) > negative_number is always True. If this is intentional for testing, document it. If not, add a __post_init__ guard:

if self.total_timeout is not None and self.total_timeout <= 0:
    raise ValueError(f"total_timeout must be positive, got {self.total_timeout}")

---

Design / Approach

4. base_url path enforcement is a breaking change — changelog entry needed

The __post_init__ validation that rejects anything without /trade-api/v2 will blow up any existing caller using a bare host URL or a /v1 path. This is intentional and correct, but it needs a CHANGELOG.md entry under a ### Breaking Changes header for the next version bump. The PR description links the issue but the changelog should surface it for SDK consumers.

5. Pool timeout raise last_error from e vs from None inconsistency

In the budget-check path, the pool timeout raises from None (suppresses chain). When retries are exhausted, it raises from e (preserves the httpx chain). The timeout handler always uses from None per the F-O-09 comment. Is the inconsistency intentional? If httpx.PoolTimeout also embeds URL-like strings in its message, it should consistently use from None. If it's safe to keep from e, a comment explaining why would help.

---

Test Coverage Gaps

6. No test for total_timeout + pool exhaustion

TestTotalTimeoutBudget / TestAsyncTotalTimeoutBudget test the budget with HTTP 502 retries, but not with pool exhaustion. The pool timeout handler has its own _would_exceed_budget call — that path isn't covered. Worth a two-line addition to TestPoolTimeout.

7. No test that total_timeout rejects negative values (once #3 above is addressed)

8. 425 (Too Early) is in RETRYABLE_STATUS_CODES but has no test

408 and 521 have explicit test cases. 425 is a niche TLS early-data code — even a one-line unit test against _map_error + retry would catch a future regression.

---

Nits

• Missing blank line before class TestAsyncWidenedRetrySet and class TestWidenedRetrySet — ruff E302 will catch this in CI but worth fixing before it blocks the run.
• test_error_message_truncated_to_1024_chars asserts len(str(err)) <= 1024 but str(err) includes the exception class name prefix (e.g. KalshiValidationError: AAAA...). The assertion might accidentally pass even if the message weren't truncated properly. Consider asserting len(err.args[0]) <= 1024 or checking .message directly.

---

Summary

Area	Status
Retry widening (#192)	✅
total_timeout (#193)	✅ (minor: no negative-value guard)
Typed error mapping (#201)	✅
base_url validation (#202)	✅ (needs CHANGELOG entry)
Body buffering bounds (#203)	✅ (comment slightly misleading)
Typed timeout exceptions (#204)	✅ (minor: from e vs from None inconsistency)
Close ownership (#210)	✅ (minor: from_env inversion pattern)
Test coverage	Good — a few gaps noted above

Blocking: none of these are hard blockers, but items 3 (negative total_timeout) and 4 (CHANGELOG) are worth a quick fix before merge. The rest can be tracked as follow-ups.