bugfix(ai): Fix AICommandParmsStorage::doXfer to transfer the command source

[bas-slats]

• Fixes AICommandParmsStorage::doXfer doesn't save command source #766

AICommandParmsStorage::doXfer transfers m_cmd twice — xfer->xferUser(&m_cmd, sizeof(m_cmdSource)) on the second line — and never transfers m_cmdSource. AICommandParmsStorage has no constructor, so when a saved game with a pending AI command is loaded, the command executes with an uninitialized command source. This change transfers m_cmdSource on the second line, for both Zero Hour and Generals.

The save record layout is unchanged: AICommandType and CommandSourceType are both enum-sized and the second field is already written with sizeof(m_cmdSource). Loading a save created before this change reads the duplicated command bytes into m_cmdSource, which can be out of range for CommandSourceType — and downstream code shifts by it unchecked (okSrcs & (1 << cmdSource) in WeaponSet.cpp). Loads therefore validate the value and fall back to CMD_FROM_AI when it is out of range. This also covers the pre-fix hazard, where m_cmdSource was left as uninitialized memory after loading.

Testing

Save/load compatibility is tested manually in game (Zero Hour, win32 preset build, retail Steam 1.04 data):

• A skirmish saved with an unpatched build loads correctly in the patched build — units respond to orders normally afterwards. This exercises the old-save path, where the duplicated command bytes are read into m_cmdSource.
• A save created and reloaded within the patched build round-trips cleanly.

There is no unit-test harness in the repository, so the semantic fix itself (the correct value arriving in m_cmdSource) is verified by inspection: the write and read sides are the same line, and doXfer runs only during save/load, so replays and normal simulation are unaffected.

Compiles for both games with the win32 preset (VS2019 16.11, Ninja Multi-Config, Release): generalszh.exe and generalsv.exe link.

---

This change was developed with AI assistance (Claude), human-directed: the duplicated transfer, the missing constructor initialization, and the record-size equivalence were verified by hand against the class definition in AI.h.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix AICommandParmsStorage save/load to transfer command source

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Fix AICommandParmsStorage::doXfer to serialize m_cmdSource instead of duplicating m_cmd.
• Prevent pending AI commands from executing with an uninitialized command source after save/load.
• Apply the fix consistently to both Generals and Zero Hour codepaths.

Diagram

graph TD
  XFER["Xfer (save/load)"] --> STORAGE["AICommandParmsStorage::doXfer"] --> CMD["m_cmd"] --> AIUPD["AIUpdate pending cmd"] --> EXEC["AI executes command"]
  STORAGE --> CMDSRC["m_cmdSource"] --> AIUPD

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Gate behavior behind an Xfer version bump

• ➕ Old saves could default cmdSource instead of inheriting duplicated command bytes
• ➕ Provides explicit long-term control over save compatibility semantics
• ➖ Requires versioning changes at the caller/containing record(s)
• ➖ Slightly more invasive and needs broader testing across versions

2. Add a constructor/default initialization for AICommandParmsStorage

• ➕ Eliminates uninitialized-field risk even if future serialization regressions occur
• ➕ Improves defensive robustness for any non-xfer initialization paths
• ➖ Does not fix the missing serialization by itself (still need this PR’s change)
• ➖ Potentially masks serialization bugs by always supplying a default

Recommendation: Keep the current minimal fix: it corrects the field transfer without changing record layout, and it makes loading older saves deterministic (bounded enum value rather than uninitialized memory). If maintaining an exact default cmdSource for pre-fix saves is a requirement, add a version bump/gate at the containing xfer layer.

Files changed (2) +4 / -2

Bug fix (2) +4 / -2

AIStates.cppFix doXfer to serialize m_cmdSource (Generals) +2/-1

Fix doXfer to serialize m_cmdSource (Generals)

• Corrects AICommandParmsStorage::doXfer to transfer m_cmdSource instead of transferring m_cmd twice. Prevents pending AI commands from using an uninitialized command source after loading a save.

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

AIStates.cppFix doXfer to serialize m_cmdSource (Zero Hour) +2/-1

Fix doXfer to serialize m_cmdSource (Zero Hour)

• Mirrors the Generals fix in the Zero Hour codebase by transferring m_cmdSource during AICommandParmsStorage::doXfer. Ensures consistent save/load behavior across both games.

GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

1. Invalid cmdSource from old saves 🐞 Bug ☼ Reliability

Description

When loading a save created before this fix, the slot now read into m_cmdSource contains the old
duplicated m_cmd bytes, producing out-of-range CommandSourceType values. That invalid value is
later used unchecked (e.g., (1 << cmdSource) in weapon selection), which can cause undefined
behavior/crashes and incorrect command-source filtering.

Code

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[R129-130]

+	// TheSuperHackers @bugfix Transfer the command source instead of the command twice.
+	xfer->xferUser(&m_cmdSource, sizeof(m_cmdSource));

Evidence

The PR change causes the second serialized field to be interpreted as CommandSourceType on load;
for older saves that field contains duplicated AICommandType bytes. CommandSourceType is a small
enum, but downstream code uses it as a shift count (1 << cmdSource), which becomes undefined
behavior for out-of-range values, so the loaded value must be validated/clamped.

Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[126-134]
Core/GameEngine/Include/Common/GameCommon.h[217-227]
Generals/Code/GameEngine/Include/GameLogic/AI.h[345-365]
Generals/Code/GameEngine/Source/GameLogic/Object/WeaponSet.cpp[812-820]
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DeployStyleAIUpdate.cpp[478-513]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
After this PR, `AICommandParmsStorage::doXfer` correctly transfers `m_cmdSource`, but **old saves written by the previous code** stored a duplicate of `m_cmd` in the second field. Loading those saves will therefore set `m_cmdSource` to an invalid `CommandSourceType` value.

This becomes dangerous because other code paths assume `CommandSourceType` is in-range and use it in bit operations (e.g., `1 << cmdSource`), which can become undefined behavior if `cmdSource` is large.

### Issue Context
- `CommandSourceType` has only a few valid values (`CMD_FROM_PLAYER`..`CMD_DEFAULT_SWITCH_WEAPON`, then `COMMAND_SOURCE_TYPE_COUNT`).
- `AICommandType` has many values and can exceed the bit-width safe range for shifting.
- `AICommandParmsStorage::doXfer` currently does not validate `m_cmdSource` after loading.

### Fix Focus Areas
- Add a post-load validation step in `AICommandParmsStorage::doXfer` to clamp/normalize `m_cmdSource` when `xfer->getXferMode() == XFER_LOAD`.
 - Example approach: after `xferUser(&m_cmdSource, ...)`, check `static_cast<Int>(m_cmdSource)` is within `[0, COMMAND_SOURCE_TYPE_COUNT)`; if not, set to a safe default (likely `CMD_FROM_AI`).
- Apply the same fix in both game variants.

Recommended code shape (illustrative):
```cpp
xfer->xferUser(&m_cmdSource, sizeof(m_cmdSource));
if (xfer->getXferMode() == XFER_LOAD) {
 const Int cs = static_cast<Int>(m_cmdSource);
 if (cs < 0 || cs >= COMMAND_SOURCE_TYPE_COUNT) {
   m_cmdSource = CMD_FROM_AI;
 }
}
```

### Fix Focus Areas (exact locations)
- Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[126-133]
- GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[129-136]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Tickets: 🎫 `AICommandParmsStorage::doXfer` doesn't save command source

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

1. Invalid cmdsource from old saves 🐞 Bug ☼ Reliability

When loading a save created before this fix, the slot now read into m_cmdSource contains the old
duplicated m_cmd bytes, producing out-of-range CommandSourceType values. That invalid value is
later used unchecked (e.g., (1 << cmdSource) in weapon selection), which can cause undefined
behavior/crashes and incorrect command-source filtering.

Agent Prompt

### Issue description
After this PR, `AICommandParmsStorage::doXfer` correctly transfers `m_cmdSource`, but **old saves written by the previous code** stored a duplicate of `m_cmd` in the second field. Loading those saves will therefore set `m_cmdSource` to an invalid `CommandSourceType` value.

This becomes dangerous because other code paths assume `CommandSourceType` is in-range and use it in bit operations (e.g., `1 << cmdSource`), which can become undefined behavior if `cmdSource` is large.

### Issue Context
- `CommandSourceType` has only a few valid values (`CMD_FROM_PLAYER`..`CMD_DEFAULT_SWITCH_WEAPON`, then `COMMAND_SOURCE_TYPE_COUNT`).
- `AICommandType` has many values and can exceed the bit-width safe range for shifting.
- `AICommandParmsStorage::doXfer` currently does not validate `m_cmdSource` after loading.

### Fix Focus Areas
- Add a post-load validation step in `AICommandParmsStorage::doXfer` to clamp/normalize `m_cmdSource` when `xfer->getXferMode() == XFER_LOAD`.
  - Example approach: after `xferUser(&m_cmdSource, ...)`, check `static_cast<Int>(m_cmdSource)` is within `[0, COMMAND_SOURCE_TYPE_COUNT)`; if not, set to a safe default (likely `CMD_FROM_AI`).
- Apply the same fix in both game variants.

Recommended code shape (illustrative):
```cpp
xfer->xferUser(&m_cmdSource, sizeof(m_cmdSource));
if (xfer->getXferMode() == XFER_LOAD) {
  const Int cs = static_cast<Int>(m_cmdSource);
  if (cs < 0 || cs >= COMMAND_SOURCE_TYPE_COUNT) {
    m_cmdSource = CMD_FROM_AI;
  }
}
```

### Fix Focus Areas (exact locations)
- Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[126-133]
- GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp[129-136]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[bas-slats]

Good catch - verified: WeaponSet.cpp shifts by the command source unchecked, so an out-of-range value from an old save is a real hazard (as was the uninitialized value before this fix). Loads now validate m_cmdSource and fall back to CMD_FROM_AI when out of range, in both games.

[Skyaero42]

Is this retail compatible?

[Caball009]

I'd expect the fix to look something like this:

// ------------------------------------------------------------------------------------------------
/** Xfer method
	* Version Info:
	* 1: TheSuperHackers @fix Add version control and fix xfer of m_cmdSource
	*/
// ------------------------------------------------------------------------------------------------
void AICommandParmsStorage::doXfer(Xfer *xfer)
{
	// version
#if RETAIL_COMPATIBLE_XFER_SAVE
	const XferVersion version = 0;
#else
	const XferVersion currentVersion = 1;
	XferVersion version = currentVersion;
	xfer->xferVersion(&version, currentVersion);
#endif

	xfer->xferUser(&m_cmd, sizeof(m_cmd));

	if (version >= 1)
	{
		xfer->xferUser(&m_cmdSource, sizeof(m_cmdSource));
	}
	else
	{
		xfer->xferUser(&m_cmd, sizeof(m_cmdSource));
	}
	
	...
}