From 6c4e85d1f72e2b1976dcb15db6d4a2472bafb271 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 16:14:38 +0000 Subject: [PATCH 1/6] Add double-scheduling guards and volatile fields to CoreTask hierarchy (#63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add re-schedule guard to RepeatableCoreTask.runTask() that logs a warning and returns when the task is already scheduled - Add volatile to cross-thread state fields in CoreTask (bukkitTaskId, taskStartTime, taskRunningAsync, taskExecuted), RepeatableCoreTask (currentInterval, delayExpired, intervalStartTime, paused), and CancelableCoreTask (cancelled) - Remove self-scheduling from PlayerUnloadTask constructor — callers must now call runTask(true) explicitly after construction - Add RepeatableCoreTaskTest cases for double-scheduling prevention - Update PlayerUnloadTaskTest to verify constructor does not schedule - Update CLAUDE.md with anti-pattern and task scheduling notes Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- CLAUDE.md | 3 +++ .../mccore/task/core/CancelableCoreTask.java | 2 +- .../mccore/task/core/CoreTask.java | 8 +++--- .../mccore/task/core/RepeatableCoreTask.java | 13 +++++++--- .../mccore/task/player/PlayerUnloadTask.java | 7 +++++- .../task/core/RepeatableCoreTaskTest.java | 25 +++++++++++++++++++ .../task/player/PlayerUnloadTaskTest.java | 12 +++++++-- 7 files changed, 58 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6344b5a..38ab0a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -341,6 +341,8 @@ managerRegistry.manager(CoreManagerKey.CORE_DATABASE_MANAGER).getDatabase().init ### Task Scheduling +`RepeatableCoreTask.runTask(boolean)` guards against double-scheduling — calling it on an already-running task logs a warning and returns without scheduling a second timer. All cross-thread state fields in the `CoreTask` hierarchy (`taskExecuted`, `bukkitTaskId`, `cancelled`, `paused`, etc.) are `volatile` to ensure visibility across the main and async scheduler threads. + ```java // One-off sync task new CoreTask(plugin) { @@ -422,6 +424,7 @@ Register with `ReloadableContentManager` so it refreshes automatically on `/relo - **No hard-coded strings for namespaced keys or config routes** — define constants on the owning class or a dedicated constants file - **No direct entity casting without a null/type guard** — use `instanceof` pattern matching: `if (entity instanceof Player player) { ... }` - **No decorative section-divider comments** — do not use `// ── Section ──`, `// --- Section ---`, or similar ASCII-art dividers to group methods or tests; rely on class structure, method naming, and `@DisplayName` annotations to communicate organization +- **No constructor-initiated task scheduling** — callers schedule tasks explicitly after construction via `runTask()`; constructors must not call `runTask()` because it couples object creation to the scheduler and complicates testing --- diff --git a/src/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.java b/src/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.java index d119958..f93f04b 100644 --- a/src/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.java +++ b/src/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.java @@ -10,7 +10,7 @@ */ public abstract class CancelableCoreTask extends RepeatableCoreTask { - protected boolean cancelled; + protected volatile boolean cancelled; public CancelableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double taskFrequency) { super(plugin, taskDelay, taskFrequency); diff --git a/src/main/java/com/diamonddagger590/mccore/task/core/CoreTask.java b/src/main/java/com/diamonddagger590/mccore/task/core/CoreTask.java index 3aeafe8..b13dbce 100644 --- a/src/main/java/com/diamonddagger590/mccore/task/core/CoreTask.java +++ b/src/main/java/com/diamonddagger590/mccore/task/core/CoreTask.java @@ -11,10 +11,10 @@ public abstract class CoreTask implements Runnable { private final CorePlugin plugin; - protected int bukkitTaskId = -1; - protected long taskStartTime = -1; - protected boolean taskRunningAsync; - protected boolean taskExecuted; + protected volatile int bukkitTaskId = -1; + protected volatile long taskStartTime = -1; + protected volatile boolean taskRunningAsync; + protected volatile boolean taskExecuted; public CoreTask(@NotNull CorePlugin plugin) { this.plugin = plugin; diff --git a/src/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.java b/src/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.java index 7a6199f..9190942 100644 --- a/src/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.java +++ b/src/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.java @@ -18,10 +18,10 @@ public abstract class RepeatableCoreTask extends MultiExecutionCoreTask { protected final double taskDelay; protected final double taskFrequency; - protected int currentInterval = 0; - protected boolean delayExpired; - protected long intervalStartTime; - protected boolean paused; + protected volatile int currentInterval = 0; + protected volatile boolean delayExpired; + protected volatile long intervalStartTime; + protected volatile boolean paused; public RepeatableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double taskFrequency) { super(plugin); @@ -31,6 +31,11 @@ public RepeatableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double t @Override public void runTask(boolean runAsync) { + if (taskExecuted) { + getPlugin().getLogger().warning("RepeatableCoreTask (id=" + bukkitTaskId + ") is already scheduled; ignoring duplicate runTask() call"); + return; + } + if (runAsync) { bukkitTaskId = Bukkit.getScheduler().runTaskTimerAsynchronously(getPlugin(), this, 0, 1).getTaskId(); taskRunningAsync = true; diff --git a/src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java b/src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java index 5e536a8..b709cdb 100644 --- a/src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java +++ b/src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java @@ -23,12 +23,17 @@ public abstract class PlayerUnloadTask extends ExpireableCoreTask { private final CompletableFuture result; private boolean completed; + /** + * Callers must invoke {@code runTask(true)} after construction to begin the async unload task. + * + * @param plugin The {@link CorePlugin} that owns this task. + * @param corePlayer The {@link CorePlayer} to unload. + */ public PlayerUnloadTask(@NotNull CorePlugin plugin, @NotNull CorePlayer corePlayer) { super(plugin, 0L, 2, 10L); this.corePlayer = corePlayer; this.result = new CompletableFuture<>(); completed = false; - runTask(true); } private void runUnloadPlayerTask() { diff --git a/src/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.java b/src/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.java index 804debd..ef902c4 100644 --- a/src/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.java +++ b/src/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.java @@ -18,6 +18,7 @@ import java.time.Instant; import java.time.ZoneOffset; import java.util.ArrayList; +import java.util.logging.Logger; import java.util.List; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -26,6 +27,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -50,6 +52,7 @@ void setUp() { mocks = MockitoAnnotations.openMocks(this); testClock = Clock.fixed(FIXED_INSTANT, ZoneOffset.UTC); when(mockPlugin.getTimeProvider()).thenReturn(new TimeProvider(testClock)); + when(mockPlugin.getLogger()).thenReturn(Logger.getLogger("RepeatableCoreTaskTest")); when(mockBukkitTask.getTaskId()).thenReturn(TASK_ID); mockedBukkit = mockStatic(Bukkit.class); @@ -264,6 +267,28 @@ void togglePause_fromPaused_resumesAndReturnsFalse() { assertFalse(task.isTaskPaused()); } + @Test + @DisplayName("Double runTask() call schedules only one Bukkit timer") + void runTask_ignoredDuplicate_whenAlreadyScheduled() { + TestRepeatableTask task = createTask(1.0, 1.0); + + task.runTask(false); + task.runTask(false); + + verify(mockScheduler, times(1)).runTaskTimer(eq(mockPlugin), eq(task), eq(0L), eq(1L)); + } + + @Test + @DisplayName("Double runTask(true) call schedules only one Bukkit timer") + void runTask_ignoredDuplicateAsync_whenAlreadyScheduled() { + TestRepeatableTask task = createTask(1.0, 1.0); + + task.runTask(true); + task.runTask(true); + + verify(mockScheduler, times(1)).runTaskTimerAsynchronously(eq(mockPlugin), eq(task), eq(0L), eq(1L)); + } + static class TestRepeatableTask extends RepeatableCoreTask { final List delayCompleteCalls = new ArrayList<>(); diff --git a/src/test/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTaskTest.java b/src/test/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTaskTest.java index d8ccc36..bfe59ad 100644 --- a/src/test/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTaskTest.java +++ b/src/test/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTaskTest.java @@ -166,9 +166,17 @@ void constructor_pluginIsCorrect() { } @Test - @DisplayName("Given a new task, when constructed, then task is started asynchronously") - void constructor_startsAsyncTask() { + @DisplayName("Constructor does not self-schedule") + void constructor_doesNotSchedule() { createTask(); + verify(mockScheduler, never()).runTaskTimerAsynchronously(any(), any(Runnable.class), anyLong(), anyLong()); + } + + @Test + @DisplayName("runTask(true) after construction schedules async timer") + void runTask_schedulesAsyncTimer_whenCalledAfterConstruction() { + PlayerUnloadTask task = createTask(); + task.runTask(true); verify(mockScheduler).runTaskTimerAsynchronously(eq(mockPlugin), any(Runnable.class), eq(0L), eq(1L)); } From 052f322ea265fb7d41de92b699d9c9e4a8d2b592 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 16:17:19 +0000 Subject: [PATCH 2/6] Add TransactionState tracking to FailSafeTransaction for failure observability (#62) - Add TransactionState enum (PENDING, COMMITTED, ROLLED_BACK) with volatile fields for cross-thread visibility - Add getTransactionState() and getFailureCause() to FailSafeTransaction so callers can inspect outcome without breaking fail-safe semantics - Replace e.printStackTrace() calls with Logger.log(Level.SEVERE, ..., e) to route stack traces through the plugin logger - Remove unused SLF4J import and field from FailSafeTransaction and BatchTransaction - Add TransactionStateTest for enum round-trip verification - Add state assertions to existing FailSafeTransactionTest cases - Update CLAUDE.md domain terminology with TransactionState entry Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- CLAUDE.md | 1 + .../transaction/BatchTransaction.java | 3 -- .../transaction/FailSafeTransaction.java | 39 ++++++++++++++++--- .../transaction/TransactionState.java | 26 +++++++++++++ .../transaction/FailSafeTransactionTest.java | 12 ++++++ .../transaction/TransactionStateTest.java | 26 +++++++++++++ 6 files changed, 98 insertions(+), 9 deletions(-) create mode 100644 src/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.java create mode 100644 src/test/java/com/diamonddagger590/mccore/database/transaction/TransactionStateTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 38ab0a9..2fa342e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -215,6 +215,7 @@ src/main/java/com/diamonddagger590/mccore/ | **CreateTableFunction** | Functional interface called once at DB init to create a table if it doesn't exist. | | **UpdateTableFunction** | Functional interface called after table creation to apply schema migrations. | | **Transaction** | Abstract base for executing an ordered list of `PreparedStatement`s against a single `Connection`. Subclasses define failure semantics: `BatchTransaction` commits whatever succeeds and logs individual failures; `FailSafeTransaction` rolls back everything if any single statement fails. | +| **TransactionState** | Enum (`PENDING`, `COMMITTED`, `ROLLED_BACK`) tracking the outcome of a `FailSafeTransaction`. Fields are `volatile` for cross-thread visibility. Callers inspect state after `executeTransaction()` returns — the method still returns `void` and never throws. | | **DAO** | Static JDBC methods for reading/writing a specific entity. Always takes `Connection` as the first argument. | | **ReloadableContent** | A config-backed value that can be refreshed at runtime without a server restart. | | **PlayerSetting** | A namespaced, persistent player preference. Stored in the database and loaded with the player. | diff --git a/src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java b/src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java index bf51cdd..68af549 100644 --- a/src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java +++ b/src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java @@ -2,7 +2,6 @@ import com.diamonddagger590.mccore.CorePlugin; import org.jetbrains.annotations.NotNull; -import org.slf4j.LoggerFactory; import java.sql.Connection; import java.sql.PreparedStatement; @@ -19,8 +18,6 @@ */ public class BatchTransaction extends Transaction { - private static final org.slf4j.Logger log = LoggerFactory.getLogger(BatchTransaction.class); - public BatchTransaction(@NotNull Connection connection) { super(connection); } diff --git a/src/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.java b/src/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.java index b2634b2..76531ad 100644 --- a/src/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.java +++ b/src/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.java @@ -2,12 +2,13 @@ import com.diamonddagger590.mccore.CorePlugin; import org.jetbrains.annotations.NotNull; -import org.slf4j.LoggerFactory; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.SQLException; import java.util.List; +import java.util.Optional; +import java.util.logging.Level; import java.util.logging.Logger; /** @@ -15,10 +16,14 @@ * to succeed in order for any of them to be committed. *

* If a single statement fails, then the transaction will instead roll back. + *

+ * After execution, callers can inspect the outcome via {@link #getTransactionState()} + * and retrieve any failure cause via {@link #getFailureCause()}. */ public class FailSafeTransaction extends Transaction { - private static final org.slf4j.Logger log = LoggerFactory.getLogger(FailSafeTransaction.class); + private volatile TransactionState transactionState = TransactionState.PENDING; + private volatile SQLException failureCause; public FailSafeTransaction(@NotNull Connection connection) { super(connection); @@ -45,16 +50,17 @@ public void executeTransaction() { } } connection.commit(); + transactionState = TransactionState.COMMITTED; } catch (SQLException e) { + transactionState = TransactionState.ROLLED_BACK; + failureCause = e; logger.severe("Encountered an exception while executing a fail safe transaction... rolling back."); - logger.severe(e.getMessage()); - e.printStackTrace(); + logger.log(Level.SEVERE, e.getMessage(), e); try { connection.rollback(); } catch (SQLException rollbackException) { logger.severe("Encountered an exception while rolling back transaction..."); - logger.severe(rollbackException.getMessage()); - rollbackException.printStackTrace(); + logger.log(Level.SEVERE, rollbackException.getMessage(), rollbackException); } } finally { @@ -73,4 +79,25 @@ public void executeTransaction() { } } } + + /** + * Gets the current state of this transaction. + * + * @return The {@link TransactionState} representing the outcome of this transaction. + */ + @NotNull + public TransactionState getTransactionState() { + return transactionState; + } + + /** + * Gets the {@link SQLException} that caused this transaction to roll back, if any. + * + * @return An {@link Optional} containing the failure cause, or empty if the transaction + * has not failed. + */ + @NotNull + public Optional getFailureCause() { + return Optional.ofNullable(failureCause); + } } diff --git a/src/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.java b/src/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.java new file mode 100644 index 0000000..fc06267 --- /dev/null +++ b/src/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.java @@ -0,0 +1,26 @@ +package com.diamonddagger590.mccore.database.transaction; + +/** + * Tracks the outcome of a {@link FailSafeTransaction} execution. + *

+ * A transaction starts in {@link #PENDING} and transitions to either + * {@link #COMMITTED} (all statements succeeded) or {@link #ROLLED_BACK} + * (a statement failed and the transaction was rolled back). + */ +public enum TransactionState { + + /** + * The transaction has not yet been executed. + */ + PENDING, + + /** + * All statements succeeded and the transaction was committed. + */ + COMMITTED, + + /** + * A statement failed and the transaction was rolled back. + */ + ROLLED_BACK +} diff --git a/src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java b/src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java index fb4e9a4..e89d53a 100644 --- a/src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java +++ b/src/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.java @@ -13,7 +13,9 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -42,6 +44,8 @@ void constructor_createsEmptyTransaction_whenSingleArgUsed() { assertNotNull(transaction); assertEquals(connection, transaction.getConnection()); assertEquals(0, transaction.getPreparedStatements().size()); + assertEquals(TransactionState.PENDING, transaction.getTransactionState()); + assertFalse(transaction.getFailureCause().isPresent()); } @Test @@ -67,6 +71,8 @@ void executeTransaction_commitsAllStatements_whenAllSucceed() throws SQLExceptio verify(ps2).executeUpdate(); verify(connection).commit(); verify(connection).setAutoCommit(true); + assertEquals(TransactionState.COMMITTED, transaction.getTransactionState()); + assertFalse(transaction.getFailureCause().isPresent()); } @Test @@ -82,6 +88,8 @@ void executeTransaction_rollsBack_whenStatementFails() throws SQLException { verify(ps2, never()).executeUpdate(); verify(connection, never()).commit(); verify(connection).rollback(); + assertEquals(TransactionState.ROLLED_BACK, transaction.getTransactionState()); + assertTrue(transaction.getFailureCause().isPresent()); } @Test @@ -130,6 +138,7 @@ void executeTransaction_commitsSuccessfully_whenNoStatementsAdded() throws SQLEx verify(connection).commit(); verify(connection).setAutoCommit(true); verify(connection, never()).rollback(); + assertEquals(TransactionState.COMMITTED, transaction.getTransactionState()); } @Test @@ -142,6 +151,8 @@ void executeTransaction_rollsBackWithoutCommit_whenSetAutoCommitFalseFails() thr verify(connection, never()).commit(); verify(connection).rollback(); + assertEquals(TransactionState.ROLLED_BACK, transaction.getTransactionState()); + assertTrue(transaction.getFailureCause().isPresent()); } @Test @@ -160,5 +171,6 @@ void executeTransaction_rollsBackAfterPartialExecution_whenMiddleStatementFails( verify(ps3, never()).executeUpdate(); verify(connection).rollback(); verify(connection, never()).commit(); + assertEquals(TransactionState.ROLLED_BACK, transaction.getTransactionState()); } } diff --git a/src/test/java/com/diamonddagger590/mccore/database/transaction/TransactionStateTest.java b/src/test/java/com/diamonddagger590/mccore/database/transaction/TransactionStateTest.java new file mode 100644 index 0000000..d86eb5a --- /dev/null +++ b/src/test/java/com/diamonddagger590/mccore/database/transaction/TransactionStateTest.java @@ -0,0 +1,26 @@ +package com.diamonddagger590.mccore.database.transaction; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +class TransactionStateTest { + + @Test + @DisplayName("Enum contains exactly three values") + void values_containsThreeEntries() { + assertEquals(3, TransactionState.values().length); + } + + @ParameterizedTest + @EnumSource(TransactionState.class) + @DisplayName("valueOf round-trips for all values") + void valueOf_roundTrips(TransactionState state) { + assertNotNull(TransactionState.valueOf(state.name())); + assertEquals(state, TransactionState.valueOf(state.name())); + } +} From 8c7187453ffec91f2e4987fd926fff82b8c768b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 16:20:17 +0000 Subject: [PATCH 3/6] Add recognition API to CustomBlockWrapper/CustomEntityWrapper and STONE fallback warning (#65) - Add isVanilla() and isCustom() instance methods to CustomBlockWrapper and CustomEntityWrapper for checking whether a wrapper represents a vanilla or custom block/entity - Add isVanillaBlock(String) and isVanillaEntity(String) static methods for checking identifiers against the Paper registry without constructing a wrapper - Add dedup-guarded warning to ItemPluginType when material resolution fails and falls back to STONE (logs once per unique string) - Add IsVanilla and IsCustom nested test classes to both wrapper tests Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- .../mccore/builder/item/ItemPluginType.java | 18 +++++++++ .../mccore/util/item/CustomBlockWrapper.java | 32 ++++++++++++++++ .../mccore/util/item/CustomEntityWrapper.java | 32 ++++++++++++++++ .../util/item/CustomBlockWrapperTest.java | 38 +++++++++++++++++++ .../util/item/CustomEntityWrapperTest.java | 38 +++++++++++++++++++ 5 files changed, 158 insertions(+) diff --git a/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java b/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java index 84e7bb8..3b09753 100644 --- a/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java +++ b/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java @@ -8,6 +8,7 @@ import org.jetbrains.annotations.NotNull; import java.util.*; +import java.util.concurrent.ConcurrentHashMap; import static com.diamonddagger590.mccore.util.Methods.fromBase64; import static com.diamonddagger590.mccore.util.Methods.getItemType; @@ -34,6 +35,7 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { + warnUnresolvableMaterial(customItem); return ItemType.STONE.createItemStack(1); } } @@ -54,6 +56,7 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { + warnUnresolvableMaterial(customItem); return ItemType.STONE.createItemStack(1); } } @@ -82,12 +85,15 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { + warnUnresolvableMaterial(customItem); return ItemType.STONE.createItemStack(1); } } }, "none"), ; + private static final Set WARNED_MATERIALS = ConcurrentHashMap.newKeySet(); + @NotNull private final CustomItemFunction customItemFunction; @NotNull @@ -119,6 +125,18 @@ public static ItemPluginType fromName(@NotNull final String name) { .orElse(ItemPluginType.NONE); } + /** + * Logs a warning the first time a given material string cannot be resolved, + * suppressing duplicates for the same string. + * + * @param customItem The unresolvable material string. + */ + private static void warnUnresolvableMaterial(@NotNull String customItem) { + if (WARNED_MATERIALS.add(customItem)) { + CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE"); + } + } + /** * A function to turn a string into an {@link ItemStack} for custom item plugins. */ diff --git a/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java b/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java index a3f0cf2..a5729a4 100644 --- a/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java +++ b/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java @@ -73,6 +73,38 @@ public CustomBlockWrapper(@NotNull Block block) { this.customBlock = customBlockResult; } + /** + * Checks whether this wrapper represents a vanilla block (resolved via the Paper registry). + * + * @return {@code true} if this wrapper holds a {@link Material}, meaning the block + * was recognized as a vanilla Minecraft block. + */ + public boolean isVanilla() { + return material != null; + } + + /** + * Checks whether this wrapper represents a custom block (from a model plugin). + * + * @return {@code true} if this wrapper holds a custom block identifier, meaning the block + * was not recognized as a vanilla Minecraft block. + */ + public boolean isCustom() { + return customBlock != null; + } + + /** + * Checks whether the given identifier resolves to a vanilla block in the Paper block registry. + * + * @param id The block identifier to check (e.g. {@code "stone"}, {@code "iron_ore"}). + * @return {@code true} if the identifier is recognized as a vanilla block. + */ + public static boolean isVanillaBlock(@NotNull String id) { + return io.papermc.paper.registry.RegistryAccess.registryAccess() + .getRegistry(io.papermc.paper.registry.RegistryKey.BLOCK) + .get(Methods.getMinecraftKey(id)) != null; + } + /** * Gets an {@link Optional} containing the {@link Material} represented * by this wrapper. diff --git a/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java b/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java index 26c5aac..bffd35b 100644 --- a/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java +++ b/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java @@ -65,6 +65,38 @@ public CustomEntityWrapper(@NotNull Entity entity) { this.customEntity = customEntityResult; } + /** + * Checks whether this wrapper represents a vanilla entity (resolved via the Paper registry). + * + * @return {@code true} if this wrapper holds an {@link EntityType}, meaning the entity + * was recognized as a vanilla Minecraft entity. + */ + public boolean isVanilla() { + return entityType != null; + } + + /** + * Checks whether this wrapper represents a custom entity (from a model plugin). + * + * @return {@code true} if this wrapper holds a custom entity identifier, meaning the entity + * was not recognized as a vanilla Minecraft entity. + */ + public boolean isCustom() { + return customEntity != null; + } + + /** + * Checks whether the given identifier resolves to a vanilla entity in the Paper entity type registry. + * + * @param id The entity identifier to check (e.g. {@code "zombie"}, {@code "creeper"}). + * @return {@code true} if the identifier is recognized as a vanilla entity type. + */ + public static boolean isVanillaEntity(@NotNull String id) { + return io.papermc.paper.registry.RegistryAccess.registryAccess() + .getRegistry(io.papermc.paper.registry.RegistryKey.ENTITY_TYPE) + .get(Methods.getMinecraftKey(id)) != null; + } + /** * Gets an {@link Optional} containing the {@link EntityType} represented * by this wrapper. diff --git a/src/test/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapperTest.java b/src/test/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapperTest.java index 37dc020..dc8624c 100644 --- a/src/test/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapperTest.java +++ b/src/test/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapperTest.java @@ -148,6 +148,44 @@ void material_returnsEmpty_whenConstructedWithCustomBlock() throws Exception { } } + @Nested + @DisplayName("isVanilla") + class IsVanilla { + + @Test + @DisplayName("Material wrapper returns true") + void isVanilla_returnsTrue_whenMaterialWrapper() { + CustomBlockWrapper wrapper = materialWrapper(Material.STONE); + assertTrue(wrapper.isVanilla()); + } + + @Test + @DisplayName("Custom block wrapper returns false") + void isVanilla_returnsFalse_whenCustomBlockWrapper() throws Exception { + CustomBlockWrapper wrapper = customBlockWrapper("nexo:ruby_ore"); + assertFalse(wrapper.isVanilla()); + } + } + + @Nested + @DisplayName("isCustom") + class IsCustom { + + @Test + @DisplayName("Custom block wrapper returns true") + void isCustom_returnsTrue_whenCustomBlockWrapper() throws Exception { + CustomBlockWrapper wrapper = customBlockWrapper("nexo:ruby_ore"); + assertTrue(wrapper.isCustom()); + } + + @Test + @DisplayName("Material wrapper returns false") + void isCustom_returnsFalse_whenMaterialWrapper() { + CustomBlockWrapper wrapper = materialWrapper(Material.STONE); + assertFalse(wrapper.isCustom()); + } + } + @Nested @DisplayName("equals(Material)") class EqualsMaterial { diff --git a/src/test/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapperTest.java b/src/test/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapperTest.java index 0bad89d..f637b54 100644 --- a/src/test/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapperTest.java +++ b/src/test/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapperTest.java @@ -126,6 +126,44 @@ void entityType_returnsEmpty_whenConstructedWithCustomEntity() throws Exception } } + @Nested + @DisplayName("isVanilla") + class IsVanilla { + + @Test + @DisplayName("Entity type wrapper returns true") + void isVanilla_returnsTrue_whenEntityTypeWrapper() { + CustomEntityWrapper wrapper = entityTypeWrapper(EntityType.ZOMBIE); + assertTrue(wrapper.isVanilla()); + } + + @Test + @DisplayName("Custom entity wrapper returns false") + void isVanilla_returnsFalse_whenCustomEntityWrapper() throws Exception { + CustomEntityWrapper wrapper = customEntityWrapper("mythicmobs:fire_dragon"); + assertFalse(wrapper.isVanilla()); + } + } + + @Nested + @DisplayName("isCustom") + class IsCustom { + + @Test + @DisplayName("Custom entity wrapper returns true") + void isCustom_returnsTrue_whenCustomEntityWrapper() throws Exception { + CustomEntityWrapper wrapper = customEntityWrapper("mythicmobs:fire_dragon"); + assertTrue(wrapper.isCustom()); + } + + @Test + @DisplayName("Entity type wrapper returns false") + void isCustom_returnsFalse_whenEntityTypeWrapper() { + CustomEntityWrapper wrapper = entityTypeWrapper(EntityType.ZOMBIE); + assertFalse(wrapper.isCustom()); + } + } + @Nested @DisplayName("equals(EntityType)") class EqualsEntityType { From d9863adf9bf5c99c58afc093e545b5e9ac1767de Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 19:37:52 +0000 Subject: [PATCH 4/6] Sync core.mdc with CLAUDE.md updates from tickets #63, #62, #65 Add missing entries to .cursor/rules/core.mdc that were added to CLAUDE.md across the three wave-1 tickets: e.printStackTrace() anti-pattern, task scheduling notes (volatile fields, re-schedule guard), and TransactionState observability section. Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- .cursor/rules/core.mdc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.cursor/rules/core.mdc b/.cursor/rules/core.mdc index 7694f32..91ce794 100644 --- a/.cursor/rules/core.mdc +++ b/.cursor/rules/core.mdc @@ -98,7 +98,17 @@ void getStatistic_returnsStatistic_whenKeyIsRegistered() { ... } - **No state stored in Registry/Manager objects beyond their scope** — domain state belongs on domain objects - **No hard-coded strings for namespaced keys or config routes** — define as constants - **No direct entity casting without guard** — use `instanceof` pattern matching: `if (entity instanceof Player player) { ... }` -- **No decorative section-divider comments** — do not use `// ── Section ──`, `// --- Section ---`, or similar ASCII-art dividers to group methods or tests; rely on class structure, method naming, and `@DisplayName` annotations instead +- **No decorative section-divider comments** — rely on class structure, method naming, and `@DisplayName` annotations instead of `// --- Section ---` dividers +- **No constructor-initiated task scheduling** — callers schedule tasks explicitly after construction via `runTask()`; constructors must not call `runTask()` because it couples object creation to the scheduler and complicates testing +- **No `e.printStackTrace()`** — always use `Logger.log(Level.SEVERE, "context message", e)` so stack traces route through the server logger + +## Task Scheduling + +`RepeatableCoreTask.runTask(boolean)` guards against double-scheduling — calling it on an already-running task logs a warning and returns without scheduling a second timer. All cross-thread state fields in the `CoreTask` hierarchy (`taskExecuted`, `bukkitTaskId`, `cancelled`, `paused`, etc.) are `volatile` to ensure visibility across the main and async scheduler threads. + +## Transaction Observability + +`FailSafeTransaction` tracks its outcome via `TransactionState` (`PENDING`, `COMMITTED`, `ROLLED_BACK`). The `executeTransaction()` method still returns `void` and never throws — callers inspect `getTransactionState()` and `getFailureCause()` after execution to detect failures. --- From 4bd49b9664645e9333a2ebdb6ab15f20f2fb123d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 19:47:06 +0000 Subject: [PATCH 5/6] Simplify ItemPluginType STONE fallback warning to log every time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the static WARNED_MATERIALS dedup set and private helper method. Log the warning directly on every unresolvable material — the spam makes misconfigured materials obvious to server owners. Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- .../mccore/builder/item/ItemPluginType.java | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java b/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java index 3b09753..b7ee54b 100644 --- a/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java +++ b/src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java @@ -8,7 +8,6 @@ import org.jetbrains.annotations.NotNull; import java.util.*; -import java.util.concurrent.ConcurrentHashMap; import static com.diamonddagger590.mccore.util.Methods.fromBase64; import static com.diamonddagger590.mccore.util.Methods.getItemType; @@ -35,7 +34,7 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { - warnUnresolvableMaterial(customItem); + CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE"); return ItemType.STONE.createItemStack(1); } } @@ -56,7 +55,7 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { - warnUnresolvableMaterial(customItem); + CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE"); return ItemType.STONE.createItemStack(1); } } @@ -85,15 +84,13 @@ public enum ItemPluginType { try { return fromBase64(customItem); } catch (Exception exception) { - warnUnresolvableMaterial(customItem); + CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE"); return ItemType.STONE.createItemStack(1); } } }, "none"), ; - private static final Set WARNED_MATERIALS = ConcurrentHashMap.newKeySet(); - @NotNull private final CustomItemFunction customItemFunction; @NotNull @@ -125,18 +122,6 @@ public static ItemPluginType fromName(@NotNull final String name) { .orElse(ItemPluginType.NONE); } - /** - * Logs a warning the first time a given material string cannot be resolved, - * suppressing duplicates for the same string. - * - * @param customItem The unresolvable material string. - */ - private static void warnUnresolvableMaterial(@NotNull String customItem) { - if (WARNED_MATERIALS.add(customItem)) { - CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE"); - } - } - /** * A function to turn a string into an {@link ItemStack} for custom item plugins. */ From 1540e41f7e941277e75266af8803b04afcaa6a64 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 20:15:17 +0000 Subject: [PATCH 6/6] Fix FQN in wrappers, steering doc logger reference, and review workflow permissions - Extract private lookupVanillaBlock/lookupVanillaEntityType helpers in CustomBlockWrapper and CustomEntityWrapper to consolidate FQN usage - Update core.mdc e.printStackTrace anti-pattern to reference CorePlugin.getInstance().getLogger() instead of Logger.log - Add Bash(git diff/log/show:*) to allowedTools in claude-review.yml so review sub-agents can run git commands Co-Authored-By: Claude Opus 4.6 Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX --- .cursor/rules/core.mdc | 2 +- .github/workflows/claude-review.yml | 4 ++-- .../mccore/util/item/CustomBlockWrapper.java | 15 +++++++++++++-- .../mccore/util/item/CustomEntityWrapper.java | 15 +++++++++++++-- 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/.cursor/rules/core.mdc b/.cursor/rules/core.mdc index 91ce794..bfd2e18 100644 --- a/.cursor/rules/core.mdc +++ b/.cursor/rules/core.mdc @@ -100,7 +100,7 @@ void getStatistic_returnsStatistic_whenKeyIsRegistered() { ... } - **No direct entity casting without guard** — use `instanceof` pattern matching: `if (entity instanceof Player player) { ... }` - **No decorative section-divider comments** — rely on class structure, method naming, and `@DisplayName` annotations instead of `// --- Section ---` dividers - **No constructor-initiated task scheduling** — callers schedule tasks explicitly after construction via `runTask()`; constructors must not call `runTask()` because it couples object creation to the scheduler and complicates testing -- **No `e.printStackTrace()`** — always use `Logger.log(Level.SEVERE, "context message", e)` so stack traces route through the server logger +- **No `e.printStackTrace()`** — always use `CorePlugin.getInstance().getLogger().log(Level.SEVERE, "context message", e)` so stack traces route through the server logger ## Task Scheduling diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index f3bea4c..510547b 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -65,7 +65,7 @@ jobs: claude_args: | --model ${{ env.CLAUDE_MODEL }} --max-turns 40 - --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment" + --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),mcp__github_inline_comment__create_inline_comment" # On-demand: an @claude mention in a PR comment or review comment. Tag mode # (no prompt) — claude-code-action handles the sticky comment natively here. @@ -101,5 +101,5 @@ jobs: claude_args: | --model ${{ env.CLAUDE_MODEL }} --max-turns 40 - --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment" + --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),mcp__github_inline_comment__create_inline_comment" --append-system-prompt "If asked to review or re-review this PR, follow .github/claude-review-prompt.md including its re-review convergence protocol. Otherwise answer the request directly." diff --git a/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java b/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java index a5729a4..6a6502c 100644 --- a/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java +++ b/src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java @@ -47,7 +47,7 @@ public CustomBlockWrapper(@NotNull Material material) { } public CustomBlockWrapper(@NotNull String customBlock) { - BlockType blockType = io.papermc.paper.registry.RegistryAccess.registryAccess().getRegistry(io.papermc.paper.registry.RegistryKey.BLOCK).get(Methods.getMinecraftKey(customBlock)); + BlockType blockType = lookupVanillaBlock(customBlock); if (blockType != null) { this.material = blockType.asMaterial(); this.customBlock = null; @@ -100,9 +100,20 @@ public boolean isCustom() { * @return {@code true} if the identifier is recognized as a vanilla block. */ public static boolean isVanillaBlock(@NotNull String id) { + return lookupVanillaBlock(id) != null; + } + + /** + * Looks up a block identifier in the Paper block registry. + * + * @param id The block identifier to look up (e.g. {@code "stone"}, {@code "iron_ore"}). + * @return The {@link BlockType} if the identifier is a vanilla block, or {@code null} if not found. + */ + @Nullable + private static BlockType lookupVanillaBlock(@NotNull String id) { return io.papermc.paper.registry.RegistryAccess.registryAccess() .getRegistry(io.papermc.paper.registry.RegistryKey.BLOCK) - .get(Methods.getMinecraftKey(id)) != null; + .get(Methods.getMinecraftKey(id)); } /** diff --git a/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java b/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java index bffd35b..a576618 100644 --- a/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java +++ b/src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java @@ -38,7 +38,7 @@ public CustomEntityWrapper(@NotNull EntityType entityType) { } public CustomEntityWrapper(@NotNull String customEntity) { - EntityType entityType = io.papermc.paper.registry.RegistryAccess.registryAccess().getRegistry(io.papermc.paper.registry.RegistryKey.ENTITY_TYPE).get(Methods.getMinecraftKey(customEntity)); + EntityType entityType = lookupVanillaEntityType(customEntity); if (entityType != null) { this.entityType = entityType; this.customEntity = null; @@ -92,9 +92,20 @@ public boolean isCustom() { * @return {@code true} if the identifier is recognized as a vanilla entity type. */ public static boolean isVanillaEntity(@NotNull String id) { + return lookupVanillaEntityType(id) != null; + } + + /** + * Looks up an entity identifier in the Paper entity type registry. + * + * @param id The entity identifier to look up (e.g. {@code "zombie"}, {@code "creeper"}). + * @return The {@link EntityType} if the identifier is a vanilla entity, or {@code null} if not found. + */ + @Nullable + private static EntityType lookupVanillaEntityType(@NotNull String id) { return io.papermc.paper.registry.RegistryAccess.registryAccess() .getRegistry(io.papermc.paper.registry.RegistryKey.ENTITY_TYPE) - .get(Methods.getMinecraftKey(id)) != null; + .get(Methods.getMinecraftKey(id)); } /**