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-29-NEO-96.md

4.0 KiB
Raw Blame History

Code review — NEO-96 (E5M2-10)

Date: 2026-05-29
Scope: Branch NEO-96-e5m2-10-npc-telemetry-hooks vs origin/main — commits dedd9a9 … 5e1e374
Base: origin/main

Verdict

Approve with nits

Summary

NEO-96 replaces NEO-93 telemetry stubs in NpcRuntimeOperations.AdvanceAll with full NEO-84-style comment blocks for Epic 5 Slice 2 catalog events telegraph_fired (at each telegraph_windup entry) and npc_state_transition (on every committed state change, including holder clear → idle). Class summary, server/README.md, E5.M2 module doc, E5M2 backlog E5M2-10, alignment register, and implementation plan are updated. No production ingest, no new tests — appropriate for comment-only scope. NpcRuntimeOperations tests pass (21/21). One mid-cycle holder-clear path adds a defensive TryGet guard beyond pure comments; dev fixture ResetAllPrototypeRows bypasses hook anchors (pre-existing gap, comment wording slightly overstates coverage).

Documentation checked

Path Result
docs/plans/NEO-96-implementation-plan.md Matches — AC checked; kickoff decisions (comments-only, engine-only, telegraph at windup start, every state commit, no manual QA) reflected in code and docs; reconciliation section accurate.
docs/plans/E5M2-prototype-backlog.md (E5M2-10) Matches — AC checked, landed note present; no client counterpart (NEO-97 deferred).
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md Partially matches — Related implementation slices landed line present; Summary Status row still jumps E5M2-09 → E5M2-12 without E5M2-10 / NEO-96 landed (same gap NEO-95 review closed for E5M2-09).
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M2 row includes NEO-96 landed with README/plan links.
docs/decomposition/epics/epic_05_pve_combat.md (Slice 2 telemetry) Matches — hook vocabulary telegraph_fired, npc_state_transition.
docs/decomposition/modules/client_server_authority.md N/A — no authority change; server-only comment anchors.
server/README.md Matches — NPC runtime telemetry hooks subsection with event timing and E9.M1 deferral.

Blocking issues

None.

Suggestions

  1. E5.M2 Summary status table — Append E5M2-10 / NEO-96 telemetry hooks landed to the module Summary Status row (between E5M2-09 and E5M2-12), matching the pattern used when NEO-95 landed.

  2. Fixture reset vs hook comments — Plan transition table and the holder-clear hook comment cite “fixture reset,” but ResetAllPrototypeRows writes idle directly in the store (dev fixture API) without passing through AdvanceOne hook sites. Either add a comment-only anchor on the reset path when E9.M1 lands, or narrow comment/plan wording to “holder cleared during AdvanceAll” so implementers do not assume fixture reset is already instrumented.

Nits

  • Nit: Mid-cycle holder-clear path (inside the while loop) adds TryGet before WriteIdle — slightly exceeds “comments only” AC; snapshot from the top of the same iteration should already be valid, so the guard is redundant unless retained for future emit code that reads fromState from snapshot.

  • Nit: telegraph_fired payload comments list archetypeKind but engine scope has behaviorDefId only — fine for planned fields; E9.M1 implementer will map via behavior catalog (same as snapshot DTO projection).

Verification

# NEO-96-adjacent runtime tests (passed 2026-05-29)
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~NpcRuntimeOperations"

# Full server suite (Postgres persistence tests require local DB)
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

Manual: none required (server-only comment hooks; kickoff decision).