diff --git a/.cursor/rules/code-review-agent.md b/.cursor/rules/code-review-agent.md index 834e216..ed79992 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. @@ -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 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 strikethroughs; 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..03eef13 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**: **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 81e8439..2d0ef56 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 @@ -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 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 - 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 new file mode 100644 index 0000000..fd32944 --- /dev/null +++ b/.cursor/rules/planning-implementation-docs.md @@ -0,0 +1,39 @@ +--- +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. 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, **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. + +- **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 + +- **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. +- 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/.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/.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 diff --git a/AGENTS.md b/AGENTS.md index f9438d4..483e291 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,9 +4,9 @@ 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, 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`). **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/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 new file mode 100644 index 0000000..dfc4be0 --- /dev/null +++ b/docs/plans/NEON-15-implementation-plan.md @@ -0,0 +1,79 @@ +# 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. + +## 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 + +- [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 **`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`, `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. **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 + +| 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` | **N/A (shipped)** — holder pattern not needed after Option 2 verification. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **N/A (shipped)** — same. | + +## 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 + +**Resolved:** Double-dispose risk accepted under **Option 2**; **Npgsql 10** + full test run showed no shutdown regressions. **None** outstanding for this story. + +## 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 new file mode 100644 index 0000000..1a2aca3 --- /dev/null +++ b/docs/reviews/2026-04-08-NEON-15.md @@ -0,0 +1,49 @@ +# 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`). + +**Follow-up:** **Suggestions** and **nits** below are **done** (strikethrough + **Done.**). + +## Verdict + +**Approve** + +## 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` | **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. | + +## Blocking issues + +_None._ + +## Suggestions + +**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 + +- ~~**`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 + +- `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. 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) + { + cancellationToken.ThrowIfCancellationRequested(); + 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(); }