From a578d9b6844950206760a2cba92172cce45eef4a Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 16:00:02 -0400 Subject: [PATCH] NEO-64: address code review follow-ups. Nest gather_node_depleted hook in remainingGathers == 0 guard, add E3M1-08 deny-branch out-of-scope note, and give README NEO-64 its own subsection anchor. --- .../modules/E3_M1_ResourceNodeAndGatherLoop.md | 2 +- docs/manual-qa/NEO-64.md | 4 ++-- docs/plans/E3M1-prototype-backlog.md | 3 ++- docs/plans/NEO-64-implementation-plan.md | 2 +- docs/reviews/2026-05-24-NEO-64.md | 6 +++--- .../Game/Gathering/GatherOperations.cs | 11 +++++++---- server/README.md | 6 ++++-- 7 files changed, 20 insertions(+), 14 deletions(-) diff --git a/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md b/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md index 8516945..5c27c9c 100644 --- a/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md +++ b/docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md @@ -96,7 +96,7 @@ Epic 3 **Slice 2** — [gather nodes and outputs](../epics/epic_03_crafting_econ **Gather engine (NEO-62):** **`GatherOperations.TryGather`** → **`GatherResult`** (`success`, `reasonCode`, `grantsApplied[]`, `remainingGathers`, optional `xpGrantSummary`) — inventory + **`salvage`** XP + depletion commit in one orchestrated path; server-internal until promoted to wire. Plan: [NEO-62 implementation plan](../../plans/NEO-62-implementation-plan.md); [server README — Gather engine (NEO-62)](../../../server/README.md#gather-engine-neo-62). -**Gather telemetry hooks (NEO-64):** comment-only **`resource_gathered`** + **`gather_node_depleted`** hook sites in **`GatherOperations.TryGather`** (`TODO(E9.M1)`; no ingest). **`gather_node_depleted`** fires on success when **`remainingGathers`** reaches **0** — not on pre-gather depletion deny. Plan: [NEO-64 implementation plan](../../plans/NEO-64-implementation-plan.md); [server README — Gather telemetry hooks (NEO-64)](../../../server/README.md#gather-engine-neo-62); manual QA: [`NEO-64`](../../manual-qa/NEO-64.md). +**Gather telemetry hooks (NEO-64):** comment-only **`resource_gathered`** + **`gather_node_depleted`** hook sites in **`GatherOperations.TryGather`** (`TODO(E9.M1)`; no ingest). **`gather_node_depleted`** fires on success when **`remainingGathers`** reaches **0** — not on pre-gather depletion deny. Plan: [NEO-64 implementation plan](../../plans/NEO-64-implementation-plan.md); [server README — Gather telemetry hooks (NEO-64)](../../../server/README.md#gather-telemetry-hooks-neo-64); manual QA: [`NEO-64`](../../manual-qa/NEO-64.md). **Resource node definitions HTTP (NEO-60):** **`GET /game/world/resource-node-definitions`** — versioned read-only projection (`schemaVersion` **1**, **`nodes`**) backed by **`IResourceNodeDefinitionRegistry`**; each row includes nested **`yield`** summary. Bruno `bruno/neon-sprawl-server/resource-node-definitions/`. Plan: [NEO-60 implementation plan](../../plans/NEO-60-implementation-plan.md). diff --git a/docs/manual-qa/NEO-64.md b/docs/manual-qa/NEO-64.md index 1b96d0a..cac0556 100644 --- a/docs/manual-qa/NEO-64.md +++ b/docs/manual-qa/NEO-64.md @@ -11,11 +11,11 @@ Reference: [implementation plan](../plans/NEO-64-implementation-plan.md), [NEO-6 - [ ] Open `server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs`. - [ ] Confirm **`resource_gathered`** comment block on the successful gather path (after depletion commit, before **`GatherResult`** return). -- [ ] Confirm **`gather_node_depleted`** comment block notes emit when **`remainingAfterGather == 0`** (not on pre-gather **`node_depleted`** deny). +- [ ] Confirm **`gather_node_depleted`** comment block is inside `if (remainingAfterGather == 0)` (not on pre-gather **`node_depleted`** deny). - [ ] Confirm **`TODO(E9.M1)`** markers and planned payload fields; **no** new `ILogger` or metrics calls. - [ ] Confirm **no** duplicate hook comments in `InteractionApi.cs`. - [ ] Open `GatherResult.cs` — summary cross-references NEO-64 hook anchor in **`TryGather`**. -- [ ] Open [server README — Gather engine (NEO-62)](../../server/README.md#gather-engine-neo-62) — NEO-64 telemetry subsection present. +- [ ] Open [server README — Gather telemetry hooks (NEO-64)](../../server/README.md#gather-telemetry-hooks-neo-64) — NEO-64 subsection present. ## Regression (optional) diff --git a/docs/plans/E3M1-prototype-backlog.md b/docs/plans/E3M1-prototype-backlog.md index 053af4d..16eda22 100644 --- a/docs/plans/E3M1-prototype-backlog.md +++ b/docs/plans/E3M1-prototype-backlog.md @@ -234,13 +234,14 @@ Working backlog for **Epic 3 — Slice 2** ([gather nodes and outputs](../decomp **In scope** -- Hook placement in gather engine (and interact deny branch if applicable). +- Hook placement in gather engine (and interact deny branch if applicable — **out of scope per NEO-64 kickoff:** engine-only; no **`InteractionApi`** hooks). - `TODO(E9.M1)` comments; no production logging. - README + module doc pointer. **Out of scope** - Telemetry ingest, dashboards. +- Interact deny-branch hook sites (**NEO-64 kickoff** — pre-gather **`node_depleted`** deny uses interact **`reasonCode`** only). **Acceptance criteria** diff --git a/docs/plans/NEO-64-implementation-plan.md b/docs/plans/NEO-64-implementation-plan.md index bdd7710..41dcf87 100644 --- a/docs/plans/NEO-64-implementation-plan.md +++ b/docs/plans/NEO-64-implementation-plan.md @@ -53,7 +53,7 @@ - **`TODO(E9.M1): catalog emit`** (NEO-49 / NEO-56 style). - Planned payload fields: `playerId`, `interactableId` (node instance key), `itemId`, `quantity` (from **`GatherGrantApplied`**), `skillId` + `xpAmount` + `sourceKind` (from **`GatherXpGrantSummary`**), `remainingGathers` after commit. -3. **Hook site B — `gather_node_depleted`:** On the same success path, when **`depletionOutcome.Snapshot.RemainingGathers == 0`** (node exhausted by this gather): +3. **Hook site B — `gather_node_depleted`:** On the same success path, inside `if (remainingAfterGather == 0)` after depletion commit (NEO-40 `level_up` conditional pattern): - Comment block names future E9.M1 event **`gather_node_depleted`**. - **`TODO(E9.M1): catalog emit`** — emit **once** when capacity transitions to zero via successful gather (not on pre-gather **`node_depleted`** deny). - Planned payload fields: `interactableId`, optional `playerId` (last gatherer), `maxGathers` from definition if needed at E9.M1 wiring time. diff --git a/docs/reviews/2026-05-24-NEO-64.md b/docs/reviews/2026-05-24-NEO-64.md index 74fdb99..c5f3c29 100644 --- a/docs/reviews/2026-05-24-NEO-64.md +++ b/docs/reviews/2026-05-24-NEO-64.md @@ -34,13 +34,13 @@ None. ## Suggestions -1. **Conditional structure for `gather_node_depleted` (optional)** — NEO-40 nests the **`level_up`** hook inside `if (nextLevel > prevLevel)`. Here **`gather_node_depleted`** documents “when **`remainingAfterGather == 0`**” but sits on the unconditional success path. Consider a brief commented guard (e.g. `// if (remainingAfterGather == 0) { … emit gather_node_depleted … }`) so E9.M1 wiring mirrors the **`level_up`** pattern and avoids accidental unconditional emit. Low priority — comment text is already explicit. +1. ~~**Conditional structure for `gather_node_depleted` (optional)** — NEO-40 nests the **`level_up`** hook inside `if (nextLevel > prevLevel)`. Here **`gather_node_depleted`** documents “when **`remainingAfterGather == 0`**” but sits on the unconditional success path. Consider a brief commented guard (e.g. `// if (remainingAfterGather == 0) { … emit gather_node_depleted … }`) so E9.M1 wiring mirrors the **`level_up`** pattern and avoids accidental unconditional emit. Low priority — comment text is already explicit.~~ **Done.** — **`gather_node_depleted`** hook comments nested in `if (remainingAfterGather == 0)`. -2. **Backlog template wording (optional)** — E3M1-08 in [`E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) still lists “interact deny branch if applicable” in **In scope**; kickoff chose engine-only. A one-line “deny branch: out of scope (NEO-64 kickoff)” under E3M1-08 would reduce future misreads. +2. ~~**Backlog template wording (optional)** — E3M1-08 in [`E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) still lists “interact deny branch if applicable” in **In scope**; kickoff chose engine-only. A one-line “deny branch: out of scope (NEO-64 kickoff)” under E3M1-08 would reduce future misreads.~~ **Done.** ## Nits -- Nit: README NEO-64 content is a bold paragraph under **Gather engine (NEO-62)** rather than a separate `### Gather telemetry hooks (NEO-64)` heading with its own anchor; module doc links to `#gather-engine-neo-62` — acceptable, optional heading if you want parity with NEO-49’s dedicated README subsection. +- ~~Nit: README NEO-64 content is a bold paragraph under **Gather engine (NEO-62)** rather than a separate `### Gather telemetry hooks (NEO-64)` heading with its own anchor; module doc links to `#gather-engine-neo-62` — acceptable, optional heading if you want parity with NEO-49’s dedicated README subsection.~~ **Done.** — dedicated `### Gather telemetry hooks (NEO-64)` subsection; module doc link updated. - Nit: **`gather_node_depleted`** comment block could repeat **`TODO(E9.M1): catalog emit`** on its own line (it does — same style as **`resource_gathered`**). No change needed. diff --git a/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs b/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs index 1e17814..8605bf9 100644 --- a/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs +++ b/server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs @@ -120,10 +120,13 @@ public static class GatherOperations // skillId, xpAmount, sourceKind (GatherXpGrantSummary), remainingGathers after commit. // No ingest or ILogger here (comments-only). InteractionApi delegates here (no duplicate hooks). - // --- Telemetry hook site (NEO-64): future E9.M1 catalog event `gather_node_depleted` --- - // TODO(E9.M1): catalog emit — when remainingAfterGather == 0 after this successful gather - // (not on pre-gather node_depleted deny). Planned payload: interactableId, optional playerId - // (last gatherer), maxGathers from definition when wiring E9.M1. + if (remainingAfterGather == 0) + { + // --- Telemetry hook site (NEO-64): future E9.M1 catalog event `gather_node_depleted` --- + // TODO(E9.M1): catalog emit — once when this successful gather drives remainingGathers to 0 + // (not on pre-gather node_depleted deny). Planned payload: interactableId, optional playerId + // (last gatherer), maxGathers from definition when wiring E9.M1. + } return new GatherResult( Success: true, diff --git a/server/README.md b/server/README.md index a384bc7..54cd76e 100644 --- a/server/README.md +++ b/server/README.md @@ -97,10 +97,12 @@ World-wide **remaining gathers** per **`interactableId`** live in **`Game/Gather | **`progression_store_missing`** | Skill progression store could not apply XP (compensating inventory remove). | | **`inventory_store_missing`** | Player inventory store could not write. | -**NEO-64 telemetry hooks:** comment-only hook sites in **`GatherOperations.TryGather`** for future E9.M1 catalog events — **`resource_gathered`** on every successful gather (inventory + XP + depletion committed); **`gather_node_depleted`** when that success drives **`remainingGathers`** to **0** (not on pre-gather **`node_depleted`** deny). **`TODO(E9.M1)`** — no production ingest. Successful gathers also pass through **`PlayerInventoryOperations`** (**`item_created`** hook, [NEO-56](#player-inventory-neo-54-store-neo-55-http)). HTTP **`POST …/interact`** calls the engine only (no duplicate API-layer hooks). Manual QA: [`docs/manual-qa/NEO-64.md`](../../docs/manual-qa/NEO-64.md); plan: [NEO-64 implementation plan](../../docs/plans/NEO-64-implementation-plan.md). - **Interact wiring** (replace NEO-41 inline XP, map denies to **`InteractionResponse`**) is **[NEO-63](#gather-via-interact-neo-63)**; plan: [NEO-63 implementation plan](../../docs/plans/NEO-63-implementation-plan.md). +### Gather telemetry hooks (NEO-64) + +Comment-only hook sites in **`GatherOperations.TryGather`** for future E9.M1 catalog events — **`resource_gathered`** on every successful gather (inventory + XP + depletion committed); **`gather_node_depleted`** when that success drives **`remainingGathers`** to **0** (not on pre-gather **`node_depleted`** deny). **`TODO(E9.M1)`** — no production ingest. Successful gathers also pass through **`PlayerInventoryOperations`** (**`item_created`** hook, [NEO-56](#player-inventory-neo-54-store-neo-55-http)). HTTP **`POST …/interact`** calls the engine only (no duplicate API-layer hooks). Manual QA: [`docs/manual-qa/NEO-64.md`](../../docs/manual-qa/NEO-64.md); plan: [NEO-64 implementation plan](../../docs/plans/NEO-64-implementation-plan.md). + ## Item definitions (NEO-53) **`GET /game/world/item-definitions`** returns a versioned JSON body (`schemaVersion` **1**, **`items`**) backed by **`IItemDefinitionRegistry`** — the same prototype rows loaded at startup (no second source of truth). Plan: [NEO-53 implementation plan](../../docs/plans/NEO-53-implementation-plan.md); manual QA: [`docs/manual-qa/NEO-53.md`](../../docs/manual-qa/NEO-53.md); Bruno: `bruno/neon-sprawl-server/item-definitions/`.