65 lines
5.5 KiB
Markdown
65 lines
5.5 KiB
Markdown
# Code review — NEO-65 prototype RecipeDef catalog + schemas + CI
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-65-e3m2-prototype-recipedef-catalog-schemas-ci` · commits `0556d5f`–`b4f27b4` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-65 delivers the **E3M2-01** infrastructure slice: `recipe-def` / `recipe-io-row` JSON Schemas, eight frozen rows in `content/recipes/prototype_recipes.json`, and `scripts/validate_content.py` gates mirroring NEO-50 / NEO-57 (schema, duplicate `id`, exact allowlist, `recipeKind` coverage, item + skill cross-checks, Slice 3 `refine`-only contract). Docs close out the plan, CT.M1 PR-gate paragraph, E3.M2 freeze table, alignment register row, and E3M2-01 backlog checkboxes. The branch also carries Epic 3 **full-stack decomposition** prep (E3M2/E3S5 backlogs, `.cursor/rules/full-stack-epic-decomposition.md`) — appropriate context for Slice 3 but orthogonal to NEO-65 runtime risk. No server, client, or C# changes; CI script is the regression signal. Negative gates verified locally (duplicate id, unknown item, wrong skill, seven-recipe roster).
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-65-implementation-plan.md`](../plans/NEO-65-implementation-plan.md) | **Matches** — schemas, catalog, validate_content gates, doc touches, acceptance checklist complete; out-of-scope (server/HTTP) respected. |
|
||
| [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) | **Matches** — E3M2-01 checkboxes + landed note; infrastructure-only (no client counterpart required for this slug). |
|
||
| [`docs/plans/E3S5-client-prototype-backlog.md`](../plans/E3S5-client-prototype-backlog.md) | **Matches** — craft UI **NEO-74** / capstone **NEO-75** exist for player-visible Slice 3 work (full-stack policy satisfied for epic, not this ticket). |
|
||
| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — freeze table, designer note, CI rules, backlog links; header **Status** **In Progress** + NEO-65 implementation snapshot (addressed post-review). |
|
||
| [`docs/decomposition/modules/CT_M1_ContentValidationPipeline.md`](../decomposition/modules/CT_M1_ContentValidationPipeline.md) | **Matches** — recipe catalog PR gate paragraph. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row **In Progress** with NEO-65 landed note; E3.M1/E3.M3 cross-link Slice 5 client issues. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M2 **note** and E3 Slice 5 note updated; table **Status** column still **Planned** (consistent with E3.M3 pattern until slice completes — optional bump when NEO-66 lands). |
|
||
| [`content/README.md`](../../content/README.md) | **Matches** — `recipes/` table row + Slice 3 freeze paragraph. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — content/CI only. |
|
||
| Manual QA | **N/A** — plan defers to CI + ad-hoc negative CLI checks (same as NEO-50 / NEO-57); no `docs/manual-qa/NEO-65.md` required for infrastructure-only content. |
|
||
|
||
Register/tracking: alignment table E3.M2 **In Progress** is appropriate after E3M2-01; register row **Planned** until Slice 3 server stories land is acceptable if intentional.
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**E3.M2 module doc status + snapshot** — [`E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) header **Status** is still **Planned** while [`documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) lists **In Progress**. Bump header to **In Progress** and add an **Implementation snapshot** bullet for NEO-65 (catalog + CI), mirroring E3.M1/E3.M3 landed notes.~~ **Done.** Header **In Progress** + NEO-65 snapshot bullet added.
|
||
|
||
2. ~~**NEO-66 sync reminder** — When the server loader lands, duplicate `PROTOTYPE_SLICE3_RECIPE_IDS` in C# with an explicit “keep in sync with `validate_content.py`” comment (already noted in the plan; worth repeating in the NEO-66 kickoff plan).~~ **Done.** Kickoff note added under E3M2-02 in [`E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) (NEO-66 plan not yet authored).
|
||
|
||
## Nits
|
||
|
||
- Nit: `_validate_recipe_catalogs` runs I/O `itemId` cross-check even when a duplicate `id` is detected (inside the `if isinstance(rid, str)` block but outside the `else`). Harmless — surfaces more errors per run; matches pragmatic CI style elsewhere.
|
||
|
||
- Nit: Branch mixes **chore** full-stack policy commits with NEO-65 implementation — fine for sequencing, but PR description should call out both so reviewers know the policy/docs delta is intentional prep, not scope creep.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd /home/don/neon-sprawl
|
||
pip install -r scripts/requirements-content.txt
|
||
python3 scripts/validate_content.py
|
||
# content OK: … 1 recipe catalog file(s), … 8 unique recipe id(s)
|
||
```
|
||
|
||
Manual negative cases (from plan — verified during review):
|
||
|
||
- Duplicate recipe `id` → exit 1
|
||
- Unknown `itemId` in inputs → exit 1
|
||
- `requiredSkillId: salvage` → exit 1 (Slice 3 gate)
|
||
- Remove one frozen id (seven recipes) → exit 1
|
||
|
||
PR gate (`.github/workflows/pr-gate.yml`) already invokes `validate_content.py` — no additional test project required for this story.
|