feat(void): Proteção de itens contra morte no vácuo (Baú seguro ou inventário) #5
Reference in New Issue
Block a user
Delete Branch "feat/void-protection"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
🛡️ Módulo de Proteção contra Morte no Vácuo (
salvavoid)O que faz:
DamageCause.VOIDou Y < altura mínima do mundo)./canalhandia modulo salvavoid <on|off>.🤖 AI Review · pragent pilot · glm-5.2:cloud ·
24ab63b9Tier: full (~280 changed lines, 6 files; touches item/inventory handling on the PlayerDeathEvent risk path). PR adds the
salvavoidmodule: on void death it scans for nearby safe ground, places a rescue chest with the player's drops, or falls back to keepInventory. No Java toolchain was available in the review environment, so mvn/compile could not be run. The core rescue contract has an item-loss/duplication bug; the scan is an O(n^2) main-thread hotspot; and the new logic is largely untested. 1 high, 2 medium, 1 low.🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 7 · duration: 206.2ssrc/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:162src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:67src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:34 inline comment(s) posted below.
@@ -0,0 +1,185 @@package dev.marcospaulo.canalhandia;import net.kyori.adventure.text.Component;[LOW] Unused imports (Component, NamedTextColor, DoubleChest, Player, Collections) — the messaging lives in Canalhandia.java and DoubleChest is never referenced, suggesting an abandoned approach.
Fix: Remove the unused imports.
🪙 ~3982 tok (31% · attributed output)
@@ -0,0 +64,4 @@int topY;try {topY = world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES);[MEDIUM] findSafeChestLocation calls getHighestBlockYAt for up to (2*radius+1)^2 (~4225 at radius 32) columns synchronously on the main thread inside PlayerDeathEvent, potentially loading/generating chunks; the worst case (no safe ground found) does the full scan and then its result is discarded for the keepInventory fallback.
Fix: Cap the number of sampled columns, spiral-early-exit on the first ring with a hit, or move the scan off the death event; at minimum short-circuit when the death is in a world/area unlikely to have terrain.
🪙 ~3466 tok (27% · attributed output)
@@ -0,0 +159,4 @@}}}return true;[HIGH] rescueToChest returns true even when items overflow the chest/double-chest (leftover silently discarded), and the broad catch can return false after partially filling a chest — the caller then setKeepInventory(true), duplicating items already cloned into the chest.
Fix: Only return true when every item is stored; on overflow roll back the placed chest(s) and return false so the caller's keepInventory fallback is safe, or return the unstored leftover so the caller clears only the stored drops.
📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/event/entity/PlayerDeathEvent.html#getDrops--
🪙 ~3248 tok (25% · attributed output)
@@ -0,0 +25,4 @@void handlesNullWorldOrLocationsGracefully() {assertNull(VoidProtection.findSafeChestLocation(null, null, 10));assertFalse(VoidProtection.rescueToChest(null, null));}[MEDIUM] Tests only cover isVoidDeath and null-argument guards; the actual rescue logic (rescueToChest success/overflow/partial-failure, findSafeChestLocation selection) has no coverage despite being the module's core behavior.
Fix: Add tests (with a mocked World/Block) for chest placement, overflow handling, and the no-safe-ground fallback path.
🪙 ~2203 tok (17% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
d93711e5Adds the salvavoid void-death item-rescue module (chest placement or keepInventory fallback) and reworks chunk-loader anchor break/interact handling. Risk is moderate on the PlayerDeathEvent item/inventory path. No Java toolchain was available, so compile/typecheck could not be run. New findings (not in prior review): orphaned chest left in world on rescue failure, missing module-enabled check on the new anchor interact handler, and a creative-mode refund gap. 0 critical, 0 high, 2 medium, 1 low.
🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 14 · duration: 217.4ssrc/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:131src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:146src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:1283 inline comment(s) posted below.
@@ -120,1 +125,4 @@event.setDropItems(false);event.setExpToDrop(0);if (player.getGameMode() != GameMode.CREATIVE) {[LOW] The anchor item refund is skipped for Creative-mode players, so an admin in Creative breaking a loader to clean up gets no item back while the success message still says 'recolhida para o seu inventário'.
Fix: Either always refund the anchor item (creative inventory can hold it), or adjust the message to reflect that creative players don't receive a refund.
🪙 ~2304 tok (18% · attributed output)
@@ -131,0 +143,4 @@}@EventHandler(priority = EventPriority.HIGH, ignoreCancelled = true)public void onPlayerInteract(PlayerInteractEvent event) {[MEDIUM] onPlayerInteract cancels right-click on chunk-loader anchors but never checks settings.moduleEnabled(Module.CHUNKLOADER), unlike onBlockPlace (line 44); if the module is disabled while anchors remain registered, players are still blocked from interacting with the block via vanilla mechanics.
Fix: Add the same moduleEnabled(Module.CHUNKLOADER) guard at the top of onPlayerInteract as onBlockPlace uses.
🪙 ~5189 tok (42% · attributed output)
@@ -0,0 +128,4 @@}try {Block chestBlock = chestLoc.getBlock();chestBlock.setType(Material.CHEST, false);[MEDIUM] chestBlock.setType(CHEST) runs before the block state is validated as a Chest (line 134); if that check fails or an exception is thrown, the method returns false but an empty chest block remains in the world at the rescue location while the caller falls back to keepInventory.
Fix: Validate the block state can be a Chest before mutating the block, or restore the original block type on any failure path before returning.
🪙 ~4999 tok (40% · attributed output)
✅ Resoluções Reviews #94 e #95 + Configuração do
pragent(commitc3c4906)VoidProtection.java):rescueToChestagora exige que todos os itens caibam no baú/baú duplo. Se houver qualquer overflow ou falha, o bloco é revertido ao estado original e retornafalse, acionando com segurança o fallback dekeepInventory.VoidProtection.java):world.isChunkLoaded) e possui teto de colunas amostradas (MAX_CHECKED_COLUMNS = 120), evitando geração/carregamento síncrono no evento de morte.ChunkLoaderListener.java):settings.moduleEnabled(Module.CHUNKLOADER)noonPlayerInteract.VoidProtectionTest.java(350/350 testes passando)..pr-review.json:.pr-review.jsonno repositório configurando foco em segurança de threads, ciclo de vida Paper, prevenção de perda de itens e performance.🤖 AI Review · pragent pilot · glm-5.2:cloud ·
c3c4906e🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 7 · duration: 132.5s⚠️ The reviewer did not emit a parseable findings block, so there are no inline comments. Its raw notes are below — treat them as unverified: line numbers were not validated against the diff.
…epInventory()
would be true by the timeonDeathComicruns. ButonDeathComic` ignores that and reads the gamerule. So the message is wrong. MEDIUM.This is a genuinely new finding not in prior reviews.
Finding 6 (LOW) —
VoidProtectionTest.java:28: The testhandlesNullWorldOrLocationsGracefullypassesnullfor both world and location tofindSafeChestLocation, which doesn't exercise the actual scan logic. Prior review #1 flagged "new logic is largely untested" and specificallyVoidProtectionTest.java:28. So this is already covered. Skip.Finding 7 (LOW) —
VoidProtection.java:75:world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES)can returnworld.getMaxHeight()(above the world) if the column is empty above the min height, meaningtopYcould be at or above the build limit. Thenworld.getBlockAt(x, topY+1, z)would be out of bounds. ActuallygetHighestBlockYAtreturns the y of the highest block per the height map, orworld.getMinHeight()if none. The checktopY <= minHeighthandles the "none" case. If topY is at max height,topY+1is out of range —getBlockAtclamps or throws? Paper'sgetBlockAtwith out-of-range y throwsIllegalArgumentExceptionin some versions. But this is wrapped in no try-catch at that point (onlygetHighestBlockYAtis in try-catch). SogetBlockAt(x, topY+1, z)at line 85 could throw if topY == max height. This would propagate up tofindSafeChestLocationand crash the death handler... butonVoidDeathdoesn't wrapfindSafeChestLocationin try-catch either. A thrown exception inonVoidDeathwould prevent the fallbacksetKeepInventory(true)from running → item loss.Wait, let me check: does Paper's
getHighestBlockYAtwithMOTION_BLOCKING_NO_LEAVESever return max height? It returns the highest y where the height map indicates a block, orminHeightif empty. It should not return above max height. Actually per Paper docs,getHighestBlockYAtreturnsyof highest block, which is ≤maxHeight - 1. SotopY + 1 ≤ maxHeight.getBlockAtaccepts y in[minHeight, maxHeight]inclusive? The world height is[minHeight, maxHeight]wheremaxHeightis exclusive top. SogetBlockAt(x, maxHeight, z)is out of bounds. If topY == maxHeight - 1, then topY+1 == maxHeight → out of bounds → throws. This is an edge case (block at the very top of the world, e.g., a build at y=320 in 1.21). Rare but possible. Low severity.Actually, I'll skip this as too speculative without confirming Paper's exact bounds behavior.
Let me finalize with the two solid new findings:
MEDIUM —
Canalhandia.java:937(inonDeathComic) reads keepInventory gamerule instead ofevent.isKeepInventory(), so when salvvoid keeps items, the death-coords message wrongly tells the player to go fetch their items. But this line is not in the diff... The diff changedCanalhandia.javaby addingonVoidDeath. TheonDeathComicline 937 is context (pre-existing). Per methodology, I can anchor on context lines. The finding is about the interaction between the new code and the old code. Anchor at line 937 (context line, exists post-change). Good.LOW —
ChunkLoaderListener.java:147off-hand double-fire. Anchor at 147 (added line).Let me also reconsider the
onPlayerInteract— does it need anignoreCancelled = true? It has it. And it cancels the event for chunk-loader anchors. But it doesn't check the hand. Let me confirm the message duplication is real and report as low.Actually, let me reconsider severity of the death-message bug. It's a UX inconsistency that could confuse players but causes no item loss. MEDIUM is right per the rubric ("likely bug on an edge case"). Actually it's not an edge case — it fires on every void death when keepInventory gamerule is false (the common case). So it's a definite bug on the normal path. HIGH? No — it's a wrong chat message, not a correctness/security bug. MEDIUM.
Let me write up the findings.
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
2c7f5f98PR adds the
salvavoidvoid-death item-rescue module (chest placement or keepInventory fallback) and reworks chunk-loader anchor break/interact handling. Risk is moderate on the PlayerDeathEvent item/inventory path. No Java toolchain was available, so compile/typecheck could not be run. New findings (not in prior reviews): rescueToChest can silently merge with a pre-existing neighbor chest and, on rollback, wipe that chest's contents; findAdjacentSpace queries adjacent blocks without isChunkLoaded, violating the repo's chunk-loading invariant. 0 critical, 1 high, 1 medium.🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 13 · duration: 267.1ssrc/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:148src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:2292 inline comment(s) posted below.
@@ -0,0 +145,4 @@Material orig2 = null;try {chestBlock.setType(Material.CHEST, false);[HIGH] findSafeChestLocation only verifies the target block is replaceable, not that no neighbor is already a chest; when chestBlock.setType(CHEST) runs next to an existing chest, Bukkit forms a double chest and chest.getInventory() returns the 54-slot combined inventory, so a later rollback's inv.clear() (line 169/193) wipes the innocent neighbor chest's pre-existing items.
Fix: Before placing the chest, check the four horizontal neighbors; if any is a CHEST/TRAPPED_CHEST, skip this location in findSafeChestLocation (or pick a different adjacent side) so rescue never merges with a foreign chest.
🪙 ~6976 tok (40% · attributed output)
@@ -0,0 +226,4 @@int[][] offsets = {{1, 0}, {-1, 0}, {0, 1}, {0, -1}};for (int[] off : offsets) {Block b = w.getBlockAt(x + off[0], y, z + off[1]);[MEDIUM] findAdjacentSpace calls w.getBlockAt(...).getType() on adjacent columns without first checking world.isChunkLoaded(), so an unloaded neighbor chunk can be queried synchronously inside the PlayerDeathEvent handler — violating repo invariant #4 (never trigger sync chunk load in event handlers).
Fix: Guard each adjacent probe with world.isChunkLoaded((x+off[0])>>4, (z+off[1])>>4) before calling getBlockAt/getType, mirroring the check already done in findSafeChestLocation line 69.
🪙 ~10346 tok (60% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
4261e8b6Tier: full (~280 changed lines, 6 files; touches PlayerDeathEvent item-handling and chunk ticketing). PR adds the salvavoid void-death rescue module (chest placement or keepInventory fallback) and reworks chunk-loader anchor break/interact handling plus force-loaded chunk tickets. No Java toolchain was available, so mvn/compile could not be run. Prior-review items (O(n²) scan, rescue dup, missing module check, creative refund gap, orphaned chest, death-message keepInventory) are addressed in this revision. New findings: a force-loaded chunk flag that can leak past plugin crashes, untested spiral-scan/material-filter helpers, and an interact handler that blocks sneaking block-placement against active anchors. 0 critical, 0 high, 2 medium, 1 low.
🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 13 · duration: 243.3ssrc/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:230src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:1703 inline comment(s) posted below.
@@ -131,0 +167,4 @@);if (loader != null) {// Prevent respawn anchor exploding or depleting charges when used as chunk loaderevent.setCancelled(true);[LOW] onPlayerInteract cancels every right-click on an active anchor unconditionally, so a player who sneaks to place a block against the anchor (the standard Bukkit sneak-place interaction) has their placement silently blocked — a usability regression on a normal input path.
Fix: Skip cancellation when the player is sneaking and holding a placeable block, mirroring how vanilla handles sneak-use on interactable blocks.
📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/event/player/PlayerInteractEvent.html
🪙 ~6075 tok (38% · attributed output)
@@ -227,6 +227,7 @@ final class ChunkLoaders {try {World w = Bukkit.getWorld(loader.world());if (w != null) {w.setChunkForceLoaded(loader.chunkX(), loader.chunkZ(), true);[MEDIUM] setChunkForceLoaded(true) is added alongside addPluginChunkTicket, but the two have different lifecycles: a plugin chunk ticket is auto-revoked by the server when the plugin disables, whereas setChunkForceLoaded is not — if the plugin crashes or is force-unloaded without onDisable() running removeTicket(), the force-loaded flag persists and the chunk stays entity-processing forever until an operator clears it manually.
Fix: Rely on addPluginChunkTicket alone (it already keeps the chunk loaded and is server-managed), or record every force-loaded chunk so onDisable()/a crash-recovery sweep can clear strays.
📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/World.html#addPluginChunkTicket(int,int,org.bukkit.plugin.Plugin)
🪙 ~5039 tok (31% · attributed output)
@@ -0,0 +25,4 @@}@Testvoid handlesNullWorldOrLocationsGracefully() {[MEDIUM] The repo's own review config requires tests for 'coordinates math, parsers, and pure helpers', but findSafeChestLocation's spiral iteration, MAX_CHECKED_COLUMNS cap, ring-best distance selection, and the isSafeGround/isReplaceable material lists have no coverage — only null-input and isVoidDeath cases are tested, so regressions in the rescue scan (e.g. off-by-one in the ring boundary, a new dangerous block added to the world) would land silently.
Fix: Add unit tests for the pure helpers using a fake/mock World+Block: assert isSafeGround/isReplaceable over the listed materials, and assert the spiral selects the nearest safe column and respects MAX_CHECKED_COLUMNS.
🪙 ~4994 tok (31% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
4b9507b3🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 14 · duration: 200.0s⚠️ The reviewer did not emit a parseable findings block, so there are no inline comments. Its raw notes are below — treat them as unverified: line numbers were not validated against the diff.
…ty = NORMAL) ordering matters.Let me look at the ChunkLoader record to confirm
y()exists and check theaddTicket/removeTicketmismatch withsetChunkForceLoaded.Now I have enough context. Let me synthesize the new findings. Prior reviews covered:event.getKeepInventory() ||Let me verify which prior findings are now fixed vs still present, and identify genuinely NEW issues not yet covered.
The death-message bug (line 937) was fixed in this diff (line 939 now reads
event.getKeepInventory() ||). The interact handler now has the module check (line 152). Let me focus on new issues in the new chunk-loader simulation code (which wasn't deeply reviewed before) and any remaining gaps.I have enough context. Let me finalize my findings. Focusing on genuinely NEW issues not covered in prior reviews:HIGH —
ChunkLoaders.java:271—spawner.update(true, false)withforce=truetriggers a block update that can cascade; but more importantly, settingRequiredPlayerRangeto 2048 every tick on every spawner in every loaded chunk-loader chunk is a persistent mutation with no rollback on remove — spawners permanently keep the inflated range after the loader is removed. This is a resource-cleanup violation per repo config.MEDIUM —
ChunkLoaders.java:283— Mob cap of 20 is per-chunk-local butchunk.getEntities()returns entities in the chunk including those that wandered in; withsetRemoveWhenFarAway(false)(lines 230, 322), mobs never despawn, so the cap fills permanently and spawning stops forever after ~20 mobs.MEDIUM —
VoidProtection.java:61— TheMAX_CHECKED_COLUMNScap returnsringBestmid-scan, but if the cap is hit while the inner loops are still on an early ring, it returns the best from a partial ring and never checks further-out rings — potentially missing closer safe spots AND returning a spot that's not actually the nearest. Wait, this returns ringBest which is the best within the current partial ring, which is fine relative to prior rings. Actually this is a reasonable cap. Skip.MEDIUM —
VoidProtection.java:166— When the second chest overflows,inv.clear()andadjInv.clear()are called, but for a double chest the two inventories are a singleDoubleChestInventory. Callinginv.clear()thenadjInv.clear()is fine. But the real issue:addItemmay partially fill before returning leftover, andremainingis only the leftover. If the first chest is a single chest (27 slots), items are added, then the second chest is placed adjacent — but placing a chest adjacent to a chest makes a double chest, soadjInvis actually the same double-chest inventory.adjInv.addItem(rem)would try to add to the already-partly-filled double chest. This is correct behavior actually. Let me reconsider the original HIGH finding from prior review (line 162) — it's already covered. Skip.LOW —
ChunkLoaders.java:301—loader.y() + rnd.nextInt(-24, 25)spawns mobs in a vertical band around the anchor's Y, buttopY - 1ground check uses the block below — iftopY == minHeight + 2(the min clamp),groundatminHeight + 1might be below the world's actual terrain. Not a bug per se. Skip.HIGH —
ChunkLoaders.java:269-271— The spawner range mutation has no corresponding reset inremoveTicketorremove(). Let me verifyremove()doesn't reset spawners.I have enough context to finalize. Let me confirm theremove()method to verify the spawner-range cleanup gap is real.🤖 AI Review · pragent pilot · glm-5.2:cloud ·
5a6bb9a1PR adds the
salvavoidvoid-death item-rescue module and a chunk-loader mob-simulation subsystem (spawner activation + natural spawning + persistent mobs). Risk is concentrated in the chunk-loader changes, which permanently mutate world state (spawner range) and unboundedly persist mobs. The void-rescue path's core bugs were already flagged in prior reviews; this review finds new issues in the simulation lifecycle and missing unit tests for the new mob-selection logic. No Java toolchain was available, so compile/typecheck was skipped. 0 critical, 1 high, 2 medium, 1 low.🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 38 · duration: 242.1ssrc/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:270src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:229src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:330src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:20994 inline comment(s) posted below.
@@ -2041,0 +2096,4 @@}try {int newLimit = Math.max(0, Integer.parseInt(args[1]));Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getName() + " permission set canalhandia.chunkloader.limite." + newLimit);[LOW] chunkloaderLimit builds a LuckPerms console command from target.getName() via Bukkit.dispatchCommand without escaping; player names are Mojang-constrained so exploitation is unlikely, but a name containing spaces or quotes would break the command.
Fix: Pass the player UUID to the lp command instead of the name, or use the LuckPerms API directly.
🪙 ~2949 tok (26% · attributed output)
@@ -171,0 +226,4 @@return;}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);if (loader != null && event.getEntity() instanceof Mob mob) {[MEDIUM] onCreatureSpawn sets setRemoveWhenFarAway(false) on every Mob spawned in a loader chunk regardless of SpawnReason, so spawner/bred/summoned mobs never despawn and entity counts grow unbounded over time.
Fix: Gate the persistence on SpawnReason.NATURAL (and the plugin's own simulated spawns) so other spawn sources keep default despawn behaviour.
📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/event/entity/CreatureSpawnEvent.SpawnReason.html
🪙 ~3256 tok (29% · attributed output)
@@ -223,0 +267,4 @@for (BlockState state : chunk.getTileEntities()) {if (state instanceof CreatureSpawner spawner) {if (spawner.getRequiredPlayerRange() < 1024) {spawner.setRequiredPlayerRange(2048);[HIGH] tickMobSimulation sets every spawner's RequiredPlayerRange to 2048 and never reverts it; stopSimulation() only cancels the task and removeTicket() never restores the original range, so spawners in formerly chunk-loaded areas stay active at 2048 forever (persisted tile-entity state).
Fix: Record each modified spawner's original range and restore it in stopSimulation()/removeTicket(), or reset spawners in loaded loader chunks each tick toward a saved default.
📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/block/CreatureSpawner.html#setRequiredPlayerRange(int)
🪙 ~2744 tok (24% · attributed output)
@@ -223,0 +327,4 @@}}static EntityType pickEntityType(World w, Block space, int light) {[MEDIUM] pickEntityType encodes the mob-distribution logic (nether/end/overworld/slime branches) as a static, near-pure function but has no unit tests, violating the repo's 'Test Coverage' house instruction; only a noop test exists for tickMobSimulation.
Fix: Add JUnit cases for pickEntityType covering each environment and light/slime-chunk branch, stubbing World.getEnvironment() and Block.getChunk().isSlimeChunk().
🪙 ~2436 tok (21% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
ce3b1ebbAdds the salvavoid void-death item-rescue module (chest placement or keepInventory fallback) and a chunk-loader pause/rename/mob-simulation feature set. The change touches the PlayerDeathEvent item-loss risk path and chunk-ticket/entity lifecycle, so item safety and resource cleanup are the main concerns. The death-rescue contract itself is sound (rollback on chest failure → keepInventory fallback), but a PlayerDeathEvent listener-ordering bug makes the mortes death-coords message misleading; the new mob simulation leaves spawner ranges and entity despawn flags permanently mutated; and the new pure spawn-picker helper is untested. No Java toolchain was available, so mvn/typecheck could not be run.
🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 20 · duration: 199.7ssrc/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:846src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:354src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:270src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2167src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:286 inline comment(s) posted below.
@@ -840,0 +843,4 @@* Rescues player items when falling into the void. Places a chest on the nearest* safe ground block, or preserves items directly in the inventory if no solid ground is nearby.*/@EventHandler(priority = EventPriority.HIGH)[MEDIUM] onVoidDeath is registered at EventPriority.HIGH but onDeathComic (which captures keepInventory for the respawn message) is at default NORMAL, which fires first — so onDeathComic reads getKeepInventory() before onVoidDeath sets it, making the line-939 fix ineffective and the respawn message tells the player to 'go fetch your items' even when they were rescued to a chest or kept.
Fix: Run onVoidDeath at a lower priority than onDeathComic (e.g. EventPriority.LOWEST) so the keepInventory/drops state is final before the coords message is captured, or move the keepInventory read into onRespawn where the state is settled.
📎 ref: https://papermc.io/javadocs/paper/org/bukkit/event/EventPriority.html
🪙 ~3990 tok (21% · attributed output)
@@ -2041,0 +2164,4 @@}try {int newLimit = Math.max(0, Integer.parseInt(args[1]));Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getName() + " permission set canalhandia.chunkloader.limite." + newLimit);[LOW] The player's name is interpolated into a console command string via Bukkit.dispatchCommand; Bedrock/Geyser usernames can contain spaces and special characters, which could break or alter the dispatched LuckPerms command even though the sender is already an admin.
Fix: Pass arguments safely rather than building a raw command string, or validate/sanitize the target name (alphanumeric + underscore only) before interpolating, or use LuckPerms' API directly instead of dispatching a command.
🪙 ~3500 tok (19% · attributed output)
@@ -171,0 +267,4 @@}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);if (loader != null && event.getEntity() instanceof Mob mob) {mob.setRemoveWhenFarAway(false);[MEDIUM] onCreatureSpawn marks every Mob that spawns in a chunk-loader chunk as setRemoveWhenFarAway(false), including vanilla-spawned and wandering mobs, so they never despawn — causing unbounded persistent-entity accumulation across all active loaders over time.
Fix: Only persist mobs the plugin itself spawned (e.g. gate on CreatureSpawnEvent.SpawnReason.NATURAL from simulateNaturalSpawning, or tag spawned mobs and restrict the despawn override to tagged mobs), and let normal server-spawned mobs despawn by default.
🪙 ~4124 tok (22% · attributed output)
@@ -223,0 +351,4 @@for (BlockState state : chunk.getTileEntities()) {if (state instanceof CreatureSpawner spawner) {if (spawner.getRequiredPlayerRange() < 1024) {spawner.setRequiredPlayerRange(2048);[MEDIUM] tickMobSimulation permanently raises each spawner's RequiredPlayerRange to 2048 and never restores it when the loader is paused (setEnabled(false)) or removed, leaving world spawners permanently altered — violating the repo's resource-cleanup invariant.
Fix: Track the original range and restore it in removeTicket/setEnabled(false), or reset spawners in the loader's chunk to their vanilla range on disable.
🪙 ~3327 tok (18% · attributed output)
@@ -223,0 +411,4 @@}}static EntityType pickEntityType(World w, Block space, int light) {[LOW] pickEntityType is a new pure static helper with environment/light/slime-chunk branching logic and no unit test, contrary to the repo's 'all pure helpers must have corresponding JUnit tests' convention.
Fix: Add a VoidProtectionTest/ChunkLoaders test covering each environment branch (NETHER light thresholds, THE_END, overworld dark/slime chunks) for pickEntityType.
🪙 ~2006 tok (11% · attributed output)
@@ -0,0 +25,4 @@}@Testvoid handlesNullWorldOrLocationsGracefully() {[LOW] The only findSafeChestLocation test passes null world/location, so the actual spiral-scan, chunk-loaded-skip, and safe-ground selection logic has no test coverage.
Fix: Add tests with a mocked World (or a fake) exercising a loaded safe column, an unloaded chunk skip, and a nearest-ring selection to verify the scan returns the closest safe location.
🪙 ~1917 tok (10% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
1ff83053🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 13 · duration: 238.2s⚠️ The reviewer did not emit a parseable findings block, so there are no inline comments. Its raw notes are below — treat them as unverified: line numbers were not validated against the diff.
…a non-expired loader:
newExpires = expiresAt + negative→ could go below 0 → clamped to permanent! Bug: a non-admin sending/chunkloader tempo <id> -1would make their loader permanent (expiresAt clamped to 0). That's a privilege escalation / economy bypass. This is high.setRemoveWhenFarAway(false)on ALL mobs in loaded chunks permanently — bothonCreatureSpawn(for any spawn in a chunk-loader chunk) andtickMobSimulationset this. Mobs in force-loaded chunks never despawn → unbounded entity accumulation over time, server TPS degradation. medium (performance/lifecycle).onPlayerInteractfuel consumption bypasses owner check order — Actually the owner check is applied (isOwner || isAdmin). OK.Missing test coverage for
rescueToChesthappy path andfindSafeChestLocationactual search logic — tests only cover null/empty edge cases. The spec requires "empacotamento de inventário" tests. medium (require_tests: true in config).onVoidDeathrunsfindSafeChestLocationon the main thread synchronously duringPlayerDeathEvent— up to 120 columns ×getHighestBlockYAt+getBlockAtcalls. This is main-thread I/O/scan during a death event, which the repo instructions explicitly say to defer. Could cause lag on death. medium.chunkloaderLimitconsole command dispatch with string concatenation — even though admin-gated and int-parsed, dispatching"lp user " + name + " permission set ..."as console is a command-injection pattern. Iftarget.getName()ever contains a space or semicolon (Bukkit generally disallows but plugins/renames can produce edge cases), it could inject. low/medium. I'll fold as low.Let me verify finding #2 once more — the non-admin negative hours path:
hours = -1,hours <= 0 && isAdminis false (not admin) → else:millis = (long)(-1 * 3600_000L) = -3600000.addTime(id, -3600000): loader hasexpiresAt = 0(permanent default) →base = (0 > now)? ...→0 > nowis false →base = now.newExpires = now + (-3600000)→ negative →withExpiresAtclamps to 0 → stays permanent. For a timed loaderexpiresAt = now+10h:base = expiresAt,newExpires = expiresAt - 1h→ reduces time (expected). So negative hours for a permanent loader keeps it permanent (no harm), for timed reduces time. Not actually an escalation. Let me re-examine: the "permanent" clamping happens viaMath.max(0L, newExpiresAt). So negative input can only reduce or keep permanent. Not a privilege escalation. I'll drop finding #2.Let me reconsider the real
chunkloaderTimeadmin-permanent bug:addTime(loader.id(), -loader.expiresAt())— ifexpiresAt == 0,addTime(id, 0)→base = now,newExpires = now + 0 = now→ NOT 0!withExpiresAt(now)→ expires immediately, not permanent! This is a real bug: an admin trying to make an already-permanent (expiresAt=0) loader permanent via/chunkloader tempo <id> 0would set it to expire now (becomes expired and gets disabled on next tick). Let me verify:addTime:base = (curr.expiresAt() > now) ? curr.expiresAt() : now. ForexpiresAt=0,0 > nowis false →base = now.newExpires = now + (-0) = now.withExpiresAt(now)=Math.max(0, now) = now(positive) → loader becomes expired! ThentickMobSimulationseesisExpired() && enabled()→ disables it. Confirmed high-severity bug: making a permanent loader "permanent" actually expires it.Actually wait —
hours <= 0triggers the permanent branch.hours = 0→hours <= 0 && isAdmintrue →addTime(id, -0) = addTime(id, 0). Same issue. The intent is "set permanent" but the implementation setsexpiresAt = now. High severity.Let me refine the fix: should call a dedicated
setPermanent/withExpiresAt(0)instead ofaddTimewith a derived negative.Now let me finalize. Let me also double-check the
findthread-safety:byIdis synchronized, the name-iteration is synchronized — fine.Let me write up findings.
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
f2a19a72Tier: full (substantial feature PR across 13 files, touches item-loss-prevention and chunk-loading risk paths). Adds the
salvavoidvoid-death item-rescue module (chest placement or keepInventory fallback), plus chunk-loader enhancements: naming, enable/disable pause, time-based expiry with fuel, mob simulation, and per-player limits. Reviewed VoidProtection, ChunkLoaders, ChunkLoaderListener, Canalhandia command/handler, and tests inline; no subagent delegation (security surface is item-safety/chunk-loading, not SQL/auth/crypto). Linter skipped: Java/Maven build is too heavy to run in-scope and no checkstyle config is present. Overall risk is moderate: the new void-rescue and expiry logic have two real correctness bugs (permanent-time command and non-admin negative-time), and the new logic is under-tested (rollback/spawn paths untested). 8 findings: 2 high, 4 medium, 2 low.🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 12 · duration: 327.2ssrc/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2005src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:855src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:220src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:1src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:412src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:496src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:1967 inline comment(s) posted below.
src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:496— chunkloaderLimit dispatches 'lp user permission set ...' as console via string concatenation; player names are validated by Minecraft today, but a future name-charset change would let a crafted name inject extra LuckPerms subcommands. — fix: Validate target.getName() against [A-Za-z0-9_]{1,16} before dispatching, or use LuckPerms' API instead of a raw console command string.@@ -840,0 +852,4 @@EntityDamageEvent damage = player.getLastDamageCause();EntityDamageEvent.DamageCause cause = damage == null ? null : damage.getCause();if (!VoidProtection.isVoidDeath(player.getLocation().getY(), player.getWorld().getMinHeight(), cause)) {[MEDIUM] isVoidDeath's Y-only check (y < minHeight) misclassifies any non-void death below world min height (e.g. a FALL/ENTITY_ATTACK death in a deep cave near the floor) as a void death, triggering item rescue/keepInventory for deaths that were not actually void.
Fix: Gate the Y-based fallback to only fire when cause is null or VOID, not for every damage cause; or require y < minHeight AND cause in {VOID, null, FALL} to avoid false positives from mob/weapon deaths deep underground.
🪙 ~5831 tok (16% · attributed output)
@@ -1903,0 +1999,4 @@try {double hours = Double.parseDouble(args[1]);if (hours <= 0 && isAdmin) {plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent[HIGH] Admin 'permanent' command expires an already-permanent loader instead of leaving it permanent: when expiresAt==0, addTime(id, -0) computes base=now and newExpires=now, so the loader expires immediately rather than staying permanent.
Fix: Make the permanent path set expiresAt to 0 directly (or guard the addTime call against expiresAt==0), instead of subtracting the current expiresAt which only works when a timer is already running.
🪙 ~4422 tok (12% · attributed output)
@@ -1903,0 +2002,4 @@plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanentMsg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE.");} else {long millis = (long) (hours * 3600_000L);[HIGH] Non-admin owners can pass a negative or zero hours value (hours<=0 but not admin falls into the else branch), which addTime treats as a negative duration, letting them shorten or immediately expire their own loader — unintended and confusing griefing/self-sabotage path.
Fix: Validate that hours is strictly positive before applying time for non-admins; reject <=0 with an error message.
🪙 ~4393 tok (12% · attributed output)
@@ -98,0 +193,4 @@synchronized (loaders) {for (int i = 0; i < loaders.size(); i++) {ChunkLoader curr = loaders.get(i);if (curr.id() == id) {[LOW] find() searches all loaders globally by name (case-insensitive) without scoping to the sender, so duplicate names across owners resolve to the first match and make '/chunkloader ' ambiguous when two players pick the same label.
Fix: Scope name lookup by owner first, or require names to be unique per owner and fall back to a disambiguation prompt; at minimum, have find prefer the sender's own loaders when the sender is a Player.
🪙 ~3127 tok (9% · attributed output)
@@ -223,0 +409,4 @@for (BlockState state : chunk.getTileEntities()) {if (state instanceof CreatureSpawner spawner) {if (spawner.getRequiredPlayerRange() < 1024) {spawner.setRequiredPlayerRange(2048);[MEDIUM] simulateNaturalSpawning/pickEntityType are new domain logic with branching by environment, light level and slime-chunk rules but have no unit tests; only the null-plugin no-op path is covered. A logic regression (e.g. spawning in lit areas, wrong mob weights) would ship silently.
Fix: Extract the pure decision logic (pickEntityType given env/light/slime flags) and add JUnit cases for each environment and light threshold, like the existing coordinate-math tests.
🪙 ~3284 tok (9% · attributed output)
@@ -0,0 +217,4 @@if (s instanceof Chest c) c.getInventory().clear();chestBlock.setType(orig1, false);}if (adjacent != null && adjacent.getType() == Material.CHEST) {[MEDIUM] findAdjacentSpace may cross into a neighbouring chunk that is not loaded (only the death-location chunk is checked for loaded state in findSafeChestLocation); calling getBlockAt on an unloaded chunk can trigger a synchronous chunk load on the main thread, against the repo's chunk-loading invariant.
Fix: Guard findAdjacentSpace with an isChunkLoaded check for each candidate neighbour, mirroring the check done in findSafeChestLocation.
🪙 ~5717 tok (16% · attributed output)
@@ -0,0 +1,76 @@package dev.marcospaulo.canalhandia;[MEDIUM] The complex rollback/failure paths of rescueToChest (overflow -> second chest -> still-overflow -> full rollback, and the exception rollback) have no tests; only null/empty-input guards are covered. The repo config requires tests for changed domain logic, and this is the riskiest new logic (item loss on a botched rollback).
Fix: Add unit tests (with a mocked World/Block/Chest or a fake Inventory) covering: single-chest success, double-chest success, overflow that cannot place a second chest (rollback returns false), and double-chest overflow (full rollback returns false) verifying the original blocks are restored.
🪙 ~4400 tok (12% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
ee0d9338Adds void-death item rescue (chest on safe ground or keep-inventory fallback) and extends chunk loaders with pause/rename/fuel-time/expiration/mob-simulation. Most of the change is well-structured with rollback-on-overflow and loaded-chunk-only scans, but there is a high-severity privilege bypass in /chunkloader tempo (negative hours make a loader permanent for any owner), a medium coords-clobber between onVoidDeath and onDeathComic that misleads the player about where their items are, and a medium resource leak from setRemoveWhenFarAway(false) with no reversal on pause/remove. Severity counts: 1 high, 2 medium.
🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 16 · duration: 216.2ssrc/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2001src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:946src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:12 inline comment(s) posted below.
src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:946— onDeathComic (MONITOR) runs after onVoidDeath (HIGH) and unconditionally overwrites pendingDeathCoords, replacing the chest-rescue coordinates with the void death location; on respawn the player is then told 'Corre buscar seus itens!' pointing at the void, even though the items are safe in a chest elsewhere. — fix: Use putIfAbsent so onVoidDeath's chest/keep-inventory coordinates survive when void protection already recorded them.src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:1— no test file changed alongside this behavioral change (require_tests=true) — fix: add a unit test exercising the changed branch@@ -1903,0 +1998,4 @@}try {double hours = Double.parseDouble(args[1]);if (hours <= 0 && isAdmin) {[HIGH] A non-admin owner can pass a negative hours value (e.g. /chunkloader tempo -1); the
hours <= 0 && isAdminguard is false so it falls through to addTime(id, negativeMillis), and ChunkLoaders.addTime + withExpiresAt clamp the result to 0 = permanent, bypassing the admin-only permanent gate.Fix: Reject non-positive hours for non-admins before calling addTime.
🪙 ~11300 tok (44% · attributed output)
@@ -171,0 +311,4 @@}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);if (loader != null && loader.enabled() && event.getEntity() instanceof Mob mob) {mob.setRemoveWhenFarAway(false);[MEDIUM] setRemoveWhenFarAway(false) is set on every spawner/custom mob spawned in a chunk-loader chunk and never reverted, so mobs accumulate indefinitely (even after the loader is paused, expired, or removed) with no cleanup path, degrading TPS and memory over time.
Fix: Track flagged mobs per loader and reset the flag when the loader is disabled/removed, or scope the flag to the loader's lifetime instead of applying it globally with no reversal.
🪙 ~5809 tok (23% · attributed output)
ee0d93385dto07e5bb6018🤖 AI Review · pragent pilot · glm-5.2:cloud ·
07e5bb60Summary of Changes
VoidProtectionmodule detects void deaths and rescues drops into a chest on the nearest safe ground, falling back to keepInventory when no solid ground is in range; integrated asModule.SALVAVOIDwithsalvavoid.*settings.name,enabled,expiresAtplus with* mutators; new commands/chunkloader nome|ativar|desativar|tempo|limiteand right-click interact (shift to toggle, fuel to add time) with a LuckPerms-based limit system.tickMobSimulationscheduled task keeps spawners active (range 2048) and simulates natural mob spawning in chunk-loader chunks;setChunkForceLoadedadded alongside the plugin chunk ticket.onDeathComicnow readsevent.getKeepInventory()so void-protection fallback no longer triggers a misleading 'go get your items' message.Key Risks & Concerns
/chunkloader tempo <id> 0(or any hours<=0) issued by an admin on an ALREADY-permanent loader (expiresAt=0) computes newExpires = now + 0 = now, so the loader becomes immediately expired and is auto-disabled on the next sim tick — the opposite of 'permanent'./chunkloader ativaror shift-click) callssetEnabled(true)without clearingexpiresAt; the nexttickMobSimulationseesisExpired() && enabled()and disables it again, so the user's 'ATIVADA' message is misleading.tickMobSimulationmutates everyCreatureSpawnerin the chunk torequiredPlayerRange=2048viaupdate(true, false), which persists the change to world NBT — spawners stay globally active even after the chunk loader is removed or the plugin is uninstalled.onCreatureSpawncallssetRemoveWhenFarAway(false)on every Mob that spawns in a chunk-loader chunk; combined with the 2-mobs/sec simulation spawn this can accumulate mobs that never despawn, degrading TPS over time./chunkloader tempodoes not reject hours<=0 for non-admins, so a player can pass a negative value to shorten/expire their own loader's timer (or no-op it if currently permanent).VoidProtectionTestonly covers null/empty/cause cases; the spec's acceptance criterion for 'empacotamento de inventário' (chest packing/overflow) is untested, and the repo config setsrequire_tests: true.find()resolves numeric-looking custom names as IDs first, so a loader named '123' is unreachable by name if loader #123 exists.Findings Overview
7 inline comment(s); 7 total.
src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002addTime(id, -loader.expiresAt())passes 0;addTimecomputes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, soisExpired()becomes true and the nexttickMobSimulationauto-disables the loader. The 'PERMANENTE' success message is shown while the loader is actually set to expire immediately.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:371setEnabled(id, true)does not clearexpiresAt, so the nexttickMobSimulationiteration hitsloader.isExpired() && loader.enabled()and callssetEnabled(loader.id(), false). A player who runs/chunkloader ativar(or shift-clicks) on an expired loader gets 'ATIVADA' feedback but the loader is disabled ~1s later, and no ticket is effectively held.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:389spawner.setRequiredPlayerRange(2048); spawner.update(true, false)writes the change to the world's spawner NBT, so the spawner stays active-without-players even after the chunk loader is removed, the module is toggled off, or the plugin is uninstalled — a permanent, non-rolling-back world change.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:310setRemoveWhenFarAway(false)is applied to every Mob that spawns in a chunk-loader chunk, so mobs never despawn. Combined withsimulateNaturalSpawningadding up to 2 mobs/sec per active loader, mob counts grow without bound (the per-chunk cap of 20 only checks the loader's own chunk, not wandering mobs), degrading TPS over time.src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2000hours <= 0branch is admin-only, but a non-admin falls through tomillis = (long)(hours * 3600_000L)with a negative value, whichaddTimethen subtracts from the current expiry (or, for a permanent loader, no-ops). A player can thus expire a timed loader early or grief their own setup, and the success message reports the negative addition as if it were fuel.src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:39rescueToChestis untested. The spec (specs/void-protection/spec.md§3) lists 'empacotamento de inventário' as an acceptance criterion and.pr-review.jsonsetsrequire_tests: true, but the new tests only coverisVoidDeath, null/empty guards, and thenullblock checks — no case exercises single-chest fit, double-chest overflow, or rollback-on-failure, which are the paths most likely to silently lose items.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:196find()resolves numeric-looking queries as IDs first, so a loader whose custom name is purely numeric (e.g. '123') is shadowed whenever loader #123 exists —find("123")returns loader #123 by ID and never matches the name. The tab-completion also offers both, but the name path is unreachable.🔋 AI Usage & Run Details
glm-5.2:cloud· opencode · 12 steps · 218.5s@@ -1903,0 +1997,4 @@return;}try {double hours = Double.parseDouble(args[1]);🟡 [MEDIUM] Non-admins can pass negative hours to shorten their own loader's timer. The
hours <= 0branch is admin-only, but a non-admin falls through tomillis = (long)(hours * 3600_000L)with a negative value, whichaddTimethen subtracts from the current expiry (or, for a permanent loader, no-ops). A player can thus expire a timed loader early or grief their own setup, and the success message reports the negative addition as if it were fuel.Fix: Reject
hours <= 0for everyone and require a positive value before dispatching toaddTime.@@ -1903,0 +1999,4 @@try {double hours = Double.parseDouble(args[1]);if (hours <= 0 && isAdmin) {plugin.chunkLoaders().addTime(loader.id(), -loader.expiresAt()); // permanent🔴 [HIGH] Admin 'permanent' command on an already-permanent loader disables it. When expiresAt==0,
addTime(id, -loader.expiresAt())passes 0;addTimecomputes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, soisExpired()becomes true and the nexttickMobSimulationauto-disables the loader. The 'PERMANENTE' success message is shown while the loader is actually set to expire immediately.Fix: Detect the already-permanent case and set expiresAt to 0 explicitly (or add a dedicated
setPermanent(id)that callswithExpiresAt(0)).@@ -171,0 +307,4 @@}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);if (loader != null && event.getEntity() instanceof Mob mob) {mob.setRemoveWhenFarAway(false);🟡 [MEDIUM]
setRemoveWhenFarAway(false)is applied to every Mob that spawns in a chunk-loader chunk, so mobs never despawn. Combined withsimulateNaturalSpawningadding up to 2 mobs/sec per active loader, mob counts grow without bound (the per-chunk cap of 20 only checks the loader's own chunk, not wandering mobs), degrading TPS over time.Fix: Only flag mobs spawned by the simulation itself (track them), or apply a global/per-loader mob cap before spawning, and let naturally-wandering mobs despawn normally.
@@ -98,0 +193,4 @@/*** Finds a chunk loader by its numeric ID (with or without '#') or custom name.*/ChunkLoader find(String query) {🔵 [LOW]
find()resolves numeric-looking queries as IDs first, so a loader whose custom name is purely numeric (e.g. '123') is shadowed whenever loader #123 exists —find("123")returns loader #123 by ID and never matches the name. The tab-completion also offers both, but the name path is unreachable.Fix: If the ID lookup succeeds, also check for a name match and prefer the name only when no ID exists, or disallow purely-numeric custom names at rename time.
@@ -223,0 +368,4 @@}for (ChunkLoader loader : currentLoaders) {if (loader.isExpired() && loader.enabled()) {🔴 [HIGH] Re-enabling an expired loader is silently reverted on the next tick.
setEnabled(id, true)does not clearexpiresAt, so the nexttickMobSimulationiteration hitsloader.isExpired() && loader.enabled()and callssetEnabled(loader.id(), false). A player who runs/chunkloader ativar(or shift-clicks) on an expired loader gets 'ATIVADA' feedback but the loader is disabled ~1s later, and no ticket is effectively held.Fix: When enabling, clear the expiry (set expiresAt=0) or refuse to enable an expired loader and surface 'expired' to the caller.
@@ -223,0 +386,4 @@for (BlockState state : chunk.getTileEntities()) {if (state instanceof CreatureSpawner spawner) {if (spawner.getRequiredPlayerRange() < 1024) {spawner.setRequiredPlayerRange(2048);🟡 [MEDIUM] Spawner range is permanently mutated to 2048 and persisted.
spawner.setRequiredPlayerRange(2048); spawner.update(true, false)writes the change to the world's spawner NBT, so the spawner stays active-without-players even after the chunk loader is removed, the module is toggled off, or the plugin is uninstalled — a permanent, non-rolling-back world change.Fix: Restore the original range when the loader is disabled/removed (store the prior value), or use a non-persistent in-memory override rather than mutating spawner NBT.
🔗 Reference: minecraft.wiki/w/Spawner
@@ -0,0 +36,4 @@void nullBlocksAreNeitherSafeNorReplaceable() {assertFalse(VoidProtection.isSafeGround(null));assertFalse(VoidProtection.isReplaceable(null));}🟡 [MEDIUM] The chest-packing/overflow path of
rescueToChestis untested. The spec (specs/void-protection/spec.md§3) lists 'empacotamento de inventário' as an acceptance criterion and.pr-review.jsonsetsrequire_tests: true, but the new tests only coverisVoidDeath, null/empty guards, and thenullblock checks — no case exercises single-chest fit, double-chest overflow, or rollback-on-failure, which are the paths most likely to silently lose items.Fix: Add a unit test (with a stub
Inventory/Block) covering: items fit in one chest, overflow triggers the adjacent chest, and overflow beyond the double chest rolls back and returns false.🤖 AI Review · pragent pilot · glm-5.2:cloud ·
b6e508c8Summary of Changes
No summary provided.
Key Risks & Concerns
None identified.
⚠️ The reviewer did not emit a parseable findings block, so there are no inline comments. Its raw notes are below — treat them as unverified: line numbers were not validated against the diff.
…obs (
chunk.getEntities()), not the surrounding spawn area** — Minecraft's natural spawn cap is per-player/per-world, not per-chunk. ThemobCount >= 20check only counts mobs in that single chunk, so mobs in adjacent loaded chunks aren't counted, easily exceeding real mob caps. This is a logic concern but matches "farm simulation" intent. Low/medium.chunkloaderTimeadmin permanent bypass: Whenhours <= 0and admin, sets permanent. Non-admins get error. ButaddTimewithhours * 3600_000Lwhere hours is a double —(long)(hours * 3600_000L). If hours is very large (e.g. 1e9), overflow possible but Double.parseDouble large values → Long overflow → wraps to negative → addTime adds negative millis → could set expiresAt in the past → instantly expired. Edge case, low.chunkloaderLimitdispatches a console command to LuckPerms:Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getName() + " permission set ...").target.getName()is not sanitized but it's a player name (validated byBukkit.getPlayerExact), so safe from injection. OK.chunkloaderRename— no length cap on combined args beyond 32: capped at 32. OK. But rename allows any characters including color codes?String.join(" ")then trim. Could inject section signs / formatting. Minor.find()query resolution — name precedence over numeric ID: The test explicitly checks this. If a loader is named "1",find("1")returns the named one, shadowing ID 1 (accessible via "#1"). This is documented/tested. Not a bug.onPlayerInteractfor respawn anchor: cancels the event when a loader is at that location. This prevents the vanilla respawn-anchor charging (glowstone) behavior — which is intended since it's repurposed. But it cancels for ALL players including non-owners who just want to... well it's a chunk loader block now. The fuel detection runsfuelDuration(inHand)first; if fuel and owner/admin → refuel. If sneaking and owner/admin → toggle. Otherwise just shows info. But the event is cancelled unconditionally ifloader != null— meaning a non-owner non-admin right-clicking does nothing (vanilla behavior blocked). That's fine for a protected block but worth noting. Not a finding.onPlayerInteractcallsinHand.subtract(1)without checking stack amount / hand empty:getItemInMainHand()could be empty (AIR) butfuelDuration(AIR)returns 0 so it won't reach. OK. But doesn't checkinHand.getAmount() > 0explicitly — AIR amount is 0, returns 0 fuel. Fine.onPlayerInteractshift-click toggle without cooldown: A player toggling rapidly could spam ticket add/remove. Minor.onVoidDeathchest path: ifrescueToChestreturns false (rollback), falls into else branch which sets keepInventory — wait, let me re-check the condition:if (safeLoc != null && !drops.isEmpty() && VoidProtection.rescueToChest(drops, safeLoc)). If rescueToChest fails (returns false), the whole condition is false → else branch → keepInventory=true. Good fallback. But ifsafeLoc != nullanddrops.isEmpty()→ else branch sets keepInventory even though nothing to keep. Harmless.rescueToChestrollback restoresorig1/orig2but doesn't restore block state data (e.g., a grass block → chest → back to grass, loses nothing; but a sign with text → chest → back to sign loses text). Minor since location is "replaceable" material typically. Low.findSafeChestLocationreturnsspace.getLocation()which is the air block above ground — thenrescueToChestdoeschestBlock.setType(CHEST)on that air block. Good. But theLocationfromgetBlockAtincludes world + integer coords. OK.onVoidDeathpriority HIGH vs other death handler: Let me check the other handler at line 939 region — what event/priority? Need to verify it doesn't conflict. But thegetKeepInventory() ||fix suggests they coordinated. Let me check briefly.Let me look at the other handler.
🔋 AI Usage & Run Details
glm-5.2:cloud· opencode · 11 steps · 203.6s🤖 AI Review · pragent pilot · glm-5.2:cloud ·
8a34565fSummary of Changes
Key Risks & Concerns
Findings Overview
3 inline comment(s); 4 total.
src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314src/main/resources/config.yml:1src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:79src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:312Unanchored Notes
src/main/resources/config.yml:1— The salvavoid module reads salvavoid.raio-busca (default 32) and salvavoid.preservar-xp (default true) from config, but config.yml has no salvavoid section. Operators have nothing to discover or adjust until a setter first writes the keys, which is inconsistent with how other modules expose their defaults in the shipped config.🔋 AI Usage & Run Details
glm-5.2:cloud· opencode · 25 steps · 174.8s@@ -171,0 +309,4 @@if (w == null) {return;}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);🔵 [LOW] onCreatureSpawn marks every mob in a loader chunk as non-despawning regardless of spawn reason. A mob spawned by a plugin, a spawner, or a vanilla natural roll all get the flag, so the listener amplifies the leak from only simulated mobs to all mobs that happen to spawn while a loader is active. Scoping the flag to SpawnReason.NATURAL (or to mobs the simulation itself spawned) would limit the blast radius.
Fix: Guard the flag with a SpawnReason check (e.g. NATURAL/CUSTOM from the simulation) or only apply it in the simulateNaturalSpawning path, not the broad listener.
🪙 ~2730 tok (23% · attributed output)
@@ -171,0 +311,4 @@}ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);if (loader != null && event.getEntity() instanceof Mob mob) {mob.setRemoveWhenFarAway(false);🟡 [MEDIUM] Mobs in chunk-loader chunks are flagged setRemoveWhenFarAway(false) and never unflagged. The onCreatureSpawn listener fires for every CreatureSpawnEvent in a loader chunk (natural, spawner, plugin, and simulation spawns alike), and there is no symmetric cleanup in removeTicket/setEnabled/onDisable that resets these mobs to despawn normally. Once a loader is paused, removed, or expires, every mob it touched stays loaded-eligible forever, growing entity count and TPS cost over the server's lifetime. The simulation task in ChunkLoaders.tickMobSimulation also sets the same flag at line 477 with the same gap.
Fix: Track mobs flagged by the module (or re-scan the chunk on removeTicket/pause) and call setRemoveWhenFarAway(true) before releasing the ticket, so mobs revert to vanilla despawn behaviour.
🪙 ~3836 tok (33% · attributed output)
@@ -0,0 +76,4 @@@Testvoid calculatesRequiredSlotsAndChestFit() {assertEquals(0, VoidProtection.calculateRequiredSlots(null));🔵 [LOW] calculateRequiredSlots, canFitInSingleChest, canFitInDoubleChest, and rescueToChest are only tested with null/empty inputs. The slot-counting logic (ceil(amount/maxStack)), the single-vs-double chest threshold (27 vs 54), and the double-chest overflow rollback path in rescueToChest have no test with real ItemStack amounts, so a regression in the core item-loss-prevention math would pass silently. The repo config sets require_tests: true.
Fix: Add tests with ItemStacks that cross the 27-slot and 54-slot boundaries, and a rescueToChest rollback case where items exceed a double chest.
🪙 ~2798 tok (24% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
bc7a88b9Summary of Changes
salvavoidmodule: on void death, searches nearby loaded chunks for safe ground, places a chest (single/double) with the player's drops, or falls back to keep-inventory when no safe block is found.ChunkLoaderrecord withname,enabled,expiresAt; adds rename/pause/enable/time/fuel commands and right-click interaction (shift to toggle, fuel item to recharge).tickMobSimulation) that forces spawner ranges to 2048 and spawns natural mobs into force-loaded chunks, plus aCreatureSpawnEventlistener pinning mobs so they don't despawn.voidProtectionRadius/voidProtectionKeepXpsettings.Key Risks & Concerns
World.spawnEntity, mutatesCreatureSpawnerranges, and forces chunk loading — all gameplay changes the architecture doc explicitly forbids.isVoidDeathtreatsy < minHeightas void for ANY damage cause, so a normal deep-cave death (fall/mob/lava below Y=-64) is misclassified as a void death and items are chested or kept instead of dropping normally.onVoidDeathrunsfindSafeChestLocation(up to 120 synchronousgetHighestBlockYAt/getBlockAtcalls) andrescueToChest(block setType + inventory writes) on the main thread duringPlayerDeathEvent, against the house rule to defer expensive scans off the main thread.simulateNaturalSpawningruns every tick for every enabled loader and marks every spawned mobsetRemoveWhenFarAway(false), so entities accumulate indefinitely in force-loaded chunks with no cleanup, risking TPS loss and memory growth over time.rescueToChestoverwrites replaceable blocks (grass/snow/fern) with a chest, destroying player-placed decoration at the rescue site.rescueToChest's chest-placement, double-chest, and rollback-on-overflow paths are exercised only by null/empty-input tests; the actual block-mutation behavior is untested (require_tests: true).canalhandia.chunkloader.limite.<N>meta-permission is set via LuckPerms dispatch but not declared in plugin.yml, though this is intentional for a numerically-suffixed dynamic permission.Findings Overview
6 inline comment(s); 7 total.
src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:14isVoidDeathreturns true whenevery < minHeightregardless of damage cause, so any death below the world floor — a zombie kill in a deep cave, fall damage, lava — is misclassified as a void death. The player's drops are then chested or kept-inventory instead of dropping normally, silently overriding normal death mechanics for every deep-cave death. The testidentifiesVoidDeathByCoordinatesBelowMinHeightexplicitly assertsFALL/CUSTOMat y=-65 return true, confirming this is the implemented (not accidental) behavior.src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:860onVoidDeathcallsVoidProtection.findSafeChestLocation(up to 120 synchronousgetHighestBlockYAt+getBlockAtcalls across loaded chunks) andrescueToChest(blocksetType+ inventory writes) directly on the main thread inside thePlayerDeathEventhandler. The repo's review instructions explicitly require deferring expensive scans and chunk loading off the main thread; this can cause a noticeable tick hitch on death when many columns are scanned.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414simulateNaturalSpawningruns every 20 ticks for every enabled loader and callssetRemoveWhenFarAway(false)on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunkmobCount >= 20guard only counts the loader's own chunk), so entities accumulate indefinitely in loaded chunks with no cleanup path, risking TPS and memory growth over days of uptime.src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:173rescueToChestoverwrites thespaceblock (whichfindSafeChestLocationallows to beSHORT_GRASS,TALL_GRASS,SNOW,FERN,LARGE_FERN) with a chest, destroying whatever decoration block was there. A player's placed grass layer or snow cover at the rescue site is deleted without consent on someone else's death.src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:84VoidProtectionTestonly covers null/empty inputs and pure material predicates;rescueToChest's actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. Withrequire_tests: trueand the rollback logic being the safety guarantee that the caller falls back tokeepInventory, this is a real coverage gap for changed logic.src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:298onCreatureSpawnmarks everyMobspawned in a loader chunksetRemoveWhenFarAway(false), affecting not only the plugin's own simulated spawns but any natural/raid/spawner mob that appears in a force-loaded chunk. This persists for the mob's lifetime and applies even when the loader is later disabled, so mobs spawned during a loader's lifetime never despawn even after the anchor is removed.src/main/resources/config.yml:1salvavoid.raio-buscaandsalvavoid.preservar-xpkeys have no entries inconfig.yml; they fall back to the hardcoded defaults inSettings.java(32 and true). Every other module documents its config keys inconfig.yml, so operators won't discover these knobs without reading source.Unanchored Notes
src/main/resources/config.yml:1— The newsalvavoid.raio-buscaandsalvavoid.preservar-xpkeys have no entries inconfig.yml; they fall back to the hardcoded defaults inSettings.java(32 and true). Every other module documents its config keys inconfig.yml, so operators won't discover these knobs without reading source.salvavoid:section toconfig.ymlwithraio-busca: 32andpreservar-xp: trueand the pt-BR comments matching the rest of the file.🔋 AI Usage & Run Details
glm-5.2:cloud· opencode · 20 steps · 118.6s@@ -856,5 +856,5 @@List<ItemStack> drops = new ArrayList<>(event.getDrops());Location safeLoc = VoidProtection.findSafeChestLocation(player.getWorld(), player.getLocation(), settings.voidProtectionRadius());if (safeLoc != null && !drops.isEmpty() && VoidProtection.rescueToChest(drops, safeLoc)) {🔴 [HIGH]
onVoidDeathcallsVoidProtection.findSafeChestLocation(up to 120 synchronousgetHighestBlockYAt+getBlockAtcalls across loaded chunks) andrescueToChest(blocksetType+ inventory writes) directly on the main thread inside thePlayerDeathEventhandler. The repo's review instructions explicitly require deferring expensive scans and chunk loading off the main thread; this can cause a noticeable tick hitch on death when many columns are scanned.Fix: Defer the search and chest placement to a scheduled task (
runTask/runTaskLater) and capture only the drops list synchronously, or run the search async and only mutate blocks back on the main thread.🪙 ~996 tok (15% · attributed output)
🟡 [MEDIUM]
onCreatureSpawnmarks everyMobspawned in a loader chunksetRemoveWhenFarAway(false), affecting not only the plugin's own simulated spawns but any natural/raid/spawner mob that appears in a force-loaded chunk. This persists for the mob's lifetime and applies even when the loader is later disabled, so mobs spawned during a loader's lifetime never despawn even after the anchor is removed.Fix: Only pin mobs spawned by the plugin's own simulation (track them), or re-evaluate
setRemoveWhenFarAwaywhen a loader is disabled rather than unconditionally on every spawn event.🪙 ~863 tok (13% · attributed output)
@@ -410,5 +410,5 @@private void save() {YamlConfiguration yaml = buildYaml();io.execute(() -> {try {🔴 [HIGH]
simulateNaturalSpawningruns every 20 ticks for every enabled loader and callssetRemoveWhenFarAway(false)on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunkmobCount >= 20guard only counts the loader's own chunk), so entities accumulate indefinitely in loaded chunks with no cleanup path, risking TPS and memory growth over days of uptime.Fix: Cap total simulated mobs per loader (or globally), and either let mobs despawn normally or reap them when the loader is disabled/expired. Avoid
setRemoveWhenFarAway(false)on simulated spawns.🪙 ~938 tok (14% · attributed output)
@@ -10,5 +10,5 @@import org.bukkit.block.BlockState;import org.bukkit.block.Chest;import org.bukkit.block.DoubleChest;import org.bukkit.entity.Player;import org.bukkit.event.entity.EntityDamageEvent;🔴 [HIGH]
isVoidDeathreturns true whenevery < minHeightregardless of damage cause, so any death below the world floor — a zombie kill in a deep cave, fall damage, lava — is misclassified as a void death. The player's drops are then chested or kept-inventory instead of dropping normally, silently overriding normal death mechanics for every deep-cave death. The testidentifiesVoidDeathByCoordinatesBelowMinHeightexplicitly assertsFALL/CUSTOMat y=-65 return true, confirming this is the implemented (not accidental) behavior.Fix: Only treat
y < minHeightas a void death when the damage cause isVOID, or add a separate guard that the death actually occurred in the void (e.g. the player's death location is below the world's logical void threshold, not just any sub-minHeight Y).🪙 ~1509 tok (23% · attributed output)
@@ -169,5 +169,5 @@World w = loc.getWorld();if (w == null) return null;int x = loc.getBlockX();int y = loc.getBlockY();int z = loc.getBlockZ();🟡 [MEDIUM]
rescueToChestoverwrites thespaceblock (whichfindSafeChestLocationallows to beSHORT_GRASS,TALL_GRASS,SNOW,FERN,LARGE_FERN) with a chest, destroying whatever decoration block was there. A player's placed grass layer or snow cover at the rescue site is deleted without consent on someone else's death.Fix: Restrict
isReplaceableMaterialfor the chest space to air variants only, or require the space block to be strictly air before placing a chest.🪙 ~826 tok (12% · attributed output)
🟡 [MEDIUM]
VoidProtectionTestonly covers null/empty inputs and pure material predicates;rescueToChest's actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. Withrequire_tests: trueand the rollback logic being the safety guarantee that the caller falls back tokeepInventory, this is a real coverage gap for changed logic.Fix: Add unit tests (with a mocked or stub
World/Block) for: items fit in single chest, items require double chest, overflow beyond double chest triggers rollback and returns false.🪙 ~818 tok (12% · attributed output)
Pull request closed