diff --git a/docs/reviews/2026-06-07-NEO-126.md b/docs/reviews/2026-06-07-NEO-126.md new file mode 100644 index 0000000..4f67fe6 --- /dev/null +++ b/docs/reviews/2026-06-07-NEO-126.md @@ -0,0 +1,53 @@ +# Code review — NEO-126 (E7M2-03) + +**Date:** 2026-06-07 +**Scope:** Branch `NEO-126-e7m2-reward-delivery-store` vs `origin/main` — commits `b20d9c1` … `2a8fe31` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** — store contract, threading model, and tests mirror the NEO-107 encounter-event precedent; plan and backlog AC are satisfied; risk is low (infrastructure-only, no router or HTTP surface yet). + +## Summary + +NEO-126 adds **`IRewardDeliveryStore`** and **`InMemoryRewardDeliveryStore`** under `Game/Rewards/` for idempotent quest-completion reward delivery records. **`RewardDeliveryEvent`** carries item and skill XP grant snapshots plus a stable **`{playerId}:quest_complete:{questId}`** idempotency key; **`RewardDeliveryIds`** handles normalization and composite store keys. DI registers an in-memory singleton via **`AddRewardDeliveryStore()`** in **`Program.cs`**. Five AAA unit tests cover idempotency, fail-closed empty ids, grant summary readability, and host DI resolution; Bruno adds a health smoke for **`Program.cs`** pre-commit. Docs (implementation plan, E7M2-03 backlog AC, alignment register, **`server/README.md`**) are updated. No player-visible behavior until NEO-127 wires the router. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-126-implementation-plan.md` | **Matches** — kickoff (in-memory only), acceptance criteria checked, reconciliation accurate. | +| `docs/plans/E7M2-prototype-backlog.md` (E7M2-03) | **Matches** — AC checkboxes complete; infrastructure-only client counterpart noted. | +| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Partially matches** — freeze idempotency key and **`RewardDeliveryEvent`** contract align; no NEO-126 implementation anchor yet (NEO-125 added a server-load paragraph; NEO-107 has encounter-store anchor). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-03 delivery store; row correctly stays **Planned** until NEO-132 capstone. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required for this slice. | +| `server/README.md` | **Matches** — reward delivery store section documents interface, event fields, idempotency key, NEO-127/NEO-129 pointers. | +| Full-stack epic decomposition | **N/A** — E7M2-03 is infrastructure-only; no client counterpart required. | + +**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment update is appropriate. + +## Blocking issues + +None. + +## Suggestions + +1. **E7.M2 module anchor** — Add a short **Implementation anchor (server)** bullet for NEO-126 in `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` (parity with the NEO-125 server-load and NEO-107 encounter-store paragraphs) so decomposition readers do not rely only on the alignment register and README. + +2. **Case-normalization idempotency test** — `MakeStoreKey` lowercases ids, but no test proves a second `TryRecord` with different casing of the same player/quest is rejected. A one-liner extension of `TryRecord_ShouldBeIdempotent_AtStoreLayer` (e.g. replay with `PlayerId = "DEV-LOCAL-1"`) would lock that behavior for NEO-127 replays. + +## Nits + +- Nit: `TryRecord` does not validate that `deliveryEvent.IdempotencyKey` matches `RewardDeliveryIds.MakeIdempotencyKey(...)` — same as `InMemoryEncounterCompleteEventStore`; NEO-127 router should populate both consistently. +- Nit: `TryClear` is untested; acceptable for dev-only API (encounter store also omits a `TryClear` test). +- Nit: Stored `PlayerId` / `QuestId` on the event are not rewritten to normalized form at record time; lookup normalizes via `MakeStoreKey`. Router (NEO-127) should pass normalized ids per plan table. + +## Verification + +```bash +dotnet test server/NeonSprawl.Server.Tests --filter InMemoryRewardDeliveryStoreTests +``` + +Optional before merge: + +- Boot dev server; confirm `/health` 200 after `AddRewardDeliveryStore()` wiring (Bruno: `bruno/neon-sprawl-server/quest-progress/Health after reward delivery store load.bru`).