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.
pull/98/head
VinPropane 2026-05-24 16:00:02 -04:00
parent 1bcaa1b350
commit a578d9b684
7 changed files with 20 additions and 14 deletions

View File

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

View File

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

View File

@ -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**

View File

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

View File

@ -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-49s 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-49s 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.

View File

@ -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,

View File

@ -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/`.