-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add Conductor configuration for development workspaces #944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add conductor.json with setup, run (REST API), and run-mcp scripts - Add conductor-setup.sh for automated dependency installation - Support both FastAPI server and MCP server development workflows - Include environment setup and initial code formatting 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
# Copy environment file if it exists in the root | ||
if [ -f "$CONDUCTOR_ROOT_PATH/.env" ]; then | ||
echo "📋 Copying environment configuration..." | ||
cp "$CONDUCTOR_ROOT_PATH/.env" .env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security concern: Using cp
without validation could potentially overwrite sensitive files. Consider adding validation to ensure the source file exists and is readable before copying:
cp "$CONDUCTOR_ROOT_PATH/.env" .env | |
if [ -f "$CONDUCTOR_ROOT_PATH/.env" ] && [ -r "$CONDUCTOR_ROOT_PATH/.env" ]; then | |
echo "📋 Copying environment configuration..." | |
cp "$CONDUCTOR_ROOT_PATH/.env" .env |
|
||
# Install server dependencies | ||
echo "📦 Installing server dependencies..." | ||
cd server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best practice improvement: The script changes directories multiple times without error handling. If cd server
fails, subsequent commands will run in the wrong directory. Consider using subshells or better error handling:
cd server | |
# Install server dependencies | |
echo "📦 Installing server dependencies..." | |
(cd server && uv sync --extra dev) || { | |
echo "❌ Failed to install server dependencies" | |
exit 1 | |
} |
|
||
# Run initial checks to ensure everything is working | ||
echo "🔍 Running initial checks..." | ||
uv run ruff check --select I --fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance consideration: Running ruff format
on the entire codebase during setup might be unnecessary and could modify files unexpectedly. Consider either:
- Just checking format without fixing:
uv run ruff check --select I
- Or making this optional with a flag
- Or limiting scope to avoid modifying existing code
uv run ruff check --select I --fix | |
# Run initial checks to ensure everything is working (without modifying files) | |
echo "🔍 Running initial checks..." | |
uv run ruff check --select I | |
echo "✨ Graphiti workspace setup complete!" |
"scripts": { | ||
"setup": "./conductor-setup.sh", | ||
"run": "cd server && uv run uvicorn graph_service.main:app --reload --port $CONDUCTOR_PORT", | ||
"run-mcp": "cd mcp_server && uv run python graphiti_mcp_server.py --transport sse --use-custom-entities" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Robustness suggestion: The MCP server command uses hardcoded flags that may not work in all environments. Consider making the transport and custom entities configurable via environment variables:
"run-mcp": "cd mcp_server && uv run python graphiti_mcp_server.py --transport sse --use-custom-entities" | |
"run-mcp": "cd mcp_server && uv run python graphiti_mcp_server.py --transport ${MCP_TRANSPORT:-sse} ${MCP_CUSTOM_ENTITIES:+--use-custom-entities}" |
This would allow users to override defaults while maintaining backward compatibility.
Overall ReviewThis PR adds Conductor workspace configuration files to enable quick development environment setup. The implementation is solid but has several areas for improvement: ✅ Strengths
🔧 Areas for ImprovementSecurity & Robustness:
Configuration:
📋 Recommendations
🧪 TestingThe PR description indicates some manual testing was done, but consider adding:
The core functionality looks good and will be valuable for team productivity. The suggested improvements would make it more robust and user-friendly. |
Summary
conductor.json
configuration with setup and run scripts for Conductor development workspacesconductor-setup.sh
script that installs dependencies and prepares workspaceKey Features
run
script starts FastAPI server with auto-reload and dynamic portrun-mcp
script starts MCP server with SSE transport for AI assistant integration.env
configuration from repository rootTest Plan
This enables team members to quickly set up consistent development environments using Conductor workspaces.
🤖 Generated with Claude Code