Validate troop limit commands

[DevOpsOfChaos]

Summary

• validate serialized soldier ranks before executing the affected rank-based game commands
• reject invalid rank values during command deserialization instead of reaching building-state access with unchecked input
• currently this applies to SetTroopLimit and ChangeReserve, both using popSoldierRank
• add regression coverage for invalid serialized rank values and unchanged troop-limit state after rejection

Motivation

Troop limits and reserves are normally changed through UI paths where rank values are bounded.

However, the affected game commands store the rank as a raw uint8_t and deserialize it from the command stream before forwarding it into game-state handling.

Review feedback pointed out that silently ignoring invalid values inside nobMilitary::SetTroopLimit is not the preferred style. This version validates the deserialized command value instead, matching the existing approach used for invalid/out-of-range enum values.

Behavior

• valid rank-based commands keep their existing behavior
• invalid serialized rank values now throw std::range_error via helpers::makeOutOfRange
• invalid ranks are rejected during deserialization before building state can be changed
• SetTroopLimit and ChangeReserve now both use popSoldierRank
• nobMilitary::SetTroopLimit keeps only an internal assert

Validation

• Built Test_integration locally with Visual Studio 2022 / Debug
• Ran Test_integration.exe --run_test=GameCommandSuite --report_level=short
  • 30 test cases passed
  • 3930 assertions passed
• Ran Test_integration.exe --run_test=AttackSuite --report_level=short
  • 23 test cases passed
  • 1262 assertions passed
• Ran full Test_integration.exe --report_level=short
  • 194 test cases passed
  • 93363 assertions passed
• Ran ctest -R "^Test_integration$" --output-on-failure
  • passed
• Ran git diff --check upstream/master...HEAD
  • clean

[Flamefire]

Even though a limit > soldier count might not look right, it doesn't cause any issues: A large limit is just the same as no limit.

So if this doesn't fix an actual wrong behavior resulting from that I'd keep it, especially as it is virtually impossible to trigger/use the wrong count, so nothing will be gained by the added code.

[DevOpsOfChaos]

Even though a limit > soldier count might not look right, it doesn't cause any issues: A large limit is just the same as no limit.

So if this doesn't fix an actual wrong behavior resulting from that I'd keep it, especially as it is virtually impossible to trigger/use the wrong count, so nothing will be gained by the added code.

Good point. I removed the count clamp.

Large troop-limit values now keep the existing behavior. The PR only keeps the invalid-rank guard before indexing the troop-limit array.

[Flamefire]

See my prior comment

[DevOpsOfChaos]

Updated this PR according to the deserialization-boundary feedback.

The invalid rank is no longer silently ignored in nobMilitary::SetTroopLimit. Instead, gc::SetTroopLimit now validates the deserialized rank before Execute() can call into the building code.

Invalid serialized ranks now throw std::range_error via helpers::makeOutOfRange, matching the existing style used for invalid enum deserialization. nobMilitary::SetTroopLimit keeps only an internal assert, and large valid troop-limit counts remain accepted unchanged.

Validation:

• clang-format version 10.0.0
• built Test_integration
• Test_integration.exe --run_test=GameCommandSuite/SetTroopLimitRejectsInvalidSerializedRank --report_level=short
  • 1 test case
  • 1 assertion
• Test_integration.exe --run_test=AttackSuite --report_level=short
  • 23 test cases
  • 1244 assertions
• git diff --check upstream/master...HEAD
  • clean

[DevOpsOfChaos]

Applied the two review suggestions with one correction.

• renamed the helper to popSoldierRank
• kept the validation scoped to SetTroopLimit
• updated the internal assert to rank < troop_limits.size()

I used < instead of the suggested <= because troop_limits[rank] uses 0-based indexing, so rank == troop_limits.size() would already be out of range.

I also noticed ChangeReserve deserializes a soldier rank directly, but I left it unchanged here to avoid broadening this PR beyond the reviewed SetTroopLimit command path.

[Flamefire]

I also noticed ChangeReserve deserializes a soldier rank directly, but I left it unchanged here to avoid broadening this PR beyond the reviewed SetTroopLimit command path.

I'd be fine with using the new function in all command deserializations where it applies, at least for consistency, possibly without (new) tests. In the future we might otherwise wonder why it wasn't used.
Of course just where it is obviously correct, i.e. also had the same rank(ser.PopUnsignedChar())

[DevOpsOfChaos]

Applied the remaining review feedback.

• ChangeReserve now uses the same popSoldierRank validation helper as SetTroopLimit; these are the only directly deserialized command fields that are clearly soldier ranks.
• Consolidated the invalid-rank coverage: the deserialize test now covers both rank-based commands.
• Strengthened the military-state regression test with deterministic, distinct initial troop limits for every rank and verifies that an invalid command leaves every value unchanged.
• Removed the redundant near-duplicate invalid-rank coverage.

Validation passed locally:

• GameCommandSuite: 30 test cases / 3930 assertions
• AttackSuite: 23 test cases / 1262 assertions
• full Test_integration: 194 test cases / 93363 assertions
• ctest -R "^Test_integration$": passed
• git diff --check upstream/master...HEAD: clean

I also checked the previously reported GitHub Actions failure. The Linux test job itself completed with 100% tests passed, 0 tests failed out of 20; the later failure was the Codecov upload step reporting that its signature could not be verified. That appears unrelated to this PR's test behavior.

[Flamefire]

Looks good now, only 2 minor things in the test.

Consolidated the invalid-rank coverage: the deserialize test now covers both rank-based commands.

How so? Because both commands would use the same function that triggers the error? Or am I missing something in the tests?

[Flamefire]

I also added an explicit uniqueness check so the test fails immediately if that setup invariant ever changes.

While not wrong I think this is where your AI overshot the target:
a) The uniqueness isn't strictly required, fully random values would do
b) There is no invariant as far as I can see. If you want to state that unique values shall be used then rather state the reason than adding complexity

See inline suggestions

[Flamefire]

To be transparent, I did not independently review the AI-assisted follow-up to the standard it needed before posting it.

That really is an issue for us. We currently get a lot of AI assisted/generated PRs but AI is still bad at understanding such a complex code base and producing clean, maintainable code.
We work on this project in our spare time so if it feels like we spend too much time on AI slop that hasn't been cleaned up before submission we'll need to ban use of AI as other projects already do.

So please help us here and make sure the changes are clean and reviewed before pushing. It would be bad if we couldn't profit from the increased speed enabled by AI due to its shortcomings.

[DevOpsOfChaos]

To be transparent, I did not independently review the AI-assisted follow-up to the standard it needed before posting it.

That really is an issue for us. We currently get a lot of AI assisted/generated PRs but AI is still bad at understanding such a complex code base and producing clean, maintainable code. We work on this project in our spare time so if it feels like we spend too much time on AI slop that hasn't been cleaned up before submission we'll need to ban use of AI as other projects already do.

So please help us here and make sure the changes are clean and reviewed before pushing. It would be bad if we couldn't profit from the increased speed enabled by AI due to its shortcomings.

You are right to raise this.

My earlier wording was imprecise: I did review the generated follow-up and validate it locally before pushing it, but I did not review the resulting test structure deeply enough against the existing codebase patterns. That distinction does not change the unnecessary review work it caused.

I understand that AI assistance is only useful here when it reduces maintainer effort rather than shifting cleanup work to reviewers. I will treat generated output as a draft, review the final diff and surrounding project patterns more critically, and only push follow-ups that I can clearly explain and justify. Sorry for the unnecessary churn.