From 5d5b8912bece744b08b5d6428f2ad12ff6969f87 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 3 May 2026 15:13:10 +0530 Subject: [PATCH] test: add tests for cmd_key preservation through name clamping - TestClampCommandNamesTriples: unit tests for 3-tuple support in _clamp_command_names (short names, long names, collisions, multiple entries, backward compat with 2-tuples) - TestDiscordSkillCmdKeyDispatch: integration test through the full discord_skill_commands pipeline verifying long skill names retain their original cmd_key after clamping - Add contributor CharlieKerfoot to AUTHOR_MAP --- hermes_cli/commands.py | 8 +- scripts/release.py | 1 + tests/hermes_cli/test_commands.py | 97 +++++++++++++++++++ .../test_discord_skill_clamp_warning.py | 75 ++++++++++++++ 4 files changed, 179 insertions(+), 2 deletions(-) diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index 6626cff08e..07e7273bf7 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -502,9 +502,9 @@ def _sanitize_telegram_name(raw: str) -> str: def _clamp_command_names( - entries: list[tuple[str, str]], + entries: list[tuple[str, ...]], reserved: set[str], -) -> list[tuple[str, str]]: +) -> list[tuple[str, ...]]: """Enforce 32-char command name limit with collision avoidance. Both Telegram and Discord cap slash command names at 32 characters. @@ -512,6 +512,10 @@ def _clamp_command_names( (against *reserved* names or earlier entries in the same batch), the name is shortened to 31 chars and a digit ``0``-``9`` is appended to differentiate. If all 10 digit slots are taken the entry is silently dropped. + + Accepts tuples of any length >= 2. Extra elements beyond ``(name, desc)`` + (e.g. ``cmd_key``) are passed through unchanged, so callers can attach + metadata that survives the rename. """ used: set[str] = set(reserved) result: list[tuple] = [] diff --git a/scripts/release.py b/scripts/release.py index 32453d723d..e06d1d2a31 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -673,6 +673,7 @@ AUTHOR_MAP = { "web3blind@gmail.com": "web3blind", "ztzheng@163.com": "chengoak", # PR #17467 "24110240104@m.fudan.edu.cn": "YuShu", # co-author only + "charliekerfoot@gmail.com": "CharlieKerfoot", # PR #18951 } diff --git a/tests/hermes_cli/test_commands.py b/tests/hermes_cli/test_commands.py index 7c19730d9e..d505c8a1a7 100644 --- a/tests/hermes_cli/test_commands.py +++ b/tests/hermes_cli/test_commands.py @@ -822,6 +822,103 @@ class TestClampTelegramNames: assert result[0] == ("foo", "d1") +class TestClampCommandNamesTriples: + """Tests for _clamp_command_names with 3-tuples (name, desc, cmd_key). + + Skill entries pass through _clamp_command_names as 3-tuples so the + original cmd_key survives name truncation. Before the fix in PR #18951, + the code stripped cmd_key into a side-dict keyed by the *original* + (name, desc) pair — after truncation the lookup key no longer matched, + silently losing the cmd_key. + """ + + def test_short_triple_preserved(self): + entries = [("skill", "A skill", "/skill")] + result = _clamp_command_names(entries, set()) + assert result == [("skill", "A skill", "/skill")] + + def test_long_name_preserves_cmd_key(self): + long = "a" * 50 + cmd_key = f"/{long}" + result = _clamp_command_names([(long, "desc", cmd_key)], set()) + assert len(result) == 1 + name, desc, key = result[0] + assert len(name) == _CMD_NAME_LIMIT + assert key == cmd_key, "cmd_key must survive name clamping" + + def test_collision_preserves_cmd_key(self): + prefix = "x" * _CMD_NAME_LIMIT + long = "x" * 50 + result = _clamp_command_names( + [(long, "desc", "/long-skill")], reserved={prefix}, + ) + assert len(result) == 1 + name, _desc, key = result[0] + assert name == "x" * (_CMD_NAME_LIMIT - 1) + "0" + assert key == "/long-skill" + + def test_multiple_long_names_preserve_respective_keys(self): + base = "y" * 40 + entries = [ + (base + "_alpha", "d1", "/alpha-skill"), + (base + "_beta", "d2", "/beta-skill"), + ] + result = _clamp_command_names(entries, set()) + assert len(result) == 2 + assert result[0][2] == "/alpha-skill" + assert result[1][2] == "/beta-skill" + + def test_backward_compat_with_pairs(self): + """Legacy 2-tuple callers (Telegram) must still work.""" + entries = [("help", "Show help"), ("status", "Show status")] + result = _clamp_command_names(entries, set()) + assert result == entries + + +class TestDiscordSkillCmdKeyDispatch: + """Integration: discord_skill_commands preserves cmd_key for long names. + + This tests the full pipeline: skill_commands → _collect_gateway_skill_entries + → _clamp_command_names → returned triples, verifying that skills with names + exceeding Discord's 32-char limit still have their original cmd_key for + dispatch. + """ + + def test_long_skill_name_retains_cmd_key(self, tmp_path, monkeypatch): + from unittest.mock import patch + + long_name = "this-is-a-very-long-skill-name-that-exceeds-limit" + cmd_key = f"/{long_name}" + fake_skills_dir = tmp_path / "skills" + fake_skills_dir.mkdir(exist_ok=True) + # Use resolved path — macOS /var → /private/var symlink + # causes SKILLS_DIR.resolve() to differ from tmp_path. + resolved_dir = str(fake_skills_dir.resolve()) + + fake_cmds = { + cmd_key: { + "name": long_name, + "description": "A skill with a long name", + "skill_md_path": f"{resolved_dir}/{long_name}/SKILL.md", + "skill_dir": f"{resolved_dir}/{long_name}", + }, + } + + with patch("agent.skill_commands.get_skill_commands", return_value=fake_cmds), \ + patch("tools.skills_tool.SKILLS_DIR", fake_skills_dir), \ + patch("agent.skill_utils.get_external_skills_dirs", return_value=[]): + entries, hidden = discord_skill_commands( + max_slots=100, reserved_names=set(), + ) + + assert len(entries) == 1 + name, desc, key = entries[0] + assert len(name) <= _CMD_NAME_LIMIT, "Name should be clamped to 32 chars" + assert key == cmd_key, ( + f"cmd_key must be the original /{long_name}, got {key!r}" + ) + + class TestTelegramMenuCommands: """Integration: telegram_menu_commands enforces the 32-char limit.""" diff --git a/tests/hermes_cli/test_discord_skill_clamp_warning.py b/tests/hermes_cli/test_discord_skill_clamp_warning.py index 541eeddc41..c9b686aae1 100644 --- a/tests/hermes_cli/test_discord_skill_clamp_warning.py +++ b/tests/hermes_cli/test_discord_skill_clamp_warning.py @@ -169,3 +169,78 @@ def test_no_collision_no_warning(tmp_path: Path, caplog) -> None: and ("clamp" in r.getMessage() or "reserved" in r.getMessage()) ] assert clamp_warnings == [] + + +def test_long_skill_name_preserves_cmd_key_through_by_category( + tmp_path: Path, +) -> None: + """Skills with names > 32 chars must keep their original cmd_key. + + ``discord_skill_commands_by_category`` clamps the display name to 32 + chars but the third tuple element (cmd_key) must stay as the original + ``/full-skill-name`` so that ``_skill_handler`` dispatches via + ``_run_simple_slash`` with the full command, not the truncated one. + + This is the actual runtime path used by the Discord adapter via + ``_refresh_skill_catalog_state``. + """ + from hermes_cli.commands import discord_skill_commands_by_category + + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + resolved = str(skills_dir.resolve()) + + long_name = "generate-ascii-art-from-text-description-detailed" + cmd_key = f"/{long_name}" + fake_cmds = { + cmd_key: { + "name": long_name, + "description": "Generate ASCII art from a text description", + "skill_md_path": f"{resolved}/creative/{long_name}/SKILL.md", + "skill_dir": f"{resolved}/creative/{long_name}", + }, + "/short-skill": { + "name": "short-skill", + "description": "A short skill", + "skill_md_path": f"{resolved}/creative/short-skill/SKILL.md", + "skill_dir": f"{resolved}/creative/short-skill", + }, + } + + with 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(), + ) + + # Flatten (same as _refresh_skill_catalog_state does) + entries = list(uncategorized) + for cat_skills in categories.values(): + entries.extend(cat_skills) + + # Build lookup (same as _refresh_skill_catalog_state does) + skill_lookup = {n: (d, k) for n, d, k in entries} + + # Find the long skill + long_entry = [e for e in entries if e[2] == cmd_key] + assert len(long_entry) == 1, f"Long skill should appear once, got: {long_entry}" + + display_name, desc, key = long_entry[0] + assert len(display_name) <= 32, ( + f"Display name should be clamped to 32 chars, got {len(display_name)}" + ) + assert key == cmd_key, ( + f"cmd_key must be the original /{long_name}, got {key!r}" + ) + + # Verify lookup works: clamped display name -> original cmd_key + assert display_name in skill_lookup + _desc, looked_up_key = skill_lookup[display_name] + assert looked_up_key == cmd_key, ( + f"Lookup must map clamped name to original cmd_key, got {looked_up_key!r}" + ) + + # Short skill should also be present and correct + short_entry = [e for e in entries if e[2] == "/short-skill"] + assert len(short_entry) == 1 + assert short_entry[0][0] == "short-skill"