fix(models): AmendOrderRequest Literal narrowing + V1 ge=0 strictness + create() overload fence

[TexasCoding]

Summary

Three model-layer integrity fixes that close drift between V1 and V2 order
request schemas and tighten the public orders.create() overload so the
static contract matches the runtime guard. The headline behavioral break
mirrors v2.5 #270 lineage: AmendOrderRequest.side / .action no longer
accept arbitrary strings.

Issues closed

• Closes AmendOrderRequest.side/.action still bare str after v2.5 narrowed CreateOrderRequest #312 — AmendOrderRequest.side/.action narrowed to
  SideLiteral / ActionLiteral, matching the v2.5 Polish bundle: model small items (AwareDatetime in WS, V1 Literal, default=None, timestamp typing, Decimal NaN) #270 narrowing on
  CreateOrderRequest. The stale module-level "leave as str to remain
  tolerant of spec drift" comment is removed; both V1 and V2 create paths
  (and now amend) are narrowed.
• Closes V1 order request models miss ge=0 on subaccount/exchange_index that V2 and other models enforce #326 — V1 CreateOrderRequest, AmendOrderRequest,
  DecreaseOrderRequest, and BatchCancelOrdersRequestOrder gain
  Field(default=None, ge=0) on subaccount and exchange_index. Sweeps
  the two V2 exchange_index slots missed by bug: int-typed request fields silently coerce bool (subaccount routing, transfers, counts) #295
  (CreateOrderV2Request, BatchCancelOrdersV2RequestOrder) for parity
  with subaccount, communications, order_groups, and the
  subaccount-transfer body.
• Closes orders.create type overload still permits omitting required action/count #350 — OrdersResource.create / AsyncOrdersResource.create
  kwarg overloads require action: ActionLiteral and count: int
  (no None, no default). Runtime requirement has been there since
  v2.5 orders.create() silently defaults count=1 and action="buy" on the kwarg path #242; this lines up the type-system contract so mypy --strict
  rejects the missing-arg shape that previously deferred to a runtime
  TypeError.

Behavioral changes

• AmendOrderRequest.side / .action now reject non-Literal strings
  (AmendOrderRequest.side/.action still bare str after v2.5 narrowed CreateOrderRequest #312). A typo like side="yess" raises ValidationError at
  construction time instead of being signed by the SDK and 400'd by the
  server. This is the same fail-fast story v2.5 Polish bundle: model small items (AwareDatetime in WS, V1 Literal, default=None, timestamp typing, Decimal NaN) #270 told for
  CreateOrderRequest; minimal surprise expected (no caller intentionally
  amends with a non-spec value), but it is a strict tightening.
• V1 subaccount / exchange_index reject negatives (V1 order request models miss ge=0 on subaccount/exchange_index that V2 and other models enforce #326).
  Previously bare StrictInt | None; now Field(default=None, ge=0).
  Mirrors V2 + every other request model in the SDK.
• orders.create(...) kwarg overload requires action and count
  (orders.create type overload still permits omitting required action/count #350). Static-only change: mypy --strict refuses
  client.orders.create(ticker="T", side="yes") against the kwarg form.
  The request=CreateOrderRequest(...) overload is unaffected.

Tests

• tests/test_models.py::TestIssue312AmendOrderRequestLiteralNarrowing —
  invalid-side / invalid-action rejected; valid Literal values accepted.
• tests/test_models.py::TestIssue326V1SubaccountGeZero — parametrized
  across all 10 newly-constrained fields (8 V1 + 2 V2-exchange_index
  slots): negative values raise ValidationError, zero remains valid
  (primary subaccount / first shard).
• tests/test_client.py::TestIssue350OrdersCreateOverloadRequiresActionCount
  — runtime TypeError when action or count is omitted; explicit
  # type: ignore[call-overload] markers also act as the static fence
  (under warn_unused_ignores/--strict these markers would themselves
  fail if the overload were re-loosened).
• Existing resource.create(ticker="TEST", side="yes") callsites in
  tests/test_client.py and tests/test_async_client.py (the auth-
  required guards) updated to satisfy the tightened overload.

EXCLUSIONS / contract drift

None. The existing tests/_contract_support.py::EXCLUSIONS entries for
AmendOrderRequest (cent-form prices, count/count_fp serialization)
are unrelated to the enum narrowing. test_contracts.py passes unchanged
— the OpenAPI spec defines side/action as enums, so narrowing the
SDK model to Literal[...] moves the SDK closer to the spec, not away.

Source

Round-3 independent audit closure plan, wave W1 (HIGH-severity integrity
fixes), PR W1-D.

[claude]

Code Review — PR #363

Overall: Solid spec-drift closure. The three fixes are well-scoped, the logic is correct, and the parametrized test suite gives good confidence. One linting bug and a few minor notes below.

---

Bug: Missing blank lines before TestIssue350 (will fail ruff)

tests/test_client.py appends the new class directly after the last line of the preceding class with no blank lines:

        assert transport._closed is True   # line 1469
class TestIssue350OrdersCreateOverloadRequiresActionCount:   # line 1470

PEP 8 and ruff E302 require two blank lines before a top-level class definition. CI will flag this unless ruff is not enforcing E302 — but since the project runs ruff check . in CI, this will likely red.

---

Code quality

subaccount: StrictInt | None = Field(default=None, ge=0) — correct but worth a note.
Pydantic v2 only runs the ge=0 numeric validator when the value is not None, so the constraint is correctly applied. This mirrors the existing V2 pattern and is fine.

DecreaseOrderRequest test uses {"reduce_by": 1} as other_kwargs.
The model_validator enforcing XOR runs after field validators in Pydantic v2, so passing subaccount=-1 will trigger the ge=0 ValidationError before the XOR check. Correct order of operations — just worth knowing if the validator ordering ever changes.

test_issue_312_amend_order_request_rejects_invalid_side passes count=Decimal(1).
count is a valid optional field on AmendOrderRequest (FixedPointCount | None), so this doesn't accidentally pass via an extra-field forbid error. The ValidationError fires from the side constraint as intended. Fine as-is, though passing count here adds irrelevant noise to an invalid_side test. Consider dropping it.

---

Overload correctness

The kwarg overload tightening (action: ActionLiteral, count: int — no defaults) against the concrete implementation (action: ActionLiteral | None = None, count: int | None = None) is the correct pattern for Python overloads. The # type: ignore[call-overload] regression-guard pattern in the tests is clever — warn_unused_ignores under --strict will catch any re-loosening.

---

Test coverage

• TestIssue312: three cases (reject bad side, reject bad action, accept valid literals).
• TestIssue326: 10-case parametrize across all affected models. Good sweep.
• TestIssue350: runtime TypeError cases covered; the static fence is self-documenting via the ignore markers.
• Async mirror updated in test_async_client.py.

One gap: TestIssue350 doesn't have a matching test in test_async_client.py for AsyncOrdersResource. Not a blocker (the overload narrowing is symmetric), but parity would be consistent with how the rest of the test suite is structured.

---

Summary

Severity	Item
Must fix	Missing two blank lines before TestIssue350 in test_client.py (ruff E302)
Nit	count=Decimal(1) noise in test_issue_312_amend_order_request_rejects_invalid_side
Nit	transport.close() at end of test without try/finally (harmless)
Optional	Add AsyncOrdersResource coverage in test_async_client.py for #350 parity

Fix the blank-line issue and this is good to merge.

[TexasCoding]

Round-2 review addressed in a1c6a21:

• Updated 2 pre-existing amend validation tests to use valid Literal inputs while still asserting the server-400 → KalshiValidationError mapping path — the new client-side Literal guard was catching the previous invalid-side triggers before the mocked 400.

[claude]

Code Review — PR #363

Overview

Three tightly scoped model-layer integrity fixes: Literal narrowing on AmendOrderRequest, ge=0 parity sweep across V1 request models, and static-type alignment of the orders.create() kwarg overload with its existing runtime guard. The scope matches the stated intent, the changes are surgical, and the new test classes directly pin the specific regressions. A few items worth addressing before merge:

---

Issues

1. Missing blank lines before new test class — will fail ruff (E302)

tests/test_client.py appends the new class immediately after the last line of TestSyncTransportLifecycle with no separating blank lines:

        assert transport._closed is True
class TestIssue350OrdersCreateOverloadRequiresActionCount:

PEP 8 (and ruff E302) require two blank lines between top-level class definitions. CI will reject this.

2. Bare dict annotation fails mypy --strict (--disallow-any-generics)

In tests/test_models.py, TestIssue326V1SubaccountGeZero:

def test_issue_326_v1_subaccount_ge_zero(
    self,
    model_path: str,
    field_name: str,
    other_kwargs: dict,   # ← bare generic
) -> None:

--disallow-any-generics (included in --strict) requires explicit type parameters. Use dict[str, object] (or dict[str, Any] with an Any import). CLAUDE.md requires mypy strict to pass before every commit.

3. pydantic.ValidationError imported inside individual test methods

TestIssue312AmendOrderRequestLiteralNarrowing imports pydantic.ValidationError separately inside two of its three methods, while the third method (and the rest of the test file) imports at module scope. Hoist the imports to module level or to the class body to stay consistent.

---

Informational

test_amend_validation_error scenario change (both test_orders.py and test_async_orders.py): The change from side="invalid"/"bad" to side="yes" is required — with Literal narrowing, side="invalid" now raises pydantic.ValidationError at construction time, which is not KalshiValidationError, so the pytest.raises(KalshiValidationError) wrapper would error. The fix is correct; the "invalid side → 400" path is now owned by TestIssue312AmendOrderRequestLiteralNarrowing. The test name remains accurate (it tests HTTP 400 → KalshiValidationError mapping), no action needed — just flagging for reviewers' awareness.

---

Verdict

The behavioral fixes are correct, spec-aligned, and appropriately fail-fast. Fix items 1 and 2 (both are CI-blocking under ruff and mypy --strict) and address item 3 for consistency, then this is good to merge.

[TexasCoding]

Round-3 review addressed in 26ccd32:

• Hoisted pydantic.ValidationError import to module scope in test_models.py.
• Typed bare dict annotation as dict[str, object] for mypy --disallow-any-generics consistency.
• Added missing PEP 8 blank lines between new test classes.

[claude]

Code Review — PR #363

Overview

Three focused, spec-aligned model hardening fixes. The scope is well-defined, the changes are surgical, and the test coverage is thorough. No architectural concerns.

---

What the PR does

Issue	Fix
#312	AmendOrderRequest.side/.action narrowed from str → SideLiteral/ActionLiteral
#326	subaccount/exchange_index on V1 models (and two missed V2 slots) gain Field(default=None, ge=0)
#350	orders.create() kwarg overload drops the `

---

Code Quality

Positive:

• Changes are minimal and directly traceable to the three issues — no scope creep.
• AmendOrderRequest narrowing mirrors the identical fix already on CreateOrderRequest (Polish bundle: model small items (AwareDatetime in WS, V1 Literal, default=None, timestamp typing, Decimal NaN) #270), creating a consistent pattern across both request types.
• ge=0 sweep correctly catches two V2 exchange_index slots missed by the original bug: int-typed request fields silently coerce bool (subaccount routing, transfers, counts) #295 pass (CreateOrderV2Request, BatchCancelOrdersV2RequestOrder).
• The comment update in orders.py is accurate — it removes the outdated "leave as str to remain tolerant of spec drift" rationale, which no longer applies after V1 and V2 create paths were both narrowed.

Issues found:

1. Redundant inner import of ValidationError (minor)

In tests/test_models.py, from pydantic import ValidationError is added at module level (good), but then re-imported inside test_issue_326_v1_subaccount_ge_zero:

def test_issue_326_v1_subaccount_ge_zero(...) -> None:
    import importlib
    from pydantic import ValidationError   # ← redundant; already at module level

The inner import is harmless but inconsistent — the module-level import is sufficient.

2. Test intent drift in test_amend_validation_error (observation, not a bug)

The original tests in test_orders.py and test_async_orders.py used side="invalid" to probe the 400-path, relying on a mocked HTTP response to trigger KalshiValidationError. The update to side="yes" is correct — with SideLiteral narrowing, the old side="invalid" would now raise a Pydantic ValidationError before the HTTP call, so the mocked 400 would never fire. The fix is right. Worth noting that these tests no longer exercise the "server rejects an amend" path through a side-validation lens, but they do still cover the 400 → KalshiValidationError mapping, which is the point.

---

Type Safety

The StrictInt | None = Field(default=None, ge=0) combination is correct. Pydantic v2 evaluates ge=0 only against the integer branch of the union; None passes through the | None union member without hitting the constraint. No false positives.

The overload change (dropping | None = ... on action and count) correctly aligns the static contract with the runtime guard that has been in place since #242. The # type: ignore[call-overload] markers in the new test serve double duty as a regression fence under warn_unused_ignores — a clever pattern, though the comment explaining it could be condensed.

---

Test Coverage

• AmendOrderRequest.side/.action still bare str after v2.5 narrowed CreateOrderRequest #312: Happy path (side="no", action="sell") + two rejection cases. Solid.
• V1 order request models miss ge=0 on subaccount/exchange_index that V2 and other models enforce #326: Parametrized across all 10 newly-constrained fields. Each case checks both the negative rejection and zero validity. Complete.
• orders.create type overload still permits omitting required action/count #350: Runtime TypeError for missing action and missing count are both exercised. The auth-required call sites in test_client.py / test_async_client.py are updated to satisfy the tightened overload.

---

Summary

This is a clean, well-scoped PR. The one actionable nit is the redundant from pydantic import ValidationError inside the test method body — worth removing before merge to keep the import style consistent. Everything else looks correct and ready.

[TexasCoding]

Round-4 review addressed in 32eec67: removed redundant inline ValidationError import in TestIssue326V1SubaccountGeZero (already at module scope per round-3).

[claude]

Code Review — PR #363

Overview

Three well-scoped model-layer integrity fixes: AmendOrderRequest Literal narrowing (#312), ge=0 sweep for V1 + missed V2 exchange_index slots (#326), and orders.create() kwarg overload tightening (#350). The changes are consistent with prior art in this SDK (V2 models in #270/#295) and the test coverage is solid.

---

What's Good

• Literal narrowing on AmendOrderRequest is the right call. The old "leave as str to remain tolerant of spec drift" comment was misleading — it deferred spec violations to a server round-trip at real cost, and the pattern was already abandoned for CreateOrderRequest.
• ge=0 sweep correctly brings V1 models to parity with V2 and catches two missed exchange_index slots in CreateOrderV2Request / BatchCancelOrdersV2RequestOrder.
• Overload tightening precisely matches the runtime guard added in orders.create() silently defaults count=1 and action="buy" on the kwarg path #242. The static contract now tells mypy what the runtime already enforces.
• StrictInt | None = Field(default=None, ge=0) composition is correct in Pydantic v2 — the BeforeValidator in StrictInt rejects booleans before the ge=0 constraint runs. No ordering issue.
• The parametrized TestIssue326V1SubaccountGeZero test covering all 10 changed fields is efficient and thorough.

---

Issues

1. Missing async runtime test for #350 (minor gap)

TestIssue350OrdersCreateOverloadRequiresActionCount only tests OrdersResource. The async overload (AsyncOrdersResource.create) was tightened identically (lines 897–918), but there's no async TypeError test. The runtime guard itself lives in the shared _build_create_order_body, so behavior is correct — but the overload change is unverified at the test boundary for the async client.

The fix in tests/test_async_client.py (line 476) only updates the auth-required guard; it doesn't add a parallel TypeError test. A minimal addition to TestIssue350:

@pytest.mark.asyncio
async def test_issue_350_async_orders_create_overload_requires_action_count(
    self, test_auth: KalshiAuth
) -> None:
    ...
    with pytest.raises(TypeError, match=r"action"):
        await resource.create(  # type: ignore[call-overload]
            ticker="TEST",
            side="yes",
            count=1,
        )

2. count=Decimal(1) in rejection tests is unnecessary (style nit)

In test_issue_312_amend_order_request_rejects_invalid_side and test_issue_312_amend_order_request_rejects_invalid_action, count=Decimal(1) is passed — but count is FixedPointCount | None = None in AmendOrderRequest, so it's optional. The happy-path test correctly omits it. The inconsistency is minor but the failure tests would read cleaner without the spurious kwarg.

3. Shifted semantics in test_amend_validation_error (observation, not a bug)

The old test asserted that side="bad" (an invalid string) eventually surfaced as KalshiValidationError. Now that the model validates side at construction, the test was correctly updated to use side="yes" + mocked 400 to keep the KalshiValidationError assertion alive. This is fine — but the test no longer demonstrates the "bad side" behavior at the resource level. That gap is covered by the new model-level TestIssue312 tests, so this is acceptable as-is.

4. Multi-paragraph docstring in test class (CLAUDE.md violation, minor)

TestIssue350OrdersCreateOverloadRequiresActionCount has a 9-line class docstring. CLAUDE.md says "Don't write multi-paragraph docstrings or multi-line comment blocks — one short line max." The warn_unused_ignores invariant comment embedded in the method body is also longer than necessary. Worth trimming, though the content is accurate.

---

Summary

The core model changes are correct and complete. The only meaningful gap is the missing async TypeError test for the AsyncOrdersResource.create overload — everything else is style-level. This is otherwise a tight, well-scoped PR that advances spec alignment without touching unrelated code.