diff --git a/docs/reviews/2026-06-07-NEO-129.md b/docs/reviews/2026-06-07-NEO-129.md new file mode 100644 index 0000000..d86d7cb --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-129.md @@ -0,0 +1,55 @@ +# Code review — NEO-129 (E7M2-06) + +**Date:** 2026-06-07 +**Scope:** Branch `NEO-129-quest-progress-completion-reward-summary` vs `main` — commits `2b06eb2` … `78e805e` +**Base:** `main` + +## Verdict + +**Approve with nits** — additive `completionRewardSummary` on completed quest-progress rows, sourced from `IRewardDeliveryStore`, matches the adopted plan and NEO-108 encounter precedent; mapper threading through `QuestAcceptApi` is correct; tests and Bruno cover primary AC paths; risk is low (server-only until NEO-131 client parse). + +## Summary + +NEO-129 extends **`GET /game/players/{id}/quest-progress`** with optional **`completionRewardSummary`** on **`completed`** rows: nested **`itemGrants`** + **`skillXpGrants`** mapped from **`RewardDeliveryEvent`** snapshots via **`IRewardDeliveryStore.TryGet`**, omitted for **`not_started`** / **`active`** and when no delivery record exists (kickoff). **`QuestProgressApi.BuildSnapshot`** / **`MapQuestProgressRow`** accept **`playerId`** + **`deliveryStore`**; **`QuestAcceptApi.MapResponse`** uses the same mapper so embedded accept rows stay consistent. DTOs in **`QuestProgressListDtos.cs`** use **`JsonIgnore(WhenWritingNull)`** and preserve commit-time grant order (XML on mappers mirrors NEO-108). Eight AAA integration tests in **`QuestProgressApiTests`** assert omit on default/active, gather-intro salvage **25**, operator-chain item + XP, and idempotent GET replay; Bruno adds gather-intro complete spine plus default GET omit assertion. Docs (plan reconciliation, E7.M2 anchor, alignment register, E7M2-06 backlog AC, **`server/README.md`**) are updated. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-129-implementation-plan.md` | **Matches** — kickoff nested shape and omit-on-miss adopted; reconciliation accurate; AC checked. | +| `docs/plans/E7M2-prototype-backlog.md` (E7M2-06) | **Matches** — AC checkboxes complete; client counterpart NEO-131 correctly deferred. | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-129 HTTP read anchor added; separated from NEO-128 wiring note. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-06 HTTP read; row correctly stays **Planned** until NEO-132 capstone. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required. | +| `server/README.md` | **Matches** — quest-progress section documents **`completionRewardSummary`** projection source and gather-intro sample. | +| Full-stack epic decomposition | **N/A** — E7M2-06 is server HTTP read; client counterpart [NEO-131](https://linear.app/neon-sprawl/issue/NEO-131) explicitly out of scope. | + +**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment update is appropriate. + +## Blocking issues + +None. + +## Suggestions + +1. **GET-refresh chain completion summary** — `GetQuestProgress_ShouldCompleteChainTerminal_WhenTokenHeldAndInventoryRefreshRunsOnGet` asserts **`completed`** via inventory refresh wiring but does not assert **`CompletionRewardSummary`** (unlike gather and operator-chain complete tests). That path exercises deliver-then-mark; adding item + skill XP asserts would lock summary projection on the GET side-effect completion path. + +2. **Bruno idempotent second GET** — The implementation plan Bruno AC calls for a second GET unchanged; **`Get quest progress after gather intro complete.bru`** performs only one GET. Optional follow-up: duplicate GET in pre-request or a second request file to match plan and integration test coverage. + +3. **Completed without delivery record** — Kickoff adopts omit-on-miss (softer than NEO-108 fail-fast). No test forces **`status: completed`** with a store miss (e.g. direct **`IPlayerQuestStateStore`** seed without **`IRewardDeliveryStore`** record). Low priority given NEO-128 deliver-then-mark invariant in normal paths; a single defensive test would document the contract. + +## Nits + +- Nit: Idempotent replay test (`GetQuestProgress_ShouldReturnSameCompletionRewardSummary_WhenGatherIntroCompletedTwice`) compares only the first **`skillXpGrants`** line, not full **`completionRewardSummary`** object equality (Bruno uses **`eql`** on arrays). Fine for single-line gather intro; operator-chain idempotent replay is untested. + +- Nit: **`QuestAcceptApiTests`** do not assert **`CompletionRewardSummary`** null on active accept embedded rows. Mapper consistency is covered indirectly via shared **`MapQuestProgressRow`**; a one-line assert on existing accept-success tests would mirror GET omit coverage. + +- Nit: Grant-order XML is present on **`MapItemGrants`** / **`MapSkillXpGrants`**; C# operator-chain test asserts fields via **`Single`** / index **0** (order-agnostic for single grants). Acceptable for prototype freeze table; Bruno gather test asserts ordered **`skillXpGrants`** array. + +## Verification + +```bash +cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~QuestProgressApiTests" +``` + +Optional: run Bruno folder `bruno/neon-sprawl-server/quest-progress/` against a running dev server (especially **`Get quest progress after gather intro complete.bru`**).