diff --git a/docs/reviews/2026-05-24-NEO-80.md b/docs/reviews/2026-05-24-NEO-80.md new file mode 100644 index 0000000..b6d85aa --- /dev/null +++ b/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.