feat: Add multi-asset screening tools (forex, crypto, ETF)

[bearmug]

Implement three new MCP tools for screening different asset classes:

• screen_forex: Screen 1,759+ forex pairs with technical indicators
• screen_crypto: Screen 9,247+ cryptocurrencies with market data
• screen_etf: Screen ETFs/funds with automatic type filtering

Features:

• Dedicated screening methods for each asset class
• Proper caching and rate limiting per tool
• Asset-specific default columns and sorting
• Comprehensive test coverage (69 tests passing)

Updates:

• Added tool definitions and handlers to MCP server
• Updated README with tool documentation and examples
• Added 2 new tests for ETF screening validation

🤖 Generated with Claude Code

[claude]

Pull Request Review: Multi-Asset Screening Tools

Thank you for this contribution! This PR adds valuable functionality by extending the screening capabilities to forex, crypto, and ETF assets. Overall, the implementation is solid and follows the existing patterns well. Below is my detailed review.

---

✅ Strengths

Code Quality

• Consistent patterns: The new methods (screenForex, screenCrypto, screenETF) follow the same structure as the existing screenStocks method, maintaining code consistency
• Good code reuse: The validateAndConvertFilters method is properly shared across all screening methods
• Clear separation: Each asset class has dedicated default columns and sorting appropriate for that asset type
• Type safety: Uses existing TypeScript interfaces effectively

Test Coverage

• Comprehensive validation tests: The two new tests for ETF screening validate both error cases and success scenarios
• Good test patterns: Tests verify that the ETF-specific type filter is correctly added
• Existing validation: All screening methods reuse the same filter validation, so existing tests apply

Documentation

• README updates: Clear documentation for each new tool with parameters and example fields
• Descriptive PR body: Good explanation of features and changes

---

🔍 Issues & Suggestions

1. Missing Limit Validation in screenForex and screenCrypto ⚠️

Issue: In src/tools/screen.ts, the screenForex (lines 201-242) and screenCrypto (lines 244-285) methods do not validate the limit parameter, while screenStocks and screenETF do.

Current code:

async screenForex(input: Omit<ScreenStocksInput, "markets">): Promise<any> {
  const {
    filters,
    sort_by = "volume",
    sort_order = "desc",
    limit = 20,
  } = input;

  const cacheKey = JSON.stringify({ type: "forex", filters, sort_by, sort_order, limit });
  // No limit validation here!

Recommendation: Add the same validation as in screenStocks and screenETF:

// Validate limit
if (limit < 1 || limit > 200) {
  throw new Error("Limit must be between 1 and 200");
}

This should be added in both screenForex (after line 208) and screenCrypto (after line 251).

---

2. Inconsistent Default Columns ℹ️

Observation: Each screening method uses different default columns:

• screenStocks: Uses DEFAULT_COLUMNS (7 fields including market_cap, P/E, ROE, etc.)
• screenForex: ["name", "close", "change"] (3 fields)
• screenCrypto: ["name", "close", "market_cap_basic", "change"] (4 fields)
• screenETF: ["name", "close", "volume", "change", "change_from_open"] (5 fields)

Question: Is this intentional based on what fields are available/relevant for each asset class? If so, this is fine. Otherwise, consider documenting why each has different defaults or standardizing where appropriate.

---

3. Type Safety Could Be Improved 🔧

Issue: The return type is Promise<any> for all screening methods.

Current:

async screenForex(input: Omit<ScreenStocksInput, "markets">): Promise<any>

Recommendation: Define explicit return types for better type safety:

interface ScreenForexResult {
  total_count: number;
  pairs: Array<{ symbol: string; [key: string]: any }>;
}

async screenForex(input: Omit<ScreenStocksInput, "markets">): Promise<ScreenForexResult>

Similarly for screenCrypto (returns cryptocurrencies) and screenETF (returns etfs).

---

4. Missing Input Validation Tests 📝

Observation: While the PR adds 2 new tests for screenETF, there are no new tests for screenForex or screenCrypto input validation.

Recommendation: Add tests to verify:

• Limit validation for screenForex and screenCrypto (once added per issue Add Claude Code GitHub Workflow #1)
• Default sorting behavior
• Default column behavior
• Response structure (verify the correct field names: pairs, cryptocurrencies, etfs)

Example test:

it("should validate limit in screenForex", async () => {
  await assert.rejects(
    async () => {
      await screenTool.screenForex({
        filters: [{ field: "volume", operator: "greater", value: 1000 }],
        limit: 201, // exceeds maximum
      });
    },
    { message: /Limit must be between 1 and 200/ }
  );
});

---

5. Tool Input Schema Duplication 🔄

Observation: In src/index.ts, the three new tool definitions (lines 161-260) have nearly identical inputSchema structures with only minor differences in descriptions and defaults.

Consideration: While this duplication is acceptable and makes each tool definition self-contained, you could consider:

• Extracting a shared schema factory function to reduce duplication
• Or keeping as-is for clarity (current approach is fine)

This is a minor style point and not a blocker.

---

6. Edge Case: Empty Filters Array 🤔

Question: What happens if a user provides an empty filters array filters: []?

Looking at the code, this would:

1. Pass validation (no invalid filters to check)
2. Create a request with no filters
3. Return all assets (up to the limit)

Is this the intended behavior? If so, it might be worth documenting. If not, consider adding validation:

if (!filters || filters.length === 0) {
  throw new Error("At least one filter is required");
}

---

🔒 Security

• ✅ No security concerns identified
• ✅ Input validation is properly implemented
• ✅ Rate limiting is consistently applied
• ✅ No injection risks (JSON serialization is safe)

---

⚡ Performance

• ✅ Caching is properly implemented for all new methods
• ✅ Rate limiting is applied consistently
• ✅ Default column sets are minimal to reduce response size
• 💡 Consider documenting cache TTL and rate limits in README for user awareness

---

📊 Summary

Category	Status
Code Quality	✅ Good
Best Practices	✅ Followed
Security	✅ No concerns
Performance	✅ Optimized
Test Coverage	⚠️ Needs expansion
Documentation	✅ Comprehensive

---

🎯 Recommendations Priority

Must Fix:

1. Add limit validation to screenForex and screenCrypto

Should Fix:
2. Add explicit return types instead of any
3. Add tests for screenForex and screenCrypto

Nice to Have:
4. Document or validate empty filters behavior
5. Document why different default columns are used per asset class

---

🚀 Conclusion

This is a well-structured PR that extends the server's capabilities in a consistent manner. The main issue is the missing limit validation in two methods, which should be added before merging. Once the "Must Fix" items are addressed and additional tests are added, this will be ready to merge.

Great work on maintaining code consistency and following the existing patterns! 👍