NEO-149: address code review follow-ups

Update E7.M4 alignment register, add quest-fixture contract reset
API test, align mark-complete deny codes with instance reason codes,
refactor idempotent replay test AAA, strike review findings done.

docs/reviews/2026-06-27-NEO-149.md

@@ -19,11 +19,11 @@ NEO-149 adds `ContractCompletionOperations.TryCompleteOnEncounterClear` — deli
 | `docs/plans/NEO-149-implementation-plan.md` | **Matches** — AC checklist complete; shipped reconciliation aligns with diff |
 | `docs/plans/E7M4-pre-production-backlog.md` (E7M4-06) | **Matches** — orchestrator + encounter wiring in scope; HTTP Bruno loop deferred to NEO-151 |
 | `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — NEO-149 completion wiring anchor added |
-| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row includes NEO-144–NEO-148 but not **E7M4-06 / NEO-149** yet |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M4 row includes **E7M4-06 / NEO-149** (orchestrator + encounter wiring + quest-fixture reset) |
 | `server/README.md` | **Matches** — contract completion operations section, wiring order, reason codes, fixture reset |
 | Client counterpart (NEO-153, NEO-154) | **N/A** — explicitly out of scope; no prototype-complete claim |
 
-Register/tracking table should be updated for E7.M4 (NEO-149) after merge.
+Register/tracking table updated for E7.M4 (NEO-149).
 
 ## Blocking issues
 
@@ -31,16 +31,16 @@ None.
 
 ## Suggestions
 
-1. **Update implementation alignment register** — Add **E7M4-06 / NEO-149 landed** to the E7.M4 row in `docs/decomposition/modules/documentation_and_implementation_alignment.md` (orchestrator + encounter wiring + quest-fixture reset), consistent with NEO-144–NEO-148 entries.
+1. ~~**Update implementation alignment register** — Add **E7M4-06 / NEO-149 landed** to the E7.M4 row in `docs/decomposition/modules/documentation_and_implementation_alignment.md` (orchestrator + encounter wiring + quest-fixture reset), consistent with NEO-144–NEO-148 entries.~~ **Done.** E7.M4 row + plan link updated.
 
-2. **Quest-fixture test for contract reset** — `QuestFixtureOperations` now clears contract instance, delivery, and outcome rows via `resetContractInstanceIds`, but `QuestFixtureApiTests` has no parallel to `PostQuestFixture_ShouldClearRewardDeliveryAndAudit_WhenResetQuestIdsProvided`. A test that issues a contract, completes it, resets via fixture, and asserts stores are empty would lock the NEO-148 follow-on reset path.
+2. ~~**Quest-fixture test for contract reset** — `QuestFixtureOperations` now clears contract instance, delivery, and outcome rows via `resetContractInstanceIds`, but `QuestFixtureApiTests` has no parallel to `PostQuestFixture_ShouldClearRewardDeliveryAndAudit_WhenResetQuestIdsProvided`. A test that issues a contract, completes it, resets via fixture, and asserts stores are empty would lock the NEO-148 follow-on reset path.~~ **Done.** `PostQuestFixture_ShouldClearContractStores_WhenResetContractInstanceIdsProvided`.
 
-3. **`TryMarkComplete` failure reason code** — When delivery succeeds but `TryMarkComplete` fails without leaving a completed snapshot, the orchestrator returns `invalid_ids` (line 122 in `ContractCompletionOperations.cs`). `QuestStateOperations.TryMarkComplete` uses richer fallback codes (`NotActive`, `UnknownPlayer`). Consider aligning deny semantics in a follow-on if store-failure paths become testable; not blocking for prototype (same deliver-then-mark stuck-state risk as quest path when mark loses after delivery).
+3. ~~**`TryMarkComplete` failure reason code** — When delivery succeeds but `TryMarkComplete` fails without leaving a completed snapshot, the orchestrator returns `invalid_ids` (line 122 in `ContractCompletionOperations.cs`). `QuestStateOperations.TryMarkComplete` uses richer fallback codes (`NotActive`, `UnknownPlayer`). Consider aligning deny semantics in a follow-on if store-failure paths become testable; not blocking for prototype (same deliver-then-mark stuck-state risk as quest path when mark loses after delivery).~~ **Done.** Final deny uses `player_not_writable`, `instance_not_active`, or `instance_not_found` instead of `invalid_ids`.
 
 ## Nits
 
 - Nit: Idempotent replay after successful completion returns `no_active_contract` (instance no longer active) rather than hitting the defensive `completed` snapshot branch at lines 59–61. Behavior is correct — no double grant — but the reason code differs from the plan’s “completed replay” wording.
-- Nit: `TryCompleteOnEncounterClear_ShouldBeIdempotent_WhenReplayed` performs success assertions on the first completion inside **Arrange**; prefer moving first-call verification to **Assert** after **Act** for strict AAA, or extract a shared setup helper.
+- ~~Nit: `TryCompleteOnEncounterClear_ShouldBeIdempotent_WhenReplayed` performs success assertions on the first completion inside **Arrange**; prefer moving first-call verification to **Assert** after **Act** for strict AAA, or extract a shared setup helper.~~ **Done.** `IssueAndCompleteOnce` setup helper; replay test Act/Assert only.
 - Nit: Plan lists optional `EncounterCombatWiring` end-to-end issue → cast → contract completed; only compile-fix call-site updates landed. Acceptable given integration + encounter completion tests cover the path.
 - Nit: Bruno `Reset contract instance via quest fixture.bru` uses placeholder `ci_000…` id — fine until NEO-151 wires issue → clear → GET; helper `resetContractInstances` in `bruno-dev-fixture-helper.js` is the useful piece for that story.
 

server/NeonSprawl.Server.Tests/Game/Contracts/ContractCompletionOperationsTests.cs

@@ -131,12 +131,7 @@ public sealed class ContractCompletionOperationsTests
         // Arrange
         await using var factory = new InMemoryWebApplicationFactory();
         var deps = ResolveDependencies(factory);
-        var issued = IssuePrototypeContract(deps);
+        var instanceId = IssueAndCompleteOnce(deps);
-        var instanceId = issued.Snapshot!.ContractInstanceId;
-        var first = CompleteOnEncounterClear(deps, EncounterId);
-        Assert.True(first.Success);
-        deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterFirst);
-        var salvageAfterFirst = deps.SkillProgressionStore.GetXpTotals(PlayerId).GetValueOrDefault("salvage");
 
         // Act
         var replay = CompleteOnEncounterClear(deps, EncounterId);
@@ -144,19 +139,27 @@ public sealed class ContractCompletionOperationsTests
         // Assert
         Assert.True(replay.Success);
         Assert.Equal(ContractCompletionReasonCodes.NoActiveContract, replay.ReasonCode);
-        deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterReplay);
+        Assert.True(deps.InstanceStore.TryGet(PlayerId, instanceId, out var instance));
-        Assert.Equal(
+        Assert.Equal(ContractInstanceStatus.Completed, instance.Status);
-            CountBagItem(afterFirst!, "scrap_metal_bulk"),
+        Assert.False(deps.InstanceStore.TryGetActiveForPlayer(PlayerId, out _));
-            CountBagItem(afterReplay!, "scrap_metal_bulk"));
-        Assert.Equal(
-            salvageAfterFirst,
-            deps.SkillProgressionStore.GetXpTotals(PlayerId).GetValueOrDefault("salvage"));
         Assert.True(
             deps.DeliveryStore.TryGet(
                 PlayerId,
                 RewardDeliverySourceKinds.ContractCompletion,
                 instanceId,
                 out _));
+        Assert.Single(deps.OutcomeStore.GetOutcomesForInstance(instanceId));
+    }
+
+    private static string IssueAndCompleteOnce(CompletionTestDependencies deps)
+    {
+        var issued = IssuePrototypeContract(deps);
+        var instanceId = issued.Snapshot!.ContractInstanceId;
+        var first = CompleteOnEncounterClear(deps, EncounterId);
+        Assert.True(first.Success);
+        Assert.Null(first.ReasonCode);
+        Assert.Equal(ContractInstanceStatus.Completed, first.Snapshot!.Status);
+        return instanceId;
     }
 
     private static ContractIssueOperationResult IssuePrototypeContract(CompletionTestDependencies deps) =>

server/NeonSprawl.Server.Tests/Game/Quests/QuestFixtureApiTests.cs

@@ -3,9 +3,13 @@ using System.Net.Http.Json;
 using System.Text;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.Extensions.DependencyInjection;
+using NeonSprawl.Server.Game.Contracts;
 using NeonSprawl.Server.Game.Factions;
+using NeonSprawl.Server.Game.Items;
+using NeonSprawl.Server.Game.Mastery;
 using NeonSprawl.Server.Game.Quests;
 using NeonSprawl.Server.Game.Rewards;
+using NeonSprawl.Server.Game.Skills;
 using NeonSprawl.Server.Tests;
 using Xunit;
 
@@ -271,4 +275,84 @@ public sealed class QuestFixtureApiTests
         Assert.False(deliveryStore.TryGet(DevPlayer, ChainQuestId, out _));
         Assert.Empty(auditStore.GetDeltasForPlayer(DevPlayer));
     }
+
+    [Fact]
+    public async Task PostQuestFixture_ShouldClearContractStores_WhenResetContractInstanceIdsProvided()
+    {
+        // Arrange
+        await using var factory = new InMemoryWebApplicationFactory();
+        var client = factory.CreateClient();
+        var templateRegistry = factory.Services.GetRequiredService<IContractTemplateRegistry>();
+        var instanceStore = factory.Services.GetRequiredService<IContractInstanceStore>();
+        var outcomeStore = factory.Services.GetRequiredService<IContractOutcomeStore>();
+        var deliveryStore = factory.Services.GetRequiredService<IRewardDeliveryStore>();
+        var standingStore = factory.Services.GetRequiredService<IFactionStandingStore>();
+        var itemRegistry = factory.Services.GetRequiredService<IItemDefinitionRegistry>();
+        var inventoryStore = factory.Services.GetRequiredService<IPlayerInventoryStore>();
+        var skillRegistry = factory.Services.GetRequiredService<ISkillDefinitionRegistry>();
+        var skillProgressionStore = factory.Services.GetRequiredService<IPlayerSkillProgressionStore>();
+        var levelCurve = factory.Services.GetRequiredService<ISkillLevelCurve>();
+        var perkUnlockEngine = factory.Services.GetRequiredService<PerkUnlockEngine>();
+        var perkStore = factory.Services.GetRequiredService<IPlayerPerkStateStore>();
+        var auditStore = factory.Services.GetRequiredService<IReputationDeltaStore>();
+        var timeProvider = factory.Services.GetRequiredService<TimeProvider>();
+        const string templateId = "prototype_contract_clear_combat_pocket";
+        const string encounterId = "prototype_combat_pocket";
+        var issued = ContractGeneratorOperations.TryIssue(
+            DevPlayer,
+            templateId,
+            "2026-06-22",
+            zoneDifficultyBand: null,
+            templateRegistry,
+            instanceStore,
+            standingStore,
+            timeProvider);
+        Assert.True(issued.Success);
+        var instanceId = issued.Snapshot!.ContractInstanceId;
+        var completed = ContractCompletionOperations.TryCompleteOnEncounterClear(
+            DevPlayer,
+            encounterId,
+            instanceStore,
+            outcomeStore,
+            templateRegistry,
+            itemRegistry,
+            inventoryStore,
+            skillRegistry,
+            skillProgressionStore,
+            levelCurve,
+            perkUnlockEngine,
+            perkStore,
+            deliveryStore,
+            standingStore,
+            auditStore,
+            timeProvider);
+        Assert.True(completed.Success);
+        Assert.True(
+            deliveryStore.TryGet(
+                DevPlayer,
+                RewardDeliverySourceKinds.ContractCompletion,
+                instanceId,
+                out _));
+        Assert.Single(outcomeStore.GetOutcomesForInstance(instanceId));
+
+        // Act
+        var reset = await client.PostAsJsonAsync(
+            FixturePath,
+            new QuestFixtureRequest
+            {
+                SchemaVersion = QuestFixtureRequest.CurrentSchemaVersion,
+                ResetContractInstanceIds = [instanceId],
+            });
+
+        // Assert
+        Assert.Equal(HttpStatusCode.OK, reset.StatusCode);
+        Assert.False(instanceStore.TryGet(DevPlayer, instanceId, out _));
+        Assert.False(
+            deliveryStore.TryGet(
+                DevPlayer,
+                RewardDeliverySourceKinds.ContractCompletion,
+                instanceId,
+                out _));
+        Assert.Empty(outcomeStore.GetOutcomesForInstance(instanceId));
+    }
 }

server/NeonSprawl.Server/Game/Contracts/ContractCompletionOperations.cs

@@ -119,7 +119,20 @@ public static class ContractCompletionOperations
             return Success(afterAttempt, TryGetExistingOutcome(outcomeStore, afterAttempt));
         }
 
-        return Deny(ContractCompletionReasonCodes.InvalidIds, activeSnapshot);
+        if (!instanceStore.CanWritePlayer(normalizedPlayerId))
+        {
+            return Deny(ContractCompletionReasonCodes.PlayerNotWritable, activeSnapshot);
+        }
+
+        if (instanceStore.TryGet(
+                normalizedPlayerId,
+                activeSnapshot.ContractInstanceId,
+                out var currentAfterFailedMark))
+        {
+            return Deny(ContractInstanceReasonCodes.InstanceNotActive, currentAfterFailedMark);
+        }
+
+        return Deny(ContractInstanceReasonCodes.InstanceNotFound, activeSnapshot);
     }
 
     private static ContractOutcomeRow? AppendOutcome(