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-81.md

6.1 KiB
Raw Permalink Blame History

Code review — NEO-81 CombatOperations.TryResolve + CombatResult

Date: 2026-05-25
Scope: Branch NEO-81-combat-operations-try-resolve · commits 849c34f–ca15e7a vs origin/main
Base: origin/main

Follow-up: Code-review suggestions and nits addressed in a follow-up commit (doc sync, test AAA/assertions, dash smoke).

Verdict

Approve with nits

Summary

NEO-81 delivers E5M1-06: a static CombatOperations.TryResolve orchestrating IAbilityDefinitionRegistry and ICombatEntityHealthStore into a server-internal CombatResult envelope with stable deny codes (unknown_ability, unknown_target, target_defeated). Implementation matches kickoff decisions: self-contained id validation, defeated pre-check before any damage (including zero-damage utilities), zero-damage success via TryGet only, and target_defeated deny carrying targetRemainingHp = 0 with targetDefeated = false on the envelope. Nine AAA unit/host tests cover pulse/guard/burst paths, four-pulse defeat chain, defeated re-hit deny, unknown ids, and DI resolution. server/README.md documents the combat engine section and NEO-82 handoff. Residual risk is low — server-only ops with no HTTP surface; module tracking docs and minor test/doc backlog sync should close on merge (same pattern as NEO-80 follow-ups).

Documentation checked

Document Result
docs/plans/NEO-81-implementation-plan.md Matches — kickoff decisions, scope, acceptance checklist, reconciliation, and expected resolve table align with code.
docs/plans/E5M1-prototype-backlog.md · E5M1-06 Partially matches — story AC checkboxes still unchecked; no landed note yet (E5M1-05 pattern). Matches — AC checked, landed note added. Done.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md Partially matches — Status still reads “E5M1-06+ pending”; no related implementation slice for NEO-81. Matches — Status + E5M1-06 slice entry updated. Done.
docs/decomposition/modules/module_dependency_register.md Partially matches — E5.M1 note stops at NEO-80; NEO-81 CombatOperations not cited. Matches — NEO-81 landed note added. Done.
docs/decomposition/modules/documentation_and_implementation_alignment.md Partially matches — E5.M1 row should add NEO-81 landed after merge. Matches — NEO-81 row + links updated. Done.
docs/decomposition/modules/client_server_authority.md N/A — server-only internal ops; no client or HTTP changes.
docs/decomposition/modules/pvp_combat_integration.md N/A — prototype NPC targets only; PvP gates out of scope per plan.
server/README.md Matches — combat engine section, reason-code table, CombatResult field semantics, NEO-82 cross-link.
Full-stack epic decomposition Matches — plan documents no client counterpart; cast wire is NEO-82, HUD is NEO-85.
Manual QA N/A — plan skip rationale holds (server-only internal ops).

Blocking issues

None.

Suggestions

  1. Module doc status line — In docs/decomposition/modules/E5_M1_CombatRulesEngine.md, update Status to cite E5M1-06 NEO-81 landed (CombatOperations.TryResolve + CombatResult), and add a Related implementation slices entry linking the NEO-81 plan and README combat engine section (mirror NEO-80 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-81 landed: CombatOperations, CombatResult, CombatReasonCodes, defeated pre-check deny vocabulary. Done.

  3. E5M1 backlog landed note — In docs/plans/E5M1-prototype-backlog.md § E5M1-06, check AC boxes and add a one-line landed note with link to NEO-81 implementation plan (same pattern as E5M1-05). Done.

  4. Defeated deny assertion parity — TryResolve_WithGuardOnDefeatedTarget_ShouldDenyWithTargetDefeated omits TargetRemainingHp / TargetDefeated envelope assertions that the pulse defeated test covers; add Assert.Equal(0, result.TargetRemainingHp) and Assert.False(result.TargetDefeated) for consistency with the plan’s deny payload contract. Done.

Nits

  • Nit: TryResolve_WithFourPulses_ShouldDefeatTargetOnFourthResolve runs four resolves in Act; pulses 1–3 are setup for the fourth outcome — move them to Arrange for stricter AAA (same note as NEO-80 four-pulse pattern). Done.

  • Nit: CreatePrototypeRegistry includes prototype_dash but no dash resolve test; guard covers the zero-damage path — optional dash smoke for catalog completeness. Done — added TryResolve_WithDashOnLiveTarget_ShouldSucceedWithZeroDamage.

  • Nit: TryApplyDamage false branch maps to unknown_target — correct for current store contract (unknown/empty/negative damage), but if store semantics expand (e.g. internal failure), NEO-82 cast wiring may want a distinct ops-level code; defer until cast integration. Deferred — no change in NEO-81.

Verification

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

CombatOperationsTests: 10/10 passed (local run after follow-up).

Full suite: 389 passed, 1 failed — TargetingApiTests.GetTarget_ShouldReturnNone_WhenNoLock (unrelated to this branch; investigate separately if CI is green on main).

Manual: N/A — no HTTP or client surface in this story.