diff --git a/docs/plans/E7M2-prototype-backlog.md b/docs/plans/E7M2-prototype-backlog.md index e5adc69..339f756 100644 --- a/docs/plans/E7M2-prototype-backlog.md +++ b/docs/plans/E7M2-prototype-backlog.md @@ -160,7 +160,7 @@ Combat intro bundle is **skill XP only** — encounter loot already granted **`c **In scope** - `RewardRouterOperations.TryDeliverQuestCompletion` in `server/NeonSprawl.Server/Game/Quests/` (or `Game/Rewards/`). -- Item grants via **`PlayerInventoryOperations`**; skill XP via **`MissionRewardSkillXpGrant`**. +- Item grants via **`PlayerInventoryOperations`**; skill XP via **`SkillProgressionGrantOperations.TryApplyGrant`** with fixed **`mission_reward`** source kind (deny envelopes; not the fire-and-forget **`MissionRewardSkillXpGrant`** wrapper). - Deny reason codes (inventory full, unknown item/skill, xp deny). - Unit + integration tests (AAA): happy path, inventory deny, idempotent replay via store. diff --git a/docs/plans/NEO-127-implementation-plan.md b/docs/plans/NEO-127-implementation-plan.md index 6b439c3..32fe435 100644 --- a/docs/plans/NEO-127-implementation-plan.md +++ b/docs/plans/NEO-127-implementation-plan.md @@ -54,7 +54,7 @@ - **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item rollback, `IRewardDeliveryStore.TryRecord`. - **Types:** `RewardDeliveryResult`, `RewardDeliveryReasonCodes`. -- **Tests:** 7 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny). +- **Tests:** 10 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny, null bundle empty record, invalid id deny). - **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated. ## Technical approach diff --git a/docs/reviews/2026-06-07-NEO-127.md b/docs/reviews/2026-06-07-NEO-127.md index 625aea7..d9bd96c 100644 --- a/docs/reviews/2026-06-07-NEO-127.md +++ b/docs/reviews/2026-06-07-NEO-127.md @@ -17,7 +17,7 @@ NEO-127 adds **`RewardRouterOperations.TryDeliverQuestCompletion`** under `Game/ | Path | Result | |------|--------| | `docs/plans/NEO-127-implementation-plan.md` | **Matches** — kickoff decisions adopted; AC checked; reconciliation accurate. | -| `docs/plans/E7M2-prototype-backlog.md` (E7M2-04) | **Partially matches** — AC checkboxes complete; **In scope** still names **`MissionRewardSkillXpGrant`** while implementation correctly uses **`TryApplyGrant`** per plan kickoff (stale backlog wording only). | +| `docs/plans/E7M2-prototype-backlog.md` (E7M2-04) | **Matches** — AC checkboxes complete; in-scope skill XP path aligned with **`TryApplyGrant`**. | | `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-127 implementation anchor documents router apply path. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-04 router apply; row correctly stays **Planned** until NEO-132 capstone. | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required for this slice. | @@ -32,11 +32,11 @@ None. ## Suggestions -1. **Empty / null bundle test** — Kickoff decision and plan both require success with empty grant snapshots and **`TryRecord`** for null or both-empty bundles. Code handles this (skips inventory pre-flight, records empty arrays), but no test locks the behavior. A single AAA test with `bundle: null` or `new QuestRewardBundleRow([], [])` would close the gap. +1. ~~**Empty / null bundle test** — Kickoff decision and plan both require success with empty grant snapshots and **`TryRecord`** for null or both-empty bundles. Code handles this (skips inventory pre-flight, records empty arrays), but no test locks the behavior. A single AAA test with `bundle: null` or `new QuestRewardBundleRow([], [])` would close the gap.~~ **Done.** `TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull`. -2. **Invalid id deny tests** — `invalid_player_id` / `invalid_quest_id` are implemented and documented in `server/README.md`, but untested. Two small deny-path tests (empty/whitespace ids) would match the fail-closed store precedent from NEO-126. +2. ~~**Invalid id deny tests** — `invalid_player_id` / `invalid_quest_id` are implemented and documented in `server/README.md`, but untested. Two small deny-path tests (empty/whitespace ids) would match the fail-closed store precedent from NEO-126.~~ **Done.** `TryDeliverQuestCompletion_ShouldDenyInvalidPlayerId_WhenPlayerIdIsEmpty` and `…InvalidQuestId…`. -3. **Replay XP assertion** — In `TryDeliverQuestCompletion_ShouldApplyGatherIntroSkillXp_WhenBundleIsSkillOnly`, the second-call assertion reuses `salvageXp` captured before the replay `Deliver` call. Re-fetch **`GetXpTotals`** after the second call to prove no double XP grant on idempotent replay. +3. ~~**Replay XP assertion** — In `TryDeliverQuestCompletion_ShouldApplyGatherIntroSkillXp_WhenBundleIsSkillOnly`, the second-call assertion reuses `salvageXp` captured before the replay `Deliver` call. Re-fetch **`GetXpTotals`** after the second call to prove no double XP grant on idempotent replay.~~ **Done.** Gather intro test re-fetches XP after replay. ## Nits @@ -54,7 +54,7 @@ None. dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests ``` -All 7 tests pass (verified 2026-06-07). +All 10 tests pass (verified 2026-06-07; was 7 before review follow-up). Optional before merge: diff --git a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs index b6952a3..9315eae 100644 --- a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs @@ -47,7 +47,9 @@ public sealed class RewardRouterOperationsTests Assert.Equal(salvageBefore + 25, salvageXp); Assert.True(second.Success); Assert.Equal(RewardDeliveryReasonCodes.AlreadyDelivered, second.ReasonCode); - Assert.Equal(salvageBefore + 25, salvageXp); + var xpAfterReplay = deps.SkillProgressionStore.GetXpTotals(PlayerId); + Assert.True(xpAfterReplay.TryGetValue("salvage", out var salvageAfterReplay)); + Assert.Equal(salvageBefore + 25, salvageAfterReplay); } [Fact] @@ -200,10 +202,92 @@ public sealed class RewardRouterOperationsTests Assert.False(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out _)); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldRecordEmptyDelivery_WhenBundleIsNull() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var xpBefore = deps.SkillProgressionStore.GetXpTotals(PlayerId); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryBefore); + + // Act + var result = Deliver(deps, GatherQuestId, bundle: null, TimeProvider.System); + + // Assert + Assert.True(result.Success); + Assert.Null(result.ReasonCode); + Assert.Empty(result.GrantedItems); + Assert.Empty(result.GrantedSkillXp); + Assert.NotNull(result.DeliveryEvent); + Assert.True(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out var stored)); + Assert.Empty(stored.GrantedItems); + Assert.Empty(stored.GrantedSkillXp); + Assert.Equal(xpBefore, deps.SkillProgressionStore.GetXpTotals(PlayerId)); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryAfter); + Assert.Equal(TotalBagQuantity(inventoryBefore!), TotalBagQuantity(inventoryAfter!)); + } + + [Fact] + public async Task TryDeliverQuestCompletion_ShouldDenyInvalidPlayerId_WhenPlayerIdIsEmpty() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]); + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + " ", + GatherQuestId, + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + deps.DeliveryStore, + TimeProvider.System); + + // Assert + Assert.False(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.InvalidPlayerId, result.ReasonCode); + Assert.False(deps.DeliveryStore.TryGet(" ", GatherQuestId, out _)); + } + + [Fact] + public async Task TryDeliverQuestCompletion_ShouldDenyInvalidQuestId_WhenQuestIdIsEmpty() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]); + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + PlayerId, + " ", + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + deps.DeliveryStore, + TimeProvider.System); + + // Assert + Assert.False(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.InvalidQuestId, result.ReasonCode); + Assert.False(deps.DeliveryStore.TryGet(PlayerId, " ", out _)); + } + private static RewardDeliveryResult Deliver( RewardRouterTestDependencies deps, string questId, - QuestRewardBundleRow bundle, + QuestRewardBundleRow? bundle, TimeProvider timeProvider) => RewardRouterOperations.TryDeliverQuestCompletion( PlayerId, @@ -259,6 +343,20 @@ public sealed class RewardRouterOperationsTests return total; } + private static int TotalBagQuantity(PlayerInventorySnapshot snapshot) + { + var total = 0; + foreach (var slot in snapshot.BagSlots) + { + if (!slot.IsEmpty) + { + total += slot.Quantity; + } + } + + return total; + } + private sealed record RewardRouterTestDependencies( IItemDefinitionRegistry ItemRegistry, IPlayerInventoryStore InventoryStore,