From 786513ba56d649f83900c0cf0bc731c31562dc79 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 13:31:18 -0400 Subject: [PATCH] NEO-56: add implementation plan after kickoff clarifications Engine-only comment hooks for item_created and inventory_transfer_denied; kickoff decisions recorded in plan. --- docs/plans/NEO-56-implementation-plan.md | 108 +++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 docs/plans/NEO-56-implementation-plan.md diff --git a/docs/plans/NEO-56-implementation-plan.md b/docs/plans/NEO-56-implementation-plan.md new file mode 100644 index 0000000..3f105b6 --- /dev/null +++ b/docs/plans/NEO-56-implementation-plan.md @@ -0,0 +1,108 @@ +# NEO-56 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-56 | +| **Title** | E3.M3: item_created / transfer-failure telemetry hook sites | +| **Linear** | https://linear.app/neon-sprawl/issue/NEO-56/e3m3-item-created-transfer-failure-telemetry-hook-sites | +| **Module** | [E3.M3 — ItemizationAndInventorySchema](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) · Epic 3 Slice 1 (E3M3-07) | +| **Branch** | `NEO-56-item-created-transfer-failure-telemetry-hooks` | +| **Precursors** | [NEO-54](https://linear.app/neon-sprawl/issue/NEO-54) — `PlayerInventoryOperations` (**Done**); [NEO-55](https://linear.app/neon-sprawl/issue/NEO-55) — inventory GET/POST HTTP (**Done**) | +| **Pattern** | [NEO-49](https://linear.app/neon-sprawl/issue/NEO-49) / [NEO-40](https://linear.app/neon-sprawl/issue/NEO-40) — comment-only engine anchors; [NEO-30](https://linear.app/neon-sprawl/issue/NEO-30) — deny event + `reasonCode` | + +## Kickoff clarifications + +| Topic | Question | Agent recommendation | Answer | +|--------|----------|----------------------|--------| +| **Hook anchor** | Engine only vs engine + HTTP? | **Engine only** — `PlayerInventoryOperations` (NEO-49 single-anchor precedent; NEO-55 deferred hooks here). | **User:** engine only. | +| **Deny event shape** | Per-reason events vs single deny + `reasonCode`? | **`inventory_transfer_denied` + `reasonCode`** (NEO-30 `ability_cast_denied`; epic `inventory_full` maps to a reason code). | **User:** single deny + `reasonCode`. | +| **`item_created` scope** | Add success only vs add + remove? | **Successful add only** (`TryAddStack` → `Applied`). | **User:** add success only. | +| **Runtime behavior** | Comments-only vs dev `ILogger`? | **Comments-only** — `TODO(E9.M1)`; no production logging. | **User:** comments-only. | +| **Manual QA doc** | `docs/manual-qa/NEO-56.md`? | **Yes** — NEO-49 verification pattern. | **User:** yes. | + +## Goal, scope, and out-of-scope + +**Goal:** Document and place **comment-only hook sites** on the server-authoritative inventory mutation path for future E9.M1 catalog events **`item_created`** (successful add) and **`inventory_transfer_denied`** (transfer failures with stable `reasonCode`). Align with Epic 3 Slice 1 telemetry vocabulary ([epic_03 — Slice 1](../decomposition/epics/epic_03_crafting_economy.md)). + +**In scope (from Linear + [E3M3-07](E3M3-prototype-backlog.md#e3m3-07--item_created--transfer-failure-telemetry-hook-sites)):** + +- Hook placement in **`PlayerInventoryOperations`** (`TryAddStack` / `TryRemoveStack`) with **`TODO(E9.M1)`** comments. +- **`server/README.md`** inventory subsection + [E3_M3 module](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) hook note. +- **`docs/manual-qa/NEO-56.md`** — verify comments + no behavior/API change. + +**Out of scope (from Linear):** + +- Telemetry ingest, dashboards (**E9.M1**). +- `ILogger` / metrics / dev-only log lines. +- Duplicate hook comments in **`PlayerInventoryApi`** (HTTP calls engine; NEO-55 plan deferred hooks to NEO-56). +- **`item_created`** on successful **remove** (kickoff decision). +- **`StoreMissing`** hooks (not a transfer failure — HTTP maps to **404**). + +## Acceptance criteria checklist + +- [ ] Hook sites documented in code (`PlayerInventoryOperations`) with **`TODO(E9.M1)`**. +- [ ] **`item_created`** on successful add; **`inventory_transfer_denied`** + `reasonCode` on rule denies (including early validation denies). +- [ ] `server/README.md` documents hook placement. +- [ ] Matches Epic 3 Slice 1 vocabulary (`item_created`, `inventory_full` / transfer failures via `reasonCode`). + +## Technical approach + +1. **Authoritative surface:** `PlayerInventoryOperations` — all inventory add/remove rules and outcomes. Callers (`PlayerInventoryApi` POST, future gather/craft) funnel here; **no** duplicate markers at HTTP. + +2. **Hook site A — `item_created`:** In **`TryAddStack`**, immediately before returning **`PlayerInventoryMutationKind.Applied`** (after successful `TryMutateSnapshot` with no `denyReason`): + - Comment block names future E9.M1 event **`item_created`**. + - **`TODO(E9.M1): catalog emit`** (NEO-49 / NEO-30 style). + - Planned payload fields: `playerId`, `itemId`, `quantity`, optional `mutationKind`/`source` when Slice 2+ callers exist. + - **Not** on **`TryRemoveStack`** success (kickoff). + +3. **Hook site B — `inventory_transfer_denied`:** On every **`Denied`** return path in **`TryAddStack`** and **`TryRemoveStack`**: + - Early **`Deny(...)`** (e.g. `invalid_quantity`, `invalid_item` before store write). + - Post-mutation deny when `denyReason` is set (`inventory_full`, `insufficient_quantity`). + - Single comment pattern: catalog event **`inventory_transfer_denied`** + **`reasonCode`** from [`PlayerInventoryReasonCodes`](../../../server/NeonSprawl.Server/Game/Items/PlayerInventoryReasonCodes.cs) (`inventory_full`, `invalid_item`, `insufficient_quantity`, `invalid_quantity`). + - Epic Slice 1 **`inventory_full`** is the flagship transfer-failure code; others share the same deny event shape (NEO-30 precedent). + +4. **No hook on `StoreMissing`:** Outcome is missing player/store — not an inventory transfer rule deny. + +5. **Cross-references (minimal):** + - One-line pointer in `PlayerInventoryMutationOutcome.cs` or `PlayerInventoryReasonCodes.cs` xml summary → hook sites in `PlayerInventoryOperations`. + - **`server/README.md`:** extend **Player inventory (NEO-54 store, NEO-55 HTTP)** with **NEO-56 telemetry hooks** subsection (anchor file + event names). + - **`E3_M3_ItemizationAndInventorySchema.md`:** **NEO-56 landed** line under Related implementation slices. + - **`documentation_and_implementation_alignment.md`:** E3.M3 row cites NEO-56 hook sites (still no E9.M1 ingest). + +6. **Regression:** Run `dotnet test` on server solution; existing `PlayerInventoryOperationsTests` / `PlayerInventoryApiTests` unchanged (comments-only). + +## Files to add + +| Path | Purpose | +|------|---------| +| `docs/manual-qa/NEO-56.md` | Manual QA: verify hook comment blocks + no API/behavior change. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs` | **NEO-56:** `item_created` on add `Applied`; `inventory_transfer_denied` on all `Denied` paths in add/remove. | +| `server/NeonSprawl.Server/Game/Items/PlayerInventoryReasonCodes.cs` | Cross-reference deny `reasonCode` values → transfer-denied hook anchor (no behavior change). | +| `server/README.md` | Document inventory telemetry hook placement (NEO-56). | +| `docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md` | Implementation snapshot: NEO-56 telemetry hooks. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E3.M3 row: NEO-56 hook sites. | + +## Tests + +| Path | Change | +|------|--------| +| **None required** | Comments-only; existing `PlayerInventoryOperationsTests` and `PlayerInventoryApiTests` cover add/remove/deny paths. | + +**Manual verification:** `docs/manual-qa/NEO-56.md` — confirm **`item_created`** and **`inventory_transfer_denied`** comments, **`TODO(E9.M1)`**, payload field notes, no new log statements; `dotnet test` regression. + +## Open questions / risks + +| Question / risk | Agent recommendation | Status | +|-----------------|----------------------|--------| +| **Future gather/craft callers** | Hooks stay in engine; callers pass `source`/`mutationKind` in E9.M1 payload when catalog exists — document in hook comments as optional fields. | `adopted` | +| **HTTP request correlation** | Defer `requestId` to E9.M1 / future sink; do not add API-layer hooks (kickoff: engine only). | `adopted` | +| **Partial stack placement on add** | NEO-54 all-or-nothing add — single `item_created` emit for full requested quantity on `Applied`. | `adopted` | + +None blocking.