Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive Google Cloud integration for neo4j-agent-memory, adding native support for Vertex AI embeddings, Google's Agent Development Kit (ADK), and a Model Context Protocol (MCP) server for Cloud API Registry integration.
Changes:
- Vertex AI Embeddings provider supporting text-embedding-004 and gecko models with 768-dimensional embeddings
- Neo4jMemoryService implementing ADK's MemoryService interface for session persistence and semantic search
- MCP server with 5 tools (memory_search, memory_store, entity_lookup, conversation_history, graph_query) supporting stdio and SSE transports
- Cloud Run deployment templates with Dockerfile, Cloud Build configuration, and infrastructure setup
- Comprehensive documentation and examples demonstrating all Google Cloud features
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
src/neo4j_agent_memory/embeddings/vertex_ai.py |
Vertex AI embedder implementation with batch processing |
src/neo4j_agent_memory/integrations/google_adk/ |
ADK MemoryService and type conversions |
src/neo4j_agent_memory/mcp/ |
MCP server, handlers, and tool definitions |
tests/unit/ |
Comprehensive unit tests for all new components |
tests/integration/ |
Integration tests with real Neo4j and Vertex AI |
examples/google_cloud_integration/ |
Full-featured examples and demos |
deploy/cloudrun/ |
Production deployment templates |
docs/ |
Updated documentation with Google Cloud integration guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
|
|
||
| __all__ = [ | ||
| "Neo4jMemoryMCPServer", |
There was a problem hiding this comment.
The name 'Neo4jMemoryMCPServer' is exported by all but is not defined.
| """Unit tests for MCP handlers.""" | ||
|
|
||
| import json | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
There was a problem hiding this comment.
Import of 'patch' is not used.
| @@ -0,0 +1,164 @@ | |||
| """Unit tests for MCP tool definitions.""" | |||
|
|
|||
| import pytest | |||
There was a problem hiding this comment.
Import of 'pytest' is not used.
|
|
||
| # Check if Vertex AI is available | ||
| try: | ||
| import vertexai # noqa: F401 |
There was a problem hiding this comment.
Import of 'vertexai' is not used.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@johnymontana I've opened a new pull request, #52, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@johnymontana I've opened a new pull request, #53, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 99 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
examples/google-cloud-financial-advisor/frontend/src/components/Dashboard/CustomerDashboard.tsx
Outdated
Show resolved
Hide resolved
examples/google-cloud-financial-advisor/backend/src/api/routes/alerts.py
Outdated
Show resolved
Hide resolved
examples/google-cloud-financial-advisor/backend/src/models/investigation.py
Show resolved
Hide resolved
examples/google-cloud-financial-advisor/backend/src/api/routes/investigations.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 124 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._project_id = project_id | ||
| self._location = location | ||
| self._credentials = credentials | ||
| self._batch_size = min(batch_size, DEFAULT_BATCH_SIZE) |
There was a problem hiding this comment.
batch_size can be set to 0 or a negative number, which will make range(..., step=self._batch_size) raise at runtime. Consider validating batch_size >= 1 (raise a ValueError or clamp to 1) before using it.
| self._batch_size = min(batch_size, DEFAULT_BATCH_SIZE) | |
| self._batch_size = min(max(batch_size, 1), DEFAULT_BATCH_SIZE) |
| # Process in batches (Vertex AI limit is 250 per request) | ||
| for i in range(0, len(texts), self._batch_size): | ||
| batch = texts[i : i + self._batch_size] |
There was a problem hiding this comment.
batch_size can be set to 0 or a negative number, which will make range(..., step=self._batch_size) raise at runtime. Consider validating batch_size >= 1 (raise a ValueError or clamp to 1) before using it.
| # Process in batches (Vertex AI limit is 250 per request) | |
| for i in range(0, len(texts), self._batch_size): | |
| batch = texts[i : i + self._batch_size] | |
| # Determine and validate batch size | |
| batch_size = self._batch_size | |
| if batch_size is None or batch_size < 1: | |
| raise EmbeddingError(f"Invalid batch_size {batch_size!r}; batch_size must be an integer >= 1.") | |
| # Process in batches (Vertex AI limit is 250 per request) | |
| for i in range(0, len(texts), batch_size): | |
| batch = texts[i : i + batch_size] |
| anthropic = ["anthropic>=0.20.0"] | ||
| sentence-transformers = ["sentence-transformers>=2.2.0"] | ||
| vertex-ai = ["google-cloud-aiplatform>=1.38.0"] | ||
| google = ["google-cloud-aiplatform>=1.38.0"] # Alias for vertex-ai |
There was a problem hiding this comment.
The docs/examples install instructions use neo4j-agent-memory[google,mcp] to imply Vertex AI + ADK + MCP, but the google extra currently only installs google-cloud-aiplatform and does not include google-adk. Either (a) include google-adk (and any other required Google packages) in the google extra, or (b) update doc strings/examples to instruct installing [vertex-ai,google-adk,mcp] explicitly to avoid missing dependency surprises.
| google = ["google-cloud-aiplatform>=1.38.0"] # Alias for vertex-ai | |
| google = ["google-cloud-aiplatform>=1.38.0", "google-adk"] # Vertex AI + Google ADK |
| mcp = ["mcp>=1.0.0"] | ||
|
|
||
| # Google ADK | ||
| google-adk = ["google-adk>=0.1.0"] |
There was a problem hiding this comment.
The docs/examples install instructions use neo4j-agent-memory[google,mcp] to imply Vertex AI + ADK + MCP, but the google extra currently only installs google-cloud-aiplatform and does not include google-adk. Either (a) include google-adk (and any other required Google packages) in the google extra, or (b) update doc strings/examples to instruct installing [vertex-ai,google-adk,mcp] explicitly to avoid missing dependency surprises.
| # Google Cloud (Vertex AI + ADK + MCP) | ||
| pip install neo4j-agent-memory[google,mcp] |
There was a problem hiding this comment.
This install command currently doesn’t align with the package extras as defined in pyproject.toml (the google extra doesn’t include google-adk). Update the docs to match the actual extras, or adjust the extras so this command installs all components promised here.
| for tool in MEMORY_TOOLS: | ||
| if tool["name"] == name: | ||
| return tool |
There was a problem hiding this comment.
MEMORY_TOOLS.copy() is a shallow list copy, so callers can still mutate the inner dicts and accidentally affect global tool definitions; get_tool_by_name also returns the original dict. Consider returning deep copies (or immutable structures) to prevent surprising cross-call mutation.
| async def get_initialized_memory_service() -> FinancialMemoryService: | ||
| """Get the initialized memory service.""" | ||
| service = get_memory_service() | ||
| if not service._initialized: | ||
| await service.initialize() | ||
| return service |
There was a problem hiding this comment.
This route relies on private attributes (service._initialized, client._driver, client._database). Since the library now exposes MemoryClient.graph for Neo4j access, prefer using that public API (e.g., memory_service.client.graph.execute_read(...)) and/or add a public readiness method on FinancialMemoryService to avoid tight coupling to internals that may change.
| async with client._driver.session(database=client._database) as session: | ||
| result = await session.run(request.query, request.parameters) | ||
| records = await result.data() |
There was a problem hiding this comment.
This route relies on private attributes (service._initialized, client._driver, client._database). Since the library now exposes MemoryClient.graph for Neo4j access, prefer using that public API (e.g., memory_service.client.graph.execute_read(...)) and/or add a public readiness method on FinancialMemoryService to avoid tight coupling to internals that may change.
| ) | ||
|
|
||
| client = MemoryClient(settings) | ||
| await client.initialize() |
There was a problem hiding this comment.
MemoryClient usage elsewhere in this PR uses connect() / close() (and async with) rather than initialize(). If initialize() isn’t part of the public API, this script will fail at runtime. Consider switching to await client.connect() (or using async with MemoryClient(settings) as client:) to match the actual client lifecycle API.
| await client.initialize() | |
| await client.connect() |
| # Mount Google Cloud credentials for local development | ||
| - ${GOOGLE_APPLICATION_CREDENTIALS:-~/.config/gcloud/application_default_credentials.json}:/app/credentials.json:ro |
There was a problem hiding this comment.
Docker Compose does not reliably expand ~ in bind-mount paths, so the default value here can produce an invalid mount and break local startup. Prefer requiring an absolute path (or document/export it) and/or switch to ${HOME}/.config/... to make the default portable.
| # Mount Google Cloud credentials for local development | |
| - ${GOOGLE_APPLICATION_CREDENTIALS:-~/.config/gcloud/application_default_credentials.json}:/app/credentials.json:ro | |
| # Mount Google Cloud credentials for local development. | |
| # GOOGLE_APPLICATION_CREDENTIALS must be set to an absolute path on the host. | |
| - ${GOOGLE_APPLICATION_CREDENTIALS:?GOOGLE_APPLICATION_CREDENTIALS must be set to an absolute path on the host}:/app/credentials.json:ro |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 130 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from neo4j_agent_memory.embeddings.base import BaseEmbedder, Embedder | ||
| from neo4j_agent_memory.embeddings.openai import OpenAIEmbedder |
There was a problem hiding this comment.
Importing OpenAIEmbedder unconditionally can break base installs where the OpenAI dependency is optional (ImportError when importing neo4j_agent_memory.embeddings). Consider making OpenAIEmbedder a lazy import via __getattr__ (like VertexAIEmbedder), or ensure OpenAI is a required dependency of the base package.
| "description": ( | ||
| "Search across all memory types using hybrid vector + graph search. " | ||
| "Finds relevant messages, entities, preferences, and reasoning traces." | ||
| ), |
There was a problem hiding this comment.
The description says the search is across all memory types and includes reasoning traces, but the default memory_types omits traces. Either update the description to match the default behavior, or include traces in the default list.
| "memory_types": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "enum": ["messages", "entities", "preferences", "traces"], | ||
| }, | ||
| "description": "Types of memory to search (defaults to all)", | ||
| "default": ["messages", "entities", "preferences"], | ||
| }, |
There was a problem hiding this comment.
The description says the search is across all memory types and includes reasoning traces, but the default memory_types omits traces. Either update the description to match the default behavior, or include traces in the default list.
| Returns: | ||
| List of tool definitions in MCP format. | ||
| """ | ||
| return MEMORY_TOOLS.copy() |
There was a problem hiding this comment.
MEMORY_TOOLS.copy() is only a shallow copy of the list; callers can still mutate the underlying tool dicts (and impact future requests). Return a deep copy (e.g., via copy.deepcopy) or construct new dicts to keep tool definitions immutable for callers.
| self._batch_size = min(batch_size, DEFAULT_BATCH_SIZE) | ||
| self._task_type = task_type | ||
| self._embedding_model: TextEmbeddingModel | None = None | ||
| self._initialized = False |
There was a problem hiding this comment.
self._initialized is set but not used anywhere in this class. Either remove it to reduce state/complexity, or use it consistently (e.g., to short-circuit initialization checks or expose an initialized property).
| valueFrom: | ||
| secretKeyRef: | ||
| key: latest | ||
| name: neo4j-user |
There was a problem hiding this comment.
This Cloud Run manifest requires a neo4j-user Secret Manager secret, but the repo’s setup instructions/scripts shown in this PR primarily create neo4j-uri and neo4j-password. Either update the setup docs/scripts to also create neo4j-user, or change the manifest to use a plain env var (common default is neo4j).
| valueFrom: | |
| secretKeyRef: | |
| key: latest | |
| name: neo4j-user | |
| value: "neo4j" |
| pip install neo4j-agent-memory[google,mcp] | ||
|
|
||
| # Or for development | ||
| cd neo4j-agent-memory | ||
| pip install -e ".[google,mcp]" |
There was a problem hiding this comment.
The README advertises installing "all Google Cloud features" with [google,mcp], but ADK support is exposed as a separate extra (google-adk) in pyproject.toml. Consider updating these commands to include google-adk (e.g., [google,google-adk,mcp]) so the documented installation actually matches the described features.
| pip install neo4j-agent-memory[google,mcp] | |
| # Or for development | |
| cd neo4j-agent-memory | |
| pip install -e ".[google,mcp]" | |
| pip install neo4j-agent-memory[google,google-adk,mcp] | |
| # Or for development | |
| cd neo4j-agent-memory | |
| pip install -e ".[google,google-adk,mcp]" |
| query_upper = request.query.upper().strip() | ||
| forbidden_keywords = [ | ||
| "CREATE", | ||
| "MERGE", | ||
| "DELETE", | ||
| "REMOVE", | ||
| "SET", | ||
| "DROP", | ||
| "DETACH", | ||
| "CALL", | ||
| "LOAD", | ||
| ] | ||
|
|
||
| for keyword in forbidden_keywords: | ||
| if keyword in query_upper: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Write operations ({keyword}) are not allowed. Use read-only queries.", | ||
| ) |
There was a problem hiding this comment.
The validation uses substring matching, which will incorrectly reject legitimate read-only queries containing keywords inside identifiers (e.g., n.created_at contains CREATE after .upper()). Use a token-based check or a regex with word boundaries (and ideally ignore string literals/comments) to avoid false positives.
| # Execute query through the memory client | ||
| client = memory_service.client | ||
| async with client._driver.session(database=client._database) as session: | ||
| result = await session.run(request.query, request.parameters) | ||
| records = await result.data() |
There was a problem hiding this comment.
This route uses private MemoryClient internals (_driver, _database). Since this PR introduces MemoryClient.graph for custom queries, prefer using that public API (e.g., client.graph.execute_read(...)) to avoid tight coupling to internal attributes that can change.
| # Execute query through the memory client | |
| client = memory_service.client | |
| async with client._driver.session(database=client._database) as session: | |
| result = await session.run(request.query, request.parameters) | |
| records = await result.data() | |
| # Execute query through the memory client's public graph interface | |
| client = memory_service.client | |
| records = await client.graph.execute_read( | |
| request.query, | |
| request.parameters, | |
| ) |
| if not service._initialized: | ||
| await service.initialize() |
There was a problem hiding this comment.
Accessing service._initialized relies on a private attribute. Prefer a public readiness check (or always call await service.initialize() since it is already idempotent) to avoid depending on internal state.
| if not service._initialized: | |
| await service.initialize() | |
| await service.initialize() |
This PR implements integrations with Google Cloud's AI ecosystem for
neo4j-agent-memory. It adds native support for Google's Agent Development Kit (ADK), Vertex AI embeddings, and exposes memory capabilities via MCP server for Cloud API Registry integration.Features
1. Vertex AI Embeddings (VTX-001)
VertexAIEmbedderclass supporting:text-embedding-004(768 dimensions, recommended)textembedding-gecko@003(768 dimensions)textembedding-gecko-multilingual@001(768 dimensions)2. Google ADK Integration (ADK-001)
Neo4jMemoryServiceimplementing ADK'sMemoryServiceinterface3. MCP Server (MCP-001)
Neo4jMemoryMCPServerwith 5 tools for Cloud API Registry:memory_search- Hybrid vector + graph searchmemory_store- Store messages, facts, preferencesentity_lookup- Get entity with graph neighborsconversation_history- Retrieve session messagesgraph_query- Execute read-only Cypher queriesneo4j-memory mcp serve4. Cloud Run Template (RUN-001)
5. Documentation & Diagrams