# Code review — NEO-115 (E7M1-04)

**Date:** 2026-06-03  
**Scope:** Branch `NEO-115-get-world-quest-definitions` vs `origin/main` — commits `a5be355` … `b3f90de`  
**Base:** `origin/main`

## Verdict

**Approve with nits**

## Summary

NEO-115 adds **`GET /game/world/quest-definitions`**, a versioned read-only projection of the frozen prototype quest catalog via **`IQuestDefinitionRegistry`**. Implementation mirrors the established encounter/item world-definition pattern: `QuestDefinitionsWorldApi` maps registry rows to flat objective DTOs with **`WhenWritingNull`** on unused fields, wired in **`Program.cs`** after encounter definitions. One AAA integration test covers schema v1, four quests in ordinal id order, and spot-checks for gather, refine prerequisite, combat, and operator-chain terminal objectives. Bruno folder validates the same frozen rows and confirms null-key omission on a gather objective via strict `eql`. Documentation (implementation plan, backlog AC, E7.M1 snapshot, alignment register, `server/README.md`) is updated. Server-only infrastructure with optional client consumer NEO-122 — correct scope. Risk is low.

## Documentation checked

| Path | Result |
|------|--------|
| `docs/plans/NEO-115-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-04) | **Matches** — AC checked; in/out of scope aligned with shipped route. |
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — HTTP read model bullet documents NEO-115 route + Bruno. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-115 HTTP projection; register stays **Planned** until E7M1-05+ runtime. |
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note includes NEO-115 HTTP read (Suggestion 2 addressed). |
| `server/README.md` | **Matches** — quest definitions section with curl, registry note, plan/Bruno links. |
| Full-stack epic decomposition | **N/A** — server-only; client counterpart NEO-122 explicitly deferred. |

## Blocking issues

(none)

## Suggestions

1. ~~**Reuse frozen id ordering constant in tests** — Plan adopted reusing **`PrototypeE7M1QuestCatalogRules`** for ids. `QuestDefinitionsWorldApiTests` defines a parallel **`FrozenQuestIdsInOrdinalOrder`** array instead of deriving from **`ExpectedQuestIds.Order(StringComparer.Ordinal)`** (as **`QuestDefinitionRegistryTests`** already does). Consolidating avoids drift if the roster changes.~~ **Done.** `QuestDefinitionsWorldApiTests` now asserts id order via **`ExpectedQuestIds.Order(StringComparer.Ordinal)`**.

2. ~~**Optional: refresh `module_dependency_register` E7.M1 note** — E7.M1 module page and alignment register mention NEO-115; the register table note still stops at NEO-114. A one-line append (“**E7M1-04 / NEO-115** HTTP read landed”) would keep the dependency register consistent with NEO-114 follow-up style.~~ **Done.** E7.M1 note appended in **`module_dependency_register.md`**.

## Nits

- ~~Nit: C# integration test asserts **`refine_intro`** prerequisite only, not the **`craft_recipe`** objective shape; Bruno likewise skips refine/combat spot-checks. Adequate for prototype freeze given gather + operator-chain + combat coverage, but a single **`craft_recipe`** assert would close the objective-kind loop in C#.~~ **Done.** `refine_intro` **`craft_recipe`** objective asserted in **`QuestDefinitionsWorldApiTests`**.

- Nit: **`MapObjective`** default switch arm copies all optional fields for unknown kinds — unreachable for validated catalog content but reasonable defensive fallback.

- Nit: Single large integration test (78 lines of asserts) follows the encounter world API test style; acceptable for a frozen four-quest roster.

## Verification

```bash
# Quest definitions world API (primary signal for this story)
cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~QuestDefinitionsWorldApiTests"

# Quest registry regression (NEO-114)
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~QuestDefinitionRegistryTests"

# Full server suite (CI)
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
```

**Reviewer note:** `QuestDefinitionsWorldApiTests` (1 test) passed locally during review.

**Manual / CI Bruno:** Run `bruno/neon-sprawl-server/quest-definitions/Get quest definitions.bru` against a dev server on port 5253.