fix(coop): host re-validate craft capacity in SHARED (#121)

src/Basescape/CraftWeaponsState.cpp

@@ -171,45 +171,49 @@ void CraftWeaponsState::lstWeaponsClick(Action *)
 
 	const RuleCraftWeapon* refWeapon = _weapons[_lstWeapons->getSelectedRow()];
 	const RuleCraftWeapon* currWeapon = current ? current->getRules() : nullptr;
+
+	std::string capError = equipCapacityError(_game->getMod(), _craft, refWeapon, currWeapon);
+	if (!capError.empty())
+	{
+		_game->popState();
+		_game->pushState(new ErrorMessageState(
+			tr(capError),
+			_palette,
+			_game->getMod()->getInterface("craftWeapons")->getElement("errorMessage")->color,
+			"BACK14.SCR",
+			_game->getMod()->getInterface("craftWeapons")->getElement("errorPalette")->color)
+		);
+		return;
+	}
+
+	equipSelectedWeapon(_weapons[_lstWeapons->getSelectedRow()]);
+	_game->popState();
+}
+
+/**
+ * Pure capacity gate shared by the UI (lstWeaponsClick) and the SHARED host validator
+ * (issue #121: craftRearmValidate). Replicates vanilla's four craft-capacity checks for
+ * mounting @a refWeapon (nullptr = dismount) in place of @a currWeapon (nullptr = empty
+ * slot), from the weapons' getBonusStats() deltas. Returns "" if the change is within
+ * every limit, else the STR_ error id vanilla would show. The math is byte-identical to
+ * the old inline block, so host and client always agree.
+ */
+std::string CraftWeaponsState::equipCapacityError(Mod *mod, Craft *craft,
+	const RuleCraftWeapon *refWeapon, const RuleCraftWeapon *currWeapon)
+{
 	{
 		int refCapBonus1 = refWeapon ? refWeapon->getBonusStats().soldiers : 0;
 		int currCapBonus1 = currWeapon ? currWeapon->getBonusStats().soldiers : 0;
 		int diff1 = (refCapBonus1 - currCapBonus1);
-		if (diff1)
-		{
-			if ((_craft->getMaxUnitsRaw() - _craft->getSpaceUsed() + diff1) < 0)
-			{
-				_game->popState();
-				_game->pushState(new ErrorMessageState(
-					tr("STR_NOT_ENOUGH_CARGO_SPACE"),
-					_palette,
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorMessage")->color,
-					"BACK14.SCR",
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorPalette")->color)
-				);
-				return;
-			}
-		}
+		if (diff1 && (craft->getMaxUnitsRaw() - craft->getSpaceUsed() + diff1) < 0)
+			return "STR_NOT_ENOUGH_CARGO_SPACE";
 	}
 	{
 		int refCapBonus2 = refWeapon ? refWeapon->getBonusStats().vehicles : 0;
 		int currCapBonus2 = currWeapon ? currWeapon->getBonusStats().vehicles : 0;
 		int diff2 = (refCapBonus2 - currCapBonus2);
-		if (diff2)
-		{
-			if ((_craft->getMaxVehiclesAndLargeSoldiersRaw() - _craft->getNumVehiclesAndLargeSoldiers() + diff2) < 0)
-			{
-				_game->popState();
-				_game->pushState(new ErrorMessageState(
-					tr("STR_NOT_ENOUGH_HWP_CAPACITY"),
-					_palette,
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorMessage")->color,
-					"BACK14.SCR",
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorPalette")->color)
-				);
-				return;
-			}
-		}
+		if (diff2 && (craft->getMaxVehiclesAndLargeSoldiersRaw() - craft->getNumVehiclesAndLargeSoldiers() + diff2) < 0)
+			return "STR_NOT_ENOUGH_HWP_CAPACITY";
 	}
 	{
 		int refCapBonus3 = refWeapon ? refWeapon->getBonusStats().maxItems : 0;
@@ -225,58 +229,25 @@ void CraftWeaponsState::lstWeaponsClick(Action *)
 		double totalItemStorageSize = 0.0;
 		if (diff3 || diff4_b)
 		{
-			for (auto& itemType : _game->getMod()->getItemsList())
+			for (auto& itemType : mod->getItemsList())
 			{
-				RuleItem* rule = _game->getMod()->getItem(itemType);
+				RuleItem* rule = mod->getItem(itemType);
 
 				Unit* isVehicle = rule->getVehicleUnit();
-				int cQty = 0;
-				if (isVehicle)
+				if (!isVehicle)
 				{
-					cQty = _craft->getVehicleCount(itemType);
-				}
-				else
-				{
-					cQty = _craft->getItems()->getItem(rule);
+					int cQty = craft->getItems()->getItem(rule);
 					totalItems += cQty;
 					totalItemStorageSize += cQty * rule->getSize();
 				}
 			}
 		}
-		if (diff3)
-		{
-			if ((_craft->getMaxItemsRaw() - totalItems + diff3) < 0)
-			{
-				_game->popState();
-				_game->pushState(new ErrorMessageState(
-					tr("STR_NOT_ENOUGH_STORAGE_SPACE_1"),
-					_palette,
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorMessage")->color,
-					"BACK14.SCR",
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorPalette")->color)
-				);
-				return;
-			}
-		}
-		if (diff4_b)
-		{
-			if ((_craft->getMaxStorageSpaceRaw() - totalItemStorageSize + diff4) < 0.0)
-			{
-				_game->popState();
-				_game->pushState(new ErrorMessageState(
-					tr("STR_NOT_ENOUGH_STORAGE_SPACE_2"),
-					_palette,
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorMessage")->color,
-					"BACK14.SCR",
-					_game->getMod()->getInterface("craftWeapons")->getElement("errorPalette")->color)
-				);
-				return;
-			}
-		}
+		if (diff3 && (craft->getMaxItemsRaw() - totalItems + diff3) < 0)
+			return "STR_NOT_ENOUGH_STORAGE_SPACE_1";
+		if (diff4_b && (craft->getMaxStorageSpaceRaw() - totalItemStorageSize + diff4) < 0.0)
+			return "STR_NOT_ENOUGH_STORAGE_SPACE_2";
 	}
-
-	equipSelectedWeapon(_weapons[_lstWeapons->getSelectedRow()]);
-	_game->popState();
+	return "";
 }
 
 /**
@@ -329,13 +300,21 @@ void CraftWeaponsState::equipSelectedWeapon(RuleCraftWeapon* selRule)
  * "None"/dismount row) via the SAME store path a list click drives. Returns false
  * if that weapon is not on this screen's list. Does not pop the state.
  */
-bool CraftWeaponsState::harnessEquip(const std::string& weaponType)
+bool CraftWeaponsState::harnessEquip(const std::string& weaponType, bool bypassGate, std::string& blockedBy)
 {
+	blockedBy.clear();
 	for (size_t i = 0; i < _weapons.size(); ++i)
 	{
 		std::string t = _weapons[i] ? _weapons[i]->getType() : "";
 		if (t == weaponType)
 		{
+			if (!bypassGate)
+			{
+				CraftWeapon* current = _craft->getWeapons()->at(_weapon);
+				const RuleCraftWeapon* currWeapon = current ? current->getRules() : nullptr;
+				blockedBy = equipCapacityError(_game->getMod(), _craft, _weapons[i], currWeapon);
+				if (!blockedBy.empty()) return true; // found, but the client gate blocked it
+			}
 			equipSelectedWeapon(_weapons[i]);
 			return true;
 		}

src/Basescape/CraftWeaponsState.h

@@ -25,6 +25,7 @@ namespace OpenXcom
 
 class Base;
 class Craft;
+class Mod;
 class TextButton;
 class Window;
 class Text;
@@ -61,8 +62,19 @@ class CraftWeaponsState : public State
 	void lstWeaponsClick(Action *action);
 	/// Handler for middle clicking the Weapons list.
 	void lstWeaponsMiddleClick(Action *action);
+	/// Pure capacity gate, shared by the UI click handler and the SHARED host validator
+	/// (issue #121): returns "" if mounting @a refWeapon (nullptr = dismount) in place of
+	/// @a currWeapon (nullptr = empty slot) keeps @a craft within all four capacity limits,
+	/// else the STR_ error id the client shows. Host and client run identical math so the
+	/// host can re-check a request whose (possibly stale) replica gate already passed.
+	static std::string equipCapacityError(Mod *mod, Craft *craft,
+		const RuleCraftWeapon *refWeapon, const RuleCraftWeapon *currWeapon);
 	/// Harness (PRD-J09 GAP-5b): drive the real weapon-mount store path for one type.
-	bool harnessEquip(const std::string& weaponType);
+	/// Returns false only when @a weaponType is not a mountable armament here. With
+	/// @a bypassGate false the client capacity gate runs first and, if it blocks, fills
+	/// @a blockedBy with its STR_ error and leaves the mount unmade; bypassGate models a
+	/// stale replica whose gate passed, letting the request reach the host un-gated.
+	bool harnessEquip(const std::string& weaponType, bool bypassGate, std::string& blockedBy);
 };
 
 }

src/Basescape/SoldierArmorState.cpp

@@ -348,14 +348,25 @@ void SoldierArmorState::applyArmorSelection(Armor* next)
  * store path a list click drives. Returns false if that armor is not on the list.
  * Does not pop the state.
  */
-bool SoldierArmorState::harnessSetArmor(const std::string& armorType)
+bool SoldierArmorState::harnessSetArmor(const std::string& armorType, bool bypassGate, std::string& blockedBy)
 {
+	blockedBy.clear();
 	for (const auto& armorItem : _armors)
 	{
 		if (armorItem.type == armorType)
 		{
 			Armor* next = _game->getMod()->getArmor(armorType, false);
 			if (!next) return false;
+			if (!bypassGate)
+			{
+				Soldier* soldier = _base->getSoldiers()->at(_soldier);
+				Craft* craft = soldier->getCraft();
+				if (craft && !craft->validateArmorChange(soldier->getArmor()->getSize(), next->getSize()))
+				{
+					blockedBy = "STR_NOT_ENOUGH_CRAFT_SPACE";
+					return true; // found, but the client gate blocked it
+				}
+			}
 			applyArmorSelection(next);
 			return true;
 		}

src/Basescape/SoldierArmorState.h

@@ -100,7 +100,12 @@ class SoldierArmorState : public State
 	/// Handler for clicking the Name arrow.
 	void sortNameClick(Action *action);
 	/// Harness (PRD-J09 GAP-5b): drive the real armor-change store path for one type.
-	bool harnessSetArmor(const std::string& armorType);
+	/// Returns false only when @a armorType is not on the soldier's armor list. With
+	/// @a bypassGate false the client craft-space gate (Craft::validateArmorChange) runs
+	/// first and, if it blocks, fills @a blockedBy with its STR_ error and leaves the
+	/// change unmade; bypassGate models a stale replica whose gate passed (issue #121),
+	/// letting the request reach the host un-gated.
+	bool harnessSetArmor(const std::string& armorType, bool bypassGate, std::string& blockedBy);
 };
 
 }

src/CoopMod/SharedEcon.cpp

@@ -79,6 +79,7 @@
 
 #include <cmath>
 #include "../Basescape/BaseView.h"
+#include "../Basescape/CraftWeaponsState.h" // issue #121: shared craft-weapon capacity gate
 #include "../Geoscape/GeoscapeState.h"
 #include "../Geoscape/ConfirmLandingState.h"
 #include "../Geoscape/Globe.h"
@@ -1958,13 +1959,23 @@ bool craftRearmValidate(Game* game, const Json::Value& payload, Base* base, int
 	int slot = payload.get("slot", -1).asInt();
 	if (slot < 0 || slot >= (int)craft->getWeapons()->size()) { failReason = "bad weapon slot"; return false; }
 	std::string wtype = payload.get("weapon", "").asString();
+	const RuleCraftWeapon* selRule = nullptr;
 	if (!wtype.empty())
 	{
-		const RuleCraftWeapon* w = game->getMod()->getCraftWeapon(wtype, false);
-		if (!w) { failReason = "unknown craft weapon"; return false; }
-		if (!craft->getRules()->isValidWeaponSlot((size_t)slot, w->getWeaponType()))
+		selRule = game->getMod()->getCraftWeapon(wtype, false);
+		if (!selRule) { failReason = "unknown craft weapon"; return false; }
+		if (!craft->getRules()->isValidWeaponSlot((size_t)slot, selRule->getWeaponType()))
 		{ failReason = "weapon not valid for slot"; return false; }
 	}
+
+	// issue #121: re-run the SAME four capacity gates the client's CraftWeaponsState
+	// enforces, against the host-authoritative craft. The client gate can pass on a
+	// stale replica view; the host is the single authority, so it must reject an
+	// over-capacity swap here rather than apply it and desync/deploy-fault later.
+	CraftWeapon* current = craft->getWeapons()->at(slot);
+	const RuleCraftWeapon* curRule = current ? current->getRules() : nullptr;
+	std::string capErr = CraftWeaponsState::equipCapacityError(game->getMod(), craft, selRule, curRule);
+	if (!capErr.empty()) { failReason = capErr; return false; }
 	return true;
 }
 
@@ -2020,8 +2031,18 @@ bool soldierArmorValidate(Game* game, const Json::Value& payload, Base* base, in
 {
 	cost = 0;
 	if (!base) { failReason = "base not found"; return false; }
-	if (!findSoldier(base, payload.get("soldierId", -1).asInt())) { failReason = "soldier not found"; return false; }
-	if (!game->getMod()->getArmor(payload.get("armor", "").asString(), false)) { failReason = "unknown armor"; return false; }
+	Soldier* s = findSoldier(base, payload.get("soldierId", -1).asInt());
+	if (!s) { failReason = "soldier not found"; return false; }
+	Armor* next = game->getMod()->getArmor(payload.get("armor", "").asString(), false);
+	if (!next) { failReason = "unknown armor"; return false; }
+
+	// issue #121: re-run the client's craft-space gate (SoldierArmorState::lstArmorClick)
+	// against the host-authoritative world. If the soldier rides a craft, a larger armor
+	// must still fit; the client gate can pass on a stale replica view, so the host is the
+	// backstop that keeps a shared craft from being pushed over capacity.
+	Craft* craft = s->getCraft();
+	if (craft && !craft->validateArmorChange(s->getArmor()->getSize(), next->getSize()))
+	{ failReason = "STR_NOT_ENOUGH_CRAFT_SPACE"; return false; }
 	return true;
 }
 

src/CoopMod/TestServer.cpp

@@ -1107,11 +1107,15 @@ bool TestServer::executeShared10(const std::string& cmd, const Json::Value& req,
 		// the shared base stores). In SHARED (after the fix) this routes a
 		// craft_rearm shared_cmd host-side; before it, it mutates THIS machine's
 		// base stores locally - the pre-battle store drift GAP-5b closes.
-		// Params: weapon, slot (default 0), optional base + craft_id.
+		// Params: weapon, slot (default 0), optional base + craft_id, optional force.
+		// force=true bypasses the client capacity gate (issue #121: models a stale
+		// replica whose gate passed), so the request reaches the host un-gated and
+		// the host validator is the one under test.
 		std::string weapon = req.get("weapon", "").asString();
 		int slot = req.get("slot", 0).asInt();
 		std::string baseName = req.get("base", "").asString();
 		int craftId = req.get("craft_id", -1).asInt();
+		bool force = req.get("force", false).asBool();
 		Base* target = nullptr;
 		if (_game->getSavedGame())
 			for (auto* b : *_game->getSavedGame()->getBases())
@@ -1130,11 +1134,28 @@ bool TestServer::executeShared10(const std::string& cmd, const Json::Value& req,
 		else
 		{
 			CraftWeaponsState* cws = new CraftWeaponsState(target, idx, (size_t)slot);
-			bool moved = cws->harnessEquip(weapon);
+			std::string blockedBy;
+			bool found = cws->harnessEquip(weapon, force, blockedBy);
 			delete cws;
-			resp["moved"] = moved;
-			resp["ok"] = moved;
-			if (!moved) resp["error"] = "weapon not on craft armament list: " + weapon;
+			if (!found)
+			{
+				resp["moved"] = false;
+				resp["ok"] = false;
+				resp["error"] = "weapon not on craft armament list: " + weapon;
+			}
+			else if (!blockedBy.empty())
+			{
+				// The client capacity gate blocked it (no request sent). Faithful to a
+				// real player's screen; the test asserts on resp["gate"].
+				resp["moved"] = false;
+				resp["ok"] = false;
+				resp["gate"] = blockedBy;
+			}
+			else
+			{
+				resp["moved"] = true;
+				resp["ok"] = true;
+			}
 		}
 	}
 	else if (cmd == "soldier_armor")
@@ -1143,10 +1164,13 @@ bool TestServer::executeShared10(const std::string& cmd, const Json::Value& req,
 		// through the REAL SoldierArmorState store path (returns the old armor's
 		// store item, consumes the new one). In SHARED (after the fix) this routes a
 		// soldier_armor shared_cmd; before it, it mutates THIS machine's base stores.
-		// Params: soldier_id, armor, optional base.
+		// Params: soldier_id, armor, optional base, optional force. force=true bypasses
+		// the client craft-space gate (issue #121: models a stale replica whose gate
+		// passed), so the host validator is the one under test.
 		std::string armor = req.get("armor", "").asString();
 		int soldierId = req.get("soldier_id", -1).asInt();
 		std::string baseName = req.get("base", "").asString();
+		bool force = req.get("force", false).asBool();
 		Base* target = nullptr;
 		if (_game->getSavedGame())
 			for (auto* b : *_game->getSavedGame()->getBases())
@@ -1165,11 +1189,26 @@ bool TestServer::executeShared10(const std::string& cmd, const Json::Value& req,
 		else
 		{
 			SoldierArmorState* sas = new SoldierArmorState(target, sidx, SA_GEOSCAPE);
-			bool moved = sas->harnessSetArmor(armor);
+			std::string blockedBy;
+			bool found = sas->harnessSetArmor(armor, force, blockedBy);
 			delete sas;
-			resp["moved"] = moved;
-			resp["ok"] = moved;
-			if (!moved) resp["error"] = "armor not on soldier list: " + armor;
+			if (!found)
+			{
+				resp["moved"] = false;
+				resp["ok"] = false;
+				resp["error"] = "armor not on soldier list: " + armor;
+			}
+			else if (!blockedBy.empty())
+			{
+				resp["moved"] = false;
+				resp["ok"] = false;
+				resp["gate"] = blockedBy;
+			}
+			else
+			{
+				resp["moved"] = true;
+				resp["ok"] = true;
+			}
 		}
 	}
 	else if (cmd == "craft_deequip_armor")