57 lines
4.6 KiB
Markdown
57 lines
4.6 KiB
Markdown
# 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` | **Matches** — implementation snapshot bullet for NEO-107 added; summary **Status** row includes **E5M3-09 / NEO-107 landed**; NEO-105 bullet cross-links NEO-107 persistence (review follow-up). |
|
|
| `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.~~ **Done.** Status row updated in `E5_M3_EncounterAndRewardTables.md`.
|
|
|
|
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.~~ **Done.** NEO-105 bullet now cross-links **`IEncounterCompleteEventStore`** (NEO-107).
|
|
|
|
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).~~ **Done.** Added `TryRecord_ShouldFailClosed_WhenEncounterIdIsEmpty`; split empty **`PlayerId`** into its own test.
|
|
|
|
## Nits
|
|
|
|
- ~~Nit: **`InMemoryEncounterCompleteEventStoreTests`** — both test methods call **`TryGet`** under **Act**; move verification reads to **Assert** for full AAA per `csharp-style.md`.~~ **Done.** **`TryGet`** moved to **Assert** in all store tests.
|
|
|
|
- Nit: **`EncounterDefinitionRegistryTests`** resolves **`IEncounterCompleteEventStore`** in **Arrange** but only asserts **`IsType<InMemoryEncounterCompleteEventStore>`** — 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.
|