NEO-103: Add code review for encounter-definitions GET.
parent
935e8d5f16
commit
573c42add7
|
|
@ -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.
|
||||||
Loading…
Reference in New Issue