From 62ff21083017c2ff5cf9ab4df80f4ec0472a0646 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 21:49:56 -0400 Subject: [PATCH] NEON-15: log plan decisions; require docs sync for planning/implementation --- .cursor/rules/code-review-agent.md | 2 +- .cursor/rules/docs-review-agent.md | 2 +- .cursor/rules/planning-implementation-docs.md | 29 +++++++++++++++++++ .cursor/rules/story-kickoff.md | 3 ++ AGENTS.md | 2 +- docs/plans/NEON-15-implementation-plan.md | 27 ++++++++++------- 6 files changed, 52 insertions(+), 13 deletions(-) create mode 100644 .cursor/rules/planning-implementation-docs.md diff --git a/.cursor/rules/code-review-agent.md b/.cursor/rules/code-review-agent.md index 834e216..ef9f217 100644 --- a/.cursor/rules/code-review-agent.md +++ b/.cursor/rules/code-review-agent.md @@ -29,7 +29,7 @@ Align recommendations with repo rules and docs, including: For every review, **identify and cite** the documentation that defines intent for the change, then **check the implementation against it** (not only style and correctness). -1. **Story / implementation plans** — If the work maps to a ticket or plan under `docs/plans/` (e.g. `NEON-*-implementation-plan.md`), treat that document as acceptance criteria. If the diff diverges without an updated plan or an explicit “out of scope” note, call it out (blocking or non-blocking by severity). +1. **Story / implementation plans** — If the work maps to a ticket or plan under `docs/plans/` (e.g. `NEON-*-implementation-plan.md`), treat that document as acceptance criteria. If the diff diverges without an updated plan or an explicit “out of scope” note, call it out (blocking or non-blocking by severity). Material **planning or implementation decisions** (options chosen, risks closed) should appear in that plan or related `docs/` per [planning-implementation-docs](planning-implementation-docs.md); if they exist only in PR/chat, note as a **should fix**. 2. **Module docs** — Map the change to one or more modules in [`docs/decomposition/modules/module_dependency_register.md`](../../docs/decomposition/modules/module_dependency_register.md) and the corresponding `docs/decomposition/modules/E*_*.md` pages. Verify behavior matches **Purpose**, **Responsibilities**, **Key contracts**, and **Authority** / linked policy sections where relevant. 3. **Cross-cutting policies** — When applicable, align with `docs/decomposition/modules/` policy docs (e.g. [`contracts.md`](../../docs/decomposition/modules/contracts.md), [`client_server_authority.md`](../../docs/decomposition/modules/client_server_authority.md), [`pvp_combat_integration.md`](../../docs/decomposition/modules/pvp_combat_integration.md), [`data_and_ops_policy.md`](../../docs/decomposition/modules/data_and_ops_policy.md)). 4. **Implementation status** — If the register or [`documentation_and_implementation_alignment.md`](../../docs/decomposition/modules/documentation_and_implementation_alignment.md) tracks the module, note whether **Status** / the implementation tracking table should be updated after this merge. diff --git a/.cursor/rules/docs-review-agent.md b/.cursor/rules/docs-review-agent.md index 81e8439..2d810b7 100644 --- a/.cursor/rules/docs-review-agent.md +++ b/.cursor/rules/docs-review-agent.md @@ -28,7 +28,7 @@ Use this rule when the user is working in **`docs/`** (design, decomposition, pl 1. **Same folder and upstream links** — Read every `docs/...` link from the target doc(s); verify paths exist and headings match intent. 2. **Hybrid progression** — `docs/game-design/progression.md`, `skills.md`, `gigs.md`, `abilities.md`, `items.md`, `gathering.md`, `crafting.md`, `economy.md`, `death-loss-recovery.md`, `risk-security-bands.md`, `zones.md`: **Gig** = combat role; **Skill** = non-combat; **combat abilities** = gig kit (+ item channels per data); **Seams** live in `skills.md` unless a dedicated doc supersedes. **Combat encounters** → **gig XP** only (no default skill XP from the fight loop). **Gather / craft** → **skill XP** (see `gathering.md`, `crafting.md`, Epic 3)—not **gig** XP. **Mission/quest rewards** may grant **skill XP**, **currency**, or **items** when explicitly defined (`progression.md`, `economy.md`)—separate from the encounter **gig XP** path. **Economy** faucets/sinks and trade (`economy.md`) do not re-gate **craft** on **gig** (see `items.md` / Seams). **Death / loss / recovery** (`death-loss-recovery.md`): server-authoritative combat death; item stakes via **E3.M4** / **E5.M1**; **PvP** loss **E6.M3** vs **PvE** **open**; **cybernetics** vs external **gear** per **Seams**. **Risk / security** (`risk-security-bands.md`): **E4.M4** `SecurityTier` + **E6.M1**/**E6.M2** for **PvP** eligibility and **consent** UX—**not** client-only PvP toggles. **Zones** (`zones.md`): **place** **fiction** and **hooks** on the **same** **E4.M1** graph—**not** a second topology. 3. **Decomposition** — Map claims to `docs/decomposition/epics/` and `docs/decomposition/modules/` where relevant; flag **conflicts** (e.g. “combat skill” vs skills-as-non-combat). **CI** enforces a minimal guard via [`scripts/check_decomposition_language.py`](../../scripts/check_decomposition_language.py) in the **PR gate** (extend the script if a vetted exception is needed). -4. **Plans** — If work maps to `docs/plans/`, check for **acceptance criteria** vs design doc (same spirit as [code-review-agent](code-review-agent.md) plan alignment). +4. **Plans** — If work maps to `docs/plans/`, check for **acceptance criteria** vs design doc (same spirit as [code-review-agent](code-review-agent.md) plan alignment). Flag plans that omit **documented decisions** or **resolved risks** when the conversation or diff shows they were settled—[planning-implementation-docs](planning-implementation-docs.md). ## Review checklist diff --git a/.cursor/rules/planning-implementation-docs.md b/.cursor/rules/planning-implementation-docs.md new file mode 100644 index 0000000..160698e --- /dev/null +++ b/.cursor/rules/planning-implementation-docs.md @@ -0,0 +1,29 @@ +--- +description: Planning and implementation decisions must be written back into docs (plans, README, decomposition) so Jira chat and files stay aligned. +alwaysApply: true +--- + +# Planning and implementation documentation + +Whenever you **decide** something during **story planning** or **implementation** (architecture pick, scope cut, “option A vs B”, test strategy, resolved risk), **reflect it in documentation** in the same pass or before the story is considered done—do not leave decisions only in chat. + +## Where to write + +| Situation | Update | +|-----------|--------| +| Ticketed story work | **`docs/plans/{JIRA_KEY}-implementation-plan.md`** — add or edit a **Decisions** subsection, **resolve** former open questions, refresh **Technical approach** / **Acceptance criteria** checkboxes, and **Files to add/modify** if reality diverged from kickoff. | +| Cross-cutting or module contract | Relevant **`docs/decomposition/modules/*.md`**, **`docs/game-design/`**, or **`README`** / **`server/README.md`** / **`client/README.md`** when the decision affects how others integrate. | +| Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. | + +## What to capture + +- **What** was chosen (or rejected) and **why** in one short paragraph or bullets. +- **Outcomes** of verification (e.g. “`dotnet test` N passed; double-dispose safe on Npgsql 10”). +- If the user **explicitly** chose an option (e.g. “Option 2”), record that label so future readers do not re-litigate. + +## Agent behavior + +- After **kickoff**, the plan is living: **amend it** when implementation choices differ from the draft. +- Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers. + +This complements [story-kickoff](story-kickoff.md) (plan creation) and [code-review-agent](code-review-agent.md) (documentation checked vs plans and decomposition). diff --git a/.cursor/rules/story-kickoff.md b/.cursor/rules/story-kickoff.md index 5b64f92..33677e3 100644 --- a/.cursor/rules/story-kickoff.md +++ b/.cursor/rules/story-kickoff.md @@ -52,8 +52,11 @@ When the user starts work on a **Jira story** (e.g. issue key `NEON-2`, phrases These three lists (**files to add**, **files to modify**, **tests**) must **always** be generated during kickoff—they are not optional prose and must not be left implicit inside “Technical approach” only. +- The plan is **living documentation**: any **decisions** during planning chat or implementation (options A/B, resolved risks, scope changes) must be **reflected in** `docs/plans/{KEY}-implementation-plan.md` (and other `docs/` when the decision affects integration). See [planning-implementation-docs](planning-implementation-docs.md). + ## 5. After the plan - Implement only after the user confirms. All implementation commits stay on the **same story branch** from step 1b; follow [git workflow](git-workflow.md). +- Before handoff or merge, **reconcile the plan** with what shipped (acceptance checkboxes, **Decisions**, open questions) per [planning-implementation-docs](planning-implementation-docs.md). When the story is **done** and merged to `main`, follow [story-end](story-end.md) to return to `main`, pull, and remove the local story branch. diff --git a/AGENTS.md b/AGENTS.md index f9438d4..a442721 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,6 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; short chat pointer | | **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; use when working in **documents** or @ mention | -Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). +Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **PR / push text:** no “Made-with: Cursor” boilerplate (same file). **Commits tied to a Jira issue** must start the subject with the **issue key** and a colon (e.g. `NEON-5: …`). Branch naming and exceptions (`chore:` when there is no ticket) — [`.cursor/rules/jira-git-naming.md`](.cursor/rules/jira-git-naming.md). diff --git a/docs/plans/NEON-15-implementation-plan.md b/docs/plans/NEON-15-implementation-plan.md index 7fb7266..949bc22 100644 --- a/docs/plans/NEON-15-implementation-plan.md +++ b/docs/plans/NEON-15-implementation-plan.md @@ -25,18 +25,25 @@ - Connection pooling tuning, migration tooling, or schema changes. - Client / Godot code. +## Decisions (planning + implementation) + +| Decision | Choice | +|----------|--------| +| **Lifecycle / double dispose** | **Option 2** — internal shutdown `IHostedService` calls `DisposeAsync` on the shared `NpgsqlDataSource`; no non-disposable holder. Rationale: smallest change; **Npgsql 10** + full `dotnet test` showed clean teardown (no failures from a second dispose path). | +| **ConfigureAwait** | Not used in `StopAsync` — matches existing server hosted-service style (`PostgresDevPlayerSeedHostedService`). | + ## Acceptance criteria checklist -- [ ] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern). -- [ ] `dotnet test` still passes (in-memory and Postgres integration paths). -- [ ] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version. +- [x] Graceful shutdown disposes the shared `NpgsqlDataSource` (or a documented alternative if upstream guidance prefers another pattern). +- [x] `dotnet test` still passes (in-memory and Postgres integration paths). +- [x] No new analyzer warnings; align with Npgsql / ASP.NET Core guidance for the installed package version. ## Technical approach -1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register a small internal **`IHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. -2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()` (with `ConfigureAwait(false)` if the file’s style uses it elsewhere). `StartAsync` is a no-op. -3. **Double disposal:** The root `ServiceProvider` may also dispose `IDisposable`/`IAsyncDisposable` singletons when the host tears down. **Verify** whether Npgsql 10’s `NpgsqlDataSource` tolerates a second dispose; if not, fall back to a **non-disposable holder** singleton that owns the `NpgsqlDataSource` and is the **only** component that disposes it (inject the holder or an abstraction into `PostgresPositionStateStore` and `PostgresDevPlayerSeedHostedService`). Prefer the minimal hosted-service-only change if tests and a local run show clean shutdown. -4. **Registration clarity:** Use an explicit `AddSingleton(…)` (or `NpgsqlDataSourceBuilder` if that matches Npgsql 10 samples) so the service type is obvious to DI and analyzers. +1. **Shutdown ordering:** `IHostedService.StopAsync` runs in **reverse registration order**. Register **`NpgsqlDataSourceShutdownHostedService` before** `PostgresDevPlayerSeedHostedService` so the dispose hook runs **last** among hosted services (after the seed service’s `StopAsync`), while ordinary request handling has already wound down per host shutdown semantics. +2. **Disposal:** In that hosted service’s `StopAsync`, `await dataSource.DisposeAsync()`. `StartAsync` is a no-op. +3. **Double disposal:** The root `ServiceProvider` may still dispose the same singleton during host teardown. **Settled:** Option 2 kept; **verification** — `dotnet test` on solution (**36** tests passed) including in-memory and Postgres factory dispose paths; no change required to store/seed constructors. +4. **Registration clarity:** `AddSingleton(_ => NpgsqlDataSource.Create(trimmed))` so the service type is explicit to DI and analyzers. 5. **Manual spot-check (optional):** Run the server with a real connection string, stop with Ctrl+C, confirm no faulting shutdown logs related to the pool (document result in PR if anything surprising). ## Files to add @@ -51,8 +58,8 @@ |------|-----------| | `server/NeonSprawl.Server/Game/PositionState/PositionStateServiceCollectionExtensions.cs` | Register `NpgsqlDataSource` with an explicit service type if adjusted; register the shutdown hosted service **before** `PostgresDevPlayerSeedHostedService`; keep the existing branch for in-memory vs Postgres. | | `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Extend the descriptor removal loop to drop the new shutdown hosted service when forcing the in-memory store (mirror the `PostgresDevPlayerSeedHostedService` pattern). | -| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **Only if** the holder / non-auto-dispose pattern is required — inject the new type instead of raw `NpgsqlDataSource`. | -| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **Only if** the holder pattern is required — same as store. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresPositionStateStore.cs` | **N/A (shipped)** — holder pattern not needed after Option 2 verification. | +| `server/NeonSprawl.Server/Game/PositionState/PostgresDevPlayerSeedHostedService.cs` | **N/A (shipped)** — same. | ## Tests @@ -63,7 +70,7 @@ ## Open questions / risks -**Double disposal** when both a shutdown hosted service and the root `ServiceProvider` dispose the same `NpgsqlDataSource` — resolve by confirming Npgsql behavior or switching to the holder pattern. **None** other if that is settled in implementation. +**Resolved:** Double-dispose risk accepted under **Option 2**; **Npgsql 10** + full test run showed no shutdown regressions. **None** outstanding for this story. ## PR / review