55 lines
3.7 KiB
Markdown
55 lines
3.7 KiB
Markdown
# Code review — NEO-102 (E5M3-03)
|
||
|
||
**Date:** 2026-05-31
|
||
**Scope:** Branch `NEO-102-injectable-encounter-reward-registries-di` vs `d7c2684` (merge-base on `main`) — commits `93eca82` … `734876c`
|
||
**Base:** `d7c2684` (main at branch point, post NEO-101 merge)
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-102 adds thin **`IEncounterDefinitionRegistry`** and **`IRewardTableDefinitionRegistry`** adapters over the NEO-101 startup-loaded catalogs, mirroring the established NEO-79 **`AbilityDefinitionRegistry`** pattern: `TryGetDefinition`, `TryNormalizeKnown` (trim + lowercase + fail-closed lookup), and cached `GetDefinitionsInIdOrder`. DI registers both singletons in `AddEncounterAndRewardCatalogs` after catalog loaders; no `Program.cs` change (catalogs remain the eager boot gate). Eighteen AAA unit + host DI tests pass locally. Docs (implementation plan, alignment register, E5M3 backlog, E5.M3 module snapshot, `server/README.md`) are updated. Risk is low: server-only registry surface with no HTTP or game call-site migration yet (correctly deferred to NEO-103+).
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/NEO-102-implementation-plan.md` | **Matches** — interfaces, adapters, DI, tests, README, alignment register; acceptance checklist checked; reconciliation section accurate. |
|
||
| `docs/plans/E5M3-prototype-backlog.md` (E5M3-03) | **Matches** — acceptance criteria checked; landed note cites plan and 18 tests. |
|
||
| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — Summary **Status** notes **NEO-102 registries landed**; **Implementation snapshot** paragraph added. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-102** registries; runtime NEO-103+ still outstanding. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E5.M3 note still ends with **Runtime/engine (NEO-102+) not started.**; should cite NEO-102 registry landing (see Suggestion 1). |
|
||
| Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until NEO-110 / NEO-111. |
|
||
| `server/README.md` | **Matches** — encounter + reward-table sections document registries as preferred lookup surface. |
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. **Sync `module_dependency_register.md` E5.M3 note** — NEO-101 review added **NEO-101 landed** to the register note; this branch updates alignment + module pages but leaves the register saying **NEO-102+ not started**. Add **NEO-102 landed:** injectable **`IEncounterDefinitionRegistry`** + **`IRewardTableDefinitionRegistry`** + DI, mirroring the alignment register row, so the three tracking surfaces stay consistent.
|
||
|
||
## Nits
|
||
|
||
- Nit: `EncounterDefinitionRegistryTests.Host_ShouldResolveRegistriesFromDi_WhenStartupSucceeds` resolves **both** registries (per plan), while `RewardTableDefinitionRegistryTests.Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` repeats reward-table assertions. Harmless overlap with NEO-79’s single-registry host test; optional consolidation later.
|
||
|
||
- Nit: `TryGetDefinition` is case-sensitive (delegates to ordinal catalog keys); only `TryNormalizeKnown` lowercases. Consistent with **`AbilityDefinitionRegistry`** — document for NEO-103 HTTP callers if they accept mixed-case query params.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# NEO-102-focused (18 tests — all passed locally)
|
||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterDefinitionRegistry|FullyQualifiedName~RewardTableDefinitionRegistry"
|
||
|
||
# Broader encounter slice (includes NEO-101 catalog tests)
|
||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"
|
||
|
||
# CI parity
|
||
python3 scripts/validate_content.py
|
||
```
|
||
|
||
**Local run:** 18/18 registry tests passed.
|