Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions agent/skill_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
27 changes: 16 additions & 11 deletions hermes_cli/profiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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


# ---------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions scripts/release.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This AUTHOR_MAP entry is unrelated to the skill-count fix β€” a separate chore riding along. Fine to keep, just flagging that the commit history splits it out so it's intentional, not stray.

}


Expand Down
85 changes: 85 additions & 0 deletions tests/tools/test_skills_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
80 changes: 60 additions & 20 deletions tools/skills_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Expand Down Expand Up @@ -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 <profile> skills list`` reports β€” including symlinked
grants and frontmatter-name dedup. Disabled skills are included, matching
the listing UI's ``skip_disabled=True`` semantics.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth making explicit in the docstring that this counts available-including-disabled, so a profile that has skills disabled in its own config will show a number higher than what it actually loads at runtime. That's the correct choice for matching hermes skills list, but the dashboard reader might expect 'loaded' rather than 'available'.

"""
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"]))
Expand Down
Loading