don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-95: address code review — alignment docs and API nits 

Append NEO-95 landed to E5.M2 tracking; echo route playerId on
combat-health GET; AttackExecute comment; tighten elite test AAA.
pull/133/head
VinPropane 2026-05-28 23:45:07 -04:00
parent 3afe720980
commit 5efa5a4ad8
6 changed files with 23 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md
View File

@ -7,7 +7,7 @@
| **Module ID** | E5.M2 | | **Module ID** | E5.M2 |
| **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) | | **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) |
| **Stage target** | Prototype | | **Stage target** | Prototype |
| **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) | | **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP landed → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) |
| **Linear** | Label **`E5.M2`** · [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md) **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) | | **Linear** | Label **`E5.M2`** · [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md) **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) |
## Purpose ## Purpose

2
docs/decomposition/modules/documentation_and_implementation_alignment.md
View File

File diff suppressed because one or more lines are too long

20
docs/reviews/2026-05-28-NEO-95.md
View File

@ -4,9 +4,11 @@
**Scope:** Branch `NEO-95-npc-attack-resolve-player-hp` vs `origin/main` — commits `ac4e1df` … `b702d9f` **Scope:** Branch `NEO-95-npc-attack-resolve-player-hp` vs `origin/main` — commits `ac4e1df` … `b702d9f`
**Base:** `origin/main` **Base:** `origin/main`
**Follow-up:** Suggestions and nits addressed in review follow-up commit; re-verified 2026-05-28.
## Verdict ## Verdict
**Approve with nits** **Approve**
## Summary ## Summary
@ -18,8 +20,8 @@ NEO-95 delivers the server slice cleanly: **`IPlayerCombatHealthStore`** (lazy-i
|------|--------| |------|--------|
| `docs/plans/NEO-95-implementation-plan.md` | **Matches** — AC checkboxes done; dedicated GET (kickoff), damage flow, holder re-check, fixture reset, tests, and file list align with the diff. | | `docs/plans/NEO-95-implementation-plan.md` | **Matches** — AC checkboxes done; dedicated GET (kickoff), damage flow, holder re-check, fixture reset, tests, and file list align with the diff. |
| `docs/plans/E5M2-prototype-backlog.md` (E5M2-09) | **Matches** — AC checked, landed note present; client counterpart NEO-97 called out. | | `docs/plans/E5M2-prototype-backlog.md` (E5M2-09) | **Matches** — AC checked, landed note present; client counterpart NEO-97 called out. |
| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Partially matches** — NEO-95 implementation section + README/Bruno links present; Summary **Status** table still lists E5M2-09 without a “landed” marker (NEO-94 pattern uses “landed” inline). | | `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — NEO-95 implementation section + README/Bruno links; Summary **Status** marks E5M2-09 landed. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E5.M2 row mentions **`IPlayerCombatHealthStore`** in “Owns” but landed bullets stop at NEO-94; NEO-95 not yet appended (same gap NEO-94 review closed for NEO-94). | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-95 landed** (`IPlayerCombatHealthStore`, combat-health GET, Bruno folder, plan/README links). |
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server owns NPC→player damage; client poll-only read model; no client damage math. | | `docs/decomposition/modules/client_server_authority.md` | **Matches** — server owns NPC→player damage; client poll-only read model; no client damage math. |
| `server/README.md` | **Matches** — session player combat HP section, GET route, fixture reset note, npc-runtime snapshot cross-link updated. | | `server/README.md` | **Matches** — session player combat HP section, GET route, fixture reset note, npc-runtime snapshot cross-link updated. |
@ -29,15 +31,17 @@ None.
## Suggestions ## Suggestions
1. **Update `documentation_and_implementation_alignment.md`** — Append **NEO-95 landed** to the E5.M2 tracking row (`IPlayerCombatHealthStore`, `NpcAttackOperations`, `GET …/combat-health`, Bruno `combat-health/`, plan/README links), matching NEO-87–NEO-94 entries. Listed implicitly by review checklist; not in the NEO-95 plan “Files to modify” table but keeps alignment doc truthful. 1. ~~**Update `documentation_and_implementation_alignment.md`** — Append **NEO-95 landed** to the E5.M2 tracking row (`IPlayerCombatHealthStore`, `NpcAttackOperations`, `GET …/combat-health`, Bruno `combat-health/`, plan/README links), matching NEO-87–NEO-94 entries. Listed implicitly by review checklist; not in the NEO-95 plan “Files to modify” table but keeps alignment doc truthful.~~ **Done.**
2. **E5.M2 Summary status table** — Mark **E5M2-09 / NEO-95 landed** inline in the module Summary **Status** row (currently jumps from “NEO-94 runtime snapshot GET landed” to “NEO-95 NPC attack resolve + player combat HP → E5M2-12” without “landed”). 2. ~~**E5.M2 Summary status table** — Mark **E5M2-09 / NEO-95 landed** inline in the module Summary **Status** row (currently jumps from “NEO-94 runtime snapshot GET landed” to “NEO-95 NPC attack resolve + player combat HP → E5M2-12” without “landed”).~~ **Done.**
## Nits ## Nits
- Nit: **`NpcBehaviorState.AttackExecute`** branch in `NpcRuntimeOperations` is unreachable in the normal windup→recover flow (NEO-93); harmless defensive path but could carry a one-line comment or be removed until a visible execute phase is needed. - ~~Nit: **`NpcBehaviorState.AttackExecute`** branch in `NpcRuntimeOperations` is unreachable in the normal windup→recover flow (NEO-93); harmless defensive path but could carry a one-line comment or be removed until a visible execute phase is needed.~~ **Done.** — one-line comment added.
- Nit: **`PlayerCombatHealthApi.BuildResponse`** echoes normalized lowercase **`snapshot.PlayerId`** while **`CooldownSnapshotApi`** echoes the route `{id}` as provided — consistent for prototype lowercase ids; note if mixed-case route ids become a concern.
- Nit: Elite integration test **`GetPlayerCombatHealth_ShouldDecreaseAfterEliteTelegraphComplete`** performs setup GETs in **Arrange** (acceptable for multi-poll timeline); baseline/mid snapshot reads could move to **Assert** if stricter AAA parity with NEO-94 follow-up is desired. - ~~Nit: **`PlayerCombatHealthApi.BuildResponse`** echoes normalized lowercase **`snapshot.PlayerId`** while **`CooldownSnapshotApi`** echoes the route `{id}` as provided — consistent for prototype lowercase ids; note if mixed-case route ids become a concern.~~ **Done.** — **`BuildResponse`** now echoes route **`{id}`** like cooldown snapshot.
- ~~Nit: Elite integration test **`GetPlayerCombatHealth_ShouldDecreaseAfterEliteTelegraphComplete`** performs setup GETs in **Arrange** (acceptable for multi-poll timeline); baseline/mid snapshot reads could move to **Assert** if stricter AAA parity with NEO-94 follow-up is desired.~~ **Done.** — poll GETs kept in **Arrange** as timeline drivers only; **`ReadFromJsonAsync`** verification confined to **Act**/**Assert**.
## Verification ## Verification

9
server/NeonSprawl.Server.Tests/Game/Combat/PlayerCombatHealthApiTests.cs
View File

@ -103,13 +103,9 @@ public sealed class PlayerCombatHealthApiTests
"/game/players/dev-local-1/ability-cast", "/game/players/dev-local-1/ability-cast",
PulseCastRequest()); PulseCastRequest());
castResponse.EnsureSuccessStatusCode(); castResponse.EnsureSuccessStatusCode();
var initSnapshot = await client.GetAsync("/game/world/npc-runtime-snapshot"); _ = await client.GetAsync("/game/world/npc-runtime-snapshot");
initSnapshot.EnsureSuccessStatusCode();
var baseline = await client.GetAsync("/game/players/dev-local-1/combat-health");
baseline.EnsureSuccessStatusCode();
factory.FakeClock!.Advance(TimeSpan.FromSeconds(5)); factory.FakeClock!.Advance(TimeSpan.FromSeconds(5));
var midSnapshot = await client.GetAsync("/game/world/npc-runtime-snapshot"); _ = await client.GetAsync("/game/world/npc-runtime-snapshot");
midSnapshot.EnsureSuccessStatusCode();
factory.FakeClock.Advance(TimeSpan.FromSeconds(2.5)); factory.FakeClock.Advance(TimeSpan.FromSeconds(2.5));
// Act // Act
var snapshotResponse = await client.GetAsync("/game/world/npc-runtime-snapshot"); var snapshotResponse = await client.GetAsync("/game/world/npc-runtime-snapshot");
@ -125,6 +121,7 @@ public sealed class PlayerCombatHealthApiTests
Assert.Equal("recover", elite.State); Assert.Equal("recover", elite.State);
Assert.Equal(75, healthBody!.CurrentHp); Assert.Equal(75, healthBody!.CurrentHp);
Assert.False(healthBody.Defeated); Assert.False(healthBody.Defeated);
Assert.Equal("dev-local-1", healthBody.PlayerId);
var store = factory.Services.GetRequiredService<IPlayerCombatHealthStore>(); var store = factory.Services.GetRequiredService<IPlayerCombatHealthStore>();
store.TryGet("dev-local-1", out var storeSnapshot); store.TryGet("dev-local-1", out var storeSnapshot);

8
server/NeonSprawl.Server/Game/Combat/PlayerCombatHealthApi.cs
View File

@ -21,16 +21,18 @@ public static class PlayerCombatHealthApi
return Results.NotFound(); return Results.NotFound();
} }
return Results.Json(BuildResponse(snapshot)); return Results.Json(BuildResponse(id, snapshot));
}); });
return app; return app;
} }
internal static PlayerCombatHealthResponse BuildResponse(in PlayerCombatHealthSnapshot snapshot) => internal static PlayerCombatHealthResponse BuildResponse(
string playerId,
in PlayerCombatHealthSnapshot snapshot) =>
new() new()
{ {
PlayerId = snapshot.PlayerId, PlayerId = playerId,
MaxHp = snapshot.MaxHp, MaxHp = snapshot.MaxHp,
CurrentHp = snapshot.CurrentHp, CurrentHp = snapshot.CurrentHp,
Defeated = snapshot.Defeated, Defeated = snapshot.Defeated,

1
server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs
View File

@ -200,6 +200,7 @@ public static class NpcRuntimeOperations
} }
case NpcBehaviorState.AttackExecute: case NpcBehaviorState.AttackExecute:
// Unreachable in normal windup→recover flow (NEO-93); defensive if execute becomes a visible phase.
NpcAttackOperations.TryResolveTelegraphComplete( NpcAttackOperations.TryResolveTelegraphComplete(
npcInstanceId, npcInstanceId,
aggroHolderPlayerId, aggroHolderPlayerId,