Centralize MAX_COUNTERS_ON_CARD and enforce [0, 999] bounds on server

- Move MAX_COUNTERS_ON_CARD to trice_limits.h
  - Server clamps values in setCounter() and incrementCounter()
  - Client uses clamped comparison to allow recovery from invalid states
  - Add tests for clamping behavior

cockatrice/src/game/board/card_item.h

@@ -12,6 +12,7 @@
 #include "card_state.h"
 
 #include <libcockatrice/network/server/remote/game/server_card.h>
+#include <libcockatrice/utility/trice_limits.h>
 
 class CardDatabase;
 class CardDragItem;
@@ -21,7 +22,6 @@ class PlayerLogic;
 class QAction;
 class QColor;
 
-const int MAX_COUNTERS_ON_CARD = 999;
 const int ROTATION_DEGREES_PER_FRAME = 10;
 
 class CardItem : public AbstractCardItem

cockatrice/src/game/player/player_actions.cpp

@@ -1501,7 +1501,10 @@ void PlayerActions::offsetCardCounter(int counterId, int offset)
         int oldValue = card->getCounters().value(counterId, 0);
         int newValue = oldValue + offset;
 
-        if (newValue >= 0 && newValue <= MAX_COUNTERS_ON_CARD) {
+        // Early exit optimization: server enforces [0, MAX_COUNTERS_ON_CARD].
+        // Compare clamped value to allow recovery from invalid states.
+        int clampedValue = qBound(0, newValue, MAX_COUNTERS_ON_CARD);
+        if (clampedValue != oldValue) {
             auto *cmd = new Command_SetCardCounter;
             cmd->set_zone(card->getZone()->getName().toStdString());
             cmd->set_card_id(card->getId());
@@ -1541,7 +1544,9 @@ void PlayerActions::actSetCardCounter(int counterId)
     for (auto card : sel) {
         int oldValue = card->getCounters().value(counterId, 0);
         Expression exp(oldValue);
-        int number = static_cast<int>(exp.parse(dialog.textValue()));
+        double parsed = exp.parse(dialog.textValue());
+        // Clamp in double precision first to avoid UB, then cast
+        int number = static_cast<int>(qBound(0.0, parsed, static_cast<double>(MAX_COUNTERS_ON_CARD)));
 
         auto *cmd = new Command_SetCardCounter;
         cmd->set_zone(card->getZone()->getName().toStdString());

libcockatrice_network/libcockatrice/network/server/remote/game/server_card.cpp

@@ -26,6 +26,7 @@
 #include <libcockatrice/protocol/pb/event_set_card_attr.pb.h>
 #include <libcockatrice/protocol/pb/event_set_card_counter.pb.h>
 #include <libcockatrice/protocol/pb/serverinfo_card.pb.h>
+#include <libcockatrice/utility/trice_limits.h>
 #include <limits>
 
 Server_Card::Server_Card(const CardRef &cardRef, int _id, int _coord_x, int _coord_y, Server_CardZone *_zone)
@@ -113,6 +114,9 @@ QString Server_Card::setAttribute(CardAttribute attribute, const QString &avalue
 
 bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event)
 {
+    // Clamp to valid card counter range [0, MAX_COUNTERS_ON_CARD]
+    value = qBound(0, value, MAX_COUNTERS_ON_CARD);
+
     const int oldValue = counters.value(_id, 0);
     if (value == oldValue) {
         return false;
@@ -134,12 +138,11 @@ bool Server_Card::setCounter(int _id, int value, Event_SetCardCounter *event)
 
 bool Server_Card::incrementCounter(int counterId, int delta, Event_SetCardCounter *event)
 {
-    // TODO: Extract overflow-safe arithmetic into shared helper.
-    // Duplicated in Server_Counter::incrementCount() - keep in sync if modified.
     const int oldValue = counters.value(counterId, 0);
     const auto result = static_cast<int64_t>(oldValue) + static_cast<int64_t>(delta);
-    const int newValue = static_cast<int>(qBound(static_cast<int64_t>(std::numeric_limits<int>::min()), result,
-                                                 static_cast<int64_t>(std::numeric_limits<int>::max())));
+    // Clamp to [0, MAX_COUNTERS_ON_CARD] for card counters
+    const int newValue =
+        static_cast<int>(qBound(static_cast<int64_t>(0), result, static_cast<int64_t>(MAX_COUNTERS_ON_CARD)));
 
     if (newValue == oldValue) {
         return false;

libcockatrice_network/libcockatrice/network/server/remote/game/server_card.h

@@ -154,9 +154,9 @@ public:
         cardRef = _cardRef;
     }
     /**
-     * @brief Sets a card counter to an exact value.
+     * @brief Sets a card counter to an exact value with clamping.
      * @param _id The counter ID.
-     * @param value The new value (0 removes the counter).
+     * @param value The desired value (clamped to [0, MAX_COUNTERS_ON_CARD]; 0 removes the counter).
      * @param event Optional event to populate with counter state.
      * @return true if the value changed, false otherwise.
      */
@@ -168,7 +168,7 @@ public:
      * @param event Optional event to populate with counter state.
      * @return true if the value changed, false otherwise.
      * @note If counter does not exist, starts from 0. Counter is removed if result is 0.
-     * @warning For direct assignment without clamping, use setCounter().
+     * @note Clamps result to [0, MAX_COUNTERS_ON_CARD].
      */
     [[nodiscard]] bool incrementCounter(int counterId, int delta, Event_SetCardCounter *event = nullptr);
     void setTapped(bool _tapped)

libcockatrice_utility/libcockatrice/utility/trice_limits.h

@@ -15,6 +15,12 @@ constexpr uint MAXIMUM_DIE_SIDES = 1000000;
 constexpr uint MINIMUM_DICE_TO_ROLL = 1;
 constexpr uint MAXIMUM_DICE_TO_ROLL = 100;
 
+// Card counter value bounds [0, MAX_COUNTERS_ON_CARD].
+// Counters on cards (e.g., +1/+1 counters, charge counters) are non-negative physical game objects.
+// The max of 999 is a display constraint (3-digit rendering) and reasonable gameplay limit.
+// Server enforces these bounds; client may also check for UX optimization.
+constexpr int MAX_COUNTERS_ON_CARD = 999;
+
 // optimized functions to get qstrings that are at most that long
 static inline QString nameFromStdString(const std::string &_string)
 {

tests/server_card_counter_test.cpp

@@ -7,6 +7,7 @@
 #include <libcockatrice/network/server/remote/game/server_card.h>
 #include <libcockatrice/protocol/pb/event_set_card_counter.pb.h>
 #include <libcockatrice/utility/card_ref.h>
+#include <libcockatrice/utility/trice_limits.h>
 #include <limits>
 
 TEST(ServerCardCounter, IncrementNewCounter)
@@ -27,17 +28,18 @@ TEST(ServerCardCounter, IncrementExistingCounter)
 TEST(ServerCardCounter, IncrementOverflowProtection)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, std::numeric_limits<int>::max()));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD));
     EXPECT_FALSE(card.incrementCounter(1, 1));
-    EXPECT_EQ(card.getCounter(1), std::numeric_limits<int>::max());
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
 }
 
 TEST(ServerCardCounter, DecrementUnderflowProtection)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, std::numeric_limits<int>::min()));
-    EXPECT_FALSE(card.incrementCounter(1, -1));
-    EXPECT_EQ(card.getCounter(1), std::numeric_limits<int>::min());
+    ASSERT_TRUE(card.setCounter(1, 5));
+    EXPECT_TRUE(card.incrementCounter(1, -10));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
 }
 
 TEST(ServerCardCounter, ReturnsFalseWhenUnchanged)
@@ -111,13 +113,13 @@ TEST(ServerCardCounter, IncrementCounterPopulatesEvent)
 TEST(ServerCardCounter, IncrementCounterEventReflectsClampedValue)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, std::numeric_limits<int>::max() - 5));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5));
 
     Event_SetCardCounter event;
     EXPECT_TRUE(card.incrementCounter(1, 10, &event));
 
     EXPECT_EQ(event.counter_id(), 1);
-    EXPECT_EQ(event.counter_value(), std::numeric_limits<int>::max());
+    EXPECT_EQ(event.counter_value(), MAX_COUNTERS_ON_CARD);
 }
 
 TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr)
@@ -131,7 +133,7 @@ TEST(ServerCardCounter, IncrementCounterNoEventWhenNullptr)
 TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged)
 {
     Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
-    ASSERT_TRUE(card.setCounter(1, std::numeric_limits<int>::max()));
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD));
 
     Event_SetCardCounter event;
     event.set_counter_id(999);
@@ -142,6 +144,38 @@ TEST(ServerCardCounter, IncrementCounterEventNotPopulatedWhenUnchanged)
     EXPECT_EQ(event.counter_value(), 999);
 }
 
+TEST(ServerCardCounter, SetCounterClampsNegativeToZero)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    EXPECT_FALSE(card.setCounter(1, -5));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
+}
+
+TEST(ServerCardCounter, SetCounterClampsAboveMaxToMax)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    EXPECT_TRUE(card.setCounter(1, 1500));
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+}
+
+TEST(ServerCardCounter, IncrementDoesNotGoBelowZero)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    ASSERT_TRUE(card.setCounter(1, 5));
+    EXPECT_TRUE(card.incrementCounter(1, -10));
+    EXPECT_EQ(card.getCounter(1), 0);
+    EXPECT_FALSE(card.getCounters().contains(1));
+}
+
+TEST(ServerCardCounter, IncrementDoesNotExceedMax)
+{
+    Server_Card card(CardRef{"TestCard", ""}, 1, 0, 0);
+    ASSERT_TRUE(card.setCounter(1, MAX_COUNTERS_ON_CARD - 5));
+    EXPECT_TRUE(card.incrementCounter(1, 10));
+    EXPECT_EQ(card.getCounter(1), MAX_COUNTERS_ON_CARD);
+}
+
 int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);