NEO-136: fix rollback applied delta and normalize ids at ops boundary

docs/plans/NEO-136-implementation-plan.md

@@ -59,7 +59,7 @@ Server-internal **`ReputationOperations.TryApplyDelta`** orchestrates standing m
 
 - **Operations:** `ReputationOperations.TryApplyDelta` — pre-flight validation, standing apply, audit append, compensating rollback on append failure.
 - **Types:** `ReputationApplyOutcome`, `ReputationApplyReasonCodes` (`invalid_delta`, `invalid_delta_id`, `invalid_source`, `audit_append_failed`).
-- **Tests:** `ReputationOperationsTests` — 10 AAA cases including host DI smoke (`780` tests green).
+- **Tests:** `ReputationOperationsTests` — 11 AAA cases including host DI smoke and clamped audit-append rollback (`781` tests green).
 - **Docs:** `server/README.md` ReputationOperations section; E7.M3 module + alignment register updated.
 
 ## Technical approach
@@ -90,15 +90,15 @@ public static ReputationApplyOutcome TryApplyDelta(
 
 **Flow:**
 
-1. Normalize ids via **`FactionStandingIds`** (reuse store normalization rules).
+1. Normalize ids via **`FactionStandingIds`**; use normalized values for store calls and audit row.
 2. **Pre-flight denies** (no store mutation):
    - `deltaAmount == 0` → `invalid_delta`
    - empty `deltaId` → `invalid_delta_id`
    - empty `sourceKind` or `sourceId` → `invalid_source`
 3. **`standingStore.TryApplyStandingDelta(playerId, factionId, deltaAmount)`** — on deny, return outcome with store **`ReasonCode`** and zeroed standings.
-4. Build **`ReputationDeltaRow`** with caller `deltaId`, applied delta, `NewStanding` from mutation outcome, normalized source fields, **`timeProvider.GetUtcNow()`**.
+4. Build **`ReputationDeltaRow`** with caller `deltaId`, **applied** delta (`newStanding - previousStanding`), `NewStanding` from mutation outcome, normalized ids, **`timeProvider.GetUtcNow()`**.
 5. **`auditStore.TryAppend(row)`** — on `true`, return success with audit row.
-6. On append `false`: **compensating rollback** — `standingStore.TryApplyStandingDelta(playerId, factionId, -deltaAmount)`; return deny with **`audit_append_failed`** (standing restored to pre-apply if rollback succeeds; document prototype risk if rollback fails — extremely unlikely after fresh apply).
+6. On append `false`: **compensating rollback** — `standingStore.TryApplyStandingDelta` with **`-appliedDelta`** where `appliedDelta = mutation.NewStanding - mutation.PreviousStanding` (skip when zero); return deny with **`audit_append_failed`**.
 
 XML remarks: consumed by **`RewardRouterOperations`** (NEO-138) and future admin tools; not HTTP directly. NEO-141 telemetry hook site on successful apply (comment-only in NEO-141).
 
@@ -115,7 +115,7 @@ XML remarks: consumed by **`RewardRouterOperations`** (NEO-138) and future admin
 | `TryApplyDelta_ShouldDenyInvalidDelta_WhenAmountZero` | `invalid_delta` |
 | `TryApplyDelta_ShouldDenyInvalidDeltaId_WhenEmpty` | `invalid_delta_id` |
 | `TryApplyDelta_ShouldDenyInvalidSource_WhenEmpty` | `invalid_source` |
-| `TryApplyDelta_ShouldRollbackStanding_WhenAuditAppendFails` | Pre-append duplicate id; standing unchanged after deny |
+| `TryApplyDelta_ShouldRestoreClampedStanding_WhenAuditAppendFails` | Pre-seeded duplicate id at standing 95; +50 clamped apply rolls back to 95 |
 | `TryApplyDelta_ShouldDenyNonWritablePlayer` | `player_not_writable` passthrough |
 
 **Host DI smoke:** one test resolving stores from `InMemoryWebApplicationFactory` and applying via `ReputationOperations` (can live in same test file).

docs/reviews/2026-06-15-NEO-136.md

@@ -2,6 +2,8 @@
 
 **Date:** 2026-06-15  
 **Scope:** Branch `NEO-136-e7m3-reputation-operations-apply-delta` — commits `e9abc8b` … `536f635` vs `main`  
+**Follow-up:** Review findings addressed in follow-up commit (applied-delta rollback, ops id normalization, AAA nit).
+
 **Base:** `main`
 
 ## Verdict
@@ -29,29 +31,13 @@ None.
 
 ## Suggestions
 
-1. **Compensating rollback delta when clamping** — On audit append failure, rollback uses `-deltaAmount`:
+1. ~~**Compensating rollback delta when clamping** — On audit append failure, rollback uses `-deltaAmount`:~~ **Done.** Rollback now uses `-appliedDelta` where `appliedDelta = mutation.NewStanding - mutation.PreviousStanding`; audit row records applied delta. Test `TryApplyDelta_ShouldRestoreClampedStanding_WhenAuditAppendFails` added.
 
-```csharp
+2. ~~**Ops-layer id normalization (plan alignment)** — Plan §2 step 1 says normalize via **`FactionStandingIds`** before store calls.~~ **Done.** `TryApplyDelta` normalizes player/faction ids at the ops boundary; audit row built from normalized values.
-_ = standingStore.TryApplyStandingDelta(playerId, factionId, -deltaAmount);
-```
-
-When the standing apply was clamped, the **stored** change is `mutation.NewStanding - mutation.PreviousStanding`, not `deltaAmount`. Example: standing **95**, request **+50** → **100**; rollback **-50** yields **50**, not **95**. Use the applied delta instead:
-
-```csharp
-var appliedDelta = mutation.NewStanding - mutation.PreviousStanding;
-if (appliedDelta != 0)
-{
-    _ = standingStore.TryApplyStandingDelta(playerId, factionId, -appliedDelta);
-}
-```
-
-Add a test: seed standing near max, apply clamped delta with pre-seeded duplicate audit id, assert standing restored to pre-apply value. Prototype duplicate-id retries are unlikely via NEO-138 delivery idempotency, but this keeps the ops invariant sound.
-
-2. **Ops-layer id normalization (plan alignment)** — Plan §2 step 1 says normalize via **`FactionStandingIds`** before store calls. Stores already normalize internally (and **`InMemoryReputationDeltaStore.TryAppend`** normalizes audit rows on persist), so behavior is correct for prototype callers. For consistency with **`RewardRouterOperations`** and predictable **`AuditRow`** in outcomes, consider normalizing `playerId` / `factionId` once at the ops boundary and building the audit row from normalized values.
 
 ## Nits
 
-- Nit: **`TryApplyDelta_ShouldApplyStandingAndAppendAudit_WhenHappyPath`** — `auditStore.GetDeltasForPlayer(PlayerId)` runs in the **Act** phase; per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert), verification reads belong in **Assert**.
+- ~~Nit: **`TryApplyDelta_ShouldApplyStandingAndAppendAudit_WhenHappyPath`** — `auditStore.GetDeltasForPlayer(PlayerId)` runs in the **Act** phase; per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert), verification reads belong in **Assert**.~~ **Done.** `GetDeltasForPlayer` moved to Assert.
 - Nit: **`ReputationApplyOutcome`** on store-boundary deny returns standings from the mutation outcome (good); pre-flight **`Deny()`** zeroes standings — callers must not treat `0` as “neutral read” on `invalid_delta` / `invalid_source` denies (documented implicitly by reason codes).
 - Nit: Telemetry hook comment for NEO-141 is correctly placed on the success path only — no action needed.
 

server/NeonSprawl.Server.Tests/Game/Factions/ReputationOperationsTests.cs

@@ -34,7 +34,6 @@ public sealed class ReputationOperationsTests
             standingStore,
             auditStore,
             timeProvider);
-        var rows = auditStore.GetDeltasForPlayer(PlayerId);
         // Assert
         Assert.True(outcome.Success);
         Assert.Null(outcome.ReasonCode);
@@ -47,6 +46,7 @@ public sealed class ReputationOperationsTests
         Assert.Equal(ReputationDeltaSourceKinds.QuestCompletion, outcome.AuditRow.SourceKind);
         Assert.Equal(QuestSourceId, outcome.AuditRow.SourceId);
         Assert.Equal(RecordedAt, outcome.AuditRow.RecordedAt);
+        var rows = auditStore.GetDeltasForPlayer(PlayerId);
         var read = Assert.Single(rows);
         Assert.Equal(outcome.AuditRow, read);
         Assert.Equal(15, standingStore.TryGetStanding(PlayerId, GridFactionId).Standing);
@@ -74,7 +74,8 @@ public sealed class ReputationOperationsTests
         Assert.True(outcome.Success);
         Assert.Equal(95, outcome.PreviousStanding);
         Assert.Equal(100, outcome.NewStanding);
-        Assert.Equal(100, outcome.AuditRow!.ResultingStanding);
+        Assert.Equal(5, outcome.AuditRow!.DeltaAmount);
+        Assert.Equal(100, outcome.AuditRow.ResultingStanding);
         Assert.Equal(100, standingStore.TryGetStanding(PlayerId, GridFactionId).Standing);
     }
 
@@ -100,7 +101,8 @@ public sealed class ReputationOperationsTests
         Assert.True(outcome.Success);
         Assert.Equal(-95, outcome.PreviousStanding);
         Assert.Equal(-100, outcome.NewStanding);
-        Assert.Equal(-100, outcome.AuditRow!.ResultingStanding);
+        Assert.Equal(-5, outcome.AuditRow!.DeltaAmount);
+        Assert.Equal(-100, outcome.AuditRow.ResultingStanding);
         Assert.Equal(-100, standingStore.TryGetStanding(PlayerId, GridFactionId).Standing);
     }
 
@@ -241,6 +243,35 @@ public sealed class ReputationOperationsTests
         Assert.Single(auditStore.GetDeltasForPlayer(PlayerId));
     }
 
+    [Fact]
+    public void TryApplyDelta_ShouldRestoreClampedStanding_WhenAuditAppendFails()
+    {
+        // Arrange
+        var standingStore = CreateStandingStore();
+        var auditStore = CreateAuditStore();
+        Assert.True(standingStore.TryApplyStandingDelta(PlayerId, GridFactionId, 95).Success);
+        Assert.True(auditStore.TryAppend(CreateAuditRow("delta-clamp-conflict", 1, 1)));
+        // Act
+        var outcome = ReputationOperations.TryApplyDelta(
+            PlayerId,
+            GridFactionId,
+            50,
+            "delta-clamp-conflict",
+            ReputationDeltaSourceKinds.QuestCompletion,
+            QuestSourceId,
+            standingStore,
+            auditStore,
+            TimeProvider.System);
+        // Assert
+        Assert.False(outcome.Success);
+        Assert.Equal(ReputationApplyReasonCodes.AuditAppendFailed, outcome.ReasonCode);
+        Assert.Equal(95, outcome.PreviousStanding);
+        Assert.Equal(95, outcome.NewStanding);
+        Assert.Null(outcome.AuditRow);
+        Assert.Equal(95, standingStore.TryGetStanding(PlayerId, GridFactionId).Standing);
+        Assert.Single(auditStore.GetDeltasForPlayer(PlayerId));
+    }
+
     [Fact]
     public void TryApplyDelta_ShouldDenyNonWritablePlayer()
     {

server/NeonSprawl.Server/Game/Factions/ReputationOperations.cs

@@ -27,6 +27,8 @@ public static class ReputationOperations
             return Deny(ReputationApplyReasonCodes.InvalidDelta);
         }
 
+        var normalizedPlayerId = FactionStandingIds.NormalizePlayerId(playerId);
+        var normalizedFactionId = FactionStandingIds.NormalizeFactionId(factionId);
         var normalizedDeltaId = deltaId.Trim();
         if (normalizedDeltaId.Length == 0)
         {
@@ -40,7 +42,7 @@ public static class ReputationOperations
             return Deny(ReputationApplyReasonCodes.InvalidSource);
         }
 
-        var mutation = standingStore.TryApplyStandingDelta(playerId, factionId, deltaAmount);
+        var mutation = standingStore.TryApplyStandingDelta(normalizedPlayerId, normalizedFactionId, deltaAmount);
         if (!mutation.Success)
         {
             return new ReputationApplyOutcome(
@@ -51,11 +53,12 @@ public static class ReputationOperations
                 null);
         }
 
+        var appliedDelta = mutation.NewStanding - mutation.PreviousStanding;
         var row = new ReputationDeltaRow(
             normalizedDeltaId,
-            playerId,
+            normalizedPlayerId,
-            factionId,
+            normalizedFactionId,
-            deltaAmount,
+            appliedDelta,
             mutation.NewStanding,
             normalizedSourceKind,
             normalizedSourceId,
@@ -72,7 +75,11 @@ public static class ReputationOperations
                 row);
         }
 
-        _ = standingStore.TryApplyStandingDelta(playerId, factionId, -deltaAmount);
+        if (appliedDelta != 0)
+        {
+            _ = standingStore.TryApplyStandingDelta(normalizedPlayerId, normalizedFactionId, -appliedDelta);
+        }
+
         return new ReputationApplyOutcome(
             false,
             ReputationApplyReasonCodes.AuditAppendFailed,

server/README.md

@@ -88,7 +88,7 @@ Game code applies standing changes through **`ReputationOperations.TryApplyDelta
 
 **`TryApplyDelta`** parameters: `playerId`, `factionId`, signed `deltaAmount`, caller-supplied `deltaId` (UUID string), `sourceKind`, `sourceId`, plus injected **`IFactionStandingStore`**, **`IReputationDeltaStore`**, and **`TimeProvider`**.
 
-**Commit order:** standing apply first, audit append second. On audit append failure (`duplicate` id or invalid row), the operation **compensating-rollback** standing via inverse delta and returns **`audit_append_failed`**.
+**Commit order:** standing apply first, audit append second. Audit row **`deltaAmount`** records the **applied** change (`newStanding - previousStanding`), not the requested delta when clamping occurs. On audit append failure (`duplicate` id or invalid row), the operation **compensating-rollback** standing via the inverse applied delta and returns **`audit_append_failed`**.
 
 **Pre-flight denies (no store mutation):** `deltaAmount == 0` → **`invalid_delta`**; empty `deltaId` → **`invalid_delta_id`**; empty `sourceKind` or `sourceId` → **`invalid_source`**. Store boundary denies (**`unknown_faction`**, **`player_not_writable`**) pass through unchanged.