Centralise error handling for commands invoked from error handler (#2436)

2 files changed, 30 insertions, 46 deletions

diff --git a/bot/exts/backend/error_handler.py b/bot/exts/backend/error_handler.py
index 0c8938918..c1e3eb70e 100644
--- a/bot/exts/backend/error_handler.py
+++ b/bot/exts/backend/error_handler.py
@@ -64,11 +64,24 @@ class ErrorHandler(Cog):
         )
 
         if isinstance(e, errors.CommandNotFound) and not getattr(ctx, "invoked_from_error_handler", False):
-            if await self.try_silence(ctx):
-                return
-            if await self.try_run_fixed_codeblock(ctx):
-                return
-            await self.try_get_tag(ctx)  # Try to look for a tag with the command's name
+            # We might not invoke a command from the error handler, but it's easier and safer to ensure
+            # this is always set rather than trying to get it exact, and shouldn't cause any issues.
+            ctx.invoked_from_error_handler = True
+
+            # All errors from attempting to execute these commands should be handled by the error handler.
+            # We wrap non CommandErrors in CommandInvokeError to mirror the behaviour of normal commands.
+            try:
+                if await self.try_silence(ctx):
+                    return
+                if await self.try_run_fixed_codeblock(ctx):
+                    return
+                await self.try_get_tag(ctx)
+            except Exception as err:
+                log.info("Re-handling error raised by command in error handler")
+                if isinstance(err, errors.CommandError):
+                    await self.on_command_error(ctx, err)
+                else:
+                    await self.on_command_error(ctx, errors.CommandInvokeError(err))
         elif isinstance(e, errors.UserInputError):
             log.debug(debug_message)
             await self.handle_user_input_error(ctx, e)
@@ -123,7 +136,6 @@ class ErrorHandler(Cog):
 
         command = ctx.invoked_with.lower()
         args = ctx.message.content.lower().split(" ")
-        ctx.invoked_from_error_handler = True
 
         try:
             if not await silence_command.can_run(ctx):
@@ -160,13 +172,7 @@ class ErrorHandler(Cog):
         return False
 
     async def try_get_tag(self, ctx: Context) -> None:
-        """
-        Attempt to display a tag by interpreting the command name as a tag name.
-
-        The invocation of tags get respects its checks. Any CommandErrors raised will be handled
-        by `on_command_error`, but the `invoked_from_error_handler` attribute will be added to
-        the context to prevent infinite recursion in the case of a CommandNotFound exception.
-        """
+        """Attempt to display a tag by interpreting the command name as a tag name."""
         tags_cog = self.bot.get_cog("Tags")
         if not tags_cog:
             log.debug("Not attempting to parse message as a tag as could not find `Tags` cog.")
@@ -177,23 +183,19 @@ class ErrorHandler(Cog):
         if not maybe_tag_name or not isinstance(ctx.author, Member):
             return
 
-        ctx.invoked_from_error_handler = True
         try:
             if not await self.bot.can_run(ctx):
                 log.debug("Cancelling attempt to fall back to a tag due to failed checks.")
                 return
+        except errors.CommandError:
+            log.debug("Cancelling attempt to fall back to a tag due to failed checks.")
+            return
 
-            if await tags_get_command(ctx, maybe_tag_name):
-                return
+        if await tags_get_command(ctx, maybe_tag_name):
+            return
 
-            if not any(role.id in MODERATION_ROLES for role in ctx.author.roles):
-                await self.send_command_suggestion(ctx, maybe_tag_name)
-        except Exception as err:
-            log.debug("Error while attempting to invoke tag fallback.")
-            if isinstance(err, errors.CommandError):
-                await self.on_command_error(ctx, err)
-            else:
-                await self.on_command_error(ctx, errors.CommandInvokeError(err))
+        if not any(role.id in MODERATION_ROLES for role in ctx.author.roles):
+            await self.send_command_suggestion(ctx, maybe_tag_name)
 
     async def try_run_fixed_codeblock(self, ctx: Context) -> bool:
         """
diff --git a/tests/bot/exts/backend/test_error_handler.py b/tests/bot/exts/backend/test_error_handler.py
index 0ba2fcf11..e28d5cfd0 100644
--- a/tests/bot/exts/backend/test_error_handler.py
+++ b/tests/bot/exts/backend/test_error_handler.py
@@ -54,12 +54,15 @@ class ErrorHandlerTests(unittest.IsolatedAsyncioTestCase):
                 self.ctx.reset_mock()
                 self.cog.try_silence.reset_mock(return_value=True)
                 self.cog.try_get_tag.reset_mock()
+                self.ctx.invoked_from_error_handler = False
 
                 self.cog.try_silence.return_value = case["try_silence_return"]
                 self.ctx.channel.id = 1234
 
                 self.assertIsNone(await self.cog.on_command_error(self.ctx, error))
 
+                self.assertTrue(self.ctx.invoked_from_error_handler)
+
                 if case["try_silence_return"]:
                     self.cog.try_get_tag.assert_not_awaited()
                     self.cog.try_silence.assert_awaited_once()
@@ -203,13 +206,6 @@ class TrySilenceTests(unittest.IsolatedAsyncioTestCase):
         self.ctx = MockContext(bot=self.bot, guild=self.guild)
         self.cog = error_handler.ErrorHandler(self.bot)
 
-    async def test_try_silence_context_invoked_from_error_handler(self):
-        """Should set `Context.invoked_from_error_handler` to `True`."""
-        self.ctx.invoked_with = "foo"
-        await self.cog.try_silence(self.ctx)
-        self.assertTrue(hasattr(self.ctx, "invoked_from_error_handler"))
-        self.assertTrue(self.ctx.invoked_from_error_handler)
-
     async def test_try_silence_get_command(self):
         """Should call `get_command` with `silence`."""
         self.ctx.invoked_with = "foo"
@@ -342,26 +338,12 @@ class TryGetTagTests(unittest.IsolatedAsyncioTestCase):
         await self.cog.try_get_tag(self.ctx)
         self.bot.get_cog.assert_called_once_with("Tags")
 
-    async def test_try_get_tag_invoked_from_error_handler(self):
-        """`self.ctx` should have `invoked_from_error_handler` `True`."""
-        self.ctx.invoked_from_error_handler = False
-        await self.cog.try_get_tag(self.ctx)
-        self.assertTrue(self.ctx.invoked_from_error_handler)
-
     async def test_try_get_tag_no_permissions(self):
         """Test how to handle checks failing."""
         self.bot.can_run = AsyncMock(return_value=False)
         self.ctx.invoked_with = "foo"
         self.assertIsNone(await self.cog.try_get_tag(self.ctx))
 
-    async def test_try_get_tag_command_error(self):
-        """Should call `on_command_error` when `CommandError` raised."""
-        err = errors.CommandError()
-        self.bot.can_run = AsyncMock(side_effect=err)
-        self.cog.on_command_error = AsyncMock()
-        self.assertIsNone(await self.cog.try_get_tag(self.ctx))
-        self.cog.on_command_error.assert_awaited_once_with(self.ctx, err)
-
     async def test_dont_call_suggestion_tag_sent(self):
         """Should never call command suggestion if tag is already sent."""
         self.ctx.message = MagicMock(content="foo")
@@ -375,7 +357,7 @@ class TryGetTagTests(unittest.IsolatedAsyncioTestCase):
     async def test_dont_call_suggestion_if_user_mod(self):
         """Should not call command suggestion if user is a mod."""
         self.ctx.invoked_with = "foo"
-        self.ctx.invoke = AsyncMock(return_value=False)
+        self.tag.get_command_ctx = AsyncMock(return_value=False)
         self.ctx.author.roles = [MockRole(id=1234)]
         self.cog.send_command_suggestion = AsyncMock()