NEO-128: address review — operator-chain delivery and wiring deny tests.
parent
f3b4438968
commit
28e4a70846
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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++)
|
||||
|
|
|
|||
Loading…
Reference in New Issue