From 1565d2cde7c01991b7a4e1a1d790bc5bccd482d3 Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Sun, 7 Jun 2026 12:09:57 -0400
Subject: [PATCH] NEO-119: Add code review for quest-progress GET branch.

---
 docs/reviews/2026-06-07-NEO-119.md | 65 ++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 docs/reviews/2026-06-07-NEO-119.md

diff --git a/docs/reviews/2026-06-07-NEO-119.md b/docs/reviews/2026-06-07-NEO-119.md
new file mode 100644
index 0000000..bb4b0ef
--- /dev/null
+++ b/docs/reviews/2026-06-07-NEO-119.md
@@ -0,0 +1,65 @@
+# 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
+
+```bash
+# 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.