From 20123b1268ba3a53d99f36516ceecf665ec292f7 Mon Sep 17 00:00:00 2001 From: webtecnica <75556242+webtecnica@users.noreply.github.com> Date: Tue, 18 Aug 2026 23:40:04 -0300 Subject: [PATCH] fix(skills): decode JSON-array string skills.disabled in panel read and toggle write (#7120) (#7134) --- api/routes.py | 44 ++++++++++++++- tests/test_issue3066_disabled_read_profile.py | 55 +++++++++++++++++++ tests/test_skills_toggle.py | 40 ++++++++++++++ 3 files changed, 137 insertions(+), 2 deletions(-) diff --git a/api/routes.py b/api/routes.py index b793a89c..94bbd81e 100644 --- a/api/routes.py +++ b/api/routes.py @@ -817,12 +817,52 @@ def _get_disabled_skill_names_for_profile() -> set: return _normalize_disabled_set(skills_cfg.get("disabled")) +def _parse_config_string_list(value) -> list: + """Decode 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']\"``), so a + disabled list read from ``config.yaml`` can arrive as a single string + instead of a YAML list. Treating it as one literal name makes the Skills + panel show every skill as enabled and makes the toggle write a destructive + single-entry list (hermes-webui#7120). + + Reuses ``agent.skill_utils.parse_config_string_list`` (hermes-agent #86661 + fix) when the bundled agent source is importable, and mirrors its logic + otherwise so the two surfaces cannot drift. A scalar string still means one + name. + """ + try: + from agent.skill_utils import parse_config_string_list + + return parse_config_string_list(value) + except ImportError: + pass + import ast + + 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_disabled_set(values) -> set: """Normalize a YAML disabled list into a set of stripped strings.""" if values is None: return set() if isinstance(values, str): - values = [values] + values = _parse_config_string_list(values) return {str(v).strip() for v in values if str(v).strip()} @@ -26841,7 +26881,7 @@ def _normalize_names_list(names) -> list[str]: if names is None: return [] if isinstance(names, str): - names = [names] + names = _parse_config_string_list(names) elif not isinstance(names, list): names = list(names) if names else [] return list(dict.fromkeys(str(d).strip() for d in names if str(d).strip())) diff --git a/tests/test_issue3066_disabled_read_profile.py b/tests/test_issue3066_disabled_read_profile.py index c32502a8..3d35a348 100644 --- a/tests/test_issue3066_disabled_read_profile.py +++ b/tests/test_issue3066_disabled_read_profile.py @@ -161,3 +161,58 @@ def test_normalize_disabled_set_handles_edge_cases(): assert _normalize_disabled_set(["a", "b"]) == {"a", "b"} assert _normalize_disabled_set([" spaced ", " "]) == {"spaced"} assert _normalize_disabled_set([]) == set() + + +def test_disabled_read_decodes_json_array_string(tmp_path, monkeypatch): + """Issue #7120: skills.disabled stored as a JSON-array string (the shape + produced by `hermes config set skills.disabled ...`) must decode to the + real names, not one literal entry.""" + from api import routes + + config_path = tmp_path / "config.yaml" + _write_config(config_path, {"skills": {"disabled": '["skill-x", "skill-y"]'}}) + monkeypatch.setattr("api.routes._get_config_path", lambda: config_path) + + result = routes._get_disabled_skill_names_for_profile() + assert result == {"skill-x", "skill-y"} + + +def test_disabled_read_platform_webui_decodes_json_array_string(tmp_path, monkeypatch): + """Issue #7120: platform_disabled.webui stored as a JSON-array string is + decoded the same way as the global disabled list.""" + from api import routes + + config_path = tmp_path / "config.yaml" + _write_config(config_path, { + "skills": { + "disabled": ["global-disabled"], + "platform_disabled": {"webui": '["webui-disabled-a", "webui-disabled-b"]'}, + } + }) + monkeypatch.setattr("api.routes._get_config_path", lambda: config_path) + + result = routes._get_disabled_skill_names_for_profile() + assert result == {"webui-disabled-a", "webui-disabled-b"} + assert "global-disabled" not in result + + +@requires_agent_modules +def test_skills_list_disabled_decodes_json_array_string(tmp_path, monkeypatch): + """Issue #7120: the Skills panel (skills list endpoint) must report both + skills as disabled when config.yaml stores disabled as a JSON-array string.""" + from api import routes + + skills_dir = tmp_path / "skills" + _write_skill(skills_dir, "skill-a") + _write_skill(skills_dir, "skill-b") + + config_path = tmp_path / "config.yaml" + _write_config(config_path, {"skills": {"disabled": '["skill-a", "skill-b"]'}}) + monkeypatch.setattr("api.routes._get_config_path", lambda: config_path) + monkeypatch.setattr("api.routes._active_skills_dir", lambda: skills_dir) + + result = routes._skills_list_from_dir(skills_dir) + skills = {s["name"]: s for s in result["skills"]} + + assert skills["skill-a"]["disabled"] is True + assert skills["skill-b"]["disabled"] is True diff --git a/tests/test_skills_toggle.py b/tests/test_skills_toggle.py index bace8d1f..80fb49b4 100644 --- a/tests/test_skills_toggle.py +++ b/tests/test_skills_toggle.py @@ -197,3 +197,43 @@ def test_platform_disabled_no_write_through_when_key_absent(tmp_path, monkeypatc assert "skill-b" in cfg_after["skills"]["disabled"] # platform_disabled was never created assert "platform_disabled" not in cfg_after.get("skills", {}) + + +def test_toggle_preserves_json_array_string_entries(tmp_path, monkeypatch): + """Issue #7120: when skills.disabled is stored as a JSON-array string, + a toggle must decode it before add/remove so the other entries survive + the write (previously the whole string was treated as one name and the + list was destroyed on the next save).""" + from unittest.mock import MagicMock + from api.routes import _handle_skill_toggle + from api.config import _load_yaml_config_file + + config_path = tmp_path / "config.yaml" + monkeypatch.setattr("api.routes._get_config_path", lambda: config_path) + + import yaml + + config = {"skills": {"disabled": '["skill-a", "skill-b"]'}} + config_path.parent.mkdir(parents=True, exist_ok=True) + config_path.write_text(yaml.dump(config), encoding="utf-8") + + fake_dir = tmp_path / "skills" / "skill-c" + fake_dir.mkdir(parents=True, exist_ok=True) + fake_md = fake_dir / "SKILL.md" + fake_md.write_text("---\nname: skill-c\n---\nC skill", encoding="utf-8") + monkeypatch.setattr( + "api.routes._find_skill_in_dirs", + lambda name, dirs: (fake_dir, fake_md), + ) + + handler = MagicMock() + + # Toggle skill-c OFF — must append without dropping skill-a/skill-b + _handle_skill_toggle(handler, {"name": "skill-c", "enabled": False}) + cfg_after = _load_yaml_config_file(config_path) + assert cfg_after["skills"]["disabled"] == ["skill-a", "skill-b", "skill-c"] + + # Toggle skill-a ON — must remove only skill-a, preserving skill-b + _handle_skill_toggle(handler, {"name": "skill-a", "enabled": True}) + cfg_after2 = _load_yaml_config_file(config_path) + assert cfg_after2["skills"]["disabled"] == ["skill-b", "skill-c"]