feat: BGE-296 detailed battle reports with resource pillaging

[discostu105]

Summary

• Extends battle result to show per-unit-type losses for both attacker and defender
• Implements resource pillaging: attacker steals 10% of all non-land resources (capped at 5000 each) on win
• Populates BattleResultViewModel with outcome, names, unit losses, resources pillaged, land transferred, workers captured, and strength-before values
• Updates EnemyBase.razor with rich post-battle UI: outcome badge, spoils of war section, collapsible unit losses table, and kill ratio
• Updates battle report messages to include pillaged resources and draw outcome
• Adds BattleResourcePillageTest covering pillage-on-win, no-pillage-on-loss, and strength fields

Test plan

• dotnet build passes
• dotnet test passes (167 tests)
• Attack an enemy and confirm battle result shows outcome, unit losses, and pillaged resources
• Lose a battle and confirm no resources are stolen
• Check inbox message to see updated battle report format including resources pillaged

Closes BGE-296

[discostu105]

PR Review: feat: BGE-296 detailed battle reports with resource pillaging

Verdict: REQUEST_CHANGES

Summary

The core battle mechanics (resource pillaging, unit loss display, strength-before fields) are implemented correctly and all tests pass. However, there are two blocking gaps: a stale TODO comment that actively misleads readers, a missing cap-enforcement test, and an unmet acceptance criterion around notification center depth.

Issues

• [blocking] src/BrowserGameEngine.StatefulGameServer/Repositories/Units/UnitRepositoryWrite.cs:297 — Stale TODO: apply resourses stolen/lost comment inside ApplyBatteResult. StealResources now fulfills this — leaving the TODO in place tells every future reader that pillage is still unimplemented. Remove it.

• [blocking] src/BrowserGameEngine.StatefulGameServer.Test/BattleResourcePillageTest.cs — No test for the 5000-per-resource cap. The PR description explicitly calls it out, but all three tests use defender resources ≤ 2000. Add a test with the defender at e.g. 100 000 minerals and assert stolen amount == 5000 (not 10 000).

• [blocking] src/BrowserGameEngine.StatefulGameServer/BattleReportGenerator.cs:32 — Acceptance criterion #3 says "Detailed report stored and viewable in notification center." The bell-icon notification center only gets "Your base was attacked by X!". The full detail (pillaged resources, unit losses) goes to the Messages inbox via messageRepositoryWrite, not into the push notification. The attacker receives no push notification at all. Either push the detail to the notification center, or clarify the acceptance criterion.

• [nit] src/BrowserGameEngine.StatefulGameServer.Test/BattleResourcePillageTest.cs:73 — TotalDefenderStrengthBefore >= 0 allows zero. Since the defender always has units, assert > 0 to actually validate the field is populated.

• [nit] src/BrowserGameEngine.GameModel/BattleResult.cs:23 — Missing newline at end of file (trivial fix while the file is open).

Positive Notes

• Thread safety correct: StealResources → resourceRepositoryWrite.AddResources holds its own lock(_lock) per call, consistent with ApplyLandTransfer.
• [Authorize] at class level + currentUserContext.IsValid checked before every action — no security issues.
• BattleResultViewModel breaking change (removing old PlayerLostUnits/EnemyLostUnits) is safe — confirmed not referenced anywhere else in the codebase.
• Outcome logic (attackerWon / draw / DefenderWon) is consistent across UnitRepositoryWrite, BattleController, and BattleReportGenerator.
• Spoils-of-war UI correctly gates display behind non-empty pillage/transfer data.

Build/Test Result

• Build: PASS
• Tests: PASS (167 passed, 0 failed)

[discostu105]

Architectural review: Approved.

What I checked:

• StealResources() correctly mirrors the existing ApplyLandTransfer() pattern — same repo injection approach, same resource ID handling, fills in the pre-existing // TODO: apply resourses stolen/lost in ApplyBatteResult.
• Cost.FromDict() exists and accepts IDictionary<ResourceDefId, decimal> — the usage is correct.
• Layering is respected: GameModel gets new fields, StatefulGameServer owns the logic, Shared expands the DTO, FrontendServer maps it, BlazorClient renders it.
• TotalAttackerStrengthBefore/TotalDefenderStrengthBefore are set before ApplyBatteResult mutates unit lists, so they correctly capture pre-battle state.
• Test coverage is adequate for the new pillage logic.

Non-blocking notes for the backlog:

1. Win/draw/loss determination is now triplicated across UnitRepositoryWrite.Attack(), BattleController.Attack(), and BattleReportGenerator.GenerateReports(). Future refactor: expose an Outcome property on BtlResult to centralize this.
2. BattleResultViewModel.Outcome is a raw string — an enum would be more type-safe downstream.

Neither blocks merge. Ship it.

[discostu105]

PR Review: feat: BGE-296 detailed battle reports with resource pillaging

Verdict: APPROVE ✅ (cannot self-approve — posting as comment)

Summary

Extends battle results with per-unit-type loss breakdowns, 10%-pillage resource stealing on win (capped at 5000 each), outcome badge, and a collapsible unit losses table in the Blazor UI. Notification body enriched with draw outcome, workers captured, and pillaged resources.

Issues

• [nit] BattleController.cs:125–128 — attackerWon/draw/outcome logic is duplicated from BattleReportGenerator. Both are correct, but any future logic change needs to be made in two places. Consider extracting a static helper on BtlResult or a shared utility.

• [nit] BattleResourcePillageTest.cs:8 — private static PlayerId Player2 => PlayerIdFactory.Create("player1") — naming is slightly confusing (field called Player2, ID is "player1"). Not a bug (consistent with TestWorldStateFactory's 0-indexed players), but a comment would help future readers.

Positive Notes

• [Authorize] on the controller class plus currentUserContext.IsValid check in Attack — authorization is correct.
• StealResources follows the exact same pattern as the pre-existing ApplyLandTransfer (calls resourceRepositoryWrite.AddResources in the same unlocked code path — not a new threading concern introduced by this PR).
• BtlResult.ResourcesStolen is initialized to an empty list in BattleBehaviorScoOriginal.CalculateResult(), so StealResources' .Add(...) is safe.
• gameDef.GetUnitDef(uc.UnitDefId)?.Name ?? uc.UnitDefId.Id — graceful fallback for unknown unit defs.
• ViewModel collections initialized with = new() — no null-ref risk in the Razor template.
• Three well-targeted tests: win+pillage, loss+no-pillage, strength fields populated. All use TestGame directly, no controller mocks.
• Blazor UI uses Bootstrap classes consistently and the <details> collapse is a clean UX pattern.

Build/Test Result

• Build: PASS (0 errors, 70 pre-existing warnings)
• Tests: PASS (167 passed, 0 failed)

[discostu105]

Review blockers addressed

• Stale TODO removed: Deleted // TODO: apply resourses stolen/lost from ApplyBatteResult — StealResources was already fulfilling this from Attack().
• Cap test added: Attack_Win_PillageCapEnforced — defender starts with 100k minerals, asserts stolen == 5000 (capped), not 10k.
• Strength assertion fixed: TotalDefenderStrengthBefore >= 0 → > 0.
• Notification center: Both attacker and defender now receive push notifications with outcome + pillage summary (e.g. "Battle vs Defender: Attacker won. Pillaged: 5000 res1").

All 168 tests pass.

[discostu105]

Architect Review — BGE-296 Detailed Battle Reports with Resource Pillaging

Verdict: Approved ✅

Analysis

Layer compliance: All changes respect the dependency graph. GameModel gets two new int fields, Shared gets a proper ViewModel, StatefulGameServer implements the logic, FrontendServer maps to the ViewModel, and BlazorClient renders it. Clean flow end-to-end.

Thread safety: StealResources() is called from within Attack() which executes under the existing write-repository lock. The resourceRepositoryWrite calls are within that same locked section. Safe.

Pillage logic: 10% capped at 5000 per resource, land excluded — straightforward and correctly bounded. Math.Floor on the steal amount avoids fractional resource leakage. Cap enforcement verified by test.

Outcome determination duplicated between BattleController and BattleReportGenerator (both compute attackerWon/draw). Minor smell but acceptable given the simplicity — no correctness risk.

Outcome as a string in the ViewModel is fine for a client-facing DTO. String values are consistent between controller and Razor template.

TotalAttackerStrengthBefore is set from attackingUnits before ApplyBatteResult() removes units — correctly captures pre-battle strength.

Tests: 4 scenarios cover win+pillage, loss+no-pillage, strength fields populated, and cap enforcement. Solid. CI passes (167 tests).

Minor note: Player2 static field in BattleResourcePillageTest is initialized to PlayerIdFactory.Create("player1") — the string "player1" is confusing naming (0-indexed id for what the test calls player 2). Tests pass so it's not a bug, but worth cleaning up if revisiting this file.

[discostu105]

PR Review: BGE-296 Detailed Battle Reports with Resource Pillaging

Verdict: APPROVE

Summary

Correctly implements detailed battle reports (per-unit losses, outcome label, strength-before fields) and resource pillaging (10% of non-land resources, capped at 5000 each, attacker-win-only). All logic flows correctly through the stack and all 168 tests pass.

Issues (nits only)

• [nit] BattleResourcePillageTest.cs:10 — Static field named Player2 has value PlayerIdFactory.Create("player1") — confusing naming. Consider SecondPlayerId with a clarifying comment.

• [nit] BattleResourcePillageTest.cs:41 — Assertion Assert.Equal(defenderResourcesBefore * 0.10m, stolen["res1"]) works because initial value is exactly 1000 (floor is a no-op). Should be Assert.Equal(Math.Floor(defenderResourcesBefore * 0.10m), stolen["res1"]) to match the spec.

• [nit] BattleResultViewModel.cs:13 — Outcome is stringly-typed ("AttackerWon", "DefenderWon", "Draw"). A BattleOutcome enum would prevent string drift.

• [nit] No test for the draw outcome (both sides wiped). Low priority but worth adding.

Positive Notes

• StealResources cleanly isolated as a private method, consistent with ApplyLandTransfer pattern.
• Resource writes correctly delegated to ResourceRepositoryWrite.AddResources (which has its own lock).
• BattleReportGenerator now sends attacker notifications including pillage info — meaningful UX improvement.
• [Authorize] inherited at controller class level; currentUserContext.IsValid checked correctly.
• Pillage cap test correctly sets up >50000 resources so 10% exceeds the 5000 cap.

Build/Test Result

• Build: PASS
• Tests: PASS (168/168 — 4 new battle pillage tests all pass)

This PR is ready to merge.