fix(gateway): match disabled/optional skills by frontmatter slug, not dir name (#18753)

_check_unavailable_skill is meant to turn a typed "/foo" command that
doesn't resolve into a specific hint — "disabled, enable with hermes
skills config" or "available but not installed, install with hermes
skills install …" — instead of the generic "unknown command" reply.

It was doing the match with `skill_md.parent.name.lower().replace("_", "-")`,
comparing that to the typed command. For every skill whose directory name
drifted from its declared frontmatter `name:`, that comparison failed and
the user got the unhelpful generic path. On a standard install today 19
skills have this drift, e.g.:

  dir: mlops/stable-diffusion
  frontmatter: name: Stable Diffusion Image Generation
  registered slug (what the user types): /stable-diffusion-image-generation

  dir: mlops/qdrant
  frontmatter: name: Qdrant Vector Search
  registered slug: /qdrant-vector-search

  dir: mlops/flash-attention
  frontmatter: name: Optimizing Attention Flash
  registered slug: /optimizing-attention-flash

In every case, _check_unavailable_skill would fall through because
"stable-diffusion" != "stable-diffusion-image-generation", even with the
skill sitting right there on disk.

Fix: extract a small `_skill_slug_from_frontmatter` helper that reads the
SKILL.md frontmatter and normalizes exactly like scan_skill_commands
(lower, spaces/underscores → hyphens, strip non-[a-z0-9-], collapse
runs of hyphens, strip edges). Use it in both the
disabled-skills branch and the optional-skills branch. The disabled-set
membership check now uses the declared frontmatter name (which is what
`hermes skills config` writes into skills.disabled / platform_disabled),
not the slug.

Tests: five cases in tests/gateway/test_unavailable_skill_hint.py —
the drift case for the disabled branch, unknown-command negative,
matched-but-not-disabled negative, non-alnum stripping, and the drift
case for the optional-skills branch. All five fail against main and
pass with the fix.
This commit is contained in:
Teknium 2026-05-02 02:00:09 -07:00 committed by GitHub
parent 8825e9044c
commit 6ec74aec07
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 253 additions and 4 deletions

View File

@ -673,11 +673,69 @@ def _is_control_interrupt_message(message: Optional[str]) -> bool:
return normalized in _CONTROL_INTERRUPT_MESSAGES return normalized in _CONTROL_INTERRUPT_MESSAGES
def _skill_slug_from_frontmatter(skill_md: Path) -> tuple[str | None, str | None]:
"""Derive the /command slug and declared frontmatter name from a SKILL.md.
Matches the exact normalization used by
:func:`agent.skill_commands.scan_skill_commands` so the slug here is the
same string a user types after the leading ``/`` (e.g. a skill with
frontmatter ``name: Stable Diffusion Image Generation`` resolves to
``stable-diffusion-image-generation`` NOT the parent directory name,
which is commonly shorter/different, e.g. ``stable-diffusion``).
Using the directory name silently broke :func:`_check_unavailable_skill`
for every skill whose directory name drifted from its frontmatter name
(19 such skills on a standard install as of 2026-05), causing a generic
"unknown command" response where a "disabled — enable with …" or
"not installed — install with …" hint was expected.
Returns ``(slug, declared_name)`` or ``(None, None)`` when the file
can't be read or lacks a ``name:`` in its frontmatter.
"""
try:
content = skill_md.read_text(encoding="utf-8", errors="replace")
except Exception:
return None, None
if not content.startswith("---"):
return None, None
end = content.find("\n---", 3)
if end < 0:
return None, None
declared_name: str | None = None
for line in content[3:end].splitlines():
line = line.strip()
if line.startswith("name:"):
raw = line.split(":", 1)[1].strip()
# Strip YAML quote wrappers if present
if len(raw) >= 2 and raw[0] == raw[-1] and raw[0] in ('"', "'"):
raw = raw[1:-1]
declared_name = raw.strip()
break
if not declared_name:
return None, None
slug = declared_name.lower().replace(" ", "-").replace("_", "-")
# Mirror _SKILL_INVALID_CHARS and _SKILL_MULTI_HYPHEN from skill_commands
import re as _re
slug = _re.sub(r"[^a-z0-9-]", "", slug)
slug = _re.sub(r"-{2,}", "-", slug).strip("-")
if not slug:
return None, declared_name
return slug, declared_name
def _check_unavailable_skill(command_name: str) -> str | None: def _check_unavailable_skill(command_name: str) -> str | None:
"""Check if a command matches a known-but-inactive skill. """Check if a command matches a known-but-inactive skill.
Returns a helpful message if the skill exists but is disabled or only Returns a helpful message if the skill exists but is disabled or only
available as an optional install. Returns None if no match found. available as an optional install. Returns None if no match found.
The slug for each on-disk skill is derived from its frontmatter ``name:``
(via :func:`_skill_slug_from_frontmatter`), NOT from its containing
directory name because the two can differ (e.g. directory
``stable-diffusion`` + frontmatter ``Stable Diffusion Image Generation``
yields slug ``stable-diffusion-image-generation``). Matching on
directory name would miss that slug entirely and fall through to the
generic "unknown command" path.
""" """
# Normalize: command uses hyphens, skill names may use hyphens or underscores # Normalize: command uses hyphens, skill names may use hyphens or underscores
normalized = command_name.lower().replace("_", "-") normalized = command_name.lower().replace("_", "-")
@ -693,8 +751,12 @@ def _check_unavailable_skill(command_name: str) -> str | None:
for skill_md in skills_dir.rglob("SKILL.md"): for skill_md in skills_dir.rglob("SKILL.md"):
if any(part in ('.git', '.github', '.hub', '.archive') for part in skill_md.parts): if any(part in ('.git', '.github', '.hub', '.archive') for part in skill_md.parts):
continue continue
name = skill_md.parent.name.lower().replace("_", "-") slug, declared_name = _skill_slug_from_frontmatter(skill_md)
if name == normalized and name in disabled: if not slug or not declared_name:
continue
# disabled is keyed by the declared frontmatter name (what
# skills.disabled / skills.platform_disabled store).
if slug == normalized and declared_name in disabled:
return ( return (
f"The **{command_name}** skill is installed but disabled.\n" f"The **{command_name}** skill is installed but disabled.\n"
f"Enable it with: `hermes skills config`" f"Enable it with: `hermes skills config`"
@ -706,8 +768,10 @@ def _check_unavailable_skill(command_name: str) -> str | None:
optional_dir = get_optional_skills_dir(repo_root / "optional-skills") optional_dir = get_optional_skills_dir(repo_root / "optional-skills")
if optional_dir.exists(): if optional_dir.exists():
for skill_md in optional_dir.rglob("SKILL.md"): for skill_md in optional_dir.rglob("SKILL.md"):
name = skill_md.parent.name.lower().replace("_", "-") slug, _declared = _skill_slug_from_frontmatter(skill_md)
if name == normalized: if not slug:
continue
if slug == normalized:
# Build install path: official/<category>/<name> # Build install path: official/<category>/<name>
rel = skill_md.parent.relative_to(optional_dir) rel = skill_md.parent.relative_to(optional_dir)
parts = list(rel.parts) parts = list(rel.parts)

View File

@ -0,0 +1,185 @@
"""Tests for gateway.run._check_unavailable_skill.
Regression coverage for the dir-name-vs-frontmatter-name drift bug.
The hint function used to compare the skill's parent-directory name
against the typed command and the disabled list. That silently missed
every skill whose directory name differs from its declared frontmatter
name (~19 skills on a standard install), so users typing a real slug
like ``/stable-diffusion-image-generation`` got a generic "unknown
command" response instead of the intended "disabled enable with "
or "not installed — install with …" hint.
These tests pin the fixed behavior:
* Slug is derived from the frontmatter ``name:`` (exactly matching
:func:`agent.skill_commands.scan_skill_commands`), so the slug differs
from the directory name when the declared name is multi-word.
* ``disabled`` membership is checked by the declared name, because that
is what :func:`hermes_cli.skills_config.save_disabled_skills` stores.
"""
from __future__ import annotations
from pathlib import Path
from unittest.mock import patch
import pytest
@pytest.fixture
def tmp_skills(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
"""Isolated skills dir + HERMES_HOME so the real user config is untouched."""
home = tmp_path / ".hermes"
home.mkdir()
(home / "skills").mkdir()
monkeypatch.setenv("HERMES_HOME", str(home))
monkeypatch.setattr(Path, "home", lambda: tmp_path)
return home / "skills"
def _write_skill(skills_dir: Path, rel: str, frontmatter_name: str) -> Path:
"""Create a SKILL.md at ``<skills_dir>/<rel>/SKILL.md``."""
skill_dir = skills_dir / rel
skill_dir.mkdir(parents=True, exist_ok=True)
skill_md = skill_dir / "SKILL.md"
skill_md.write_text(
f"---\nname: {frontmatter_name}\ndescription: test skill\n---\nBody.\n",
encoding="utf-8",
)
return skill_md
def test_frontmatter_slug_matched_even_when_dir_name_differs(
tmp_skills: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Directory ``stable-diffusion`` + frontmatter ``Stable Diffusion Image Generation``.
Command typed: ``stable-diffusion-image-generation`` (the slug the
agent actually registers). The old dir-name-based check would have
compared ``stable-diffusion`` to the typed command and missed.
"""
from gateway import run as gateway_run
_write_skill(tmp_skills, "mlops/stable-diffusion", "Stable Diffusion Image Generation")
# Config disables by declared name (matches what `hermes skills config` writes).
monkeypatch.setattr(
"gateway.run._get_disabled_skill_names",
lambda: {"Stable Diffusion Image Generation"},
raising=False,
)
with patch(
"tools.skills_tool._get_disabled_skill_names",
return_value={"Stable Diffusion Image Generation"},
), patch(
"agent.skill_utils.get_all_skills_dirs",
return_value=[tmp_skills],
):
msg = gateway_run._check_unavailable_skill("stable-diffusion-image-generation")
assert msg is not None, (
"expected a 'disabled' hint for the frontmatter-derived slug; "
"the old code compared the dir name 'stable-diffusion' and returned None"
)
assert "disabled" in msg.lower()
assert "hermes skills config" in msg
def test_unknown_command_still_returns_none(
tmp_skills: Path,
) -> None:
"""A command that matches no on-disk skill still returns None."""
from gateway import run as gateway_run
_write_skill(tmp_skills, "creative/ascii-art", "ascii-art")
with patch(
"tools.skills_tool._get_disabled_skill_names", return_value=set()
), patch(
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
):
assert gateway_run._check_unavailable_skill("no-such-skill") is None
def test_matched_but_not_disabled_returns_none(
tmp_skills: Path,
) -> None:
"""A skill that exists and isn't disabled shouldn't produce a hint."""
from gateway import run as gateway_run
_write_skill(tmp_skills, "creative/ascii-art", "ascii-art")
with patch(
"tools.skills_tool._get_disabled_skill_names", return_value=set()
), patch(
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
):
assert gateway_run._check_unavailable_skill("ascii-art") is None
def test_slug_normalization_strips_non_alnum(
tmp_skills: Path,
) -> None:
"""Frontmatter ``C++ Code Review`` → slug ``c-code-review`` (``+`` stripped)."""
from gateway import run as gateway_run
_write_skill(tmp_skills, "software-development/cpp-review", "C++ Code Review")
with patch(
"tools.skills_tool._get_disabled_skill_names",
return_value={"C++ Code Review"},
), patch(
"agent.skill_utils.get_all_skills_dirs", return_value=[tmp_skills]
):
msg = gateway_run._check_unavailable_skill("c-code-review")
assert msg is not None
assert "disabled" in msg.lower()
def test_optional_skill_uses_frontmatter_slug(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Same drift bug applies to the optional-skills branch.
Before: directory name was matched against the typed command, so an
optional skill at ``optional-skills/mlops/stable-diffusion/SKILL.md``
with frontmatter ``Stable Diffusion Image Generation`` returned None
when the user typed the real slug.
"""
from gateway import run as gateway_run
# Build an isolated optional-skills dir
optional = tmp_path / "optional-skills"
skill_dir = optional / "mlops" / "stable-diffusion"
skill_dir.mkdir(parents=True)
(skill_dir / "SKILL.md").write_text(
"---\nname: Stable Diffusion Image Generation\ndescription: test\n---\n",
encoding="utf-8",
)
# Point the optional lookup at our tmp dir. The source reads from
# ``get_optional_skills_dir(repo_root / "optional-skills")`` — we
# can't easily retarget ``repo_root``, so patch the resolver.
monkeypatch.setattr(
"hermes_constants.get_optional_skills_dir",
lambda _default: optional,
raising=False,
)
# Ensure the "disabled" branch doesn't match anything so we fall
# through to the optional-skills branch.
empty_skills = tmp_path / "empty-skills"
empty_skills.mkdir()
with patch(
"tools.skills_tool._get_disabled_skill_names", return_value=set()
), patch(
"agent.skill_utils.get_all_skills_dirs", return_value=[empty_skills]
):
msg = gateway_run._check_unavailable_skill("stable-diffusion-image-generation")
assert msg is not None, (
"optional-skills branch should recognize the frontmatter-derived slug; "
"the old dir-name-based check returned None here too"
)
assert "not installed" in msg.lower()
assert "official/mlops/stable-diffusion" in msg