diff --git a/docs/reviews/2026-05-30-NEO-100.md b/docs/reviews/2026-05-30-NEO-100.md new file mode 100644 index 0000000..aa7f80c --- /dev/null +++ b/docs/reviews/2026-05-30-NEO-100.md @@ -0,0 +1,60 @@ +# Code review — NEO-100 (E5M3-01) + +**Date:** 2026-05-30 +**Scope:** Branch `NEO-100-e5m3-encounter-reward-catalog-schemas-ci` vs `cb86645` (merge-base on `main`) — commits `4f16c84` … `ee5e1a0` +**Base:** `cb86645` (main after PR #138) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-100 delivers the **E5M3-01** content spine: three JSON Schemas (`encounter-def`, `reward-table`, `reward-grant-row`), prototype catalog files with the frozen **`prototype_combat_pocket`** encounter and **`prototype_combat_pocket_clear`** reward table, and **`scripts/validate_content.py`** extensions mirroring the NEO-87 / NEO-76 catalog pattern (schema validation, duplicate `id` rejection, prototype id allowlists, item / NPC instance / reward-table cross-refs, reward-table-before-encounter ordering). Documentation updates (E5.M3 freeze CI line, CT.M1 PR gate paragraphs, alignment register, E5M3-01 backlog checkboxes, implementation plan) match the shipped artifacts. No server, C#, or Godot changes — appropriate for a CI-only catalog story that unblocks NEO-101. Residual risk is low: validation is script-only (no pytest), and **`module_dependency_register.md`** E5.M3 note still reads “implementation not started.” + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-100-implementation-plan.md` | **Matches** — scope, constants, file list, acceptance checklist, and implementation reconciliation align with the diff. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-01) | **Matches** — three AC items checked; landed note cites schemas, catalogs, and CI gates. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — CI enforcement paragraph under freeze table; ids and cross-ref rules match `validate_content.py` constants. | +| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Matches** — reward-table and encounter catalog paragraphs added with correct schema paths and gate semantics. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-100 catalog landed**; server load still outstanding. | +| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E5.M3 status **In Progress** is correct, but the **E5.M3 note** still says “implementation not started” (stale vs alignment row). | +| `content/README.md` | **Matches** — NEO-100 paragraph and table rows for `encounters/` / `reward-tables/` match shipped ids and cross-ref rules (plan “verify only”). | +| Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until E5M3-08+ / NEO-110 / NEO-111; no player-visible claim in this story. | +| `server/NeonSprawl.Server/Game/Npc/PrototypeNpcRegistry.cs` | **Matches** (cross-ref) — CI `PROTOTYPE_E5M2_NPC_INSTANCE_IDS` matches server constants (`prototype_npc_melee`, `prototype_npc_ranged`, `prototype_npc_elite`). | + +## Blocking issues + +None. + +## Suggestions + +1. **Update `module_dependency_register.md` E5.M3 note** — Replace “implementation not started” with **E5M3-01 / NEO-100 catalog landed** (schemas + CI); server load/runtime still NEO-101+. Keeps register consistent with `documentation_and_implementation_alignment.md` and E5.M3 module doc. + +2. **Duplicate `fixedGrants` rows** — `_prototype_e5m3_reward_grant_content_gate` folds grants into a dict (last row wins). Duplicate `itemId` rows that merge to the frozen quantities **pass CI** (verified locally). For NEO-101 loader clarity, consider rejecting duplicate `itemId` in `fixedGrants` (schema `uniqueItems` on a composite key is awkward; a small loop in the grant gate matches mastery `perkId` duplicate rejection). Same pattern gap exists on recipe I/O rows — optional to defer until roster expansion. + +## Nits + +- Nit: **`requiredNpcInstanceIds`** schema omits `uniqueItems: true`; the set-equality gate catches wrong sets for the one-row prototype, but adding `uniqueItems` would fail fast on duplicate ids before the custom gate (and documents intent for NEO-101). + +- Nit: Plan item 7 lists **`content/README.md` verify** — reconciliation section could explicitly note “verified, no edit required” for audit trail (optional). + +## Verification + +```bash +# Primary regression (also runs in PR gate) +python3 scripts/validate_content.py + +# Manual negative spot-checks (from plan) +# 1. Duplicate encounter or reward-table id → non-zero exit +# 2. Remove frozen id from catalog → gate error +# 3. Unknown itemId in fixedGrants → cross-ref error +# 4. Wrong requiredNpcInstanceIds set → cross-ref error +# 5. Broken rewardTableId → cross-ref error +# 6. Invalid completionCriteria.kind or empty displayName → schema error +``` + +**Before merge:** Confirm PR gate green on the branch (`validate_content.py` is the sole automated test surface for this story).