NEO-105: Address code review findings for encounter completion.

Add apply-phase rollback test, E9.M1 deny hook comment, inventory
reason passthrough, and tighten idempotent replay assertions.

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
 

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
 

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);
     }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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<PlayerInventorySnapshot, PlayerInventoryMutationWrite> 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;
+                }
+            }
+        }
+    }
 }

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) =>
+    private static EncounterCompletionResult Deny(string reasonCode)
-        new(
+    {
+        // --- 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);
+    }
 }