NEON-15: add code review (2026-04-08)

docs/reviews/2026-04-08-NEON-15.md

@@ -0,0 +1,47 @@
+# Code review: NEON-15
+
+**Date:** 2026-04-08  
+**Scope:** Branch `NEON-15-dispose-npgsql-datasource` vs `main` — [NEON-15](https://neon-sprawl.atlassian.net/browse/NEON-15) (dispose `NpgsqlDataSource` on shutdown), plus planning-doc rule cross-links.  
+**Base:** `main` (range `main...HEAD`, commits `abffd29` → `62ff210`).
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+The branch adds `NpgsqlDataSourceShutdownHostedService`, registers it **before** `PostgresDevPlayerSeedHostedService` so `StopAsync` runs **last** (correct use of reverse shutdown order), switches to `AddSingleton<NpgsqlDataSource>(…)`, and extends `InMemoryWebApplicationFactory` so the new hosted service is stripped with the rest of the Postgres path. The implementation matches the written plan and acceptance criteria. A separate commit adds `.cursor/rules/planning-implementation-docs.md` and wires it into kickoff, code-review, docs-review, and `AGENTS.md`; that is coherent and self-consistent.
+
+Residual risk is low and already acknowledged in the plan: the root `ServiceProvider` may still dispose the same `NpgsqlDataSource` after hosted services stop; the team accepted **Option 2** after green tests. No API or authority contract changes.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| `docs/plans/NEON-15-implementation-plan.md` | **Matches** — Decisions (Option 2, ConfigureAwait), AC checked, technical approach and file lists align with the diff; open questions resolved per [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md). |
+| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **N/A** for merge blockers — Postgres path is already documented at prototype level; this change is **lifecycle / ops hygiene** under the same `Game/PositionState` area, not a new contract. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E1.M1 snapshot lists many NEON plans; **NEON-15** is not linked yet. Optional follow-up to add a short pointer (suggestion below). |
+| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no status or dependency change required for a shutdown-only fix. |
+| `docs/decomposition/modules/client_server_authority.md`, `contracts.md` | **N/A** — no authority or wire-shape change. |
+| `.cursor/rules/planning-implementation-docs.md`, `story-kickoff.md`, `code-review-agent.md`, `docs-review-agent.md`, `AGENTS.md` | **Matches** — rule text is clear; relative links in agent rules remain valid. |
+
+**Register / tracking table:** No update **required** for merge; optional E1.M1 snapshot pointer to NEON-15 for traceability.
+
+## Blocking issues
+
+_None._
+
+## Suggestions
+
+1. **Traceability (optional):** Add `NEON-15` / `docs/plans/NEON-15-implementation-plan.md` to the E1.M1 **Plans / pointers** column in `documentation_and_implementation_alignment.md` (or a one-line note under Postgres in `E1_M1_InputAndMovementRuntime.md`) so future readers find shutdown lifecycle work next to NEON-5.
+2. **PR narrative:** Call out the **two themes** in the branch (server disposal + planning-doc rule) so reviewers do not miss the `.cursor/` and `AGENTS.md` changes if they only skim `server/`.
+
+## Nits
+
+- **Nit:** `NpgsqlDataSourceShutdownHostedService.StopAsync` ignores `cancellationToken`; acceptable for short dispose; if Npgsql ever exposes cancellation-aware teardown, consider threading it through.
+- **Nit:** Plan still lists optional manual Ctrl+C run with a real connection string — worth doing once before merge if CI does not exercise long-lived `WebApplication` shutdown the same way as a local server.
+
+## Verification
+
+- `dotnet test NeonSprawl.sln` — **Passed** (36 tests) at review time.
+- Optional: run `NeonSprawl.Server` with `ConnectionStrings:NeonSprawl` set, stop the process, confirm clean shutdown logs.