From c066121c9d5cb069cf8ce239e57d30ba3e108c53 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 15 Jun 2026 20:25:35 -0400 Subject: [PATCH] NEO-135: add code review for faction standing stores --- docs/reviews/2026-06-15-NEO-135.md | 56 ++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 docs/reviews/2026-06-15-NEO-135.md diff --git a/docs/reviews/2026-06-15-NEO-135.md b/docs/reviews/2026-06-15-NEO-135.md new file mode 100644 index 0000000..41e4d5a --- /dev/null +++ b/docs/reviews/2026-06-15-NEO-135.md @@ -0,0 +1,56 @@ +# 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](https://linear.app/neon-sprawl/issue/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): + +```bash +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+).