From 7520c8f8d759123e16506ead78f2d7cb07daf5c1 Mon Sep 17 00:00:00 2001 From: Daniel Chalef <131175+danielchalef@users.noreply.github.com> Date: Thu, 30 Oct 2025 16:25:34 -0700 Subject: [PATCH] Fix critical PR review issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed high-impact issues from PR #1024 code review: 1. **Boolean conversion bug (schema.py)** - Fixed _expand_env_vars returning strings 'true'/'false' instead of booleans - Now properly converts boolean-like strings (true/false/1/0/yes/no/on/off) to actual booleans - Simplified logic by removing redundant string-to-string conversions - Added support for common boolean string variations 2. **Dependency management (pyproject.toml)** - Removed pytest from main dependencies (now only in dev dependencies) - Moved azure-identity to optional dependencies under new [azure] group - Prevents forcing Azure and testing dependencies on all users 3. **Conditional Azure imports (utils.py)** - Made azure-identity import conditional in create_azure_credential_token_provider() - Raises helpful ImportError with installation instructions if not available - Follows lazy-import pattern for optional dependencies 4. **Documentation fix (graphiti_mcp_server.py)** - Fixed confusing JSON escaping in add_memory docstring example - Changed from triple-backslash escaping to standard JSON string - Updated comment to clarify standard JSON escaping is used Issues verified as already fixed: - Docker build context (all docker-compose files use context: ..) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- mcp_server/pyproject.toml | 5 +++-- mcp_server/src/config/schema.py | 26 ++++++++++---------------- mcp_server/src/graphiti_mcp_server.py | 4 ++-- mcp_server/src/utils/utils.py | 19 ++++++++++++++++--- 4 files changed, 31 insertions(+), 23 deletions(-) diff --git a/mcp_server/pyproject.toml b/mcp_server/pyproject.toml index ca67c2a1..d5de92c9 100644 --- a/mcp_server/pyproject.toml +++ b/mcp_server/pyproject.toml @@ -8,13 +8,14 @@ dependencies = [ "mcp>=1.9.4", "openai>=1.91.0", "graphiti-core[falkordb]>=0.16.0", - "azure-identity>=1.21.0", "pydantic-settings>=2.0.0", "pyyaml>=6.0", - "pytest>=8.4.1", ] [project.optional-dependencies] +azure = [ + "azure-identity>=1.21.0", +] providers = [ "google-genai>=1.8.0", "anthropic>=0.49.0", diff --git a/mcp_server/src/config/schema.py b/mcp_server/src/config/schema.py index eae275f3..31d73b9d 100644 --- a/mcp_server/src/config/schema.py +++ b/mcp_server/src/config/schema.py @@ -29,30 +29,24 @@ class YamlSettingsSource(PydanticBaseSettingsSource): def replacer(match): var_name = match.group(1) default_value = match.group(3) if match.group(3) is not None else '' - result = os.environ.get(var_name, default_value) - - # Convert string booleans to actual booleans - if result.lower() == 'true': - return 'true' # Keep as string, let Pydantic handle conversion - elif result.lower() == 'false': - return 'false' # Keep as string, let Pydantic handle conversion - return result + return os.environ.get(var_name, default_value) pattern = r'\$\{([^:}]+)(:([^}]*))?\}' - # Check if the entire value is a single env var expression with boolean default + # Check if the entire value is a single env var expression full_match = re.fullmatch(pattern, value) if full_match: result = replacer(full_match) - # If the result is a boolean string and the whole value was the env var, - # return the actual boolean - if result == 'true': - return True - elif result == 'false': - return False + # Convert boolean-like strings to actual booleans + if isinstance(result, str): + lower_result = result.lower().strip() + if lower_result in ('true', '1', 'yes', 'on'): + return True + elif lower_result in ('false', '0', 'no', 'off', ''): + return False return result else: - # Otherwise, do string substitution + # Otherwise, do string substitution (keep as strings for partial replacements) return re.sub(pattern, replacer, value) elif isinstance(value, dict): return {k: self._expand_env_vars(v) for k, v in value.items()} diff --git a/mcp_server/src/graphiti_mcp_server.py b/mcp_server/src/graphiti_mcp_server.py index 1be3f9b1..6f4306bb 100644 --- a/mcp_server/src/graphiti_mcp_server.py +++ b/mcp_server/src/graphiti_mcp_server.py @@ -286,10 +286,10 @@ async def add_memory( ) # Adding structured JSON data - # NOTE: episode_body must be a properly escaped JSON string. Note the triple backslashes + # NOTE: episode_body should be a JSON string (standard JSON escaping) add_memory( name="Customer Profile", - episode_body="{\\\"company\\\": {\\\"name\\\": \\\"Acme Technologies\\\"}, \\\"products\\\": [{\\\"id\\\": \\\"P001\\\", \\\"name\\\": \\\"CloudSync\\\"}, {\\\"id\\\": \\\"P002\\\", \\\"name\\\": \\\"DataMiner\\\"}]}", + episode_body='{"company": {"name": "Acme Technologies"}, "products": [{"id": "P001", "name": "CloudSync"}, {"id": "P002", "name": "DataMiner"}]}', source="json", source_description="CRM data" ) diff --git a/mcp_server/src/utils/utils.py b/mcp_server/src/utils/utils.py index 31c3afb3..ec9b4e51 100644 --- a/mcp_server/src/utils/utils.py +++ b/mcp_server/src/utils/utils.py @@ -2,11 +2,24 @@ from collections.abc import Callable -from azure.identity import DefaultAzureCredential, get_bearer_token_provider - def create_azure_credential_token_provider() -> Callable[[], str]: - """Create Azure credential token provider for managed identity authentication.""" + """ + Create Azure credential token provider for managed identity authentication. + + Requires azure-identity package. Install with: pip install mcp-server[azure] + + Raises: + ImportError: If azure-identity package is not installed + """ + try: + from azure.identity import DefaultAzureCredential, get_bearer_token_provider + except ImportError: + raise ImportError( + 'azure-identity is required for Azure AD authentication. ' + 'Install it with: pip install mcp-server[azure]' + ) from None + credential = DefaultAzureCredential() token_provider = get_bearer_token_provider( credential, 'https://cognitiveservices.azure.com/.default'