Incendo / Incendo/cloud-minecraft

[Bug] Cloud Doesn't Throw Exceptions for Brigadier Registered Commands

Open
#110 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
110
Forks
29
PR merge metrics
No merged PRs in 30d

Description

Cloud's exception handlers do not handle/process misspelt brigadier commands and throw an InvalidSyntaxException as you would expect.

Packet logging and debugging confirms the commands are being sent by the client. This bug exists in both Bukkit/Spigot/Paper and Modded, I will be using Fabric and its associated mappings below because it was easier to debug to explain this issue in death.

For commands that exist (not misspelt) they are executed here in ContextChain.java

    public static <S> int runExecutable(final CommandContext<S> executable, final S source, final ResultConsumer<S> resultConsumer, final boolean forkedMode) throws CommandSyntaxException {
        final CommandContext<S> contextToUse = executable.copyFor(source);
        try {
            final int result = executable.getCommand().run(contextToUse); // Cloud is run by this line over here

            resultConsumer.onCommandComplete(contextToUse, true, result);
            return forkedMode ? 1 : result;
        } catch (final CommandSyntaxException ex) {
            resultConsumer.onCommandComplete(contextToUse, false, 0);
            if (forkedMode) {
                return 0;
            }
            throw ex;
        }
    }

Which calls and executes https://github.com/Incendo/cloud-minecraft/blob/master/cloud-brigadier/src/main/java/org/incendo/cloud/brigadier/CloudBrigadierCommand.java#L94 with .run()

The stack trace for processing the command server side looks something like this

handleHelp:24, GrimHelp (ac.grim.grimac.command.commands)
execute:-1, GrimHelp$$Lambda/0x00007e230cdb8900 (ac.grim.grimac.command.commands)
executeFuture:91, CommandExecutionHandler (org.incendo.cloud.execution)
lambda$coordinateExecution$4:121, ExecutionCoordinatorImpl (org.incendo.cloud.execution)
apply:-1, ExecutionCoordinatorImpl$$Lambda/0x00007e230d8f97c8 (org.incendo.cloud.execution)
tryFire:1150, CompletableFuture$UniCompose (java.util.concurrent)
run:482, CompletableFuture$Completion (java.util.concurrent)
execute:53, ExecutionCoordinatorImpl$NonSchedulingExecutor (org.incendo.cloud.execution)
uniComposeStage:1184, CompletableFuture (java.util.concurrent)
thenComposeAsync:2352, CompletableFuture (java.util.concurrent)
coordinateExecution:104, ExecutionCoordinatorImpl (org.incendo.cloud.execution)
executeCommand:91, StandardCommandExecutor (org.incendo.cloud)
executeCommand:65, StandardCommandExecutor (org.incendo.cloud)
run:94, CloudBrigadierCommand (org.incendo.cloud.brigadier) <--- **Cloud entrypoint**
runExecutable:73, ContextChain (com.mojang.brigadier.context)
execute:26, FixedCommandAction (net.minecraft.command)
execute:-1, FixedCommandAction (net.minecraft.command)
method_54405:8, SourcedCommandAction (net.minecraft.command)
execute:-1, SourcedCommandAction$$Lambda/0x00007e230d8f2360 (net.minecraft.command)
execute:5, CommandQueueEntry (net.minecraft.command)
run:101, CommandExecutionContext (net.minecraft.command)
callWithContext:294, CommandManager (net.minecraft.server.command)
execute:223, CommandManager (net.minecraft.server.command) <----**Here is where successful execution vs misspelt commands differ**
executeCommand:1307, ServerPlayNetworkHandler (net.minecraft.server.network)
method_44356:1296, ServerPlayNetworkHandler (net.minecraft.server.network)
run:-1, ServerPlayNetworkHandler$$Lambda/0x00007e230d8eb988 (net.minecraft.server.network)
run:17, ServerTask (net.minecraft.server)
executeTask:144, ThreadExecutor (net.minecraft.util.thread)
executeTask:24, ReentrantThreadExecutor (net.minecraft.util.thread)
executeTask:935, MinecraftServer (net.minecraft.server)
executeTask:-1, MinecraftServer (net.minecraft.server)
runTask:118, ThreadExecutor (net.minecraft.util.thread)
runOneTask:918, MinecraftServer (net.minecraft.server)
runTask:912, MinecraftServer (net.minecraft.server)
runTasks:128, ThreadExecutor (net.minecraft.util.thread)
runTasks:877, MinecraftServer (net.minecraft.server)
runTasksTillTickEnd:885, MinecraftServer (net.minecraft.server)
runServer:769, MinecraftServer (net.minecraft.server)
method_29739:299, MinecraftServer (net.minecraft.server)
run:-1, MinecraftServer$$Lambda/0x00007e230d2a1e18 (net.minecraft.server)
runWith:1596, Thread (java.lang)
run:1583, Thread (java.lang)

The difference is here:

* Executes {@code command}. The command cannot be prefixed with a slash.
     * 
     * @see #executeWithPrefix(ServerCommandSource, String)
     */
    public void execute(ParseResults<ServerCommandSource> parseResults, String command) {
        ServerCommandSource serverCommandSource = parseResults.getContext().getSource();
        Profilers.get().push((Supplier<String>)(() -> "/" + command));
        ContextChain<ServerCommandSource> contextChain = checkCommand(parseResults, command, serverCommandSource); 

        try { // **for a command that exists contextChain is not null, if no command exists it returns null**
            if (contextChain != null) { 
                callWithContext(
                    serverCommandSource, context -> CommandExecutionContext.enqueueCommand(context, command, contextChain, serverCommandSource, ReturnValueConsumer.EMPTY) // **this eventually leads to /grim help execution**
                );
            }
        } catch (Exception var12) {
            MutableText mutableText = Text.literal(var12.getMessage() == null ? var12.getClass().getName() : var12.getMessage());
            if (LOGGER.isDebugEnabled()) {
                LOGGER.error("Command exception: /{}", command, var12);
                StackTraceElement[] stackTraceElements = var12.getStackTrace();

                for (int i = 0; i < Math.min(stackTraceElements.length, [3](https://mclo.gs/TYuWdgE#L3)); i++) {
                    mutableText.append("\n\n")
                        .append(stackTraceElements[i].getMethodName())
                        .append("\n ")
                        .append(stackTraceElements[i].getFileName())
                        .append(":")
                        .append(String.valueOf(stackTraceElements[i].getLineNumber()));
                }
            }

            serverCommandSource.sendError(Text.translatable("command.failed").styled(style -> style.withHoverEvent(new HoverEvent(Action.SHOW_TEXT, mutableText))));
            if (SharedConstants.isDevelopment) {
                serverCommandSource.sendError(Text.literal(Util.getInnermostMessage(var[12](https://mclo.gs/TYuWdgE#L12))));
                LOGGER.error("'/{}' threw an exception", command, var12);
            }
        } finally {
            Profilers.get().pop();
        }
    }

Ordinarily I'd make a PR and fix this on my own but I'm not familiar enough with Cloud to even begin to know how to fix this nicely in a way that would get upstreamed and I would appreciate some help in this matter.

  1. How does cloud-brigadier register itself to handler these commands exactly?
  2. How should we intercept execution of these non-existence/failed commands so that clouds exception handlers can take over them?

On modded platforms a simple Mixin will do to grab the parsed input when the contextChain is null and then just run it through cloud as a command but I'm not sure sure on Bukkit or other platforms, and I'm not even sure if that's ideal. Is there another cloud method or class we should be looking at to just directly parse only for exceptions or raise the exception with the input or will that break other things? Should we find a different place to try and hook into to fix this behaviour?

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with cloud-brigadier/src/main/java/org/incendo/cloud/brigadier/CloudBrigadierCommand.java and trace its run() entry point through Brigadier's ContextChain.runExecutable and the server CommandManager.execute/checkCommand flow. Reproduce a misspelled command on the affected platforms and determine how the existing Cloud exception handlers can process it when contextChain is null, with completion verified by the expected exception handling behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
cli
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.