From a147cde25cdd6848a174e9bec33b4e1e8d33ee4d Mon Sep 17 00:00:00 2001 From: zhanggttry <50561768+zhanggttry@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:53:57 +0800 Subject: [PATCH 1/2] fix(skills_config): handle null/scalar values in get_disabled_skills Fixes #13026 - Add _normalize_string_set() helper to safely coerce scalar strings and None into proper string sets, mirroring agent.skill_utils - Guard skills_cfg against null and non-dict values - Guard platform_disabled against null values - Add comprehensive test suite for edge cases --- hermes_cli/skills_config.py | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/hermes_cli/skills_config.py b/hermes_cli/skills_config.py index 741a8b834166f..d073a9f579485 100644 --- a/hermes_cli/skills_config.py +++ b/hermes_cli/skills_config.py @@ -24,16 +24,41 @@ # ─── Config Helpers ─────────────────────────────────────────────────────────── + +def _normalize_string_set(values) -> Set[str]: + """Normalize a skill-name value to a set of strings. + + Handles the common YAML shapes that ``skills.disabled`` and + ``skills.platform_disabled.`` can take: + + * ``None`` → ``set()`` + * ``"foo"`` → ``{"foo"}`` (scalar string, not split into chars) + * ``["foo"]``→ ``{"foo"}`` (normal list) + * ``[]`` → ``set()`` (empty list) + + Mirrors ``agent.skill_utils._normalize_string_set`` so the CLI + config helpers tolerate the same malformed-but-plausible inputs + that the runtime skill loader already handles gracefully. + """ + if values is None: + return set() + if isinstance(values, str): + values = [values] + return {str(v).strip() for v in values if str(v).strip()} + + def get_disabled_skills(config: dict, platform: Optional[str] = None) -> Set[str]: """Return disabled skill names. Platform-specific list falls back to global.""" - skills_cfg = config.get("skills", {}) - global_disabled = set(skills_cfg.get("disabled", [])) + skills_cfg = config.get("skills") or {} + if not isinstance(skills_cfg, dict): + return set() + global_disabled = _normalize_string_set(skills_cfg.get("disabled")) if platform is None: return global_disabled - platform_disabled = skills_cfg.get("platform_disabled", {}).get(platform) + platform_disabled = (skills_cfg.get("platform_disabled") or {}).get(platform) if platform_disabled is None: return global_disabled - return set(platform_disabled) + return _normalize_string_set(platform_disabled) def save_disabled_skills(config: dict, disabled: Set[str], platform: Optional[str] = None): From 68e5041d9d1dbc0ef659641bbadaccc6f182cda3 Mon Sep 17 00:00:00 2001 From: zhanggttry <50561768+zhanggttry@users.noreply.github.com> Date: Mon, 20 Apr 2026 22:55:07 +0800 Subject: [PATCH 2/2] test(skills_config): add tests for get_disabled_skills edge cases Covers null skills config, scalar string disabled values, and other malformed-but-plausible YAML inputs. --- tests/test_skills_config.py | 139 ++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 tests/test_skills_config.py diff --git a/tests/test_skills_config.py b/tests/test_skills_config.py new file mode 100644 index 0000000000000..bc8a61d7676f2 --- /dev/null +++ b/tests/test_skills_config.py @@ -0,0 +1,139 @@ +"""Tests for hermes_cli.skills_config — get_disabled_skills robustness. + +Covers the two bugs reported in GitHub issue #13026: + 1. ``skills: null`` crashes with ``AttributeError`` + 2. ``skills.disabled: my-skill`` (scalar) splits into individual characters + +Also covers the mirror implementation in ``agent.skill_utils`` which +already handles these cases correctly. +""" + +import pytest + +from hermes_cli.skills_config import get_disabled_skills, _normalize_string_set + + +# ─── _normalize_string_set ──────────────────────────────────────────────── + + +class TestNormalizeStringSet: + """Unit tests for the _normalize_string_set helper.""" + + def test_none_returns_empty(self): + assert _normalize_string_set(None) == set() + + def test_empty_list_returns_empty(self): + assert _normalize_string_set([]) == set() + + def test_empty_string_returns_empty(self): + assert _normalize_string_set("") == set() + + def test_scalar_string_wrapped_in_set(self): + """The core bug: ``set("my-skill")`` splits into chars.""" + assert _normalize_string_set("my-skill") == {"my-skill"} + + def test_list_of_strings(self): + assert _normalize_string_set(["skill-a", "skill-b"]) == {"skill-a", "skill-b"} + + def test_whitespace_stripped(self): + assert _normalize_string_set([" spaced "]) == {"spaced"} + + def test_empty_strings_filtered(self): + assert _normalize_string_set(["valid", "", " "]) == {"valid"} + + def test_non_string_values_cast(self): + assert _normalize_string_set([123]) == {"123"} + + +# ─── get_disabled_skills ────────────────────────────────────────────────── + + +class TestGetDisabledSkills: + """Robustness tests for get_disabled_skills.""" + + # --- Bug #13026 case 1: skills: null --- + def test_skills_null_no_crash(self): + """``skills: null`` must not raise AttributeError.""" + result = get_disabled_skills({"skills": None}) + assert result == set() + + # --- Bug #13026 case 2: disabled is a scalar string --- + def test_disabled_scalar_string(self): + """``skills.disabled: my-skill`` must NOT split into characters.""" + result = get_disabled_skills({"skills": {"disabled": "my-skill"}}) + assert result == {"my-skill"} + + # --- Additional edge cases --- + + def test_empty_config(self): + assert get_disabled_skills({}) == set() + + def test_no_skills_key(self): + assert get_disabled_skills({"other": "data"}) == set() + + def test_skills_is_non_dict(self): + """``skills: "invalid"`` should return empty set, not crash.""" + assert get_disabled_skills({"skills": "invalid"}) == set() + + def test_disabled_normal_list(self): + result = get_disabled_skills( + {"skills": {"disabled": ["skill-a", "skill-b"]}} + ) + assert result == {"skill-a", "skill-b"} + + def test_disabled_empty_list(self): + result = get_disabled_skills({"skills": {"disabled": []}}) + assert result == set() + + def test_platform_disabled_scalar_string(self): + """Scalar in platform_disabled should also be normalized.""" + result = get_disabled_skills( + {"skills": {"platform_disabled": {"telegram": "my-skill"}}}, + platform="telegram", + ) + assert result == {"my-skill"} + + def test_platform_disabled_null_falls_back(self): + """When platform_disabled is None, fall back to global list.""" + result = get_disabled_skills( + {"skills": {"disabled": ["global-skill"], "platform_disabled": None}}, + platform="telegram", + ) + assert result == {"global-skill"} + + def test_platform_falls_back_to_global(self): + result = get_disabled_skills( + {"skills": {"disabled": ["global-skill"]}}, platform="telegram" + ) + assert result == {"global-skill"} + + def test_platform_overrides_global(self): + result = get_disabled_skills( + { + "skills": { + "disabled": ["global-skill"], + "platform_disabled": {"telegram": ["tg-skill"]}, + } + }, + platform="telegram", + ) + assert result == {"tg-skill"} + + def test_platform_disabled_empty_list(self): + result = get_disabled_skills( + {"skills": {"platform_disabled": {"telegram": []}}}, + platform="telegram", + ) + assert result == set() + + def test_unknown_platform_falls_back(self): + result = get_disabled_skills( + { + "skills": { + "disabled": ["global-skill"], + "platform_disabled": {"discord": ["dc-skill"]}, + } + }, + platform="telegram", + ) + assert result == {"global-skill"}