fix(mcp): review findings — reinstall no longer clobbers user exclude lists + 4 curation gaps
Review blockers (independent reviewer on #94513): 1. Reinstalling an exclude-mode catalog entry wiped the user's edited tools.exclude, replacing it with manifest defaults. install_entry now reads the prior exclude (like it already did for include) and re-writes it verbatim on reinstall. Regression test added + sabotage-verified (fails on old behavior); include-priority test added too. 2. aws-knowledge: exclude aws___retrieve_skill — vendor SKILL.md loader is a vendor skill layer (live tools/list confirmed the tool exists). 3. betterstack: exclude list rewritten to cover the snake_case wire names (vendor's own header examples show remove_dashboard) via globs alongside the doc display-labels; caveat documented in the manifest — server is OAuth-gated so pre-auth enumeration is impossible. 4. railway: exclude railway-agent (opaque server-side agent delegation, acts outside Hermes's per-tool approval loop). 5. twelve-data: exclude oauth plumbing pseudo-tools + quota probe. 6. betterstack post_install no longer claims a fully-checked checklist — exclude-mode bypasses the checklist; text now describes the applied exclude list. Live E2E: fresh temp HERMES_HOME — install applies manifest excludes, user edit survives reinstall. 33/33 catalog tests green.
This commit is contained in:
@@ -623,6 +623,25 @@ def _read_prior_tool_selection(name: str) -> Optional[List[str]]:
|
||||
return None
|
||||
|
||||
|
||||
def _read_prior_tool_exclude(name: str) -> Optional[List[str]]:
|
||||
"""Return the user's prior `tools.exclude` for *name*, if any.
|
||||
|
||||
The exclude-mode counterpart of :func:`_read_prior_tool_selection`.
|
||||
Read BEFORE a reinstall overwrites the server entry, so a user-edited
|
||||
exclude list survives reinstalling an exclude-mode catalog entry instead
|
||||
of being clobbered by the manifest's ``default_excluded``.
|
||||
"""
|
||||
servers = installed_servers()
|
||||
cfg = servers.get(name) or {}
|
||||
tools_cfg = cfg.get("tools") or {}
|
||||
if not isinstance(tools_cfg, dict):
|
||||
return None
|
||||
exclude = tools_cfg.get("exclude")
|
||||
if isinstance(exclude, list) and all(isinstance(t, str) for t in exclude):
|
||||
return list(exclude)
|
||||
return None
|
||||
|
||||
|
||||
def _probe_tools(name: str) -> Optional[List[tuple]]:
|
||||
"""Connect to a freshly-configured MCP and list its tools.
|
||||
|
||||
@@ -684,7 +703,10 @@ def _write_tools_exclude(name: str, exclude: List[str]) -> None:
|
||||
|
||||
|
||||
def _apply_tool_selection(
|
||||
entry: CatalogEntry, *, prior_selection: Optional[List[str]]
|
||||
entry: CatalogEntry,
|
||||
*,
|
||||
prior_selection: Optional[List[str]],
|
||||
prior_exclude: Optional[List[str]] = None,
|
||||
) -> None:
|
||||
"""Probe the server and let the user pick which tools to enable.
|
||||
|
||||
@@ -707,9 +729,22 @@ def _apply_tool_selection(
|
||||
|
||||
# Exclude-mode manifests short-circuit the checklist entirely: the curated
|
||||
# exclude list (names or glob patterns) is written as-is, everything else
|
||||
# stays enabled — including tools the server adds later. A reinstall with
|
||||
# a prior include selection still honours the user's own choice below.
|
||||
# stays enabled — including tools the server adds later. Reinstalls
|
||||
# preserve the user's own prior filter in EITHER mode: a prior include
|
||||
# selection falls through to the checklist below, and a prior user-edited
|
||||
# exclude list is re-written verbatim instead of being clobbered by the
|
||||
# manifest defaults.
|
||||
if entry.tools.default_excluded and prior_selection is None:
|
||||
if prior_exclude is not None:
|
||||
_write_tools_exclude(entry.name, prior_exclude)
|
||||
print(color(
|
||||
f" Kept your existing exclude list ({len(prior_exclude)} "
|
||||
f"entries). Edit mcp_servers.{entry.name}.tools.exclude in "
|
||||
"config.yaml or run "
|
||||
f"`hermes mcp configure {entry.name}` to change.",
|
||||
Colors.GREEN,
|
||||
))
|
||||
return
|
||||
_write_tools_exclude(entry.name, entry.tools.default_excluded)
|
||||
print(color(
|
||||
f" Applied manifest exclude list "
|
||||
@@ -885,8 +920,10 @@ def install_entry(entry: CatalogEntry, *, enable: bool = True) -> None:
|
||||
|
||||
# ── Preserve any prior user tool selection across reinstalls ────────
|
||||
# Reading BEFORE we overwrite the entry below so a reinstall pre-checks
|
||||
# whatever the user picked last time.
|
||||
# whatever the user picked last time (include mode) or keeps the user's
|
||||
# edited exclude list (exclude mode).
|
||||
prior_selection = _read_prior_tool_selection(entry.name)
|
||||
prior_exclude = _read_prior_tool_exclude(entry.name)
|
||||
|
||||
# Build and write the mcp_servers entry (without tools filter yet;
|
||||
# _apply_tool_selection() finalizes it below).
|
||||
@@ -901,7 +938,9 @@ def install_entry(entry: CatalogEntry, *, enable: bool = True) -> None:
|
||||
)
|
||||
|
||||
# ── Probe + tool selection ──────────────────────────────────────────
|
||||
_apply_tool_selection(entry, prior_selection=prior_selection)
|
||||
_apply_tool_selection(
|
||||
entry, prior_selection=prior_selection, prior_exclude=prior_exclude
|
||||
)
|
||||
|
||||
print()
|
||||
print(color(
|
||||
|
||||
@@ -17,6 +17,13 @@ transport:
|
||||
auth:
|
||||
type: none
|
||||
|
||||
# aws___retrieve_skill fetches vendor-authored SKILL.md workflow files — a
|
||||
# vendor skill/discovery layer. Hermes's own skills and tool_search are the
|
||||
# only instruction/deferral layers we ship (verified live via tools/list).
|
||||
tools:
|
||||
default_excluded:
|
||||
- aws___retrieve_skill
|
||||
|
||||
# Composer-suggestion triggers (desktop brand pills).
|
||||
suggest:
|
||||
keywords:
|
||||
|
||||
@@ -18,28 +18,28 @@ transport:
|
||||
auth:
|
||||
type: oauth
|
||||
|
||||
# Curated exclude list (106-tool surface). Excluded: vendor-docs search;
|
||||
# eight instruction-fetcher pseudo-tools (static how-to text as tools);
|
||||
# Curated exclude list (~106-tool surface). Excluded: vendor-docs search;
|
||||
# the instruction-fetcher pseudo-tools (static how-to text as tools);
|
||||
# Execute query / Create cloud connection (raw ClickHouse-SQL escape hatch +
|
||||
# direct-DB credential minting); team-membership management (account access
|
||||
# changes). ~85 product tools stay enabled; re-enable any with
|
||||
# `hermes mcp configure betterstack`.
|
||||
# changes). NOTE: Better Stack's docs list display labels while the wire
|
||||
# uses snake_case (their own header examples show `remove_dashboard`); the
|
||||
# server is OAuth-gated so names could not be enumerated pre-auth. Entries
|
||||
# below cover BOTH shapes — glob patterns for the snake_case wire names plus
|
||||
# the display-label literals; unmatched entries no-op harmlessly.
|
||||
# ~85+ product tools stay enabled; re-check with
|
||||
# `hermes mcp configure betterstack` after first login.
|
||||
tools:
|
||||
default_excluded:
|
||||
- search_documentation
|
||||
- Search documentation
|
||||
- Get query instructions
|
||||
- Get metric query instructions
|
||||
- Get errors query instructions
|
||||
- Get replays query instructions
|
||||
- Get explore logs query instructions
|
||||
- Get chart building instructions
|
||||
- Get chart alert instructions
|
||||
- Get dashboard query instructions
|
||||
- "*instructions*"
|
||||
- execute_query
|
||||
- Execute query
|
||||
- create_cloud_connection
|
||||
- Create cloud connection
|
||||
- Invite team member
|
||||
- Remove team member
|
||||
- Change team member role
|
||||
- "*team_member*"
|
||||
- "*team member*"
|
||||
|
||||
# Composer-suggestion triggers (desktop brand pills).
|
||||
suggest:
|
||||
@@ -55,8 +55,9 @@ post_install: |
|
||||
Better Stack (or run `hermes mcp login betterstack`). Approve access,
|
||||
then restart the session so tools load.
|
||||
|
||||
Heads-up: this server exposes a LARGE tool surface (~111 tools as of
|
||||
Aug 2026 — roughly 40-60K tokens of schema if all stay enabled). The
|
||||
install-time checklist starts fully checked; prune to the product areas
|
||||
you actually use (logs, monitors, incidents...), or re-run later with:
|
||||
Heads-up: this server exposes a LARGE tool surface (~106 tools as of
|
||||
Aug 2026). Hermes applies a curated exclude list automatically at install
|
||||
(docs search, instruction pseudo-tools, the raw-SQL query hatch, cloud
|
||||
credential minting, and team-membership tools); everything else stays
|
||||
enabled. Review or change the selection any time with:
|
||||
hermes mcp configure betterstack
|
||||
|
||||
@@ -18,6 +18,14 @@ transport:
|
||||
auth:
|
||||
type: oauth
|
||||
|
||||
# Excluded: railway-agent hands the request to Railway's server-side AI
|
||||
# agent for multi-step infra operations — an opaque delegation meta-layer
|
||||
# that acts outside Hermes's per-tool approval loop. The remaining tools
|
||||
# are direct (and destructive ones carry vendor destructive-hints).
|
||||
tools:
|
||||
default_excluded:
|
||||
- railway-agent
|
||||
|
||||
# Composer-suggestion triggers (desktop brand pills).
|
||||
suggest:
|
||||
keywords:
|
||||
|
||||
@@ -19,6 +19,15 @@ transport:
|
||||
auth:
|
||||
type: oauth
|
||||
|
||||
# Excluded: OAuth plumbing exposed as tools on the cloud server, plus the
|
||||
# account-quota probe. All remaining tools are read-only market data.
|
||||
tools:
|
||||
default_excluded:
|
||||
- oauth_login
|
||||
- auth_status
|
||||
- oauth_configure
|
||||
- get_api_usage
|
||||
|
||||
# Composer-suggestion triggers (desktop brand pills).
|
||||
suggest:
|
||||
keywords:
|
||||
|
||||
@@ -327,6 +327,71 @@ class TestInstall:
|
||||
assert server["tools"]["include"] == ["tool_a"]
|
||||
assert "exclude" not in server["tools"]
|
||||
|
||||
def test_reinstall_preserves_user_edited_exclude_list(
|
||||
self, catalog_dir, monkeypatch
|
||||
):
|
||||
"""A user-edited tools.exclude survives reinstall of an exclude-mode
|
||||
manifest instead of being clobbered by the manifest defaults."""
|
||||
body = _basic_manifest(
|
||||
tools={"default_excluded": ["docs", "*_radar_*"]},
|
||||
)
|
||||
_write_manifest(catalog_dir, "demo", body)
|
||||
import hermes_cli.mcp_catalog as mc
|
||||
from hermes_cli.config import load_config, save_config
|
||||
|
||||
user_exclude = ["docs", "*_radar_*", "my_custom_block"]
|
||||
cfg = load_config()
|
||||
cfg.setdefault("mcp_servers", {})["demo"] = {
|
||||
"command": "npx",
|
||||
"args": ["-y", "demo-mcp"],
|
||||
"enabled": True,
|
||||
"tools": {"exclude": list(user_exclude)},
|
||||
}
|
||||
save_config(cfg)
|
||||
|
||||
def _fail_probe(name):
|
||||
raise AssertionError("probe must not run for exclude-mode manifests")
|
||||
|
||||
monkeypatch.setattr(mc, "_probe_tools", _fail_probe)
|
||||
mc.install_entry(_entry("demo"), enable=True)
|
||||
|
||||
server = load_config()["mcp_servers"]["demo"]
|
||||
assert server["tools"]["exclude"] == user_exclude
|
||||
assert "include" not in server["tools"]
|
||||
|
||||
def test_include_mode_reinstall_ignores_stale_exclude(
|
||||
self, catalog_dir, monkeypatch
|
||||
):
|
||||
"""When the user previously chose an include selection, a leftover
|
||||
exclude value must not shadow it on reinstall of an exclude-mode
|
||||
manifest — include (explicit user checklist choice) wins."""
|
||||
body = _basic_manifest(
|
||||
tools={"default_excluded": ["*_radar_*"]},
|
||||
)
|
||||
_write_manifest(catalog_dir, "demo", body)
|
||||
import hermes_cli.mcp_catalog as mc
|
||||
from hermes_cli.config import load_config, save_config
|
||||
|
||||
cfg = load_config()
|
||||
cfg.setdefault("mcp_servers", {})["demo"] = {
|
||||
"command": "npx",
|
||||
"args": ["-y", "demo-mcp"],
|
||||
"enabled": True,
|
||||
"tools": {"include": ["tool_a"]},
|
||||
}
|
||||
save_config(cfg)
|
||||
|
||||
import sys as _sys
|
||||
probed = [("tool_a", "a"), ("tool_b", "b")]
|
||||
monkeypatch.setattr(mc, "_probe_tools", lambda name: probed)
|
||||
monkeypatch.setattr(_sys.stdin, "isatty", lambda: False)
|
||||
|
||||
mc.install_entry(_entry("demo"), enable=True)
|
||||
|
||||
server = load_config()["mcp_servers"]["demo"]
|
||||
assert server["tools"]["include"] == ["tool_a"]
|
||||
assert "exclude" not in server["tools"]
|
||||
|
||||
def test_install_rejects_exfil_shaped_stdio_manifest(self, catalog_dir):
|
||||
body = _basic_manifest(
|
||||
"evil",
|
||||
|
||||
Reference in New Issue
Block a user