diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 11f28d0..074fb85 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -43,7 +43,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen | Module ID | Module Name | Depends On | Contract Needed | Phase Required | Status | |---|---|---|---|---|---| | E3.M1 | ResourceNodeAndGatherLoop | E1.M3, E2.M2 | ResourceNodeDef, GatherResult, ResourceYieldTable | Prototype | Ready | -| E3.M2 | RefinementAndRecipeExecution | E3.M1, E3.M3 | RecipeDef, CraftRequest, CraftResult | Prototype | Planned | +| E3.M2 | RefinementAndRecipeExecution | E3.M1, E3.M3 | RecipeDef, CraftRequest, CraftResult | Prototype | In Progress | | E3.M3 | ItemizationAndInventorySchema | None | ItemDef, ItemInstance, InventorySlot | Prototype | In Progress | | E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned | | E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned | diff --git a/docs/reviews/2026-05-24-NEO-69.md b/docs/reviews/2026-05-24-NEO-69.md index 22b7b3d..cefb0d0 100644 --- a/docs/reviews/2026-05-24-NEO-69.md +++ b/docs/reviews/2026-05-24-NEO-69.md @@ -20,12 +20,12 @@ The branch adds **`CraftOperations.TryCraft`** and supporting **`CraftResult`** | [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) · **E3M2-05** | **Matches** — acceptance criteria checked; landed note present. | | [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — implementation snapshot + **`CraftResult`** field table; NEO-69 craft engine documented. | | [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row notes NEO-69 landed. | -| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M2 **Status** still **Planned** while module doc / alignment say **In Progress** (optional register bump when Slice 3 spine completes). | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M2 **Status** updated to **In Progress** (was **Planned**). | | [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** (planned) — server-authoritative inventory mutation; no client in this story. | | [NEO-62 / NEO-64 reference — `GatherOperations`](../../server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs) | **Matches** — same **`SkillProgressionGrantOperations.TryApplyGrant`** + compensating rollback pattern; craft correctly uses **`TryApplyGrant`** (not void **`RefineActivitySkillXpGrant`**) so XP deny can roll back inventory. | | Full-stack epic decomposition | **Matches** — NEO-74 client counterpart linked; no claim of prototype-complete player-visible craft. | -Register/tracking: optional E3.M2 **Status** → **In Progress** in dependency register when authors next touch alignment docs. +Register/tracking: ~~optional E3.M2 **Status** → **In Progress** in dependency register when authors next touch alignment docs.~~ **Done.** E3.M2 **Status** is **In Progress** in `module_dependency_register.md`. ## Blocking issues @@ -33,19 +33,19 @@ None. ## Suggestions -1. **AAA — move verification reads to Assert** — In `CraftOperationsTests`, `TryGetSnapshot` / `GetXpTotals` after `TryCraft` sit in the **Act** block; [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) requires outcome verification reads in **Assert** (Act = SUT invocation only). Mirror the rule even though `GatherOperationsTests` has the same legacy layout. +1. ~~**AAA — move verification reads to Assert** — In `CraftOperationsTests`, `TryGetSnapshot` / `GetXpTotals` after `TryCraft` sit in the **Act** block; [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) requires outcome verification reads in **Assert** (Act = SUT invocation only). Mirror the rule even though `GatherOperationsTests` has the same legacy layout.~~ **Done.** Verification reads moved to **Assert** in all `CraftOperationsTests` methods. -2. **Test `quantity > 1` scaling** — Plan and kickoff adopt batch **`quantity`** with scaled I/O. Add one test (e.g. **`refine_scrap_standard`** at **`quantity: 2`**) asserting 10× scrap consumed and 2× plate granted. +2. ~~**Test `quantity > 1` scaling** — Plan and kickoff adopt batch **`quantity`** with scaled I/O. Add one test (e.g. **`refine_scrap_standard`** at **`quantity: 2`**) asserting 10× scrap consumed and 2× plate granted.~~ **Done.** `TryCraft_RefineScrapStandard_AtQuantityTwo_ShouldScaleInputsAndOutputs`. -3. **Defensive rollback on partial input removal** — If **`TryRemoveStack`** fails mid-loop after earlier inputs were removed (e.g. **`inventory_store_missing`**), **`TryCraft`** returns deny without restoring already-removed inputs. Pre-flight makes **`Denied`** unlikely; consider compensating restore for symmetry with output/XP rollback paths. +3. ~~**Defensive rollback on partial input removal** — If **`TryRemoveStack`** fails mid-loop after earlier inputs were removed (e.g. **`inventory_store_missing`**), **`TryCraft`** returns deny without restoring already-removed inputs. Pre-flight makes **`Denied`** unlikely; consider compensating restore for symmetry with output/XP rollback paths.~~ **Done.** Input loop tracks **`inputsRemoved`** and calls **`CompensatingRestoreInputs`** on mid-loop deny. -4. **Optional `TrySimulateAddStack` unit test** — Plan listed an optional dedicated test in **`PlayerInventoryOperationsTests`**; one bag-full vs equipment-route case would lock pre-flight simulation to **`TryAddStack`** behavior without a full craft scenario. +4. ~~**Optional `TrySimulateAddStack` unit test** — Plan listed an optional dedicated test in **`PlayerInventoryOperationsTests`**; one bag-full vs equipment-route case would lock pre-flight simulation to **`TryAddStack`** behavior without a full craft scenario.~~ **Done.** Two tests: bag-full deny for bag item; equipment route succeeds when bag full. ## Nits -- Nit: **`module_dependency_register.md`** E3.M2 **Status** (**Planned**) lags **`E3_M2_RefinementAndRecipeExecution.md`** (**In Progress**). +- ~~Nit: **`module_dependency_register.md`** E3.M2 **Status** (**Planned**) lags **`E3_M2_RefinementAndRecipeExecution.md`** (**In Progress**).~~ **Done.** -- Nit: No test for whitespace-only **`recipeId`** (maps to **`unknown_recipe`** via trim — fine behavior, low priority). +- ~~Nit: No test for whitespace-only **`recipeId`** (maps to **`unknown_recipe`** via trim — fine behavior, low priority).~~ **Done.** `TryCraft_ForWhitespaceRecipeId_ShouldDenyWithUnknownRecipe`. - Nit: Compensating helpers discard rollback outcomes (`_ = TryAddStack` / `TryRemoveStack`) — same as gather engine; acceptable for prototype. @@ -59,4 +59,6 @@ dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj **Results (2026-05-24):** all **8** `CraftOperationsTests` pass. Full suite reported **2** failures in `SkillProgressionGrantApiTests` (unrelated to this branch; NEO-69 diff does not touch that file). +**Results (post-review follow-up):** **11** `CraftOperationsTests` + **2** new `TrySimulateAddStack` tests pass; full suite **328** tests pass. + Manual: none required for engine-only story — NEO-70 owns HTTP / Bruno verification. diff --git a/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs index c0ccf5a..a7e5d2b 100644 --- a/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Crafting/CraftOperationsTests.cs @@ -33,10 +33,9 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); + // Assert deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); var refineXp = deps.XpStore.GetXpTotals(PlayerId).GetValueOrDefault("refine"); - - // Assert Assert.True(result.Success); Assert.Null(result.ReasonCode); Assert.Single(result.InputsConsumed); @@ -54,6 +53,38 @@ public sealed class CraftOperationsTests Assert.Equal(10, refineXp); } + [Fact] + public void TryCraft_RefineScrapStandard_AtQuantityTwo_ShouldScaleInputsAndOutputs() + { + // Arrange + using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveCraftDependencies(factory); + SeedStack(deps, "scrap_metal_bulk", 10); + + // Act + var result = CraftOperations.TryCraft( + PlayerId, + "refine_scrap_standard", + quantity: 2, + deps.RecipeRegistry, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.XpStore, + deps.LevelCurve, + deps.PerkUnlockEngine); + + // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); + Assert.True(result.Success); + Assert.Single(result.InputsConsumed); + Assert.Equal(10, result.InputsConsumed[0].Quantity); + Assert.Single(result.OutputsGranted); + Assert.Equal(2, result.OutputsGranted[0].Quantity); + Assert.Equal(0, CountBagItem(inventory!, "scrap_metal_bulk")); + Assert.Equal(2, CountBagItem(inventory!, "refined_plate_stock")); + } + [Fact] public void TryCraft_WhenMaterialsMissing_ShouldDenyWithInsufficientMaterialsWithoutMutatingInventory() { @@ -75,10 +106,9 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); + // Assert deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); var refineXp = deps.XpStore.GetXpTotals(PlayerId).GetValueOrDefault("refine"); - - // Assert Assert.False(result.Success); Assert.Equal(CraftReasonCodes.InsufficientMaterials, result.ReasonCode); Assert.Empty(result.InputsConsumed); @@ -111,9 +141,8 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); - deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); - // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); Assert.False(result.Success); Assert.Equal(CraftReasonCodes.InventoryFull, result.ReasonCode); Assert.Equal(5, CountBagItem(afterInventory!, "scrap_metal_bulk")); @@ -143,9 +172,8 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); - deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); - // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); Assert.True(result.Success); Assert.Equal("prototype_armor_shell", inventory!.EquipmentSlots[0].ItemId); Assert.Equal(1, inventory.EquipmentSlots[0].Quantity); @@ -178,6 +206,31 @@ public sealed class CraftOperationsTests Assert.Equal(CraftReasonCodes.UnknownRecipe, result.ReasonCode); } + [Fact] + public void TryCraft_ForWhitespaceRecipeId_ShouldDenyWithUnknownRecipe() + { + // Arrange + using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveCraftDependencies(factory); + + // Act + var result = CraftOperations.TryCraft( + PlayerId, + " ", + quantity: 1, + deps.RecipeRegistry, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.XpStore, + deps.LevelCurve, + deps.PerkUnlockEngine); + + // Assert + Assert.False(result.Success); + Assert.Equal(CraftReasonCodes.UnknownRecipe, result.ReasonCode); + } + [Theory] [InlineData(0)] [InlineData(-1)] @@ -201,9 +254,8 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); - deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); - // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory); Assert.False(result.Success); Assert.Equal(CraftReasonCodes.InvalidQuantity, result.ReasonCode); Assert.Equal(5, CountBagItem(inventory!, "scrap_metal_bulk")); @@ -232,9 +284,8 @@ public sealed class CraftOperationsTests deps.LevelCurve, deps.PerkUnlockEngine); - deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); - // Assert + deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory); Assert.False(result.Success); Assert.Equal(CraftReasonCodes.ProgressionStoreMissing, result.ReasonCode); Assert.Equal(5, CountBagItem(afterInventory!, "scrap_metal_bulk")); diff --git a/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryOperationsTests.cs index 4bed3ae..fa4be35 100644 --- a/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryOperationsTests.cs @@ -293,6 +293,78 @@ public sealed class PlayerInventoryOperationsTests Assert.Null(outcome.Snapshot); } + [Fact] + public void TrySimulateAddStack_WhenBagFullForBagItem_ShouldReturnFalse() + { + // Arrange + using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var store = factory.Services.GetRequiredService(); + for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++) + { + _ = PlayerInventoryOperations.TryAddStack( + "dev-local-1", + "survey_drone_kit", + quantity: 1, + registry, + store); + } + + store.TryGetSnapshot("dev-local-1", out var snapshot); + var simulated = new PlayerInventorySnapshot + { + BagSlots = PlayerInventorySnapshot.CloneSlots(snapshot!.BagSlots), + EquipmentSlots = PlayerInventorySnapshot.CloneSlots(snapshot.EquipmentSlots), + }; + + // Act + var canPlace = PlayerInventoryOperations.TrySimulateAddStack( + ref simulated, + "refined_plate_stock", + quantity: 1, + registry); + + // Assert + Assert.False(canPlace); + } + + [Fact] + public void TrySimulateAddStack_WhenBagFullButEquipmentRouteAvailable_ShouldReturnTrue() + { + // Arrange + using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var store = factory.Services.GetRequiredService(); + for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++) + { + _ = PlayerInventoryOperations.TryAddStack( + "dev-local-1", + "survey_drone_kit", + quantity: 1, + registry, + store); + } + + store.TryGetSnapshot("dev-local-1", out var snapshot); + var simulated = new PlayerInventorySnapshot + { + BagSlots = PlayerInventorySnapshot.CloneSlots(snapshot!.BagSlots), + EquipmentSlots = PlayerInventorySnapshot.CloneSlots(snapshot.EquipmentSlots), + }; + + // Act + var canPlace = PlayerInventoryOperations.TrySimulateAddStack( + ref simulated, + "prototype_armor_shell", + quantity: 1, + registry); + + // Assert + Assert.True(canPlace); + Assert.Equal("prototype_armor_shell", simulated.EquipmentSlots[0].ItemId); + Assert.Equal(1, simulated.EquipmentSlots[0].Quantity); + } + private static int CountItem(PlayerInventorySnapshot snapshot, string itemId) { var total = 0; diff --git a/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs b/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs index ad2180d..969cd90 100644 --- a/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs +++ b/server/NeonSprawl.Server/Game/Crafting/CraftOperations.cs @@ -65,6 +65,7 @@ public static class CraftOperations } } + var inputsRemoved = new List(scaledInputs.Length); foreach (var input in scaledInputs) { var removeOutcome = PlayerInventoryOperations.TryRemoveStack( @@ -76,13 +77,17 @@ public static class CraftOperations if (removeOutcome.Kind == PlayerInventoryMutationKind.StoreMissing) { + CompensatingRestoreInputs(playerId, inputsRemoved, itemRegistry, inventoryStore); return Deny(CraftReasonCodes.InventoryStoreMissing); } if (removeOutcome.Kind == PlayerInventoryMutationKind.Denied) { + CompensatingRestoreInputs(playerId, inputsRemoved, itemRegistry, inventoryStore); return Deny(CraftReasonCodes.InsufficientMaterials); } + + inputsRemoved.Add(input); } var outputsApplied = new List(scaledOutputs.Length);