NEO-80: Add code review for combat entity health store.

docs/reviews/2026-05-24-NEO-80.md

@@ -0,0 +1,62 @@
+# Code review — NEO-80 combat entity health store
+
+**Date:** 2026-05-24  
+**Scope:** Branch `NEO-80-combat-entity-health-store` · commits `e729996`–`b060b9d` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-80 delivers **E5M1-05**: a thread-safe, in-memory `ICombatEntityHealthStore` for prototype combat dummies (`prototype_target_alpha`, `prototype_target_beta`), with lazy init to `PrototypeCombatConstants.MaxPrototypeTargetHp` (100), damage flooring at zero, defeated flag semantics, and `TryResetToFull` for tests/fixtures. DI registers the store as a singleton via `AddCombatEntityHealthStore()`, chained from `AddAbilityDefinitionCatalog` per the plan. Ten AAA unit/host tests cover lazy init, damage, defeat/overkill, reset, alpha/beta independence, empty/unknown/negative deny paths, and host resolution. Full suite (377 tests) passes. Implementation aligns with the kickoff plan and mirrors the NEO-61 per-id locking pattern. Residual risk is low — server-only internal store with no HTTP surface; module tracking docs and one minor test gap should be closed on merge.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-80-implementation-plan.md`](../plans/NEO-80-implementation-plan.md) | **Matches** — kickoff decisions, scope, acceptance checklist, reconciliation; DI chained from ability catalog (not `Program.cs`) as documented in Files to modify. |
+| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-05** | **Partially matches** — implementation complete; backlog AC checkboxes still unchecked and no landed note yet. |
+| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Partially matches** — Status still says “E5M1-05+ pending”; should cite NEO-80 landed after merge. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E5.M1 note ends at NEO-78; NEO-80 health store not yet recorded. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E5.M1 row should add NEO-80 landed on merge. |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server-only in-memory store; no client or HTTP changes. |
+| [`server/README.md`](../../server/README.md) | **Matches** — combat entity health section documents init, damage, reset policy, and NEO-81 handoff. |
+| Full-stack epic decomposition | **Matches** — plan documents no client counterpart; HP visibility deferred to NEO-83/NEO-85. |
+| Manual QA | **N/A** — plan skip rationale holds (server-only internal store). |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Module doc status line** — In `docs/decomposition/modules/E5_M1_CombatRulesEngine.md`, update **Status** from “E5M1-05+ pending” to cite **E5M1-05 NEO-80 landed** (`ICombatEntityHealthStore` + lazy dummy init), matching prior NEO-76–78 landed pattern.
+
+2. **Dependency register + alignment table** — Extend the **E5.M1 note** in `module_dependency_register.md` and the E5.M1 row in `documentation_and_implementation_alignment.md` with **NEO-80 landed:** `ICombatEntityHealthStore` / `InMemoryCombatEntityHealthStore`, `PrototypeCombatConstants.MaxPrototypeTargetHp = 100`, DI via `AddCombatEntityHealthStore()`.
+
+3. **E5M1 backlog landed note** — In `docs/plans/E5M1-prototype-backlog.md` § E5M1-05, check AC boxes and add a one-line landed note with link to [NEO-80 implementation plan](../plans/NEO-80-implementation-plan.md) (same pattern as E5M1-03/04).
+
+4. **`TryApplyDamage` deny coverage** — Plan Tests table lists “Deny: unknown id, empty id, negative damage.” Negative damage and empty id are covered only on `TryGet`; add `TryApplyDamage` false-path tests for unknown and empty ids so both mutation entry points are symmetrically guarded.
+
+## Nits
+
+- Nit: No test for case-variant ids (e.g. `"Prototype_Target_Alpha"`) — normalization is implemented via `Trim().ToLowerInvariant()` but untested; low risk given registry keys are lowercase constants.
+
+- Nit: `TryResetToFull` deny path (unknown id) is untested; happy path and `TryGet` deny are covered.
+
+- Nit: `AddCombatEntityHealthStore()` is chained inside `AddAbilityDefinitionCatalog` — acceptable per plan, but couples unrelated concerns; a future `AddCombatServices()` aggregator may be cleaner when NEO-81+ adds more combat DI.
+
+- Nit: `TryGet_OnBeta_ShouldLazyInitIndependentlyFromAlpha` performs two `TryGet` calls in **Act** (beta + alpha read); alpha state could be asserted via a prior snapshot from Arrange instead — AAA style only.
+
+## Verification
+
+```bash
+cd /home/don/neon-sprawl/server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~CombatEntityHealthStoreTests"
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+Manual: **N/A** — no HTTP or client surface in this story.