60 lines
5.2 KiB
Markdown
60 lines
5.2 KiB
Markdown
# 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.~~ **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.~~ **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.~~ **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.~~ **Done (Bugbot PR #148).** Restored early **`return;`** after failure log to match guard-clause pattern in **`TryProcessCastOutcome`**.
|
|
|
|
- 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.
|