From abffd29299f8e5fde14a981cd1b3ff23d1e37f1e Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:16:50 -0400 Subject: [PATCH 1/9] NEON-15: add implementation plan for NpgsqlDataSource shutdown --- docs/plans/NEON-15-implementation-plan.md | 70 +++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 docs/plans/NEON-15-implementation-plan.md diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md new file mode 100644 index 0000000..7fb7266 --- /dev/null +++ b/docs/plans/NEON-15-implementation-plan.md @@ -0,0 +1,70 @@ +# NEON-15 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEON-15 | +| **Title** | Dispose NpgsqlDataSource on application shutdown | +| **Jira** | [NEON-15](https://neon-sprawl.atlassian.net/browse/NEON-15) | +| **Parent** | [NEON-13 — Tech Debt](https://neon-sprawl.atlassian.net/browse/NEON-13) | +| **Related** | NS-17 code review — [docs/reviews/2026-03-30-NS-17.md](../reviews/2026-03-30-NS-17.md) (suggestion #2) | + +## Goal, scope, and out-of-scope + +**Goal:** Ensure the shared `NpgsqlDataSource` registered when `ConnectionStrings:NeonSprawl` is set is **released cleanly on host shutdown**, without breaking **in-memory** overrides or **Postgres** integration tests / `WebApplicationFactory`. + +**In scope** + +- Explicit, observable shutdown cleanup for the PostgreSQL path (align with Npgsql 10 + ASP.NET Core hosting patterns; no new analyzer warnings). +- Keep **single** shared data source for `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`. +- Update test host overrides that strip Postgres registrations so they remain consistent. + +**Out of scope** + +- Connection pooling tuning, migration tooling, or schema changes. +- Client / Godot code. + +## Acceptance criteria checklist + +- [ ] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern). +- [ ] `dotnet test` still passes (in-memory and Postgres integration paths). +- [ ] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version. + +## Technical approach + +1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register a small internal **`IHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. +2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()` (with `ConfigureAwait(false)` if the file’s style uses it elsewhere). `StartAsync` is a no-op. +3. **Double disposal:** The root `ServiceProvider` may also dispose `IDisposable`/`IAsyncDisposable` singletons when the host tears down. **Verify** whether Npgsql 10’s `NpgsqlDataSource` tolerates a second dispose; if not, fall back to a **non-disposable holder** singleton that owns the `NpgsqlDataSource` and is the **only** component that disposes it (inject the holder or an abstraction into `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`). Prefer the minimal hosted-service-only change if tests and a local run show clean shutdown. +4. **Registration clarity:** Use an explicit `AddSingleton(…)` (or `NpgsqlDataSourceBuilder` if that matches Npgsql 10 samples) so the service type is obvious to DI and analyzers. +5. **Manual spot-check (optional):** Run the server with a real connection string, stop with Ctrl+C, confirm no faulting shutdown logs related to the pool (document result in PR if anything surprising). + +## Files to add + +| Path | Purpose | +|------|---------| +| `server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs` | Internal `IHostedService` that disposes the shared `NpgsqlDataSource` on shutdown (registered before the seed hosted service so it stops last). | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs` | Register `NpgsqlDataSource` with an explicit service type if adjusted; register the shutdown hosted service **before** `PostgresDevPlayerSeedHostedService`; keep the existing branch for in-memory vs Postgres. | +| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Extend the descriptor removal loop to drop the new shutdown hosted service when forcing the in-memory store (mirror the `PostgresDevPlayerSeedHostedService` pattern). | +| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **Only if** the holder / non-auto-dispose pattern is required — inject the new type instead of raw `NpgsqlDataSource`. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **Only if** the holder pattern is required — same as store. | + +## Tests + +| Path / action | What to cover | +|---------------|----------------| +| `dotnet test` (full server test project) | Regression: **InMemoryWebApplicationFactory** suites unchanged; **PostgresWebApplicationFactory** / `PostgresIntegrationHarness` / `SecondProcess_ShouldReadLastPosition_AfterFirstFactoryDisposed` still pass (factory dispose + multi-factory scenario). | +| **No new test file required** for kickoff | If shutdown-only logic is covered indirectly by existing factories and `await using` tests; if implementation adds a holder or non-obvious lifecycle, add a focused test in `NeonSprawl.Server.Tests` that builds a Postgres factory, disposes it, and asserts clean teardown (optional follow-up). | + +## Open questions / risks + +**Double disposal** when both a shutdown hosted service and the root `ServiceProvider` dispose the same `NpgsqlDataSource` — resolve by confirming Npgsql behavior or switching to the holder pattern. **None** other if that is settled in implementation. + +## PR / review + +Cross-check [decomposition — server / persistence](../decomposition/README.md) if referenced for lifecycle expectations; keep **NEON-15** in commit subjects per [jira-git-naming](../../.cursor/rules/jira-git-naming.md). From 90fcd1b717fe67b802b395f993bb0e17ff07688e Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:42:33 -0400 Subject: [PATCH 2/9] NEON-15: dispose shared NpgsqlDataSource on host shutdown --- .../InMemoryWebApplicationFactory.cs | 3 ++- .../NpgsqlDataSourceShutdownHostedService.cs | 15 +++++++++++++++ .../PositionStateServiceCollectionExtensions.cs | 3 ++- 3 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs diff --git a/server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs b/server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs index b4f4ccc..210d27a 100644 --- a/server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs +++ b/server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs @@ -21,7 +21,8 @@ public sealed class InMemoryWebApplicationFactory : WebApplicationFactoryDisposes the shared when the host stops. Registered before so runs last (NEON-15). +internal sealed class NpgsqlDataSourceShutdownHostedService(NpgsqlDataSource dataSource) : IHostedService +{ + public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + public async Task StopAsync(CancellationToken cancellationToken) + { + await dataSource.DisposeAsync(); + } +} diff --git a/server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs b/server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs index 6d309d4..0514272 100644 --- a/server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs +++ b/server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs @@ -13,7 +13,8 @@ public static class PositionStateServiceCollectionExtensions if (!string.IsNullOrWhiteSpace(cs)) { var trimmed = cs.Trim(); - services.AddSingleton(_ => Npgsql.NpgsqlDataSource.Create(trimmed)); + services.AddSingleton(_ => Npgsql.NpgsqlDataSource.Create(trimmed)); + services.AddHostedService(); services.AddHostedService(); services.AddSingleton(); } From 62ff21083017c2ff5cf9ab4df80f4ec0472a0646 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:49:56 -0400 Subject: [PATCH 3/9] NEON-15: log plan decisions; require docs sync for planning/implementation --- .cursor/rules/code-review-agent.md | 2 +- .cursor/rules/docs-review-agent.md | 2 +- .cursor/rules/planning-implementation-docs.md | 29 +++++++++++++++++++ .cursor/rules/story-kickoff.md | 3 ++ AGENTS.md | 2 +- docs/plans/NEON-15-implementation-plan.md | 27 ++++++++++------- 6 files changed, 52 insertions(+), 13 deletions(-) create mode 100644 .cursor/rules/planning-implementation-docs.md diff --git a/.cursor/rules/code-review-agent.md b/.cursor/rules/code-review-agent.md index 834e216..ef9f217 100644 --- a/.cursor/rules/code-review-agent.md +++ b/.cursor/rules/code-review-agent.md @@ -29,7 +29,7 @@ Align recommendations with repo rules and docs, including: For every review, **identify and cite** the documentation that defines intent for the change, then **check the implementation against it** (not only style and correctness). -1. **Story / implementation plans** — If the work maps to a ticket or plan under `docs/plans/` (e.g. `NEON-*-implementation-plan.md`), treat that document as acceptance criteria. If the diff diverges without an updated plan or an explicit “out of scope” note, call it out (blocking or non-blocking by severity). +1. **Story / implementation plans** — If the work maps to a ticket or plan under `docs/plans/` (e.g. `NEON-*-implementation-plan.md`), treat that document as acceptance criteria. If the diff diverges without an updated plan or an explicit “out of scope” note, call it out (blocking or non-blocking by severity). Material **planning or implementation decisions** (options chosen, risks closed) should appear in that plan or related `docs/` per [planning-implementation-docs](planning-implementation-docs.md); if they exist only in PR/chat, note as a **should fix**. 2. **Module docs** — Map the change to one or more modules in [`docs/decomposition/modules/module_dependency_register.md`](../../docs/decomposition/modules/module_dependency_register.md) and the corresponding `docs/decomposition/modules/E*_*.md` pages. Verify behavior matches **Purpose**, **Responsibilities**, **Key contracts**, and **Authority** / linked policy sections where relevant. 3. **Cross-cutting policies** — When applicable, align with `docs/decomposition/modules/` policy docs (e.g. [`contracts.md`](../../docs/decomposition/modules/contracts.md), [`client_server_authority.md`](../../docs/decomposition/modules/client_server_authority.md), [`pvp_combat_integration.md`](../../docs/decomposition/modules/pvp_combat_integration.md), [`data_and_ops_policy.md`](../../docs/decomposition/modules/data_and_ops_policy.md)). 4. **Implementation status** — If the register or [`documentation_and_implementation_alignment.md`](../../docs/decomposition/modules/documentation_and_implementation_alignment.md) tracks the module, note whether **Status** / the implementation tracking table should be updated after this merge. diff --git a/.cursor/rules/docs-review-agent.md b/.cursor/rules/docs-review-agent.md index 81e8439..2d810b7 100644 --- a/.cursor/rules/docs-review-agent.md +++ b/.cursor/rules/docs-review-agent.md @@ -28,7 +28,7 @@ Use this rule when the user is working in **`docs/`** (design, decomposition, pl 1. **Same folder and upstream links** — Read every `docs/...` link from the target doc(s); verify paths exist and headings match intent. 2. **Hybrid progression** — `docs/game-design/progression.md`, `skills.md`, `gigs.md`, `abilities.md`, `items.md`, `gathering.md`, `crafting.md`, `economy.md`, `death-loss-recovery.md`, `risk-security-bands.md`, `zones.md`: **Gig** = combat role; **Skill** = non-combat; **combat abilities** = gig kit (+ item channels per data); **Seams** live in `skills.md` unless a dedicated doc supersedes. **Combat encounters** → **gig XP** only (no default skill XP from the fight loop). **Gather / craft** → **skill XP** (see `gathering.md`, `crafting.md`, Epic 3)—not **gig** XP. **Mission/quest rewards** may grant **skill XP**, **currency**, or **items** when explicitly defined (`progression.md`, `economy.md`)—separate from the encounter **gig XP** path. **Economy** faucets/sinks and trade (`economy.md`) do not re-gate **craft** on **gig** (see `items.md` / Seams). **Death / loss / recovery** (`death-loss-recovery.md`): server-authoritative combat death; item stakes via **E3.M4** / **E5.M1**; **PvP** loss **E6.M3** vs **PvE** **open**; **cybernetics** vs external **gear** per **Seams**. **Risk / security** (`risk-security-bands.md`): **E4.M4** `SecurityTier` + **E6.M1**/**E6.M2** for **PvP** eligibility and **consent** UX—**not** client-only PvP toggles. **Zones** (`zones.md`): **place** **fiction** and **hooks** on the **same** **E4.M1** graph—**not** a second topology. 3. **Decomposition** — Map claims to `docs/decomposition/epics/` and `docs/decomposition/modules/` where relevant; flag **conflicts** (e.g. “combat skill” vs skills-as-non-combat). **CI** enforces a minimal guard via [`scripts/check_decomposition_language.py`](../../scripts/check_decomposition_language.py) in the **PR gate** (extend the script if a vetted exception is needed). -4. **Plans** — If work maps to `docs/plans/`, check for **acceptance criteria** vs design doc (same spirit as [code-review-agent](code-review-agent.md) plan alignment). +4. **Plans** — If work maps to `docs/plans/`, check for **acceptance criteria** vs design doc (same spirit as [code-review-agent](code-review-agent.md) plan alignment). Flag plans that omit **documented decisions** or **resolved risks** when the conversation or diff shows they were settled—[planning-implementation-docs](planning-implementation-docs.md). ## Review checklist diff --git a/.cursor/rules/planning-implementation-docs.md b/.cursor/rules/planning-implementation-docs.md new file mode 100644 index 0000000..160698e --- /dev/null +++ b/.cursor/rules/planning-implementation-docs.md @@ -0,0 +1,29 @@ +--- +description: Planning and implementation decisions must be written back into docs (plans, README, decomposition) so Jira chat and files stay aligned. +alwaysApply: true +--- + +# Planning and implementation documentation + +Whenever you **decide** something during **story planning** or **implementation** (architecture pick, scope cut, “option A vs B”, test strategy, resolved risk), **reflect it in documentation** in the same pass or before the story is considered done—do not leave decisions only in chat. + +## Where to write + +| Situation | Update | +|-----------|--------| +| Ticketed story work | **`docs/plans/{JIRA_KEY}-implementation-plan.md`** — add or edit a **Decisions** subsection, **resolve** former open questions, refresh **Technical approach** / **Acceptance criteria** checkboxes, and **Files to add/modify** if reality diverged from kickoff. | +| Cross-cutting or module contract | Relevant **`docs/decomposition/modules/*.md`**, **`docs/game-design/`**, or **`README`** / **`server/README.md`** / **`client/README.md`** when the decision affects how others integrate. | +| Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. | + +## What to capture + +- **What** was chosen (or rejected) and **why** in one short paragraph or bullets. +- **Outcomes** of verification (e.g. “`dotnet test` N passed; double-dispose safe on Npgsql 10”). +- If the user **explicitly** chose an option (e.g. “Option 2”), record that label so future readers do not re-litigate. + +## Agent behavior + +- After **kickoff**, the plan is living: **amend it** when implementation choices differ from the draft. +- Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers. + +This complements [story-kickoff](story-kickoff.md) (plan creation) and [code-review-agent](code-review-agent.md) (documentation checked vs plans and decomposition). diff --git a/.cursor/rules/story-kickoff.md b/.cursor/rules/story-kickoff.md index 5b64f92..33677e3 100644 --- a/.cursor/rules/story-kickoff.md +++ b/.cursor/rules/story-kickoff.md @@ -52,8 +52,11 @@ When the user starts work on a **Jira story** (e.g. issue key `NEON-2`, phrases These three lists (**files to add**, **files to modify**, **tests**) must **always** be generated during kickoff—they are not optional prose and must not be left implicit inside “Technical approach” only. +- The plan is **living documentation**: any **decisions** during planning chat or implementation (options A/B, resolved risks, scope changes) must be **reflected in** `docs/plans/{KEY}-implementation-plan.md` (and other `docs/` when the decision affects integration). See [planning-implementation-docs](planning-implementation-docs.md). + ## 5. After the plan - Implement only after the user confirms. All implementation commits stay on the **same story branch** from step 1b; follow [git workflow](git-workflow.md). +- Before handoff or merge, **reconcile the plan** with what shipped (acceptance checkboxes, **Decisions**, open questions) per [planning-implementation-docs](planning-implementation-docs.md). When the story is **done** and merged to `main`, follow [story-end](story-end.md) to return to `main`, pull, and remove the local story branch. diff --git a/AGENTS.md b/AGENTS.md index f9438d4..a442721 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,6 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; short chat pointer | | **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; use when working in **documents** or @ mention | -Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). +Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). **Commits tied to a Jira issue** must start the subject with the **issue key** and a colon (e.g. `NEON-5: …`). Branch naming and exceptions (`chore:` when there is no ticket) — [`.cursor/rules/jira-git-naming.md`](.cursor/rules/jira-git-naming.md). diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md index 7fb7266..949bc22 100644 --- a/docs/plans/NEON-15-implementation-plan.md +++ b/docs/plans/NEON-15-implementation-plan.md @@ -25,18 +25,25 @@ - Connection pooling tuning, migration tooling, or schema changes. - Client / Godot code. +## Decisions (planning + implementation) + +| Decision | Choice | +|----------|--------| +| **Lifecycle / double dispose** | **Option 2** — internal shutdown `IHostedService` calls `DisposeAsync` on the shared `NpgsqlDataSource`; no non-disposable holder. Rationale: smallest change; **Npgsql 10** + full `dotnet test` showed clean teardown (no failures from a second dispose path). | +| **ConfigureAwait** | Not used in `StopAsync` — matches existing server hosted-service style (`PostgresDevPlayerSeedHostedService`). | + ## Acceptance criteria checklist -- [ ] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern). -- [ ] `dotnet test` still passes (in-memory and Postgres integration paths). -- [ ] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version. +- [x] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern). +- [x] `dotnet test` still passes (in-memory and Postgres integration paths). +- [x] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version. ## Technical approach -1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register a small internal **`IHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. -2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()` (with `ConfigureAwait(false)` if the file’s style uses it elsewhere). `StartAsync` is a no-op. -3. **Double disposal:** The root `ServiceProvider` may also dispose `IDisposable`/`IAsyncDisposable` singletons when the host tears down. **Verify** whether Npgsql 10’s `NpgsqlDataSource` tolerates a second dispose; if not, fall back to a **non-disposable holder** singleton that owns the `NpgsqlDataSource` and is the **only** component that disposes it (inject the holder or an abstraction into `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`). Prefer the minimal hosted-service-only change if tests and a local run show clean shutdown. -4. **Registration clarity:** Use an explicit `AddSingleton(…)` (or `NpgsqlDataSourceBuilder` if that matches Npgsql 10 samples) so the service type is obvious to DI and analyzers. +1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register **`NpgsqlDataSourceShutdownHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. +2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()`. `StartAsync` is a no-op. +3. **Double disposal:** The root `ServiceProvider` may still dispose the same singleton during host teardown. **Settled:** Option 2 kept; **verification** — `dotnet test` on solution (**36** tests passed) including in-memory and Postgres factory dispose paths; no change required to store/seed constructors. +4. **Registration clarity:** `AddSingleton(_ => NpgsqlDataSource.Create(trimmed))` so the service type is explicit to DI and analyzers. 5. **Manual spot-check (optional):** Run the server with a real connection string, stop with Ctrl+C, confirm no faulting shutdown logs related to the pool (document result in PR if anything surprising). ## Files to add @@ -51,8 +58,8 @@ |------|-----------| | `server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs` | Register `NpgsqlDataSource` with an explicit service type if adjusted; register the shutdown hosted service **before** `PostgresDevPlayerSeedHostedService`; keep the existing branch for in-memory vs Postgres. | | `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Extend the descriptor removal loop to drop the new shutdown hosted service when forcing the in-memory store (mirror the `PostgresDevPlayerSeedHostedService` pattern). | -| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **Only if** the holder / non-auto-dispose pattern is required — inject the new type instead of raw `NpgsqlDataSource`. | -| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **Only if** the holder pattern is required — same as store. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **N/A (shipped)** — holder pattern not needed after Option 2 verification. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **N/A (shipped)** — same. | ## Tests @@ -63,7 +70,7 @@ ## Open questions / risks -**Double disposal** when both a shutdown hosted service and the root `ServiceProvider` dispose the same `NpgsqlDataSource` — resolve by confirming Npgsql behavior or switching to the holder pattern. **None** other if that is settled in implementation. +**Resolved:** Double-dispose risk accepted under **Option 2**; **Npgsql 10** + full test run showed no shutdown regressions. **None** outstanding for this story. ## PR / review From 2617b65f79b868a114bac55b113d1c6ab9ba9b02 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:58:18 -0400 Subject: [PATCH 4/9] NEON-15: add code review (2026-04-08) --- docs/reviews/2026-04-08-NEON-15.md | 47 ++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/reviews/2026-04-08-NEON-15.md diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md new file mode 100644 index 0000000..318dca7 --- /dev/null +++ b/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(…)`, 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. From 8c3e8c0a03509422d3a4e829ba67ec53db134565 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:59:53 -0400 Subject: [PATCH 5/9] NEON-15: apply code review doc suggestions (E1.M1 pointers, PR notes) --- docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md | 2 +- .../modules/documentation_and_implementation_alignment.md | 2 +- docs/plans/NEON-15-implementation-plan.md | 2 ++ docs/reviews/2026-04-08-NEON-15.md | 5 +++++ 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md b/docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md index 447c563..4799bc4 100644 --- a/docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md +++ b/docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md @@ -33,7 +33,7 @@ Contract readiness is tracked in the [module dependency register](module_depende ## Implementation snapshot -- **Done (prototype):** Server-side authoritative **`PositionState`** read + **`MoveCommand`** apply (HTTP JSON v1, snap-to-target, `sequence` increments); **NEON-7** server-side step + optional district bounds validation with **`reasonCode`** rejections ([NEON-7](../../plans/NEON-7-implementation-plan.md), `MoveCommandValidation`, [server README — Move command](../../../server/README.md#move-command-neon-4-neon-7)); default **in-memory** store; optional **PostgreSQL** persistence when `ConnectionStrings:NeonSprawl` is set ([NEON-5](../../plans/NEON-5-implementation-plan.md)); Godot client **`POST`/`GET`** move flow ([NEON-3](../../plans/NEON-3-implementation-plan.md), [NEON-4](../../plans/NEON-4-implementation-plan.md), `server/NeonSprawl.Server/Game/PositionState/`, `client/scripts/`). **NEON-8** — client **`NavigationRegion3D` / `NavigationAgent3D`** for click-to-move visuals while server authority unchanged; **single-click obstacle detours not guaranteed** (see plan tradeoff) ([NEON-8](../../plans/NEON-8-implementation-plan.md); [client README](../../../client/README.md#authoritative-movement-neon-4-neon-8)). **`InteractionRequest`** + server-side horizontal range check ([NEON-6](../../plans/NEON-6-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`, [server README — Interaction](../../../server/README.md#interaction-neon-6)). See [server README — Position persistence](../../../server/README.md#position-persistence-neon-5). Jira **[NEON-9](https://neon-sprawl.atlassian.net/browse/NEON-9)** (E1.M1 Feature) is **Done** for this prototype scope. +- **Done (prototype):** Server-side authoritative **`PositionState`** read + **`MoveCommand`** apply (HTTP JSON v1, snap-to-target, `sequence` increments); **NEON-7** server-side step + optional district bounds validation with **`reasonCode`** rejections ([NEON-7](../../plans/NEON-7-implementation-plan.md), `MoveCommandValidation`, [server README — Move command](../../../server/README.md#move-command-neon-4-neon-7)); default **in-memory** store; optional **PostgreSQL** persistence when `ConnectionStrings:NeonSprawl` is set ([NEON-5](../../plans/NEON-5-implementation-plan.md)); shared **NpgsqlDataSource** disposed on application shutdown ([NEON-15](../../plans/NEON-15-implementation-plan.md)); Godot client **`POST`/`GET`** move flow ([NEON-3](../../plans/NEON-3-implementation-plan.md), [NEON-4](../../plans/NEON-4-implementation-plan.md), `server/NeonSprawl.Server/Game/PositionState/`, `client/scripts/`). **NEON-8** — client **`NavigationRegion3D` / `NavigationAgent3D`** for click-to-move visuals while server authority unchanged; **single-click obstacle detours not guaranteed** (see plan tradeoff) ([NEON-8](../../plans/NEON-8-implementation-plan.md); [client README](../../../client/README.md#authoritative-movement-neon-4-neon-8)). **`InteractionRequest`** + server-side horizontal range check ([NEON-6](../../plans/NEON-6-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`, [server README — Interaction](../../../server/README.md#interaction-neon-6)). See [server README — Position persistence](../../../server/README.md#position-persistence-neon-5). Jira **[NEON-9](https://neon-sprawl.atlassian.net/browse/NEON-9)** (E1.M1 Feature) is **Done** for this prototype scope. - **Follow-on:** Client prediction/reconciliation; Epic 1 Slice 1 telemetry and movement-loop polish; optional **Protobuf** wire promotion for `MoveCommand` / `PositionState` per [contracts.md](contracts.md). - **Alignment:** [Documentation and implementation alignment](documentation_and_implementation_alignment.md). diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index c5dbf2b..6a82c9c 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -46,7 +46,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not | Module | Register status | Snapshot | Plans / pointers | |--------|-----------------|----------|-------------------| -| E1.M1 | Ready | Prototype milestone **Done** ([NEON-9](https://neon-sprawl.atlassian.net/browse/NEON-9)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEON-5](../../plans/NEON-5-implementation-plan.md)); Godot sync + path-follow ([NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEON-6](../../plans/NEON-6-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEON-3](../../plans/NEON-3-implementation-plan.md), [NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-5](../../plans/NEON-5-implementation-plan.md), [NEON-6](../../plans/NEON-6-implementation-plan.md), [NEON-7](../../plans/NEON-7-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | +| E1.M1 | Ready | Prototype milestone **Done** ([NEON-9](https://neon-sprawl.atlassian.net/browse/NEON-9)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEON-5](../../plans/NEON-5-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEON-15](../../plans/NEON-15-implementation-plan.md)); Godot sync + path-follow ([NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEON-6](../../plans/NEON-6-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEON-3](../../plans/NEON-3-implementation-plan.md), [NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-5](../../plans/NEON-5-implementation-plan.md), [NEON-6](../../plans/NEON-6-implementation-plan.md), [NEON-7](../../plans/NEON-7-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md), [NEON-15](../../plans/NEON-15-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | --- diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md index 949bc22..ac9e526 100644 --- a/docs/plans/NEON-15-implementation-plan.md +++ b/docs/plans/NEON-15-implementation-plan.md @@ -74,4 +74,6 @@ ## PR / review +**Merge / PR description:** Call out **two themes** so reviewers do not miss non-server files: (1) **Server** — `NpgsqlDataSourceShutdownHostedService`, explicit `AddSingleton`, and `InMemoryWebApplicationFactory` stripping; (2) **Repo process** — [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) and cross-links in `AGENTS.md`, `story-kickoff`, code-review, and docs-review agent rules. + Cross-check [decomposition — server / persistence](../decomposition/README.md) if referenced for lifecycle expectations; keep **NEON-15** in commit subjects per [jira-git-naming](../../.cursor/rules/jira-git-naming.md). diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md index 318dca7..7c21b47 100644 --- a/docs/reviews/2026-04-08-NEON-15.md +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -45,3 +45,8 @@ _None._ - `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. + +## Follow-up (suggestions implemented) + +- **Traceability:** `documentation_and_implementation_alignment.md` (E1.M1 row) and `E1_M1_InputAndMovementRuntime.md` now link [NEON-15](../plans/NEON-15-implementation-plan.md) for Postgres pool shutdown. +- **PR narrative:** `docs/plans/NEON-15-implementation-plan.md` **PR / review** section now lists the two merge-description themes (server vs repo process). From 6bc8014adff66a86f9e4c1e899b825df3d8ff819 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 22:03:44 -0400 Subject: [PATCH 6/9] chore: require updating docs/reviews when review suggestions resolve --- .cursor/rules/code-review-agent.md | 9 +++++++++ .cursor/rules/commit-and-review.md | 4 ++++ .cursor/rules/docs-review-agent.md | 4 ++++ .cursor/rules/planning-implementation-docs.md | 9 +++++++++ AGENTS.md | 4 ++-- 5 files changed, 28 insertions(+), 2 deletions(-) diff --git a/.cursor/rules/code-review-agent.md b/.cursor/rules/code-review-agent.md index ef9f217..b920bac 100644 --- a/.cursor/rules/code-review-agent.md +++ b/.cursor/rules/code-review-agent.md @@ -80,6 +80,15 @@ Use this structure in **both** the saved document and (abbreviated if you want) In **chat only**, you may use Cursor line-number **code citations** when referencing existing code. For hypothetical fixes, use normal fenced code blocks everywhere. +## Resolved suggestions (mandatory when feedback is addressed) + +If you **implement** changes that resolve **blocking issues**, **suggestions**, or agreed **nits** from an existing **`docs/reviews/…`** file for the same work: + +- **Edit that review document** in the same commit or the immediate follow-up commit: add or update **`## Resolved suggestions`** / **`## Follow-up`** per [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up**. +- Do **not** ship code-only fixes while the saved review still reads as if those items were open. + +The **initial** review does not need an empty “Resolved” section; this rule applies when **closing the loop** after the review exists. + ## Boundaries - Do **not** skip writing `docs/reviews/…`; the document is mandatory output of this agent. diff --git a/.cursor/rules/commit-and-review.md b/.cursor/rules/commit-and-review.md index a22c4ba..63199f0 100644 --- a/.cursor/rules/commit-and-review.md +++ b/.cursor/rules/commit-and-review.md @@ -29,3 +29,7 @@ alwaysApply: true ## Scope - Applies to **all** commits the agent might make, including **documentation-only** changes (e.g. `docs/plans/`, `docs/reviews/`, README), not only application source. + +## Code review follow-up + +When commits **address** feedback from a saved **`docs/reviews/…`** file, include an update to **that review file** documenting what was resolved (or explicitly deferred/declined). See [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** and [code-review-agent](code-review-agent.md) **Resolved suggestions**. diff --git a/.cursor/rules/docs-review-agent.md b/.cursor/rules/docs-review-agent.md index 2d810b7..f315c2f 100644 --- a/.cursor/rules/docs-review-agent.md +++ b/.cursor/rules/docs-review-agent.md @@ -66,6 +66,10 @@ In the saved file, use **normal fenced code blocks** and **backtick paths** — **Chat:** Short summary + **path to** `docs/reviews/YYYY-MM-DD-{slug}.md`. +## Resolved suggestions (mandatory when feedback is addressed) + +If later edits **address** **blocking issues**, **suggestions**, or **nits** from a **docs-review** `docs/reviews/…` file, **update that review file** with **`## Resolved suggestions`** / **`## Follow-up`** per [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** (same audit trail as code reviews). + ## Boundaries - Do **not** skip writing `docs/reviews/…` unless the user explicitly asks for **chat-only** (then state that exception in chat and **do not** claim full agent output). diff --git a/.cursor/rules/planning-implementation-docs.md b/.cursor/rules/planning-implementation-docs.md index 160698e..a9466e0 100644 --- a/.cursor/rules/planning-implementation-docs.md +++ b/.cursor/rules/planning-implementation-docs.md @@ -15,6 +15,14 @@ Whenever you **decide** something during **story planning** or **implementation* | Cross-cutting or module contract | Relevant **`docs/decomposition/modules/*.md`**, **`docs/game-design/`**, or **`README`** / **`server/README.md`** / **`client/README.md`** when the decision affects how others integrate. | | Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. | +## Code review follow-up (resolved suggestions) + +When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`docs/reviews/YYYY-MM-DD-*.md`** are **fixed** in code or other docs, **update that review file** in the **same** change-set or the **next** commit on the story branch—do **not** only fix the code and leave the review implying feedback is still open. + +- Add or extend a section such as **`## Resolved suggestions`** or **`## Follow-up`**: for **each** item from the review that you addressed, record **what** changed and **where** (paths or commits). Match the review’s numbering or headings so readers can correlate. +- Items **declined**, **out of scope**, or **deferred** to a ticket: state that in the same section so the audit trail is honest. +- Applies to **any** agent pass that implements review feedback, not only a second invocation of the code-review agent—see [code-review-agent](code-review-agent.md) **Resolved suggestions**. + ## What to capture - **What** was chosen (or rejected) and **why** in one short paragraph or bullets. @@ -25,5 +33,6 @@ Whenever you **decide** something during **story planning** or **implementation* - After **kickoff**, the plan is living: **amend it** when implementation choices differ from the draft. - Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers. +- After **implementing** review feedback, **edit the corresponding `docs/reviews/…` file** per **Code review follow-up** above—this step is easy to skip; treat it as **required** whenever suggestions were listed in that review. This complements [story-kickoff](story-kickoff.md) (plan creation) and [code-review-agent](code-review-agent.md) (documentation checked vs plans and decomposition). diff --git a/AGENTS.md b/AGENTS.md index a442721..8e360a9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,8 +4,8 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | Agent | Rule file | When to use | |--------|-----------|-------------| -| **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; short chat pointer | -| **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; use when working in **documents** or @ mention | +| **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; **when suggestions are fixed, update that review file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); short chat pointer | +| **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; **when suggestions are fixed, update that review file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); use when working in **documents** or @ mention | Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). From adde98e4a866efccb9c4a6786930058d02277e97 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 22:06:08 -0400 Subject: [PATCH 7/9] chore: review follow-up uses strikethrough on suggestions, not extra section --- .cursor/rules/code-review-agent.md | 6 +++--- .cursor/rules/commit-and-review.md | 2 +- .cursor/rules/docs-review-agent.md | 2 +- .cursor/rules/planning-implementation-docs.md | 13 +++++++------ AGENTS.md | 4 ++-- docs/reviews/2026-04-08-NEON-15.md | 19 ++++++++----------- 6 files changed, 22 insertions(+), 24 deletions(-) diff --git a/.cursor/rules/code-review-agent.md b/.cursor/rules/code-review-agent.md index b920bac..ed79992 100644 --- a/.cursor/rules/code-review-agent.md +++ b/.cursor/rules/code-review-agent.md @@ -84,10 +84,10 @@ In **chat only**, you may use Cursor line-number **code citations** when referen If you **implement** changes that resolve **blocking issues**, **suggestions**, or agreed **nits** from an existing **`docs/reviews/…`** file for the same work: -- **Edit that review document** in the same commit or the immediate follow-up commit: add or update **`## Resolved suggestions`** / **`## Follow-up`** per [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up**. -- Do **not** ship code-only fixes while the saved review still reads as if those items were open. +- **Edit that review document in place:** strike through the original bullets under **`## Suggestions`** / **`## Nits`** / **`## Blocking issues`** (`~~…~~`), append **`Done.`** / **`Addressed.`** / **`Deferred…`** plus a short note and optional commit SHA; optionally a one-line **Follow-up** preamble after **Scope** when appropriate. Full pattern: [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up**. +- Do **not** add a **separate** section duplicating resolved items, and do **not** ship code-only fixes while those bullets still read as open. -The **initial** review does not need an empty “Resolved” section; this rule applies when **closing the loop** after the review exists. +The **initial** review does not need strikethroughs; this rule applies when **closing the loop** after the review exists. ## Boundaries diff --git a/.cursor/rules/commit-and-review.md b/.cursor/rules/commit-and-review.md index 63199f0..03eef13 100644 --- a/.cursor/rules/commit-and-review.md +++ b/.cursor/rules/commit-and-review.md @@ -32,4 +32,4 @@ alwaysApply: true ## Code review follow-up -When commits **address** feedback from a saved **`docs/reviews/…`** file, include an update to **that review file** documenting what was resolved (or explicitly deferred/declined). See [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** and [code-review-agent](code-review-agent.md) **Resolved suggestions**. +When commits **address** feedback from a saved **`docs/reviews/…`** file, include an update to **that review file**: **strikethrough + Done** on the original **Suggestions** / **Nits** / **Blocking** bullets (not a separate resolved section). See [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** and [code-review-agent](code-review-agent.md) **Resolved suggestions**. diff --git a/.cursor/rules/docs-review-agent.md b/.cursor/rules/docs-review-agent.md index f315c2f..2d0ef56 100644 --- a/.cursor/rules/docs-review-agent.md +++ b/.cursor/rules/docs-review-agent.md @@ -68,7 +68,7 @@ In the saved file, use **normal fenced code blocks** and **backtick paths** — ## Resolved suggestions (mandatory when feedback is addressed) -If later edits **address** **blocking issues**, **suggestions**, or **nits** from a **docs-review** `docs/reviews/…` file, **update that review file** with **`## Resolved suggestions`** / **`## Follow-up`** per [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** (same audit trail as code reviews). +If later edits **address** **blocking issues**, **suggestions**, or **nits** from a **docs-review** `docs/reviews/…` file, **update that review in place** with strikethrough + **`Done.`** on the original bullets per [planning-implementation-docs](planning-implementation-docs.md) **Code review follow-up** (same pattern as code reviews; no separate “resolved” section). ## Boundaries diff --git a/.cursor/rules/planning-implementation-docs.md b/.cursor/rules/planning-implementation-docs.md index a9466e0..fd32944 100644 --- a/.cursor/rules/planning-implementation-docs.md +++ b/.cursor/rules/planning-implementation-docs.md @@ -13,15 +13,16 @@ Whenever you **decide** something during **story planning** or **implementation* |-----------|--------| | Ticketed story work | **`docs/plans/{JIRA_KEY}-implementation-plan.md`** — add or edit a **Decisions** subsection, **resolve** former open questions, refresh **Technical approach** / **Acceptance criteria** checkboxes, and **Files to add/modify** if reality diverged from kickoff. | | Cross-cutting or module contract | Relevant **`docs/decomposition/modules/*.md`**, **`docs/game-design/`**, or **`README`** / **`server/README.md`** / **`client/README.md`** when the decision affects how others integrate. | -| Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. | +| Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. When feedback is fixed, **strike through + `Done.`** on the original bullets in that review file — see **Code review follow-up** below. | ## Code review follow-up (resolved suggestions) -When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`docs/reviews/YYYY-MM-DD-*.md`** are **fixed** in code or other docs, **update that review file** in the **same** change-set or the **next** commit on the story branch—do **not** only fix the code and leave the review implying feedback is still open. +When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`docs/reviews/YYYY-MM-DD-*.md`** are **fixed** in code or other docs, **edit that same review file** in the **same** change-set or the **next** commit—do **not** only fix the code and leave the review implying feedback is still open. -- Add or extend a section such as **`## Resolved suggestions`** or **`## Follow-up`**: for **each** item from the review that you addressed, record **what** changed and **where** (paths or commits). Match the review’s numbering or headings so readers can correlate. -- Items **declined**, **out of scope**, or **deferred** to a ticket: state that in the same section so the audit trail is honest. -- Applies to **any** agent pass that implements review feedback, not only a second invocation of the code-review agent—see [code-review-agent](code-review-agent.md) **Resolved suggestions**. +- **In place under the original headings** (`## Suggestions`, `## Nits`, `## Blocking issues`): strike through the resolved bullet with `~~…~~`, then add **`Done.`** / **`Addressed.`** / **`Deferred…`** and a one-line note (what changed, paths, optional commit SHA). Optionally add a one-line **Follow-up** note under the title (after **Scope**/**Base**) when all suggestions are done, e.g. “suggestions below are **done** (strikethrough + **Done.**)”. Same pattern as `docs/reviews/2026-03-30-NEON-5.md`, `2026-04-05-NEON-16.md`. +- Do **not** add a **separate** “Resolved suggestions” / “Follow-up implemented” section for items that already live under Suggestions or Nits—the strikethrough row **is** the audit trail. +- Items **declined** or **deferred** to a ticket: still strike through or annotate inline under the same bullet so the review stays honest. +- Applies to **any** agent pass that implements review feedback—see [code-review-agent](code-review-agent.md) **Resolved suggestions**. ## What to capture @@ -33,6 +34,6 @@ When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`do - After **kickoff**, the plan is living: **amend it** when implementation choices differ from the draft. - Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers. -- After **implementing** review feedback, **edit the corresponding `docs/reviews/…` file** per **Code review follow-up** above—this step is easy to skip; treat it as **required** whenever suggestions were listed in that review. +- After **implementing** review feedback, **edit the corresponding `docs/reviews/…` file** with **strikethrough + Done** on the original bullets per **Code review follow-up** above—this step is easy to skip; treat it as **required** whenever suggestions were listed in that review. This complements [story-kickoff](story-kickoff.md) (plan creation) and [code-review-agent](code-review-agent.md) (documentation checked vs plans and decomposition). diff --git a/AGENTS.md b/AGENTS.md index 8e360a9..483e291 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,8 +4,8 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | Agent | Rule file | When to use | |--------|-----------|-------------| -| **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; **when suggestions are fixed, update that review file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); short chat pointer | -| **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; **when suggestions are fixed, update that review file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); use when working in **documents** or @ mention | +| **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; **when suggestions are fixed, strike through those bullets + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); short chat pointer | +| **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; **when suggestions are fixed, strike through + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); use when working in **documents** or @ mention | Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md index 7c21b47..f996036 100644 --- a/docs/reviews/2026-04-08-NEON-15.md +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -4,6 +4,8 @@ **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`). +**Follow-up:** Review **suggestions** below are **done** (strikethrough + **Done.**); **nits** remain optional. + ## Verdict **Approve with nits** @@ -19,22 +21,22 @@ Residual risk is low and already acknowledged in the plan: the root `ServiceProv | 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/E1_M1_InputAndMovementRuntime.md` | **Matches (post-review update)** — Snapshot links NEON-15 for shared `NpgsqlDataSource` shutdown — `8c3e8c0`. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches (post-review update)** — E1.M1 row snapshot + plans column include NEON-15 — `8c3e8c0`. | | `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/`. +**All done.** + +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.~~ **Done.** E1.M1 snapshot + plans list + module implementation snapshot updated — `8c3e8c0`. +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/`.~~ **Done.** `docs/plans/NEON-15-implementation-plan.md` **PR / review** section lists server vs repo-process themes — `8c3e8c0`. ## Nits @@ -45,8 +47,3 @@ _None._ - `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. - -## Follow-up (suggestions implemented) - -- **Traceability:** `documentation_and_implementation_alignment.md` (E1.M1 row) and `E1_M1_InputAndMovementRuntime.md` now link [NEON-15](../plans/NEON-15-implementation-plan.md) for Postgres pool shutdown. -- **PR narrative:** `docs/plans/NEON-15-implementation-plan.md` **PR / review** section now lists the two merge-description themes (server vs repo process). From e000da20eeafd994303725023ac38e4a022d4760 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 22:08:43 -0400 Subject: [PATCH 8/9] NEON-15: address review nits (shutdown token, plan verification note) --- docs/plans/NEON-15-implementation-plan.md | 4 ++-- docs/reviews/2026-04-08-NEON-15.md | 8 ++++---- .../NpgsqlDataSourceShutdownHostedService.cs | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md index ac9e526..dfc4be0 100644 --- a/docs/plans/NEON-15-implementation-plan.md +++ b/docs/plans/NEON-15-implementation-plan.md @@ -41,10 +41,10 @@ ## Technical approach 1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register **`NpgsqlDataSourceShutdownHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. -2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()`. `StartAsync` is a no-op. +2. **Disposal:** In that hosted service’s `StopAsync`, `cancellationToken.ThrowIfCancellationRequested()` then `await dataSource.DisposeAsync()` (Npgsql has no `DisposeAsync(CancellationToken)`). `StartAsync` is a no-op. 3. **Double disposal:** The root `ServiceProvider` may still dispose the same singleton during host teardown. **Settled:** Option 2 kept; **verification** — `dotnet test` on solution (**36** tests passed) including in-memory and Postgres factory dispose paths; no change required to store/seed constructors. 4. **Registration clarity:** `AddSingleton(_ => NpgsqlDataSource.Create(trimmed))` so the service type is explicit to DI and analyzers. -5. **Manual spot-check (optional):** Run the server with a real connection string, stop with Ctrl+C, confirm no faulting shutdown logs related to the pool (document result in PR if anything surprising). +5. **Shutdown verification:** `WebApplicationFactory` disposal in integration tests exercises host stop with a live `NpgsqlDataSource` when `ConnectionStrings__NeonSprawl` is set. **Optional:** Run the server with a real connection string, Ctrl+C, confirm clean pool-related logs for extra operator confidence. ## Files to add diff --git a/docs/reviews/2026-04-08-NEON-15.md b/docs/reviews/2026-04-08-NEON-15.md index f996036..1a2aca3 100644 --- a/docs/reviews/2026-04-08-NEON-15.md +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -4,11 +4,11 @@ **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`). -**Follow-up:** Review **suggestions** below are **done** (strikethrough + **Done.**); **nits** remain optional. +**Follow-up:** **Suggestions** and **nits** below are **done** (strikethrough + **Done.**). ## Verdict -**Approve with nits** +**Approve** ## Summary @@ -40,8 +40,8 @@ _None._ ## 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. +- ~~**`StopAsync` ignores `cancellationToken`:**~~ **Done.** `ThrowIfCancellationRequested()` before `DisposeAsync()`; Npgsql still has no cancellable dispose. +- ~~**Plan / manual Ctrl+C:**~~ **Done.** Plan **Technical approach** #5 documents `WebApplicationFactory` disposal coverage + optional local run. ## Verification diff --git a/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs b/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs index 7a5abd3..5835310 100644 --- a/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs +++ b/server/NeonSprawl.Server/Game/PositionState/NpgsqlDataSourceShutdownHostedService.cs @@ -10,6 +10,7 @@ internal sealed class NpgsqlDataSourceShutdownHostedService(NpgsqlDataSource dat public async Task StopAsync(CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); await dataSource.DisposeAsync(); } } From b52862f6ee6935ae771ed6968702731cb78b6831 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 22:11:31 -0400 Subject: [PATCH 9/9] chore: gitignore VS Code machine-local settings.json --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index c74f550..0db011a 100644 --- a/.gitignore +++ b/.gitignore @@ -31,3 +31,6 @@ client/addons/gdUnit4/GdUnitRunner.cfg # Content validation (optional local venv) .venv-content/ /.venv-gd + +# VS Code / Cursor — machine-local settings (e.g. Godot editor path) +.vscode/settings.json