feat(fix): market-data session + order-book reconstruction (closes #426)

[TexasCoding]

Summary

FIX Market Data on the dedicated KalshiMD session — order-book snapshots/incrementals and per-market trading-status — for both products. Builds on the foundation (#422), repeating groups (#423), order entry (#424), and drop copy (#425).

Spec note: the bundled FIX dictionary v1.03 predates market data (it has no V/W/X/Y/e/f and no MD fields), so the message layouts were derived from docs.kalshi.com/fix/market-data and the margin variant — which carry the authoritative tag tables and wire examples. The two product docs are identical for V/W/X/Y; Security Status (e/f) is documented on prediction only (harmlessly shared).

Messages (messages/market_data.py)

35=	Class	Dir	Notes
V	MarketDataRequest	out	snapshot() / subscribe() / unsubscribe() / unsubscribe_all() constructors encode the 263/146/55 rules (cancel-all omits NoRelatedSym/Symbol)
e	SecurityStatusRequest	out	subscribe() / unsubscribe() a single market
W	MarketDataSnapshotFullRefresh	in	top-level Symbol + NoMDEntries group {type,px,size}
X	MarketDataIncrementalRefresh	in	NoMDEntries group {action,symbol,type,px,size} (per-entry symbol)
Y	MarketDataRequestReject	in	md_req_rej_reason + text
f	SecurityStatus	in	symbol + security_trading_status

Inbound code fields stay raw (str/int, compare against the enums) — matching the order-entry convention so a slightly off-spec message still parses. New enums: SubscriptionRequestType, MDEntryType, MDUpdateAction, MDReqRejReason, SecurityTradingStatus. Prices use DollarDecimal (dollars on both products; margin enforces UseDollars).

Order-book reconstruction (orderbook.py)

FixOrderBook builds aggregated bid/offer books from a W snapshot + X incrementals. FIX MD uses absolute level sizes (not signed deltas): Change sets a level's new size, Delete removes it. A snapshot replaces a market's book; incrementals route per-entry by Symbol; an incremental for an un-seeded market is dropped. clear()/remove() support the reconnect→re-subscribe path (KalshiMD has no retransmission, so a sequence gap tears the session down and the client re-snapshots).

book = FixOrderBook()
async with client.market_data(on_message=handle) as session:
    await session.send(MarketDataRequest.subscribe(["KXNBAGAME-..."]))
# handle(raw): book.apply(decode_app_message(raw)); view = book.get("KXNBAGAME-...")

The client.market_data() session factory and the MD host-split / port-8233 / non-retransmission config already existed from the foundation.

Pre-PR adversarial review hardening

A multi-agent review (4 lenses → per-finding skeptical verification) ran before this PR; the confirmed findings are folded in:

• apply_incremental drops an unknown MDUpdateAction instead of silently applying it as a Change (was a robustness gap vs the sibling unknown-MDEntryType path).
• apply_snapshot drops 0-size levels — parity with an incremental Delete, so "size 0 = no level" holds on both paths.
• The per-market snapshot-replacement contract is now explicit on the public API: the caller must clear() before re-subscribing after a gap/teardown, else markets not re-seeded keep stale books.

Tests (tests/fix/test_market_data.py, 34 new)

Golden exact-wire fixtures (pinned to the doc examples — they catch field-order / repeating-group-layout drift that round-trips can't) + structural round-trips for all six messages; dispatch; order-book snapshot ordering, Change/Delete, change-to-zero, 0-size-snapshot drop, empty-snapshot clears book, unknown-action drop, incremental-before-snapshot, per-symbol routing, stale-survival contract; a KalshiMD subscription integration test (subscribe → W → X → reconstruct); and a drop-connection gap-recovery integration test (reconnect → clear() → re-subscribe → fresh W rebuilds) — both against the mock acceptor.

Verification

• uv run ruff check . — clean
• uv run mypy kalshi/ (strict) — clean (154 files)
• uv run pytest tests/fix/ — 165 passed

Closes #426. Part of #402.

🤖 Generated with Claude Code

[claude]

Code Review — PR #434: FIX market-data session + order-book reconstruction

Overview

Well-structured addition of FIX market-data support (messages V/W/X/Y/e/f), a clean FixOrderBook reconstructor, five new enums, eight new tags, and 34 tests including golden wire fixtures and two session-level integration tests. The pre-PR adversarial hardening shows (0-size snap drops, unknown-action drop, empty-snapshot-clears-book contract). Overall quality is high — one logic bug and a few smaller points to address.

---

Bug: apply_incremental — unknown action + zero size silently deletes

kalshi/fix/orderbook.py, lines 619–630

if action == MDUpdateAction.DELETE.value or entry.md_entry_size <= 0:
    levels.pop(entry.md_entry_px, None)
elif action == MDUpdateAction.CHANGE.value:
    levels[entry.md_entry_px] = entry.md_entry_size
else:
    # Out-of-spec action: don't silently apply it as a Change.
    logger.debug(...)
    continue

The size <= 0 guard is evaluated before the action check, so an unknown action (e.g. "9") arriving with size=0 hits the first branch and silently deletes the level rather than being dropped. This is the exact class of bug the "don't silently apply as a Change" comment is trying to prevent — just for Delete instead of Change.

The test test_orderbook_incremental_unknown_action_dropped only exercises the action="9", size=5 path and won't catch this.

Suggested fix — check action first:

if action == MDUpdateAction.DELETE.value:
    levels.pop(entry.md_entry_px, None)
elif action == MDUpdateAction.CHANGE.value:
    if entry.md_entry_size <= 0:
        levels.pop(entry.md_entry_px, None)
    else:
        levels[entry.md_entry_px] = entry.md_entry_size
else:
    logger.debug(
        "FIX MD incremental %s: unknown MDUpdateAction %r; dropping entry",
        entry.symbol, action,
    )
    continue

Add a test: action="9", size=Decimal("0") — should return applied=0 and leave the book unchanged.

---

Missing MdReqId (tag 262)

FIX spec mandates MdReqId (262) in MarketDataRequest (35=V) as a correlation handle, and in MarketDataRequestReject (35=Y) to reference the rejected request. Without it you can't correlate a reject to its original subscribe/snapshot call.

The current MarketDataRequest has no tag-262 field, and MarketDataRequestReject can't echo one back. This may be a deliberate simplification if Kalshi's KalshiMD implementation doesn't enforce it, but it should be called out explicitly — either add the field or add a comment noting it's intentionally omitted and add it to EXCLUSIONS with a reason.

---

Style / convention notes

Module docstring length (kalshi/fix/messages/market_data.py):
The module docstring is 40+ lines and includes a code example. Per CLAUDE.md conventions ("Never write multi-paragraph docstrings or multi-line comment blocks — one short line max"), this should be trimmed significantly. The message-table and code snippet are better suited for the PR description and project docs.

Unnecessary f-string prefix (tests/fix/test_market_data.py, line 771):

assert _wire(msg) == f"55={TICKER}|268=2|..."  # f-prefix not needed when TICKER is the only interpolation

Minor, but inconsistent with some of the other golden-wire assertions.

---

What's good

• Golden wire fixtures are the right call — they catch field-order and group-layout bugs that symmetric encode/decode round-trips cannot.
• _BookState.side() returning None for unknown MDEntryType cleanly unifies the unknown-type skip path across both snapshot and incremental without duplication.
• apply_snapshot dropping 0-size levels creates parity with the incremental Delete path — correct and consistent.
• unsubscribe_all() omitting NoRelatedSym/Symbol is spec-correct and the dedicated test verifies it at the tag level.
• MarketDataBook as a frozen dataclass with tuple levels gives callers an immutable snapshot safe to read without holding any lock.
• Gap-recovery integration test (test_market_data_gap_recovery_resubscribe) covers the clear() → re-subscribe → fresh snapshot path end-to-end, which is the highest-risk usage pattern.

---

Summary

One real bug (unknown-action + zero-size deletes silently), one spec compliance question (tag 262), and a style note on the module docstring. The logic bug is low-severity in practice (Kalshi is unlikely to send unknown actions) but should be fixed to match the documented invariant. Everything else looks correct and well-tested.

[claude]

Code Review — feat(fix): market-data session + order-book reconstruction

Overall this is high-quality work. The design is clean, the pre-PR adversarial-review hardening shows (the unknown-action / 0-size-snapshot paths are explicitly tested), and the golden wire fixtures are the right tool for catching field-order / repeating-group drift. Comments below focus on the things that actually matter.

---

Potential Bug / Fragility

MDIncrementalEntry.md_entry_size and md_entry_px have no defaults

class MDIncrementalEntry(FixGroup):
    md_update_action: str = fixfield(Tag.MD_UPDATE_ACTION, FixType.CHAR)
    symbol: str = fixfield(Tag.SYMBOL, FixType.STRING)
    md_entry_type: str = fixfield(Tag.MD_ENTRY_TYPE, FixType.CHAR)
    md_entry_px: DollarDecimal = fixfield(Tag.MD_ENTRY_PX, FixType.PRICE)          # required
    md_entry_size: FixedPointCount = fixfield(Tag.MD_ENTRY_SIZE, FixType.QTY)       # required

Standard FIX 5.0SP2 allows MDEntryPx and MDEntrySize to be omitted for DELETE actions. If Kalshi's server ever sends a delete without them (which is spec-compliant), the parser will raise before apply_incremental can handle it gracefully.

The PR description says md_entry_size is 0 when deleted — so this is an implicit assumption about Kalshi's wire behavior. It would be safer to give them defaults (default=None / Optional) or at minimum add a note in the docstring that the parser requires them. The orderbook.apply_incremental ignores md_entry_size entirely for DELETE, so the fix would be cheap:

md_entry_px: DollarDecimal | None = fixfield(Tag.MD_ENTRY_PX, FixType.PRICE, default=None)
md_entry_size: FixedPointCount | None = fixfield(Tag.MD_ENTRY_SIZE, FixType.QTY, default=None)

apply_incremental would then need entry.md_entry_size or Decimal(0) on the CHANGE branch — still simple.

---

Convention Violation (CLAUDE.md)

Issue references in module docstrings

orderbook.py, market_data.py, and the test file all open with (GH #426) in their docstrings/comments. CLAUDE.md is explicit:

Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves.

# orderbook.py line 1
"""Local order-book reconstruction from FIX market data (GH #426).
# tests/fix/test_market_data.py line 1
"""Tests for the market-data FIX flow + order-book reconstruction (GH #426)."""

Drop the (GH #426) tags from module-level docstrings.

---

Minor

MDUpdateAction is missing NEW = "0"

Standard FIX defines MDUpdateAction as New=0 / Change=1 / Delete=2. The PR defines only Change and Delete, so a server-sent action="0" would hit the unknown-action drop path (which is the correct safe behavior). This is fine, but the enum comment should document the omission — otherwise a future developer will wonder if "0" is a bug or intentional.

class MDUpdateAction(StrEnum):
    """Tag 279 — action for an incremental market-data entry (35=X).

    FIX 5.0SP2 also defines New=0; Kalshi MD does not emit it, so it is
    intentionally absent — an action="0" will be dropped as unknown.
    """
    CHANGE = "1"
    DELETE = "2"

test_market_data_request_unsubscribe — inconsistent assertion style

# uses .value (string comparison)
assert body[int(Tag.SUBSCRIPTION_REQUEST_TYPE)] == SubscriptionRequestType.DISABLE.value
# neighboring test uses enum directly
assert body[int(Tag.SUBSCRIPTION_REQUEST_TYPE)] == SubscriptionRequestType.SNAPSHOT_PLUS_UPDATES

Pick one style consistently (enum direct comparison is cleaner).

Missing golden wire test for subscribe (263=1)

test_golden_wire_market_data_request covers snapshot (263=0) and unsubscribe_all (263=2) but not subscribe (263=1). A single-liner would close the gap:

assert _wire(MarketDataRequest.subscribe([TICKER])) == f"263=1|146=1|55={TICKER}"

---

Design Note (not blocking)

_BookState.to_view() sorts on every get() call — O(n log n) per invocation. For typical prediction markets the book is tiny, so this is fine. If FixOrderBook ever sees high-throughput read patterns, a dirty-flag cache would help. Not a concern now, just flagging for the record.

---

What's excellent

• The "check action FIRST" ordering in apply_incremental is exactly right — the comment explaining the invariant (lines 607–608) is one of the rare cases where a comment adds real value.
• apply_snapshot replacing a whole _BookState (not mutating in place) gives the per-market snapshot-replacement contract cleanly and atomically.
• Golden exact-wire tests + structural round-trip tests at the same time is the right combination.
• The gap-recovery integration test (test_market_data_gap_recovery_resubscribe) is the hardest scenario and it's covered end-to-end.
• FixOrderBook.apply(msg: object) accepting any decoded message silently is ergonomic and correct — consumers don't need a type switch.

Priority of the above: the MDIncrementalEntry required-field fragility is the only item I'd want resolved before merge (or at least explicitly annotated as a known Kalshi-specific assumption). Everything else is minor polish.

[claude]

Code Review — PR #434: FIX market-data session + order-book reconstruction

Overall verdict: looks solid. The design is clean, the test coverage is thorough, and the edge-case handling is clearly thought-through. A few findings below, roughly in priority order.

---

Findings

1. apply_incremental — missing test for the md_entry_px is None path

The code has an explicit guard at orderbook.py:617-620:

px = entry.md_entry_px
if px is None:
    logger.debug("FIX MD incremental %s: entry without MDEntryPx; dropping", ...)
    continue

There is no test that exercises this branch. The delete-without-size test covers md_entry_size=None for DELETE, but there is no equivalent for a missing price on CHANGE or DELETE. Since the field is declared optional for inbound robustness, this path is reachable.

Suggestion: Add a test analogous to test_orderbook_incremental_delete_without_size_parses_and_applies but omitting MDEntryPx (270).

---

2. FixOrderBook.remove() — no test

The remove(symbol) method is public API and the module docstring mentions it as part of the unsubscribe/settlement path, but there is no test covering it. Given it delegates to dict.pop, the risk is low, but for a public contract it's worth a single test.

---

3. MDUpdateAction intentionally omits New=0 — worth a module-level note

The enum docstring on MDUpdateAction documents this correctly:

FIX 5.0SP2 also defines New=0; Kalshi MD does not emit it, so it is
intentionally absent (an action='0' is dropped as unknown).

This is fine for current behavior. The risk is that a future Kalshi spec revision that starts emitting New=0 (add-level semantics) would silently drop those entries rather than applying them. The docstring and the unknown-action test both make this visible, but it's worth a note in CHANGELOG.md or a # TODO: add New=0 comment if/when the spec is promoted. Not blocking, just flagging.

---

4. MDReqRejReason covers only codes 2 and 4

Standard FIX 5.0SP2 defines more codes (0=unknown symbol, 1=dup req ID, 3=exceed max req, etc.). The raw str storage means unknown codes still parse without error, which is the right inbound robustness call. But the enum docstring doesn't say "Kalshi-emitted subset only" — a future reader seeing only 2 and 4 might think the enum is complete.

Suggestion: Add a line to the MDReqRejReason docstring: "(Kalshi-emitted subset of FIX 5.0SP2 tag 281.)" — same pattern would apply if other enums are similarly constrained.

---

5. unsubscribe_all() relies on groupfield() defaulting to []

MarketDataRequest.unsubscribe_all() calls cls(subscription_request_type=...) without related_symbols. This works because groupfield() provides an empty-list default. The golden-wire test confirms the behavior, so this is not broken — but it's an implicit coupling. If groupfield() ever changes its default behavior the failure would be non-obvious.

Not requesting a change here — just noting the dependency is tested and not a bug.

---

6. FixOrderBook is not concurrency-safe (undocumented)

_books is a plain dict with no locking. For the asyncio context this is safe since there's no concurrent mutation across event-loop iterations. But the class docstring doesn't mention this constraint. If someone tries to use it from multiple threads (e.g., feeding from a background reader thread), the lack of a lock would be silently racy.

Suggestion: One sentence in the class docstring: "Not thread-safe; designed for use from a single asyncio event loop." — consistent with how FixSession and the WS client are scoped.

---

Minor / Nits

• _for is a private classmethod that calls the public constructors snapshot() / subscribe() / unsubscribe() — actually the reverse: the constructors call _for. The symbol validation happens in the caller, not in _for, which is fine but means _for itself could construct a zero-symbol request internally. The existing tests cover the validation path, so no bug.
• test_golden_wire_snapshot_full_refresh wraps the f-string assertion with a leading assert _wire(msg) == — unnecessary f prefix on the string literal (f"55={TICKER}|...") since TICKER is already formatted in. Not a bug, just style.
• test_market_data_gap_recovery_resubscribe is the most integration-heavy test and doubles as a doc example for the reconnect contract — that's a good use of the integration harness.

---

What's well done

• The "check action FIRST" comment in apply_incremental explains a non-obvious invariant cleanly.
• Atomic snapshot replacement: building a fresh _BookState before assigning to self._books[symbol] means the old book is replaced only when the new one is fully built, and no partial state leaks on a continue.
• Golden wire fixtures pinned to doc examples are the right call — they catch field-order and group-layout drift that symmetric round-trips can't.
• The stale-survival contract test (test_orderbook_resubscribe_without_clear_leaves_stale_book) explicitly documents a footgun rather than hiding it, which is excellent.
• The apply() dispatch method is a clean ergonomic wrapper that avoids type-switching at every call site.

---

Summary: Ready to merge after the apply_incremental missing-price-test gap (finding 1) is addressed — or with the acknowledgement that it's accepted as a known gap. The other items are suggestions, not blockers.