fix(paper): rebuild root commands instead of mutating dispatcher mirror nodes

[steveb05]

registerCommand does nothing when the command's root is already registered. The command is inserted into cloud's tree and registerCommand returns true, but nothing reaches the server and no exception is raised. New roots register correctly and deleteRootCommand works, so only added subcommands are affected. This has been true since ModernPaperBrigadier was added.

It only affects plugins that set ManagerSetting.ALLOW_UNSAFE_REGISTRATION and register after the COMMANDS lifecycle event. register calls lockRegistration as its first statement, so CommandManager#command throws for everyone else.

This was reported on Discord in January 2025 (https://discord.com/channels/766366162388123678/766381452639731722/1330678321700278313). The reproduction there snapshots commandManager.commands(), calls deleteRootCommand, then re-registers everything from that snapshot in a loop, and only the first command in the loop ends up registered. The same thread also hit a NullPointerException, attributed there to deleteRootCommand, though the throw is really in the other branch of registerCommand, which the delete path never reaches. That second one is a separate missing null check rather than the problem described below.

The handler grabs the root's node out of the API dispatcher and adds the new children to it, on the assumption that the node it got back is what the server dispatches from. It isn't. Commands#getDispatcher() is backed by ApiMirrorRootNode, and its accessors aren't symmetric. addChild unwraps and then forwards to the server root, so writes land. getChild reads from the server root and then wraps, so reads come back as something else:

@Override
public CommandNode<CommandSourceStack> getChild(String name) {
    return this.wrapNode(this.getDispatcher().getRoot().getChild(name));
}

private @Nullable CommandNode<CommandSourceStack> wrapNode(
    @Nullable final CommandNode<net.minecraft.commands.CommandSourceStack> unwrapped
) {
    ...
    if (unwrapped.wrappedCached != null) {
        return unwrapped.wrappedCached;
    }
    CommandNode<CommandSourceStack> shadow = new ShadowBrigNode(unwrapped);
    unwrapped.wrappedCached = shadow;
    return shadow;
}

convertFromPureBrigNode pairs the two graphs with converted.wrappedCached = pureNode; pureNode.unwrappedCached = converted;. So for a root cloud registered, getChild hands back cloud's own API side LiteralCommandNode, which isn't in the server dispatcher at all. Children added to it change nothing that gets parsed, executed, or sent to clients, and nothing throws. Nothing re-syncs the two graphs afterwards either, so the addition is just lost.

The loop only ever visits labels cloud registered itself, so that's the case it hits. Another plugin holding the label through the Paper API gives the same result, since anything registered through ApiMirrorRootNode#addChild gets the pairing set too. You get a ShadowBrigNode back only for a node registered straight onto the server dispatcher with no API counterpart, in practice a vanilla command, and then addChild throws UnsupportedOperationException out of CommandManager#command rather than failing quietly.

That accounts for the reported reproduction. After deleteRootCommand the root has no entry in the alias bookkeeping, so the first registration in the loop takes the other branch and registers properly. Every one after it finds an entry, takes the graft, and is lost. One command survives, whichever went first.

Since a fetched node can't be mutated into the right state, the root has to be rebuilt from cloud's tree and registered again. Paper does the same to its own registrations in PaperCommands#registerIntoDispatcher, which removes the existing child before adding when it overrides, rather than letting Brigadier merge, under the comment // Avoid merging behavior. Maybe something to look into in the future.

Removing first isn't tidiness. registerWithFlagsInternal passes override = true for the primary and namespaced labels, so those two would be replaced in place, but alias labels go through registerCopy with override = false and get refused outright while a node carrying apiCommandMeta still sits on them. Skip the removal and aliases silently stop updating. I remove every recorded label rather than only the alias ones because a label that has dropped out of the root's alias set won't appear in the new registration at all, so nothing would displace it and it would keep answering for a command that no longer has it.

The other branch passed the result of getNamedNode straight into the registration without checking it. That only bites when the handler is driven directly rather than through CommandManager#command, since insertCommand puts the root into the tree before verifyAndRegister runs, so the lookup can't come back null on that route. Drive it directly against a root the tree doesn't hold and you get a NullPointerException on rootNode.component() instead of nothing happening. The lookup is null checked now, which covers the second symptom in the report.

Two things in the diff aren't obviously part of the fix, so to save you asking.

registeredCommands looks like it should go along with the branch it served, and it shouldn't. CommandTree#verifyAndRegister calls back into registerCommand for every leaf of the entire tree on every insert, and that set is the only thing collapsing the replay into one unit of work per genuinely new command. Delete it and N registrations turn into O(N squared) full root rebuilds, each one followed by a command tree broadcast to every online player.

The unregisterRootCommand rework fixes a second silent failure rather than supporting the first. It cleaned its bookkeeping only after two early returns, so a root deleted before the lifecycle event had fired, or one whose label set came back empty, left entries behind in a set that's keyed by identity, since Command declares no equals. Re-registering that same instance later then hit the duplicate check, reported success, and registered nothing.

Nothing in cloud core needed to change. deleteRootCommand and deleteRecursively are fine, and register is fine as it stands, since every COMMANDS fire is preceded by a fresh dispatcher and clearing the alias bookkeeping without removing anything is therefore safe.

Three problems remain.

Labels get removed on the strength of cloud's own bookkeeping, without checking that cloud still owns the node sitting at each one. If another plugin has taken a label over in the meantime, cloud deletes their node and registers its own. The old deletion path did this too, so it's only a question of when it runs.

If the rebuild throws after the labels are gone, the root is left fully unregistered, including commands on it that were working a moment earlier. That one is a regression. The old graft removed nothing, so a failure there left the existing registration intact. It is the price of remove then register and I don't see a way around it. Building the replacement node won't realistically fail, since LiteralBrigadierNodeFactory falls back to StringArgumentType.word() rather than throwing, but ApiMirrorRootNode#convertFromPureBrigNode rejects an unrecognised argument type from inside registerWithFlags, well after the removal. A reflective failure inside the removal loop leaves the labels partly removed, same outcome. Dropping the whole root from registeredCommands on failure at least lets a later insert rebuild it. Actually restoring the previous state would mean holding onto the literal from the last good registration, which I don't do, and rebuilding from the tree would just fail again, because the tree already contains the command that caused the failure.

Cloud's two name comparisons also don't agree on case. getNamedNode matches case insensitively, while CommandComponent#equals and checkAmbiguity compare exactly, so two root literals differing only in case can coexist. CommandComponent#compareTo orders literals by name and insertCommand sorts children, so the lookup resolves both of them to whichever sorts first, deterministically. When that isn't the root the command belongs to, the wrong root gets rebuilt and the new command is silently never registered. Startup avoids it, because register iterates the root nodes directly and registers both. The permission checker built into each root node resolves the same way and has the same blind spot. Fixing it properly means reconciling those two comparisons in core, which I've left alone.

One thing I found while tracing the alias handling that belongs in core rather than here. insertCommand feeds LiteralParser#insertAlias with component.aliases(), which includes the component's own name, and insertAlias files everything it gets under alternativeAliases. So as soon as two commands share a root, that root's own name turns up in its own alias list. Every platform handler reading alternativeAliases() inherits it, not just Paper: VelocityPluginRegistrationHandler, BungeeCommand, BukkitPluginRegistrationHandler and CloudburstPluginRegistrationHandler all do.

On Paper the effect is small. registerWithFlagsInternal assigns apiCommandMeta only after its alias loop, so an alias equal to the primary label sees a child with a null apiCommandMeta, takes it for a vanilla command, and replaces the node that was just registered with a flattened copy carrying an empty alias list. The copy behaves the same, so what's lost is getCommandMap().getCommand(root).getAliases() and the help map's alias index. The fix is for insertCommand to iterate alternativeAliases() instead of aliases(), though that isn't purely a removal: aliases() is backed by a case insensitive set and alternativeAliases isn't, so the switch would start propagating case variant aliases that currently get deduplicated away. Happy to open it separately if you want it.

[jpenilla]

The API-mirror diagnosis is correct.

I’ve addressed the unsafe-registration issue in #173 with a smaller change that adds updated roots through the dispatcher root, so the change reaches Paper’s server dispatcher. It was manually verified on Paper 26.2 for primary, namespaced, alias, and namespaced-alias labels.

Closing in favor of #173.

Please report independent concerns as individual issues before bundling them into a PR. A PR should stay scoped to the changes required for its stated fix; grouping unrelated observations makes triage and review harder, and shifts the work of extracting and splitting them onto maintainers.