neon-sprawl/docs/reviews/2026-03-29-NS-15.md

Code review — NS-15 (authoritative position read API)

Review metadata

Field	Value
Date	2026-03-29
Scope	Branch NS-15-position-state-api vs origin/main, plus working tree: modified tracked files and untracked implementation/tests/agent files (full change set as present locally)
Base	origin/main
Issue	NS-15 — E1.M1: Authoritative PositionState API (in-memory)

---

Verdict

Request changes (for merge readiness). Implementation quality is Approve with nits; the request changes is because the feature and tests are largely not yet in git relative to origin/main (untracked paths), so a PR built only from pushed commits would be incomplete.

Summary

The NS-15 work adds an in-memory IPositionStateStore, Game configuration for a dev player id and default position, PositionStateApi.MapPositionStateApi() for GET /game/players/{id}/position, XML-documented DTOs with schemaVersion, integration tests via WebApplicationFactory<Program>, and Program.Waf.cs for the test host. Program.cs stays thin (DI + map extension calls only), matching docs/plans/NS-15-implementation-plan.md. README documents curl and sample JSON. Separately, .cursor/rules/csharp-style.md gains primary-constructor guidance, and the code-review agent + AGENTS.md add process docs. Overall risk is low for a read-only spike; no persistence or client integration.

Blocking issues

1. Version control gap — As of this review, server/NeonSprawl.Server/Game/, server/NeonSprawl.Server.Tests/, server/NeonSprawl.Server/Program.Waf.cs, .cursor/rules/code-review-agent.md, and AGENTS.md are untracked, while Program.cs, appsettings.json, server/README.md, NeonSprawl.sln, docs/plans/NS-15-implementation-plan.md, and .cursor/rules/csharp-style.md are modified but uncommitted. Before merge: git add all intended files and push so CI/reviewers see the real diff.

Suggestions

1. Null-safe DefaultPosition — In InMemoryPositionStateStore, configuration binding could theoretically set GamePositionOptions.DefaultPosition to null. Use var p = o.DefaultPosition ?? new DefaultPositionOptions(); before reading p.X / p.Y / p.Z to avoid a startup NullReferenceException.

2. Case sensitivity — Lookups use StringComparer.Ordinal. Document that player ids in URLs are case-sensitive, or normalize route id and stored keys the same way if product expects case-insensitivity.

3. Implementation plan markdown — In docs/plans/NS-15-implementation-plan.md, the acceptance section mixes raw ** markers with bullets (e.g. `**dotnet test**`) and no longer uses - [ ] checkboxes. Restoring valid Markdown and checkboxes improves scanability.

4. Solution file noise — If NeonSprawl.sln picks up a UTF-8 BOM or Windows-style path separators, consider normalizing to reduce cross-platform diff churn (non-functional).

5. PR description — When opening the PR, paste the sample JSON contract (as in README) so Jira AC (“document field names”) is satisfied outside the repo alone.

Nits

• Nit: SchemaVersion is set to 1 in both PositionStateResponse and the handler; a single named constant could prevent drift.
• Nit: No logging on 404 vs success—acceptable for NS-15; revisit when debugging multi-player seeds.

Verification

Run from repo root (.NET 10 SDK on PATH):

git status   # expect clean after add/commit, no missing Game/ or Tests/
dotnet build NeonSprawl.sln
dotnet test NeonSprawl.sln

Manual:

cd server/NeonSprawl.Server && dotnet run
# elsewhere:
curl -s http://localhost:5253/game/players/dev-local-1/position

Expect HTTP 200 and JSON including schemaVersion, playerId, position with x/y/z, and sequence. Unknown id should return 404.

Files touched (review scope)

Modified (tracked): .cursor/rules/csharp-style.md, NeonSprawl.sln, docs/plans/NS-15-implementation-plan.md, server/NeonSprawl.Server/Program.cs, server/NeonSprawl.Server/appsettings.json, server/README.md

Untracked (must be added for a complete PR): .cursor/rules/code-review-agent.md, AGENTS.md, server/NeonSprawl.Server/Program.Waf.cs, server/NeonSprawl.Server/Game/PositionState/*.cs, server/NeonSprawl.Server.Tests/*

Conformance notes

• architecture-authority: Server-held in-memory state, HTTP read spike; no client or move submission—matches NS-15 out-of-scope.
• testing-expectations: First server module gets NeonSprawl.Server.Tests with integration-style coverage for happy path and 404—appropriate.
• csharp-style: Test class uses primary constructor and using Xunit;—aligned with updated rule.
• Plan alignment: Separate PositionStateApi, PositionStateServiceCollectionExtensions, no position routes inlined in Program.cs—matches plan.