NEO-91: add code review for NPC instance registry migration
Co-authored-by: Cursor <cursoragent@cursor.com>pull/128/head
Don
parent
fa9acc85ac
commit
d14879ac75
|
|
@ -0,0 +1,73 @@
|
||||||
|
# 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) | **Partially matches** — NPC instance ids bullet added; **Status** / **Linear** summary rows still stop at NEO-90 (see Suggestions). |
|
||||||
|
| [`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 still cites flat **`PrototypeCombatConstants.MaxPrototypeTargetHp (100)`** for NEO-80 (stale after this migration). |
|
||||||
|
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) · **E5.M2 note** | **Partially matches** — no NEO-91 / **`PrototypeNpcRegistry`** note yet (see Suggestions). |
|
||||||
|
| [`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.
|
||||||
|
|
||||||
|
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).
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
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”).
|
||||||
|
|
||||||
|
## 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).
|
||||||
|
|
||||||
|
- Nit: **`GetCombatTargets_ShouldShowDefeated_AfterFourPulsesOnMelee`** — Asserts melee defeated and ranged unchanged; optional elite-at-200 assertion matches the plan’s expected snapshot table.
|
||||||
|
|
||||||
|
- 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`).
|
||||||
Loading…
Reference in New Issue