From 8fa828daa024cb66055129a408f5833652cc10af Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 21:27:59 -0400 Subject: [PATCH] =?UTF-8?q?NEO-50:=20Add=20code=20review=20=E2=80=94=20App?= =?UTF-8?q?rove=20for=20prototype=20item=20catalog=20+=20CI.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reviews/2026-05-17-NEO-50.md | 56 +++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 docs/reviews/2026-05-17-NEO-50.md diff --git a/docs/reviews/2026-05-17-NEO-50.md b/docs/reviews/2026-05-17-NEO-50.md new file mode 100644 index 0000000..4904456 --- /dev/null +++ b/docs/reviews/2026-05-17-NEO-50.md @@ -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).