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

Code review — NEO-135 (E7M3-03)

Date: 2026-06-15
Scope: Branch NEO-135-e7m3-faction-standing-reputation-stores — commits 08ebfce … 4fca45b vs main
Base: main

Verdict

Approve with nits

Summary

This branch lands E7M3-03: durable per-player IFactionStandingStore snapshots and append-only IReputationDeltaStore audit rows, with in-memory fallbacks and Postgres implementations (V009, V010) when ConnectionStrings:NeonSprawl is set. Stores inject IFactionDefinitionRegistry for fail-closed faction validation, default missing standing to 0 with read/write clamping, and gate mutations (and reads) on the dev player bucket / player_position row — mirroring NEO-116 quest-store patterns. Test coverage is solid: in-memory AAA for apply/clamp/deny paths, audit ordering/limit/duplicate-id semantics, Postgres persistence round-trips, and host DI smoke. 766 server tests pass locally. Risk is low — infrastructure-only, no HTTP or player-facing behavior. Minor gaps: decomposition tracking docs not updated for E7M3-03, and one plan-listed in-memory read-clamp case is only covered via Postgres integration.

Documentation checked

Path	Result
docs/plans/NEO-135-implementation-plan.md	Matches — shipped types, stores, migrations, DI, tests, and README reconcile with plan; AC checklist marked complete. Plan types table lists invalid_delta reason code not present in code (reserved for NEO-136 — see nits).
docs/plans/E7M3-pre-production-backlog.md	Matches — E7M3-03 scope (dual stores, clamp, audit append, no public apply API, no client counterpart) aligns with implementation. Backlog AC checkboxes still unchecked (plan doc is authoritative for this branch).
docs/decomposition/modules/E7_M3_FactionReputationLedger.md	Partially matches — module purpose/contracts align; Status line still stops at E7M3-02 (NEO-134) and does not record E7M3-03 store landing.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Partially matches — E7.M3 row still lists E7M3-03+ as future; should note stores landed after merge.
docs/decomposition/modules/module_dependency_register.md	N/A change — register row already lists FactionStanding / ReputationDelta contracts.
server/README.md	Matches — new faction standing + reputation delta store sections; correctly defers auditable apply to NEO-136.
bruno/neon-sprawl-server/faction-standing/	N/A plan item — health smoke folder added (reasonable host-start verification; not listed in plan).

Blocking issues

None.

Suggestions

1. Decomposition status — Update E7_M3_FactionReputationLedger.md Status (one line) and documentation_and_implementation_alignment.md E7.M3 row to record E7M3-03 faction standing + reputation delta stores landed (NEO-135). Same pattern as NEO-134 review follow-up.

2. In-memory read clamp test — Plan §7 lists “Read clamp when stored value exceeds max” under InMemoryFactionStandingStoreTests. TryGetStanding_ShouldClampRead_WhenStoredValueExceedsMax exists only in Postgres integration tests. Add an in-memory case (e.g. apply to max then manually seed out-of-band via a second apply + direct dictionary seed is not possible — use apply overflow then verify read clamp, or test via Postgres-only path is insufficient for unit layer). Simplest: after clamp-to-max apply, verify read returns 100; for true out-of-band raw, mirror Postgres test with reflection/internal test hook or document that read-clamp defense is integration-tested only. Prefer a dedicated in-memory test that applies deltas to max then asserts read clamp on subsequent reads at boundary (partial coverage) plus keeping Postgres test for stale DB values.

3. Plan types table drift — NEO-135-implementation-plan.md §1 lists FactionStandingReasonCodes.invalid_delta; shipped code only has unknown_faction and player_not_writable. Add a one-line note in the plan that invalid_delta is deferred to NEO-136 ReputationOperations, or add the constant now without wiring (avoid dead code — note-only is fine).

Nits

• Nit: PostgresReputationDeltaStore.TryAppend relies on FK to player_position; a row for a non-existent player throws rather than returning false. Acceptable if NEO-136 always applies standing first; worth a one-line remark in README or interface XML for orchestrators.
• Nit: Host DI smoke (Host_ShouldResolveFactionStandingStoresFromDi_WhenStartupSucceeds) lives in InMemoryFactionStandingStoreTests.cs rather than a host-focused test file — fine per plan “optional one-liner.”
• Nit: FactionStandingPersistenceIntegrationTests places Assert.True(outcome.Success) inside the Act block for the first factory write — minor AAA deviation; assertions could move entirely to Assert phase.
• Nit: Empty factionId on read/write returns player_not_writable rather than unknown_faction — consistent with empty-id handling elsewhere, but slightly imprecise reason code.

Verification

Commands run during review (passed):

dotnet test server/NeonSprawl.Server.Tests

Author should confirm PR CI green before merge. With Postgres configured locally, RequirePostgresFact integration tests should also pass (ConnectionStrings__NeonSprawl). Bruno smoke (bruno/neon-sprawl-server/faction-standing/) is appropriate — no manual QA doc required per plan (infrastructure-only).

Intentional follow-on (not blockers): ReputationOperations orchestration (NEO-136), gate eval reads (NEO-137), reward bundle rep wiring (NEO-138), HTTP GET (NEO-139), Godot HUD (NEO-142+).