68 lines
4.6 KiB
Markdown
68 lines
4.6 KiB
Markdown
# 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**.
|