From 309cf2c5e2b3327bf302f8d3c8de240a70604221 Mon Sep 17 00:00:00 2001 From: Merge_Conflict - Pasi Date: Sat, 15 Aug 2026 09:23:30 +0000 Subject: [PATCH] fix: honor JSON-array string forms for skills.disabled and agent.disabled_toolsets `hermes config set` and JSON-mode editor saves store lists as quoted strings (e.g. '["skill-a","skill-b"]' or "['memory']"). Both disable filters treated such a string as a single name, so curated disable lists silently filtered nothing with zero diagnostics. Add parse_config_string_list() in agent.skill_utils and use it in _normalize_string_set (skills.disabled / platform_disabled) and at every agent.disabled_toolsets read site: tools_config resolve + reconcile, CLI, gateway agent construction (both sites), cron scheduler, and prompt_size. A scalar string still names a single entry (#13026); malformed JSON falls back to the single-name behavior instead of raising. Fixes #86661 --- agent/skill_utils.py | 33 ++++++++++-- cli.py | 4 +- cron/scheduler.py | 4 +- gateway/run.py | 8 ++- hermes_cli/prompt_size.py | 4 +- hermes_cli/tools_config.py | 20 +++++--- tests/agent/test_skill_utils.py | 74 +++++++++++++++++++++++++++ tests/hermes_cli/test_tools_config.py | 30 +++++++++++ 8 files changed, 161 insertions(+), 16 deletions(-) diff --git a/agent/skill_utils.py b/agent/skill_utils.py index 0454f3f7e9..a07615077b 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -5,6 +5,7 @@ heavy dependency chain. It is safe to import at module level without triggering tool registration or provider resolution. """ +import ast import logging import os import re @@ -471,12 +472,34 @@ def get_disabled_skill_names(platform: str | None = None) -> Set[str]: return global_disabled +def parse_config_string_list(value) -> List[str]: + """Normalize a config value that may hold a JSON-array string into a list. + + ``hermes config set`` and JSON-mode editor saves store lists as quoted + JSON strings (``'["a","b"]'`` or the Python-literal ``"['a']"``). Treating + such a string as a single name makes a curated disabled list silently + filter nothing (#86661); parsing it restores the intended list. A scalar + string still means one name (#13026). + """ + if value is None: + return [] + if isinstance(value, str): + stripped = value.strip() + if stripped.startswith("["): + try: + parsed = ast.literal_eval(stripped) + except (ValueError, SyntaxError): + parsed = None + if isinstance(parsed, list): + return [str(item) for item in parsed] + return [value] + if isinstance(value, (list, tuple, set, frozenset)): + return [str(item) for item in value] + return [] + + def _normalize_string_set(values) -> Set[str]: - if values is None: - return set() - if isinstance(values, str): - values = [values] - return {str(v).strip() for v in values if str(v).strip()} + return {name.strip() for name in parse_config_string_list(values) if name.strip()} # ── External skills directories ────────────────────────────────────────── diff --git a/cli.py b/cli.py index 370afec4bb..5b737212d5 100644 --- a/cli.py +++ b/cli.py @@ -4933,7 +4933,9 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): # Parse and validate toolsets self.enabled_toolsets = toolsets - self.disabled_toolsets = CLI_CONFIG["agent"].get("disabled_toolsets") or [] + from agent.skill_utils import parse_config_string_list + + self.disabled_toolsets = parse_config_string_list(CLI_CONFIG["agent"].get("disabled_toolsets")) if toolsets and "all" not in toolsets and "*" not in toolsets: # Validate each toolset — MCP server names are resolved via diff --git a/cron/scheduler.py b/cron/scheduler.py index e7816502de..6598f0e102 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -338,7 +338,9 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: else: disabled = ["cronjob", "messaging", "clarify", "memory"] agent_cfg = (cfg or {}).get("agent") or {} - user_disabled = agent_cfg.get("disabled_toolsets") or [] + from agent.skill_utils import parse_config_string_list + + user_disabled = parse_config_string_list(agent_cfg.get("disabled_toolsets")) for name in user_disabled: name = str(name).strip() if name and name not in disabled: diff --git a/gateway/run.py b/gateway/run.py index f81e364aad..6c187fe9cb 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -21935,7 +21935,9 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew user_config, source, platform_key ) agent_cfg = user_config.get("agent") or {} - disabled_toolsets = agent_cfg.get("disabled_toolsets") or None + from agent.skill_utils import parse_config_string_list + + disabled_toolsets = parse_config_string_list(agent_cfg.get("disabled_toolsets")) or None pr = self._provider_routing max_iterations = _current_max_iterations() @@ -27461,7 +27463,9 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew user_config, source, platform_key ) agent_cfg_local = user_config.get("agent") or {} - disabled_toolsets = agent_cfg_local.get("disabled_toolsets") or None + from agent.skill_utils import parse_config_string_list + + disabled_toolsets = parse_config_string_list(agent_cfg_local.get("disabled_toolsets")) or None display_config = user_config.get("display", {}) if not isinstance(display_config, dict): diff --git a/hermes_cli/prompt_size.py b/hermes_cli/prompt_size.py index 0629b9346d..aac473db3d 100644 --- a/hermes_cli/prompt_size.py +++ b/hermes_cli/prompt_size.py @@ -65,7 +65,9 @@ def _build_inspection_agent(platform: str) -> Any: # Resolve platform-specific toolsets the same way the gateway does. enabled_toolsets = sorted(_get_platform_tools(cfg, platform)) agent_cfg = cfg.get("agent") or {} - disabled_toolsets = agent_cfg.get("disabled_toolsets") or None + from agent.skill_utils import parse_config_string_list + + disabled_toolsets = parse_config_string_list(agent_cfg.get("disabled_toolsets")) or None return AIAgent( model=model, diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 2ee458ff18..530c8b7b37 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -2551,11 +2551,17 @@ def _get_platform_tools( # Honor agent.disabled_toolsets from config.yaml — allows users to # globally suppress specific toolsets (e.g. "memory") across all # platforms without per-platform toolset configuration. This runs - # last so it overrides everything above. + # last so it overrides everything above. The value may arrive as a + # JSON-array string (e.g. "['memory']") from `hermes config set` or a + # JSON-mode editor save; parse it so the list is not silently dead (#86661). agent_cfg = config.get("agent") or {} disabled_toolsets = agent_cfg.get("disabled_toolsets") or [] if disabled_toolsets: - disabled_set = {str(ts) for ts in disabled_toolsets} + from agent.skill_utils import parse_config_string_list + + disabled_set = { + name.strip() for name in parse_config_string_list(disabled_toolsets) if name.strip() + } enabled_toolsets -= disabled_set # #38798: if this platform was explicitly configured but every toolset name @@ -2669,14 +2675,16 @@ def _save_platform_tools(config: dict, platform: str, enabled_toolset_keys: Set[ agent_cfg = config.get("agent") if isinstance(agent_cfg, dict): disabled_toolsets = agent_cfg.get("disabled_toolsets") - if isinstance(disabled_toolsets, list) and disabled_toolsets: + if disabled_toolsets: + from agent.skill_utils import parse_config_string_list + + parsed_disabled = parse_config_string_list(disabled_toolsets) newly_enabled = enabled_toolset_keys - preserved_entries if newly_enabled: remaining = [ - ts for ts in disabled_toolsets - if str(ts) not in newly_enabled + ts for ts in parsed_disabled if ts not in newly_enabled ] - if remaining != disabled_toolsets: + if remaining != parsed_disabled: agent_cfg["disabled_toolsets"] = remaining save_config(config) diff --git a/tests/agent/test_skill_utils.py b/tests/agent/test_skill_utils.py index a1a71e093b..26211ba604 100644 --- a/tests/agent/test_skill_utils.py +++ b/tests/agent/test_skill_utils.py @@ -13,6 +13,7 @@ from agent.skill_utils import ( is_external_skill_path, is_skill_support_path, iter_skill_index_files, + parse_config_string_list, parse_frontmatter, resolve_skill_config_values, skill_matches_platform, @@ -73,6 +74,79 @@ skills: assert parse_count == 1 +class TestParseConfigStringList: + """#86661: `hermes config set` and JSON-mode editor saves store lists as + quoted strings (e.g. '["a","b"]'). Treating such a string as a single name + made curated disabled lists silently filter nothing.""" + + def test_json_array_string_parses(self): + assert parse_config_string_list('["skill-a","skill-b"]') == [ + "skill-a", + "skill-b", + ] + + def test_python_literal_array_string_parses(self): + # `hermes config set` can persist single-quoted Python-literal forms. + assert parse_config_string_list("['skill-a']") == ["skill-a"] + + def test_scalar_string_means_one_name(self): + # #13026: a scalar string still names a single entry. + assert parse_config_string_list("skill-a") == ["skill-a"] + + def test_real_list_passes_through(self): + assert parse_config_string_list(["skill-a", "skill-b"]) == [ + "skill-a", + "skill-b", + ] + assert parse_config_string_list(("skill-a",)) == ["skill-a"] + + def test_none_returns_empty(self): + assert parse_config_string_list(None) == [] + + def test_malformed_json_falls_back_to_single_name(self): + assert parse_config_string_list('["skill-a"') == ['["skill-a"'] + + def test_empty_array_string_returns_empty(self): + assert parse_config_string_list("[]") == [] + + +class TestDisabledSkillsJsonArrayString: + """The skills.disabled setting must honor a JSON-array string form, not + treat the whole string as one dead skill name (#86661).""" + + def test_get_disabled_skill_names_parses_json_array_string( + self, tmp_path, monkeypatch + ): + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text( + "skills:\n disabled: '[\"skill-a\",\"skill-b\"]'\n", + encoding="utf-8", + ) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + from agent import skill_utils + + getattr(skill_utils, "_raw_config_cache_clear", lambda: None)() + + assert get_disabled_skill_names() == {"skill-a", "skill-b"} + + def test_get_disabled_skill_names_scalar_string_still_single_name( + self, tmp_path, monkeypatch + ): + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text( + "skills:\n disabled: 'hidden-skill'\n", + encoding="utf-8", + ) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + from agent import skill_utils + + getattr(skill_utils, "_raw_config_cache_clear", lambda: None)() + + assert get_disabled_skill_names() == {"hidden-skill"} + + diff --git a/tests/hermes_cli/test_tools_config.py b/tests/hermes_cli/test_tools_config.py index b4618848c1..5d19c8ff3b 100644 --- a/tests/hermes_cli/test_tools_config.py +++ b/tests/hermes_cli/test_tools_config.py @@ -1053,6 +1053,36 @@ def test_agent_disabled_toolsets_still_wins(): assert not (_RECENTLY_SHIPPED_TOOLSETS & enabled) +@_requires_recently_shipped +def test_agent_disabled_toolsets_json_array_string_form_still_wins(): + """#86661: the suppression list may arrive as a JSON-array string (e.g. + `hermes config set agent.disabled_toolsets '["memory"]'`). It must be + parsed, not treated as one dead toolset name that filters nothing.""" + config = _saved_list_from_before() + import json as _json + + config["agent"] = { + "disabled_toolsets": _json.dumps(sorted(_RECENTLY_SHIPPED_TOOLSETS)) + } + + enabled = _get_platform_tools(config, "cli", include_default_mcp_servers=False) + + assert not (_RECENTLY_SHIPPED_TOOLSETS & enabled) + + +@_requires_recently_shipped +def test_agent_disabled_toolsets_python_literal_string_form_still_wins(): + """Single-quoted Python-literal form (as written by some config editors) + must resolve the same way as the JSON form.""" + config = _saved_list_from_before() + quoted = ", ".join(repr(ts) for ts in sorted(_RECENTLY_SHIPPED_TOOLSETS)) + config["agent"] = {"disabled_toolsets": f"[{quoted}]"} + + enabled = _get_platform_tools(config, "cli", include_default_mcp_servers=False) + + assert not (_RECENTLY_SHIPPED_TOOLSETS & enabled) + + @_requires_recently_shipped def test_platforms_whose_composite_excludes_it_are_left_narrow(): """Parity is the justification, so don't widen a deliberately small