Skip to content

fix(models): reject bool on int request fields (#295) - #305

Merged
TexasCoding merged 2 commits into
mainfrom
fix/295-strict-int
May 22, 2026
Merged

TexasCoding merged 2 commits into
mainfrom
fix/295-strict-int

Conversation

@TexasCoding

@TexasCoding TexasCoding commented May 21, 2026 •

Copy link
Copy Markdown
Owner

Closes #295.

What

Introduces kalshi.types.StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)] and swaps int → StrictInt on every integer field of every Request model that lacks a dedicated validator. bool is an int subclass, so a plain int annotation silently coerces True→1 / False→0. #243 closed this for buy_max_cost; #295 extends the same guard across every money-routing / counting integer.

Affected request models

  • CreateOrderRequest: expiration_ts, subaccount, exchange_index
  • AmendOrderRequest: subaccount, exchange_index
  • DecreaseOrderRequest: reduce_by, reduce_to, subaccount, exchange_index
  • BatchCancelOrdersRequestOrder: subaccount, exchange_index
  • CreateOrderGroupRequest: contracts_limit, subaccount, exchange_index
  • UpdateOrderGroupLimitRequest: contracts_limit
  • CreateRFQRequest: contracts, subaccount
  • ApplySubaccountTransferRequest: from_subaccount, to_subaccount, amount_cents
  • CreateOrderV2Request: expiration_time, subaccount, exchange_index
  • DecreaseOrderV2Request: exchange_index
  • AmendOrderV2Request: exchange_index
  • BatchCancelOrdersV2RequestOrder: subaccount, exchange_index
  • UpdateSubaccountNettingRequest: subaccount_number
  • CreateQuoteRequest: subaccount

Response models (Order, Fill, Position, …) keep plain int — they parse server-emitted ints that are never Python bools.

Tests

New TestStrictIntRejectsBoolOnRequestModels parametrizes 20 (Model, field) rows against both True and False, asserting ValidationError with a "bool" substring match.

Behavioral fence

Callers that today pass True/False for an integer request field were already doing something wrong; they now get a loud ValidationError instead of silent routing. Worth a "Breaking" entry in the v2.6.0 release notes.

Part of the post-v2.5 audit closure (Wave W1).

…ransfers, counts)

Adds StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)] in
kalshi/types.py and swaps int -> StrictInt on every int field of every
Request model that does not already have a dedicated validator. bool
is an int subclass so plain-int annotations silently coerce True->1 /
False->0; #243 fixed this for CreateOrderRequest.buy_max_cost and #295
extends the same guard to the remaining money-routing / counting ints.

Affected request models:
- CreateOrderRequest: expiration_ts, subaccount, exchange_index
- AmendOrderRequest: subaccount, exchange_index
- DecreaseOrderRequest: reduce_by, reduce_to, subaccount, exchange_index
- BatchCancelOrdersRequestOrder: subaccount, exchange_index
- CreateOrderGroupRequest: contracts_limit, subaccount, exchange_index
- UpdateOrderGroupLimitRequest: contracts_limit
- CreateRFQRequest: contracts, subaccount
- ApplySubaccountTransferRequest: from_subaccount, to_subaccount, amount_cents

Response models (Order, Fill, Position, ...) keep plain int — they
parse server-emitted ints which are never Python bools.
buy_max_cost keeps its existing dedicated validator (#243) which also
rejects Decimal/float beyond bool.

Adds parametrized test_strict_int_rejects_bool_on_request_models in
tests/test_models.py covering every (Model, field) pair on True/False.

Closes #295.
@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — PR #305: fix(models): reject bool on int request fields

Overview

Solid, targeted PR. Introduces StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)] and applies it to integer fields on request models where a bool silently coercing to 0/1 would cause financial routing errors. The request-vs-response split is correct, and the parametrized test suite is comprehensive.


Issues

1. Multi-line docstrings violate CLAUDE.md conventions

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

Both additions violate this:

kalshi/types.py — _reject_bool_int (10-line docstring):

def _reject_bool_int(value: object) -> object:
    """Reject ``bool`` on ``int`` fields used by request models.
    ...9 more lines...
    """

kalshi/types.py — StrictInt (4-line comment):

StrictInt = Annotated[int, BeforeValidator(_reject_bool_int)]
"""An ``int`` that rejects ``bool`` to avoid silent True->1 coercion on
money-routing fields ...
... 3 more lines...
"""

Both should be reduced to one short line (or removed entirely where a good name is self-documenting).


2. V2 Request models not covered — gap should be tracked

Several V2 request models still have plain int on subaccount/exchange_index fields, which carry the same financial routing risk:

Model Fields
CreateOrderV2Request (line 478–484) expiration_time, subaccount, exchange_index
DecreaseOrderV2Request (line 523) exchange_index
AmendOrderV2Request (line 563) exchange_index
BatchCancelOrdersV2RequestOrder (lines 625–626) subaccount, exchange_index
UpdateSubaccountNettingRequest subaccount_number
CreateQuoteRequest (line 202) subaccount

The PR description marks this as "Wave W1" which is fine — but if there isn't already a follow-up issue for Wave W2 covering V2 endpoints, one should be opened before this merges so the gap is tracked rather than forgotten.


3. StrictInt not exported from kalshi/__init__.py

NullableList is in __all__; StrictInt is not. Anyone extending the SDK or building on top of it who wants to reference the type is left digging into kalshi.types. Should be added to the exports alongside NullableList:

# kalshi/__init__.py
from kalshi.types import NullableList, StrictInt
...
__all__ = [
    ...
    "NullableList",
    "StrictInt",
    ...
]

Minor observations

  • importlib in test body: importlib.import_module inside each test invocation works, but since these modules are already imported in the same test file, a top-level import is cleaner and consistent with the rest of test_models.py. No functional issue.

  • buy_max_cost left as field_validator: Intentional and correct — that validator also guards against Decimal/float, which StrictInt doesn't cover. Leaving it separate is the right call; just noting the divergence for anyone reading later.


Positive

  • Clean, minimal type alias — no new abstractions, no overengineering.
  • Correct BeforeValidator placement ensures rejection happens before Pydantic's coercion.
  • 20-row parametrized test suite hits every affected model/field combination, including the highest-risk ApplySubaccountTransferRequest surface.
  • Request vs. response separation is precise and well-reasoned.

Summary: Two changes needed before merge — trim the docstrings to one-liners (convention) and export StrictInt. The V2 gap should be tracked as a follow-up issue if not already filed.

…t models

Addresses bot review on #305:

- Trim _reject_bool_int and StrictInt docstrings to 1 line each (terse style).
- Export StrictInt from kalshi/__init__.py (import + __all__).
- Apply StrictInt to V2 request-model int fields:
  - CreateOrderV2Request: expiration_time, subaccount, exchange_index
  - DecreaseOrderV2Request: exchange_index
  - AmendOrderV2Request: exchange_index
  - BatchCancelOrdersV2RequestOrder: subaccount, exchange_index
  - UpdateSubaccountNettingRequest: subaccount_number
  - CreateQuoteRequest: subaccount
- Add 9 parametrize rows to TestStrictIntRejectsBoolOnRequestModels covering each new field.
@TexasCoding

Copy link
Copy Markdown
Owner Author

Round-2 follow-up pushed in c99efda addressing bot review:

  • Trimmed _reject_bool_int and StrictInt docstrings to 1 line each.
  • Exported StrictInt from kalshi/__init__.py (import + __all__).
  • Applied StrictInt to V2 request-model int fields:
    • CreateOrderV2Request: expiration_time, subaccount, exchange_index
    • DecreaseOrderV2Request: exchange_index
    • AmendOrderV2Request: exchange_index
    • BatchCancelOrdersV2RequestOrder: subaccount, exchange_index
    • UpdateSubaccountNettingRequest: subaccount_number
    • CreateQuoteRequest: subaccount
  • Added 9 parametrize rows to TestStrictIntRejectsBoolOnRequestModels covering each new field.

Verified: ruff check, mypy kalshi/, pytest tests/test_models.py::TestStrictIntRejectsBoolOnRequestModels -v (29 passed).

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — Round 2 (post follow-up)

All issues from round 1 have been addressed. Here's the second-pass assessment.


Round-1 Fixes — Verified ✓

Issue Status
Docstrings trimmed to 1-liners ✓ Done — both _reject_bool_int and StrictInt are now single-line
StrictInt exported from __init__.py ✓ Added to import and __all__
V2 request models (CreateOrderV2Request, DecreaseOrderV2Request, AmendOrderV2Request, BatchCancelOrdersV2RequestOrder, UpdateSubaccountNettingRequest, CreateQuoteRequest) ✓ All fields covered; 9 new parametrize rows added (29 total)

Remaining Minor Observations

1. importlib inside test body (style)

The test resolves model classes at call time via importlib.import_module. These modules are already imported at the top of test_models.py; top-level imports are consistent with the rest of the file and avoid per-test-invocation overhead. No functional issue — just a style note for future test additions.

# current (inside test body)
module_name, class_name = model_path.split(":")
model_cls = getattr(importlib.import_module(module_name), class_name)

# consistent with rest of file would be a top-level import + a mapping dict

2. PR description doesn't mention V2 models in "Affected request models"

The round-2 commit extended coverage to V2 endpoints, but the PR description's "Affected request models" section wasn't updated to reflect them. Not a code issue, but the mismatch will make the CHANGELOG entry harder to write accurately. Worth a description edit before merge.

3. buy_max_cost intentionally kept as field_validator

buy_max_cost: int | None = None still uses _reject_decimal_float_and_bool_buy_max_cost from #243. This is correct — that validator guards against Decimal and float inputs too, which StrictInt doesn't cover. No action needed; noting for completeness.


No New Issues

  • BeforeValidator ordering is correct: bool rejection runs before Pydantic's ge/gt constraints.
  • True=1 case exercised in the tests is the critical path (it would have silently passed plain int).
  • Response models (IndexedBalance.exchange_index, Balance, Fill, etc.) correctly stay on plain int.
  • StrictInt = Annotated[int, ...] means mypy sees it as int, preserving type-checker compatibility while enforcing at runtime.

Verdict: Ready to merge with the two minor observations above — fix the PR description and optionally clean up the importlib pattern. Neither is a blocker.

@TexasCoding

Copy link
Copy Markdown
Owner Author

Description updated to list the V2 request models added in c99efda. Skipping the importlib-in-test-body cleanup — the per-call import is intentional so the test resolves model classes by FQN string in the parametrize table (lets us add new rows without growing the top-level import list). Re-review welcome.

@TexasCoding
TexasCoding merged commit f160315 into main May 22, 2026
5 checks passed
@TexasCoding
TexasCoding deleted the fix/295-strict-int branch May 22, 2026 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: int-typed request fields silently coerce bool (subaccount routing, transfers, counts)

1 participant