Market data review followups

[jkirkmannz]

No description provided.

[Copilot]

🟡 Changes recommended

The inline-review workflow lacks pull-request write permission, and documentation inconsistencies remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request completes market-data follow-up work with simulator, ticker-normalization, and SSE routing fixes, expanded regression coverage, documentation updates, and Claude workflow changes.

Changes:

• Adds market-data tests for SSE behavior, concurrency, full watchlists, timestep scaling, and ticker normalization.
• Updates implementation, documentation, and development dependencies.
• Adjusts Claude review workflow configuration.

File summaries

File	Description
planning/MARKET_DATA_REVIEW.md	Records review results and coverage.
planning/MARKET_DATA_DESIGN.md	Documents market-data architecture and testing.
backend/uv.lock	Locks HTTP dependencies.
backend/tests/market/test_stream.py	Adds SSE coverage.
backend/tests/market/test_simulator.py	Adds full-watchlist coverage.
backend/tests/market/test_simulator_source.py	Tests interval-scaled timesteps.
backend/tests/market/test_models.py	Applies formatting updates.
backend/tests/market/test_massive.py	Tests ticker normalization.
backend/tests/market/test_cache.py	Tests concurrent updates.
backend/tests/conftest.py	Removes a deprecated fixture.
backend/pyproject.toml	Adds httpx development dependency.
backend/app/market/stream.py	Creates independent routers.
backend/app/market/simulator.py	Scales simulator timestep with interval.
backend/app/market/massive_client.py	Normalizes startup tickers.
.github/workflows/claude.yml	Updates a commented CLI example.
.github/workflows/claude-code-review.yml	Enables inline review comments.

Review details

Suppressed comments (9)

.github/workflows/claude.yml:49

• This example changed the Claude Bash permission pattern from the colon form to Bash(gh pr *). The action's documented command patterns use a colon before the argument wildcard (for example, Bash(gh pr comment:*)); users copying this comment may not grant the intended permission. Restore the colon form or list the specific subcommands.

          # claude_args: '--allowed-tools Bash(gh pr *)'

backend/tests/market/test_stream.py:1

• The module is labeled as an integration test, but it does not exercise an ASGI app or transport; it directly drives _generate_events and the route endpoint. Rename this to endpoint-level/unit tests so the test type is accurately communicated.

"""Integration tests for the SSE price streaming endpoint.

planning/MARKET_DATA_DESIGN.md:1027

• The implementation checks the cache version and yields a data event only when that version changes (lines 1041–1047); it does not send all prices on every interval. This docstring is therefore an inaccurate API description and should state that the loop checks every interval and sends only on changes.

    Sends all prices every `interval` seconds. Stops when the client
    disconnects (detected via request.is_disconnected()).

planning/MARKET_DATA_DESIGN.md:96

• The as-built file tree omits the new backend/tests/market/test_stream.py module, so this document still describes the pre-follow-up test layout. Add the stream test file here so the documented repository structure matches the PR.

      test_massive.py

planning/MARKET_DATA_DESIGN.md:1291

• This gap is also closed by test_cache.py::test_concurrent_updates_are_not_lost, which was added in this PR. Remove or rewrite this bullet so the design document does not tell future contributors to implement a test that already exists.

- No dedicated concurrent-writer stress test for `PriceCache` (lock
  correctness is verified by inspection, not empirically under contention).

planning/MARKET_DATA_DESIGN.md:671

• The as-built SimulatorDataSource.start() now derives dt from self._interval; omitting that argument here makes this design snippet silently revert to the fixed 500 ms timestep whenever it is followed. Add the same interval-derived dt calculation and pass it to GBMSimulator.

    async def start(self, tickers: list[str]) -> None:
        self._sim = GBMSimulator(tickers=tickers, event_probability=self._event_prob)

planning/MARKET_DATA_REVIEW.md:7

• This says the second pass “fixed all six” issues, but the table immediately below says issue 4 was deliberately left unchanged and §4 repeats that it remains open. Please make the status say five were fixed and one was intentionally accepted so the review record is internally consistent.

This is the third pass on this document. The first pass found six issues (§1 below); the second pass fixed all six and added regression tests for the two behavioral bugs. This third pass closes out the remaining items that were previously left open on purpose (§2): an SSE integration test, a `PriceCache` concurrency test, and a full-10-ticker `GBMSimulator` test. The market data subsystem is now considered complete and ready for the rest of the backend to build on.

planning/MARKET_DATA_REVIEW.md:49

• The rationale here says the concurrent test matches production because MassiveDataSource calls into the cache via asyncio.to_thread, but only _fetch_snapshots runs in that worker; _cache.update(...) executes after the await on the event-loop thread (backend/app/market/massive_client.py:97-108). This is still a useful defensive stress test, but the explanation should not claim it models the current poller's cache writes.

`tests/market/test_cache.py::test_concurrent_updates_are_not_lost` spins up 8 real OS threads (matching how the cache is actually used — `MassiveDataSource` calls into it via `asyncio.to_thread`), each performing 200 `update()` calls across 10 shared tickers, then asserts `cache.version` exactly equals `8 * 200 = 1600`. This is a meaningful assertion, not a smoke test: a broken lock (or a `+=` race) would show up here as a version count *less than* 1600 — a lost update — with high probability under real thread interleaving. Ran the full suite three times back-to-back to confirm no flakiness.

planning/MARKET_DATA_REVIEW.md:32

• This is described as an SSE integration test, but the added tests intentionally avoid an ASGI app/transport and call the private generator plus the routed endpoint function directly. Rename the heading to endpoint/unit tests (and update the module docstring) so the coverage claim does not imply transport-level integration is covered.

### 2.1 SSE integration test for `stream.py` (was 31% coverage, no tests)

Getting this right took a real detour worth recording: the natural first attempt — `httpx.ASGITransport` and, separately, FastAPI's `TestClient` — both **deadlock** against this endpoint. `_generate_events` is an unbounded `while True` loop that only exits when it observes `request.is_disconnected()`. Both of those test clients fully run the ASGI call to completion (buffering the entire response) *before* handing anything back to the caller to consume — there is no mechanism for the client to signal a disconnect mid-stream, so the server-side generator never sees one and the client never gets anything back. Confirmed this empirically with `faulthandler`-dumped stack traces showing both hung inside the initial `send()`/`handle_request()` call, before the streaming body was ever reached.

The fix was to test `_generate_events` directly: a minimal fake `Request` (just `.client.host` and a controllable `is_disconnected()`) drives the async generator with `__anext__()`, so the test controls disconnection deterministically instead of depending on transport-level streaming semantics that don't exist in either test client. `create_stream_router()`'s route-building and the endpoint's `StreamingResponse`/headers are tested separately by calling the routed endpoint function directly (via `router.routes[0].endpoint`) without consuming its body.

• Files reviewed: 15/16 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.