NEO-92: Reconcile docs, backlog, alignment, and code review.
Update E5M2-06 landed notes, alignment row, review follow-ups, and exact-leash-boundary test documentation. Co-authored-by: Cursor <cursoragent@cursor.com>pull/130/head
parent
843abf7126
commit
7682fac46a
|
|
@ -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-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-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
|
||||||
|
|
@ -83,6 +83,8 @@ The **first shipped three-archetype NPC spine** under `content/npc-behaviors/*.j
|
||||||
|
|
||||||
**NPC instance registry (NEO-91):** **`PrototypeNpcRegistry`** in `server/NeonSprawl.Server/Game/Npc/` — instance ids **`prototype_npc_melee`**, **`prototype_npc_ranged`**, **`prototype_npc_elite`** with behavior bindings + world anchors; **`ICombatEntityHealthStore`** uses catalog **`maxHp`** per instance. Plan: [NEO-91 implementation plan](../../plans/NEO-91-implementation-plan.md). **Breaking:** Godot constants migrate in [NEO-97](https://linear.app/neon-sprawl/issue/NEO-97).
|
**NPC instance registry (NEO-91):** **`PrototypeNpcRegistry`** in `server/NeonSprawl.Server/Game/Npc/` — instance ids **`prototype_npc_melee`**, **`prototype_npc_ranged`**, **`prototype_npc_elite`** with behavior bindings + world anchors; **`ICombatEntityHealthStore`** uses catalog **`maxHp`** per instance. Plan: [NEO-91 implementation plan](../../plans/NEO-91-implementation-plan.md). **Breaking:** Godot constants migrate in [NEO-97](https://linear.app/neon-sprawl/issue/NEO-97).
|
||||||
|
|
||||||
|
**Threat / aggro store (NEO-92):** **`IThreatStateStore`** + **`AggroOperations`** in `server/NeonSprawl.Server/Game/Npc/` — first damaging cast acquires holder; leash break clears holder (move + cast hooks). HTTP projection in [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94). Plan: [NEO-92 implementation plan](../../plans/NEO-92-implementation-plan.md).
|
||||||
|
|
||||||
**NPC behavior definitions HTTP (NEO-90):** **`GET /game/world/npc-behavior-definitions`** — versioned read-only projection (`schemaVersion` **1**, **`npcBehaviors`**) backed by **`INpcBehaviorDefinitionRegistry`**. Plan: [NEO-90 implementation plan](../../plans/NEO-90-implementation-plan.md); [server README — NPC behavior definitions (NEO-90)](../../../server/README.md#npc-behavior-definitions-neo-90); Bruno `bruno/neon-sprawl-server/npc-behavior-definitions/`.
|
**NPC behavior definitions HTTP (NEO-90):** **`GET /game/world/npc-behavior-definitions`** — versioned read-only projection (`schemaVersion` **1**, **`npcBehaviors`**) backed by **`INpcBehaviorDefinitionRegistry`**. Plan: [NEO-90 implementation plan](../../plans/NEO-90-implementation-plan.md); [server README — NPC behavior definitions (NEO-90)](../../../server/README.md#npc-behavior-definitions-neo-90); Bruno `bruno/neon-sprawl-server/npc-behavior-definitions/`.
|
||||||
|
|
||||||
## Risks and telemetry
|
## Risks and telemetry
|
||||||
|
|
|
||||||
File diff suppressed because one or more lines are too long
|
|
@ -208,7 +208,7 @@ Working backlog for **Epic 5 — Slice 2** ([NPC archetypes and telegraphs](../d
|
||||||
|
|
||||||
- `IThreatStateStore` + in-memory implementation keyed by NPC instance id.
|
- `IThreatStateStore` + in-memory implementation keyed by NPC instance id.
|
||||||
- `AggroOperations.TryAcquire` / `TryClearOnLeash` using player **`PositionState`** + registry anchors.
|
- `AggroOperations.TryAcquire` / `TryClearOnLeash` using player **`PositionState`** + registry anchors.
|
||||||
- Hook from **`CombatOperations.TryResolve`** on successful NPC damage (player → NPC).
|
- Hook from **`AbilityCastApi`** after successful resolve when **`DamageDealt > 0`** (player → NPC).
|
||||||
- Unit + integration tests (AAA).
|
- Unit + integration tests (AAA).
|
||||||
|
|
||||||
**Out of scope**
|
**Out of scope**
|
||||||
|
|
@ -217,9 +217,11 @@ Working backlog for **Epic 5 — Slice 2** ([NPC archetypes and telegraphs](../d
|
||||||
|
|
||||||
**Acceptance criteria**
|
**Acceptance criteria**
|
||||||
|
|
||||||
- [ ] First damaging cast on NPC sets aggro holder to casting player id.
|
- [x] First damaging cast on NPC sets aggro holder to casting player id.
|
||||||
- [ ] Holder clears when player leaves `leashRadius` (deterministic distance check).
|
- [x] Holder clears when player leaves `leashRadius` (deterministic distance check).
|
||||||
- [ ] Re-aggro inside radius after clear follows same first-hit rule.
|
- [x] Re-aggro inside radius after clear follows same first-hit rule.
|
||||||
|
|
||||||
|
**Landed ([NEO-92](https://linear.app/neon-sprawl/issue/NEO-92)):** **`IThreatStateStore`** + **`AggroOperations`** — acquire on damaging cast, leash clear on move/cast; plan [NEO-92-implementation-plan.md](NEO-92-implementation-plan.md).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,55 @@
|
||||||
|
# Code review — NEO-92 (E5M2-06)
|
||||||
|
|
||||||
|
**Date:** 2026-05-27
|
||||||
|
**Scope:** Branch `NEO-92` vs `main` — NEO-92 aggro store implementation, plan/docs, and code review
|
||||||
|
**Base:** `main`
|
||||||
|
|
||||||
|
**Follow-up:** Code review suggestions addressed (docs alignment, backlog hook bullet, exact-leash-boundary test).
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
**Approve with nits**
|
||||||
|
|
||||||
|
## Summary
|
||||||
|
|
||||||
|
This branch adds the prototype aggro spine for Epic 5 Slice 2: an in-memory `IThreatStateStore`, deterministic `AggroOperations` (first-hit acquire, leash-radius clear), and wiring in `AbilityCastApi`, `PositionStateApi`, and the dev combat-target fixture. The design keeps `CombatOperations` engine-pure and matches kickoff decisions (cast-layer acquire, move + pre-acquire leash clear, no proximity re-aggro). Test coverage is solid — unit, HTTP integration, cast guard skip, move clear, and fixture reset — and the filtered NEO-92-related test run passed locally. Risk is low for merge; remaining gaps are documentation tracking and a few optional test/doc polish items.
|
||||||
|
|
||||||
|
## Documentation checked
|
||||||
|
|
||||||
|
| Path | Result |
|
||||||
|
|------|--------|
|
||||||
|
| `docs/plans/NEO-92-implementation-plan.md` | **Matches** — store, ops, hooks, fixture, tests, and reconciliation section align with the diff. |
|
||||||
|
| `docs/plans/E5M2-prototype-backlog.md` (E5M2-06) | **Matches** — AC checkboxes, landed note, and in-scope hook bullet updated to `AbilityCastApi`. |
|
||||||
|
| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — E5M2-06 landed note and threat-store section present. |
|
||||||
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-92 landed** note and plan link. |
|
||||||
|
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server-owned aggro holder; no client projection in this slice (NEO-94 deferred). |
|
||||||
|
| `server/README.md` | **Matches** — threat/aggro section documents acquire, leash clear, fixture reset, and NEO-94 deferral. |
|
||||||
|
|
||||||
|
## Blocking issues
|
||||||
|
|
||||||
|
None.
|
||||||
|
|
||||||
|
## Suggestions
|
||||||
|
|
||||||
|
1. ~~**Update `documentation_and_implementation_alignment.md` E5.M2 row** — Add **NEO-92 landed** (`IThreatStateStore`, `AggroOperations`, cast/move/fixture hooks) and link `NEO-92-implementation-plan.md` in the References column, consistent with NEO-87–NEO-91 entries. The implementation plan and E5_M2 module doc already claim this update.~~ **Done.** E5.M2 row updated with NEO-92 landed note and References link.
|
||||||
|
2. ~~**Align E5M2-06 backlog “In scope” hook bullet** — Change “Hook from `CombatOperations.TryResolve`” to **`AbilityCastApi` after successful resolve when `DamageDealt > 0`** so the backlog matches the adopted kickoff decision and avoids misleading NEO-93 readers.~~ **Done.** E5M2-06 in-scope bullet updated.
|
||||||
|
3. ~~**Add an exact-leash-boundary unit test** — Plan calls out inclusive boundary (`distance > leashRadius` keeps holder). Existing tests use clearly inside/outside positions; a case at `distance == leashRadius` would lock that invariant alongside `HorizontalReach` conventions.~~ **Done.** `TryClearOnLeashForPlayer_ShouldKeepHolder_WhenPlayerAtExactLeashRadius` in `AggroOperationsTests.cs`.
|
||||||
|
|
||||||
|
## Nits
|
||||||
|
|
||||||
|
- ~~Nit: Branch mixes five unrelated `chore:` commits (`docs/dev/local-mlx-cursor.md`, README pointer) with NEO-92 work. Consider splitting or noting in the PR description so reviewers can focus on the aggro diff.~~ **Done.** MLX guide removed from NEO-92 branch (lives on `docs/local-mlx-cursor`); history rewritten to NEO-92-only commits.
|
||||||
|
- Nit: `AbilityCastApiTests` still names helpers `LockPrototypeTargetAlphaAsync` while locking `prototype_npc_melee` — harmless legacy naming from NEO-91 migration.
|
||||||
|
- Nit: Plan mentioned an optional thread-safety spot check for `InMemoryThreatStateStore`; acceptable to defer for three fixed prototype ids.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
```bash
|
||||||
|
# NEO-92-focused tests (passed 2026-05-27)
|
||||||
|
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
|
||||||
|
--filter "FullyQualifiedName~Npc|FullyQualifiedName~MoveCommandApiTests.PostMove_ShouldClearAggro|FullyQualifiedName~AbilityCastApiTests.PostAbilityCast_ShouldSetAggro|FullyQualifiedName~AbilityCastApiTests.PostAbilityCast_ShouldReturnCombatResolution_WhenZeroDamage|FullyQualifiedName~CombatTargetFixtureApiTests"
|
||||||
|
|
||||||
|
# Full server suite before merge
|
||||||
|
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
|
||||||
|
```
|
||||||
|
|
||||||
|
Manual: no Bruno changes required (store not on wire). Confirm dev fixture still resets HP + aggro between Bruno runs if exercising combat spines locally.
|
||||||
Loading…
Reference in New Issue