diff --git a/docs/reviews/2026-05-30-NEO-98.md b/docs/reviews/2026-05-30-NEO-98.md new file mode 100644 index 0000000..801b2f2 --- /dev/null +++ b/docs/reviews/2026-05-30-NEO-98.md @@ -0,0 +1,56 @@ +# Code review — NEO-98 (E5M2-12) + +**Date:** 2026-05-30 +**Scope:** Branch `NEO-98-e5m2-12-playable-npc-telegraph-combat-capstone` vs `origin/main` — commits `ceef54c` … `ae8b01d` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-98 closes **E5M2-12** and Epic 5 Slice 2 client acceptance with a **docs-only** capstone: `docs/manual-qa/NEO-98.md` (Melee full telegraph spine → Ranged/Elite defeat + telegraph visibility), `client/README.md` end-to-end loop section, implementation plan with kickoff decisions, and module/backlog/alignment/register updates marking **E5.M2 Ready**. No client or server code changes — consistent with kickoff (“docs + manual QA primary; fix gaps only if QA fails on `main`”) and server test evidence cited in the plan (`NpcRuntimeOperationsTests`, `NpcRuntimeSnapshotWorldApiTests` at 4.5 s / **85** HP). Residual risk is low and operational: a long single-session script (~20 steps, elite ×8 casts) and reliance on human Godot verification before merge; timing math in the checklist matches frozen catalog values and server tests. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-98-implementation-plan.md` | **Matches** — kickoff decisions (docs-primary, fresh restart, Melee→Ranged→Elite, melee full / ranged+elite light, NEO-97 as component regression), scope, file list, and reconciliation align with the diff; no integration fixes shipped. | +| `docs/plans/E5M2-prototype-backlog.md` (E5M2-12) | **Partially matches** — landed note and three AC items checked; plan’s fourth AC (“all three archetypes defeated”) is absent from backlog AC list (see Suggestions). | +| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — Status **Ready** with E5M2-11 + E5M2-12 landed; capstone cross-link + manual QA pointer added. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row **Ready**, **NEO-98 landed**, Slice 2 client capstone complete. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M2 note summarizes NEO-92–NEO-98 and marks Slice 2 complete. | +| `docs/manual-qa/NEO-98.md` | **Matches** — single-session Godot script, archetype table, melee defeat math, zero Bruno/curl in main checklist; acceptance boxes intentionally unchecked pre-human QA (NEO-86 precedent). | +| `client/README.md` | **Matches** — **End-to-end NPC telegraph combat loop (NEO-98)** section after NEO-97; flow table, cross-links, script list, capstone manual QA link. | +| `docs/decomposition/epics/epic_05_pve_combat.md` (Slice 2) | **Matches** — backlog pointer to NEO-98 + Godot verification; no epic doc edit required for this story. | +| Full-stack epic decomposition | **Matches** — client capstone **NEO-98** with Godot manual QA; not Bruno-only prototype proof. | + +## Blocking issues + +None. + +## Suggestions + +1. **Regression step — clear lock before `invalid_target` check** — After step 18 the tester is still locked on defeated **`prototype_npc_elite`**; step 19 (“cast without lock”) will hit **`target_defeated`**, not **`invalid_target`**, unless lock is cleared first. Add an **Esc** step (mirror [NEO-86 manual QA](../manual-qa/NEO-86.md) steps 8–10) before the NEO-28 regression assertion. + +2. **E5M2-12 backlog AC parity** — [NEO-98 implementation plan](../plans/NEO-98-implementation-plan.md) lists a fourth AC (“All three archetypes defeated… **`CombatTargetHpLabel`** shows defeated for each”); [E5M2-prototype-backlog.md](../plans/E5M2-prototype-backlog.md) E5M2-12 section omits it. Add the checkbox for traceability with the plan and manual QA steps 9/14/18. + +## Nits + +- Nit: **E5.M2 Summary `Linear` row** still abbreviates **`E5M2-05 … → E5M2-12`** while the **Status** row now lists E5M2-06–E5M2-11 inline — optional consistency pass. + +- Nit: Manual QA / README use shorthand **`100/100`** on **`CombatTargetHpLabel`**; NEO-85 format includes **`Target HP: {id}`** prefix — testers should match substring, not exact label text (or tighten one example step to full format). + +- Nit: Plan / backlog AC boxes are **`[x]`** while **`docs/manual-qa/NEO-98.md` Acceptance** stays **`[ ]`** until a human run — intentional per NEO-86; keep that distinction when pasting PR text. + +## Verification + +```bash +# Server timing spine (no client required — confirms melee telegraph math cited in plan) +cd server/NeonSprawl.Server.Tests && dotnet test --filter "FullyQualifiedName~NpcRuntime" +``` + +**Manual (required before merge):** [`docs/manual-qa/NEO-98.md`](../manual-qa/NEO-98.md) — fresh server restart, Godot **F5**, Melee steps 3–10 (telegraph visible **before** **85/100**), Ranged + Elite defeat with telegraph visible, regression steps 19–20. + +**Optional component regression:** [`docs/manual-qa/NEO-97.md`](../manual-qa/NEO-97.md) — elite incoming-threat row when locking a different NPC while elite holds aggro.