🤖 test: full-login validation matrix and loopback provisioner guard (LK-18 PR2)

lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java

@@ -36,7 +36,9 @@
 
 import java.time.Duration;
 import java.util.Objects;
+import java.util.Set;
 import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
@@ -66,6 +68,11 @@ final class AgentPlayerActions
      * NMS-backed synthetic player implementation.
      */
     private final IBotPlayerNmsAdapter botPlayerNmsAdapter;
+    /**
+     * UUIDs of synthetic players that joined through the FULL_LOGIN pipeline and therefore own a real TCP
+     * connection; their removal must run the real disconnect flow instead of the legacy player-list removal.
+     */
+    private final Set<UUID> fullLoginPlayerIds = ConcurrentHashMap.newKeySet();
 
     /**
      * @param plugin
@@ -368,6 +375,7 @@ private void registerJoinedPlayer(Player player, CreatePlayer.Command command)
             player.teleport(target);
 
             playerStore.registerSyntheticPlayer(uuid, player);
+            fullLoginPlayerIds.add(uuid);
             if (health != null)
                 player.setHealth(Math.min(player.getMaxHealth(), health));
             if (permissionsCsv != null && !permissionsCsv.isBlank())
@@ -393,9 +401,7 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command)
         mainThreadExecutor.callOnMainThread(() ->
         {
             final Player player = playerStore.getRequiredPlayer(uuid);
-            playerStore.removePermissionAttachment(uuid, player);
-            botPlayerNmsAdapter.removePlayer(player);
-            playerStore.removeSyntheticPlayer(uuid);
+            removeRegisteredPlayer(uuid, player);
             plugin.getLogger().info(
                 "LK_AGENT: Removed synthetic player '%s' (%s)."
                     .formatted(player.getName(), player.getUniqueId())
@@ -406,6 +412,50 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command)
         return new RemovePlayer.Response();
     }
 
+    /**
+     * Removes a registered synthetic player through the path matching how it joined (main thread only).
+     *
+     * <p>A {@code FULL_LOGIN} bot owns a real TCP connection, so it must leave through the real disconnect
+     * flow ({@code Player#kickPlayer}): the play-phase disconnect packet reaches the login driver, the channel
+     * closes, and {@code PlayerQuitEvent} fires on the regular quit path. Removing such a bot through the
+     * legacy player-list removal would leave its connection open — the server times it out ~30 seconds later
+     * and runs the removal a second time, which crashes Paper ({@code EntityScheduler}: "Already retired").
+     * Legacy-spawn bots have no real connection and keep the reflective removal path.
+     *
+     * <p>{@code PlayerKickEvent} is cancellable on both distros, so a vetoing plugin would silently turn the
+     * kick into a no-op while the agent reports success — a permanent ghost bot whose driver keeps answering
+     * keep-alives. The disconnect completes synchronously inside {@code kickPlayer} on both jars, so the
+     * {@code isOnline()} probe below reliably reflects the outcome; a vetoed kick fails loud BEFORE any agent
+     * state is mutated, keeping the removal cleanly retryable.
+     *
+     * @param uuid
+     *     UUID of the player to remove.
+     * @param player
+     *     The registered player instance.
+     * @throws AgentProtocolException
+     *     If a plugin cancelled the {@code PlayerKickEvent} of a full-login bot's removal.
+     */
+    private void removeRegisteredPlayer(UUID uuid, Player player)
+    {
+        if (fullLoginPlayerIds.contains(uuid))
+        {
+            player.kickPlayer("LightKeeper bot removed.");
+            if (player.isOnline())
+                throw new AgentProtocolException(
+                    AgentErrorCode.REQUEST_FAILED,
+                    ("PlayerKickEvent was cancelled by a plugin: full-login bot '%s' (%s) is still online and "
+                        + "was NOT removed.").formatted(player.getName(), uuid));
+            fullLoginPlayerIds.remove(uuid);
+            playerStore.removePermissionAttachment(uuid, player);
+        }
+        else
+        {
+            playerStore.removePermissionAttachment(uuid, player);
+            botPlayerNmsAdapter.removePlayer(player);
+        }
+        playerStore.removeSyntheticPlayer(uuid);
+    }
+
     /**
      * Handles {@code EXECUTE_PLAYER_COMMAND} by dispatching the command in the player's execution context.
      *
@@ -638,9 +688,7 @@ void cleanupSyntheticPlayers()
             try
             {
                 final Player player = playerStore.getRequiredPlayer(uuid);
-                playerStore.removePermissionAttachment(uuid, player);
-                botPlayerNmsAdapter.removePlayer(player);
-                playerStore.removeSyntheticPlayer(uuid);
+                removeRegisteredPlayer(uuid, player);
             }
             catch (Exception exception)
             {

lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java

@@ -20,6 +20,7 @@
 import nl.pim16aap2.lightkeeper.protocol.MutatePlayerPermission;
 import nl.pim16aap2.lightkeeper.protocol.PlacePlayerBlock;
 import nl.pim16aap2.lightkeeper.protocol.PlayerChat;
+import nl.pim16aap2.lightkeeper.protocol.RemovePlayer;
 import nl.pim16aap2.lightkeeper.protocol.RightClickBlock;
 import nl.pim16aap2.lightkeeper.protocol.TeleportPlayer;
 import org.bukkit.Bukkit;
@@ -28,12 +29,15 @@
 import org.bukkit.block.Block;
 import org.bukkit.block.BlockFace;
 import org.bukkit.entity.Player;
+import org.bukkit.event.Listener;
 import org.bukkit.event.block.Action;
 import org.bukkit.event.player.PlayerDropItemEvent;
 import org.bukkit.event.player.PlayerInteractEvent;
+import org.bukkit.event.player.PlayerJoinEvent;
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.PlayerInventory;
 import org.bukkit.permissions.PermissionAttachment;
+import org.bukkit.plugin.EventExecutor;
 import org.bukkit.plugin.PluginManager;
 import org.bukkit.plugin.java.JavaPlugin;
 import org.bukkit.scheduler.BukkitScheduler;
@@ -672,17 +676,117 @@ void handleCreatePlayer_shouldSurfaceMissingJoinEventAsTypedTimeout()
         }
     }
 
+    @Test
+    void handleRemovePlayer_shouldKickFullLoginBotInsteadOfLegacyRemoval()
+        throws Exception
+    {
+        // setup — drive a successful FULL_LOGIN join (the stubbed plugin manager fires PlayerJoinEvent on
+        // registration) so the bot is tracked as owning a real connection.
+        final PlayerActionsFixture fixture = createPlayerActionsFixture();
+        final IBotLoginDriver loginDriver = mock();
+        when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver);
+        when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot"));
+        final UUID uuid = UUID.randomUUID();
+        final Player player = mock();
+        when(player.getUniqueId()).thenReturn(uuid);
+        when(player.getName()).thenReturn("fullbot");
+        final CreatePlayer.Command createCommand = new CreatePlayer.Command(
+            "request-full-rm", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN, null);
+
+        try (MockedStatic<Bukkit> bukkit = mockStatic(Bukkit.class))
+        {
+            stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player));
+            final CreatePlayer.Response created = fixture.playerActions().handleCreatePlayer(createCommand);
+            assertThat(created.uuid()).isEqualTo(uuid);
+
+            // execute
+            fixture.playerActions().handleRemovePlayer(new RemovePlayer.Command("request-remove-full", uuid));
+
+            // verify — the bot leaves through the real disconnect flow (kick), never the legacy player-list
+            // removal that would leave its TCP connection open (Paper crashes on the second removal when the
+            // server times that connection out).
+            verify(player).kickPlayer(anyString());
+            verify(fixture.nmsAdapter(), never()).removePlayer(any());
+        }
+    }
+
+    @Test
+    void handleRemovePlayer_shouldFailLoudWhenKickOfFullLoginBotIsCancelled()
+        throws Exception
+    {
+        // setup — a kick-cancelling plugin (PlayerKickEvent is cancellable on both distros) leaves the bot
+        // online after kickPlayer returns; the agent must fail loud instead of reporting a successful removal.
+        final PlayerActionsFixture fixture = createPlayerActionsFixture();
+        final IBotLoginDriver loginDriver = mock();
+        when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver);
+        when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot"));
+        final UUID uuid = UUID.randomUUID();
+        final Player player = mock();
+        when(player.getUniqueId()).thenReturn(uuid);
+        when(player.getName()).thenReturn("fullbot");
+        when(player.isOnline()).thenReturn(true);
+        final CreatePlayer.Command createCommand = new CreatePlayer.Command(
+            "request-full-veto", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN,
+            null);
+
+        try (MockedStatic<Bukkit> bukkit = mockStatic(Bukkit.class))
+        {
+            stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player));
+            fixture.playerActions().handleCreatePlayer(createCommand);
+
+            // execute
+            final Throwable thrown = catchThrowable(() -> fixture.playerActions()
+                .handleRemovePlayer(new RemovePlayer.Command("request-remove-veto", uuid)));
+
+            // verify — typed failure naming the veto, and no agent state mutated: the bot stays registered
+            // (the removal is retryable) and the legacy removal path is never used as a fallback.
+            assertThat(thrown)
+                .isInstanceOf(AgentProtocolException.class)
+                .satisfies(failure -> assertThat(((AgentProtocolException) failure).errorCode())
+                    .isEqualTo(AgentErrorCode.REQUEST_FAILED))
+                .hasMessageContaining("PlayerKickEvent was cancelled");
+            verify(player).kickPlayer(anyString());
+            verify(fixture.nmsAdapter(), never()).removePlayer(any());
+            assertThat(fixture.playerStore().getRequiredPlayer(uuid)).isSameAs(player);
+        }
+    }
+
+    /**
+     * Creates a plugin-manager mock that immediately fires a {@code PlayerJoinEvent} for the given player
+     * whenever an event executor is registered, completing a FULL_LOGIN join's join-latch synchronously.
+     */
+    private static PluginManager joinEventFiringPluginManager(Player player)
+    {
+        final PluginManager pluginManager = mock();
+        doAnswer(invocation ->
+        {
+            final Listener listener = invocation.getArgument(1);
+            final EventExecutor eventExecutor = invocation.getArgument(3);
+            eventExecutor.execute(listener, new PlayerJoinEvent(player, "joined"));
+            return null;
+        }).when(pluginManager).registerEvent(eq(PlayerJoinEvent.class), any(), any(), any(), any());
+        return pluginManager;
+    }
+
     /**
      * Stubs the Bukkit statics a FULL_LOGIN join touches: NOT the primary thread (the handler rejects
      * main-thread execution), a server exposing a port, a known world named {@code "world"}, a no-op plugin
      * manager, and a scheduler that runs main-thread callables inline.
      */
     private static void stubFullLoginBukkitStatics(MockedStatic<Bukkit> bukkit)
+    {
+        stubFullLoginBukkitStatics(bukkit, mock(PluginManager.class));
+    }
+
+    /**
+     * Variant of {@link #stubFullLoginBukkitStatics(MockedStatic)} with a caller-supplied plugin manager, for
+     * tests that need event registrations to actually fire events.
+     */
+    private static void stubFullLoginBukkitStatics(MockedStatic<Bukkit> bukkit, PluginManager pluginManager)
     {
         final org.bukkit.Server server = mock();
         when(server.getPort()).thenReturn(25_565);
         final World world = mock();
-        final PluginManager pluginManager = mock();
         final BukkitScheduler scheduler = mock();
         when(scheduler.callSyncMethod(any(), any())).thenAnswer(invocation ->
         {

lightkeeper-integration-tests/pom.xml

@@ -108,6 +108,15 @@
                             <version>${project.version}</version>
                             <renameTo>lightkeeper-spigot-test-plugin.jar</renameTo>
                         </plugin>
+                        <!-- LuckPerms provisions the Permissible-injection survival check for FULL_LOGIN bots
+                             (LightkeeperFullLoginSessionIT). Version pinned for deterministic provisioning. -->
+                        <plugin>
+                            <sourceType>modrinth</sourceType>
+                            <modrinthProject>luckperms</modrinthProject>
+                            <modrinthVersion>v5.5.53-bukkit</modrinthVersion>
+                            <modrinthGameVersion>${lightkeeper.minecraft-version}</modrinthGameVersion>
+                            <renameTo>LuckPerms.jar</renameTo>
+                        </plugin>
                     </plugins>
                     <worlds>
                         <world>

lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java

@@ -0,0 +1,89 @@
+package nl.pim16aap2.lightkeeper.maven.test;
+
+import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot;
+import nl.pim16aap2.lightkeeper.framework.EntitySnapshot;
+import nl.pim16aap2.lightkeeper.framework.Vec3;
+import nl.pim16aap2.lightkeeper.framework.WorldHandle;
+import nl.pim16aap2.lightkeeper.protocol.IProtocolValue;
+
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.Optional;
+import java.util.UUID;
+
+import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat;
+import static org.assertj.core.api.Assertions.within;
+
+/**
+ * Shared assertions and lookups for the FULL_LOGIN validation-matrix integration tests.
+ *
+ * <p>Kept as plain static helpers so both matrix classes ({@code LightkeeperFullLoginValidationIT} and
+ * {@code LightkeeperFullLoginSessionIT}) assert identities, positions, and captured events the same way.
+ */
+final class FullLoginItSupport
+{
+    private FullLoginItSupport()
+    {
+    }
+
+    /**
+     * Computes the offline-mode UUID the server derives from a player name during a {@code FULL_LOGIN} join.
+     *
+     * @param name
+     *     The player name.
+     * @return The server-derived offline UUID.
+     */
+    static UUID offlineUuid(String name)
+    {
+        return UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(StandardCharsets.UTF_8));
+    }
+
+    /**
+     * Looks up a player's current position through the live entity query.
+     *
+     * @param world
+     *     World to query.
+     * @param playerId
+     *     UUID of the player to find.
+     * @return The player's position, or empty when the player is not online in the world.
+     */
+    static Optional<Vec3> playerPosition(WorldHandle world, UUID playerId)
+    {
+        return world.entities().ofType("minecraft:player").snapshot().stream()
+            .filter(snapshot -> snapshot.uuid().equals(playerId))
+            .map(EntitySnapshot::position)
+            .findFirst();
+    }
+
+    /**
+     * Asserts that a position matches the expected coordinates within half a block on every axis.
+     *
+     * @param actual
+     *     Observed position.
+     * @param expected
+     *     Expected position.
+     */
+    static void assertPositionCloseTo(Vec3 actual, Vec3 expected)
+    {
+        assertThat(actual.x()).isCloseTo(expected.x(), within(0.5));
+        assertThat(actual.y()).isCloseTo(expected.y(), within(0.5));
+        assertThat(actual.z()).isCloseTo(expected.z(), within(0.5));
+    }
+
+    /**
+     * Filters captured events down to those whose {@code getPlayer} value references the given player.
+     *
+     * @param events
+     *     Captured event snapshots.
+     * @param playerId
+     *     UUID of the acting player.
+     * @return The events attributed to the player, in capture order.
+     */
+    static List<CapturedEventSnapshot> eventsWithPlayerRef(List<CapturedEventSnapshot> events, UUID playerId)
+    {
+        return events.stream()
+            .filter(event -> event.value("getPlayer") instanceof IProtocolValue.PRef playerRef
+                && playerRef.id().equals(playerId.toString()))
+            .toList();
+    }
+}