From f1835adbfbdc55b73582a4086a0fc86e5f593998 Mon Sep 17 00:00:00 2001 From: steve <50219120+steveb05@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:55:12 +0200 Subject: [PATCH] fix(paper): rebuild root commands instead of mutating dispatcher mirror nodes Registering a command under an already registered root added its children to the node returned by Commands#getDispatcher().getRoot().getChild(). That node is not the one the server dispatches from. ApiMirrorRootNode#getChild reads from the server root and then wraps, returning the API side node paired in convertFromPureBrigNode, so the addition never reached the dispatcher and no exception was raised. Rebuild the root from the command tree and register it again through registerWithFlags instead, removing the labels first because registerCopy refuses an alias label while a node carrying apiCommandMeta occupies it. Also null check the getNamedNode result that previously threw a NullPointerException when the handler was driven directly, and clean registeredCommands unconditionally in unregisterRootCommand so a stale entry cannot turn a later registration of the same Command instance into a silent success that registers nothing. --- .../cloud/paper/ModernPaperBrigadier.java | 150 ++++++++++-------- 1 file changed, 87 insertions(+), 63 deletions(-) diff --git a/cloud-paper/src/main/java/org/incendo/cloud/paper/ModernPaperBrigadier.java b/cloud-paper/src/main/java/org/incendo/cloud/paper/ModernPaperBrigadier.java index bc270d90..868aaf2b 100644 --- a/cloud-paper/src/main/java/org/incendo/cloud/paper/ModernPaperBrigadier.java +++ b/cloud-paper/src/main/java/org/incendo/cloud/paper/ModernPaperBrigadier.java @@ -23,7 +23,6 @@ // package org.incendo.cloud.paper; -import com.mojang.brigadier.CommandDispatcher; import com.mojang.brigadier.tree.LiteralCommandNode; import com.mojang.brigadier.tree.RootCommandNode; import io.papermc.paper.command.brigadier.CommandRegistrationFlag; @@ -41,7 +40,6 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Consumer; -import java.util.function.Function; import java.util.logging.Logger; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; @@ -135,19 +133,20 @@ private void register(final ReloadableRegistrarEvent event) { this.aliases.clear(); for (final CommandNode rootNode : this.manager.commandTree().rootNodes()) { - this.registerCommand(commands, rootNode); + this.registerRoot(commands, rootNode); } } - private void registerCommand(final Commands commands, final CommandNode rootNode) { + private void registerRoot(final Commands commands, final CommandNode rootNode) { + final String rootName = rootNode.component().name(); final Set registered = commands.registerWithFlags( this.metaHolder.owningPluginMeta(), - this.createRootNode(rootNode, rootNode.component().name()), + this.createRootNode(rootNode, rootName), this.findBukkitDescription(rootNode), new ArrayList<>(rootNode.component().alternativeAliases()), new HashSet<>(Collections.singletonList(CommandRegistrationFlag.FLATTEN_ALIASES)) ); - this.aliases.put(rootNode.component().name(), registered); + this.aliases.put(rootName, registered); } private LiteralCommandNode createRootNode(final CommandNode rootNode, final String label) { @@ -204,81 +203,113 @@ public boolean registerCommand(final @NonNull Command command) { return true; } - if (this.aliases.containsKey(command.rootComponent().name())) { - final CommandDispatcher dispatcher = - unsafeGet(commands, Commands::getDispatcher); - final Set registered = this.aliases.get(command.rootComponent().name()); - final LiteralCommandNode newRoot = this.createRootNode( - this.manager.commandTree().getNamedNode(command.rootComponent().name()), - command.rootComponent().name() - ); - for (final String label : registered) { - final com.mojang.brigadier.tree.CommandNode node = - dispatcher.getRoot().getChild(label); - for (final com.mojang.brigadier.tree.CommandNode newChild : newRoot.getChildren()) { - node.addChild(newChild); - } + try { + this.syncRoot(commands, command.rootComponent().name()); + } catch (final RuntimeException e) { + try { + this.resendCommands(); + } catch (final RuntimeException resendFailure) { + e.addSuppressed(resendFailure); } - } else { - unsafeOperation(commands, cmds -> this.registerCommand( - cmds, - this.manager.commandTree().getNamedNode(command.rootComponent().name()) - )); + throw e; } this.resendCommands(); + return true; + } - final @Nullable Set registered = this.aliases.get(command.rootComponent().name()); + /** + * Rebuilds the root literal that {@code rootName} resolves to and replaces its registration. + * + *

The nodes handed out by the API dispatcher are mirrors of the server nodes rather than the server nodes + * themselves, so adding children to a node fetched from the dispatcher does not change what the server dispatches or + * sends to clients. The entire root has to be unregistered and registered again for a change to take effect.

+ * + *

If the registration fails the root is left unregistered, so every command belonging to it is dropped from + * {@link #registeredCommands} to make a later insert able to restore it.

+ * + * @param commands the registrar to operate on + * @param rootName a name or alias of the root literal + */ + private void syncRoot(final Commands commands, final String rootName) { + final @Nullable CommandNode rootNode = this.manager.commandTree().getNamedNode(rootName); + if (rootNode == null) { + this.forgetRoot(rootName); + return; + } - boolean ret = registered != null && !registered.isEmpty(); - if (!ret) { - this.registeredCommands.remove(command); + final String resolvedName = rootNode.component().name(); + try { + unsafeOperation(commands, cmds -> { + this.removeRootLabels(cmds, resolvedName); + this.registerRoot(cmds, rootNode); + }); + } catch (final RuntimeException e) { + this.forgetRoot(resolvedName); + throw e; } - return ret; } - private static @MonotonicNonNull Method commandnodeRemoveMethod = null; + /** + * Drops every command belonging to the root literal named {@code rootName} from {@link #registeredCommands}. + * + * @param rootName the primary name of the root literal + */ + private void forgetRoot(final String rootName) { + this.registeredCommands.removeIf(registered -> registered.rootComponent().name().equals(rootName)); + } - private void unregisterRoot(final Commands commands, final String label) { - final @Nullable Set removed = this.aliases.remove(label); - if (removed == null || removed.isEmpty()) { - return; - } - this.registeredCommands.removeIf(command -> command.rootComponent().name().equals(label)); + private static @MonotonicNonNull Method commandNodeRemoveMethod = null; - try { - if (commandnodeRemoveMethod == null) { - commandnodeRemoveMethod = com.mojang.brigadier.tree.CommandNode.class.getMethod( + private static @NonNull Method commandNodeRemoveMethod() { + if (commandNodeRemoveMethod == null) { + try { + commandNodeRemoveMethod = com.mojang.brigadier.tree.CommandNode.class.getMethod( "removeCommand", String.class ); - commandnodeRemoveMethod.setAccessible(true); + commandNodeRemoveMethod.setAccessible(true); + } catch (final ReflectiveOperationException e) { + throw new RuntimeException("Failed to find removeCommand method", e); } - } catch (final ReflectiveOperationException e) { - throw new RuntimeException("Failed to find removeCommand method", e); } + return commandNodeRemoveMethod; + } - unsafeOperation(commands, cmds -> { - final CommandDispatcher dispatcher = cmds.getDispatcher(); - final RootCommandNode root = dispatcher.getRoot(); - for (final String removedLabel : removed) { - try { - commandnodeRemoveMethod.invoke(root, removedLabel); - } catch (final ReflectiveOperationException e) { - throw new RuntimeException("Failed to delete node " + removedLabel, e); - } + /** + * Removes every label that the root literal named {@code rootName} is currently registered under. + * + *

Must be called from within {@link #unsafeOperation(Commands, Consumer)}.

+ * + * @param commands the registrar to operate on + * @param rootName the primary name of the root literal + */ + private void removeRootLabels(final Commands commands, final String rootName) { + final @Nullable Set removed = this.aliases.remove(rootName); + if (removed == null) { + return; + } + + final RootCommandNode root = commands.getDispatcher().getRoot(); + for (final String label : removed) { + try { + commandNodeRemoveMethod().invoke(root, label); + } catch (final ReflectiveOperationException e) { + throw new RuntimeException("Failed to delete node " + label, e); } - }); + } } @Override public void unregisterRootCommand(final @NonNull CommandComponent rootCommand) { + final String rootName = rootCommand.name(); + this.forgetRoot(rootName); + final @Nullable Commands commands = this.commands; if (commands == null) { return; } - this.unregisterRoot(commands, rootCommand.name()); - + unsafeOperation(commands, cmds -> this.removeRootLabels(cmds, rootName)); this.resendCommands(); } @@ -291,13 +322,6 @@ private void resendCommands() { private static @MonotonicNonNull Field commandsInvalidField = null; private static void unsafeOperation(final Commands commands, final Consumer task) { - unsafeGet(commands, cmds -> { - task.accept(cmds); - return null; - }); - } - - private static T unsafeGet(final Commands commands, final Function task) { try { if (commandsInvalidField == null) { commandsInvalidField = commands.getClass().getDeclaredField("invalid"); @@ -306,7 +330,7 @@ private static T unsafeGet(final Commands commands, final Function