don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-84: Add code review for combat telemetry hook sites.

pull/119/head
VinPropane 2026-05-25 13:05:08 -04:00
parent 667a1a960a
commit e61ee333ee
1 changed files with 61 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
docs/reviews/2026-05-25-NEO-84.md 100644
View File

@ -0,0 +1,61 @@
# Code review — NEO-84 combat telemetry hook sites
**Date:** 2026-05-25
**Scope:** Branch `NEO-84-combat-telemetry-hooks` · commits `fb2bfe5`–`667a1a9` vs `origin/main`
**Base:** `origin/main`
## Verdict
**Approve with nits**
## Summary
NEO-84 delivers **E5M1-09**: comment-only telemetry hook sites in **`CombatOperations.TryResolve`** for Epic 5 Slice 1 catalog names **`ability_used`** (every successful resolve, including zero-damage abilities) and **`enemy_defeat`** (lethal defeat transition only), plus class-level reserved comments for **`encounter_start`** and **`player_death`**. All active sites carry **`TODO(E9.M1): catalog emit`** with planned payload fields; no runtime behavior, API shape, or test changes. **`server/README.md`**, **`CombatResult.cs`**, **`E5_M1_CombatRulesEngine.md`**, **`E5M1-prototype-backlog.md`**, and **`documentation_and_implementation_alignment.md`** were updated in the implementation commit. Implementation matches the kickoff plan and acceptance checklist. Residual risk is negligible — comments-only server slice; client combat HUD remains **NEO-85** out of scope.
## Documentation checked
| Document | Result |
|----------|--------|
| [`docs/plans/NEO-84-implementation-plan.md`](../plans/NEO-84-implementation-plan.md) | **Matches** — kickoff decisions (engine-only hooks, zero-damage `ability_used`, comments-only, skip manual QA), scope, hook placement table, and acceptance checklist align with code. |
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-09** | **Matches** — AC checked; landed note cites **`TryResolve`** and README anchor. |
| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Matches** — Status and **Related implementation slices** entry for E5M1-09 NEO-84. |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row cites NEO-84 landed + README link. |
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — **E5.M1 note** still ends at NEO-83; does not mention NEO-44 or NEO-84 landed (see Suggestions). |
| [`docs/decomposition/epics/epic_05_pve_combat.md`](../decomposition/epics/epic_05_pve_combat.md) · Slice 1 telemetry vocabulary | **Matches** — hook names align with epic list. |
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative resolve path; no client telemetry. |
| [`server/README.md`](../../server/README.md) | **Matches** — combat telemetry hooks subsection; distinguishes NEO-30 cast funnel from E5 combat-outcome names. |
| Full-stack epic decomposition | **Matches** — plan documents no client counterpart; NEO-85 owns HUD. |
| Manual QA | **N/A** — kickoff skip (NEO-71/NEO-83 pattern). |
## Blocking issues
None.
## Suggestions
1. **Stale `module_dependency_register` E5.M1 note** — The long **E5.M1 note** paragraph still ends at **NEO-83 landed** and omits **NEO-44** (gig XP) and **NEO-84** (telemetry hooks) even though **`E5_M1_CombatRulesEngine.md`** and the alignment register were updated. Append landed one-liners (mirror alignment row) so dependency-register readers stay consistent.
2. **Damage-path `ability_used` payload comment parity** — The zero-damage branch documents that **`playerId`** is out of engine scope and should be correlated at **`AbilityCastApi`** accept; the post-damage **`ability_used`** block lists payload fields but omits that note. Copy the one-line **`playerId`** correlation hint for E9.M1 wiring consistency.
## Nits
- Nit: **`enemy_defeat`** uses an empty `if (after.Defeated) { …comments… }` block — intentional hook anchoring (same pattern as **`gather_node_depleted`** in **`GatherOperations`**); fine as-is.
- Nit: Class-level reserved-site comments sit immediately above the **`TryResolve`** XML summary rather than in a dedicated region after all methods — readable and matches “class-level reserved block” kickoff decision; optional reorder only if a style guide emerges.
- Nit: **`E2.M2 note`** in **`module_dependency_register`** still says **NEO-44 remains backlog** — pre-existing staleness unrelated to this diff; fix when touching register for suggestion 1.
## Verification
```bash
cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
--filter "FullyQualifiedName~CombatOperations"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
--filter "FullyQualifiedName~AbilityCastApiTests"
```
**CombatOperationsTests:** 10/10 passed (local run).
**AbilityCastApiTests:** 21/21 passed (local run).
**Manual (optional):** Confirm hook comment blocks in `server/NeonSprawl.Server/Game/Combat/CombatOperations.cs` cite **`ability_used`** / **`enemy_defeat`** / reserved names, include **`TODO(E9.M1)`**, and that **`enemy_defeat`** sits only after **`TryApplyDamage`** when **`after.Defeated`**. Optional Bruno `ability-cast/` + `combat-targets/` regression — no request/response change expected.