# 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

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