diff --git a/docs/reviews/2026-06-07-NEO-120.md b/docs/reviews/2026-06-07-NEO-120.md new file mode 100644 index 0000000..d79dbfa --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-120.md @@ -0,0 +1,67 @@ +# Code review — NEO-120 (E7M1-09) + +**Date:** 2026-06-07 +**Scope:** Branch `NEO-120-quest-accept-post` vs `origin/main` — commits `b22b68a` … `9fe6cb3` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-120 adds **`POST /game/players/{id}/quests/{questId}/accept`** — a thin HTTP wrapper over landed **`QuestStateOperations.TryAccept`**. The handler mirrors **`QuestProgressApi`** (trim id → position-store **404**), accepts optional v1 body with **`schemaVersion` 0 treated as omitted**, and returns a versioned **`QuestAcceptResponse`** (`accepted`, optional `reasonCode`, optional **`quest`** row) with **HTTP 200** on structured denies (inventory/craft precedent). Row projection is shared via extracted **`QuestProgressApi.MapQuestProgressRow(QuestStepState)`** overloads so GET and POST stay aligned. Eight AAA integration tests cover player 404 gates, happy accept (with GET confirmation), omitted body, invalid schema, prerequisite deny, duplicate accept snapshot, and unknown quest. Three Bruno accept spine requests complement the existing quest-progress folder. Plan reconciliation, **`server/README.md`**, and E7.M1 decomposition docs are updated. Server-only slice — Godot accept HUD correctly deferred to NEO-122. Risk is low; no blocking correctness or contract issues found. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-120-implementation-plan.md` | **Matches** — kickoff decisions adopted (`accepted` naming, shared mapper, optional body v1); acceptance checklist checked; reconciliation section accurate. | +| `docs/plans/E7M1-prototype-backlog.md` (E7M1-09) | **Matches** — acceptance checkboxes checked; client counterpart NEO-122 noted. | +| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — NEO-120 accept POST bullet + README/Bruno links. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M1-09 / NEO-120 landed note appended. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — NEO-120 note appended. | +| `server/README.md` | **Matches** — route, optional body rules, response table, curl sample, wiring note. | +| 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 client capstone (NEO-123); no further register status change required for this merge. + +## Blocking issues + +(none) + +## Suggestions + +1. **HTTP test for `already_completed` deny with snapshot** — The plan response table and README document **`already_completed`** returning **`accepted: false`** plus a **`quest`** row. Integration tests cover **`already_active`** and operations-layer **`TryAccept_ShouldDenyAlreadyCompleted`** exists in `QuestStateOperationsTests`, but no HTTP test exercises **`MapResponse`** for a completed row (status **`completed`**, optional **`completedAt`**). A single test completing gather intro then re-accepting would lock the wire contract for NEO-122. + +2. **Empty `{}` body test** — README documents `{}` as valid (default **`schemaVersion` 0**). Tests cover omitted body and explicit **`schemaVersion: 1`**; a one-liner POST with **`StringContent("{}")`** would close the loop on the documented third valid shape. + +## Nits + +- Nit: **`MapQuestProgressRow(QuestStepState)`** throws **`InvalidOperationException`** on unknown **`QuestProgressStatus`** (would surface as 500). Inherited defensive guard from NEO-119; acceptable for prototype. + +- Nit: **`PostQuestAccept_ShouldDenyAlreadyActive_WhenQuestAlreadyAccepted`** asserts on the first accept response inside **Arrange** (lines 159–162). Works fine; optional move of **`firstBody`** checks into **Assert** if strict AAA phase purity is desired. + +- Nit: Bruno **`Accept duplicate.bru`** pre-request re-accepts gather intro (idempotent when seq 3 ran). Correct for standalone runs; collection order matters only when hitting a persistent dev server without reset. + +## Verification + +```bash +# Primary signal for this story +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~QuestAcceptApiTests" + +# 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 the three accept requests in `bruno/neon-sprawl-server/quest-progress/` (seq 3–5) against a local server with fresh `dev-local-1`. + +**Reviewer note:** All 8 `QuestAcceptApiTests` and 93 quest-namespace tests passed locally during review. + +**Manual:** None required beyond Bruno for this server-only story; Godot accept binding verification belongs to NEO-122.