don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-25-NEO-82.md

5.8 KiB
Raw Permalink Blame History

Code review — NEO-82 Wire ability-cast into combat engine + cast response

Date: 2026-05-25
Scope: Branch NEO-82-wire-ability-cast-combat-resolution · commits 4126ac9–e8be100 vs origin/main
Base: origin/main

Follow-up: Code review suggestions and nits addressed in follow-up commit on NEO-82-wire-ability-cast-combat-resolution. Bugbot + CI fixes in 52bf9fc (definition-before-resolve; Bruno beta/alpha isolation).

Verdict

Approve with nits

Summary

NEO-82 delivers E5M1-07: after E1.M4 cast gates pass, AbilityCastApi invokes CombatOperations.TryResolve, returns nested wire combatResolution on accept, maps combat denies (notably target_defeated) to JSON cast denies without cooldown commit, and replaces the global prototype cooldown with per-ability catalog cooldownSeconds on successful resolve. Handler flow, DTO shape, kickoff decisions (nested block, no cooldown on defeated deny), Bruno happy + defeat spine, and README updates align with the implementation plan. Four new integration tests cover zero-damage accept, four-pulse defeat chain, and guard-vs-pulse catalog cooldown; existing cast tests assert resolution fields. Residual risk is low — server-only HTTP extension with clear deny vocabulary; module tracking docs should sync on merge (same follow-up pattern as NEO-81).

Documentation checked

Document Result
docs/plans/NEO-82-implementation-plan.md Matches — kickoff decisions, scope, acceptance checklist, expected cast table, and reconciliation align with code.
docs/plans/E5M1-prototype-backlog.md · E5M1-07 Matches — AC checked, landed note added.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md Matches — Status cites E5M1-07 NEO-82 landed; Related implementation slices entry added. Partially matches Done.
docs/decomposition/modules/module_dependency_register.md Matches — E5.M1 note includes NEO-82 landed. Partially matches Done.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E5.M1 row cites NEO-82 cast → combatResolution. Partially matches Done.
docs/decomposition/modules/client_server_authority.md Matches — server validates and computes all combat outcomes; client unchanged (NEO-85).
docs/decomposition/modules/pvp_combat_integration.md N/A — prototype NPC targets only.
server/README.md Matches — cast combatResolution table, target_defeated deny, catalog cooldown policy, combat engine cross-link.
Full-stack epic decomposition Matches — plan documents NEO-85 client counterpart; no Godot change in scope.
Manual QA N/A — kickoff skip; Bruno + README sufficient.

Blocking issues

None.

Suggestions

  1. Module doc status + slice entry — In docs/decomposition/modules/E5_M1_CombatRulesEngine.md, update Status to cite E5M1-07 NEO-82 landed (cast POST returns combatResolution), and add E5M1-07 (NEO-82) under Related implementation slices linking the plan and README ability-cast section (mirror NEO-81 landed pattern). Done.

  2. Dependency register + alignment table — Extend the E5.M1 note in module_dependency_register.md and the E5.M1 row in documentation_and_implementation_alignment.md with NEO-82 landed: AbilityCastApi → CombatOperations.TryResolve, nested combatResolution, per-ability catalog cooldown, target_defeated cast deny. Done.

  3. Bruno defeat spine HP stepping — Plan §6 calls for asserting HP 75 → 50 → 25 → 0 on casts 1–4; Post cast lock pulse defeat spine.bru only captures/asserts the fourth cast. Consider storing intermediate cast bodies in pre-request and asserting targetRemainingHp on each pulse for stronger manual lock-in (non-blocking — fifth deny + integration test cover the critical path). Done. — neo82CastResults stores all four casts; tests assert HP stepping.

Nits

  • Nit: PostAbilityCast_ShouldDefeatTargetAfterFourPulses_ThenDenyTargetDefeated runs pulses 1–3 inside Act; move them to Arrange for stricter AAA (same note as NEO-81 four-pulse pattern). Done. — Pulses 1–3 in Arrange; Act is 4th + 5th cast; Assert includes HP stepping.

  • Nit: After successful TryResolve, TryGetDefinition runs again solely for CooldownSeconds — CombatOperations already loaded the definition internally. Acceptable for thin handler clarity; optional future refactor if catalog lookups become hot-path. Deferred — no hot-path concern in prototype. Done. — TryGetDefinition moved before TryResolve so cooldown lookup cannot deny after HP mutation (Bugbot fix).

  • Nit: AbilityPrototypeCooldown.GlobalDuration is [Obsolete] but retained — matches plan; no remaining runtime references in tests or accept path. No action — intentional per plan.

Verification

cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~AbilityCastApiTests|FullyQualifiedName~CooldownSnapshotApiTests"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

AbilityCast + CooldownSnapshot tests: 22/22 passed (local run).

Full suite: 394/394 passed (local run).

Bruno (manual): Run bruno/neon-sprawl-server/ability-cast/ folder — happy cast + defeat spine (seq 23, ~10s pre-request cooldown waits).