PR Review: MCP Tool Handling Refactor

Summary

This PR refactors the MCP tool handling architecture, transitioning from direct session management to a cleaner composition-based design where MultiMCPHandler delegates to individual MCPHandler instances.

Strengths

Architecture Improvements

1. Better Separation of Concerns: MultiMCPHandler now acts as a coordinator, delegating connection management to individual MCPHandler instances
2. Cleaner Abstraction: Removed AsyncExitStack complexity from MultiMCPHandler
3. Improved Introspection: Added get_info(), get_server_info(), get_tools_by_server() methods

Code Quality

1. Better Documentation: Improved docstrings explaining the delegation pattern
2. Removed Dead Code: Cleaned up unused PseudoHandler class and tool_definitions field
3. Enhanced Error Messages: More user-friendly console output

Critical Issues

1. UUID-based Server Naming May Break Existing Code (HIGH PRIORITY)

Location: src/upsonic/tools/mcp.py:296-297

The server name is now randomized with UUID when server_params is provided without a command. This could break tool name expectations, tests, and user code that filters by server name.

Recommendation: Use a class-level counter or allow users to specify server_name parameter.

2. Overly Broad Exception Suppression (MEDIUM PRIORITY)

Location: src/upsonic/tools/mcp.py:440-445, 450-455, 949-955

Catching all Exception types with string matching on error messages is fragile. This could hide legitimate bugs during cleanup.

Recommendation: Be more specific with exception handling - catch asyncio.CancelledError separately, and log all unexpected exceptions with full tracebacks.

Other Concerns

3. Test Coverage Gap (MEDIUM)

Only smoke tests exist. Need unit tests for MCPHandler lifecycle, MultiMCPHandler delegation, error handling, server name generation, and new introspection methods.

4. Potential Resource Leak (LOW)

In get_tools(), the event loop is closed immediately after connect(), but handlers may not be properly cleaned up.

5. Unrelated Prompt Change (LOW)

The default_prompt.py change seems unrelated to MCP refactor - consider separate PR.

Security & Performance

Security: No concerns. Command sanitization remains unchanged.
Performance: Delegation pattern is cleaner. UUID overhead negligible. 60s timeout in threaded discovery seems long.

Overall Assessment

This is a solid refactoring with improved architecture. The delegation pattern is much cleaner. However, address the non-deterministic server naming and broad exception handling before merging.

Recommendation: Request changes for issues 1 and 2, then approve.

Great work on the composition-based approach!

Code Review: MCP Tool Handling Class Refactor

Summary

This is a substantial refactoring effort that improves the MCP tool handling architecture and adds Slack interface support. The changes show good architectural thinking with +949/-263 lines across 18 files.

Strengths

Architecture and Design

1. Excellent class structure - The refactored MCPHandler and MultiMCPHandler classes provide clear separation of concerns
2. Comprehensive error handling - Good use of try-except blocks with proper error suppression for event loop issues
3. Security-first approach - Strong command sanitization in prepare_command() with whitelist validation
4. Async context manager support - Proper resource management
5. Tool filtering capabilities - Nice include/exclude tool functionality with validation

Slack Integration

6. Production-ready Slack interface - Event deduplication, signature verification, thread support
7. Good documentation - Comprehensive docstrings throughout SlackInterface class
8. Graceful degradation - Proper handling of missing configuration

Interface Manager Improvements

9. Request ID middleware - Excellent addition for traceability
10. Enhanced health checks - Health endpoint now provides per-interface status details
11. Better error logging - Using traceback.format_exc() for cleaner logs

Critical Issues

1. Protobuf Dependency Management (pyproject.toml:31)

Issue: Moves protobuf from optional to required dependency, significantly increasing installation footprint.
Recommendation: Move protobuf back to optional dependencies under MCP extras group

2. Event Deduplication Memory Leak (slack/slack.py:192-198)

Issue: Cleanup only triggers when len > 1000, allowing unbounded growth in low-traffic scenarios.
Recommendation: Use TTLCache from cachetools for O(1) cleanup

3. Slack Signature Verification (slack/slack.py:113)

Issue: No check if signing_secret is None before HMAC computation, could raise AttributeError
Recommendation: Add early return when signing_secret is None

4. MCP Tool Call Session Pattern (mcp.py:657-691)

Issue: Creating new session for every tool call is inefficient, may cause resource exhaustion
Recommendation: Reuse existing self.session when available

Medium Priority Issues

5. Breaking Change: Optional Dependency Naming (pyproject.toml:35)

Issue: rag renamed to vectordb breaks pip install upsonic[rag]
Recommendation: Keep both rag and vectordb for backward compatibility

6. Slack Message Length (slack/slack.py:152-158)

Issue: 4000 char limit is very conservative
Recommendation: Increase to 10000+ chars, use file upload for larger messages

7. Thread Safety (mcp.py:606-620)

Issue: asyncio.set_event_loop() modifies thread-local state
Recommendation: Remove set_event_loop() call

Security Assessment

Positive

• Command sanitization with whitelist
• Slack signature verification with timing-safe comparison
• Request timestamp validation
• No eval() or exec() usage

Concerns

• Slack signing secret can be None - should fail fast at init
• No rate limiting on Slack webhook endpoint
• MCP command args need additional validation

Performance Considerations

1. Fresh session per MCP tool call: 100-500ms overhead
2. Slack event deduplication: O(n) cleanup
3. Thread pool creation per tool discovery
4. Multiple JSON parse attempts in _process_tool_result

Test Coverage

Critical: No test files included despite significant new functionality

Needs:

• Unit tests for MCPHandler/MultiMCPHandler
• Slack signature verification edge cases
• MCP command sanitization with malicious inputs
• Event deduplication under high load
• Message batching logic

Verdict

Overall Score: 7.5/10

• Code Quality: 8/10
• Security: 7/10
• Performance: 6/10
• Test Coverage: 3/10

Recommendation: Address critical issues 1-4 before merging. Other issues can be follow-up PRs.

Great refactoring work!