``discord_skill_commands_by_category`` was lagging the flat ``discord_skill_commands`` collector on two counts. Both were actively dropping skills from Discord's ``/skill`` autocomplete dropdown. 1. External-dir skills were filtered out. #18741 widened the flat collector to accept ``SKILLS_DIR + skills.external_dirs`` but left this sibling collector — the one ``_register_skill_group`` actually uses on Discord — still matching ``SKILLS_DIR`` only. External skills were visible in ``hermes skills list`` and the agent's ``/skill-name`` dispatch but silently absent from Discord's ``/skill`` picker. Widen the accepted roots to match, and derive categories from whichever root the skill lives under so ``<ext>/mlops/foo/SKILL.md`` still lands in the ``mlops`` group. 2. 25-group × 25-subcommand caps were still applied. PR #11580 refactored ``/skill`` to a flat autocomplete (whose options Discord fetches dynamically — no per-command payload concern) and its docstring promises "no hidden skills." The collector kept the old nested-layout caps anyway, silently dropping anything past the 25th alphabetical category. On installs with 29 category dirs today (real example: tail categories ``social-media``, ``software-development``, ``yuanbao`` going missing) this was biting immediately. Remove the caps; ``hidden`` now reports only 32-char name-clamp collisions against reserved names. Tests: guard both behaviors. ``test_no_legacy_25x25_cap`` builds 30 categories × 30 skills each and asserts all 900 are returned. ``test_external_dirs_skills_included`` monkeypatches ``get_external_skills_dirs`` and asserts an external-dir skill makes it into the result grouped under its own top-level directory.
This commit is contained in:
@@ -1420,6 +1420,119 @@ class TestDiscordSkillCommandsByCategory:
|
||||
assert "vllm" in names
|
||||
assert len(uncategorized) == 0
|
||||
|
||||
def test_no_legacy_25x25_cap(self, tmp_path, monkeypatch):
|
||||
"""The old nested-layout caps (25 groups × 25 skills/group) are gone.
|
||||
|
||||
The live caller flattens categories into a single autocomplete list,
|
||||
which Discord fetches dynamically — the per-command 8KB payload
|
||||
concern from the old nested layout (#11321, #10259) no longer applies.
|
||||
Guards against accidentally re-introducing the caps, which would
|
||||
silently drop skills in the 26th+ alphabetical category (the exact
|
||||
failure mode users were hitting with 29 category dirs on real
|
||||
installs).
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
fake_skills_dir = str(tmp_path / "skills")
|
||||
|
||||
# Build 30 categories (> old _MAX_GROUPS=25) each with 30 skills
|
||||
# (> old _MAX_PER_GROUP=25).
|
||||
fake_cmds = {}
|
||||
for c in range(30):
|
||||
cat = f"cat{c:02d}" # cat00, cat01, ..., cat29 — 30 categories
|
||||
for s in range(30):
|
||||
name = f"skill-{c:02d}-{s:02d}"
|
||||
skill_subdir = tmp_path / "skills" / cat / name
|
||||
skill_subdir.mkdir(parents=True, exist_ok=True)
|
||||
(skill_subdir / "SKILL.md").write_text("---\nname: x\n---\n")
|
||||
fake_cmds[f"/{name}"] = {
|
||||
"name": name,
|
||||
"description": f"Category {cat} skill {s}",
|
||||
"skill_md_path": f"{fake_skills_dir}/{cat}/{name}/SKILL.md",
|
||||
}
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
with (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds),
|
||||
patch("tools.skills_tool.SKILLS_DIR", tmp_path / "skills"),
|
||||
):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# Every category should be present — no 25-group cap
|
||||
assert len(categories) == 30, (
|
||||
f"expected all 30 categories, got {len(categories)} "
|
||||
f"(cap from old nested layout must be removed)"
|
||||
)
|
||||
# Every skill in every category must be present — no 25-per-group cap
|
||||
for cat_name, entries in categories.items():
|
||||
assert len(entries) == 30, (
|
||||
f"category {cat_name}: expected 30 skills, got {len(entries)} "
|
||||
f"(cap from old nested layout must be removed)"
|
||||
)
|
||||
# Nothing should be reported hidden for the cap reason (the only
|
||||
# legitimate hidden reason now is name clamp collisions, which
|
||||
# don't happen here since all names are unique).
|
||||
assert hidden == 0
|
||||
|
||||
def test_external_dirs_skills_included(self, tmp_path, monkeypatch):
|
||||
"""Skills in ``skills.external_dirs`` must appear in /skill autocomplete.
|
||||
|
||||
#18741 fixed this for the flat ``discord_skill_commands`` collector
|
||||
but left ``discord_skill_commands_by_category`` (the live caller for
|
||||
Discord's ``/skill`` command) still filtering by
|
||||
``SKILLS_DIR`` prefix only. Regression guard that both collectors
|
||||
now accept external-dir skills.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
local_skills_dir = tmp_path / "local-skills"
|
||||
external_dir = tmp_path / "external-skills"
|
||||
|
||||
(local_skills_dir / "creative" / "local-skill").mkdir(parents=True)
|
||||
(local_skills_dir / "creative" / "local-skill" / "SKILL.md").write_text("")
|
||||
|
||||
(external_dir / "mlops" / "external-skill").mkdir(parents=True)
|
||||
(external_dir / "mlops" / "external-skill" / "SKILL.md").write_text("")
|
||||
|
||||
fake_cmds = {
|
||||
"/local-skill": {
|
||||
"name": "local-skill",
|
||||
"description": "Local",
|
||||
"skill_md_path": str(local_skills_dir / "creative" / "local-skill" / "SKILL.md"),
|
||||
},
|
||||
"/external-skill": {
|
||||
"name": "external-skill",
|
||||
"description": "External",
|
||||
"skill_md_path": str(external_dir / "mlops" / "external-skill" / "SKILL.md"),
|
||||
},
|
||||
}
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
with (
|
||||
patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds),
|
||||
patch("tools.skills_tool.SKILLS_DIR", local_skills_dir),
|
||||
patch(
|
||||
"agent.skill_utils.get_external_skills_dirs",
|
||||
return_value=[external_dir],
|
||||
),
|
||||
):
|
||||
categories, uncategorized, hidden = discord_skill_commands_by_category(
|
||||
reserved_names=set(),
|
||||
)
|
||||
|
||||
# Local skill → grouped under "creative"
|
||||
assert "creative" in categories
|
||||
assert any(n == "local-skill" for n, _d, _k in categories["creative"])
|
||||
# External skill → grouped under its own top-level dir "mlops"
|
||||
assert "mlops" in categories, (
|
||||
"external-dir skills must be included — the old SKILLS_DIR-only "
|
||||
"prefix check was broken for by_category (completes #18741)"
|
||||
)
|
||||
assert any(n == "external-skill" for n, _d, _k in categories["mlops"])
|
||||
assert uncategorized == []
|
||||
assert hidden == 0
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Plugin slash command integration
|
||||
|
||||
Reference in New Issue
Block a user