fix: address prefix matching recursion and skill command coverage
Per teknium1 review on PR #968: 1. Guard against infinite recursion: if expanded name equals the typed token (already exact), fall through to Unknown command instead of redispatching the same string forever. 2. Include skill slash commands in prefix resolution so execution-time matching agrees with tab-completion (set(COMMANDS) | set(_skill_commands)). 3. Add missing test cases: - unambiguous prefix with extra args does not recurse - exact command with args does not loop - skill command prefix matches correctly - exact builtin takes priority over skill prefix ambiguity 8 tests passing.
This commit is contained in:
24
cli.py
24
cli.py
@@ -3094,15 +3094,27 @@ class HermesCLI:
|
||||
else:
|
||||
self.console.print(f"[bold red]Failed to load skill for {base_cmd}[/]")
|
||||
else:
|
||||
# Prefix matching: if input uniquely identifies one command, execute it
|
||||
# Prefix matching: if input uniquely identifies one command, execute it.
|
||||
# Matches against both built-in COMMANDS and installed skill commands so
|
||||
# that execution-time resolution agrees with tab-completion.
|
||||
from hermes_cli.commands import COMMANDS
|
||||
typed_base = cmd_lower.split()[0]
|
||||
matches = [c for c in COMMANDS if c.startswith(typed_base)]
|
||||
all_known = set(COMMANDS) | set(_skill_commands)
|
||||
matches = [c for c in all_known if c.startswith(typed_base)]
|
||||
if len(matches) == 1:
|
||||
# Re-dispatch with the full command name, preserving any arguments
|
||||
remainder = cmd_original.strip()[len(typed_base):]
|
||||
full_cmd = matches[0] + remainder
|
||||
return self.process_command(full_cmd)
|
||||
# Expand the prefix to the full command name, preserving arguments.
|
||||
# Guard against redispatching the same token to avoid infinite
|
||||
# recursion when the expanded name still doesn't hit an exact branch
|
||||
# (e.g. /config with extra args that are not yet handled above).
|
||||
full_name = matches[0]
|
||||
if full_name == typed_base:
|
||||
# Already an exact token — no expansion possible; fall through
|
||||
self.console.print(f"[bold red]Unknown command: {cmd_lower}[/]")
|
||||
self.console.print("[dim #B8860B]Type /help for available commands[/]")
|
||||
else:
|
||||
remainder = cmd_original.strip()[len(typed_base):]
|
||||
full_cmd = full_name + remainder
|
||||
return self.process_command(full_cmd)
|
||||
elif len(matches) > 1:
|
||||
self.console.print(f"[bold yellow]Ambiguous command: {cmd_lower}[/]")
|
||||
self.console.print(f"[dim]Did you mean: {', '.join(sorted(matches))}?[/]")
|
||||
|
||||
@@ -20,28 +20,53 @@ class TestSlashCommandPrefixMatching:
|
||||
cli_obj.process_command("/con")
|
||||
mock_config.assert_called_once()
|
||||
|
||||
def test_unique_prefix_with_args_dispatches_command(self):
|
||||
"""/mo with argument should dispatch to /model."""
|
||||
def test_unique_prefix_with_args_does_not_recurse(self):
|
||||
"""/con set key value should expand to /config set key value without infinite recursion."""
|
||||
cli_obj = _make_cli()
|
||||
with patch.object(cli_obj, 'process_command', wraps=cli_obj.process_command):
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=None), \
|
||||
patch("cli.save_config_value"):
|
||||
cli_obj.model = "current-model"
|
||||
cli_obj.provider = "openrouter"
|
||||
cli_obj.base_url = "https://openrouter.ai/api/v1"
|
||||
cli_obj.api_key = "test"
|
||||
cli_obj._explicit_api_key = None
|
||||
cli_obj._explicit_base_url = None
|
||||
cli_obj.requested_provider = "openrouter"
|
||||
# /mod uniquely matches /model
|
||||
result = cli_obj.process_command("/mod")
|
||||
assert result is True
|
||||
dispatched = []
|
||||
|
||||
original = cli_obj.process_command.__func__
|
||||
|
||||
def counting_process_command(self_inner, cmd):
|
||||
dispatched.append(cmd)
|
||||
if len(dispatched) > 5:
|
||||
raise RecursionError("process_command called too many times")
|
||||
return original(self_inner, cmd)
|
||||
|
||||
with patch.object(type(cli_obj), 'process_command', counting_process_command):
|
||||
try:
|
||||
cli_obj.process_command("/con set key value")
|
||||
except RecursionError:
|
||||
assert False, "process_command recursed infinitely"
|
||||
|
||||
# Should have been called at most twice: once for /con set..., once for /config set...
|
||||
assert len(dispatched) <= 2
|
||||
|
||||
def test_exact_command_with_args_does_not_recurse(self):
|
||||
"""/config set key value hits exact branch and does not loop back to prefix."""
|
||||
cli_obj = _make_cli()
|
||||
call_count = [0]
|
||||
|
||||
original_pc = HermesCLI.process_command
|
||||
|
||||
def guarded(self_inner, cmd):
|
||||
call_count[0] += 1
|
||||
if call_count[0] > 10:
|
||||
raise RecursionError("Infinite recursion detected")
|
||||
return original_pc(self_inner, cmd)
|
||||
|
||||
with patch.object(HermesCLI, 'process_command', guarded):
|
||||
try:
|
||||
cli_obj.process_command("/config set key value")
|
||||
except RecursionError:
|
||||
assert False, "Recursed infinitely on /config set key value"
|
||||
|
||||
assert call_count[0] <= 3
|
||||
|
||||
def test_ambiguous_prefix_shows_suggestions(self):
|
||||
"""/re matches /reset, /retry, /reload-mcp, /reasoning, /rollback — should show suggestions."""
|
||||
"""/re matches multiple commands — should show ambiguous message."""
|
||||
cli_obj = _make_cli()
|
||||
cli_obj.process_command("/re")
|
||||
# Should print ambiguous message, not unknown command
|
||||
printed = " ".join(str(c) for c in cli_obj.console.print.call_args_list)
|
||||
assert "Ambiguous" in printed or "Did you mean" in printed
|
||||
|
||||
@@ -58,3 +83,33 @@ class TestSlashCommandPrefixMatching:
|
||||
with patch.object(cli_obj, 'show_help') as mock_help:
|
||||
cli_obj.process_command("/help")
|
||||
mock_help.assert_called_once()
|
||||
|
||||
def test_skill_command_prefix_matches(self):
|
||||
"""A prefix that uniquely matches a skill command should dispatch it."""
|
||||
cli_obj = _make_cli()
|
||||
fake_skill = {"/test-skill-xyz": {"name": "Test Skill", "description": "test"}}
|
||||
printed = []
|
||||
cli_obj.console.print = lambda *a, **kw: printed.append(str(a))
|
||||
|
||||
import cli as cli_mod
|
||||
with patch.object(cli_mod, '_skill_commands', fake_skill):
|
||||
cli_obj.process_command("/test-skill-xy")
|
||||
|
||||
# Should NOT show "Unknown command" — should have dispatched or attempted skill
|
||||
unknown = any("Unknown command" in p for p in printed)
|
||||
assert not unknown, f"Expected skill prefix to match, got: {printed}"
|
||||
|
||||
def test_ambiguous_between_builtin_and_skill(self):
|
||||
"""Ambiguous prefix spanning builtin + skill commands shows suggestions."""
|
||||
cli_obj = _make_cli()
|
||||
# /help-extra is a fake skill that shares /hel prefix with /help
|
||||
fake_skill = {"/help-extra": {"name": "Help Extra", "description": "test"}}
|
||||
|
||||
import cli as cli_mod
|
||||
with patch.object(cli_mod, '_skill_commands', fake_skill), patch.object(cli_obj, 'show_help') as mock_help:
|
||||
cli_obj.process_command("/help")
|
||||
|
||||
# /help is an exact match so should work normally, not show ambiguous
|
||||
mock_help.assert_called_once()
|
||||
printed = " ".join(str(c) for c in cli_obj.console.print.call_args_list)
|
||||
assert "Ambiguous" not in printed
|
||||
|
||||
Reference in New Issue
Block a user