feat(mod): enforce cmd response ordering and termination

4 files changed, 81 insertions, 8 deletions

diff --git a/mod/README.md b/mod/README.md
index d84a718..1e23817 100644
--- a/mod/README.md
+++ b/mod/README.md
@@ -212,7 +212,7 @@ Parameters:
 
 **Command result:**
 
-When the command finishes without issues (?), Acron will send the following response:
+When the command finishes, Acron will send the following response:
 
 ```json
 {
@@ -225,11 +225,12 @@ When the command finishes without issues (?), Acron will send the following resp
 
 All parameters always present.
 
-> **Note**
+> **Important:**
 > 
-> The result completely depends on Minecraft server's response.
-> It may not be reliable, and the values of `.result` and `.success` are
-> undocumented.
+> For `cmd` requests, the response will be either a single `error`, or a series of:
+> single `ok` (Successfully scheduled) -> zero or more `cmd_out` (Output) -> single `cmd_result` (Final result)
+> The response will always end with either error or cmd_result.
+> After that, the server should not return anything with the same ID.
 
 ### Receiving Messages
 
diff --git a/mod/src/main/java/moe/ymc/acron/cmd/CmdResConsumer.java b/mod/src/main/java/moe/ymc/acron/cmd/CmdResConsumer.java
index d22b77e..36f7605 100644
--- a/mod/src/main/java/moe/ymc/acron/cmd/CmdResConsumer.java
+++ b/mod/src/main/java/moe/ymc/acron/cmd/CmdResConsumer.java
@@ -16,6 +16,8 @@ public class CmdResConsumer implements ResultConsumer<ServerCommandSource> {
     private final @NotNull Channel channel;
     private final int id;
 
+    private boolean hasResult;
+
     public CmdResConsumer(@NotNull Channel channel,
                   int id) {
         this.channel = channel;
@@ -28,6 +30,20 @@ public class CmdResConsumer implements ResultConsumer<ServerCommandSource> {
                 id,
                 success,
                 result);
-        channel.writeAndFlush(Serializer.serialize(new EventCmdRes(id, success, result)));
+        // Ignore any failed results because CommandDispatcher#execute does not reliably send error
+        // results to the consumer.
+        // For example, fail results are either not sent at all (pre-processing errors), or
+        // sent before writing the result, which both violates the spec.
+        // We will send the failed results in a custom mixin.
+        if (success) {
+            hasResult = true;
+            channel.writeAndFlush(Serializer.serialize(new EventCmdRes(id, success, result)));
+        }
+    }
+
+    public void sendResultIfNot() {
+        if (!hasResult) {
+            channel.writeAndFlush(Serializer.serialize(new EventCmdRes(id, false, -1)));
+        }
     }
 }
diff --git a/mod/src/main/java/moe/ymc/acron/cmd/CmdSrc.java b/mod/src/main/java/moe/ymc/acron/cmd/CmdSrc.java
index 983b4ed..9fde8a4 100644
--- a/mod/src/main/java/moe/ymc/acron/cmd/CmdSrc.java
+++ b/mod/src/main/java/moe/ymc/acron/cmd/CmdSrc.java
@@ -1,24 +1,34 @@
 package moe.ymc.acron.cmd;
 
+import com.mojang.brigadier.ResultConsumer;
 import io.netty.channel.Channel;
 import moe.ymc.acron.net.ClientConfiguration;
 import net.minecraft.command.argument.EntityAnchorArgumentType;
+import net.minecraft.entity.Entity;
 import net.minecraft.server.MinecraftServer;
+import net.minecraft.server.command.CommandOutput;
 import net.minecraft.server.command.ServerCommandSource;
+import net.minecraft.server.world.ServerWorld;
 import net.minecraft.text.LiteralText;
+import net.minecraft.text.Text;
+import net.minecraft.util.math.Vec2f;
+import net.minecraft.util.math.Vec3d;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 
-class CmdSrc extends ServerCommandSource {
+public class CmdSrc extends ServerCommandSource {
     private static final Logger LOGGER = LogManager.getLogger();
 
+    private final CmdResConsumer resConsumer;
+
     public CmdSrc(@NotNull Channel channel,
                      int id,
                      boolean display,
                      @NotNull ClientConfiguration configuration,
                      @NotNull MinecraftServer server) {
-        super(new CmdOut(channel, id, display),
+        this(new CmdOut(channel, id, display),
                 configuration.pos(),
                 configuration.rot(),
                 configuration.world(),
@@ -31,4 +41,35 @@ class CmdSrc extends ServerCommandSource {
                 new CmdResConsumer(channel, id),
                 EntityAnchorArgumentType.EntityAnchor.FEET);
     }
+
+    public CmdSrc(CommandOutput output,
+                  Vec3d pos,
+                  Vec2f rot,
+                  ServerWorld world,
+                  int level,
+                  String name,
+                  Text displayName,
+                  MinecraftServer server,
+                  @Nullable Entity entity,
+                  boolean silent,
+                  CmdResConsumer consumer,
+                  EntityAnchorArgumentType.EntityAnchor entityAnchor) {
+        super(output,
+                pos,
+                rot,
+                world,
+                level,
+                name,
+                displayName,
+                server,
+                entity,
+                silent,
+                consumer,
+                entityAnchor);
+        this.resConsumer = consumer;
+    }
+
+    public void sendResultIfNot() {
+        resConsumer.sendResultIfNot();
+    }
 }
diff --git a/mod/src/main/java/moe/ymc/acron/mixin/CommandManagerMixin.java b/mod/src/main/java/moe/ymc/acron/mixin/CommandManagerMixin.java
index 9aaed2e..ccde6f0 100644
--- a/mod/src/main/java/moe/ymc/acron/mixin/CommandManagerMixin.java
+++ b/mod/src/main/java/moe/ymc/acron/mixin/CommandManagerMixin.java
@@ -1,6 +1,7 @@
 package moe.ymc.acron.mixin;
 
 import com.mojang.brigadier.CommandDispatcher;
+import moe.ymc.acron.cmd.CmdSrc;
 import moe.ymc.acron.config.ConfigReloadCmd;
 import net.minecraft.server.command.CommandManager;
 import net.minecraft.server.command.ServerCommandSource;
@@ -12,6 +13,7 @@ import org.spongepowered.asm.mixin.Shadow;
 import org.spongepowered.asm.mixin.injection.At;
 import org.spongepowered.asm.mixin.injection.Inject;
 import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
 
 import static net.minecraft.server.command.CommandManager.literal;
 
@@ -35,4 +37,17 @@ public abstract class CommandManagerMixin {
                 )
         );
     }
+
+    @Inject(at = @At(value = "RETURN"),
+    method = "execute")
+    // Unfortunately we cannot inject into the catch blocks, so
+    // a cursed approach would be always sending the result if we
+    // haven't.
+    public void execute(ServerCommandSource commandSource,
+                        String command,
+                        CallbackInfoReturnable<Integer> cir) {
+        if (commandSource instanceof CmdSrc src) {
+            src.sendResultIfNot();
+        }
+    }
 }