NEO-109: Add code review for encounter telemetry hook sites.

docs/reviews/2026-05-31-NEO-109.md

@@ -0,0 +1,59 @@
+# Code review — NEO-109 (E5M3-10)
+
+**Date:** 2026-05-31  
+**Scope:** Branch `NEO-109-slice-3-encounter-telemetry-hook-sites` vs `4aa7665` (merge-base on `main`) — commits `e6debc2` … `8cda0d7`  
+**Base:** `4aa7665` (main at branch point, post NEO-108 merge)
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-109 lands **comment-only** E9.M1 telemetry hook sites for Epic 5 Slice 3 encounter funnel events: **`encounter_start`**, **`encounter_complete`**, **`reward_attribution`**, plus retained **`encounter_complete_denied`**. Hooks follow the **NEO-64** / **NEO-84** pattern (`TODO(E9.M1)`, planned payload fields, no ingest or `ILogger`). Duplicate **`encounter_complete`** stub in **`EncounterCombatWiring`** is removed in favor of a delegate pointer to **`EncounterCompletionOperations.TryCompleteAndGrant`**; NEO-84 reserved **`encounter_start`** in **`CombatOperations`** now points at **`EncounterProgressOperations`**. Docs (implementation plan, backlog E5M3-10, E5.M3 module snapshot, alignment register, module register, `server/README.md`) are updated consistently. No tests changed — appropriate for comments-only work. Godot client correctly remains **NEO-110** / **NEO-111**. Overall risk is negligible.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-109-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate. |
+| `docs/plans/E5M3-prototype-backlog.md` (E5M3-10) | **Matches** — acceptance criteria checked; landed note cites README + plan. |
+| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — NEO-109 telemetry hooks snapshot bullet added. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-109**; outstanding work updated to **E5M3-11+ client HUD**. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-109** landed; **E5M3-11+** outstanding. |
+| `docs/decomposition/epics/epic_05_pve_combat.md` (Slice 3) | **Matches** — Slice 3 vocabulary **`encounter_complete`** + reward attribution reflected; **`encounter_start`** is Slice 1 reserved hook now anchored per NEO-84 → NEO-109 (plan documents this). |
+| Full-stack epic decomposition | **N/A** — plan explicitly states no client counterpart; server-only comments. |
+| `server/README.md` | **Matches** — new **Encounter telemetry hooks (NEO-109)** section; NEO-106 table and NEO-84 combat hooks cross-link correctly. |
+
+## Blocking issues
+
+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.
+
+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.
+
+## 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: **`EncounterCombatWiring`** removed `return;` after the deny log block — behavior unchanged (method ends immediately after the delegate comment). Harmless; no action needed.
+
+- Nit: **`encounter_complete_denied`** is not in the Epic 5 Slice 3 telemetry list — plan documents the **NEO-56** deny precedent; acceptable.
+
+## Verification
+
+```bash
+# Grep hook consolidation (manual plan check)
+rg 'NEO-109|encounter_complete|reward_attribution|encounter_start' \
+  server/NeonSprawl.Server/Game/Encounters/
+# Confirm EncounterCombatWiring has delegate pointer only, no encounter_complete emit block
+
+# Regression (encounters module)
+cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~Encounters"
+```
+
+**Local run:** 81/82 Encounters-filter tests passed; 1 failure in `RewardTableDefinitionRegistryTests.Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` (host DI at `Program.cs:39`) — appears **environment/pre-existing**, unrelated to NEO-109 comment changes. Re-run full `dotnet test NeonSprawl.sln` on CI before merge.