From 8eccb148984a45cf04496993596adac0d35c5a0d Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 17:06:38 -0400 Subject: [PATCH] NEO-68: Add code review for kickoff implementation plan. --- docs/reviews/2026-05-24-NEO-68.md | 69 +++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-68.md diff --git a/docs/reviews/2026-05-24-NEO-68.md b/docs/reviews/2026-05-24-NEO-68.md new file mode 100644 index 0000000..5ea212c --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-68.md @@ -0,0 +1,69 @@ +# Code review — NEO-68 GET world recipe-definitions (kickoff plan) + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-68-get-world-recipe-definitions-bruno` · commit `6de6a17` vs `origin/main` (implementation plan only — no server/Bruno/test code yet) +**Base:** `origin/main` + +## Verdict + +**Approve** — kickoff plan is ready to implement; **re-review required** once HTTP/DTO/tests/Bruno land. + +## Summary + +The branch adds only [`docs/plans/NEO-68-implementation-plan.md`](../plans/NEO-68-implementation-plan.md): kickoff clarifications, acceptance criteria, and a technical approach that mirrors the landed [NEO-53](../plans/NEO-53-implementation-plan.md) item-definitions pattern. Scope is correctly bounded to **`GET /game/world/recipe-definitions`** backed by **`IRecipeDefinitionRegistry`** (NEO-67 on `main`), with **`schemaVersion` 1** / **`recipes`** envelope, six required row fields plus I/O arrays, Bruno, manual QA, and AAA integration tests. Client work is explicitly deferred to **NEO-74**. No correctness or security risk yet — there is no runtime code on this branch. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-68-implementation-plan.md`](../plans/NEO-68-implementation-plan.md) | **Matches** — kickoff decisions recorded; files-to-add/modify list complete; registry-only injection; frozen eight id order documented; NEO-74 cross-link present. | +| [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) · **E3M2-04** | **Matches** — route, DTOs, API tests, Bruno, README align with backlog; plan adds `docs/manual-qa/NEO-68.md` (appropriate for player-visible HTTP, NEO-53 precedent). | +| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Partially matches** — freeze table and eight ids match plan test assertions; **Related implementation slices** HTTP bullet correctly listed as a modify-on-land item. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E3.M2 row update deferred until implementation (correct). | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M2 **Status** still **Planned** (same as post–NEO-67); optional bump when Slice 3 server spine completes. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** (planned) — read-only world catalog; server source of truth; no client mutation in this story. | +| NEO-53 reference implementation | **Matches** — plan cites [`ItemDefinitionsWorldApi`](../../server/NeonSprawl.Server/Game/Items/ItemDefinitionsWorldApi.cs), DTO pattern, Bruno folder layout, and integration test shape. | +| Full-stack epic decomposition | **Matches** — **NEO-74** documented as client counterpart; plan does not claim prototype slice complete without Godot. | + +Register/tracking: no alignment updates expected until implementation merges. + +## Blocking issues + +None (plan-only scope). + +## Suggestions + +1. **Bruno — exact frozen-eight id order** — Plan section 6 requires ascending order and frozen eight presence. After NEO-53 review, [`Get item definitions.bru`](../../bruno/neon-sprawl-server/item-definitions/Get%20item%20definitions.bru) also asserts the **exact** registry sequence (not just sort + set membership). Mirror that with a `frozen prototype eight matches registry id order` Bruno test using the plan’s frozen list so manual runs catch ordering regressions without `dotnet test`. + +2. **Shared frozen id constant** — Follow [`ItemDefinitionsWorldApiTests`](../../server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionsWorldApiTests.cs): define a `FrozenEightInIdOrder` string array in the integration test file and reference the same sequence in manual QA / Bruno docs to avoid drift from the E3.M2 freeze table. + +3. **E3M2 backlog checkboxes on land** — When implementation merges, update **E3M2-04** acceptance checkboxes and landed note in [`E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) (same pattern as NEO-66/67). + +## Nits + +- Nit: Plan lists **`RecipeDefinitionsListDtos.cs`** as a separate file — NEO-53 keeps DTOs in one file with the API class; either layout is fine; pick one and stay consistent with the Items folder convention. + +- Nit: Manual QA template in plan says “mirror NEO-53” — include the metadata table header (`Key`, `Title`, `Linear`, `Plan`, `Branch`) from [`docs/manual-qa/NEO-53.md`](../manual-qa/NEO-53.md) for consistency. + +## Verification + +Plan phase (current): + +```bash +# Confirm branch is plan-only vs main +git diff origin/main...HEAD --stat +``` + +After implementation (required before merge): + +```bash +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~RecipeDefinitionsWorldApiTests" +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +Manual: + +- Start server; `curl -sS "http://localhost:5253/game/world/recipe-definitions"` — **200**, **`schemaVersion` 1**, **`recipes.length` 8**, frozen eight ids in ordinal order. +- Run `bruno/neon-sprawl-server/recipe-definitions/Get recipe definitions.bru` against `environments/Local.bru`. +- Walk through `docs/manual-qa/NEO-68.md`.