NEO-106: Address code review findings for combat encounter wiring.
parent
a7b835290c
commit
9c74933e6e
|
|
@ -54,7 +54,7 @@
|
|||
|
||||
- **Wiring:** `EncounterCombatWiring.TryProcessCastOutcome` — activate on `damageDealt > 0`, mark on `targetDefeated`, `TryCompleteAndGrant` when all required NPCs defeated.
|
||||
- **Integration:** `AbilityCastApi` injects encounter/inventory deps; call order preserves NEO-44 gig XP before encounter completion.
|
||||
- **Tests:** `EncounterCombatWiringTests` (6); `AbilityCastApiTests` — defeat-all-three loot once, two-defeat no loot, 75 gig XP after three defeats.
|
||||
- **Tests:** `EncounterCombatWiringTests` (8); `AbilityCastApiTests` — defeat-all-three loot once, two-defeat no loot, 75 gig XP after three defeats.
|
||||
- **Docs:** `server/README.md`; E5.M3 module snapshot + alignment register + backlog updated.
|
||||
|
||||
## Technical approach
|
||||
|
|
@ -127,7 +127,7 @@
|
|||
|
||||
| Test file | What it covers |
|
||||
|-----------|----------------|
|
||||
| `EncounterCombatWiringTests.cs` | **Arrange:** `InMemoryWebApplicationFactory` services. **Act/Assert:** damaging hit activates encounter; defeat marks progress; third defeat invokes completion (inventory + `IsCompleted`); non-encounter NPC no-op; already-completed encounter skips activate/mark; completion not called when only 2/3 defeated. |
|
||||
| `EncounterCombatWiringTests.cs` | **Arrange:** `InMemoryWebApplicationFactory` services. **Act/Assert:** damaging hit activates encounter; defeat marks progress; third defeat invokes completion (inventory + `IsCompleted`); non-encounter NPC no-op; already-completed encounter skips activate/mark; completion not called when only 2/3 defeated (non-lethal third hit); bag-full third defeat retains all-defeated progress without completion mark. |
|
||||
| `AbilityCastApiTests.cs` | **Arrange:** HTTP client + pulse bind + per-NPC lock/defeat helpers. **Act/Assert:** defeat all three via cast → `scrap_metal_bulk` ×10 + `contract_handoff_token` ×1 once; two defeats → zero loot; gig XP 75 on breach after three defeats, skill XP unchanged; existing NEO-44 single-defeat test still passes. |
|
||||
|
||||
## Open questions / risks
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ NEO-106 adds **`EncounterCombatWiring.TryProcessCastOutcome`**, a static orchest
|
|||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-106** combat wiring; NEO-108+ HTTP still outstanding. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-106** wiring landed. |
|
||||
| Full-stack epic decomposition | **N/A** — plan explicitly defers client counterpart to **NEO-110** via NEO-108; no prototype slice completion claim. |
|
||||
| `server/README.md` | **Partially matches** — new **Encounter combat wiring (NEO-106)** section is accurate; encounter progress section still has one stale “land in NEO-106 / NEO-108” line (see Nits). |
|
||||
| `server/README.md` | **Matches** — new **Encounter combat wiring (NEO-106)** section is accurate; encounter progress section defers HTTP read to NEO-108 only. |
|
||||
|
||||
## Blocking issues
|
||||
|
||||
|
|
@ -30,29 +30,29 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Unit test for 2/3 defeated** — Plan test table lists “completion not called when only 2/3 defeated.” Integration test **`PostAbilityCast_ShouldNotGrantEncounterLoot_WhenOnlyTwoNpcDefeated`** covers this at HTTP level; a wiring-level AAA test (two **`DefeatNpc`** calls, third **`TryProcessCastOutcome`** with `targetDefeated: false`) would mirror the plan table and isolate completion gating without HTTP.
|
||||
1. ~~**Unit test for 2/3 defeated** — Plan test table lists “completion not called when only 2/3 defeated.” Integration test **`PostAbilityCast_ShouldNotGrantEncounterLoot_WhenOnlyTwoNpcDefeated`** covers this at HTTP level; a wiring-level AAA test (two **`DefeatNpc`** calls, third **`TryProcessCastOutcome`** with `targetDefeated: false`) would mirror the plan table and isolate completion gating without HTTP.~~ **Done.** `TryProcessCastOutcome_ShouldNotComplete_WhenOnlyTwoTargetsDefeatedAndThirdHitNonLethal`.
|
||||
|
||||
2. **Inventory-full on third defeat (wiring path)** — README and plan document fail-closed retry semantics; NEO-105 ops tests cover grant deny. Consider a wiring or integration test that fills the bag before the third lethal cast and asserts progress all-defeated but **`IsCompleted`** false and zero loot delta — exercises the documented retry path end-to-end.
|
||||
2. ~~**Inventory-full on third defeat (wiring path)** — README and plan document fail-closed retry semantics; NEO-105 ops tests cover grant deny. Consider a wiring or integration test that fills the bag before the third lethal cast and asserts progress all-defeated but **`IsCompleted`** false and zero loot delta — exercises the documented retry path end-to-end.~~ **Done.** `TryProcessCastOutcome_ShouldRetainProgressWithoutCompletion_WhenBagFullOnThirdDefeat`.
|
||||
|
||||
3. **`encounter_complete` success hook comment** — **`EncounterCompletionOperations`** has NEO-109 deny hook + E7.M2 success TODO; **`EncounterCombatWiring`** logs completion deny at Debug. A brief `// TODO(E9.M1): catalog emit — encounter_complete` on the success branch (or in **`TryCompleteAndGrant`**) would align with NEO-109 scope without runtime code.
|
||||
3. ~~**`encounter_complete` success hook comment** — **`EncounterCompletionOperations`** has NEO-109 deny hook + E7.M2 success TODO; **`EncounterCombatWiring`** logs completion deny at Debug. A brief `// TODO(E9.M1): catalog emit — encounter_complete` on the success branch (or in **`TryCompleteAndGrant`**) would align with NEO-109 scope without runtime code.~~ **Done.** Hook comments on **`EncounterCombatWiring`** success path and **`EncounterCompletionOperations.TryCompleteAndGrant`**.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`server/README.md`** encounter progress section (line ~154) still reads “Combat cast wiring … land in NEO-106 / NEO-108” — combat wiring is landed; reword to “HTTP progress read lands in NEO-108” (the next line already links the NEO-106 section).
|
||||
- ~~Nit: **`server/README.md`** encounter progress section (line ~154) still reads “Combat cast wiring … land in NEO-106 / NEO-108” — combat wiring is landed; reword to “HTTP progress read lands in NEO-108” (the next line already links the NEO-106 section).~~ **Done.**
|
||||
|
||||
- Nit: **`PostAbilityCast_ShouldGrantEncounterLootOnce_WhenAllThreeNpcDefeated`** reads **`inventoryAfter`** and resolves **`completionStore`** after the defeat loop (still under **Act**). Moving those reads into **Assert** would tighten AAA; behavior is unchanged.
|
||||
- ~~Nit: **`PostAbilityCast_ShouldGrantEncounterLootOnce_WhenAllThreeNpcDefeated`** reads **`inventoryAfter`** and resolves **`completionStore`** after the defeat loop (still under **Act**). Moving those reads into **Assert** would tighten AAA; behavior is unchanged.~~ **Done.** Inventory GET and completion store resolve moved to **Assert** in both encounter-loot integration tests.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# NEO-106-focused (9 tests — passed locally)
|
||||
# NEO-106-focused (11 tests — passed locally)
|
||||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~EncounterCombatWiring|FullyQualifiedName~PostAbilityCast_ShouldGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldNotGrantEncounterLoot|FullyQualifiedName~PostAbilityCast_ShouldGrantGigXpPerDefeat"
|
||||
|
||||
# Broader encounter slice (69 tests — passed locally)
|
||||
# Broader encounter slice (71 tests — passed locally)
|
||||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters"
|
||||
|
||||
# CI parity
|
||||
python3 scripts/validate_content.py
|
||||
```
|
||||
|
||||
**Local run:** 9/9 NEO-106-focused tests passed; 69/69 `Encounters` filter tests passed.
|
||||
**Local run:** 11/11 NEO-106-focused tests passed; 71/71 `Encounters` filter tests passed.
|
||||
|
|
|
|||
|
|
@ -880,11 +880,11 @@ public sealed class AbilityCastApiTests
|
|||
client,
|
||||
factory,
|
||||
PrototypeNpcRegistry.PrototypeNpcEliteId);
|
||||
|
||||
// Assert
|
||||
var inventoryAfter = await client.GetFromJsonAsync<PlayerInventorySnapshotResponse>(
|
||||
"/game/players/dev-local-1/inventory");
|
||||
var completionStore = factory.Services.GetRequiredService<IEncounterCompletionStore>();
|
||||
|
||||
// Assert
|
||||
Assert.NotNull(inventoryBefore);
|
||||
Assert.NotNull(inventoryAfter);
|
||||
Assert.Equal(
|
||||
|
|
@ -915,11 +915,11 @@ public sealed class AbilityCastApiTests
|
|||
client,
|
||||
factory,
|
||||
PrototypeNpcRegistry.PrototypeNpcRangedId);
|
||||
|
||||
// Assert
|
||||
var inventoryAfter = await client.GetFromJsonAsync<PlayerInventorySnapshotResponse>(
|
||||
"/game/players/dev-local-1/inventory");
|
||||
var completionStore = factory.Services.GetRequiredService<IEncounterCompletionStore>();
|
||||
|
||||
// Assert
|
||||
Assert.NotNull(inventoryBefore);
|
||||
Assert.NotNull(inventoryAfter);
|
||||
Assert.Equal(
|
||||
|
|
|
|||
|
|
@ -167,6 +167,81 @@ public sealed class EncounterCombatWiringTests
|
|||
Assert.Equal(CountBagItem(before!, "contract_handoff_token"), CountBagItem(after!, "contract_handoff_token"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryProcessCastOutcome_ShouldNotComplete_WhenOnlyTwoTargetsDefeatedAndThirdHitNonLethal()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var deps = ResolveDependencies(factory);
|
||||
DefeatNpc(deps, MeleeNpc);
|
||||
DefeatNpc(deps, RangedNpc);
|
||||
|
||||
// Act
|
||||
EncounterCombatWiring.TryProcessCastOutcome(
|
||||
PlayerId,
|
||||
EliteNpc,
|
||||
damageDealt: 25,
|
||||
targetDefeated: false,
|
||||
deps.EncounterRegistry,
|
||||
deps.RewardTableRegistry,
|
||||
deps.ProgressStore,
|
||||
deps.CompletionStore,
|
||||
deps.ItemRegistry,
|
||||
deps.InventoryStore,
|
||||
TimeProvider.System);
|
||||
|
||||
// Assert
|
||||
Assert.False(
|
||||
EncounterProgressOperations.IsAllRequiredTargetsDefeated(
|
||||
PlayerId,
|
||||
EncounterId,
|
||||
deps.EncounterRegistry,
|
||||
deps.ProgressStore));
|
||||
Assert.False(deps.CompletionStore.IsCompleted(PlayerId, EncounterId));
|
||||
deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventory);
|
||||
Assert.Equal(0, CountBagItem(inventory!, "scrap_metal_bulk"));
|
||||
Assert.Equal(0, CountBagItem(inventory!, "contract_handoff_token"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryProcessCastOutcome_ShouldRetainProgressWithoutCompletion_WhenBagFullOnThirdDefeat()
|
||||
{
|
||||
// Arrange
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var deps = ResolveDependencies(factory);
|
||||
DefeatNpc(deps, MeleeNpc);
|
||||
DefeatNpc(deps, RangedNpc);
|
||||
FillBag(deps);
|
||||
deps.InventoryStore.TryGetSnapshot(PlayerId, out var beforeInventory);
|
||||
|
||||
// Act
|
||||
EncounterCombatWiring.TryProcessCastOutcome(
|
||||
PlayerId,
|
||||
EliteNpc,
|
||||
damageDealt: 25,
|
||||
targetDefeated: true,
|
||||
deps.EncounterRegistry,
|
||||
deps.RewardTableRegistry,
|
||||
deps.ProgressStore,
|
||||
deps.CompletionStore,
|
||||
deps.ItemRegistry,
|
||||
deps.InventoryStore,
|
||||
TimeProvider.System);
|
||||
|
||||
// Assert
|
||||
Assert.True(
|
||||
EncounterProgressOperations.IsAllRequiredTargetsDefeated(
|
||||
PlayerId,
|
||||
EncounterId,
|
||||
deps.EncounterRegistry,
|
||||
deps.ProgressStore));
|
||||
Assert.False(deps.CompletionStore.IsCompleted(PlayerId, EncounterId));
|
||||
deps.InventoryStore.TryGetSnapshot(PlayerId, out var afterInventory);
|
||||
Assert.Equal(TotalBagQuantity(beforeInventory!), TotalBagQuantity(afterInventory!));
|
||||
Assert.Equal(0, CountBagItem(afterInventory!, "scrap_metal_bulk"));
|
||||
Assert.Equal(0, CountBagItem(afterInventory!, "contract_handoff_token"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task TryProcessCastOutcome_ShouldNotActivate_WhenZeroDamageWithoutDefeat()
|
||||
{
|
||||
|
|
@ -215,6 +290,34 @@ public sealed class EncounterCombatWiringTests
|
|||
DefeatNpc(deps, EliteNpc);
|
||||
}
|
||||
|
||||
private static void FillBag(EncounterCombatWiringTestDependencies deps)
|
||||
{
|
||||
for (var i = 0; i < PlayerInventorySnapshot.BagSlotCount; i++)
|
||||
{
|
||||
var add = PlayerInventoryOperations.TryAddStack(
|
||||
PlayerId,
|
||||
"survey_drone_kit",
|
||||
quantity: 1,
|
||||
deps.ItemRegistry,
|
||||
deps.InventoryStore);
|
||||
Assert.Equal(PlayerInventoryMutationKind.Applied, add.Kind);
|
||||
}
|
||||
}
|
||||
|
||||
private static int TotalBagQuantity(PlayerInventorySnapshot snapshot)
|
||||
{
|
||||
var total = 0;
|
||||
foreach (var slot in snapshot.BagSlots)
|
||||
{
|
||||
if (!slot.IsEmpty)
|
||||
{
|
||||
total += slot.Quantity;
|
||||
}
|
||||
}
|
||||
|
||||
return total;
|
||||
}
|
||||
|
||||
private static EncounterCombatWiringTestDependencies ResolveDependencies(InMemoryWebApplicationFactory factory)
|
||||
{
|
||||
var services = factory.Services;
|
||||
|
|
|
|||
|
|
@ -86,6 +86,10 @@ public static class EncounterCombatWiring
|
|||
playerId,
|
||||
encounterId,
|
||||
completion.ReasonCode);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// --- Telemetry hook site (NEO-109): future E9.M1 catalog event `encounter_complete` ---
|
||||
// TODO(E9.M1): catalog emit — once per player+encounter when grants commit (NEO-106 cast path).
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -108,6 +108,7 @@ public static class EncounterCompletionOperations
|
|||
}
|
||||
|
||||
// TODO(E7.M2): quest-credit router consumes EncounterCompleteEvent (NEO-107 expands persistence + hook).
|
||||
// TODO(E9.M1): catalog emit — encounter_complete on successful grant commit (NEO-109).
|
||||
|
||||
var normalizedPlayerId = EncounterProgressIds.NormalizePlayerId(playerId);
|
||||
var completeEvent = new EncounterCompleteEvent(
|
||||
|
|
|
|||
|
|
@ -151,7 +151,7 @@ Per-player encounter state is server-owned in **`IEncounterProgressStore`** (act
|
|||
- **`TryMarkTargetDefeated(…)`** — requires prior activation; accumulates defeats in any order; idempotent per NPC.
|
||||
- **`IsAllRequiredTargetsDefeated(playerId, encounterId, …)`** — set equality vs catalog **`requiredNpcInstanceIds`**.
|
||||
|
||||
Combat cast wiring (**`AbilityCastApi`**) and **`GET /game/players/{id}/encounter-progress`** land in NEO-106 / NEO-108. Plan: [NEO-104 implementation plan](../../docs/plans/NEO-104-implementation-plan.md).
|
||||
**`GET /game/players/{id}/encounter-progress`** lands in NEO-108. Plan: [NEO-104 implementation plan](../../docs/plans/NEO-104-implementation-plan.md).
|
||||
|
||||
**Combat wiring (NEO-106):** see [Encounter combat wiring (NEO-106)](#encounter-combat-wiring-neo-106) — **`EncounterCombatWiring`** on **`POST …/ability-cast`** accept path.
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue