From cc7786ba3a450b9321ae39da9bcde26b0fb2e8bd Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Tue, 16 Jun 2026 17:22:40 -0400
Subject: [PATCH] NEO-139: add code review for faction-standing GET API.

---
 docs/reviews/2026-06-16-NEO-139.md | 65 ++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 docs/reviews/2026-06-16-NEO-139.md

diff --git a/docs/reviews/2026-06-16-NEO-139.md b/docs/reviews/2026-06-16-NEO-139.md
new file mode 100644
index 0000000..b51c9bb
--- /dev/null
+++ b/docs/reviews/2026-06-16-NEO-139.md
@@ -0,0 +1,65 @@
+# Code review — NEO-139 (E7M3-07)
+
+**Date:** 2026-06-16  
+**Scope:** Branch `NEO-139-e7m3-get-faction-standing-http-read` — commits `5fc780c` … `3f08812` vs `main`  
+**Base:** `main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+This branch lands **E7M3-07**: **`GET /game/players/{id}/faction-standing`** returns a versioned snapshot (`schemaVersion` 1) with one row per catalog faction, gated by **`IPositionStateStore.TryGetPosition`** (unknown player **404**). **`FactionStandingApi.TryBuildSnapshot`** iterates **`IFactionDefinitionRegistry.GetDefinitionsInIdOrder()`** and reads standing via **`IFactionStandingStore.TryGetStanding`**, surfacing store read failures as **500** per the adopted plan risk table. Integration tests cover 404, neutral defaults, and post–operator-chain **+15** Grid Operators standing; Bruno adds three requests under `faction-standing/` and cross-links NEO-138 grid-contract accept docs. **`819`** server tests pass locally. Implementation mirrors skill/gig snapshot GET precedent and unblocks NEO-140/NEO-142.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-139-implementation-plan.md` | **Matches** — route, DTOs, position gate, catalog roster, Bruno folder, README, module/alignment updates, and reconciliation checklist all reflected in the diff. |
+| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-07 server read scope; client HUD (NEO-142) and quest projections (NEO-140) explicitly deferred. |
+| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-07 landed (NEO-139). |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated with HTTP read + README/Bruno links. |
+| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc / alignment table. |
+| `server/README.md` | **Matches** — Faction standing read section with curl, sample JSON, 404 gate, consumer notes, NEO-142 cross-link. |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Add trim-path integration test.** The plan §4 prototype scenario table lists “GET with trimmed id → **200**; body **`playerId`** echoes trimmed path” (skill/gig snapshot precedent). The handler trims (`id.Trim()`) but no test exercises whitespace-padded path segments or asserts echoed **`playerId`**. A small fourth `[Fact]` would lock the contract.
+
+## Nits
+
+- Nit: Empty or whitespace-only `{id}` returns **404** (trim → empty) but is not covered by tests — low risk given identical gate to other snapshot APIs.
+- Nit: **`BuildSnapshot`** (throws on failure) is exposed for reuse while HTTP uses **`TryBuildSnapshot`** — fine; no direct unit tests call **`BuildSnapshot`** today.
+- Nit: **`FactionStandingApiTests`** duplicates a **`RewardRouterTestDependencies`** helper record also seen in reward-router tests — acceptable for now; extract only if a third caller appears.
+- Nit: Bruno **`Get faction standing default.bru`** (seq 2) documents Postgres/restart caveats; re-running the folder on a warm in-memory server after seq 4 can leave standing at **15** — docs already warn; optional one-line note in folder README if authors hit flaky local runs.
+
+## Verification
+
+```bash
+# Full server suite (819 green locally)
+dotnet test NeonSprawl.sln
+
+# NEO-139-focused filter
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~FactionStandingApi"
+
+# Bruno faction-standing collection
+cd bruno/neon-sprawl-server
+npx --yes @usebruno/cli@3.3.0 run faction-standing --env Local --sandbox=developer --bail
+```
+
+With Postgres + dev fixtures (matches CI):
+
+```bash
+ConnectionStrings__NeonSprawl='Host=localhost;Port=5432;Database=neon_sprawl;Username=neon_sprawl;Password=neon_sprawl_dev' \
+Game__EnableCombatTargetFixtureApi=true \
+Game__EnableQuestFixtureApi=true \
+Game__EnableResourceNodeFixtureApi=true \
+dotnet run --project server/NeonSprawl.Server/NeonSprawl.Server.csproj --urls http://127.0.0.1:5253
+```
+
+Manual: after operator-chain quest flow, **`GET …/faction-standing`** for **`dev-local-1`** shows Grid Operators **15**, Rust Collective **0**.