NEO-69: Address code review findings for craft engine.
Fix AAA test layout, add batch quantity and simulate-add tests, restore partial input rollback, and bump E3.M2 register status to In Progress.pull/103/head
parent
b381cadf23
commit
51e9f1f5d0
|
|
@ -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 |
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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"));
|
||||
|
|
|
|||
|
|
@ -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<IItemDefinitionRegistry>();
|
||||
var store = factory.Services.GetRequiredService<IPlayerInventoryStore>();
|
||||
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<IItemDefinitionRegistry>();
|
||||
var store = factory.Services.GetRequiredService<IPlayerInventoryStore>();
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -65,6 +65,7 @@ public static class CraftOperations
|
|||
}
|
||||
}
|
||||
|
||||
var inputsRemoved = new List<CraftIoApplied>(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<CraftIoApplied>(scaledOutputs.Length);
|
||||
|
|
|
|||
Loading…
Reference in New Issue