mirror of
https://github.com/agessaman/meshcore-bot.git
synced 2026-08-28 23:00:41 +00:00
feat(command_manager, help_command): improve command prefix handling and help command visibility
- Enhanced `CommandManager` to persist normalized message content, preventing prefix stripping from interfering with command matching. - Updated `BaseCommand` to skip prefix handling if already normalized, ensuring all commands can be matched correctly. - Modified `HelpCommand` to ensure unused commands are listed even when command statistics are present, improving user accessibility to all commands. - Added unit tests to verify the correct behavior of command matching and help command functionality under various scenarios.
This commit is contained in:
+3
-1
@@ -106,7 +106,9 @@ node_id =
|
||||
# Command prefix (optional)
|
||||
# If set, all commands must start with this prefix (e.g., "!", ".", "b", "abc")
|
||||
# Examples: "!" for !ping, "." for .ping, "b" for bping, "abc" for abcping
|
||||
# Leave empty or unset to allow commands without prefix (backward compatible)
|
||||
# The prefix applies uniformly to every command (including help); messages without
|
||||
# it are ignored. Leave empty or unset to allow commands without a prefix (backward
|
||||
# compatible; a leading "!" is still accepted in that mode).
|
||||
#command_prefix =
|
||||
|
||||
# Respond to channel messages when the bot is mentioned via @[bot_name]
|
||||
|
||||
@@ -288,6 +288,7 @@ meshcore_log_level = INFO
|
||||
# - path (p): Decode hex path data to show repeaters #
|
||||
# - prefix: Look up repeaters by two-character prefix #
|
||||
# - multitest (mt): Listen for multiple path variations #
|
||||
# - help: List available commands ('help <command>' for details) #
|
||||
# #
|
||||
####################################################################################################
|
||||
|
||||
@@ -412,6 +413,13 @@ enabled = true
|
||||
# Example: "Found {path_count} unique path(s) for @[{sender}]:\n{paths}"
|
||||
response_format = Found {path_count} unique path(s) for @[{sender}]:\n{paths}
|
||||
|
||||
[Help_Command]
|
||||
# Enable or disable the help command
|
||||
# true: Help command is available (responds to 'help'; 'help <command>' for details)
|
||||
# false: Help command is disabled
|
||||
# Recommended to leave enabled so users can discover the commands above.
|
||||
enabled = true
|
||||
|
||||
####################################################################################################
|
||||
# #
|
||||
# All Other Commands Disabled #
|
||||
@@ -421,10 +429,6 @@ response_format = Found {path_count} unique path(s) for @[{sender}]:\n{paths}
|
||||
# #
|
||||
####################################################################################################
|
||||
|
||||
[Help_Command]
|
||||
# Enable or disable the help command
|
||||
enabled = false
|
||||
|
||||
[Cmd_Command]
|
||||
# Enable or disable the cmd command
|
||||
enabled = false
|
||||
|
||||
@@ -705,6 +705,15 @@ class CommandManager:
|
||||
|
||||
content_lower = content.lower()
|
||||
|
||||
# Persist the normalized (prefix-stripped) content to the shared message once,
|
||||
# before iterating commands. Each command's cleanup_message_for_matching would
|
||||
# otherwise re-strip/re-reject the prefix off this same object; the first
|
||||
# keyword command would consume the prefix and break matching for every command
|
||||
# after it. The flag tells per-command cleanup the prefix is already handled.
|
||||
message.content = content
|
||||
message.content_lower = content_lower
|
||||
message.prefix_normalized = True
|
||||
|
||||
# Check for help requests first (special handling)
|
||||
# Check both English "help" and translated help keywords
|
||||
help_keywords = ['help']
|
||||
|
||||
@@ -833,13 +833,18 @@ class BaseCommand(ABC):
|
||||
"""
|
||||
content = message.content.strip()
|
||||
|
||||
if self._command_prefix:
|
||||
if not content.startswith(self._command_prefix):
|
||||
return ""
|
||||
content = content[len(self._command_prefix):].strip()
|
||||
else:
|
||||
if content.startswith('!'):
|
||||
content = content[1:].strip()
|
||||
# When CommandManager.check_keywords has already stripped the configured prefix
|
||||
# (and legacy "!") from this message, skip prefix handling entirely: the content
|
||||
# is canonical and re-stripping/re-rejecting it here would break matching for
|
||||
# every command after the first in the check_keywords scan.
|
||||
if not getattr(message, 'prefix_normalized', False):
|
||||
if self._command_prefix:
|
||||
if not content.startswith(self._command_prefix):
|
||||
return ""
|
||||
content = content[len(self._command_prefix):].strip()
|
||||
else:
|
||||
if content.startswith('!'):
|
||||
content = content[1:].strip()
|
||||
|
||||
mention_mode = self.bot.config.get('Bot', 'respond_to_mentions', fallback='also').strip().lower()
|
||||
if mention_mode != 'false':
|
||||
|
||||
@@ -254,6 +254,14 @@ class HelpCommand(BaseCommand):
|
||||
primary_name = self.bot.command_manager.commands[cmd_name].name if hasattr(self.bot.command_manager.commands[cmd_name], 'name') else cmd_name
|
||||
command_counts[primary_name] = 0
|
||||
|
||||
# Ensure every channel-valid command appears even if it has no usage stats
|
||||
# yet. Without this, a populated command_stats table would make the list show
|
||||
# only previously-used commands (the fallback below only fires when the map
|
||||
# is empty). Unused commands get count 0 and sort after used ones.
|
||||
for pname in primary_names:
|
||||
if pname not in command_counts:
|
||||
command_counts[pname] = 0
|
||||
|
||||
# If we have stats, sort by count descending, otherwise use all commands
|
||||
if command_counts:
|
||||
# Sort by count descending, then by name for consistency
|
||||
|
||||
@@ -31,6 +31,11 @@ class MeshMessage:
|
||||
reply_scope: Optional[str] = None
|
||||
# Lowercased content set by base_command.cleanup_message_for_matching
|
||||
content_lower: str = ""
|
||||
# Transient flag: True once CommandManager.check_keywords has stripped the
|
||||
# configured command prefix (and legacy "!") from content. Prevents per-command
|
||||
# cleanup_message_for_matching from re-stripping/re-rejecting an already-normalized
|
||||
# message, which previously broke matching for all-but-the-first command.
|
||||
prefix_normalized: bool = False
|
||||
|
||||
def effective_outgoing_flood_scope(self, bot: Any) -> str:
|
||||
"""Resolve outbound flood scope the same way as ``CommandManager.send_channel_message``.
|
||||
|
||||
@@ -290,3 +290,79 @@ class TestCommandPrefix:
|
||||
# Should also match with legacy ! prefix
|
||||
mock_message.content = "!test"
|
||||
assert command.matches_keyword(mock_message) is True
|
||||
|
||||
|
||||
class MockAlphaCommand(BaseCommand):
|
||||
"""Second mock command, used to exercise multi-command iteration."""
|
||||
name = "alpha"
|
||||
keywords = ['alpha']
|
||||
description = "Alpha command"
|
||||
category = "test"
|
||||
|
||||
async def execute(self, message: MeshMessage) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
class TestCommandPrefixMultipleCommands:
|
||||
"""Regression tests for issue #137.
|
||||
|
||||
With a configured prefix, the bot must respond to every command, not just the
|
||||
first one in iteration order. The original bug: cleanup_message_for_matching
|
||||
mutated the shared message (stripping the prefix), so the first command consumed
|
||||
the prefix and every later command then failed its own prefix check.
|
||||
"""
|
||||
|
||||
def _make_manager(self, mock_bot, commands):
|
||||
for section in ('Keywords', 'Custom_Syntax'):
|
||||
if not mock_bot.config.has_section(section):
|
||||
mock_bot.config.add_section(section)
|
||||
with patch('modules.command_manager.PluginLoader') as mock_loader_class:
|
||||
loader = Mock()
|
||||
loader.load_all_plugins = Mock(return_value=commands)
|
||||
loader.keyword_mappings = {}
|
||||
mock_loader_class.return_value = loader
|
||||
manager = CommandManager(mock_bot)
|
||||
# Commands resolve channel access via bot.command_manager at call time.
|
||||
mock_bot.command_manager = manager
|
||||
return manager
|
||||
|
||||
def test_non_first_command_matches_with_prefix(self, mock_bot, mock_message):
|
||||
"""A command that is NOT first in iteration order still matches with a prefix."""
|
||||
mock_bot.config.set('Bot', 'command_prefix', '!')
|
||||
# 'alpha' is iterated before 'test'; under the old bug it would strip the
|
||||
# prefix and leave 'test' unable to match.
|
||||
commands = {'alpha': MockAlphaCommand(mock_bot), 'test': MockTestCommand(mock_bot)}
|
||||
manager = self._make_manager(mock_bot, commands)
|
||||
|
||||
mock_message.content = "!test"
|
||||
matches = manager.check_keywords(mock_message)
|
||||
assert any(trigger == 'test' for trigger, _ in matches)
|
||||
|
||||
def test_first_command_still_matches_with_prefix(self, mock_bot, mock_message):
|
||||
"""The first-iterated command continues to match with a prefix."""
|
||||
mock_bot.config.set('Bot', 'command_prefix', '!')
|
||||
commands = {'alpha': MockAlphaCommand(mock_bot), 'test': MockTestCommand(mock_bot)}
|
||||
manager = self._make_manager(mock_bot, commands)
|
||||
|
||||
mock_message.content = "!alpha"
|
||||
matches = manager.check_keywords(mock_message)
|
||||
assert any(trigger == 'alpha' for trigger, _ in matches)
|
||||
|
||||
def test_bare_command_blocked_when_prefix_required(self, mock_bot, mock_message):
|
||||
"""Prefix enforcement still rejects unprefixed messages (now centralized)."""
|
||||
mock_bot.config.set('Bot', 'command_prefix', '!')
|
||||
commands = {'alpha': MockAlphaCommand(mock_bot), 'test': MockTestCommand(mock_bot)}
|
||||
manager = self._make_manager(mock_bot, commands)
|
||||
|
||||
mock_message.content = "test"
|
||||
assert manager.check_keywords(mock_message) == []
|
||||
|
||||
def test_multiple_commands_match_without_prefix(self, mock_bot, mock_message):
|
||||
"""Without a prefix, a non-first command still matches (no regression)."""
|
||||
mock_bot.config.set('Bot', 'command_prefix', '')
|
||||
commands = {'alpha': MockAlphaCommand(mock_bot), 'test': MockTestCommand(mock_bot)}
|
||||
manager = self._make_manager(mock_bot, commands)
|
||||
|
||||
mock_message.content = "test"
|
||||
matches = manager.check_keywords(mock_message)
|
||||
assert any(trigger == 'test' for trigger, _ in matches)
|
||||
|
||||
@@ -481,6 +481,56 @@ class TestGetAvailableCommandsList:
|
||||
result = cmd.get_available_commands_list()
|
||||
assert "ping" in result
|
||||
|
||||
def test_unused_commands_listed_when_stats_present(self):
|
||||
"""Regression for issue #137: commands with no usage stats must still appear.
|
||||
|
||||
Previously, once command_stats had any rows, only commands present in the
|
||||
table were listed, so never-used commands were missing from help.
|
||||
"""
|
||||
from contextlib import contextmanager
|
||||
|
||||
bot = _make_bot()
|
||||
conn = _create_tracked_connection()
|
||||
conn.execute("""
|
||||
CREATE TABLE command_stats (
|
||||
id INTEGER PRIMARY KEY,
|
||||
timestamp INTEGER,
|
||||
sender_id TEXT,
|
||||
command_name TEXT,
|
||||
channel TEXT,
|
||||
is_dm BOOLEAN,
|
||||
response_sent BOOLEAN
|
||||
)
|
||||
""")
|
||||
# Only 'ping' has been used; 'version' has never been used.
|
||||
conn.execute("INSERT INTO command_stats (timestamp, sender_id, command_name, channel, is_dm, response_sent) VALUES (1, 'u1', 'ping', 'general', 0, 1)")
|
||||
conn.commit()
|
||||
|
||||
db = MagicMock()
|
||||
|
||||
@contextmanager
|
||||
def _conn_ctx():
|
||||
yield conn
|
||||
|
||||
db.connection = _conn_ctx
|
||||
bot.db_manager = db
|
||||
|
||||
mock_ping = MagicMock()
|
||||
mock_ping.name = "ping"
|
||||
mock_version = MagicMock()
|
||||
mock_version.name = "version"
|
||||
bot.command_manager.commands = {"ping": mock_ping, "version": mock_version}
|
||||
bot.command_manager.plugin_loader.keyword_mappings = {
|
||||
"ping": "ping",
|
||||
"version": "version",
|
||||
}
|
||||
|
||||
cmd = HelpCommand(bot)
|
||||
result = cmd.get_available_commands_list()
|
||||
# The used command appears first, but the unused one must also be listed.
|
||||
assert "ping" in result
|
||||
assert "version" in result
|
||||
|
||||
def test_db_exception_falls_back_gracefully(self):
|
||||
"""If DB raises, falls back to sorted command names."""
|
||||
bot = _make_bot()
|
||||
|
||||
Reference in New Issue
Block a user