# 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` | **Matches** — **Implementation snapshot** HTTP bullet added; Summary **Status** row marks **E5M3-04 / NEO-103 HTTP read landed** (review follow-up). |
| `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.~~ **Done.** Summary **Status** row updated.

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).~~ **Done.** `FrozenPrototypeGrantsInOrder` + indexed assertions in `EncounterDefinitionsWorldApiTests`.

## 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.~~ **Done.** Ordinal comment added to Bruno id-order test.

- 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.