109 lines
7.7 KiB
Markdown
109 lines
7.7 KiB
Markdown
# 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
|
|
|
|
- [x] Hook sites documented in code (`PlayerInventoryOperations`) with **`TODO(E9.M1)`**.
|
|
- [x] **`item_created`** on successful add; **`inventory_transfer_denied`** + `reasonCode` on rule denies (including early validation denies).
|
|
- [x] `server/README.md` documents hook placement.
|
|
- [x] 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.
|