From aafa5e472a9583625b8993eb5e700ee02682aa08 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 25 May 2026 11:22:13 -0400 Subject: [PATCH] NEO-83: Add code review for combat-targets GET snapshot. --- docs/reviews/2026-05-25-NEO-83.md | 61 +++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 docs/reviews/2026-05-25-NEO-83.md diff --git a/docs/reviews/2026-05-25-NEO-83.md b/docs/reviews/2026-05-25-NEO-83.md new file mode 100644 index 0000000..f4c444d --- /dev/null +++ b/docs/reviews/2026-05-25-NEO-83.md @@ -0,0 +1,61 @@ +# 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` + +## 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`](../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`](../plans/E5M1-prototype-backlog.md) · **E5M1-08** | **Matches** — AC checked, landed note added. | +| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../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`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M1 note includes NEO-83 landed. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../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`](../decomposition/modules/client_server_authority.md) | **Matches** — server-owned HP snapshot; no client-side damage math. | +| [`docs/decomposition/modules/pvp_combat_integration.md`](../decomposition/modules/pvp_combat_integration.md) | **N/A** — prototype NPC targets only. | +| [`server/README.md`](../../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. + +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. + +## Nits + +- Nit: **`BindSlot0PulseAsync` / `LockPrototypeTargetAlphaAsync` / `PulseCastRequest`** duplicate helpers in `AbilityCastApiTests` — acceptable for story scope; optional shared test fixture if a third combat cast consumer appears. + +- 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). + +- 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. + +## Verification + +```bash +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 cast spine (~13s pre-request for four-pulse defeat + HP stepping assertions).