NEO-101: Add code review for encounter/reward catalog load.
parent
f921095fb0
commit
5e26143fbd
|
|
@ -0,0 +1,57 @@
|
|||
# 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` | **Partially matches** — freeze table and CI paragraph reference NEO-101 loader sync; Summary **Status** still generic “In Progress — Slice 3 backlog” without an explicit **NEO-101 server load landed** bullet (alignment register was updated; module page could mirror). |
|
||||
| `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.
|
||||
|
||||
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.
|
||||
|
||||
## 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.
|
||||
|
||||
- Nit: `AddEncounterAndRewardCatalogs` re-binds `ContentPathsOptions` (same as other catalog extensions); harmless but duplicates registration if consolidated later.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# NEO-101-focused (19 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:** 19/19 encounter-catalog tests passed. Full suite: 519 passed, 11 failed — all Postgres/docker-compose harness failures unrelated to this diff.
|
||||
Loading…
Reference in New Issue