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-25-NEO-83.md

5.5 KiB
Raw Permalink Blame History

Code review — NEO-83 GET /game/world/combat-targets HP snapshot

Date: 2026-05-25
Scope: Branch NEO-83-combat-targets-snapshot · commits 902e038–52b1d4d vs origin/main
Base: origin/main

Follow-up: Code review suggestions and nits addressed in follow-up commit on NEO-83-combat-targets-snapshot.

Verdict

Approve with nits

Summary

NEO-83 delivers E5M1-08: a read-only GET /game/world/combat-targets route backed by ICombatEntityHealthStore, returning schemaVersion 1 and a targets array (targetId, maxHp, currentHp, defeated) for both prototype dummies in ascending id order. Handler flow mirrors NEO-78 world GET patterns; PrototypeTargetRegistry.GetPrototypeTargetIdsInOrder() centralizes enumeration. Three AAA integration tests cover fresh snapshot, post-cast damage reflection, and four-pulse defeat; Bruno happy GET + cast→GET defeat spine (with HP stepping assertions on cast bodies) satisfy AC. Module tracking docs, E5M1 backlog, and server/README.md were updated in the implementation commit — better doc sync than prior E5.M1 slices. Residual risk is low — server-only read projection with no client change (NEO-85 owns HUD).

Documentation checked

Document Result
docs/plans/NEO-83-implementation-plan.md Matches — kickoff decisions, scope, acceptance checklist, expected snapshot table, and reconciliation align with code.
docs/plans/E5M1-prototype-backlog.md · E5M1-08 Matches — AC checked, landed note added.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md Matches — Status cites E5M1-08 NEO-83 landed; Related implementation slices entry added.
docs/decomposition/modules/module_dependency_register.md Matches — E5.M1 note includes NEO-83 landed.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M1 row cites combat-targets GET + NEO-83 plan link.
docs/decomposition/modules/client_server_authority.md Matches — server-owned HP snapshot; no client-side damage math.
docs/decomposition/modules/pvp_combat_integration.md N/A — prototype NPC targets only.
server/README.md Matches — combat-targets section, field table, curl example; NEO-80 HTTP read row updated.
Full-stack epic decomposition Matches — plan documents NEO-85 client counterpart; no Godot change in scope.
Manual QA N/A — kickoff skip; Bruno + README + API tests sufficient.

Blocking issues

None.

Suggestions

  1. Silent omission on TryGet miss — In CombatTargetsWorldApi, registry ids that fail healthStore.TryGet are skipped via continue, which could return a partial targets array without error. For prototype registry ids, InMemoryCombatEntityHealthStore.TryGet always lazy-inits and succeeds; consider removing the skip or logging/asserting in dev so a store regression cannot silently drop HUD rows. Done. — TryGet miss throws InvalidOperationException instead of skipping.

  2. Bruno one-pulse GET smoke (optional) — Plan §7 described a spine asserting alpha currentHp 75 after one pulse; implementation consolidates into the four-pulse defeat spine only. Integration test GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha covers the gap — optional separate Bruno request if manual QA wants a fast (~0s) cast→GET check without cooldown waits. Done. — Added Get combat targets after one cast.bru (seq 2); defeat spine moved to seq 3.

Nits

  • Nit: BindSlot0PulseAsync / LockPrototypeTargetAlphaAsync / PulseCastRequest duplicate helpers in AbilityCastApiTests — acceptable for story scope; optional shared test fixture if a third combat cast consumer appears. Deferred — no third consumer yet.

  • Nit: GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha uses EnsureSuccessStatusCode on cast but does not assert combatResolution.targetRemainingHp == 75 in Arrange — would tighten cast→GET coherence (mirror defeat test’s cast-body checks). Done. — Assert block now verifies cast combatResolution.targetRemainingHp before GET row checks.

  • Nit: Plan §6 mentions teleport-in-range setup like some Bruno flows; cast tests on this branch omit move/teleport and still pass (same as AbilityCastApiTests default) — no action unless range gates tighten. No action — per review.

Verification

cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~CombatTargetsWorldApiTests"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

CombatTargetsWorldApiTests: 3/3 passed (local run).

Full suite: unrelated failures observed locally (e.g. crafting/inventory tests — flaky or environment-specific; not touched by this diff). Confirm CI on PR before merge.

Bruno (manual): Run bruno/neon-sprawl-server/combat-targets/ — seq 1 fresh GET; seq 2 one-pulse cast→GET (~0s); seq 3 four-pulse defeat spine (~13s pre-request + HP stepping assertions).