60 lines
5.1 KiB
Markdown
60 lines
5.1 KiB
Markdown
# Code review — NEO-46 mastery catalog startup load
|
||
|
||
**Date:** 2026-05-17
|
||
**Scope:** Branch `NEO-46-mastery-catalog-startup-load` · commits `898f935`–`174c390` vs `main`
|
||
**Base:** `main`
|
||
|
||
**Follow-up:** Addressed review suggestions (tests, `validate_content.py` `tierIndex` gate, E2.M3 module doc).
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
This change adds fail-fast loading of `content/mastery/*_mastery.json` at server startup, mirroring the NEO-34 skill-catalog pattern: path discovery, JsonSchema.Net validation, post-schema gates aligned with `scripts/validate_content.py`, a server-only `tierIndex` 1..N gate, cross-check against loaded skill ids, and a read-only `IMasteryCatalogRegistry` for NEO-47+. DI wires mastery load after `SkillDefinitionCatalog`; `Program.cs` eagerly resolves both catalogs before mapping routes. Tests cover the loader happy path, several deny paths, and host startup success/failure; all 13 mastery-related tests pass locally. Overall risk is low: read-only content load, no player state or HTTP surface yet.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-46-implementation-plan.md`](../plans/NEO-46-implementation-plan.md) | **Matches** — `Game/Mastery/`, config keys, loader gates, registry API, eager resolve, README, alignment table, acceptance checklist marked complete. |
|
||
| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Matches** — Status and NEO-46 handoff updated after follow-up. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E2.M3 row updated for NEO-46 landed. |
|
||
| [`docs/plans/NEO-45-implementation-plan.md`](../plans/NEO-45-implementation-plan.md) | **Matches** — Slice 4 constant/gate parity via `PrototypeSlice4MasteryCatalogRules`. |
|
||
| [`scripts/validate_content.py`](../../scripts/validate_content.py) | **Matches** — `tierIndex` 1..N gate added in CI (follow-up). |
|
||
|
||
Register/tracking: E2.M3 alignment row is updated; module doc refreshed (follow-up).
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Test coverage vs plan table** — The implementation plan lists deny cases for malformed JSON, schema violation, duplicate global `perkIds` reference, branch-set mismatch, non-increasing `requiredLevel`, Slice 4 wrong `skillId`, and duplicate `tierIndex`. Current tests cover unknown skill, duplicate perk across files, orphan perk, Slice 4 multi-track, `tierIndex` gap, and host negative startup; adding 2–3 focused loader tests (e.g. duplicate branch perk reference, `tierIndex` duplicate, wrong sole `skillId` for Slice 4) would close the gap and guard C#/Python drift.~~ **Done.** Added loader tests for malformed JSON, schema violation, duplicate perk reference, branch-set mismatch, flat `requiredLevel`, wrong Slice 4 sole `skillId`, and duplicate `tierIndex`.
|
||
2. ~~**Registry enumeration** — `GetTracksInSkillIdOrder()` / `GetPerksInIdOrder()` are part of the contract but untested; a small unit test on a constructed catalog would lock ordering for NEO-47 consumers.~~ **Done.** `MasteryCatalogRegistryTests` covers both enumeration methods.
|
||
3. ~~**E2.M3 module doc** — Update the **NEO-46 handoff** bullet to state that `tierIndex` uniqueness and sequential 1..N are enforced at server boot (not merely “consider”), and refresh the module **Status** line to mention NEO-46 (optional but keeps decomposition docs authoritative).~~ **Done.** `E2_M3_MasteryAndPerkUnlocks.md` updated.
|
||
4. ~~**CI follow-up (already in plan risks)** — Consider adding the `tierIndex` gate to `validate_content.py` so PR CI and server boot cannot diverge on catalog edits.~~ **Done.** `_tier_index_gate` in `validate_content.py` mirrors `MasteryCatalogLoader.TryGetTierIndexGateError`.
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: `MasteryCatalogLoaderTests.CreateTempContentLayout()` leaves temp directories behind; consider `try/finally` with `Directory.Delete(..., recursive: true)` or `IDisposable` fixture (same pattern as other catalog tests if they exist).~~ **Done.** `TempContentLayout` implements `IDisposable`.
|
||
- ~~Nit: Host startup is exercised in both `MasteryCatalogLoaderTests` and `MasteryCatalogRegistryTests`; consolidating to one host integration test would reduce duplication (not required for merge).~~ **Done.** Host success path lives only in `MasteryCatalogRegistryTests`; loader tests keep host negative startup only.
|
||
- Nit: Bruno `mastery-catalog` folder only hits `/health` — appropriate for NEO-46 scope; NEO-48 will own real mastery HTTP Bruno coverage. **Deferred** — out of scope for NEO-46.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# From repo root
|
||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~MasteryCatalog"
|
||
|
||
# Full server test suite (recommended before merge)
|
||
dotnet test NeonSprawl.sln
|
||
|
||
# Content gate (should stay green with repo catalog)
|
||
python3 scripts/validate_content.py
|
||
```
|
||
|
||
Manual: start server from clone with default content discovery; confirm Information log lists mastery directory, track count `1`, perk count `2` for prototype salvage catalog.
|