[TexasCoding]

Closes #296.

What

_apply_snapshot_inplace wrapped msg.msg.yes / msg.msg.no in dict(...), silently nullifying the ~5× speedup CHANGELOG v2.5.0 #263 advertises. For a 200-level book that's 400 needless re-hashes and reallocs per snapshot on the recv hot path.

Why it's safe

OrderbookSnapshotPayload._levels_to_dict already produces a typed dict[Decimal, Decimal]. The _BookState becomes the canonical owner of the dict after _apply_snapshot_inplace and mutates it in place on every delta.

Subscriber-side aliasing: raw consumers of subscribe_orderbook_delta who receive an OrderbookSnapshotMessage and hold .msg.yes / .no will observe future mutations — docstrings on both subscribe_orderbook_delta and OrderbookSnapshotPayload document this. Use client.orderbook(ticker) when an immutable snapshot view is needed.

Tests

New TestSnapshotIdentityAdoption pins:

• state.yes is msg.msg.yes (identity, no defensive copy)
• subsequent _apply_delta_inplace mutates the adopted dict in place
• cache invalidation via state.invalidate() still fires
• subscriber-held msg.msg.yes observes a subsequent delta (aliasing contract)

Part of the post-v2.5 audit closure (Wave W1).

[claude]

Code Review — PR #306: perf(ws): adopt orderbook snapshot dicts by identity

Overview

Removes the redundant dict(...) copy in _apply_snapshot_inplace that was silently undoing the single-walk optimization introduced in #263. The change is a 2-line production edit backed by two new regression tests. The safety argument is sound and well-scoped.

---

Correctness & Safety ✅

The optimization is correct. _levels_to_dict (orderbook_delta.py:13) already constructs a fully typed dict[Decimal, Decimal] in a single walk. The dict(...) copy on lines 126–127 (old code) was a redundant second hash-pass with no safety benefit, because:

1. The OrderbookSnapshotMessage is discarded immediately after _apply_snapshot_inplace returns in the recv hot path — no external references can observe later in-place delta mutations through msg.msg.yes.
2. _BookState is internal and owns the dict exclusively after adoption.

---

Issues to Fix Before Merge

1. Ruff E302: missing blank line before tests/ws/test_orderbook.py:366

        assert len(book.no) == 1
                                   ← only 1 blank line here
class TestSnapshotIdentityAdoption:

PEP 8 (and ruff E302) require two blank lines before a top-level class definition. This will fail CI. Add one more blank line.

2. Comment references issue numbers — violates CLAUDE.md style

# #263/#296: the validator returns these dicts already typed as
# ``dict[Decimal, Decimal]``; adopt them by identity. The model is
# discarded immediately after this call so there are no external
# references that would observe later in-place delta mutations.

CLAUDE.md: "Don't reference the current task, fix, or callers… since those belong in the PR description and rot as the codebase evolves." The issue references #263/#296 are fine for the PR description (where they already appear) but not in source. The safety rationale in the second sentence is the non-obvious WHY worth keeping. Suggested trim:

# Adopt by identity — validator already produced dict[Decimal, Decimal],
# and the model is discarded immediately after this call so nothing
# outside _BookState can observe later in-place delta mutations.

---

Test Quality ✅ (with notes)

The two new tests in TestSnapshotIdentityAdoption are well-designed:

• test_snapshot_adopts_validated_dicts_by_identity: correctly scopes the assertion to state.yes is msg.msg.yes rather than state.yes is yes_in, with a comment explaining why the Pydantic-level identity isn't asserted. This is exactly the right boundary to pin.
• test_delta_mutates_in_place_and_invalidates_cache: exercises the full mutation cycle and cache invalidation — good end-to-end coverage of the hot path.

Subtle note on the identity fast path in _levels_to_dict: When yes_in = {Decimal(...): Decimal(...)} is passed via model_validate, the all(type(k) is Decimal ...) fast path on orderbook_delta.py:37 returns the same dict unchanged. This means msg.msg.yes is yes_in is state.yes in the test — yes_in is implicitly aliased to live book state. Not a bug here (since yes_in is a local that isn't mutated after snapshot apply), but future tests that pass a Decimal dict directly and then mutate it could corrupt book state unexpectedly. A comment near the yes_in declaration flagging this implicit coupling would help future maintainers.

---

Summary

Item	Status
Optimization correctness	✅ Sound
Safety argument	✅ Valid
Ruff E302 blank line	❌ Will fail CI — must fix
Comment issue references	⚠️ Minor CLAUDE.md violation
Test coverage	✅ Good
mypy / types	✅ No type changes

Two items to address before merge: the missing blank line (CI-blocking) and trimming the issue numbers from the inline comment. The core change is correct and well-motivated.

[TexasCoding]

Round-2 follow-up pushed as 77cf673:

• Trimmed the snapshot-adopt comment in kalshi/ws/orderbook.py from 4 lines to 3 and dropped the #263/#296 references (issue refs live in the PR/commit metadata, not in source).
• The E302 finding on tests/ws/test_orderbook.py was a false positive — uv run ruff check . was already clean on the prior commit and remains clean now.

Verified: ruff check ., mypy kalshi/, and pytest tests/ws/test_orderbook.py all green.

[claude]

Code Review

Overview

This PR removes two dict(...) defensive copies in _apply_snapshot_inplace, adopting the validator-built dict[Decimal, Decimal] by identity instead of rebuildling it. The reasoning is sound: _levels_to_dict already produces the right type, and _BookState becomes the sole owner of the dict. The regression tests in TestSnapshotIdentityAdoption are well-targeted.

---

Issues

1. Comment overstates a caller contract as a code invariant (minor)

kalshi/ws/orderbook.py, new comment:

# Adopt by identity — validator already produced dict[Decimal, Decimal],
# and the model is discarded immediately after this call so nothing
# outside _BookState can observe later in-place delta mutations.

"the model is discarded immediately after this call" is a requirement on the caller, not something this function enforces or can know. The msg object (and its msg.yes/msg.no aliases) can outlive this call if the caller holds a reference — e.g., a recv loop that logs on error, or a future test that inspects msg after calling _apply_snapshot_inplace directly.

The aliasing is safe in the current codebase, but the comment as written will mislead the next reader who asks "what if I hold msg after this call?" A tighter phrasing:

# Adopt by identity — the validator already produced dict[Decimal, Decimal].
# Callers must not access msg.msg.yes/.no after this call; _BookState takes
# ownership and will mutate the dict in-place on every subsequent delta.

2. The docstring for _apply_snapshot_inplace does not mention the ownership transfer

The method's docstring is otherwise thorough, but there is no mention that _BookState now takes ownership of msg.msg.yes / msg.msg.no by reference. Future callers that wrap this method need to know this. One sentence in the docstring is enough:

Ownership of msg.msg.yes and msg.msg.no is transferred to the new _BookState;
callers must treat those dicts as consumed after this call returns.

3. Missing blank line before TestSnapshotIdentityAdoption (style)

test_public_apply_snapshot_returns_orderbook ends at line 362, and class TestSnapshotIdentityAdoption starts immediately on line 364 with only one blank line. PEP 8 / project style requires two blank lines between top-level definitions. Running ruff check would catch this.

---

Strengths

• Correct ownership reasoning. _books[ticker] is the only live reference to _BookState, and _BookState.yes/.no are the only references to the adopted dicts, so the aliasing can't escape to a caller.
• Good regression test. state.yes is msg.msg.yes pins the SDK-level contract cleanly, and the comment correctly avoids asserting state.yes is yes_in since Pydantic's internal dict identity is not the contract being tested.
• Cache-invalidation coverage. test_delta_mutates_in_place_and_invalidates_cache verifies the #244 cache path still fires correctly after the identity-adopt — this is the most likely place a refactor could silently break things.
• Scoped change. Two lines of production code changed, directly traceable to the fix.

---

Verdict

Approve with nits. The correctness argument is solid. The two documentation gaps (inline comment and docstring) and the missing blank line are the only things worth addressing before merge. None of them affect runtime behavior.

[TexasCoding]

Round-2 follow-up (876da84):

• Reframed the inline comment in _apply_snapshot_inplace as an ownership contract (no longer leans on 'model is discarded by the caller').
• Added an ownership-transfer note to the docstring so future wrappers know msg.msg.yes / .no are consumed.
• Item Add async test coverage (AsyncTransport, AsyncKalshiClient, async resources) #3 (E302 before TestSnapshotIdentityAdoption) was a false alarm — ruff check is clean both before and after this commit. No change needed there.

[claude]

Code Review — PR #306: perf(ws): adopt orderbook snapshot dicts by identity

Overview

The 2-line core change is correct and well-motivated. dict(msg.msg.yes) / dict(msg.msg.no) was quietly undoing the single-walk optimisation introduced in v2.5.0 (#263), adding a full per-level rehash/realloc on every snapshot. Removing those wraps and adopting the Pydantic-validated dicts directly is the right fix.

---

Issues

🟡 Correctness concern — aliasing hazard for raw subscribe_orderbook_delta consumers

After this PR, snapshot.msg.yes and _BookState.yes are the same object. The ownership contract is documented on the private _apply_snapshot_inplace, but the recv loop then dispatches snapshot to the public subscriber queue (see client.py:404-414):

self._orderbook_mgr._apply_snapshot_inplace(snapshot, sid=snapshot.sid)
pre_validated = snapshot
await self._dispatcher.dispatch(data, pre_validated=pre_validated)  # → sub.queue.put(snapshot)

subscribe_orderbook_delta() returns AsyncIterator[OrderbookSnapshotMessage | OrderbookDeltaMessage], so consumers who receive OrderbookSnapshotMessage objects and access .msg.yes / .no after storing the message will see future delta mutations reflected in those dicts.

Before this PR there was an implicit immutability guarantee (_BookState.yes was a copy; snapshot.msg.yes was untouched). That guarantee is now gone without any public-facing documentation.

The high-level orderbook() iterator is not affected (it goes through mgr.get() → to_orderbook() which builds a fresh Orderbook with new sorted lists). But the raw channel is now hazardous for hold-and-inspect patterns.

Suggested mitigation: add a one-line note to subscribe_orderbook_delta's docstring and/or OrderbookSnapshotPayload that .yes/.no are live dicts owned by the manager after dispatch, and that client.orderbook() should be used when snapshot immutability is needed.

---

🟡 Test comment contradicts its own assertion

In test_snapshot_adopts_validated_dicts_by_identity:

# NOTE: Pydantic may re-bind the validator output to a fresh model
# field — we don't control that and shouldn't assert it.
mgr._apply_snapshot_inplace(msg)
state = mgr._books["T"]
assert state.yes is msg.msg.yes   # ← but this IS asserting identity
assert state.no is msg.msg.no

The comment creates an apparent contradiction. The intent (which is fine) is:

• We're not asserting state.yes is yes_in — Pydantic may or may not copy during field assignment.
• We are asserting state.yes is msg.msg.yes — the SDK must not make a second copy on top of whatever Pydantic stores.

The comment should reflect that distinction:

# We don't assert state.yes is yes_in (Pydantic may copy the input during
# field binding), but we DO assert state.yes is msg.msg.yes: the SDK must
# adopt the already-validated dict without adding another dict() copy (#296).

---

🔵 Missing blank lines before new test class (PEP 8 / project style)

class TestSnapshotIdentityAdoption: should be preceded by two blank lines per PEP 8 and project convention. The diff shows only one blank line after the previous method's final assert.

---

🔵 Inline comment duplicates docstring

The ownership note appears in both the method docstring and as an inline comment on the same 2-line block:

"""
...
Ownership note: ``msg.msg.yes`` / ``.no`` are adopted by identity
into the new ``_BookState``; callers must treat them as consumed.
"""
...
# Adopt msg.msg.yes / .no by identity. _BookState takes ownership and
# mutates them in place on every delta — callers must not access these
# dicts on `msg` after this returns.
yes_levels = msg.msg.yes

Per CLAUDE.md, the inline comment is redundant given the docstring already covers it. One or the other is sufficient.

---

What's correct and doesn't need changing

• The two-line dict(...) removal is minimal and surgical — nothing else in _apply_snapshot_inplace or apply_snapshot accesses msg.msg.yes / .no after the adoption.
• apply_snapshot (the public wrapper) only reads msg.msg.market_ticker post-call — no aliasing there.
• The _levels_to_dict fast-path analysis is accurate: the BeforeValidator returns the same object when given an already-Decimal-keyed dict, and pydantic-core's dict validation doesn't re-copy when types match — so the test's yes is msg.msg.yes assertion is reliable.
• test_delta_mutates_in_place_and_invalidates_cache is well-structured: snapshot → materialise cache → apply delta → assert in-place mutation + cache invalidation. Correctly covers the interaction.

---

Summary

Core fix	✅ Correct and well-motivated
Aliasing hazard for raw subscribers	🟡 Needs a docstring note on the public API
Test comment accuracy	🟡 Minor — clarify what's being asserted and why
Blank lines / inline comment duplication	🔵 Trivial style

The aliasing concern is the only one that might surprise real users; the others are polish. Happy to approve once the public-API docstring is updated.

[TexasCoding]

Round-3 follow-up at b548a5b:

• Aliasing hazard (correctness): added docstring notes on subscribe_orderbook_delta and OrderbookSnapshotPayload warning that msg.yes/.no are live dicts owned by OrderbookManager after dispatch; pointed raw subscribers at client.orderbook() for an immutable view.
• Redundant inline comment: dropped from _apply_snapshot_inplace — ownership note already in the docstring.
• Test comment: rephrased per your suggested wording so it no longer reads as contradicting the state.yes is msg.msg.yes assertion.
• Blank-line claim: re-verified uv run ruff check . — clean. PEP 8 / E302 enforced; no diagnostic on the new class. Skipping.

Verified: ruff clean, mypy clean, tests/ws/test_orderbook.py 26 passed.

[claude]

Code Review

Overview

This PR removes two dict(...) copy calls in _apply_snapshot_inplace that were redundant because _levels_to_dict already produces a fully-typed dict[Decimal, Decimal]. The fix is correct, the perf motivation is sound, and the new tests enforce the identity constraint against future regression. A few things worth flagging below.

---

Correctness

The core change is safe. _levels_to_dict has an identity fast-path:

if all(type(k) is Decimal and type(v) is Decimal for k, v in value.items()):
    return value  # same object — no rehash

Wrapping that result in dict(...) was rebuilding a dict that was already correct and typed, which is exactly the redundancy the PR removes. ✓

---

Aliasing Hazard — PR Description Is Slightly Inaccurate

The PR description says:

"the Pydantic model is discarded immediately after _apply_snapshot_inplace returns"

That's not what happens. Looking at the dispatch path in client.py (~line 442):

self._orderbook_mgr._apply_snapshot_inplace(snapshot, sid=snapshot.sid)
pre_validated = snapshot
# ...
await self._dispatcher.dispatch(data, pre_validated=pre_validated)

The snapshot is enqueued for subscribers — not discarded. This means a subscriber who calls async for msg in stream: and receives an OrderbookSnapshotMessage holds a msg.msg.yes that is the live dict mutated by every future delta. The docstrings added to subscribe_orderbook_delta and OrderbookSnapshotPayload correctly document this, but the PR description should be corrected to match. If someone reads the description to assess safety and misses the docstrings, they'll draw the wrong conclusion.

Consider: is this the intended consumer contract? Subscribing via the raw subscribe_orderbook_delta stream now hands callers a mutable alias into the internal book state. A caller who does:

async for msg in stream:
    if isinstance(msg, OrderbookSnapshotMessage):
        saved_yes = msg.msg.yes  # this dict mutates on every delta!

will observe aliasing bugs without an obvious cause. The docstring warns "they mutate on every subsequent delta" — but this is a class-level invariant that IDEs and type checkers won't surface. Worth considering whether the subscriber copy should be cheap enough to keep (it's one dict() call on a pre-built map, not the inner loop), or whether this contract is acceptable given that orderbook() is the recommended high-level API.

---

Test Coverage

Good: test_snapshot_adopts_validated_dicts_by_identity — pins the identity assertion from the _BookState side. This is the right regression test.

Good: test_delta_mutates_in_place_and_invalidates_cache — verifies in-place mutation and cache invalidation together. ✓

Missing — subscriber-side aliasing test: The documented footgun (subscriber's snapshot dict mutates on delta) has no test. A test like:

def test_subscriber_snapshot_dict_is_live_after_delta(self) -> None:
    mgr = OrderbookManager()
    msg = OrderbookSnapshotMessage.model_validate(...)
    mgr._apply_snapshot_inplace(msg)
    subscriber_ref = msg.msg.yes  # simulate what a raw-stream subscriber stores
    mgr._apply_delta_inplace(make_delta(price="0.50", delta="25", side="yes"))
    # Aliasing contract: subscriber_ref mutated because it IS state.yes
    assert subscriber_ref[Decimal("0.50")] == Decimal("125.00")

This would explicitly document the contract and prevent future "safety" refactors from accidentally breaking it by reintroducing a copy.

Minor — blank line before new test class: PEP 8 requires two blank lines between top-level class definitions. The new TestSnapshotIdentityAdoption class is missing the blank line between it and the previous class's last line. Ruff should catch this on CI, but worth checking.

---

Documentation

The docstring additions are accurate and well-placed:

• subscribe_orderbook_delta — correctly points users to orderbook() for immutable views. ✓
• OrderbookSnapshotPayload — aliasing note is accurate. ✓
• _apply_snapshot_inplace — ownership note matches the new semantics. ✓

---

Summary

Category	Assessment
Core correctness	✅ Correct — the dict() copy was redundant
Performance claim	✅ Accurate — removes O(n) rehash on the recv hot path
Test coverage	⚠️ Solid but missing subscriber-side aliasing test
PR description accuracy	⚠️ "discarded immediately" is wrong — model is dispatched to subscribers
Style	⚠️ Missing blank line before new test class
Aliasing contract	⚠️ Correct but worth deciding whether raw-stream subscribers should see a live dict

The change is ready to merge with the minor nits above. The aliasing contract question is the only one worth a deliberate decision before merging.

[TexasCoding]

Round 4 follow-ups (6fb6115):

• PR description updated: "discarded immediately" replaced with the actual contract — _BookState is the canonical owner after _apply_snapshot_inplace, and raw subscribe_orderbook_delta subscribers who hold msg.msg.yes / .no will observe future mutations. Pointer to client.orderbook(ticker) for immutable snapshot views.
• Subscriber-side aliasing test added (test_subscriber_held_snapshot_dict_mutates_on_delta): pins the documented live-dict contract so a future "defensive copy" refactor fails the test instead of silently changing the contract.
• Blank-line nit (4th flag): false positive. uv run ruff check . is clean on this branch and has been every round; PEP 8’s two-blank-lines rule applies between top-level classes, and the file does have the required two blank lines before class TestSnapshotIdentityAdoption. Skipping.
• "Should we keep the copy?" design question: keeping the perf gain + documenting the aliasing is the deliberate decision. The docstrings on subscribe_orderbook_delta and OrderbookSnapshotPayload make the contract explicit, the new test enforces it, and client.orderbook(ticker) remains the supported immutable view. Skipping.

uv run ruff check ., uv run mypy kalshi/, and uv run pytest tests/ws/test_orderbook.py all green.

[claude]

Code Review — PR #306: perf(ws): adopt orderbook snapshot dicts by identity

Overview

This PR removes two dict(...) copy operations in _apply_snapshot_inplace, letting _BookState adopt the Pydantic-validated dict[Decimal, Decimal] by reference instead of rebuilding it. The optimization is real and the reasoning is sound — _levels_to_dict already produces the right type in a single pass, so the previous dict(msg.msg.yes) was a full second hash+realloc for every snapshot.

---

What's Good

• Surgical: only two lines changed in the hot path; zero unrelated cleanup.
• Well-documented: docstrings added to subscribe_orderbook_delta, OrderbookSnapshotPayload, and _apply_snapshot_inplace all explain the aliasing contract.
• Tests pin the invariant: TestSnapshotIdentityAdoption fails immediately if a future refactor reintroduces a dict(...) copy. The comment "We don't assert state.yes is yes_in" is the right call — pydantic-core's identity guarantee on the outer container is not part of the public API.

---

Issues

1. Behavioral change for apply_snapshot callers is undocumented (moderate)

The public apply_snapshot(msg) also calls _apply_snapshot_inplace. After this PR, passing a snapshot message to the public method means msg.msg.yes is now owned (and will be mutated) by the _BookState. The docstring on apply_snapshot doesn't mention this:

def apply_snapshot(self, msg: OrderbookSnapshotMessage, ...) -> Orderbook:
    """Initialize (or reset) a book from a full snapshot.

    Public wrapper that mutates in place then materializes...
    """

The same aliasing warning added to subscribe_orderbook_delta and OrderbookSnapshotPayload should also appear here, since a caller doing mgr.apply_snapshot(msg); use(msg.msg.yes) will get the same surprise.

2. OrderbookManager class docstring is now misleading (minor)

The class-level docstring says:

Each call to apply_snapshot, apply_delta, or get returns a fresh Orderbook instance. Consumers may safely retain references; subsequent updates will not mutate previously-returned books.

This is still true for the returned Orderbook objects, but could lead a reader to believe the input message's fields are also safe to retain. A brief clarifying parenthetical would help:

(This guarantee applies to returned Orderbook instances; msg.msg.yes / .no fields on input snapshot messages are adopted by reference — see OrderbookSnapshotPayload.)

3. Missing CHANGELOG entry (minor)

This is a perf win with a user-visible semantic change (the aliasing contract). Given CHANGELOG is updated per release (CHANGELOG.md), this seems like it warrants an entry even as a pre-release note, similar to how v2.5.0 / #263 was called out.

---

Questions / Edge Cases Worth Confirming

Re-snapshot on same ticker: If the server sends a second snapshot for the same ticker (gap recovery), _apply_snapshot_inplace creates a new _BookState with the new message's dicts. A raw subscriber still holding the first snapshot's msg.msg.yes now holds a detached dict (no longer in _books["T"]). This is correct behavior but worth a sentence in the docstring — the live-dict contract is scoped to the most recent snapshot.

_levels_to_dict identity is pydantic-core-version-dependent: The fast path returning value directly only avoids a copy if pydantic-core doesn't reconstruct the dict during dict[Decimal, Decimal] field validation. This is currently true and the test pins it — just worth noting this is an implementation-detail assumption about pydantic-core, not a guarantee.

---

Summary

The optimization is correct, the tests are solid, and the documentation is mostly good. The main gap is that apply_snapshot (public method) inherits the aliasing behavior but doesn't document it, which could surprise callers who use the public API directly. No blocking issues — the two docstring additions and a CHANGELOG line would make this PR complete.

[claude]

Code Review — PR #306: adopt orderbook snapshot dicts by identity

Summary: The optimization is legitimate and well-motivated — removing a redundant dict(...) copy from the recv-loop hot path is real work saved (O(n) per snapshot level). The tests are well-designed and pin the right invariants. A few things worth flagging before merge:

---

Breaking public API contract (moderate)

apply_snapshot() is documented as a public method in the class docstring usage example:

book = mgr.apply_snapshot(snapshot_msg)  # Initialize

After this PR, any caller who retains snapshot_msg.msg.yes after calling apply_snapshot() will see future mutations through that reference. The old dict(msg.msg.yes) copy made this safe. The new "callers must treat them as consumed" ownership note in the docstring is the right disclosure, but it's easy to miss at the call site, and it's a behavioral breaking change for existing callers.

Suggestion: either (a) keep the dict() copy inside apply_snapshot() and only remove it from _apply_snapshot_inplace (the recv-loop private entry point that warrants the optimization), or (b) bump the minor version and call this out in CHANGELOG.md as a breaking change for direct apply_snapshot() callers.

---

_BookState docstring not updated

The class docstring at orderbook.py:29 still says:

"Kept separate from the public Orderbook model so the manager can mutate freely without leaking changes to previously-handed-out snapshots."

This is still true for Orderbook objects returned by get() / apply_delta(), but the new aliasing behavior of the input msg.msg.yes / .no is unmentioned here. A single sentence pointing at OrderbookSnapshotPayload would complete the picture.

---

PEP 8: two blank lines between top-level classes

The diff adds only one blank line between the end of the previous class's last method and class TestSnapshotIdentityAdoption. PEP 8 requires two blank lines between top-level class definitions.

---

Identity guarantee relies on Pydantic internals (minor, well-mitigated)

The test comment is honest about this:

# We don't assert state.yes is yes_in (Pydantic may copy the input during
# field binding), but we DO assert state.yes is msg.msg.yes

The identity from _levels_to_dict's fast path through to _BookState depends on Pydantic-core not re-copying an already-typed dict[Decimal, Decimal]. The regression test pins this correctly and will catch it if a Pydantic upgrade changes the behavior. No action needed — just worth being aware of.

---

What's done well

• Tests are strong. TestSnapshotIdentityAdoption covers the right three cases: identity adoption, in-place mutation + cache invalidation, and the live-dict aliasing contract for raw subscribers. The third test is particularly valuable — it documents observable behavior that would otherwise be a silent footgun.
• Docstring coverage is thorough. Three locations updated (subscribe_orderbook_delta, OrderbookSnapshotPayload, _apply_snapshot_inplace) ensures whoever reads any of the three entry points encounters the aliasing warning.
• The _levels_to_dict fast path is correctly exploited. The PR description correctly traces the full chain: validator builds the dict → Pydantic stores by identity → _apply_snapshot_inplace adopts by identity. No redundant walk.
• apply_snapshot docstring "Ownership note" is the right approach; just needs a CHANGELOG callout.

---

Suggested follow-up (not blocking)

If apply_snapshot() is meant to remain truly safe for external callers: keep the dict() copy there and only strip it from _apply_snapshot_inplace. The benchmark win on the recv hot path comes from the private path; the public API doesn't appear in tight loops.

[TexasCoding]

Round-5 fix (53393c3): applied the bot's suggested design — keep the defensive dict() copy inside the public apply_snapshot() so caller-supplied msgs are not aliased, and confine the identity-adoption optimization to _apply_snapshot_inplace (which the recv loop calls directly per #199). Public-API safety contract restored.

• apply_snapshot() copies state.yes/.no after _apply_snapshot_inplace returns.
• Docstrings on apply_snapshot and OrderbookManager no longer warn about aliasing on the public path.
• Raw subscribe_orderbook_delta consumers still see the live dict (they bypass apply_snapshot); docstring there + on OrderbookSnapshotPayload keep that warning.
• New test_public_apply_snapshot_does_not_alias_input_msg pins the restored contract.
• The three existing TestSnapshotIdentityAdoption tests still pin the recv-loop identity adoption on _apply_snapshot_inplace.

Blank-line nit is the fifth false positive; confirmed ruff clean.

[claude]

Review: perf(ws): adopt orderbook snapshot dicts by identity (#296)

Overall this is a well-reasoned performance fix with solid test coverage. The two-tier design — identity adoption on the recv-loop hot path, defensive copy on the public API — is the right call. A few things worth addressing before merge.

---

Correctness

The core optimization is correct. _apply_snapshot_inplace adopting msg.msg.yes/.no by identity instead of calling dict(...) on them is safe because OrderbookSnapshotPayload._levels_to_dict already produces a dict[Decimal, Decimal]. No duplicate walk needed.

The apply_snapshot defensive copy is correct but the sequencing is worth noting: _apply_snapshot_inplace sets state.yes = msg.msg.yes, then apply_snapshot immediately does state.yes = dict(state.yes). Net result for the public path: one identity assignment + one dict copy = same wall-clock cost as before. The perf win is 100% recv-loop–only, which matches the claim. The comment makes this clear — good.

---

Issues

1. Redundant local imports in test_public_apply_snapshot_does_not_alias_input_msg

def test_public_apply_snapshot_does_not_alias_input_msg(self) -> None:
    from decimal import Decimal                                      # already top-of-file
    from kalshi.ws.models.orderbook_delta import (
        OrderbookDeltaMessage, OrderbookSnapshotMessage,            # already top-of-file
    )
    from kalshi.ws.orderbook import OrderbookManager                # already top-of-file

All four symbols are imported at the module level (tests/ws/test_orderbook.py lines 5–12). These local imports are dead weight — remove them.

2. Missing blank line before TestSnapshotIdentityAdoption

The class has only one blank line separating it from the previous test method. PEP 8 and the rest of this file use two blank lines before top-level class definitions. Add the second blank line.

3. Aliasing behavior is a behavioral change — CHANGELOG omitted

Raw subscribe_orderbook_delta consumers who retain msg.msg.yes / .no after receiving a snapshot frame will now observe future delta mutations through those references. Before this PR, dict(...) in _apply_snapshot_inplace prevented that. The docstring updates are good, but this is a user-visible behavioral change that should appear in CHANGELOG under the upcoming release (even as a note, not a breaking change, since subscribe_orderbook_delta was already documented as a lower-level API).

---

Minor observation (no action needed)

The test_subscriber_held_snapshot_dict_mutates_on_delta test documents the aliasing contract clearly. One thing it does not cover: what happens when a second snapshot for the same ticker arrives over the recv loop? After the second _apply_snapshot_inplace, state is replaced with a new _BookState, so the previously adopted dict is orphaned and the subscriber's held reference no longer aliases anything live. That's arguably the safer direction (subscription view freezes at the first snapshot), but it may surprise someone who expects continuous aliasing across server-side resets. Worth a comment in the test or a short mention in the docstring (_apply_snapshot_inplace ownership note).

---

Summary

Optimization correctness	✅
Public-path safety	✅
Test coverage of stated contracts	✅
Redundant imports in one test	❌ clean up
Missing blank line	❌ trivial fix
CHANGELOG entry	❌ add one

[claude]

Code Review — PR #306: perf(ws): adopt orderbook snapshot dicts by identity

Overview

This PR removes the redundant dict() copy in _apply_snapshot_inplace, letting the recv-loop hot path adopt the already-validated dict[Decimal, Decimal] from msg.msg.yes / .no by identity. The correctness of this is grounded in _levels_to_dict's identity fast-path, which already returns the same object when keys/values are already Decimal. The public apply_snapshot wrapper adds a defensive copy after inplace adoption so callers keep a stable view. The design split is clean and well-reasoned.

---

Correctness ✅

The core logic is sound:

• _levels_to_dict returns the same dict object when input is already dict[Decimal, Decimal], so msg.msg.yes is already the canonical owner.
• _apply_snapshot_inplace assigns it directly → state.yes IS msg.msg.yes. Subsequent deltas mutate in place.
• apply_snapshot then does state.yes = dict(state.yes) which breaks the alias before materializing. The caller's msg.msg.yes is stable after return.
• Cache lifecycle is correct: _BookState is freshly constructed on snapshot (_cached = None), so the defensive copy does not strand a stale Orderbook in _cached.

---

Issues

1. Missing blank lines before TestSnapshotIdentityAdoption (ruff E302)

The diff shows only one blank line between the end of the previous class and the new one. PEP 8 and ruff require two blank lines between top-level class definitions:

        assert len(book.no) == 1
                                      # ← only one blank line here
class TestSnapshotIdentityAdoption:

This will fail ruff check. Add a second blank line.

---

2. Redundant inline imports in test_public_apply_snapshot_does_not_alias_input_msg

def test_public_apply_snapshot_does_not_alias_input_msg(self) -> None:
    from decimal import Decimal
    from kalshi.ws.models.orderbook_delta import (
        OrderbookDeltaMessage,
        OrderbookSnapshotMessage,
    )
    from kalshi.ws.orderbook import OrderbookManager

All four of these are already available at module scope for the other tests in the same file. Inline imports suggest they were copy-pasted from a standalone script. Move them to the module-level imports to match the file's existing style and avoid the confusion of "why are these special here?"

---

3. Minor clarity nit in apply_snapshot defensive copy

self._apply_snapshot_inplace(msg, sid=sid)
ticker = msg.msg.market_ticker
state = self._books[ticker]
state.yes = dict(state.yes)   # state.yes IS msg.msg.yes at this point
state.no = dict(state.no)

After _apply_snapshot_inplace, state.yes is msg.msg.yes — so dict(state.yes) and dict(msg.msg.yes) are identical operations. Writing dict(msg.msg.yes) is marginally clearer because it makes the "we're copying the input" intent explicit rather than the reader needing to know the ownership invariant. Not a blocker, just something to consider.

---

4. No CHANGELOG entry for the aliasing contract change

The behavioral change — that raw subscribe_orderbook_delta consumers holding msg.msg.yes after a snapshot will now observe future delta mutations — is a non-trivial user-visible contract shift. The docstrings document it well, but the CHANGELOG has no entry for this PR. Given the active release cadence (v2.5.0 just shipped), this warrants at least a short entry under the next release noting the aliasing contract for raw stream consumers.

---

What's Good

• The public/recv-loop split is the right design: identity-adopt on the hot path, defensive copy on the public API boundary. This avoids regressing the perf win where it matters while keeping the public API safe.
• test_subscriber_held_snapshot_dict_mutates_on_delta explicitly pinning the aliasing contract as a test is exactly the right move — it turns the behavior into a documented invariant.
• test_public_apply_snapshot_does_not_alias_input_msg is a solid regression guard for the defensive copy.
• Docstrings on _apply_snapshot_inplace, apply_snapshot, subscribe_orderbook_delta, and OrderbookSnapshotPayload all consistently describe the ownership model.

---

Summary

Two items need fixing before merge: the missing blank line (will break ruff) and the redundant inline imports. The CHANGELOG gap is worth addressing. The core logic is correct and well-tested.

[TexasCoding]

Round-6 fix (5250428):

• Dropped redundant inline imports in test_public_apply_snapshot_does_not_alias_input_msg.
• Rewrote the defensive copy as dict(msg.msg.yes) / dict(msg.msg.no) to make "copy the caller's input back out" the visible intent.
• Blank-line claim re-verified false (6th time): there are TWO blank lines before TestSnapshotIdentityAdoption at line 366; ruff check tests/ws/test_orderbook.py clean. The diff context likely shows only one.
• CHANGELOG entry deferred to v2.6.0 release packaging per the active audit-closure plan (release cutter handles it across all 7 issues, not per-PR).

Ready for merge.

[claude]

Code Review — PR #306: perf(ws): adopt orderbook snapshot dicts by identity

Overview

This PR eliminates a redundant dict(...) copy in _apply_snapshot_inplace (the recv-loop hot path) and compensates with a targeted defensive copy in the public apply_snapshot() call site. The design is sound and the test coverage is good. A few things worth flagging before merge.

---

Correctness

The adopt-then-copy pattern in apply_snapshot is correct but non-obvious.

The flow is:

1. _apply_snapshot_inplace runs → state.yes = msg.msg.yes (same object)
2. Back in apply_snapshot: state.yes = dict(msg.msg.yes) — since state.yes is msg.msg.yes at this point, this is identical to dict(state.yes). The alias is broken; caller's dict is preserved.

The comment explains this, but the code says "copy via msg.msg to make the 'copy the caller's input' intent explicit" — which is a bit circular (you're copying from the caller's dict that IS state.yes). It would be marginally clearer to write state.yes = dict(state.yes) with a comment explaining why the two are equivalent here, but this is a style preference, not a bug.

to_orderbook() is unaffected. It iterates self.yes.items() so it always uses whichever dict state.yes currently points to — no change needed there.

---

Behavioral Breaking Change — Not Called Out in CHANGELOG

This is the most important flag. Raw subscribe_orderbook_delta consumers who hold msg.msg.yes or msg.msg.no after receiving an OrderbookSnapshotMessage will now observe future delta mutations. Previously the dict was a defensive copy; now it is a live alias into _BookState.

The docstrings document this, but the CHANGELOG does not. For any consumer relying on the old snapshot dicts being immutable, this is a silent behavioral break. Recommend adding a note under the upcoming version entry — even a single bullet in a "Breaking / Behavioral Changes" section.

---

Test Coverage

Tests are excellent — all four scenarios are pinned:

• Identity adoption (state.yes is msg.msg.yes) ✓
• Delta mutates in-place + cache invalidation ✓
• Raw subscriber aliasing contract ✓
• Public apply_snapshot defensive copy ✓

Minor nits:

• Missing a blank line before class TestSnapshotIdentityAdoption — inconsistent with the separator style used elsewhere in the test file.
• The make_delta helper used in the new tests is not in the diff; it presumably exists earlier in the file. Worth a quick sanity-check that it creates a fully valid OrderbookDeltaMessage (price, delta, side, sid matching the snapshot's sid=1).

---

Performance Claim

The PR claims "~5× speedup" for a 200-level book. To be precise: the 5× claim applies to the dict construction portion of _apply_snapshot_inplace (removing 400 re-hashes), not the whole function. The overall recv-loop improvement will be smaller in absolute wall-clock terms once you account for Pydantic validation, socket I/O, etc. The claim in the PR description is reasonable as a component-level characterization; just ensure the CHANGELOG doesn't overstate it as a whole-function 5× speedup.

---

Spec / Pydantic Stability Note

The identity contract depends on OrderbookSnapshotPayload._levels_to_dict returning a dict[Decimal, Decimal] and Pydantic field access returning the same object (not a defensive copy). This holds for Pydantic v2 model fields today. If a future Pydantic version or model config change (e.g., frozen=True) were to make field access return copies, the identity guarantee would silently break. The new test test_snapshot_adopts_validated_dicts_by_identity pins this behavior and would catch a regression — good.

---

Summary

Area	Verdict
Core correctness	✅ Correct
Recv-loop perf win	✅ Real and meaningful
Public API safety	✅ Defensive copy preserves contract
Test coverage	✅ Comprehensive
CHANGELOG	❌ Behavioral change not documented
Style	Minor: missing blank line before test class
Docstrings	✅ Aliasing contract clearly documented

Recommend: Add a CHANGELOG entry calling out the aliasing behavior change for raw subscribe_orderbook_delta consumers before merging. Everything else is in good shape.

[TexasCoding]

Round-7 review: zero new blocking items.

• CHANGELOG entry: handled by the v2.6.0 release-packaging step in the audit-closure plan (one entry across all 7 issues, not per-PR). Already documented in PR description.
• Blank line before TestSnapshotIdentityAdoption: 6th false positive. Verified locally: lines 364-365 are both blank (nl -ba confirms), and uv run ruff check tests/ws/test_orderbook.py is clean. The bot is reading from the diff context which only shows one of the two.
• dict(state.yes) vs dict(msg.msg.yes): round-6 asked for dict(msg.msg.yes) for clearer intent; round-7 now says dict(state.yes) would be marginally clearer. Picking the round-6 version with the explanatory comment and moving on.

Merging.