diff --git a/docs/plans/NEO-105-implementation-plan.md b/docs/plans/NEO-105-implementation-plan.md
index 52e14a2..e9e5910 100644
--- a/docs/plans/NEO-105-implementation-plan.md
+++ b/docs/plans/NEO-105-implementation-plan.md
@@ -55,7 +55,7 @@
- **Operations:** `EncounterCompletionOperations.TryCompleteAndGrant` — pre-flight simulate, sequential grant apply with compensating rollback, idempotent `TryMarkCompleted`.
- **Types:** `EncounterCompletionResult`, `EncounterCompletionReasonCodes`, `EncounterGrantApplied`, `EncounterCompleteEvent` (result payload only; NEO-107 persists).
-- **Tests:** `EncounterCompletionOperationsTests` — grant once, `already_completed` replay, `not_ready`, `inventory_full` fail-closed, mark-fail rollback.
+- **Tests:** `EncounterCompletionOperationsTests` — grant once, `already_completed` replay, `not_ready`, `inventory_full` fail-closed, apply-phase rollback (`InventoryStoreFullAfterFirstGrant`), mark-fail rollback.
- **Docs:** `server/README.md`; E5.M3 module snapshot + alignment register + backlog updated.
## Technical approach
@@ -119,7 +119,7 @@
| Test file | What it covers |
|-----------|----------------|
-| `EncounterCompletionOperationsTests.cs` | **Arrange:** `InMemoryWebApplicationFactory` + activate/defeat-all helper for `prototype_combat_pocket`. **Act/Assert:** first `TryCompleteAndGrant` success with scrap ×10 + token ×1 in inventory and `CompleteEvent` populated; second call `already_completed` with unchanged quantities; `not_ready` when only 2/3 defeated; bag-full deny `inventory_full` with `IsCompleted` false and zero grant delta; compensating rollback when bag cannot fit both grants (fill bag before call). |
+| `EncounterCompletionOperationsTests.cs` | **Arrange:** `InMemoryWebApplicationFactory` + activate/defeat-all helper for `prototype_combat_pocket`. **Act/Assert:** first `TryCompleteAndGrant` success with scrap ×10 + token ×1 in inventory and `CompleteEvent` populated; second call `already_completed` with unchanged quantities; `not_ready` when only 2/3 defeated; bag-full deny `inventory_full` with `IsCompleted` false and zero grant delta; apply-phase rollback via `InventoryStoreFullAfterFirstGrant` when second grant fails after first succeeds; mark-fail rollback when `TryMarkCompleted` returns false. |
## Open questions / risks
diff --git a/docs/reviews/2026-05-31-NEO-105.md b/docs/reviews/2026-05-31-NEO-105.md
index d282648..827e1e6 100644
--- a/docs/reviews/2026-05-31-NEO-105.md
+++ b/docs/reviews/2026-05-31-NEO-105.md
@@ -4,6 +4,8 @@
**Scope:** Branch `NEO-105-encounter-completion-inventory-reward-apply` vs `719a14f` (merge-base on `main`) — commits `b5306aa` … `009fedf`
**Base:** `719a14f` (main at branch point, post NEO-104 merge)
+**Follow-up:** Review suggestions and nits addressed in commit after `009fedf`.
+
## Verdict
**Approve with nits**
@@ -30,17 +32,17 @@ None.
## Suggestions
-1. **Apply-phase partial rollback test** — The plan test table mentions compensating rollback when the bag cannot fit both grants, but **`FillBag`** fills every slot so pre-flight denies before any apply; the mark-fail test covers rollback after both grants succeed. Consider an AAA test where pre-flight passes but the second **`TryAddStack`** fails during apply (e.g. stub inventory store that allows simulate/add for the first grant only) to exercise **`CompensatingRemoveGrants`** on the apply path, not only the mark-complete race path.
+1. ~~**Apply-phase partial rollback test** — The plan test table mentions compensating rollback when the bag cannot fit both grants, but **`FillBag`** fills every slot so pre-flight denies before any apply; the mark-fail test covers rollback after both grants succeed. Consider an AAA test where pre-flight passes but the second **`TryAddStack`** fails during apply (e.g. stub inventory store that allows simulate/add for the first grant only) to exercise **`CompensatingRemoveGrants`** on the apply path, not only the mark-complete race path.~~ **Done.** `TryCompleteAndGrant_ShouldRollbackFirstGrant_WhenSecondApplyFails` uses **`InventoryStoreFullAfterFirstGrant`** to fill the bag after the first apply so pre-flight still passes.
-2. **Denial telemetry hook comment** — **`EncounterProgressOperations`** documents an E9.M1 hook on activation; **`CraftOperations`** / **`GatherOperations`** comment deny sites. **`EncounterCompletionOperations`** only has the E7.M2 success TODO. A brief `// TODO(E9.M1): catalog emit — encounter_complete_denied` (or similar) on **`Deny`** would align with NEO-109 without adding runtime code.
+2. ~~**Denial telemetry hook comment** — **`EncounterProgressOperations`** documents an E9.M1 hook on activation; **`CraftOperations`** / **`GatherOperations`** comment deny sites. **`EncounterCompletionOperations`** only has the E7.M2 success TODO. A brief `// TODO(E9.M1): catalog emit — encounter_complete_denied` (or similar) on **`Deny`** would align with NEO-109 without adding runtime code.~~ **Done.** `encounter_complete_denied` hook comment added on **`Deny`**.
-3. **`unknown_reward_table` for empty `FixedGrants`** — Empty grants return **`unknown_reward_table`** (also documented in README). Prototype catalog gates prevent this at startup; if a future table legitimately has zero fixed grants, a dedicated reason code may be clearer. Fine for prototype; note for catalog expansion.
+3. ~~**`unknown_reward_table` for empty `FixedGrants`** — Empty grants return **`unknown_reward_table`** (also documented in README). Prototype catalog gates prevent this at startup; if a future table legitimately has zero fixed grants, a dedicated reason code may be clearer. Fine for prototype; note for catalog expansion.~~ **Addressed.** Inline comment on empty-grants branch; dedicated reason deferred until zero-grant tables ship.
## Nits
-- Nit: **`MapInventoryReason`** default branch maps unrecognized inventory reason codes to **`inventory_full`**. Safe given startup validation; passthrough of the raw code (or a dedicated **`inventory_denied`**) would aid debugging if invalid catalog rows slip through in tests.
+- ~~Nit: **`MapInventoryReason`** default branch maps unrecognized inventory reason codes to **`inventory_full`**. Safe given startup validation; passthrough of the raw code (or a dedicated **`inventory_denied`**) would aid debugging if invalid catalog rows slip through in tests.~~ **Done.** Unrecognized codes passthrough; **`null`** still maps to **`inventory_full`**.
-- Nit: **`TryCompleteAndGrant_ShouldDenyAlreadyCompleted_WhenCalledTwice`** — first successful completion is correctly in **Arrange**; consider asserting **`first.ReasonCode`** is null and **`second.GrantsApplied`** is empty in one block for symmetry with the success test (already partially covered).
+- ~~Nit: **`TryCompleteAndGrant_ShouldDenyAlreadyCompleted_WhenCalledTwice`** — first successful completion is correctly in **Arrange**; consider asserting **`first.ReasonCode`** is null and **`second.GrantsApplied`** is empty in one block for symmetry with the success test (already partially covered).~~ **Done.** Assert **`first.ReasonCode`** null and **`first.GrantsApplied.Count`** == 2 alongside second-call denies.
## Verification
diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterCompletionOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterCompletionOperationsTests.cs
index 94aa65f..70a847a 100644
--- a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterCompletionOperationsTests.cs
+++ b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterCompletionOperationsTests.cs
@@ -88,6 +88,8 @@ public sealed class EncounterCompletionOperationsTests
// Assert
deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterSecond);
Assert.True(first.Success);
+ Assert.Null(first.ReasonCode);
+ Assert.Equal(2, first.GrantsApplied.Count);
Assert.False(second.Success);
Assert.Equal(EncounterCompletionReasonCodes.AlreadyCompleted, second.ReasonCode);
Assert.Empty(second.GrantsApplied);
@@ -207,6 +209,36 @@ public sealed class EncounterCompletionOperationsTests
Assert.Equal(0, CountBagItem(afterInventory!, "contract_handoff_token"));
}
+ [Fact]
+ public async Task TryCompleteAndGrant_ShouldRollbackFirstGrant_WhenSecondApplyFails()
+ {
+ // Arrange
+ await using var factory = new InMemoryWebApplicationFactory();
+ var deps = ResolveDependencies(factory);
+ DefeatAllRequiredTargets(deps);
+ var inventoryStore = new InventoryStoreFullAfterFirstGrant(deps.InventoryStore, deps.ItemRegistry);
+
+ // Act
+ var result = EncounterCompletionOperations.TryCompleteAndGrant(
+ PlayerId,
+ EncounterId,
+ deps.EncounterRegistry,
+ deps.RewardTableRegistry,
+ deps.ProgressStore,
+ deps.CompletionStore,
+ deps.ItemRegistry,
+ inventoryStore,
+ TimeProvider.System);
+
+ // Assert
+ deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory);
+ Assert.False(result.Success);
+ Assert.Equal(EncounterCompletionReasonCodes.InventoryFull, result.ReasonCode);
+ Assert.False(deps.CompletionStore.IsCompleted(PlayerId, EncounterId));
+ Assert.Equal(0, CountBagItem(inventory!, "scrap_metal_bulk"));
+ Assert.Equal(0, CountBagItem(inventory!, "contract_handoff_token"));
+ }
+
[Fact]
public async Task TryCompleteAndGrant_ShouldDenyUnknownEncounter_WhenIdIsInvalid()
{
@@ -331,4 +363,58 @@ public sealed class EncounterCompletionOperationsTests
public bool TryGetCompletedAt(string playerId, string encounterId, out DateTimeOffset completedAt) =>
inner.TryGetCompletedAt(playerId, encounterId, out completedAt);
}
+
+ ///
+ /// After the first successful inventory mutation, fills remaining bag slots so the next apply fails
+ /// while pre-flight simulate (on an empty snapshot) still passes.
+ ///
+ private sealed class InventoryStoreFullAfterFirstGrant(
+ IPlayerInventoryStore inner,
+ IItemDefinitionRegistry itemRegistry) : IPlayerInventoryStore
+ {
+ private bool _firstMutationApplied;
+
+ public bool TryGetSnapshot(string playerId, out PlayerInventorySnapshot snapshot) =>
+ inner.TryGetSnapshot(playerId, out snapshot);
+
+ public bool TryReplaceSnapshot(string playerId, PlayerInventorySnapshot snapshot) =>
+ inner.TryReplaceSnapshot(playerId, snapshot);
+
+ public bool TryMutateSnapshot(
+ string playerId,
+ Func mutator,
+ out PlayerInventorySnapshot result)
+ {
+ if (!inner.TryMutateSnapshot(playerId, mutator, out result))
+ {
+ return false;
+ }
+
+ if (!_firstMutationApplied)
+ {
+ _firstMutationApplied = true;
+ FillRemainingBagSlots(playerId);
+ inner.TryGetSnapshot(playerId, out result);
+ }
+
+ return true;
+ }
+
+ private void FillRemainingBagSlots(string playerId)
+ {
+ for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++)
+ {
+ var add = PlayerInventoryOperations.TryAddStack(
+ playerId,
+ "survey_drone_kit",
+ quantity: 1,
+ itemRegistry,
+ inner);
+ if (add.Kind != PlayerInventoryMutationKind.Applied)
+ {
+ break;
+ }
+ }
+ }
+ }
}
diff --git a/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs b/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs
index 880e145..ba8bf72 100644
--- a/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs
+++ b/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs
@@ -53,6 +53,7 @@ public static class EncounterCompletionOperations
var grants = rewardTable.FixedGrants;
if (grants.Count == 0)
{
+ // Prototype catalog gates prevent empty tables at startup; dedicated reason if zero-grant tables ship later.
return Deny(EncounterCompletionReasonCodes.UnknownRewardTable);
}
@@ -131,7 +132,8 @@ public static class EncounterCompletionOperations
PlayerInventoryReasonCodes.InventoryFull => EncounterCompletionReasonCodes.InventoryFull,
PlayerInventoryReasonCodes.InvalidItem => EncounterCompletionReasonCodes.InvalidItem,
PlayerInventoryReasonCodes.InvalidQuantity => EncounterCompletionReasonCodes.InvalidQuantity,
- _ => reasonCode ?? EncounterCompletionReasonCodes.InventoryFull,
+ null => EncounterCompletionReasonCodes.InventoryFull,
+ _ => reasonCode,
};
private static string MakeIdempotencyKey(string normalizedPlayerId, string normalizedEncounterId) =>
@@ -162,11 +164,17 @@ public static class EncounterCompletionOperations
}
}
- private static EncounterCompletionResult Deny(string reasonCode) =>
- new(
+ private static EncounterCompletionResult Deny(string reasonCode)
+ {
+ // --- Telemetry hook site (NEO-109): future E9.M1 catalog event `encounter_complete_denied` ---
+ // TODO(E9.M1): catalog emit — structured deny with reasonCode (EncounterCompletionReasonCodes).
+ // Planned payload: playerId, encounterId, reasonCode. No ingest or ILogger here (comments-only).
+
+ return new EncounterCompletionResult(
Success: false,
ReasonCode: reasonCode,
GrantsApplied: EmptyGrants,
CompleteEvent: null,
CompletedAt: null);
+ }
}