fix(cli): accept configured MCP prefixed toolsets before discovery - #44044
thedavidweng wants to merge 1 commit into
Conversation
|
Confirming this reproduces in a real-world multi-profile deployment on We run a fleet of Hermes profiles where CodeGraph's installer added even though the server is configured correctly and fully functional — This PR's approach — accepting both |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for covering the three pre-discovery validators. The premise is confirmed on current main: cli.py:3887-3890 excludes only the bare configured server key, while MCP registration creates mcp-<server> at tools/mcp_tool.py:4750 and the docs endorse that spelling at website/docs/reference/toolsets-reference.md:124-134.
Problems
hermes_cli/mcp_toolsets.py:53aliases non-mapping server values. Existing oneshot/TUI validation skips them, and runtime loading only retains dict entries intools/mcp_tool.py:3773-3776; this can validate a toolset that discovery cannot register.
Suggested changes
- Skip non-mapping server entries in the shared helper and cover that malformed-config case with a focused test.
The PR is currently conflicting, but the current CLI, oneshot, and TUI code still contains the verified bug class, so this otherwise looks mechanically salvageable.
Automated hermes-sweeper review.
1ffedde to
5ce21da
Compare
Update after rebase + teknium1 reviewRebased onto latest main (resolved 2 conflict in test files, kept both sides). Addressed teknium1's review:
All targeted tests pass:
Pre-existing test failures in tests/tools/test_mcp_reconnect_signal.py and test_mcp_server_log_notifications.py exist on unmodified main — unrelated to this PR. |
5ce21da to
d3d769f
Compare
Rebased onto current main (was 9295 commits behind)Force-pushed as
Verification on the rebased commit: |
What does this PR do?
Fix the same class of false-positive MCP toolset validation across CLI, TUI, and oneshot (
-z) startup paths.Current
mainalready suppresses the warning for bare MCP server names (agentmail,notion, etc.), but it still warns for the documentedmcp-<server>spelling (mcp-agentmail,mcp-notion, etc.) before MCP discovery registers runtime toolsets. That causes startup warnings even when the user has configured MCP servers correctly inconfig.yaml.This PR adds shared pre-discovery MCP toolset helpers and applies them consistently to the three startup validators so
mcp-<server>is accepted when the underlying MCP server is configured (enabled or disabled), while genuinely unknown toolsets still warn.Fixes #78102
Related Issue
Related #41625
Related #23997
Related #29532
Related #5279
Type of Change
Changes Made
hermes_cli/mcp_toolsets.pycli.py-z/ oneshot validator to accept configured MCP prefixed toolsets before discovery:hermes_cli/oneshot.pytui_gateway/server.pytests/cli/test_mcp_toolset_validation.pytests/hermes_cli/test_tui_resume_flow.pytests/test_tui_gateway_server.pyHow to Test
mcp_serversin~/.hermes/config.yamlwith one or more servers.platform_toolsetsentries such as:mcp-agentmailmcp-notionmcp-notion-finditmcp-tinyfishhermes -z) and confirm no false:Warning: Unknown toolsets: mcp-agentmail, mcp-notion, ...venv/bin/python -m pytest tests/cli/test_mcp_toolset_validation.py -qvenv/bin/python -m pytest tests/hermes_cli/test_tui_resume_flow.py -qvenv/bin/python -m pytest tests/test_tui_gateway_server.py -qvenv/bin/python -m pytest tests/tools/test_mcp*.py -qChecklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs