diff --git a/docs/reviews/2026-06-03-NEO-115.md b/docs/reviews/2026-06-03-NEO-115.md new file mode 100644 index 0000000..599d5d8 --- /dev/null +++ b/docs/reviews/2026-06-03-NEO-115.md @@ -0,0 +1,63 @@ +# 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` | **Partially matches** — E7.M1 note still ends at NEO-114 registry; alignment doc covers NEO-115 (see Suggestion 2). | +| `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. + +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. + +## 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#. + +- 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.