diff --git a/docs/reviews/2026-05-31-NEO-107.md b/docs/reviews/2026-05-31-NEO-107.md new file mode 100644 index 0000000..5f08e3a --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-107.md @@ -0,0 +1,56 @@ +# Code review — NEO-107 (E5M3-09) + +**Date:** 2026-05-31 +**Scope:** Branch `NEO-107-encounter-complete-event-e7m2-hook` vs `856dc8d` (merge-base on `main`) — commits `986cf55` … `555c323` +**Base:** `856dc8d` (main at branch point, post NEO-106 merge) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-107 adds **`IEncounterCompleteEventStore`** and **`InMemoryEncounterCompleteEventStore`** — a dedicated, idempotent per-player+encounter record for **`EncounterCompleteEvent`**, mirroring the NEO-104 split-store pattern (ConcurrentDictionary + per-key locks). **`EncounterCompletionOperations.TryCompleteAndGrant`** records the event immediately after a successful **`TryMarkCompleted`**, with expanded E7.M2 **`reward_delivery`** hook comments and no runtime router behavior. The store is registered in **`AddEncounterAndRewardCatalogs`** and threaded through **`EncounterCombatWiring`** → **`AbilityCastApi`**. Tests cover store idempotency, all completion deny paths (store stays empty), wiring third-defeat success, and host DI resolution. Docs (plan, backlog E5M3-09, E5.M3 snapshot, E7.M2 anchor, alignment register, module register, `server/README.md`) are updated. HTTP projection and Godot HUD remain correctly deferred to NEO-108 / NEO-110. Risk is low: append-only record on an already-idempotent completion path; **`TryRecord`** false-after-fresh-mark is documented as benign idempotent success. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-107-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; frozen completion→event flow table reflected in code and tests. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-09) | **Matches** — acceptance criteria checked; landed note cites plan. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — implementation snapshot bullet for NEO-107 added; summary **Status** row (line 10) still omits **E5M3-09 / NEO-107 landed**; NEO-105 bullet still says “event persistence deferred to NEO-107” (now landed). | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-107 producer hook anchor added with correct consumer contract vocabulary. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-107** event record. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-107** event store landed. | +| Full-stack epic decomposition | **N/A** — plan explicitly defers client counterpart to **NEO-110** via NEO-108; no prototype slice completion claim. | +| `server/README.md` | **Matches** — new **Encounter complete event record (NEO-107)** section is accurate; NEO-105 section updated to reflect persistence. | + +## Blocking issues + +None. + +## Suggestions + +1. **E5.M3 summary Status row** — Update the module summary table (line 10) to include **E5M3-09 / NEO-107** alongside the other landed slices so the header matches the implementation snapshot bullets below. + +2. **Stale NEO-105 deferral line** — In `E5_M3_EncounterAndRewardTables.md`, the NEO-105 bullet still reads “event persistence deferred to NEO-107.” Reword to point at the NEO-107 snapshot (or drop the deferral phrase) now that persistence is landed. + +3. **Empty encounter id fail-closed test** — Plan test table lists “empty key / invalid ids fail closed.” `InMemoryEncounterCompleteEventStoreTests` covers whitespace **`PlayerId`** only; a symmetric case with empty **`EncounterId`** would close the gap (store layer only). + +## Nits + +- Nit: **`InMemoryEncounterCompleteEventStoreTests`** — both test methods call **`TryGet`** under **Act**; move verification reads to **Assert** for full AAA per `csharp-style.md`. + +- Nit: **`EncounterDefinitionRegistryTests`** resolves **`IEncounterCompleteEventStore`** in **Arrange** but only asserts **`IsType`** — sufficient for DI smoke; no functional **`TryRecord`** assertion needed unless you want parity with progress/completion store tests. + +## Verification + +```bash +# NEO-107-focused (73 tests — passed locally) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters" + +# CI parity +python3 scripts/validate_content.py +``` + +**Local run:** 73/73 `Encounters` filter tests passed.