neon-sprawl/docs/reviews/2026-05-31-NEO-108.md

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

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