From 24fdfb23703eeeebe9e8162622862b618356b2d7 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 28 Jun 2026 08:42:58 -0700 Subject: [PATCH] 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. --- config.ini.example | 4 +- config.ini.minimal-example | 12 +++-- modules/command_manager.py | 9 ++++ modules/commands/base_command.py | 19 +++++--- modules/commands/help_command.py | 8 ++++ modules/models.py | 5 +++ tests/test_command_prefix.py | 76 ++++++++++++++++++++++++++++++++ tests/test_help_command.py | 50 +++++++++++++++++++++ 8 files changed, 171 insertions(+), 12 deletions(-) diff --git a/config.ini.example b/config.ini.example index 53b6aef..ec2f4f3 100644 --- a/config.ini.example +++ b/config.ini.example @@ -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] diff --git a/config.ini.minimal-example b/config.ini.minimal-example index 1fdfcd4..3ffe896 100644 --- a/config.ini.minimal-example +++ b/config.ini.minimal-example @@ -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 ' 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 ' 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 diff --git a/modules/command_manager.py b/modules/command_manager.py index 64f7d1d..1a41466 100644 --- a/modules/command_manager.py +++ b/modules/command_manager.py @@ -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'] diff --git a/modules/commands/base_command.py b/modules/commands/base_command.py index 0ab127b..506f19d 100644 --- a/modules/commands/base_command.py +++ b/modules/commands/base_command.py @@ -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': diff --git a/modules/commands/help_command.py b/modules/commands/help_command.py index f0c9904..cb5d13c 100644 --- a/modules/commands/help_command.py +++ b/modules/commands/help_command.py @@ -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 diff --git a/modules/models.py b/modules/models.py index c4c49cc..badb256 100644 --- a/modules/models.py +++ b/modules/models.py @@ -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``. diff --git a/tests/test_command_prefix.py b/tests/test_command_prefix.py index c699fc0..a8a966a 100644 --- a/tests/test_command_prefix.py +++ b/tests/test_command_prefix.py @@ -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) diff --git a/tests/test_help_command.py b/tests/test_help_command.py index 8051e09..3a77acb 100644 --- a/tests/test_help_command.py +++ b/tests/test_help_command.py @@ -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()