68 lines
6.1 KiB
Markdown
68 lines
6.1 KiB
Markdown
# 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`](../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`](../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`](../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`](../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`](../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`](../decomposition/modules/client_server_authority.md) | **N/A** — server-only internal ops; no client or HTTP changes. |
|
||
| [`docs/decomposition/modules/pvp_combat_integration.md`](../decomposition/modules/pvp_combat_integration.md) | **N/A** — prototype NPC targets only; PvP gates out of scope per plan. |
|
||
| [`server/README.md`](../../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](../plans/NEO-81-implementation-plan.md) (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
|
||
|
||
```bash
|
||
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.
|