# 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`

Follow-up: suggestion + applicable nits below are **done** (strikethrough + **Done.**).

## 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.~~ **Done.** Added `GetFactionStanding_ShouldReturnOk_WhenPathIdHasLeadingAndTrailingWhitespace` and `GetFactionStanding_ShouldReturnNotFound_WhenPathIdIsWhitespaceOnly` in `FactionStandingApiTests.cs`.

## 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.~~ **Done.** Whitespace-only path covered by `GetFactionStanding_ShouldReturnNotFound_WhenPathIdIsWhitespaceOnly`.
- 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.~~ **Done.** `faction-standing/folder.bru` docs note seq 4 side effect and seq 2 re-run caveat.

## 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**.