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	Matches — Status line notes E7M3-03 stores landed (NEO-135).
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E7.M3 row updated for E7M3-03 / NEO-135.
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. Done. Module status + alignment table updated for E7M3-03 / NEO-135.

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. Done. Added TryGetStanding_ShouldClampRead_WhenRawStoredValueExceedsMax using internal SeedRawStandingForTests; Postgres integration test retained 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). Done. Plan types table notes invalid_delta deferred to NEO-136.

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. Done. Remark added to IReputationDeltaStore.TryAppend XML and server/README.md.
• 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. Done.
• 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. Done. Empty faction id now returns unknown_faction.

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