don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-31-NEO-101.md

4.5 KiB
Raw Permalink Blame History

Code review — NEO-101 (E5M3-02)

Date: 2026-05-31
Scope: Branch NEO-101-encounter-reward-catalog-load-fail-fast vs 7c0e2e8 (merge-base on main) — commits 06c5899 … f921095
Base: 7c0e2e8 (main at branch point)

Verdict

Approve with nits

Summary

NEO-101 adds fail-fast startup loading for content/reward-tables/*_reward_tables.json and content/encounters/*_encounters.json, mirroring the established NEO-51 / NEO-66 / NEO-88 catalog pattern: path discovery, JsonSchema.Net row validation, duplicate-id rejection, prototype E5M3 allowlist gates, and cross-refs (item ids → item catalog; NPC instance sets → PrototypeNpcRegistry; rewardTableId → reward-table catalog). DI registers reward tables after items and encounters after reward tables; Program.cs eagerly resolves both singletons before Run(). Nineteen AAA loader + host boot tests pass locally. Docs (server/README.md, alignment register, module register, E5M3 backlog) are updated. Risk is low: server-only, no HTTP surface, no registry interfaces yet (correctly deferred to NEO-102).

Documentation checked

Path Result
docs/plans/NEO-101-implementation-plan.md Matches — shipped loaders, DI order, config keys, tests, README, alignment register; acceptance checklist checked; reconciliation section accurate.
docs/plans/E5M3-prototype-backlog.md (E5M3-02) Matches — landed note cites NEO-101 plan and 19 tests.
docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md Matches — freeze table and CI paragraph reference NEO-101 loader sync; Summary Status notes NEO-101 server load landed; Implementation snapshot (NEO-101) paragraph added.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M3 row notes NEO-101 server load; runtime NEO-102+ still outstanding.
docs/decomposition/modules/module_dependency_register.md Matches — E5.M3 note cites NEO-101 landed + README anchor.
docs/decomposition/modules/CT_M1_ContentValidationPipeline.md N/A — no server-load paragraph required for this story; CI gates unchanged.
Full-stack epic decomposition N/A — plan explicitly excludes client counterpart until NEO-110 / NEO-111.
server/README.md Matches — reward-table and encounter catalog sections document config keys, discovery, fail-fast, load order, NEO-102 deferral.

Blocking issues

None.

Suggestions

  1. Add Load_ShouldThrow_WhenRowViolatesSchema tests — The implementation plan test table lists schema violation for both loaders; peer catalog tests (ItemDefinitionCatalogLoaderTests, RecipeDefinitionCatalogLoaderTests, etc.) use empty displayName or similar. Current negative coverage uses wrong top-level shape / schemaVersion / cross-refs but not a row-level JsonSchema failure. Add one case per loader (e.g. empty displayName on a otherwise-valid prototype row) to lock schema messaging and match precedent. Done. RewardTableDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema and EncounterDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema (empty displayName).

  2. Optional module page snapshot — Add a short NEO-101 landed bullet under E5_M3_EncounterAndRewardTables.md (or an Implementation snapshot paragraph) pointing at server/NeonSprawl.Server/Game/Encounters/ and the README sections, so the module page stays aligned with the register without opening the alignment doc. Done. Summary Status + Implementation snapshot (NEO-101) section.

Nits

  • Nit: Host boot failure test (Host_ShouldFailStartup_WhenEncountersDirectoryInvalid) covers empty encounters dir only; a symmetric empty reward-tables dir case would mirror NEO-88 completeness but is not required by the plan. Done. Host_ShouldFailStartup_WhenRewardTablesDirectoryInvalid added.

  • Nit: AddEncounterAndRewardCatalogs re-binds ContentPathsOptions (same as other catalog extensions); harmless but duplicates registration if consolidated later.

Verification

# NEO-101-focused (22 tests — all passed locally)
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"

# CI parity
python3 scripts/validate_content.py

# Full server suite (Postgres integration tests need Docker locally)
dotnet test NeonSprawl.sln

Local run: 22/22 encounter-catalog tests passed. Full suite: 519 passed, 11 failed — all Postgres/docker-compose harness failures unrelated to this diff.