diff --git a/docs/reviews/2026-05-25-NEO-84.md b/docs/reviews/2026-05-25-NEO-84.md new file mode 100644 index 0000000..ebb1960 --- /dev/null +++ b/docs/reviews/2026-05-25-NEO-84.md @@ -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.