59 lines
4.6 KiB
Markdown
59 lines
4.6 KiB
Markdown
# 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**
|
||
|
||
**Follow-up:** Review suggestions below are **done** (strikethrough + **Done.**).
|
||
|
||
## 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.~~ **Done.** Gate now resolves the row with `prototypeRole == "equip_stub"` and asserts `equipment` + `stackMax: 1` on that id (`scripts/validate_content.py`).
|
||
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.~~ **Done.** E3M3-01 in-scope text, ticked acceptance criteria, and NEO-50 landed note.
|
||
|
||
## 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).
|