fix: optimize Neo4j Community Edition support and enhance MCP environment loading #754
Conversation
fancyboi999 force-pushed the fix/neo4j-community-and-mcp-env branch 3 times, most recently from 0deea14 to 8527232 Compare 
| Hi! |
fancyboi999 force-pushed the fix/neo4j-community-and-mcp-env branch from 8527232 to ce1cdd5 Compare 
Already Done！ 😍 |
fancyboi999 force-pushed the fix/neo4j-community-and-mcp-env branch from ce1cdd5 to ddded82 Compare fancyboi999 force-pushed the fix/neo4j-community-and-mcp-env branch from ddded82 to 85e385d Compare 
…ment loading - Update default Neo4j DB name to 'neo4j' for Community Edition compatibility - Add neo4j_auto_create and use_multi_db configuration options - Enhance MCP server to load all relevant environment variables from .env - Add defensive error handling in Neo4j driver for administrative commands on Community Edition - Update .env.example with detailed Neo4j setup instructions
fancyboi999 force-pushed the fix/neo4j-community-and-mcp-env branch from 85e385d to e88c928 Compare There was a problem hiding this comment.
Pull request overview
This PR enhances Neo4j Community Edition support and improves MCP server environment variable loading. The changes add graceful error handling for Community Edition's lack of administrative commands and introduce a comprehensive environment variable mapping system for the MCP server.
Key Changes:
- Added error handling in Neo4j driver to catch and warn about unsupported administrative commands instead of crashing
- Implemented comprehensive environment variable mapping in MCP server with support for both clean and MOS-prefixed variable names
- Changed default scheduler consume interval from 0.01s to 3s and made neo4j_auto_create configurable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/memos/graph_dbs/neo4j.py | Added graceful error handling for Community Edition's unsupported administrative commands |
| src/memos/mem_os/utils/default_config.py | Updated scheduler interval default, added Community Edition documentation, changed default db_name to "neo4j", and made auto_create configurable |
| src/memos/api/mcp_serve.py | Implemented comprehensive environment variable mapping with type conversion, fixed cube auto-registration, and removed redundant env variable assignments |
| docker/.env.example | Added NEO4J_AUTO_CREATE and NEO4J_USE_MULTI_DB variables with Community Edition documentation |
Comments suppressed due to low confidence (1)
src/memos/mem_os/utils/default_config.py:113
- The default value for consume_interval_seconds has been changed from 0.01 seconds (10ms) to 3 seconds, which is a 300x increase. This significant change in polling frequency could dramatically affect scheduler responsiveness and performance. Such a substantial change to a default value should be accompanied by either a comment explaining the reasoning or mentioned in the PR description, especially since it affects all users who don't explicitly override this value.
"consume_interval_seconds": kwargs.get("scheduler_consume_interval_seconds", 0.01), 💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env_mapping = { | ||
| "OPENAI_API_KEY": "openai_api_key", | ||
| "OPENAI_API_BASE": "openai_api_base", | ||
| "MOS_TEXT_MEM_TYPE": "text_mem_type", | ||
| "NEO4J_URI": "neo4j_uri", | ||
| "NEO4J_USER": "neo4j_user", | ||
| "NEO4J_PASSWORD": "neo4j_password", | ||
| "NEO4J_DB_NAME": "neo4j_db_name", | ||
| "NEO4J_AUTO_CREATE": "neo4j_auto_create", | ||
| "NEO4J_USE_MULTI_DB": "use_multi_db", | ||
| "MOS_NEO4J_SHARED_DB": "mos_shared_db", # Special handle later | ||
| "MODEL_NAME": "model_name", | ||
| "MOS_CHAT_MODEL": "model_name", | ||
| "EMBEDDER_MODEL": "embedder_model", | ||
| "MOS_EMBEDDER_MODEL": "embedder_model", | ||
| "CHUNK_SIZE": "chunk_size", | ||
| "CHUNK_OVERLAP": "chunk_overlap", | ||
| "ENABLE_MEM_SCHEDULER": "enable_mem_scheduler", | ||
| "MOS_ENABLE_SCHEDULER": "enable_mem_scheduler", | ||
| "ENABLE_ACTIVATION_MEMORY": "enable_activation_memory", | ||
| "TEMPERATURE": "temperature", | ||
| "MOS_CHAT_TEMPERATURE": "temperature", | ||
| "MAX_TOKENS": "max_tokens", | ||
| "MOS_MAX_TOKENS": "max_tokens", | ||
| "TOP_P": "top_p", | ||
| "MOS_TOP_P": "top_p", | ||
| "TOP_K": "top_k", | ||
| "MOS_TOP_K": "top_k", | ||
| "SCHEDULER_TOP_K": "scheduler_top_k", | ||
| "MOS_SCHEDULER_TOP_K": "scheduler_top_k", | ||
| "SCHEDULER_TOP_N": "scheduler_top_n", | ||
| } |
There was a problem hiding this comment.
The env_mapping dictionary contains duplicate entries that map to the same parameter. For example, both MODEL_NAME and MOS_CHAT_MODEL map to "model_name" (lines 42-43). If both environment variables are set, the later one (MOS_CHAT_MODEL) will silently overwrite the earlier one (MODEL_NAME), which may lead to unexpected behavior. The comment states this is for "compatibility", but the implementation doesn't properly handle precedence when both are present.
| "db_name": db_name, | ||
| "password": kwargs.get("neo4j_password", "12345678"), | ||
| "auto_create": True, | ||
| "auto_create": kwargs.get("neo4j_auto_create", True), |
There was a problem hiding this comment.
The comment at line 189 instructs users to "Set 'auto_create' to False", but the default value for neo4j_auto_create at line 199 is True. This creates a contradiction between the documentation and the actual default behavior. If Community Edition users don't explicitly set NEO4J_AUTO_CREATE=false in their environment, they will still encounter the "CREATE DATABASE" permission errors that this PR aims to prevent.
| "auto_create": kwargs.get("neo4j_auto_create", True), | |
| "auto_create": kwargs.get("neo4j_auto_create", False), |
| # Handle boolean conversions | ||
| if val.lower() in ("true", "false"): | ||
| kwargs[param_key] = val.lower() == "true" | ||
| else: | ||
| # Try numeric conversions (int first, then float) | ||
| try: | ||
| if "." in val: | ||
| kwargs[param_key] = float(val) | ||
| else: | ||
| kwargs[param_key] = int(val) | ||
| except ValueError: | ||
| kwargs[param_key] = val |
There was a problem hiding this comment.
The numeric type conversion logic attempts to convert all non-boolean string values to numbers. This will incorrectly convert string values that happen to be numeric but should remain strings. For example, if someone sets NEO4J_DB_NAME="123" or NEO4J_PASSWORD="456", these will be converted to integers instead of remaining as strings.
The conversion should only be attempted for parameters where numeric values are expected (e.g., chunk_size, chunk_overlap, max_tokens, etc.), not for all parameters universally.
| cube_to_register, mem_cube_id=cube_id, user_id=user_id | ||
| ) | ||
| return f"Cube registered successfully: {cube_id or cube_name_or_path}" | ||
| return f"Cube registered successfully: {cube_id or cube_to_register}" |
There was a problem hiding this comment.
The return message for cube registration is inconsistent. When a new cube is created (path doesn't exist), it returns cube_id or cube (the object), but when registering an existing cube, it returns cube_id or cube_name_or_path (the string path). If cube_id is None and a new cube was created, this will return a string representation of the cube object instead of a meaningful identifier.
| try: | ||
| if not os.path.exists(cube_name_or_path): | ||
| mos_config, cube_name_or_path = load_default_config(user_id=user_id) | ||
| _, cube = load_default_config(user_id=user_id) |
There was a problem hiding this comment.
When calling load_default_config with a user_id parameter in the register_cube function, the user_id might be None. The load_default_config function will then create a configuration with user_id=None, which gets passed to kwargs["user_id"] at line 64. This could lead to unexpected behavior if the downstream functions don't handle None user_id properly.
| if (val.startswith('"') and val.endswith('"')) or ( | ||
| val.startswith("'") and val.endswith("'") |
There was a problem hiding this comment.
The quote stripping logic doesn't handle single character values properly. If an environment variable is set to a single quote character (val = "'") or a single double quote character (val = '"'), the conditions will pass but val[1:-1] will result in an empty string, which may not be the intended behavior. While this is an edge case, it could cause silent failures.
| if (val.startswith('"') and val.endswith('"')) or ( | |
| val.startswith("'") and val.endswith("'") | |
| if len(val) > 1 and ( | |
| (val.startswith('"') and val.endswith('"')) | |
| or (val.startswith("'") and val.endswith("'")) |
| MOS_NEO4J_SHARED_DB=true # if true, all users share one DB; if false, each user gets their own DB | ||
| NEO4J_AUTO_CREATE=false # [IMPORTANT] set to false for Neo4j Community Edition | ||
| NEO4J_USE_MULTI_DB=false # alternative to MOS_NEO4J_SHARED_DB (logic is inverse) |
There was a problem hiding this comment.
The configuration has both MOS_NEO4J_SHARED_DB=true and NEO4J_USE_MULTI_DB=false, which according to the comment have "inverse logic". Setting MOS_NEO4J_SHARED_DB=true means users share one DB (no multi-db), and NEO4J_USE_MULTI_DB=false also means no multi-db. However, if someone changes one without changing the other, they could end up with conflicting settings. The example should either use only one of these variables or include a comment warning about keeping them synchronized.
| MOS_NEO4J_SHARED_DB=true # if true, all users share one DB; if false, each user gets their own DB | |
| NEO4J_AUTO_CREATE=false # [IMPORTANT] set to false for Neo4j Community Edition | |
| NEO4J_USE_MULTI_DB=false # alternative to MOS_NEO4J_SHARED_DB (logic is inverse) | |
| MOS_NEO4J_SHARED_DB=true # if true, all users share one DB (no multi-db); if false, each user gets their own DB (multi-db) | |
| NEO4J_AUTO_CREATE=false # [IMPORTANT] set to false for Neo4j Community Edition | |
| NEO4J_USE_MULTI_DB=false # inverse of MOS_NEO4J_SHARED_DB: when MOS_NEO4J_SHARED_DB=true, this MUST be false; when MOS_NEO4J_SHARED_DB=false, this MUST be true |
| "MOS_TOP_K": "top_k", | ||
| "SCHEDULER_TOP_K": "scheduler_top_k", | ||
| "MOS_SCHEDULER_TOP_K": "scheduler_top_k", | ||
| "SCHEDULER_TOP_N": "scheduler_top_n", |
There was a problem hiding this comment.
The environment variable mapping is missing the MOS-prefixed variant for SCHEDULER_TOP_N. While SCHEDULER_TOP_N is mapped, there's no MOS_SCHEDULER_TOP_N mapping for consistency with other scheduler parameters like MOS_SCHEDULER_TOP_K. This breaks the pattern of supporting both clean names and MOS-prefixed names mentioned in the comment at line 30.
| "SCHEDULER_TOP_N": "scheduler_top_n", | |
| "SCHEDULER_TOP_N": "scheduler_top_n", | |
| "MOS_SCHEDULER_TOP_N": "scheduler_top_n", |
2 participants
Description
This PR addresses two main issues: Neo4j Community Edition compatibility and the incomplete environment variable loading in the MCP server.
1. Neo4j Community Edition Optimization
Users on the Community Edition encountered crashes because the system attempted to execute administrative commands (like
CREATE DATABASE) which are Enterprise-only features.Neo4jGraphDBto catchUnsupported administration commanderrors, logging a warning instead of crashing.neo4jand exposedneo4j_auto_createanduse_multi_dboptions.default_config.pyanddocker/.env.examplespecifically for Community Edition users.2. MCP Server Enhancements
Improved the
mcp_serve.pyto be more robust and easier to configure via.envfiles.MODEL_NAME) and MOS-prefixed names (e.g.,MOS_CHAT_MODEL) for better backward compatibility with.env.example.Key Changes
src/memos/graph_dbs/neo4j.py(Error handling)src/memos/mem_os/utils/default_config.py(Default values & docs)src/memos/api/mcp_serve.py(Env loading logic & tool setup)docker/.env.example(Updated guidance)Verification
.env.