fix(orders): require count+action on create() kwarg path; reject bool in buy_max_cost

[TexasCoding]

Closes #242
Closes #243

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

[claude]

Code Review — PR #281

Overview

This PR closes two money-risk issues: it removes the silent count=1 default from CreateOrderRequest (#242) and rejects bool inputs on buy_max_cost (#243). Both are safety-correct changes — a missing count or action arg previously placed a real 1-contract BUY on the wire rather than raising early.

---

What's good

• Surgical scope. The diff touches only the affected model field, the kwarg-path guard in _build_create_order_body, and the existing validator rename. No unrelated cleanup.
• Two-layer defense is correct. The kwarg path raises TypeError before model construction; the model itself raises ValidationError when built directly via CreateOrderRequest(...). Both paths are tested independently (test_missing_count_and_action_raises_before_http, test_request_overload_unaffected_by_242).
• Bool check placement. Checking isinstance(v, bool) before isinstance(v, Decimal) is correct — bool is a subclass of int, not Decimal, so it would have slipped past the existing guard.
• Test coverage. Three TypeError cases (both missing / only action / only count), plus regression happy-path, request-overload path, and both True/False for buy_max_cost.
• test_buy_max_cost_accepts_plain_int including assert not isinstance(req.buy_max_cost, bool) is a nice guard against Pydantic coercing the stored value back into a bool.

---

Issues / suggestions

1. Missing breaking label

CreateOrderRequest(ticker=..., side=..., action="buy") (no count) was valid before this PR and is now a ValidationError. That's a breaking change. The repo's label set includes breaking — it should be applied so the CHANGELOG entry and version bump land in the right tier.

2. TypeError regex is fragile (minor)

with pytest.raises(TypeError, match=r"count.*action"):

This passes today because the message reads "…requires \ticker`, `side`, `count`, and `action`…"`. If the message is ever reordered or reformatted, the regex silently stops testing what it claims to. Two alternatives:

# Option A — test both words independently
with pytest.raises(TypeError) as exc_info:
    client.orders.create(ticker="X", side="yes")
msg = str(exc_info.value)
assert "count" in msg and "action" in msg

# Option B — anchor on the actual required-field clause
match=r"`count`, and `action`"

3. Verbose historical context in error messages

The TypeError body includes issue-number history:

"Pre-#242 the SDK silently defaulted missing count to 1 contract and missing action to "buy" — that has been removed: a missing arg would otherwise translate into a real 1-contract BUY on the wire."

This is helpful to SDK developers reading the code, but end users of the SDK (who read TypeError tracebacks) don't need the issue number. Consider moving the Pre-#242… sentence into an inline comment or the docstring, and keeping the error message to the actionable part:

raise TypeError(
    "create() requires `ticker`, `side`, `count`, and `action` "
    "(or pass `request=...`). A missing `count` or `action` would "
    "otherwise place a real order on the wire."
)

4. Missing bool rejection for count (question, not a blocker)

The PR description references #225 as having closed bool-rejection for DollarDecimal / FixedPointCount. Confirm that FixedPointCount already rejects bool inputs — if it does, there's nothing to do here; if it doesn't, count=True (→ 1 contract) is the same class of bug as buy_max_cost=True (→ 1-cent cap). This is likely already handled, but worth verifying explicitly.

---

No concerns

• to_decimal(count) call after the None check is safe.
• The existing action silent-default removal (action if action is not None else "buy" → action) is clean.
• All updated tests properly pass both count and action explicitly.
• The async and sync resource changes are symmetric and correct.

---

Summary

The logic is correct and the test coverage is solid. Three items before merge:

1. Apply the breaking label (required for semver tracking).
2. Harden the TypeError regex in the two new test_missing_count_and_action_raises_before_http tests.
3. Confirm FixedPointCount already rejects bool (or open a follow-up if it doesn't).

The money-risk motivation is sound and the changes are well-scoped. Approve pending the label and optional test hardening.

[claude]

Code Review — PR #281

Overview: This PR hardens two money-risk vectors on CreateOrderRequest:

1. orders.create() silently defaults count=1 and action="buy" on the kwarg path #242 — count loses its silent Decimal("1") default; action loses its silent "buy" default. Both are now required on the kwarg path, raising TypeError before any HTTP request.
2. Orders: CreateOrderRequest.buy_max_cost validator accepts bool — silently becomes 1 cent #243 — buy_max_cost rejects bool inputs (Python's bool is an int subclass, so True would otherwise slip through as 1 cent).

The intent is exactly right and the structural approach is sound. A few things worth tightening before merge:

---

Bugs / Correctness

None found. The guard if ticker is None or side is None or count is None or action is None correctly gates the kwarg path, and the model-level change (count: FixedPointCount = Field(serialization_alias="count_fp") with no default=) enforces it on the request= overload path too. Both paths are independently tested.

---

Historical context belongs in git history, not in code

Per the project's own CLAUDE.md:

"Don't reference the current task, fix, or callers… since those belong in the PR description and rot as the codebase evolves."

Several places violate this:

kalshi/models/orders.py — class docstring:

Pre-v2.3.0 the SDK defaulted ``action`` to ``"buy"`` (#172); the
follow-up (#242) also removed the silent ``count=1`` default — both are
money-risk drivers, since a missing arg would otherwise translate into
a real 1-contract BUY rather than a clear error.

Issue numbers and "previously the SDK did X" history belongs in the commit message, not the docstring. The docstring should state what is currently required.

kalshi/resources/orders.py — create() docstring (both sync and async):

#242 (v2.5): on the kwarg path, ``count`` and ``action`` are now
REQUIRED… Previously the SDK silently defaulted to ``count=1`` and
``action="buy"``…

Same issue — this is changelog content, not API documentation.

TypeError message in _build_create_order_body:

raise TypeError(
    "create() requires `ticker`, `side`, `count`, and `action` "
    "(or pass `request=...`). Pre-#242 the SDK silently defaulted "
    "missing `count` to 1 contract and missing `action` to \"buy\" — "
    "that has been removed: a missing arg would otherwise translate "
    "into a real 1-contract BUY on the wire."
)

The part after the first sentence is changelog noise at runtime. A caller hitting this error doesn't need to know what the SDK used to do. Trim to:

raise TypeError(
    "create() requires `ticker`, `side`, `count`, and `action` "
    "(or pass `request=...`)"
)

Validator docstring and inline test comment:

"""Reject Decimal, float, and bool inputs on buy_max_cost.
… Bool is an ``int`` subclass, so ``True`` would otherwise
slip through as 1 (= 1 cent cap), the same class of bug #225
closed for ``DollarDecimal`` / ``FixedPointCount``. Reject at
the boundary (#243).

# #242: `count` no longer defaults to Decimal("1") — it is required.
# A missing-arg bug would otherwise silently become a 1-contract BUY.

#225, #243, #242 are private issue references that decay as soon as the ticket is closed. Drop them; the WHY ("bool is an int subclass; True would be a 1-cent cap") is worth keeping as a one-liner.

---

Minor

match=r"count.*action" is fragile: If the error message word order ever changes, this silently passes even when the message is wrong. Consider match=r"count.*action|action.*count" or match the exact required-arg substring.

Imports inside test body (test_create_order_count_no_default): import pytest and from pydantic import ValidationError are inside the function body. Other tests in the file do this too, so it's a pre-existing style inconsistency — not introduced here, but worth noting if you clean up later.

---

Test coverage

Coverage is good: missing-both, missing-count-only, missing-action-only, regression (explicit args work), request= overload unaffected, bool rejected, bool rejected for False as well as True, plain int still accepted. No gaps spotted.

---

Summary

The logic is correct and the safety intent is the right call for a financial API. Main ask before merge: trim the historical/issue-reference prose out of the TypeError message, the create() docstrings, and the class docstring. That content is already captured in this PR description and the linked issues.