74 lines
6.9 KiB
Markdown
74 lines
6.9 KiB
Markdown
# Code review — NEO-91 Prototype NPC instance registry + combat-target migration
|
||
|
||
**Date:** 2026-05-26
|
||
**Scope:** Branch `NEO-91-npc-instance-registry-combat-target-migration` · commits `dadc35a`–`fa9acc8` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-91 delivers **E5M2-05**: a frozen **`PrototypeNpcRegistry`** with three NPC combat instances (`prototype_npc_melee`, `prototype_npc_ranged`, `prototype_npc_elite`), each bound to a behavior def and world anchor. **`PrototypeTargetRegistry`** and **`PrototypeCombatConstants`** are removed; **`InMemoryCombatEntityHealthStore`** now gates on the NPC registry and resolves per-instance **`maxHp`** via **`INpcBehaviorDefinitionRegistry`**. Targeting, cast, combat-targets GET, and the dev fixture reset all enumerate the three ids. Server tests and Bruno smokes are migrated; the melee 4× pulse defeat spine remains the AC verification path. Client constants are intentionally unchanged (breaking change documented for **NEO-97**). Implementation is coherent, thread-safe, and aligned with kickoff decisions. Risk is moderate only because of the breaking id change until NEO-97 lands — appropriately scoped and documented.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-91-implementation-plan.md`](../plans/NEO-91-implementation-plan.md) | **Matches** — kickoff table, three-instance bindings, per-archetype HP table, acceptance checklist, reconciliation section. |
|
||
| [`docs/plans/E5M2-prototype-backlog.md`](../plans/E5M2-prototype-backlog.md) · **E5M2-05** | **Matches** — AC checkboxes + landed note. |
|
||
| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md) | **Matches** — NPC instance ids bullet added; **Status** / **Linear** summary rows cite **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91). |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E5.M2** | **Matches** — NEO-91 landed note with README link. **E5.M1** row NEO-80 blurb notes NEO-91 supersession of flat HP / dummy ids. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) · **E5.M2 note** | **Matches** — NEO-91 / **`PrototypeNpcRegistry`** note appended. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-owned HP and target validity unchanged in authority model; id migration is server contract change only. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — three NPC ids, per-archetype max HP, breaking change + NEO-97 cross-link. |
|
||
| Full-stack epic decomposition | **Matches** — server-only story; plan and README explicitly defer Godot to NEO-97; no false prototype-complete claim. |
|
||
| Manual QA | **N/A** — plan documents skip (`Bruno` + automated tests only). |
|
||
|
||
Register/tracking: E5.M2 **In Progress** with NEO-91 in alignment table is correct; module register and E5.M2 Summary **Status** row should be extended (Suggestions).
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Add legacy-id targeting integration test**~~ — Plan test table calls for `TargetingApiTests`: select melee in range + **`prototype_target_alpha` → `unknown_target`**. Registry and health-store unit tests cover legacy rejection; add an HTTP-level test so tab-cycle / client migration regressions are caught at the API boundary. **Done.**
|
||
|
||
2. ~~**Update `module_dependency_register.md` E5.M2 note**~~ — Append **NEO-91 landed:** **`PrototypeNpcRegistry`** + combat-target migration (three instance ids, catalog max HP) with links to [NEO-91 plan](../plans/NEO-91-implementation-plan.md) and [server README — Combat entity health (NEO-91)](../../server/README.md#combat-entity-health-neo-80-neo-91) (same pattern as NEO-90 note). **Done.**
|
||
|
||
3. ~~**E5.M2 Summary table rows**~~ — In [`E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md), extend **Status** (line 10) and **Linear** (line 11) to cite **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) so they match the instance-id bullet and alignment table. **Done.**
|
||
|
||
4. ~~**Refresh stale Bruno `docs {}` blocks**~~ — Request bodies use new NPC ids, but several combat Bruno files still describe “alpha”/“beta” in prose (e.g. `combat-targets/Get combat targets after cast spine.bru`, `ability-cast/folder.bru`, `gig-progression/Get gig progression after defeat spine.bru`). Update docs to `prototype_npc_melee` / `prototype_npc_ranged` to avoid confusing manual runs. **Done.**
|
||
|
||
5. ~~**E5.M1 alignment row (NEO-80 blurb)**~~ — In `documentation_and_implementation_alignment.md`, the E5.M1 **NEO-80 landed** sentence still references **`PrototypeCombatConstants.MaxPrototypeTargetHp (100)`** and alpha/beta dummies. Add a forward note that NEO-91 superseded flat HP / dummy ids (or shorten the NEO-80 clause to “see NEO-91”). **Done.**
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: **`HostBehaviorRegistry_ShouldResolveEachInstanceBehaviorDef`**~~ — `Assert.True` / `Assert.Equal` run inside the `foreach` under `// Act`; move resolution to Act and assertions to `// Assert` (or use `[Theory]`) per [csharp-style AAA](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). **Done.**
|
||
|
||
- ~~Nit: **`GetCombatTargets_ShouldShowDefeated_AfterFourPulsesOnMelee`**~~ — Asserts melee defeated and ranged unchanged; optional elite-at-200 assertion matches the plan’s expected snapshot table. **Done.**
|
||
|
||
- Nit: **`GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnMelee`** — `ReadFromJsonAsync` for the GET body runs in Act before `// Assert`; minor AAA blur (acceptable pattern elsewhere in repo).
|
||
|
||
- Nit: **`TryResolveMaxHp`** silently returns false when a registry binding points at a missing behavior def; **`HostBehaviorRegistry_ShouldResolveEachInstanceBehaviorDef`** mitigates at integration level. Optional fail-fast startup validation (mirror catalog load) if instance bindings become editable later.
|
||
|
||
- Nit: Bruno defeat spine does not assert elite unchanged (200/200); plan deferred elite pulse spine — consistent.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
|
||
--filter "FullyQualifiedName~Npc|FullyQualifiedName~CombatTarget|FullyQualifiedName~CombatEntityHealth|FullyQualifiedName~Targeting|FullyQualifiedName~AbilityCast"
|
||
```
|
||
|
||
Confirmed during review: **96** filtered tests passed.
|
||
|
||
Postgres/Docker integration tests (11) fail in this environment without Docker — unrelated to NEO-91.
|
||
|
||
Optional: run Bruno `combat-targets/` folder against a dev server; **`Get combat targets after cast spine.bru`** is the AC defeat spine.
|
||
|
||
Before playable Godot verification: merge **NEO-97** client constant migration (`client/scripts/prototype_target_constants.gd`).
|