[TexasCoding]

Wave 1-B of the audit umbrella. 3 issues; resources API hardening. Contains a breaking change (#194) — folded into v2.4.0 per maintainer decision.

Changes

• Orders: V1 batch_create / batch_cancel must return typed per-leg responses (partial-success defect) #194 [breaking] orders.batch_create now returns BatchCreateOrdersResponse (was list[Order]); orders.batch_cancel now returns BatchCancelOrdersResponse (was None). Mirrors V2 shape. Fixes partial-success crash and exposes per-leg reduced_by_fp.
• Resources: URL-encode user-supplied path segments (urllib.parse.quote) #211 New _seg() helper URL-encodes user-supplied path segments and rejects empty / whitespace / . / ... Applied to ~40 sites across all resources.
• Resources: validate "limit" ranges client-side per spec bounds #214 New _validate_limit() helper enforces spec-correct bounds per endpoint. Applied with ranges sourced from specs/openapi.yaml.

Tests

8 new test classes; 658 tests pass across the in-scope test files; ruff + mypy --strict clean across 22 changed files.

Closes #194, Closes #211, Closes #214
Refs #224

[claude]

Code Review — PR #229

Wave 1-B audit hardening: batch typed responses, URL path encoding, limit validation

Overall this is a solid, well-scoped PR. The three fixes each have clear motivation, the implementation is clean, and test coverage is genuinely thorough. A few items below warrant attention before merge.

---

Issues

1. Inconsistent _validate_limit() return-value usage

Some callers rebind the result, others discard it:

# Pattern A — rebind (orders.py, markets.py, historical.py, etc.)
limit = _validate_limit(limit, hi=1000)
return _params(limit=limit, ...)

# Pattern B — discard (incentive_programs.py, milestones.py, portfolio.py, subaccounts.py, structured_targets.py)
_validate_limit(limit, hi=10000)
return _params(limit=limit, ...)  # limit unchanged either way

Both are functionally correct today (the function raises on violation and returns the value unchanged on success), but Pattern B looks like the return value was accidentally ignored. A future maintainer who refactors _validate_limit to clamp rather than reject would silently break limit enforcement in Pattern B sites. Standardise on Pattern A (rebind) or annotate Pattern B sites with a comment like # raises ValueError on violation.

---

2. Async batch_cancel docstring is thin on the 204-raises contract

The sync batch_cancel docstring documents the new 204 behaviour. The async version was reduced to "Batch-cancel orders. See OrdersResource.batch_cancel." — callers reading only the async docs won't see the raises note unless they follow the cross-reference. One extra sentence would close the gap: "Raises KalshiError if the server returns 204 No Content."

---

3. No CHANGELOG.md update visible in the diff

The project updates CHANGELOG.md per release. This PR lands a documented breaking change (#194) targeted at v2.4.0. The changelog entry should appear before or with this PR so the release commit has it.

---

4. _contract_map.py — verify no new entries needed

BatchCreateOrdersResponse and BatchCancelOrdersResponse are new hand-crafted response models. CLAUDE.md says: "If the resource has generated OpenAPI counterparts, register them in _contract_map.py." If kalshi/_generated/ has spec-generated equivalents for these shapes, they should be mapped. If not (likely, since these are V1 envelope shapes), a one-line comment noting the intentional omission would save the next contributor from checking.

---

5. _seg() docstring — note it is a no-op for well-formed IDs

The encoding is sound: urllib.parse.quote(..., safe="") handles traversal strings like "/../admin" → "%2F..%2Fadmin", and the "." / ".." check catches the exact-component case. Worth adding to the docstring: "For well-formed Kalshi identifiers (uppercase alphanumeric + hyphens) this is a no-op; encoding only fires on adversarial input." Reviewers unfamiliar with the Kalshi ID character set might worry it could corrupt valid tickers.

---

Nits

• Extra blank line in models/orders.py after the new BatchCancelOrdersResponse class — looks like 3 blank lines before AmendOrderResponse rather than 2. If ruff passes clean, the project config allows it; otherwise a quick fix.
• Inline imports inside test methods (from kalshi.models.orders import BatchCreateOrdersResponse inside test bodies). Functionally fine but inconsistent with the top-of-file import style in the same test files.

---

What is done well

• _seg() is minimal and focused with a clear security rationale in the docstring.
• _validate_limit() correctly short-circuits on None (no validation for omitted limits).
• New response models use extra="allow" (correct for responses) while request models keep extra="forbid" (correct per project convention).
• Partial-failure regression tests directly reproduce the pre-fix crash scenario — exactly the right way to document a bug fix.
• Boundary tests for _validate_limit cover lo, hi, and both endpoints simultaneously.
• Sync and async paths receive symmetric test coverage throughout.

[claude]

Code Review — PR #229: Batch typed responses, URL-encoded paths, client-side limit validation

Overall this is a well-structured PR. Three focused changes, each tied to a tracked issue, with tests for every new code path. Some observations below, ranging from one real concern to a handful of nits.

---

Issue 1 — batch_cancel raises KalshiError on 204: too aggressive (potential future breakage)

data = self._delete_with_body("/portfolio/orders/batched", json=body)
if data is None:
    raise KalshiError("Expected BatchCancelOrdersResponse body, got 204 No Content.")

The docstring says "every modern Kalshi environment returns 200 with the typed body" — but baking a hard exception into the SDK means any deployment (staging, future version, etc.) that returns 204 will silently break callers. A safer option that still exposes the typed response would be to return BatchCancelOrdersResponse(orders=[]) on 204, which is semantically correct (no per-leg info available) and doesn't turn an empty-but-valid response into an exception. If the API contract truly guarantees a body, a log warning is less disruptive than an error. The test test_batch_cancel_raises_on_204 would need updating, but this is worth considering before merging a breaking behaviour into a breaking release.

---

Issue 2 — error: dict[str, object] | None loses all type safety for consumers

Both BatchCreateOrdersResponseEntry and BatchCancelOrdersResponseEntry use:

error: dict[str, object] | None = None

The spec appears to have at least code and message as well-known fields. Even a minimal TypedDict (or a small BatchOrderError Pydantic model) would let callers do entry.error.code instead of entry.error["code"] and would self-document the shape. Given this is a breaking change anyway, it's the right moment to get the type right.

---

Issue 3 — Inconsistent usage of _validate_limit return value

_validate_limit returns the validated value, but the call-sites are inconsistent:

# events.py, markets.py, orders.py — reassigns:
limit = _validate_limit(limit, hi=200)
return _params(limit=limit, ...)

# incentive_programs.py, milestones.py, portfolio.py — discards return:
_validate_limit(limit, hi=10000)
params = _params(..., limit=limit, ...)

Both patterns are functionally correct (the value doesn't change on success), but the inconsistency is confusing. Pick one and apply it uniformly. The limit = ... form is self-explanatory; the discard form requires knowing that _validate_limit returns the original value unchanged.

---

Issue 4 — _seg tests live only inside TestPathSegmentEncoding via orders.get()

There are no unit tests for _seg() itself in _base.py. The existing tests go through the full resource stack (orders.get("/../admin")), which is fine for integration coverage, but it means a bug in _seg would look like a routing bug when debugging. A small direct test of _seg (empty string, whitespace, ., .., a slash, a normal value) in a tests/test_base.py or similar would make the invariants explicit and cheap to maintain.

---

Nit — Double blank line after BatchCancelOrdersResponse

kalshi/models/orders.py line ~176 has two consecutive blank lines between the end of BatchCancelOrdersResponse and class AmendOrderResponse. PEP 8 / project style uses single blank lines between class bodies at module scope.

---

Nit — Multi-line docstrings on response models

CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The new BatchCreateOrdersResponseEntry, BatchCancelOrdersResponseEntry, and their envelope classes all have multi-paragraph docstrings. The version/breaking-change history is valuable, but belongs in CHANGELOG.md and the PR description rather than inline docstrings that will age poorly.

---

Positives worth calling out

• _seg() is clean: a 10-line helper that does exactly one thing and composes without ceremony.
• extra="allow" on response models is correct — response models should tolerate new fields without breaking; request models correctly keep extra="forbid".
• The reduced_by_fp alias (AliasChoices("reduced_by_fp", "reduced_by")) properly handles the wire format variation.
• The partial-failure crash is well-motivated, well-tested, and the fix is minimal.
• _contract_map.py and _body_map registrations are complete, which means the drift tests will catch any future spec deviation.
• # noqa: E501 suppression is placed precisely where the _seg() call pushes a line over the limit, rather than on unrelated lines.