From af3ce9816b68b7d12c5dc4d7f75f872c28ade83c Mon Sep 17 00:00:00 2001 From: qbit-mirror-bot Date: Thu, 2 Jul 2026 09:19:24 +0000 Subject: [PATCH] fix(tools): don't drop a toolset from platform inference when a tool is registered into it (salvage #56480) --- hermes_cli/tools_config.py | 18 +++++-- scripts/release.py | 3 +- tests/gateway/test_api_server_toolset.py | 52 ++++++++++++++++++++ tests/test_toolsets.py | 38 +++++++++++++++ toolsets.py | 60 ++++++++++++++++++------ 5 files changed, 151 insertions(+), 20 deletions(-) diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 404796322a851..eba4e4c0b6741 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -1471,7 +1471,11 @@ def _get_platform_tools( for ts_key, _, _ in CONFIGURABLE_TOOLSETS: if not _toolset_allowed_for_platform(ts_key, platform): continue - ts_tools = set(resolve_toolset(ts_key)) + # Compare the toolset's STATIC membership: a tool registered + # into a toolset (e.g. delegate_cli -> delegation, desktop-only + # read_terminal -> terminal) that the composite never listed must + # not drop the whole toolset. See issue #49622. + ts_tools = set(resolve_toolset(ts_key, include_registry=False)) if ts_tools and ts_tools.issubset(composite_tools): expanded.add(ts_key) @@ -1494,7 +1498,12 @@ def _get_platform_tools( for ts_key, _, _ in CONFIGURABLE_TOOLSETS: if not _toolset_allowed_for_platform(ts_key, platform): continue - ts_tools = set(resolve_toolset(ts_key)) + # Compare the toolset's STATIC membership against the composite (see + # issue #49622): get_toolset() merges registry-registered tools into + # a toolset, but platform composites enumerate static tool names, so + # an all-tools subset test against the merged set drops the whole + # toolset the moment a plugin/overlay/desktop tool joins it. + ts_tools = set(resolve_toolset(ts_key, include_registry=False)) if ts_tools and ts_tools.issubset(all_tool_names): enabled_toolsets.add(ts_key) @@ -1566,7 +1575,10 @@ def _get_platform_tools( # by agent/coding_context.py — not per-platform capabilities to recover. if ts_def.get("posture"): continue - ts_tools = set(resolve_toolset(ts_key)) + # Static membership (see #49622): a registry-added tool absent from the + # platform composite must not block recovery of a non-configurable + # toolset whose authored tools the composite does list. + ts_tools = set(resolve_toolset(ts_key, include_registry=False)) if not ts_tools or not ts_tools.issubset(platform_tool_universe): continue if ts_tools.issubset(configurable_tool_universe): diff --git a/scripts/release.py b/scripts/release.py index 7c7c90371f956..5e6b801164b89 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -45,8 +45,6 @@ # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { - "jvsantos.cunha@gmail.com": "plcunha", # PR #55300 salvage (gateway: record child gateway peer metadata after a compression session-id rotation and repoint stale sessions.json compression-parent entries to the recovered live child; consolidated in the compression-routing-integrity salvage) - "jakepresent1@gmail.com": "jakepresent", # PR #55721 salvage (gateway: identity-guard stale in-flight compression splits — a late run may publish its compressed child only if its run generation is still current and the session key still points at the run's original parent, so an old run can't overwrite a newer /new or moved binding) "zhangml@tech.icbc.com.cn": "zmlgit", # PR #54872 salvage (multiplex-profile kanban: route task notifications via the owning profile's adapter + wake the creator agent with a synthetic internal MessageEvent on terminal events) "1079826437@qq.com": "nankingjing", # PR #56404 salvage (gateway: while a state.db compression lock is held for the session, demote busy_input_mode 'interrupt' to 'queue' so a rapid message burst can't interrupt and fork orphaned compression siblings off a stale parent; #56391) "ud@arubangles.com": "udatny", # PR #29433 salvage (subdirectory_hints: catch RuntimeError from Path.expanduser()/Path.home() so a literal ~ in tool-call args — e.g. LLM "~500-700" or ~unknownuser — can't escape the hint walker and crash the conversation loop) @@ -1836,6 +1834,7 @@ "max.petrusenko.agent@gmail.com": "maxpetrusenko", # PR #54128 co-author "poli.koltsova@gmail.com": "wnuuee1", # commit 9fd2b2cb PR author "yosapol@jitrak.dev": "Eji4h", # direct email match + "kiljadn@gmail.com": "designnotdrum", # PR #56480 salvage (toolset static-inference fix) } diff --git a/tests/gateway/test_api_server_toolset.py b/tests/gateway/test_api_server_toolset.py index add2ce27345a8..5940ee8c2f392 100644 --- a/tests/gateway/test_api_server_toolset.py +++ b/tests/gateway/test_api_server_toolset.py @@ -63,6 +63,58 @@ def test_platforms_dict_includes_api_server(self): assert "api_server" in PLATFORMS assert PLATFORMS["api_server"]["default_toolset"] == "hermes-api-server" + def test_default_api_server_includes_terminal_toolset(self): + """Regression #49622: desktop-only read_terminal is registered into the + 'terminal' toolset (ships in-repo), so resolve_toolset('terminal') grows + to include it after discovery. read_terminal is NOT in the + hermes-api-server composite, so the old all-tools subset test dropped + 'terminal' entirely. Its static membership (terminal, process) IS in the + composite, so it must stay enabled.""" + from tools.registry import discover_builtin_tools + from hermes_cli.tools_config import _get_platform_tools + discover_builtin_tools() + assert "terminal" in _get_platform_tools({}, "api_server") + + def test_registering_tool_into_toolset_does_not_drop_toolset_from_inference(self): + """Class invariant (covers the delegate_cli overlay case): registering a + NEW tool into an existing configurable toolset must never remove that + toolset from a platform whose composite lists the toolset's static + tools. Synthetic registration keeps the test hermetic in CI.""" + from tools.registry import registry + from hermes_cli.tools_config import _get_platform_tools + + sentinel = "test_sentinel_delegation_tool" + registry.register( + name=sentinel, + toolset="delegation", + schema={"name": sentinel, "description": "test", + "parameters": {"type": "object", "properties": {}}}, + handler=lambda args, **kw: "{}", + ) + try: + # delegation's static membership (delegate_task) is in the composite, + # so the toolset must survive inference despite the extra registry tool. + assert "delegation" in _get_platform_tools({}, "api_server"), ( + "registering a tool into 'delegation' dropped it from api_server" + ) + finally: + registry.deregister(sentinel) + + def test_default_off_and_restricted_toolsets_stay_off_on_api_server(self): + """Negative contract: the static-membership comparison must NOT newly + enable default-off or platform-restricted toolsets.""" + import os + from unittest.mock import patch + from hermes_cli.tools_config import _get_platform_tools + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("HASS_TOKEN", None) + os.environ.pop("XAI_API_KEY", None) + enabled = _get_platform_tools({}, "api_server") + assert "homeassistant" not in enabled + assert "discord" not in enabled + assert "discord_admin" not in enabled + assert "x_search" not in enabled + class TestApiServerAdapterToolset: @patch("gateway.platforms.api_server.AIOHTTP_AVAILABLE", True) diff --git a/tests/test_toolsets.py b/tests/test_toolsets.py index 1773d281af97c..f9e4969b7beaa 100644 --- a/tests/test_toolsets.py +++ b/tests/test_toolsets.py @@ -253,3 +253,41 @@ def test_hermes_whatsapp_toolset_includes_web_search(self): def test_hermes_api_server_toolset_includes_web_search(self): assert "web_search" in resolve_toolset("hermes-api-server") + + +class TestResolveToolsetIncludeRegistry: + """include_registry flag exposes the static (pre-registry-merge) view used + by platform reverse-mapping. Regression harness for issue #49622.""" + + def test_include_registry_false_excludes_registry_tools(self): + from tools.registry import discover_builtin_tools + discover_builtin_tools() # registers read_terminal into 'terminal' + + merged = set(resolve_toolset("terminal")) + static = set(resolve_toolset("terminal", include_registry=False)) + + assert static == {"terminal", "process"}, static + # read_terminal is registered into 'terminal' but is desktop-only and + # not part of the static definition — it must only appear in the merged view. + assert "read_terminal" in merged + assert "read_terminal" not in static + + def test_get_toolset_include_registry_false_is_static(self): + ts = get_toolset("delegation", include_registry=False) + assert ts is not None + assert ts["tools"] == ["delegate_task"] + + def test_static_view_threads_through_includes(self): + # 'debugging' has direct tools [terminal, process] and includes [web, file] + static = set(resolve_toolset("debugging", include_registry=False)) + assert {"terminal", "process"} <= static + assert "web_search" in static + assert "read_file" in static + + def test_all_alias_accepts_include_registry(self): + merged = set(resolve_toolset("all")) + static = set(resolve_toolset("all", include_registry=False)) + assert static <= merged + + def test_registry_only_toolset_static_view_is_empty(self): + assert resolve_toolset("__definitely_not_a_real_toolset__", include_registry=False) == [] diff --git a/toolsets.py b/toolsets.py index 083ab9d89138f..03e64fdba4c01 100644 --- a/toolsets.py +++ b/toolsets.py @@ -583,19 +583,41 @@ -def get_toolset(name: str) -> Optional[Dict[str, Any]]: +def get_toolset(name: str, *, include_registry: bool = True) -> Optional[Dict[str, Any]]: """ Get a toolset definition by name. - + Args: name (str): Name of the toolset - + include_registry (bool): When True (default), merge in tools that + plugins/overlays registered into this toolset via the registry. + When False, return only the static ``TOOLSETS`` definition (the + composite-authored view). Platform reverse-mapping in + ``_get_platform_tools`` uses False so that a tool registered into a + toolset but absent from a platform's static composite does not drop + the whole toolset from inference. See issue #49622. + Returns: Dict: Toolset definition with description, tools, and includes - None: If toolset not found + None: If toolset not found. With include_registry=False the static + view only recognizes names literally present in ``TOOLSETS``, so + registry/MCP-only toolsets AND registry-derived aliases return None + (they have no static counterpart). """ toolset = TOOLSETS.get(name) + if not include_registry: + # Static view only: return the built-in definition (copying the nested + # tools/includes lists so callers can't mutate TOOLSETS), or None for + # registry/MCP-only toolsets that have no static counterpart. + if not toolset: + return None + return { + **toolset, + "tools": list(toolset.get("tools", [])), + "includes": list(toolset.get("includes", [])), + } + try: from tools.registry import registry except Exception: @@ -662,30 +684,36 @@ def bundle_non_core_tools(toolset_name: str) -> Set[str]: return to_remove -def resolve_toolset(name: str, visited: Set[str] = None) -> List[str]: +def resolve_toolset(name: str, visited: Set[str] = None, *, include_registry: bool = True) -> List[str]: """ Recursively resolve a toolset to get all tool names. - + This function handles toolset composition by recursively resolving included toolsets and combining all tools. - + Args: name (str): Name of the toolset to resolve visited (Set[str]): Set of already visited toolsets (for cycle detection) - + include_registry (bool): When True (default), include tools that + plugins/overlays registered into a toolset. When False, resolve only + the static ``TOOLSETS`` definition (includes are still resolved, but + statically). Platform reverse-mapping uses False so a registry-added + tool cannot drop the whole toolset from inference (see #49622 and + ``_get_platform_tools``). + Returns: List[str]: List of all tool names in the toolset """ if visited is None: visited = set() - + # Special aliases that represent all tools across every toolset # This ensures future toolsets are automatically included without changes. if name in {"all", "*"}: all_tools: Set[str] = set() for toolset_name in get_toolset_names(): # Use a fresh visited set per branch to avoid cross-branch contamination - resolved = resolve_toolset(toolset_name, visited.copy()) + resolved = resolve_toolset(toolset_name, visited.copy(), include_registry=include_registry) all_tools.update(resolved) return sorted(all_tools) @@ -698,12 +726,14 @@ def resolve_toolset(name: str, visited: Set[str] = None) -> List[str]: visited.add(name) # Get toolset definition - toolset = get_toolset(name) + toolset = get_toolset(name, include_registry=include_registry) if not toolset: # Auto-generate a toolset for plugin platforms (hermes-). # Gives them _HERMES_CORE_TOOLS plus any tools the plugin registered - # into a toolset matching the platform name. - if name.startswith("hermes-"): + # into a toolset matching the platform name. This is a registry-derived + # view, so it only applies when registry tools are requested; the static + # view (include_registry=False) has no plugin-platform definition. + if include_registry and name.startswith("hermes-"): platform_name = name[len("hermes-"):] try: from gateway.platform_registry import platform_registry @@ -730,9 +760,9 @@ def resolve_toolset(name: str, visited: Set[str] = None) -> List[str]: # sibling includes so diamond dependencies are only resolved once and # cycle warnings don't fire multiple times for the same cycle. for included_name in toolset.get("includes", []): - included_tools = resolve_toolset(included_name, visited) + included_tools = resolve_toolset(included_name, visited, include_registry=include_registry) tools.update(included_tools) - + return sorted(tools)