refactor: extract shared curses checklist, fix skill discovery perf
Four cleanups to code merged today: 1. New hermes_cli/curses_ui.py — shared curses_checklist() used by both hermes tools and hermes skills. Eliminates ~140 lines of near-identical curses code (scrolling, key handling, color setup, numbered fallback). 2. Fix _find_all_skills() perf — was calling load_config() per skill (~100+ YAML parses). Now loads disabled set once via _get_disabled_skill_names() and does a set lookup. 3. Eliminate _list_all_skills_unfiltered() duplication — _find_all_skills() now accepts skip_disabled=True for the config UI, removing 30 lines of copy-pasted discovery logic from skills_config.py. 4. Fix fragile label round-trip in skills_command — was building label strings, passing to checklist, then mapping labels back to skill names (collision-prone). Now works with indices directly, like tools_config.
This commit is contained in:
parent
f1510ec33e
commit
4864a5684a
5 changed files with 257 additions and 319 deletions
|
|
@ -68,7 +68,7 @@ import os
|
|||
import re
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Dict, Any, List, Optional, Tuple
|
||||
from typing import Dict, Any, List, Optional, Set, Tuple
|
||||
|
||||
import yaml
|
||||
|
||||
|
|
@ -223,62 +223,80 @@ def _parse_tags(tags_value) -> List[str]:
|
|||
|
||||
|
||||
|
||||
def _is_skill_disabled(name: str, platform: str = None) -> bool:
|
||||
"""Check if a skill is disabled in config, globally or for a specific platform.
|
||||
def _get_disabled_skill_names() -> Set[str]:
|
||||
"""Load disabled skill names from config (once per call).
|
||||
|
||||
Platform is resolved from the ``platform`` argument, then the
|
||||
``HERMES_PLATFORM`` env var, then falls back to the global disabled list.
|
||||
Resolves platform from ``HERMES_PLATFORM`` env var, falls back to
|
||||
the global disabled list.
|
||||
"""
|
||||
import os
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
config = load_config()
|
||||
skills_cfg = config.get("skills", {})
|
||||
# Resolve platform
|
||||
resolved_platform = os.getenv("HERMES_PLATFORM")
|
||||
if resolved_platform:
|
||||
platform_disabled = skills_cfg.get("platform_disabled", {}).get(resolved_platform)
|
||||
if platform_disabled is not None:
|
||||
return set(platform_disabled)
|
||||
return set(skills_cfg.get("disabled", []))
|
||||
except Exception:
|
||||
return set()
|
||||
|
||||
|
||||
def _is_skill_disabled(name: str, platform: str = None) -> bool:
|
||||
"""Check if a skill is disabled in config."""
|
||||
import os
|
||||
try:
|
||||
from hermes_cli.config import load_config
|
||||
config = load_config()
|
||||
skills_cfg = config.get("skills", {})
|
||||
resolved_platform = platform or os.getenv("HERMES_PLATFORM")
|
||||
if resolved_platform:
|
||||
platform_disabled = skills_cfg.get("platform_disabled", {}).get(resolved_platform)
|
||||
if platform_disabled is not None:
|
||||
return name in platform_disabled
|
||||
# Fall back to global disabled list
|
||||
return name in skills_cfg.get("disabled", [])
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
def _find_all_skills() -> List[Dict[str, Any]]:
|
||||
"""
|
||||
Recursively find all skills in ~/.hermes/skills/.
|
||||
|
||||
Returns metadata for progressive disclosure (tier 1):
|
||||
- name, description, category
|
||||
|
||||
|
||||
def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]:
|
||||
"""Recursively find all skills in ~/.hermes/skills/.
|
||||
|
||||
Args:
|
||||
skip_disabled: If True, return ALL skills regardless of disabled
|
||||
state (used by ``hermes skills`` config UI). Default False
|
||||
filters out disabled skills.
|
||||
|
||||
Returns:
|
||||
List of skill metadata dicts
|
||||
List of skill metadata dicts (name, description, category).
|
||||
"""
|
||||
skills = []
|
||||
|
||||
|
||||
if not SKILLS_DIR.exists():
|
||||
return skills
|
||||
|
||||
|
||||
# Load disabled set once (not per-skill)
|
||||
disabled = set() if skip_disabled else _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
|
||||
|
||||
|
||||
skill_dir = skill_md.parent
|
||||
|
||||
|
||||
try:
|
||||
content = skill_md.read_text(encoding='utf-8')
|
||||
frontmatter, body = _parse_frontmatter(content)
|
||||
|
||||
# Skip skills incompatible with the current OS platform
|
||||
if not skill_matches_platform(frontmatter):
|
||||
continue
|
||||
|
||||
|
||||
name = frontmatter.get('name', skill_dir.name)[:MAX_NAME_LENGTH]
|
||||
# Skip disabled skills
|
||||
if _is_skill_disabled(name):
|
||||
if name in disabled:
|
||||
continue
|
||||
|
||||
|
||||
description = frontmatter.get('description', '')
|
||||
if not description:
|
||||
for line in body.strip().split('\n'):
|
||||
|
|
@ -286,25 +304,25 @@ def _find_all_skills() -> List[Dict[str, Any]]:
|
|||
if line and not line.startswith('#'):
|
||||
description = line
|
||||
break
|
||||
|
||||
|
||||
if len(description) > MAX_DESCRIPTION_LENGTH:
|
||||
description = description[:MAX_DESCRIPTION_LENGTH - 3] + "..."
|
||||
|
||||
|
||||
category = _get_category_from_path(skill_md)
|
||||
|
||||
|
||||
skills.append({
|
||||
"name": name,
|
||||
"description": description,
|
||||
"category": category,
|
||||
})
|
||||
|
||||
|
||||
except (UnicodeDecodeError, PermissionError) as e:
|
||||
logger.warning("Failed to read skill file %s: %s", skill_md, e)
|
||||
continue
|
||||
except Exception as e:
|
||||
logger.warning("Error parsing skill %s: %s", skill_md, e, exc_info=True)
|
||||
continue
|
||||
|
||||
|
||||
return skills
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue