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-07-NEO-119.md

4.6 KiB
Raw Blame History

Code review — NEO-119 (E7M1-08)

Date: 2026-06-07
Scope: Branch NEO-119-get-quest-progress vs origin/main — commits 78e5de3 … 9e4c52f
Base: origin/main

Verdict

Approve with nits

Summary

NEO-119 adds GET /game/players/{id}/quest-progress — a versioned v1 snapshot merging IQuestDefinitionRegistry catalog rows with IPlayerQuestStateStore progress (missing row → not_started). The handler mirrors EncounterProgressApi: trim id, position-store gate → 404, then JSON. Before snapshot build it best-effort calls QuestObjectiveWiring.TryRefreshInventoryProgressForPlayer (inventory refresh + step completion sweep), adopting the kickoff side-effecting GET decision for HUD polling. Seven AAA integration tests cover 404 gates, default not_started, active partial counters, completed gather/chain terminal index, and GET-triggered chain completion with a pre-held token. Bruno default-state smoke, server/README.md, plan reconciliation, and E7.M1 decomposition docs are updated. Server-only slice — client HUD correctly deferred to NEO-122. Risk is low; no blocking correctness or contract issues found.

Documentation checked

Path Result
docs/plans/NEO-119-implementation-plan.md Matches — kickoff GET refresh adopted; acceptance checklist checked; reconciliation section accurate.
docs/plans/E7M1-prototype-backlog.md (E7M1-08) Matches — acceptance checkboxes checked; client counterpart NEO-122 noted.
docs/decomposition/modules/E7_M1_QuestStateMachine.md Matches — NEO-119 GET bullet + README/Bruno links.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E7M1-08 / NEO-119 landed note; register stays Planned until E7M1-09+ (correct).
docs/decomposition/modules/module_dependency_register.md Matches — NEO-119 note appended.
server/README.md Matches — route, envelope, side-effecting GET note, sample JSON.
Full-stack epic decomposition Matches — server-only story; plan explicitly defers Godot to NEO-122; does not claim prototype slice complete.

Register / tracking: E7.M1 row correctly remains Planned until accept HTTP (NEO-120) and client capstone (NEO-123); no further register edits required for this merge.

Blocking issues

(none)

Suggestions

  1. Assert response array order explicitly — GetQuestProgress_ShouldReturnNotStartedForAllQuests_WhenNoProgress and the Bruno default test compare sorted quest ids. The plan and DTO doc promise catalog id ordinal order (same as GetDefinitionsInIdOrder()). For the fixed four-quest prototype, sorted vs sequence is equivalent today, but asserting sequence (as in QuestDefinitionRegistryTests) would lock the contract if ids ever stop sorting identically to registry order.

  2. Side-effecting GET test store read-back — GetQuestProgress_ShouldCompleteChainTerminal_WhenTokenHeldAndInventoryRefreshRunsOnGet asserts HTTP completed and pre-GET Active, but does not re-read IPlayerQuestStateStore after GET. Adding a post-GET store assertion would strengthen coverage of the documented mutating read semantics (mirrors the beforeGet guard already in Arrange).

Nits

  • Nit: MapRow throws InvalidOperationException on unknown QuestProgressStatus (would surface as 500). Acceptable defensive guard; optional direct BuildSnapshot unit test if parity with EncounterProgressApiTests corrupt-state tests is desired later.

  • Nit: QuestProgressApiTests helper block (~200 lines) duplicates patterns from QuestObjectiveWiringTests / encounter quest helpers — acceptable for integration isolation; consider shared test fixture only if NEO-120 adds more HTTP quest tests.

Verification

# Primary signal for this story
cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~QuestProgressApiTests"

# Quest module regression
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~NeonSprawl.Server.Tests.Game.Quests"

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

Bruno: run bruno/neon-sprawl-server/quest-progress/Get quest progress default.bru against a local server with fresh dev-local-1.

Reviewer note: All 7 QuestProgressApiTests and 85 quest-namespace tests passed locally during review.

Manual: None required beyond Bruno for this server-only story; Godot HUD verification belongs to NEO-122.