From 5eac6084bc781377cc1432165ebb489ccf5d6fbf Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 2 May 2026 02:05:01 -0700 Subject: [PATCH] fix(discord): warn on 32-char clamp collisions in the /skill collector (#18759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discord's per-command name limit is 32 chars. When two skill slugs share the same first 32 chars (or a skill slug clamps onto a reserved gateway command name), only the first seen wins — the second is dropped from the /skill autocomplete. The old behavior incremented a ``hidden`` counter silently, so skill authors had no way to discover the drop short of noticing their skill was missing from the picker. Not an actively-biting bug today (no collisions on the default catalog as of 2026-05), but a landmine the moment someone ships a skill with a long name. The earlier series in #18745 / #18753 / #18754 dropped the other silent data-loss paths in the Discord /skill collector; this one lights up the last remaining one. Fix: promote ``_names_used`` from a set to a dict keyed by the clamped name, mapping to the source cmd_key (or a ``""`` sentinel for names inherited via ``reserved_names``). On collision, log a WARNING naming both sides — the winner, the loser, the clamped name, and what to rename. Two phrasings: * skill-vs-skill — "both clamp to X on Discord's 32-char command-name limit; only the winner appears in /skill. Rename one skill's frontmatter ``name:`` to differ in its first 32 chars." * skill-vs-reserved — "collides with a reserved gateway command name; the skill will not appear in /skill. Rename the skill's frontmatter ``name:``." Tests: three cases in ``tests/hermes_cli/test_discord_skill_clamp_warning.py`` — skill-vs-skill collision (warning names both cmd_keys + clamped prefix), skill-vs-reserved collision (warning uses the distinct phrasing), and a no-collision negative (zero warnings emitted). --- hermes_cli/commands.py | 45 ++++- .../test_discord_skill_clamp_warning.py | 171 ++++++++++++++++++ 2 files changed, 211 insertions(+), 5 deletions(-) create mode 100644 tests/hermes_cli/test_discord_skill_clamp_warning.py diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index ba056022..1b4b85bd 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -10,6 +10,7 @@ To add an alias: set ``aliases=("short",)`` on the existing ``CommandDef``. from __future__ import annotations +import logging import os import re import shutil @@ -21,6 +22,8 @@ from typing import Any from utils import is_truthy_value +logger = logging.getLogger(__name__) + # prompt_toolkit is an optional CLI dependency — only needed for # SlashCommandCompleter and SlashCommandAutoSuggest. Gateway and test # environments that lack it must still be able to import this module @@ -781,7 +784,12 @@ def discord_skill_commands_by_category( # Collect raw skill data -------------------------------------------------- categories: dict[str, list[tuple[str, str, str]]] = {} uncategorized: list[tuple[str, str, str]] = [] - _names_used: set[str] = set(reserved_names) + # Map clamped-32-char-name → what it came from, so we can emit an + # actionable warning on collision. Reserved (gateway-builtin) command + # names are marked with a sentinel so the warning distinguishes + # "skill collided with a reserved command" from "two skills collided + # on the 32-char clamp" — the latter is the rename-worthy case. + _names_used: dict[str, str] = {n: "" for n in reserved_names} hidden = 0 try: @@ -836,12 +844,39 @@ def discord_skill_commands_by_category( # 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). + # Two skills whose first 32 chars are identical. One wins + # (the first one seen, which is alphabetical because the + # caller iterates ``sorted(skill_cmds)``); the other is + # dropped from Discord's /skill autocomplete. + # + # Silently counting this as ``hidden`` (the old behavior) + # meant skill authors had no way to discover the drop — + # their skill just didn't appear in the picker. Emit a + # WARNING naming both sides so the author can rename the + # losing skill's frontmatter name to something with a + # distinct 32-char prefix. + prior = _names_used[discord_name] + if prior == "": + logger.warning( + "Discord /skill: %r (from %r) collides on its 32-char " + "clamp with a reserved gateway command name %r — the " + "skill will not appear in the /skill autocomplete. " + "Rename the skill's frontmatter ``name:`` to differ " + "in its first 32 chars.", + discord_name, cmd_key, discord_name, + ) + else: + logger.warning( + "Discord /skill: %r and %r both clamp to %r on " + "Discord's 32-char command-name limit — only %r " + "will appear in the /skill autocomplete. Rename " + "one skill's frontmatter ``name:`` to differ in " + "its first 32 chars.", + prior, cmd_key, discord_name, prior, + ) hidden += 1 continue - _names_used.add(discord_name) + _names_used[discord_name] = cmd_key desc = info.get("description", "") if len(desc) > 100: diff --git a/tests/hermes_cli/test_discord_skill_clamp_warning.py b/tests/hermes_cli/test_discord_skill_clamp_warning.py new file mode 100644 index 00000000..541eeddc --- /dev/null +++ b/tests/hermes_cli/test_discord_skill_clamp_warning.py @@ -0,0 +1,171 @@ +"""Tests for Discord /skill 32-char clamp collision warnings. + +Discord's per-command name limit is 32 chars, so +``discord_skill_commands_by_category`` clamps skill slugs to that width +before deduping. When two skills share the same 32-char prefix, only +the first (alphabetical) wins; the second is dropped. Previously the +drop was silent — the ``hidden`` count incremented but nothing named +which skills collided, so authors had no way to discover the drop +short of noticing that their skill was missing from the autocomplete. + +This module pins the upgraded behavior: a WARNING log with both full +cmd_keys + the clamped name, so whoever named the skills sees the +collision and can rename one. +""" +from __future__ import annotations + +import logging +from pathlib import Path +from unittest.mock import patch + + +def test_clamp_collision_emits_warning_naming_both_skills( + tmp_path: Path, caplog +) -> None: + """Two skills with identical first 32 chars — warning names both.""" + from hermes_cli.commands import discord_skill_commands_by_category + + # Craft cmd_keys that share the first 32 chars. + # 40-char prefix 'skill-collision-prefix-identical-first-32' + # -> clamped to 'skill-collision-prefix-identical' + prefix = "skill-collision-prefix-identical" # exactly 32 chars + name_a = prefix + "-alpha" # /skill-collision-prefix-identical-alpha + name_b = prefix + "-bravo" # /skill-collision-prefix-identical-bravo + assert name_a[:32] == name_b[:32] == prefix + + skills_dir = tmp_path / "skills" + for nm in (name_a, name_b): + d = skills_dir / "creative" / nm + d.mkdir(parents=True) + (d / "SKILL.md").write_text("---\nname: x\n---\n") + + fake_cmds = { + f"/{name_a}": { + "name": name_a, + "description": "Alpha", + "skill_md_path": str(skills_dir / "creative" / name_a / "SKILL.md"), + }, + f"/{name_b}": { + "name": name_b, + "description": "Bravo", + "skill_md_path": str(skills_dir / "creative" / name_b / "SKILL.md"), + }, + } + + with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), ( + patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds) + ), patch("tools.skills_tool.SKILLS_DIR", skills_dir): + categories, uncategorized, hidden = discord_skill_commands_by_category( + reserved_names=set(), + ) + + # One skill made it through, one was dropped (hidden counted). + assert hidden == 1 + kept_names = [n for n, _d, _k in categories.get("creative", [])] + assert len(kept_names) == 1 + # Alphabetical iteration means the -alpha variant wins the slot. + assert kept_names[0] == prefix # clamped + + # Exactly one warning, naming BOTH full cmd_keys and the clamped name. + warnings = [ + r for r in caplog.records + if r.levelno == logging.WARNING and "clamp" in r.getMessage() + ] + assert len(warnings) == 1, ( + f"expected exactly one clamp-collision warning, got {len(warnings)}: " + f"{[r.getMessage() for r in warnings]}" + ) + msg = warnings[0].getMessage() + assert f"/{name_a}" in msg, f"winner not named in warning: {msg!r}" + assert f"/{name_b}" in msg, f"loser not named in warning: {msg!r}" + assert prefix in msg, f"clamped name not in warning: {msg!r}" + + +def test_clamp_collision_with_reserved_name_emits_distinct_warning( + tmp_path: Path, caplog +) -> None: + """A skill clashing with a reserved gateway command gets its own phrasing. + + The reserved-vs-skill case is operationally different — the fix is + still "rename the skill," but there's no second skill to also + rename. The warning should say so explicitly. + """ + from hermes_cli.commands import discord_skill_commands_by_category + + # Reserved name 'help' is 4 chars — make a skill whose slug + # clamps to 'help' (so, exactly 'help'). + reserved = "help" + skills_dir = tmp_path / "skills" + d = skills_dir / "creative" / reserved + d.mkdir(parents=True) + (d / "SKILL.md").write_text("---\nname: x\n---\n") + + fake_cmds = { + f"/{reserved}": { + "name": reserved, + "description": "desc", + "skill_md_path": str(d / "SKILL.md"), + }, + } + + with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), ( + patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds) + ), patch("tools.skills_tool.SKILLS_DIR", skills_dir): + categories, uncategorized, hidden = discord_skill_commands_by_category( + reserved_names={"help"}, + ) + + # Skill dropped in favor of the reserved command. + assert hidden == 1 + assert categories == {} + assert uncategorized == [] + + warnings = [ + r for r in caplog.records + if r.levelno == logging.WARNING and "reserved" in r.getMessage() + ] + assert len(warnings) == 1, ( + f"expected one reserved-name collision warning, got " + f"{[r.getMessage() for r in warnings]}" + ) + msg = warnings[0].getMessage() + assert f"/{reserved}" in msg + assert "reserved" in msg.lower() + + +def test_no_collision_no_warning(tmp_path: Path, caplog) -> None: + """Sanity: two distinct-prefix skills produce zero warnings.""" + from hermes_cli.commands import discord_skill_commands_by_category + + skills_dir = tmp_path / "skills" + for nm in ("alpha", "bravo"): + d = skills_dir / "creative" / nm + d.mkdir(parents=True) + (d / "SKILL.md").write_text("---\nname: x\n---\n") + + fake_cmds = { + "/alpha": { + "name": "alpha", "description": "", + "skill_md_path": str(skills_dir / "creative" / "alpha" / "SKILL.md"), + }, + "/bravo": { + "name": "bravo", "description": "", + "skill_md_path": str(skills_dir / "creative" / "bravo" / "SKILL.md"), + }, + } + + with caplog.at_level(logging.WARNING, logger="hermes_cli.commands"), ( + patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds) + ), patch("tools.skills_tool.SKILLS_DIR", skills_dir): + categories, uncategorized, hidden = discord_skill_commands_by_category( + reserved_names=set(), + ) + + assert hidden == 0 + assert {n for n, _d, _k in categories["creative"]} == {"alpha", "bravo"} + clamp_warnings = [ + r for r in caplog.records + if r.levelno == logging.WARNING + and ("clamp" in r.getMessage() or "reserved" in r.getMessage()) + ] + assert clamp_warnings == []