diff --git a/docs/plans/NEO-109-implementation-plan.md b/docs/plans/NEO-109-implementation-plan.md index b270fcb..0b89457 100644 --- a/docs/plans/NEO-109-implementation-plan.md +++ b/docs/plans/NEO-109-implementation-plan.md @@ -51,7 +51,7 @@ ## Implementation reconciliation (shipped) -- **`encounter_start`:** full comment block in **`EncounterProgressOperations.TryActivateOnFirstEngagement`**; NEO-84 **`CombatOperations`** reserved pointer updated. +- **`encounter_start`:** full comment block in **`EncounterProgressOperations.TryActivateOnFirstEngagement`** after successful **`TryActivate`** (anchor matches once-per-activation emit); NEO-84 **`CombatOperations`** reserved pointer updated. - **`encounter_complete`** + **`reward_attribution`:** full comment blocks in **`EncounterCompletionOperations.TryCompleteAndGrant`** after event record; duplicate removed from **`EncounterCombatWiring`** (delegate pointer only). - **`encounter_complete_denied`:** retained on **`Deny`**. - **Docs:** `server/README.md` encounter telemetry subsection; E5.M3 module snapshot; alignment register; module dependency register; E5M3-10 backlog landed note. diff --git a/docs/reviews/2026-05-31-NEO-109.md b/docs/reviews/2026-05-31-NEO-109.md index 7e828ea..da17681 100644 --- a/docs/reviews/2026-05-31-NEO-109.md +++ b/docs/reviews/2026-05-31-NEO-109.md @@ -31,13 +31,13 @@ None. ## Suggestions -1. **`encounter_start` hook anchor vs emit condition** — The comment block sits **before** `progressStore.TryActivate`, but the TODO correctly says emit **when `TryActivate` returns true**. On re-engagement after activation, execution still passes the comment then `TryActivate` returns `false`. For E9.M1 wiring clarity, consider moving the block to immediately **after** a successful `TryActivate` (or wrapping with `if (progressStore.TryActivate(...)) { /* hook */ return true; }`) so the anchor matches the once-per-activation contract. Comments-only today; non-blocking. +1. ~~**`encounter_start` hook anchor vs emit condition** — The comment block sits **before** `progressStore.TryActivate`, but the TODO correctly says emit **when `TryActivate` returns true**. On re-engagement after activation, execution still passes the comment then `TryActivate` returns `false`. For E9.M1 wiring clarity, consider moving the block to immediately **after** a successful `TryActivate` (or wrapping with `if (progressStore.TryActivate(...)) { /* hook */ return true; }`) so the anchor matches the once-per-activation contract. Comments-only today; non-blocking.~~ **Done.** Hook block moved after successful **`TryActivate`**; method returns **`true`** on first activation (same outward behavior). -2. **`encounter_complete_denied` payload scope** — The **`Deny`** hook comment lists `playerId`, `encounterId`, `reasonCode`, but **`Deny(string reasonCode)`** has no player/encounter context. Pre-existing from NEO-105; E9.M1 will need call-site context or a signature change. Worth a one-line note in the comment (“wire at caller with playerId + encounterId”) to avoid implementer confusion. +2. ~~**`encounter_complete_denied` payload scope** — The **`Deny`** hook comment lists `playerId`, `encounterId`, `reasonCode`, but **`Deny(string reasonCode)`** has no player/encounter context. Pre-existing from NEO-105; E9.M1 will need call-site context or a signature change. Worth a one-line note in the comment (“wire at caller with playerId + encounterId”) to avoid implementer confusion.~~ **Done.** Comment notes wire at each **`TryCompleteAndGrant`** **`Deny()`** call site. ## Nits -- Nit: Plan lists optional **`requiredNpcInstanceIds` count** in **`encounter_start`** payload; the comment block omits it. Fine for prototype; add when E9.M1 shapes the catalog schema. +- ~~Nit: Plan lists optional **`requiredNpcInstanceIds` count** in **`encounter_start`** payload; the comment block omits it. Fine for prototype; add when E9.M1 shapes the catalog schema.~~ **Done.** Payload comment includes **`requiredNpcInstanceIdsCount`**. - Nit: **`EncounterCombatWiring`** removed `return;` after the deny log block — behavior unchanged (method ends immediately after the delegate comment). Harmless; no action needed. diff --git a/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs b/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs index 5e621f4..c725eff 100644 --- a/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs +++ b/server/NeonSprawl.Server/Game/Encounters/EncounterCompletionOperations.cs @@ -178,7 +178,8 @@ public static class EncounterCompletionOperations { // --- Telemetry hook site (NEO-109): future E9.M1 catalog event `encounter_complete_denied` --- // TODO(E9.M1): catalog emit — structured deny with reasonCode (EncounterCompletionReasonCodes). - // Planned payload: playerId, encounterId, reasonCode. No ingest or ILogger here (comments-only). + // Planned payload: playerId, encounterId, reasonCode — wire at each TryCompleteAndGrant Deny() call site + // (this helper has reasonCode only; callers hold playerId + encounterId). No ingest or ILogger here. return new EncounterCompletionResult( Success: false, diff --git a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs index 19707ec..f7e7a5f 100644 --- a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs +++ b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs @@ -53,7 +53,7 @@ public static class EncounterProgressOperations IEncounterProgressStore progressStore, IEncounterCompletionStore completionStore) { - if (!TryResolveEncounterForNpc(npcInstanceId, encounterRegistry, out var encounterId, out _)) + if (!TryResolveEncounterForNpc(npcInstanceId, encounterRegistry, out var encounterId, out var definition)) { return false; } @@ -63,12 +63,18 @@ public static class EncounterProgressOperations return false; } + if (!progressStore.TryActivate(playerId, encounterId)) + { + return false; + } + // --- Telemetry hook site (NEO-109): future E9.M1 catalog event `encounter_start` --- - // TODO(E9.M1): catalog emit — when TryActivate returns true (first activation for player+encounter). - // Planned payload: playerId, encounterId, npcInstanceId (engagement target). No ingest or ILogger here + // TODO(E9.M1): catalog emit — once per player+encounter on first successful TryActivate (anchor matches emit). + // Planned payload: playerId, encounterId, npcInstanceId (engagement target), + // requiredNpcInstanceIdsCount (catalog requiredNpcInstanceIds for encounterId). No ingest or ILogger here // (comments-only). EncounterCombatWiring calls this on first damaging hit vs a required NPC (NEO-106). - return progressStore.TryActivate(playerId, encounterId); + return true; } /// diff --git a/server/README.md b/server/README.md index f189617..019be62 100644 --- a/server/README.md +++ b/server/README.md @@ -212,7 +212,7 @@ Comment-only hook sites for Epic 5 **Slice 3** catalog events ([epic_05 Slice 3] | Event | Anchor | |-------|--------| -| **`encounter_start`** | **`EncounterProgressOperations.TryActivateOnFirstEngagement`** — emit when **`TryActivate`** returns true (first engagement). Activates NEO-84 reserved **`encounter_start`** pointer in **`CombatOperations`**. | +| **`encounter_start`** | **`EncounterProgressOperations.TryActivateOnFirstEngagement`** — hook anchor after successful **`TryActivate`** (first engagement only). Activates NEO-84 reserved **`encounter_start`** pointer in **`CombatOperations`**. | | **`encounter_complete`** | **`EncounterCompletionOperations.TryCompleteAndGrant`** — once per successful grant + completion mark commit. | | **`reward_attribution`** | Same success path — once per completion (batch **`grantedItems`**); per-stack **`item_created`** remains in **`PlayerInventoryOperations`** ([NEO-56](#player-inventory-neo-54-store-neo-55-http)). | | **`encounter_complete_denied`** | **`EncounterCompletionOperations.Deny`** — structured deny + **`EncounterCompletionReasonCodes`**. |