fix: Order.type rename, BASE_URL validation, Retry-After NaN/negative guard

[TexasCoding]

⚠️ BREAKING CHANGE

Order.type → Order.order_type. Wire format unchanged (incoming JSON type still populates the model via validation_alias=AliasChoices("type", "order_type")), but any user code accessing .type on an Order instance must migrate to .order_type. This must be called out in CHANGELOG.md and is a candidate for v2.0 rather than v1.2 — defer to release-cut decision.

Rationale (per #91 issue body + agent review): the spec v3.13.0 actually defines type as required (enum: limit, market), so removing the field would orphan a spec-required field. Renaming follows the existing project convention for milestone_type / target_type / incentive_type.

---

Summary

Three disjoint correctness fixes from the v1.2 audit:

#91 — Order.type cleanup + bool query-param normalization (de92e87)

• Order.type: str | None → Order.order_type: str | None with validation_alias=AliasChoices("type", "order_type") (see breaking-change banner above)
• 5 inline "true" if x else None ternaries in kalshi/resources/events.py and kalshi/resources/series.py replaced with _bool_param(...). Behavioral delta: False now serializes as "false" instead of being dropped from the query string — matches the issue's documented intent.

#94 — KALSHI_API_BASE_URL validation (0198a73)

• New _validate_base_url called from KalshiConfig.__post_init__.
• Reject at config-load: non-http(s) schemes, http (non-https) on remote hosts.
• Warn (not reject) on https with unknown host — keeps legitimate proxy use cases working.

#96 — Retry-After negative/NaN/infinite guard (6bfb585)

• math.isfinite + < 0 check added in _map_error. Negative/NaN/inf treated as "no retry-after" (falls back to backoff).

Test deltas

1407 → 1429 (+22 regression tests). All passing. ruff + mypy --strict clean.

Closes #91
Closes #94
Closes #96

Test plan

• uv run pytest tests/ --ignore=tests/integration -v
• uv run ruff check .
• uv run mypy kalshi/
• Confirm v1.2 vs v2.0 version decision at release cut given the breaking rename

[claude]

Code Review — PR #112

Overall: Three focused, well-targeted correctness fixes. Logic is sound, test coverage is solid. A few items to address before merge.

---

#91 — Order.type → Order.order_type

What's good:

• Rename follows the established convention (milestone_type, target_type, incentive_type).
• validation_alias=AliasChoices("type", "order_type") correctly accepts both the wire name and the SDK name, so deserialization is unbroken.
• Three-case test coverage (wire format, SDK name, field-absence check) is exactly right.

Issues:

🔴 CHANGELOG not updated (blocker). The PR description says this must be called out in CHANGELOG.md and version decision is deferred — but merging without a changelog entry ships a breaking rename silently. Even if the version decision is v1.2 vs v2.0, the entry should be drafted now and gated, not left out entirely. Users upgrading on a ^1.x pin will get AttributeError: 'Order' object has no attribute 'type' with no migration guide.

🟡 No deprecation path. There is no @property def type(self) shim emitting a DeprecationWarning. A soft deprecation period (one minor, then remove) is lower risk than a hard break, especially since Order is a public return type from nearly every orders call. Worth a one-liner property if there's any appetite for it.

🟡 Order has no model_config (pre-existing, not introduced here). From the file, class Order(BaseModel) at line 34 has no model_config, meaning it uses Pydantic defaults (extra="ignore"). Several sibling models explicitly set extra="allow". This is pre-existing and the test passes because "order_type" is an alias choice (not relying on populate_by_name), but it's worth a follow-up issue to audit response models for consistent extra policy.

---

#94 — KalshiConfig base URL validation

What's good:

• Warn-not-reject for unknown https hosts is the right call — preserves proxy and test harness use cases.
• IPv6 handling is correct: urlparse("http://[::1]:8000/") yields hostname="::1" (brackets stripped), which matches _LOCAL_HOSTS.
• Test coverage hits all the important rejection cases including file:// and scheme-less URLs.

Issues:

🟡 ws_base_url is not validated. __post_init__ calls _validate_base_url(self.base_url) but not self.ws_base_url. WebSocket connections also carry the KALSHI-ACCESS-KEY header, so ws://attacker.example is the same credential-leakage risk the issue was filed to address. If the exclusion is intentional (e.g., because WebSocket URLs use wss:// which _validate_base_url would reject), that should be documented. If it's an oversight, it needs fixing.

Nit: Test variable name host in test_http_to_loopback_is_allowed loops over ("localhost", "127.0.0.1", "[::1]") — the third element is the URL-representation (with brackets), not the parsed hostname. Functionally fine (urlparse strips the brackets), but renaming the variable to url_host would prevent future confusion.

---

#96 — Retry-After guard

What's good:

• Covers all three failure modes (negative → busy-loop, NaN → time.sleep crash, infinity → survives min() cap).
• Zero is correctly kept as a valid "retry immediately" value — the < 0 check is precise.
• Five regression tests including the existing HTTP-date path.

Issues:

🟡 Comment block violates CLAUDE.md style guide. The four-line comment inside _map_error is the only multi-line block in the transport layer:

# Reject negative, NaN, and infinity. Negative would make
# ``min(retry_after, retry_max_delay)`` go negative and turn
# ``time.sleep`` into a no-op (busy-loop). NaN propagates
# through ``min`` and crashes ``time.sleep`` with ValueError.
# Fall back to computed backoff in either case.

Per CLAUDE.md: "multi-line comment blocks — one short line max." Trim to one line, e.g.:

# Reject non-finite / negative values; NaN crashes time.sleep, negative creates a busy-loop.

---

_bool_param consistency

No issues. Events and series are the only two files in the PR branch that had the old inline ternary — markets.py was already using _bool_param. The fix is complete.

---

Test delta (+22 tests)

Coverage is well-distributed across the three fixes. One thing that could be added but isn't strictly required: a regression test confirming Order.order_type round-trips through the full portfolio.orders.list() call (not just Order.model_validate). The unit test is sufficient for the model contract, but an integration-style respx mock asserting the resource method returns the renamed field would protect against a resource-layer regression.

---

Summary

Item	Severity
CHANGELOG missing breaking-change entry	🔴 Blocker
ws_base_url not validated	🟡 Should fix
No deprecation shim for .type	🟡 Consider
Multi-line comment in _base_client.py	🟡 Style
Order lacks model_config (pre-existing)	📝 Follow-up issue

The ws_base_url gap and the CHANGELOG entry are the two items I'd want resolved before merge. Everything else is non-blocking.

---

Review by claude[bot]

[TexasCoding]

Addressed in latest commit. Summary of what landed vs. what was deferred:

Fixed

• 🔴 CHANGELOG entry for the breaking Order.type → Order.order_type rename added under [Unreleased] → ### Breaking. Version-bump decision (v1.2 vs v2.0) still deferred to release cut, but the entry exists.
• 🟡 ws_base_url not validated — generalized _validate_base_url → _validate_url(secure=, plaintext=) so http/https and ws/wss share the rule. __post_init__ now calls it on both URLs. Added TestWsBaseUrlValidation (+6 tests).
• 🟡 Multi-line comment in _base_client.py — trimmed to one line per CLAUDE.md.

Deferred

• 🟡 @property def type(self) deprecation shim — user explicitly picked the clean-break rename (no shim). This is the trade-off behind the version-bump deferral; if the call ends up as v2.0 the lack of shim is fine, if v1.2 we should revisit.
• 📝 Order lacks model_config (pre-existing) — opened Audit response models for consistent extra= policy #114 as follow-up to audit the extra= policy across all response models uniformly.

Skipped

• Nit: host → url_host rename in the _LOCAL_HOSTS test loop (cosmetic).
• Optional respx integration test for Order.order_type round-trip through portfolio.orders.list() (the model-level unit + identity test already cover the contract).

uv run pytest tests/test_client.py -q → 75 passed. ruff + mypy --strict clean.

[claude]

Code Review — PR #112

Three disjoint correctness fixes, well-scoped and surgical. Each one ships with solid regression tests. Overall this is clean work. A few things worth addressing before merge.

---

Bug: Retry-After: 0 is preserved by _map_error but silently dropped by the transport

kalshi/_base_client.py — the _map_error fix is correct, but the transport layer still has a falsy-check bug.

The new guard correctly keeps retry_after_val = 0.0 ("retry immediately"), and test_429_retry_after_zero_is_kept verifies it lands on the error object. But the retry loop in SyncTransport (and presumably AsyncTransport) uses:

if isinstance(error, KalshiRateLimitError) and error.retry_after:
    delay = min(error.retry_after, self._config.retry_max_delay)

0.0 is falsy in Python, so a Retry-After: 0 header falls through to _compute_backoff and is ignored. The test proves the value is stored correctly but never verifies the transport actually honours it. Either:

• Change the transport guard to error.retry_after is not None, or
• Tighten the test to mock a full retry cycle and assert the delay used was 0.

This is pre-existing, but since the PR explicitly documents "Zero is a valid 'retry immediately' value" and adds a test for it, leaving the transport drop undocumented feels like it will trip someone up.

---

Convention violation: multi-paragraph docstring on _validate_url

kalshi/config.py:57–72

CLAUDE.md is explicit: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The docstring on _validate_url is 8 lines. The rationale is good, but it belongs in the commit message / issue thread, not the code. A single line like:

\"\"\"Reject URLs that would expose credentials (bad scheme or plaintext-to-remote).\"\"\"

is sufficient.

---

Minor: @staticmethod called via self — prefer the explicit form

kalshi/config.py:51–52

self._validate_url(self.base_url, "base_url", ...)
self._validate_url(self.ws_base_url, "ws_base_url", ...)

Calling a @staticmethod through self works but hides the fact that no instance state is involved. Prefer KalshiConfig._validate_url(...) or factor it out as a module-level function (_validate_url(...)) alongside _KNOWN_HOSTS. Either makes the intent obvious without any behavioral change.

---

Nit: _map_error guard — order of checks

kalshi/_base_client.py:62–63

if retry_after_val < 0 or not math.isfinite(retry_after_val):

math.isfinite already returns False for NaN, but NaN comparisons are tricky — nan < 0 is False in Python (not True), so the current order is actually correct (the isfinite check catches NaN). Worth a brief inline comment since the NaN-comparison behaviour surprises people:

# Check isfinite first: NaN < 0 is False in Python, so order matters.
if not math.isfinite(retry_after_val) or retry_after_val < 0:

Not a bug, just a readability note.

---

Positive notes

• Order.order_type rename: Clean, follows the milestone_type/target_type convention exactly. AliasChoices(\"type\", \"order_type\") preserves wire compatibility and the populate_by_name config lets the Python name work in model_validate. Three tests (wire name, Python name, field absence) cover it well.
• _bool_param extraction: The behavioral delta (False now sends \"false\" instead of being dropped) is the documented intent from Order.type is a dead field; inconsistent bool query-param serialization #91, and the tests verify both directions correctly.
• _validate_url security logic: The warn-not-reject stance for unknown-HTTPS hosts is the right call for proxy scenarios. Loopback-only plaintext is correctly scoped. Test matrix is thorough (file://, ftp://, missing host, loopback IPv6).
• Test count (+22): All three fixes have happy path, negative path, and edge cases. The test docstrings are well-tied to issue numbers.

---

Summary

One real bug to fix (Retry-After: 0 transport falsy check), one convention violation to fix (docstring length), and two nits. The core logic of all three fixes is correct.

[TexasCoding]

Second-pass review addressed:

Fixed

• 🔴 Real bug: Retry-After: 0 was dropped by the transport. Switched both sync and async retry-loop guards from if error.retry_after: to is not None. Added end-to-end regression tests that monkeypatch time.sleep / asyncio.sleep and assert the recorded delay is exactly 0.0 (not the backoff fallback). +2 tests.
• ✅ Docstring length on _validate_url — trimmed to one line per CLAUDE.md.
• ✅ @staticmethod via self — called as KalshiConfig._validate_url(...) to make the absence of instance state obvious.

Skipped

• Nit: explicit comment about NaN-comparison ordering in _map_error. Current order is already correct (bot agreed); a comment about Python nan < 0 semantics doesn't pull its weight given not math.isfinite(...) is short-circuited first.

uv run pytest tests/ --ignore=tests/integration -q → 1437 passed, 48 skipped. ruff + mypy --strict clean.

[claude]

Code Review — PR #112

Three focused correctness fixes with clear motivation, well-isolated commits, and solid regression coverage. Mostly approval-ready; a few items below worth addressing before merge.

---

Overview

Fix	File(s) changed	Verdict
Order.type → Order.order_type (#91)	models/orders.py	✅ Correct approach
KALSHI_API_BASE_URL scheme/host validation (#94)	config.py	✅ Well-scoped
Retry-After NaN/negative/inf guard (#96)	_base_client.py	✅ Clean fix

---

Issues

1. CHANGELOG missing ### Fixed entries for the three other fixes

[Unreleased] only documents the breaking Order.type rename under ### Breaking. The other three fixes represent observable behavioral changes and should each get a line under ### Fixed:

• Bool param serialization: False now serializes as "false" instead of being silently dropped (affects events.list(with_nested_markets=False), series.list(include_volume=False), etc.). Even though this is the correct intent, it's a behavioral delta that downstream code relying on the old drop-silently behavior would notice.
• Retry-After: 0: The is not None fix means a Retry-After: 0 header now causes an immediate retry instead of falling back to computed backoff.
• Base URL validation: New ValueError at config-construction time for bad schemes/plaintext-to-remote.

2. Order.order_type has no serialization_alias

order_type: str | None = Field(
    default=None,
    validation_alias=AliasChoices("type", "order_type"),
    # no serialization_alias
)

Order is a response model so round-tripping to wire format is not a primary use case. However, any caller doing order.model_dump(by_alias=True) expecting spec-conformant JSON will get order_type instead of type. The existing pattern elsewhere in the SDK (e.g. yes_price with validation_alias but no serialization_alias) is consistent, so this is low priority — but worth a comment in the PR description so consumers know the caveat.

3. Multi-line comment on order_type violates project style

CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."

# Spec field is named ``type`` (enum: limit, market). Renamed to
# ``order_type`` on the SDK side to avoid shadowing the Python builtin —
# same rationale as milestone_type / target_type / incentive_type
# elsewhere. The wire still sends ``type``; validation alias accepts both.

This is a 4-line block. Trim to one line, e.g.:

# Renamed from ``type`` to avoid builtin shadow; wire still uses ``type`` via validation_alias.

4. Async Retry-After: 0 test patches asyncio.sleep globally

monkeypatch.setattr("asyncio.sleep", fake_sleep)

The sync counterpart patches "kalshi._base_client.time.sleep" — more targeted, less likely to interfere with pytest-asyncio internals. If _base_client.py imports asyncio at module level, prefer monkeypatch.setattr("kalshi._base_client.asyncio.sleep", fake_sleep) for consistency and isolation. The current form works (monkeypatch reverts after the test), but it's fragile if the import site changes.

---

Positive notes

• _bool_param helper cleanly eliminates 8 identical inline ternaries. The docstring explaining the tri-state semantics is exactly the right level of detail.
• _validate_url error messages are actionable — they name the security risk ("would expose the KALSHI-ACCESS-KEY header") rather than just saying "invalid".
• Warn-not-error for unknown hosts is the right policy: blocks credential leakage by accident while keeping proxy/staging setups working.
• Test coverage: negative, NaN, inf, zero, HTTP-date format, and end-to-end sleep assertion — comprehensive without being bloated.
• is not None instead of truthiness for retry_after is exactly the right fix; 0 is a semantically valid "retry immediately" value.
• CHANGELOG migration example with before/after code snippet is user-friendly.

---

Summary

Fix items 1 (CHANGELOG ### Fixed entries) and 3 (comment length) before merging — both are straightforward edits. Items 2 and 4 are low-priority callouts that can be tracked as follow-up if preferred. The core logic in all three fixes is correct.

[claude]

Code Review

Overall: Well-structured PR. Three independent correctness fixes, each with clear rationale, regression tests, and CHANGELOG entries. The bundled approach is appropriate given the fixes are unrelated and small.

---

Fix #91 — Order.type → Order.order_type

Looks correct.

• validation_alias=AliasChoices("type", "order_type") correctly handles both the wire name and the SDK name for deserialization.
• Order.model_config already has populate_by_name=True, so test_order_type_accepts_python_name will pass as-is — no additional config needed.
• Order is a response model; nothing serializes it back to the wire, so the absence of a serialization_alias is fine. One edge case to keep in mind: if a caller ever calls order.model_dump(by_alias=True), Pydantic v2 will use the first alias ("type") as the output key, which actually matches the wire — so it's harmless.

One concern: This is a hard break with no deprecation period. The CHANGELOG migration guide is clear, but there's no @property type shim to give a DeprecationWarning. That's consistent with how the project has handled other renames (milestone_type, etc.), so I'm not blocking on it — just flagging that anyone doing order.type will get an AttributeError at runtime, not a helpful deprecation message.

---

Fix #94 — KALSHI_API_BASE_URL validation

Looks correct. Good security fix.

• IPv6 handling is correct: urlparse("http://[::1]:8000/...") sets .hostname = "::1" (brackets stripped), and "::1" is in _LOCAL_HOSTS. The test [::1] URL confirms this.
• The warn-not-reject path for unknown HTTPS hosts is the right call — it keeps proxies and staging environments working.
• _validate_url is a clean staticmethod; easy to unit-test and reason about.

Minor nit — warning verbosity at construction time: logger.warning(...) fires every time KalshiConfig is instantiated with an unknown host (e.g., a test fixture that creates configs in a loop). This is not a bug, but if the warning becomes noise in a high-churn test suite, a warnings.warn(..., stacklevel=2) with a UserWarning would be filterable via -W flags. Low priority.

Minor nit — shared _KNOWN_HOSTS for HTTP and WS: The same frozenset is used for both URL families. Since Kalshi's HTTP and WS endpoints share hostnames, this is correct. It might be worth a comment noting that intentionally, since future readers might expect separate sets.

---

Fix #96 — Retry-After NaN/negative/∞ guard

Looks correct and complete.

• math.isfinite elegantly handles NaN, +inf, and -inf in a single expression.
• NaN comparison quirk is handled correctly: float("nan") < 0 is False in Python (all NaN comparisons return False), so the not math.isfinite(...) branch correctly catches it.
• The is not None fix for Retry-After: 0 ("retry immediately") is subtle but important — the old truthy check silently dropped it.
• The async lazy import asyncio at the method level (line 238 of _base_client.py) justifies patching asyncio.sleep globally in the async test rather than via the module attribute. The comment in the test explains this clearly.

Minor comment accuracy: The inline comment says "NaN crashes time.sleep" — more precisely, CPython raises ValueError: Invalid value NaN for sleep interval. "Crashes" is colloquial but could mislead a reader into thinking it's a segfault. Consider: # NaN raises ValueError in time.sleep; negative busy-loops or sleeps 0.

---

Testing

+22 regression tests, all targeted at the three issues. A few observations:

1. _bool_param — only sync tested for events/series. TestBoolParamSerialization and TestSeriesBoolParamSerialization cover EventsResource and SeriesResource (sync). The async equivalents (AsyncEventsResource, AsyncSeriesResource) share the same _bool_param call via the same param-builder functions, so a bug in those functions would be caught by the sync tests. Not blocking, but worth noting.

2. Missing True case for _bool_param regression. The new tests only assert that False emits "false". A companion assertion that True still emits "true" (not a regression from the old "true" if x else None) would complete the story. Low priority since _bool_param is directly unit-testable via _base.py.

3. Retry-After 0 end-to-end tests (sync + async) are the right level of test for the is not None fix — they verify the transport actually sleeps 0 rather than falling back to computed backoff.

---

Summary

Area	Status
Order.order_type rename correctness	✅
Breaking change documented	✅
URL validation security logic	✅
Retry-After guard correctness	✅
is not None fix for Retry-After: 0	✅
Test coverage	✅ (minor async gap, non-blocking)
CHANGELOG	✅

No blockers. The minor nits above are purely polish-level. Ship when the v1.2 vs v2.0 version decision is made.