Application Command Manager

[Flame442]

Description of the changes

Closes #5672

Checklist for this PR

• [p]slash enable <command> [type] - Enables a specific app command.
• [p]slash disable <command> [type] - Disables a specific app command.
• [p]slash enablecog <cog> - Enables all app commands from a cog.
• [p]slash disablecog <cog> - Disables all app commands from a cog.
• [p]slash list - Lists all app commands the bot can see, and whether they are enabled.
• [p]slash sync - Syncs app commands to discord.
• Slash command error handling.
• Notifications on cog update warning users about modified app commands.
• Documentation
  • Document RedTree.
  • Document new commands.

Future work

• Guild commands
  • Currently this just auto-accepts any command that is in the guild scope. Might want to make guild scoped commands be managed and/or add the ability for guild owners to enable/disable a command at the guild level.
• Translation support
  • I don't really know how to do this, or if it's even really feasible.
• Override app_commands with Red objects, similar to existing overrides for ext.commands.
  • Subclass app_commands.errors.BotMissingPermissions
    • This is necessary if we want permissions to work with slash commands/hybrid commands. Since slash commands can be restricted with built in discord permissions, I don't think it's a major concern to get permissions working with them. Still, it might be something we want to do in the future.
  • Local caching of AppCommand objects (Add functionality to mention app commands #5976)
    • Add an id attribute filled from the config values stored in this PR to the Command subclass.
  • Support commands that are required to be enabled at all times, and will prevent a cog load if they cannot be. (Allow force enabling app commands using flag in extras #6018)
    • Add a kwarg to the @app_commands.command() deco which causes the command to bypass normal load logic.
    • This has the benefits of allowing some commands that a CC denotes as "required", while still supporting a system for non-required commands to be added without entirely blocking a cog from loading.
• Handle slash versions of hybrid commands not working in DMs (Ensure global slash commands don't break in DMs #5995)
  • Either the bot needs to send a message explaining why it isn't responding, or we need to figure out how to get around the PartialMessageable limitation in general.
• Check Red specific global checks in app_command commands. (Add global checks to app commands #6015)
  • block/allowlist settings, possibly the permissions cog, and possibly discord permissions.

Have the changes in this PR been tested?

Lightly

[Luc1412]

You can link #5672 as it will be solved with this PR. Additionally including #5976 could be an option while already implementing a custom tree.

[Kowlin]

This is working great, just gonna wait for a sanity check from TrustyJAID! 👍

[Rapptz]

Regarding the second route of this PR that can break the promise that extensions are never loaded in a partial state, I believe that this is a necessary trade off to enable customization for bot owners and to allow Cog Creators to add app commands to their cogs without concern for the command limits.

I think a good middle ground for this is to provide a tag that would label a command as "essential" and if the essential command is disabled/not there then it fails to load completely. Or it can be flipped so that there's a tag that marks it as "optional".

[Rapptz]

Just a quick glance on things from an API hygiene POV.

From a different POV, I'm not really sure how I feel about the partial loads like I've mentioned before long ago. I get the limits are a thing and there needs to be some sort of solution to the problem. One of the ones I've thought about involve tagging the commands as optional or not and when installing a cog the user can pick which optional components they want to add or have it fail if the slash commands themselves can't fit within the limit. It's a middle ground that allows extension developers to pick what functionality they expect to be there without it being taken from them should they do something with it at runtime.

Anyway these are just random musings. I didn't spend much time on this review, only like 5 minutes or so.

[Rapptz]

Note this method is private so it'll probably break in the future.

[TrustyJAID]

After reading through the comments here I think there are some minor things we can address before this gets merged. @Zephyrkul makes a good point about the promise of partial loading. I still believe this is the best path forward for a modular Red the way this PR currently is but there could be cases where a cog creator may make strictly slash command only cogs and it would be inconvenient to have the cog fail to function because we intervened when attempting to add it. Perhaps it would be best to add a new info.json key that can let a cog author designate that a certain application command(s) must be loaded with this cog when the cog is installed as a middle ground. If that cog happens to add a command that puts the bot over the command limit then fail to load the cog entirely rather than leave the cog in a partial state.

[Drapersniper]

After reading through the comments here I think there are some minor things we can address before this gets merged. @Zephyrkul makes a good point about the promise of partial loading. I still believe this is the best path forward for a modular Red the way this PR currently is but there could be cases where a cog creator may make strictly slash command only cogs and it would be inconvenient to have the cog fail to function because we intervened when attempting to add it. Perhaps it would be best to add a new info.json key that can let a cog author designate that a certain application command(s) must be loaded with this cog when the cog is installed as a middle ground. If that cog happens to add a command that puts the bot over the command limit then fail to load the cog entirely rather than leave the cog in a partial state.

Info.json would work fine for published cogs, perhaps instead we can provide a decorator that tags a command as necessary at the code level that way this would work for both published and unpublished cogs

[TrustyJAID]

Most things seem fine. I would like to see Danny's review addressed as well as a few things here I mentioned.

[Drapersniper]

Another question here, should the slash control commands also be special cased like the licenseinfo commands to allow them to be run via mention?

While I know red doesn't officially supports slash only/public bots this could be a convenient way to allow slash-only/public bots based on red to sync their commands without the message content intent.

[Flame442]

_remove_with_module needs to be overridden in the subclass to get the same functionality it performs for the superclass with the added internal command containers. Technically it could be omitted, but then the cases where a module raises an exception in the loading process, but added commands to the tree would leave dangling objects, which isn't the best. It's understandable that it's internal, however that entire object has to meddle with internals in order to function, so I think it needs to stay.

I know Zeph was pointing out that the current max_concurrency solution can technically be bypassed by someone using [p]slash enable at the same time as [p]slash enablecog. With how fast [p]slash enablecog runs, I would say that doing that is far too much of an edge case to warrant additional logic to prevent that from happening. Yes, it technically could happen, but all that it would cause is a cog to be partially enabled, which is not even a huge deal since it doesn't actually break, just has some number of commands that would not be enabled, which could be viewed with [p]slash list to fix. I personally chalk that up to PEBCAK.

In terms of adding a way to make a command required-enabled in order to load a cog, I would not be comfortable adding that to info.json, partially because non-downloader code would need to be checking that value, and partially because of the fact Draper pointed out that it would not be applicable to local cogs. We are most likely going to need to subclass Command and override the respective app_command decorators (similar to our overrides to ext.commands objects) in order to implement #5976, and it would probably be best to add that as a kwarg to the decorators. The tree can easily read that attribute to skip the normal logic of enabling and disabling. For the same reasons that logic is not included in this PR, that functionality will be included in a followup PR. This PR is already large and hard enough to review as it is.

I have no opinion on special casing [p]slash to work with the mention prefix, so I will wait to add that until further discussion.
EDIT: I forgot that the command can still be used in DMs, as Kowlin pointed out. Since that's still an option, I think that there is no reason to add the override for this command, since it does not need to be always available, it just needs to be available in general.

All other comments up to this point should be addressed at this point.

[Kowlin]

I have no opinion on special casing [p]slash to work with the mention prefix, so I will wait to add that until further discussion.

Theoretically it doesn't hurt making it an always available command, but at the same time. We're talking about a owner only command which is just fine being run in DMs, I expect a bot owner to know their own prefix and not rely solely on slash commands. So for me its a pass on making it always available.

[TrustyJAID]

This looks good to me!

[Kowlin]

Gonna go ahead and merge this. Since all the outstanding issues have been resolved.