From 035e9159341d9185c5cdbcb3de64705625831a7c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 4 May 2026 19:53:53 -0400 Subject: [PATCH] NEO-35: address NEO-35 code review (AAA, docs, null id, cleanup) --- ...umentation_and_implementation_alignment.md | 2 +- docs/reviews/2026-05-04-NEO-35.md | 45 +++++++++++++++++++ .../Skills/SkillDefinitionRegistryTests.cs | 26 ++++++++--- .../Game/Skills/ISkillDefinitionRegistry.cs | 4 +- .../Game/Skills/SkillDefinitionRegistry.cs | 8 +++- 5 files changed, 75 insertions(+), 10 deletions(-) create mode 100644 docs/reviews/2026-05-04-NEO-35.md diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index a5821c7..06c9950 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -50,7 +50,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not | E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) | | E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **NEO-27 landed:** Slice 3 telemetry hook sites documented — `selection_event` maps to product `target_changed` in `target_selection_client.gd`; server lock transition hook comments in `InMemoryPlayerTargetLockStore`; reserved `ability_cast_requested` / `ability_cast_denied` comments in `client/scripts/main.gd` (all TODO(E9.M1)); see [NEO-27](../../plans/NEO-27-implementation-plan.md) and [`docs/manual-qa/NEO-27.md`](../../manual-qa/NEO-27.md). **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`; production telemetry ingest/catalog after E9.M1. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) | | E1.M4 | In Progress | **NEO-29 landed:** prototype `HotbarLoadout` v1 server contract (`GET`/`POST /game/players/{id}/hotbar-loadout`) with stable deny reason codes, per-player scope, and persistence policy matching NEO-8/NS-17 (Postgres when configured, in-memory fallback otherwise). Client boot hydration is wired via `hotbar_loadout_client.gd` + `hotbar_state.gd` from `main.gd`; manual QA checklist created. **NEO-31 landed:** prototype **`POST /game/players/{id}/ability-cast`**, digit-key cast wiring from `main.gd`, `ability_cast_client.gd`, and `hotbar_cast_slot_resolver.gd` (target id from `lockedTargetId` in cached target state); GdUnit covers resolver + HTTP client; manual QA [NEO-31](../../manual-qa/NEO-31.md). **NEO-28 landed:** cast POST validates **lock + registry + range** (`invalid_target`, `out_of_range`); **`AbilityCastClient.cast_result_received`** + **`CastFeedbackLabel`** HUD line; manual QA [NEO-28](../../manual-qa/NEO-28.md). **NEO-30 landed:** Slice 3 cast funnel telemetry **hook-site comments** in [`AbilityCastApi.cs`](../../../server/NeonSprawl.Server/Game/AbilityInput/AbilityCastApi.cs) (`ability_cast_requested` on authoritative accept, `ability_cast_denied` + non-empty `reasonCode` on each JSON deny branch); client dev hooks unchanged; manual QA [NEO-30](../../manual-qa/NEO-30.md); plan [NEO-30](../../plans/NEO-30-implementation-plan.md). **NEO-32 landed:** `GET /game/players/{id}/cooldown-snapshot` + `on_cooldown` cast deny + prototype global cooldown commit; [`cooldown_snapshot_client.gd`](../../../client/scripts/cooldown_snapshot_client.gd), [`cooldown_state.gd`](../../../client/scripts/cooldown_state.gd), **`CooldownSlotsLabel`** in [`main.gd`](../../../client/scripts/main.gd); manual QA [NEO-32](../../manual-qa/NEO-32.md); plan [NEO-32](../../plans/NEO-32-implementation-plan.md). | [NEO-29](../../plans/NEO-29-implementation-plan.md), [NEO-31](../../plans/NEO-31-implementation-plan.md), [NEO-28](../../plans/NEO-28-implementation-plan.md), [NEO-30](../../plans/NEO-30-implementation-plan.md), [NEO-32](../../plans/NEO-32-implementation-plan.md); [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md); [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); `server/NeonSprawl.Server/Game/AbilityInput/`; [`client/scripts/hotbar_loadout_client.gd`](../../../client/scripts/hotbar_loadout_client.gd), [`client/scripts/hotbar_state.gd`](../../../client/scripts/hotbar_state.gd), [`client/scripts/ability_cast_client.gd`](../../../client/scripts/ability_cast_client.gd), [`client/scripts/hotbar_cast_slot_resolver.gd`](../../../client/scripts/hotbar_cast_slot_resolver.gd), [`client/scripts/cooldown_snapshot_client.gd`](../../../client/scripts/cooldown_snapshot_client.gd), [`client/scripts/cooldown_state.gd`](../../../client/scripts/cooldown_state.gd); [server README — Hotbar loadout](../../../server/README.md#hotbar-loadout-neo-29), [Ability cast (NEO-31)](../../../server/README.md#ability-cast-neo-31), [Cooldown snapshot (NEO-32)](../../../server/README.md#cooldown-snapshot-neo-32) | -| E2.M1 | In Progress | **NEO-33 landed:** frozen prototype trio **`salvage`** / **`refine`** / **`intrusion`** in [`content/skills/prototype_skills.json`](../../../content/skills/prototype_skills.json); PR gate + [`validate_content.py`](../../../scripts/validate_content.py) enforce schema, duplicate `id`, exact trio ids, and category coverage; designer note on **`allowedXpSourceKinds`** in [E2_M1](E2_M1_SkillDefinitionRegistry.md) + [`content/README.md`](../../../content/README.md). **NEO-34 landed:** fail-fast server load of `content/skills/*.json` at startup — `server/NeonSprawl.Server/Game/Skills/` ([NEO-34](../../plans/NEO-34-implementation-plan.md)); config + discovery in [server README — Skill catalog](../../../server/README.md#skill-catalog-contentskills-neo-34). **Follow-on:** registry service ([NEO-35](https://linear.app/neon-sprawl/issue/NEO-35)), read HTTP + Bruno ([NEO-36](https://linear.app/neon-sprawl/issue/NEO-36)). | [NEO-33](../../plans/NEO-33-implementation-plan.md), [NEO-34](../../plans/NEO-34-implementation-plan.md); [E2_M1](E2_M1_SkillDefinitionRegistry.md); [module_dependency_register.md](module_dependency_register.md) **E2.M1 note**; `server/NeonSprawl.Server/Game/Skills/` | +| E2.M1 | In Progress | **NEO-33 landed:** frozen prototype trio **`salvage`** / **`refine`** / **`intrusion`** in [`content/skills/prototype_skills.json`](../../../content/skills/prototype_skills.json); PR gate + [`validate_content.py`](../../../scripts/validate_content.py) enforce schema, duplicate `id`, exact trio ids, and category coverage; designer note on **`allowedXpSourceKinds`** in [E2_M1](E2_M1_SkillDefinitionRegistry.md) + [`content/README.md`](../../../content/README.md). **NEO-34 landed:** fail-fast server load of `content/skills/*.json` at startup — `server/NeonSprawl.Server/Game/Skills/` ([NEO-34](../../plans/NEO-34-implementation-plan.md)); config + discovery in [server README — Skill catalog](../../../server/README.md#skill-catalog-contentskills-neo-34). **NEO-35 landed:** `ISkillDefinitionRegistry` / `SkillDefinitionRegistry` + DI ([NEO-35](../../plans/NEO-35-implementation-plan.md)). **Follow-on:** read HTTP + Bruno ([NEO-36](https://linear.app/neon-sprawl/issue/NEO-36)). | [NEO-33](../../plans/NEO-33-implementation-plan.md), [NEO-34](../../plans/NEO-34-implementation-plan.md), [NEO-35](../../plans/NEO-35-implementation-plan.md); [E2_M1](E2_M1_SkillDefinitionRegistry.md); [module_dependency_register.md](module_dependency_register.md) **E2.M1 note**; `server/NeonSprawl.Server/Game/Skills/` | --- diff --git a/docs/reviews/2026-05-04-NEO-35.md b/docs/reviews/2026-05-04-NEO-35.md new file mode 100644 index 0000000..ef148ff --- /dev/null +++ b/docs/reviews/2026-05-04-NEO-35.md @@ -0,0 +1,45 @@ +# Code review: NEO-35 (SkillDefinitionRegistry) + +**Date:** 2026-05-04 + +**Scope:** Branch `NEO-35-e2m1-skilldefinition-registry` vs `origin/main` (ahead 2 commits): `ISkillDefinitionRegistry`, `SkillDefinitionRegistry`, DI registration, `SkillDefinitionRegistryTests`, plan `docs/plans/NEO-35-implementation-plan.md`, comment tweak on `SkillDefinitionCatalog`. + +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +The change adds a thin registry adapter over `SkillDefinitionCatalog` with `TryGetDefinition`, ordered enumeration, XML guidance for E2.M2 and NEO-36, and singleton DI registration. Unit tests cover happy path, unknown id, id-ordered listing, loader-backed consistency, and host DI resolution. Behavior matches the NEO-35 plan and E2.M1 module intent for this slice (read-only metadata; grant enforcement remains E2.M2). `dotnet test` on `NeonSprawl.Server.Tests` passes (110 tests). + +## Documentation checked + +| Document | Result | +|----------|--------| +| `docs/plans/NEO-35-implementation-plan.md` | **Matches** — interface + adapter + DI + tests align with acceptance checklist and technical approach (`TryGet*` semantics, `SkillDefRow`, enumeration, AAA tests). | +| `docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md` | **Matches** — exposes stable ids, categories, display names, `allowedXpSourceKinds`; documents E2.M2 validation expectation on the interface; does not implement unlock gating or XP engine (correctly out of scope). | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E2.M1 row describes registry / `allowedXpSourceKinds`; implementation fits. | +| `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` (cross-read) | **Matches** — interface remarks align with “reject grants when `sourceKind` not listed.” | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E2.M1 snapshot row updated for NEO-35 landed. | + +## Blocking issues + +(none) + +## Suggestions + +1. ~~**`Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` AAA (`SkillDefinitionRegistryTests.cs`)** — `// Act` currently performs `GetAsync("/health")`, `GetRequiredService()`, and `TryGetDefinition` together. Per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert), Act should be the behavior under test only (typically one call, or a short intentional sequence for one scenario). Prefer moving `/health` to **Arrange** as a startup warm-up if ordering matters, dropping it if `CreateClient()` already builds the host enough for the catalog, or splitting host-health vs DI-resolution into separate tests so each method has a single clear Act.~~ **Done.** + +2. ~~**Post-merge doc hygiene** — Update `documentation_and_implementation_alignment.md` E2.M1 / NEO-35 line when this story merges so the implementation tracking table matches reality.~~ **Done.** + +## Nits + +1. ~~**Nit:** `finally` block uses an empty `catch { }` around `Directory.Delete` in `TryGetDefinition_ShouldMatchLoaderCatalog_WhenUsingPrototypeFixture`. [csharp-style](../../.cursor/rules/csharp-style.md) prefers avoiding empty catches; for tests, catching a specific exception type (or a tiny helper) is enough if you want to silence flaky cleanup on Windows/locked files.~~ **Done.** + +2. ~~**Nit:** `TryGetDefinition` with a `null` `skillId` will throw from the underlying dictionary lookup; the XML says misses do not throw. That is fine for normal string ids; if you want the contract to include null as “not found,” add an explicit guard—only worth it if public API should treat null like unknown.~~ **Done.** + +## Verification + +- `dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj` — all tests passed at review time. diff --git a/server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs b/server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs index 6d20643..77141cc 100644 --- a/server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs @@ -1,4 +1,4 @@ -using System.Net; +using System.IO; using System.Text; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; @@ -40,6 +40,22 @@ public class SkillDefinitionRegistryTests Assert.Contains("book_or_item", def.AllowedXpSourceKinds, StringComparer.Ordinal); } + [Fact] + public void TryGetDefinition_ShouldReturnFalse_WhenSkillIdIsNull() + { + // Arrange + var rows = new Dictionary(StringComparer.Ordinal) + { + ["salvage"] = new SkillDefRow("salvage", "gather", "Salvage", new[] { "activity" }), + }; + var registry = CreateRegistryFromRows(rows); + // Act + var found = registry.TryGetDefinition(null, out var def); + // Assert + Assert.False(found); + Assert.Null(def); + } + [Fact] public void TryGetDefinition_ShouldReturnFalse_WhenIdUnknown() { @@ -130,9 +146,9 @@ public class SkillDefinitionRegistryTests { Directory.Delete(root.FullName, recursive: true); } - catch + catch (IOException) { - // best-effort cleanup for temp dir + // Best-effort: transient lock or race on some hosts; temp dir is unique per run. } } } @@ -143,13 +159,11 @@ public class SkillDefinitionRegistryTests // Arrange await using var factory = new InMemoryWebApplicationFactory(); using var client = factory.CreateClient(); + _ = await client.GetAsync("/health"); // Act - var response = await client.GetAsync("/health"); var registry = factory.Services.GetRequiredService(); var found = registry.TryGetDefinition("salvage", out var salvage); - var status = response.StatusCode; // Assert - Assert.Equal(HttpStatusCode.OK, status); Assert.True(found); Assert.NotNull(salvage); Assert.Contains("activity", salvage.AllowedXpSourceKinds, StringComparer.Ordinal); diff --git a/server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs b/server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs index ef7cfb8..40a6c42 100644 --- a/server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs +++ b/server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs @@ -13,8 +13,8 @@ namespace NeonSprawl.Server.Game.Skills; /// public interface ISkillDefinitionRegistry { - /// Attempts to resolve a skill by stable id (see skill-def.schema.json). Misses do not throw. - bool TryGetDefinition(string skillId, [NotNullWhen(true)] out SkillDefRow? definition); + /// Attempts to resolve a skill by stable id (see skill-def.schema.json). Unknown ids and null return false without throwing. + bool TryGetDefinition(string? skillId, [NotNullWhen(true)] out SkillDefRow? definition); /// Every loaded definition, ordered by (ordinal). IReadOnlyList GetDefinitionsInIdOrder(); diff --git a/server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs b/server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs index d929af0..fca38d2 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs @@ -6,8 +6,14 @@ namespace NeonSprawl.Server.Game.Skills; public sealed class SkillDefinitionRegistry(SkillDefinitionCatalog catalog) : ISkillDefinitionRegistry { /// - public bool TryGetDefinition(string skillId, [NotNullWhen(true)] out SkillDefRow? definition) + public bool TryGetDefinition(string? skillId, [NotNullWhen(true)] out SkillDefRow? definition) { + if (skillId is null) + { + definition = null; + return false; + } + if (catalog.ById.TryGetValue(skillId, out var row)) { definition = row;