From 005fedd7109ecc621db37bd32955188a297c14a2 Mon Sep 17 00:00:00 2001 From: Casey West Date: Sat, 20 Jun 2026 16:36:57 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(profiles):=20count=20ext?= =?UTF-8?q?ernal=5Fdirs=20skills=20so=20dashboard=20matches=20CLI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profiles dashboard and `hermes profile list` showed "Skills: 0" for any profile whose skills come from a shared `skills.external_dirs` grant rather than a profile-local `skills/` directory — e.g. the eckert and lamport engineering profiles, which source all skills from the shared ~/.hermes/skills registry. `_count_skills` only looked at `/skills` via `rglob("SKILL.md")`, which (a) ignored the `external_dirs` grant entirely and (b) does not follow symlinks, so even when pointed at the shared dir it missed the symlinked skill packages (imported/, homestead/, ...). Fix it at the right layer: extract the active-env scan in `_find_all_skills` into a parameterized `_scan_skill_dirs(local, external)` helper, add `count_profile_skills(profile_dir)` that resolves a profile's own local + external_dirs grant, and a per-profile-dir resolver `get_external_skills_dirs_for`. `_count_skills` now delegates to that single source of truth, so the dashboard count equals `hermes skills list` exactly (129 here) — symlink-following and frontmatter-name dedup included. Regression tests cover the external-only profile, symlinked packages, local-precedence dedup, and the missing-config case. --- agent/skill_utils.py | 59 +++++++++++++++++++++++ hermes_cli/profiles.py | 27 ++++++----- tests/tools/test_skills_tool.py | 85 +++++++++++++++++++++++++++++++++ tools/skills_tool.py | 80 +++++++++++++++++++++++-------- 4 files changed, 220 insertions(+), 31 deletions(-) diff --git a/agent/skill_utils.py b/agent/skill_utils.py index 62bcc5a2b4b2..9494e76b3b54 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -338,6 +338,65 @@ def _external_dirs_cache_clear() -> None: _EXTERNAL_DIRS_CACHE.clear() +def get_external_skills_dirs_for(profile_dir: Path) -> List[Path]: + """Resolve ``skills.external_dirs`` from *profile_dir*'s ``config.yaml``. + + The per-profile-directory form of :func:`get_external_skills_dirs`: it reads + the config of an *arbitrary* profile instead of the active environment, so + callers that enumerate other profiles (the dashboard, ``hermes profile + list``) resolve each profile's own grant. *profile_dir* doubles as that + profile's HERMES_HOME for relative-path resolution. Entries are expanded + (``~``, ``${VAR}``), resolved to absolute paths, deduped, and dropped if + they don't exist or point back at the profile's own local ``skills/``. + + Not cached — callers enumerate a handful of profiles at most, and each has + a distinct config path. + """ + config_path = profile_dir / "config.yaml" + if not config_path.exists(): + return [] + try: + parsed = yaml_load(config_path.read_text(encoding="utf-8")) + except Exception: + return [] + if not isinstance(parsed, dict): + return [] + + skills_cfg = parsed.get("skills") + if not isinstance(skills_cfg, dict): + return [] + raw_dirs = skills_cfg.get("external_dirs") + if not raw_dirs: + return [] + if isinstance(raw_dirs, str): + raw_dirs = [raw_dirs] + if not isinstance(raw_dirs, list): + return [] + + local_skills = (profile_dir / "skills").resolve() + seen: Set[Path] = set() + result: List[Path] = [] + for entry in raw_dirs: + entry = str(entry).strip() + if not entry: + continue + expanded = os.path.expanduser(os.path.expandvars(entry)) + p = Path(expanded) + # Resolve relative paths against the profile's own home, not cwd. + if not p.is_absolute(): + p = (profile_dir / p).resolve() + else: + p = p.resolve() + if p == local_skills or p in seen: + continue + if p.is_dir(): + seen.add(p) + result.append(p) + else: + logger.debug("External skills dir does not exist, skipping: %s", p) + return result + + def get_external_skills_dirs() -> List[Path]: """Read ``skills.external_dirs`` from config.yaml and return validated paths. diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 31dbf8dfb4aa..b3ab36997136 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -30,8 +30,6 @@ from pathlib import Path, PurePosixPath, PureWindowsPath from typing import List, Optional -from agent.skill_utils import is_excluded_skill_path - _PROFILE_ID_RE = re.compile(r"^[a-z0-9][a-z0-9_-]{0,63}$") # Directories bootstrapped inside every new profile @@ -509,16 +507,23 @@ def _check_gateway_running(profile_dir: Path) -> bool: def _count_skills(profile_dir: Path) -> int: - """Count installed skills in a profile.""" - skills_dir = profile_dir / "skills" - if not skills_dir.is_dir(): + """Count skills available to the profile at *profile_dir*. + + Delegates to :func:`tools.skills_tool.count_profile_skills`, the single + source of truth for skill enumeration. That counts the profile's local + ``skills/`` directory **and** its ``skills.external_dirs`` grant through the + same scanner ``hermes skills list`` uses — so the dashboard's per-profile + count matches the CLI exactly, including symlinked shared grants and + frontmatter-name dedup. A profile that sources all its skills from a shared + external grant (with no local ``skills/`` dir) reports its true count, not 0. + """ + try: + from tools.skills_tool import count_profile_skills + + return count_profile_skills(profile_dir) + except Exception: + # Never let a counting failure break profile listing — degrade to 0. return 0 - count = 0 - for md in skills_dir.rglob("SKILL.md"): - if is_excluded_skill_path(md): - continue - count += 1 - return count # --------------------------------------------------------------------------- diff --git a/tests/tools/test_skills_tool.py b/tests/tools/test_skills_tool.py index 756e1e3b3b2a..8184281e0315 100644 --- a/tests/tools/test_skills_tool.py +++ b/tests/tools/test_skills_tool.py @@ -1267,3 +1267,88 @@ def test_local_only_skill_loads_normally(self, tmp_path): result = json.loads(raw) assert result["success"] is True assert "LOCAL BODY" in result["content"] + + +class TestCountProfileSkills: + """count_profile_skills resolves a profile's external_dirs grant. + + Regression guard for the dashboard/`hermes profile list` counter that + showed 0 for profiles whose skills come entirely from a shared + external_dirs grant (and undercounted symlinked skill packages). + """ + + def _write_config(self, profile_dir: Path, external_dirs: list) -> None: + profile_dir.mkdir(parents=True, exist_ok=True) + lines = "\n".join(f" - {d}" for d in external_dirs) + (profile_dir / "config.yaml").write_text( + "skills:\n external_dirs:\n" + lines + "\n" + ) + + def test_counts_skills_from_external_dirs(self, tmp_path): + """A profile with no local skills/ but an external grant counts them.""" + from tools.skills_tool import count_profile_skills + + shared = tmp_path / "shared" + _make_skill(shared, "alpha") + _make_skill(shared, "beta") + _make_skill(shared, "gamma") + + profile = tmp_path / "profiles" / "worker" + self._write_config(profile, [str(shared)]) + + assert count_profile_skills(profile) == 3 + + def test_external_grant_with_no_local_dir_is_not_zero(self, tmp_path): + """Regression: the exact bug — external-only profile must not report 0.""" + from tools.skills_tool import count_profile_skills + + shared = tmp_path / "shared" + _make_skill(shared, "only-skill") + profile = tmp_path / "profiles" / "eckert" + self._write_config(profile, [str(shared)]) + + assert not (profile / "skills").exists() + assert count_profile_skills(profile) == 1 + + def test_counts_symlinked_skill_packages(self, tmp_path): + """Symlinked skill packages in the grant are followed (rglob missed these).""" + from tools.skills_tool import count_profile_skills + + shared = tmp_path / "shared" + shared.mkdir(parents=True, exist_ok=True) + _make_skill(shared, "direct") + # A category symlink pointing outside the shared tree. + external_real = _symlink_category(shared, tmp_path / "elsewhere", "imported") + _make_skill(external_real, "linked-one") + _make_skill(external_real, "linked-two") + + profile = tmp_path / "profiles" / "lamport" + self._write_config(profile, [str(shared)]) + + # 1 direct + 2 behind the symlink = 3; rglob (no followlinks) would see 1. + assert count_profile_skills(profile) == 3 + + def test_local_takes_precedence_and_dedups_by_name(self, tmp_path): + """Local skills/ wins over external on a name collision; no double count.""" + from tools.skills_tool import count_profile_skills + + shared = tmp_path / "shared" + _make_skill(shared, "dup") + _make_skill(shared, "external-only") + + profile = tmp_path / "profiles" / "worker" + local = profile / "skills" + _make_skill(local, "dup") + _make_skill(local, "local-only") + self._write_config(profile, [str(shared)]) + + # dup (deduped) + external-only + local-only = 3 + assert count_profile_skills(profile) == 3 + + def test_missing_config_returns_zero(self, tmp_path): + """A profile dir with no config.yaml and no skills/ counts 0, no error.""" + from tools.skills_tool import count_profile_skills + + profile = tmp_path / "profiles" / "empty" + profile.mkdir(parents=True, exist_ok=True) + assert count_profile_skills(profile) == 0 diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 04bf35abe69d..a0a49d55624d 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -565,30 +565,31 @@ def _is_skill_disabled(name: str, platform: str = None) -> bool: return False -def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: - """Recursively find all skills in ~/.hermes/skills/ and external dirs. - - 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 (name, description, category). +def _scan_skill_dirs( + local_dir: Optional[Path], + external_dirs: List[Path], + *, + disabled: Set[str], +) -> List[Dict[str, Any]]: + """Scan *local_dir* then *external_dirs* and return skill metadata dicts. + + This is the single source of truth for "what skills does this set of + directories provide" — used both for the active environment + (:func:`_find_all_skills`) and for enumerating an arbitrary profile + (:func:`count_profile_skills`). Local takes precedence over external on a + name collision; skills whose frontmatter ``name`` is in *disabled* or that + fail the platform/environment gates are skipped. Symlinked skill packages + are followed (via :func:`iter_skill_index_files`). """ - from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files + from agent.skill_utils import iter_skill_index_files - skills = [] + skills: List[Dict[str, Any]] = [] seen_names: set = set() - # Load disabled set once (not per-skill) - disabled = set() if skip_disabled else _get_disabled_skill_names() - - # Scan local dir first, then external dirs (local takes precedence) - dirs_to_scan = [] - if SKILLS_DIR.exists(): - dirs_to_scan.append(SKILLS_DIR) - dirs_to_scan.extend(get_external_skills_dirs()) + dirs_to_scan: List[Path] = [] + if local_dir is not None and local_dir.exists(): + dirs_to_scan.append(local_dir) + dirs_to_scan.extend(external_dirs) for scan_dir in dirs_to_scan: for skill_md in iter_skill_index_files(scan_dir, "SKILL.md"): @@ -645,6 +646,45 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: return skills +def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: + """Recursively find all skills in ~/.hermes/skills/ and external dirs. + + 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 (name, description, category). + """ + from agent.skill_utils import get_external_skills_dirs + + disabled = set() if skip_disabled else _get_disabled_skill_names() + return _scan_skill_dirs( + SKILLS_DIR, + list(get_external_skills_dirs()), + disabled=disabled, + ) + + +def count_profile_skills(profile_dir: Path) -> int: + """Count skills available to the profile rooted at *profile_dir*. + + Resolves the profile's own local ``skills/`` directory plus its + ``skills.external_dirs`` grant (from that profile's ``config.yaml``) and + counts them through the same scanner the active-environment listing uses, + so a per-profile count (the dashboard, ``hermes profile list``) matches + what ``hermes -p skills list`` reports — including symlinked + grants and frontmatter-name dedup. Disabled skills are included, matching + the listing UI's ``skip_disabled=True`` semantics. + """ + from agent.skill_utils import get_external_skills_dirs_for + + local = profile_dir / "skills" + external = get_external_skills_dirs_for(profile_dir) + return len(_scan_skill_dirs(local, external, disabled=set())) + + def _sort_skills(skills: List[Dict[str, Any]]) -> List[Dict[str, Any]]: """Keep every skill listing path ordered the same way.""" return sorted(skills, key=lambda s: (s.get("category") or "", s["name"])) From e5cd98e1e0cb476c2141ddc702963954fcd1b828 Mon Sep 17 00:00:00 2001 From: Casey West Date: Sat, 20 Jun 2026 16:36:57 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=94=A7=20chore(release):=20map=20case?= =?UTF-8?q?y@geeknest.com=20to=20cwest=20in=20AUTHOR=5FMAP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check-attribution CI requires every contributor email to map to a GitHub username in scripts/release.py. Commits authored from casey@geeknest.com were unmapped, failing the gate. Add the mapping. --- scripts/release.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/release.py b/scripts/release.py index 5e574aec9f12..6ff04922ebbb 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -1459,6 +1459,7 @@ "wasdhkzk@gmail.com": "whyhkzk", # PR #32407 (sandbox-mirror inner-container guard; commits authored as whyhkzk + zhukun) "leonard@sellem.me": "leonardsellem", # PR #37405 (desktop WS origin guard on remote/Tailscale binds) "42903577+ohMyJason@users.noreply.github.com": "ohMyJason", # PR #29810 (discover_models in custom_providers section 4) + "casey@geeknest.com": "cwest", }