feat(void): Proteção de itens contra morte no vácuo (Baú seguro ou inventário) #5

Closed
masi wants to merge 0 commits from feat/void-protection into main
Contributor

🛡️ Módulo de Proteção contra Morte no Vácuo (salvavoid)

O que faz:

  1. Detecção: Identifica quando um jogador morre por dano de vácuo (DamageCause.VOID ou Y < altura mínima do mundo).
  2. Baú em Terreno Seguro: Busca em espiral o bloco sólido seguro mais próximo num raio de 32 blocos. Se encontrar, cria um baú (ou baú duplo) e guarda todos os itens/drops, informando as coordenadas exatas no chat.
  3. Fallback para Inventário: Se cair no meio do nada (sem terra firme no raio de busca), preserva os itens e XP diretamente no inventário do jogador com mensagem informativa.
  4. Configuração e Módulo: Controlado via /canalhandia modulo salvavoid <on|off>.
  5. Testes: 349/349 testes passando (100%).
## 🛡️ Módulo de Proteção contra Morte no Vácuo (`salvavoid`) ### O que faz: 1. **Detecção:** Identifica quando um jogador morre por dano de vácuo (`DamageCause.VOID` ou Y < altura mínima do mundo). 2. **Baú em Terreno Seguro:** Busca em espiral o bloco sólido seguro mais próximo num raio de 32 blocos. Se encontrar, cria um baú (ou baú duplo) e guarda todos os itens/drops, informando as coordenadas exatas no chat. 3. **Fallback para Inventário:** Se cair no meio do nada (sem terra firme no raio de busca), preserva os itens e XP diretamente no inventário do jogador com mensagem informativa. 4. **Configuração e Módulo:** Controlado via `/canalhandia modulo salvavoid <on|off>`. 5. **Testes:** 349/349 testes passando (100%).
masi added 1 commit 2026-08-20 12:53:21 +00:00
masi added the AI-REVIEWAI-USAGE labels 2026-08-20 12:53:24 +00:00
pragent-bot bot reviewed 2026-08-20 12:56:52 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 24ab63b9

Tier: full (~280 changed lines, 6 files; touches item/inventory handling on the PlayerDeathEvent risk path). PR adds the salvavoid module: 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 7 · duration: 206.2s
  • tokens: 150536 in · 12899 out · 0 reasoning · cache 0 read / 0 write → 163435 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
HIGH src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:162 3248 25%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:67 3466 27%
MEDIUM src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28 2203 17%
LOW src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:3 3982 31%

4 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `24ab63b9` Tier: full (~280 changed lines, 6 files; touches item/inventory handling on the PlayerDeathEvent risk path). PR adds the `salvavoid` module: 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 7 · duration: 206.2s - tokens: 150536 in · 12899 out · 0 reasoning · cache 0 read / 0 write → 163435 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | HIGH | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:162` | 3248 | 25% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:67` | 3466 | 27% | | MEDIUM | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28` | 2203 | 17% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:3` | 3982 | 31% | _4 inline comment(s) posted below._ <!-- pragent:sha=24ab63b9be66cac5ca08e9a1bef114e7f61a5199 -->
@@ -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.

import org.bukkit.HeightMap;
import org.bukkit.Location;
import org.bukkit.Material;
import org.bukkit.World;
import org.bukkit.block.Block;
import org.bukkit.block.BlockState;
import org.bukkit.block.Chest;
import org.bukkit.event.entity.EntityDamageEvent;
import org.bukkit.inventory.Inventory;
import org.bukkit.inventory.ItemStack;

import java.util.ArrayList;
import java.util.List;

🪙 ~3982 tok (31% · attributed output)

**[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. ```java import org.bukkit.HeightMap; import org.bukkit.Location; import org.bukkit.Material; import org.bukkit.World; import org.bukkit.block.Block; import org.bukkit.block.BlockState; import org.bukkit.block.Chest; import org.bukkit.event.entity.EntityDamageEvent; import org.bukkit.inventory.Inventory; import org.bukkit.inventory.ItemStack; import java.util.ArrayList; import java.util.List; ``` 🪙 ~3982 tok (31% · attributed output)
masi marked this conversation as resolved
@@ -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)

**[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)
masi marked this conversation as resolved
@@ -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)

**[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)
masi marked this conversation as resolved
@@ -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)

**[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)
masi marked this conversation as resolved
masi added 1 commit 2026-08-20 13:02:03 +00:00
pragent-bot bot reviewed 2026-08-20 13:05:44 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · d93711e5

Adds 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 14 · duration: 217.4s
  • tokens: 409696 in · 12492 out · 0 reasoning · cache 0 read / 0 write → 422188 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
MEDIUM src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:131 4999 40%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:146 5189 42%
LOW src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:128 2304 18%

3 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `d93711e5` Adds 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 14 · duration: 217.4s - tokens: 409696 in · 12492 out · 0 reasoning · cache 0 read / 0 write → 422188 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:131` | 4999 | 40% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:146` | 5189 | 42% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:128` | 2304 | 18% | _3 inline comment(s) posted below._ <!-- pragent:sha=d93711e5c175696c54d7a8b744bb59aa537f7215 -->
@@ -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)

**[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)
masi marked this conversation as resolved
@@ -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.

public void onPlayerInteract(PlayerInteractEvent event) {
        if (event.getAction() != Action.RIGHT_CLICK_BLOCK) {
            return;
        }
        Block block = event.getClickedBlock();
        if (block == null || block.getType() != Material.RESPAWN_ANCHOR) {
            return;
        }
        if (!plugin.settings().moduleEnabled(Module.CHUNKLOADER)) {
            return;
        }

🪙 ~5189 tok (42% · attributed output)

**[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. ```java public void onPlayerInteract(PlayerInteractEvent event) { if (event.getAction() != Action.RIGHT_CLICK_BLOCK) { return; } Block block = event.getClickedBlock(); if (block == null || block.getType() != Material.RESPAWN_ANCHOR) { return; } if (!plugin.settings().moduleEnabled(Module.CHUNKLOADER)) { return; } ``` 🪙 ~5189 tok (42% · attributed output)
masi marked this conversation as resolved
@@ -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.

Block chestBlock = chestLoc.getBlock();
            Material original = chestBlock.getType();
            chestBlock.setType(Material.CHEST, false);

            BlockState state = chestBlock.getState();
            if (!(state instanceof Chest chest)) {
                chestBlock.setType(original, false);
                return false;
            }

🪙 ~4999 tok (40% · attributed output)

**[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. ```java Block chestBlock = chestLoc.getBlock(); Material original = chestBlock.getType(); chestBlock.setType(Material.CHEST, false); BlockState state = chestBlock.getState(); if (!(state instanceof Chest chest)) { chestBlock.setType(original, false); return false; } ``` 🪙 ~4999 tok (40% · attributed output)
masi marked this conversation as resolved
masi added 1 commit 2026-08-20 13:14:21 +00:00
Author
Contributor

Resoluções Reviews #94 e #95 + Configuração do pragent (commit c3c4906)

  1. [HIGH / MEDIUM] Transacionalidade & Rollback no Resgate do Vácuo (VoidProtection.java):
    • rescueToChest agora exige que todos os itens caibam no baú/baú duplo. Se houver qualquer overflow ou falha, o bloco é revertido ao estado original e retorna false, acionando com segurança o fallback de keepInventory.
  2. [MEDIUM] Otimização da Busca de Terreno (VoidProtection.java):
    • A busca agora só avalia chunks já carregadas (world.isChunkLoaded) e possui teto de colunas amostradas (MAX_CHECKED_COLUMNS = 120), evitando geração/carregamento síncrono no evento de morte.
  3. [LOW / MEDIUM] ChunkLoader (ChunkLoaderListener.java):
    • Adicionada checagem settings.moduleEnabled(Module.CHUNKLOADER) no onPlayerInteract.
    • Mensagem clara para quebra em Modo Criativo sem drop.
  4. [LOW] Limpeza de Imports & Testes Unitários:
    • Imports limpos e testes expandidos em VoidProtectionTest.java (350/350 testes passando).
  5. [NOVO] Configuração .pr-review.json:
    • Criado .pr-review.json no repositório configurando foco em segurança de threads, ciclo de vida Paper, prevenção de perda de itens e performance.
### ✅ Resoluções Reviews #94 e #95 + Configuração do `pragent` (commit `c3c4906`) 1. **[HIGH / MEDIUM] Transacionalidade & Rollback no Resgate do Vácuo (`VoidProtection.java`):** - `rescueToChest` agora exige que **todos** os itens caibam no baú/baú duplo. Se houver qualquer overflow ou falha, o bloco é revertido ao estado original e retorna `false`, acionando com segurança o fallback de `keepInventory`. 2. **[MEDIUM] Otimização da Busca de Terreno (`VoidProtection.java`):** - A busca agora só avalia chunks já carregadas (`world.isChunkLoaded`) e possui teto de colunas amostradas (`MAX_CHECKED_COLUMNS = 120`), evitando geração/carregamento síncrono no evento de morte. 3. **[LOW / MEDIUM] ChunkLoader (`ChunkLoaderListener.java`):** - Adicionada checagem `settings.moduleEnabled(Module.CHUNKLOADER)` no `onPlayerInteract`. - Mensagem clara para quebra em Modo Criativo sem drop. 4. **[LOW] Limpeza de Imports & Testes Unitários:** - Imports limpos e testes expandidos em `VoidProtectionTest.java` (350/350 testes passando). 5. **[NOVO] Configuração `.pr-review.json`:** - Criado `.pr-review.json` no repositório configurando foco em segurança de threads, ciclo de vida Paper, prevenção de perda de itens e performance.
pragent-bot bot reviewed 2026-08-20 13:16:36 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · c3c4906e

🔋 AI usage

  • model: glm-5.2:cloud · engine: opencode · agent steps: 7 · duration: 132.5s
  • tokens: 189254 in · 4514 out · 0 reasoning · cache 0 read / 0 write → 193768 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)

⚠️ 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 test handlesNullWorldOrLocationsGracefully passes null for both world and location to findSafeChestLocation, which doesn't exercise the actual scan logic. Prior review #1 flagged "new logic is largely untested" and specifically VoidProtectionTest.java:28. So this is already covered. Skip.

Finding 7 (LOW) — VoidProtection.java:75: world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES) can return world.getMaxHeight() (above the world) if the column is empty above the min height, meaning topY could be at or above the build limit. Then world.getBlockAt(x, topY+1, z) would be out of bounds. Actually getHighestBlockYAt returns the y of the highest block per the height map, or world.getMinHeight() if none. The check topY <= minHeight handles the "none" case. If topY is at max height, topY+1 is out of range — getBlockAt clamps or throws? Paper's getBlockAt with out-of-range y throws IllegalArgumentException in some versions. But this is wrapped in no try-catch at that point (only getHighestBlockYAt is in try-catch). So getBlockAt(x, topY+1, z) at line 85 could throw if topY == max height. This would propagate up to findSafeChestLocation and crash the death handler... but onVoidDeath doesn't wrap findSafeChestLocation in try-catch either. A thrown exception in onVoidDeath would prevent the fallback setKeepInventory(true) from running → item loss.

Wait, let me check: does Paper's getHighestBlockYAt with MOTION_BLOCKING_NO_LEAVES ever return max height? It returns the highest y where the height map indicates a block, or minHeight if empty. It should not return above max height. Actually per Paper docs, getHighestBlockYAt returns y of highest block, which is ≤ maxHeight - 1. So topY + 1 ≤ maxHeight. getBlockAt accepts y in [minHeight, maxHeight] inclusive? The world height is [minHeight, maxHeight] where maxHeight is exclusive top. So getBlockAt(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:

  1. MEDIUMCanalhandia.java:937 (in onDeathComic) reads keepInventory gamerule instead of event.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 changed Canalhandia.java by adding onVoidDeath. The onDeathComic line 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.

  2. LOWChunkLoaderListener.java:147 off-hand double-fire. Anchor at 147 (added line).

Let me also reconsider the onPlayerInteract — does it need an ignoreCancelled = 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 · `c3c4906e` ## 🔋 AI usage - model: `glm-5.2:cloud` · engine: opencode · agent steps: 7 · duration: 132.5s - tokens: 189254 in · 4514 out · 0 reasoning · cache 0 read / 0 write → 193768 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) ⚠️ _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 time `onDeathComic` runs. But `onDeathComic` 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 test `handlesNullWorldOrLocationsGracefully` passes `null` for both world and location to `findSafeChestLocation`, which doesn't exercise the actual scan logic. Prior review #1 flagged "new logic is largely untested" and specifically `VoidProtectionTest.java:28`. So this is already covered. Skip. **Finding 7 (LOW) — `VoidProtection.java:75`**: `world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES)` can return `world.getMaxHeight()` (above the world) if the column is empty above the min height, meaning `topY` could be at or above the build limit. Then `world.getBlockAt(x, topY+1, z)` would be out of bounds. Actually `getHighestBlockYAt` returns the y of the highest block per the height map, or `world.getMinHeight()` if none. The check `topY <= minHeight` handles the "none" case. If topY is at max height, `topY+1` is out of range — `getBlockAt` clamps or throws? Paper's `getBlockAt` with out-of-range y throws `IllegalArgumentException` in some versions. But this is wrapped in no try-catch at that point (only `getHighestBlockYAt` is in try-catch). So `getBlockAt(x, topY+1, z)` at line 85 could throw if topY == max height. This would propagate up to `findSafeChestLocation` and crash the death handler... but `onVoidDeath` doesn't wrap `findSafeChestLocation` in try-catch either. A thrown exception in `onVoidDeath` would prevent the fallback `setKeepInventory(true)` from running → **item loss**. Wait, let me check: does Paper's `getHighestBlockYAt` with `MOTION_BLOCKING_NO_LEAVES` ever return max height? It returns the highest y where the height map indicates a block, or `minHeight` if empty. It should not return above max height. Actually per Paper docs, `getHighestBlockYAt` returns `y` of highest block, which is ≤ `maxHeight - 1`. So `topY + 1 ≤ maxHeight`. `getBlockAt` accepts y in `[minHeight, maxHeight]` inclusive? The world height is `[minHeight, maxHeight]` where `maxHeight` is exclusive top. So `getBlockAt(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: 1. **MEDIUM** — `Canalhandia.java:937` (in `onDeathComic`) reads keepInventory *gamerule* instead of `event.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 changed `Canalhandia.java` by *adding* `onVoidDeath`. The `onDeathComic` line 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. 2. **LOW** — `ChunkLoaderListener.java:147` off-hand double-fire. Anchor at 147 (added line). Let me also reconsider the `onPlayerInteract` — does it need an `ignoreCancelled = 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. <!-- pragent:sha=c3c4906e1148fddd7cc1ce3eee1b65de377d5b38 -->
masi added 1 commit 2026-08-20 13:21:15 +00:00
pragent-bot bot reviewed 2026-08-20 13:25:45 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 2c7f5f98

PR adds 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 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 13 · duration: 267.1s
  • tokens: 460021 in · 17322 out · 0 reasoning · cache 0 read / 0 write → 477343 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
HIGH src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:148 6976 40%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:229 10346 60%

2 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `2c7f5f98` PR adds 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 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 13 · duration: 267.1s - tokens: 460021 in · 17322 out · 0 reasoning · cache 0 read / 0 write → 477343 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | HIGH | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:148` | 6976 | 40% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:229` | 10346 | 60% | _2 inline comment(s) posted below._ <!-- pragent:sha=2c7f5f98ad4f7e75e77a135b3ef02b252fe20c76 -->
@@ -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)

**[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.

for (int[] off : offsets) {
            int ax = x + off[0];
            int az = z + off[1];
            if (!w.isChunkLoaded(ax >> 4, az >> 4)) {
                continue;
            }
            Block b = w.getBlockAt(ax, y, az);
            Block ground = w.getBlockAt(ax, y - 1, az);
            if (isReplaceable(b) && isSafeGround(ground)) {
                return b;
            }
        }

🪙 ~10346 tok (60% · attributed output)

**[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. ```java for (int[] off : offsets) { int ax = x + off[0]; int az = z + off[1]; if (!w.isChunkLoaded(ax >> 4, az >> 4)) { continue; } Block b = w.getBlockAt(ax, y, az); Block ground = w.getBlockAt(ax, y - 1, az); if (isReplaceable(b) && isSafeGround(ground)) { return b; } } ``` 🪙 ~10346 tok (60% · attributed output)
masi added 1 commit 2026-08-20 14:16:44 +00:00
pragent-bot bot reviewed 2026-08-20 14:20:49 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 4261e8b6

Tier: 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 13 · duration: 243.3s
  • tokens: 430881 in · 16108 out · 0 reasoning · cache 0 read / 0 write → 446989 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:230 5039 31%
MEDIUM src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28 4994 31%
LOW src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:170 6075 38%

3 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `4261e8b6` Tier: 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 13 · duration: 243.3s - tokens: 430881 in · 16108 out · 0 reasoning · cache 0 read / 0 write → 446989 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:230` | 5039 | 31% | | MEDIUM | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28` | 4994 | 31% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:170` | 6075 | 38% | _3 inline comment(s) posted below._ <!-- pragent:sha=4261e8b660eb4f5c83224c399a1df66018ff54fc -->
@@ -131,0 +167,4 @@
);
if (loader != null) {
// Prevent respawn anchor exploding or depleting charges when used as chunk loader
event.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.

if (loader != null) {
            if (event.getPlayer().isSneaking() && event.getItem() != null
                    && event.getItem().getType().isBlock()) {
                return;
            }
            event.setCancelled(true);
            Player player = event.getPlayer();
            Msg.ok(player, "Âncora de Chunk #" + loader.id() + " (" + loader.ownerName() + ") está ativa.");
        }

📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/event/player/PlayerInteractEvent.html

🪙 ~6075 tok (38% · attributed output)

**[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. ```java if (loader != null) { if (event.getPlayer().isSneaking() && event.getItem() != null && event.getItem().getType().isBlock()) { return; } event.setCancelled(true); Player player = event.getPlayer(); Msg.ok(player, "Âncora de Chunk #" + loader.id() + " (" + loader.ownerName() + ") está ativa."); } ``` 📎 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.

w.addPluginChunkTicket(loader.chunkX(), loader.chunkZ(), plugin);

📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/World.html#addPluginChunkTicket(int,int,org.bukkit.plugin.Plugin)

🪙 ~5039 tok (31% · attributed output)

**[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. ```java w.addPluginChunkTicket(loader.chunkX(), loader.chunkZ(), plugin); ``` 📎 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 @@
}
@Test
void 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)

**[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)
masi added 1 commit 2026-08-20 15:53:27 +00:00
masi added 1 commit 2026-08-20 15:55:42 +00:00
pragent-bot bot reviewed 2026-08-20 15:56:49 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 4b9507b3

🔋 AI usage

  • model: glm-5.2:cloud · engine: opencode · agent steps: 14 · duration: 200.0s
  • tokens: 507343 in · 5876 out · 0 reasoning · cache 0 read / 0 write → 513219 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)

⚠️ 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 the addTicket/removeTicket mismatch with setChunkForceLoaded.Now I have enough context. Let me synthesize the new findings. Prior reviews covered:

  1. Rescue chest item loss/dup bug (HIGH, line 162)
  2. O(n²) scan hotspot (MEDIUM, line 67)
  3. Weak tests (MEDIUM, line 28)
  4. Low finding (line 3)
  5. Orphaned chest on rescue failure (MEDIUM, line 131)
  6. Missing module check on anchor interact (MEDIUM, line 146) — wait, the code now HAS a module check at line 152, so that was fixed
  7. Creative-mode refund gap (LOW, line 128)
  8. Death message keepInventory bug (MEDIUM, line 937) — FIXED via line 939 event.getKeepInventory() ||
  9. Off-hand double-fire (LOW, line 147)

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:

  1. HIGH — ChunkLoaders.java:271spawner.update(true, false) with force=true triggers a block update that can cascade; but more importantly, setting RequiredPlayerRange to 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.

  2. MEDIUM — ChunkLoaders.java:283 — Mob cap of 20 is per-chunk-local but chunk.getEntities() returns entities in the chunk including those that wandered in; with setRemoveWhenFarAway(false) (lines 230, 322), mobs never despawn, so the cap fills permanently and spawning stops forever after ~20 mobs.

  3. MEDIUM — VoidProtection.java:61 — The MAX_CHECKED_COLUMNS cap returns ringBest mid-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.

  4. MEDIUM — VoidProtection.java:166 — When the second chest overflows, inv.clear() and adjInv.clear() are called, but for a double chest the two inventories are a single DoubleChestInventory. Calling inv.clear() then adjInv.clear() is fine. But the real issue: addItem may partially fill before returning leftover, and remaining is 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, so adjInv is 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.

  5. LOW — ChunkLoaders.java:301loader.y() + rnd.nextInt(-24, 25) spawns mobs in a vertical band around the anchor's Y, but topY - 1 ground check uses the block below — if topY == minHeight + 2 (the min clamp), ground at minHeight + 1 might be below the world's actual terrain. Not a bug per se. Skip.

  6. HIGH — ChunkLoaders.java:269-271 — The spawner range mutation has no corresponding reset in removeTicket or remove(). Let me verify remove() doesn't reset spawners.I have enough context to finalize. Let me confirm the remove() method to verify the spawner-range cleanup gap is real.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `4b9507b3` ## 🔋 AI usage - model: `glm-5.2:cloud` · engine: opencode · agent steps: 14 · duration: 200.0s - tokens: 507343 in · 5876 out · 0 reasoning · cache 0 read / 0 write → 513219 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) ⚠️ _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 the `addTicket`/`removeTicket` mismatch with `setChunkForceLoaded`.Now I have enough context. Let me synthesize the new findings. Prior reviews covered: 1. Rescue chest item loss/dup bug (HIGH, line 162) 2. O(n²) scan hotspot (MEDIUM, line 67) 3. Weak tests (MEDIUM, line 28) 4. Low finding (line 3) 5. Orphaned chest on rescue failure (MEDIUM, line 131) 6. Missing module check on anchor interact (MEDIUM, line 146) — wait, the code now HAS a module check at line 152, so that was fixed 7. Creative-mode refund gap (LOW, line 128) 8. Death message keepInventory bug (MEDIUM, line 937) — FIXED via line 939 `event.getKeepInventory() ||` 9. Off-hand double-fire (LOW, line 147) 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: 1. **HIGH — `ChunkLoaders.java:271`** — `spawner.update(true, false)` with `force=true` triggers a block update that can cascade; but more importantly, setting `RequiredPlayerRange` to 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. 2. **MEDIUM — `ChunkLoaders.java:283`** — Mob cap of 20 is per-chunk-local but `chunk.getEntities()` returns entities in the chunk including those that wandered in; with `setRemoveWhenFarAway(false)` (lines 230, 322), mobs never despawn, so the cap fills permanently and spawning stops forever after ~20 mobs. 3. **MEDIUM — `VoidProtection.java:61`** — The `MAX_CHECKED_COLUMNS` cap returns `ringBest` mid-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. 4. **MEDIUM — `VoidProtection.java:166`** — When the second chest overflows, `inv.clear()` and `adjInv.clear()` are called, but for a double chest the two inventories are a single `DoubleChestInventory`. Calling `inv.clear()` then `adjInv.clear()` is fine. But the real issue: `addItem` may partially fill before returning leftover, and `remaining` is 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, so `adjInv` is 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. 5. **LOW — `ChunkLoaders.java:301`** — `loader.y() + rnd.nextInt(-24, 25)` spawns mobs in a vertical band around the anchor's Y, but `topY - 1` ground check uses the block below — if `topY == minHeight + 2` (the min clamp), `ground` at `minHeight + 1` might be below the world's actual terrain. Not a bug per se. Skip. 6. **HIGH — `ChunkLoaders.java:269-271`** — The spawner range mutation has no corresponding reset in `removeTicket` or `remove()`. Let me verify `remove()` doesn't reset spawners.I have enough context to finalize. Let me confirm the `remove()` method to verify the spawner-range cleanup gap is real. <!-- pragent:sha=4b9507b36e388a5ee32f06cad493f40bc6155afd -->
pragent-bot bot reviewed 2026-08-20 15:59:46 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 5a6bb9a1

PR adds the salvavoid void-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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 38 · duration: 242.1s
  • tokens: 1686782 in · 11385 out · 0 reasoning · cache 0 read / 0 write → 1698167 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
HIGH src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:270 2744 24%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:229 3256 29%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:330 2436 21%
LOW src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2099 2949 26%

4 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `5a6bb9a1` PR adds the `salvavoid` void-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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 38 · duration: 242.1s - tokens: 1686782 in · 11385 out · 0 reasoning · cache 0 read / 0 write → 1698167 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | HIGH | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:270` | 2744 | 24% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:229` | 3256 | 29% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:330` | 2436 | 21% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2099` | 2949 | 26% | _4 inline comment(s) posted below._ <!-- pragent:sha=5a6bb9a13dc7d40439f5d69a831c1411649cc59e -->
@@ -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.

Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getUniqueId() + " permission set canalhandia.chunkloader.limite." + newLimit);

🪙 ~2949 tok (26% · attributed output)

**[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. ```java Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getUniqueId() + " permission set canalhandia.chunkloader.limite." + newLimit); ``` 🪙 ~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.

if (loader != null && event.getEntity() instanceof Mob mob
                && event.getSpawnReason() == CreatureSpawnEvent.SpawnReason.NATURAL) {
            mob.setRemoveWhenFarAway(false);
        }

📎 ref: https://jd.papermc.io/paper/1.21/org/bukkit/event/entity/CreatureSpawnEvent.SpawnReason.html

🪙 ~3256 tok (29% · attributed output)

**[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. ```java if (loader != null && event.getEntity() instanceof Mob mob && event.getSpawnReason() == CreatureSpawnEvent.SpawnReason.NATURAL) { mob.setRemoveWhenFarAway(false); } ``` 📎 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)

**[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)

**[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)
masi added 1 commit 2026-08-20 16:13:11 +00:00
pragent-bot bot reviewed 2026-08-20 16:16:32 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · ce3b1ebb

Adds 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 20 · duration: 199.7s
  • tokens: 896162 in · 18865 out · 0 reasoning · cache 0 read / 0 write → 915027 total
  • est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
MEDIUM src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:846 3990 21%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:354 3327 18%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:270 4124 22%
LOW src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2167 3500 19%
LOW src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414 2006 11%
LOW src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28 1917 10%

6 inline comment(s) posted below.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `ce3b1ebb` Adds 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 20 · duration: 199.7s - tokens: 896162 in · 18865 out · 0 reasoning · cache 0 read / 0 write → 915027 total - est. cost: $0.00 (on-network glm-5.2:cloud via headroom — no per-token charge) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:846` | 3990 | 21% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:354` | 3327 | 18% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:270` | 4124 | 22% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2167` | 3500 | 19% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414` | 2006 | 11% | | LOW | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:28` | 1917 | 10% | _6 inline comment(s) posted below._ <!-- pragent:sha=ce3b1ebbd9ab3ebdfed3075ef772b024c1e83a42 -->
@@ -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.

@EventHandler(priority = EventPriority.LOWEST)
    public void onVoidDeath(PlayerDeathEvent event) {

📎 ref: https://papermc.io/javadocs/paper/org/bukkit/event/EventPriority.html

🪙 ~3990 tok (21% · attributed output)

**[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. ```java @EventHandler(priority = EventPriority.LOWEST) public void onVoidDeath(PlayerDeathEvent event) { ``` 📎 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.

Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getName() + " permission set canalhandia.chunkloader.limite." + newLimit);

🪙 ~3500 tok (19% · attributed output)

**[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. ```java Bukkit.dispatchCommand(Bukkit.getConsoleSender(), "lp user " + target.getName() + " permission set canalhandia.chunkloader.limite." + newLimit); ``` 🪙 ~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.

ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4);
        if (loader != null && event.getEntity() instanceof Mob mob) {
            mob.setRemoveWhenFarAway(false);
        }

🪙 ~4124 tok (22% · attributed output)

**[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. ```java ChunkLoader loader = plugin.chunkLoaders().byChunk(w.getName(), loc.getBlockX() >> 4, loc.getBlockZ() >> 4); if (loader != null && event.getEntity() instanceof Mob mob) { mob.setRemoveWhenFarAway(false); } ``` 🪙 ~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.

if (spawner.getRequiredPlayerRange() < 1024) {
                            spawner.setRequiredPlayerRange(2048);
                            spawner.update(true, false);
                        }

🪙 ~3327 tok (18% · attributed output)

**[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. ```java if (spawner.getRequiredPlayerRange() < 1024) { spawner.setRequiredPlayerRange(2048); spawner.update(true, false); } ``` 🪙 ~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)

**[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 @@
}
@Test
void 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)

**[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)
masi added 1 commit 2026-08-20 16:20:01 +00:00
pragent-bot bot reviewed 2026-08-20 16:24:02 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 1ff83053

🔋 AI usage

  • model: glm-5.2:cloud · engine: opencode · agent steps: 13 · duration: 238.2s
  • tokens: 571929 in · 24237 out · 0 reasoning · cache 0 read / 0 write → 596166 total
  • est. cost on Claude Sonnet 5: $1.3862
  • actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)

⚠️ 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> -1 would make their loader permanent (expiresAt clamped to 0). That's a privilege escalation / economy bypass. This is high.

  1. setRemoveWhenFarAway(false) on ALL mobs in loaded chunks permanently — both onCreatureSpawn (for any spawn in a chunk-loader chunk) and tickMobSimulation set this. Mobs in force-loaded chunks never despawn → unbounded entity accumulation over time, server TPS degradation. medium (performance/lifecycle).

  2. onPlayerInteract fuel consumption bypasses owner check order — Actually the owner check is applied (isOwner || isAdmin). OK.

  3. Missing test coverage for rescueToChest happy path and findSafeChestLocation actual search logic — tests only cover null/empty edge cases. The spec requires "empacotamento de inventário" tests. medium (require_tests: true in config).

  4. onVoidDeath runs findSafeChestLocation on the main thread synchronously during PlayerDeathEvent — up to 120 columns × getHighestBlockYAt + getBlockAt calls. 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.

  5. chunkloaderLimit console 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. If target.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 && isAdmin is false (not admin) → else: millis = (long)(-1 * 3600_000L) = -3600000. addTime(id, -3600000): loader has expiresAt = 0 (permanent default) → base = (0 > now)? ...0 > now is false → base = now. newExpires = now + (-3600000) → negative → withExpiresAt clamps to 0 → stays permanent. For a timed loader expiresAt = 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 via Math.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 chunkloaderTime admin-permanent bug: addTime(loader.id(), -loader.expiresAt()) — if expiresAt == 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> 0 would set it to expire now (becomes expired and gets disabled on next tick). Let me verify: addTime: base = (curr.expiresAt() > now) ? curr.expiresAt() : now. For expiresAt=0, 0 > now is false → base = now. newExpires = now + (-0) = now. withExpiresAt(now) = Math.max(0, now) = now (positive) → loader becomes expired! Then tickMobSimulation sees isExpired() && enabled() → disables it. Confirmed high-severity bug: making a permanent loader "permanent" actually expires it.

Actually wait — hours <= 0 triggers the permanent branch. hours = 0hours <= 0 && isAdmin true → addTime(id, -0) = addTime(id, 0). Same issue. The intent is "set permanent" but the implementation sets expiresAt = now. High severity.

Let me refine the fix: should call a dedicated setPermanent/withExpiresAt(0) instead of addTime with a derived negative.

Now let me finalize. Let me also double-check the find thread-safety: byId is synchronized, the name-iteration is synchronized — fine.

Let me write up findings.

🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `1ff83053` ## 🔋 AI usage - model: `glm-5.2:cloud` · engine: opencode · agent steps: 13 · duration: 238.2s - tokens: 571929 in · 24237 out · 0 reasoning · cache 0 read / 0 write → 596166 total - est. cost on **Claude Sonnet 5**: $1.3862 - actual: $0.00 (headroom glm-5.2:cloud — free tier) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) ⚠️ _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> -1` would make their loader permanent (expiresAt clamped to 0). That's a privilege escalation / economy bypass. This is **high**. 3. **`setRemoveWhenFarAway(false)` on ALL mobs in loaded chunks permanently** — both `onCreatureSpawn` (for any spawn in a chunk-loader chunk) and `tickMobSimulation` set this. Mobs in force-loaded chunks never despawn → unbounded entity accumulation over time, server TPS degradation. **medium** (performance/lifecycle). 4. **`onPlayerInteract` fuel consumption bypasses owner check order** — Actually the owner check is applied (`isOwner || isAdmin`). OK. 5. **Missing test coverage** for `rescueToChest` happy path and `findSafeChestLocation` actual search logic — tests only cover null/empty edge cases. The spec requires "empacotamento de inventário" tests. **medium** (require_tests: true in config). 6. **`onVoidDeath` runs `findSafeChestLocation` on the main thread** synchronously during `PlayerDeathEvent` — up to 120 columns × `getHighestBlockYAt` + `getBlockAt` calls. 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**. 7. **`chunkloaderLimit` console 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. If `target.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 && isAdmin` is false (not admin) → else: `millis = (long)(-1 * 3600_000L) = -3600000`. `addTime(id, -3600000)`: loader has `expiresAt = 0` (permanent default) → `base = (0 > now)? ...` → `0 > now` is false → `base = now`. `newExpires = now + (-3600000)` → negative → `withExpiresAt` clamps to 0 → **stays permanent**. For a timed loader `expiresAt = 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 via `Math.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 `chunkloaderTime` admin-permanent bug: `addTime(loader.id(), -loader.expiresAt())` — if `expiresAt == 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> 0` would set it to expire **now** (becomes expired and gets disabled on next tick). Let me verify: `addTime`: `base = (curr.expiresAt() > now) ? curr.expiresAt() : now`. For `expiresAt=0`, `0 > now` is false → `base = now`. `newExpires = now + (-0) = now`. `withExpiresAt(now)` = `Math.max(0, now) = now` (positive) → loader becomes expired! Then `tickMobSimulation` sees `isExpired() && enabled()` → disables it. **Confirmed high-severity bug**: making a permanent loader "permanent" actually expires it. Actually wait — `hours <= 0` triggers the permanent branch. `hours = 0` → `hours <= 0 && isAdmin` true → `addTime(id, -0) = addTime(id, 0)`. Same issue. The intent is "set permanent" but the implementation sets `expiresAt = now`. High severity. Let me refine the fix: should call a dedicated `setPermanent`/`withExpiresAt(0)` instead of `addTime` with a derived negative. Now let me finalize. Let me also double-check the `find` thread-safety: `byId` is synchronized, the name-iteration is synchronized — fine. Let me write up findings. <!-- pragent:sha=1ff830537e0a71d2095169b3856a971174c2487e -->
pragent-bot bot added 1 commit 2026-08-20 16:38:44 +00:00
pragent-bot bot reviewed 2026-08-20 16:44:13 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · f2a19a72

Tier: full (substantial feature PR across 13 files, touches item-loss-prevention and chunk-loading risk paths). Adds the salvavoid void-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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 12 · duration: 327.2s
  • tokens: 579602 in · 35580 out · 0 reasoning · cache 0 read / 0 write → 615182 total
  • est. cost on Claude Sonnet 5: $1.5150
  • actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
HIGH src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002 4422 12%
HIGH src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2005 4393 12%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:855 5831 16%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:220 5717 16%
MEDIUM src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:1 4400 12%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:412 3284 9%
LOW src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:496 4407 12%
LOW src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:196 3127 9%

7 inline comment(s) posted below.

  • [LOW] 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.
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `f2a19a72` Tier: full (substantial feature PR across 13 files, touches item-loss-prevention and chunk-loading risk paths). Adds the `salvavoid` void-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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 12 · duration: 327.2s - tokens: 579602 in · 35580 out · 0 reasoning · cache 0 read / 0 write → 615182 total - est. cost on **Claude Sonnet 5**: $1.5150 - actual: $0.00 (headroom glm-5.2:cloud — free tier) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | HIGH | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002` | 4422 | 12% | | HIGH | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2005` | 4393 | 12% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:855` | 5831 | 16% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:220` | 5717 | 16% | | MEDIUM | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:1` | 4400 | 12% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:412` | 3284 | 9% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:496` | 4407 | 12% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:196` | 3127 | 9% | _7 inline comment(s) posted below._ - **[LOW]** `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:496` — chunkloaderLimit dispatches 'lp user <name> 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. <!-- pragent:sha=f2a19a729a189924e7073628c03e73f3856a376b -->
@@ -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.

public static boolean isVoidDeath(double y, double minHeight, EntityDamageEvent.DamageCause cause) {
        if (cause == EntityDamageEvent.DamageCause.VOID) {
            return true;
        }
        if (cause == null || cause == EntityDamageEvent.DamageCause.FALL) {
            return y < minHeight;
        }
        return false;
    }

🪙 ~5831 tok (16% · attributed output)

**[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. ```java public static boolean isVoidDeath(double y, double minHeight, EntityDamageEvent.DamageCause cause) { if (cause == EntityDamageEvent.DamageCause.VOID) { return true; } if (cause == null || cause == EntityDamageEvent.DamageCause.FALL) { return y < minHeight; } return false; } ``` 🪙 ~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.

if (hours <= 0 && isAdmin) {
                plugin.chunkLoaders().setPermanent(loader.id());
                Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE.");

🪙 ~4422 tok (12% · attributed output)

**[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. ```java if (hours <= 0 && isAdmin) { plugin.chunkLoaders().setPermanent(loader.id()); Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE."); ``` 🪙 ~4422 tok (12% · attributed output)
@@ -1903,0 +2002,4 @@
plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent
Msg.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.

double hours = Double.parseDouble(args[1]);
            if (hours <= 0 && !isAdmin) {
                Msg.error(sender, "Horas deve ser maior que zero.");
                return;
            }
            if (hours <= 0 && isAdmin) {

🪙 ~4393 tok (12% · attributed output)

**[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. ```java double hours = Double.parseDouble(args[1]); if (hours <= 0 && !isAdmin) { Msg.error(sender, "Horas deve ser maior que zero."); return; } if (hours <= 0 && isAdmin) { ``` 🪙 ~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)

**[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 <cmd> <name>' 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)

**[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.

int[][] offsets = {{1, 0}, {-1, 0}, {0, 1}, {0, -1}};
        for (int[] off : offsets) {
            int nx = x + off[0];
            int nz = z + off[1];
            if (!w.isChunkLoaded(nx >> 4, nz >> 4)) {
                continue;
            }
            Block b = w.getBlockAt(nx, y, z + off[1]);
            Block ground = w.getBlockAt(nx, y - 1, z + off[1]);

🪙 ~5717 tok (16% · attributed output)

**[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. ```java int[][] offsets = {{1, 0}, {-1, 0}, {0, 1}, {0, -1}}; for (int[] off : offsets) { int nx = x + off[0]; int nz = z + off[1]; if (!w.isChunkLoaded(nx >> 4, nz >> 4)) { continue; } Block b = w.getBlockAt(nx, y, z + off[1]); Block ground = w.getBlockAt(nx, y - 1, z + off[1]); ``` 🪙 ~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)

**[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)
pragent-bot bot reviewed 2026-08-20 16:47:46 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · ee0d9338

Adds 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

  • model: glm-5.2:cloud · engine: opencode · agent steps: 16 · duration: 216.2s
  • tokens: 716126 in · 25591 out · 0 reasoning · cache 0 read / 0 write → 741717 total
  • est. cost on Claude Sonnet 5: $1.6882
  • actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff
  • per-comment tokens are attributed (one model pass produces all findings; output split by each finding's body weight)
severity location ≈out tok %
HIGH src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2001 11300 44%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:946 6900 27%
MEDIUM src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314 5809 23%
LOW src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:1 1582 6%

2 inline comment(s) posted below.

  • [MEDIUM] 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.
  • [LOW] 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
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `ee0d9338` Adds 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 - model: `glm-5.2:cloud` · engine: opencode · agent steps: 16 · duration: 216.2s - tokens: 716126 in · 25591 out · 0 reasoning · cache 0 read / 0 write → 741717 total - est. cost on **Claude Sonnet 5**: $1.6882 - actual: $0.00 (headroom glm-5.2:cloud — free tier) - scope: whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff - per-comment tokens are *attributed* (one model pass produces all findings; output split by each finding's body weight) | severity | location | ≈out tok | % | |---|---|---:|---:| | HIGH | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2001` | 11300 | 44% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:946` | 6900 | 27% | | MEDIUM | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314` | 5809 | 23% | | LOW | `src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:1` | 1582 | 6% | _2 inline comment(s) posted below._ - **[MEDIUM]** `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. - **[LOW]** `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 <!-- pragent:sha=ee0d93385d9ea2d4eba58616d9d142c9d3e004c7 -->
@@ -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 && isAdmin guard 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.

if (hours <= 0 && isAdmin) {
                plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent
                Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE.");
            } else if (hours <= 0) {
                Msg.error(sender, "Horas deve ser maior que zero.");
                return;
            } else {
                long millis = (long) (hours * 3600_000L);
                plugin.chunkLoaders().addTime(loader.id(), millis);

🪙 ~11300 tok (44% · attributed output)

**[HIGH]** A non-admin owner can pass a negative hours value (e.g. /chunkloader tempo <myloader> -1); the `hours <= 0 && isAdmin` guard 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. ```java if (hours <= 0 && isAdmin) { plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE."); } else if (hours <= 0) { Msg.error(sender, "Horas deve ser maior que zero."); return; } else { long millis = (long) (hours * 3600_000L); plugin.chunkLoaders().addTime(loader.id(), millis); ``` 🪙 ~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)

**[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)
masi marked this conversation as resolved
pragent-bot bot added 1 commit 2026-08-20 17:18:02 +00:00
pragent-bot bot force-pushed feat/void-protection from ee0d93385d to 07e5bb6018 2026-08-20 17:18:02 +00:00 Compare
pragent-bot bot reviewed 2026-08-20 17:21:45 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 07e5bb60

Summary of Changes

  • New VoidProtection module 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 as Module.SALVAVOID with salvavoid.* settings.
  • ChunkLoader record gains name, enabled, expiresAt plus with* mutators; new commands /chunkloader nome|ativar|desativar|tempo|limite and right-click interact (shift to toggle, fuel to add time) with a LuckPerms-based limit system.
  • New tickMobSimulation scheduled task keeps spawners active (range 2048) and simulates natural mob spawning in chunk-loader chunks; setChunkForceLoaded added alongside the plugin chunk ticket.
  • BlueMap markers, command list, and tab-completion updated to show status/time/names; onDeathComic now reads event.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'.
  • Reactivating an expired loader (/chunkloader ativar or shift-click) calls setEnabled(true) without clearing expiresAt; the next tickMobSimulation sees isExpired() && enabled() and disables it again, so the user's 'ATIVADA' message is misleading.
  • tickMobSimulation mutates every CreatureSpawner in the chunk to requiredPlayerRange=2048 via update(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.
  • onCreatureSpawn calls setRemoveWhenFarAway(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 tempo does 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).
  • VoidProtectionTest only covers null/empty/cause cases; the spec's acceptance criterion for 'empacotamento de inventário' (chest packing/overflow) is untested, and the repo config sets require_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.

Severity Location Finding
🔴 [HIGH] src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002 Admin 'permanent' command on an already-permanent loader disables it. When expiresAt==0, addTime(id, -loader.expiresAt()) passes 0; addTime computes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, so isExpired() becomes true and the next tickMobSimulation auto-disables the loader. The 'PERMANENTE' success message is shown while the loader is actually set to expire immediately.
🔴 [HIGH] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:371 Re-enabling an expired loader is silently reverted on the next tick. setEnabled(id, true) does not clear expiresAt, so the next tickMobSimulation iteration hits loader.isExpired() && loader.enabled() and calls setEnabled(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.
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:389 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.
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:310 setRemoveWhenFarAway(false) is applied to every Mob that spawns in a chunk-loader chunk, so mobs never despawn. Combined with simulateNaturalSpawning adding 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.
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2000 Non-admins can pass negative hours to shorten their own loader's timer. The hours <= 0 branch is admin-only, but a non-admin falls through to millis = (long)(hours * 3600_000L) with a negative value, which addTime then 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.
🟡 [MEDIUM] src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:39 The chest-packing/overflow path of rescueToChest is untested. The spec (specs/void-protection/spec.md §3) lists 'empacotamento de inventário' as an acceptance criterion and .pr-review.json sets require_tests: true, but the new tests only cover isVoidDeath, null/empty guards, and the null block checks — no case exercises single-chest fit, double-chest overflow, or rollback-on-failure, which are the paths most likely to silently lose items.
🔵 [LOW] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:196 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.
🔋 AI Usage & Run Details
  • Model / Engine: glm-5.2:cloud · opencode · 12 steps · 218.5s
  • Total Tokens: 558338 in / 16137 out (0 reasoning, cache 0 read / 0 write, 574475 total)
  • Est. cost on Claude Sonnet 5: $1.2780
  • Actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `07e5bb60` ### Summary of Changes - New `VoidProtection` module 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 as `Module.SALVAVOID` with `salvavoid.*` settings. - ChunkLoader record gains `name`, `enabled`, `expiresAt` plus with* mutators; new commands `/chunkloader nome|ativar|desativar|tempo|limite` and right-click interact (shift to toggle, fuel to add time) with a LuckPerms-based limit system. - New `tickMobSimulation` scheduled task keeps spawners active (range 2048) and simulates natural mob spawning in chunk-loader chunks; `setChunkForceLoaded` added alongside the plugin chunk ticket. - BlueMap markers, command list, and tab-completion updated to show status/time/names; `onDeathComic` now reads `event.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'. - Reactivating an expired loader (`/chunkloader ativar` or shift-click) calls `setEnabled(true)` without clearing `expiresAt`; the next `tickMobSimulation` sees `isExpired() && enabled()` and disables it again, so the user's 'ATIVADA' message is misleading. - `tickMobSimulation` mutates every `CreatureSpawner` in the chunk to `requiredPlayerRange=2048` via `update(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. - `onCreatureSpawn` calls `setRemoveWhenFarAway(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 tempo` does 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). - `VoidProtectionTest` only covers null/empty/cause cases; the spec's acceptance criterion for 'empacotamento de inventário' (chest packing/overflow) is untested, and the repo config sets `require_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._ | Severity | Location | Finding | |---|---|---| | 🔴 [HIGH] | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2002` | Admin 'permanent' command on an already-permanent loader disables it. When expiresAt==0, `addTime(id, -loader.expiresAt())` passes 0; `addTime` computes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, so `isExpired()` becomes true and the next `tickMobSimulation` auto-disables the loader. The 'PERMANENTE' success message is shown while the loader is actually set to expire immediately. | | 🔴 [HIGH] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:371` | Re-enabling an expired loader is silently reverted on the next tick. `setEnabled(id, true)` does not clear `expiresAt`, so the next `tickMobSimulation` iteration hits `loader.isExpired() && loader.enabled()` and calls `setEnabled(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. | | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:389` | 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. | | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:310` | `setRemoveWhenFarAway(false)` is applied to every Mob that spawns in a chunk-loader chunk, so mobs never despawn. Combined with `simulateNaturalSpawning` adding 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. | | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/CanalhandiaCommand.java:2000` | Non-admins can pass negative hours to shorten their own loader's timer. The `hours <= 0` branch is admin-only, but a non-admin falls through to `millis = (long)(hours * 3600_000L)` with a negative value, which `addTime` then 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. | | 🟡 [MEDIUM] | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:39` | The chest-packing/overflow path of `rescueToChest` is untested. The spec (`specs/void-protection/spec.md` §3) lists 'empacotamento de inventário' as an acceptance criterion and `.pr-review.json` sets `require_tests: true`, but the new tests only cover `isVoidDeath`, null/empty guards, and the `null` block checks — no case exercises single-chest fit, double-chest overflow, or rollback-on-failure, which are the paths most likely to silently lose items. | | 🔵 [LOW] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:196` | `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. | <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `glm-5.2:cloud` · opencode · 12 steps · 218.5s - **Total Tokens**: 558338 in / 16137 out (0 reasoning, cache 0 read / 0 write, 574475 total) - **Est. cost on Claude Sonnet 5**: $1.2780 - **Actual**: $0.00 (headroom glm-5.2:cloud — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=07e5bb601833703f6dbacd9e82541aa3a98c5277 -->
@@ -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 <= 0 branch is admin-only, but a non-admin falls through to millis = (long)(hours * 3600_000L) with a negative value, which addTime then 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 <= 0 for everyone and require a positive value before dispatching to addTime.

double hours = Double.parseDouble(args[1]);
            if (hours <= 0) {
                Msg.error(sender, "Use um valor de horas maior que zero (admin pode usar 0 para tornar permanente).");
                return;
            }
            if (isAdmin) {
🟡 [MEDIUM] Non-admins can pass negative hours to shorten their own loader's timer. The `hours <= 0` branch is admin-only, but a non-admin falls through to `millis = (long)(hours * 3600_000L)` with a negative value, which `addTime` then 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 <= 0` for everyone and require a positive value before dispatching to `addTime`. ```suggestion double hours = Double.parseDouble(args[1]); if (hours <= 0) { Msg.error(sender, "Use um valor de horas maior que zero (admin pode usar 0 para tornar permanente)."); return; } if (isAdmin) { ```
@@ -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; addTime computes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, so isExpired() becomes true and the next tickMobSimulation auto-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 calls withExpiresAt(0)).

if (hours <= 0 && isAdmin) {
                plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent
                Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE.");
🔴 [HIGH] Admin 'permanent' command on an already-permanent loader disables it. When expiresAt==0, `addTime(id, -loader.expiresAt())` passes 0; `addTime` computes base=now (since expiresAt<=now) and newExpires=now+0=now, which is >0, so `isExpired()` becomes true and the next `tickMobSimulation` auto-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 calls `withExpiresAt(0)`). ```suggestion if (hours <= 0 && isAdmin) { plugin.chunkLoaders().setExpiresAt(loader.id(), 0L); // permanent Msg.ok(sender, "Âncora " + loader.displayName() + " definida como PERMANENTE."); ```
@@ -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 with simulateNaturalSpawning adding 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.

🟡 [MEDIUM] `setRemoveWhenFarAway(false)` is applied to every Mob that spawns in a chunk-loader chunk, so mobs never despawn. Combined with `simulateNaturalSpawning` adding 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.

🔵 [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 clear expiresAt, so the next tickMobSimulation iteration hits loader.isExpired() && loader.enabled() and calls setEnabled(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.

if (enabled) {
                updated = curr.withEnabled(true).withExpiresAt(0L);
                loaders.set(i, updated);
🔴 [HIGH] Re-enabling an expired loader is silently reverted on the next tick. `setEnabled(id, true)` does not clear `expiresAt`, so the next `tickMobSimulation` iteration hits `loader.isExpired() && loader.enabled()` and calls `setEnabled(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. ```suggestion if (enabled) { updated = curr.withEnabled(true).withExpiresAt(0L); loaders.set(i, updated); ```
@@ -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

🟡 [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](https://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 rescueToChest is untested. The spec (specs/void-protection/spec.md §3) lists 'empacotamento de inventário' as an acceptance criterion and .pr-review.json sets require_tests: true, but the new tests only cover isVoidDeath, null/empty guards, and the null block 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.

🟡 [MEDIUM] The chest-packing/overflow path of `rescueToChest` is untested. The spec (`specs/void-protection/spec.md` §3) lists 'empacotamento de inventário' as an acceptance criterion and `.pr-review.json` sets `require_tests: true`, but the new tests only cover `isVoidDeath`, null/empty guards, and the `null` block 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.
masi added 2 commits 2026-08-20 17:51:10 +00:00
pragent-bot bot reviewed 2026-08-20 17:54:39 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · b6e508c8

Summary 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. The mobCount >= 20 check 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.

  1. chunkloaderTime admin permanent bypass: When hours <= 0 and admin, sets permanent. Non-admins get error. But addTime with hours * 3600_000L where 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.

  2. chunkloaderLimit dispatches 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 by Bukkit.getPlayerExact), so safe from injection. OK.

  3. 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.

  4. 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.

  5. onPlayerInteract for 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 runs fuelDuration(inHand) first; if fuel and owner/admin → refuel. If sneaking and owner/admin → toggle. Otherwise just shows info. But the event is cancelled unconditionally if loader != 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.

  6. onPlayerInteract calls inHand.subtract(1) without checking stack amount / hand empty: getItemInMainHand() could be empty (AIR) but fuelDuration(AIR) returns 0 so it won't reach. OK. But doesn't check inHand.getAmount() > 0 explicitly — AIR amount is 0, returns 0 fuel. Fine.

  7. onPlayerInteract shift-click toggle without cooldown: A player toggling rapidly could spam ticket add/remove. Minor.

  8. onVoidDeath chest path: if rescueToChest returns 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 if safeLoc != null and drops.isEmpty() → else branch sets keepInventory even though nothing to keep. Harmless.

  9. rescueToChest rollback restores orig1/orig2 but 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.

  10. findSafeChestLocation returns space.getLocation() which is the air block above ground — then rescueToChest does chestBlock.setType(CHEST) on that air block. Good. But the Location from getBlockAt includes world + integer coords. OK.

  11. onVoidDeath priority 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 the getKeepInventory() || fix suggests they coordinated. Let me check briefly.

Let me look at the other handler.

🔋 AI Usage & Run Details
  • Model / Engine: glm-5.2:cloud · opencode · 11 steps · 203.6s
  • Total Tokens: 432909 in / 11659 out (0 reasoning, cache 0 read / 0 write, 444568 total)
  • Est. cost on Claude Sonnet 5: $0.9824
  • Actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `b6e508c8` ### Summary 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. The `mobCount >= 20` check 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. 10. **`chunkloaderTime` admin permanent bypass**: When `hours <= 0` and admin, sets permanent. Non-admins get error. But `addTime` with `hours * 3600_000L` where 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. 11. **`chunkloaderLimit` dispatches 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 by `Bukkit.getPlayerExact`), so safe from injection. OK. 12. **`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. 13. **`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. 14. **`onPlayerInteract` for 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 runs `fuelDuration(inHand)` first; if fuel and owner/admin → refuel. If sneaking and owner/admin → toggle. Otherwise just shows info. But the event is cancelled unconditionally if `loader != 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. 15. **`onPlayerInteract` calls `inHand.subtract(1)` without checking stack amount / hand empty**: `getItemInMainHand()` could be empty (AIR) but `fuelDuration(AIR)` returns 0 so it won't reach. OK. But doesn't check `inHand.getAmount() > 0` explicitly — AIR amount is 0, returns 0 fuel. Fine. 16. **`onPlayerInteract` shift-click toggle without cooldown**: A player toggling rapidly could spam ticket add/remove. Minor. 17. **`onVoidDeath` chest path: if `rescueToChest` returns 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 if `safeLoc != null` and `drops.isEmpty()` → else branch sets keepInventory even though nothing to keep. Harmless. 18. **`rescueToChest` rollback restores `orig1`/`orig2` but 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. 19. **`findSafeChestLocation` returns `space.getLocation()` which is the air block above ground** — then `rescueToChest` does `chestBlock.setType(CHEST)` on that air block. Good. But the `Location` from `getBlockAt` includes world + integer coords. OK. 20. **`onVoidDeath` priority 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 the `getKeepInventory() ||` fix suggests they coordinated. Let me check briefly. Let me look at the other handler. <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `glm-5.2:cloud` · opencode · 11 steps · 203.6s - **Total Tokens**: 432909 in / 11659 out (0 reasoning, cache 0 read / 0 write, 444568 total) - **Est. cost on Claude Sonnet 5**: $0.9824 - **Actual**: $0.00 (headroom glm-5.2:cloud — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=b6e508c8c9f4937d226e956842075b978d9a1847 -->
gitea_admin added 1 commit 2026-08-20 17:56:04 +00:00
pragent-bot bot reviewed 2026-08-20 17:59:02 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · 8a34565f

Summary of Changes

  • New salvavoid module: detects void death (DamageCause.VOID or Y<minHeight), spiral-searches loaded chunks for safe ground, places a single/double chest with rollback on overflow, or falls back to keepInventory when no safe ground is nearby.
  • ChunkLoader record extended with name, enabled, expiresAt; new commands for rename/ativar/desativar/tempo/limite plus right-click fueling and shift-click toggle on the anchor block.
  • ChunkLoaders gains a per-tick mob simulation task (spawner range boost + natural spawning) and a CreatureSpawnEvent listener that marks mobs non-despawning in loaded chunks.
  • BlueMap marker updated to show status (Ativo/Pausado) and display name; plugin.yml chunkloader usage line updated to list new subcommands.

Key Risks & Concerns

  • Permanent mob leak: setRemoveWhenFarAway(false) is applied to every mob in a chunk-loader chunk (both simulated spawns and the onCreatureSpawn listener) and never reverted when the loader is paused, removed, or expires — those mobs never despawn, accumulating entity load over time.
  • onCreatureSpawn flags ALL CreatureSpawnEvent mobs in a loader chunk, not just the ones the simulation spawned, so even normal ambient spawns become permanent.
  • salvavoid config keys (salvavoid.raio-busca, salvavoid.preservar-xp) have no default section in config.yml, so operators only see them after first changing a value via the setter.
  • VoidProtectionTest only exercises null/empty inputs for rescueToChest, findSafeChestLocation, and chest-fit helpers; the slot-counting and double-chest overflow/rollback paths have no test with real ItemStack amounts.
  • rescueToChest places a chest in the world with no protection; any player can loot it before the victim returns — accepted by the spec, but worth noting for ops.

Findings Overview

3 inline comment(s); 4 total.

Severity Location Finding
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314 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.
🔵 [LOW] 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.
🔵 [LOW] src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:79 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.
🔵 [LOW] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:312 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.

Unanchored Notes

  • 🔵 [LOW] 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.
    • Fix: Add a salvavoid: { raio-busca: 32, preservar-xp: true } block to config.yml alongside the other module sections.
🔋 AI Usage & Run Details
  • Model / Engine: glm-5.2:cloud · opencode · 25 steps · 174.8s
  • Total Tokens: 1255146 in / 11715 out (0 reasoning, cache 0 read / 0 write, 1266861 total)
  • Est. cost on Claude Sonnet 5: $2.6274
  • Actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `8a34565f` ### Summary of Changes - New salvavoid module: detects void death (DamageCause.VOID or Y<minHeight), spiral-searches loaded chunks for safe ground, places a single/double chest with rollback on overflow, or falls back to keepInventory when no safe ground is nearby. - ChunkLoader record extended with name, enabled, expiresAt; new commands for rename/ativar/desativar/tempo/limite plus right-click fueling and shift-click toggle on the anchor block. - ChunkLoaders gains a per-tick mob simulation task (spawner range boost + natural spawning) and a CreatureSpawnEvent listener that marks mobs non-despawning in loaded chunks. - BlueMap marker updated to show status (Ativo/Pausado) and display name; plugin.yml chunkloader usage line updated to list new subcommands. ### Key Risks & Concerns - Permanent mob leak: setRemoveWhenFarAway(false) is applied to every mob in a chunk-loader chunk (both simulated spawns and the onCreatureSpawn listener) and never reverted when the loader is paused, removed, or expires — those mobs never despawn, accumulating entity load over time. - onCreatureSpawn flags ALL CreatureSpawnEvent mobs in a loader chunk, not just the ones the simulation spawned, so even normal ambient spawns become permanent. - salvavoid config keys (salvavoid.raio-busca, salvavoid.preservar-xp) have no default section in config.yml, so operators only see them after first changing a value via the setter. - VoidProtectionTest only exercises null/empty inputs for rescueToChest, findSafeChestLocation, and chest-fit helpers; the slot-counting and double-chest overflow/rollback paths have no test with real ItemStack amounts. - rescueToChest places a chest in the world with no protection; any player can loot it before the victim returns — accepted by the spec, but worth noting for ops. ### Findings Overview _3 inline comment(s); 4 total._ | Severity | Location | Finding | |---|---|---| | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:314` | 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. | | 🔵 [LOW] | `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. | | 🔵 [LOW] | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:79` | 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. | | 🔵 [LOW] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:312` | 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. | ### Unanchored Notes - 🔵 [LOW] `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. - **Fix:** Add a salvavoid: { raio-busca: 32, preservar-xp: true } block to config.yml alongside the other module sections. <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `glm-5.2:cloud` · opencode · 25 steps · 174.8s - **Total Tokens**: 1255146 in / 11715 out (0 reasoning, cache 0 read / 0 write, 1266861 total) - **Est. cost on Claude Sonnet 5**: $2.6274 - **Actual**: $0.00 (headroom glm-5.2:cloud — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=8a34565f3c1083826eed2b95706bc96cd5a44eb9 -->
@@ -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)

🔵 [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)
masi marked this conversation as resolved
@@ -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)

🟡 [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)
masi marked this conversation as resolved
@@ -0,0 +76,4 @@
@Test
void 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)

🔵 [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)
masi marked this conversation as resolved
masi added 1 commit 2026-08-20 18:08:45 +00:00
pragent-bot bot reviewed 2026-08-20 18:10:47 +00:00
pragent-bot bot left a comment

🤖 AI Review · pragent pilot · glm-5.2:cloud · bc7a88b9

Summary of Changes

  • Adds salvavoid module: 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.
  • Extends ChunkLoader record with name, enabled, expiresAt; adds rename/pause/enable/time/fuel commands and right-click interaction (shift to toggle, fuel item to recharge).
  • Adds a per-tick mob-simulation task (tickMobSimulation) that forces spawner ranges to 2048 and spawns natural mobs into force-loaded chunks, plus a CreatureSpawnEvent listener pinning mobs so they don't despawn.
  • Adds persistence for name/enabled/expiresAt in chunks.yml and exposes voidProtectionRadius/voidProtectionKeepXp settings.

Key Risks & Concerns

  • Breaks the repo's hard 'chat-only — never touch gameplay' constraint: the PR places/overwrites blocks, calls World.spawnEntity, mutates CreatureSpawner ranges, and forces chunk loading — all gameplay changes the architecture doc explicitly forbids.
  • isVoidDeath treats y < minHeight as 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.
  • onVoidDeath runs findSafeChestLocation (up to 120 synchronous getHighestBlockYAt/getBlockAt calls) and rescueToChest (block setType + inventory writes) on the main thread during PlayerDeathEvent, against the house rule to defer expensive scans off the main thread.
  • simulateNaturalSpawning runs every tick for every enabled loader and marks every spawned mob setRemoveWhenFarAway(false), so entities accumulate indefinitely in force-loaded chunks with no cleanup, risking TPS loss and memory growth over time.
  • rescueToChest overwrites 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).
  • New 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.

Severity Location Finding
🔴 [HIGH] src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:14 isVoidDeath returns true whenever y < minHeight regardless 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 test identifiesVoidDeathByCoordinatesBelowMinHeight explicitly asserts FALL/CUSTOM at y=-65 return true, confirming this is the implemented (not accidental) behavior.
🔴 [HIGH] src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:860 onVoidDeath calls VoidProtection.findSafeChestLocation (up to 120 synchronous getHighestBlockYAt + getBlockAt calls across loaded chunks) and rescueToChest (block setType + inventory writes) directly on the main thread inside the PlayerDeathEvent handler. 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.
🔴 [HIGH] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414 simulateNaturalSpawning runs every 20 ticks for every enabled loader and calls setRemoveWhenFarAway(false) on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunk mobCount >= 20 guard 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.
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:173 rescueToChest overwrites the space block (which findSafeChestLocation allows to be SHORT_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.
🟡 [MEDIUM] src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:84 VoidProtectionTest only covers null/empty inputs and pure material predicates; rescueToChest's actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. With require_tests: true and the rollback logic being the safety guarantee that the caller falls back to keepInventory, this is a real coverage gap for changed logic.
🟡 [MEDIUM] src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:298 onCreatureSpawn marks every Mob spawned in a loader chunk setRemoveWhenFarAway(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.
🔵 [LOW] src/main/resources/config.yml:1 The new salvavoid.raio-busca and salvavoid.preservar-xp keys have no entries in config.yml; they fall back to the hardcoded defaults in Settings.java (32 and true). Every other module documents its config keys in config.yml, so operators won't discover these knobs without reading source.

Unanchored Notes

  • 🔵 [LOW] src/main/resources/config.yml:1 — The new salvavoid.raio-busca and salvavoid.preservar-xp keys have no entries in config.yml; they fall back to the hardcoded defaults in Settings.java (32 and true). Every other module documents its config keys in config.yml, so operators won't discover these knobs without reading source.
    • Fix: Add a salvavoid: section to config.yml with raio-busca: 32 and preservar-xp: true and the pt-BR comments matching the rest of the file.
🔋 AI Usage & Run Details
  • Model / Engine: glm-5.2:cloud · opencode · 20 steps · 118.6s
  • Total Tokens: 906032 in / 6611 out (0 reasoning, cache 0 read / 0 write, 912643 total)
  • Est. cost on Claude Sonnet 5: $1.8782
  • Actual: $0.00 (headroom glm-5.2:cloud — free tier)
  • Scope: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is attributed (one model pass produces all findings; output split by each finding's body weight).
🤖 **AI Review** · pragent pilot · glm-5.2:cloud · `bc7a88b9` ### Summary of Changes - Adds `salvavoid` module: 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. - Extends `ChunkLoader` record with `name`, `enabled`, `expiresAt`; adds rename/pause/enable/time/fuel commands and right-click interaction (shift to toggle, fuel item to recharge). - Adds a per-tick mob-simulation task (`tickMobSimulation`) that forces spawner ranges to 2048 and spawns natural mobs into force-loaded chunks, plus a `CreatureSpawnEvent` listener pinning mobs so they don't despawn. - Adds persistence for name/enabled/expiresAt in chunks.yml and exposes `voidProtectionRadius`/`voidProtectionKeepXp` settings. ### Key Risks & Concerns - Breaks the repo's hard 'chat-only — never touch gameplay' constraint: the PR places/overwrites blocks, calls `World.spawnEntity`, mutates `CreatureSpawner` ranges, and forces chunk loading — all gameplay changes the architecture doc explicitly forbids. - `isVoidDeath` treats `y < minHeight` as 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. - `onVoidDeath` runs `findSafeChestLocation` (up to 120 synchronous `getHighestBlockYAt`/`getBlockAt` calls) and `rescueToChest` (block setType + inventory writes) on the main thread during `PlayerDeathEvent`, against the house rule to defer expensive scans off the main thread. - `simulateNaturalSpawning` runs every tick for every enabled loader and marks every spawned mob `setRemoveWhenFarAway(false)`, so entities accumulate indefinitely in force-loaded chunks with no cleanup, risking TPS loss and memory growth over time. - `rescueToChest` overwrites 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`). - New `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._ | Severity | Location | Finding | |---|---|---| | 🔴 [HIGH] | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:14` | `isVoidDeath` returns true whenever `y < minHeight` regardless 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 test `identifiesVoidDeathByCoordinatesBelowMinHeight` explicitly asserts `FALL`/`CUSTOM` at y=-65 return true, confirming this is the implemented (not accidental) behavior. | | 🔴 [HIGH] | `src/main/java/dev/marcospaulo/canalhandia/Canalhandia.java:860` | `onVoidDeath` calls `VoidProtection.findSafeChestLocation` (up to 120 synchronous `getHighestBlockYAt` + `getBlockAt` calls across loaded chunks) and `rescueToChest` (block `setType` + inventory writes) directly on the main thread inside the `PlayerDeathEvent` handler. 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. | | 🔴 [HIGH] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaders.java:414` | `simulateNaturalSpawning` runs every 20 ticks for every enabled loader and calls `setRemoveWhenFarAway(false)` on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunk `mobCount >= 20` guard 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. | | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/VoidProtection.java:173` | `rescueToChest` overwrites the `space` block (which `findSafeChestLocation` allows to be `SHORT_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. | | 🟡 [MEDIUM] | `src/test/java/dev/marcospaulo/canalhandia/VoidProtectionTest.java:84` | `VoidProtectionTest` only covers null/empty inputs and pure material predicates; `rescueToChest`'s actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. With `require_tests: true` and the rollback logic being the safety guarantee that the caller falls back to `keepInventory`, this is a real coverage gap for changed logic. | | 🟡 [MEDIUM] | `src/main/java/dev/marcospaulo/canalhandia/ChunkLoaderListener.java:298` | `onCreatureSpawn` marks every `Mob` spawned in a loader chunk `setRemoveWhenFarAway(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. | | 🔵 [LOW] | `src/main/resources/config.yml:1` | The new `salvavoid.raio-busca` and `salvavoid.preservar-xp` keys have no entries in `config.yml`; they fall back to the hardcoded defaults in `Settings.java` (32 and true). Every other module documents its config keys in `config.yml`, so operators won't discover these knobs without reading source. | ### Unanchored Notes - 🔵 [LOW] `src/main/resources/config.yml:1` — The new `salvavoid.raio-busca` and `salvavoid.preservar-xp` keys have no entries in `config.yml`; they fall back to the hardcoded defaults in `Settings.java` (32 and true). Every other module documents its config keys in `config.yml`, so operators won't discover these knobs without reading source. - **Fix:** Add a `salvavoid:` section to `config.yml` with `raio-busca: 32` and `preservar-xp: true` and the pt-BR comments matching the rest of the file. <details> <summary>🔋 AI Usage & Run Details</summary> - **Model / Engine**: `glm-5.2:cloud` · opencode · 20 steps · 118.6s - **Total Tokens**: 906032 in / 6611 out (0 reasoning, cache 0 read / 0 write, 912643 total) - **Est. cost on Claude Sonnet 5**: $1.8782 - **Actual**: $0.00 (headroom glm-5.2:cloud — free tier) - **Scope**: Whole-repo checkout at head sha (agent can read any file + run linters, not just the diff) — input tokens include files read beyond the diff. Per-comment output is *attributed* (one model pass produces all findings; output split by each finding's body weight). </details> <!-- pragent:sha=bc7a88b9e2bd904111e59d5f8a70cf99bc7875e3 -->
@@ -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] onVoidDeath calls VoidProtection.findSafeChestLocation (up to 120 synchronous getHighestBlockYAt + getBlockAt calls across loaded chunks) and rescueToChest (block setType + inventory writes) directly on the main thread inside the PlayerDeathEvent handler. 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)

🔴 [HIGH] `onVoidDeath` calls `VoidProtection.findSafeChestLocation` (up to 120 synchronous `getHighestBlockYAt` + `getBlockAt` calls across loaded chunks) and `rescueToChest` (block `setType` + inventory writes) directly on the main thread inside the `PlayerDeathEvent` handler. 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] onCreatureSpawn marks every Mob spawned in a loader chunk setRemoveWhenFarAway(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 setRemoveWhenFarAway when a loader is disabled rather than unconditionally on every spawn event.

🪙 ~863 tok (13% · attributed output)

🟡 [MEDIUM] `onCreatureSpawn` marks every `Mob` spawned in a loader chunk `setRemoveWhenFarAway(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 `setRemoveWhenFarAway` when 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] simulateNaturalSpawning runs every 20 ticks for every enabled loader and calls setRemoveWhenFarAway(false) on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunk mobCount >= 20 guard 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)

🔴 [HIGH] `simulateNaturalSpawning` runs every 20 ticks for every enabled loader and calls `setRemoveWhenFarAway(false)` on every spawned mob, so mobs in force-loaded chunks never despawn. With multiple anchors there is no cap across loaders (the per-chunk `mobCount >= 20` guard 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] isVoidDeath returns true whenever y < minHeight regardless 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 test identifiesVoidDeathByCoordinatesBelowMinHeight explicitly asserts FALL/CUSTOM at y=-65 return true, confirming this is the implemented (not accidental) behavior.

Fix: Only treat y < minHeight as a void death when the damage cause is VOID, 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).

public static boolean isVoidDeath(double y, double minHeight, EntityDamageEvent.DamageCause cause) {
        if (cause == EntityDamageEvent.DamageCause.VOID) {
            return true;
        }
        return false;
    }

🪙 ~1509 tok (23% · attributed output)

🔴 [HIGH] `isVoidDeath` returns true whenever `y < minHeight` regardless 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 test `identifiesVoidDeathByCoordinatesBelowMinHeight` explicitly asserts `FALL`/`CUSTOM` at y=-65 return true, confirming this is the implemented (not accidental) behavior. **Fix:** Only treat `y < minHeight` as a void death when the damage cause is `VOID`, 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). ```suggestion public static boolean isVoidDeath(double y, double minHeight, EntityDamageEvent.DamageCause cause) { if (cause == EntityDamageEvent.DamageCause.VOID) { return true; } return false; } ``` 🪙 ~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] rescueToChest overwrites the space block (which findSafeChestLocation allows to be SHORT_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 isReplaceableMaterial for the chest space to air variants only, or require the space block to be strictly air before placing a chest.

return mat == Material.AIR || mat == Material.CAVE_AIR || mat == Material.VOID_AIR;

🪙 ~826 tok (12% · attributed output)

🟡 [MEDIUM] `rescueToChest` overwrites the `space` block (which `findSafeChestLocation` allows to be `SHORT_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 `isReplaceableMaterial` for the chest space to air variants only, or require the space block to be strictly air before placing a chest. ```suggestion return mat == Material.AIR || mat == Material.CAVE_AIR || mat == Material.VOID_AIR; ``` 🪙 ~826 tok (12% · attributed output)

🟡 [MEDIUM] VoidProtectionTest only covers null/empty inputs and pure material predicates; rescueToChest's actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. With require_tests: true and the rollback logic being the safety guarantee that the caller falls back to keepInventory, 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)

🟡 [MEDIUM] `VoidProtectionTest` only covers null/empty inputs and pure material predicates; `rescueToChest`'s actual behavior — chest placement, double-chest overflow, and rollback on failure — is untested. With `require_tests: true` and the rollback logic being the safety guarantee that the caller falls back to `keepInventory`, 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)
masi closed this pull request 2026-08-20 18:13:19 +00:00

Pull request closed

Sign in to join this conversation.
No Reviewers
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gitea_admin/canalhandia#5