From b70dd51cfab01ec7e61ef8db730319d8df621f86 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 18 Mar 2026 03:17:37 -0700 Subject: [PATCH] fix: disabled skills respected across banner, system prompt, slash commands, and skill_view (#1897) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: banner skill count now respects disabled skills and platform filtering The banner's get_available_skills() was doing a raw rglob scan of ~/.hermes/skills/ without checking: - Whether skills are disabled (skills.disabled config) - Whether skills match the current platform (platforms: frontmatter) This caused the banner to show inflated skill counts (e.g. '100 skills' when many are disabled) and list macOS-only skills on Linux. Fix: delegate to _find_all_skills() from tools/skills_tool which already handles both platform gating and disabled-skill filtering. * fix: system prompt and slash commands now respect disabled skills Two more places where disabled skills were still surfaced: 1. build_skills_system_prompt() in prompt_builder.py — disabled skills appeared in the system prompt section, causing the agent to suggest/load them despite being disabled. 2. scan_skill_commands() in skill_commands.py — disabled skills still registered as /skill-name slash commands in CLI help and could be invoked. Both now load _get_disabled_skill_names() and filter accordingly. * fix: skill_view blocks disabled skills skill_view() checked platform compatibility but not disabled state, so the agent could still load and read disabled skills directly. Now returns a clear error when a disabled skill is requested, telling the user to enable it via hermes skills or inspect the files manually. --------- Co-authored-by: Test --- agent/prompt_builder.py | 26 ++++++---- agent/skill_commands.py | 6 ++- hermes_cli/banner.py | 33 ++++++------- tests/agent/test_prompt_builder.py | 29 +++++++++++ tests/agent/test_skill_commands.py | 15 ++++++ tests/hermes_cli/test_banner_skills.py | 68 ++++++++++++++++++++++++++ tests/tools/test_skills_tool.py | 29 +++++++++++ tools/skills_tool.py | 14 ++++++ 8 files changed, 190 insertions(+), 30 deletions(-) create mode 100644 tests/hermes_cli/test_banner_skills.py diff --git a/agent/prompt_builder.py b/agent/prompt_builder.py index 4ce84473f..8dc3124ba 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -330,28 +330,34 @@ def build_skills_system_prompt( # Each entry: (skill_name, description) # Supports sub-categories: skills/mlops/training/axolotl/SKILL.md # -> category "mlops/training", skill "axolotl" + # Load disabled skill names once for the entire scan + try: + from tools.skills_tool import _get_disabled_skill_names + disabled = _get_disabled_skill_names() + except Exception: + disabled = set() + skills_by_category: dict[str, list[tuple[str, str]]] = {} for skill_file in skills_dir.rglob("SKILL.md"): - is_compatible, _, desc = _parse_skill_file(skill_file) + is_compatible, frontmatter, desc = _parse_skill_file(skill_file) if not is_compatible: continue - # Skip skills whose conditional activation rules exclude them - conditions = _read_skill_conditions(skill_file) - if not _skill_should_show(conditions, available_tools, available_toolsets): - continue rel_path = skill_file.relative_to(skills_dir) parts = rel_path.parts if len(parts) >= 2: - # Category is everything between skills_dir and the skill folder - # e.g. parts = ("mlops", "training", "axolotl", "SKILL.md") - # → category = "mlops/training", skill_name = "axolotl" - # e.g. parts = ("github", "github-auth", "SKILL.md") - # → category = "github", skill_name = "github-auth" skill_name = parts[-2] category = "/".join(parts[:-2]) if len(parts) > 2 else parts[0] else: category = "general" skill_name = skill_file.parent.name + # Respect user's disabled skills config + fm_name = frontmatter.get("name", skill_name) + if fm_name in disabled or skill_name in disabled: + continue + # Skip skills whose conditional activation rules exclude them + conditions = _read_skill_conditions(skill_file) + if not _skill_should_show(conditions, available_tools, available_toolsets): + continue skills_by_category.setdefault(category, []).append((skill_name, desc)) if not skills_by_category: diff --git a/agent/skill_commands.py b/agent/skill_commands.py index 67315ee8d..b266ad251 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -157,9 +157,10 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: global _skill_commands _skill_commands = {} try: - from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform + from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform, _get_disabled_skill_names if not SKILLS_DIR.exists(): return _skill_commands + disabled = _get_disabled_skill_names() for skill_md in SKILLS_DIR.rglob("SKILL.md"): if any(part in ('.git', '.github', '.hub') for part in skill_md.parts): continue @@ -170,6 +171,9 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: if not skill_matches_platform(frontmatter): continue name = frontmatter.get('name', skill_md.parent.name) + # Respect user's disabled skills config + if name in disabled: + continue description = frontmatter.get('description', '') if not description: for line in body.strip().split('\n'): diff --git a/hermes_cli/banner.py b/hermes_cli/banner.py index c1a1d4c77..addcf98c2 100644 --- a/hermes_cli/banner.py +++ b/hermes_cli/banner.py @@ -102,27 +102,22 @@ COMPACT_BANNER = """ # ========================================================================= def get_available_skills() -> Dict[str, List[str]]: - """Scan ~/.hermes/skills/ and return skills grouped by category.""" - import os + """Return skills grouped by category, filtered by platform and disabled state. - hermes_home = Path(os.getenv("HERMES_HOME", Path.home() / ".hermes")) - skills_dir = hermes_home / "skills" - skills_by_category = {} - - if not skills_dir.exists(): - return skills_by_category - - for skill_file in skills_dir.rglob("SKILL.md"): - rel_path = skill_file.relative_to(skills_dir) - parts = rel_path.parts - if len(parts) >= 2: - category = parts[0] - skill_name = parts[-2] - else: - category = "general" - skill_name = skill_file.parent.name - skills_by_category.setdefault(category, []).append(skill_name) + Delegates to ``_find_all_skills()`` from ``tools/skills_tool`` which already + handles platform gating (``platforms:`` frontmatter) and respects the + user's ``skills.disabled`` config list. + """ + try: + from tools.skills_tool import _find_all_skills + all_skills = _find_all_skills() # already filtered + except Exception: + return {} + skills_by_category: Dict[str, List[str]] = {} + for skill in all_skills: + category = skill.get("category") or "general" + skills_by_category.setdefault(category, []).append(skill["name"]) return skills_by_category diff --git a/tests/agent/test_prompt_builder.py b/tests/agent/test_prompt_builder.py index 1de37efbe..07c8da189 100644 --- a/tests/agent/test_prompt_builder.py +++ b/tests/agent/test_prompt_builder.py @@ -309,6 +309,35 @@ class TestBuildSkillsSystemPrompt: assert "imessage" in result assert "Send iMessages" in result + def test_excludes_disabled_skills(self, monkeypatch, tmp_path): + """Skills in the user's disabled list should not appear in the system prompt.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + skills_dir = tmp_path / "skills" / "tools" + skills_dir.mkdir(parents=True) + + enabled_skill = skills_dir / "web-search" + enabled_skill.mkdir() + (enabled_skill / "SKILL.md").write_text( + "---\nname: web-search\ndescription: Search the web\n---\n" + ) + + disabled_skill = skills_dir / "old-tool" + disabled_skill.mkdir() + (disabled_skill / "SKILL.md").write_text( + "---\nname: old-tool\ndescription: Deprecated tool\n---\n" + ) + + from unittest.mock import patch + + with patch( + "tools.skills_tool._get_disabled_skill_names", + return_value={"old-tool"}, + ): + result = build_skills_system_prompt() + + assert "web-search" in result + assert "old-tool" not in result + 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) diff --git a/tests/agent/test_skill_commands.py b/tests/agent/test_skill_commands.py index c02446138..f6a114db6 100644 --- a/tests/agent/test_skill_commands.py +++ b/tests/agent/test_skill_commands.py @@ -85,6 +85,21 @@ class TestScanSkillCommands: result = scan_skill_commands() assert "/generic-tool" in result + def test_excludes_disabled_skills(self, tmp_path): + """Disabled skills should not register slash commands.""" + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch( + "tools.skills_tool._get_disabled_skill_names", + return_value={"disabled-skill"}, + ), + ): + _make_skill(tmp_path, "enabled-skill") + _make_skill(tmp_path, "disabled-skill") + result = scan_skill_commands() + assert "/enabled-skill" in result + assert "/disabled-skill" not in result + class TestBuildPreloadedSkillsPrompt: def test_builds_prompt_for_multiple_named_skills(self, tmp_path): diff --git a/tests/hermes_cli/test_banner_skills.py b/tests/hermes_cli/test_banner_skills.py new file mode 100644 index 000000000..1006fcc86 --- /dev/null +++ b/tests/hermes_cli/test_banner_skills.py @@ -0,0 +1,68 @@ +"""Tests for banner get_available_skills() — disabled and platform filtering.""" + +from unittest.mock import patch + +import pytest + + +_MOCK_SKILLS = [ + {"name": "skill-a", "description": "A skill", "category": "tools"}, + {"name": "skill-b", "description": "B skill", "category": "tools"}, + {"name": "skill-c", "description": "C skill", "category": "creative"}, +] + + +def test_get_available_skills_delegates_to_find_all_skills(): + """get_available_skills should call _find_all_skills (which handles filtering).""" + with patch("tools.skills_tool._find_all_skills", return_value=list(_MOCK_SKILLS)): + from hermes_cli.banner import get_available_skills + result = get_available_skills() + + assert "tools" in result + assert "creative" in result + assert sorted(result["tools"]) == ["skill-a", "skill-b"] + assert result["creative"] == ["skill-c"] + + +def test_get_available_skills_excludes_disabled(): + """Disabled skills should not appear in the banner count.""" + # _find_all_skills already filters disabled skills, so if we give it + # a filtered list, get_available_skills should reflect that. + filtered = [s for s in _MOCK_SKILLS if s["name"] != "skill-b"] + with patch("tools.skills_tool._find_all_skills", return_value=filtered): + from hermes_cli.banner import get_available_skills + result = get_available_skills() + + all_names = [n for names in result.values() for n in names] + assert "skill-b" not in all_names + assert "skill-a" in all_names + assert len(all_names) == 2 + + +def test_get_available_skills_empty_when_no_skills(): + """No skills installed returns empty dict.""" + with patch("tools.skills_tool._find_all_skills", return_value=[]): + from hermes_cli.banner import get_available_skills + result = get_available_skills() + + assert result == {} + + +def test_get_available_skills_handles_import_failure(): + """If _find_all_skills import fails, return empty dict gracefully.""" + with patch("tools.skills_tool._find_all_skills", side_effect=ImportError("boom")): + from hermes_cli.banner import get_available_skills + result = get_available_skills() + + assert result == {} + + +def test_get_available_skills_null_category_becomes_general(): + """Skills with None category should be grouped under 'general'.""" + skills = [{"name": "orphan-skill", "description": "No cat", "category": None}] + with patch("tools.skills_tool._find_all_skills", return_value=skills): + from hermes_cli.banner import get_available_skills + result = get_available_skills() + + assert "general" in result + assert result["general"] == ["orphan-skill"] diff --git a/tests/tools/test_skills_tool.py b/tests/tools/test_skills_tool.py index bd72228aa..6af2c83cb 100644 --- a/tests/tools/test_skills_tool.py +++ b/tests/tools/test_skills_tool.py @@ -374,6 +374,35 @@ class TestSkillView: result = json.loads(raw) assert result["success"] is False + def test_view_disabled_skill_blocked(self, tmp_path): + """Disabled skills should not be viewable via skill_view.""" + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch( + "tools.skills_tool._is_skill_disabled", + return_value=True, + ), + ): + _make_skill(tmp_path, "hidden-skill") + raw = skill_view("hidden-skill") + result = json.loads(raw) + assert result["success"] is False + assert "disabled" in result["error"].lower() + + def test_view_enabled_skill_allowed(self, tmp_path): + """Non-disabled skills should be viewable normally.""" + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch( + "tools.skills_tool._is_skill_disabled", + return_value=False, + ), + ): + _make_skill(tmp_path, "active-skill") + raw = skill_view("active-skill") + result = json.loads(raw) + assert result["success"] is True + class TestSkillViewSecureSetupOnLoad: def test_requests_missing_required_env_and_continues(self, tmp_path, monkeypatch): diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 771d7684f..bc31cff34 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -920,6 +920,20 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str: ensure_ascii=False, ) + # Check if the skill is disabled by the user + resolved_name = parsed_frontmatter.get("name", skill_md.parent.name) + if _is_skill_disabled(resolved_name): + return json.dumps( + { + "success": False, + "error": ( + f"Skill '{resolved_name}' is disabled. " + "Enable it with `hermes skills` or inspect the files directly on disk." + ), + }, + ensure_ascii=False, + ) + # If a specific file path is requested, read that instead if file_path and skill_dir: # Security: Prevent path traversal attacks