Add market data backend code review - #6
Merged
Merged
Conversation
Comprehensive review of backend/app/market/ and its test suite. Test suite is green (73 passed, 91% coverage, ruff clean), but two defects pass through it undetected: - The Massive data source never populates the price cache: it reads snap.last_trade.timestamp, a field absent from the SDK's LastTrade model, and the resulting AttributeError is swallowed by the skip-malformed-snapshot handler. Timestamps are also nanoseconds, not milliseconds. - Shock events exceed GBM diffusion variance by ~317x, collapsing inter-ticker correlation from 0.60 to 0.003 and inflating realized volatility 8-28x above the configured per-ticker sigma. Also documents the verified SSE wire format (closing PLAN A5), the missing keepalive, ticker-normalization divergence between the two MarketDataSource implementations, and the module-level router singleton that defeats cache injection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FKCL7F4QhyBuoDcxXrgipW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comprehensive review of
backend/app/market/and its test suite, written toplanning/MARKET_DATA_REVIEW.md. Supersedes the round-1 review inplanning/archive/.Test results
All round-1 action items were applied and the suite is green: 73 passed, 91% coverage,
ruff checkclean.Two defects the passing suite does not catch
Both verified empirically, not inferred from reading.
1. The Massive (real market data) path is 100% non-functional.
massive_client.py:103readssnap.last_trade.timestamp— a field absent from the SDK'sLastTrademodel (it'ssip_timestamp). TheAttributeErroris swallowed by the skip-malformed-snapshot handler two lines down, so every well-formed snapshot is dropped and the cache is never written. SetMASSIVE_API_KEYand the whole app has no prices. The same lines also divide by 1000 assuming milliseconds; Polygon timestamps are nanoseconds.It survived testing because
test_massive.py's_make_snapshotbuilds aMagicMockand assigns the invented field — test and code share one wrong assumption. Coverage on that module is 94%.2. Shock events swamp the GBM engine by ~317x in variance. With
event_probability=0the math is exact (AAPL/GOOGL correlation 0.598 vs target 0.60; realized sigma 0.221 vs configured 0.22). At the shipped0.001, correlation collapses to 0.003 and realized volatility runs 8-28x configured and roughly equal across all tickers — so the per-ticker sigma tuning and the entire Cholesky correlation design are inert. Prices wander 9.2%/hour instead of 0.7%.Also covered
change_percentincludedMarketDataSourceimplementationsTickerSnapshot.prev_dayandtodays_change_percentwould close A1 for freeDocumentation only — no source changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VGUSZLyF2LLqRMz18yg2Re