From 2117b2f8d39617b3b55bde055b5acfc021417017 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 15 Jun 2026 20:50:57 -0400 Subject: [PATCH] NEO-136: add code review for ReputationOperations --- docs/reviews/2026-06-15-NEO-136.md | 67 ++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 docs/reviews/2026-06-15-NEO-136.md diff --git a/docs/reviews/2026-06-15-NEO-136.md b/docs/reviews/2026-06-15-NEO-136.md new file mode 100644 index 0000000..271afdc --- /dev/null +++ b/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+).