62 lines
5.4 KiB
Markdown
62 lines
5.4 KiB
Markdown
# 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) | **Matches** — E5.M1 note includes NEO-44 and NEO-84 landed (addressed post-review). |
|
||
| [`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.~~ **Done.** E5.M1 note appended NEO-44 + NEO-84 landed; E2.M2 note corrected NEO-44 status.
|
||
|
||
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.~~ **Done.** Post-damage `ability_used` block now includes the `playerId` correlation line.
|
||
|
||
## 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.~~ **Done.** Addressed with suggestion 1 register update.
|
||
|
||
## 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.
|