[TexasCoding]

Closes #240

Part of #273 (post-v2.4 audit), Wave 1 money-risk batch.

[claude]

Code Review — PR #279: retry idempotent verbs on httpx transport errors; add KalshiNetworkError

Overview

This PR closes #240 by adding proper handling for httpx.TransportError (TCP/TLS/DNS/protocol faults) that previously fell through to the bare KalshiError catch-all. The new KalshiNetworkError gives callers a typed exception to catch, and the retry logic correctly distinguishes pre-wire errors (ConnectError) from post-wire errors (ReadError, WriteError, RemoteProtocolError) for non-idempotent methods. Implementation is well-reasoned and the exception ordering is correct (more specific subclasses first).

---

Issues

1. from e vs from None inconsistency in budget-exceeded path

The two existing handlers use raise last_error from None when the retry budget is exhausted before sleeping (lines 248, 267). The new TransportError handler uses from e in the same path (line 286 sync, line 478 async), which preserves the cause. That is actually better behaviour, but the inconsistency means callers inspecting __cause__ on KalshiNetworkError will sometimes find it set (budget-exceeded mid-retry) and sometimes not (for the sibling exceptions).

Suggestion: Either document this deliberately or unify all three handlers — updating the budget-exceeded path of PoolTimeout and TimeoutException to also use from e is the cleaner fix.

---

2. ConnectTimeout asymmetry (pre-existing, but now more visible)

httpx.ConnectTimeout is a subclass of TimeoutException, so it is caught by the existing timeout handler, which only retries idempotent methods. httpx.ConnectError is caught by the new TransportError handler and retries POST/DELETE via pre_wire = True. Both represent "connection never established — nothing reached the wire", so the asymmetry may surprise callers. This is pre-existing behaviour and out of scope here, but worth a follow-up issue or a note in the TimeoutException handler.

---

3. fake_sleep boilerplate in async tests (style)

Each async test defines its own async def fake_sleep + monkeypatch.setattr. The sync tests use the tighter one-liner lambda d: None. A small fixture would avoid repeating it across four tests.

---

4. Missing test: budget-exceeded mid-retry path

No test covers lines 285-286 (sync) / 477-478 (async) where _would_exceed_budget returns True inside the retry branch. Consistent with the existing suite, but worth noting.

---

What is good

• Exception ordering is correct: PoolTimeout → TimeoutException → TransportError → HTTPError respects the httpx subclass hierarchy.
• Cause preservation is consistent on all non-budget-exceeded raise paths (raise last_error from e).
• Error message follows F-O-09: only method and path are interpolated — no URL, no query string, no token-like values.
• route.call_count and exc_info.value.__cause__ assertions in tests are exactly right.
• Sync/async parity — both transports got the same handler with the same logic.
• Docs are accurate: the error hierarchy tree and prose in docs/errors.md precisely describe the new behaviour.

---

Verdict

Approve with item 1 flagged. Items 2–4 are not blockers. Item 1 is the only one that could cause a subtle debugging surprise for callers who inspect __cause__ and should be made intentional (unified or explicitly documented).

[claude]

Code Review — PR #279: Transport-level retry + KalshiNetworkError

Overview

This PR closes a real gap: before this change, httpx.TransportError subclasses (DNS failure, TCP RST, HTTP/2 RST_STREAM, etc.) surfaced as generic KalshiError, losing the type signal and bypassing retry logic entirely. The design — retry idempotent verbs freely, retry non-idempotent verbs only on ConnectError (pre-wire) — mirrors the existing PoolTimeout treatment and is sound.

---

✅ What's correct

Exception ordering is critical and right.
The full ordering is now PoolTimeout → TimeoutException → TransportError → HTTPError. Since TimeoutException is a subclass of TransportError in httpx, getting this wrong would silently route timeouts to KalshiNetworkError. The PR gets it right.

Pre-wire semantics for ConnectError are sound.
A ConnectError means the TCP handshake never completed — no bytes reached the server. Retrying POST/DELETE on ConnectError is safe and consistent with how KalshiPoolExhaustedError (also pre-wire) is treated.

Budget check is applied.
_would_exceed_budget guards the new retry path, preventing transport errors from extending wall-clock time past total_timeout. Correct.

__cause__ chain preserved.
raise last_error from e on every exit path. Callers and log aggregators can always inspect the root httpx exception.

---

Issues

1. CLAUDE.md says "POST and DELETE are NEVER retried" — needs a clarifying update (minor)

The project conventions state:

POST and DELETE are NEVER retried (duplicate order/cancel risk). Only GET/HEAD/OPTIONS retry.

That rule targets HTTP-level retries (429/5xx responses). This PR introduces transport-level pre-wire retries, which are a distinct category. The ConnectError semantics make it correct, but the convention text is now misleading without qualification. A one-line clarification in CLAUDE.md would prevent confusion:

POST and DELETE HTTP responses (429/5xx) are never retried.
ConnectError (pre-wire) is retried on all methods — the server never received bytes.

2. Missing test: DELETE + ConnectError should retry (gap)

The test suite covers POST+ConnectError (retries) and POST+ReadError (no retry). It doesn't cover DELETE+ConnectError. Since DELETE cancellations are money-risk operations, an explicit test that a pre-wire ConnectError retries — and a ReadError does not — would match the stated Wave 1 "money-risk batch" scope.

3. Missing test: WriteError on POST (gap)

ReadError is tested as non-retryable on POST. WriteError (partial send) has the same or higher risk (bytes may have been accepted mid-write) and deserves its own test case. Same applies to RemoteProtocolError.

4. asyncio.sleep patching is global, not module-scoped (minor precision issue)

Sync tests patch kalshi._base_client.time.sleep — the module-level reference. Async tests patch the global asyncio.sleep instead of kalshi._base_client.asyncio.sleep. Either works currently, but module-level patching is more precise and won't break if the async transport ever aliases the import:

# consistent with sync tests:
monkeypatch.setattr("kalshi._base_client.asyncio.sleep", fake_sleep)

5. fake_sleep is duplicated four times in the async test class (nitpick)

Each async test method re-defines the same two-liner. A shared @pytest.fixture or a class-level helper would eliminate the repetition:

@pytest.fixture(autouse=True)
def patch_sleep(self, monkeypatch: pytest.MonkeyPatch) -> None:
    async def _noop(d: float) -> None:
        pass
    monkeypatch.setattr("kalshi._base_client.asyncio.sleep", _noop)

---

Summary

Category	Finding	Severity
Correctness	Exception ordering (TimeoutException before TransportError) is right	✅
Correctness	Pre-wire ConnectError retry logic is sound	✅
Correctness	Budget check applied to new retry path	✅
Documentation	CLAUDE.md "POST/DELETE NEVER retried" needs scope clarification	Minor
Tests	DELETE+ConnectError retry not tested	Minor
Tests	WriteError/RemoteProtocolError on POST not tested	Minor
Tests	async sleep patching is slightly imprecise	Nitpick
Tests	fake_sleep duplicated 4× in async class	Nitpick

The core implementation is solid and ready to merge. Items 2–3 (DELETE and WriteError test coverage) are worth adding given this is explicitly a money-risk batch — but could be a fast follow if you want to keep the PR scope tight.