polish(models): uniform extra="allow" on all response models (#114)

[TexasCoding]

Summary

Audit + fix for #114 — make response models forwards-compatible by default.

Audit findings

Policy	Count	Action
extra="allow"	80	✅ already correct
extra="forbid" (request bodies)	17	✅ keep (per v0.8.0+ convention)
extra="ignore" (implicit Pydantic default)	5	🔧 fixed

The 5 outliers: Page, Orderbook, OrderbookLevel, BidAskDistribution, PriceDistribution. All gained model_config = {"extra": "allow"} (preserving the existing populate_by_name=True where present).

Behavior change

Instances of those 5 classes will now preserve unknown fields on __pydantic_extra__ rather than silently dropping them. Called out in CHANGELOG.md [Unreleased] → Changed.

Drift guard

New tests/test_model_extra_policy.py parametrizes over kalshi.models.__all__:

• 85 response models must have extra="allow" (every BaseModel not ending in Request/RequestOrder)
• 17 request bodies must have extra="forbid"

A future model added with the wrong policy fails at collect time.

Closes #114

Test plan

• Full suite: 1808 passed (was 1705; +103 from the new parametrized tests)
• uv run ruff check . clean
• uv run mypy kalshi/ clean (strict)

[claude]

Code Review — PR #143: polish(models): uniform extra="allow" on all response models

Overview

Clean, well-scoped fix for #114. The 5 outlier response models (Page, Orderbook, OrderbookLevel, BidAskDistribution, PriceDistribution) have been promoted from Pydantic's default extra="ignore" to the project-standard extra="allow". The new drift guard (test_model_extra_policy.py) locks this in going forward. Changes are surgical and consistent with the CLAUDE.md convention ("touch only what you must").

---

Issues / Suggestions

1. model_config placement in Page (minor style)

class Page(BaseModel, Generic[T]):
    items: list[T]
    cursor: str | None = None

    model_config = {"extra": "allow"}   # ← between fields and @property

    @property
    def has_next(self) -> bool:
        ...

Pydantic v2 accepts this, but the rest of the codebase places model_config at the bottom of field declarations (as the other four fixes in this PR do). Placing it after the fields but before the @property creates a minor visual inconsistency. Suggest moving it below cursor and before the blank line that precedes has_next.

---

2. _REQUEST_BODY_SUFFIXES includes "RequestOrder" — purpose unclear (minor)

_REQUEST_BODY_SUFFIXES = ("Request", "RequestOrder")

No model in this SDK currently ends with "RequestOrder" (all 17 request bodies end with "Request"). If "RequestOrder" was added prophylactically for a future model, a short inline comment would make the intent clear. If it's genuinely unused, removing it keeps the heuristic crisp and avoids puzzling future readers.

---

3. Test ids= missing — noisy pytest -v output (nit)

@pytest.mark.parametrize(
    "name,cls",
    [(n, c) for n, c in _exported_model_classes() if not _is_request_body(n)],
)

Without ids=, pytest auto-generates IDs from the class objects, producing output like test_response_models_use_extra_allow[name0-cls0]. Adding ids=[n for n, _ in ...] would give test_response_models_use_extra_allow[Market], making failures immediately legible. Same applies to test_request_bodies_use_extra_forbid.

---

4. Enforcement scope limited to __all__ exports (low risk, worth noting)

_exported_model_classes() iterates kalshi.models.__all__. A response model added to a models/*.py file but not wired into __all__ would silently escape the policy check. Given the project's convention of exporting everything through __init__.py, this is a theoretical gap rather than a practical one — but documenting the assumption in a comment on _exported_model_classes would make the invariant explicit.

---

What's Done Well

• Diff is minimal: 5 two-line additions + 2 line modifications. No scope creep.
• BidAskDistribution / PriceDistribution correctly merge the new key into the existing model_config dict rather than overwriting it — populate_by_name is preserved.
• CHANGELOG entry is precise and correctly placed under [Unreleased] → Changed.
• The parametrized test design is the right approach; 103 new test cases for ~55 lines of test code is a good ratio.
• No security or performance concerns — extra="allow" vs extra="ignore" has negligible overhead and no attack surface.

---

Summary

Approve with minor suggestions. The ids= nit and _REQUEST_BODY_SUFFIXES clarification are the most actionable items; the model_config placement in Page is the smallest but easiest to fix in the same pass.

[claude]

Code Review — PR #143: uniform extra=\"allow\" on all response models

Overview

Clean, well-scoped fix for #114. The 5 outlier response models that were silently dropping unknown fields (Page, Orderbook, OrderbookLevel, BidAskDistribution, PriceDistribution) have been brought into line with the 80 sibling models. The new parametrized drift guard (test_model_extra_policy.py) is a meaningful addition — future model additions will fail immediately at CI if the wrong policy is used.

---

Code quality

✅ Strengths

• Surgical changes only — no adjacent cleanup, exactly what CLAUDE.md calls for.
• Audit table in the PR description makes the scope clear and verifiable.
• Drift guard test is well-structured: parametrizes over __all__, no fixtures needed, failure message includes both the observed and expected value plus the issue number.
• CHANGELOG entry is accurate and readable.

---

Issues

Minor: model_config key ordering is now inconsistent for the two updated models

BidAskDistribution and PriceDistribution end up with {"populate_by_name": True, "extra": "allow"} (new key appended). But Candlestick on line 237 and Market on line 138 use {"extra": "allow", "populate_by_name": True} (extra first).

# After this PR — inconsistent with siblings
model_config = {"populate_by_name": True, "extra": "allow"}  # BidAskDistribution, PriceDistribution

# Pre-existing style in the same file
model_config = {"extra": "allow", "populate_by_name": True}  # Candlestick (line 237), Market (line 138)

Suggestion: flip the key order to match the rest of the file.

---

Observation (non-blocking): naming-based request detection carries a silent assumption

_is_request_body() infers a model's role purely from its name suffix:

_REQUEST_BODY_SUFFIXES = ("Request", "RequestOrder")

This works for every current model, but there is no enforcement that a response model cannot be named FooRequest. A comment explaining that this relies on the project convention documented in CLAUDE.md ("request bodies end in Request/RequestOrder") would make the fragility visible to future editors without adding code.

---

Observation (non-blocking): WS envelope classes are outside the policy scope

kalshi/ws/models/orderbook_delta.py has OrderbookSnapshotMessage and OrderbookDeltaMessage without model_config. The test is correctly scoped to kalshi.models.__all__, so these are out of scope for this PR. Noting it for a potential follow-up: if the same extra="allow" policy should apply to kalshi.ws.models, a separate audit/test would be needed.

---

Nit: _exported_model_classes() is evaluated twice

The function is called once per @pytest.mark.parametrize decorator at module import time. Two calls over ~102 models is inconsequential, but if you ever want to clean it up: assign _ALL_MODELS = _exported_model_classes() at module level and reference it in both parametrize calls.

---

Test coverage

+103 tests from the new parametrized policy check. Happy path and failure path are both implicitly covered (the policy fix is what makes the response-model tests pass). No integration tests are needed here.

---

Summary

Approve with one suggestion: normalize the model_config key order in BidAskDistribution and PriceDistribution to {"extra": "allow", "populate_by_name": True} for consistency with the rest of markets.py. Everything else is correct and well-executed.

[TexasCoding]

Both reviews addressed in latest commit:

From bot #1:

• 🟦 Page.model_config placement — moved adjacent to fields, no longer between fields and @property.
• 🟦 _REQUEST_BODY_SUFFIXES includes "RequestOrder" — clarified. Actually load-bearing: BatchCancelOrdersRequestOrder doesn't end in plain Request. Added an inline comment so the next reader doesn't chase it.
• 🟦 ids= added — pytest -v output now reads [Market] / [CreateRFQRequest] instead of [name0-cls0].
• 🟦 __all__ scope assumption — added to _exported_model_classes() docstring.

From bot #2:

• 🟦 Key ordering in BidAskDistribution / PriceDistribution — flipped to {"extra": ..., "populate_by_name": ...} matching Market and Candlestick in the same file.
• 🟦 _exported_model_classes() evaluated twice — cached at module scope (_ALL_MODELS / _RESPONSE_MODELS / _REQUEST_MODELS).
• ✅ WS envelope classes — acknowledged out of scope per the issue; separate audit if we want the same policy in kalshi/ws/models/.

Verify: tests/test_model_extra_policy.py 102 passed. ruff + mypy --strict clean.

[claude]

Code Review — PR #143: Uniform extra=\"allow\" on response models

Overall: Clean, surgical PR that fixes a real forward-compatibility gap. The drift guard test is the most valuable addition here — it turns a one-time audit into a permanent policy. A few minor items below, none blocking.

---

What the PR Does

Five response models (Page, Orderbook, OrderbookLevel, BidAskDistribution, PriceDistribution) were silently falling through to Pydantic's default extra=\"ignore\" while the 80 sibling models used extra=\"allow\". This PR fixes the inconsistency and adds a parametrized test to enforce the policy on every future model at collection time.

---

Strengths

• Surgical diff. Only the five outlier models are touched; nothing adjacent is disturbed. Exactly right per CLAUDE.md.
• Drift guard is excellent. test_model_extra_policy.py parametrizes over the live kalshi.models.__all__ so any future model added with the wrong policy fails immediately at pytest collect time — no review required to catch it.
• CHANGELOG entry is precise. Names the affected classes, explains the old behavior, and links the issue. Easy to parse when debugging a future regression.
• Error messages in assertions are actionable. They tell you exactly which model is wrong, what value it has, and what it should have — no need to dig into the test to understand a failure.

---

Issues / Suggestions

1. model_config placement in Page is inconsistent (minor style)

# current (diff)
class Page(BaseModel, Generic[T]):
    items: list[T]
    cursor: str | None = None
    model_config = {"extra": "allow"}   # ← between fields and methods

    @property
    def has_next(self) -> bool:
        ...

In BidAskDistribution and PriceDistribution, model_config sits at the bottom of the field block, which is the existing codebase convention. Page puts it between fields and method definitions, which is a bit surprising in a class this long (it has to_dataframe, to_polars, __iter__, __len__, etc. below). The conventional Pydantic placement is at the top of the class, before field definitions — which also avoids mid-class surprises. Suggest moving it to the top or following the bottom-of-fields convention the other models use.

2. Naming heuristic could misclassify future sub-models (low risk, worth a comment)

_REQUEST_BODY_SUFFIXES = ("Request", "RequestOrder")

def _is_request_body(name: str) -> bool:
    return any(name.endswith(suffix) for suffix in _REQUEST_BODY_SUFFIXES)

This works perfectly for current models, but the convention is implicit. If a future developer adds a request-body sub-model that doesn't follow the *Request / *RequestOrder naming pattern (e.g., BatchOrderItem embedded inside a BatchCreateOrdersRequest), it will be misclassified as a response model and the test will enforce extra=\"allow\" on it — wrong policy for a request sub-model.

Suggest adding a short comment here explaining that the convention is load-bearing:

# Convention (CLAUDE.md §"Adding a new resource"): every model that is part of
# a request body must be named with a *Request or *RequestOrder suffix so this
# heuristic correctly classifies it. Sub-models of request bodies must follow
# the same convention or be added to an explicit exclusion list here.
_REQUEST_BODY_SUFFIXES = ("Request", "RequestOrder")

3. __all__ scope gap is acknowledged but not guarded (informational)

The docstring on _exported_model_classes() notes that models not wired into __all__ silently escape the check. This is acceptable given the "always export" project convention, but if the __all__ entry is ever accidentally omitted for a new model, the policy test gives no signal. No action required — just flagging it as a known blind spot.

4. Test count appears off by one (trivial)

PR description says "+103 from the new parametrized tests" but the audit table shows 85 response + 17 request = 102 parametrized test cases. Either the count is off or there is a 103rd test I am not seeing (possibly collected from a fixture). Negligible, but worth correcting in the PR description.

---

Verdict

Items 2 and 3 are worth addressing before merge; item 1 is purely cosmetic. Nothing here is a correctness or security issue. The core change is correct, the test is a real improvement, and the CHANGELOG is exemplary.