From 42ce3feb332388cc6ff8a322670c3f7451ab769d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:57:13 +0000 Subject: [PATCH 1/3] Initial plan From c048d0c79ba2d788918194ef1a722444a5d4cd0c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:14:37 +0000 Subject: [PATCH 2/3] Fix repeating message bug in island/team chat and shared-state risk in IslandChatCommand --- .../commands/island/IslandChatCommand.java | 17 ++--- .../chat/listeners/ChatListenerTest.java | 63 +++++++++++++++++++ 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java index bad6444..e22dab6 100644 --- a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java +++ b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java @@ -2,8 +2,6 @@ import java.util.List; -import org.eclipse.jdt.annotation.Nullable; - import world.bentobox.bentobox.api.commands.CompositeCommand; import world.bentobox.bentobox.api.user.User; import world.bentobox.bentobox.database.objects.Island; @@ -14,8 +12,6 @@ */ public class IslandChatCommand extends CompositeCommand { - private @Nullable Island island; - public IslandChatCommand(Chat addon, CompositeCommand parent, String label) { super(addon, parent, label); } @@ -30,15 +26,22 @@ public void setup() { @Override public boolean canExecute(User user, String label, List args) { - - island = this.getIslands().getIslandAt(user.getLocation()).orElse(null); - return island != null; + // Command instances are shared across all players, so the resolved island must not be + // cached on an instance field here - it is recomputed per-player in execute() instead. + return this.getIslands().getIslandAt(user.getLocation()).isPresent(); } @Override public boolean execute(User user, String label, List args) { Chat addon = this.getAddon(); + // Resolve the island fresh for this specific invocation, since this command + // instance is shared by all players and must not rely on state set in canExecute(). + Island island = this.getIslands().getIslandAt(user.getLocation()).orElse(null); + if (island == null) { + return false; + } + // Send the message directly into island chat without the need of toggling it // if there is existence of more arguments if (!args.isEmpty()) { diff --git a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java index 0501a88..cae965d 100644 --- a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java +++ b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java @@ -271,6 +271,69 @@ public void testOnChatIslandChatPlayerNotRegistered() { assertFalse(event.isCancelled()); } + // ----------------------------------------------------------------------- + // onChat asynchronous tests - regression tests for repeating message bug + // ----------------------------------------------------------------------- + + /** + * Stubs the scheduler so that {@code callSyncMethod} runs the given callable + * immediately (as if the next server tick had already happened) and returns + * an already completed future, mimicking the real Bukkit scheduler behaviour + * closely enough for the async {@link ChatListener#onChat(AsyncPlayerChatEvent)} + * code path to be exercised in a unit test. + */ + private void stubSyncScheduler() { + when(sch.callSyncMethod(any(), any())).thenAnswer(invocation -> { + java.util.concurrent.Callable callable = invocation.getArgument(1); + try { + return java.util.concurrent.CompletableFuture.completedFuture(callable.call()); + } catch (Exception ex) { + throw new RuntimeException(ex); + } + }); + } + + @Test + public void testOnChatAsyncIslandChatUsesCurrentMessageNotStale() { + stubSyncScheduler(); + + ChatListener spyListener = org.mockito.Mockito.spy(listener); + spyListener.toggleIslandChat(island, player); + when(im.getIslandAt(any())).thenReturn(Optional.of(island)); + + // First async chat message + AsyncPlayerChatEvent event1 = new AsyncPlayerChatEvent(true, player, "aaaa", Collections.emptySet()); + spyListener.onChat(event1); + assertTrue(event1.isCancelled()); + verify(spyListener).islandChat(island, player, "aaaa"); + + // Second async chat message must use its own content, not the previous one + AsyncPlayerChatEvent event2 = new AsyncPlayerChatEvent(true, player, "test", Collections.emptySet()); + spyListener.onChat(event2); + assertTrue(event2.isCancelled()); + verify(spyListener).islandChat(island, player, "test"); + verify(spyListener, org.mockito.Mockito.never()).islandChat(island, player, "aaaa test"); + } + + @Test + public void testOnChatAsyncTeamChatUsesCurrentMessageNotStale() { + stubSyncScheduler(); + + ChatListener spyListener = org.mockito.Mockito.spy(listener); + spyListener.togglePlayerTeamChat(uuid); + when(im.inTeam(any(World.class), any(UUID.class))).thenReturn(true); + + AsyncPlayerChatEvent event1 = new AsyncPlayerChatEvent(true, player, "aaaa", Collections.emptySet()); + spyListener.onChat(event1); + assertTrue(event1.isCancelled()); + verify(spyListener).teamChat(world, player, "aaaa"); + + AsyncPlayerChatEvent event2 = new AsyncPlayerChatEvent(true, player, "test", Collections.emptySet()); + spyListener.onChat(event2); + assertTrue(event2.isCancelled()); + verify(spyListener).teamChat(world, player, "test"); + } + // ----------------------------------------------------------------------- // teamChat tests // ----------------------------------------------------------------------- From 6fa0dcb138779b764fb83709c524bc5e31770743 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:16:35 +0000 Subject: [PATCH 3/3] Address code review feedback: simplify test assertion, clarify comment --- .../world/bentobox/chat/commands/island/IslandChatCommand.java | 3 ++- .../java/world/bentobox/chat/listeners/ChatListenerTest.java | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java index e22dab6..b0db53e 100644 --- a/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java +++ b/src/main/java/world/bentobox/chat/commands/island/IslandChatCommand.java @@ -27,7 +27,8 @@ public void setup() { @Override public boolean canExecute(User user, String label, List args) { // Command instances are shared across all players, so the resolved island must not be - // cached on an instance field here - it is recomputed per-player in execute() instead. + // cached on an instance field here (e.g. player A's island could otherwise leak into + // player B's execute() call) - it is recomputed per-player in execute() instead. return this.getIslands().getIslandAt(user.getLocation()).isPresent(); } diff --git a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java index cae965d..02dce89 100644 --- a/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java +++ b/src/test/java/world/bentobox/chat/listeners/ChatListenerTest.java @@ -312,7 +312,6 @@ public void testOnChatAsyncIslandChatUsesCurrentMessageNotStale() { spyListener.onChat(event2); assertTrue(event2.isCancelled()); verify(spyListener).islandChat(island, player, "test"); - verify(spyListener, org.mockito.Mockito.never()).islandChat(island, player, "aaaa test"); } @Test