diff --git a/docs/reviews/2026-05-31-NEO-108.md b/docs/reviews/2026-05-31-NEO-108.md new file mode 100644 index 0000000..8e26cfa --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-108.md @@ -0,0 +1,62 @@ +# Code review — NEO-108 (E5M3-08) + +**Date:** 2026-05-31 +**Scope:** Branch `NEO-108-get-encounter-progress-wire-dtos-bruno` vs `51e2007` (merge-base on `main`) — commits `1601858` … `07ebb9e` +**Base:** `51e2007` (main at branch point, post NEO-107 merge) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-108 adds **`GET /game/players/{id}/encounter-progress`** — a versioned read model projecting catalog encounters from **`IEncounterDefinitionRegistry`**, **`IEncounterProgressStore`**, **`IEncounterCompletionStore`**, and **`IEncounterCompleteEventStore`**, with the same **`IPositionStateStore`** 404 gate as gig/skill progression. **`EncounterProgressApi.BuildSnapshot`** is internal and testable; state derivation (`inactive` / `active` / `completed`), ordinal-sorted **`defeatedTargetIds`**, and commit-time **`rewardGrantSummary`** match the kickoff plan. Five AAA integration tests cover 404, inactive default, 1- and 2-defeat active rows, and completed + grant summary; Bruno adds inactive smoke and a three-NPC defeat spine with intermediate GET assertions and idempotent second GET. Docs (plan, backlog E5M3-08, E5.M3 snapshot, alignment register, module register, `server/README.md`) are updated. Godot poll client remains correctly deferred to **NEO-110**. Risk is low: read-only projection with fail-fast invariant checks on completed rows. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-108-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; frozen row progression table reflected in code, tests, and Bruno. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-08) | **Matches** — acceptance criteria checked; landed note cites plan; client counterpart **NEO-110** called out. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — Status row includes **E5M3-08 / NEO-108**; implementation snapshot bullet added with README + Bruno links. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-108** per-player GET. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-108** GET landed. | +| Full-stack epic decomposition | **N/A** — plan explicitly defers client counterpart to **NEO-110**; no prototype slice completion claim. | +| `server/README.md` | **Matches** — new **Per-player encounter progress (NEO-108)** section; NEO-106 section cross-links GET for loot verification. | + +## Blocking issues + +None. + +## Suggestions + +1. **Invariant violation tests for `BuildSnapshot`** — Plan adopts fail-fast **`InvalidOperationException`** when **`completed`** but **`completedAt`** or **`EncounterCompleteEvent`** is missing. No direct unit test exercises these paths (would require a small fake/mocked store pair). A focused **`BuildSnapshot`** test would lock the contract without a full defeat spine. + +2. **Idempotent second GET in C# tests** — Bruno **`Get encounter progress after defeat spine.bru`** asserts a second GET stays **`completed`** with unchanged grant summary; the C# suite stops after one GET on the completed case. A second **`GetAsync`** in the completed test (or a dedicated test) would mirror Bruno at the integration layer. + +3. **Whitespace / empty player id 404** — Route trims **`id`** and returns 404 when empty; only **`missing-player`** is tested. A **`/game/players/%20/encounter-progress`** (or `""` after trim) case would match the documented gate explicitly (same gap exists on gig progression — low priority). + +## Nits + +- Nit: **`MapRow`** calls **`progressStore.TryGetProgress`** twice on the **`active`** path (once for **`defeatedTargetIds`**, again for **`Started`**). A single **`TryGet`** with reused **`progress`** variable would avoid the duplicate lookup (prototype catalog size = 1; cosmetic only). + +- Nit: **`EncounterProgressApiTests`** defeat helper binds hotbar **slot 0**; Bruno spine uses **slot 3** per plan (cooldown isolation). Both are fine in isolation; no change required unless slot 0 conflicts emerge in combined Bruno runs. + +- Nit: **`MapGrantSummary`** preserves reward-table grant order (scrap then token). C# completed test asserts quantities via **`Single(g => g.ItemId == …)`** (order-agnostic); Bruno asserts array order. Document or test order explicitly if clients will diff arrays. + +## Verification + +```bash +# NEO-108-focused (5 tests — passed locally) +cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~EncounterProgressApiTests" + +# Encounters module parity (79 tests — passed locally) +cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~Encounters" + +# Bruno (server running on baseUrl) +# bruno/neon-sprawl-server/encounter-progress/ +``` + +**Local run:** 5/5 `EncounterProgressApiTests` passed; 79/79 `Encounters` filter passed.