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-27-NEO-93.md

4.8 KiB
Raw Permalink Blame History

Code review — NEO-93 (E5M2-07)

Date: 2026-05-27
Scope: Branch NEO-93-npc-behavior-state-machine-lazy-tick vs origin/main — NPC runtime state machine, lazy tick advance, tests, and docs
Base: origin/main

Follow-up: Code review suggestions addressed (backlog in-scope/AC alignment, AAA test layout, single-call multi-phase test, plan test count, windup→recover write simplification).

Verdict

Approve with nits

Summary

This branch delivers the E5M2-07 server spine: INpcRuntimeStateStore, thread-safe in-memory rows, and NpcRuntimeOperations.AdvanceAll driving the prototype idle → aggro → telegraph_windup → attack_execute → recover loop from catalog timings and IThreatStateStore holders. Kickoff decisions are honored — holder-only aggro sync, no HTTP wire, instant attack_execute stub, comment-only telemetry hooks, and dev fixture reset alongside HP + threat. Tests cover the acceptance paths (cooldown, windup, full melee cycle, single-call multi-phase advance, holder clear, archetype spot checks, delta cap) plus store and fixture integration; 22 focused tests passed locally.

Documentation checked

Path Result
docs/plans/NEO-93-implementation-plan.md Matches — store, ops, state machine, fixture, DI, tests, and reconciliation section align with the diff.
docs/plans/E5M2-prototype-backlog.md (E5M2-07) Matches — AC checkboxes (including full-chain), landed note, and engine-only in-scope bullet align with kickoff and implementation.
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md Matches — E5M2-07 landed note and runtime state machine section present.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M2 row includes NEO-93 landed note and plan/README links.
docs/decomposition/modules/client_server_authority.md Matches — server-owned behavior state; no client projection in this slice (NEO-94 deferred).
server/README.md Matches — NPC runtime section documents states, lazy advance, holder sync, fixture reset, and NEO-94 deferral.

Blocking issues

None.

Suggestions

  1. Align E5M2-07 backlog “In scope” bullet — Change “called from snapshot GET handler” to engine + DI only in NEO-93; NEO-94 invokes AdvanceAll from GET /game/world/npc-runtime-snapshot so the backlog matches the adopted kickoff decision and the implementation plan. Done. E5M2-07 in-scope bullet updated.
  2. Add full-chain AC to E5M2-07 backlog — The implementation plan checks “full transition chain reachable”; add the same checkbox to the backlog AC list for traceability with Linear/E5M2-07. Done. Fourth AC checkbox added to E5M2-07 backlog.
  3. AAA layout in new tests — Several NpcRuntimeOperationsTests methods call TryGet in Act (e.g. AdvanceAll_ShouldCompleteMeleeWindupDuration_BeforeAttackExecute performs a mid-Act read and a second Advance). Move verification reads to Assert per csharp-style. Non-blocking but worth fixing before NEO-94 adds more tests on this file. Done. TryGet moved to Assert; windup test split into two facts.
  4. Optional: single-call multi-phase test — Add one test that advances from aggro entry to second telegraph in one AdvanceAll call (e.g. T0 → T0+7.5) to lock the inner while (cursor < windowEnd) multi-step loop independently of sequential polls. Done. AdvanceAll_ShouldRunFullMeleeCycle_ToSecondTelegraph_InSingleAdvanceCall added.

Nits

  • Nit: TelegraphWindup case writes AttackExecute then immediately overwrites with Recover; the intermediate write is never observable — harmless but could be a single Recover write until NEO-95 needs a visible execute phase. Done. Windup completion writes Recover directly with comment for logical attack_execute.
  • Nit: Plan reconciliation cites 17 NEO-93-focused tests; local filter run reports 18 (theory counts as two cases) — trivial count drift. Done. Plan reconciliation updated to 18.
  • Nit: LastAdvancedUtc is an unlocked property on the store (same pattern as typical session singletons); acceptable for prototype poll-driven advance.

Verification

# NEO-93-focused tests (passed 2026-05-27)
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~NpcRuntime|FullyQualifiedName~InMemoryNpcRuntime|FullyQualifiedName~CombatTargetFixtureApiTests"

# Full server suite before merge
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

Manual: no Bruno changes (no HTTP wire). Confirm dev fixture still resets HP + aggro + runtime rows between Bruno combat runs.