NEON-15: log plan decisions; require docs sync for planning/implementation

.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.

.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
 

.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).

.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.

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).

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).
+- [x] 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).
+- [x] `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] 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.
+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()` (with `ConfigureAwait(false)` if the file’s style uses it elsewhere). `StartAsync` is a no-op.
+2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()`. `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.
+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:** Use an explicit `AddSingleton<NpgsqlDataSource>(…)` (or `NpgsqlDataSourceBuilder` if that matches Npgsql 10 samples) so the service type is obvious to DI and analyzers.
+4. **Registration clarity:** `AddSingleton<NpgsqlDataSource>(_ => 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/PostgresPositionStateStore.cs` | **N/A (shipped)** — holder pattern not needed after Option 2 verification. |
-| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **Only if** the holder pattern is required — same as store. |
+| `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