NEO-136: add code review for ReputationOperations

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

@@ -0,0 +1,67 @@
+# Code review — NEO-136 (E7M3-04)
+
+**Date:** 2026-06-15  
+**Scope:** Branch `NEO-136-e7m3-reputation-operations-apply-delta` — commits `e9abc8b` … `536f635` vs `main`  
+**Base:** `main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+This branch lands **E7M3-04**: server-internal **`ReputationOperations.TryApplyDelta`** orchestrates standing mutation via **`IFactionStandingStore`**, append-only audit via **`IReputationDeltaStore`**, and compensating standing rollback when audit append fails. Supporting types **`ReputationApplyOutcome`** and **`ReputationApplyReasonCodes`** cover ops-level pre-flight denies and store passthrough. Ten AAA unit tests plus host DI smoke exercise happy path, clamp, unknown faction, invalid delta/id/source, audit-append rollback, non-writable player, and factory resolution. **`780`** server tests pass locally. Risk is low — infrastructure-only, no HTTP or Godot wiring (NEO-138+). One non-blocking correctness gap: compensating rollback uses `-deltaAmount` rather than the **actual applied delta** when clamping occurred, which can leave wrong standing on rare audit-append failure after a clamped apply.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-136-implementation-plan.md` | **Matches** — shipped ops, types, tests, README, module/alignment updates reconcile with plan; AC checklist marked complete. Plan flow step 1 cites explicit `FactionStandingIds` normalization at ops layer; implementation delegates player/faction normalization to stores (see Suggestions). |
+| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-04 scope (auditable apply orchestration, no client counterpart, no HTTP) aligns with implementation. |
+| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-04 `ReputationOperations` landed (NEO-136). |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated for E7M3-04 / NEO-136. |
+| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — register row already lists `FactionStanding` / `ReputationDelta` contracts. |
+| `server/README.md` | **Matches** — new ReputationOperations section documents commit order, rollback policy, pre-flight denies, and “do not call standing store alone.” |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Compensating rollback delta when clamping** — On audit append failure, rollback uses `-deltaAmount`:
+
+```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.
+
+## 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: **`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.
+
+## Verification
+
+Commands run during review (passed):
+
+```bash
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ReputationOperationsTests"
+dotnet test NeonSprawl.sln
+```
+
+Author should confirm PR CI green before merge. No manual QA doc required (server engine only per plan). **Intentional follow-on:** reward bundle rep wiring (NEO-138), quest accept gate eval (NEO-137), HTTP GET standing (NEO-139), Godot HUD (NEO-142+).