mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
fix(skills): guard skill slash commands against core-command and slug collisions
scan_skill_commands() had two collision bugs in the same loop body:
1. Core-command collision: a skill whose normalized slug matches a core
Hermes command name or alias (e.g. "skills", "learn", "bg") would
get an auto-generated /command that shadows the core command in the
gateway dispatch path (skill map is consulted before built-in
handlers). The skill command silently overrode the core command.
2. Inter-skill slug collision: the seen_names set deduped on the raw
frontmatter name, but the command map was keyed by the normalized
slug. Two distinct names collapsing to the same slug (e.g.
"git_helper" vs "git-helper") both passed the dedup, and the second
silently clobbered the first.
Fix: add two guards in scan_skill_commands() after slug normalization:
- resolve_command(cmd_name) check skips skills colliding with any core
CommandDef (name or alias), logging a warning. Uses the existing
resolve_command() API so aliases and case variants are covered
without a separate cache. The skill remains loadable via /skill.
- cmd_key in _skill_commands check dedups on the resolved slug,
first-wins (preserving local-before-external precedence), logging
a warning naming the shadowed skill.
Combines and supersedes #31204 (@cyrkstudios), #53450 (@Gridzilla),
#50304 (@petrichor-op), and #63305 (@Vissirexa).
Co-authored-by: cyrkstudios <cyrkstudios@users.noreply.github.com>
Co-authored-by: Gridzilla <Gridzilla@users.noreply.github.com>
Co-authored-by: petrichor-op <petrichor-op@users.noreply.github.com>
Co-authored-by: Vissirexa <Vissirexa@users.noreply.github.com>
This commit is contained in:
parent
f9c6f92c4b
commit
370ebf2d35
2 changed files with 103 additions and 1 deletions
|
|
@ -329,6 +329,7 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]:
|
|||
try:
|
||||
from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform, skill_matches_environment, _get_disabled_skill_names
|
||||
from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files
|
||||
from hermes_cli.commands import resolve_command
|
||||
disabled = _get_disabled_skill_names()
|
||||
seen_names: set = set()
|
||||
|
||||
|
|
@ -374,7 +375,32 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]:
|
|||
cmd_name = _SKILL_MULTI_HYPHEN.sub('-', cmd_name).strip('-')
|
||||
if not cmd_name:
|
||||
continue
|
||||
_skill_commands[f"/{cmd_name}"] = {
|
||||
# Skip if this skill's auto-generated /command collides
|
||||
# with a core Hermes slash command (name or alias). The
|
||||
# skill remains fully loadable via /skill <name>.
|
||||
# Uses resolve_command() so aliases and case variants are
|
||||
# covered without maintaining a separate cache.
|
||||
if resolve_command(cmd_name) is not None:
|
||||
logger.warning(
|
||||
"Skill %r generates slash command '/%s' which "
|
||||
"collides with a core Hermes command; skipping "
|
||||
"auto-registration. Use '/skill %s' instead.",
|
||||
name, cmd_name, name,
|
||||
)
|
||||
continue
|
||||
# Dedup on the resolved slug, not just the raw name: two
|
||||
# distinct frontmatter names can normalize to the same
|
||||
# slug (e.g. "git_helper" vs "git-helper"). First-wins
|
||||
# preserves local-before-external precedence.
|
||||
cmd_key = f"/{cmd_name}"
|
||||
if cmd_key in _skill_commands:
|
||||
logger.warning(
|
||||
"Skill %r maps to slash command %s already claimed "
|
||||
"by %r; keeping the first and skipping this one.",
|
||||
name, cmd_key, _skill_commands[cmd_key]["name"],
|
||||
)
|
||||
continue
|
||||
_skill_commands[cmd_key] = {
|
||||
"name": name,
|
||||
"description": description or f"Invoke the {name} skill",
|
||||
"skill_md_path": str(skill_md),
|
||||
|
|
|
|||
|
|
@ -377,6 +377,82 @@ class TestScanSkillCommands:
|
|||
assert "/sonarr-v3v4-api" in result
|
||||
assert any("/" in k[1:] for k in result) is False # no unescaped /
|
||||
|
||||
# -- core-command collision guard (#31204 / #53450) ---------------------
|
||||
|
||||
def test_skill_collides_with_core_command_is_skipped(self, tmp_path):
|
||||
"""A skill whose auto-generated /command collides with a core Hermes
|
||||
command (e.g. 'skills') should be excluded from the slash-command map.
|
||||
The skill remains loadable via /skill <name>."""
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
_make_skill(tmp_path, "skills")
|
||||
result = scan_skill_commands()
|
||||
assert "/skills" not in result
|
||||
|
||||
def test_skill_collides_with_core_alias_is_skipped(self, tmp_path):
|
||||
"""A skill whose slug matches a core command *alias* is also skipped."""
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
_make_skill(tmp_path, "bg")
|
||||
result = scan_skill_commands()
|
||||
# "bg" is an alias of the "background" command
|
||||
assert "/bg" not in result
|
||||
|
||||
def test_core_command_collision_does_not_block_others(self, tmp_path):
|
||||
"""A colliding skill is skipped, but non-colliding skills in the same
|
||||
scan pass are still registered."""
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
_make_skill(tmp_path, "skills")
|
||||
_make_skill(tmp_path, "my-other-skill")
|
||||
result = scan_skill_commands()
|
||||
assert "/skills" not in result
|
||||
assert "/my-other-skill" in result
|
||||
|
||||
# -- inter-skill slug collision dedup (#50304 / #63305) ------------------
|
||||
|
||||
def test_slug_collision_keeps_first_skill(self, tmp_path):
|
||||
"""Two skills whose names normalize to the same slug do not clobber.
|
||||
|
||||
``git_helper`` and ``git-helper`` are distinct frontmatter names but
|
||||
both reduce to the ``/git-helper`` command. The first one scanned must
|
||||
keep the command rather than being silently overwritten by the second.
|
||||
"""
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
# ``a-first`` sorts before ``z-second`` so the index walk visits the
|
||||
# underscore-named skill first; that one must win the slash command.
|
||||
first = tmp_path / "a-first"
|
||||
first.mkdir()
|
||||
(first / "SKILL.md").write_text(
|
||||
"---\nname: git_helper\ndescription: First skill.\n---\n\nBody.\n"
|
||||
)
|
||||
second = tmp_path / "z-second"
|
||||
second.mkdir()
|
||||
(second / "SKILL.md").write_text(
|
||||
"---\nname: git-helper\ndescription: Second skill.\n---\n\nBody.\n"
|
||||
)
|
||||
result = scan_skill_commands()
|
||||
assert "/git-helper" in result
|
||||
# First-wins: the entry resolves to the first skill, not the shadowing one.
|
||||
assert result["/git-helper"]["name"] == "git_helper"
|
||||
assert result["/git-helper"]["skill_dir"] == str(first)
|
||||
|
||||
def test_slug_collision_warns(self, tmp_path, caplog):
|
||||
"""A slug collision emits a warning so the user can diagnose the
|
||||
shadowed skill."""
|
||||
import logging as _logging
|
||||
with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
|
||||
first = tmp_path / "a-first"
|
||||
first.mkdir()
|
||||
(first / "SKILL.md").write_text(
|
||||
"---\nname: my-skill\ndescription: First.\n---\n\nBody.\n"
|
||||
)
|
||||
second = tmp_path / "z-second"
|
||||
second.mkdir()
|
||||
(second / "SKILL.md").write_text(
|
||||
"---\nname: my_skill\ndescription: Second.\n---\n\nBody.\n"
|
||||
)
|
||||
with caplog.at_level(_logging.WARNING, logger="agent.skill_commands"):
|
||||
scan_skill_commands()
|
||||
assert any("already claimed" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
class TestResolveSkillCommandKey:
|
||||
"""Telegram bot-command names disallow hyphens, so the menu registers
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue