NEO-50: Add code review — Approve for prototype item catalog + CI.

pull/85/head
VinPropane 2026-05-17 21:27:59 -04:00
parent bbc6b20fd3
commit 8fa828daa0
1 changed files with 56 additions and 0 deletions

View File

@ -0,0 +1,56 @@
# Code review — NEO-50 prototype ItemDef catalog + CI
**Date:** 2026-05-17
**Scope:** Branch `NEO-50-itemdef-starter-set-schemas-ci` · commits `6616bfb``bbc6b20` · full branch vs `main`
**Base:** `main`
## Verdict
**Approve**
## Summary
NEO-50 delivers a **content-only** Slice 1 spine: `item-def.schema.json`, six-row `prototype_items.json`, and extended `validate_content.py` with per-row schema validation, cross-file duplicate `id` detection, frozen six-id allowlist, one-row-per-`prototypeRole` coverage, and post-schema checks that `prototype_armor_shell` uses `equipment` + `stackMax: 1` when it carries `equip_stub`. Documentation (E3.M3 freeze table, `content/README.md`, CT.M1 CI paragraph, alignment register, implementation plan) matches kickoff decisions (`prototypeRole`, explicit `stackMax` / `inventorySlotKind`, optional forward-compat stubs). No server/C#/client surface — risk is low; PR gate already runs the script. One **should fix** tightens the equip-stub slot gate so a role/id swap cannot bypass equipment rules; backlog wording still mentions bucket/tags.
## Documentation checked
| Document | Result |
|----------|--------|
| [`docs/plans/NEO-50-implementation-plan.md`](../plans/NEO-50-implementation-plan.md) | **Matches** — acceptance checklist, freeze table, schema fields, and validation gates implemented as specified. |
| [`docs/plans/E3M3-prototype-backlog.md`](../plans/E3M3-prototype-backlog.md) | **Partially matches** — E3M3-01 scope and ids align; line 51 still says “required bucket/tag fields” (kickoff chose `prototypeRole` instead); acceptance checkboxes not ticked in backlog (plan checklist is ticked). |
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — designer note, freeze table, CI pointers, v1 scope (no durability mutation). |
| [`docs/decomposition/modules/CT_M1_ContentValidationPipeline.md`](../decomposition/modules/CT_M1_ContentValidationPipeline.md) | **Matches** — item catalog paragraph added; module **Status** in summary table still **Planned** (CT.M1 row not promoted — optional follow-up). |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row → In Progress / NEO-50 content landed (appropriate on branch pre-merge). |
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M3 → **In Progress** with NEO-50 note. |
| [`content/README.md`](../../content/README.md) | **Matches** — items path active with six-id freeze pointer. |
## Blocking issues
None.
## Suggestions
1. **Equip-stub slot gate by role, not id**`_prototype_slice1_item_gate` only enforces `inventorySlotKind: equipment` and `stackMax: 1` when `prototype_armor_shell` has `prototypeRole: equip_stub`. Swapping `prototypeRole` between frozen ids (e.g. `survey_drone_kit``equip_stub` with `bag`) would pass CI while violating the designer contract. Resolve the row where `id_to_role[*] == "equip_stub"` (there is exactly one) and assert slot/stack on that id.
2. **Backlog doc sync** — Update [`E3M3-prototype-backlog.md`](../plans/E3M3-prototype-backlog.md) E3M3-01 bullet from “bucket/tag fields” to `prototypeRole` (+ optional tick acceptance criteria) so backlog matches the implementation plan and shipped validator.
## Nits
- Nit: Add a `// Keep in sync with scripts/validate_content.py` comment placeholder in NEO-51 server loader constants when C# load lands (plan risk table already calls this out).
- Nit: `CT_M1` module summary **Status** remains **Planned** while skill/mastery/item CI is live — consider **In Progress** when promoting CT.M1 is intentional scope.
## Verification
```bash
# From repo root
pip install -r scripts/requirements-content.txt
python3 scripts/validate_content.py
python3 scripts/check_decomposition_language.py
# Manual negatives (expect non-zero exit + stderr message)
# 1. Duplicate id in prototype_items.json
# 2. Remove one frozen id
# 3. Invalid stackMax (0) on a row
# 4. Duplicate prototypeRole on two rows
```
Confirm `.github/workflows/pr-gate.yml` passes on the PR (content validation step unchanged except script behavior).