Answer every wrong command invocation

cogs/music.py

@@ -269,21 +269,8 @@ async def autoplay(self, ctx):
 
     # ── Errors & lifecycle ────────────────────────────────────────────
 
-    @play.error
-    async def play_error(self, ctx, error):
-        if isinstance(error, commands.MissingRequiredArgument):
-            await ctx.send(embed=error_embed("Please provide a song name or URL.\nUsage: `!play <song>`"))
-        elif isinstance(error, commands.CommandOnCooldown):
-            await ctx.send(embed=error_embed(
-                f"Slow down — try again in {error.retry_after:.1f}s."
-            ))
-        elif isinstance(error, commands.MaxConcurrencyReached):
-            await ctx.send(embed=error_embed("You already have a `!play` in progress."))
-
-    @volume.error
-    async def volume_error(self, ctx, error):
-        if isinstance(error, commands.BadArgument):
-            await ctx.send(embed=error_embed("Please provide a number between 0 and 100."))
+    # Missing/bad arguments, cooldowns and concurrency limits are handled
+    # centrally in utils.errors, so every command reports them the same way.
 
     @commands.Cog.listener()
     async def on_voice_state_update(self, member, before, after):

main.py

@@ -6,6 +6,7 @@
 
 import config
 from config import DISCORD_TOKEN, COMMAND_PREFIX, COGS
+from utils import errors
 
 config.configure_logging()
 config.validate()
@@ -40,13 +41,8 @@ async def on_ready():
 
 @bot.event
 async def on_command_error(ctx, error):
-    # Unwrap the original exception where discord.py wrapped it.
-    error = getattr(error, "original", error)
-    if isinstance(error, (commands.CommandNotFound, commands.CheckFailure)):
-        return                # unknown command / checks send their own errors
-    if isinstance(error, commands.MissingRequiredArgument):
-        return                # per-command handlers deal with these
-    log.warning("Error in command %s: %s", ctx.command, error)
+    # All of it lives in utils.errors so it can be tested without a gateway.
+    await errors.handle(ctx, error)
 
 
 # ── Custom help command ───────────────────────────────────────────────

tests/test_errors.py

@@ -0,0 +1,206 @@
+"""
+Central command-error handling.
+
+The rule under test: a command invoked wrongly must always say so. A silent
+failure is indistinguishable from the bot being down.
+"""
+
+import inspect
+from unittest.mock import AsyncMock, MagicMock
+
+import discord
+import pytest
+from discord.ext import commands
+
+from utils import errors
+
+
+@pytest.fixture
+def ctx():
+    c = MagicMock()
+    c.send = AsyncMock()
+    c.clean_prefix = "!"
+    c.command = MagicMock()
+    c.command.qualified_name = "volume"
+    c.command.signature = "<vol>"
+    c.command.has_error_handler.return_value = False
+    return c
+
+
+def sent_text(ctx) -> str:
+    assert ctx.send.await_count == 1, f"expected exactly one reply, got {ctx.send.await_count}"
+    embed = ctx.send.await_args.kwargs["embed"]
+    return embed.description or ""
+
+
+def missing_arg(name: str = "vol") -> commands.MissingRequiredArgument:
+    # discord.py has its own Parameter type, distinct from inspect.Parameter.
+    param = commands.Parameter(name=name, kind=inspect.Parameter.POSITIONAL_OR_KEYWORD)
+    return commands.MissingRequiredArgument(param)
+
+
+# -- usage strings -----------------------------------------------------
+
+def test_usage_includes_prefix_name_and_signature(ctx):
+    assert errors.usage(ctx) == "!volume <vol>"
+
+
+def test_usage_omits_the_signature_when_a_command_takes_no_arguments(ctx):
+    ctx.command.qualified_name = "skip"
+    ctx.command.signature = ""
+    assert errors.usage(ctx) == "!skip"
+
+
+def test_usage_is_empty_for_an_unknown_command(ctx):
+    ctx.command = None
+    assert errors.usage(ctx) == ""
+
+
+def test_usage_follows_a_custom_prefix(ctx):
+    ctx.clean_prefix = "?"
+    assert errors.usage(ctx) == "?volume <vol>"
+
+
+# -- the bug: silent failures ------------------------------------------
+
+async def test_a_missing_argument_is_explained(ctx):
+    """`!volume` with no number used to produce total silence."""
+    await errors.handle(ctx, missing_arg("vol"))
+    text = sent_text(ctx)
+    assert "vol" in text
+    assert "!volume <vol>" in text
+
+
+async def test_a_bad_argument_is_explained(ctx):
+    """`!queue abc` used to be logged server-side and never answered."""
+    ctx.command.qualified_name = "queue"
+    ctx.command.signature = "[page]"
+    await errors.handle(ctx, commands.BadArgument("not an int"))
+    assert "!queue [page]" in sent_text(ctx)
+
+
+async def test_too_many_arguments_is_explained(ctx):
+    await errors.handle(ctx, commands.TooManyArguments())
+    assert "!volume <vol>" in sent_text(ctx)
+
+
+async def test_any_other_user_input_error_still_gets_a_reply(ctx):
+    """UserInputError has many subclasses; none of them may go unanswered."""
+    await errors.handle(ctx, commands.UserInputError("something odd"))
+    assert "!volume <vol>" in sent_text(ctx)
+
+
+@pytest.mark.parametrize("command_name,signature", [
+    ("volume", "<vol>"),
+    ("remove", "<index>"),
+    ("move", "<from_pos> <to_pos>"),
+    ("play", "<query>"),
+])
+async def test_every_argument_taking_command_reports_its_usage(ctx, command_name, signature):
+    ctx.command.qualified_name = command_name
+    ctx.command.signature = signature
+    await errors.handle(ctx, missing_arg())
+    assert f"!{command_name} {signature}" in sent_text(ctx)
+
+
+# -- generic failures that apply to any command ------------------------
+
+async def test_cooldown_reports_the_wait(ctx):
+    error = commands.CommandOnCooldown(MagicMock(), retry_after=2.5, type=MagicMock())
+    await errors.handle(ctx, error)
+    assert "2.5" in sent_text(ctx)
+
+
+async def test_max_concurrency_names_the_command(ctx):
+    ctx.command.qualified_name = "play"
+    error = commands.MaxConcurrencyReached(number=1, per=MagicMock())
+    await errors.handle(ctx, error)
+    assert "!play" in sent_text(ctx)
+
+
+# -- things that must stay quiet ---------------------------------------
+
+async def test_an_unknown_command_is_ignored(ctx):
+    """People type a prefix by accident; the bot must not nag."""
+    await errors.handle(ctx, commands.CommandNotFound())
+    ctx.send.assert_not_awaited()
+
+
+async def test_a_failed_check_is_ignored_because_checks_explain_themselves(ctx):
+    await errors.handle(ctx, commands.CheckFailure())
+    ctx.send.assert_not_awaited()
+
+
+async def test_a_command_with_its_own_handler_is_left_alone(ctx):
+    """Otherwise the user gets the same complaint twice."""
+    ctx.command.has_error_handler.return_value = True
+    await errors.handle(ctx, missing_arg())
+    ctx.send.assert_not_awaited()
+
+
+async def test_an_unexpected_error_is_logged_not_shown(ctx, caplog):
+    """Internal failures are the operator's problem, not the user's."""
+    await errors.handle(ctx, RuntimeError("something broke internally"))
+    ctx.send.assert_not_awaited()
+    assert "something broke internally" in caplog.text
+
+
+async def test_the_original_exception_is_unwrapped(ctx):
+    """discord.py wraps command exceptions in CommandInvokeError."""
+    wrapped = commands.CommandInvokeError(missing_arg())
+    await errors.handle(ctx, wrapped)
+    assert "!volume <vol>" in sent_text(ctx)
+
+
+# -- resilience --------------------------------------------------------
+
+async def test_a_send_failure_does_not_escalate(ctx):
+    """No permission to post is not a reason to blow up the error handler."""
+    ctx.send.side_effect = discord.Forbidden(MagicMock(status=403), "no perms")
+    await errors.handle(ctx, missing_arg())      # must not raise
+
+
+async def test_handling_survives_a_command_with_no_context(ctx):
+    ctx.command = None
+    await errors.handle(ctx, missing_arg())      # must not raise
+
+
+# -- against the real commands, not mocks ------------------------------
+
+@pytest.mark.parametrize("name,expected", [
+    ("play",   "!play <query>"),
+    ("volume", "!volume <vol>"),
+    ("remove", "!remove <index>"),
+    ("move",   "!move <from_pos> <to_pos>"),
+    ("queue",  "!queue [page=1]"),
+    ("loop",   "!loop [mode=track]"),
+    ("skip",   "!skip"),
+])
+async def test_usage_matches_the_real_command_definitions(ctx, name, expected):
+    """
+    Renaming a parameter must not silently produce a wrong usage hint, so this
+    reads the signature off the actual Command objects.
+    """
+    from cogs.music import Music
+
+    command = getattr(Music, name)
+    ctx.command = MagicMock()
+    ctx.command.qualified_name = command.qualified_name
+    ctx.command.signature = command.signature
+    ctx.command.has_error_handler.return_value = False
+
+    await errors.handle(ctx, missing_arg())
+    assert expected in sent_text(ctx)
+
+
+def test_no_music_command_keeps_a_private_error_handler():
+    """
+    A leftover per-command handler would suppress the central one and bring the
+    silent-failure bug back for that command.
+    """
+    from cogs.music import Music
+
+    with_handlers = [
+        c.qualified_name for c in Music.__cog_commands__ if c.has_error_handler()
+    ]
+    assert with_handlers == []

utils/errors.py

@@ -0,0 +1,81 @@
+"""
+Central command-error handling.
+
+Every wrong invocation gets an answer. A command that fails silently is
+indistinguishable from the bot being offline, so the only errors that stay
+quiet here are the two where silence is correct: an unrecognised command (people
+type the prefix by accident) and a failed check (checks explain themselves).
+
+Generic failures that can happen to *any* command — missing arguments, bad
+types, cooldowns — are handled here rather than per-command, so behaviour stays
+consistent as commands are added.
+"""
+
+import logging
+
+import discord
+from discord.ext import commands
+
+from utils.embeds import error_embed
+
+log = logging.getLogger("loopify.errors")
+
+
+def usage(ctx) -> str:
+    """
+    How the command should have been invoked, e.g. ``!move <from_pos> <to_pos>``.
+
+    ``signature`` renders required parameters as ``<name>`` and optional ones as
+    ``[name]``, so the hint stays correct as commands change.
+    """
+    command = ctx.command
+    if command is None:
+        return ""
+    text = f"{ctx.clean_prefix}{command.qualified_name}"
+    return f"{text} {command.signature}" if command.signature else text
+
+
+def _input_detail(error: commands.UserInputError) -> str:
+    """A one-line explanation of what was wrong with the arguments."""
+    if isinstance(error, commands.MissingRequiredArgument):
+        return f"Missing the `{error.param.name}` argument."
+    if isinstance(error, commands.TooManyArguments):
+        return "That command takes fewer arguments than you gave it."
+    if isinstance(error, commands.BadArgument):
+        return "One of those arguments isn't the right type."
+    return "I couldn't make sense of that."
+
+
+async def handle(ctx, error: Exception) -> None:
+    """Reply to the user, or log, depending on what went wrong."""
+    # discord.py wraps exceptions raised inside a command body.
+    error = getattr(error, "original", error)
+
+    if isinstance(error, commands.CommandNotFound):
+        return
+    if isinstance(error, commands.CheckFailure):
+        return                      # the check already sent its own message
+    if ctx.command is not None and ctx.command.has_error_handler():
+        return                      # the command replied itself; don't repeat it
+
+    if isinstance(error, commands.CommandOnCooldown):
+        return await _reply(ctx, f"Slow down — try again in {error.retry_after:.1f}s.")
+
+    if isinstance(error, commands.MaxConcurrencyReached):
+        name = usage(ctx).split(" ", 1)[0] or "that command"
+        return await _reply(ctx, f"You already have a `{name}` in progress.")
+
+    if isinstance(error, commands.UserInputError):
+        hint = f"\nUsage: `{usage(ctx)}`" if ctx.command is not None else ""
+        return await _reply(ctx, f"{_input_detail(error)}{hint}")
+
+    # Anything left is a bug or an outage — the operator's problem, not the
+    # user's. Log it with a traceback and stay quiet in the channel.
+    log.warning("Error in command %s: %s", ctx.command, error, exc_info=error)
+
+
+async def _reply(ctx, message: str) -> None:
+    try:
+        await ctx.send(embed=error_embed(message))
+    except (discord.HTTPException, discord.Forbidden) as e:
+        log.debug("Could not report an error to the channel: %s", e)