From 3134bb6ff6d57a94be0b0cc16dca409c42a6a744 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 25 May 2026 17:28:46 -0400 Subject: [PATCH] =?UTF-8?q?NEO-89:=20Address=20code=20review=20=E2=80=94?= =?UTF-8?q?=20E5.M2=20module=20doc=20and=20case-id=20test.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../modules/E5_M2_NpcAiAndBehaviorProfiles.md | 6 +++++- docs/plans/NEO-89-implementation-plan.md | 2 +- docs/reviews/2026-05-25-NEO-89.md | 8 ++++---- .../Npc/NpcBehaviorDefinitionRegistryTests.cs | 16 ++++++++++++++++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md index 0f3eaf8..5f37af3 100644 --- a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md +++ b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md @@ -75,7 +75,11 @@ The **first shipped three-archetype NPC spine** under `content/npc-behaviors/*.j **NPC instance ids (E5M2-05):** **`prototype_npc_melee`**, **`prototype_npc_ranged`**, **`prototype_npc_elite`** — each binds one behavior def + world anchor (replaces **`prototype_target_alpha` / `beta`**). -**Rules:** Do **not** rename frozen behavior **`id`** values without a migration issue. CI ([`scripts/validate_content.py`](../../../scripts/validate_content.py)) and server startup ([NEO-88](../../plans/NEO-88-implementation-plan.md)) enforce **exactly** these three behavior ids, duplicate-`id` rejection, and **`leashRadius` > `aggroRadius`** per row. Relaxing or changing that gate belongs in a new Linear issue once the catalog intentionally grows. Catalog + CI plan: [NEO-87 implementation plan](../../plans/NEO-87-implementation-plan.md). +**Rules:** Do **not** rename frozen behavior **`id`** values without a migration issue. CI ([`scripts/validate_content.py`](../../../scripts/validate_content.py)) and server startup ([NEO-88](../../plans/NEO-88-implementation-plan.md)) enforce **exactly** these three behavior ids, duplicate-`id` rejection, and **`leashRadius` > `aggroRadius`** per row. Relaxing or changing that gate belongs in a new Linear issue once the catalog intentionally grows. Catalog + CI plan: [NEO-87 implementation plan](../../plans/NEO-87-implementation-plan.md). **`INpcBehaviorDefinitionRegistry`** + DI **landed** ([NEO-89](../../plans/NEO-89-implementation-plan.md), E5M2-03) — game code and HTTP read models should inject the registry, not **`NpcBehaviorDefinitionCatalog`**. + +**Server load (NEO-88):** On host startup, `server/NeonSprawl.Server/Game/Npc/` loads `content/npc-behaviors/*_npc_behaviors.json` with the same validation gates as CI and **refuses to listen** when the catalog is invalid. Config and discovery: [server README — NPC behavior catalog](../../../server/README.md#npc-behavior-catalog-contentnpc-behaviors-neo-88). Plan: [NEO-88 implementation plan](../../plans/NEO-88-implementation-plan.md). + +**NPC behavior definition registry (NEO-89):** **`INpcBehaviorDefinitionRegistry`** in `server/NeonSprawl.Server/Game/Npc/` wraps the startup-loaded catalog for read-only lookup by stable behavior `id` (`TryGetDefinition`), trim/lowercase validation (`TryNormalizeKnown`), and ordered enumeration (`GetDefinitionsInIdOrder`). Stable prototype id constants: **`PrototypeNpcBehaviorRegistry`**. **NEO-90+** (HTTP definitions, instance registry, runtime tick) should inject the interface rather than `NpcBehaviorDefinitionCatalog`. Plan: [NEO-89 implementation plan](../../plans/NEO-89-implementation-plan.md). ## Risks and telemetry diff --git a/docs/plans/NEO-89-implementation-plan.md b/docs/plans/NEO-89-implementation-plan.md index 92cf127..541b4ac 100644 --- a/docs/plans/NEO-89-implementation-plan.md +++ b/docs/plans/NEO-89-implementation-plan.md @@ -55,7 +55,7 @@ - **`INpcBehaviorDefinitionRegistry`** + **`NpcBehaviorDefinitionRegistry`** registered in **`AddNpcBehaviorDefinitionCatalog`**; catalog-only eager resolve unchanged in **`Program.cs`**. - **`PrototypeNpcBehaviorRegistry`** — three frozen behavior id constants for tests/fixtures. -- **11** new AAA tests in **`NpcBehaviorDefinitionRegistryTests`**; **27** total Npc-filter tests green (includes existing loader tests). +- **12** new AAA tests in **`NpcBehaviorDefinitionRegistryTests`** (includes case-variant **`TryGetDefinition`** deny); **28** total Npc-filter tests green (includes existing loader tests). ## Technical approach diff --git a/docs/reviews/2026-05-25-NEO-89.md b/docs/reviews/2026-05-25-NEO-89.md index e3892a6..300fe46 100644 --- a/docs/reviews/2026-05-25-NEO-89.md +++ b/docs/reviews/2026-05-25-NEO-89.md @@ -18,7 +18,7 @@ NEO-89 delivers **E5M2-03**: a thin **`INpcBehaviorDefinitionRegistry`** adapter |----------|--------| | [`docs/plans/NEO-89-implementation-plan.md`](../plans/NEO-89-implementation-plan.md) | **Matches** — kickoff decisions, scope, acceptance checklist, reconciliation (11 tests, DI pattern, no `Program.cs` eager registry resolve). | | [`docs/plans/E5M2-prototype-backlog.md`](../plans/E5M2-prototype-backlog.md) · **E5M2-03** | **Matches** — AC checkboxes + landed note. | -| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md) | **Partially matches** — Status/Linear rows cite NEO-89; freeze **Rules** still only cite CI + server startup (NEO-88), not registry lookup surface (see Suggestion 1). | +| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md) | **Matches** — Status/Linear rows cite NEO-89; freeze **Rules** + NEO-88/NEO-89 implementation blurbs (review follow-up). | | [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E5.M2** | **Matches** — NEO-89 landed note added. | | [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M2 note cites NEO-89 registry + DI. | | [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server-only read registry; no runtime or client mutation. | @@ -35,13 +35,13 @@ None. ## Suggestions -1. **E5.M2 freeze Rules paragraph** — In [`E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md), extend the **Rules** sentence to cite **`INpcBehaviorDefinitionRegistry`** + NEO-89 as the preferred game lookup path (mirror E5.M1 freeze rules citing **`IAbilityDefinitionRegistry`** / NEO-79 after NEO-77 server load). +1. ~~**E5.M2 freeze Rules paragraph** — In [`E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md), extend the **Rules** sentence to cite **`INpcBehaviorDefinitionRegistry`** + NEO-89 as the preferred game lookup path (mirror E5.M1 freeze rules citing **`IAbilityDefinitionRegistry`** / NEO-79 after NEO-77 server load).~~ **Done.** -2. **Optional E5.M2 implementation blurb** — Add a short “NPC behavior definition registry (NEO-89)” paragraph under the module doc (same style as E3.M3’s **`IItemDefinitionRegistry`** section) so future NEO-90+ stories have a single anchor for the contract without rereading the plan. +2. ~~**Optional E5.M2 implementation blurb** — Add a short “NPC behavior definition registry (NEO-89)” paragraph under the module doc (same style as E3.M3’s **`IItemDefinitionRegistry`** section) so future NEO-90+ stories have a single anchor for the contract without rereading the plan.~~ **Done.** ## Nits -- Nit: No dedicated unit test that **`TryGetDefinition`** rejects case-variant ids (e.g. `"Prototype_Melee_Pressure"`) — fail-closed by design and covered indirectly via **`TryNormalizeKnown`**; optional explicit test if NEO-90 HTTP validation wants to document the split. +- ~~Nit: No dedicated unit test that **`TryGetDefinition`** rejects case-variant ids (e.g. `"Prototype_Melee_Pressure"`) — fail-closed by design and covered indirectly via **`TryNormalizeKnown`**; optional explicit test if NEO-90 HTTP validation wants to document the split.~~ **Done.** - Nit: **`TryGetDefinition`** for **`prototype_ranged_control`** metadata is only asserted in the loader-fixture and host tests, not a standalone unit test like melee/elite — acceptable given loader coverage. diff --git a/server/NeonSprawl.Server.Tests/Game/Npc/NpcBehaviorDefinitionRegistryTests.cs b/server/NeonSprawl.Server.Tests/Game/Npc/NpcBehaviorDefinitionRegistryTests.cs index 0f28a8e..692a5be 100644 --- a/server/NeonSprawl.Server.Tests/Game/Npc/NpcBehaviorDefinitionRegistryTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Npc/NpcBehaviorDefinitionRegistryTests.cs @@ -125,6 +125,22 @@ public class NpcBehaviorDefinitionRegistryTests Assert.Null(def); } + [Fact] + public void TryGetDefinition_ShouldReturnFalse_WhenIdHasMixedCase() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal) + { + [PrototypeNpcBehaviorRegistry.PrototypeMeleePressure] = MeleeRow(), + }; + var registry = CreateRegistryFromRows(rows); + // Act + var found = registry.TryGetDefinition("Prototype_Melee_Pressure", out var def); + // Assert + Assert.False(found); + Assert.Null(def); + } + [Fact] public void TryNormalizeKnown_ShouldReturnTrueAndLowercaseId_WhenInputHasWhitespaceAndMixedCase() {