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

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
Follow-up: Code review suggestions addressed (module/alignment docs, E5M1 backlog landed note, TryApplyDamage deny tests, normalization + reset deny tests, AAA fix on beta independence test).

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	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 · E5M1-05	Matches — AC checkboxes checked; landed note with plan link added.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md	Matches — Status cites NEO-80 landed; related implementation slice added.
docs/decomposition/modules/module_dependency_register.md	Matches — E5.M1 note includes NEO-80 health store.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E5.M1 row includes NEO-80 landed.
docs/decomposition/modules/client_server_authority.md	N/A — server-only in-memory store; no client or HTTP changes.
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. Done. Status + related implementation slice updated.

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(). Done.

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 (same pattern as E5M1-03/04). Done.

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. Done. Added unknown/empty deny tests plus case normalization and TryResetToFull deny tests.

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. Done. TryGet_ShouldNormalizeCaseVariantTargetIds.

• Nit: TryResetToFull deny path (unknown id) is untested; happy path and TryGet deny are covered. Done. TryResetToFull_ShouldReturnFalse_WhenTargetIdIsUnknown.

• 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. Deferred — documented in plan; revisit with NEO-81 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. Done. Alpha HP asserted from damage snapshot in Arrange.

Verification

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.