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.8 KiB
Raw Permalink 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. Done. Test asserts GetDefinitionsInIdOrder() sequence; Bruno compares response order without sort.

  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). Done. Post-GET TryGetProgress asserts Completed, terminal step index, and CompletedAt.

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.