diff --git a/docs/reviews/2026-05-31-NEO-103.md b/docs/reviews/2026-05-31-NEO-103.md new file mode 100644 index 0000000..801b956 --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-103.md @@ -0,0 +1,59 @@ +# Code review — NEO-103 (E5M3-04) + +**Date:** 2026-05-31 +**Scope:** Branch `NEO-103-get-world-encounter-definitions` vs `fb5137c` (merge-base on `main`) — commits `62e6bfd` … `935e8d5` +**Base:** `fb5137c` (main at branch point, post NEO-102 merge) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-103 adds **`GET /game/world/encounter-definitions`**, mirroring the established NEO-78 / NEO-60 world-definition pattern: versioned envelope (`schemaVersion` **1**, **`encounters[]`**), registry-backed projection ( **`IEncounterDefinitionRegistry`** + **`IRewardTableDefinitionRegistry`**, not catalog types), nested **`completionCriteria`** and **`rewardTable`** summary, id-order from **`GetDefinitionsInIdOrder()`**, and fail-fast **`InvalidOperationException`** when a reward-table cross-ref is missing at HTTP time. One AAA integration test, Bruno smoke with four assertion blocks, `Program.cs` wiring after resource-node definitions, and docs (plan, backlog, alignment register, module register, `server/README.md`) are included. Risk is low: read-only server route with no player state; client wiring correctly deferred to NEO-110 / NEO-111. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-103-implementation-plan.md` | **Matches** — nested `rewardTable`, nested `completionCriteria`, registry injection, fail-fast reward-table guard, Bruno + AAA test, README; acceptance checklist checked; reconciliation section accurate. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-04) | **Matches** — acceptance criteria checked; landed note cites plan. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — **Implementation snapshot** HTTP bullet added; Summary **Status** row still lists **E5M3-04 NEO-103 →** without a **landed** marker (NEO-101/102 updated Summary when their slices shipped). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-103** HTTP read; runtime **NEO-104+** still outstanding. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-103 landed** + README + Bruno. | +| Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until NEO-110 / NEO-111. | +| `server/README.md` | **Matches** — encounter-definitions section documents route, fields, curl, plan + Bruno links. | + +## Blocking issues + +None. + +## Suggestions + +1. **Sync `E5_M3_EncounterAndRewardTables.md` Summary Status** — Update the **Status** table row to mark **E5M3-04 / NEO-103 HTTP read landed** (and point forward to **E5M3-05 / NEO-104**), mirroring how **NEO-101** and **NEO-102** landed markers appear in the same row. The implementation snapshot and register are already updated; only the Summary line lags. + +2. **Optional: assert `fixedGrants` order in C# test** — Bruno asserts the exact grant sequence (`scrap_metal_bulk` then `contract_handoff_token`); the integration test checks count and per-item quantities via `Single` but not array order. Adding an ordered assertion (or a frozen grants constant) would lock catalog-order regressions in `dotnet test` without relying on Bruno alone (NEO-53 precedent). + +## Nits + +- Nit: Bruno **`encounters are ascending by id (ordinal)`** uses a hand-rolled string compare; peer requests (e.g. item-definitions) document **StringComparer.Ordinal** in a comment. Harmless for ASCII snake_case ids; copy the comment for parity if touching the file. + +- Nit: With a single prototype encounter, the id-order Bruno test and C# **`Assert.Single`** are structurally redundant today; they become valuable when the roster expands—no change required now. + +## Verification + +```bash +# NEO-103-focused (1 test — passed locally) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterDefinitionsWorldApi" + +# Broader encounter slice (catalog + registries + HTTP) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters" + +# CI parity +python3 scripts/validate_content.py + +# Bruno (full collection — new folder auto-included) +cd bruno/neon-sprawl-server && npx --yes @usebruno/cli@3.3.0 run --env Local --sandbox=developer --bail +``` + +**Local run:** 1/1 `EncounterDefinitionsWorldApi` test passed.