Merge pull request #32 from ViPro-Technologies/NEON-15-dispose-npgsql-datasource
Neon 15 dispose npgsql datasourcepull/34/head
commit
8865329210
|
|
@ -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).
|
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.
|
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)).
|
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.
|
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.
|
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
|
## Boundaries
|
||||||
|
|
||||||
- Do **not** skip writing `docs/reviews/…`; the document is mandatory output of this agent.
|
- Do **not** skip writing `docs/reviews/…`; the document is mandatory output of this agent.
|
||||||
|
|
|
||||||
|
|
@ -29,3 +29,7 @@ alwaysApply: true
|
||||||
## Scope
|
## Scope
|
||||||
|
|
||||||
- Applies to **all** commits the agent might make, including **documentation-only** changes (e.g. `docs/plans/`, `docs/reviews/`, README), not only application source.
|
- 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**.
|
||||||
|
|
|
||||||
|
|
@ -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.
|
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.
|
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).
|
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
|
## 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`.
|
**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
|
## 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).
|
- 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).
|
||||||
|
|
|
||||||
|
|
@ -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).
|
||||||
|
|
@ -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.
|
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
|
## 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).
|
- 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.
|
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.
|
||||||
|
|
|
||||||
|
|
@ -31,3 +31,6 @@ client/addons/gdUnit4/GdUnitRunner.cfg
|
||||||
# Content validation (optional local venv)
|
# Content validation (optional local venv)
|
||||||
.venv-content/
|
.venv-content/
|
||||||
/.venv-gd
|
/.venv-gd
|
||||||
|
|
||||||
|
# VS Code / Cursor — machine-local settings (e.g. Godot editor path)
|
||||||
|
.vscode/settings.json
|
||||||
|
|
|
||||||
|
|
@ -4,9 +4,9 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or
|
||||||
|
|
||||||
| Agent | Rule file | When to use |
|
| 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 |
|
| **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`; use when working in **documents** or @ mention |
|
| **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).
|
**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).
|
||||||
|
|
|
||||||
|
|
@ -33,7 +33,7 @@ Contract readiness is tracked in the [module dependency register](module_depende
|
||||||
|
|
||||||
## Implementation snapshot
|
## 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).
|
- **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).
|
- **Alignment:** [Documentation and implementation alignment](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 |
|
| 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) |
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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>(_ => 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<NpgsqlDataSource>`, 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).
|
||||||
|
|
@ -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<NpgsqlDataSource>(…)`, and extends `InMemoryWebApplicationFactory` so the new hosted service is stripped with the rest of the Postgres path. The implementation matches the written plan and acceptance criteria. A separate commit adds `.cursor/rules/planning-implementation-docs.md` and wires it into kickoff, code-review, docs-review, and `AGENTS.md`; that is coherent and self-consistent.
|
||||||
|
|
||||||
|
Residual risk is low and already acknowledged in the plan: the root `ServiceProvider` may still dispose the same `NpgsqlDataSource` after hosted services stop; the team accepted **Option 2** after green tests. No API or authority contract changes.
|
||||||
|
|
||||||
|
## Documentation checked
|
||||||
|
|
||||||
|
| Document | Result |
|
||||||
|
|----------|--------|
|
||||||
|
| `docs/plans/NEON-15-implementation-plan.md` | **Matches** — Decisions (Option 2, ConfigureAwait), AC checked, technical approach and file lists align with the diff; open questions resolved per [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md). |
|
||||||
|
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **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.
|
||||||
|
|
@ -21,7 +21,8 @@ public sealed class InMemoryWebApplicationFactory : WebApplicationFactory<Progra
|
||||||
if (d.ServiceType == typeof(IPositionStateStore) ||
|
if (d.ServiceType == typeof(IPositionStateStore) ||
|
||||||
d.ServiceType == typeof(NpgsqlDataSource) ||
|
d.ServiceType == typeof(NpgsqlDataSource) ||
|
||||||
(d.ServiceType == typeof(IHostedService) &&
|
(d.ServiceType == typeof(IHostedService) &&
|
||||||
d.ImplementationType == typeof(PostgresDevPlayerSeedHostedService)))
|
(d.ImplementationType == typeof(PostgresDevPlayerSeedHostedService) ||
|
||||||
|
d.ImplementationType == typeof(NpgsqlDataSourceShutdownHostedService))))
|
||||||
{
|
{
|
||||||
services.RemoveAt(i);
|
services.RemoveAt(i);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,16 @@
|
||||||
|
using Microsoft.Extensions.Hosting;
|
||||||
|
using Npgsql;
|
||||||
|
|
||||||
|
namespace NeonSprawl.Server.Game.PositionState;
|
||||||
|
|
||||||
|
/// <summary>Disposes the shared <see cref="NpgsqlDataSource"/> when the host stops. Registered before <see cref="PostgresDevPlayerSeedHostedService"/> so <see cref="IHostedService.StopAsync"/> runs last (NEON-15).</summary>
|
||||||
|
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();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -13,7 +13,8 @@ public static class PositionStateServiceCollectionExtensions
|
||||||
if (!string.IsNullOrWhiteSpace(cs))
|
if (!string.IsNullOrWhiteSpace(cs))
|
||||||
{
|
{
|
||||||
var trimmed = cs.Trim();
|
var trimmed = cs.Trim();
|
||||||
services.AddSingleton(_ => Npgsql.NpgsqlDataSource.Create(trimmed));
|
services.AddSingleton<Npgsql.NpgsqlDataSource>(_ => Npgsql.NpgsqlDataSource.Create(trimmed));
|
||||||
|
services.AddHostedService<NpgsqlDataSourceShutdownHostedService>();
|
||||||
services.AddHostedService<PostgresDevPlayerSeedHostedService>();
|
services.AddHostedService<PostgresDevPlayerSeedHostedService>();
|
||||||
services.AddSingleton<IPositionStateStore, PostgresPositionStateStore>();
|
services.AddSingleton<IPositionStateStore, PostgresPositionStateStore>();
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue