don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-28-NEO-95.md

4.6 KiB
Raw Blame History

Code review — NEO-95 (E5M2-09)

Date: 2026-05-28
Scope: Branch NEO-95-npc-attack-resolve-player-hp vs origin/main — commits ac4e1df … b702d9f
Base: origin/main

Verdict

Approve with nits

Summary

NEO-95 delivers the server slice cleanly: IPlayerCombatHealthStore (lazy-init 100 HP, damage floor, dev reset) mirrors the NPC ICombatEntityHealthStore pattern; NpcAttackOperations.TryResolveTelegraphComplete re-checks the threat holder before applying catalog attackDamage; NpcRuntimeOperations.AdvanceAll invokes resolve at telegraph windup completion and passes the store from NpcRuntimeSnapshotWorldApi poll; GET /game/players/{id}/combat-health gates on position row (same as cooldown snapshot) and returns schema v1 JSON. Dev fixture reset restores player HP. Tests cover store unit behavior, resolve no-ops, runtime windup→damage wiring, integration elite timeline (FakeClock 7.5 s → 75 HP), fixture reset, and Bruno smoke with documented sleep(8000). Build succeeds; NEO-95-filtered tests pass (40/40). Server-only scope — client HUD correctly deferred to NEO-97.

Documentation checked

Path Result
docs/plans/NEO-95-implementation-plan.md Matches — AC checkboxes done; dedicated GET (kickoff), damage flow, holder re-check, fixture reset, tests, and file list align with the diff.
docs/plans/E5M2-prototype-backlog.md (E5M2-09) Matches — AC checked, landed note present; client counterpart NEO-97 called out.
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md Partially matches — NEO-95 implementation section + README/Bruno links present; Summary Status table still lists E5M2-09 without a “landed” marker (NEO-94 pattern uses “landed” inline).
docs/decomposition/modules/documentation_and_implementation_alignment.md Partially matches — E5.M2 row mentions IPlayerCombatHealthStore in “Owns” but landed bullets stop at NEO-94; NEO-95 not yet appended (same gap NEO-94 review closed for NEO-94).
docs/decomposition/modules/client_server_authority.md Matches — server owns NPC→player damage; client poll-only read model; no client damage math.
server/README.md Matches — session player combat HP section, GET route, fixture reset note, npc-runtime snapshot cross-link updated.

Blocking issues

None.

Suggestions

  1. Update documentation_and_implementation_alignment.md — Append NEO-95 landed to the E5.M2 tracking row (IPlayerCombatHealthStore, NpcAttackOperations, GET …/combat-health, Bruno combat-health/, plan/README links), matching NEO-87–NEO-94 entries. Listed implicitly by review checklist; not in the NEO-95 plan “Files to modify” table but keeps alignment doc truthful.

  2. E5.M2 Summary status table — Mark E5M2-09 / NEO-95 landed inline in the module Summary Status row (currently jumps from “NEO-94 runtime snapshot GET landed” to “NEO-95 NPC attack resolve + player combat HP → E5M2-12” without “landed”).

Nits

  • Nit: NpcBehaviorState.AttackExecute branch in NpcRuntimeOperations is unreachable in the normal windup→recover flow (NEO-93); harmless defensive path but could carry a one-line comment or be removed until a visible execute phase is needed.
  • Nit: PlayerCombatHealthApi.BuildResponse echoes normalized lowercase snapshot.PlayerId while CooldownSnapshotApi echoes the route {id} as provided — consistent for prototype lowercase ids; note if mixed-case route ids become a concern.
  • Nit: Elite integration test GetPlayerCombatHealth_ShouldDecreaseAfterEliteTelegraphComplete performs setup GETs in Arrange (acceptable for multi-poll timeline); baseline/mid snapshot reads could move to Assert if stricter AAA parity with NEO-94 follow-up is desired.

Verification

# Build + NEO-95-related tests (passed 2026-05-28)
dotnet build NeonSprawl.sln
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~PlayerCombatHealth|FullyQualifiedName~NpcAttackOperations|FullyQualifiedName~CombatTargetFixture|FullyQualifiedName~NpcRuntimeOperations"

# Full server suite (Postgres persistence tests require local DB)
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

# Bruno (CI runs entire bruno/neon-sprawl-server/)
# bruno/neon-sprawl-server/combat-health/

Manual: run Get combat health after elite attack.bru against dev server — after ~8 s wait, currentHp should be 75 with elite cast accepted on prototype_npc_elite.