perf: batch broker enrichment lookups concurrently (#986)

[jwesleye]

Closes #986

Changes

• New tools/batch_fetch.py helper: gather_bounded (bounded concurrent gather with return_exceptions=True) and dedupe_preserving_order. Helpers do not call execute_with_retry themselves so existing module-level mocks keep working.
• broker_comparison_tools: Robinhood and Schwab per-symbol quote loops now fan out via gather_bounded.
• robinhood_account_tools.get_positions: dedupes instrument URLs, resolves symbols concurrently, builds a URL→symbol map.
• robinhood_dividend_tools.get_dividends / get_stock_loan_payments: shared module-local _resolve_instruments helper deduplicates and fans out instrument lookups; both functions now share the same URL→instrument map within a request.
• options/positions.get_open_option_positions_with_details: _extract_option_id + _resolve_option_instruments dedupes by option_id and gathers fetches.
• robinhood_trading_tools.get_all_open_stock_orders: dedupes instrument URLs and gathers symbol lookups.
• New tests/unit/test_batch_fetch.py: helper unit tests + one regression per flagged call site for duplicate keys, plus a 50-row payload that exercises the concurrency cap.
• Tightened two existing tests (test_get_positions_success, test_stock_loan_payments_success) to map URL → symbol via callable side_effect, since concurrent gather no longer guarantees serial call order.

Test plan

• uv run pytest tests/unit/test_batch_fetch.py -q --tb=short — 14 passed
• uv run pytest tests/unit/ -q --tb=line — 1386 passed, 69 skipped
• uv run ruff check src/open_stocks_mcp/tools/ tests/unit/test_batch_fetch.py tests/unit/test_account_tools.py tests/unit/test_analytics_tools.py — clean
• uv run ruff format --check src/open_stocks_mcp/tools/ tests/unit/test_batch_fetch.py tests/unit/test_account_tools.py tests/unit/test_analytics_tools.py — clean
• uv run mypy src/ — no issues found in 88 source files

[claude]

Claude finished @jwesleye's task —— View job

---

Code Review for PR #986: Batch Broker Enrichment Lookups

Todo List:

• Read and analyze the new batch_fetch.py module
• Review modifications to existing tools for concurrent patterns
• Examine test coverage and quality
• Assess performance considerations
• Check for potential bugs or security issues
• Provide comprehensive feedback

🎯 Overall Assessment: EXCELLENT ⭐⭐⭐⭐⭐

This is a high-quality performance optimization that addresses a real N+1 query problem affecting broker API calls. The implementation is well-architected, thoroughly tested, and follows excellent software engineering practices.

✅ Code Quality & Best Practices

Excellent Architecture:

• Clean separation of concerns - batch_fetch.py provides reusable primitives without coupling to specific broker logic
• Consistent patterns - All 6 modified call sites follow the same dedupe_preserving_order → gather_bounded pattern
• Backwards compatibility - Existing module-level mocks continue working since helpers don't call execute_with_retry directly

Strong Type Safety:

• Proper generic typing with TypeVar("T")
• Good use of strict=True in zip() calls
• Comprehensive type hints throughout

Good Error Handling:

• return_exceptions=True prevents single failures from aborting entire batches
• Graceful degradation with warning logs for failed lookups
• Proper exception propagation through BaseException checks

✅ Performance Considerations

Significant Improvements:

• O(n) → O(unique(n)) reduction in broker API calls through deduplication
• Serial → concurrent execution with bounded concurrency (DEFAULT_BATCH_CONCURRENCY = 8)
• Smart batching in dividend/loan payment tools that share _resolve_instruments helper

Well-Tuned Concurrency:

• Semaphore-based limiting prevents overwhelming broker APIs
• Default limit of 8 is reasonable for financial APIs
• Proper async/await patterns with no blocking operations

✅ Security Assessment

No security concerns identified. The changes:

• Don't introduce new authentication paths
• Maintain existing retry/rate limiting contracts
• Don't modify any credential handling
• Use defensive programming with exception handling

✅ Test Coverage - Exceptional

Comprehensive Unit Tests (14 new tests in test_batch_fetch.py):

Core Helper Testing:

• ✅ gather_bounded concurrency limiting verification
• ✅ Exception handling and order preservation
• ✅ dedupe_preserving_order edge cases and correctness

N+1 Regression Tests:

• ✅ test_get_positions_dedupes_repeated_instrument_urls() - ensures duplicate instrument URLs hit broker only once
• ✅ test_get_dividends_dedupes_repeated_instrument_urls() - dividend history deduplication
• ✅ test_get_open_option_positions_with_details_dedupes_option_ids() - options instrument batching
• ✅ 50-item payload tests that verify concurrency behavior under load

Smart Test Design:

• Uses side_effect with callable mocks to verify deduplication
• Tracks peak concurrent operations to verify bounded execution
• Updated existing tests (test_get_positions_success, test_stock_loan_payments_success) to use callable mocks since concurrent execution no longer guarantees call order

✅ Code Changes Analysis

broker_comparison_tools.py - Lines 52-54, 145-147:

price_results = await gather_bounded(
    [get_stock_price(symbol) for symbol in symbols]
)

Excellent: Transforms serial quote fetching into concurrent batches for both Robinhood and Schwab.

robinhood_account_tools.py - Lines 219-226:

instrument_urls = dedupe_preserving_order(
    position.get("instrument") for position in positions
)
symbol_results = await gather_bounded(
    [execute_with_retry(rh.get_symbol_by_url, url) for url in instrument_urls]
)

Perfect: Classic N+1 fix with deduplication + concurrent resolution.

options/positions.py - Lines 34-50:

async def _resolve_option_instruments(option_ids: list[str]) -> dict[str, dict[str, Any]]:
    results = await gather_bounded([
        execute_with_retry(rh.options.get_option_instrument_data_by_id, option_id, max_retries=2)
        for option_id in option_ids
    ])

Excellent: Shared helper pattern that multiple functions can reuse.

robinhood_dividend_tools.py - Lines 17-42:

async def _resolve_instruments(instrument_urls: list[str]) -> dict[str, dict[str, Any]]:

Smart: Module-level helper shared by both get_dividends and get_stock_loan_payments - avoids code duplication.

📊 Potential Performance Impact

Conservative estimate for large portfolios:

• Before: 100 positions = 100 serial API calls ≈ 50-100 seconds
• After: 100 positions with 20 unique instruments = 20 concurrent calls ≈ 3-5 seconds
• Improvement: ~10-20x speedup for typical portfolios with position overlaps

🔍 Minor Suggestions (Non-Blocking)

1. Consider adding metrics collection to track deduplication effectiveness in production
2. Documentation opportunity - consider adding usage examples in docstrings
3. Future enhancement - could expose limit parameter in high-level tool functions for power users

🏆 Recommendation: APPROVE ✅

This PR exemplifies excellent engineering practices:

• ✅ Addresses real performance pain points
• ✅ Maintains backward compatibility
• ✅ Includes comprehensive test coverage
• ✅ Uses proven concurrency patterns
• ✅ No security or correctness risks

The implementation is production-ready and will provide significant performance improvements for users with larger portfolios. Great work!

[unseriousAI]

AGENTSHORE_CODE_REVIEW
head_sha: 0811f92
status: PASS
spec_compliance: PASS
blocking_findings: 0
non_blocking_findings: 0

Summary

PASS: implementation matches issue #986 scope and adds bounded concurrent enrichment with strong regression coverage.

Blocking

None

Suggestions

None