NEO-78: Address code review follow-ups for ability-definitions GET.
Update dependency register and E5M1 backlog, tighten tests and Bruno spot-checks, strike through resolved review items.pull/113/head
parent
8397e74ec1
commit
94b6c67b07
|
|
@ -38,15 +38,6 @@ tests {
|
|||
]);
|
||||
});
|
||||
|
||||
test("frozen prototype four is present", function () {
|
||||
const body = res.getBody();
|
||||
const ids = new Set(body.abilities.map((x) => x.id));
|
||||
expect(ids.has("prototype_burst")).to.equal(true);
|
||||
expect(ids.has("prototype_dash")).to.equal(true);
|
||||
expect(ids.has("prototype_guard")).to.equal(true);
|
||||
expect(ids.has("prototype_pulse")).to.equal(true);
|
||||
});
|
||||
|
||||
test("prototype_pulse row matches catalog", function () {
|
||||
const body = res.getBody();
|
||||
const row = body.abilities.find((x) => x.id === "prototype_pulse");
|
||||
|
|
@ -66,4 +57,22 @@ tests {
|
|||
expect(row.cooldownSeconds).to.equal(5);
|
||||
expect(row.abilityKind).to.equal("attack");
|
||||
});
|
||||
|
||||
test("prototype_guard row matches catalog", function () {
|
||||
const body = res.getBody();
|
||||
const row = body.abilities.find((x) => x.id === "prototype_guard");
|
||||
expect(row).to.be.an("object");
|
||||
expect(row.abilityKind).to.equal("utility");
|
||||
expect(row.baseDamage).to.equal(0);
|
||||
expect(row.cooldownSeconds).to.equal(6);
|
||||
});
|
||||
|
||||
test("prototype_dash row matches catalog", function () {
|
||||
const body = res.getBody();
|
||||
const row = body.abilities.find((x) => x.id === "prototype_dash");
|
||||
expect(row).to.be.an("object");
|
||||
expect(row.abilityKind).to.equal("movement");
|
||||
expect(row.baseDamage).to.equal(0);
|
||||
expect(row.cooldownSeconds).to.equal(4);
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
| E5.M3 | EncounterAndRewardTables | E5.M1, E3.M3, E7.M2 | EncounterDef, RewardTable, EncounterCompleteEvent | Prototype | Planned |
|
||||
| E5.M4 | GroupCombatScaling | E5.M3, E8.M1 | ScalingProfile, PartySizeModifier, CombatDifficultyBand | Pre-production | Planned |
|
||||
|
||||
**E5.M1 note:** Epic 5 **Slice 1** backlog in Linear ([Epic 5 — PvE Combat and Encounter Content](https://linear.app/neon-sprawl/project/epic-5-pve-combat-and-encounter-content-cf3c94c3-5346-40e0-8aaf-281e846f2846)): [NEO-76](https://linear.app/neon-sprawl/issue/NEO-76) → [NEO-86](https://linear.app/neon-sprawl/issue/NEO-86); label **`E5.M1`**. Gig XP on defeat: **E5M1-10** [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44). See [E5M1-prototype-backlog.md](../../plans/E5M1-prototype-backlog.md), [E5_M1_CombatRulesEngine.md](E5_M1_CombatRulesEngine.md). Builds on E1.M4 cast funnel (NEO-28/31/32); extends **`POST …/ability-cast`** with **`CombatResolution`**. **NEO-76 landed:** frozen four-ability catalog + CI ([NEO-76 plan](../../plans/NEO-76-implementation-plan.md)). **NEO-77 landed:** fail-fast server load of `content/abilities/*_abilities.json` ([NEO-77 plan](../../plans/NEO-77-implementation-plan.md)). **NEO-79 landed:** injectable **`IAbilityDefinitionRegistry`** + DI; hotbar/cast use **`TryNormalizeKnown`** ([NEO-79 plan](../../plans/NEO-79-implementation-plan.md)).
|
||||
**E5.M1 note:** Epic 5 **Slice 1** backlog in Linear ([Epic 5 — PvE Combat and Encounter Content](https://linear.app/neon-sprawl/project/epic-5-pve-combat-and-encounter-content-cf3c94c3-5346-40e0-8aaf-281e846f2846)): [NEO-76](https://linear.app/neon-sprawl/issue/NEO-76) → [NEO-86](https://linear.app/neon-sprawl/issue/NEO-86); label **`E5.M1`**. Gig XP on defeat: **E5M1-10** [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44). See [E5M1-prototype-backlog.md](../../plans/E5M1-prototype-backlog.md), [E5_M1_CombatRulesEngine.md](E5_M1_CombatRulesEngine.md). Builds on E1.M4 cast funnel (NEO-28/31/32); extends **`POST …/ability-cast`** with **`CombatResolution`**. **NEO-76 landed:** frozen four-ability catalog + CI ([NEO-76 plan](../../plans/NEO-76-implementation-plan.md)). **NEO-77 landed:** fail-fast server load of `content/abilities/*_abilities.json` ([NEO-77 plan](../../plans/NEO-77-implementation-plan.md)). **NEO-79 landed:** injectable **`IAbilityDefinitionRegistry`** + DI; hotbar/cast use **`TryNormalizeKnown`** ([NEO-79 plan](../../plans/NEO-79-implementation-plan.md)). **NEO-78 landed:** **`GET /game/world/ability-definitions`** — `AbilityDefinitionsWorldApi` + DTOs in `Game/Combat/` ([NEO-78 plan](../../plans/NEO-78-implementation-plan.md)); Bruno `bruno/neon-sprawl-server/ability-definitions/`.
|
||||
|
||||
### Epic 6 — PvP Security
|
||||
|
||||
|
|
|
|||
|
|
@ -154,7 +154,7 @@ Working backlog for **Epic 5 — Slice 1** ([combat rules MVP](../decomposition/
|
|||
|
||||
- `GET /game/world/ability-definitions` → list DTO (`schemaVersion`, `abilities[]` with id, displayName, baseDamage, cooldownSeconds, abilityKind).
|
||||
- API + integration tests (AAA); Bruno folder `bruno/neon-sprawl-server/ability-definitions/`.
|
||||
- `server/README.md`; manual QA stub if needed.
|
||||
- `server/README.md`; no manual QA (server-only per NEO-78 kickoff).
|
||||
|
||||
**Out of scope**
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,8 @@
|
|||
**Scope:** Branch `NEO-78-get-world-ability-definitions` · commits `374b4b5`–`44e0056` vs `origin/main`
|
||||
**Base:** `origin/main`
|
||||
|
||||
**Follow-up:** Code review suggestions addressed (dependency register, E5M1 backlog in-scope line, test count assert, Bruno cleanup + guard/dash spot-checks).
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits**
|
||||
|
|
@ -17,10 +19,10 @@ NEO-78 delivers **E5M1-04**: a read-only **`GET /game/world/ability-definitions`
|
|||
| Document | Result |
|
||||
|----------|--------|
|
||||
| [`docs/plans/NEO-78-implementation-plan.md`](../plans/NEO-78-implementation-plan.md) | **Matches** — kickoff decisions (no manual QA, omit null `abilityKind`, registry-only injection); acceptance checklist complete; reconciliation section reflects landed files. |
|
||||
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-04** | **Matches** — AC checkboxes checked; landed note with plan + Bruno links. In-scope line still mentions “manual QA stub if needed” (kickoff override documented in plan only). |
|
||||
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-04** | **Matches** — AC checkboxes checked; landed note with plan + Bruno links; in-scope line reflects no manual QA (server-only kickoff). |
|
||||
| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Matches** — Status cites NEO-78 landed; **Related implementation slices** HTTP bullet added; freeze table ids align with test assertions. |
|
||||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row includes NEO-78 HTTP projection + Bruno folder. |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E5.M1 note still ends at **NEO-79 landed**; does not yet cite NEO-78 GET (see suggestion). |
|
||||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M1 note includes NEO-78 GET + Bruno folder. |
|
||||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — read-only world catalog; server source of truth; no client mutation in this story. |
|
||||
| NEO-68 / NEO-53 reference implementations | **Matches** — `AbilityDefinitionsWorldApi` + separate DTO file mirror `RecipeDefinitionsWorldApi` / `ItemDefinitionsWorldApi`; integration test shape matches `RecipeDefinitionsWorldApiTests`. |
|
||||
| Full-stack epic decomposition | **Matches** — plan documents no client counterpart; NEO-85 optional enrichment; does not claim prototype slice complete without Godot. |
|
||||
|
|
@ -32,17 +34,17 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Dependency register E5.M1 note** — Extend the **E5.M1 note** in `docs/decomposition/modules/module_dependency_register.md` with **NEO-78 landed:** `GET /game/world/ability-definitions` — `AbilityDefinitionsWorldApi` + DTOs; Bruno `ability-definitions/` (same pattern as NEO-79 review follow-up for registry DI).
|
||||
1. ~~**Dependency register E5.M1 note** — Extend the **E5.M1 note** in `docs/decomposition/modules/module_dependency_register.md` with **NEO-78 landed:** `GET /game/world/ability-definitions` — `AbilityDefinitionsWorldApi` + DTOs; Bruno `ability-definitions/` (same pattern as NEO-79 review follow-up for registry DI).~~ **Done.**
|
||||
|
||||
2. **E5M1 backlog in-scope line** — In **E5M1-04** “In scope”, consider replacing “manual QA stub if needed” with “no manual QA (server-only per NEO-78 kickoff)” so the backlog does not contradict the plan decision.
|
||||
2. ~~**E5M1 backlog in-scope line** — In **E5M1-04** “In scope”, consider replacing “manual QA stub if needed” with “no manual QA (server-only per NEO-78 kickoff)” so the backlog does not contradict the plan decision.~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: Bruno test **“frozen prototype four is present”** is redundant given **“frozen prototype four matches registry id order”** already asserts the exact four ids.
|
||||
- ~~Nit: Bruno test **“frozen prototype four is present”** is redundant given **“frozen prototype four matches registry id order”** already asserts the exact four ids.~~ **Done.** Removed redundant test.
|
||||
|
||||
- Nit: C# integration test infers count **4** via `FrozenFourInIdOrder` length only; an explicit `Assert.Equal(4, body.Abilities.Count)` would match Bruno’s explicit length check (optional — recipe test uses the same frozen-array pattern).
|
||||
- ~~Nit: C# integration test infers count **4** via `FrozenFourInIdOrder` length only; an explicit `Assert.Equal(4, body.Abilities.Count)` would match Bruno’s explicit length check (optional — recipe test uses the same frozen-array pattern).~~ **Done.**
|
||||
|
||||
- Nit: Spot-checks cover **`prototype_pulse`** and **`prototype_burst`** only; **`prototype_guard`** (`utility`) and **`prototype_dash`** (`movement`) kinds are untested in automated/Bruno paths — acceptable per plan scope but easy to add later if desired.
|
||||
- ~~Nit: Spot-checks cover **`prototype_pulse`** and **`prototype_burst`** only; **`prototype_guard`** (`utility`) and **`prototype_dash`** (`movement`) kinds are untested in automated/Bruno paths — acceptable per plan scope but easy to add later if desired.~~ **Done.** Guard/dash spot-checks added to C# test and Bruno.
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ public class AbilityDefinitionsWorldApiTests
|
|||
Assert.NotNull(body);
|
||||
Assert.Equal(AbilityDefinitionsListResponse.CurrentSchemaVersion, body!.SchemaVersion);
|
||||
Assert.NotNull(body.Abilities);
|
||||
Assert.Equal(4, body.Abilities.Count);
|
||||
var ids = body.Abilities.Select(static a => a.Id).ToList();
|
||||
Assert.Equal(FrozenFourInIdOrder, ids);
|
||||
|
||||
|
|
@ -45,5 +46,15 @@ public class AbilityDefinitionsWorldApiTests
|
|||
Assert.Equal(40, burst.BaseDamage);
|
||||
Assert.Equal(5.0, burst.CooldownSeconds);
|
||||
Assert.Equal("attack", burst.AbilityKind);
|
||||
|
||||
var guard = body.Abilities.Single(a => a.Id == "prototype_guard");
|
||||
Assert.Equal("utility", guard.AbilityKind);
|
||||
Assert.Equal(0, guard.BaseDamage);
|
||||
Assert.Equal(6.0, guard.CooldownSeconds);
|
||||
|
||||
var dash = body.Abilities.Single(a => a.Id == "prototype_dash");
|
||||
Assert.Equal("movement", dash.AbilityKind);
|
||||
Assert.Equal(0, dash.BaseDamage);
|
||||
Assert.Equal(4.0, dash.CooldownSeconds);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue