NEO-100: Add code review for encounter + reward catalog CI
parent
ee5e1a0caf
commit
01b7e23c5e
|
|
@ -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).
|
||||
Loading…
Reference in New Issue