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-06-NEO-117.md

4.9 KiB
Raw Blame History

Code review — NEO-117 (E7M1-06)

Date: 2026-06-06
Scope: Branch NEO-117-quest-state-operations vs origin/main — commits 44ef5f2 … f053ffb
Base: origin/main

Verdict

Approve with nits

Summary

NEO-117 adds QuestStateOperations — static accept, step advance, and complete wrappers over IPlayerQuestStateStore with QuestStateReasonCodes and QuestStateOperationResult, mirroring the NEO-104/105 encounter store + operations pattern. Prerequisite enforcement reads catalog prerequisiteQuestIds; complete is idempotent at the operations layer (replay → Success: true with unchanged snapshot), including a defensive re-read after store denial for concurrent complete. Fifteen AAA tests cover accept prerequisites/denials, advance bounds, idempotent complete, and host DI smoke via InMemoryWebApplicationFactory. server/README.md documents operations and reason codes. Server-only infrastructure — correct scope; HTTP and objective wiring remain deferred. Risk is low; main follow-ups are test helper consistency, minor coverage gaps, and post-merge decomposition register updates.

Documentation checked

Path Result
docs/plans/NEO-117-implementation-plan.md Matches — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate.
docs/plans/E7M1-prototype-backlog.md (E7M1-06) Partially matches — implementation satisfies AC; backlog checkboxes still [ ] (expected until merge/story end).
docs/decomposition/modules/E7_M1_QuestStateMachine.md Partially matches — module page still lists QuestStateOperations as future E7M1-06; should note NEO-117 landed after merge.
docs/decomposition/modules/documentation_and_implementation_alignment.md Partially matches — E7.M1 row ends at E7M1-05 / NEO-116; plan defers register update until story lands — append E7M1-06 / NEO-117 on merge.
docs/decomposition/modules/module_dependency_register.md Partially matches — no NEO-117 note yet; same deferred update as NEO-116 review.
server/README.md Matches — quest operations section, reason-code table, idempotent complete semantics, HTTP deferral to NEO-119/NEO-120.
Full-stack epic decomposition N/A — server-only; client counterpart explicitly NEO-120/NEO-122; plan does not claim prototype slice complete.

Register / tracking: E7.M1 register row and module Status should gain an E7M1-06 / NEO-117 landed bullet after merge (register stays Planned until E7M1-07+ runtime and client capstone).

Blocking issues

(none)

Suggestions

  1. Test ResolveDependencies pattern — QuestStateOperationsTests.ResolveDependencies creates an IServiceScope that is never disposed. EncounterCompletionOperationsTests resolves from factory.Services root (singleton stores). Align to avoid undisposed scopes and match the established encounter test pattern.

  2. unknown_quest coverage on advance/complete — Accept path has TryAccept_ShouldDenyUnknownQuest_WhenQuestIdNotInCatalog; advance and complete lack parity. Add two small [Fact] cases (mirror EncounterCompletionOperationsTests unknown-encounter test) for registry gate consistency.

  3. Post-merge decomposition docs — Plan defers register/module updates; on story end append E7M1-06 / NEO-117 to documentation_and_implementation_alignment.md, E7_M1_QuestStateMachine.md (operations bullet + README link), and check E7M1-06 AC boxes in E7M1-prototype-backlog.md.

Nits

  • Nit: TryAccept progress switch default _ => Deny(UnknownQuest) is unreachable while QuestProgressStatus is only Active/Completed; if kept for exhaustiveness, a more accurate code (or NotActive) would aid future enum extensions.

  • Nit: Chain-quest setup tests hardcode prototype_quest_combat_intro and prototype_quest_refine_intro; plan deferred RefineIntroQuestId / CombatIntroQuestId constants — acceptable for now, but NEO-118 wiring will reuse these ids.

  • Nit: TryMarkComplete re-calls TryGetProgress after the active-row read (lines 117–118) where active could be reused on the not_active deny path — minor readability only.

Verification

# Quest state operations (primary signal for this story)
cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~QuestStateOperationsTests"

# Quest store regression (precursor)
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~InMemoryPlayerQuestStateStoreTests|FullyQualifiedName~PlayerQuestProgressPersistence"

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

Reviewer note: All 15 QuestStateOperationsTests and full server suite (663 tests) passed locally during review.

Manual: None required — no HTTP or player-visible surface in this story.