[TexasCoding]

Closes #252
Closes #251
Closes #262

Part of #273 (post-v2.4 audit), Wave 2.

[claude]

Code Review — PR #286

Overview

Three targeted fixes to _base_client.py:

1. Errors: content-length cap on error body drops the typed-exception class #252 — Preserve typed error class when the body is capped (oversized Content-Length). Previously the body-cap path early-returned a bare KalshiError; now it sets a suppressed flag and falls through to the status dispatch so callers still get KalshiRateLimitError, KalshiAuthError, etc.
2. Errors: 408 and 504 do not map to KalshiTimeoutError #251 — Map HTTP 408 and 504 to KalshiTimeoutError so callers can branch on the "may have committed" semantic and reconcile via client_order_id.
3. Perf: header merge allocates a fresh 4-way dict per retry attempt #262 — Hoist the invariant header layers (config_extra, per_call_extra, body_headers) out of the retry loop; only auth_headers changes per attempt (fresh timestamp).

The approach is clean and targeted. A few observations below.

---

What's Good

• suppressed flag pattern is the right refactor: it avoids duplicating the entire status-dispatch block and makes the invariant ("body may be absent, status is always known") explicit.
• Retry semantics preserved: 408 and 504 are still in RETRYABLE_STATUS_CODES, and retry decisions are keyed on response.status_code, not the exception type — so the routing change has no effect on retry behaviour.
• Header optimization is safe: base_headers is built from values that don't change across attempts (config.extra_headers, the per-call extra_headers, and headers). The single dict construction outside the loop is correct.
• Test coverage is solid: the three new test_oversized_body_* tests directly exercise the Errors: content-length cap on error body drops the typed-exception class #252 fix across 429/401/409, and test_500_still_server_error is a clean regression guard.

---

Observations / Suggestions

1. Semantic mismatch for HTTP 408

KalshiTimeoutError documents: "The server may or may not have processed it." That's accurate for 504 (the gateway timed out waiting for the upstream, which may have committed). For 408 it's overly cautious — a 408 means the server closed the connection because the client was too slow to send the full request. The server definitively did not process it.

This isn't a correctness bug (treating 408 as potentially committed is conservative, not wrong), but callers who follow the KalshiTimeoutError reconciliation doc and hit a 408 will do unnecessary client_order_id lookups. Worth a one-liner in the docstring or a comment explaining why 408 is grouped here despite the asymmetry.

2. Missing test: oversized body + 408/504 → KalshiTimeoutError

The PR has test_oversized_body_401_preserves_auth_error and test_oversized_body_409_preserves_conflict_error, but there's no equivalent for the new 408/504 → KalshiTimeoutError path. Given that this PR introduces both features, a test combining them would confirm they compose correctly:

def test_oversized_body_408_preserves_timeout_error(self) -> None:
    # #251 + #252
    resp = httpx.Response(408, headers={"content-length": "20000"}, text="x")
    err = _map_error(resp)
    assert isinstance(err, KalshiTimeoutError)
    assert "suppressed" in str(err)

3. body: dict[str, Any] annotation placement

Minor: the annotation body: dict[str, Any] appears above the if suppressed: branch. Both branches assign it before any use, so mypy is happy, but it's slightly unusual to separate the annotation from the first assignment like this. Consider initializing at the declaration (body: dict[str, Any] = {}) and letting the else branch overwrite — eliminates any "used before assignment" confusion at a glance.

4. Reformatting noise

~80 of the 124 deletions / 161 additions are reformatting (line-length adjustments in tests). These are cosmetically fine and ruff-compliant, but they make the meaningful diff harder to scan. Not a blocker.

---

Summary

The logic changes are correct and the test coverage is good. The only actionable gap is a missing test for the 408/504 + oversized-body combination. The semantic note on 408 is worth a comment but not a blocker. Happy to approve once the missing test is added.

[claude]

Code Review — PR #286: transport: preserve typed errors on body cap; map 408/504; perf header merge

Three distinct fixes, each well-scoped. Overall the logic is correct; a few gaps worth addressing before merge.

---

Fix #252 — Preserve typed exception on body cap

Approach is correct. The suppressed: bool flag lets body parsing be skipped while status dispatch runs unchanged. The empty-dict fallback is safe: all body reads (body.get(...)) return None cleanly, and the 429 block reads Retry-After from headers (not body), so rate-limit + oversized body works.

Gap — oversized 408/504 path not tested. Now that 408/504 map to KalshiTimeoutError (#251), the combination of both fixes (suppressed body + 408 or 504) is untested. A hostile intermediary could send a 504 with a multi-MB body, which would previously have returned KalshiServerError, then KalshiError after #203 — now it should return KalshiTimeoutError with suppressed message. Suggest adding:

def test_oversized_body_504_preserves_timeout_error(self) -> None:
    resp = httpx.Response(504, headers={"content-length": "50000"}, text="x")
    err = _map_error(resp)
    assert isinstance(err, KalshiTimeoutError)
    assert "suppressed" in str(err)

Gap — oversized 400/422 path not tested. The body.get("details") call runs on body = {} in the suppressed case; it returns None → details=None. Correct behavior but not exercised by the new tests.

---

Fix #251 — Map 408/504 to KalshiTimeoutError

Placement is correct — the new if status in (408, 504) block sits between the 429 handler and the >= 500 catch-all, so 504 is intercepted before KalshiServerError. The retry loop at line 319 checks response.status_code in RETRYABLE_STATUS_CODES, not the exception type, so the retry behavior for both codes is unchanged.

Potential breaking change for 504. Previously 504 fell through to KalshiServerError; callers catching except KalshiServerError to handle gateway timeouts will silently stop catching them. This is the correct semantic fix, but it should appear in CHANGELOG.md as a breaking change (or at minimum a notable behavior change), especially since KalshiTimeoutError is not a subclass of KalshiServerError.

Minor semantic note on 408. The KalshiTimeoutError docstring says "The server may or may not have processed it — reconcile via client_order_id." A 408 means the server timed out waiting for the client to finish sending; the server almost certainly did not process the request. The exception class is still the right choice for consistency, but the docstring's reconciliation guidance is more cautious than necessary for 408. Low priority.

Gap — no retry test for 504 on GET. test_408_retried_on_get exists; a symmetric test_504_retried_on_get is missing. Both codes are in RETRYABLE_STATUS_CODES.

---

Fix #262 — Hoist invariant header layers out of retry loop

Correct. base_headers is computed once; merged_headers = {**base_headers, **auth_headers} creates a fresh dict each attempt, so there's no aliasing issue. Header precedence is preserved: config < per-request < body-helper < auth.

No test. The optimization has no targeted test. The existing test_extra_headers_forwarded tests likely cover it indirectly, but a test that verifies per-request extra_headers still override config.extra_headers after the refactor would pin the ordering invariant. Not a blocker, but good to have.

---

Minor

• The blank line before class TestWidenedRetrySet was removed (line 1011 → 1013 in the diff). Most class boundaries in this file keep a blank line between them; worth restoring for consistency.
• New test comments (# #252) are terse. They meet the "WHY non-obvious" bar since they link to the issue — no change needed.

---

Summary

Area	Status
#252 logic (suppressed body + typed error)	✅ Correct
#251 logic (408/504 → KalshiTimeoutError)	✅ Correct
#262 logic (base_headers hoisted)	✅ Correct
RETRYABLE_STATUS_CODES interaction preserved	✅ Confirmed
Test: oversized body + 408/504	❌ Missing
Test: oversized body + 400/422	❌ Missing
Test: 504 retried on GET	❌ Missing
CHANGELOG entry for 504 behavior change	❌ Missing
Blank line style	Minor

The three core fixes are sound. The missing test combinations (oversized body × new error types) are the most important gap — they cover the intersection of two distinct changes in this PR.