NEO-83: Address code review follow-ups for combat-targets GET.

Fail fast on health store TryGet miss, tighten one-pulse integration test assertions, add fast Bruno cast→GET smoke, update review doc.
pull/117/head
VinPropane 2026-05-25 11:23:32 -04:00
parent aafa5e472a
commit 50a2c9ca9d
7 changed files with 113 additions and 12 deletions

View File

@ -1,7 +1,7 @@
meta {
name: GET combat targets after cast spine (NEO-83)
type: http
seq: 2
seq: 3
}
docs {

View File

@ -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);
});
}

View File

@ -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.
}

View File

@ -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.

View File

@ -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 tests 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 tests 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).

View File

@ -94,11 +94,18 @@ public class CombatTargetsWorldApiTests
"/game/players/dev-local-1/ability-cast",
PulseCastRequest());
castResponse.EnsureSuccessStatusCode();
var castBody = await castResponse.Content.ReadFromJsonAsync<AbilityCastResponse>();
// Act
var response = await client.GetAsync("/game/world/combat-targets");
// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
var body = await response.Content.ReadFromJsonAsync<CombatTargetsListResponse>();
// 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);

View File

@ -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(