test(coverage): backfill WS edge cases and small auth/type/config branches

[TexasCoding]

Summary

Wave 2 #102: closes the umbrella audit-Q backlog covering 7 small coverage gaps across WS edge cases, auth, type validators, and small base-helper branches.

+10 tests across 7 files:

Item	File	Coverage
F-Q-09	tests/test_auth.py	PermissionError on unreadable key, encrypted-PEM passphrase path
F-Q-10	tests/test_types.py (new)	DollarDecimal BeforeValidator raises TypeError for non-coercible inputs
F-Q-12	tests/test_client.py + test_async_client.py	sync + async transport
F-Q-13	tests/ws/test_client.py + test_integration.py	5 of 7 sleep-then-assert sites converted to asyncio.Event
F-Q-15	tests/ws/test_integration.py	two subscribe_orderbook_delta with different tickers, distinct sids via session._sub_mgr.active_subscriptions
F-Q-16	tests/test_base_helpers.py	current crash path pinned for bool and int mistypes
F-Q-18	tests/ws/test_integration.py	force disconnect + reject_auth → iterator exits via StopAsyncIteration within timeout

Notes

• F-Q-13: 2 of 7 sleep sites left in place — they are negative timing assertions ("task still pending" / "recv loop still alive") that can't be event-driven.
• F-Q-10 deviation: Pydantic BeforeValidator does NOT wrap a non-ValueError exception in ValidationError — the TypeError propagates raw. Tests assert TypeError directly. Issue body's mention of ValidationError wrapping was incorrect for current Pydantic behavior.
• Stayed out of Wave 3's territory — no edits inside kalshi/ws/{client,dispatch,orderbook,channels}.py.
• Bugs surfaced: none.

Closes #102

Test plan

• Suite: 1622 passed (+10)
• uv run ruff check . clean
• uv run mypy kalshi/ clean (strict)

[claude]

Code Review — PR #134: Backfill WS edge cases and small auth/type/config branches

Overall: LGTM. This is clean, focused coverage work with no production-code changes. The sleep→Event conversions are a genuine quality improvement. A few minor items below.

---

What the PR does

• Converts 5 of 7 asyncio.sleep sites to asyncio.Event-driven waits — making those tests deterministic rather than time-dependent.
• Adds 10 new tests across 7 files covering: auth error wrapping, DollarDecimal/FixedPointCount type fallbacks, is_authenticated, _join_tickers crash paths, dual-ticker orderbook subscriptions, and sentinel-on-reconnect-failure.

---

Positives

• Event-based waits in test_on_decorator_registers, test_on_error_called, test_gap_detection_fires_callback, test_callback_and_iterator_coexist, test_error_message_triggers_callback are exactly right — asyncio.wait_for(event.wait(), timeout=2.0) is strictly better than sleep(0.2/0.3).
• skipif(os.geteuid() == 0) guard in test_permission_denied_wraps_with_helpful_message correctly handles root-bypass semantics in CI.
• The note in test_reconnect_failure_broadcasts_sentinel_to_iterators that the pre-disconnect message may or may not be buffered is correct and the assertion received == [42] or received == [] is the right call. The comment explains the non-determinism clearly.
• test_types.py correctly documents the Pydantic BeforeValidator deviation (non-ValueError propagates unwrapped) — this was a genuine gotcha worth pinning.

---

Minor / Nits

1. Multi-paragraph docstrings in test_base_helpers.py

test_non_string_bool_element_raises_unhelpful_type_error and test_non_string_int_element_raises_unhelpful_type_error both have 6-line multi-paragraph docstrings. CLAUDE.md says "never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The WHY is genuine here (pinning crash path so a future fix trips the test), but it can be condensed to one line each:

def test_non_string_bool_element_raises_unhelpful_type_error(self) -> None:
    # Pins crash path: bool fails "," in elem check; update if validation is added.
    with pytest.raises(TypeError, match=r"argument of type 'bool' is not iterable"):
        _join_tickers([True, "A"])  # type: ignore[list-item]

2. Module-level docstring in test_types.py

Nine lines for what could be two. The key facts (TypeError propagates unwrapped, lines 27/60 are the branches) are worth keeping, but the "Note:" paragraph restates what the test names already say. Same CLAUDE.md guideline.

3. session._sub_mgr access in test_two_subs_same_channel_distinct_params

Reaching into _sub_mgr.active_subscriptions is the only way to verify distinct sids, so it's justified — just worth noting for future readers that this is an implementation-detail assertion that will break if the sub manager is restructured. No change needed; mentioning for awareness.

4. Insertion-order assumption in test_two_subs_same_channel_distinct_params

sids = [sub.server_sid for sub in session._sub_mgr.active_subscriptions.values()]
sid_a, sid_b = sids[0], sids[1]

This relies on dict insertion order (guaranteed in Python 3.7+, project requires 3.12, so it's safe). Still, if the test starts flapping, this is the first place to look.

---

No issues found in

• Retry/error-handling logic (no production code changed).
• Auth error wrapping — from_key_path correctly wraps PermissionError and detects encrypted keys at lines 72 and 85 in kalshi/auth.py.
• DollarDecimal/FixedPointCount tests correctly target the raise TypeError(...) branches at kalshi/types.py:27,60.
• is_authenticated tests are correct against the property at _base_client.py:110,225.

---

Summary

The sleep→Event conversion alone justifies this PR. The new coverage is accurate and the tricky behavioral deviations (Pydantic TypeError, _join_tickers crash, non-deterministic reconnect timing) are documented appropriately. Docstring length is the only style drift from CLAUDE.md. Ready to merge after addressing or explicitly accepting the nits.

[TexasCoding]

Addressed:

• ✅ Multi-paragraph docstring on test_non_string_bool_element_raises_unhelpful_type_error — trimmed to a one-line inline comment.
• ✅ test_types.py module docstring — 9 → 1 line; the Pydantic-doesn't-wrap note was redundant with the test names.

Acknowledged (informational only, no change):

• session._sub_mgr.active_subscriptions access is an implementation-detail assertion — noted in your review; agreed it's justified since there's no public surface for sid inspection.
• Insertion-order assumption is safe (Python 3.12 required); first place to look if the test ever flaps.

[claude]

Code Review — PR #134: test(coverage): backfill WS edge cases and small auth/type/config branches

Overall: Approve with minor notes. This is a well-scoped, test-only PR. The asyncio.Event conversion is the highlight — it eliminates an entire class of timing-based flakiness. No production code was changed.

---

What's Working Well

• asyncio.Event conversion (F-Q-13): Replacing asyncio.sleep(0.2/0.3) with asyncio.Event + asyncio.wait_for(..., timeout=2.0) is exactly the right approach. Tests are now deterministic, faster, and self-documenting. Good discipline to leave the two negative-timing sites alone and explain why.

• test_auth.py: Both new tests are solid. The skipif(os.geteuid() == 0, ...) guard is correct and necessary — CI often runs as root. The finally block in both tests properly restores chmod and unlinks, preventing test pollution even on failure.

• test_types.py: Clean new file. Good call documenting the Pydantic BeforeValidator deviation in the PR description — the spec said ValidationError but raw TypeError propagates. Testing what actually happens (not what was hoped) is the right call.

• Reconnect-failure test: test_reconnect_failure_broadcasts_sentinel_to_iterators is thorough and honest about the timing non-determinism in assert received == [42] or received == []. The pytest.fail(...) path gives a clear, actionable failure message if the sentinel is never broadcast.

---

Minor Concerns

1. Crash-path tests pin known-bad behavior (test_base_helpers.py)

def test_non_string_bool_element_raises_unhelpful_type_error(self) -> None:
    # Pins crash path: bool fails `"," in elem` check; update if validation is added.
    with pytest.raises(TypeError, match=r"argument of type 'bool' is not iterable"):
        _join_tickers([True, "A"])

Asserting that _join_tickers throws an unhelpful TypeError for non-string inputs sets a low bar. If someone later adds proper input validation (which this tests arguably argues for), these tests will break as a reminder to update them — that part is fine. But consider whether a GitHub Issue tracking "add type validation to _join_tickers" would be more actionable than pinning the crash. The tests are correct as written; this is a judgment call.

2. Accessing session._sub_mgr in test_two_subs_same_channel_distinct_params

sids = [
    sub.server_sid
    for sub in session._sub_mgr.active_subscriptions.values()
]

This reaches into a private attribute to verify internal subscription tracking. It works, and for this kind of structural assertion there's not always a better option. One alternative: assert only on observable behavior (both iterators yield the correct messages, already done below), and drop the sids inspection entirely. The msg_a.msg.market_ticker == "T1" / msg_b.msg.market_ticker == "T2" assertions at the end already prove the routing is correct without coupling to internals.

3. asyncio.timeout vs asyncio.wait_for inconsistency

test_reconnect_failure_broadcasts_sentinel_to_iterators uses asyncio.timeout(5.0) context manager, while all other converted tests use asyncio.wait_for(..., timeout=2.0). Both are fine on Python 3.12+, but the mixed style is a minor inconsistency. Not a blocker.

4. assert sid_a is not None and sid_b is not None is vacuous given prior assertion

assert len(sids) == 2
assert sids[0] != sids[1]
sid_a, sid_b = sids[0], sids[1]
assert sid_a is not None and sid_b is not None  # this line

sids[0] != sids[1] already proves neither is None (since None != None is False). The extra is not None guard reads as defensive noise. Consider removing it.

---

No Issues Found

• type: ignore[list-item] / type: ignore[arg-type] suppressions are correctly scoped.
• asyncio.timeout is safe given requires-python = ">=3.12".
• All sleep-replaced sites use timeout=2.0, which is generous without being absurd.
• Staying out of Wave 3 territory (no edits to ws/{client,dispatch,orderbook,channels}.py) is correct scoping.

---

Summary

Area	Verdict
asyncio.Event conversions	✅ Excellent
Auth error coverage	✅ Clean
Type validator coverage	✅ Clean
is_authenticated coverage	✅ Trivial but correct
Crash-path pinning tests	⚠️ Minor: consider a tracking issue instead
Private _sub_mgr access	⚠️ Minor: could assert on behavior alone
sid_a is not None guard	⚠️ Nit: vacuous, remove
asyncio.timeout vs wait_for	⚠️ Nit: minor style inconsistency

Ready to merge after addressing the nit on the vacuous is not None check. Other items are judgment calls, not blockers.

🤖 Generated with Claude Code

[claude]

Code Review — PR #134 (Wave 2 Coverage: WS edge cases, auth, types, base helpers)

Overall this is a clean, well-scoped wave of coverage tests. The asyncio.sleep → event-driven conversion is the most impactful improvement and is done correctly throughout. A few items worth addressing before merge:

---

Issues

1. Potential sid-ordering assumption in test_two_subs_same_channel_distinct_params

sids = [
    sub.server_sid
    for sub in session._sub_mgr.active_subscriptions.values()
]
sid_a, sid_b = sids[0], sids[1]
# then sends sid_a → T1 message, sid_b → T2 message
assert msg_a.msg.market_ticker == "T1"
assert msg_b.msg.market_ticker == "T2"

This silently assumes that active_subscriptions.values() returns sids in subscribe-call order (T1 first, T2 second). While CPython 3.7+ dicts are insertion-ordered, the test is relying on the subscription manager's internal insertion behavior rather than the observable contract. If subscriptions are ever stored in a different structure or keyed non-sequentially, this test could falsely pass with swapped sids.

Suggestion: Match sids back to streams by looking up the client sid from the subscribe call's return value, or add a brief comment that calls out the insertion-order assumption explicitly.

---

2. Loose assertion in test_reconnect_failure_broadcasts_sentinel_to_iterators

assert received == [42] or received == []

The comment explains the timing rationale, but received in ([42], []) is more idiomatic and a bit clearer in pytest failure output. More importantly: the test's primary contract (iterator terminates) is correctly verified via the timeout, but the loose data assertion means a bug that delivers the message twice ([42, 42]) would silently pass. Worth tightening to assert len(received) <= 1 or at minimum the in form.

---

3. No tracking issue for the pinned crash paths in test_base_helpers.py

The two _join_tickers tests explicitly pin broken behavior:

def test_non_string_bool_element_raises_unhelpful_type_error(self) -> None:
    # Pins crash path: bool fails `"," in elem` check; update if validation is added.
    with pytest.raises(TypeError, match=r"argument of type 'bool' is not iterable"):

CLAUDE.md says all bugs/enhancements are tracked in GitHub Issues. There doesn't appear to be an issue filed for adding proper input validation to _join_tickers. Please open one and reference it in the test comment (e.g., # See #NNN) so the next person who hits this knows it's a known gap, not an oversight.

---

Minor nits

• Style inconsistency: The new reconnect test uses async with asyncio.timeout(5.0) while every other test in the same file uses asyncio.wait_for(..., timeout=X). Both are valid on Python 3.12, but the inconsistency is surprising. Not a blocker, but worth noting.

• Mixed comment styles in test_base_helpers.py: The bool case uses an inline # comment; the int case uses a docstring. Pick one.

• test_async_client.py — no close() call: The new tests don't call await client.close(). The sync equivalents do call client.close(). This is actually consistent with the adjacent test_no_auth_constructs_unauthenticated (which also doesn't close), and calling async close from a sync test method isn't straightforward — so this is fine, just noting it for completeness.

---

What's good

• Event-driven waits replace 5 asyncio.sleep() calls cleanly. The 2 remaining sleep sites are correctly left (negative timing assertions — event-driven conversion would be wrong there).
• PermissionError test correctly skips when running as root with a clear reason string.
• Encrypted PEM test asserts both the error type and key message fragments ("Passphrase-protected" and "openssl pkey"), which is the right level of specificity.
• test_types.py correctly documents that Pydantic BeforeValidator does not wrap TypeError in ValidationError — that's a non-obvious Pydantic behavior and the test comment helps future maintainers.
• All finally blocks clean up temp files properly, including chmod restore before unlink on the permission test.

---

Verdict: Approve with the _join_tickers issue filed and the received assertion tightened. The sid-ordering assumption is the most fragile thing here — at minimum a comment is needed.