From d1a67a89bd3eb9db2db61fecb1db95fa33130182 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 19:04:55 -0400 Subject: [PATCH] NEO-109: Add code review for encounter telemetry hook sites. --- docs/reviews/2026-05-31-NEO-109.md | 59 ++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 docs/reviews/2026-05-31-NEO-109.md diff --git a/docs/reviews/2026-05-31-NEO-109.md b/docs/reviews/2026-05-31-NEO-109.md new file mode 100644 index 0000000..7e828ea --- /dev/null +++ b/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.