From 36adfd18977f7edacf0a33f4caf59e926ccb05c5 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 17:28:43 -0400 Subject: [PATCH] NEO-69: Fix Bugbot pre-flight and output rollback issues. Simulate input removal before output capacity checks, roll back partial outputs on store-missing, and add tests for slot-freeing craft success. --- docs/plans/NEO-69-implementation-plan.md | 2 +- docs/reviews/2026-05-24-NEO-69.md | 6 +++ .../Game/Crafting/CraftOperationsTests.cs | 35 +++++++++++++- .../Game/Crafting/CraftOperations.cs | 9 ++++ .../Game/Items/PlayerInventoryOperations.cs | 48 +++++++++++++++++++ 5 files changed, 97 insertions(+), 3 deletions(-) diff --git a/docs/plans/NEO-69-implementation-plan.md b/docs/plans/NEO-69-implementation-plan.md index 209aa88..7c2c02b 100644 --- a/docs/plans/NEO-69-implementation-plan.md +++ b/docs/plans/NEO-69-implementation-plan.md @@ -65,7 +65,7 @@ 4. **Pre-flight inputs (read-only):** For each input row, required amount = **`row.Quantity * quantity`**. Sum held quantity in the correct container (bag vs equipment via catalog **`inventorySlotKind`**) from current snapshot. Any shortfall → **`insufficient_materials`** without mutations. -5. **Pre-flight outputs (read-only, kickoff decision):** Clone current snapshot; for each output row with scaled quantity, simulate **`TryAddStack`** placement rules (merge + fill empty slots, catalog **`stackMax`**, equipment routing) on the clone. If any output cannot be fully placed → **`inventory_full`** without mutations. Implementation: add **`PlayerInventoryOperations.CanPlaceStack(snapshot, itemId, quantity, registry)`** (or equivalent simulate helper) to avoid duplicating merge/fill logic. +5. **Pre-flight outputs (read-only, kickoff decision):** Clone current snapshot; **simulate removing all scaled inputs** on the clone, then simulate each scaled output add. If any output cannot be fully placed → **`inventory_full`** without mutations. Implementation: **`TrySimulateRemoveStack`** + **`TrySimulateAddStack`** on `PlayerInventoryOperations`. 6. **Commit mutations (only after steps 4–5 pass):** - **Remove inputs:** foreach input row, **`TryRemoveStack`** scaled quantity. (Should succeed given pre-flight; defensive deny **`insufficient_materials`** if not.) diff --git a/docs/reviews/2026-05-24-NEO-69.md b/docs/reviews/2026-05-24-NEO-69.md index cefb0d0..9b0d753 100644 --- a/docs/reviews/2026-05-24-NEO-69.md +++ b/docs/reviews/2026-05-24-NEO-69.md @@ -49,6 +49,12 @@ None. - Nit: Compensating helpers discard rollback outcomes (`_ = TryAddStack` / `TryRemoveStack`) — same as gather engine; acceptable for prototype. +## Bugbot follow-up (PR #103) + +1. ~~**Output pre-flight ignores input removal** — simulate output placement on snapshot with inputs still present; false `inventory_full` when input removal would free slots.~~ **Done.** Pre-flight now **`TrySimulateRemoveStack`** all inputs on clone before output simulation; test **`TryCraft_WhenBagFullWithScrapInputs_ShouldSucceedByFreeingInputSlots`**. + +2. ~~**Missing output rollback in store-missing branch** — output-add `StoreMissing` restored inputs but not partial outputs.~~ **Done.** `CompensatingRemoveOutputs` added before input restore on that path. + ## Verification ```bash diff --git a/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs index a7e5d2b..51d2129 100644 --- a/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs @@ -119,14 +119,13 @@ public sealed class CraftOperationsTests } [Fact] - public void TryCraft_WhenBagFullForOutput_ShouldDenyWithInventoryFullWithoutConsumingInputs() + public void TryCraft_WhenBagFullWithScrapInputs_ShouldSucceedByFreeingInputSlots() { // Arrange using var factory = new InMemoryWebApplicationFactory(); var deps = ResolveCraftDependencies(factory); SeedStack(deps, "scrap_metal_bulk", 5); FillBagExcept(deps, occupiedSlots: 1); - deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); // Act var result = CraftOperations.TryCraft( @@ -141,10 +140,42 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); + // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); + Assert.True(result.Success); + Assert.Equal(0, CountBagItem(inventory!, "scrap_metal_bulk")); + Assert.Equal(1, CountBagItem(inventory!, "refined_plate_stock")); + } + + [Fact] + public void TryCraft_MakePrototypeArmor_WhenEquipmentOccupied_ShouldDenyWithInventoryFull() + { + // Arrange + using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveCraftDependencies(factory); + SeedStack(deps, "refined_plate_stock", 8); + SeedStack(deps, "scrap_metal_bulk", 5); + SeedStack(deps, "prototype_armor_shell", 1); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory); + + // Act + var result = CraftOperations.TryCraft( + PlayerId, + "make_prototype_armor", + quantity: 1, + deps.RecipeRegistry, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.XpStore, + deps.LevelCurve, + deps.PerkUnlockEngine); + // Assert deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); Assert.False(result.Success); Assert.Equal(CraftReasonCodes.InventoryFull, result.ReasonCode); + Assert.Equal(8, CountBagItem(afterInventory!, "refined_plate_stock")); Assert.Equal(5, CountBagItem(afterInventory!, "scrap_metal_bulk")); Assert.Equal(TotalBagQuantity(beforeInventory!), TotalBagQuantity(afterInventory!)); } diff --git a/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs b/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs index 969cd90..9f3000f 100644 --- a/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs +++ b/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs @@ -57,6 +57,14 @@ public static class CraftOperations } var simulated = CloneSnapshot(snapshot); + foreach (var input in scaledInputs) + { + if (!PlayerInventoryOperations.TrySimulateRemoveStack(ref simulated, input.ItemId, input.Quantity, itemRegistry)) + { + return Deny(CraftReasonCodes.InsufficientMaterials); + } + } + foreach (var output in scaledOutputs) { if (!PlayerInventoryOperations.TrySimulateAddStack(ref simulated, output.ItemId, output.Quantity, itemRegistry)) @@ -102,6 +110,7 @@ public static class CraftOperations if (addOutcome.Kind == PlayerInventoryMutationKind.StoreMissing) { + CompensatingRemoveOutputs(playerId, outputsApplied, itemRegistry, inventoryStore); CompensatingRestoreInputs(playerId, scaledInputs, itemRegistry, inventoryStore); return Deny(CraftReasonCodes.InventoryStoreMissing); } diff --git a/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs b/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs index 652a8e8..4e73da8 100644 --- a/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs +++ b/server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs @@ -187,6 +187,54 @@ public static class PlayerInventoryOperations return true; } + /// + /// Simulates removing from a snapshot clone without persisting (NEO-69 craft pre-flight). + /// + public static bool TrySimulateRemoveStack( + ref PlayerInventorySnapshot snapshot, + string itemId, + int quantity, + IItemDefinitionRegistry registry) + { + if (quantity <= 0) + { + return false; + } + + var lookup = itemId.Trim(); + if (lookup.Length == 0 || !registry.TryGetDefinition(lookup, out var def)) + { + return false; + } + + var container = PlayerInventorySnapshot.ContainerFromSlotKind(def.InventorySlotKind); + var slots = PlayerInventorySnapshot.CloneSlots(snapshot.GetSlots(container)); + if (CountQuantity(slots, def.Id) < quantity) + { + return false; + } + + var remaining = quantity; + for (var i = 0; i < slots.Length && remaining > 0; i++) + { + var slot = slots[i]; + if (slot.IsEmpty || !string.Equals(slot.ItemId, def.Id, StringComparison.Ordinal)) + { + continue; + } + + var take = Math.Min(remaining, slot.Quantity); + var left = slot.Quantity - take; + slots[i] = left <= 0 + ? new InventorySlotState(slot.SlotIndex, null, 0) + : new InventorySlotState(slot.SlotIndex, def.Id, left); + remaining -= take; + } + + snapshot = snapshot.WithSlots(container, slots); + return true; + } + private static PlayerInventoryMutationOutcome Deny(string playerId, IPlayerInventoryStore store, string reasonCode) { // --- Telemetry hook site (NEO-56): future E9.M1 catalog event `inventory_transfer_denied` ---