Add market data backend code review document - #151
Open
Sarita8888 wants to merge 19 commits into
Open
Conversation
Documents the implemented app/market/ subsystem (unified interface, GBM simulator, Massive API client, SSE streaming, price cache) with current source snippets, plus forward-looking guidance for wiring it into the not-yet-built FastAPI main.py and other routers.
Both workflows only passed claude_code_oauth_token to anthropics/claude-code-action, so a repo secret named ANTHROPIC_API_KEY was silently ignored. Pass it through as an alternative credential.
…gn-76r18z Add detailed market data backend design document
Issue #2 failed because claude.yml only granted contents:read, which caused claude-code-action to restrict the agent to a read-only toolset (no Write/Edit tools). The agent had no way to create the requested backend files and errored out after 81 turns.
v4 still bundles a Node 20 runtime, which GitHub Actions now forces onto Node 24 with a deprecation warning on every run. v5 is Node 24-native.
Fix Claude Code workflow permissions and Node 20 deprecation warning
Every real @claude trigger since the permissions fix landed fails instantly (is_error:true, 0 turns, $0 cost) instead of running. The action redacts the SDK error by default; this surfaces it so we can see what's actually failing.
Temporarily enable show_full_output for diagnosis
The default model (Opus with 1M context) spent $5.34 over 81 turns on a single incomplete run. Sonnet is materially cheaper and sufficient for this kind of implementation/audit task.
Root cause (Anthropic account billing_error) is confirmed; no longer need the SDK's raw output surfaced.
Pin Claude Code Action to Sonnet 5 to reduce run cost
The Anthropic Console account behind ANTHROPIC_API_KEY is out of credits and blocked on a payment-processing error, so drop that input from both workflows and rely solely on CLAUDE_CODE_OAUTH_TOKEN, which bills against the Claude.ai subscription's usage allowance instead of pay-per-token API credits.
Switch to Claude Code OAuth token auth (Pro subscription)
- stream.py: build the APIRouter inside create_stream_router() instead of at module level, so calling the factory more than once never registers duplicate routes on a shared router. - cache.py: use `timestamp if timestamp is not None else time.time()` instead of `timestamp or time.time()`, so an explicit timestamp=0 isn't treated as missing. - simulator.py / massive_client.py: normalize tickers (upper/strip) consistently across start()/add_ticker()/remove_ticker() on both SimulatorDataSource and MassiveDataSource, so the same call produces the same ticker casing regardless of which source is active. - Add missing test coverage: SSE stream generator + router factory, MarketDataSource ABC conformance, seed_prices.py sanity checks, and ticker-normalization regression tests for both data sources. Co-authored-by: Sarita8888 <62361812+Sarita8888@users.noreply.github.com>
Fix market data defects and add missing test coverage
Fresh review of backend/app/market/ against real dependencies; flags a high-severity bug where the Massive/Polygon integration is broken against the installed SDK (wrong timestamp attribute and unit conversion), plus several lower-severity test-coverage gaps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add market data code review
Addresses every issue raised in planning/MARKET_DATA_REVIEW.md: - §3.1 (High): massive_client.py used snap.last_trade.timestamp, which doesn't exist on the real SDK's LastTrade dataclass, and divided by 1000 assuming milliseconds. Fixed to use sip_timestamp / 1e9 (nanoseconds -> seconds). Added a regression test built from a real TickerSnapshot.from_dict() payload instead of a MagicMock, so a future SDK-shape mismatch fails a test instead of failing silently. - §3.2: added a test that steps GBMSimulator with the full 10-ticker default watchlist, covering the real block-correlation matrix shape. - §3.3: test_exception_resilience now actually injects a failure into GBMSimulator.step() and asserts the background loop survives and keeps updating the cache afterward. - §3.4: _poll_once() now normalizes ticker casing like every other MassiveDataSource write path. - §3.5: documented the implementation-defined pre-start() behavior on MarketDataSource.add_ticker(). - §3.7: added a concurrent-writer stress test for PriceCache using real OS threads. - Ran ruff format on the 5 previously-flagged whitespace-only files. 103/103 tests passing, 98% coverage, ruff check and format clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix Massive integration bug and close market data test gaps
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
2 participants
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/ covering test results, architecture assessùment, and actionable issues found.