neon-sprawl/docs/reviews/2026-03-30-NEO-8.md

Code review — NEO-8 (PostgreSQL position persistence)

Date: 2026-03-30

Scope: Branch NEO-8-persist-position-postgres; Linear NEO-8. Working tree includes modified and untracked files (e.g. PostgresPositionStateStore.cs, server/db/, Postgres test types are untracked at review time).

Base: main (comparison: git diff main --stat plus untracked NEO-8 assets).

---

Verdict

Approve with nits — Persistence wiring matches the NEO-8 plan and authority docs; tests and CI cover the durable path. Decomposition implementation snapshot docs updated for NEO-8 (see Author follow-up below).

---

Summary

This work adds a PostgreSQL-backed IPositionStateStore, a versioned DDL file under server/db/migrations/, configuration via ConnectionStrings:NeonSprawl, idempotent schema application once per process, and per-request idempotent dev-row seeding so behavior stays consistent with TRUNCATE-heavy tests. GitHub Actions gains a Postgres service and passes the connection string into dotnet test. In-memory API tests are isolated via InMemoryWebApplicationFactory + ConfigureTestServices so a global CI env var does not poison the in-memory suite. Overall risk is low–medium: prototype-grade lifecycle (no explicit NpgsqlDataSource disposal), runtime DDL from the app, and every request paying a seed INSERT (no-op when row exists).

The diff also includes repo-wide C# brace style updates in .cursor/rules/csharp-style.md and touched server files — acceptable but mixed scope vs NEO-8; consider separate commit or callout in PR.

---

Documentation checked

Document	Assessment
docs/plans/NEO-8-implementation-plan.md	Matches — columns not JSONB, player_position, IPositionStateStore, connection string toggle, CI service + env, integration tests (POST→GET, second factory, unknown player), README/env docs. Partially matches — plan listed optional appsettings.Development.json; implementation correctly leaves connection string unset by default (still within plan).
docs/decomposition/modules/client_server_authority.md	Matches — server remains authoritative for PositionState; persistence does not move truth to the client.
docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md	Matches (post-review update) — Snapshot describes optional Postgres persistence + NEO-8; “Not yet” trimmed to prediction / full slice / telemetry.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches (post-review update) — E1.M1 tracking row includes NEO-8 and Postgres-when-configured.
docs/decomposition/modules/module_dependency_register.md	Matches — E1.M1 remains In Progress; no contract version promotion required for this slice.
docs/decomposition/modules/contracts.md	Matches — HTTP JSON shapes unchanged; NEO-8 is durability, not a breaking wire change.
server/README.md	Matches — persistence, env vars, local Postgres test instructions.

---

Blocking issues

None identified.

---

Suggestions

1. Docs alignment (recommended before merge): Done — E1_M1_InputAndMovementRuntime.md and documentation_and_implementation_alignment.md updated for NEO-8 / optional Postgres (planned follow-up; separate from Nit 1 below).
2. Lifecycle: Tracked: NEO-13 under epic Tech Debt — dispose NpgsqlDataSource on host shutdown (was: singleton never disposed; acceptable for prototype until addressed).
3. Naming clarity: Done — EnsureSchema() XML summary in PostgresPositionStateStore.cs now states the heavy path (read migration + DDL) runs at most once per process; later calls are the fast volatile-read path. No rename.

---

Nits

• .vscode/ appears as untracked in the working tree — Resolved (author): no longer listed as untracked; do not commit editor-local .vscode unless the team opts in.
• EnsureDevPlayerRow every call Addressed: dev row is seeded at PostgreSQL host startup (PostgresDevPlayerSeedHostedService); integration tests re-seed after TRUNCATE via PostgresPositionBootstrap.SeedDevPlayer + IOptions<GamePositionOptions> from the factory.
• Unicode normalization: Deferred / accepted: team expects ASCII player ids; ToLowerInvariant() vs in-memory StringComparer.OrdinalIgnoreCase parity for full Unicode is not a priority unless requirements change.

---

Verification

Before merge, the author should run:

1. No Postgres: dotnet test NeonSprawl.sln -c Release — expect in-memory tests green; Postgres tests skipped if ConnectionStrings__NeonSprawl is unset.
2. With Postgres: docker compose up -d from repo root, then
   export ConnectionStrings__NeonSprawl='Host=localhost;Port=5432;Database=neon_sprawl;Username=neon_sprawl;Password=neon_sprawl_dev'
   and dotnet test NeonSprawl.sln -c Release — all tests including PostgresPositionStateIntegrationTests should pass.
3. Manual AC (optional): dotnet run with connection string set, POST …/move, restart server, GET …/position unchanged.

Ensure all NEO-8 files (including server/db/ and new test types) are staged and committed — at review time much of the implementation was still untracked relative to git status.