54 lines
4.0 KiB
Markdown
54 lines
4.0 KiB
Markdown
# 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`).
|