Feature/refactor mcp server - #160
Conversation
…dis with mcp tools integration - Added middleware tenant interceptor using x-tenant-id - Configured central global.tenants registry lookup - Implemented Tenant DB pool manager & dynamic redis_config_registry resolution - Connected isolated Redis and Postgres connection pools to MCP tool execution layer - Added background TTL eviction for idle connection pool cleanup
…is/Postgres pooling and TTL eviction
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| except RedisError as e: | ||
| return f"Error retrieving vector field '{vector_field}' from hash '{name}': {str(e)}" | ||
| return f"Error retrieving vector field '{vector_field}' from hash '{name}': {str(e)}" No newline at end of file |
There was a problem hiding this comment.
Vector retrieval broken by decode_responses=True client
High Severity
All tenant Redis clients are created with decode_responses=True. The get_vector_from_hash function retrieves binary float32 blobs via r.hget(), but with decode_responses=True the client attempts UTF-8 decoding of arbitrary binary bytes. This raises UnicodeDecodeError (not a RedisError), causing an unhandled exception. The old code explicitly used decode_responses=False for this operation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cf4a9d7. Configure here.
|
|
||
| # return create_from_default_azure_credential( | ||
| # scopes=scopes, token_manager_config=token_manager_config | ||
| # ) |
There was a problem hiding this comment.
Entire file commented out instead of deleted
Low Severity
The entire entraid_auth.py file (162 lines) is commented out with # prefixes rather than being removed. This leaves dead code in the repository that adds noise, confuses future developers, and increases maintenance burden without providing any value.
Reviewed by Cursor Bugbot for commit cf4a9d7. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
There are 5 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 8ec1f73. Configure here.
| "username": config_working.get("REDIS_USERNAME"), | ||
| "password": config_working.get("REDIS_PWD"), | ||
| "ssl": config_working.get("REDIS_SSL", False), | ||
| "decode_responses": True, |
There was a problem hiding this comment.
Binary vector retrieval broken by forced decode_responses=True
High Severity
All Redis clients are now created with decode_responses=True hardcoded, but get_vector_from_hash needs raw bytes to reconstruct vectors via np.frombuffer. The old code explicitly used decode_responses=False for this function. With the new code, hget will attempt UTF-8 decoding of binary float32 data, raising a UnicodeDecodeError that isn't caught by the except RedisError handler, crashing the tool entirely.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8ec1f73. Configure here.
| "fastapi>=0.139.2", | ||
| "uvicorn>=0.37.0", | ||
| "asyncpg>=0.31.0", | ||
| "cryptography>=48.0.1", |
There was a problem hiding this comment.
CLI entrypoint references removed function
Medium Severity
The [project.scripts] entry still references src.main:cli, but the cli function (the Click command) was completely removed from src/main.py during this refactor. Installing the package and running redis-mcp-server will fail with an AttributeError.
Reviewed by Cursor Bugbot for commit 8ec1f73. Configure here.
| logger = logging.getLogger("mcp.tenant_db") | ||
|
|
||
| CENTRAL_DB_URL = os.environ.get("CENTRAL_DATABASE_URL", "postgresql://postgres:Admin12@localhost:5432/global") | ||
| ENCRYPTION_SECRET = os.environ.get("CREDENTIAL_ENCRYPTION_KEY", "MySecure32CharacterEncryptKey!!!") |
There was a problem hiding this comment.
Hardcoded default credentials in production code
High Severity
CENTRAL_DB_URL and ENCRYPTION_SECRET have hardcoded default values containing a database password (Admin12) and a static AES encryption key. If these environment variables are not explicitly set in production, the server silently falls back to these insecure defaults, potentially exposing the central registry or allowing credential decryption with a known key.
Reviewed by Cursor Bugbot for commit 8ec1f73. Configure here.
1 participant
Note
High Risk
Large architectural change affecting all Redis access paths, credential decryption, and deployment; removes prior CLI/env Redis and Entra ID integration without clear replacement in the diff.
Overview
Replaces the stdio / Click CLI deployment model with a FastAPI + Uvicorn HTTP service on port 8000, with Docker updated to match (Python 3.12, layered
uv sync, HTTP CMD).Multi-tenancy is enforced in new
http_server.py: middleware requiresX-Tenant-ID(optionalX-MCP-ID), setsContextVartenant context, rewrites/mcp→/mcp/for gateway compatibility, runs periodic idle pool cleanup, and drains Redis/Postgres pools on shutdown. FastMCP is mounted at/mcpwith stateless streamable HTTP.Connection architecture is new:
AsyncLRUCachebacks per-tenant asyncpg pools (central registry + AES password decryption) and per-tenanttenant_id:mcp_idRedis clients loaded from each tenant’smcp_registryviaRedisMCPConfig.from_any(). Tools acrosssrc/tools/*nowawait tenant_redis_manager.get_client()instead of a singleton sync client.Config / auth regressions to note: env/CLI Redis and Entra ID wiring in
config.py/entraid_auth.pyis removed or commented out; Redis targets come from Postgres metadata instead.Reviewed by Cursor Bugbot for commit 8ec1f73. Bugbot is set up for automated code reviews on this repo. Configure here.