-
Notifications
You must be signed in to change notification settings - Fork 216
feat(sdk): project-level .mcp.json discovery and merge #2772
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,11 +42,13 @@ | |
| from openhands.sdk.llm.llm_registry import LLMRegistry | ||
| from openhands.sdk.logger import get_logger | ||
| from openhands.sdk.observability.laminar import observe | ||
| from openhands.sdk.mcp.project_config import try_load_project_mcp_config | ||
| from openhands.sdk.plugin import ( | ||
| Plugin, | ||
| PluginSource, | ||
| ResolvedPluginSource, | ||
| fetch_plugin_with_resolution, | ||
| merge_mcp_configs, | ||
| ) | ||
| from openhands.sdk.security.analyzer import SecurityAnalyzerBase | ||
| from openhands.sdk.security.confirmation_policy import ( | ||
|
|
@@ -78,6 +80,7 @@ class LocalConversation(BaseConversation): | |
| _cleanup_initiated: bool | ||
| _hook_processor: HookEventProcessor | None | ||
| delete_on_close: bool = True | ||
| _trust_project_mcp: bool | ||
| # Plugin lazy loading state | ||
| _plugin_specs: list[PluginSource] | None | ||
| _resolved_plugins: list[ResolvedPluginSource] | None | ||
|
|
@@ -106,6 +109,7 @@ def __init__( | |
| delete_on_close: bool = True, | ||
| cipher: Cipher | None = None, | ||
| tags: dict[str, str] | None = None, | ||
| trust_project_mcp: bool = False, | ||
| **_: object, | ||
| ): | ||
| """Initialize the conversation. | ||
|
|
@@ -147,6 +151,10 @@ def __init__( | |
| (lost) on serialization. | ||
| tags: Optional key-value tags for the conversation. Keys must be | ||
| lowercase alphanumeric, values up to 256 characters. | ||
| trust_project_mcp: When True, load ``.openhands/.mcp.json`` or root | ||
| ``.mcp.json`` from the workspace and merge under user/agent MCP | ||
| settings. UIs should set this only after the user approves | ||
| project-scoped servers. | ||
| """ | ||
| super().__init__() # Initialize with span tracking | ||
| # Mark cleanup as initiated as early as possible to avoid races or partially | ||
|
|
@@ -160,6 +168,7 @@ def __init__( | |
| self._plugins_loaded = False | ||
| self._pending_hook_config = hook_config # Will be combined with plugin hooks | ||
| self._agent_ready = False # Agent initialized lazily after plugins loaded | ||
| self._trust_project_mcp = trust_project_mcp | ||
|
|
||
| self.agent = agent | ||
| if isinstance(workspace, (str, Path)): | ||
|
|
@@ -328,14 +337,20 @@ def _ensure_plugins_loaded(self) -> None: | |
| all_plugin_hooks: list[HookConfig] = [] | ||
| all_plugin_agents: list[AgentDefinition] = [] | ||
|
|
||
| project_dir = Path(self.workspace.working_dir) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _ensure_plugins_loaded is about loading plugins, discovering and merging project-level MCP config is a separate concern that happens to run at the same time. Can you move that into another method ? Something like def _load_project_mcp_config(self) -> dict[str, Any] | None:
"""Load project .mcp.json if trusted, return merged config or None."""
if not self._trust_project_mcp:
return None
project_dir = Path(self.workspace.working_dir)
project_mcp = try_load_project_mcp_config(project_dir)
if project_mcp is None:
return None
merged = merge_mcp_configs(project_mcp, self.agent.mcp_config)
if merged != self.agent.mcp_config:
self.agent = self.agent.model_copy(update={"mcp_config": merged})
with self._state:
self._state.agent = self.agent
return mergedand then you can call it before _ensure_plugins_loaded() in the _ensure_agent_ready method |
||
| project_mcp = ( | ||
| try_load_project_mcp_config(project_dir) | ||
| if self._trust_project_mcp | ||
| else None | ||
| ) | ||
| merged_mcp = merge_mcp_configs(project_mcp, self.agent.mcp_config) | ||
|
|
||
| # Load plugins if specified | ||
| if self._plugin_specs: | ||
| logger.info(f"Loading {len(self._plugin_specs)} plugin(s)...") | ||
| self._resolved_plugins = [] | ||
|
|
||
| # Start with agent's existing context and MCP config | ||
| merged_context = self.agent.agent_context | ||
| merged_mcp = dict(self.agent.mcp_config) if self.agent.mcp_config else {} | ||
|
|
||
| for spec in self._plugin_specs: | ||
| # Fetch plugin and get resolved commit SHA | ||
|
|
@@ -382,6 +397,11 @@ def _ensure_plugins_loaded(self) -> None: | |
|
|
||
| logger.info(f"Loaded {len(self._plugin_specs)} plugin(s) via Conversation") | ||
|
|
||
| elif merged_mcp != self.agent.mcp_config: | ||
| self.agent = self.agent.model_copy(update={"mcp_config": merged_mcp}) | ||
| with self._state: | ||
| self._state.agent = self.agent | ||
|
|
||
| # Register file-based agents defined in plugins | ||
| if all_plugin_agents: | ||
| register_plugin_agents( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||
| """Project-level .mcp.json discovery and loading.""" | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't need this import. |
||||||
|
|
||||||
| from pathlib import Path | ||||||
| from typing import Any | ||||||
|
|
||||||
| from openhands.sdk.context.skills.exceptions import SkillValidationError | ||||||
| from openhands.sdk.context.skills.utils import load_mcp_config | ||||||
| from openhands.sdk.logger import get_logger | ||||||
|
|
||||||
| logger = get_logger(__name__) | ||||||
|
|
||||||
|
|
||||||
| def find_project_mcp_json(project_dir: Path) -> Path | None: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. private method? |
||||||
| """Return the first project MCP config path if present. | ||||||
|
|
||||||
| Preference order: ``.openhands/.mcp.json``, then root ``.mcp.json``. | ||||||
| """ | ||||||
| for candidate in ( | ||||||
| project_dir / ".openhands" / ".mcp.json", | ||||||
| project_dir / ".mcp.json", | ||||||
|
Comment on lines
+21
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be constant at the top of the file. _PROJECT_MCP_CANDIDATES: Final[tuple[str]] = (".openhands/.mcp.json", ".mcp.json") |
||||||
| ): | ||||||
| if candidate.is_file(): | ||||||
| return candidate | ||||||
| return None | ||||||
|
|
||||||
|
|
||||||
| def try_load_project_mcp_config(project_dir: Path) -> dict[str, Any] | None: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
just make clear in the docstring that it returns None if there is a problem |
||||||
| """Load and validate project ``.mcp.json``, or return None if missing or invalid.""" | ||||||
| path = find_project_mcp_json(project_dir) | ||||||
| if path is None: | ||||||
| return None | ||||||
| try: | ||||||
| return load_mcp_config(path, skill_root=project_dir) | ||||||
| except SkillValidationError as e: | ||||||
| logger.warning("Ignoring invalid project MCP config at %s: %s", path, e) | ||||||
| return None | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,32 @@ | |
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| def merge_mcp_configs( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this method in another place. It is a general MCP utility. Perhaps something like |
||
| base: dict[str, Any] | None, | ||
| overlay: dict[str, Any] | None, | ||
| ) -> dict[str, Any]: | ||
| """Merge two MCP config dicts; overlay wins on key conflicts.""" | ||
| if base is None and overlay is None: | ||
| return {} | ||
| if base is None: | ||
| return dict(overlay) if overlay is not None else {} | ||
| if overlay is None: | ||
| return dict(base) | ||
|
|
||
|
Comment on lines
+39
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can write that with a match (base, overlay):
case (None, None):
return {}
case (None, _):
return dict(overlay)
case (_, None):
return dict(base) |
||
| result = dict(base) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things:
def merge_mcp_configs(
base: dict[str, Any] | None,
overlay: dict[str, Any] | None,
) -> dict[str, Any]:
"""Merge two MCP config dicts; overlay wins on key conflicts."""
match (base, overlay):
case (None, None):
return {}
case (None, _):
return dict(overlay)
case (_, None):
return dict(base)
result = {**base, **overlay}
if "mcpServers" in base and "mcpServers" in overlay:
result["mcpServers"] = {**base["mcpServers"], **overlay["mcpServers"]}
return resultThe {**base, **overlay} handles all top-level keys (overlay wins). Then the mcpServers fix-up deep-merges server entries instead of letting overlay completely replace the dict. If only one side has mcpServers, the spread already did the right thing. |
||
| if "mcpServers" in overlay: | ||
| existing_servers = result.get("mcpServers", {}) | ||
| result["mcpServers"] = { | ||
| **existing_servers, | ||
| **overlay["mcpServers"], | ||
| } | ||
| for key, value in overlay.items(): | ||
| if key != "mcpServers": | ||
| result[key] = value | ||
| return result | ||
|
|
||
|
|
||
| # Directories to check for plugin manifest | ||
| PLUGIN_MANIFEST_DIRS = [".plugin", ".claude-plugin"] | ||
| PLUGIN_MANIFEST_FILE = "plugin.json" | ||
|
|
@@ -201,36 +227,24 @@ def add_mcp_config_to( | |
| if base_config is None and plugin_config is None: | ||
| return {} | ||
| if base_config is None: | ||
| return dict(plugin_config) if plugin_config else {} | ||
| return dict(plugin_config) if plugin_config is not None else {} | ||
| if plugin_config is None: | ||
| return dict(base_config) | ||
|
|
||
| # Shallow copy to avoid mutating inputs | ||
| result = dict(base_config) | ||
|
|
||
| # Merge mcpServers by server name (Claude Code compatible behavior) | ||
| if "mcpServers" in plugin_config: | ||
| existing_servers = result.get("mcpServers", {}) | ||
| existing_servers = base_config.get("mcpServers", {}) | ||
| for server_name in plugin_config["mcpServers"]: | ||
| if server_name in existing_servers: | ||
| logger.warning( | ||
| f"Plugin MCP server '{server_name}' overrides existing server" | ||
| ) | ||
| result["mcpServers"] = { | ||
| **existing_servers, | ||
| **plugin_config["mcpServers"], | ||
| } | ||
|
|
||
| # Other top-level keys: plugin wins (shallow override) | ||
| for key, value in plugin_config.items(): | ||
| if key != "mcpServers": | ||
| if key in result: | ||
| logger.warning( | ||
| f"Plugin MCP config key '{key}' overrides existing value" | ||
| ) | ||
| result[key] = value | ||
| if key != "mcpServers" and key in base_config: | ||
| logger.warning( | ||
| f"Plugin MCP config key '{key}' overrides existing value" | ||
| ) | ||
|
|
||
| return result | ||
| return merge_mcp_configs(base_config, plugin_config) | ||
|
|
||
| @classmethod | ||
| def fetch( | ||
|
|
||
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.