-
Notifications
You must be signed in to change notification settings - Fork 8.2k
fix: MCP server update to support partial updates #10749
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
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 4 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/backend/base/langflow/api/v2/mcp.py (1)
245-249: Consider implementing complete PATCH semantics for all fields.The selective preservation of
envandargscorrectly solves the immediate issue described in the PR. However, this implementation only merges these two specific fields. A complete PATCH implementation would merge all fields from the existing configuration with the incoming one, preserving any omitted fields.If other configuration fields exist (e.g.,
command,disabled,timeout), they would still be lost when not included in the update payload. Consider whether a full merge is more appropriate:else: existing_config = server_list["mcpServers"].get(server_name, {}) - if "env" in existing_config and "env" not in server_config: - server_config["env"] = existing_config["env"] - if "args" in existing_config and "args" not in server_config: - server_config["args"] = existing_config["args"] - - server_list["mcpServers"][server_name] = server_config + # Merge existing config with new config (new values override existing) + merged_config = {**existing_config, **server_config} + server_list["mcpServers"][server_name] = merged_configThis approach would preserve all existing fields unless explicitly overridden. If the current selective approach is intentional (e.g., only
envandargsare complex fields users typically want preserved), then the current implementation is appropriate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/backend/base/langflow/api/v2/mcp.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
src/backend/**/*.py: Use FastAPI async patterns withawaitfor async operations in component execution methods
Useasyncio.create_task()for background tasks and implement proper cleanup with try/except forasyncio.CancelledError
Usequeue.put_nowait()for non-blocking queue operations andasyncio.wait_for()with timeouts for controlled get operations
Files:
src/backend/base/langflow/api/v2/mcp.py
src/backend/base/langflow/api/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
Backend API endpoints should be organized by version (v1/, v2/) under
src/backend/base/langflow/api/with specific modules for features (chat.py, flows.py, users.py, etc.)
Files:
src/backend/base/langflow/api/v2/mcp.py
|
It was fixed in 1.7.0, so Close |
Description
This PR fixes an issue where updating an MCP server configuration would unintentionally clear existing fields like
envandargsif they were not included in the request payload.Reasoning
The endpoint
@router.patch("/servers/{server_name}")implies a PATCH operation (partial update). However, the previous implementation performed a full replacement of the configuration object.This change modifies
update_serverinsrc/backend/base/langflow/api/v2/mcp.pyto merge the new configuration with the existing one, preserving critical fields likeenvandargswhen they are not explicitly provided in the update. This aligns the backend behavior with proper PATCH semantics.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.