feat(fix): surface inbound decode failures (closes #432)

[TexasCoding]

Summary

Make a malformed inbound application message observable instead of silently dropped. decode_app_message(raw) returns None for two distinct cases — (1) no registered model (admin / unimplemented flow) and (2) a registered model whose payload failed schema validation (e.g. a DollarDecimal in an off-spec format). Wired into a production consumer, case (2) means a genuine ExecutionReport could be lost to a single bad field with only a logger.warning. This implements the issue's recommended option 3 (an on_decode_error hook on FixSession), plus a strict decoder for direct callers.

Changes

• errors.py — FixDecodeError (carries raw + msg_type, chains the underlying validation error via __cause__) for the registered-but-malformed case.
• dispatch.py — decode_app_message_strict(raw) raises FixDecodeError on a malformed known message (returns None only for an unregistered type). decode_app_message now delegates to it and swallows — its lenient None contract is unchanged.
• FixSession — optional on_decode_error: (RawMessage, FixDecodeError) -> Awaitable[None]. When set, the session decodes each inbound app message to detect a malformed known one and routes it there (the raw is still delivered to on_message, so nothing is lost). Zero behaviour change when unset. Threaded through every client session factory (order_entry/drop_copy/market_data/post_trade/rfq/session).

async def on_decode_error(raw, exc):  # dead-letter / alert / halt
    ...
async with client.order_entry(on_message=handle, on_decode_error=on_decode_error) as session:
    ...

Trade-off: setting on_decode_error makes the session decode each inbound app message once (to detect failures); pair it with on_message for the happy path. No cost when the hook is unset.

Tests (tests/fix/test_decode_routing.py, 7 new)

decode_app_message_strict valid / malformed-raises / unregistered-None; decode_app_message still swallows; and FixSession routing — a malformed ExecutionReport surfaces via on_decode_error (raw still delivered), a valid report doesn't trigger it, and without the hook the raw is still delivered and the session stays active.

Verification

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

Closes #432. Part of #402.

🤖 Generated with Claude Code

[claude]

Code Review

Overall: Well-scoped, clean implementation of option 3 from #432. The three-layer design (strict decoder → lenient wrapper → session hook) is the right shape for this problem. A few items worth discussing before merge.

---

Design concern: double-decode in the happy path

The most impactful issue. In session.py:_process (lines 636–645), decode_app_message_strict(raw) is called and its return value is discarded on every inbound app message whenever on_decode_error is set — even when the message is perfectly valid:

if self._on_decode_error is not None:
    try:
        decode_app_message_strict(raw)   # result thrown away on happy path
    except FixDecodeError as exc:
        ...
if self._on_message is not None:
    await self._on_message(raw)          # consumer re-decodes from raw

This means a FixSession with on_decode_error set pays a full Pydantic validation round-trip per inbound app message even when nothing is wrong. On a busy order-entry session receiving hundreds of ExecutionReports per second, that's real overhead. The PR description acknowledges the cost but understates it — "one decode per app message" is a full Pydantic model validation, not a cheap operation.

Options:

1. Accept it. Document the cost more explicitly in the FixSession docstring so users can make an informed trade-off.
2. Return the decoded result from the error hook so it can be re-used. Change DecodeErrorHandler to receive the decoded FixMessage | None as a third argument (for non-error invocations), letting consumers skip re-decoding in on_message. Breaking change, but cleaner.
3. Lazy-decode: store the Result[FixMessage, FixDecodeError] on the raw message itself or in a local variable and thread it to on_message. More invasive.

Option 1 is fine for v1 of the feature, but the docstring on FixSession.session() / the factory methods should warn callers.

---

Minor issues

FixDecodeError.raw is typed Optional but always non-None when raised

# errors.py
def __init__(
    self, message: str, *, raw: RawMessage | None = None, msg_type: str | None = None
) -> None:

decode_app_message_strict always passes raw=raw (non-None). Making both raw and msg_type optional widens the type unnecessarily and means callers have to null-check before using them. Consider either removing the default Nones or providing a narrower overload. At minimum, the attributes could be RawMessage (non-optional) since the constructor is only called from one place.

test_session_no_decode_error_on_valid_message relies on implicit ExecutionReport leniency

The test pushes only ORDER_ID (tag 37) into an ExecutionReport. If Pydantic's model is later tightened to require additional fields, this test will silently start triggering on_decode_error instead of failing with a clear assertion. It would be more robust to use the same avg_px fixture as the valid case in test_strict_decode_valid_message:

await acceptor.push("8", [(int(Tag.AVG_PX), "0.66")], seq=2)

Missing test: on_decode_error callback itself raises → on_message still fires

The production code at session.py:644 catches and logs a raising on_decode_error callback and then continues to deliver to on_message. That exception-isolation behavior has no test coverage.

Comment style

The comment block at session.py:136–138 explains what the code does more than why. The inline # See GH #432 is the load-bearing bit — the multi-line explanation above it reads like a docstring. CLAUDE.md convention is one short line for non-obvious WHY; the GH reference alone would suffice here.

---

What's working well

• The dispatch.py refactor is clean: decode_app_message now delegates to decode_app_message_strict and swallows, which is the right inversion — no logic duplication.
• FixDecodeError.__cause__ chaining is correct; consumers can inspect the original ValidationError.
• All six session factory methods (order_entry, drop_copy, market_data, post_trade, rfq, session) are consistently updated.
• TYPE_CHECKING import guard in errors.py avoids a circular import correctly.
• The three session-level integration tests cover the meaningful behavioral cases (malformed routed + raw still delivered, valid no-trigger, no-hook backward compat).

---

Summary

The double-decode cost is the only issue I'd want resolved before merge — either accept it with explicit documentation, or find a way to reuse the decoded result. The other items are minor polish. The feature itself is the right fix for #432.

[claude]

Code Review: feat(fix): surface inbound decode failures (closes #432)

Overall: This is a clean, well-scoped change that solves a real observability gap. The design is solid, conventions are followed, and the backward-compatible hook pattern is the right approach.

---

What the PR does

Adds an on_decode_error hook to FixSession (and all session factory methods) that fires when a registered application message (e.g. an ExecutionReport) fails Pydantic validation. Previously both "unregistered type" and "registered but malformed" collapsed to a silent None. Now the two cases are distinguishable: decode_app_message_strict returns None only for unregistered, raises FixDecodeError for malformed. The session still delivers the raw to on_message regardless.

---

Issues / observations

1. Double-decode when on_decode_error is set (result discarded)

In session.py:644-658, decode_app_message_strict(raw) is called but its return value is thrown away, so the consumer decodes again in on_message. The PR description acknowledges this trade-off. Passing the decoded result to the hook as an optional third argument would eliminate the redundancy without breaking anything. Not a blocker, but worth a follow-up issue.

2. Test count mismatch

The PR description says "7 new" tests, but test_decode_routing.py contains 8 test functions. Minor.

3. No test for on_decode_error set without on_message

When on_decode_error is set but on_message is None, a malformed message is routed to the hook but the raw is not delivered anywhere else. This is probably the right behavior, but it is an untested edge case that could surprise callers.

4. FixDecodeError docstring length

The docstring is unusually long for an exception class in this codebase and repeats information already in the dispatch.py docstring. Could be trimmed. Not a blocker.

---

What is well done

• Backward-compatible: zero behavior change when on_decode_error is unset; lenient decode_app_message contract preserved.
• Exception chaining: raise FixDecodeError(...) from exc correctly threads __cause__; tested explicitly.
• Isolation: buggy hooks do not tear down the session (verified by test_session_survives_raising_decode_error_callback).
• TYPE_CHECKING guard on RawMessage import in errors.py avoids a circular import correctly.
• Export hygiene: FixDecodeError and decode_app_message_strict are wired through __init__.py at all levels.
• Session factories: on_decode_error is threaded consistently through all five factory methods.
• Test coverage: happy path, malformed-raises, unregistered-None, lenient-unchanged, and three session routing scenarios all covered.

---

Verdict

Approve with minor notes. The double-decode and missing edge-case test are worth a follow-up issue but not PR-blocking. The core design is correct and the implementation is clean.