From 28e4a70846b761b6b6e3fb8117e087a088c2539b Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 18:27:10 -0400 Subject: [PATCH] =?UTF-8?q?NEO-128:=20address=20review=20=E2=80=94=20opera?= =?UTF-8?q?tor-chain=20delivery=20and=20wiring=20deny=20tests.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/NEO-128-implementation-plan.md | 4 +- docs/reviews/2026-06-07-NEO-128.md | 8 +- .../Game/Quests/QuestObjectiveWiringTests.cs | 74 +++++++++++++++++++ .../Game/Quests/QuestStateOperationsTests.cs | 45 +++++++++++ 4 files changed, 125 insertions(+), 6 deletions(-) diff --git a/docs/plans/NEO-128-implementation-plan.md b/docs/plans/NEO-128-implementation-plan.md index b540c4c..758c350 100644 --- a/docs/plans/NEO-128-implementation-plan.md +++ b/docs/plans/NEO-128-implementation-plan.md @@ -19,7 +19,7 @@ | Topic | Question | Agent recommendation | Answer | |--------|----------|----------------------|--------| | **Delivery vs mark ordering** | When should bundle apply run relative to `TryMarkComplete`? | **Deliver bundle first, then mark complete** — E7M2 “transactional equivalent” to fail-closed; mirrors NEO-105 (grants before completion mark); avoids new `TryRevertCompletion` store API. | **Adopted** — deliver-then-mark | -| **Bruno smoke** | Add Bruno requests for quest-complete reward delivery? | **Skip** — server-internal wiring only; no HTTP DTO changes (NEO-129); automated integration tests cover AC; Bruno optional per backlog because verification is store/inventory/skill assertions, not a new route. | **Adopted** — skip Bruno | +| **Bruno smoke** | Add Bruno requests for quest-complete reward delivery? | **Skip** — server-internal wiring only; no HTTP DTO changes (NEO-129); automated integration tests cover AC; Bruno optional per backlog because verification is store/inventory/skill assertions, not a new route. | **Adopted** — skip Bruno (functional smoke); **`GET /health`** Bruno added only for pre-commit hook when `*Api.cs` DI signatures changed — bundle asserts remain in `dotnet test`. | **Additional defaults (no kickoff question — settled by backlog / landed code):** @@ -58,7 +58,7 @@ - **Operations:** `QuestStateOperations.TryMarkComplete` — deliver-then-mark via `RewardRouterOperations`; idempotent `completed` skip; deny passthrough from `RewardDeliveryReasonCodes`. - **Wiring:** `QuestObjectiveWiring` + `GatherOperations` / `CraftOperations` / `EncounterCompletionOperations` / quest HTTP APIs thread reward-router dependencies. -- **Tests:** `QuestStateOperationsTests` — bundle delivery, idempotent replay, inventory-full deny; `QuestObjectiveWiringTests` — gather-intro wiring + replay no double-grant. +- **Tests:** `QuestStateOperationsTests` — gather + operator-chain bundle delivery, idempotent replay, inventory-full deny; `QuestObjectiveWiringTests` — gather-intro wiring + replay no double-grant; operator-chain wiring happy path (`survey_drone_kit` + salvage +50); wiring inventory-full stay-active on terminal advance. - **Docs:** `server/README.md`; E7.M2 module anchor + alignment register updated. ## Technical approach diff --git a/docs/reviews/2026-06-07-NEO-128.md b/docs/reviews/2026-06-07-NEO-128.md index bd6f125..d5de0ad 100644 --- a/docs/reviews/2026-06-07-NEO-128.md +++ b/docs/reviews/2026-06-07-NEO-128.md @@ -32,13 +32,13 @@ None. ## Suggestions -1. **Operator-chain happy-path delivery assertions** — The plan lists integration coverage for “operator-chain → item + skill grants.” `TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull` exercises chain quest at the operations layer (deny only). `ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet` completes via wiring but does not assert **`IRewardDeliveryStore`**, **`survey_drone_kit`**, or salvage +50. NEO-127 covers router apply in isolation; extending the wiring or operations happy-path test would lock end-to-end **`TryMarkComplete`** + item grant for the only bundle with **`itemGrants`**. +1. ~~**Operator-chain happy-path delivery assertions** — The plan lists integration coverage for “operator-chain → item + skill grants.” `TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull` exercises chain quest at the operations layer (deny only). `ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet` completes via wiring but does not assert **`IRewardDeliveryStore`**, **`survey_drone_kit`**, or salvage +50. NEO-127 covers router apply in isolation; extending the wiring or operations happy-path test would lock end-to-end **`TryMarkComplete`** + item grant for the only bundle with **`itemGrants`**.~~ **Done.** Added `TryMarkComplete_ShouldDeliverOperatorChainBundle_WhenFirstCompletion`; extended `ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet` with store, item, and skill XP asserts. -2. **Inventory-full via wiring path** — Deny is tested via direct **`TryMarkComplete`** after manual chain setup. Optional: one wiring-driven test (e.g. fill bag then complete terminal **`inventory_has_item`** step) to prove **`QuestObjectiveWiring`** best-effort ignore does not mask the deny for primary ops while quest stays **active** for retry. +2. ~~**Inventory-full via wiring path** — Deny is tested via direct **`TryMarkComplete`** after manual chain setup. Optional: one wiring-driven test (e.g. fill bag then complete terminal **`inventory_has_item`** step) to prove **`QuestObjectiveWiring`** best-effort ignore does not mask the deny for primary ops while quest stays **active** for retry.~~ **Done.** Added `ChainQuest_ShouldStayActive_WhenBagFullOnTerminalWiringComplete` (terminal advance via `TryAdvanceStep` while bag full; quest stays **active**, no store record). ## Nits -- Nit: Kickoff adopted “skip Bruno” for bundle verification, but a **`GET /health`** smoke was added under `bruno/neon-sprawl-server/quest-progress/` with docs pointing to dotnet test for bundle asserts. Harmless and consistent with backlog “optional” wording; consider a one-line plan note if authors want kickoff vs backlog Bruno policy explicit. +- ~~Nit: Kickoff adopted “skip Bruno” for bundle verification, but a **`GET /health`** smoke was added under `bruno/neon-sprawl-server/quest-progress/` with docs pointing to dotnet test for bundle asserts. Harmless and consistent with backlog “optional” wording; consider a one-line plan note if authors want kickoff vs backlog Bruno policy explicit.~~ **Done.** Plan kickoff clarifications note health-only Bruno for pre-commit hook; functional verification via `dotnet test`. - Nit: **`TryCompleteStepIfSatisfied`** discards **`TryMarkComplete`** return value (by design per plan — wiring best-effort). Inventory-full deny therefore does not surface on gather/craft/encounter envelopes; acceptable for prototype, worth remembering for NEO-131 client surfacing. @@ -53,7 +53,7 @@ dotnet test server/NeonSprawl.Server.Tests --filter "QuestStateOperationsTests|Q dotnet test server/NeonSprawl.Server.Tests --filter "FullyQualifiedName~Quests|FullyQualifiedName~GatherOperations|FullyQualifiedName~CraftOperations|FullyQualifiedName~EncounterCompletion|FullyQualifiedName~EncounterCombatWiring" ``` -All 30 quest-state/wiring tests and 141 related tests pass (verified 2026-06-07). +All 32 quest-state/wiring tests and 141 related tests pass (verified 2026-06-07; +2 after review follow-up). Optional before merge: diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs index a412e11..518342d 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestObjectiveWiringTests.cs @@ -148,12 +148,56 @@ public sealed class QuestObjectiveWiringTests // Act — step 2: field stim craft SeedStack(deps, "refined_plate_stock", 2); SeedStack(deps, "scrap_metal_bulk", 1); + var salvageXpBeforeComplete = deps.XpStore.GetXpTotals(PlayerId).GetValueOrDefault("salvage"); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); + var kitBefore = CountBagItem(beforeInventory!, "survey_drone_kit"); Assert.True(TryCraft(deps, "make_field_stim_mk0").Success); // Assert — terminal inventory_has_item (contract_handoff_token from combat intro) Assert.True(deps.ProgressStore.TryGetProgress(PlayerId, ChainQuestId, out var completed)); Assert.Equal(QuestProgressStatus.Completed, completed.Status); Assert.Equal(3, completed.CurrentStepIndex); + Assert.True(deps.DeliveryStore.TryGet(PlayerId, ChainQuestId, out var deliveryEvent)); + Assert.Single(deliveryEvent.GrantedItems); + Assert.Equal("survey_drone_kit", deliveryEvent.GrantedItems[0].ItemId); + Assert.Single(deliveryEvent.GrantedSkillXp); + Assert.Equal(50, deliveryEvent.GrantedSkillXp[0].Amount); + Assert.Equal(salvageXpBeforeComplete + 50, deps.XpStore.GetXpTotals(PlayerId).GetValueOrDefault("salvage")); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); + Assert.Equal(kitBefore + 1, CountBagItem(afterInventory!, "survey_drone_kit")); + } + + [Fact] + public async Task ChainQuest_ShouldStayActive_WhenBagFullOnTerminalWiringComplete() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + AcceptAndMarkComplete(deps, GatherQuestId); + AcceptAndMarkComplete(deps, RefineQuestId); + AcceptAndMarkComplete(deps, CombatQuestId); + SeedStack(deps, PrototypeE7M1QuestCatalogRules.ChainTerminalItemId, 1); + Assert.True(TryAccept(deps, ChainQuestId).Success); + Assert.True(deps.ProgressStore.TryUpdateObjectiveCounter(PlayerId, ChainQuestId, ChainGatherObjectiveId, 5, out _)); + Assert.True(TryAdvanceStep(deps, ChainQuestId, 1).Success); + Assert.True(deps.ProgressStore.TryUpdateObjectiveCounter(PlayerId, ChainQuestId, "chain_obj_refine", 1, out _)); + Assert.True(TryAdvanceStep(deps, ChainQuestId, 2).Success); + Assert.True(deps.ProgressStore.TryUpdateObjectiveCounter(PlayerId, ChainQuestId, "chain_obj_stim", 1, out _)); + FillRemainingBagSlots(deps); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); + var kitBefore = CountBagItem(beforeInventory!, "survey_drone_kit"); + + // Act + var advance = TryAdvanceStep(deps, ChainQuestId, 3); + + // Assert + Assert.True(advance.Success); + Assert.True(deps.ProgressStore.TryGetProgress(PlayerId, ChainQuestId, out var progress)); + Assert.Equal(QuestProgressStatus.Active, progress.Status); + Assert.Equal(3, progress.CurrentStepIndex); + Assert.False(deps.DeliveryStore.TryGet(PlayerId, ChainQuestId, out _)); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); + Assert.Equal(kitBefore, CountBagItem(afterInventory!, "survey_drone_kit")); } [Fact] @@ -335,6 +379,36 @@ public sealed class QuestObjectiveWiringTests deps.DeliveryStore, deps.TimeProvider); + private static int CountBagItem(PlayerInventorySnapshot snapshot, string itemId) + { + var total = 0; + foreach (var slot in snapshot.BagSlots) + { + if (string.Equals(slot.ItemId, itemId, StringComparison.Ordinal)) + { + total += slot.Quantity; + } + } + + return total; + } + + private static void FillRemainingBagSlots(QuestWiringTestDependencies deps) + { + deps.InventoryStore.TryGetSnapshot(PlayerId, out var snapshot); + var emptySlots = snapshot!.BagSlots.Count(static slot => slot.IsEmpty); + for (var i = 0; i < emptySlots; i++) + { + var add = PlayerInventoryOperations.TryAddStack( + PlayerId, + "survey_drone_kit", + quantity: 1, + deps.ItemRegistry, + deps.InventoryStore); + Assert.Equal(PlayerInventoryMutationKind.Applied, add.Kind); + } + } + private static void SeedStack(QuestWiringTestDependencies deps, string itemId, int quantity) { var outcome = PlayerInventoryOperations.TryAddStack( diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs index cb3bd4a..3d39198 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs @@ -306,6 +306,37 @@ public sealed class QuestStateOperationsTests Assert.Equal(firstEvent.DeliveredAt, replayEvent.DeliveredAt); } + [Fact] + public async Task TryMarkComplete_ShouldDeliverOperatorChainBundle_WhenFirstCompletion() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + AcceptAndComplete(deps, GatherQuestId); + AcceptAndComplete(deps, RefineQuestId); + AcceptAndComplete(deps, CombatQuestId); + Assert.True(TryAccept(deps, ChainQuestId).Success); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); + + // Act + var result = TryMarkComplete(deps, ChainQuestId); + + // Assert + Assert.True(result.Success); + Assert.Null(result.ReasonCode); + Assert.Equal(QuestProgressStatus.Completed, result.Snapshot!.Status); + Assert.True(deps.DeliveryStore.TryGet(PlayerId, ChainQuestId, out var deliveryEvent)); + Assert.Single(deliveryEvent.GrantedItems); + Assert.Equal("survey_drone_kit", deliveryEvent.GrantedItems[0].ItemId); + Assert.Equal(1, deliveryEvent.GrantedItems[0].Quantity); + Assert.Single(deliveryEvent.GrantedSkillXp); + Assert.Equal("salvage", deliveryEvent.GrantedSkillXp[0].SkillId); + Assert.Equal(50, deliveryEvent.GrantedSkillXp[0].Amount); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); + Assert.Equal(CountBagItem(beforeInventory!, "survey_drone_kit") + 1, CountBagItem(afterInventory!, "survey_drone_kit")); + Assert.Equal(100, deps.SkillProgressionStore.GetXpTotals(PlayerId).GetValueOrDefault("salvage")); + } + [Fact] public async Task TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull() { @@ -407,6 +438,20 @@ public sealed class QuestStateOperationsTests Assert.True(canWrite); } + private static int CountBagItem(PlayerInventorySnapshot snapshot, string itemId) + { + var total = 0; + foreach (var slot in snapshot.BagSlots) + { + if (string.Equals(slot.ItemId, itemId, StringComparison.Ordinal)) + { + total += slot.Quantity; + } + } + + return total; + } + private static void FillBag(QuestTestDependencies deps) { for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++)