fix: spec drift — settlement_sources on Event + lifecycle strike fields (#451)

[TexasCoding]

Summary

Resolves the nightly strict contract failures in #451. The live OpenAPI/AsyncAPI specs gained fields after #449 synced them, redding pytest tests/test_contracts.py -W error::UserWarning:

Spec schema	New field(s)	SDK model
EventData	settlement_sources (required, nullable)	kalshi.models.events.Event
marketLifecycleV2Payload	strike_type, cap_strike, custom_strike	kalshi.ws.models.market_lifecycle.MarketLifecyclePayload

Changes

• Event.settlement_sources → NullableList[SettlementSource]. Spec marks it required + nullable: true, so the SDK field is required (passes test_required_drift) but coerces a JSON null → [] (matches EventMetadata.settlement_sources / Series.settlement_sources). SettlementSource was moved above Event so the forward reference resolves under from __future__ import annotations. The central event_dict() fixture gains the now-required key.
• MarketLifecyclePayload gains strike_type: str | None, cap_strike: DollarDecimal | None, custom_strike: dict | None — all optional (present only on metadata_updated), mirroring the existing floor_strike handling.

Spec handling note ⚠️

The freshly-fetched live openapi.yaml also transiently dropped CreateOrder / BatchCreateOrders / AmendOrder (104 → 101 operations) — this reds the body-drift tests for those endpoints (KeyError: path '…/amend' not found). That is an upstream publishing glitch, not a real API change, so I did not import it. The committed openapi.yaml was kept at its known-good 104-operation state and settlement_sources was added to EventData surgically. asyncapi.yaml was synced normally (additive strike fields + WS error-doc refinements; no dropped channels).

If a future nightly still reds on the order-endpoint drop, that's a separate upstream-flap issue (the drift workflow may want to tolerate transient operation removals).

Verification

• uv run ruff check . — clean
• uv run mypy kalshi/ — clean
• uv run pytest tests/test_contracts.py -W error::UserWarning (nightly strict) — 676 passed
• uv run pytest tests/ --ignore=tests/integration — 4315 passed, 3 skipped

Closes #451

🤖 Generated with Claude Code

[claude]

Code Review — fix/spec-drift-451

Summary: Minimal, well-targeted spec-drift fix. Two new field groups are added faithfully against the live v3.21.0/asyncapi specs, contract tests pass (676 / 4315), and the decision to hold the openapi.yaml at the known-good 104-operation state rather than import a transient breakage is the right call and well-documented.

---

✅ What's done well

• NullableList[SettlementSource] on Event.settlement_sources — exactly right. Spec marks the field required (key must exist) and nullable: true (value may be null). NullableList holds the contract-required-key invariant while coercing null → [] so callers see a stable type. Consistent with EventMetadata.settlement_sources and Series.settlement_sources patterns.
• SettlementSource moved above Event — necessary under Pydantic v2's eager field resolution path; the reasoning is sound.
• MarketLifecyclePayload new fields — all three (strike_type, cap_strike, custom_strike) are properly | None = None, mirroring floor_strike exactly. Logical grouping adjacent to floor_strike makes the "between uses both, greater uses floor only, less uses cap only" relationship clear.
• AsyncAPI sync — removal of error code 28 and market_tickers from errorResponsePayload, updated maximum: 27, and the new terminal-errors doc section all look like faithful upstream changes. extra="allow" on WS models means no parse breakage if the field still arrives.
• event_dict() fixture — adding "settlement_sources": [] is required since the field now has no default; good catch.

---

⚠️ Concerns

1. Missing unit tests for the new nullable coercion behaviour

The CLAUDE.md convention is "new function → write a test" and "bug fix → write a regression test." The contract suite verifies the field is present, but there's no unit test that explicitly exercises:

• Event(settlement_sources=None, ...) → event.settlement_sources == []
• Event(settlement_sources=[{"url": "https://example.com", "name": "source"}], ...) → parsed SettlementSource list

Without it, a future refactor that changes NullableList semantics could silently regress the null-coercion path. A two-case parametrize in tests/test_events.py would be enough.

Similarly, no tests for the new MarketLifecyclePayload fields — at least a round-trip parse test covering strike_type/cap_strike together (the "between" example from the asyncapi spec) would give confidence.

2. Event.settlement_sources has no default — de-facto breaking change

Any caller constructing Event(...) directly without settlement_sources will now get a Pydantic ValidationError. That's spec-correct, but worth a note in the changelog or PR body for downstream consumers. The PR body focuses on the nightly CI fix and doesn't call this out.

3. Minor type inconsistency: EventMetadata.settlement_sources uses list, not NullableList

# Event (this PR):
settlement_sources: NullableList[SettlementSource]   # nullable: true in spec

# EventMetadata (unchanged):
settlement_sources: list[SettlementSource]           # presumably not nullable in spec

This is intentional if the spec treats them differently, but the comment on the Event field ("Mirrors EventMetadata.settlement_sources") slightly implies parity that doesn't fully exist. If the live server has been observed to return null for EventMetadata.settlement_sources too, it should be NullableList there as well — but that would be a separate issue. The comment phrasing could be tightened to "same pattern as EventMetadata.settlement_sources / Series.settlement_sources" instead of "Mirrors."

4. strike_type typed as str | None vs. an enum

The asyncapi spec describes known values "between" | "greater" | "less". Using str | None is consistent with how event_type is handled in the same model, and extra="allow" gives forward-compat. This is a conscious tradeoff; just flagging it in case a Literal["between", "greater", "less"] | None was considered and rejected — fine either way.

---

Nit

# metadata_updated only (added to the live asyncapi spec post-#449).
# strike_type decides how floor_strike/cap_strike are read ("between" uses
# both, "greater" floor only, "less" cap only); custom_strike carries
# structured strikes. All optional — present only on metadata_updated.
strike_type: str | None = None
cap_strike: DollarDecimal | None = None
custom_strike: dict[str, Any] | None = None

The comment block is accurate and helpful, but it's four lines. Could compress to:

# metadata_updated only (#449 asyncapi). strike_type: "between"/"greater"/"less"
# controls floor_strike / cap_strike interpretation; custom_strike for structured strikes.
strike_type: str | None = None
cap_strike: DollarDecimal | None = None
custom_strike: dict[str, Any] | None = None

Minor — not a blocker.

---

Verdict

Approve with a suggestion to add tests before merge. The model and spec changes are correct and well-reasoned. The main gap is test coverage for the new nullable coercion and the WS strike fields. Given the project's 4315-test suite and the CLAUDE.md standard, two targeted test cases would close that gap cleanly.

🤖 Generated with Claude Code

[TexasCoding]

Thanks for the review — addressed in 1577e81.

① Missing unit tests — fixed. Added two test classes:

• TestEventSettlementSources (tests/test_models.py): null → [] coercion, typed SettlementSource list parsing, and required-key enforcement.
• TestMarketLifecycleStrikeFields (tests/ws/test_models.py): the asyncapi "between" example (strike_type + floor_strike + cap_strike + custom_strike) plus the default-None path on non-metadata_updated events.

③ Comment precision — fixed. Reworded to "same nullable shape as Series.settlement_sources; EventMetadata's is a plain list because its spec field isn't nullable" — you're right that only Series truly mirrors it.

② No default = de-facto breaking — acknowledged. Event is a response model, so direct construction is rare (mostly tests), and the spec marks the field required, so making it required is the spec-faithful choice. CHANGELOG here is updated per-release rather than per-PR, so I've left it for the release cut but flagged the nuance here.

④ str vs Literal["between","greater","less"] — kept str. Consistent with how event_type is typed in the same model, and extra="allow" keeps it forward-compatible if Kalshi adds a strike type.

Nit (comment length) — kept the 4-liner. The "between/greater/less → floor/cap interpretation" detail earns its space, and you flagged it as non-blocking.

Local: ruff + mypy clean, new tests green, affected suites (test_models / ws/test_models / test_events) 297 passed.

[claude]

---

Code Review: PR 452 — Spec drift fix (settlement_sources + lifecycle strike fields)

Overview

Clean, targeted spec drift fix. Changes are minimal, well-explained, and consistent with existing SDK conventions. The PR description is thorough about the upstream publishing glitch; not importing the transiently-broken spec state was the right call.

Positives

• Surgical: only 7 files changed, all of them necessary.
• NullableList[SettlementSource] is the correct pattern, consistent with Series.settlement_sources and EventMetadata.
• Moving SettlementSource above Event is the correct fix for forward-reference resolution under from __future__ import annotations.
• Test coverage is solid: null coercion, populated list, required-key enforcement, and both lifecycle strike scenarios.
• The field comment on settlement_sources justifies non-obvious nullable handling — exactly when CLAUDE.md says comments are warranted.

---

Issues

1. Breaking change not flagged in changelog (medium)

settlement_sources has no default, making it strictly required on Event. Any consumer deserializing cached responses, hitting a demo endpoint that has not caught up, or running against test fixtures that predate this PR will get ValidationError at runtime instead of a graceful None.

test_field_is_required confirms this is intentional, which is acceptable if the API always sends the key. However, the markets field (also required in spec) carries = [] as a safety default, suggesting the SDK has historically been lenient here. The changelog entry should flag this as a potentially breaking behavior change.

Suggestion: add = [] default with a comment similar to markets, or document the strictness explicitly in CHANGELOG.md. The contract test still enforces key-presence, so a default would not weaken the spec check.

2. market_tickers is now spec-orphaned (low)

asyncapi.yaml removes market_tickers from errorResponsePayload (alongside error code 28 removal). ws/models/base.py still has market_tickers: list[str] | None = None. Since it is optional and models use extra="allow", nothing breaks — but it is undocumented drift. Worth a follow-up issue so it does not get lost.

3. strike_type valid values untested (low)

The spec documents "between", "greater", and "less" as known values. Only "between" is exercised in tests. Fine for now, worth noting if business logic around these variants grows.

4. Series.settlement_sources type mismatch (low, pre-existing)

Series.settlement_sources is NullableList[dict] while Event.settlement_sources is now NullableList[SettlementSource]. Same concept, different precision. Not a blocker here, but worth aligning on the next Series spec sync.

---

Summary

Area	Status
Correctness	✅
Spec fidelity	✅
Test coverage	✅ (minor: "greater"/"less" strike_type untested)
Convention adherence	✅
Breaking change handling	⚠️ Should be documented in CHANGELOG.md
Pre-existing drift	ℹ️ market_tickers orphan worth a follow-up issue

Recommend merging after addressing item 1. Items 2–4 can be tracked as follow-up issues.

---

Reviewed by Claude Sonnet 4.6