Conversation
WalkthroughIntroduces transport-agnostic MCP tool and toolset abstractions, adds stdio transport and StdioMCPTool/Toolset, refactors RemoteMCPTool/Toolset to share BaseMCPTool/BaseMCPToolset, adds a config-driven factory, updates tests, and expands docs to cover SSE and stdio configurations and examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as Holmes (loader)
participant Factory as get_mcp_toolset_from_config
participant Toolset as BaseMCPToolset
participant Client as Client (sse_client / stdio_client)
participant Server as MCP Server
Loader->>Factory: get_mcp_toolset_from_config(config, name)
Factory-->>Loader: RemoteMCPToolset or StdioMCPToolset
Loader->>Toolset: init_server_tools()
Toolset->>Client: connect/discover tools
Client->>Server: list_tools()
Server-->>Client: tools[]
Client-->>Toolset: tools[]
Loader->>Toolset: invoke(tool_id, params)
Toolset->>Client: call_tool(tool_id, params)
Client->>Server: execute
Server-->>Client: result
Client-->>Toolset: StructuredToolResult
Toolset-->>Loader: StructuredToolResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential hotspots to review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
c4ac0ec to
29bfb2a
Compare
fec417d to
31fa18a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
268-290: Add type validation for stdio transport configuration.The function should validate that required fields are present for stdio transport before creating the toolset.
if mcp_type == "sse": return RemoteMCPToolset(**config, name=name) if mcp_type == "stdio": + # Validate required fields for stdio transport + if not mcp_config.get("command") and not config.get("command"): + raise ValueError("stdio transport requires 'command' field in configuration") return StdioMCPToolset(**config, name=name)
🧹 Nitpick comments (10)
docs/data-sources/remote-mcp-servers.md (2)
22-22: Fix markdown heading style inconsistency.The heading uses setext style (underlines) instead of atx style (hashes) which is inconsistent with the rest of the document.
-Transport selection and supported fields --------------------------------------- +### Transport selection and supported fields
54-54: Fix markdown heading style inconsistency.The heading uses setext style instead of atx style.
-Security notes --------------- +### Security notesholmes/plugins/toolsets/mcp/toolset_mcp.py (4)
1-290: Code formatting required.The pipeline indicates that ruff-format needs to be run on this file to fix formatting issues.
Run the following command to fix formatting:
#!/bin/bash # Format the file using ruff ruff format holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Remove unused parameteruser_approved.The
user_approvedparameter is never used in this method implementation.- def _invoke( - self, params: Dict, user_approved: bool = False - ) -> StructuredToolResult: + def _invoke(self, params: Dict) -> StructuredToolResult:Note: This would need to be coordinated with the base
Toolclass interface if it expects this parameter.
95-95: Use explicit string conversion flags for better readability.Replace string concatenation with f-string conversion flags.
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"Also applies to: 136-136
150-164: Improve exception handling specificity.The code catches all exceptions broadly and converts them to strings using
e.args, which may not always provide useful error messages.def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: try: tools_result = asyncio.run(self._get_server_tools()) self.tools = self._create_tools(tools_result.tools) if not self.tools: logging.warning( f"{self._get_server_type()} mcp server {self.name} loaded 0 tools." ) return (True, "") - except Exception as e: + except (ConnectionError, TimeoutError) as e: + return ( + False, + f"Failed to connect to {self._get_server_type()} mcp server {self.name} {self._get_connection_info()}: {str(e)}", + ) + except Exception as e: return ( False, - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()}: {str(e)}", )tests/test_mcp_toolset.py (4)
1-208: Code formatting required.The pipeline indicates that ruff-format needs to be run on this file.
Run the following command to fix formatting:
#!/bin/bash # Format the test file using ruff ruff format tests/test_mcp_toolset.py
57-57: Use Pythonic false check instead of explicit comparison.- assert result[0] == False + assert not result[0]
84-86: Fix inconsistent formatting of monkeypatch.setattr call.- monkeypatch.setattr(mcp_toolset, "_get_server_tools", - mock_get_server_tools) + monkeypatch.setattr(mcp_toolset, "_get_server_tools", mock_get_server_tools)
90-99: Remove unused monkeypatch parameter.The
monkeypatchparameter is not used in this test function.-def test_mcpserver_headers(monkeypatch): +def test_mcpserver_headers():
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/data-sources/remote-mcp-servers.md(3 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)tests/test_mcp_toolset.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
In MkDocs content, always add a blank line between a header/bold text and a list to render lists properly
Files:
docs/data-sources/remote-mcp-servers.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/__init__.pytests/test_mcp_toolset.pyholmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/mcp/toolset_mcp.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/test_mcp_toolset.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/test_mcp_toolset.py
🧠 Learnings (3)
📚 Learning: 2025-09-15T07:09:16.052Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T07:09:16.052Z
Learning: Applies to {holmes/plugins/toolsets/**/*.yaml,tests/llm/**/toolsets.yaml} : Valid top-level fields for toolset YAMLs are limited to: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Applied to files:
docs/data-sources/remote-mcp-servers.md
📚 Learning: 2025-08-05T00:42:23.792Z
Learnt from: vishiy
PR: robusta-dev/holmesgpt#782
File: config.example.yaml:31-49
Timestamp: 2025-08-05T00:42:23.792Z
Learning: In robusta-dev/holmesgpt config.example.yaml, the azuremonitorlogs toolset configuration shows "enabled: true" as an example of how to enable the toolset, not as a default setting. The toolset is disabled by default and requires explicit enablement in user configurations.
Applied to files:
docs/data-sources/remote-mcp-servers.md
📚 Learning: 2025-09-15T07:09:16.052Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T07:09:16.052Z
Learning: Applies to holmes/plugins/toolsets/** : Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Applied to files:
docs/data-sources/remote-mcp-servers.md
🧬 Code graph analysis (3)
holmes/plugins/toolsets/__init__.py (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
get_mcp_toolset_from_config(268-289)
tests/test_mcp_toolset.py (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (7)
RemoteMCPToolset(183-223)RemoteMCPTool(59-95)StdioMCPToolset(226-265)get_mcp_toolset_from_config(268-289)init_server_tools(150-164)headers(200-201)url(204-205)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (11)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)ToolParameter(154-159)StructuredToolResultStatus(51-75)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)
🪛 markdownlint-cli2 (0.17.2)
docs/data-sources/remote-mcp-servers.md
22-22: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
54-54: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
🪛 Ruff (0.12.2)
tests/test_mcp_toolset.py
57-57: Avoid equality comparisons to False; use not result[0]: for false checks
Replace with not result[0]
(E712)
90-90: Unused function argument: monkeypatch
(ARG001)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
150-150: Unused method argument: config
(ARG002)
159-159: Consider moving this statement to an else block
(TRY300)
160-160: Do not catch blind exception: Exception
(BLE001)
163-163: Use explicit conversion flag
Replace with conversion flag
(RUF010)
270-270: Avoid specifying long messages outside the exception class
(TRY003)
284-286: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Build and test HolmesGPT
tests/test_mcp_toolset.py
[error] 57-57: Ruff E712: Avoid equality comparisons to False; use if not result[0]: for false checks.
[error] 1-1: Ruff-format reformatted tests/test_mcp_toolset.py.
holmes/plugins/toolsets/mcp/toolset_mcp.py
[error] 1-1: Ruff-format reformatted holmes/plugins/toolsets/mcp/toolset_mcp.py.
[error] 183-183: Mypy: Name 'url' already defined on line 183 [no-redef].
[error] 232-232: Mypy: Argument 1 to 'dict' has incompatible type 'Any | None'; expected 'SupportsKeysAndGetItem[Any, Any]' [arg-type].
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: llm_evals
🔇 Additional comments (5)
docs/data-sources/remote-mcp-servers.md (1)
238-255: LGTM! Clear documentation for stdio transport configuration.The example effectively demonstrates how to configure Holmes to run a stdio-based MCP process directly without needing Supergateway, including all necessary fields like command, args, and environment variables.
holmes/plugins/toolsets/mcp/toolset_mcp.py (2)
23-57: LGTM! Well-structured base class abstraction.The
BaseMCPToolclass provides a clean abstraction for both SSE and stdio transports with proper async handling and shared input schema parsing logic.
98-137: LGTM! Clean stdio implementation following the same pattern as RemoteMCPTool.The
StdioMCPToolclass properly implements the stdio transport with appropriate error handling and result formatting.holmes/plugins/toolsets/__init__.py (1)
40-40: LGTM! Clean factory pattern implementation.The change from direct
RemoteMCPToolsetinstantiation to usingget_mcp_toolset_from_configfactory function is a good design pattern that supports multiple transport types while maintaining backward compatibility.Also applies to: 179-179
tests/test_mcp_toolset.py (1)
124-183: LGTM! Comprehensive test coverage for the factory function.The tests thoroughly cover all scenarios including:
- Empty config validation
- SSE and stdio transport types
- Backward compatibility with top-level url
- Edge cases with missing fields
e8b240e to
4be1689
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
69-81: Return detailed error info and handle no‑data responses from remote MCP calls.Aligns with holmes plugins guideline to surface API errors and what was invoked.
Apply this diff:
- return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - data=merged_text, - params=params, - invocation=f"MCPtool {self.name} with params {params}", - ) + is_error = bool(getattr(tool_result, "isError", False)) + has_text = bool(merged_text) + status = ( + StructuredToolResultStatus.ERROR + if is_error + else (StructuredToolResultStatus.NO_DATA if not has_text else StructuredToolResultStatus.SUCCESS) + ) + return StructuredToolResult( + status=status, + error=merged_text if is_error else (f"No data returned from tool {self.name} at {self.url}" if not has_text else None), + data=None if is_error else merged_text, + params=params, + invocation=f"Remote MCP {self.url} tool {self.name} with params {params!s}", + )
🧹 Nitpick comments (4)
tests/test_mcp_toolset.py (1)
56-58: Fix Ruff E712: avoid equality comparison to False.Use
is Falseornot result[0]to satisfy the linter and improve readability.Apply this diff:
- assert result[0] == False + assert result[0] is Falseholmes/plugins/toolsets/mcp/toolset_mcp.py (3)
107-119: Mirror error/no‑data handling for stdio transport.Apply this diff:
- return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - data=merged_text, - params=params, - invocation=f"Stdio MCP tool {self.name} with params {params}", - ) + is_error = bool(getattr(tool_result, "isError", False)) + has_text = bool(merged_text) + status = ( + StructuredToolResultStatus.ERROR + if is_error + else (StructuredToolResultStatus.NO_DATA if not has_text else StructuredToolResultStatus.SUCCESS) + ) + return StructuredToolResult( + status=status, + error=merged_text if is_error else (f"No data returned from stdio tool {self.name} ({self.server_params.command})" if not has_text else None), + data=None if is_error else merged_text, + params=params, + invocation=f"Stdio MCP ({self.server_params.command}) tool {self.name} with params {params!s}", + )
94-95: Use explicit f‑string conversion flags and avoidstr()inside f‑strings.Satisfies Ruff RUF010, and improves clarity.
Apply this diff:
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}" @@ - return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}" @@ - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {e.args!s}",Also applies to: 136-136, 162-162
26-37: Silence unused-arg warnings (ARG002) without changing behavior.Keep signatures for framework compatibility but mark params as intentionally unused.
Apply this diff:
def _invoke( - self, params: Dict, user_approved: bool = False + self, params: Dict, user_approved: bool = False ) -> StructuredToolResult: + _ = user_approved # unused but required by interface try: return asyncio.run(self._invoke_async(params)) except Exception as e: return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=str(e.args), + error=f"{e.__class__.__name__}: {e}", params=params, - invocation=f"{self.__class__.__name__} {self.name} with params {params}", + invocation=f"{self.__class__.__name__} {self.name} with params {params!s}", ) @@ - def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + _ = config # unused; signature required by CallablePrerequisite try: tools_result = asyncio.run(self._get_server_tools())Also applies to: 149-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)tests/test_mcp_toolset.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
tests/test_mcp_toolset.pyholmes/plugins/toolsets/mcp/toolset_mcp.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/test_mcp_toolset.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/test_mcp_toolset.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (2)
tests/test_mcp_toolset.py (2)
holmes/core/tools.py (2)
ToolParameter(154-159)Tool(162-353)holmes/plugins/toolsets/mcp/toolset_mcp.py (7)
RemoteMCPToolset(182-220)RemoteMCPTool(59-95)StdioMCPToolset(223-263)get_mcp_toolset_from_config(266-287)init_server_tools(149-163)headers(197-198)url(201-202)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (14)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)ToolParameter(154-159)StructuredToolResultStatus(51-75)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)get_example_config(727-728)get_example_config(746-747)get_example_config(779-780)
🪛 Ruff (0.12.2)
tests/test_mcp_toolset.py
57-57: Avoid equality comparisons to False; use not result[0]: for false checks
Replace with not result[0]
(E712)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
149-149: Unused method argument: config
(ARG002)
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
282-284: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Build and test HolmesGPT
tests/test_mcp_toolset.py
[error] 57-57: E712 Avoid equality comparisons to False; use not result[0] for false checks.
holmes/plugins/toolsets/mcp/toolset_mcp.py
[warning] 1-1: pre-commit: Removed unused import 'AnyUrl' from 'from pydantic import AnyUrl, Field, field_validator' to 'from pydantic import Field, field_validator' (lint cleanup).
🪛 GitHub Actions: Evaluate LLM test cases
holmes/plugins/toolsets/mcp/toolset_mcp.py
[error] 182-182: PydanticUserError: Decorators defined with incorrect fields: holmes.plugins.toolsets.mcp.toolset_mcp.RemoteMCPToolset:94303699898320.append_sse_if_missing (use check_fields=False if you're inheriting from the model and intended this). Step: poetry run pytest --no-cov tests/llm/test_ask_holmes.py tests/llm/test_investigate.py -s -n10 -m 'llm and easy'
🔇 Additional comments (4)
tests/test_mcp_toolset.py (2)
130-138: Tests assume SSE URL normalization happens in config; factory must write back normalized URL.These assertions will only pass if
get_mcp_toolset_from_confignormalizes/sseand updatestoolset.config["url"](not just a computed property). Ensure the factory mutates/setsconfig["config"]["url"]to the normalized value and removes any top‑levelurl. See suggested fixes in toolset_mcp.py.Also applies to: 153-162
185-201: Authorization header “normalization” expectation is unsupported — no sanitizer found.No code maps "***" → "Bearer token". Either implement a sanitizer/validator to perform this normalization (e.g., in get_mcp_toolset_from_config or RemoteMCPToolset) or change the test to assert the literal input or pass a real token.
Locations: tests/test_mcp_toolset.py (lines ~185–201); holmes/plugins/toolsets/mcp/toolset_mcp.py (headers property).
holmes/plugins/toolsets/mcp/toolset_mcp.py (2)
26-31: Note: asyncio.run in library code can fail inside a running loop.If these paths execute under an active event loop (e.g., async runtimes),
asyncio.runwill raise. Consider an event‑loop aware runner (detect running loop and useasyncio.create_task/awaitvia a bridge) or document the sync‑only contract.Also applies to: 149-163
1-11: Import cleanup — remove AnyUrl; keep field_validatorfield_validator is used at holmes/plugins/toolsets/mcp/toolset_mcp.py:185 (@field_validator("url", mode="before")); remove AnyUrl from the pydantic import on line 11 and keep field_validator.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
holmes/plugins/toolsets/mcp/toolset_mcp.py (3)
185-190: Fix PydanticUserError: validator targets a non-fieldThe
@field_validator("url")will fail withPydanticUserErrorbecauseurlis a property (lines 200-202), not a model field. Remove this validator and implement normalization directly in theurlproperty getter and in the factory function.Apply this diff to fix the issue:
-from pydantic import Field, field_validator +from pydantic import Fieldclass RemoteMCPToolset(BaseMCPToolset): tools: List[RemoteMCPTool] = Field(default_factory=list) # type: ignore - @field_validator("url", mode="before") - def append_sse_if_missing(cls, v): - if isinstance(v, str) and not v.rstrip("/").endswith("/sse"): - v = v.rstrip("/") + "/sse" - return v - def _create_tools(self, tools: List[MCP_Tool]) -> List[Tool]:
200-202: Normalize SSE URL on property accessThe
urlproperty should ensure the/ssesuffix is always present when accessed, supporting both factory-based and direct constructor usage.Apply this diff to normalize the URL:
@property def url(self) -> Optional[str]: - return self.config and self.config.get("url") + raw = self.config and self.config.get("url") + if isinstance(raw, str): + u = raw.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" + return raw
266-287: Fix backward compatibility and URL normalization in factoryThe current implementation has several issues:
- Top-level
urlcreates an invalid model with extra field, not nested inconfig- URL normalization via the field validator won't work (see line 185 issue)
- Missing support for top-level
command/argsfor stdioApply this comprehensive fix:
def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: if not config: raise ValueError("Config must not be empty") - # Prefer explicit mcp type set in nested 'config' for clarity. - # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). - mcp_config = config.get("config", {}) - mcp_type = mcp_config.get("type") - - if mcp_type == "sse": - return RemoteMCPToolset(**config, name=name) - if mcp_type == "stdio": - return StdioMCPToolset(**config, name=name) - - # Backwards compatibility: still support using url or command top-level keys. - url = config.get("url") - if not url: - raise ValueError( - "MCP Server config must include 'config.type' to specify the transport type." - ) - # fill the mcp server config with URL in case it's not set. - mcp_config["url"] = url - return RemoteMCPToolset(**config, name=name) + + # Copy and normalize config + cfg = dict(config) + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") + + def _normalize_sse(u: str) -> str: + u = u.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" + + if mcp_type == "sse": + if isinstance(mcp_config.get("url"), str): + mcp_config["url"] = _normalize_sse(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if mcp_type == "stdio": + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + # Backwards compatibility: accept top-level url/command and migrate into nested config + if "url" in cfg: + mcp_config.setdefault("type", "sse") + mcp_config["url"] = _normalize_sse(str(cfg.pop("url"))) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if "command" in cfg: + mcp_config.setdefault("type", "stdio") + mcp_config["command"] = str(cfg.pop("command")) + mcp_config["args"] = cfg.pop("args", []) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + raise ValueError("MCP Server config must include 'config.type' to specify the transport type.")
🧹 Nitpick comments (5)
holmes/plugins/toolsets/mcp/toolset_mcp.py (5)
253-254: Remove redundant parameter constructionThe method creates a new
StdioServerParametersinstance fromself.commandandself.args, but these properties already derive fromself.stdio_server_params. Use the existingself.stdio_server_paramsdirectly.Apply this diff to remove redundancy:
async def _get_server_tools(self): - server_params = StdioServerParameters(command=self.command, args=self.args) - async with stdio_client(server_params) as ( + async with stdio_client(self.stdio_server_params) as ( read_stream, write_stream, ):
36-37: Use f-string conversion flag for cleaner formattingApply this diff:
- invocation=f"{self.__class__.__name__} {self.name} with params {params}", + invocation=f"{self.__class__.__name__} {self.name} with params {params!s}",
94-95: Use f-string conversion flagApply this diff:
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"
135-136: Use f-string conversion flagApply this diff:
- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"
162-162: Use f-string conversion flagApply this diff:
- f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {e.args!s}",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (14)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)ToolParameter(154-159)StructuredToolResultStatus(51-75)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)get_example_config(727-728)get_example_config(746-747)get_example_config(779-780)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
149-149: Unused method argument: config
(ARG002)
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
282-284: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
231-236: LGTM with early config checkThe method correctly constructs
StdioServerParametersfrom config. The early validation prevents aTypeErrorwhenself.configis None.
6e6b195 to
49a0e98
Compare
49a0e98 to
69fa445
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
264-285: Fix backward compatibility and normalize URLs in config.The backward compatibility path has issues:
- When
urlis at top-level, it's not moved intoconfig, causing "extra inputs" validation error- URL normalization isn't consistent across all paths
- The function mutates the input config dict which can cause unexpected side effects
Apply this diff to fix the issues:
def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: if not config: raise ValueError("Config must not be empty") + + # Make a copy to avoid mutating the input + cfg = dict(config) + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") - # Prefer explicit mcp type set in nested 'config' for clarity. - # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). - mcp_config = config.get("config", {}) - mcp_type = mcp_config.get("type") + def _normalize_sse(u: str) -> str: + u = u.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" if mcp_type == "sse": - return RemoteMCPToolset(**config, name=name) + if "url" in mcp_config and isinstance(mcp_config["url"], str): + mcp_config["url"] = _normalize_sse(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + if mcp_type == "stdio": - return StdioMCPToolset(**config, name=name) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) # Backwards compatibility: still support using url or command top-level keys. - url = config.get("url") - if not url: - raise ValueError( - "MCP Server config must include 'config.type' to specify the transport type." - ) - # fill the mcp server config with URL in case it's not set. - mcp_config["url"] = url - return RemoteMCPToolset(**config, name=name) + if "url" in cfg: + mcp_config["type"] = "sse" + mcp_config["url"] = _normalize_sse(str(cfg.pop("url"))) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if "command" in cfg: + mcp_config["type"] = "stdio" + mcp_config["command"] = str(cfg.pop("command")) + mcp_config["args"] = cfg.pop("args", []) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + raise ValueError( + "MCP Server config must include 'config.type' to specify the transport type." + )
🧹 Nitpick comments (8)
holmes/plugins/toolsets/mcp/toolset_mcp.py (6)
27-27: Consider usinguser_approvedparameter or document as future-ready.The
user_approvedparameter is currently unused. Either utilize it for authorization checks (e.g., some MCP operations might require approval), document it as reserved for future use, or remove it to align with the abstract base class signature.def _invoke( - self, params: Dict, user_approved: bool = False + self, params: Dict, user_approved: bool = False # Reserved for future authorization checks ) -> StructuredToolResult:
95-95: Use f-string instead of string concatenation.Static analysis suggests using an f-string for better readability.
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"
136-136: Use f-string conversion flag.Consider using conversion flag for string formatting.
- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"
149-149: Remove unused parameter or document as required by interface.The
configparameter is unused ininit_server_tools. Either use it, document it as required by theCallablePrerequisiteinterface, or remove it.-def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: +def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + # config parameter is required by CallablePrerequisite interface but not used here try:
159-162: Use specific exception types and handle errors gracefully.The code catches a broad
Exceptionand uses problematic string conversion withe.args. Consider catching specific exceptions and usingstr(e)for better error messages.- except Exception as e: + except (ConnectionError, TimeoutError, ValueError) as e: return ( False, - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()}: {e!s}", ) + except Exception as e: + return ( + False, + f"Unexpected error loading {self._get_server_type()} mcp server {self.name}: {e!s}", + )
251-252: Remove redundant server parameters construction.You're constructing
StdioServerParameterstwice - once instdio_server_paramsproperty and again here. Use the property directly.async def _get_server_tools(self): - server_params = StdioServerParameters(command=self.command, args=self.args) - async with stdio_client(server_params) as ( + async with stdio_client(self.stdio_server_params) as ( read_stream,tests/test_mcp_toolset.py (2)
133-137: Ensure URL normalization works correctly in tests.The test verifies that the URL in config remains unchanged when it already ends with
/sse, but doesn't check the normalizedtoolset.urlproperty. Add an assertion to verify URL normalization.assert isinstance(toolset, RemoteMCPToolset) assert toolset.name == "test_sse" assert str(toolset.config.get("url")) == "http://example.com/sse" + assert str(toolset.url) == "http://example.com/sse" # Verify property normalization
149-150: Add property verification for stdio toolset.The test verifies config values but doesn't check the exposed properties (
command,args).assert toolset.config.get("command") == "python" assert toolset.config.get("args") == ["/path/to/server.py"] + # Verify exposed properties + assert toolset.command == "python" + assert toolset.args == ["/path/to/server.py"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)tests/test_mcp_toolset.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/test_mcp_toolset.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/test_mcp_toolset.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (14)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)ToolParameter(154-159)StructuredToolResultStatus(51-75)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)get_example_config(727-728)get_example_config(746-747)get_example_config(779-780)
tests/test_mcp_toolset.py (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (7)
RemoteMCPToolset(182-218)RemoteMCPTool(59-95)StdioMCPToolset(221-261)get_mcp_toolset_from_config(264-285)init_server_tools(149-163)headers(191-192)url(195-200)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
149-149: Unused method argument: config
(ARG002)
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
280-282: Avoid specifying long messages outside the exception class
(TRY003)
🪛 GitHub Actions: Build and test HolmesGPT
tests/test_mcp_toolset.py
[error] 156-156: RemoteMCPToolset.url: Extra inputs are not permitted. ValidationError loading config (backward compatibility with URL).
[error] 167-167: RemoteMCPToolset.url: Extra inputs are not permitted. ValidationError loading config (URL with SSE).
[error] 200-200: AssertionError: toolset.config['headers']['Authorization'] == '***' (test_mcp_toolset.py:200).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
268-290: Fix backward compatibility and validate URL normalization in factory.The current implementation has several issues:
- It mutates the input config dict with
config.pop("url", None)- The backward compatibility path doesn't handle SSE URL normalization consistently
- The logic flow is confusing with multiple conditional branches
Apply this refactored implementation:
def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: if not config: raise ValueError("Config must not be empty") + + # Make a copy to avoid mutating the input + cfg = dict(config) + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") + + # Helper function for SSE URL normalization + def normalize_sse_url(url: str) -> str: + url = url.rstrip("/") + return url if url.endswith("/sse") else f"{url}/sse" + + if mcp_type == "stdio": + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + if mcp_type == "sse": + # Normalize URL if present + if "url" in mcp_config: + mcp_config["url"] = normalize_sse_url(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + # Backwards compatibility: still support using url or command top-level keys. - url = config.pop("url", None) - if mcp_type == "sse": - return RemoteMCPToolset(**config, name=name) - if url is not None: - # fill the mcp server config with URL in case the mcp config is not set - if mcp_config == {}: - config["config"] = {"url": url} - else: - mcp_config["url"] = url - return RemoteMCPToolset(**config, name=name) + if "url" in cfg: + # Move top-level url to nested config and normalize + mcp_config["type"] = "sse" + mcp_config["url"] = normalize_sse_url(cfg.pop("url")) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) raise ValueError( "MCP Server config must include a transport type ('sse' or 'stdio') " "either in 'config.type' or by providing transport-specific keys." )
🧹 Nitpick comments (7)
holmes/plugins/toolsets/mcp/toolset_mcp.py (6)
27-27: Unused argumentuser_approvedin_invoke.The
user_approvedparameter is never used in the base implementation. Consider whether this parameter should be passed to_invoke_asyncfor subclasses to utilize, or if it should be removed from the base class signature.If the parameter is intended for future use by subclasses, consider passing it to
_invoke_async:- return asyncio.run(self._invoke_async(params)) + return asyncio.run(self._invoke_async(params, user_approved=user_approved))And update the abstract method signature:
-async def _invoke_async(self, params: Dict) -> StructuredToolResult: +async def _invoke_async(self, params: Dict, user_approved: bool = False) -> StructuredToolResult:
95-95: Use explicit string conversion flag for better maintainability.Consider using an f-string conversion flag for explicit string conversion rather than calling
str().- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"
136-136: Use explicit string conversion flag for consistency.- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"
149-149: Unused parameterconfigininit_server_tools.The
configparameter is never used. The method signature is defined by theCallablePrerequisiteinterface which expects a config parameter, but it appears to be unnecessary here.Consider documenting why the parameter exists if it's required by the interface:
def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + # Note: config parameter required by CallablePrerequisite interface but not used try:
159-163: Avoid catching broad exceptions and improve error formatting.The broad
Exceptioncatch could mask unexpected errors. Also, converting exception args to string may not provide the best error message.Consider catching specific exceptions and improving error formatting:
- except Exception as e: - return ( - False, - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", - ) + except Exception as e: + error_msg = str(e) if str(e) else repr(e) + return ( + False, + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()}: {error_msg}", + )
251-251: Redundant server parameters construction.The
stdio_server_paramsproperty already constructs aStdioServerParametersobject. Creating it again here is redundant.- server_params = StdioServerParameters(command=self.command, args=self.args) - async with stdio_client(server_params) as ( + async with stdio_client(self.stdio_server_params) as (tests/test_mcp_toolset.py (1)
57-59: Test passes None toinit_server_toolsbut expects non-None behavior.The test passes
config=Nonetoinit_server_tools, but based on the implementation, this parameter is unused. This could be confusing for maintainers.Consider removing the parameter or passing the actual config:
- result = mcp_toolset.init_server_tools(config=None) + result = mcp_toolset.init_server_tools(mcp_toolset.config)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)tests/test_mcp_toolset.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
tests/test_mcp_toolset.pyholmes/plugins/toolsets/mcp/toolset_mcp.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/test_mcp_toolset.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/test_mcp_toolset.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (2)
tests/test_mcp_toolset.py (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (7)
RemoteMCPToolset(182-218)RemoteMCPTool(59-95)StdioMCPToolset(221-261)get_mcp_toolset_from_config(264-290)init_server_tools(149-163)headers(191-192)url(195-200)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (14)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)ToolParameter(154-159)StructuredToolResultStatus(51-75)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)get_example_config(727-728)get_example_config(746-747)get_example_config(779-780)
🪛 Ruff (0.12.2)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
149-149: Unused method argument: config
(ARG002)
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
287-290: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
tests/test_mcp_toolset.py (1)
202-202: Test expectation doesn't match actual behavior for Authorization header.The test expects the raw value
"***"but there's no header transformation logic in the code that would change this to "Bearer token" as mentioned in past review comments.The test correctly validates that the Authorization header value is preserved as-is without transformation.
|
cc @arikalon1 |
| # Prefer explicit mcp type set in nested 'config' for clarity. | ||
| # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). | ||
| mcp_config = config.get("config", {}) | ||
| mcp_type = mcp_config.get("type") |
There was a problem hiding this comment.
Shouldn't the default be sse?
There was a problem hiding this comment.
If the user specify the url, it's ok but if the user set no type and no url, we'll raise ValidationError instead of
ValueError( "MCP Server config must include a transport type ('sse' or 'stdio') " "either in 'config.type' or by providing transport-specific keys." )
There was a problem hiding this comment.
I can backfill the type to sse when url is set but type is empty
There was a problem hiding this comment.
I think that keep it backward compatible is a good idea. The thing that im not sure is clear from the flow is what will happen if the user did provided type:sse and also url: xxx not under the config field. Looks like it won't work right?
28ef8df to
70d8e2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (2)
26-37: Avoid asyncio.run in library code; fix error formatting and f-string conversionsCalling asyncio.run can raise at runtime if already inside an event loop. Also use str(e) (or e!s) instead of str(e.args), and add explicit conversion flags in f-strings.
- def _invoke( - self, params: Dict, user_approved: bool = False - ) -> StructuredToolResult: - try: - return asyncio.run(self._invoke_async(params)) - except Exception as e: - return StructuredToolResult( - status=StructuredToolResultStatus.ERROR, - error=str(e.args), - params=params, - invocation=f"{self.__class__.__name__} {self.name} with params {params}", - ) + def _invoke( + self, params: Dict, user_approved: bool = False + ) -> StructuredToolResult: + try: + return asyncio.run(self._invoke_async(params)) + except Exception as e: + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR, + error=str(e), + params=params, + invocation=f"{self.__class__.__name__} {self.name} with params {params!s}", + )If this can run under an existing loop, consider a safe adapter (e.g., anyio) or offloading to a thread. Example helper (place near the top, and use it instead of asyncio.run):
# helper outside the diff range from concurrent.futures import ThreadPoolExecutor T = TypeVar("T") def run_coro_sync(coro: Coroutine[Any, Any, T]) -> T: try: asyncio.get_running_loop() except RuntimeError: return asyncio.run(coro) with ThreadPoolExecutor(max_workers=1) as ex: return ex.submit(lambda: asyncio.run(coro)).result()
69-81: Propagate error details from CallToolResultWhen isError is True, set error accordingly so the UI sees the underlying failure.
- return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - data=merged_text, - params=params, - invocation=f"MCPtool {self.name} with params {params}", - ) + is_err = bool(tool_result.isError) + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR if is_err else StructuredToolResultStatus.SUCCESS, + error=merged_text if is_err else None, + data=None if is_err else merged_text, + params=params, + invocation=f"MCP tool {self.name} with params {params!s}", + )
🧹 Nitpick comments (4)
holmes/plugins/toolsets/mcp/toolset_mcp.py (4)
94-96: Use explicit conversion flag in f-string (RUF010)Avoid wrapping with str() inside f-strings.
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"
135-136: Use explicit conversion flag in f-string (RUF010)- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"
146-148: Don’t clobber existing prerequisites; append insteadPreserve any prerequisites defined elsewhere.
- self.prerequisites = [CallablePrerequisite(callable=self.init_server_tools)] + self.prerequisites = list(self.prerequisites or []) + [ + CallablePrerequisite(callable=self.init_server_tools) + ]
149-163: Event loop risk and error formatting in prerequisite loaderasyncio.run may fail under a running loop; also prefer str(e) and conversion flags.
- tools_result = asyncio.run(self._get_server_tools()) + tools_result = asyncio.run(self._get_server_tools()) self.tools = self._create_tools(tools_result.tools) @@ - return ( - False, - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", - ) + return ( + False, + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {e!s}", + )If this can run under an existing loop, use the same thread-offload helper suggested for BaseMCPTool._invoke.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (9)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)StructuredToolResultStatus(51-75)ToolParameter(154-159)Toolset(520-735)_invoke(342-349)_invoke(402-432)model_post_init(181-215)
🪛 Ruff (0.13.1)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
95-95: Use explicit conversion flag
Replace with conversion flag
(RUF010)
136-136: Use explicit conversion flag
Replace with conversion flag
(RUF010)
149-149: Unused method argument: config
(ARG002)
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not catch blind exception: Exception
(BLE001)
162-162: Use explicit conversion flag
Replace with conversion flag
(RUF010)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
266-266: Avoid specifying long messages outside the exception class
(TRY003)
288-291: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (3)
holmes/plugins/toolsets/mcp/toolset_mcp.py (3)
227-235: Good: stdio_server_params validates presence of config and filters typeConfirm StdioServerParameters supports all keys you pass from self.config (env, cwd, etc.). If additional keys are present, they’ll raise a validation error — which is desired.
264-291: Do not mutate caller config; normalize and support top-level command/args; normalize SSE URLThis function pops "url" from the input and doesn’t handle top-level command/args. It should copy, normalize, and write into nested config without side effects.
-def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: - if not config: - raise ValueError("Config must not be empty") - # Prefer explicit mcp type set in nested 'config' for clarity. - # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). - mcp_config = config.get("config", {}) - mcp_type = mcp_config.get("type") - - if mcp_type == "stdio": - return StdioMCPToolset(**config, name=name) - - # Backwards compatibility: still support using url or command top-level keys. - url = config.pop("url", None) - if mcp_type == "sse": - return RemoteMCPToolset(**config, name=name) - if url is not None: - # fill the mcp server config with URL in case the mcp config is not set - if mcp_config == {}: - config["config"] = {"url": url} - else: - mcp_config["url"] = url - mcp_config["type"] = "sse" - return RemoteMCPToolset(**config, name=name) - - raise ValueError( - "MCP Server config must include a transport type ('sse' or 'stdio') " - "either in 'config.type' or by providing transport-specific keys." - ) +def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: + if not config: + raise ValueError("Config must not be empty") + cfg = dict(config) + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") + + def _normalize_sse(u: str) -> str: + u = u.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" + + if mcp_type == "sse": + if isinstance(mcp_config.get("url"), str): + mcp_config["url"] = _normalize_sse(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if mcp_type == "stdio": + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + # Back-compat: migrate top-level url/command/args + if "url" in cfg: + mcp_config.setdefault("type", "sse") + mcp_config["url"] = _normalize_sse(str(cfg.pop("url"))) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if "command" in cfg: + mcp_config.setdefault("type", "stdio") + mcp_config["command"] = str(cfg.pop("command")) + mcp_config["args"] = cfg.pop("args", []) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + raise ValueError( + "MCP Server config must include a transport type ('sse' or 'stdio') either in 'config.type' or by providing transport-specific keys." + )
5-5: No change required: top-level import of StdioServerParameters is valid.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
holmes/plugins/toolsets/mcp/toolset_mcp.py (3)
71-83: Propagate errors from remote CallToolResult; include URL in invocationCurrent code sets status=ERROR but still returns data and no error. Return error on failure and include connection info.
- merged_text = " ".join( - c.text for c in tool_result.content if c.type == "text" - ) - return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - data=merged_text, - params=params, - invocation=f"MCPtool {self.name} with params {params}", - ) + merged_text = " ".join(c.text for c in tool_result.content if c.type == "text") + is_err = bool(tool_result.isError) + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR if is_err else StructuredToolResultStatus.SUCCESS, + error=merged_text if is_err else None, + data=None if is_err else merged_text, + params=params, + invocation=f"Remote MCP tool {self.name} @ {self.url} with params {params!s}", + )
210-218: Guard against missing URL when listing remote toolsSame str(None) issue in connection setup.
- async def _get_server_tools(self): - async with sse_client(str(self.url), headers=self.headers) as ( + async def _get_server_tools(self): + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") + async with sse_client(url, headers=self.headers) as ( read_stream, write_stream, ): async with ClientSession(read_stream, write_stream) as session: _ = await session.initialize() return await session.list_tools()
265-293: Fix backward-compat and avoid mutating input; also normalize SSE URLs and support top-level command/args
- Current early return when mcp_type == "sse" drops a top-level url (already popped), breaking configs that rely on it.
- Do not mutate the passed config; copy and migrate top-level keys into nested config.
- Normalize SSE URLs by appending /sse when missing.
- Support top-level command/args for stdio as advertised.
-def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: - if not config: - raise ValueError("Config must not be empty") - # Prefer explicit mcp type set in nested 'config' for clarity. - # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). - mcp_config = config.get("config", {}) - mcp_type = mcp_config.get("type") - - if mcp_type == "stdio": - return StdioMCPToolset(**config, name=name) - - # Backwards compatibility: still support using url or command top-level keys. - url = config.pop("url", None) - if mcp_type == "sse": - return RemoteMCPToolset(**config, name=name) - if url is not None: - # fill the mcp server config with URL in case the mcp config is not set - if mcp_config == {}: - config["config"] = {"url": url} - else: - mcp_config["url"] = url - mcp_config["type"] = "sse" - return RemoteMCPToolset(**config, name=name) - - raise ValueError( - "MCP Server config must include a transport type ('sse' or 'stdio') " - "either in 'config.type' or by providing transport-specific keys." - ) +def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: + if not config: + raise ValueError("Config must not be empty") + + cfg = dict(config) + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") + + def _normalize_sse(u: str) -> str: + u = u.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" + + if mcp_type == "stdio": + # Ensure stdio params reside in nested config; accept top-level fallbacks. + if "command" not in mcp_config and "command" in cfg: + mcp_config["command"] = str(cfg.pop("command")) + mcp_config["args"] = cfg.pop("args", []) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + if mcp_type == "sse": + # Ensure URL present; accept top-level fallback and normalize. + if "url" not in mcp_config and "url" in cfg: + mcp_config["url"] = str(cfg.pop("url")) + if isinstance(mcp_config.get("url"), str): + mcp_config["url"] = _normalize_sse(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + # Backward-compat: infer transport from top-level keys. + if "url" in cfg: + mcp_config.setdefault("type", "sse") + mcp_config["url"] = _normalize_sse(str(cfg.pop("url"))) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if "command" in cfg: + mcp_config.setdefault("type", "stdio") + mcp_config["command"] = str(cfg.pop("command")) + mcp_config["args"] = cfg.pop("args", []) + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + raise ValueError( + "MCP Server config must include a transport type ('sse' or 'stdio') " + "either in 'config.type' or by providing transport-specific keys." + )
🧹 Nitpick comments (5)
holmes/plugins/toolsets/mcp/toolset_mcp.py (5)
43-59: Allow inputSchema to be None (type hint + guard)Aligns with remote servers that omit inputSchema and avoids mypy/type errors.
@classmethod def parse_input_schema( - cls, input_schema: dict[str, Any] + cls, input_schema: Optional[dict[str, Any]] ) -> Dict[str, ToolParameter]: if not input_schema: return {}
96-98: Use explicit conversion flag in f-string (RUF010)Minor formatting nit.
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!s}"
137-139: Use explicit conversion flag in f-string (RUF010)Minor formatting nit.
- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!s}"
151-166: Silence unusedconfigparam and improve error message textRename to underscore and avoid tuple-ified exception text.
- def init_server_tools(self, config: dict[str, Any]) -> Tuple[bool, str]: + def init_server_tools(self, _config: dict[str, Any]) -> Tuple[bool, str]: @@ - except Exception as e: + except Exception as e: return ( False, - f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {str(e.args)}", + f"Failed to load {self._get_server_type()} mcp server {self.name} {self._get_connection_info()} {e!s}", )
26-38: Renameuser_approvedto_user_approved, usestr(e)for errors, and add!son params;asyncio.runis safe
- Rename the unused
user_approvedparameter to_user_approved.- Change
error=str(e.args)toerror=str(e)and updateinvocationto
f"{self.__class__.__name__} {self.name} with params {params!s}".
No calls to_invokeoccur within async functions, so usingasyncio.runhere is safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (14)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)StructuredToolResultStatus(51-75)ToolParameter(154-159)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)get_example_config(727-728)get_example_config(746-747)get_example_config(779-780)
🪛 Ruff (0.13.1)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
97-97: Use explicit conversion flag
Replace with conversion flag
(RUF010)
138-138: Use explicit conversion flag
Replace with conversion flag
(RUF010)
151-151: Unused method argument: config
(ARG002)
160-160: Consider moving this statement to an else block
(TRY300)
161-161: Do not catch blind exception: Exception
(BLE001)
164-164: Use explicit conversion flag
Replace with conversion flag
(RUF010)
232-232: Avoid specifying long messages outside the exception class
(TRY003)
267-267: Avoid specifying long messages outside the exception class
(TRY003)
289-292: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (2)
187-191: Guard against missing URL when creating RemoteMCPTool instancesAvoid str(None) → "None" and confusing connection attempts.
- return [ - RemoteMCPTool.create(str(self.url), tool, self.headers) for tool in tools - ] + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") + return [RemoteMCPTool.create(url, tool, self.headers) for tool in tools]
103-121: Propagate errors from stdio CallToolResult; include command/args in invocationMirror SSE behavior for error propagation and add connection info.
- merged_text = " ".join( - c.text for c in tool_result.content if c.type == "text" - ) - return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - data=merged_text, - params=params, - invocation=f"Stdio MCP tool {self.name} with params {params}", - ) + merged_text = " ".join(c.text for c in tool_result.content if c.type == "text") + is_err = bool(tool_result.isError) + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR if is_err else StructuredToolResultStatus.SUCCESS, + error=merged_text if is_err else None, + data=None if is_err else merged_text, + params=params, + invocation=f"Stdio MCP tool {self.name} ({self.server_params.command} {' '.join(self.server_params.args)}) with params {params!s}", + )
moshemorad
left a comment
moshemorad
left a comment
There was a problem hiding this comment.
Great job. couple of small comments.
| server_params: StdioServerParameters | ||
|
|
||
| async def _invoke_async(self, params: Dict) -> StructuredToolResult: | ||
| async with stdio_client(self.server_params) as (read_stream, write_stream): |
There was a problem hiding this comment.
Do we really want to create the process for each tool call?
There was a problem hiding this comment.
Good point, we can reuse the same session.
There was a problem hiding this comment.
Sharing the session keeps raise the following error. I need more time to investigate
Traceback (most recent call last):
File "/home/azureuser/.cache/pypoetry/virtualenvs/holmesgpt-N4JKwMcz-py3.12/lib/python3.12/site-packages/mcp/client/stdio/__init__.py", line 180, in
stdio_client
async with (
File "/home/azureuser/.cache/pypoetry/virtualenvs/holmesgpt-N4JKwMcz-py3.12/lib/python3.12/site-packages/anyio/_backends/_asyncio.py", line 778, in
__aexit__
if self.cancel_scope.__exit__(type(exc), exc, exc.__traceback__):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/azureuser/.cache/pypoetry/virtualenvs/holmesgpt-N4JKwMcz-py3.12/lib/python3.12/site-packages/anyio/_backends/_asyncio.py", line 457, in
__exit__
raise RuntimeError(
RuntimeError: Attempted to exit cancel scope in a different task than it was entered in
``
| # Prefer explicit mcp type set in nested 'config' for clarity. | ||
| # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). | ||
| mcp_config = config.get("config", {}) | ||
| mcp_type = mcp_config.get("type") |
There was a problem hiding this comment.
I think that keep it backward compatible is a good idea. The thing that im not sure is clear from the flow is what will happen if the user did provided type:sse and also url: xxx not under the config field. Looks like it won't work right?
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (2)
71-84: Set data=None on error; keep merged text in error.Avoids populating both data and error simultaneously.
- return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - error=merged_text if tool_result.isError else None, - data=merged_text, - params=params, - invocation=f"MCPtool {self.name} with params {params}", - ) + is_err = bool(tool_result.isError) + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR if is_err else StructuredToolResultStatus.SUCCESS, + error=merged_text if is_err else None, + data=None if is_err else merged_text, + params=params, + invocation=f"MCPtool {self.name} with params {params!r}", + )
211-218: Validate URL before connecting in list_tools path.Prevents sse_client("None") if misconfigured.
- async with sse_client(str(self.url), headers=self.headers) as ( + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") + async with sse_client(url, headers=self.headers) as ( read_stream, write_stream, ):
🧹 Nitpick comments (10)
docs/data-sources/remote-mcp-servers.md (4)
22-27: Fix heading style (MD003) and prefer ATX headers.Convert the setext header to ATX to satisfy markdownlint.
-Transport selection and supported fields --------------------------------------- +## Transport selection and supported fields
54-60: Fix heading style (MD003) and add a blank line before the list.Current setext header plus immediate list violates our MkDocs rule.
-Security notes --------------- -- Avoid placing secrets directly in versioned config files. Prefer environment +## Security notes + +- Avoid placing secrets directly in versioned config files. Prefer environment variables or Holmes' secret interpolation for API keys and tokens. -- Holmes strips transport-only and common sensitive fields (for example - `type` and `key`) from subprocess parameters when constructing `StdioServerParameters`. +- Holmes strips transport-only and common sensitive fields (for example + `type`) from subprocess parameters when constructing `StdioServerParameters`.Note: The code currently only strips "type" in StdioMCPToolset; if you want to strip "key" too, align the code accordingly (see toolset_mcp.py suggestion).
96-96: Typo: “supergatway” → “Supergateway”.-Check out supergatway docs to find out other useful flags. +Check out Supergateway docs to find out other useful flags.
221-221: Fix formatting typo.Remove stray asterisks in bolded filename.
-Alternatively, you can add the `mcp_servers` configurations to **custom_toolset.yaml:***, and run: +Alternatively, you can add the `mcp_servers` configurations to **custom_toolset.yaml**, and run:tests/test_mcp_toolset.py (1)
216-225: Add test for top‑levelcommandbackward‑compat to match docs.Current tests don’t cover top-level
command. Add this to prevent regressions once the factory supports it.+def test_get_mcp_toolset_from_config_backward_compatibility_with_top_level_command(): + """Top-level command/args should be migrated into nested config for stdio.""" + config = {"command": "node", "args": ["server.js"]} + toolset = get_mcp_toolset_from_config(config, "test_stdio_backward") + assert isinstance(toolset, StdioMCPToolset) + assert toolset.name == "test_stdio_backward" + assert toolset.config.get("command") == "node" + assert toolset.config.get("args") == ["server.js"]holmes/plugins/toolsets/mcp/toolset_mcp.py (5)
26-38: Use str(e) (not e.args), and mark unused param; improve invocation formatting.Prevents tuple-y error strings and satisfies Ruff RUF010.
def _invoke( - self, params: Dict, user_approved: bool = False + self, params: Dict, user_approved: bool = False ) -> StructuredToolResult: try: return asyncio.run(self._invoke_async(params)) except Exception as e: return StructuredToolResult( status=StructuredToolResultStatus.ERROR, - error=str(e.args), + error=str(e), params=params, - invocation=f"{self.__class__.__name__} {self.name} with params {params}", + invocation=f"{self.__class__.__name__} {self.name} with params {params!r}", )
43-59: Accept Optional inputSchema to avoid AttributeError/mypy issues.Remote servers may omit inputSchema; signature should reflect Optional.
- def parse_input_schema( - cls, input_schema: dict[str, Any] - ) -> Dict[str, ToolParameter]: + def parse_input_schema( + cls, input_schema: Optional[dict[str, Any]] + ) -> Dict[str, ToolParameter]: if not input_schema: return {}
97-98: Use explicit conversion flag in f-string.Satisfies Ruff RUF010 and avoids manual str().
- return f"Call mcp server {self.url} tool {self.name} with params {str(params)}" + return f"Call mcp server {self.url} tool {self.name} with params {params!r}"
139-141: Use explicit conversion flag in f-string.- return f"Call stdio mcp server tool {self.name} with params {str(params)}" + return f"Call stdio mcp server tool {self.name} with params {params!r}"
231-237: Strip additional non-stdio keys before constructing StdioServerParameters.Docs imply sensitive keys are stripped. Also avoids unexpected kwargs.
params = dict(self.config) # pop out type which is not a parameter of StdioServerParameters - params.pop("type", None) + params.pop("type", None) + # defensively drop unrelated keys that could be present + for k in ("key", "headers", "url"): + params.pop(k, None) return StdioServerParameters(**params)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/data-sources/remote-mcp-servers.md(3 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(5 hunks)tests/test_mcp_toolset.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
In MkDocs content, always add a blank line between a header/bold text and a list to render lists properly
Files:
docs/data-sources/remote-mcp-servers.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
All Python code must include type hints (mypy enforced)
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/{core,plugins}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
All tools must return detailed error messages from underlying APIs, including executed command/query, time ranges/parameters, and full API error response; 'no data' responses must specify what was searched and where
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are declared in pyproject.toml; never introduce undeclared markers/tags
Files:
tests/test_mcp_toolset.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test layout should mirror the source structure under tests/
Files:
tests/test_mcp_toolset.py
🧠 Learnings (3)
📚 Learning: 2025-09-15T07:09:16.052Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T07:09:16.052Z
Learning: Applies to {holmes/plugins/toolsets/**/*.yaml,tests/llm/**/toolsets.yaml} : Valid top-level fields for toolset YAMLs are limited to: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (MCP only)
Applied to files:
docs/data-sources/remote-mcp-servers.md
📚 Learning: 2025-08-05T00:42:23.792Z
Learnt from: vishiy
PR: robusta-dev/holmesgpt#782
File: config.example.yaml:31-49
Timestamp: 2025-08-05T00:42:23.792Z
Learning: In robusta-dev/holmesgpt config.example.yaml, the azuremonitorlogs toolset configuration shows "enabled: true" as an example of how to enable the toolset, not as a default setting. The toolset is disabled by default and requires explicit enablement in user configurations.
Applied to files:
docs/data-sources/remote-mcp-servers.md
📚 Learning: 2025-09-15T07:09:16.052Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T07:09:16.052Z
Learning: Applies to holmes/plugins/toolsets/** : Toolsets must be organized under holmes/plugins/toolsets/ as either {name}.yaml files or directories
Applied to files:
docs/data-sources/remote-mcp-servers.md
🧬 Code graph analysis (2)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (11)
Tool(162-353)CallablePrerequisite(507-508)StructuredToolResult(78-102)StructuredToolResultStatus(51-75)ToolParameter(154-159)Toolset(520-735)_invoke(342-349)_invoke(402-432)get_parameterized_one_liner(352-353)get_parameterized_one_liner(379-386)model_post_init(181-215)
tests/test_mcp_toolset.py (2)
holmes/core/tools.py (2)
Tool(162-353)ToolParameter(154-159)holmes/plugins/toolsets/mcp/toolset_mcp.py (7)
RemoteMCPTool(61-98)RemoteMCPToolset(185-221)StdioMCPToolset(224-263)get_mcp_toolset_from_config(266-297)init_server_tools(152-166)headers(194-195)url(198-203)
🪛 markdownlint-cli2 (0.18.1)
docs/data-sources/remote-mcp-servers.md
22-22: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
54-54: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
🪛 Ruff (0.13.1)
holmes/plugins/toolsets/mcp/toolset_mcp.py
27-27: Unused method argument: user_approved
(ARG002)
98-98: Use explicit conversion flag
Replace with conversion flag
(RUF010)
140-140: Use explicit conversion flag
Replace with conversion flag
(RUF010)
152-152: Unused method argument: config
(ARG002)
161-161: Consider moving this statement to an else block
(TRY300)
162-162: Do not catch blind exception: Exception
(BLE001)
165-165: Use explicit conversion flag
Replace with conversion flag
(RUF010)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
294-297: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (4)
holmes/plugins/toolsets/mcp/toolset_mcp.py (4)
205-210: Optional: deprecation clarity for server type.Consider returning "remote" vs "stdio" in logs, or explicitly "sse" as done here; OK to keep.
253-261: Good: preserves full stdio parameters when listing tools.
188-191: Guard against missing URL; avoid str(None) → "None".Raise a clear error instead of attempting to connect to "None".
- return [ - RemoteMCPTool.create(str(self.url), tool, self.headers) for tool in tools - ] + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") + return [RemoteMCPTool.create(url, tool, self.headers) for tool in tools]
104-124: Mirror error handling for stdio: data=None on error; improve invocation.Aligns with SSE behavior and avoids dual data/error.
- merged_text = " ".join( - c.text for c in tool_result.content if c.type == "text" - ) - return StructuredToolResult( - status=( - StructuredToolResultStatus.ERROR - if tool_result.isError - else StructuredToolResultStatus.SUCCESS - ), - error=merged_text if tool_result.isError else None, - data=merged_text, - params=params, - invocation=f"Stdio MCP tool {self.name} with params {params}", - ) + merged_text = " ".join(c.text for c in tool_result.content if c.type == "text") + is_err = bool(tool_result.isError) + return StructuredToolResult( + status=StructuredToolResultStatus.ERROR if is_err else StructuredToolResultStatus.SUCCESS, + error=merged_text if is_err else None, + data=None if is_err else merged_text, + params=params, + invocation=f"Stdio MCP tool {self.name} with params {params!r}", + )
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
211-218: Guard againstNoneURL before callingsse_client.If
self.urlisNone,str(self.url)becomes the string"None", causing connection failures.Apply this diff:
async def _get_server_tools(self): - async with sse_client(str(self.url), headers=self.headers) as ( + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") + async with sse_client(url, headers=self.headers) as ( read_stream, write_stream, ):
♻️ Duplicate comments (3)
holmes/plugins/toolsets/mcp/toolset_mcp.py (3)
188-191: Guard againstNoneURL before creating tools.If
self.urlisNone,str(self.url)becomes the string"None", causing confusing connection attempts.Apply this diff:
def _create_tools(self, tools: List[MCP_Tool]) -> List[Tool]: + url = self.url + if not url: + raise ValueError("Remote MCP toolset requires a non-empty URL") - return [ - RemoteMCPTool.create(str(self.url), tool, self.headers) for tool in tools - ] + return [RemoteMCPTool.create(url, tool, self.headers) for tool in tools]Based on learnings
119-120: SetdatatoNonewhenisErrorisTrue.Currently, both
erroranddataare set tomerged_textwhen an error occurs. The error details should only populate theerrorfield, leavingdataasNone.Apply this diff:
- error=merged_text if tool_result.isError else None, - data=merged_text, + error=merged_text if tool_result.isError else None, + data=None if tool_result.isError else merged_text,
266-297: Do not mutate input config; support top-levelcommand/args; add deprecation warnings; normalize SSE URL.Current code has several critical issues:
- Line 278:
config.pop()mutates the caller's input, causing side effects.- Missing support for top-level
command/argskeys (documented in examples).- No deprecation warnings for backward-compatibility paths.
- SSE URL normalization not consistently applied.
Apply this diff to fix all issues:
def get_mcp_toolset_from_config(config: dict[str, Any], name: str) -> BaseMCPToolset: if not config: raise ValueError("Config must not be empty") - # Prefer explicit mcp type set in nested 'config' for clarity. - # Supported values: 'sse' (remote SSE server) and 'stdio' (stdio server). - mcp_config = config.get("config", {}) - mcp_type = mcp_config.get("type") - - if mcp_type == "stdio": - return StdioMCPToolset(**config, name=name) - - # DEPRECATED: support top-level url key for backward compatibility will be removed - url = config.pop("url", None) - # In case the user combine the new and old config styles with top-level url key and sse as config.type - if mcp_type == "sse": - if url is not None: - mcp_config["url"] = url - return RemoteMCPToolset(**config, name=name) - # When config.type is empty and url is set, assume sse transport. - if url is not None: - # fill the mcp server config with URL in case the mcp config is not set - if mcp_config == {}: - config["config"] = {"url": url} - else: - mcp_config["url"] = url - mcp_config["type"] = "sse" - return RemoteMCPToolset(**config, name=name) - - raise ValueError( - "MCP Server config must include a transport type ('sse' or 'stdio') " - "either in 'config.type' or by providing transport-specific keys." - ) + + cfg = dict(config) # avoid mutating caller input + mcp_config = dict(cfg.get("config", {})) + mcp_type = mcp_config.get("type") + + def _normalize_sse(u: str) -> str: + u = u.rstrip("/") + return u if u.endswith("/sse") else f"{u}/sse" + + if mcp_type == "stdio": + # allow mixing: promote any top-level command/args into nested config + if "command" in cfg and "command" not in mcp_config: + logging.warning("DEPRECATED: top-level 'command' will be removed; use config.command") + mcp_config["command"] = str(cfg.pop("command")) + if "args" in cfg and "args" not in mcp_config: + logging.warning("DEPRECATED: top-level 'args' will be removed; use config.args") + mcp_config["args"] = cfg.pop("args") + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + # DEPRECATED: support top-level url/command for backward compatibility + top_url = cfg.pop("url", None) + top_cmd = cfg.pop("command", None) + top_args = cfg.pop("args", None) + + if mcp_type == "sse": + if top_url is not None and "url" not in mcp_config: + logging.warning("DEPRECATED: top-level 'url' will be removed; use config.url") + mcp_config["url"] = top_url + if isinstance(mcp_config.get("url"), str): + mcp_config["url"] = _normalize_sse(mcp_config["url"]) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + + if mcp_type is None: + if top_url is not None: + logging.warning("DEPRECATED: inferring 'sse' from top-level 'url'; set config.type") + mcp_config["type"] = "sse" + mcp_config["url"] = _normalize_sse(str(top_url)) + cfg["config"] = mcp_config + return RemoteMCPToolset(**cfg, name=name) + if top_cmd is not None: + logging.warning("DEPRECATED: inferring 'stdio' from top-level 'command'; set config.type") + mcp_config["type"] = "stdio" + mcp_config["command"] = str(top_cmd) + if top_args is not None: + mcp_config["args"] = top_args + cfg["config"] = mcp_config + return StdioMCPToolset(**cfg, name=name) + + raise ValueError( + "MCP Server config must include a transport type ('sse' or 'stdio') " + "either in 'config.type' or by providing transport-specific keys." + )
🧹 Nitpick comments (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
103-103: Consider reusing the stdio client session.The TODO correctly identifies a performance concern: spawning a new process per tool invocation is inefficient.
Do you want me to investigate session reuse patterns or open an issue to track this optimization?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/mcp/toolset_mcp.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/init.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/mcp/toolset_mcp.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/mcp/toolset_mcp.py (1)
holmes/core/tools.py (13)
Tool(173-365)CallablePrerequisite(521-522)StructuredToolResult(79-103)StructuredToolResultStatus(52-76)ToolInvokeContext(164-170)ToolParameter(155-161)Toolset(534-768)get_parameterized_one_liner(364-365)get_parameterized_one_liner(391-398)model_post_init(192-226)get_example_config(760-761)get_example_config(779-780)get_example_config(812-813)
🪛 Ruff (0.13.3)
holmes/plugins/toolsets/mcp/toolset_mcp.py
97-97: Use explicit conversion flag
Replace with conversion flag
(RUF010)
140-140: Use explicit conversion flag
Replace with conversion flag
(RUF010)
152-152: Unused method argument: config
(ARG002)
161-161: Consider moving this statement to an else block
(TRY300)
162-162: Do not catch blind exception: Exception
(BLE001)
165-165: Use explicit conversion flag
Replace with conversion flag
(RUF010)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
268-268: Avoid specifying long messages outside the exception class
(TRY003)
294-297: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: build
🔇 Additional comments (4)
holmes/plugins/toolsets/mcp/toolset_mcp.py (4)
42-57: LGTM! inputSchema guard properly implemented.The
parse_input_schemamethod now correctly handlesNoneor missinginputSchemaby returning an empty dict, addressing the previous review comment.
197-203: LGTM! SSE URL normalization correctly implemented.The property correctly normalizes the URL by ensuring the
/ssesuffix is present, addressing the previous review comment.
230-237: LGTM! Config guard properly implemented.The property now correctly guards against
Noneconfig before constructingStdioServerParameters, addressing the previous review comment.
253-260: LGTM! Full stdio parameters preserved.The method now correctly uses
self.stdio_server_paramsto preserve all parameters (env, cwd, etc.), addressing the previous review comment.
|
we updated mcp_toolset to support http-streamable and opened a seperate pr for stdio support |
3 participants
No description provided.