feat(mcp): add SSE server for remote MCP deployment#71
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- Add server_sse.py with Starlette/SSE transport for remote access - Add Dockerfile for Railway deployment (Python 3.13-slim + uv) - Add security.py for credential handling via HTTP headers - Update exchange_manager.py with context-based credential injection - Support all exchanges: Polymarket, Limitless, Opinion, Kalshi, Predict.fun Endpoints: - /health - Health check - /sse - MCP SSE connection - /messages/ - MCP message handling Deployed at: https://dr-manhattan-mcp-production.up.railway.app Closes #50 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously, claude-default would run on all pull_request_target events, causing OIDC token failures for PRs without @claude mention. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security: - Replace CORS wildcard with configurable allowed origins - Fully mask credentials in logs (no partial exposure) - Fix Docker healthcheck (curl not available in slim image) - Remove HTTP-specific error messages from exchange_manager Code quality: - Extract shared tool definitions to avoid 348-line duplication - Add environment variable validation (PORT, LOG_LEVEL) - Optimize Dockerfile layer caching - Add comprehensive tests for SSE server Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make TOOL_DISPATCH lazy to avoid mcp import in non-mcp tests - Fix test file import ordering for ruff - Skip MCP-dependent tests when mcp package not installed Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The conditional `import mcp` inside try/except block confuses isort. Using skip_file directive is the standard solution for test files with conditional imports. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
guzus
force-pushed
the
feat/deploy-mcp-sse-server
branch
from
3813883 to
48f8d21
Compare
PR Review: SSE-based MCP Server for Remote DeploymentI've reviewed PR #71 and overall this is a well-structured implementation that adds SSE transport for remote MCP deployment. Here's my detailed feedback: ✅ StrengthsCode Quality
Security
Testing
Deployment
|
- Create wiki/mcp/remote-server.md with full setup guide - Document credential headers for all exchanges - Add multi-exchange configuration example - Update README.md with remote server quick start - Update wiki index Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: SSE-based MCP Server for Remote DeploymentOverall AssessmentThis is a well-structured PR that adds SSE (Server-Sent Events) transport for remote MCP deployment. The implementation is thoughtful with good security practices and comprehensive test coverage. However, there are several security concerns and potential issues that should be addressed. Security Concerns🔴 Critical
🟡 High Priority
Code Quality IssuesArchitecture & Design
Error Handling
Code Clarity
CLAUDE.md Compliance✅ Follows Guidelines
❌ Violates Guidelines
Performance Considerations
Testing✅ Strengths
🟡 Gaps
Documentation✅ Strengths
🟡 Improvements Needed
Recommendations SummaryMust Fix Before Merge
Should Fix Soon
Nice to Have
Positive Highlights
ConclusionThis PR adds valuable functionality but has security concerns that should be addressed before deploying to production with real user credentials. The code quality is generally good, but the security architecture needs hardening for a public-facing service handling financial credentials. Recommendation: Address critical security issues before merge, especially around credential transmission, rate limiting, and CORS configuration. |
- CLI command option (claude mcp add) - Global vs project configuration - Verification step with /mcp Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request Review: SSE-based MCP Server for Remote DeploymentSummaryThis PR adds comprehensive SSE transport support for remote MCP server deployment, enabling users to connect to Dr. Manhattan from Claude Desktop/Code without local installation. The implementation is well-structured and demonstrates solid engineering practices. Code Quality & Architecture ✅Strengths:
Areas for improvement:
Security 🔒Strong security implementation:
Security concerns:
Test Coverage ✅Excellent test coverage:
Minor gaps:
Recommendation: Add at least one integration test that verifies credentials flow through the context variable to the exchange manager. Performance ⚡Good performance considerations:
Potential issues:
Best Practices Alignment 📋Alignment with CLAUDE.md:
Documentation concerns:
While this documentation is valuable for users, it technically violates the project convention. Consider whether this exception is justified for a major new feature. Bugs & Issues 🐛
Additional Recommendations
ConclusionThis is a high-quality PR that adds significant value to the project. The implementation is solid, well-tested, and follows good security practices. The main concerns are:
Recommendation: ✅ Approve with minor fixes The identified issues are not blockers, but addressing the performance concern (#1) and security hardening (#1-2) would significantly improve the production readiness of this feature. Great work on this implementation! 🚀 |
Pull Request Review: SSE MCP Server for Remote DeploymentSummaryThis is a well-architected addition that enables remote access to Dr. Manhattan via SSE transport. The implementation demonstrates strong security awareness, good code organization, and thorough testing. Overall, this is production-ready code with a few recommendations for improvement. Code Quality & Best PracticesStrengths
Areas for Improvement1. Security: CORS Configuration May Be Too PermissiveLocation: server_sse.py:265-285 Issue:
Recommendation: Priority: High - CORS misconfiguration is a common security vulnerability 2. Bug: Unused cleanup_handler FunctionLocation: server_sse.py:304-310, 346-347 Issue: Signal handlers are registered but the cleanup() async function is never called. The cleanup_handler sets a flag but doesn't trigger actual cleanup. Recommendation: Priority: High - This is a functional bug that prevents proper resource cleanup 3. Performance: Missing Request TimeoutLocation: server_sse.py:354-360 Issue: No timeouts configured for uvicorn server, which can lead to resource exhaustion. Recommendation: Priority: Medium - Important for production stability 4. Missing: Rate Limiting Per UserLocation: server_sse.py:166-167, call_tool() function Issue: For a multi-tenant SSE server, rate limiting should be per-user/credential to prevent one user from exhausting resources. Recommendation: Priority: Medium - Important for fair resource allocation 5. Code Quality: Logger Monkey-Patching is FragileLocation: server_sse.py:30-122 Issue: Complex logger patching at module level is error-prone. The fix_all_loggers() function could interfere with third-party library logging. Recommendation: Priority: Medium - Works currently but technical debt 6. Documentation: Missing Deployment Security GuideLocation: wiki/mcp/remote-server.md Issue: Documentation is excellent for setup but lacks security best practices for self-hosting. Recommendation: Priority: Medium - Important for users who self-host Performance ConsiderationsGood Practices
Suggestions
Test CoverageStrong Coverage
Missing Tests
Final RecommendationsMust Fix (Before Merge)
Should Fix (Soon After Merge)
Nice to Have
ConclusionThis is high-quality, production-ready code with strong security awareness. The architecture is sound, testing is comprehensive for core functionality, and it follows the project's guidelines well. The critical issues are minor and easily addressable. Once the cleanup handler bug and CORS configuration are fixed, this is ready to merge. Recommendation: Approve with minor changes requested Great work on this feature! |
Security improvements for SSE server: - Write operations (create_order, cancel_order, etc.) only allowed for Polymarket - Other exchanges are read-only for security (no private keys on server) - Polymarket uses Builder profile: API key, secret, passphrase (no private key) - Updated tests and documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request Review: SSE-based MCP Server for Remote DeploymentSummaryThis PR adds a production-ready SSE (Server-Sent Events) transport for remote MCP server deployment, enabling users to connect to Dr. Manhattan from Claude Desktop/Code without local installation. The implementation is well-structured with strong security considerations. Code Quality Assessment: ⭐⭐⭐⭐ (4/5)Strengths1. Excellent Security Design ✅
2. Clean Architecture ✅
3. Comprehensive Testing ✅
4. Documentation ✅
5. Production Ready ✅
Issues & Recommendations🔴 CRITICAL: Security VulnerabilityLocation: The credential validation logic contains a security mismatch: # SSE mode validation expects private_key + funder
if exchange_name.lower() == "polymarket":
if not exchange_creds.get("private_key"):
raise ValueError("Missing private_key credential...")
if not exchange_creds.get("funder"):
raise ValueError("Missing funder credential...")But required_fields = {
"polymarket": ["api_key", "api_secret", "api_passphrase"],
}Impact: Users providing Builder profile credentials (api_key, api_secret, passphrase) will fail validation because the code checks for Fix: Update if exchange_name.lower() == "polymarket":
# Builder profile credentials (SSE mode - no private key)
if not exchange_creds.get("api_key"):
raise ValueError("Missing api_key credential...")
if not exchange_creds.get("api_secret"):
raise ValueError("Missing api_secret credential...")
if not exchange_creds.get("api_passphrase"):
raise ValueError("Missing api_passphrase credential...")🟡 MAJOR: Potential Issues1. Logger Patching Fragility (
|
| Rule | Status | Notes |
|---|---|---|
| 1. Focused, clean, easy to understand | ✅ | No emojis, clear structure |
| 2. Don't create unnecessary docs | ✅ | Wiki docs are essential for feature |
| 3. Only use UV | ✅ | Dockerfile uses UV correctly |
| 4. Single Source of Truth (no .env) | Uses env vars but documented | |
| 5. Run and Debug proactively | ✅ | Railway deployment tested |
| 6. Exchange wiki template | N/A | Not adding exchange |
Final Recommendations
Before Merge:
- FIX CRITICAL: Update credential validation in
exchange_manager.pyto use Builder profile fields - FIX MAJOR: Restrict CORS
allow_headersto specific headers - FIX MINOR: Update Dockerfile HEALTHCHECK to respect $PORT
Post-Merge:
- Add integration tests for SSE endpoints
- Simplify logger patching logic
- Add rate limit configuration documentation
- Consider credential caching for performance
Verdict: Approve with Required Changes ✅⚠️
This is a high-quality implementation with excellent security design and documentation. The critical credential validation bug must be fixed before merge, but the overall architecture is solid. Once the required fixes are applied, this will be a production-ready feature.
Estimated Risk: Medium (due to critical bug, but easily fixable)
Code Maintainability: High
Security Posture: High (after fixes)
Great work on this feature! The security-first approach and comprehensive documentation are particularly impressive. 🚀
- Create separate PolymarketBuilder class that uses API credentials (api_key, api_secret, passphrase) instead of private keys - Update exchange_manager to route to PolymarketBuilder when Builder credentials are provided in SSE mode - Add detailed step-by-step guide for getting Polymarket API credentials - Add tests for PolymarketBuilder class This enables secure trading on the remote MCP server without exposing private keys. Users provide Builder profile credentials via HTTP headers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request Review: SSE-based MCP Server for Remote DeploymentOverviewThis PR adds significant functionality for remote MCP deployment via SSE transport. The implementation is well-structured and follows security best practices. Below is my detailed review: Code Quality & Best PracticesStrengths
Issues & ConcernsCritical1. Credentials in Memory (Security)
2. Missing Rate Limiting Implementation
3. CORS Configuration
Major4. Timeout Handling
5. Error Message Sanitization
6. Context Variable Cleanup
Minor7. Verbose Mode Hardcoded
8. Health Check
9. Logger Patching
10. Type Hints
Performance Considerations
Security ConcernsAddressed ✓
Needs Attention
|
- Add detailed documentation explaining MCP servers and Operator Mode - Explain security model (what operator can/cannot do) - Show contract addresses with PolygonScan links - Integrate wallet approval steps with explanations - Add FAQ section addressing common concerns - Style with clear visual hierarchy and info cards
PR Review: SSE-based MCP Server for Remote DeploymentExecutive SummaryThis PR adds significant functionality for remote MCP deployment, but has several critical issues that need addressing before merge. Overall assessment: 3/5 - Good implementation with critical violations that must be fixed. 🔴 CRITICAL Issues (Must Fix Before Merge)1. CLAUDE.md Rule 4 Violation: Environment VariablesIssue: The codebase rule explicitly states:
Yet this PR heavily relies on environment variables:
Fix Required: Replace environment variables with:
2. Docker Security: Running as RootLocation: Issue: Container runs as root user (no Fix Required: # Add after installing dependencies, before CMD
RUN addgroup --system --gid 1001 appuser && \
adduser --system --uid 1001 --gid 1001 appuser
USER appuser3. CORS Security IssueLocation: Issue: allow_credentials=True,
allow_headers=["*"], # ⚠️ Allows ANY header with credentialsThis combination is a security risk - allows any header to be sent with credentials. Fix Required: Restrict to specific headers: allow_headers=["Content-Type", "X-Polymarket-Private-Key", "X-Polymarket-Chain-Id", ...],4. HTTP-Specific Errors in Exchange ManagerLocation: Issue: Exchange manager has HTTP-specific error messages (mentions "headers"), coupling it to SSE transport: raise ValueError(
"Missing X-Polymarket-Private-Key header. " # HTTP-specific!
"Please include your private key in the request headers."
)Fix Required: Use generic error messages: raise ValueError("Missing required credential: private_key")Let the SSE server translate to HTTP-specific messages. 🟡 HIGH Priority (Should Fix)5. Scope Creep: Website FrontendThe PR adds 20+ website files (React/TypeScript/Vite, 1,893 lines of CSS) not mentioned in the PR title or main description. This significantly increases review complexity. Recommendation: Move website to separate PR for clearer review and git history. 6. No HTTPS EnforcementLocation: Credentials are passed via HTTP headers but there's no validation that HTTPS is being used in production. Recommendation: Add explicit HTTPS check: if not request.url.scheme == "https" and os.getenv("ENV") == "production":
raise ValueError("Credentials can only be sent over HTTPS")7. Silent Type Coercion FailureLocation: try:
exchange_creds[cred_key] = int(value)
except ValueError:
exchange_creds[cred_key] = 0 # Silently defaults - masks errorsRecommendation: Raise exception instead of silent default to catch configuration errors. 🟢 MEDIUM Priority (Consider Fixing)8. Missing Connection ManagementNo visible timeout or connection limits for long-lived SSE connections. Could exhaust server resources under load. Recommendation: Add connection timeout (e.g., 1 hour) and max connections limit. 9. Dictionary Mutation BugLocation: def _create_exchange_with_credentials(self, exchange_name: str, config_dict: Dict[str, Any]):
config_dict["verbose"] = DEFAULT_VERBOSE # Modifies input dictFix: Create copy: 10. Missing Integration TestsGood unit test coverage, but no end-to-end tests for:
✅ STRENGTHS
Ratings
Overall: ⭐⭐⭐☆☆ (3/5) - Good work, but needs fixes before merge Action ItemsRequired for Merge:
Strongly Recommended:
Nice to Have:
Great work on the architecture and refactoring! The code is well-organized and shows strong engineering practices. Once the critical issues are addressed, this will be a solid addition to the codebase. |
PR Review: SSE-based MCP Server for Remote DeploymentOverviewThis PR adds substantial infrastructure for remote MCP deployment via SSE transport, including Docker support and multiple authentication methods for Polymarket. The implementation is well-structured with good security considerations. Code Quality & Best PracticesStrengths✅ Clean architecture: Separation of concerns between server, security, exchange managers Areas for Improvement1. Security: Rate Limiting is Per-Server, Not Per-User 🔴 CRITICALLocation: The global rate limiter is shared across all users. A single user can exhaust the rate limit and deny service to others. # Current: Single global instance
_rate_limiter: Optional[RateLimiter] = None
def check_rate_limit() -> bool:
limiter = get_rate_limiter() # Same instance for all users
if not limiter.try_acquire():
return FalseRecommendation: Implement per-user rate limiting keyed by user address or IP:
2. Security: Operator Mode Missing Authentication Verification 🟡 HIGHLocation: Operator mode validates signatures but doesn't verify the operator approval on-chain before allowing trades. # Validates signature but not on-chain approval
if has_user_address and not has_private_key and not has_builder_creds:
from ..utils.security import validate_operator_credentials
is_valid, error = validate_operator_credentials(exchange_creds)
if not is_valid:
raise ValueError(error)Location: def check_operator_approval(self) -> bool:
"""Check if user has approved the operator."""
# This would require checking the CTF Exchange contract
# For now, we'll rely on order placement errors to detect this
return True # Assume approved, error on order if notRecommendation:
3. Security: CORS Configuration Too Permissive 🟡 MEDIUMLocation: Middleware(
CORSMiddleware,
allow_origins=ALLOWED_ORIGINS,
allow_methods=["GET", "POST", "OPTIONS"],
allow_headers=["*"], # Too permissive
allow_credentials=True,
)Issue: Recommendation: Explicitly list allowed headers: allow_headers=[
"Content-Type",
"X-Polymarket-Api-Key",
"X-Polymarket-Api-Secret",
"X-Polymarket-Passphrase",
"X-Polymarket-Wallet-Address",
"X-Polymarket-Auth-Signature",
"X-Polymarket-Auth-Timestamp",
"X-Polymarket-Auth-Expiry",
]4. Potential Bug: Context Variable Not Reset on Exception 🟡 MEDIUMLocation: async def handle_sse(request: Request) -> Response:
credentials = get_credentials_from_headers(headers)
token = _request_credentials.set(credentials)
try:
async with sse_transport.connect_sse(...) as streams:
await mcp_app.run(...)
finally:
_request_credentials.reset(token)
return Response() # Unreachable if exception in try blockIssue: If an exception occurs before the Recommendation: Make exception handling explicit or add comment explaining intended behavior. 5. Code Duplication: Three Similar Polymarket Classes 🔵 LOWFiles: Significant code duplication in Recommendation: Consider refactoring to use composition or a base class with a factory method for client creation. Not critical for this PR but worth noting for future maintenance. 6. Missing Input Validation 🔵 LOWLocation: The Recommendation: Add Web3 address validation: if not Web3.is_address(wallet_address):
return False, "Invalid wallet address format."Performance ConsiderationsDocker Image OptimizationLocation: ✅ Good: Multi-stage layer caching, slim base image Minor suggestion: Consider multi-stage build to reduce final image size: FROM python:3.13-slim AS builder
# ... install deps ...
FROM python:3.13-slim
COPY --from=builder /usr/local/lib/python3.13/site-packages /usr/local/lib/python3.13/site-packagesTimeout HandlingLocation: ✅ Good: Proper timeout handling with ThreadPoolExecutor The timeout values (10s exchange, 5s client) seem reasonable for remote deployment. Security Concerns Summary
Test Coverage✅ Excellent: 372 lines of SSE server tests Missing:
Additional ObservationsWebsite Changes (25 files)Large number of website changes included. Consider whether these belong in a separate PR for easier review. The website files don't appear to have been reviewed in detail here. Documentation✅ Wiki documentation added for remote server setup RecommendationsBefore Merging
Future Enhancements
VerdictThis is a well-engineered PR with good security practices overall. The critical issue with global rate limiting should be addressed before merging to prevent DoS scenarios. The CORS configuration should also be tightened. After addressing the rate limiting issue and CORS headers, this will be ready to merge. Estimated Risk: MEDIUM-HIGH (due to rate limiting DoS vector) Great work on the comprehensive security model and clean architecture! 🚀 |
- Change route from /approve to /mcp - Add "Quick Start: Read-Only Mode" section with copy-paste config - No wallet required for read-only market data access - Rename "How it Works" to "Trading Mode Setup" for clarity
PR Review: SSE-based MCP Server for Remote DeploymentSummaryRequest changes - Critical issues found. This PR adds SSE transport for remote MCP deployment with good architecture but has critical security and configuration issues. Critical Issues1. Invalid Operator Address (website/src/wagmi.ts:32)
2. Signature Replay Vulnerability (dr_manhattan/mcp/utils/security.py)
3. Documentation/Implementation Mismatch
High Priority Issues4. Missing Operator Key Validation (polymarket_operator.py:70-74)
5. No On-Chain Approval Validation (polymarket_operator.py:297-305)
6. Missing Operator Address Discovery
7. Error Handling Gaps (server_sse.py:197-232)
Positive Aspects
RecommendationsBlock Merge:
Before Production: ConclusionSolid architecture and high-quality implementation in most areas. The operator mode is incomplete - either finish it or pivot to Builder mode as primary. Address signature replay before merging. |
Pull Request ReviewSummaryThis is a substantial and well-structured PR that adds remote MCP deployment via SSE transport. Code quality is high, but there are several critical security concerns that must be addressed before merge. Critical Issues (BLOCKERS)1. SECURITY: Operator Mode Lacks On-Chain Approval VerificationLocation: dr_manhattan/exchanges/polymarket_operator.py The server uses POLYMARKET_OPERATOR_KEY to sign orders but does NOT verify that users have approved the operator on-chain via setApprovalForAll(). The signature verification only proves wallet ownership, not operator approval. Recommendation: Add on-chain isApprovedForAll() check before accepting trading requests. 2. SECURITY: Missing Operator Address ValidationLocation: dr_manhattan/exchanges/polymarket_operator.py:70-74 Operator address should be: (1) Derived from private key, (2) Logged on startup, (3) Provided to users for approval. 3. SECURITY: CORS Too PermissiveLocation: dr_manhattan/mcp/server_sse.py:301 allow_headers=['*'] is too permissive. Use explicit allowlist. 4. Missing Wiki Documentation (CLAUDE.md Rule 6)No wiki documentation at wiki/exchanges/polymarket_operator.md. Required per CLAUDE.md when adding new exchange. High Priority Issues5. Dockerfile Security Hardening
6. Incomplete Environment Validation_validate_env() doesnt validate POLYMARKET_OPERATOR_KEY or CORS_ALLOWED_ORIGINS. 7. Test Coverage GapsMissing tests for: signature expiry edge cases, operator approval verification, rate limiting, CORS handling. 8. Fragile Logger PatchingComplex monkey-patching violates CLAUDE.md rule 1 (clean, easy to understand). Use logging config instead. Positive Aspects
RecommendationDO NOT MERGE until critical issues 1-4 are resolved. Once fixed, this will be an excellent addition. Post-fix recommendations:
Great work overall! The architecture is solid. Security issues are fixable and dont detract from the design quality. |
Comprehensive Code Review - PR #71Overall Assessment: APPROVE WITH REQUIRED CHANGES This is a well-engineered PR (11,732 additions, 4,034 deletions) that significantly improves the codebase architecture and adds valuable remote deployment capabilities. The Polymarket refactoring from a 1,930-line monolith into a modular package is excellent. However, several security issues and missing safeguards must be addressed before production use. Blocking Issues (Must Fix Before Merge)1. Operator Approval Not VerifiedLocation: dr_manhattan/exchanges/polymarket/polymarket_operator.py:305 Risk: HIGH - Orders attempted even if user has not approved operator on-chain 2. Missing CTF Operation TestsStatus: No tests found for split/merge/redeem functionality 3. HMAC Body Serialization BugLocation: dr_manhattan/exchanges/polymarket/polymarket_ctf.py:165 The code uses str(body).replace("'", '"') which does not match JSON format reliably. Python dict repr is not JSON. 4. Silent Exception Handling in CTF RelayerLocation: dr_manhattan/exchanges/polymarket/polymarket_ctf.py:256-258 Silent except Exception: pass in _poll_transaction hides network errors and transaction status. High Priority (Before Production)5. Operator Key Rotation Not DocumentedServer-wide operator private key in POLYMARKET_OPERATOR_KEY has no rotation mechanism. 6. Rate Limiting Is Server-Wide OnlyCurrent 10 calls/sec limit is per-server, not per-user. Multiple users can collectively exceed API rate limits. 7. Missing Integration TestsCoverage gaps: PolymarketBuilder order flow (85 lines unit tests only), PolymarketOperator with mock approval (75 lines), SSE concurrent requests, rate limiter burst scenarios Excellent WorkArchitecture & RefactoringPolymarket Package Split: Outstanding refactoring from 1,930-line monolith into clean modular structure with single responsibility per module. Import path unchanged for backward compatibility. Security Implementation
Test CoverageSSE Server Tests (372 lines): Excellent coverage of credential extraction, masking, write restrictions, health check, environment validation Docker & DeploymentDockerfile: Excellent implementation following CLAUDE.md rules with layer caching, uv for dependencies, Python 3.13-slim, clever health check without curl Security Checklist
Code Metrics
Recommendations SummaryMust Fix (Blocking):
Should Fix (Before Production):
Nice to Have:
VerdictAPPROVE pending the 4 blocking fixes. This PR represents significant architectural improvements and adds valuable remote deployment capabilities. The security model with Builder and Operator modes is well thought out. Address the blocking issues and document the security considerations, and this will be ready for production. Great work on the modular refactoring! The code is much more maintainable now. |
minkyun12
force-pushed
the
feat/deploy-mcp-sse-server
branch
from
c2b13bb to
7de2707
Compare
Pull Request Review: SSE-based MCP Server for Remote DeploymentSummaryThis PR adds significant infrastructure for remote MCP server deployment via SSE transport, along with Polymarket Builder and Operator pattern support. Overall, the implementation is well-structured and production-ready, with strong security considerations. However, there are several areas that need attention before merging. Critical Issues1. Operator Mode Security Vulnerability (HIGH PRIORITY)Location: dr_manhattan/exchanges/polymarket_operator.py:70 The operator mode stores a sensitive private key (POLYMARKET_OPERATOR_KEY) in environment variables on the server. This creates a single point of failure:
Recommendation: Add comprehensive documentation warning about this security model, implement per-user spending limits, and document the revocation process clearly. 2. Missing CORS Security in ProductionLocation: dr_manhattan/mcp/server_sse.py:287-294 The CORS configuration allows unrestricted override via env var with no HTTPS validation. Recommendation: Add HTTPS validation for all CORS origins in production mode. 3. Signature Expiry Validation GapLocation: dr_manhattan/mcp/utils/security.py:286-296 A 90-day signature expiry is too long for security. Clock skew tolerance is undocumented. Recommendation: Document security tradeoffs and consider shorter defaults with warnings for long expiries. Security Concerns4. Credential Zeroization is IncompleteLocation: dr_manhattan/mcp/session/exchange_manager.py:161-179 Python strings are immutable, so setting to empty string doesn't overwrite memory. No gc.collect() call. Recommendation: Add gc.collect() after zeroization to force memory cleanup. 5. Sensitive Data in Error MessagesLocation: dr_manhattan/mcp/utils/security.py:106-119 Third-party library errors might leak credentials. Regex patterns don't cover all formats. Recommendation: Wrap ALL external API calls in sanitizing try-except handlers. 6. No Rate Limiting Implementation DetailsLocation: dr_manhattan/mcp/server_sse.py:169 Rate limiting is called but implementation and strategy are not documented. Recommendation: Document rate limiting strategy in wiki/mcp/remote-server.md Code Quality Issues7. Logger Monkey-Patching is FragileLocation: dr_manhattan/mcp/server_sse.py:37-70 Import-order dependent patching is error-prone and hard to maintain. 8. Inconsistent Error HandlingLocation: dr_manhattan/exchanges/polymarket_builder.py:169-170 Error messages vary between generic and specific formats. 9. Magic Numbers Without ConstantsTimeout values (10.0s, 5.0s) and clock skew (300s) are hardcoded throughout. 10. Missing Input ValidationLocation: dr_manhattan/exchanges/polymarket_builder.py:104-143 No validation of price bounds, size minimums, or market status. Performance & Architecture11. No Connection PoolingEach ClobClient may create new connections leading to potential port exhaustion. 12. Timeout Strategy Needs DocumentationTimeout values aren't justified, no exponential backoff. 13. Singleton Pattern ComplexityThe pattern could be simplified with a module-level instance. Strengths
Minor Issues
Recommendations Before MergeMust Fix (Blocking)
Should Fix (High Priority)
Final VerdictThis PR represents significant, high-quality work with strong security considerations. The architecture is sound and follows best practices. However, the operator mode security model needs more documentation and safeguards before production deployment. Recommendation: Request changes for critical security issues and documentation, then approve after fixes. Great work overall! The remote MCP deployment is a powerful feature. |
* Adds PM CTF split/merge/redeem functionality
Implements split, merge, and redeem operations for Conditional Token Framework (CTF) tokens on Polymarket.
Introduces new methods for splitting USDC into conditional tokens, merging them back, and redeeming winning tokens from resolved markets.
Leverages Polymarket's Builder API and relayer infrastructure for transaction submission and confirmation.
Adds helper functions for encoding transaction data and signing Safe transactions.
* refactor: split polymarket.py into mixin modules + add Data/Gamma/WebSocket APIs
- Split 2564-line monolith into 6 mixin files:
- polymarket_core.py: constants, init, request helpers
- polymarket_clob.py: CLOB API (orders, positions, balance)
- polymarket_gamma.py: Gamma API (markets, search, tags)
- polymarket_data.py: Data API (trades, analytics)
- polymarket_ctf.py: CTF operations (split/merge/redeem)
- polymarket_ws_ext.py: Sports/RTDS WebSocket
- New Gamma API methods: fetch_events, fetch_event, fetch_event_by_slug,
fetch_series, fetch_series_by_id, fetch_comments, fetch_profile,
fetch_sports_metadata
- New Data API methods: fetch_leaderboard, fetch_user_activity,
fetch_top_holders, fetch_open_interest, fetch_closed_positions,
fetch_accounting_snapshot
- New WebSocket: PolymarketSportsWebSocket, PolymarketRTDSWebSocket
- Import path unchanged: from dr_manhattan.exchanges.polymarket import Polymarket
- All existing function logic preserved as-is
* refactor: move polymarket modules into exchanges/polymarket/ package
- polymarket.py → polymarket/__init__.py
- All mixin files moved into polymarket/ directory
- Updated import paths (.. → ...) for package depth
- Fixed exchanges/__init__.py imports for Builder/Operator
- All imports verified working
* docs: update README with new polymarket package structure
* fix: correct leaderboard endpoint to /v1/leaderboard with proper params
- Fixed endpoint path: /leaderboard → /v1/leaderboard
- Added params: category, timePeriod, orderBy (from docs)
- Verified working with real API calls
* feat: add remaining CLOB/Gamma/Data/Bridge API endpoints
* fix: fetch_live_volume param name (eventId→id), fetch_supported_assets response parsing
- polymarket_data.py: fetch_live_volume used 'eventId' but API requires 'id'
- polymarket_bridge.py: fetch_supported_assets now handles {supportedAssets:[...]} response
- Added VERIFICATION_REPORT.md with full API verification results
* fix: add default side param to get_price()
* remove: drop non-working endpoints (batch CLOB, comments, profile, bridge status)
Polymarket API issues:
- /books, /prices, /spreads: batch endpoints disabled by Polymarket
- /comments: parameter format undocumented, always returns 422
- /profiles: requires authentication (401)
- /status/{addr}: server 500 error
* chore: remove broken fetch_accounting_snapshot endpoint
* feat: unify market identifiers - all market methods accept Market | str
- Add _resolve_condition_id, _resolve_gamma_id, _resolve_token_id helpers
- fetch_market() now accepts condition_id, token_id, gamma_id, slug, or Market
- get_price/get_midpoint/get_orderbook accept Market | str with outcome param
- fetch_token_ids/fetch_open_interest/fetch_top_holders accept Market | str
- split/merge/redeem accept Market | str
- fetch_market_tags accepts Market | str (auto-resolves gamma_id)
- _resolve_token_id handles condition_id (0x...) by fetching token_ids via CLOB
* feat: warn when multiple markets match in fetch_market
* docs: add comprehensive README for polymarket package
* chore: link polymarket docs in README, fix target-version to py311
* chore: remove test scripts and pycache
* chore: fix ruff lint errors and format polymarket package
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: guzus <storminggalaxys3@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
PR #71 Code Review: Dr. Manhattan MCP Server & Polymarket IntegrationExecutive SummaryThis PR represents a significant architectural refactoring and feature addition to Dr. Manhattan. It introduces:
Overall assessment: High quality implementation with solid security practices, though some edge cases and improvements are worth addressing. Critical Issues (Fix Before Merge)1. Global Variable Bug in
|
PR Review: Remote MCP Server ImplementationI've completed a thorough review of this PR. Overall, this is excellent work - the implementation is well-structured, secure, and follows best practices. Below are my findings organized by category. ✅ StrengthsArchitecture & Design
Security
Code Quality
🔧 Issues Found1. CRITICAL: Operator Key Security Risk (High Priority)Location: Issue: The Current Code: self._operator_key = os.getenv("POLYMARKET_OPERATOR_KEY")Recommendations:
2. Security: CORS Wildcard Risk (Medium Priority)Location: Issue: The default CORS configuration allows Current Code: ALLOWED_ORIGINS: List[str] = [o.strip() for o in _cors_origins_env.split(",") if o.strip()]
if not ALLOWED_ORIGINS:
ALLOWED_ORIGINS = [
"https://claude.ai",
"https://console.anthropic.com",
]Recommendations:
3. Bug: Missing Rate Limiting Implementation (Medium Priority)Location: Issue: The code calls Current Code: if not check_rate_limit():
raise ValueError("Rate limit exceeded. Please wait before making more requests.")Recommendations:
4. Performance: Potential Memory Leak (Low Priority)Location: Issue: In SSE mode with operator credentials, a new exchange instance is created for each request without caching. For high-traffic deployments, this could cause memory churn. Current Code: # Create exchange without caching (each user has different credentials)
return self._create_exchange_with_credentials(exchange_name, exchange_creds)Recommendations:
5. Code Quality: Inconsistent Error Handling (Low Priority)Location: Multiple files Issue: Some functions return Examples:
Recommendations:
6. Testing: Missing Integration Tests (Medium Priority)Issue: The tests in
Recommendations:
7. Documentation: Operator Address Not Exposed (Low Priority)Location: Issue: Users need to know the operator address to approve it on-chain, but the operator address isn't clearly exposed through an API endpoint. Recommendations:
📝 Minor Suggestions
🎯 Recommendations SummaryMust Fix Before Merge
Should Fix Soon
Nice to Have
🏆 Overall AssessmentThis PR is production-ready with minor security hardening. The code quality is high, the architecture is sound, and the security model is well-thought-out. The operator pattern is clever and avoids the major security risk of storing user private keys. Recommendation: Approve with minor changes requested (security warnings + rate limiting clarification). Great work on this feature! The remote MCP server will significantly improve the user experience for Claude integration. Review completed using Claude Sonnet 4.5 |
PR Review: feat(mcp): add SSE server for remote MCP deploymentOverviewThis is a substantial and well-architected PR that adds SSE-based MCP server support for remote deployment, along with a major Polymarket codebase refactoring. The implementation demonstrates strong security practices and thoughtful design decisions. Overall Assessment: APPROVE with minor suggestions Strengths1. Security ImplementationThe security utilities in dr_manhattan/mcp/utils/security.py are excellent:
Security Best Practices Observed:
2. SSE Server Designdr_manhattan/mcp/server_sse.py shows careful attention to detail:
3. Polymarket RefactoringThe monolithic polymarket.py (1930 lines) split into focused modules:
4. Test Coverage532 lines of new tests across 3 files covering credential extraction, masking, and validation 5. Dockerfile QualityClean, production-ready Dockerfile with layer caching, UV usage, health check, and slim base image Issues & SuggestionsCritical IssuesNone found - the implementation is production-ready. Medium Priority
Low Priority
Code QualityAdherence to CLAUDE.md Rules
Security AssessmentThe security implementation is exemplary:
Recommendations:
Test CoveragePositive: 532 lines of new tests, security functions well-tested Gaps: Add tests for:
SummaryThis PR represents high-quality work with excellent security practices and thoughtful architecture. Recommendations Before Merge
Post-Merge Enhancements (Optional)
Verdict: LGTM with minor follow-up suggestions Great work on this implementation! The security-first approach and clean architecture are commendable. |
Pull Request Review: SSE-based MCP Server for Remote DeploymentSummaryThis PR adds significant infrastructure for remote MCP deployment via Server-Sent Events (SSE), containerization, and a web-based authentication flow. The implementation is well-thought-out with strong security considerations. ✅ StrengthsArchitecture & Design
Code Quality
|
🔴 Critical Issues1. SECURITY: .env.example Violates CLAUDE.md Rule #4File: .env.example:5-9 Issue: CLAUDE.md explicitly states "DO NOT place many variables in .env file. Place them in the code instead." These Builder API credentials are being added to .env when they should follow the pattern in exchange_manager.py where non-sensitive config uses code defaults. Recommendation: If these are truly sensitive (API keys), this is acceptable. However, if they're just configuration, they should use code defaults with optional env override like POLYMARKET_SIGNATURE_TYPE does. 2. SECURITY: Operator Mode - Missing Server Private Key ValidationFile: dr_manhattan/mcp/session/exchange_manager.py:256-277 The operator mode implementation checks for user_address but doesn't validate that the server's private key is configured. The server needs its own private key to sign orders on behalf of users in operator mode. If POLYMARKET_PRIVATE_KEY is not set in the server environment, this will fail at runtime when trying to place orders. Recommendation: Add validation before creating PolymarketOperator instance to ensure server private key exists. 3. SECURITY: Railway Public Deployment ConcernsContext: PR description mentions Railway deployment at production URL Issues: b) Operator Mode Risk: If the server's private key has significant funds, a malicious user could drain it by sending valid signatures for their own wallet and having the server sign malicious transactions. Recommendations:
|
|
💡 Suggestions & Minor Issues7. Magic Numbers in Security ModuleFile: dr_manhattan/mcp/utils/security.py:303 Use named constants instead of magic number 300 for clock skew tolerance. 8. Missing Error Context in SSE HandlerFile: dr_manhattan/mcp/server_sse.py:186-189 Errors are caught but original exception context is lost. Log the full traceback with logger.exception() for debugging. 9. Website Config Hardcoded URLFile: website/src/pages/ApprovePage.tsx:92 Hardcoded production URL makes it hard for users to test against local/staging instances. Make this configurable via environment variable at build time. 10. Missing CORS Preflight TestThe SSE server allows OPTIONS method but there are no tests for CORS preflight requests. Given the security implications, this should be tested. |
📊 Statistics & Final VerdictFiles Changed: 62 (manageable for a feature PR) Code Distribution:
🎯 RecommendationsMust Fix (Before Merge)
Should Fix (Before Production)
Nice to Have
✨ Final VerdictThis is a well-designed PR with strong security awareness. The code quality is high, tests are comprehensive, and the architecture is solid. However, the critical security issues (#2, #3) must be addressed before merging, especially for a public-facing deployment. The Polymarket refactoring is a huge win for maintainability, and the SSE implementation follows best practices. Once the security concerns are addressed, this will be a great addition to the project. Recommendation: Request changes for critical issues, then approve. 📚 Additional Notes
Great work overall! 🚀 |
3 participants
Summary
Closes #50
Test plan
python -m dr_manhattan.mcp.server_sse@claude
Generated with Claude Code