From ddaf33ce8762d0f5d370a081716c1fdcdcb47d19 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 15 Jun 2026 20:52:04 -0400 Subject: [PATCH] NEO-136: fix rollback applied delta and normalize ids at ops boundary --- docs/plans/NEO-136-implementation-plan.md | 10 ++--- docs/reviews/2026-06-15-NEO-136.md | 24 +++--------- .../Factions/ReputationOperationsTests.cs | 37 +++++++++++++++++-- .../Game/Factions/ReputationOperations.cs | 17 ++++++--- server/README.md | 2 +- 5 files changed, 57 insertions(+), 33 deletions(-) diff --git a/docs/plans/NEO-136-implementation-plan.md b/docs/plans/NEO-136-implementation-plan.md index ad499ca..392bc7b 100644 --- a/docs/plans/NEO-136-implementation-plan.md +++ b/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). diff --git a/docs/reviews/2026-06-15-NEO-136.md b/docs/reviews/2026-06-15-NEO-136.md index 271afdc..82a265a 100644 --- a/docs/reviews/2026-06-15-NEO-136.md +++ b/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 -_ = 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. +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. ## 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. diff --git a/server/NeonSprawl.Server.Tests/Game/Factions/ReputationOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Factions/ReputationOperationsTests.cs index 590a979..68f9861 100644 --- a/server/NeonSprawl.Server.Tests/Game/Factions/ReputationOperationsTests.cs +++ b/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() { diff --git a/server/NeonSprawl.Server/Game/Factions/ReputationOperations.cs b/server/NeonSprawl.Server/Game/Factions/ReputationOperations.cs index 2365afc..0343fa5 100644 --- a/server/NeonSprawl.Server/Game/Factions/ReputationOperations.cs +++ b/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, - factionId, - deltaAmount, + normalizedPlayerId, + normalizedFactionId, + 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, diff --git a/server/README.md b/server/README.md index a1426f8..1aedd94 100644 --- a/server/README.md +++ b/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.