don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-31-NEO-109.md

5.2 KiB
Raw Blame History

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

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