# 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.~~ **Done.** `BuildSnapshot_ShouldThrow_WhenCompletedButCompleteEventMissing` + `BuildSnapshot_ShouldThrow_WhenCompletedButCompletedAtMissing` (`CompletedWithoutTimestampStore` fake).

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.~~ **Done.** Second **`GetAsync`** in completed test asserts stable state, **`completedAt`**, and grant summary.

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).~~ **Done.** `GetEncounterProgress_ShouldReturnNotFound_WhenPlayerIdIsWhitespaceOnly`.

## 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).~~ **Done.**

- 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.~~ **Done.** XML on **`MapGrantSummary`**; completed test asserts full grant array order (matches Bruno).

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