don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-126: address code review — module anchor and casing test 

Add E7.M2 implementation anchor for the delivery store and assert
mixed-case TryRecord replays are rejected at the store layer.
pull/165/head
VinPropane 2026-06-07 17:33:07 -04:00
parent 3d8df939b6
commit 35fa4ad192
4 changed files with 15 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md
View File

@ -67,3 +67,5 @@ Epic 7 **Slice 2** — `reward_delivery`, `unlock_granted`; no double-claim on r
**NEO-43:** When applying scripted **skill XP** from a mission/quest reward bundle, call **`MissionRewardSkillXpGrant.GrantFromMissionReward`** (`server/NeonSprawl.Server/Game/Skills/`) — fixed **`sourceKind: mission_reward`**, **`skillId`** and **`amount`** from reward data — [NEO-43 implementation plan](../../plans/NEO-43-implementation-plan.md).
**NEO-107 (E5.M3 producer hook):** On encounter completion grant commit, **`EncounterCompletionOperations.TryCompleteAndGrant`** records **`EncounterCompleteEvent`** in **`IEncounterCompleteEventStore`** (idempotency key **`{playerId}:{encounterId}`**). Future E7.M2 **`QuestRewardBundle`** router consumes this record for **`reward_delivery`** quest credit beyond prototype **`contract_handoff_token`** item loot — comment-only hook stub; no runtime router yet. See [NEO-107 implementation plan](../../plans/NEO-107-implementation-plan.md); [server README — Encounter complete event (NEO-107)](../../../server/README.md#encounter-complete-event-record-neo-107).
**NEO-126 (E7M2-03 delivery store):** Quest completion reward delivery uses **`IRewardDeliveryStore`** + **`InMemoryRewardDeliveryStore`** in `server/NeonSprawl.Server/Game/Rewards/` — idempotent **`RewardDeliveryEvent`** per player+quest (idempotency key **`{playerId}:quest_complete:{questId}`**; grant snapshots for item + skill XP rows). Router apply and quest-state wiring land in NEO-127/NEO-128; HTTP **`completionRewardSummary`** in NEO-129. See [NEO-126 implementation plan](../../plans/NEO-126-implementation-plan.md); [server README — Reward delivery store (NEO-126)](../../../server/README.md#reward-delivery-store-neo-126).

2
docs/plans/NEO-126-implementation-plan.md
View File

@ -58,7 +58,7 @@
- **Types:** `RewardDeliveryEvent`, `RewardItemGrantApplied`, `RewardSkillXpGrantApplied`, `RewardDeliveryIds` in `Game/Rewards/`.
- **Store:** `IRewardDeliveryStore` + `InMemoryRewardDeliveryStore` (`TryRecord`, `TryGet`, dev `TryClear`).
- **DI:** `AddRewardDeliveryStore()` in `Program.cs` — in-memory singleton only (Postgres deferred per kickoff).
- **Tests:** 5 AAA tests in `InMemoryRewardDeliveryStoreTests` (idempotency, fail-closed, grant summary, host DI resolve).
- **Tests:** 5 AAA tests in `InMemoryRewardDeliveryStoreTests` (idempotency incl. mixed-case replay, fail-closed, grant summary, host DI resolve).
- **Docs:** `server/README.md` reward delivery store section; E7M2-03 backlog AC checked; alignment register updated.
- **Bruno:** `bruno/neon-sprawl-server/quest-progress/Health after reward delivery store load.bru` (host boot smoke; pre-commit gate for `Program.cs`).

7
docs/reviews/2026-06-07-NEO-126.md
View File

@ -3,6 +3,7 @@
**Date:** 2026-06-07
**Scope:** Branch `NEO-126-e7m2-reward-delivery-store` vs `origin/main` — commits `b20d9c1` … `2a8fe31`
**Base:** `origin/main`
**Follow-up:** Suggestions below are **done** (strikethrough + **Done.**).
## Verdict
@ -18,7 +19,7 @@ NEO-126 adds **`IRewardDeliveryStore`** and **`InMemoryRewardDeliveryStore`** un
|------|--------|
| `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/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-126 implementation anchor added (review follow-up). |
| `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. |
@ -32,9 +33,9 @@ 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.
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.~~ **Done.** NEO-126 paragraph under **Implementation anchor (server)** in `E7_M2_RewardAndUnlockRouter.md`.
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.
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.~~ **Done.** Third replay with mixed-case ids in `InMemoryRewardDeliveryStoreTests.TryRecord_ShouldBeIdempotent_AtStoreLayer`.
## Nits

7
server/NeonSprawl.Server.Tests/Game/Rewards/InMemoryRewardDeliveryStoreTests.cs
View File

@ -20,9 +20,16 @@ public sealed class InMemoryRewardDeliveryStoreTests
// Act
var first = store.TryRecord(deliveryEvent);
var second = store.TryRecord(deliveryEvent with { DeliveredAt = DeliveredAt.AddHours(1) });
var third = store.TryRecord(
deliveryEvent with
{
PlayerId = "DEV-LOCAL-1",
QuestId = "PROTOTYPE_QUEST_OPERATOR_CHAIN",
});
// Assert
Assert.True(first);
Assert.False(second);
Assert.False(third);
Assert.True(store.TryGet(PlayerId, QuestId, out var stored));
Assert.Equal(deliveryEvent.IdempotencyKey, stored.IdempotencyKey);
Assert.Equal(DeliveredAt, stored.DeliveredAt);