Skip to content

Fix MCP toolset prerequisites check - #990

Merged
mainred merged 2 commits into
HolmesGPT:masterfrom
feiskyer:fix-mcp-tools
Sep 27, 2025
Merged

mainred merged 2 commits into
HolmesGPT:masterfrom
feiskyer:fix-mcp-tools

Conversation

@feiskyer

Copy link
Copy Markdown
Contributor

There are high chances of empty MCP tools when using remote MCP server.

This PR ensures MCP servers reload their tools even when previously failed by including MCP toolsets in prerequisites check regardless of status.

@CLAassistant

CLAassistant commented Sep 25, 2025 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Sep 25, 2025 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The cache collection logic in holmes/core/toolset_manager.py now collects enabled toolsets whose status is ENABLED or whose type is MCP, so MCP-typed toolsets are reloaded from cache even when their cached status is not ENABLED. An inline comment documents this behavior.

Changes

Cohort / File(s) Summary
Toolset cache handling
holmes/core/toolset_manager.py
Broadened condition when collecting enabled toolsets from cache: include toolsets that are enabled and either have status == ENABLED or type == MCP; added inline comment noting MCP prerequisite reload behavior.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Caller
    participant TM as ToolsetManager
    participant Cache
    participant TS as Toolset

    Caller->>TM: load_toolsets()
    TM->>Cache: get_cached_toolsets()
    Cache-->>TM: cached_toolsets

    loop per cached toolset t
        TM->>TM: is_enabled(t)?
        alt t.status == ENABLED or t.type == MCP
            note right of TM #E7F6E7: collect t into enabled_toolsets_from_cache
            TM->>TS: reload prerequisites / init_config as needed
            TS-->>TM: ready
        else
            TM->>TM: skip t
        end
    end

    TM-->>Caller: enabled_toolsets_from_cache
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • nherment
  • aantn
  • mainred

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title accurately and concisely summarizes the primary change by indicating that the PR fixes the prerequisites check specifically for MCP toolsets, and it avoids unnecessary detail or noise.
Description Check ✅ Passed The description clearly explains the underlying issue with empty MCP tools when using a remote MCP server and how the PR modifies the prerequisites check to ensure MCP toolsets are reloaded even after failures, directly matching the changes in the code.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mainred mainred left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently refresh toolsets happens when the first the user runs holmesgpt or specify refresh-toolsets, and mcp server is reachable, its status will be cached.
I think your case is mcp server is not reachable the first time we run hollmesgpt?

@feiskyer

Copy link
Copy Markdown
Contributor Author

per my local test, the mcp endpoint is always reachable, but holmes failed to fetch its tool list. That's why I have this PR.
And holmes should retry here as well when last refresh has failed.

Comment thread holmes/core/toolset_manager.py
mainred
mainred previously approved these changes Sep 26, 2025
@mainred
mainred enabled auto-merge (squash) September 26, 2025 03:12
Ensure MCP servers reload their tools even when previously failed by including MCP toolsets in prerequisites check regardless of status.
auto-merge was automatically disabled September 26, 2025 05:08

Head branch was pushed to by a user without write access

@mainred
mainred enabled auto-merge (squash) September 27, 2025 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants