From 5a1e04dcf1bbdea09efaab14bb9d50227fbe4a71 Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Sat, 26 Sep 2026 22:28:43 +0900 Subject: [PATCH] fix(mcp): resolve the adopter's identity in its own scope, and never abort the pass on it discover_mcp_tools binds the owner secret scope only around the config load (#113746), so a routed profile's reconciliation ran with no ambient scope. The adopter's stdio identity then resolved unscoped, and with a source-tagged secret name get_secret raised UnscopedSecretError: the stack's None sentinel kept that safe (the share was refused) but two profiles holding the same value never shared the owner's child. The omitted-name config load and the per-name identity resolution now run under this profile's own secret scope (_owner_secret_scope), outside the registry lock. Salvage resolution: the out-of-lock, once-per-name resolution, the None refuse sentinel and the per-server refusal were already on the stack (_adopter_identity_digest / resolved_ids), so this keeps that one implementation and takes the contributor's scope binding. The contributor's unscoped-routed test is folded as an assertion into the kept multi-credential test (test budget); their 'one unresolvable identity refuses only that share' test duplicates the kept 'boom' case and is dropped, as are the test tweaks written against their _resolved_identity signature. Co-authored-by: JoaoMarcos44 (cherry picked from commit a8627071e7367cd544af77f921b4403a0b6c3e36) --- .../test_mcp_multiplex_connection_keys.py | 33 ++++++++++++++++--- tools/mcp_tool_registration.py | 17 ++++++---- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/tests/tools/test_mcp_multiplex_connection_keys.py b/tests/tools/test_mcp_multiplex_connection_keys.py index 9d680a39cb..d2268ea375 100644 --- a/tests/tools/test_mcp_multiplex_connection_keys.py +++ b/tests/tools/test_mcp_multiplex_connection_keys.py @@ -52,9 +52,10 @@ def two_profiles(tmp_path, monkeypatch): return hermes_home_key(homes[which]) yield enter - for tool_name in list(registry.get_tool_names_for_toolset("mcp-x")): - for home in homes.values(): - registry.deregister(tool_name, scope=hermes_home_key(home)) + for toolset in ("mcp-x", "mcp-s"): + for tool_name in list(registry.get_tool_names_for_toolset(toolset)): + for home in homes.values(): + registry.deregister(tool_name, scope=hermes_home_key(home)) for token in reversed(tokens): reset_hermes_home_override(token) for n in ledgers: @@ -62,7 +63,8 @@ def two_profiles(tmp_path, monkeypatch): getattr(core, n).update(saved[n]) -def test_same_named_server_with_other_credentials_is_a_separate_connection(two_profiles): +def test_same_named_server_with_other_credentials_is_a_separate_connection(two_profiles, tmp_path, + monkeypatch): import tools.mcp_tool as core from tools import mcp_tool_discovery as disc, mcp_tool_handlers as handlers from tools import mcp_tool_registration as reg @@ -114,6 +116,29 @@ def test_same_named_server_with_other_credentials_is_a_separate_connection(two_p assert scope_b in core._server_tool_scopes[(scope_a, "ok")] assert scope_b not in core._server_tool_scopes[(scope_a, "boom")] + # A routed profile reconciles with NO ambient secret scope (``discover_mcp_tools`` binds the + # owner scope only around the config load, #113746): with a source-tagged secret the adopter's + # stdio identity still resolves in ITS OWN scope, so an equal value shares the owner's child. + import sys + import agent.secret_scope as secret_scope + import hermes_cli.env_loader as env_loader + monkeypatch.setattr(secret_scope, "_MULTIPLEX_ACTIVE", True) + monkeypatch.setattr(env_loader, "_SECRET_SOURCES", {"FIXTURE_TOKEN": "op"}) + for profile in ("a", "b"): + (tmp_path / "profiles" / profile / ".env").write_text("FIXTURE_TOKEN=tok\n", encoding="utf-8") + cfg_s = {"command": sys.executable, "args": ["-c", "pass"]} + two_profiles("a") + with disc._owner_secret_scope(): # the connecting task records its digest in the owner's scope + srv_s = _server("s", cfg_s) + assert srv_s._resolved_identity is not None + disc._adopt_server("s", srv_s) + srv_s._registered_tool_names = reg._register_server_tools("s", srv_s, cfg_s) + two_profiles("b") + assert secret_scope.current_secret_scope() is None + assert reg.register_connected_into_current_scope({"s": dict(cfg_s)}) == 1 + assert registry.get_tool_names_for_toolset("mcp-s") == ["mcp__s__t"] + assert "s" not in disc._select_new_servers({"s": dict(cfg_s)}) + def test_oauth_server_is_not_adopted_across_profiles(two_profiles): import tools.mcp_tool as core diff --git a/tools/mcp_tool_registration.py b/tools/mcp_tool_registration.py index 4fbb79afc8..1875e38f91 100644 --- a/tools/mcp_tool_registration.py +++ b/tools/mcp_tool_registration.py @@ -508,15 +508,20 @@ def _register_connected_into_current_scope(servers: dict) -> int: if scope in scopes and _key_name(key) not in servers} # Only a name another profile holds a connection for reaches a cross-profile comparison. foreign = {_key_name(key) for key in _core._servers if _key_scope(key) != scope} - profile_servers = _config._load_mcp_config() if omitted else {} - # Resolving what this profile would connect with does PATH lookups, secret-scope reads and # live-endpoint probes: do it only for names that can be compared across profiles, once each, # before taking the global registry lock. A foreign key that appears after the snapshot has - # no digest here and is refused until the next pass. - judged = {**{name: profile_servers.get(name) for name in omitted}, **servers} - resolved_ids = {name: _adopter_identity_digest(name, config) for name, config in judged.items() - if name in foreign and config is not None and mcp_server_enabled(config)} + # no digest here and is refused until the next pass. A routed profile reconciles after + # ``discover_mcp_tools`` released its temporary owner scope (#113746), so the config load and + # the resolution bind THIS profile's own secret scope; unscoped, a source-tagged secret read + # would refuse a share whose values are equal. + from tools.mcp_tool_discovery import _owner_secret_scope + with _owner_secret_scope(): + profile_servers = _config._load_mcp_config() if omitted else {} + judged = {**{name: profile_servers.get(name) for name in omitted}, **servers} + resolved_ids = {name: _adopter_identity_digest(name, config) + for name, config in judged.items() + if name in foreign and config is not None and mcp_server_enabled(config)} with _core._lock: stale = []