fix(ws): raise KalshiSubscriptionError from run_forever() with no subscribe (#175)

[TexasCoding]

Closes #175.

The foot-gun

KalshiWebSocket.run_forever() previously returned immediately when _recv_task was None:

async def run_forever(self) -> None:
    """Block until the connection is closed. Use with callback API."""
    if self._recv_task:
        await self._recv_task

_recv_task is only set inside _do_subscribe() (via _ensure_recv_loop()), so any session that registered an @ws.on(channel) callback but never called a subscribe_* would have _recv_task is None → silent return. No messages, no error, no signal.

The docs themselves propagated the trap. docs/websockets.md showed exactly this pattern:

@ws.on("ticker")
async def on_ticker(msg: TickerMessage) -> None: ...

async with ws.connect():
    await ws.run_forever()    # silently returns

Registering an @ws.on() callback doesn't tell the server to send frames — only subscribe_* does that. So the callback never fires, and run_forever() returns immediately with no signal.

Fix (option B per the issue)

Raise KalshiSubscriptionError at the call site with an actionable message:

run_forever() requires at least one active subscription.
Call subscribe_ticker(...) / subscribe_trade(...) / etc. (or the
generic subscribe(channel, ...)) before run_forever() so the recv
loop has something to drain. Registering an @ws.on(channel) callback
does not subscribe — the server only sends frames for channels you
explicitly subscribe to.

Errors at the obvious failure point beat silent no-ops.

Docs

docs/websockets.md callback example now shows the correct pairing:

@ws.on("ticker")
async def on_ticker(msg: TickerMessage) -> None:
    print(msg.msg.yes_bid)

async with ws.connect() as session:
    # Subscribing is what tells the server to send frames; the @ws.on
    # callback above is purely the routing destination. The iterator
    # returned by subscribe_ticker is unused here — callbacks fan out
    # alongside iterators, so registering the callback is enough.
    await session.subscribe_ticker(tickers=["EXAMPLE-25-T"])
    await session.run_forever()

Plus a follow-on note pointing at the new error behavior.

README WS section was sanity-checked — only shows the iterator-style pattern, no foot-gun to fix there.

Test

tests/ws/test_client.py::TestRunForever::test_run_forever_without_subscription_raises replaces the prior test_run_forever_returns_immediately_without_subscribe (which was asserting the buggy behavior). The companion test_run_forever_blocks_until_close still exercises the correct subscribe-then-run-forever path.

Soft-breaking footprint

Code that relied on run_forever() returning silently as a sleep-until-disconnect for a connection it never intended to use for streaming will now raise. There's no production-shaped usage of that — the foot-gun was the bug.

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.

Out of scope

The sibling foot-gun #177 (run_forever() lacks graceful shutdown / signal-driven stop path) stays in its own issue. This PR is purely the silent-no-op fix.

[claude]

Code Review — PR #185: fix(ws): raise KalshiSubscriptionError from run_forever() with no subscribe

Overview

A clean, well-scoped fix for a genuine foot-gun: run_forever() now raises KalshiSubscriptionError instead of silently returning when no subscription has been established. The PR description is thorough, the scope is tightly bounded, and it ships with a correct test inversion. No concerns blocking merge — a few nits follow.

---

Code correctness ✅

kalshi/ws/client.py — The guard is correct and placed at the right layer:

if self._recv_task is None:
    raise KalshiSubscriptionError(...)
await self._recv_task

_recv_task is only ever set by _ensure_recv_loop() (called from _do_subscribe()), so None reliably means "no subscribe call has landed yet." The check is safe under asyncio's cooperative scheduling — no race between the None check and the await.

One untested edge case worth being aware of (not a blocker): if _recv_task is set but has already completed before run_forever() is called (e.g. the connection dropped mid-session), await self._recv_task returns immediately. That's arguably correct behaviour — the session is over — but the docstring doesn't mention it. Consider noting it if you add any graceful-shutdown work downstream (issue #177).

---

Style / conventions

Docstring length — CLAUDE.md says "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." The new run_forever() docstring is 8 lines across three paragraphs plus a :raises: block. The :raises: annotation is genuinely useful for API consumers, but the prose paragraphs duplicate what the error message already says at runtime. A trimmed version would stay within conventions:

async def run_forever(self) -> None:
    """Block until the recv loop terminates. Use with the callback API.

    :raises KalshiSubscriptionError: No ``subscribe_*`` call has landed yet.
        The recv loop starts lazily inside subscribe; an ``@ws.on()``
        callback alone does not subscribe.
    """

Test docstring — same guideline applies; the multi-paragraph block explaining the pre-fix behaviour is clear intent but exceeds the one-line-max convention. The test name test_run_forever_without_subscription_raises already tells the story.

Incidental # type: ignore[no-untyped-def] on fake_ws — this wasn't in the original signature and is unrelated to the fix. Fine to keep if mypy was flagging it, but it looks like scope creep if it wasn't.

---

Test coverage ✅

The test inversion is correct:

• test_run_forever_without_subscription_raises — asserts the new error with pytest.raises(KalshiSubscriptionError, match="at least one active subscription"). No asyncio.wait_for timeout needed since the raise is synchronous; removing it is right.
• test_run_forever_blocks_until_close — happy path still exercises the subscribe-then-run-forever flow.

The match string "at least one active subscription" is specific enough to catch regressions without being fragile.

Missing: no test covers calling run_forever() on an already-completed _recv_task (dropped connection scenario). Low priority but worth adding if you address #177.

---

Documentation ✅

docs/websockets.md fix is correct. The async with ws.connect() as session pattern, paired with the inline comment explaining that the iterator is unused when callbacks are registered, is exactly right. The follow-on note about KalshiSubscriptionError closes the loop for readers.

---

CHANGELOG ✅

Entry is accurate and well-placed under Unreleased. The "Soft-breaking" callout is appropriate — any code depending on the silent return was already broken by definition.

---

Summary

Area	Status
Correctness	✅ Clean fix, right layer
Tests	✅ Inversion is correct; edge case (completed task) optional
Docs	✅ Callback example now shows correct pairing
CHANGELOG	✅ Accurate and complete
Style	⚠️ Docstrings exceed CLAUDE.md one-line-max convention

The style nits are minor and don't affect correctness. LGTM otherwise.