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
This commit is contained in:
@@ -502,9 +502,9 @@ def _sanitize_telegram_name(raw: str) -> str:
|
|||||||
|
|
||||||
|
|
||||||
def _clamp_command_names(
|
def _clamp_command_names(
|
||||||
entries: list[tuple[str, str]],
|
entries: list[tuple[str, ...]],
|
||||||
reserved: set[str],
|
reserved: set[str],
|
||||||
) -> list[tuple[str, str]]:
|
) -> list[tuple[str, ...]]:
|
||||||
"""Enforce 32-char command name limit with collision avoidance.
|
"""Enforce 32-char command name limit with collision avoidance.
|
||||||
|
|
||||||
Both Telegram and Discord cap slash command names at 32 characters.
|
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
|
(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.
|
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.
|
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)
|
used: set[str] = set(reserved)
|
||||||
result: list[tuple] = []
|
result: list[tuple] = []
|
||||||
|
|||||||
@@ -673,6 +673,7 @@ AUTHOR_MAP = {
|
|||||||
"web3blind@gmail.com": "web3blind",
|
"web3blind@gmail.com": "web3blind",
|
||||||
"ztzheng@163.com": "chengoak", # PR #17467
|
"ztzheng@163.com": "chengoak", # PR #17467
|
||||||
"24110240104@m.fudan.edu.cn": "YuShu", # co-author only
|
"24110240104@m.fudan.edu.cn": "YuShu", # co-author only
|
||||||
|
"charliekerfoot@gmail.com": "CharlieKerfoot", # PR #18951
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -822,6 +822,103 @@ class TestClampTelegramNames:
|
|||||||
assert result[0] == ("foo", "d1")
|
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:
|
class TestTelegramMenuCommands:
|
||||||
"""Integration: telegram_menu_commands enforces the 32-char limit."""
|
"""Integration: telegram_menu_commands enforces the 32-char limit."""
|
||||||
|
|
||||||
|
|||||||
@@ -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())
|
and ("clamp" in r.getMessage() or "reserved" in r.getMessage())
|
||||||
]
|
]
|
||||||
assert clamp_warnings == []
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user