From 50a2c9ca9dd5fecb051a889b08adb82e1fcbbd5f Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 25 May 2026 11:23:32 -0400 Subject: [PATCH] NEO-83: Address code review follow-ups for combat-targets GET. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fail fast on health store TryGet miss, tighten one-pulse integration test assertions, add fast Bruno cast→GET smoke, update review doc. --- .../Get combat targets after cast spine.bru | 2 +- .../Get combat targets after one cast.bru | 90 +++++++++++++++++++ .../combat-targets/folder.bru | 2 +- docs/plans/NEO-83-implementation-plan.md | 3 +- docs/reviews/2026-05-25-NEO-83.md | 14 +-- .../Game/Combat/CombatTargetsWorldApiTests.cs | 11 ++- .../Game/Combat/CombatTargetsWorldApi.cs | 3 +- 7 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 bruno/neon-sprawl-server/combat-targets/Get combat targets after one cast.bru diff --git a/bruno/neon-sprawl-server/combat-targets/Get combat targets after cast spine.bru b/bruno/neon-sprawl-server/combat-targets/Get combat targets after cast spine.bru index a01f6dd..a4bf633 100644 --- a/bruno/neon-sprawl-server/combat-targets/Get combat targets after cast spine.bru +++ b/bruno/neon-sprawl-server/combat-targets/Get combat targets after cast spine.bru @@ -1,7 +1,7 @@ meta { name: GET combat targets after cast spine (NEO-83) type: http - seq: 2 + seq: 3 } docs { diff --git a/bruno/neon-sprawl-server/combat-targets/Get combat targets after one cast.bru b/bruno/neon-sprawl-server/combat-targets/Get combat targets after one cast.bru new file mode 100644 index 0000000..824a197 --- /dev/null +++ b/bruno/neon-sprawl-server/combat-targets/Get combat targets after one cast.bru @@ -0,0 +1,90 @@ +meta { + name: GET combat targets after one cast (NEO-83) + type: http + seq: 2 +} + +docs { + Fast cast→GET smoke: one pulse on alpha, no cooldown waits. Four-pulse defeat spine is seq 3. +} + +script:pre-request { + const axios = require("axios"); + const baseUrl = bru.getEnvVar("baseUrl") || bru.getVar("baseUrl"); + const playerId = bru.getEnvVar("playerId") || bru.getVar("playerId"); + const jsonHeaders = { headers: { "Content-Type": "application/json" } }; + + await axios.post( + `${baseUrl}/game/players/${playerId}/move`, + { schemaVersion: 1, target: { x: -5, y: 0.9, z: -5 } }, + jsonHeaders, + ); + + await axios.post( + `${baseUrl}/game/players/${playerId}/hotbar-loadout`, + { + schemaVersion: 1, + slots: [{ slotIndex: 0, abilityId: "prototype_pulse" }], + }, + jsonHeaders, + ); + + await axios.post( + `${baseUrl}/game/players/${playerId}/target/select`, + { schemaVersion: 1, targetId: "prototype_target_alpha" }, + jsonHeaders, + ); + + const castResponse = await axios.post( + `${baseUrl}/game/players/${playerId}/ability-cast`, + { + schemaVersion: 1, + slotIndex: 0, + abilityId: "prototype_pulse", + targetId: "prototype_target_alpha", + }, + jsonHeaders, + ); + + const castBody = castResponse.data; + if (!castBody?.accepted || !castBody?.combatResolution) { + throw new Error(`neo83 one-pulse cast failed: ${JSON.stringify(castBody)}`); + } + + bru.setVar("neo83OnePulseCast", JSON.stringify(castBody)); +} + +get { + url: {{baseUrl}}/game/world/combat-targets + body: none + auth: none +} + +tests { + test("one-pulse cast body shows 75 HP remaining", function () { + const castBody = JSON.parse(bru.getVar("neo83OnePulseCast")); + expect(castBody.combatResolution.targetRemainingHp).to.equal(75); + expect(castBody.combatResolution.targetDefeated).to.equal(false); + }); + + test("GET alpha reflects cast damage authoritatively", function () { + const body = res.getBody(); + const alpha = body.targets.find( + (x) => x.targetId === "prototype_target_alpha", + ); + expect(alpha).to.be.an("object"); + expect(alpha.currentHp).to.equal(75); + expect(alpha.maxHp).to.equal(100); + expect(alpha.defeated).to.equal(false); + }); + + test("beta unchanged after alpha-only cast", function () { + const body = res.getBody(); + const beta = body.targets.find( + (x) => x.targetId === "prototype_target_beta", + ); + expect(beta).to.be.an("object"); + expect(beta.currentHp).to.equal(100); + expect(beta.defeated).to.equal(false); + }); +} diff --git a/bruno/neon-sprawl-server/combat-targets/folder.bru b/bruno/neon-sprawl-server/combat-targets/folder.bru index 6b9d9cd..935d26f 100644 --- a/bruno/neon-sprawl-server/combat-targets/folder.bru +++ b/bruno/neon-sprawl-server/combat-targets/folder.bru @@ -3,6 +3,6 @@ meta { } docs { - NEO-83: fresh GET on seq 1; cast spine on seq 2 (~13s pre-request for four-pulse defeat). + NEO-83: fresh GET on seq 1; one-pulse cast→GET on seq 2 (~0s); four-pulse defeat spine on seq 3 (~13s pre-request). Cross-link: bruno/neon-sprawl-server/ability-cast/ for cast-only verification. } diff --git a/docs/plans/NEO-83-implementation-plan.md b/docs/plans/NEO-83-implementation-plan.md index bf82be1..c6ab6d9 100644 --- a/docs/plans/NEO-83-implementation-plan.md +++ b/docs/plans/NEO-83-implementation-plan.md @@ -73,7 +73,8 @@ 7. **Bruno (`bruno/neon-sprawl-server/combat-targets/`):** - **`Get combat targets.bru`** — assert 200, **`schemaVersion` 1**, **`targets.length === 2`**, ascending ids, both at full HP on fresh server. - - **`Get combat targets after cast spine.bru`** — pre-request: move + hotbar + lock alpha + one pulse cast; GET asserts alpha **`currentHp` 75**; optional extended pre-request for four casts + **`defeated` true** on alpha (AC spine). + - **`Get combat targets after one cast.bru`** — pre-request: move + hotbar + lock alpha + one pulse cast; GET asserts alpha **`currentHp` 75**. + - **`Get combat targets after cast spine.bru`** — pre-request: four pulse casts + defeat; GET asserts alpha **`defeated` true** (AC spine). - **`folder.bru`** — run order note; cross-link ability-cast folder. 8. **Docs:** Add **`server/README.md`** **`GET /game/world/combat-targets`** section with field table + curl example; update NEO-80 health store row (“HTTP read” no longer pending). Reconcile [E5M1-prototype-backlog.md](E5M1-prototype-backlog.md) E5M1-08 checkboxes and [E5_M1_CombatRulesEngine.md](../decomposition/modules/E5_M1_CombatRulesEngine.md) when landed. diff --git a/docs/reviews/2026-05-25-NEO-83.md b/docs/reviews/2026-05-25-NEO-83.md index f4c444d..3dcfb5a 100644 --- a/docs/reviews/2026-05-25-NEO-83.md +++ b/docs/reviews/2026-05-25-NEO-83.md @@ -4,6 +4,8 @@ **Scope:** Branch `NEO-83-combat-targets-snapshot` · commits `902e038`–`52b1d4d` vs `origin/main` **Base:** `origin/main` +**Follow-up:** Code review suggestions and nits addressed in follow-up commit on `NEO-83-combat-targets-snapshot`. + ## Verdict **Approve with nits** @@ -33,17 +35,17 @@ None. ## Suggestions -1. **Silent omission on `TryGet` miss** — In `CombatTargetsWorldApi`, registry ids that fail `healthStore.TryGet` are skipped via `continue`, which could return a partial `targets` array without error. For prototype registry ids, `InMemoryCombatEntityHealthStore.TryGet` always lazy-inits and succeeds; consider removing the skip or logging/asserting in dev so a store regression cannot silently drop HUD rows. +1. ~~**Silent omission on `TryGet` miss** — In `CombatTargetsWorldApi`, registry ids that fail `healthStore.TryGet` are skipped via `continue`, which could return a partial `targets` array without error. For prototype registry ids, `InMemoryCombatEntityHealthStore.TryGet` always lazy-inits and succeeds; consider removing the skip or logging/asserting in dev so a store regression cannot silently drop HUD rows.~~ **Done.** — `TryGet` miss throws `InvalidOperationException` instead of skipping. -2. **Bruno one-pulse GET smoke (optional)** — Plan §7 described a spine asserting alpha **`currentHp` 75** after one pulse; implementation consolidates into the four-pulse defeat spine only. Integration test **`GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha`** covers the gap — optional separate Bruno request if manual QA wants a fast (~0s) cast→GET check without cooldown waits. +2. ~~**Bruno one-pulse GET smoke (optional)** — Plan §7 described a spine asserting alpha **`currentHp` 75** after one pulse; implementation consolidates into the four-pulse defeat spine only. Integration test **`GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha`** covers the gap — optional separate Bruno request if manual QA wants a fast (~0s) cast→GET check without cooldown waits.~~ **Done.** — Added `Get combat targets after one cast.bru` (seq 2); defeat spine moved to seq 3. ## Nits -- Nit: **`BindSlot0PulseAsync` / `LockPrototypeTargetAlphaAsync` / `PulseCastRequest`** duplicate helpers in `AbilityCastApiTests` — acceptable for story scope; optional shared test fixture if a third combat cast consumer appears. +- Nit: **`BindSlot0PulseAsync` / `LockPrototypeTargetAlphaAsync` / `PulseCastRequest`** duplicate helpers in `AbilityCastApiTests` — acceptable for story scope; optional shared test fixture if a third combat cast consumer appears. **Deferred** — no third consumer yet. -- Nit: **`GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha`** uses `EnsureSuccessStatusCode` on cast but does not assert `combatResolution.targetRemainingHp == 75` in Arrange — would tighten cast→GET coherence (mirror defeat test’s cast-body checks). +- ~~Nit: **`GetCombatTargets_ShouldReflectCastDamage_AfterOnePulseOnAlpha`** uses `EnsureSuccessStatusCode` on cast but does not assert `combatResolution.targetRemainingHp == 75` in Arrange — would tighten cast→GET coherence (mirror defeat test’s cast-body checks).~~ **Done.** — Assert block now verifies cast `combatResolution.targetRemainingHp` before GET row checks. -- Nit: Plan §6 mentions teleport-in-range setup like some Bruno flows; cast tests on this branch omit move/teleport and still pass (same as `AbilityCastApiTests` default) — no action unless range gates tighten. +- Nit: Plan §6 mentions teleport-in-range setup like some Bruno flows; cast tests on this branch omit move/teleport and still pass (same as `AbilityCastApiTests` default) — no action unless range gates tighten. **No action** — per review. ## Verification @@ -58,4 +60,4 @@ dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj **Full suite:** unrelated failures observed locally (e.g. crafting/inventory tests — flaky or environment-specific; not touched by this diff). Confirm CI on PR before merge. -**Bruno (manual):** Run `bruno/neon-sprawl-server/combat-targets/` — seq 1 fresh GET; seq 2 cast spine (~13s pre-request for four-pulse defeat + HP stepping assertions). +**Bruno (manual):** Run `bruno/neon-sprawl-server/combat-targets/` — seq 1 fresh GET; seq 2 one-pulse cast→GET (~0s); seq 3 four-pulse defeat spine (~13s pre-request + HP stepping assertions). diff --git a/server/NeonSprawl.Server.Tests/Game/Combat/CombatTargetsWorldApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Combat/CombatTargetsWorldApiTests.cs index 509a929..5509ef7 100644 --- a/server/NeonSprawl.Server.Tests/Game/Combat/CombatTargetsWorldApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Combat/CombatTargetsWorldApiTests.cs @@ -94,11 +94,18 @@ public class CombatTargetsWorldApiTests "/game/players/dev-local-1/ability-cast", PulseCastRequest()); castResponse.EnsureSuccessStatusCode(); + var castBody = await castResponse.Content.ReadFromJsonAsync(); + // Act var response = await client.GetAsync("/game/world/combat-targets"); - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); var body = await response.Content.ReadFromJsonAsync(); + + // Assert + Assert.NotNull(castBody); + Assert.True(castBody!.Accepted); + Assert.NotNull(castBody.CombatResolution); + Assert.Equal(75, castBody.CombatResolution!.TargetRemainingHp); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); Assert.NotNull(body); var alpha = body!.Targets.Single(t => t.TargetId == PrototypeTargetRegistry.PrototypeTargetAlphaId); Assert.Equal(75, alpha.CurrentHp); diff --git a/server/NeonSprawl.Server/Game/Combat/CombatTargetsWorldApi.cs b/server/NeonSprawl.Server/Game/Combat/CombatTargetsWorldApi.cs index 1247817..ef1e40c 100644 --- a/server/NeonSprawl.Server/Game/Combat/CombatTargetsWorldApi.cs +++ b/server/NeonSprawl.Server/Game/Combat/CombatTargetsWorldApi.cs @@ -17,7 +17,8 @@ public static class CombatTargetsWorldApi { if (!healthStore.TryGet(id, out var snapshot)) { - continue; + throw new InvalidOperationException( + $"Prototype combat target '{id}' is registered but missing from {nameof(ICombatEntityHealthStore)}."); } targets.Add(