From 8825e9044c2657b726f589f4287cf827f77ff44e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 2 May 2026 02:00:06 -0700 Subject: [PATCH] fix(discord): complete #18741 for /skill autocomplete and drop legacy 25x25 caps (#18745) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``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 ``/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. --- hermes_cli/commands.py | 101 +++++++++++++++----------- tests/hermes_cli/test_commands.py | 113 ++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 40 deletions(-) diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index 2175661ba..ba0560224 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -734,24 +734,40 @@ def discord_skill_commands( def discord_skill_commands_by_category( reserved_names: set[str], ) -> tuple[dict[str, list[tuple[str, str, str]]], list[tuple[str, str, str]], int]: - """Return skill entries organized by category for Discord ``/skill`` subcommand groups. + """Return skill entries organized by category for Discord ``/skill`` autocomplete. - Skills whose directory is nested at least 2 levels under ``SKILLS_DIR`` + Skills whose directory is nested at least 2 levels under a scan root (e.g. ``creative/ascii-art/SKILL.md``) are grouped by their top-level category. Root-level skills (e.g. ``dogfood/SKILL.md``) are returned as - *uncategorized* — the caller should register them as direct subcommands - of the ``/skill`` group. + *uncategorized*. - The same filtering as :func:`discord_skill_commands` is applied: hub - skills excluded, per-platform disabled excluded, names clamped. + Scan roots include the local ``SKILLS_DIR`` **and** any configured + ``skills.external_dirs`` — matching the widened filter applied to the + flat ``discord_skill_commands()`` collector in #18741. Without this + parity, external-dir skills are visible via ``hermes skills list`` and + the agent's ``/skill-name`` dispatch but silently absent from Discord's + ``/skill`` autocomplete. + + Filtering mirrors :func:`discord_skill_commands`: hub skills excluded, + per-platform disabled excluded, names clamped to 32 chars, descriptions + clamped to 100 chars. + + The legacy 25-group × 25-subcommand caps (from the old nested + ``/skill `` layout) are **not** applied — the live caller + (``_register_skill_group`` in ``gateway/platforms/discord.py``, refactored + in PR #11580) flattens these results and feeds them into a single + autocomplete callback, which scales to thousands of entries without any + per-command payload concerns. ``hidden_count`` is retained in the return + tuple for backward compatibility and still reports skills dropped for + other reasons (32-char clamp collision vs a reserved name). Returns: ``(categories, uncategorized, hidden_count)`` - *categories*: ``{category_name: [(name, description, cmd_key), ...]}`` - *uncategorized*: ``[(name, description, cmd_key), ...]`` - - *hidden_count*: skills dropped due to Discord group limits - (25 subcommand groups, 25 subcommands per group) + - *hidden_count*: skills dropped due to name clamp collisions + against already-registered command names. """ from pathlib import Path as _P @@ -770,9 +786,23 @@ def discord_skill_commands_by_category( try: from agent.skill_commands import get_skill_commands + from agent.skill_utils import get_external_skills_dirs from tools.skills_tool import SKILLS_DIR + _skills_dir = SKILLS_DIR.resolve() _hub_dir = (SKILLS_DIR / ".hub").resolve() + # Build list of (resolved_root, is_local) tuples. Each external dir + # becomes its own scan root for category derivation — a skill at + # ``/mlops/foo/SKILL.md`` is still categorized as "mlops". + _scan_roots: list[_P] = [_skills_dir] + try: + for ext in get_external_skills_dirs(): + try: + _scan_roots.append(_P(ext).resolve()) + except Exception: + continue + except Exception: + pass skill_cmds = get_skill_commands() for cmd_key in sorted(skill_cmds): @@ -781,20 +811,35 @@ def discord_skill_commands_by_category( if not skill_path: continue sp = _P(skill_path).resolve() - # Skip skills outside SKILLS_DIR or from the hub - if not str(sp).startswith(str(_skills_dir)): - continue + # Hub skills are loaded via the skill hub, not surfaced as + # slash commands. if str(sp).startswith(str(_hub_dir)): continue + # Accept skill if it lives under any scan root; record the + # matching root so we can derive the category correctly. + matched_root: _P | None = None + for root in _scan_roots: + try: + sp.relative_to(root) + except ValueError: + continue + matched_root = root + break + if matched_root is None: + continue skill_name = info.get("name", "") if skill_name in _platform_disabled: continue raw_name = cmd_key.lstrip("/") - # Clamp to 32 chars (Discord limit) + # Clamp to 32 chars (Discord per-command name limit) discord_name = raw_name[:32] if discord_name in _names_used: + # Collision with a previously-registered name — drop and + # count. Almost always caused by a reserved built-in name, + # not by another skill (frontmatter names are unique). + hidden += 1 continue _names_used.add(discord_name) @@ -802,12 +847,9 @@ def discord_skill_commands_by_category( if len(desc) > 100: desc = desc[:97] + "..." - # Determine category from the relative path within SKILLS_DIR. - # e.g. creative/ascii-art/SKILL.md → parts = ("creative", "ascii-art") - try: - rel = sp.parent.relative_to(_skills_dir) - except ValueError: - continue + # Determine category from the relative path within the matched + # scan root. e.g. creative/ascii-art/SKILL.md → ("creative", ...) + rel = sp.parent.relative_to(matched_root) parts = rel.parts if len(parts) >= 2: cat = parts[0] @@ -817,28 +859,7 @@ def discord_skill_commands_by_category( except Exception: pass - # Enforce Discord limits: 25 subcommand groups, 25 subcommands each ------ - _MAX_GROUPS = 25 - _MAX_PER_GROUP = 25 - - trimmed_categories: dict[str, list[tuple[str, str, str]]] = {} - group_count = 0 - for cat in sorted(categories): - if group_count >= _MAX_GROUPS: - hidden += len(categories[cat]) - continue - entries = categories[cat][:_MAX_PER_GROUP] - hidden += max(0, len(categories[cat]) - _MAX_PER_GROUP) - trimmed_categories[cat] = entries - group_count += 1 - - # Uncategorized skills also count against the 25 top-level limit - remaining_slots = _MAX_GROUPS - group_count - if len(uncategorized) > remaining_slots: - hidden += len(uncategorized) - remaining_slots - uncategorized = uncategorized[:remaining_slots] - - return trimmed_categories, uncategorized, hidden + return categories, uncategorized, hidden # --------------------------------------------------------------------------- diff --git a/tests/hermes_cli/test_commands.py b/tests/hermes_cli/test_commands.py index 296143a03..7c19730d9 100644 --- a/tests/hermes_cli/test_commands.py +++ b/tests/hermes_cli/test_commands.py @@ -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