From bc1357cf94d283080bf28709fa029e22f9e95fa2 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 30 May 2026 18:01:52 -0400 Subject: [PATCH] NEO-98: add follow-up code review for capstone integration fixes --- docs/reviews/2026-05-30-NEO-98-followup.md | 67 ++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 docs/reviews/2026-05-30-NEO-98-followup.md diff --git a/docs/reviews/2026-05-30-NEO-98-followup.md b/docs/reviews/2026-05-30-NEO-98-followup.md new file mode 100644 index 0000000..cad42f6 --- /dev/null +++ b/docs/reviews/2026-05-30-NEO-98-followup.md @@ -0,0 +1,67 @@ +# Code review — NEO-98 follow-up (capstone QA integration fixes) + +**Date:** 2026-05-30 +**Scope:** Branch `NEO-98-e5m2-12-playable-npc-telegraph-combat-capstone` vs `origin/main` — commits `b03128a` … `44fb4ad` (delta since [2026-05-30-NEO-98.md](2026-05-30-NEO-98.md) at `817ed26`) +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +Since the first review, NEO-98 expanded beyond docs-only into **capstone-driven server integration**: defeated NPCs no longer keep attacking (`TryStopOnTargetDefeat` on cast accept + defeated guard in `AdvanceOne`), and telegraph damage is **range-gated** via new NPC attack abilities (`content/abilities/prototype_npc_abilities.json`, behavior **`attackAbilityId`**, `NpcAttackOperations` horizontal **`maxRange`** check). Content/schema/CI gates cross-reference behavior **`attackDamage`** against ability **`baseDamage`**. Manual QA and README now document melee **2 m** strike range and the whiff case. First-review doc nits (Esc regression, backlog AC parity, label format) are addressed. Server tests for defeat-stop, out-of-range whiff, and cast integration pass locally (**41** NpcRuntime/NpcAttack/stop-combat cases; **500** total in-memory tests). Residual risk is **documentation drift** on the E5.M2 module page (still describes NEO-95 resolve via flat **`attackDamage`**, freeze box omits **`attackAbilityId`** / strike ranges) and the plan **Out of scope** line that still forbids catalog changes. + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-98-implementation-plan.md` | **Partially matches** — reconciliation documents defeat-stop fix; Files to modify lists server touch points; **Out of scope** still says “no behavior catalog changes” while diff adds NPC attack abilities + **`attackAbilityId`** (scope expansion not reflected in kickoff/out-of-scope). | +| `docs/manual-qa/NEO-98.md` | **Matches** — strike-range column, step 8 **2 m** proximity + whiff note, Esc regression (steps 19–21), full **`CombatTargetHpLabel`** format on step 4. | +| `client/README.md` | **Partially matches** — flow table uses full target HP label; melee hit row omits **stay within 2 m** note present in manual QA step 8. | +| `docs/plans/E5M2-prototype-backlog.md` (E5M2-12) | **Matches** — fourth AC (three-archetype defeat) present; landed note intact. | +| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Conflicts** — NEO-95 bullet still says telegraph resolve applies catalog **`attackDamage`**; freeze table has no **`attackAbilityId`** or strike **`maxRange`**; NEO-98 capstone bullet is docs-only (no server integration note). **`server/README.md`** threat/runtime sections are authoritative for NEO-98 behavior changes. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E5.M2 **Ready** + NEO-98 capstone note; row does not mention defeat-clear or attack-ability range binding landed in NEO-98. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M2 note unchanged from docs batch; still cites NEO-98 capstone manual QA. | +| `server/README.md` | **Matches** — threat table documents **Attack range**, **Defeat clear**, and **`attackAbilityId`** resolve path. | +| `docs/manual-qa/NEO-97.md` | **Matches** — component regression still valid (elite at origin within **4 m** slam); no update required for capstone range work. | +| Full-stack epic decomposition | **Matches** — Godot capstone manual QA remains the prototype-complete proof; server fixes support playable QA without Bruno. | + +## Blocking issues + +None. + +## Suggestions + +1. **E5.M2 module doc — NEO-95 / NEO-98 server behavior** — Update the **Session player combat HP (NEO-95)** bullet and prototype freeze table to document **`attackAbilityId`**, NPC attack ability ids, and strike **`maxRange`** values (**2 / 10 / 4 m**). Note **`attackDamage`** on behavior rows is a projection/cross-ref field, not the resolve source after NEO-98. + +2. **Plan scope reconciliation** — Revise **Out of scope** (or add a **Decisions** row) to record capstone QA drove catalog + server changes: `prototype_npc_abilities.json`, behavior **`attackAbilityId`**, defeat-stop, range-gated resolve. Keeps kickoff “docs-primary” honest while documenting the adopted integration path. + +3. **`documentation_and_implementation_alignment.md` E5.M2 row** — Append a short **NEO-98 server integration** clause (defeat clears aggro/runtime; telegraph damage uses attack ability **`maxRange`**) alongside the existing capstone manual QA note. + +4. **`client/README.md` melee hit row** — Mirror manual QA step 8: tester must stay **within ~2 m** of the melee marker for **85/100**; backing off during windup whiffs. + +## Nits + +- Nit: Plan §1 melee spine prose still collapses telegraph + HP into a single **≥ 4.5 s** wait; manual QA correctly splits **≥ 3 s** (telegraph) and **≥ 4.5 s** (damage) — tighten plan technical approach for consistency. + +- Nit: `AdvanceAll` now requires **`IAbilityDefinitionRegistry`** + **`ICombatEntityHealthStore`** — any future caller besides `NpcRuntimeSnapshotWorldApi` must pass the expanded signature (only one production caller today). + +- Nit: Whiff path advances NPC state to **`recover`** without damage — correct per server README; optional manual QA note for Ranged/Elite if testers back off during windup (not required for capstone AC). + +## Verification + +```bash +# NEO-98 server spine (in-memory; no Docker) +cd server/NeonSprawl.Server.Tests && dotnet test \ + --filter "FullyQualifiedName~NpcRuntime|FullyQualifiedName~NpcAttack|FullyQualifiedName~PostAbilityCast_ShouldStopNpcCombat" + +# Content cross-ref gates +python3 scripts/validate_content.py +``` + +**Local run:** 41/41 filtered tests passed; 500/511 full suite passed (11 Postgres/docker integration failures — environment infra, unrelated to this diff). + +**Manual (required before merge):** [`docs/manual-qa/NEO-98.md`](../manual-qa/NEO-98.md) — confirm step 8 (**85/100** only when within **2 m**), defeated melee does **not** keep damaging player after step 9, three-archetype defeat, Esc + NEO-28 deny (steps 19–20). + +**Optional:** [`docs/manual-qa/NEO-97.md`](../manual-qa/NEO-97.md) incoming-threat row regression.