don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-03-NEO-115.md

4.5 KiB
Raw Permalink Blame History

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

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