NEO-68: Add code review for kickoff implementation plan.
parent
6de6a1781f
commit
8eccb14898
|
|
@ -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`.
|
||||
Loading…
Reference in New Issue