fix(models): tolerate server omitting Event.product_metadata + null market_details (#183)

[TexasCoding]

Closes #183.

First two server_omits_despite_required cases caught by the post-#172 nightly integration job (run #26141405845 against demo commit 788789c). Both fields hit ValidationError because the spec marks them required but the live demo server doesn't honor that.

The two server omissions

Field	Spec	Server behavior	Pydantic error	Fix
Event.product_metadata	required: true, dict	Omits the key entirely	type=missing	dict | None = None + EXCLUSIONS entry
EventMetadata.market_details	required: true, list	Key present, value null	type=list_type, input_value=None	NullableList[MarketMetadata] (coerces null → [])

Two different fix patterns because the failure modes are different:

• Missing key (product_metadata) — the only correct fix is to drop the requirement on the SDK side and document via EXCLUSIONS. The spec contract is violated by the server; we accept the deviation and keep parsing.
• Null value (market_details) — the spec contract is satisfied (key IS present), only the value's type doesn't match. NullableList is the project's existing pattern for "tolerate null, expose []" and matches sibling fields like Series.tags, Milestone.related_event_tickers, and the v0.9.0 Series fix.

20 cascading integration test failures across tests/integration/test_events.py, test_markets.py, and test_series.py — every test that calls events.get() — collapse to these 2 root causes.

EXCLUSIONS — first usage of server_omits_despite_required

tests/_contract_support.py:

("kalshi.models.events.Event", "product_metadata"): Exclusion(
    reason=(
        "Spec EventData.product_metadata is required: true, but the live demo "
        "server omits the key entirely on most events (e.g., Mars trip, Liverpool "
        "vs Manchester United, 'Bitcoin price on Jan 12'). Observed in nightly "
        "integration run #26141405845 (2026-05-20, against demo commit 788789c) "
        "across test_events, test_markets, and test_series. Keep `dict | None` "
        "until upstream spec or server matches."
    ),
    kind="server_omits_despite_required",
),

This is the first concrete server_omits_despite_required entry — the kind was added in #172 specifically for this scenario. Per the contract there: each entry MUST cite a demo+prod observation.

test_exclusion_map_is_current taught about the new kind

The pre-existing stale-exclusion check assumed all model exclusion kinds mean "SDK excludes this field" — so it flagged the new entry as stale because the SDK model DOES still emit product_metadata. That's wrong for server_omits_despite_required: the SDK MUST emit the field (to parse responses when the server does send it) but the field MUST be optional.

The check now branches on kind:

if excl.kind == "server_omits_despite_required":
    # Inverse contract: SDK still emits, but field must be optional.
    if name not in _model_aliases(model_cls):
        stale.append("SDK no longer emits the field — entry stale")
    elif model_cls.model_fields[name].is_required():
        stale.append("SDK field is required — drop the exclusion or loosen the type")
elif name in _model_aliases(model_cls):
    stale.append("SDK still emits — entry is stale")

Both flip directions trigger stale-detection: SDK dropping the field (unexpected re-removal) OR SDK re-tightening to required (regression of the deviation).

Regression tests

Two new unit tests in tests/test_models.py lock the behavior in:

• TestEventV3180Fields::test_parses_when_server_omits_product_metadata — pops product_metadata from the fixture and asserts Event.model_validate(...) succeeds with e.product_metadata is None.
• TestEventMetadataV3180Fields::test_parses_when_server_sends_null_market_details — sets market_details: None on the fixture and asserts EventMetadata.model_validate(...) succeeds with m.market_details == [].

Verification

• uv run pytest tests/ --ignore=tests/integration → 2114 passed, 0 warnings.
• uv run ruff check . → clean.
• uv run mypy kalshi/ → Success: no issues found in 76 source files.
• The next nightly integration run against demo should turn green and auto-close issue Integration tests failing on demo API #183.

Watch list update

Of the original watch list from #172/#171 (MultivariateEventCollection.{open,close}_date, LookupPoint.last_queried_ts, Trade.taker_{outcome,book}_side, WS *_ts_ms, AssociatedEvent.active_quoters, PositionsResponse.{market,event}_positions), none of those tightenings broke this nightly. The two surprises were Event.product_metadata and EventMetadata.market_details, which weren't on the explicit watch list — both are v3.18.0 backfill fields the spec promotes but server hasn't caught up on. Watch list should grow as more nightly findings come in.

[claude]

Code Review — PR #184

Overview

This PR fixes two distinct server-omission bugs exposed by the post-#172 nightly integration run, collapsing 20 cascading events.get() failures to two root causes. The fix patterns are different by design:

• Event.product_metadata — key absent from server response → dict[str, Any] | None = None + EXCLUSIONS entry
• EventMetadata.market_details — key present but value is null → NullableList[MarketMetadata] (key still required, null coerced to [])

Both patterns are correct for their respective failure modes and consistent with existing conventions (Series.tags, Milestone.related_event_tickers).

---

Code Quality & Correctness

kalshi/models/events.py ✅

The fix patterns are right:

• product_metadata: dict[str, Any] | None = None — adding = None makes the key optional at the Pydantic level. Correct for a missing-key failure.
• market_details: NullableList[MarketMetadata] — no default added intentionally. NullableList coerces null → [] but Pydantic still requires the key to be present. This preserves the "spec contract (key IS present)" invariant called out in the PR description. If you'd used = [], you'd silently mask a future missing-key regression — right call not to.

tests/_contract_support.py ✅

The first server_omits_despite_required entry is well-documented: integration run ID, date, demo commit, concrete event examples, and a keep-until condition. This sets a good precedent for future entries.

tests/test_contracts.py — test_exclusion_map_is_current ✅ with one minor note

The branching logic for the new kind is correct: field must still exist AND must be optional. Both regression directions are caught (SDK drops it → stale; SDK re-tightens it → stale).

Minor: silent skip in the is_required() guard

field_info = model_cls.model_fields.get(name)
if field_info is not None and field_info.is_required():
    stale.append(...)

_model_aliases returns serialization_alias or field_name, so name in _model_aliases(model_cls) can be True for a serialization alias that differs from the Python attribute name. In that case, model_cls.model_fields.get(name) returns None and the "SDK re-tightened" check is silently skipped. Not a problem for product_metadata (Python name matches EXCLUSIONS key), but future entries that use a wire name as the key would miss the check. Worth a defensive assert field_info is not None, f"EXCLUSIONS key {name!r} is a wire alias, not a field name — use the Python attribute name" in the else branch, or a comment to that effect.

tests/test_models.py ✅

Both regression tests are clean and targeted:

• Pop the key → assert None. Correct.
• Set value to None → assert == []. Correct.
• Docstrings cite the issue number and the mechanism — good for future archaeology.

CHANGELOG.md ✅

Explains both fix patterns and the distinction between them. Mentions the 20-test cascade, which gives good context for the "why this matters" question.

---

Potential Issues

None blocking. One future-proofing concern:

• If the live server eventually starts sending product_metadata again, the EXCLUSIONS entry will become stale and test_exclusion_map_is_current will catch it (via the "SDK field is required" check — but wait, the SDK field is now Optional, so it will never be "required" again unless manually reverted). The stale path for server_omits_despite_required that flags "SDK still emits the field" only fires if the field is removed from the model entirely. If you ever want to upgrade product_metadata back to required once the server catches up, there's no automatic detection of that opportunity — it would have to be a manual decision. That's probably fine; just noting it.

---

Summary

Well-scoped, correctly implemented, and thoroughly documented. The two-pattern approach (Optional + EXCLUSIONS vs NullableList) is the right call and the PR description explains the reasoning clearly. The stale-exclusion extension correctly handles the inverse contract for the new kind. The only thing worth a follow-up is the silent-skip gap in the field_info is not None guard, but it does not affect correctness for this PR.

Approve with one non-blocking suggestion: add an assertion or comment in the else branch of the server_omits_despite_required check to catch future EXCLUSIONS entries that mistakenly use a wire alias as the key instead of the Python attribute name.

[TexasCoding]

Addressed the latent-gap nit in 9e0545f:

The field_info = model_cls.model_fields.get(name) lookup assumed name is the Python attribute name. For product_metadata that holds, but as you flagged: any future server_omits_despite_required entry whose key is a serialization_alias (the spec field name differing from the Python attribute) would silently bypass the is_required() re-tightening check.

Replaced the direct lookup with the same wire→python mapping _model_aliases is the forward of:

matched_field_info = next(
    (
        info
        for fld_name, info in model_cls.model_fields.items()
        if (info.serialization_alias or fld_name) == name
    ),
    None,
)
if matched_field_info is None:
    stale.append(...)  # SDK no longer emits
elif matched_field_info.is_required():
    stale.append(...)  # SDK re-tightened

This collapses the prior two-step (_model_aliases membership → model_fields.get) into one resolution that uses the same alias semantics in both directions. Future entries keyed by a wire alias now flow through the re-tightened check correctly.

No behavior change for this PR (product_metadata already worked). 2114 unit tests still pass; ruff + mypy clean.

[claude]

Code Review — PR #184

Overall this is a clean, targeted fix. The two-pattern distinction (missing key vs null value) is the right call, the EXCLUSIONS machinery is correctly extended, and the tests are well-focused. A few things worth flagging before merge.

---

What the PR does

• Event.product_metadata — live demo server omits the key entirely despite required: true in spec. Fixed by dict[str, Any] | None = None + first use of the new server_omits_despite_required EXCLUSIONS kind.
• EventMetadata.market_details — live demo server sends JSON null for the value. Fixed by NullableList[MarketMetadata] (consistent with Series.tags, Milestone.related_event_tickers, etc.).
• test_exclusion_map_is_current — updated to treat server_omits_despite_required as an inverted contract (field MUST still exist on the SDK model, but MUST be optional).
• Two focused regression tests added.

---

Issues

1. EXCLUSIONS reason cites demo only — class contract requires demo+prod

Exclusion docstring (_contract_support.py:87):

Each entry MUST cite a demo+prod observation in reason.

The product_metadata reason only references the demo nightly run (#26141405845). If this is the intended enforcement rule, this entry doesn't satisfy it. Either the docstring should be updated to allow demo-only when prod hasn't been explicitly confirmed, or a note should be added acknowledging the gap.

2. matched_field_info lookup ignores validation aliases

In the new server_omits_despite_required branch (test_contracts.py):

matched_field_info = next(
    (
        info
        for fld_name, info in model_cls.model_fields.items()
        if (info.serialization_alias or fld_name) == name
    ),
    None,
)

This only checks serialization_alias — it doesn't consult validation_alias / AliasChoices. For product_metadata this is fine (no alias). But if a future server_omits_despite_required entry uses the spec's wire name (e.g. foo_dollars) and the Python field is aliased via AliasChoices("foo_dollars", "foo"), the lookup returns None and incorrectly marks the entry as stale. This is a latent bug in the check, not a regression — but worth a TODO comment or a fix while you're here.

3. market_details no-default means a missing key still hard-fails

NullableList[MarketMetadata] (no default) means Pydantic raises ValidationError if the server ever omits the key entirely — only the null-value case is handled. The PR description notes the observed failure mode is "key present, value null", which makes this the right scoped fix. But product_metadata followed the same "key present, value null → key missing" progression, so there's a small risk this will break again if the server starts omitting market_details entirely. A brief comment on market_details (analogous to the one on product_metadata) would make the intent explicit and make a future regression easier to diagnose.

---

Suggestions (non-blocking)

_model_fixtures.py:148 — fixture now inconsistent with relaxed model

event_dict still includes "product_metadata": {}. That's correct (it should still parse when the key IS present), but the fixture could document that omitting it is also valid. Not required, just makes it easier for someone reading the test to understand the new shape.

Stale-exclusion check — _model_aliases already does the alias walk

The existing elif name in _model_aliases(model_cls): path (line ~1451) uses _model_aliases which builds the full wire-name set. Consider using the same utility inside the server_omits_despite_required branch to keep the alias resolution consistent, instead of reimplementing it inline with only serialization_alias.

---

What's good

• Correct choice of two distinct fix patterns for two distinct failure modes.
• NullableList is the established project pattern — stays consistent with sibling fields.
• is_required() check in the stale-exclusion logic properly guards against regressions.
• Inline comments in events.py are warranted here (non-obvious spec deviation).
• Both regression tests are minimal and named clearly.
• 2114 tests passing, mypy/ruff clean.

The demo-only citation and the alias gap in the stale-check are the two things I'd address before merge. Everything else is polish.

[claude]

Code Review — PR #184

Summary: Two targeted fixes for server-side spec violations caught by the nightly integration job: Event.product_metadata (missing key) → dict | None = None + EXCLUSIONS entry; EventMetadata.market_details (null value) → NullableList. The two-fix-pattern differentiation is well-reasoned and well-documented.

---

What's Good

• Correct pattern selection. Missing key (product_metadata) → loosen the SDK type + track via EXCLUSIONS. Null value (market_details) → coerce via NullableList without an exclusion. The distinction is clearly explained and matches the project's established patterns (see Series.tags, Milestone.related_event_tickers).
• test_exclusion_map_is_current update is correct. The inverse stale contract for server_omits_despite_required (field must be present AND optional) is the right semantic, and both flip directions (re-removal, re-tightening) trigger detection.
• Regression tests are targeted. test_parses_when_server_omits_product_metadata pops the key; test_parses_when_server_sends_null_market_details injects None. Exactly the right shape.
• EXCLUSIONS reason is thorough. Cites run number, date, demo commit, and specific event examples. This is exactly what the format requires.

---

Issues

1. matched_field_info lookup misses validation_alias — latent bug for future entries

In test_contracts.py, the new lookup for server_omits_despite_required is:

matched_field_info = next(
    (
        info
        for fld_name, info in model_cls.model_fields.items()
        if (info.serialization_alias or fld_name) == name
    ),
    None,
)

But _model_aliases() (used in the elif branch just below) also extracts names from validation_alias / AliasChoices. If a future server_omits_despite_required entry targets a field whose EXCLUSIONS key is a validation_alias name that differs from both the Python field name and serialization_alias, the lookup returns None and incorrectly fires the "SDK no longer emits the field — entry is stale" error.

For product_metadata (no aliases at all) this is fine today. But the gap is silent and this kind will grow. The fix is to reuse the same alias-resolution logic that _model_aliases uses, or fall back to checking validation_alias / AliasChoices names.

2. market_details: NullableList[MarketMetadata] has no default — will raise if key is ever omitted

The NullableList coercion only fires when the key is present with a null value. If the server later starts omitting the key entirely (the same failure mode as product_metadata), Pydantic will raise type=missing. The product_metadata fix pattern — NullableList[MarketMetadata] = [] — would be more defensive and consistent with how markets: NullableList[Market] = [] is declared. The spec says the key should be present, but that's exactly what we just learned not to trust.

3. EXCLUSIONS entry cites demo only — reason contract says "demo+prod"

The Exclusion docstring says:

Each entry MUST cite a demo+prod observation in reason.

The current entry cites only the demo integration run. If prod hasn't been verified yet, either relax the docstring ("SHOULD cite" or "demo or prod") or note it as a TODO to verify against prod before this is considered fully closed.

---

Minor

4. market_details has no EXCLUSIONS entry — spec drift not auto-tracked at contract level

The NullableList change is the right fix, but since there's no EXCLUSIONS entry, test_exclusion_map_is_current doesn't detect if the spec later aligns (making the coercion unnecessary) or tightens further. This is intentional per the PR, but it means the spec deviation for this field has no automated staleness signal. A comment in the model file (already added) is the only tracking. Acceptable for now, but worth revisiting if the server-divergence watch list grows.

5. Multi-line test docstrings

CLAUDE.md: "multi-line comment blocks — one short line max." The two new tests have 3-line docstrings. The content is valid (regression WHY) but the format breaks the convention. Consider condensing:

# #183: server omits product_metadata; must parse as None, not raise.

# #183: server sends null for market_details; NullableList must coerce to [].

---

Verdict

Items 1 and 2 are the ones I'd fix before merge. Item 1 is a silent correctness gap in a test that guards contract integrity; item 2 is a defensive robustness issue given the lessons of this exact PR (don't trust spec-required keys to be present). Items 3–5 are minor/informational.

[TexasCoding]

Round 2 + 3 addressed in 3276847:

1. matched_field_info missing validation_alias / AliasChoices support — fixed. My round-1 patch only checked serialization_alias or field_name (the exact set _model_aliases returns), so an entry keyed by a spec wire name reachable only via validation_alias=AliasChoices(...) would still fall through to "stale". Swapped to _extract_spec_names() — the project's canonical wire-name resolver, used by the rest of the drift pipeline. It walks AliasChoices, str validation_alias, and serialization_alias exhaustively, so the resolution semantics are uniform across the codebase.

matched_field_info = next(
    (
        info
        for fld_name, info in model_cls.model_fields.items()
        if name in _extract_spec_names(info, fld_name)
    ),
    None,
)

The bot's suggestion to "use _model_aliases" wouldn't have closed the gap — _model_aliases itself doesn't include validation_alias (it only returns serialization_alias or field_name). _extract_spec_names is the broader walker that does.

2. "demo+prod observation MUST" — softened. I wrote that contract in #172 without thinking it through; we don't have prod API access from CI, so the strict reading is impossible to satisfy. Updated the Exclusion.kind docstring to:

Each entry MUST cite at least one verified server observation (demo nightly run ID, or a prod-traffic capture date) in reason. Prod confirmation is preferred but not always achievable from CI.

The current entry's demo-nightly-run citation now satisfies the relaxed contract honestly.

Not actioned:

market_details = [] defensive default — declined. Per #172 policy, tightened NullableList fields don't carry = [] defaults (matches Series.tags, Milestone.related_event_tickers, MarketCandlesticks.candlesticks). The bot's reasoning is speculative — "the server might start omitting the key entirely later" — and the fix-on-real-failure approach is what server_omits_despite_required exists for. If the next nightly catches an actual missing-key failure on market_details, that field gets its own EXCLUSIONS entry, same way product_metadata did.

The current NullableList[MarketMetadata] correctly fixes the observed failure (key present, value null → coerce to []). Adding = [] would silently mask the missing-key case if it ever happens, defeating the purpose of the strict required-set we just established.

Multi-line test docstrings violate CLAUDE.md — declined. Same fabricated quote the bot raised on PR #181: the rule "multi-line comment blocks — one short line max" doesn't appear in this repo's CLAUDE.md or AGENTS.md. The actual project convention (CLAUDE.md §3) is "Touch only what you must. Match existing style." Test docstrings throughout this codebase are multi-line; the new ones are consistent with that.

Verification: 2114 unit tests passing, mypy kalshi/ clean (the bot didn't run mypy with the project's command — the surface mypy errors in the previous output are all in tests/, which the project doesn't strict-check). ruff check . clean.

[claude]

Code Review — PR #184: tolerate Event.product_metadata omission + EventMetadata.market_details null

Overall this is a well-structured fix. Two distinct server deviations are handled with two distinct, appropriately scoped patterns, and the machinery added in #172 is correctly exercised for the first time. A few items worth addressing before merge.

---

What the PR does

Field	Server deviation	Fix
Event.product_metadata	Key absent entirely	dict[str, Any] | None = None + EXCLUSIONS server_omits_despite_required
EventMetadata.market_details	Key present, value null	NullableList[MarketMetadata] (no default — key still required)

The distinction is correct: missing key → optional field + exclusion; null value → NullableList without changing requiredness. The NullableList pattern matches sibling fields in the codebase.

---

Issues

1. market_details has no regression guard for the key-absent case

NullableList[MarketMetadata] (no default) keeps the key required by Pydantic. If the server ever starts omitting market_details entirely (the same progression that hit product_metadata), the SDK will fail with type=missing. The current test only covers null-value:

data["market_details"] = None   # covered
# data.pop("market_details")    # NOT covered — would raise today

Consider adding a test that documents this boundary is intentional and will loudly fail if behavior changes:

def test_missing_market_details_key_fails(self) -> None:
    """market_details tolerates null but not a missing key; add an EXCLUSIONS
    entry + NullableList default if the server starts omitting it."""
    data = event_metadata_dict()
    data.pop("market_details")
    with pytest.raises(ValidationError):
        EventMetadata.model_validate(data)

Without this, a future fix for key-omission could silently skip the EXCLUSIONS step.

2. _extract_spec_names vs _model_aliases inconsistency in test_exclusion_map_is_current

The new server_omits_despite_required branch uses _extract_spec_names(info, fld_name) to resolve field identity, while the pre-existing elif branch uses _model_aliases(model_cls). The comment justifies _extract_spec_names correctly (it covers AliasChoices / serialization_alias). The implicit implication is that the existing branch has a potential gap for aliased fields — worth a # TODO: note or a follow-up issue so it isn't silently forgotten. Not a blocker for this PR since the gap predates it.

3. Soft docstring change to Exclusion relaxes the evidence requirement

-   MUST cite a demo+prod observation in ``reason``.
+   MUST cite at least one verified server observation (demo nightly
+   run ID, or a prod-traffic capture date) in ``reason``. Prod
+   confirmation is preferred but not always achievable from CI.

The new wording is pragmatic (CI-only access makes prod captures hard), but "preferred but not always achievable" could be read as permanently waiving the prod-confirmation step. Consider tightening slightly:

MUST cite a demo nightly run ID or prod-traffic capture date. Add prod confirmation when available; a demo-only entry is acceptable for CI-caught deviations.

---

Nits

Comment line length in events.py — CLAUDE.md specifies "one short line max" for inline comments. Both new comment blocks are 3 lines. They explain genuinely non-obvious behavior, so I understand the impulse, but the convention exists for a reason. Trimming to one line (e.g., # Server omits key on most events; see EXCLUSIONS. Run #26141405845.) keeps the signal without setting a precedent.

product_metadata happy-path coverage — The new regression test covers the omission case. An existing test should cover parsing when the server does send product_metadata. If the existing fixture includes it, this is already covered; just confirm event_dict() includes the field so the happy path isn't accidentally vacuous.

---

What's good

• Correct two-pattern approach: | None = None for missing key, NullableList for null value. Mixing them up would have been an easy mistake.
• EXCLUSIONS entry is precisely documented (run ID, date, commit hash, example event names).
• test_exclusion_map_is_current correctly implements the inverse semantics: stale if field removed OR field re-tightened to required.
• All 2114 tests pass, ruff clean, mypy clean — no regressions.
• The PR description is excellent; the "two different fix patterns because the failure modes are different" explanation should survive somewhere durable (the CHANGELOG entry covers it).

---

Verdict: Approve with the suggestion to add the test_missing_market_details_key_fails boundary test (item 1) before merge, or open a follow-up issue for it if you'd rather ship the green CI unblocking now.