From 8def746896dce8532db288c600dbf696095243ee Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 24 Sep 2026 05:36:08 -0500 Subject: [PATCH] fix(gateway): resolve catalog MCP ids in mcp.servers.add Desktop per-bot add-from-catalog sends a catalog id as preset. The handler only consulted the CLI preset registry, which raises before the 4063 check, so every catalog add returned 5024. Resolve the catalog first; an explicit url or command still wins; an unknown id returns 4063. Fixes #118944 --- tests/tui_gateway/test_mcp_profile_rpcs.py | 132 +++++++++++++++++++++ tui_gateway/methods_tools.py | 21 +++- 2 files changed, 149 insertions(+), 4 deletions(-) diff --git a/tests/tui_gateway/test_mcp_profile_rpcs.py b/tests/tui_gateway/test_mcp_profile_rpcs.py index ca1b7f6a56..2abbfd1107 100644 --- a/tests/tui_gateway/test_mcp_profile_rpcs.py +++ b/tests/tui_gateway/test_mcp_profile_rpcs.py @@ -347,6 +347,138 @@ def test_add_requires_transport(hermes_root): assert resp["error"]["code"] == 4063 +def _catalog_http_entry(*, auth: str | None = None): + """A real HTTP catalog entry. Assertions compare the saved block to this manifest.""" + from hermes_cli.mcp_catalog import list_catalog + + for entry in list_catalog(): + if entry.transport.type != "http" or not entry.transport.url: + continue + if auth is not None and entry.auth.type != auth: + continue + return entry + raise AssertionError(f"catalog has no http entry with auth={auth!r}") + + +def _saved_server(root: Path, profile: str, name: str) -> dict: + path = root / "profiles" / profile / "config.yaml" if profile else root / "config.yaml" + return (_read_yaml(path).get("mcp_servers") or {}).get(name) or {} + + +def test_add_catalog_id_in_profile_param_saves_manifest_in_that_profile(hermes_root): + """Desktop add-from-catalog sends {profile, name, preset} with a catalog id.""" + entry = _catalog_http_entry() + result = _result( + _call( + "mcp.servers.add", + {"profile": "work", "name": entry.name, "preset": entry.name}, + ) + ) + + assert result["ok"] is True + assert result["server"]["transport"] == "http" + saved = _saved_server(hermes_root, "work", entry.name) + assert saved["url"] == entry.transport.url + assert entry.name not in (_read_yaml(hermes_root / "config.yaml").get("mcp_servers") or {}) + other = _read_yaml(hermes_root / "profiles" / "other" / "config.yaml").get("mcp_servers") or {} + assert entry.name not in other + + +def test_oauth_catalog_add_follows_routed_profile_not_payload(hermes_root): + """OAuth add sends {name, preset} only. requestGatewayForAgent carries the + profile as routing metadata, so the write follows the bound scope.""" + from agent.secret_scope import is_multiplex_active, set_multiplex_active + from hermes_constants import reset_hermes_home_override, set_hermes_home_override + + entry = _catalog_http_entry(auth="oauth") + routed = hermes_root / "profiles" / "work" + previous = is_multiplex_active() + set_multiplex_active(False) + token = set_hermes_home_override(routed) + try: + result = _result( + _call("mcp.servers.add", {"name": entry.name, "preset": entry.name}) + ) + finally: + reset_hermes_home_override(token) + set_multiplex_active(previous) + + assert result["server"]["transport"] == "http" + assert result["server"]["auth"] == "oauth" + saved = _saved_server(hermes_root, "work", entry.name) + assert saved["url"] == entry.transport.url + assert saved["auth"] == "oauth" + assert entry.auth.type == "oauth" + assert entry.name not in (_read_yaml(hermes_root / "config.yaml").get("mcp_servers") or {}) + assert not _saved_server(hermes_root, "other", entry.name) + + +def test_explicit_transport_wins_over_catalog_or_unknown_preset(hermes_root): + entry = _catalog_http_entry() + catalog = _result( + _call( + "mcp.servers.add", + { + "profile": "work", + "name": "kept-catalog", + "preset": entry.name, + "config": {"command": "explicit-bin"}, + }, + ) + ) + unknown = _result( + _call( + "mcp.servers.add", + { + "profile": "work", + "name": "kept-unknown", + "preset": "not-a-catalog-entry", + "config": {"url": "https://override.example/mcp"}, + }, + ) + ) + + assert catalog["server"]["command"] == "explicit-bin" + assert catalog["server"]["url"] != entry.transport.url + saved_catalog = _saved_server(hermes_root, "work", "kept-catalog") + assert saved_catalog["command"] == "explicit-bin" + assert saved_catalog.get("url") != entry.transport.url + assert unknown["server"]["url"] == "https://override.example/mcp" + assert _saved_server(hermes_root, "work", "kept-unknown")["url"] == "https://override.example/mcp" + + +def test_unknown_preset_returns_4063_and_writes_nothing(hermes_root): + resp = _call( + "mcp.servers.add", + {"profile": "work", "name": "unknown", "preset": "not-a-catalog-entry"}, + ) + + assert resp["error"]["code"] == 4063 + assert not _saved_server(hermes_root, "work", "unknown") + assert "unknown" not in (_read_yaml(hermes_root / "config.yaml").get("mcp_servers") or {}) + + +def test_cli_preset_still_fills_transport_when_not_in_catalog(hermes_root): + import hermes_cli.mcp_config as mcp_config + from hermes_cli.mcp_catalog import get_entry + + preset_name = next( + name for name in mcp_config._MCP_PRESETS if get_entry(name) is None + ) + expected = mcp_config._MCP_PRESETS[preset_name] + result = _result( + _call( + "mcp.servers.add", + {"profile": "work", "name": "cli-preset", "preset": preset_name}, + ) + ) + + saved = _saved_server(hermes_root, "work", "cli-preset") + assert result["server"]["command"] == expected["command"] + assert saved["command"] == expected["command"] + assert saved.get("args") == list(expected.get("args") or []) + + def test_default_profile_add_when_profile_omitted(hermes_root): root = hermes_root _result( diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index 856f59fce4..d491e6a6cb 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -1362,10 +1362,23 @@ def _(rid, params: dict) -> dict: return _err(rid, 4090, f"server '{name}' already exists") raw_cfg = params.get("config") server_config: dict = dict(raw_cfg) if isinstance(raw_cfg, dict) else {} - if preset: # fills url/command/args when omitted; mutates server_config in place - mc._apply_mcp_preset( - name, preset_name=preset, url=server_config.get("url"), command=server_config.get("command"), - cmd_args=list(server_config.get("args") or []), server_config=server_config) + # Explicit url/command wins. Otherwise a desktop catalog id is resolved + # before the CLI preset registry — that registry raises, and the wrapper + # turns the raise into 5024 before the 4063 check below can run. + if preset and not (server_config.get("url") or server_config.get("command")): + catalog = _tools_mod("hermes_cli.mcp_catalog") + entry = catalog.get_entry(preset) + if entry is not None: + for key, value in catalog._build_server_config(entry, install_dir=None).items(): + server_config.setdefault(key, value) + else: + try: + mc._apply_mcp_preset( + name, preset_name=preset, url=server_config.get("url"), + command=server_config.get("command"), + cmd_args=list(server_config.get("args") or []), server_config=server_config) + except ValueError: + return _err(rid, 4063, f"Unknown MCP catalog entry or preset: {preset}") if not server_config.get("url") and not server_config.get("command"): return _err(rid, 4063, "config must specify a 'url' (http) or 'command' (stdio), or a valid 'preset'") if bearer_token := params.get("bearer_token"):