mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-15 09:21:36 +00:00
fix(skills): apply global|platform disabled union to all resolution sites
The platform-disabled fix landed only in agent.skill_utils.get_disabled_skill_names (the system-prompt path). Two sibling resolvers still used the old replace-not-union semantics, so the same skill could be hidden from the <available_skills> prompt yet reported enabled elsewhere: - hermes_cli/skills_config.get_disabled_skills (the 'hermes skills config' UI) returned only the platform list, so a globally-disabled skill showed as enabled (unchecked) on any platform with a platform_disabled entry. - tools/skills_tool._is_skill_disabled (gates whether skill_view loads a skill) ignored the global list when a platform list existed, so a globally-disabled skill could still be loaded on such a platform. Both now union the global list with the platform list, matching get_disabled_skill_names. An explicit empty platform list no longer re-enables a globally-disabled skill — global disables hold on every platform (#46201). Also: fix the now-stale get_disabled_skill_names docstring and drop a stray blank line. Regression tests added for both sites (proven to fail on the old replace semantics).
This commit is contained in:
parent
7bbe7024c2
commit
ce19fdb7ce
5 changed files with 47 additions and 11 deletions
|
|
@ -278,8 +278,10 @@ def get_disabled_skill_names(platform: str | None = None) -> Set[str]:
|
|||
Args:
|
||||
platform: Explicit platform name (e.g. ``"telegram"``). When
|
||||
*None*, resolves from ``HERMES_PLATFORM`` or
|
||||
``HERMES_SESSION_PLATFORM`` env vars. Falls back to the
|
||||
global disabled list when no platform is determined.
|
||||
``HERMES_SESSION_PLATFORM`` env vars. Returns the global
|
||||
disabled list, unioned with the platform-specific list when a
|
||||
platform is resolved (a globally-disabled skill stays disabled
|
||||
on every platform).
|
||||
|
||||
Reads the config file directly (no CLI config imports) to stay
|
||||
lightweight.
|
||||
|
|
|
|||
|
|
@ -25,7 +25,13 @@ PLATFORMS = {k: info.label for k, info in _PLATFORMS.items() if k != "api_server
|
|||
# ─── Config Helpers ───────────────────────────────────────────────────────────
|
||||
|
||||
def get_disabled_skills(config: dict, platform: Optional[str] = None) -> Set[str]:
|
||||
"""Return disabled skill names. Platform-specific list falls back to global."""
|
||||
"""Return disabled skill names: the global list unioned with the
|
||||
platform-specific list when a platform is given.
|
||||
|
||||
A globally-disabled skill stays disabled on every platform, so the
|
||||
platform list adds to the global list rather than replacing it. This
|
||||
mirrors ``agent.skill_utils.get_disabled_skill_names``.
|
||||
"""
|
||||
skills_cfg = config.get("skills", {})
|
||||
global_disabled = set(skills_cfg.get("disabled", []))
|
||||
if platform is None:
|
||||
|
|
@ -33,7 +39,7 @@ def get_disabled_skills(config: dict, platform: Optional[str] = None) -> Set[str
|
|||
platform_disabled = cfg_get(skills_cfg, "platform_disabled", platform)
|
||||
if platform_disabled is None:
|
||||
return global_disabled
|
||||
return set(platform_disabled)
|
||||
return global_disabled | set(platform_disabled)
|
||||
|
||||
|
||||
def save_disabled_skills(config: dict, disabled: Set[str], platform: Optional[str] = None):
|
||||
|
|
|
|||
|
|
@ -419,7 +419,6 @@ class TestBuildSkillsSystemPrompt:
|
|||
second = build_skills_system_prompt()
|
||||
assert "cached-skill" not in second
|
||||
|
||||
|
||||
def test_includes_setup_needed_skills(self, monkeypatch, tmp_path):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
monkeypatch.delenv("MISSING_API_KEY_XYZ", raising=False)
|
||||
|
|
|
|||
|
|
@ -22,7 +22,19 @@ class TestGetDisabledSkills:
|
|||
"disabled": ["skill-a"],
|
||||
"platform_disabled": {"telegram": ["skill-b"]}
|
||||
}}
|
||||
assert get_disabled_skills(config, platform="telegram") == {"skill-b"}
|
||||
# Union of global + platform: a globally-disabled skill stays disabled
|
||||
# on every platform, and the platform list adds to it.
|
||||
assert get_disabled_skills(config, platform="telegram") == {"skill-a", "skill-b"}
|
||||
|
||||
def test_platform_list_unions_with_global(self):
|
||||
from hermes_cli.skills_config import get_disabled_skills
|
||||
config = {"skills": {
|
||||
"disabled": ["global-skill"],
|
||||
"platform_disabled": {"telegram": []}
|
||||
}}
|
||||
# An explicit empty platform list does NOT re-enable a globally-disabled
|
||||
# skill (matches issue #46201 — global disables hold everywhere).
|
||||
assert get_disabled_skills(config, platform="telegram") == {"global-skill"}
|
||||
|
||||
def test_platform_falls_back_to_global(self):
|
||||
from hermes_cli.skills_config import get_disabled_skills
|
||||
|
|
@ -102,14 +114,27 @@ class TestIsSkillDisabled:
|
|||
assert _is_skill_disabled("tg-skill", platform="telegram") is True
|
||||
|
||||
@patch("hermes_cli.config.load_config")
|
||||
def test_platform_enabled_overrides_global(self, mock_load):
|
||||
def test_globally_disabled_stays_disabled_on_platform(self, mock_load):
|
||||
mock_load.return_value = {"skills": {
|
||||
"disabled": ["skill-a"],
|
||||
"platform_disabled": {"telegram": ["tg-skill"]}
|
||||
}}
|
||||
from tools.skills_tool import _is_skill_disabled
|
||||
# Union: a globally-disabled skill stays disabled on a platform that
|
||||
# has its own platform_disabled list (matches issue #46201).
|
||||
assert _is_skill_disabled("skill-a", platform="telegram") is True
|
||||
assert _is_skill_disabled("tg-skill", platform="telegram") is True
|
||||
|
||||
@patch("hermes_cli.config.load_config")
|
||||
def test_empty_platform_list_keeps_global_disabled(self, mock_load):
|
||||
mock_load.return_value = {"skills": {
|
||||
"disabled": ["skill-a"],
|
||||
"platform_disabled": {"telegram": []}
|
||||
}}
|
||||
from tools.skills_tool import _is_skill_disabled
|
||||
# telegram has explicit empty list -> skill-a is NOT disabled for telegram
|
||||
assert _is_skill_disabled("skill-a", platform="telegram") is False
|
||||
# An explicit empty platform list does NOT re-enable a globally-disabled
|
||||
# skill — global disables hold on every platform.
|
||||
assert _is_skill_disabled("skill-a", platform="telegram") is True
|
||||
|
||||
@patch("hermes_cli.config.load_config")
|
||||
def test_platform_falls_back_to_global(self, mock_load):
|
||||
|
|
|
|||
|
|
@ -583,11 +583,15 @@ def _is_skill_disabled(name: str, platform: str = None) -> bool:
|
|||
config = load_config()
|
||||
skills_cfg = config.get("skills", {})
|
||||
resolved_platform = platform or os.getenv("HERMES_PLATFORM") or _get_session_platform()
|
||||
global_disabled = skills_cfg.get("disabled", [])
|
||||
if resolved_platform:
|
||||
platform_disabled = cfg_get(skills_cfg, "platform_disabled", resolved_platform)
|
||||
if platform_disabled is not None:
|
||||
return name in platform_disabled
|
||||
return name in skills_cfg.get("disabled", [])
|
||||
# A globally-disabled skill stays disabled on every platform;
|
||||
# the platform list adds to it rather than replacing it. Keep
|
||||
# in sync with agent.skill_utils.get_disabled_skill_names.
|
||||
return name in platform_disabled or name in global_disabled
|
||||
return name in global_disabled
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue