Wire command zone menu, harden counter command handling, and expand tests

- Connect aMoveToCommandZone to cmMoveToCommandZone; the menu item was
    added, shown, and shortcut-bound but never wired, so it did nothing
  - Extract evaluateModifyCounter() and route cmdIncCounter/cmdSetCounter
    through it, replacing isCommandZoneCounterBlocked; reject inc/set on
    an inactive tax counter so a hidden counter cannot accumulate value
  - Reject reserved tax counter names in cmdCreateCounter to prevent a
    client from spoofing a system tax counter via a user-created counter
  - Route AbstractCounter::valueChanged through the virtual setValue() so
    CommanderTaxCounter clamps and refreshes its tooltip on value changes
  - Deduplicate actPlayAndIncreaseTax/actPlayAndIncreasePartnerTax into a
    single playAndIncreaseTax(counterId) helper
  - Move MINIMUM_STACKING_HEIGHT from CommandZone to the PlayerGraphicsItem
    layout code, its only consumer
  - Remove unused AbstractCounter forward declaration from player_logic.h
  - Add EvaluateModifyCounter tests and PartnerTax cases for
    evaluateSetCounterActive
  - Pin the FirstUserId floor in new_counter_id_test using ids 3 and 5 so
    a naive "highest id + 1" regression fails the test

cockatrice/src/game/player/player_actions.cpp

@@ -1663,23 +1663,21 @@ void PlayerActions::playSelectedCardsImpl(QList<CardItem *> selectedCards,
 
 void PlayerActions::actPlayAndIncreaseTax(QList<CardItem *> selectedCards)
 {
-    playSelectedCardsImpl(selectedCards, false, [this](CardItem * /*card*/, const QString &originalZone) {
+    playAndIncreaseTax(selectedCards, CounterIds::CommanderTax);
-        if (originalZone == ZoneNames::COMMAND) {
-            CounterState *state = player->getCounters().value(CounterIds::CommanderTax, nullptr);
-            if (state && state->isActive()) {
-                sendIncCounter(CounterIds::CommanderTax, 1);
-            }
-        }
-    });
 }
 
 void PlayerActions::actPlayAndIncreasePartnerTax(QList<CardItem *> selectedCards)
 {
-    playSelectedCardsImpl(selectedCards, false, [this](CardItem * /*card*/, const QString &originalZone) {
+    playAndIncreaseTax(selectedCards, CounterIds::PartnerTax);
+}
+
+void PlayerActions::playAndIncreaseTax(QList<CardItem *> selectedCards, int counterId)
+{
+    playSelectedCardsImpl(selectedCards, false, [this, counterId](CardItem * /*card*/, const QString &originalZone) {
         if (originalZone == ZoneNames::COMMAND) {
-            CounterState *state = player->getCounters().value(CounterIds::PartnerTax, nullptr);
+            CounterState *state = player->getCounters().value(counterId, nullptr);
             if (state && state->isActive()) {
-                sendIncCounter(CounterIds::PartnerTax, 1);
+                sendIncCounter(counterId, 1);
             }
         }
     });

cockatrice/src/game/player/player_actions.h

@@ -267,6 +267,14 @@ private:
                                bool faceDown,
                                const std::function<void(CardItem *, const QString &)> &postPlayCallback = nullptr);
 
+    /**
+     * @brief Plays the selected cards and, for each that came from the command zone,
+     *        increments the given (active) tax counter by one.
+     * @param selectedCards Cards to play
+     * @param counterId The tax counter to increment (CounterIds::CommanderTax or PartnerTax)
+     */
+    void playAndIncreaseTax(QList<CardItem *> selectedCards, int counterId);
+
     void cmdSetTopCard(Command_MoveCard &cmd);
     void cmdSetBottomCard(Command_MoveCard &cmd);
 

cockatrice/src/game/player/player_logic.h

@@ -59,7 +59,6 @@ class ServerInfo_Counter;
 class ServerInfo_Player;
 class ServerInfo_User;
 class TabGame;
-class AbstractCounter;
 
 const int MAX_TOKENS_PER_DIALOG = 99;
 

cockatrice/src/game_graphics/board/abstract_counter.cpp

@@ -28,10 +28,9 @@ AbstractCounter::AbstractCounter(CounterState *state,
 {
     setAcceptHoverEvents(true);
 
-    connect(state, &CounterState::valueChanged, this, [this](int, int newValue) {
+    // Route through the (possibly overridden) virtual setValue so subclasses such as
-        value = newValue;
+    // CommanderTaxCounter can clamp and refresh their tooltip on every value change.
-        update();
+    connect(state, &CounterState::valueChanged, this, [this](int, int newValue) { setValue(newValue); });
-    });
 
     connect(state, &CounterState::activeChanged, this, [this](bool newActive) {
         setActive(newActive);

cockatrice/src/game_graphics/player/menu/move_menu.cpp

@@ -39,6 +39,7 @@ MoveMenu::MoveMenu(PlayerGraphicsItem *player) : QMenu(tr("Move to"))
     connect(aMoveToHand, &QAction::triggered, actions, invoke(cmMoveToHand));
     connect(aMoveToGraveyard, &QAction::triggered, actions, invoke(cmMoveToGraveyard));
     connect(aMoveToExile, &QAction::triggered, actions, invoke(cmMoveToExile));
+    connect(aMoveToCommandZone, &QAction::triggered, actions, invoke(cmMoveToCommandZone));
 
     addAction(aMoveToTopLibrary);
     addAction(aMoveToXfromTopOfLibrary);

cockatrice/src/game_graphics/player/player_graphics_item.cpp

@@ -18,6 +18,10 @@
 #include <QGraphicsView>
 #include <libcockatrice/utility/counter_ids.h>
 
+// Minimum height the stack zone keeps when the command zone shares its vertical space,
+// so the stack remains usable. Owned by this layout code, the sole consumer.
+static constexpr qreal MINIMUM_STACKING_HEIGHT = 50.0;
+
 PlayerGraphicsItem::PlayerGraphicsItem(PlayerLogic *_player) : player(_player)
 {
     connect(&SettingsCache::instance(), &SettingsCache::horizontalHandChanged, this,
@@ -278,8 +282,8 @@ void PlayerGraphicsItem::rearrangeZones()
     qreal stackHeight = tableHeight;
     if (commandZoneVisible) {
         stackHeight = tableHeight - totalCommandZoneHeight();
-        if (stackHeight < CommandZone::MINIMUM_STACKING_HEIGHT) {
+        if (stackHeight < MINIMUM_STACKING_HEIGHT) {
-            stackHeight = CommandZone::MINIMUM_STACKING_HEIGHT;
+            stackHeight = MINIMUM_STACKING_HEIGHT;
         }
     }
     stackZoneGraphicsItem->setHeight(stackHeight);

cockatrice/src/game_graphics/zones/command_zone.h

@@ -47,9 +47,6 @@ constexpr qreal COMMAND_ZONE_WIDTH = CardDimensions::WIDTH_F * 1.5;
 class CommandZone : public SelectZone
 {
     Q_OBJECT
-public:
-    static constexpr qreal MINIMUM_STACKING_HEIGHT = 50.0;
-
 private:
     static constexpr double MINIMIZED_HEIGHT_RATIO = 0.25;
     int zoneHeight;                       ///< Full height in pixels when expanded

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

@@ -441,10 +441,33 @@ Server_Player::cmdUndoDraw(const Command_UndoDraw & /*cmd*/, ResponseContainer &
     return retVal;
 }
 
-bool Server_Player::isCommandZoneCounterBlocked(int counterId) const
+Response::ResponseCode Server_Player::evaluateModifyCounter(bool gameStarted,
+                                                            bool playerConceded,
+                                                            bool commandZoneEnabled,
+                                                            int counterId,
+                                                            const Server_Counter *counter)
 {
-    return (counterId == CounterIds::CommanderTax || counterId == CounterIds::PartnerTax) &&
+    if (!gameStarted) {
-           !game->getEnableCommandZone();
+        return Response::RespGameNotStarted;
+    }
+    if (playerConceded) {
+        return Response::RespContextError;
+    }
+    if (CounterIds::isTaxCounter(counterId)) {
+        // Tax counters are server-managed: they only exist in Commander games, and an
+        // inactive (hidden) tax counter must stay at zero. Block modification in either
+        // case so the value can never diverge from what players can see.
+        if (!commandZoneEnabled) {
+            return Response::RespContextError;
+        }
+        if (counter && !counter->isActive()) {
+            return Response::RespContextError;
+        }
+    }
+    if (!counter) {
+        return Response::RespNameNotFound;
+    }
+    return Response::RespOk;
 }
 
 Response::ResponseCode
@@ -458,14 +481,12 @@ Server_Player::cmdIncCounter(const Command_IncCounter &cmd, ResponseContainer &
     }
 
     const int counterId = cmd.counter_id();
-
-    if (isCommandZoneCounterBlocked(counterId)) {
-        return Response::RespContextError;
-    }
-
     Server_Counter *c = counters.value(counterId, nullptr);
-    if (!c) {
+
-        return Response::RespNameNotFound;
+    const Response::ResponseCode authResult =
+        evaluateModifyCounter(game->getGameStarted(), conceded, game->getEnableCommandZone(), counterId, c);
+    if (authResult != Response::RespOk) {
+        return authResult;
     }
 
     bool didChange = c->incrementCount(cmd.delta());
@@ -489,8 +510,15 @@ Server_Player::cmdCreateCounter(const Command_CreateCounter &cmd, ResponseContai
         return Response::RespContextError;
     }
 
-    auto *c = new Server_Counter(newCounterId(), nameFromStdString(cmd.counter_name()), cmd.counter_color(),
+    const QString counterName = nameFromStdString(cmd.counter_name());
-                                 cmd.radius(), cmd.value());
+    // Reserved system counter names (commander/partner tax) are how clients identify
+    // server-managed tax counters for rendering and logging; a client must not be able
+    // to spoof one via a user-created counter.
+    if (CounterNames::isTaxCounter(counterName)) {
+        return Response::RespFunctionNotAllowed;
+    }
+
+    auto *c = new Server_Counter(newCounterId(), counterName, cmd.counter_color(), cmd.radius(), cmd.value());
     addCounter(c);
 
     Event_CreateCounter event;
@@ -516,14 +544,12 @@ Server_Player::cmdSetCounter(const Command_SetCounter &cmd, ResponseContainer &
     }
 
     const int counterId = cmd.counter_id();
-
-    if (isCommandZoneCounterBlocked(counterId)) {
-        return Response::RespContextError;
-    }
-
     Server_Counter *c = counters.value(counterId, nullptr);
-    if (!c) {
+
-        return Response::RespNameNotFound;
+    const Response::ResponseCode authResult =
+        evaluateModifyCounter(game->getGameStarted(), conceded, game->getEnableCommandZone(), counterId, c);
+    if (authResult != Response::RespOk) {
+        return authResult;
     }
 
     bool didChange = c->setCount(cmd.value());

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

@@ -9,7 +9,6 @@ class Server_Player : public Server_AbstractPlayer
 private:
     QMap<int, Server_Counter *> counters;
     QList<int> lastDrawList;
-    bool isCommandZoneCounterBlocked(int counterId) const;
 
 public:
     Server_Player(Server_Game *_game,
@@ -37,6 +36,14 @@ public:
                                                            int counterId,
                                                            const Server_Counter *counter,
                                                            bool requestedActive);
+    // Authorization shared by cmdIncCounter and cmdSetCounter. Reserved tax counters
+    // may only be modified inside a Commander game and only while active, so that an
+    // inactive (hidden) tax counter can never accumulate a value behind the scenes.
+    static Response::ResponseCode evaluateModifyCounter(bool gameStarted,
+                                                        bool playerConceded,
+                                                        bool commandZoneEnabled,
+                                                        int counterId,
+                                                        const Server_Counter *counter);
 
     void setupZones() override;
     void clearZones() override;

tests/command_zone_tests/counter_command_auth_test.cpp

@@ -1,7 +1,8 @@
 // Unit tests for the pure authorization logic extracted from the counter command
-// handlers. Server_Player::evaluateDelCounter() and evaluateSetCounterActive() are
+// handlers. Server_Player::evaluateDelCounter(), evaluateSetCounterActive() and
-// static and depend only on their arguments, so each guard branch can be exercised
+// evaluateModifyCounter() are static and depend only on their arguments, so each
-// directly without a started game, network stack, or player fixture.
+// guard branch can be exercised directly without a started game, network stack, or
+// player fixture.
 
 #include "game/server_counter.h"
 #include "game/server_player.h"
@@ -158,6 +159,89 @@ TEST(EvaluateSetCounterActive, AllowsDisablingWhenCounterIsZero)
               Response::RespOk);
 }
 
+// PartnerTax is the second reserved tax counter and must flow through the same auth
+// path as CommanderTax: enabling is always permitted...
+TEST(EvaluateSetCounterActive, AllowsEnablingPartnerTax)
+{
+    Server_Counter counter = makeCounter(CounterIds::PartnerTax, 0);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::PartnerTax, &counter,
+                                                      /*requestedActive=*/true),
+              Response::RespOk);
+}
+
+// ...and disabling is rejected while it still holds accumulated tax.
+TEST(EvaluateSetCounterActive, RejectsDisablingPartnerTaxWhenAccumulated)
+{
+    Server_Counter counter = makeCounter(CounterIds::PartnerTax, 2);
+    EXPECT_EQ(Server_Player::evaluateSetCounterActive(true, false, true, CounterIds::PartnerTax, &counter,
+                                                      /*requestedActive=*/false),
+              Response::RespContextError);
+}
+
+// ---------------------------------------------------------------------------
+// evaluateModifyCounter (shared by cmdIncCounter / cmdSetCounter)
+// ---------------------------------------------------------------------------
+
+// No counter may be modified before the game starts.
+TEST(EvaluateModifyCounter, RejectsWhenGameNotStarted)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(/*gameStarted=*/false, false, /*commandZoneEnabled=*/true,
+                                                   UserCounterId, &counter),
+              Response::RespGameNotStarted);
+}
+
+// A conceded player may no longer modify counters.
+TEST(EvaluateModifyCounter, RejectsWhenPlayerConceded)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, /*playerConceded=*/true, true, UserCounterId, &counter),
+              Response::RespContextError);
+}
+
+// An ordinary user counter can be modified regardless of command-zone state.
+TEST(EvaluateModifyCounter, AllowsUserCounter)
+{
+    Server_Counter counter = makeCounter(UserCounterId, 0);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/false, UserCounterId, &counter),
+              Response::RespOk);
+}
+
+// A non-existent counter is reported as not found.
+TEST(EvaluateModifyCounter, RejectsMissingCounter)
+{
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, true, UserCounterId, nullptr),
+              Response::RespNameNotFound);
+}
+
+// A tax counter may not be modified outside a Commander game (command zone disabled).
+TEST(EvaluateModifyCounter, RejectsTaxCounterWhenCommandZoneDisabled)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/false, CounterIds::CommanderTax,
+                                                   &counter),
+              Response::RespContextError);
+}
+
+// An inactive (hidden) tax counter must not accumulate value behind the scenes.
+TEST(EvaluateModifyCounter, RejectsInactiveTaxCounter)
+{
+    Server_Counter counter = makeCounter(CounterIds::PartnerTax, 0);
+    (void)counter.setActive(false);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/true, CounterIds::PartnerTax,
+                                                   &counter),
+              Response::RespContextError);
+}
+
+// An active tax counter in a Commander game may be modified.
+TEST(EvaluateModifyCounter, AllowsActiveTaxCounter)
+{
+    Server_Counter counter = makeCounter(CounterIds::CommanderTax, 0);
+    EXPECT_EQ(Server_Player::evaluateModifyCounter(true, false, /*commandZoneEnabled=*/true, CounterIds::CommanderTax,
+                                                   &counter),
+              Response::RespOk);
+}
+
 int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);

tests/command_zone_tests/new_counter_id_test.cpp

@@ -41,13 +41,15 @@ TEST(NewCounterId, ReturnsFirstUserIdWhenNoCounters)
     EXPECT_EQ(f.player.newCounterId(), CounterIds::FirstUserId);
 }
 
-// Reserved ids 0-9 (standard player counters and the commander tax counters) must
+// Reserved ids must not drag a new user counter down into the reserved range. The
-// not drag a new user counter down into the reserved range.
+// ids here (3 and 5) are chosen so a naive "highest id + 1" implementation would
+// return 6 (inside the reserved range) and fail this test; only the FirstUserId
+// floor yields the correct 10, so this pins the floor against regression.
 TEST(NewCounterId, SkipsReservedRangeWhenOnlyReservedCountersExist)
 {
     PlayerFixture f;
     f.player.addCounter(new Server_Counter(3, "g", color(), 20, 0));
-    f.player.addCounter(new Server_Counter(CounterIds::PartnerTax, CounterNames::PartnerTax, color(), 20, 0));
+    f.player.addCounter(new Server_Counter(5, "h", color(), 20, 0));
     EXPECT_EQ(f.player.newCounterId(), CounterIds::FirstUserId);
 }