68 lines
6.2 KiB
Markdown
68 lines
6.2 KiB
Markdown
# 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.
|