test: enforce full line coverage and fix unsafe market trades

.github/workflows/build.yml

@@ -74,3 +74,11 @@ jobs:
           name: unit-test-reports-${{ github.run_id }}-${{ github.run_attempt }}
           path: target/surefire-reports/
           if-no-files-found: error
+
+      - name: Upload coverage report
+        if: ${{ !cancelled() && hashFiles('target/site/jacoco/jacoco.xml') != '' }}
+        uses: actions/upload-artifact@v7
+        with:
+          name: coverage-report-${{ github.run_id }}-${{ github.run_attempt }}
+          path: target/site/jacoco/
+          if-no-files-found: error

README.md

@@ -35,7 +35,11 @@ mvn clean verify
 
 Point the installer at your TLibs checkout.
 
-Tests use JUnit and run without a live Minecraft server.
+Tests use JUnit, MockBukkit, and Mockito without a live Minecraft server. They
+cover trade creation, purchases, menus, persistence, demand recovery, and the
+plugin lifecycle, including regressions for invalid quantities and stale state.
+`mvn clean verify` enforces 100% runtime line coverage with no production-code
+exclusions. JaCoCo's HTML and XML reports are written to `target/site/jacoco/`.
 
 ## License
 

pom.xml

@@ -12,12 +12,37 @@
         <maven.compiler.release>21</maven.compiler.release>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <maven.compiler.proc>full</maven.compiler.proc>
+        <argLine></argLine>
         <cooking.version>0.2.1</cooking.version>
         <denareconomy.version>0.2.0</denareconomy.version>
         <tlibs.version>2.0.0</tlibs.version>
     </properties>
 
+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>org.junit</groupId>
+                <artifactId>junit-bom</artifactId>
+                <version>5.14.0</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
+
     <dependencies>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>5.23.0</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.mockbukkit.mockbukkit</groupId>
+            <artifactId>mockbukkit-v1.21</artifactId>
+            <version>4.95.0</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>me.plugins</groupId>
             <artifactId>tlibs</artifactId>
@@ -94,6 +119,46 @@
             </resource>
         </resources>
         <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-dependency-plugin</artifactId>
+                <version>3.7.0</version>
+                <executions>
+                    <execution><goals><goal>properties</goal></goals></execution>
+                </executions>
+            </plugin>
+            <plugin>
+                <groupId>org.jacoco</groupId>
+                <artifactId>jacoco-maven-plugin</artifactId>
+                <version>0.8.15</version>
+                <executions>
+                    <execution><goals><goal>prepare-agent</goal></goals></execution>
+                    <execution>
+                        <id>coverage-report</id>
+                        <phase>verify</phase>
+                        <goals><goal>report</goal></goals>
+                    </execution>
+                    <execution>
+                        <id>coverage-check</id>
+                        <phase>verify</phase>
+                        <goals><goal>check</goal></goals>
+                        <configuration>
+                            <rules>
+                                <rule>
+                                    <element>BUNDLE</element>
+                                    <limits>
+                                        <limit>
+                                            <counter>LINE</counter>
+                                            <value>MISSEDCOUNT</value>
+                                            <maximum>0</maximum>
+                                        </limit>
+                                    </limits>
+                                </rule>
+                            </rules>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>
@@ -103,6 +168,10 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-surefire-plugin</artifactId>
                 <version>3.5.4</version>
+                <configuration>
+                    <argLine>@{argLine} -javaagent:${org.mockito:mockito-core:jar} -Xshare:off</argLine>
+                    <failIfNoTests>true</failIfNoTests>
+                </configuration>
             </plugin>
         </plugins>
     </build>

src/main/java/net/tfminecraft/marketblock/Cache.java

@@ -3,6 +3,7 @@
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 
 import org.bukkit.block.Block;
@@ -24,7 +25,7 @@ public static double freshnessMultiplier(String stepId) {
         if (stepId == null || stepId.isBlank()) {
             return 1.0;
         }
-        Double value = freshnessPrice.get(stepId.toLowerCase());
+        Double value = freshnessPrice.get(stepId.toLowerCase(Locale.ROOT));
         return value == null ? 1.0 : value;
     }
 

src/main/java/net/tfminecraft/marketblock/inventory/CategoryView.java

@@ -29,6 +29,7 @@ public void categoryView(Inventory inv, Player p) {
             inv = MarketBlock.plugin.getServer().createInventory(new MBHolder(MBGUI.CATEGORY, "none"), 54, "§7Market Categories");
             open = true;
         }
+        inv.clear();
         List<Category> categories = CategoryLoader.getAsList();
 		for(int i = 0; i < Cache.slots.size() && i < categories.size(); i++) {
             int slot = Cache.slots.get(i);

src/main/java/net/tfminecraft/marketblock/inventory/TradeView.java

@@ -32,6 +32,7 @@ public void tradeView(Inventory inv, Player p, Category cat) {
             inv = MarketBlock.plugin.getServer().createInventory(new MBHolder(MBGUI.TRADE, cat.getId()), 54, cat.getTradesTitle());
             open = true;
         }
+        inv.clear();
         List<Trade> trades = new ArrayList<>(cat.getTrades());
         trades.sort(Comparator.comparingInt(Trade::getGroup));
 		for(int i = 0; i < Cache.slots.size() && i < trades.size(); i++) {
@@ -58,12 +59,10 @@ public void tradeView(Inventory inv, Player p, Category cat) {
     @SuppressWarnings("deprecation")
     private ItemStack createTradeItem(Trade trade, Category cat) {
         ItemStack i = trade.getIconItem();
-        if (i == null || i.getItemMeta() == null) {
-            i = new ItemStack(Material.GRAY_DYE, 1);
-        }
-        ItemMeta m = i.getItemMeta();
+        ItemMeta m = i == null ? null : i.getItemMeta();
         if (m == null) {
-            return i;
+            i = new ItemStack(Material.GRAY_DYE, 1);
+            m = i.getItemMeta();
         }
         m.setDisplayName(StringFormatter.formatHex(cat.getColorHex() + getPlainName(i) + "#d6d3a7x#b6e66e" + trade.getAmount()));
         List<String> lore = new ArrayList<>();

src/main/java/net/tfminecraft/marketblock/manager/TradeManager.java

@@ -12,6 +12,7 @@
 import org.bukkit.event.player.PlayerInteractEvent;
 import org.bukkit.inventory.Inventory;
 import org.bukkit.inventory.ItemStack;
+import org.bukkit.inventory.meta.ItemMeta;
 import org.bukkit.persistence.PersistentDataType;
 import org.bukkit.scheduler.BukkitRunnable;
 import org.bukkit.scheduler.BukkitTask;
@@ -48,19 +49,13 @@ public void update() {
     }
 
     private String getItemId(MBGUI type, ItemStack i) {
-        NamespacedKey key;
-        switch (type) {
-            case CATEGORY:
-                key = new NamespacedKey(MarketBlock.plugin, "category_id");
-                break;
-            case TRADE:
-                key = new NamespacedKey(MarketBlock.plugin, "trade_id");
-                break;
-            default:
-                key = new NamespacedKey(MarketBlock.plugin, "none");
-                break;
-        }
-        return i.getItemMeta().getPersistentDataContainer().get(key, PersistentDataType.STRING);
+        ItemMeta meta = i.getItemMeta();
+        if (meta == null) return null;
+        NamespacedKey key = new NamespacedKey(MarketBlock.plugin, switch (type) {
+            case CATEGORY -> "category_id";
+            case TRADE -> "trade_id";
+        });
+        return meta.getPersistentDataContainer().get(key, PersistentDataType.STRING);
     }
 
     public void start() {
@@ -104,6 +99,7 @@ public void openMarket(PlayerInteractEvent e) {
     public void click(InventoryClickEvent e) {
         if(!(e.getView().getTopInventory().getHolder() instanceof MBHolder)) return;
         e.setCancelled(true);
+        if(e.getClickedInventory() != e.getView().getTopInventory()) return;
         Player p = (Player) e.getWhoClicked();
         MBHolder h = (MBHolder) e.getView().getTopInventory().getHolder();
         ItemStack i = e.getCurrentItem();
@@ -152,21 +148,22 @@ public void tradeClick(Player p, MBHolder h, ItemStack i, InventoryClickEvent e)
             return;
         }
         SaleTake take = InventoryUtils.removeItems(p, tradePath, requiredAmount);
+        if (take.total() == 0) {
+            p.sendMessage("§cThe market could not take the items for this trade.");
+            return;
+        }
         p.playSound(p.getLocation(), Sound.ENTITY_EXPERIENCE_ORB_PICKUP, 1f, 1f);
         double base = PriceCalculator.calculatePrice(trade);
         double price = applyFreshness(base, take);
         DenarEconomy.getMoneyManager().addMoney(p, price, false, true);
-        sendSaleBreakdown(p, take, requiredAmount, base, price);
+        sendSaleBreakdown(p, take, (int) requiredAmount, base, price);
         trade.sell();
         Bukkit.getPluginManager().callEvent(new MarketSaleEvent(p, trade, price, requiredAmount));
         update();
     }
 
     private static double applyFreshness(double base, SaleTake take) {
         int taken = take.total();
-        if (taken <= 0) {
-            return Math.max(0.01, base);
-        }
         double weight = 0;
         for (var entry : take.getCounts().entrySet()) {
             weight += entry.getValue() * Cache.freshnessMultiplier(entry.getKey());
@@ -176,8 +173,8 @@ private static double applyFreshness(double base, SaleTake take) {
         return Math.max(0.01, price);
     }
 
-    private static void sendSaleBreakdown(Player p, SaleTake take, double amount, double base, double price) {
-        p.sendMessage("§aSold " + formatAmount(amount) + " for " + formatMoney(price) + "d");
+    private static void sendSaleBreakdown(Player p, SaleTake take, int amount, double base, double price) {
+        p.sendMessage("§aSold " + amount + " for " + formatMoney(price) + "d");
         if (take.allFresh()) {
             return;
         }
@@ -198,13 +195,6 @@ private static void sendSaleBreakdown(Player p, SaleTake take, double amount, do
         }
     }
 
-    private static String formatAmount(double amount) {
-        if (amount == Math.rint(amount)) {
-            return String.valueOf((long) amount);
-        }
-        return String.valueOf(amount);
-    }
-
     private static String formatMoney(double value) {
         if (value == Math.rint(value)) {
             return String.valueOf((long) value);

src/main/java/net/tfminecraft/marketblock/manager/commands/ChatListener.java

@@ -57,12 +57,18 @@ void answerIfCurrent(AsyncPlayerChatEvent event, Player player, MarketblockConve
     private void answer(Player player, MarketblockConversation convo, String message) {
         switch (convo.getStep()) {
             case 0 -> {
-                if (TradeLoader.getTradeById(message) != null) {
+                // IDs are one YAML path segment and one /marketblock command argument.
+                String id = message.strip();
+                if (id.isEmpty() || id.contains(".") || id.chars().anyMatch(Character::isWhitespace)) {
+                    player.sendMessage("§cInvalid ID. Use a nonempty ID without dots or spaces.");
+                    return;
+                }
+                if (TradeLoader.getTradeById(id) != null) {
                     player.sendMessage("§cThat id is already taken");
                     return;
                 }
-                convo.setId(message);
-                player.sendMessage("§aID set to: " + message);
+                convo.setId(id);
+                player.sendMessage("§aID set to: " + id);
                 convo.nextStep();
                 player.sendMessage("§aEnter the demand limit:");
             }
@@ -113,15 +119,9 @@ private void answer(Player player, MarketblockConversation convo, String message
             case 5 -> {
                 // Done! Claim the conversation first, so a quit that got there first stops the save.
                 if (!ConversationManager.finishConversation(player, convo)) return;
-                Category cat = CategoryLoader.getByString(message);
-                if (cat == null || cat.getId().equalsIgnoreCase("unknown")) {
-                    player.sendMessage("§cWarning, no category found, default selected");
-                }
-                convo.setCategory(cat);
-                player.sendMessage("§aCategory set to: " + cat.getId());
                 // Chat arrives on an async thread; trades and trades.yml belong to the main thread.
                 Bukkit.getScheduler().runTask(MarketBlock.plugin, () -> {
-                    if (!saveTrade(player, convo)) return;
+                    if (!saveTrade(player, convo, message)) return;
                     player.sendMessage("§aTrade successfully created!");
                 });
             }
@@ -133,7 +133,12 @@ public void onQuit(PlayerQuitEvent event) {
         ConversationManager.endConversation(event.getPlayer());
     }
 
-    private boolean saveTrade(Player p, MarketblockConversation convo) {
+    private boolean saveTrade(Player p, MarketblockConversation convo, String categoryId) {
+        // The final answer and this queued task may straddle a permission change.
+        if (!p.hasPermission(CommandManager.ADMIN_PERMISSION)) {
+            p.sendMessage("§cYou do not have permission to create a trade.");
+            return false;
+        }
         String path = TLibs.getItemAPI().getChecker().getAsStringPath(convo.getItem());
         if(path == null) {
             p.sendMessage("§cCould not parse the item you are holding");
@@ -143,6 +148,17 @@ private boolean saveTrade(Player p, MarketblockConversation convo) {
             p.sendMessage("§cA trade already exists with that id");
             return false;
         }
+        // Resolve after scheduling so a category reload cannot leave a trade on an obsolete object.
+        Category cat = CategoryLoader.getByString(categoryId);
+        if (cat == null) {
+            cat = new Category();
+            CategoryLoader.categories.put(cat.getId(), cat);
+        }
+        if (cat.getId().equalsIgnoreCase("unknown")) {
+            p.sendMessage("§cWarning, no category found, default selected");
+        }
+        convo.setCategory(cat);
+        p.sendMessage("§aCategory set to: " + cat.getId());
         TradeLoader.add(new Trade(convo));
         return true;
     }

src/main/java/net/tfminecraft/marketblock/manager/commands/MarketblockConversation.java

@@ -22,7 +22,7 @@ public class MarketblockConversation {
 
     public MarketblockConversation(Player player, ItemStack item) {
         this.player = player;
-        this.item = item;
+        this.item = item == null ? null : item.clone();
     }
 
     public Category getCategory() { return category; }
@@ -58,4 +58,3 @@ public double getRestingPrice() {
         return restingPrice;
     }
 }
-

src/main/java/net/tfminecraft/marketblock/trade/Trade.java

@@ -37,7 +37,7 @@ public Trade(MarketblockConversation convo) {
         this.category = convo.getCategory();
         category.addTrade(this);
         this.demandLimit = Math.max(1, convo.getDemandLimit());
-        this.demand = this.demandLimit / 2;
+        this.demand = clampDemand(this.demandLimit / 2);
         this.item = TLibs.getItemAPI().getChecker().getAsStringPath(convo.getItem());
         this.itemRestingPrice = convo.getRestingPrice();
         this.priceChange = convo.getPriceChange();
@@ -46,7 +46,7 @@ public Trade(MarketblockConversation convo) {
     }
 
     public void resetDemand() {
-        demand = demandLimit / 2;
+        demand = clampDemand(demandLimit / 2);
     }
 
     public void setDemand(double demand) {

src/main/java/net/tfminecraft/marketblock/util/FreshnessLookup.java

@@ -1,5 +1,7 @@
 package net.tfminecraft.marketblock.util;
 
+import java.util.Locale;
+
 import org.bukkit.Bukkit;
 import org.bukkit.inventory.ItemStack;
 
@@ -25,6 +27,6 @@ public static String stepId(ItemStack item) {
             return "fresh";
         }
         String id = track.getCurrentStep().getId();
-        return id == null || id.isBlank() ? "fresh" : id.toLowerCase();
+        return id == null || id.isBlank() ? "fresh" : id.toLowerCase(Locale.ROOT);
     }
 }