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

Code review — NEO-136 (E7M3-04)

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

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: Done. Rollback now uses -appliedDelta where appliedDelta = mutation.NewStanding - mutation.PreviousStanding; audit row records applied delta. Test TryApplyDelta_ShouldRestoreClampedStanding_WhenAuditAppendFails added.

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, 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.

Verification

Commands run during review (passed):

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+).