From eed176d2260836c4eb217e8e86566c7733d9e540 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 27 May 2026 22:58:33 -0400 Subject: [PATCH] NEO-93: Add code review for NPC runtime state machine branch. Co-authored-by: Cursor --- docs/reviews/2026-05-27-NEO-93.md | 54 +++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 docs/reviews/2026-05-27-NEO-93.md diff --git a/docs/reviews/2026-05-27-NEO-93.md b/docs/reviews/2026-05-27-NEO-93.md new file mode 100644 index 0000000..5f41f64 --- /dev/null +++ b/docs/reviews/2026-05-27-NEO-93.md @@ -0,0 +1,54 @@ +# 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` + +## 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, holder clear, archetype spot checks, delta cap) plus store and fixture integration; **18** focused tests passed locally. Risk is low for merge; remaining items are doc parity and test-layout polish. + +## 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) | **Partially matches** — AC checkboxes and landed note updated; **In scope** still says advance is “called from snapshot GET handler” (NEO-94 wires that; NEO-93 is engine-only). Plan’s fourth AC (full transition chain) is not mirrored in backlog AC list. | +| `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. +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. +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](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). Non-blocking but worth fixing before NEO-94 adds more tests on this file. +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. + +## 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. +- Nit: Plan reconciliation cites **17** NEO-93-focused tests; local filter run reports **18** (theory counts as two cases) — trivial count drift. +- Nit: `LastAdvancedUtc` is an unlocked property on the store (same pattern as typical session singletons); acceptable for prototype poll-driven advance. + +## Verification + +```bash +# 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.