From 8890f2d858862d7d912e902a00356933fa5152d9 Mon Sep 17 00:00:00 2001 From: Jeff West Date: Tue, 19 May 2026 05:43:26 -0500 Subject: [PATCH 1/2] test(infra): wire response-side drift through EXCLUSIONS map (#158) Response-side spec drift (TestSpecDrift / TestWsSpecDrift additive + required checks) is warn-only. The v2.1.0 Balance.balance_dollars regression slipped through five rounds of review because additive warnings don't fail CI. Promoting that warning to a hard-fail is the highest-leverage fix in the response-side hardening stack. This PR is the load-bearing infrastructure prerequisite. It wires the response-side drift checks through the existing EXCLUSIONS allowlist without changing warn->fail behavior yet (that's PR5). Changes ------- tests/_contract_support.py - Rewrite module docstring + EXCLUSIONS comment header to document all four consumers (TestRequestParamDrift, TestRequestBodyDrift, TestSpecDrift, TestWsSpecDrift) and note that a single (fqn, field) key may serve more than one drift check (e.g. CreateOrderRequest is both a request body and a response schema). - Add 4 new response-side entries: Market.response_price_units, Order.action, Orderbook.orderbook_fp, TickerPayload.time. The other 3 entries listed in the issue (CreateRFQRequest.target_cost_centi_cents, CreateRFQRequest.contracts_fp, CreateOrderRequest.sell_position_floor) already existed as body-side entries with more detailed reasons and are now reused by response-side checks via the new wiring; adding duplicate keys would have overwritten the existing reasons. tests/test_contracts.py - _classify_drift now consults EXCLUSIONS.get((entry.sdk_model, field)) alongside entry.ignored_fields in both the additive and required loops. This covers test_additive_drift, test_required_drift, and test_ws_additive_drift (all three funnel through this helper). - test_ws_required_drift inline loop gets the same EXCLUSIONS lookup. - test_exclusion_map_is_current case 1 now walks all three source maps (BODY_MODEL_MAP, CONTRACT_MAP, WS_CONTRACT_MAP) and flags an entry stale only when the field is absent from every applicable spec source. Verification ------------ uv run pytest tests/test_contracts.py -> 372 passed, 57 warnings uv run pytest tests/test_contracts.py -W error -> 57 failed (was 60) uv run mypy kalshi/ -> Success: no issues uv run mypy tests/_contract_support.py tests/test_contracts.py -> 11 errors, all pre-existing on main (verified via git stash) uv run ruff check . -> All checks passed! Smoke-tested test_exclusion_map_is_current by flipping Market.response_price_units to a bogus name (and separately TickerPayload.time): both branches correctly fail with "absent from every spec source (...)". Note on the failure count ------------------------- The issue's acceptance criterion 2 claimed "exactly 53 failures" under -W error. Actual count is 57. The author appears to have assumed each of the 7 allowlisted entries would convert a unique parametrized test from fail->pass; in practice, only 3 tests have that property (test_additive_drift[Orderbook], [CreateRFQRequest], [CreateOrderRequest] -- the only ones where the allowlisted field was the sole drift bullet). Market / Order / TickerPayload retain many other unfiltered drift entries, so their tests still warn. The substantive intent -- EXCLUSIONS is consulted by the response-side checks -- is verified by direct grep: each of the 7 (model, field) keys no longer surfaces in any drift warning, while every other drift bullet is preserved. Closes #158. --- tests/_contract_support.py | 84 +++++++++++++++++++++++++---------- tests/test_contracts.py | 90 ++++++++++++++++++++++++++++++-------- 2 files changed, 133 insertions(+), 41 deletions(-) diff --git a/tests/_contract_support.py b/tests/_contract_support.py index d336b07..2e8ca44 100644 --- a/tests/_contract_support.py +++ b/tests/_contract_support.py @@ -1,13 +1,27 @@ -"""Request-side contract support: SDK method → OpenAPI endpoint mapping and helpers. +"""Contract test infrastructure: SDK ↔ spec mapping + the EXCLUSIONS allowlist. -This module is test infrastructure. It lives in ``tests/`` (not ``kalshi/``) so that -users importing the SDK don't get spec-parsing code shipped in the PyPI wheel. +This module is test infrastructure. It lives in ``tests/`` (not ``kalshi/``) so +that users importing the SDK don't get spec-parsing code shipped in the PyPI +wheel. -The map covers ``path``, ``query``, and ``requestBody`` surface. Body schemas -for POST/PUT endpoints are referenced via ``MethodEndpointEntry.request_body_schema`` -(a spec ``$ref`` string) and resolved via ``_resolve_request_body_schema``. Drift -tests that consume this infrastructure (``TestRequestParamDrift``, -``TestRequestBodyDrift``) land in subsequent v0.8.0 tasks. +``METHOD_ENDPOINT_MAP`` covers the request side: ``path``, ``query``, and +``requestBody`` surface. Body schemas for POST/PUT/DELETE-with-body endpoints +are referenced via ``MethodEndpointEntry.request_body_schema`` (a spec ``$ref`` +string) and resolved via ``_resolve_request_body_schema``. + +``EXCLUSIONS`` is consulted by FOUR drift checks: + + - ``TestRequestParamDrift`` — query/path kwargs vs. spec parameters + - ``TestRequestBodyDrift`` — request body model properties vs. spec + - ``TestSpecDrift`` — REST response model fields vs. OpenAPI + (``test_additive_drift`` + ``test_required_drift``) + - ``TestWsSpecDrift`` — WS payload model fields vs. AsyncAPI + (``test_ws_additive_drift`` + ``test_ws_required_drift``) + +A single ``(sdk_fqn, field_name)`` key may serve more than one check — e.g. +``CreateOrderRequest`` and ``CreateRFQRequest`` are both request bodies AND +response schemas in the spec, so a field exclusion is reused by both sides. +Add a new entry once at the model+field coordinate; do NOT duplicate it. Async siblings are derived at test time via ``Async`` substitution with identical method names. Do NOT add separate async entries to the map. @@ -737,24 +751,25 @@ class Exclusion: # --------------------------------------------------------------------------- -# EXCLUSIONS allowlist for request-side drift tests +# EXCLUSIONS allowlist for drift tests (request + response) # --------------------------------------------------------------------------- # Keys are ``(sdk_fqn, param_or_field_name)`` tuples. Values are ``Exclusion`` -# dataclasses with a required ``reason`` string. An entry declares "this is -# not drift; here's why." Tests that fail to find the corresponding drift -# should also fail (see ``test_exclusion_map_is_current``), so stale entries -# don't accumulate. +# dataclasses with required ``reason`` and ``kind`` fields. An entry declares +# "this is not drift; here's why." # -# BOOTSTRAP (v0.8.0) — two classes of entries: -# 1. ``CreateOrderRequest`` spec fields the SDK deliberately omits (cent-form -# redundant with ``_dollars`` variants; deprecated-in-spec). -# 2. ``cursor`` on every ``list_all`` method — paginator-handled internally; -# the kwarg is absent from the method signature by design. +# Consumers (see this module's docstring for full coverage): +# - request side : ``TestRequestParamDrift``, ``TestRequestBodyDrift`` +# - response side: ``TestSpecDrift`` (additive + required), +# ``TestWsSpecDrift`` (additive + required) # -# Additional entries for ``AmendOrderRequest`` get appended in Task 3 (after -# that model is created). Do NOT preload them here — forward references to -# a model that doesn't exist yet will trip mypy on import. - +# A single key may serve more than one check — e.g. ``CreateOrderRequest`` is +# both a request body and a response schema in the spec, so a field-level +# exclusion is reused by both sides. Add a new entry once at the model+field +# coordinate; do NOT duplicate it per drift test. +# +# Tests that fail to find the corresponding drift should also fail (see +# ``test_exclusion_map_is_current``), so stale entries can't silently +# accumulate. EXCLUSIONS: dict[tuple[str, str], Exclusion] = { # --- CreateOrderRequest spec fields deliberately not on the model --- ("kalshi.models.orders.CreateOrderRequest", "yes_price"): Exclusion( @@ -1111,6 +1126,31 @@ class Exclusion: reason="paginator-handled; not a caller-facing kwarg on list_all", kind="paginator_handled", ), + # --- Response-side additive drift: spec fields the SDK intentionally omits --- + # Consumed by TestSpecDrift / TestWsSpecDrift in addition to any request-side + # body/param checks above that happen to share the same model+field key. + # Each entry is a spec-documented deviation; flipping it to a hard fail + # would surface as a real bug (see issue #157 stack for the v2.1.0 + # ``Balance.balance_dollars`` regression that motivated this PR). + ("kalshi.models.markets.Market", "response_price_units"): Exclusion( + reason="spec marks DEPRECATED; superseded by price_level_structure / price_ranges", + kind="spec_deprecated", + ), + ("kalshi.models.orders.Order", "action"): Exclusion( + reason="spec marks Deprecated; superseded by outcome_side / book_side", + kind="spec_deprecated", + ), + ("kalshi.models.markets.Orderbook", "orderbook_fp"): Exclusion( + reason=( + "SDK reshapes the orderbook_fp nested object into yes/no level lists " + "at the resource layer; the model itself holds the unwrapped lists" + ), + kind="wire_normalization", + ), + ("kalshi.ws.models.ticker.TickerPayload", "time"): Exclusion( + reason="spec marks deprecated; superseded by ts_ms", + kind="spec_deprecated", + ), } diff --git a/tests/test_contracts.py b/tests/test_contracts.py index 87822c6..2e5513c 100644 --- a/tests/test_contracts.py +++ b/tests/test_contracts.py @@ -375,6 +375,11 @@ def _classify_drift( Returns (additive_issues, required_issues). Both are warnings, not failures (SDK is intentionally permissive). + + Per-field skips are honored in this order: + 1. ``entry.ignored_fields`` — per-contract-entry allowlist (legacy). + 2. ``EXCLUSIONS[(entry.sdk_model, field_name)]`` — typed, reasoned + allowlist shared with the request-side drift checks (#157). """ additive: list[str] = [] required_issues: list[str] = [] @@ -385,6 +390,8 @@ def _classify_drift( for spec_field_name in spec_fields: if spec_field_name in entry.ignored_fields: continue + if (entry.sdk_model, spec_field_name) in EXCLUSIONS: + continue if spec_field_name not in reverse_map: additive.append(f"Spec field '{spec_field_name}' has no SDK mapping") @@ -393,6 +400,8 @@ def _classify_drift( for req_field in required_fields: if req_field in entry.ignored_fields: continue + if (entry.sdk_model, req_field) in EXCLUSIONS: + continue sdk_name = reverse_map.get(req_field) if sdk_name and sdk_name in model_class.model_fields: field_info = model_class.model_fields[sdk_name] @@ -758,6 +767,8 @@ def test_ws_required_drift(self, entry: ContractEntry) -> None: for req_field in ws_required: if req_field in entry.ignored_fields: continue + if (entry.sdk_model, req_field) in EXCLUSIONS: + continue sdk_name = reverse_map.get(req_field) if sdk_name and sdk_name in model_class.model_fields: field_info = model_class.model_fields[sdk_name] @@ -1341,37 +1352,77 @@ def test_exclusion_map_is_current() -> None: stale: list[str] = [] for (fqn, name), excl in EXCLUSIONS.items(): - # Case 1: spec-side exclusion keyed on a model FQN (kalshi.models.*) - if fqn.startswith("kalshi.models."): + # Case 1: model-keyed exclusion. The FQN may belong to a request body + # (``BODY_MODEL_MAP``), a REST response model (``CONTRACT_MAP``), a WS + # payload (``WS_CONTRACT_MAP``), or several of these at once — e.g. + # ``CreateOrderRequest`` is both a request body and a response schema. + # ``name`` must appear in at least one of the spec sources, and the + # SDK model must NOT emit it as a wire alias. + if fqn.startswith("kalshi.models.") or fqn.startswith("kalshi.ws.models."): + spec_sources: list[tuple[str, dict[str, Any]]] = [] + + # Source 1: request body via BODY_MODEL_MAP + METHOD_ENDPOINT_MAP spec_ref = next( (ref for ref, m in BODY_MODEL_MAP.items() if m == fqn), None, ) - if spec_ref is None: - stale.append( - f"EXCLUSIONS[{(fqn, name)}] references unknown model " - f"{fqn} (not in BODY_MODEL_MAP); reason={excl.reason!r}" + if spec_ref is not None: + body_schema = None + for e in METHOD_ENDPOINT_MAP: + if e.request_body_schema == spec_ref: + body_schema = _resolve_request_body_schema( + spec, e.path_template, e.http_method, + ) + break + if body_schema is None: + stale.append( + f"EXCLUSIONS[{(fqn, name)}] references schema {spec_ref} " + f"not reachable via METHOD_ENDPOINT_MAP" + ) + continue + spec_sources.append( + (f"request body {spec_ref}", body_schema.get("properties", {})) ) - continue - schema = None - for e in METHOD_ENDPOINT_MAP: - if e.request_body_schema == spec_ref: - schema = _resolve_request_body_schema( - spec, e.path_template, e.http_method, + + # Source 2: REST response model via CONTRACT_MAP + for c_entry in CONTRACT_MAP: + if c_entry.sdk_model == fqn: + spec_sources.append( + ( + f"response schema {c_entry.spec_schema}", + _get_schema_fields(spec, c_entry.spec_schema), + ) ) break - if schema is None: + + # Source 3: WS payload model via WS_CONTRACT_MAP + for w_entry in WS_CONTRACT_MAP: + if w_entry.sdk_model == fqn: + ws_spec = _load_asyncapi_spec() + spec_sources.append( + ( + f"WS schema {w_entry.spec_schema}", + _get_ws_msg_fields(ws_spec, w_entry.spec_schema), + ) + ) + break + + if not spec_sources: stale.append( - f"EXCLUSIONS[{(fqn, name)}] references schema {spec_ref} " - f"not reachable via METHOD_ENDPOINT_MAP" + f"EXCLUSIONS[{(fqn, name)}] references unknown model {fqn} " + f"(not in BODY_MODEL_MAP, CONTRACT_MAP, or WS_CONTRACT_MAP); " + f"reason={excl.reason!r}" ) continue - if name not in schema.get("properties", {}): + + if not any(name in props for _label, props in spec_sources): + source_list = ", ".join(label for label, _ in spec_sources) stale.append( f"EXCLUSIONS[{(fqn, name)}] claims spec has {name!r} on " - f"{spec_ref}, but spec does NOT — entry is stale. " - f"reason={excl.reason!r}" + f"{fqn}, but it is absent from every spec source " + f"({source_list}) — entry is stale. reason={excl.reason!r}" ) + model_cls = _get_model_class_from_fqn(fqn) if name in _model_aliases(model_cls): stale.append( @@ -1410,7 +1461,8 @@ def test_exclusion_map_is_current() -> None: else: stale.append( f"EXCLUSIONS[{(fqn, name)}] has unexpected FQN prefix; " - f"expected kalshi.models.* or kalshi.resources.*" + f"expected kalshi.models.*, kalshi.ws.models.*, or " + f"kalshi.resources.*" ) if stale: From 36b9ab88ae3d86f1b8f2e76af5e3fd9b9d50d18d Mon Sep 17 00:00:00 2001 From: Jeff West Date: Tue, 19 May 2026 05:50:32 -0500 Subject: [PATCH 2/2] test(infra): address PR #165 review feedback Three fixes from claude[bot]'s review (#165): 1. Lazy-cache `ws_spec` across the exclusion loop instead of reloading the AsyncAPI YAML on every WS-keyed entry. Sentinel-init outside the loop, populated on first use; reused for the rest. Currently exercised live by `TickerPayload.time`; cost scales 1x instead of Nx as the WS exclusion list grows. 2. Drop the `continue` after the BODY_MODEL_MAP -> METHOD_ENDPOINT_MAP resolution failure. The stale diagnostic still surfaces (it IS a real infrastructure bug), but other sources (CONTRACT_MAP, WS_CONTRACT_MAP) now still get checked. Prevents a latent false-positive 'absent from every spec source' when a shared model has a broken body-side ref but a valid response-side entry. 3. Add an inline comment noting that the `ws_spec` lazy-load branch is exercised live by `TickerPayload.time`, so future readers know there is real coverage of the WS source path. Not addressed: the dual-stale-message behavior when a field is both 'absent from spec' AND 'emitted by SDK'. Reviewer correctly notes this is pre-existing (predates this PR) and not blocking. Both messages carry independent diagnostic value (spec-moved vs SDK-implemented are different remediations), so keeping them separate is defensible; not in scope for PR0. Verification (re-run after fixes): uv run pytest tests/test_contracts.py -> 372 passed, 57 warnings uv run ruff check . -> All checks passed! uv run mypy kalshi/ -> Success: no issues Smoke-retested all three test_exclusion_map_is_current branches: - REST stale (Market.bogus_rest_xyz) -> FAILED as expected - WS stale (TickerPayload.bogus_ws_xyz) -> FAILED as expected (exercises lazy-load) - baseline (no patch) -> PASSED --- tests/test_contracts.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/test_contracts.py b/tests/test_contracts.py index 2e5513c..94c2f95 100644 --- a/tests/test_contracts.py +++ b/tests/test_contracts.py @@ -1349,6 +1349,10 @@ def test_exclusion_map_is_current() -> None: intentional — prevent that here. """ spec = _load_spec() + # Lazy-loaded on first WS-keyed exclusion (avoids parsing the AsyncAPI + # YAML when no WS entry needs it; reused across all WS entries when one + # does — currently exercised live by ``TickerPayload.time``). + ws_spec: dict[str, Any] | None = None stale: list[str] = [] for (fqn, name), excl in EXCLUSIONS.items(): @@ -1375,14 +1379,18 @@ def test_exclusion_map_is_current() -> None: ) break if body_schema is None: + # METHOD_ENDPOINT_MAP / BODY_MODEL_MAP inconsistency is a + # real bug; surface it, but still try CONTRACT_MAP and + # WS_CONTRACT_MAP so a shared model isn't falsely flagged + # as unknown when one of its other sources would resolve. stale.append( f"EXCLUSIONS[{(fqn, name)}] references schema {spec_ref} " f"not reachable via METHOD_ENDPOINT_MAP" ) - continue - spec_sources.append( - (f"request body {spec_ref}", body_schema.get("properties", {})) - ) + else: + spec_sources.append( + (f"request body {spec_ref}", body_schema.get("properties", {})) + ) # Source 2: REST response model via CONTRACT_MAP for c_entry in CONTRACT_MAP: @@ -1398,7 +1406,8 @@ def test_exclusion_map_is_current() -> None: # Source 3: WS payload model via WS_CONTRACT_MAP for w_entry in WS_CONTRACT_MAP: if w_entry.sdk_model == fqn: - ws_spec = _load_asyncapi_spec() + if ws_spec is None: + ws_spec = _load_asyncapi_spec() spec_sources.append( ( f"WS schema {w_entry.spec_schema}",